SEC-999: Moved caching from AbstractFallbackMethodDefinitionSource to DelegatingMethodDefinitionSource, to allow ExpressionBasedMethodDefinitionSource to take advantage of it. The latter no-longer uses the fallback approach as it requires its own strategy to combine annotations which may be defined at method-on-class, class, method-on-interface or interface level.

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

@@ -13,6 +13,13 @@ public class SecurityExpressionRoot {
     private Object filterObject;
     private Object returnObject;
 
+    /** Allows "permitAll" expression */
+    public final boolean permitAll = true;
+
+    /** Allows "denyAll" expression */
+    public final boolean denyAll = false;
+
+
     public SecurityExpressionRoot(Authentication a) {
         this.authentication = a;
     }

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

@@ -1,5 +1,6 @@
 package org.springframework.security.expression.support;
 
+import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -13,7 +14,8 @@ import org.springframework.security.expression.annotation.PostAuthorize;
 import org.springframework.security.expression.annotation.PostFilter;
 import org.springframework.security.expression.annotation.PreAuthorize;
 import org.springframework.security.expression.annotation.PreFilter;
-import org.springframework.security.intercept.method.AbstractFallbackMethodDefinitionSource;
+import org.springframework.security.intercept.method.AbstractMethodDefinitionSource;
+import org.springframework.util.ClassUtils;
 
 /**
  * MethodDefinitionSource which extracts metadata from the @PreFilter and @PreAuthorize annotations
@@ -22,6 +24,9 @@ import org.springframework.security.intercept.method.AbstractFallbackMethodDefin
  * Annotations may be specified on classes or methods, and method-specific annotations will take precedence.
  * If you use any annotation and do not specify a pre-authorization condition, then the method will be
  * allowed as if a @PreAuthorize("permitAll") were present.
+ * <p>
+ * Since we are handling multiple annotations here, it's possible that we may have to combine annotations defined in
+ * multiple locations for a single method - they may be defined on the method itself, or at interface or class level.
  *
  * @see MethodExpressionVoter
  *
@@ -29,56 +34,71 @@ import org.springframework.security.intercept.method.AbstractFallbackMethodDefin
  * @since 2.5
  * @version $Id$
  */
-public class ExpressionAnnotationMethodDefinitionSource extends AbstractFallbackMethodDefinitionSource {
+public class ExpressionAnnotationMethodDefinitionSource extends AbstractMethodDefinitionSource {
 
-    @Override
-    protected List<ConfigAttribute> findAttributes(Method method, Class targetClass) {
-        PreFilter preFilter = AnnotationUtils.findAnnotation(method, PreFilter.class);
-        PreAuthorize preAuthorize = AnnotationUtils.findAnnotation(method, PreAuthorize.class);
-        PostFilter postFilter = AnnotationUtils.findAnnotation(method, PostFilter.class);
-        PostAuthorize postAuthorize = AnnotationUtils.findAnnotation(method, PostAuthorize.class);
+    public List<ConfigAttribute> getAttributes(Method method, Class targetClass) {
+        logger.debug("Looking for expression annotations for method '" +
+                method.getName() + "' on target class '" + targetClass + "'");
+        PreFilter preFilter = findAnnotation(method, targetClass, PreFilter.class);
+        PreAuthorize preAuthorize = findAnnotation(method, targetClass, PreAuthorize.class);
+        PostFilter postFilter = findAnnotation(method, targetClass, PostFilter.class);
+     // TODO: Can we check for void methods and throw an exception here?
+        PostAuthorize postAuthorize = findAnnotation(method, targetClass, PostAuthorize.class);
 
         if (preFilter == null && preAuthorize == null && postFilter == null && postAuthorize == null ) {
-            // There is no method level meta-data so return and allow parent to query at class-level
+            // There is no meta-data so return
+            logger.debug("No expression annotations found");
             return null;
         }
 
-        // There is at least one non-null value, so the parent class will not query for class-specific annotations
-        // and we have to locate them here as appropriate.
+        return createAttributeList(preFilter, preAuthorize, postFilter, postAuthorize);
+    }
 
-        if (preAuthorize == null) {
-            preAuthorize = (PreAuthorize)targetClass.getAnnotation(PreAuthorize.class);
+    /**
+     * See {@link org.springframework.security.intercept.method.AbstractFallbackMethodDefinitionSource#getAttributes(Method, Class)}
+     * for the logic of this method. The ordering here is slightly different in that we consider method-specific
+     * annotations on an interface before class-level ones.
+     */
+    private <A  extends Annotation> A findAnnotation(Method method, Class targetClass, Class<A> annotationClass) {
+        // The method may be on an interface, but we need attributes from the target class.
+        // If the target class is null, the method will be unchanged.
+        Method specificMethod = ClassUtils.getMostSpecificMethod(method, targetClass);
+        A annotation = AnnotationUtils.findAnnotation(specificMethod, annotationClass);
+
+        if (annotation != null) {
+            logger.debug(annotation + " found on specific method: " + specificMethod);
+            return annotation;
         }
 
-        if (preFilter == null) {
-            preFilter = (PreFilter)targetClass.getAnnotation(PreFilter.class);
-        }
+        // Check the original (e.g. interface) method
+        if (specificMethod != method) {
+            annotation = AnnotationUtils.findAnnotation(method, annotationClass);
 
-        if (postFilter == null) {
-            // TODO: Can we check for void methods and throw an exception here?
-            postFilter = (PostFilter)targetClass.getAnnotation(PostFilter.class);
+            if (annotation != null) {
+                logger.debug(annotation + " found on: " + method);
+                return annotation;
+            }
         }
 
-        if (postAuthorize == null) {
-            postAuthorize = (PostAuthorize)targetClass.getAnnotation(PostAuthorize.class);
-        }
+        // Check the class-level (note declaringClass, not targetClass, which may not actually implement the method)
+        annotation = specificMethod.getDeclaringClass().getAnnotation(annotationClass);
 
-        return createAttributeList(preFilter, preAuthorize, postFilter, postAuthorize);
-    }
+        if (annotation != null) {
+            logger.debug(annotation + " found on: " + specificMethod.getDeclaringClass().getName());
+            return annotation;
+        }
 
-    @Override
-    protected List<ConfigAttribute> findAttributes(Class targetClass) {
-        PreFilter preFilter = (PreFilter)targetClass.getAnnotation(PreFilter.class);
-        PreAuthorize preAuthorize = (PreAuthorize)targetClass.getAnnotation(PreAuthorize.class);
-        PostFilter postFilter = (PostFilter)targetClass.getAnnotation(PostFilter.class);
-        PostAuthorize postAuthorize = (PostAuthorize)targetClass.getAnnotation(PostAuthorize.class);
+        // Check for a possible interface annotation which would not be inherited by the declaring class
+        if (specificMethod != method) {
+            annotation = method.getDeclaringClass().getAnnotation(annotationClass);
 
-        if (preFilter == null && preAuthorize == null && postFilter == null && postAuthorize == null ) {
-            // There is no class level meta-data (and by implication no meta-data at all)
-            return null;
+            if (annotation != null) {
+                logger.debug(annotation + " found on: " + method.getDeclaringClass().getName());
+                return annotation;
+            }
         }
 
-        return createAttributeList(preFilter, preAuthorize, postFilter, postAuthorize);
+        return null;
     }
 
     public Collection<List<? extends ConfigAttribute>> getConfigAttributeDefinitions() {
@@ -91,7 +111,7 @@ public class ExpressionAnnotationMethodDefinitionSource extends AbstractFallback
         ConfigAttribute post = null;
 
         // TODO: Optimization of permitAll
-        String preAuthorizeExpression = preAuthorize == null ? "permitAll()" : preAuthorize.value();
+        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();
@@ -106,7 +126,6 @@ public class ExpressionAnnotationMethodDefinitionSource extends AbstractFallback
             throw new SecurityConfigurationException("Failed to parse expression '" + e.getExpressionString() + "'", e);
         }
 
-
         List<ConfigAttribute> attrs = new ArrayList<ConfigAttribute>(2);
         if (pre != null) {
             attrs.add(pre);

core/src/main/java/org/springframework/security/intercept/method/AbstractFallbackMethodDefinitionSource.java

@@ -1,117 +1,33 @@
 package org.springframework.security.intercept.method;
 
 import java.lang.reflect.Method;
-import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 
-import org.aopalliance.intercept.MethodInvocation;
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-import org.aspectj.lang.JoinPoint;
-import org.aspectj.lang.reflect.CodeSignature;
 import org.springframework.security.ConfigAttribute;
-import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
-import org.springframework.util.ObjectUtils;
 
 /**
  * Abstract implementation of {@link MethodDefinitionSource} that supports both Spring AOP and AspectJ and
- * caches configuration attribute resolution from: 1. specific target method; 2. target class;  3. declaring method;
- * 4. declaring class/interface.
- *
+ * performs attribute resolution from: 1. specific target method; 2. target class;  3. declaring method;
+ * 4. declaring class/interface. Use with {@link DelegatingMethodDefinitionSource} for caching support.
  * <p>
  * This class mimics the behaviour of Spring's AbstractFallbackTransactionAttributeSource class.
- * </p>
- *
  * <p>
  * Note that this class cannot extract security metadata where that metadata is expressed by way of
- * a target method/class (ie #1 and #2 above) AND the target method/class is encapsulated in another
+ * a target method/class (i.e. #1 and #2 above) AND the target method/class is encapsulated in another
  * proxy object. Spring Security does not walk a proxy chain to locate the concrete/final target object.
  * Consider making Spring Security your final advisor (so it advises the final target, as opposed to
  * another proxy), move the metadata to declared methods or interfaces the proxy implements, or provide
  * your own replacement <tt>MethodDefinitionSource</tt>.
- * </p>
  *
  * @author Ben Alex
+ * @author Luke taylor
  * @version $Id$
  * @since 2.0
  */
-public abstract class AbstractFallbackMethodDefinitionSource implements MethodDefinitionSource {
-    protected final Log logger = LogFactory.getLog(getClass());
-
-    private final static List<ConfigAttribute> NULL_CONFIG_ATTRIBUTE = new ArrayList<ConfigAttribute>(0);
-    private final Map<DefaultCacheKey, List<ConfigAttribute>> attributeCache = new HashMap();
-
-    public List<ConfigAttribute> getAttributes(Object object) throws IllegalArgumentException {
-        Assert.notNull(object, "Object cannot be null");
-
-        if (object instanceof MethodInvocation) {
-            MethodInvocation mi = (MethodInvocation) object;
-            Object target = mi.getThis();
-            return getAttributes(mi.getMethod(), target == null ? null : target.getClass());
-        }
-
-        if (object instanceof JoinPoint) {
-            JoinPoint jp = (JoinPoint) object;
-            Class targetClass = jp.getTarget().getClass();
-            String targetMethodName = jp.getStaticPart().getSignature().getName();
-            Class[] types = ((CodeSignature) jp.getStaticPart().getSignature()).getParameterTypes();
-            Class declaringType = ((CodeSignature) jp.getStaticPart().getSignature()).getDeclaringType();
-
-            Method method = ClassUtils.getMethodIfAvailable(declaringType, targetMethodName, types);
-            Assert.notNull(method, "Could not obtain target method from JoinPoint: '"+ jp + "'");
-
-            return getAttributes(method, targetClass);
-        }
-
-        throw new IllegalArgumentException("Object must be a MethodInvocation or JoinPoint");
-    }
-
-    public final boolean supports(Class clazz) {
-        return (MethodInvocation.class.isAssignableFrom(clazz) || JoinPoint.class.isAssignableFrom(clazz));
-    }
+public abstract class AbstractFallbackMethodDefinitionSource extends AbstractMethodDefinitionSource {
 
     public List<ConfigAttribute> getAttributes(Method method, Class targetClass) {
-        // First, see if we have a cached value.
-        DefaultCacheKey cacheKey = new DefaultCacheKey(method, targetClass);
-        synchronized (attributeCache) {
-            List<ConfigAttribute> cached = attributeCache.get(cacheKey);
-            if (cached != null) {
-                // Value will either be canonical value indicating there is no config attribute,
-                // or an actual config attribute.
-                if (cached == NULL_CONFIG_ATTRIBUTE) {
-                    return null;
-                }
-                else {
-                    return cached;
-                }
-            }
-            else {
-                // We need to work it out.
-                List<ConfigAttribute> attributes = computeAttributes(method, targetClass);
-                // Put it in the cache.
-                if (attributes == null) {
-                    this.attributeCache.put(cacheKey, NULL_CONFIG_ATTRIBUTE);
-                } else {
-                    if (logger.isDebugEnabled()) {
-                        logger.debug("Adding security method [" + cacheKey + "] with attributes " + attributes);
-                    }
-                    this.attributeCache.put(cacheKey, attributes);
-                }
-                return attributes;
-            }
-        }
-    }
-
-    /**
-     *
-     * @param method the method for the current invocation (never <code>null</code>)
-     * @param targetClass the target class for this invocation (may be <code>null</code>)
-     * @return
-     */
-    private List<ConfigAttribute> computeAttributes(Method method, Class targetClass) {
         // The method may be on an interface, but we need attributes from the target class.
         // If the target class is null, the method will be unchanged.
         Method specificMethod = ClassUtils.getMostSpecificMethod(method, targetClass);
@@ -137,7 +53,6 @@ public abstract class AbstractFallbackMethodDefinitionSource implements MethodDe
             return findAttributes(method.getDeclaringClass());
         }
         return null;
-
     }
 
     /**
@@ -169,35 +84,5 @@ public abstract class AbstractFallbackMethodDefinitionSource implements MethodDe
      */
     protected abstract List<ConfigAttribute> findAttributes(Class clazz);
 
-    private static class DefaultCacheKey {
-
-        private final Method method;
-        private final Class targetClass;
-
-        public DefaultCacheKey(Method method, Class targetClass) {
-            this.method = method;
-            this.targetClass = targetClass;
-        }
-
-        public boolean equals(Object other) {
-            if (this == other) {
-                return true;
-            }
-            if (!(other instanceof DefaultCacheKey)) {
-                return false;
-            }
-            DefaultCacheKey otherKey = (DefaultCacheKey) other;
-            return (this.method.equals(otherKey.method) &&
-                    ObjectUtils.nullSafeEquals(this.targetClass, otherKey.targetClass));
-        }
-
-        public int hashCode() {
-            return this.method.hashCode() * 21 + (this.targetClass != null ? this.targetClass.hashCode() : 0);
-        }
-
-        public String toString() {
-            return "CacheKey[" + (targetClass == null ? "-" : targetClass.getName()) + "; " + method + "]";
-        }
-    }
 
 }

core/src/main/java/org/springframework/security/intercept/method/AbstractMethodDefinitionSource.java

@@ -26,73 +26,50 @@ import org.aspectj.lang.JoinPoint;
 import org.aspectj.lang.reflect.CodeSignature;
 
 import org.springframework.util.Assert;
+import org.springframework.util.ClassUtils;
 
 import java.lang.reflect.Method;
 import java.util.List;
 
 
 /**
- * Abstract implementation of <code>MethodDefinitionSource</code>.
+ * Abstract implementation of <tt>MethodDefinitionSource</tt> which resolves the secured object type to
+ * either a MethodInvocation or a JoinPoint.
  *
  * @author Ben Alex
- * @deprecated No longer used within the framework and will be removed
+ * @author Luke Taylor
  * @version $Id$
  */
 public abstract class AbstractMethodDefinitionSource implements MethodDefinitionSource {
-    //~ Static fields/initializers =====================================================================================
 
-    private static final Log logger = LogFactory.getLog(AbstractMethodDefinitionSource.class);
+    protected final Log logger = LogFactory.getLog(getClass());
 
     //~ Methods ========================================================================================================
 
-    public List<ConfigAttribute> getAttributes(Object object)
-        throws IllegalArgumentException {
-        Assert.notNull(object, "Object cannot be null");
-
+    public final List<ConfigAttribute> getAttributes(Object object) {
         if (object instanceof MethodInvocation) {
-            return this.lookupAttributes(((MethodInvocation) object).getMethod());
+            MethodInvocation mi = (MethodInvocation) object;
+            Object target = mi.getThis();
+            return getAttributes(mi.getMethod(), target == null ? null : target.getClass());
         }
 
         if (object instanceof JoinPoint) {
             JoinPoint jp = (JoinPoint) object;
-            Class targetClazz = jp.getTarget().getClass();
+            Class targetClass = jp.getTarget().getClass();
             String targetMethodName = jp.getStaticPart().getSignature().getName();
             Class[] types = ((CodeSignature) jp.getStaticPart().getSignature()).getParameterTypes();
+            Class declaringType = ((CodeSignature) jp.getStaticPart().getSignature()).getDeclaringType();
+
+            Method method = ClassUtils.getMethodIfAvailable(declaringType, targetMethodName, types);
+            Assert.notNull(method, "Could not obtain target method from JoinPoint: '"+ jp + "'");
 
-            if (logger.isDebugEnabled()) {
-                logger.debug("Target Class: " + targetClazz);
-                logger.debug("Target Method Name: " + targetMethodName);
-
-                for (int i = 0; i < types.length; i++) {
-                    if (logger.isDebugEnabled()) {
-                        logger.debug("Target Method Arg #" + i + ": " + types[i]);
-                    }
-                }
-            }
-
-            try {
-                return this.lookupAttributes(targetClazz.getMethod(targetMethodName, types));
-            } catch (NoSuchMethodException nsme) {
-                throw new IllegalArgumentException("Could not obtain target method from JoinPoint: " + jp);
-            }
+            return getAttributes(method, targetClass);
         }
 
-        throw new IllegalArgumentException("Object must be a MethodInvocation or JoinPoint");
+        throw new IllegalArgumentException("Object must be a non-null MethodInvocation or JoinPoint");
     }
 
-    /**
-     * Performs the actual lookup of the relevant <code>ConfigAttributeDefinition</code> for the specified
-     * <code>Method</code> which is subject of the method invocation.<P>Provided so subclasses need only to
-     * provide one basic method to properly interface with the <code>MethodDefinitionSource</code>.</p>
-     *  <p>Returns <code>null</code> if there are no matching attributes for the method.</p>
-     *
-     * @param method the method being invoked for which configuration attributes should be looked up
-     *
-     * @return the <code>ConfigAttributeDefinition</code> that applies to the specified <code>Method</code>
-     */
-    protected abstract List<ConfigAttribute> lookupAttributes(Method method);
-
-    public boolean supports(Class clazz) {
+    public final boolean supports(Class clazz) {
         return (MethodInvocation.class.isAssignableFrom(clazz) || JoinPoint.class.isAssignableFrom(clazz));
     }
 }

core/src/main/java/org/springframework/security/intercept/method/DelegatingMethodDefinitionSource.java

@@ -2,52 +2,75 @@ package org.springframework.security.intercept.method;
 
 import java.lang.reflect.Method;
 import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.security.ConfigAttribute;
-import org.springframework.security.ConfigAttributeDefinition;
 import org.springframework.util.Assert;
+import org.springframework.util.ObjectUtils;
 
 /**
  * Automatically tries a series of method definition sources, relying on the first source of metadata
- * that provides a non-null response.
+ * that provides a non-null response. Provides automatic caching of the retrieved metadata.
  *
  * @author Ben Alex
+ * @author Luke Taylor
  * @version $Id$
  */
-public final class DelegatingMethodDefinitionSource implements MethodDefinitionSource, InitializingBean {
-    private List methodDefinitionSources;
+public final class DelegatingMethodDefinitionSource extends AbstractMethodDefinitionSource implements InitializingBean {
+    private final static List<ConfigAttribute> NULL_CONFIG_ATTRIBUTE = Collections.emptyList();
+
+    private List<MethodDefinitionSource> methodDefinitionSources;
+    private final Map<DefaultCacheKey, List<ConfigAttribute>> attributeCache = new HashMap();
+
+    //~ Methods ========================================================================================================
 
     public void afterPropertiesSet() throws Exception {
         Assert.notEmpty(methodDefinitionSources, "A list of MethodDefinitionSources is required");
     }
 
     public List<ConfigAttribute> getAttributes(Method method, Class targetClass) {
-        Iterator i = methodDefinitionSources.iterator();
-        while (i.hasNext()) {
-            MethodDefinitionSource s = (MethodDefinitionSource) i.next();
-            List<ConfigAttribute> result = s.getAttributes(method, targetClass);
-            if (result != null) {
-                return result;
+        DefaultCacheKey cacheKey = new DefaultCacheKey(method, targetClass);
+        synchronized (attributeCache) {
+            List<ConfigAttribute> cached = attributeCache.get(cacheKey);
+            // Check for canonical value indicating there is no config attribute,
+            if (cached == NULL_CONFIG_ATTRIBUTE) {
+                return null;
             }
-        }
-        return null;
-    }
 
-    public List<ConfigAttribute> getAttributes(Object object) throws IllegalArgumentException {
-        Iterator i = methodDefinitionSources.iterator();
-        while (i.hasNext()) {
-            MethodDefinitionSource s = (MethodDefinitionSource) i.next();
-            List<ConfigAttribute> result = s.getAttributes(object);
-            if (result != null) {
-                return result;
+            if (cached != null) {
+                return cached;
             }
+
+            // No cached value, so query the sources to find a result
+            List<ConfigAttribute> attributes = null;
+            for (MethodDefinitionSource s : methodDefinitionSources) {
+                attributes = s.getAttributes(method, targetClass);
+                if (attributes != null) {
+                    break;
+                }
+            }
+
+            // Put it in the cache.
+            if (attributes == null) {
+                this.attributeCache.put(cacheKey, NULL_CONFIG_ATTRIBUTE);
+                return null;
+            }
+
+            if (logger.isDebugEnabled()) {
+                logger.debug("Adding security method [" + cacheKey + "] with attributes " + attributes);
+            }
+
+            this.attributeCache.put(cacheKey, attributes);
+
+            return attributes;
         }
-        return null;
     }
 
     public Collection<List<? extends ConfigAttribute>> getConfigAttributeDefinitions() {
@@ -63,19 +86,40 @@ public final class DelegatingMethodDefinitionSource implements MethodDefinitionS
         return set;
     }
 
-    public boolean supports(Class clazz) {
-        Iterator i = methodDefinitionSources.iterator();
-        while (i.hasNext()) {
-            MethodDefinitionSource s = (MethodDefinitionSource) i.next();
-            if (s.supports(clazz)) {
+    public void setMethodDefinitionSources(List methodDefinitionSources) {
+        Assert.notEmpty(methodDefinitionSources, "A list of MethodDefinitionSources is required");
+        this.methodDefinitionSources = methodDefinitionSources;
+    }
+
+    //~ Inner Classes ==================================================================================================
+
+    private static class DefaultCacheKey {
+        private final Method method;
+        private final Class targetClass;
+
+        public DefaultCacheKey(Method method, Class targetClass) {
+            this.method = method;
+            this.targetClass = targetClass;
+        }
+
+        public boolean equals(Object other) {
+            if (this == other) {
                 return true;
             }
+            if (!(other instanceof DefaultCacheKey)) {
+                return false;
+            }
+            DefaultCacheKey otherKey = (DefaultCacheKey) other;
+            return (this.method.equals(otherKey.method) &&
+                    ObjectUtils.nullSafeEquals(this.targetClass, otherKey.targetClass));
         }
-        return false;
-    }
 
-    public void setMethodDefinitionSources(List methodDefinitionSources) {
-        Assert.notEmpty(methodDefinitionSources, "A list of MethodDefinitionSources is required");
-        this.methodDefinitionSources = methodDefinitionSources;
+        public int hashCode() {
+            return this.method.hashCode() * 21 + (this.targetClass != null ? this.targetClass.hashCode() : 0);
+        }
+
+        public String toString() {
+            return "CacheKey[" + (targetClass == null ? "-" : targetClass.getName()) + "; " + method + "]";
+        }
     }
 }

core/src/main/java/org/springframework/security/intercept/method/MapBasedMethodDefinitionSource.java

@@ -18,14 +18,11 @@ package org.springframework.security.intercept.method;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
 import org.springframework.beans.factory.BeanClassLoaderAware;
 import org.springframework.security.ConfigAttribute;
 import org.springframework.security.ConfigAttributeDefinition;
@@ -47,9 +44,6 @@ import org.springframework.util.ClassUtils;
  * @since 2.0
  */
 public class MapBasedMethodDefinitionSource extends AbstractFallbackMethodDefinitionSource implements BeanClassLoaderAware {
-    //~ Static fields/initializers =====================================================================================
-
-    private static final Log logger = LogFactory.getLog(MapBasedMethodDefinitionSource.class);
 
     //~ Instance fields ================================================================================================
     private ClassLoader beanClassLoader = ClassUtils.getDefaultClassLoader();

core/src/test/java/org/springframework/security/expression/support/ExpressionAnnotationMethodDefinitionSourceTests.java

@@ -0,0 +1,169 @@
+package org.springframework.security.expression.support;
+
+import static org.junit.Assert.*;
+
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.springframework.security.ConfigAttribute;
+import org.springframework.security.expression.annotation.PostAuthorize;
+import org.springframework.security.expression.annotation.PostFilter;
+import org.springframework.security.expression.annotation.PreAuthorize;
+import org.springframework.security.expression.annotation.PreFilter;
+import org.springframework.security.intercept.method.MockMethodInvocation;
+
+
+public class ExpressionAnnotationMethodDefinitionSourceTests {
+    private ExpressionAnnotationMethodDefinitionSource mds = new ExpressionAnnotationMethodDefinitionSource();
+
+    private MockMethodInvocation voidImpl1;
+    private MockMethodInvocation voidImpl2;
+    private MockMethodInvocation voidImpl3;
+    private MockMethodInvocation listImpl1;
+    private MockMethodInvocation notherListImpl1;
+    private MockMethodInvocation notherListImpl2;
+
+    @Before
+    public void setUpData() throws Exception {
+        voidImpl1 = new MockMethodInvocation(new ReturnVoidImpl1(), ReturnVoid.class, "doSomething", List.class);
+        voidImpl2 = new MockMethodInvocation(new ReturnVoidImpl2(), ReturnVoid.class, "doSomething", List.class);
+        voidImpl3 = new MockMethodInvocation(new ReturnVoidImpl3(), ReturnVoid.class, "doSomething", List.class);
+        listImpl1 = new MockMethodInvocation(new ReturnAListImpl1(), ReturnAList.class, "doSomething", List.class);
+        notherListImpl1 = new MockMethodInvocation(new ReturnAnotherListImpl1(), ReturnAnotherList.class, "doSomething", List.class);
+        notherListImpl2 = new MockMethodInvocation(new ReturnAnotherListImpl2(), ReturnAnotherList.class, "doSomething", List.class);
+    }
+
+    @Test
+    public void classLevelPreAnnotationIsPickedUpWhenNoMethodLevelExists() throws Exception {
+        List<ConfigAttribute> attrs = mds.getAttributes(voidImpl1);
+
+        assertEquals(1, attrs.size());
+        assertTrue(attrs.get(0) instanceof PreInvocationExpressionConfigAttribute);
+        PreInvocationExpressionConfigAttribute pre = (PreInvocationExpressionConfigAttribute) attrs.get(0);
+        assertNotNull(pre.getAuthorizeExpression());
+        assertEquals("someExpression", pre.getAuthorizeExpression().getExpressionString());
+        assertNull(pre.getFilterExpression());
+    }
+
+    @Test
+    public void mixedClassAndMethodPreAnnotationsAreBothIncluded() {
+        List<ConfigAttribute> attrs = mds.getAttributes(voidImpl2);
+
+        assertEquals(1, attrs.size());
+        assertTrue(attrs.get(0) instanceof PreInvocationExpressionConfigAttribute);
+        PreInvocationExpressionConfigAttribute pre = (PreInvocationExpressionConfigAttribute)attrs.get(0);
+        assertEquals("someExpression", pre.getAuthorizeExpression().getExpressionString());
+        assertNotNull(pre.getFilterExpression());
+        assertEquals("somePreFilterExpression", pre.getFilterExpression().getExpressionString());
+    }
+
+    @Test
+    public void methodWithPreFilterOnlyIsAllowed() {
+        List<ConfigAttribute> attrs = mds.getAttributes(voidImpl3);
+
+        assertEquals(1, attrs.size());
+        assertTrue(attrs.get(0) instanceof PreInvocationExpressionConfigAttribute);
+        PreInvocationExpressionConfigAttribute pre = (PreInvocationExpressionConfigAttribute)attrs.get(0);
+        assertEquals("permitAll", pre.getAuthorizeExpression().getExpressionString());
+        assertNotNull(pre.getFilterExpression());
+        assertEquals("somePreFilterExpression", pre.getFilterExpression().getExpressionString());
+    }
+
+    @Test
+    public void methodWithPostFilterOnlyIsAllowed() {
+        List<ConfigAttribute> attrs = mds.getAttributes(listImpl1);
+
+        assertEquals(2, attrs.size());
+        assertTrue(attrs.get(0) instanceof PreInvocationExpressionConfigAttribute);
+        assertTrue(attrs.get(1) instanceof PostInvocationExpressionConfigAttribute);
+        PreInvocationExpressionConfigAttribute pre = (PreInvocationExpressionConfigAttribute)attrs.get(0);
+        PostInvocationExpressionConfigAttribute post = (PostInvocationExpressionConfigAttribute)attrs.get(1);
+        assertEquals("permitAll", pre.getAuthorizeExpression().getExpressionString());
+        assertNotNull(post.getFilterExpression());
+        assertEquals("somePostFilterExpression", post.getFilterExpression().getExpressionString());
+    }
+
+    @Test
+    public void interfaceAttributesAreIncluded() {
+        List<ConfigAttribute> attrs = mds.getAttributes(notherListImpl1);
+
+        assertEquals(1, attrs.size());
+        assertTrue(attrs.get(0) instanceof PreInvocationExpressionConfigAttribute);
+        PreInvocationExpressionConfigAttribute pre = (PreInvocationExpressionConfigAttribute)attrs.get(0);
+        assertNotNull(pre.getFilterExpression());
+        assertNotNull(pre.getAuthorizeExpression());
+        assertEquals("interfaceMethodAuthzExpression", pre.getAuthorizeExpression().getExpressionString());
+        assertEquals("interfacePreFilterExpression", pre.getFilterExpression().getExpressionString());
+    }
+
+    @Test
+    public void classAttributesTakesPrecedeceOverInterfaceAttributes() {
+        List<ConfigAttribute> attrs = mds.getAttributes(notherListImpl2);
+
+        assertEquals(1, attrs.size());
+        assertTrue(attrs.get(0) instanceof PreInvocationExpressionConfigAttribute);
+        PreInvocationExpressionConfigAttribute pre = (PreInvocationExpressionConfigAttribute)attrs.get(0);
+        assertNotNull(pre.getFilterExpression());
+        assertNotNull(pre.getAuthorizeExpression());
+        assertEquals("interfaceMethodAuthzExpression", pre.getAuthorizeExpression().getExpressionString());
+        assertEquals("classMethodPreFilterExpression", pre.getFilterExpression().getExpressionString());
+    }
+
+    //~ Inner Classes ==================================================================================================
+
+    public static interface ReturnVoid {
+        public void doSomething(List param);
+    }
+
+    public static interface ReturnAList {
+        public List doSomething(List param);
+    }
+
+    @PreAuthorize("interfaceAuthzExpression")
+    public static interface ReturnAnotherList {
+        @PreAuthorize("interfaceMethodAuthzExpression")
+        @PreFilter(filterTarget="param", value="interfacePreFilterExpression")
+        public List doSomething(List param);
+    }
+
+
+    @PreAuthorize("someExpression")
+    public static class ReturnVoidImpl1 implements ReturnVoid {
+        public void doSomething(List param) {}
+    }
+
+    @PreAuthorize("someExpression")
+    public static class ReturnVoidImpl2 implements ReturnVoid {
+        @PreFilter(filterTarget="param", value="somePreFilterExpression")
+        public void doSomething(List param) {}
+    }
+
+    public static class ReturnVoidImpl3 implements ReturnVoid {
+        @PreFilter(filterTarget="param", value="somePreFilterExpression")
+        public void doSomething(List param) {}
+    }
+
+    public static class ReturnAListImpl1 implements ReturnAList {
+        @PostFilter("somePostFilterExpression")
+        public List doSomething(List param) {return param;}
+    }
+
+    public static class ReturnAListImpl2 implements ReturnAList {
+        @PreAuthorize("someExpression")
+        @PreFilter(filterTarget="param", value="somePreFilterExpression")
+        @PostFilter("somePostFilterExpression")
+        @PostAuthorize("somePostAuthorizeExpression")
+        public List doSomething(List param) {return param;}
+    }
+
+    public static class ReturnAnotherListImpl1 implements ReturnAnotherList {
+        public List doSomething(List param) {return param;}
+    }
+
+    public static class ReturnAnotherListImpl2 implements ReturnAnotherList {
+        @PreFilter(filterTarget="param", value="classMethodPreFilterExpression")
+        public List doSomething(List param) {return param;}
+    }
+
+}

core/src/test/java/org/springframework/security/intercept/method/MockMethodInvocation.java

@@ -35,5 +35,3 @@ public class MockMethodInvocation implements MethodInvocation {
         return null;
     }
 }
-
-

core/src/test/resources/log4j.properties

@@ -6,7 +6,7 @@ log4j.rootCategory=INFO, stdout
 
 log4j.appender.stdout=org.apache.log4j.ConsoleAppender
 log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
-log4j.appender.stdout.layout.ConversionPattern=%d %p %c - %m%n
+log4j.appender.stdout.layout.ConversionPattern=%d %p %c{1} - %m%n
 
 log4j.category.org.springframework.security=DEBUG
 log4j.category.org.springframework.ldap=DEBUG