Pass StartAssertionOptions.userHandle through to finishAssertion

emlun · emlun · commit b234847d2c6c · 2022-04-01T16:54:09.000+02:00
diff --git a/NEWS b/NEWS
@@ -9,6 +9,15 @@ Fixes:
 * Fixed bug in `RelyingParty.finishAssertion` that would throw a nondescript
   `NoSuchElementException` if username and user handle are both absent, instead
   of an `IllegalArgumentException` with a better error message.
+* Fixed bug in `RelyingParty.finishAssertion` where if
+  `StartAssertionOptions.userHandle` was set, it did not propagate to
+  `RelyingParty.finishAssertion` and caused an error saying username and user
+  handle are both absent unless a user handle was returned by the authenticator.
+
+New features:
+
+* Added `userHandle` field to `AssertionRequest` as part of above bug fix.
+  `userHandle` is mutually exclusive with `username`.
 
 
 == Version 1.12.2 ==
diff --git a/webauthn-server-core/src/main/java/com/yubico/webauthn/AssertionRequest.java b/webauthn-server-core/src/main/java/com/yubico/webauthn/AssertionRequest.java
@@ -37,7 +37,7 @@
 
 /**
  * A combination of a {@link PublicKeyCredentialRequestOptions} and, optionally, a {@link
- * #getUsername() username}.
+ * #getUsername() username} or {@link #getUserHandle() user handle}.
  */
 @Value
 @Builder(toBuilder = true)
