From cb4c058c5db2f404d691220373a89c4dea89d2fb Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Wed, 6 Nov 2024 17:00:23 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20add=20minimal=20Recording?= =?UTF-8?q?=20viewset=20for=20room=20recordings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements routes to manage recordings within rooms, following the patterns established in Impress. The viewset exposes targeted endpoints rather than full CRUD operations, with recordings being created (soon) through room-specific routes (e.g. room/123/start-recording). The implementation draws from @sampaccoud's initial work and advices. Review focus areas: - Permission implementation choices - Serializer design and structure Credit: Initial work by @sampaccoud --- src/backend/core/api/permissions.py | 14 +- src/backend/core/api/serializers.py | 20 ++ src/backend/core/api/viewsets.py | 24 +++ src/backend/core/tests/recording/__init__.py | 0 .../recording/test_api_recordings_delete.py | 114 +++++++++++ .../recording/test_api_recordings_list.py | 180 ++++++++++++++++++ src/backend/core/urls.py | 1 + 7 files changed, 348 insertions(+), 5 deletions(-) create mode 100644 src/backend/core/tests/recording/__init__.py create mode 100644 src/backend/core/tests/recording/test_api_recordings_delete.py create mode 100644 src/backend/core/tests/recording/test_api_recordings_list.py diff --git a/src/backend/core/api/permissions.py b/src/backend/core/api/permissions.py index 7e6d4988..de0985af 100644 --- a/src/backend/core/api/permissions.py +++ b/src/backend/core/api/permissions.py @@ -65,15 +65,11 @@ class RoomPermissions(permissions.BasePermission): return obj.is_administrator(user) -class ResourceAccessPermission(permissions.BasePermission): +class ResourceAccessPermission(IsAuthenticated): """ Permissions for a room that can only be updated by room administrators. """ - def has_permission(self, request, view): - """Only allow authenticated users.""" - return request.user.is_authenticated - def has_object_permission(self, request, view, obj): """ Check that the logged-in user is administrator of the linked room. @@ -83,3 +79,11 @@ class ResourceAccessPermission(permissions.BasePermission): return obj.user == user return obj.resource.is_administrator(user) + + +class HasAbilityPermission(IsAuthenticated): + """Permission class for access objects.""" + + def has_object_permission(self, request, view, obj): + """Check permission for a given object.""" + return obj.get_abilities(request.user).get(view.action, False) diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index a1af11a2..f3ef3b9c 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -87,6 +87,15 @@ class NestedResourceAccessSerializer(ResourceAccessSerializer): user = UserSerializer(read_only=True) +class ListRoomSerializer(serializers.ModelSerializer): + """Serialize Room model for a list API endpoint.""" + + class Meta: + model = models.Room + fields = ["id", "name", "slug", "is_public"] + read_only_fields = ["id", "slug"] + + class RoomSerializer(serializers.ModelSerializer): """Serialize Room model for the API.""" @@ -136,3 +145,14 @@ class RoomSerializer(serializers.ModelSerializer): output["is_administrable"] = is_admin return output + + +class RecordingSerializer(serializers.ModelSerializer): + """Serialize Recording for the API.""" + + room = ListRoomSerializer(read_only=True) + + class Meta: + model = models.Recording + fields = ["id", "room", "created_at", "updated_at", "status"] + read_only_fields = fields diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index af1cd818..31bbb53d 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -277,3 +277,27 @@ class ResourceAccessViewSet( permission_classes = [permissions.ResourceAccessPermission] queryset = models.ResourceAccess.objects.all() serializer_class = serializers.ResourceAccessSerializer + + +class RecordingViewSet( + mixins.DestroyModelMixin, + mixins.ListModelMixin, + viewsets.GenericViewSet, +): + """ + API endpoints to access and perform actions on recordings. + """ + + pagination_class = Pagination + permission_classes = [permissions.HasAbilityPermission] + queryset = models.Recording.objects.all() + serializer_class = serializers.RecordingSerializer + + def get_queryset(self): + """Restrict recordings to the user's ones.""" + user = self.request.user + return ( + super() + .get_queryset() + .filter(Q(accesses__user=user) | Q(accesses__team__in=user.get_teams())) + ) diff --git a/src/backend/core/tests/recording/__init__.py b/src/backend/core/tests/recording/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/src/backend/core/tests/recording/test_api_recordings_delete.py b/src/backend/core/tests/recording/test_api_recordings_delete.py new file mode 100644 index 00000000..5cf9e1fb --- /dev/null +++ b/src/backend/core/tests/recording/test_api_recordings_delete.py @@ -0,0 +1,114 @@ +""" +Test recordings API endpoints in the Meet core app: delete. +""" + +import pytest +from rest_framework.test import APIClient + +from ...factories import RecordingFactory, UserFactory, UserRecordingAccessFactory +from ...models import Recording + +pytestmark = pytest.mark.django_db + + +def test_api_recordings_delete_anonymous(): + """Anonymous users should not be allowed to destroy a recording.""" + recording = RecordingFactory() + client = APIClient() + + response = client.delete( + f"/api/v1.0/recordings/{recording.id!s}/", + ) + + assert response.status_code == 401 + assert Recording.objects.count() == 1 + + +def test_api_recordings_delete_authenticated(): + """ + Authenticated users should not be allowed to delete a recording + from which they are not related. + """ + recording = RecordingFactory() + user = UserFactory() + + client = APIClient() + client.force_login(user) + + response = client.delete( + f"/api/v1.0/recordings/{recording.id!s}/", + ) + + assert response.status_code == 404 + assert Recording.objects.count() == 1 + + +def test_api_recordings_delete_members(): + """ + Authenticated users should not be allowed to delete a recording + from which they are only a member. + """ + + user = UserFactory() + access = UserRecordingAccessFactory(role="member", user=user) + + client = APIClient() + client.force_login(user) + + response = client.delete( + f"/api/v1.0/recordings/{access.recording.id}/", + ) + + assert response.status_code == 403 + assert Recording.objects.count() == 1 + + +@pytest.mark.parametrize( + "role", + ["owner", "administrator"], +) +def test_api_recordings_delete_active(role): + """ + Authenticated users cannot delete active recordings, even with deletion privileges. + """ + + user = UserFactory() + + recording = RecordingFactory(status="active") + access = UserRecordingAccessFactory(role=role, user=user, recording=recording) + + client = APIClient() + client.force_login(user) + + response = client.delete( + f"/api/v1.0/recordings/{access.recording.id}/", + ) + + assert response.status_code == 403 + assert Recording.objects.count() == 1 + + +@pytest.mark.parametrize( + "role", + ["owner", "administrator"], +) +def test_api_recordings_delete_final(role): + """ + Authenticated users should not be allowed to delete an active recording + from which they are an admin or owner. + """ + + user = UserFactory() + + recording = RecordingFactory(status="saved") + access = UserRecordingAccessFactory(role=role, user=user, recording=recording) + + client = APIClient() + client.force_login(user) + + response = client.delete( + f"/api/v1.0/recordings/{access.recording.id}/", + ) + + assert response.status_code == 204 + assert Recording.objects.count() == 0 diff --git a/src/backend/core/tests/recording/test_api_recordings_list.py b/src/backend/core/tests/recording/test_api_recordings_list.py new file mode 100644 index 00000000..90318727 --- /dev/null +++ b/src/backend/core/tests/recording/test_api_recordings_list.py @@ -0,0 +1,180 @@ +""" +Test recordings API endpoints in the Meet core app: list. +""" + +import operator +from unittest import mock + +import pytest +from rest_framework.pagination import PageNumberPagination +from rest_framework.test import APIClient + +from core import factories + +pytestmark = pytest.mark.django_db + + +def test_api_recordings_list_anonymous(): + """Anonymous users should not be able to list recordings.""" + factories.RecordingFactory() + response = APIClient().get("/api/v1.0/recordings/") + + assert response.status_code == 401 + + +@pytest.mark.parametrize( + "role", + ["administrator", "member", "owner"], +) +def test_api_recordings_list_authenticated_direct(role): + """ + Authenticated users listing recordings, should only see the recordings + to which they are related. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + + access = factories.UserRecordingAccessFactory(role=role, user=user) + factories.UserRecordingAccessFactory(user=other_user) + + recording = access.recording + room = recording.room + + response = client.get( + "/api/v1.0/recordings/", + ) + + assert response.status_code == 200 + results = response.json()["results"] + assert len(results) == 1 + expected_ids = { + str(recording.id), + } + result_ids = {result["id"] for result in results} + assert expected_ids == result_ids + assert results[0] == { + "id": str(recording.id), + "created_at": recording.created_at.isoformat().replace("+00:00", "Z"), + "room": { + "id": str(room.id), + "is_public": room.is_public, + "name": room.name, + "slug": room.slug, + }, + "status": "initiated", + "updated_at": recording.updated_at.isoformat().replace("+00:00", "Z"), + } + + +def test_api_recording_list_authenticated_via_team(mock_user_get_teams): + """ + Authenticated users should be able to list recordings they are a + owner/administrator/member of via a team. + """ + user = factories.UserFactory() + + client = APIClient() + client.force_login(user) + + mock_user_get_teams.return_value = ["team1", "team2", "unknown"] + + recordings_team1 = [ + access.recording + for access in factories.TeamRecordingAccessFactory.create_batch(2, team="team1") + ] + recordings_team2 = [ + access.recording + for access in factories.TeamRecordingAccessFactory.create_batch(3, team="team2") + ] + + expected_ids = { + str(recording.id) for recording in recordings_team1 + recordings_team2 + } + + response = client.get("/api/v1.0/recordings/") + + assert response.status_code == 200 + results = response.json()["results"] + assert len(results) == 5 + results_id = {result["id"] for result in results} + assert expected_ids == results_id + + +@mock.patch.object(PageNumberPagination, "get_page_size", return_value=2) +def test_api_recordings_list_pagination(_mock_page_size): + """Pagination should work as expected.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + recording_ids = [ + str(access.recording_id) + for access in factories.UserRecordingAccessFactory.create_batch(3, user=user) + ] + + response = client.get("/api/v1.0/recordings/") + + assert response.status_code == 200 + content = response.json() + assert content["count"] == 3 + assert content["next"] == "http://testserver/api/v1.0/recordings/?page=2" + assert content["previous"] is None + + assert len(content["results"]) == 2 + for item in content["results"]: + recording_ids.remove(item["id"]) + + # Get page 2 + response = client.get( + "/api/v1.0/recordings/?page=2", + ) + + assert response.status_code == 200 + content = response.json() + + assert content["count"] == 3 + assert content["next"] is None + assert content["previous"], "http://testserver/api/v1.0/recordings/" + + assert len(content["results"]) == 1 + recording_ids.remove(content["results"][0]["id"]) + assert recording_ids == [] + + +def test_api_recordings_list_authenticated_distinct(): + """A recording for a room with several related users should only be listed once.""" + user = factories.UserFactory() + other_user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + recording = factories.RecordingFactory(users=[user, other_user]) + + response = client.get("/api/v1.0/recordings/") + + assert response.status_code == 200 + content = response.json() + assert len(content["results"]) == 1 + assert content["results"][0]["id"] == str(recording.id) + + +def test_api_recordings_list_ordering_default(): + """Recordings should be ordered by descending "updated_at" by default""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + factories.RecordingFactory.create_batch(5, users=[user]) + + response = client.get("/api/v1.0/recordings/") + + assert response.status_code == 200 + results = response.json()["results"] + assert len(results) == 5 + + # Check that results are sorted by descending "updated_at" as expected + for i in range(4): + assert operator.ge(results[i]["updated_at"], results[i + 1]["updated_at"]) diff --git a/src/backend/core/urls.py b/src/backend/core/urls.py index f7f0c5a9..88a33811 100644 --- a/src/backend/core/urls.py +++ b/src/backend/core/urls.py @@ -12,6 +12,7 @@ from core.authentication.urls import urlpatterns as oidc_urls router = DefaultRouter() router.register("users", viewsets.UserViewSet, basename="users") router.register("rooms", viewsets.RoomViewSet, basename="rooms") +router.register("recordings", viewsets.RecordingViewSet, basename="recordings") router.register( "resource-accesses", viewsets.ResourceAccessViewSet, basename="resource_accesses" )