Opened 8 years ago

Closed 7 years ago

#8049 closed (fixed)

AdminSite login has_permission inconsistency

Reported by: nathan Owned by: brosner
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:


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 isagalaev 7 years ago.

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 changed from Unreviewed to Design decision needed

comment:2 Changed 8 years ago by mredar

  • Version changed from SVN to 1.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 SmileyChris

  • Owner changed from nobody to brosner
  • Triage Stage changed from Design decision needed to Accepted

comment:4 Changed 7 years ago by isagalaev

  • Owner changed from brosner to isagalaev
  • Status changed from new to assigned

Changed 7 years ago by isagalaev


comment:5 Changed 7 years ago by isagalaev

  • 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 brosner

  • Owner changed from isagalaev to brosner
  • Status changed from assigned to new

comment:7 Changed 7 years ago by brosner

  • Status changed from new to assigned

comment:8 Changed 7 years ago by adrian

  • Resolution set to fixed
  • Status changed from assigned to closed

(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