Fix hard-coded preflight origin

[Tectu]

router::router::generate_preflight_response() currently uses a hardcoded value for Access-Control-Allow-Origin. This should be instead replaced by whatever configuration was passed to the controller: https://github.com/Tectu/malloy/blob/beb0cc66de657187c52f3793694f657ebbcf3e8f/lib/malloy/server/routing/router.cpp#L169

This will require that we also add a scheme field to the controller's configuration as the value for this header field needs to include the scheme if an URL is supplied.

Thinking of it, we should also allow the user of the library to specify other values such as a wildcard.

[Tectu]

I still haven't decided how to do this properly.

I was hoping that we could deduce the information from the controller's config somehow but that doesn't seem to be a very elegant solution given that the the Origin header field might have different information. For example, it needs to include the schema (eg. http:// or https://) and might also list a domain rather than just IP + port.

Currently I think that the best solution is to add a base_url field of type std::string to server::controller::config. This needs to be passed to server::router which would be a good time to create a server::router::config struct which will also hold the shared_ptr to the logger. Furthermore, the current m_generate_preflights field might also be added to that struct.

It might be worth considering having the base_url field in server::controller::config be of type std::shared_ptr<const std::string> instead to avoid unnecessary copies/duplicates. However, that would make filling the configuration struct less elegant. One might instead go for adding std::enable_shared_from_this() to controller::config and pass an std::shared_ptr<const server::controller::config> to the router. Then again, this would give the router access to fields such as the controller's logger which is... not elegant either.

@0x00002a Thoughts?

[0x00002a]

From my perspective I need to be able to have CORS access for webui backends and such. Off the top of my head, maybe it could also be done like the current routes, with an add_preflight method that allowed generation of the preflight (essentially just add(...) but with method::options), and then m_generate_preflights could be used to automatically do it in the absence of a dedicated handler.

Currently I have to disable the automatic preflight stuff and handle options manually with my own response setup with the needed Access-Control-Allow-Origin.

[Tectu]

I think there might have been a slight misunderstanding. I am not proposing to drop this feature. I too need it for exactly the same reason :p

What I'd like to discuss is the design of how this should work - especially how the necessary information for filling the preflight response is passed into the router.

I agree that preflights should only be generated automatically if none was provided manually.

[0x00002a]

Yeah sorry, thats what I meant too. What I mean is, I need to have a way to specify the Access-Control-Allow-Origin and the current API doesn't provide it unless I disable part of it an do it by hand. So if the API is being improved I vote for allowing the user to specify any kind (so, what you said above :p).

I also have other custom setup for the preflight such as allowed headers, and methods. Speaking of which the methods should probably be filled automagically, since we have that information. Perhaps we could expose an object that allowed customising these things, exposing the set(http::field) methods from beast::http::header and providing QoL stuff like an "autogenerate allowed methods" switch or something. Then have a method in router that attached it to specific endpoints via regex (which would also allow the user to make it as specific or general as they like).

Just some ideas

[Tectu]

Improving the API is the entire point of this discussion/issue 😝

So, how about creating struct preflight_config. The router will have an instance of that which will be used to automatically generate preflights (if supposed to).

Speaking of which the methods should probably be filled automagically, since we have that information.

That is already the (somewhat?) the case: https://github.com/Tectu/malloy/blob/8d0529b7060d6b2f8af9b16178e10dd10181d0f6/lib/malloy/server/routing/router.cpp#L170

[0x00002a]

That is already the (somewhat?) the case:

:woman_facepalming: can't believe I missed that

So, how about creating struct preflight_config. The router will have an instance of that which will be used to automatically generate preflights (if supposed to).

I agree but I think it would be helpful to have a per-route config rather than a global or nothing situation, since certain parts of an API may have different requirements but still be mostly the same. If we allow the user to set it via regex for which routes it covers, it could reduce boilerplate/pain for the user while still being flexible enough to be global if the user wants or pinpoint precise as well. I was thinking we could actually just do this as a wrapper on router::add, either wrapped in a lambda or give the config struct an operator().

Thoughts?

[Tectu]

I agree but I think it would be helpful to have a per-route config rather than a global or nothing situation, since certain parts of an API may have different requirements but still be mostly the same.

Well, each (sub-)router would have its own instance of the preflight_config :p

If we allow the user to set it via regex for which routes it covers, it could reduce boilerplate/pain for the user while still being flexible enough to be global if the user wants or pinpoint precise as well.

+1

I was thinking we could actually just do this as a wrapper on router::add, either wrapped in a lambda or give the config struct an operator().

Sounds like a good plan. Just thinking about it - why not just adding another endpoint that is specialized for preflight requests? We could have something like:

struct preflight_config
{
    std::string origin;

    // ...
}

struct endpoint_http_preflight : 
    endpoint,
    resource_matcher
{
    preflight_config cfg;
};

and

bool router::add_preflight(/* ... */);

We can still have the router to optionally generate preflights automatically (if none was specified manually). router::set_generate_preflights() needs to be adapted to accept a preflight_response.

[0x00002a]

That also works. I was thinking more like:

auto add_preflight(const std::string& resource, const preflight_config& cfg) -> bool {
    return add(http::method::options, resource, [cfg](const auto& req) { /* setup response based on cfg */ return res; });
}

That way the user has a preflight config that can be copied and tweaked across multiple instances and isn't tied to a single endpoint. Also we could even add logic for capture group usage if we wanted to.

Having it as its own endpoint might be cleaner in the long run though I guess? (though we would have to reimplement the regex matching of the regex endpoint if we wanted regex so I'm not sure)

[Tectu]

Today we are really good at talking about the same thing but still "disagreeing" :p