Spring-security: Support Mono<Boolean> for Method Security

[rwinch]

Summary

Reactive method security requires a method return Boolean which does not work of the logic to determine access is blocking. We should allow the result to be Mono<Boolean>

[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();
  }
}

[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?

[rwinch]

That and we need a reactive equivalent of PrePostAdviceReactiveMethodInterceptor

[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?

[edeandrea]

Would you be open to a PR for this or is it something you are already actively working on?

[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.

[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.

[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.

[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.

[rwinch]

Can you point me to your work in progress with a test that I can run that illustrates the problem you are having?

[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.

[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.

[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.

[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:

[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.

[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.

[edeandrea]

I just have not had a chance to get back to the PR unfortunately.

[eusokolov]

Also waiting for this PR

[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.

[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?

[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.

[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.

[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.

[masoudparvari]

Hi @rwinch , is there any estimation on when this feature will be in ?

[bollywood-coder]

Rob Winch, any idea of when we can see this feature in Spring Security?

[bollywood-coder]

Or is anybody working on this (community or from the spring team) ?

[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

[bollywood-coder]

Hi Rob, I would be interested in. However, I could not start before December.

[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?

[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).

[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

[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.

[RobMaskell]

@rwinch is it covered by this one https://github.com/spring-projects/spring-security/issues/5867 before I create a new ticket?

[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?

[sheiy]

Three years, no one to support this feature?

[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.