Opened 10 years ago
Last modified 10 years ago
#23559 new New feature
Staff (not superusers) should not manage perms of Users
Reported by: | Vlada Macek | Owned by: | |
---|---|---|---|
Component: | contrib.auth | Version: | |
Severity: | Normal | Keywords: | |
Cc: | Chris Foresman, cmawebsite@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In our project we want to let some colleagues to see and modify several tables including the Users table. Nevertheless I don't want the colleague to be able to elevate his or anybody else's privileges.
May I submit little change of UserAdmin
similar to the following for consideration?
def get_readonly_fields(self, request, obj=None): rof = super(UserAdmin, self).get_readonly_fields(request, obj) if not request.user.is_superuser: rof += ('is_staff', 'is_superuser', 'groups', 'user_permissions') return rof
I rather doubt there is a use-case for current behaviour: Once the access to Users table is given, one can do anything.
In case the behavior change will get rejected, how about to add it as a tip in the doc?
Change History (11)
comment:1 by , 10 years ago
Component: | contrib.admin → Documentation |
---|---|
Easy pickings: | set |
Needs documentation: | set |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 10 years ago
It was only a sketch from me, but thanks for the security audit. :-) That's something not immediately apparent.
In my project I also add this to prevent staff user from editing other users who possibly have any permission:
def has_change_permission(self, request, obj=None): has = super(MyUserAdmin, self).has_change_permission(request, obj) if obj and not request.user.is_superuser: if obj != request.user: if obj.is_superuser or obj.groups.exists() or obj.user_permissions.exists(): has = False return has
Indeed, while this depends on how particular project manages users and their perms, I still humbly think there's an idea in it worth spreading.
comment:3 by , 10 years ago
I usually have this worry too about staff members being able to change their own permissions. I usually will create a separate UserAdmin (and corresponding proxy model) for the staff users to edit the fields they need to edit. I like the readonly fields idea though.
I would certainly be in favor of a good, secure example in the documentation.
comment:4 by , 10 years ago
Cc: | added |
---|
So is this best resolved by a change in behavior, or a documentation change?
comment:5 by , 10 years ago
Cc: | added |
---|
I think if there's a really good, secure behavior change, we should do that, otherwise a documentation change.
comment:6 by , 10 years ago
I think explicit is always better than implicit and as such suggest that only superusers should be able to elevate other user's permissions unless admin is configured otherwise.
But most important is to document this, even in the tutorial for new users, as this is something I find people are generally overlooking.
comment:7 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Hi, I'm a new contributor. I would like to work on this today as part of the django sprint.
comment:11 by , 10 years ago
Component: | Documentation → contrib.auth |
---|---|
Easy pickings: | unset |
Needs documentation: | unset |
Owner: | removed |
Status: | assigned → new |
Updating ticket to contrib.auth
component for investigation about making a change to the admin.
What about
password
andemail
both of which could be used to gain access to a superuser account; in my opinion this use-case is better served by a customUserAdmin
in your project where you whitelist the few fields that should be editable.I tentatively accept this as a documentation issue, we could warn about the consequences of giving edit permissions to the user model.