Skip to content

LG-11433: FT unlock redirect to options page if eligible only#9835

Merged
mdiarra3 merged 34 commits intomainfrom
LG-11433-default-ft-unlock-only-if-eligible
Jan 19, 2024
Merged

LG-11433: FT unlock redirect to options page if eligible only#9835
mdiarra3 merged 34 commits intomainfrom
LG-11433-default-ft-unlock-only-if-eligible

Conversation

@mdiarra3
Copy link
Contributor

🎫 Ticket

LG-11433: Redirect if ineligible for F/T Unlock

🛠 Summary of changes

This adds an additional check when the user is showing F/T unlock first to be redirected to the options page.

@mdiarra3 mdiarra3 requested a review from a team January 4, 2024 14:32
@mdiarra3 mdiarra3 marked this pull request as ready for review January 4, 2024 14:32
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: There are many ways that a user might be prompted for MFA, such as reauthentication. Can we always rely on the session value being present in those other scenarios? My thinking is yes since someone's session should always begin from the sign-in page, but I wanted to ask to double-check.

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 should for sure, but I dont know if in reauthenthication we want this? Maybe we can remove it when its directed.

)
end

def mock_setup_eligible_user_device
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Should we just update the default user agent string for mobile to be able to avoid this?
  • I'd suggest incorporating "webauthn" somewhere in the name of the method, even if it's in WebauthnHelper, to clarify at call-sites, and for consistency with other methods in this file.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looking good, couple build failures look related

password = user.password
allow(UserMailer).to receive(:new_device_sign_in).and_call_original
visit new_user_session_path
set_hidden_field('platform_authenticator_available', 'true')
Copy link
Contributor

Choose a reason for hiding this comment

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

Per some of the build failures, maybe we should set this as the default behavior for signing in in the existing spec helper(s)?

@mdiarra3 mdiarra3 changed the title changelog: User-Facing Improvements, Authentication, MFA redirect for F/T unlock LG-11433: FT unlock redirect to options page if eligible only Jan 10, 2024
@mdiarra3 mdiarra3 requested a review from aduth January 11, 2024 13:18
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Couple minor comments, but LGTM overall 👍

@mdiarra3 mdiarra3 merged commit dad3a29 into main Jan 19, 2024
@mdiarra3 mdiarra3 deleted the LG-11433-default-ft-unlock-only-if-eligible branch January 19, 2024 14:07
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