Add features to allow users specify regular expression or Predicate for CORS allowed origins

[seonWKim]

Motivation:

Allow users to specify CORS allowed origins by regular expression or Predicate<String>

Modifications:

Users can specify allowed origins by using regular expression or Predicate<String> like below.

CorsDecorator

@CorsDecorator(
        originRegex = "http://example.*",
        allowedRequestMethods = HttpMethod.GET
)

CorsService's builderForOriginRegex or builder(Predicate<String> originPredicate)

  sb.service("/cors15", myService.decorate(
          CorsService.builderForOriginRegex("http://example.*")
                     .allowRequestMethods(HttpMethod.GET)
                     .newDecorator()));

  sb.service("/cors17", myService.decorate(
          CorsService.builder(origin -> origin.contains("example") || origin.contains("line"))
                     .allowRequestMethods(HttpMethod.GET)
                     .newDecorator()));

or per route

sb.annotatedService("/cors18", new Object() {
                        @Get("/index1")
                        public void index1() {}

                        @Post("/index2")
                        public void index2() {}

                        @Delete("/index3")
                        public void index3() {}
                    }, CorsService.builder()
                                  .andForOriginRegex("http://example.*")
                                  .route("/cors18/index1")
                                  .allowRequestMethods(HttpMethod.GET)
                                  .and()
                                  .andForOriginRegex(Pattern.compile(".*line.*"))
                                  .route("/cors18/index2")
                                  .allowRequestMethods(HttpMethod.POST)
                                  .and()
                                  .andForOrigin((origin) -> origin.contains("armeria"))
                                  .route("/cors18/index3")
                                  .allowRequestMethods(HttpMethod.DELETE)
                                  .and()
                                  .newDecorator()

WebSocketServiceBuilder's allowedOrigins(Predicate<String> originPredicate>

 sb.route()
    .path("/chat")
    .build(WebSocketService.builder(new CustomWebSocketServiceHandler())
                           .allowedOrigins(origin -> origin.contains("armeria"))
                           .build());

Result:

• Closes #Provide a way to specify a regex for allowed origins #4963. (If this resolves the issue.)
• Users can use regular expression or Predicate<String> to specify allowed origins

[seonWKim]

@minwoox What should happen when users specify WebSocket's allowed origins like below?

sb.route()
  .path("/chat")
  .build(WebSocketService.builder(new CustomWebSocketServiceHandler())
                         .allowedOrigins(origin -> origin.contains("armeria"))
                         .allowedOrigins("http://line.com")
                         .build());

Should we throw an exception which specifies it's not possible to set allowedOrigins using Prediate<String> and Strings at the same time OR just use one of Predicate<String> or Strings to verify which origins are allowed?

By the way, I've made a test code in WebSocketServiceCorsTest which is related to this topic.

[trustin]

I think we need to clear this question up before proceeding:

• What should we use for the Access-Control-Allow-Origin header value when a user specified the allowed origins as a regular expression? Should it be * or is a user supposed to provide the header value?
  • If it's the former, is it OK to reject the request for non-matching origins even if we responded that we allow all (*) origins?
  • If it's the latter, is it OK not to list all allowed origins, given the regexes like http://[abc]{1000}.com can yield a very large list?

[seonWKim]

Oh, that's a great point. Let's clear below questions first.

• If it's the former, is it OK to reject the request for non-matching origins even if we responded that we allow all (*) origins?

I think users might be confused in above situation because even though they received * for allowed origins, their requests are rejected.

• If it's the latter, is it OK not to list all allowed origins, given the regexes like http://[abc]{1000}.com can yield a very large list?

Returning all or part of the allowed origins from regex matching might be too expensive. In this case(when using regex or predicate for allowed origins), what do you think of returning the origin from the request?

By the way, this PR's implementation will simply set Access-Control-Allow-Origin header with the Origin value from the request.

[codecov]

Codecov Report

Attention: Patch coverage is 77.94118% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 74.12%. Comparing base (db3973d) to head (3c56648).
Report is 15 commits behind head on main.

Files	Patch %	Lines
...inecorp/armeria/server/cors/CorsPolicyBuilder.java	20.00%	4 Missing ⚠️
...eria/server/websocket/WebSocketServiceBuilder.java	63.63%	2 Missing and 2 partials ⚠️
...a/com/linecorp/armeria/server/cors/CorsPolicy.java	40.00%	3 Missing ⚠️
...eria/server/cors/CorsDecoratorFactoryFunction.java	83.33%	1 Missing and 1 partial ⚠️
...necorp/armeria/server/cors/CorsServiceBuilder.java	80.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4982      +/-   ##
============================================
+ Coverage     74.09%   74.12%   +0.03%     
- Complexity    20907    21002      +95     
============================================
  Files          1809     1819      +10     
  Lines         76940    77357     +417     
  Branches       9833     9880      +47     
============================================
+ Hits          57005    57341     +336     
- Misses        15291    15367      +76     
- Partials       4644     4649       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

🔍 Build Scan® (commit: 3c56648)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/ws5k7gllk45xq
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/kilmbcgxnkjmu
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/ispqsmuaorc46
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/ywh3hdlejz6nw
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/jeyiwozy5tame
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/cdqzncc7vpic2
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/eses56exa5q3y
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/tku5th752bkr2

[minwoox]

By the way, this PR's implementation will simply set Access-Control-Allow-Origin header with the Origin value from the request.

Oops, you are already doing it correctly. Let me continue to review the PR. 😉

[minwoox]

Thanks! Looks pretty good. I left some suggestions. 😉

[jrhee17]

Looks good overall! Left a comment on the overall design

[CLAassistant]

All committers have signed the CLA.

[seonWKim]

Thank you @minwoox and @jrhee17 for reviewing my old PR.

I've applied the comments which are mainly of

• Add requireNonNull
• Use OR logic if allowedOrigins and allowedPredicate is both configured

[minwoox]

After discussing with the team, we've decided to deprecate the CorsPolicy.origins() method. I'll push a patch for that. 😉 The patch also includes some other miscellaneous changes, so please take a look.

[jrhee17]

👍 👍 👍

[jrhee17]

Please sign the CLA if you haven't already @seonwoo960000 🙇

[seonWKim]

@jrhee17 thanks for the review. Seems like I've already signed up the CLA as follows

[minwoox]

Thanks, @seonwoo960000! 👍 👍 👍 👍 👍

[jrhee17]

@jrhee17 thanks for the review. Seems like I've already signed up the CLS as follows

#4982 (comment)

Can you try modifying the authoring email of your commits so that we can move forward with this PR?

[seonWKim]

@jrhee17, thank you for detailed explanation. I've fixed my authoring email address

[ikhoon]

It looks useful. Thanks, @seonwoo960000! 🙇‍♂️