feat: let apps define the actor of an activity via the activity manager - #2889
Open
bakiburakogun wants to merge 1 commit into
Open
feat: let apps define the actor of an activity via the activity manager#2889bakiburakogun wants to merge 1 commit into
bakiburakogun wants to merge 1 commit into
Conversation
CurrentUser::getUID() read the user straight from the session, so an action performed outside of a user's session could not be attributed to anybody. File activities created from a background job end up rendered as "remote account" did something, because FilesHooks asks CurrentUser for the actor. IManager::setCurrentUserId() already exists for exactly this, and OC\Activity\Manager::getCurrentUserId() honours it, but the activity app never consulted it. Ask the activity manager first and fall back to the session when it has nothing to offer. With no override the result is unchanged: the manager itself returns the session user when one is logged in, and when there is neither a session nor a valid feed token it throws, which is caught here so the previous behaviour of returning null is preserved. Signed-off-by: Baki Burak Öğün <63836730+bakiburakogun@users.noreply.github.com>
bakiburakogun
added a commit
to bakiburakogun/spreed
that referenced
this pull request
Aug 29, 2026
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>
Member
|
From my perspective this is what Talk needs to be able to overwrite the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CurrentUser::getUID()reads the user straight from the session:FilesHooksasksCurrentUserfor the actor of a file activity, so anything an app does on a user's behalf outside of their session — from a background job, from a webhook — cannot be attributed to them. The activity stream renders it as"remote account" created ….IManager::setCurrentUserId()exists for exactly this case, andOC\Activity\Manager::getCurrentUserId()honours it, but the activity app never consults it. There is currently no way for an app to name the actor.Change
Ask the activity manager first, and fall back to the session when it has nothing to offer.
Without an override nothing changes:
Manager::getCurrentUserId()returns that session user, which is what this method returned before;\UnexpectedValueException, which is caught here so the previousnullis still returned.The one behavioural difference is a request authenticated with an activity feed token:
getCurrentUserId()resolves it to the owning user, where this method previously returnednullandgetUserIdentifier()fell through to the cloud id or the nickname header. Attributing those to the token owner looks more correct to me, but say the word if you would rather keep them going through the fallback chain — restricting the new lookup to the case where the session has no user would do it.Motivation
Raised in nextcloud/spreed#19078, where call recordings are stored by a background job and end up attributed to nobody. The current workaround there is to swap the session user around the file write, which @nickvergessen rightly pushed back on:
With this in place Talk can call
setCurrentUserId()around storing the recording, and the follow-up on the Talk side drops the session juggling entirely.Testing
Two unit tests added: one asserting the override is used and the session is not consulted, one asserting the fallback to the session when the manager throws. The existing
CurrentUsertests pass unchanged — an unstubbedgetCurrentUserId()returns''from the mock, which falls through to the session lookup.