Make PrePostAdviceReactiveMethodInterceptor more flexible in how it detects supported methods

[kaqqao]

Expected Behavior

PrePostAdviceReactiveMethodInterceptor should enable the user to customize how the supported methods are detected.

Current Behavior

The current logic checks whether the intercepted method returns a Publisher and, as of a couple of days ago, whether it is a Kotlin suspending function or returns Kotlin's Flow. It enables no customization by the user.

Context

Enabling a degree of extensibility to this logic could make Spring Security usable in more scenarios.

More supported kinds of methods could be added by framework/language/library authors as a part of their own Spring integration. Examples might include libraries (like Mutiny, which doesn't comply with the reactive streams spec), or languages (e.g. Scala or Groovy specific types, akin to Kotlin's Flow). An even more interesting example are workflow orchestration/BPMN engines, that invoke configured methods (usually reflectively) and commonly have their own interceptors and a shared context available around every invocation. This context can act as an alternative side-channel by which the Reactor context can be propagated even when the reactive chain is interrupted. This means that in such environments any method could potentially be wrapped in e.g. Mono with the Reactor context fully preserved. This exact scenario is the one I'm facing. I'm developing a Spring Boot starter around graphql-java, which (like all GraphQL environments) provides a shared context available around each method invocation. This enables me to store Reactor's context inside the GraphQL context, wrap any method invocation into a Mono and re-attach the Reactor context. So despite graphql-java having no support for reactive streams and working only with CompletableFutures, Spring's reactive security keeps working just fine as the chain of reactive calls remains effectively unbroken. I have a limited yet fully working implementation, but it requires the underlying method to return a Publisher for no other reason than to satisfy the checks in PrePostAdviceReactiveMethodInterceptor. If these could be extended somehow, any method would be supported. My current implementation provides no way for the wrapped methods to enrich/replace the Reactor context, but that is entirely possible too, with all the semantics properly preserved (like only downstream methods having the new context etc).

Additionally, Spring Security could itself use this extensibility to cleanly register more supported method types as they get added in the future. The current list consisting of Publisher-returning and Kotlin-specific methods is unlikely to stay fixed forever, and PrePostAdviceReactiveMethodInterceptor is already complicated enough with a few nested ifs containing entirely divergent logic. E.g. Flow-handling logic shares virtually nothing with the Mono-handling one.

Possible approaches

PrePostAdviceReactiveMethodInterceptor could delegate the check if the method is supported to an injectable strategy, that would also be an adapter to/from Publisher (like how Flow is currently adapted). Perhaps hooking into ReactiveAdapterRegistry more formally is the way to go...
Alternatively, maybe a special subtype of MethodInvocation, e.g. ReactiveAdaptedInvocation that itself acts as the strategy/adapter could be used.

I'd happily undertake this work if there is interest, but I'd really need some discussion on the approach first.

[rwinch]

An even more interesting example are workflow orchestration/BPMN engines, that invoke configured methods (usually reflectively) and commonly have their own interceptors and a shared context available around every invocation. This context can act as an alternative side-channel by which the Reactor context can be propagated even when the reactive chain is interrupted. This means that in such environments any method could potentially be wrapped in e.g. Mono with the Reactor context fully preserved.

Can you please demonstrate to me how this would work?

[kaqqao]

It is entirely case-specific and relies on the capabilities of the framework in question. But this is how I implemented it for GraphQL:

1. When a GraphQL request comes in:

