Spring Security OAuth2: ServletOAuth2AuthorizedClientExchangeFilterFunction can fail to remove client if webclient receives retryable responses.

Describe the bug

In some configuration setups, adding a ServletOAuth2AuthorizedClientExchangeFilterFunction to a WebClient that can retry on authorization failures causes an IllegalArgumentException when trying to remove the authorized client in cases 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

• Execute sample repository according to the readme.md

Expected behavior

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

Sample

See Issue 17379 Sample

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).

Comment From: jjstreet

I've created a sample repository that illustrates the issue.