Opened 7 years ago

Last modified 8 days ago

#10686 assigned New feature

Add class name interpolation in Meta.permissions codenames

Reported by: faldridge Owned by: sergei-maertens
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: permissions inheritance
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I've got a patch for a slight behavior modification that I needed and that might be useful for others, and I wanted to collect some thoughts on it before writing up the regression tests and documentation changes.

Twice now, I've come across a situation where the default Django behavior for inheriting permissions is inappropriate for my security model.

Here's the situation: I have a permission on an abstract base model class that I want all child classes to inherit, and I want to then append specific permission(s) to one or more of the children.

Example:

class MyAppBaseModel(models.Model):
    class Meta:
        abstract = True
        permissions = (("view_%(class)s", "Can view %(class)s"),)

class ChildModel(MyAppBaseModel):
    class Meta:
        permissions =  (("foobar_childmodel", "Can foobar childmodel"),)

Two problems arise:

  1. Although permissions currently may be inherited, the Options class does not currently implement %(class)s replacement like the RelatedField class does, so my permissions end up actually being stored in the database with %(class)s in the name and codename.
  2. The way Meta attributes are currently processed in the ModelBase metaclass causes inherited permissions to be completely replaced if any explicit permissions are defined on the child class. So instead of can_view and can_foobar on ChildModel, I only get can_foobar.

This patch changes Django's behavior such that any explicit child class permissions would be appended to the inherited ones, rather than completely replacing them.

Also, I've added a backwards-compatible flag to the Meta options, 'inherit_permissions'. This flag would only be required in the case that one wanted Django's current behavior which is to discard base class permissions when explicit permissions are declared on the child class.

Example:

class MyAppBaseModel(models.Model):
    class Meta:
        abstract = True
        permissions = (("view_%(class)s", "Can view %(class)s"),)

class ChildModel(MyAppBaseModel):
    class Meta:
        permissions =  (("foobar_childmodel", "Can foobar childmodel"),)
        inherit_permissions = False

This would result in ChildModel only having the can_foobar permission (Django's current behavior). If you wanted to inherit/append the view_class
permission instead (proposed behavior), you could set the attribute to True or leave it out entirely.

This, of course, assumes that my desired behavior is what most other people would want. I suspect, but am not certain that this is the case.

Though a small change, I believe it requires a design decision.

Thanks!

Attachments (2)

permission_inheritance.diff (2.7 KB) - added by faldridge 7 years ago.
permission_inheritance.2.diff (2.9 KB) - added by Soby 5 years ago.
Updated patch that works on rev 16527. Also supports de-duping permissions and combining permissions from multiple inheritance on the model.

Download all attachments as: .zip

Change History (24)

Changed 7 years ago by faldridge

comment:1 Changed 7 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 6 years ago by anonymous

I think both of these improvements need to be implemented. I make heavy use of abstract classes and writing the permissions for each child model is a lot of redundant coding.

comment:3 Changed 5 years ago by sethtrain

  • Severity set to Normal
  • Type set to New feature

Changed 5 years ago by Soby

Updated patch that works on rev 16527. Also supports de-duping permissions and combining permissions from multiple inheritance on the model.

comment:4 Changed 5 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:5 Changed 5 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:6 Changed 3 years ago by akaariai

  • Triage Stage changed from Design decision needed to Accepted

Seems like good changes to me.

comment:7 Changed 3 years ago by derega

I managed to write tests for the new functionality described in this ticket.

https://github.com/derega/django/tree/ticket_10686

comment:8 Changed 3 years ago by timo

  • Needs documentation set

These changes also need to be documented.

comment:9 Changed 7 months ago by sergei-maertens

  • Owner changed from nobody to sergei-maertens
  • Status changed from new to assigned

comment:10 Changed 7 months ago by sergei-maertens

I'm going to apply the patch to current master and add documentation updates.

comment:11 Changed 7 months ago by sergei-maertens

  • Needs documentation unset

Applied the patch to current master and I've added documentation.

comment:12 Changed 7 months ago by timgraham

  • Patch needs improvement set

django-developers discussion which suggests to avoid the proposed inherit_permissions Meta option.

comment:13 Changed 8 weeks ago by sergei-maertens

I'm starting work on this again such that inherit_permissions is not needed.

Models inheriting from abstract models inherit the Meta, so unless permissions are being added in the concrete model, there is no boilerplate required. In the event that the concrete class also has custom permissions, the pythonic way is the way to go:

class Task(BaseTask):
    ...

    class Meta(BaseTask.Meta):
        permissions = BaseTask.Meta.permissions + (
            ('reassign_assignedtask', 'Can reassign assigned task')
        )

comment:14 Changed 8 weeks ago by sergei-maertens

Addition: it's not possible to inherit a concrete class' Meta per https://docs.djangoproject.com/en/1.9/topics/db/models/#meta-and-multi-table-inheritance

comment:15 Changed 8 weeks ago by sergei-maertens

  • Patch needs improvement unset
Last edited 7 weeks ago by timgraham (previous) (diff)

comment:16 Changed 7 weeks ago by timgraham

  • Summary changed from 2 simple improvements to permission inheritance. to Add class name interpolation in Meta.permissions codenames

comment:17 Changed 7 weeks ago by timgraham

  • Patch needs improvement set

Comments for improvement on the PR.

comment:18 Changed 4 weeks ago by sergei-maertens

  • Patch needs improvement unset

Processed the remarks, up for re-review

comment:19 Changed 12 days ago by timgraham

  • Patch needs improvement set

Left some more comments.

comment:20 Changed 12 days ago by sergei-maertens

  • Patch needs improvement unset

Processed comments - 1 comment was replied to on Github & no change made in docs: https://github.com/django/django/pull/6861#discussion-diff-74686863

comment:21 Changed 8 days ago by timgraham

  • Patch needs improvement set

comment:22 Changed 8 days ago by sergei-maertens

Note to self: the unit test should check that the correct permissions are created in the db, per:

https://github.com/django/django/pull/6861#issuecomment-240427426

Worked on some cosmetic edits: http://dpaste.com/19R4PZE but I also noticed that this doesn't seem to create the correct permission in the database. For the pizza example in the docs I see things like "can_deliver_base" in the database.

Note: See TracTickets for help on using tickets.
Back to Top