[Django] #23754: DisallowedModelAdminToField when using get_inline_instances()
Django
-------------------------------------------+------------------------
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.4
Severity: Release blocker | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
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"
--
Ticket URL: <https://code.djangoproject.com/ticket/23754>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:1>
Django
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* severity: Release blocker => Normal
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:2>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* owner: nobody => charettes
* status: new => assigned
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:3>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* cc: jphalip (added)
* has_patch: 0 => 1
* severity: Normal => Release blocker
Comment:
#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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:4>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
Here's [https://groups.google.com/forum/#!topic/django-
developers/GdnqvntKXoQ the mailing list thread].
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:5>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"e0d1f2684ae60573b209783f9fd4f9db163ad704"]:
{{{
#!CommitTicketReference repository=""
revision="e0d1f2684ae60573b209783f9fd4f9db163ad704"
Added warning about get_inline_instances() permission checking; refs
#23754.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:6>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"17ffd24d9b3528c5dcb0d50e86df2b03a9b288fc"]:
{{{
#!CommitTicketReference repository=""
revision="17ffd24d9b3528c5dcb0d50e86df2b03a9b288fc"
[1.5.x] Added warning about get_inline_instances() permission checking;
refs #23754.
Backport of e0d1f2684ae60573b209783f9fd4f9db163ad704 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:8>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"c3a40af6ec50e537b82ce6ca3acf5d665db76f29"]:
{{{
#!CommitTicketReference repository=""
revision="c3a40af6ec50e537b82ce6ca3acf5d665db76f29"
[1.6.x] Added warning about get_inline_instances() permission checking;
refs #23754.
Backport of e0d1f2684ae60573b209783f9fd4f9db163ad704 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:7>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f46ebc680b4868d61aec629ceded380ef313a687"]:
{{{
#!CommitTicketReference repository=""
revision="f46ebc680b4868d61aec629ceded380ef313a687"
[1.7.x] Added warning about get_inline_instances() permission checking;
refs #23754.
Backport of e0d1f2684ae60573b209783f9fd4f9db163ad704 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:9>
Django
Reporter: timgraham | Owner: charettes
Component: contrib.admin | Version: 1.4
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f9c4e14aeca7df79991bca8ac2d743953cbd095c"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:10>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"2a20bccda984e6d516bba3eb7e8f307185754a3b"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:11>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"2fd16232b106cf99ed42eccf57da1eed1f815d41"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:12>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"3d35ea43001991d94c192abca1832628cd255bb0"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:13>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5940da16afb314c52cf52d4aebfedb77c6cc886b"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:14>
Django
Reporter: timgraham | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"3a9aa155e2f7326df669953980ac87e78e932c43"]:
{{{
#!CommitTicketReference repository=""
revision="3a9aa155e2f7326df669953980ac87e78e932c43"
Fixed #23915 -- Made sure m2m fields through non-pk to_field are allowed
in the admin.
refs #23754, #23862
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23754#comment:15>