Support comma-delimited list of origin patterns in @CrossOrigin

[funky-eyes]

example

@CrossOrigin(originPatterns = "${originPatterns:http://a.b.c:[8088,8089],http://d.e.f}", methods = {POST, OPTIONS})

I found that when using the above example configuration, it is not possible to configure multiple source addresses through properties, hope to support this function, thank you very much

fixed #24982

[pivotal-cla]

@a364176773 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@a364176773 Thank you for signing the Contributor License Agreement!

[funky-eyes]

I’m not sure if it’s better to deal with RequestMappingHandlerMapping#initCorsConfiguration, or CorsConfiguration is more appropriate, but I think CorsConfiguration is more low-level, and the implementation details should be shielded at the bottom.

[sbrannen]

Closely related to:

• Cannot use SpEL expression to inject an array into @CrossOrigin's value attribute #24982

[funky-eyes]

Closely related to:

• Cannot use SpEL expression to inject an array into @CrossOrigin's value attribute #24982

hello, I have paid attention to this issue, but the comma has been used by the configured port. For simple distinction, I will use a semicolon

In addition, the configuration of #{'${origin}'.split(',')} will directly call toString for the array. This is not elegant. I think it can be supported in the resolveCorsAnnotationValue method. What do you think?

[funky-eyes]

@rstoyanchev PTAL

[rstoyanchev]

Sorry for the slow reply.

I think that CorsConfiguration should remain simple, and should not surprise in cases where input is collected in other ways, that could lead to a potential surprise. So, at the risk of some duplication (between spring-webmvc and spring-webflux), I think the support for splitting should be done in RequestMappingHandlerMapping, at the point of collection, and be documented as a feature on the respective @CrossOrigin attributes. Currently the annotation does not have any mention of the capbaility but it's where the feature needs to apply and be advertised as such.

Second, for the implementation, I think we should stick to using "," i.e. the common way of separating, but obviously the parsing will need to be smarter to iterate over the full string and ignore any port-related commas, i.e. within square brackets.

[funky-eyes]

Sorry for the slow reply.

I think that CorsConfiguration should remain simple, and should not surprise in cases where input is collected in other ways, that could lead to a potential surprise. So, at the risk of some duplication (between spring-webmvc and spring-webflux), I think the support for splitting should be done in RequestMappingHandlerMapping, at the point of collection, and be documented as a feature on the respective @CrossOrigin attributes. Currently the annotation does not have any mention of the capbaility but it's where the feature needs to apply and be advertised as such.

Second, for the implementation, I think we should stick to using "," i.e. the common way of separating, but obviously the parsing will need to be smarter to iterate over the full string and ignore any port-related commas, i.e. within square brackets.

thx, I'm going to improve that logic

[funky-eyes]

@sbrannen @rstoyanchev There is a timeout error in concourse- CI, I do not know what the specific error is, please help me to check

[sbrannen]

There is a timeout error in concourse- CI, I do not know what the specific error is, please help me to check

A timeout on the CI server typically means that the CI server was under too much load at that point in time.

It's more important that you have successful local builds on your machine before pushing commits to your PR.

For example, I see that you are using org.junit.jupiter.api.Assertions which is forbidden in Spring Framework's Checkstyle rules. For assertions in tests, we use AssertJ exclusively.

Please make sure you get a passing build using ./gradlew build locally before pushing commits, and please push a commit that fixes any current issues breaking the build locally for you.

[funky-eyes]

There is a timeout error in concourse- CI, I do not know what the specific error is, please help me to check

A timeout on the CI server typically means that the CI server was under too much load at that point in time.

It's more important that you have successful local builds on your machine before pushing commits to your PR.

For example, I see that you are using org.junit.jupiter.api.Assertions which is forbidden in Spring Framework's Checkstyle rules. For assertions in tests, we use AssertJ exclusively.

Please make sure you get a passing build using ./gradlew build locally before pushing commits, and please push a commit that fixes any current issues breaking the build locally for you.

ok, thank you

[funky-eyes]

@sbrannen

I've compiled successfully, but concourse-ci is still hanging

[sbrannen]

I've compiled successfully,

Great!

but concourse-ci is still hanging

Yeah, our CI server is timing out a lot lately. We'll have to see what we can do about that.

[rstoyanchev]

As this is purely a web concern, there should be no changes in spring-beans, nor in spring-context. The parsing of an origin can be simplified a little and re-used differently (without involving spring-context).

I'm also not sure we should support expressions that split into an array. It should be sufficient to support coma-separated origin values.

[rstoyanchev]

I've gone for a slightly different implementation. Thanks for the PR in any case!

[funky-eyes]

@rstoyanchev Sorry I'm just now seeing your message, thanks, and hopefully I'll have the opportunity to contribute again in the future

[funky-eyes]

badba7c
This commit doesn't seem to carry support for SpEL expressions
@rstoyanchev

[rstoyanchev]

The reporter tried using SpEL and suggested support for it, but my read of the issue is that the main goal was to inject an array through an external property. So we've added support for a coma-delimited value, which takes care of that goal.