Improve consistency when marking users as fully authenticated after successful multi-factor authenticator setup#8431
Conversation
There was a problem hiding this comment.
This line is only used by the otp_verification_controller when in a confirmation context, so it was moved there. Ideally I would have moved it in #8424
mitchellhenke
force-pushed
the
mitchellhenke/confirmation-contexts-2
branch
from
7edc63f to
3c1b7b6
Compare
|
Looks like legitimate test failures, due to returning the method instead of the proper authentication type. |
There was a problem hiding this comment.
I'm trying to follow how OTP previously would have set user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] to false, or whether it would have (or even if it needed to?). Just flagging since that will be happening now.
There was a problem hiding this comment.
Previously it wouldn't have for phone OTP confirmation. We did do it when confirming other methods though, so this brings it all in line to always set it.
There was a problem hiding this comment.
Also, I assume this may cause some additional analytics to be logged via mark_user_session_authenticated's call to mark_user_session_authenticated_analytics. Should we have specs for that?
There was a problem hiding this comment.
Yep, they were causing a failure, added a few changes to add specs around it.
…uccessful multi-factor authenticator setup changelog: Internal, Authentication, Improve consistency when marking users as fully authenticated after successful multi-factor authenticator setup
mitchellhenke
force-pushed
the
mitchellhenke/confirmation-contexts-2
branch
from
3c1b7b6 to
b7f4c4b
Compare
3 participants
🛠 Summary of changes
When setting up a new authenticator, some of the controllers implement a
mark_user_as_fully_authenticatedmethod when we have a method in the sharedTwoFactorAuthenticatableMethodsconcern. This PR removes the per-controller implementation in favor of usinghandle_valid_verification_for_confirmation_context.