DATAREST-573 Add support for Spring Web MVC CORS configuration mechanisms. #233

mp911de · 2016-10-10T14:52:41Z

We now support CORS configuration mechanisms introduced in Spring 4.2. CORS can be configured on multiple levels: Repository interface, Repository REST controller and global level. Spring Data REST CORS configuration is isolated so Spring Web MVC'S CORS configuration does not apply to Spring Data REST resources.

Multiple configuration sources are merged so different aspects of CORS can be configured in separate locations.

@CrossOrigin
interface PersonRepository extends CrudRepository<Person, Long> {}

@RepositoryRestController
@RequestMapping("/person")
public class PersonController {

    @CrossOrigin(maxAge = 3600)
    @RequestMapping(method = RequestMethod.GET, "/xml/{id}", produces = MediaType.APPLICATION_XML_VALUE)
    public Person retrieve(@PathVariable Long id) {
        // ...
    }
}

@Component
public class SpringDataRestCustomization extends RepositoryRestConfigurerAdapter {

  @Override
  public void configureRepositoryRestConfiguration(RepositoryRestConfiguration config) {

    config.addCorsMapping("/person/**")
        .allowedOrigins("http://domain2.com")
        .allowedMethods("PUT", "DELETE")
        .allowedHeaders("header1", "header2", "header3")
        .exposedHeaders("header1", "header2")
        .allowCredentials(false).maxAge(3600);
  }
}

…oduced in Spring 4.2. Prepare issue branch.

odrotbohm

Great one. Added a few minor comments. I wouldn't mind having @sdeleuze skim over the changes, especially the parts that use the new API for whether he can detect some usage patterns that look unnatural.

odrotbohm · 2016-10-14T09:43:50Z

...-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestHandlerMapping.java

