helidon-io / helidon

Java libraries for writing microservices
https://helidon.io
Apache License 2.0
3.49k stars 562 forks source link

Decoupled CORS supports #7631

Open romain-grecourt opened 12 months ago

romain-grecourt commented 12 months ago

Currently (as of 4.0.0-M2), the CORS support is an adhoc solution implemented in various component that need it (E.g. OpenApi). The configuration is nested under each component.

How about having a centralized solution implemented as a filter, similar to SecurityFeature.

E.g.

cors:
    paths:
        - path: "/greeting[/{*}]"
          methods: ["GET"]
          allow-origins: ["http://foo.com", "http://there.com", "http://other.com"]
          allow-methods: ["PUT", "DELETE"]
static void routing(HttpRouting.Builder routing) {
    routing.addFeature(CorsFeature.create())
           .register("/greet", GreetService::new)
}
tomas-langer commented 12 months ago

All features that expose endpoints (and are not part of observability) extend HelidonFeatureSupport that takes care of CORS setup for each of them. This common feature has setup for endpoint and CORS. If needed, there is CorsSupport (an HttpService or Handler) that does what you want for a single endpoint (or whole of Helidon, if you register it in the root).

romain-grecourt commented 12 months ago

The current 4.x implementation simplifies the code code maintenance. Although HelidonFeatureSupport is still using hand-crafted builder making it very hard to use the new code-gen tooling.

However, effectively the configuration is coupled to individual features.. Why does CORS require such coupling ? A simple centralized mapping like security would provide a much better user experience IMO.

tomas-langer commented 12 months ago

Actually this feature already exists - you can register cors on root with configuration (at least in MP there are tests validating multiple paths, so I guess this works in SE as well. Maybe @tjquinno can comment?

We have support for component specific CORS to be added for metrics, health etc. as these may have different requirements. There is probably space for improvement and simplification, but I think the use case should work.

romain-grecourt commented 12 months ago

Then the question is can we remove the component specific glue and have only the root configuration.