diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java index 42d6ab71032..b4e99ac52e2 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java @@ -156,7 +156,7 @@ public void configureWhenAuthorizationCodeRequestThenRedirectForAuthorization() .andExpect(status().is3xxRedirection()).andReturn(); assertThat(mvcResult.getResponse().getRedirectedUrl()) .matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&" - + "scope=user&state=.{15,}&" + "redirect_uri=http://localhost/client-1"); + + "scope=user&state=.{15,}&" + "redirect_uri=http://localhost/client-1&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); // @formatter:on } @@ -166,9 +166,9 @@ public void configureWhenOauth2ClientInLambdaThenRedirectForAuthorization() thro MvcResult mvcResult = this.mockMvc.perform(get("/oauth2/authorization/registration-1")) .andExpect(status().is3xxRedirection()) .andReturn(); - assertThat(mvcResult.getResponse().getRedirectedUrl()) - .matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&" - + "scope=user&state=.{15,}&" + "redirect_uri=http://localhost/client-1"); + assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?" + + "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&" + + "redirect_uri=http://localhost/client-1&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -215,9 +215,9 @@ public void configureWhenRequestCacheProvidedAndClientAuthorizationRequiredExcep MvcResult mvcResult = this.mockMvc.perform(get("/resource1").with(user("user1"))) .andExpect(status().is3xxRedirection()) .andReturn(); - assertThat(mvcResult.getResponse().getRedirectedUrl()) - .matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&" - + "scope=user&state=.{15,}&" + "redirect_uri=http://localhost/client-1"); + assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?" + + "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&" + + "redirect_uri=http://localhost/client-1&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); verify(requestCache).saveRequest(any(HttpServletRequest.class), any(HttpServletResponse.class)); } diff --git a/config/src/test/java/org/springframework/security/config/http/OAuth2ClientBeanDefinitionParserTests.java b/config/src/test/java/org/springframework/security/config/http/OAuth2ClientBeanDefinitionParserTests.java index 3a84f768e6b..63bb77caae3 100644 --- a/config/src/test/java/org/springframework/security/config/http/OAuth2ClientBeanDefinitionParserTests.java +++ b/config/src/test/java/org/springframework/security/config/http/OAuth2ClientBeanDefinitionParserTests.java @@ -112,9 +112,9 @@ public void requestWhenAuthorizeThenRedirect() throws Exception { .andExpect(status().is3xxRedirection()) .andReturn(); // @formatter:on - assertThat(result.getResponse().getRedirectedUrl()).matches( - "https://accounts.google.com/o/oauth2/v2/auth\\?" + "response_type=code&client_id=google-client-id&" - + "scope=scope1%20scope2&state=.{15,}&redirect_uri=http://localhost/callback/google"); + assertThat(result.getResponse().getRedirectedUrl()).matches("https://accounts.google.com/o/oauth2/v2/auth\\?" + + "response_type=code&client_id=google-client-id&" + + "scope=scope1%20scope2&state=.{15,}&redirect_uri=http://localhost/callback/google&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -134,9 +134,9 @@ public void requestWhenCustomClientRegistrationRepositoryThenCalled() throws Exc .andExpect(status().is3xxRedirection()) .andReturn(); // @formatter:on - assertThat(result.getResponse().getRedirectedUrl()).matches( - "https://accounts.google.com/o/oauth2/v2/auth\\?" + "response_type=code&client_id=google-client-id&" - + "scope=scope1%20scope2&state=.{15,}&redirect_uri=http://localhost/callback/google"); + assertThat(result.getResponse().getRedirectedUrl()).matches("https://accounts.google.com/o/oauth2/v2/auth\\?" + + "response_type=code&client_id=google-client-id&" + + "scope=scope1%20scope2&state=.{15,}&redirect_uri=http://localhost/callback/google&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); verify(this.clientRegistrationRepository).findByRegistrationId(any()); } diff --git a/docs/modules/ROOT/pages/servlet/oauth2/client/client-authentication.adoc b/docs/modules/ROOT/pages/servlet/oauth2/client/client-authentication.adoc index 8a3a4403664..3317422455d 100644 --- a/docs/modules/ROOT/pages/servlet/oauth2/client/client-authentication.adoc +++ b/docs/modules/ROOT/pages/servlet/oauth2/client/client-authentication.adoc @@ -293,6 +293,6 @@ spring: [NOTE] ==== -Public Clients are supported using https://tools.ietf.org/html/rfc7636[Proof Key for Code Exchange] (PKCE). -PKCE will automatically be used when `client-authentication-method` is set to "none" (`ClientAuthenticationMethod.NONE`). +https://tools.ietf.org/html/rfc7636[Proof Key for Code Exchange] (PKCE) will be enabled by default for public as well as confidential clients. Refer https://docs.spring.io/spring-security/reference/servlet/oauth2/client/client-authentication.html#oauth2-client-authentication-public[this +section] to disable PKCE. ==== diff --git a/docs/modules/ROOT/pages/servlet/oauth2/client/core.adoc b/docs/modules/ROOT/pages/servlet/oauth2/client/core.adoc index 04188773718..0b9514713e8 100644 --- a/docs/modules/ROOT/pages/servlet/oauth2/client/core.adoc +++ b/docs/modules/ROOT/pages/servlet/oauth2/client/core.adoc @@ -69,7 +69,8 @@ This information is available only if the Spring Boot property `spring.security. <15> `(userInfoEndpoint)authenticationMethod`: The authentication method used when sending the access token to the UserInfo Endpoint. The supported values are *header*, *form*, and *query*. <16> `userNameAttributeName`: The name of the attribute returned in the UserInfo Response that references the Name or Identifier of the end-user. -<17> [[oauth2Client-client-registration-requireProofKey]]`requireProofKey`: If `true` or if `authorizationGrantType` is `none`, then PKCE will be enabled by default. +<17> [[oauth2Client-client-registration-requireProofKey]]`requireProofKey`: This will by default be `true`, as PKCE will be enabled by default. +If set to `false`,then PKCE will be disabled for public as well as confidential clients. You can initially configure a `ClientRegistration` by using discovery of an OpenID Connect Provider's https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig[Configuration endpoint] or an Authorization Server's https://tools.ietf.org/html/rfc8414#section-3[Metadata endpoint]. 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 b492a6d8015..d34b93d91f5 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 @@ -655,6 +655,10 @@ private ClientRegistration create() { clientRegistration.clientName = StringUtils.hasText(this.clientName) ? this.clientName : this.registrationId; clientRegistration.clientSettings = this.clientSettings; + if (this.clientSettings.requireProofKey) { + clientRegistration.clientSettings.requireProofKey = clientRegistration.authorizationGrantType + .equals(AuthorizationGrantType.AUTHORIZATION_CODE); + } return clientRegistration; } @@ -713,12 +717,6 @@ private void validateAuthorizationGrantTypes() { "AuthorizationGrantType: %s does not match the pre-defined constant %s and won't match a valid OAuth2AuthorizedClientProvider", this.authorizationGrantType, authorizationGrantType)); } - if (!AuthorizationGrantType.AUTHORIZATION_CODE.equals(this.authorizationGrantType) - && this.clientSettings.isRequireProofKey()) { - throw new IllegalStateException( - "clientSettings.isRequireProofKey=true is only valid with authorizationGrantType=AUTHORIZATION_CODE. Got authorizationGrantType=" - + this.authorizationGrantType); - } } } @@ -756,9 +754,7 @@ public static final class ClientSettings implements Serializable { private boolean requireProofKey; - private ClientSettings() { - - } + private ClientSettings() {} public boolean isRequireProofKey() { return this.requireProofKey; @@ -794,6 +790,7 @@ public static final class Builder { private boolean requireProofKey; private Builder() { + this.requireProofKey = true; } /** diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java index a2297accd76..92137e56e11 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java @@ -31,7 +31,6 @@ import org.springframework.security.oauth2.client.registration.ClientRegistration; import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; import org.springframework.security.oauth2.core.AuthorizationGrantType; -import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; import org.springframework.security.oauth2.core.oidc.OidcScopes; @@ -184,8 +183,7 @@ private OAuth2AuthorizationRequest.Builder getBuilder(ClientRegistration clientR // value. applyNonce(builder); } - if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod()) - || clientRegistration.getClientSettings().isRequireProofKey()) { + if (clientRegistration.getClientSettings().isRequireProofKey()) { DEFAULT_PKCE_APPLIER.accept(builder); } return builder; diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java index 0123a2aab7d..5ceb33aa3ae 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 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. @@ -34,7 +34,6 @@ import org.springframework.security.oauth2.client.registration.ReactiveClientRegistrationRepository; import org.springframework.security.oauth2.client.web.OAuth2AuthorizationRequestCustomizers; import org.springframework.security.oauth2.core.AuthorizationGrantType; -import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; import org.springframework.security.oauth2.core.oidc.OidcScopes; @@ -196,8 +195,7 @@ private OAuth2AuthorizationRequest.Builder getBuilder(ClientRegistration clientR // value. applyNonce(builder); } - if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod()) - || clientRegistration.getClientSettings().isRequireProofKey()) { + if (clientRegistration.getClientSettings().isRequireProofKey()) { DEFAULT_PKCE_APPLIER.accept(builder); } return builder; 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 9dbcbd5a5c8..6e9158adc4a 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 @@ -37,7 +37,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; -import static org.assertj.core.api.Assertions.assertThatIllegalStateException; /** * Tests for {@link ClientRegistration}. @@ -780,7 +779,7 @@ void buildWhenDefaultClientSettingsThenDefaulted() { // should not be null assertThat(clientRegistration.getClientSettings()).isNotNull(); // proof key should be false for passivity - assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse(); + assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isTrue(); } // gh-16382 @@ -816,10 +815,8 @@ void buildWhenInvalidGrantTypeForPkceThenException(AuthorizationGrantType invali .authorizationUri(AUTHORIZATION_URI) .tokenUri(TOKEN_URI); - assertThatIllegalStateException().describedAs( - "clientSettings.isRequireProofKey=true is only valid with authorizationGrantType=AUTHORIZATION_CODE. Got authorizationGrantType={}", - invalidGrantType) - .isThrownBy(builder::build); + ClientRegistration clientRegistration = builder.build(); + assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse(); } static List invalidPkceGrantTypes() { diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/TestClientRegistrations.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/TestClientRegistrations.java index e2ef5a8bb1f..293d93dc320 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/TestClientRegistrations.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/TestClientRegistrations.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 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. @@ -64,6 +64,22 @@ public static ClientRegistration.Builder clientRegistration2() { // @formatter:on } + public static ClientRegistration.Builder publicClientRegistrationWithNoPkce() { + return ClientRegistration.withRegistrationId("no-pkce") + .redirectUri("{baseUrl}/{action}/oauth2/code/{registrationId}") + .clientAuthenticationMethod(ClientAuthenticationMethod.NONE) + .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) + .clientSettings(ClientRegistration.ClientSettings.builder().requireProofKey(false).build()) + .scope("read:user") + .authorizationUri("https://example.com/login/oauth/authorize") + .tokenUri("https://example.com/login/oauth/access_token") + .userInfoUri("https://api.example.com/user") + .userNameAttributeName("id") + .clientName("Client Name") + .clientId("client-id") + .clientSecret(null); + } + public static ClientRegistration.Builder clientCredentials() { // @formatter:off return clientRegistration() 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 a0abf7132e4..202abd1eb3b 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 @@ -58,6 +58,8 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { private ClientRegistration pkceClientRegistration; + private ClientRegistration nonProofKeyPublicClientRegistration; + private ClientRegistration fineRedirectUriTemplateRegistration; private ClientRegistration publicClientRegistration; @@ -76,7 +78,7 @@ public void setUp() { this.registration2 = TestClientRegistrations.clientRegistration2().build(); this.pkceClientRegistration = pkceClientRegistration().build(); - + this.nonProofKeyPublicClientRegistration = TestClientRegistrations.publicClientRegistrationWithNoPkce().build(); this.fineRedirectUriTemplateRegistration = fineRedirectUriTemplateClientRegistration().build(); // @formatter:off this.publicClientRegistration = TestClientRegistrations.clientRegistration() @@ -92,7 +94,7 @@ public void setUp() { // @formatter:on this.clientRegistrationRepository = new InMemoryClientRegistrationRepository(this.registration1, this.registration2, this.pkceClientRegistration, this.fineRedirectUriTemplateRegistration, - this.publicClientRegistration, this.oidcRegistration); + this.publicClientRegistration, this.oidcRegistration, this.nonProofKeyPublicClientRegistration); this.resolver = new DefaultOAuth2AuthorizationRequestResolver(this.clientRegistrationRepository, this.authorizationRequestBaseUri); } @@ -173,12 +175,14 @@ public void resolveWhenAuthorizationRequestWithValidClientThenResolves() { assertThat(authorizationRequest.getState()).isNotNull(); assertThat(authorizationRequest.getAdditionalParameters()) .doesNotContainKey(OAuth2ParameterNames.REGISTRATION_ID); - assertThat(authorizationRequest.getAttributes()) - .containsExactly(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); + assertThat(authorizationRequest.getAttributes()).containsExactly( + entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId()), + entry(PkceParameterNames.CODE_VERIFIER, + authorizationRequest.getAttributes().get(PkceParameterNames.CODE_VERIFIER))); assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/login/oauth2/code/registration-id"); + + "redirect_uri=http://localhost/login/oauth2/code/registration-id&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -190,8 +194,10 @@ public void resolveWhenClientAuthorizationRequiredExceptionAvailableThenResolves OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request, clientRegistration.getRegistrationId()); assertThat(authorizationRequest).isNotNull(); - assertThat(authorizationRequest.getAttributes()) - .containsExactly(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); + assertThat(authorizationRequest.getAttributes()).containsExactly( + entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId()), + entry(PkceParameterNames.CODE_VERIFIER, + authorizationRequest.getAttributes().get(PkceParameterNames.CODE_VERIFIER))); } @Test @@ -299,7 +305,8 @@ public void resolveWhenAuthorizationRequestIncludesPort80ThenExpandedRedirectUri assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/login/oauth2/code/registration-id"); + + "redirect_uri=http://localhost/login/oauth2/code/registration-id" + + "&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -315,7 +322,8 @@ public void resolveWhenAuthorizationRequestIncludesPort443ThenExpandedRedirectUr assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=https://example.com/login/oauth2/code/registration-id"); + + "redirect_uri=https://example.com/login/oauth2/code/registration-id" + + "&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -329,7 +337,7 @@ public void resolveWhenClientAuthorizationRequiredExceptionAvailableThenRedirect assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id"); + + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -342,7 +350,8 @@ public void resolveWhenAuthorizationRequestOAuth2LoginThenRedirectUriIsLogin() { assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id-2&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/login/oauth2/code/registration-id-2"); + + "redirect_uri=http://localhost/login/oauth2/code/registration-id-2" + + "&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -356,7 +365,8 @@ public void resolveWhenAuthorizationRequestHasActionParameterAuthorizeThenRedire assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id"); + + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id&" + + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -370,7 +380,7 @@ public void resolveWhenAuthorizationRequestHasActionParameterLoginThenRedirectUr assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id-2&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/login/oauth2/code/registration-id-2"); + + "redirect_uri=http://localhost/login/oauth2/code/registration-id-2&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -427,33 +437,32 @@ public void resolveWhenAuthorizationRequestApplyPkceToConfidentialClientsThenApp 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); - } - }); - }); - + public void resolveWhenAuthorizationRequestApplyPkceToConfidentialClientThenApplied() { + // pkce enabled by default for private clients ClientRegistration clientRegistration = this.registration1; String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); - request.setServletPath(requestUri); OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request); assertPkceApplied(authorizationRequest, clientRegistration); + } - clientRegistration = this.registration2; - requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); - request = new MockHttpServletRequest("GET", requestUri); - request.setServletPath(requestUri); - authorizationRequest = this.resolver.resolve(request); + @Test + public void resolveWhenAuthorizationRequestApplyPkceToPublicClientWithRequireProofKeyFalseThenApplied() { + ClientRegistration clientRegistration = this.nonProofKeyPublicClientRegistration; // change + // to + // non + // proof + // key + // public + // client + String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); + MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); + OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request); assertPkceNotApplied(authorizationRequest, clientRegistration); } + // gh-6548 private void assertPkceApplied(OAuth2AuthorizationRequest authorizationRequest, ClientRegistration clientRegistration) { assertThat(authorizationRequest.getAdditionalParameters()).containsKey(PkceParameterNames.CODE_CHALLENGE); @@ -510,7 +519,7 @@ public void resolveWhenAuthenticationRequestWithValidOidcClientThenResolves() { .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=openid&state=.{15,}&" + "redirect_uri=http://localhost/login/oauth2/code/oidc-registration-id&" - + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } // gh-7696 @@ -530,7 +539,8 @@ public void resolveWhenAuthorizationRequestCustomizerRemovesNonceThenQueryExclud assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=openid&state=.{15,}&" - + "redirect_uri=http://localhost/login/oauth2/code/oidc-registration-id"); + + "redirect_uri=http://localhost/login/oauth2/code/oidc-registration-id&" + + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -548,7 +558,8 @@ public void resolveWhenAuthorizationRequestCustomizerAddsParameterThenQueryInclu .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=openid&state=.{15,}&" + "redirect_uri=http://localhost/login/oauth2/code/oidc-registration-id&" - + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "param1=value1"); + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}" + + "&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256¶m1=value1"); } @Test @@ -565,7 +576,8 @@ public void resolveWhenAuthorizationRequestCustomizerOverridesParameterThenQuery assertThat(authorizationRequest.getAuthorizationRequestUri()).matches( "https://example.com/login/oauth/authorize\\?" + "response_type=code&" + "scope=openid&state=.{15,}&" + "redirect_uri=http://localhost/login/oauth2/code/oidc-registration-id&" - + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "appid=client-id"); + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}" + + "&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256&appid=client-id"); } @Test diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java index 59676b14619..2d6226bd58a 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java @@ -186,7 +186,7 @@ public void doFilterWhenAuthorizationRequestOAuth2LoginThenRedirectForAuthorizat verifyNoMoreInteractions(filterChain); assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/login/oauth2/code/registration-id"); + + "redirect_uri=http://localhost/login/oauth2/code/registration-id&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -220,7 +220,7 @@ public void doFilterWhenCustomAuthorizationRequestBaseUriThenRedirectForAuthoriz verifyNoMoreInteractions(filterChain); assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/login/oauth2/code/registration-id"); + + "redirect_uri=http://localhost/login/oauth2/code/registration-id&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -237,7 +237,7 @@ public void doFilterWhenNotAuthorizationRequestAndClientAuthorizationRequiredExc verify(filterChain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id"); + + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); verify(this.requestCache).saveRequest(any(HttpServletRequest.class), any(HttpServletResponse.class)); } @@ -286,7 +286,7 @@ public void doFilterWhenAuthorizationRequestAndAdditionalParametersProvidedThenA assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + "redirect_uri=http://localhost/login/oauth2/code/registration-id&" - + "idp=https://other.provider.com"); + + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256&idp=https://other.provider.com"); } // gh-4911, gh-5244 @@ -327,7 +327,7 @@ public void doFilterWhenAuthorizationRequestAndCustomAuthorizationRequestUriSetT assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + "redirect_uri=http://localhost/login/oauth2/code/registration-id&" - + "login_hint=user@provider\\.com"); + + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256&login_hint=user@provider\\.com"); } @Test @@ -354,7 +354,7 @@ public void doFilterWhenCustomAuthorizationRedirectStrategySetThenCustomAuthoriz assertThat(response.getContentAsString(StandardCharsets.UTF_8)) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/login/oauth2/code/registration-id"); + + "redirect_uri=http://localhost/login/oauth2/code/registration-id&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } // gh-11602 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 bf7ab096784..7aa3e522c92 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 @@ -59,11 +59,14 @@ 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.publicClientRegistrationWithNoPkce().build(); } @Test @@ -90,7 +93,8 @@ public void resolveWhenClientRegistrationFoundThenWorks() { OAuth2AuthorizationRequest request = resolve("/oauth2/authorization/not-found-id"); assertThat(request.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" - + "scope=read:user&state=.*?&" + "redirect_uri=/login/oauth2/code/registration-id"); + + "scope=read:user&state=.*?&" + "redirect_uri=/login/oauth2/code/registration-id" + + "&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -105,7 +109,8 @@ public void resolveWhenForwardedHeadersClientRegistrationFoundThenWorks() { OAuth2AuthorizationRequest request = this.resolver.resolve(exchange).block(); assertThat(request.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" - + "scope=read:user&state=.*?&" + "redirect_uri=/login/oauth2/code/registration-id"); + + "scope=read:user&state=.*?&" + "redirect_uri=/login/oauth2/code/registration-id" + + "&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256"); } @Test @@ -149,9 +154,9 @@ public void resolveWhenAuthorizationRequestApplyPkceToSpecificConfidentialClient ClientRegistration registration1 = TestClientRegistrations.clientRegistration().build(); given(this.clientRegistrationRepository.findByRegistrationId(eq(registration1.getRegistrationId()))) .willReturn(Mono.just(registration1)); - ClientRegistration registration2 = TestClientRegistrations.clientRegistration2().build(); - given(this.clientRegistrationRepository.findByRegistrationId(eq(registration2.getRegistrationId()))) - .willReturn(Mono.just(registration2)); + given(this.clientRegistrationRepository + .findByRegistrationId(eq(this.nonProofKeyPublicClientRegistration.getRegistrationId()))) + .willReturn(Mono.just(this.nonProofKeyPublicClientRegistration)); this.resolver.setAuthorizationRequestCustomizer((builder) -> { builder.attributes((attrs) -> { @@ -165,8 +170,8 @@ public void resolveWhenAuthorizationRequestApplyPkceToSpecificConfidentialClient OAuth2AuthorizationRequest request = resolve("/oauth2/authorization/" + registration1.getRegistrationId()); assertPkceApplied(request, registration1); - request = resolve("/oauth2/authorization/" + registration2.getRegistrationId()); - assertPkceNotApplied(request, registration2); + request = resolve("/oauth2/authorization/" + this.nonProofKeyPublicClientRegistration.getRegistrationId()); + assertPkceNotApplied(request, this.nonProofKeyPublicClientRegistration); } @Test @@ -220,7 +225,8 @@ public void resolveWhenAuthenticationRequestWithValidOidcClientThenResolves() { assertThat((String) request.getAttribute(OidcParameterNames.NONCE)).matches("^([a-zA-Z0-9\\-\\.\\_\\~]){128}$"); assertThat(request.getAuthorizationRequestUri()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=openid&state=.*?&" - + "redirect_uri=/login/oauth2/code/registration-id&" + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + + "redirect_uri=/login/oauth2/code/registration-id&" + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge_method=S256"); } // gh-7696 @@ -237,7 +243,8 @@ public void resolveWhenAuthorizationRequestCustomizerRemovesNonceThenQueryExclud assertThat(authorizationRequest.getAttributes()).containsKey(OAuth2ParameterNames.REGISTRATION_ID); assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" - + "scope=openid&state=.{15,}&" + "redirect_uri=/login/oauth2/code/registration-id"); + + "scope=openid&state=.{15,}&" + "redirect_uri=/login/oauth2/code/registration-id&" + + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge_method=S256"); } @Test @@ -252,7 +259,8 @@ public void resolveWhenAuthorizationRequestCustomizerAddsParameterThenQueryInclu assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=openid&state=.{15,}&" + "redirect_uri=/login/oauth2/code/registration-id&" - + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "param1=value1"); + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + + "code_challenge_method=S256&" + "param1=value1"); } @Test @@ -267,7 +275,8 @@ public void resolveWhenAuthorizationRequestCustomizerOverridesParameterThenQuery assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&" + "scope=openid&state=.{15,}&" + "redirect_uri=/login/oauth2/code/registration-id&" - + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "appid=client-id"); + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + + "code_challenge_method=S256&" + "appid=client-id"); } private OAuth2AuthorizationRequest resolve(String path) {