Spring Security OAuth2 WebClient - refresh token expiration is being ignored

Describe the bug I`m using Oauth2 WebClient to do some rest calls outside of ServerWebExchange scope. In my environment JWT refresh token do have an expiration date. And it seems like the expiration date is being ignored by the webclient/AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager. As a result the client wont automatically re-authenticate after the expiration of refresh token.

Configuration:

@Bean
public ReactiveClientRegistrationRepository getRegistration() {
 ClientRegistration registration = ClientRegistration
                .withRegistrationId("TEST")
                .tokenUri(tokeni)
                .clientId(clientid)
                .clientSecret(secret)
                .authorizationGrantType(AuthorizationGrantType.PASSWORD)
                .clientAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_POST)
                .build();
        return new InMemoryReactiveClientRegistrationRepository(registration);
}

@Bean
  public  WebClient webClient(ReactiveClientRegistrationRepository clientRegistrations) {
        InMemoryReactiveOAuth2AuthorizedClientService clientService = new InMemoryReactiveOAuth2AuthorizedClientService(clientRegistrations);

        AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager authorizedClientManager = new AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager(clientRegistrations, clientService);

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

        authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);
        authorizedClientManager.setContextAttributesMapper(oAuth2AuthorizeRequest -> Mono.just(Map.of(
                OAuth2AuthorizationContext.USERNAME_ATTRIBUTE_NAME, "xx",
                OAuth2AuthorizationContext.PASSWORD_ATTRIBUTE_NAME, "xx"
        )));

        ServerOAuth2AuthorizedClientExchangeFilterFunction oauth = new ServerOAuth2AuthorizedClientExchangeFilterFunction(authorizedClientManager);
        oauth.setDefaultClientRegistrationId("TEST");


        HttpClient httpClient = HttpClient
                .create()
                .wiretap("reactor.netty.http.client.HttpClient", LogLevel.DEBUG, AdvancedByteBufFormat.TEXTUAL);

        return WebClient.builder()
                        .baseUrl("xx")
                        .filter(oauth)
                        .clientConnector(new ReactorClientHttpConnector(httpClient))
                        .build();
}

This configuration works for refreshing access tokens, but once the refresh token itself expires the client just throws an exception.

org.springframework.security.oauth2.client.ClientAuthorizationException: [invalid_grant] Token is not active

    at org.springframework.security.oauth2.client.RefreshTokenReactiveOAuth2AuthorizedClientProvider.lambda$authorize$0(RefreshTokenReactiveOAuth2AuthorizedClientProvider.java:97)
    Suppressed: The stacktrace has been enhanced by Reactor, refer to additional information below: 
Error has been observed at the following site(s):
    *__checkpoint ⇢ Request to GET xxx
    [DefaultWebClient]
Original Stack Trace:
        at org.springframework.security.oauth2.client.RefreshTokenReactiveOAuth2AuthorizedClientProvider.lambda$authorize$0(RefreshTokenReactiveOAuth2AuthorizedClientProvider.java:97)
        at reactor.core.publisher.Mono.lambda$onErrorMap$27(Mono.java:3759)
        at reactor.core.publisher.Mono.lambda$onErrorResume$29(Mono.java:3849)
        at reactor.core.publisher.FluxOnErrorResume$ResumeSubscriber.onError(FluxOnErrorResume.java:94)
        at reactor.core.publisher.FluxHide$SuppressFuseableSubscriber.onError(FluxHide.java:142)

After some debugging i did find some problems (?) in implementation

1]

https://github.com/spring-projects/spring-security/blob/main/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AccessTokenResponse.java#L194C4-L194C92

The refresh token expiration is not being provided into the constructor of OAuth2RefreshToken

1]

https://github.com/spring-projects/spring-security/blob/main/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/PasswordReactiveOAuth2AuthorizedClientProvider.java#L101

I feel like this conditional should check the expiration of refresh token as well

if (authorizedClient != null && hasTokenExpired(authorizedClient.getAccessToken())
                && authorizedClient.getRefreshToken() != null) {

if (authorizedClient != null && hasTokenExpired(authorizedClient.getAccessToken())
                && (authorizedClient.getRefreshToken() != null && authorizedClient.getRefreshToken().getExpiresAt() != null && !hasTokenExpired(authorizedClient.getRefreshToken())) {

Expected behavior Once the refresh token expires the webclient should automatically re-authenticate.

Comment From: sjohnr

@FSichinger thanks for reaching out!

According to the OAuth 2.0 core spec (rfc 6749, section 5.1):

expires_in RECOMMENDED. The lifetime in seconds of the access token. For example, the value "3600" denotes that the access token will expire in one hour from the time the response was generated. If omitted, the authorization server SHOULD provide the expiration time via other means or document the default value.

The expiration is for the access token. There is not a separate expiration for the refresh token, and AFAICT the spec does not outline behavior regarding refresh token expiration.

If you wish to handle an expired refresh token, you can customize error handling for ClientAuthorizationException. If you have a question about how to do so, please open a question on Stack Overflow. We prefer to use GitHub issues only for bugs and enhancements. Feel free to update this issue with a link to the re-posted question (so that other people can find it).

I'm going to close this issue for now. If you feel I've misunderstood anything in your comment, please let me know.

Comment From: Xiphoseer

We also have issues with this behavior and maybe I can elaborate why I also think it's a bug. If you do the following:

• Use the standard ReactiveOAuth2AuthorizedClientProviderBuilder
• Call refreshToken and one of the standard flows on it (password, clientCredentials)
• Call build and use it with an AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager

Then the expectation is, that it will "just work", even for a long-running service. Which it doesn't and is probably the reason why DEFAULT_AUTHORIZED_CLIENT_PROVIDER does not call refreshToken.

---

Which means at least one of the ReactiveOAuth2AuthorizedClientProviders is bugged because:

1. I know I could implement a ReactiveOAuth2AuthorizedClientProvider that always succeeds (excl. network issues) because I can just re-authenticate every time.
2. I can't use the PasswordReactiveOAuth2AuthorizedClientProvider / password flow because of #8831, which returns Mono.empty() when it thinks refresh should have happend as long as the server sends a refresh_token.
3. So I either can't used the standard implementation (unfortunate) or need to add the refresh mechanism (which has this issue)
   • Which in my book is about RefreshTokenReactiveOAuth2AuthorizedClientProvider failing hard if the authentication server considers the token invalid (e.g. expired). This "breaks" the short-circuiting implicit to Flux::concatMap in DelegatingReactiveOAuth2AuthorizedClientProvider
   • The reply to this issue seems to indicate there's suppoed to be some sort of request retry mechanism outside the OAuth handling that really should not be needed in the first place (again, because we do have valid credentials available)
4. On top of that, there's the inconsistency with client_credentials (the other non-interactive flow), that only password has that weird check for getRefreshToken returning a (non-)null value.
5. And there's basically no documentation on the ordering that is used for ReactiveOAuth2AuthorizedClientProvider::build. Based on the LinkedHashMap documentation, this should be insertion-order so match the order of calls on the builder but that's not obvious from the external API. Which ties back to #8831 probably being this way to make an order of .password().refreshToken() use the refresh token at all.

Other notes:

In our case, the refresh token is from keycloak and a "normal" JWT. It has an exp claim, so it very much has an expiration date that is not the same as the expiration of the access_token

Comment From: sjohnr

@Xiphoseer if you believe you have found a bug, please consider providing a minimal, reproducible sample which in this case would Ideally be a test case with mock responses from an auth server.

• Call refreshToken and one of the standard flows on it (password, clientCredentials)

Please keep in mind the password grant support is deprecated in Spring Security and will not likely be enhanced. Also, refresh tokens are not valid for client_credentials which doesn't require a refresh mechanism. It seems possible you are expecting something that is not happening by design. Providing a test case will allow us to discover this efficiently.

Comment From: Xiphoseer

Hi @sjohnr, thanks for the reply

if you believe you have found a bug, please consider providing a minimal, reproducible sample which in this case would Ideally be a test case with mock responses from an auth server.

I created a self-contained JUnit Test class here: https://gist.github.com/Xiphoseer/4012f09d83e6b40525b6702b77166e9c. All these are written to succeed to demonstrate the behavior I mentioned.

From my POV the bug is that it's impossible to use the default builder with an authorization server that returns refresh_tokens which have an internal expiration date: - Can't use a plain PasswordReactiveOAuth2AuthorizedClientProvider because that returns Mono.empty() when a refresh_token is set in the authorized client, i.e. just stops working once the first token is expired. - Can't use a combination with RefreshTokenReactiveOAuth2AuthorizedClientProvider because that will try the refresh (independent of provider insertion order, because of that other Mono::empty return) when refresh_token is set, and hard fail if its expired, i.e. produces "random" errors without some sort of custom external request retry mechanism.

I consider this a bug because it depends much more on if the server returns a refresh_token than on what is configured in the builder. The solution is less clear though. My though process is that:

• The PasswordReactiveOAuth2AuthorizedClientProvider should ignore refresh tokens completely, it can't know whether the provider is configured, and might be considered a bug.
• The RefreshTokenReactiveOAuth2AuthorizedClientProvider should get a new flag that turns (some?) errors into "no result", this would not be a breaking change. (edit: this could be implemented as a generic wrapper that implements the provider interface).
• The ReactiveOAuth2AuthorizedClientProviderBuilder should warn if refreshToken is called after password, or remove and re-insert the latter. This might be reasonable to do because password is deprecated anyways.
• The DelegatingReactiveOAuth2AuthorizedClientProvider is already using insertion-order, which would be an advantage here.

Please keep in mind the password grant support is deprecated in Spring Security and will not likely be enhanced.

I'm aware of this, and we want to move away from it anyway, but this is a somewhat significant papercut in migrating software to spring(-security) while keeping existing observable behavior. We intend this as a first step to move to client_credentials later.

Also, refresh tokens are not valid for client_credentials which doesn't require a refresh mechanism. It seems possible you are expecting something that is not happening by design. Providing a test case will allow us to discover this efficiently.

Thank you for the pointer, I was indeed not aware of this. This does limit the issue to the password flow, which is where we encountered this.

Comment From: sjohnr

Thanks for the update @Xiphoseer. Unfortunately, I'm having a hard time following. Can you please update the test case to provide a failing test that you expect to succeed?

Comment From: Xiphoseer

Thanks for the update @Xiphoseer. Unfortunately, I'm having a hard time following. Can you please update the test case to provide a failing test that you expect to succeed?

Hi, revised the test case, so three of the cases fail now. The cases have in common that the "server" replies with a refresh_token: - When only the password flow is configured on the builder, the password provider backs off and doesn't provide any tokens anymore, after the first one expired. - When password and refreshToken flows are configured together, it will always hit the refresh token implementation and throw from the delegate provider if the refresh is rejected by the server, irrespective of the order of the two providers, for much the same reason.

Comment From: sjohnr

@Xiphoseer thank you for providing test cases that break down what you think should happen.

However, I'm not seeing you using the delegating ReactiveOAuth2AuthorizedClientProvider in the context of a ReactiveOAuth2AuthorizedClientManager. In that section, the docs state:

In the case of a re-authorization failure, eg. a refresh token is no longer valid, the previously saved OAuth2AuthorizedClient will be removed from the ServerOAuth2AuthorizedClientRepository via the RemoveAuthorizedClientReactiveOAuth2AuthorizationFailureHandler.

This is critical for your use case, as the refresh token has expired. Without this component detecting the failure and removing the authorized client, it will continue to fail. It is true that the first request once a refresh token expires will fail, and this is by design.

If this is unacceptable for your use case, you may wrap the authorizedClientManager.authorize(...) in retry logic. However, it will not likely succeed unless the user's credentials are available. This is why the password grant is deprecated (it encourages clients to keep user credentials around, which is bad practice). What should actually happen is the authorized client is removed, and the user must (on the next request) re-authorize the client by providing their credentials.

The following is a test case (based on yours) that works correctly and makes the correct expectations of the framework:

public class SpringSecurityIssue13915Tests {
    private static final ClientRegistration CLIENT_REGISTRATION = ClientRegistration.withRegistrationId("id")
        .authorizationGrantType(AuthorizationGrantType.PASSWORD)
        .clientId("156a4261-c1bc-47f1-b9ce-0b40b63cef1f")
        .tokenUri("https://example.com/token")
        .build();
    private static final String USERNAME = "user";
    private static final String PASSWORD = "password";
    private static final String REFRESH_TOKEN = "refresh";
    private static final long TOKEN_EXPIRES_IN_SECONDS = 60;
    private static final Authentication PRINCIPAL = new AnonymousAuthenticationToken(
        "anonymous", "anonymousUser", AuthorityUtils.createAuthorityList("ROLE_USER"));
    private static final OAuth2AuthorizeRequest AUTHORIZE_REQUEST = OAuth2AuthorizeRequest.withClientRegistrationId(CLIENT_REGISTRATION.getRegistrationId())
        .principal(PRINCIPAL)
        .attribute(USERNAME_ATTRIBUTE_NAME, USERNAME)
        .attribute(PASSWORD_ATTRIBUTE_NAME, PASSWORD)
        .build();

    private ReactiveClientRegistrationRepository clientRegistrationRepository;

    private ReactiveOAuth2AuthorizedClientService authorizedClientService;

    @BeforeEach
    public void setUp() {
        this.clientRegistrationRepository = new InMemoryReactiveClientRegistrationRepository(CLIENT_REGISTRATION);
        this.authorizedClientService = new InMemoryReactiveOAuth2AuthorizedClientService(this.clientRegistrationRepository);
    }

    @Test
    public void authorizeWhenPasswordAndRefreshGrantsUsedThenWorksAsDesigned() {
        MockClock mockClock = new MockClock();
        MockClient mockClient = new MockClient("a", REFRESH_TOKEN);

        // 1: create a password-then-refresh provider
        AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager authorizedClientManager =
            new AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager(
                this.clientRegistrationRepository, this.authorizedClientService);
        authorizedClientManager.setContextAttributesMapper((request) -> Mono.just(request.getAttributes()));

        ReactiveOAuth2AuthorizedClientProvider authorizedClientProvider =
            ReactiveOAuth2AuthorizedClientProviderBuilder.builder()
                .password(mockClient.password(mockClock))
                .refreshToken(mockClient.refreshToken(mockClock))
                .build();
        authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);

        Instant start = mockClock.instant();

        // 2: provider can authorize the client
        OAuth2AuthorizedClient authorizedClient1 = authorizedClientManager.authorize(AUTHORIZE_REQUEST).block();
        assertThat(authorizedClient1).isNotNull();
        assertThat(authorizedClient1.getAccessToken().getTokenValue()).isEqualTo("password-a");

        mockClient.nextAccessToken = "b";
        // 3: Before the token is expired...
        mockClock.setInstant(start.plusSeconds(TOKEN_EXPIRES_IN_SECONDS / 2));

        OAuth2AuthorizedClient authorizedClient2 = authorizedClientManager.authorize(AUTHORIZE_REQUEST).block();
        assertThat(authorizedClient2).isNotNull();
        assertThat(authorizedClient2).isEqualTo(authorizedClient1);

        mockClient.nextAccessToken = "c";
        // 4: After the token is expired...
        mockClock.setInstant(start.plusSeconds(TOKEN_EXPIRES_IN_SECONDS * 2));

        OAuth2AuthorizedClient authorizedClient3 = authorizedClientManager.authorize(AUTHORIZE_REQUEST).block();
        assertThat(authorizedClient3).isNotNull();
        assertThat(authorizedClient3.getAccessToken().getTokenValue()).isEqualTo("refresh_token-c");

        // ... but this may fail!
        mockClient.nextAccessToken = "d";
        mockClient.fail = AuthorizationGrantType.REFRESH_TOKEN;
        assertThatExceptionOfType(ClientAuthorizationException.class)
            .isThrownBy(() -> authorizedClientManager.authorize(AUTHORIZE_REQUEST).block())
            .withMessage("[invalid_grant] refresh_token flow failed");

        // ... so we try again
        OAuth2AuthorizedClient authorizedClient4 = authorizedClientManager.authorize(AUTHORIZE_REQUEST).block();
        assertThat(authorizedClient4).isNotNull();
        assertThat(authorizedClient4.getAccessToken().getTokenValue()).isEqualTo("password-d");
        assertThat(authorizedClient4.getRefreshToken().getTokenValue()).isEqualTo("refresh");
        // ... and get a fresh access_token and refresh_token
    }

    /**
     * Mock Authorization Server Responses
     */
    private static class MockClient {
        String nextAccessToken;
        String nextRefreshToken;
        AuthorizationGrantType fail = null;

        MockClient(String nextAccessToken, String nextRefreshToken) {
            this.nextAccessToken = nextAccessToken;
            this.nextRefreshToken = nextRefreshToken;
        }

        Consumer<ReactiveOAuth2AuthorizedClientProviderBuilder.PasswordGrantBuilder> password(MockClock mockClock) {
            return c -> c.accessTokenResponseClient(this::tokenResponse).clock(mockClock).clockSkew(Duration.ZERO);
        }

        Consumer<ReactiveOAuth2AuthorizedClientProviderBuilder.RefreshTokenGrantBuilder> refreshToken(MockClock mockClock) {
            return c -> c.accessTokenResponseClient(this::tokenResponse).clock(mockClock).clockSkew(Duration.ZERO);
        }

        public Mono<OAuth2AccessTokenResponse> tokenResponse(AbstractOAuth2AuthorizationGrantRequest request) {
            if (Objects.equals(request.getGrantType(), fail)) {
                OAuth2Error error = new OAuth2Error(INVALID_GRANT,  fail.getValue() + " flow failed", null);
                return Mono.error(new OAuth2AuthorizationException(error));
            } else {
                return Mono.just(OAuth2AccessTokenResponse
                    .withToken(request.getGrantType().getValue() + "-" + nextAccessToken)
                    .tokenType(BEARER)
                    .expiresIn(TOKEN_EXPIRES_IN_SECONDS)
                    .refreshToken(nextRefreshToken).build());
            }
        }
    }

    /**
     * Simple Mock Clock Implementation
     */
    private static class MockClock extends Clock {
        private final ZoneId zoneId;
        private Instant instant;

        MockClock(ZoneId zoneId, Instant instant) {
            this.zoneId = zoneId;
            this.instant = instant;
        }

        MockClock() {
            this(TimeZone.getDefault().toZoneId(), Instant.now());
        }

        @Override
        public ZoneId getZone() {
            return zoneId;
        }

        @Override
        public Clock withZone(ZoneId zone) {
            return new MockClock();
        }

        @Override
        public Instant instant() {
            return instant;
        }

        void setInstant(Instant instant) {
            this.instant = instant;
        }
    }
}

Comment From: Xiphoseer

Hi @sjohnr, thank you very much for taking the time to provide feedback. I'm aware of ReactiveOAuth2AuthorizedClientManager, as well as RemoveAuthorizedClientReactiveOAuth2AuthorizationFailureHandler. We're using that (specifically AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager) with a ServerOAuth2AuthorizedClientExchangeFilterFunction already.

Since ReactiveOAuth2AuthorizedClientManager has just a single method, I can indeed follow your suggestion and build something like

@AllArgsConstructor
private static final class RetryingOAuth2AuthorizedClientManager implements ReactiveOAuth2AuthorizedClientManager {
    private final ReactiveOAuth2AuthorizedClientManager delegate;

    @Override
    public Mono<OAuth2AuthorizedClient> authorize(OAuth2AuthorizeRequest authorizeRequest) {
        return delegate.authorize(authorizeRequest).onErrorResume(ClientAuthorizationException.class, e -> {
            if (INVALID_GRANT.equals(e.getError().getErrorCode())) {
                log.warn("Failed to authorize client, retrying once", e);
                return delegate.authorize(authorizeRequest);
            } else {
                return Mono.error(e);
            }
        });
    }
}

In general, I'm a bit sceptical of the RemoveAuthorizedClientReactiveOAuth2AuthorizationFailureHandler because it smells of TOCTOU, given that the calls to loadAuthorizedClient and removeAuthorizedClient on ReactiveOAuth2AuthorizedClientService are async and only use keys (principal and client registration), not the identity of the OAuth2AuthorizedClient and two concurrent authorization requests for the same principal may hit the authorizedClients map in InMemoryReactiveOAuth2AuthorizedClientService interleaved as far as I understand.

But this ticket is about potential bugs or papercuts in the implementation, you rightfully point to stack-overflow for general advice, so can you please elaborate on

This is critical for your use case, as the refresh token has expired. Without this component detecting the failure and removing the authorized client, it will continue to fail. It is true that the first request once a refresh token expires will fail, and this is by design.

My understanding is that the authorizationSuccessHandler will also replace the expired authorized client if the provider succeeds. I can see why the removal is necessary in the general case, but that's somewhat orthogonal to what the providers should be capable of in my books. Again, in particular the lack of configuration options between RefreshToken and Delegating provider to not short-circuit and the Password provider to do anything at all with an authorization context / authorized client that has a refresh token set.

A note on that: I actually tried updating spring-security to no have that check in the Password flow and the only thing that breaks is the test for that behavior in particular, none of the others seem to rely on it.

And yes, I realized I keep mixing this ticket with #8831, and that is because that one led me to introduce RefreshToken provider in the first place, because you get a 403 from the resource server for an expired token because the delegate provider with just a password provider does not throw, so the manager returns the old client.

Comment From: sjohnr

@Xiphoseer, I have already demonstrated how to solve the issue I believe you were bringing forward. If you feel you have found a bug, please produce a minimal sample demonstrating it. Please open a new issue to discuss it, as this issue is closed and will not be reopened. Please move any relevant comments from this issue to the new issue if you open one. If additional comments are added to this issue, I will to lock this thread so that this issue does not grow with unrelated comments and discussion.