Passing opaque user data down into `CredentialRepository`

Sibling issue to #274, I suppose...

Is there a way to pass opaque data about the user to the CredentialRepository (in particular, the getCredentialIdsForUsername implementation) without that data being exposed to the user?

For context, we have an internal unique user identifier which is not human readable. We do not have unique human-readable usernames. We set .name() to a non-unique self-descriptor for the user. It is impossible for us to map this self-descriptor back to a unique user for purposes of getCredentialIdsForUsername.

From an implementation point of view, I suppose this is complicated by getCredentialIdsForUsername receiving the username (in particular, the same .name that is included in the PublicKeyCredentialCreationOptions that's sent to the client) instead of the full User object. Changing this would break backwards compatibility.

One backwards-compatible way of solving this would be to provide an @JsonIgnore Optional<String> internalUsername field (and associated builder method) on UserIdentity. In RelyingParty::startRegistration, you could then pass internalUsername to getCredentialIdsForUsername if it is set, and otherwise fall back to name.

Hm, these are good questions. Let's think about this for a bit. I suspect your needs could also be served by adding an optional getCredentialIdsForUserHandle(ByteArray) method to CredentialRepository.

First, some background.

As of now, the getCredentialIdsForUsername(String) method is used for two things: to set the excludeCredentials parameter for new registrations and to set the allowCredentials parameter for (most likely second-factor) authentication requests. For passkey (first-factor, "username-less") authentication, getCredentialIdsForUsername is not called since the user is not yet identified; an empty allowCredentials is sent instead, and the user ID can be discovered via the PublicKeyCredential.response.userHandle method instead.

The most important of these uses is the latter: setting allowCredentials, because second-factor authentications just don't work without it. excludeCredentials is not necessary for registrations to function, but it does help improve the user experience by preventing the confusing situation where a user ends up with two credentials but both are on the same authenticator.

So if we ignore excludeCredentials for a bit and think only about allowCredentials: The design behind getCredentialIdsForUsername comes from the assumption that if you are using second-factor authentication, then you have a unique, human-readable identifier that users can enter into a form or the like, so that identifier can also be used to accurately look up credentials for that user. If you do not have such an identifier, then the second-factor flow doesn't work, so you must be using passkey authentication instead, in which case getCredentialIdsForUsername is not needed. If that is the only user flow you support, then as far as authentication is concerned you could leave getCredentialIdsForUsername unimplemented, although this is not expressed well in the type system.

Now if we again include excludeCredentials in the discussion, the above argument breaks down, because for registration you obviously already know the user identity (unless you're creating a new account, in which case it obviously has no credentials yet), but the design of getCredentialIdsForUsername assumes you must have a unique human-readable user identifier. Registrations do always call getCredentialIdsForUsername, so you can't actually leave it unimplemented.

Now, about your use case as the real-world example in question: you say that you indeed have no unique human-readable identifier. I presume this means there is no way for your users to first enter a user identifier into a form and then get prompted with the WebAuthn request as the second step - either you use passkey ("username-less") authentication exclusively, or you have some other, external way to discover a unique (but non-human-readable) user identifier.

If those presumptions are correct, it seems to me like at least one of these is always true in your case:

You use the passkey (username-less, allowCredentials: []), authentication flow.
You know a unique identifier for the user, which you can use to look up the corresponding user handle (or perhaps the two are the same).

So how about adding a method like this to CredentialRepository? It would be optional for backwards compatibility, but implementers of this method would be allowed to not implement getCredentialIdsForUsername instead. Would that solve your use case?

/**
  * Get the credential IDs of all credentials registered to the user with the given user handle.
  *
  * <p>The <i>user handle</i> is defined by the <code>
  * {@link UserIdentity}.{@link UserIdentity.UserIdentityBuilder#id(ByteArray) id(ByteArray)}
  * </code> setting in {@link StartRegistrationOptions}.
  *
  * <p>After a successful registration ceremony, the {@link RegistrationResult#getKeyId()} method
  * returns a value suitable for inclusion in this set.
  *
  * @param userHandle the user handle of the user whose credential IDs to look up.
  * @return a {@link Set} containing one {@link PublicKeyCredentialDescriptor} for each credential
  *     registered to the user with the given user handle.
  */
Set<PublicKeyCredentialDescriptor> getCredentialIdsForUserHandle(ByteArray userHandle);

If so, I'll have to think a bit about how exactly to implement it. The simple approach is to give it a default implementation that just throws UnsupportedOperationException, but then there's also the question of how exactly it should interact with getUserHandleForUsername and getUsernameForUserHandle. A lot of that complexity ends up exposed to the library user in the form of interface requirements in prose.

Another option is to make RelyingParty accept several versions of the interface. For example, you could build a RelyingParty as usual:

RelyingParty rp = RelyingParty.builder()
    .identity(/* ... */)
    .credentialRepository(new CredentialRepository() { /* ... */ })
    // ...other settings
    .build();

or with a new, stripped-down CredentialRepositoryV2 interface without support for usernames:

RelyingPartyWithoutUsernames rp = RelyingParty.builder()
    .identity(/* ... */)
    .credentialRepository(new CredentialRepositoryV2() {
        Set<PublicKeyCredentialDescriptor> getCredentialIdsForUserHandle(ByteArray userHandle) { /* ... */ }
        Optional<RegisteredCredential> lookup(ByteArray credentialId, ByteArray userHandle) { /* ... */ }
        boolean credentialIdExists(ByteArray credentialId) { /* ... */ }
    })
    // ...other settings
    .build();

rp.startAssertion(
    StartAssertionOptionsV2.builder()  // Note: new variant of StartAssertionOptions
        .username("foo")               // Compile error: method "username(String)" does not exist
        .build()
);

or with the new interface but also with support for usernames:

RelyingParty rp = RelyingParty.builder()
    .identity(/* ... */)
    .credentialRepository(
        new CredentialRepositoryV2() {
            Set<PublicKeyCredentialDescriptor> getCredentialIdsForUserHandle(ByteArray userHandle) { /* ... */ }
            Optional<RegisteredCredential> lookup(ByteArray credentialId, ByteArray userHandle) { /* ... */ }
            boolean credentialIdExists(ByteArray credentialId) { /* ... */ }
        },
        new UsernameRepository() {
            Optional<ByteArray> getUserHandleForUsername(String username) { /* ... */ }
            Optional<String> getUsernameForUserHandle(ByteArray userHandle) { /* ... */ }
        })
    // ...other settings
    .build();

rp.startAssertion(
    StartAssertionOptions.builder()
        .username("foo")             // OK, method exists
        .build()
);

Then perhaps a future major version could drop the older variant in favor of the new ones.

I think I like this idea, as it allows the implementer to pick the features they need while keeping most of the complexity encapsulated within RelyingParty (and possibly variants of it). The reduced feature set could also be captured in the type system instead of by UnsupportedOperationExceptions at runtime.

And yes, this would certainly relate to #274 as well. This "type state" kind of design would also open some opportunities for type safe passthrough of auxiliary credential data as discussed in https://github.com/Yubico/java-webauthn-server/issues/274#issuecomment-1549818196 and attestation data which tangentially relates to https://github.com/Yubico/java-webauthn-server/issues/279. With more than one use for that kind of design, I'm more inclined to extend the library in that direction.

Sorry for the wall of text... :sweat_smile: What do you think?

For our use case, we are exclusively concerned with excludeCredentials, yes. We are looking to use WebAuthn credentials on appropriately-certified authenticators as a single login factor – the "passkey setup".

The "internal username" suggestion I gave would cover our very narrow use case and leave the interface untouched, but if you are already open to extending the interface, I would offer the following suggestion instead:

Extend UserIdentity with a (potentially type-safe?) optional auxiliary data field, similar to what was discussed for RegisteredCredential in #274. Add getCredentialIdsForUser(UserIdentity identity) to the interface, and have it default to getCredentialIdsForUsername(identity.getName()) for backwards compatibility. (You can then also consider deprecating the "old" variant.) This covers both our "internal identifier" use case, but also other use cases I can easily envision, such as one where the user's credentials have already been looked up by the caller, and this information can be passed into to the repository (the reverse of #274).

To cover the allowCredentials application, you could also allow passing a UserIdentity (which could then also contain relevant auxiliary data) instead of handle/name as part of StartAssertionOptions.

In particular, the getCredentialIdsForUserHandle approach would not allow use cases where the "internal identifier" should not be exposed to the user; it still forces the credential repository to solely operate on information that is available to the user side.

Continuing from https://github.com/Yubico/java-webauthn-server/pull/293#issuecomment-1609634380:

However, even more saliently: what about a situation where the consumer code already has the list of registered credentials, for example because it automatically receives information about the currently-logged-in user from the server framework. Currently, there is no way for the consumer code to provide this list-of-registered-credentials to the CredentialRepository to use (i.e., map to PKCD and return); you're forcing the repository to query for this information again (based on just the user handle/user name).

Would it make sense to add this as an optional property in FinishAssertionOptions? I'm thinking something along these lines:

RelyingParty rp = /* ... */;
AssertionRequest request = /* ... */;
PublicKeyCredential</* ... */> response = /* ... */;
Set<RegisteredCredential> credentials = /* Credentials already looked up... */;

AssertionResult result =
    rp.finishAssertion(
        FinishAssertionOptions.builder()
            .request(request)
            .response(response)
            .preloadCredentials(credentials)
            .build());

Would that help your use case? (In addition to the CredentialRepository redesign to better support username-less applications)

Adding this kind of field to StartRegistrationOptions would also solve our specific use case (by allowing us to circumvent the CredentialRepository entirely,) yes. It still feels like it'd lose flexibility that might be useful for other applications, though.

PS: For our specific use case, we wouldn't actually need this information in FinishAssertionOptions, though I can also see how it'd be useful there. It still feels very narrowly tailored.

Interesting, my intuition says the opposite! :slightly_smiling_face:

It also occurred to me that the {Start,Finish}{Registration,Assertion}Options parameter could also be a whole CredentialRepository instance instead, allowing the caller to override the global one on a per-request basis (perhaps using the global one as a fallback, I'm not sure if that would make sense).

...actually, come to think of it: that's actually already supported, in a way.

The library as a whole is designed with no internal mutable state, and one major reason for that is that this means RelyingParty instances don't have to be long-lived. Two RelyingParty instances created with the same arguments will behave the same way if you call their methods with the same arguments. This helps support parallelism and multi-machine deployments, among other things, as the library makes no assumptions on how you want to manage your state. So it's definitely not an anti-pattern to construct a new RelyingParty instance with its own specialized CredentialRepository on a per-request basis if the application needs to. Of course that does come at a nonzero cost in performance, but it's not a lot of code that runs to create a new instance. It would actually be very interesting to make a proper performance benchmark to measure that impact.

Would that be an acceptable solution for your use case? (Again, apart from the redesign to better support username-less application.) If so, new library features might not actually be necessary.

Hmm, yes, we could potentially make that work. Even though I understand the library guarantees you describe, it still doesn't feel like a clean solution; it feels like something that'd confuse a maintenance developer stumbling across my code in a few years' time.

Hm, yeah, I can understand that. I've been considering back and forth whether to go ahead and add that method to override CredentialRepository in FinishAssertionOptions, but I'm not sure. So far I think this use case is fairly rare, so I'm not sure it's worth having multiple ways to do the same thing.

Anyway, see also my latest comment in https://github.com/Yubico/java-webauthn-server/issues/274#issuecomment-1625675423 . The new, experimental CredentialRepositoryV2 interface switches things around so usernames aren't required with that interface, which should hopefully resolve the main blocker that remains here.

Thanks for your patience and sorry for the delay - the experimental CredentialRepositoryV2 suite is now released in experimental release 2.6.0-alpha4. I hope this supports your use case - if not, please let us know so we can refine it further!

Yubico / java-webauthn-server

Passing opaque user data down into `CredentialRepository` #289