RFC 92: Permissions registry

[laymonage]

Rendered

This RFC proposes a new permissions registry that maps a model class to a permission configuration. Everywhere a permission check is performed in Wagtail, the registry will be consulted to determine the configuration to use for the model or model instance in question. The registry will make it easier for developers to access the permission configuration of a model from anywhere in the code. It will also allow developers to define custom permission logic for both custom models and Wagtail's built-in models, to be used in place of the default.

The main question in this RFC is what will be stored by the registry as the main permission configuration for a given model. Several options are outlined in the RFC. After writing this, I'm leaning towards option B, but I'm keen to hear your thoughts.

[SebCorbin]

Thanks for this RFC, although it's a lot of content, it's a pretty clear vision of implementation. I'm also leaning towards option B, and I have some more questions :

• would it affect/improve the way permissions are checked in templates (in regard of an instance, notably)?
• would it affect/improve the way permissions are configured in groups?

[laymonage]

Thanks for this RFC, although it's a lot of content, it's a pretty clear vision of implementation. I'm also leaning towards option B, and I have some more questions :

Thanks @SebCorbin!

• would it affect/improve the way permissions are checked in templates (in regard of an instance, notably)?

I believe so. I'm thinking we would introduce a generic {% permissions %} tag that would accept an object and a user instance. This will replace Wagtail's existing {% page_permissions %}, so you can do:

{% comment %}
  Maybe also allow `user` to be inferred from `request.user`
  if `request` is available in the context, like the `page_permissions` tag does.
{% endcomment %}

{% permissions obj user as obj_perms %}
{% if obj_perms.can_edit %}
  ...
{% endif %}

• would it affect/improve the way permissions are configured in groups?

Not really, although I think this would open up the possibility of developers implementing instance-based and or user-based permissions for custom models, as the permission checks in Wagtail's built-in views will respect what's in the registry. Combined with the ability to customise the group create/edit views, developers can render their own permissions panel to e.g. add instance-based permissions configuration. Maybe we'll allow the user create/edit views to be customisable as well in the future, which means developers can add the ability to control user-based permissions in the admin.

[th3hamm0r]

For https://github.com/wagtail/wagtail/issues/11707 I realized, that the ModelViewSets is missing something like the PermissionHelper from the ModelAdmin, which has some similarities with the proposed PermissionTester (at least it acts on instances).

With that, all permission checks could be changed to either check the tester, if an instance is available (e.g. in edit-views or listings), or the policy if no instance is available (e.g. basic view access or create-view). This would allow the same customizations as with the ModelAdmin.

With that in mind I'm also more with option B.

Btw, much content in the RFC, but very easy to read! Amazing work! :ok_hand:

[lb-]

Thank you @laymonage - this area is not really my expertise but I'm excited to see something more flexible and central come in to Wagtail. I don't have any strong opinions on direction here but hab read through and cannot see anything to flag.

[gasman]

Thanks for putting this together @laymonage - excellent summary of the current state of play, and a really thorough analysis of how to improve it!

I'd venture that "should this object be a PermissionTester or a PermissionPolicy" might not be the best way of framing the question: whichever answer we pick, it's going to take a considerable amount of work to make it fit the requirements, to the point that the eventual object is inevitably going to be very different from what currently exists. PermissionPolicy (in its current form) isn't sufficient because PagePermissionPolicy doesn't handle business logic such as workflows, locking and subpage_types - if the aim is to be able to ask questions like "can user X delete page Y?", then an answer that doesn't account for those things isn't much use. On the other hand, if we go with PermissionTester then we still have the task of figuring out what a general-purpose ModelPermissionTester would look like.

