-
Notifications
You must be signed in to change notification settings - Fork 204
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
Prevent exposing Django Admin features referencing media tables in prod #4349
Conversation
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.
Tests are passing and I've verified the correct behavior locally. 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.
It's unfortunate that these models have to be removed from the admin but also understandable. Thanks for fixing this so quickly, LGTM!
Agreed, but only for prod! Hopefully we'll have the official features for actually handling this well available soon 😄 |
I don't understand why this is the solution? Is the problem not the media object identifier auto-complete query on the Django admin page? There's no way we need to entirely remove these pages from Django admin, that's just not how this kind of thing works. There is something about the admin configuration causing it to make the query, it isn't an inherent feature of those page, but something we can change. Was this meant as a temporary solution? I'm confused about what the long-term intention is here. @AetherUnbound can you clarify on this? This seems a fine stop-gap if we don't know what is causing it, but it's under no circumstances an acceptable long-term solution. |
This is indeed a temporary solution. The The problem is also the media object identifier auto-complete query, which is why those were production deferred on the other models. I also deferred the search fields because those are used by the auto-complete query and (again, my assumption) cause unoptimized queries as well. |
Fixes
Fixes #4344 by @AetherUnbound
Description
This PR modifies the Django Admin registrations for various media report related views in order to prevent exposing access to the primary media tables in production. Queries that are made to these tables are often naive and, for such a large table, cause significant issues. However, they're extremely useful to have when running the application locally in order to easily create/reference media while we build out the content safety reporting features.
This will necessarily reduce our available features in production, but because of the nature of the failure which caused this, we couldn't have used those features anyway!
Testing Instructions
Tests should pass. You can also verify this locally by:
just api/init
Image Decision
shows up in the list of available models for site administrationENVIRONMENT=production
andDJANGO_SECRET_KEY=blahblahblah
toapi/.env
just down && just a
to restart the API stackChecklist
Update index.md
).main
) or a parent feature branch.just catalog/generate-docs
for catalogPRs) or the media properties generator (
just catalog/generate-docs media-props
for the catalog or
just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin