From f92589f05109513a67574addd28db606c0db619a Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Wed, 6 Jul 2011 00:12:48 +0100 Subject: [PATCH] Extract a SecurityFilterChain interface and create a default implementation to facilitate other configuration options. --- .../http/DefaultFilterChainValidator.java | 33 +++++++++++--- .../http/FilterChainBeanDefinitionParser.java | 3 +- .../HttpSecurityBeanDefinitionParser.java | 3 +- .../http/MultiHttpBlockConfigTests.groovy | 2 +- .../config/FilterChainProxyConfigTests.java | 16 +++---- .../security/util/filtertest-valid.xml | 11 +++-- .../web/DefaultSecurityFilterChain.java | 45 +++++++++++++++++++ .../security/web/FilterChainProxy.java | 20 +-------- .../security/web/SecurityFilterChain.java | 41 +++-------------- .../security/web/FilterChainProxyTests.java | 6 +-- 10 files changed, 101 insertions(+), 79 deletions(-) create mode 100644 web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java diff --git a/config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java b/config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java index f1118d1772..711c90ae50 100644 --- a/config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java +++ b/config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java @@ -9,6 +9,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.ConfigAttribute; import org.springframework.security.authentication.AnonymousAuthenticationToken; +import org.springframework.security.web.DefaultSecurityFilterChain; import org.springframework.security.web.FilterChainProxy; import org.springframework.security.web.FilterInvocation; import org.springframework.security.web.SecurityFilterChain; @@ -24,6 +25,7 @@ import org.springframework.security.web.context.SecurityContextPersistenceFilter import org.springframework.security.web.jaasapi.JaasApiIntegrationFilter; import org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter; import org.springframework.security.web.session.SessionManagementFilter; +import org.springframework.security.web.util.AnyRequestMatcher; public class DefaultFilterChainValidator implements FilterChainProxy.FilterChainValidator { private final Log logger = LogFactory.getLog(getClass()); @@ -34,17 +36,34 @@ public class DefaultFilterChainValidator implements FilterChainProxy.FilterChain checkFilterStack(filterChain.getFilters()); } + checkPathOrder(new ArrayList(fcp.getFilterChains())); checkForDuplicateMatchers(new ArrayList(fcp.getFilterChains())); } - private void checkForDuplicateMatchers(List chains) { - SecurityFilterChain chain = chains.remove(0); + private void checkPathOrder(List filterChains) { + // Check that the universal pattern is listed at the end, if at all + Iterator chains = filterChains.iterator(); - for (SecurityFilterChain test : chains) { - if (chain.getRequestMatcher().equals(test.getRequestMatcher())) { - throw new IllegalArgumentException("The FilterChainProxy contains two filter chains using the" + - " matcher " + chain.getRequestMatcher() + ". If you are using multiple namespace " + - "elements, you must use a 'pattern' attribute to define the request patterns to which they apply."); + while (chains.hasNext()) { + if (((DefaultSecurityFilterChain)chains.next()).getRequestMatcher() instanceof AnyRequestMatcher && chains.hasNext()) { + throw new IllegalArgumentException("A universal match pattern ('/**') is defined " + + " before other patterns in the filter chain, causing them to be ignored. Please check the " + + "ordering in your namespace or FilterChainProxy bean configuration"); + } + } + } + + private void checkForDuplicateMatchers(List chains) { + + while (chains.size() > 1) { + DefaultSecurityFilterChain chain = (DefaultSecurityFilterChain)chains.remove(0); + + for (SecurityFilterChain test : chains) { + if (chain.getRequestMatcher().equals(((DefaultSecurityFilterChain)test).getRequestMatcher())) { + throw new IllegalArgumentException("The FilterChainProxy contains two filter chains using the" + + " matcher " + chain.getRequestMatcher() + ". If you are using multiple namespace " + + "elements, you must use a 'pattern' attribute to define the request patterns to which they apply."); + } } } } diff --git a/config/src/main/java/org/springframework/security/config/http/FilterChainBeanDefinitionParser.java b/config/src/main/java/org/springframework/security/config/http/FilterChainBeanDefinitionParser.java index e4b833a43e..d05cee3aa4 100644 --- a/config/src/main/java/org/springframework/security/config/http/FilterChainBeanDefinitionParser.java +++ b/config/src/main/java/org/springframework/security/config/http/FilterChainBeanDefinitionParser.java @@ -7,6 +7,7 @@ import org.springframework.beans.factory.support.ManagedList; import org.springframework.beans.factory.xml.AbstractSingleBeanDefinitionParser; import org.springframework.beans.factory.xml.BeanDefinitionParser; import org.springframework.beans.factory.xml.ParserContext; +import org.springframework.security.web.DefaultSecurityFilterChain; import org.springframework.security.web.SecurityFilterChain; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -26,7 +27,7 @@ public class FilterChainBeanDefinitionParser implements BeanDefinitionParser { String requestMatcher = elt.getAttribute(ATT_REQUEST_MATCHER_REF); String filters = elt.getAttribute(HttpSecurityBeanDefinitionParser.ATT_FILTERS); - BeanDefinitionBuilder builder = BeanDefinitionBuilder.rootBeanDefinition(SecurityFilterChain.class); + BeanDefinitionBuilder builder = BeanDefinitionBuilder.rootBeanDefinition(DefaultSecurityFilterChain.class); if (StringUtils.hasText(path)) { Assert.isTrue(!StringUtils.hasText(requestMatcher), ""); diff --git a/config/src/main/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParser.java b/config/src/main/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParser.java index dd3dc6f27b..a7e1e94aed 100644 --- a/config/src/main/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParser.java +++ b/config/src/main/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParser.java @@ -21,6 +21,7 @@ import org.springframework.security.authentication.ProviderManager; import org.springframework.security.config.BeanIds; import org.springframework.security.config.Elements; import org.springframework.security.config.authentication.AuthenticationManagerFactoryBean; +import org.springframework.security.web.DefaultSecurityFilterChain; import org.springframework.security.web.FilterChainProxy; import org.springframework.security.web.SecurityFilterChain; import org.springframework.security.web.util.AnyRequestMatcher; @@ -147,7 +148,7 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { filterChainMatcher = new RootBeanDefinition(AnyRequestMatcher.class); } - BeanDefinitionBuilder filterChainBldr = BeanDefinitionBuilder.rootBeanDefinition(SecurityFilterChain.class); + BeanDefinitionBuilder filterChainBldr = BeanDefinitionBuilder.rootBeanDefinition(DefaultSecurityFilterChain.class); filterChainBldr.addConstructorArgValue(filterChainMatcher); filterChainBldr.addConstructorArgValue(filterChain); diff --git a/config/src/test/groovy/org/springframework/security/config/http/MultiHttpBlockConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/MultiHttpBlockConfigTests.groovy index ca1fed2c78..51622d4c02 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/MultiHttpBlockConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/MultiHttpBlockConfigTests.groovy @@ -42,7 +42,7 @@ class MultiHttpBlockConfigTests extends AbstractHttpConfigTests { createAppContext() then: BeanCreationException e = thrown() - e.cause.cause instanceof IllegalArgumentException + e.cause instanceof IllegalArgumentException } def duplicatePatternsAreRejected () { diff --git a/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java b/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java index 43f77af321..cc31dc0c8a 100644 --- a/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java +++ b/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java @@ -33,6 +33,7 @@ import org.springframework.beans.factory.BeanCreationException; import org.springframework.context.support.ClassPathXmlApplicationContext; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.web.DefaultSecurityFilterChain; import org.springframework.security.web.FilterChainProxy; import org.springframework.security.web.SecurityFilterChain; import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter; @@ -67,11 +68,6 @@ public class FilterChainProxyConfigTests { } } - @Test(expected=BeanCreationException.class) - public void misplacedUniversalPathShouldBeDetected() throws Exception { - appCtx.getBean("newFilterChainProxyWrongPathOrder", FilterChainProxy.class); - } - @Test public void normalOperation() throws Exception { FilterChainProxy filterChainProxy = appCtx.getBean("filterChain", FilterChainProxy.class); @@ -111,9 +107,13 @@ public class FilterChainProxyConfigTests { FilterChainProxy fcp = appCtx.getBean("sec1235FilterChainProxy", FilterChainProxy.class); List chains = fcp.getFilterChains(); - assertEquals("/login*", ((AntPathRequestMatcher)chains.get(0).getRequestMatcher()).getPattern()); - assertEquals("/logout", ((AntPathRequestMatcher)chains.get(1).getRequestMatcher()).getPattern()); - assertTrue(chains.get(2).getRequestMatcher() instanceof AnyRequestMatcher); + assertEquals("/login*", getPattern(chains.get(0))); + assertEquals("/logout", getPattern(chains.get(1))); + assertTrue(((DefaultSecurityFilterChain)chains.get(2)).getRequestMatcher() instanceof AnyRequestMatcher); + } + + private String getPattern(SecurityFilterChain chain) { + return ((AntPathRequestMatcher)((DefaultSecurityFilterChain)chain).getRequestMatcher()).getPattern(); } private void checkPathAndFilterOrder(FilterChainProxy filterChainProxy) throws Exception { diff --git a/config/src/test/resources/org/springframework/security/util/filtertest-valid.xml b/config/src/test/resources/org/springframework/security/util/filtertest-valid.xml index e4c9dd5fbc..a2cda067c6 100644 --- a/config/src/test/resources/org/springframework/security/util/filtertest-valid.xml +++ b/config/src/test/resources/org/springframework/security/util/filtertest-valid.xml @@ -98,8 +98,11 @@ + + + @@ -124,7 +127,7 @@ - + @@ -132,7 +135,7 @@ - + @@ -146,7 +149,7 @@ - + @@ -156,7 +159,7 @@ - + diff --git a/web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java b/web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java new file mode 100644 index 0000000000..cc4e418347 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java @@ -0,0 +1,45 @@ +package org.springframework.security.web; + +import org.springframework.security.web.util.RequestMatcher; + +import javax.servlet.Filter; +import javax.servlet.http.HttpServletRequest; +import java.util.*; + +/** + * Standard implementation of {@code SecurityFilterChain}. + * + * @author Luke Taylor + * + * @since 3.1 + */ +public final class DefaultSecurityFilterChain implements SecurityFilterChain { + private final RequestMatcher requestMatcher; + private final List filters; + + public DefaultSecurityFilterChain(RequestMatcher requestMatcher, Filter... filters) { + this(requestMatcher, Arrays.asList(filters)); + } + + public DefaultSecurityFilterChain(RequestMatcher requestMatcher, List filters) { + this.requestMatcher = requestMatcher; + this.filters = new ArrayList(filters); + } + + public RequestMatcher getRequestMatcher() { + return requestMatcher; + } + + public List getFilters() { + return filters; + } + + public boolean matches(HttpServletRequest request) { + return requestMatcher.matches(request); + } + + @Override + public String toString() { + return "[ " + requestMatcher + ", " + filters + "]"; + } +} diff --git a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java index 1d07c49460..19a3d44da5 100644 --- a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java +++ b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java @@ -142,7 +142,6 @@ public class FilterChainProxy extends GenericFilterBean { public FilterChainProxy(List filterChains) { this.filterChains = filterChains; - checkPathOrder(); } @Override @@ -219,10 +218,8 @@ public class FilterChainProxy extends GenericFilterBean { filterChains = new ArrayList(filterChainMap.size()); for (Map.Entry> entry : filterChainMap.entrySet()) { - filterChains.add(new SecurityFilterChain(entry.getKey(), entry.getValue())); + filterChains.add(new DefaultSecurityFilterChain(entry.getKey(), entry.getValue())); } - - checkPathOrder(); } /** @@ -238,25 +235,12 @@ public class FilterChainProxy extends GenericFilterBean { LinkedHashMap> map = new LinkedHashMap>(); for (SecurityFilterChain chain : filterChains) { - map.put(chain.getRequestMatcher(), chain.getFilters()); + map.put(((DefaultSecurityFilterChain)chain).getRequestMatcher(), chain.getFilters()); } return map; } - private void checkPathOrder() { - // Check that the universal pattern is listed at the end, if at all - Iterator chains = filterChains.iterator(); - - while(chains.hasNext()) { - if ((chains.next().getRequestMatcher() instanceof AnyRequestMatcher && chains.hasNext())) { - throw new IllegalArgumentException("A universal match pattern ('/**') is defined " + - " before other patterns in the filter chain, causing them to be ignored. Please check the " + - "ordering in your namespace or FilterChainProxy bean configuration"); - } - } - } - /** * @return the list of {@code SecurityFilterChain}s which will be matched against and * applied to incoming requests. diff --git a/web/src/main/java/org/springframework/security/web/SecurityFilterChain.java b/web/src/main/java/org/springframework/security/web/SecurityFilterChain.java index fced2e0814..9a850fd358 100644 --- a/web/src/main/java/org/springframework/security/web/SecurityFilterChain.java +++ b/web/src/main/java/org/springframework/security/web/SecurityFilterChain.java @@ -1,54 +1,23 @@ package org.springframework.security.web; -import org.springframework.security.web.util.RequestMatcher; - import javax.servlet.Filter; -import javax.servlet.FilterChain; -import javax.servlet.FilterConfig; -import javax.servlet.ServletException; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; -import java.io.IOException; import java.util.*; /** - * Bean which defines a filter chain which is capable of being matched against an {@code HttpServletRequest}. + * Defines a filter chain which is capable of being matched against an {@code HttpServletRequest}. * in order to decide whether it applies to that request. *

* Used to configure a {@code FilterChainProxy}. * + * * @author Luke Taylor * * @since 3.1 */ -public final class SecurityFilterChain { - private final RequestMatcher requestMatcher; - private final List filters; +public interface SecurityFilterChain { - public SecurityFilterChain(RequestMatcher requestMatcher, Filter... filters) { - this(requestMatcher, Arrays.asList(filters)); - } + boolean matches(HttpServletRequest request); - public SecurityFilterChain(RequestMatcher requestMatcher, List filters) { - this.requestMatcher = requestMatcher; - this.filters = new ArrayList(filters); - } - - public RequestMatcher getRequestMatcher() { - return requestMatcher; - } - - public List getFilters() { - return filters; - } - - public boolean matches(HttpServletRequest request) { - return requestMatcher.matches(request); - } - - @Override - public String toString() { - return "[ " + requestMatcher + ", " + filters + "]"; - } + List getFilters(); } diff --git a/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java index 582368a679..b564f6d51f 100644 --- a/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java +++ b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java @@ -47,7 +47,7 @@ public class FilterChainProxyTests { return null; } }).when(filter).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class), any(FilterChain.class)); - fcp = new FilterChainProxy(new SecurityFilterChain(matcher, Arrays.asList(filter))); + fcp = new FilterChainProxy(new DefaultSecurityFilterChain(matcher, Arrays.asList(filter))); fcp.setFilterChainValidator(mock(FilterChainProxy.FilterChainValidator.class)); request = new MockHttpServletRequest(); request.setServletPath("/path"); @@ -94,7 +94,7 @@ public class FilterChainProxyTests { @Test public void originalFilterChainIsInvokedIfMatchingSecurityChainIsEmpty() throws Exception { List noFilters = Collections.emptyList(); - fcp = new FilterChainProxy(new SecurityFilterChain(matcher, noFilters)); + fcp = new FilterChainProxy(new DefaultSecurityFilterChain(matcher, noFilters)); when(matcher.matches(any(HttpServletRequest.class))).thenReturn(true); fcp.doFilter(request, response, chain); @@ -137,7 +137,7 @@ public class FilterChainProxyTests { @Test public void bothWrappersAreResetWithNestedFcps() throws Exception { HttpFirewall fw = mock(HttpFirewall.class); - FilterChainProxy firstFcp = new FilterChainProxy(new SecurityFilterChain(matcher, fcp)); + FilterChainProxy firstFcp = new FilterChainProxy(new DefaultSecurityFilterChain(matcher, fcp)); firstFcp.setFirewall(fw); fcp.setFirewall(fw); FirewalledRequest firstFwr = mock(FirewalledRequest.class, "firstFwr");