spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.5k stars 5.78k forks source link

Add support for requesting protected resources with RestClient #13588

Open mjeffrey opened 10 months ago

mjeffrey commented 10 months ago

Expected Behavior Allow the use RestClient (to be introduced in Spring 6.1) for blocking calls in a non reactive application In Oauth2 Client. See https://spring.io/blog/2023/07/13/new-in-spring-6-1-restclient.

Current Behavior Only WebClient is supported which means a lot of reactive dependencies are pulled in when using Oauth2 Client even in a blocking application.

Context To avoid the dependency issue we are using RestTemplate (with interceptors) but would prefer to see a supported solution.

sjohnr commented 10 months ago

@mjeffrey thanks for starting the conversation on this!

To clarify, are you specifically directing this issue toward providing support for RestClient in OAuth2 equivalent to the existing ServletOAuth2AuthorizedClientExchangeFilterFunction which supports WebClient?

mjeffrey commented 10 months ago

@sjohnr I don't want to ask for too much related to implementation, you guys know this much better than me but I think what you said would do what is needed for us.

What I'd like to achieve is to be able to use the new RestClient with the Oauth2 client and to not need the reactive libraries.

In my company we are in the process of migrating a number of Spring Boot 2 (keycloak Oauth2 client) to Spring Boot 3 projects. The Keycloak client no longer supports Spring Boot 3 and so we are moving to Spring Security Oauth2 client. However the requirement to use WebClient is one of the things holding some teams back.

sjohnr commented 10 months ago

Ok, I've changed the title of the issue to what I think aligns with the request as a starting point, and we can go from there. Since the only part of OAuth2 Client for a Servlet environment that directly uses reactive libraries is ServletOAuth2AuthorizedClientExchangeFilterFunction, this issue can be used to provide equivalent support.

alexcrownus commented 10 months ago

I was about to create a similar issue, glad someone already did. Many thanks @mjeffrey.

Since support for RestTemplate was dropped because it is now in maintenance mode, there should now be support for the new RestClient for Servlet applications without using WebClient in blocking mode.

tudburley commented 10 months ago

I am maintaining a library, which provides a blocking WebClient with OAuth2 client credentials support and also want to migrate to the new RestClient. At the moment the OAuth2 support in this library is built with ServerOAuth2AuthorizedClientExchangeFilterFunction.

Will there be also non-reactive equivalents for ServerOAuth2AuthorizedClientExchangeFilterFunction?

sjohnr commented 10 months ago

@tudburley we already have ServletOAuth2AuthorizedClientExchangeFilterFunction, is that what you're asking about? Otherwise, I'm not sure I understand the question. If you have a different enhancement suggestion than what is being discussed here, feel free to open a new issue with the details.

tudburley commented 9 months ago

@tudburley we already have ServletOAuth2AuthorizedClientExchangeFilterFunction, is that what you're asking about? Otherwise, I'm not sure I understand the question. If you have a different enhancement suggestion than what is being discussed here, feel free to open a new issue with the details.

Its hard to understand what is the difference of ServletOAuth2AuthorizedClientExchangeFilterFunction and ServerOAuth2AuthorizedClientExchangeFilterFunction. My guess is, that the servlet variant stores authorized clients in the servlet context or to the current servlet request attributes.

But I am maintaining a web client library, and I am confused why I should use a ServletOAuth2AuthorizedClientExchangeFilterFunction and coupling it to servlets. This client library should by used by any Spring Boot application, even non-servlet applications.

And even if I would switch to ServletOAuth2AuthorizedClientExchangeFilterFunction: This class also depends on reactive classes. So I wouldn't get rid off the reactive stuff.

sjohnr commented 9 months ago

Ah, thanks for clarifying @tudburley!

Its hard to understand what is the difference of ServletOAuth2AuthorizedClientExchangeFilterFunction and ServerOAuth2AuthorizedClientExchangeFilterFunction. My guess is, that the servlet variant stores authorized clients in the servlet context or to the current servlet request attributes.

Without some extra context, it is indeed confusing. The difference is in which components are utilized internally to process the request.

Spring Security (and Spring more widely) provides support for both reactive and non-reactive stacks and they have completely separate interfaces/implementations/etc. There's probably a better guide on the history and naming conventions, but typically "servlet" refers to the imperative stack and "reactive" obviously refers to the reactive stack. However, some components use the name "server" to also refer to the reactive stack.

It just so happens that both implementations can be plugged into a WebClient through the above-mentioned ExchangeFilterFunctions. You would use only the "Server" or "Servlet" variant for the stack your application is using. Hopefully that's enough context for now.

Will there be also non-reactive equivalents for ServerOAuth2AuthorizedClientExchangeFilterFunction?

