Skip to content

Refactor handling of successful two-factor phone confirmation #8347

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.

Sign up for GitHub

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

Merged
mitchellhenke merged 1 commit into from
May 8, 2023
Merged

Refactor handling of successful two-factor phone confirmation #8347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 2 additions & 75 deletions app/controllers/concerns/two_factor_authenticatable_methods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def handle_valid_otp_for_context(auth_method:)
if UserSessionContext.authentication_or_reauthentication_context?(context)
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
Copy link
Contributor

@aduth aduth May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mitchellhenke mitchellhenke May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s my plan, yeah

end
end

Expand Down Expand Up @@ -168,13 +168,11 @@ def update_invalid_user
current_user.increment_second_factor_attempts_count!
end

def handle_valid_otp_for_confirmation_context
def handle_valid_verification_for_confirmation_context
user_session[:authn_at] = Time.zone.now
assign_phone
track_mfa_method_added
@next_mfa_setup_path = next_setup_path
reset_second_factor_attempts_count
flash[:success] = t('notices.phone_confirmed')
end

def track_mfa_method_added
Expand All @@ -192,53 +190,10 @@ def handle_valid_otp_for_authentication_context(auth_method:)
reset_second_factor_attempts_count
end

def assign_phone
@updating_existing_number = user_session[:phone_id].present?

if @updating_existing_number && UserSessionContext.confirmation_context?(context)
phone_changed
else
phone_confirmed
end

update_phone_attributes
end

def reset_second_factor_attempts_count
UpdateUser.new(user: current_user, attributes: { second_factor_attempts_count: 0 }).call
end

def phone_changed
create_user_event(:phone_changed)
send_phone_added_email
end

def phone_confirmed
create_user_event(:phone_confirmed)
# If the user has MFA configured, then they are not adding a phone during sign up and are
# instead adding it outside the sign up flow
return unless MfaPolicy.new(current_user).two_factor_enabled?
send_phone_added_email
end

def send_phone_added_email
_event, disavowal_token = create_user_event_with_disavowal(:phone_added, current_user)
current_user.confirmed_email_addresses.each do |email_address|
UserMailer.with(user: current_user, email_address: email_address).
phone_added(disavowal_token: disavowal_token).deliver_now_or_later
end
end

def update_phone_attributes
UpdateUser.new(
user: current_user,
attributes: { phone_id: user_session[:phone_id],
phone: user_session[:unconfirmed_phone],
phone_confirmed_at: Time.zone.now,
otp_make_default_number: selected_otp_make_default_number },
).call
end

def reset_otp_session_data
user_session.delete(:unconfirmed_phone)
user_session[:context] = 'authentication'
Expand All @@ -262,10 +217,6 @@ def mark_user_session_authenticated_analytics(authentication_type)
)
end

def direct_otp_code
current_user.direct_otp if FeatureManagement.prefill_otp_codes?
end

def otp_expiration
return if current_user.direct_otp_sent_at.blank?
current_user.direct_otp_sent_at + TwoFactorAuthenticatable::DIRECT_OTP_VALID_FOR_SECONDS
Expand All @@ -275,33 +226,9 @@ def user_opted_remember_device_cookie
cookies.encrypted[:user_opted_remember_device_preference]
end

def unconfirmed_phone?
user_session[:unconfirmed_phone] && UserSessionContext.confirmation_context?(context)
end

def selected_otp_make_default_number
params&.dig(:otp_make_default_number)
end

def generic_data
{
user_opted_remember_device_cookie: user_opted_remember_device_cookie,
}
end

def display_phone_to_deliver_to
if UserSessionContext.authentication_or_reauthentication_context?(context)
phone_configuration.masked_phone
else
user_session[:unconfirmed_phone]
end
end

def confirmation_for_add_phone?
UserSessionContext.confirmation_context?(context) && user_fully_authenticated?
end

def phone_configuration
MfaContext.new(current_user).phone_configuration(user_session[:phone_id])
end
end
73 changes: 73 additions & 0 deletions app/controllers/two_factor_authentication/otp_verification_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def create
result = otp_verification_form.submit
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])
Copy link
Contributor

@aduth aduth May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to pull handle_valid_otp into this class? And then maybe consolidate this to a single "handle_valid_*" call?

Copy link
Contributor Author

@mitchellhenke mitchellhenke May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
end

and 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.

else
handle_invalid_otp(context: context, type: 'otp')
Expand All @@ -29,6 +30,11 @@ def create

private

def handle_valid_confirmation_otp
assign_phone
flash[:success] = t('notices.phone_confirmed')
end

def otp_verification_form
OtpVerificationForm.new(current_user, sanitized_otp_code)
end
Expand Down Expand Up @@ -162,8 +168,75 @@ def phone_view_data
}.merge(generic_data)
end

def display_phone_to_deliver_to
if UserSessionContext.authentication_or_reauthentication_context?(context)
phone_configuration.masked_phone
else
user_session[:unconfirmed_phone]
end
end

def unconfirmed_phone?
user_session[:unconfirmed_phone] && UserSessionContext.confirmation_context?(context)
end

def confirmation_for_add_phone?
UserSessionContext.confirmation_context?(context) && user_fully_authenticated?
end

def check_sp_required_mfa
check_sp_required_mfa_bypass(auth_method: params[:otp_delivery_preference])
end

def assign_phone
@updating_existing_number = user_session[:phone_id].present?

if @updating_existing_number
phone_changed
else
phone_confirmed
end

update_phone_attributes
end

def update_phone_attributes
UpdateUser.new(
user: current_user,
attributes: { phone_id: user_session[:phone_id],
phone: user_session[:unconfirmed_phone],
phone_confirmed_at: Time.zone.now,
otp_make_default_number: selected_otp_make_default_number },
).call
end

def phone_changed
create_user_event(:phone_changed)
send_phone_added_email
end

def phone_confirmed
create_user_event(:phone_confirmed)
# If the user has MFA configured, then they are not adding a phone during sign up and are
# instead adding it outside the sign up flow
return unless MfaPolicy.new(current_user).two_factor_enabled?
send_phone_added_email
end

def send_phone_added_email
_event, disavowal_token = create_user_event_with_disavowal(:phone_added, current_user)
current_user.confirmed_email_addresses.each do |email_address|
UserMailer.with(user: current_user, email_address: email_address).
phone_added(disavowal_token: disavowal_token).deliver_now_or_later
end
end

def selected_otp_make_default_number
params&.dig(:otp_make_default_number)
end

def direct_otp_code
current_user.direct_otp if FeatureManagement.prefill_otp_codes?
end
end
end