-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-22323] Do not show 2FA warning for 2FA setup and login emails #5964
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
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5964 +/- ##
==========================================
+ Coverage 47.72% 47.87% +0.15%
==========================================
Files 1692 1703 +11
Lines 74972 75555 +583
Branches 6758 6798 +40
==========================================
+ Hits 35777 36169 +392
- Misses 37739 37919 +180
- Partials 1456 1467 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Overall looks good, just a few nitpicks.
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 like the idea of explicit call sites, but since that's the case I would like to tighten up the implementation to reduce redundancies.
Suggestions are untested.
Addressed all the feedback, with the exception of removing |
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22323
📔 Objective
When we consolidated all 2FA token emails into one template for maintenance purposes as a part of New Device Verification, we added a line instructing users to "Turn on two-step login".
However, on some of the flows that use this email, users are turning on two-step login (or have already done so), so this instruction doesn't make sense.
This PR does two major things to fix this:
TwoFactorEmailService
, pulling the methods off of theUserService
and into their own service. The methods themselves did not change, with the exception ofResendNewDeviceVerificationEmail
(see below). The interface of this service allows the sending of:ResendNewDeviceVerificationEmail
method. Previously, an email was passed in, and it was the responsibility of theUserService
to look up the email and validate the secret. However, that changes the interface from the other methods, which all accept a (validated)User
. I opted to pull that logic out of theUserService
and instead of moving it to theTwoFactorEmailService
I moved it into theAccountsController
. This mirrors the responsibility of other methods there as well, who validate secrets and call services. I moved the relevant tests there as well.TwoFactorEmailPurpose
enum to pass from theTwoFactorEmailService
into theMailService
to convey why the email was sent, with only theNewDeviceVerification
purpose displaying the 2FA reminder.📸 Screenshots
2FA setup
setup_2fa_email.mov
2FA code entry
send_2fa_email.mov
New device verification code entry
new_device_verification_email.mov
⏰ 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