Skip to content

feat: let apps define the actor of an activity via the activity manager - #2889

Open
bakiburakogun wants to merge 1 commit into
nextcloud:masterfrom
bakiburakogun:feat/current-user-honours-activity-manager
Open

feat: let apps define the actor of an activity via the activity manager#2889
bakiburakogun wants to merge 1 commit into
nextcloud:masterfrom
bakiburakogun:feat/current-user-honours-activity-manager

Conversation

@bakiburakogun

Copy link
Copy Markdown

Problem

CurrentUser::getUID() reads the user straight from the session:

public function getUID(): ?string {
	$user = $this->userSession->getUser();
	if ($user instanceof IUser) {
		return $user->getUID();
	}
	return null;
}

FilesHooks asks CurrentUser for 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, and OC\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:

  • when a user is logged in, Manager::getCurrentUserId() returns that session user, which is what this method returned before;
  • when there is neither a session nor a valid feed token it throws \UnexpectedValueException, which is caught here so the previous null is 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 returned null and getUserIdentifier() 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:

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 […] I think CurrentUser should first check IManager::getCurrentUserId() that would allow Talk to overwrite the user for the action

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 CurrentUser tests pass unchanged — an unstubbed getCurrentUserId() returns '' from the mock, which falls through to the session lookup.

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>
@nickvergessen

Copy link
Copy Markdown
Member

From my perspective this is what Talk needs to be able to overwrite the "remote user" with the current user.
Not sure if possible, but if Talk would be able to influence this part even more so it could refer to the recording backend explicitely that'd be even better, but not sure how we could do that. So leaving the approval to @miaulalala and @artonge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants