Improve CORS list properties combination logic

This commit allows CorsConfiguration#combine()
to differentiate permit default values set by
CorsConfiguration#applyPermitDefaultValues()
from values configured explicitly by the user.

Those permit default values will be overridden
by any user-provided ones while user-provided values
will be combined in an additive way, including
when "*" is specified.

Documentation has been improved accordingly.

Issue: SPR-15772

spring-web/src/main/java/org/springframework/web/bind/annotation/CrossOrigin.java

@@ -36,6 +36,12 @@ import org.springframework.web.cors.CorsConfiguration;
  * {@link CorsConfiguration} and then default values are applied via
  * {@link CorsConfiguration#applyPermitDefaultValues()}.
  *
+ * <p>The rules for combining global and local configuration are generally
+ * additive -- e.g. all global and all local origins. For those attributes
+ * where only a single value can be accepted such as {@code allowCredentials}
+ * and {@code maxAge}, the local overrides the global value.
+ * See {@link CorsConfiguration#combine(CorsConfiguration)} for more details.
+ *
  * @author Russell Allen
  * @author Sebastien Deleuze
  * @author Sam Brannen

spring-web/src/main/java/org/springframework/web/cors/CorsConfiguration.java

@@ -22,6 +22,7 @@ import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import org.springframework.http.HttpMethod;
 import org.springframework.lang.Nullable;
@@ -51,14 +52,14 @@ public class CorsConfiguration {
 	/** Wildcard representing <em>all</em> origins, methods, or headers. */
 	public static final String ALL = "*";
 
-	private static final List<HttpMethod> DEFAULT_METHODS;
+	private static final List<HttpMethod> DEFAULT_METHODS =
+			Collections.unmodifiableList(Arrays.asList(HttpMethod.GET, HttpMethod.HEAD));
 
-	static {
+	private static final List<String> DEFAULT_PERMIT_ALL =
-		List<HttpMethod> rawMethods = new ArrayList<>(2);
+			Collections.unmodifiableList(Arrays.asList(ALL));
-		rawMethods.add(HttpMethod.GET);
+
-		rawMethods.add(HttpMethod.HEAD);
+	private static final List<String> DEFAULT_PERMIT_METHODS =
-		DEFAULT_METHODS = Collections.unmodifiableList(rawMethods);
+			Collections.unmodifiableList(Arrays.asList(HttpMethod.GET.name(), HttpMethod.HEAD.name(), HttpMethod.POST.name()));
-	}
 
 
 	@Nullable
@@ -132,6 +133,9 @@ public class CorsConfiguration {
 		if (this.allowedOrigins == null) {
 			this.allowedOrigins = new ArrayList<>(4);
 		}
+		else if (this.allowedOrigins == DEFAULT_PERMIT_ALL) {
+			setAllowedOrigins(DEFAULT_PERMIT_ALL);
+		}
 		this.allowedOrigins.add(origin);
 	}
 
@@ -187,6 +191,9 @@ public class CorsConfiguration {
 				this.allowedMethods = new ArrayList<>(4);
 				this.resolvedMethods = new ArrayList<>(4);
 			}
+			else if (this.allowedMethods == DEFAULT_PERMIT_METHODS) {
+				setAllowedMethods(DEFAULT_PERMIT_METHODS);
+			}
 			this.allowedMethods.add(method);
 			if (ALL.equals(method)) {
 				this.resolvedMethods = null;
@@ -228,6 +235,9 @@ public class CorsConfiguration {
 		if (this.allowedHeaders == null) {
 			this.allowedHeaders = new ArrayList<>(4);
 		}
+		else if (this.allowedHeaders == DEFAULT_PERMIT_ALL) {
+			setAllowedHeaders(DEFAULT_PERMIT_ALL);
+		}
 		this.allowedHeaders.add(allowedHeader);
 	}
 
@@ -325,25 +335,41 @@ public class CorsConfiguration {
 	 */
 	public CorsConfiguration applyPermitDefaultValues() {
 		if (this.allowedOrigins == null) {
-			this.addAllowedOrigin(ALL);
+			this.allowedOrigins = DEFAULT_PERMIT_ALL;
 		}
 		if (this.allowedMethods == null) {
-			this.setAllowedMethods(Arrays.asList(
+			this.allowedMethods = DEFAULT_PERMIT_METHODS;
-					HttpMethod.GET.name(), HttpMethod.HEAD.name(), HttpMethod.POST.name()));
+			this.resolvedMethods = DEFAULT_PERMIT_METHODS
+					.stream().map(HttpMethod::resolve).collect(Collectors.toList());
 		}
 		if (this.allowedHeaders == null) {
-			this.addAllowedHeader(ALL);
+			this.allowedHeaders = DEFAULT_PERMIT_ALL;
 		}
 		if (this.maxAge == null) {
-			this.setMaxAge(1800L);
+			this.maxAge = 1800L;
 		}
 		return this;
 	}
 
 	/**
-	 * Combine the supplied {@code CorsConfiguration} with this one.
+	 * Combine the non-null properties of the supplied
-	 * <p>Properties of this configuration are overridden by any non-null
+	 * {@code CorsConfiguration} with this one.
-	 * properties of the supplied one.
+	 *
+	 * <p>When combining single values like {@code allowCredentials} or
+	 * {@code maxAge}, {@code this} properties are overridden by non-null
+	 * {@code other} properties if any.
+	 *
+	 * <p>Combining lists like {@code allowedOrigins}, {@code allowedMethods},
+	 * {@code allowedHeaders} or {@code exposedHeaders} is done in an additive
+	 * way. For example, combining {@code ["GET", "POST"]} with
+	 * {@code ["PATCH"]} results in {@code ["GET", "POST", "PATCH"]}, but keep
+	 * in mind that combining {@code ["GET", "POST"]} with {@code ["*"]}
+	 * results in {@code ["*"]}.
+	 *
+	 * <p>Notice that default permit values set by
+	 * {@link CorsConfiguration#applyPermitDefaultValues()} are overridden by
+	 * any value explicitly defined.
+	 *
 	 * @return the combined {@code CorsConfiguration} or {@code this}
 	 * configuration if the supplied configuration is {@code null}
 	 */
@@ -369,12 +395,21 @@ public class CorsConfiguration {
 	}
 
 	private List<String> combine(@Nullable List<String> source, @Nullable List<String> other) {
-		if (other == null || other.contains(ALL)) {
+		if (other == null) {
 			return (source != null ? source : Collections.emptyList());
 		}
-		if (source == null || source.contains(ALL)) {
+		if (source == null) {
 			return other;
 		}
+		if (source == DEFAULT_PERMIT_ALL || source == DEFAULT_PERMIT_METHODS) {
+			return other;
+		}
+		if (other == DEFAULT_PERMIT_ALL || other == DEFAULT_PERMIT_METHODS) {
+			return source;
+		}
+		if (source.contains(ALL) || other.contains(ALL)) {
+			return new ArrayList<>(Collections.singletonList(ALL));
+		}
 		Set<String> combined = new LinkedHashSet<>(source);
 		combined.addAll(other);
 		return new ArrayList<>(combined);

spring-web/src/test/java/org/springframework/web/cors/CorsConfigurationTests.java

@@ -108,6 +108,37 @@ public class CorsConfigurationTests {
 		assertTrue(config.getAllowCredentials());
 	}
 
+	@Test  // SPR-15772
+	public void combineWithDefaultPermitValues() {
+		CorsConfiguration config = new CorsConfiguration().applyPermitDefaultValues();
+		CorsConfiguration other = new CorsConfiguration();
+		other.addAllowedOrigin("http://domain.com");
+		other.addAllowedHeader("header1");
+		other.addAllowedMethod(HttpMethod.PUT.name());
+
+		CorsConfiguration combinedConfig = config.combine(other);
+		assertEquals(Arrays.asList("http://domain.com"), combinedConfig.getAllowedOrigins());
+		assertEquals(Arrays.asList("header1"), combinedConfig.getAllowedHeaders());
+		assertEquals(Arrays.asList(HttpMethod.PUT.name()), combinedConfig.getAllowedMethods());
+
+		combinedConfig = other.combine(config);
+		assertEquals(Arrays.asList("http://domain.com"), combinedConfig.getAllowedOrigins());
+		assertEquals(Arrays.asList("header1"), combinedConfig.getAllowedHeaders());
+		assertEquals(Arrays.asList(HttpMethod.PUT.name()), combinedConfig.getAllowedMethods());
+
+		combinedConfig = config.combine(new CorsConfiguration());
+		assertEquals(Arrays.asList("*"), config.getAllowedOrigins());
+		assertEquals(Arrays.asList("*"), config.getAllowedHeaders());
+		assertEquals(Arrays.asList(HttpMethod.GET.name(), HttpMethod.HEAD.name(),
+				HttpMethod.POST.name()), combinedConfig.getAllowedMethods());
+
+		combinedConfig = new CorsConfiguration().combine(config);
+		assertEquals(Arrays.asList("*"), config.getAllowedOrigins());
+		assertEquals(Arrays.asList("*"), config.getAllowedHeaders());
+		assertEquals(Arrays.asList(HttpMethod.GET.name(), HttpMethod.HEAD.name(),
+				HttpMethod.POST.name()), combinedConfig.getAllowedMethods());
+	}
+
 	@Test
 	public void combineWithAsteriskWildCard() {
 		CorsConfiguration config = new CorsConfiguration();
@@ -120,15 +151,13 @@ public class CorsConfigurationTests {
 		other.addExposedHeader("header2");
 		other.addAllowedMethod(HttpMethod.PUT.name());
 		CorsConfiguration combinedConfig = config.combine(other);
-		assertEquals(Arrays.asList("http://domain.com"), combinedConfig.getAllowedOrigins());
+		assertEquals(Arrays.asList("*"), combinedConfig.getAllowedOrigins());
-		assertEquals(Arrays.asList("header1"), combinedConfig.getAllowedHeaders());
+		assertEquals(Arrays.asList("*"), combinedConfig.getAllowedHeaders());
-		assertEquals(Arrays.asList("header2"), combinedConfig.getExposedHeaders());
+		assertEquals(Arrays.asList("*"), combinedConfig.getAllowedMethods());
-		assertEquals(Arrays.asList(HttpMethod.PUT.name()), combinedConfig.getAllowedMethods());
 		combinedConfig = other.combine(config);
-		assertEquals(Arrays.asList("http://domain.com"), combinedConfig.getAllowedOrigins());
+		assertEquals(Arrays.asList("*"), combinedConfig.getAllowedOrigins());
-		assertEquals(Arrays.asList("header1"), combinedConfig.getAllowedHeaders());
+		assertEquals(Arrays.asList("*"), combinedConfig.getAllowedHeaders());
-		assertEquals(Arrays.asList("header2"), combinedConfig.getExposedHeaders());
+		assertEquals(Arrays.asList("*"), combinedConfig.getAllowedMethods());
-		assertEquals(Arrays.asList(HttpMethod.PUT.name()), combinedConfig.getAllowedMethods());
 	}
 
 	@Test  // SPR-14792
@@ -250,4 +279,15 @@ public class CorsConfigurationTests {
 		assertNull(config.checkHeaders(Arrays.asList("header1")));
 	}
 
+	@Test  // SPR-15772
+	public void changePermitDefaultValues() {
+		CorsConfiguration config = new CorsConfiguration().applyPermitDefaultValues();
+		config.addAllowedOrigin("http://domain.com");
+		config.addAllowedHeader("header1");
+		config.addAllowedMethod("PATCH");
+		assertEquals(Arrays.asList("*", "http://domain.com"), config.getAllowedOrigins());
+		assertEquals(Arrays.asList("*", "header1"), config.getAllowedHeaders());
+		assertEquals(Arrays.asList("GET", "HEAD", "POST", "PATCH"), config.getAllowedMethods());
+	}
+
 }

src/docs/asciidoc/web/webflux-cors.adoc

@@ -55,9 +55,10 @@ class or method-level `@CrossOrigin` annotations (other handlers can implement
 `CorsConfigurationSource`).
 
 The rules for combining global and local configuration are generally additive -- e.g.
-all global and all local origins. The only exception are those attributes where only a
+all global and all local origins. For those attributes where only a single value can be
-single value can be accepted such as `allowCredentials` and `maxAge`, in which case the
+accepted such as `allowCredentials` and `maxAge`, the local overrides the global value. See
-local overrides the global value.
+{api-spring-framework}/web/cors/CorsConfiguration.html#combine-org.springframework.web.cors.CorsConfiguration-[`CorsConfiguration#combine(CorsConfiguration)`]
+for more details.
 
 [TIP]
 ====

src/docs/asciidoc/web/webmvc-cors.adoc

@@ -55,9 +55,10 @@ class or method-level `@CrossOrigin` annotations (other handlers can implement
 `CorsConfigurationSource`).
 
 The rules for combining global and local configuration are generally additive -- e.g.
-all global and all local origins. The only exception are those attributes where only a
+all global and all local origins. For those attributes where only a single value can be
-single value can be accepted such as `allowCredentials` and `maxAge`, in which case the
+accepted such as `allowCredentials` and `maxAge`, the local overrides the global value. See
-local overrides the global value.
+{api-spring-framework}/web/cors/CorsConfiguration.html#combine-org.springframework.web.cors.CorsConfiguration-[`CorsConfiguration#combine(CorsConfiguration)`]
+for more details.
 
 [TIP]
 ====