From 8cfa413d22ef4eaea0fcaa0bb959488510b64924 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Fri, 7 Nov 2025 15:21:16 +0100 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F(backend)=20limit=20accesses?= =?UTF-8?q?=20list=20to=20the=20last=20access=20for=20a=20user?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don't need the whole accesses list for all the users on the list endpoint. We want to return the last access, meaning the access on the deepest item in the items tree. --- src/backend/core/api/viewsets.py | 46 ++++++---- .../tests/items/test_api_item_accesses.py | 92 ++----------------- 2 files changed, 36 insertions(+), 102 deletions(-) diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 385db09d..46875a86 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -4,6 +4,7 @@ import json import logging import re +from collections import defaultdict from urllib.parse import unquote, urlparse from django.conf import settings @@ -1415,30 +1416,36 @@ class ItemAccessViewSet( ) ancestor_items = list(ancestors_qs.order_by("path")) - queryset = self.get_queryset().filter(item__in=ancestors_qs) + accesses_queryset = self.get_queryset().filter(item__in=ancestors_qs) if role not in PRIVILEGED_ROLES: # Restrict the queryset to only privileged roles # to not leak information to non-privileged users - queryset = queryset.filter(role__in=PRIVILEGED_ROLES) - - accesses = list(queryset.order_by("item__path", "created_at")) + accesses_queryset = accesses_queryset.filter(role__in=PRIVILEGED_ROLES) + + accesses = list(accesses_queryset.order_by("item__path", "created_at")) + + request_target_keys = {f"user:{user.id}"} + request_target_keys.update( + f"team:{team}" for team in getattr(user, "teams", []) + ) max_role_by_target = {} user_roles_by_path = {} - accesses_by_target = {} + accesses_by_target = defaultdict(list) for access in accesses: - previous_max_role_ancestor = max_role_by_target.get(access.target_key, {}) - previous_role = previous_max_role_ancestor.get("role") + previous = max_role_by_target.get(access.target_key) + previous_role = previous["role"] if previous else None access.max_ancestors_role = previous_role - access.max_ancestors_role_item_id = previous_max_role_ancestor.get( - "item_id" + access.max_ancestors_role_item_id = ( + previous["item_id"] if previous else None ) + max_role_by_target[access.target_key] = { "role": models.RoleChoices.max(previous_role, access.role), "item_id": access.item_id, } - accesses_by_target.setdefault(access.target_key, []).append(access) + accesses_by_target[access.target_key].append(access) if access.target_key in request_target_keys: path = str(access.item.path) @@ -1451,22 +1458,23 @@ class ItemAccessViewSet( for ancestor in ancestor_items: path = str(ancestor.path) parent_path = str(ancestor.path[:-1]) if ancestor.depth > 1 else "" - ancestors_role = user_max_role_by_path.get(parent_path) - user_ancestor_role_by_path[path] = ancestors_role + ancestor_role = user_max_role_by_path.get(parent_path) + user_ancestor_role_by_path[path] = ancestor_role user_max_role_by_path[path] = models.RoleChoices.max( - ancestors_role, user_roles_by_path.get(path) + ancestor_role, user_roles_by_path.get(path) ) - selected_access_ids = set() - for target_accesses in accesses_by_target.values(): - target_accesses.sort( + selected_access_ids = { + max( + target_accesses, key=lambda item_access: ( item_access.item.depth if item_access.item_id else 0, item_access.created_at, ), - reverse=True, - ) - selected_access_ids.update(access.pk for access in target_accesses[:2]) + ).pk + for target_accesses in accesses_by_target.values() + if target_accesses + } # serialize and return the response context = self.get_serializer_context() diff --git a/src/backend/core/tests/items/test_api_item_accesses.py b/src/backend/core/tests/items/test_api_item_accesses.py index 62ac2816..c389e118 100644 --- a/src/backend/core/tests/items/test_api_item_accesses.py +++ b/src/backend/core/tests/items/test_api_item_accesses.py @@ -325,16 +325,15 @@ def test_api_item_accesses_retrieve_set_role_to_child(): assert result_dict[str(parent_access.id)] == [] # Add an access for the other user on the parent - parent_access_other_user = factories.UserItemAccessFactory( - item=parent, user=other_user, role="editor" - ) + factories.UserItemAccessFactory(item=parent, user=other_user, role="editor") response = client.get(f"/api/v1.0/items/{item.id!s}/accesses/") assert response.status_code == 200 content = response.json() - assert len(content) == 3 + assert len(content) == 2 + # the new added access is not present in the result so the list should be the same result_dict = { result["id"]: result["abilities"]["set_role_to"] for result in content } @@ -344,12 +343,6 @@ def test_api_item_accesses_retrieve_set_role_to_child(): "owner", ] assert result_dict[str(parent_access.id)] == [] - assert result_dict[str(parent_access_other_user.id)] == [ - "reader", - "editor", - "administrator", - "owner", - ] @pytest.mark.parametrize( @@ -359,7 +352,6 @@ def test_api_item_accesses_retrieve_set_role_to_child(): ["administrator", "reader", "reader", "reader"], [ ["reader", "editor", "administrator"], - [], ["reader", "editor", "administrator"], ], ], @@ -367,7 +359,6 @@ def test_api_item_accesses_retrieve_set_role_to_child(): ["owner", "reader", "reader", "reader"], [ ["reader", "editor", "administrator", "owner"], - [], ["reader", "editor", "administrator", "owner"], ], ], @@ -375,7 +366,6 @@ def test_api_item_accesses_retrieve_set_role_to_child(): ["owner", "reader", "reader", "owner"], [ ["reader", "editor", "administrator", "owner"], - [], ["reader", "editor", "administrator", "owner"], ], ], @@ -402,9 +392,9 @@ def test_api_item_accesses_list_authenticated_related_same_user(roles, results): # Create accesses for another user other_user = factories.UserFactory() factories.UserItemAccessFactory(item=grand_parent, user=other_user, role=roles[1]) + factories.UserItemAccessFactory(item=parent, user=other_user, role=roles[2]) accesses = [ factories.UserItemAccessFactory(item=item, user=user, role=roles[0]), - factories.UserItemAccessFactory(item=parent, user=other_user, role=roles[2]), factories.UserItemAccessFactory(item=item, user=other_user, role=roles[3]), ] @@ -412,7 +402,7 @@ def test_api_item_accesses_list_authenticated_related_same_user(roles, results): assert response.status_code == 200 content = response.json() - assert len(content) == 3 + assert len(content) == 2 for result in content: assert ( @@ -434,7 +424,6 @@ def test_api_item_accesses_list_authenticated_related_same_user(roles, results): ["administrator", "reader", "reader", "reader"], [ ["reader", "editor", "administrator"], - [], ["reader", "editor", "administrator"], ], ], @@ -442,7 +431,6 @@ def test_api_item_accesses_list_authenticated_related_same_user(roles, results): ["owner", "reader", "reader", "reader"], [ ["reader", "editor", "administrator", "owner"], - [], ["reader", "editor", "administrator", "owner"], ], ], @@ -450,7 +438,6 @@ def test_api_item_accesses_list_authenticated_related_same_user(roles, results): ["owner", "reader", "reader", "owner"], [ ["reader", "editor", "administrator", "owner"], - [], ["reader", "editor", "administrator", "owner"], ], ], @@ -458,7 +445,6 @@ def test_api_item_accesses_list_authenticated_related_same_user(roles, results): ["reader", "reader", "reader", "owner"], [ ["reader", "editor", "administrator", "owner"], - [], ["reader", "editor", "administrator", "owner"], ], ], @@ -467,7 +453,6 @@ def test_api_item_accesses_list_authenticated_related_same_user(roles, results): [ ["reader", "editor", "administrator"], [], - [], ], ], [ @@ -475,7 +460,6 @@ def test_api_item_accesses_list_authenticated_related_same_user(roles, results): [ ["reader", "editor", "administrator"], [], - [], ], ], ], @@ -502,10 +486,10 @@ def test_api_item_accesses_list_authenticated_related_same_team( mock_user_teams.return_value = ["lasuite", "unknown"] factories.TeamItemAccessFactory(item=grand_parent, team="lasuite", role=roles[1]) + factories.TeamItemAccessFactory(item=parent, team="lasuite", role=roles[2]) accesses = [ factories.UserItemAccessFactory(item=item, user=user, role=roles[0]), # Create accesses for a team - factories.TeamItemAccessFactory(item=parent, team="lasuite", role=roles[2]), factories.TeamItemAccessFactory(item=item, team="lasuite", role=roles[3]), ] @@ -513,7 +497,7 @@ def test_api_item_accesses_list_authenticated_related_same_team( assert response.status_code == 200 content = response.json() - assert len(content) == 3 + assert len(content) == 2 for result in content: assert ( @@ -1523,7 +1507,7 @@ def test_api_item_accesses_explicit(): item = factories.ItemFactory(parent=parent, type=models.ItemTypeChoices.FOLDER) # explicit access on root. - root_access = factories.UserItemAccessFactory(item=root, user=user, role="editor") + factories.UserItemAccessFactory(item=root, user=user, role="editor") # explicit access on item. item_access = factories.UserItemAccessFactory(item=item, user=user, role="owner") other_admin_access = factories.UserItemAccessFactory( @@ -1537,34 +1521,6 @@ def test_api_item_accesses_explicit(): assert response.status_code == 200 content = response.json() assert content == [ - { - "id": str(root_access.id), - "item": { - "id": str(root.id), - "path": str(root.path), - "depth": root.depth, - }, - "user": { - "id": str(user.id), - "email": user.email, - "full_name": user.full_name, - "short_name": user.short_name, - "language": user.language, - }, - "team": "", - "role": "editor", - "abilities": { - "destroy": False, - "update": False, - "partial_update": False, - "retrieve": True, - "set_role_to": [], - }, - "max_ancestors_role": None, - "max_ancestors_role_item_id": None, - "max_role": "editor", - "is_explicit": False, - }, { "id": str(other_admin_access.id), "item": { @@ -1657,9 +1613,7 @@ def test_api_item_accesses_explicit(): owner_access = factories.UserItemAccessFactory( item=root, user=other_owner, role="owner" ) - parent_access = factories.UserItemAccessFactory( - item=parent, user=user, role="administrator" - ) + factories.UserItemAccessFactory(item=parent, user=user, role="administrator") response = client.get(f"/api/v1.0/items/{item.id!s}/accesses/") assert response.status_code == 200 @@ -1749,34 +1703,6 @@ def test_api_item_accesses_explicit(): "max_role": "owner", "is_explicit": False, }, - { - "id": str(parent_access.id), - "item": { - "id": str(parent.id), - "path": str(parent.path), - "depth": parent.depth, - }, - "user": { - "id": str(user.id), - "email": user.email, - "full_name": user.full_name, - "short_name": user.short_name, - "language": user.language, - }, - "team": "", - "role": "administrator", - "abilities": { - "destroy": True, - "update": False, - "partial_update": False, - "retrieve": True, - "set_role_to": [], - }, - "max_ancestors_role": "editor", - "max_ancestors_role_item_id": str(root.id), - "max_role": "administrator", - "is_explicit": False, - }, { "id": str(item_access.id), "item": {