ServletOAuth2AuthorizedClientExchangeFilterFunction supports chaining #6526

jgrandja · 2019-02-12T20:03:23Z

Fixes gh-6483

jgrandja · 2019-02-12T20:11:12Z

@rwinch A couple of things for you to consider during the review.

Do we need to add Hooks.resetOnLastOperator()
I noticed that RequestContextSubscriber is instantiated a few times along with the call to currentContext()...I'm wondering if this behaviour is expected? Please double check this on your end.

rwinch

I provided feedback inline. We also need some tests added.

rwinch · 2019-02-12T21:37:03Z

...client/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunction.java

-				.map(authorizedClient -> bearer(request, authorizedClient))
-				.flatMap(next::exchange)
-				.switchIfEmpty(next.exchange(request));
+		if (!attribute.isPresent()) {


This code is the same as the else block. It should be able to be restructured like: try and get the authorized client, if not there default it, process (same process in either case).

Applied in latest commit

rwinch · 2019-02-12T21:44:38Z

...client/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunction.java

+				.map(ClientRequest.Builder::build);
+	}
+
+	private void populateRequestAttributes(Map<String, Object> attrs, Context ctx) {


Do we want Context to override potentially existing attrs?

I original had attrs.putIfAbsent() but didn't see any reason for it. Do you anticipate a potential issue with overriding? The attributes in the Context will always be what the filter needs so I don't see any issue with this. What do you think?

Applied putIfAbsent()

My concern was if someone had a custom Authentication they were working with, it might be explicitly set as an attribute. Perhaps this is so that a global access token is used for client_credentials access tokens.

I see. Ok so putIfAbsent() is needed...and it was updated in latest commit

rwinch · 2019-02-12T21:57:02Z

...client/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunction.java

@@ -169,9 +176,23 @@ public void setDefaultClientRegistrationId(String clientRegistrationId) {
 	 * @return the {@link Consumer} to configure the builder
 	 */
 	public Consumer<WebClient.Builder> oauth2Configuration() {
+		Hooks.onLastOperator(REQUEST_CONTEXT_OPERATOR_KEY, Operators.lift((s, sub) -> createRequestContextSubscriber(sub)));


We can make this implement DisposableBean to clean up the hooks

Not sure I understand? The ExchangeFilterFunction would not really be a @Bean. The WebClient would be but that's not always the case either. What were you thinking exactly?

We need a way to clean up the Hooks and a DisposableBean makes a lot of sense for that reason. Why wouldn't/couldn't it be a Bean? How would you clean it up?

I agree DisposableBean is an option and yes we need to cleanup/release the Hook. I just don't understand who would be responsible for registering the ExchangeFilterFunction as a @Bean? Would this be required by the user to register? What if they don't register it? Am I missing something from what you are suggesting?

The user is already in charge of creating the ExchangeFilterFunction and registering it with the WebClient. This just means they should create it as a Bean.

If they forget, then it is the same problem as any other DisposableBean that they didn't register as a Bean. Things end up in an inconsistent state. This scenario is really more important when the JVM process runs longer than the ApplicationContext (i.e. tests) I doubt it will cause a lot of issues, but it is not semantically correct to leave the Hook registered.

If you don't think DisposableBean is a good idea, what do you think we should do to ensure it gets cleaned up?

This just means they should create it as a Bean.

As a user I would question why do I need to register it as a @Bean?

then it is the same problem as any other DisposableBean that they didn't register as a Bean

Yes but if the user is registering a user-defined @Bean and they have implemented a cleanup/release of resources than they know why they need to register it as a @Bean. With the ExchangeFilterFunction, the framework is doing the cleanup so it's not as obvious to them that they need to register it. It will have to be well documented so it's not missed.

it is not semantically correct to leave the Hook registered.

Agreed, or else the Context will contain the request attributes for every sequence created which is not good.

We need to do something for sure. I'm thinking PreDestroy instead of DisposableBean since the framework recommends this approach as well. What do you think?

Fixes spring-projectsgh-6483

jgrandja · 2019-02-13T03:32:03Z

@rwinch Test has been added and applied all feedback comments except for this comment. I'll apply that after you reply with more detail.

jgrandja · 2019-02-13T16:58:09Z

Merged via 0c27f64

robotmrv · 2019-08-03T09:21:45Z

...client/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunction.java

+			if (context.hasKey(CONTEXT_DEFAULTED_ATTR_NAME)) {
+				return context;
+			}
+			return Context.of(


@jgrandja @rwinch
This subscriber applies for each reactive chain by Hooks.onLastOperator() globally
Context does not alllow null values (see reactor/reactor-core#1800)
what if Mono.subscribe()/block() is executed out of security or web context (rabbitmq listener or just some scheduled task...)?

robotmrv · 2019-08-03T09:34:36Z

...client/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunction.java

+			if (context.hasKey(CONTEXT_DEFAULTED_ATTR_NAME)) {
+				return context;
+			}
+			return Context.of(


Context javadoc states

* Note that contexts are optimized for low cardinality key/value storage, and a user * might want to associate a dedicated mutable structure to a single key to represent his * own context instead of using multiple {@link #put}, which could be more costly. * Past five user key/value pair, the {@link Context} will use a copy-on-write * implementation backed by a new {@link java.util.Map} on each {@link #put}.

https://github.com/reactor/reactor-core/blob/master/reactor-core/src/main/java/reactor/util/context/Context.java#L36-L44

but this subscriber creates 4 keys (occupies almost all "optimmized" implementations of Context)
and as far as I understand CONTEXT_DEFAULTED_ATTR_NAME key is just a marker

Maybe it would be better to put all this attrs into some holder so you can reduce number of keys from 4 to 1. And it allows remove extra CONTEXT_DEFAULTED_ATTR_NAME

I mean that it is not just spring security who populates context and it could impact overall performance if context is populated by other libraries frequently

@robotmrv Thanks for the report. Can you create a ticket or a a PR for this?

@rwinch
I've created new issue
please see #7228

and here is PR #7235

jgrandja requested a review from rwinch February 12, 2019 20:03

rwinch requested changes Feb 12, 2019

View reviewed changes

ServletOAuth2AuthorizedClientExchangeFilterFunction supports chaining

6cf8513

Fixes spring-projectsgh-6483

jgrandja force-pushed the 5.1.x branch from 28eb0d9 to 6cf8513 Compare February 13, 2019 03:27

jgrandja added in: web An issue in web modules (web, webmvc) type: bug A general bug in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Feb 13, 2019

jgrandja added this to the 5.2.0.M2 milestone Feb 13, 2019

jgrandja closed this Feb 13, 2019

rwinch assigned jgrandja Feb 14, 2019

robotmrv reviewed Aug 3, 2019

View reviewed changes

robotmrv mentioned this pull request Aug 6, 2019

RequestContextSubscriber could put null value in Reactor Context #7228

Closed

ServletOAuth2AuthorizedClientExchangeFilterFunction supports chaining #6526

ServletOAuth2AuthorizedClientExchangeFilterFunction supports chaining #6526

Uh oh!

Conversation

jgrandja commented Feb 12, 2019

Uh oh!

jgrandja commented Feb 12, 2019

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja Feb 13, 2019 • 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!

jgrandja commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgrandja commented Feb 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robotmrv Aug 3, 2019 • 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!

Uh oh!

jgrandja Feb 13, 2019 •

edited

Loading

jgrandja commented Feb 13, 2019 •

edited

Loading

robotmrv Aug 3, 2019 •

edited

Loading