Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(auth): Fix for checking current preferred type before setting the MFAType as enabled to ensure the current preference is not cleared out #2580

Merged
merged 18 commits into from
Oct 11, 2023

Conversation

gpanshu
Copy link
Contributor

@gpanshu gpanshu commented Aug 31, 2023

Fix for checking current preferred type before setting the MFAType as enabled to ensure the current preference is not cleared out

  • PR title and description conform to Pull Request guidelines.

Issue #, if available:

Description of changes: For updating of mfa preferences we were just updating based on what customer had provided without taking into consideration what current preferences were which meant we could get into scenarios where 2 preferences were set to preferred. Hence in this PR I aim to fix that by prefetching the current preferences before updating it.

How did you test these changes?
(Please add a line here how the changes were tested)

Documentation update required?

  • No
  • Yes (Please include a PR link for the documentation update)

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… enabled to ensure the current preference is not cleared out
@gpanshu gpanshu requested a review from a team as a code owner August 31, 2023 21:10
@gpanshu gpanshu enabled auto-merge (squash) September 1, 2023 15:19
@gpanshu gpanshu requested a review from tylerjroach September 1, 2023 18:29
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2023

Codecov Report

Merging #2580 (2bbb89d) into main (45d19c1) will increase coverage by 0.03%.
The diff coverage is 54.54%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main    #2580      +/-   ##
==========================================
+ Coverage   41.72%   41.76%   +0.03%     
==========================================
  Files         900      900              
  Lines       28791    28814      +23     
  Branches     4082     4093      +11     
==========================================
+ Hits        12012    12033      +21     
- Misses      15459    15462       +3     
+ Partials     1320     1319       -1     

@gpanshu gpanshu changed the title fix(auth): Fix for checking current preferred type before setting the MFAType as enabled to ensure the current preference is not cleared ou fix(auth): Fix for checking current preferred type before setting the MFAType as enabled to ensure the current preference is not cleared out Sep 5, 2023
@gpanshu gpanshu requested a review from tylerjroach September 6, 2023 17:24
Copy link
Member

@tylerjroach tylerjroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discovered an issue that can be seen in the described scenario.'

Say TOTP was enabled and preferred.

Next, if a user were to set SMS as enabled and preferred, the update call would also attempt to set TOTP as preferred due to the previous setting.

2 MFA settings can't be preferred at the same time.

@gpanshu gpanshu requested a review from tylerjroach October 6, 2023 16:59
@gpanshu gpanshu merged commit 7cba616 into main Oct 11, 2023
1 check passed
@gpanshu gpanshu deleted the fix-totp-preferredmfatype branch October 11, 2023 19:36
gpanshu added a commit that referenced this pull request Oct 23, 2023
… MFAType as enabled to ensure the current preference is not cleared ou (#2580)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants