Implementing custom permission behavior?

[cjmochrie]

I am not sure if this is a bug or a feature request or simply a frustrated user, but I am struggling with developing custom permission behavior - namely: preventing the deletion of published pages. If there is a way to do this without altering Wagtail source I am all ears, but here is a brief overview of my solution:

# Create custom PagePermissionTester and UserPagePermissionsProxy classes
class CustomPagePermissionTester(PagePermissionTester):
    def can_delete(self):
        return not self.page.live and super().can_delete()

class CustomPermProxy(UserPagePermissionsProxy):
    def for_page(self, page):
        return CustomPagePermissionTester(self, page)

# ... within my page model
    def permissions_for_user(self, user):
        user_perms = CustomPermProxy(user)
        return user_perms.for_page(self)

Within views/pages.py

def delete(request, page_id):
    page = get_object_or_404(Page, id=page_id).specific # access specific page type
    if not page.permissions_for_user(request.user).can_delete():
        raise PermissionDenied

And to remove the option to delete a page from the dropdown - simply add page_perms to the render context of def edit(request, page_id): and remove the following lines from the edit template:

{% page_permissions page as page_perms %}

Would something like the above be possible to implement for Wagtail as a whole or are there other reasons why overriding permissions_for_user should not be allowed?

[nickhudkins]

Hey @cjmochrie I just ran into this same issue, and noticed that the {% page_permissions %} template tag has this line in it: https://github.com/wagtail/wagtail/blob/de9ffaab4978ad5e50ead1c52b2c1036ee92fc0c/wagtail/wagtailadmin/templatetags/wagtailadmin_tags.py#L136

Unfortunately, those page permissions (although retrieved in the edit view: https://github.com/wagtail/wagtail/blob/de9ffaab4978ad5e50ead1c52b2c1036ee92fc0c/wagtail/wagtailadmin/views/pages.py#L301) go unused outside of the view. Which means that default Page permissions are used in context.

@gasman is this intended behavior or did we stumble on a bug?

The result of this is that if permissions_for_user is defined on a page sub class, the custom permissions returned are not respected.

[nickhudkins]

@cjmochrie update: https://github.com/wagtail/wagtail/commit/72b411b60fc9313d43a2a604bdf3273216218029 resolves this. I am going to make sure tests are passing and hopefully add a test case here, but if you need to get moving quickly, this will do the trick.

[cjmochrie]

@nickhudkins Oh awesome! I will close this issue as clearly I haven't been keeping close enough tabs on the progress made.

[nickhudkins]

I'm not sure how that commit didn't end up on my fork or in any place other than that link. I think this still needs to be open. I'll put in a PR once I have followed the contributing guidelines :)

[gasman]

FWIW, I'm currently undecided about the value of supporting custom permission models. The subject of permissions is a much deeper rabbit-hole than it appears at first: Wagtail's default (and de-facto only) permission model is built on lots of design considerations that have accumulated over time, including:

• the need to keep user actions consistent, usable and free of loopholes - for example, if we set a permission rule to prevent users from deleting published pages, but they can still unpublish and delete as two separate steps, then all we've done is thrown a usability hurdle in their way. (Which is not automatically a bad thing when it comes to destructive actions, but that's a whole other discussion...)
• the need to ensure performance - it's not enough to write a function to check "can user X perform action Y on page Z" and return true or false, because we need to be able to query the permission model efficiently in different ways, such as "which pages can user X perform action Y on", or "which users can perform action Y on any page".

In principle we could still support custom permission models anyhow, subject to carefully-documented rules about how they should behave, i.e.: "you can override the permission model, but it needs to comply with considerations A, B and C or else things might break". The problem is that those considerations will inevitably evolve further over time, and when they do, we need to roll out changes carefully to ensure that we don't break existing custom permission models. My feeling is that developers will be better served by Wagtail providing an all-encompassing permission model with enough flexibility built in to do what people want, rather than giving them the option of swapping it out (and then discovering how much of a nightmare it is to get the details right).

[nickhudkins]

Thank you for taking the time to consider this. I completely agree that this is a far larger question than should my commit get merged.

For some insight into why I went down this path, the ability to delete is based on the combination of edit and publish permissions. I am not so terribly concerned about can or cannot delete published pages, as I am about destructive actions having explicit permissions defining them. Unpublishing is destructive in the sense that, that page is temporarily unable to be acccessed, it is not destructive in the sense that the data is lost.

I think if we can come to a consensus on if deleting should be treated as a unique permission, that would be a huge step forward. I have plenty of users that should be able to publish, unpublish, and edit, but should NOT be able to delete the page without asking someone.

