Spring Security OAuth2: ServletOAuth2AuthorizedClientExchangeFilterFunction fails to remove client webclient receives authentication failure responses

Describe the bug

In some configuration setups, adding a ServletOAuth2AuthorizedClientExchangeFilterFunction to a WebClient that can retry causes an IllegalArgumentException when trying to remove the client in the case where the WebClient receives a response actionable by the authorization failure forwarder. The ClientRequest is not always populated with a HttpServletRequest attribute when the filter is added via the WebClientBuilder.filter() method instead of using the the filter's filter.oauth2Configuration() method.

The filter.filter() method does merge the required objects into the ClientRequest when making an authorized client, but then does not use that merged request when executing the handler within the filter. Without the merged attributes, its impossible for the AuthorizationFailureForwarder to clean up the client in case there is a 401/403 response from the original ClientRequest.

To Reproduce * Create a blank MVC-based project * Create a WebClient * Attach ServletOAuth2AuthorizedClientExchangeFilterFunction to it via builder.filter() * Make sure the WebClient is set to retry on WebClientResponseExceptions for 401 or 403 responses. * Make sure the ServletOAuth2AuthorizedClientExchangeFilterFunction is able to retrieve a token successfully * Make sure the WebClient is able to call an endpoint that will fail with 401 or 403 even if the token is valid to simulate the situation. * WebClient should emit an IllegalArgumentException with "request must not be null" as a message.

Expected behavior

It is expected that the authorizedClient is removed via the AuthorizationFailureForwarder in the case a webclient with a ServletOAuth2AuthorizedClientExchangeFilterFunction.

Sample

No sample available yet. Working to extract a minimal setup from my own programs.

Comment From: jjstreet

@Override
    public Mono<ClientResponse> filter(ClientRequest request, ExchangeFunction next) {
        // @formatter:off
        return mergeRequestAttributesIfNecessary(request)
                .filter((req) -> req.attribute(OAUTH2_AUTHORIZED_CLIENT_ATTR_NAME).isPresent())
                .flatMap((req) -> reauthorizeClient(getOAuth2AuthorizedClient(req.attributes()), req))
                .switchIfEmpty(
                        Mono.defer(() ->
                            mergeRequestAttributesIfNecessary(request)
                                .filter((req) -> resolveClientRegistrationId(req) != null)
                                .flatMap((req) -> authorizeClient(resolveClientRegistrationId(req), req))
                        )
                )
                .map((authorizedClient) -> bearer(request, authorizedClient))
                .flatMap((requestWithBearer) -> exchangeAndHandleResponse(requestWithBearer, next))
                .switchIfEmpty(Mono.defer(() -> exchangeAndHandleResponse(request, next)));
        // @formatter:on
    }

The problem is the ClientRequest that is created by mergeRequestAttributesIfNecessary() is not passed to exchangeAndHandleResponse and thus, situations where the client must be cleaned up fail with an IllegalArgumentException

Comment From: jjstreet

By adjusting the filter methods as shown, the merged request is used by the exchangeAndHandleResponse methods. Then the client can be cleaned up properly.

@Override
    public Mono<ClientResponse> filter(ClientRequest request, ExchangeFunction next) {
        return mergeRequestAttributesIfNecessary(request)
                .flatMap(merged -> Mono.just(merged)
                        .filter((req) -> req.attribute(OAUTH2_AUTHORIZED_CLIENT_ATTR_NAME).isPresent())
                        .flatMap((req) -> reauthorizeClient(getOAuth2AuthorizedClient(req.attributes()), req))
                        .switchIfEmpty(
                                Mono.just(merged)
                                        .filter((req) -> resolveClientRegistrationId(req) != null)
                                        .flatMap((req) -> authorizeClient(resolveClientRegistrationId(req), req)))
                        .map((authorizedClient) -> bearer(merged, authorizedClient))
                        .flatMap((requestWithBearer) -> exchangeAndHandleResponse(requestWithBearer, next))
                        .switchIfEmpty(exchangeAndHandleResponse(merged, next)));
    }

As you can see, I flatMap on the merging so that I can use it in rest of the pipeline, which includes the the points where the exchange is handled.

Comment From: jjstreet

The mergeRequestAttributesIfNecessary() is what is confusing here, because if we use the convenience method to initially set up the filter on the webClient, it adds the the request/response objects to the client request attributes. There is no reason to do so again?

With the proposed fix, it might not be necessary to have the convenience method filter.oauth2Configuration(). It seems to be the only oauth2 filter function that has it, as the reactive variants do not. But I also do not know if there is any problem with the reactive variants either (though i doubt it, given there are no servlet requests objects to attach).