Skip to content
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

Invoking the authenticatorMakeCredential operation clarification #2025

Closed
dimitribouniol opened this issue Feb 16, 2024 · 2 comments · Fixed by #2027
Closed

Invoking the authenticatorMakeCredential operation clarification #2025

dimitribouniol opened this issue Feb 16, 2024 · 2 comments · Fixed by #2027

Comments

@dimitribouniol
Copy link

Proposed Change

I could be misinterpreting this, so I wanted to verify with an issue first, but it seems like the steps mentioned in 5.1.3 for creating a new credential are mis-indented, suggesting that if the optional excludeCredentials are not provided, invoking authenticatorMakeCredential would never be done:

image

After several re-readings, this seems to make more sense logically:

 5.1.3. Create a New Credential - PublicKeyCredential’s [[Create]](origin, options, sameOriginWithAncestors) Method
     ...
     25. While lifetimeTimer has not expired, perform the following actions depending upon lifetimeTimer, and the state and response for each authenticator in authenticators:
         ...
         → If an authenticator becomes available on this client device,
             ...
             23. For each credential descriptor C in pkOptions.excludeCredentials:
                 1. If C.transports is not empty, and authenticator is connected over a transport not mentioned in C.transports, the client MAY continue.
                     NOTE: If the client chooses to continue, this could result in inadvertently registering multiple credentials bound to the same authenticator if the transport hints in C.transports are not accurate. For example, stored transport hints could become inaccurate as a result of software upgrades adding new connectivity options.
                 2. Otherwise, Append C to excludeCredentialDescriptorList.
-                3. Invoke the authenticatorMakeCredential operation on authenticator with clientDataHash, pkOptions.rp, pkOptions.user, requireResidentKey, userVerification, credTypesAndPubKeyAlgs, excludeCredentialDescriptorList, enterpriseAttestationPossible, attestationFormats, and authenticatorExtensions as parameters.
-            24. Append authenticator to issuedRequests.
+            24. Invoke the authenticatorMakeCredential operation on authenticator with clientDataHash, pkOptions.rp, pkOptions.user, requireResidentKey, userVerification, credTypesAndPubKeyAlgs, excludeCredentialDescriptorList, enterpriseAttestationPossible, attestationFormats, and authenticatorExtensions as parameters.
+            25. Append authenticator to issuedRequests.

As currently listed, this suggests that authenticatorMakeCredential should be invoked for every excluded credential descriptor instead of once with the list of excluded credential descriptors. If however the logical positioning is correct here, then perhaps some clarifying language about when authenticatorMakeCredential is called when excludeCredentials is not provided is warranted (I hope I'm not the only one confused here 😅).

@emlun
Copy link
Member

emlun commented Feb 16, 2024

This is indeed incorrect, thanks for catching it! The indentation appears to have been broken in commit b44009c (originally 7acc1d5) in PR #1366, caused by a mix of tab and space characters being replaced with just spaces. This will be fixed by PR #2027.

@dimitribouniol
Copy link
Author

Thanks for confirming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants