SEC-756: Refactored GlobalMethodSecurityDefinitionParser and added check for duplicate registration.

core-tiger/src/test/java/org/springframework/security/config/GlobalMethodSecurityBeanDefinitionParserTests.java

@@ -5,6 +5,7 @@ import static org.junit.Assert.assertEquals;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.springframework.beans.factory.parsing.BeanDefinitionParsingException;
 import org.springframework.context.support.AbstractXmlApplicationContext;
 import org.springframework.context.support.ClassPathXmlApplicationContext;
 import org.springframework.security.AccessDeniedException;
@@ -79,6 +80,14 @@ public class GlobalMethodSecurityBeanDefinitionParserTests {
         assertEquals("Hello from the post processor!", service.getPostProcessorWasHere());
     }
     
+    @Test(expected=BeanDefinitionParsingException.class)
+    public void duplicateElementCausesError() {
+        setContext(
+                "<global-method-security />" +
+                "<global-method-security />"
+        );
+    }
+    
     private void setContext(String context) {
         appContext = new InMemoryXmlApplicationContext(context);
     }

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

@@ -10,6 +10,7 @@ import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.RuntimeBeanReference;
 import org.springframework.beans.factory.parsing.BeanComponentDefinition;
 import org.springframework.beans.factory.support.BeanDefinitionBuilder;
+import org.springframework.beans.factory.support.BeanDefinitionRegistry;
 import org.springframework.beans.factory.support.ManagedList;
 import org.springframework.beans.factory.support.RootBeanDefinition;
 import org.springframework.beans.factory.xml.BeanDefinitionParser;
@@ -50,102 +51,135 @@ class GlobalMethodSecurityBeanDefinitionParser implements BeanDefinitionParser {
     }
     
     public BeanDefinition parse(Element element, ParserContext parserContext) {
-    	boolean useJsr250 = "enabled".equals(element.getAttribute(ATT_USE_JSR250));
-        boolean useSecured = "enabled".equals(element.getAttribute(ATT_USE_SECURED));
-
-        // Check the required classes are present
-        if (useSecured) {
-        	validatePresent(SECURED_METHOD_DEFINITION_SOURCE_CLASS, element, parserContext);
-        	validatePresent(SECURED_DEPENDENCY_CLASS, element, parserContext);
-        }
-
-        if (useJsr250) {
-        	validatePresent(JSR_250_SECURITY_METHOD_DEFINITION_SOURCE_CLASS, element, parserContext);
-        	validatePresent(JSR_250_VOTER_CLASS, element, parserContext);
-        }
+        Object source = parserContext.extractSource(element);
+        // The list of method metadata delegates
+        ManagedList delegates = new ManagedList();
+        
+        boolean jsr250Enabled = registerAnnotationBasedMethodDefinitionSources(element, parserContext, delegates);
         
-        // Now create a Map<String, ConfigAttribute> for each <protect-pointcut> sub-element
-        Map pointcutMap = new LinkedHashMap();
-        List protect = DomUtils.getChildElementsByTagName(element, Elements.PROTECT_POINTCUT);
-
-        for (Iterator i = protect.iterator(); i.hasNext();) {
-            Element childElt = (Element) i.next();
-            String accessConfig = childElt.getAttribute(ATT_ACCESS);
-            String expression = childElt.getAttribute(ATT_EXPRESSION);
-            Assert.hasText(accessConfig, "Access configuration required for '" + childElt + "'");
-            Assert.hasText(expression, "Expression required for '" + childElt + "'");
-            
-            ConfigAttributeDefinition def = new ConfigAttributeDefinition(StringUtils.commaDelimitedListToStringArray(accessConfig));
-            pointcutMap.put(expression, def);
-        }
-
         MapBasedMethodDefinitionSource mapBasedMethodDefinitionSource = new MapBasedMethodDefinitionSource();
+        delegates.add(mapBasedMethodDefinitionSource);
+        
+        // Now create a Map<String, ConfigAttribute> for each <protect-pointcut> sub-element        
+        Map pointcutMap = parseProtectPointcuts(parserContext, 
+                DomUtils.getChildElementsByTagName(element, Elements.PROTECT_POINTCUT));
         
-        // Now create and populate our ProtectPointcutBeanPostProcessor, if needed
         if (pointcutMap.size() > 0) {
-            RootBeanDefinition ppbp = new RootBeanDefinition(ProtectPointcutPostProcessor.class);
-            ppbp.setRole(BeanDefinition.ROLE_INFRASTRUCTURE);
-            ppbp.setSource(parserContext.extractSource(element));
-            ppbp.getConstructorArgumentValues().addGenericArgumentValue(mapBasedMethodDefinitionSource);
-            ppbp.getPropertyValues().addPropertyValue("pointcutMap", pointcutMap);
-            parserContext.getRegistry().registerBeanDefinition(BeanIds.PROTECT_POINTCUT_POST_PROCESSOR, ppbp);
+            registerProtectPointcutPostProcessor(parserContext, pointcutMap, mapBasedMethodDefinitionSource, source);
         }
         
-        // Create our list of method metadata delegates
-        ManagedList delegates = new ManagedList();
-        delegates.add(mapBasedMethodDefinitionSource);
+        registerDelegatingMethodDefinitionSource(parserContext, delegates, source);
+        
+        // Register the applicable AccessDecisionManager, handling the special JSR 250 voter if being used
+        String accessManagerId = element.getAttribute(ATT_ACCESS_MGR);
+
+        if (!StringUtils.hasText(accessManagerId)) {
+            ConfigUtils.registerDefaultAccessManagerIfNecessary(parserContext);
+
+            if (jsr250Enabled) {
+                ConfigUtils.addVoter(new RootBeanDefinition(JSR_250_VOTER_CLASS, null, null), parserContext);                
+            }
+
+            accessManagerId = BeanIds.ACCESS_MANAGER;
+        }
+        
+        registerMethodSecurityInterceptor(parserContext, accessManagerId, source);
+        
+        registerAdvisor(parserContext, source);
+
+        AopNamespaceUtils.registerAutoProxyCreatorIfNecessary(parserContext, element);
+        
+        return null;
+    }
+    
+    /**
+     * Checks whether JSR-250 and/or Secured annotations are enabled and adds the appropriate 
+     * MethodDefinitionSource delegates if required. 
+     */
+    private boolean registerAnnotationBasedMethodDefinitionSources(Element element, ParserContext pc, ManagedList delegates) {
+        boolean useJsr250 = "enabled".equals(element.getAttribute(ATT_USE_JSR250));
+        boolean useSecured = "enabled".equals(element.getAttribute(ATT_USE_SECURED));
         
+        // Check the required classes are present
         if (useSecured) {
+            validatePresent(SECURED_METHOD_DEFINITION_SOURCE_CLASS, element, pc);
+            validatePresent(SECURED_DEPENDENCY_CLASS, element, pc);
             delegates.add(BeanDefinitionBuilder.rootBeanDefinition(SECURED_METHOD_DEFINITION_SOURCE_CLASS).getBeanDefinition());
         }
-        
+
         if (useJsr250) {
-            delegates.add(BeanDefinitionBuilder.rootBeanDefinition(JSR_250_SECURITY_METHOD_DEFINITION_SOURCE_CLASS).getBeanDefinition());            
+            validatePresent(JSR_250_SECURITY_METHOD_DEFINITION_SOURCE_CLASS, element, pc);
+            validatePresent(JSR_250_VOTER_CLASS, element, pc);
+            delegates.add(BeanDefinitionBuilder.rootBeanDefinition(JSR_250_SECURITY_METHOD_DEFINITION_SOURCE_CLASS).getBeanDefinition());           
         }
         
-    	// Register our DelegatingMethodDefinitionSource
+        return useJsr250;
+    }
+    
+    private void registerDelegatingMethodDefinitionSource(ParserContext parserContext, ManagedList delegates, Object source) {
+        if (parserContext.getRegistry().containsBeanDefinition(BeanIds.DELEGATING_METHOD_DEFINITION_SOURCE)) {
+            parserContext.getReaderContext().error("Duplicate <global-method-security> detected.", source);
+        }
         RootBeanDefinition delegatingMethodDefinitionSource = new RootBeanDefinition(DelegatingMethodDefinitionSource.class);
         delegatingMethodDefinitionSource.setRole(BeanDefinition.ROLE_INFRASTRUCTURE);
-        delegatingMethodDefinitionSource.setSource(parserContext.extractSource(element));
+        delegatingMethodDefinitionSource.setSource(source);
         delegatingMethodDefinitionSource.getPropertyValues().addPropertyValue("methodDefinitionSources", delegates);
-        parserContext.getRegistry().registerBeanDefinition(BeanIds.DELEGATING_METHOD_DEFINITION_SOURCE, delegatingMethodDefinitionSource);
-        
-        // Register the applicable AccessDecisionManager, handling the special JSR 250 voter if being used
-        String accessManagerId = element.getAttribute(ATT_ACCESS_MGR);
+        parserContext.getRegistry().registerBeanDefinition(BeanIds.DELEGATING_METHOD_DEFINITION_SOURCE, delegatingMethodDefinitionSource);        
+    }
+    
+    private void registerProtectPointcutPostProcessor(ParserContext parserContext, Map pointcutMap,
+            MapBasedMethodDefinitionSource mapBasedMethodDefinitionSource, Object source) {
+        RootBeanDefinition ppbp = new RootBeanDefinition(ProtectPointcutPostProcessor.class);
+        ppbp.setRole(BeanDefinition.ROLE_INFRASTRUCTURE);
+        ppbp.setSource(source);
+        ppbp.getConstructorArgumentValues().addGenericArgumentValue(mapBasedMethodDefinitionSource);
+        ppbp.getPropertyValues().addPropertyValue("pointcutMap", pointcutMap);
+        parserContext.getRegistry().registerBeanDefinition(BeanIds.PROTECT_POINTCUT_POST_PROCESSOR, ppbp);
+    }
 
