Skip to content

add cors #48

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add cors #48

wants to merge 1 commit into from

Conversation

finish06
Copy link
Member

Explanation

Adds CORS headers default allow all. Initially needed for building UI.

Rationale

UI request from testing partners were being blocked due to cross origin.

Tests

  1. What testing did you do?
    Rebuilt docker image with these changes. Confirmed with @jrlegrand UI is not acccessible.

@yevgenybulochnik
Copy link
Member

Looks good to me for now to get this working with the UI repo. @finish06 how do you feel about using a LB and maybe docker to remove the necessity of CORS altogether, just serve everything same-site. Tho I guess if we want others to build UIs based off the api we would still need CORS.

Only other comment is per the django-cors-header doc the position of the middleware is confusing.

MIDDLEWARE = [
    ...
    'corsheaders.middleware.CorsMiddleware',
    'django.middleware.common.CommonMiddleware',
    ...
]

Does this mean it should always be above CommonMiddleware? I know there is some stuff in the middlewares array that is higher up, Django SessionMiddleware and SecurityMiddleware. I couldnt tell from the docs if they mean place Cors at the very top or just place it above commonmiddleware. Also not really sure if this matters.

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.

2 participants