#22828 closed Bug (fixed)
Model admins should return copies of its attributes
Reported by: | Vlastimil Zíma | Owned by: | Karol Alvarado |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Tim Graham, Karol Alvarado | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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:
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.
Change History (16)
comment:1 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 11 years ago
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.
comment:3 by , 11 years ago
Good point. I can't imagine any time when this would be unwanted behavior.
comment:4 by , 11 years ago
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.
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.
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
comment:5 by , 11 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
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.
comment:6 by , 10 years ago
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
comment:8 by , 4 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:9 by , 3 months ago
If the solution is to add a section/warning in the ModelAdmin docs (maybe somewhere on the top of 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.
comment:10 by , 3 months ago
Cc: | added |
---|
comment:11 by , 3 months ago
Has patch: | set |
---|---|
Owner: | set to |
Status: | new → assigned |
Triage Stage: | Accepted → Ready for checkin |
I've added a warning to the ModelAdmin docs in this PR 18617
comment:12 by , 3 months ago
Triage Stage: | Ready for checkin → Accepted |
---|
comment:13 by , 2 months ago
Patch needs improvement: | set |
---|
comment:14 by , 2 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
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. Couldcopy.deepcopy()
just be used or would that cause problems with copying too deep?