Opened 16 years ago
Last modified 2 years ago
#10686 assigned New feature
Add class name interpolation in Meta.permissions codenames
Reported by: | faldridge | Owned by: | julian@pinabausch.org |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | permissions inheritance |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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:
- 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.
- 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)
Change History (31)
by , 16 years ago
Attachment: | permission_inheritance.diff added |
---|
comment:1 by , 16 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:2 by , 14 years ago
comment:3 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
by , 13 years ago
Attachment: | permission_inheritance.2.diff added |
---|
Updated patch that works on rev 16527. Also supports de-duping permissions and combining permissions from multiple inheritance on the model.
comment:6 by , 12 years ago
Triage Stage: | Design decision needed → Accepted |
---|
Seems like good changes to me.
comment:7 by , 11 years ago
I managed to write tests for the new functionality described in this ticket.
comment:9 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:10 by , 9 years ago
I'm going to apply the patch to current master and add documentation updates.
comment:11 by , 9 years ago
Needs documentation: | unset |
---|
Applied the patch to current master and I've added documentation.
comment:12 by , 9 years ago
Patch needs improvement: | set |
---|
django-developers discussion which suggests to avoid the proposed inherit_permissions
Meta
option.
comment:13 by , 8 years ago
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 by , 8 years ago
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 by , 8 years ago
Patch needs improvement: | unset |
---|
comment:16 by , 8 years ago
Summary: | 2 simple improvements to permission inheritance. → Add class name interpolation in Meta.permissions codenames |
---|
comment:20 by , 8 years ago
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 by , 8 years ago
Patch needs improvement: | set |
---|
comment:22 by , 8 years ago
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.
comment:23 by , 3 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:24 by , 2 years ago
Description: | modified (diff) |
---|
Dear community,
just picked this up while figuring out how to build permission names dynamically.
Here’s a new PR: https://github.com/django/django/pull/15936
Open issues:
- Tim Graham commented that the code may be [generating faulty permissions](https://github.com/django/django/pull/6861#issuecomment-240427426). I checked the behavior in a dummy project, but cannot confirm. I struggled to write a test for this, because it needs real migrations, real data etc. in the database. Please let me know if there’s an example in the existing tests that I can check (the advanced testing docs don’t mention “defining a real model in a testcase” either)
- Authors and version added are still missing – I’ll add this if it has a chance to get merged.
- (Changed to
class
argument tomodel_name
– I believe this is more consistent with content types & auth)
comment:25 by , 2 years ago
Description: | modified (diff) |
---|
comment:26 by , 2 years ago
Needs documentation: | set |
---|
comment:27 by , 2 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:28 by , 2 years ago
Thanks Mariusz Felisiak for being so responsive!
I’d love to hear your comment on this:
Tim Graham commented that the code may be [generating faulty permissions](https://github.com/django/django/pull/6861#issuecomment-240427426).
I checked the behavior in a dummy project, but cannot confirm. I struggled to write a test for this, because it needs real migrations, real data etc. in the
database. Please let me know if there’s an example in the existing tests that I can check (the advanced testing docs don’t mention “defining a real model
in a testcase” either)
There are quite a few nonsense commits – when you consider the main work done, please let me know. I’d like to squash the commits and add the co-authors before merging.
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.