diff --git a/NEWS b/NEWS index 7a8410a81..ac05bf270 100644 --- 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 index 13f29b89b..c531b8131 100644 --- 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. * - *

If this is absent, this indicates that this is a request for an assertion by a 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. + * + *

If both this and {@link #getUserHandle() userHandle} are empty, this indicates that this is + * a request for an assertion by a client-side-resident * credential, 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. + * + *

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}. + * + *

If both this and {@link #getUsername() username} are empty, this indicates that this is a + * request for an assertion by a client-side-resident + * credential, 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. * - *

If this is absent, this indicates that this is a request for an assertion by a This is mutually exclusive with {@link #getUserHandle()}; if this is present, then {@link + * #getUserHandle()} will be empty. + * + *

If both this and {@link #getUserHandle()} are empty, this indicates that this is a request + * for an assertion by a client-side-resident * credential, and identification of the user has been deferred until the response is * received. @@ -80,6 +112,22 @@ public Optional getUsername() { return Optional.ofNullable(username); } + /** + * The user handle of the user to authenticate, if the user has already been identified. + * + *

This is mutually exclusive with {@link #getUsername()}; if this is present, then {@link + * #getUsername()} will be empty. + * + *

If both this and {@link #getUsername()} are empty, this indicates that this is a request for + * an assertion by a client-side-resident + * credential, and identification of the user has been deferred until the response is + * received. + */ + public Optional 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. * - *

If this is absent, this indicates that this is a request for an assertion by a This is mutually exclusive with {@link #userHandle(ByteArray)}; setting this to non-empty + * will unset {@link #userHandle(ByteArray)}. + * + *

If this is empty, this indicates that this is a request for an assertion by a client-side-resident * credential, and identification of the user has been deferred until the response is * received. @@ -173,13 +225,55 @@ public AssertionRequestBuilder username(@NonNull Optional username) { /** * The username of the user to authenticate, if the user has already been identified. * - *

If this is absent, this indicates that this is a request for an assertion by a This is mutually exclusive with {@link #userHandle(ByteArray)}; setting this to non- + * null will unset {@link #userHandle(ByteArray)}. + * + *

If this is empty, this indicates that this is a request for an assertion by a client-side-resident * credential, 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. + * + *

This is mutually exclusive with {@link #username(String)}; setting this to non-empty will + * unset {@link #username(String)}. + * + *

If both this and {@link #username(String)} are empty, this indicates that this is a + * request for an assertion by a client-side-resident + * credential, and identification of the user has been deferred until the response is + * received. + */ + public AssertionRequestBuilder userHandle(@NonNull Optional userHandle) { + return this.userHandle(userHandle.orElse(null)); + } + + /** + * The user handle of the user to authenticate, if the user has already been identified. + * + *

This is mutually exclusive with {@link #username(String)}; setting this to non-null + * will unset {@link #username(String)}. + * + *

If both this and {@link #username(String)} are empty, this indicates that this is a + * request for an assertion by a client-side-resident + * credential, 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 index 04024cad7..49e8ccec5 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 @@ -119,10 +119,11 @@ default AssertionResult run() throws InvalidSignatureCountException { class Step0 implements Step { private final Optional 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 { 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 index 1242b1906..1faf6420e 100644 --- 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 index e65eb6ffe..00ab8acbe 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 @@ -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 index 84602be6d..ec05dcb0b 100644 --- a/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyStartOperationSpec.scala +++ b/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyStartOperationSpec.scala @@ -599,6 +599,34 @@ class RelyingPartyStartOperationSpec } } + it("passes username through to AssertionRequest.") { + forAll { username: String => + val testCaseUserId = userId.toBuilder.name(username).build() + val rp = relyingParty(userId = testCaseUserId) + val result = rp.startAssertion( + StartAssertionOptions + .builder() + .username(testCaseUserId.getName) + .build() + ) + result.getUsername.asScala should equal(Some(testCaseUserId.getName)) + } + } + + it("passes user handle through to AssertionRequest.") { + forAll { userHandle: ByteArray => + val testCaseUserId = userId.toBuilder.id(userHandle).build() + val rp = relyingParty(userId = testCaseUserId) + val result = rp.startAssertion( + StartAssertionOptions + .builder() + .userHandle(testCaseUserId.getId) + .build() + ) + result.getUserHandle.asScala should equal(Some(testCaseUserId.getId)) + } + } + it("includes transports in allowCredentials when available.") { forAll( Gen.nonEmptyContainerOf[Set, AuthenticatorTransport]( @@ -911,4 +939,95 @@ class RelyingPartyStartOperationSpec } } + describe("AssertionRequest") { + + it("resets username when userHandle is set.") { + forAll { (ar: AssertionRequest, userHandle: ByteArray) => + val result = ar.toBuilder.userHandle(userHandle).build() + result.getUsername.asScala shouldBe empty + } + + forAll { (ar: AssertionRequest, userHandle: ByteArray) => + val result = ar.toBuilder.userHandle(Some(userHandle).asJava).build() + result.getUsername.asScala shouldBe empty + } + } + + it("resets userHandle when username is set.") { + forAll { (ar: AssertionRequest, username: String) => + val result = ar.toBuilder.username(username).build() + result.getUserHandle.asScala shouldBe empty + } + + forAll { (ar: AssertionRequest, username: String) => + val result = ar.toBuilder.username(Some(username).asJava).build() + result.getUserHandle.asScala shouldBe empty + } + } + + it("does not reset username when userHandle is set to empty.") { + forAll { (ar: AssertionRequest, username: String) => + val result = ar.toBuilder + .username(username) + .userHandle(Optional.empty[ByteArray]) + .build() + result.getUsername.asScala should equal(Some(username)) + } + + forAll { (ar: AssertionRequest, username: String) => + val result = ar.toBuilder + .username(username) + .userHandle(null: ByteArray) + .build() + result.getUsername.asScala should equal(Some(username)) + } + } + + it("does not reset userHandle when username is set to empty.") { + forAll { (ar: AssertionRequest, userHandle: ByteArray) => + val result = ar.toBuilder + .userHandle(userHandle) + .username(Optional.empty[String]) + .build() + result.getUserHandle.asScala should equal(Some(userHandle)) + } + + forAll { (ar: AssertionRequest, userHandle: ByteArray) => + val result = ar.toBuilder + .userHandle(userHandle) + .username(null: String) + .build() + result.getUserHandle.asScala should equal(Some(userHandle)) + } + } + + it("allows unsetting username.") { + forAll { (ar: AssertionRequest, username: String) => + val preresult = ar.toBuilder.username(username).build() + preresult.getUsername.asScala should equal(Some(username)) + + val result1 = + preresult.toBuilder.username(Optional.empty[String]).build() + result1.getUsername.asScala shouldBe empty + + val result2 = preresult.toBuilder.username(null: String).build() + result2.getUsername.asScala shouldBe empty + } + } + + it("allows unsetting userHandle.") { + forAll { (ar: AssertionRequest, userHandle: ByteArray) => + val preresult = ar.toBuilder.userHandle(userHandle).build() + preresult.getUserHandle.asScala should equal(Some(userHandle)) + + val result1 = + preresult.toBuilder.userHandle(Optional.empty[ByteArray]).build() + result1.getUserHandle.asScala shouldBe empty + + val result2 = preresult.toBuilder.userHandle(null: ByteArray).build() + result2.getUserHandle.asScala shouldBe empty + } + } + } + }