RFC 102: Permissions registry #102
Conversation
laymonage
force-pushed
the
permissions-registry-v2
branch
3 times, most recently
from
50c9c7d to
159ed75
Compare
laymonage
force-pushed
the
permissions-registry-v2
branch
from
159ed75 to
131fd47
Compare
| | ------------------------------------------- | ----------------------- | ----------------------------------------------------------------- | | ||
| | `user_has_lock()` | - | not really a permission test, only used in `can_unlock()` | | ||
| | `page_locked()` | - | short for `page.get_lock().for_user(user)`, only used in `can_unpublish()` and `can_submit_for_moderation()` | | ||
| | `can_add_subpage()` | `add` ❓ | "subpage" is implicitly always the case, given the tree structure | |
There was a problem hiding this comment.
add vs add_subpage, do we want to differentiate the two?
The generic version of add won't require a "parent" instance, but it should be OK as the registry and BasePermissionTester interface allow both cases.
| | `user_has_lock()` | - | not really a permission test, only used in `can_unlock()` | | ||
| | `page_locked()` | - | short for `page.get_lock().for_user(user)`, only used in `can_unpublish()` and `can_submit_for_moderation()` | | ||
| | `can_add_subpage()` | `add` ❓ | "subpage" is implicitly always the case, given the tree structure | | ||
| | `can_edit()` | `edit` ❓ | `edit` vs. `change` (what the `Permission` object uses)? | |
There was a problem hiding this comment.
Currently the PermissionCheckedMixin just relies on a part of the Permission model's codename, so we use change. It's not like we have to follow that, but if we want to keep the approach of permission_required/any_permission_required of the view mixin, using edit would mean we have to tell developers to update their views. (Remember that we also changed GroupPagePermission.permission_type from using the "edit" terminology to "change" fairly recently.)
Or we can also register the same tester class for both change and edit, but I'm not sure if that's a good idea.
| | `page_locked()` | - | short for `page.get_lock().for_user(user)`, only used in `can_unpublish()` and `can_submit_for_moderation()` | | ||
| | `can_add_subpage()` | `add` ❓ | "subpage" is implicitly always the case, given the tree structure | | ||
| | `can_edit()` | `edit` ❓ | `edit` vs. `change` (what the `Permission` object uses)? | | ||
| | `can_delete(ignore_bulk=False)` | `delete` ❓ | split into `delete` and `bulk_delete`? | |
There was a problem hiding this comment.
We can split this into two separate tester classes and actions (delete and bulk_delete), but we may have to duplicate some of the logic, unless we want the tester to access the registry and make use of the other tester in the process.
| | `can_move()` | `move` | | | ||
| | `can_copy()` | `copy` | | | ||
| | `can_move_to(destination)` | `move_to` | | | ||
| | `can_copy_to(destination, recursive=False)` | `copy_to` ❓ | split into two actions for the recursive and non-recursive cases? | |
There was a problem hiding this comment.
Same as delete and bulk_delete, we can split this but we may have to duplicate some of the logic.
| | `can_copy()` | `copy` | | | ||
| | `can_move_to(destination)` | `move_to` | | | ||
| | `can_copy_to(destination, recursive=False)` | `copy_to` ❓ | split into two actions for the recursive and non-recursive cases? | | ||
| | `can_view_revisions()` | `view_revisions` | | |
There was a problem hiding this comment.
We probably also need a basic view action
|
|
||
| The generic `BasePermissionTester` is a new concept. Non-page models currently only have the permission policies – mostly used in views via the `PermissionCheckedMixin`, and sometimes with additional ad-hoc permission logic in the view code itself. The mixin only has support for the model-based methods of the permission policy, i.e. it does not check on the instance level. To fully get the benefits of adding the generic permission tester interface, we need to refactor the views to use permission testers instead of the policies. | ||
|
|
||
| Before we can use permission testers in generic views, we need to implement the generic permission tester classes for actions that are not page-specific. In simple cases, these testers will make use of the permission policy's `user_has_permission()` and `user_has_permission_for_instance()` methods. These testers will be registered with Django's base `Model` class, so it applies to all models by default (unless there's a more specific registration, e.g. with `Page`). |
There was a problem hiding this comment.
This means the tester classes would need to be aware of the registry (so that it can get the policy instance). Maybe we should use two separate registries?
|
|
||
| Before we can use permission testers in generic views, we need to implement the generic permission tester classes for actions that are not page-specific. In simple cases, these testers will make use of the permission policy's `user_has_permission()` and `user_has_permission_for_instance()` methods. These testers will be registered with Django's base `Model` class, so it applies to all models by default (unless there's a more specific registration, e.g. with `Page`). | ||
|
|
||
| For actions that require specific model mixins, they will be registered with the mixin class (e.g. the tester class for the `publish` action will be registered with `DraftStateMixin`). In some cases, the basic actions may be tested differently if the model has a specific mixin. For example, the `delete` action requires the user to also have `publish` permission if the object is currently live. This example can be implemented by having a subclass of the delete tester class that also checks for the `publish` permission and registering the class with `DraftStateMixin`, or by adding the mixin check in the generic delete tester class (TBD). |
There was a problem hiding this comment.
I'm not sure how feasible it is to register the tester classes with the mixin classes. For actions that map directly to the mixin like publish, it may be straightforward. However, for actions that need to be tested differently based on which mixins are applied (e.g. edit, delete), this may be difficult.
There can be different combinations of mixins, and the permission logic that comes with each mixin may need to take precedence differently (before, in the middle of, or after the default logic).
The simplest way would be to incorporate the mixin-specific logic directly into the base tester classes, and check with isinstance()/issubclass() to selectively do the logic. This is the same reason why the "optional features mixin" for the views was added: https://github.com/wagtail/wagtail/blob/82aa1c1a61f910cb48de5f9af549ecef96bc102a/wagtail/admin/views/generic/mixins.py#L222-L226
|
|
||
| ## Open Questions | ||
|
|
||
| 1. Naming. Permission policy and permission tester both use the term "action" to refer to the action being tested. However, they are different concepts: "action" in the permission policy relates directly to the Django `Permission` object's `codename` (e.g. `"delete"` action for `"delete_advert"` codename), while "action" in the permission tester is a more abstract concept that can be anything (e.g. `"add_subpage"`, `"move_to"`). Should we use different terms to avoid confusion? |
|
|
||
| 1. Naming. Permission policy and permission tester both use the term "action" to refer to the action being tested. However, they are different concepts: "action" in the permission policy relates directly to the Django `Permission` object's `codename` (e.g. `"delete"` action for `"delete_advert"` codename), while "action" in the permission tester is a more abstract concept that can be anything (e.g. `"add_subpage"`, `"move_to"`). Should we use different terms to avoid confusion? | ||
| - Also note that the `BasePermissionPolicy` interface's initial design doesn't strictly require the use of Django's `Permission` model. This means technically the "action" can be as abstract as it is in permission testers. However, in practice, concrete implementations of `BasePermissionPolicy` that are in use within Wagtail always use Django's `Permission` model. | ||
| 2. Should we use the same registry for both permission policies and permission testers? |
There was a problem hiding this comment.
A centralized managing registry would benefit the documentation, explaining one concept without explaining the other seems quite tricky, so might as well use a single registry.
A PermissionRegistry with two attributes policies and testers of type ObjectTypeRegistry?
|
|
||
| Most other places can be refactored in a similar way. | ||
|
|
||
| For `ModelViewSet` and its subclasses, we can change the `get_{foo}_view_kwargs()` method to _not_ pass the `permission_policy` attribute. Instead, the viewset's `permission_policy` will be registered with the registry in the viewset's `on_register()` method. The corresponding views will then automatically get the policy from the registry, using the default logic in `PermissionCheckedMixin`. |
There was a problem hiding this comment.
So supposing we do that, how can we ensure custom code (that registers policies/testers) will effectively be executed after (pasting the call trace):
register_admin_urlshookviewsets.populate()register_admin_viewsethookviewset.on_register()
|
|
||
| After creating a tester class for each action, we will register them with the base `Page` model. Then, we will refactor all usages of `PagePermissionTester`. A bulk of the work is in replacing `Page.permissions_for_user()` calls (and subsequently the methods of `PagePermissionTester`) with individual permission checks using `permission_registry.test()`. Then, the `{% page_permissions %}` template tag should be replaced with the generic `{% permissions %}` tag. After all internal usages have been replaced, the `Page.permissions_for_user()` method and the `{% page_permissions %}` template tag will be deprecated. This will be done as part of the second milestone. | ||
|
|
||
| ### Generic permission tester classes |
There was a problem hiding this comment.
I've had to fiddle with CollectionOwnershipPermissionPolicy recently, would these be also part of the new registry concept?
|
We’re looking for reviewers for this, to get this through and approved. Anyone up for joining a review session for this? |
|
I don't think I have the technical depth on this topic to add a useful review or to add ideas, but from what I understand this is a lot more simple and the sites I've worked on could benefit from this approach. Big fan! 💯 |
| 1. Naming. Permission policy and permission tester both use the term "action" to refer to the action being tested. However, they are different concepts: "action" in the permission policy relates directly to the Django `Permission` object's `codename` (e.g. `"delete"` action for `"delete_advert"` codename), while "action" in the permission tester is a more abstract concept that can be anything (e.g. `"add_subpage"`, `"move_to"`). Should we use different terms to avoid confusion? | ||
| - Also note that the `BasePermissionPolicy` interface's initial design doesn't strictly require the use of Django's `Permission` model. This means technically the "action" can be as abstract as it is in permission testers. However, in practice, concrete implementations of `BasePermissionPolicy` that are in use within Wagtail always use Django's `Permission` model. | ||
| 2. Should we use the same registry for both permission policies and permission testers? | ||
| - They do not need to be in the same registry, but having two separate registries may be confusing. |
There was a problem hiding this comment.
Hello Sage, thanks for the RFC. Great work.
Wagtail has a model that only exist to create a Django permission. Should the removal of Admin model be mentioned in this RFC? This is the one model/permission I know, maybe there are others?
https://github.com/wagtail/wagtail/blob/main/wagtail/admin/models.py#L19-L27
Have you considered Django AUTHENTICATION_BACKENDS for permission checks?
The django.contrib.auth.backends.ModelBackend.has_perm signature accepts an obj. Which means Django with a custom authentication backend can handle permissions per instance. Maybe it is good to stay close to Django permission machinery? And if not, mention why it is discarded.
At work we sometimes use Wagtail Admin and Django admin in parallel. It would be weird if there is a mismatch in operations a user can perform. Django Admin will not use this Wagtail registry.
Bulk actions and list views have an N+1 risk when the test method hits the db. We might want to document an example where we shift the heavy lifting to get_queryset/database. For example: annotate the queryset first and test against the annotated value.
What about assigning permissions to a group or user? If we want to assign a permission to a user, the permission needs to live as database object too.
I just wanted to share these thoughts. None of this is meant as criticism, just context and considerations that might be useful. Feel free to ignore any of it! I like the concept of a Wagtail permissions registry. 👍
6 participants
Supersedes #92.
View the rendered RFC.