Fix to ensure endpoints distinguish between form and query parameters #1468
Conversation
…uthorizationCodeAuthenticationConverter.
|
@jgrandja , I just modify the OAuth2AuthorizationCodeAuthenticationConverter to use the method |
spring-projects-issues
added
the
status: waiting-for-triage
| @@ -54,6 +57,49 @@ static MultiValueMap<String, String> getParameters(HttpServletRequest request) { | |||
| return parameters; | |||
| } | |||
|
|
|||
| static MultiValueMap<String, String> getFormParameters(HttpServletRequest request) { | |||
There was a problem hiding this comment.
This method can be simplified. Given the existing getParameters() method, the only change required to make it function as getFormParameters() is:
...
parameterMap.forEach((key, values) -> {
if (!request.getQueryString().contains(key) && // If not query parameter then it's a form parameter
values.length > 0) {
for (String value : values) {
parameters.add(key, value);
}
}
});
...And for getQueryParameters(), applying if (request.getQueryString().contains(key) ... would work.
There was a problem hiding this comment.
Thanks. That's a simple way to implement that.
| @@ -355,7 +356,8 @@ private OAuth2AccessTokenResponse assertTokenRequestReturnsAccessTokenResponse(R | |||
| OAuth2Authorization authorization, String tokenEndpointUri) throws Exception { | |||
| MvcResult mvcResult = this.mvc.perform(post(tokenEndpointUri) | |||
| .params(getTokenRequestParameters(registeredClient, authorization)) | |||
| .header(HttpHeaders.AUTHORIZATION, getAuthorizationHeader(registeredClient))) | |||
| .header(HttpHeaders.AUTHORIZATION, getAuthorizationHeader(registeredClient)) | |||
| .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)) | |||
There was a problem hiding this comment.
Please remove all changes where the content type is set in the request as it's redundant. Let's keep the changes in this PR to only what's required for the fix.
There was a problem hiding this comment.
Reverted all changes for the content type changes.
jgrandja
changed the title
jgrandja
added
type: bug
No. Please include the updates to all Lastly, I noticed some formatting changes, e.g. reorganized imports. Please ensure the only code changes in this PR are related to the fix only. Thank you. |
1. Update all AuthenticationConverters with either getFormParameters or getQueryParameters method 2. Remove useless method getParameters 3. Refactor code according to PR's comments
Thanks for review the PR. I have updated the PR accordingly. |
|
Thanks for the updates @gregecho. FYI, I added a polish commit to get this merged before tomorrow's release. |
Fix gh-1451, initial implementation of get parameters from body for OAuth2AuthorizationCodeAuthenticationConverter.