diff --git a/CHANGELOG.md b/CHANGELOG.md index d0392b7ae..eff4b3906 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ and this project adheres to ## [Unreleased] +### Changed + +- ♻️(backend) reset collaboration connection in cascade for all children #2507 + ## [v5.4.0] - 2026-07-07 ### Added diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 0b8e5f213..5d9991bcb 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -68,6 +68,7 @@ from core.services.search_indexers import ( get_document_indexer, get_visited_document_ids_of, ) +from core.tasks.access import reset_service_connections_in_cascade from core.tasks.mail import send_ask_for_access_mail from core.utils.analytics import PosthogEventName, posthog_capture from core.utils.paths import filter_descendants @@ -1825,7 +1826,7 @@ class DocumentViewSet( serializer.save() # Notify collaboration server about the link updated - CollaborationService().reset_connections(str(document.id)) + reset_service_connections_in_cascade.delay(str(document.id)) return drf.response.Response(serializer.data, status=drf.status.HTTP_200_OK) @@ -2839,7 +2840,7 @@ class DocumentAccessViewSet( access_user_id = str(access.user.id) # Notify collaboration server about the access change - CollaborationService().reset_connections( + reset_service_connections_in_cascade.delay( str(access.document.id), access_user_id ) @@ -2860,7 +2861,7 @@ class DocumentAccessViewSet( ) # Notify collaboration server about the access removed - CollaborationService().reset_connections(document_id, user_id) + reset_service_connections_in_cascade.delay(document_id, user_id) class InvitationViewset( diff --git a/src/backend/core/services/collaboration_services.py b/src/backend/core/services/collaboration_services.py index 9587a1b19..fa1e1e867 100644 --- a/src/backend/core/services/collaboration_services.py +++ b/src/backend/core/services/collaboration_services.py @@ -1,10 +1,16 @@ """Collaboration services.""" +from logging import getLogger + from django.conf import settings from django.core.exceptions import ImproperlyConfigured import requests +from core import models + +logger = getLogger(__name__) + class CollaborationService: """Service class for Collaboration related operations.""" @@ -14,12 +20,34 @@ class CollaborationService: if settings.COLLABORATION_API_URL is None: raise ImproperlyConfigured("Collaboration configuration not set") - def reset_connections(self, room, user_id=None): + def reset_connections(self, document_id, user_id=None): """ - Reset connections of a room in the collaboration server. + Reset the connections of a document and all its descendants in the + collaboration server. + Resetting a connection means that the user will be disconnected and will have to reconnect to the collaboration server, with updated rights. """ + try: + document = models.Document.objects.get(pk=document_id) + except models.Document.DoesNotExist: + logger.error("Document %s does not exists anymore", document_id) + return + + documents = models.Document.objects.filter( + path__startswith=document.path, depth__gte=document.depth + ).order_by("path") + + for doc in documents: + try: + self._reset_connection(doc.id, user_id) + except requests.HTTPError: + logger.error("impossible to reset connections for document %s", doc.id) + + def _reset_connection(self, room, user_id=None): + """ + Reset connections of a single room in the collaboration server. + """ endpoint = "reset-connections" # room is necessary as a parameter, it is easier to stick to the diff --git a/src/backend/core/tasks/access.py b/src/backend/core/tasks/access.py new file mode 100644 index 000000000..1f7dd1ea8 --- /dev/null +++ b/src/backend/core/tasks/access.py @@ -0,0 +1,14 @@ +"""Tasks dedicated to document's accesses.""" + +from core.services.collaboration_services import CollaborationService + +from impress.celery_app import app + + +@app.task +def reset_service_connections_in_cascade(document_id, user_id=None): + """ + For a given document_id, reset the connections of the document and all its + descendants by delegating to the CollaborationService. + """ + CollaborationService().reset_connections(document_id, user_id) 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 0feda4a7b..83c4b392f 100644 --- a/src/backend/core/tests/documents/test_api_document_accesses.py +++ b/src/backend/core/tests/documents/test_api_document_accesses.py @@ -4,6 +4,7 @@ Test document accesses API endpoints for users in impress's core app. # pylint: disable=too-many-lines import random +from contextlib import contextmanager from unittest import mock from uuid import uuid4 @@ -13,14 +14,30 @@ from rest_framework.test import APIClient from core import choices, factories, models from core.api import serializers from core.tests.conftest import TEAM, USER, VIA -from core.tests.test_services_collaboration_services import ( # pylint: disable=unused-import - mock_reset_connections, -) from core.utils.analytics import PosthogEventName pytestmark = pytest.mark.django_db +@pytest.fixture(name="mock_reset_connections") +def mock_reset_connections_fixture(): + """ + Provide a context manager that patches the ``reset_service_connections_in_cascade`` + Celery task and asserts its ``delay`` method is called exactly once for the given + document and user when leaving the context. + """ + + @contextmanager + def _mock_reset_connections(document_id, user_id=None): + with mock.patch( + "core.api.viewsets.reset_service_connections_in_cascade.delay" + ) as mock_delay: + yield mock_delay + mock_delay.assert_called_once_with(str(document_id), user_id) + + return _mock_reset_connections + + def test_api_document_accesses_list_anonymous(): """Anonymous users should not be allowed to list document accesses.""" document = factories.DocumentFactory() @@ -737,7 +754,7 @@ def test_api_document_accesses_update_administrator_except_owner( create_for, via, mock_user_teams, - mock_reset_connections, # pylint: disable=redefined-outer-name + mock_reset_connections, ): """ A user who is a direct administrator in a document should be allowed to update a user @@ -847,7 +864,7 @@ def test_api_document_accesses_update_administrator_from_owner(via, mock_user_te def test_api_document_accesses_update_administrator_to_owner( via, mock_user_teams, - mock_reset_connections, # pylint: disable=redefined-outer-name + mock_reset_connections, ): """ A user who is an administrator in a document, should not be allowed to update @@ -914,7 +931,7 @@ def test_api_document_accesses_update_owner( create_for, via, mock_user_teams, - mock_reset_connections, # pylint: disable=redefined-outer-name + mock_reset_connections, ): """ A user who is an owner in a document should be allowed to update @@ -977,7 +994,7 @@ def test_api_document_accesses_update_owner( def test_api_document_accesses_update_owner_self_root( via, mock_user_teams, - mock_reset_connections, # pylint: disable=redefined-outer-name + mock_reset_connections, ): """ A user who is owner of a document should be allowed to update @@ -1039,7 +1056,7 @@ def test_api_document_accesses_update_owner_self_root( def test_api_document_accesses_update_owner_self_child( via, mock_user_teams, - mock_reset_connections, # pylint: disable=redefined-outer-name + mock_reset_connections, ): """ A user who is owner of a document should be allowed to update @@ -1153,7 +1170,7 @@ def test_api_document_accesses_delete_reader_or_editor(via, role, mock_user_team def test_api_document_accesses_delete_administrators_except_owners( via, mock_user_teams, - mock_reset_connections, # pylint: disable=redefined-outer-name + mock_reset_connections, ): """ Users who are administrators in a document should be allowed to delete an access @@ -1238,7 +1255,7 @@ def test_api_document_accesses_delete_administrator_on_owners(via, mock_user_tea def test_api_document_accesses_delete_owners( via, mock_user_teams, - mock_reset_connections, # pylint: disable=redefined-outer-name + mock_reset_connections, ): """ Users should be able to delete the document access of another user @@ -1311,7 +1328,7 @@ def test_api_document_accesses_delete_owners_last_owner_root(via, mock_user_team def test_api_document_accesses_delete_owners_last_owner_child_user( - mock_reset_connections, # pylint: disable=redefined-outer-name + mock_reset_connections, ): """ It should be possible to delete the last owner access from a document that is not a root. @@ -1342,7 +1359,7 @@ def test_api_document_accesses_delete_owners_last_owner_child_user( ) def test_api_document_accesses_delete_owners_last_owner_child_team( mock_user_teams, - mock_reset_connections, # pylint: disable=redefined-outer-name + mock_reset_connections, ): """ It should be possible to delete the last owner access from a document that diff --git a/src/backend/core/tests/documents/test_api_documents_link_configuration.py b/src/backend/core/tests/documents/test_api_documents_link_configuration.py index 2c271ca1e..9a3a3c8fd 100644 --- a/src/backend/core/tests/documents/test_api_documents_link_configuration.py +++ b/src/backend/core/tests/documents/test_api_documents_link_configuration.py @@ -1,18 +1,37 @@ """Tests for link configuration of documents on API endpoint""" +from contextlib import contextmanager +from unittest import mock + import pytest from rest_framework.test import APIClient from core import factories, models from core.api import serializers from core.tests.conftest import TEAM, USER, VIA -from core.tests.test_services_collaboration_services import ( # pylint: disable=unused-import - mock_reset_connections, -) pytestmark = pytest.mark.django_db +@pytest.fixture(name="mock_reset_connections") +def mock_reset_connections_fixture(): + """ + Provide a context manager that patches the ``reset_service_connections_in_cascade`` + Celery task and asserts its ``delay`` method is called exactly once for the given + document when leaving the context. + """ + + @contextmanager + def _mock_reset_connections(document_id): + with mock.patch( + "core.api.viewsets.reset_service_connections_in_cascade.delay" + ) as mock_delay: + yield mock_delay + mock_delay.assert_called_once_with(str(document_id)) + + return _mock_reset_connections + + @pytest.mark.parametrize("role", models.LinkRoleChoices.values) @pytest.mark.parametrize("reach", models.LinkReachChoices.values) def test_api_documents_link_configuration_update_anonymous(reach, role): diff --git a/src/backend/core/tests/external_api/test_external_api_documents_link_configuration.py b/src/backend/core/tests/external_api/test_external_api_documents_link_configuration.py index 9fc3d8dd3..885862f0f 100644 --- a/src/backend/core/tests/external_api/test_external_api_documents_link_configuration.py +++ b/src/backend/core/tests/external_api/test_external_api_documents_link_configuration.py @@ -63,7 +63,7 @@ def test_external_api_documents_link_configuration_not_allowed( COLLABORATION_API_URL="http://example.com/", COLLABORATION_SERVER_SECRET="secret-token", ) -@patch("core.services.collaboration_services.CollaborationService.reset_connections") +@patch("core.api.viewsets.reset_service_connections_in_cascade.delay") def test_external_api_documents_link_configuration_can_be_allowed( mock_reset, user_token, resource_server_backend, user_specific_sub ): @@ -103,3 +103,6 @@ def test_external_api_documents_link_configuration_can_be_allowed( document.refresh_from_db() assert document.link_reach == models.LinkReachChoices.PUBLIC assert document.link_role == models.LinkRoleChoices.EDITOR + + # the collaboration server should be notified through the Celery task + mock_reset.assert_called_once_with(str(document.id)) diff --git a/src/backend/core/tests/test_services_collaboration_services.py b/src/backend/core/tests/test_services_collaboration_services.py index 5a9151700..35e607d21 100644 --- a/src/backend/core/tests/test_services_collaboration_services.py +++ b/src/backend/core/tests/test_services_collaboration_services.py @@ -4,8 +4,11 @@ core.services.collaboration_services module. """ import json +import logging import re from contextlib import contextmanager +from unittest import mock +from uuid import uuid4 from django.core.exceptions import ImproperlyConfigured @@ -13,11 +16,14 @@ import pytest import requests import responses +from core import factories, models from core.services.collaboration_services import CollaborationService +# pylint: disable=protected-access -@pytest.fixture -def mock_reset_connections(settings): + +@pytest.fixture(name="mock_reset_connections") +def mock_reset_connections_fixture(settings): """ Creates a context manager to mock the reset-connections endpoint for collaboration services. Args: @@ -88,8 +94,8 @@ def test_init_with_api_url(settings): @responses.activate -def test_reset_connections_with_user_id(settings): - """Test reset_connections with a provided user_id.""" +def test_reset_connection_with_user_id(settings): + """Test _reset_connection with a provided user_id.""" settings.COLLABORATION_API_URL = "http://example.com/" settings.COLLABORATION_SERVER_SECRET = "secret-token" service = CollaborationService() @@ -100,7 +106,7 @@ def test_reset_connections_with_user_id(settings): responses.add(responses.POST, endpoint_url, json={}, status=200) - service.reset_connections(room, user_id) + service._reset_connection(room, user_id) assert len(responses.calls) == 1 request = responses.calls[0].request @@ -111,8 +117,8 @@ def test_reset_connections_with_user_id(settings): @responses.activate -def test_reset_connections_without_user_id(settings): - """Test reset_connections without a user_id.""" +def test_reset_connection_without_user_id(settings): + """Test _reset_connection without a user_id.""" settings.COLLABORATION_API_URL = "http://example.com/" settings.COLLABORATION_SERVER_SECRET = "secret-token" service = CollaborationService() @@ -128,7 +134,7 @@ def test_reset_connections_without_user_id(settings): status=200, ) - service.reset_connections(room, user_id) + service._reset_connection(room, user_id) assert len(responses.calls) == 1 request = responses.calls[0].request @@ -139,7 +145,7 @@ def test_reset_connections_without_user_id(settings): @responses.activate -def test_reset_connections_non_200_response(settings): +def test_reset_connection_non_200_response(settings): """Test that an HTTPError is raised when the response status is not 200.""" settings.COLLABORATION_API_URL = "http://example.com/" settings.COLLABORATION_SERVER_SECRET = "secret-token" @@ -157,13 +163,13 @@ def test_reset_connections_non_200_response(settings): ) + re.escape(json.dumps(response_body)) with pytest.raises(requests.HTTPError, match=expected_exception_message): - service.reset_connections(room, user_id) + service._reset_connection(room, user_id) assert len(responses.calls) == 1 @responses.activate -def test_reset_connections_request_exception(settings): +def test_reset_connection_request_exception(settings): """Test that an HTTPError is raised when a RequestException occurs.""" settings.COLLABORATION_API_URL = "http://example.com/" settings.COLLABORATION_SERVER_SECRET = "secret-token" @@ -180,6 +186,159 @@ def test_reset_connections_request_exception(settings): ) with pytest.raises(requests.HTTPError, match="Failed to notify WebSocket server."): - service.reset_connections(room, user_id) + service._reset_connection(room, user_id) assert len(responses.calls) == 1 + + +@pytest.fixture(name="collaboration_service") +def collaboration_service_fixture(settings): + """Return a configured CollaborationService instance.""" + settings.COLLABORATION_API_URL = "http://example.com/" + settings.COLLABORATION_SERVER_SECRET = "secret-token" + return CollaborationService() + + +@pytest.mark.django_db +@mock.patch.object(CollaborationService, "_reset_connection") +def test_reset_connections_document_does_not_exist( + mock_reset_connection, + collaboration_service, + caplog, +): + """ + When the document does not exist anymore, an error is logged and no + connection is reset. + """ + unknown_id = uuid4() + + with caplog.at_level(logging.ERROR, logger="core.services.collaboration_services"): + collaboration_service.reset_connections(unknown_id) + + mock_reset_connection.assert_not_called() + assert f"Document {unknown_id} does not exists anymore" in caplog.text + + +@pytest.mark.django_db +@mock.patch.object(CollaborationService, "_reset_connection") +def test_reset_connections_single_document( + mock_reset_connection, + collaboration_service, +): + """A document without descendants should have its own connections reset.""" + document = factories.DocumentFactory() + + collaboration_service.reset_connections(document.id) + + mock_reset_connection.assert_called_once_with(document.id, None) + + +@pytest.mark.django_db +@mock.patch.object(CollaborationService, "_reset_connection") +def test_reset_connections_cascade_on_document_and_descendants( + mock_reset_connection, + collaboration_service, +): + """ + The document itself and every one of its descendants should be reset, + ordered by path. + """ + root = factories.DocumentFactory() + child1 = factories.DocumentFactory(parent=root) + child2 = factories.DocumentFactory(parent=root) + grandchild = factories.DocumentFactory(parent=child1) + + collaboration_service.reset_connections(root.id) + + expected_ids = [ + doc.id + for doc in models.Document.objects.filter( + path__startswith=root.path, depth__gte=root.depth + ).order_by("path") + ] + assert set(expected_ids) == {root.id, child1.id, child2.id, grandchild.id} + + called_ids = [call.args[0] for call in mock_reset_connection.call_args_list] + assert called_ids == expected_ids + assert mock_reset_connection.call_count == 4 + + +@pytest.mark.django_db +@mock.patch.object(CollaborationService, "_reset_connection") +def test_reset_connections_starts_from_a_sub_document( + mock_reset_connection, + collaboration_service, +): + """ + When called on a sub-document, only that sub-document and its own + descendants should be reset, not its ancestors or siblings. + """ + root = factories.DocumentFactory() + child = factories.DocumentFactory(parent=root) + sibling = factories.DocumentFactory(parent=root) + grandchild = factories.DocumentFactory(parent=child) + + collaboration_service.reset_connections(child.id) + + called_ids = {call.args[0] for call in mock_reset_connection.call_args_list} + assert called_ids == {child.id, grandchild.id} + assert root.id not in called_ids + assert sibling.id not in called_ids + + +@pytest.mark.django_db +@mock.patch.object(CollaborationService, "_reset_connection") +def test_reset_connections_forwards_user_id( + mock_reset_connection, + collaboration_service, +): + """The provided user_id should be forwarded to every reset call.""" + root = factories.DocumentFactory() + factories.DocumentFactory(parent=root) + user_id = str(uuid4()) + + collaboration_service.reset_connections(root.id, user_id=user_id) + + assert mock_reset_connection.call_count == 2 + for call in mock_reset_connection.call_args_list: + assert call.args[1] == user_id + + +@pytest.mark.django_db +@mock.patch.object(CollaborationService, "_reset_connection") +def test_reset_connections_continues_on_http_error( + mock_reset_connection, + collaboration_service, + caplog, +): + """ + An HTTPError raised while resetting one document should be logged and must + not prevent the remaining documents from being processed. + """ + root = factories.DocumentFactory() + child1 = factories.DocumentFactory(parent=root) + child2 = factories.DocumentFactory(parent=root) + + ordered_docs = list( + models.Document.objects.filter( + path__startswith=root.path, depth__gte=root.depth + ).order_by("path") + ) + failing_doc = ordered_docs[1] + + def _side_effect(room, _user_id=None): + if room == failing_doc.id: + raise requests.HTTPError("boom") + + mock_reset_connection.side_effect = _side_effect + + with caplog.at_level(logging.ERROR, logger="core.services.collaboration_services"): + collaboration_service.reset_connections(root.id) + + assert mock_reset_connection.call_count == 3 + called_ids = [call.args[0] for call in mock_reset_connection.call_args_list] + assert set(called_ids) == {root.id, child1.id, child2.id} + + assert ( + f"impossible to reset connections for document {failing_doc.id}" in caplog.text + ) diff --git a/src/backend/core/tests/test_tasks_access.py b/src/backend/core/tests/test_tasks_access.py new file mode 100644 index 000000000..d794c6b24 --- /dev/null +++ b/src/backend/core/tests/test_tasks_access.py @@ -0,0 +1,48 @@ +""" +Tests for the `reset_service_connections_in_cascade` Celery task in the +core.tasks.access module. +""" + +from unittest import mock + +from django.core.exceptions import ImproperlyConfigured + +import pytest + +from core.tasks.access import reset_service_connections_in_cascade + + +@mock.patch("core.tasks.access.CollaborationService") +def test_reset_service_connections_delegates_to_service(mock_service): + """ + The task should delegate the whole reset to the CollaborationService, + forwarding both the document id and the user id. + """ + reset_service_connections_in_cascade("document-id", "user-id") + + mock_service.return_value.reset_connections.assert_called_once_with( + "document-id", "user-id" + ) + + +@mock.patch("core.tasks.access.CollaborationService") +def test_reset_service_connections_defaults_user_id_to_none(mock_service): + """When no user id is provided, the task should forward None to the service.""" + reset_service_connections_in_cascade("document-id") + + mock_service.return_value.reset_connections.assert_called_once_with( + "document-id", None + ) + + +@mock.patch( + "core.tasks.access.CollaborationService", + side_effect=ImproperlyConfigured("Collaboration configuration not set"), +) +def test_reset_service_connections_propagates_improperly_configured(mock_service): # pylint: disable=unused-argument + """ + If the collaboration service is not configured, instantiating it raises + ImproperlyConfigured, which should propagate out of the task. + """ + with pytest.raises(ImproperlyConfigured): + reset_service_connections_in_cascade("document-id")