Skip to content

Commit 4f7a21c

Browse files
rohan-naik07rohan-naik07
committed
PKCE configuration - enabled by default
Signed-off-by: Rohan Naik <[email protected]>
1 parent d3b143d commit 4f7a21c

File tree

8 files changed

+237
-128
lines changed

8 files changed

+237
-128
lines changed

‎config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ public void configureWhenAuthorizationCodeRequestThenRedirectForAuthorization()
156156
.andExpect(status().is3xxRedirection()).andReturn();
157157
assertThat(mvcResult.getResponse().getRedirectedUrl())
158158
.matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&"
159-
+ "scope=user&state=.{15,}&" + "redirect_uri=http://localhost/client-1");
159+
+ "scope=user&state=.{15,}&" + "redirect_uri=http://localhost/client-1&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256");
160160
// @formatter:on
161161
}
162162

@@ -166,9 +166,9 @@ public void configureWhenOauth2ClientInLambdaThenRedirectForAuthorization() thro
166166
MvcResult mvcResult = this.mockMvc.perform(get("/oauth2/authorization/registration-1"))
167167
.andExpect(status().is3xxRedirection())
168168
.andReturn();
169-
assertThat(mvcResult.getResponse().getRedirectedUrl())
170-
.matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&"
171-
+ "scope=user&state=.{15,}&" + "redirect_uri=http://localhost/client-1");
169+
assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?"
170+
+ "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&"
171+
+ "redirect_uri=http://localhost/client-1&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256");
172172
}
173173

174174
@Test
@@ -215,9 +215,9 @@ public void configureWhenRequestCacheProvidedAndClientAuthorizationRequiredExcep
215215
MvcResult mvcResult = this.mockMvc.perform(get("/resource1").with(user("user1")))
216216
.andExpect(status().is3xxRedirection())
217217
.andReturn();
218-
assertThat(mvcResult.getResponse().getRedirectedUrl())
219-
.matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&"
220-
+ "scope=user&state=.{15,}&" + "redirect_uri=http://localhost/client-1");
218+
assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?"
219+
+ "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&"
220+
+ "redirect_uri=http://localhost/client-1&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256");
221221
verify(requestCache).saveRequest(any(HttpServletRequest.class), any(HttpServletResponse.class));
222222
}
223223

‎config/src/test/java/org/springframework/security/config/http/OAuth2ClientBeanDefinitionParserTests.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,9 @@ public void requestWhenAuthorizeThenRedirect() throws Exception {
112112
.andExpect(status().is3xxRedirection())
113113
.andReturn();
114114
// @formatter:on
115-
assertThat(result.getResponse().getRedirectedUrl()).matches(
116-
"https://accounts.google.com/o/oauth2/v2/auth\\?" + "response_type=code&client_id=google-client-id&"
117-
+ "scope=scope1%20scope2&state=.{15,}&redirect_uri=http://localhost/callback/google");
115+
assertThat(result.getResponse().getRedirectedUrl()).matches("https://accounts.google.com/o/oauth2/v2/auth\\?"
116+
+ "response_type=code&client_id=google-client-id&"
117+
+ "scope=scope1%20scope2&state=.{15,}&redirect_uri=http://localhost/callback/google&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256");
118118
}
119119

