diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 2b881ade..31702b05 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -1454,24 +1454,23 @@ class DocumentAccessViewSet( accesses = list(queryset.order_by("document__path")) # Annotate more information on roles - path_to_key_to_max_ancestors_role = defaultdict( - lambda: defaultdict(lambda: None) - ) + # - accesses of the user (direct or via a team) path_to_ancestors_roles = defaultdict(list) path_to_role = defaultdict(lambda: None) + # - accesses of other users and teams + key_to_path_to_max_ancestors_role = defaultdict( + lambda: defaultdict(lambda: None) + ) for access in accesses: key = access.target_key path = access.document.path parent_path = path[: -models.Document.steplen] - path_to_key_to_max_ancestors_role[path][key] = choices.RoleChoices.max( - path_to_key_to_max_ancestors_role[path][key], access.role - ) - if parent_path: - path_to_key_to_max_ancestors_role[path][key] = choices.RoleChoices.max( - path_to_key_to_max_ancestors_role[parent_path][key], - path_to_key_to_max_ancestors_role[path][key], + key_to_path_to_max_ancestors_role[key][parent_path] = ( + choices.RoleChoices.max( + *key_to_path_to_max_ancestors_role[key].values() + ) ) path_to_ancestors_roles[path].extend( path_to_ancestors_roles[parent_path] @@ -1480,6 +1479,10 @@ class DocumentAccessViewSet( else: path_to_ancestors_roles[path] = [] + key_to_path_to_max_ancestors_role[key][path] = choices.RoleChoices.max( + key_to_path_to_max_ancestors_role[key][parent_path], access.role + ) + if access.user_id == user.id or access.team in user.teams: path_to_role[path] = choices.RoleChoices.max( path_to_role[path], access.role @@ -1493,7 +1496,7 @@ class DocumentAccessViewSet( path = access.document.path parent_path = path[: -models.Document.steplen] access.max_ancestors_role = ( - path_to_key_to_max_ancestors_role[parent_path][access.target_key] + key_to_path_to_max_ancestors_role[access.target_key][parent_path] if parent_path else None ) diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 4d72c77b..414bddbe 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -1150,6 +1150,12 @@ class DocumentAccess(BaseAccess): for candidate_role in set_role_to if RoleChoices.get_priority(candidate_role) > ancestors_role_priority ] + child_set_role_to = [ + candidate_role + for candidate_role in set_role_to + if RoleChoices.get_priority(candidate_role) + > RoleChoices.get_priority(self.role) + ] return { "destroy": can_delete, @@ -1157,6 +1163,7 @@ class DocumentAccess(BaseAccess): "partial_update": bool(set_role_to) and is_owner_or_admin, "retrieve": (self.user and self.user.id == user.id) or is_owner_or_admin, "set_role_to": set_role_to, + "child_set_role_to": child_set_role_to, } diff --git a/src/backend/core/tests/documents/test_api_document_accesses.py b/src/backend/core/tests/documents/test_api_document_accesses.py index 92c4798e..f384fb63 100644 --- a/src/backend/core/tests/documents/test_api_document_accesses.py +++ b/src/backend/core/tests/documents/test_api_document_accesses.py @@ -159,6 +159,7 @@ def test_api_document_accesses_list_authenticated_related_non_privileged( "partial_update": False, "retrieve": False, "set_role_to": [], + "child_set_role_to": [], "update": False, }, } @@ -326,196 +327,284 @@ def test_api_document_accesses_retrieve_set_role_to_child(): @pytest.mark.parametrize( - "roles,results", + "self_role, other_grandparent, other_parent, other_document", [ - [ - ["administrator", "reader", "reader", "reader"], - [ - ["reader", "editor", "administrator"], - [], - [], - ["editor", "administrator"], - ], - ], - [ - ["owner", "reader", "reader", "reader"], - [ - ["reader", "editor", "administrator", "owner"], - [], - [], - ["editor", "administrator", "owner"], - ], - ], - [ - ["owner", "reader", "reader", "owner"], - [ - ["reader", "editor", "administrator", "owner"], - [], - [], - ["editor", "administrator", "owner"], - ], - ], + # Test case 1 + ( + { + "role": "administrator", + "set_role_to": ["reader", "editor", "administrator"], + "child_set_role_to": [], + }, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + { + "role": "reader", + "set_role_to": ["editor", "administrator"], + "child_set_role_to": ["editor", "administrator"], + }, + ), + # Test case 2 + ( + { + "role": "owner", + "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": [], + }, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + { + "role": "reader", + "set_role_to": ["editor", "administrator", "owner"], + "child_set_role_to": ["editor", "administrator", "owner"], + }, + ), + # Test case 3 + ( + { + "role": "owner", + "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": [], + }, + {"role": "administrator", "set_role_to": [], "child_set_role_to": []}, + {"role": None}, + { + "role": "reader", # May happen after a move + "set_role_to": ["owner"], + "child_set_role_to": ["owner"], + }, + ), + # Test case 4 + ( + { + "role": "owner", + "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": [], + }, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + { + "role": "owner", + "set_role_to": ["editor", "administrator", "owner"], + "child_set_role_to": [], + }, + ), ], ) -def test_api_document_accesses_list_authenticated_related_same_user(roles, results): +def test_api_document_accesses_list_authenticated_related_same_user( + self_role, other_grandparent, other_parent, other_document +): """ - The maximum role across ancestor documents and set_role_to options for - a given user should be filled as expected. + Test correct 'set_role_to' and 'child_set_role_to' values for each access. """ user = factories.UserFactory() + other_user = factories.UserFactory() client = APIClient() client.force_login(user) - # Create documents structured as a tree + # Create document hierarchy grand_parent = factories.DocumentFactory(link_reach="authenticated") parent = factories.DocumentFactory(parent=grand_parent) document = factories.DocumentFactory(parent=parent) - # Create accesses for another user - other_user = factories.UserFactory() - accesses = [ - factories.UserDocumentAccessFactory( - document=document, user=user, role=roles[0] - ), - factories.UserDocumentAccessFactory( - document=grand_parent, user=other_user, role=roles[1] - ), - factories.UserDocumentAccessFactory( - document=parent, user=other_user, role=roles[2] - ), - factories.UserDocumentAccessFactory( - document=document, user=other_user, role=roles[3] - ), + # Accesses: (document, user, spec) + access_specs = [ + (document, user, self_role), + (grand_parent, other_user, other_grandparent), + (parent, other_user, other_parent), + (document, other_user, other_document), ] - response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/") + accesses = [] + for doc, usr, spec in access_specs: + if spec["role"] is not None: + access = factories.UserDocumentAccessFactory( + document=doc, user=usr, role=spec["role"] + ) + accesses.append((access, spec)) + response = client.get(f"/api/v1.0/documents/{document.id}/accesses/") assert response.status_code == 200 - content = response.json() - assert len(content) == 4 - for result in content: - assert ( - result["max_ancestors_role"] is None - if result["user"]["id"] == str(user.id) - else choices.RoleChoices.max(roles[1], roles[2]) - ) + content_by_id = {entry["id"]: entry for entry in response.json()} - result_dict = { - result["id"]: result["abilities"]["set_role_to"] for result in content - } - assert [result_dict[str(access.id)] for access in accesses] == results + for access, expected in accesses: + abilities = content_by_id[str(access.id)]["abilities"] + assert abilities["set_role_to"] == expected["set_role_to"] + assert abilities["child_set_role_to"] == expected["child_set_role_to"] @pytest.mark.parametrize( - "roles,results", + "self_role, other_grandparent, other_parent, other_document", [ - [ - ["administrator", "reader", "reader", "reader"], - [ - ["reader", "editor", "administrator"], - [], - [], - ["editor", "administrator"], - ], - ], - [ - ["owner", "reader", "reader", "reader"], - [ - ["reader", "editor", "administrator", "owner"], - [], - [], - ["editor", "administrator", "owner"], - ], - ], - [ - ["owner", "reader", "reader", "owner"], - [ - ["reader", "editor", "administrator", "owner"], - [], - [], - ["editor", "administrator", "owner"], - ], - ], - [ - ["reader", "reader", "reader", "owner"], - [ - ["reader", "editor", "administrator", "owner"], - [], - [], - ["editor", "administrator", "owner"], - ], - ], - [ - ["reader", "administrator", "reader", "editor"], - [ - ["reader", "editor", "administrator"], - ["reader", "editor", "administrator"], - [], - [], - ], - ], - [ - ["editor", "editor", "administrator", "editor"], - [ - ["reader", "editor", "administrator"], - [], - ["administrator"], - [], - ], - ], + # Test case 1 + ( + { + "role": "administrator", + "set_role_to": ["reader", "editor", "administrator"], + "child_set_role_to": [], + }, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + { + "role": "reader", + "set_role_to": ["editor", "administrator"], + "child_set_role_to": ["editor", "administrator"], + }, + ), + # Test case 2 + ( + { + "role": "owner", + "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": [], + }, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + { + "role": "reader", + "set_role_to": ["editor", "administrator", "owner"], + "child_set_role_to": ["editor", "administrator", "owner"], + }, + ), + # Test case 3 + ( + { + "role": "owner", + "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": [], + }, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + { + "role": "owner", + "set_role_to": ["editor", "administrator", "owner"], + "child_set_role_to": [], + }, + ), + # Test case 4 + ( + { + "role": "reader", + "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": ["editor", "administrator", "owner"], + }, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + { + "role": "owner", + "set_role_to": ["editor", "administrator", "owner"], + "child_set_role_to": [], + }, + ), + # Test case 5 + ( + { + "role": "reader", + "set_role_to": ["reader", "editor", "administrator"], + "child_set_role_to": ["editor", "administrator"], + }, + { + "role": "administrator", + "set_role_to": ["reader", "editor", "administrator"], + "child_set_role_to": [], + }, + {"role": "reader", "set_role_to": [], "child_set_role_to": []}, + {"role": "editor", "set_role_to": [], "child_set_role_to": []}, + ), + # Test case 6 + ( + { + "role": "owner", + "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": [], + }, + { + "role": "administrator", + "set_role_to": ["reader", "editor", "administrator"], + "child_set_role_to": [], + }, + {"role": None}, + { + "role": "reader", # May happen after a move + "set_role_to": ["owner"], + "child_set_role_to": ["owner"], + }, + ), + # Test case 7 + ( + { + "role": "editor", + "set_role_to": ["reader", "editor", "administrator"], + "child_set_role_to": ["administrator"], + }, + {"role": "editor", "set_role_to": [], "child_set_role_to": []}, + { + "role": "administrator", + "set_role_to": ["administrator"], + "child_set_role_to": [], + }, + {"role": "editor", "set_role_to": [], "child_set_role_to": []}, + ), ], ) def test_api_document_accesses_list_authenticated_related_same_team( - roles, results, mock_user_teams + self_role, other_grandparent, other_parent, other_document, mock_user_teams ): """ - The maximum role across ancestor documents and set_role_to optionsfor - a given team should be filled as expected. + Test correct 'set_role_to' values from team-based access and max_ancestors_role logic. """ user = factories.UserFactory() client = APIClient() client.force_login(user) - # Create documents structured as a tree + # Create document hierarchy grand_parent = factories.DocumentFactory(link_reach="authenticated") parent = factories.DocumentFactory(parent=grand_parent) document = factories.DocumentFactory(parent=parent) mock_user_teams.return_value = ["lasuite", "unknown"] + accesses = [ - factories.UserDocumentAccessFactory( - document=document, user=user, role=roles[0] - ), - # Create accesses for a team - factories.TeamDocumentAccessFactory( - document=grand_parent, team="lasuite", role=roles[1] - ), - factories.TeamDocumentAccessFactory( - document=parent, team="lasuite", role=roles[2] - ), - factories.TeamDocumentAccessFactory( - document=document, team="lasuite", role=roles[3] - ), + ( + factories.UserDocumentAccessFactory( + document=document, + user=user, + role=self_role["role"], + ), + self_role, + ) ] - response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/") + # --- Team-based accesses --- + team_access_specs = [ + (grand_parent, other_grandparent), + (parent, other_parent), + (document, other_document), + ] + for doc, spec in team_access_specs: + if spec["role"] is not None: + accesses.append( + ( + factories.TeamDocumentAccessFactory( + document=doc, + team="lasuite", + role=spec["role"], + ), + spec, + ) + ) + + response = client.get(f"/api/v1.0/documents/{document.id}/accesses/") assert response.status_code == 200 - content = response.json() - assert len(content) == 4 - for result in content: - assert ( - result["max_ancestors_role"] is None - if result["user"] and result["user"]["id"] == str(user.id) - else choices.RoleChoices.max(roles[1], roles[2]) - ) + content_by_id = {entry["id"]: entry for entry in response.json()} - result_dict = { - result["id"]: result["abilities"]["set_role_to"] for result in content - } - assert [result_dict[str(access.id)] for access in accesses] == results + for access, expected in accesses: + abilities = content_by_id[str(access.id)]["abilities"] + assert abilities["set_role_to"] == expected["set_role_to"] + assert abilities["child_set_role_to"] == expected["child_set_role_to"] def test_api_document_accesses_retrieve_anonymous(): diff --git a/src/backend/core/tests/test_models_document_accesses.py b/src/backend/core/tests/test_models_document_accesses.py index 2fa88cf1..0181df0e 100644 --- a/src/backend/core/tests/test_models_document_accesses.py +++ b/src/backend/core/tests/test_models_document_accesses.py @@ -90,6 +90,7 @@ def test_models_document_access_get_abilities_anonymous(): "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -104,6 +105,7 @@ def test_models_document_access_get_abilities_authenticated(): "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -124,6 +126,7 @@ def test_models_document_access_get_abilities_for_owner_of_self_allowed(): "update": True, "partial_update": True, "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": [], } @@ -145,6 +148,7 @@ def test_models_document_access_get_abilities_for_owner_of_self_last_on_root( "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -167,6 +171,7 @@ def test_models_document_access_get_abilities_for_owner_of_self_last_on_child( "update": True, "partial_update": True, "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": [], } @@ -184,6 +189,7 @@ def test_models_document_access_get_abilities_for_owner_of_owner(): "update": True, "partial_update": True, "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": [], } @@ -201,6 +207,7 @@ def test_models_document_access_get_abilities_for_owner_of_administrator(): "update": True, "partial_update": True, "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": ["owner"], } @@ -218,6 +225,7 @@ def test_models_document_access_get_abilities_for_owner_of_editor(): "update": True, "partial_update": True, "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": ["administrator", "owner"], } @@ -235,6 +243,7 @@ def test_models_document_access_get_abilities_for_owner_of_reader(): "update": True, "partial_update": True, "set_role_to": ["reader", "editor", "administrator", "owner"], + "child_set_role_to": ["editor", "administrator", "owner"], } @@ -255,6 +264,7 @@ def test_models_document_access_get_abilities_for_administrator_of_owner(): "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -272,6 +282,7 @@ def test_models_document_access_get_abilities_for_administrator_of_administrator "update": True, "partial_update": True, "set_role_to": ["reader", "editor", "administrator"], + "child_set_role_to": [], } @@ -289,6 +300,7 @@ def test_models_document_access_get_abilities_for_administrator_of_editor(): "update": True, "partial_update": True, "set_role_to": ["reader", "editor", "administrator"], + "child_set_role_to": ["administrator"], } @@ -306,6 +318,7 @@ def test_models_document_access_get_abilities_for_administrator_of_reader(): "update": True, "partial_update": True, "set_role_to": ["reader", "editor", "administrator"], + "child_set_role_to": ["editor", "administrator"], } @@ -326,6 +339,7 @@ def test_models_document_access_get_abilities_for_editor_of_owner(): "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -343,6 +357,7 @@ def test_models_document_access_get_abilities_for_editor_of_administrator(): "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -365,6 +380,7 @@ def test_models_document_access_get_abilities_for_editor_of_editor_user( "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -385,6 +401,7 @@ def test_models_document_access_get_abilities_for_reader_of_owner(): "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -402,6 +419,7 @@ def test_models_document_access_get_abilities_for_reader_of_administrator(): "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -424,6 +442,7 @@ def test_models_document_access_get_abilities_for_reader_of_reader_user( "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], } @@ -444,6 +463,7 @@ def test_models_document_access_get_abilities_preset_role(django_assert_num_quer "update": False, "partial_update": False, "set_role_to": [], + "child_set_role_to": [], }