Introduce Reactive OAuth2Authorization success/failure handlers

[philsttr]

Summary

When an OAuth2 Client sends a token to a downstream OAuth2 Resource Server, and the OAuth2 Resource Server returns an HTTP 401 response, I would like to prevent the OAuth2 Client from sending that same token in future requests (and instead retrieve a new token for future requests).

Currently, spring-security-oauth2-client provides good support for re-retrieving/refreshing tokens before they expire. However, spring-security-oauth2-client does not provide good support for re-retrieving/refreshing access tokens when they have become invalid for reasons other than expiration. A good signal that a token is no longer valid is when an OAuth2 Resource Server returns an HTTP 401 response to the client.

Both ServerOAuth2AuthorizedClientRepository and ReactiveOAuth2AuthorizedClientService provide a removeAuthorizedClient method to remove the cached OAuth2AuthorizedClient. It would be nice if spring-security-oauth2-client provided something that actually called that method when an HTTP 401 response is received from an OAuth2 Resource Server.

A possible implementation might be:

1. the DefaultReactiveOAuth2AuthorizedClientManager (and the future AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager from Reactive Implementation of AuthorizedClientServiceOAuth2AuthorizedCli… #7589) could put a Mono<Void> in the subscriber context that will remove the token from the cache when subscribed
   • DefaultReactiveOAuth2AuthorizedClientManager would put a Mono<Void> that invokes its ServerOAuth2AuthorizedClientRepository.removeAuthorizedClient
   • AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager would put a Mono<Void> that invokes its ReactiveOAuth2AuthorizedClientService.removeAuthorizedClient
2. A new ExchangeFilterFunction (or one of the existing ones) checks for HTTP 401 responses, and chains the Mono<Void> from step 1 if it exists
   • usage of the subscriber context would decouple the ExchangeFilterFunction from the implementation of how to remove the cached authorized client (i.e. separate filter function implementations would not be needed depending on whether a ServerOAuth2AuthorizedClientRepository or ReactiveOAuth2AuthorizedClientService is being used to persist OAuth2AuthorizedClient s)
   • It would also allow other places downstream (e.g. non-WebClient related functionality, like WebSocketClient) to potentially invalidate the authorized client, without direct knowledge of exactly how it is persisted, other than just subscribing to a Mono<Void>)

(After looking at this a bit more, I realize the above approach probably won't work, since anything put into the subscriber context by a ReactiveOAuth2AuthorizedClientManager wouldn't actually be visible downstream in an ExchangeFilterFunction . So, something else is needed. Perhaps something in ServerOAuth2AuthorizedClientExchangeFilterFunction)

Actual Behavior

spring-security-oauth2-client continues to reuse an OAuth2 token, even after an OAuth2 Resource server has returned a 401 response, which usually indicates the token is invalid.

Expected Behavior

spring-security-oauth2-client removes the cached token after an OAuth2 Resource server has returned a 401 response. Future requests will trigger a re-retrieval of a new token.

Configuration

• spring-security-oauth2-client
• using either DefaultReactiveOAuth2AuthorizedClientManager or AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager from Reactive Implementation of AuthorizedClientServiceOAuth2AuthorizedCli… #7589
• WebClient or WebSocketClient usage downstream

Version

5.2.1.RELEASE

[jgrandja]

@philsttr This sounds like a valuable enhancement. Would you be interested in submitting a PR for this?

[philsttr]

Yeah, I could tackle the implementation, once we settle on an approach. I'd like your input on what approach to take, before I get started.

My current thinking is to:

1. expose a new method in ReactiveOAuth2AuthorizedClientManager for invalidating an OAuth2AuthorizedClient. (named invalidate ?)
   • This would have to be a default method for backwards compatibility, with a default implementation that just returns Mono.empty().
   • DefaultReactiveOAuth2AuthorizedClientManager would delegate to its ServerOAuth2AuthorizedClientRepository.removeAuthorizedClient
   • AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager would delegate to its ReactiveOAuth2AuthorizedClientService.removeAuthorizedClient
