Fix NoSuchElementException when username and user handle are both absent

Yubico · Mar 31, 2022 · c81c9a8 · c81c9a8
1 parent a30239d
commit c81c9a8
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 11 deletions.
diff --git a/NEWS b/NEWS
@@ -6,6 +6,9 @@ Fixes:
   `"authenticatorAttachment"` attribute was present.
 * Bumped Jackson dependency to version [2.13.2.1,3) in response to
   CVE-2020-36518
+* 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.
 
 
 == Version 1.12.2 ==

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,16 +124,19 @@ class Step0 implements Step<Step1> {
             .getUserHandle()
             .map(Optional::of)
             .orElseGet(
-                () -> credentialRepository.getUserHandleForUsername(request.getUsername().get()));
+                () ->
+                    request.getUsername().flatMap(credentialRepository::getUserHandleForUsername));
 
     private final Optional<String> username =
         request
             .getUsername()
             .map(Optional::of)
             .orElseGet(
                 () ->
-                    credentialRepository.getUsernameForUserHandle(
-                        response.getResponse().getUserHandle().get()));
+                    response
+                        .getResponse()
+                        .getUserHandle()
+                        .flatMap(credentialRepository::getUsernameForUserHandle));
 
     @Override
     public Step1 nextStep() {
@@ -147,12 +150,12 @@ public void validate() {
           "At least one of username and user handle must be given; none was.");
       assure(
           userHandle.isPresent(),
-          "No user found for username: %s, userHandle: %s",
+          "User handle not found for username: %s",
           request.getUsername(),
           response.getResponse().getUserHandle());
       assure(
           username.isPresent(),
-          "No user found for username: %s, userHandle: %s",
+          "Username not found for userHandle: %s",
           request.getUsername(),
           response.getResponse().getUserHandle());
     }

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
@@ -179,7 +179,7 @@ class RelyingPartyAssertionSpec
         Defaults.requestedExtensions,
       rpId: RelyingPartyIdentity = Defaults.rpId,
       signature: ByteArray = Defaults.signature,
-      userHandleForResponse: ByteArray = Defaults.userHandle,
+      userHandleForResponse: Option[ByteArray] = Some(Defaults.userHandle),
       userHandleForUser: ByteArray = Defaults.userHandle,
       usernameForRequest: Option[String] = Some(Defaults.username),
       usernameForUser: String = Defaults.username,
@@ -220,7 +220,7 @@ class RelyingPartyAssertionSpec
             if (clientDataJsonBytes == null) null else clientDataJsonBytes
           )
           .signature(if (signature == null) null else signature)
-          .userHandle(userHandleForResponse)
+          .userHandle(userHandleForResponse.asJava)
           .build()
       )
       .clientExtensionResults(clientExtensionResults)
@@ -523,7 +523,7 @@ class RelyingPartyAssertionSpec
                 credentialRepository = credentialRepository,
                 usernameForRequest = Some(owner.username),
                 userHandleForUser = owner.userHandle,
-                userHandleForResponse = nonOwner.userHandle,
+                userHandleForResponse = Some(nonOwner.userHandle),
               )
               val step: FinishAssertionSteps#Step2 = steps.begin.next.next
 
@@ -537,7 +537,7 @@ class RelyingPartyAssertionSpec
                 credentialRepository = credentialRepository,
                 usernameForRequest = Some(owner.username),
                 userHandleForUser = owner.userHandle,
-                userHandleForResponse = owner.userHandle,
+                userHandleForResponse = Some(owner.userHandle),
               )
               val step: FinishAssertionSteps#Step2 = steps.begin.next.next
 
@@ -554,7 +554,7 @@ class RelyingPartyAssertionSpec
                 credentialRepository = credentialRepository,
                 usernameForRequest = None,
                 userHandleForUser = owner.userHandle,
-                userHandleForResponse = nonOwner.userHandle,
+                userHandleForResponse = Some(nonOwner.userHandle),
               )
               val step: FinishAssertionSteps#Step2 = steps.begin.next.next
 
@@ -563,12 +563,26 @@ class RelyingPartyAssertionSpec
               step.tryNext shouldBe a[Failure[_]]
             }
 
+            it("Fails if neither username nor user handle is given.") {
+              val steps = finishAssertion(
+                credentialRepository = credentialRepository,
+                usernameForRequest = None,
+                userHandleForUser = owner.userHandle,
+                userHandleForResponse = 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("Succeeds if credential ID is owned by the given user handle.") {
               val steps = finishAssertion(
                 credentialRepository = credentialRepository,
                 usernameForRequest = None,
                 userHandleForUser = owner.userHandle,
-                userHandleForResponse = owner.userHandle,
+                userHandleForResponse = Some(owner.userHandle),
               )
               val step: FinishAssertionSteps#Step2 = steps.begin.next.next