Support Mono<Boolean> for Method Security #8584

sheiy · 2020-05-22T17:12:34Z

#4841

pivotal-issuemaster · 2020-05-22T17:12:36Z

@sheiy Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2020-05-22T17:12:49Z

@sheiy Thank you for signing the Contributor License Agreement!

rwinch

Thanks for the PR @sheiy I've left some comments inline. Can you also put together some tests? Unit tests are good, but we also would want some similar to those in EnableReactiveMethodSecurityTests

rwinch · 2020-05-27T13:26:02Z

core/src/main/java/org/springframework/security/access/expression/ExpressionUtils.java

@@ -30,4 +31,14 @@ public static boolean evaluateAsBoolean(Expression expr, EvaluationContext ctx)
 					+ expr.getExpressionString() + "'", e);
 		}
 	}
+
+	public static Mono<Boolean> evaluateAsMonoBoolean(Expression expr, EvaluationContext ctx) {


Adding a Reactive return type to a class used by non-reactive applications can potentially break non-reactive applications since depending on the JDK it may load all method definitions as it loads the class. In general, optional dependencies must be placed in distinct classes.

rwinch · 2020-05-27T13:27:48Z

...va/org/springframework/security/access/prepost/PreInvocationAuthorizationReactiveAdvice.java

+	 *                               annotations.
+	 * @return Mono<true> if authorised, Mono<false> otherwise
+	 */
+	Mono<Boolean> before(Authentication authentication, MethodInvocation mi,


Generally on the reactive side we use Mono<Authentication> so that if the Authentication is not needed, it is not subscribed to (i.e. if a method had permitAll we wouldn't need to load Authentication from session).

rwinch · 2020-05-27T13:28:06Z

...java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java

@@ -58,7 +58,7 @@
 	 * @param preInvocationAdvice the {@link PreInvocationAuthorizationAdvice} to use
 	 * @param postInvocationAdvice the {@link PostInvocationAuthorizationAdvice} to use
 	 */
-	public PrePostAdviceReactiveMethodInterceptor(MethodSecurityMetadataSource attributeSource, PreInvocationAuthorizationAdvice preInvocationAdvice, PostInvocationAuthorizationAdvice postInvocationAdvice) {
+	public PrePostAdviceReactiveMethodInterceptor(MethodSecurityMetadataSource attributeSource, PreInvocationAuthorizationReactiveAdvice preInvocationAdvice, PostInvocationAuthorizationAdvice postInvocationAdvice) {


This is a breaking change that we cannot make. We would need to add another constructor

rwinch · 2020-05-27T13:33:58Z

Another note is that we may want to ensure what we do for evaluating SpEL expressions works for #5867

rwinch

Thanks for the updates. I'm wondering how this fits into #8584 (comment) Is there a way to leverage this to parse SpEL expressions independent of where they come from? I cannot find out how this would be leveraged. I'm also not convinced a direct mapping of imperative to reactive makes sense here. Spring Security's ReactiveAuthorizationManager supports a richer model (Mono<AuthorizationDecision>) than the imperative model (true/false).

I'm thinking the first step is to create an ReactiveAuthorizationManager<Expression>. It should be able to evaluate the Expression and turn it into Mono<AuthorizationDecision>. It would support reactive (see ReactiveAdapterRegistry) or imperative results of Boolean and AuthorizationDecision. This could be reused for #5867 I'd create a separate pull request for this.

Then we can create a separate pull request with an implementation of ReactiveAuthorizationManager<MethodInvocation> that obtains the Expression and delegates to ReactiveAuthorizationManager<Expression>.

rwinch · 2020-06-01T19:08:42Z

...rg/springframework/security/access/expression/AbstractSecurityExpressionReactiveHandler.java

+	private ExpressionParser expressionParser = new SpelExpressionParser();
+	private BeanResolver br;
+	private RoleHierarchy roleHierarchy;
+	private PermissionEvaluator permissionEvaluator = new DenyAllPermissionEvaluator();


PermissionEvaluator is not a reactive API, so I don't think we should use it here.

rwinch · 2020-06-01T19:10:18Z

...main/java/org/springframework/security/access/expression/SecurityExpressionReactiveRoot.java

+	}
+
+	private Mono<Boolean> hasAnyAuthorityName(String prefix, String... roles) {
+		return getAuthoritySet().map(roleSet -> {


I think this could be replaced with AuthorityReactiveAuthorizationManager

rwinch · 2020-06-01T19:12:32Z

...main/java/org/springframework/security/access/expression/SecurityExpressionReactiveRoot.java

+	}
+
+	public final Mono<Boolean> isAuthenticated() {
+		return isAnonymous().map(aBoolean -> !aBoolean);


I think this could be replaced with AuthenticatedReactiveAuthorizationManager

rwinch · 2020-06-01T19:14:16Z

...main/java/org/springframework/security/access/expression/SecurityExpressionReactiveRoot.java

+ * @author Sheiy
+ * @since 5.4
+ */
+public abstract class SecurityExpressionReactiveRoot implements SecurityExpressionReactiveOperations {


I think that most of the expressions should be mapped to ReactiveAuthorizationManager implementations. I gave a few examples below. If there isn't one, then we should add it.

rwinch · 2020-06-01T19:14:55Z

...main/java/org/springframework/security/access/expression/SecurityExpressionReactiveRoot.java

+	 * Allows "denyAll" expression
+	 */
+	public final boolean denyAll = false;
+	private PermissionEvaluator permissionEvaluator;


PermissionEvaluator is not reactive, so we should probably not be using it here.

rwinch · 2020-06-01T19:16:42Z

...gframework/security/access/expression/method/ExpressionBasedPreInvocationReactiveAdvice.java

+			return Mono.just(true);
+		}
+		try {
+			return Mono.just(ExpressionUtils.evaluateAsBoolean(preAuthorize, ctx));


This should not be a try/catch. Instead I'd expect that ReactiveExpressionUtils can handle multiple return types using instanceof checks

rwinch · 2020-06-01T19:17:41Z

...java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java


 	private final MethodSecurityMetadataSource attributeSource;

-	private final PreInvocationAuthorizationAdvice preInvocationAdvice;
+	private PreInvocationAuthorizationAdvice preInvocationAdvice;


Rather than have another member and and if/else statement, if the user passes in PreInvocationAuthorizationAdvice it could be adapted to match PreInvocationAuthorizationReactiveAdvice

rwinch · 2020-06-01T19:18:11Z

core/src/main/java/org/springframework/security/access/expression/ReactiveExpressionUtils.java

+import org.springframework.expression.Expression;
+import reactor.core.publisher.Mono;
+
+public final class ReactiveExpressionUtils {


This is only used in a single location and can probably be moved to a private method.

rwinch · 2020-06-01T19:31:14Z

...ava/org/springframework/security/access/expression/SecurityExpressionReactiveOperations.java

+ * @author Sheiy
+ * @since 5.1
+ */
+public interface SecurityExpressionReactiveOperations {


I think we should use the naming ReactiveSecurityExpressionOperations

rwinch · 2020-06-01T19:31:43Z

...main/java/org/springframework/security/access/expression/SecurityExpressionReactiveRoot.java

+ * @author Sheiy
+ * @since 5.4
+ */
+public abstract class SecurityExpressionReactiveRoot implements SecurityExpressionReactiveOperations {


I think the name should be ReactiveSecurityExpressionRoot

sheiy · 2020-06-02T02:53:09Z

@rwinch Thanks for your reply. I'm sorry for that I'm not familiar with spring-security.
I will close this PR and wait for your commit of ReactiveAuthorizationManager.

fix issues:#4841

f1ec7e5

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 22, 2020

rwinch requested changes May 27, 2020

View reviewed changes

rwinch changed the title ~~fix issues #4841~~ Support Mono<Boolean> for Method Security May 27, 2020

support for issues: #4841

cefb59e

rwinch self-assigned this Jun 1, 2020

rwinch requested changes Jun 1, 2020

View reviewed changes

sheiy closed this Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Mono<Boolean> for Method Security #8584

Support Mono<Boolean> for Method Security #8584

sheiy commented May 22, 2020

pivotal-issuemaster commented May 22, 2020

pivotal-issuemaster commented May 22, 2020

rwinch left a comment

rwinch May 27, 2020

rwinch May 27, 2020

rwinch May 27, 2020

rwinch commented May 27, 2020

rwinch left a comment

rwinch Jun 1, 2020

rwinch Jun 1, 2020

rwinch Jun 1, 2020

rwinch Jun 1, 2020

rwinch Jun 1, 2020

rwinch Jun 1, 2020

rwinch Jun 1, 2020

rwinch Jun 1, 2020

rwinch Jun 1, 2020

rwinch Jun 1, 2020

sheiy commented Jun 2, 2020

Support Mono<Boolean> for Method Security #8584

Support Mono<Boolean> for Method Security #8584

Conversation

sheiy commented May 22, 2020

pivotal-issuemaster commented May 22, 2020

pivotal-issuemaster commented May 22, 2020

rwinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwinch commented May 27, 2020

rwinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheiy commented Jun 2, 2020