🔒️(backend) fix race conditions between actions updating a docs

If two requests at the exact same time update a document and
link-configuration, a race condition can happen and one action can
overwrite the other one. If a document link_reach is changed from public
to restricted, this change can be overwritten and the owner does not
have this information. To fix this issue we set the updated_fields for
every save.

src/backend/core/api/serializers.py

@@ -289,10 +289,26 @@ class DocumentSerializer(ListDocumentSerializer):
         """
         When no data is sent on the update, skip making the update in the database and return
         directly the instance unchanged.
+
+        We don't rely on the update methof from the ModelSerializer to only save
+        validated_data fields to not override updated value from other
+        serializer like the LinkDocumentSerializer. A race condition triggering both
+        serializer in the same time can lead to an overlap and data can be lost.
         """
         if not validated_data:
             return instance  # No data provided, skip the update
-        return super().update(instance, validated_data)
+        for attr, value in validated_data.items():
+            setattr(instance, attr, value)
+        try:
+            # `updated_at` is explicit because it is an `auto_now` field: it has
+            # no validated_data entry and would not be written otherwise.
+            instance.save(update_fields=[*validated_data, "updated_at"])
+        except Exception:
+            # The in-memory instance may hold values the database has not
+            # accepted: reload them so the response reports the row as it is.
+            instance.refresh_from_db(fields=validated_data.keys())
+            raise
+        return instance
 
 
 class SearchDocumentSerializer(ListDocumentSerializer):

src/backend/core/api/viewsets.py

