Refactor auth token/login providers #7356
Conversation
pdelert
commented
Sep 4, 2019
- OAuth2AuthorizationCodeAuthenticationToken uses OAuth2LoginAuthenticationToken
- OAuth2LoginAuthenticationProvider uses OAuth2AuthorizationCodeAuthenticationProvider
| @@ -39,11 +39,9 @@ | |||
| * @see OAuth2AccessToken | |||
| * @see <a target="_blank" href="https://tools.ietf.org/html/rfc6749#section-4.1">Section 4.1 Authorization Code Grant Flow</a> | |||
| */ | |||
| public class OAuth2AuthorizationCodeAuthenticationToken extends AbstractAuthenticationToken { | |||
| public class OAuth2AuthorizationCodeAuthenticationToken extends OAuth2LoginAuthenticationToken { | |||
There was a problem hiding this comment.
The proposed update should be OAuth2LoginAuthenticationToken extends OAuth2AuthorizationCodeAuthenticationToken. Please see comment.
OAuth2LoginAuthenticationProvider implements authorization_code grant flow on top of OpenID Connect, so it's logical for OAuth2LoginAuthenticationToken to extend OAuth2AuthorizationCodeAuthenticationToken.
| * | ||
| * @return the {@link ClientRegistration} | ||
| */ | ||
| public ClientRegistration getClientRegistration() { |
There was a problem hiding this comment.
This is a breaking change for a patch release. Please see Semantic versioning for more details.
| @@ -59,8 +59,7 @@ | |||
| * @see <a target="_blank" href="https://tools.ietf.org/html/rfc6749#section-4.1.3">Section 4.1.3 Access Token Request</a> | |||
| * @see <a target="_blank" href="https://tools.ietf.org/html/rfc6749#section-4.1.4">Section 4.1.4 Access Token Response</a> | |||
| */ | |||
| public class OAuth2LoginAuthenticationProvider implements AuthenticationProvider { | |||
| private final OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> accessTokenResponseClient; | |||
| public class OAuth2LoginAuthenticationProvider extends OAuth2AuthorizationCodeAuthenticationProvider { | |||
There was a problem hiding this comment.
The preference is that OAuth2LoginAuthenticationProvider uses OAuth2AuthorizationCodeAuthenticationProvider as a collaborator (composition) instead of through extension. Similar to how OAuth2LoginReactiveAuthenticationManager uses OAuth2AuthorizationCodeReactiveAuthenticationManager internally.
| @@ -118,7 +118,7 @@ public OAuth2LoginAuthenticationToken(ClientRegistration clientRegistration, | |||
| } | |||
|
|
|||
| @Override | |||
| public OAuth2User getPrincipal() { | |||
| public Object getPrincipal() { | |||
There was a problem hiding this comment.
This does not need to be changed.
* OAuth2LoginAuthenticationToken uses OAuth2AuthorizationCodeAuthenticationToken * OAuth2LoginAuthenticationProvider uses OAuth2AuthorizationCodeAuthenticationProvider
| @@ -83,4 +83,8 @@ public Authentication authenticate(Authentication authentication) throws Authent | |||
| public boolean supports(Class<?> authentication) { | |||
| return OAuth2AuthorizationCodeAuthenticationToken.class.isAssignableFrom(authentication); | |||
| } | |||
|
|
|||
| public OAuth2AccessTokenResponse getAccessTokenResponse(OAuth2AuthorizationCodeGrantRequest oAuth2AuthorizationCodeGrantRequest) { | |||
There was a problem hiding this comment.
This change cannot go into a patch release as this is targeted for 5.1.x. Please review the acceptable changes for patch, minor and major.
| authorizationCodeAuthentication.getClientRegistration(), | ||
| authorizationCodeAuthentication.getAuthorizationExchange())); | ||
| accessTokenResponse = ((OAuth2AuthorizationCodeAuthenticationProvider) this.authenticationProvider) | ||
| .getAccessTokenResponse( |
There was a problem hiding this comment.
Instead call OAuth2AuthorizationCodeAuthenticationProvider.authenticate()
| private OAuth2AuthorizationExchange authorizationExchange; | ||
| private OAuth2AccessToken accessToken; | ||
| private OAuth2RefreshToken refreshToken; | ||
| private OAuth2AuthorizationCodeAuthenticationToken oAuth2AuthorizationCodeAuthenticationToken; |
There was a problem hiding this comment.
Instead of composition use inheritance OAuth2LoginAuthenticationToken extends OAuth2AuthorizationCodeAuthenticationToken as noted in this comment
|
@pdelert Have you had time to apply the latest changes? |