[Django] #28517: admin does not check if a model has default_permissions before raising PermissionDenied
Django
PermissionDenied
-----------------------------------------+------------------------
Reporter: Paulo | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Hello,
Given the following models:
{{{
class Product(models.Model):
name = models.CharField(max_length=200)
def __str__(self):
return self.name
class ProductMeta(models.Model):
name = models.CharField(max_length=200)
product = models.ForeignKey(to=Product, on_delete=models.CASCADE)
class Meta:
default_permissions = ()
def __str__(self):
return self.name
class InternalProduct(Product):
class Meta:
proxy = True
default_permissions = ()
}}}
Assumptions:
* All models are registered with the admin.
* Current user is staff with all permissions (non-superuser).
Create Product instances:
* product_a = Product.objects.create(name='product a')
* product_b = Product.objects.create(name='product b')
Create ProductMeta instances:
* ProductMeta.objects.create(name='product a metadata', product=product_a)
* ProductMeta.objects.create(name='product b metadata', product=product_b)
The following two scenarios will fail:
* User tries to delete a Product from admin.
* User tries to delete an InternalProduct from admin.
The first scenario fails because the admin checks if user has delete
permission on the collected objects without checking if the delete
permission is present on the default_permissions for the collected models.
The second scenario fails because the has_x_permission checks assume
add/change/delete are present in default_permissions.
--
Ticket URL: <https://code.djangoproject.com/ticket/28517>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
PermissionDenied
Reporter: Paulo | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Cleanup/optimization
* component: Uncategorized => contrib.admin
Comment:
The documentation for `Options.default_permissions` says, "You may
customize this list, for example, by setting this to an empty list if your
app doesn’t require any of the default permissions." Since the admin app
requires those permissions, it seems that setting `default_permissions` to
an empty list would be invalid for this case.
Do you have a use case and proposal for changing the behavior?
--
Ticket URL: <https://code.djangoproject.com/ticket/28517#comment:1>
Django
PermissionDenied
Reporter: Paulo | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Paulo):
* Attachment "28517.patch" added.
Django
PermissionDenied
Reporter: Paulo | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Paulo):
Because the admin allows us to override the has_x_permission methods, I
think the main problem is the delete check buried in get_deleted_objects.
The main use-case I see is to support creating proxy models that need to
override parts of the admin for the base model but do not need their own
permissions.
This is achievable by setting default_permissions to empty on the proxy
model and pointing all has_x_permission methods to the same logic from the
base model.
I've attached a patch (untested) which I think fixes this. I can push a PR
later today with tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/28517#comment:2>
Django
PermissionDenied
Reporter: Paulo | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Paulo):
Pushed test-case with proposed solution.
https://github.com/django/django/pull/8958
--
Ticket URL: <https://code.djangoproject.com/ticket/28517#comment:3>
Django
PermissionDenied
Reporter: Paulo | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/28517#comment:4>
Django
PermissionDenied
Reporter: Paulo | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"d1286a8a689e31435c07534fee7b61f41cea37f8" d1286a8a]:
{{{
#!CommitTicketReference repository=""
revision="d1286a8a689e31435c07534fee7b61f41cea37f8"
Fixed #28517 -- Fixed admin delete confirmation view crash when related
models don't have a delete permission.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28517#comment:5>