[Django] #20151: get_deleted_objects does not check permissions on proxy model objects without ModelAdmin

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin
-------------------------+-------------------------------------------------
Reporter: | Owner: nobody
anonymous | Status: new
Type: Bug | Version: 1.5
Component: | Keywords: ModelAdmin; get_deleted_objects;
contrib.admin | proxy
Severity: Normal | Has patch: 0
Triage Stage: | UI/UX: 0
Unreviewed |
Easy pickings: 0 |
-------------------------+-------------------------------------------------
When deleting an object through django admin interface, only related
objects with Admin pages are checked for delete permissions. The
following objects are NOT checked for delete permissions:

* proxy models with no ModelAdmin (even if the concrete model as an admin
page)
* models with InlineAdmin

--
Ticket URL: <https://code.djangoproject.com/ticket/20151>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage:
get_deleted_objects; proxy | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by charettes):

* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0

Comment:

The proxy model issue is somehow related to #11154 -- if proxy permissions
were created we could check them just like any other model.

I'm not sure
[https://github.com/django/django/blob/2b48fcc607010065c0f8107baf669dd41b164f3c/django/contrib/admin/util.py#L121-L124
we should checks for delete permission on objects which model is not
registered to the current admin site]. If we don't why aren't we relying
on `ModelAdmin.has_delete_permission` and do the same with inlines?

The only drawback is that you '''must''' register your model in order to
get deletion permission checks. This should be documented at least.

IMO checks should be made this way:

1. If a `ModelAdmin` has been registered for this model or an
`InlineAdmin` matches the relationship collected it should be used.
2. Else if the model is a proxy attempt '''1.''' with the the model it's
proxying (allow multiple levels of proxying).
3. Else fallback on user has_perm.

--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:1>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:

Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by charettes):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:2>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jerlo F.
| De Leon
Type: Bug | Status: assigned

Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy |

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jerlo F. De Leon):

* has_patch: 0 => 1
* owner: nobody => Jerlo F. De Leon
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:3>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jerlo F.
| De Leon
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jerlo F. De Leon):

So, I have reproduced this on the current main branch and submitted a
[https://github.com/django/django/pull/21058 Pull Request]

Following the excellent insight from Simon Charette (Comment 1)

This patch implements the suggested permission hierarchy:
1. Checks the concrete model's admin first for has_delete_permission.

2. If not found (or not registered), it recursively checks proxy levels.

3. It finally falls back to a global user permission check for the
specific proxy.

The fix ensures get_deleted_objects correctly checks permissions for proxy
models, even if they aren't registered in the admin.
I also added regression tests in tests/admin_utils/tests.py." which
verifies that perms_needed correctly identifies missing proxy permissions
to block unauthorized deletions.
--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:4>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jerlo F.
| De Leon
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by James Osgood):

As I remarked on the PR
(https://github.com/django/django/pull/21058#pullrequestreview-4134066520),
following from the resolution of #11154, proxy model permissions use their
own ContentType, rather than their parents' ContentType. So the concrete
model check suggested by Simon Charette is no longer applicable.

The fallback to user.has_perm is still applicable, as it implements the
missing part of "models that the user doesn’t have permission to delete"
for perms_needed as mentioned in the
[https://docs.djangoproject.com/en/6.0/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_deleted_objects
docs for ModelAdmin.get_deleted_objects]. However, this is a change in
behaviour, rather than a strict bug-fix, as admin users may no longer be
able to delete objects they previously could; so it must be documented as
such.

Alternatively, if it is decided that the existing behaviour should be
maintained, then the documentation for ModelAdmin.get_deleted_objects
should be updated to specify that the permission check applies only to
models registered in the admin. E.g.
"perms_needed is a set of verbose_names of the models ''registered in the
admin'' that the user doesn’t have permission to delete."

Personally, I think the user.has_perm check is appropriate, as it matches
the expectations set forth by the documentation. Not respecting the
permissions for models being deleted seems like a security oversight.
--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:5>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jerlo F.
| De Leon
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy |

Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by James Osgood):

* cc: James Osgood (added)
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:6>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jerlo F.
| De Leon
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jerlo F. De Leon):

I have opened a new PR (https://github.com/django/django/pull/21127) to
address the feedback, as the original PR
(https://github.com/django/django/pull/21058) was closed by the GitHub
Actions bot.

As you suggested, I moved the tests to
tests.admin_views.test_actions.AdminActionsTest. The new test case now
verifies that deleting a Persona (registered) is blocked when the user
lacks delete permissions for the related FooAccount (not registered),
confirming the expected HTML output and ensuring the deletion does not
occur on POST.

I have also added the .. versionchanged:: documentation to
ref/contrib/admin/index.txt to note the behavior change regarding
permission checks for non-registered models.
--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:7>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jerlo F.
| De Leon
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy |

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jerlo F. De Leon):

* needs_better_patch: 1 => 0
* needs_docs: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:8>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jerlo F.
| De Leon
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by James Osgood):

Thank you Jerlo for making the new PR. The structure of your solution is
much better, especially the documentation and test. I reviewed
(https://github.com/django/django/pull/21127#pullrequestreview-4135634159)
- it needs just a bit more work.
--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:9>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jerlo F.
| De Leon
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by James Osgood):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:10>

[Django]

#20151: get_deleted_objects does not check permissions on proxy model objects
without ModelAdmin

-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jerlo F.
| De Leon
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: ModelAdmin; | Triage Stage: Accepted
get_deleted_objects; proxy |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by James Osgood):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/20151#comment:11>