-        if (!StringUtils.hasText(accessManagerId)) {
-            ConfigUtils.registerDefaultAccessManagerIfNecessary(parserContext);
+    private Map parseProtectPointcuts(ParserContext parserContext, List protectPointcutElts) {
+        Map pointcutMap = new LinkedHashMap();
 
-            if (useJsr250) {
-                ConfigUtils.addVoter(new RootBeanDefinition(JSR_250_VOTER_CLASS, null, null), parserContext);                
+        for (Iterator i = protectPointcutElts.iterator(); i.hasNext();) {
+            Element childElt = (Element) i.next();
+            String accessConfig = childElt.getAttribute(ATT_ACCESS);
+            String expression = childElt.getAttribute(ATT_EXPRESSION);
+            
+            if (!StringUtils.hasText(accessConfig)) {
+                parserContext.getReaderContext().error("Access configuration required", parserContext.extractSource(childElt));
             }
-
-            accessManagerId = BeanIds.ACCESS_MANAGER;
+            
+            if (!StringUtils.hasText(expression)) {
+                parserContext.getReaderContext().error("Pointcut expression required", parserContext.extractSource(childElt));
+            }
+            
+            ConfigAttributeDefinition def = new ConfigAttributeDefinition(StringUtils.commaDelimitedListToStringArray(accessConfig));
+            pointcutMap.put(expression, def);
         }
 
-        // MethodSecurityInterceptor
+        return pointcutMap;
+    }
+    
+    private void registerMethodSecurityInterceptor(ParserContext parserContext, String accessManagerId, Object source) {
         RootBeanDefinition interceptor = new RootBeanDefinition(MethodSecurityInterceptor.class);
         interceptor.setRole(BeanDefinition.ROLE_INFRASTRUCTURE);
-        interceptor.setSource(parserContext.extractSource(element));
+        interceptor.setSource(source);
         
         interceptor.getPropertyValues().addPropertyValue("accessDecisionManager", new RuntimeBeanReference(accessManagerId));
         interceptor.getPropertyValues().addPropertyValue("authenticationManager", new RuntimeBeanReference(BeanIds.AUTHENTICATION_MANAGER));
         interceptor.getPropertyValues().addPropertyValue("objectDefinitionSource", new RuntimeBeanReference(BeanIds.DELEGATING_METHOD_DEFINITION_SOURCE));
         parserContext.getRegistry().registerBeanDefinition(BeanIds.METHOD_SECURITY_INTERCEPTOR, interceptor);
-        parserContext.registerComponent(new BeanComponentDefinition(interceptor, BeanIds.METHOD_SECURITY_INTERCEPTOR));
-        
-        // MethodDefinitionSourceAdvisor
+        parserContext.registerComponent(new BeanComponentDefinition(interceptor, BeanIds.METHOD_SECURITY_INTERCEPTOR));        
+    }
+    
+    private void registerAdvisor(ParserContext parserContext, Object source) {
         RootBeanDefinition advisor = new RootBeanDefinition(MethodDefinitionSourceAdvisor.class);
         advisor.setRole(BeanDefinition.ROLE_INFRASTRUCTURE);
-        advisor.setSource(parserContext.extractSource(element));
+        advisor.setSource(source);
         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);
-        
-        return null;
+        parserContext.getRegistry().registerBeanDefinition(BeanIds.METHOD_DEFINITION_SOURCE_ADVISOR, advisor);        
     }
+    
 }