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

Better encapsulate and Javadoc CORS configuration defaults [SPR-14798] #19364

Closed
spring-projects-issues opened this issue Oct 11, 2016 · 9 comments
Closed
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Oct 11, 2016

Rossen Stoyanchev opened SPR-14798 and commented

At present the @CrossOrigin annotation has constants that define some default attribute values (although not for HTTP methods?) and these values are used in places where CorsConfiguration is created.

We need to check if these defaults are applied consistently and consider consolidating the logic to do that, most likely in CorsConfiguration. Either by setting its attributes to those values by default, or providing some convenience method to apply it.

Also the Javadoc on individual methods of CorsRegistration could be added to reflect those defaults since that's that what users of the MVC Java config are more likely to see rather than the class-level Javadoc.


Affects: 5.0 M2

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 11, 2016

Sébastien Deleuze commented

The original idea was to have a non-opinionated CorsConfiguration mainly used internally or for advanced use cases, and a more opinionated and convenient default CORS configuration for high-level user facing CORS configuration capabilities (@CrossOrigin or CorsRegistration). The CorsFilter that uses directly CorsConfiguration was added after that.

@CrossOrigin does not define default HTTP methods because by default it uses the @RequestMapping methods.

I agree that the default methods at CorsConfiguration could be confusing. Originally it supported GET as default method allowed as a convenience, then this commit added HEAD (not 100% sure why). Maybe it would have been more clear to be 100% non-opinionated and require users to set allowed method explicitly, but that would be a breaking change ... Any thoughts on this Rossen Stoyanchev ?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 11, 2016

Rossen Stoyanchev commented

Okay so for anyone instantiating a CorsConfiguration by default it does not allow anything. However in all places where allow CORS to be enabled such as @CrossOrigin, CorsRegistry, mvc:cors, and even in SockJS we immediately switch to "permit all" as the starting point. Perhaps we can add a initEmptyToPermitAll() method in CorsConfiguration that sets those defaults for all fields that aren't set. Then it's still the caller's choice but we can at least ensure better encapsulation and consistency. Also document more clearly in CorsConfiguration that it does not enable anything when create, also referencing the initPermitAll method that can be used.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 11, 2016

Rossen Stoyanchev commented

Actually one more idea that might work better. A constant PERMIT_ALL_CORS_CONFIGURATION of type CorsConfiguration that can then be easily combined with any other CorsConfiguration via CorsConfiguration#combine. That would make it cleaner to apply "permit all" defaults to those CORS attributes that haven't been set.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 18, 2016

Sébastien Deleuze commented

This pull-request is available for review.

I did not implement the CorsConfiguration #PERMIT_ALL_CORS_CONFIGURATION proposal because the properties won't be immutable, so I chose the previous proposal using applyDefaultPermitConfiguration() method name instead of initEmptyToPermitAll().

I chose this name because this is not totally a permit all strategy, not all methods are allowed and it also set allowCredentials and maxAge to defaults that make sense for most users.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 19, 2016

Rossen Stoyanchev commented

I've processed the PR in master (50f2cd, 72397e).

Juergen Hoeller I think this is a candidate for a 4.3 backport. Effectively a change-neutral tightening of how CORS configuration defaults are applied. Also a few constants are deprecated in @CrossOrigin which with a backport could be deprecated in 4.3.

Such a backport would probably need to be applied manually since there are no reactive classes there. Unfortunately they are two commits but we could create a branch, cherry-pick and squash those commits, and then manually backport selectively.

Juergen if you agree please update the fix version.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 19, 2016

Juergen Hoeller commented

Sounds fine to me. Let's consider a separate JIRA issue for the 4.3.4 backport though since this one here is declared as a sub-task of the wider reactive CORS support in 5.0... or redeclare this issue as a standalone ticket in which case it could have 4.3.4 as a target itself.

From a commit perspective, I wouldn't mind a simple standalone commit on 4.3.x where the applicable changes are manually picked from the corresponding files in master and adapted to Java 6 source style right away. There's no need for direct references to master commits from my side, in particular if they originally came from a different context. I've been doing quite a few 5.0 -> 4.3 backports that way already.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 19, 2016

Rossen Stoyanchev commented

Okay I've converted this to an issue and set the fix to 4.3 as well.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2016

Sébastien Deleuze commented

Thanks, I will backport that as a single commit on 4.3.x.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2016

Sébastien Deleuze commented

Backported on 4.3.x branch with 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