Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RestClient interceptor #15437

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

sjohnr
Copy link
Member

@sjohnr sjohnr commented Jul 17, 2024

To Do:

  • Add unit tests
  • Add integration tests
  • Add reference documentation
  • Add sample(s)

Closes gh-13588

@sjohnr sjohnr added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Jul 17, 2024
@sjohnr sjohnr added this to the 6.4.0-M2 milestone Jul 17, 2024
@sjohnr sjohnr self-assigned this Jul 17, 2024
@sjohnr sjohnr marked this pull request as draft July 17, 2024 21:24
@azdanov
Copy link

azdanov commented Jul 18, 2024

Hey. If using an HTTP Interface, how would it be possible to dynamically change clientRegistrationId since there is no access to the restClient after the proxy is created?

In webclient I could use ServletOAuth2AuthorizedClientExchangeFilterFunction:

@Bean
public WebClient webClient(
        WebClient.Builder builder,
        HttpClient httpClient,
        OAuth2AuthorizedClientManager authorizedClientManager
) {
    ServletOAuth2AuthorizedClientExchangeFilterFunction oauth2 =
            new ServletOAuth2AuthorizedClientExchangeFilterFunction(authorizedClientManager);

    oauth2.setDefaultOAuth2AuthorizedClient(true);

    return builder
            .apply(oauth2.oauth2Configuration())
            .clientConnector(new ReactorClientHttpConnector(httpClient))
            .build();
}

@sjohnr
Copy link
Member Author

sjohnr commented Jul 18, 2024

Thanks for asking, @azdanov. See the per-request arrangement in this comment. This aspect of the interceptor is still in need of feedback so feel free to weigh in on it. The other option would be to use RestClient#attributes but I am not sure if using attributes to dynamically configure on a per-request basis is intuitive for users, so I'm not yet leaning towards using them.

However, I'm not clear how you would dynamically change the clientRegistrationId in your example with WebClient since I don't believe you can use attributes there. Using setDefaultOAuth2AuthorizedClient(true) would allow detecting the id from the currently logged in user (if they logged in using OAuth2 or OIDC), but you couldn't specify it dynamically. Can you please provide an example of how you would incorporate the WebClient from your example into an HTTP interface while dynamically specifying the clientRegistrationId?

@azdanov
Copy link

azdanov commented Jul 18, 2024

Using setDefaultOAuth2AuthorizedClient(true) would allow detecting the id from the currently logged in user

This is what I meant, thank you for clarifying things 🙂. That's what I considered dynamic, that the interceptor will do it, and not me. It would be cool to see a setDefaultOAuth2AuthorizedClient option RestClient interceptor too.

@sjohnr
Copy link
Member Author

sjohnr commented Jul 18, 2024

This is what I meant, thank you for clarifying things 🙂. That's what I considered dynamic, that the interceptor will do it, and not me. It would be cool to see a setDefaultOAuth2AuthorizedClient option RestClient interceptor too.

Thanks @azdanov, I suspected so but didn't want to assume. I have not yet determined whether it is truly necessary to introduce this functionality into the interceptor. I consider it a convenience feature, and it creates more ways to use the interceptor which can confuse users as much as help them. That's something I'd like to avoid. However, this is good feedback so I will think about it some more.

@Lonelysoul-HayashiNoMirai

@sjohnr
Bro, I found another problem, when using ur interceptor with the @async annotation, the method handleAuthorizationFailure () throws an exception [IllegalArgumentException: request cannot be null] if the current access token failed...

I've tried to debug it, and I guess the root cause of the problem is that ur code tried to get the RequestAttributes from the RequestContextHolder and then get the request from the RequestAttributes. Since @async is using, there is nothing in the RequestContextHolder... => failed

~ Could you do me a favour and suggest me a way to fix it?

Request_Is_Null_Exception_Screen

@sjohnr
Copy link
Member Author

sjohnr commented Aug 2, 2024

@Lonelysoul-HayashiNoMirai

~ Could you do me a favour and suggest me a way to fix it?

To use OAuth2 Client features with @Async, TaskExecutor or other background thread processing, you should use AuthorizedClientServiceOAuth2AuthorizedClientManager, by registering a bean like so:

@Bean
public OAuth2AuthorizedClientManager authorizedClientManager(
		ClientRegistrationRepository clientRegistrationRepository,
		OAuth2AuthorizedClientService authorizedClientService) {

	return new AuthorizedClientServiceOAuth2AuthorizedClientManager(
		clientRegistrationRepository, authorizedClientService);
}

I believe that will solve the issue, but note that this wouldn't be an issue with the interceptor but only with your configuration. If this doesn't solve the issue, please provide a minimal sample as I am not able to reproduce your issue without this.

@sjohnr sjohnr force-pushed the gh-13588-rest-client branch from c875f2c to 5caf889 Compare August 7, 2024 21:04
@sjohnr
Copy link
Member Author

sjohnr commented Aug 7, 2024

