Skip to content

LG-14052: Convert mismatched WebAuthn platform attachment#11788

Merged
aduth merged 1 commit intomainfrom
aduth-lg-14052-webauthn-setup-mismatch
Jan 28, 2025
Merged

LG-14052: Convert mismatched WebAuthn platform attachment#11788
aduth merged 1 commit intomainfrom
aduth-lg-14052-webauthn-setup-mismatch

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented Jan 23, 2025

🎫 Ticket

LG-14052

🛠 Summary of changes

Updates Security Key and Face or Touch Unlock setup to automatically convert to the other if the reported transports are inconsistent with the expected platform attachment.

As outlined in the Testing Plan, despite hints that we provide to the browser to try to ensure that the user sets up a WebAuthn credential with the intended platform attachment, it is still often possible for a user to set up an authenticator of the other type (e.g. Chrome's "Save another way"). In these scenarios, converting the credential to the inferred type will ensure that the user has a better sign-in experience and that our usage tracking is more accurate to the actual type of authenticator used. Previous investigation found that a majority of "Security Key" enrollments are, in-fact, platform authenticators.

This is the second of two pull requests, using routes implemented in #11795. Those changes must be fully deployed to production before this can be merged.

📜 Testing Plan

Verify mismatched behavior displays confirmation prompt as expected:

Setting up Security Key using a platform authenticator:

  1. Prerequisite: Have a mobile phone capable of setting up passkeys (iPhone with iCloud Keychain enabled, or Android)
  2. On desktop, go to http://localhost:3000
  3. Sign in or create a new account†
  4. When presented the opportunity, add a Security Key
    • From the account dashboard for an existing account, click "Add security key" in the sidebar
    • From the MFA selection screen for a new account, choose "Security key" as one of your selected option(s) and "Continue"
  5. Enter a nickname for the security key
  6. Click "Set up security key"
  7. As permitted by your browser, navigate the browser dialog to set up using a platform authenticator (e.g. phone)
    • In Chrome, click "Save another way", click "Use a different phone or tablet", and scan the displayed QR code using your phone
  8. Observe that you're shown the screen "Face or touch unlock detected"
  9. Click either "Continue" or "Undo"
  10. Observe that...
  • ...if you click "Continue", you proceed to the next logical step (account page if set up from account page, or account page if set up as second MFA method, or 2nd MFA requirement if set up as sole MFA in account creation, or consent screen if set up as second MFA method while signing in to partner)
  • ...if you click "Undo", you're returned to the Security Key setup screen. You should be able to either set up again or cancel and return to the preceding step (MFA selection, account page)

Setting up Security Key using a platform authenticator:

The steps below are currently hypothetical, but my own testing reveals that it is not possible to complete Step 5 in tested supported browsers.

  1. Go to http://localhost:3000
  2. Sign in or create a new account†
  3. When presented the opportunity, add Face or Touch unlock
    • From the account dashboard for an existing account, click "Add face or touch unlock" in the sidebar
    • From the MFA selection screen for a new account, choose "Face or touch unlock" as one of your selected option(s) and "Continue"
  4. Click "Continue"
  5. As permitted by your browser, navigate the browser dialog to set up using a security key
  6. Observe that you're shown the screen "Security key detected"
  7. Click either "Continue" or "Undo"
  8. Observe that...
    • ...if you click "Continue", you proceed to the next logical step (account page if set up from account page, or account page if set up as second MFA method, or consent screen if set up as second MFA method while signing in to partner)
    • ...if you click "Undo", you're returned to the Face or Touch Unlock setup screen. You should be able to either set up again or cancel and return to the preceding step (MFA selection, account page)

† Creating a new account can be a useful test case, since the application currently has some assumptions about not permitting a user to delete their only MFA, but the process of "Undo"-ing a mismatched WebAuthn credential requires that we allow a user to delete their only MFA during account creation.

👀 Screenshots

Language Face or Touch Unlock Security Key
English ft-mismatch-en sk-mismatch-en
Spanish ft-mismatch-es sk-mismatch-es
French ft-mismatch-fr sk-mismatch-fr
Chinese ft-mismatch-zh sk-mismatch-zh

Comment on lines 38 to 40
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO it's clearer to have one manually defined method than two auto-genned ones

Suggested change
attr_reader :skip_multiple_mfa_validation
alias_method :skip_multiple_mfa_validation?, :skip_multiple_mfa_validation
def skip_multiple_mfa_validation?
!!@skip_multiple_mfa_validation
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm, I kinda like the attr_reader approach, though I do wish it were possible to alias in one go (would love something like attr_reader :skip_multiple_mfa_validation, as: :skip_multiple_mfa_validation?).

I recently discovered Ruby 3 "endless method" syntax, maybe this is a good use-case?

Suggested change
attr_reader :skip_multiple_mfa_validation
alias_method :skip_multiple_mfa_validation?, :skip_multiple_mfa_validation
def skip_multiple_mfa_validation? = @skip_multiple_mfa_validation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not used to the endless methods yet so if it were up to me, I'd stick with my suggestion! But even the PR as-is is probably fine, this is not that crucial to me

@aduth
Copy link
Copy Markdown
Contributor Author

aduth commented Jan 24, 2025

Marking ready for review after merge of #11795, but this cannot be merged until that other pull request is fully deployed to production.

@aduth aduth marked this pull request as ready for review January 24, 2025 16:49
@aduth aduth requested a review from a team January 24, 2025 16:49
@aduth aduth force-pushed the aduth-lg-14052-webauthn-setup-mismatch branch from 97ae0c8 to 582e74f Compare January 27, 2025 16:14
Copy link
Copy Markdown
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

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

Tested locally and behaves as expected.
Changes LGTM 👍

changelog: User-Facing Improvements, Multi-Factor Authentication, Convert Security Key to Face or Touch Unlock when detected as platform authenticator
@aduth aduth force-pushed the aduth-lg-14052-webauthn-setup-mismatch branch from 3e8097b to c6787e4 Compare January 28, 2025 15:15
@aduth aduth merged commit ccb7d6d into main Jan 28, 2025
@aduth aduth deleted the aduth-lg-14052-webauthn-setup-mismatch branch January 28, 2025 15:33
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