Skip to content

Commit 4dfef14

Browse files
jgrandjajgrandja
committed
Polish gh-17507 
1 parent 8c65dc9 commit 4dfef14

File tree

4 files changed

+22
-106
lines changed

4 files changed

+22
-106
lines changed

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

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -651,10 +651,6 @@ 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-
}
658654
return clientRegistration;
659655
}
660656

@@ -706,6 +702,13 @@ private void validateAuthorizationGrantTypes() {
706702
this.authorizationGrantType, authorizationGrantType));
707703
}
708704
}
705+
if (!AuthorizationGrantType.AUTHORIZATION_CODE.equals(this.authorizationGrantType)
706+
&& this.clientSettings.isRequireProofKey()) {
707+
this.clientSettings = ClientSettings.builder().requireProofKey(false).build();
708+
logger.warn(LogMessage.format(
709+
"clientSettings.isRequireProofKey=true is only valid with authorizationGrantType=%s. Got authorizationGrantType=%s. Resetting to clientSettings.isRequireProofKey=false",
710+
AuthorizationGrantType.AUTHORIZATION_CODE, this.authorizationGrantType));
711+
}
709712
}
710713

711714
private void validateScopes() {

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

Lines changed: 3 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@
3535
import org.springframework.security.oauth2.core.AuthorizationGrantType;
3636
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
3737

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

4041
/**
4142
* Tests for {@link ClientRegistration}.
@@ -680,7 +681,6 @@ void buildWhenDefaultClientSettingsThenDefaulted() {
680681

681682
// should not be null
682683
assertThat(clientRegistration.getClientSettings()).isNotNull();
683-
// proof key should be false for passivity
684684
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isTrue();
685685
}
686686

@@ -719,37 +719,9 @@ void buildWhenNewAuthorizationCodeAndPkceDisabledThenBuilds() {
719719
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isFalse();
720720
}
721721

722-
@Test
723-
void buildWhenNewAuthorizationCodeAndPrivateClientThenPkceEnabledAndExceptionThrown() {
724-
List<ClientAuthenticationMethod> clientAuthenticationMethods = Arrays
725-
.stream(ClientAuthenticationMethod.class.getFields())
726-
.filter((field) -> Modifier.isFinal(field.getModifiers())
727-
&& field.getType() == ClientAuthenticationMethod.class)
728-
.map((field) -> getStaticValue(field, ClientAuthenticationMethod.class))
729-
.filter((authenticationMethod) -> authenticationMethod != ClientAuthenticationMethod.NONE)
730-
.map((authenticationMethod) -> new ClientAuthenticationMethod(authenticationMethod.getValue()))
731-
.toList();
732-
for (ClientAuthenticationMethod clientAuthenticationMethod : clientAuthenticationMethods) {
733-
ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder()
734-
.requireProofKey(true)
735-
.build();
736-
ClientRegistration clientRegistration = ClientRegistration.withRegistrationId(REGISTRATION_ID)
737-
.clientId(CLIENT_ID)
738-
.clientSettings(pkceEnabled)
739-
.authorizationGrantType(
740-
new AuthorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE.getValue()))
741-
.clientAuthenticationMethod(clientAuthenticationMethod)
742-
.redirectUri(REDIRECT_URI)
743-
.authorizationUri(AUTHORIZATION_URI)
744-
.tokenUri(TOKEN_URI)
745-
.build();
746-
assertThat(clientRegistration.getClientSettings().isRequireProofKey()).isTrue();
747-
}
748-
}
749-
750722
@ParameterizedTest
751723
@MethodSource("invalidPkceGrantTypes")
752-
void buildWhenInvalidGrantTypeForPkceThenException(AuthorizationGrantType invalidGrantType) {
724+
void buildWhenInvalidGrantTypeForPkceThenPkceDisabled(AuthorizationGrantType invalidGrantType) {
753725
ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder()
754726
.requireProofKey(true)
755727
.build();

‎oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java‎

Lines changed: 12 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,6 @@ public class DefaultOAuth2AuthorizationRequestResolverTests {
6060

6161
private ClientRegistration pkceClientRegistration;
6262

63-
private ClientRegistration nonProofKeyPublicClientRegistration;
64-
6563
private ClientRegistration fineRedirectUriTemplateRegistration;
6664

6765
private ClientRegistration publicClientRegistration;
@@ -80,11 +78,6 @@ public void setUp() {
8078
this.registration2 = TestClientRegistrations.clientRegistration2().build();
8179

8280
this.pkceClientRegistration = pkceClientRegistration().build();
83-
this.nonProofKeyPublicClientRegistration = TestClientRegistrations.clientRegistration()
84-
.registrationId("invalid-public-client-registration-id")
85-
.clientAuthenticationMethod(ClientAuthenticationMethod.NONE)
86-
.clientSettings(ClientRegistration.ClientSettings.builder().requireProofKey(false).build())
87-
.build();
8881
this.fineRedirectUriTemplateRegistration = fineRedirectUriTemplateClientRegistration().build();
8982
// @formatter:off
9083
this.publicClientRegistration = TestClientRegistrations.clientRegistration()
@@ -100,7 +93,7 @@ public void setUp() {
10093
// @formatter:on
10194
this.clientRegistrationRepository = new InMemoryClientRegistrationRepository(this.registration1,
10295
this.registration2, this.pkceClientRegistration, this.fineRedirectUriTemplateRegistration,
103-
this.publicClientRegistration, this.oidcRegistration, this.nonProofKeyPublicClientRegistration);
96+
this.publicClientRegistration, this.oidcRegistration);
10497
this.resolver = new DefaultOAuth2AuthorizationRequestResolver(this.clientRegistrationRepository,
10598
this.authorizationRequestBaseUri);
10699
}
@@ -396,33 +389,6 @@ public void resolveWhenAuthorizationRequestWithValidPublicClientThenResolves() {
396389
// gh-6548
397390
@Test
398391
public void resolveWhenAuthorizationRequestApplyPkceToConfidentialClientsThenApplied() {
399-
this.resolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce());
400-
401-
ClientRegistration clientRegistration = this.registration1;
402-
String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId();
403-
MockHttpServletRequest request = get(requestUri).build();
404-
OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request);
405-
assertPkceApplied(authorizationRequest, clientRegistration);
406-
407-
clientRegistration = this.registration2;
408-
requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId();
409-
request = get(requestUri).build();
410-
authorizationRequest = this.resolver.resolve(request);
411-
assertPkceApplied(authorizationRequest, clientRegistration);
412-
}
413-
414-
// gh-6548
415-
@Test
416-
public void resolveWhenAuthorizationRequestApplyPkceToSpecificConfidentialClientThenApplied() {
417-
this.resolver.setAuthorizationRequestCustomizer((builder) -> {
418-
builder.attributes((attrs) -> {
419-
String registrationId = (String) attrs.get(OAuth2ParameterNames.REGISTRATION_ID);
420-
if (this.registration1.getRegistrationId().equals(registrationId)) {
421-
OAuth2AuthorizationRequestCustomizers.withPkce().accept(builder);
422-
}
423-
});
424-
});
425-
426392
ClientRegistration clientRegistration = this.registration1;
427393
String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId();
428394
MockHttpServletRequest request = get(requestUri).build();
@@ -549,6 +515,17 @@ public void resolveWhenAuthorizationRequestCustomizerOverridesParameterThenQuery
549515
+ "&code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&code_challenge_method=S256&appid=client-id");
550516
}
551517

518+
@Test
519+
public void resolveWhenAuthorizationRequestNoProvideAuthorizationRequestBaseUri() {
520+
OAuth2AuthorizationRequestResolver resolver = new DefaultOAuth2AuthorizationRequestResolver(
521+
this.clientRegistrationRepository);
522+
String requestUri = this.authorizationRequestBaseUri + "/" + this.registration2.getRegistrationId();
523+
MockHttpServletRequest request = get(requestUri).build();
524+
OAuth2AuthorizationRequest authorizationRequest = resolver.resolve(request);
525+
assertThat(authorizationRequest.getRedirectUri())
526+
.isEqualTo("http://localhost/login/oauth2/code/" + this.registration2.getRegistrationId());
527+
}
528+
552529
@Test
553530
public void resolveWhenAuthorizationRequestProvideCodeChallengeMethod() {
554531
ClientRegistration clientRegistration = this.pkceClientRegistration;

‎oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java‎

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import org.springframework.security.oauth2.client.registration.ClientRegistration;
3030
import org.springframework.security.oauth2.client.registration.ReactiveClientRegistrationRepository;
3131
import org.springframework.security.oauth2.client.registration.TestClientRegistrations;
32-
import org.springframework.security.oauth2.client.web.OAuth2AuthorizationRequestCustomizers;
3332
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
3433
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
3534
import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames;
@@ -59,18 +58,11 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests {
5958

6059
private DefaultServerOAuth2AuthorizationRequestResolver resolver;
6160

62-
private ClientRegistration nonProofKeyPublicClientRegistration;
63-
6461
private ClientRegistration registration = TestClientRegistrations.clientRegistration().build();
6562

6663
@BeforeEach
6764
public void setup() {
6865
this.resolver = new DefaultServerOAuth2AuthorizationRequestResolver(this.clientRegistrationRepository);
69-
this.nonProofKeyPublicClientRegistration = TestClientRegistrations.clientRegistration()
70-
.registrationId("invalid-public-client-registration-id")
71-
.clientAuthenticationMethod(ClientAuthenticationMethod.NONE)
72-
.clientSettings(ClientRegistration.ClientSettings.builder().requireProofKey(false).build())
73-
.build();
7466
}
7567

7668
@Test
@@ -143,41 +135,13 @@ public void resolveWhenAuthorizationRequestApplyPkceToConfidentialClientsThenApp
143135
given(this.clientRegistrationRepository.findByRegistrationId(eq(registration2.getRegistrationId())))
144136
.willReturn(Mono.just(registration2));
145137

146-
this.resolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce());
147-
148138
OAuth2AuthorizationRequest request = resolve("/oauth2/authorization/" + registration1.getRegistrationId());
149139
assertPkceApplied(request, registration1);
150140

151141
request = resolve("/oauth2/authorization/" + registration2.getRegistrationId());
152142
assertPkceApplied(request, registration2);
153143
}
154144

155-
// gh-6548
156-
@Test
157-
public void resolveWhenAuthorizationRequestApplyPkceToSpecificConfidentialClientThenApplied() {
158-
ClientRegistration registration1 = TestClientRegistrations.clientRegistration().build();
159-
given(this.clientRegistrationRepository.findByRegistrationId(eq(registration1.getRegistrationId())))
160-
.willReturn(Mono.just(registration1));
161-
given(this.clientRegistrationRepository
162-
.findByRegistrationId(eq(this.nonProofKeyPublicClientRegistration.getRegistrationId())))
163-
.willReturn(Mono.just(this.nonProofKeyPublicClientRegistration));
164-
165-
this.resolver.setAuthorizationRequestCustomizer((builder) -> {
166-
builder.attributes((attrs) -> {
167-
String registrationId = (String) attrs.get(OAuth2ParameterNames.REGISTRATION_ID);
168-
if (registration1.getRegistrationId().equals(registrationId)) {
169-
OAuth2AuthorizationRequestCustomizers.withPkce().accept(builder);
170-
}
171-
});
172-
});
173-
174-
OAuth2AuthorizationRequest request = resolve("/oauth2/authorization/" + registration1.getRegistrationId());
175-
assertPkceApplied(request, registration1);
176-
177-
request = resolve("/oauth2/authorization/" + this.nonProofKeyPublicClientRegistration.getRegistrationId());
178-
assertPkceApplied(request, this.nonProofKeyPublicClientRegistration);
179-
}
180-
181145
@Test
182146
void resolveWhenRequireProofKeyTrueThenPkceEnabled() {
183147
ClientRegistration.ClientSettings pkceEnabled = ClientRegistration.ClientSettings.builder()

0 commit comments

Comments
 (0)