If the question is "which of these classes should be the starting point for refactoring into the object that we eventually return from the registry", then PermissionTester is probably closer in functionality. However, if we're asking "which of these is closer to how the API should look", I might actually lean towards PermissionPolicy. The trouble with PagePermissionTester is that it's just an ad-hoc collection of methods managed exclusively within wagtail core - if an add-on package wants to introduce a new action like "can translate" or "can post to Twitter" with its own business logic, how do we inject that into the PermissionTester object? Having 'action' be a parameter to a generic "can perform action" method would seem to make that a bit more maintainable.

The PermissionPolicy classes still serve a useful purpose for efficiently querying database-backed permission records, and in the spirit of the "classes should do one thing" principle I think they can be usefully left that way as utilities to be called by PermissionTester, rather than folded in with the rest of the business logic PermissionTester will have to consider. It may well be that for simple snippet classes that don't use locking or workflows, the PermissionTester is a thin wrapper around PermissionPolicy, but that's fine - it leaves the option open to add more bespoke logic to PermissionTester as and when needed. (Incidentally, I can't help but think that the names PermissionTester and PermissionPolicy have ended up the wrong way round from their real purposes... if there's a way to do a spot of renaming as part of this refactor without it becoming too disruptive, I'd be very happy :-) )

Further thoughts to follow tomorrow...

[laymonage]

Thanks @gasman!

I took an initial stab at a general purpose ModelPermissionTester before this RFC in https://github.com/wagtail/wagtail/pull/10578, but then I did a fresh take on it in https://github.com/wagtail/wagtail/pull/11956.

The trouble with PagePermissionTester is that it's just an ad-hoc collection of methods managed exclusively within wagtail core - if an add-on package wants to introduce a new action like "can translate" or "can post to Twitter" with its own business logic, how do we inject that into the PermissionTester object?

With the ability for users to register custom tester classes for their own models, one way this can be done is:

• The add-on package provides a mixin class that provides can_translate, can_post_to_twitter, etc.
• The user (developer) add this mixin to the tester class for the model they want to use it with.
  • The tester class (inheriting from the mixin) is registered to the registry.

It's a bit more involved compared to the add-on package just registering its actions in some way and have their respective methods called directly from the package.

There might be some mechanism we can make in the registry to make it easier for users. For example, the package may register its model mixin to the registry, with its own tester class. Then, somehow make the registry's getter return that tester class when you ask a permission question on an instance of that mixin (for those particular actions only, and correctly routing to the appropriate class for the other actions). Just thinking out loud here...

Having 'action' be a parameter to a generic "can perform action" method would seem to make that a bit more maintainable.

I see how it can make it easier for plugging in custom actions. However, if all the permission checks are done in that method itself, I can imagine it turning into one big method that checks for all the possible "action" string value. Unless that method is just the entrypoint and it delegates the actual permission check for each action to a separate method. At which point, we're basically going back to the tester class...

Another positive note about having the action be a parameter is that it allows for much less refactoring for our PermissionCheckedMixin view class, as that currently uses action strings to perform the checks via the permission policy.

Or maybe we could do a getattr(tester, f"can_{action}", lambda: False)() instead? 🤔

It may well be that for simple snippet classes that don't use locking or workflows, the PermissionTester is a thin wrapper around PermissionPolicy, but that's fine - it leaves the option open to add more bespoke logic to PermissionTester as and when needed.

I agree! Coincidentally, with my new take on ModelPermissionTester above, it seems possible to delegate most permission checks to the policy. Especially once the quirks around workflows and locking are resolved. Might be worth looking at that PR 😄

(Incidentally, I can't help but think that the names PermissionTester and PermissionPolicy have ended up the wrong way round from their real purposes... if there's a way to do a spot of renaming as part of this refactor without it becoming too disruptive, I'd be very happy :-) )

I don't have a strong preference towards the naming. That said, we've mentioned the Tester and Policy a few times in the docs (mostly release notes I think), so switching the two might be very surprising. If we had a new name at least for one of them I think that would help clear up the confusion (compared to just switching the two). I'm not so good at coming up with names, though, so suggestions welcome 🙂

[gasman]

There might be some mechanism we can make in the registry to make it easier for users. For example, the package may register its model mixin to the registry, with its own tester class. Then, somehow make the registry's getter return that tester class when you ask a permission question on an instance of that mixin (for those particular actions only, and correctly routing to the appropriate class for the other actions). Just thinking out loud here...

This is the point where I start wondering: is there actually any benefit to having one object that handles all permission tests for all actions on a given model? Could we instead have a separate object for each action? That way, we don't need to come up with clever mechanisms for composing one object from mixins. A single object made sense when it held onto the permission records as part of its internal state, and doing a series of "can edit" / "can move" / "can delete" checks avoided database hits - but now that we're caching those on the user object, it doesn't have that benefit.

Perhaps it's useful to consider the places where we expect to be making use of the permissions registry. It seems to me that there are two main cases:

• in the views that actually perform the action, where we're doing the equivalent of

  def delete(request, id):
      instance = get_object_or_404(MyModel, id)
      permission_tester = permission_registry.get_tester(request.user, instance)
      if not permission_tester.can_delete():
          raise PermissionDenied
  

• when presenting a list of available actions to offer to the user. This will generally come in the form of a register_action_buttons hook, that returns a set of components that look like:

  class DeleteButton(Button):
      def is_shown(self, user, instance):
          permission_tester = permission_registry.get_tester(user, instance)
          return permission_tester.can_delete()
  

In both cases, we're retrieving a permission tester to perform one check before discarding it, so it doesn't seem like much of a leap to change that to:

permission_tester = permission_registry.get_tester(user, instance, "delete")
return permission_tester.can_act()

One last thought: we already have the concept of classes that encapsulate actions, in wagtail.actions. Would it make sense for can_act to be a method on those objects? Could this be an "action registry" rather than a permission registry, where you're retrieving an object that not only tells you whether you can do the thing, but provides the implementation for the thing too? Are there other methods / attributes that those action objects could usefully provide, like specifying an icon or label to be used on buttons for that action?

[laymonage]

This is the point where I start wondering: is there actually any benefit to having one object that handles all permission tests for all actions on a given model? Could we instead have a separate object for each action? That way, we don't need to come up with clever mechanisms for composing one object from mixins. A single object made sense when it held onto the permission records as part of its internal state, and doing a series of "can edit" / "can move" / "can delete" checks avoided database hits - but now that we're caching those on the user object, it doesn't have that benefit.

Good point. Yeah, there isn't really. I tried to approach this from an angle that would be least intrusive to the current implementation, which has been around for years and depended on by external packages and site implementers, despite not being documented. If we're willing to make more drastic changes, I won't be against it.

Perhaps it's useful to consider the places where we expect to be making use of the permissions registry. It seems to me that there are two main cases:

Another case is permission testing in templates, e.g.

{% page_permissions page as page_perms %}
{% if page_perms.can_delete %}

Although I suppose it is possible to change it into something like

{% permissions request.user page "delete" as can_delete %}
{% if can_delete %}

where the permissions template tag would do the .get_tester() and return the can_act() value.

One last thought: we already have the concept of classes that encapsulate actions, in wagtail.actions. Would it make sense for can_act to be a method on those objects? Could this be an "action registry" rather than a permission registry, where you're retrieving an object that not only tells you whether you can do the thing, but provides the implementation for the thing too?

I like the idea of an actions registry, but I'd be wary of shoehorning this into wagtail.actions, at least with the current state:

• Not all business logic has been encapsulated in action classes.
• Apart from the recently generalised features like publish/unpublish, the action classes are mostly page-centric.
• The action classes, while they share similar methods (check(), execute()), don't have a base interface class to enforce this.
  • Right now, the __init__ is tailored to what each action requires, all the way up to the point of execution.
  • Some of the parameters may not be necessary to perform the permission check, e.g. the log_action to use, or the action's implementation details like UnpublishAction.set_expired.
  • We can make such parameters optional (most already are) so we don't need to have them in hand when we only want to do permission checks. However, I don't like the idea of calling __init__ differently based on whether you're using it for doing the actual action vs just doing permission checks, as that means there would be action instances where calling .execute() would give unexpected results (or worse, a crash).
    • Perhaps, the can_act() method should be a class method that takes the user, the model instance, and the minimum necessary parameters for the permission checks? Therefore, you can't mistakenly create an "incomplete" action instance and call .execute(). If you want to do permission checks, you don't need to create an instance of the action.
  • For some actions, we might need to do the check in different context: to access the initial step vs the actual execution. Examples:
    • "can move", whether they can move the page at all (i.e. accessing the move view)
    • "can move to", whether they can move the page to a specific destination
    • In the context of lockable models, "can access the editor" vs "can make edits (saving)"
  • Some actions take a model class instead of a model instance, e.g. "create"
    • For non-page models, we need the class and the user, that's it.
    • For pages, we generally also need the parent instance. However, there can also be cases where you don't know the parent instance ahead of time, e.g. the "add" button in PageListingViewSet that leads to the "choose parent" view. I suppose this is similar to the "can move" vs "can move to" case above.

Not saying it's impossible, it just sounds like a lot of prerequisite work would need to be done to tidy up the action classes and move towards them before we can get to the permissions stuff. I can see how the action classes would be useful when background workers come into play, though, so I think all the refactoring would be worthwhile if we were to go down this route.

Are there other methods / attributes that those action objects could usefully provide, like specifying an icon or label to be used on buttons for that action?

Yeah, would be nice to tie all of those together with the action if possible I think. But again, just not quite sure how much refactoring that would entail 😄

[laymonage]

@gasman Something else to think about if we want to approach this with "actions":

We may want to support the "view" permission e.g. for the Inspect view (and maybe others like the usage and history views), see https://github.com/wagtail/wagtail/issues/11909.

If we use the actions approach, what would it correspond to? I suppose we can have something like a ViewAction that implements the permission check, without doing anything in .execute(), but it feels a little bit unnatural...

[gasman]

@laymonage That's a fair point. I think we're likely to end up with some convoluted definitions for permissions / actions in any case, as we start using permission rules in a more formal way - for example, if a page is locked, we probably still want to let editors access the edit view in read-only form, which would seem to call for a "can access the edit view" permission that's distinct from "can edit".

[gasman]

I like the idea of an actions registry, but I'd be wary of shoehorning this into wagtail.actions, at least with the current state

Yeah, happy to dial back on that idea. If we do refactor things in the far future so that everything related to performing an action is contained in a single module, I think it's probably more likely that the permission tester part of it will still remain as a distinct object - in much the same way that viewsets have things like views, menu items and widgets spun off as separate classes from the central Viewset class. So, trying to combine all of that into one do-everything object would be a mis-step, and the work we do now to define permission tester objects would still form part of that far-future vision.

[laymonage]

Yeah, happy to dial back on that idea. If we do refactor things in the far future so that everything related to performing an action is contained in a single module, I think it's probably more likely that the permission tester part of it will still remain as a distinct object - in much the same way that viewsets have things like views, menu items and widgets spun off as separate classes from the central Viewset class. So, trying to combine all of that into one do-everything object would be a mis-step, and the work we do now to define permission tester objects would still form part of that far-future vision.

I agree, and I think even if it turns out that moving the permission check to actions (or something else other than the permission testers) is the better approach, having the generic permission tester will help in the initial parts of the refactoring, since there will be less disparity between pages and other models.

For now, the main goal of this RFC is to unify the permissions interface between pages and non-pages, given the current state of play. It would be nice to have a solution that works better than what we have now as part of it – but I think that's a long way from here, especially if we want to be extra careful with breaking changes to undocumented APIs.

[laymonage]

Closing in favour of RFC #102.