Polish PathPattern Support

Author: jzheaux

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

@@ -46,6 +46,7 @@
 import org.springframework.util.Assert;
 import org.springframework.web.filter.DelegatingFilterProxy;
 import org.springframework.web.filter.GenericFilterBean;
+import org.springframework.web.filter.ServletRequestPathFilter;
 
 /**
  * Delegates {@code Filter} requests to a list of Spring-managed filter beans. As of
@@ -162,6 +163,8 @@ public class FilterChainProxy extends GenericFilterBean {
 
 	private FilterChainDecorator filterChainDecorator = new VirtualFilterChainDecorator();
 
+	private Filter springWebFilter = new ServletRequestPathFilter();
+
 	public FilterChainProxy() {
 	}
 
@@ -210,27 +213,29 @@ private void doFilterInternal(ServletRequest request, ServletResponse response,
 			throws IOException, ServletException {
 		FirewalledRequest firewallRequest = this.firewall.getFirewalledRequest((HttpServletRequest) request);
 		HttpServletResponse firewallResponse = this.firewall.getFirewalledResponse((HttpServletResponse) response);
-		List<Filter> filters = getFilters(firewallRequest);
-		if (filters == null || filters.isEmpty()) {
-			if (logger.isTraceEnabled()) {
-				logger.trace(LogMessage.of(() -> "No security for " + requestLine(firewallRequest)));
+		this.springWebFilter.doFilter(firewallRequest, firewallResponse, (r, s) -> {
+			List<Filter> filters = getFilters(firewallRequest);
+			if (filters == null || filters.isEmpty()) {
+				if (logger.isTraceEnabled()) {
+					logger.trace(LogMessage.of(() -> "No security for " + requestLine(firewallRequest)));
+				}
+				firewallRequest.reset();
+				this.filterChainDecorator.decorate(chain).doFilter(firewallRequest, firewallResponse);
+				return;
 			}
-			firewallRequest.reset();
-			this.filterChainDecorator.decorate(chain).doFilter(firewallRequest, firewallResponse);
-			return;
-		}
-		if (logger.isDebugEnabled()) {
-			logger.debug(LogMessage.of(() -> "Securing " + requestLine(firewallRequest)));
-		}
-		FilterChain reset = (req, res) -> {
 			if (logger.isDebugEnabled()) {
-				logger.debug(LogMessage.of(() -> "Secured " + requestLine(firewallRequest)));
+				logger.debug(LogMessage.of(() -> "Securing " + requestLine(firewallRequest)));
 			}
-			// Deactivate path stripping as we exit the security filter chain
-			firewallRequest.reset();
-			chain.doFilter(req, res);
-		};
-		this.filterChainDecorator.decorate(reset, filters).doFilter(firewallRequest, firewallResponse);
+			FilterChain reset = (req, res) -> {
+				if (logger.isDebugEnabled()) {
+					logger.debug(LogMessage.of(() -> "Secured " + requestLine(firewallRequest)));
+				}
+				// Deactivate path stripping as we exit the security filter chain
+				firewallRequest.reset();
+				chain.doFilter(req, res);
+			};
+			this.filterChainDecorator.decorate(reset, filters).doFilter(firewallRequest, firewallResponse);
+		});
 	}
 
 	/**
@@ -447,4 +452,23 @@ public FilterChain decorate(FilterChain original, List<Filter> filters) {
 
 	}
 
+	private static final class FirewallFilter implements Filter {
+
+		private final HttpFirewall firewall;
+
+		private FirewallFilter(HttpFirewall firewall) {
+			this.firewall = firewall;
+		}
+
+		@Override
+		public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+				throws IOException, ServletException {
+			HttpServletRequest request = (HttpServletRequest) servletRequest;
+			HttpServletResponse response = (HttpServletResponse) servletResponse;
+			filterChain.doFilter(this.firewall.getFirewalledRequest(request),
+					this.firewall.getFirewalledResponse(response));
+		}
+
+	}
+
 }

web/src/main/java/org/springframework/security/web/servlet/util/matcher/PathPatternRequestMatcher.java

@@ -32,7 +32,6 @@
 import org.springframework.lang.Nullable;
 import org.springframework.security.web.access.intercept.RequestAuthorizationContext;
 import org.springframework.security.web.util.matcher.AnyRequestMatcher;
-import org.springframework.security.web.util.matcher.MethodPathRequestMatcherFactory;
 import org.springframework.security.web.util.matcher.RequestMatcher;
 import org.springframework.util.Assert;
 import org.springframework.web.util.ServletRequestPathUtils;
@@ -154,7 +153,7 @@ void setServletPath(RequestMatcher servletPath) {
 	}
 
 	private RequestPath getRequestPath(HttpServletRequest request) {
-		return ServletRequestPathUtils.parseAndCache(request);
+		return ServletRequestPathUtils.getParsedRequestPath(request);
 	}
 
 	/**
@@ -209,7 +208,7 @@ public String toString() {
 	 *             ...
 	 * </code>
 	 */
