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.

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

When parsing OAuth2 access token response a nested JSON object causes the response parsing to fail #6463

Open
buckett opened this issue Jan 21, 2019 · 5 comments

Comments

@buckett
Copy link

@buckett buckett commented Jan 21, 2019

Summary

When parsing OAuth2 access token response a nested JSON object causes the response parsing to fail.

Actual Behavior

When attempting to use Spring Security OAuth to allow logins against a provider that responds with objects in their access token reponse an error message is shown:

An error occurred reading the OAuth 2.0 Access Token Response: JSON parse error: Cannot deserialize instance of `java.lang.String` out of START_OBJECT token; nested exception is com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot deserialize instance of `java.lang.String` out of START_OBJECT token
 at [Source: (ByteArrayInputStream); line: 7, column: 14] (through reference chain: java.util.LinkedHashMap["object"]); nested exception is org.springframework.http.converter.HttpMessageNotReadableException: JSON parse error: Cannot deserialize instance of `java.lang.String` out of START_OBJECT token; nested exception is com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot deserialize instance of `java.lang.String` out of START_OBJECT token
 at [Source: (ByteArrayInputStream); line: 7, column: 14] (through reference chain: java.util.LinkedHashMap["object"])

Expected Behavior

According to the OAuth spec https://tools.ietf.org/html/rfc6749#section-5.1 clients must ignore values they don't understand. The value should either end up in the additionalParameters of the OAuth2AccessTokenResponse or it should be ignored.

Configuration

Jackson is being used to parse the JSON response (seems to be default in my spring-boot application).

Version

Spring Security 5.1.3, issue also looks to be present on master.

Sample

You can see a test case that currently fails in: https://github.com/spring-projects/spring-security/compare/master...buckett:oauth-response?expand=1

@buckett

This comment has been minimized.

Copy link
Author

@buckett buckett commented Jan 21, 2019

An example response that triggers this bug is:

{
   "access_token": "access-token-1234",
   "token_type": "bearer",
   "expires_in": "3600",
   "scope": "read write",
   "refresh_token": "refresh-token-1234",
   "object": {"param": "value" },
   "custom_parameter": "custom-value"
}
@buckett

This comment has been minimized.

Copy link
Author

@buckett buckett commented Jan 21, 2019

To workaround this I copied OAuth2AccessTokenResponseHttpMessageConverter into my own project and updated the tokenResponseParameters to be a Map<String, Object>. Here's a gist of the file: https://gist.github.com/buckett/7fe338901b4fcfefb6cfce637629af3f

Then I just connect this converter up to the RestTemplate that's used by the OAuth 2 code:

        .oauth2Login().tokenEndpoint().accessTokenResponseClient(accessTokenResposeClient());

[..snipped..]

    private OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> accessTokenResposeClient() {
        DefaultAuthorizationCodeTokenResponseClient client = new DefaultAuthorizationCodeTokenResponseClient();
        RestTemplate restTemplate = new RestTemplate(Arrays.asList(
                new FormHttpMessageConverter(), new OAuth2AccessTokenResponseHttpMessageConverter()));
        HttpClient requestFactory = HttpClientBuilder.create().build();
        restTemplate.setRequestFactory(new HttpComponentsClientHttpRequestFactory(requestFactory));
        restTemplate.setErrorHandler(new OAuth2ErrorResponseErrorHandler());
        client.setRestOperations(restTemplate);
        return client;
    }

This isn't the actual code I'm using as I've removed other changes that aren't needed so it may not work as expected.

@jgrandja jgrandja self-assigned this Jan 22, 2019
@jgrandja jgrandja added the in: oauth2 label Jan 22, 2019
@jgrandja

This comment has been minimized.

Copy link
Collaborator

@jgrandja jgrandja commented Jan 22, 2019

@buckett The solution you implemented with providing your own AbstractHttpMessageConverter<OAuth2AccessTokenResponse> is exactly what you would do for these kind of custom token responses. This is the main extension point for RestTemplate, the HttpMessageConverter for request/response serializing/deserializing. OAuth2AccessTokenResponseHttpMessageConverter is simply a default implementation.

It's not clear to me if you're still having a problem or are you expecting a different way of implementing your setup?

As per spec:

The parameters are serialized into a JavaScript Object Notation (JSON)
structure by adding each parameter at the highest structure level.
Parameter names and string values are included as JSON strings.
Numerical values are included as JSON numbers. The order of
parameters does not matter and can vary.

Based on my understanding how the spec reads, each parameter must be at the highest (root) structure level and are either strings or numbers. However, your example token response has a JSON object value at the root level with the parameter names/values at the next level below the object. So therefore the value of the root level parameter is an object and not a string or number as the spec dictates.

Does this make sense?

@buckett

This comment has been minimized.

Copy link
Author

@buckett buckett commented Jan 22, 2019

When I read the spec I was reading that paragraph as applying to the previously specified parameters (access_token, token_type, expires_in, refresh_token, scope).

It also goes on to say that the "the client MUST ignore unrecognized values names in the response", which again suggests we should be a little more forgiving in how we deal with unexpected value names.

If nothing else it would have been helpful if this was easier to code around as this took me a little while to debug (I came across this upgrading from Spring 5 to Spring 5.1, Spring 5 was forgiving of extra values I think).

@jgrandja

This comment has been minimized.

Copy link
Collaborator

@jgrandja jgrandja commented Jan 22, 2019

As per spec:

The client MUST ignore unrecognized value names in the response. The
sizes of tokens and other values received from the authorization
server are left undefined. The client should avoid making
assumptions about value sizes.
The authorization server SHOULD
document the size of any value it issues.

I see your point based on the reference in bold.

If nothing else it would have been helpful if this was easier to code around

Our goal is to make it easy for the user so this is valuable feedback for us. Do you have a suggestion on improving? Are you possibly interested in submitting a PR for this improvement?

@jgrandja jgrandja removed their assignment Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.