Summary

ReactiveSecurityContextHolder is broken when used with Futures. It does not always provide results and sometimes just fires the onComplete signal.

Actual Behavior

Executes onComplete()

Expected Behavior

Should execute onNext()

Version

5.0.7 Release

Sample

@Test
public void testWorkingContext() {
    Authentication authentication = new PreAuthenticatedAuthenticationToken("TEST", "");
    Mono<String> working = ReactiveSecurityContextHolder.getContext()
            .map(securityContext -> (String)securityContext.getAuthentication().getPrincipal());

    Mono<String> stringMono = working.subscriberContext(ReactiveSecurityContextHolder.withAuthentication(authentication));
    StepVerifier.create(stringMono).expectNext("TEST").verifyComplete();
}


@Test
public void testBrokenContext() {
    Authentication authentication = new PreAuthenticatedAuthenticationToken("TEST", "");
    Mono<String> working = ReactiveSecurityContextHolder.getContext()
            .map(securityContext -> (String)securityContext.getAuthentication().getPrincipal());
    Mono<String> broken = Mono.fromFuture(working.toFuture());
    Mono<String> stringMono = broken.subscriberContext(ReactiveSecurityContextHolder.withAuthentication(authentication));
    StepVerifier.create(stringMono).expectNext("TEST").verifyComplete();
}

Comment From: rwinch

Thanks for the report. If it is just an arbitrary Future I would not expect it to work. However, if the Future was produced from a Mono I would think it could work. If it were to work it would be a Project Reactor change.

What are your thoughts @simonbasle and @smaldini?

Comment From: dave-fl

@rwinch In my test, the CompletableFuture is produced from a Mono.

Comment From: rwinch

@dave-fl Right. This is why I didn't immediately close the issue since this seems somewhat reasonable to support to me.

In any case, this is something we will need Project Reactor to support. which is why I pinged Simon and Stephane. We will wait to see what their thoughts are.

Comment From: dave-fl

Another simple example.

@Test
public void simple() {
    Authentication authentication = new PreAuthenticatedAuthenticationToken("SIMPLE SIMON", "");
    Mono<String> context = ReactiveSecurityContextHolder.getContext().flatMap(securityContext -> {
        return Mono.just((String)securityContext.getAuthentication().getPrincipal());
    });

    Mono<String> wrappedWithSubscribe = Mono.create(monoSink -> context.subscribe(s -> monoSink.success(s), e -> monoSink.error(e), () -> monoSink.success()));
    Mono<String> addedContext = wrappedWithSubscribe
            .subscriberContext(ReactiveSecurityContextHolder.withAuthentication(authentication));

    StepVerifier.create(addedContext).expectNext("SIMPLE SIMON").verifyComplete();
}

Comment From: simonbasle

@dave-fl you are switching to a Future and then back to a Mono. The Context is purely a Reactor concept, and is propagated along Reactive Streams signals by Flux and Mono as long as you don't break the chain of operators (your second example with a subscribe breaks that assumption).

I gather you did that first test to simulate using an external Future-based API in the middle?

The MonoCompletionStage can probably be made aware of the Context of a MonoToFuture instance, but that would only help if you're guaranteed that external API doesn't wrap the Mono-produced Future, or change it in any way.

Comment From: dave-fl

@simonbasle Yes using Caffeine Async Cache - which uses futures. I don't believe it changes the future as its just storing what its being provided.

Comment From: dave-fl

Is there anything that can be done with the subscription to preserve the context?

Comment From: simonbasle

Is there anything that can be done with the subscription to preserve the context?

How do you mean? Context is accessed by an operator's internal Subscriber by looking at its immediate downstream Subscriber. If there is a stopgap in the reactive chain and there's no such Subscriber, then the Context cannot be accessed.

Comment From: dave-fl

@simonbasle I guess what I am asking you is that if one wanted to introduce their own Mono as I have done in this example, is there not a means to patch in that context so that chain is not broken e.g. do whatever it is that is normally done in the completion stage.

Comment From: simonbasle

The sink has a currentContext() method so you could do sink -> context.subscriberContext(sink.currentContext()).subscribe(...).

That said, subscribe inside create is an anti-pattern to me, and it achieves nothing. Probably just an attempt at reproducing the issue, but it fails by nature and not as a byproduct of a bug. (bit like saying "this code throws a NPE: throw new NullPointerException();" 😄).

Comment From: sepanniemi

The same problem seems to exist if I try to use SecurityContext in RxJava2 flow. 

Single.fromPublisher(ReactiveSecurityContextHolder.getContext())

Is there any way to access the current security context if using RxJava data types in WebFlux RestController?

I tried also with RxJava2Adapter and with Mono.toFuture, as said above the context is visible only when staying purely in reactor flow.

Comment From: simonbasle

Nope, the reactive metadata Context only works as long as the reactive sequence is made of Reactor operators

Comment From: sepanniemi

I was able to transfer the SecurityContext to RxJava flow using reactor Hooks and custom operator which copies the SecurityContext from reactor context to ThreadLocal as Single created from Mono. Needs some more testing, but looks promising for some of our existing services heavily using RxJava and soon migrating to SpringBoot2.

Comment From: lee-gilbert

Might this also provide a solution for retrieving the SecurityContext within Spring Data AuditorAware implementations?

Comment From: rwinch

@lee-gilbert I'm not sure. Can you provide more information?

Comment From: lee-gilbert

@rwinch It appears the issue is discussed without a solution here: https://jira.spring.io/browse/DATACMNS-1231

Comment From: mgumerov

Sorry, this might be a bit off topic but not by much because it questions if ReactiveSecurityContextHolder and Future are related at all. So, in my project I subscribe to ReactiveSecurityContextHolder.getContext in a bean (i.e. the subscription outlives a single chain), like @Bean Mono convertedPrincipal { return ReactiveSecurityContextHolder.getContext().map(...) } Turns out that for every new request the map() gets called again. Meaning, that desprite getContext() being Mono<>, it emits multiple values to the subscriber - although each chain only observes one value.

Doesn't that violate a Mono<> protocol? And anyway, clearly that is not compatible with Future<> is it?

Comment From: simonbasle

(sounds indeed a bit off-topic and potentially a separate issue / question, but here goes)

emits multiple values to the subscriber - although each chain only observes one value.

these statements seem to contradict each other. are you subscribing to the Mono multiple times?

the Mono contract of at most one onNext signal is like similar Reactive Streams contracts: it is only true in the context of a single Subscription - ie. for a specific Subscriber-Publisher relationship. A Mono/Publisher is like a Subscription factory, multiple subscriptions can happen.

Bear in mind that some Mono will explicitly reject more than one subscription (e.g. Monos representing a transient source that can be only read once, like a unique response in a Network I/O). Others will conceptually re-generate the source data ("cold" Monos - e.g. higher level "remote call" => perform the request again) and other will cache or just complete empty to late subscribers.

You should definitely not disconnect the getContext Mono from the request chain and make it a Bean IMHO. If you're already doing that, I assume you have implemented another kind of scope yourself, but it better be solid. You might have overlooked the potential cold and deferred nature of the getContext Mono (if someone from the @spring-projects/spring-security team can confirm it is cold).