-
-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
initial implemention of cors #11
Conversation
|
|
@bloodbare any news on that? we need tests and docs before we can merge... |
Its been rebased, I merged our code on this branch so we have cors and permission overwriting. @tisto @lukasgraf can you review + merge ? |
Crap. After I merged #38, we need to rebase this pr. @bloodbare would you mind rebasing again? @buchi you had some architectural doubts regarding CORS support in plone.rest, right? Could you repeat those or point me our discussion. |
Uff I've been trying to merge but the change its really big, sorry to miss #38 but I have doubts about it. |
…ic CORS OPTION view vs WEBDAV
@tisto @bloodbare regarding my architectural doubts, my point is that configuring CORS in ZCML is not flexible enough. In general you need to configure CORS headers per website and not per Python package. E.g. plone.restapi cannot configure any CORS headers because it doesn't know which origins it should allow. Somebody using plone.restapi and in need of CORS support would have to override the ZCML service definitions of plone.restapi. |
Hey @buchi, I see your points, in defense of my implementation I would say:
I have the use case of needing different cors for different services, so I would not touch plone.rest. In my opinion plone.rest should be all code that is no plone related, agnostic about plone.restapi and sites. |
@@ -13,6 +13,14 @@ def mark_as_api_request(event): | |||
"""Mark a request with Accept 'application/json' with the IAPIRequest | |||
interface. | |||
""" | |||
# In cors calls there is accept header so we need to force | |||
|
|||
if event.request.get('REQUEST_METHOD') == 'OPTIONS': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling all OPTIONS requests as IAPIRequest is a bad idea. It will break WebDAV support. Also HAProxy is doing backend healthchecks with OPTIONS requests. I would propose checking for the "Origin" HTTP header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, both together is great. Pushed.
…Request to void webdav problems
After our discussion and after thinking again about CORS support I would propose an additional directive for configuring CORS instead of having it in the service directive. Without thinking about implementation but from a user's point of view the directive could look like this: <plone:serviceCORS
accept="application/json"
methods="GET POST OPTIONS"
origin="http://www.foo.bar http://spam.and.eggs"
headers="X-Custom-Header"
max_age="3600"
/> The association between a service and a CORS configuration would be made through the @bloodbare Does this fit your use-case? @tisto @lukasgraf @jone any thoughts? |
@buchi I like the idea of a separate directive for configuring CORS a lot. I agree with your earlier that more flexibility is needed - I think it's unlikely that A dedicated ZCML directive is nice because it avoids persisting configuration in the database. I'm not certain yet what exactly the CORS spec says regarding combinations of methods and origins - would it in theory be possible to allow a different set of origins for As for the |
After thinking again I came to the following conclusions:
The directive for a CORS policy could then look like this: <plone:corsPolicy
for="plone.dexterity.interfaces.IDexterityContent"
layer="IMyBrowserlayer"
methods="GET POST OPTIONS"
origin="http://www.foo.bar http://spam.and.eggs"
headers="X-Custom-Header"
max_age="3600"
/> |
I'll not try to merge the master onto cors, I've been trying for some time and its so different aproach. I've implemented content negotiation and I'll push with the working cors. |
…n and this implementation has the problem of defining content negotiation for each method only, we will need to improve to link the content negotiation to a specific service
…d Origin besides the defined ones
As discussed with @bloodbare we will close this branch. |
Work in progress cors