Just a note for those following along: Based on feedback, I've pushed changes to the PR (gh-15437) with an updated API for the interceptor that takes advantage of attributes for configuring the clientRegistrationId on a per-request basis. This effectively consolidates everything to a single primary usage example:

import static org.springframework.security.oauth2.client.web.function.client.OAuth2ClientHttpRequestInterceptor.clientRegistrationId;

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

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

	return this.restClient.get()
		.uri("http://localhost:8090/cashcards")
		.attributes(clientRegistrationId(CLIENT_REGISTRATION_ID))
		.retrieve()
		.body(Cashcard[].class);
}

Note that you can also call requestInterceptor.setDefaultClientRegistrationId(...), or omit the clientRegistrationId entirely and it will be derived from the current user (requires http.oauth2Login()).

cc: @azdanov @barbetb @mcordeiro73 @ThomasVitale @mjeffrey @ThomasKasene @rwinch @jgrandja

@sjohnr sjohnr force-pushed the gh-13588-rest-client branch from 5caf889 to a3d61bd Compare August 7, 2024 21:29
@Lonelysoul-HayashiNoMirai

@sjohnr
Finally I got some free time for u, sorry bro.
Obviously, I can't give u the production code, so here u go: I created 2 new projects to reproduce this issue:
request-cannot-be-null-exception

Steps To Reproduce:

  • Step 1: Run both projects.
  • Step 2: Send request to the client app to trigger it send request to the server app:
  • Step 3: Check the log of the client app, if u see "The response: protected-string", that means the interceptor successfully fetched the access token and then the client app is free to access the server's APIs.
  • Step 4: Restart the server app to invalidate the token.
  • Step 5: Send request to the client app (Step 2) again, exception will be thrown in the client app: IllegalArgumentException: request cannot be null

Expected Behavior: The client app should discard the invalid token and fetch a new one.

@sjohnr
Copy link
Member Author

sjohnr commented Aug 8, 2024

@Lonelysoul-HayashiNoMirai thank you again for trying the interceptor, and providing samples for the issue you encountered. There are a couple of things going on.

  1. You have modified the interceptor with the baseUrl issue we discussed, which is not necessary. You are using a URI to interact with RestClient, and the javadoc for the uri(URI) method clearly states:

    Specify the URI using an absolute, fully constructed URI.

    If you want to let RestClient fill in the baseUrl for you, you should provide it to RestClient when building using .baseUrl(...) on the builder, and then use the uri(String) or one of the non-URI variants to provide the path.

  2. The issue you are encountering with IllegalArgumentException: request cannot be null is because you are calling requestInterceptor.setAuthorizedClientRepository(authorizedClientRepository) when setting up the interceptor. This sets up failure handling to expect a request. There is also requestInterceptor.setAuthorizedClientService(authorizedClientService) which is correct for your use case, because you are using AuthorizedClientServiceOAuth2AuthorizedClientManager with @Async. Please use this setup in order to resolve your issue. Note: Calling an async method causes the RequestContextHolder to lose the context of the original request, which causes the issue you encountered.

Again, thank you for trying things out and looking for issues. It is very helpful to know that this use case requires careful setup in order to avoid issues, and I think clear documentation will help immensely.

@Lonelysoul-HayashiNoMirai
Copy link

Lonelysoul-HayashiNoMirai commented Aug 9, 2024

@sjohnr
Well, i don't know what to say: thanks bro, love u, u saved my life:

Point 1: "You have modified..." It works flawlessly now.
Point 2: "The issue you are encountering...": It seems to be working fine using ur advice... yet the first request call (after the server app restart) still fails with 401, it becomes Ok if I resend the request again (pls check the image below), is this expected behavior?, do I need to implement the retry logic myself? (May be, I would use spring-retry):

image

@sjohnr
Copy link
Member Author

sjohnr commented Aug 9, 2024

is this expected behavior?, do I need to implement the retry logic myself? (May be, I would use spring-retry):

It is expected behavior, yes. Adding retry would be pretty cool, let me know if that works for you!

@sjohnr sjohnr force-pushed the gh-13588-rest-client branch from 2dcb92d to cb7c727 Compare August 16, 2024 22:07
@sjohnr sjohnr marked this pull request as ready for review August 16, 2024 22:09
@sjohnr sjohnr enabled auto-merge (rebase) August 16, 2024 22:09
@sjohnr sjohnr merged commit e3c19ba into spring-projects:main Aug 16, 2024
4 checks passed
@sjohnr sjohnr deleted the gh-13588-rest-client branch August 17, 2024 05:07
@Lonelysoul-HayashiNoMirai

is this expected behavior?, do I need to implement the retry logic myself? (May be, I would use spring-retry):

It is expected behavior, yes. Adding retry would be pretty cool, let me know if that works for you!

@sjohnr
FYI, the combination with the spring-retry works perfectly! Once again, thank u for... everything, bro <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add support for requesting protected resources with RestClient
3 participants