I now understand your question. Such support would be possible only with this issue, through the use of a RestClient. So yes, but "only" not "also".

There is no planned support for using these underlying components with a RestTemplate, though the servlet components themselves currently use RestTemplate internally.

rawadolb commented 8 months ago

Hi @sjohnr i don't understand why this issue was closed. What is the suggest solution to use RestClient to call protected resources with OAuth2 ?

for the moment i'm doing it in this way :

      @Bean("client-credentials")
      RestClient restClient(OAuth2AuthorizedClientManager authorizedClientManager,
                         ClientRegistrationRepository clientRegistrationRepository) {
      ClientRegistration clientRegistration = clientRegistrationRepository.findByRegistrationId("registration-id");
      ClientHttpRequestInterceptor oauthInterceptor = new OAuthClientCredentialsRestTemplateInterceptor(authorizedClientManager, clientRegistration);
       return RestClient.builder()
              .requestInterceptor(oauthInterceptor)
               .build();
   }

and in my inteceptor i call the authorization method to fetch a token and add it as a header

     @Override
    public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {
        OAuth2AuthorizeRequest oAuth2AuthorizeRequest = OAuth2AuthorizeRequest
                .withClientRegistrationId(clientRegistration.getRegistrationId())
                .principal(principal)
                .build();
        OAuth2AuthorizedClient client = manager.authorize(oAuth2AuthorizeRequest);
        if (isNull(client)) {
            throw new IllegalStateException("client credentials flow on " + clientRegistration.getRegistrationId() + " failed, client is null");
        }
        System.out.println("Bearer " + client.getAccessToken().getTokenValue() +
                " - issued at : " + client.getAccessToken().getIssuedAt() +
                " - expired at : " + client.getAccessToken().getExpiresAt()
        );
        request.getHeaders().add(HttpHeaders.AUTHORIZATION, "Bearer " + client.getAccessToken().getTokenValue());
        return execution.execute(request, body);
    }

The question is why we need to do it manually ? it could be better if it is handled by spring? it is not possible to add the bean ClientRegistration inject by spring oauth2-client directly to the RestClient object ?

Regards

sjohnr commented 8 months ago

@rawadolb

i don't understand why this issue was closed.

This issue is still open. Perhaps you saw that I closed gh-8882 which is a separate issue? We do still intend to add support for RestClient, but we don't have a timeline for when this support will be added yet.

rawadolb commented 8 months ago

@rawadolb

i don't understand why this issue was closed.

This issue is still open. Perhaps you saw that I closed gh-8882 which is a separate issue? We do still intend to add support for RestClient, but we don't have a timeline for when this support will be added yet.

my bad sorry :(

bigunyak commented 6 months ago

I'm also very much waiting when the new RestClient is supported, as this is what we want to switch to from the old RestTemplate. Hope this issue can be prioritised. 🙏

dev-praveen commented 6 months ago

I am just curious and couldn't find equivalent bean configuration for newly introduced RestClient. Attaching the code snippet that can make call to oauth2 protected apis with the WebClient. Looking for similar with RestClient. oauth2

Nhattd97 commented 5 months ago

May I please have an update on this matter?

ThomasKasene commented 5 months ago

Making a custom interceptor is simple enough to do (see for example rawaldop's comment) but since Spring Security is a framework its components have to be able to function in a variety of different setups. For example, the reactive ServletOAuth2AuthorizedClientExchangeFilterFunction and ServerOAuth2AuthorizedClientExchangeFilterFunction allow the user to define some attributes courtesy of the ClientRequest's API, and then those attributes are later used by the *ExchangeFilterFunction and beyond:

webClient
     .get()
     .uri(uri)
     .attributes(clientRegistrationId("my-special-client-registration"))
     // ...
     .retrieve()
     .bodyToMono(String.class);

Neither RestClient nor ClientHttpRequest has an "attribute" concept as far as I've been able to find out. Does anyone have any notion as to how one might pass customizations to a ClientHttpRequestInterceptor on a per-request basis? Is it even possible to do it (cleanly) with the RestClient in its current state?

mohmf commented 5 months ago

Hi @sjohnr i don't understand why this issue was closed. What is the suggest solution to use RestClient to call protected resources with OAuth2 ?

for the moment i'm doing it in this way :

      @Bean("client-credentials")
      RestClient restClient(OAuth2AuthorizedClientManager authorizedClientManager,
                         ClientRegistrationRepository clientRegistrationRepository) {
      ClientRegistration clientRegistration = clientRegistrationRepository.findByRegistrationId("registration-id");
      ClientHttpRequestInterceptor oauthInterceptor = new OAuthClientCredentialsRestTemplateInterceptor(authorizedClientManager, clientRegistration);
       return RestClient.builder()
              .requestInterceptor(oauthInterceptor)
               .build();
   }

and in my inteceptor i call the authorization method to fetch a token and add it as a header

     @Override
    public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {
        OAuth2AuthorizeRequest oAuth2AuthorizeRequest = OAuth2AuthorizeRequest
                .withClientRegistrationId(clientRegistration.getRegistrationId())
                .principal(principal)
                .build();
        OAuth2AuthorizedClient client = manager.authorize(oAuth2AuthorizeRequest);
        if (isNull(client)) {
            throw new IllegalStateException("client credentials flow on " + clientRegistration.getRegistrationId() + " failed, client is null");
        }
        System.out.println("Bearer " + client.getAccessToken().getTokenValue() +
                " - issued at : " + client.getAccessToken().getIssuedAt() +
                " - expired at : " + client.getAccessToken().getExpiresAt()
        );
        request.getHeaders().add(HttpHeaders.AUTHORIZATION, "Bearer " + client.getAccessToken().getTokenValue());
        return execution.execute(request, body);
    }

The question is why we need to do it manually ? it could be better if it is handled by spring? it is not possible to add the bean ClientRegistration inject by spring oauth2-client directly to the RestClient object ?

Regards

Could you provide the full implementation for this please?

ThomasKasene commented 5 months ago

@mohmf I do believe that is the full implementation, except they've left out the interceptor's constructor and the definition of the principal. For a normal client_credentials flow this can perhaps be as simple as using the client ID:

public class OAuthClientCredentialsRestTemplateInterceptor implements ClientHttpRequestInterceptor {

    private final OAuth2AuthorizedClientManager authorizedClientManager;
    private final ClientRegistration clientRegistration;

    public OAuthClientCredentialsRestTemplateInterceptor(
            OAuth2AuthorizedClientManager authorizedClientManager, ClientRegistration clientRegistration) {
        this.authorizedClientManager = authorizedClientManager;
        this.clientRegistration = clientRegistration;
    }

    @Override
    public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {
        var clientRegistrationId = this.clientRegistration.getRegistrationId();
        var principal = this.clientRegistration.getClientId();
        var authorizeRequest = OAuth2AuthorizeRequest
                .withClientRegistrationId(clientRegistrationId)
                .principal(principal)
                .build();
        var authorizedClient = manager.authorize(authorizeRequest);
        if (authorizedClient == null) {
            throw new IllegalStateException("client credentials flow on " + clientRegistrationId + " failed, client is null");
        }
        request.setBearerAuth(client.getAccessToken().getTokenValue());
        return execution.execute(request, body);
    }
}
ntenherkel commented 4 months ago

On the page where Spring introduces RestClient, there is a nice "sales talk" that introduces RestClient and it says:

[...] With RestClient we are introducing a HTTP client that offers an API similar to WebClient [...]

Great! I convinced my team mates to switch because WebClient supports OAuth2 as bealdung described here. Well now it turns out, RestClient does not have an easy way to do this after reading all of the above.

Feels a bit like buying something from AliExpress. The product looks very nice on the web page, but when you receive it it is trash. 😛

stenkil-sn commented 4 months ago

problem is in DefaultContextAttributesMapper

public class CustomContextAttributesMapper implements Function<OAuth2AuthorizeRequest, Map<String, Object>> {
        public Map<String, Object> apply(OAuth2AuthorizeRequest authorizeRequest) {
            return new HashMap<>(authorizeRequest.getAttributes());
        }
    }

update

        @Bean
        OAuth2AuthorizedClientManager authorizedClientManager(
                final ClientRegistrationRepository clientRegistrationRepository) {
            ..............................................
            final AuthorizedClientServiceOAuth2AuthorizedClientManager authorizedClientManager =
                    new AuthorizedClientServiceOAuth2AuthorizedClientManager(
                            clientRegistrationRepository,
                            new InMemoryOAuth2AuthorizedClientService(clientRegistrationRepository));
            authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);
            authorizedClientManager.setContextAttributesMapper(new CustomContextAttributesMapper()); // set custom context attributes mapper
            return authorizedClientManager;
        }
    }

and

public class OAuthRestClientRequestInterceptor implements ClientHttpRequestInterceptor {

    private final ClientRegistration clientRegistration;
    private final OAuth2AuthorizedClientManager manager;

