Yubico / java-webauthn-server

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

Support PublicKeyCredentialHints in PublicKeyCredentialCreationOptions #371

Closed hertg closed 1 month ago

hertg commented 3 months ago

WebAuthn Relying Parties may use this enumeration to communicate hints to the user-agent about how a request may be best completed. These hints are not requirements, and do not bind the user-agent, but may guide it in providing the best experience by using contextual information that the Relying Party has about the request.

https://w3c.github.io/webauthn/#enum-hints

This would be a good, and more nuanced alternative to the authenticatorAttachment parameter. Also see this discussion on Mastodon.

emlun commented 3 months ago

Indeed, thanks for the poke. Note that since the hints don't affect the verification procedures at all, you can always add them in your client-side JavaScript (or on the server before sending the serialized options to the client) and that will work just as well as if they were supported in the library. But that is also true of timeout, so it certainly makes sense to support these in the library to have it all in one place.

hertg commented 3 months ago

Good point about the client-side addition. Though, I quite like the approach of having all the registration parameters come directly from the server and for my case, I think adding additional client-side complexity is not justified for "just" those hints, where an additional config in the server code would be reasonable.

Would you appreciate getting a PR for this? I might have some time to do that. I'm assuming that creating a PublicKeyCredentialHints enum and adding it as a field to StartRegistrationOptionsBuilder would be the way to go?

emlun commented 3 months ago

Sure, you're welcome to contribute! And yes, this will be very analogous to the timeout parameter in most aspects. Please make the builder setter take a variadic argument and wrap it with Collections.unmodifiableList.

...Hm, on second thought I'm not actually sure we should restrict the type of that list to a PublicKeyCredentialHints enum. Similar to how it's explained in the spec, an enum type may be less future-compatible than it should. We have no need to process these values internally in the library, so we don't really have a reason to restrict what values are allowed, so it's nice if the library can allow values added in the future without having to update the library. Perhaps instead of an enum, we should just have a static PublicKeyCredentialHints class with constants for the currently defined values, and make the hints property a plain List<String>?

I think the best would be a bit of both: define the property as List<String>, but provide both a hints(...String) setter and a hints(...PublicKeyCredentialHints) setter that unpacks a public String value from each of the given enum values. This way, users can use the enum variant for convenience with IDE autocompletion etc., but can also fall back to the raw String variant in case they need to pass a value defined in the future.

emlun commented 3 months ago

See also:

(The discussion here is what alerted me to that issue in the spec)

hertg commented 3 months ago

Yeah, the odd plural form stuck out to me too when I started to implement the PR yesterday :+1:

I also agree with your List<String> proposal. When the value is omitted, what do you prefer it to be, null or an empty list? I'd go with an empty list, since the default in webauthn is that the hints default to [] and I personally don't like dealing with Optional<List<T>> when there is no difference between an omitted list and an explicitly defined empty list.

emlun commented 3 months ago

Yep, I agree - empty list and undefined are equivalent in the spec, so let's make it non-nullable and default empty.

hertg commented 3 months ago

I have a really hard time working on this, for some reason my IntelliJ IDEA can't handle this project very well. Having it open uses a lot of CPU, and once open I can't even gracefully close IntelliJ, it needs a SIGKILL to stop. Do you have any tips for the development setup?

emlun commented 3 months ago

That's strange, I've been using IDEA without issues. Indexing the project can sometimes take upwards of a few minutes, but I don't think I've had IDEA unresponsive. Have you tried clearing the cache? You might also try running ./gradlew idea in the project root and see if that helps.

fdennis commented 2 months ago

Hey @hertg We started working on a branch for supporting this and have opened a draft PR https://github.com/Yubico/java-webauthn-server/pull/378. It is still not entirely finished but feel free to check it out and add to it if you want.

hertg commented 2 months ago

Oops sorry, I wanted to report back but forgot. I eventually fixed my IDE issues with a combination of updates and the "Repair IDE" feature. I'm getting more busy at work which is why I didn't come around to submit a PR yet. Thanks for opening the PR from your side, I'll take a look and comment if there's anything I want to add.