From 6627f76df7d93dfd85dd57954f11f595b1ab5f07 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Thu, 29 Jan 2015 16:57:56 -0600 Subject: [PATCH] SEC-2758: Make ROLE_ consistent --- .../GlobalMethodSecurityConfiguration.java | 22 ++- .../GlobalMethodSecuritySelector.java | 20 ++- .../Jsr250MetadataSourceConfiguration.java | 14 ++ .../annotation/sec2758/Sec2758Tests.groovy | 143 ++++++++++++++++++ ...tationDrivenBeanDefinitionParserTests.java | 10 ++ .../Jsr250MethodSecurityMetadataSource.java | 34 ++++- .../expression/SecurityExpressionRoot.java | 52 ++++++- ...efaultMethodSecurityExpressionHandler.java | 19 +++ .../access/annotation/BusinessService.java | 3 + .../annotation/BusinessServiceImpl.java | 3 + ...xpressionProtectedBusinessServiceImpl.java | 4 + .../annotation/Jsr250BusinessServiceImpl.java | 4 + ...250MethodSecurityMetadataSourceTests.java} | 65 ++++++-- .../SecurityExpressionRootTests.java | 82 +++++++++- .../method/MethodExpressionVoterTests.java | 2 +- docs/manual/src/docs/asciidoc/index.adoc | 28 ++-- .../DefaultWebSecurityExpressionHandler.java | 21 ++- 17 files changed, 479 insertions(+), 47 deletions(-) create mode 100644 config/src/main/java/org/springframework/security/config/annotation/method/configuration/Jsr250MetadataSourceConfiguration.java create mode 100644 config/src/test/groovy/org/springframework/security/config/annotation/sec2758/Sec2758Tests.groovy rename core/src/test/java/org/springframework/security/access/annotation/{Jsr250MethodDefinitionSourceTests.java => Jsr250MethodSecurityMetadataSourceTests.java} (77%) diff --git a/config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java index 855fef62cd..b4892e6da5 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java +++ b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java @@ -88,6 +88,7 @@ public class GlobalMethodSecurityConfiguration implements ImportAware { private AnnotationAttributes enableMethodSecurity; private ApplicationContext context; private MethodSecurityExpressionHandler expressionHandler; + private Jsr250MethodSecurityMetadataSource jsr250MethodSecurityMetadataSource; /** * Creates the default MethodInterceptor which is a MethodSecurityInterceptor using the following methods to @@ -172,7 +173,6 @@ public class GlobalMethodSecurityConfiguration implements ImportAware { * * @return */ - @SuppressWarnings("rawtypes") protected AccessDecisionManager accessDecisionManager() { List> decisionVoters = new ArrayList>(); ExpressionBasedPreInvocationAdvice expressionAdvice = new ExpressionBasedPreInvocationAdvice(); @@ -282,14 +282,13 @@ public class GlobalMethodSecurityConfiguration implements ImportAware { sources.add(customMethodSecurityMetadataSource); } if (prePostEnabled()) { - sources.add(new PrePostAnnotationSecurityMetadataSource( - attributeFactory)); + sources.add(new PrePostAnnotationSecurityMetadataSource(attributeFactory)); } if (securedEnabled()) { sources.add(new SecuredAnnotationSecurityMetadataSource()); } if (jsr250Enabled()) { - sources.add(new Jsr250MethodSecurityMetadataSource()); + sources.add(jsr250MethodSecurityMetadataSource); } return new DelegatingMethodSecurityMetadataSource(sources); } @@ -344,6 +343,12 @@ public class GlobalMethodSecurityConfiguration implements ImportAware { this.defaultMethodExpressionHandler = objectPostProcessor.postProcess(defaultMethodExpressionHandler); } + @Autowired(required = false) + public void setJsr250MethodSecurityMetadataSource( + Jsr250MethodSecurityMetadataSource jsr250MethodSecurityMetadataSource) { + this.jsr250MethodSecurityMetadataSource = jsr250MethodSecurityMetadataSource; + } + @Autowired(required = false) public void setPermissionEvaluator(List permissionEvaluators) { if(permissionEvaluators.size() != 1) { @@ -352,6 +357,15 @@ public class GlobalMethodSecurityConfiguration implements ImportAware { this.defaultMethodExpressionHandler.setPermissionEvaluator(permissionEvaluators.get(0)); } + @Autowired(required = false) + public void setMethodSecurityExpressionHandler(List handlers) { + if(handlers.size() != 1) { + logger.debug("Not autwiring PermissionEvaluator since size != 1. Got " + handlers); + return; + } + this.expressionHandler = handlers.get(0); + } + @Autowired public void setApplicationContext(ApplicationContext context) { this.context = context; diff --git a/config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecuritySelector.java b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecuritySelector.java index 8af89c51ca..8ad1b00bf7 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecuritySelector.java +++ b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecuritySelector.java @@ -15,6 +15,8 @@ */ package org.springframework.security.config.annotation.method.configuration; +import java.util.ArrayList; +import java.util.List; import java.util.Map; import org.springframework.context.annotation.AdviceMode; @@ -49,10 +51,20 @@ final class GlobalMethodSecuritySelector implements ImportSelector { AdviceMode mode = attributes.getEnum("mode"); String autoProxyClassName = AdviceMode.PROXY == mode ? AutoProxyRegistrar.class.getName() : GlobalMethodSecurityAspectJAutoProxyRegistrar.class.getName(); - if(skipMethodSecurityConfiguration) { - return new String[] { autoProxyClassName }; + + boolean jsr250Enabled = attributes.getBoolean("jsr250Enabled"); + + List classNames = new ArrayList(4); + classNames.add(autoProxyClassName); + + if(!skipMethodSecurityConfiguration) { + classNames.add(GlobalMethodSecurityConfiguration.class.getName()); } - return new String[] { autoProxyClassName, - GlobalMethodSecurityConfiguration.class.getName()}; + + if(jsr250Enabled) { + classNames.add(Jsr250MetadataSourceConfiguration.class.getName()); + } + + return classNames.toArray(new String[0]); } } diff --git a/config/src/main/java/org/springframework/security/config/annotation/method/configuration/Jsr250MetadataSourceConfiguration.java b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/Jsr250MetadataSourceConfiguration.java new file mode 100644 index 0000000000..3a6faaa76b --- /dev/null +++ b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/Jsr250MetadataSourceConfiguration.java @@ -0,0 +1,14 @@ +package org.springframework.security.config.annotation.method.configuration; + +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.security.access.annotation.Jsr250MethodSecurityMetadataSource; + +@Configuration +class Jsr250MetadataSourceConfiguration { + + @Bean + public Jsr250MethodSecurityMetadataSource jsr250MethodSecurityMetadataSource() { + return new Jsr250MethodSecurityMetadataSource(); + } +} diff --git a/config/src/test/groovy/org/springframework/security/config/annotation/sec2758/Sec2758Tests.groovy b/config/src/test/groovy/org/springframework/security/config/annotation/sec2758/Sec2758Tests.groovy new file mode 100644 index 0000000000..9327e55717 --- /dev/null +++ b/config/src/test/groovy/org/springframework/security/config/annotation/sec2758/Sec2758Tests.groovy @@ -0,0 +1,143 @@ +/* + * Copyright 2002-2013 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * 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.config.annotation.sec2758; + +import javax.annotation.security.RolesAllowed; + +import org.springframework.beans.BeansException; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.config.BeanPostProcessor; +import org.springframework.context.annotation.Bean; +import org.springframework.core.PriorityOrdered; +import org.springframework.mock.web.MockFilterChain; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.access.annotation.Jsr250MethodSecurityMetadataSource; +import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler; +import org.springframework.security.access.prepost.PreAuthorize; +import org.springframework.security.authentication.TestingAuthenticationToken +import org.springframework.security.config.annotation.BaseSpringSpec +import org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder; +import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity; +import org.springframework.security.config.annotation.web.builders.HttpSecurity; +import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; +import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; +import org.springframework.security.config.annotation.web.configuration.sec2377.a.* +import org.springframework.security.config.annotation.web.configuration.sec2377.b.* +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.web.access.expression.DefaultWebSecurityExpressionHandler; +import org.springframework.security.web.access.intercept.FilterSecurityInterceptor; +import org.springframework.web.context.support.AnnotationConfigWebApplicationContext + +public class Sec2758Tests extends BaseSpringSpec { + + def cleanup() { + SecurityContextHolder.clearContext() + } + + def "SEC-2758: Verify Passivity Restored with Advice from JIRA"() { + setup: + SecurityContextHolder.context.authentication = new TestingAuthenticationToken("user", "pass", "USER") + loadConfig(SecurityConfig) + Service service = context.getBean(Service) + + when: + findFilter(FilterSecurityInterceptor).doFilter(new MockHttpServletRequest(), new MockHttpServletResponse(), new MockFilterChain()) + then: + noExceptionThrown() + + when: + service.doPreAuthorize() + then: + noExceptionThrown() + + when: + service.doJsr250() + then: + noExceptionThrown() + } + + @EnableWebSecurity + @EnableGlobalMethodSecurity(prePostEnabled=true) + static class SecurityConfig extends WebSecurityConfigurerAdapter { + + @Override + protected void configure(HttpSecurity http) throws Exception { + http + .authorizeRequests() + .anyRequest().hasAnyAuthority("USER"); + } + + @Autowired + public void configureGlobal(AuthenticationManagerBuilder auth) { + auth + .inMemoryAuthentication() + .withUser("user").password("password").authorities("USER") + } + + @Bean + Service service() { + return new ServiceImpl() + } + + @Bean + static DefaultRolesPrefixPostProcessor defaultRolesPrefixPostProcessor() { + new DefaultRolesPrefixPostProcessor() + } + } + + interface Service { + void doPreAuthorize() + void doJsr250() + } + + static class ServiceImpl implements Service { + @PreAuthorize("hasRole('USER')") + void doPreAuthorize() {} + + @RolesAllowed("USER") + void doJsr250() {} + } + + static class DefaultRolesPrefixPostProcessor implements BeanPostProcessor, PriorityOrdered { + + @Override + public Object postProcessAfterInitialization(Object bean, String beanName) + throws BeansException { + if(bean instanceof Jsr250MethodSecurityMetadataSource) { + ((Jsr250MethodSecurityMetadataSource) bean).setDefaultRolePrefix(null); + } + if(bean instanceof DefaultMethodSecurityExpressionHandler) { + ((DefaultMethodSecurityExpressionHandler) bean).setDefaultRolePrefix(null); + } + if(bean instanceof DefaultWebSecurityExpressionHandler) { + ((DefaultWebSecurityExpressionHandler) bean).setDefaultRolePrefix(null); + } + return bean; + } + + @Override + public Object postProcessBeforeInitialization(Object bean, String beanName) + throws BeansException { + return bean; + } + + @Override + public int getOrder() { + return PriorityOrdered.HIGHEST_PRECEDENCE; + } +} +} diff --git a/config/src/test/java/org/springframework/security/config/method/Jsr250AnnotationDrivenBeanDefinitionParserTests.java b/config/src/test/java/org/springframework/security/config/method/Jsr250AnnotationDrivenBeanDefinitionParserTests.java index 52fb9c02ab..c966999908 100644 --- a/config/src/test/java/org/springframework/security/config/method/Jsr250AnnotationDrivenBeanDefinitionParserTests.java +++ b/config/src/test/java/org/springframework/security/config/method/Jsr250AnnotationDrivenBeanDefinitionParserTests.java @@ -68,4 +68,14 @@ public class Jsr250AnnotationDrivenBeanDefinitionParserTests { target.someAdminMethod(); } + + + @Test + public void hasAnyRoleAddsDefaultPrefix() { + UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("Test", "Password", + AuthorityUtils.createAuthorityList("ROLE_USER")); + SecurityContextHolder.getContext().setAuthentication(token); + + target.rolesAllowedUser(); + } } diff --git a/core/src/main/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSource.java index 541f207f83..80fec3b793 100644 --- a/core/src/main/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSource.java +++ b/core/src/main/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSource.java @@ -38,6 +38,24 @@ import org.springframework.security.access.method.AbstractFallbackMethodSecurity */ public class Jsr250MethodSecurityMetadataSource extends AbstractFallbackMethodSecurityMetadataSource { + private String defaultRolePrefix = "ROLE_"; + + /** + *

+ * Sets the default prefix to be added to {@link RolesAllowed}. For example, if {@code @RolesAllowed("ADMIN")} or {@code @RolesAllowed("ADMIN")} is used, + * then the role ROLE_ADMIN will be used when the defaultRolePrefix is "ROLE_" (default). + *

+ * + *

+ * If null or empty, then no default role prefix is used. + *

+ * + * @param defaultRolePrefix the default prefix to add to roles. Default "ROLE_". + */ + public void setDefaultRolePrefix(String defaultRolePrefix) { + this.defaultRolePrefix = defaultRolePrefix; + } + protected Collection findAttributes(Class clazz) { return processAnnotations(clazz.getAnnotations()); } @@ -69,11 +87,25 @@ public class Jsr250MethodSecurityMetadataSource extends AbstractFallbackMethodSe RolesAllowed ra = (RolesAllowed) a; for (String allowed : ra.value()) { - attributes.add(new Jsr250SecurityConfig(allowed)); + String defaultedAllowed = getRoleWithDefaultPrefix(allowed); + attributes.add(new Jsr250SecurityConfig(defaultedAllowed)); } return attributes; } } return null; } + + private String getRoleWithDefaultPrefix(String role) { + if(role == null) { + return role; + } + if(defaultRolePrefix == null || defaultRolePrefix.length() == 0) { + return role; + } + if(role.startsWith(defaultRolePrefix)) { + return role; + } + return defaultRolePrefix + role; + } } diff --git a/core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java b/core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java index fe0a08c463..2887144560 100644 --- a/core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java +++ b/core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java @@ -24,6 +24,7 @@ public abstract class SecurityExpressionRoot implements SecurityExpressionOperat private AuthenticationTrustResolver trustResolver; private RoleHierarchy roleHierarchy; private Set roles; + private String defaultRolePrefix = "ROLE_"; /** Allows "permitAll" expression */ public final boolean permitAll = true; @@ -49,22 +50,27 @@ public abstract class SecurityExpressionRoot implements SecurityExpressionOperat } public final boolean hasAuthority(String authority) { - return hasRole(authority); + return hasAnyAuthority(authority); } public final boolean hasAnyAuthority(String... authorities) { - return hasAnyRole(authorities); + return hasAnyAuthorityName(null, authorities); } public final boolean hasRole(String role) { - return getAuthoritySet().contains(role); + return hasAnyRole(role); } public final boolean hasAnyRole(String... roles) { + return hasAnyAuthorityName(defaultRolePrefix, roles); + } + + private boolean hasAnyAuthorityName(String prefix, String... roles) { Set roleSet = getAuthoritySet(); for (String role : roles) { - if (roleSet.contains(role)) { + String defaultedRole = getRoleWithDefaultPrefix(prefix, role); + if (roleSet.contains(defaultedRole)) { return true; } } @@ -116,6 +122,23 @@ public abstract class SecurityExpressionRoot implements SecurityExpressionOperat this.roleHierarchy = roleHierarchy; } + /** + *

+ * Sets the default prefix to be added to {@link #hasAnyRole(String...)} or + * {@link #hasRole(String)}. For example, if hasRole("ADMIN") or hasRole("ROLE_ADMIN") is passed in, + * then the role ROLE_ADMIN will be used when the defaultRolePrefix is "ROLE_" (default). + *

+ * + *

+ * If null or empty, then no default role prefix is used. + *

+ * + * @param defaultRolePrefix the default prefix to add to roles. Default "ROLE_". + */ + public void setDefaultRolePrefix(String defaultRolePrefix) { + this.defaultRolePrefix = defaultRolePrefix; + } + private Set getAuthoritySet() { if (roles == null) { roles = new HashSet(); @@ -143,4 +166,25 @@ public abstract class SecurityExpressionRoot implements SecurityExpressionOperat public void setPermissionEvaluator(PermissionEvaluator permissionEvaluator) { this.permissionEvaluator = permissionEvaluator; } + + /** + * Prefixes role with defaultRolePrefix if defaultRolePrefix is non-null and + * if role does not already start with defaultRolePrefix. + * + * @param defaultRolePrefix + * @param role + * @return + */ + private static String getRoleWithDefaultPrefix(String defaultRolePrefix, String role) { + if(role == null) { + return role; + } + if(defaultRolePrefix == null || defaultRolePrefix.length() == 0) { + return role; + } + if(role.startsWith(defaultRolePrefix)) { + return role; + } + return defaultRolePrefix + role; + } } diff --git a/core/src/main/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java b/core/src/main/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java index f5094efb5b..c9698c4dcd 100644 --- a/core/src/main/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java +++ b/core/src/main/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java @@ -37,6 +37,7 @@ public class DefaultMethodSecurityExpressionHandler extends AbstractSecurityExpr private AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl(); private ParameterNameDiscoverer parameterNameDiscoverer = new DefaultSecurityParameterNameDiscoverer(); private PermissionCacheOptimizer permissionCacheOptimizer = null; + private String defaultRolePrefix = "ROLE_"; public DefaultMethodSecurityExpressionHandler() { } @@ -57,6 +58,7 @@ public class DefaultMethodSecurityExpressionHandler extends AbstractSecurityExpr root.setPermissionEvaluator(getPermissionEvaluator()); root.setTrustResolver(trustResolver); root.setRoleHierarchy(getRoleHierarchy()); + root.setDefaultRolePrefix(defaultRolePrefix); return root; } @@ -172,4 +174,21 @@ public class DefaultMethodSecurityExpressionHandler extends AbstractSecurityExpr public void setReturnObject(Object returnObject, EvaluationContext ctx) { ((MethodSecurityExpressionOperations)ctx.getRootObject().getValue()).setReturnObject(returnObject); } + + /** + *

+ * Sets the default prefix to be added to {@link #hasAnyRole(String...)} or + * {@link #hasRole(String)}. For example, if hasRole("ADMIN") or hasRole("ROLE_ADMIN") is passed in, + * then the role ROLE_ADMIN will be used when the defaultRolePrefix is "ROLE_" (default). + *

+ * + *

+ * If null or empty, then no default role prefix is used. + *

+ * + * @param defaultRolePrefix the default prefix to add to roles. Default "ROLE_". + */ + public void setDefaultRolePrefix(String defaultRolePrefix) { + this.defaultRolePrefix = defaultRolePrefix; + } } diff --git a/core/src/test/java/org/springframework/security/access/annotation/BusinessService.java b/core/src/test/java/org/springframework/security/access/annotation/BusinessService.java index 8c15effe1a..825e6ad4b0 100644 --- a/core/src/test/java/org/springframework/security/access/annotation/BusinessService.java +++ b/core/src/test/java/org/springframework/security/access/annotation/BusinessService.java @@ -47,6 +47,9 @@ public interface BusinessService extends Serializable { @RolesAllowed({"ROLE_USER"}) public void someUserMethod2(); + @RolesAllowed({"USER"}) + public void rolesAllowedUser(); + public int someOther(String s); public int someOther(int input); diff --git a/core/src/test/java/org/springframework/security/access/annotation/BusinessServiceImpl.java b/core/src/test/java/org/springframework/security/access/annotation/BusinessServiceImpl.java index 721d72b91f..5f060e6c28 100644 --- a/core/src/test/java/org/springframework/security/access/annotation/BusinessServiceImpl.java +++ b/core/src/test/java/org/springframework/security/access/annotation/BusinessServiceImpl.java @@ -49,4 +49,7 @@ public class BusinessServiceImpl implements BusinessService { return null; } + public void rolesAllowedUser() { + + } } diff --git a/core/src/test/java/org/springframework/security/access/annotation/ExpressionProtectedBusinessServiceImpl.java b/core/src/test/java/org/springframework/security/access/annotation/ExpressionProtectedBusinessServiceImpl.java index d0480a1797..b194696cbf 100644 --- a/core/src/test/java/org/springframework/security/access/annotation/ExpressionProtectedBusinessServiceImpl.java +++ b/core/src/test/java/org/springframework/security/access/annotation/ExpressionProtectedBusinessServiceImpl.java @@ -49,4 +49,8 @@ public class ExpressionProtectedBusinessServiceImpl implements BusinessService { public void methodWithBeanNamePropertyAccessExpression(String x) { } + + public void rolesAllowedUser() { + + } } diff --git a/core/src/test/java/org/springframework/security/access/annotation/Jsr250BusinessServiceImpl.java b/core/src/test/java/org/springframework/security/access/annotation/Jsr250BusinessServiceImpl.java index 01b59c26c6..ada25cca71 100644 --- a/core/src/test/java/org/springframework/security/access/annotation/Jsr250BusinessServiceImpl.java +++ b/core/src/test/java/org/springframework/security/access/annotation/Jsr250BusinessServiceImpl.java @@ -50,4 +50,8 @@ public class Jsr250BusinessServiceImpl implements BusinessService { return null; } + @RolesAllowed({"USER"}) + public void rolesAllowedUser() { + + } } diff --git a/core/src/test/java/org/springframework/security/access/annotation/Jsr250MethodDefinitionSourceTests.java b/core/src/test/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSourceTests.java similarity index 77% rename from core/src/test/java/org/springframework/security/access/annotation/Jsr250MethodDefinitionSourceTests.java rename to core/src/test/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSourceTests.java index 4058e3dcde..1fa8cd814e 100644 --- a/core/src/test/java/org/springframework/security/access/annotation/Jsr250MethodDefinitionSourceTests.java +++ b/core/src/test/java/org/springframework/security/access/annotation/Jsr250MethodSecurityMetadataSourceTests.java @@ -24,6 +24,7 @@ import javax.annotation.security.PermitAll; import javax.annotation.security.RolesAllowed; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; import org.springframework.security.access.ConfigAttribute; import org.springframework.security.access.intercept.method.MockMethodInvocation; @@ -32,10 +33,17 @@ import org.springframework.security.access.intercept.method.MockMethodInvocation * @author Luke Taylor * @author Ben Alex */ -public class Jsr250MethodDefinitionSourceTests { - Jsr250MethodSecurityMetadataSource mds = new Jsr250MethodSecurityMetadataSource(); - A a = new A(); - UserAllowedClass userAllowed = new UserAllowedClass(); +public class Jsr250MethodSecurityMetadataSourceTests { + Jsr250MethodSecurityMetadataSource mds; + A a; + UserAllowedClass userAllowed; + + @Before + public void setup() { + mds = new Jsr250MethodSecurityMetadataSource(); + a = new A(); + userAllowed = new UserAllowedClass(); + } private ConfigAttribute[] findAttributes(String methodName) throws Exception { return mds.findAttributes(a.getClass().getMethod(methodName), null).toArray(new ConfigAttribute[0]); @@ -45,7 +53,7 @@ public class Jsr250MethodDefinitionSourceTests { public void methodWithRolesAllowedHasCorrectAttribute() throws Exception { ConfigAttribute[] accessAttributes = findAttributes("adminMethod"); assertEquals(1, accessAttributes.length); - assertEquals("ADMIN", accessAttributes[0].toString()); + assertEquals("ROLE_ADMIN", accessAttributes[0].toString()); } @Test @@ -71,7 +79,41 @@ public class Jsr250MethodDefinitionSourceTests { public void methodRoleOverridesClassRole() throws Exception { Collection accessAttributes = mds.findAttributes(userAllowed.getClass().getMethod("adminMethod"), null); assertEquals(1, accessAttributes.size()); - assertEquals("ADMIN", accessAttributes.toArray()[0].toString()); + assertEquals("ROLE_ADMIN", accessAttributes.toArray()[0].toString()); + } + + @Test + public void customDefaultRolePrefix() throws Exception { + mds.setDefaultRolePrefix("CUSTOMPREFIX_"); + + ConfigAttribute[] accessAttributes = findAttributes("adminMethod"); + assertEquals(1, accessAttributes.length); + assertEquals("CUSTOMPREFIX_ADMIN", accessAttributes[0].toString()); + } + + @Test + public void emptyDefaultRolePrefix() throws Exception { + mds.setDefaultRolePrefix(""); + + ConfigAttribute[] accessAttributes = findAttributes("adminMethod"); + assertEquals(1, accessAttributes.length); + assertEquals("ADMIN", accessAttributes[0].toString()); + } + + @Test + public void nullDefaultRolePrefix() throws Exception { + mds.setDefaultRolePrefix(null); + + ConfigAttribute[] accessAttributes = findAttributes("adminMethod"); + assertEquals(1, accessAttributes.length); + assertEquals("ADMIN", accessAttributes[0].toString()); + } + + @Test + public void alreadyHasDefaultPrefix() throws Exception { + ConfigAttribute[] accessAttributes = findAttributes("roleAdminMethod"); + assertEquals(1, accessAttributes.length); + assertEquals("ROLE_ADMIN", accessAttributes[0].toString()); } // JSR-250 Spec Tests @@ -98,7 +140,7 @@ public class Jsr250MethodDefinitionSourceTests { Collection accessAttributes = mds.getAttributes(mi); assertEquals(1, accessAttributes.size()); - assertEquals("DERIVED", accessAttributes.toArray()[0].toString()); + assertEquals("ROLE_DERIVED", accessAttributes.toArray()[0].toString()); } @Test @@ -108,7 +150,7 @@ public class Jsr250MethodDefinitionSourceTests { Collection accessAttributes = mds.getAttributes(mi); assertEquals(1, accessAttributes.size()); - assertEquals("DERIVED", accessAttributes.toArray()[0].toString()); + assertEquals("ROLE_DERIVED", accessAttributes.toArray()[0].toString()); } @Test @@ -118,7 +160,7 @@ public class Jsr250MethodDefinitionSourceTests { Collection accessAttributes = mds.getAttributes(mi); assertEquals(1, accessAttributes.size()); - assertEquals("EXPLICIT", accessAttributes.toArray()[0].toString()); + assertEquals("ROLE_EXPLICIT", accessAttributes.toArray()[0].toString()); } /** @@ -151,7 +193,7 @@ public class Jsr250MethodDefinitionSourceTests { Collection accessAttributes = mds.getAttributes(mi); assertEquals(1, accessAttributes.size()); - assertEquals("DERIVED", accessAttributes.toArray()[0].toString()); + assertEquals("ROLE_DERIVED", accessAttributes.toArray()[0].toString()); } //~ Inner Classes ====================================================================================================== @@ -163,6 +205,9 @@ public class Jsr250MethodDefinitionSourceTests { @RolesAllowed("ADMIN") public void adminMethod() {} + @RolesAllowed("ROLE_ADMIN") + public void roleAdminMethod() {} + @PermitAll public void permitAllMethod() {} } diff --git a/core/src/test/java/org/springframework/security/access/expression/SecurityExpressionRootTests.java b/core/src/test/java/org/springframework/security/access/expression/SecurityExpressionRootTests.java index 8cfe9feb72..dfe0270ab4 100644 --- a/core/src/test/java/org/springframework/security/access/expression/SecurityExpressionRootTests.java +++ b/core/src/test/java/org/springframework/security/access/expression/SecurityExpressionRootTests.java @@ -3,9 +3,11 @@ package org.springframework.security.access.expression; import static org.junit.Assert.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.fest.assertions.Assertions.*; import java.util.Collection; +import org.junit.Before; import org.junit.Test; import org.springframework.security.access.hierarchicalroles.RoleHierarchy; import org.springframework.security.authentication.AuthenticationTrustResolver; @@ -20,11 +22,17 @@ import org.springframework.security.core.authority.AuthorityUtils; * @since 3.0 */ public class SecurityExpressionRootTests { - final static Authentication JOE = new TestingAuthenticationToken("joe", "pass", "A", "B"); + final static Authentication JOE = new TestingAuthenticationToken("joe", "pass", "ROLE_A", "ROLE_B"); + + SecurityExpressionRoot root; + + @Before + public void setup() { + root = new SecurityExpressionRoot(JOE) {}; + } @Test public void denyAllIsFalsePermitAllTrue() throws Exception { - SecurityExpressionRoot root = new SecurityExpressionRoot(JOE) {}; assertFalse(root.denyAll()); assertFalse(root.denyAll); assertTrue(root.permitAll()); @@ -33,7 +41,6 @@ public class SecurityExpressionRootTests { @Test public void rememberMeIsCorrectlyDetected() throws Exception { - SecurityExpressionRoot root = new SecurityExpressionRoot(JOE) {}; AuthenticationTrustResolver atr = mock(AuthenticationTrustResolver.class); root.setTrustResolver(atr); when(atr.isRememberMe(JOE)).thenReturn(true); @@ -43,20 +50,79 @@ public class SecurityExpressionRootTests { @Test public void roleHierarchySupportIsCorrectlyUsedInEvaluatingRoles() throws Exception { - SecurityExpressionRoot root = new SecurityExpressionRoot(JOE) {}; - root.setRoleHierarchy(new RoleHierarchy() { public Collection getReachableGrantedAuthorities(Collection authorities) { - return AuthorityUtils.createAuthorityList("C"); + return AuthorityUtils.createAuthorityList("ROLE_C"); } }); assertTrue(root.hasRole("C")); - assertTrue(root.hasAuthority("C")); + assertTrue(root.hasAuthority("ROLE_C")); assertFalse(root.hasRole("A")); assertFalse(root.hasRole("B")); assertTrue(root.hasAnyRole("C", "A", "B")); - assertTrue(root.hasAnyAuthority("C", "A", "B")); + assertTrue(root.hasAnyAuthority("ROLE_C", "ROLE_A", "ROLE_B")); assertFalse(root.hasAnyRole("A", "B")); } + + @Test + public void hasRoleAddsDefaultPrefix() throws Exception { + assertThat(root.hasRole("A")).isTrue(); + assertThat(root.hasRole("NO")).isFalse(); + } + + @Test + public void hasRoleEmptyPrefixDoesNotAddsDefaultPrefix() throws Exception { + root.setDefaultRolePrefix(""); + assertThat(root.hasRole("A")).isFalse(); + assertThat(root.hasRole("ROLE_A")).isTrue(); + } + + @Test + public void hasRoleNullPrefixDoesNotAddsDefaultPrefix() throws Exception { + root.setDefaultRolePrefix(null); + assertThat(root.hasRole("A")).isFalse(); + assertThat(root.hasRole("ROLE_A")).isTrue(); + } + + @Test + public void hasRoleDoesNotAddDefaultPrefixForAlreadyPrefixedRoles() throws Exception { + SecurityExpressionRoot root = new SecurityExpressionRoot(JOE) {}; + + assertThat(root.hasRole("ROLE_A")).isTrue(); + assertThat(root.hasRole("ROLE_NO")).isFalse(); + } + + @Test + public void hasAnyRoleAddsDefaultPrefix() throws Exception { + assertThat(root.hasAnyRole("NO","A")).isTrue(); + assertThat(root.hasAnyRole("NO","NOT")).isFalse(); + } + + @Test + public void hasAnyRoleDoesNotAddDefaultPrefixForAlreadyPrefixedRoles() throws Exception { + assertThat(root.hasAnyRole("ROLE_NO","ROLE_A")).isTrue(); + assertThat(root.hasAnyRole("ROLE_NO","ROLE_NOT")).isFalse(); + } + + @Test + public void hasAnyRoleEmptyPrefixDoesNotAddsDefaultPrefix() throws Exception { + root.setDefaultRolePrefix(""); + assertThat(root.hasRole("A")).isFalse(); + assertThat(root.hasRole("ROLE_A")).isTrue(); + } + + @Test + public void hasAnyRoleNullPrefixDoesNotAddsDefaultPrefix() throws Exception { + root.setDefaultRolePrefix(null); + assertThat(root.hasAnyRole("A")).isFalse(); + assertThat(root.hasAnyRole("ROLE_A")).isTrue(); + } + + @Test + public void hasAuthorityDoesNotAddDefaultPrefix() throws Exception { + assertThat(root.hasAuthority("A")).isFalse(); + assertThat(root.hasAnyAuthority("NO","A")).isFalse(); + assertThat(root.hasAnyAuthority("ROLE_A","NOT")).isTrue(); + } } diff --git a/core/src/test/java/org/springframework/security/access/expression/method/MethodExpressionVoterTests.java b/core/src/test/java/org/springframework/security/access/expression/method/MethodExpressionVoterTests.java index a72741b9e1..1a2b7f8453 100644 --- a/core/src/test/java/org/springframework/security/access/expression/method/MethodExpressionVoterTests.java +++ b/core/src/test/java/org/springframework/security/access/expression/method/MethodExpressionVoterTests.java @@ -19,7 +19,7 @@ import org.springframework.security.util.SimpleMethodInvocation; @SuppressWarnings("unchecked") public class MethodExpressionVoterTests { - private TestingAuthenticationToken joe = new TestingAuthenticationToken("joe", "joespass", "blah"); + private TestingAuthenticationToken joe = new TestingAuthenticationToken("joe", "joespass", "ROLE_blah"); private PreInvocationAuthorizationAdviceVoter am = new PreInvocationAuthorizationAdviceVoter(new ExpressionBasedPreInvocationAdvice()); diff --git a/docs/manual/src/docs/asciidoc/index.adoc b/docs/manual/src/docs/asciidoc/index.adoc index 3359e8fd65..d76d373dfc 100644 --- a/docs/manual/src/docs/asciidoc/index.adoc +++ b/docs/manual/src/docs/asciidoc/index.adoc @@ -628,7 +628,7 @@ protected void configure(HttpSecurity http) throws Exception { .authorizeRequests() <1> .antMatchers("/resources/**", "/signup", "/about").permitAll() <2> .antMatchers("/admin/**").hasRole("ADMIN") <3> - .antMatchers("/db/**").access("hasRole('ROLE_ADMIN') and hasRole('ROLE_DBA')") <4> + .antMatchers("/db/**").access("hasRole('ADMIN') and hasRole('DBA')") <4> .anyRequest().authenticated() <5> .and() // ... @@ -639,7 +639,7 @@ protected void configure(HttpSecurity http) throws Exception { <1> There are multiple children to the `http.authorizeRequests()` method each matcher is considered in the order they were declared. <2> We specified multiple URL patterns that any user can access. Specifically, any user can access a request if the URL starts with "/resources/", equals "/signup", or equals "/about". <3> Any URL that starts with "/admin/" will be resticted to users who have the role "ROLE_ADMIN". You will notice that since we are invoking the `hasRole` method we do not need to specify the "ROLE_" prefix. -<4> Any URL that starts with "/db/" requires the user to have both "ROLE_ADMIN" and "ROLE_DBA" +<4> Any URL that starts with "/db/" requires the user to have both "ROLE_ADMIN" and "ROLE_DBA". You will notice that since we are using the `hasRole` expression we do not need to specify the "ROLE_" prefix. <5> Any URL that has not already been matched on only requires that the user be authenticated [[jc-authentication]] @@ -817,7 +817,7 @@ public class MethodSecurityConfig { } ---- -Adding an annotation to a method (on an class or interface) would then limit the access to that method accordingly. Spring Security���s native annotation support defines a set of attributes for the method. These will be passed to the AccessDecisionManager for it to make the actual decision: +Adding an annotation to a method (on an class or interface) would then limit the access to that method accordingly. Spring Security's native annotation support defines a set of attributes for the method. These will be passed to the AccessDecisionManager for it to make the actual decision: [source,java] ---- @@ -844,7 +844,7 @@ public class MethodSecurityConfig { } ---- -These are standards-based and allow simple role-based constraints to be applied but do not have the power Spring Security���s native annotations. To use the new expression-based syntax, you would use +These are standards-based and allow simple role-based constraints to be applied but do not have the power Spring Security's native annotations. To use the new expression-based syntax, you would use [source,java] ---- @@ -1017,7 +1017,7 @@ All you need to enable web security to begin with is [source,xml] ---- - + @@ -2388,7 +2388,7 @@ As we saw earlier in the namespace chapter, it's possible to use multiple `http` ---- - + @@ -2397,7 +2397,7 @@ As we saw earlier in the namespace chapter, it's possible to use multiple `http` - + @@ -4270,16 +4270,16 @@ The base class for expression root objects is `SecurityExpressionRoot`. This pro | Expression | Description | `hasRole([role])` -| Returns `true` if the current principal has the specified role. This is a synonym for `hasAuthority([authority])` +| Returns `true` if the current principal has the specified role. By default if the supplied role does not start with 'ROLE_' it will be added. This can be customized by modifying the `defaultRolePrefix` on `DefaultWebSecurityExpressionHandler`. | `hasAnyRole([role1,role2])` -| Returns `true` if the current principal has any of the supplied roles (given as a comma-separated list of strings) This is a synonym for `hasAnyAuthority([authority1,authority2])` +| Returns `true` if the current principal has any of the supplied roles (given as a comma-separated list of strings). By default if the supplied role does not start with 'ROLE_' it will be added. This can be customized by modifying the `defaultRolePrefix` on `DefaultWebSecurityExpressionHandler`. | `hasAuthority([authority])` -| Returns `true` if the current principal has the specified authority. This is a synonym for `hasRole([role])` +| Returns `true` if the current principal has the specified authority. | `hasAnyAuthority([authority1,authority2])` -| Returns `true` if the current principal has any of the supplied roles (given as a comma-separated list of strings) `hasAnyRole([role1,role2])``hasAnyRole([role1,role2])` +| Returns `true` if the current principal has any of the supplied roles (given as a comma-separated list of strings) | `principal` | Allows direct access to the principal object representing the current user @@ -4351,7 +4351,7 @@ The most obviously useful annotation is `@PreAuthorize` which decides whether a [source,java] ---- -@PreAuthorize("hasRole('ROLE_USER')") +@PreAuthorize("hasRole('USER')") public void create(Contact contact); ---- @@ -4430,7 +4430,7 @@ As you may already be aware, Spring Security supports filtering of collections a [source,java] ---- -@PreAuthorize("hasRole('ROLE_USER')") +@PreAuthorize("hasRole('USER')") @PostFilter("hasPermission(filterObject, 'read') or hasPermission(filterObject, 'admin')") public List getAll(); ---- @@ -7694,7 +7694,7 @@ Defines an authorization rule for a message. * **pattern** An ant based pattern that matches on the Message destination. For example, "/**" matches any Message with a destination; "/admin/**" matches any Message that has a destination that starts with "/admin/**". [[nsa-message-interceptor-access]] -* **access** The expression used to secure the Message. For example, "denyAll" will deny access to all of the matching Messages; "permitAll" will grant access to all of the matching Messages; "hasRole('ROLE_ADMIN') requires the current user to have the role 'ROLE_ADMIN' for the matching Messages. +* **access** The expression used to secure the Message. For example, "denyAll" will deny access to all of the matching Messages; "permitAll" will grant access to all of the matching Messages; "hasRole('ADMIN') requires the current user to have the role 'ROLE_ADMIN' for the matching Messages. [[nsa-authentication]] === Authentication Services diff --git a/web/src/main/java/org/springframework/security/web/access/expression/DefaultWebSecurityExpressionHandler.java b/web/src/main/java/org/springframework/security/web/access/expression/DefaultWebSecurityExpressionHandler.java index dc5794d5fc..ade0c51a74 100644 --- a/web/src/main/java/org/springframework/security/web/access/expression/DefaultWebSecurityExpressionHandler.java +++ b/web/src/main/java/org/springframework/security/web/access/expression/DefaultWebSecurityExpressionHandler.java @@ -17,6 +17,7 @@ import org.springframework.util.Assert; public class DefaultWebSecurityExpressionHandler extends AbstractSecurityExpressionHandler implements SecurityExpressionHandler { private AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl(); + private String defaultRolePrefix = "ROLE_"; @Override protected SecurityExpressionOperations createSecurityExpressionRoot(Authentication authentication, FilterInvocation fi) { @@ -24,6 +25,7 @@ public class DefaultWebSecurityExpressionHandler extends AbstractSecurityExpress root.setPermissionEvaluator(getPermissionEvaluator()); root.setTrustResolver(trustResolver); root.setRoleHierarchy(getRoleHierarchy()); + root.setDefaultRolePrefix(defaultRolePrefix); return root; } @@ -39,4 +41,21 @@ public class DefaultWebSecurityExpressionHandler extends AbstractSecurityExpress Assert.notNull(trustResolver, "trustResolver cannot be null"); this.trustResolver = trustResolver; } -} + + /** + *

+ * Sets the default prefix to be added to {@link #hasAnyRole(String...)} or + * {@link #hasRole(String)}. For example, if hasRole("ADMIN") or hasRole("ROLE_ADMIN") is passed in, + * then the role ROLE_ADMIN will be used when the defaultRolePrefix is "ROLE_" (default). + *

+ * + *

+ * If null or empty, then no default role prefix is used. + *

+ * + * @param defaultRolePrefix the default prefix to add to roles. Default "ROLE_". + */ + public void setDefaultRolePrefix(String defaultRolePrefix) { + this.defaultRolePrefix = defaultRolePrefix; + } +} \ No newline at end of file