OPEN - issue SEC-905: <protect-pointcut /> pointcuts do not respect method arguments

http://jira.springframework.org/browse/SEC-905. Added extra registration method to MapBasedMethodDefinitionSource which takes a Method instance rather than the method name.
This commit is contained in:
Luke Taylor 2008-08-12 17:11:38 +00:00
parent d9ab0758ee
commit 55d357f42d
8 changed files with 295 additions and 200 deletions

View File

@ -42,5 +42,7 @@ public interface BusinessService {
@RolesAllowed({"ROLE_USER"}) @RolesAllowed({"ROLE_USER"})
public void someUserMethod2(); public void someUserMethod2();
public int someOther(String s);
public int someOther(int input); public int someOther(int input);
} }

View File

@ -26,7 +26,11 @@ public class BusinessServiceImpl<E extends Entity> implements BusinessService {
return entity; return entity;
} }
public int someOther(int input) { public int someOther(String s) {
return input; return 0;
} }
public int someOther(int input) {
return input;
}
} }

View File

@ -27,7 +27,11 @@ public class Jsr250BusinessServiceImpl implements BusinessService {
public void someAdminMethod() { public void someAdminMethod() {
} }
public int someOther(String input) {
return 0;
}
public int someOther(int input) { public int someOther(int input) {
return input; return input;
} }
} }

View File

