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

Clarify usage of credentialRecord.transports #2016

Closed
jameshartig opened this issue Jan 19, 2024 · 5 comments · Fixed by #2031
Closed

Clarify usage of credentialRecord.transports #2016

jameshartig opened this issue Jan 19, 2024 · 5 comments · Fixed by #2031

Comments

@jameshartig
Copy link

Proposed Change

The current spec says that credentialRecord.transports is RECOMMENDED but I can't find a reference for when to use it. I believe this was lost in #1773 where it was previously used to populate the allowCredentials.

I would like to understand how RPs should use this field in practice. We (an RP) require the use of discoverable credentials and therefore never fill in allowCredentials. My understanding is that credentialRecord.transports has no purpose in our use-case. I don't know how the WG decides between "RECOMMENDED" and "OPTIONAL" but it's confusing that it's currently "RECOMMENDED" without any mention of how/when to use it.

Also, I wasn't sure where to post this question, I didn't see much non-GitHub communication in the public mailing list and my company is not a W3C member. If this is the wrong avenue for this type of discussion, I apologize.

@sbweeden
Copy link
Contributor

The right place to ask this question is the fido-dev mailing list: https://groups.google.com/a/fidoalliance.org/g/fido-dev

If you are only using empty allowCredentials then you don't need to store it. It has a purpose though - when passed back to the client (browser) when using an allowCredentials list, it allows the browser to choose the best UI for the credentials it knows the RP wants to use (e.g. USB only, etc).

Please close this issue if satisfied with the answer, otherwise it will likely be closed during the next working group call.

@jameshartig
Copy link
Author

@sbweeden I appreciate the quick response and the pointer as for a better avenue in the future.

Should the spec be updated to reflect that's the intended use-case? I couldn't find any reference to credentialRecord.transports or transports that seems to indicate that it should be used in the way you described.

@sbweeden
Copy link
Contributor

That's the primary use case (at least AFAIK). There is perhaps statistical or UX value in the RP knowing the transports of a user's registered credentials (you might make suggestions about registering the platform authenticator for example), so I don't think there's a compelling reason to change the specification in this regard since there is more than one possible reason for storing them.

@emlun
Copy link
Member

emlun commented Jan 19, 2024

Thanks for pointing this out! There is a connection, but it is a bit buried:

So technically there is a recommendation of how to use it, but the chain of steps is certainly not obvious just by looking at the list of places where credentialRecord.transports is referenced.

It might help to somehow reference credentialRecord.transports in §7.2. Verifying an Authentication Assertion, like there is a reference in §7.1. Registering a New Credential, but the first step of §7.2 already reads:

  1. Let options be a new PublicKeyCredentialRequestOptions structure configured to the Relying Party's needs for the ceremony.

and I don't really want to start weighing that down with specific callouts of individual parameters of note.

I think we could improve the description of PublicKeyCredentialDescriptor.transports and include a reference to credentialRecord.transports there, though. I think that could have the additional benefit of making the paragraph a bit shorter, too. @jameshartig does that seem like a fair solution?

@emlun emlun self-assigned this Jan 19, 2024
@jameshartig
Copy link
Author

The definition of allowCredentials recommends to set it to a list of the credential descriptors for credential records in the user account.

I think that was the part I was missing.

I think we could improve the description of PublicKeyCredential.transports and include a reference to credentialRecord.transports there, though. I think that could have the additional benefit of making the paragraph a bit shorter, too. @jameshartig does that seem like a fair solution?

I agree, the current paragraph talks about storing the getTransports() return and using that stored value when it could just reference the credentialRecord.transports and it would be clearer.

Thank you!

@jameshartig jameshartig changed the title credentialRecord.transports has no purpose Clarify usage of credentialRecord.transports Jan 19, 2024
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.

3 participants