From f32aded27aeec51ec110d4aa6a21ab37e44f6db1 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 19 Feb 2016 13:47:48 -0500 Subject: [PATCH] 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 --- .../condition/ConsumesRequestCondition.java | 9 ++++- .../condition/HeadersRequestCondition.java | 9 ++++- .../condition/ProducesRequestCondition.java | 9 ++++- .../mvc/condition/RequestCondition.java | 35 ++++++++-------- .../RequestMethodsRequestCondition.java | 40 ++++++++++++++++--- .../mvc/method/RequestMappingInfo.java | 26 +----------- .../RequestMethodsRequestConditionTests.java | 14 +++++++ .../mvc/method/RequestMappingInfoTests.java | 4 +- 8 files changed, 93 insertions(+), 53 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/ConsumesRequestCondition.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/ConsumesRequestCondition.java index f53e34b9e2..579ba1af35 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/ConsumesRequestCondition.java +++ b/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.http.MediaType; 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 @@ import org.springframework.web.servlet.mvc.condition.HeadersRequestCondition.Hea */ public final class ConsumesRequestCondition extends AbstractRequestCondition { + private final static ConsumesRequestCondition PRE_FLIGHT_MATCH = new ConsumesRequestCondition(); + + private final List expressions; @@ -160,6 +164,9 @@ public final class ConsumesRequestCondition extends AbstractRequestCondition { + private final static HeadersRequestCondition PRE_FLIGHT_MATCH = new HeadersRequestCondition(); + + private final Set expressions; @@ -105,6 +109,9 @@ public final class HeadersRequestCondition extends AbstractRequestCondition { + private final static ProducesRequestCondition PRE_FLIGHT_MATCH = new ProducesRequestCondition(); + + private final List MEDIA_TYPE_ALL_LIST = Collections.singletonList(new ProduceMediaTypeExpression("*/*")); @@ -176,6 +180,9 @@ public final class ProducesRequestCondition extends AbstractRequestConditionRequest 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 the type of objects that this RequestCondition can be combined with and compared to + * @param the type of objects that this RequestCondition can be combined + * with and compared to */ public interface RequestCondition { /** - * 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. @@ -46,18 +44,21 @@ public interface RequestCondition { 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. + *

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. diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java index b5ef6cb67b..d174251ccb 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java @@ -24,7 +24,9 @@ import java.util.List; 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 final class RequestMethodsRequestCondition extends AbstractRequestConditi */ @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 final class RequestMethodsRequestCondition extends AbstractRequestConditi 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; diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java index 395ecb9cc3..7374f3d503 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java @@ -215,15 +215,7 @@ public final class RequestMappingInfo implements RequestConditionNote: It is assumed both instances have been obtained via diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestConditionTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestConditionTests.java index ac51a5fae4..10503065c2 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestConditionTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestConditionTests.java @@ -22,6 +22,7 @@ import javax.servlet.http.HttpServletRequest; import org.junit.Test; +import org.springframework.http.HttpHeaders; import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.web.bind.annotation.RequestMethod; @@ -29,10 +30,12 @@ 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 class RequestMethodsRequestConditionTests { 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); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java index 29661840ce..ea95c1d385 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java +++ b/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 class RequestMappingInfoTests { 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); } }