    @Override
    public ClientHttpResponse intercept(final HttpRequest request, final byte[] body, final ClientHttpRequestExecution execution) throws IOException {
        final OAuth2AuthorizeRequest oAuth2AuthorizeRequest = OAuth2AuthorizeRequest
                .withClientRegistrationId(clientRegistration.getRegistrationId())
                .attribute(OAuth2AuthorizationContext.USERNAME_ATTRIBUTE_NAME, clientRegistration.getClientName())
                .attribute(OAuth2AuthorizationContext.PASSWORD_ATTRIBUTE_NAME, clientRegistration.getClientSecret())
                .principal(clientRegistration.getRegistrationId()) // can be any name
                .build();

        final OAuth2AuthorizedClient client = manager.authorize(oAuth2AuthorizeRequest);
        if (client == null) {
            throw new IllegalStateException("client credentials on " + clientRegistration.getRegistrationId() + " not defined");
        }
        request.getHeaders().setBearerAuth(client.getAccessToken().getTokenValue());
        return execution.execute(request, body);
    }

I'm use only PasswordOAuth2AuthorizedClientProvider, you can add all attributes from clientRegistration to support other providers

    @Bean
    RestClient restClient(final RestClient.Builder builder,
                           final OAuth2AuthorizedClientManager authorizedClientManager,
                           final ClientRegistrationRepository clientRegistrationRepository) {
        final ClientRegistration registration =
                clientRegistrationRepository.findByRegistrationId("client_id");
        final ClientHttpRequestInterceptor interceptor =
                new OAuthRestClientRequestInterceptor(registration, authorizedClientManager);
        return builder.requestInterceptor(interceptor)
                .build();
    }
sjohnr commented 4 months ago

Thanks everyone for the feedback, conversation and upvotes on this issue.

May I please have an update on this matter?

We're watching this issue closely as it already has many upvotes, and we will have a better update once a few other high priority items have been completed. I still don't know for sure that we will be able to provide anything for this in the 6.3 release but as soon as we know anything for certain, we will share it.

In the meantime, if anyone is interested in working on a standalone implementation or POC (separate project for now) and/or sharing ideas and feedback with each other on that, I think that would be great. Based on the activity on this issue already, it could be quite helpful to folks.

ColdFireIce commented 4 months ago

@stenkil-sn Nice solution and findings. I could have used this 24h ago ;) I did something very similar:

    @Bean
    public OAuth2AuthorizedClientManager authorizedClientManager(
            ClientRegistrationRepository clientRegistrationRepository
    ) {
        OAuth2AuthorizedClientProvider authorizedClientProvider =
                OAuth2AuthorizedClientProviderBuilder.builder()
                        .clientCredentials()
                        .password() // used for our usecase
                        .refreshToken() // must be added to handle refresh tokens
                        .build();

        AuthorizedClientServiceOAuth2AuthorizedClientManager authorizedClientManager =
                new AuthorizedClientServiceOAuth2AuthorizedClientManager(
                        clientRegistrationRepository,
                        new InMemoryOAuth2AuthorizedClientService(clientRegistrationRepository)
                );

        authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);
        authorizedClientManager.setContextAttributesMapper(oAuth2AuthorizeRequest -> {
            var attributes = new HashMap<>(
                    new AuthorizedClientServiceOAuth2AuthorizedClientManager.DefaultContextAttributesMapper()
                            .apply(oAuth2AuthorizeRequest)
            );
            attributes.put(OAuth2AuthorizationContext.USERNAME_ATTRIBUTE_NAME, username);
            attributes.put(OAuth2AuthorizationContext.PASSWORD_ATTRIBUTE_NAME, password);
            return attributes;
        });

        return authorizedClientManager;
    }

but instead of using an interceptor i opted for the ClientHttpRequestInitializer. Is there a difference, or is one better than the other?

    @Bean
    public ClientHttpRequestInitializer outh2ClientHttpRequestInitializer(OAuth2AuthorizedClientManager authorizedClientManager) {
        return request -> {
            OAuth2AuthorizeRequest authorizeRequest = OAuth2AuthorizeRequest.withClientRegistrationId("keycloak")
                    .principal("<principal>")
                    .build();

            OAuth2AuthorizedClient authorizedClient = authorizedClientManager.authorize(authorizeRequest);

            OAuth2AccessToken accessToken = Objects.requireNonNull(authorizedClient).getAccessToken();

            log.debug("Issued: {}, Expires: {}", accessToken.getIssuedAt(),  accessToken.getExpiresAt());
            log.debug("Scopes: {}", accessToken.getScopes().toString());
            log.debug("Token: {}", accessToken.getTokenValue());

            // Add access token to request header
            request.getHeaders().setBearerAuth(accessToken.getTokenValue());
        };
    }

makes the client somewhat ok, but I vote for this issue none the less. It's too mush non standard stuff.

    @Bean
    public RestClient restClient(ClientHttpRequestInitializer outh2ClientHttpRequestInitializer) {
        return RestClient.builder()
                .baseUrl("<url>")
                .requestInitializer(outh2ClientHttpRequestInitializer)
                .requestInterceptor((request, body, execution) -> {
                    if (log.isDebugEnabled()) {
                        HttpHeaders headers = request.getHeaders();
                        log.debug("Request to {}\n\tRequest-Method: {}, \n\tRequest-Headers: {}",
                                v("request_url", request.getURI()),
                                v("request_method", request.getMethod()),
                                v("request_headers", headers.entrySet().stream()
                                        .map((entry) -> entry.getKey() + ":" + entry.getValue()).toList()));
                    }

                    return execution.execute(request, body);
                })
                .build();
    }
