feat: add auto complete ui for AM-33773 #571
Conversation
|
george-bafaloukas-forgerock
requested review from
ancheetah,
cerebrl,
george-bafaloukas-forgerock and
ryanbas21
There was a problem hiding this comment.
We need to ensure that because this is a newer browser feature we are backwards compatible with browsers that may not support this feature.
So graceful degradation of conditional ui should occur where the browser doesn't break the user and we can at least continue the flow as needed.
Any details regarding this would be helpful because i'm fairly new to this feature
This is specific to authentication right? Is this testable with virtual authenticators? If we can pre-populate a user with a passkey in AM, i think we should be able to create an e2e for this?
If not, can we add an e2e test that at least can be run manually (doesn't need actual assertions, but it would be helpful for us to document how to do this work in code)
Also, please add a changeset to this PR, it should be a new feature, with a description of the feature being added so we can release correctly.
| credentialRequestOptions.mediation = 'conditional' as CredentialMediationRequirement; | ||
| } else { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('Conditional UI was requested, but is not supported by this browser.'); |
There was a problem hiding this comment.
we need to use logger for this, in case a user does not want console log's to happen. the logger module will call console logs if enabled.
There was a problem hiding this comment.
Done, shout if this is correct/incorrect please.
| } | ||
|
|
||
| interface WebAuthnAuthenticationMetadata { | ||
| _action?: string; |
There was a problem hiding this comment.
Can we have a better type than a string for this? seems like we can have 'webauthn_authentication', I assume the alternative is 'webauthn_registration'?
| userVerification: UserVerificationType; | ||
| conditionalWebAuthn?: boolean; | ||
| extensions?: Record<string, unknown>; | ||
| _type?: string; |
There was a problem hiding this comment.
What are the possible _type's that we can get? Ideally we'd like to be more strict than string where possible. In the test data I see WebAuthn but what other options are there?
What role does _type play in this data if any as it relates to the sdk?
There was a problem hiding this comment.
Change made to literal string WebAuthn
Note for Kian: This means that the passed _type must be exactly that. If for whatever reason we want to pass in anything, then change it back to string. If we want to change it to discrete values then change it to 'X|Y|Z'
|
|
||
| // Check if the browser supports conditional mediation | ||
| if (typeof PublicKeyCredential.isConditionalMediationAvailable === 'function') { | ||
| return await PublicKeyCredential.isConditionalMediationAvailable(); |
There was a problem hiding this comment.
This method looks like it can throw so we may need to handle the error in case it does.
[Exceptions](https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredential/isConditionalMediationAvailable_static#exceptions)
The returned [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) may be rejected with the following values:
SecurityError [DOMException](https://developer.mozilla.org/en-US/docs/Web/API/DOMException)
The RP domain is not valid.
Otherwise we may leave the auth state in a failure and unrecoverable error.
| _action: 'webauthn_authentication', | ||
| challenge: 'JEisuqkVMhI490jM0/iEgrRz+j94OoGc7gdY4gYicSk=', | ||
| allowCredentials: '', | ||
| _allowCredentials: [], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| * | ||
| * // For conditional UI to work in the browser, add autocomplete="webauthn" | ||
| * // to your username input field: | ||
| * // <input type="text" name="username" autocomplete="webauthn" /> |
There was a problem hiding this comment.
What happens if this isn't provided and the promise is hanging?
There was a problem hiding this comment.
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.
Kian note:
There is a current question in my fix if after timeout we shout throw an exception or just return false. Either way we should make it so the admin can specify a timeout time (have a default just incase).
Throwing errors seems to be what we do in the SDK
| */ | ||
| public static async getAuthenticationCredential( | ||
| options: PublicKeyCredentialRequestOptions, | ||
| useConditionalUI = false, |
There was a problem hiding this comment.
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.
Also abort signal may be the right path, we would have to figure out how to do it.
3 participants
JIRA Ticket
Ticket
Description
Change enables support for autofill conditional UI in Javascript SDK.
Relates to https://pingidentity.atlassian.net/browse/OPENAM-23288