diff --git a/src/backend/core/models.py b/src/backend/core/models.py index adb560b10..c6aa36f7e 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -743,28 +743,32 @@ class Recording(BaseModel): return { "destroy": is_owner_or_admin and is_final_status, "partial_update": False, - "retrieve": is_owner_or_admin, + "retrieve": bool(roles), "stop": is_owner_or_admin and not is_final_status, "update": False, } def grant_initial_accesses(self, starter): - """Make the starter and the room's owners/administrators OWNER of the recording. + """Grant the starter and the room's owners/administrators access to the recording. The starter's access is created first, so it is the oldest one: the - transcript summary, which goes to a single owner, is sent to them. + transcript summary, which goes to a single user, is sent to them. Room roles are copied when the recording starts: later changes to the room's accesses do not affect existing recordings. """ - RecordingAccess.objects.create( - user=starter, role=RoleChoices.OWNER, recording=self + room_privileged_user_ids = set( + self.room.accesses.filter( + role__in=[RoleChoices.OWNER, RoleChoices.ADMIN] + ).values_list("user_id", flat=True) ) + starter_is_privileged = starter.id in room_privileged_user_ids + room_privileged_user_ids.discard(starter.id) - room_privileged_user_ids = ( - self.room.accesses.filter(role__in=[RoleChoices.OWNER, RoleChoices.ADMIN]) - .exclude(user=starter) - .values_list("user_id", flat=True) + RecordingAccess.objects.create( + user=starter, + role=RoleChoices.OWNER if starter_is_privileged else RoleChoices.MEMBER, + recording=self, ) RecordingAccess.objects.bulk_create( RecordingAccess(user_id=user_id, role=RoleChoices.OWNER, recording=self) diff --git a/src/backend/core/recording/event/notification.py b/src/backend/core/recording/event/notification.py index 5e56f244d..7c3917164 100644 --- a/src/backend/core/recording/event/notification.py +++ b/src/backend/core/recording/event/notification.py @@ -68,7 +68,8 @@ class NotificationService: @staticmethod def _notify_user_by_email(recording) -> bool: """ - Queue an email notification to recording owners when their recording is ready. + Queue an email notification to every user with access to the recording + when it is ready: its starter and the room's owners/administrators. The email includes a direct link that redirects owners to a dedicated download page in the frontend where they can access their specific recording. @@ -80,10 +81,7 @@ class NotificationService: owner_accesses = ( models.RecordingAccess.objects.select_related("user") - .filter( - role=models.RoleChoices.OWNER, - recording_id=recording.id, - ) + .filter(recording_id=recording.id) .order_by("created_at") ) @@ -211,18 +209,22 @@ class NotificationService: def _get_summary_recipient(recording: models.Recording): """Return the single user who receives the transcript summary. - A recording can have several owners (its starter and the room's - owners/administrators), but the summary is generated once. It goes to - the oldest owner, which is the starter: their access is created first - (see `Recording.grant_initial_accesses`). + A recording can have multiple accesses (the starter and the room's + owners/administrators), but its summary is generated only once. + + Temporarily, until the document can be created and shared with all + recording accesses, the summary is assigned to the oldest access, + which belongs to the starter (see `Recording.grant_initial_accesses`). + + Since the starter may have a MEMBER role, the lookup must not filter by role. """ - oldest_owner_access = ( + oldest_access = ( models.RecordingAccess.objects.select_related("user") - .filter(role=models.RoleChoices.OWNER, recording_id=recording.id) + .filter(recording_id=recording.id) .order_by("created_at") .first() ) - return oldest_owner_access.user if oldest_owner_access else None + return oldest_access.user if oldest_access else None @staticmethod def _notify_summary_service_v1(recording: models.Recording): diff --git a/src/backend/core/tests/recording/event/test_notification.py b/src/backend/core/tests/recording/event/test_notification.py index 0a3423864..9bba7953b 100644 --- a/src/backend/core/tests/recording/event/test_notification.py +++ b/src/backend/core/tests/recording/event/test_notification.py @@ -100,7 +100,7 @@ def test_notify_external_services_unknown_mode(caplog): # pylint: disable=too-many-locals def test_notify_user_by_email_success(mocked_current_site, settings): - """Test successful email notification to recording owners.""" + """Test successful email notification to every user with access to the recording.""" settings.EMAIL_BRAND_NAME = "ACME" settings.EMAIL_SUPPORT_EMAIL = "support@acme.com" settings.EMAIL_LOGO_IMG = "https://acme.com/logo" @@ -133,12 +133,10 @@ def test_notify_user_by_email_success(mocked_current_site, settings): recording=recording, role=models.RoleChoices.OWNER, user=english_user ) - # Create non-owner users to verify they don't receive emails + # Members (e.g. a starter with no privileged room role) are notified too + member = factories.UserFactory(email="member@test.com", language="en-us") factories.UserRecordingAccessFactory( - recording=recording, role=models.RoleChoices.MEMBER - ) - factories.UserRecordingAccessFactory( - recording=recording, role=models.RoleChoices.ADMIN + recording=recording, role=models.RoleChoices.MEMBER, user=member ) notification_service = NotificationService() @@ -147,7 +145,7 @@ def test_notify_user_by_email_success(mocked_current_site, settings): result = notification_service._notify_user_by_email(recording) assert result is True - assert mock_send_mail.call_count == 3 + assert mock_send_mail.call_count == 4 call_args_list = mock_send_mail.call_args_list @@ -211,6 +209,9 @@ def test_notify_user_by_email_success(mocked_current_site, settings): assert recipients3 == ["john@test.com"] assert sender3 == "notifications@acme.com" + _, _, _, recipients4 = call_args_list[3][0] + assert recipients4 == ["member@test.com"] + def test_notify_user_by_email_no_owners(mocked_current_site, caplog): """Test email notification when no owners are found.""" @@ -536,8 +537,8 @@ def test_notify_summary_service_v2_payload_json_serializable_without_timestamps( json.dumps(payload) -def test_summary_recipient_is_oldest_owner(): - """With several owners, the summary goes to the oldest one.""" +def test_summary_recipient_is_oldest_access(): + """With several accesses, the summary goes to the oldest one.""" recording = factories.RecordingFactory() oldest = factories.UserRecordingAccessFactory( recording=recording, role=models.RoleChoices.OWNER @@ -550,8 +551,21 @@ def test_summary_recipient_is_oldest_owner(): assert NotificationService._get_summary_recipient(recording) == oldest.user -def test_summary_recipient_none_without_owner(): - """No owner: nobody to send the summary to.""" +def test_summary_recipient_is_member_starter(): + """A starter holding a MEMBER access still receives the summary.""" + recording = factories.RecordingFactory() + starter = factories.UserRecordingAccessFactory( + recording=recording, role=models.RoleChoices.MEMBER + ) + factories.UserRecordingAccessFactory( + recording=recording, role=models.RoleChoices.OWNER + ) + + assert NotificationService._get_summary_recipient(recording) == starter.user + + +def test_summary_recipient_none_without_access(): + """No access: nobody to send the summary to.""" recording = factories.RecordingFactory() assert NotificationService._get_summary_recipient(recording) is None diff --git a/src/backend/core/tests/recording/test_api_recordings_media_auth.py b/src/backend/core/tests/recording/test_api_recordings_media_auth.py index cd9a63882..f828ae9da 100644 --- a/src/backend/core/tests/recording/test_api_recordings_media_auth.py +++ b/src/backend/core/tests/recording/test_api_recordings_media_auth.py @@ -89,9 +89,9 @@ def test_api_recordings_media_auth_no_abilities(): assert response.status_code == 403 -def test_api_recordings_media_auth_wrong_abilities(): +def test_api_recordings_media_auth_member(): """ - Test that users with insufficient role permissions cannot download recordings + Members of a recording (e.g. its starter) are allowed to download it. """ user = UserFactory() @@ -102,13 +102,14 @@ def test_api_recordings_media_auth_wrong_abilities(): UserRecordingAccessFactory(user=user, recording=recording, role="member") - original_url = f"http://localhost/media/recordings/{recording.id!s}.mp4" + original_url = f"http://localhost/media/{recording.key:s}" response = client.get( "/api/v1.0/recordings/media-auth/", HTTP_X_ORIGINAL_URL=original_url ) - assert response.status_code == 403 + assert response.status_code == 200 + assert "AWS4-HMAC-SHA256 Credential=" in response["Authorization"] @pytest.mark.parametrize("wrong_status", ["initiated", "active", "failed_to_stop"]) diff --git a/src/backend/core/tests/recording/test_api_recordings_retrieve.py b/src/backend/core/tests/recording/test_api_recordings_retrieve.py index 9f1867490..c960419e6 100644 --- a/src/backend/core/tests/recording/test_api_recordings_retrieve.py +++ b/src/backend/core/tests/recording/test_api_recordings_retrieve.py @@ -47,7 +47,8 @@ def test_api_recording_retrieve_authenticated(): def test_api_recording_retrieve_members(): """ - A user who is a member of a recording should not be able to retrieve it. + A user who is a member of a recording (e.g. its starter) should be able + to retrieve it. """ user = UserFactory() recording = RecordingFactory() @@ -58,10 +59,8 @@ def test_api_recording_retrieve_members(): client.force_login(user) response = client.get(f"/api/v1.0/recordings/{recording.id!s}/") - assert response.status_code == 403 - assert response.json() == { - "detail": "You do not have permission to perform this action." - } + assert response.status_code == 200 + assert response.json()["id"] == str(recording.id) def test_api_recording_retrieve_administrators(settings): diff --git a/src/backend/core/tests/rooms/test_api_rooms_recording_participants.py b/src/backend/core/tests/rooms/test_api_rooms_recording_participants.py index 8cdaaa53e..02abf886f 100644 --- a/src/backend/core/tests/rooms/test_api_rooms_recording_participants.py +++ b/src/backend/core/tests/rooms/test_api_rooms_recording_participants.py @@ -48,7 +48,7 @@ def enable_recording(settings): def test_open_room_present_user_can_start_recording( mock_check, access_level, mock_worker_manager ): - """Authenticated + present in a trusted/public room -> 201, starter owns it.""" + """Authenticated + present in a trusted/public room -> 201, starter is member.""" user = UserFactory() room = RoomFactory(access_level=access_level) @@ -65,7 +65,7 @@ def test_open_room_present_user_can_start_recording( mock_worker_manager.start.assert_called_once_with(recording) access = recording.accesses.get() assert access.user == user - assert access.role == RoleChoices.OWNER + assert access.role == RoleChoices.MEMBER @mock.patch( @@ -282,7 +282,7 @@ def test_config_exposes_authenticated_participants_setting(settings, enabled): def test_room_owners_and_admins_get_access_to_participant_recording( mock_check, mock_worker_manager ): - """Starter and room owners/admins all own the recording; members get nothing.""" + """Room owners/admins own the recording, the starter is member; room members get nothing.""" starter = UserFactory() room_owner = UserFactory() room_admin = UserFactory() @@ -300,7 +300,7 @@ def test_room_owners_and_admins_get_access_to_participant_recording( assert response.status_code == 201 recording = Recording.objects.get() assert dict(recording.accesses.values_list("user_id", "role")) == { - starter.id: RoleChoices.OWNER, + starter.id: RoleChoices.MEMBER, room_owner.id: RoleChoices.OWNER, room_admin.id: RoleChoices.OWNER, } @@ -378,3 +378,35 @@ def test_summary_goes_to_starter_among_several_owners(mock_check, mock_worker_ma recording = Recording.objects.get() assert recording.accesses.count() == 3 assert NotificationService._get_summary_recipient(recording) == starter + + +@mock.patch( + "core.services.participants_management.ParticipantsManagement.check_if_in_meeting", + return_value=True, +) +def test_participant_starter_can_retrieve_but_not_delete( + mock_check, mock_worker_manager +): + """A starter without a privileged room role can download but not delete.""" + starter = UserFactory() + room_owner = UserFactory() + room = RoomFactory(access_level=RoomAccessLevel.TRUSTED) + room.accesses.create(user=room_owner, role=RoleChoices.OWNER) + client = APIClient() + client.force_login(starter) + + response = client.post( + f"/api/v1.0/rooms/{room.id}/start-recording/", {"mode": "screen_recording"} + ) + assert response.status_code == 201 + recording = Recording.objects.get() + recording.status = RecordingStatusChoices.SAVED + recording.save() + + assert client.get(f"/api/v1.0/recordings/{recording.id}/").status_code == 200 + assert client.delete(f"/api/v1.0/recordings/{recording.id}/").status_code == 403 + assert Recording.objects.filter(id=recording.id).exists() + + client.force_login(room_owner) + assert client.delete(f"/api/v1.0/recordings/{recording.id}/").status_code == 204 + assert not Recording.objects.filter(id=recording.id).exists() diff --git a/src/backend/core/tests/test_models_recording.py b/src/backend/core/tests/test_models_recording.py index 6c7c3e756..fa14f54dd 100644 --- a/src/backend/core/tests/test_models_recording.py +++ b/src/backend/core/tests/test_models_recording.py @@ -91,7 +91,7 @@ def test_models_recording_get_abilities_privileges_active(role): def test_models_recording_get_abilities_member_active(): - """Test abilities for a user who is unprivileged.""" + """A member can retrieve the recording but not stop or delete it.""" user = UserFactory() access = UserRecordingAccessFactory(role="member", user=user) @@ -101,7 +101,7 @@ def test_models_recording_get_abilities_member_active(): assert abilities == { "destroy": False, "partial_update": False, - "retrieve": False, + "retrieve": True, "stop": False, "update": False, } @@ -129,7 +129,7 @@ def test_models_recording_get_abilities_privileges_final(role): def test_models_recording_get_abilities_member_final(): - """Test abilities for a user who is unprivileged.""" + """A member can retrieve the recording but not stop or delete it.""" user = UserFactory() access = UserRecordingAccessFactory(role="member", user=user) @@ -139,7 +139,7 @@ def test_models_recording_get_abilities_member_final(): assert abilities == { "destroy": False, "partial_update": False, - "retrieve": False, + "retrieve": True, "stop": False, "update": False, }