ThomasKasene commented 4 months ago

Apparently, some form of RestClient.attributes might be making a return in Spring Framework 6.2 according to this issue, so maybe that'll make it easier to implement this.

mjeffrey commented 4 months ago

The interceptor from @stenkil-sn got me thinking that this is probably the way to do it. So I tried using the existing Spring Security Oauth classes and it seems pretty ok but probably misses stuff.

Since @sjohnr suggested a POC I made one here based on collected examples from @jgrandja.

So far it is only for the client_credentials grant, may not work with the authorization code grant (not really my use case so I de-prioritized it ).

POC on Github. It covers both password and private_jwt auth with Keycloak.

public class OAuth2ClientInterceptor implements ClientHttpRequestInterceptor {

    private final OAuth2AuthorizedClientManager manager;
    private final ClientRegistration clientRegistration;

    public OAuth2ClientInterceptor(OAuth2AuthorizedClientManager manager, ClientRegistration clientRegistration) {
        this.manager = manager;
        this.clientRegistration = clientRegistration;
    }

    @Override
    public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {
        OAuth2AuthorizeRequest oAuth2AuthorizeRequest = OAuth2AuthorizeRequest
                .withClientRegistrationId(clientRegistration.getRegistrationId())
                .principal(clientRegistration.getClientId())
                .build();

        OAuth2AuthorizedClient client = manager.authorize(oAuth2AuthorizeRequest);
        Assert.notNull(client, () -> "Authorized client failed for Registration id: '" + clientRegistration.getRegistrationId() + "', returned client is null");
        request.getHeaders().setBearerAuth(client.getAccessToken().getTokenValue());
        return execution.execute(request, body);
    }
}
mjeffrey commented 4 months ago

@ColdFireIce

but instead of using an interceptor i opted for the ClientHttpRequestInitializer. Is there a difference, or is one better than the other

I think ClientHttpRequestInitializer is probably the way to go idd. It has less access to the request so is probably preferred. Not sure if there are cases where the Initializer is not enough (maybe on some other auth mechanisms?). Note there is no reason we could not implement both like this (will update the POC a bit later)

public class OAuth2ClientInterceptor implements ClientHttpRequestInterceptor, ClientHttpRequestInitializer {

    private final OAuth2AuthorizedClientManager manager;
    private final ClientRegistration clientRegistration;

    public OAuth2ClientInterceptor(OAuth2AuthorizedClientManager manager, ClientRegistration clientRegistration) {
        this.manager = manager;
        this.clientRegistration = clientRegistration;
    }

    @Override
    public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {
        request.getHeaders().setBearerAuth(getBearerToken());
        return execution.execute(request, body);
    }

    @Override
    public void initialize(ClientHttpRequest request) {
        request.getHeaders().setBearerAuth(getBearerToken());
    }

    private String getBearerToken() {
        OAuth2AuthorizeRequest oAuth2AuthorizeRequest = OAuth2AuthorizeRequest
                .withClientRegistrationId(clientRegistration.getRegistrationId())
                .principal(clientRegistration.getClientId())
                .build();

        OAuth2AuthorizedClient client = manager.authorize(oAuth2AuthorizeRequest);
        Assert.notNull(client, () -> "Authorized client failed for Registration id: '" + clientRegistration.getRegistrationId() + "', returned client is null");
        return client.getAccessToken().getTokenValue();
    }
}
tudburley commented 4 months ago

@mjeffrey I had a look at your code in POC on Github (thanks for sharing it). It still depends on the reactive stack, especially when using ServletOAuth2AuthorizedClientExchangeFilterFunction . But I thought with RestClient we should get rid of reactive?

mjeffrey commented 4 months ago

@mjeffrey I had a look at your code in POC on Github (thanks for sharing it). It still depends on the reactive stack, especially when using ServletOAuth2AuthorizedClientExchangeFilterFunction . But I thought with RestClient we should get rid of reactive?

Yes indeed, the whole point was to get rid of reactive (I hate that stuff and hope it dies with virtual threads). I just wanted to compare the two approaches.

I made a branch with the existing poc (with webclient) and removed webflux/webclient from the main branch. Take a look now.

tudburley commented 4 months ago

Thanks @mjeffrey for the update, looks good now.

