diff --git a/core/src/main/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSource.java index e86355ed0f..f9a8d4267a 100644 --- a/core/src/main/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSource.java +++ b/core/src/main/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSource.java @@ -55,7 +55,7 @@ public class SecuredAnnotationSecurityMetadataSource extends AbstractFallbackMet } protected Collection findAttributes(Class clazz) { - return processAnnotation(clazz.getAnnotation(annotationType)); + return processAnnotation(AnnotationUtils.findAnnotation(clazz, annotationType)); } protected Collection findAttributes(Method method, Class targetClass) { diff --git a/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityMetadataSourceAdvisor.java b/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityMetadataSourceAdvisor.java index 9ce083ae34..d7e3054ed8 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityMetadataSourceAdvisor.java +++ b/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(); } } } diff --git a/core/src/main/java/org/springframework/security/access/method/AbstractFallbackMethodSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/method/AbstractFallbackMethodSecurityMetadataSource.java index 343faeaac2..5dfd646dfb 100644 --- a/core/src/main/java/org/springframework/security/access/method/AbstractFallbackMethodSecurityMetadataSource.java +++ b/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(); } /** diff --git a/core/src/main/java/org/springframework/security/access/prepost/PrePostAnnotationSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/prepost/PrePostAnnotationSecurityMetadataSource.java index 64ad62c392..d8f852e9f8 100644 --- a/core/src/main/java/org/springframework/security/access/prepost/PrePostAnnotationSecurityMetadataSource.java +++ b/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 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 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; } diff --git a/core/src/test/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataDefinitionSourceTests.java b/core/src/test/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSourceTests.java similarity index 56% rename from core/src/test/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataDefinitionSourceTests.java rename to core/src/test/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataSourceTests.java index 56d5dfd30c..56b5699bd8 100644 --- a/core/src/test/java/org/springframework/security/access/annotation/SecuredAnnotationSecurityMetadataDefinitionSourceTests.java +++ b/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); -interface DepartmentService extends BusinessService { + ConfigAttribute[] attrs = mds.getAttributes(annotatedAtClassLevel).toArray(new ConfigAttribute[0]); - @Secured({"ROLE_USER"}) - Department someUserMethod3(Department dept); -} - -class DepartmentServiceImpl extends BusinessServiceImpl implements DepartmentService { - - @Secured({"ROLE_ADMIN"}) - public Department someUserMethod3(final Department dept) { - return super.someUserMethod3(dept); - } -} - -// SEC-1491 Related classes. PoC for custom annotation with enum value. - -@CustomSecurityAnnotation(SecurityEnum.ADMIN) -interface CustomAnnotatedService { -} - -class CustomAnnotatedServiceImpl implements CustomAnnotatedService { -} - -enum SecurityEnum implements ConfigAttribute, GrantedAuthority { - ADMIN, - USER; - - public String getAttribute() { - return toString(); + assertEquals(1, attrs.length); + assertEquals("CUSTOM", attrs[0].getAttribute()); } - public String getAuthority() { - return toString(); - } -} - -@Target({ElementType.METHOD, ElementType.TYPE}) -@Retention(RetentionPolicy.RUNTIME) -@interface CustomSecurityAnnotation { - SecurityEnum[] value(); -} - -class CustomSecurityAnnotationMetadataExtractor implements AnnotationMetadataExtractor { - - public Collection extractAttributes(CustomSecurityAnnotation securityAnnotation) { - SecurityEnum[] values = securityAnnotation.value(); - - return EnumSet.copyOf(Arrays.asList(values)); + @Test + public void annotatedAnnotationAtInterfaceLevelIsDetected() throws Exception { + MockMethodInvocation annotatedAtInterfaceLevel = new MockMethodInvocation(new AnnotatedAnnotationAtInterfaceLevel(), ReturnVoid2.class, "doSomething", List.class); + + ConfigAttribute[] attrs = mds.getAttributes(annotatedAtInterfaceLevel).toArray(new ConfigAttribute[0]); + + 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]); + + assertEquals(1, attrs.length); + assertEquals("CUSTOM", attrs[0].getAttribute()); + } + + // Inner classes + class Department extends Entity { + public Department(String name) { + super(name); + } + } + + interface DepartmentService extends BusinessService { + @Secured({"ROLE_USER"}) + Department someUserMethod3(Department dept); + } + + class DepartmentServiceImpl extends BusinessServiceImpl implements DepartmentService { + @Secured({"ROLE_ADMIN"}) + public Department someUserMethod3(final Department dept) { + return super.someUserMethod3(dept); + } + } + + // SEC-1491 Related classes. PoC for custom annotation with enum value. + + @CustomSecurityAnnotation(SecurityEnum.ADMIN) + interface CustomAnnotatedService { + } + + class CustomAnnotatedServiceImpl implements CustomAnnotatedService { + } + + 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 { + public Collection 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) {} + } + + public static class AnnotatedAnnotationAtMethodLevel implements ReturnVoid { + @AnnotatedAnnotation + public void doSomething(List param) {} } } diff --git a/core/src/test/java/org/springframework/security/access/expression/method/PrePostAnnotationSecurityMetadataSourceTests.java b/core/src/test/java/org/springframework/security/access/expression/method/PrePostAnnotationSecurityMetadataSourceTests.java index 18de1406fb..4a77ce43ff 100644 --- a/core/src/test/java/org/springframework/security/access/expression/method/PrePostAnnotationSecurityMetadataSourceTests.java +++ b/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) {} + } } diff --git a/taglibs/src/test/java/org/springframework/security/taglibs/authz/AuthorizeTagExpressionLanguageTests.java b/taglibs/src/test/java/org/springframework/security/taglibs/authz/AuthorizeTagExpressionLanguageTests.java deleted file mode 100644 index e426412e31..0000000000 --- a/taglibs/src/test/java/org/springframework/security/taglibs/authz/AuthorizeTagExpressionLanguageTests.java +++ /dev/null @@ -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()); - } -}