2. have ServerOAuth2AuthorizedClientExchangeFilterFunction look for HTTP 401 responses, and invoke the new invalidate method on ReactiveOAuth2AuthorizedClientManager.

We could also combine this approach somewhat with the approach I mentioned in the issue description, where ServerOAuth2AuthorizedClientExchangeFilterFunction would put a Mono<Void> into subscriber context, and then anything downstream could subscribe to that mono to perform the invalidation. (e.g. another optional filter function, or whatever) This would be a little more complex, but more flexible.

Your thoughts?

(It would also be great if #7702 was merged before I start on this, so I can modify AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager)

[jgrandja]

@philsttr I'm not sure if ReactiveOAuth2AuthorizedClientManager.invalidate() makes sense.

I'm leaning towards introducing 2 new strategies similar to ServerAuthenticationSuccessHandler and ServerAuthenticationFailureHandler, which is used by AuthenticationWebFilter. This design would align the authorization flow with the authentication flow to provide some consistency.

For example:

public interface ServerOAuth2AuthorizationSuccessHandler {
	Mono<Void> onAuthorizationSuccess(OAuth2AuthorizedClient authorizedClient, ...);
}

public interface ServerOAuth2AuthorizationFailureHandler {
	Mono<Void> onAuthorizationFailure(OAuth2AuthorizationException exception, ...);
}

We could consider extending OAuth2AuthorizationException, similar to ClientAuthorizationRequiredException, that captures the details the ServerOAuth2AuthorizationFailureHandler requires to do it's thing, eg. remove the OAuth2AuthorizedClient from the repository. However, I think I would keep OAuth2AuthorizationException as the type parameter.

The default implementation of ServerOAuth2AuthorizationSuccessHandler could simply do what DefaultReactiveOAuth2AuthorizedClientManager.saveAuthorizedClient() does today.

Thoughts?

[jgrandja]

Related #7583

[philsttr]

A few questions/comments:

What would have a reference to the handlers, detect authentication success/failure, and call the handlers? ServerOAuth2AuthorizedClientExchangeFilterFunction?

Since ServerOAuth2AuthorizedClientExchangeFilterFunction has a ReactiveOAuth2AuthorizedClientManager (either a DefaultReactiveOAuth2AuthorizedClientManager or AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager), are you saying there would be two implementations of the failure handler... one would call ServerOAuth2AuthorizedClientRepository.removeAuthorizedClient and the other would call ReactiveOAuth2AuthorizedClientService.removeAuthorizedClient ?

This is where I was thinking that ReactiveOAuth2AuthorizedClientManager.invalidate would be good... so that the ReactiveOAuth2AuthorizedClientManager implementation could decide how to handle the invalidation. The main complexity that I'm trying to solve here is that there are two possible ways to remove cached authorized clients (depending on which ReactiveOAuth2AuthorizedClientManager is being used):

1. DefaultReactiveOAuth2AuthorizedClientManager
   -> ServerOAuth2AuthorizedClientRepository.removeAuthorizedClient
2. AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager -> ReactiveOAuth2AuthorizedClientService.removeAuthorizedClient

Also, is "authorization" is the proper term here? I was thinking "authentication" is more correct. When the token is no longer valid, that is an authentication failure and a 401 returned. If the token was valid, but did not have the proper scopes, that would be an authorization failure, and a 403 would be returned. The case being handled here is the former.

[jgrandja]

What would have a reference to the handlers

The ReactiveOAuth2AuthorizedClientManager implementations would have these handlers with associated setters so applications can customize. This would handle the following cases:

1. Client is not authorized and the authorization request fails, eg. client requests a scope that they are not allowed to request.
2. Client is authorized but the token is expired and the refresh token request fails, eg. refresh token is not valid anymore RefreshTokenOAuth2AuthorizedClientProvider does not handle expired refresh token #7583
3. Client is authorized but the token is expired and the token renewal request fails, eg. client_credentials client had it's credentials leaked so it was rotated on provider

We also would need to handle 401 errors in ServerOAuth2AuthorizedClientExchangeFilterFunction so I believe we would need to reuse the failure handler here to. Assuming the OAuth2AuthorizedClient is returned from the manager, the WebClient will initiate the request but it fails with 401 as a result of the token being revoked on provider.

are you saying there would be two implementations of the failure handler... one would call ServerOAuth2AuthorizedClientRepository.removeAuthorizedClient and the other would call ReactiveOAuth2AuthorizedClientService.removeAuthorizedClient

Yes, there would need to be two since they call different collaborators.

Another advantage of having these handlers is that user's can customize the behaviour (via setters) above the standard behaviour of simply removing the authorized client from the repository/service.

Also, is "authorization" is the proper term here? I was thinking "authentication" is more correct.

Yes, authentication is more correct when a 401 occurs during a protected resource request because the token is no longer valid (expired/revoked). However, what about the cases when the client attempts to obtain a new token or renew/refresh an existing expired token? For these cases the authorization fails for the client and typically a 400 is returned as per spec. I'm going with the term authorization to suit all cases. Makes sense?

[philsttr]

Ok, I think I'm following you now. I was only thinking about using these handlers when communicating with a resource server. But you make an excellent point that these same handlers could be used when communicating with the authorization server to retrieve/refresh a token (your cases 1-3)

We also would need to handle 401 errors in ServerOAuth2AuthorizedClientExchangeFilterFunction so I believe we would need to reuse the failure handler here too.

Yeah, this was the original use case for this issue. I'd like to have ServerOAuth2AuthorizedClientExchangeFilterFunction 's default behavior be to invalidate the client when it receives a 401. In order to do that, what do you think about:

• making ReactiveOAuth2AuthorizedClientManager extend ServerOAuth2AuthorizationFailureHandler, and provide a default implementation of onAuthorizationFailure that does nothing
• modify DefaultReactiveOAuth2AuthorizedClientManager and AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager to delegate the onAuthorizationFailure call to their handlers (which default to removing the authorized client appropriately)
• Have ServerOAuth2AuthorizedClientExchangeFilterFunction's default failure handler call clientManager.onAuthenticationFailure on receiving a 401 response from the resource server

Without doing the above, I don't see how I can make ServerOAuth2AuthorizedClientExchangeFilterFunction 's default behavior be to invalidate the client when it receives a 401. I could make the default behavior of ServerOAuth2AuthorizedClientExchangeFilterFunction do nothing on a 401, and therefore require the user explicitly configure a failure handler based on which type of client manager is being used, but that is not a good out-of-the-box experience for such a common use case IMHO.

authentication vs authorization

Fair point. I'm on board with using "authorization" since we're also now including use cases for handling responses from the authorization server. I was originally only looking at the use case for handling responses from a resource server.

[jgrandja]

what do you think about: making ReactiveOAuth2AuthorizedClientManager extends ServerOAuth2AuthorizationFailureHandler

I'd rather keep the ReactiveOAuth2AuthorizedClientManager contract minimal (functional). As well, I see the handling of errors an implementation detail so I feel it makes sense to define this in the implementation rather than the interface. We currently provide setters for these type of handlers in AuthenticationWebFilter or AbstractAuthenticationProcessingFilter so we would follow the same pattern in the 2x ReactiveOAuth2AuthorizedClientManager implementations.

Without doing the above, I don't see how I can make ServerOAuth2AuthorizedClientExchangeFilterFunction 's default behavior be to invalidate the client when it receives a 401

The default ReactiveOAuth2AuthorizedClientManager is DefaultReactiveOAuth2AuthorizedClientManager via ServerOAuth2AuthorizedClientExchangeFilterFunction.createDefaultAuthorizedClientManager(). Could we not update createDefaultAuthorizedClientManager() with defaultReactiveOAuth2AuthorizedClientManager.setAuthorizationFailureHandler()?

If the application uses the constructor ServerOAuth2AuthorizedClientExchangeFilterFunction(ReactiveOAuth2AuthorizedClientManager authorizedClientManager), than the application will be responsible for configuring the ServerOAuth2AuthorizationFailureHandler.

[philsttr]

Ok, that works for me. I think I have enough to start on an implementation. I have a couple other things on my plate, but should be able to start soon.

[jgrandja]

Sounds good and thanks for all your great work @philsttr

[philsttr]

Next observations after looking deeper into this:

1. It seems like this change also needs to be made in non-reactive environments. I'll take a look at this as well as part of this issue.

2. I'm trying to figure out where to get the values to pass as the arguments needed by the *OAuth2AuthorizedClientRepository/Service save/removeAuthorizedClient methods.

Specifically:

• Authentication principal (or String principalName) is needed by all save/removeAuthorizedClient methods
• ServerWebExchange serverWebExchange is needed by ServerOAuth2AuthorizedClientRepository save/removeAuthorizedClient
• HttpServletRequest request and HttpServletResponse response are needed by OAuth2AuthorizedClientRepository save/removeAuthorizedClient

For Authentication principal, the success/failure handler methods could take a Authentication principal argument, since all the locations that would invoke the handler have access to the principal.
(Note that OAuth2AuthorizedClient only has a principalName, not the whole principal, so the handler can't just retrieve the principal from the OAuth2AuthorizedClient)
e.g.:

public interface ServerOAuth2AuthorizationSuccessHandler {
	Mono<Void> onAuthorizationSuccess(OAuth2AuthorizedClient authorizedClient, Authentication principal);
}

public interface ServerOAuth2AuthorizationFailureHandler {
	Mono<Void> onAuthorizationFailure(OAuth2AuthorizationException authorizationException, Authentication principal);
}

public interface OAuth2AuthorizationSuccessHandler {
	void onAuthorizationSuccess(OAuth2AuthorizedClient authorizedClient, Authentication principal);
}

public interface OAuth2AuthorizationFailureHandler {
	void onAuthorizationFailure(OAuth2AuthorizationException authorizationException, Authentication principal);
}

For ServerWebExchange and HttpServletRequest/Response it's not so easy. I'd like to not add them as handler method args, because they do not apply when *OAuth2AuthorizedClientService is being used (i.e. OAuth2AuthorizedClientRepository is not being used). So that means I need to get them from somewhere else. The handler could get the ServerWebExchange from the subscriber context. But unfortunately, it might not be in the context (e.g. DefaultReactiveOAuth2AuthorizedClientManager pulls it from an authorize request attribute inside authorize, and the handler doesn't have access to the authorize request). And the same situation occurs for non-reactive environments as well (e.g. The handler could get them from thread locals, but they might not be there).

So, now I'm back to just including ServerWebExchange or HttpServletRequest/Response as an arg to the handler. They would have to be nullable though. I don't really want to create two sets of handlers (one with them, and one without them).

Do you have any recommendations here?

3. I think that the ServerOAuth2AuthorizedClientExchangeFilterFunction would only need/call a ServerOAuth2AuthorizationFailureHandler. It wouldn't need/call a success handler. Do you agree?

4. Regarding the use of OAuth2AuthorizationException in the failure handler.

OAuth2AuthorizationException requires a non-null OAuth2Error. Unfortunately, OAuth2Error only makes sense when communicating with the authorization server (not a resource server), because it is explicitly for modeling the values returned in the authorization server response.

If we use OAuth2AuthorizationException for authentication errors from a resource server, it seems odd and not the best choice.

5. Where to create and handle the exceptions

Given the changes in 2. above, the only additional thing the exception would need is the clientRegistrationId.

I'm trying to figure out where the exception would actually be created. My current thoughts are:

• When communicating to the authorization server, the OAuth2AccessTokenResponseClient/ReactiveOAuth2AccessTokenResponseClient would "throw" this new exception, and it would be "caught" by the client manager and passed to its the failure handler
• When communicating to the resource server, the ServletOAuth2AuthorizedClientExchangeFilterFunction and ServerOAuth2AuthorizedClientExchangeFilterFunction would look for 401 responses and create the exception to pass to its failure handler

Does that make sense?

In any case, thank you for your insights and patience as I look through this. I'm just trying to nail down the design before I bang my head against the wall on implementation.

[jgrandja]

(1)

It seems like this change also needs to be made in non-reactive environments

Yes it does. I was planning on adding another ticket for the Servlet implementation. However, it would be ideal to have both merged together. Thanks for looking into this as well !

(2)

For Authentication principal, the success/failure handler methods could take a Authentication principal argument

Agreed. We should pass in the Authentication instead of String for principal

For ServerWebExchange and HttpServletRequest/Response it's not so easy. I'd like to not add them as handler method args, because they do not apply when OAuth2AuthorizedClientService is being used

I feel we need to pass in all required arguments into the handler. We just need to figure out the best way to do this from an API viewpoint.

The handler could get the ServerWebExchange from the subscriber context. But unfortunately, it might not be in the context (e.g. DefaultReactiveOAuth2AuthorizedClientManager pulls it from an authorize request attribute inside authorize, and the handler doesn't have access to the authorize request)

The handler doesn't need access to the authorize request. The DefaultReactiveOAuth2AuthorizedClientManager will resolve the ServerWebExchange from either OAuth2AuthorizeRequest or the subscriber context so it will be able to pass it to the handler.

So, now I'm back to just including ServerWebExchange or HttpServletRequest/Response as an arg to the handler. They would have to be nullable though. I don't really want to create two sets of handlers (one with them, and one without them).

Yeah, we definitely don't want to create 2 sets of handlers. If we look at the design so far for reactive we have this:

package org.springframework.security.oauth2.client.web.server;

public interface ServerOAuth2AuthorizationSuccessHandler {

	Mono<Void> onAuthorizationSuccess(OAuth2AuthorizedClient authorizedClient,
						Authentication principal,
						@Nullable ServerWebExchange exchange);
}

public interface ServerOAuth2AuthorizationFailureHandler {

	Mono<Void> onAuthorizationFailure(OAuth2AuthorizationException exception,
						Authentication principal,
						@Nullable ServerWebExchange exchange);
}

I'm not sure if I'm keen on the @Nullable ServerWebExchange exchange, but we do need it for "inside-request-context" and don't need it for "outside-request-context". And we want to avoid creating 2 sets of interfaces as well.

I'm wondering if we could borrow the pattern used for OAuth2AuthorizeRequest.attributes?

Let's say we have this design instead:

package org.springframework.security.oauth2.client;

public interface ReactiveOAuth2AuthorizationSuccessHandler {

	Mono<Void> onAuthorizationSuccess(OAuth2AuthorizationSuccessEvent successEvent);
}

public interface ReactiveOAuth2AuthorizationFailureHandler {

	Mono<Void> onAuthorizationFailure(OAuth2AuthorizationFailureEvent failureEvent);
}

public final class OAuth2AuthorizationSuccessEvent {
	private OAuth2AuthorizedClient authorizedClient;
	private Authentication principal;
	private Map<String, Object> attributes;

...
}

public final class OAuth2AuthorizationFailureEvent {
	private OAuth2AuthorizationException exception;
	private Authentication principal;
	private Map<String, Object> attributes;

...
}

NOTE: I'm not convinced on the suffix naming *Event, but it's what I got now.

The Map<String, Object> attributes could be populated with ServerWebExchange or not at all depending on whether the callee requires it. The caller will have the knowledge on how to construct the event object before it calls the handler.

This pattern worked well for OAuth2AuthorizeRequest and kept the ReactiveOAuth2AuthorizedClientManager.authorize() pretty clean.

What are your thoughts on this approach?

(3)

I think that the ServerOAuth2AuthorizedClientExchangeFilterFunction would only need/call a ServerOAuth2AuthorizationFailureHandler. It wouldn't need/call a success handler. Do you agree?

Yes I agree. However, it might be strange not to have symmetry here. For now, I would configure the failure handler only but I'm not sure we need to expose a setter during first iteration. Let's keep it internal for now and see how it works out.

(4)

Unfortunately, OAuth2Error only makes sense when communicating with the authorization server (not a resource server), because it is explicitly for modeling the values returned in the authorization server response.

Take a look at "Bearer Token Usage: Error Codes":

When a request fails, the resource server responds using the
appropriate HTTP status code (typically, 400, 401, 403, or 405)...

So the resource server may respond with other status codes than 401. The way I'm looking at this is we're dealing with OAuth 2.0 protected resources which is all about authorization. OpenID Connect is all about authentication. I agree that a 401 returned from a resource server because of a invalid (expired) token may look strange if we're handling OAuth2AuthorizationException, but it does make sense for 403 or 400 which could also be returned.

FYI, the base exception OAuth2AuthenticationException is meant to be used for oauth2Login() flows (eg. OIDC). Whereas the base exception OAuth2AuthorizationException is meant to be used for standard OAuth 2.0 flows.

Does this clear things up or do you still have some concerns?

(5)

When communicating to the authorization server, the OAuth2AccessTokenResponseClient/ReactiveOAuth2AccessTokenResponseClient would "throw" this new exception, and it would be "caught" by the client manager and passed to its the failure handler

Yes, this is the way I see it being handled as well. FYI, most (if not all) *TokenResponseClient are already handling and throwing OAuth2AuthorizationException.

When communicating to the resource server, the ServletOAuth2AuthorizedClientExchangeFilterFunction and ServerOAuth2AuthorizedClientExchangeFilterFunction would look for 401 responses and create the exception to pass to its failure handler

Yes but you'll have to look for more than just 401, as indicated by "Bearer Token Usage: Error Codes".

Great questions and sorry for the long response. But I think we're really close on design now.

[philsttr]

(1) 👍

(2) I like the introduction of *Event with attributes. Excellent idea.

(3) 👍

(4) Ok, yeah, after looking at the rfc, I can see now how an OAuth2Error makes sense when handling responses from a resource server. I'll introduce new constants for invalid_token and insufficient_scope in OAuth2ErrorCodes as part of this change.

(5) 👍

(6)
The next (hopefully final) piece of the puzzle is how the clientRegistrationId is retrieved by the handler for failures.

I can see two options for clientRegistrationId:
A) Include it in an exception (whatever throws the exception must know it)
B) Include it in an Event attribute (whatever invokes the handler must know it)

For A, I see there is already a ClientAuthorizationRequiredException subclass that has a clientRegistrationId. Would it make sense to pull clientRegistrationId up to OAuth2AuthorizationException? (most, but not all, places that construct a OAuth2AuthorizationException have access to the clientRegistrationId)

Or maybe introduce a new class between OAuth2AuthorizationException and ClientAuthorizationRequiredException that has a clientRegistrationId? (not sure what this would be named though)

Or introduce one or more new subclasses of OAuth2AuthorizationException that have a clientRegistrationId?

For B, it would be fairly simple, since all places invoking the handler have access to the clientRegistrationId

I don't have a strong opinion on this, so I'd like to defer to you as a maintainer of the library.

[jgrandja]

(6)
I feel option A makes the most sense. I would introduce a new exception named ClientAuthorizationException. This exception would serve as the base exception for all authorization related errors for client. The new hierarchy would be:

OAuth2AuthorizationException <- ClientAuthorizationException <- ClientAuthorizationRequiredException

We could than push ClientAuthorizationRequiredException.clientRegistrationId up to ClientAuthorizationException.

How does that sound?

Just a heads up that I'll be off for the holidays between Dec 23 - Jan 3 (inclusive). And next week I'm pretty swamped as well. I will be available next week but just wanted to let you know so you are not rushing to implement this feature :) Thanks again for taking this on. It will be a valuable add to client.

[philsttr]

I made a first pass at introducing reactive success/failure handlers today.

See the https://github.com/philsttr/spring-security/tree/gh-7699 branch for work-in-progress.

I have lots of unit tests to write, but the basics are there.

[jgrandja]

That is an excellent first pass @philsttr ! I'm excited about this new feature ! Thank you for your great work 👍

[jgrandja]

@philsttr To make things easier during the review process, let's focus on the reactive implementation first and submit in it's own PR. After it's merged, we can switch to the Servlet implementation.