[Django] #22828: Model admins should return copies of its attributes
Django
-------------------------------+--------------------
Reporter: vzima | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
I found out there is hidden problem in `ModelAdmin`s. Getters returns
directly attributes and not their copies. This has a hidden danger in
cases when you override the getters. You can easily change the
configuration of the model admin.
Example:
{{{
#!python
from django.contrib.admin.options import BaseModelAdmin
class MyAdmin(BaseModelAdmin):
readonly_fields = ['foo', ]
admin = MyAdmin()
rf = admin.get_readonly_fields(None)
# For example, but it can very easily happen in the method override.
rf.append('bar')
MyAdmin.readonly_fields #>>> ['foo', 'bar']
}}}
=== Affected attributes ===
* fieldsets
* fileds
* ordering
* readonly_fields
* prepopulated_fields
* list_display
* list_display_links
* list_filter
* search_fields
Django should return copies in getters of these attributes to avoid
unwanted changes of the `ModelAdmin` at runtime.
--
Ticket URL: <https://code.djangoproject.com/ticket/22828>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: vzima | Owner: ericpauley
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by ericpauley):
* owner: nobody => ericpauley
* needs_docs: => 0
* status: new => assigned
* needs_tests: => 0
* needs_better_patch: => 0
Comment:
I'm taking a look at this and trying to figure out the best way to deal
with `get_fieldsets()`, since just copying the list wouldn't really solve
the problem. Could `copy.deepcopy()` just be used or would that cause
problems with copying too deep?
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:1>
Django
Reporter: vzima | Owner: ericpauley
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bmispelon):
Hi,
I'm ont too convinced that putting a bunch of `copy.deepcopy` is a viable
solution to this.
What's your use-case for manipulating `ModelAdmin` instances in such a
fashion? It doesn't seem very typical to me.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:2>
Django
Reporter: vzima | Owner: ericpauley
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by ericpauley):
Good point. I can't imagine any time when this would be unwanted behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:3>
Django
Reporter: vzima | Owner: ericpauley
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vzima):
I see more than a few examples.
Example 1: You can have admin, who can edit all of the user's data, and
people from personal department, who can't edit username and permissions.
{{{
#!python
class UserAdmin(ModelAdmin):
readonly_fields = ['uuid'] # There can be other fields, e.g.
identifiers to some other databases
def get_readonly_fields(self, request, obj=None):
readonly = super(UserAdmin, self).get_readonly_fields(request,
obj=obj)
if is_admin(request.user):
return readonly
elif is_personal_dept(request.user):
readonly += ['permissions', 'groups'] # This will edit the
class, so it will become readonly for everybody
return readonly
# Methods `has_*_permission` are modified appropriately
}}}
Example 2: Anything that needs somebody else's approval, e.g. vacation.
{{{
#!python
class VacationAdmin(ModelAdmin):
readonly_fields = ['uuid', 'fields_for_accounting']
def get_readonly_fields(self, request, obj=None):
readonly = super(VacationAdmin, self).get_readonly_fields(request,
obj=obj)
if is_admin(request.user) or is_boss(request.user):
return readonly
else:
readonly.append('approved')
return readonly
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:4>
Django
Reporter: vzima | Owner: ericpauley
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* cc: timo (added)
* stage: Unreviewed => Accepted
Comment:
I guess this hasn't come up before because I think most apps probably
don't define both the attribute and the method versions, but I can see it
could be useful.
I am not sure if the overhead of copying the attribute all the time is
worth it (since it probably doesn't matter for most users), but if not, we
should at least document the caveat.
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:5>
Django
Reporter: vzima | Owner: ericpauley
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by synotna):
This bug just hit me: I had set my base fields/readonly_fields on the
ModelAdmin, and overwrote get_fields & get_readonly_fields thinking it
would /always/ modify the base fields
I highly recommend explaining in the docs for the getters that if you
overwrite them, it should be to replace the setting of the fields directly
on the ModelAdmin
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:6>
Django
Reporter: vzima | Owner: ericpauley
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
If you could write a patch, I'll be happy to review it.
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:7>
Django
Reporter: Vlastimil Zíma | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by Karol Alvarado):
If the solution is to add a section/warning in the ModelAdmin docs (maybe
somewhere on the top of
[https://docs.djangoproject.com/en/5.1/ref/contrib/admin/#modeladmin-
methods ModelAdmin methods section]) about that behaviour and then point
all of the affected getters to that warning, then I'd love to give it a
go.
I played with the issue described a bit, and although I have never
encountered it myself before, it does feel like something that has to be
noted because it's confusing.
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:9>
Django
Reporter: Vlastimil Zíma | Owner: (none)
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Karol Alvarado):
* cc: Karol Alvarado (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:10>
Django
Reporter: Vlastimil Zíma | Owner: Karol
| Alvarado
Type: Bug | Status: assigned
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Karol Alvarado):
* has_patch: 0 => 1
* owner: (none) => Karol Alvarado
* stage: Accepted => Ready for checkin
I've added a warning to the ModelAdmin docs in this PR
[https://github.com/django/django/pull/18617 18617]
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:11>
Django
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Changes (by Karol Alvarado):
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:12>
Django
Reporter: Vlastimil Zíma | Owner: Karol Alvarado
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Sarah Boyce):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:13>
Django
| Alvarado
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
| checkin
Changes (by Sarah Boyce):
* needs_better_patch: 1 => 0
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:14>
Django
Reporter: Vlastimil Zíma | Owner: Karol
| Alvarado
Component: contrib.admin | Version: dev
Severity: Normal | Resolution: fixed
| checkin
Has patch: 1 | Needs documentation: 0
Changes (by Sarah Boyce <42296566+sarahboyce@…>):
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"b8e9cdf13b7ab6621926a5d2aad3e2bb745aae00" b8e9cdf]:
{{{#!CommitTicketReference repository=""
revision="b8e9cdf13b7ab6621926a5d2aad3e2bb745aae00"
Fixed #22828 -- Warned that ModelAdmin get hooks return the property
itself rather a copy.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:15>
Django
Reporter: Vlastimil Zíma | Owner: Karol
| Alvarado
Type: Bug | Status: closed
Component: contrib.admin | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Comment (by Sarah Boyce <42296566+sarahboyce@…>):
In [changeset:"34989e076b639ee1c67cbabb426a35ceaaa83760" 34989e07]:
{{{#!CommitTicketReference repository=""
revision="34989e076b639ee1c67cbabb426a35ceaaa83760"
[5.1.x] Fixed #22828 -- Warned that ModelAdmin get hooks return the
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22828#comment:16>