Opened 2 weeks ago
Last modified 12 days ago
#35959 assigned Bug
Admin "Change password" Button Visible with Only "Can view user" Permission
Reported by: | Dev Namdev | Owned by: | Brock Smickley |
---|---|---|---|
Component: | contrib.admin | Version: | 5.1 |
Severity: | Normal | Keywords: | Permissions, Admin Interface, Change Password, View User, Permission Bug |
Cc: | Dev Namdev | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description
There seems to be a bug (or design oversight) in the Django admin panel where the "Change password" button is visible to users who only have the Can view user permission. According to Django's permission model, a user who can only view users should not have access to modifying user details, including changing the password.
Steps to reproduce:
- Create a superuser and a test user.
- Grant the test user only the Can view user permission.
- Log in to the admin panel as the test user.
- Navigate to the user change page for any user.
- Observe that the "Change password" button is visible despite the user having no permission to change user details.
Attachments (2)
Change History (10)
by , 2 weeks ago
Attachment: | Screenshot 2024-12-01 112851.png added |
---|
by , 2 weeks ago
Attachment: | image-20241201-113218.png added |
---|
comment:1 by , 2 weeks ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 2 weeks ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:3 by , 2 weeks ago
struggling with this one but I think I figured out how to test it!
-
tests/auth_tests/test_views.py
diff --git a/tests/auth_tests/test_views.py b/tests/auth_tests/test_views.py index 98fdfe79b7..6e1ebc2b3b 100644
a b class ChangelistTests(MessagesTestMixin, AuthViewsTestCase): 1704 1704 ), 1705 1705 html=True, 1706 1706 ) 1707 self.assertNotContains(response, '<a class="button" href="../password/">Reset password</a>') 1707 1708 # Value in POST data is ignored. 1708 1709 data = self.get_user_data(u) 1709 1710 data["password"] = "shouldnotchange"
Note to self for later: I should also probably test to make sure that the button does show for users with permission.
comment:4 by , 2 weeks ago
I speculate that the solution involves extracting the user's permission and passing it to the context in ReadOnlyPasswordHashWidget
, but I'm struggling to figure out how to do that. I was trying to figure out a way to use the auth function from context_processors.py
but the template isn't rendered by a request, right?
follow-up: 6 comment:5 by , 13 days ago
This is actually quite similar to #33171
I think maybe if the user has view only permission the password field shouldn't be available (maybe updating get_fieldsets
)
comment:6 by , 13 days ago
Replying to Sarah Boyce:
This is actually quite similar to #33171
I think maybe if the user has view only permission the password field shouldn't be available (maybe updatingget_fieldsets
)
ok, I understand how to check the user's permissions from get_fieldsets
but then how do I omit the password field from there? I tried messing around with the exclude
option as well as the raw fieldsets
tuple but I couldn't figure anything out. is there an example of something like this already in the codebase?
comment:7 by , 12 days ago
-
django/contrib/auth/admin.py
a b 1 import copy 1 2 from django.conf import settings 2 3 from django.contrib import admin, messages 3 4 from django.contrib.admin.options import IS_POPUP_VAR … … class UserAdmin(admin.ModelAdmin): 82 83 "user_permissions", 83 84 ) 84 85 86 @staticmethod 87 def _remove_fields_from_fieldsets(fieldsets, fields): 88 fieldset_without_fields = [] 89 for fieldset_name, fieldset in copy.deepcopy(fieldsets): 90 fieldset["fields"] = [f for f in fieldset["fields"] if f not in fields] 91 fieldset_without_fields.append((fieldset_name, fieldset)) 92 return fieldset_without_fields 93 85 94 def get_fieldsets(self, request, obj=None): 86 95 if not obj: 87 96 return self.add_fieldsets 88 return super().get_fieldsets(request, obj) 97 fieldsets = super().get_fieldsets(request, obj) 98 if not self.has_change_permission(request, obj): 99 return self._remove_fields_from_fieldsets( 100 fieldsets=fieldsets, 101 fields=["password"] 102 ) 103 return fieldsets 89 104 90 105 def get_form(self, request, obj=None, **kwargs): 91 106 """ -
tests/auth_tests/test_views.py
diff --git a/tests/auth_tests/test_views.py b/tests/auth_tests/test_views.py index 98fdfe79b7..e9ae523293 100644
a b class ChangelistTests(MessagesTestMixin, AuthViewsTestCase): 1692 1692 algo, salt, hash_string = u.password.split("$") 1693 1693 self.assertContains(response, '<div class="readonly">testclient</div>') 1694 1694 # ReadOnlyPasswordHashWidget is used to render the field. 1695 self.assert Contains(1695 self.assertNotContains( 1696 1696 response, 1697 1697 "<strong>algorithm</strong>: <bdi>%s</bdi>\n\n" 1698 1698 "<strong>salt</strong>: <bdi>%s********************</bdi>\n\n" … … class ChangelistTests(MessagesTestMixin, AuthViewsTestCase): 1704 1704 ), 1705 1705 html=True, 1706 1706 ) 1707 self.assertNotContains(response,'<a class="button" href="../password/">Reset password</a>') 1707 1708 # Value in POST data is ignored. 1708 1709 data = self.get_user_data(u)
Something like this maybe?
comment:8 by , 12 days ago
Has patch: | set |
---|
Thank you for the report
Although this widget was updated in #34977, this behavior existed prior to that so not a release blocker