Register RestOperations @Bean for OAuth 2.0 Client #8732

jgrandja · 2020-06-19T20:20:07Z

This PR addresses the Servlet implementation for OAuth 2.0 Client.

The changes in this PR will also be required for OAuth 2.0 Client WebFlux.

In addition to OAuth 2.0 Client, similar changes will need to be applied to OAuth 2.0 Resource Server and the JOSE stack.

I've done all the legwork as to which changes are required for the follow-up PR's:

OAuth 2.0 Client WebFlux

WebClientReactiveAuthorizationCodeTokenResponseClient
WebClientReactiveRefreshTokenTokenResponseClient
WebClientReactiveClientCredentialsTokenResponseClient
WebClientReactivePasswordTokenResponseClient

DefaultReactiveOAuth2UserService

OAuth 2.0 Resource Server

NimbusOpaqueTokenIntrospector

OAuth 2.0 Resource Server WebFlux

NimbusReactiveOpaqueTokenIntrospector

JOSE

NimbusJwtDecoder
NimbusJwtDecoderJwkSupport
JwtDecoderProviderConfigurationUtils
JwtDecoders

JOSE WebFlux

NimbusReactiveJwtDecoder
ReactiveRemoteJWKSource
ReactiveJwtDecoders

Closes spring-projectsgh-5607

rwinch

I've provided some feedback inline.

rwinch · 2020-06-22T17:40:01Z

...rk/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerUtils.java

 	}
