-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-9826] Remove validation from 2fa GET and mask sensitive data #4526
Conversation
New Issues
Fixed Issues
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4526 +/- ##
==========================================
+ Coverage 41.57% 41.58% +0.01%
==========================================
Files 1268 1268
Lines 60091 60110 +19
Branches 5509 5514 +5
==========================================
+ Hits 24981 24999 +18
+ Misses 33950 33948 -2
- Partials 1160 1163 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified the bypass to check closer to the verification in the UserService
to only affect users using the email OTP. This keeps the experience the same for master password users.
I defaulted the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
* remove validation from 2fa GET and mask sensitive data * skip verification check on put email * disable verification on send-email and reenable on put email * validate authenticator on set instead of get * Revert "validate authenticator on set instead of get" This reverts commit 7bf2084. * fix tests * fix more tests * Narrow scope of verify bypass * Defaulted to false on VerifySecretAsync * fix default param value --------- Co-authored-by: Ike Kottlowski <ikottlowski@bitwarden.com> Co-authored-by: Todd Martin <tmartin@bitwarden.com>
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-9826
📔 Objective
Since we can't re-use TOTPs anymore, updating 2fa on accounts without a MP is currently broken. This is because we're validating the TOTP multiple times in the process (once on the GET, and once on the POST).
This removes the validation from 2fa getters, since this information mostly isn't sensitive. For the information that IS sensitive (Duo Client Secret), it masks the value before returning to user.
The TOTP is then only validated when updating the 2fa information.
Note: I also removed the validation from
/get-webauthn-challenge
and/send-email
endpoints since they would invalidate the TOTP too early.📸 Screenshots
See Jira for video
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes