Fix NPE in RequestContextSubscriber #7235

robotmrv · 2019-08-07T21:52:01Z

RequestContextSubscriber could cause NPE if Mono/Flux.subscribe
was invoked outside of Web Context.
In addition it replaced source Context with its own without respect
to old data.
Now Request Context Data is Propagated within holder class and
it is added to existing reactor Context

Fixes gh-7228

pivotal-issuemaster · 2019-08-07T21:52:03Z

@robotmrv 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 · 2019-08-07T22:01:01Z

@robotmrv Thank you for signing the Contributor License Agreement!

rwinch

Thanks for the PR! I added feedback inline

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

robotmrv · 2019-08-10T07:42:24Z

I have doubts about ServletOAuth2AuthorizedClientExchangeFilterFunction.createRequestContextSubscriber()
It creates RequestContextSubscriber on every subscribe()/block() even outside of Security or WebContext and propagates empty DataHolder.
Maybe it would be better to create RequestContextSubscriber only in case if (authentication != null || requestAttributes != null) ?

rwinch · 2019-08-12T21:46:44Z

@robotmrv - It looks like something needs to change to me as well. The ThreadLocal should only be invoked inside of defaultRequest which is guaranteed to be invoked on the original Servlet container Thread. I'm going to let @jgrandja take a look at this since he worked on some of the cleanup in this class.

jgrandja

Thank you for these improvements @robotmrv! I've left some comments inline

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

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

jgrandja · 2019-08-20T21:18:57Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

+
+	// gh-7228
+	@Test
+	public void publisherWhenHooksInitAndSubscribedOutsideOfWebContextAndOutsideOfSecurityContextAndWithParentContextThenShouldNotReplaceParentContextAndShouldNotAddRequestContext() throws Exception {


I'm not sure about this test and the next one. It's quite low level and not directly testing the API of ServletOAuth2AuthorizedClientExchangeFilterFunction. Most tests are directly testing filter() and defaultRequest(), whereas these 2 tests are directly using the Reactor API's.

The preference would be to change these tests so they are aligned with the existing filter() tests.

The main issue was that current instrumentation affects all application
So this tests check if it is possible to performe operations on Publishers without web/sucurity context with initialized reactor instrumentation (which is ServletOAuth2AuthorizedClientExchangeFilterFunction functionality)

Both of these tests do not directly test the API of ServletOAuth2AuthorizedClientExchangeFilterFunction but instead are testing internal implementation details. In order to simplify the tests, please make RequestContextSubscriber package-private along with the constructor. This will allow you to test the constructed state of RequestContextSubscriber using currentContext(). Please remove these 2 tests after you have added the new ones. Thanks.

as this PR fixes scenario without web/security context (see #7235 (comment) ) and in this scenario ExchangeFilterFunction API of ServletOAuth2AuthorizedClientExchangeFilterFunction is not used
but ServletOAuth2AuthorizedClientExchangeFilterFunction#afterPropertiesSet is used and it is part of API it is makes sense to test these scenarios without usage of ExchangeFilterFunction part of API.

Other scenarios with direct usage of ExchangeFilterFunction API are covered by existing tests.
I will make RequestContextSubscriber package-private but test will not change very much as they could not use ExchangeFilterFunction API by scenario.

These test could be moved to a new test class but I do not see point in this as they still test ServletOAuth2AuthorizedClientExchangeFilterFunction#afterPropertiesSet API.

robotmrv · 2019-08-21T17:08:06Z

@jgrandja I've fixed codestyle according to convention standard and removed publisherWhenHooksInitAndSubscribedOutsideOfWebContextThenShouldNotThrowNpe()
Regarding other tests:

They check reactor Hook which is part of the ServletOAuth2AuthorizedClientExchangeFilterFunction
RequestContextSubscriber is low level implementation which should be checked
these tests cover scenarios from RequestContextSubscriber could put null value in Reactor Context #7228 affect of global reactor hook on whole application - it should not affect unrelated functionality (e.g. reactive DB init on application startup etc.)
I do not know how to test it better
so I leave them as is
If you still have concerns about them please let me know.

jgrandja

Thanks for the updates @robotmrv. I've left a couple comments inline

jgrandja · 2019-08-22T14:25:07Z

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

@@ -484,6 +494,9 @@ private ClientRequest bearer(ClientRequest request, OAuth2AuthorizedClient autho
 			response = requestAttributes.getResponse();
 		}
 		Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+		if (authentication == null && request == null && response == null) {
+			return delegate;


This is misleading since it doesn't create and return a RequestContextSubscriber instance, but instead returns the "current" Subscriber. I think we should return null for these cases and ensure Hooks.onLastOperator is NOT called in afterPropertiesSet(). Please add a test for this as well.

Hooks.onLastOperator() - applies global lift function on subscribe()/block() and it should be better installed once per application life circle.
As Lift function is invoked only at the moment of subscribe() lazily (not at the moment of application context / Filter initialization) it is not possible to unsure is there web/security context or not.

Not sure we are on the same page with case I try to fix
One of the cases:
I want to initialize application with some data. I have my reactive service API to do it.
I make ApplicationRunner or just Application listener which invokes my reactive API on startup and call subscribe()/block().
So If my API implementation uses reactor context (e.g. reactive transactions, or just some custom implementation) with initialized hook from filter and without current fix I will get NPE, because RequestContextSubscriber will populate reactor context with null values (as there is no Web and Security Context) and this is forbidden.

Another scenario - my reactive API is invoked form RabbitMQ listener and there is no web context - as the result NPE.

So what PR fixes:

it ensures that no null values are propagated to the reactor context.

As reactor context recommended to be as small as possible (see javadocs) this fix reduces number of used entries from 4 to 1/0.

RequestContextSubscriber replaced original context so data from other hooks could be lost. This fix ensures that RequestContextSubscriber will not replace all parent context but adds values to it.

In described case RequestContextDataHolder will be created with null values and will be useless as it is not required at all for the operation (application initialization), so it is make sense not to populate reactor context with useless data because of p.2 and not to create additional Subscriber to reduce allocation and invocation stack size on each emit or currentContext()

Lift function can't return null as later we will get NPE so delegate subscriber looks fine to me as it means NOOP lift function.

@jgrandja, do you still want to return null from lift function and create RequestContextSubscriber in any case with empty RequestContextDataHolder?

jgrandja · 2019-08-22T14:33:23Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

@@ -146,9 +153,10 @@ public void setup() {
 	}

 	@After
-	public void cleanup() {
+	public void cleanup() throws Exception {


Please revert the 2 changes in cleanup() as I believe it was added as a result of the removed test.

No.
It removes reactor Hook from the filter, i.e. makes cleanup of the global state.
So it should be better here.

jgrandja · 2019-08-22T14:42:48Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

+
+	// gh-7228
+	@Test
+	public void publisherWhenHooksInitAndSubscribedOutsideOfWebContextAndOutsideOfSecurityContextAndWithParentContextThenShouldNotReplaceParentContextAndShouldNotAddRequestContext() throws Exception {


Both of these tests do not directly test the API of ServletOAuth2AuthorizedClientExchangeFilterFunction but instead are testing internal implementation details. In order to simplify the tests, please make RequestContextSubscriber package-private along with the constructor. This will allow you to test the constructed state of RequestContextSubscriber using currentContext(). Please remove these 2 tests after you have added the new ones. Thanks.

robotmrv · 2019-08-28T21:44:15Z

@jgrandja ,
I've updated tests, please check

jgrandja

Thanks for the updates @robotmrv! I left a couple of other minor requests.

jgrandja · 2019-08-29T19:17:17Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

@@ -635,6 +641,69 @@ public void filterWhenRequestAttributesNotSetAndHooksInitHooksResetThenDefaultsN
 		assertThat(getBody(request)).isEmpty();
 	}

+	// gh-7228
+	@Test
+	public void afterPropertiesSetWhenHooksInitAndPublisherSubscribedOutsideOfWebContextAndOutsideOfSecurityContextThenSubscriptionShouldNotThrowException() throws Exception {


Let's shorten this name to afterPropertiesSetWhenHooksInitAndOutsideWebSecurityContextThenShouldNotThrowException

jgrandja · 2019-08-29T19:23:05Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

+	}
+
+	@Test
+	public void createRequestContextSubscriberIfNecessaryWhenOutsideOfWebContextAndOutsideOfSecurityContextThenShouldReturnOriginalSubscriber() throws Exception {


Please rename to createRequestContextSubscriberIfNecessaryWhenOutsideWebSecurityContextThenReturnOriginalSubscriber

jgrandja · 2019-08-29T19:32:38Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

+
+	// gh-7228
+	@Test
+	public void requestSubscriberWhenHasRequestAndHasResponseThenShouldAddToParentContextRequestContextDataHolder() throws Exception {


Please rename to createRequestContextSubscriberWhenRequestResponseProvidedThenCreateWithParentContext

jgrandja · 2019-08-29T19:33:45Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

+
+	// gh-7228
+	@Test
+	public void requestSubscriberWhenHasAuthenticationThenShouldAddToParentContextRequestContextDataHolder() throws Exception {


Please rename to createRequestContextSubscriberWhenAuthenticationProvidedThenCreateWithParentContext

jgrandja · 2019-08-29T19:37:42Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

+	public void requestSubscriberWhenHasAuthenticationThenShouldAddToParentContextRequestContextDataHolder() throws Exception {
+		testRequestContextSubscriber(null, null, this.authentication);
+	}
+


Can you also add a test where RequestContextSubscriber constructor uses parentContext

if (parentContext.hasKey(REQUEST_CONTEXT_DATA_HOLDER)) { context = parentContext; ``

@jgrandja
I've added test and renamed existing as you suggested.
please check

jgrandja

Thanks @robotmrv. I left one minor request. After you apply the update can you please rebase off master and squash commits. Thank you.

jgrandja · 2019-08-30T12:59:10Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

 		testRequestContextSubscriber(null, null, this.authentication);
 	}

+	@Test
+	public void createRequestContextSubscriberWhenParentContextHasDataHolderThenShouldReuseParentContext() throws Exception {
+		RequestContextDataHolder testValue = mock(RequestContextDataHolder.class);


Instead of a mock, we can just set an empty RequestContextDataHolder instance

@jgrandja done

RequestContextSubscriber could cause NPE if Mono/Flux.subscribe() was invoked outside of Web Context. In addition it replaced source Context with its own without respect to old data. Now Request Context Data is Propagated within holder class and it is added to existing reactor Context if Holder is not empty. Fixes spring-projectsgh-7228

jgrandja · 2019-08-30T20:34:03Z

Thank you for all the updates @robotmrv. This is now in master.

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

robotmrv mentioned this pull request Aug 7, 2019

ServletOAuth2AuthorizedClientExchangeFilterFunction supports chaining #6526

Closed

rwinch requested changes Aug 9, 2019

View reviewed changes

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java Outdated Show resolved Hide resolved

rwinch assigned jgrandja Aug 12, 2019

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

jgrandja requested changes Aug 20, 2019

View reviewed changes

jgrandja requested changes Aug 22, 2019

View reviewed changes

jgrandja requested changes Aug 29, 2019

View reviewed changes

jgrandja requested changes Aug 30, 2019

View reviewed changes

robotmrv force-pushed the gh-7228 branch from a1f9c80 to ffc43e0 Compare August 30, 2019 13:52

jgrandja added this to the 5.2.0.RC1 milestone Aug 30, 2019

jgrandja approved these changes Aug 30, 2019

View reviewed changes

jgrandja merged commit ffc43e0 into spring-projects:master Aug 30, 2019

Fix NPE in RequestContextSubscriber #7235

Fix NPE in RequestContextSubscriber #7235

Uh oh!

Conversation

robotmrv commented Aug 7, 2019

Uh oh!

pivotal-issuemaster commented Aug 7, 2019

Uh oh!

pivotal-issuemaster commented Aug 7, 2019

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robotmrv commented Aug 10, 2019

Uh oh!

rwinch commented Aug 12, 2019

Uh oh!

jgrandja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

robotmrv commented Aug 21, 2019

Uh oh!

jgrandja 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!

robotmrv commented Aug 28, 2019

Uh oh!

jgrandja 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!

jgrandja 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!

jgrandja commented Aug 30, 2019

Uh oh!

Uh oh!