Skip to content

When OAuth2AccessTokenResponse AdditionalParameters contain long data would be truncated #1411

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

Closed
wanghongzhou opened this issue Oct 20, 2023 · 9 comments
Assignees
Labels
status: duplicate A duplicate of another issue

Comments

@wanghongzhou
Copy link

Oauth2AuthenticationSuccessHandler class invokes OAuth2AccessTokenResponseHttpMessageConverter transformed to the returned token, When OAuth2AccessTokenResponse additionalParameters contains Long data return an error value, jsonMessageConverter is Fixed in OAuth2AccessTokenResponseHttpMessageConverter :

private GenericHttpMessageConverter<Object> jsonMessageConverter = HttpMessageConverters.getJsonMessageConverter();

Hope to be able to share existing MappingJackson2HttpMessageConverter object in the container

@wanghongzhou wanghongzhou added the type: bug A general bug label Oct 20, 2023
@jgrandja
Copy link
Collaborator

@wanghongzhou I'm not sure I understand the issue you are having.

Please provide a minimal sample that reproduces the issue so I can look into it further.

@jgrandja jgrandja self-assigned this Oct 20, 2023
@jgrandja jgrandja added status: waiting-for-feedback We need additional information before we can continue and removed type: bug A general bug labels Oct 20, 2023
@wanghongzhou
Copy link
Author

@jgrandja
Sorry, because it was translated using a translation tool, I may not have expressed it clearly. The problem I want to say is: if there is a Long type value in the additionalParameters data in the token, when serializing it into json, the Long type number will not be serialized. into String, which will cause it to be truncated in JS. JSON serialization has been solidified. Can I use the MappingJackson2HttpMessageConverter that exists in the Spring container?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 24, 2023
@jgrandja
Copy link
Collaborator

jgrandja commented Oct 26, 2023

@wanghongzhou

jsonMessageConverter is Fixed in OAuth2AccessTokenResponseHttpMessageConverter

Can I use the MappingJackson2HttpMessageConverter that exists in the Spring container?

If Jackson is available in the classapth, then jsonMessageConverter will point to MappingJackson2HttpMessageConverter, see:

https://github.com/spring-projects/spring-security/blob/1ad2cfcf7b94f7dbcad1af10018b841fb99d3338/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/http/converter/HttpMessageConverters.java#L54

As mentioned in previous comment, can you provide a minimal sample or test that reproduces the issue so I can look into it further?

@jgrandja jgrandja added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 26, 2023
@wanghongzhou
Copy link
Author

@jgrandja

in https://github.com/spring-projects/spring-security/blob/1ad2cfcf7b94f7dbcad1af10018b841fb99d3338/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/http/converter/HttpMessageConverters.java#L54

MappingJackson2HttpMessageConverter is new, I can't to configure Jackson. For example, configure a value of type Long to be converted to a string when serialized. If not configured, the values of type Long are serialized with the JSON, and the values read by JavaScript are truncated.

You can refer to this address:
https://stackoverflow.com/questions/15869275/json-not-converting-long-numbers-appropriately

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 30, 2023
@wanghongzhou
Copy link
Author

in HttpMessageConverters.getJsonMessageConverter(), You need to prioritize getting it from the spring container rather than creating a new MappingJackson2HttpMessageConverter,Or provide other ways to customize jackson's behavior

@wanghongzhou
Copy link
Author

Still thank you for reading, my code is not good to peel off, in general:

If returned token data contained in the value of the Long after serialization into JSON, Javascript to read the value of is wrong, now I am through custom AuthenticationSuccessHandler to modify the behavior of the Jackson serialization, Will the value of the Long when serialized into a string, if you can support custom Jackson's behavior, need not custom AuthenticationSuccessHandler

@jgrandja
Copy link
Collaborator

@wanghongzhou

As a standard process to bug reporting, we ask the user to provide a minimal example or test that reproduces the issue. Explaining it is not sufficient.

I went ahead and wrote a test in OAuth2TokenEndpointFilterTests that adds a Long.MAX_VALUE in OAuth2AccessTokenResponse.additionalParameters() and the test passed without any truncation of the Long value.

Here is the test:

@Test
public void writeAccessTokenResponseWithAdditionalParameterLongMax() throws Exception {
	Instant expiresAt = Instant.now().plusSeconds(3600);
	Set<String> scopes = new LinkedHashSet<>(Arrays.asList("read", "write"));
	Map<String, Object> additionalParameters = new HashMap<>();
	additionalParameters.put("long-max-value", Long.MAX_VALUE);
	// @formatter:off
	OAuth2AccessTokenResponse accessTokenResponse = OAuth2AccessTokenResponse.withToken("access-token-1234")
			.tokenType(OAuth2AccessToken.TokenType.BEARER)
			.expiresIn(expiresAt.toEpochMilli())
			.scopes(scopes)
			.additionalParameters(additionalParameters)
			.build();
	// @formatter:on
	MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
	this.accessTokenHttpResponseConverter.write(accessTokenResponse, null, outputMessage);
	String tokenResponse = outputMessage.getBodyAsString();
	assertThat(tokenResponse).contains("\"access_token\":\"access-token-1234\"");
	assertThat(tokenResponse).contains("\"token_type\":\"Bearer\"");
	assertThat(tokenResponse).contains("\"scope\":\"read write\"");
	assertThat(tokenResponse).contains("\"long-max-value\":" + Long.MAX_VALUE + "");
}

Also, the SO post you referenced states the following:

This is a Javascript precision problem

Based on the test above, I'm not able to reproduce the issue. Please provide a test similar to above that reproduces the issue, otherwise this will get closed.

@jgrandja jgrandja added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 31, 2023
@wanghongzhou
Copy link
Author

@jgrandja

This is not a spring authorization BUG, this is really a javascript problem, because of this problem exists, so when Jackson serialization I want to serialize the value of type Long into a string return, I hope I can control Jackson serialization. However, Jackson's serialization is currently not configurable.

I just hope the spring - authorization can realize configuration of Jackson, rather than directly fixed use new MappingJackson2HttpMessageConverter (), if I can to configuration of Jackson, I don't need to go to the custom implementation AuthenticationSuccessHandler, it is only a suggestion, if you do not not appropriate, please ignore it. Thank you for your reply and attention to this issue. Thank you!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 4, 2023
@jgrandja
Copy link
Collaborator

jgrandja commented Nov 6, 2023

@wanghongzhou I now understand what you are looking to accomplish. Apologies that it took me long to understand.

Instead of allowing to configure Jackson serialization in order to

I want to serialize the value of type Long into a String return

You can configure this behaviour in a custom Converter<OAuth2AccessTokenResponse, Map<String, Object>> and set it via OAuth2AccessTokenResponseHttpMessageConverter.setAccessTokenResponseParametersConverter(). The default is DefaultOAuth2AccessTokenResponseMapConverter. However, the current issue is that there is no way to configure OAuth2AccessTokenResponseHttpMessageConverter because there currently is no access to it in OAuth2TokenEndpointFilter.

Please take a look at gh-1429 as we are looking to address gh-925. You will notice that the new AuthenticationSuccessHandler will expose a setter for HttpMessageConverter<OAuth2AccessTokenResponse> that will allow you to configure a custom OAuth2AccessTokenResponseHttpMessageConverter here

I'm going to close this as a duplicate. Please add any additional comments to gh-925.

@jgrandja jgrandja closed this as completed Nov 6, 2023
@jgrandja jgrandja added status: duplicate A duplicate of another issue and removed status: feedback-provided Feedback has been provided labels Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

3 participants