From 0bc9313fddc1e758fc62ad9d9de9bc4654f90848 Mon Sep 17 00:00:00 2001 From: Borghi <137845283+Borghii@users.noreply.github.com> Date: Sun, 16 Feb 2025 22:50:34 -0300 Subject: [PATCH 1/8] Fix bug PublicKeyCredentialUserEntityRepository saves anonymousUser Issue gh-16385 Signed-off-by: Borghi <137845283+Borghii@users.noreply.github.com> --- .../Webauthn4JRelyingPartyOperations.java | 16 +++++++++++++--- .../Webauthn4jRelyingPartyOperationsTests.java | 12 ++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java index 4dc7efc5a8..98ef0cb77f 100644 --- a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java +++ b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java @@ -46,6 +46,7 @@ import com.webauthn4j.data.client.challenge.DefaultChallenge; import com.webauthn4j.data.extension.authenticator.RegistrationExtensionAuthenticatorOutput; import com.webauthn4j.server.ServerProperty; +import org.springframework.security.authentication.AnonymousAuthenticationToken; import org.springframework.security.authentication.AuthenticationTrustResolver; import org.springframework.security.authentication.AuthenticationTrustResolverImpl; import org.springframework.security.core.Authentication; @@ -333,9 +334,7 @@ public class Webauthn4JRelyingPartyOperations implements WebAuthnRelyingPartyOpe public PublicKeyCredentialRequestOptions createCredentialRequestOptions( PublicKeyCredentialRequestOptionsRequest request) { Authentication authentication = request.getAuthentication(); - // FIXME: do not load credentialRecords if anonymous - PublicKeyCredentialUserEntity userEntity = findUserEntityOrCreateAndSave(authentication.getName()); - List credentialRecords = this.userCredentials.findByUserId(userEntity.getId()); + List credentialRecords = findCredentialRecords(authentication); return PublicKeyCredentialRequestOptions.builder() .allowCredentials(credentialDescriptors(credentialRecords)) .challenge(Bytes.random()) @@ -346,6 +345,17 @@ public class Webauthn4JRelyingPartyOperations implements WebAuthnRelyingPartyOpe .build(); } + private List findCredentialRecords(Authentication authentication) { + if (authentication instanceof AnonymousAuthenticationToken) { + return Collections.emptyList(); + } + PublicKeyCredentialUserEntity userEntity = this.userEntities.findByUsername(authentication.getName()); + if (userEntity == null) { + return Collections.emptyList(); + } + return this.userCredentials.findByUserId(userEntity.getId()); + } + @Override public PublicKeyCredentialUserEntity authenticate(RelyingPartyAuthenticationRequest request) { PublicKeyCredentialRequestOptions requestOptions = request.getRequestOptions(); diff --git a/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java b/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java index 4061746e88..fcabb7f69f 100644 --- a/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java +++ b/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java @@ -536,6 +536,18 @@ class Webauthn4jRelyingPartyOperationsTests { .isEqualTo(creationOptions.getAuthenticatorSelection().getUserVerification()); } + @Test + void shouldReturnEmptyCredentialsWhenUserIsAnonymous() { + AnonymousAuthenticationToken authentication = new AnonymousAuthenticationToken("key", "anonymousUser", + Set.of(() -> "ROLE_ANONYMOUS")); + PublicKeyCredentialRequestOptionsRequest createRequest = new ImmutablePublicKeyCredentialRequestOptionsRequest( + authentication); + PublicKeyCredentialRequestOptions credentialRequestOptions = this.rpOperations + .createCredentialRequestOptions(createRequest); + + assertThat(credentialRequestOptions.getAllowCredentials()).isEmpty(); + } + private static AuthenticatorAttestationResponse setFlag(byte... flags) throws Exception { AuthenticatorAttestationResponseBuilder authAttResponseBldr = TestAuthenticatorAttestationResponse .createAuthenticatorAttestationResponse(); From e3a715b8f5c8b5000a24324de1f5cbe61c640b78 Mon Sep 17 00:00:00 2001 From: Borghi <137845283+Borghii@users.noreply.github.com> Date: Mon, 24 Mar 2025 12:58:08 -0300 Subject: [PATCH 2/8] Fix issues identified in PR review Signed-off-by: Borghi <137845283+Borghii@users.noreply.github.com> --- .../management/Webauthn4JRelyingPartyOperations.java | 5 +++-- .../Webauthn4jRelyingPartyOperationsTests.java | 11 ++++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java index 98ef0cb77f..e31cb44c0e 100644 --- a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java +++ b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -346,7 +346,8 @@ public class Webauthn4JRelyingPartyOperations implements WebAuthnRelyingPartyOpe } private List findCredentialRecords(Authentication authentication) { - if (authentication instanceof AnonymousAuthenticationToken) { + AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl(); + if (authentication == null || trustResolver.isAnonymous(authentication)) { return Collections.emptyList(); } PublicKeyCredentialUserEntity userEntity = this.userEntities.findByUsername(authentication.getName()); diff --git a/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java b/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java index fcabb7f69f..db2477ce54 100644 --- a/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java +++ b/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -548,6 +548,15 @@ class Webauthn4jRelyingPartyOperationsTests { assertThat(credentialRequestOptions.getAllowCredentials()).isEmpty(); } + @Test + void shouldReturnEmptyCredentialsWhenAnonymousUserIsDisabled() { + PublicKeyCredentialRequestOptionsRequest createRequest = new ImmutablePublicKeyCredentialRequestOptionsRequest(null); + PublicKeyCredentialRequestOptions credentialRequestOptions = this.rpOperations + .createCredentialRequestOptions(createRequest); + + assertThat(credentialRequestOptions.getAllowCredentials()).isEmpty(); + } + private static AuthenticatorAttestationResponse setFlag(byte... flags) throws Exception { AuthenticatorAttestationResponseBuilder authAttResponseBldr = TestAuthenticatorAttestationResponse .createAuthenticatorAttestationResponse(); From 5571ad1b27f3c46904b1b920aa70efb1d33811a7 Mon Sep 17 00:00:00 2001 From: Tomas Borghi <137845283+Borghii@users.noreply.github.com> Date: Mon, 24 Mar 2025 13:18:23 -0300 Subject: [PATCH 3/8] Fix issues identified in PR review Signed-off-by: Tomas Borghi <137845283+Borghii@users.noreply.github.com> --- .../webauthn/management/Webauthn4JRelyingPartyOperations.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java index e31cb44c0e..228a385a7f 100644 --- a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java +++ b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java @@ -346,8 +346,7 @@ public class Webauthn4JRelyingPartyOperations implements WebAuthnRelyingPartyOpe } private List findCredentialRecords(Authentication authentication) { - AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl(); - if (authentication == null || trustResolver.isAnonymous(authentication)) { + if (authentication == null || this.trustResolver.isAnonymous(authentication)) { return Collections.emptyList(); } PublicKeyCredentialUserEntity userEntity = this.userEntities.findByUsername(authentication.getName()); From 0a084135ecfeff64b9af6aba4f2a9536577799ca Mon Sep 17 00:00:00 2001 From: Tomas Borghi <137845283+Borghii@users.noreply.github.com> Date: Mon, 24 Mar 2025 16:50:39 -0300 Subject: [PATCH 4/8] Delete import unused Signed-off-by: Tomas Borghi <137845283+Borghii@users.noreply.github.com> --- .../webauthn/management/Webauthn4JRelyingPartyOperations.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java index 228a385a7f..38070587c8 100644 --- a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java +++ b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java @@ -46,7 +46,7 @@ import com.webauthn4j.data.client.challenge.DefaultChallenge; import com.webauthn4j.data.extension.authenticator.RegistrationExtensionAuthenticatorOutput; import com.webauthn4j.server.ServerProperty; -import org.springframework.security.authentication.AnonymousAuthenticationToken; + import org.springframework.security.authentication.AuthenticationTrustResolver; import org.springframework.security.authentication.AuthenticationTrustResolverImpl; import org.springframework.security.core.Authentication; From 4e20d56d2d2f65784a2aa1b2cb00e8123b1a77de Mon Sep 17 00:00:00 2001 From: Rob Winch <362503+rwinch@users.noreply.github.com> Date: Tue, 25 Mar 2025 13:55:15 -0500 Subject: [PATCH 5/8] Fix format for WebAuthn4jRelyingPartyOperations Issue gh-16385 --- .../webauthn/management/Webauthn4JRelyingPartyOperations.java | 1 - 1 file changed, 1 deletion(-) diff --git a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java index 38070587c8..cdd5065e36 100644 --- a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java +++ b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java @@ -46,7 +46,6 @@ import com.webauthn4j.data.client.challenge.DefaultChallenge; import com.webauthn4j.data.extension.authenticator.RegistrationExtensionAuthenticatorOutput; import com.webauthn4j.server.ServerProperty; - import org.springframework.security.authentication.AuthenticationTrustResolver; import org.springframework.security.authentication.AuthenticationTrustResolverImpl; import org.springframework.security.core.Authentication; From 593f7c44903a5c451357f8b5f8da0b74de40f194 Mon Sep 17 00:00:00 2001 From: Rob Winch <362503+rwinch@users.noreply.github.com> Date: Tue, 25 Mar 2025 13:49:20 -0500 Subject: [PATCH 6/8] Use !isAuthenticated It's more verbose to see if the user is not null and not anonymous Issue gh-16385 --- .../webauthn/management/Webauthn4JRelyingPartyOperations.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java index cdd5065e36..59d0729288 100644 --- a/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java +++ b/web/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java @@ -345,7 +345,7 @@ public class Webauthn4JRelyingPartyOperations implements WebAuthnRelyingPartyOpe } private List findCredentialRecords(Authentication authentication) { - if (authentication == null || this.trustResolver.isAnonymous(authentication)) { + if (!this.trustResolver.isAuthenticated(authentication)) { return Collections.emptyList(); } PublicKeyCredentialUserEntity userEntity = this.userEntities.findByUsername(authentication.getName()); From 9c054474a8b39c274d9d0971178e1c98d8e0058c Mon Sep 17 00:00:00 2001 From: Rob Winch <362503+rwinch@users.noreply.github.com> Date: Tue, 25 Mar 2025 13:50:53 -0500 Subject: [PATCH 7/8] Use Test Name Conventions Issue gh-16385 --- .../management/Webauthn4jRelyingPartyOperationsTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java b/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java index db2477ce54..f1b3eb7ded 100644 --- a/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java +++ b/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java @@ -537,7 +537,7 @@ class Webauthn4jRelyingPartyOperationsTests { } @Test - void shouldReturnEmptyCredentialsWhenUserIsAnonymous() { + void createCredentialRequestOptionsWhenAnonymousAuthentication() { AnonymousAuthenticationToken authentication = new AnonymousAuthenticationToken("key", "anonymousUser", Set.of(() -> "ROLE_ANONYMOUS")); PublicKeyCredentialRequestOptionsRequest createRequest = new ImmutablePublicKeyCredentialRequestOptionsRequest( @@ -549,7 +549,7 @@ class Webauthn4jRelyingPartyOperationsTests { } @Test - void shouldReturnEmptyCredentialsWhenAnonymousUserIsDisabled() { + void createCredentialRequestOptionsWhenNullAuthentication() { PublicKeyCredentialRequestOptionsRequest createRequest = new ImmutablePublicKeyCredentialRequestOptionsRequest(null); PublicKeyCredentialRequestOptions credentialRequestOptions = this.rpOperations .createCredentialRequestOptions(createRequest); From a6b5c05da919fb2a5cab02852cd186149a5cdb66 Mon Sep 17 00:00:00 2001 From: Rob Winch <362503+rwinch@users.noreply.github.com> Date: Tue, 25 Mar 2025 13:52:40 -0500 Subject: [PATCH 8/8] Additional WebAuthn4jRelyingPartyOperationTests - verify that anonymous users not saved - verify that when user found the CredentialRecord is allowed Issue gh-16385 --- ...Webauthn4jRelyingPartyOperationsTests.java | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java b/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java index f1b3eb7ded..733d7ec98b 100644 --- a/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java +++ b/web/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java @@ -42,6 +42,8 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.security.authentication.AnonymousAuthenticationToken; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.authority.AuthorityUtils; +import org.springframework.security.core.userdetails.PasswordEncodedUser; +import org.springframework.security.core.userdetails.UserDetails; import org.springframework.security.web.webauthn.api.AuthenticatorAttestationResponse; import org.springframework.security.web.webauthn.api.AuthenticatorAttestationResponse.AuthenticatorAttestationResponseBuilder; import org.springframework.security.web.webauthn.api.AuthenticatorSelectionCriteria; @@ -66,6 +68,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.assertThatRuntimeException; import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.verifyNoInteractions; @ExtendWith(MockitoExtension.class) class Webauthn4jRelyingPartyOperationsTests { @@ -546,15 +549,38 @@ class Webauthn4jRelyingPartyOperationsTests { .createCredentialRequestOptions(createRequest); assertThat(credentialRequestOptions.getAllowCredentials()).isEmpty(); + // verify anonymous user not saved + verifyNoInteractions(this.userEntities); } @Test void createCredentialRequestOptionsWhenNullAuthentication() { - PublicKeyCredentialRequestOptionsRequest createRequest = new ImmutablePublicKeyCredentialRequestOptionsRequest(null); + PublicKeyCredentialRequestOptionsRequest createRequest = new ImmutablePublicKeyCredentialRequestOptionsRequest( + null); PublicKeyCredentialRequestOptions credentialRequestOptions = this.rpOperations .createCredentialRequestOptions(createRequest); assertThat(credentialRequestOptions.getAllowCredentials()).isEmpty(); + // verify anonymous user not saved + verifyNoInteractions(this.userEntities); + } + + @Test + void createCredentialRequestOptionsWhenAuthenticated() { + UserDetails user = PasswordEncodedUser.user(); + UsernamePasswordAuthenticationToken auth = new UsernamePasswordAuthenticationToken(user, null, + user.getAuthorities()); + PublicKeyCredentialUserEntity userEntity = TestPublicKeyCredentialUserEntity.userEntity().build(); + CredentialRecord credentialRecord = TestCredentialRecord.userCredential().build(); + given(this.userEntities.findByUsername(user.getUsername())).willReturn(userEntity); + given(this.userCredentials.findByUserId(userEntity.getId())).willReturn(Arrays.asList(credentialRecord)); + PublicKeyCredentialRequestOptionsRequest createRequest = new ImmutablePublicKeyCredentialRequestOptionsRequest( + auth); + PublicKeyCredentialRequestOptions credentialRequestOptions = this.rpOperations + .createCredentialRequestOptions(createRequest); + + assertThat(credentialRequestOptions.getAllowCredentials()).extracting(PublicKeyCredentialDescriptor::getId) + .containsExactly(credentialRecord.getCredentialId()); } private static AuthenticatorAttestationResponse setFlag(byte... flags) throws Exception {