@@ -52,26 +52,58 @@ public class AssertionRequest {
   /**
    * The username of the user to authenticate, if the user has already been identified.
    *
-   * <p>If this is absent, this indicates that this is a request for an assertion by a <a
+   * <p>This is mutually exclusive with {@link #getUserHandle() userHandle}; setting this will unset
+   * {@link #getUserHandle() userHandle}. When parsing from JSON, {@link #getUserHandle()
+   * userHandle} takes precedence over this.
+   *
+   * <p>If both this and {@link #getUserHandle() userHandle} are empty, this indicates that this is
+   * a request for an assertion by a <a
    * href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#client-side-discoverable-public-key-credential-source">client-side-resident
    * credential</a>, and identification of the user has been deferred until the response is
    * received.
    */
   private final String username;
 
+  /**
+   * The user handle of the user to authenticate, if the user has already been identified.
+   *
+   * <p>This is mutually exclusive with {@link #getUsername() username}; setting this will unset
+   * {@link #getUsername() username}. When parsing from JSON, this takes precedence over {@link
+   * #getUsername() username}.
+   *
+   * <p>If both this and {@link #getUsername() username} are empty, this indicates that this is a
+   * request for an assertion by a <a
+   * href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#client-side-discoverable-public-key-credential-source">client-side-resident
+   * credential</a>, and identification of the user has been deferred until the response is
+   * received.
+   */
+  private final ByteArray userHandle;
+
   @JsonCreator
   private AssertionRequest(
       @NonNull @JsonProperty("publicKeyCredentialRequestOptions")
           PublicKeyCredentialRequestOptions publicKeyCredentialRequestOptions,
-      @JsonProperty("username") String username) {
+      @JsonProperty("username") String username,
+      @JsonProperty("userHandle") ByteArray userHandle) {
     this.publicKeyCredentialRequestOptions = publicKeyCredentialRequestOptions;
-    this.username = username;
+
+    if (userHandle != null) {
+      this.username = null;
+      this.userHandle = userHandle;
+    } else {
+      this.username = username;
+      this.userHandle = null;
+    }
   }
 
   /**
    * The username of the user to authenticate, if the user has already been identified.
    *
-   * <p>If this is absent, this indicates that this is a request for an assertion by a <a
+   * <p>This is mutually exclusive with {@link #getUserHandle()}; if this is present, then {@link
+   * #getUserHandle()} will be empty.
+   *
+   * <p>If both this and {@link #getUserHandle()} are empty, this indicates that this is a request
+   * for an assertion by a <a
    * href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#client-side-discoverable-public-key-credential-source">client-side-resident
    * credential</a>, and identification of the user has been deferred until the response is
    * received.
@@ -80,6 +112,22 @@ public Optional<String> getUsername() {
     return Optional.ofNullable(username);
   }
 
+  /**
+   * The user handle of the user to authenticate, if the user has already been identified.
+   *
+   * <p>This is mutually exclusive with {@link #getUsername()}; if this is present, then {@link
+   * #getUsername()} will be empty.
+   *
+   * <p>If both this and {@link #getUsername()} are empty, this indicates that this is a request for
+   * an assertion by a <a
+   * href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#client-side-discoverable-public-key-credential-source">client-side-resident
+   * credential</a>, and identification of the user has been deferred until the response is
+   * received.
+   */
+  public Optional<ByteArray> getUserHandle() {
+    return Optional.ofNullable(userHandle);
+  }
+
   /**
    * Serialize this {@link AssertionRequest} value to JSON suitable for sending to the client.
    *
@@ -140,6 +188,7 @@ public static AssertionRequestBuilder.MandatoryStages builder() {
 
   public static class AssertionRequestBuilder {
     private String username = null;
+    private ByteArray userHandle = null;
 
     public static class MandatoryStages {
       private final AssertionRequestBuilder builder = new AssertionRequestBuilder();
@@ -161,7 +210,10 @@ public AssertionRequestBuilder publicKeyCredentialRequestOptions(
     /**
      * The username of the user to authenticate, if the user has already been identified.
      *
-     * <p>If this is absent, this indicates that this is a request for an assertion by a <a
+     * <p>This is mutually exclusive with {@link #userHandle(ByteArray)}; setting this to non-empty
+     * will unset {@link #userHandle(ByteArray)}.
+     *
+     * <p>If this is empty, this indicates that this is a request for an assertion by a <a
      * href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#client-side-discoverable-public-key-credential-source">client-side-resident
      * credential</a>, and identification of the user has been deferred until the response is
      * received.
@@ -173,13 +225,55 @@ public AssertionRequestBuilder username(@NonNull Optional<String> username) {
     /**
      * The username of the user to authenticate, if the user has already been identified.
      *
-     * <p>If this is absent, this indicates that this is a request for an assertion by a <a
+     * <p>This is mutually exclusive with {@link #userHandle(ByteArray)}; setting this to non-<code>
+     * null</code> will unset {@link #userHandle(ByteArray)}.
+     *
+     * <p>If this is empty, this indicates that this is a request for an assertion by a <a
      * href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#client-side-discoverable-public-key-credential-source">client-side-resident
      * credential</a>, and identification of the user has been deferred until the response is
      * received.
      */
     public AssertionRequestBuilder username(String username) {
       this.username = username;
+      if (username != null) {
+        this.userHandle = null;
+      }
+      return this;
+    }
+
+    /**
+     * The user handle of the user to authenticate, if the user has already been identified.
+     *
+     * <p>This is mutually exclusive with {@link #username(String)}; setting this to non-empty will
+     * unset {@link #username(String)}.
+     *
+     * <p>If both this and {@link #username(String)} are empty, this indicates that this is a
+     * request for an assertion by a <a
+     * href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#client-side-discoverable-public-key-credential-source">client-side-resident
+     * credential</a>, and identification of the user has been deferred until the response is
+     * received.
+     */
+    public AssertionRequestBuilder userHandle(@NonNull Optional<ByteArray> userHandle) {
+      return this.userHandle(userHandle.orElse(null));
+    }
+
+    /**
+     * The user handle of the user to authenticate, if the user has already been identified.
+     *
+     * <p>This is mutually exclusive with {@link #username(String)}; setting this to non-<code>null
+     * </code> will unset {@link #username(String)}.
+     *
+     * <p>If both this and {@link #username(String)} are empty, this indicates that this is a
+     * request for an assertion by a <a
+     * href="https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#client-side-discoverable-public-key-credential-source">client-side-resident
+     * credential</a>, and identification of the user has been deferred until the response is
+     * received.
+     */
+    public AssertionRequestBuilder userHandle(ByteArray userHandle) {
+      if (userHandle != null) {
+        this.username = null;
+      }
+      this.userHandle = userHandle;
       return this;
     }
   }
diff --git a/webauthn-server-core/src/main/java/com/yubico/webauthn/FinishAssertionSteps.java b/webauthn-server-core/src/main/java/com/yubico/webauthn/FinishAssertionSteps.java
@@ -119,10 +119,11 @@ default AssertionResult run() throws InvalidSignatureCountException {
   class Step0 implements Step<Step1> {
 
     private final Optional<ByteArray> userHandle =
-        response
-            .getResponse()
+        request
             .getUserHandle()
             .map(Optional::of)
+            .orElseGet(() -> response.getResponse().getUserHandle())
+            .map(Optional::of)
             .orElseGet(
                 () ->
                     request.getUsername().flatMap(credentialRepository::getUserHandleForUsername));
@@ -131,12 +132,7 @@ class Step0 implements Step<Step1> {
         request
             .getUsername()
             .map(Optional::of)
-            .orElseGet(
-                () ->
-                    response
-                        .getResponse()
-                        .getUserHandle()
-                        .flatMap(credentialRepository::getUsernameForUserHandle));
+            .orElseGet(() -> userHandle.flatMap(credentialRepository::getUsernameForUserHandle));
 
     @Override
     public Step1 nextStep() {
@@ -146,8 +142,18 @@ public Step1 nextStep() {
     @Override
     public void validate() {
       assure(
-          request.getUsername().isPresent() || response.getResponse().getUserHandle().isPresent(),
+          request.getUsername().isPresent()
+              || request.getUserHandle().isPresent()
+              || response.getResponse().getUserHandle().isPresent(),
           "At least one of username and user handle must be given; none was.");
+      if (request.getUserHandle().isPresent()
+          && response.getResponse().getUserHandle().isPresent()) {
+        assure(
+            request.getUserHandle().get().equals(response.getResponse().getUserHandle().get()),
+            "User handle set in request (%s) does not match user handle in response (%s).",
+            request.getUserHandle().get(),
+            response.getResponse().getUserHandle().get());
+      }
       assure(
           userHandle.isPresent(),
           "User handle not found for username: %s",
diff --git a/webauthn-server-core/src/main/java/com/yubico/webauthn/RelyingParty.java b/webauthn-server-core/src/main/java/com/yubico/webauthn/RelyingParty.java
@@ -483,6 +483,7 @@ public AssertionRequest startAssertion(StartAssertionOptions startAssertionOptio
     return AssertionRequest.builder()
         .publicKeyCredentialRequestOptions(pkcro.build())
         .username(startAssertionOptions.getUsername())
+        .userHandle(startAssertionOptions.getUserHandle())
         .build();
   }
 
diff --git a/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyAssertionSpec.scala b/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyAssertionSpec.scala
@@ -180,6 +180,7 @@ class RelyingPartyAssertionSpec
       rpId: RelyingPartyIdentity = Defaults.rpId,
       signature: ByteArray = Defaults.signature,
       userHandleForResponse: Option[ByteArray] = Some(Defaults.userHandle),
+      userHandleForRequest: Option[ByteArray] = None,
       userHandleForUser: ByteArray = Defaults.userHandle,
       usernameForRequest: Option[String] = Some(Defaults.username),
       usernameForUser: String = Defaults.username,
@@ -205,6 +206,7 @@ class RelyingPartyAssertionSpec
           .build()
       )
       .username(usernameForRequest.asJava)
+      .userHandle(userHandleForRequest.asJava)
       .build()
 
     val response = PublicKeyCredential
@@ -521,9 +523,9 @@ class RelyingPartyAssertionSpec
             ) {
               val steps = finishAssertion(
                 credentialRepository = credentialRepository,
-                usernameForRequest = Some(owner.username),
-                userHandleForUser = owner.userHandle,
                 userHandleForResponse = Some(nonOwner.userHandle),
+                userHandleForUser = owner.userHandle,
+                usernameForRequest = Some(owner.username),
               )
               val step: FinishAssertionSteps#Step2 = steps.begin.next.next
 
@@ -535,9 +537,9 @@ class RelyingPartyAssertionSpec
             it("Succeeds if credential ID is owned by the given user handle.") {
               val steps = finishAssertion(
                 credentialRepository = credentialRepository,
-                usernameForRequest = Some(owner.username),
-                userHandleForUser = owner.userHandle,
                 userHandleForResponse = Some(owner.userHandle),
+                userHandleForUser = owner.userHandle,
+                usernameForRequest = Some(owner.username),
               )
               val step: FinishAssertionSteps#Step2 = steps.begin.next.next
 
@@ -548,13 +550,30 @@ class RelyingPartyAssertionSpec
 
           describe("If the user was not identified before the authentication ceremony was initiated, verify that credential.response.userHandle is present, and that the user identified by this value is the owner of credentialSource.") {
             it(
-              "Fails if credential ID is not owned by the given user handle."
+              "Fails if credential ID is not owned by the user handle in the response."
             ) {
               val steps = finishAssertion(
                 credentialRepository = credentialRepository,
+                userHandleForResponse = Some(nonOwner.userHandle),
+                userHandleForUser = owner.userHandle,
                 usernameForRequest = None,
+              )
+              val step: FinishAssertionSteps#Step2 = steps.begin.next.next
+
+              step.validations shouldBe a[Failure[_]]
+              step.validations.failed.get shouldBe an[IllegalArgumentException]
+              step.tryNext shouldBe a[Failure[_]]
+            }
+
+            it(
+              "Fails if credential ID is not owned by the user handle in the request."
+            ) {
+              val steps = finishAssertion(
+                credentialRepository = credentialRepository,
+                userHandleForRequest = Some(nonOwner.userHandle),
+                userHandleForResponse = None,
                 userHandleForUser = owner.userHandle,
-                userHandleForResponse = Some(nonOwner.userHandle),
+                usernameForRequest = None,
               )
               val step: FinishAssertionSteps#Step2 = steps.begin.next.next
 
@@ -566,9 +585,25 @@ class RelyingPartyAssertionSpec
             it("Fails if neither username nor user handle is given.") {
               val steps = finishAssertion(
                 credentialRepository = credentialRepository,
+                userHandleForRequest = None,
+                userHandleForResponse = None,
+                userHandleForUser = owner.userHandle,
                 usernameForRequest = None,
+              )
+              val step: FinishAssertionSteps#Step0 = steps.begin
+
+              step.validations shouldBe a[Failure[_]]
+              step.validations.failed.get shouldBe an[IllegalArgumentException]
+              step.tryNext shouldBe a[Failure[_]]
+            }
+
+            it("Fails if user handle in request does not agree with user handle in response.") {
+              val steps = finishAssertion(
+                credentialRepository = credentialRepository,
+                userHandleForRequest = Some(owner.userHandle),
+                userHandleForResponse = Some(nonOwner.userHandle),
                 userHandleForUser = owner.userHandle,
-                userHandleForResponse = None,
+                usernameForRequest = None,
               )
               val step: FinishAssertionSteps#Step0 = steps.begin
 
@@ -577,12 +612,26 @@ class RelyingPartyAssertionSpec
               step.tryNext shouldBe a[Failure[_]]
             }
 
-            it("Succeeds if credential ID is owned by the given user handle.") {
+            it("Succeeds if credential ID is owned by the user handle in the response.") {
               val steps = finishAssertion(
                 credentialRepository = credentialRepository,
+                userHandleForResponse = Some(owner.userHandle),
+                userHandleForUser = owner.userHandle,
                 usernameForRequest = None,
+              )
+              val step: FinishAssertionSteps#Step2 = steps.begin.next.next
+
+              step.validations shouldBe a[Success[_]]
+              step.tryNext shouldBe a[Success[_]]
+            }
+
+            it("Succeeds if credential ID is owned by the user handle in the request.") {
+              val steps = finishAssertion(
+                credentialRepository = credentialRepository,
+                userHandleForRequest = Some(owner.userHandle),
+                userHandleForResponse = None,
                 userHandleForUser = owner.userHandle,
-                userHandleForResponse = Some(owner.userHandle),
+                usernameForRequest = None,
               )
               val step: FinishAssertionSteps#Step2 = steps.begin.next.next
 
diff --git a/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyStartOperationSpec.scala b/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyStartOperationSpec.scala

Original file line number	Diff line number	Diff line change
`@@ -483,6 +483,7 @@ public AssertionRequest startAssertion(StartAssertionOptions startAssertionOptio`
`483`	`483`	`return AssertionRequest.builder()`
`484`	`484`	`.publicKeyCredentialRequestOptions(pkcro.build())`
`485`	`485`	`.username(startAssertionOptions.getUsername())`
	`486`	`+ .userHandle(startAssertionOptions.getUserHandle())`
`486`	`487`	`.build();`
`487`	`488`	`}`
`488`	`489`