Opened 3 months ago

Last modified 3 months ago

#28517 new Cleanup/optimization

admin does not check if a model has default_permissions before raising PermissionDenied

Reported by: Paulo Owned by: nobody
Component: contrib.admin Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

28517.patch (650 bytes) - added by Paulo 3 months ago.

Download all attachments as: .zip

Change History (5)

comment:1 Changed 3 months ago by Tim Graham

Component: Uncategorizedcontrib.admin
Type: UncategorizedCleanup/optimization

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?

Changed 3 months ago by Paulo

Attachment: 28517.patch added

comment:2 Changed 3 months ago 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.

comment:3 Changed 3 months ago by Paulo

Pushed test-case with proposed solution.
https://github.com/django/django/pull/8958

comment:4 Changed 3 months ago by Tim Graham

Has patch: set
Triage Stage: UnreviewedAccepted
Note: See TracTickets for help on using tickets.
Back to Top