Authorization Endpoint filter for Authorization Code flow #77

paurav-munshi · 2020-05-04T22:36:34Z

Fixes #66

Functionality Covered

Validation of client id & response type
Generate authorization code
Save Authorization
Redirect to client redirect uri
Send error in http response
Send error as query params in redirect uri

Validations covered

Client Id should be mandatory
Client Id should be registered
Client Id should be configured with Authorization grant type
Response type is mandaotry
Response type should be code
Redirect Uri should be specified if client has multiple redirect uris configured
If supplied, redirect uri should be same as the one configured on client
User should be authenticated
Endpoint should be /oauth2/authorize

kratostaine · 2020-05-06T17:39:43Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+
+	@Override
+	protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
+		return !authorizationEndpiontMatcher.matches(request);


Minor typo in authorizationEndpiontMatcher

Fixed. Thanks.

kratostaine · 2020-05-06T18:05:33Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+		if (client==null)
+			throw new OAuth2AuthorizationException(CLIENT_ID_NOT_FOUND_ERROR);
+
+		boolean isAuthoirzationGrantAllowed = Stream.of(client.getAuthorizationGrantTypes())


Minor typo in isAuthoirzationGrantAllowed

Fixed. Thanks.

pgerhard · 2020-05-06T22:22:02Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+		if (errorCode.equals(OAuth2ErrorCodes.ACCESS_DENIED))
+			errorStatus=HttpStatus.FORBIDDEN.value();
+		else errorStatus=HttpStatus.INTERNAL_SERVER_ERROR.value();
+		response.sendError(errorStatus, authorizationError.getErrorCode()+":"+authorizationError.getDescription());


I find not using curly braces in "if" statements to be very bug prone. I'd suggest introducing them here and in other other locations in this PR. I've noticed this in numerous locations within this PR

@jgrandja do you have any suggestions on which coding guidelines we should follow? I found this document in the spring-framework repository. But it does say its still a work-in-progress

@pgerhard Take a look at the Getting Started, specifically:

Submitted work via pull requests should follow the same coding style/conventions and adopt the same or similar design patterns that have been established in Spring Security’s OAuth 2.0 support

Essentially, we are looking for the same coding style/patterns that are implemented in the OAuth 2.0 codebase in Spring Security 5.x. Consistency is key, so the Authorization Server code should align as well.

jgrandja

Thanks for the updates @paurav-munshi. Please see my review comments.

Also, I noticed quite a few issues with code formatting and style, eg. missing curly braces in if blocks, missing space between if and (, etc. Again, please review Getting Started, as it's very imprortant that the code style/conventions are adopted from what exists today in Spring Security 5 OAuth 2.0 support classes.

jgrandja · 2020-05-07T13:54:31Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+
+	private static final String DEFAULT_ENDPOINT = "/oauth2/authorize";
+
+	private static final OAuth2Error CLIENT_ID_ABSENT_ERROR = new OAuth2Error(OAuth2ErrorCodes.INVALID_REQUEST, OAuth2AuthorizationServerMessages.REQUEST_MISSING_CLIENT_ID, null);


Please remove these constants as well as OAuth2AuthorizationServerMessages. We only need to set OAuth2Error.errorCode at this point - messages/localization will be implemented at a much later point.

As well, let's create OAuth2Error at the point in code where it's required instead of reusing the constant instances. We can optimize object creation later, if need be.

Fixed in PR. Thanks.

jgrandja · 2020-05-07T13:56:09Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+
+
+	public OAuth2AuthorizationEndpointFilter() {
+		authorizationEndpiontMatcher = new AntPathRequestMatcher(DEFAULT_ENDPOINT);


Class members should be prefixed with this

Fixed in PR. Thanks.

jgrandja · 2020-05-07T14:08:21Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

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


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.

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.

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

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 ?

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 ?

jgrandja · 2020-05-07T14:16:35Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+		OAuth2Authorization authorization = null;
+
+		try {
+			checkUserAuthenticated();


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.

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

jgrandja · 2020-05-07T14:19:33Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

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


Why not pass authorizationRequest here instead of request?

Actually, when we build authorizationRequest, it automatically populates authorization code when creating the Builder using the OAuth2AuthorizationRequest.authorizationCode method. So even if we get blank response_type from request, authorizationRequest would still have response type as code. So instead of doing one validation on request and other validations on authorizationRequest, it felt more obvious to do all validations on request.

I had to separate out client id validation to be done in fetchRegisteredClient because other validation like redirect_uri validations or scope validations(future story) are dependent on registered client configuration. Otherwise I earlier planned to also perform client id validation in validateAuthorizationRequest method. Infact I was just thinking, I can move

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)); }

