From 0003f9d0de588b21a6751b5e8a26d9b62b4dbb33 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Thu, 2 Jan 2025 14:02:03 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20add=20user=20roles=20as=20?= =?UTF-8?q?field=20in=20the=20document=20API=20representation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit user roles were already computed as an annotation on the query for performance as we must look at all the document's ancestors to determine the roles that apply recursively. We can easily expose them as readonly via the serializer. --- src/backend/core/api/serializers.py | 15 +++ .../test_api_documents_children_list.py | 20 +++- .../documents/test_api_documents_list.py | 6 +- .../documents/test_api_documents_retrieve.py | 97 +++++++++++++++---- 4 files changed, 114 insertions(+), 24 deletions(-) diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 28cd4efa..556f209e 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -147,6 +147,7 @@ class ListDocumentSerializer(BaseResourceSerializer): is_favorite = serializers.BooleanField(read_only=True) nb_accesses = serializers.IntegerField(read_only=True) + user_roles = serializers.SerializerMethodField(read_only=True) class Meta: model = models.Document @@ -165,6 +166,7 @@ class ListDocumentSerializer(BaseResourceSerializer): "path", "title", "updated_at", + "user_roles", ] read_only_fields = [ "id", @@ -180,8 +182,19 @@ class ListDocumentSerializer(BaseResourceSerializer): "numchild", "path", "updated_at", + "user_roles", ] + def get_user_roles(self, document): + """ + Return roles of the logged-in user for the current document, + taking into account ancestors. + """ + request = self.context.get("request") + if request: + return document.get_roles(request.user) + return [] + class DocumentSerializer(ListDocumentSerializer): """Serialize documents with all fields for display in detail views.""" @@ -206,6 +219,7 @@ class DocumentSerializer(ListDocumentSerializer): "path", "title", "updated_at", + "user_roles", ] read_only_fields = [ "id", @@ -220,6 +234,7 @@ class DocumentSerializer(ListDocumentSerializer): "numchild", "path", "updated_at", + "user_roles", ] def get_fields(self): diff --git a/src/backend/core/tests/documents/test_api_documents_children_list.py b/src/backend/core/tests/documents/test_api_documents_children_list.py index 1f25cf7b..81c3f3c1 100644 --- a/src/backend/core/tests/documents/test_api_documents_children_list.py +++ b/src/backend/core/tests/documents/test_api_documents_children_list.py @@ -43,6 +43,7 @@ def test_api_documents_children_list_anonymous_public_standalone(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], }, { "abilities": child2.get_abilities(AnonymousUser()), @@ -59,6 +60,7 @@ def test_api_documents_children_list_anonymous_public_standalone(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], }, ], } @@ -102,6 +104,7 @@ def test_api_documents_children_list_anonymous_public_parent(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], }, { "abilities": child2.get_abilities(AnonymousUser()), @@ -118,6 +121,7 @@ def test_api_documents_children_list_anonymous_public_parent(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], }, ], } @@ -179,6 +183,7 @@ def test_api_documents_children_list_authenticated_unrelated_public_or_authentic "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], }, { "abilities": child2.get_abilities(user), @@ -195,6 +200,7 @@ def test_api_documents_children_list_authenticated_unrelated_public_or_authentic "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], }, ], } @@ -242,6 +248,7 @@ def test_api_documents_children_list_authenticated_public_or_authenticated_paren "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], }, { "abilities": child2.get_abilities(user), @@ -258,6 +265,7 @@ def test_api_documents_children_list_authenticated_public_or_authenticated_paren "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], }, ], } @@ -327,6 +335,7 @@ def test_api_documents_children_list_authenticated_related_direct(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [access.role], }, { "abilities": child2.get_abilities(user), @@ -343,6 +352,7 @@ def test_api_documents_children_list_authenticated_related_direct(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [access.role], }, ], } @@ -365,8 +375,10 @@ def test_api_documents_children_list_authenticated_related_parent(): child1, child2 = factories.DocumentFactory.create_batch(2, parent=document) factories.UserDocumentAccessFactory(document=child1) + grand_parent_access = factories.UserDocumentAccessFactory( + document=grand_parent, user=user + ) factories.UserDocumentAccessFactory(document=grand_parent, user=user) - factories.UserDocumentAccessFactory(document=grand_parent) response = client.get( f"/api/v1.0/documents/{document.id!s}/children/", @@ -392,6 +404,7 @@ def test_api_documents_children_list_authenticated_related_parent(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [grand_parent_access.role], }, { "abilities": child2.get_abilities(user), @@ -408,6 +421,7 @@ def test_api_documents_children_list_authenticated_related_parent(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [grand_parent_access.role], }, ], } @@ -479,7 +493,7 @@ def test_api_documents_children_list_authenticated_related_team_members( document = factories.DocumentFactory(link_reach="restricted") child1, child2 = factories.DocumentFactory.create_batch(2, parent=document) - factories.TeamDocumentAccessFactory(document=document, team="myteam") + access = factories.TeamDocumentAccessFactory(document=document, team="myteam") response = client.get(f"/api/v1.0/documents/{document.id!s}/children/") @@ -505,6 +519,7 @@ def test_api_documents_children_list_authenticated_related_team_members( "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [access.role], }, { "abilities": child2.get_abilities(user), @@ -521,6 +536,7 @@ def test_api_documents_children_list_authenticated_related_team_members( "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [access.role], }, ], } diff --git a/src/backend/core/tests/documents/test_api_documents_list.py b/src/backend/core/tests/documents/test_api_documents_list.py index ca2de1b1..b9ab03c2 100644 --- a/src/backend/core/tests/documents/test_api_documents_list.py +++ b/src/backend/core/tests/documents/test_api_documents_list.py @@ -35,16 +35,16 @@ def test_api_documents_list_anonymous(reach, role): def test_api_documents_list_format(): """Validate the format of documents as returned by the list view.""" user = factories.UserFactory() - client = APIClient() client.force_login(user) other_users = factories.UserFactory.create_batch(3) document = factories.DocumentFactory( - users=[user, *factories.UserFactory.create_batch(2)], + users=factories.UserFactory.create_batch(2), favorited_by=[user, *other_users], link_traces=other_users, ) + access = factories.UserDocumentAccessFactory(document=document, user=user) response = client.get("/api/v1.0/documents/") @@ -72,6 +72,7 @@ def test_api_documents_list_format(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [access.role], } @@ -230,7 +231,6 @@ def test_api_documents_list_authenticated_link_reach_public_or_authenticated( assert response.status_code == 200 results = response.json()["results"] - assert len(results) == 3 results_id = {result["id"] for result in results} assert expected_ids == results_id diff --git a/src/backend/core/tests/documents/test_api_documents_retrieve.py b/src/backend/core/tests/documents/test_api_documents_retrieve.py index 239c9acb..7c530e4f 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -56,6 +56,7 @@ def test_api_documents_retrieve_anonymous_public_standalone(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], } @@ -109,6 +110,7 @@ def test_api_documents_retrieve_anonymous_public_parent(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], } @@ -195,6 +197,7 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], } assert ( models.LinkTrace.objects.filter(document=document, user=user).exists() is True @@ -255,6 +258,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [], } @@ -340,7 +344,7 @@ def test_api_documents_retrieve_authenticated_related_direct(): client.force_login(user) document = factories.DocumentFactory() - factories.UserDocumentAccessFactory(document=document, user=user) + access = factories.UserDocumentAccessFactory(document=document, user=user) factories.UserDocumentAccessFactory(document=document) response = client.get( @@ -363,6 +367,7 @@ def test_api_documents_retrieve_authenticated_related_direct(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [access.role], } @@ -423,6 +428,7 @@ def test_api_documents_retrieve_authenticated_related_parent(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": [access.role], } @@ -516,16 +522,16 @@ def test_api_documents_retrieve_authenticated_related_team_none(mock_user_teams) @pytest.mark.parametrize( - "teams", + "teams,roles", [ - ["readers"], - ["unknown", "readers"], - ["editors"], - ["unknown", "editors"], + [["readers"], ["reader"]], + [["unknown", "readers"], ["reader"]], + [["editors"], ["editor"]], + [["unknown", "editors"], ["editor"]], ], ) def test_api_documents_retrieve_authenticated_related_team_members( - teams, mock_user_teams + teams, roles, mock_user_teams ): """ Authenticated users should be allowed to retrieve a document to which they @@ -573,19 +579,20 @@ def test_api_documents_retrieve_authenticated_related_team_members( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": roles, } @pytest.mark.parametrize( - "teams", + "teams,roles", [ - ["administrators"], - ["editors", "administrators"], - ["unknown", "administrators"], + [["administrators"], ["administrator"]], + [["editors", "administrators"], ["administrator", "editor"]], + [["unknown", "administrators"], ["administrator"]], ], ) def test_api_documents_retrieve_authenticated_related_team_administrators( - teams, mock_user_teams + teams, roles, mock_user_teams ): """ Authenticated users should be allowed to retrieve a document to which they @@ -633,20 +640,21 @@ def test_api_documents_retrieve_authenticated_related_team_administrators( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": roles, } @pytest.mark.parametrize( - "teams", + "teams,roles", [ - ["owners"], - ["owners", "administrators"], - ["members", "administrators", "owners"], - ["unknown", "owners"], + [["owners"], ["owner"]], + [["owners", "administrators"], ["owner", "administrator"]], + [["members", "administrators", "owners"], ["owner", "administrator"]], + [["unknown", "owners"], ["owner"]], ], ) def test_api_documents_retrieve_authenticated_related_team_owners( - teams, mock_user_teams + teams, roles, mock_user_teams ): """ Authenticated users should be allowed to retrieve a restricted document to which @@ -655,7 +663,6 @@ def test_api_documents_retrieve_authenticated_related_team_owners( mock_user_teams.return_value = teams user = factories.UserFactory() - client = APIClient() client.force_login(user) @@ -694,4 +701,56 @@ def test_api_documents_retrieve_authenticated_related_team_owners( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), + "user_roles": roles, } + + +def test_api_documents_retrieve_user_roles(django_assert_num_queries): + """ + Roles should be annotated on querysets taking into account all documents ancestors. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + grand_parent = factories.DocumentFactory( + users=factories.UserFactory.create_batch(2) + ) + parent = factories.DocumentFactory( + parent=grand_parent, users=factories.UserFactory.create_batch(2) + ) + document = factories.DocumentFactory( + parent=parent, users=factories.UserFactory.create_batch(2) + ) + + accesses = ( + factories.UserDocumentAccessFactory(document=grand_parent, user=user), + factories.UserDocumentAccessFactory(document=parent, user=user), + factories.UserDocumentAccessFactory(document=document, user=user), + ) + expected_roles = {access.role for access in accesses} + + with django_assert_num_queries(8): + response = client.get(f"/api/v1.0/documents/{document.id!s}/") + + assert response.status_code == 200 + + user_roles = response.json()["user_roles"] + assert set(user_roles) == expected_roles + + +def test_api_documents_retrieve_numqueries_with_link_trace(django_assert_num_queries): + """If the link traced already exists, the number of queries should be minimal.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory(users=[user], link_traces=[user]) + + with django_assert_num_queries(2): + response = client.get(f"/api/v1.0/documents/{document.id!s}/") + + assert response.status_code == 200 + + assert response.json()["id"] == str(document.id) +