Mono.deferContextual(ctx -> Mono.fromFuture(graphQL.executeAsync(buildInput(query, variables, ctx);

Note how Reactor's subscriber context is used as GraphQL context (in buildInput). GraphQL engine then propagates it to each field resolver function. In my case, all field resolvers are backed by a method that will be invoked reflectively.

2. Around each resolver invocation:

//dataFetchingEnvironment is provided to all resolvers by the GraphQL engine
return Mono.fromSupplier(() -> proceedWithReflectiveInvocation())
                    .contextWrite(dataFetchingEnvironment.getContext()) //restores the subscriber context
                    .toFuture(); //because graphql-java only works with CompletableFuture and isn't Reactor aware

And apart from a one-liner hack I made in PrePostAdviceReactiveMethodInterceptor (explained below) nothing else is needed to get Spring Security working, despite graphql-java knowing nothing about Reactor (and the reactive chain being interrupted because of that).
I can protect non-reactive (not returning Publisher) methods in WebFlux just fine, e.g.

@PreAuthorize("hasRole('ADMIN')")
@GraphQLQuery //Meaning this method will be called by the GraphQL engine (triggering the wrapping code from above)
public Project project(String id) {
    ...
}

I only had to trick PrePostAdviceReactiveMethodInterceptor into letting me get away with it, which I did by simply replacing method.getReturnType() in this line:

with

//Ignore the real return type on GraphQL methods, and pretend it's Mono
Class<?> returnType = method.isAnnotationPresent(GraphQLQuery.class) ? Mono.class : method.getReturnType();

This is of course a clumsy proof of concept, not a real implementation but, strictly speaking, my use-case would be fully satisfied by merely replacing Class<?> returnType = method.getReturnType() with Class<?> returnType = getReturnType(method).

Alternative implementation

I also have an alternative implementation where I trigger PrePostAdviceReactiveMethodInterceptor from my own interceptor and give it a special subtype of MethodInvocation that can override the return type. I then use it like this:

Class<?> returnType = invocation instancof ReactiveWrappedInvocation
    ? ((ReactiveWrappedInvocation) invocation).getReturnType();
    : method.getReturnType();

In this implementation, ReactiveWrappedInvocation itself contains the Mono-wrapping/context juggling code from above... The end result is the same. Interestingly, in this implementation any method invoked by the GraphQL engine can be protected by Spring Security, not only the ones on Spring-managed beans, but I'm absolutely not expecting exhibitions like that to be supported 😅 All I really need is the bare minimum - a way to tell the PrePostAdviceReactiveMethodInterceptor trust me, the result of proceed will be a Mono, even if the method return type says otherwise 😄.

Fair warning

I experimented quite a lot with different implementations, so I might have mixed something up in my examples. I'll push a fully working example somewhere if you're interested (including the hacked version of PrePostAdviceReactiveMethodInterceptor).

Conclusion

As you can see, this whole thing depends on the framework having its own context propagation and being the one invoking (at least) the non-reactive protected methods. So it can't be Spring invoking them like it usually would. Now, I completely understand this is an exotic scenario, but I find it common enough (as I mentioned, I've seen workflow engines having something similar, GraphQL too...) and the changes needed in Spring Security minuscule enough (and potentially useful for Spring's own internal extensibility), that it's easily worth it.

[rwinch]

The biggest problem is that we need to ensure that there is the glue code that allows stitching the contexts together. The example you have provided does demonstrate a way to do this, but is not fully baked. For example, how do we know the type is a Flux?

It may be of interest, but we are working on GraphQL + Spring (which will include Spring Security support) in https://github.com/spring-projects-experimental/spring-graphql

[kaqqao]

Not sure I'm following? If it's already a Flux, there's nothing to do, is there? Or you meant if the method result should be wrapped in a Flux instead of a Mono? That scenario is really no different at all. As long as the user can substitute the return type, they can do whatever is appropriate.
Anyway, I'm not asking for Spring Security to provide the glue code, just not to block me from providing it myself.

Would you be open to making a trivial addition to the interceptor, something along the lines of:

public Class<?> getReturnType(Method method) {
    return method.getReturnType();
}

? This would already open up a world of possibilities...

[rwinch]

Not sure I'm following? If it's already a Flux, there's nothing to do, is there?

Class<?> returnType = method.isAnnotationPresent(GraphQLQuery.class) ? Mono.class : method.getReturnType();

Why does GraphQLQuery mean it is a Mono and not a Flux?

Anyway, I'm not asking for Spring Security to provide the glue code, just not to block me from providing it myself.

We cannot just open it up without some sort of API that ensures the user knows what they are doing. Out of the box the code only supports Reactor return types, so we cannot just allow anything to go through. We'd need a strategy or something.

I'll push a fully working example somewhere if you're interested (including the hacked version of PrePostAdviceReactiveMethodInterceptor).

I'd be interested in seeing the fully working example

[kaqqao]

(I now realize I've edited my previous response before seeing your last message)

The example you have provided does demonstrate a way to do this, but is not fully baked.

Well... it's a proof of concept, that's exactly what it was supposed to do 😅 I opened this issue to discuss what a fully baked solution would look like.

Why does GraphQLQuery mean it is a Mono and not a Flux?

Was just proving a point, of course a real implementation wouldn't have such hard-coded expectations.

(In my case, Mono was a good choice here as its only purpose is to hold the subscriber context long enough for the interceptor to do its thing. The actual underlying value is already computed at that point (no actual reactivity) and Mono itself is immediately turned into a CompletableFuture as this whole problem stems from the surrounding framework not working with reactive types, so having a Flux there would've made no difference. But I do of course understand this might not be generally applicable.)

We cannot just open it up without some sort of API that ensures the user knows what they are doing.

Yes, of course, this is exactly what I want to discuss. To see if you'd be open to something like that, how would you go about implementing such a thing etc...

I'll try coming up with a proper implementation, so we have something more concrete to discuss.

[kaqqao]

Oh, and thanks for pointing me to the experimental starter! I wasn't aware of its existence. While I'm very fond of my own GraphQL SPQR (also my project) based starter, I might be able to use it to learn how to better integrate with certain Spring features, like Spring Security!
Since the new starter works with WebFlux, I can only imagine it must run into the same hurdle with subscriber context loss. Do you maybe know how this will be dealt with? By stitching the contexts like I'm doing, or perhaps a custom execution strategy or something else?

[rwinch]

We (the Spring and graphql teams) are still experimenting with the best way to handle the context. The first step is allowing users to return Mono and Flux and letting graphql work with an adapted version of it.

[kaqqao]

I see. In SPQR starter, I handled that the way I described above (no changes in PrePostAdviceReactiveMethodInterceptor needed in that case). But if you make further customizations (likely in graphql-java) to enable a more natural integration, I'd be very interested...
Actually, maybe I can be of use to that project, as I've been deeply involved with graphql-java and Spring its integration since its earliest days and have solved quite a few pain points already...

[rwinch]

I added a sample application that takes advantage of the newly added ReactorDataFetcherAdapter support in spring-graphql.

The SalaryService is protected using @PreAuthorize("hasRole('ADMIN')") and works without any customization to Spring Security now.

The preferred way to support other scenarios is to adapt to ReactorContext externally rather than within the method support. This demonstrates that this is possible for GraphQL java support and so I don't think this ticket is necessary. Does this resolve your specific concrete scenario? If so, I think we should close this issue.

[kaqqao]

(Closed the issue by accident)
Cool, thanks for this. It validates my approach.

As for this issue, it was really for the step beyond: protecting even the non-publisher returning methods. If in the ReactorDataFetcherAdapter you'd wrap a non-publisher in Mono, both Spring Security and graphql-java would still work. That is, if only PrePostAdviceReactiveMethodInterceptor would allow it. So I was asking for a way to safely tell the interceptor to make an exception for certain invocations (the invocations from GraphQL in this case). I've already had the case solved by the ReactorDataFetcherAdapter covered...

The problem is that the distinction would have to be made per invocation... GraphQL invocations should be exempted (because only GraphQL invocations would be adapted) while other invocations not... And that seems to not be solvable generically... So I guess we can close this issue and I'll have to stick to a custom implementation to trigger the interceptor for GraphQL only. I'm almost done with this, so I'll post an example soon.

[rwinch]

Closing as duplicate of gh-9401