Consolidate CORS/OPTIONS request mapping

The CORS pre-flight request matching logic for all request conditions
was added (in 4.2) to RequestMappingInfo. However the logic for
default handling of all HTTP OPTIONS requests for 4.3 unintentionally
overrode some of the pre-flight request handling thus causing issues.

This commit moves CORS pre-flight matching logic into each respective
RequestMethodCondition implementations so each has to consider in one
place what happens for pre-flight and for all other requests.

Issue: SPR-13130

Author: rstoyanchev

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/ConsumesRequestCondition.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -30,6 +30,7 @@
 import org.springframework.util.StringUtils;
 import org.springframework.web.HttpMediaTypeNotSupportedException;
 import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.cors.CorsUtils;
 import org.springframework.web.servlet.mvc.condition.HeadersRequestCondition.HeaderExpression;
 
 /**
@@ -46,6 +47,9 @@
  */
 public final class ConsumesRequestCondition extends AbstractRequestCondition<ConsumesRequestCondition> {
 
+	private final static ConsumesRequestCondition PRE_FLIGHT_MATCH = new ConsumesRequestCondition();
+
+
 	private final List<ConsumeMediaTypeExpression> expressions;
 
 
@@ -160,6 +164,9 @@ public ConsumesRequestCondition combine(ConsumesRequestCondition other) {
 	 */
 	@Override
 	public ConsumesRequestCondition getMatchingCondition(HttpServletRequest request) {
+		if (CorsUtils.isPreFlightRequest(request)) {
+			return PRE_FLIGHT_MATCH;
+		}
 		if (isEmpty()) {
 			return this;
 		}

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/HeadersRequestCondition.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -23,6 +23,7 @@
 import javax.servlet.http.HttpServletRequest;
 
 import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.cors.CorsUtils;
 
 /**
  * A logical conjunction (' && ') request condition that matches a request against
@@ -38,6 +39,9 @@
  */
 public final class HeadersRequestCondition extends AbstractRequestCondition<HeadersRequestCondition> {
 
+	private final static HeadersRequestCondition PRE_FLIGHT_MATCH = new HeadersRequestCondition();
+
+
 	private final Set<HeaderExpression> expressions;
 
 
@@ -105,6 +109,9 @@ public HeadersRequestCondition combine(HeadersRequestCondition other) {
 	 */
 	@Override
 	public HeadersRequestCondition getMatchingCondition(HttpServletRequest request) {
+		if (CorsUtils.isPreFlightRequest(request)) {
+			return PRE_FLIGHT_MATCH;
+		}
 		for (HeaderExpression expression : expressions) {
 			if (!expression.match(request)) {
 				return null;

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/ProducesRequestCondition.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -30,6 +30,7 @@
 import org.springframework.web.accept.ContentNegotiationManager;
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.context.request.ServletWebRequest;
+import org.springframework.web.cors.CorsUtils;
 import org.springframework.web.servlet.mvc.condition.HeadersRequestCondition.HeaderExpression;
 
 /**
@@ -45,6 +46,9 @@
  */
 public final class ProducesRequestCondition extends AbstractRequestCondition<ProducesRequestCondition> {
 
+	private final static ProducesRequestCondition PRE_FLIGHT_MATCH = new ProducesRequestCondition();
+
+
 	private final List<ProduceMediaTypeExpression> MEDIA_TYPE_ALL_LIST =
 			Collections.singletonList(new ProduceMediaTypeExpression("*/*"));
 
@@ -176,6 +180,9 @@ public ProducesRequestCondition combine(ProducesRequestCondition other) {
 	 */
 	@Override
 	public ProducesRequestCondition getMatchingCondition(HttpServletRequest request) {
+		if (CorsUtils.isPreFlightRequest(request)) {
+			return PRE_FLIGHT_MATCH;
+		}
 		if (isEmpty()) {
 			return this;
 		}

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/RequestCondition.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,46 +18,47 @@
 
 import javax.servlet.http.HttpServletRequest;
 
-import org.springframework.web.bind.annotation.RequestMapping;
-
 /**
- * The contract for request conditions in Spring MVC's mapping infrastructure.
+ * Contract for request mapping conditions.
  *
  * <p>Request conditions can be combined via {@link #combine(Object)}, matched to
  * a request via {@link #getMatchingCondition(HttpServletRequest)}, and compared
  * to each other via {@link #compareTo(Object, HttpServletRequest)} to determine
- * which matches a request more closely.
+ * which is a closer match for a given request.
  *
  * @author Rossen Stoyanchev
  * @author Arjen Poutsma
  * @since 3.1
- * @param <T> the type of objects that this RequestCondition can be combined with and compared to
+ * @param <T> the type of objects that this RequestCondition can be combined
+ * with and compared to
  */
 public interface RequestCondition<T> {
 
 	/**
-	 * Defines the rules for combining this condition (i.e. the current instance)
-	 * with another condition. For example combining type- and method-level
-	 * {@link RequestMapping} conditions.
+	 * Combine this condition with another such as conditions from a
+	 * type-level and method-level {@code @RequestMapping} annotation.
 	 * @param other the condition to combine with.
 	 * @return a request condition instance that is the result of combining
 	 * the two condition instances.
 	 */
 	T combine(T other);
 
 	/**
-	 * Checks if this condition matches the given request and returns a
-	 * potentially new request condition with content tailored to the
-	 * current request. For example a condition with URL patterns might
-	 * return a new condition that contains matching patterns sorted
-	 * with best matching patterns on top.
-	 * @return a condition instance in case of a match;
-	 * or {@code null} if there is no match
+	 * Check if the condition matches the request returning a potentially new
+	 * instance created for the current request. For example a condition with
+	 * multiple URL patterns may return a new instance only with those patterns
+	 * that match the request.
+	 * <p>For CORS pre-flight requests, conditions should match to the would-be,
+	 * actual request (e.g. URL pattern, query parameters, and the HTTP method
+	 * from the "Access-Control-Request-Method" header). If a condition cannot
+	 * be matched to a pre-flight request it should return an instance with
+	 * empty content thus not causing a failure to match.
+	 * @return a condition instance in case of a match or {@code null} otherwise.
 	 */
 	T getMatchingCondition(HttpServletRequest request);
 
 	/**
-	 * Compares this condition to another condition in the context of
+	 * Compare this condition to another condition in the context of
 	 * a specific request. This method assumes both instances have
 	 * been obtained via {@link #getMatchingCondition(HttpServletRequest)}
 	 * to ensure they have content relevant to current request only.

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java

@@ -24,7 +24,9 @@
 import java.util.Set;
 import javax.servlet.http.HttpServletRequest;
 
+import org.springframework.http.HttpHeaders;
 import org.springframework.web.bind.annotation.RequestMethod;
+import org.springframework.web.cors.CorsUtils;
 
 /**
  * A logical disjunction (' || ') request condition that matches a request
@@ -101,12 +103,38 @@ public RequestMethodsRequestCondition combine(RequestMethodsRequestCondition oth
 	 */
 	@Override
 	public RequestMethodsRequestCondition getMatchingCondition(HttpServletRequest request) {
-		RequestMethod requestMethod = getRequestMethod(request);
-		if (this.methods.isEmpty()) {
-			return (RequestMethod.OPTIONS.equals(requestMethod) ? null : this);
+
+		if (CorsUtils.isPreFlightRequest(request)) {
+			return matchPreFlight(request);
+		}
+
+		if (getMethods().isEmpty()) {
+			if (RequestMethod.OPTIONS.name().equals(request.getMethod())) {
+				return null; // No implicit match for OPTIONS (we handle it)
+			}
+			return this;
+		}
+
+		return matchRequestMethod(request.getMethod());
+	}
+
+	/**
+	 * On a pre-flight request match to the would-be, actual request.
+	 * Hence empty conditions is a match, otherwise try to match to the HTTP
+	 * method in the "Access-Control-Request-Method" header.
+	 */
+	private RequestMethodsRequestCondition matchPreFlight(HttpServletRequest request) {
+		if (getMethods().isEmpty()) {
+			return this;
 		}
+		String expectedMethod = request.getHeader(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD);
+		return matchRequestMethod(expectedMethod);
+	}
+
+	private RequestMethodsRequestCondition matchRequestMethod(String httpMethod) {
+		RequestMethod requestMethod = getRequestMethod(httpMethod);
 		if (requestMethod != null) {
-			for (RequestMethod method : this.methods) {
+			for (RequestMethod method : getMethods()) {
 				if (method.equals(requestMethod)) {
 					return new RequestMethodsRequestCondition(method);
 				}
@@ -118,9 +146,9 @@ public RequestMethodsRequestCondition getMatchingCondition(HttpServletRequest re
 		return null;
 	}
 
-	private RequestMethod getRequestMethod(HttpServletRequest request) {
+	private RequestMethod getRequestMethod(String httpMethod) {
 		try {
-			return RequestMethod.valueOf(request.getMethod());
+			return RequestMethod.valueOf(httpMethod);
 		}
 		catch (IllegalArgumentException ex) {
 			return null;

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java

@@ -215,15 +215,7 @@ public RequestMappingInfo getMatchingCondition(HttpServletRequest request) {
 		ProducesRequestCondition produces = this.producesCondition.getMatchingCondition(request);
 
 		if (methods == null || params == null || headers == null || consumes == null || produces == null) {
-			if (CorsUtils.isPreFlightRequest(request)) {
-				methods = getAccessControlRequestMethodCondition(request);
-				if (methods == null || params == null) {
-					return null;
-				}
-			}
-			else {
-				return null;
-			}
+			return null;
 		}
 
 		PatternsRequestCondition patterns = this.patternsCondition.getMatchingCondition(request);
@@ -240,22 +232,6 @@ public RequestMappingInfo getMatchingCondition(HttpServletRequest request) {
 				methods, params, headers, consumes, produces, custom.getCondition());
 	}
 
-	/**
-	 * Return a matching RequestMethodsRequestCondition based on the expected
-	 * HTTP method specified in a CORS pre-flight request.
-	 */
-	private RequestMethodsRequestCondition getAccessControlRequestMethodCondition(HttpServletRequest request) {
-		String expectedMethod = request.getHeader(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD);
-		if (StringUtils.hasText(expectedMethod)) {
-			for (RequestMethod method : getMethodsCondition().getMethods()) {
-				if (expectedMethod.equalsIgnoreCase(method.name())) {
-					return new RequestMethodsRequestCondition(method);
-				}
-			}
-		}
-		return null;
-	}
-
 	/**
 	 * Compares "this" info (i.e. the current instance) with another info in the context of a request.
 	 * <p>Note: It is assumed both instances have been obtained via

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestConditionTests.java

@@ -22,17 +22,20 @@
 
 import org.junit.Test;
 
+import org.springframework.http.HttpHeaders;
 import org.springframework.mock.web.test.MockHttpServletRequest;
 import org.springframework.web.bind.annotation.RequestMethod;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.springframework.web.bind.annotation.RequestMethod.DELETE;
 import static org.springframework.web.bind.annotation.RequestMethod.GET;
 import static org.springframework.web.bind.annotation.RequestMethod.HEAD;
 import static org.springframework.web.bind.annotation.RequestMethod.OPTIONS;
 import static org.springframework.web.bind.annotation.RequestMethod.POST;
+import static org.springframework.web.bind.annotation.RequestMethod.PUT;
 
 /**
  * @author Arjen Poutsma
@@ -73,6 +76,17 @@ public void getMatchingConditionWithCustomMethod() {
 		assertNull(new RequestMethodsRequestCondition(GET, POST).getMatchingCondition(request));
 	}
 
+	@Test
+	public void getMatchingConditionWithCorsPreFlight() throws Exception {
+		MockHttpServletRequest request = new MockHttpServletRequest("OPTIONS", "");
+		request.addHeader("Origin", "http://example.com");
+		request.addHeader(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "PUT");
+
+		assertNotNull(new RequestMethodsRequestCondition().getMatchingCondition(request));
+		assertNotNull(new RequestMethodsRequestCondition(PUT).getMatchingCondition(request));
+		assertNull(new RequestMethodsRequestCondition(DELETE).getMatchingCondition(request));
+	}
+
 	@Test
 	public void compareTo() {
 		RequestMethodsRequestCondition c1 = new RequestMethodsRequestCondition(GET, HEAD);

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -332,7 +332,7 @@ public void preFlightRequest() {
 				new PatternsRequestCondition("/foo"), new RequestMethodsRequestCondition(RequestMethod.OPTIONS), null,
 				null, null, null, null);
 		match = info.getMatchingCondition(request);
-		assertNotNull(match);
+		assertNull("Pre-flight should match the ACCESS_CONTROL_REQUEST_METHOD", match);
 	}
 
 }