Skip to content

Support POST for authorization code request flow #1874

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.

Sign up for GitHub

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
wants to merge 4 commits into from
Closed

Support POST for authorization code request flow #1874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1df3e76
Remove the openid scope matcher in OAuth2AuthorizationEndpointFilter
sylvain-costanzo Jan 13, 2025
40c8b22
Remove the openid scope matcher in OAuth2AuthorizationCodeRequestAuth…
sylvain-costanzo Jan 17, 2025
a8d0772
Disctinct Authorize matcher and Consent matcher in OAuth2Authorizatio…
sylvain-costanzo Feb 3, 2025
bc6b470
Disctinct Authorize matcher and Consent matcher in OAuth2Authorizatio…
sylvain-costanzo Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions ...framework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import org.springframework.security.oauth2.core.OAuth2Error;
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationResponse;
import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames;
import org.springframework.security.oauth2.core.oidc.OidcScopes;
import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationException;
import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationProvider;
import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationToken;
Expand Down Expand Up @@ -151,16 +150,12 @@ private static RequestMatcher createDefaultRequestMatcher(String authorizationEn
HttpMethod.GET.name());
RequestMatcher authorizationRequestPostMatcher = new AntPathRequestMatcher(authorizationEndpointUri,
HttpMethod.POST.name());
RequestMatcher openidScopeMatcher = (request) -> {
String scope = request.getParameter(OAuth2ParameterNames.SCOPE);
return StringUtils.hasText(scope) && scope.contains(OidcScopes.OPENID);
};

RequestMatcher responseTypeParameterMatcher = (
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

request) -> request.getParameter(OAuth2ParameterNames.RESPONSE_TYPE) != null;

RequestMatcher authorizationRequestMatcher = new OrRequestMatcher(authorizationRequestGetMatcher,
new AndRequestMatcher(authorizationRequestPostMatcher, responseTypeParameterMatcher,
openidScopeMatcher));
new AndRequestMatcher(authorizationRequestPostMatcher, responseTypeParameterMatcher));
RequestMatcher authorizationConsentMatcher = new AndRequestMatcher(authorizationRequestPostMatcher,
new NegatedRequestMatcher(responseTypeParameterMatcher));

Expand Down
12 changes: 4 additions & 8 deletions ...thorization/web/authentication/OAuth2AuthorizationCodeRequestAuthenticationConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationConverter impleme
private static final Authentication ANONYMOUS_AUTHENTICATION = new AnonymousAuthenticationToken("anonymous",
"anonymousUser", AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS"));

private static final RequestMatcher OIDC_REQUEST_MATCHER = createOidcRequestMatcher();
private static final RequestMatcher POST_WITH_RESPONSE_TYPE_REQUEST_MATCHER = createPostWithResponseTypeRequestMatcher();

@Override
public Authentication convert(HttpServletRequest request) {
if (!"GET".equals(request.getMethod()) && !OIDC_REQUEST_MATCHER.matches(request)) {
if (!"GET".equals(request.getMethod()) && !POST_WITH_RESPONSE_TYPE_REQUEST_MATCHER.matches(request)) {
return null;
}

Expand Down Expand Up @@ -153,15 +153,11 @@ else if (!responseType.equals(OAuth2AuthorizationResponseType.CODE.getValue()))
state, scopes, additionalParameters);
}

private static RequestMatcher createOidcRequestMatcher() {
private static RequestMatcher createPostWithResponseTypeRequestMatcher() {
RequestMatcher postMethodMatcher = (request) -> "POST".equals(request.getMethod());
RequestMatcher responseTypeParameterMatcher = (
request) -> request.getParameter(OAuth2ParameterNames.RESPONSE_TYPE) != null;
RequestMatcher openidScopeMatcher = (request) -> {
String scope = request.getParameter(OAuth2ParameterNames.SCOPE);
return StringUtils.hasText(scope) && scope.contains(OidcScopes.OPENID);
};
return new AndRequestMatcher(postMethodMatcher, responseTypeParameterMatcher, openidScopeMatcher);
return new AndRequestMatcher(postMethodMatcher, responseTypeParameterMatcher);
}

private static void throwError(String errorCode, String parameterName) {
Expand Down
8 changes: 2 additions & 6 deletions ...work/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,19 +611,15 @@ public void doFilterWhenAuthorizationRequestAuthenticatedThenAuthorizationRespon

@Test
public void doFilterWhenAuthenticationRequestAuthenticatedThenAuthorizationResponse() throws Exception {
// Setup OpenID Connect request
RegisteredClient registeredClient = TestRegisteredClients.registeredClient().scopes((scopes) -> {
scopes.clear();
scopes.add(OidcScopes.OPENID);
}).build();
RegisteredClient registeredClient = TestRegisteredClients.registeredClient().scopes(Set::clear).build();
OAuth2AuthorizationCodeRequestAuthenticationToken authorizationCodeRequestAuthenticationResult = new OAuth2AuthorizationCodeRequestAuthenticationToken(
AUTHORIZATION_URI, registeredClient.getClientId(), this.principal, this.authorizationCode,
registeredClient.getRedirectUris().iterator().next(), STATE, registeredClient.getScopes());
authorizationCodeRequestAuthenticationResult.setAuthenticated(true);
given(this.authenticationManager.authenticate(any())).willReturn(authorizationCodeRequestAuthenticationResult);

MockHttpServletRequest request = createAuthorizationRequest(registeredClient);
request.setMethod("POST"); // OpenID Connect supports POST method
request.setMethod("POST");
request.setQueryString(null);
MockHttpServletResponse response = new MockHttpServletResponse();
FilterChain filterChain = mock(FilterChain.class);
Expand Down