Add CORS support for SE and MP applications to 2.x

[tjquinno]

Resolves #931 (adding CORS support to the built-in Helidon services will be separate)

This PR refactors lots of earlier MP work into SE and builds on that to support CORS for SE applications. The previous MP code was also changed to layer on CORS SE.

See the SE package-info.java file where I captured -- at least for the moment -- quite a bit of how-to information from the SE developer's perspective.

Brief overview:

SE:

• CrossOriginHelper does most of the heavy lifting. Much of this was harvested from the earlier MP-only work and refactored. This class does all the header checking and addition needed to implement CORS. It is public so the MP module can see it but developers should not use it themselves. (They should not need to.) This class also defines very simple abstractions of HTTP requests and responses which SE and MP implement separately.
• CrossOriginConfig represents the information we need to make CORS work for a given endpoint: allowed origins, allowed methods, etc.
• CORSSupport is the SE developer's entry point to CORS. An instance behaves as both a service and a handler so developers can pass it to Routing.Rules#register and also the various method-specific methods (any, put, get, etc.).

MP:

• CrossOrigin is the annotation which MP developers can use on their JAX-RS options method to set up CORS.
• CrossOriginFilter is invoked as requests arrive at endpoints annotated with @CrossOrigin and as response flow back. It relies heavily on the SE class CrossOriginHelper.

[romain-grecourt]

IIUC you are suggesting introducing a Headers abstraction, and when SE and MP call into the CORS code they would pass in two instances of Headers (for request and response) along with all the other data currently accessed via the request and reply abstractions. That would have to include a way to let the caller know to, in some cases, send an OK or FORBIDDEN response (with a message) upon return.

That removes the two request and response abstractions but introduces a headers one, and complicates the API significantly. Is that better than what's there now?

The header abstraction can just be Map<String,List<String>>.

Maybe the shared API could be something like this?

CorsResult CorsSupport.processRequest(String path, Map<String,List<String>> headers);
class CorsResult {
    Map<String,List<String>> headers;
    int status;
    String message;
}

BTW, it is possible to inject the SE request and response in the filter ? maybe that removes the need for an abstraction.

[tjquinno]

Re: your latest, Romain...
Thanks for the note. A few thoughts...

• It looks as if the suggestion is that the header abstraction is an actual Map instance, created and populated as the input argument and another created and populated to store in the CorsResult. The calling code would have to copy headers from the request into the Map passed in and copy the headers from the CorsResult map into the response. This sounds potentially costly at runtime.
• Or, if you meant that we would write an implementations of the Map interface that wrap the headers in the SE ServerRequest and ServerResponse that means essentially writing adapters so the existing headers abstractions in SE and MP act like maps, because they do not already implement Map. We would trading the existing RequestAdapter and ResponseAdapter for two new adapters that, I think, would be more complicated because the Map interface is much broader than than the two we have. Granted, we don't need all of the methods to actually work. I'm missing the benefit.
• The processRequest signature above would have to be broader: CORS needs to get the request's method (yes, easily added as another parameter) and to create new instances of whatever the actual response type is, populated with a status and, for FORBIDDEN responses, a message. Those could be done by adding more parameters, one or two functions to do those tasks in an SE- and MP-specific way. That's a different design approach, but is it a better one?

[romain-grecourt]

• It looks as if the suggestion is that the header abstraction is an actual Map instance, created and populated as the input argument and another created and populated to store in the CorsResult. The calling code would have to copy headers from the request into the Map passed in and copy the headers from the CorsResult map into the response. This sounds potentially costly at runtime.

requestContext.getHeaders() returns a MultiValuedMap<String, String> and MultivaluedMap<K, V> extends Map<K, List<V>>

At least for the MP scenario, the input map would not require copying headers into a new map.
For SE, calling req.headers().toMap() would mean copying so this should be avoided. Perhaps the API can also accepts io.helidon.common.http.Parameters so that the common code is tailored for SE and has a variant that can work for MP without actually depending on JAXRS's MultiValuedMap.

