Add support for customizing the Access Token Request #5656

jgrandja · 2018-08-10T16:43:25Z

This PR resolves #5547 and #5466

The 1st commit pushes most of the code from DefaultClientCredentialsTokenResponseClient to an abstract implementation AbstractOAuth2AccessTokenResponseClient to allow for reuse in the next commit.

The 2nd commit adds a new implementation of OAuth2AccessTokenResponseClient for the authorization_code grant DefaultAuthorizationCodeTokenResponseClient - extending AbstractOAuth2AccessTokenResponseClient.

The 3rd commit makes DefaultAuthorizationCodeTokenResponseClient the default client replacing the previous default NimbusAuthorizationCodeTokenResponseClient.

gburboz · 2018-08-10T19:51:37Z

...springframework/security/oauth2/client/endpoint/AbstractOAuth2AccessTokenResponseClient.java

+	}
+
+	@Override
+	public OAuth2AccessTokenResponse getTokenResponse(T authorizationGrantRequest) throws OAuth2AuthenticationException {


Rename parameter authorizationGrantRequest to accessTokenRequest and mark it final

rwinch · 2018-08-14T21:39:56Z

At a high level, I don't really like the idea of the AbstractOAuth2AccessTokenResponseClient. It seems to be introducing additional APIs that are already available in RestTemplate. For example, there is a buildRequestMethod which is already supported in RestTemplate using a MessageConverter. The conversion of the error response is already possible using a ResponseErrorHandler. The parsing of the response is possible using a MessageConverter as well. The disadvantage is that we now need specific hooks for each request type times each type of customization part of the request that needs customized (i.e. headers, body, etc).

I propose the code be refactored to be exposed using the existing APIs within RestTemplate. The DefaultClientCredentialsTokenResponseClient and DefaultAuthorizationCodeTokenResponseClient can then use a RestTemplate with the appropriate API implementations registered by default. User's would then be required to provide a RestOperations implementation that can convert from a request/response to the OAuth2AccessTokenResponseClient.

Note that proper setup of the RestTempalte is already required since the AbstractOAuth2AccessTokenResponseClient relies on a NoOpResponseErrorHandler being set to handle the errors. As it stands the user cannot inject a custom RestOperations and expect this to work without creating their own NoOpResponseErrorHandler.

rwinch

Please see #5656 (comment)

jgrandja · 2018-08-20T14:42:14Z

@rwinch Ok this makes sense as far as leveraging HttpMessageConverter for reading OAuth2AccessTokenResponse on a success response and reading OAuth2Error using ResponseErrorHandler on error response.

Can you clarify:

there is a buildRequestMethod which is already supported in RestTemplate using a MessageConverter.

I can't seem to find buildRequestMethod? Did you mean to leverage RequestCallback?

rwinch · 2018-08-20T16:12:28Z

Sorry that was a typo. I meant to type buildRequest method.

To be clear...I'm not certain which pieces of RestOperations/RestTemplate make the most sense for us to leverage. What seems clear to me is that there are enough APIs for us to leverage within RestTemplate already.

rwinch · 2018-08-23T19:51:05Z

...springframework/security/oauth2/client/endpoint/AbstractOAuth2AuthorizationGrantRequest.java

+	 * @param authorizationGrantType the authorization grant type
+	 * @param clientRegistration the client registration
+	 */
+	protected AbstractOAuth2AuthorizationGrantRequest(


Is this change necessary for this PR? I'm not sure it adds a lot especially with the value being optional vs in subclasses we know it is non-null. At minimum, I'd like to consider this change separately if possible.

Agreed this change can be removed

rwinch · 2018-08-23T19:52:54Z

...ngframework/security/oauth2/client/endpoint/DefaultAuthorizationCodeTokenResponseClient.java

+import org.springframework.http.ResponseEntity;
+import org.springframework.http.client.ClientHttpResponse;
+import org.springframework.http.converter.FormHttpMessageConverter;
+import org.springframework.security.oauth2.client.endpoint.converter.OAuth2AuthorizationCodeGrantRequestEntityConverter;


This is a package tangle because a parent package is accessing a subpackage of itself.

i'm wondering if to fix the tangle if we could move OAuth2AuthorizationCodeGrantRequestEntityConverter and OAuth2ClientCredentialsGrantRequestEntityConverter to org.springframework.security.oauth2.core.http.converter

OAuth2AuthorizationCodeGrantRequest and OAuth2ClientCredentialsGrantRequest reside in the client.endpoint package so we should keep it in the client module.

The 2 options I can think of right now are:
org.springframework.security.oauth2.client.endpoint (keep in same package)
org.springframework.security.oauth2.client.converter

Good point, I think given it is associated with RequestEntity, we should consider a variation of what you proposed. What do you think of org.springframework.security.oauth2.client.http.converter

Agreed that makes sense....let's go with org.springframework.security.oauth2.client.http.converter

rwinch · 2018-08-23T19:55:23Z

...ngframework/security/oauth2/client/endpoint/DefaultAuthorizationCodeTokenResponseClient.java

+ * @see <a target="_blank" href="https://tools.ietf.org/html/rfc6749#section-4.1.3">Section 4.1.3 Access Token Request (Authorization Code Grant)</a>
+ * @see <a target="_blank" href="https://tools.ietf.org/html/rfc6749#section-4.1.4">Section 4.1.4 Access Token Response (Authorization Code Grant)</a>
+ */
+public class DefaultAuthorizationCodeTokenResponseClient implements OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> {


Most of the methods are already final, let's make the class final

rwinch · 2018-08-23T19:57:53Z

...ngframework/security/oauth2/client/endpoint/DefaultClientCredentialsTokenResponseClient.java

 	 *
-	 * @param restOperations the {@link RestOperations} used when requesting the access token response
+	 * @param restOperations the {@link RestOperations} used when requesting the Access Token Response
 	 */
 	public final void setRestOperations(RestOperations restOperations) {


How can someone inject the RestOperations and still get valid error handling? I think we may need to make OAuth2ErrorResponseErrorHandler public.

We should document what the RestOperations required (i.e. the types of converters it would need, error handling, etc)

I was thinking of exposing OAuth2ErrorResponseErrorHandler but the implementation was pretty easy so decided to hide it.

Either way, I'm fine with exposing it.

rwinch · 2018-08-23T19:59:18Z

...ity/oauth2/client/endpoint/converter/OAuth2AuthorizationCodeGrantRequestEntityConverter.java

+	 * @param authorizationCodeGrantRequest the authorization code grant request
+	 * @return the {@link HttpHeaders} used for the Access Token Request
+	 */
+	protected HttpHeaders buildHeaders(OAuth2AuthorizationCodeGrantRequest authorizationCodeGrantRequest) {


I'd like this to be private. User's that need to customize the headers can do so by delegation to this class and constructing a modified RequestEntity.

rwinch · 2018-08-23T19:59:25Z

...ity/oauth2/client/endpoint/converter/OAuth2AuthorizationCodeGrantRequestEntityConverter.java

+	 * @param authorizationCodeGrantRequest the authorization code grant request
+	 * @return a {@link MultiValueMap} of the form parameters used for the Access Token Request body
+	 */
+	protected MultiValueMap<String, String> buildFormParameters(OAuth2AuthorizationCodeGrantRequest authorizationCodeGrantRequest) {


I'd like this to be private. User's that need to customize the headers can do so by delegation to this class and constructing a modified RequestEntity.

rwinch · 2018-08-23T19:59:56Z

...ity/oauth2/client/endpoint/converter/OAuth2ClientCredentialsGrantRequestEntityConverter.java

+	 * @param clientCredentialsGrantRequest the client credentials grant request
+	 * @return the {@link HttpHeaders} used for the Access Token Request
+	 */
+	protected HttpHeaders buildHeaders(OAuth2ClientCredentialsGrantRequest clientCredentialsGrantRequest) {


I'd like this to be private. User's that need to customize the headers can do so by delegation to this class and constructing a modified RequestEntity.

rwinch · 2018-08-23T20:00:01Z

...ity/oauth2/client/endpoint/converter/OAuth2ClientCredentialsGrantRequestEntityConverter.java

+	 * @param clientCredentialsGrantRequest the client credentials grant request
+	 * @return a {@link MultiValueMap} of the form parameters used for the Access Token Request body
+	 */
+	protected MultiValueMap<String, String> buildFormParameters(OAuth2ClientCredentialsGrantRequest clientCredentialsGrantRequest) {


I'd like this to be private. User's that need to customize the headers can do so by delegation to this class and constructing a modified RequestEntity.

rwinch · 2018-08-23T20:05:05Z

...ngframework/security/oauth2/client/endpoint/DefaultClientCredentialsTokenResponseClient.java

 	 *
-	 * @param restOperations the {@link RestOperations} used when requesting the access token response
+	 * @param restOperations the {@link RestOperations} used when requesting the Access Token Response
 	 */
 	public final void setRestOperations(RestOperations restOperations) {


We should document what the RestOperations required (i.e. the types of converters it would need, error handling, etc)

rwinch · 2018-08-23T20:05:40Z

...ngframework/security/oauth2/client/endpoint/DefaultAuthorizationCodeTokenResponseClient.java

+	 *
+	 * @param restOperations the {@link RestOperations} used when requesting the Access Token Response
+	 */
+	public final void setRestOperations(RestOperations restOperations) {


Same comments as the other setRestOperations

rwinch · 2018-08-23T20:07:11Z

...ngframework/security/oauth2/client/endpoint/DefaultAuthorizationCodeTokenResponseClient.java

+import org.springframework.http.ResponseEntity;
+import org.springframework.http.client.ClientHttpResponse;
+import org.springframework.http.converter.FormHttpMessageConverter;
+import org.springframework.security.oauth2.client.endpoint.converter.OAuth2AuthorizationCodeGrantRequestEntityConverter;


i'm wondering if to fix the tangle if we could move OAuth2AuthorizationCodeGrantRequestEntityConverter and OAuth2ClientCredentialsGrantRequestEntityConverter to org.springframework.security.oauth2.core.http.converter

Fixes spring-projectsgh-5547

jgrandja · 2018-08-25T00:58:59Z

@rwinch I've applied the updates as discussed. I've also rebased/squashed to 1 commit and excluded the changes to DefaultClientCredentialsTokenResponseClient. After this is merged, I'll resolve #5735

rwinch · 2018-08-27T02:49:30Z

...ngframework/security/oauth2/client/endpoint/DefaultAuthorizationCodeTokenResponseClient.java

+import org.springframework.http.converter.FormHttpMessageConverter;
+import org.springframework.http.converter.HttpMessageConverter;
+import org.springframework.security.oauth2.client.http.OAuth2ErrorResponseErrorHandler;
+import org.springframework.security.oauth2.client.http.converter.OAuth2AuthorizationCodeGrantRequestEntityConverter;


Unfortunately this is another package tangle. The client.endpoint package refers to http.converter above and http.converter refers to client.endpoint

jgrandja · 2018-08-27T13:37:06Z

@rwinch DefaultAuthorizationCodeTokenResponseClient and OAuth2AuthorizationCodeGrantRequest reside in the client.endpoint so I moved OAuth2AuthorizationCodeGrantRequestEntityConverter to the same package as this made the most sense rather than it's own http.converter package.

DefaultAuthorizationCodeTokenResponseClient uses OAuth2AuthorizationCodeGrantRequestEntityConverter which uses OAuth2AuthorizationCodeGrantRequest.

rwinch · 2018-08-27T16:01:18Z

Thanks for the change. Overall it looks good. Before we merge:

Please squash
Please rebase off master and ensure the build passes as there have been some recent checkstyle changes

Once that is done and the build passes, please feel free to merge.

jgrandja · 2018-08-27T17:00:41Z

Thanks @rwinch. This is now in master.

jgrandja requested a review from rwinch August 10, 2018 16:43

This was referenced Aug 10, 2018

Support Custom Parameters within OAuth Token Request #5466

Closed

Some OAuth providers do not comply with specs #5657

Closed

gburboz reviewed Aug 10, 2018

View reviewed changes

rwinch requested changes Aug 14, 2018

View reviewed changes

rwinch reviewed Aug 23, 2018

View reviewed changes

rwinch requested changes Aug 23, 2018

View reviewed changes

Add DefaultAuthorizationCodeTokenResponseClient

34f7f5d

Fixes spring-projectsgh-5547

jgrandja force-pushed the gh-5547-restops-token-client branch from d54e598 to 34f7f5d Compare August 25, 2018 00:48

jgrandja mentioned this pull request Aug 25, 2018

Re-factor DefaultClientCredentialsTokenResponseClient #5735

Closed

rwinch requested changes Aug 27, 2018

View reviewed changes

Fix package tangle

4347223

Move test

800c8de

jgrandja closed this Aug 27, 2018

jgrandja deleted the gh-5547-restops-token-client branch August 27, 2018 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for customizing the Access Token Request #5656

Add support for customizing the Access Token Request #5656

jgrandja commented Aug 10, 2018

gburboz Aug 10, 2018

rwinch commented Aug 14, 2018 •

edited

Loading

rwinch left a comment

jgrandja commented Aug 20, 2018

rwinch commented Aug 20, 2018

rwinch Aug 23, 2018 •

edited

Loading

jgrandja Aug 23, 2018

rwinch Aug 23, 2018

rwinch Aug 23, 2018

jgrandja Aug 23, 2018

rwinch Aug 24, 2018

jgrandja Aug 24, 2018

rwinch Aug 23, 2018

rwinch Aug 23, 2018

rwinch Aug 23, 2018

jgrandja Aug 23, 2018

rwinch Aug 23, 2018

rwinch Aug 23, 2018

rwinch Aug 23, 2018

rwinch Aug 23, 2018

rwinch Aug 23, 2018

rwinch Aug 23, 2018

rwinch Aug 23, 2018

jgrandja commented Aug 25, 2018

rwinch Aug 27, 2018

jgrandja commented Aug 27, 2018 •

edited

Loading

rwinch commented Aug 27, 2018 •

edited

Loading

jgrandja commented Aug 27, 2018

Add support for customizing the Access Token Request #5656

Add support for customizing the Access Token Request #5656

Conversation

jgrandja commented Aug 10, 2018

Choose a reason for hiding this comment

rwinch commented Aug 14, 2018 • edited Loading

rwinch left a comment

Choose a reason for hiding this comment

jgrandja commented Aug 20, 2018

rwinch commented Aug 20, 2018

rwinch Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja commented Aug 25, 2018

Choose a reason for hiding this comment

jgrandja commented Aug 27, 2018 • edited Loading

rwinch commented Aug 27, 2018 • edited Loading

jgrandja commented Aug 27, 2018

rwinch commented Aug 14, 2018 •

edited

Loading

rwinch Aug 23, 2018 •

edited

Loading

jgrandja commented Aug 27, 2018 •

edited

Loading

rwinch commented Aug 27, 2018 •

edited

Loading