-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LG-12571 Specify "hints" for WebAuthn security key enrollment #10382
Conversation
@@ -56,6 +56,7 @@ function webauthn() { | |||
.filter(Boolean), | |||
), | |||
authenticatorAttachment: platformAuthenticator ? 'platform' : 'cross-platform', | |||
publicKeyCredentialHints: platformAuthenticator ? 'client-device' : 'security-key', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should support "hybrid" for platform authenticator enrollments, like using a smartphone QR code to complete MFA setup initiated on a computer. I'd think we should either leave the hints unspecified for platform authenticators, or provide an array including both client-device
and hybrid
(in order of preference).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 'hybrid' into the acceptable options.
@@ -37,6 +39,10 @@ interface EnrollResult { | |||
transports?: string[]; | |||
} | |||
|
|||
interface AuthenticatorSelectionCriteriaWithHints extends AuthenticatorSelectionCriteria { | |||
publicKeyCredentialHints?: 'client-device' | 'security-key'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The property of
publicKey
is calledhints
, notpublicKeyCredentialHints
- The value is an array of strings, not a string
'hybrid'
is a validhints
string value
Reference: https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialcreationoptions-hints
publicKeyCredentialHints?: 'client-device' | 'security-key'; | |
hints?: Array<'client-device' | 'security-key' | 'hybrid'>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated value to array of strings and set property name to 'hints'
@@ -23,6 +23,8 @@ interface EnrollOptions { | |||
excludeCredentials: PublicKeyCredentialDescriptor[]; | |||
|
|||
authenticatorAttachment?: AuthenticatorAttachment; | |||
|
|||
publicKeyCredentialHints?: 'client-device' | 'security-key'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this 'client-device' | 'security-key'
union is used in two spots, should we name it & typedef it?
typedef PublicKeyCredentialType = 'client-device' | 'security-key';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea. Could even use an enum and call it the same as in the spec, PublicKeyCredentialHints
https://www.w3.org/TR/webauthn-3/#enumdef-publickeycredentialhints
https://www.typescriptlang.org/docs/handbook/enums.html#string-enums
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have those cast in a Type now. I'm not entirely clear on how to leverage an enum there but can spend a little more time on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
🎫 Ticket
LG-12571
🛠 Summary of changes
When enrolling a non-platform webauthn device it passes the value 'security-key' with the
authenticatorSelection
object.Platform devices will have 'client-device' added.
https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialhints-client-device
📜 Testing Plan
Test for enroll-webauthn-device should pass.
Setup should work as before without any issues.