-
Notifications
You must be signed in to change notification settings - Fork 47
feat: add auto complete ui for AM-33773 #571
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import { CallbackType } from '../auth/enums'; | |
| import type HiddenValueCallback from '../fr-auth/callbacks/hidden-value-callback'; | ||
| import type MetadataCallback from '../fr-auth/callbacks/metadata-callback'; | ||
| import type FRStep from '../fr-auth/fr-step'; | ||
| import { FRLogger } from '../util/logger'; | ||
| import { WebAuthnOutcome, WebAuthnOutcomeType, WebAuthnStepType } from './enums'; | ||
| import { | ||
| arrayBufferToString, | ||
|
|
@@ -30,6 +31,7 @@ import type { | |
| } from './interfaces'; | ||
| import type TextOutputCallback from '../fr-auth/callbacks/text-output-callback'; | ||
| import { parseWebAuthnAuthenticateText, parseWebAuthnRegisterText } from './script-parser'; | ||
| import { withTimeout } from '../util/timeout'; | ||
|
|
||
| // <clientdata>::<attestation>::<publickeyCredential>::<DeviceName> | ||
| type OutcomeWithName< | ||
|
|
@@ -44,6 +46,8 @@ type OutcomeWithName< | |
| type WebAuthnMetadata = WebAuthnAuthenticationMetadata | WebAuthnRegistrationMetadata; | ||
| // Script-based WebAuthn | ||
| type WebAuthnTextOutput = WebAuthnTextOutputRegistration; | ||
| const ONE_SECOND = 1000; | ||
|
|
||
| /** | ||
| * Utility for integrating a web browser's WebAuthn API. | ||
| * | ||
|
|
@@ -60,6 +64,24 @@ type WebAuthnTextOutput = WebAuthnTextOutputRegistration; | |
| * await FRWebAuthn.authenticate(step); | ||
| * } | ||
| * ``` | ||
| * | ||
| * Conditional UI (Autofill) Support: | ||
| * | ||
| * ```js | ||
| * // Check if browser supports conditional UI | ||
| * const supportsConditionalUI = await FRWebAuthn.isConditionalUISupported(); | ||
| * | ||
| * if (supportsConditionalUI) { | ||
| * // The authenticate() method automatically handles conditional UI | ||
| * // when the server indicates support via conditionalWebAuthn: true | ||
| * // in the metadata. No additional code changes needed. | ||
| * await FRWebAuthn.authenticate(step); | ||
| * | ||
| * // For conditional UI to work in the browser, add autocomplete="webauthn" | ||
| * // to your username input field: | ||
| * // <input type="text" name="username" autocomplete="webauthn" /> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if this isn't provided and the promise is hanging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've got a fix in based on code I've seen elsewhere in the code base. Feel free to correct me if I've mssed up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kian note: |
||
| * } | ||
| * ``` | ||
| */ | ||
| abstract class FRWebAuthn { | ||
| /** | ||
|
|
@@ -94,8 +116,29 @@ abstract class FRWebAuthn { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the browser supports conditional UI (autofill) for WebAuthn. | ||
| * | ||
| * @return Promise<boolean> indicating if conditional mediation is available | ||
| */ | ||
| public static async isConditionalUISupported(): Promise<boolean> { | ||
| if (!window.PublicKeyCredential) { | ||
| return false; | ||
| } | ||
|
|
||
| // Check if the browser supports conditional mediation | ||
| try{ | ||
| return withTimeout(PublicKeyCredential.isConditionalMediationAvailable(), ONE_SECOND) | ||
| } catch { | ||
| throw new Error('Error determining conditional mediation support'); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Populates the step with the necessary authentication outcome. | ||
| * Automatically handles conditional UI if indicated by the server metadata. | ||
| * | ||
| * @param step The step that contains WebAuthn authentication data | ||
| * @return The populated step | ||
|
|
@@ -108,19 +151,27 @@ abstract class FRWebAuthn { | |
|
|
||
| try { | ||
| let publicKey: PublicKeyCredentialRequestOptions; | ||
| let useConditionalUI = false; | ||
|
|
||
| if (metadataCallback) { | ||
| const meta = metadataCallback.getOutputValue('data') as WebAuthnAuthenticationMetadata; | ||
|
|
||
| // Check if server indicates conditional UI should be used | ||
| useConditionalUI = meta.conditionalWebAuthn === true; | ||
|
|
||
| publicKey = this.createAuthenticationPublicKey(meta); | ||
|
|
||
| credential = await this.getAuthenticationCredential( | ||
| publicKey as PublicKeyCredentialRequestOptions, | ||
| useConditionalUI, | ||
| ); | ||
| outcome = this.getAuthenticationOutcome(credential); | ||
| } else if (textOutputCallback) { | ||
| publicKey = parseWebAuthnAuthenticateText(textOutputCallback.getMessage()); | ||
|
|
||
| credential = await this.getAuthenticationCredential( | ||
| publicKey as PublicKeyCredentialRequestOptions, | ||
| false, // Script-based callbacks don't support conditional UI | ||
| ); | ||
| outcome = this.getAuthenticationOutcome(credential); | ||
| } else { | ||
|
|
@@ -300,18 +351,37 @@ abstract class FRWebAuthn { | |
| * Retrieves the credential from the browser Web Authentication API. | ||
| * | ||
| * @param options The public key options associated with the request | ||
| * @param useConditionalUI Whether to use conditional UI (autofill) | ||
| * @return The credential | ||
| */ | ||
| public static async getAuthenticationCredential( | ||
| options: PublicKeyCredentialRequestOptions, | ||
| useConditionalUI = false, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as per the hanging promise comment, we may need to use an abort signal here to allow developers to abort the promise if the passkey option isn't there? I may be not following this correctly, so please correct me where I'm wrong, but if a developer is using autofill ui, but fails to add the html correctly, can't we end up in a hanging promise state? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also abort signal may be the right path, we would have to figure out how to do it. |
||
| ): Promise<PublicKeyCredential | null> { | ||
| // Feature check before we attempt registering a device | ||
| // Feature check before we attempt authenticating | ||
| if (!window.PublicKeyCredential) { | ||
| const e = new Error('PublicKeyCredential not supported by this browser'); | ||
| e.name = WebAuthnOutcomeType.NotSupportedError; | ||
| throw e; | ||
| } | ||
| const credential = await navigator.credentials.get({ publicKey: options }); | ||
| // Build the credential request options | ||
| const credentialRequestOptions: CredentialRequestOptions = { | ||
| publicKey: options, | ||
| }; | ||
|
|
||
| // Add conditional mediation if requested and supported | ||
| if (useConditionalUI) { | ||
| const isConditionalSupported = await this.isConditionalUISupported(); | ||
| if (isConditionalSupported) { | ||
| credentialRequestOptions.mediation = 'conditional' as CredentialMediationRequirement; | ||
| } else { | ||
| // eslint-disable-next-line no-console | ||
| FRLogger.warn('Conditional UI was requested, but is not supported by this browser.'); | ||
| console.warn('Conditional UI was requested, but is not supported by this browser.'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, shout if this is correct/incorrect please. |
||
| } | ||
| } | ||
|
|
||
| const credential = await navigator.credentials.get(credentialRequestOptions); | ||
| return credential as PublicKeyCredential; | ||
| } | ||
|
|
||
|
|
@@ -433,22 +503,51 @@ abstract class FRWebAuthn { | |
| const { | ||
| acceptableCredentials, | ||
| allowCredentials, | ||
| _allowCredentials, | ||
| challenge, | ||
| relyingPartyId, | ||
| _relyingPartyId, | ||
| timeout, | ||
| userVerification, | ||
| extensions, | ||
| } = metadata; | ||
| const rpId = parseRelyingPartyId(relyingPartyId); | ||
| const allowCredentialsValue = parseCredentials(allowCredentials || acceptableCredentials || ''); | ||
|
|
||
| return { | ||
| // Use the structured _allowCredentials if available, otherwise parse the string format | ||
| let allowCredentialsValue: PublicKeyCredentialDescriptor[] | undefined; | ||
| if (_allowCredentials && Array.isArray(_allowCredentials)) { | ||
| allowCredentialsValue = _allowCredentials; | ||
| } else { | ||
| allowCredentialsValue = parseCredentials(allowCredentials || acceptableCredentials || ''); | ||
| } | ||
|
|
||
| // Use _relyingPartyId if available, otherwise parse the old format | ||
| const rpId = _relyingPartyId || parseRelyingPartyId(relyingPartyId); | ||
|
|
||
| const options: PublicKeyCredentialRequestOptions = { | ||
| challenge: Uint8Array.from(atob(challenge), (c) => c.charCodeAt(0)).buffer, | ||
| timeout, | ||
| // only add key-value pair if proper value is provided | ||
| ...(allowCredentialsValue && { allowCredentials: allowCredentialsValue }), | ||
| ...(userVerification && { userVerification }), | ||
| ...(rpId && { rpId }), | ||
| }; | ||
| // For conditional UI, allowCredentials can be omitted. | ||
| // For standard WebAuthn, it may or may not be present. | ||
| // Only add the property if the array is not empty. | ||
| if (allowCredentialsValue && allowCredentialsValue.length > 0) { | ||
| options.allowCredentials = allowCredentialsValue; | ||
| } | ||
|
|
||
| // Add optional properties only if they have values | ||
| if (userVerification) { | ||
| options.userVerification = userVerification; | ||
| } | ||
|
|
||
| if (rpId) { | ||
| options.rpId = rpId; | ||
| } | ||
|
|
||
| if (extensions && Object.keys(extensions).length > 0) { | ||
| options.extensions = extensions; | ||
| } | ||
|
|
||
| return options; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
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.
So AM now sends both of these values? we need to take precedence on the array I assume because it can list the possible passkeys?
Is this AM being backwards compatible with the older
allowCredentials?Uh oh!
There was an error while loading. Please reload this page.
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.
They are effectively identical as AM supports both for as you mentioned backwards compatibility. If you feel like this answers your question, give me a should and I can resolve this thread.