Gh 5760 #5777
Gh 5760 #5777
Conversation
- Check for query parameters in the authorization uri and pass them as additional parameters to the Oauth2Authorization request builder
- fix static import
| @@ -96,6 +96,12 @@ private OAuth2AuthorizationRequest resolve(HttpServletRequest request, String re | |||
| if (registrationId == null) { | |||
| return null; | |||
| } | |||
| String[] params = new String[0]; | |||
There was a problem hiding this comment.
Parsing URLs is hard. Any fixes should avoid manually parsing the URL.
| String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId()+"?"+queryParams; | ||
|
|
||
| MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); | ||
| request.setServletPath(requestUri); |
There was a problem hiding this comment.
I think this is part of the reason the manual parsing is necessary. The Javadoc of servletPath states:
but does not include any extra path information or a query string
This means that the value that is being set here is invalid. It should not include the query string.
There was a problem hiding this comment.
Thank you for this, you're right it's an invalid URL. I will try to fix this as soon as possible and resubmit the PR
- removes hard coded url parsing and fetches the query string from the request
- remove non needed unit test
|
@rwinch I updated the PR based on your comments. Let me know if you have any more feedback :) Thank you |
| } | ||
| for (String param : params){ | ||
| int idx = param.indexOf("="); | ||
| additionalParameters.put(param.substring(0, idx), param.substring(idx + 1)); |
There was a problem hiding this comment.
We should not propagate all query parameters (coming into the app) into the Authorization Request. Some of those query parameters may not be applicable to the Provider. Furthermore, the Provider may reject the Authorization Request if it receives parameters that it does not understand OR it may choose to ignore it. This is really dependent on how the Provider is implemented.
Either way, the user should be in control on what additional parameters should be sent in the Authorization Request. With this change, the user does not have that control.
There was a problem hiding this comment.
The goal of #5760 is to allow query parameter(s) to be configured in the authorization-uri property.
For example:
authorization-uri: https://accounts.google.com/o/oauth2/v2/auth?access_type=offline
However, the recent updates in this PR don't fulfill this requirement.
Are you planning on further updates?
There was a problem hiding this comment.
@mlevkovsky Thanks for the updates. I second @jgrandja comments. I also added an additional comment
There was a problem hiding this comment.
I second what @jgrandja is saying here.
In addition, a more general comment is that we do not want to be parsing URLs. Instead something like UriComponentsBuilder should be used.
There was a problem hiding this comment.
@mlevkovsky I forgot to mention - the changes I was expecting to see should be in OAuth2AuthorizationRequest.buildAuthorizationRequestUri() instead of DefaultOAuth2AuthorizationRequestResolver.
@rwinch I agree we should not be parsing URL's and instead reuse UriComponentsBuilder for example. The only issue I'm seeing with leveraging UriComponentsBuilder is that it lives in spring-web which would mean we would need to add that dependency to oauth2-core, since OAuth2AuthorizationRequest lives there. What are your thoughts on this?
There was a problem hiding this comment.
I think that is fine to have a dependency on spring-web given OAuth assumes HTTP as a transport. This is especially true of a dependency that is encapsulated.
There was a problem hiding this comment.
yes sorry for the delays. I'll provide an update as soon as possible
@jgrandja I see your point. I think it's the user's responsibility to provide the proper query params for the appropriate provider.
IMO what the user configures is what we should add to the request uri
| } | ||
| for (String param : params){ | ||
| int idx = param.indexOf("="); | ||
| additionalParameters.put(param.substring(0, idx), param.substring(idx + 1)); |
There was a problem hiding this comment.
I second what @jgrandja is saying here.
In addition, a more general comment is that we do not want to be parsing URLs. Instead something like UriComponentsBuilder should be used.
|
@mlevkovsky I'm going to close this PR in favour of #6299 as we would like to get this improvement in for the next release. |
Related #5760
This PR will enable users to add query parameters to their authorization URI thru their application.yaml (or.xml)