Opened 14 months ago

Last modified 6 months ago

#22828 assigned Bug

Model admins should return copies of its attributes

Reported by: vzima Owned by: ericpauley
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: timo Triage Stage: Accepted
Has patch: no 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 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.

Change History (7)

comment:1 Changed 14 months ago by ericpauley

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to ericpauley
  • Patch needs improvement unset
  • Status changed from new to assigned

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 14 months ago 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.

comment:3 Changed 14 months ago by ericpauley

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

comment:4 Changed 14 months ago 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.

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 Changed 14 months ago by timo

  • Cc timo added
  • Triage Stage changed from Unreviewed to 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 Changed 6 months 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 6 months ago by timgraham

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

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