From 5d51b35cfa58107daf688085f58b7f9ad147bee7 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Wed, 23 Apr 2008 23:19:44 +0000 Subject: [PATCH] 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. --- .../config/FilterChainProxyPostProcessor.java | 40 +++++++++++++++++++ ...HttpSecurityBeanDefinitionParserTests.java | 20 +++++----- 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/springframework/security/config/FilterChainProxyPostProcessor.java b/core/src/main/java/org/springframework/security/config/FilterChainProxyPostProcessor.java index 822584a9cf..998a1d5275 100644 --- a/core/src/main/java/org/springframework/security/config/FilterChainProxyPostProcessor.java +++ b/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 and not avoiding the use of ."); + } + } + } + + 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; diff --git a/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java b/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java index c30192b83b..8199ba88ed 100644 --- a/core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java +++ b/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( "" + AUTH_PROVIDER_XML + - "" + - " " + + "" + + " " + "" + + "" + + " " + + "" + "" + " " + "" + @@ -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