From aabacc5926b9e79aaccc32e52beb1f9accd9938d Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Thu, 16 Jan 2025 14:47:56 +0100 Subject: [PATCH] fixup! Revert new experimental interfaces and classes --- .../yubico/webauthn/FinishAssertionSteps.java | 147 ++++++++---------- .../webauthn/RelyingPartyAssertionSpec.scala | 47 ++---- .../webauthn/InMemoryRegistrationStorage.java | 88 ++++++----- .../webauthn/data/CredentialRegistration.java | 6 - 4 files changed, 131 insertions(+), 157 deletions(-) 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 57d51c9be..79cf442db 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 @@ -142,91 +142,82 @@ public void validate() { @Value class Step6 implements Step { - private final Optional requestedUserHandle; - private final Optional requestedUsername; - private final Optional responseUserHandle; - - private final Optional effectiveRequestUserHandle; - private final Optional effectiveRequestUsername; - private final boolean userHandleDerivedFromUsername; - - private final Optional finalUserHandle; - private final Optional finalUsername; - private final Optional registration; - - public Step6() { - requestedUserHandle = request.getUserHandle(); - requestedUsername = request.getUsername(); - responseUserHandle = response.getResponse().getUserHandle(); - - effectiveRequestUserHandle = - OptionalUtil.orElseOptional( - requestedUserHandle, - () -> requestedUsername.flatMap(credentialRepository::getUserHandleForUsername)); - - effectiveRequestUsername = - OptionalUtil.orElseOptional( - requestedUsername, - () -> - requestedUserHandle.flatMap(FinishAssertionSteps.this::getUsernameForUserHandle)); - - userHandleDerivedFromUsername = - !requestedUserHandle.isPresent() && effectiveRequestUserHandle.isPresent(); - - finalUserHandle = OptionalUtil.orOptional(effectiveRequestUserHandle, responseUserHandle); - finalUsername = - OptionalUtil.orElseOptional( - effectiveRequestUsername, - () -> finalUserHandle.flatMap(FinishAssertionSteps.this::getUsernameForUserHandle)); - - registration = - finalUserHandle.flatMap(uh -> credentialRepository.lookup(response.getId(), uh)); - } + private final Optional userHandle = + OptionalUtil.orElseOptional( + request.getUserHandle(), + () -> + OptionalUtil.orElseOptional( + response.getResponse().getUserHandle(), + () -> + request + .getUsername() + .flatMap(credentialRepository::getUserHandleForUsername))); + + private final Optional username = + OptionalUtil.orElseOptional( + request.getUsername(), + () -> userHandle.flatMap(credentialRepository::getUsernameForUserHandle)); + + private final Optional registration = + userHandle.flatMap(uh -> credentialRepository.lookup(response.getId(), uh)); @Override public Step7 nextStep() { - return new Step7(finalUsername, finalUserHandle.get(), registration); + return new Step7(username.get(), userHandle.get(), registration); } @Override public void validate() { assertTrue( - finalUserHandle.isPresent(), - "Could not identify user to authenticate: none of requested username, requested user handle or response user handle are set."); - - if (requestedUserHandle.isPresent() && responseUserHandle.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( - requestedUserHandle.get().equals(responseUserHandle.get()), + request.getUserHandle().get().equals(response.getResponse().getUserHandle().get()), "User handle set in request (%s) does not match user handle in response (%s).", - requestedUserHandle.get(), - responseUserHandle.get()); + request.getUserHandle().get(), + response.getResponse().getUserHandle().get()); } - if (userHandleDerivedFromUsername && responseUserHandle.isPresent()) { - assertTrue( - effectiveRequestUserHandle.get().equals(responseUserHandle.get()), - "User handle in request (%s) (derived from username: %s) does not match user handle in response (%s).", - effectiveRequestUserHandle.get(), - requestedUsername.get(), - responseUserHandle.get()); - } + assertTrue( + userHandle.isPresent(), + "User handle not found for username: %s", + request.getUsername(), + response.getResponse().getUserHandle()); + + assertTrue( + username.isPresent(), + "Username not found for userHandle: %s", + request.getUsername(), + response.getResponse().getUserHandle()); assertTrue(registration.isPresent(), "Unknown credential: %s", response.getId()); assertTrue( - finalUserHandle.get().equals(registration.get().getUserHandle()), + userHandle.get().equals(registration.get().getUserHandle()), "User handle %s does not own credential %s", - finalUserHandle.get(), + userHandle.get(), response.getId()); - assertTrue( - finalUsername.isPresent(), "Unknown username for user handle: %s", finalUserHandle.get()); + final Optional usernameFromRequest = request.getUsername(); + final Optional userHandleFromResponse = response.getResponse().getUserHandle(); + if (usernameFromRequest.isPresent() && userHandleFromResponse.isPresent()) { + assertTrue( + userHandleFromResponse.equals( + credentialRepository.getUserHandleForUsername(usernameFromRequest.get())), + "User handle %s in response does not match username %s in request", + userHandleFromResponse, + usernameFromRequest); + } } } @Value class Step7 implements Step { - private final Optional username; + private final String username; private final ByteArray userHandle; private final Optional credential; @@ -248,7 +239,7 @@ public void validate() { @Value class Step8 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -280,7 +271,7 @@ public ByteArray signature() { @Value class Step10 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -300,7 +291,7 @@ public CollectedClientData clientData() { @Value class Step11 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; private final CollectedClientData clientData; @@ -323,7 +314,7 @@ public Step12 nextStep() { @Value class Step12 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -344,7 +335,7 @@ public Step13 nextStep() { @Value class Step13 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -364,7 +355,7 @@ public Step14 nextStep() { @Value class Step14 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -381,7 +372,7 @@ public Step15 nextStep() { @Value class Step15 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -413,7 +404,7 @@ public Step16 nextStep() { @Value class Step16 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -431,7 +422,7 @@ public Step17 nextStep() { @Value class Step17 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -456,7 +447,7 @@ public PendingStep16 nextStep() { // Step 16 in editor's draft as of 2022-11-09 https://w3c.github.io/webauthn/ // TODO: Finalize this when spec matures class PendingStep16 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -479,7 +470,7 @@ public Step18 nextStep() { @Value class Step18 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -493,7 +484,7 @@ public Step19 nextStep() { @Value class Step19 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; @Override @@ -513,7 +504,7 @@ public ByteArray clientDataJsonHash() { @Value class Step20 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; private final ByteArray clientDataJsonHash; @@ -558,12 +549,12 @@ public ByteArray signedBytes() { @Value class Step21 implements Step { - private final Optional username; + private final String username; private final RegisteredCredential credential; private final long assertionSignatureCount; private final long storedSignatureCountBefore; - public Step21(Optional username, RegisteredCredential credential) { + public Step21(String username, RegisteredCredential credential) { this.username = username; this.credential = credential; this.assertionSignatureCount = @@ -593,7 +584,7 @@ public Finished nextStep() { @Value class Finished implements Step { private final RegisteredCredential credential; - private final Optional username; + private final String username; private final long assertionSignatureCount; private final boolean signatureCounterValid; @@ -610,7 +601,7 @@ public Finished nextStep() { @Override public Optional result() { return Optional.of( - new AssertionResult(true, response, credential, username.get(), signatureCounterValid)); + new AssertionResult(true, response, credential, username, signatureCounterValid)); } } } 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 e6c57be86..8e1eb804d 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 @@ -577,8 +577,7 @@ class RelyingPartyAssertionSpec ), credentialId = new ByteArray(Array(0, 1, 2, 3)), ) - val step: FinishAssertionSteps#Step5 = - steps.begin + val step: FinishAssertionSteps#Step5 = steps.begin step.validations shouldBe a[Failure[_]] step.validations.failed.get shouldBe an[IllegalArgumentException] @@ -601,8 +600,7 @@ class RelyingPartyAssertionSpec ), credentialId = new ByteArray(Array(4, 5, 6, 7)), ) - val step: FinishAssertionSteps#Step5 = - steps.begin + val step: FinishAssertionSteps#Step5 = steps.begin step.validations shouldBe a[Success[_]] step.tryNext shouldBe a[Success[_]] @@ -619,8 +617,7 @@ class RelyingPartyAssertionSpec allowCredentials = allowCredentials, credentialId = new ByteArray(Array(0, 1, 2, 3)), ) - val step: FinishAssertionSteps#Step5 = - steps.begin + val step: FinishAssertionSteps#Step5 = steps.begin step.validations shouldBe a[Success[_]] step.tryNext shouldBe a[Success[_]] @@ -677,8 +674,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, usernameForRequest = Some(owner.username), ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Failure[_]] step.validations.failed.get shouldBe an[IllegalArgumentException] @@ -694,8 +690,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, userHandleForResponse = Some(owner.userHandle), ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Failure[_]] step.validations.failed.get shouldBe an[IllegalArgumentException] @@ -709,8 +704,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, usernameForRequest = Some(owner.username), ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Success[_]] step.tryNext shouldBe a[Success[_]] @@ -727,8 +721,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, userHandleForResponse = None, ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Failure[_]] step.validations.failed.get shouldBe an[IllegalArgumentException] @@ -744,8 +737,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, usernameForRequest = None, ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Failure[_]] step.validations.failed.get shouldBe an[IllegalArgumentException] @@ -762,8 +754,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, usernameForRequest = None, ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Failure[_]] step.validations.failed.get shouldBe an[IllegalArgumentException] @@ -778,8 +769,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, usernameForRequest = None, ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Failure[_]] step.validations.failed.get shouldBe an[IllegalArgumentException] @@ -794,8 +784,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, usernameForRequest = None, ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Failure[_]] step.validations.failed.get shouldBe an[IllegalArgumentException] @@ -809,8 +798,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, usernameForRequest = None, ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Success[_]] step.tryNext shouldBe a[Success[_]] @@ -824,8 +812,7 @@ class RelyingPartyAssertionSpec userHandleForUser = owner.userHandle, usernameForRequest = None, ) - val step: FinishAssertionSteps#Step6 = - steps.begin.next + val step: FinishAssertionSteps#Step6 = steps.begin.next step.validations shouldBe a[Success[_]] step.tryNext shouldBe a[Success[_]] @@ -852,7 +839,7 @@ class RelyingPartyAssertionSpec ) ) val step: steps.Step7 = new steps.Step7( - Some(Defaults.username).toJava, + Defaults.username, Defaults.userHandle, None.toJava, ) @@ -877,8 +864,7 @@ class RelyingPartyAssertionSpec ) ) ) - val step: FinishAssertionSteps#Step7 = - steps.begin.next.next + val step: FinishAssertionSteps#Step7 = steps.begin.next.next step.validations shouldBe a[Success[_]] step.tryNext shouldBe a[Success[_]] @@ -888,8 +874,7 @@ class RelyingPartyAssertionSpec describe("8. Let cData, authData and sig denote the value of response’s clientDataJSON, authenticatorData, and signature respectively.") { it("Succeeds if all three are present.") { val steps = finishAssertion() - val step: FinishAssertionSteps#Step8 = - steps.begin.next.next.next + val step: FinishAssertionSteps#Step8 = steps.begin.next.next.next step.validations shouldBe a[Success[_]] step.clientData should not be null diff --git a/webauthn-server-demo/src/main/java/demo/webauthn/InMemoryRegistrationStorage.java b/webauthn-server-demo/src/main/java/demo/webauthn/InMemoryRegistrationStorage.java index c91054486..0cba71a9c 100644 --- a/webauthn-server-demo/src/main/java/demo/webauthn/InMemoryRegistrationStorage.java +++ b/webauthn-server-demo/src/main/java/demo/webauthn/InMemoryRegistrationStorage.java @@ -26,15 +26,14 @@ import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; +import com.yubico.internal.util.CollectionUtil; import com.yubico.webauthn.AssertionResult; import com.yubico.webauthn.CredentialRepository; import com.yubico.webauthn.RegisteredCredential; import com.yubico.webauthn.data.ByteArray; import com.yubico.webauthn.data.PublicKeyCredentialDescriptor; -import com.yubico.webauthn.data.PublicKeyCredentialType; import demo.webauthn.data.CredentialRegistration; import java.util.Collection; -import java.util.Collections; import java.util.HashSet; import java.util.NoSuchElementException; import java.util.Optional; @@ -58,20 +57,28 @@ public class InMemoryRegistrationStorage implements CredentialRepository { @Override public Set getCredentialIdsForUsername(String username) { - return getUserHandleForUsername(username) - .map(this::getRegistrationsByUserHandle) + return getRegistrationsByUsername(username).stream() .map( - regs -> - regs.stream() - .map( - reg -> - PublicKeyCredentialDescriptor.builder() - .id(reg.getCredential().getCredentialId()) - .type(PublicKeyCredentialType.PUBLIC_KEY) - .transports(reg.getTransports()) - .build()) - .collect(Collectors.toSet())) - .orElseGet(Collections::emptySet); + registration -> + PublicKeyCredentialDescriptor.builder() + .id(registration.getCredential().getCredentialId()) + .transports(registration.getTransports()) + .build()) + .collect(Collectors.toSet()); + } + + @Override + public Optional getUsernameForUserHandle(ByteArray userHandle) { + return getRegistrationsByUserHandle(userHandle).stream() + .findAny() + .map(CredentialRegistration::getUsername); + } + + @Override + public Optional getUserHandleForUsername(String username) { + return getRegistrationsByUsername(username).stream() + .findAny() + .map(reg -> reg.getUserIdentity().getId()); } @Override @@ -79,10 +86,7 @@ public Optional lookup(ByteArray credentialId, ByteArray u Optional registrationMaybe = storage.asMap().values().stream() .flatMap(Collection::stream) - .filter( - credReg -> - credentialId.equals(credReg.getCredential().getCredentialId()) - && userHandle.equals(credReg.getUserHandle())) + .filter(credReg -> credentialId.equals(credReg.getCredential().getCredentialId())) .findAny(); logger.debug( @@ -90,31 +94,31 @@ public Optional lookup(ByteArray credentialId, ByteArray u credentialId, userHandle, registrationMaybe); - - return registrationMaybe.map(CredentialRegistration::getCredential); + return registrationMaybe.map( + registration -> + RegisteredCredential.builder() + .credentialId(registration.getCredential().getCredentialId()) + .userHandle(registration.getUserIdentity().getId()) + .publicKeyCose(registration.getCredential().getPublicKeyCose()) + .signatureCount(registration.getCredential().getSignatureCount()) + .build()); } @Override public Set lookupAll(ByteArray credentialId) { - return storage.asMap().values().stream() - .flatMap(Collection::stream) - .map(CredentialRegistration::getCredential) - .filter(credential -> credential.getCredentialId().equals(credentialId)) - .collect(Collectors.toSet()); - } - - @Override - public Optional getUserHandleForUsername(String username) { - return getRegistrationsByUsername(username).stream() - .findAny() - .map(reg -> reg.getUserIdentity().getId()); - } - - @Override - public Optional getUsernameForUserHandle(ByteArray userHandle) { - return getRegistrationsByUserHandle(userHandle).stream() - .findAny() - .map(CredentialRegistration::getUsername); + return CollectionUtil.immutableSet( + storage.asMap().values().stream() + .flatMap(Collection::stream) + .filter(reg -> reg.getCredential().getCredentialId().equals(credentialId)) + .map( + reg -> + RegisteredCredential.builder() + .credentialId(reg.getCredential().getCredentialId()) + .userHandle(reg.getUserIdentity().getId()) + .publicKeyCose(reg.getCredential().getPublicKeyCose()) + .signatureCount(reg.getCredential().getSignatureCount()) + .build()) + .collect(Collectors.toSet())); } //////////////////////////////////////////////////////////////////////////////// @@ -139,13 +143,13 @@ public Collection getRegistrationsByUsername(String user } } - public Set getRegistrationsByUserHandle(ByteArray userHandle) { + public Collection getRegistrationsByUserHandle(ByteArray userHandle) { return storage.asMap().values().stream() .flatMap(Collection::stream) .filter( credentialRegistration -> userHandle.equals(credentialRegistration.getUserIdentity().getId())) - .collect(Collectors.toSet()); + .collect(Collectors.toList()); } public void updateSignatureCount(AssertionResult result) { diff --git a/webauthn-server-demo/src/main/java/demo/webauthn/data/CredentialRegistration.java b/webauthn-server-demo/src/main/java/demo/webauthn/data/CredentialRegistration.java index a17e17e2c..ef5878821 100644 --- a/webauthn-server-demo/src/main/java/demo/webauthn/data/CredentialRegistration.java +++ b/webauthn-server-demo/src/main/java/demo/webauthn/data/CredentialRegistration.java @@ -28,13 +28,11 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.yubico.webauthn.RegisteredCredential; import com.yubico.webauthn.data.AuthenticatorTransport; -import com.yubico.webauthn.data.ByteArray; import com.yubico.webauthn.data.UserIdentity; import java.time.Instant; import java.util.Optional; import java.util.SortedSet; import lombok.Builder; -import lombok.NonNull; import lombok.Value; import lombok.With; @@ -60,8 +58,4 @@ public String getRegistrationTimestamp() { public String getUsername() { return userIdentity.getName(); } - - public @NonNull ByteArray getUserHandle() { - return userIdentity.getId(); - } }