From 469f55ce05382e71db46c58c4edd7076f179dbbd Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Fri, 18 Apr 2008 13:15:56 +0000 Subject: [PATCH] SEC-773: global-method-security fails with JPA http://jira.springframework.org/browse/SEC-773. Added extra constructor to MethodDefinitionSourceAdvisor to allow for lazy initialization of the advice (MethodSecurityInterceptor), and in turn the AuthenticationManager and ay referenced UserDetailsService implementations. --- core-tiger/pom.xml | 6 ++ ...thodSecurityBeanDefinitionParserTests.java | 29 ++++++++- ...balMethodSecurityBeanDefinitionParser.java | 5 +- .../MethodDefinitionSourceAdvisor.java | 61 ++++++++++++++++--- 4 files changed, 89 insertions(+), 12 deletions(-) diff --git a/core-tiger/pom.xml b/core-tiger/pom.xml index b16d4ff2c1..cf9d06c82a 100644 --- a/core-tiger/pom.xml +++ b/core-tiger/pom.xml @@ -15,6 +15,12 @@ spring-security-core ${project.version} + + org.springframework.security + spring-security-core + ${project.version} + tests + org.aspectj aspectjrt diff --git a/core-tiger/src/test/java/org/springframework/security/config/GlobalMethodSecurityBeanDefinitionParserTests.java b/core-tiger/src/test/java/org/springframework/security/config/GlobalMethodSecurityBeanDefinitionParserTests.java index 9a235f55e9..dbeb9ae3df 100644 --- a/core-tiger/src/test/java/org/springframework/security/config/GlobalMethodSecurityBeanDefinitionParserTests.java +++ b/core-tiger/src/test/java/org/springframework/security/config/GlobalMethodSecurityBeanDefinitionParserTests.java @@ -1,8 +1,11 @@ package org.springframework.security.config; +import static org.junit.Assert.assertEquals; + import org.junit.After; import org.junit.Before; import org.junit.Test; +import org.springframework.context.support.AbstractXmlApplicationContext; import org.springframework.context.support.ClassPathXmlApplicationContext; import org.springframework.security.AccessDeniedException; import org.springframework.security.AuthenticationCredentialsNotFoundException; @@ -11,17 +14,17 @@ import org.springframework.security.GrantedAuthorityImpl; import org.springframework.security.annotation.BusinessService; import org.springframework.security.context.SecurityContextHolder; import org.springframework.security.providers.UsernamePasswordAuthenticationToken; +import org.springframework.security.util.InMemoryXmlApplicationContext; /** * @author Ben Alex * @version $Id$ */ public class GlobalMethodSecurityBeanDefinitionParserTests { - private ClassPathXmlApplicationContext appContext; + private AbstractXmlApplicationContext appContext; private BusinessService target; - @Before public void loadContext() { appContext = new ClassPathXmlApplicationContext("org/springframework/security/config/global-method-security.xml"); target = (BusinessService) appContext.getBean("target"); @@ -37,11 +40,13 @@ public class GlobalMethodSecurityBeanDefinitionParserTests { @Test(expected=AuthenticationCredentialsNotFoundException.class) public void targetShouldPreventProtectedMethodInvocationWithNoContext() { + loadContext(); target.someUserMethod1(); } @Test public void targetShouldAllowProtectedMethodInvocationWithCorrectRole() { + loadContext(); UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("Test", "Password", new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_USER")}); SecurityContextHolder.getContext().setAuthentication(token); @@ -51,10 +56,30 @@ public class GlobalMethodSecurityBeanDefinitionParserTests { @Test(expected=AccessDeniedException.class) public void targetShouldPreventProtectedMethodInvocationWithIncorrectRole() { + loadContext(); UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("Test", "Password", new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_SOMEOTHERROLE")}); SecurityContextHolder.getContext().setAuthentication(token); target.someAdminMethod(); } + + @Test + public void doesntInterfereWithBeanPostProcessing() { + setContext( + "" + + "" + + // "" + + "" + + "" + ); + + PostProcessedMockUserDetailsService service = (PostProcessedMockUserDetailsService)appContext.getBean("myUserService"); + + assertEquals("Hello from the post processor!", service.getPostProcessorWasHere()); + } + + private void setContext(String context) { + appContext = new InMemoryXmlApplicationContext(context); + } } diff --git a/core/src/main/java/org/springframework/security/config/GlobalMethodSecurityBeanDefinitionParser.java b/core/src/main/java/org/springframework/security/config/GlobalMethodSecurityBeanDefinitionParser.java index fd9b72825f..1b3111f915 100644 --- a/core/src/main/java/org/springframework/security/config/GlobalMethodSecurityBeanDefinitionParser.java +++ b/core/src/main/java/org/springframework/security/config/GlobalMethodSecurityBeanDefinitionParser.java @@ -138,7 +138,10 @@ class GlobalMethodSecurityBeanDefinitionParser implements BeanDefinitionParser { RootBeanDefinition advisor = new RootBeanDefinition(MethodDefinitionSourceAdvisor.class); advisor.setRole(BeanDefinition.ROLE_INFRASTRUCTURE); advisor.setSource(parserContext.extractSource(element)); - advisor.getConstructorArgumentValues().addGenericArgumentValue(interceptor); + advisor.getConstructorArgumentValues().addGenericArgumentValue(BeanIds.METHOD_SECURITY_INTERCEPTOR); + advisor.getConstructorArgumentValues().addGenericArgumentValue(new RuntimeBeanReference(BeanIds.DELEGATING_METHOD_DEFINITION_SOURCE)); + + //advisor.getConstructorArgumentValues().addGenericArgumentValue(interceptor); parserContext.getRegistry().registerBeanDefinition(BeanIds.METHOD_DEFINITION_SOURCE_ADVISOR, advisor); AopNamespaceUtils.registerAutoProxyCreatorIfNecessary(parserContext, element); diff --git a/core/src/main/java/org/springframework/security/intercept/method/aopalliance/MethodDefinitionSourceAdvisor.java b/core/src/main/java/org/springframework/security/intercept/method/aopalliance/MethodDefinitionSourceAdvisor.java index b99b992c7a..e1606d87f9 100644 --- a/core/src/main/java/org/springframework/security/intercept/method/aopalliance/MethodDefinitionSourceAdvisor.java +++ b/core/src/main/java/org/springframework/security/intercept/method/aopalliance/MethodDefinitionSourceAdvisor.java @@ -23,13 +23,18 @@ import org.aopalliance.intercept.MethodInvocation; import org.springframework.aop.Pointcut; import org.springframework.aop.support.AbstractPointcutAdvisor; import org.springframework.aop.support.StaticMethodMatcherPointcut; +import org.springframework.beans.BeansException; +import org.springframework.beans.factory.BeanFactory; +import org.springframework.beans.factory.BeanFactoryAware; import org.springframework.security.intercept.method.MethodDefinitionSource; import org.springframework.util.Assert; /** * Advisor driven by a {@link MethodDefinitionSource}, used to exclude a {@link MethodSecurityInterceptor} from - * public (ie non-secure) methods.

Because the AOP framework caches advice calculations, this is normally faster - * than just letting the MethodSecurityInterceptor run and find out itself that it has no work to do. + * public (ie non-secure) methods. + *

+ * Because the AOP framework caches advice calculations, this is normally faster than just letting the + * MethodSecurityInterceptor run and find out itself that it has no work to do. *

* This class also allows the use of Spring's * {@link org.springframework.aop.framework.autoproxy.DefaultAdvisorAutoProxyCreator}, which makes @@ -42,22 +47,47 @@ import org.springframework.util.Assert; * @author Ben Alex * @version $Id$ */ -public class MethodDefinitionSourceAdvisor extends AbstractPointcutAdvisor { +public class MethodDefinitionSourceAdvisor extends AbstractPointcutAdvisor implements BeanFactoryAware { //~ Instance fields ================================================================================================ private MethodDefinitionSource attributeSource; private MethodSecurityInterceptor interceptor; - private Pointcut pointcut; + private Pointcut pointcut = new MethodDefinitionSourcePointcut(); + private BeanFactory beanFactory; + private String adviceBeanName; + private final Object adviceMonitor = new Object(); //~ Constructors =================================================================================================== + /** + * @deprecated use the decoupled approach instead + */ public MethodDefinitionSourceAdvisor(MethodSecurityInterceptor advice) { - this.interceptor = advice; - - Assert.notNull(advice.getObjectDefinitionSource(), "Cannot construct a MethodDefinitionSourceAdvisor using a MethodSecurityInterceptor that has no ObjectDefinitionSource configured"); + Assert.notNull(advice.getObjectDefinitionSource(), "Cannot construct a MethodDefinitionSourceAdvisor using a " + + "MethodSecurityInterceptor that has no ObjectDefinitionSource configured"); + this.interceptor = advice; this.attributeSource = advice.getObjectDefinitionSource(); - this.pointcut = new MethodDefinitionSourcePointcut(); + } + + /** + * Alternative constructor for situations where we want the advisor decoupled from the advice. Instead the advice + * bean name should be set. This prevents eager instantiation of the interceptor + * (and hence the AuthenticationManager). See SEC-773, for example. + *

+ * This is essentially the approach taken by subclasses of {@link AbstractBeanFactoryPointcutAdvisor}, which this + * class should extend in future. The original hierarchy and constructor have been retained for backwards + * compatibilty. + * + * @param adviceBeanName name of the MethodSecurityInterceptor bean + * @param attributeSource the attribute source (should be the same as the one used on the interceptor) + */ + public MethodDefinitionSourceAdvisor(String adviceBeanName, MethodDefinitionSource attributeSource) { + Assert.notNull(adviceBeanName, "The adviceBeanName cannot be null"); + Assert.notNull(attributeSource, "The attributeSource cannot be null"); + + this.adviceBeanName = adviceBeanName; + this.attributeSource = attributeSource; } //~ Methods ======================================================================================================== @@ -67,7 +97,20 @@ public class MethodDefinitionSourceAdvisor extends AbstractPointcutAdvisor { } public Advice getAdvice() { - return interceptor; + synchronized (this.adviceMonitor) { + if (interceptor == null) { + Assert.notNull(adviceBeanName, "'adviceBeanName' must be set for use with bean factory lookup."); + Assert.state(beanFactory != null, "BeanFactory must be set to resolve 'adviceBeanName'"); + interceptor = (MethodSecurityInterceptor) + beanFactory.getBean(this.adviceBeanName, MethodSecurityInterceptor.class); + attributeSource = interceptor.getObjectDefinitionSource(); + } + return interceptor; + } + } + + public void setBeanFactory(BeanFactory beanFactory) throws BeansException { + this.beanFactory = beanFactory; } //~ Inner Classes ==================================================================================================