-
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-15614] Allow Users to opt out of new device verification #5176
Conversation
…vices column in [dbo].[User].
…r has opted out of device verification. Added endpoint to AccountsController.cs to allow editing of new User.VerifyDevices property.
…t. Removed Anon attribute from the POST account/verify-devices endpoint.
…fyDevices. Added update to verify email to the new device verification flow.
New Issues
Fixed Issues
|
…onSignUpCommand that were failing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5176 +/- ##
=======================================
Coverage ? 43.34%
=======================================
Files ? 1459
Lines ? 66510
Branches ? 6082
=======================================
Hits ? 28831
Misses ? 36387
Partials ? 1292 ☔ 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.
Looks good! And thank you for the good tests! Approving but wouldn't mind feedback on the question about a global const. Nothing blocking.
|
||
if (!await _userService.VerifySecretAsync(user, request.Secret)) | ||
{ | ||
await Task.Delay(2000); |
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.
why do we delay for this specific duration? Could we turn this into a global const that we can reference? I imagine we do this in other places too.
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.
We do it a lot in other places but I'm only perpetuating the tradition. Historically I believe this has been used to reduce the efficacy of bot brute forcing. But I'm not sure what our current bot mitigation posture is.
cc: @Hinton
@@ -128,6 +130,8 @@ public async Task SignUp_SM_Passes(PlanType planType, OrganizationSignup signup, | |||
signup.PaymentMethodType = PaymentMethodType.Card; | |||
signup.PremiumAccessAddon = false; | |||
signup.IsFromSecretsManagerTrial = false; | |||
signup.IsFromProvider = false; | |||
|
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.
Nitpick - extra line.
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.
lgtm
🎟️ Tracking
PM-15614
📔 Objective
Some users would like to be able to turn off the new device verification feature. This PR is to enable that decision.
However, turning this off will reduce overall security of your account if you do not already have 2FA enabled.
📸 Screenshots
User is able to login when
dbo.User.VerifyDevices
is set to false (0)PRRh7o4c8a.mp4
Transcript
User tries to login and is met with New Device Verification prompt.
dbo.User.VerifyDevices
is changed to false (0); user attempts to login again and are allowed to authenticate this time.dbo.User.VerifyDevices
is set back to true (1); User tries to authenticate again and is successful again because the device has been saved as a knowndevice.⏰ 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