Skip to content

LG-10290: Fix F/T unlock cancel sign-in redirect#8738

Merged
aduth merged 2 commits intomainfrom
aduth-lg-10290-ft-cancel
Jul 10, 2023
Merged

LG-10290: Fix F/T unlock cancel sign-in redirect#8738
aduth merged 2 commits intomainfrom
aduth-lg-10290-ft-cancel

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 10, 2023

🎫 Ticket

LG-10290

🛠 Summary of changes

Fixes an issue introduced in #8723 where a user would be redirected to the Security Key version of the WebAuthn MFA verification page if they cancelled the browser dialog for face or touch unlock.

📜 Testing Plan

  1. Sign in to an account with Face or Touch Unlock configured
  2. Click "Use face or touch unlock" when prompted for MFA
  3. Click "Cancel" in the browser dialog

Before: User is redirected to an error page and prompted to "Connect your security key"
After: Error state reflects cancelled Face or Touch Unlock MFA

changelog: Upcoming Features, Face or Touch Unlock, Fix redirect for cancelled sign-in
@aduth aduth requested a review from a team July 10, 2023 15:23
Comment on lines -9 to -10
const webauthnPlatformRequested =
webauthnInProgressContainer.dataset.platformAuthenticatorRequested === 'true';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explanation for the fix:

This data- property was removed in #8723, and was meant to be substituted by hard-coding the value into the form...

Comment on lines -44 to -45
(document.getElementById('platform') as HTMLInputElement).value =
String(webauthnPlatformRequested);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

...but since we were still assigning a value here from the non-existent data- attribute, the form value was being overridden.

The solution is to just not set the value when the error happens, so that the form value remains undisturbed.

@aduth
Copy link
Contributor Author

aduth commented Jul 10, 2023

We don't have good test coverage for this, but I might plan to merge this anyways, with a quick follow-on with one or the other of...

  • An implementation of LG-10225 (i.e. proper WebAuthn stubbing for JavaScript-enabled drivers in RSpec)
  • A refactor of webauthn-authenticate.ts pack as a custom element lg-webauthn-authenticate-button, which could be tested.

@aduth aduth marked this pull request as ready for review July 10, 2023 17:09
@aduth aduth merged commit a1fcfbf into main Jul 10, 2023
@aduth aduth deleted the aduth-lg-10290-ft-cancel branch July 10, 2023 17:10
This was referenced Jul 10, 2023
@aduth
Copy link
Contributor Author

aduth commented Jul 12, 2023

  • An implementation of LG-10225 (i.e. proper WebAuthn stubbing for JavaScript-enabled drivers in RSpec)

See: #8761

  • A refactor of webauthn-authenticate.ts pack as a custom element lg-webauthn-authenticate-button, which could be tested.

See: #8753

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