Support reactive method security with reactive types

[edeandrea]

From #4841 - copying in last comment from that issue into this PR, while adding some comments. What is in this PR is probably not the final version - there will probably be some re-work, but I want to settle on a few design decisions before re-working a bit:

We don't want to remove tests that already exist (i.e. EnableReactiveMethodSecurityTests) This runs the risk of breaking backwards compatibility.

Focus on as few changes as possible. For example, ReactiveMessageService has methods removed from it.

In my opinion - the tests that were removed don't really make sense for reactive types. Things like

@PostAuthorize("returnObject?.contains(authentication?.name)")

when the return type is a Mono / Flux / Publisher don't make a whole lot of sense. Spring-el doesn't yet support lambda functions within expressions so calling methods on those types within an expression doesn't make much sense to me.

I also added a ton of new functional/integration (see ReactiveMethodSecurityConfigurationTests) tests to cover lots of the scenarios.

We cannot remove a constructor because it is a breaking change. The old constructor should be adapted to work with the new reactive APIs and a new constructor should be added.

I'm unsure of how to "adapt" the old constructor and its arguments (PreInvocationAuthorizationAdvice and PostInvocationAuthorizationAdvice) to work with the new reactive api (ReactivePreInvocationAuthorizationAdvice and ReactivePostInvocationAuthorizationAdvice).

• Their method signatures are completely different (blocking types vs reactive types)
• Functionally their implementations are very different (blocking (using ExpressionUtils) vs non-blocking (using ReactiveExpressionUtils))
• It isn't enough to change these arguments - the body of the invoke method needs to change as well in order to work with the reactive streams

What I ended up doing was leaving PrePostAdviceReactiveMethodInterceptor alone & marking the class itself as deprecated and introducing a new ReactivePrePostAdviceMethodInterceptor class which uses the new reactive pre/post advice implementations.

If you have a clearer picture of how to adapt the existing pre/post advice interfaces into the new reactive ones then I'm all ears - I just don't see what they have in common to be able to programmatically adapt the old ones to the new ones.

We do not want to make SimpleMethodInvocation mutable. This adds concerns around concurrency

I made that change because of the way the MethodInvocation itself works. In the pre-advice scenario we need to be able to alter the arguments that are "fed" into MethodInvocation.proceed(). In the blocking scenario, DefaultMethodSecurityExpressionHandler.filter actually mutates the arguments in place, assuming the argument is a Collection. In the reactive world we don't have that luxury - the reactive types are all immutable and can't be modified in place. So in order to get around that we need to be able to supply a different set of arguments completely (i.e. a Mono / Flux with a filterWhen attached to it).

The only solution I was able to come up with was in ExpressionBasedReactivePreInvocationAuthorizationAdvice.setArguments. I thought it best to be able to support all the various implementations of MethodInvocation as possible (not just ProxyMethodInvocation). That's why I added the setArguments method to SimpleMethodInvocation.

Not to mention all the tests I wrote for testing all this functionality use a sub-class of SimpleMethodInvocation, so without it none of my tests will work either.

I can refactor a bit and take SimpleMethodInvocation out of the picture but then the whole pre-advice will only work correctly if the MethodInvocation is a ProxyMethodInvocation. I don't know enough about the rest of the codebase to know whether that is acceptable or not.

The only other solution I thought about but I feel would be a somewhat larger undertaking would be to change the implementation of the ReactivePrePostAdviceMethodInterceptor altogether and use the AspectJ classes (ProceedingJoinPoint) instead of the Spring AOP MethodInvocation. From there instead of only having a single MethodInvocation.proceed() method, there would be a ProceedingJoinPoint.proceed(Object[]) method that we could use to supply different arguments. This would also involve some re-structuring of the way the pre-advice class works as well, since right now it only has a single before method that returns Mono<Boolean>. I think we would instead need 2 methods - 1 for beforeAuthorize and one for beforeFilter.

I didn't go that route originally though because I didn't see AspectJ being used anywhere and I wanted to keep the implementation on the reactive side as close as possible to the non-reactive side.

I'm open to ideas though - just wanted to put some context into why I made some of the decisions that I did.

[edeandrea]

I pushed a new commit (& rebased previous one) to bring back the tests that were removed. I changed the expression

@PostAuthorize("returnObject?.contains(authentication?.name)")

to be

@PostAuthorize("@authz.containsAuthenticationName(returnObject, authentication?.name)")

and added some methods in the new ReactiveAuthz class to handle the calls.

[rwinch]

I submitted some more feedback.

