SEC-599: Refactoring of FilterInvocationDefinitionSource implementations to use a LinkedHashMap internally rather than list of "EntryHolder" classes.

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.Map;
-import java.util.Vector;
+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)
+    public ConfigAttributeDefinition getAttributes(Object object) throws IllegalArgumentException {
-        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;
     }
 }

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 
+        while (paths.hasNext()) {
-        if (fids.getConfigAttributeDefinitions() == null) {
+            Object entry = paths.next();
-            throw new IllegalArgumentException("FilterChainProxy requires the FilterInvocationDefinitionSource to " +
+            String path = entry instanceof Pattern ? ((Pattern)entry).pattern() : (String)entry;
-                    "return a non-null response to getConfigAttributeDefinitions()");
+            ConfigAttributeDefinition configAttributeDefinition = (ConfigAttributeDefinition) requestMap.get(entry);
-        }
-
-        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();
-            }
 
             List filters = new ArrayList();
-
             Iterator attributes = configAttributeDefinition.getConfigAttributes();
 
             while (attributes.hasNext()) {
                 ConfigAttribute attr = (ConfigAttribute) attributes.next();
                 String filterName = attr.getAttribute();
 
-                if (filterName == null) {
+                Assert.notNull(filterName, "Configuration attribute: '" + attr + "' returned null to the getAttribute() " +
-                    throw new IllegalArgumentException("Configuration attribute: '" + attr
+                        "method, which is invalid when used with FilterChainProxy");
-                        + "' 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));

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 <code>List</code> of <code>ConfigAttributeDefinition</code>s associated with different HTTP request
  * URL Apache Ant path-based patterns.<p>Apache Ant path expressions are used to match a HTTP request URL against a
- * <code>ConfigAttributeDefinition</code>.</p>
+ * <code>ConfigAttributeDefinition</code>.
- * <p>The order of registering the Ant paths using the {@link #addSecureUrl(String,ConfigAttributeDefinition)} is
+ * <p>
+ * The order of registering the Ant paths using the {@link #addSecureUrl(String,ConfigAttributeDefinition)} is
  * very important. The system will identify the <b>first</b>  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.</p>
+ * registered first, with the most general paths registered last.
- * <p>If no registered paths match the HTTP URL, <code>null</code> is returned.</p>
+ * <p>
+ * If no registered paths match the HTTP URL, <code>null</code> 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();
+        return getRequestMap().values().iterator();
-        Iterator iter = getRequestMap().iterator();
-
-        while (iter.hasNext()) {
-            EntryHolder entryHolder = (EntryHolder) iter.next();
-            set.add(entryHolder.getConfigAttributeDefinition());
-        }
-
-        return set.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()) {
+        while (paths.hasNext()) {
-            EntryHolder entryHolder = (EntryHolder) iter.next();
+            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="
+                logger.debug("Candidate is: '" + url + "'; pattern is " + path + "; matched=" + 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;
-        }
-    }
 }

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 <code>List</code> of <code>ConfigAttributeDefinition</code>s associated with different HTTP request
- * URL regular expression patterns.<p>Regular expressions are used to match a HTTP request URL against a
+ * URL regular expression patterns.
- * <code>ConfigAttributeDefinition</code>.</p>
+ * <p>
- *  <p>The order of registering the regular expressions using the {@link #addSecureUrl(String,
+ * Regular expressions are used to match a HTTP request URL against a <code>ConfigAttributeDefinition</code>.
+ * <p>
+ * The order of registering the regular expressions using the {@link #addSecureUrl(String,
  * ConfigAttributeDefinition)} is very important. The system will identify the <b>first</b>  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.</p>
+ * regular expressions registered last.
- *  <p>If no registered regular expressions match the HTTP URL, <code>null</code> is returned.</p>
+ * <p>
+ * If no registered regular expressions match the HTTP URL, <code>null</code> 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();
+        return getRequestMap().values().iterator();
-        Iterator iter = getRequestMap().iterator();
-
-        while (iter.hasNext()) {
-            EntryHolder entryHolder = (EntryHolder) iter.next();
-            set.add(entryHolder.getConfigAttributeDefinition());
-        }
-
-        return set.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()) {
+        Iterator patterns = getRequestMap().keySet().iterator();
-            EntryHolder entryHolder = (EntryHolder) iter.next();
 
-            Matcher matcher = entryHolder.getCompiledPattern().matcher(url);
+        while (patterns.hasNext()) {
-
+            Pattern p = (Pattern) patterns.next();
-            boolean matched = matcher.matches();
+            boolean matched = p.matcher(url).matches();
 
             if (logger.isDebugEnabled()) {
-                logger.debug("Candidate is: '" + url + "'; pattern is " + entryHolder.getCompiledPattern()
+                logger.debug("Candidate is: '" + url + "'; pattern is " + p.pattern() + "; matched=" + matched);
-                    + "; 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;
-        }
-    }
 }

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);

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(