RefreshTokenOAuth2AuthorizedClientProvider does not handle expired refresh token

[janzyka]

Summary

Refresh tokens don't support timeouts - if refresh token expires application will keep trying to refresh access token through it forever.

Also all class around OAuth2 client are final which doesn't allow to fix the problem on my side temporary.

Actual Behavior

1. App authorizes client and gets access & refresh tokens
2. App is making requests through access token
3. Access token expires, app is able to refresh
4. Now refresh token expires (!)
5. Attempt to refresh fails -> request fails
6. Any subsequent access leads to attempt to refresh through expired refresh token
7. Back to 5.

=> application will never recover

Expected Behavior

In order of preference

Option 1 - allow & handle refresh tokens expiry
Option 2 - add flag to optionally disable refresh tokens in PasswordOAuth2AuthorizedClientProvider
Option 3 - if refresh was attempted and failed remove client from authorizeClientRepository so the subsequent call starts from scratch allowing to recover

Idea 1 - don't make all classes final, it is difficult to add own behaviour. I remember nearly all classes in Spring used to be opened for extension.

Configuration

    @Bean
    public WebClient webClient(ClientRegistrationRepository clientRegistrationRepository, OAuth2AuthorizedClientService authorizedClientService) {

        AuthorizedClientServiceOAuth2AuthorizedClientManager manager = new AuthorizedClientServiceOAuth2AuthorizedClientManager(clientRegistrationRepository, authorizedClientService);
        manager.setAuthorizedClientProvider(new DelegatingOAuth2AuthorizedClientProvider(
                new RefreshTokenOAuth2AuthorizedClientProvider(),
                new PasswordOAuth2AuthorizedClientProvider()));

        Map<String, Object> passwordAttributes = new HashMap<>();
        passwordAttributes.put(OAuth2AuthorizationContext.USERNAME_ATTRIBUTE_NAME, "user");
        passwordAttributes.put(OAuth2AuthorizationContext.PASSWORD_ATTRIBUTE_NAME, "password");

        manager.setContextAttributesMapper(request -> passwordAttributes);

        ServletOAuth2AuthorizedClientExchangeFilterFunction oauth2 = new ServletOAuth2AuthorizedClientExchangeFilterFunction(manager);

        oauth2.setDefaultClientRegistrationId("oauth-service-to-service-client");

        return WebClient.builder()
                .filter(oauth2)
                .apply(oauth2.oauth2Configuration())
                .build();
    }

Version

5.2

Sample

[janzyka]

Few days earlier I created SO question on this topic: https://stackoverflow.com/questions/58604304/how-to-re-initialize-password-grant-in-spring-security-5-2-oauth

[jgrandja]

@janzyka This is an interesting use case. Can you provide more details of the error when the refresh_token grant fails on the provider?

As per spec, in 6. Refreshing an Access Token:

If the request failed
verification or is invalid, the authorization server returns an error
response as described in Section 5.2

If you look at Section 5.2, it seems to me that the provider would return invalid_grant for an expired refresh token?

don't make all classes final, it is difficult to add own behaviour. I remember nearly all classes in Spring used to be opened for extension.

Our design preference is "composition over inheritance". Can you be more specific on what you are looking for? Is there a specific class you would like to extend?

[janzyka]

@jgrandja First of all, thank you very much for following up! I really appreciate it.

Error on our proprietary corporate OAuth2 is HTTP 400:

{
    "error": "invalid_request",
    "error_description": "Error fetching token"
}

I agree this is not according to the spec - in this particular case it should be indeed invalid_grant. It may be tricky for me to fix though as I don't own the authorization server. .

Talking from a perspective of long running service with backend (@Scheduled) jobs: I think there should be some recovery mechanism which - if the refresh is failing - drops the authorized client from AuthorizedClientRepository and allows to start the auth from scratch (e.g. in my case start password grant from scratch and forget about refreshinig). Timeout is not the only thing which may happen btw. Maybe the authorization server has in-memory token store and was restarted?

