Reactive Implementation of AuthorizedClientServiceOAuth2AuthorizedCli… #7589

ankurpathak · 2019-10-31T13:54:19Z

…entManager

ReactiveOAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager is reactive
version of AuthorizedClientServiceOAuth2AuthorizedClientManager

Fixes: gh-7569

jgrandja

Thanks for the PR @ankurpathak and for your patience waiting on me. Please see my comments.

jgrandja · 2019-11-11T16:09:34Z

...auth2/client/ReactiveOAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager.java

+ * @see ReactiveOAuth2AuthorizedClientService
+ * @since 5.3
+ */
+public final class ReactiveOAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager implements ReactiveOAuth2AuthorizedClientManager {


Let's rename to OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager

Apologies. I meant to say AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager - without OAuth2 prefix so the naming is consistent with existing AuthorizedClientServiceOAuth2AuthorizedClientManager

...auth2/client/ReactiveOAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager.java

jgrandja · 2019-11-11T16:31:49Z

...auth2/client/ReactiveOAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager.java

+											})
+											.build()
+				).flatMap(authorizationContext -> this.reactiveAuthorizedClientProvider.authorize(authorizationContext)
+						.doOnNext(x -> reactiveAuthorizedClientService.saveAuthorizedClient(x, principal))


Rename x to authorizedClient

Renamed to _authorizedClient as conflicting with local variable authorizedClient.

ghostd · 2019-11-25T12:54:51Z

Hi,

(not sure where i should post this comment... so i put it here)

I had to change some code of the class to make it work with my use case.

How i tested this PR (maybe wrongly?)

(in my code, i already renamed ReactiveOAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager to OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager)

    ReactiveClientRegistrationRepository clientRegistrationRepository = new InMemoryReactiveClientRegistrationRepository(clientRegistration);
    ReactiveOAuth2AuthorizedClientService authorizedClientService = new InMemoryReactiveOAuth2AuthorizedClientService(clientRegistrationRepository);

    OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager authorizedClientManager =
        new OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager(
                clientRegistrationRepository,
                authorizedClientService
        );
    authorizedClientManager.setReactiveOAuth2AuthorizedClientProvider(new SpecificPartnerReactiveOAuth2AuthorizedClientProvider());

The current implementation does not save the authorized client into the repository.

The code i changed to make it work

I changed

.doOnNext(x -> reactiveAuthorizedClientService.saveAuthorizedClient(x, principal))

to

.flatMap(authClient -> reactiveAuthorizedClientService.saveAuthorizedClient(authClient, principal).thenReturn(authClient))

Regards

ankurpathak · 2019-11-25T21:53:28Z

Hi,

(not sure where i should post this comment... so i put it here)

I had to change some code of the class to make it work with my use case.

How i tested this PR (maybe wrongly?)

(in my code, i already renamed ReactiveOAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager to OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager)
    ReactiveClientRegistrationRepository clientRegistrationRepository = new InMemoryReactiveClientRegistrationRepository(clientRegistration);
    ReactiveOAuth2AuthorizedClientService authorizedClientService = new InMemoryReactiveOAuth2AuthorizedClientService(clientRegistrationRepository);

    OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager authorizedClientManager =
        new OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager(
                clientRegistrationRepository,
                authorizedClientService
        );
    authorizedClientManager.setReactiveOAuth2AuthorizedClientProvider(new SpecificPartnerReactiveOAuth2AuthorizedClientProvider());
The current implementation does not save the authorized client into the repository.

The code i changed to make it work

I changed
.doOnNext(x -> reactiveAuthorizedClientService.saveAuthorizedClient(x, principal))
to
.flatMap(authClient -> reactiveAuthorizedClientService.saveAuthorizedClient(authClient, principal).thenReturn(authClient))
Regards

@ghostd I will wait for @jgrandja to review above commets and if your changes are required will do the needful.

…entManager ReactiveOAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager is reactive version of AuthorizedClientServiceOAuth2AuthorizedClientManager Fixes: spring-projectsgh-7569

ghostd · 2019-11-27T20:37:47Z

Hi,

I did some tests with the new version. It seems if DefaultContextAttributesMapper#apply returns (a mono of) an empty map, the authentication does not occur. If i change the DefaultContextAttributesMapper to return a non-empty map, the authentication succeeds.

jgrandja · 2019-12-02T16:34:52Z

...curity/oauth2/client/OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager.java

+									attributes.putAll(contextAttributes);
+								}).build())
+				).flatMap(authorizationContext -> this.authorizedClientProvider.authorize(authorizationContext)
+						.doOnNext(_authorizedClient -> authorizedClientService.saveAuthorizedClient(_authorizedClient, principal))


@ghostd Thanks for catching this...

.flatMap(authClient -> reactiveAuthorizedClientService.saveAuthorizedClient(authClient, principal).thenReturn(authClient))

@ankurpathak This bug was fixed in this commit.

Can you also apply the update to the test to make use of PublisherProbe<Void> saveAuthorizedClientProbe to ensure we cover the scenario of the Mono being subscribed to. See the commit that makes use of PublisherProbe in the test

jgrandja

Thanks for the updates @ankurpathak. Please see my comments.

jgrandja · 2019-12-02T16:42:03Z

...curity/oauth2/client/OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager.java

+
+		@Override
+		public Mono<Map<String, Object>> apply(OAuth2AuthorizeRequest authorizeRequest) {
+			ServerWebExchange serverWebExchange = authorizeRequest.getAttribute(ServerWebExchange.class.getName());


ServerWebExchange won't be available in this scenario so .defaultIfEmpty(Collections.emptyMap()) will execute every time. The logic should be similar to AuthorizedClientServiceOAuth2AuthorizedClientManager.DefaultContextAttributesMapper. Looks like a test is missing for this scenario, can you please add.

Also see this comment as the logic for .flatMap(contextBuilder -> this.contextAttributesMapper.apply(authorizeRequest) is incorrect. Please add a test for this as well. If contextAttributesMapper returns an empty Map, authorizedClientProvider.authorize() should still be called.

philsttr · 2019-12-02T19:08:52Z

Any chance this could be added in 5.2.x ?

philsttr · 2019-12-02T21:25:30Z

...y/oauth2/client/OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManagerTests.java

+				.build();
+		Mono<OAuth2AuthorizedClient> authorizedClient = this.authorizedClientManager.authorize(authorizeRequest);
+
+		authorizedClient.subscribe();


Don't subscribe() here. Instead, rely on StepVerifier subscribing (during verifyComplete()). Move the verifications and assertions after the StepVerifier.

The same comment applies to all these test methods where .subscribe() is explicitly called

ankurpathak · 2019-12-05T01:42:29Z

@jgrandja you can takeover this as I tried a lot on this but can't make it.

philsttr · 2019-12-05T16:59:03Z

I can pick this up to continue it.

jgrandja · 2019-12-05T19:12:43Z

@ankurpathak No worries and thank you for your work thus far.

@philsttr That would be great if you can take over this. Much appreciated.

philsttr · 2019-12-05T21:23:29Z

Followup PR created as #7702

Please close this PR (#7589) in favor of #7702

jgrandja · 2019-12-06T19:59:59Z

Closing in favour of #7702

Rename OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager to AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager. Handle empty mono returned from contextAttributesMapper. Handle empty map returned from contextAttributesMapper. Fix DefaultContextAttributesMapper so that it doesn't access ServerWebExchange. Fix unit tests so that they pass. Use StepVerifier in unit tests, rather than .subscribe(). Fixes spring-projectsgh-7569

Rename OAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager to AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager. Handle empty mono returned from contextAttributesMapper. Handle empty map returned from contextAttributesMapper. Fix DefaultContextAttributesMapper so that it doesn't access ServerWebExchange. Fix unit tests so that they pass. Use StepVerifier in unit tests, rather than .subscribe(). Fixes gh-7569

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 31, 2019

ankurpathak mentioned this pull request Oct 31, 2019

Provide reactive implementation of AuthorizedClientServiceOAuth2AuthorizedClientManager #7569

Closed

ankurpathak force-pushed the gh-7569-fix branch 5 times, most recently from 116f941 to 643fa9c Compare November 2, 2019 07:14

ankurpathak mentioned this pull request Nov 2, 2019

Build Failure Because of Gradle Task checkstyleNohttp #7595

Closed

eleftherias assigned jgrandja Nov 5, 2019

jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 11, 2019

jgrandja added this to the 5.3.x milestone Nov 11, 2019

jgrandja requested changes Nov 11, 2019

View reviewed changes

ankurpathak force-pushed the gh-7569-fix branch from 643fa9c to 3d55728 Compare November 25, 2019 21:51

ankurpathak force-pushed the gh-7569-fix branch from 3d55728 to e9c2081 Compare November 25, 2019 22:41

Reactive Implementation of AuthorizedClientServiceOAuth2AuthorizedCli…

9c8d027

…entManager ReactiveOAuth2AuthorizedClientServiceReactiveOAuth2AuthorizedClientManager is reactive version of AuthorizedClientServiceOAuth2AuthorizedClientManager Fixes: spring-projectsgh-7569

ankurpathak force-pushed the gh-7569-fix branch from e9c2081 to 9c8d027 Compare November 27, 2019 02:24

jgrandja reviewed Dec 2, 2019

View reviewed changes

jgrandja requested changes Dec 2, 2019

View reviewed changes

philsttr reviewed Dec 2, 2019

View reviewed changes

philsttr mentioned this pull request Dec 4, 2019

Introduce Reactive OAuth2Authorization success/failure handlers #7699

Closed

philsttr mentioned this pull request Dec 5, 2019

Reactive Implementation of AuthorizedClientServiceOAuth2AuthorizedClientManager (take 2) #7702

Closed

jgrandja removed this from the 5.3.x milestone Dec 6, 2019

jgrandja closed this Dec 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reactive Implementation of AuthorizedClientServiceOAuth2AuthorizedCli… #7589

Reactive Implementation of AuthorizedClientServiceOAuth2AuthorizedCli… #7589

ankurpathak commented Oct 31, 2019

jgrandja left a comment

jgrandja Nov 11, 2019

ankurpathak Nov 25, 2019

jgrandja Dec 2, 2019

jgrandja Nov 11, 2019

ankurpathak Nov 25, 2019

ghostd commented Nov 25, 2019

ankurpathak commented Nov 25, 2019 •

edited

Loading

How i tested this PR (maybe wrongly?)

The code i changed to make it work

ghostd commented Nov 27, 2019

jgrandja Dec 2, 2019 •

edited

Loading

jgrandja left a comment

jgrandja Dec 2, 2019

jgrandja Dec 2, 2019

philsttr commented Dec 2, 2019

philsttr Dec 2, 2019 •

edited

Loading

ankurpathak commented Dec 5, 2019 •

edited

Loading

philsttr commented Dec 5, 2019

jgrandja commented Dec 5, 2019

philsttr commented Dec 5, 2019

jgrandja commented Dec 6, 2019

Reactive Implementation of AuthorizedClientServiceOAuth2AuthorizedCli… #7589

Reactive Implementation of AuthorizedClientServiceOAuth2AuthorizedCli… #7589

Conversation

ankurpathak commented Oct 31, 2019

jgrandja left a comment

Choose a reason for hiding this comment

jgrandja Nov 11, 2019

Choose a reason for hiding this comment

ankurpathak Nov 25, 2019

Choose a reason for hiding this comment

jgrandja Dec 2, 2019

Choose a reason for hiding this comment

jgrandja Nov 11, 2019

Choose a reason for hiding this comment

ankurpathak Nov 25, 2019

Choose a reason for hiding this comment

ghostd commented Nov 25, 2019

How i tested this PR (maybe wrongly?)

The code i changed to make it work

ankurpathak commented Nov 25, 2019 • edited Loading

How i tested this PR (maybe wrongly?)

The code i changed to make it work

ghostd commented Nov 27, 2019

jgrandja Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

jgrandja left a comment

Choose a reason for hiding this comment

jgrandja Dec 2, 2019

Choose a reason for hiding this comment

jgrandja Dec 2, 2019

Choose a reason for hiding this comment

philsttr commented Dec 2, 2019

philsttr Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

ankurpathak commented Dec 5, 2019 • edited Loading

philsttr commented Dec 5, 2019

jgrandja commented Dec 5, 2019

philsttr commented Dec 5, 2019

jgrandja commented Dec 6, 2019

ankurpathak commented Nov 25, 2019 •

edited

Loading

jgrandja Dec 2, 2019 •

edited

Loading

philsttr Dec 2, 2019 •

edited

Loading

ankurpathak commented Dec 5, 2019 •

edited

Loading