Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Disabling application/x-www-form-urlencoded Encoding Client ID and Secret #14859

Closed
wants to merge 1 commit into from
+155 −122
Closed

Simplify Disabling application/x-www-form-urlencoded Encoding Client ID and Secret #14859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/modules/ROOT/pages/servlet/oauth2/client/client-authentication.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ val tokenResponseClient = DefaultAuthorizationCodeTokenResponseClient()
tokenResponseClient.setRequestEntityConverter(requestEntityConverter)
----
======

[NOTE]
If you're using the `client-authentication-method: client_secret_basic` and you need to skip URL encoding,
create a new `DefaultOAuth2TokenRequestHeadersConverter` and set it in the Request Entity Converter above.
Comment on lines +95 to +97
Copy link
Member

@sjohnr sjohnr Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note seems to be added in the "Authenticate using private_key_jwt" section? If so, this wouldn't be the best place to add this note. Perhaps it could be a [TIP] elsewhere? For example, it could be added to authorization-grants.adoc where we cover customizing the access token request for each grant type. But this would require adding the tip 5 times because we unfortunately have structured the content that way currently. Or we could (for now) add an "Authenticate using client_secret_basic" section on this page.

Copy link
Contributor Author

@Crain-32 Crain-32 Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll format and push the code I have broken tests (See other comments) so I'll post the relevant stuff here, and we can discuss the documentation side after we feel good about the integration of the DefaultOAuthtokenREquestHeadersConverter into the Reactive and Non-Reactive side. Since I can see that impacting the documentation we set up.
I've swapped the AbstractWebClientReactiveOAuth2AccessTokenResponseClient to use the DefaultOAuth2TokenREquestHeadersConverter

private Converter<T, HttpHeaders> headersConverter = new DefaultOAuth2TokenRequestHeadersConverter<>();

As other comments state, I've also swapped the default headers to be done as follows.

private HttpHeaders initialTokenHeaders = getDefaultTokenRequestHeaders();

private static HttpHeaders getDefaultTokenRequestHeaders() {
HttpHeaders headers = new HttpHeaders();
headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON));
headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED);
return headers;
}
// There is a setter for the initialTokenHeaders as well

I do currently have the DefaultOAuth2TokenRequestHeadersConverter set to be public final, so that way consumers can customize it by just creating their own instance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix things so that tests are not broken (see above recommendations). As mentioned above, let's also remove getDefaultTokenRequestHeaders().


=== Authenticate using `client_secret_jwt`

Expand Down
9 changes: 3 additions & 6 deletions ...curity/oauth2/client/endpoint/AbstractOAuth2AuthorizationGrantRequestEntityConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2024 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.
Expand Down Expand Up @@ -42,11 +42,8 @@
abstract class AbstractOAuth2AuthorizationGrantRequestEntityConverter<T extends AbstractOAuth2AuthorizationGrantRequest>
implements Converter<T, RequestEntity<?>> {

// @formatter:off
private Converter<T, HttpHeaders> headersConverter =
(authorizationGrantRequest) -> OAuth2AuthorizationGrantRequestEntityUtils
.getTokenRequestHeaders(authorizationGrantRequest.getClientRegistration());
// @formatter:on
private Converter<T, HttpHeaders> headersConverter = DefaultOAuth2TokenRequestHeadersConverter
.historicalConverter();

private Converter<T, MultiValueMap<String, String>> parametersConverter = this::createParameters;

Expand Down
37 changes: 3 additions & 34 deletions ...rity/oauth2/client/endpoint/AbstractWebClientReactiveOAuth2AccessTokenResponseClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2023 the original author or authors.
* Copyright 2002-2024 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.
Expand All @@ -16,17 +16,13 @@

package org.springframework.security.oauth2.client.endpoint;

import java.io.UnsupportedEncodingException;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.Set;

import reactor.core.publisher.Mono;

import org.springframework.core.convert.converter.Converter;
import org.springframework.http.HttpHeaders;
import org.springframework.http.MediaType;
import org.springframework.http.ReactiveHttpInputMessage;
import org.springframework.security.oauth2.client.registration.ClientRegistration;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
Expand Down Expand Up @@ -65,6 +61,7 @@
* @see WebClientReactiveClientCredentialsTokenResponseClient
* @see WebClientReactivePasswordTokenResponseClient
* @see WebClientReactiveRefreshTokenTokenResponseClient
* @see DefaultOAuth2TokenRequestHeadersConverter
*/
public abstract class AbstractWebClientReactiveOAuth2AccessTokenResponseClient<T extends AbstractOAuth2AuthorizationGrantRequest>
implements ReactiveOAuth2AccessTokenResponseClient<T> {
Expand All @@ -73,7 +70,7 @@ public abstract class AbstractWebClientReactiveOAuth2AccessTokenResponseClient<T

private Converter<T, RequestHeadersSpec<?>> requestEntityConverter = this::validatingPopulateRequest;

private Converter<T, HttpHeaders> headersConverter = this::populateTokenRequestHeaders;
private Converter<T, HttpHeaders> headersConverter = new DefaultOAuth2TokenRequestHeadersConverter<>();

private Converter<T, MultiValueMap<String, String>> parametersConverter = this::populateTokenRequestParameters;

Expand Down Expand Up @@ -131,34 +128,6 @@ private RequestHeadersSpec<?> populateRequest(T grantRequest) {
.body(createTokenRequestBody(grantRequest));
}

/**
* Populates the headers for the token request.
* @param grantRequest the grant request
* @return the headers populated for the token request
*/
private HttpHeaders populateTokenRequestHeaders(T grantRequest) {
HttpHeaders headers = new HttpHeaders();
ClientRegistration clientRegistration = clientRegistration(grantRequest);
headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED);
headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON));
if (ClientAuthenticationMethod.CLIENT_SECRET_BASIC.equals(clientRegistration.getClientAuthenticationMethod())) {
String clientId = encodeClientCredential(clientRegistration.getClientId());
String clientSecret = encodeClientCredential(clientRegistration.getClientSecret());
headers.setBasicAuth(clientId, clientSecret);
}
return headers;
}

private static String encodeClientCredential(String clientCredential) {
try {
return URLEncoder.encode(clientCredential, StandardCharsets.UTF_8.toString());
}
catch (UnsupportedEncodingException ex) {
// Will not happen since UTF-8 is a standard charset
throw new IllegalArgumentException(ex);
}
}

