Skip to content

Authorization Endpoint filter for Authorization Code flow #77

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
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,231 @@
*/
package org.springframework.security.oauth2.server.authorization.web;

import java.io.IOException;
import java.util.stream.Stream;

import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.springframework.core.convert.converter.Converter;
import org.springframework.http.HttpStatus;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.crypto.keygen.Base64StringKeyGenerator;
import org.springframework.security.crypto.keygen.StringKeyGenerator;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.OAuth2AuthorizationException;
import org.springframework.security.oauth2.core.OAuth2Error;
import org.springframework.security.oauth2.core.OAuth2ErrorCodes;
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames;
import org.springframework.security.oauth2.server.authorization.OAuth2Authorization;
import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationService;
import org.springframework.security.oauth2.server.authorization.TokenType;
import org.springframework.security.oauth2.server.authorization.client.RegisteredClient;
import org.springframework.security.oauth2.server.authorization.client.RegisteredClientRepository;
import org.springframework.security.web.DefaultRedirectStrategy;
import org.springframework.security.web.RedirectStrategy;
import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
import org.springframework.security.web.util.matcher.RequestMatcher;
import org.springframework.util.Assert;
import org.springframework.util.StringUtils;
import org.springframework.web.filter.OncePerRequestFilter;

import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import org.springframework.web.util.UriComponentsBuilder;

/**
* @author Joe Grandja
* @author Paurav Munshi
* @since 0.0.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc to be added

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 am thinking of adding Javadoc once everything is finalized

*/
public class OAuth2AuthorizationEndpointFilter extends OncePerRequestFilter {
private Converter<HttpServletRequest, OAuth2AuthorizationRequest> authorizationRequestConverter;

private static final String DEFAULT_ENDPOINT = "/oauth2/authorize";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be named DEFAULT_AUTHORIZATION_ENDPOINT_URI


private Converter<HttpServletRequest, OAuth2AuthorizationRequest> authorizationRequestConverter = new OAuth2AuthorizationRequestConverter();
private RegisteredClientRepository registeredClientRepository;
private OAuth2AuthorizationService authorizationService;
private StringKeyGenerator codeGenerator;
private StringKeyGenerator codeGenerator = new Base64StringKeyGenerator();
private RedirectStrategy authorizationRedirectStrategy = new DefaultRedirectStrategy();
private RequestMatcher authorizationEndpointMatcher = new AntPathRequestMatcher(DEFAULT_ENDPOINT);

public OAuth2AuthorizationEndpointFilter(RegisteredClientRepository registeredClientRepository,
OAuth2AuthorizationService authorizationService) {
Assert.notNull(registeredClientRepository, "registeredClientRepository cannot be null.");
Assert.notNull(authorizationService, "authorizationService cannot be null.");
this.registeredClientRepository = registeredClientRepository;
this.authorizationService = authorizationService;
}

public final void setAuthorizationRequestConverter(
Converter<HttpServletRequest, OAuth2AuthorizationRequest> authorizationRequestConverter) {
Assert.notNull(authorizationRequestConverter, "authorizationRequestConverter cannot be set to null");
this.authorizationRequestConverter = authorizationRequestConverter;
}

public final void setCodeGenerator(StringKeyGenerator codeGenerator) {
Assert.notNull(codeGenerator, "codeGenerator cannot be set to null");
this.codeGenerator = codeGenerator;
}

public final void setAuthorizationRedirectStrategy(RedirectStrategy authorizationRedirectStrategy) {
Assert.notNull(authorizationRedirectStrategy, "authorizationRedirectStrategy cannot be set to null");
this.authorizationRedirectStrategy = authorizationRedirectStrategy;
}

public final void setAuthorizationEndpointMatcher(RequestMatcher authorizationEndpointMatcher) {
Assert.notNull(authorizationEndpointMatcher, "authorizationEndpointMatcher cannot be set to null");
this.authorizationEndpointMatcher = authorizationEndpointMatcher;
}

@Override
protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
boolean pathMatch = this.authorizationEndpointMatcher.matches(request);
String responseType = request.getParameter(OAuth2ParameterNames.RESPONSE_TYPE);
Copy link
Contributor

@kratostaine kratostaine May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the spec of Error response for Authorization request, the server should respond with invalid_request if missing a required parameter or unsupported_response_type for an invalid response type. So ideally the check for response_type should be added at the place of validating the auth request as response_type is a required param according to the spec of Authorization request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was how it was. I, initially, had the request validation also perform response_type check. But @jgrandja suggested to perform this check at the time of verifying if filter is applicable or not. Please look at this comment. And I think this is also a good approach because response_type could also be token and in that case a different Filter will handle the request and I think for erroneous response_type we can have an separate error filter. I am fine with both the options, but just wanted to point out that this was kept this way deliberately.

boolean responseTypeMatch = OAuth2ParameterNames.CODE.equals(responseType);
if (pathMatch && responseTypeMatch) {
return false;
}else {
return true;
}
}

