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

Add a CORS Filter [SPR-13192] #17784

Closed
spring-issuemaster opened this issue Jul 2, 2015 · 8 comments
Closed

Add a CORS Filter [SPR-13192] #17784

spring-issuemaster opened this issue Jul 2, 2015 · 8 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Jul 2, 2015

Sébastien Deleuze opened SPR-13192 and commented

In addition to current global and fine grained CORS Spring MVC capabilities, we should provide an optional CORS filter that could be executed before the DispatcherServlet.

This filter should use the existing DefaultCorsProcessor and a CorsConfigurationSource implementation that could be configured with a Map<String, CorsConfiguration>.


Referenced from: commits 70a03ee, cd9b390

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 7, 2015

Sébastien Deleuze commented

Hey Rossen Stoyanchev and Rob Winch, could you have a look to this draft commit and send me your feedbacks ?

I have tried to provide a CorsFilter implementation suitable for simple web.xml configuration as well as advanced Java API based configuration with multiple mapped CorsConfiguration.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 8, 2015

Rob Winch commented

Thanks for the opportunity to review Sébastien Deleuze!

I'm not certain I'm convinced on tying the CorsFilter to a specific implementation of CorsConfigurationSource is a good idea. The problem is that the CorsFilter will only support CorsConfigurationMapping instead of any implementation of CorsConfigurationSource.

Another concern I have is that having logic like DEFAULT_PATH hardwired into the CorsFilter makes it difficult to support other PathMatcher implementations / instances which might be necessary to support path delimiters of "." instead of "/".

Instead you might consider CorsFilter having a setter method for CorsConfigurationSource. Users can then inject any implementation of CorsConfigurationMapping into the CorsFilter. All logic for determining the CorsConfiguration would be contained within the CorsConfigurationSource

User's could leverage the DelegatingFilterProxy to take advantage of the CorsFilter. If you really wanted to support using Filter init parameters, I think you could register a custom ConversionService that would convert from a String to the CorsConfigurationSource object you want.

If all the logic is contained in the CorsConfigurationSource implementation, it may be possible to reuse it within Spring MVC rather than having to repeat the logic (i.e. within the HandlerMappings).

Thoughts?

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 8, 2015

Sébastien Deleuze commented

Thanks for you feedback Rob Winch!

You're right CorsFilter should allow to use any CorsConfigurationSource implementation.

I thought initially that providing a filter that works out of the box for users using only filter init params was mandatory, but after your feedback and more thoughts about this, I agree that using DelegatingFilterProxy + CorsConfigurationMapping make perfectly sense.

About sharing the logic between the filter and Spring MVC, that's why I introduced CorsConfigurationMapping, so I think this goal is already achieved.

I will update this commit shortly, and will notice you as soon as it is done.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 9, 2015

Sébastien Deleuze commented

Hey Rob Winch, could you please have a look to this updated commit?

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 9, 2015

Rob Winch commented

Thanks for the quick turnaround Sébastien Deleuze. I like the CorsFilter much better now.

Any reason the method getCorsConfiguration was renamed to getCorsConfigurations? It seems weird since the result is a single CorsConfiguration.

Thoughts?

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 9, 2015

Sébastien Deleuze commented

Good catch, it was an unexpected side effect of the renaming of Map<String, CorsConfiguration> getConfiguration() to Map<String, CorsConfiguration> getConfigurations(), my IDE detected CorsConfiguration getConfiguration(Object handler, HttpServletRequest request) as an overloaded method, and renamed it as well (it asked me but I didn't notice). It is now fixed, thanks.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 9, 2015

Rob Winch commented

+1 for these changes Thanks again for the fast turnaround :)

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 9, 2015

Sébastien Deleuze commented

You're welcome!

These changes are now merged in master thanks to this commit.

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.