SEC-792: Filters should only be added to the default stack if they are labelled using custom-filter.

http://jira.springframework.org/browse/SEC-792. Updated FilterChainProxyPostProcessor to raise an exception if two filters have the same order, and also to unwrap wrapped filters once the sorting by order has been performed.

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

@@ -5,6 +5,8 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
+import javax.servlet.Filter;
+
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.beans.BeansException;
@@ -13,6 +15,7 @@ import org.springframework.beans.factory.BeanFactoryAware;
 import org.springframework.beans.factory.ListableBeanFactory;
 import org.springframework.beans.factory.config.BeanPostProcessor;
 import org.springframework.core.OrderComparator;
+import org.springframework.core.Ordered;
 import org.springframework.security.config.ConfigUtils.FilterChainList;
 import org.springframework.security.util.FilterChainProxy;
 
@@ -37,6 +40,32 @@ public class FilterChainProxyPostProcessor implements BeanPostProcessor, BeanFac
         
         List filters = new ArrayList(filterList.getFilters());
         Collections.sort(filters, new OrderComparator());
+        
+        logger.info("Checking sorted filter chain: " + filters);
+        
+        for(int i=0; i < filters.size(); i++) {
+            Ordered filter = (Ordered)filters.get(i);
+
+            if (i > 0) {
+                Ordered previous = (Ordered)filters.get(i-1);
+                if (filter.getOrder() == previous.getOrder()) {
+                    throw new SecurityConfigurationException("Filters '" + unwrapFilter(filter) + "' and '" + 
+                            unwrapFilter(previous) + "' have the same 'order' value. When using custom filters, " +
+                            		"please make sure the positions do not conflict with default filters. " +
+                            		"Alternatively you can disable the default filters by removing the corresponding " +
+                            		"child elements from <http> and not avoiding the use of <http auto-config='true'>.");
+                }
+            }
+        }
+
+        logger.info("Filter chain...");        
+        for(int i=0; i < filters.size(); i++) {
+        // Remove the ordered wrapper from the filter and put it back in the chain at the same position.
+            Filter filter = unwrapFilter(filters.get(i));
+            logger.info("[" + i + "] - " + filter);            
+            filters.set(i, filter);
+        }
+        
         // Note that this returns a copy
         Map filterMap = filterChainProxy.getFilterChainMap();
         filterMap.put(filterChainProxy.getMatcher().getUniversalMatchPattern(), filters);
@@ -46,6 +75,17 @@ public class FilterChainProxyPostProcessor implements BeanPostProcessor, BeanFac
 
         return bean;
     }
+    
+    /** 
+     * Returns the delegate filter of a wrapper, or the unchanged filter if it isn't wrapped. 
+     */
+    private Filter unwrapFilter(Object filter) {
+        if (filter instanceof OrderedFilterBeanDefinitionDecorator.OrderedFilterDecorator) {
+            return ((OrderedFilterBeanDefinitionDecorator.OrderedFilterDecorator)filter).getDelegate();
+        }
+        
+        return (Filter) filter;
+    }
 
     public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
         return bean;

core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java

@@ -42,6 +42,7 @@ import org.springframework.security.ui.webapp.DefaultLoginPageGeneratingFilter;
 import org.springframework.security.util.FieldUtils;
 import org.springframework.security.util.FilterChainProxy;
 import org.springframework.security.util.InMemoryXmlApplicationContext;
+import org.springframework.security.util.MockFilter;
 import org.springframework.security.util.PortMapperImpl;
 import org.springframework.security.wrapper.SecurityContextHolderAwareRequestFilter;
 import org.springframework.util.ReflectionUtils;
@@ -259,13 +260,15 @@ public class HttpSecurityBeanDefinitionParserTests {
 
     @Test
     public void externalFiltersAreTreatedCorrectly() throws Exception {
-        // Decorated user-filter should be added to stack. The other MockFilter and the un-decorated standard filter
-    	// should be ignored
+        // Decorated user-filters should be added to stack. The others should be ignored.
         setContext(
                 "<http auto-config='true'/>" + AUTH_PROVIDER_XML +
-                "<b:bean id='userFilter' class='org.springframework.security.util.MockFilter'>" +
-                "    <custom-filter after='SESSION_CONTEXT_INTEGRATION_FILTER'/>" +
+                "<b:bean id='userFilter' class='org.springframework.security.wrapper.SecurityContextHolderAwareRequestFilter'>" +
+                "    <custom-filter after='LOGOUT_FILTER'/>" +
                 "</b:bean>" +
+                "<b:bean id='userFilter1' class='org.springframework.security.wrapper.SecurityContextHolderAwareRequestFilter'>" +
+                "    <custom-filter before='SESSION_CONTEXT_INTEGRATION_FILTER'/>" +
+                "</b:bean>" +                
                 "<b:bean id='userFilter2' class='org.springframework.security.util.MockFilter'>" +
                 "    <custom-filter position='FIRST'/>" +
                 "</b:bean>" +                
@@ -274,11 +277,10 @@ public class HttpSecurityBeanDefinitionParserTests {
                 );
         List filters = getFilters("/someurl");
 
-        assertEquals(13, filters.size());
-        assertTrue(filters.get(0) instanceof OrderedFilterBeanDefinitionDecorator.OrderedFilterDecorator);        
-        assertTrue(filters.get(2) instanceof OrderedFilterBeanDefinitionDecorator.OrderedFilterDecorator);
-        assertEquals("userFilter", ((OrderedFilterBeanDefinitionDecorator.OrderedFilterDecorator)filters.get(2)).getBeanName());
-        assertEquals("userFilter2", ((OrderedFilterBeanDefinitionDecorator.OrderedFilterDecorator)filters.get(0)).getBeanName());        
+        assertEquals(14, filters.size());
+        assertTrue(filters.get(0) instanceof MockFilter);        
+        assertTrue(filters.get(1) instanceof SecurityContextHolderAwareRequestFilter);
+        assertTrue(filters.get(5) instanceof SecurityContextHolderAwareRequestFilter);
     }
 
     @Test