search

Yubico / java-webauthn-server

Server-side Web Authentication library for Java https://www.w3.org/TR/webauthn/#rp-operations
Other
439 stars 137 forks source link

The usage of PublicKeyCredentialDescriptor in AllowCredentials vs excludeCredentials #320

Closed nbenjamin closed 7 months ago

nbenjamin commented 9 months ago

Hi Team,

Currently getCredentialIdsForUsername(String username) from CredentialRepository returns Set<PublicKeyCredentialDescriptor> and its used mainly for adding excludeCredentials . AllowCredentials in PublicKeyCredentialRequestOptions takes List of PublicKeyCredentialDescriptor. So rather than returning Set<PublicKeyCredentialDescriptor> we should return List<PublicKeyCredentialDescriptor> getCredentialIdsForUsername(String username); to provide a better reuse of the method. Is there any specific reason to return Set<PublicKeyCredentialDescriptor>?

emlun commented 9 months ago

Hi!

There is indeed a slight mismatch here. PublicKeyCredentialRequestOptions takes a List value to reflect the allowCredentials definition in the spec, while CredentialRepository returns a Set to communicate to the implementer that there can be no duplicates and order is unimportant. The RelyingParty.startAssertion method converts the collection type automatically, so downstream users should rarely be affected by the type mismatch.

However, I was reminded now that the allowCredentials definition does say that the list is ordered in descending order of preference. So yes, the CredentialRepository interface cannot express that feature of the spec.

The order of the list has little or no effect in practice, though. Browsers generally offer the user all available authenticators and goes with the first one the user picks. So I'd call this a minor issue that's unlikely to have any practical impact. It's certainly not worth breaking the CredentialRepository interface for it, but we could consider it if we make other backwards-incompatible changes like those discussed in https://github.com/Yubico/java-webauthn-server/issues/274#issuecomment-1625675423 .

Does that answer your questions?

nbenjamin commented 9 months ago

Thank you @emlun for your quick response. Any plans for releasing this #274 changes

emlun commented 9 months ago

We plan to release it at some point, but there's no concrete time plan at the moment. Perhaps later this year.

emlun commented 8 months ago

The features discussed in #274 are now released in experimental release 2.6.0-alpha4, but the new CredentialRepositoryV2 interface still returns a Set of credentials to be included in allowCredentials. We decided to stick with the Set because the CredentialRepositoryV2 implementations doesn't get any other context about the request beyond just a user ID, so there isn't really any way for it to know if or how to prioritize the credentials. But it's still possible for applications to edit the PublicKeyCredentialRequestOptions (using the .toBuilder() method) before sending it to the client, if they need to.