SEC-999: Refactoring out specific dependencies on Spring EL into SecurityExpressionHandler.

SEC:1023: Updates to expression root to allow evaluationof permissions.

core/src/main/java/org/springframework/security/expression/DefaultSecurityExpressionHandler.java

@@ -0,0 +1,95 @@
+package org.springframework.security.expression;
+
+import java.lang.reflect.Array;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Set;
+
+import org.aopalliance.intercept.MethodInvocation;
+import org.springframework.core.LocalVariableTableParameterNameDiscoverer;
+import org.springframework.core.ParameterNameDiscoverer;
+import org.springframework.expression.EvaluationContext;
+import org.springframework.expression.Expression;
+import org.springframework.security.Authentication;
+import org.springframework.security.AuthenticationTrustResolver;
+import org.springframework.security.AuthenticationTrustResolverImpl;
+
+public class DefaultSecurityExpressionHandler implements SecurityExpressionHandler {
+    private ParameterNameDiscoverer parameterNameDiscoverer = new LocalVariableTableParameterNameDiscoverer();
+    private PermissionEvaluator permissionEvaluator = new DenyAllPermissionEvaluator();
+    private AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl();
+
+    public DefaultSecurityExpressionHandler() {
+    }
+
+    public EvaluationContext createEvaluationContext(Authentication auth, MethodInvocation mi) {
+        SecurityEvaluationContext ctx = new SecurityEvaluationContext(auth, mi, parameterNameDiscoverer);
+        SecurityExpressionRoot root = new SecurityExpressionRoot(auth);
+        root.setTrustResolver(trustResolver);
+        root.setPermissionEvaluator(permissionEvaluator);
+        ctx.setRootObject(root);
+
+        return ctx;
+    }
+
+    public Object doFilter(Object filterTarget, Expression filterExpression, EvaluationContext ctx) {
+        SecurityExpressionRoot rootObject = (SecurityExpressionRoot) ctx.getRootContextObject();
+        Set removeList = new HashSet();
+
+        if (filterTarget instanceof Collection) {
+            for (Object filterObject : (Collection)filterTarget) {
+                rootObject.setFilterObject(filterObject);
+
+                if (!ExpressionUtils.evaluateAsBoolean(filterExpression, ctx)) {
+                    removeList.add(filterObject);
+                }
+            }
+
+            for(Object toRemove : removeList) {
+                ((Collection)filterTarget).remove(toRemove);
+            }
+
+            return filterTarget;
+        }
+
+        if (filterTarget.getClass().isArray()) {
+            Object[] array = (Object[])filterTarget;
+
+            for (int i = 0; i < array.length; i++) {
+                rootObject.setFilterObject(array[i]);
+
+                if (!ExpressionUtils.evaluateAsBoolean(filterExpression, ctx)) {
+                    removeList.add(array[i]);
+                }
+            }
+
+            Object[] filtered = (Object[]) Array.newInstance(filterTarget.getClass().getComponentType(),
+                    array.length - removeList.size());
+            for (int i = 0, j = 0; i < array.length; i++) {
+                if (!removeList.contains(array[i])) {
+                    filtered[j++] = array[i];
+                }
+            }
+
+            return filtered;
+        }
+
+        throw new IllegalArgumentException("Filter target must be a collection or array type, but was " + filterTarget);
+    }
+
+    public void setParameterNameDiscoverer(ParameterNameDiscoverer parameterNameDiscoverer) {
+        this.parameterNameDiscoverer = parameterNameDiscoverer;
+    }
+
+    public void setPermissionEvaluator(PermissionEvaluator permissionEvaluator) {
+        this.permissionEvaluator = permissionEvaluator;
+    }
+
+    public void setTrustResolver(AuthenticationTrustResolver trustResolver) {
+        this.trustResolver = trustResolver;
+    }
+
+    public void setReturnObject(Object returnObject, EvaluationContext ctx) {
+        ((SecurityExpressionRoot)ctx.getRootContextObject()).setReturnObject(returnObject);
+    }
+}

