Skip to content

ReactiveOAuth2AccessTokenResponseClients should support setting a custom WebClient #6182

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

Closed
jzheaux opened this issue Nov 28, 2018 · 17 comments · Fixed by #6238
Closed

ReactiveOAuth2AccessTokenResponseClients should support setting a custom WebClient #6182

jzheaux opened this issue Nov 28, 2018 · 17 comments · Fixed by #6238
Labels
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

@jzheaux
Copy link
Contributor

jzheaux commented Nov 28, 2018

WebClientReactiveAuthorizationCodeTokenResponseClient would be more convenient to use if their respective webClient properties could be set.

Let's add a setter, like:

/**
  Use the given {@link WebClient}.
  @param webClient The {@link WebClient} to use
  @since 5.2
*/
public void setWebClient(WebClient webClient) {
    Assert.notNull(webClient, "webClient cannot be null");
    this.webClient = webClient;
}

As well as tests to confirm that the setter works.

Related to #6051

@jzheaux jzheaux added type: enhancement A general enhancement 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 Nov 28, 2018
@jzheaux jzheaux added this to the 5.2.0.M1 milestone Nov 28, 2018
@shraiysh
Copy link
Contributor

Hey! I am a first timer and would like to handle this issue!

@rwinch
Copy link
Member

rwinch commented Nov 30, 2018

@shraiysh Thanks! The issue is yours. If you have any questions, please don't hesitate to let us know!

@shraiysh
Copy link
Contributor

shraiysh commented Nov 30, 2018

Hi, @rwinch I have never worked with spring boot applications. Can you please provide me a link to an example that could possibly use the required getter function, for testing?

Thanks!

@rwinch
Copy link
Member

rwinch commented Nov 30, 2018

@shraiysh You do not need a getter. The tests will be like those in WebClientReactiveClientCredentialsTokenResponseClientTests Does that help?

@richardvaldiviesomacias
Copy link

When there is another first time...I would love to try!

@rwinch
Copy link
Member

rwinch commented Dec 1, 2018

@richardvaldiviesomacias We'd love for you to contribute! Given the high degree of interest we post the first timer only tickets and let it be a first come first serve system, so keep an eye out for tickets flagged as first-timers-only.

PS: If you find another ticket you are interested in, don't let the lack of first-timers-only flag discourage you. We are glad to help you contribute to other tickets too. There is a Help Wanted label that are typically fairly simple issues too.

@shraiysh
Copy link
Contributor

shraiysh commented Dec 1, 2018

Hey @rwinch ! Thanks, that helped. I'm sorry for the typo there; I meant setter instead of getter! :D

@shraiysh
Copy link
Contributor

shraiysh commented Dec 1, 2018

Hey, @rwinch I might take a little longer to solve this issue because many things are new to me, but I will do it. If you have anything to guide me, that'll help. Right now, I'm reading about Tests and the docs on Spring security for this class. I will be trying to understand the working and then write the tests for this.

@richardvaldiviesomacias
Copy link

Thanks, @rwinch. I will take a look. Is any documentation for sending a PR. I mean is there any particular way to send a PR (Format, new of branch, etc.)

@shraiysh
Copy link
Contributor

shraiysh commented Dec 2, 2018

@richardvaldiviesomacias, I think this is the link you're looking for! Hope that helps.

@shraiysh
Copy link
Contributor

shraiysh commented Dec 3, 2018

Hi, I need some help! For testing custom webClient, I have written the following function

public void setCustomWebClientThenCustomWebClientIsUsed() {
	WebClient customClient = mock(WebClient.class);
	// This way, customClient.post() gives NullPointerException
	// So, I guess I want to use mock over WebClient.builder().build()

	this.tokenResponseClient.setWebClient(customClient);

	Mono<OAuth2AccessTokenResponse> response = this.tokenResponseClient.getTokenResponse(authorizationCodeGrantRequest());
	verify(customClient, atLeastOnce()).post();
}

I don't understand how I mock over it without changing the other classes. Also, if I use WebClient.builder().build(), verify cannot this object.

The solution that I came to was surrounding the Mono<OAuth2AccessTokenResponse> response = this.tokenResponseClient.getTokenResponse(authorizationCodeGrantRequest()); by a try-catch block.

WebClient customClient = mock(WebClient.class);
tokenResponseClient.setWebClient(customClient);
try {
	OAuth2AccessTokenResponse response = this.tokenResponseClient.getTokenResponse(authorizationCodeGrantRequest()).block();
} catch(Exception e) {
	// Do Nothing!
}
verify(customClient, atLeastOnce()).post();

Please let me know if this is right! Or if there's any other way!
Thanks

shraiysh added a commit to shraiysh/spring-security that referenced this issue Dec 3, 2018
@richardvaldiviesomacias
Copy link

@richardvaldiviesomacias, I think this is the link you're looking for! Hope that helps.
Thanks @shraiysh

@rwinch
Copy link
Member

rwinch commented Dec 3, 2018

@shraiysh You can use something like this to ensure you don't get a null value and then verify it was used here

@shraiysh
Copy link
Contributor

shraiysh commented Dec 4, 2018

Hey, @rwinch I am sorry for creating a mess with so many commits... Please check it (the last one - I have squashed others) and let me know if I should submit a pull request.

@rwinch
Copy link
Member

rwinch commented Dec 4, 2018

@shraiysh Thanks for the fast turnaround!

Can you please update the commit message to align with the Spring Security conventions?

A few notes:

  • Try to keep the subject as short as possible. This can be difficult or even impossible but try to keep it meaningful and concise
  • There is no need to add anything in the message about the tests since tests are required (thus implied)
  • Ensure the commit ends with Fixes: gh-6182
Add WebClientReactiveAuthorizationCodeTokenResponseClient.setWebClient

Fixes: gh-6182

shraiysh added a commit to shraiysh/spring-security that referenced this issue Dec 5, 2018
Add WebClientReactiveAuthorizationCodeTokenResponseClient.setWebClient

Fixes spring-projectsgh-6182
@shraiysh
Copy link
Contributor

shraiysh commented Dec 5, 2018

Thanks for the comment @rwinch. I have updated the commit message to

Add WebClientReactiveAuthorizationCodeTokenResponseClient.setWebClient

Fixes: gh-6182

Please let me know if there's an issue.

@rwinch
Copy link
Member

rwinch commented Dec 5, 2018

Looks good. Can you submit a pull request?

rwinch pushed a commit that referenced this issue Dec 6, 2018
Add WebClientReactiveAuthorizationCodeTokenResponseClient.setWebClient

Fixes gh-6182
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) status: first-timers-only An issue that can only be worked on by brand new contributors type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants