SEC-599: Refactoring of FilterInvocationDefinitionSource implementations to use UrlPathMatcher strategy.

core/src/main/java/org/springframework/security/intercept/web/AbstractFilterInvocationDefinitionSource.java

@@ -16,25 +16,64 @@
 package org.springframework.security.intercept.web;
 
 import org.springframework.security.ConfigAttributeDefinition;
+import org.springframework.security.util.UrlMatcher;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 
 import java.util.Map;
 import java.util.LinkedHashMap;
+import java.util.Iterator;
 
 
 /**
  * Abstract implementation of <Code>FilterInvocationDefinitionSource</code>.
+ * <p>
+ * Stores an ordered map of compiled URL paths to <tt>ConfigAttributeDefinition</tt>s and provides URL matching
+ * against the items stored in this map using the confgured <tt>UrlMatcher</tt>.
+ * <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.
  *
  * @author Ben Alex
+ * @author Luke Taylor
  * @version $Id$
  */
 public abstract class AbstractFilterInvocationDefinitionSource implements FilterInvocationDefinitionSource {
 
+    protected final Log logger = LogFactory.getLog(getClass());
+
     private Map requestMap = new LinkedHashMap();
 
-    private boolean convertUrlToLowercaseBeforeComparison = false;
+    private UrlMatcher urlMatcher;
+
+    protected AbstractFilterInvocationDefinitionSource(UrlMatcher urlMatcher) {
+        this.urlMatcher = urlMatcher;
+    }
 
     //~ Methods ========================================================================================================
 
+    /**
+     * Adds a URL-ConfigAttributeDefinition pair to the request map, first allowing the <tt>UrlMatcher</tt> to
+     * process the pattern if required, using its <tt>compile</tt> method. The returned object will be used as the key
+     * to the request map and will be passed back to the <tt>UrlMatcher</tt> when iterating through the map to find
+     * a match for a particular URL.
+     */
+    public void addSecureUrl(String pattern, ConfigAttributeDefinition attr) {
+        requestMap.put(urlMatcher.compile(pattern), attr);
+
+        if (logger.isDebugEnabled()) {
+            logger.debug("Added URL pattern: " + pattern + "; attributes: " + attr);
+        }
+    }
+
+    public Iterator getConfigAttributeDefinitions() {
+        return getRequestMap().values().iterator();
+    }
+
     public ConfigAttributeDefinition getAttributes(Object object) throws IllegalArgumentException {
         if ((object == null) || !this.supports(object.getClass())) {
             throw new IllegalArgumentException("Object must be a FilterInvocation");
@@ -42,24 +81,53 @@ public abstract class AbstractFilterInvocationDefinitionSource implements Filter
 
         String url = ((FilterInvocation) object).getRequestUrl();
 
-        return this.lookupAttributes(url);
+        return lookupAttributes(url);
     }
 
     /**
      * Performs the actual lookup of the relevant <code>ConfigAttributeDefinition</code> for the specified
      * <code>FilterInvocation</code>.
-     * <p>Provided so subclasses need only to provide one basic method to properly interface with the
+     * <p>
-     * <code>FilterInvocationDefinitionSource</code>.
+     * By default, iterates through the stored URL map and calls the
-     * </p>
+     * {@link UrlMatcher#pathMatchesUrl(Object path, String url)} method until a match is found.
-     * <p>Public visiblity so that tablibs or other view helper classes can access the
+     * <p>
+     * Subclasses can override if required to perform any modifications to the URL.
+     * <p>
+     * Public visiblity so that tablibs or other view helper classes can access the
      * <code>ConfigAttributeDefinition</code> applying to a given URI pattern without needing to construct a mock
-     * <code>FilterInvocation</code> and retrieving the attibutes via the {@link #getAttributes(Object)} method.</p>
+     * <code>FilterInvocation</code> and retrieving the attibutes via the {@link #getAttributes(Object)} method.
      *
      * @param url the URI to retrieve configuration attributes for
      *
      * @return the <code>ConfigAttributeDefinition</code> that applies to the specified <code>FilterInvocation</code>
+     * or null if no match is foud
      */
-    public abstract ConfigAttributeDefinition lookupAttributes(String url);
+    public ConfigAttributeDefinition lookupAttributes(String url) {
+        if (urlMatcher.requiresLowerCaseUrl()) {
+            url = url.toLowerCase();
+
+            if (logger.isDebugEnabled()) {
+                logger.debug("Converted URL to lowercase, from: '" + url + "'; to: '" + url + "'");
+            }
+        }
+
+        Iterator patterns = requestMap.keySet().iterator();
+
+        while (patterns.hasNext()) {
+            Object p = patterns.next();            
+            boolean matched = urlMatcher.pathMatchesUrl(p, url);
+
+            if (logger.isDebugEnabled()) {
+                logger.debug("Candidate is: '" + url + "'; pattern is " + p + "; matched=" + matched);
+            }
+
+            if (matched) {
+                return (ConfigAttributeDefinition) getRequestMap().get(p);
+            }
+        }
+
+        return null;
+    }
 
     public boolean supports(Class clazz) {
         return FilterInvocation.class.isAssignableFrom(clazz);
@@ -69,15 +137,15 @@ public abstract class AbstractFilterInvocationDefinitionSource implements Filter
         return this.requestMap.size();
     }
 
-    public boolean isConvertUrlToLowercaseBeforeComparison() {
-        return convertUrlToLowercaseBeforeComparison;
-    }
-
-    public void setConvertUrlToLowercaseBeforeComparison(boolean convertUrlToLowercaseBeforeComparison) {
-        this.convertUrlToLowercaseBeforeComparison = convertUrlToLowercaseBeforeComparison;
-    }
-
     Map getRequestMap() {
         return requestMap;
     }
+
+    protected UrlMatcher getUrlMatcher() {
+        return urlMatcher;
+    }
+
+    public boolean isConvertUrlToLowercaseBeforeComparison() {
+        return urlMatcher.requiresLowerCaseUrl();
+    }
 }

core/src/main/java/org/springframework/security/intercept/web/FilterInvocationDefinitionSourceEditor.java

@@ -78,6 +78,8 @@ public class FilterInvocationDefinitionSourceEditor extends PropertyEditorSuppor
             }
 
             source.setConvertUrlToLowercaseBeforeComparison(true);
+        } else {
+            source.setConvertUrlToLowercaseBeforeComparison(false);
         }
 
         BufferedReader br = new BufferedReader(new StringReader(s));

core/src/main/java/org/springframework/security/intercept/web/PathBasedFilterInvocationDefinitionMap.java

@@ -16,20 +16,16 @@
 package org.springframework.security.intercept.web;
 
 import org.springframework.security.ConfigAttributeDefinition;
-
+import org.springframework.security.util.AntUrlPathMatcher;
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-
-import org.springframework.util.AntPathMatcher;
-import org.springframework.util.PathMatcher;
 
 import java.util.Iterator;
 
 
 /**
- * Maintains a <code>List</code> of <code>ConfigAttributeDefinition</code>s associated with different HTTP request
+ * Extends AbstractFilterInvocationDefinitionSource, configuring it with a {@link AntUrlPathMatcher} to match URLs
- * URL Apache Ant path-based patterns.<p>Apache Ant path expressions are used to match a HTTP request URL against a
+ * using Apache Ant path-based patterns.
- * <code>ConfigAttributeDefinition</code>.
+ * <p>
+ * Apache Ant path expressions are used to match a HTTP request URL against a <code>ConfigAttributeDefinition</code>.
  * <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
@@ -37,32 +33,33 @@ import java.util.Iterator;
  * registered first, with the most general paths registered last.
  * <p>
  * If no registered paths match the HTTP URL, <code>null</code> is returned.
+ * <p>
+ * Note that as of 2.0, lower case URL comparisons are made by default, as this is the default strategy for
+ * <tt>AntUrlPathMatcher</tt>.
  *
  * @author Ben Alex
+ * @author Luke taylor
  * @version $Id$
  */
 public class PathBasedFilterInvocationDefinitionMap extends AbstractFilterInvocationDefinitionSource
         implements FilterInvocationDefinition {
-    //~ Static fields/initializers =====================================================================================
 
-    private static final Log logger = LogFactory.getLog(PathBasedFilterInvocationDefinitionMap.class);
+    //~ Constructors ===================================================================================================
 
-    private PathMatcher pathMatcher = new AntPathMatcher();
+    public PathBasedFilterInvocationDefinitionMap() {
+        super(new AntUrlPathMatcher());
+    }
 
     //~ Methods ========================================================================================================
 
     public void addSecureUrl(String antPath, ConfigAttributeDefinition attr) {
         // SEC-501: If using lower case comparison, we should convert the paths to lower case
         // as any upper case characters included by mistake will prevent the URL from ever being matched.
-        if (isConvertUrlToLowercaseBeforeComparison()) {
+        if (getUrlMatcher().requiresLowerCaseUrl()) {
             antPath = antPath.toLowerCase();
         }
 
-        getRequestMap().put(antPath, attr);
+        super.addSecureUrl(antPath, attr);
-
-        if (logger.isDebugEnabled()) {
-            logger.debug("Added Ant path: " + antPath + "; attributes: " + attr);
-        }
     }
 
     public Iterator getConfigAttributeDefinitions() {
@@ -77,30 +74,10 @@ public class PathBasedFilterInvocationDefinitionMap extends AbstractFilterInvoca
             url = url.substring(0, firstQuestionMarkIndex);
         }
 
-        if (isConvertUrlToLowercaseBeforeComparison()) {
+        return super.lookupAttributes(url);
-            url = url.toLowerCase();
+    }
 
-            if (logger.isDebugEnabled()) {
+    public void setConvertUrlToLowercaseBeforeComparison(boolean bool) {
-                logger.debug("Converted URL to lowercase, from: '" + url + "'; to: '" + url + "'");
+        ((AntUrlPathMatcher)getUrlMatcher()).setRequiresLowerCaseUrl(bool);
-            }
-        }
-
-        Iterator paths = getRequestMap().keySet().iterator();
-
-        while (paths.hasNext()) {
-            String path = (String) paths.next();
-
-            boolean matched = pathMatcher.match(path, url);
-
-            if (logger.isDebugEnabled()) {
-                logger.debug("Candidate is: '" + url + "'; pattern is " + path + "; matched=" + matched);
-            }
-
-            if (matched) {
-                return (ConfigAttributeDefinition) getRequestMap().get(path);
-            }
-        }
-
-        return null;
     }
 }

