Skip to content
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

Introduce OAuth2AuthorizedClient Manager/Provider #6845

Closed

Conversation

@jgrandja
Copy link
Collaborator

commented May 6, 2019

This PR addresses the Servlet implementation for #6811 - the Reactive implementation will be submitted in a separate PR.

The primary responsibility of an implementation of an OAuth2AuthorizedClientProvider is to authorize (or re-authorize) a client. This interface would be implemented by specific grant types, e.g. authorization_code, refresh_token, client_credentials, password, custom/extension grants, etc.

@jgrandja jgrandja requested review from rwinch and jzheaux May 6, 2019

@jgrandja jgrandja added the in: oauth2 label May 6, 2019

@rwinch

This comment has been minimized.

Copy link
Member

commented May 7, 2019

It's worth asking ourselves...are we trying to just look up an access token or combine it with a lookup of the current access token with resolving it if it isn't found.

Another concern I have with the current APIs is that method inputs are things like ClientRegistration which might not be very easily accessed without injecting multiple collaborators. I rather like how we currently support WebClient support where the user can provide just a clientRegistrationId or the ClientRegistration and the support will handle the details for the user. I also like how things like HttpServletRequest can be defaulted to the current ThreadLocal.

To help us understand what is going to be of the most use to our users, I'd like the PR to include some sample usage of the API (in services not just controllers) too.

@jgrandja

This comment has been minimized.

Copy link
Collaborator Author

commented May 7, 2019

are we trying to just look up an access token or combine it with a lookup of the current access token with resolving it if it isn't found

The OAuth2ClientAuthorizationProvider should handle both use-cases - authorizing a new client OR re-authorizing an existing client (e.g. access token is expired).

