From f59e11f685fa4c46422c7210d548eb535ff79bc2 Mon Sep 17 00:00:00 2001 From: Nicolas Clerc Date: Mon, 27 Jul 2026 10:32:41 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20add=20restrict=20ability?= =?UTF-8?q?=20with=20activation=20and=20deactivation=20states?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only an explicit owner can toggle restriction on a folder. A folder needs a parent to host its restriction on activation, while an already restricted folder lives at the tree root and must stay deactivatable. Listings annotate the restriction presence so the ability stays free of extra queries. --- src/backend/core/api/viewsets.py | 4 +- src/backend/core/permissions/backends/role.py | 30 ++++++- src/backend/core/tests/test_models_items.py | 18 +++- .../tests/test_models_items_restricted.py | 88 +++++++++++++++++++ .../core/tests/test_models_items_root.py | 15 +++- 5 files changed, 148 insertions(+), 7 deletions(-) diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 6dbf1a32..90d5d235 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -457,7 +457,7 @@ class ItemViewSet( def get_queryset(self): """Get queryset performing all annotation and filtering on the item tree structure.""" user = self.request.user - queryset = super().get_queryset().select_related("creator") + queryset = super().get_queryset().select_related("creator").annotate_has_restriction() # Remove items with upload_state SUSPICIOUS for non-creators queryset = self._filter_suspicious_items(queryset, user) @@ -522,7 +522,7 @@ class ItemViewSet( for path in root_paths: path_list |= db.Q(path__descendants=path) - queryset = self.queryset.select_related("creator") + queryset = self.queryset.select_related("creator").annotate_has_restriction() # Remove items with upload_state SUSPICIOUS for non-creators queryset = self._filter_suspicious_items(queryset, user) queryset = self._exclude_pending_items(queryset) diff --git a/src/backend/core/permissions/backends/role.py b/src/backend/core/permissions/backends/role.py index 4554f093..1674afc6 100644 --- a/src/backend/core/permissions/backends/role.py +++ b/src/backend/core/permissions/backends/role.py @@ -2,6 +2,8 @@ from __future__ import annotations +from functools import cached_property + from django.conf import settings from django.contrib.auth.models import AnonymousUser from django.db.models import Q, QuerySet @@ -37,6 +39,16 @@ class ItemAbilities: # pylint: disable=too-many-public-methods self.is_owner = self.access_role == RoleChoices.OWNER self.is_owner_or_admin = self.is_owner or self.access_role == RoleChoices.ADMIN + @cached_property + def is_restricted(self) -> bool: + """Return whether the item is a restricted folder, querying once and only for roots.""" + # Only a folder at the tree root can be restricted + return ( + self.item.type == models.ItemTypeChoices.FOLDER + and self.item.is_root + and self.item.is_restricted + ) + def has_access_role(self) -> bool: """Return whether the user holds a role through accesses on a live item.""" # Based on accesses only so that anonymous users granted by a link @@ -61,6 +73,10 @@ class ItemAbilities: # pylint: disable=too-many-public-methods """Return whether the user can manage the item and its accesses.""" return self.is_owner_or_admin and not self.is_deleted + def can_move(self) -> bool: + """Return whether the user can move the item: a restricted folder must stay at the root.""" + return self.can_manage() and not self.is_restricted + def can_update(self) -> bool: """Return whether the user can modify the item.""" return (self.is_owner_or_admin or self.role == RoleChoices.EDITOR) and not self.is_deleted @@ -111,6 +127,17 @@ class ItemAbilities: # pylint: disable=too-many-public-methods and bool(settings.WOPI_ONLYOFFICE_CONVERT_JWT_SECRET) ) + def can_restrict(self) -> bool: + """Return whether the user can toggle restriction on the folder.""" + # A restricted folder lives at the tree root: deactivation must stay + # possible there, while activation requires a parent for the restriction + return ( + self.is_owner + and not self.is_deleted + and self.item.type == models.ItemTypeChoices.FOLDER + and (self.item.depth > 1 or self.is_restricted) + ) + def can_favorite(self) -> bool: """Return whether the user can mark the item as favorite.""" return self.can_get() and self.user.is_authenticated @@ -144,7 +171,8 @@ class ItemAbilities: # pylint: disable=too-many-public-methods "link_configuration": self.can_manage(), "invite_owner": self.can_invite_owner(), "link_select_options": self.link_select_options(), - "move": self.can_manage(), + "move": self.can_move(), + "restrict": self.can_restrict(), "restore": self.can_restore(), "retrieve": self.can_retrieve(), "tree": self.can_get(), diff --git a/src/backend/core/tests/test_models_items.py b/src/backend/core/tests/test_models_items.py index eb3f6181..f8b6c1d5 100644 --- a/src/backend/core/tests/test_models_items.py +++ b/src/backend/core/tests/test_models_items.py @@ -319,6 +319,7 @@ def test_models_items_get_abilities_forbidden( "link_configuration": False, "link_select_options": {}, "partial_update": False, + "restrict": False, "restore": False, "retrieve": False, "tree": False, @@ -369,6 +370,7 @@ def test_models_items_get_abilities_reader(is_authenticated, reach, django_asser "download": True, "move": False, "partial_update": False, + "restrict": False, "restore": False, "retrieve": True, "tree": True, @@ -474,6 +476,7 @@ def test_models_items_get_abilities_editor( # noqa: PLR0913 "download": True, "move": False, "partial_update": True, + "restrict": False, "restore": False, "retrieve": True, "tree": True, @@ -544,6 +547,7 @@ def test_models_items_not_root_get_abilities_owner( "download": True, "move": True, "partial_update": True, + "restrict": False, "restore": True, "retrieve": True, "tree": True, @@ -552,7 +556,9 @@ def test_models_items_not_root_get_abilities_owner( "wopi": True, "convert": False, } - with django_assert_num_queries(1): + # A folder at the tree root checks for a targeting restriction + nb_queries = 2 if item_type == models.ItemTypeChoices.FOLDER else 1 + with django_assert_num_queries(nb_queries): assert item.get_abilities(user) == expected_abilities item.soft_delete() item.refresh_from_db() @@ -574,6 +580,7 @@ def test_models_items_not_root_get_abilities_owner( "download": False, "move": False, "partial_update": False, + "restrict": False, "restore": True, "retrieve": True, "tree": False, @@ -634,6 +641,7 @@ def test_models_items_not_root_get_abilities_administrator( "download": True, "move": True, "partial_update": True, + "restrict": False, "restore": False, "retrieve": True, "tree": True, @@ -642,7 +650,9 @@ def test_models_items_not_root_get_abilities_administrator( "wopi": True, "convert": False, } - with django_assert_num_queries(1): + # A folder at the tree root checks for a targeting restriction + nb_queries = 2 if item_type == models.ItemTypeChoices.FOLDER else 1 + with django_assert_num_queries(nb_queries): assert item.get_abilities(user) == expected_abilities item.soft_delete() item.refresh_from_db() @@ -704,6 +714,7 @@ def test_models_items_not_root_get_abilities_editor_user( "download": True, "move": False, "partial_update": True, + "restrict": False, "restore": False, "retrieve": True, "tree": True, @@ -757,6 +768,7 @@ def test_models_items_not_root_get_abilities_reader_user(django_assert_num_queri "download": True, "move": False, "partial_update": access_from_link, + "restrict": False, "restore": False, "retrieve": True, "tree": True, @@ -815,6 +827,7 @@ def test_models_items_get_abilities_hard_delete_non_root_by_non_creator( "media_auth": True, "move": True, "partial_update": True, + "restrict": False, "restore": True, "retrieve": True, "tree": True, @@ -846,6 +859,7 @@ def test_models_items_get_abilities_hard_delete_non_root_by_non_creator( "media_auth": False, "move": False, "partial_update": False, + "restrict": False, "restore": True, "retrieve": True, "tree": False, diff --git a/src/backend/core/tests/test_models_items_restricted.py b/src/backend/core/tests/test_models_items_restricted.py index 1ee2ef8d..6de925b7 100644 --- a/src/backend/core/tests/test_models_items_restricted.py +++ b/src/backend/core/tests/test_models_items_restricted.py @@ -9,6 +9,94 @@ from core import factories, models pytestmark = pytest.mark.django_db +@pytest.mark.parametrize( + "role,expected", + [ + ("owner", True), + ("administrator", False), + ("editor", False), + ("reader", False), + ], +) +def test_models_items_restricted_get_abilities_restrict_requires_owner(role, expected): + """Only an owner can restrict a folder.""" + user = factories.UserFactory() + parent = factories.ItemFactory(type=models.ItemTypeChoices.FOLDER) + folder = factories.ItemFactory(parent=parent, type=models.ItemTypeChoices.FOLDER) + factories.UserItemAccessFactory(item=folder, user=user, role=role) + + abilities = folder.get_abilities(user) + + assert abilities["restrict"] is expected + + +def test_models_items_restricted_get_abilities_restrict_forbidden_on_roots(): + """A root folder cannot be restricted: no parent can hold its restriction.""" + user = factories.UserFactory() + folder = factories.ItemFactory( + type=models.ItemTypeChoices.FOLDER, + users=[(user, models.RoleChoices.OWNER)], + ) + + abilities = folder.get_abilities(user) + + assert abilities["restrict"] is False + + +def test_models_items_restricted_get_abilities_restrict_allowed_on_restricted_root(): + """An explicit owner can deactivate a restricted folder moved to the root.""" + user = factories.UserFactory() + folder = factories.ItemFactory( + type=models.ItemTypeChoices.FOLDER, + users=[(user, models.RoleChoices.OWNER)], + ) + factories.RestrictionFactory(target=folder) + + abilities = folder.get_abilities(user) + + assert abilities["restrict"] is True + + +def test_models_items_restricted_get_abilities_move_forbidden_on_restricted_root(): + """A restricted folder cannot be moved, even by an explicit owner.""" + user = factories.UserFactory() + folder = factories.ItemFactory( + type=models.ItemTypeChoices.FOLDER, + users=[(user, models.RoleChoices.OWNER)], + ) + + assert folder.get_abilities(user)["move"] is True + + factories.RestrictionFactory(target=folder) + + assert folder.get_abilities(user)["move"] is False + + +def test_models_items_restricted_get_abilities_restrict_forbidden_on_files(): + """A file cannot be restricted.""" + user = factories.UserFactory() + parent = factories.ItemFactory(type=models.ItemTypeChoices.FOLDER) + item = factories.ItemFactory(parent=parent, type=models.ItemTypeChoices.FILE) + factories.UserItemAccessFactory(item=item, user=user, role="owner") + + abilities = item.get_abilities(user) + + assert abilities["restrict"] is False + + +def test_models_items_restricted_get_abilities_restrict_forbidden_when_deleted(): + """A soft deleted folder cannot be restricted.""" + user = factories.UserFactory() + parent = factories.ItemFactory(type=models.ItemTypeChoices.FOLDER) + folder = factories.ItemFactory(parent=parent, type=models.ItemTypeChoices.FOLDER) + factories.UserItemAccessFactory(item=folder, user=user, role="owner") + folder.soft_delete() + + abilities = folder.get_abilities(user) + + assert abilities["restrict"] is False + + def test_models_items_restricted_folder_can_be_restricted(): """A folder is restricted when a restriction targets it.""" folder = factories.ItemFactory(type=models.ItemTypeChoices.FOLDER) diff --git a/src/backend/core/tests/test_models_items_root.py b/src/backend/core/tests/test_models_items_root.py index ccb62272..0c8378e2 100644 --- a/src/backend/core/tests/test_models_items_root.py +++ b/src/backend/core/tests/test_models_items_root.py @@ -65,6 +65,7 @@ def test_models_sub_item_abilities_downgraded(): "download": True, "move": False, "partial_update": True, + "restrict": False, "restore": False, "retrieve": True, "tree": True, @@ -101,6 +102,7 @@ def test_models_sub_item_abilities_downgraded(): "download": True, "move": False, "partial_update": False, + "restrict": False, "restore": False, "retrieve": True, "tree": True, @@ -155,6 +157,7 @@ def test_models_items_root_get_abilities_owner( "download": True, "move": True, "partial_update": True, + "restrict": False, "restore": True, "retrieve": True, "tree": True, @@ -163,7 +166,9 @@ def test_models_items_root_get_abilities_owner( "wopi": True, "convert": False, } - with django_assert_num_queries(1): + # A folder at the tree root checks for a targeting restriction + nb_queries = 2 if item_type == models.ItemTypeChoices.FOLDER else 1 + with django_assert_num_queries(nb_queries): assert item.get_abilities(user) == expected_abilities item.soft_delete() item.refresh_from_db() @@ -185,6 +190,7 @@ def test_models_items_root_get_abilities_owner( "download": False, "move": False, "partial_update": False, + "restrict": False, "restore": True, "retrieve": True, "tree": False, @@ -242,6 +248,7 @@ def test_models_items_root_get_abilities_administrator( "download": True, "move": True, "partial_update": True, + "restrict": False, "restore": False, "retrieve": True, "tree": True, @@ -250,7 +257,9 @@ def test_models_items_root_get_abilities_administrator( "wopi": True, "convert": False, } - with django_assert_num_queries(1): + # A folder at the tree root checks for a targeting restriction + nb_queries = 2 if item_type == models.ItemTypeChoices.FOLDER else 1 + with django_assert_num_queries(nb_queries): assert item.get_abilities(user) == expected_abilities item.soft_delete() item.refresh_from_db() @@ -308,6 +317,7 @@ def test_models_items_root_get_abilities_editor_user( "download": True, "move": False, "partial_update": True, + "restrict": False, "restore": False, "retrieve": True, "tree": True, @@ -361,6 +371,7 @@ def test_models_items_root_get_abilities_reader_user( "download": True, "move": False, "partial_update": access_from_link, + "restrict": False, "restore": False, "retrieve": True, "tree": True,