From b0cebe186cfb540b5e51ce35ca0eedb976d4bc13 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Sat, 22 Jun 2024 23:11:13 +0200 Subject: [PATCH 1/8] Feat: Calculating page permissions does not require evaluating the full page tree --- cms/admin/pageadmin.py | 5 ++- cms/models/managers.py | 9 +++-- cms/models/permissionmodels.py | 20 ++++++++++ cms/tests/test_permissions.py | 6 +-- cms/tests/test_permmod.py | 9 ++--- cms/utils/page_permissions.py | 69 ++++++++++++++++++---------------- cms/utils/permissions.py | 47 ++++++++++------------- 7 files changed, 90 insertions(+), 75 deletions(-) diff --git a/cms/admin/pageadmin.py b/cms/admin/pageadmin.py index 17f0e19bb97..156a9140c0f 100644 --- a/cms/admin/pageadmin.py +++ b/cms/admin/pageadmin.py @@ -645,13 +645,14 @@ def get_permissions(self, request, page_id): ) if not can_change_global_permissions: - allowed_pages = frozenset(page_permissions.get_change_id_list(user, site, check_global=False)) + allowed_pages = frozenset(page_permissions.get_change_paths_list(user, site, check_global=False)) for permission in _page_permissions.iterator(): if can_change_global_permissions: can_change = True else: - can_change = permission.page_id in allowed_pages + page_path = permission.page.node.path + "!" + can_change = any(page_path.startswith(path) for path in allowed_pages) row = PermissionRow( is_global=False, diff --git a/cms/models/managers.py b/cms/models/managers.py index dd32afd6d7d..b209dcb4917 100644 --- a/cms/models/managers.py +++ b/cms/models/managers.py @@ -295,7 +295,7 @@ def subordinate_to_user(self, user, site): Result of this is used in admin for page permissions inline. """ # get user level - from cms.utils.page_permissions import get_change_permissions_id_list + from cms.utils.page_permissions import get_change_permissions_paths_list from cms.utils.permissions import get_user_permission_level try: @@ -307,12 +307,15 @@ def subordinate_to_user(self, user, site): return self.all() # get all permissions - page_id_allow_list = get_change_permissions_id_list(user, site, check_global=False) + allow_list = Q() + for path in get_change_permissions_paths_list(user, site, check_global=False): + allow_list |= Q(page__node__path=path[:-1]) if path.endswith("!") \ + else Q(page__node__path__startswith=path) # get permission set, but without objects targeting user, or any group # in which he can be qs = self.filter( - page__id__in=page_id_allow_list, + allow_list, page__node__depth__gte=user_level, ) qs = qs.exclude(user=user).exclude(group__user=user) diff --git a/cms/models/permissionmodels.py b/cms/models/permissionmodels.py index 51884f13a51..a3067a1d851 100644 --- a/cms/models/permissionmodels.py +++ b/cms/models/permissionmodels.py @@ -243,6 +243,26 @@ def clean(self): "by its creator.") raise ValidationError(message) + def get_page_paths(self): + if self.grant_on == ACCESS_PAGE: + yield self.page.node.path + "!" + elif self.grant_on == ACCESS_CHILDREN: + children = self.page.get_child_pages().values_list('node__path', flat=True) + for child in children: + yield child + "!" + elif self.grant_on == ACCESS_DESCENDANTS: + children = self.page.get_child_pages().values_list('node__path', flat=True) + for child in children: + yield child + elif self.grant_on == ACCESS_PAGE_AND_DESCENDANTS: + yield self.page.node.path + elif self.grant_on == ACCESS_PAGE_AND_CHILDREN: + yield self.page.node.path + "!" + + children = self.page.get_child_pages().values_list('node__path', flat=True) + for child in children: + yield child + "!" + def get_page_ids(self): if self.grant_on & MASK_PAGE: yield self.page_id diff --git a/cms/tests/test_permissions.py b/cms/tests/test_permissions.py index fe32cf6deeb..749f3801254 100644 --- a/cms/tests/test_permissions.py +++ b/cms/tests/test_permissions.py @@ -10,7 +10,7 @@ from cms.models.permissionmodels import GlobalPagePermission from cms.test_utils.testcases import CMSTestCase from cms.utils.page_permissions import ( - get_change_id_list, + get_change_paths_list, user_can_publish_page, ) @@ -59,10 +59,10 @@ def test_permission_manager(self): cached_permissions = get_permission_cache(self.user_normal, "change_page") self.assertIsNone(cached_permissions) - live_permissions = get_change_id_list(self.user_normal, Site.objects.get_current()) + live_permissions = get_change_paths_list(self.user_normal, Site.objects.get_current()) cached_permissions_permissions = get_permission_cache(self.user_normal, "change_page") - self.assertEqual(live_permissions, [page_b.id]) + self.assertEqual(live_permissions, [page_b.node.path]) self.assertEqual(cached_permissions_permissions, live_permissions) def test_cached_permission_precedence(self): diff --git a/cms/tests/test_permmod.py b/cms/tests/test_permmod.py index f1ad9525a04..d45919cdf4f 100644 --- a/cms/tests/test_permmod.py +++ b/cms/tests/test_permmod.py @@ -488,13 +488,12 @@ def test_page_permissions(self): request = self.get_request(user) PagePermission.objects.create(can_view=True, user=user, page=self.page, grant_on=ACCESS_PAGE) - with self.assertNumQueries(6): + with self.assertNumQueries(5): """ The queries are: PagePermission query (is this page restricted) content type lookup (x2) GlobalpagePermission query for user - TreeNode lookup PagePermission query for this user """ self.assertViewAllowed(self.page, user) @@ -506,13 +505,12 @@ def test_page_group_permissions(self): user.groups.add(self.group) request = self.get_request(user) - with self.assertNumQueries(6): + with self.assertNumQueries(5): """ The queries are: PagePermission query (is this page restricted) content type lookup (x2) GlobalpagePermission query for user - TreeNode lookup PagePermission query for user """ self.assertViewAllowed(self.page, user) @@ -540,13 +538,12 @@ def test_basic_perm_denied(self): user = self.get_staff_user_with_no_permissions() request = self.get_request(user) - with self.assertNumQueries(6): + with self.assertNumQueries(5): """ The queries are: PagePermission query (is this page restricted) content type lookup x2 GlobalpagePermission query for user - TreeNode lookup PagePermission query for this user """ self.assertViewNotAllowed(self.page, user) diff --git a/cms/utils/page_permissions.py b/cms/utils/page_permissions.py index cf5796bab36..9783bff6276 100644 --- a/cms/utils/page_permissions.py +++ b/cms/utils/page_permissions.py @@ -41,7 +41,7 @@ def _check_delete_translation(user, page, language, site=None): return user_can_change_page(user, page, site=site) -def _get_page_ids_for_action(user, site, action, check_global=True, use_cache=True): +def _get_page_paths_for_action(user, site, action, check_global=True, use_cache=True): if user.is_superuser or not get_cms_setting('PERMISSION'): # got superuser, or permissions aren't enabled? # just return grant all mark @@ -62,9 +62,9 @@ def _get_page_ids_for_action(user, site, action, check_global=True, use_cache=Tr return cached page_actions = get_page_actions(user, site) - page_ids = list(page_actions[action]) - set_permission_cache(user, action, page_ids) - return page_ids + page_paths = list(page_actions[action]) + set_permission_cache(user, action, page_paths) + return page_paths def auth_permission_required(action): @@ -310,7 +310,7 @@ def user_can_change_all_pages(user, site): @auth_permission_required('change_page') def user_can_change_at_least_one_page(user, site, use_cache=True): - page_ids = get_change_id_list( + page_ids = get_change_paths_list( user=user, site=site, check_global=True, @@ -344,12 +344,12 @@ def user_can_view_all_pages(user, site): return has_global_permission(user, site, action='view_page') -def get_add_id_list(user, site, check_global=True, use_cache=True): +def get_add_paths_list(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has add page rights or the string "All" if the user has all rights. """ - page_ids = _get_page_ids_for_action( + page_ids = _get_page_paths_for_action( user=user, site=site, action='add_page', @@ -359,12 +359,12 @@ def get_add_id_list(user, site, check_global=True, use_cache=True): return page_ids -def get_change_id_list(user, site, check_global=True, use_cache=True): +def get_change_paths_list(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has edit rights or the string "All" if the user has all rights. """ - page_ids = _get_page_ids_for_action( + page_ids = _get_page_paths_for_action( user=user, site=site, action='change_page', @@ -374,12 +374,12 @@ def get_change_id_list(user, site, check_global=True, use_cache=True): return page_ids -def get_change_advanced_settings_id_list(user, site, check_global=True, use_cache=True): +def get_change_advanced_settings_paths_list(user, site, check_global=True, use_cache=True): """ Give a list of page where the user can change advanced settings or the string "All" if the user has all rights. """ - page_ids = _get_page_ids_for_action( + page_ids = _get_page_paths_for_action( user=user, site=site, action='change_page_advanced_settings', @@ -389,10 +389,10 @@ def get_change_advanced_settings_id_list(user, site, check_global=True, use_cach return page_ids -def get_change_permissions_id_list(user, site, check_global=True, use_cache=True): +def get_change_permissions_paths_list(user, site, check_global=True, use_cache=True): """Give a list of page where the user can change permissions. """ - page_ids = _get_page_ids_for_action( + page_ids = _get_page_paths_for_action( user=user, site=site, action='change_page_permissions', @@ -402,12 +402,12 @@ def get_change_permissions_id_list(user, site, check_global=True, use_cache=True return page_ids -def get_delete_id_list(user, site, check_global=True, use_cache=True): +def get_delete_paths_list(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has delete rights or the string "All" if the user has all rights. """ - page_ids = _get_page_ids_for_action( + page_ids = _get_page_paths_for_action( user=user, site=site, action='delete_page', @@ -417,10 +417,10 @@ def get_delete_id_list(user, site, check_global=True, use_cache=True): return page_ids -def get_move_page_id_list(user, site, check_global=True, use_cache=True): +def get_move_page_paths_list(user, site, check_global=True, use_cache=True): """Give a list of pages which user can move. """ - page_ids = _get_page_ids_for_action( + page_ids = _get_page_paths_for_action( user=user, site=site, action='move_page', @@ -430,12 +430,12 @@ def get_move_page_id_list(user, site, check_global=True, use_cache=True): return page_ids -def get_publish_id_list(user, site, check_global=True, use_cache=True): +def get_publish_paths_list(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has publish rights or the string "All" if the user has all rights. """ - page_ids = _get_page_ids_for_action( + page_ids = _get_page_paths_for_action( user=user, site=site, action='publish_page', @@ -445,10 +445,10 @@ def get_publish_id_list(user, site, check_global=True, use_cache=True): return page_ids -def get_view_id_list(user, site, check_global=True, use_cache=True): +def get_view_paths_list(user, site, check_global=True, use_cache=True): """Give a list of pages which user can view. """ - page_ids = _get_page_ids_for_action( + page_ids = _get_page_paths_for_action( user=user, site=site, action='view_page', @@ -462,19 +462,22 @@ def has_generic_permission(page, user, action, site=None, check_global=True): if site is None: site = get_current_site() - page_id = page.pk + page_path = page.node.path + "!" actions_map = { - 'add_page': get_add_id_list, - 'change_page': get_change_id_list, - 'change_page_advanced_settings': get_change_advanced_settings_id_list, - 'change_page_permissions': get_change_permissions_id_list, - 'delete_page': get_delete_id_list, - 'delete_page_translation': get_delete_id_list, - 'publish_page': get_publish_id_list, - 'move_page': get_move_page_id_list, - 'view_page': get_view_id_list, + 'add_page': get_add_paths_list, + 'change_page': get_change_paths_list, + 'change_page_advanced_settings': get_change_advanced_settings_paths_list, + 'change_page_permissions': get_change_permissions_paths_list, + 'delete_page': get_delete_paths_list, + 'delete_page_translation': get_delete_paths_list, + 'publish_page': get_publish_paths_list, + 'move_page': get_move_page_paths_list, + 'view_page': get_view_paths_list, } func = actions_map[action] - page_ids = func(user, site, check_global=check_global) - return page_ids == GRANT_ALL_PERMISSIONS or page_id in page_ids + + page_paths = func(user, site, check_global=check_global) + return page_paths == GRANT_ALL_PERMISSIONS or any( + page_path.startswith(path) for path in page_paths + ) diff --git a/cms/utils/permissions.py b/cms/utils/permissions.py index e7b175b3349..f7aa07c55b7 100644 --- a/cms/utils/permissions.py +++ b/cms/utils/permissions.py @@ -174,36 +174,19 @@ def get_global_actions_for_user(user, site): @cached_func def get_page_actions_for_user(user, site): actions = defaultdict(set) - pages = ( - Page - .objects - .on_site(site) - .select_related('node') - .order_by('node__path') - ) - nodes = [page.node for page in pages] - pages_by_id = {} - - for page in pages: - if page.node.is_root(): - page.node._set_hierarchy(nodes) - page.node.__dict__['item'] = page - pages_by_id[page.pk] = page page_permissions = ( PagePermission .objects .with_user(user) - .filter(page__in=pages_by_id) + .select_related('page__node') + .filter(page__node__site=site) ) for perm in page_permissions.iterator(): - PagePermission.page.field.set_cached_value(perm, pages_by_id[perm.page_id]) - - page_ids = frozenset(perm.get_page_ids()) - + page_paths = frozenset(perm.get_page_paths()) for action in perm.get_configured_actions(): - actions[action].update(page_ids) + actions[action].update(page_paths) return actions @@ -216,11 +199,13 @@ def has_global_permission(user, site, action, use_cache=True): def has_page_permission(user, page, action, use_cache=True): + # DEPRECATE? if use_cache: actions = get_page_actions_for_user(user, page.node.site) else: actions = get_page_actions_for_user.without_cache(user, page.node.site) - return page.pk in actions[action] + path = page.node.path + "!" + return any(path.startswith(allowed_paths) for allowed_paths in actions[action]) def get_subordinate_users(user, site): @@ -253,7 +238,7 @@ def get_subordinate_users(user, site): Will return [user, C, X, D, Y, Z]. W was created by user, but is also assigned to higher level. """ - from cms.utils.page_permissions import get_change_permissions_id_list + from cms.utils.page_permissions import get_change_permissions_paths_list try: user_level = get_user_permission_level(user, site) @@ -270,12 +255,15 @@ def get_subordinate_users(user, site): if user_level == ROOT_USER_LEVEL: return get_user_model().objects.all() - page_id_allow_list = get_change_permissions_id_list(user, site, check_global=False) + allow_list = Q() + for path in get_change_permissions_paths_list(user, site, check_global=False): + allow_list |= Q(pagepermission__page__node__path=path[:-1]) if path.endswith("!") \ + else Q(pagepermission__page__node__path__startswith=path) # normal query qs = get_user_model().objects.distinct().filter( Q(is_staff=True) & ( - Q(pagepermission__page__id__in=page_id_allow_list) & Q(pagepermission__page__node__depth__gte=user_level) + allow_list & Q(pagepermission__page__node__depth__gte=user_level) ) | ( Q(pageuser__created_by=user) & Q(pagepermission__page=None) ) @@ -289,7 +277,7 @@ def get_subordinate_groups(user, site): Similar to get_subordinate_users, but returns queryset of Groups instead of Users. """ - from cms.utils.page_permissions import get_change_permissions_id_list + from cms.utils.page_permissions import get_change_permissions_paths_list try: user_level = get_user_permission_level(user, site) @@ -312,11 +300,14 @@ def get_subordinate_groups(user, site): if user_level == ROOT_USER_LEVEL: return Group.objects.all() - page_id_allow_list = get_change_permissions_id_list(user, site, check_global=False) + allow_list = Q() + for path in get_change_permissions_paths_list(user, site, check_global=False): + allow_list |= Q(pagepermission__page__node__path=path[:-1]) if path.endswith("!") \ + else Q(pagepermission__page__node__path__startswith=path) return Group.objects.distinct().filter( ( - Q(pagepermission__page__id__in=page_id_allow_list) & Q(pagepermission__page__node__depth__gte=user_level) + allow_list & Q(pagepermission__page__node__depth__gte=user_level) ) | ( Q(pageusergroup__created_by=user) & Q(pagepermission__page__isnull=True) ) From 8819ddc0b2723db0194357529038b7613eed1f2a Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Sun, 23 Jun 2024 18:23:11 +0200 Subject: [PATCH 2/8] Fix N+1 issue, cache all calculated permissions --- cms/admin/pageadmin.py | 6 +- cms/cms_menus.py | 2 - cms/menu_bases.py | 2 +- cms/models/managers.py | 12 ++-- cms/models/permissionmodels.py | 57 ++++++++++------- cms/tests/test_permissions.py | 8 +-- cms/utils/page_permissions.py | 109 +++++++++++++++++---------------- cms/utils/permissions.py | 22 +++---- 8 files changed, 116 insertions(+), 102 deletions(-) diff --git a/cms/admin/pageadmin.py b/cms/admin/pageadmin.py index 156a9140c0f..5e3a54d9c04 100644 --- a/cms/admin/pageadmin.py +++ b/cms/admin/pageadmin.py @@ -645,14 +645,14 @@ def get_permissions(self, request, page_id): ) if not can_change_global_permissions: - allowed_pages = frozenset(page_permissions.get_change_paths_list(user, site, check_global=False)) + allowed_pages = page_permissions.get_change_perm_tuples(user, site, check_global=False) for permission in _page_permissions.iterator(): if can_change_global_permissions: can_change = True else: - page_path = permission.page.node.path + "!" - can_change = any(page_path.startswith(path) for path in allowed_pages) + page_path = permission.page.node.path + can_change = any(perm_tuple.contains(page_path) for perm_tuple in allowed_pages) row = PermissionRow( is_global=False, diff --git a/cms/cms_menus.py b/cms/cms_menus.py index 5d9b3b22b43..a2af08f8d2f 100644 --- a/cms/cms_menus.py +++ b/cms/cms_menus.py @@ -1,9 +1,7 @@ from typing import Optional from django.db.models.query import Prefetch, prefetch_related_objects -from django.urls import reverse from django.utils.functional import SimpleLazyObject -from django.utils.translation import override as force_language from cms import constants from cms.apphook_pool import apphook_pool diff --git a/cms/menu_bases.py b/cms/menu_bases.py index f16c4ce1044..7bb0b5edca4 100644 --- a/cms/menu_bases.py +++ b/cms/menu_bases.py @@ -7,7 +7,7 @@ class CMSAttachMenu(Menu): - """Base class that can be subclassed to allow your app to attach its oqn menus.""" + """Base class that can be subclassed to allow your app to attach its own menus.""" cms_enabled = True instance = None name = None diff --git a/cms/models/managers.py b/cms/models/managers.py index b209dcb4917..a2cfb02a91d 100644 --- a/cms/models/managers.py +++ b/cms/models/managers.py @@ -286,8 +286,8 @@ def subordinate_to_user(self, user, site): users C,X,D,Y,I,J but not A, because A user in higher in hierarchy. If permission object holds group, this permission object can be visible - to user only if all of the group members are lover in hierarchy. If any - of members is higher then given user, this entry must stay invisible. + to user only if all the group members are lover in hierarchy. If any + of members is higher than given user, this entry must stay invisible. If user is superuser, or haves global can_change_permission permissions, show him everything. @@ -295,7 +295,7 @@ def subordinate_to_user(self, user, site): Result of this is used in admin for page permissions inline. """ # get user level - from cms.utils.page_permissions import get_change_permissions_paths_list + from cms.utils.page_permissions import get_change_permissions_perm_tuples from cms.utils.permissions import get_user_permission_level try: @@ -307,10 +307,10 @@ def subordinate_to_user(self, user, site): return self.all() # get all permissions + from cms.models import PermissionTuple allow_list = Q() - for path in get_change_permissions_paths_list(user, site, check_global=False): - allow_list |= Q(page__node__path=path[:-1]) if path.endswith("!") \ - else Q(page__node__path__startswith=path) + for perm_tuple in get_change_permissions_perm_tuples(user, site, check_global=False): + allow_list |= PermissionTuple(perm_tuple).allow_list("page__node") # get permission set, but without objects targeting user, or any group # in which he can be diff --git a/cms/models/permissionmodels.py b/cms/models/permissionmodels.py index a3067a1d851..b69d51e9f95 100644 --- a/cms/models/permissionmodels.py +++ b/cms/models/permissionmodels.py @@ -4,10 +4,11 @@ from django.contrib.sites.models import Site from django.core.exceptions import ImproperlyConfigured, ValidationError from django.db import models +from django.db.models import Q from django.utils.encoding import force_str from django.utils.translation import gettext_lazy as _ -from cms.models import Page +from cms.models import Page, TreeNode from cms.models.managers import ( GlobalPagePermissionManager, PagePermissionManager, @@ -214,6 +215,36 @@ def __str__(self): return "%s :: GLOBAL" % self.audience +class PermissionTuple(tuple): + def contains(self, path: str, steplen: int = TreeNode.steplen) -> bool: + grant_on, perm_path = self + if grant_on == ACCESS_PAGE: + return path == perm_path + elif grant_on == ACCESS_CHILDREN: + return path.startswith(perm_path) and len(path) == len(perm_path) + steplen + elif grant_on == ACCESS_DESCENDANTS: + return path.startswith(perm_path) and len(path) > len(perm_path) + elif grant_on == ACCESS_PAGE_AND_DESCENDANTS: + return path.startswith(perm_path) + elif grant_on == ACCESS_PAGE_AND_CHILDREN: + return path.startswith(perm_path) and len(path) <= len(perm_path) + steplen + return False + + def allow_list(self, filter: str, steplen: int = TreeNode.steplen) -> Q: + grant_on, path = self + if grant_on == ACCESS_PAGE: + return Q(**{f"{filter}__path": path}) + elif grant_on == ACCESS_CHILDREN: + return Q(**{f"{filter}__path__startswith": path, f"{filter}__path__length": len(path) + steplen}) + elif grant_on == ACCESS_DESCENDANTS: + return Q(**{f"{filter}__path__startswith": path, f"{filter}__path__length__gt": len(path)}) + elif grant_on == ACCESS_PAGE_AND_DESCENDANTS: + return Q(**{f"{filter}__path__startswith": path}) + elif grant_on == ACCESS_PAGE_AND_CHILDREN: + return Q(**{f"{filter}__path__startswith": path, f"{filter}__path__length__lte": len(path) + steplen}) + return Q() + + class PagePermission(AbstractPagePermission): """Page permissions for a single page """ @@ -236,32 +267,16 @@ def clean(self): if self.can_add and self.grant_on == ACCESS_PAGE: # this is a misconfiguration - user can add/move page to current - # page but after he does this, he will not have permissions to + # page, but after he does this, he will not have permissions to # access this page anymore, so avoid this. message = _("Add page permission requires also access to children, " "or descendants, otherwise added page can't be changed " "by its creator.") raise ValidationError(message) - def get_page_paths(self): - if self.grant_on == ACCESS_PAGE: - yield self.page.node.path + "!" - elif self.grant_on == ACCESS_CHILDREN: - children = self.page.get_child_pages().values_list('node__path', flat=True) - for child in children: - yield child + "!" - elif self.grant_on == ACCESS_DESCENDANTS: - children = self.page.get_child_pages().values_list('node__path', flat=True) - for child in children: - yield child - elif self.grant_on == ACCESS_PAGE_AND_DESCENDANTS: - yield self.page.node.path - elif self.grant_on == ACCESS_PAGE_AND_CHILDREN: - yield self.page.node.path + "!" - - children = self.page.get_child_pages().values_list('node__path', flat=True) - for child in children: - yield child + "!" + def get_page_permission_tuple(self): + node = self.page.node + return PermissionTuple(self.grant_on, node.path) def get_page_ids(self): if self.grant_on & MASK_PAGE: diff --git a/cms/tests/test_permissions.py b/cms/tests/test_permissions.py index 749f3801254..7579e06af8c 100644 --- a/cms/tests/test_permissions.py +++ b/cms/tests/test_permissions.py @@ -7,10 +7,10 @@ get_permission_cache, set_permission_cache, ) -from cms.models.permissionmodels import GlobalPagePermission +from cms.models.permissionmodels import ACCESS_PAGE_AND_DESCENDANTS, GlobalPagePermission from cms.test_utils.testcases import CMSTestCase from cms.utils.page_permissions import ( - get_change_paths_list, + get_change_perm_tuples, user_can_publish_page, ) @@ -59,10 +59,10 @@ def test_permission_manager(self): cached_permissions = get_permission_cache(self.user_normal, "change_page") self.assertIsNone(cached_permissions) - live_permissions = get_change_paths_list(self.user_normal, Site.objects.get_current()) + live_permissions = get_change_perm_tuples(self.user_normal, Site.objects.get_current()) cached_permissions_permissions = get_permission_cache(self.user_normal, "change_page") - self.assertEqual(live_permissions, [page_b.node.path]) + self.assertEqual(live_permissions, [(ACCESS_PAGE_AND_DESCENDANTS, page_b.node.path)]) self.assertEqual(cached_permissions_permissions, live_permissions) def test_cached_permission_precedence(self): diff --git a/cms/utils/page_permissions.py b/cms/utils/page_permissions.py index 9783bff6276..5a0496615f3 100644 --- a/cms/utils/page_permissions.py +++ b/cms/utils/page_permissions.py @@ -2,7 +2,7 @@ from cms.cache.permissions import get_permission_cache, set_permission_cache from cms.constants import GRANT_ALL_PERMISSIONS -from cms.models import Page +from cms.models import Page, PermissionTuple from cms.utils import get_current_site from cms.utils.compat.dj import available_attrs from cms.utils.conf import get_cms_setting @@ -41,7 +41,7 @@ def _check_delete_translation(user, page, language, site=None): return user_can_change_page(user, page, site=site) -def _get_page_paths_for_action(user, site, action, check_global=True, use_cache=True): +def _get_page_permission_tuples_for_action(user, site, action, check_global=True, use_cache=True): if user.is_superuser or not get_cms_setting('PERMISSION'): # got superuser, or permissions aren't enabled? # just return grant all mark @@ -62,9 +62,10 @@ def _get_page_paths_for_action(user, site, action, check_global=True, use_cache= return cached page_actions = get_page_actions(user, site) - page_paths = list(page_actions[action]) - set_permission_cache(user, action, page_paths) - return page_paths + # Set cache for all actions calculated + for act, page_paths in page_actions.items(): + set_permission_cache(user, act, list(page_paths)) + return page_actions[action] def auth_permission_required(action): @@ -310,13 +311,13 @@ def user_can_change_all_pages(user, site): @auth_permission_required('change_page') def user_can_change_at_least_one_page(user, site, use_cache=True): - page_ids = get_change_paths_list( + perm_tuples = get_change_perm_tuples( user=user, site=site, check_global=True, use_cache=use_cache, ) - return page_ids == GRANT_ALL_PERMISSIONS or bool(page_ids) + return perm_tuples == GRANT_ALL_PERMISSIONS or bool(perm_tuples) @cached_func @@ -344,140 +345,140 @@ def user_can_view_all_pages(user, site): return has_global_permission(user, site, action='view_page') -def get_add_paths_list(user, site, check_global=True, use_cache=True): +def get_add_perm_tuples(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has add page rights or the string "All" if the user has all rights. """ - page_ids = _get_page_paths_for_action( + perm_tuples = _get_page_permission_tuples_for_action( user=user, site=site, action='add_page', check_global=check_global, use_cache=use_cache, ) - return page_ids + return perm_tuples -def get_change_paths_list(user, site, check_global=True, use_cache=True): +def get_change_perm_tuples(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has edit rights or the string "All" if the user has all rights. """ - page_ids = _get_page_paths_for_action( + perm_tuples = _get_page_permission_tuples_for_action( user=user, site=site, action='change_page', check_global=check_global, use_cache=use_cache, ) - return page_ids + return perm_tuples -def get_change_advanced_settings_paths_list(user, site, check_global=True, use_cache=True): +def get_change_advanced_settings_perm_tuples(user, site, check_global=True, use_cache=True): """ Give a list of page where the user can change advanced settings or the string "All" if the user has all rights. """ - page_ids = _get_page_paths_for_action( + perm_tuples = _get_page_permission_tuples_for_action( user=user, site=site, action='change_page_advanced_settings', check_global=check_global, use_cache=use_cache, ) - return page_ids + return perm_tuples -def get_change_permissions_paths_list(user, site, check_global=True, use_cache=True): +def get_change_permissions_perm_tuples(user, site, check_global=True, use_cache=True): """Give a list of page where the user can change permissions. """ - page_ids = _get_page_paths_for_action( + perm_tuples = _get_page_permission_tuples_for_action( user=user, site=site, action='change_page_permissions', check_global=check_global, use_cache=use_cache, ) - return page_ids + return perm_tuples -def get_delete_paths_list(user, site, check_global=True, use_cache=True): +def get_delete_perm_tuples(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has delete rights or the string "All" if the user has all rights. """ - page_ids = _get_page_paths_for_action( + perm_tuples = _get_page_permission_tuples_for_action( user=user, site=site, action='delete_page', check_global=check_global, use_cache=use_cache, ) - return page_ids + return perm_tuples -def get_move_page_paths_list(user, site, check_global=True, use_cache=True): +def get_move_page_perm_tuples(user, site, check_global=True, use_cache=True): """Give a list of pages which user can move. """ - page_ids = _get_page_paths_for_action( + perm_tuples = _get_page_permission_tuples_for_action( user=user, site=site, action='move_page', check_global=check_global, use_cache=use_cache, ) - return page_ids + return perm_tuples -def get_publish_paths_list(user, site, check_global=True, use_cache=True): - """ - Give a list of page where the user has publish rights or the string "All" if - the user has all rights. - """ - page_ids = _get_page_paths_for_action( - user=user, - site=site, - action='publish_page', - check_global=check_global, - use_cache=use_cache, - ) - return page_ids +def get_publish_perm_tuples(user, site, check_global=True, use_cache=True): + """ + Give a list of page where the user has publish rights or the string "All" if + the user has all rights. + """ + perm_tuples = _get_page_permission_tuples_for_action( + user=user, + site=site, + action='publish_page', + check_global=check_global, + use_cache=use_cache, + ) + return perm_tuples -def get_view_paths_list(user, site, check_global=True, use_cache=True): +def get_view_perm_tuples(user, site, check_global=True, use_cache=True): """Give a list of pages which user can view. """ - page_ids = _get_page_paths_for_action( + perm_tuples = _get_page_permission_tuples_for_action( user=user, site=site, action='view_page', check_global=check_global, use_cache=use_cache, ) - return page_ids + return perm_tuples def has_generic_permission(page, user, action, site=None, check_global=True): if site is None: site = get_current_site() - page_path = page.node.path + "!" + page_path = page.node.path actions_map = { - 'add_page': get_add_paths_list, - 'change_page': get_change_paths_list, - 'change_page_advanced_settings': get_change_advanced_settings_paths_list, - 'change_page_permissions': get_change_permissions_paths_list, - 'delete_page': get_delete_paths_list, - 'delete_page_translation': get_delete_paths_list, - 'publish_page': get_publish_paths_list, - 'move_page': get_move_page_paths_list, - 'view_page': get_view_paths_list, + 'add_page': get_add_perm_tuples, + 'change_page': get_change_perm_tuples, + 'change_page_advanced_settings': get_change_advanced_settings_perm_tuples, + 'change_page_permissions': get_change_permissions_perm_tuples, + 'delete_page': get_delete_perm_tuples, + 'delete_page_translation': get_delete_perm_tuples, + 'publish_page': get_publish_perm_tuples, + 'move_page': get_move_page_perm_tuples, + 'view_page': get_view_perm_tuples, } func = actions_map[action] - page_paths = func(user, site, check_global=check_global) - return page_paths == GRANT_ALL_PERMISSIONS or any( - page_path.startswith(path) for path in page_paths + page_perms = func(user, site, check_global=check_global) + return page_perms == GRANT_ALL_PERMISSIONS or any( + PermissionTuple(perm).contains(page_path) for perm in page_perms ) diff --git a/cms/utils/permissions.py b/cms/utils/permissions.py index f7aa07c55b7..24f23f19e93 100644 --- a/cms/utils/permissions.py +++ b/cms/utils/permissions.py @@ -173,7 +173,7 @@ def get_global_actions_for_user(user, site): @cached_func def get_page_actions_for_user(user, site): - actions = defaultdict(set) + actions = defaultdict(list) page_permissions = ( PagePermission @@ -184,9 +184,9 @@ def get_page_actions_for_user(user, site): ) for perm in page_permissions.iterator(): - page_paths = frozenset(perm.get_page_paths()) + permission_tuple = perm.grant_on, perm.page.node.path for action in perm.get_configured_actions(): - actions[action].update(page_paths) + actions[action].append(permission_tuple) return actions @@ -238,7 +238,7 @@ def get_subordinate_users(user, site): Will return [user, C, X, D, Y, Z]. W was created by user, but is also assigned to higher level. """ - from cms.utils.page_permissions import get_change_permissions_paths_list + from cms.utils.page_permissions import get_change_permissions_perm_tuples try: user_level = get_user_permission_level(user, site) @@ -255,10 +255,10 @@ def get_subordinate_users(user, site): if user_level == ROOT_USER_LEVEL: return get_user_model().objects.all() + from cms.models import PermissionTuple allow_list = Q() - for path in get_change_permissions_paths_list(user, site, check_global=False): - allow_list |= Q(pagepermission__page__node__path=path[:-1]) if path.endswith("!") \ - else Q(pagepermission__page__node__path__startswith=path) + for perm_tuple in get_change_permissions_perm_tuples(user, site, check_global=False): + allow_list |= PermissionTuple(perm_tuple).allow_list("pagepermission__page__node") # normal query qs = get_user_model().objects.distinct().filter( @@ -277,7 +277,7 @@ def get_subordinate_groups(user, site): Similar to get_subordinate_users, but returns queryset of Groups instead of Users. """ - from cms.utils.page_permissions import get_change_permissions_paths_list + from cms.utils.page_permissions import get_change_permissions_perm_tuples try: user_level = get_user_permission_level(user, site) @@ -300,10 +300,10 @@ def get_subordinate_groups(user, site): if user_level == ROOT_USER_LEVEL: return Group.objects.all() + from cms.models import PermissionTuple allow_list = Q() - for path in get_change_permissions_paths_list(user, site, check_global=False): - allow_list |= Q(pagepermission__page__node__path=path[:-1]) if path.endswith("!") \ - else Q(pagepermission__page__node__path__startswith=path) + for perm_tuple in get_change_permissions_perm_tuples(user, site, check_global=False): + allow_list |= PermissionTuple(perm_tuple).allow_list("pagepermission__page__node") return Group.objects.distinct().filter( ( From 56599230cb62f5bb83b29593e146e3a91de94c50 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Sun, 23 Jun 2024 20:30:43 +0200 Subject: [PATCH 3/8] Deprecate `cms.utils.permissions.has_page_permission` in favor of `cms.utils.page_permissions.has_generic_permission` --- cms/models/permissionmodels.py | 2 +- cms/utils/page_permissions.py | 4 ++-- cms/utils/permissions.py | 17 ++++++++++------- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/cms/models/permissionmodels.py b/cms/models/permissionmodels.py index b69d51e9f95..bdcdb7fa1e3 100644 --- a/cms/models/permissionmodels.py +++ b/cms/models/permissionmodels.py @@ -276,7 +276,7 @@ def clean(self): def get_page_permission_tuple(self): node = self.page.node - return PermissionTuple(self.grant_on, node.path) + return PermissionTuple((self.grant_on, node.path)) def get_page_ids(self): if self.grant_on & MASK_PAGE: diff --git a/cms/utils/page_permissions.py b/cms/utils/page_permissions.py index 5a0496615f3..da59d0168bf 100644 --- a/cms/utils/page_permissions.py +++ b/cms/utils/page_permissions.py @@ -459,7 +459,7 @@ def get_view_perm_tuples(user, site, check_global=True, use_cache=True): return perm_tuples -def has_generic_permission(page, user, action, site=None, check_global=True): +def has_generic_permission(page, user, action, site=None, check_global=True, use_cache=True): if site is None: site = get_current_site() @@ -478,7 +478,7 @@ def has_generic_permission(page, user, action, site=None, check_global=True): func = actions_map[action] - page_perms = func(user, site, check_global=check_global) + page_perms = func(user, site, check_global=check_global, use_cache=use_cache) return page_perms == GRANT_ALL_PERMISSIONS or any( PermissionTuple(perm).contains(page_path) for perm in page_perms ) diff --git a/cms/utils/permissions.py b/cms/utils/permissions.py index 24f23f19e93..9c52bb43de5 100644 --- a/cms/utils/permissions.py +++ b/cms/utils/permissions.py @@ -199,13 +199,16 @@ def has_global_permission(user, site, action, use_cache=True): def has_page_permission(user, page, action, use_cache=True): - # DEPRECATE? - if use_cache: - actions = get_page_actions_for_user(user, page.node.site) - else: - actions = get_page_actions_for_user.without_cache(user, page.node.site) - path = page.node.path + "!" - return any(path.startswith(allowed_paths) for allowed_paths in actions[action]) + import warnings + + from cms.utils.compat.warnings import RemovedInDjangoCMS43Warning + from cms.utils.page_permissions import has_generic_permission + + warnings.warn("has_page_permission is deprecated and will be removed in django CMS 4.3. " + "Use cms.utils.page_permissions.has_generic_permission instead.", + RemovedInDjangoCMS43Warning, stacklevel=2) + + return has_generic_permission(page, user, action, site=None, check_global=False, use_cache=use_cache) def get_subordinate_users(user, site): From c741134b46cf6f94200ca9d4478e159a7d233aad Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Sun, 23 Jun 2024 22:13:22 +0200 Subject: [PATCH 4/8] Deprecate `.get_page_ids` since it is less efficient --- cms/cms_menus.py | 55 ++++++++++++++++++---------------- cms/models/permissionmodels.py | 9 ++++++ cms/utils/permissions.py | 9 +++++- 3 files changed, 47 insertions(+), 26 deletions(-) diff --git a/cms/cms_menus.py b/cms/cms_menus.py index a2af08f8d2f..2271d3692ba 100644 --- a/cms/cms_menus.py +++ b/cms/cms_menus.py @@ -5,7 +5,7 @@ from cms import constants from cms.apphook_pool import apphook_pool -from cms.models import EmptyPageContent, PageContent, PageUrl +from cms.models import EmptyPageContent, PageContent, PagePermission, PageUrl from cms.toolbar.utils import get_object_preview_url, get_toolbar_from_request from cms.utils.conf import get_cms_setting from cms.utils.i18n import ( @@ -16,7 +16,6 @@ ) from cms.utils.page import get_page_queryset from cms.utils.page_permissions import user_can_view_all_pages -from cms.utils.permissions import get_view_restrictions from menus.base import Menu, Modifier, NavigationNode from menus.menu_pool import menu_pool @@ -26,45 +25,51 @@ def get_visible_nodes(request, pages, site): This code is a many-pages-at-once version of cms.utils.page_permissions.user_can_view_page. `pages` contains all published pages. """ - user = request.user public_for = get_cms_setting("PUBLIC_FOR") - can_see_unrestricted = public_for == "all" or (public_for == "staff" and user.is_staff) + can_see_unrestricted = public_for == "all" or (public_for == "staff" and request.user.is_staff) - if not user.is_authenticated and not can_see_unrestricted: + if not request.user.is_authenticated and not can_see_unrestricted: # User is not authenticated and can't see unrestricted pages, # no need to check for page restrictions because if there's some, # user is anon and if there is not any, user can't see unrestricted. return [] - if user_can_view_all_pages(user, site): + if user_can_view_all_pages(request.user, site): return list(pages) - restricted_pages = get_view_restrictions(pages) - - if not restricted_pages: + if not get_cms_setting('PERMISSION'): # If there's no restrictions, let the user see all pages # only if he can see unrestricted, otherwise return no pages. return list(pages) if can_see_unrestricted else [] - user_id = user.pk - user_groups = SimpleLazyObject(lambda: frozenset(user.groups.values_list("pk", flat=True))) - is_auth_user = user.is_authenticated - - def user_can_see_page(page): - page_permissions = restricted_pages.get(page.pk, []) + restrictions = PagePermission.objects.filter( + page__in=pages, + can_view=True, + ) + restriction_map = {perm.page_id: perm for perm in restrictions} - if not page_permissions: - # Page has no view restrictions, fallback to the project's - # CMS_PUBLIC_FOR setting. - return can_see_unrestricted + user_id = request.user.pk + user_groups = SimpleLazyObject(lambda: frozenset(request.user.groups.values_list("pk", flat=True))) + is_auth_user = request.user.is_authenticated - if not is_auth_user: - return False + def user_can_see_page(page): + if page.pk in restriction_map: + # set internal fk cache to our page with loaded ancestors and descendants + PagePermission.page.field.set_cached_value(restriction_map[page.pk], page) + + restricted = False + for perm in restrictions: + if perm.get_page_permission_tuple().contains(page.node.path): + if not is_auth_user: + return False + if perm.user_id == user_id or perm.group_id in user_groups: + return True + restricted = True + + # Page has no view restrictions, fallback to the project's + # CMS_PUBLIC_FOR setting. + return can_see_unrestricted and not restricted - for perm in page_permissions: - if perm.user_id == user_id or perm.group_id in user_groups: - return True - return False return [page for page in pages if user_can_see_page(page)] diff --git a/cms/models/permissionmodels.py b/cms/models/permissionmodels.py index bdcdb7fa1e3..faa691bde44 100644 --- a/cms/models/permissionmodels.py +++ b/cms/models/permissionmodels.py @@ -279,6 +279,15 @@ def get_page_permission_tuple(self): return PermissionTuple((self.grant_on, node.path)) def get_page_ids(self): + import warnings + + from cms.utils.compat.warnings import RemovedInDjangoCMS43Warning + warnings.warn("get_page_ids is deprecated and will be removed in django CMS 4.3, " + "use get_page_permission_tuple instead", RemovedInDjangoCMS43Warning, stacklevel=2) + + return self._get_page_ids() + + def _get_page_ids(self): if self.grant_on & MASK_PAGE: yield self.page_id diff --git a/cms/utils/permissions.py b/cms/utils/permissions.py index 9c52bb43de5..c0277f8cfa2 100644 --- a/cms/utils/permissions.py +++ b/cms/utils/permissions.py @@ -1,3 +1,4 @@ +import warnings from collections import defaultdict from contextlib import contextmanager from functools import lru_cache, wraps @@ -321,6 +322,12 @@ def get_view_restrictions(pages): """ Load all view restrictions for the pages """ + + from cms.utils.compat.warnings import RemovedInDjangoCMS43Warning + + warnings.warn("get_view_restrictions will be removed in django CMS 4.3", + RemovedInDjangoCMS43Warning, stacklevel=2) + restricted_pages = defaultdict(list) if not get_cms_setting('PERMISSION'): @@ -348,7 +355,7 @@ def get_view_restrictions(pages): # set internal fk cache to our page with loaded ancestors and descendants PagePermission.page.field.set_cached_value(perm, pages_by_id[perm.page_id]) - for page_id in perm.get_page_ids(): + for page_id in perm._get_page_ids(): restricted_pages[page_id].append(perm) return restricted_pages From 5c73a8ec0f0e581e17d8bdc5b78826ad208ffa37 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Mon, 24 Jun 2024 15:16:59 +0200 Subject: [PATCH 5/8] Fix: get_draft_placholders --- cms/utils/page_permissions.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cms/utils/page_permissions.py b/cms/utils/page_permissions.py index da59d0168bf..27255bf01f7 100644 --- a/cms/utils/page_permissions.py +++ b/cms/utils/page_permissions.py @@ -34,7 +34,10 @@ def _get_draft_placeholders(page, language): - return page.get_placeholders(language) + from cms.models import PageContent, Placeholder + + page_content = PageContent.admin_manager.current_content().get(language=language, page=page) + return Placeholder.objects.get_for_obj(page_content) def _check_delete_translation(user, page, language, site=None): @@ -479,6 +482,6 @@ def has_generic_permission(page, user, action, site=None, check_global=True, use func = actions_map[action] page_perms = func(user, site, check_global=check_global, use_cache=use_cache) - return page_perms == GRANT_ALL_PERMISSIONS or any( + return page_perms == False or any( PermissionTuple(perm).contains(page_path) for perm in page_perms ) From ebf550568dc492dd239511b74d146e1fd52cd458 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Mon, 24 Jun 2024 17:22:29 +0200 Subject: [PATCH 6/8] Fix: Delete permissions --- cms/admin/pageadmin.py | 2 +- cms/templates/admin/cms/page/tree/menu.html | 4 +-- cms/tests/test_page_admin.py | 10 +++---- cms/utils/mail.py | 2 +- cms/utils/page_permissions.py | 31 +++++++++++---------- 5 files changed, 26 insertions(+), 23 deletions(-) diff --git a/cms/admin/pageadmin.py b/cms/admin/pageadmin.py index 5e3a54d9c04..b8835443c78 100644 --- a/cms/admin/pageadmin.py +++ b/cms/admin/pageadmin.py @@ -1047,7 +1047,7 @@ def has_change_permission(self, request, obj=None): return page_permissions.user_can_change_page(request.user, page=obj.page, site=site) can_change_page = page_permissions.user_can_change_at_least_one_page( user=request.user, - site=get_site(request), + site=site, use_cache=False, ) return can_change_page diff --git a/cms/templates/admin/cms/page/tree/menu.html b/cms/templates/admin/cms/page/tree/menu.html index f5c6be98eda..63b2b1d4d60 100644 --- a/cms/templates/admin/cms/page/tree/menu.html +++ b/cms/templates/admin/cms/page/tree/menu.html @@ -138,7 +138,7 @@ {% get_admin_url_for_language page preview_language as content_admin_url %} - {% if content_admin_url and page_content %} + {% if has_change_permission and page_content and content_admin_url %} {% endif %} {% trans "Page settings (SHIFT click for advanced settings)" %} - {% if has_change_permission and content_admin_url or has_change_advanced_settings_permission %} + {% if has_change_permission and page_content and content_admin_url %} {% endif %} diff --git a/cms/tests/test_page_admin.py b/cms/tests/test_page_admin.py index 350d02e5629..b6a6ec07c8d 100644 --- a/cms/tests/test_page_admin.py +++ b/cms/tests/test_page_admin.py @@ -1948,7 +1948,7 @@ def test_user_cant_delete_non_empty_page(self): self._add_plugin_to_page(page) self.add_permission(staff_user, 'change_page') - self.add_permission(staff_user, 'delete_page') + self.remove_permission(staff_user, 'delete_page') gp = self.add_global_permission(staff_user, can_change=True, can_delete=True) with self.login_user_context(staff_user): @@ -2066,7 +2066,7 @@ def test_user_cant_delete_non_empty_translation(self): self._add_plugin_to_page(page, language=translation.language) self.add_permission(staff_user, 'change_page') - self.add_permission(staff_user, 'delete_page') + self.remove_permission(staff_user, 'delete_page') self.add_global_permission(staff_user, can_change=True, can_delete=True) with self.login_user_context(staff_user): @@ -3410,8 +3410,8 @@ def test_user_cant_delete_non_empty_page(self): page_perm = self.add_page_permission( staff_user, page, - can_change=True, - can_delete=True, + can_change=False, + can_delete=False, ) with self.login_user_context(staff_user): @@ -3548,7 +3548,7 @@ def test_user_cant_delete_non_empty_translation(self): self.add_page_permission( staff_user, page, - can_change=True, + can_change=False, can_delete=True, ) diff --git a/cms/utils/mail.py b/cms/utils/mail.py index 8f6f53e4d72..e6b01055033 100644 --- a/cms/utils/mail.py +++ b/cms/utils/mail.py @@ -14,7 +14,7 @@ def send_mail(subject, txt_template, to, context=None, html_template=None, fail_ context = context or {} context.update({ - 'login_url': "http://%s" % urljoin(site.domain, admin_reverse('index')), + 'login_url': "https://%s" % urljoin(site.domain, admin_reverse('index')), 'title': subject, }) diff --git a/cms/utils/page_permissions.py b/cms/utils/page_permissions.py index 27255bf01f7..3263b0f7394 100644 --- a/cms/utils/page_permissions.py +++ b/cms/utils/page_permissions.py @@ -33,11 +33,19 @@ } -def _get_draft_placeholders(page, language): +def _get_all_placeholders(page, language=None): + from django.contrib.contenttypes.models import ContentType + from cms.models import PageContent, Placeholder - page_content = PageContent.admin_manager.current_content().get(language=language, page=page) - return Placeholder.objects.get_for_obj(page_content) + page_contents = PageContent.admin_manager.filter(page=page) + if language: + page_contents = page_contents.filter(language=language) + content_type = ContentType.objects.get_for_model(Placeholder) + return Placeholder.objects.filter( + content_type=content_type, + object_id__in=page_contents.values_list('pk', flat=True) + ) def _check_delete_translation(user, page, language, site=None): @@ -165,15 +173,10 @@ def user_can_delete_page(user, page, site=None): if not has_perm: return False - for language in page.get_languages(): - placeholders = ( - _get_draft_placeholders(page, language) - .filter(cmsplugin__language=language) - .distinct() - ) - for placeholder in placeholders: - if not placeholder.has_delete_plugins_permission(user, [language]): - return False + placeholders = _get_all_placeholders(page) + for placeholder in placeholders: + if not placeholder.has_delete_plugins_permission(user, [placeholders.source.language]): + return False return True @@ -191,7 +194,7 @@ def user_can_delete_page_translation(user, page, language, site=None): return False placeholders = ( - _get_draft_placeholders(page, language) + _get_all_placeholders(page, language) .filter(cmsplugin__language=language) .distinct() ) @@ -482,6 +485,6 @@ def has_generic_permission(page, user, action, site=None, check_global=True, use func = actions_map[action] page_perms = func(user, site, check_global=check_global, use_cache=use_cache) - return page_perms == False or any( + return page_perms == GRANT_ALL_PERMISSIONS or any( PermissionTuple(perm).contains(page_path) for perm in page_perms ) From ddde761b5db562e51cd2c2253b5019b40d46f669 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Tue, 25 Jun 2024 23:29:09 +0200 Subject: [PATCH 7/8] Add compatibility shims with deprecation warning --- cms/models/permissionmodels.py | 14 ++++--- cms/utils/page_permissions.py | 67 ++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/cms/models/permissionmodels.py b/cms/models/permissionmodels.py index faa691bde44..daca7078d50 100644 --- a/cms/models/permissionmodels.py +++ b/cms/models/permissionmodels.py @@ -230,18 +230,20 @@ def contains(self, path: str, steplen: int = TreeNode.steplen) -> bool: return path.startswith(perm_path) and len(path) <= len(perm_path) + steplen return False - def allow_list(self, filter: str, steplen: int = TreeNode.steplen) -> Q: + def allow_list(self, filter: str = "", steplen: int = TreeNode.steplen) -> Q: + if filter !="": + filter = f"{filter}__" grant_on, path = self if grant_on == ACCESS_PAGE: - return Q(**{f"{filter}__path": path}) + return Q(**{f"{filter}path": path}) elif grant_on == ACCESS_CHILDREN: - return Q(**{f"{filter}__path__startswith": path, f"{filter}__path__length": len(path) + steplen}) + return Q(**{f"{filter}path__startswith": path, f"{filter}__path__length": len(path) + steplen}) elif grant_on == ACCESS_DESCENDANTS: - return Q(**{f"{filter}__path__startswith": path, f"{filter}__path__length__gt": len(path)}) + return Q(**{f"{filter}path__startswith": path, f"{filter}__path__length__gt": len(path)}) elif grant_on == ACCESS_PAGE_AND_DESCENDANTS: - return Q(**{f"{filter}__path__startswith": path}) + return Q(**{f"{filter}path__startswith": path}) elif grant_on == ACCESS_PAGE_AND_CHILDREN: - return Q(**{f"{filter}__path__startswith": path, f"{filter}__path__length__lte": len(path) + steplen}) + return Q(**{f"{filter}path__startswith": path, f"{filter}__path__length__lte": len(path) + steplen}) return Q() diff --git a/cms/utils/page_permissions.py b/cms/utils/page_permissions.py index 3263b0f7394..5cc6313889a 100644 --- a/cms/utils/page_permissions.py +++ b/cms/utils/page_permissions.py @@ -5,6 +5,7 @@ from cms.models import Page, PermissionTuple from cms.utils import get_current_site from cms.utils.compat.dj import available_attrs +from cms.utils.compat.warnings import RemovedInDjangoCMS43Warning from cms.utils.conf import get_cms_setting from cms.utils.permissions import ( cached_func, @@ -351,6 +352,22 @@ def user_can_view_all_pages(user, site): return has_global_permission(user, site, action='view_page') +def _perm_tuples_to_ids(perm_tuples): + import inspect + import warnings + from django.db.models import Q + + fn_name = "_".join(inspect.stack()[1][3].split("_")[:-1]) # Calling function's name + warnings.warn(f"{fn_name}_ids is deprecated. Use {fn_name}_perm_tuples instead.", + RemovedInDjangoCMS43Warning, stacklevel=3) + + allowed_pages = Q() + for perm in perm_tuples: + allowed_pages |= PermissionTuple(perm).allow_list("node") + + return list(Page.objects.filter(allowed_pages).values_list('pk', flat=True)) + + def get_add_perm_tuples(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has add page rights or the string @@ -366,6 +383,11 @@ def get_add_perm_tuples(user, site, check_global=True, use_cache=True): return perm_tuples +def get_add_ids(user, site, check_global=True, use_cache=True): + perm_tuples = get_add_perm_tuples(user, site, check_global=check_global, use_cache=use_cache) + return _perm_tuples_to_ids(perm_tuples) + + def get_change_perm_tuples(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has edit rights or the string "All" if @@ -381,6 +403,11 @@ def get_change_perm_tuples(user, site, check_global=True, use_cache=True): return perm_tuples +def get_change_ids(user, site, check_global=True, use_cache=True): + perm_tuples = get_change_perm_tuples(user, site, check_global=check_global, use_cache=use_cache) + return _perm_tuples_to_ids(perm_tuples) + + def get_change_advanced_settings_perm_tuples(user, site, check_global=True, use_cache=True): """ Give a list of page where the user can change advanced settings or the @@ -396,6 +423,16 @@ def get_change_advanced_settings_perm_tuples(user, site, check_global=True, use_ return perm_tuples +def get_change_advanced_settings_ids(user, site, check_global=True, use_cache=True): + perm_tuples = get_change_advanced_settings_perm_tuples( + user=user, + site=site, + check_global=check_global, + use_cache=use_cache, + ) + return _perm_tuples_to_ids(perm_tuples) + + def get_change_permissions_perm_tuples(user, site, check_global=True, use_cache=True): """Give a list of page where the user can change permissions. """ @@ -409,6 +446,16 @@ def get_change_permissions_perm_tuples(user, site, check_global=True, use_cache= return perm_tuples +def get_change_permissions_ids(user, site, check_global=True, use_cache=True): + perm_tuples = get_change_permissions_perm_tuples( + user=user, + site=site, + check_global=check_global, + use_cache=use_cache, + ) + return _perm_tuples_to_ids(perm_tuples) + + def get_delete_perm_tuples(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has delete rights or the string "All" if @@ -424,6 +471,11 @@ def get_delete_perm_tuples(user, site, check_global=True, use_cache=True): return perm_tuples +def get_delete_ids(user, site, check_global=True, use_cache=True): + perm_tuples = get_delete_perm_tuples(user, site, check_global=check_global, use_cache=use_cache) + return _perm_tuples_to_ids(perm_tuples) + + def get_move_page_perm_tuples(user, site, check_global=True, use_cache=True): """Give a list of pages which user can move. """ @@ -437,6 +489,11 @@ def get_move_page_perm_tuples(user, site, check_global=True, use_cache=True): return perm_tuples +def get_move_page_ids(user, site, check_global=True, use_cache=True): + perm_tuples = get_move_page_perm_tuples(user, site, check_global=check_global, use_cache=use_cache) + return _perm_tuples_to_ids(perm_tuples) + + def get_publish_perm_tuples(user, site, check_global=True, use_cache=True): """ Give a list of page where the user has publish rights or the string "All" if @@ -452,6 +509,11 @@ def get_publish_perm_tuples(user, site, check_global=True, use_cache=True): return perm_tuples +def get_publish_ids(user, site, check_global=True, use_cache=True): + perm_tuples = get_publish_perm_tuples(user, site, check_global=check_global, use_cache=use_cache) + return _perm_tuples_to_ids(perm_tuples) + + def get_view_perm_tuples(user, site, check_global=True, use_cache=True): """Give a list of pages which user can view. """ @@ -465,6 +527,11 @@ def get_view_perm_tuples(user, site, check_global=True, use_cache=True): return perm_tuples +def get_view_ids(user, site, check_global=True, use_cache=True): + perm_tuples = get_view_perm_tuples(user, site, check_global=check_global, use_cache=use_cache) + return _perm_tuples_to_ids(perm_tuples) + + def has_generic_permission(page, user, action, site=None, check_global=True, use_cache=True): if site is None: site = get_current_site() From 3975a11e339b2e57f75b7e0e47686699ce767649 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Wed, 26 Jun 2024 08:11:59 +0200 Subject: [PATCH 8/8] Fix ruff issue --- cms/utils/page_permissions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/utils/page_permissions.py b/cms/utils/page_permissions.py index 5cc6313889a..2d5475c6caa 100644 --- a/cms/utils/page_permissions.py +++ b/cms/utils/page_permissions.py @@ -355,6 +355,7 @@ def user_can_view_all_pages(user, site): def _perm_tuples_to_ids(perm_tuples): import inspect import warnings + from django.db.models import Q fn_name = "_".join(inspect.stack()[1][3].split("_")[:-1]) # Calling function's name