SEC-1796: Check for annotated annotations at class/interface level. Previously only the specific security annotation was checked for. By delegating to Spring's AnnotationUtils, custom annotations carrying the security annotation are also detected.

core/src/main/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSource.java

@@ -55,7 +55,7 @@ public class SecuredAnnotationSecurityMetadataSource extends AbstractFallbackMet
     }
 
     protected Collection<ConfigAttribute> findAttributes(Class<?> clazz) {
-        return processAnnotation(clazz.getAnnotation(annotationType));
+        return processAnnotation(AnnotationUtils.findAnnotation(clazz, annotationType));
     }
 
     protected Collection<ConfigAttribute> findAttributes(Method method, Class<?> targetClass) {

core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityMetadataSourceAdvisor.java

@@ -19,6 +19,7 @@ import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.Serializable;
 import java.lang.reflect.Method;
+import java.util.*;
 
 import org.aopalliance.aop.Advice;
 import org.springframework.aop.Pointcut;
@@ -113,7 +114,8 @@ public class MethodSecurityMetadataSourceAdvisor extends AbstractPointcutAdvisor
     class MethodSecurityMetadataSourcePointcut extends StaticMethodMatcherPointcut implements Serializable {
         @SuppressWarnings("unchecked")
         public boolean matches(Method m, Class targetClass) {
-            return attributeSource.getAttributes(m, targetClass) != null;
+            Collection attributes = attributeSource.getAttributes(m, targetClass);
+            return attributes != null && !attributes.isEmpty();
         }
     }
 }

core/src/main/java/org/springframework/security/access/method/AbstractFallbackMethodSecurityMetadataSource.java

@@ -1,7 +1,7 @@
 package org.springframework.security.access.method;
 
 import java.lang.reflect.Method;
-import java.util.Collection;
+import java.util.*;
 
 import org.springframework.aop.support.AopUtils;
 import org.springframework.security.access.ConfigAttribute;
@@ -51,7 +51,7 @@ public abstract class AbstractFallbackMethodSecurityMetadataSource extends Abstr
             // Last fallback is the class of the original method.
             return findAttributes(method.getDeclaringClass());
         }
-        return null;
+        return Collections.emptyList();
     }
 
     /**

core/src/main/java/org/springframework/security/access/prepost/PrePostAnnotationSecurityMetadataSource.java

@@ -2,8 +2,7 @@ package org.springframework.security.access.prepost;
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
-import java.util.ArrayList;
-import java.util.Collection;
+import java.util.*;
 
 import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.security.access.ConfigAttribute;
@@ -38,13 +37,12 @@ public class PrePostAnnotationSecurityMetadataSource extends AbstractMethodSecur
 
     public Collection<ConfigAttribute> getAttributes(Method method, Class<?> targetClass) {
         if (method.getDeclaringClass() == Object.class) {
-            return null;
+            return Collections.emptyList();
         }
 
         logger.trace("Looking for Pre/Post 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?
@@ -53,7 +51,7 @@ public class PrePostAnnotationSecurityMetadataSource extends AbstractMethodSecur
         if (preFilter == null && preAuthorize == null && postFilter == null && postAuthorize == null ) {
             // There is no meta-data so return
             logger.trace("No expression annotations found");
-            return null;
+            return Collections.emptyList();
         }
 
         String preFilterAttribute = preFilter == null ? null : preFilter.value();
@@ -78,7 +76,7 @@ public class PrePostAnnotationSecurityMetadataSource extends AbstractMethodSecur
 
         attrs.trimToSize();
 
-        return attrs.isEmpty() ? null : attrs;
+        return attrs;
     }
 
     public Collection<ConfigAttribute> getAllConfigAttributes() {
@@ -112,23 +110,13 @@ public class PrePostAnnotationSecurityMetadataSource extends AbstractMethodSecur
         }
 
         // Check the class-level (note declaringClass, not targetClass, which may not actually implement the method)
-        annotation = specificMethod.getDeclaringClass().getAnnotation(annotationClass);
+        annotation = AnnotationUtils.findAnnotation(specificMethod.getDeclaringClass(), annotationClass);
 
         if (annotation != null) {
             logger.debug(annotation + " found on: " + specificMethod.getDeclaringClass().getName());
             return annotation;
         }
 
-        // Check for a possible interface annotation which would not be inherited by the declaring class
-        if (specificMethod != method) {
-            annotation = method.getDeclaringClass().getAnnotation(annotationClass);
-
-            if (annotation != null) {
-                logger.debug(annotation + " found on: " + method.getDeclaringClass().getName());
-                return annotation;
-            }
-        }
-
         return null;
     }
 

core/src/test/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataDefinitionSourceTests.java → core/src/test/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSourceTests.java

@@ -19,9 +19,11 @@ import static org.junit.Assert.*;
 import org.junit.*;
 import org.springframework.security.access.ConfigAttribute;
 import org.springframework.security.access.SecurityConfig;
+import org.springframework.security.access.intercept.method.MockMethodInvocation;
 import org.springframework.security.core.GrantedAuthority;
 
 import java.lang.annotation.ElementType;
+import java.lang.annotation.Inherited;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
@@ -37,7 +39,7 @@ import java.util.*;
  * @author Ben Alex
  * @author Luke Taylor
  */
-public class SecuredAnnotationSecurityMetadataDefinitionSourceTests {
+public class SecuredAnnotationSecurityMetadataSourceTests {
     //~ Instance fields ================================================================================================
 
     private SecuredAnnotationSecurityMetadataSource mds = new SecuredAnnotationSecurityMetadataSource();
@@ -137,6 +139,7 @@ public class SecuredAnnotationSecurityMetadataDefinitionSourceTests {
         assertTrue(user && admin);
     }
 
+    // SEC-1491
     @Test
     public void customAnnotationAttributesAreFound() throws Exception {
         SecuredAnnotationSecurityMetadataSource mds =
@@ -145,61 +148,117 @@ public class SecuredAnnotationSecurityMetadataDefinitionSourceTests {
         assertEquals(1, attrs.size());
         assertEquals(SecurityEnum.ADMIN, attrs.toArray()[0]);
     }
-}
 
-class Department extends Entity {
-    public Department(String name) {
-        super(name);
+    @Test
+    public void annotatedAnnotationAtClassLevelIsDetected() throws Exception {
+        MockMethodInvocation annotatedAtClassLevel = new MockMethodInvocation(new AnnotatedAnnotationAtClassLevel(), ReturnVoid.class, "doSomething", List.class);
+
+        ConfigAttribute[] attrs = mds.getAttributes(annotatedAtClassLevel).toArray(new ConfigAttribute[0]);
+
+        assertEquals(1, attrs.length);
+        assertEquals("CUSTOM", attrs[0].getAttribute());
     }
-}
 
-interface DepartmentService extends BusinessService {
+    @Test
+    public void annotatedAnnotationAtInterfaceLevelIsDetected() throws Exception {
+        MockMethodInvocation annotatedAtInterfaceLevel = new MockMethodInvocation(new AnnotatedAnnotationAtInterfaceLevel(), ReturnVoid2.class, "doSomething", List.class);
 
-    @Secured({"ROLE_USER"})
-    Department someUserMethod3(Department dept);
-}
+        ConfigAttribute[] attrs = mds.getAttributes(annotatedAtInterfaceLevel).toArray(new ConfigAttribute[0]);
 
-class DepartmentServiceImpl extends BusinessServiceImpl<Department> implements DepartmentService {
+        assertEquals(1, attrs.length);
+        assertEquals("CUSTOM", attrs[0].getAttribute());
+    }
+
+    @Test
+    public void annotatedAnnotationAtMethodLevelIsDetected() throws Exception {
+        MockMethodInvocation annotatedAtMethodLevel = new MockMethodInvocation(new AnnotatedAnnotationAtMethodLevel(), ReturnVoid.class, "doSomething", List.class);
+        ConfigAttribute[] attrs = mds.getAttributes(annotatedAtMethodLevel).toArray(new ConfigAttribute[0]);
 
-    @Secured({"ROLE_ADMIN"})
-    public Department someUserMethod3(final Department dept) {
-        return super.someUserMethod3(dept);
+        assertEquals(1, attrs.length);
+        assertEquals("CUSTOM", attrs[0].getAttribute());
     }
-}
 
-// SEC-1491 Related classes. PoC for custom annotation with enum value.
+    // Inner classes
+    class Department extends Entity {
+        public Department(String name) {
+            super(name);
+        }
+    }
 
-@CustomSecurityAnnotation(SecurityEnum.ADMIN)
-interface CustomAnnotatedService {
-}
+    interface DepartmentService extends BusinessService {
+        @Secured({"ROLE_USER"})
+        Department someUserMethod3(Department dept);
+    }
 
-class CustomAnnotatedServiceImpl implements CustomAnnotatedService {
-}
+    class DepartmentServiceImpl extends BusinessServiceImpl<Department> implements DepartmentService {
+        @Secured({"ROLE_ADMIN"})
+        public Department someUserMethod3(final Department dept) {
+            return super.someUserMethod3(dept);
+        }
+    }
 
-enum SecurityEnum implements ConfigAttribute, GrantedAuthority {
-    ADMIN,
-    USER;
+    // SEC-1491 Related classes. PoC for custom annotation with enum value.
 
-    public String getAttribute() {
-        return toString();
+    @CustomSecurityAnnotation(SecurityEnum.ADMIN)
+    interface CustomAnnotatedService {
     }
 
-    public String getAuthority() {
-        return toString();
+    class CustomAnnotatedServiceImpl implements CustomAnnotatedService {
     }
-}
 
-@Target({ElementType.METHOD, ElementType.TYPE})
-@Retention(RetentionPolicy.RUNTIME)
-@interface CustomSecurityAnnotation {
-    SecurityEnum[] value();
-}
+    enum SecurityEnum implements ConfigAttribute, GrantedAuthority {
+        ADMIN,
+        USER;
+
+        public String getAttribute() {
+            return toString();
+        }
+
+        public String getAuthority() {
+            return toString();
+        }
+    }
+
+    @Target({ElementType.METHOD, ElementType.TYPE})
+    @Retention(RetentionPolicy.RUNTIME)
+    @interface CustomSecurityAnnotation {
+        SecurityEnum[] value();
+    }
 
-class CustomSecurityAnnotationMetadataExtractor implements AnnotationMetadataExtractor<CustomSecurityAnnotation> {
+    class CustomSecurityAnnotationMetadataExtractor implements AnnotationMetadataExtractor<CustomSecurityAnnotation> {
+        public Collection<? extends ConfigAttribute> extractAttributes(CustomSecurityAnnotation securityAnnotation) {
+            SecurityEnum[] values = securityAnnotation.value();
 
-    public Collection<? extends ConfigAttribute> extractAttributes(CustomSecurityAnnotation securityAnnotation) {
-        SecurityEnum[] values = securityAnnotation.value();
+            return EnumSet.copyOf(Arrays.asList(values));
+        }
+    }
+
+    @Target({ ElementType.METHOD, ElementType.TYPE })
+    @Retention(RetentionPolicy.RUNTIME)
+    @Inherited
+    @Secured("CUSTOM")
+    public @interface AnnotatedAnnotation {}
+
+    public static interface ReturnVoid {
+        public void doSomething(List<?> param);
+    }
+
+    @AnnotatedAnnotation
+    public static interface ReturnVoid2 {
+        public void doSomething(List<?> param);
+    }
+
+    @AnnotatedAnnotation
+    public static class AnnotatedAnnotationAtClassLevel implements ReturnVoid {
+        public void doSomething(List<?> param) {}
+    }
+
+    public static class AnnotatedAnnotationAtInterfaceLevel implements ReturnVoid2 {
+        public void doSomething(List<?> param) {}
+    }
 
-        return EnumSet.copyOf(Arrays.asList(values));
+    public static class AnnotatedAnnotationAtMethodLevel implements ReturnVoid {
+        @AnnotatedAnnotation
+        public void doSomething(List<?> param) {}
     }
 }

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

@@ -2,6 +2,11 @@ package org.springframework.security.access.expression.method;
 
 import static org.junit.Assert.*;
 
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Inherited;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
 import java.util.List;
 
 import org.junit.Before;
@@ -29,6 +34,9 @@ public class PrePostAnnotationSecurityMetadataSourceTests {
     private MockMethodInvocation listImpl1;
     private MockMethodInvocation notherListImpl1;
     private MockMethodInvocation notherListImpl2;
+    private MockMethodInvocation annotatedAtClassLevel;
+    private MockMethodInvocation annotatedAtInterfaceLevel;
+    private MockMethodInvocation annotatedAtMethodLevel;
 
     @Before
     public void setUpData() throws Exception {
@@ -38,6 +46,9 @@ public class PrePostAnnotationSecurityMetadataSourceTests {
         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);
+        annotatedAtClassLevel = new MockMethodInvocation(new CustomAnnotationAtClassLevel(), ReturnVoid.class, "doSomething", List.class);
+        annotatedAtInterfaceLevel = new MockMethodInvocation(new CustomAnnotationAtInterfaceLevel(), ReturnVoid2.class, "doSomething", List.class);
+        annotatedAtMethodLevel = new MockMethodInvocation(new CustomAnnotationAtMethodLevel(), ReturnVoid.class, "doSomething", List.class);
     }
 
     @Test
@@ -116,6 +127,27 @@ public class PrePostAnnotationSecurityMetadataSourceTests {
         assertEquals("classMethodPreFilterExpression", pre.getFilterExpression().getExpressionString());
     }
 
+    @Test
+    public void customAnnotationAtClassLevelIsDetected() throws Exception {
+        ConfigAttribute[] attrs = mds.getAttributes(annotatedAtClassLevel).toArray(new ConfigAttribute[0]);
+
+        assertEquals(1, attrs.length);
+    }
+
+    @Test
+    public void customAnnotationAtInterfaceLevelIsDetected() throws Exception {
+        ConfigAttribute[] attrs = mds.getAttributes(annotatedAtInterfaceLevel).toArray(new ConfigAttribute[0]);
+
+        assertEquals(1, attrs.length);
+    }
+
+    @Test
+    public void customAnnotationAtMethodLevelIsDetected() throws Exception {
+        ConfigAttribute[] attrs = mds.getAttributes(annotatedAtMethodLevel).toArray(new ConfigAttribute[0]);
+
+        assertEquals(1, attrs.length);
+    }
+
     //~ Inner Classes ==================================================================================================
 
     public static interface ReturnVoid {
@@ -172,4 +204,28 @@ public class PrePostAnnotationSecurityMetadataSourceTests {
         public List<?> doSomething(List<?> param) {return param;}
     }
 
+    @Target({ ElementType.METHOD, ElementType.TYPE })
+    @Retention(RetentionPolicy.RUNTIME)
+    @Inherited
+    @PreAuthorize("customAnnotationExpression")
+    public @interface CustomAnnotation {}
+
+    @CustomAnnotation
+    public static interface ReturnVoid2 {
+        public void doSomething(List<?> param);
+    }
+
+    @CustomAnnotation
+    public static class CustomAnnotationAtClassLevel implements ReturnVoid {
+        public void doSomething(List<?> param) {}
+    }
+
+    public static class CustomAnnotationAtInterfaceLevel implements ReturnVoid2 {
+        public void doSomething(List<?> param) {}
+    }
+
+    public static class CustomAnnotationAtMethodLevel implements ReturnVoid {
+        @CustomAnnotation
+        public void doSomething(List<?> param) {}
+    }
 }

taglibs/src/test/java/org/springframework/security/taglibs/authz/AuthorizeTagExpressionLanguageTests.java

@@ -1,75 +0,0 @@
-/* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.springframework.security.taglibs.authz;
-
-import junit.framework.TestCase;
-import org.springframework.mock.web.MockPageContext;
-import org.springframework.security.authentication.TestingAuthenticationToken;
-import org.springframework.security.core.context.SecurityContextHolder;
-
-import javax.servlet.jsp.JspException;
-import javax.servlet.jsp.el.VariableResolver;
-import javax.servlet.jsp.tagext.Tag;
-
-
-/**
- * Test case to implement commons-el expression language expansion.
- */
-public class AuthorizeTagExpressionLanguageTests extends TestCase {
-    //~ Instance fields ================================================================================================
-
-    private final JspAuthorizeTag authorizeTag = new JspAuthorizeTag();
-    private MockPageContext pageContext;
-
-    //~ Methods ========================================================================================================
-
-    protected void setUp() throws Exception {
-        pageContext = new MockPageContext() {
-            public VariableResolver getVariableResolver() {
-                return null;
-            }
-        };
-        authorizeTag.setPageContext(pageContext);
-        SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken("abc", "123", "ROLE_TELLER"));
-    }
-
-    protected void tearDown() throws Exception {
-        SecurityContextHolder.clearContext();
-    }
-
-    public void testAllGrantedUsesExpressionLanguageWhenExpressionIsEL() throws JspException {
-        pageContext.setAttribute("authority", "ROLE_TELLER");
-        authorizeTag.setIfAllGranted("${authority}");
-
-        assertEquals("allows body - authority var contains ROLE_TELLER", Tag.EVAL_BODY_INCLUDE,
-            authorizeTag.doStartTag());
-    }
-
-    public void testAnyGrantedUsesExpressionLanguageWhenExpressionIsEL() throws JspException {
-        pageContext.setAttribute("authority", "ROLE_TELLER");
-        authorizeTag.setIfAnyGranted("${authority}");
-
-        assertEquals("allows body - authority var contains ROLE_TELLER", Tag.EVAL_BODY_INCLUDE,
-            authorizeTag.doStartTag());
-    }
-
-    public void testNotGrantedUsesExpressionLanguageWhenExpressionIsEL() throws JspException {
-        pageContext.setAttribute("authority", "ROLE_TELLER");
-        authorizeTag.setIfNotGranted("${authority}");
-
-        assertEquals("allows body - authority var contains ROLE_TELLER", Tag.SKIP_BODY, authorizeTag.doStartTag());
-    }
-}