@Override
protected void doFilterInternal(HttpServletRequest request,
HttpServletResponse response, FilterChain filterChain)
throws ServletException, IOException {

RegisteredClient client = null;
OAuth2AuthorizationRequest authorizationRequest = null;
OAuth2Authorization authorization = null;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected to see the use of authorizationEndpointMatcher as the very first call here. However, it's not being used at all. Please review 4.1.1. Authorization Request. At a minimum, there should be a RequestMatcher that matches on a valid Authorization Request. A valid authorization request should at least match on the configured endpoint path (using AntPathRequestMatcher) and the required parameters response_type=code, client_id=*. This is a common pattern used throughout Spring Security, where Filter use a RequestMatcher to filter out request or process them.

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 have used the RequestMatcher in the shouldNotFilter method. That method is called by OncePerRequestFilter.doFilter method effectively calling it before doFilterInternal. I thought it is much more natural to filter this way for checking endpoint. And I also think that validity of the query parameters is not supposed to be done in RequestMatcher because it is only responsible to check if filter should be invoked for that request or not. Once invoked, I do not think RequestMatcher should validate the request and therefore I have those validations in separate methods. Let me know if it is not to be done in that way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used the RequestMatcher in the shouldNotFilter method

Sorry missed that.

I also think that validity of the query parameters is not supposed to be done in RequestMatcher

A valid Authorization Request requires response_type=code and client_id != empty so both of these need to be applied in RequestMatcher

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A valid Authorization Request requires response_type=code and client_id != empty so both of these need to be applied in RequestMatcher

So here is where my confusion lies. RequestMatcher by its nature, it seems, is supposed to decide whether request is supported by that filter or not. So if the RequestMatcher does not find a match it will silently allow the request to proceed to FilterChain. But if we look at first line of 4.1.2.1 Error Response, it says :-

If the request fails due to a missing, invalid, or mismatching
redirection URI, or if the client identifier is missing or invalid,
the authorization server SHOULD inform the resource owner of the
error

So if we are putting response_type=code and client_id != empty in RequestMatcher it will not send the error response back to resource owner's user-agent. So how are supposed to handle this scenario if we want to perform this validation in RequestMatcher. I think that in future if Implicit grant is going to be implemented using a different filter then we can add response_type=code in RequestMatcher but I would still not think of validating client id in ReqeustMatcher.

Can you please provide some more light here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a minimum I have added the check for response_type in RequestMatcher. So if the response_type is blank or if its not code then this filter will not be invoked and filter chain will proceeded with. Tests have been added to check the same.

Can you please review the PR ?

try {
checkUserAuthenticated();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the RequestMatcher determines to process the request, it should delegate to authorizationRequestConverter to convert to OAuth2AuthorizationRequest. Then the OAuth2AuthorizationRequest should be used to perform the required validations as the next step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment below. There I have tried to explain why I have used request instead of OAuth2AuthorizationRequest.

Authentication auth = SecurityContextHolder.getContext().getAuthentication();
client = fetchRegisteredClient(request);

authorizationRequest = this.authorizationRequestConverter.convert(request);
validateAuthorizationRequest(authorizationRequest, client);

String code = this.codeGenerator.generateKey();
authorization = buildOAuth2Authorization(auth, client, authorizationRequest, code);
this.authorizationService.save(authorization);

String redirectUri = getRedirectUri(authorizationRequest, client);
sendCodeOnSuccess(request, response, authorizationRequest, redirectUri, code);
}
catch(OAuth2AuthorizationException authorizationException) {
OAuth2Error authorizationError = authorizationException.getError();

if (authorizationError.getErrorCode().equals(OAuth2ErrorCodes.INVALID_REQUEST)
|| authorizationError.getErrorCode().equals(OAuth2ErrorCodes.ACCESS_DENIED)) {
sendErrorInResponse(response, authorizationError);
}
else if (authorizationError.getErrorCode().equals(OAuth2ErrorCodes.UNSUPPORTED_RESPONSE_TYPE)
|| authorizationError.getErrorCode().equals(OAuth2ErrorCodes.UNAUTHORIZED_CLIENT)) {
String redirectUri = getRedirectUri(authorizationRequest, client);
sendErrorInRedirect(request, response, authorizationRequest, authorizationError, redirectUri);
}
else {
throw new ServletException(authorizationException);
}
}

}