120120
@Test
@@ -134,9 +134,9 @@ public void requestWhenCustomClientRegistrationRepositoryThenCalled() throws Exc
134134
.andExpect(status().is3xxRedirection())
135135
.andReturn();
136136
// @formatter:on
137-
assertThat(result.getResponse().getRedirectedUrl()).matches(
138-
"https://accounts.google.com/o/oauth2/v2/auth\\?" + "response_type=code&client_id=google-client-id&"
139-
+ "scope=scope1%20scope2&state=.{15,}&redirect_uri=http://localhost/callback/google");
137+
assertThat(result.getResponse().getRedirectedUrl()).matches("https://accounts.google.com/o/oauth2/v2/auth\\?"
138+
+ "response_type=code&client_id=google-client-id&"
139+
+ "scope=scope1%20scope2&state=.{15,}&redirect_uri=http://localhost/callback/google&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256");
140140
verify(this.clientRegistrationRepository).findByRegistrationId(any());
141141
}
142142

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ This information is available only if the Spring Boot property `spring.security.
6969
<15> `(userInfoEndpoint)authenticationMethod`: The authentication method used when sending the access token to the UserInfo Endpoint.
7070
The supported values are *header*, *form*, and *query*.
7171
<16> `userNameAttributeName`: The name of the attribute returned in the UserInfo Response that references the Name or Identifier of the end-user.
72-
<17> [[oauth2Client-client-registration-requireProofKey]]`requireProofKey`: If `true` or if `authorizationGrantType` is `none`, then PKCE will be enabled by default.
72+
<17> [[oauth2Client-client-registration-requireProofKey]]`requireProofKey`: If `true` or if `authorizationGrantType` is `none`, then PKCE will be enabled.
7373

7474
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].
7575

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

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -651,6 +651,10 @@ private ClientRegistration create() {
651651
clientRegistration.clientName = StringUtils.hasText(this.clientName) ? this.clientName
652652
: this.registrationId;
653653
clientRegistration.clientSettings = this.clientSettings;
654+
if (clientRegistration.clientSettings.requireProofKey) {
655+
clientRegistration.clientSettings.requireProofKey = AuthorizationGrantType.AUTHORIZATION_CODE
656+
.equals(this.authorizationGrantType);
657+
}
654658
return clientRegistration;
655659
}
656660

@@ -701,12 +705,6 @@ private void validateAuthorizationGrantTypes() {
701705
"AuthorizationGrantType: %s does not match the pre-defined constant %s and won't match a valid OAuth2AuthorizedClientProvider",
702706
this.authorizationGrantType, authorizationGrantType));
703707
}
704-
if (!AuthorizationGrantType.AUTHORIZATION_CODE.equals(this.authorizationGrantType)
705-
&& this.clientSettings.isRequireProofKey()) {
706-
throw new IllegalStateException(
707-
"clientSettings.isRequireProofKey=true is only valid with authorizationGrantType=AUTHORIZATION_CODE. Got authorizationGrantType="
708-
+ this.authorizationGrantType);
709-
}
710708
}
711709
}
712710

@@ -742,7 +740,7 @@ public static final class ClientSettings implements Serializable {
742740
@Serial
743741
private static final long serialVersionUID = 7495627155437124692L;
744742

745-
private boolean requireProofKey;
743+
private boolean requireProofKey = true;
746744

747745
private ClientSettings() {
748746

@@ -779,7 +777,7 @@ public static Builder builder() {
779777

780778
public static final class Builder {
781779

782-
private boolean requireProofKey;
780+
private boolean requireProofKey = true;
783781

784782
private Builder() {
785783
}

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

Lines changed: 71 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,10 @@
3434
import org.springframework.security.oauth2.core.AuthenticationMethod;
3535
import org.springframework.security.oauth2.core.AuthorizationGrantType;
3636
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
37+
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
38+
import org.springframework.security.oauth2.core.endpoint.PkceParameterNames;
3739

38-
import static org.assertj.core.api.Assertions.assertThat;
39-
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
40-
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
40+
import static org.assertj.core.api.Assertions.*;
4141

4242
/**
4343
* Tests for {@link ClientRegistration}.
@@ -683,7 +683,7 @@ void buildWhenDefaultClientSettingsThenDefaulted() {
683683
// should not be null
684684
assertThat(clientRegistration.getClientSettings()).isNotNull();
685685
// proof key should be false for passivity
686-
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse();
686+
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isTrue();
687687
}
688688

689689
// gh-16382
@@ -701,10 +701,64 @@ void buildWhenNewAuthorizationCodeAndPkceThenBuilds() {
701701
.tokenUri(TOKEN_URI)
702702
.build();
703703

704-
// proof key should be false for passivity
705704
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isTrue();
706705
}
707706

707+
@Test
708+
void buildWhenNewAuthorizationCodeAndPkceDisabledThenBuilds() {
709+
ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder()
710+
.requireProofKey(false)
711+
.build();
712+
ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
713+
.clientId(CLIENT_ID)
714+
.clientSettings(pkceEnabled)
715+
.authorizationGrantType(new AuthorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE.getValue()))
716+
.redirectUri(REDIRECT_URI)
717+
.authorizationUri(AUTHORIZATION_URI)
718+
.tokenUri(TOKEN_URI)
719+
.build();
720+
721+
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse();
722+
}
723+
724+
@ParameterizedTest
725+
@MethodSource("privateClientAuthenticationMethods")
726+
void buildWhenNewAuthorizationCodeAndPrivateClientThenPkceEnabledAndBuilds(
727+
ClientAuthenticationMethod clientAuthenticationMethod) {
728+
ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder()
729+
.requireProofKey(true)
730+
.build();
731+
ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
732+
.clientId(CLIENT_ID)
733+
.clientSettings(pkceEnabled)
734+
.authorizationGrantType(new AuthorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE.getValue()))
735+
.clientAuthenticationMethod(clientAuthenticationMethod)
736+
.redirectUri(REDIRECT_URI)
737+
.authorizationUri(AUTHORIZATION_URI)
738+
.tokenUri(TOKEN_URI)
739+
.build();
740+
741+
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isTrue();
742+
}
743+
744+
@Test
745+
void buildWhenNewAuthorizationCodeAndPublicClientAndPkceDisabledThenBuilds() {
746+
ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder()
747+
.requireProofKey(false)
748+
.build();
749+
ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
750+
.clientId(CLIENT_ID)
751+
.clientSettings(pkceEnabled)
752+
.authorizationGrantType(new AuthorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE.getValue()))
753+
.clientAuthenticationMethod(ClientAuthenticationMethod.NONE)
754+
.redirectUri(REDIRECT_URI)
755+
.authorizationUri(AUTHORIZATION_URI)
756+
.tokenUri(TOKEN_URI)
757+
.build();
758+
// even if this is an invalid state pkce will be applied later
759+
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse();
760+
}
761+
708762
@ParameterizedTest
709763
@MethodSource("invalidPkceGrantTypes")
710764
void buildWhenInvalidGrantTypeForPkceThenException(AuthorizationGrantType invalidGrantType) {
@@ -719,10 +773,18 @@ void buildWhenInvalidGrantTypeForPkceThenException(AuthorizationGrantType invali
719773
.authorizationUri(AUTHORIZATION_URI)
720774
.tokenUri(TOKEN_URI);
721775

722-
assertThatIllegalStateException().describedAs(
723-
"clientSettings.isRequireProofKey=true is only valid with authorizationGrantType=AUTHORIZATION_CODE. Got authorizationGrantType={}",
724-
invalidGrantType)
725-
.isThrownBy(builder::build);
776+
ClientRegistration clientRegistration = builder.build();
777+
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse();
778+
}
779+
780+
static List<ClientAuthenticationMethod> privateClientAuthenticationMethods() {
781+
return Arrays.stream(ClientAuthenticationMethod.class.getFields())
782+
.filter((field) -> Modifier.isFinal(field.getModifiers())
783+
&& field.getType() == ClientAuthenticationMethod.class)
784+
.map((field) -> getStaticValue(field, ClientAuthenticationMethod.class))
785+
.filter((authenticationMethod) -> authenticationMethod != ClientAuthenticationMethod.NONE)
786+
.map((authenticationMethod) -> new ClientAuthenticationMethod(authenticationMethod.getValue()))
787+
.collect(Collectors.toList());
726788
}
727789

728790
static List<AuthorizationGrantType> invalidPkceGrantTypes() {

0 commit comments

Comments
 (0)