From bfa9a953fc6e59166e7c0bf2e0eb6d9ecb405b77 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Tue, 27 Sep 2016 11:45:24 +0100 Subject: [PATCH] Rewrite page delete permission rules so that publish permission doesn't automatically grant deletion The old permission logic allowed anyone with publish permission to delete pages, with no further checks applied. This is incorrect, because the permission rules applied elsewhere establish that 1) deletion is equivalent to editing, and 2) publish permission DOES NOT imply edit permission. --- wagtail/wagtailcore/models.py | 31 +++++---- .../tests/test_page_permissions.py | 64 ++++++++++++++++++- 2 files changed, 82 insertions(+), 13 deletions(-) diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index 8071b619a..c804952b9 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -1691,21 +1691,30 @@ class PagePermissionTester(object): if self.page_is_root: # root node is not a page and can never be deleted, even by superusers return False - if self.user.is_superuser or ('publish' in self.permissions): - # Users with publish permission can unpublish any pages that need to be unpublished to achieve deletion + if self.user.is_superuser: + # superusers require no further checks return True - elif 'edit' in self.permissions: - # user can only delete if there are no live pages in this subtree - return (not self.page.live) and (not self.page.get_descendants().filter(live=True).exists()) + if 'edit' in self.permissions: + # if the user does not have publish permission, we also need to confirm that there + # are no published pages here + if 'publish' not in self.permissions: + pages_to_delete = self.page.get_descendants(inclusive=True) + if pages_to_delete.live().exists(): + return False + + return True elif 'add' in self.permissions: - # user can only delete if all pages in this subtree are unpublished and owned by this user - return ( - (not self.page.live) and - (self.page.owner_id == self.user.pk) and - (not self.page.get_descendants().exclude(live=False, owner=self.user).exists()) - ) + pages_to_delete = self.page.get_descendants(inclusive=True) + if 'publish' in self.permissions: + # we don't care about live state, but all pages must be owned by this user + # (i.e. eliminating pages owned by this user must give us the empty set) + return not pages_to_delete.exclude(owner=self.user).exists() + else: + # all pages must be owned by this user and non-live + # (i.e. eliminating non-live pages owned by this user must give us the empty set) + return not pages_to_delete.exclude(live=False, owner=self.user).exists() else: return False diff --git a/wagtail/wagtailcore/tests/test_page_permissions.py b/wagtail/wagtailcore/tests/test_page_permissions.py index 475604422..1f4a141a3 100644 --- a/wagtail/wagtailcore/tests/test_page_permissions.py +++ b/wagtail/wagtailcore/tests/test_page_permissions.py @@ -4,7 +4,7 @@ from django.contrib.auth import get_user_model from django.test import TestCase from wagtail.tests.testapp.models import BusinessSubIndex, EventPage -from wagtail.wagtailcore.models import Page, UserPagePermissionsProxy +from wagtail.wagtailcore.models import GroupPagePermission, Page, UserPagePermissionsProxy class TestPagePermission(TestCase): @@ -95,7 +95,8 @@ class TestPagePermission(TestCase): self.assertTrue(unpub_perms.can_edit()) self.assertFalse(homepage_perms.can_delete()) - self.assertTrue(christmas_page_perms.can_delete()) # cannot delete because it is published + # can delete a published page because we have publish permission + self.assertTrue(christmas_page_perms.can_delete()) self.assertTrue(unpub_perms.can_delete()) self.assertFalse(homepage_perms.can_publish()) @@ -129,6 +130,65 @@ class TestPagePermission(TestCase): # cannot move because the parent_page_types rule of BusinessSubIndex forbids EventPage as a parent self.assertFalse(board_meetings_perms.can_move_to(christmas_page)) + def test_publish_page_permissions_without_edit(self): + event_moderator = get_user_model().objects.get(username='eventmoderator') + + # Remove 'edit' permission from the event_moderator group + GroupPagePermission.objects.filter(group__name='Event moderators', permission_type='edit').delete() + + homepage = Page.objects.get(url_path='/home/') + christmas_page = EventPage.objects.get(url_path='/home/events/christmas/') + unpublished_event_page = EventPage.objects.get(url_path='/home/events/tentative-unpublished-event/') + # 'someone else's event' is owned by eventmoderator + moderator_event_page = EventPage.objects.get(url_path='/home/events/someone-elses-event/') + + homepage_perms = homepage.permissions_for_user(event_moderator) + christmas_page_perms = christmas_page.permissions_for_user(event_moderator) + unpub_perms = unpublished_event_page.permissions_for_user(event_moderator) + moderator_event_perms = moderator_event_page.permissions_for_user(event_moderator) + + # we still have add permission within events + self.assertFalse(homepage_perms.can_add_subpage()) + self.assertTrue(christmas_page_perms.can_add_subpage()) + + # add permission lets us edit our own event + self.assertFalse(christmas_page_perms.can_edit()) + self.assertTrue(moderator_event_perms.can_edit()) + + # with add + publish permissions, can delete a published page owned by us + self.assertTrue(moderator_event_perms.can_delete()) + # but NOT a page owned by someone else (which would require edit permission) + self.assertFalse(christmas_page_perms.can_delete()) + # ...even an unpublished one + self.assertFalse(unpub_perms.can_delete()) + + # we can still publish/unpublish events regardless of owner + self.assertFalse(homepage_perms.can_publish()) + self.assertTrue(christmas_page_perms.can_publish()) + self.assertTrue(unpub_perms.can_publish()) + + self.assertFalse(homepage_perms.can_unpublish()) + self.assertTrue(christmas_page_perms.can_unpublish()) + self.assertFalse(unpub_perms.can_unpublish()) # cannot unpublish a page that isn't published + + self.assertFalse(homepage_perms.can_publish_subpage()) + self.assertTrue(christmas_page_perms.can_publish_subpage()) + self.assertTrue(unpub_perms.can_publish_subpage()) + + # reorder permission is considered equivalent to publish permission + # (so we can do it on pages we can't edit) + self.assertFalse(homepage_perms.can_reorder_children()) + self.assertTrue(christmas_page_perms.can_reorder_children()) + self.assertTrue(unpub_perms.can_reorder_children()) + + # moving requires edit permission + self.assertFalse(homepage_perms.can_move()) + self.assertFalse(christmas_page_perms.can_move()) + self.assertTrue(moderator_event_perms.can_move()) + # and add permission on the destination + self.assertFalse(moderator_event_perms.can_move_to(homepage)) + self.assertTrue(moderator_event_perms.can_move_to(unpublished_event_page)) + def test_inactive_user_has_no_permissions(self): user = get_user_model().objects.get(username='inactiveuser') christmas_page = EventPage.objects.get(url_path='/home/events/christmas/')