Just one detail question: The OAuth2ClientInterceptor#initialize method will be never called, because the OAuth2ClientInterceptor is only added to the RestClient.Builder#requestInterceptor(interceptor) as a ClientHttpRequestInterceptor. Maybe the OAuth2ClientInterceptor#initialize method is not needed at all? Or do I miss something?

mjeffrey commented 4 months ago

The "OAuth2ClientInterceptor" actually implements both ClientHttpRequestInterceptor and ClientHttpRequestInitializer just as a demonstration (bad idea to do this in practice I think - a bit confusing).

I made one RestClient use it as an Interceptor and the other as an Initializer (just for demo purposes).

My personal preference is a ClientHttpRequestInitializer and it should then be called "OAuth2ClientInitializer" or something with Initializer in the name.

    @Bean
    RestClient restClientPassword(RestClient.Builder builder,
                          OAuth2AuthorizedClientManager authorizedClientManager,
                          ClientRegistrationRepository clientRegistrationRepository) {
        ClientRegistration clientRegistration = clientRegistrationRepository.findByRegistrationId("secret");
        ClientHttpRequestInterceptor interceptor = new OAuth2ClientInterceptor(authorizedClientManager, clientRegistration);
        return builder.requestInterceptor(interceptor).build();
    }

    /**
     * This uses requestInitializer (just as a demonstration - we could use interceptor as well)
     */
    @Bean
    RestClient restClientJwt(RestClient.Builder builder,
                          OAuth2AuthorizedClientManager authorizedClientManager,
                          ClientRegistrationRepository clientRegistrationRepository) {
        ClientRegistration clientRegistration = clientRegistrationRepository.findByRegistrationId("jwt");
        ClientHttpRequestInitializer initializer = new OAuth2ClientInterceptor(authorizedClientManager, clientRegistration);
        return builder.requestInitializer(initializer).build();
    }
mjeffrey commented 4 months ago

After experimenting with the implementation a bit, I came to the following conclusions.

  1. It is not difficult to implement this ourselves (with guidance) so maybe it does not need to be provided by Spring Security?
  2. If not provided it should be documented somewhere (oauth2 is complex and it is sometimes difficult to find what you need in the docs)
  3. I did not find an easy way to cache the token (and get a new one on expiry) probably an issue with my simplistic implementation?

If implemented in Spring Security, it could be as a ClientHttpRequestInitializer or a ClientHttpRequestInterceptor (or something else like a Customizer I guess). I don't know if this will make a difference in this case but the docs for ClientHttpRequestInitializer say:

Unlike ClientHttpRequestInterceptor, this interface can apply customizations without needing to read the entire request body into memory

Both the interceptor and the initializer can be used on RestClient and RestTemplate. However RestTemplateBuilder does not support the Initializer directly, you need to construct the RestTemplate first and then add the initializer, so slightly less convenient than the Interceptor that can be used directly with the builder.

sjohnr commented 4 months ago

It is not difficult to implement this ourselves (with guidance) so maybe it does not need to be provided by Spring Security?

I agree that it is not difficult to implement this minimally. As for whether it is needed in the framework, it seems the comfort level of most folks would be to use something that is provided instead of rolling it themselves. What level of support is actually needed is more the question I have. The current WebClient support for both Servlet and Reactive is quite complex, largely because it is intended to support a wide variety of use cases within a single implementation.

I did not find an easy way to cache the token (and get a new one on expiry) probably an issue with my simplistic implementation?

I see you are using OAuth2AuthorizedClientManager. If you're using refresh_token, this is all that is necessary to obtain an existing or refresh an expired token. Or am I misunderstanding the challenge you're having with caching/expired tokens?

mjeffrey commented 4 months ago

