Spring Security oauth2-client: Parallel refresh requests are very slow (WebFlux)

Expected Behavior

If there are parallel requests that require a refreshed token, this should only need to be done once - not by each concurrent request.

Current Behavior

Right now, if there are several concurrent requests that require a refresh they all make requests to the provider for a new token. This is not efficient.

Moreover, if the refresh token is reissued each time its used - concurrent refresh requests will fail, as the old refresh token is invalidated.

Context

    @Bean
    public ReactiveOAuth2AuthorizedClientManager authorizedClientManager(
            ReactiveClientRegistrationRepository clientRegistrationRepository,
            ServerOAuth2AuthorizedClientRepository authorizedClientRepository) {

        ReactiveOAuth2AuthorizedClientProvider authorizedClientProvider =
                ReactiveOAuth2AuthorizedClientProviderBuilder.builder()
                        .authorizationCode()
                        .refreshToken()
                        .build();

        DefaultReactiveOAuth2AuthorizedClientManager authorizedClientManager =
                new DefaultReactiveOAuth2AuthorizedClientManager(
                        clientRegistrationRepository, authorizedClientRepository);
        authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);

        return authorizedClientManager;

    }

1. The DefaultReactiveOAuth2AuthorizedClientManager handles all the oauth2 stuff nicely, loads (loadAuthorizedClient) the OAuth2AuthorizedClient from ReactiveClientRegistrationRepository
2. Then DelegatingReactiveOAuth2AuthorizedClientProvider runs an authorize method for each provider (in our case: AuthorizationCodeReactiveOAuth2AuthorizedClientProvider, RefreshTokenReactiveOAuth2AuthorizedClientProvider)
3. RefreshTokenReactiveOAuth2AuthorizedClientProvider always runs this logic on the context passed to the method to check if a refresh is required, but ignores the fact that OAuth2AuthorizedClient might have been updated (and that refresh has already happened).

OAuth2AuthorizedClient authorizedClient = context.getAuthorizedClient();
        if (authorizedClient == null || authorizedClient.getRefreshToken() == null
                || !hasTokenExpired(authorizedClient.getAccessToken())) {
            return Mono.empty();
        }

Because there is no synchronization around the refreshing block and that OAuth2AuthorizedClient is loaded at DefaultReactiveOAuth2AuthorizedClientManager and never reloaded in `RefreshTokenReactiveOAuth2AuthorizedClientProvider, the refresh is done for all concurrent requests.

I'm wondering can this be optimism so that when a refresh is required it will block other requests from that user session, refresh the token (once) and then other requests will be able to use that refreshed token.

Example of slow requests we've seen at refresh

Comment From: JJRdec

Any ideas on what is happening here?

Comment From: sjohnr

Thanks for reacyhing out @JJRdec!

There seem to be two things you're looking to solve in this issue:

1. Highly concurrent requests resulting in multiple token requests to the authorization server.
2. Slow throughput of concurrent requests to the authorization server

Regarding (1), this has been discussed before (for example, see here and here). From my perspective, this is not really something the framework can solve generally. See comments above for more details and possible ideas for workarounds.

Regarding (2), I would highly encourage you to look into the authorization server to determine why concurrent requests (I don't know how many based on the information provided) are slow. There is not a good reason that several concurrent requests (even as many as 1k requests) would take that long, and the issue is likely on the authorization server side.

Also, I don't see enough information to go further with this issue, but please note that based on the information provided I don't believe there would likely be a change in the framework. For these reasons I'm going to close this issue. If you feel I have misunderstood anything (which is entirely possible), please provide a minimal sample to reproduce the issue you're seeing and I can take a deeper look.

Comment From: JJRdec

Coming back give some insight into the issue I was seeing here. The bottle neck was on our oauth2 provider, created spring-security-oauth2-authorization-server.

It was down to our BCryptPasswordEncoder, when we made concurrent requests on the token endpoint we had to validate the clientSecret for each request. Our provider app got very slow as the CPU cost of validating many clientSecret was too much.

This is something that should be considered when running load tests on security-oauth2-authorization-server, as BCryptPasswordEncoder is a commonly used PasswordEncoder implementation.

Comment From: sjohnr

@JJRdec thanks for the additional feedback. The reality is that BCryptPasswordEncoder taking an amount of time to validate passwords and client secrets is intentional. Please see Password Storage in the reference for more information.

I don't know why your situation requires so many parallel requests. In your case, you will need to find a way to serialize those requests (there are many options at the application level), but may require some additional thought beyond simply using the ReactiveOAuth2AuthorizedClientManager. If you feel the framework should do more, I'd encourage you to review and provide feedback on issue gh-15295. The solution proposed there may also help you (as a custom solution in the meantime).

Comment From: ch4mpy

you will need to find a way to serialize those requests

@sjohnr do you mean that in the case of an SPA calling Spring Cloud Gateway configured with oauth2Login and the TokenRelay= filter, your advice is to queue the (asynchronous) requests instead of letting it run in parallel or to replace the authentication manager implementation?

In the case of rotating refresh tokens (and it should), there is no need for a high concurrency for token refreshing to fail. Two requests from the same source in less time than required for the refresh token flow to run are enough.

Shouldn't this issue be reopened to modify the default implementation so that it handles correctly parallel requests from the same source?