-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add CORS parameters to Cornice services #48
Conversation
68e20c3
to
38a86d7
Compare
I have made a rebase to fix some conflicts with master. In the process it seems that @tsauerwein following comment is now less visible: 68e20c3#commitcomment-13926722 |
I pushed a commit to your branch making the origins configurable. Could you please test with that? |
087c58f
to
634fe6e
Compare
I have juste made a rebase origin/master to take your #50 changes into account and fixed a conflict. The rebase is pushed. The API works as expected. I have not had the opportunity to test with the UI calls, it's currently broken in my instance... |
Thanks for the improvements by the way! |
Well, I think there's a problem:
By the way: I agree that we should avoid that anyone may write using the API, but that's probably wanted that anyone may read the documents (at least non user-related docs). For instance if a hut website would like to show the outings starting from the hut (not sure we want to add websites in the config in that case). |
In addition shouldn't we set the cors-origins values in the config/* files rather than directly in common.ini.in? |
At least when setting cors.origins to * in https://github.com/c2corg/v6_api/blob/cornice_cors/common.ini.in#L12 it seems to work :P |
So, let's use "*" for uncritical GET requests and a specific origin for POST/PUT and critical GET requests (like for the user data)?
Oh yes, I originally wanted to that, but then forgot. |
Makes sense indeed |
We discussed with @asaunier and it appears we should have two policies:
|
@@ -12,6 +12,12 @@ | |||
import json | |||
|
|||
|
|||
cors_policy = dict( |
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.
Would be renamed to cors_public_policy.
Yes, that's what I also hand in mind. |
634fe6e
to
ecb3bec
Compare
I have rebased this branch to get the last changes in master and fixed the conflicts.
What about merging this PR (with |
With @gberaudo we have figured out that the cors.origins should contain the port number as well as the URL scheme (http://). We may use * to allow whatever port and scheme. For instance
works. |
What the use of "internalurl" set in https://github.com/c2corg/v6_api/blob/cornice_cors/config/default#L11 and https://github.com/c2corg/v6_api/blob/cornice_cors/config/dev#L6 ? It seems to be used nowhere else. |
615bbab
to
ba57fca
Compare
Looks good! Will you open tickets for #48 (comment) ? |
Add CORS parameters to Cornice services
Replace #47
Based upon
https://blog.mozilla.org/services/2013/02/04/implementing-cross-origin-resource-sharing-cors-for-cornice/
https://cornice.readthedocs.org/en/latest/api.html?highlight=cors
I have tried to add the cors_policy to the DocumentRest class directly but it failed, most likely because I had omitted the path params in the @resource decorator. Anyway it's probably better to have all the cornice params explicitly set for each service.