+
+	static <B extends HttpSecurityBuilder<B>> RestOperations getRestOperationsBean(B builder) {
+		return builder.getSharedObject(ApplicationContext.class).getBean(


This looks like it would fail if there isn't a bean by that name and type or if there is more than one bean of this type.

Is there a reason you think we should use a bean name here? We don't typically do the lookups by bean name.

This looks like it would fail if there isn't a bean by that name and type or if there is more than one bean of this type

OAuth2ClientRestOperationsPostProcessor defined in OAuth2ClientConfiguration will register the @Bean using OAuth2ClientBeanNames.REST_OPERATIONS so it won't fail. It's expected to be registered.

Is there a reason you think we should use a bean name here?

I thought this is what we decided on, as per comment

Furthermore, an application may register one or more RestOperations @Bean so we need to ensure we pick up the correct RestOperations @Bean to be used for the client flows - hence the use of bean name.

Also, see tests in OAuth2ClientConfigurationTests starting at loadContextWhenPostProcessedThenRestOperationsRegistered() which demonstrates that the @Bean is registered by default and how a user could override the @Bean

Rather than relying on OAuth2ClientRestOperationsPostProcessor could we just default the RestOperations if none was found? This is what we do in other circumstances.

This is what we do in other circumstances

Can you please be specific and provide a code sample I could reference?

@rwinch Are you referring to the way we conditionally @Import using ImportSelector? For example, OAuth2ClientConfiguration.OAuth2ClientWebMvcImportSelector?

We often try to get a Bean if it doesn't exist we create a default instance. An example is PasswordEncoder

rwinch · 2020-06-22T17:46:57Z

.../springframework/security/config/annotation/web/configuration/OAuth2ClientConfiguration.java

+		@Autowired
+		@Qualifier(OAuth2ClientBeanNames.REST_OPERATIONS)
+		void configure(RestOperations restOperations) {
+			ClientRegistrations.setRestOperations(restOperations);


Setting a static method from a bean definition is not a typical pattern I find in Spring. Instead, I'd expect to be able to provide the RestOperations to the builder. A few concerns around this design:

First, it is not easy to provide different RestOperations for different issuers in a thread safe way. The builders should ideally be thread safe. This is especially true, given the performance improvements being made to Spring and talks around making bean creation multi-threaded.

Another and more immediate concern is that it appears that there is likely a bean ordering issue here. What guarantees this will happen before ClientRegistrations is used to create a client registration?

Setting a static method from a bean definition is not a typical pattern I find in Spring.

Understood. This is the simplest solution I came up. We're kind of in a bind with this class since it's not designed to be a @Bean and instead a utility class.

I'd expect to be able to provide the RestOperations to the builder

There is no builder in ClientRegistrations? I don't understand. Can you clarify what you mean here?

Honestly, I'm not a fan of this static setter either and would be fine not exposing it. But then how do we address #7027? Do we introduce #5543 and deprecate ClientRegistrations?

There is no builder in ClientRegistrations? I don't understand. Can you clarify what you mean here?

Sorry for being unclear. An option would be to introduce a builder to ClientRegistrations that allows passing in the RestOperations that would be used.

Honestly, I'm not a fan of this static setter either and would be fine not exposing it. But then how do we address #7027? Do we introduce #5543 and deprecate ClientRegistrations?

I think an option would be that Spring Security leverages the configured RestOperations to pass into the newly added builder in ClientRegistrations.

An option would be to introduce a builder to ClientRegistrations

I'm not sure how this would work @rwinch? Maybe I'm missing something.

ClientRegistrations was designed as a utility final class with static methods.

What would ClientRegistrations.Builder.build() return? An instance of ClientRegistrations?

jzheaux · 2020-06-24T15:54:32Z

.../springframework/security/config/annotation/web/configuration/OAuth2ClientConfiguration.java

+
+		@Bean
+		BeanDefinitionRegistryPostProcessor defaultOAuth2AuthorizedClientManagerPostProcessor() {
+			return new DefaultOAuth2AuthorizedClientManagerPostProcessor();


I'm having trouble seeing the value of registering OAuth2AuthorizedClientManager with a known bean name.

Doing so with RestOperations makes sense since there are many use cases when an application publishes a RestOperations but doesn't intend it to be used by Spring Security.

What is it about OAuth2AuthorizedClientManager that requires the same treatment as RestOperations? Is this a needed pattern for other OAuth 2.0 Client beans?

The OAuth2AuthorizedClientManager is the central interface for client since 5.2. It is used primarily by ServletOAuth2AuthorizedClientExchangeFilterFunction and OAuth2AuthorizedClientArgumentResolver. It indirectly uses RestOperations. The relationship chain is as follows:

OAuth2AuthorizedClientManager has 1 or more OAuth2AuthorizedClientProvider
OAuth2AuthorizedClientProvider has an OAuth2AccessTokenResponseClient
OAuth2AccessTokenResponseClient has a RestOperations

If an application configures a custom RestOperations then it will automatically be configured with the "default" OAuth2AuthorizedClientManager and associated OAuth2AuthorizedClientProvider's, and the user could leverage this default @Bean instead of explicitly configuring it. It really is meant to be a convenience mechanism.

One important thing to note is that an application may configure more than 1 OAuth2AuthorizedClientManager in their setup. For example, they may have a background service that performs client_credentials grant and another OAuth2AuthorizedClientManager that handles authorization_code and refresh_token in the web context.

The "default" OAuth2AuthorizedClientManager will be wired into the OAuth2AuthorizedClientArgumentResolver and may be used by ServletOAuth2AuthorizedClientExchangeFilterFunction.

jgrandja · 2021-11-18T16:49:27Z

Closing this PR as it needs more work. I'll submit a new PR when I get a chance to revisit this and apply the necessary changes.

Register RestOperations @bean for OAuth 2.0 Client

0bbf58f

Closes spring-projectsgh-5607

jgrandja requested review from jzheaux and rwinch June 19, 2020 20:20

jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement labels Jun 19, 2020

jgrandja added this to the 5.4.0-M2 milestone Jun 19, 2020

jgrandja mentioned this pull request Jun 22, 2020

OAuth2AuthorizedClientArgumentResolver could use OAuth2AuthorizedClientManager registered in context #8700

Closed

rwinch requested changes Jun 22, 2020

View reviewed changes

jgrandja added 2 commits June 24, 2020 09:02

Remove ClientRegistrations.setRestOperations()

19f57f9

Update sample

0a724f3

jzheaux reviewed Jun 24, 2020

View reviewed changes

jgrandja mentioned this pull request Jun 26, 2020

JwtDecoders and ReactiveJwtDecoders with customizable RestTemplate and WebClient #8690

Closed

jzheaux modified the milestones: 5.4.0-M2, 5.4.0-RC1 Jul 1, 2020

jgrandja mentioned this pull request Jul 28, 2020

Customize RestOperations / WebClient for OAuth 2.0 #8882

Closed

jgrandja removed this from the 5.4.0-RC1 milestone Jul 28, 2020

jgrandja self-assigned this Jul 28, 2020

jgrandja closed this Nov 18, 2021

jgrandja added the status: declined A suggestion or change that we don't feel we should currently apply label Nov 18, 2021

jgrandja deleted the gh-5607-default-restoperations branch July 21, 2023 11:51

Register RestOperations @Bean for OAuth 2.0 Client #8732

Register RestOperations @Bean for OAuth 2.0 Client #8732

Uh oh!

Conversation

jgrandja commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

OAuth 2.0 Client WebFlux

OAuth 2.0 Resource Server

OAuth 2.0 Resource Server WebFlux

JOSE

JOSE WebFlux

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja commented Nov 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jgrandja commented Jun 19, 2020 •

edited

Loading

jgrandja Jun 22, 2020 •

edited

Loading

jgrandja Jun 30, 2020 •

edited

Loading

jgrandja Jun 22, 2020 •

edited

Loading

jgrandja Jun 26, 2020 •

edited

Loading