Opened 3 years ago

Last modified 3 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 Changed 3 years ago by Loic Bistuer

Component: contrib.adminDocumentation
Easy pickings: set
Needs documentation: set
Triage Stage: UnreviewedAccepted

What about password and email both of which could be used to gain access to a superuser account; in my opinion this use-case is better served by a custom UserAdmin 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.

Last edited 3 years ago by Vlada Macek (previous) (diff)

comment:2 Changed 3 years ago by Vlada Macek

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.

Last edited 3 years ago by Vlada Macek (previous) (diff)

comment:3 Changed 3 years ago by Collin Anderson

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 Changed 3 years ago by Chris Foresman

Cc: Chris Foresman added

So is this best resolved by a change in behavior, or a documentation change?

comment:5 Changed 3 years ago by Collin Anderson

Cc: cmawebsite@… added

I think if there's a really good, secure behavior change, we should do that, otherwise a documentation change.

comment:6 Changed 3 years ago by Ken Lewerentz

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 Changed 3 years ago by Remco

Owner: changed from nobody to Remco
Status: newassigned

Hi, I'm a new contributor. I would like to work on this today as part of the django sprint.

comment:8 Changed 3 years ago by Tim Graham <timograham@…>

In f6b09a7f:

Refs #23559 -- warned about consequences of letting users edit User model in admin.

comment:9 Changed 3 years ago by Tim Graham <timograham@…>

In 96bbade:

[1.7.x] Refs #23559 -- warned about consequences of letting users edit User model in admin.

Backport of f6b09a7f85c3b67b2011553838b079788c413432 from master

comment:10 Changed 3 years ago by Tim Graham <timograham@…>

In 6f555e5:

[1.8.x] Refs #23559 -- warned about consequences of letting users edit User model in admin.

Backport of f6b09a7f85c3b67b2011553838b079788c413432 from master

comment:11 Changed 3 years ago by Tim Graham

Component: Documentationcontrib.auth
Easy pickings: unset
Needs documentation: unset
Owner: Remco deleted
Status: assignednew

Updating ticket to contrib.auth component for investigation about making a change to the admin.

Note: See TracTickets for help on using tickets.
Back to Top