Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repository cors configurations are ignored unless there is a global cors configuration set [DATAREST-1397] #1756

Closed
spring-projects-issues opened this issue Jun 15, 2019 · 3 comments
Assignees
Labels

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Jun 15, 2019

Paul Warren opened DATAREST-1397 and commented

Looks to me like spring-webmvc recently refactored their cors code and broke the cors functionality in spring-data-rest.

In this commit the getHandler method was refactored to call a new method hasCorsConfigurationSource(handler) instead of CorsUtils.isCorsRequest(request).

hasCorsConfigurationSource(handler) only checks whether the handler is a CorsConfigurationSource or whether the handler mapping has a global cors configuration source set.  

Since this is a new method there is no overridden implementation in RepositoryRestHandlerMapping.

As a result repository cors configurations are ignored unless there is a global cors configuration set.

I have committed a spring boot app into my github repo that demonstrates the issue.

Had a quick look at the code.  If I were to submit a PR then I would probably modify the signature of hasCorsConfigurationSource(handler) to hasCorsConfigurationSource(handler, request).  Before the refactor, handler mappings used the request to make cores decisions so maybe it is reasonable to reinstate this capability by having this method (in RepositoryRestHandlerMapping) return getCorsConfiguration(handler, request).  But this fix span at least two repos (teams?) and also seems quite surgical.  I may not (probably not) seeing the bigger picture somewhere along the line.   

However, if that does seem appropriate and you want someone to do the grunt work then let me know and I will be happy to PR that


Affects: 3.2 RC2 (Moore)

Reference URL: https://github.com/paulcwarren/DATAREST-1397

Referenced from: pull request #357

Backported to: 3.1.10 (Lovelace SR10)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jun 21, 2019

Mark Paluch commented

The changes in AbstractHandlerMethodMapping indeed break Spring Data REST's CORS discovery on repository interfaces. @RepositoryRestController's are not affected. As you noted, we cannot determine a repository-bound CORS config solely based on the handler but we require path/request details

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jun 21, 2019

Mark Paluch commented

We decided to override hasCorsConfigurationSource returning always true to align with Spring Framework's idea of the refactoring

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jun 21, 2019

Oliver Drotbohm commented

That's merged and backported to 3.1 so that it works for Lovelace on Spring 5.2, too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants