From 06b8a09ec05caa05ccff0df10f80dee115ee400a Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Mon, 14 Apr 2025 17:23:52 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F(back)=20don't=20allow=20a?= =?UTF-8?q?n=20owner=20to=20change=20or=20delete=20other=20owner=20accesse?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Owner accesses can not be modified or deleted from other owners. Only current owner can modify and delete its own access. --- CHANGELOG.md | 4 + src/backend/core/models.py | 8 +- .../documents/test_api_document_accesses.py | 163 ++++++++++++++++-- .../tests/test_models_document_accesses.py | 17 ++ 4 files changed, 176 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4f1a6431..e016505d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to - 🚩 add homepage feature flag #861 +## Fixed + +- 🔒️(back) don't allow an owner to change or delete other owner accesses + ## [3.1.0] - 2025-04-07 diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 2c5239ead..d7d56baff 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -1113,11 +1113,17 @@ def get_abilities(self, user): """ roles = self._get_roles(self.document, user) is_owner_or_admin = bool(set(roles).intersection(set(PRIVILEGED_ROLES))) + if self.role == RoleChoices.OWNER: + # An owner can only delete its own access if other owners exist can_delete = ( - RoleChoices.OWNER in roles + self.user + and self.user.id == user.id and self.document.accesses.filter(role=RoleChoices.OWNER).count() > 1 ) + + # An owner can only update its own access to a non-owner role + # and only if other owners exist set_role_to = ( [RoleChoices.ADMIN, RoleChoices.EDITOR, RoleChoices.READER] if can_delete 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 bf5ef1827..00c89ae78 100644 --- a/src/backend/core/tests/documents/test_api_document_accesses.py +++ b/src/backend/core/tests/documents/test_api_document_accesses.py @@ -1,6 +1,7 @@ """ Test document accesses API endpoints for users in impress's core app. """ +# pylint: disable=too-many-lines import random from uuid import uuid4 @@ -624,14 +625,18 @@ def test_api_document_accesses_update_administrator_to_owner( @pytest.mark.parametrize("via", VIA) +@pytest.mark.parametrize( + "role", [role for role in models.RoleChoices if role != models.RoleChoices.OWNER] +) def test_api_document_accesses_update_owner( via, + role, mock_user_teams, mock_reset_connections, # pylint: disable=redefined-outer-name ): """ A user who is an owner in a document should be allowed to update - a user access for this document whatever the role. + a user access for this document for all roles except owner. """ user = factories.UserFactory(with_owned_document=True) @@ -648,9 +653,7 @@ def test_api_document_accesses_update_owner( ) factories.UserFactory() - access = factories.UserDocumentAccessFactory( - document=document, - ) + access = factories.UserDocumentAccessFactory(document=document, role=role) old_values = serializers.DocumentAccessSerializer(instance=access).data new_values = { @@ -729,11 +732,30 @@ def test_api_document_accesses_update_owner_self( access.refresh_from_db() assert access.role == "owner" - # Add another owner and it should now work + # Add another owner and it should now work only for user, not for team factories.UserDocumentAccessFactory(document=document, role="owner") - user_id = str(access.user_id) if via == USER else None - with mock_reset_connections(document.id, user_id): + if via == USER: + factories.UserDocumentAccessFactory(document=document, role="owner") + + user_id = str(access.user_id) if via == USER else None + with mock_reset_connections(document.id, user_id): + response = client.put( + f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/", + data={ + **old_values, + "role": new_role, + "user_id": old_values.get("user", {}).get("id") + if old_values.get("user") is not None + else None, + }, + format="json", + ) + + assert response.status_code == 200 + access.refresh_from_db() + assert access.role == new_role + else: response = client.put( f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/", data={ @@ -745,13 +767,41 @@ def test_api_document_accesses_update_owner_self( }, format="json", ) + assert response.status_code == 403 - assert response.status_code == 200 - access.refresh_from_db() - assert access.role == new_role +@pytest.mark.parametrize("via", VIA) +def test_api_document_accesses_update_owner_from_other_owner(via, mock_user_teams): + """ + A user who is an owner in a document should not be allowed to update + access for another user who is also an owner in the document. + """ + user = factories.UserFactory(with_owned_document=True) + + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory() + if via == USER: + factories.UserDocumentAccessFactory(document=document, user=user, role="owner") + elif via == TEAM: + mock_user_teams.return_value = ["lasuite", "unknown"] + factories.TeamDocumentAccessFactory( + document=document, team="lasuite", role="owner" + ) + other_owner = factories.UserFactory() + access = factories.UserDocumentAccessFactory( + document=document, user=other_owner, role="owner" + ) + + old_values = serializers.DocumentAccessSerializer(instance=access).data + response = client.put( + f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/", + data={**old_values, "role": models.RoleChoices.ADMIN}, + format="json", + ) -# Delete + assert response.status_code == 403 def test_api_document_accesses_delete_anonymous(): @@ -898,14 +948,18 @@ def test_api_document_accesses_delete_administrator_on_owners(via, mock_user_tea @pytest.mark.parametrize("via", VIA) +@pytest.mark.parametrize( + "role", [role for role in models.RoleChoices if role != models.RoleChoices.OWNER] +) def test_api_document_accesses_delete_owners( via, + role, mock_user_teams, mock_reset_connections, # pylint: disable=redefined-outer-name ): """ Users should be able to delete the document access of another user - for a document of which they are owner. + for a document of which they are owner but can not delete other owners. """ user = factories.UserFactory() @@ -921,7 +975,7 @@ def test_api_document_accesses_delete_owners( document=document, team="lasuite", role="owner" ) - access = factories.UserDocumentAccessFactory(document=document) + access = factories.UserDocumentAccessFactory(document=document, role=role) assert models.DocumentAccess.objects.count() == 2 assert models.DocumentAccess.objects.filter(user=access.user).exists() @@ -935,6 +989,79 @@ def test_api_document_accesses_delete_owners( assert models.DocumentAccess.objects.count() == 1 +@pytest.mark.parametrize("via", VIA) +def test_api_document_accesses_delete_other_owners(via, mock_user_teams): + """ + A user should not be able to delete the document access of another owner. + """ + user = factories.UserFactory() + + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory() + if via == USER: + factories.UserDocumentAccessFactory(document=document, user=user, role="owner") + elif via == TEAM: + mock_user_teams.return_value = ["lasuite", "unknown"] + factories.TeamDocumentAccessFactory( + document=document, team="lasuite", role="owner" + ) + + access = factories.UserDocumentAccessFactory(document=document, role="owner") + + assert models.DocumentAccess.objects.count() == 2 + assert models.DocumentAccess.objects.filter(user=access.user).exists() + + response = client.delete( + f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/", + ) + + assert response.status_code == 403 + assert models.DocumentAccess.objects.count() == 2 + + +@pytest.mark.parametrize("via", VIA) +def test_api_document_accesses_delete_self_owner_if_other_owner_exists( + via, + mock_reset_connections, # pylint: disable=redefined-outer-name +): + """ + A user should be able to delete their own ownership access if there is another owner in the + document. + """ + user = factories.UserFactory(with_owned_document=True) + + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory() + access = None + if via == USER: + access = factories.UserDocumentAccessFactory( + document=document, user=user, role="owner" + ) + elif via == TEAM: + pytest.skip("Implement when team ownership is implemented") + + factories.UserDocumentAccessFactory(document=document, role="owner") + + assert ( + models.DocumentAccess.objects.filter(document=document, role="owner").count() + == 2 + ) + with mock_reset_connections(document.id, str(access.user_id)): + response = client.delete( + f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/", + ) + + assert response.status_code == 204 + assert ( + models.DocumentAccess.objects.filter(document=document, role="owner").count() + == 1 + ) + + @pytest.mark.parametrize("via", VIA) def test_api_document_accesses_delete_owners_last_owner(via, mock_user_teams): """ @@ -957,10 +1084,16 @@ def test_api_document_accesses_delete_owners_last_owner(via, mock_user_teams): document=document, team="lasuite", role="owner" ) - assert models.DocumentAccess.objects.count() == 2 + assert ( + models.DocumentAccess.objects.filter(document=document, role="owner").count() + == 1 + ) response = client.delete( f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/", ) assert response.status_code == 403 - assert models.DocumentAccess.objects.count() == 2 + assert ( + models.DocumentAccess.objects.filter(document=document, role="owner").count() + == 1 + ) diff --git a/src/backend/core/tests/test_models_document_accesses.py b/src/backend/core/tests/test_models_document_accesses.py index fe0e7c1c7..c20b03d4c 100644 --- a/src/backend/core/tests/test_models_document_accesses.py +++ b/src/backend/core/tests/test_models_document_accesses.py @@ -150,6 +150,23 @@ def test_models_document_access_get_abilities_for_owner_of_owner(): document=access.document, role="owner" ).user abilities = access.get_abilities(user) + assert abilities == { + "destroy": False, + "retrieve": True, + "update": False, + "partial_update": False, + "set_role_to": [], + } + + +def test_models_document_access_get_abilities_for_owner_of_self_with_other_owner(): + """ + Check abilities of self access for the owner of a document when there is at least one other + owner left. + """ + access = factories.UserDocumentAccessFactory(role="owner") + factories.UserDocumentAccessFactory(document=access.document, role="owner") + abilities = access.get_abilities(access.user) assert abilities == { "destroy": True, "retrieve": True,