From 52c542e11efb717fe86eed05de1a66f124a323e7 Mon Sep 17 00:00:00 2001 From: motey Date: Wed, 28 Oct 2026 13:50:24 +0100 Subject: [PATCH] fix: keep numeric user IDs when merging file activity recipients User IDs are used as array keys for the recipient lists and the notification settings. PHP stores numeric string keys like '55' as integers, and array_merge() renumbers integer keys from 0. With activity_use_cached_mountpoints enabled, file activities on instances with numeric user IDs were written for the users 0, 1, 2, ... instead of the real recipients. The email and notification settings of users who favorited the file were lost the same way. Use array_replace() for these keyed merges and cast the keys back to strings before comparing them with the current user, so the actor gets the "self" subject on rename and move. Fixes https://github.com/nextcloud/activity/issues/3071 Signed-off-by: motey --- lib/FilesHooks.php | 18 ++-- tests/FilesHooksTest.php | 198 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+), 6 deletions(-) diff --git a/lib/FilesHooks.php b/lib/FilesHooks.php index 62859f6a5..11d03bd63 100644 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -128,8 +128,8 @@ private function getFileChangeActivitySettings(int $fileId, array $users, string if (!empty($favoriteUsers)) { $favoriteUsers = array_intersect($users, $favoriteUsers); if (!empty($favoriteUsers)) { - $filteredEmailUsers = array_merge($filteredEmailUsers, $this->userSettings->filterUsersBySetting($favoriteUsers, 'email', Files::TYPE_FAVORITE_CHANGED)); - $filteredNotificationUsers = array_merge($filteredNotificationUsers, $this->userSettings->filterUsersBySetting($favoriteUsers, 'notification', Files::TYPE_FAVORITE_CHANGED)); + $filteredEmailUsers = array_replace($filteredEmailUsers, $this->userSettings->filterUsersBySetting($favoriteUsers, 'email', Files::TYPE_FAVORITE_CHANGED)); + $filteredNotificationUsers = array_replace($filteredNotificationUsers, $this->userSettings->filterUsersBySetting($favoriteUsers, 'notification', Files::TYPE_FAVORITE_CHANGED)); } } @@ -166,7 +166,7 @@ protected function addNotificationsForFileAction(Node $node, string $activityTyp // file can be shared using GroupFolders, including ACL check if ($this->config->getSystemValueBool('activity_use_cached_mountpoints', false)) { - $affectedUsers = array_merge($affectedUsers, $this->getAffectedUsersFromCachedMounts($fileId)); + $affectedUsers = array_replace($affectedUsers, $this->getAffectedUsersFromCachedMounts($fileId)); } [$filteredEmailUsers, $filteredNotificationUsers] @@ -301,7 +301,7 @@ public function fileMove(Node $source, Node $target): void { // file can be shared using GroupFolders, including ACL check if ($this->config->getSystemValueBool('activity_use_cached_mountpoints', false)) { [, , $oldFileId] = $this->getSourcePathAndOwner($source); - $oldAccessList['users'] = array_merge($oldAccessList['users'], $this->getAffectedUsersFromCachedMounts($oldFileId)); + $oldAccessList['users'] = array_replace($oldAccessList['users'], $this->getAffectedUsersFromCachedMounts($oldFileId)); } $this->oldAccessList = $oldAccessList; @@ -374,7 +374,7 @@ protected function fileRenaming(Node $source, Node $target): void { // file can be shared using GroupFolders, including ACL check if ($this->config->getSystemValueBool('activity_use_cached_mountpoints', false)) { - $affectedUsers = array_merge($affectedUsers, $this->getAffectedUsersFromCachedMounts($fileId)); + $affectedUsers = array_replace($affectedUsers, $this->getAffectedUsersFromCachedMounts($fileId)); } [$filteredEmailUsers, $filteredNotificationUsers] = $this->getFileChangeActivitySettings($fileId, array_keys($affectedUsers)); @@ -382,6 +382,8 @@ protected function fileRenaming(Node $source, Node $target): void { $shouldFlush = $this->startActivityTransaction(); try { foreach ($affectedUsers as $user => $path) { + $user = (string)$user; + if ($user === $this->currentUser->getUID()) { $userSubject = 'renamed_self'; $userParams = [ @@ -443,7 +445,7 @@ protected function fileMoving(Node $source, Node $target): void { // file can be shared using GroupFolders, including ACL check if ($this->config->getSystemValueBool('activity_use_cached_mountpoints', false)) { $this->userMountCache->clear(); // clear cache for new data - $affectedUsers = array_merge($affectedUsers, $this->getAffectedUsersFromCachedMounts($fileId)); + $affectedUsers = array_replace($affectedUsers, $this->getAffectedUsersFromCachedMounts($fileId)); } $beforeUsers = array_keys($oldUsers); @@ -509,6 +511,7 @@ protected function generateDeleteActivities($users, array $pathMap, $fileId, $ol $shouldFlush = $this->startActivityTransaction(); try { foreach ($users as $user) { + $user = (string)$user; $path = $pathMap[$user]; if ($user === $this->currentUser->getUID()) { @@ -548,6 +551,7 @@ protected function generateAddActivities($users, array $pathMap, $fileId, $fileN $shouldFlush = $this->startActivityTransaction(); try { foreach ($users as $user) { + $user = (string)$user; $path = $pathMap[$user]; if ($user === $this->currentUser->getUID()) { @@ -590,6 +594,8 @@ protected function generateMoveActivities($users, array $beforePathMap, array $a $shouldFlush = $this->startActivityTransaction(); try { foreach ($users as $user) { + $user = (string)$user; + if ($oldFileName === $fileName) { $userParams = [[$newParentId => $afterPathMap[$user] . '/']]; } else { diff --git a/tests/FilesHooksTest.php b/tests/FilesHooksTest.php index 570a22f95..f939b20e5 100644 --- a/tests/FilesHooksTest.php +++ b/tests/FilesHooksTest.php @@ -173,6 +173,19 @@ protected function getUserMock(string $uid): IUser { return $user; } + protected function getCachedMountFileInfo(string $uid): CachedMountFileInfo { + return new CachedMountFileInfo( + $this->getUserMock($uid), + 1, + 1, + '/' . $uid . '/files', + null, + '', + 'path', + 'files/path' + ); + } + public static function dataFileCreate(): array { return [ ['user', false, 'created_self', 'created_by', Files::TYPE_SHARE_CREATED], @@ -499,6 +512,191 @@ public function testAddNotificationsForFileAction(array $filterUsers, bool $moun $this->assertEquals($addCalls, array_slice($receivedActivities, 0, count($addCalls))); } + public static function dataAddNotificationsForFileActionNumericUserIds(): array { + return [ + 'access list only' => [ + false, + [ + ['56', 'created_self', false, false], + ['55', 'created_by', 20, true], + ], + ], + 'access list and cached mounts' => [ + true, + [ + ['56', 'created_self', false, false], + ['55', 'created_by', 20, true], + ['232', 'created_by', 30, true], + ], + ], + ]; + } + + /** + * Numeric user IDs become integer array keys, they must not be renumbered + * when the recipient lists and the notification settings are merged. + */ + #[DataProvider('dataAddNotificationsForFileActionNumericUserIds')] + public function testAddNotificationsForFileActionNumericUserIds(bool $mountCacheUsed, array $expected): void { + // User 55 favorited the file, so their settings are merged from two lookups + $this->tagManager = $this->createMock(TagManager::class); + $this->tagManager->method('getUsersFavoritingObject') + ->willReturn(['55']); + $filesHooks = $this->getFilesHooks([ + 'getSourcePathAndOwner', + 'getUserPathsFromPath', + 'addNotificationsForUser', + ], '56'); + + $filesHooks->method('getSourcePathAndOwner') + ->willReturn(['/path', '56', 1337]); + $filesHooks->method('getUserPathsFromPath') + ->willReturn([ + 'ownerPath' => '/path', + 'users' => ['56' => '/path', '55' => '/path'], + 'remotes' => [], + ]); + + $this->config->expects($this->once()) + ->method('getSystemValueBool') + ->with('activity_use_cached_mountpoints', false) + ->willReturn($mountCacheUsed); + $this->userMountCache->method('getMountsForFileId') + ->willReturn([ + $this->getCachedMountFileInfo('56'), + $this->getCachedMountFileInfo('232'), + ]); + + $settings = [ + 'email' => ['55' => 20, '232' => 30], + 'notification' => ['232' => true], + ]; + $favoriteSettings = [ + 'email' => [], + 'notification' => ['55' => true], + ]; + $this->settings->method('filterUsersBySetting') + ->willReturnCallback(fn (array $users, string $method, string $type): array => array_intersect_key( + $type === Files::TYPE_FAVORITE_CHANGED ? $favoriteSettings[$method] : $settings[$method], + array_flip($users), + )); + + $received = []; + $filesHooks->method('addNotificationsForUser') + ->willReturnCallback(function ($user, $subject, $subjectParams, $fileId, $path, $isFile, $email, $notification) use (&$received): void { + $received[] = [$user, $subject, $email, $notification]; + }); + + self::invokePrivate($filesHooks, 'addNotificationsForFileAction', [$this->getNodeMock(1337, '/56/files/path'), Files::TYPE_SHARE_CREATED, 'created_self', 'created_by']); + + $this->assertSame($expected, $received); + } + + public function testFileRenamingNumericUserIdsWithMountCache(): void { + $filesHooks = $this->getFilesHooks([ + 'getSourcePathAndOwner', + 'getUserPathsFromPath', + 'addNotificationsForUser', + ], '56'); + + $filesHooks->method('getSourcePathAndOwner') + ->willReturn(['/folder', '56', 42]); + $filesHooks->method('getUserPathsFromPath') + ->willReturn([ + 'users' => ['56' => '/folder', '55' => '/folder'], + 'remotes' => [], + ]); + + $this->config->expects($this->once()) + ->method('getSystemValueBool') + ->with('activity_use_cached_mountpoints', false) + ->willReturn(true); + $this->userMountCache->method('getMountsForFileId') + ->willReturn([ + $this->getCachedMountFileInfo('56'), + $this->getCachedMountFileInfo('232'), + ]); + $this->settings->method('filterUsersBySetting') + ->willReturn([]); + + $received = []; + $filesHooks->method('addNotificationsForUser') + ->willReturnCallback(function ($user, $subject) use (&$received): void { + $received[] = [$user, $subject]; + }); + + $target = $this->getNodeMock(42, '/56/files/folder/new.txt'); + $target->method('getParent') + ->willReturn($this->getNodeMock(23, '/56/files/folder', false)); + + self::invokePrivate($filesHooks, 'fileRenaming', [$this->getNodeMock(42, '/56/files/folder/old.txt'), $target]); + + $this->assertSame([ + ['56', 'renamed_self'], + ['55', 'renamed_by'], + ['232', 'renamed_by'], + ], $received); + } + + public function testFileMoveNumericUserIdsWithMountCache(): void { + $filesHooks = $this->getFilesHooks([ + 'getSourcePathAndOwner', + 'getUserPathsFromPath', + 'addNotificationsForUser', + ], '56'); + + $sourceParent = $this->getNodeMock(10, '/56/files/a', false); + $targetParent = $this->getNodeMock(11, '/56/files/b', false); + $source = $this->getNodeMock(42, '/56/files/a/file.txt'); + $source->method('getParent') + ->willReturn($sourceParent); + $target = $this->getNodeMock(42, '/56/files/b/file.txt'); + $target->method('getParent') + ->willReturn($targetParent); + + $filesHooks->method('getSourcePathAndOwner') + ->willReturnCallback(fn (Node $node): array => match ($node) { + $sourceParent => ['/a', '56', 10], + $targetParent => ['/b', '56', 11], + $source => ['/a/file.txt', '56', 42], + $target => ['/b/file.txt', '56', 42], + }); + $filesHooks->method('getUserPathsFromPath') + ->willReturnCallback(fn (string $path): array => [ + 'users' => $path === '/a' ? ['56' => '/a', '55' => '/a'] : ['56' => '/b'], + 'remotes' => [], + ]); + + $this->config->method('getSystemValueBool') + ->with('activity_use_cached_mountpoints', false) + ->willReturn(true); + $this->userMountCache->expects($this->exactly(2)) + ->method('getMountsForFileId') + ->with(42) + ->willReturnOnConsecutiveCalls( + [$this->getCachedMountFileInfo('232')], + [$this->getCachedMountFileInfo('77')], + ); + $this->settings->method('filterUsersBySetting') + ->willReturn([]); + + $received = []; + $filesHooks->method('addNotificationsForUser') + ->willReturnCallback(function ($user, $subject) use (&$received): void { + $received[] = [$user, $subject]; + }); + + $filesHooks->fileMove($source, $target); + $filesHooks->fileMovePost($source, $target); + + $this->assertSame([ + ['55', 'deleted_by'], + ['232', 'deleted_by'], + ['77', 'created_by'], + ['56', 'moved_self'], + ], $received); + } + public function testFileMoveCollectsOldAccessList(): void { $filesHooks = $this->getFilesHooks([ 'getSourcePathAndOwner',