Add support for Preflighted CORS requests, closes #99

[jhthorsen]

Please review the following PR for adding support for Preflighted CORS requests.

I followed the specification on https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS, but I have not tested this in a browser. Any help in doing so is more than welcome.

@mario-minati, @plk.

[plk]

From a casual review, this looks good to me, exactly what is needed in terms of functionality. I am in a position to test this with a large, live site (in DEV ...) so perhaps you can apply to a dev branch and I can build form source? Or, if it's all opt-in options, will you make a release?

[jhthorsen]

@plk: Can install it with the command below?

cpanm https://github.com/jhthorsen/mojolicious-plugin-openapi/archive/feature/cors.tar.gz

[plk]

Looks good - I have managed to test simple and preflight and it all seems to work. One small issue is that the check in simple requests using @CORS_SIMPLE_CONTENT_TYPES fails when there is no Content-Type header at all (which is possible - GET requests generally don't have this) and so I think this needs to be changed (line 93) so that the check against @CORS_SIMPLE_CONTENT_TYPES is only done if the Content-Type header is present.

[jhthorsen]

Thanks for the feedback! I just added a fix for the simple CORS requests.

What do you think about the API? Does it make sense to you? Does it provide nice default values? I want to make an API where the focus is for the end user to do less coding and be as much DWIM as possible.

[jhthorsen]

I just force pushed because of the addition of Mojolicious::Plugin::OpenAPI::SpecRenderer in master, but the unit test is exactly the same as before, so I do believe it works as expected.

[plk]

Yes, the API is nice - I started to worry about allowed headers etc. and look into it but the defaults took care of it and my authentication with custom headers worked fine with no extra config.

[jhthorsen]

Cool!

I'll leave it open until tomorrow, in case someone else got input.

[plk]

Will you then release? I actually need this feature ...

[jhthorsen]

Yes. Unless I forget or someone has important inout, I’ll make a release in about 16 hours - when I wake up.

You can still install it (or download) using the “cpanm” command above though.

[mrenvoize]

Looks sane and works for me under basic testing too. Nice work jhthorsen

[plk]

Hmm, I'm trying the latest and now Swagger UI is complaining about bad network request in the post-preflight real request (even though it succeeds). This seems to be because the Access-Control-Allow-Origin header isn't returned with the post-OPTIONS, POST request as per the specs?

[jhthorsen]

Oh! I missed out... I see now that I have a major flaw in the design. I might have to change the API to make it simpler. I'll get back to you when I have more information.

[jhthorsen]

@plk: So I just gave it another try. Please note that the API is completely new. Let me know what you think.

Here some highlights:

mojolicious-plugin-openapi/lib/Mojolicious/Plugin/OpenAPI/Cors.pm

Line 257 in 35f67e7

=head1 STASH VARIABLES

mojolicious-plugin-openapi/lib/Mojolicious/Plugin/OpenAPI/Cors.pm

Line 194 in 35f67e7

=head2 Custom exchange

mojolicious-plugin-openapi/lib/Mojolicious/Plugin/OpenAPI/Cors.pm

Line 286 in 35f67e7

=head2 openapi.cors_exchange

mojolicious-plugin-openapi/lib/Mojolicious/Plugin/OpenAPI/Cors.pm

Line 27 in 35f67e7

# TODO: Remove support for openapi.cors_simple

[plk]

Works fine now, thanks. It required no code changes from the previous test as I was only using _exchange as I suspect many would and so the API changes make sense to me.

[jberger]

Overall this is fine. I left a few ideas to ponder.

Note I didn't read the code just the documentation. If you want a full code review ask and I'll try to find time.

[jberger]

I especially like the stash var interface here. I wonder if you could go further with it and allow the exchange callback to be defined here too? Then it just gets run with the regular validators?

[jhthorsen]

I don't understand. Isn't that what openapi_cors_default_exchange_callback is?

[jhthorsen]

Works fine now, thanks. It required no code changes from the previous test as I was only using _exchange as I suspect many would and so the API changes make sense to me.

I don't understand. Do you mean you did shift->openapi->cors_exchange->openapi->valid_input and also the openapi_cors_allowed_origins stash variable? (Note that that openapi_cors_allowed_origins has been renamed now)

[jhthorsen]

I'm going to merge this PR now, even if not all comments have been answered, since I think it's just minor modifications that is needed if any, and I think it's about time to let this out in the wild.

Either way, it's not too late to change things if that is desired.

@plk: Version 2.10 should be available pretty soon.

[plk]

I opened a new issue as with 2.10 and really basic setup, CORS all works but my routes all now fail with 404 not found when no CORS is involved ... this seems to be something to do with what add_preflighted_routes does.

[plk]

2.11 fixed the issue - I might have been getting 400 but my setup is a bit complex and this resulted in 404s but the issue was the same so thanks for fixing this.