diff --git a/src/backend/core/api/locks.py b/src/backend/core/api/locks.py new file mode 100644 index 000000000..d98ec9bbd --- /dev/null +++ b/src/backend/core/api/locks.py @@ -0,0 +1,17 @@ +"""Row locks guarding the API write paths.""" + +import contextlib + +from django.db import transaction + +from core import models +from core.api.exceptions import ensure_room_not_deleted + + +@contextlib.contextmanager +def room_locked_for_update(room): + """Lock a room's row for the duration of a write.""" + with transaction.atomic(): + locked_room = models.Room.all_objects.select_for_update().get(pk=room.pk) + ensure_room_not_deleted(locked_room) + yield locked_room diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 2897b88eb..ccfa4b03f 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -44,6 +44,7 @@ from core import analytics, enums, models, utils from core.api import throttling from core.api.exceptions import ensure_room_not_deleted from core.api.filters import ListFileFilter +from core.api.locks import room_locked_for_update from core.enums import MEDIA_STORAGE_URL_PATTERN from core.recording.enums import FileExtension from core.recording.event.authentication import RecordingProcessWebhookAuthentication @@ -405,12 +406,20 @@ class RoomViewSet( # pylint: disable=too-many-public-methods ) def perform_update(self, serializer): - """Persist the room update, then sync metadata to LiveKit.""" + """Persist the room update under a row lock, then sync metadata to LiveKit. - old_configuration = serializer.instance.configuration - old_access_level = serializer.instance.access_level + The lock is what keeps a concurrent soft delete from being undone: the + full-row write of `save()` would otherwise carry back the `deleted_at` + read before the deletion committed. + """ - room = serializer.save() + with room_locked_for_update(serializer.instance) as locked_room: + serializer.instance = locked_room + + old_configuration = locked_room.configuration + old_access_level = locked_room.access_level + + room = serializer.save() if ( room.configuration == old_configuration diff --git a/src/backend/core/external_api/viewsets.py b/src/backend/core/external_api/viewsets.py index e528d5538..203c5bbc8 100644 --- a/src/backend/core/external_api/viewsets.py +++ b/src/backend/core/external_api/viewsets.py @@ -26,6 +26,7 @@ from rest_framework.generics import get_object_or_404 from core import analytics, api, models from core.api.exceptions import ensure_room_not_deleted from core.api.feature_flag import FeatureFlag +from core.api.locks import room_locked_for_update from core.services.jwt_token import JwtTokenService from core.services.room_management import RoomManagement, RoomManagementException from core.services.transit_code import TransitCodeService @@ -268,14 +269,17 @@ class RoomViewSet( self._track_room_event(instance, analytics.AnalyticsEvent.ROOM_DELETED) def perform_update(self, serializer: serializers.RoomSerializer): - """Persist the room update, sync it to LiveKit, then log and track it.""" + """Persist the room update under a row lock, sync it to LiveKit, then track it.""" - previous_values = { - "access_level": serializer.instance.access_level, - "configuration": copy.deepcopy(serializer.instance.configuration), - } + with room_locked_for_update(serializer.instance) as locked_room: + serializer.instance = locked_room - room = serializer.save() + previous_values = { + "access_level": locked_room.access_level, + "configuration": copy.deepcopy(locked_room.configuration), + } + + room = serializer.save() # Report the fields that actually changed, not the ones that were submitted. updated_fields = sorted( diff --git a/src/backend/core/tests/rooms/test_api_rooms_update.py b/src/backend/core/tests/rooms/test_api_rooms_update.py index afaee8419..9f5c5a962 100644 --- a/src/backend/core/tests/rooms/test_api_rooms_update.py +++ b/src/backend/core/tests/rooms/test_api_rooms_update.py @@ -11,8 +11,9 @@ from django.utils import timezone import pytest from rest_framework.test import APIClient +from ...api.serializers import RoomSerializer from ...factories import RoomFactory, UserFactory -from ...models import RoomAccessLevel +from ...models import Room, RoomAccessLevel from ...services.room_management import ( RoomManagement, RoomManagementException, @@ -468,3 +469,40 @@ def test_api_rooms_update_authenticated_with_user_access_token(privileged_role): assert response.status_code == 200 room.refresh_from_db() assert room.name == "new name" + + +@patch.object(RoomManagement, "sync_room_metadata") +def test_api_rooms_update_soft_deleted_after_load(mock_sync_room_metadata): + """A room soft deleted while an update is in flight should not be restored.""" + user = UserFactory() + room = RoomFactory( + name="Old name", + access_level=RoomAccessLevel.RESTRICTED, + users=[(user, "owner")], + ) + + original_is_valid = RoomSerializer.is_valid + + def soft_delete_while_validating(self, **kwargs): + is_valid = original_is_valid(self, **kwargs) + Room.all_objects.get(pk=self.instance.pk).soft_delete() + return is_valid + + client = APIClient() + client.force_login(user) + + with patch.object(RoomSerializer, "is_valid", soft_delete_while_validating): + response = client.patch( + f"/api/v1.0/rooms/{room.id!s}/", + {"name": "New name", "access_level": RoomAccessLevel.PUBLIC}, + ) + + assert response.status_code == 410 + assert response.json() == {"detail": "This room has been deleted."} + + refreshed_room = Room.all_objects.get(pk=room.pk) + assert refreshed_room.deleted_at is not None + assert refreshed_room.name == "Old name" + assert refreshed_room.access_level == RoomAccessLevel.RESTRICTED + + mock_sync_room_metadata.assert_not_called() diff --git a/src/backend/core/tests/test_external_api_rooms.py b/src/backend/core/tests/test_external_api_rooms.py index 91170da04..b9db583e4 100644 --- a/src/backend/core/tests/test_external_api_rooms.py +++ b/src/backend/core/tests/test_external_api_rooms.py @@ -17,6 +17,7 @@ from lasuite.oidc_resource_server.authentication import ResourceServerAuthentica from rest_framework.test import APIClient from core.analytics import AnalyticsEvent +from core.external_api.serializers import RoomSerializer from core.factories import ApplicationFactory, RoomFactory, UserFactory from core.models import ( Application, @@ -1444,6 +1445,47 @@ def test_api_rooms_update_tracks_analytics(mock_update_metadata, mock_capture): mock_update_metadata.assert_called_once() +@mock.patch("core.external_api.viewsets.analytics.capture") +@mock.patch.object(RoomManagement, "sync_room_metadata") +def test_api_rooms_update_soft_deleted_after_load( + mock_sync_room_metadata, mock_capture +): + """A room soft deleted while an update is in flight should not be restored.""" + + user = UserFactory() + room = RoomFactory( + users=[(user, RoleChoices.OWNER)], access_level=RoomAccessLevel.TRUSTED + ) + + original_is_valid = RoomSerializer.is_valid + + def soft_delete_while_validating(self, **kwargs): + is_valid = original_is_valid(self, **kwargs) + Room.all_objects.get(pk=self.instance.pk).soft_delete() + return is_valid + + token = generate_test_token(user, [ApplicationScope.ROOMS_UPDATE]) + + client = APIClient() + client.credentials(HTTP_AUTHORIZATION=f"Bearer {token}") + + with mock.patch.object(RoomSerializer, "is_valid", soft_delete_while_validating): + response = client.patch( + f"/external-api/v1.0/rooms/{room.id}/", + {"access_level": RoomAccessLevel.RESTRICTED}, + format="json", + ) + + assert response.status_code == 410 + + refreshed_room = Room.all_objects.get(pk=room.pk) + assert refreshed_room.deleted_at is not None + assert refreshed_room.access_level == RoomAccessLevel.TRUSTED + + mock_sync_room_metadata.assert_not_called() + mock_capture.assert_not_called() + + @mock.patch.object(RoomManagement, "delete_room") def test_api_rooms_delete_requires_authentication(mock_delete_room): """Deleting a room without authentication should return 401."""