Model admins should return copies of its attributes

[Vlastimil Zíma]

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.

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.

[ericpauley]

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?

[Baptiste Mispelon]

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.

[ericpauley]

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

[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
        else:
            readonly.append('approved')
            return readonly

[Tim Graham]

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.

[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

[Tim Graham]

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