search

Yubico / java-webauthn-server

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

Add extra-data field to `UserIdentity` #293

Closed iaik-jheher closed 1 year ago

iaik-jheher commented 1 year ago

This is part of the UserIdentity work I proposed in https://github.com/Yubico/java-webauthn-server/issues/289#issuecomment-1572100280; I am breaking it down into smaller parts for ease of review.

In parallel to #290, this proposes an opaque, type-safe extra-data field on UserIdentity. This field can be used by library consumers to pass arbitrary data from their business logic to their credential repository.

I've additionally taken the liberty of replacing the @JsonCreator constructor with Lombok's @Jacksonized on the class itself; this should be equivalent for JSON deserialization purposes (the test cases check this), but allows the Lombok @Builder annotation to generate its own private constructor.

emlun commented 1 year ago

Thanks! I'll look into these PRs next week.

emlun commented 1 year ago

Sorry for the delay. Thanks, but I want to keep the UserIdentity type in sync with the PublicKeyCredentialUserEntity type in WebAuthn that it reflects. Adding properties to this type would muddy that connection. There were some other solutions proposed in #289, I'd rather proceed with one of those.

iaik-jheher commented 1 year ago

As mentioned in #289, I do not see how a CredentialRepository that operates exclusively off of user-visible information (i.e., information in the current UserIdentity type) can cover cases where internal identifiers have organizational processing/storage requirements that prevent them from being transmitted to the user and/or stored on a user device.

emlun commented 1 year ago

Could such a system not translate the user handle to an internal identifier before proceeding with the lookup?

iaik-jheher commented 1 year ago

Hm; plausibly yes, I suppose, though requiring an additional translation step (or additional linking/indexing) before requesting internal data still bothers me (for our specific use case).

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).

I understand the reluctance to deviate from 1:1 correspondence with the client-side object, but this induces very frustrating behavior for consumers: we're being forced to find workarounds to accomodate something that "seems" like it should be easy - tacking some arbitrary data onto an existing object that's being passed around; in particular, data that, from the point of view of our application, is part of "the user".

PS: Another alternative would be to specify Lombok's @NonFinal (and potentially @AllArgsConstructor with protected visibility) on UserIdentity. This would let you keep a "pristine" type in the library while letting consumers attach arbitrary data by subclassing.

emlun commented 1 year ago

Hm; plausibly yes, I suppose, though requiring an additional translation step (or additional linking/indexing) before requesting internal data still bothers me (for our specific use case).

Wouldn't the RP have to do that at some point anyway, though? Unless the RP doesn't need the user handle at all, of course - in that case, right now the library requires a getUserHandleFromUsername lookup that's perhaps not really necessary. That is the topic of issue #289 and PR #292.

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).

I appreciate this point, and agree we should help alleviate that. Let's continue this discussion in issue #289.