The reason I chose the implementation I did was because the documentation lead me to believe that I could override permissions_for_user on any given page. When I dug in further, I saw that the action buttons were driven by page_perms which, from its name, lead me to believe those may be the permissions returned for the given page, and when I found that that was not the case, it did appear that it was an oversight.

So perhaps what I am looking for is not "Custom Permissions" but "Can delete be a unique permission? (Even if it is site-wide)"

[gasman]

The reason we currently treat delete permission as equivalent to edit is that - in my experience - whenever a system denies ordinary users the ability to delete records, it invariably degenerates into records marked "XXX DO NOT USE" and items created by mistake (or items that a user started writing and then had second thoughts about). Essentially, users find workarounds that have the same effect as deletion but a worse user experience.

If we were to change this, we'd also have to contend with the fact that 'add' permission automatically grants edit/delete rights on your own articles (since it's pretty useless to give users permission that disappears as soon as they've saved the first draft) - logically we'd need two new permissions, corresponding to "can delete their own articles" and "can delete anyone's articles".

The point you raise about delete being a permanently destructive action is a fair point, especially since deletion also currently removes the page's revision history. Perhaps that suggests we should soft-delete items by default (see #2213 for earlier discussion about soft-deletion / undo functionality), and leave hard-deletion as a more privileged (most likely admin-only) action?

[luizboaretto]

A system wide recycle bin and undelete would be very interesting.

[kutenai]

@gasman - I wanted to comment on your statement

FWIW, I'm currently undecided about the value of supporting custom permission models

I am using wagtail to build a platform. For me, that terms means that I will have an audience and customers. Customers will be vendors of services (gardening related). Each vendor will have a page fir their company.

A vendor can sign-in, and then be associated with a vendor account (this is a manual admin account, and uses Django Group, GroupPagePermission and GroupCollectionPermission models. Once associated, they have access to a wagtail admin where they can edit their main page. As part of this, I also need to:

• Give them the ability to upload images to their own collection (collections are created for each vendor, I have about 300)
• Documents, while not really used, are handled the same as image collections.
• The vendor cannot delete the vendor page, but they can edit it.

Vendors will have free, paid, and premium accounts, with increasing permissions.

• Free can edit the main page
• Paid can add a gallery page and a single "project page"
• Premium can add multiple project pages, and create articles that get highlighted on the site.

I'm sure my use of wagtail goes beyond the intended usage, but before you point that out, please consider that wagtail is still THE BEST solution for this problem -- in my opinion.

I would ask you to re-consider expanding upon the "extensibility" features that are a part of Django. Having the ability to override a key class with a setting is certainly consistent with Django, even if it means someone using that feature may have issues in the future. Middleware changed significantly from 1.x to 2.x, for example. So, just because something is part of the "public api", it does not mean it can't be broken with breaking changes, Uses have to be aware of that, and the core team only needs to point out that "hey, as of version x.y.z, this is broken!".

For now, I'm just monkey patching the source, which is the worst solution, but also the best - since it's possible - and I have a product to launch!!

Some additional comments

• the need to keep user actions consistent In some applications, consistent varies with user options (paid versus free..)
• the need to ensure performance Yes, critical, hence a need to point out where performance issues could occur, i.e. "this method is called repeatedly, make it fast, or cache it, etc"

And, the big one:

The problem is that those considerations will inevitably evolve further over time, and when they do, we need to roll out changes carefully to ensure that we don't break existing custom permission models

I think it is perfectly acceptable to roll out changes that BREAK internal swapping of models, as long as that fact is made clear, and then anyone that has used such internal overrides will be aware of the need to modify their code remove it and use a new approach, etc. This should not limit or hold back the core from moving forward - IMO.

This message is already so long, that nobody will read it!! haha

[drcongo]

@kutenai I read the whole thing. Every client we have over a certain size has asked for the ability to prevent some of their staff from deleting pages, especially as deleting a page at the top of the tree can delete a hell of a lot of pages.

[gasman]

especially as deleting a page at the top of the tree can delete a hell of a lot of pages.

That's only true if they have the 'bulk delete' permission, which in the default setup is only enabled for superusers.

[gasman]

Putting this here for the next time this discussion comes up :-) https://github.com/wagtail/wagtail/pull/5634#discussion_r339006969 This is the sort of low-level detail that Wagtail has to account for in its permission model to keep the UI running smoothly. A site implementor intending to customise the permission model almost certainly doesn't want to be bogged down in this level of detail ("fine, ordinary editors should not be able to edit the index page, but what sort of not-editable UI state should it be in?"), but that's what they'll be stuck with if they customise UserPagePermissionsProxy / PagePermissionsTester directly.

The problem is, I'm not sure what the alternative is - if we give site implementors a higher-level interface to specify permission logic, then we as Wagtail developers have the impossible task of translating that high-level logic into UI micro-decisions.

[zeroepix]

How far have you investigated the idea of wagtail multi-sites? I can see the base is there but this topic inherently relates to it.

I'm planning on building a niche intranet targeted to a certain industry, but there are probably many use-cases for multi-sites using Django and Wagtail. Through all my researching of this, a common problem developers have is each client or user needs an encapsulated admin area. The dev wants to focus on only one version of their product, and not have to deploy it to multiple installs. The need is for a way to create site administrators, which have limited powers only the superusers can define, using the existing permissions system.

One might say to use the group system for this, but it doesn't really solve the problem. Org A and Org B might each have administrators, web-masters, regular staff of their own, and each organization should know nothing of the existence of the other. Webmasters from both Org A and B will both be in the "Webmaster" group, but of course they shouldn't have access to each other's objects, pages, and especially documents. You could make a unique group for them but you still have the problem of people with "add, edit, delete" user access being able to mess with the wrong users. That's a minefield, and you don't want to have to manage people's staff accounts for them either.

The simple solution to this, at least in my so-far-limited understanding of the source code, is to lock 100% of all objects to a site. Users, Pages, Snippets, Objects, everything, excluding the superuser. For my intranet I've created a meta-group model called Organization, which along with client-specific information, has a foreign key to a site. To make this work, whenever someone tries to view, add, edit or delete any content at all, I have find the right places to intercept that code and filter it against this unique organization. This is a lot of work and not always possible without making changes to the Wagtail code.

I'm interested in possibly making a pull-request for this sort of system when I have some time. In the meantime, do you think something like this is feasible?

For the original post in this issue, site-locking would resolve the entire thing, and it would not interfere with the existing UI at all. It merely filters all objects by this meta-group to which the non-superuser also belongs before it ever reaches any view.

[th3hamm0r]

Fyi: I think with the upcoming 5.1 release and the removal/deprecation of the UserPagePermissionsProxy (see #10562), it looks a bit easier to adapt the permission checks for a project (most code seems to use page.permissions_for_user() now). I haven't tested it yet, but the UserPagePermissionsProxy always required some monkey-patching in our projects :grimacing:

But I'm not sure, if the changes of #10562 have changed anything on the status, whether this should be used or not (documented feature? #5671) :thinking:

[SebCorbin]

Adding my 2 cents about this, my use case is quite common to those already stated: take the CMS editing features of Wagtail, but for some business reasons, prevent some actions from being displayed/possible.

Now I understand Wagtail development is a fine-tuned balance between giving enough customization through the UI (for site builders and moderators) while still allowing developers to deeply customize it.

I understand the permissions policy is central to that, and I'm all for not being able to swap it completely, but extending it should be possible because the Group-based and tree-based solution may not be adapted to all use cases.

As for the moment, I resorted to do these kind of alterations to just be able to prevent not editing a Homepage model (that has a bit of business):

# in wagtail_hooks.py
@hooks.register("construct_page_listing_buttons")
def remove_actions_for_homepage(buttons, page, page_perms, context=None):
    if page.cached_content_type.model_class() == Homepage:
        for index, item in enumerate(buttons):
            buttons.pop(index)

@hooks.register("before_edit_page")
@hooks.register("before_copy_page")
@hooks.register("before_delete_page")
@hooks.register("before_move_page")
@hooks.register("before_unpublish_page")
def denied_for_homepage(request, page, destination=None):
    if page.cached_content_type.model_class() == Homepage:
        return permission_denied(request)

Now, overriding permissions_for_user() seemed like a good architecture to tie that to the model in question, but it is not usable in the wild since the overridden method is not called (see #10702). But if it were to be possible, that would be much cleaner:

# in models.py
class HomepagePermissionTester(PagePermissionTester):
    def can_delete(self, ignore_bulk=False):
        return False

    def can_edit(self):
        return False

    def can_move(self):
        return False

    def can_copy(self):
        return False

    def can_view_revisions(self):
        return False

    def can_unpublish(self):
        return False

class Homepage(Page):
    """A singleton not meant to be edited by users"""
    parent_page_types = []

    class Meta:
        verbose_name = _("Homepage")
        verbose_name_plural = _("Homepage")

    def serve(self, request, *args, **kwargs):
        return some_business_here()

    def permissions_for_user(self, user):
        return HomepagePermissionTester(user, self)

[laymonage]

I made an RFC that will aim to add the ability to override built-in permission behaviours in Wagtail: https://github.com/wagtail/rfcs/pull/92

Keen to hear your thoughts!