Opened 8 years ago

Closed 7 years ago

#8049 closed (fixed)

AdminSite login has_permission inconsistency

Reported by: nathan Owned by: Brian Rosner
Component: contrib.admin Version: 1.0
Severity: Keywords: AdminSite login has_permission
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This is mainly a suggestion for refactoring but it may have security implications. Within the new AdminSite class you have a has_permission method and a login method.

has_permission checks for:
request.user.is_authenticated() and request.user.is_staff

while after authenticating login checks for:
if user.is_active and user.is_staff:

This results in a situation where a user may no longer be able to login but they still have permission to use the site, for example if a user were deactivated while logged in. Since the has_perm method of the user object checks if the user is active this doesn't seem to be too big of a problem under the current configuration, any attempt to access a specific object fails when the user.has_perm method is called. However this seems to be relying on a side effect, as the code evolves or for people subclassing the AdminSite real problems may result. Things could get ugly if someone were to override the has_permission method in a subclass and not the login method or vice versa.

I perhaps may not have dug deeply enough and there is a good reason for having two sets of tests that can be different but in general this seems to violate the DRY principle and open the door for future problems. It appears the code would be more robust if there were a single set of tests that both login and has_permission checked during verification. At the very least it would seem to make sense to explicitly check for user.is_active within the has_permission method rather than waiting for the admin model object to implicitly check it during an add/change/delete has_perm test.

Thank you,

Attachments (1)

8049.diff (10.8 KB) - added by Ivan Sagalaev 7 years ago.
Patch

Download all attachments as: .zip

Change History (9)

comment:1 Changed 8 years ago by Jacob

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 8 years ago by mredar

Version: SVN1.0

This problem also limits the utility of custom AdminSites. I wanted to override the has_permission to allow non-staff users to access a limited customized AdminSite, while retaining the default admin site with full access for staff users. By using the has_permission test in both places, it will allow more custom usage of the admin interface.
Could also create a new function, perhaps 'has_access_permission', as separate test to see if the user can access the AdminSite at all? Subclasses will then have an easy way to control access to custom AdminSites.

comment:3 Changed 8 years ago by Chris Beaven

Owner: changed from nobody to Brian Rosner
Triage Stage: Design decision neededAccepted

comment:4 Changed 7 years ago by Ivan Sagalaev

Owner: changed from Brian Rosner to Ivan Sagalaev
Status: newassigned

Changed 7 years ago by Ivan Sagalaev

Attachment: 8049.diff added

Patch

comment:5 Changed 7 years ago by Ivan Sagalaev

Has patch: set

Added a patch fixing all mentioned cases and a couple more in obscure places (special header generation).

comment:6 Changed 7 years ago by Brian Rosner

Owner: changed from Ivan Sagalaev to Brian Rosner
Status: assignednew

comment:7 Changed 7 years ago by Brian Rosner

Status: newassigned

comment:8 Changed 7 years ago by Adrian Holovaty

Resolution: fixed
Status: assignedclosed

(In [12159]) Fixed #8049 -- Fixed inconsistency in admin site is_active checks. Thanks for patch and tests, isagalaev

Note: See TracTickets for help on using tickets.
Back to Top