Skip to content

Remove after_otp_verification_confirmation_url#8498

Merged
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/haha-yes
May 30, 2023
Merged

Remove after_otp_verification_confirmation_url#8498
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/haha-yes

Conversation

@mitchellhenke
Copy link
Contributor

🛠 Summary of changes

A little bit of cleanup following #8392. The after_otp_verification_confirmation_url function only behaves differently in the OtpVerificationController when in the confirmation context due to the prior work. The @next_mfa_setup_path and @updating_existing_number instance variables are only set there, which is what results in the different behavior.

This PR removes those instance variables and replaces the after_otp_verification_confirmation_url with the default after_sign_in_path_for(current_user) in the other controllers that used it. This should not result in a change in behavior and hopefully results in a little bit less abstraction.

Mitchell Henke added 2 commits May 26, 2023 14:57
changelog: Internal, Refactor, Remove after_otp_verification_confirmation_url
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

mark_user_session_authenticated(:device_remembered)
handle_valid_remember_device_analytics(cookie_created_at: remember_device_cookie.created_at)
redirect_to after_otp_verification_confirmation_url unless reauthn?
redirect_to after_sign_in_path_for(current_user) unless reauthn?
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that since it's defined on application_controller, the after_sign_in_path_for method would already have access to current_user and not need it passed as an arg, but then looking at it's source, the _user is unused!

def after_sign_in_path_for(_user)

Looks like the method comes from devise but what if we redefined it to have a default value for user and then just stopped passing current_user everywhere to simplify?

Suggested change
redirect_to after_sign_in_path_for(current_user) unless reauthn?
redirect_to after_sign_in_path_for unless reauthn?
def after_sign_in_path_for(_scope = nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be good to try this, but probably better tackled in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm!

auth_method: params[:otp_delivery_preference],
)
flash[:success] = t('notices.phone_confirmed')
redirect_to next_setup_path || after_mfa_setup_path
Copy link
Contributor

Choose a reason for hiding this comment

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

How was the second half of this (|| after_mfa_setup_path) happening in the previous code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was happening via a combination of behavior in these methods:

def after_sign_in_path_for(_user)
service_provider_mfa_setup_url ||
add_piv_cac_setup_url ||
fix_broken_personal_key_url ||
user_session.delete(:stored_location) ||
sp_session_request_url_with_updated_params ||
signed_in_url
end
def signed_in_url
return user_two_factor_authentication_url unless user_fully_authenticated?
return reactivate_account_url if user_needs_to_reactivate_account?
return url_for_pending_profile_reason if user_has_pending_profile?
return backup_code_reminder_url if user_needs_backup_code_reminder?
account_url
end

I think the behavior of the change is effectively zero. The order of checking for redirects is slightly different, but I don't think it could result in a different set of requests for a user after doing some testing.

The above change brings this controller in line with the other setup controllers in using redirect_to next_setup_path || after_mfa_setup_path (TOTP, Backup Codes, WebAuthn)

@mitchellhenke mitchellhenke merged commit 11c379c into main May 30, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/haha-yes branch May 30, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants