Opened 6 years ago

Last modified 11 months ago

#13539 new Bug

The delete confirmation page does not check for object-level permissions when building the related list

Reported by: delinhabit Owned by:
Component: contrib.admin Version: 1.8
Severity: Normal Keywords: delete object-level permissions
Cc: slav0nic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no


I implemented a custom authentication backend for providing object level permissions. It's all working fine, except the delete confirmation page for a particular object.
I found that when building the related objects list for the confirmation page, the permissions are checked only for the model itself and not the object being processed.
In django/contrib/admin/ at the 77th line you can see this check:

    if not user.has_perm(p):

which should be:

    if not user.has_perm(p, obj):

I'm attaching a patch for this. I hope that this fix will be included in the 1.2 final release.

Attachments (2)

delete_object_permissions.diff (555 bytes) - added by delinhabit 6 years ago.
r16544_13539.diff (2.0 KB) - added by cyrus 5 years ago.

Download all attachments as: .zip

Change History (20)

Changed 6 years ago by delinhabit

comment:1 Changed 6 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

This isn't going into 1.2 unless it has tests; plus, your proposed patch breaks existing tests for deletion in admin_views.

comment:2 Changed 6 years ago by julien

#15139 was a dupe.

comment:3 Changed 6 years ago by julien

Related issue: #11383.

comment:4 Changed 5 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 5 years ago by cyrus

  • Easy pickings unset
  • Owner changed from nobody to cyrus
  • Status changed from new to assigned
  • UI/UX unset

Changed 5 years ago by cyrus

comment:6 Changed 5 years ago by cyrus

While investigating the failing test, I came across an interesting thing: if you have a backend that does not support object-level permissions (like the default ModelBackend) a user will not have permissions for an object even if that user have permissions for the model itself. I think this is weird, because philosophically if I have delete permissions for MyModel I should also have delete permissions for an object of MyModel if my authentication back-end does not enforce object-level permissions.

Attached you can find a patch for this issue (r16544_13539.diff). Note that this patch make the following tests to fail:

  • django.contrib.auth.tests.auth_backends.BackendTest.test_has_no_object_perm
  • django.contrib.auth.tests.auth_backends.NoInActiveUserBackendTest.test_has_perm
  • django.contrib.auth.tests.auth_backends.RowlevelBackendTest.test_has_perm

I think the reason for this is that those tests were written with this assumption in mind. I can fix the failing tests and create additional tests to regress this issue, but I want to discuss first. Any thoughts?

comment:7 Changed 5 years ago by cyrus

  • Triage Stage changed from Accepted to Ready for checkin

Making 'ready for checkin' so this ticket gets attention

comment:8 Changed 5 years ago by aaugustin

  • Triage Stage changed from Ready for checkin to Accepted

It isn't acceptable to mark your own patch as RFC — and RFC doesn't mean "look at this patch!".

Please review the workflow described on

Last edited 5 years ago by aaugustin (previous) (diff)

comment:9 Changed 5 years ago by cyrus

  • Triage Stage changed from Accepted to Unreviewed

Sorry for that. Making unreviewed to get some attention

comment:10 Changed 5 years ago by cyrus

  • Triage Stage changed from Unreviewed to Design decision needed

comment:11 Changed 5 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

cyrus, triage stages aren't designed to "get attention"; they are set to allow developers to know what's the current status of a given ticket. Imagine the mess that would result if everyone changed triages stages arbitrarily!

Please, read the contribution guidelines: The FAQ describes what to do when a ticket doesn't move forward as fast as you'd like.

If you desperately need someone to look at this ticket, here's a solution:

comment:12 Changed 4 years ago by cyrus

  • Owner cyrus deleted
  • Status changed from assigned to new

comment:13 Changed 12 months ago by timgraham

  • Version changed from 1.2-beta to 1.8

#16862 was a duplicate (also had a patch).

comment:14 Changed 12 months ago by slav0nic

  • Cc slav0nic@… added

comment:15 Changed 12 months ago by claudep

I took some time today to examine this issue and found that we're still confronted to the problem mentioned by cyrus on comment:6. If we suddenly pass an object to has_perm, False is returned because the default behavior of the ModelBackend is to return False when any object is passed (see #12462).

So the first step to solve this issue would be to revert that, which might bring backwards incompatibilities. Unfortunately, ticket #12462 doesn't explain the rationale behind that code.

comment:16 Changed 11 months ago by claudep

Discussed with apollo13 on IRC, summary:
We should have always returned True in the first place, agreed. Now the path forward is very tricky. Just changing the return value to True now is very dangerous in a security point of view, because if someone has a different backend added to the default ModelBackend, has_perm() will suddenly change its behavior and always return True as the True value has priority (unless the custom backend is first and returns PermissionDenied (#15716), but we can count on that being always implemented this way).

In the longer term, splitting the authentication/authorization steps in different backends might allow us to change the current situation.

comment:17 Changed 11 months ago by claudep

One possible solution at "shorter" term would be to add a new default backend (say ModelBackendNG) which always return True for object permissions, and deprecate the previous ModelBackend. Then only after the ModelBackend is removed at the end of the deprecation period, we could then pass obj to has_perm in the admin.

comment:18 Changed 11 months ago by claudep

Another solution: create a new DefaultObjPermsBackend (returning True for object permissions) which is added by default in AUTHENTICATION_BACKENDS. With that solution, we could add the obj param to has_perm in the admin immediately.

Projects with default AUTHENTICATION_BACKENDS will work fine as before.
Projects with customized AUTHENTICATION_BACKENDS will probably see some unauthorized object access in the admin, and will have to manually add the DefaultObjPermsBackend in their customized AUTHENTICATION_BACKENDS setting. This is a bit annoying for them, but at least they will have to think about the issue and won't get unexpected new object permissions if they implemented object permissions in their custom backend.

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