Opened 5 years ago

Closed 4 months ago

#14434 closed Bug (duplicate)

AdminSite should rely on self.has_permission for all permission checks

Reported by: bkonkle Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Normal Keywords: admin views
Cc: alexkoshelev Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

At the end of the login method on django.contrib.admin.sites.AdminSite , if the user data is correct the method checks for user.is_active and user.is_staff and then logs the user in. The admin_view method calls the has_permission method, which also checks for user.is_active and user.is_staff by default. Putting this into a separate method provides an extension point, however, to customize the permissions checked.

The fact that the login method doesn't check has_permission but checks is_active and is_staff explicitly is redundant and breaks the customization if a developer wants to create a separate, limited admin site where user.is_staff isn't a requirement. A use case would be in multi-tenancy situations, where users with a special permission may be able to access a site-specific admin site where they can only access data for their site.

Attachments (1)

django-admin-login-view.diff (537 bytes) - added by bkonkle 5 years ago.

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by bkonkle

comment:1 Changed 5 years ago by alexkoshelev

  • Cc alexkoshelev added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by ramiro

  • Triage Stage changed from Unreviewed to Accepted

This the one of the issues that had been reported in #8049, unfortunately the solution implemented when fixing it wasn't to centralize the logic but rather to duplicate the if checks.

comment:3 Changed 4 years ago by julien

  • Component changed from Contrib apps to django.contrib.admin

comment:4 Changed 4 years ago by julien

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

This has been fixed as a side effect of [14769].

comment:5 Changed 4 years ago by danny.adair@…

  • Easy pickings unset
  • Resolution fixed deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Uncategorized
  • UI/UX unset

comment:6 Changed 4 years ago by SmileyChris

  • Needs tests set
  • Patch needs improvement set
  • Summary changed from AdminSite should check self.has_permission in self.login to AdminSite should rely on self.has_permission for all permission checks
  • Type changed from Uncategorized to Bug

comment:7 Changed 4 years ago by danny.adair@…

I don't know enough about the workings of AdminSite to create a patch, but wanted to leave a comment on what I observed during my recent investigations:

AdminSite.login_form is an AdminAuthenticationForm but the latter doesn't yet have an attribute to go the other direction and ask "its" AdminSite (more than one could share the same login_form...) for has_permission()... That could be passed into the constructor but I don't know the consequences - getting instantiated with request (cookies) and all... The template has the same problem. On a case-by-case basis I can swap one hardcoded piece of code with another, a generic solution is harder...

That said I think "only" those two locations need to be adjusted.

comment:8 Changed 4 years ago by danny.adair@…

Regarding access to has_permission() in the form, maybe give AdminAuthenticationForm a class attribute admin_site=None which gets defaulted to a contrib.admin.sites.AdminSite if it stays None? Regarding admin/base.html I have no idea how it could be told its AdminSite...

comment:9 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:10 Changed 4 months ago by tanner

  • Resolution set to duplicate
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top