diff --git a/core/src/main/java/org/springframework/security/intercept/web/AbstractFilterInvocationDefinitionSource.java b/core/src/main/java/org/springframework/security/intercept/web/AbstractFilterInvocationDefinitionSource.java index 4cd52ee18f..ce9cdde663 100644 --- a/core/src/main/java/org/springframework/security/intercept/web/AbstractFilterInvocationDefinitionSource.java +++ b/core/src/main/java/org/springframework/security/intercept/web/AbstractFilterInvocationDefinitionSource.java @@ -17,8 +17,8 @@ package org.springframework.security.intercept.web; import org.springframework.security.ConfigAttributeDefinition; -import java.util.List; -import java.util.Vector; +import java.util.Map; +import java.util.LinkedHashMap; /** @@ -29,14 +29,13 @@ import java.util.Vector; */ public abstract class AbstractFilterInvocationDefinitionSource implements FilterInvocationDefinitionSource { - protected List requestMap = new Vector(); + private Map requestMap = new LinkedHashMap(); private boolean convertUrlToLowercaseBeforeComparison = false; //~ Methods ======================================================================================================== - public ConfigAttributeDefinition getAttributes(Object object) - throws IllegalArgumentException { + public ConfigAttributeDefinition getAttributes(Object object) throws IllegalArgumentException { if ((object == null) || !this.supports(object.getClass())) { throw new IllegalArgumentException("Object must be a FilterInvocation"); } @@ -78,7 +77,7 @@ public abstract class AbstractFilterInvocationDefinitionSource implements Filter this.convertUrlToLowercaseBeforeComparison = convertUrlToLowercaseBeforeComparison; } - List getRequestMap() { + Map getRequestMap() { return requestMap; } } diff --git a/core/src/main/java/org/springframework/security/intercept/web/FIDSToFilterChainMapConverter.java b/core/src/main/java/org/springframework/security/intercept/web/FIDSToFilterChainMapConverter.java index c51ca02dbc..5951c35196 100644 --- a/core/src/main/java/org/springframework/security/intercept/web/FIDSToFilterChainMapConverter.java +++ b/core/src/main/java/org/springframework/security/intercept/web/FIDSToFilterChainMapConverter.java @@ -1,12 +1,14 @@ package org.springframework.security.intercept.web; import org.springframework.context.ApplicationContext; +import org.springframework.util.Assert; import org.springframework.security.ConfigAttribute; import org.springframework.security.ConfigAttributeDefinition; import org.springframework.security.util.FilterChainProxy; import javax.servlet.Filter; import java.util.*; +import java.util.regex.Pattern; /** * Used internally to provide backward compatibility for configuration of FilterChainProxy using a @@ -23,51 +25,34 @@ public class FIDSToFilterChainMapConverter { private LinkedHashMap filterChainMap = new LinkedHashMap(); - public FIDSToFilterChainMapConverter(FilterInvocationDefinitionSource fids, ApplicationContext appContext) { + public FIDSToFilterChainMapConverter(FilterInvocationDefinitionSource source, ApplicationContext appContext) { + // TODO: Check if this is necessary. Retained from refactoring of FilterChainProxy + Assert.notNull(source.getConfigAttributeDefinitions(), "FilterChainProxy requires the " + + "FilterInvocationDefinitionSource to return a non-null response to getConfigAttributeDefinitions()"); + Assert.isTrue( + source instanceof PathBasedFilterInvocationDefinitionMap || + source instanceof RegExpBasedFilterInvocationDefinitionMap, + "Can't handle FilterInvocationDefinitionSource type " + source.getClass()); + - List requestMap; + AbstractFilterInvocationDefinitionSource fids = (AbstractFilterInvocationDefinitionSource)source; + Map requestMap = fids.getRequestMap(); + Iterator paths = requestMap.keySet().iterator(); - // TODO: Check if this is necessary. Retained from refactoring of FilterChainProxy - if (fids.getConfigAttributeDefinitions() == null) { - throw new IllegalArgumentException("FilterChainProxy requires the FilterInvocationDefinitionSource to " + - "return a non-null response to getConfigAttributeDefinitions()"); - } - - if (fids instanceof PathBasedFilterInvocationDefinitionMap) { - requestMap = ((PathBasedFilterInvocationDefinitionMap)fids).getRequestMap(); - } else if (fids instanceof RegExpBasedFilterInvocationDefinitionMap) { - requestMap = ((RegExpBasedFilterInvocationDefinitionMap)fids).getRequestMap(); - } else { - throw new IllegalArgumentException("Can't handle FilterInvocationDefinitionSource type " + fids.getClass()); - } - - Iterator entries = requestMap.iterator(); - - while (entries.hasNext()) { - Object entry = entries.next(); - String path; - ConfigAttributeDefinition configAttributeDefinition; - - if (entry instanceof PathBasedFilterInvocationDefinitionMap.EntryHolder) { - path = ((PathBasedFilterInvocationDefinitionMap.EntryHolder)entry).getAntPath(); - configAttributeDefinition = ((PathBasedFilterInvocationDefinitionMap.EntryHolder)entry).getConfigAttributeDefinition(); - } else { - path = ((RegExpBasedFilterInvocationDefinitionMap.EntryHolder)entry).getCompiledPattern().pattern(); - configAttributeDefinition = ((RegExpBasedFilterInvocationDefinitionMap.EntryHolder)entry).getConfigAttributeDefinition(); - } + while (paths.hasNext()) { + Object entry = paths.next(); + String path = entry instanceof Pattern ? ((Pattern)entry).pattern() : (String)entry; + ConfigAttributeDefinition configAttributeDefinition = (ConfigAttributeDefinition) requestMap.get(entry); List filters = new ArrayList(); - Iterator attributes = configAttributeDefinition.getConfigAttributes(); while (attributes.hasNext()) { ConfigAttribute attr = (ConfigAttribute) attributes.next(); String filterName = attr.getAttribute(); - if (filterName == null) { - throw new IllegalArgumentException("Configuration attribute: '" + attr - + "' returned null to the getAttribute() method, which is invalid when used with FilterChainProxy"); - } + Assert.notNull(filterName, "Configuration attribute: '" + attr + "' returned null to the getAttribute() " + + "method, which is invalid when used with FilterChainProxy"); if (!filterName.equals(FilterChainProxy.TOKEN_NONE)) { filters.add(appContext.getBean(filterName, Filter.class)); diff --git a/core/src/main/java/org/springframework/security/intercept/web/PathBasedFilterInvocationDefinitionMap.java b/core/src/main/java/org/springframework/security/intercept/web/PathBasedFilterInvocationDefinitionMap.java index 01aaa17564..bf02a6c781 100644 --- a/core/src/main/java/org/springframework/security/intercept/web/PathBasedFilterInvocationDefinitionMap.java +++ b/core/src/main/java/org/springframework/security/intercept/web/PathBasedFilterInvocationDefinitionMap.java @@ -23,22 +23,20 @@ import org.apache.commons.logging.LogFactory; import org.springframework.util.AntPathMatcher; import org.springframework.util.PathMatcher; -import java.util.HashSet; import java.util.Iterator; -import java.util.List; -import java.util.Set; -import java.util.Vector; /** * Maintains a List of ConfigAttributeDefinitions associated with different HTTP request * URL Apache Ant path-based patterns.

Apache Ant path expressions are used to match a HTTP request URL against a - * ConfigAttributeDefinition.

- *

The order of registering the Ant paths using the {@link #addSecureUrl(String,ConfigAttributeDefinition)} is + * ConfigAttributeDefinition. + *

+ * The order of registering the Ant paths using the {@link #addSecureUrl(String,ConfigAttributeDefinition)} is * very important. The system will identify the first matching path for a given HTTP URL. It will not proceed * to evaluate later paths if a match has already been found. Accordingly, the most specific paths should be - * registered first, with the most general paths registered last.

- *

If no registered paths match the HTTP URL, null is returned.

+ * registered first, with the most general paths registered last. + *

+ * If no registered paths match the HTTP URL, null is returned. * * @author Ben Alex * @version $Id$ @@ -60,7 +58,7 @@ public class PathBasedFilterInvocationDefinitionMap extends AbstractFilterInvoca antPath = antPath.toLowerCase(); } - getRequestMap().add(new EntryHolder(antPath, attr)); + getRequestMap().put(antPath, attr); if (logger.isDebugEnabled()) { logger.debug("Added Ant path: " + antPath + "; attributes: " + attr); @@ -68,15 +66,7 @@ public class PathBasedFilterInvocationDefinitionMap extends AbstractFilterInvoca } public Iterator getConfigAttributeDefinitions() { - Set set = new HashSet(); - Iterator iter = getRequestMap().iterator(); - - while (iter.hasNext()) { - EntryHolder entryHolder = (EntryHolder) iter.next(); - set.add(entryHolder.getConfigAttributeDefinition()); - } - - return set.iterator(); + return getRequestMap().values().iterator(); } public ConfigAttributeDefinition lookupAttributes(String url) { @@ -95,47 +85,22 @@ public class PathBasedFilterInvocationDefinitionMap extends AbstractFilterInvoca } } - Iterator iter = requestMap.iterator(); + Iterator paths = getRequestMap().keySet().iterator(); - while (iter.hasNext()) { - EntryHolder entryHolder = (EntryHolder) iter.next(); + while (paths.hasNext()) { + String path = (String) paths.next(); - boolean matched = pathMatcher.match(entryHolder.getAntPath(), url); + boolean matched = pathMatcher.match(path, url); if (logger.isDebugEnabled()) { - logger.debug("Candidate is: '" + url + "'; pattern is " + entryHolder.getAntPath() + "; matched=" - + matched); + logger.debug("Candidate is: '" + url + "'; pattern is " + path + "; matched=" + matched); } if (matched) { - return entryHolder.getConfigAttributeDefinition(); + return (ConfigAttributeDefinition) getRequestMap().get(path); } } return null; } - - //~ Inner Classes ================================================================================================== - - protected class EntryHolder { - private ConfigAttributeDefinition configAttributeDefinition; - private String antPath; - - public EntryHolder(String antPath, ConfigAttributeDefinition attr) { - this.antPath = antPath; - this.configAttributeDefinition = attr; - } - - protected EntryHolder() { - throw new IllegalArgumentException("Cannot use default constructor"); - } - - public String getAntPath() { - return antPath; - } - - public ConfigAttributeDefinition getConfigAttributeDefinition() { - return configAttributeDefinition; - } - } } diff --git a/core/src/main/java/org/springframework/security/intercept/web/RegExpBasedFilterInvocationDefinitionMap.java b/core/src/main/java/org/springframework/security/intercept/web/RegExpBasedFilterInvocationDefinitionMap.java index 176f05acb2..11581c56cd 100644 --- a/core/src/main/java/org/springframework/security/intercept/web/RegExpBasedFilterInvocationDefinitionMap.java +++ b/core/src/main/java/org/springframework/security/intercept/web/RegExpBasedFilterInvocationDefinitionMap.java @@ -20,25 +20,22 @@ import org.springframework.security.ConfigAttributeDefinition; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import java.util.HashSet; import java.util.Iterator; -import java.util.List; -import java.util.Set; -import java.util.Vector; import java.util.regex.Pattern; -import java.util.regex.Matcher; - /** * Maintains a List of ConfigAttributeDefinitions associated with different HTTP request - * URL regular expression patterns.

Regular expressions are used to match a HTTP request URL against a - * ConfigAttributeDefinition.

- *

The order of registering the regular expressions using the {@link #addSecureUrl(String, + * URL regular expression patterns. + *

+ * Regular expressions are used to match a HTTP request URL against a ConfigAttributeDefinition. + *

+ * The order of registering the regular expressions using the {@link #addSecureUrl(String, * ConfigAttributeDefinition)} is very important. The system will identify the first matching regular * expression for a given HTTP URL. It will not proceed to evaluate later regular expressions if a match has already * been found. Accordingly, the most specific regular expressions should be registered first, with the most general - * regular expressions registered last.

- *

If no registered regular expressions match the HTTP URL, null is returned.

+ * regular expressions registered last. + *

+ * If no registered regular expressions match the HTTP URL, null is returned. */ public class RegExpBasedFilterInvocationDefinitionMap extends AbstractFilterInvocationDefinitionSource implements FilterInvocationDefinition { @@ -51,7 +48,7 @@ public class RegExpBasedFilterInvocationDefinitionMap extends AbstractFilterInvo public void addSecureUrl(String regExp, ConfigAttributeDefinition attr) { Pattern pattern = Pattern.compile(regExp); - getRequestMap().add(new EntryHolder(pattern, attr)); + getRequestMap().put(pattern, attr); if (logger.isDebugEnabled()) { logger.debug("Added regular expression: " + regExp + "; attributes: " + attr); @@ -59,20 +56,10 @@ public class RegExpBasedFilterInvocationDefinitionMap extends AbstractFilterInvo } public Iterator getConfigAttributeDefinitions() { - Set set = new HashSet(); - Iterator iter = getRequestMap().iterator(); - - while (iter.hasNext()) { - EntryHolder entryHolder = (EntryHolder) iter.next(); - set.add(entryHolder.getConfigAttributeDefinition()); - } - - return set.iterator(); + return getRequestMap().values().iterator(); } public ConfigAttributeDefinition lookupAttributes(String url) { - Iterator iter = getRequestMap().iterator(); - if (isConvertUrlToLowercaseBeforeComparison()) { url = url.toLowerCase(); @@ -81,47 +68,21 @@ public class RegExpBasedFilterInvocationDefinitionMap extends AbstractFilterInvo } } - while (iter.hasNext()) { - EntryHolder entryHolder = (EntryHolder) iter.next(); + Iterator patterns = getRequestMap().keySet().iterator(); - Matcher matcher = entryHolder.getCompiledPattern().matcher(url); - - boolean matched = matcher.matches(); + while (patterns.hasNext()) { + Pattern p = (Pattern) patterns.next(); + boolean matched = p.matcher(url).matches(); if (logger.isDebugEnabled()) { - logger.debug("Candidate is: '" + url + "'; pattern is " + entryHolder.getCompiledPattern() - + "; matched=" + matched); + logger.debug("Candidate is: '" + url + "'; pattern is " + p.pattern() + "; matched=" + matched); } if (matched) { - return entryHolder.getConfigAttributeDefinition(); + return (ConfigAttributeDefinition) getRequestMap().get(p); } } return null; } - - //~ Inner Classes ================================================================================================== - - protected class EntryHolder { - private ConfigAttributeDefinition configAttributeDefinition; - private Pattern compiledPattern; - - public EntryHolder(Pattern compiledPattern, ConfigAttributeDefinition attr) { - this.compiledPattern = compiledPattern; - this.configAttributeDefinition = attr; - } - - protected EntryHolder() { - throw new IllegalArgumentException("Cannot use default constructor"); - } - - public Pattern getCompiledPattern() { - return compiledPattern; - } - - public ConfigAttributeDefinition getConfigAttributeDefinition() { - return configAttributeDefinition; - } - } } diff --git a/core/src/test/java/org/springframework/security/intercept/web/FilterInvocationDefinitionSourceEditorTests.java b/core/src/test/java/org/springframework/security/intercept/web/FilterInvocationDefinitionSourceEditorTests.java index 8413320b49..b7680c3786 100644 --- a/core/src/test/java/org/springframework/security/intercept/web/FilterInvocationDefinitionSourceEditorTests.java +++ b/core/src/test/java/org/springframework/security/intercept/web/FilterInvocationDefinitionSourceEditorTests.java @@ -177,17 +177,6 @@ public class FilterInvocationDefinitionSourceEditorTests extends TestCase { assertEquals(2, map.getMapSize()); } - public void testNoArgConstructorDoesntExist() { - Class clazz = RegExpBasedFilterInvocationDefinitionMap.EntryHolder.class; - - try { - clazz.getDeclaredConstructor((Class[]) null); - fail("Should have thrown NoSuchMethodException"); - } catch (NoSuchMethodException expected) { - assertTrue(true); - } - } - public void testNullReturnsEmptyMap() { FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); editor.setAsText(null); diff --git a/core/src/test/java/org/springframework/security/intercept/web/FilterInvocationDefinitionSourceEditorWithPathsTests.java b/core/src/test/java/org/springframework/security/intercept/web/FilterInvocationDefinitionSourceEditorWithPathsTests.java index b91c93d459..9b13836bc8 100644 --- a/core/src/test/java/org/springframework/security/intercept/web/FilterInvocationDefinitionSourceEditorWithPathsTests.java +++ b/core/src/test/java/org/springframework/security/intercept/web/FilterInvocationDefinitionSourceEditorWithPathsTests.java @@ -47,14 +47,6 @@ public class FilterInvocationDefinitionSourceEditorWithPathsTests extends TestCa //~ Methods ======================================================================================================== - public static void main(String[] args) { - junit.textui.TestRunner.run(FilterInvocationDefinitionSourceEditorWithPathsTests.class); - } - - public final void setUp() throws Exception { - super.setUp(); - } - public void testAntPathDirectiveIsDetected() { FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); editor.setAsText( @@ -133,17 +125,6 @@ public class FilterInvocationDefinitionSourceEditorWithPathsTests extends TestCa assertEquals(2, map.getMapSize()); } - public void testNoArgConstructorDoesntExist() { - Class clazz = PathBasedFilterInvocationDefinitionMap.EntryHolder.class; - - try { - clazz.getDeclaredConstructor((Class[]) null); - fail("Should have thrown NoSuchMethodException"); - } catch (NoSuchMethodException expected) { - assertTrue(true); - } - } - public void testOrderOfEntriesIsPreservedOrderA() { FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); editor.setAsText(