/**
* Populates default parameters for the token request.
* @param grantRequest the grant request
Expand Down
110 changes: 110 additions & 0 deletions ...gframework/security/oauth2/client/endpoint/DefaultOAuth2TokenRequestHeadersConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/*
* Copyright 2002-2024 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.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.security.oauth2.client.endpoint;

import org.springframework.core.convert.converter.Converter;
import org.springframework.http.HttpHeaders;
import org.springframework.http.MediaType;
import org.springframework.http.RequestEntity;
import org.springframework.security.oauth2.client.registration.ClientRegistration;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;

import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.Collections;

/**
* Default {@link Converter} used to convert an
* {@link AbstractOAuth2AuthorizationGrantRequest} to the {@link HttpHeaders} of aKk
* {@link RequestEntity} representation of an OAuth 2.0 Access Token Request for the
* specific Authorization Grant.
*
* @author Peter Eastham
* @author Joe Grandja
* @see AbstractOAuth2AuthorizationGrantRequestEntityConverter
* @since 6.3
*/
public final class DefaultOAuth2TokenRequestHeadersConverter<T extends AbstractOAuth2AuthorizationGrantRequest>
implements Converter<T, HttpHeaders> {

private MediaType accept = MediaType.APPLICATION_JSON;

private MediaType contentType = MediaType.APPLICATION_FORM_URLENCODED;

private boolean encodeClientCredentialsIfRequired = true;

/**
* Populates the headers for the token request.
* @param grantRequest the grant request
* @return the headers populated for the token request
*/
@Override
public HttpHeaders convert(T grantRequest) {
HttpHeaders headers = new HttpHeaders();
headers.setAccept(Collections.singletonList(accept));
headers.setContentType(contentType);
ClientRegistration clientRegistration = grantRequest.getClientRegistration();
if (ClientAuthenticationMethod.CLIENT_SECRET_BASIC.equals(clientRegistration.getClientAuthenticationMethod())) {
String clientId = encodeClientCredential(clientRegistration.getClientId());
String clientSecret = encodeClientCredential(clientRegistration.getClientSecret());
headers.setBasicAuth(clientId, clientSecret);
}
return headers;
}

private String encodeClientCredential(String clientCredential) {
String encodedCredential = clientCredential;
if (this.encodeClientCredentialsIfRequired) {
encodedCredential = URLEncoder.encode(clientCredential, StandardCharsets.UTF_8);
}
return encodedCredential;
}

/**
* Sets the behavior for if this URL Encoding the Client Credentials during the
* conversion.
* @param encodeClientCredentialsIfRequired if false, no URL encoding will happen
*/
public void setEncodeClientCredentials(boolean encodeClientCredentialsIfRequired) {
this.encodeClientCredentialsIfRequired = encodeClientCredentialsIfRequired;
}

/**
* MediaType to set for the Accept header. Default is application/json
* @param accept MediaType to use for the Accept header
*/
private void setAccept(MediaType accept) {
this.accept = accept;
}

/**
* MediaType to set for the Content Type header. Default is
* application/x-www-form-urlencoded
* @param contentType MediaType to use for the Content Type header
*/
private void setContentType(MediaType contentType) {
this.contentType = contentType;
}

static <T extends AbstractOAuth2AuthorizationGrantRequest> DefaultOAuth2TokenRequestHeadersConverter<T> historicalConverter() {
DefaultOAuth2TokenRequestHeadersConverter<T> converter = new DefaultOAuth2TokenRequestHeadersConverter<>();
converter.setAccept(MediaType.APPLICATION_JSON_UTF8);
converter.setContentType(MediaType.valueOf(MediaType.APPLICATION_FORM_URLENCODED_VALUE + ";charset=UTF-8"));
return converter;
}

}
78 changes: 0 additions & 78 deletions ...framework/security/oauth2/client/endpoint/OAuth2AuthorizationGrantRequestEntityUtils.java
View file
Open in desktop

This file was deleted.