-	public static final class Builder implements MethodPathRequestMatcherFactory {
+	public static final class Builder {
 
 		private final PathPatternParser parser;
 
@@ -234,6 +233,40 @@ public Builder servletPath(String servletPath) {
 			return this;
 		}
 
+		/**
+		 * Match requests having this path pattern.
+		 *
+		 * <p>
+		 * When the HTTP {@code method} is null, then the matcher does not consider the
+		 * HTTP method
+		 *
+		 * <p>
+		 * Path patterns always start with a slash and may contain placeholders. They can
+		 * also be followed by {@code /**} to signify all URIs under a given path.
+		 *
+		 * <p>
+		 * These must be specified relative to any servlet path prefix (meaning you should
+		 * exclude the context path and any servlet path prefix in stating your pattern).
+		 *
+		 * <p>
+		 * The following are valid patterns and their meaning
+		 * <ul>
+		 * <li>{@code /path} - match exactly and only `/path`</li>
+		 * <li>{@code /path/**} - match `/path` and any of its descendents</li>
+		 * <li>{@code /path/{value}/**} - match `/path/subdirectory` and any of its
+		 * descendents, capturing the value of the subdirectory in
+		 * {@link RequestAuthorizationContext#getVariables()}</li>
+		 * </ul>
+		 *
+		 * <p>
+		 * A more comprehensive list can be found at {@link PathPattern}.
+		 * @param path the path pattern to match
+		 * @return the {@link Builder} for more configuration
+		 */
+		public PathPatternRequestMatcher matcher(String path) {
+			return matcher(null, path);
+		}
+
 		/**
 		 * Match requests having this {@link HttpMethod} and path pattern.
 		 *

web/src/main/java/org/springframework/security/web/util/matcher/MethodPathRequestMatcherFactory.java

@@ -48,6 +48,7 @@
 import org.springframework.security.web.firewall.HttpFirewall;
 import org.springframework.security.web.firewall.RequestRejectedException;
 import org.springframework.security.web.firewall.RequestRejectedHandler;
+import org.springframework.security.web.servlet.TestMockHttpServletMappings;
 import org.springframework.security.web.util.matcher.RequestMatcher;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -166,6 +167,7 @@ public void wrapperIsResetWhenNoMatchingFilters() throws Exception {
 		FirewalledRequest fwr = mock(FirewalledRequest.class);
 		given(fwr.getRequestURI()).willReturn("/");
 		given(fwr.getContextPath()).willReturn("");
+		given(fwr.getHttpServletMapping()).willReturn(TestMockHttpServletMappings.defaultMapping());
 		this.fcp.setFirewall(fw);
 		given(fw.getFirewalledRequest(this.request)).willReturn(fwr);
 		given(this.matcher.matches(any(HttpServletRequest.class))).willReturn(false);
@@ -183,9 +185,11 @@ public void bothWrappersAreResetWithNestedFcps() throws Exception {
 		FirewalledRequest firstFwr = mock(FirewalledRequest.class, "firstFwr");
 		given(firstFwr.getRequestURI()).willReturn("/");
 		given(firstFwr.getContextPath()).willReturn("");
+		given(firstFwr.getHttpServletMapping()).willReturn(TestMockHttpServletMappings.defaultMapping());
 		FirewalledRequest fwr = mock(FirewalledRequest.class, "fwr");
 		given(fwr.getRequestURI()).willReturn("/");
 		given(fwr.getContextPath()).willReturn("");
+		given(fwr.getHttpServletMapping()).willReturn(TestMockHttpServletMappings.defaultMapping());
 		given(fw.getFirewalledRequest(this.request)).willReturn(firstFwr);
 		given(fw.getFirewalledRequest(firstFwr)).willReturn(fwr);
 		given(fwr.getRequest()).willReturn(firstFwr);

web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java

@@ -90,18 +90,25 @@ void matcherWhenServletPathThenMatchesOnlyServletPath() {
 		PathPatternRequestMatcher.Builder servlet = PathPatternRequestMatcher.servletPath("/servlet/path");
 		RequestMatcher matcher = servlet.matcher(HttpMethod.GET, "/endpoint");
 		ServletContext servletContext = servletContext("/servlet/path");
-		assertThat(matcher
-			.matches(get("/servlet/path/endpoint").servletPath("/servlet/path").buildRequest(servletContext))).isTrue();
-		assertThat(matcher.matches(get("/endpoint").servletPath("/endpoint").buildRequest(servletContext))).isFalse();
+		MockHttpServletRequest mock = get("/servlet/path/endpoint").servletPath("/servlet/path")
+			.buildRequest(servletContext);
+		ServletRequestPathUtils.parseAndCache(mock);
+		assertThat(matcher.matches(mock)).isTrue();
+		mock = get("/endpoint").servletPath("/endpoint").buildRequest(servletContext);
+		ServletRequestPathUtils.parseAndCache(mock);
+		assertThat(matcher.matches(mock)).isFalse();
 	}
 
 	@Test
 	void matcherWhenRequestPathThenIgnoresServletPath() {
 		PathPatternRequestMatcher.Builder request = PathPatternRequestMatcher.path();
 		RequestMatcher matcher = request.matcher(HttpMethod.GET, "/endpoint");
-		assertThat(matcher.matches(get("/servlet/path/endpoint").servletPath("/servlet/path").buildRequest(null)))
-			.isTrue();
-		assertThat(matcher.matches(get("/endpoint").servletPath("/endpoint").buildRequest(null))).isTrue();
+		MockHttpServletRequest mock = get("/servlet/path/endpoint").servletPath("/servlet/path").buildRequest(null);
+		ServletRequestPathUtils.parseAndCache(mock);
+		assertThat(matcher.matches(mock)).isTrue();
+		mock = get("/endpoint").servletPath("/endpoint").buildRequest(null);
+		ServletRequestPathUtils.parseAndCache(mock);
+		assertThat(matcher.matches(mock)).isTrue();
 	}
 
 	@Test