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

[bakiburakogun]

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

Read the actor from the activity manager instead of the session:

public function getUID(): ?string {
	try {
		$userId = $this->activityManager->getCurrentUserId();
	} catch (\UnexpectedValueException) {
		// No override, no session and no valid feed token
		return null;
	}

	return $userId === '' ? null : $userId;
}

Manager::getCurrentUserId() returns the override when an app set one, otherwise the session user, and only when there is neither a session nor a valid feed token does it throw, which is caught here so null is still returned.

Against master:

	master	this PR
session, no override	alice	alice
override set	alice	bob
no session, no token	null	null
no session, valid feed token	null	carol
session user disabled	dave	null
override set to ''	alice	null

The last three rows are the differences. The feed-token one attributes such a request to the token owner, where the method previously returned null and getUserIdentifier() fell through to the cloud id or the nickname header. The disabled-account one follows from Session::isLoggedIn() being false for a disabled user, so the manager goes to the token lookup and throws rather than reading the session; a request from a disabled account should not be producing activities in the first place. The last one only happens when an app passes '' to setCurrentUserId(), and null fits the ?string return better there. Say the word if you would rather keep any of them on the old path.

Motivation

Raised in nextcloud/spreed#19078, where call recordings are stored by a background job and end up attributed to nobody. The first attempt there swapped 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 calls setCurrentUserId() around storing the recording and the session juggling is gone. That PR is a no-op until this one lands.

Testing

The CurrentUser tests are updated along with the change: getUID() no longer touches the session, so the data provider drives the manager and every case asserts the session is never read, with separate cases for an override being set and for the manager throwing.

[nickvergessen]

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

[jospoortvliet]

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

would this also allow filtering activity by Team? It's a challenge we have with the Teams app, where we'd like to show a stream of activity related to team owned resources...

[artonge]

would this also allow filtering activity by Team? It's a challenge we have with the Teams app, where we'd like to show a stream of activity related to team owned resources...

Not related from what I understand

[artonge]

Looks good to me

[artonge]

Looks like getCurrentUserId() already looks in the session, so we can drop that part.

https://github.com/nextcloud/server/blob/c9192dde80aceb58909c4c01c13dcf258139d31d/lib/private/Activity/Manager.php#L365-L376

For consistency, we can also replace all $this->userSession->getUser(); calls in this file with $userId = $this->activityManager->getCurrentUserId();

[bakiburakogun]

Dropped the session part, thank you — the fallback was indeed unreachable for everything that matters.

public function getUID(): ?string {
	try {
		$userId = $this->activityManager->getCurrentUserId();
	} catch (\UnexpectedValueException) {
		// No override, no session and no valid feed token
		return null;
	}

	return $userId === '' ? null : $userId;
}

Two edge cases do come out differently, so rather than leaving them buried in the diff:

	master	this PR before	now
session, no override	alice	alice	alice
override set	alice	bob	bob
no session, no token	null	null	null
no session, valid feed token	null	carol	carol
session user disabled	dave	dave	null
override set to ''	alice	alice	null

The bottom two rows are new with this push. The disabled-account one follows from Session::isLoggedIn() being false for a disabled user, so getCurrentUserId() goes to the token lookup and throws instead of reading the session. Such a request should not be producing activities in the first place, but say the word if you would rather keep the fallback for it. The empty-string one only happens when an app passes '' to setCurrentUserId(), and there null fits the ?string return better than an empty string does.

On the second half of your comment, the remaining $this->userSession->getUser() calls: getCloudId() needs the IUser object rather than the id, so it would mean injecting IUserManager plus a lookup that can come back empty, and it already falls back to getCloudIDFromToken() when there is no session. getUser() hands out the object as well and is used from outside this class. Happy to convert both here if you want that dependency, otherwise I left them alone.

Tests updated along with it: getUID() no longer touches the session, so the provider drives the manager and every case asserts the session is never read.

[bakiburakogun]

@nickvergessen on naming the recording backend rather than the owner: I do not think it can be expressed with the current interface, and here is why.

setCurrentUserId() takes a user id, and that id is what ends up in the author column: FilesHooks calls $event->setAuthor($this->currentUser->getUID()) and Data stores it as 'user' => $event->getAuthor(). Everything downstream reads that column as an account, so passing something that is not one would render as an unknown account rather than as a service name. Giving the recording backend a real account would work, but that is a much larger decision than this PR.

There is a second, softer place where it could live. getUserIdentifier() already produces a non-account label today, for the nickname header:

return $nickname . ' (' . $this->l10nFactory->get('comments')->t('remote user') . ')';

so a similar labelled actor for the subject parameters would not be a new concept, but it would be a separate change to the manager interface and the schema rather than something this PR can carry.

Worth adding for the Talk side: even with a way to name it, Talk could only do so for part of the flow. On the chunked-upload path the recording file is written by the recording backend through the public share created in requestUpload(), in a request Talk takes no part in, so nothing Talk sets reaches that write. What Talk does cover is the recording folder and the direct-upload path, both of which run in its own process.

Separately, and only because it is easy to miss: the workflow runs on this PR sit in "action required" because it comes from a fork, so nothing beyond DCO has run on it. If one of you approves the runs, the test and lint results will show up.