@sjohnr Caching of the token was a problem with my configuration so ignore it (we don't use refresh_token with client_credentials since it is discouraged https://www.rfc-editor.org/rfc/rfc6749#section-4.4.3).

I'll update the POC with the correct config when I get a chance over the weekend.

angryuser56742 commented 1 month ago

Any update on this feature? Can this be supported on spring side?

sjohnr commented 1 month ago

I'm taking a deeper look at this in preparation for the next release cycle.

since Spring Security is a framework its components have to be able to function in a variety of different setups. For example, the reactive ServletOAuth2AuthorizedClientExchangeFilterFunction and ServerOAuth2AuthorizedClientExchangeFilterFunction allow the user to define some attributes courtesy of the ClientRequest's API, and then those attributes are later used by the *ExchangeFilterFunction and beyond

The attributes actually add quite a bit of complexity to the both servlet and reactive implementations. On the servlet side, attributes were largely required to propagate the request/response/authentication and configuration to the filter (prior to context propagation). On the reactive side, attributes are used primarily as per-request configuration to allow a single implementation to handle a variety of use cases. I don't think we want to repeat this pattern if there are other options, and therefore I don't think we would use attributes even if they become available for RestClient.

I'm seeing three possible implementation options:

Given these options, configuring ClientHttpRequestInitializer with the RestClient.Builder seems appropriate for per-application (or per application context) arrangements, where the configuration is known at startup time. On the other hand, if a more dynamic per-request arrangement is required, configuring a Consumer<ClientHttpRequest> directly with RestClient seems appropriate. It doesn't seem necessary provide an implementation of ClientHttpRequestInterceptor.

Therefore, I think we could provide three simple implementations instead of one more complex implementation:

I don't feel like we need an initializer that accepts an authorizedClient as it would seem unusual to have a pre-existing instance at startup. However, there is one additional case of discovering the clientRegistrationId from the currently logged in user (see docs). If this scenario is required, we could add another implementation later on.

This commit (from this branch) demonstrates these options.

aka-unk commented 1 month ago

Thanks! It was a bit tricky to integrate with resource server (after a client registration it wanted to play authorization_code and redirected the request to the login server), it could be corrected adding this:

    @Bean
    public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
        http.oauth2ResourceServer(oauth2 -> oauth2.jwt(Customizer.withDefaults()));
        return http.build();
    }

but, I'm not sure that's the expected behavior. This is the config:

spring:
  security:
    oauth2:
      resourceserver:
        jwt:
          issuer-uri: http://localhost:18321
      client:
        registration:
          sample-client:
            provider: token-provider
            client-id: client
            client-secret: ********
            authorization-grant-type: client_credentials
            client-authentication-method: client_secret_basic
            scope: user.read,user.write
        provider:
          token-provider:
            issuer-uri: http://localhost:18321

@sjohnr, can you please take a look, did I mess up something or does it require any further correction?

sjohnr commented 1 month ago

Thinking about this more:

It doesn't seem necessary provide an implementation of ClientHttpRequestInterceptor.

I think it might be beneficial to provide the interceptors for consistency, because in the core framework there is already a BasicAuthenticationInterceptor. What I like about this is the use of the terminology "interceptor" seems more familiar and therefore provides an easier learning curve for users.

I also think that reducing to only a single interface could simplify the number of implementations and further reduce learning curve. The only challenge is finding a way to continue to support the per-request arrangement. I feel that a simple factory method for producing a Consumer<ClientHttpRequest> could be the way to go here. This commit demonstrates this option.

Here's what an example per-application configuration could look like:

@Bean
public RestClient restClient(OAuth2AuthorizedClientManager authorizedClientManager) {
    OAuth2ClientHttpRequestInterceptor requestInterceptor =
        new OAuth2ClientHttpRequestInterceptor(authorizedClientManager, CLIENT_REGISTRATION_ID);

    return RestClient.builder()
        .requestInterceptor(requestInterceptor)
        .build();
}

and a per-request arrangement might look like this:

@GetMapping("/cashcards")
public Cashcard[] cashcards() {
    OAuth2AuthorizedClient authorizedClient = ...;

    OAuth2AuthorizedClientHttpRequestInterceptor requestInterceptor =
        new OAuth2AuthorizedClientHttpRequestInterceptor(
            this.authorizedClientManager, authorizedClient);

    return this.restClient.get()
        .uri("http://localhost:8090/cashcards")
        .httpRequest(requestInterceptor.apply())
        .retrieve()
        .body(Cashcard[].class);
}

Any thoughts or feedback on this instead of the 3 implementations I proposed earlier?

ThomasVitale commented 1 month ago

@sjohnr I really like the design of this final proposal, especially the fact that it requires one single interface and that it re-uses the familiar concept of Interceptor (and consistent with BasicAuthenticationInterceptor).

mcordeiro73 commented 1 month ago

@sjohnr Your Interceptor is almost identical to what I did.

One other thing we created was an ErrorHandler that will remove the authorized client when an authorization error occurs. We interact with some 3rd party services that will revoke a token before it expires, so we have a need to ensure when that occurs we remove the authorize client so the next attempt will result in a new token being requested.

Hers is what we did for an ErrorHandler:

public class RemoveAuthorizedClientOAuth2ResponseErrorHandler extends DefaultResponseErrorHandler {

    private static final Predicate<HttpStatusCode> STATUS_PREDICATE = httpStatusCode -> httpStatusCode.value() == HttpStatus.UNAUTHORIZED.value();

    private final OAuth2AuthorizedClientService oauth2AuthorizedClientService;
    private final ClientRegistration clientRegistration;

    public RemoveAuthorizedClientOAuth2ResponseErrorHandler(@NotNull OAuth2AuthorizedClientService oauth2AuthorizedClientService, @NotNull ClientRegistration clientRegistration) {
        this.oauth2AuthorizedClientService = oauth2AuthorizedClientService;
        this.clientRegistration = clientRegistration;
    }

