Summary
Reactive method security requires the SpEL expression to return Boolean which does not work of the logic to determine access is blocking. We should allow the result to be Mono<Boolean>
Comment From: edeandrea
Based on your comment on this issue the fact that this ticket remains open does that mean it is not possible to do the following? Do the methods referenced within the expression in the @PreAuthorize annotations still need to return boolean over Mono<Boolean>, therefore meaning you can't perform any blocking operations within the method?
@Component
public class SomeBean {
public Mono<Boolean> someMethod() {
return Mono.fromRunnable(() -> Mono.just(true));
}
}
@RestController
public class SomeController {
@GetMapping("/somePath")
@PreAuthorize("@someBean.someMethod()")
public SomePojo getSomething() {
return new SomePojo();
}
}
Comment From: edeandrea
I believe I've answered my own question and the answer is that I can not do what I posted above.
Seems what is missing in Spring Security are reactive versions of ExpressionBasedPreInvocationAdvice & ExpressionBasedPostInvocationAdvice (as well as reactive versions of base-level interfaces PreInvocationAuthorizationAdvice & PostInvocationAuthorizationAdvice).
Am I correct in that assessment?
Comment From: rwinch
That and we need a reactive equivalent of PrePostAdviceReactiveMethodInterceptor
Comment From: edeandrea
Isn't PrePostAdviceReactiveMethodInterceptor already reactive? Seems like its reactive in the sense that its looking in the ReactiveSecurityContextHolder for the Authentication, but still calling the non-reactive PreInvocationAuthorizationAdvice/PostInvocationAuthorizationAdvice. The wordReactive in the class name is misleading :)
Would it be better to create a new version of PrePostAdviceReactiveMethodInterceptor or to modify the existing one to support the reactive pre/post advice classes?
Comment From: edeandrea
Would you be open to a PR for this or is it something you are already actively working on?
Comment From: rwinch
Isn't PrePostAdviceReactiveMethodInterceptor already reactive?
You are right but it would need to accept the reactive advice.
Would it be better to create a new version of PrePostAdviceReactiveMethodInterceptor or to modify the existing one to support the reactive pre/post advice classes?
Modify the existing one, deprecate the existing constructor (modify it to just adapt to use the reactive APIs to simplify the code), add a new constructor w/ reactive equivalents
Would you be open to a PR for this or is it something you are already actively working on?
I'd love a PR for this.
Comment From: edeandrea
After looking at this and getting most of the way through it - its not as simple as originally planned. It stems from the fact that org.springframework.security.access.expression.ExpressionUtils assumes the return type from any expression evaluation is a boolean and doesn't take into account reactive types. I needed to start by creating a ReactiveExpressionUtils which can do that.
public final class ReactiveExpressionUtils {
/**
* Evaluates an {@link Expression} that can either return a {@code boolean} or a {@code Mono<Boolean>}.
* @param expr The {@link Expression}
* @param ctx The {@link EvaluationContext}
* @return A {@link Mono} that can be subscribed to containing the result of the expression
*/
public static Mono<Boolean> evaluateAsBoolean(Expression expr, EvaluationContext ctx) {
return Mono.defer(() -> {
try {
Object value = expr.getValue(ctx);
if (value instanceof Boolean) {
return Mono.just((Boolean) value);
}
else if (value instanceof Mono) {
return ((Mono<?>) value)
.filter(Boolean.class::isInstance)
.cast(Boolean.class)
.switchIfEmpty(createInvalidTypeMono(expr));
}
else {
return createInvalidTypeMono(expr);
}
}
catch (EvaluationException ex) {
return Mono.error(new IllegalArgumentException(String.format("Failed to evaluate expression '%s': %s", expr.getExpressionString(), ex.getMessage()), ex));
}
});
}
private static Mono<Boolean> createInvalidTypeMono(Expression expr) {
return Mono.error(new IllegalArgumentException(String.format("Expression '%s' needs to either return boolean or Mono<Boolean> but it does not", expr.getExpressionString())));
}
}
From there we also need a reactive version of DefaultMethodSecurityExpressionHandler because the filter method assumes it is modifying/mutating Collections/Arrays, whereas in a reactive version this would simply be adding a filter condition to a Flux (@PreFilter / @PostFilter don't really make sense for anything but a Flux IMO). I created an intermediary base class and moved most of the stuff from DefaultMethodSecurityExpressionHandler down and created a reactive version of it that has a different implementation of the filter method
I should have something in the next few days to share. Just need to finish a couple things & apply some polish.
Comment From: edeandrea
Hi @rwinch I'm just about done but the last piece I'm stuck on is how to handle @PreFilter. The current ExpressionBasedPreInvocationAdvice.findFilterTarget only supports pre-filtering things that are Collections. This is because at the intercept point (the MethodInvocation) we have to actually mutate the parameter so that the mutated version is what gets passed as the method argument.
In the reactive version I am building, the argument wouldn't be a Collection - it would be a Flux or Mono. This leaves us in the same situation as if the argument were an Array - there is nothing I can do to effectively mutate the existing Flux/Mono argument and apply a filterWhen to it, so that it gets passed into the actual method.
I've been scratching my head on the best way to handle that but I'm just not sure how to do it. I thought I would ask for help. Do we punt on supporting @PreFilter for reactive type arguments for now? Since Mono/Flux types are immutable I just don't see a way to mutate it in place so that the mutated version gets supplied as the argument to the target method, nor do I see a way to provide a new one as the argument to the target method.
Essentially what we need is a MethodInvocation.proceed method that takes arguments (similar to what I've used before when using AspectJ's org.aspectj.lang.ProceedingJoinPoint.proceed(Object[] args)). I don't see that option here though.
Comment From: edeandrea
Little more investigating - I can do something I consider somewhat dirty by doing something like
if (ProxyMethodInvocation.class.isAssignableFrom(mi.getClass())) {
args[0] = this.expressionHandler.filter(filterTarget, preFilter, ctx);
((ProxyMethodInvocation) mi).setArguments(args);
}
This would work in the case where the Mono or Flux was the only argument in the method signature - but it involves type checking that the MethodInvocation is an instance of ProxyMethodInvocation.
I'm still not sure how to handle the other case where the user used the filterTarget attribute within the @PreFilter annotation.
Comment From: rwinch
Can you point me to your work in progress with a test that I can run that illustrates the problem you are having?
Comment From: edeandrea
Hi @rwinch I've committed everything to the 4841-reactive-method-security branch in my local repo, including new tests for all new functionality. The entire source tree builds to success. I have not yet raised a PR though as I wanted to get your feedback first.
If you take a look at ExpressionBasedReactivePreInvicationAuthorizationAdvice, you'll notice the need to be able to set the arguments back on the MethodInvocation. I couldn't really think of another way to do it other than do a type check on the type of MethodInvocation. Not the cleanest I know but its the only solution I could think of in order to make @PreFilter on reactive types work.
There are an entire suite of unit tests within ExpressionBasedReactivePreInvocationAuthorizationAdviceTests as well as integration tests in ReactiveMethodSecurityConfigurationTests.
Comment From: rwinch
Thanks for the look at the code. I may be easier to submit a PR so that feedback can be provided. However, I will provide feedback here this time:
- We do not want to make SimpleMethodInvocation mutable. This adds concerns around concurrency
- We don't want to remove tests that already exist (i.e. EnableReactiveMethodSecurityTests) This runs the risk of breaking backwards compatibility.
- 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.
- Focus on as few changes as possible. For example, ReactiveMessageService has methods removed from it.
I'd like to see the above resolved and a PR submitted so we can more easily discuss.
Comment From: edeandrea
Thanks @rwinch for the feedback. I'll work in some of the changes and submit as a PR.
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.
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.
We can certainly discuss further once I get a PR going.
Comment From: rwinch
It should support the non-reactive return type as it did before though.
We can certainly discuss further once I get a PR going.
:+1:
Comment From: edeandrea
Thanks @rwinch we can take the comments over to #5980 if you'd like. I made some changes before creating that PR and also added some more comments there.
Comment From: pdanelson
The linked PR seems to have stalled. Are there any plans to continue with this in the near future or is it very unlikely to make it into the 5.2 release?
In its current state, reactive method security is close to unusable for securing REST endpoints, as certain blocking operations like database calls are pretty much unavoidable for all but the most basic access control scenarios.
Comment From: edeandrea
I just have not had a chance to get back to the PR unfortunately.
Comment From: eusokolov
Also waiting for this PR
Comment From: edeandrea
Sorry I just have not had time to get back to this. I don't work for Spring - I just do this in my spare time. I'll see if I can allocate some time to get back to this.
Comment From: rwinch
@edeandrea If it is something that you aren't able to find time for, is this something that you feel might be worth seeing if someone else has time to contribute?
Comment From: edeandrea
@rwinch I'm fine to hand this off or collaborate with someone else that may have more time for this. Unfortunately I just got really busy with other things & haven't had a chance to come back to it. If anyone else would like to take over from where I left off I'm fine with that.
Comment From: rwinch
Thanks for the fast response. It's ok you haven't had time...we all appreciate the transparency. I just wanted to find out if that was alright by you. Now that it is out there, we can see if anyone else might have the time to pick this up.
Comment From: edeandrea
I do have some locally un-committed changes I've made based on some of the other review comments. I'm happy to post them to my own fork under a separate branch for others to look at as well and/or use as a reference.
Comment From: masoudparvari
Hi @rwinch , is there any estimation on when this feature will be in ?
Comment From: bollywood-coder
Rob Winch, any idea of when we can see this feature in Spring Security?
Comment From: bollywood-coder
Or is anybody working on this (community or from the spring team) ?
Comment From: rwinch
There is no update on it. If people are interested in it, please react with :+1: to the original report. Better yet, if you are interested in contributing, please let us know
Comment From: bollywood-coder
Hi Rob, I would be interested in. However, I could not start before December.
Comment From: RobMaskell
@rwinch just so I can stop looking, is that what the thymeleaf guys are waiting for before something like <div data-th-if="${#authorization.expression('hasRole(''ROLE_ADMIN'')')}"> will work in a webflux / spring security app?
Comment From: rwinch
I'm not sure. The issue is that we need Mono<Boolean> in the spel expression. So if the return type is a Mono, then possibly. However, I'm not sure how it would help theymeleaf since Thymeleaf requires everything to be resolved before the template is rendered (nothing subscribes to the template).
Comment From: RobMaskell
Thanks for the response @rwinch. I'm trying to trace what might be causing this error from a webflux / spring boot / spring security / okta / thymeleaf app using all the starters and autoconfig etc
org.thymeleaf.exceptions.TemplateProcessingException: Authorization-oriented expressions (such as those in 'sec:authorize') are restricted in WebFlux applications due to a lack of support in the reactive side of Spring Security (as of Spring Security 5.1). Only a minimal set of security expressions is allowed: [isAuthenticated(), isFullyAuthenticated(), isAnonymous(), isRememberMe()] (template: "layout" - line 49, col 14)
and this ticket looked the most likely candidate.
Similar exception here https://github.com/thymeleaf/thymeleaf-extras-springsecurity/issues/68 for someone else but no love from the thymeleaf guys
Comment From: rwinch
@RobMaskell No that is a separate issue. Since WebFlux uses Java Configuration it doesn't really do much with SpEL (typically a lambda can be used instead). Please create a ticket for SpEL authorization support in Reactive security and link to your comment.
Comment From: RobMaskell
@rwinch is it covered by this one https://github.com/spring-projects/spring-security/issues/5867 before I create a new ticket?
Comment From: rwinch
You are right this is the same issue. I had totally forgotten about it. Would you be interested in submitting a PR for the issue?
Comment From: sheiy
Three years, no one to support this feature?
Comment From: jnfeinstein
If there isn't planned support for this, could we perhaps get public access to the various helper classes so we can roll home-grown implementations? 🙃 It seems that some of them are public while others are not.
Comment From: PaperPlane01
I've stumbled upon this issue in my pet project, so I've created custom hacky workaround which basically just evaluates SpEL expression which is placed in my custom annotation and has Mono<Boolean> return type. I'm sharing it here, maybe this will be useful to someone https://gist.github.com/PaperPlane01/ffb93f21d81c51f92108ebb452ef9180
I really hope that there will be built-in support for it in the future.
Comment From: RobertHeim
@PaperPlane01 nice thanks!
Here is the kotlin implementation also supporting Flow:
https://gist.github.com/RobertHeim/fc2a87c453c15da8d56e2ae141c24d1d
But it does not support coroutines for now.
Do you have an idea how to get the authentication on the SpEl Context?
@ReactivePermissionCheck("@myPermissions.ownsEntity(#entityId, authentication)")
suspend fun foo(entityId: Long)
I thought of something along the lines of (blocking just for testing now)
val authentication = runBlocking {
ReactiveSecurityContextHolder.getContext().awaitFirstOrNull()
}
evaluationContext.setVariable("authentication", authentication)
But it complains that it cannot find authentication during evaluation.
Overall it seems that we will need to stop working with @PreAuthorize on the reactive stack because we cannot build production grade applications without proper security integrations and there seems to be no milestone for this to happen. :-(
Our "workaround" probably will be to just have another layer between controllers and service that implement the permissions. in plain code. Note that in this case it might be nice to implement the service interface twice to ensure that the interface is always up to date:
@Service
@Qualifier("feature") // <-- focus feature implementation and don't care for permissions
class FooServiceFeatureImpl(...) : FooService { ... }
@Service
@Primary // <-- by default use the secure version
@Qualifier("secure")
class FooServicePermissionsImpl(
@Qualifier("feature") // <-- we will delegate to the feature implementation after permission checks
private val fooService: FooService
// ... other dependencies for permission evaluation.
) : FooService // <-- implement again
{
@PreAuthorize("hasRole('USER') && #userId == authentication.getId()") // <-- can still use the supported features
override suspend fun foo(userId: Long): BarDTO {
// do advanced permission checks that are not supported in @PreAuthorize
check(someBean.myCheck(userId))
// and then delegate to the feature implementation
return fooService.foo(userId)
}
// just a helper to reduce boilerplate
protected fun check(result: Boolean) {
if (!result) {
throw AccessDeniedException("Access denied")
}
}
}
Controllers Autowire the service normally and get the primary, secured implementation. Each part is easily testable and only focuses on one layer.
Comment From: PaperPlane01
@RobertHeim
@ReactivePermissionCheck("@myPermissions.ownsEntity(#entityId, authentication)") suspend fun foo(entityId: Long)
Isn't this supposed to be #authentication? (Like with normal variables)
Comment From: RobertHeim
yes, your right, since it is a normal variable in this implementation.
Comment From: rwinch
I'm closing this in favor of gh-9401 which will address this issue as well
Comment From: MinaSalari
is there any new state for this ticket in spring boot 3.1? does it support customizing @PreAuthorize?