Skip to content

Fix NPE in RequestContextSubscriber #7235

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 30, 2019
+168 −32
Merged

Fix NPE in RequestContextSubscriber #7235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 77 additions & 31 deletions ...ent/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.reactivestreams.Subscription;
import org.springframework.beans.factory.DisposableBean;
import org.springframework.beans.factory.InitializingBean;
import org.springframework.lang.Nullable;
import org.springframework.security.authentication.AnonymousAuthenticationToken;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.GrantedAuthority;
Expand Down Expand Up @@ -95,6 +96,7 @@
*
* @author Rob Winch
* @author Joe Grandja
* @author Roman Matiushchenko
* @since 5.1
* @see OAuth2AuthorizedClientManager
*/
Expand Down Expand Up @@ -174,7 +176,7 @@ private static OAuth2AuthorizedClientManager createDefaultAuthorizedClientManage

@Override
public void afterPropertiesSet() throws Exception {
Hooks.onLastOperator(REQUEST_CONTEXT_OPERATOR_KEY, Operators.lift((s, sub) -> createRequestContextSubscriber(sub)));
Hooks.onLastOperator(REQUEST_CONTEXT_OPERATOR_KEY, Operators.liftPublisher((s, sub) -> createRequestContextSubscriberIfNecessary(sub)));
}

@Override
Expand Down Expand Up @@ -378,14 +380,22 @@ private Mono<ClientRequest> mergeRequestAttributesFromContext(ClientRequest requ
}

private void populateRequestAttributes(Map<String, Object> attrs, Context ctx) {
if (ctx.hasKey(HTTP_SERVLET_REQUEST_ATTR_NAME)) {
attrs.putIfAbsent(HTTP_SERVLET_REQUEST_ATTR_NAME, ctx.get(HTTP_SERVLET_REQUEST_ATTR_NAME));
}
if (ctx.hasKey(HTTP_SERVLET_RESPONSE_ATTR_NAME)) {
attrs.putIfAbsent(HTTP_SERVLET_RESPONSE_ATTR_NAME, ctx.get(HTTP_SERVLET_RESPONSE_ATTR_NAME));
}
if (ctx.hasKey(AUTHENTICATION_ATTR_NAME)) {
attrs.putIfAbsent(AUTHENTICATION_ATTR_NAME, ctx.get(AUTHENTICATION_ATTR_NAME));
RequestContextDataHolder holder = RequestContextSubscriber.getRequestContext(ctx);
if (holder != null) {
HttpServletRequest request = holder.getRequest();
if (request != null) {
attrs.putIfAbsent(HTTP_SERVLET_REQUEST_ATTR_NAME, request);
}

HttpServletResponse response = holder.getResponse();
if (response != null) {
attrs.putIfAbsent(HTTP_SERVLET_RESPONSE_ATTR_NAME, response);
}

Authentication authentication = holder.getAuthentication();
if (authentication != null) {
attrs.putIfAbsent(AUTHENTICATION_ATTR_NAME, authentication);
}
}
}

Expand Down Expand Up @@ -472,7 +482,7 @@ private ClientRequest bearer(ClientRequest request, OAuth2AuthorizedClient autho
.build();
}

private <T> CoreSubscriber<T> createRequestContextSubscriber(CoreSubscriber<T> delegate) {
<T> CoreSubscriber<T> createRequestContextSubscriberIfNecessary(CoreSubscriber<T> delegate) {
HttpServletRequest request = null;
HttpServletResponse response = null;
ServletRequestAttributes requestAttributes =
Expand All @@ -482,6 +492,10 @@ private <T> CoreSubscriber<T> createRequestContextSubscriber(CoreSubscriber<T> d
response = requestAttributes.getResponse();
}
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
if (authentication == null && request == null && response == null) {
//do not need to create RequestContextSubscriber with empty data
return delegate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. it ensures that no null values are propagated to the reactor context.
  2. As reactor context recommended to be as small as possible (see javadocs) this fix reduces number of used entries from 4 to 1/0.
  3. 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.
  4. 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?

}
return new RequestContextSubscriber<>(delegate, request, response, authentication);
}

Expand Down Expand Up @@ -553,34 +567,37 @@ private UnsupportedOperationException unsupported() {
}
}

private static class RequestContextSubscriber<T> implements CoreSubscriber<T> {
private static final String CONTEXT_DEFAULTED_ATTR_NAME = RequestContextSubscriber.class.getName().concat(".CONTEXT_DEFAULTED_ATTR_NAME");
static class RequestContextSubscriber<T> implements CoreSubscriber<T> {
static final String REQUEST_CONTEXT_DATA_HOLDER =
RequestContextSubscriber.class.getName().concat(".REQUEST_CONTEXT_DATA_HOLDER");
private final CoreSubscriber<T> delegate;
private final HttpServletRequest request;
private final HttpServletResponse response;
private final Authentication authentication;
private final Context context;

private RequestContextSubscriber(CoreSubscriber<T> delegate,
HttpServletRequest request,
HttpServletResponse response,
Authentication authentication) {
RequestContextSubscriber(CoreSubscriber<T> delegate,
HttpServletRequest request,
HttpServletResponse response,
Authentication authentication) {
this.delegate = delegate;
this.request = request;
this.response = response;
this.authentication = authentication;

Context parentContext = this.delegate.currentContext();
Context context;
if (parentContext.hasKey(REQUEST_CONTEXT_DATA_HOLDER)) {
context = parentContext;
} else {
context = parentContext.put(REQUEST_CONTEXT_DATA_HOLDER, new RequestContextDataHolder(request, response, authentication));
}

this.context = context;
}

@Nullable
private static RequestContextDataHolder getRequestContext(Context ctx) {
return ctx.getOrDefault(REQUEST_CONTEXT_DATA_HOLDER, null);
}

@Override
public Context currentContext() {
Context context = this.delegate.currentContext();
if (context.hasKey(CONTEXT_DEFAULTED_ATTR_NAME)) {
return context;
}
return Context.of(
CONTEXT_DEFAULTED_ATTR_NAME, Boolean.TRUE,
HTTP_SERVLET_REQUEST_ATTR_NAME, this.request,
HTTP_SERVLET_RESPONSE_ATTR_NAME, this.response,
AUTHENTICATION_ATTR_NAME, this.authentication);
return this.context;
}

@Override
Expand All @@ -603,4 +620,33 @@ public void onComplete() {
this.delegate.onComplete();
}
}

static class RequestContextDataHolder {
private final HttpServletRequest request;
private final HttpServletResponse response;
private final Authentication authentication;

RequestContextDataHolder(@Nullable HttpServletRequest request,
@Nullable HttpServletResponse response,
@Nullable Authentication authentication) {
this.request = request;
this.response = response;
this.authentication = authentication;
}

@Nullable
private HttpServletRequest getRequest() {
return this.request;
}

@Nullable
private HttpServletResponse getResponse() {
return this.response;
}

@Nullable
private Authentication getAuthentication() {
return this.authentication;
}
}
}
92 changes: 91 additions & 1 deletion ...eb/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@
import org.springframework.web.reactive.function.BodyInserter;
import org.springframework.web.reactive.function.client.ClientRequest;
import org.springframework.web.reactive.function.client.WebClient;
import reactor.core.CoreSubscriber;
import reactor.core.publisher.BaseSubscriber;
import reactor.core.publisher.Mono;
import reactor.util.context.Context;

import java.net.URI;
import java.time.Duration;
Expand All @@ -84,6 +88,7 @@
import java.util.function.Consumer;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
import static org.mockito.Mockito.*;
import static org.springframework.http.HttpMethod.GET;
Expand Down Expand Up @@ -144,9 +149,10 @@ public void setup() {
}

@After
public void cleanup() {
public void cleanup() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SecurityContextHolder.clearContext();
RequestContextHolder.resetRequestAttributes();
this.function.destroy();
}

@Test
Expand Down Expand Up @@ -633,6 +639,90 @@ public void filterWhenRequestAttributesNotSetAndHooksInitHooksResetThenDefaultsN
assertThat(getBody(request)).isEmpty();
}

// gh-7228
@Test
public void afterPropertiesSetWhenHooksInitAndOutsideWebSecurityContextThenShouldNotThrowException() throws Exception {
this.function.afterPropertiesSet(); // Hooks.onLastOperator() initialized
assertThatCode(() -> Mono.subscriberContext().block())
.as("RequestContext Hook brakes application outside of web/security context")
.doesNotThrowAnyException();
}

@Test
public void createRequestContextSubscriberIfNecessaryWhenOutsideWebSecurityContextThenReturnOriginalSubscriber() throws Exception {
BaseSubscriber<Object> originalSubscriber = new BaseSubscriber<Object>() {};
CoreSubscriber<Object> resultSubscriber = this.function.createRequestContextSubscriberIfNecessary(originalSubscriber);
assertThat(resultSubscriber).isSameAs(originalSubscriber);
}

// gh-7228
@Test
public void createRequestContextSubscriberWhenRequestResponseProvidedThenCreateWithParentContext() throws Exception {
testRequestContextSubscriber(new MockHttpServletRequest(), new MockHttpServletResponse(), null);
}

// gh-7228
@Test
public void createRequestContextSubscriberWhenAuthenticationProvidedThenCreateWithParentContext() throws Exception {
testRequestContextSubscriber(null, null, this.authentication);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test where RequestContextSubscriber constructor uses parentContext

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

``

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Test
public void createRequestContextSubscriberWhenParentContextHasDataHolderThenShouldReuseParentContext() throws Exception {
RequestContextDataHolder testValue = new RequestContextDataHolder(null, null, null);
final Context parentContext = Context.of(RequestContextSubscriber.REQUEST_CONTEXT_DATA_HOLDER, testValue);
BaseSubscriber<Object> parent = new BaseSubscriber<Object>() {
@Override
public Context currentContext() {
return parentContext;
}
};

RequestContextSubscriber<Object> requestContextSubscriber =
new RequestContextSubscriber<>(parent, null, null, authentication);

Context resultContext = requestContextSubscriber.currentContext();

assertThat(resultContext)
.describedAs("parent context was replaced")
.isSameAs(parentContext);
}

private void testRequestContextSubscriber(MockHttpServletRequest servletRequest,
MockHttpServletResponse servletResponse,
Authentication authentication) {
String testKey = "test_key";
String testValue = "test_value";

BaseSubscriber<Object> parent = new BaseSubscriber<Object>() {
@Override
public Context currentContext() {
return Context.of(testKey, testValue);
}
};

RequestContextSubscriber<Object> requestContextSubscriber =
new RequestContextSubscriber<>(parent, servletRequest, servletResponse, authentication);

Context resultContext = requestContextSubscriber.currentContext();

assertThat(resultContext)
.describedAs("result context is null")
.isNotNull();

assertThat(resultContext.getOrEmpty(testKey))
.describedAs("context is replaced")
.hasValue(testValue);

Object dataHolder = resultContext.getOrDefault(RequestContextSubscriber.REQUEST_CONTEXT_DATA_HOLDER, null);
assertThat(dataHolder)
.describedAs("context is not populated with REQUEST_CONTEXT_DATA_HOLDER")
.isNotNull()
.hasFieldOrPropertyWithValue("request", servletRequest)
.hasFieldOrPropertyWithValue("response", servletResponse)
.hasFieldOrPropertyWithValue("authentication", authentication);
}

private static String getBody(ClientRequest request) {
final List<HttpMessageWriter<?>> messageWriters = new ArrayList<>();
messageWriters.add(new EncoderHttpMessageWriter<>(new ByteBufferEncoder()));
Expand Down