Merge tag '1.12.3-RC3'

emlun · emlun · commit a9fcd89b25c2 · 2023-01-20T15:53:45.000+01:00
This pre-release was accidentally left out of release 1.12.3 somehow.
diff --git a/NEWS b/NEWS
@@ -1,4 +1,4 @@
-== Version 2.3.1 (unreleased) ==
+== Version 2.4.0 (unreleased) ==
 
 `webauthn-server-core`:
 
@@ -8,6 +8,20 @@ Fixes:
   configured, if the `aaguid` in the attestation data is zero, the call to
   `AttestationTrustSource.findTrustRoots` will fall back to reading the AAGUID
   from the attestation certificate if possible.
+* 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.
+  This was originally released in pre-release `1.12.3-RC3`, but was accidentally
+  left out of the `1.12.3` release.
+
+New features:
+
+* Added `userHandle` field to `AssertionRequest` as part of above bug fix.
+  `userHandle` is mutually exclusive with `username`. This was originally
+  released in pre-release `1.12.3-RC3`, but was accidentally left out of the
+  `1.12.3` release.
+
 
 `webauthn-server-attestation`:
 
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
@@ -124,10 +124,11 @@ public void validate() {
   class Step6 implements Step<Step7> {
 
     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));
@@ -136,12 +137,7 @@ class Step6 implements Step<Step7> {
         request
             .getUsername()
             .map(Optional::of)
-            .orElseGet(
-                () ->
-                    response
-                        .getResponse()
-                        .getUserHandle()
-                        .flatMap(credentialRepository::getUsernameForUserHandle));
+            .orElseGet(() -> userHandle.flatMap(credentialRepository::getUsernameForUserHandle));
 
     private final Optional<RegisteredCredential> registration =
         userHandle.flatMap(uh -> credentialRepository.lookup(response.getId(), uh));
@@ -154,8 +150,18 @@ public Step7 nextStep() {
     @Override
     public void validate() {
       assertTrue(
-          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()) {
+        assertTrue(
+            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());
+      }
 
       assertTrue(
           userHandle.isPresent(),
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
@@ -555,6 +555,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
@@ -185,6 +185,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,
@@ -210,6 +211,7 @@ class RelyingPartyAssertionSpec
           .build()
       )
       .username(usernameForRequest.toJava)
+      .userHandle(userHandleForRequest.toJava)
       .build()
 
     val response = PublicKeyCredential
@@ -648,9 +650,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#Step6 = steps.begin.next
 
@@ -678,9 +680,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#Step6 = steps.begin.next
 
@@ -707,13 +709,30 @@ class RelyingPartyAssertionSpec
             }
 
             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#Step6 = steps.begin.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#Step6 = steps.begin.next
 
@@ -725,9 +744,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#Step6 = steps.begin.next
+
+              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#Step6 = steps.begin.next
 
@@ -736,12 +771,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#Step6 = steps.begin.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#Step6 = steps.begin.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
`@@ -555,6 +555,7 @@ public AssertionRequest startAssertion(StartAssertionOptions startAssertionOptio`
`555`	`555`	`return AssertionRequest.builder()`
`556`	`556`	`.publicKeyCredentialRequestOptions(pkcro.build())`
`557`	`557`	`.username(startAssertionOptions.getUsername())`
	`558`	`+ .userHandle(startAssertionOptions.getUserHandle())`
`558`	`559`	`.build();`
`559`	`560`	`}`
`560`	`561`