SEC-1033: Refactored DefaultFilterInvocationDefinitionSource to remove legacy methods and make it immutable.

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

@@ -57,12 +57,8 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation
 
     protected final Log logger = LogFactory.getLog(getClass());
 
-    /**
-     * Non method-specific map of URL patterns to <tt>List<ConfiAttribute></tt>s
-     * TODO: Store in the httpMethod map with null key.
-     */
-    private Map<Object, List<ConfigAttribute>> requestMap = new LinkedHashMap<Object, List<ConfigAttribute>>();
-    /** Stores request maps keyed by specific HTTP methods */
+    //private Map<Object, List<ConfigAttribute>> requestMap = new LinkedHashMap<Object, List<ConfigAttribute>>();
+    /** Stores request maps keyed by specific HTTP methods. A null key matches any method */
     private Map<String, Map<Object, List<ConfigAttribute>>> httpMethodMap =
         new HashMap<String, Map<Object, List<ConfigAttribute>>>();
 
@@ -70,13 +66,7 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation
 
     private boolean stripQueryStringFromUrls;
 
-    /**
-     * Creates a FilterInvocationDefinitionSource with the supplied URL matching strategy.
-     * @param urlMatcher
-     */
-    DefaultFilterInvocationDefinitionSource(UrlMatcher urlMatcher) {
-        this.urlMatcher = urlMatcher;
-    }
+    //~ Constructors ===================================================================================================
 
     /**
      * Builds the internal request map from the supplied map. The key elements should be of type {@link RequestKey},
@@ -97,17 +87,13 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation
 
     //~ Methods ========================================================================================================
 
-    void addSecureUrl(String pattern, List<ConfigAttribute> attr) {
-        addSecureUrl(pattern, null, attr);
-    }
-
     /**
      * Adds a URL,attribute-list 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.
      */
-    void addSecureUrl(String pattern, String method, List<ConfigAttribute> attr) {
+    private void addSecureUrl(String pattern, String method, List<ConfigAttribute> attr) {
         Map<Object, List<ConfigAttribute>> mapToUse = getRequestMapForHttpMethod(method);
 
         mapToUse.put(urlMatcher.compile(pattern), attr);
@@ -124,28 +110,27 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation
      * @return map of URL patterns to <tt>ConfigAttribute</tt>s for this method.
      */
     private Map<Object, List<ConfigAttribute>> getRequestMapForHttpMethod(String method) {
-        if (method == null) {
-            return requestMap;
-        }
-        if (!HTTP_METHODS.contains(method)) {
+        if (method != null && !HTTP_METHODS.contains(method)) {
             throw new IllegalArgumentException("Unrecognised HTTP method: '" + method + "'");
         }
 
-        Map<Object, List<ConfigAttribute>> methodRequestmap = httpMethodMap.get(method);
+        Map<Object, List<ConfigAttribute>> methodRequestMap = httpMethodMap.get(method);
 
-        if (methodRequestmap == null) {
-            methodRequestmap = new LinkedHashMap<Object, List<ConfigAttribute>>();
-            httpMethodMap.put(method, methodRequestmap);
+        if (methodRequestMap == null) {
+            methodRequestMap = new LinkedHashMap<Object, List<ConfigAttribute>>();
+            httpMethodMap.put(method, methodRequestMap);
         }
 
-        return methodRequestmap;
+        return methodRequestMap;
     }
 
     public Collection<ConfigAttribute> getAllConfigAttributes() {
         Set<ConfigAttribute> allAttributes = new HashSet<ConfigAttribute>();
 
-        for(List<ConfigAttribute> attrs : requestMap.values()) {
-            allAttributes.addAll(attrs);
+        for (Map.Entry<String, Map<Object, List<ConfigAttribute>>> entry : httpMethodMap.entrySet()) {
+            for (List<ConfigAttribute> attrs : entry.getValue().values()) {
+                allAttributes.addAll(attrs);
+            }
         }
 
         return allAttributes;
@@ -163,10 +148,6 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation
         return lookupAttributes(url, method);
     }
 
-    protected List<? extends ConfigAttribute> lookupAttributes(String url) {
-        return lookupAttributes(url, null);
-    }
-
     /**
      * Performs the actual lookup of the relevant <tt>ConfigAttribute</tt>s for the given <code>FilterInvocation</code>.
      * <p>
@@ -179,9 +160,9 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation
      * @param method the HTTP method (GET, POST, DELETE...).
      *
      * @return the <code>ConfigAttribute</code>s that apply to the specified <code>FilterInvocation</code>
-     * or null if no match is foud
+     * or null if no match is found
      */
-    public List<ConfigAttribute> lookupAttributes(String url, String method) {
+    public final List<ConfigAttribute> lookupAttributes(String url, String method) {
         if (stripQueryStringFromUrls) {
             // Strip anything after a question mark symbol, as per SEC-161. See also SEC-321
             int firstQuestionMarkIndex = url.indexOf("?");
@@ -199,33 +180,26 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation
             }
         }
 
-        List<ConfigAttribute> attributes = null;
-
-        Map<Object, List<ConfigAttribute>> methodSpecificMap = httpMethodMap.get(method);
-
-        if (methodSpecificMap != null) {
-            attributes = lookupUrlInMap(methodSpecificMap, url);
-        }
+        // Obtain the map of request patterns to attributes for this method and lookup the url.
+        Map<Object, List<ConfigAttribute>> requestMap = httpMethodMap.get(method);
 
-        if (attributes == null) {
-            attributes = lookupUrlInMap(requestMap, url);
+        // If no method-specific map, use the general one stored under the null key
+        if (requestMap == null) {
+            requestMap = httpMethodMap.get(null);
         }
 
-        return attributes;
-    }
-
-    private List<ConfigAttribute> lookupUrlInMap(Map<Object, List<ConfigAttribute>> requestMap, String url) {
-
-        for (Map.Entry<Object, List<ConfigAttribute>> entry : requestMap.entrySet()) {
-            Object p = entry.getKey();
-            boolean matched = urlMatcher.pathMatchesUrl(entry.getKey(), url);
+        if (requestMap != null) {
+            for (Map.Entry<Object, List<ConfigAttribute>> entry : requestMap.entrySet()) {
+                Object p = entry.getKey();
+                boolean matched = urlMatcher.pathMatchesUrl(entry.getKey(), url);
 
-            if (logger.isDebugEnabled()) {
-                logger.debug("Candidate is: '" + url + "'; pattern is " + p + "; matched=" + matched);
-            }
+                if (logger.isDebugEnabled()) {
+                    logger.debug("Candidate is: '" + url + "'; pattern is " + p + "; matched=" + matched);
+                }
 
-            if (matched) {
-                return entry.getValue();
+                if (matched) {
+                    return entry.getValue();
+                }
             }
         }
 

core/src/test/java/org/springframework/security/config/FilterInvocationDefinitionSourceParserTests.java

@@ -1,6 +1,6 @@
 package org.springframework.security.config;
 
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.*;
 
 import java.util.List;
 
@@ -44,6 +44,7 @@ public class FilterInvocationDefinitionSourceParserTests {
                 "</filter-invocation-definition-source>");
         DefaultFilterInvocationDefinitionSource fids = (DefaultFilterInvocationDefinitionSource) appContext.getBean("fids");
         List<? extends ConfigAttribute> cad = fids.getAttributes(createFilterInvocation("/anything", "GET"));
+        assertNotNull(cad);
         assertTrue(cad.contains(new SecurityConfig("ROLE_A")));
     }
 

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

@@ -15,13 +15,11 @@
 
 package org.springframework.security.intercept.web;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.*;
 
+import java.util.LinkedHashMap;
 import java.util.List;
 
-import org.junit.Before;
 import org.junit.Test;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
@@ -37,29 +35,41 @@ import org.springframework.security.util.AntUrlPathMatcher;
  * @author Ben Alex
  * @version $Id$
  */
+@SuppressWarnings("unchecked")
 public class DefaultFilterInvocationDefinitionSourceTests {
-    private DefaultFilterInvocationDefinitionSource map;
+    private DefaultFilterInvocationDefinitionSource fids;
     private List<ConfigAttribute> def = SecurityConfig.createList("ROLE_ONE");
 
     //~ Methods ========================================================================================================
-    @Before
-    public void createMap() {
-        map = new DefaultFilterInvocationDefinitionSource(new AntUrlPathMatcher());
-        map.setStripQueryStringFromUrls(true);
+    private void createFids(String url, String method) {
+        LinkedHashMap requestMap = new LinkedHashMap();
+        requestMap.put(new RequestKey(url, method), def);
+        fids = new DefaultFilterInvocationDefinitionSource(new AntUrlPathMatcher(), requestMap);
+        fids.setStripQueryStringFromUrls(true);
+    }
+
+    private void createFids(String url, boolean convertToLowerCase) {
+        LinkedHashMap requestMap = new LinkedHashMap();
+        requestMap.put(new RequestKey(url), def);
+        fids = new DefaultFilterInvocationDefinitionSource(new AntUrlPathMatcher(convertToLowerCase), requestMap);
+        fids.setStripQueryStringFromUrls(true);
     }
 
     @Test
     public void convertUrlToLowercaseIsTrueByDefault() {
-        assertTrue(map.isConvertUrlToLowercaseBeforeComparison());
+        LinkedHashMap requestMap = new LinkedHashMap();
+        requestMap.put(new RequestKey("/something"), def);
+        fids = new DefaultFilterInvocationDefinitionSource(new AntUrlPathMatcher(), requestMap);
+        assertTrue(fids.isConvertUrlToLowercaseBeforeComparison());
     }
 
     @Test
-    public void lookupNotRequiringExactMatchSuccessIfNotMatching() {
-        map.addSecureUrl("/secure/super/**", def);
+    public void lookupNotRequiringExactMatchSucceedsIfNotMatching() {
+        createFids("/secure/super/**", null);
 
         FilterInvocation fi = createFilterInvocation("/SeCuRE/super/somefile.html", null);
 
-        assertEquals(def, map.lookupAttributes(fi.getRequestUrl()));
+        assertEquals(def, fids.lookupAttributes(fi.getRequestUrl(), null));
     }
 
     /**
@@ -67,81 +77,86 @@ public class DefaultFilterInvocationDefinitionSourceTests {
      */
     @Test
     public void lookupNotRequiringExactMatchSucceedsIfSecureUrlPathContainsUpperCase() {
-        map.addSecureUrl("/SeCuRE/super/**", def);
+        createFids("/SeCuRE/super/**", null);
 
         FilterInvocation fi = createFilterInvocation("/secure/super/somefile.html", null);
 
-        List<? extends ConfigAttribute> response = map.lookupAttributes(fi.getRequestUrl());
+        List<? extends ConfigAttribute> response = fids.lookupAttributes(fi.getRequestUrl(), null);
         assertEquals(def, response);
     }
 
-
     @Test
     public void lookupRequiringExactMatchFailsIfNotMatching() {
-        map = new DefaultFilterInvocationDefinitionSource(new AntUrlPathMatcher(false));
-        map.addSecureUrl("/secure/super/**", def);
+        createFids("/secure/super/**", false);
 
         FilterInvocation fi = createFilterInvocation("/SeCuRE/super/somefile.html", null);
 
-        List<? extends ConfigAttribute> response = map.lookupAttributes(fi.getRequestUrl());
+        List<? extends ConfigAttribute> response = fids.lookupAttributes(fi.getRequestUrl(), null);
         assertEquals(null, response);
     }
 
     @Test
     public void lookupRequiringExactMatchIsSuccessful() {
-        map = new DefaultFilterInvocationDefinitionSource(new AntUrlPathMatcher(false));
-        map.addSecureUrl("/SeCurE/super/**", def);
+        createFids("/SeCurE/super/**", false);
 
         FilterInvocation fi = createFilterInvocation("/SeCurE/super/somefile.html", null);
 
-        List<? extends ConfigAttribute> response = map.lookupAttributes(fi.getRequestUrl());
+        List<? extends ConfigAttribute> response = fids.lookupAttributes(fi.getRequestUrl(), null);
         assertEquals(def, response);
     }
 
     @Test
     public void lookupRequiringExactMatchWithAdditionalSlashesIsSuccessful() {
-        map.addSecureUrl("/someAdminPage.html**", def);
+        createFids("/someAdminPage.html**", null);
 
         FilterInvocation fi = createFilterInvocation("/someAdminPage.html?a=/test", null);
 
-        List<? extends ConfigAttribute> response = map.lookupAttributes(fi.getRequestUrl());
+        List<? extends ConfigAttribute> response = fids.lookupAttributes(fi.getRequestUrl(), null);
         assertEquals(def, response); // see SEC-161 (it should truncate after ? sign)
     }
 
     @Test(expected = IllegalArgumentException.class)
     public void unknownHttpMethodIsRejected() {
-        map.addSecureUrl("/someAdminPage.html**", "UNKNOWN", def);
+        createFids("/someAdminPage.html**", "UNKNOWN");
     }
 
     @Test
     public void httpMethodLookupSucceeds() {
-        map.addSecureUrl("/somepage**", "GET", def);
+        createFids("/somepage**", "GET");
 
         FilterInvocation fi = createFilterInvocation("/somepage", "GET");
-        List<? extends ConfigAttribute> attrs = map.getAttributes(fi);
+        List<? extends ConfigAttribute> attrs = fids.getAttributes(fi);
         assertEquals(def, attrs);
     }
 
+    @Test
+    public void generalMatchIsUsedIfNoMethodSpecificMatchExists() {
+        createFids("/somepage**", null);
+
+        FilterInvocation fi = createFilterInvocation("/somepage", "GET");
+        List<? extends ConfigAttribute> attrs = fids.getAttributes(fi);
+        assertEquals(def, attrs);
+    }
+    
     @Test
     public void requestWithDifferentHttpMethodDoesntMatch() {
-        map.addSecureUrl("/somepage**", "GET", def);
+        createFids("/somepage**", "GET");
 
         FilterInvocation fi = createFilterInvocation("/somepage", null);
-        List<? extends ConfigAttribute> attrs = map.getAttributes(fi);
+        List<? extends ConfigAttribute> attrs = fids.getAttributes(fi);
         assertNull(attrs);
     }
 
     @Test
     public void httpMethodSpecificUrlTakesPrecedence() {
-        // Even though this is added before the method-specific def, the latter should match
-        List<ConfigAttribute> allMethodDef = def;
-        map.addSecureUrl("/**", null, allMethodDef);
-
+        LinkedHashMap<RequestKey, List<ConfigAttribute>> requestMap = new LinkedHashMap<RequestKey, List<ConfigAttribute>>();
+        // Even though this is added before the Http method-specific def, the latter should match
+        requestMap.put(new RequestKey("/**"), def);
         List<ConfigAttribute> postOnlyDef = SecurityConfig.createList("ROLE_TWO");
-        map.addSecureUrl("/somepage**", "POST", postOnlyDef);
+        requestMap.put(new RequestKey("/somepage**", "POST"), postOnlyDef);
+        fids = new DefaultFilterInvocationDefinitionSource(new AntUrlPathMatcher(), requestMap);
 
-        FilterInvocation fi = createFilterInvocation("/somepage", "POST");
-        List<ConfigAttribute> attrs = map.getAttributes(fi);
+        List<ConfigAttribute> attrs = fids.getAttributes(createFilterInvocation("/somepage", "POST"));
         assertEquals(postOnlyDef, attrs);
     }
 
@@ -150,16 +165,16 @@ public class DefaultFilterInvocationDefinitionSourceTests {
      */
     @Test
     public void extraQuestionMarkStillMatches() {
-        map.addSecureUrl("/someAdminPage.html*", def);
+        createFids("/someAdminPage.html*", null);
 
         FilterInvocation fi = createFilterInvocation("/someAdminPage.html?x=2/aa?y=3", null);
 
-        List<? extends ConfigAttribute> response = map.lookupAttributes(fi.getRequestUrl());
+        List<? extends ConfigAttribute> response = fids.lookupAttributes(fi.getRequestUrl(), null);
         assertEquals(def, response);
 
         fi = createFilterInvocation("/someAdminPage.html??", null);
 
-        response = map.lookupAttributes(fi.getRequestUrl());
+        response = fids.lookupAttributes(fi.getRequestUrl(), null);
         assertEquals(def, response);
     }