[Django] #23559: Staff (not superusers) should not manage perms of Users
Django
-------------------------------+--------------------
Reporter: Tuttle | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version:
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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?
{{{#!python
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23559>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Tuttle | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
Changes (by loic):
* needs_better_patch: => 0
* component: contrib.admin => Documentation
* needs_tests: => 0
* easy: 0 => 1
* needs_docs: => 1
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:1>
Django
Reporter: Tuttle | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by Tuttle):
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 to edit other users
who possibly have any permission:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:2>
Django
Reporter: Tuttle | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by collinanderson):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:3>
Django
Reporter: Tuttle | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
* cc: foresmac (added)
Comment:
So is this best resolved by a change in behavior, or a documentation
change?
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:4>
Django
Reporter: Tuttle | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
* cc: cmawebsite@… (added)
Comment:
I think if there's a really good, secure behavior change, we should do
that, otherwise a documentation change.
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:5>
Django
Reporter: Tuttle | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by kneckinator):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:6>
Django
Reporter: Tuttle | Owner: Remco47
Type: New feature | Status: assigned
Component: Documentation | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
* owner: nobody => Remco47
* status: new => assigned
Comment:
Hi, I'm a new contributor. I would like to work on this today as part of
the django sprint.
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:7>
Django
Reporter: Tuttle | Owner: Remco47
Type: New feature | Status: assigned
Component: Documentation | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"f6b09a7f85c3b67b2011553838b079788c413432" f6b09a7f]:
{{{
#!CommitTicketReference repository=""
revision="f6b09a7f85c3b67b2011553838b079788c413432"
Refs #23559 -- warned about consequences of letting users edit User model
in admin.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:8>
Django
Reporter: Tuttle | Owner: Remco47
Type: New feature | Status: assigned
Component: Documentation | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"6f555e54f727f49ac1f4982b6e6126f3238746e4" 6f555e5]:
{{{
#!CommitTicketReference repository=""
revision="6f555e54f727f49ac1f4982b6e6126f3238746e4"
[1.8.x] Refs #23559 -- warned about consequences of letting users edit
User model in admin.
Backport of f6b09a7f85c3b67b2011553838b079788c413432 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:10>
Django
Reporter: Tuttle | Owner: Remco47
Type: New feature | Status: assigned
Component: Documentation | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"96bbade674a9bde23d54f68350f83893666f18a5" 96bbade]:
{{{
#!CommitTicketReference repository=""
revision="96bbade674a9bde23d54f68350f83893666f18a5"
[1.7.x] Refs #23559 -- warned about consequences of letting users edit
User model in admin.
Backport of f6b09a7f85c3b67b2011553838b079788c413432 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:9>
Django
Reporter: Tuttle | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* owner: Remco47 =>
* needs_docs: 1 => 0
* status: assigned => new
* component: Documentation => contrib.auth
* easy: 1 => 0
Comment:
Updating ticket to `contrib.auth` component for investigation about making
a change to the admin.
--
Ticket URL: <https://code.djangoproject.com/ticket/23559#comment:11>