@ -31,8 +31,8 @@ public class GlobalMethodSecurityBeanDefinitionParserTests {
setContext( setContext(
"<b:bean id='target' class='org.springframework.security.annotation.BusinessServiceImpl'/>" + "<b:bean id='target' class='org.springframework.security.annotation.BusinessServiceImpl'/>" +
"<global-method-security>" + "<global-method-security>" +
" <protect-pointcut expression='execution(* *.someUser*(..))' access='ROLE_USER'/>" + " <protect-pointcut expression='execution(* *.someUser*(..))' access='ROLE_USER'/>" +
" <protect-pointcut expression='execution(* *.someAdmin*(..))' access='ROLE_ADMIN'/>" + " <protect-pointcut expression='execution(* *.someAdmin*(..))' access='ROLE_ADMIN'/>" +
"</global-method-security>" + ConfigTestUtils.AUTH_PROVIDER_XML "</global-method-security>" + ConfigTestUtils.AUTH_PROVIDER_XML
); );
target = (BusinessService) appContext.getBean("target"); target = (BusinessService) appContext.getBean("target");
@ -106,6 +106,21 @@ public class GlobalMethodSecurityBeanDefinitionParserTests {
service.loadUserByUsername("notused"); service.loadUserByUsername("notused");
} }
@Test
public void supportsMethodArgumentsInPointcut() {
setContext(
"<b:bean id='target' class='org.springframework.security.annotation.BusinessServiceImpl'/>" +
"<global-method-security>" +
" <protect-pointcut expression='execution(* *.someOther(String))' access='ROLE_ADMIN'/>" +
" <protect-pointcut expression='execution(* *.BusinessService*(..))' access='ROLE_USER'/>" +
"</global-method-security>" + ConfigTestUtils.AUTH_PROVIDER_XML
);
SecurityContextHolder.getContext().setAuthentication(new UsernamePasswordAuthenticationToken("user", "password"));
target = (BusinessService) appContext.getBean("target");
// someOther(int) should not be matched by someOther(String)
target.someOther(0);
}
@Test(expected=BeanDefinitionParsingException.class) @Test(expected=BeanDefinitionParsingException.class)
public void duplicateElementCausesError() { public void duplicateElementCausesError() {
setContext( setContext(
@ -116,13 +131,13 @@ public class GlobalMethodSecurityBeanDefinitionParserTests {
@Test(expected=AccessDeniedException.class) @Test(expected=AccessDeniedException.class)
public void worksWithoutTargetOrClass() { public void worksWithoutTargetOrClass() {
setContext( setContext(
"<global-method-security secured-annotations='enabled'/>" + "<global-method-security secured-annotations='enabled'/>" +
"<b:bean id='businessService' class='org.springframework.remoting.httpinvoker.HttpInvokerProxyFactoryBean'>" + "<b:bean id='businessService' class='org.springframework.remoting.httpinvoker.HttpInvokerProxyFactoryBean'>" +
" <b:property name='serviceUrl' value='http://localhost:8080/SomeService'/>" + " <b:property name='serviceUrl' value='http://localhost:8080/SomeService'/>" +
" <b:property name='serviceInterface' value='org.springframework.security.annotation.BusinessService'/>" + " <b:property name='serviceInterface' value='org.springframework.security.annotation.BusinessService'/>" +
"</b:bean>" + AUTH_PROVIDER_XML "</b:bean>" + AUTH_PROVIDER_XML
); );
UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("Test", "Password", UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("Test", "Password",
new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_SOMEOTHERROLE")}); new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_SOMEOTHERROLE")});

View File

@ -51,9 +51,9 @@ public class MapBasedMethodDefinitionSource extends AbstractFallbackMethodDefini
private static final Log logger = LogFactory.getLog(MapBasedMethodDefinitionSource.class); private static final Log logger = LogFactory.getLog(MapBasedMethodDefinitionSource.class);
//~ Instance fields ================================================================================================ //~ Instance fields ================================================================================================
private ClassLoader beanClassLoader = ClassUtils.getDefaultClassLoader(); private ClassLoader beanClassLoader = ClassUtils.getDefaultClassLoader();
/** Map from RegisteredMethod to ConfigAttributeDefinition */ /** Map from RegisteredMethod to ConfigAttributeDefinition */
protected Map methodMap = new HashMap(); protected Map methodMap = new HashMap();
/** Map from RegisteredMethod to name pattern used for registration */ /** Map from RegisteredMethod to name pattern used for registration */
@ -77,45 +77,30 @@ public class MapBasedMethodDefinitionSource extends AbstractFallbackMethodDefini
} }
} }
/** /**
* Implementation does not support class-level attributes. * Implementation does not support class-level attributes.
*/ */
protected ConfigAttributeDefinition findAttributes(Class clazz) { protected ConfigAttributeDefinition findAttributes(Class clazz) {
return null; return null;
} }
/**
* Will walk the method inheritance tree to find the most specific declaration applicable.
*/
protected ConfigAttributeDefinition findAttributes(Method method, Class targetClass) {
return findAttributesSpecifiedAgainst(method, targetClass);
}
private ConfigAttributeDefinition findAttributesSpecifiedAgainst(Method method, Class clazz) {
RegisteredMethod registeredMethod = new RegisteredMethod(method, clazz);
if (methodMap.containsKey(registeredMethod)) {
return (ConfigAttributeDefinition) methodMap.get(registeredMethod);
}
// Search superclass
if (clazz.getSuperclass() != null) {
return findAttributesSpecifiedAgainst(method, clazz.getSuperclass());
}
return null;
}
/** /**
* Add configuration attributes for a secure method. * Will walk the method inheritance tree to find the most specific declaration applicable.
*
* @param method the method to be secured
* @param attr required authorities associated with the method
*/ */
private void addSecureMethod(RegisteredMethod method, ConfigAttributeDefinition attr) { protected ConfigAttributeDefinition findAttributes(Method method, Class targetClass) {
Assert.notNull(method, "RegisteredMethod required"); return findAttributesSpecifiedAgainst(method, targetClass);
Assert.notNull(attr, "Configuration attribute required"); }
if (logger.isInfoEnabled()) {
logger.info("Adding secure method [" + method + "] with attributes [" + attr + "]"); private ConfigAttributeDefinition findAttributesSpecifiedAgainst(Method method, Class clazz) {
} RegisteredMethod registeredMethod = new RegisteredMethod(method, clazz);
this.methodMap.put(method, attr); if (methodMap.containsKey(registeredMethod)) {
return (ConfigAttributeDefinition) methodMap.get(registeredMethod);
}
// Search superclass
if (clazz.getSuperclass() != null) {
return findAttributesSpecifiedAgainst(method, clazz.getSuperclass());
}
return null;
} }
/** /**
@ -126,7 +111,7 @@ public class MapBasedMethodDefinitionSource extends AbstractFallbackMethodDefini
* @param attr required authorities associated with the method * @param attr required authorities associated with the method
*/ */
public void addSecureMethod(String name, ConfigAttributeDefinition attr) { public void addSecureMethod(String name, ConfigAttributeDefinition attr) {
int lastDotIndex = name.lastIndexOf("."); int lastDotIndex = name.lastIndexOf(".");
if (lastDotIndex == -1) { if (lastDotIndex == -1) {
throw new IllegalArgumentException("'" + name + "' is not a valid method name: format is FQN.methodName"); throw new IllegalArgumentException("'" + name + "' is not a valid method name: format is FQN.methodName");
@ -192,6 +177,38 @@ public class MapBasedMethodDefinitionSource extends AbstractFallbackMethodDefini
} }
} }
/**
* Adds configuration attributes for a specific method, for example where the method has been
* matched using a pointcut expression. If a match already exists in the map for the method, then
* the existing match will be retained, so that if this method is called for a more general pointcut
* it will not override a more specific one which has already been added. This
*/
public void addSecureMethod(Class javaType, Method method, ConfigAttributeDefinition attr) {
RegisteredMethod key = new RegisteredMethod(method, javaType);
if (methodMap.containsKey(key)) {
logger.debug("Method [" + method + "] is already registered with attributes [" + methodMap.get(key) + "]");
return;
}
methodMap.put(key, attr);
}
/**
* Add configuration attributes for a secure method.
*
* @param method the method to be secured
* @param attr required authorities associated with the method
*/
private void addSecureMethod(RegisteredMethod method, ConfigAttributeDefinition attr) {
Assert.notNull(method, "RegisteredMethod required");
Assert.notNull(attr, "Configuration attribute required");
if (logger.isInfoEnabled()) {
logger.info("Adding secure method [" + method + "] with attributes [" + attr + "]");
}
this.methodMap.put(method, attr);
}
/** /**
* Obtains the configuration attributes explicitly defined against this bean. * Obtains the configuration attributes explicitly defined against this bean.
* *
@ -254,54 +271,54 @@ public class MapBasedMethodDefinitionSource extends AbstractFallbackMethodDefini
attributes.addAll(toMerge.getConfigAttributes()); attributes.addAll(toMerge.getConfigAttributes());
} }
public void setBeanClassLoader(ClassLoader beanClassLoader) { public void setBeanClassLoader(ClassLoader beanClassLoader) {
Assert.notNull(beanClassLoader, "Bean class loader required"); Assert.notNull(beanClassLoader, "Bean class loader required");
this.beanClassLoader = beanClassLoader; this.beanClassLoader = beanClassLoader;
} }
/** /**
* @return map size (for unit tests and diagnostics) * @return map size (for unit tests and diagnostics)
*/ */
public int getMethodMapSize() { public int getMethodMapSize() {
return methodMap.size(); return methodMap.size();
} }
/** /**
* Stores both the Java Method as well as the Class we obtained the Method from. This is necessary because Method only * Stores both the Java Method as well as the Class we obtained the Method from. This is necessary because Method only
* provides us access to the declaring class. It doesn't provide a way for us to introspect which Class the Method * provides us access to the declaring class. It doesn't provide a way for us to introspect which Class the Method
* was registered against. If a given Class inherits and redeclares a method (i.e. calls super();) the registered Class * was registered against. If a given Class inherits and redeclares a method (i.e. calls super();) the registered Class
* and declaring Class are the same. If a given class merely inherits but does not redeclare a method, the registered * and declaring Class are the same. If a given class merely inherits but does not redeclare a method, the registered
* Class will be the Class we're invoking against and the Method will provide details of the declared class. * Class will be the Class we're invoking against and the Method will provide details of the declared class.
*/ */
private class RegisteredMethod { private class RegisteredMethod {
private Method method; private Method method;
private Class registeredJavaType; private Class registeredJavaType;
public RegisteredMethod(Method method, Class registeredJavaType) { public RegisteredMethod(Method method, Class registeredJavaType) {
Assert.notNull(method, "Method required"); Assert.notNull(method, "Method required");
Assert.notNull(registeredJavaType, "Registered Java Type required"); Assert.notNull(registeredJavaType, "Registered Java Type required");
this.method = method; this.method = method;
this.registeredJavaType = registeredJavaType; this.registeredJavaType = registeredJavaType;
} }
public boolean equals(Object obj) { public boolean equals(Object obj) {
if (this == obj) { if (this == obj) {
return true; return true;
} }
if (obj != null && obj instanceof RegisteredMethod) { if (obj != null && obj instanceof RegisteredMethod) {
RegisteredMethod rhs = (RegisteredMethod) obj; RegisteredMethod rhs = (RegisteredMethod) obj;
return method.equals(rhs.method) && registeredJavaType.equals(rhs.registeredJavaType); return method.equals(rhs.method) && registeredJavaType.equals(rhs.registeredJavaType);
} }
return false; return false;
} }
public int hashCode() { public int hashCode() {
return method.hashCode() * registeredJavaType.hashCode(); return method.hashCode() * registeredJavaType.hashCode();
} }
public String toString() { public String toString() {
return "RegisteredMethod[" + registeredJavaType.getName() + "; " + method + "]"; return "RegisteredMethod[" + registeredJavaType.getName() + "; " + method + "]";
} }
} }
} }

View File

@ -57,18 +57,18 @@ public final class ProtectPointcutPostProcessor implements BeanPostProcessor {
private static final Log logger = LogFactory.getLog(ProtectPointcutPostProcessor.class); private static final Log logger = LogFactory.getLog(ProtectPointcutPostProcessor.class);
private Map pointcutMap = new LinkedHashMap(); /** Key: string-based pointcut, value: ConfigAttributeDefinition */ private Map pointcutMap = new LinkedHashMap(); /** Key: string-based pointcut, value: ConfigAttributeDefinition */
private MapBasedMethodDefinitionSource mapBasedMethodDefinitionSource; private MapBasedMethodDefinitionSource mapBasedMethodDefinitionSource;
private PointcutParser parser; private PointcutParser parser;
public ProtectPointcutPostProcessor(MapBasedMethodDefinitionSource mapBasedMethodDefinitionSource) { public ProtectPointcutPostProcessor(MapBasedMethodDefinitionSource mapBasedMethodDefinitionSource) {
Assert.notNull(mapBasedMethodDefinitionSource, "MapBasedMethodDefinitionSource to populate is required"); Assert.notNull(mapBasedMethodDefinitionSource, "MapBasedMethodDefinitionSource to populate is required");
this.mapBasedMethodDefinitionSource = mapBasedMethodDefinitionSource; this.mapBasedMethodDefinitionSource = mapBasedMethodDefinitionSource;
// Setup AspectJ pointcut expression parser // Setup AspectJ pointcut expression parser
Set supportedPrimitives = new HashSet(); Set supportedPrimitives = new HashSet();
supportedPrimitives.add(PointcutPrimitive.EXECUTION); supportedPrimitives.add(PointcutPrimitive.EXECUTION);
supportedPrimitives.add(PointcutPrimitive.ARGS); supportedPrimitives.add(PointcutPrimitive.ARGS);
supportedPrimitives.add(PointcutPrimitive.REFERENCE); supportedPrimitives.add(PointcutPrimitive.REFERENCE);
// supportedPrimitives.add(PointcutPrimitive.THIS); // supportedPrimitives.add(PointcutPrimitive.THIS);
// supportedPrimitives.add(PointcutPrimitive.TARGET); // supportedPrimitives.add(PointcutPrimitive.TARGET);
// supportedPrimitives.add(PointcutPrimitive.WITHIN); // supportedPrimitives.add(PointcutPrimitive.WITHIN);
@ -76,79 +76,79 @@ public final class ProtectPointcutPostProcessor implements BeanPostProcessor {
// supportedPrimitives.add(PointcutPrimitive.AT_WITHIN); // supportedPrimitives.add(PointcutPrimitive.AT_WITHIN);
// supportedPrimitives.add(PointcutPrimitive.AT_ARGS); // supportedPrimitives.add(PointcutPrimitive.AT_ARGS);
// supportedPrimitives.add(PointcutPrimitive.AT_TARGET); // supportedPrimitives.add(PointcutPrimitive.AT_TARGET);
parser = PointcutParser.getPointcutParserSupportingSpecifiedPrimitivesAndUsingContextClassloaderForResolution(supportedPrimitives); parser = PointcutParser.getPointcutParserSupportingSpecifiedPrimitivesAndUsingContextClassloaderForResolution(supportedPrimitives);
} }
public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException { public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
return bean; return bean;
} }
public Object postProcessBeforeInitialization(Object bean, String beanName) throws BeansException { public Object postProcessBeforeInitialization(Object bean, String beanName) throws BeansException {
// Obtain methods for the present bean // Obtain methods for the present bean
Method[] methods; Method[] methods;
try { try {
methods = bean.getClass().getMethods(); methods = bean.getClass().getMethods();
} catch (Exception e) { } catch (Exception e) {
throw new IllegalStateException(e.getMessage()); throw new IllegalStateException(e.getMessage());
} }
// Check to see if any of those methods are compatible with our pointcut expressions // Check to see if any of those methods are compatible with our pointcut expressions
for (int i = 0; i < methods.length; i++) { for (int i = 0; i < methods.length; i++) {
Iterator iter = pointcutMap.keySet().iterator(); Iterator iter = pointcutMap.keySet().iterator();
while (iter.hasNext()) { while (iter.hasNext()) {
String ex = iter.next().toString(); String ex = iter.next().toString();
// Parse the presented AspectJ pointcut expression // Parse the presented AspectJ pointcut expression
PointcutExpression expression = parser.parsePointcutExpression(ex); PointcutExpression expression = parser.parsePointcutExpression(ex);
// Try for the bean class directly // Try for the bean class directly
if (attemptMatch(bean.getClass(), methods[i], expression, beanName)) { if (attemptMatch(bean.getClass(), methods[i], expression, beanName)) {
// We've found the first expression that matches this method, so move onto the next method now // We've found the first expression that matches this method, so move onto the next method now
break; // the "while" loop, not the "for" loop break; // the "while" loop, not the "for" loop
} }
} }
} }
return bean; return bean;
} }
private boolean attemptMatch(Class targetClass, Method method, PointcutExpression expression, String beanName) { private boolean attemptMatch(Class targetClass, Method method, PointcutExpression expression, String beanName) {
// Determine if the presented AspectJ pointcut expression matches this method // Determine if the presented AspectJ pointcut expression matches this method
boolean matches = expression.matchesMethodExecution(method).alwaysMatches(); boolean matches = expression.matchesMethodExecution(method).alwaysMatches();
// Handle accordingly // Handle accordingly
if (matches) { if (matches) {
ConfigAttributeDefinition attr = (ConfigAttributeDefinition) pointcutMap.get(expression.getPointcutExpression()); ConfigAttributeDefinition attr = (ConfigAttributeDefinition) pointcutMap.get(expression.getPointcutExpression());
if (logger.isDebugEnabled()) { if (logger.isDebugEnabled()) {
logger.debug("AspectJ pointcut expression '" + expression.getPointcutExpression() + "' matches target class '" + targetClass.getName() + "' (bean ID '" + beanName + "') for method '" + method + "'; registering security configuration attribute '" + attr + "'"); logger.debug("AspectJ pointcut expression '" + expression.getPointcutExpression() + "' matches target class '" + targetClass.getName() + "' (bean ID '" + beanName + "') for method '" + method + "'; registering security configuration attribute '" + attr + "'");
} }
mapBasedMethodDefinitionSource.addSecureMethod(targetClass, method.getName(), attr); mapBasedMethodDefinitionSource.addSecureMethod(targetClass, method, attr);
} }
return matches; return matches;
} }
public void setPointcutMap(Map map) { public void setPointcutMap(Map map) {
Assert.notEmpty(map); Assert.notEmpty(map);
Iterator i = map.keySet().iterator(); Iterator i = map.keySet().iterator();
while (i.hasNext()) { while (i.hasNext()) {
String expression = i.next().toString(); String expression = i.next().toString();
Object value = map.get(expression); Object value = map.get(expression);
Assert.isInstanceOf(ConfigAttributeDefinition.class, value, "Map keys must be instances of ConfigAttributeDefinition"); Assert.isInstanceOf(ConfigAttributeDefinition.class, value, "Map keys must be instances of ConfigAttributeDefinition");
addPointcut(expression, (ConfigAttributeDefinition) value); addPointcut(expression, (ConfigAttributeDefinition) value);
} }
} }
public void addPointcut(String pointcutExpression, ConfigAttributeDefinition definition) { public void addPointcut(String pointcutExpression, ConfigAttributeDefinition definition) {
Assert.hasText(pointcutExpression, "An AspectJ pointcut expression is required"); Assert.hasText(pointcutExpression, "An AspectJ pointcut expression is required");
Assert.notNull(definition, "ConfigAttributeDefinition required"); Assert.notNull(definition, "ConfigAttributeDefinition required");
pointcutMap.put(pointcutExpression, definition); pointcutMap.put(pointcutExpression, definition);
if (logger.isDebugEnabled()) { if (logger.isDebugEnabled()) {
logger.debug("AspectJ pointcut expression '" + pointcutExpression + "' registered for security configuration attribute '" + definition + "'"); logger.debug("AspectJ pointcut expression '" + pointcutExpression + "' registered for security configuration attribute '" + definition + "'");
} }
} }
} }

View File

@ -6,9 +6,8 @@ public abstract class ConfigTestUtils {
" <user-service id='us'>" + " <user-service id='us'>" +
" <user name='bob' password='bobspassword' authorities='ROLE_A,ROLE_B' />" + " <user name='bob' password='bobspassword' authorities='ROLE_A,ROLE_B' />" +
" <user name='bill' password='billspassword' authorities='ROLE_A,ROLE_B,AUTH_OTHER' />" + " <user name='bill' password='billspassword' authorities='ROLE_A,ROLE_B,AUTH_OTHER' />" +
" <user name='admin' password='password' authorities='ROLE_ADMIN,ROLE_USER' />" +
" <user name='user' password='password' authorities='ROLE_USER' />" +
" </user-service>" + " </user-service>" +
" </authentication-provider>"; " </authentication-provider>";
} }

View File

@ -0,0 +1,54 @@
package org.springframework.security.intercept.method;
import static org.junit.Assert.*;
import java.lang.reflect.Method;
import org.junit.Before;
import org.junit.Test;
import org.springframework.security.ConfigAttributeDefinition;
/**
* Tests for {@link MapBasedMethodDefinitionSource}.
*
* @author Luke Taylor
* @since 2.0.4
*/
public class MapBasedMethodDefinitionSourceTests {
private final ConfigAttributeDefinition ROLE_A = new ConfigAttributeDefinition("ROLE_A");
private final ConfigAttributeDefinition ROLE_B = new ConfigAttributeDefinition("ROLE_B");
private MapBasedMethodDefinitionSource mds;
private Method someMethodString;
private Method someMethodInteger;
@Before
public void initialize() throws Exception {
mds = new MapBasedMethodDefinitionSource();
someMethodString = MockService.class.getMethod("someMethod", String.class);
someMethodInteger = MockService.class.getMethod("someMethod", Integer.class);
}
@Test
public void wildcardedMatchIsOverwrittenByMoreSpecificMatch() {
mds.addSecureMethod(MockService.class, "some*", ROLE_A);
mds.addSecureMethod(MockService.class, "someMethod*", ROLE_B);
assertEquals(ROLE_B, mds.getAttributes(someMethodInteger, MockService.class));
}
@Test
public void methodsWithDifferentArgumentsAreMatchedCorrectly() throws Exception {
mds.addSecureMethod(MockService.class, someMethodInteger, ROLE_A);
mds.addSecureMethod(MockService.class, someMethodString, ROLE_B);
assertEquals(ROLE_A, mds.getAttributes(someMethodInteger, MockService.class));
assertEquals(ROLE_B, mds.getAttributes(someMethodString, MockService.class));
}
private class MockService {
public void someMethod(String s) {
}
public void someMethod(Integer i) {
}
}
}