`InMemory[Reactive]OAuth2AuthorizedClientService` does not support changes to the `ClientRegistration` at runtime

[kzander91]

Describe the bug
We're calling an API that requires JWTs for authentication. The JWTs are obtained from an authorization server using Client Credentials Grant.
The authorization server requires us to periodically rotate the client secret. We store the secret in a separate configuration service (where we can update it out-of-band) and we have implemented a custom ReactiveClientRegistrationRepository that retrieves the secret from there.

This setup works fine until the secret is changed: We noticed that our app keeps using the previous secret, even though our ReactiveClientRegistrationRepository is returning the new secret.

We have tracked this down to InMemoryReactiveOAuth2AuthorizedClientService#loadAuthorizedClient which internally uses our repository implementation:

spring-security/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/InMemoryReactiveOAuth2AuthorizedClientService.java

Lines 60 to 67 in 6adc3b3

	public <T extends OAuth2AuthorizedClient> Mono<T> loadAuthorizedClient(String clientRegistrationId,
	String principalName) {
	Assert.hasText(clientRegistrationId, "clientRegistrationId cannot be empty");
	Assert.hasText(principalName, "principalName cannot be empty");
	return (Mono<T>) this.clientRegistrationRepository.findByRegistrationId(clientRegistrationId)
	.map((clientRegistration) -> new OAuth2AuthorizedClientId(clientRegistrationId, principalName))
	.flatMap((identifier) -> Mono.justOrEmpty(this.authorizedClients.get(identifier)));
	}

Note how the ClientRegistration retrieved from our clientRegistrationRepository (containing the current secret) is effectively ignored in favor of the ClientRegistration cached in the authorizedClients map (as a member of OAuth2AuthorizedClient). This means that the client secret that was current at the time of the first token request is used forever, even if the ClientRegistration changes afterwards.
After we rotated the secret, the authorization server rejects the previous one, resulting in all token requests to fail shortly after a secret rotation.

Expected behavior
The InMemoryReactiveOAuth2AuthorizedClientService should consider the fact that the ClientRegistration may change at runtime and thus not ignore it after fetching it from the registration repository.

Workaround

• First quick workaround was to reboot our app after every secret rotation.
• Next, we implemented our own ReactiveOAuth2AuthorizedClientService that is basically a copy of InMemoryReactiveOAuth2AuthorizedClientService, but with the following modification to loadAuthorizedClient:

return this.clientRegistrationRepository.findByRegistrationId(clientRegistrationId)
  .mapNotNull(clientRegistration -> {
    OAuth2AuthorizedClientId id = new OAuth2AuthorizedClientId(clientRegistrationId, principalName);
    OAuth2AuthorizedClient cachedAuthorizedClient = this.authorizedClients.get(id);
    if (cachedAuthorizedClient == null) {
      return null;
    }
    // Use current registration here  vvvvvvvvvvvvvvvvvv
    return new OAuth2AuthorizedClient(clientRegistration, cachedAuthorizedClient.getPrincipalName(), cachedAuthorizedClient.getAccessToken(), cachedAuthorizedClient.getRefreshToken());
  });

Note that the non-reactive version has the same behaviour, so this isn't specific to the reactive implementation.

---

Is there a specific reason for why the service behaves that way? What would be the recommended way to implement an OAuth2 client with secrets that can change at runtime?

The R2DBC implementation for example does behave as expected and always returns the ClientRegistration from the repository, see here:

spring-security/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/R2dbcReactiveOAuth2AuthorizedClientService.java

Lines 150 to 156 in 6adc3b3

	private Mono<OAuth2AuthorizedClient> getAuthorizedClient(OAuth2AuthorizedClientHolder authorizedClientHolder) {
	return this.clientRegistrationRepository.findByRegistrationId(authorizedClientHolder.getClientRegistrationId())
	.switchIfEmpty(Mono.error(dataRetrievalFailureException(authorizedClientHolder.getClientRegistrationId())))
	.map((clientRegistration) -> new OAuth2AuthorizedClient(clientRegistration,
	authorizedClientHolder.getPrincipalName(), authorizedClientHolder.getAccessToken(),
	authorizedClientHolder.getRefreshToken()));
	}

[sjohnr]

Hi @kzander91, thanks for the report. Apologies on the delay, I'm just going through some issues that we haven't been able to get to until now.

We have tracked this down to InMemoryReactiveOAuth2AuthorizedClientService#loadAuthorizedClient which internally uses our repository implementation:

The InMemoryReactiveOAuth2AuthorizedClientService implementation (and in-memory implementations in general) is not recommended for production use. This is a general guideline, but I don't see it specifically stated anywhere in our documentation so it's understandable that you might not be aware.

Typically, you would want to use R2dbcReactiveOAuth2AuthorizedClientService (which doesn't have this issue) or a custom implementation anyway. Before going any further, I want to make sure there's not a specific reason you chose to keep using the in-memory one?

[kzander91]

Hi @sjohnr, no worries!

I am aware that the in-memory implementations of various beans aren't recommended for production use, but my interpretation of the reason behind that was always that there's no mechanism to limit the size of the map, resulting in potential OOMEs. Are there other reasons for it not being recommended for production?

In my use case, the map will only ever contain a single mapping, because the user in the SecurityContext will always be the same, single user, and API calls will only ever be made to the same endpoint with bearer tokens obtained from the same IDP. Thus, the complexity of setting up a database for that (plus the additional latency for reading from/writing to the DB) just isn't worth it and the in-memory variant should be perfectly suitable for that use case.

[sjohnr]

Are there other reasons for it not being recommended for production?

The in-memory implementations are just not designed for production use. Any issues they have would be specific to their implementation.

Having said that, we still want to fix bugs that are obviously incorrect (and not directly related to being in memory), so I think the bug you mention here could be fixed since the r2dbc version doesn't have the same issue. Would you be interested in sending a PR?

[kzander91]

@sjohnr sure I'll give it a shot! Would you say that the workaround I showed above was a suitable fix? Then I'll try submitting that as a PR.

[sjohnr]

Would you say that the workaround I showed above was a suitable fix?

Yes, I think so! Though I would find it preferable to use a fluent return value by nesting flatMap:

// @formatter:off
return (Mono<T>) this.clientRegistrationRepository.findByRegistrationId(clientRegistrationId)
	.flatMap((clientRegistration) -> Mono.just(new OAuth2AuthorizedClientId(clientRegistrationId, principalName))
		.mapNotNull(this.authorizedClients::get)
		.map((authorizedClient) -> new OAuth2AuthorizedClient(clientRegistration,
			authorizedClient.getPrincipalName(),
			authorizedClient.getAccessToken(),
			authorizedClient.getRefreshToken()))
	);
// @formatter:on