Opened 14 years ago
Last modified 17 months ago
#13539 new Bug
The delete confirmation page does not check for object-level permissions when building the related list
Reported by: | Ion Scerbatiuc | Owned by: | |
---|---|---|---|
Component: | contrib.admin | Version: | 1.8 |
Severity: | Normal | Keywords: | delete object-level permissions |
Cc: | slav0nic@…, Sarah Boyce | 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)
Change History (21)
by , 14 years ago
Attachment: | delete_object_permissions.diff added |
---|
comment:1 by , 14 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:4 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:5 by , 13 years ago
Easy pickings: | unset |
---|---|
Owner: | changed from | to
Status: | new → assigned |
UI/UX: | unset |
by , 13 years ago
Attachment: | r16544_13539.diff added |
---|
comment:6 by , 13 years ago
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:12 by , 12 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:14 by , 9 years ago
Cc: | added |
---|
follow-up: 20 comment:15 by , 9 years ago
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 by , 9 years ago
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 by , 9 years ago
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 by , 9 years ago
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.
follow-up: 21 comment:19 by , 8 years ago
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.
comment:20 by , 4 years ago
Replying to 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 theModelBackend
is to returnFalse
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 ?
comment:21 by , 4 years ago
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 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 ?
follow-up: 23 comment:22 by , 4 years ago
IMO it is not a proper solution because user can have a permission but without access to the specific object.
comment:23 by , 4 years ago
Replying to felixxm:
IMO it is not a proper solution because user can have a permission but without access to the specific object.
I'm not sure I understand you. Doesn't the left side of the expression user.has_perm(p) or user.has_perm(p, obj)
cover having permission for the actual type (e.g. delete Blog posts) and the right side of it covers the more specialized case (delete Blog post X) ?
comment:24 by , 17 months ago
Cc: | added |
---|
This isn't going into 1.2 unless it has tests; plus, your proposed patch breaks existing tests for deletion in admin_views.