Skip to content
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 Cross-Origin Resource Sharing (CORS) support. #98

Merged
merged 2 commits into from
Jan 26, 2013
Merged

Conversation

almet
Copy link
Contributor

@almet almet commented Jan 8, 2013

This still misses documentation, but I want to have a review on the code first.

@rfk r?

def _options_view(request):
origin = request.headers.get('Origin')
if not origin:
request.errors.add('headers', 'Origin',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the docs say that the error location field should be "header", not "headers". Not sure if this is enforced or assumed anywhere though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, we should add a way to check this in the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #99

@rfk
Copy link
Contributor

rfk commented Jan 8, 2013

Cool, I like there this is going and it will be awesome to have easy CORS support baked in. It looks like some parts of the spec are unimplemented:

  • the Access-Control-Max-Age header
  • the Access-Control-Allow-Credentials header (although there is a setting for it)
  • the Access-Control-Expose-Headers header
  • handling of "simple cross-origin requests" without pre-flighting

Are they all things that can safely be left till a later date and/or left to the caller to fill in as part of the view?

@almet
Copy link
Contributor Author

almet commented Jan 8, 2013

Access-Control-Max-Age and Access-Control-Allow-Credentials will be implemented, yep.

As you said, I still need to implement simple cross-origin requests, and Access-Control-Expose-Headers as part of that.

more work, I'll ping you here when I have something with these missing steps.

@almet
Copy link
Contributor Author

almet commented Jan 23, 2013

I've updated the branch with the whole specification implemented, it still misses the documentation part but I'm waiting feedback on the implementation etc. before.

@rfk do you want to review this again?

This can be useful if you don't want to specify all the cors-related parameters
each time you define a service
@almet
Copy link
Contributor Author

almet commented Jan 24, 2013

updated again; I've successfully used it for web services on Chrome and Firefox. It doesn't mean it will work everywhere, but it seems to be okay.

Also, I added the support for cors_policy which can be useful some times.

'%s not allowed' % origin)
else:
response.headers['Access-Control-Allow-Origin'] = origin
return _cors_validator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a larger architectural thought, I wonder if we should pass the service object into each validator by default. IOW, make the signature of a validator function "validate(service, request)" or similar. How common is it to make a validator that closes over the service object like this?

@rfk
Copy link
Contributor

rfk commented Jan 25, 2013

This looks great @ametaireau, very clean and easy to follow. r+

almet added a commit that referenced this pull request Jan 26, 2013
Add Cross-Origin Resource Sharing (CORS) support.
@almet almet merged commit 3ffb747 into master Jan 26, 2013
@leplatrem leplatrem deleted the cors-support branch October 24, 2016 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants