♻️(backend) reset collaboration connection in cascade for all children

When an access is updated or deleted, or a link_configuration is
changed, the collaration_service is used to reset the connection in the
collaboration server. As accesses and link_configuration are inherited
in a Docs tree, if the user is connected to a child, the connection is
not reset. This commit fix this issue by calling the reset on every
children in the tree.
This commit is contained in:
Manuel Raynaud
2026-07-09 14:46:11 +02:00
parent d1db000c76
commit d35b81a6ed
9 changed files with 326 additions and 33 deletions
+4
View File
@@ -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
+4 -3
View File
@@ -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(
@@ -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
+14
View File
@@ -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)
@@ -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
@@ -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):
@@ -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))
@@ -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
)
@@ -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")