I don't really have a lot of time to figure out what my advice is on how to proceed at this point. We cannot accept a breaking change (so we should not modify tests) since we preserve backwards comparability.

What I typically do in these scenarios is take a step back and just add a single test that I need to fix (without modifying existing tests). Then I try and trace through the code where it fails and find a fix that doesn't introduce a breaking change. Often when taking the step back I copy paste snippets of the original code.

I suspect the end solution will need to look at the result of the SpEL expression and see if the type is a Publisher or a boolean and adapt from there (i.e. if a boolean, then just wrap in Mono.just(booleanSpelResult))

[rwinch]

Changing the tests means we are potentially breaking backwards compatibility. We need to leave tests alone and ensure that any changes that we make don't break the existing tests.

[rwinch]

Again please leave existing tests as they are. Instead create only new tests that demonstrate the new functionality.

[rwinch]

This looks like a race condition because each subscription to the publisher (which can happen on any thread) is going to modify the same rootObject

[rwinch]

We cannot add a reactive interface to an implementation that is used in imperative world because we will get NoClassDefFoundErrors

[edeandrea]

Thank you @rwinch for the feedback. I'm sure you are busy so your feedback is much appreciated.

I think there is a disconnect between my understanding of how the expressions are supposed to behave vs the current implementation on the reactive-side as well as the current implementation on the non-reactive-side, specifically around the use of returnObject (or more generally the returnedObject parameter & return to the current ExpressionBasedPostInvocationAdvice.after method).

I'll use the example above here

@PostAuthorize("returnObject?.contains(authentication?.name)")
public Flux<String> fluxPostAuthorizeFindById(long id)

As things work today before any changes I made, in the above example returnObject is actually a reference to each item in the returned Flux, not the Flux itself (same if the return is a Mono). ExpressionBasedPostInvocationAdvice.after is called for each item in the Flux instead of once with the Flux itself. My understanding of how method security expressions work is that returnObject should be a reference to the Flux (or Mono) itself.

All the work I did in this PR assumes that returnObject is a reference to the Flux / Mono itself, and not the elements within it.

If we were to use the same example on the blocking side and write the expression

@PostAuthorize("returnObject?.contains(authentication?.name)")
public List<String> listPostAuthorizeFindById(long id)

then in that case returnObject references the List itself, not the elements in the List (I built a test in GlobalMethodSecurityConfigurationTests which can confirm this - ExpressionBasedPostInvocationAdvice.after is only called once & the returnedObject passed in is indeed the List itself).

Based on that, plus the way the documentation is written

Less commonly, you may wish to perform an access-control check after the method has been invoked. This can be achieved using the @PostAuthorize annotation. To access the return value from a method, use the built-in name returnObject in the expression.

I would say that the returnObject that currently is used for each item in the Flux / Mono is actually a bug because functionally it does not work the same as it does on the blocking side. If returnObject is indeed intended to be a reference to the Mono / Flux itself, then the expression returnObject?.contains(authentication?.name) is invalid and will never compile/execute because there is no contains method on a Mono / Flux object (hence the reason I modified the tests to change the expression).

If the decision is that the behavior of returnObject should indeed be the items inside the Flux / Mono rather than the Flux / Mono itself, then yes adapting the old interface to the new reactive one becomes much simpler. In that scenario though I think the documentation would need to be updated to reflect that there is an implementation difference in what returnObject actually means in the reactive world vs the blocking world.

In my opinion I think functionally it should behave the same in both blocking & reactive, which would mean the change being made as part of this PR would not be functionally backwards-compatible in some use cases. Ultimately its your call and I'll adapt to whichever way you want to go with it. If left as-is though, that would mean that the post-expression evaluation would get re-run for each & every item in the returned Flux / Mono instead of just once. If the expression makes I/O calls/etc could become very expensive to the implementer if they are only assuming their post-expression is only running once.

[rwinch]

The values are currently being resolved because SpEL doesn't support working with the Publisher types. If returnObject refers to the publisher type (i.e. Mono or Flux) how does the user leverage that in their SpEL expressions without being forced to use a Bean?

[sjohnr]

Hello @edeandrea. This PR has been open quite some time without activity, and is currently in conflict. I know you worked quite hard on what is here, but reviewing comments, it doesn't seem like a conclusion was quite reached and quite a bit of additional work would be required it seems. Given all of the above, I'm going to close this PR in a few days, but wanted to give you a heads up first just in case.

[edeandrea]

Thank you @sjohnr you are correct it has been quite some time. I do not have plans to resume working on it.

[sjohnr]

Thanks for your quick reply. I will go ahead and close this for now.