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

Support PublicKeyCredentialHints in PublicKeyCredentialCreationOptions #371

Closed
hertg opened this issue Aug 5, 2024 · 10 comments · Fixed by #378
Closed

Support PublicKeyCredentialHints in PublicKeyCredentialCreationOptions #371

hertg opened this issue Aug 5, 2024 · 10 comments · Fixed by #378
Labels
enhancement New feature or request

Comments

@hertg
Copy link

hertg commented Aug 5, 2024

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 emlun added the enhancement New feature or request label Aug 5, 2024
@emlun
Copy link
Member

emlun commented Aug 5, 2024

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
Copy link
Author

hertg commented Aug 5, 2024

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
Copy link
Member

emlun commented Aug 7, 2024

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
Copy link
Member

emlun commented Aug 7, 2024

See also:

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

@hertg
Copy link
Author

hertg commented Aug 8, 2024

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

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
Copy link
Member

emlun commented Aug 8, 2024

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

@hertg
Copy link
Author

hertg commented Aug 8, 2024

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
Copy link
Member

emlun commented Aug 8, 2024

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
Copy link
Contributor

fdennis commented Sep 6, 2024

Hey @hertg
We started working on a branch for supporting this and have opened a draft PR #378.
It is still not entirely finished but feel free to check it out and add to it if you want.

@hertg
Copy link
Author

hertg commented Sep 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

3 participants