Skip to content

Cache phone_configuration queries during OTP authentication #6998

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
Sep 21, 2022
Merged

Cache phone_configuration queries during OTP authentication #6998

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
16 changes: 11 additions & 5 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 @@ -36,10 +36,11 @@ def redirect_if_blank_phone
end

def confirm_multiple_factors_enabled
return if UserSessionContext.confirmation_context?(context) || phone_enabled?
phone_enabled = phone_enabled?
return if UserSessionContext.confirmation_context?(context) || phone_enabled
Comment on lines +39 to +40
Copy link
Copy Markdown
Contributor

@aduth aduth Sep 21, 2022

Choose a reason for hiding this comment

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

I dunno if short-circuiting would have saved us anything previously, but by assigning this variable, we're no longer bailing based on the result of confirmation_context?, which I assume would be a pretty trivial check.

If we cared for short-circuiting, I would have expected something like...

Suggested change
phone_enabled = phone_enabled?
return if UserSessionContext.confirmation_context?(context) || phone_enabled
return if UserSessionContext.confirmation_context?(context)
phone_enabled = phone_enabled?
return if phone_enabled

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke Sep 21, 2022

Choose a reason for hiding this comment

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

ahhhh, shoot, thank you

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke Sep 21, 2022

Choose a reason for hiding this comment

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

#7002


if MfaPolicy.new(current_user).two_factor_enabled? &&
!phone_enabled? && user_signed_in?
!phone_enabled && user_signed_in?
return redirect_to user_two_factor_authentication_url
end

Expand Down Expand Up @@ -67,10 +68,16 @@ def confirm_voice_capability
end

def phone
MfaContext.new(current_user).phone_configuration(user_session[:phone_id])&.phone ||
phone_configuration&.phone ||
user_session[:unconfirmed_phone]
end

def phone_configuration
return @phone_configuration if defined?(@phone_configuration)
@phone_configuration =
MfaContext.new(current_user).phone_configuration(user_session[:phone_id])
end

def sanitized_otp_code
form_params[:code].to_s.strip
end
Expand Down Expand Up @@ -112,8 +119,7 @@ def analytics_properties
area_code: parsed_phone.area_code,
country_code: parsed_phone.country,
phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164),
phone_configuration_id: user_session[:phone_id] ||
current_user.default_phone_configuration&.id,
phone_configuration_id: phone_configuration&.id,
in_multi_mfa_selection_flow: in_multi_mfa_selection_flow?,
enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count,
}
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/users/two_factor_authentication_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ def phone_enabled?
end

def phone_configuration
MfaContext.new(current_user).phone_configuration(user_session[:phone_id])
return @phone_configuration if defined?(@phone_configuration)
@phone_configuration =
MfaContext.new(current_user).phone_configuration(user_session[:phone_id])
end

def validate_otp_delivery_preference_and_send_code
Expand Down