Fix 500's when visiting pages guarded by is_admin#752
Merged
Conversation
Visiting various pages triggers 500s because we have a special `is_admin` check in various places. We now have a more graceful fallback than triggering a 500
Member
Author
|
@timcowlishaw When I was chatting to Fershad this morning, I discovered a couple of 500's being triggered. This PR fixes them. Tagging you so you know it's gone through and what it was for.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At the moment, visiting some paths like the SQL explorer path used by our staff , and Some user facing paths like the provide a request update wizard ones can trigger 500 errors, because we often check for
is_adminwhen we do some of our access checks.The default anonymous user does not have this method, and trying to call it raises an exception, triggering the 500.
This change introduces a more graceful fallback, and verifies that the pages that were throwing errors no longer throw errors.
Machine generated summary below
This pull request addresses issues where code was assuming the presence of an is_admin property on Django's AnonymousUser, which previously caused AttributeError exceptions for unauthenticated users. The main change is a patch to AnonymousUser to ensure is_admin always exists and returns False, preventing errors across the application. Additional tests have been added to verify that unauthenticated users do not trigger server errors when accessing certain endpoints.
Key changes:
AnonymousUser patching:
apps/accounts/apps.pyso thatAnonymousUser.is_adminalways returnsFalse, preventing AttributeError when code checksrequest.user.is_adminfor unauthenticated users.Testing improvements:
apps/accounts/tests/test_models.pyto confirm thatAnonymousUser.is_adminreturnsFalseand does not raise an error.Test setup:
AnonymousUserinapps/accounts/tests/test_models.pyfor use in new tests.