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
Re-add accidentally removed CORS middleware #2075
Conversation
@dhruvkb Could you explain the motivation of these changes since there is no issue linked? |
@krysal good point, sorry about that. I updated the description with the problem this PR is aiming to fix. |
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.
Nice! Thank you. LGTM!
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.
This works great! I was able to test this using both curl -L -v -H "Origin: http://localhost:8443/" "http://localhost:50280/"
and running the API and frontend with env API_URL="http://0.0.0.0:50280" just frontend/run dev
. I have a few comments but nothing blocking 😄
middleware = ( | ||
"api.middleware.force_debug_cursor_middleware.force_debug_cursor_middleware" | ||
) | ||
if middleware not in MIDDLEWARE: | ||
MIDDLEWARE.append(middleware) |
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.
Based on the comment, did we actually want to remove this?
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.
I didn't try to understand or change this bit. It was added by @zackkrida in #945.
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.
Yeah this enables the middleware which lets us log all DB queries, so we should keep it!
if "oauth2_provider" not in INSTALLED_APPS: | ||
INSTALLED_APPS.append("oauth2_provider") | ||
|
||
MIDDLEWARE += [ | ||
"oauth2_provider.middleware.OAuth2TokenMiddleware", | ||
] | ||
middleware = "oauth2_provider.middleware.OAuth2TokenMiddleware" | ||
if middleware not in MIDDLEWARE: | ||
MIDDLEWARE.append(middleware) |
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 it make sense to make INSTALLED_APPS
and MIDDLEWARE
sets so we could just call .add
, or is order important?
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.
The order is important. For example, the CORS middleware specifies that it should be set as early in the list as possible, hence https://github.com/WordPress/openverse/pull/2075/files#diff-1e4c90df768f3514290795d5e58d3b561644bd37e4dfdb9938b60083dd062d30R45
Fixes
This PR re-adds the accidentally removed CORS middleware. The absence of this middleware prevents cross-origin requests which consequently prevents using the local API as the server for the frontend.
This was reported by @AetherUnbound.
Description
This PR
corsheaders.middleware.CorsMiddleware
toMIDDLEWARE
INSTALLED_APPS
and middleware when inheriting between settings filesTesting Instructions
Check out this PR and run the dev server:
just api/up
Check that the CORS headers are applied.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin