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

OAuth2LoginSpec discovers ReactiveOAuth2AccessTokenResponseClient @Bean #6477

Closed
jgrandja opened this issue Jan 23, 2019 · 14 comments
Closed
Assignees
Labels
in: config An issue in spring-security-config in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: first-timers-only An issue that can only be worked on by brand new contributors type: enhancement A general enhancement
Milestone

Comments

@jgrandja
Copy link
Contributor

We should allow for a @Bean of type ReactiveOAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> to be discovered by OAuth2LoginSpec.

This will allow the user to register a WebClientReactiveAuthorizationCodeTokenResponseClient @Bean with a configured WebClient via WebClientReactiveAuthorizationCodeTokenResponseClient.setWebClient().

@jgrandja jgrandja added Reactive in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: first-timers-only An issue that can only be worked on by brand new contributors labels Jan 23, 2019
@jgrandja jgrandja added this to the 5.2.0.M2 milestone Jan 23, 2019
@jgrandja jgrandja added in: config An issue in spring-security-config type: enhancement A general enhancement labels Jan 23, 2019
@aotitoola
Copy link

Hello @jgrandja Does this have to do with autowiring that bean?

I’ll like to work on it regardless.

@jgrandja
Copy link
Contributor Author

@darthCodr Yes, it's all about wiring the ReactiveOAuth2AccessTokenResponseClient @Bean if the application has registered it, otherwise same behaviour as today. Take a look at getOauth2UserService() or getOidcUserService() - it will follow the same pattern.
Please add a test as well. Thanks for taking this on!

@aotitoola
Copy link

You are welcome.

@jgrandja you mentioned that this @Bean ReactiveOAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> will allow user to register WebClientReactiveAuthorizationCodeTokenResponseClient @Bean.

Will my implementation be in ReactiveAuthenticationManager createDefault() ? just like this check - if (oidcAuthenticationProviderEnabled) ?

Also, I have looked at 'getOauth2UserService()' and 'getOidcUserService()'. Is there a particular method name you would like me to use?

I currently have something like this:

		private ReactiveOAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> getOAuth2.......() {
			ResolvableType type = ResolvableType.forClassWithGenerics(ReactiveOAuth2AccessTokenResponseClient.class, OAuth2AuthorizationCodeGrantRequest.class);
			ReactiveOAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> bean = getBeanOrNull(type);

			if (bean ==  null) {
				return new _DefaultReactiveOAuth2UserService();_
			}

			return bean;
		}

If the bean is null, should I return null? ... as DefaultReactiveOAuth2UserService() is not the same type as ReactiveOAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest>. What would you like me to return when the bean is not found?

Maybe I didn't understand what you meant by "follow the same pattern". Would appreciate if you could explain more.

Many thanks.

@jgrandja jgrandja self-assigned this Jan 28, 2019
@jgrandja
Copy link
Contributor Author

@darthCodr

Will my implementation be in createDefault()

Yes, this is exactly where you would apply the change.

Is there a particular method name you would like me to use?

Let's go with ReactiveOAuth2AccessTokenResponseClient getAccessTokenResponseClient()

If the bean is null, should I return null?

It should behave as it currently does today, which is defaulting to WebClientReactiveAuthorizationCodeTokenResponseClient client = new WebClientReactiveAuthorizationCodeTokenResponseClient() in createDefault().

@aotitoola
Copy link

aotitoola commented Jan 30, 2019

Hello, @jgrandja
I currently have this:


private ReactiveOAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> getAccessTokenResponseClient() {
			ResolvableType type = ResolvableType.forClassWithGenerics(ReactiveOAuth2AccessTokenResponseClient.class, OAuth2AuthorizationCodeGrantRequest.class);
			ReactiveOAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> bean = getBeanOrNull(type);
			if (bean ==  null) {
				return new WebClientReactiveAuthorizationCodeTokenResponseClient();
			}
			return bean;
		}

At what point do I invoke/call getAccessTokenResponseClient()?

@jgrandja
Copy link
Contributor Author

jgrandja commented Jan 30, 2019

You would call it in createDefault() and pass it when instantiating OAuth2LoginReactiveAuthenticationManager and OidcAuthorizationCodeReactiveAuthenticationManager

@aotitoola
Copy link

Hello @jgrandja I have implemented this and it's fine.
I'm a bit confused as to where to implement the tests. Kindly assist. Many thanks.

@jgrandja
Copy link
Contributor Author

jgrandja commented Feb 1, 2019

The tests should go in OAuth2LoginTests. Thanks.

@aotitoola
Copy link

aotitoola commented Feb 5, 2019

Hello @jgrandja. I found some existing tests in OAuth2LoginTests that check ReactiveOAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest>. I don't know how to test it without messing up existing code. Is there an implementation I could use as a guide? I would appreciate.

I know this might be an urgent fix. Should I do a PR regardless then work through the tests together with you from there?

Sincere apologies for the delay.

@jgrandja
Copy link
Contributor Author

jgrandja commented Feb 6, 2019

@darthCodr

I know this might be an urgent fix. Should I do a PR regardless then work through the tests together with you from there?

No worries, it's not urgent. It's planned for 5.2.0.M2 so we have some time. The tests need to go with the PR so let's keep working through it.

Is there an implementation I could use as a guide?

Take a look at OAuth2LoginTests.oauth2LoginWhenCustomJwtDecoderFactoryThenUsed().

Note how it registers:

		@Bean
		public ReactiveJwtDecoderFactory<ClientRegistration> jwtDecoderFactory() {
			return jwtDecoderFactory;
		}

and jwtDecoderFactory was declared as:

ReactiveJwtDecoderFactory<ClientRegistration> jwtDecoderFactory = spy(new JwtDecoderFactory());

and than in the test we verify that the actual @Bean got called:

verify(config.jwtDecoderFactory).createDecoder(any());

This is the same type of test logic you need to apply for ReactiveOAuth2AccessTokenResponseClient.

Let me know if this helps?

@aotitoola
Copy link

Thanks a lot for your help @jgrandja
I did a PR gh-6530

Kindly check. Thanks.

@aotitoola
Copy link

Hello @jgrandja the PR failed. I can't place what I did wrong. I would appreciate your help with the tests. Many thanks.

@jgrandja
Copy link
Contributor Author

@darthCodr The issue is with this line

verify(tokenResponseClient).getTokenResponse(any()).thenReturn(Mono.just(accessTokenResponse))

change it to the following and it will pass

verify(tokenResponseClient).getTokenResponse(any())

You will notice a few lines above the verify we mock the response with when

when(tokenResponseClient.getTokenResponse(any())).thenReturn(Mono.just(accessTokenResponse))

Also, since you're putting the test in oauth2LoginWhenCustomJwtDecoderFactoryThenUsed we should rename it to keep it generic so we can reuse in this instance. Please rename the following:

oauth2LoginWhenCustomJwtDecoderFactoryThenUsed -> oauth2LoginWhenCustomBeansThenUsed

OAuth2LoginWithJwtDecoderFactoryBeanConfig -> OAuth2LoginWithCustomBeansConfig

Thank you!

@aotitoola
Copy link

@jgrandja All checks finally passed. Grateful for the help and the opportunity to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: first-timers-only An issue that can only be worked on by brand new contributors type: enhancement A general enhancement
Projects
None yet
2 participants