39 changes: 36 additions & 3 deletions ...ework/security/oauth2/client/endpoint/OAuth2PasswordGrantRequestEntityConverterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2024 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.
Expand Down Expand Up @@ -110,7 +110,10 @@ public void convertWhenParametersConverterSetThenCalled() {
@SuppressWarnings("unchecked")
@Test
public void convertWhenGrantRequestValidThenConverts() {
ClientRegistration clientRegistration = TestClientRegistrations.password().build();
ClientRegistration clientRegistration = TestClientRegistrations.password()
.clientId("clientId")
.clientSecret("clientSecret=")
.build();
Comment on lines +113 to +116
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes should be fully compatible with existing tests, so this change should not be necessary. Is there some reason it needed to change? Instead, we should add new tests where appropriate for the new functionality. (See other comments)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the convertWhenGrantRequestValidThenConvertsWithoutUrlEncoding is the test that confirms the URL encoding toggle works, it felt like the correct move would be to include a slightly change on an earlier test that confirms the URL encoding default was enabled.

This felt better than copying and pasting an entire test just to add in a =, without any other changes. Where as the convertWhenGrantRequestValidThenConvertsWithoutUrlEncoding is a copy+paste of another test with the = added, and the toggle swapped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise the historical tests + this minor change mean the class has coverage, which sure is a little bit of a weak argument, but adding in several tests for this feels a bit much.

OAuth2PasswordGrantRequest passwordGrantRequest = new OAuth2PasswordGrantRequest(clientRegistration, "user1",
"password");
RequestEntity<?> requestEntity = this.converter.convert(passwordGrantRequest);
Expand All @@ -121,7 +124,7 @@ public void convertWhenGrantRequestValidThenConverts() {
assertThat(headers.getAccept()).contains(MediaType.APPLICATION_JSON_UTF8);
assertThat(headers.getContentType())
.isEqualTo(MediaType.valueOf(MediaType.APPLICATION_FORM_URLENCODED_VALUE + ";charset=UTF-8"));
assertThat(headers.getFirst(HttpHeaders.AUTHORIZATION)).startsWith("Basic ");
assertThat(headers.getFirst(HttpHeaders.AUTHORIZATION)).isEqualTo("Basic Y2xpZW50SWQ6Y2xpZW50U2VjcmV0JTNE");
MultiValueMap<String, String> formParameters = (MultiValueMap<String, String>) requestEntity.getBody();
assertThat(formParameters.getFirst(OAuth2ParameterNames.GRANT_TYPE))
.isEqualTo(AuthorizationGrantType.PASSWORD.getValue());
Expand All @@ -130,4 +133,34 @@ public void convertWhenGrantRequestValidThenConverts() {
assertThat(formParameters.getFirst(OAuth2ParameterNames.SCOPE)).contains(clientRegistration.getScopes());
}

@SuppressWarnings("unchecked")
@Test
public void convertWhenGrantRequestValidThenConvertsWithoutUrlEncoding() {
Crain-32 marked this conversation as resolved.
Show resolved Hide resolved
ClientRegistration clientRegistration = TestClientRegistrations.password()
.clientId("clientId")
.clientSecret("clientSecret=")
.build();
OAuth2PasswordGrantRequest passwordGrantRequest = new OAuth2PasswordGrantRequest(clientRegistration, "user1",
"password=");
DefaultOAuth2TokenRequestHeadersConverter<OAuth2PasswordGrantRequest> headersConverter = DefaultOAuth2TokenRequestHeadersConverter
.historicalConverter();
headersConverter.setEncodeClientCredentials(false);
this.converter.setHeadersConverter(headersConverter);
RequestEntity<?> requestEntity = this.converter.convert(passwordGrantRequest);
assertThat(requestEntity.getMethod()).isEqualTo(HttpMethod.POST);
assertThat(requestEntity.getUrl().toASCIIString())
.isEqualTo(clientRegistration.getProviderDetails().getTokenUri());
HttpHeaders headers = requestEntity.getHeaders();
assertThat(headers.getAccept()).contains(MediaType.APPLICATION_JSON_UTF8);
assertThat(headers.getContentType())
.isEqualTo(MediaType.valueOf(MediaType.APPLICATION_FORM_URLENCODED_VALUE + ";charset=UTF-8"));
assertThat(headers.getFirst(HttpHeaders.AUTHORIZATION)).isEqualTo("Basic Y2xpZW50SWQ6Y2xpZW50U2VjcmV0PQ==");
MultiValueMap<String, String> formParameters = (MultiValueMap<String, String>) requestEntity.getBody();
assertThat(formParameters.getFirst(OAuth2ParameterNames.GRANT_TYPE))
.isEqualTo(AuthorizationGrantType.PASSWORD.getValue());
assertThat(formParameters.getFirst(OAuth2ParameterNames.USERNAME)).isEqualTo("user1");
assertThat(formParameters.getFirst(OAuth2ParameterNames.PASSWORD)).isEqualTo("password=");
assertThat(formParameters.getFirst(OAuth2ParameterNames.SCOPE)).contains(clientRegistration.getScopes());
}

}