mirror of
https://github.com/suitenumerique/meet.git
synced 2026-10-15 09:23:08 +00:00
🔒️(backend) grant MEMBER access to the recording starter instead of OWNER
In the previous commit, an authenticated participant starting a recording was granted an `OWNER` `RecordingAccess`. @briquet pointed out that this let any starter call the API to delete the recording, and this destructive action should be limited to genuinely privileged users. Agreed. Refactor: an authenticated but non-privileged starter now receives a `MEMBER` `RecordingAccess`, so they can retrieve and download the recording but not delete it. Room owners and administrators still get `OWNER` access, which matches their role on the room. Known follow-up on the docs side: ensure the transcript is shared with all accesses granted on the recording, and that the room's owners and admins are also owners on the resulting document.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"])
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user