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? |
Supersedes #92.
View the rendered RFC.