@@ -1897,7 +1897,7 @@ class DocumentViewSet(
 
         # saving a changed link definition is what tells the collaboration
         # server to re-check the connections of the document and its descendants
-        serializer.save()
+        serializer.save(update_fields=["link_reach", "link_role", "updated_at"])
 
         return drf.response.Response(serializer.data, status=drf.status.HTTP_200_OK)
 
@@ -2024,7 +2024,10 @@ class DocumentViewSet(
 
         # Make the attachment readable by document readers
         document.attachments.append(key)
-        document.save()
+        # Only the attachments column is written: the instance was loaded at the
+        # start of the request and an overlapping change must not be undone
+        # (e.g. a withdrawal of the link configuration committed in between).
+        document.save(update_fields=["attachments", "updated_at"])
 
         malware_detection.analyse_file(key, document_id=document.id)
 

src/backend/core/tests/documents/test_api_documents_attachment_upload.py

@@ -13,7 +13,7 @@ from django.core.files.uploadedfile import SimpleUploadedFile
 import pytest
 from rest_framework.test import APIClient
 
-from core import factories
+from core import factories, models
 from core.api.viewsets import malware_detection
 from core.tests.conftest import TEAM, USER, VIA
 
@@ -492,3 +492,45 @@ def test_api_documents_attachment_upload_unsafe_mime_types_disabled(settings):
         "application/octet-stream",
     ]
     assert file_head["ContentDisposition"] == 'attachment; filename="script.exe"'
+
+
+def test_api_documents_attachment_upload_stale_instance_does_not_revert_link_configuration():
+    """
+    An attachment upload must only write the attachments column.
+
+    A request that loaded the document before an owner withdrew the link
+    configuration must not put the old configuration back when it saves the
+    attachment.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    document = factories.DocumentFactory(
+        link_reach="public", link_role="editor", users=[(user, "editor")]
+    )
+
+    # The upload request loads the document while it is still public.
+    stale_document = models.Document.objects.get(pk=document.pk)
+
+    # The owner withdraws the link in between.
+    withdrawal = models.Document.objects.get(pk=document.pk)
+    withdrawal.link_reach = models.LinkReachChoices.RESTRICTED
+    withdrawal.link_role = models.LinkRoleChoices.READER
+    withdrawal.save(update_fields=["link_reach", "link_role"])
+
+    # The upload commits after the withdrawal.
+    file = SimpleUploadedFile(name="test.png", content=PIXEL, content_type="image/png")
+    url = f"/api/v1.0/documents/{document.id!s}/attachment-upload/"
+    with mock.patch.object(malware_detection, "analyse_file"):
+        response = client.post(url, {"file": file}, format="multipart")
+
+    assert response.status_code == 201
+
+    document.refresh_from_db()
+    assert len(document.attachments) == 1
+    assert document.link_reach == models.LinkReachChoices.RESTRICTED
+    assert document.link_role == models.LinkRoleChoices.READER
+
+    # The stale in-memory instance is left untouched for reference.
+    assert stale_document.link_reach == "public"

src/backend/core/tests/documents/test_api_documents_link_configuration.py

@@ -419,3 +419,91 @@ def test_api_documents_link_configuration_update_invalid_role_for_reach_validati
         in error_message
     )
     assert "Allowed roles: editor" in error_message
+
+
+def test_api_documents_link_configuration_survives_concurrent_title_patch(
+    mock_reset_connections,
+):
+    """
+    A title patch loaded before a withdrawal of the link configuration and
+    saved after it must not put the withdrawn link configuration back.
+    """
+    owner = factories.UserFactory()
+    client = APIClient()
+    client.force_login(owner)
+
+    document = factories.DocumentFactory(
+        link_reach=models.LinkReachChoices.PUBLIC,
+        link_role=models.LinkRoleChoices.EDITOR,
+        title="before",
+    )
+    factories.UserDocumentAccessFactory(
+        document=document, user=owner, role=models.RoleChoices.OWNER
+    )
+
+    # The concurrent writer (e.g. an anonymous editor) loads the document
+    # before the withdrawal: the request holds an instance in memory.
+    stale_document = models.Document.objects.get(pk=document.pk)
+
+    # The owner withdraws the link in between.
+    with mock_reset_connections(document.id):
+        response = client.put(
+            f"/api/v1.0/documents/{document.id!s}/link-configuration/",
+            {"link_reach": models.LinkReachChoices.RESTRICTED},
+            format="json",
+        )
+        assert response.status_code == 200
+
+    # The concurrent write commits after the withdrawal.
+    stale_document.title = "written by an anonymous visitor"
+    stale_document.save(update_fields=["title"])
+
+    # The withdrawal holds: the write did not write the sharing columns back.
+    # The withdrawal only changed the reach, so the editor role of the link is
+    # kept in the database while the reach is now restricted.
+    document.refresh_from_db()
+    assert document.link_reach == models.LinkReachChoices.RESTRICTED
+    assert document.title == "written by an anonymous visitor"
+
+
+def test_api_documents_link_configuration_save_writes_link_fields_only(
+    mock_reset_connections,
+):
+    """
+    The link-configuration endpoint must only write the link fields: a stale
+    instance must not be able to rewrite the rest of the row.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    document = factories.DocumentFactory(
+        link_reach=models.LinkReachChoices.PUBLIC,
+        link_role=models.LinkRoleChoices.READER,
+        title="untouched",
+    )
+    factories.UserDocumentAccessFactory(
+        document=document, user=user, role=models.RoleChoices.OWNER
+    )
+
+    # Corrupt the in-memory instance the request will come from to prove that
+    # none of the other columns are written by the endpoint.
+    stale_document = models.Document.objects.get(pk=document.pk)
+    stale_document.title = "title the endpoint must not save"
+    stale_document.path = "000000"  # a stale tree path
+
+    with mock_reset_connections(document.id):
+        response = client.put(
+            f"/api/v1.0/documents/{document.id!s}/link-configuration/",
+            {
+                "link_reach": models.LinkReachChoices.AUTHENTICATED,
+                "link_role": models.LinkRoleChoices.READER,
+            },
+            format="json",
+        )
+        assert response.status_code == 200
+
+    document.refresh_from_db()
+    assert document.link_reach == models.LinkReachChoices.AUTHENTICATED
+    assert document.title == "untouched"
+    assert document.path != "000000"

src/backend/core/tests/documents/test_api_documents_update.py

@@ -660,3 +660,170 @@ def test_api_documents_patch_empty_body():
     new_document_values = serializers.DocumentSerializer(instance=document).data
     assert new_document_values == old_document_values
     assert document_updated_at == document.updated_at
+
+
+@pytest.mark.parametrize("via", VIA)
+def test_api_documents_patch_stale_instance_does_not_revert_link_configuration(
+    via, mock_user_teams
+):
+    """
+    A write must only save the fields the request carries.
+
+    An editor patching a document from an instance loaded before an owner
+    withdrew the link configuration must not put the old link configuration
+    back: the patch saves the title only. This is the interleaving of the
+    reported race condition, reproduced deterministically by loading the
+    stale instance before the withdrawal and saving after it.
+    """
+    owner = factories.UserFactory()
+    editor = factories.UserFactory(with_owned_document=True)
+
+    client = APIClient()
+    client.force_login(editor)
+
+    document = factories.DocumentFactory(
+        link_reach="public", link_role="editor", title="before"
+    )
+    factories.UserDocumentAccessFactory(document=document, user=owner, role="owner")
+
+    if via == USER:
+        factories.UserDocumentAccessFactory(
+            document=document, user=editor, role="editor"
+        )
+    elif via == TEAM:
+        mock_user_teams.return_value = ["lasuite", "unknown"]
+        factories.TeamDocumentAccessFactory(
+            document=document, team="lasuite", role="editor"
+        )
+
+    # The editor's request loads the document while it is still public.
+    stale_document = models.Document.objects.get(pk=document.pk)
+
+    # The owner withdraws the link in between.
+    withdrawal = models.Document.objects.get(pk=document.pk)
+    withdrawal.link_reach = models.LinkReachChoices.RESTRICTED
+    withdrawal.link_role = models.LinkRoleChoices.READER
+    withdrawal.save(update_fields=["link_reach", "link_role"])
+
+    # The editor's write commits after the withdrawal.
+    response = client.patch(
+        f"/api/v1.0/documents/{document.id!s}/",
+        {"title": "written while revoked"},
+        format="json",
+    )
+    assert response.status_code == 200
+
+    # The withdrawal holds: the patch did not write the sharing columns back.
+    document.refresh_from_db()
+    assert document.title == "written while revoked"
+    assert document.link_reach == models.LinkReachChoices.RESTRICTED
+    assert document.link_role == models.LinkRoleChoices.READER
+
+    # The stale instance the patch came from is left untouched for reference.
+    assert stale_document.link_reach == models.LinkReachChoices.PUBLIC
+
+
+def test_api_documents_patch_stale_instance_does_not_resurrect_soft_deletion():
+    """
+    A write loaded before a soft deletion must not undo the deletion by
+    writing the deleted_at and ancestors_deleted_at columns back.
+    """
+    user = factories.UserFactory()
+
+    client = APIClient()
+    client.force_login(user)
+
+    document = factories.DocumentFactory(title="to delete", users=[(user, "owner")])
+
+    # The request loads the document while it is alive.
+    stale_document = models.Document.objects.get(pk=document.pk)
+
+    # The owner soft deletes it in between.
+    document.soft_delete()
+
+    # A write admitted before the deletion commits after it.
+    response = client.patch(
+        f"/api/v1.0/documents/{document.id!s}/",
+        {"title": "written while deleted"},
+        format="json",
+    )
+    assert response.status_code in [200, 403, 404]
+
+    document.refresh_from_db()
+    assert document.deleted_at is not None
+    assert document.ancestors_deleted_at is not None
+    # The title may have been saved on the deleted document: what must hold is
+    # that the deletion itself was not reverted.
+    assert stale_document.deleted_at is None
+
+
+def test_api_documents_patch_stale_instance_does_not_revert_path():
+    """
+    A write loaded before a move must not write the stale tree path back,
+    which would leave the document under one parent and counted by another.
+    """
+    user = factories.UserFactory()
+
+    client = APIClient()
+    client.force_login(user)
+
+    document = factories.DocumentFactory(title="to move", users=[(user, "owner")])
+    old_path = document.path
+
+    # The request loads the document before the move.
+    models.Document.objects.get(pk=document.pk)
+
+    # The owner moves it under another parent in between.
+    new_parent = factories.DocumentFactory()
+    document.move(new_parent, pos="last-child")
+
+    # The write commits after the move.
+    response = client.patch(
+        f"/api/v1.0/documents/{document.id!s}/",
+        {"title": "written after the move"},
+        format="json",
+    )
+    assert response.status_code == 200
+
+    document.refresh_from_db()
+    assert document.title == "written after the move"
+    assert document.path != old_path
+    assert document.get_parent().id == new_parent.id
+
+
+def test_api_documents_update_stale_instance_does_not_revert_link_configuration():
+    """
+    Same lost update through a full PUT: an update carrying only writable
+    fields must not rewrite the read-only link columns of the row.
+    """
+    user = factories.UserFactory()
+
+    client = APIClient()
+    client.force_login(user)
+
+    document = factories.DocumentFactory(
+        link_reach="public", link_role="editor", title="old title"
+    )
+    factories.UserDocumentAccessFactory(document=document, user=user, role="owner")
+
+    # The request loads the document while it is still public.
+    models.Document.objects.get(pk=document.pk)
+
+    # The owner withdraws the link in between.
+    withdrawal = models.Document.objects.get(pk=document.pk)
+    withdrawal.link_reach = models.LinkReachChoices.RESTRICTED
+    withdrawal.link_role = models.LinkRoleChoices.READER
+    withdrawal.save(update_fields=["link_reach", "link_role"])
+
+    # The write commits after the withdrawal: PUT with the writable fields only.
+    response = client.put(
+        f"/api/v1.0/documents/{document.id!s}/",
+        {"title": "written while revoked", "excerpt": "excerpt"},
+        format="json",
+    )
+    assert response.status_code == 200
+
+    document.refresh_from_db()
+    assert document.title == "written while revoked"
+    assert document.link_reach == models.LinkReachChoices.RESTRICTED
+    assert document.link_role == models.LinkRoleChoices.READER