core/src/main/java/org/springframework/security/expression/DenyAllPermissionEvaluator.java

@@ -0,0 +1,22 @@
+package org.springframework.security.expression;
+
+import org.springframework.security.Authentication;
+
+/**
+ * A null PermissionEvaluator which denies all access. Used by default for situations when permission
+ * evaluation should not be required.
+ *
+ * @author Luke Taylor
+ * @version $Id$
+ * @since 2.5
+ */
+public class DenyAllPermissionEvaluator implements PermissionEvaluator {
+
+    /**
+     * @return false always
+     */
+    public boolean hasPermission(Authentication authentication, Object targetDomainObject, Object permission) {
+        return false;
+    }
+
+}

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

@@ -10,50 +10,6 @@ import org.springframework.expression.EvaluationException;
 import org.springframework.expression.Expression;
 
 public class ExpressionUtils {
-    public static Object doFilter(Object filterTarget, Expression filterExpression, EvaluationContext ctx) {
-        SecurityExpressionRoot rootObject = (SecurityExpressionRoot) ctx.getRootContextObject();
-        Set removeList = new HashSet();
-
-        if (filterTarget instanceof Collection) {
-            for (Object filterObject : (Collection)filterTarget) {
-                rootObject.setFilterObject(filterObject);
-
-                if (!evaluateAsBoolean(filterExpression, ctx)) {
-                    removeList.add(filterObject);
-                }
-            }
-
-            for(Object toRemove : removeList) {
-                ((Collection)filterTarget).remove(toRemove);
-            }
-
-            return filterTarget;
-        }
-
-        if (filterTarget.getClass().isArray()) {
-            Object[] array = (Object[])filterTarget;
-
-            for (int i = 0; i < array.length; i++) {
-                rootObject.setFilterObject(array[i]);
-
-                if (!evaluateAsBoolean(filterExpression, ctx)) {
-                    removeList.add(array[i]);
-                }
-            }
-
-            Object[] filtered = (Object[]) Array.newInstance(filterTarget.getClass().getComponentType(),
-                    array.length - removeList.size());
-            for (int i = 0, j = 0; i < array.length; i++) {
-                if (!removeList.contains(array[i])) {
-                    filtered[j++] = array[i];
-                }
-            }
-
-            return filtered;
-        }
-
-        throw new IllegalArgumentException("Filter target must be a collection or array type, but was " + filterTarget);
-    }
 
     public static boolean evaluateAsBoolean(Expression expr, EvaluationContext ctx) {
         try {

core/src/main/java/org/springframework/security/expression/MethodInvocationSecurityExpressionRoot.java

@@ -1,14 +1,11 @@
 package org.springframework.security.expression;
 
-import org.aopalliance.intercept.MethodInvocation;
 import org.springframework.security.Authentication;
 
 public class MethodInvocationSecurityExpressionRoot extends SecurityExpressionRoot {
 
-    MethodInvocationSecurityExpressionRoot(Authentication a, MethodInvocation mi) {
+    MethodInvocationSecurityExpressionRoot(Authentication a) {
         super(a);
-
-        mi.getArguments();
     }
 
 }

core/src/main/java/org/springframework/security/expression/SecurityEvaluationContext.java

@@ -10,23 +10,40 @@ import org.springframework.security.Authentication;
 import org.springframework.util.ClassUtils;
 
 /**
+ * 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
  * @since 2.5
  */
 public class SecurityEvaluationContext extends StandardEvaluationContext {
-
-    private ParameterNameDiscoverer parameterNameDiscoverer = new LocalVariableTableParameterNameDiscoverer();
+    private ParameterNameDiscoverer parameterNameDiscoverer;
     private boolean argumentsAdded;
     private MethodInvocation mi;
 
+    /**
+     * 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 SecurityEvaluationContext(Authentication user, MethodInvocation mi) {
-        setRootObject(new SecurityExpressionRoot(user));
+        this(user, mi, new LocalVariableTableParameterNameDiscoverer());
+    }
+
+    public SecurityEvaluationContext(Authentication user, MethodInvocation mi,
+                    ParameterNameDiscoverer parameterNameDiscoverer) {
         this.mi = mi;
+        this.parameterNameDiscoverer = parameterNameDiscoverer;
     }
 
     @Override
     public Object lookupVariable(String name) {
+        Object variable = super.lookupVariable(name);
+        if (variable != null) {
+            return variable;
+        }
+
         if (!argumentsAdded) {
             addArgumentsAsVariables();
         }
@@ -34,6 +51,10 @@ public class SecurityEvaluationContext extends StandardEvaluationContext {
         return super.lookupVariable(name);
     }
 
+    public void setParameterNameDiscoverer(ParameterNameDiscoverer parameterNameDiscoverer) {
+        this.parameterNameDiscoverer = parameterNameDiscoverer;
+    }
+
     private void addArgumentsAsVariables() {
         Object[] args = mi.getArguments();
         Object targetObject = mi.getThis();

core/src/main/java/org/springframework/security/expression/SecurityExpressionHandler.java

@@ -0,0 +1,43 @@
+package org.springframework.security.expression;
+
+import org.aopalliance.intercept.MethodInvocation;
+import org.springframework.expression.EvaluationContext;
+import org.springframework.expression.Expression;
+import org.springframework.security.Authentication;
+
+/**
+ * Facade which isolates Spring Security's requirements from the implementation of the underlying
+ * expression objects.
+ *
+ * @author Luke Taylor
+ * @version $Id$
+ * @since 2.5
+ */
+public interface SecurityExpressionHandler {
+
+    /**
+     * Provides a evaluation context in which to evaluate security expressions for a method invocation.
+     */
+    EvaluationContext createEvaluationContext(Authentication auth, MethodInvocation mi);
+
+    /**
+     * Filters a target collection or array.
+     *
+     * @param filterTarget the array or collection to be filtered.
+     * @param filterExpression the expression which should be used as the filter condition. If it returns false on
+     *          evaluation, the object will be removed from the returned collection
+     * @param ctx the current evaluation context (usualy as created through a call to
+     *          {@link #createEvaluationContext(Authentication, MethodInvocation)}
+     * @return the filtered collection or array
+     */
+    Object doFilter(Object filterTarget, Expression filterExpression, EvaluationContext ctx);
+
+    /**
+     * Used to inform the expression system of the return object
+     *
+     * @param returnObject the return object value
+     * @param ctx the context within which the object should be set
+     */
+    void setReturnObject(Object returnObject, EvaluationContext ctx);
+
+}

core/src/main/java/org/springframework/security/expression/SecurityExpressionRoot.java

@@ -4,18 +4,19 @@ import java.util.Set;
 
 import org.springframework.security.Authentication;
 import org.springframework.security.AuthenticationTrustResolver;
-import org.springframework.security.AuthenticationTrustResolverImpl;
 import org.springframework.security.util.AuthorityUtils;
 
 /**
  * Default root object for use in Spring Security expression evaluations.
  *
  * @author Luke Taylor
- *
+ * @version $Id$
+ * @since 2.5
  */
 public class SecurityExpressionRoot {
     private Authentication authentication;
-    private AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl();
+    private AuthenticationTrustResolver trustResolver;
+    private PermissionEvaluator permissionEvaluator;
     private Object filterObject;
     private Object returnObject;
 
@@ -25,8 +26,14 @@ public class SecurityExpressionRoot {
     /** Allows "denyAll" expression */
     public final boolean denyAll = false;
 
+    public final String read = "read";
+    public final String write = "write";
+    public final String create = "create";
+    public final String delete = "delete";
+    public final String admin = "admin";
+
 
-    public SecurityExpressionRoot(Authentication a) {
+    SecurityExpressionRoot(Authentication a) {
         if (a == null) {
             throw new IllegalArgumentException("Authentication object cannot be null");
         }
@@ -34,10 +41,10 @@ public class SecurityExpressionRoot {
     }
 
     public final boolean hasRole(String role) {
-        return hasAnyRole(role);
+        return AuthorityUtils.userHasAuthority(role);
     }
 
-    public boolean hasAnyRole(String... roles) {
+    public final boolean hasAnyRole(String... roles) {
         Set roleSet = AuthorityUtils.authorityArrayToSet(authentication.getAuthorities());
 
         for (String role : roles) {
@@ -69,6 +76,10 @@ public class SecurityExpressionRoot {
         return !trustResolver.isAnonymous(authentication) && !trustResolver.isRememberMe(authentication);
     }
 
+    public boolean hasPermission(Object target, Object permission) {
+        return permissionEvaluator.hasPermission(authentication, target, permission);
+    }
+
     public Authentication getAuthentication() {
         return authentication;
     }
@@ -92,4 +103,14 @@ public class SecurityExpressionRoot {
     public Object getPrincipal() {
         return authentication.getPrincipal();
     }
+
+    public void setPermissionEvaluator(PermissionEvaluator permissionEvaluator) {
+        this.permissionEvaluator = permissionEvaluator;
+    }
+
+    public void setTrustResolver(AuthenticationTrustResolver trustResolver) {
+        this.trustResolver = trustResolver;
+    }
+
+
 }

core/src/main/java/org/springframework/security/expression/support/ExpressionAnnotationMethodDefinitionSource.java

@@ -7,7 +7,10 @@ import java.util.Collection;
 import java.util.List;
 
 import org.springframework.core.annotation.AnnotationUtils;
+import org.springframework.expression.Expression;
+import org.springframework.expression.ExpressionParser;
 import org.springframework.expression.ParseException;
+import org.springframework.expression.spel.SpelExpressionParser;
 import org.springframework.security.ConfigAttribute;
 import org.springframework.security.config.SecurityConfigurationException;
 import org.springframework.security.expression.annotation.PostAuthorize;
@@ -35,6 +38,7 @@ import org.springframework.util.ClassUtils;
  * @version $Id$
  */
 public class ExpressionAnnotationMethodDefinitionSource extends AbstractMethodDefinitionSource {
+    private ExpressionParser parser = new SpelExpressionParser();
 
     public List<ConfigAttribute> getAttributes(Method method, Class targetClass) {
         if (method.getDeclaringClass() == Object.class) {
@@ -115,13 +119,13 @@ public class ExpressionAnnotationMethodDefinitionSource extends AbstractMethodDe
         ConfigAttribute post = null;
 
         // TODO: Optimization of permitAll
-        String preAuthorizeExpression = preAuthorize == null ? "permitAll" : preAuthorize.value();
-        String preFilterExpression = preFilter == null ? null : preFilter.value();
-        String filterObject = preFilter == null ? null : preFilter.filterTarget();
-        String postAuthorizeExpression = postAuthorize == null ? null : postAuthorize.value();
-        String postFilterExpression = postFilter == null ? null : postFilter.value();
-
         try {
+            Expression preAuthorizeExpression = preAuthorize == null ? parser.parseExpression("permitAll") : parser.parseExpression(preAuthorize.value());
+            Expression preFilterExpression = preFilter == null ? null : parser.parseExpression(preFilter.value());
+            String filterObject = preFilter == null ? null : preFilter.filterTarget();
+            Expression postAuthorizeExpression = postAuthorize == null ? null : parser.parseExpression(postAuthorize.value());
+            Expression postFilterExpression = postFilter == null ? null : parser.parseExpression(postFilter.value());
+
             pre = new PreInvocationExpressionAttribute(preFilterExpression, filterObject, preAuthorizeExpression);
             if (postFilterExpression != null || postAuthorizeExpression != null) {
                 post = new PostInvocationExpressionAttribute(postFilterExpression, postAuthorizeExpression);

core/src/main/java/org/springframework/security/expression/support/MethodExpressionAfterInvocationProvider.java

@@ -1,23 +1,19 @@
 package org.springframework.security.expression.support;
 
-import java.lang.reflect.Method;
 import java.util.List;
 
 import org.aopalliance.intercept.MethodInvocation;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-import org.springframework.core.LocalVariableTableParameterNameDiscoverer;
-import org.springframework.core.ParameterNameDiscoverer;
 import org.springframework.expression.EvaluationContext;
 import org.springframework.expression.Expression;
-import org.springframework.expression.spel.standard.StandardEvaluationContext;
 import org.springframework.security.AccessDeniedException;
 import org.springframework.security.Authentication;
 import org.springframework.security.ConfigAttribute;
 import org.springframework.security.afterinvocation.AfterInvocationProvider;
+import org.springframework.security.expression.DefaultSecurityExpressionHandler;
 import org.springframework.security.expression.ExpressionUtils;
-import org.springframework.security.expression.SecurityExpressionRoot;
-import org.springframework.util.ClassUtils;
+import org.springframework.security.expression.SecurityExpressionHandler;
 
 /**
  * AfterInvocationProvider which handles the @PostAuthorize and @PostFilter annotation expressions.
@@ -30,7 +26,7 @@ public class MethodExpressionAfterInvocationProvider implements AfterInvocationP
 
     protected final Log logger = LogFactory.getLog(getClass());
 
-    private ParameterNameDiscoverer parameterNameDiscoverer = new LocalVariableTableParameterNameDiscoverer();
+    private SecurityExpressionHandler expressionHandler = new DefaultSecurityExpressionHandler();
 
     public Object decide(Authentication authentication, Object object, List<ConfigAttribute> config, Object returnedObject)
             throws AccessDeniedException {
@@ -41,10 +37,10 @@ public class MethodExpressionAfterInvocationProvider implements AfterInvocationP
             return returnedObject;
         }
 
-        StandardEvaluationContext ctx = new StandardEvaluationContext();
-        populateContextVariables(ctx, (MethodInvocation) object);
-        SecurityExpressionRoot expressionRoot = new SecurityExpressionRoot(authentication);
-        ctx.setRootObject(expressionRoot);
+        EvaluationContext ctx =
+            expressionHandler.createEvaluationContext(authentication, (MethodInvocation)object);
+        //SecurityExpressionRoot expressionRoot = new SecurityExpressionRoot(authentication);
+        //ctx.setRootObject(expressionRoot);
 
         Expression postFilter = mca.getFilterExpression();
         Expression postAuthorize = mca.getAuthorizeExpression();
@@ -55,7 +51,7 @@ public class MethodExpressionAfterInvocationProvider implements AfterInvocationP
             }
 
             if (returnedObject != null) {
-                returnedObject = ExpressionUtils.doFilter(returnedObject, postFilter, ctx);
+                returnedObject = expressionHandler.doFilter(returnedObject, postFilter, ctx);
             } else {
                 if (logger.isDebugEnabled()) {
                     logger.debug("Return object is null, filtering will be skipped");
@@ -63,7 +59,7 @@ public class MethodExpressionAfterInvocationProvider implements AfterInvocationP
             }
         }
 
-        expressionRoot.setReturnObject(returnedObject);
+        expressionHandler.setReturnObject(returnedObject, ctx);
 
         if (postAuthorize != null && !ExpressionUtils.evaluateAsBoolean(postAuthorize, ctx)) {
             if (logger.isDebugEnabled()) {
@@ -75,17 +71,6 @@ public class MethodExpressionAfterInvocationProvider implements AfterInvocationP
         return returnedObject;
     }
 
-    private void populateContextVariables(EvaluationContext ctx, MethodInvocation mi) {
-        Object[] args = mi.getArguments();
-        Object targetObject = mi.getThis();
-        Method method = ClassUtils.getMostSpecificMethod(mi.getMethod(), targetObject.getClass());
-        String[] paramNames = parameterNameDiscoverer.getParameterNames(method);
-
-        for(int i=0; i < args.length; i++) {
-            ctx.setVariable(paramNames[i], args[i]);
-        }
-    }
-
     private PostInvocationExpressionAttribute findMethodAccessControlExpression(List<ConfigAttribute> config) {
         // Find the MethodAccessControlExpression attribute
         for (ConfigAttribute attribute : config) {

core/src/main/java/org/springframework/security/expression/support/MethodExpressionVoter.java

@@ -10,8 +10,9 @@ import org.springframework.expression.EvaluationContext;
 import org.springframework.expression.Expression;
 import org.springframework.security.Authentication;
 import org.springframework.security.ConfigAttribute;
+import org.springframework.security.expression.DefaultSecurityExpressionHandler;
 import org.springframework.security.expression.ExpressionUtils;
-import org.springframework.security.expression.SecurityEvaluationContext;
+import org.springframework.security.expression.SecurityExpressionHandler;
 import org.springframework.security.vote.AccessDecisionVoter;
 
 /**
@@ -28,6 +29,8 @@ import org.springframework.security.vote.AccessDecisionVoter;
 public class MethodExpressionVoter implements AccessDecisionVoter {
     protected final Log logger = LogFactory.getLog(getClass());
 
+    private SecurityExpressionHandler expressionHandler = new DefaultSecurityExpressionHandler();
+
     public boolean supports(ConfigAttribute attribute) {
         return attribute instanceof AbstractExpressionBasedMethodConfigAttribute;
     }
@@ -45,14 +48,14 @@ public class MethodExpressionVoter implements AccessDecisionVoter {
         }
 
         MethodInvocation mi = (MethodInvocation)object;
-        EvaluationContext ctx = new SecurityEvaluationContext(authentication, mi);
+        EvaluationContext ctx = expressionHandler.createEvaluationContext(authentication, mi);
         Expression preFilter = mace.getFilterExpression();
         Expression preAuthorize = mace.getAuthorizeExpression();
 
         if (preFilter != null) {
             Object filterTarget = findFilterTarget(mace.getFilterTarget(), ctx, mi);
 
-            ExpressionUtils.doFilter(filterTarget, preFilter, ctx);
+            expressionHandler.doFilter(filterTarget, preFilter, ctx);
         }
 
         if (preAuthorize == null) {

core/src/main/java/org/springframework/security/expression/support/PostInvocationExpressionAttribute.java

@@ -1,5 +1,6 @@
 package org.springframework.security.expression.support;
 
+import org.springframework.expression.Expression;
 import org.springframework.expression.ParseException;
 
 class PostInvocationExpressionAttribute extends AbstractExpressionBasedMethodConfigAttribute {
@@ -9,4 +10,8 @@ class PostInvocationExpressionAttribute extends AbstractExpressionBasedMethodCon
         super(filterExpression, authorizeExpression);
     }
 
+    PostInvocationExpressionAttribute(Expression filterExpression, Expression authorizeExpression)
+                    throws ParseException {
+        super(filterExpression, authorizeExpression);
+    }
 }

core/src/main/java/org/springframework/security/expression/support/PreInvocationExpressionAttribute.java

@@ -1,5 +1,6 @@
 package org.springframework.security.expression.support;
 
+import org.springframework.expression.Expression;
 import org.springframework.expression.ParseException;
 
 class PreInvocationExpressionAttribute extends AbstractExpressionBasedMethodConfigAttribute {
@@ -12,6 +13,13 @@ class PreInvocationExpressionAttribute extends AbstractExpressionBasedMethodConf
         this.filterTarget = filterTarget;
     }
 
+    PreInvocationExpressionAttribute(Expression filterExpression, String filterTarget, Expression authorizeExpression)
+            throws ParseException {
+        super(filterExpression, authorizeExpression);
+
+        this.filterTarget = filterTarget;
+    }
+
     /**
      * The parameter name of the target argument (must be a Collection) to which filtering will be applied.
      *