from fetchRegisteredClient to validateAuthorizationRequest method so that there is only one universal validation method.

Hope if makes sense. Please suggest if you think there is a better approach.

So even if we get blank response_type from request

As mentioned in other comment, a valid Authorization Request requires response_type=code so this should be applied in RequestMatcher. When OAuth2AuthorizationRequest.authorizationCode() is called, we know that response_type=code (based on match) or else authorizationRequestConverter would never be called.

Modified the validation method to validate on OAuth2AuthorizationRequest instead of request. Please review. Thanks.

jgrandja · 2020-05-07T14:21:58Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+		OAuth2Authorization authorization = OAuth2Authorization.createBuilder()
+					.clientId(authorizationRequest.getClientId())
+					.addAttribute(OAuth2ParameterNames.CODE, code)
+					.addAttribute(OAuth2Authorization.ISSUED_AT, Instant.now())


Please remove ISSUED_AT, CODE_USED and SCOPE. These will be added in a later story. We only need to store CODE and OAuth2AuthorizationRequest attributes as part of this story.

Fixed in PR. Thanks.

jgrandja · 2020-05-07T14:25:13Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+
+	}
+
+	protected OAuth2Authorization buildOAuth2Authorization(RegisteredClient client,


We typically encapsulate in the initial design and only expose when needed. Please change to private for all exposed methods.

Fixed in PR. Thanks.

jgrandja · 2020-05-07T14:35:04Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+		this.authorizationRequestConverter = authorizationRequestConverter;
+	}
+
+	public RegisteredClientRepository getRegisteredClientRepository() {


Please remove getters. The convention is to provide final setters for optional attributes and required attributes are provided via constructors. See OAuth2AuthorizationRequestRedirectFilter and DefaultOAuth2AuthorizedClientManager for example

Fixed in PR. Thanks.

jgrandja · 2020-05-07T14:39:48Z

...springframework/security/oauth2/server/authorization/util/AuthorizationCodeKeyGenerator.java

+ * @author Paurav Munshi
+ * @since 0.0.1
+ */
+public class AuthorizationCodeKeyGenerator implements StringKeyGenerator {


Please remove this and instead leverage the same that is used in DefaultOAuth2AuthorizationRequestResolver.stateGenerator

That state generator uses Base64StringKeyGenerator internally. Earlier I looked at that option but because Base64StringKeyGenerator uses SecureRandom internally I thought its better to use something which explicitly guarantees of uniqueness. Therefore I used a generator based on UUID. But it seems UUID also uses SecureRandom internally for type 4 UUID. So I would change my code to use Base64StringKeyGenerator.

Fixed in PR. Thanks.

jgrandja · 2020-05-07T14:43:02Z

...ramework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTest.java

+	}
+
+	@Test
+	public void testFilterRedirectsWithCodeOnValidReq() throws Exception {


There is a naming convention for test methods. For an example, please see DefaultOAuth2AuthorizationRequestResolverTests and OAuth2AuthorizationCodeGrantFilterTests

Fixed in PR. Thanks.

paurav-munshi · 2020-05-11T16:14:33Z

Thanks for the updates @paurav-munshi. Please see my review comments.

Also, I noticed quite a few issues with code formatting and style, eg. missing curly braces in if blocks, missing space between if and (, etc. Again, please review Getting Started, as it's very imprortant that the code style/conventions are adopted from what exists today in Spring Security 5 OAuth 2.0 support classes.

I have not found a definitive guide for code formatting but I mainly followed this document and this document. And I looked on other existing classes from Spring Security OAuth Core for validating my understanding of code formatting. I have applied following changes which are now incorporated in PR :-

Line width of 120 characters with few exceptions
Method sequence to be constructors, setters, over-ridden methods and then private methods
Add braces to if conditions with single statement
Moved else and catch to the next line of closing brace of preview try / if block
this keyword for accessing instance members
Constructors for mandatory members not initiated by default
final setters for optional or default initiated members
Assert non null for constructor and setter arguments
Renamed test methods to methodName-When-Then format

Do let me know if you find more discrepancies. Thanks.

kratostaine · 2020-05-16T06:36:52Z

@paurav-munshi I don't think you should follow this particular code formatting:

Moved else and catch to the next line of closing brace of preview try / if block

Reference files: OAuth2AuthorizationRequestRedirectFilter.java and HttpSessionOAuth2AuthorizationRequestRepository.java
The else/else if block and catch block should be on the same line of the closing brace of previous try/if block.

paurav-munshi · 2020-05-18T16:24:18Z

@kratostaine

@paurav-munshi I don't think you should follow this particular code formatting:

Moved else and catch to the next line of closing brace of preview try / if block

Reference files: OAuth2AuthorizationRequestRedirectFilter.java and HttpSessionOAuth2AuthorizationRequestRepository.java
The else/else if block and catch block should be on the same line of the closing brace of previous try/if block.

Actually I saw class OncePerRequestFilter from which I followed the styling and also I came across link https://github.com/spring-projects/spring-framework/wiki/Code-Style which states

Line break after the closing brace if that brace terminates a statement or the body of a method, constructor, or named class

Line break before else, catch and finally statements

Functionality Covered ------------------------- - Validation of client id & response type - Generate authorization code - Save Authorization - Redirect to client redirect uri - Send error in http response - Send error as query params in redirect uri Validations covered ------------------- - Client Id should be mandatory - Client Id should be registered - Client Id should be configured with Authorization grant type - Response type is mandaotry - Response type should be code

- On successfull redirect code was not being sent in the Redirect URL - validations methods were private, so made them protected

- Adding key constants for issues time and code used flag in OAuth2Authorization - Changing error code when Authorization grant is not supported by client - Change in error method for Unauthorized client - Corrected the validation for AuthorizationGrantType check - Adding attributes Issue time and Code use flag in attributes map - Adding state to redirect uri for both success and failure - Changing method name from get to set as it was wrongly used - Adding a basic test class which will be enhanced more

- Added a UUID based Authorization Code generator - Added an Ant Path request matcher to check endpoint validity - Added a new OAuth2 authorization request converter - Use DefaultRedirectStrategy from Spring - Added a check for user authentication present and authenticated - Added check for respone type parameter validity in request - Added check for redirect uri validity in request - Replaced StringBuilder with UriComponentsBuilder - Added JUnit tests for validation in section 4.1.1 of the RFC specs - Added a class which will be responsible for holding sever level messages as constants

- Added space after commans where ever applicable - Removed trailing whilte spaces - Added white space after if condition - Added headers before package declaration

- Removed Constants from the filter class - Remove ServerMessages file - Removed AuthorizationCodeKeyGenerator instead used Base64StringKeyGenerator - Removed getter methods - Only provide setters for default and options fields - Added mandatory fields to ctor - Add braces to single statement if conditions - Adopted method declaration seq to be ctor, setter, overridden methods and private methods - Used this keyword where ever access instance members - Added tests for ctor and setters - Renamed tests to methodName-When-Then format

- Removed response type checks from validation method and added to RequestMatcher - Changed the validation method to accept OAuth2AuthorizationRequest instead of HttpServletRequest - Changed the response_type tests to expect proceeding with filter chain instead of getting an error

- Updated the api to build OAuth2Authorization in endpoint filter - Changed the api to create the builder for OAuth2Authorization - Changed the api to add attributes to OAuth2Authorization - Changed the key of attribute with which authorization code was added - Set up test user to be returned by mock object

kratostaine · 2020-05-18T18:22:36Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java


 /**
 * @author Joe Grandja
+ * @author Paurav Munshi
+ * @since 0.0.1


Javadoc to be added

I am thinking of adding Javadoc once everything is finalized

kratostaine · 2020-05-18T18:25:11Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

 */
 public class OAuth2AuthorizationEndpointFilter extends OncePerRequestFilter {
-	private Converter<HttpServletRequest, OAuth2AuthorizationRequest> authorizationRequestConverter;
+
+	private static final String DEFAULT_ENDPOINT = "/oauth2/authorize";


This should probably be named DEFAULT_AUTHORIZATION_ENDPOINT_URI

kratostaine · 2020-05-18T18:31:49Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

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


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.

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.

kratostaine · 2020-05-18T18:51:01Z

...ingframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java

+		OAuth2Authorization authorization = OAuth2Authorization.withRegisteredClient(client)
+					.principalName(auth.getPrincipal().toString())
+					.attribute(TokenType.AUTHORIZATION_CODE.getValue(), code)
+					.attributes(attirbutesMap -> attirbutesMap.putAll(authorizationRequest.getAttributes()))


Minor typo in attirbutesMap

jgrandja · 2020-05-21T10:45:53Z

Thanks for all the updates @paurav-munshi.

I decided to add a polish commit in order to get this merged, as I'm targeting to complete the first iteration of authorization_code grant later next week. The implementation had some missing/invalid validation checks as per spec. As well, I didn't feel the implementation had the same "look-and-feel" as the OAuth 2.0 support in Spring Security 5.x classes, eg. coding style/patterns/conventions.

Please take a look at the changes applied in the polish commit and let me know if you have any questions.

I would also recommend doing a comparison of OAuth2AuthorizationEndpointFilter against OAuth2AuthorizationRequestRedirectFilter and OAuth2AuthorizationCodeGrantFilter to better understand the implementation we are looking for. We are really looking to align this codebase with the OAuth 2.0 codebase in Spring Security 5.x.

Issue gh-77

Issue spring-projectsgh-77

paurav-munshi mentioned this pull request May 4, 2020

Implement Authorization Endpoint #66

Closed

kratostaine reviewed May 6, 2020

View reviewed changes

pgerhard reviewed May 6, 2020

View reviewed changes

jgrandja requested changes May 7, 2020

View reviewed changes

paurav-munshi marked this pull request as ready for review May 18, 2020 15:39

paurav-munshi added 9 commits May 18, 2020 13:29

Enhancements in Authorization Code Endpoint Filter

8d56851

- On successfull redirect code was not being sent in the Redirect URL - validations methods were private, so made them protected

Resolved checkstyle issues

29defd4

- Added space after commans where ever applicable - Removed trailing whilte spaces - Added white space after if condition - Added headers before package declaration

Added check for same redirect uri should be configured in client id

3834f17

Fixed checkstyle issue in redirect uri test in Test class

4018cb2

paurav-munshi force-pushed the authorization-filter branch from df6f818 to 7a3e658 Compare May 18, 2020 17:31

paurav-munshi requested a review from jgrandja May 18, 2020 18:04

kratostaine reviewed May 18, 2020

View reviewed changes

jgrandja self-assigned this May 21, 2020

jgrandja added status: duplicate A duplicate of another issue type: enhancement A general enhancement labels May 21, 2020

jgrandja added this to the 0.0.1 milestone May 21, 2020

jgrandja added a commit that referenced this pull request May 21, 2020

Polish gh-77

fbc98d5

jgrandja closed this May 21, 2020

jgrandja added a commit that referenced this pull request Jun 28, 2020

Polish OAuth2AuthorizationEndpointFilterTests

02b64f0

Issue gh-77

doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023

Polish spring-projectsgh-77

0d5305a

doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023

Polish OAuth2AuthorizationEndpointFilterTests

1a261e8

Issue spring-projectsgh-77


		private static final String DEFAULT_ENDPOINT = "/oauth2/authorize";

		private static final OAuth2Error CLIENT_ID_ABSENT_ERROR = new OAuth2Error(OAuth2ErrorCodes.INVALID_REQUEST, OAuth2AuthorizationServerMessages.REQUEST_MISSING_CLIENT_ID, null);



		public OAuth2AuthorizationEndpointFilter() {
		authorizationEndpiontMatcher = new AntPathRequestMatcher(DEFAULT_ENDPOINT);


		}

		protected OAuth2Authorization buildOAuth2Authorization(RegisteredClient client,

Authorization Endpoint filter for Authorization Code flow #77

Authorization Endpoint filter for Authorization Code flow #77

Conversation

paurav-munshi commented May 4, 2020 • edited Loading

Functionality Covered

Validations covered

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paurav-munshi commented May 11, 2020

kratostaine commented May 16, 2020

paurav-munshi commented May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kratostaine May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja commented May 21, 2020

paurav-munshi commented May 4, 2020 •

edited

Loading

jgrandja May 11, 2020 •

edited

Loading

paurav-munshi commented May 18, 2020 •

edited

Loading

kratostaine May 18, 2020 •

edited

Loading