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

Limit CORS on internal API and allow cookies #2527

Merged
merged 8 commits into from
May 6, 2016

Conversation

mstriemer
Copy link
Contributor

@mstriemer
Copy link
Contributor Author

This actually needs to support the internal search view, will update.

@mstriemer
Copy link
Contributor Author

It seems like this should actually be a middleware instead. Unless there's something I'm missing it looks like corsheaders only supports one configuration. So I guess I'll need to write a very similar middleware or update that to support multiple configurations (this sounds like more work than I was expecting). Perhaps there's something in django-rest-framework that I can hook into instead?

Any ideas @EnTeQuAk?

@EnTeQuAk
Copy link
Contributor

EnTeQuAk commented Apr 29, 2016

hmm, there is nothing specific to django-rest-framework and you're right corsheaders only supports one configuration and not based on the origin.

Given the way corsheaders is written you'd essentially rewrite the whole thing to, say, support a separate config based on HTTP_ORIGIN or based on different endpoints.

The best way still would probably be to write a custom django-rest-framework renderer that patches in our custom headers… something like

from rest_framework.renderers import JSONRenderer
from corsheaders.middleware import ACCESS_CONTROL_ALLOW_HEADERS

class InternalJSONRenderer(JSONRenderer):
    def render(self, data, accepted_media_type=None, renderer_context=None):
        render_context = render_context or {}
        response = render_context.get('response', None)
        if not response:
            return super(InternalJSONRenderer, self).render(data, accepted_media_type, renderer_context)

        response[ACCESS_CONTROL_ALLOW_HEADERS] = '....'
        return super(InternalJSONRenderer, self).render(data, accepted_media_type, renderer_context)

(untested code, this is how I think it could work)

And then still make sure that corsheaders is disabled for our internal endpoints. I don't like duplicating this kind of work but forking corsheaders is even more work but maybe worth it. I'm undecided here.

@diox
Copy link
Member

diox commented May 2, 2016

Alternate approach based on @EnTeQuAk idea, but without using a custom renderer:

  • Build a mixin with a custom finalize_response() method or default_response_headers property that sets the CORS headers we need for the internal API
  • Use that in the internal API views
  • Disable corsheaders for /api/v3/internal/

Although I don't know if either approach would still work with preflight stuff, or if we need to

@mstriemer
Copy link
Contributor Author

@EnTeQuAk @diox I added some settings to this and updated django-cors-headers at mstriemer/django-cors-headers#1. I thought I'd give updating django-cors-headers a try as to avoid two different CORS implementations.

Let me know how it looks, I'm open to changing things.

@EnTeQuAk
Copy link
Contributor

EnTeQuAk commented May 4, 2016

I added some comments on the pr for django-cors-headers and I think I like the implementation. We would have to use your fork for the time being while this is being upstreamed though. But I think that's fine.

lgtm

@mstriemer mstriemer merged commit 2daad31 into mozilla:master May 6, 2016
@mstriemer mstriemer deleted the internal-api-cors-2519 branch May 6, 2016 21:13
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.

Update CORS for internal API endpoints
3 participants