Opened 5 years ago

Last modified 2 years 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.2-beta
Severity: Normal Keywords: delete object-level permissions
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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/util.py 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.
Thanks!

Attachments (2)

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

Download all attachments as: .zip

Change History (14)

Changed 5 years ago by delinhabit

comment:1 Changed 5 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 4 years ago by julien

#15139 was a dupe.

comment:3 Changed 4 years ago by julien

Related issue: #11383.

comment:4 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 4 years ago by cyrus

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

Changed 4 years ago by cyrus

comment:6 Changed 4 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 4 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 4 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 https://docs.djangoproject.com/en/dev/internals/contributing/triaging-tickets/

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

comment:9 Changed 4 years ago by cyrus

  • Triage Stage changed from Accepted to Unreviewed

Sorry for that. Making unreviewed to get some attention

comment:10 Changed 4 years ago by cyrus

  • Triage Stage changed from Unreviewed to Design decision needed

comment:11 Changed 4 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: https://docs.djangoproject.com/en/dev/internals/contributing/ 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: https://groups.google.com/group/django-developers/browse_thread/thread/abc6cf0450812d82?pli=1

comment:12 Changed 2 years ago by cyrus

  • Owner cyrus deleted
  • Status changed from assigned to new
Note: See TracTickets for help on using tickets.
Back to Top