Refactor handling of successful two-factor phone confirmation#8347
Conversation
changelog: Internal, Two-Factor Authentication, Refactor handling of successful two-factor phone confirmation
| handle_valid_otp_for_authentication_context(auth_method: auth_method) | ||
| elsif UserSessionContext.confirmation_context?(context) | ||
| handle_valid_otp_for_confirmation_context | ||
| handle_valid_verification_for_confirmation_context |
There was a problem hiding this comment.
Just to clarify my understanding, currently this only happens for phones, but eventually we'd want this method to be called for any MFA in the confirmation context?
There was a problem hiding this comment.
That’s my plan, yeah
| post_analytics(result) | ||
| if result.success? | ||
| handle_valid_confirmation_otp if UserSessionContext.confirmation_context?(context) | ||
| handle_valid_otp(next_url: nil, auth_method: params[:otp_delivery_preference]) |
There was a problem hiding this comment.
Would it make sense to pull handle_valid_otp into this class? And then maybe consolidate this to a single "handle_valid_*" call?
There was a problem hiding this comment.
session context really only exists for phone OTPs, so I'm leaning towards making the handle_valid_confirmation/handle_valid_authentication calls explicit, and the phone controller can have the conditional checks with UserSessionContext.confirmation_context?, etc.
I'm ultimately hoping this controller will be something like:
if UserSessionContext.confirmation_context?(context)
handle_valid_confirmation_otp
handle_valid_verification_for_confirmation
else
handle_valid_verification_for_authentication
endand the remaining auth controllers can call handle_valid_verification_for_confirmation / handle_valid_verification_for_authentication without using session context since the controller action should be enough to know the difference.
mitchellhenke
deleted the
mitchellhenke/handle-valid-verification-consolidation
branch
2 participants
🛠 Summary of changes
A bit of a followup to #8178 and #8316 (specifically this comment) to start consolidating how we implement
handle_valid_otp. We have repeated ourselves a few times with methods likemark_user_as_fully_authenticated, but there's a significant amount of phone-specific functionality now. This PR starts to separate those, and aims to eventually consolidate intohandle_valid_2FAor similar since not all 2FAs have an OTP (which makeshandle_valid_otpkind of misleading).