Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#23754 closed Bug (fixed)

DisallowedModelAdminToField when using get_inline_instances()

Reported by: Tim Graham Owned by: Simon Charette
Component: contrib.admin Version: 1.4
Severity: Release blocker Keywords:
Cc: Simon Charette, Julien Phalip Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As reported in the comments of #23552:

models.py

class Invoice(models.Model):
    ...

class Payment(models.Model):
    ...

class InvoicePayment(models.Model):
    payment = models.OneToOneField(Payment)
    invoice = models.ForeignKey(Invoice)

admin.py:

class InvoicePaymentInline(admin.StackedInline):
    model = InvoicePayment
    extra = 1


class InvoiceAdmin(admin.ModelAdmin):
    #Using sample code from https://docs.djangoproject.com/en/1.7/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_inline_instances
    def get_inline_instances(self, request, obj=None):
        return [inline(self.model, self.admin_site) for inline in [InvoicePaymentInline]]


# Register your models here.
admin.site.register(Invoice, InvoiceAdmin)
admin.site.register(Payment, )

In admin, from invoice change_form, click on "+" button next payment in inlines display popup with "bad request 400"

Change History (15)

comment:1 by Simon Charette, 10 years ago

The to_field check is failing here because it only collects statically defined inline classes for its sanity checks. This could be solved by one of the following:

  1. Document that get_inline_instances() should always return instances of a subset of the classes defined in inlines.
  2. Publicly document to_field_allowed() and link to it get_inline_instances()'s documentation noting that it might require overriding if 1. criteria is not met.
  3. Completely abandon the whole static analysis against site admin registries and only rely on model relationships to allow of deny reference to fields.

Given the many regression caused by the inner admin site references enforcement I think 3. might be our best option. Thoughts?

comment:2 by Tim Graham, 10 years ago

Severity: Release blockerNormal

How about the documentation route, at least for older versions of Django? Then we can revisit the issue using 3 if you feel that will be less problematic going forward.

comment:3 by Simon Charette, 10 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned

Good idea, let's document this as a requirement for older versions of Django. I should be able to come up with a draft in the next few days.

comment:4 by Simon Charette, 10 years ago

Cc: Julien Phalip added
Has patch: set
Severity: NormalRelease blocker

#23839 was a duplicate.

After further analysis I think the correct solution would be to backport the fix proposed by @jphalip there which is to always allow reference to the primary key of the model instead to avoid documenting to_field_allowed() and expose a security foot-gun.

Since the admin doesn't currently work with many to many through a model pointing to a non-primary key field (see #23862) the only remaining edge case would be dealing with dynamically generated inlines with a foreign key pointing to a non-primary key field. For this case I suggest we follow comment:1 suggestion to document that get_inline_instances() should always return instances of a subset of the classes defined in inlines.

Since the initial to_field_allowed patch introduced many regression I'll try to get feedback from the developer mailing list before committing to this solution.

Patch should follow shortly.

comment:6 by Tim Graham <timograham@…>, 10 years ago

In e0d1f2684ae60573b209783f9fd4f9db163ad704:

Added warning about get_inline_instances() permission checking; refs #23754.

comment:7 by Tim Graham <timograham@…>, 10 years ago

In c3a40af6ec50e537b82ce6ca3acf5d665db76f29:

[1.6.x] Added warning about get_inline_instances() permission checking; refs #23754.

Backport of e0d1f2684ae60573b209783f9fd4f9db163ad704 from master

comment:8 by Tim Graham <timograham@…>, 10 years ago

In 17ffd24d9b3528c5dcb0d50e86df2b03a9b288fc:

[1.5.x] Added warning about get_inline_instances() permission checking; refs #23754.

Backport of e0d1f2684ae60573b209783f9fd4f9db163ad704 from master

comment:9 by Tim Graham <timograham@…>, 10 years ago

In f46ebc680b4868d61aec629ceded380ef313a687:

[1.7.x] Added warning about get_inline_instances() permission checking; refs #23754.

Backport of e0d1f2684ae60573b209783f9fd4f9db163ad704 from master

comment:10 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In f9c4e14aeca7df79991bca8ac2d743953cbd095c:

Fixed #23754 -- Always allowed reference to the primary key in the admin

This change allows dynamically created inlines "Add related" button to work
correcly as long as their associated foreign key is pointing to the primary
key of the related model.

Thanks to amorce for the report, Julien Phalip for the initial patch,
and Collin Anderson for the review.

comment:11 by Tim Graham <timograham@…>, 10 years ago

In 2a20bccda984e6d516bba3eb7e8f307185754a3b:

[1.7.x] Fixed #23754 -- Always allowed reference to the primary key in the admin

This change allows dynamically created inlines "Add related" button to work
correcly as long as their associated foreign key is pointing to the primary
key of the related model.

Thanks to amorce for the report, Julien Phalip for the initial patch,
and Collin Anderson for the review.

Backport of f9c4e14aeca7df79991bca8ac2d743953cbd095c from master

comment:12 by Tim Graham <timograham@…>, 10 years ago

In 2fd16232b106cf99ed42eccf57da1eed1f815d41:

[1.6.x] Fixed #23754 -- Always allowed reference to the primary key in the admin

This change allows dynamically created inlines "Add related" button to work
correcly as long as their associated foreign key is pointing to the primary
key of the related model.

Thanks to amorce for the report, Julien Phalip for the initial patch,
and Collin Anderson for the review.

Backport of f9c4e14aeca7df79991bca8ac2d743953cbd095c from master

comment:13 by Tim Graham <timograham@…>, 10 years ago

In 3d35ea43001991d94c192abca1832628cd255bb0:

[1.5.x] Fixed #23754 -- Always allowed reference to the primary key in the admin

This change allows dynamically created inlines "Add related" button to work
correcly as long as their associated foreign key is pointing to the primary
key of the related model.

Thanks to amorce for the report, Julien Phalip for the initial patch,
and Collin Anderson for the review.

Backport of f9c4e14aeca7df79991bca8ac2d743953cbd095c from master

comment:14 by Tim Graham <timograham@…>, 10 years ago

In 5940da16afb314c52cf52d4aebfedb77c6cc886b:

[1.4.x] Fixed #23754 -- Always allowed reference to the primary key in the admin

This change allows dynamically created inlines "Add related" button to work
correcly as long as their associated foreign key is pointing to the primary
key of the related model.

Thanks to amorce for the report, Julien Phalip for the initial patch,
and Collin Anderson for the review.

Backport of f9c4e14aeca7df79991bca8ac2d743953cbd095c from master

comment:15 by Simon Charette <charette.s@…>, 10 years ago

In 3a9aa155e2f7326df669953980ac87e78e932c43:

Fixed #23915 -- Made sure m2m fields through non-pk to_field are allowed in the admin.

refs #23754, #23862

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