-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support POST for authorization code request flow #1874
base: main
Are you sure you want to change the base?
Support POST for authorization code request flow #1874
Conversation
The removal of the oidc scope matcher now allow OAuth2AuthorizationEndpointFilter to send POST /authorize request without the openid scope to the converters. But I see two issues with this development: 1- The default converter Tackling this issue seems a bit out of scope for this PR so I didn't touch anything, and it can be surely be solved with some custom converters. 2- Another minor issue I see is that the oauth2 specification is saying that a /authorize request (GET or POST) is supposed to have the parameters in the query string,
contradicting the OIDC spec saying that POST requests must use the body
So this way of getting the request parameters may have to be updated, but again it can be solved with a custom converter, so it also felt out of scope of this PR. Considering these 2 issues, I am not sure the PR is answering the "Support POST for authorization code request flow", and I would like your opinion on this |
Rewording my thoughts, I don't think these issues are completely out of scope, but they need significant refactor with more regression risks than the small change here. |
String scope = request.getParameter(OAuth2ParameterNames.SCOPE); | ||
return StringUtils.hasText(scope) && scope.contains(OidcScopes.OPENID); | ||
}; | ||
RequestMatcher responseTypeParameterMatcher = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responseTypeParameterMatcher
should not be removed as it will break the Authorization Consent flow. Only remove openidScopeMatcher
here as well as in OAuth2AuthorizationCodeRequestAuthenticationConverter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In OAuth2AuthorizationEndpointFilter
, removing the openid scope and letting the response_type would result of a matcher like this:
(GET or (POST and response_type)) or (POST and !response_type)
which is the same as (GET or POST) that's why I removed the response_type matcher.
I can add it back if you prefer it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the openid scope matcher from the converter OAuth2AuthorizationCodeRequestAuthenticationConverter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would result of a matcher like this:
(GET or (POST and response_type)) or (POST and !response_type)
I believe we still need the response_type
parameter in the matching because it's the difference between an Authorization Request or Authorization Consent Request.
However, I think we need to adjust the matching as follows:
Authorization Request
(GET or POST) and response_type
Authorization Consent Request
POST and !response_type
This should be applied to OAuth2AuthorizationEndpointFilter
and both OAuth2AuthorizationCodeRequestAuthenticationConverter
and OAuth2AuthorizationConsentAuthenticationConverter
to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the response_type
matcher to the GET request in OAuth2AuthorizationEndpointFilter
will make the server return a 404 if a GET /authorize request is performed without the required response_type
parameter, instead of a 400.
Specifically it is breaking the test OAuth2AuthorizationEndpointFilterTests.doFilterWhenAuthorizationRequestMissingResponseTypeThenInvalidRequestError()
so I don't think adding this matcher in OAuth2AuthorizationEndpointFilter
is a good solution.
For the same reason, adding these matcher rules to OAuth2AuthorizationCodeRequestAuthenticationConverter
will make the request not enter the converter and prevent the server to return a 400 (it is breaking the same test).
This highlight the fact that a POST /authorize request without the response_type
parameter will be treated as a consent request, so maybe we could add a condition in OAuth2AuthorizationConsentAuthenticationConverter
to check if the user is authenticated?
A way to solve all this could be to create a new converter after OAuth2AuthorizationCodeRequestAuthenticationConverter
and OAuth2AuthorizationConsentAuthenticationConverter
to catch this missing parameter, so we can use the matcher in the converters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the matching rules stated in the previous comment need to be adjusted as follows:
Authorization Request
GET or (POST and response_type)
Authorization Consent Request
POST and !response_type
I tried this on my end and all tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a distinction between the Authorize matcher and the Consent matcher in OAuth2AuthorizationEndpointFilter
.
A previous commit updated OAuth2AuthorizationCodeRequestAuthenticationConverter
.
Nothing to do for OAuth2AuthorizationConsentAuthenticationConverter
It should be fine now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sylvain-costanzo I will get to this next week. I'm trying to wrap up a priority task by end of this week.
The Spring team has recently migrated to the Developer Certificate of Origin (DCO) for our contribution process. See Submitting Pull Requests for additional details on the new process. Please format the commits in this PR as the DCO check did not pass. |
Closes spring-projectsgh-1811 Signed-off-by: sylvain-costanzo <[email protected]>
…enticationConverter Closes spring-projectsgh-1811 Signed-off-by: sylvain-costanzo <[email protected]>
99b5960
to
40c8b22
Compare
…nEndpointFilter Closes spring-projectsgh-1811 Signed-off-by: sylvain-costanzo <[email protected]>
…nEndpointFilter Closes spring-projectsgh-1811 Signed-off-by: sylvain-costanzo <[email protected]>
Closes gh-1811