From c81c9a8841c70854b385f2bf8e4f97bbb0778b09 Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Thu, 31 Mar 2022 11:52:50 +0200 Subject: [PATCH] Fix NoSuchElementException when username and user handle are both absent --- NEWS | 3 +++ .../yubico/webauthn/FinishAssertionSteps.java | 13 ++++++---- .../webauthn/RelyingPartyAssertionSpec.scala | 26 ++++++++++++++----- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/NEWS b/NEWS index 85e47c2c0..7a8410a81 100644 --- 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 index 4f5ba22a9..04024cad7 100644 --- 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,7 +124,8 @@ class Step0 implements Step { .getUserHandle() .map(Optional::of) .orElseGet( - () -> credentialRepository.getUserHandleForUsername(request.getUsername().get())); + () -> + request.getUsername().flatMap(credentialRepository::getUserHandleForUsername)); private final Optional username = request @@ -132,8 +133,10 @@ class Step0 implements Step { .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 index b5ed3ff7f..e65eb6ffe 100644 --- 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