Opened 6 years ago

Closed 2 years ago

#14434 closed Bug (duplicate)

AdminSite should rely on self.has_permission for all permission checks

Reported by: Brandon Konkle Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Normal Keywords: admin views
Cc: Alexander Koshelev 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 Brandon Konkle 6 years ago.

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by Brandon Konkle

comment:1 Changed 6 years ago by Alexander Koshelev

Cc: Alexander Koshelev added

comment:2 Changed 6 years ago by Ramiro Morales

Triage Stage: UnreviewedAccepted

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 6 years ago by Julien Phalip

Component: Contrib appsdjango.contrib.admin

comment:4 Changed 6 years ago by Julien Phalip

Resolution: fixed
Status: newclosed

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

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

Easy pickings: unset
Resolution: fixed
Severity: Normal
Status: closedreopened
Type: Uncategorized
UI/UX: unset

comment:6 Changed 5 years ago by Chris Beaven

Needs tests: set
Patch needs improvement: set
Summary: AdminSite should check self.has_permission in self.loginAdminSite should rely on self.has_permission for all permission checks
Type: UncategorizedBug

comment:7 Changed 5 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 5 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 4 years ago by Aymeric Augustin

Status: reopenednew

comment:10 Changed 2 years ago by tanner

Resolution: duplicate
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top