private void checkUserAuthenticated() {
Authentication currentAuth = SecurityContextHolder.getContext().getAuthentication();
if (currentAuth==null || !currentAuth.isAuthenticated()) {
throw new OAuth2AuthorizationException(new OAuth2Error(OAuth2ErrorCodes.ACCESS_DENIED));
}
}

private RegisteredClient fetchRegisteredClient(HttpServletRequest request) throws OAuth2AuthorizationException {
String clientId = request.getParameter(OAuth2ParameterNames.CLIENT_ID);
if (StringUtils.isEmpty(clientId)) {
throw new OAuth2AuthorizationException(new OAuth2Error(OAuth2ErrorCodes.INVALID_REQUEST));
}

RegisteredClient client = this.registeredClientRepository.findByClientId(clientId);
if (client==null) {
throw new OAuth2AuthorizationException(new OAuth2Error(OAuth2ErrorCodes.ACCESS_DENIED));
}

boolean isAuthorizationGrantAllowed = Stream.of(client.getAuthorizationGrantTypes())
.anyMatch(grantType -> grantType.contains(AuthorizationGrantType.AUTHORIZATION_CODE));
if (!isAuthorizationGrantAllowed) {
throw new OAuth2AuthorizationException(new OAuth2Error(OAuth2ErrorCodes.ACCESS_DENIED));
}

return client;

}

private OAuth2Authorization buildOAuth2Authorization(Authentication auth, RegisteredClient client,
OAuth2AuthorizationRequest authorizationRequest, String code) {
OAuth2Authorization authorization = OAuth2Authorization.withRegisteredClient(client)
.principalName(auth.getPrincipal().toString())
.attribute(TokenType.AUTHORIZATION_CODE.getValue(), code)
.attributes(attirbutesMap -> attirbutesMap.putAll(authorizationRequest.getAttributes()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo in attirbutesMap

.build();

return authorization;
}


private void validateAuthorizationRequest(OAuth2AuthorizationRequest authorizationRequest, RegisteredClient client) {
String redirectUri = authorizationRequest.getRedirectUri();
if (StringUtils.isEmpty(redirectUri) && client.getRedirectUris().size() > 1) {
throw new OAuth2AuthorizationException(new OAuth2Error(OAuth2ErrorCodes.INVALID_REQUEST));
}
if (!StringUtils.isEmpty(redirectUri) && !client.getRedirectUris().contains(redirectUri)) {
throw new OAuth2AuthorizationException(new OAuth2Error(OAuth2ErrorCodes.INVALID_REQUEST));
}
}

private String getRedirectUri(OAuth2AuthorizationRequest authorizationRequest, RegisteredClient client) {
return !StringUtils.isEmpty(authorizationRequest.getRedirectUri())
? authorizationRequest.getRedirectUri()
: client.getRedirectUris().stream().findFirst().get();
}

private void sendCodeOnSuccess(HttpServletRequest request, HttpServletResponse response,
OAuth2AuthorizationRequest authorizationRequest, String redirectUri, String code) throws IOException {
UriComponentsBuilder redirectUriBuilder = UriComponentsBuilder.fromUriString(redirectUri)
.queryParam(OAuth2ParameterNames.CODE, code);
if (!StringUtils.isEmpty(authorizationRequest.getState())) {
redirectUriBuilder.queryParam(OAuth2ParameterNames.STATE, authorizationRequest.getState());
}

String finalRedirectUri = redirectUriBuilder.toUriString();
this.authorizationRedirectStrategy.sendRedirect(request, response, finalRedirectUri);
}

private void sendErrorInResponse(HttpServletResponse response, OAuth2Error authorizationError) throws IOException {
int errorStatus = -1;
String errorCode = authorizationError.getErrorCode();
if (errorCode.equals(OAuth2ErrorCodes.ACCESS_DENIED)) {
errorStatus=HttpStatus.FORBIDDEN.value();
}
else {
errorStatus=HttpStatus.INTERNAL_SERVER_ERROR.value();
}
response.sendError(errorStatus, authorizationError.getErrorCode());
}

private void sendErrorInRedirect(HttpServletRequest request, HttpServletResponse response,
OAuth2AuthorizationRequest authorizationRequest, OAuth2Error authorizationError,
String redirectUri) throws IOException {
UriComponentsBuilder redirectUriBuilder = UriComponentsBuilder.fromUriString(redirectUri)
.queryParam(OAuth2ParameterNames.ERROR, authorizationError.getErrorCode());

if (!StringUtils.isEmpty(authorizationRequest.getState())) {
redirectUriBuilder.queryParam(OAuth2ParameterNames.STATE, authorizationRequest.getState());
}

String finalRedirectURI = redirectUriBuilder.toUriString();
this.authorizationRedirectStrategy.sendRedirect(request, response, finalRedirectURI);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright 2020 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.springframework.security.oauth2.server.authorization.web;

import java.util.Arrays;
import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.Set;

import javax.servlet.http.HttpServletRequest;

import org.springframework.core.convert.converter.Converter;
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames;
import org.springframework.util.StringUtils;

/**
* @author Paurav Munshi
* @since 0.0.1
* @see Converter
*/
public class OAuth2AuthorizationRequestConverter implements Converter<HttpServletRequest, OAuth2AuthorizationRequest> {

@Override
public OAuth2AuthorizationRequest convert(HttpServletRequest request) {
String scope = request.getParameter(OAuth2ParameterNames.SCOPE);
Set<String> scopes = !StringUtils.isEmpty(scope)
? new LinkedHashSet<String>(Arrays.asList(scope.split(" ")))
: Collections.emptySet();

OAuth2AuthorizationRequest authorizationRequest = OAuth2AuthorizationRequest.authorizationCode()
.clientId(request.getParameter(OAuth2ParameterNames.CLIENT_ID))
.redirectUri(request.getParameter(OAuth2ParameterNames.REDIRECT_URI))
.scopes(scopes)
.state(request.getParameter(OAuth2ParameterNames.STATE))
.authorizationUri(request.getServletPath())
.build();

return authorizationRequest;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,40 @@ public static RegisteredClient.Builder registeredClient2() {
.scope("profile")
.scope("email");
}

public static RegisteredClient.Builder validAuthorizationGrantRegisteredClient() {
return RegisteredClient.withId("valid_client_id")
.clientId("valid_client")
.clientSecret("valid_secret")
.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
.clientAuthenticationMethod(ClientAuthenticationMethod.BASIC)
.redirectUri("http://localhost:8080/test-application/callback")
.scope("openid")
.scope("profile")
.scope("email");
}

public static RegisteredClient.Builder validAuthorizationGrantClientMultiRedirectUris() {
return RegisteredClient.withId("valid_client_multi_uri_id")
.clientId("valid_client_multi_uri")
.clientSecret("valid_secret")
.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
.clientAuthenticationMethod(ClientAuthenticationMethod.BASIC)
.redirectUri("http://localhost:8080/test-application/callback")
.redirectUri("http://localhost:8080/another-test-application/callback")
.scope("openid")
.scope("profile")
.scope("email");
}

public static RegisteredClient.Builder validClientCredentialsGrantRegisteredClient() {
return RegisteredClient.withId("valid_cc_client_id")
.clientId("valid_cc_client")
.clientSecret("valid_secret")
.authorizationGrantType(AuthorizationGrantType.CLIENT_CREDENTIALS)
.clientAuthenticationMethod(ClientAuthenticationMethod.BASIC)
.scope("openid")
.scope("profile")
.scope("email");
}
}
Loading