Skip to content

Remove WebAuthn support detection, error page#8764

Merged
aduth merged 1 commit intomainfrom
aduth-rm-webauthn-not-enabled
Jul 13, 2023
Merged

Remove WebAuthn support detection, error page#8764
aduth merged 1 commit intomainfrom
aduth-rm-webauthn-not-enabled

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 12, 2023

🛠 Summary of changes

Removes all logic associated with checking the user browser for WebAuthn support, based on the assumption that our browser support commitments are such that WebAuthn would be expected to be available in all supported browsers.

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/credentials#browser_compatibility

📜 Testing Plan

Verify no regressions in signing in and MFA'ing to an account with a Security Key or Face or Touch Unlock configured.

Base automatically changed from aduth-lg-10233-ft-sign-in-revisions to main July 12, 2023 15:36
changelog: Upcoming Features, Face or Touch Unlock, Remove unnecessary feature detection for WebAuthn
@aduth aduth force-pushed the aduth-rm-webauthn-not-enabled branch from d331998 to 6e5ba06 Compare July 12, 2023 15:37
@aduth aduth marked this pull request as ready for review July 12, 2023 15:37
@aduth aduth requested a review from a team July 12, 2023 15:37
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

@aduth aduth merged commit 68adb25 into main Jul 13, 2023
@aduth aduth deleted the aduth-rm-webauthn-not-enabled branch July 13, 2023 13:18
get '/login/two_factor/piv_cac' => 'two_factor_authentication/piv_cac_verification#show'
get '/login/two_factor/piv_cac/present_piv_cac' => 'two_factor_authentication/piv_cac_verification#redirect_to_piv_cac_service'
get '/login/two_factor/webauthn' => 'two_factor_authentication/webauthn_verification#show'
get '/login/two_factor/webauthn_error' => 'two_factor_authentication/webauthn_verification#error'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe should have removed this in a separate deployment, to avoid 50/50 issues. Pretty low risk though, since this error would only be seen by people using unsupported browsers. I'll check the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After checking logs, there's some traffic to this page still, but reasonably low overall.

Also worth noting the actual experience for someone on an unsupported browser should be marginally improved by these changes, since they'll still see an error message when trying to authenticate. Unlike before, they'll now have the option to select another MFA method on the unsupported device, which was not possible previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be on the safe side: #8773

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