From de9f27872efce108670003c62294c6619f3b1182 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Sat, 20 Jun 2015 16:12:18 +0200 Subject: [PATCH] Polish CORS support classes - Simplified "check" algorithms in CorsConfiguration - Improved robustness of setter methods in CorsConfiguration in order to avoid attempts to modify immutable lists - Improved CORS documentation and fixed typo - Introduced constants in CorsConfiguration - Removed auto-boxing in CorsRegistration --- .../web/cors/CorsConfiguration.java | 194 +++++++++++------- .../web/cors/CorsConfigurationTests.java | 22 +- .../config/annotation/CorsRegistration.java | 31 +-- .../config/annotation/CorsRegistry.java | 17 +- 4 files changed, 162 insertions(+), 102 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/cors/CorsConfiguration.java b/spring-web/src/main/java/org/springframework/web/cors/CorsConfiguration.java index a30dff93901..f4fc0e5219b 100644 --- a/spring-web/src/main/java/org/springframework/web/cors/CorsConfiguration.java +++ b/spring-web/src/main/java/org/springframework/web/cors/CorsConfiguration.java @@ -22,18 +22,31 @@ import java.util.Collections; import java.util.List; import org.springframework.http.HttpMethod; +import org.springframework.util.ObjectUtils; +import org.springframework.util.StringUtils; /** - * A container for CORS configuration also providing methods to check actual or - * or requested origin, HTTP method, and headers. + * A container for CORS configuration that also provides methods to check + * the actual or requested origin, HTTP methods, and headers. * * @author Sebastien Deleuze * @author Rossen Stoyanchev + * @author Sam Brannen * @since 4.2 - * @see CORS W3C recommandation + * @see CORS W3C recommendation */ public class CorsConfiguration { + /** + * Wildcard representing all origins, methods, or headers. + */ + public static final String ALL = "*"; + + /** + * Default maximum age (30 minutes). + */ + public static final Long DEFAULT_MAX_AGE = Long.valueOf(1800); + private List allowedOrigins; private List allowedMethods; @@ -48,13 +61,14 @@ public class CorsConfiguration { /** - * Default constructor. + * Construct a new, empty {@code CorsConfiguration} instance. */ public CorsConfiguration() { } /** - * Copy constructor. + * Construct a new {@code CorsConfiguration} instance by copying all + * values from the supplied {@code CorsConfiguration}. */ public CorsConfiguration(CorsConfiguration other) { this.allowedOrigins = other.allowedOrigins; @@ -66,10 +80,11 @@ public class CorsConfiguration { } /** - * Combine the specified {@link CorsConfiguration} with this one. - * Properties of this configuration are overridden only by non-null properties - * of the provided one. - * @return the combined {@link CorsConfiguration} + * Combine the supplied {@code CorsConfiguration} with this one. + *

Properties of this configuration are overridden by any non-null + * properties of the supplied one. + * @return the combined {@code CorsConfiguration} or {@code this} + * configuration if the supplied configuration is {@code null} */ public CorsConfiguration combine(CorsConfiguration other) { if (other == null) { @@ -95,7 +110,7 @@ public class CorsConfiguration { if (other == null) { return source; } - if (source == null || source.contains("*")) { + if (source == null || source.contains(ALL)) { return other; } List combined = new ArrayList(source); @@ -104,12 +119,12 @@ public class CorsConfiguration { } /** - * Configure origins to allow, e.g. "http://domain1.com". The special value - * "*" allows all domains. + * Set the origins to allow, e.g. {@code "http://domain1.com"}. + *

The special value {@code "*"} allows all domains. *

By default this is not set. */ - public void setAllowedOrigins(List origins) { - this.allowedOrigins = origins; + public void setAllowedOrigins(List allowedOrigins) { + this.allowedOrigins = (allowedOrigins == null ? null : new ArrayList(allowedOrigins)); } /** @@ -124,51 +139,72 @@ public class CorsConfiguration { /** * Return the configured origins to allow, possibly {@code null}. + * @see #addAllowedOrigin(String) + * @see #setAllowedOrigins(List) */ public List getAllowedOrigins() { return this.allowedOrigins; } /** - * Configure HTTP methods to allow, e.g. "GET", "POST", "PUT". The special - * value "*" allows all method. When not set only "GET is allowed. + * Set the HTTP methods to allow, e.g. {@code "GET"}, {@code "POST"}, + * {@code "PUT"}, etc. + *

The special value {@code "*"} allows all methods. + *

If not set, only {@code "GET"} is allowed. *

By default this is not set. */ - public void setAllowedMethods(List methods) { - this.allowedMethods = methods; + public void setAllowedMethods(List allowedMethods) { + this.allowedMethods = (allowedMethods == null ? null : new ArrayList(allowedMethods)); + } + + /** + * Add an HTTP method to allow. + */ + public void addAllowedMethod(HttpMethod method) { + if (method != null) { + addAllowedMethod(method.name()); + } } /** * Add an HTTP method to allow. */ public void addAllowedMethod(String method) { - if (this.allowedMethods == null) { - this.allowedMethods = new ArrayList(); + if (StringUtils.hasText(method)) { + if (this.allowedMethods == null) { + this.allowedMethods = new ArrayList(); + } + this.allowedMethods.add(method); } - this.allowedMethods.add(method); } /** - * Return the allowed HTTP methods, possibly {@code null} in which case only - * HTTP GET is allowed. + * Return the allowed HTTP methods, possibly {@code null} in which case + * only {@code "GET"} is allowed. + * @see #addAllowedMethod(HttpMethod) + * @see #addAllowedMethod(String) + * @see #setAllowedMethods(List) */ public List getAllowedMethods() { return this.allowedMethods; } /** - * Configure the list of headers that a pre-flight request can list as allowed - * for use during an actual request. The special value of "*" allows actual - * requests to send any header. A header name is not required to be listed if - * it is one of: Cache-Control, Content-Language, Expires, Last-Modified, Pragma. + * Set the list of headers that a pre-flight request can list as allowed + * for use during an actual request. + *

The special value {@code "*"} allows actual requests to send any + * header. + *

A header name is not required to be listed if it is one of: + * {@code Cache-Control}, {@code Content-Language}, {@code Expires}, + * {@code Last-Modified}, or {@code Pragma}. *

By default this is not set. */ public void setAllowedHeaders(List allowedHeaders) { - this.allowedHeaders = allowedHeaders; + this.allowedHeaders = (allowedHeaders == null ? null : new ArrayList(allowedHeaders)); } /** - * Add one actual request header to allow. + * Add an actual request header to allow. */ public void addAllowedHeader(String allowedHeader) { if (this.allowedHeaders == null) { @@ -179,29 +215,34 @@ public class CorsConfiguration { /** * Return the allowed actual request headers, possibly {@code null}. + * @see #addAllowedHeader(String) + * @see #setAllowedHeaders(List) */ public List getAllowedHeaders() { return this.allowedHeaders; } /** - * Configure the list of response headers other than simple headers (i.e. - * Cache-Control, Content-Language, Content-Type, Expires, Last-Modified, - * Pragma) that an actual response might have and can be exposed. + * Set the list of response headers other than simple headers (i.e. + * {@code Cache-Control}, {@code Content-Language}, {@code Content-Type}, + * {@code Expires}, {@code Last-Modified}, or {@code Pragma}) that an + * actual response might have and can be exposed. + *

Note that {@code "*"} is not a valid exposed header value. *

By default this is not set. */ public void setExposedHeaders(List exposedHeaders) { - if (exposedHeaders != null && exposedHeaders.contains("*")) { + if (exposedHeaders != null && exposedHeaders.contains(ALL)) { throw new IllegalArgumentException("'*' is not a valid exposed header value"); } - this.exposedHeaders = exposedHeaders; + this.exposedHeaders = (exposedHeaders == null ? null : new ArrayList(exposedHeaders)); } /** - * Add a single response header to expose. + * Add a response header to expose. + *

Note that {@code "*"} is not a valid exposed header value. */ public void addExposedHeader(String exposedHeader) { - if ("*".equals(exposedHeader)) { + if (ALL.equals(exposedHeader)) { throw new IllegalArgumentException("'*' is not a valid exposed header value"); } if (this.exposedHeaders == null) { @@ -212,6 +253,8 @@ public class CorsConfiguration { /** * Return the configured response headers to expose, possibly {@code null}. + * @see #addExposedHeader(String) + * @see #setExposedHeaders(List) */ public List getExposedHeaders() { return this.exposedHeaders; @@ -219,14 +262,15 @@ public class CorsConfiguration { /** * Whether user credentials are supported. - *

By default this is not set (i.e. user credentials not supported). + *

By default this is not set (i.e. user credentials are not supported). */ public void setAllowCredentials(Boolean allowCredentials) { this.allowCredentials = allowCredentials; } /** - * Return the configured allowCredentials, possibly {@code null}. + * Return the configured {@code allowCredentials} flag, possibly {@code null}. + * @see #setAllowCredentials(Boolean) */ public Boolean getAllowCredentials() { return this.allowCredentials; @@ -242,7 +286,8 @@ public class CorsConfiguration { } /** - * Return the configured maxAge value, possibly {@code null}. + * Return the configured {@code maxAge} value, possibly {@code null}. + * @see #setMaxAge(Long) */ public Long getMaxAge() { return maxAge; @@ -250,24 +295,26 @@ public class CorsConfiguration { /** * Check the origin of the request against the configured allowed origins. - * @param requestOrigin the origin to check. + * @param requestOrigin the origin to check * @return the origin to use for the response, possibly {@code null} which - * means the request origin is not allowed. + * means the request origin is not allowed */ public String checkOrigin(String requestOrigin) { - if (requestOrigin == null) { + if (!StringUtils.hasText(requestOrigin)) { return null; } - List allowedOrigins = this.allowedOrigins == null ? - new ArrayList() : this.allowedOrigins; - if (allowedOrigins.contains("*")) { - if (this.allowCredentials == null || !this.allowCredentials) { - return "*"; + if (ObjectUtils.isEmpty(this.allowedOrigins)) { + return null; + } + + if (this.allowedOrigins.contains(ALL)) { + if ((this.allowCredentials == null) || !this.allowCredentials.booleanValue()) { + return ALL; } else { return requestOrigin; } } - for (String allowedOrigin : allowedOrigins) { + for (String allowedOrigin : this.allowedOrigins) { if (requestOrigin.equalsIgnoreCase(allowedOrigin)) { return requestOrigin; } @@ -276,20 +323,19 @@ public class CorsConfiguration { } /** - * Check the request HTTP method (or the method from the - * Access-Control-Request-Method header on a pre-flight request) against the - * configured allowed methods. - * @param requestMethod the HTTP method to check. + * Check the HTTP request method (or the method from the + * {@code Access-Control-Request-Method} header on a pre-flight request) + * against the configured allowed methods. + * @param requestMethod the HTTP request method to check * @return the list of HTTP methods to list in the response of a pre-flight - * request, or {@code null} if the requestMethod is not allowed. + * request, or {@code null} if the supplied {@code requestMethod} is not allowed */ public List checkHttpMethod(HttpMethod requestMethod) { if (requestMethod == null) { return null; } - List allowedMethods = this.allowedMethods == null ? - new ArrayList() : this.allowedMethods; - if (allowedMethods.contains("*")) { + List allowedMethods = (this.allowedMethods == null ? new ArrayList() : this.allowedMethods); + if (allowedMethods.contains(ALL)) { return Arrays.asList(requestMethod); } if (allowedMethods.isEmpty()) { @@ -298,21 +344,21 @@ public class CorsConfiguration { List result = new ArrayList(allowedMethods.size()); boolean allowed = false; for (String method : allowedMethods) { - if (method.equals(requestMethod.name())) { + if (requestMethod.name().equals(method)) { allowed = true; } result.add(HttpMethod.valueOf(method)); } - return allowed ? result : null; + return (allowed ? result : null); } /** - * Check the request headers (or the headers listed in the - * Access-Control-Request-Headers of a pre-flight request) against the - * configured allowed headers. - * @param requestHeaders the headers to check. + * Check the supplied request headers (or the headers listed in the + * {@code Access-Control-Request-Headers} of a pre-flight request) against + * the configured allowed headers. + * @param requestHeaders the request headers to check * @return the list of allowed headers to list in the response of a pre-flight - * request, or {@code null} if a requestHeader is not allowed. + * request, or {@code null} if none of the supplied request headers is allowed */ public List checkHeaders(List requestHeaders) { if (requestHeaders == null) { @@ -321,20 +367,24 @@ public class CorsConfiguration { if (requestHeaders.isEmpty()) { return Collections.emptyList(); } - List allowedHeaders = this.allowedHeaders == null ? - new ArrayList() : this.allowedHeaders; - boolean allowAnyHeader = allowedHeaders.contains("*"); + if (ObjectUtils.isEmpty(this.allowedHeaders)) { + return null; + } + + boolean allowAnyHeader = this.allowedHeaders.contains(ALL); List result = new ArrayList(); for (String requestHeader : requestHeaders) { - requestHeader = requestHeader.trim(); - for (String allowedHeader : allowedHeaders) { - if (allowAnyHeader || requestHeader.equalsIgnoreCase(allowedHeader)) { - result.add(requestHeader); - break; + if (StringUtils.hasText(requestHeader)) { + requestHeader = requestHeader.trim(); + for (String allowedHeader : this.allowedHeaders) { + if (allowAnyHeader || requestHeader.equalsIgnoreCase(allowedHeader)) { + result.add(requestHeader); + break; + } } } } - return result.isEmpty() ? null : result; + return (result.isEmpty() ? null : result); } } diff --git a/spring-web/src/test/java/org/springframework/web/cors/CorsConfigurationTests.java b/spring-web/src/test/java/org/springframework/web/cors/CorsConfigurationTests.java index cf26bcf0b73..0d76e27b7fa 100644 --- a/spring-web/src/test/java/org/springframework/web/cors/CorsConfigurationTests.java +++ b/spring-web/src/test/java/org/springframework/web/cors/CorsConfigurationTests.java @@ -20,25 +20,21 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import static org.junit.Assert.*; -import org.junit.Before; import org.junit.Test; import org.springframework.http.HttpMethod; +import static org.junit.Assert.*; + /** - * Test case for {@link CorsConfiguration}. + * Unit tests for {@link CorsConfiguration}. * * @author Sebastien Deleuze + * @author Sam Brannen */ public class CorsConfigurationTests { - private CorsConfiguration config; - - @Before - public void setup() { - config = new CorsConfiguration(); - } + private CorsConfiguration config = new CorsConfiguration(); @Test public void setNullValues() { @@ -204,10 +200,14 @@ public class CorsConfigurationTests { @Test public void checkHeadersNotAllowed() { assertNull(config.checkHeaders(null)); + assertNull(config.checkHeaders(Arrays.asList("header1"))); + + config.setAllowedHeaders(Collections.emptyList()); + assertNull(config.checkHeaders(Arrays.asList("header1"))); + config.addAllowedHeader("header2"); - assertNull(config.checkHeaders(Arrays.asList("header1"))); - config.setAllowedHeaders(new ArrayList<>()); + config.addAllowedHeader("header3"); assertNull(config.checkHeaders(Arrays.asList("header1"))); } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/CorsRegistration.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/CorsRegistration.java index 572f1f54a83..0fe3b186f29 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/CorsRegistration.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/CorsRegistration.java @@ -23,14 +23,21 @@ import org.springframework.http.HttpMethod; import org.springframework.web.cors.CorsConfiguration; /** - * Assists with the creation of a {@link CorsConfiguration} mapped to one or more path patterns. - * If no path pattern is specified, cross-origin request handling is mapped on "/**" . + * {@code CorsRegistration} assists with the creation of a + * {@link CorsConfiguration} instance mapped to a path pattern. * - *

By default, all origins, all headers, credentials and GET, HEAD, POST methods are allowed. - * Max age is set to 30 minutes.

+ *

If no path pattern is specified, cross-origin request handling is + * mapped to {@code "/**"}. + * + *

By default, all origins, all headers, credentials and {@code GET}, + * {@code HEAD}, and {@code POST} methods are allowed, and the max age is + * set to 30 minutes. * * @author Sebastien Deleuze + * @author Sam Brannen * @since 4.2 + * @see CorsConfiguration + * @see CorsRegistry */ public class CorsRegistration { @@ -40,15 +47,15 @@ public class CorsRegistration { public CorsRegistration(String pathPattern) { this.pathPattern = pathPattern; - // Same default values than @CrossOrigin annotation + allows simple methods + // Same implicit default values as the @CrossOrigin annotation + allows simple methods this.config = new CorsConfiguration(); - this.config.addAllowedOrigin("*"); - this.config.addAllowedMethod(HttpMethod.GET.name()); - this.config.addAllowedMethod(HttpMethod.HEAD.name()); - this.config.addAllowedMethod(HttpMethod.POST.name()); - this.config.addAllowedHeader("*"); - this.config.setAllowCredentials(true); - this.config.setMaxAge(1800L); + this.config.addAllowedOrigin(CorsConfiguration.ALL); + this.config.addAllowedMethod(HttpMethod.GET); + this.config.addAllowedMethod(HttpMethod.HEAD); + this.config.addAllowedMethod(HttpMethod.POST); + this.config.addAllowedHeader(CorsConfiguration.ALL); + this.config.setAllowCredentials(Boolean.TRUE); + this.config.setMaxAge(CorsConfiguration.DEFAULT_MAX_AGE); } public CorsRegistration allowedOrigins(String... origins) { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/CorsRegistry.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/CorsRegistry.java index be129d138ec..3f4e334effc 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/CorsRegistry.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/CorsRegistry.java @@ -24,9 +24,10 @@ import java.util.Map; import org.springframework.web.cors.CorsConfiguration; /** - * Assist with the registration of {@link CorsConfiguration} mapped on a path pattern. - * @author Sebastien Deleuze + * {@code CorsRegistry} assists with the registration of {@link CorsConfiguration} + * mapped to a path pattern. * + * @author Sebastien Deleuze * @since 4.2 * @see CorsRegistration */ @@ -36,12 +37,14 @@ public class CorsRegistry { /** - * Enable cross origin requests processing on the specified path pattern. - * Exact path mapping URIs (such as "/admin") are supported as well as Ant-stype path - * patterns (such as /admin/**). + * Enable cross origin request handling for the specified path pattern. * - *

By default, all origins, all headers, credentials and GET, HEAD, POST methods are allowed. - * Max age is set to 30 minutes.

+ *

Exact path mapping URIs (such as {@code "/admin"}) are supported as + * well as Ant-style path patterns (such as {@code "/admin/**"}). + * + *

By default, all origins, all headers, credentials and {@code GET}, + * {@code HEAD}, and {@code POST} methods are allowed, and the max age + * is set to 30 minutes. */ public CorsRegistration addMapping(String pathPattern) { CorsRegistration registration = new CorsRegistration(pathPattern);