    @Override
    public void handleError(ClientHttpResponse clientHttpResponse) throws IOException {
        if (STATUS_PREDICATE.test(clientHttpResponse.getStatusCode())) {
            oauth2AuthorizedClientService.removeAuthorizedClient(clientRegistration.getRegistrationId(), clientRegistration.getClientId());
        }
        super.handleError(clientHttpResponse);
    }

    public static RestClient.ResponseSpec.ErrorHandler createErrorHandler(@NotNull OAuth2AuthorizedClientService oauth2AuthorizedClientService, @NotNull ClientRegistration clientRegistration) {
        ResponseErrorHandler responseErrorHandler = new RemoveAuthorizedClientOAuth2ResponseErrorHandler(oauth2AuthorizedClientService, clientRegistration);
        return (request, response) -> responseErrorHandler.handleError(response);
    }

    public static Predicate<HttpStatusCode> unauthorizedStatusPredicate() {
        return STATUS_PREDICATE;
    }

}

With my RestClient config then looking like:

@Bean
protected RestClient restClient(RestClient.Builder builder, ClientRegistrationRepository clientRegistrationRepository, OAuth2AuthorizedClientService oauth2AuthorizedClientService) {
    OAuth2AuthorizedClientManager authorizedClientManager = new AuthorizedClientServiceOAuth2AuthorizedClientManager(clientRegistrationRepository, oauth2AuthorizedClientService);
    ClientRegistration clientRegistration = clientRegistrationRepository.findByRegistrationId(CLIENT_REGISTRATION_ID);

    return builder.requestInterceptor(new Oauth2ClientHttpRequestInterceptor(authorizedClientManager, clientRegistration))
            .defaultStatusHandler(RemoveAuthorizedClientOAuth2ResponseErrorHandler.unauthorizedStatusPredicate(), RemoveAuthorizedClientOAuth2ResponseErrorHandler.createErrorHandler(authorizedClientService, clientRegistration))
            .build();
}

This is by no means a perfect solution as it essentially requires the use of the DefaultResponseErrorHandler to handle other errors. I may have been better served to provide my ErrorHandler a delegate ResponseErrorHandler instead.

Removing the authorized client is one thing that used to be automatic with Resttemplate that was lost when moving to WebClient (though there were ways to ensure it was in place) and then to RestClient. Another thing lost was the automatic retry of a request with a new token after an authorization failure (via OAuth2RestTemplate). My hope is that we can bring both these features back to the core Spring libraries as they were very useful in certain situations.

goafabric commented 1 month ago

@goafabric: remindme

sjohnr commented 3 weeks ago

Thanks for the feedback @mcordeiro73! The failure handling is what I wanted to work on next, which I have added in this version. I have further simplified things by dropping support for a separate OAuth2AuthorizedClient, in favor of a single interceptor that accepts only a clientRegistrationId. This makes the usage pattern very simple, and we can evaluate whether we need anything further via feedback from users. This commit contains the latest version.

The per-application configuration looks like this (with only a new setter added):

@Bean
public RestClient restClient(OAuth2AuthorizedClientManager authorizedClientManager,
        OAuth2AuthorizedClientRepository authorizedClientRepository) {

    OAuth2ClientHttpRequestInterceptor requestInterceptor =
        new OAuth2ClientHttpRequestInterceptor(authorizedClientManager, CLIENT_REGISTRATION_ID);

    // Configure the interceptor to remove invalid authorized clients
    requestInterceptor.setAuthorizedClientRepository(authorizedClientRepository);

    return RestClient.builder()
        .requestInterceptor(requestInterceptor)
        .build();
}

and a per-request arrangement might look like this:

@GetMapping("/cashcards")
public Cashcard[] cashcards() {
    OAuth2ClientHttpRequestInterceptor requestInterceptor =
        new OAuth2ClientHttpRequestInterceptor(this.authorizedClientManager, CLIENT_REGISTRATION_ID);

    // Configure the interceptor to remove invalid authorized clients
    requestInterceptor.setAuthorizedClientRepository(this.authorizedClientRepository);

    return this.restClient.get()
        .uri("http://localhost:8090/cashcards")
        .httpRequest(requestInterceptor.httpRequest())
        .retrieve()
        .onStatus(requestInterceptor.errorHandler())
        .body(Cashcard[].class);
}

In the per-request case, we are taking a generic (globally configured) RestClient and adding a Consumer<ClientHttpRequest> via requestInterceptor.httpRequest() and a ResponseErrorHandler via requestInterceptor.errorHandler() on a request-by-request basis.

This is necessary since we don't have the ability to intercept the request so we have to specify an error handler separately. The error handler is optional if removal of the authorized client isn't required.