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

Tokens for Client Credentials grant type should be cached only based on clientRegistrationId #14991

Closed
pzgadzaj-equinix opened this issue Apr 30, 2024 · 7 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: invalid An issue that we don't feel is valid type: enhancement A general enhancement

Comments

@pzgadzaj-equinix
Copy link

Expected Behavior

  1. Configure spring oauth2 registration with authorization grant type set to "client credentials"
  2. Use Code described in https://docs.spring.io/spring-security/reference/servlet/oauth2/client/authorization-grants.html#_using_the_access_token to get "client credentials" Access token

Execution of the above code (from the link) twice, each time for different principal will result in two "client credentials" calls.
I would expect that as client credentials token is user-agnostic, then Spring security does not cache such tokens based on principal

Current Behavior

Current implementation caches the "client credentials" tokens in a map which key is based on principal name (both JdbcOAuth2AuthorizedClientService and InMemoryOAuth2AuthorizedClientService)

Context

Workaround which might be an option is to extend InMemoryOAuth2AuthorizedClientService in a way that it considers "authorizationGrantType" of a clientRegistration. If it's "client_credentials" then the token is stored in a cache with key containing only the registration id, otherwise, current behavior would apply

@pzgadzaj-equinix pzgadzaj-equinix added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Apr 30, 2024
@sjohnr
Copy link
Member

sjohnr commented May 6, 2024

@pzgadzaj-equinix thanks for reaching out!

The code example you linked from the docs shows binding the access token to a principal using an Authentication injected into the method. You can bind the token to any authentication by simply constructing your own instead. Even easier, you can simply provide a principal(String) (e.g. principal("anonymous") to the builder instead of a principal(Authentication). So the library already has this capability, I don't think we need to make any changes.

Do you feel that the docs should have a clear example of this use case? Would that have helped you find the solution more easily?

@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels May 6, 2024
@sjohnr sjohnr self-assigned this May 6, 2024
@pzgadzaj-equinix
Copy link
Author

Initially i reported that as an improvement, but this is not an improvement but bug in my opinion

One more time, tokens retrieved in scope oauth flow with Client credentials grant are user agnostic. So if we imagine following situation:

  1. We have spring boot application which user OIDC login flow, and it communicates with other applications via two kind of REST rest endpoints:
  • one kind which uses m2m tokens
  • second kind which uses user tokens
  1. Both of clients are registered in separate entries within spring securty, with different grant type
  2. Now let's imagine that application is serving traffic for 1h. and within that one 1h it will serve traffic for 3600 distinctive users. When serving each user, application will make one call which requires m2m token and one call which will require user based token
  3. Serving such traffic will lead to storing of 3600 m2m tokens (and making 3600 token endpoint calls) in the cache and 3600 user based tokens

As for user based tokens, it's completely understandable that each user will have it;s own token stored in cache. But for m2m token is really bizarre, that when serving each distinctive user a fresh m2m token will be obtain from IDP. Especially that:

  • m2m tokens are user agnostic
  • m2m tokens are long lasting to avoid freequent interractions with IDP

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 8, 2024
@sjohnr
Copy link
Member

sjohnr commented May 8, 2024

@pzgadzaj-equinix I'm sorry you're having trouble with the client_credentials support in Spring Security.

Initially i reported that as an improvement, but this is not an improvement but bug in my opinion

As I mentioned, the link you shared points to an example in the reference for binding the token to an Authentication which is injected into a Spring MVC @Controller endpoint. This would not be a bug as you describe, but an intentional choice by the user. The docs do not state that the behavior you expect would be achieved by this configuration. This is why I mention a documentation improvement as a possible solution.

In order to proceed, please share the code you are using as a minimal, reproducible sample if you believe you have found a bug, and we can discuss it further.

@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels May 8, 2024
@pzgadzaj-equinix
Copy link
Author

@sjohnr

I can provide minimal reproducible sample, but first i spotted one issue with my initial description. We don't use DefaultOAuth2AuthorizedClientManager (how it's described in https://docs.spring.io/spring-security/reference/servlet/oauth2/client/authorization-grants.html#_using_the_access_token).

Instead, we use: https://docs.spring.io/spring-security/reference/servlet/oauth2/client/authorized-clients.html - so ServletOAuth2AuthorizedClientExchangeFilterFunction which does not provide a way to nullify (or set to anonymous) the Authentication. It always rely on SecurityContextHolder (thread local storage)

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 13, 2024
@sjohnr
Copy link
Member

sjohnr commented May 13, 2024

@pzgadzaj-equinix

ServletOAuth2AuthorizedClientExchangeFilterFunction which does not provide a way to nullify (or set to anonymous) the Authentication. It always rely on SecurityContextHolder (thread local storage)

You can actually still affect the Authentication used by providing it to attributes via the ServletOAuth2AuthorizedClientExchangeFilterFunction.authentication() helper method (covered in the docs page you linked).

@pzgadzaj-equinix
Copy link
Author

pzgadzaj-equinix commented May 13, 2024

So when i have 10 m2m calls to be made, and each of them uses web client, then in each of them i have to put that code:

`@GetMapping("/")
public String index() {
String resourceUri = ...

Authentication anonymousAuthentication = new AnonymousAuthenticationToken(
		"anonymous", "anonymousUser", AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS"));
String body = webClient
		.get()
		.uri(resourceUri)
		.attributes(authentication(anonymousAuthentication))
		.retrieve()
		.bodyToMono(String.class)
		.block();

...

return "index";

}`

There is no other way? And also, one more time, it's illogical that the solution which should serve use M2M tokens, cache the token on "user level"

@sjohnr
Copy link
Member

sjohnr commented May 13, 2024

@pzgadzaj-equinix thanks for your response.

At this point, I am going to close this issue since there is nothing to do. I do feel that there is room for improvement in the docs, but I will open specific issues for that at a later time as we are mostly enhancing the docs on a release-by-release basis. If you have any further questions, please open a stackoverflow question and share the link to that (so that others can find it) and I will be happy to answer.

@sjohnr sjohnr closed this as completed May 13, 2024
@sjohnr sjohnr added status: invalid An issue that we don't feel is valid and removed status: feedback-provided Feedback has been provided labels May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: invalid An issue that we don't feel is valid type: enhancement A general enhancement
Projects
Status: Done
Development

No branches or pull requests

3 participants