This leads me to the second part of the question, final classes. I had 2 problems which could have been solved by extending existing PasswordOAuth2AuthorizedClientProvider. I had to copy-paste the class instead (In full honesty subclassing wouldn't really save me here but if the class has been written in different way it could have ).

1. I came across the bug with reverted skew sign
2. Because of the refresh token potential issues I wanted to disable refresh functionality. But my authorization server is still sending the refresh token. Now whenever PasswordOAuth2AuthorizedClientProvider sees refresh token in authorized client it will not attempt re-login. But what if RefreshTokenOAuth2AuthorizedClientProvider was not even registered? The PasswordOAuth2AuthorizedClientProvider may be the only provider ...

Generally subclassing allows me to:

• Fix small bugs without having to copy-paste/fork/wait for official release. I still want to use 99% of the class with some fix in place - inheritance usually works better here than composition
• Allows for tweaking the behaviour to fit concrete situation. Like disable refresh behaviour. Or maybe I will need in the future to tweak the refresh token timeouts functionality differently because our OAuth is returning bad error code (as above) and I don't have means to fix the root cause in authorization server.

Having said that - composition over inheritance is default choice if possible, no doubt about that.

[jgrandja]

@janzyka

I think there should be some recovery mechanism which - if the refresh is failing...

Agreed. I'll give this some thought and see if we can deliver an improvement here.

This leads me to the second part of the question, final classes. I had 2 problems which could have been solved by extending existing PasswordOAuth2AuthorizedClientProvider

As you may have noticed, PasswordOAuth2AuthorizedClientProvider and RefreshTokenOAuth2AuthorizedClientProvider both have minimal logic. They both check if the access token is expired accounting for clock skew and than just delegate to their associated OAuth2AccessTokenResponseClient. It's not clear to me how extending these classes would have saved you time. However, I'm totally open to improvement. I would welcome a PR if you're interested?

As already mentioned, I prefer composition over inheritance. It's very difficult to modify a public class without breaking existing external usages. It's much easier to plug-in a different strategy to change behaviour. It simply makes things easier on our end to manage the framework. Having said that, I'm certainly not opposed to inheritance. There just needs to be a valid case for this design choice. And I'm not seeing that with PasswordOAuth2AuthorizedClientProvider and RefreshTokenOAuth2AuthorizedClientProvider. However, as new features are introduced, it may make sense to open it up and remove the final modifier.

Because of the refresh token potential issues I wanted to disable refresh functionality

There is a way. You can provide a customized DefaultPasswordTokenResponseClient via PasswordOAuth2AuthorizedClientProvider.setAccessTokenResponseClient(). You would need to supply DefaultPasswordTokenResponseClient.setRestOperations() with a custom configured RestOperations that modifies the OAuth2AccessTokenResponse and excludes the refresh token returned by the provider. This can be done by configuring OAuth2AccessTokenResponseHttpMessageConverter.setTokenResponseConverter. The details are in the reference doc. This will allow you to move forward in the meantime.

Either way, I'll leave this issue open and think about some recovery mechanism that may be added as an improvement.

[janzyka]

Regarding disable refresh tokens. Ideally (to me) it would be as easy as not registering the RefreshTokenOAuth2AuthorizedClientProvider, this won't fly as DefaultPasswordTokenResponseClient acts as if there always was RefreshTokenOAuth2AuthorizedClientProvider.

My solution was as follows:

1. Copy the class (eh)
2. Add supportRefreshTokens field + constructor:

public MyCopiedPasswordOAuth2AuthorizedClientProvider() {
    this.supportRefreshTokens = true;
}

public MyCopiedPasswordOAuth2AuthorizedClientProvider(boolean supportRefreshTokens) {
    this.supportRefreshTokens = supportRefreshTokens;
}

3. Amend the return statement of authorize() method as:

@Override
@Nullable
public OAuth2AuthorizedClient authorize(OAuth2AuthorizationContext context) {
...
...
...
    return new OAuth2AuthorizedClient(clientRegistration, context.getPrincipal().getName(),
                tokenResponse.getAccessToken(), supportRefreshTokens ? tokenResponse.getRefreshToken() : null);
}

The effect is the same as you suggested but it feels a bit less complex to me.

Talking about inheritance, if the class were public, I would just extend it, call super.authorize() and conditionally copy the AuthorizedClient without copying the refresh token ...

If you would like me to turn this into PR I would be glad to do it.