Opened 6 years ago

Last modified 5 years ago

#22828 assigned Bug

Model admins should return copies of its attributes

Reported by: Vlastimil Zíma Owned by: ericpauley
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Tim Graham Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


I found out there is hidden problem in ModelAdmins. 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.


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.
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 (7)

comment:1 Changed 6 years ago by ericpauley

Owner: changed from nobody to ericpauley
Status: newassigned

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?

comment:2 Changed 6 years ago by Baptiste Mispelon


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.


comment:3 Changed 6 years ago by ericpauley

Good point. I can't imagine any time when this would be unwanted behavior.

comment:4 Changed 6 years ago by Vlastimil Zíma

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
            return readonly

comment:5 Changed 6 years ago by Tim Graham

Cc: Tim Graham added
Triage Stage: UnreviewedAccepted

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 Changed 5 years ago 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

comment:7 Changed 5 years ago by Tim Graham

If you could write a patch, I'll be happy to review it.

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