From 4dfef1483dae0ca8e36ba69403d26f1ae5be1fad Mon Sep 17 00:00:00 2001 From: Joe Grandja <10884212+jgrandja@users.noreply.github.com> Date: Thu, 2 Oct 2025 16:37:46 -0400 Subject: [PATCH] Polish gh-17507 --- .../registration/ClientRegistration.java | 11 +++-- .../registration/ClientRegistrationTests.java | 34 ++------------ ...uth2AuthorizationRequestResolverTests.java | 47 +++++-------------- ...uth2AuthorizationRequestResolverTests.java | 36 -------------- 4 files changed, 22 insertions(+), 106 deletions(-) diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java index d6a3982991..8e88404d5d 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java @@ -651,10 +651,6 @@ public final class ClientRegistration implements Serializable { clientRegistration.clientName = StringUtils.hasText(this.clientName) ? this.clientName : this.registrationId; clientRegistration.clientSettings = this.clientSettings; - if (clientRegistration.clientSettings.requireProofKey) { - clientRegistration.clientSettings.requireProofKey = AuthorizationGrantType.AUTHORIZATION_CODE - .equals(this.authorizationGrantType); - } return clientRegistration; } @@ -706,6 +702,13 @@ public final class ClientRegistration implements Serializable { this.authorizationGrantType, authorizationGrantType)); } } + if (!AuthorizationGrantType.AUTHORIZATION_CODE.equals(this.authorizationGrantType) + && this.clientSettings.isRequireProofKey()) { + this.clientSettings = ClientSettings.builder().requireProofKey(false).build(); + logger.warn(LogMessage.format( + "clientSettings.isRequireProofKey=true is only valid with authorizationGrantType=%s. Got authorizationGrantType=%s. Resetting to clientSettings.isRequireProofKey=false", + AuthorizationGrantType.AUTHORIZATION_CODE, this.authorizationGrantType)); + } } private void validateScopes() { diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationTests.java index 40dc7c17a8..a7e6d3c165 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationTests.java @@ -35,7 +35,8 @@ import org.springframework.security.oauth2.core.AuthenticationMethod; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; -import static org.assertj.core.api.Assertions.*; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; /** * Tests for {@link ClientRegistration}. @@ -680,7 +681,6 @@ public class ClientRegistrationTests { // should not be null assertThat(clientRegistration.getClientSettings()).isNotNull(); - // proof key should be false for passivity assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isTrue(); } @@ -719,37 +719,9 @@ public class ClientRegistrationTests { assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse(); } - @Test - void buildWhenNewAuthorizationCodeAndPrivateClientThenPkceEnabledAndExceptionThrown() { - List clientAuthenticationMethods = Arrays - .stream(ClientAuthenticationMethod.class.getFields()) - .filter((field) -> Modifier.isFinal(field.getModifiers()) - && field.getType() == ClientAuthenticationMethod.class) - .map((field) -> getStaticValue(field, ClientAuthenticationMethod.class)) - .filter((authenticationMethod) -> authenticationMethod != ClientAuthenticationMethod.NONE) - .map((authenticationMethod) -> new ClientAuthenticationMethod(authenticationMethod.getValue())) - .toList(); - for (ClientAuthenticationMethod clientAuthenticationMethod : clientAuthenticationMethods) { - ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder() - .requireProofKey(true) - .build(); - ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID) - .clientId(CLIENT_ID) - .clientSettings(pkceEnabled) - .authorizationGrantType( - new AuthorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE.getValue())) - .clientAuthenticationMethod(clientAuthenticationMethod) - .redirectUri(REDIRECT_URI) - .authorizationUri(AUTHORIZATION_URI) - .tokenUri(TOKEN_URI) - .build(); - assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isTrue(); - } - } - @ParameterizedTest @MethodSource("invalidPkceGrantTypes") - void buildWhenInvalidGrantTypeForPkceThenException(AuthorizationGrantType invalidGrantType) { + void buildWhenInvalidGrantTypeForPkceThenPkceDisabled(AuthorizationGrantType invalidGrantType) { ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder() .requireProofKey(true) .build(); diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java index 6cbb0c22a3..b1caffa055 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java @@ -60,8 +60,6 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { private ClientRegistration pkceClientRegistration; - private ClientRegistration nonProofKeyPublicClientRegistration; - private ClientRegistration fineRedirectUriTemplateRegistration; private ClientRegistration publicClientRegistration; @@ -80,11 +78,6 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { this.registration2 = TestClientRegistrations.clientRegistration2().build(); this.pkceClientRegistration = pkceClientRegistration().build(); - this.nonProofKeyPublicClientRegistration = TestClientRegistrations.clientRegistration() - .registrationId("invalid-public-client-registration-id") - .clientAuthenticationMethod(ClientAuthenticationMethod.NONE) - .clientSettings(ClientRegistration.ClientSettings.builder().requireProofKey(false).build()) - .build(); this.fineRedirectUriTemplateRegistration = fineRedirectUriTemplateClientRegistration().build(); // @formatter:off this.publicClientRegistration = TestClientRegistrations.clientRegistration() @@ -100,7 +93,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { // @formatter:on this.clientRegistrationRepository = new InMemoryClientRegistrationRepository(this.registration1, this.registration2, this.pkceClientRegistration, this.fineRedirectUriTemplateRegistration, - this.publicClientRegistration, this.oidcRegistration, this.nonProofKeyPublicClientRegistration); + this.publicClientRegistration, this.oidcRegistration); this.resolver = new DefaultOAuth2AuthorizationRequestResolver(this.clientRegistrationRepository, this.authorizationRequestBaseUri); } @@ -396,33 +389,6 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { // gh-6548 @Test public void resolveWhenAuthorizationRequestApplyPkceToConfidentialClientsThenApplied() { - this.resolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce()); - - ClientRegistration clientRegistration = this.registration1; - String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); - MockHttpServletRequest request = get(requestUri).build(); - OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request); - assertPkceApplied(authorizationRequest, clientRegistration); - - clientRegistration = this.registration2; - requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); - request = get(requestUri).build(); - authorizationRequest = this.resolver.resolve(request); - assertPkceApplied(authorizationRequest, clientRegistration); - } - - // gh-6548 - @Test - public void resolveWhenAuthorizationRequestApplyPkceToSpecificConfidentialClientThenApplied() { - this.resolver.setAuthorizationRequestCustomizer((builder) -> { - builder.attributes((attrs) -> { - String registrationId = (String) attrs.get(OAuth2ParameterNames.REGISTRATION_ID); - if (this.registration1.getRegistrationId().equals(registrationId)) { - OAuth2AuthorizationRequestCustomizers.withPkce().accept(builder); - } - }); - }); - ClientRegistration clientRegistration = this.registration1; String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); MockHttpServletRequest request = get(requestUri).build(); @@ -549,6 +515,17 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { + "&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256&appid=client-id"); } + @Test + public void resolveWhenAuthorizationRequestNoProvideAuthorizationRequestBaseUri() { + OAuth2AuthorizationRequestResolver resolver = new DefaultOAuth2AuthorizationRequestResolver( + this.clientRegistrationRepository); + String requestUri = this.authorizationRequestBaseUri + "/" + this.registration2.getRegistrationId(); + MockHttpServletRequest request = get(requestUri).build(); + OAuth2AuthorizationRequest authorizationRequest = resolver.resolve(request); + assertThat(authorizationRequest.getRedirectUri()) + .isEqualTo("http://localhost/login/oauth2/code/" + this.registration2.getRegistrationId()); + } + @Test public void resolveWhenAuthorizationRequestProvideCodeChallengeMethod() { ClientRegistration clientRegistration = this.pkceClientRegistration; diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java index 63e9baf245..0fe44d2abf 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java @@ -29,7 +29,6 @@ import org.springframework.mock.web.server.MockServerWebExchange; import org.springframework.security.oauth2.client.registration.ClientRegistration; import org.springframework.security.oauth2.client.registration.ReactiveClientRegistrationRepository; import org.springframework.security.oauth2.client.registration.TestClientRegistrations; -import org.springframework.security.oauth2.client.web.OAuth2AuthorizationRequestCustomizers; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; @@ -59,18 +58,11 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { private DefaultServerOAuth2AuthorizationRequestResolver resolver; - private ClientRegistration nonProofKeyPublicClientRegistration; - private ClientRegistration registration = TestClientRegistrations.clientRegistration().build(); @BeforeEach public void setup() { this.resolver = new DefaultServerOAuth2AuthorizationRequestResolver(this.clientRegistrationRepository); - this.nonProofKeyPublicClientRegistration = TestClientRegistrations.clientRegistration() - .registrationId("invalid-public-client-registration-id") - .clientAuthenticationMethod(ClientAuthenticationMethod.NONE) - .clientSettings(ClientRegistration.ClientSettings.builder().requireProofKey(false).build()) - .build(); } @Test @@ -143,8 +135,6 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { given(this.clientRegistrationRepository.findByRegistrationId(eq(registration2.getRegistrationId()))) .willReturn(Mono.just(registration2)); - this.resolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce()); - OAuth2AuthorizationRequest request = resolve("/oauth2/authorization/" + registration1.getRegistrationId()); assertPkceApplied(request, registration1); @@ -152,32 +142,6 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { assertPkceApplied(request, registration2); } - // gh-6548 - @Test - public void resolveWhenAuthorizationRequestApplyPkceToSpecificConfidentialClientThenApplied() { - ClientRegistration registration1 = TestClientRegistrations.clientRegistration().build(); - given(this.clientRegistrationRepository.findByRegistrationId(eq(registration1.getRegistrationId()))) - .willReturn(Mono.just(registration1)); - given(this.clientRegistrationRepository - .findByRegistrationId(eq(this.nonProofKeyPublicClientRegistration.getRegistrationId()))) - .willReturn(Mono.just(this.nonProofKeyPublicClientRegistration)); - - this.resolver.setAuthorizationRequestCustomizer((builder) -> { - builder.attributes((attrs) -> { - String registrationId = (String) attrs.get(OAuth2ParameterNames.REGISTRATION_ID); - if (registration1.getRegistrationId().equals(registrationId)) { - OAuth2AuthorizationRequestCustomizers.withPkce().accept(builder); - } - }); - }); - - OAuth2AuthorizationRequest request = resolve("/oauth2/authorization/" + registration1.getRegistrationId()); - assertPkceApplied(request, registration1); - - request = resolve("/oauth2/authorization/" + this.nonProofKeyPublicClientRegistration.getRegistrationId()); - assertPkceApplied(request, this.nonProofKeyPublicClientRegistration); - } - @Test void resolveWhenRequireProofKeyTrueThenPkceEnabled() { ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder()