SEC-933: global-method-security and aop:aspectj-autoproxy throws NullPointerException in some situations

http://jira.springframework.org/browse/SEC-933. Removed the setting of the attributeSource field from the interceptor in MethodDefinitionSourceAdvisor as this was overwriting the version supplied with the constructor with null (causing the NPE).
Also implemented lazy initialization of the authentication provider list from the bean factory in a custom NamespaceAuthenticationManager (extends ProviderManager and introspects the BeanFactory when getProviders() is first called). This should prevent the perennial problem of the eager initialization of UserDetailsService and other beans when the interceptor is eagerly initialized by something like aspectj-autoproxy.

core/src/main/java/org/springframework/security/config/AnonymousBeanDefinitionParser.java

@@ -61,14 +61,13 @@ public class AnonymousBeanDefinitionParser implements BeanDefinitionParser {
         filter.getPropertyValues().addPropertyValue("userAttribute", username + "," + grantedAuthority);
         filter.getPropertyValues().addPropertyValue(ATT_KEY, key);
 
-        BeanDefinition authManager = ConfigUtils.registerProviderManagerIfNecessary(parserContext);
         RootBeanDefinition provider = new RootBeanDefinition(AnonymousAuthenticationProvider.class);
         provider.setRole(BeanDefinition.ROLE_INFRASTRUCTURE);
         provider.setSource(source);
         provider.getPropertyValues().addPropertyValue(ATT_KEY, key);
-
-        ManagedList authMgrProviderList = (ManagedList) authManager.getPropertyValues().getPropertyValue("providers").getValue();
-        authMgrProviderList.add(provider);
+        
+        parserContext.getRegistry().registerBeanDefinition(BeanIds.ANONYMOUS_AUTHENTICATION_PROVIDER, provider);
+        ConfigUtils.addAuthenticationProvider(parserContext, BeanIds.ANONYMOUS_AUTHENTICATION_PROVIDER);
 
         parserContext.getRegistry().registerBeanDefinition(BeanIds.ANONYMOUS_PROCESSING_FILTER, filter);
         ConfigUtils.addHttpFilter(parserContext, new RuntimeBeanReference(BeanIds.ANONYMOUS_PROCESSING_FILTER));

core/src/main/java/org/springframework/security/config/AuthenticationManagerBeanDefinitionParser.java

@@ -22,7 +22,7 @@ public class AuthenticationManagerBeanDefinitionParser implements BeanDefinition
 	private static final String ATT_ALIAS = "alias";
 
     public BeanDefinition parse(Element element, ParserContext parserContext) {
-    	BeanDefinition authManager = ConfigUtils.registerProviderManagerIfNecessary(parserContext);
+    	ConfigUtils.registerProviderManagerIfNecessary(parserContext);
     	
         String alias = element.getAttribute(ATT_ALIAS);
 
@@ -33,6 +33,7 @@ public class AuthenticationManagerBeanDefinitionParser implements BeanDefinition
         String sessionControllerRef = element.getAttribute(ATT_SESSION_CONTROLLER_REF);
         
         if (StringUtils.hasText(sessionControllerRef)) {
+        	BeanDefinition authManager = parserContext.getRegistry().getBeanDefinition(BeanIds.AUTHENTICATION_MANAGER);
             ConfigUtils.setSessionControllerOnAuthenticationManager(parserContext, 
             		BeanIds.CONCURRENT_SESSION_CONTROLLER, element);
         	authManager.getPropertyValues().addPropertyValue("sessionController", 

core/src/main/java/org/springframework/security/config/AuthenticationProviderBeanDefinitionParser.java

@@ -94,7 +94,7 @@ class AuthenticationProviderBeanDefinitionParser implements BeanDefinitionParser
         parserContext.getRegistry().registerBeanDefinition(name , cacheResolver);
         parserContext.registerComponent(new BeanComponentDefinition(cacheResolver, name));
         
-        ConfigUtils.getRegisteredProviders(parserContext).add(new RuntimeBeanReference(id));        
+        ConfigUtils.addAuthenticationProvider(parserContext, id);        
         
         return null;
     }

core/src/main/java/org/springframework/security/config/BeanIds.java

@@ -42,6 +42,7 @@ public abstract class BeanIds {
 	public static final String MAIN_ENTRY_POINT = "_mainEntryPoint";	
 	public static final String FILTER_CHAIN_PROXY = "_filterChainProxy";
 	public static final String HTTP_SESSION_CONTEXT_INTEGRATION_FILTER = "_httpSessionContextIntegrationFilter";
+	public static final String LDAP_AUTHENTICATION_PROVIDER = "_ldapAuthenticationProvider";	
 	public static final String LOGOUT_FILTER = "_logoutFilter";
 	public static final String EXCEPTION_TRANSLATION_FILTER = "_exceptionTranslationFilter";
 	public static final String FILTER_SECURITY_INTERCEPTOR = "_filterSecurityInterceptor";
@@ -49,6 +50,7 @@ public abstract class BeanIds {
 	public static final String CHANNEL_DECISION_MANAGER = "_channelDecisionManager";
 	public static final String REMEMBER_ME_FILTER = "_rememberMeFilter";
 	public static final String REMEMBER_ME_SERVICES = "_rememberMeServices";
+	public static final String REMEMBER_ME_AUTHENTICATION_PROVIDER = "_rememberMeAuthenticationProvider";
 	public static final String DEFAULT_LOGIN_PAGE_GENERATING_FILTER = "_defaultLoginPageFilter";
 	public static final String SECURITY_CONTEXT_HOLDER_AWARE_REQUEST_FILTER = "_securityContextHolderAwareRequestFilter";
 	public static final String SESSION_FIXATION_PROTECTION_FILTER = "_sessionFixationProtectionFilter";	
@@ -66,5 +68,4 @@ public abstract class BeanIds {
     public static final String X509_AUTH_PROVIDER = "_x509AuthenticationProvider";
     public static final String PRE_AUTH_ENTRY_POINT = "_preAuthenticatedProcessingFilterEntryPoint";
     public static final String REMEMBER_ME_SERVICES_INJECTION_POST_PROCESSOR = "_rememberMeServicesInjectionBeanPostProcessor";
-
 }

core/src/main/java/org/springframework/security/config/ConfigUtils.java

@@ -1,7 +1,7 @@
 package org.springframework.security.config;
 
+import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -9,15 +9,12 @@ import org.springframework.beans.BeanMetadataElement;
 import org.springframework.beans.MutablePropertyValues;
 import org.springframework.beans.PropertyValue;
 import org.springframework.beans.factory.config.BeanDefinition;
-import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
 import org.springframework.beans.factory.config.RuntimeBeanReference;
 import org.springframework.beans.factory.support.BeanDefinitionBuilder;
 import org.springframework.beans.factory.support.ManagedList;
 import org.springframework.beans.factory.support.RootBeanDefinition;
 import org.springframework.beans.factory.xml.ParserContext;
 import org.springframework.security.afterinvocation.AfterInvocationProviderManager;
-import org.springframework.security.providers.ProviderManager;
-import org.springframework.security.userdetails.UserDetailsService;
 import org.springframework.security.util.UrlUtils;
 import org.springframework.security.vote.AffirmativeBased;
 import org.springframework.security.vote.AuthenticatedVoter;
@@ -80,21 +77,20 @@ public abstract class ConfigUtils {
      * using the <security:provider /> tag or by other beans which have a dependency on the
      * authentication manager.
      */
-    static BeanDefinition registerProviderManagerIfNecessary(ParserContext parserContext) {
+    static void registerProviderManagerIfNecessary(ParserContext parserContext) {
         if(parserContext.getRegistry().containsBeanDefinition(BeanIds.AUTHENTICATION_MANAGER)) {
-            return parserContext.getRegistry().getBeanDefinition(BeanIds.AUTHENTICATION_MANAGER);
+            return;
         }
 
-        BeanDefinition authManager = new RootBeanDefinition(ProviderManager.class);
-        authManager.getPropertyValues().addPropertyValue("providers", new ManagedList());
+        BeanDefinition authManager = new RootBeanDefinition(NamespaceAuthenticationManager.class);
+        authManager.getPropertyValues().addPropertyValue("providerBeanNames", new ArrayList());
         parserContext.getRegistry().registerBeanDefinition(BeanIds.AUTHENTICATION_MANAGER, authManager);
-
-        return authManager;
     }
 
-    static ManagedList getRegisteredProviders(ParserContext parserContext) {
-        BeanDefinition authManager = registerProviderManagerIfNecessary(parserContext);
-        return (ManagedList) authManager.getPropertyValues().getPropertyValue("providers").getValue();
+    static void addAuthenticationProvider(ParserContext parserContext, String beanName) {
+        registerProviderManagerIfNecessary(parserContext);
+        BeanDefinition authManager = parserContext.getRegistry().getBeanDefinition(BeanIds.AUTHENTICATION_MANAGER);
+        ((ArrayList) authManager.getPropertyValues().getPropertyValue("providerBeanNames").getValue()).add(beanName);
     }
     
 	static ManagedList getRegisteredAfterInvocationProviders(ParserContext parserContext) {
@@ -172,7 +168,8 @@ public abstract class ConfigUtils {
     }
     
     static void setSessionControllerOnAuthenticationManager(ParserContext pc, String beanName, Element sourceElt) {
-    	BeanDefinition authManager = registerProviderManagerIfNecessary(pc);
+    	registerProviderManagerIfNecessary(pc);
+    	BeanDefinition authManager = pc.getRegistry().getBeanDefinition(BeanIds.AUTHENTICATION_MANAGER);
         PropertyValue pv = authManager.getPropertyValues().getPropertyValue("sessionController");
         
         if (pv != null && pv.getValue() != null) {

core/src/main/java/org/springframework/security/config/CustomAuthenticationProviderBeanDefinitionDecorator.java

@@ -17,7 +17,7 @@ import org.w3c.dom.Node;
  */
 public class CustomAuthenticationProviderBeanDefinitionDecorator implements BeanDefinitionDecorator {
     public BeanDefinitionHolder decorate(Node node, BeanDefinitionHolder holder, ParserContext parserContext) {
-        ConfigUtils.getRegisteredProviders(parserContext).add(new RuntimeBeanReference(holder.getBeanName()));
+        ConfigUtils.addAuthenticationProvider(parserContext, holder.getBeanName());
 
         return holder;
     }

core/src/main/java/org/springframework/security/config/HttpSecurityBeanDefinitionParser.java

@@ -403,7 +403,7 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser {
             
             BeanDefinition openIDProvider = openIDProviderBuilder.getBeanDefinition();
             pc.getRegistry().registerBeanDefinition(BeanIds.OPEN_ID_PROVIDER, openIDProvider);
-            ConfigUtils.getRegisteredProviders(pc).add(new RuntimeBeanReference(BeanIds.OPEN_ID_PROVIDER));
+            ConfigUtils.addAuthenticationProvider(pc, BeanIds.OPEN_ID_PROVIDER);
         }
         
         boolean needLoginPage = false;

core/src/main/java/org/springframework/security/config/LdapProviderBeanDefinitionParser.java

@@ -100,8 +100,9 @@ public class LdapProviderBeanDefinitionParser implements BeanDefinitionParser {
         ldapProvider.addConstructorArg(LdapUserServiceBeanDefinitionParser.parseAuthoritiesPopulator(elt, parserContext));
         ldapProvider.addPropertyValue("userDetailsContextMapper", 
         		LdapUserServiceBeanDefinitionParser.parseUserDetailsClass(elt, parserContext));
+        parserContext.getRegistry().registerBeanDefinition(BeanIds.LDAP_AUTHENTICATION_PROVIDER, ldapProvider.getBeanDefinition());
         
-        ConfigUtils.getRegisteredProviders(parserContext).add(ldapProvider.getBeanDefinition());
+        ConfigUtils.addAuthenticationProvider(parserContext, BeanIds.LDAP_AUTHENTICATION_PROVIDER);
 
         return null;
     }

core/src/main/java/org/springframework/security/config/NamespaceAuthenticationManager.java

@@ -0,0 +1,59 @@
+package org.springframework.security.config;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+
+import org.springframework.beans.factory.BeanFactory;
+import org.springframework.beans.factory.BeanFactoryAware;
+import org.springframework.security.providers.ProviderManager;
+import org.springframework.util.Assert;
+
+/**
+ * Extended version of {@link ProviderManager the default authentication manager} which lazily initializes
+ * the list of {@link AuthenticationProvider}s. This prevents some of the issues that have occurred with 
+ * namespace configuration where early instantiation of a security interceptor has caused the AuthenticationManager
+ * and thus dependent beans (typically UserDetailsService implementations or DAOs) to be initialized too early. 
+ * 
+ * @author Luke Taylor
+ * @since 2.0.4
+ */
+public class NamespaceAuthenticationManager extends ProviderManager implements BeanFactoryAware {
+	BeanFactory beanFactory;
+	List providerBeanNames;
+	
+	public void setBeanFactory(BeanFactory beanFactory) {
+		this.beanFactory = beanFactory;
+	}
+
+	public void afterPropertiesSet() throws Exception {
+		Assert.notNull(providerBeanNames, "provideBeanNames has not been set");
+		Assert.notEmpty(providerBeanNames, "No authentication providers were found in the application context");
+		
+		super.afterPropertiesSet();
+	}
+
+	/**
+	 * Overridden to lazily-initialize the list of providers on first use.
+	 */
+	public List getProviders() {
+		// We use the names array to determine whether the list has been set yet.
+		if (providerBeanNames != null) {
+			List providers = new ArrayList();
+			Iterator beanNames = providerBeanNames.iterator();
+			
+			while (beanNames.hasNext()) {
+				providers.add(beanFactory.getBean((String) beanNames.next()));
+			}
+			providerBeanNames = null;
+			
+			setProviders(providers);
+		}
+		
+		return super.getProviders();
+	}
+
+	public void setProviderBeanNames(List provideBeanNames) {
+		this.providerBeanNames = provideBeanNames;
+	}
+}

core/src/main/java/org/springframework/security/config/RememberMeBeanDefinitionParser.java

@@ -122,12 +122,12 @@ public class RememberMeBeanDefinitionParser implements BeanDefinitionParser {
 	}
 
 	private void registerProvider(ParserContext pc, Object source, String key) {
-        BeanDefinition authManager = ConfigUtils.registerProviderManagerIfNecessary(pc);
+        //BeanDefinition authManager = ConfigUtils.registerProviderManagerIfNecessary(pc);
         RootBeanDefinition provider = new RootBeanDefinition(RememberMeAuthenticationProvider.class);
         provider.setSource(source);
         provider.getPropertyValues().addPropertyValue(ATT_KEY, key);
-        ManagedList providers = (ManagedList) authManager.getPropertyValues().getPropertyValue("providers").getValue();
-        providers.add(provider);    	
+        pc.getRegistry().registerBeanDefinition(BeanIds.REMEMBER_ME_AUTHENTICATION_PROVIDER, provider);
+        ConfigUtils.addAuthenticationProvider(pc, BeanIds.REMEMBER_ME_AUTHENTICATION_PROVIDER);
     }
     
     private void registerFilter(ParserContext pc, Object source) {

core/src/main/java/org/springframework/security/config/X509BeanDefinitionParser.java

@@ -46,7 +46,7 @@ public class X509BeanDefinitionParser implements BeanDefinitionParser {
 
         BeanDefinition provider = new RootBeanDefinition(PreAuthenticatedAuthenticationProvider.class);
         parserContext.getRegistry().registerBeanDefinition(BeanIds.X509_AUTH_PROVIDER, provider);
-        ConfigUtils.getRegisteredProviders(parserContext).add(new RuntimeBeanReference(BeanIds.X509_AUTH_PROVIDER));
+        ConfigUtils.addAuthenticationProvider(parserContext, BeanIds.X509_AUTH_PROVIDER);
 
         String userServiceRef = element.getAttribute(ATT_USER_SERVICE_REF);
 

core/src/main/java/org/springframework/security/intercept/method/aopalliance/MethodDefinitionSourceAdvisor.java

@@ -77,7 +77,7 @@ public class MethodDefinitionSourceAdvisor extends AbstractPointcutAdvisor imple
      * <p>
      * 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. 
+     * compatibility. 
      * 
      * @param adviceBeanName name of the MethodSecurityInterceptor bean
      * @param attributeSource the attribute source (should be the same as the one used on the interceptor)
@@ -103,7 +103,7 @@ public class MethodDefinitionSourceAdvisor extends AbstractPointcutAdvisor imple
 				Assert.state(beanFactory != null, "BeanFactory must be set to resolve 'adviceBeanName'");
 				interceptor = (MethodSecurityInterceptor) 
 						beanFactory.getBean(this.adviceBeanName, MethodSecurityInterceptor.class);
-				attributeSource = interceptor.getObjectDefinitionSource();
+//				attributeSource = interceptor.getObjectDefinitionSource();
 			}
 			return interceptor;
 		}

core/src/main/java/org/springframework/security/providers/ProviderManager.java

@@ -144,17 +144,10 @@ public class ProviderManager extends AbstractAuthenticationManager implements In
     //~ Methods ========================================================================================================
 
     public void afterPropertiesSet() throws Exception {
-        checkIfValidList(this.providers);
         Assert.notNull(this.messages, "A message source must be set");
         exceptionMappings.putAll(additionalExceptionMappings);
     }
 
-    private void checkIfValidList(List listToCheck) {
-        if ((listToCheck == null) || (listToCheck.size() == 0)) {
-            throw new IllegalArgumentException("A list of AuthenticationProviders is required");
-        }
-    }
-
     /**
      * Attempts to authenticate the passed {@link Authentication} object.
      * <p>
@@ -174,7 +167,7 @@ public class ProviderManager extends AbstractAuthenticationManager implements In
      * @throws AuthenticationException if authentication fails.
      */
     public Authentication doAuthentication(Authentication authentication) throws AuthenticationException {
-        Iterator iter = providers.iterator();
+        Iterator iter = getProviders().iterator();
 
         Class toTest = authentication.getClass();
 
@@ -273,7 +266,11 @@ public class ProviderManager extends AbstractAuthenticationManager implements In
     }
 
     public List getProviders() {
-        return this.providers;
+        if (providers == null || providers.size() == 0) {
+            throw new IllegalArgumentException("A list of AuthenticationProviders is required");
+        }
+
+        return providers;
     }
 
     /**
@@ -303,8 +300,7 @@ public class ProviderManager extends AbstractAuthenticationManager implements In
      * AuthenticationProvider instance.
      */
     public void setProviders(List providers) {
-        checkIfValidList(providers);
-
+    	Assert.notEmpty(providers, "A list of AuthenticationProviders is required");
         Iterator iter = providers.iterator();
 
         while (iter.hasNext()) {

core/src/main/java/org/springframework/security/ui/preauth/AbstractPreAuthenticatedProcessingFilter.java

@@ -38,6 +38,8 @@ public abstract class AbstractPreAuthenticatedProcessingFilter extends SpringSec
     private AuthenticationDetailsSource authenticationDetailsSource = new WebAuthenticationDetailsSource();
 
     private AuthenticationManager authenticationManager = null;
+    
+    private boolean continueFilterChainOnUnsuccessfulAuthentication = true;
 
     /**
      * Check whether all required properties have been set.
@@ -88,6 +90,10 @@ public abstract class AbstractPreAuthenticatedProcessingFilter extends SpringSec
             successfulAuthentication(request, response, authResult);
         } catch (AuthenticationException failed) {
             unsuccessfulAuthentication(request, response, failed);
+            
+            if (!continueFilterChainOnUnsuccessfulAuthentication) {
+            	throw failed;
+            }
         }
     }
 
@@ -143,8 +149,19 @@ public abstract class AbstractPreAuthenticatedProcessingFilter extends SpringSec
     public void setAuthenticationManager(AuthenticationManager authenticationManager) {
         this.authenticationManager = authenticationManager;
     }
+    
+    public void setContinueFilterChainOnUnsuccessfulAuthentication(boolean shouldContinue) {
+    	continueFilterChainOnUnsuccessfulAuthentication = shouldContinue;
+    }
 
+    /**
+     * Override to extract the principal information from the current request 
+     */
     protected abstract Object getPreAuthenticatedPrincipal(HttpServletRequest request);
 
+    /**
+     * Override to extract the credentials (if applicable) from the current request. Some implementations
+     * may return a dummy value.
+     */    
     protected abstract Object getPreAuthenticatedCredentials(HttpServletRequest request);
 }

core/src/test/java/org/springframework/security/providers/ProviderManagerTests.java

@@ -119,10 +119,11 @@ public class ProviderManagerTests {
     }
 
     @Test(expected=IllegalArgumentException.class)
-    public void startupFailsIfProviderListNotSet() throws Exception {
+    public void getProvidersFailsIfProviderListNotSet() throws Exception {
         ProviderManager mgr = new ProviderManager();
 
         mgr.afterPropertiesSet();
+        mgr.getProviders();
     }
 
     @Test(expected=IllegalArgumentException.class)