Re: [Django] #13539: The delete confirmation page does not check for object-level permissions when building the related list
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: | Owner: cyrus
delinhabit | Status: assigned
Type: Bug | Component: contrib.admin
Milestone: | Severity: Normal
Version: 1.2-beta | Keywords: delete object-level
Resolution: | permissions
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 1 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by cyrus):
* owner: nobody => cyrus
* status: new => assigned
* ui_ux: => 0
* easy: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: | Owner: cyrus
delinhabit | Status: assigned
Type: Bug | Component: contrib.admin
Milestone: | Severity: Normal
Version: 1.2-beta | Keywords: delete object-level
Resolution: | permissions
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 1 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (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
([attachment: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?
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:6>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: | Owner: cyrus
delinhabit | Status: assigned
Type: Bug | Component: contrib.admin
Milestone: | Severity: Normal
Version: 1.2-beta | Keywords: delete object-level
Resolution: | permissions
checkin | Needs tests: 1
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 1 |
-------------------------------------+-------------------------------------
Changes (by cyrus):
* stage: Accepted => Ready for checkin
Comment:
Making 'ready for checkin' so this ticket gets attention
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:7>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: | Owner: cyrus
delinhabit | Status: assigned
Type: Bug | Component: contrib.admin
Milestone: | Severity: Normal
Version: 1.2-beta | Keywords: delete object-level
Resolution: | permissions
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 1 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Ready for checkin => Accepted
Comment:
It isn't acceptable to mark your own ticket 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/
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:8>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: | Owner: cyrus
delinhabit | Status: assigned
Type: Bug | Component: contrib.admin
Milestone: | Severity: Normal
Version: 1.2-beta | Keywords: delete object-level
Resolution: | permissions
Unreviewed | Needs tests: 1
Patch needs improvement: 1 |
-------------------------------------+-------------------------------------
Changes (by cyrus):
* stage: Accepted => Unreviewed
Comment:
Sorry for that. Making unreviewed to get some attention
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:9>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: | Owner: cyrus
delinhabit | Status: assigned
Type: Bug | Component: contrib.admin
Milestone: | Severity: Normal
Version: 1.2-beta | Keywords: delete object-level
Resolution: | permissions
decision needed | Needs tests: 1
Patch needs improvement: 1 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by cyrus):
* stage: Unreviewed => Design decision needed
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:10>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: | Owner: cyrus
delinhabit | Status: assigned
Type: Bug | Component: contrib.admin
Milestone: | Severity: Normal
Version: 1.2-beta | Keywords: delete object-level
Resolution: | permissions
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 1 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Design decision needed => Accepted
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:11>
Django
when building the related list
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by cyrus):
* owner: cyrus =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:12>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: delinhabit | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 1.2-beta => 1.8
Comment:
#16862 was a duplicate (also had a patch).
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:13>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: delinhabit | Owner:
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: slav0nic@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:14>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: delinhabit | Owner:
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:15>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: delinhabit | Owner:
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:16>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: delinhabit | Owner:
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:17>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: delinhabit | Owner:
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:18>
Django
when building the related list
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Virtosu Bogdan):
Can't the check be changed to `user.has_perm(p) or user.has_perm(p, obj)`
?
Default backend will work as expected and custom object-level backends
will work as long as they return `False` for `obj=None`, which should
probably be the case.
This would not have any performance costs.
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:19>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: Ion Scerbatiuc | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by selected-pixel-jameson):
Is there no work around for this problem? It's pretty much preventing me
from deleting anything in my application.
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:20>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: Ion Scerbatiuc | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by selected-pixel-jameson):
Nevermind. I had this setup wrong.
Replying to [comment:20 selected-pixel-jameson]:
> Is there no work around for this problem? It's pretty much preventing me
from deleting anything in my application.
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:21>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: Ion Scerbatiuc | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Alexander Todorov):
Replying to [comment:15 Claude Paroz]:
> 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.
I'm working on a related ticket
https://code.djangoproject.com/ticket/32001 and trying out the following
patch doesn't seem to break any current tests:
{{{
--- a/django/contrib/auth/backends.py
+++ b/django/contrib/auth/backends.py
@@ -70,7 +70,7 @@ class ModelBackend(BaseBackend):
be either "group" or "user" to return permissions from
`_get_group_permissions` or `_get_user_permissions` respectively.
"""
- if not user_obj.is_active or user_obj.is_anonymous or obj is not
None:
+ if not user_obj.is_active or user_obj.is_anonymous:
return set()
}}}
Can anyone of the Django maintainers comment why the `or obj is not None`
part of this if condition is needed ? Can we remove it so we can work
towards checking object level permissions in ModelAdmin ?
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:20>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: Ion Scerbatiuc | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Alexander Todorov):
Scratch my previous comment.
I think I've figured out the reasoning behind returning empty set when obj
is passed: if you want to get all perms for the specified object then this
is an empty set() because the ModelBackend doesn't support object-level
permissions. Sounds reasonable to me but took some time to figure it out.
Replying to [comment:19 Virtosu Bogdan]:
> Can't the check be changed to `user.has_perm(p) or user.has_perm(p,
obj)` ?
I've tried this approach in https://github.com/django/django/pull/13418
and all local tests passed for me.
@felixxm I'm sorry for cross posting but these tickets are probably one
and the same issue and the latest approach seems to work. Can you comment
on that ?
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:21>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: Ion Scerbatiuc | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
IMO it is not a proper solution because user can have a permission but
without access to the specific object.
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:22>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: Ion Scerbatiuc | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Sarah Boyce (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:24>
Django
when building the related list
-------------------------------------+-------------------------------------
Reporter: Ion Scerbatiuc | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: delete object-level | Triage Stage: Accepted
permissions |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13539#comment:25>