Polishing FilterChainProxy and its tests.

config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java

@@ -73,43 +73,43 @@ public class FilterChainProxyConfigTests {
 
     @Test
     public void normalOperation() throws Exception {
-        FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("filterChain", FilterChainProxy.class);
+        FilterChainProxy filterChainProxy = appCtx.getBean("filterChain", FilterChainProxy.class);
         doNormalOperation(filterChainProxy);
     }
 
     @Test
     public void normalOperationWithNewConfig() throws Exception {
-        FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxy", FilterChainProxy.class);
+        FilterChainProxy filterChainProxy = appCtx.getBean("newFilterChainProxy", FilterChainProxy.class);
         checkPathAndFilterOrder(filterChainProxy);
         doNormalOperation(filterChainProxy);
     }
 
     @Test
     public void normalOperationWithNewConfigRegex() throws Exception {
-        FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyRegex", FilterChainProxy.class);
+        FilterChainProxy filterChainProxy = appCtx.getBean("newFilterChainProxyRegex", FilterChainProxy.class);
         checkPathAndFilterOrder(filterChainProxy);
         doNormalOperation(filterChainProxy);
     }
 
     @Test
     public void normalOperationWithNewConfigNonNamespace() throws Exception {
-        FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyNonNamespace", FilterChainProxy.class);
+        FilterChainProxy filterChainProxy = appCtx.getBean("newFilterChainProxyNonNamespace", FilterChainProxy.class);
         checkPathAndFilterOrder(filterChainProxy);
         doNormalOperation(filterChainProxy);
     }
 
     @Test
     public void pathWithNoMatchHasNoFilters() throws Exception {
-        FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("newFilterChainProxyNoDefaultPath", FilterChainProxy.class);
+        FilterChainProxy filterChainProxy = appCtx.getBean("newFilterChainProxyNoDefaultPath", FilterChainProxy.class);
         assertEquals(null, filterChainProxy.getFilters("/nomatch"));
     }
 
     // SEC-1235
     @Test
     public void mixingPatternsAndPlaceholdersDoesntCauseOrderingIssues() throws Exception {
-        FilterChainProxy filterChainProxy = (FilterChainProxy) appCtx.getBean("sec1235FilterChainProxy", FilterChainProxy.class);
+        FilterChainProxy fcp = appCtx.getBean("sec1235FilterChainProxy", FilterChainProxy.class);
 
-        RequestMatcher[] matchers = filterChainProxy.getFilterChainMap().keySet().toArray(new RequestMatcher[0]);
+        RequestMatcher[] matchers = fcp.getFilterChainMap().keySet().toArray(new RequestMatcher[fcp.getFilterChainMap().keySet().size()]);
         assertEquals("/login*", ((AntPathRequestMatcher)matchers[0]).getPattern());
         assertEquals("/logout", ((AntPathRequestMatcher)matchers[1]).getPattern());
         assertTrue(matchers[2] instanceof AnyRequestMatcher);

web/src/main/java/org/springframework/security/web/FilterChainProxy.java

@@ -15,22 +15,6 @@
 
 package org.springframework.security.web;
 
-import java.io.IOException;
-import java.util.Collection;
-import java.util.Iterator;
-import java.util.LinkedHashMap;
-import java.util.LinkedHashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-
-import javax.servlet.Filter;
-import javax.servlet.FilterChain;
-import javax.servlet.ServletException;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.HttpServletRequest;
-
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.security.web.util.AnyRequestMatcher;
@@ -39,6 +23,11 @@ import org.springframework.util.Assert;
 import org.springframework.web.filter.DelegatingFilterProxy;
 import org.springframework.web.filter.GenericFilterBean;
 
+import javax.servlet.*;
+import javax.servlet.http.HttpServletRequest;
+import java.io.IOException;
+import java.util.*;
+
 
 /**
  * Delegates <code>Filter</code> requests to a list of Spring-managed beans.
@@ -96,7 +85,6 @@ public class FilterChainProxy extends GenericFilterBean {
     //~ Static fields/initializers =====================================================================================
 
     private static final Log logger = LogFactory.getLog(FilterChainProxy.class);
-    public static final String TOKEN_NONE = "#NONE#";
 
     //~ Instance fields ================================================================================================
 
@@ -121,7 +109,7 @@ public class FilterChainProxy extends GenericFilterBean {
         if (filters == null || filters.size() == 0) {
             if (logger.isDebugEnabled()) {
                 logger.debug(fi.getRequestUrl() +
-                        filters == null ? " has no matching filters" : " has an empty filter list");
+                        (filters == null ? " has no matching filters" : " has an empty filter list"));
             }
 
             chain.doFilter(request, response);
@@ -162,26 +150,6 @@ public class FilterChainProxy extends GenericFilterBean {
         return getFilters(new FilterInvocation(url, null).getRequest());
     }
 
-    /**
-     * Obtains all of the <b>unique</b><code>Filter</code> instances registered in the map of
-     * filter chains.
-     * <p>This is useful in ensuring a <code>Filter</code> is not initialized or destroyed twice.</p>
-     *
-     * @return all of the <code>Filter</code> instances in the application context which have an entry
-     *         in the map (only one entry is included in the array for
-     *         each <code>Filter</code> that actually exists in application context, even if a given
-     *         <code>Filter</code> is defined multiples times in the filter chain map)
-     */
-    protected Collection<Filter> obtainAllDefinedFilters() {
-        Set<Filter> allFilters = new LinkedHashSet<Filter>();
-
-        for (List<Filter> filters : filterChainMap.values()) {
-            allFilters.addAll(filters);
-        }
-
-        return allFilters;
-    }
-
     /**
      * Sets the mapping of URL patterns to filter chains.
      *
@@ -217,10 +185,10 @@ public class FilterChainProxy extends GenericFilterBean {
 
     private void checkPathOrder() {
         // Check that the universal pattern is listed at the end, if at all
-        RequestMatcher[] matchers = filterChainMap.keySet().toArray(new RequestMatcher[0]);
+        Iterator<RequestMatcher> matchers = filterChainMap.keySet().iterator();
 
-        for (int i=0; i < matchers.length-1; i++) {
+        while(matchers.hasNext()) {
-            if (matchers[i] instanceof AnyRequestMatcher) {
+            if ((matchers.next() instanceof AnyRequestMatcher && matchers.hasNext())) {
                 throw new IllegalArgumentException("A universal match pattern ('/**') is defined " +
                         " before other patterns in the filter chain, causing them to be ignored. Please check the " +
                         "ordering in your <security:http> namespace or FilterChainProxy bean configuration");
@@ -241,7 +209,8 @@ public class FilterChainProxy extends GenericFilterBean {
     /**
      * Used (internally) to specify a validation strategy for the filters in each configured chain.
      *
-     * @param filterChainValidator
+     * @param filterChainValidator the validator instance which will be invoked on during initialization
+     * to check the {@code FilterChainProxy} instance.
      */
     public void setFilterChainValidator(FilterChainValidator filterChainValidator) {
         this.filterChainValidator = filterChainValidator;
@@ -260,24 +229,25 @@ public class FilterChainProxy extends GenericFilterBean {
     //~ Inner Classes ==================================================================================================
 
     /**
-     * A <code>FilterChain</code> that records whether or not {@link
+     * Internal {@code FilterChain} implementation that is used to pass a request through the additional
-     * FilterChain#doFilter(javax.servlet.ServletRequest, javax.servlet.ServletResponse)} is called.
+     * internal list of filters which match the request. Records the position in the additional filter chain and, when
-     * <p>
+     * completed, passes the request back to the original {@code FilterChain} supplied by the servlet container.
-     * This <code>FilterChain</code> is used by <code>FilterChainProxy</code> to determine if the next
-     * <code>Filter</code> should be called or not.</p>
      */
     private static class VirtualFilterChain implements FilterChain {
         private final FilterInvocation fi;
         private final List<Filter> additionalFilters;
+        private final int size;
         private int currentPosition = 0;
 
+
         private VirtualFilterChain(FilterInvocation filterInvocation, List<Filter> additionalFilters) {
             this.fi = filterInvocation;
             this.additionalFilters = additionalFilters;
+            this.size = additionalFilters.size();
         }
 
         public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException {
-            if (currentPosition == additionalFilters.size()) {
+            if (currentPosition == size) {
                 if (logger.isDebugEnabled()) {
                     logger.debug(fi.getRequestUrl()
                         + " reached end of additional filter chain; proceeding with original chain");
@@ -291,7 +261,7 @@ public class FilterChainProxy extends GenericFilterBean {
 
                 if (logger.isDebugEnabled()) {
                     logger.debug(fi.getRequestUrl() + " at position " + currentPosition + " of "
-                        + additionalFilters.size() + " in additional filter chain; firing Filter: '"
+                        + size + " in additional filter chain; firing Filter: '"
                         + nextFilter + "'");
                 }