Expected Behavior

OAuth2AuthorizedClientService#saveAuthorizedClient accepts OAuth2AuthorizedClient authorizedClient and String principalName instead of OAuth2AuthorizedClient authorizedClient and Authentication principal

Current Behavior

OAuth2AuthorizedClientService#loadAuthorizedClient and OAuth2AuthorizedClientService#removeAuthorizedClient both take in a String for the principal name. OAuth2AuthorizedClientService#saveAuthorizedClient takes in and Authentication. The default implementation InMemoryOAuth2AuthorizedClientService just extracts the name from the Authentication.

Context

Not all OAuth2AuthorizedClient are derived from user based interactions. MANY instances of OAuth2AuthorizedClient are based on client_credentials grant. I can have an application that communicates with a OAuth protected resource but to invoke this operation doesn't require a user to authenticate with my application. It could be triggered via cron or using a scheduler. Yet I will want to persist that OAuth2AuthorizedClient for future use until it is expired.

I don't see any reason why saveAuthorizedClient method would require an Authentication object to save when the other methods only accept a String

Comment From: jgrandja

@loesak OAuth2AuthorizedClientService.saveAuthorizedClient() requires an Authentication instance to ensure the OAuth2AuthorizedClient (client) is correctly associated to the Authentication (resource owner OR client principal).

Not all OAuth2AuthorizedClient are derived from user based interactions. MANY instances of OAuth2AuthorizedClient are based on client_credentials grant

Yes, that is correct. However, an OAuth2AuthorizedClient is always associated with an Authentication and in the client_credentials instance the Authentication is the client Principal.

Furthermore, the Authentication instance is available after the authorization_code or client_credentials grant flow succeeds so it makes sense to pass in the Authentication instance to the OAuth2AuthorizedClientService implementation so it can ensure the correct association between client and principal owner.

The loadAuthorizedClient() and removeAuthorizedClient() takes in a String for principalName since the Authentication may not be known at this time OR it gives flexibility to pass in another principalName other then the current one, eg. an admin web app may want to fetch an OAuth2AuthorizedClient associated with a specific principalName for the purpose of removeAuthorizedClient() - to force re-authorization

I'm going to close this given the API design was intentional.

Comment From: loesak

@jgrandja Thank you for your reply. Maybe you can help me understand what I am doing wrong then. I have this code that is allowing me to use the new Spring Security OAuth framework with Open Feign.

https://gist.github.com/loesak/ec97dfb891381932187e655153e741ae#file-oauth2feignrequestinterceptor-springsecurity5

The code is loosely taken from code you are familiar with

https://github.com/spring-projects/spring-security/blob/5.1.8.RELEASE/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunction.java

Its job is similar, accept it only deals with client_credentials grant type, but for Feign instead of WebClient. You can see on line 78 of the gist that i need to create a bogus Authentication object just for the purpose of persistence to the OAuth2AuthorizedClientService this was taken from the implementation of ServletOAuth2AuthorizedClientExchangeFilterFunction (line 476) which pretty much does the same using a PrincipalNameAuthentication which is a private static class that is not reusable.

You state

and in the client_credentials instance the Authentication is the client Principal.

Furthermore, the Authentication instance is available after the authorization_code or client_credentials grant flow succeeds so it makes sense to pass in the Authentication instance to the OAuth2AuthorizedClientService implementation so it can ensure the correct association between client and principal owner.

I'm not seeing where the authentication you speak of is coming from in this case. None of the methods for obtaining a token return an Authentication that I am aware of.

Comment From: jgrandja

@loesak I noticed that you are referencing ServletOAuth2AuthorizedClientExchangeFilterFunction from Spring Security 5.1.8. This is quite an old version as far as the OAuth support goes. You really should at least use 5.2.0 or the latest. The 5.2.0 version introduced OAuth2AuthorizedClientManager / OAuth2AuthorizedClientProvider, which handles all the logic necessary for authorizing (or re-authorizing) a client. There is no need to directly use ClientRegistrationRepository and OAuth2AuthorizedClientService and instead use OAuth2AuthorizedClientManager in OAuth2FeignRequestInterceptor. This will simplify your code greatly.

Also take a look at Client Credentials ref doc for more sample code. If you're using the client in a back-end service than you may want to use AuthorizedClientServiceOAuth2AuthorizedClientManager.

Comment From: loesak

@jgrandja just getting back to this. You are correct. That was much easier.

https://gist.github.com/loesak/d042f545a57bb6e875347542b1eb1793#file-oauth2feignrequestinterceptor-v2-java

I still find it strange that I still need to create a fake authentication object ...

"When operating outside of a HttpServletRequest context, use AuthorizedClientServiceOAuth2AuthorizedClientManager instead." - https://docs.spring.io/spring-security/site/docs/5.3.2.RELEASE/reference/html5/#oauth2Client-authorized-manager-provider

OAuth2AuthorizeRequest authorizeRequest = OAuth2AuthorizeRequest
                .withClientRegistrationId(this.clientRegistrationId)
                .principal(ANONYMOUS_AUTHENTICATION)
                .build();

OAuth2AuthorizedClient authorizedClient = this.authorizedClientManager.authorize(authorizeRequest);
OAuth2AccessToken accessToken = Objects.requireNonNull(authorizedClient).getAccessToken();
template.header(HttpHeaders.AUTHORIZATION, String.format("Bearer %s", accessToken.getTokenValue()));

But overall much cleaner. Thank you.

Comment From: jgrandja

@loesak

I still find it strange that I still need to create a fake authentication object

This was added in 5.3.0 -> OAuth2AuthorizeRequest.Builder.principal(String principalName)

Comment From: loesak

I guess the question is, in the case when using working outside of a HttpServletRequest context, or with client credentials or password grant for that matter, is a principal needed at all? When would the value ever be different? And whatever value it is, in these cases, will it ever actually represent a authentication principal or be more just a cache key?

I think I understand why it's needed inside a HttpServletRequest context and/or authorization code grant, maybe you can correct me if I am wrong, but I assume its to associate the obtained access tokens to the authenticated user to avoid cross user access token use (a.k.a. leakage)?

Comment From: loesak

@jgrandja apologies. forgot to tag you. see above.

Comment From: jgrandja

@loesak OAuth2AuthorizedClient.principalName is required as it specifies the "owner" of the OAuth2AccessToken. For the authorization_code grant, the principal is the Resource Owner. However, for the client_credentials grant, the principal is the client and is typically assigned the client_id.

Comment From: loesak

@jgrandja ok. Makes sense. Thank you for your quick responses.