wip #188
wip #188
Conversation
| RECORDING = "recording", _("Recording") | ||
| DONE = "done", _("Done") |
There was a problem hiding this comment.
missing status :
- starting
- aborted
- error
| def get_roles(self, user): | ||
| """Compute the roles a user has in a room.""" | ||
| if not user or not user.is_authenticated: | ||
| return self.accesses.none() | ||
|
|
||
| try: | ||
| roles = self.user_roles or [] | ||
| except AttributeError: | ||
| try: | ||
| roles = self.accesses.filter(user=user).values_list("role", flat=True) | ||
| except (models.ObjectDoesNotExist, IndexError): | ||
| roles = self.accesses.none() | ||
| return roles | ||
|
|
||
| def get_abilities(self, user): | ||
| """Compute and return abilities for a given user on the room.""" | ||
| roles = self.get_roles(user) | ||
| is_owner_or_admin = RoleChoices.is_administrator(roles) | ||
|
|
||
| return { | ||
| "start_recording": is_owner_or_admin, | ||
| "destroy": RoleChoices.OWNER in roles, | ||
| "manage_accesses": is_owner_or_admin, | ||
| "partial_update": is_owner_or_admin, | ||
| "retrieve": True, | ||
| "update": is_owner_or_admin, | ||
| } |
There was a problem hiding this comment.
Mentioning room / adding room-related logic in the Resource model is not appropriate
| def start_recording(self): | ||
| """Create a new related recording object to which Livekit will be able to save a file.""" | ||
| return Recording.objects.create(room=self) |
There was a problem hiding this comment.
the need to enforce a single recording session per room at a time, might be declared somewhere else in the code, I am only starting the review
| room = models.ForeignKey( | ||
| Room, on_delete=models.CASCADE, related_name="meetings", verbose_name=_("Room") | ||
| ) | ||
| stopped_at = models.DateTimeField(verbose_name=_("End Time"), null=True, blank=True) | ||
| status = models.CharField( | ||
| choices=RecordingStatusChoices, default=RecordingStatusChoices.RECORDING | ||
| ) |
There was a problem hiding this comment.
Should we track who initiated the recording for accountability and traceability ?
And also be able to notify this user by email and not all rooms admin/owners in some scenario (eg. your video recording is finished)
| return Recording.objects.create(room=self) | ||
|
|
||
|
|
||
| class Recording(BaseModel): |
There was a problem hiding this comment.
Should we store the type of recording—audio, video, or both—or rely on LiveKit Egress types for classification?
| @property | ||
| def key(self): | ||
| """Return the path where the recording file will be stored in object storage.""" | ||
| return f"recordings/{self.pk!s}/file.mp4/" |
There was a problem hiding this comment.
Recordings won't always be in MP4 format; they could be in OGG or other formats. I love the idea of organizing each recording into its own folder, where we could also include additional files like transcripts, etc. What inspired this design choice?
| def generate_s3_authorization_headers(key): | ||
| """ | ||
| Generate authorization headers for an s3 object. | ||
| These headers can be used as an alternative to signed urls with many benefits: | ||
| - the urls of our files never expire and can be stored in our documents' content | ||
| - we don't leak authorized urls that could be shared (file access can only be done | ||
| with cookies) | ||
| - access control is truly realtime | ||
| - the object storage service does not need to be exposed on internet | ||
| """ | ||
| url = default_storage.unsigned_connection.meta.client.generate_presigned_url( | ||
| "get_object", | ||
| ExpiresIn=0, | ||
| Params={"Bucket": default_storage.bucket_name, "Key": key}, | ||
| ) | ||
| request = botocore.awsrequest.AWSRequest(method="get", url=url) | ||
|
|
||
| s3_client = default_storage.connection.meta.client | ||
| # pylint: disable=protected-access | ||
| credentials = s3_client._request_signer._credentials # noqa: SLF001 | ||
| frozen_credentials = credentials.get_frozen_credentials() | ||
| region = s3_client.meta.region_name | ||
| auth = botocore.auth.S3SigV4Auth(frozen_credentials, "s3", region) | ||
| auth.add_auth(request) | ||
|
|
||
| return request |
There was a problem hiding this comment.
we need to discuss this code
| room = self.get_object() | ||
| recording = room.start_recording() | ||
|
|
There was a problem hiding this comment.
interaction with the livekit client here
| user=self.request.user, | ||
| event="Get Room", | ||
| properties={"slug": instance.slug}, | ||
| ) |
There was a problem hiding this comment.
You refactored the analytics to follow a minimal try-except approach, right?
| permission_classes = [permissions.RoomPermissions] | ||
| permission_classes = [permissions.AccessPermission] |
| return drf_response.Response(serializer.data) | ||
|
|
||
| @decorators.action(detail=True, methods=["post"], url_path="stop") | ||
| def stop(self, request, *args, **kwargs): |
There was a problem hiding this comment.
interaction with the livekit client
| def retrieve_auth(self, request, *args, **kwargs): | ||
| """ | ||
| This view is used by an Nginx subrequest to control access to a recording file. | ||
|
|
||
| The original url is passed by nginx in the "HTTP_X_ORIGINAL_URL" header. | ||
| See corresponding ingress configuration in Helm chart and read about the | ||
| nginx.ingress.kubernetes.io/auth-url annotation to understand how the Nginx ingress | ||
| is configured to do this. | ||
|
|
||
| Based on the original url and the logged in user, we must decide if we authorize Nginx | ||
| to let this request go through (by returning a 200 code) or if we block it (by returning | ||
| a 403 error). Note that we return 403 errors without any further details for security | ||
| reasons. | ||
|
|
||
| When we let the request go through, we compute authorization headers that will be added to | ||
| the request going through thanks to the nginx.ingress.kubernetes.io/auth-response-headers | ||
| annotation. The request will then be proxied to the object storage backend who will | ||
| respond with the file after checking the signature included in headers. | ||
| """ | ||
| if not request.user.is_authenticated: | ||
| raise exceptions.AuthenticationFailed() | ||
|
|
||
| original_url = urlparse(request.META.get("HTTP_X_ORIGINAL_URL")) | ||
| match = RECORDING_URL_PATTERN.search(original_url.path) | ||
|
|
||
| try: | ||
| (pk,) = match.groups() | ||
| except AttributeError as excpt: | ||
| raise exceptions.PermissionDenied() from excpt | ||
|
|
||
| # Check permission | ||
| if not models.Recording.objects.filter( | ||
| pk=pk, room__accesses__user=request.user | ||
| ).exists(): | ||
| raise exceptions.PermissionDenied() | ||
|
|
||
| # Generate authorization headers and return an authorization to proceed with the request | ||
| key = models.Recording(pk=pk).key | ||
| request = utils.generate_s3_authorization_headers(key) | ||
| return drf_response.Response("authorized", headers=request.headers, status=200) |
There was a problem hiding this comment.
I will be more than happy to understand why temporary signed url is not enough ?
| raise exceptions.PermissionDenied() | ||
|
|
||
| recording.stopped_at = timezone.now() | ||
| recording.save() |
There was a problem hiding this comment.
Actually, the egress will save the recording and notify the backend via a webhook. We need a robust security mechanism to ensure the webhook isn’t triggered multiple times for the same recording, preventing duplicate callbacks. This follows more of an event-driven architecture.
| return { | ||
| "destroy": RoleChoices.OWNER in roles, | ||
| "partial_update": False, | ||
| "retrieve": False, |
There was a problem hiding this comment.
why a recording cannot be retrieved ?
Purpose
Description...
Proposal
Description...