The result map would contain the headers to set, so I was thinking the cost would be minimal. I guess this is the cost of a no-side-effect function: the return value needs to be passed back to the caller.

• Or, if you meant that we would write an implementations of the Map interface that wrap the headers in the SE ServerRequest and ServerResponse that means essentially writing adapters so the existing headers abstractions in SE and MP act like maps, because they do not already implement Map. We would trading the existing RequestAdapter and ResponseAdapter for two new adapters that, I think, would be more complicated because the Map interface is much broader than than the two we have. Granted, we don't need all of the methods to actually work. I'm missing the benefit.

I meant the first scenario.

• The processRequest signature above would have to be broader: CORS needs to get the request's method (yes, easily added as another parameter) and to create new instances of whatever the actual response type is, populated with a status and, for FORBIDDEN responses, a message. Those could be done by adding more parameters, one or two functions to do those tasks in an SE- and MP-specific way. That's a different design approach, but is it a better one?

The key difference between the two is that one returns a value and does not have side effects, where-as the other one does not have a return value but works by creating side-effects.

The no-side-effect approach is simpler with a minor extra cost (few more objects).
However I think that the current design would be worth it if we'd provide the request/response abstraction as a re-usable building block. (Maybe it's something to coordinate with Tomas as that could used by security).

[tjquinno]

I missed that MultiMap extended Map which seems obvious in retrospect.

I agree that there certainly are ways to avoid having the two adapters. I guess I am not convinced it's worth it. With the latest refactoring earlier today they are not public, so we are not committed to supporting them. I understand the advantages of devising more broadly useful adapters which other components could use, but let's not delay this PR for that unification to take place. We could revisit that later on and do some refactoring in CORS as needed.

I will spend a little time looking into exactly what the full adapter-less API might look like. First I need to follow up on some other things Joe brought up earlier today.

[tjquinno]

I spent some time trying to work on a usable abstraction for headers that would work with both SE and MP, but it actually added code and complexity over the code in the PR. So at least for now I've kept the request and response adapter approach.

The latest push involves significant refactoring. It also contains changes that make it quite easy for SE developers to load CORS set-up from a config file and apply that to endpoints with Routing.Rules methods such as .put, .get, etc. In such use cases the configuration includes no paths; that's solely in the developer's code so there's no need to keep paths in sync between the code and the configuration.

But we still support the path-included format for MP support.

[tomas-langer]

In MP - shouldn't we map this to resource class and resource method?
Maybe the approach of fully qualified classname + method name + CORS parameters would be easier for MP users?
Such as:

io.helidon.microprofile.cors.CrossOriginTest.CorsResource2.options.value=http://foo.bar,http://bar.foo
io.helidon.microprofile.cors.CrossOriginTest.CorsResource2.options.allowHeaders=X-foo,X-bar
io.helidon.microprofile.cors.CrossOriginTest.CorsResource2.options.allowMethods=DELETE,PUT
io.helidon.microprofile.cors.CrossOriginTest.CorsResource2.options.allowCredentials=true
io.helidon.microprofile.cors.CrossOriginTest.CorsResource2.options.maxAge=-1

[tjquinno]

The existing format has several advantages:

• Much less verbose.
• Immune to refactoring of the code.
• Supports use cases that the alternative does not. (see below)
• Matches more closely to the mental model for the person setting up the configuration.

The existing format allows path expressions for the path. This lets users easily configure multiple related endpoints in one CORS setting, including using a single entry for the entire app (which seems to be what's often done in the wild).

Also, the alternative described here does not allow for one resource to have multiple endpoints at different paths with the same HTTP method. To support that the alternative format would have to also include either the Java method name or the URI path. In that case why not just use path expressions as in the existing format?

Paths are likely to be more stable than implementation packages and class names because paths are used externally; code is subject to refactoring.

[tomas-langer]

If it is possible to register cors on the root, such as:

routing.register(CorsSupport.create(config.get("cors"))
 .register("/greet", new GreetService);

where the "cors" configuration takes care of all the paths of this webserver, then LGTM.