From 9bf8656d667d9b0e1db764494c6da6021612a159 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Sat, 22 Aug 2009 21:09:34 +0000 Subject: [PATCH] SEC-1201: PropertyPlaceholderConfigurer does not work for intercept-url attributes. Added use of ManagedMaps and BeanDefinitions to support placeholders in the pattern and access attributes. --- ...ityMetadataSourceBeanDefinitionParser.java | 73 +++++++++++++- .../HttpSecurityBeanDefinitionParser.java | 94 +++++++------------ ...tadataSourceBeanDefinitionParserTests.java | 20 +++- ...HttpSecurityBeanDefinitionParserTests.java | 67 ++++++++++++- .../security/access/SecurityConfig.java | 7 +- 5 files changed, 190 insertions(+), 71 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/http/FilterInvocationSecurityMetadataSourceBeanDefinitionParser.java b/config/src/main/java/org/springframework/security/config/http/FilterInvocationSecurityMetadataSourceBeanDefinitionParser.java index 6d14716ab3..b70b36ba7e 100644 --- a/config/src/main/java/org/springframework/security/config/http/FilterInvocationSecurityMetadataSourceBeanDefinitionParser.java +++ b/config/src/main/java/org/springframework/security/config/http/FilterInvocationSecurityMetadataSourceBeanDefinitionParser.java @@ -1,12 +1,15 @@ package org.springframework.security.config.http; -import java.util.LinkedHashMap; import java.util.List; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.support.BeanDefinitionBuilder; +import org.springframework.beans.factory.support.ManagedMap; import org.springframework.beans.factory.xml.AbstractSingleBeanDefinitionParser; import org.springframework.beans.factory.xml.ParserContext; -import org.springframework.security.access.ConfigAttribute; +import org.springframework.security.access.SecurityConfig; import org.springframework.security.web.access.intercept.FilterInvocationSecurityMetadataSource; import org.springframework.security.web.access.intercept.RequestKey; import org.springframework.security.web.util.AntUrlPathMatcher; @@ -23,6 +26,11 @@ import org.w3c.dom.Element; */ public class FilterInvocationSecurityMetadataSourceBeanDefinitionParser extends AbstractSingleBeanDefinitionParser { + private static final String ATT_HTTP_METHOD = "method"; + private static final String ATT_PATTERN = "pattern"; + private static final String ATT_ACCESS = "access"; + private static final Log logger = LogFactory.getLog(FilterInvocationSecurityMetadataSourceBeanDefinitionParser.class); + protected String getBeanClassName(Element element) { return "org.springframework.security.web.access.intercept.DefaultFilterInvocationSecurityMetadataSource"; } @@ -44,11 +52,66 @@ public class FilterInvocationSecurityMetadataSourceBeanDefinitionParser extends UrlMatcher matcher = HttpSecurityBeanDefinitionParser.createUrlMatcher(element); boolean convertPathsToLowerCase = (matcher instanceof AntUrlPathMatcher) && matcher.requiresLowerCaseUrl(); - LinkedHashMap> requestMap = - HttpSecurityBeanDefinitionParser.parseInterceptUrlsForFilterInvocationRequestMap(interceptUrls, - convertPathsToLowerCase, false, parserContext); + ManagedMap requestMap = parseInterceptUrlsForFilterInvocationRequestMap( + interceptUrls, convertPathsToLowerCase, false, parserContext); builder.addConstructorArgValue(matcher); builder.addConstructorArgValue(requestMap); } + + static ManagedMap parseInterceptUrlsForFilterInvocationRequestMap(List urlElts, + boolean useLowerCasePaths, boolean useExpressions, ParserContext parserContext) { + + ManagedMap filterInvocationDefinitionMap = new ManagedMap(); + + for (Element urlElt : urlElts) { + String access = urlElt.getAttribute(ATT_ACCESS); + if (!StringUtils.hasText(access)) { + continue; + } + + String path = urlElt.getAttribute(ATT_PATTERN); + + if(!StringUtils.hasText(path)) { + parserContext.getReaderContext().error("path attribute cannot be empty or null", urlElt); + } + + if (useLowerCasePaths) { + path = path.toLowerCase(); + } + + String method = urlElt.getAttribute(ATT_HTTP_METHOD); + if (!StringUtils.hasText(method)) { + method = null; + } + + // Use beans to + + BeanDefinitionBuilder keyBldr = BeanDefinitionBuilder.rootBeanDefinition(RequestKey.class); + keyBldr.addConstructorArgValue(path); + keyBldr.addConstructorArgValue(method); + + BeanDefinitionBuilder attributeBuilder = BeanDefinitionBuilder.rootBeanDefinition(SecurityConfig.class); + attributeBuilder.addConstructorArgValue(access); + + if (useExpressions) { + logger.info("Creating access control expression attribute '" + access + "' for " + path); + // The expression will be parsed later by the ExpressionFilterInvocationSecurityMetadataSource + attributeBuilder.setFactoryMethod("createList"); + + } else { + attributeBuilder.setFactoryMethod("createListFromCommaDelimitedString"); + } + + BeanDefinition key = keyBldr.getBeanDefinition(); + + if (filterInvocationDefinitionMap.containsKey(key)) { + logger.warn("Duplicate URL defined: " + path + ". The original attribute values will be overwritten"); + } + + filterInvocationDefinitionMap.put(key, attributeBuilder.getBeanDefinition()); + } + + return filterInvocationDefinitionMap; + } } 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 e6449ec511..135d606065 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 @@ -102,14 +102,11 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { private static final String OPT_SESSION_FIXATION_NO_PROTECTION = "none"; private static final String OPT_SESSION_FIXATION_MIGRATE_SESSION = "migrateSession"; - private static final String ATT_ACCESS_CONFIG = "access"; static final String ATT_REQUIRES_CHANNEL = "requires-channel"; private static final String OPT_REQUIRES_HTTP = "http"; private static final String OPT_REQUIRES_HTTPS = "https"; private static final String OPT_ANY_CHANNEL = "any"; - private static final String ATT_HTTP_METHOD = "method"; - private static final String ATT_CREATE_SESSION = "create-session"; private static final String DEF_CREATE_SESSION_IF_REQUIRED = "ifRequired"; private static final String OPT_CREATE_SESSION_ALWAYS = "always"; @@ -169,7 +166,6 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { * the map of filter chains defined, with the "universal" match pattern mapped to the list of beans which have been parsed here. */ public BeanDefinition parse(Element element, ParserContext pc) { -// WebConfigUtils.registerProviderManagerIfNecessary(pc, element); CompositeComponentDefinition compositeDef = new CompositeComponentDefinition(element.getTagName(), pc.extractSource(element)); pc.pushContainingComponent(compositeDef); @@ -181,12 +177,12 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { final boolean convertPathsToLowerCase = (matcher instanceof AntUrlPathMatcher) && matcher.requiresLowerCaseUrl(); final boolean allowSessionCreation = !OPT_CREATE_SESSION_NEVER.equals(element.getAttribute(ATT_CREATE_SESSION)); final boolean autoConfig = "true".equals(element.getAttribute(ATT_AUTO_CONFIG)); - final Map> filterChainMap = new ManagedMap>(); - final LinkedHashMap> channelRequestMap = new LinkedHashMap>(); - - // filterChainMap and channelRequestMap are populated by this call - parseInterceptUrlsForChannelSecurityAndEmptyFilterChains(DomUtils.getChildElementsByTagName(element, Elements.INTERCEPT_URL), - filterChainMap, channelRequestMap, convertPathsToLowerCase, pc); + final List interceptUrls = DomUtils.getChildElementsByTagName(element, Elements.INTERCEPT_URL); + // Use ManagedMap to allow placeholder resolution + final ManagedMap> filterChainMap = + parseInterceptUrlsForEmptyFilterChains(interceptUrls, convertPathsToLowerCase, pc); + final LinkedHashMap> channelRequestMap = + parseInterceptUrlsForChannelSecurity(interceptUrls, convertPathsToLowerCase, pc); BeanDefinition cpf = null; BeanReference sessionRegistryRef = null; @@ -840,11 +836,10 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { boolean useExpressions = "true".equals(element.getAttribute(ATT_USE_EXPRESSIONS)); - LinkedHashMap> requestToAttributesMap = + ManagedMap requestToAttributesMap = parseInterceptUrlsForFilterInvocationRequestMap(DomUtils.getChildElementsByTagName(element, Elements.INTERCEPT_URL), convertPathsToLowerCase, useExpressions, pc); - RootBeanDefinition accessDecisionMgr; ManagedList voters = new ManagedList(2); @@ -1180,7 +1175,6 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { lowercaseComparisons = null; } - // Only change from the defaults if the attribute has been set if ("true".equals(lowercaseComparisons)) { if (useRegex) { @@ -1200,10 +1194,13 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { /** * Parses the intercept-url elements and populates the FilterChainProxy's filter chain Map and the * map used to create the FilterInvocationDefintionSource for the FilterSecurityInterceptor. + * @return */ - void parseInterceptUrlsForChannelSecurityAndEmptyFilterChains(List urlElts, Map> filterChainMap, Map> channelRequestMap, + LinkedHashMap> parseInterceptUrlsForChannelSecurity(List urlElts, boolean useLowerCasePaths, ParserContext parserContext) { + LinkedHashMap> channelRequestMap = new ManagedMap>(); + for (Element urlElt : urlElts) { String path = urlElt.getAttribute(ATT_PATH_PATTERN); @@ -1233,6 +1230,25 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { channelRequestMap.put(new RequestKey(path), SecurityConfig.createList((StringUtils.commaDelimitedListToStringArray(channelConfigAttribute)))); } + } + + return channelRequestMap; + } + + private ManagedMap> parseInterceptUrlsForEmptyFilterChains(List urlElts, + boolean useLowerCasePaths, ParserContext parserContext) { + ManagedMap> filterChainMap = new ManagedMap>(); + + for (Element urlElt : urlElts) { + String path = urlElt.getAttribute(ATT_PATH_PATTERN); + + if(!StringUtils.hasText(path)) { + parserContext.getReaderContext().error("path attribute cannot be empty or null", urlElt); + } + + if (useLowerCasePaths) { + path = path.toLowerCase(); + } String filters = urlElt.getAttribute(ATT_FILTERS); @@ -1246,62 +1262,20 @@ public class HttpSecurityBeanDefinitionParser implements BeanDefinitionParser { filterChainMap.put(path, noFilters); } } + + return filterChainMap; } /** * Parses the filter invocation map which will be used to configure the FilterInvocationSecurityMetadataSource * used in the security interceptor. */ - static LinkedHashMap> + private static ManagedMap parseInterceptUrlsForFilterInvocationRequestMap(List urlElts, boolean useLowerCasePaths, boolean useExpressions, ParserContext parserContext) { - LinkedHashMap> filterInvocationDefinitionMap = new LinkedHashMap>(); - - for (Element urlElt : urlElts) { - String access = urlElt.getAttribute(ATT_ACCESS_CONFIG); - if (!StringUtils.hasText(access)) { - continue; - } - - String path = urlElt.getAttribute(ATT_PATH_PATTERN); - - if(!StringUtils.hasText(path)) { - parserContext.getReaderContext().error("path attribute cannot be empty or null", urlElt); - } - - if (useLowerCasePaths) { - path = path.toLowerCase(); - } - - String method = urlElt.getAttribute(ATT_HTTP_METHOD); - if (!StringUtils.hasText(method)) { - method = null; - } - - // Convert the comma-separated list of access attributes to a List - - RequestKey key = new RequestKey(path, method); - List attributes = null; - - if (useExpressions) { - logger.info("Creating access control expression attribute '" + access + "' for " + key); - attributes = new ArrayList(1); - // The expression will be parsed later by the ExpressionFilterInvocationSecurityMetadataSource - attributes.add(new SecurityConfig(access)); - - } else { - attributes = SecurityConfig.createList(StringUtils.commaDelimitedListToStringArray(access)); - } - - if (filterInvocationDefinitionMap.containsKey(key)) { - logger.warn("Duplicate URL defined: " + key + ". The original attribute values will be overwritten"); - } - - filterInvocationDefinitionMap.put(key, attributes); - } + return FilterInvocationSecurityMetadataSourceBeanDefinitionParser.parseInterceptUrlsForFilterInvocationRequestMap(urlElts, useLowerCasePaths, useExpressions, parserContext); - return filterInvocationDefinitionMap; } } diff --git a/config/src/test/java/org/springframework/security/config/http/FilterSecurityMetadataSourceBeanDefinitionParserTests.java b/config/src/test/java/org/springframework/security/config/http/FilterSecurityMetadataSourceBeanDefinitionParserTests.java index 9be6bb319c..a8446ca122 100644 --- a/config/src/test/java/org/springframework/security/config/http/FilterSecurityMetadataSourceBeanDefinitionParserTests.java +++ b/config/src/test/java/org/springframework/security/config/http/FilterSecurityMetadataSourceBeanDefinitionParserTests.java @@ -58,6 +58,23 @@ public class FilterSecurityMetadataSourceBeanDefinitionParserTests { assertTrue(cad.contains(new SecurityConfig("ROLE_A"))); } + // SEC-1201 + @Test + public void interceptUrlsSupportPropertyPlaceholders() { + System.setProperty("secure.url", "/secure"); + System.setProperty("secure.role", "ROLE_A"); + setContext( + "" + + "" + + " " + + ""); + DefaultFilterInvocationSecurityMetadataSource fids = (DefaultFilterInvocationSecurityMetadataSource) appContext.getBean("fids"); + List cad = fids.getAttributes(createFilterInvocation("/secure", "GET")); + assertNotNull(cad); + assertEquals(1, cad.size()); + assertEquals("ROLE_A", cad.get(0).getAttribute()); + } + @Test public void parsingWithinFilterSecurityInterceptorIsSuccessful() { setContext( @@ -72,11 +89,8 @@ public class FilterSecurityMetadataSourceBeanDefinitionParserTests { " " + " "+ "" + ConfigTestUtils.AUTH_PROVIDER_XML); - - } - private FilterInvocation createFilterInvocation(String path, String method) { MockHttpServletRequest request = new MockHttpServletRequest(); request.setRequestURI(null); diff --git a/config/src/test/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParserTests.java b/config/src/test/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParserTests.java index 3ec1b806f1..2b57a5bee9 100644 --- a/config/src/test/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParserTests.java +++ b/config/src/test/java/org/springframework/security/config/http/HttpSecurityBeanDefinitionParserTests.java @@ -149,7 +149,7 @@ public class HttpSecurityBeanDefinitionParserTests { } @Test - public void filterListShouldBeEmptyForUnprotectedUrl() throws Exception { + public void filterListShouldBeEmptyForPatternWithNoFilters() throws Exception { setContext( " " + " " + @@ -160,6 +160,22 @@ public class HttpSecurityBeanDefinitionParserTests { assertTrue(filters.size() == 0); } + @Test + public void filtersEqualsNoneSupportsPlaceholderForPattern() throws Exception { + System.setProperty("pattern.nofilters", "/unprotected"); + setContext( + " " + + " " + + " " + + " " + + " " + AUTH_PROVIDER_XML); + + List filters = getFilters("/unprotected"); + + assertTrue(filters.size() == 0); + } + + @Test public void regexPathsWorkCorrectly() throws Exception { setContext( @@ -274,7 +290,7 @@ public class HttpSecurityBeanDefinitionParserTests { FilterSecurityInterceptor fis = (FilterSecurityInterceptor) getFilter(FilterSecurityInterceptor.class); FilterInvocationSecurityMetadataSource fids = fis.getSecurityMetadataSource(); - List attrDef = fids.getAttributes(createFilterinvocation("/Secure", null)); + List attrDef = fids.getAttributes(createFilterinvocation("/Secure", null)); assertEquals(2, attrDef.size()); assertTrue(attrDef.contains(new SecurityConfig("ROLE_A"))); assertTrue(attrDef.contains(new SecurityConfig("ROLE_B"))); @@ -283,6 +299,40 @@ public class HttpSecurityBeanDefinitionParserTests { assertTrue(attrDef.contains(new SecurityConfig("ROLE_C"))); } + // SEC-1201 + @Test + public void interceptUrlsAndFormLoginSupportPropertyPlaceholders() throws Exception { + System.setProperty("secure.url", "/secure"); + System.setProperty("secure.role", "ROLE_A"); + System.setProperty("login.page", "/loginPage"); + System.setProperty("default.target", "/defaultTarget"); + System.setProperty("auth.failure", "/authFailure"); + setContext( + "" + + "" + + " " + + " " + + "" + AUTH_PROVIDER_XML); + + // Check the security attribute + FilterSecurityInterceptor fis = (FilterSecurityInterceptor) getFilter(FilterSecurityInterceptor.class); + FilterInvocationSecurityMetadataSource fids = fis.getSecurityMetadataSource(); + List attrs = fids.getAttributes(createFilterinvocation("/secure", null)); + assertNotNull(attrs); + assertEquals(1, attrs.size()); + assertEquals("ROLE_A",attrs.get(0).getAttribute()); + + // Check the form login properties are set + UsernamePasswordAuthenticationProcessingFilter apf = (UsernamePasswordAuthenticationProcessingFilter) + getFilter(UsernamePasswordAuthenticationProcessingFilter.class); + assertEquals("/defaultTarget", FieldUtils.getFieldValue(apf, "successHandler.defaultTargetUrl")); + assertEquals("/authFailure", FieldUtils.getFieldValue(apf, "failureHandler.defaultFailureUrl")); + + ExceptionTranslationFilter etf = (ExceptionTranslationFilter) getFilter(ExceptionTranslationFilter.class); + assertEquals("/loginPage", FieldUtils.getFieldValue(etf, "authenticationEntryPoint.loginFormUrl")); + } + @Test public void httpMethodMatchIsSupported() throws Exception { setContext( @@ -338,6 +388,19 @@ public class HttpSecurityBeanDefinitionParserTests { assertTrue(filters.get(0) instanceof ChannelProcessingFilter); } + @Test + public void requiresChannelSupportsPlaceholder() throws Exception { + setContext( + " " + + " " + + " " + AUTH_PROVIDER_XML); + List filters = getFilters("/someurl"); + + assertEquals("Expected " + (AUTO_CONFIG_FILTERS + 1) +" filters in chain", AUTO_CONFIG_FILTERS + 1, filters.size()); + + assertTrue(filters.get(0) instanceof ChannelProcessingFilter); + } + @Test public void portMappingsAreParsedCorrectly() throws Exception { setContext( diff --git a/core/src/main/java/org/springframework/security/access/SecurityConfig.java b/core/src/main/java/org/springframework/security/access/SecurityConfig.java index 3bd5c3dbed..e3ac56dae3 100644 --- a/core/src/main/java/org/springframework/security/access/SecurityConfig.java +++ b/core/src/main/java/org/springframework/security/access/SecurityConfig.java @@ -19,6 +19,7 @@ import java.util.ArrayList; import java.util.List; import org.springframework.util.Assert; +import org.springframework.util.StringUtils; /** * Stores a {@link ConfigAttribute} as a String. @@ -62,7 +63,11 @@ public class SecurityConfig implements ConfigAttribute { return this.attrib; } - public static List createList(String... attributeNames) { + public final static List createListFromCommaDelimitedString(String access) { + return createList(StringUtils.commaDelimitedListToStringArray(access)); + } + + public final static List createList(String... attributeNames) { Assert.notNull(attributeNames, "You must supply a list of argument names"); List attributes = new ArrayList(attributeNames.length);