Enable PKCE by default

Closes gh-17507

Signed-off-by: Rohan Naik <[email protected]>

Author: rohan-naik07

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));
 	}
 

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());
 	}
 

docs/modules/ROOT/pages/servlet/oauth2/client/core.adoc

@@ -69,7 +69,7 @@ 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`: If `true` or if `clientAuthenticationMethod` is `none`, then PKCE will be enabled.
 
 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].
 

oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java

@@ -651,6 +651,10 @@ private ClientRegistration create() {
 			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;
 		}
 
@@ -701,12 +705,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);
-				}
 			}
 		}
 
@@ -779,7 +777,7 @@ public static Builder builder() {
 
 		public static final class Builder {
 
-			private boolean requireProofKey;
+			private boolean requireProofKey = true;
 
 			private Builder() {
 			}

oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationTests.java

@@ -35,9 +35,7 @@
 import org.springframework.security.oauth2.core.AuthorizationGrantType;
 import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
-import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
+import static org.assertj.core.api.Assertions.*;
 
 /**
  * Tests for {@link ClientRegistration}.
@@ -683,7 +681,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
@@ -701,28 +699,70 @@ void buildWhenNewAuthorizationCodeAndPkceThenBuilds() {
 			.tokenUri(TOKEN_URI)
 			.build();
 
-		// proof key should be false for passivity
 		assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isTrue();
 	}
 
+	@Test
+	void buildWhenNewAuthorizationCodeAndPkceDisabledThenBuilds() {
+		ClientRegistration.ClientSettings pkceDisabled = ClientRegistration.ClientSettings.builder()
+			.requireProofKey(false)
+			.build();
+		ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
+			.clientId(CLIENT_ID)
+			.clientSettings(pkceDisabled)
+			.authorizationGrantType(new AuthorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE.getValue()))
+			.redirectUri(REDIRECT_URI)
+			.authorizationUri(AUTHORIZATION_URI)
+			.tokenUri(TOKEN_URI)
+			.build();
+
+		assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse();
+	}
+
+	@Test
+	void buildWhenNewAuthorizationCodeAndPrivateClientThenPkceEnabledAndExceptionThrown() {
+		List<ClientAuthenticationMethod> 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) {
 		ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder()
 			.requireProofKey(true)
 			.build();
-		ClientRegistration.Builder builder = ClientRegistration.withRegistrationId(REGISTRATION_ID)
+		ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
 			.clientId(CLIENT_ID)
 			.clientSettings(pkceEnabled)
 			.authorizationGrantType(invalidGrantType)
 			.redirectUri(REDIRECT_URI)
 			.authorizationUri(AUTHORIZATION_URI)
-			.tokenUri(TOKEN_URI);
+			.tokenUri(TOKEN_URI)
+			.build();
 
-		assertThatIllegalStateException().describedAs(
-				"clientSettings.isRequireProofKey=true is only valid with authorizationGrantType=AUTHORIZATION_CODE. Got authorizationGrantType={}",
-				invalidGrantType)
-			.isThrownBy(builder::build);
+		assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse();
 	}
 
 	static List<AuthorizationGrantType> invalidPkceGrantTypes() {