Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update requirements to include django-cors-headers with DRF [PR #2650] #2683

Merged
merged 6 commits into from
Jul 24, 2020

Conversation

aadithpm
Copy link
Contributor

@aadithpm aadithpm commented Jul 17, 2020

Description

Adding django_cors_headers as a part of requirements when a project includes Django REST Framework (DRF). This was brought up in issue #2650.

Rationale

Data from DRF usually requires CORS to be consumed. django-cors-headers is installed for a DRF project to get past this, so adding it by default to DRF projects will help.

Fixes #2650

Use case(s) / visualization(s)

  • Create a new Django (or any framework, for that matter) project
  • Create a REST endpoint on your backend that returns some placeholder data
  • Try to consume this endpoint on a front-end on a different origin - you will receive a Same Origin Policy error
  • Use a CORS middleware to bypass this

@browniebroke
Copy link
Member

Thanks for the pull request.

After giving it another look, I was wrong in my reply to your question yesterday. We need to do a couple more things than updating the requirements:

https://github.com/adamchainz/django-cors-headers#setup

So we need to at least:

  • Install the Django app into our settings
  • Add the Middleware

We could also check if there are any settings we can set. For example, we should probably be able to set CORS_URLS_REGEX, as our DRF URLs start with /api/ I think. Keeping in mind that these settings should be conditional of using DRF.

Would you be able to make the changes please?

@aadithpm
Copy link
Contributor Author

Sure, I'll take a look and outline the steps here so I can be sure I haven't missed anything. Thanks for the information.

@aadithpm
Copy link
Contributor Author

I've added what we need to INSTALLED_APPS and MIDDLEWARE. I've also added the URL pattern for all /api URLs to CORS_URLS_REGEX. I couldn't see anything else we would need to add in terms of configuration.

Would love to hear your thoughts and anything else I can add to the PR, thanks!

@browniebroke
Copy link
Member

Looks good otherwise!

Co-authored-by: Bruno Alla <browniebroke@users.noreply.github.com>
@aadithpm
Copy link
Contributor Author

Thanks for the review, I've committed your suggestion.

Copy link
Contributor

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

The PR is great for its intention. However, I'd be very careful to assert that this can be helpful for JS frontend tech. I help maintain djangorestframework-simplejwt, which a lot of articles about SPAs love, absolutely adore, using (just know that it's for access-refresh-style JWT authentication library). It's not ready for browser: no httpOnly flag for the tokens, handling of CSRF has a lot of security issues, and no standard implementation meaning several attack vectors can appear.

For now, at least for anyone trying to integrate Django with frontend frameworks, I recommend trying to integrate it with session middlewear. There is an article online about how someone got React to work using templates.

@browniebroke
Copy link
Member

@Andrew-Chen-Wang can you elaborate a bit please? I think I'm not getting your point across...

Are you saying that integrating django-cors-headers is more complicated than this? When you say "It's not ready for browser", are you talking about CORS headers or JWT?

I think we integrate with the session middleware at work, but we still need CORS headers... Please let me know if I am missing anything.

@Andrew-Chen-Wang
Copy link
Contributor

Andrew-Chen-Wang commented Jul 22, 2020

TL;DR Main point of PR is to use JS framework. I advise against it. Nothing wrong with PR, but it pushes for something that has potentially insecure doings. It was discussed heavily in the Django developers group.

Are you saying that integrating django-cors-headers is more complicated than this?

No, not more complicated.

When you say "It's not ready for browser", are you talking about CORS headers or JWT?

JWT. I'm talking about integrating a frontend JS framework like React, Vue.js, or Angular. The initial comment of the OP listed:

Use case(s) / visualization(s)

Create a new Django (or any framework, for that matter) project
Create a JavaScript driven frontend for the application
Create a REST endpoint on your backend that returns some placeholder data
Try to consume this endpoint with your front-end - you will receive a Same Origin Policy error
Use a CORS middleware to bypass this

I'm talking more about integrating a frontend framework with Django, which is what this PR was more about. Implementing CORS is needed (I'm not entirely against the PR), but the reason behind this PR is not the same as what you're thinking. It's probably why, if you use normal Django templates, you never bump in to this issue: "Try to consume this endpoint with your front-end - you will receive a Same Origin Policy error". This integration is for using something like React, Vue, or Angular.js for your website.

Much of the problem with integration is authentication + authorization. The implementation methods that developers use don't follow a standard; thus, much of the ways people integrate the Auth methods utilize local storage in some manner, continuously accessing it during vital and vulnerable moments -- for example, a 401 error during form submission on a POST request requires refreshing the access token by accessing some local storage and, usually, manually grabbing the CSRF token.

@aadithpm
Copy link
Contributor Author

@Andrew-Chen-Wang firstly, sorry for being the source of all this confusion in the OP - the issue for this stated a need to add CORS to DRF projects. I wrote out the usecase for that so might be slightly misleading from the original issue.

My assumption also was that this PR was for adding functionality for projects where the front-end or client consuming the API is in a different domain. That would be a less specific example of a usecase I thought of - as such, I honestly think my wording was wrong there and I should not have said Javascript driven frontend. My intention with the PR was purely adding CORS to DRF projects because that's something that people always add if they know their API will be used outside of their domain.

@Andrew-Chen-Wang
Copy link
Contributor

@aadithpm No problem. Again, I saw nothing wrong with the PR in general, I am just concerned we're driving for something that seemed to be about JS frontend frameworks (something I just can't support for Django unless the framework can incorporate React in Django templates).

Consuming an API from a different domain makes sense :) I'm not saying this should be an extra feature; it definitely goes with DRF, regardless of implementation, but the way your comment was written was just concerning.

@Andrew-Chen-Wang
Copy link
Contributor

This isn't the original article that I found for integration between the JS framework and Django: https://hackernoon.com/reconciling-djangos-mvc-templates-with-react-components-3aa986cf510a

But it does a pretty good job explaining how to do it. This allows you to completely avoid the CORS thing for JS frameworks and, needless to say, does not require DRF. This means you can use session authentication for the framework.

@aadithpm
Copy link
Contributor Author

Like I said, the motivation behind it was adding CORS simply because it's a pretty universal requirement for an API. I understand your reservations about actually pushing for supporting front-end frameworks as such - I don't feel strongly either way about that.

However, I would agree in that the intention behind this was not clearly addressed. I've changed the PR to reflect that.

  • Create a new Django (or any framework, for that matter) project
  • Create a JavaScript driven frontend for the application
  • Create a REST endpoint on your backend that returns some placeholder data
  • Try to consume this endpoint with your front-end - you will receive a Same Origin Policy error
  • Use a CORS middleware to bypass this

has now been changed to:

  • Create a new Django (or any framework, for that matter) project
  • Create a REST endpoint on your backend that returns some placeholder data
  • Try to consume this endpoint on a front-end on a different origin - you will receive a Same Origin Policy error
  • Use a CORS middleware to bypass this

Hope that does a better job of conveying what we're going for here.

@Andrew-Chen-Wang
Copy link
Contributor

Thank you @aadithpm! LGTM regardless!

@browniebroke browniebroke merged commit 8d5542d into cookiecutter:master Jul 24, 2020
@browniebroke
Copy link
Member

Thanks both! 🎉

@LECbg
Copy link
Contributor

LECbg commented Mar 22, 2021

I'm not very familiar with CORS but, for me, it seems this kind of "server configuration" would be better placed at Traefik (for projects using Docker). I see CORS more as "server stuff" than "project stuff". What do you all think?

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding django-cors-headers to requirements if drf selected
4 participants