Skip to content

LG-11399: Show Face/Touch message for screen lock error#9564

Merged
aduth merged 3 commits intomainfrom
aduth-lg-11399-screen-lock-error
Nov 21, 2023
Merged

LG-11399: Show Face/Touch message for screen lock error#9564
aduth merged 3 commits intomainfrom
aduth-lg-11399-screen-lock-error

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Nov 8, 2023

🎫 Ticket

LG-11399, and somewhat related to LG-11135 in terms of form error refactoring

🛠 Summary of changes

Updates error handling for Face or Touch Unlock sign-in to show a specific error message when the browser produces an error informing us that the user is attempting to sign in on a device ineligible due to lack of screen lock for the userVerification requirement of authentication.

Note that this error is specific to Chrome, is not a behavior described anywhere in the relevant WebAuthn specification, and could change at any point. See the related Slack discussion on this point, but the consensus is that we should still want to target this specific error type, and that as long as we continue to log to our frontend error logger for unexpected errors, we should be made aware if the messaging changes over time.

This is an error we see frequently logged in our frontend error logger, and the changes here are meant to address it directly and prevent further logging.

Draft: This pull request currently contains a lot of refactoring and proposals which may make sense to split off to their own pull request, but compiled here as an initial MVP. Edit: See separate pull requests #9613, #9614, #9572

📜 Testing Plan

I've been using the following diff to emulate the behavior locally:

diff --git a/app/javascript/packages/webauthn/verify-webauthn-device.ts b/app/javascript/packages/webauthn/verify-webauthn-device.ts
index ee39c2016..9f42ad140 100644
--- a/app/javascript/packages/webauthn/verify-webauthn-device.ts
+++ b/app/javascript/packages/webauthn/verify-webauthn-device.ts
@@ -45,6 +45,11 @@ async function verifyWebauthnDevice({
 
   const response = credential.response as AuthenticatorAssertionResponse;
 
+  throw new DOMException(
+    'The specified `userVerification` requirement cannot be fulfilled by this device unless the device is secured with a screen lock.',
+    'NotSupportedError',
+  );
+
   return {
     credentialId: arrayBufferToBase64(credential.rawId),
     authenticatorData: arrayBufferToBase64(response.authenticatorData),

Verify that you're presented with the relevant error message when signing in with Face or Touch Unlock.

👀 Screenshots

With another method available:

image

Without another method available:

image

@aduth aduth marked this pull request as ready for review November 20, 2023 14:00
@aduth aduth requested a review from a team November 20, 2023 14:01
@aduth
Copy link
Contributor Author

aduth commented Nov 20, 2023

I'm marking this ready for review since it's code-complete. I'm still awaiting final decision on the text of the error message before I get translations, and will plan to swap that in once it's available.

See related Slack discussion: https://gsa-tts.slack.com/archives/C01710KMYUB/p1699390502877849

changelog: User-Facing Improvements, Face or Touch Unlock, Show specific error message when authenticating on a device without screen lock
Co-Authored-By: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth force-pushed the aduth-lg-11399-screen-lock-error branch from fd5f305 to 0fbd098 Compare November 20, 2023 19:05
if user_has_other_authentication_method?
t(
'two_factor_authentication.webauthn_error.screen_lock_other_mfa',
link: link_to(
Copy link
Contributor

Choose a reason for hiding this comment

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

this probably should have _html on it? the only exception is when strings get written to flash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test got angry at me about it. I tested without the _html and it seemed to work fine for me.

I think the alternative would be to apply _html to both the "parent" and "child" strings like what's done above with connect_html, but I'd opted to avoid it since it seemed to work without.

Copy link
Contributor

Choose a reason for hiding this comment

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

what's rendering this error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's rendering this error message?

I'm not sure what you mean by this. Do you mean how to reproduce the error message being shown?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant, where is the code that renders the error in a template.

I did some spelunking and found that the error gets written to the flash:

which explains why it renders correctly without being named _html because all flash messages get .html_safe 😭

<%= render AlertComponent.new(type:, message: message.html_safe, class: 'margin-bottom-4') %>

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'm not sure I'd realized that. I'd be interested in seeing what it might take to move away from that, though definitely a separate effort. I'll see if it also works to just add _html to both the "parent" and "child" and that would be future-proof at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the suffix in 05bc0f0 . I did a little bit of testing of what it'd look like to remove html_safe to verify that it'd work, but it looks like something is dropping the HTML safety of the suffixed string somewhere along the way. Something to look at later if / when we try to drop the auto-safety.

@aduth aduth merged commit 06aeff4 into main Nov 21, 2023
@aduth aduth deleted the aduth-lg-11399-screen-lock-error branch November 21, 2023 13:48
mitchellhenke pushed a commit that referenced this pull request Nov 21, 2023
* LG-11399: Show Face/Touch message for screen lock error

changelog: User-Facing Improvements, Face or Touch Unlock, Show specific error message when authenticating on a device without screen lock
Co-Authored-By: Zach Margolis <zachmargolis@users.noreply.github.com>

* Remove unnecessary _html suffix

* Add _html suffix for HTML interpolation

---------

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
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.

2 participants