Make MethodSecurityEvaluationContext Delegates to MethodBasedEvaluationContext

Spring Security's MethodSecurityEvaluationContext should delegate to Spring Framework's
MethodBasedEvaluationContext

Fixes: gh-6224

Author: dbuos

core/src/main/java/org/springframework/security/access/expression/method/MethodSecurityEvaluationContext.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,105 +15,38 @@
  */
 package org.springframework.security.access.expression.method;
 
-import java.lang.reflect.Method;
-
 import org.aopalliance.intercept.MethodInvocation;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.aop.framework.AopProxyUtils;
 import org.springframework.aop.support.AopUtils;
+import org.springframework.context.expression.MethodBasedEvaluationContext;
 import org.springframework.core.ParameterNameDiscoverer;
-import org.springframework.expression.spel.support.StandardEvaluationContext;
 import org.springframework.security.core.Authentication;
-import org.springframework.security.core.parameters.DefaultSecurityParameterNameDiscoverer;
+
+import java.lang.reflect.Method;
 
 /**
  * Internal security-specific EvaluationContext implementation which lazily adds the
  * method parameter values as variables (with the corresponding parameter names) if and
  * when they are required.
  *
  * @author Luke Taylor
+ * @author Daniel Bustamante
  * @since 3.0
  */
-class MethodSecurityEvaluationContext extends StandardEvaluationContext {
+class MethodSecurityEvaluationContext extends MethodBasedEvaluationContext {
 	private static final Log logger = LogFactory
 			.getLog(MethodSecurityEvaluationContext.class);
 
-	private ParameterNameDiscoverer parameterNameDiscoverer;
-	private final MethodInvocation mi;
-	private boolean argumentsAdded;
-
-	/**
-	 * Intended for testing. Don't use in practice as it creates a new parameter resolver
-	 * for each instance. Use the constructor which takes the resolver, as an argument
-	 * thus allowing for caching.
-	 */
-	public MethodSecurityEvaluationContext(Authentication user, MethodInvocation mi) {
-		this(user, mi, new DefaultSecurityParameterNameDiscoverer());
-	}
 
-	public MethodSecurityEvaluationContext(Authentication user, MethodInvocation mi,
+	MethodSecurityEvaluationContext(Authentication user, MethodInvocation mi,
 			ParameterNameDiscoverer parameterNameDiscoverer) {
-		this.mi = mi;
-		this.parameterNameDiscoverer = parameterNameDiscoverer;
+		super(mi.getThis(), getSpecificMethod(mi), mi.getArguments(), parameterNameDiscoverer);
 	}
 
-	@Override
-	public Object lookupVariable(String name) {
-		Object variable = super.lookupVariable(name);
-
-		if (variable != null) {
-			return variable;
-		}
-
-		if (!argumentsAdded) {
-			addArgumentsAsVariables();
-			argumentsAdded = true;
-		}
-
-		variable = super.lookupVariable(name);
-
-		if (variable != null) {
-			return variable;
-		}
-
-		return null;
-	}
-
-	public void setParameterNameDiscoverer(ParameterNameDiscoverer parameterNameDiscoverer) {
-		this.parameterNameDiscoverer = parameterNameDiscoverer;
-	}
-
-	private void addArgumentsAsVariables() {
-		Object[] args = mi.getArguments();
-
-		if (args.length == 0) {
-			return;
-		}
-
-		Object targetObject = mi.getThis();
-		// SEC-1454
-		Class<?> targetClass = AopProxyUtils.ultimateTargetClass(targetObject);
-
-		if (targetClass == null) {
-			// TODO: Spring should do this, but there's a bug in ultimateTargetClass()
-			// which returns null
-			targetClass = targetObject.getClass();
-		}
-
-		Method method = AopUtils.getMostSpecificMethod(mi.getMethod(), targetClass);
-		String[] paramNames = parameterNameDiscoverer.getParameterNames(method);
-
-		if (paramNames == null) {
-			logger.warn("Unable to resolve method parameter names for method: "
-					+ method
-					+ ". Debug symbol information is required if you are using parameter names in expressions.");
-			return;
-		}
-
-		for (int i = 0; i < args.length; i++) {
-			super.setVariable(paramNames[i], args[i]);
-		}
+	private static Method getSpecificMethod(MethodInvocation mi) {
+		return AopUtils.getMostSpecificMethod(mi.getMethod(), AopProxyUtils.ultimateTargetClass(mi.getThis()));
 	}
 
 }

core/src/test/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandlerTests.java

@@ -35,9 +35,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.*;
 
 @RunWith(MockitoJUnitRunner.class)
 public class DefaultMethodSecurityExpressionHandlerTests {
@@ -53,6 +51,8 @@ public class DefaultMethodSecurityExpressionHandlerTests {
 	@Before
 	public void setup() {
 		handler = new DefaultMethodSecurityExpressionHandler();
+		when(methodInvocation.getThis()).thenReturn(new Foo());
+		when(methodInvocation.getMethod()).thenReturn(Foo.class.getMethods()[0]);
 	}
 
 	@After
@@ -82,7 +82,6 @@ public void createEvaluationContextCustomTrustResolver() {
 	@SuppressWarnings("unchecked")
 	public void filterWhenUsingStreamThenFiltersStream() {
 		final Stream<String> stream = Stream.of("1", "2", "3");
-
 		Expression expression = handler.getExpressionParser().parseExpression("filterObject ne '2'");
 
 		EvaluationContext context = handler.createEvaluationContext(authentication,
@@ -108,4 +107,9 @@ public void filterStreamWhenClosedThenUpstreamGetsClosed() {
 		((Stream) handler.filter(upstream, expression, context)).close();
 		verify(upstream).close();
 	}
+
+	private static class Foo {
+		public void bar(){
+		}
+	}
 }