core/src/main/java/org/springframework/security/intercept/web/RegExpBasedFilterInvocationDefinitionMap.java

@@ -15,74 +15,28 @@
 
 package org.springframework.security.intercept.web;
 
-import org.springframework.security.ConfigAttributeDefinition;
+import org.springframework.security.util.RegexUrlPathMatcher;
+import org.springframework.security.util.AntUrlPathMatcher;
 
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-
-import java.util.Iterator;
-import java.util.regex.Pattern;
 
 /**
- * Maintains a <code>List</code> of <code>ConfigAttributeDefinition</code>s associated with different HTTP request
+ * Configures an {@link AbstractFilterInvocationDefinitionSource} with a regular expression URL matching strategy
- * URL regular expression patterns.
+ * {@link RegexUrlPathMatcher}.
- * <p>
+ *
- * Regular expressions are used to match a HTTP request URL against a <code>ConfigAttributeDefinition</code>.
+ * @author Ben Alex
- * <p>
+ * @author Luke Taylor
- * The order of registering the regular expressions using the {@link #addSecureUrl(String,
+ * @version $Id$
- * 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>
- * If no registered regular expressions match the HTTP URL, <code>null</code> is returned.
  */
 public class RegExpBasedFilterInvocationDefinitionMap extends AbstractFilterInvocationDefinitionSource
     implements FilterInvocationDefinition {
-    //~ Static fields/initializers =====================================================================================
 
-    private static final Log logger = LogFactory.getLog(RegExpBasedFilterInvocationDefinitionMap.class);
+    //~ Constructors ===================================================================================================
-
+    
-    //~ Methods ========================================================================================================
+    public RegExpBasedFilterInvocationDefinitionMap() {
-
+        super(new RegexUrlPathMatcher());
-    public void addSecureUrl(String regExp, ConfigAttributeDefinition attr) {
-        Pattern pattern = Pattern.compile(regExp);
-
-        getRequestMap().put(pattern, attr);
-
-        if (logger.isDebugEnabled()) {
-            logger.debug("Added regular expression: " + regExp + "; attributes: " + attr);
-        }
     }
 
-    public Iterator getConfigAttributeDefinitions() {
+    public void setConvertUrlToLowercaseBeforeComparison(boolean bool) {
-        return getRequestMap().values().iterator();
+        ((RegexUrlPathMatcher)getUrlMatcher()).setRequiresLowerCaseUrl(bool);
-    }
-
-    public ConfigAttributeDefinition lookupAttributes(String url) {
-        if (isConvertUrlToLowercaseBeforeComparison()) {
-            url = url.toLowerCase();
-
-            if (logger.isDebugEnabled()) {
-                logger.debug("Converted URL to lowercase, from: '" + url + "'; to: '" + url + "'");
-            }
-        }
-
-        Iterator patterns = getRequestMap().keySet().iterator();
-
-        while (patterns.hasNext()) {
-            Pattern p = (Pattern) patterns.next();
-            boolean matched = p.matcher(url).matches();
-
-            if (logger.isDebugEnabled()) {
-                logger.debug("Candidate is: '" + url + "'; pattern is " + p.pattern() + "; matched=" + matched);
-            }
-
-            if (matched) {
-                return (ConfigAttributeDefinition) getRequestMap().get(p);
-            }
-        }
-
-        return null;
     }
 }

core/src/main/java/org/springframework/security/util/AntUrlPathMatcher.java

@@ -8,39 +8,36 @@ import org.apache.commons.logging.LogFactory;
 /**
  * Ant path strategy for URL matching.
  *
- * @author luke
+ * @author Luke Taylor
  * @version $Id$
  */
 public class AntUrlPathMatcher implements UrlMatcher {
     private static final Log logger = LogFactory.getLog(AntUrlPathMatcher.class);
 
-    private boolean convertToLowercaseBeforeComparison = true;
+    private boolean requiresLowerCaseUrl = true;
     private PathMatcher pathMatcher = new AntPathMatcher();
 
     public Object compile(String path) {
-        if (convertToLowercaseBeforeComparison) {
+        if (requiresLowerCaseUrl) {
             return path.toLowerCase();
         }
 
         return path;
     }
 
-    public void setConvertToLowercaseBeforeComparison(boolean convertToLowercaseBeforeComparison) {
+    public void setRequiresLowerCaseUrl(boolean requiresLowerCaseUrl) {
-        this.convertToLowercaseBeforeComparison = convertToLowercaseBeforeComparison;
+        this.requiresLowerCaseUrl = requiresLowerCaseUrl;
     }
 
     public boolean pathMatchesUrl(Object path, String url) {
-        if (convertToLowercaseBeforeComparison) {
-            url = url.toLowerCase();
-            if (logger.isDebugEnabled()) {
-                logger.debug("Converted URL to lowercase, from: '" + url + "'; to: '" + url + "'");
-            }
-        }
-
         return pathMatcher.match((String)path, url);
     }
 
     public String getUniversalMatchPattern() {
         return "/**";
     }
+
+    public boolean requiresLowerCaseUrl() {
+        return requiresLowerCaseUrl;
+    }
 }

core/src/main/java/org/springframework/security/util/FilterChainProxy.java

@@ -184,6 +184,14 @@ public class FilterChainProxy implements Filter, InitializingBean, ApplicationCo
             Map.Entry entry = (Map.Entry) filterChains.next();
             Object path = entry.getKey();
 
+            if (matcher.requiresLowerCaseUrl()) {
+                url = url.toLowerCase();
+
+                if (logger.isDebugEnabled()) {
+                    logger.debug("Converted URL to lowercase, from: '" + url + "'; to: '" + url + "'");
+                }
+            }
+            
             boolean matched = matcher.pathMatchesUrl(path, url);
 
             if (logger.isDebugEnabled()) {

core/src/main/java/org/springframework/security/util/RegexUrlPathMatcher.java

@@ -6,36 +6,33 @@ import org.apache.commons.logging.LogFactory;
 import java.util.regex.Pattern;
 
 /**
- * @author luke
+ * @author Luke Taylor
  * @version $Id$
  */
 public class RegexUrlPathMatcher implements UrlMatcher {
     private static final Log logger = LogFactory.getLog(RegexUrlPathMatcher.class);
 
-    private boolean convertUrlToLowercaseBeforeComparison = true;
+    private boolean requiresLowerCaseUrl = false;
 
     public Object compile(String path) {
         return Pattern.compile(path);
     }
 
-    public void setConvertUrlToLowercaseBeforeComparison(boolean convertUrlToLowercaseBeforeComparison) {
+    public void setRequiresLowerCaseUrl(boolean requiresLowerCaseUrl) {
-        this.convertUrlToLowercaseBeforeComparison = convertUrlToLowercaseBeforeComparison;
+        this.requiresLowerCaseUrl = requiresLowerCaseUrl;
     }
 
     public boolean pathMatchesUrl(Object compiledPath, String url) {
         Pattern pattern = (Pattern)compiledPath;
 
-        if (convertUrlToLowercaseBeforeComparison) {
-            url = url.toLowerCase();
-            if (logger.isDebugEnabled()) {
-                logger.debug("Converted URL to lowercase, from: '" + url + "'; to: '" + url + "'");
-            }
-        }
-
         return pattern.matcher(url).matches();
     }
 
     public String getUniversalMatchPattern() {
         return "/.*";
     }
+
+    public boolean requiresLowerCaseUrl() {
+        return requiresLowerCaseUrl;
+    }
 }

core/src/main/java/org/springframework/security/util/UrlMatcher.java

@@ -15,4 +15,10 @@ public interface UrlMatcher {
 
     /** Returns the path which matches every URL */
     String getUniversalMatchPattern();
+
+    /**
+     * Returns true if the matcher expects the URL to be converted to lower case before
+     * calling {@link #pathMatchesUrl(Object, String)}.
+     */
+    boolean requiresLowerCaseUrl();
 }

core/src/test/java/org/springframework/security/intercept/web/MockFilterInvocationDefinitionSource.java

@@ -17,6 +17,7 @@ package org.springframework.security.intercept.web;
 
 import org.springframework.security.ConfigAttributeDefinition;
 import org.springframework.security.SecurityConfig;
+import org.springframework.security.util.AntUrlPathMatcher;
 
 import java.util.Iterator;
 import java.util.List;
@@ -38,6 +39,7 @@ public class MockFilterInvocationDefinitionSource extends AbstractFilterInvocati
     //~ Constructors ===================================================================================================
 
     public MockFilterInvocationDefinitionSource(boolean includeInvalidAttributes, boolean returnAnIteratorWhenRequested) {
+        super(new AntUrlPathMatcher()); // doesn't matter
         returnAnIterator = returnAnIteratorWhenRequested;
         list = new Vector();
 

core/src/test/java/org/springframework/security/intercept/web/PathBasedFilterDefinitionMapTests.java

@@ -44,15 +44,15 @@ public class PathBasedFilterDefinitionMapTests extends TestCase {
 
     //~ Methods ========================================================================================================
 
-    public void testConvertUrlToLowercaseIsFalseByDefault() {
+    public void testConvertUrlToLowercaseIsTrueByDefault() {
         PathBasedFilterInvocationDefinitionMap map = new PathBasedFilterInvocationDefinitionMap();
-        assertFalse(map.isConvertUrlToLowercaseBeforeComparison());
+        assertTrue(map.isConvertUrlToLowercaseBeforeComparison());
     }
 
     public void testConvertUrlToLowercaseSetterRespected() {
         PathBasedFilterInvocationDefinitionMap map = new PathBasedFilterInvocationDefinitionMap();
-        map.setConvertUrlToLowercaseBeforeComparison(true);
+        map.setConvertUrlToLowercaseBeforeComparison(false);
-        assertTrue(map.isConvertUrlToLowercaseBeforeComparison());
+        assertFalse(map.isConvertUrlToLowercaseBeforeComparison());
     }
 
     public void testLookupNotRequiringExactMatchSuccessIfNotMatching() {
@@ -70,11 +70,10 @@ public class PathBasedFilterDefinitionMapTests extends TestCase {
     }
 
     /**
-     * SEC-501
+     * SEC-501. Not that as of 2.0, lower case comparisons are the default for this class.
      */
     public void testLookupNotRequiringExactMatchSucceedsIfSecureUrlPathContainsUpperCase() {
         PathBasedFilterInvocationDefinitionMap map = new PathBasedFilterInvocationDefinitionMap();
-        map.setConvertUrlToLowercaseBeforeComparison(true);
 
         ConfigAttributeDefinition def = new ConfigAttributeDefinition();
         def.addConfigAttribute(new SecurityConfig("ROLE_ONE"));
@@ -89,6 +88,7 @@ public class PathBasedFilterDefinitionMapTests extends TestCase {
 
     public void testLookupRequiringExactMatchFailsIfNotMatching() {
         PathBasedFilterInvocationDefinitionMap map = new PathBasedFilterInvocationDefinitionMap();
+        map.setConvertUrlToLowercaseBeforeComparison(false);
         ConfigAttributeDefinition def = new ConfigAttributeDefinition();
         def.addConfigAttribute(new SecurityConfig("ROLE_ONE"));
         map.addSecureUrl("/secure/super/**", def);
@@ -101,6 +101,7 @@ public class PathBasedFilterDefinitionMapTests extends TestCase {
 
     public void testLookupRequiringExactMatchIsSuccessful() {
         PathBasedFilterInvocationDefinitionMap map = new PathBasedFilterInvocationDefinitionMap();
+        map.setConvertUrlToLowercaseBeforeComparison(false);
         ConfigAttributeDefinition def = new ConfigAttributeDefinition();
         def.addConfigAttribute(new SecurityConfig("ROLE_ONE"));
         map.addSecureUrl("/SeCurE/super/**", def);

core/src/test/java/org/springframework/security/util/FilterChainProxyTests.java

@@ -167,7 +167,7 @@ public class FilterChainProxyTests {
         assertEquals(1, filters.size());
         assertTrue(filters.get(0) instanceof MockFilter);
 
-        filters = filterChainProxy.getFilters("/some/other/path/blah");
+        filters = filterChainProxy.getFilters("/sOme/other/path/blah");
         assertEquals(3, filters.size());
         assertTrue(filters.get(0) instanceof HttpSessionContextIntegrationFilter);
         assertTrue(filters.get(1) instanceof MockFilter);

core/src/test/resources/org/springframework/security/util/filtertest-valid.xml

@@ -73,7 +73,7 @@ http://www.springframework.org/schema/security http://www.springframework.org/sc
     <bean id="newFilterChainProxyRegex" class="org.springframework.security.util.FilterChainProxy">
         <sec:filter-chain-map path-type="regex">
             <sec:filter-chain pattern="\A/foo/.*\Z" filters="mockFilter"/>
-            <sec:filter-chain pattern="\A/some/other/path/.*\Z" filters="sif,mockFilter,mockFilter2"/>
+            <sec:filter-chain pattern="\A/s[oO]me/other/path/.*\Z" filters="sif,mockFilter,mockFilter2"/>
             <sec:filter-chain pattern="\A/do/not/filter\Z" filters="none"/>
             <sec:filter-chain pattern="\A/.*\Z" filters="sif,apf,mockFilter"/>
         </sec:filter-chain-map>