fix(recording): store recordings and transcripts as the owning user - #19078
fix(recording): store recordings and transcripts as the owning user#19078bakiburakogun wants to merge 1 commit into
Conversation
|
Deployed on the instance from the report (Nextcloud 34.0.3, Talk 24.0.4, nextcloud-talk-recording 0.2.1) and recorded a call with it. The activity entry for the new recording now reads |
|
Pushed a follow-up commit. After running the first version on the instance from the report, the recordings themselves are attributed correctly, but the activity stream still shows an entry like for the first recording of a conversation. The reason is that The new commit moves the user switch into a small |
|
Verified on the instance from the report (Nextcloud 34.0.3, Talk 24.0.4, nextcloud-talk-recording 0.2.1, four signaling servers, seven application servers). Recorded a call in a conversation that had never been recorded before, so the per-conversation folder was created as part of storing the recording. The activity stream now attributes both the folder and the Recording, the "recording available" notification and sharing it into the chat behave as before. |
| } | ||
|
|
||
| $previousUser = $this->userSession->getUser(); | ||
| $this->userSession->setUser($user); |
There was a problem hiding this comment.
I'm not sure it's the best idea to overwrite the session user like this.
Also it's not going to work on the new chunked-uploading of recordings we implemented recently.
I think instead we need work in the activity app to allow defining different actors:
https://github.com/nextcloud/activity/blob/f95186c14cb89cb8e9f7f367e9ac394b2c398c50/lib/CurrentUser.php#L35
The activity manager has code for this already: https://github.com/nextcloud/server/blob/1ab09ec753f106661beadee1794b390ebec455dd/lib/private/Activity/Manager.php#L346-L354
but it's not used in the activity app itself. I think CurrentUser should first check IManager::getCurrentUserId() that would allow Talk to overwrite the user for the action
Recordings and transcripts are written by a background job, outside of any session, so the folder and the file end up attributed to nobody and the activity stream renders them as "remote account" created … . Set the actor through the activity manager for the operations that create nodes, rather than swapping the session user around them: the session swap does not survive into the chunked upload, which happens in a separate request against a public share, and overwriting the session for unrelated code running in the same process is not something this service should do. This depends on nextcloud/activity#2889, which makes the activity app consult IManager::getCurrentUserId(). Without it the call here is a no-op and the behaviour is unchanged. The file created by the chunked upload itself is not covered: it is uploaded by the recording backend through the public share created in requestUpload(), in a request Talk does not take part in. The recording folder created for that upload is attributed correctly, as is everything on the direct-upload and transcript paths. Signed-off-by: Baki Burak Öğün <63836730+bakiburakogun@users.noreply.github.com>
e033e8e to
6b093d8
Compare
|
Reworked along the lines you suggested — thank you, the session swap was the wrong tool and the chunked upload made that obvious. What changed The service no longer touches private function runAsActor(string $userId, callable $callback) {
$this->activityManager->setCurrentUserId($userId);
try {
return $callback();
} finally {
$this->activityManager->setCurrentUserId(null);
}
}and wraps the operations that create nodes with it: the recording folder (which covers all four call sites — The branch is rebased onto current Dependency This needs nextcloud/activity#2889, which makes The chunked upload You were right that the previous approach could not work there, and I want to be explicit that this one only partly does. The file created by a chunked upload is written by the recording backend through the public share created in Making the uploaded file itself land on the owner would mean doing something about the public-share upload — either attributing it at share level or re-attributing in |
Fixes #19077
Summary
Call recordings are stored through
POST /ocs/v2.php/apps/spreed/api/v1/recording/{token}/store, which is a#[PublicPage]endpoint authenticated with the recording shared secret only. There is no active user during that request, so whenRecordingService::store()writes the file the activity subsystem cannot resolve an actor and falls back to an empty one. The Files activity provider renders that as"remote account", so users seeinstead of their own name, even though the file is in their own recording folder.
This sets the owner as active user while the file is created and restores the previous user afterwards, so the file events and the resulting activity are attributed to them. The same helper is used for the transcript and summary files written in
storeTranscript(), which have the same problem.Notes
ownerparameter that the recording backend sends is already validated instore()(getParticipant()throwsowner_participantfor an unknown owner), the helper additionally falls back to the previous behaviour if the user cannot be loaded.finallyblock, so a failing write cannot leak the session user into the rest of the request.Testing
Reproduced on Nextcloud 34.0.3 with Talk 24.0.4 and nextcloud-talk-recording 0.2.1 (four node signaling cluster). The patch is being rolled out on that instance now and I will confirm here once a recording has been stored with it.