diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e28fcbe..0167f633 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to ### Changed +- ♻️(backend) stop requiring owner for non-root documents #846 - ♻️(backend) simplify roles by ranking them and return only the max role #846 - 📌(yjs) stop pinning node to minor version on yjs docker image #1005 - 🧑‍💻(docker) add .next to .dockerignore #1055 diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 4675a0fd..f5422da1 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -234,43 +234,6 @@ class ResourceAccessViewsetMixin: context["resource_id"] = self.kwargs["resource_id"] return context - def destroy(self, request, *args, **kwargs): - """Forbid deleting the last owner access""" - instance = self.get_object() - resource = getattr(instance, self.resource_field_name) - - # Check if the access being deleted is the last owner access for the resource - if ( - instance.role == "owner" - and resource.accesses.filter(role="owner").count() == 1 - ): - return drf.response.Response( - {"detail": "Cannot delete the last owner access for the resource."}, - status=drf.status.HTTP_403_FORBIDDEN, - ) - - return super().destroy(request, *args, **kwargs) - - def perform_update(self, serializer): - """Check that we don't change the role if it leads to losing the last owner.""" - instance = serializer.instance - - # Check if the role is being updated and the new role is not "owner" - if ( - "role" in self.request.data - and self.request.data["role"] != models.RoleChoices.OWNER - ): - resource = getattr(instance, self.resource_field_name) - # Check if the access being updated is the last owner access for the resource - if ( - instance.role == models.RoleChoices.OWNER - and resource.accesses.filter(role=models.RoleChoices.OWNER).count() == 1 - ): - message = "Cannot change the role to a non-owner role for the last owner access." - raise drf.exceptions.PermissionDenied({"detail": message}) - - serializer.save() - class DocumentMetadata(drf.metadata.SimpleMetadata): """Custom metadata class to add information""" @@ -726,7 +689,7 @@ class DocumentViewSet( position = validated_data["position"] message = None - + owner_accesses = [] if position in [ enums.MoveNodePositionChoices.FIRST_CHILD, enums.MoveNodePositionChoices.LAST_CHILD, @@ -736,12 +699,15 @@ class DocumentViewSet( "You do not have permission to move documents " "as a child to this target document." ) - elif not target_document.is_root(): - if not target_document.get_parent().get_abilities(user).get("move"): - message = ( - "You do not have permission to move documents " - "as a sibling of this target document." - ) + elif target_document.is_root(): + owner_accesses = document.get_root().accesses.filter( + role=models.RoleChoices.OWNER + ) + elif not target_document.get_parent().get_abilities(user).get("move"): + message = ( + "You do not have permission to move documents " + "as a sibling of this target document." + ) if message: return drf.response.Response( @@ -751,6 +717,19 @@ class DocumentViewSet( document.move(target_document, pos=position) + # Make sure we have at least one owner + if ( + owner_accesses + and not document.accesses.filter(role=models.RoleChoices.OWNER).exists() + ): + for owner_access in owner_accesses: + models.DocumentAccess.objects.update_or_create( + document=document, + user=owner_access.user, + team=owner_access.team, + defaults={"role": models.RoleChoices.OWNER}, + ) + return drf.response.Response( {"message": "Document moved successfully."}, status=status.HTTP_200_OK ) @@ -797,11 +776,7 @@ class DocumentViewSet( creator=request.user, **serializer.validated_data, ) - models.DocumentAccess.objects.create( - document=child_document, - user=request.user, - role=models.RoleChoices.OWNER, - ) + # Set the created instance to the serializer serializer.instance = child_document diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 74ef4520..8c4cd35b 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -1121,9 +1121,12 @@ class DocumentAccess(BaseAccess): is_owner_or_admin = role in PRIVILEGED_ROLES if self.role == RoleChoices.OWNER: - can_delete = ( - role == RoleChoices.OWNER - and DocumentAccess.objects.filter( + can_delete = role == RoleChoices.OWNER and ( + # check if document is not root trying to avoid an extra query + # "document_path" is annotated by the viewset's list method + len(getattr(self, "document_path", "")) > Document.steplen + or not self.document.is_root() + or DocumentAccess.objects.filter( document_id=self.document_id, role=RoleChoices.OWNER ).count() > 1 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 593d2a7b..beae157f 100644 --- a/src/backend/core/tests/documents/test_api_document_accesses.py +++ b/src/backend/core/tests/documents/test_api_document_accesses.py @@ -224,12 +224,7 @@ def test_api_document_accesses_list_authenticated_related_privileged( other_access = factories.UserDocumentAccessFactory(user=user) factories.UserDocumentAccessFactory(document=other_access.document) - nb_queries = 3 - if role == "owner": - # Queries that secure the owner status - nb_queries += sum(acc.role == "owner" for acc in document_accesses) - - with django_assert_num_queries(nb_queries): + with django_assert_num_queries(3): response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/") assert response.status_code == 200 @@ -335,7 +330,12 @@ def test_api_document_accesses_retrieve_set_role_to_child(): ], [ ["owner", "reader", "reader", "reader"], - [[], [], [], ["reader", "editor", "administrator", "owner"]], + [ + ["reader", "editor", "administrator", "owner"], + [], + [], + ["reader", "editor", "administrator", "owner"], + ], ], [ ["owner", "reader", "reader", "owner"], @@ -412,7 +412,12 @@ def test_api_document_accesses_list_authenticated_related_same_user(roles, resul ], [ ["owner", "reader", "reader", "reader"], - [[], [], [], ["reader", "editor", "administrator", "owner"]], + [ + ["reader", "editor", "administrator", "owner"], + [], + [], + ["reader", "editor", "administrator", "owner"], + ], ], [ ["owner", "reader", "reader", "owner"], @@ -425,7 +430,12 @@ def test_api_document_accesses_list_authenticated_related_same_user(roles, resul ], [ ["reader", "reader", "reader", "owner"], - [["reader", "editor", "administrator", "owner"], [], [], []], + [ + ["reader", "editor", "administrator", "owner"], + [], + [], + ["reader", "editor", "administrator", "owner"], + ], ], [ ["reader", "administrator", "reader", "editor"], @@ -929,7 +939,7 @@ def test_api_document_accesses_update_owner( @pytest.mark.parametrize("via", VIA) -def test_api_document_accesses_update_owner_self( +def test_api_document_accesses_update_owner_self_root( via, mock_user_teams, mock_reset_connections, # pylint: disable=redefined-outer-name @@ -990,6 +1000,51 @@ def test_api_document_accesses_update_owner_self( assert access.role == new_role +@pytest.mark.parametrize("via", VIA) +def test_api_document_accesses_update_owner_self_child( + via, + mock_user_teams, + mock_reset_connections, # pylint: disable=redefined-outer-name +): + """ + A user who is owner of a document should be allowed to update + their own user access even if they are the only owner in the document, + provided the document is not a root. + """ + user = factories.UserFactory(with_owned_document=True) + + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory() + document = factories.DocumentFactory(parent=parent) + access = None + if via == USER: + access = factories.UserDocumentAccessFactory( + document=document, user=user, role="owner" + ) + elif via == TEAM: + mock_user_teams.return_value = ["lasuite", "unknown"] + access = factories.TeamDocumentAccessFactory( + document=document, team="lasuite", role="owner" + ) + + old_values = serializers.DocumentAccessSerializer(instance=access).data + new_role = random.choice(["administrator", "editor", "reader"]) + + 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}, + format="json", + ) + + assert response.status_code == 200 + access.refresh_from_db() + assert access.role == new_role + + # Delete @@ -1170,17 +1225,16 @@ def test_api_document_accesses_delete_owners( f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/", ) - assert response.status_code == 204 - assert models.DocumentAccess.objects.count() == 1 + assert response.status_code == 204 + assert models.DocumentAccess.objects.count() == 1 @pytest.mark.parametrize("via", VIA) -def test_api_document_accesses_delete_owners_last_owner(via, mock_user_teams): +def test_api_document_accesses_delete_owners_last_owner_root(via, mock_user_teams): """ - It should not be possible to delete the last owner access from a document + It should not be possible to delete the last owner access from a root document """ user = factories.UserFactory(with_owned_document=True) - client = APIClient() client.force_login(user) @@ -1203,3 +1257,63 @@ def test_api_document_accesses_delete_owners_last_owner(via, mock_user_teams): assert response.status_code == 403 assert models.DocumentAccess.objects.count() == 2 + + +def test_api_document_accesses_delete_owners_last_owner_child_user( + mock_reset_connections, # pylint: disable=redefined-outer-name +): + """ + It should be possible to delete the last owner access from a document that is not a root. + """ + user = factories.UserFactory(with_owned_document=True) + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory() + document = factories.DocumentFactory(parent=parent) + access = None + access = factories.UserDocumentAccessFactory( + document=document, user=user, role="owner" + ) + + assert models.DocumentAccess.objects.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.count() == 1 + + +@pytest.mark.skip( + reason="Pending fix on https://github.com/suitenumerique/docs/issues/969" +) +def test_api_document_accesses_delete_owners_last_owner_child_team( + mock_user_teams, + mock_reset_connections, # pylint: disable=redefined-outer-name +): + """ + It should be possible to delete the last owner access from a document that + is not a root. + """ + user = factories.UserFactory(with_owned_document=True) + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory() + document = factories.DocumentFactory(parent=parent) + access = None + mock_user_teams.return_value = ["lasuite", "unknown"] + access = factories.TeamDocumentAccessFactory( + document=document, team="lasuite", role="owner" + ) + + assert models.DocumentAccess.objects.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.count() == 1 diff --git a/src/backend/core/tests/documents/test_api_documents_children_create.py b/src/backend/core/tests/documents/test_api_documents_children_create.py index c5b6f2bf..b39653d5 100644 --- a/src/backend/core/tests/documents/test_api_documents_children_create.py +++ b/src/backend/core/tests/documents/test_api_documents_children_create.py @@ -114,7 +114,8 @@ def test_api_documents_children_create_authenticated_success(reach, role, depth) child = Document.objects.get(id=response.json()["id"]) assert child.title == "my child" assert child.link_reach == "restricted" - assert child.accesses.filter(role="owner", user=user).exists() + # Access objects on the child are not necessary + assert child.accesses.exists() is False @pytest.mark.parametrize("depth", [1, 2, 3]) @@ -182,7 +183,8 @@ def test_api_documents_children_create_related_success(role, depth): child = Document.objects.get(id=response.json()["id"]) assert child.title == "my child" assert child.link_reach == "restricted" - assert child.accesses.filter(role="owner", user=user).exists() + # Access objects on the child are not necessary + assert child.accesses.exists() is False def test_api_documents_children_create_authenticated_title_null(): diff --git a/src/backend/core/tests/documents/test_api_documents_move.py b/src/backend/core/tests/documents/test_api_documents_move.py index a0dd8350..ad4f68d4 100644 --- a/src/backend/core/tests/documents/test_api_documents_move.py +++ b/src/backend/core/tests/documents/test_api_documents_move.py @@ -124,8 +124,8 @@ def test_api_documents_move_authenticated_target_roles_mocked( target_role, target_parent_role, position ): """ - Authenticated users with insufficient permissions on the target document (or its - parent depending on the position chosen), should not be allowed to move documents. + Only authenticated users with sufficient permissions on the target document (or its + parent depending on the position chosen), should be allowed to move documents. """ user = factories.UserFactory() @@ -208,6 +208,107 @@ def test_api_documents_move_authenticated_target_roles_mocked( assert document.is_root() is True +def test_api_documents_move_authenticated_no_owner_user_and_team(): + """ + Moving a document with no owner to the root of the tree should automatically declare + the owner of the previous root of the document as owner of the document itself. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent_owner = factories.UserFactory() + parent = factories.DocumentFactory( + users=[(parent_owner, "owner")], teams=[("lasuite", "owner")] + ) + # A document with no owner + document = factories.DocumentFactory(parent=parent, users=[(user, "administrator")]) + child = factories.DocumentFactory(parent=document) + target = factories.DocumentFactory() + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-sibling"}, + ) + + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + assert list(target.get_siblings()) == [document, parent, target] + + document.refresh_from_db() + assert list(document.get_children()) == [child] + assert document.accesses.count() == 3 + assert document.accesses.get(user__isnull=False, role="owner").user == parent_owner + assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite" + assert document.accesses.get(role="administrator").user == user + + +def test_api_documents_move_authenticated_no_owner_same_user(): + """ + Moving a document should not fail if the user moving a document with no owner was + at the same time owner of the previous root and has a role on the document being moved. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory( + users=[(user, "owner")], teams=[("lasuite", "owner")] + ) + # A document with no owner + document = factories.DocumentFactory(parent=parent, users=[(user, "reader")]) + child = factories.DocumentFactory(parent=document) + target = factories.DocumentFactory() + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-sibling"}, + ) + + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + assert list(target.get_siblings()) == [document, parent, target] + + document.refresh_from_db() + assert list(document.get_children()) == [child] + assert document.accesses.count() == 2 + assert document.accesses.get(user__isnull=False, role="owner").user == user + assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite" + + +def test_api_documents_move_authenticated_no_owner_same_team(): + """ + Moving a document should not fail if the team that is owner of the document root was + already declared on the document with a different role. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory(teams=[("lasuite", "owner")]) + # A document with no owner but same team + document = factories.DocumentFactory( + parent=parent, users=[(user, "administrator")], teams=[("lasuite", "reader")] + ) + child = factories.DocumentFactory(parent=document) + target = factories.DocumentFactory() + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-sibling"}, + ) + + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + assert list(target.get_siblings()) == [document, parent, target] + + document.refresh_from_db() + assert list(document.get_children()) == [child] + assert document.accesses.count() == 2 + assert document.accesses.get(user__isnull=False, role="administrator").user == user + assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite" + + def test_api_documents_move_authenticated_deleted_document(): """ It should not be possible to move a deleted document or its descendants, even diff --git a/src/backend/core/tests/test_models_document_accesses.py b/src/backend/core/tests/test_models_document_accesses.py index 1de3e019..2fa88cf1 100644 --- a/src/backend/core/tests/test_models_document_accesses.py +++ b/src/backend/core/tests/test_models_document_accesses.py @@ -127,12 +127,18 @@ def test_models_document_access_get_abilities_for_owner_of_self_allowed(): } -def test_models_document_access_get_abilities_for_owner_of_self_last(): +def test_models_document_access_get_abilities_for_owner_of_self_last_on_root( + django_assert_num_queries, +): """ - Check abilities of self access for the owner of a document when there is only one owner left. + Check abilities of self access for the owner of a root document when there + is only one owner left. """ access = factories.UserDocumentAccessFactory(role="owner") - abilities = access.get_abilities(access.user) + + with django_assert_num_queries(2): + abilities = access.get_abilities(access.user) + assert abilities == { "destroy": False, "retrieve": True, @@ -142,6 +148,28 @@ def test_models_document_access_get_abilities_for_owner_of_self_last(): } +def test_models_document_access_get_abilities_for_owner_of_self_last_on_child( + django_assert_num_queries, +): + """ + Check abilities of self access for the owner of a child document when there + is only one owner left. + """ + parent = factories.DocumentFactory() + access = factories.UserDocumentAccessFactory(document__parent=parent, role="owner") + + with django_assert_num_queries(1): + abilities = access.get_abilities(access.user) + + assert abilities == { + "destroy": True, + "retrieve": True, + "update": True, + "partial_update": True, + "set_role_to": ["reader", "editor", "administrator", "owner"], + } + + def test_models_document_access_get_abilities_for_owner_of_owner(): """Check abilities of owner access for the owner of a document.""" access = factories.UserDocumentAccessFactory(role="owner")