Expected Behavior AuthorizedClientServiceOAuth2AuthorizedClientManager/DefaultOAuth2AuthorizedClientManager should generate only one OAuth-Client-Credentials-Grant request when authorizedClient is expired.
Current Behavior AuthorizedClientServiceOAuth2AuthorizedClientManager/DefaultOAuth2AuthorizedClientManager could generate large amount of OAuth-Client-Credentials-Grant requests when authorizedClient is expired.
Context When using AuthorizedClientServiceOAuth2AuthorizedClientManager/DefaultOAuth2AuthorizedClientManager, the OAuth2AuthorizedClient is cached globally. The OAuth2AuthorizedClient in my case is a OAuth-Client-Credentials authorized client. When OAuth2AuthorizedClient expired, a new OAuth2AuthorizedClient is generated by calling authorizedClientProvider.authorize(). However, in highly concurrent scenarios, this behavior will generate large amount of OAuth-Client-Credentials-Grant requests, which is harmful to the OAuth server.
Comment From: sjohnr
Hi @zhenchuan9r, thanks for reaching out!
AuthorizedClientServiceOAuth2AuthorizedClientManager/DefaultOAuth2AuthorizedClientManager could generate large amount of OAuth-Client-Credentials-Grant requests when authorizedClient is expired.
This seems like it would require a large number of simultaneous requests to the client, is that correct? Approximately how many requests would you anticipate happening at about the same time?
However, in highly concurrent scenarios, this behavior will generate large amount of OAuth-Client-Credentials-Grant requests, which is harmful to the OAuth server.
Can you clarify what you mean by "which is harmful to the OAuth server"? For example, do you mean that the authorization server would simply be required to generate more tokens than is absolutely necessary? Do you see this as being more than just an optimization on the client side to prevent concurrent requests?
Also, can you provide an example scenario, or perhaps a minimal sample that reproduces the issue?
Comment From: spring-projects-issues
If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.
Comment From: zhenchuan9r
@GetMapping("/test")
public String getRemoteValue() {
String accessToken = getAccessTokenValue();
RequestEntity requestEntity = RequestEntity.get("http://my-service/api/dummy")
.header("Authorization", accessToken)
.build();
restTemplate.exchange(requestEntity, String.class);
}
public String getAccessTokenValue() {
OAuth2AuthorizeRequest authorizeRequest = OAuth2AuthorizeRequest
.withClientRegistrationId("regId")
.principal("dummy")
.build();
OAuth2AuthorizedClient authorizedClient = authorizedClientServiceOAuth2AuthorizedClientManager.authorize(authorizeRequest);
OAuth2AccessToken accessToken = authorizedClient.getAccessToken();
if (accessToken == null) {
throw new RuntimeException("Failed to fetch access token");
}
return accessToken.getTokenType().getValue() + " " + accessToken.getTokenValue();
}
@Bean
@ConditionalOnBean({ClientRegistrationRepository.class, OAuth2AuthorizedClientService.class})
public AuthorizedClientServiceOAuth2AuthorizedClientManager getAuthorizedClientServiceOAuth2AuthorizedClientManager (
ClientRegistrationRepository clientRegistrationRepository,
OAuth2AuthorizedClientService authorizedClientService) {
OAuth2AuthorizedClientProvider authorizedClientProvider =
OAuth2AuthorizedClientProviderBuilder.builder()
.clientCredentials()
.build();
AuthorizedClientServiceOAuth2AuthorizedClientManager authorizedClientManager =
new AuthorizedClientServiceOAuth2AuthorizedClientManager(
clientRegistrationRepository, authorizedClientService);
authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);
return authorizedClientManager;
}
A basic example would be like above. When authorizedClient in OAuth2AuthorizedClientService (InMemoryOAuth2AuthorizedClientService) expires and many requests (1000 eg.) rush to /test endpoint, OAuth2AuthorizedClientProvider will generate approximately equal number of token requests to the authorization server. This behaviour is not necessary at all as the token can be reused across requests.
Comment From: sjohnr
Hi @zhenchuan9r.
When authorizedClient in OAuth2AuthorizedClientService (InMemoryOAuth2AuthorizedClientService) expires and many requests (1000 eg.) rush to /test endpoint,
The example uses client_credentials with an AuthorizedClientServiceOAuth2AuthorizedClientManager. This service will scope the token based on the combination of registrationId and principal.
OAuth2AuthorizeRequest authorizeRequest = OAuth2AuthorizeRequest
.withClientRegistrationId("regId")
.principal("dummy")
.build();
In this example, the token is then global to your application and could be shared by many threads. What you're describing here sounds like a typical race condition one might encounter in this situation.
OAuth2AuthorizedClientProvider will generate approximately equal number of token requests to the authorization server.
Perhaps 1000 requests is just an example. However, it would probably take quite a few more requests than that to create problems for most authorization servers. This would not typically be an issue, because rate limiting requests would be handled at the edge of the architecture (e.g. with an API gateway).
If it's an issue for you and requests are not rate limited, or requests are rate limited but you need additional measures to prevent excessive requests, you can employ normal race condition mitigation techniques, such as synchronized blocks or other more advanced techniques as needed.
The following example will easily restrict to only a single simultaneous request per instance of the client application:
OAuth2AuthorizedClient authorizedClient;
synchronized (authorizedClientServiceOAuth2AuthorizedClientManager) {
authorizedClient = authorizedClientServiceOAuth2AuthorizedClientManager.authorize(authorizeRequest);
}
...
Whether this technique or any other makes sense is application-dependant, but I don't believe we would want to place logic like this in the framework since there are many use cases supported by the OAuth2AuthorizedClientManager and OAuth2AuthorizedClientProvider, and many of them would suffer performance problems with such a solution.
I'm going to close this issue for now, given the existing level of detail describing the problem is quite lacking and there are normal workarounds available (see above example) for the problem as stated. If you have additional details or you feel I have misunderstood something, please add those details and we can continue the conversation, and even re-open the issue if necessary.
Comment From: zhenchuan9r
A possible solution, is to set two limits in OAuth2AuthorizedClientProvider, which is how I am doing now. Originally, when the authorizedClient expires within 1 min, OAuth2AuthorizedClientProvider will request a new authorizedClient in current thread. We can change the 1 min limit to a 30 sec hard limit and 1 min hard limit. When the soft limit (1 min) reaches, use a separated thread to request a new authorizedClient. When the hard limit (30 sec) reaches, block all threads and wait for a new authorizedClient.
Comment From: sjohnr
@zhenchuan9r, that's a nice idea. However, I'm not clear on how it solves the original problem you mentioned, e.g.
many requests (1000 eg.) rush to /test endpoint
We could continue tweaking this, though. For example, add an AtomicBoolean to guard against excessive requests during the soft period, etc. All of this would be good application-specific additional protection, but doesn't make much sense in the framework as again, the abstractions and classes support numerous use cases and grant types and not all of them need or would benefit from this additional logic and protective code. It sounds like you know of a way to layer this logic into the framework, correct? If not, let me know and I'd be happy to put together an example.