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 #6587

Closed
wants to merge 1 commit into from

Conversation

aotitoola
Copy link

Fixes: gh-6477

@pivotal-issuemaster
Copy link

@d3jie Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@d3jie Thank you for signing the Contributor License Agreement!

@aotitoola aotitoola changed the title OAuth2LoginSpec discovers ReactiveOAuth2AccessTokenResponseClient @bean #6477 OAuth2LoginSpec discovers ReactiveOAuth2AccessTokenResponseClient @bean Mar 3, 2019
@jgrandja jgrandja self-assigned this Mar 4, 2019
@jgrandja jgrandja added in: config An issue in spring-security-config type: enhancement A general enhancement Reactive in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Mar 4, 2019
@jgrandja jgrandja added this to the 5.2.0.M2 milestone Mar 4, 2019
@jgrandja
Copy link
Contributor

jgrandja commented Mar 4, 2019

Thanks for the PR @d3jie! I applied some minor polish to the commit and merged to master!
Thanks again and we look forward to your next contribution :)

@jgrandja jgrandja closed this Mar 4, 2019
@aotitoola
Copy link
Author

Thanks @jgrandja. Can you tell me about the minor polish you applied? Maybe I can learn a thing or two. :)

@jgrandja
Copy link
Contributor

jgrandja commented Mar 5, 2019

Sure @d3jie

In createDefault(), you had...

ReactiveAuthenticationManager result = new OAuth2LoginReactiveAuthenticationManager(getAccessTokenResponseClient(), getOauth2UserService());

...

OidcAuthorizationCodeReactiveAuthenticationManager oidc = new OidcAuthorizationCodeReactiveAuthenticationManager(getAccessTokenResponseClient(), getOidcUserService());

I changed it to...

ReactiveOAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> client = getAccessTokenResponseClient();

ReactiveAuthenticationManager result = new OAuth2LoginReactiveAuthenticationManager(client, getOauth2UserService());

...

OidcAuthorizationCodeReactiveAuthenticationManager oidc = new OidcAuthorizationCodeReactiveAuthenticationManager(client, getOidcUserService());

So we re-use the client instance in both managers if the @Bean was not provided.

And inOAuth2LoginTests, I renamed the @Bean method from oAuth2AccessTokenResponseClient() to accessTokenResponseClient(). We typically don't prefix member variables or method declarations with oauth2*.

Those were the only 2 changes.

@aotitoola
Copy link
Author

@jgrandja I can see what you did there. It didn't occur to me that I could do it this way. Many thanks. Looking forward to the next. 👍

@aotitoola aotitoola deleted the gh-6477 branch March 7, 2019 07:59
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) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuth2LoginSpec discovers ReactiveOAuth2AccessTokenResponseClient @Bean
3 participants