Skip to content

Conversation

@kratostaine
Copy link
Contributor

@kratostaine kratostaine commented May 7, 2020

Fixes #67

Implementation Requirements

  • the Filter should process requests for the (default) path /oauth2/token
  • authorizationGrantConverter should convert a valid authorization_code Access Token Request to OAuth2AuthorizationCodeAuthenticationToken
  • the OAuth2AuthorizationCodeAuthenticationToken should be passed to the AuthenticationManager
  • the AuthenticationManager should be composed of OAuth2AuthorizationCodeAuthenticationProvider
  • OAuth2AccessTokenAuthenticationToken should be returned from AuthenticationManager
  • the OAuth2AccessToken should be updated in the in-flight OAuth2Authorization
  • failure scenarios
  • javadoc class and public methods
  • Unit tests

@kratostaine
Copy link
Contributor Author

@jgrandja Have added the initial changes for early feedback. Tests, javadoc to follow. Also need your help to understand the required failure scenarios to be handled here so that I don't add unnecessary checks and failure handling.

@jgrandja
Copy link
Collaborator

jgrandja commented May 7, 2020

@kratostaine Please review 4.1.3. Access Token Request to understand all the validations that need to be applied. At minimum, you will need to validate the REQUIRED parameters. However, some of the parameter values will be validated by the OAuth2AuthorizationCodeAuthenticationProvider #68, eg. code

If you are still unsure on what to validate, please send me the reference in the spec and I could let you know if it should be performed in OAuth2TokenEndpointFilter or OAuth2AuthorizationCodeAuthenticationProvider.

@kratostaine
Copy link
Contributor Author

  grant_type
         REQUIRED.  Value MUST be set to "authorization_code".

   code
         REQUIRED.  The authorization code received from the
         authorization server.

   redirect_uri
         REQUIRED, if the "redirect_uri" parameter was included in the
         authorization request as described in Section 4.1.1, and their
         values MUST be identical.

   client_id
         REQUIRED, if the client is not authenticating with the
         authorization server as described in Section 3.2.1.

@jgrandja the above 4 are the reqd parameters as per the spec. I have done a validation on grant_type already. I also need to do a non-nullable string validation on code, redirect_uri and client_id in the OAuth2TokenEndpointFilter right? Also, how do I validate here the redirect_uri to have value identical to the one as part of the authorization request?

@jgrandja
Copy link
Collaborator

jgrandja commented May 8, 2020

@kratostaine

I also need to do a non-nullable string validation on code, redirect_uri and client_id in the OAuth2TokenEndpointFilter right?

Yes for code and redirect_uri. The (confidential) client is required to authenticate in the authorization_code flow, so the expectation is that OAuth2ClientAuthenticationToken #72 is available in the SecurityContext - no need to validate client_id.

how do I validate here the redirect_uri to have value identical to the one as part of the authorization request?

The OAuth2AuthorizationCodeAuthenticationProvider #68 is responsible for this validation.

@kratostaine kratostaine force-pushed the master branch 2 times, most recently from 1c0bd69 to 871b37f Compare May 9, 2020 06:55
@kratostaine
Copy link
Contributor Author

kratostaine commented May 9, 2020

Tests to follow. @jgrandja Please review this so that if there are any changes required, I won't have to rework on the tests.

@kratostaine
Copy link
Contributor Author

kratostaine commented May 9, 2020

@jgrandja Also regarding AuthenticationManager implementation, I have created a custom class implementing it as OAuth2ClientAuthenticationManager. Is that fine or should I be using the AuthenticationManagerBuilder and set the auth provider with its method authenticationProvider(AuthenticationProvider authenticationProvider) or should I be using ProviderManager?

Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @kratostaine. Please see review comments.

Also, please add tests before next review. I typically do not review until tests are written. Writing tests should demonstrate if the implementation is correct or if gaps are missing.

@kratostaine kratostaine force-pushed the master branch 3 times, most recently from 5bafec8 to 7f5ddfc Compare May 16, 2020 15:09
@kratostaine kratostaine marked this pull request as ready for review May 16, 2020 15:10
@kratostaine kratostaine requested a review from jgrandja May 16, 2020 15:10
@kratostaine kratostaine force-pushed the master branch 7 times, most recently from 3205ee1 to 058ca53 Compare May 18, 2020 06:06
@kratostaine
Copy link
Contributor Author

@jgrandja could you please help with reviewing this?

@jgrandja jgrandja self-assigned this May 26, 2020
@jgrandja jgrandja added status: duplicate A duplicate of another issue type: enhancement A general enhancement labels May 26, 2020
@jgrandja jgrandja added this to the 0.0.1 milestone May 26, 2020
jgrandja added a commit that referenced this pull request May 26, 2020
@jgrandja
Copy link
Collaborator

Thank you for all the updates @kratostaine!

Overall, the implementation looked good and complied to spec for the most part.

I did add a polish commit in order to get this merged, as I'm hoping to complete the first iteration of authorization_code grant later this week. I added some missing validation checks and re-organized some of the code.

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

Thank you again and I look forward to another contribution 👍

@jgrandja jgrandja closed this May 26, 2020
@jgrandja jgrandja mentioned this pull request May 29, 2020
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: duplicate A duplicate of another issue type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Token Endpoint

3 participants