From 128b319b9990c01ddcda3f827cf9961a050971ac Mon Sep 17 00:00:00 2001
From: Thijs Kramer <thijskramer@gmail.com>
Date: Fri, 18 Feb 2022 08:50:39 +0100
Subject: [PATCH] Fix permission error when sorting pages having page type
restrictions
---
CHANGELOG.txt | 1 +
docs/releases/2.16.2.md | 1 +
wagtail/actions/move_page.py | 25 ++-
wagtail/admin/tests/pages/test_move_page.py | 6 -
.../admin/tests/pages/test_reorder_page.py | 142 ++++++++++++++++++
5 files changed, 156 insertions(+), 19 deletions(-)
create mode 100644 wagtail/admin/tests/pages/test_reorder_page.py
diff --git a/CHANGELOG.txt b/CHANGELOG.txt
index aef79ee3d0..84f7e3d508 100644
--- a/CHANGELOG.txt
+++ b/CHANGELOG.txt
@@ -44,6 +44,7 @@ Changelog
~~~~~~~~~~~~~~~~~~~
* Fix: Update django-treebeard dependency to 4.5.1 or above (Serafeim Papastefanos)
+ * Fix: Fix permission error when sorting pages having page type restrictions (Thijs Kramer)
2.16.1 (11.02.2022)
diff --git a/docs/releases/2.16.2.md b/docs/releases/2.16.2.md
index cad4026898..52fdb83151 100644
--- a/docs/releases/2.16.2.md
+++ b/docs/releases/2.16.2.md
@@ -12,3 +12,4 @@ depth: 1
### Bug fixes
* Update django-treebeard dependency to 4.5.1 or above (Serafeim Papastefanos)
+ * Fix permission error when sorting pages having page type restrictions (Thijs Kramer)
diff --git a/wagtail/actions/move_page.py b/wagtail/actions/move_page.py
index 57feba5ef8..6ffed3bc53 100644
--- a/wagtail/actions/move_page.py
+++ b/wagtail/actions/move_page.py
@@ -25,25 +25,19 @@ class MovePageAction:
self.pos = pos
self.user = user
- def check(self, skip_permission_checks=False):
+ def check(self, parent_after, skip_permission_checks=False):
if self.user and not skip_permission_checks:
- if not self.page.permissions_for_user(self.user).can_move_to(self.target):
+ if not self.page.permissions_for_user(self.user).can_move_to(parent_after):
raise MovePagePermissionError(
"You do not have permission to move the page to the target specified."
)
- def _move_page(self, page, target, pos=None):
+ def _move_page(self, page, target, parent_after):
from wagtail.models import Page
- # Determine old and new parents
- parent_before = page.get_parent()
- if pos in ("first-child", "last-child", "sorted-child"):
- parent_after = target
- else:
- parent_after = target.get_parent()
-
# Determine old and new url_paths
# Fetching new object to avoid affecting `page`
+ parent_before = page.get_parent()
old_page = Page.objects.get(id=page.id)
old_url_path = old_page.url_path
new_url_path = old_page.set_url_path(parent=parent_after)
@@ -62,7 +56,7 @@ class MovePageAction:
# Only commit when all descendants are properly updated
with transaction.atomic():
# Allow treebeard to update `path` values
- MP_MoveHandler(page, target, pos).process()
+ MP_MoveHandler(page, target, self.pos).process()
# Treebeard's move method doesn't actually update the in-memory instance,
# so we need to work with a freshly loaded one now
@@ -103,6 +97,11 @@ class MovePageAction:
logger.info('Page moved: "%s" id=%d path=%s', page.title, page.id, new_url_path)
def execute(self, skip_permission_checks=False):
- self.check(skip_permission_checks=skip_permission_checks)
+ if self.pos in ("first-child", "last-child", "sorted-child"):
+ parent_after = self.target
+ else:
+ parent_after = self.target.get_parent()
- return self._move_page(self.page, self.target, pos=self.pos)
+ self.check(parent_after, skip_permission_checks=skip_permission_checks)
+
+ return self._move_page(self.page, self.target, parent_after)
diff --git a/wagtail/admin/tests/pages/test_move_page.py b/wagtail/admin/tests/pages/test_move_page.py
index f51e20ad5a..9022a979b3 100644
--- a/wagtail/admin/tests/pages/test_move_page.py
+++ b/wagtail/admin/tests/pages/test_move_page.py
@@ -196,12 +196,6 @@ class TestPageMove(TestCase, WagtailTestUtils):
)
)
- def test_page_set_page_position(self):
- response = self.client.get(
- reverse("wagtailadmin_pages:set_page_position", args=(self.test_page_a.id,))
- )
- self.assertEqual(response.status_code, 200)
-
def test_before_move_page_hook(self):
def hook_func(request, page, destination):
self.assertIsInstance(request, HttpRequest)
diff --git a/wagtail/admin/tests/pages/test_reorder_page.py b/wagtail/admin/tests/pages/test_reorder_page.py
new file mode 100644
index 0000000000..a4dce82350
--- /dev/null
+++ b/wagtail/admin/tests/pages/test_reorder_page.py
@@ -0,0 +1,142 @@
+from django.test import TestCase
+from django.urls import reverse
+
+from wagtail.core.models import Page
+from wagtail.test.testapp.models import BusinessChild, BusinessIndex, SimplePage
+from wagtail.tests.utils import WagtailTestUtils
+
+
+class TestPageReorder(TestCase, WagtailTestUtils):
+ fixtures = ["test.json"]
+
+ def __init__(self, methodName: str = ...) -> None:
+ super().__init__(methodName)
+
+ def setUp(self):
+ # Find root page
+ self.root_page = Page.objects.get(id=2)
+
+ # root
+ # |- simple_index (SimplePage)
+ # | |- simple_child_1 (SimplePage)
+ # | |- simple_child_2 (SimplePage)
+ # | |- simple_child_3 (SimplePage)
+
+ self.index_page = SimplePage(title="Simple", slug="simple", content="hello")
+ self.root_page.add_child(instance=self.index_page)
+
+ self.child_1 = SimplePage(
+ title="Child 1 of SimplePage", slug="child-1", content="hello"
+ )
+ self.index_page.add_child(instance=self.child_1)
+ self.child_2 = SimplePage(
+ title="Child 2 of SimplePage", slug="child-2", content="hello"
+ )
+ self.index_page.add_child(instance=self.child_2)
+ self.child_3 = SimplePage(
+ title="Child 3 of SimplePage", slug="child-3", content="hello"
+ )
+ self.index_page.add_child(instance=self.child_3)
+
+ # Login
+ self.user = self.login()
+
+ def test_page_set_page_position_get_request_with_simple_page(self):
+ """
+ Test that GET requests to set_page_position view don't alter the page order.
+ """
+ response = self.client.get(
+ reverse("wagtailadmin_pages:set_page_position", args=(self.child_1.id,))
+ )
+ self.assertEqual(response.status_code, 200)
+
+ # Ensure page order does not change:
+ child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+ self.assertListEqual(list(child_slugs), ["child-1", "child-2", "child-3"])
+
+ def test_page_set_page_position_without_position_argument_moves_it_to_the_end(self):
+ response = self.client.post(
+ reverse("wagtailadmin_pages:set_page_position", args=(self.child_1.id,))
+ )
+ self.assertEqual(response.status_code, 200)
+
+ # check if child_1 is the last child page:
+ child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+ self.assertListEqual(list(child_slugs), ["child-2", "child-3", "child-1"])
+
+ def test_page_move_page_position_up(self):
+ """Moves child 3 to the first position."""
+ response = self.client.post(
+ reverse("wagtailadmin_pages:set_page_position", args=(self.child_3.id,))
+ + "?position=0"
+ )
+ self.assertEqual(response.status_code, 200)
+ # check if child_3 is the first child page:
+ child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+ self.assertListEqual(list(child_slugs), ["child-3", "child-1", "child-2"])
+
+ def test_page_move_page_position_down(self):
+ """
+ Moves child 3 to the first position."""
+ response = self.client.post(
+ reverse("wagtailadmin_pages:set_page_position", args=(self.child_1.id,))
+ + "?position=1"
+ )
+ self.assertEqual(response.status_code, 200)
+ # check if child_1 is the second child page:
+ child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+ self.assertListEqual(list(child_slugs), ["child-2", "child-1", "child-3"])
+
+ def test_page_move_page_position_to_the_same_position(self):
+ """
+ Moves child 3 to the first position."""
+ response = self.client.post(
+ reverse("wagtailadmin_pages:set_page_position", args=(self.child_1.id,))
+ + "?position=0"
+ )
+ self.assertEqual(response.status_code, 200)
+ # Ensure page order does not change:
+ child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+ self.assertListEqual(list(child_slugs), ["child-1", "child-2", "child-3"])
+
+ def test_page_set_page_position_with_invalid_target_position(self):
+ response = self.client.post(
+ reverse("wagtailadmin_pages:set_page_position", args=(self.child_3.id,))
+ + "?position=99"
+ )
+ self.assertEqual(response.status_code, 200)
+
+ # Ensure page order does not change:
+ child_slugs = self.index_page.get_children().values_list("slug", flat=True)
+ self.assertListEqual(list(child_slugs), ["child-1", "child-2", "child-3"])
+
+
+class TestPageReorderWithParentPageRestrictions(TestPageReorder):
+ """
+ This testCase is the same as the TestPageReorder class above, but with a different
+ page type: BusinessChild has a parent_page_types restriction.
+ This ensures that this restriction doesn't affect the ability to reorder pages.
+ """
+
+ def setUp(self):
+ # Find root page
+ self.root_page = Page.objects.get(id=2)
+
+ # root
+ # |- index_page (BusinessIndex)
+ # | |- child_1 (BusinessChild)
+ # | |- child_2 (BusinessChild)
+ # | |- child_3 (BusinessChild)
+
+ self.index_page = BusinessIndex(title="Simple", slug="simple")
+ self.root_page.add_child(instance=self.index_page)
+
+ self.child_1 = BusinessChild(title="Child 1 of BusinessIndex", slug="child-1")
+ self.index_page.add_child(instance=self.child_1)
+ self.child_2 = BusinessChild(title="Child 2 of BusinessIndex", slug="child-2")
+ self.index_page.add_child(instance=self.child_2)
+ self.child_3 = BusinessChild(title="Child 3 of BusinessIndex", slug="child-3")
+ self.index_page.add_child(instance=self.child_3)
+
+ # Login
+ self.user = self.login()