diff --git a/core/src/main/java/org/springframework/security/intercept/web/DefaultFilterInvocationDefinitionSource.java b/core/src/main/java/org/springframework/security/intercept/web/DefaultFilterInvocationDefinitionSource.java index 4c26bb5c94..b7b95f6a0a 100644 --- a/core/src/main/java/org/springframework/security/intercept/web/DefaultFilterInvocationDefinitionSource.java +++ b/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 Lists - * TODO: Store in the httpMethod map with null key. - */ - private Map> requestMap = new LinkedHashMap>(); - /** Stores request maps keyed by specific HTTP methods */ + //private Map> requestMap = new LinkedHashMap>(); + /** Stores request maps keyed by specific HTTP methods. A null key matches any method */ private Map>> httpMethodMap = new HashMap>>(); @@ -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 attr) { - addSecureUrl(pattern, null, attr); - } - /** * Adds a URL,attribute-list pair to the request map, first allowing the UrlMatcher to * process the pattern if required, using its compile method. The returned object will be used as the key * to the request map and will be passed back to the UrlMatcher when iterating through the map to find * a match for a particular URL. */ - void addSecureUrl(String pattern, String method, List attr) { + private void addSecureUrl(String pattern, String method, List attr) { Map> mapToUse = getRequestMapForHttpMethod(method); mapToUse.put(urlMatcher.compile(pattern), attr); @@ -124,28 +110,27 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation * @return map of URL patterns to ConfigAttributes for this method. */ private Map> 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> methodRequestmap = httpMethodMap.get(method); + Map> methodRequestMap = httpMethodMap.get(method); - if (methodRequestmap == null) { - methodRequestmap = new LinkedHashMap>(); - httpMethodMap.put(method, methodRequestmap); + if (methodRequestMap == null) { + methodRequestMap = new LinkedHashMap>(); + httpMethodMap.put(method, methodRequestMap); } - return methodRequestmap; + return methodRequestMap; } public Collection getAllConfigAttributes() { Set allAttributes = new HashSet(); - for(List attrs : requestMap.values()) { - allAttributes.addAll(attrs); + for (Map.Entry>> entry : httpMethodMap.entrySet()) { + for (List attrs : entry.getValue().values()) { + allAttributes.addAll(attrs); + } } return allAttributes; @@ -163,10 +148,6 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation return lookupAttributes(url, method); } - protected List lookupAttributes(String url) { - return lookupAttributes(url, null); - } - /** * Performs the actual lookup of the relevant ConfigAttributes for the given FilterInvocation. *

@@ -179,9 +160,9 @@ public class DefaultFilterInvocationDefinitionSource implements FilterInvocation * @param method the HTTP method (GET, POST, DELETE...). * * @return the ConfigAttributes that apply to the specified FilterInvocation - * or null if no match is foud + * or null if no match is found */ - public List lookupAttributes(String url, String method) { + public final List 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 attributes = null; - - Map> 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> 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 lookupUrlInMap(Map> requestMap, String url) { - - for (Map.Entry> entry : requestMap.entrySet()) { - Object p = entry.getKey(); - boolean matched = urlMatcher.pathMatchesUrl(entry.getKey(), url); + if (requestMap != null) { + for (Map.Entry> 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(); + } } } diff --git a/core/src/test/java/org/springframework/security/config/FilterInvocationDefinitionSourceParserTests.java b/core/src/test/java/org/springframework/security/config/FilterInvocationDefinitionSourceParserTests.java index 949a7c7735..2fa49fff70 100644 --- a/core/src/test/java/org/springframework/security/config/FilterInvocationDefinitionSourceParserTests.java +++ b/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 { ""); DefaultFilterInvocationDefinitionSource fids = (DefaultFilterInvocationDefinitionSource) appContext.getBean("fids"); List cad = fids.getAttributes(createFilterInvocation("/anything", "GET")); + assertNotNull(cad); assertTrue(cad.contains(new SecurityConfig("ROLE_A"))); } diff --git a/core/src/test/java/org/springframework/security/intercept/web/DefaultFilterInvocationDefinitionSourceTests.java b/core/src/test/java/org/springframework/security/intercept/web/DefaultFilterInvocationDefinitionSourceTests.java index f2951f8d9f..9b5a507113 100644 --- a/core/src/test/java/org/springframework/security/intercept/web/DefaultFilterInvocationDefinitionSourceTests.java +++ b/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 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 response = map.lookupAttributes(fi.getRequestUrl()); + List 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 response = map.lookupAttributes(fi.getRequestUrl()); + List 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 response = map.lookupAttributes(fi.getRequestUrl()); + List 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 response = map.lookupAttributes(fi.getRequestUrl()); + List 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 attrs = map.getAttributes(fi); + List attrs = fids.getAttributes(fi); assertEquals(def, attrs); } + @Test + public void generalMatchIsUsedIfNoMethodSpecificMatchExists() { + createFids("/somepage**", null); + + FilterInvocation fi = createFilterInvocation("/somepage", "GET"); + List 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 attrs = map.getAttributes(fi); + List 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 allMethodDef = def; - map.addSecureUrl("/**", null, allMethodDef); - + LinkedHashMap> requestMap = new LinkedHashMap>(); + // Even though this is added before the Http method-specific def, the latter should match + requestMap.put(new RequestKey("/**"), def); List 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 attrs = map.getAttributes(fi); + List 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 response = map.lookupAttributes(fi.getRequestUrl()); + List 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); }