Navigation shows edit button on pages for which user has no edit permission.

[khink]

Issue Summary

Explorer menu in some cases shows edit button for all pages, even though the group a user belongs to has only rights for a specific set of pages.

Steps to Reproduce

• Start up the Wagtail Bakery demo (https://github.com/wagtail/bakerydemo)
• Add a new Standard Page below the Home Page. Call it "Different Section"
• Add a new BreadIndexPage below "Different Section". Call it "Different Section Breads".
• Create a new Group. This group will have only the permission to work on Bread pages, so we call them "Breaditors".
• Give the Breaditors group admin acces, and all permissions on both "Different Section" and "Different Section Breads".
  
• Now add a user, add them to Breaditors and log in as this user.
• Go to the admin and expand the "Pages" explorer. You will see all items below the homepage are showing. That's okay for navigating to the "Different Section Breads".
• But the "Edit" crayon icon is showing on all items. That's a problem.
  
• Clicking on the edit button of an item where the user has no permissions yields an ugly 403 page. Safe enough, but not nice.

Technical details

• Python 3.6.1
• Django 2.0.6
• Wagtail 2.1
• Browser version: irrelevant

[khink]

It seems that the first item you see in the explorer is the common ancestor of both pages you have permission on. That would explain why it works fine if you give permissions on one page (and its children) only in the tree.

[thibaudcolas]

The code to display the "Edit" button is here:

wagtail/client/src/components/Explorer/ExplorerItem.js

Lines 46 to 51 in b4a2e13

	<Button
	href={`${ADMIN_URLS.PAGES}${id}/edit/`}
	className="c-explorer__item__action c-explorer__item__action--small"
	>
	{editIcon}
	</Button>

There is no permission check, it is always displayed. I'm not sure whether the current API can send this information for pages (and then we just have to edit the client-side code to have a conditional), or whether we need to implement that server-side first.

[ChristophTr]

+1 I'm facing the same problem. Is there a fix available for this issue?

[thibaudcolas]

@ChristophTrappe not that I'm aware of. Fixing this requires updating some of Wagtail's client-side code that is compiled for releases, so I wouldn't expect anyone to attempt patching over Wagtail's internals for this.

If you or anyone else wants to attempt to fix this bug, here are the steps I think it would take:

1. Check if the API endpoint used by the explorer menu (/admin/api/v2beta/pages/?child_of=<page>&for_explorer=1) can query permission information. If yes, great. If not, implement this on the API.
2. Then update the API client (if needed) to get the permission info.
3. Finally, update the ExplorerItem component I linked to above so the "Edit" button is rendered only if the page is editable (according to the permissions).

[LennartC]

I wrote a small workaround that hides the pages in the list for which the user doesn't have edit rights, which is what I want in a multitenancy setup.
This example only works on the top level, but if you remove the if statement it works on all levels.

from django.shortcuts import get_object_or_404
from rest_framework.response import Response
from wagtail.admin.api.endpoints import PagesAdminAPIEndpoint as DefaultPagesAdminAPIEndpoint
from wagtail.api.v2.router import WagtailAPIRouter

from wagtail.core.models import Page


class FilteredPagesAdminAPIEndpoint(DefaultPagesAdminAPIEndpoint):

    def listing_view(self, request):
        if request.query_params.get('child_of') == '1':
            queryset = self.get_queryset()
            self.check_query_parameters(queryset)
            queryset = self.filter_queryset(queryset)
            serializer = self.get_serializer(queryset, many=True)

            data = {
                "items": [],
                "meta": {
                    "total_count": 0
                }
            }

            for item in serializer.data:
                page = get_object_or_404(Page, id=item['id'])
                if page.permissions_for_user(request.user).can_edit():
                    data["items"].append(item)
                    data["meta"]["total_count"] += 1

            data['__types'] = self.get_type_info()

            return Response(data)

        else:
            return super().listing_view(request)


admin_api_fix = WagtailAPIRouter('wagtailadmin_api_fix')
admin_api_fix.register_endpoint('pages', FilteredPagesAdminAPIEndpoint)

And then add url(r'^admin/api/v2beta/', admin_api_fix.urls) to the urlpatterns in urls.py, above the r'^admin/' pattern.

[gasman]

As @khink mentions #4660 (comment) , the explorer tree (both the pop-out menu and the explorer listing views) starts from the closest common ancestor of the pages the user has add/edit/lock/publish permission on, to allow them to navigate between those areas:

wagtail/wagtail/admin/navigation.py

Line 16 in e77338f

def get_explorable_root_page(user):

Currently, this means they can see 'siblings' of those pages they have permission on. I believe it would be more correct to hide these completely, as in @LennartC's example - having access to two sub-sites on a multi-tenant setup shouldn't let you see everyone else's sites. In this respect, the explorer listing views are wrong too: they do hide the edit/publish/delete buttons appropriately, but it would be more correct to hide these pages completely from the listing.

I propose adding a new method explorable_pages to UserPagePermissionProxy to go alongside the existing editable_pages / publishable_pages methods. This would return a page queryset filtered to pages which are within the explorable root (the closest common ancestor), and are either ancestors or descendants (including self) of a page they have direct add/edit/publish/lock permission on.

Applying this filter on explorer listing views would be a simpler first step, as it doesn't involve changing the API; with that done, we can look at adding a param to the admin API. I suggest that this should be separate from the existing for_explorer flag (which tells the API to apply the construct_explorer_page_queryset hook).

cc @mike-hearn

[gasman]

Now partially fixed by #5069. Pages which previously appeared in the menu due to being siblings of the "navigable tree" (i.e. the set of pages the user has permission on, plus the minimal set of common ancestors necessary to navigate between them) are now omitted.

Remaining sub-issues:

1. Ancestor pages within the navigable tree should be shown but not have the 'edit' button
2. The explorer listing view should now be updated to match the behaviour of the menu, i.e. hide sibling pages of the navigable tree.

[HugoDelval]

I'm trying to implement this locally. I've come up with this code, which seems to correctly filter the pages in the explorer (the second sub-issue @gasman mentionned here) :

def accessible_pages(user):
    """
    Return a queryset of the pages that this user has direct permission to
     add/edit/publish/lock
    """
    # Deal with the trivial cases first...
    if not user.is_active:
        return Page.objects.none()
    if user.is_superuser:
        return Page.objects.all()

    pages = Page.objects.none()

    permissions = GroupPagePermission.objects.filter(group__user=user).select_related('page')

    for perm in permissions.filter(permission_type='add'):
        # user has add permission on any subpage/ancestor of perm.page
        # (including perm.page itself) that is owned by them
        pages |= Page.objects.descendant_of(perm.page).filter(owner=user)
        pages |= Page.objects.ancestor_of(perm.page, inclusive=True)

    for perm in permissions.filter(Q(permission_type='edit') | Q(permission_type='publish') | Q(permission_type='lock')):
        # user has edit/publish/lock permission on any subpage/ancestor of perm.page
        # (including perm.page itself)
        pages |= Page.objects.descendant_of(perm.page, inclusive=True)
        pages |= Page.objects.ancestor_of(perm.page)

    return pages


@hooks.register('construct_explorer_page_queryset')
def show_pages_user_has_access_to(parent_page, pages, request):
    # show pages user has access to
    pages &= accessible_pages(request.user)
    return pages

Would this be an acceptable solution for wagttail? (sorry I'm not used to the internals of wagtail yet)

Also this solution does not treat the first sub-issue (Ancestor pages within the navigable tree should be shown but not have the 'edit' button). And I think the only way to deal with this issue is to modify the code of PagesAdminAPIViewSet or to do something as @LennartC did here