Opened 5 years ago

Closed 5 years ago

Last modified 5 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 Changed 5 years ago by Simon Charette

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

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

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

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:5 Changed 5 years ago by Simon Charette

comment:6 Changed 5 years ago by Tim Graham <timograham@…>

In e0d1f2684ae60573b209783f9fd4f9db163ad704:

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

comment:7 Changed 5 years ago by Tim Graham <timograham@…>

In c3a40af6ec50e537b82ce6ca3acf5d665db76f29:

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

Backport of e0d1f2684ae60573b209783f9fd4f9db163ad704 from master

comment:8 Changed 5 years ago by Tim Graham <timograham@…>

In 17ffd24d9b3528c5dcb0d50e86df2b03a9b288fc:

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

Backport of e0d1f2684ae60573b209783f9fd4f9db163ad704 from master

comment:9 Changed 5 years ago by Tim Graham <timograham@…>

In f46ebc680b4868d61aec629ceded380ef313a687:

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

Backport of e0d1f2684ae60573b209783f9fd4f9db163ad704 from master

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

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 Changed 5 years ago by Tim Graham <timograham@…>

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 Changed 5 years ago by Tim Graham <timograham@…>

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 Changed 5 years ago by Tim Graham <timograham@…>

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 Changed 5 years ago by Tim Graham <timograham@…>

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 Changed 5 years ago by Simon Charette <charette.s@…>

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