The flow is triggered by the input to ServletOAuth2ClientAuthorizationContext.authorize(ClientRegistration ... (authorize new client) OR ServletOAuth2ClientAuthorizationContext.reauthorize(OAuth2AuthorizedClient ... (re-authorize existing client because the access token is expired)

@rwinch

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Can we put together a sample of its usage? Also demo what our internal classes will look like when consuming this API? I'd really like to see this driven by the usage and a firm understanding of how the user will interact with the API.

@jzheaux
Copy link
Contributor

left a comment

This looks great, @jgrandja. I especially like how this has the same feel as authentication providers.

I've left some feedback inline.

@jgrandja jgrandja force-pushed the jgrandja:gh-6811-webclient-ext-reuse branch from ebee74f to 359bbeb Jun 7, 2019

@jgrandja jgrandja changed the title Introduce OAuth2ClientAuthorizationProvider Introduce OAuth2AuthorizedClientProvider Jun 7, 2019

@jgrandja jgrandja force-pushed the jgrandja:gh-6811-webclient-ext-reuse branch 5 times, most recently from 7c393dd to 5ab9d67 Jun 7, 2019

@jgrandja jgrandja force-pushed the jgrandja:gh-6811-webclient-ext-reuse branch from 5ab9d67 to eef15bc Jun 16, 2019

@jgrandja jgrandja self-assigned this Jun 16, 2019

jgrandja added some commits Jun 27, 2019

OAuth2AuthorizedClientProvider implementations load/save OAuth2Author…
…izedClient

- #59 Redesign OAuth2AuthorizedClientProvider to load/save OAuth2AuthorizedClient
- #60 ClientCredentialsOAuth2AuthorizedClientProvider should load/save OAuth2AuthorizedClient
- #61 RefreshTokenOAuth2AuthorizedClientProvider should load/save OAuth2AuthorizedClient
- #62 Refactor and use redesigned OAuth2AuthorizedClientProvider implementations
@rwinch
Copy link
Member

left a comment

Thanks for the updates.

I'm not a fan with the fact that we need to look up all the information multiple times. For example, AuthorizationCodeOAuth2AuthorizedClientProvider can look up the ClientRegistration and the OAuth2AuthorizedClient. ClientCredentialsOAuth2AuthorizedClientProvider can also look up the ClientRegistration and OAuth2AuthorizedClient. Nearly every implementation of OAuth2AuthorizedClientProvider has the potential to look up duplicate information which is going to add a lot of unnecessary overhead.

Another thing I question is "Why are these split up into separate implementations to begin with?" It seems that the information for what a user wants is already going to be provided. For example, if the grant type is client credentials they probably want to support that mechanism. They probably don't want to ensure that they have created the DelegatingOAuth2AuthorizedClientProvider properly.

It might be nice to support situations where pieces of this information is already known (i.e. we already have a ClientRegistration or OAuth2AuthorizedClient) without needing to look up additional information.

Introduce OAuth2AuthorizedClientManager
- #73 Introduce OAuth2AuthorizedClientManager
- #74 Integrate OAuth2AuthorizedClientManager with OAuth2AuthorizedClientProvider(s)
- #81 Add builder for OAuth2AuthorizedClientProvider
@rwinch
Copy link
Member

left a comment

Thanks for the updates. I have replied inline

jgrandja added some commits Jul 12, 2019

private ClientRegistration clientRegistration;
private OAuth2AuthorizedClient authorizedClient;
private Authentication principal;
private Map<String, Object> attributes;

This comment has been minimized.

Copy link
@rwinch

rwinch Jul 16, 2019

Member

I'm still curious how this is used in practice without custom code. At the moment, DefaultOAuth2AuthorizedClientManager doesn't add any attributes. Perhaps it is better to wait on such functionality (including the DefaultOAuth2AuthorizedClientManager.contextAttributesMapper).

This comment has been minimized.

Copy link
@jgrandja

jgrandja Jul 16, 2019

Author Collaborator

Ok, I'll remove from this PR and add contextAttributesMapper back in for password grant PR, as that's where it will be used initially

This comment has been minimized.

Copy link
@jgrandja

jgrandja Jul 17, 2019

Author Collaborator

Just to clarify from my last comment, I will remove DefaultOAuth2AuthorizedClientManager.contextAttributesMapper but will not remove OAuth2AuthorizationContext.attributes as it's being used by RefreshTokenOAuth2AuthorizedClientProvider. You can see the usage in this test

This comment has been minimized.

Copy link
@jgrandja

jgrandja Jul 17, 2019

Author Collaborator

@rwinch There indeed was a gap in my initial implementation of DefaultOAuth2AuthorizedClientManager. There now is a default implementation of contextAttributesMapper that provides the support for RefreshTokenOAuth2AuthorizedClientProvider and very possibly future implementations of OAuth2AuthorizedClientProvider (e.g. dynamic scopes). Please see this test for usage.

* @see OAuth2AuthorizeRequest
* @see OAuth2AuthorizedClientManager
*/
public class OAuth2ReauthorizeRequest extends OAuth2AuthorizeRequest {

This comment has been minimized.

Copy link
@rwinch

rwinch Jul 16, 2019

Member

I don't think it makes sense for OAuth2ReauthorizeRequest to extend OAuth2AuthorizeRequest. What does it mean for OAuth2ReauthorizeRequest to be passed into OAuth2AuthorizedClientManager.authorize? The types should enforce correct usage.

This comment has been minimized.

Copy link
@jgrandja

jgrandja Jul 16, 2019

Author Collaborator

I don't think it makes sense for OAuth2ReauthorizeRequest to extend OAuth2AuthorizeRequest

This makes perfect sense to me. A request to re-authorize, (e.g. token is expired) is considered an authorization request as well. Therefore, a re-authorization request is an authorization request.

What does it mean for OAuth2ReauthorizeRequest to be passed into OAuth2AuthorizedClientManager.authorize()

Why would this even happen? The API is clear authorize(OAuth2AuthorizeRequest) so even if a OAuth2ReauthorizeRequest was passed into authorize() it would only be used as an OAuth2AuthorizeRequest and never cast to an OAuth2ReauthorizeRequest.
Can you expand on the use case because as far as I see it the user would not be using the API correctly in this instance if they were to pass in an OAuth2ReauthorizeRequest, but the method implementation would be correctly implemented based on the clear definition of the operation and it's input.

Also, the added benefit of having the hierarchy OAuth2ReauthorizeRequest extends OAuth2AuthorizeRequest is the DefaultOAuth2AuthorizedClientManager will only need one setContextAttributesMapper(Function<OAuth2AuthorizeRequest, Map<String, Object>> contextAttributesMapper). It would not be ideal to have a second setContextAttributesMapper(Function<OAuth2ReauthorizeRequest, Map<String, Object>> contextAttributesMapper) (typed OAuth2ReauthorizeRequest)

This comment has been minimized.

Copy link
@rwinch

rwinch Jul 17, 2019

Member

The API should enforce proper usage. It is surprising behavior that the API allows an
OAuth2AuthorizedClient to be provided to authorize via the OAuth2ReauthorizeRequest but it will be completely ignored.

This comment has been minimized.

Copy link
@jgrandja

jgrandja Jul 17, 2019

Author Collaborator

I'd like to hear your answer regarding my question...

Why would this even happen?...

Is this really a valid scenario? Like I already mentioned this would be a user error on the usage of the API. But the API would still behave as expected, ignoring the OAuth2ReauthorizeRequest attributes.

Furthermore, based on this understanding does this issue become invalid? See comment. Because based on this issue, an OAuth2LoginAuthenticationToken could get passed into an OAuth2AuthorizationCodeAuthenticationProvider which is identically scenario we are discussing here. @jzheaux Let's get your input here as I'm still not convinced.

This comment has been minimized.

Copy link
@rwinch

rwinch Jul 18, 2019

Member

Is this really a valid scenario?

That is exactly my point. The API should not allow this to happen, but it does.

But the API would still behave as expected,

As a user I find this confusing. If I create an OAuth2ReauthorizeRequest providing the OAuth2AuthorizedClient, it is surprising to me that the OAuth2AuthorizedClient would be ignored.

This comment has been minimized.

Copy link
@jzheaux

jzheaux Jul 18, 2019

Contributor

Is there a need for the two types and the separate methods? Looking at the default implementations, there seems to be a large overlap. If both are indeed authorize requests, as the hierarchy implies, perhaps OAuth2AuthorizeRequest could optionally hold an OAuth2AuthorizedClient - would this remove the need for reauthorize and OAuth2ReauthorizeRequest?

This comment has been minimized.

Copy link
@jgrandja

jgrandja Jul 19, 2019

Author Collaborator

I personally like the clarity of the API for OAuth2AuthorizedClientManager having both authorize() and reauthorize(). However, I'm open to discussing the removal of reauthorize(). If we were to do that than we have this:

public interface OAuth2AuthorizedClientManager {
	@Nullable
	OAuth2AuthorizedClient authorize(OAuth2AuthorizeRequest authorizeRequest);
}

public class OAuth2AuthorizeRequest {
	private final String clientRegistrationId;
	private final OAuth2AuthorizedClient authorizedClient;
	private final Authentication principal;
	private final HttpServletRequest servletRequest;
	private final HttpServletResponse servletResponse;

	public OAuth2AuthorizeRequest(String clientRegistrationId, Authentication principal,
									HttpServletRequest servletRequest, HttpServletResponse servletResponse) {

	}

	public OAuth2AuthorizeRequest(OAuth2AuthorizedClient authorizedClient, Authentication principal,
									HttpServletRequest servletRequest, HttpServletResponse servletResponse) {

	}
	

OAuth2AuthorizeRequest would provide 2 constructors and would be similar to the members in OAuth2AuthorizationContext which is passed downstream to the OAuth2AuthorizedClientProvider(s).

@rwinch What are your thoughts on this proposed design?

This comment has been minimized.

Copy link
@rwinch

rwinch Jul 23, 2019

Member

I'm ok with this proposal as it solve my concern and aligns with our previous discussions (i.e. we can have two separate methods with two request types or the same method with a single request type).

@rwinch rwinch requested a review from jzheaux Jul 16, 2019

@jgrandja jgrandja referenced this pull request Jul 17, 2019
@jgrandja

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2019

@rwinch @jzheaux I've pushed all the updates from last review. There are only 2 items that still need to be addressed:

@jgrandja

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 23, 2019

@rwinch @jzheaux I've addressed the remaining 2 items. Ready for review.

@rwinch

rwinch approved these changes Jul 24, 2019

@jgrandja jgrandja added this to the 5.2.0.M4 milestone Jul 25, 2019

@jgrandja jgrandja changed the title Introduce OAuth2AuthorizedClientProvider Introduce OAuth2AuthorizedClient Manager/Provider Jul 25, 2019

@jgrandja jgrandja closed this in c05b076 Jul 25, 2019

@jgrandja jgrandja deleted the jgrandja:gh-6811-webclient-ext-reuse branch Jul 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.