+		String repositoryLookupPath = new BaseUri(configuration.getBaseUri()).getRepositoryLookupPath(lookupPath);
+
+		// Repository root resource
+		if (StringUtils.hasText(repositoryLookupPath) && repositories != null) {


Can we invert the conditions to be able to use early returns and thus avoid the nesting?

odrotbohm · 2016-10-14T09:44:09Z

...-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestHandlerMapping.java

+		 */
+		CorsConfigurationAccessor(ResourceMappings mappings, Repositories repositories,
+				StringValueResolver embeddedValueResolver) {
+			this.mappings = mappings;


Non-null assertions missing.

odrotbohm · 2016-10-14T09:44:40Z

...-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestHandlerMapping.java

+		CorsConfiguration findCorsConfiguration(String lookupPath) {
+
+			ResourceMetadata resource = getResourceMetadata(getRepositoryBasePath(lookupPath));
+			if (resource != null) {


Ternary if?

odrotbohm · 2016-10-14T09:45:08Z

...-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestHandlerMapping.java

+			}
+
+			String allowCredentials = resolveCorsAnnotationValue(annotation.allowCredentials());
+			if ("true".equalsIgnoreCase(allowCredentials)) {


Insert blank line before.

odrotbohm · 2016-10-14T09:45:42Z

...rc/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java

 	 * The {@link HandlerMapping} to delegate requests to Spring Data REST controllers. Sets up a
 	 * {@link DelegatingHandlerMapping} to make sure manually implemented {@link BasePathAwareController} instances that
 	 * register custom handlers for certain media types don't cause the {@link RepositoryRestHandlerMapping} to be
 	 * omitted.
-	 * 


Avoid whitespace change.

odrotbohm · 2016-10-14T09:46:07Z

...c/src/test/java/org/springframework/data/rest/webmvc/CorsConfigurationAccessorUnitTests.java

+@RunWith(MockitoJUnitRunner.class)
+public class CorsConfigurationAccessorUnitTests {
+
+	private CorsConfigurationAccessor accessor;


Fields don't need to be private in tests.

odrotbohm · 2016-10-14T09:46:53Z

...t-tests-jpa/src/test/java/org/springframework/data/rest/webmvc/jpa/CorsIntegrationTests.java

+		}
+	}
+
+	@RepositoryRestController


Let's move those classes under the test methods so that we stay consistent to other tests. Configuration can stay here.

odrotbohm · 2016-10-14T09:47:47Z

...est-core/src/main/java/org/springframework/data/rest/core/config/RepositoryCorsRegistry.java

+	/**
+	 * Creates a new {@link RepositoryCorsRegistry}.
+	 */
+	public RepositoryCorsRegistry() {}


I don't think we need the default constructor, do we?

odrotbohm · 2016-10-14T09:48:40Z

...est-core/src/main/java/org/springframework/data/rest/core/config/RepositoryCorsRegistry.java

+ * @author Mark Paluch
+ * @since 2.6
+ */
+public class RepositoryCorsRegistry extends CorsRegistry {


What's the reason we need this dedicated class? It seems we could just get away with using CorsRegistry, right?

CorsRegistry's getCorsConfigurations method is protected so we need to make it usable for us. Didn't want to rely on reflection.

odrotbohm · 2016-10-14T09:50:08Z

...-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestHandlerMapping.java

+	 *
+	 * @param mappings must not be {@literal null}.
+	 * @param config must not be {@literal null}.
+	 * @param repositories must not be {@literal null}.


Looks like repositories can in fact be null.

sdeleuze · 2016-10-14T12:11:47Z

src/main/asciidoc/configuring-cors.adoc

+
+== Repository interface CORS configuration
+
+You can add a `@CrossOrigin` annotation to your repository interfaces to enable CORS for the whole repository. By default `@CrossOrigin` allows origins and `HEAD` and `GET` HTTP methods:


As mentioned here, the current behaviour with Spring Data REST + @CrossOrigin is to allow GET and HEAD HTTP methods (default for the low level CorsConfiguration class).

In Spring MVC, the default behaviour for @CrossOrigin annotations without any method specified is to use methods of the related @RequestMapping annotation. I am wondering if by default in Spring Data REST, we could not just allow HTTP methods of the related REST endpoint, in order to avoid surprising the user when for example his repository methods exposed with a POST HTTP method will not work by default. Any thoughts?

GET and HEAD are in sync with Spring MVC (initially it's the same behavior) and to not allow any methods that change data by default.

We check the allowed methods on collection/item level inside the particular controllers – we could derive the allowed methods from there but the CORS configuration would be required to mirror the logic that is already present inside the controllers.

After discussing with @sdeleuze, @CrossOrigin on a repository will enable all HTTP methods by default. This default can be overridden. We can evaluate in a later step how we can provide the enabled repository methods to expose only the allowed CORS methods on a resource.

sdeleuze · 2016-10-14T12:16:38Z

Nice work @mp911de ! I have mainly a question about default HTTP methods with @CrossOrigin in Spring Data REST context.

@crossorigin

…oduced in Spring 4.2. We now support CORS configuration mechanisms introduced in Spring 4.2. CORS can be configured on multiple levels: Repository interface, Repository REST controller and global level. Spring Data REST CORS configuration is isolated so Spring Web MVC'S CORS configuration does not apply to Spring Data REST resources. Multiple configuration sources are merged so different aspects of CORS can be configured in separate locations. @crossorigin interface PersonRepository extends CrudRepository<Person, Long> {} @RepositoryRestController @RequestMapping("/person") public class PersonController { @crossorigin(maxAge = 3600) @RequestMapping(method = RequestMethod.GET, "/xml/{id}", produces = MediaType.APPLICATION_XML_VALUE) public Person retrieve(@PathVariable Long id) { // ... } } @component public class SpringDataRestCustomization extends RepositoryRestConfigurerAdapter { @OverRide public void configureRepositoryRestConfiguration(RepositoryRestConfiguration config) { config.addCorsMapping("/person/**") .allowedOrigins("http://domain2.com") .allowedMethods("PUT", "DELETE") .allowedHeaders("header1", "header2", "header3") .exposedHeaders("header1", "header2") .allowCredentials(false).maxAge(3600); } }

Removed RepositoryRestConfiguration.addCorsMapping(…) as we currently don't have any other shortcut methods for configuration like this. Tweaked the setup of (now Repository)CorsConfigurationAccessor to be created earlier so that we avoid recreation for every lookup. Introduced a NoOpStringValueResolver to be used by default so that we don't need to deal with the case of the resolver being null at the end of the call chain. Replaced constructor of RepositoryCorsConfigurationAccessor with corresponding Lombok annotation. Updated reference documentation accordingly. Original pull request: #233.

odrotbohm · 2016-12-04T14:46:42Z

That's been merged a while ago.

neodevelop · 2016-12-13T15:33:53Z

So, which version has actually this new feature and how can I use it?

odrotbohm · 2016-12-13T16:48:57Z

The referenced ticket has all the information.

DATAREST-573 - Add support for new CORS configuration mechanisms intr…

7e8b137

…oduced in Spring 4.2. Prepare issue branch.

odrotbohm requested changes Oct 14, 2016

View reviewed changes

sdeleuze reviewed Oct 14, 2016

View reviewed changes

mp911de force-pushed the issue/DATAREST-573 branch from b223a2e to 273dac7 Compare October 17, 2016 07:34

odrotbohm closed this Dec 4, 2016

odrotbohm self-assigned this Dec 4, 2016

This was referenced Dec 31, 2020

Add support for Spring Web MVC CORS configuration mechanisms [DATAREST-573] #947

Closed

RepositoryRestHandlerMapping 2 arg constructor broken [DATAREST-994] #1358

Closed


		== Repository interface CORS configuration

		You can add a `@CrossOrigin` annotation to your repository interfaces to enable CORS for the whole repository. By default `@CrossOrigin` allows origins and `HEAD` and `GET` HTTP methods:

DATAREST-573 Add support for Spring Web MVC CORS configuration mechanisms. #233

DATAREST-573 Add support for Spring Web MVC CORS configuration mechanisms. #233

Uh oh!

Conversation

mp911de commented Oct 10, 2016

Uh oh!

odrotbohm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdeleuze commented Oct 14, 2016

Uh oh!

odrotbohm commented Dec 4, 2016

Uh oh!

neodevelop commented Dec 13, 2016

Uh oh!

odrotbohm commented Dec 13, 2016

Uh oh!

Uh oh!