diff --git a/lib/FilesHooks.php b/lib/FilesHooks.php index 0c910e3c6..cf97b0241 100644 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -8,7 +8,6 @@ namespace OCA\Activity; -use OC\Files\Filesystem; use OCA\Activity\BackgroundJob\RemoteActivity; use OCA\Activity\Extension\Files; use OCA\Activity\Extension\Files_Sharing; @@ -48,7 +47,7 @@ class FilesHooks { protected $oldParentPath; /** @var string */ protected $oldParentOwner; - /** @var string */ + /** @var int */ protected $oldParentId; public function __construct( @@ -76,15 +75,14 @@ public function __construct( * @param Node $node The node that has been created */ public function fileCreate(Node $node): void { - $path = $this->getVisiblePath($node->getPath()); - if ($path === '/') { + if ($this->getVisiblePath($node->getPath()) === '/') { return; } if ($this->currentUser->getUserIdentifier() === '' && $this->currentUser->isPublicShareToken()) { - $this->addNotificationsForFileAction($path, Files_Sharing::TYPE_PUBLIC_UPLOAD, '', 'created_public'); + $this->addNotificationsForFileAction($node, Files_Sharing::TYPE_PUBLIC_UPLOAD, '', 'created_public'); } else { - $this->addNotificationsForFileAction($path, Files::TYPE_SHARE_CREATED, 'created_self', 'created_by'); + $this->addNotificationsForFileAction($node, Files::TYPE_SHARE_CREATED, 'created_self', 'created_by'); } } @@ -94,7 +92,7 @@ public function fileCreate(Node $node): void { * @param Node $node The node that has been modified */ public function fileUpdate(Node $node): void { - $this->addNotificationsForFileAction($this->getVisiblePath($node->getPath()), Files::TYPE_FILE_CHANGED, 'changed_self', 'changed_by'); + $this->addNotificationsForFileAction($node, Files::TYPE_FILE_CHANGED, 'changed_self', 'changed_by'); } /** @@ -103,7 +101,7 @@ public function fileUpdate(Node $node): void { * @param Node $node The node that is about to be deleted */ public function fileDelete(Node $node): void { - $this->addNotificationsForFileAction($this->getVisiblePath($node->getPath()), Files::TYPE_SHARE_DELETED, 'deleted_self', 'deleted_by'); + $this->addNotificationsForFileAction($node, Files::TYPE_SHARE_DELETED, 'deleted_self', 'deleted_by'); } /** @@ -112,7 +110,7 @@ public function fileDelete(Node $node): void { * @param Node $node The node that has been restored */ public function fileRestore(Node $node): void { - $this->addNotificationsForFileAction($this->getVisiblePath($node->getPath()), Files::TYPE_SHARE_RESTORED, 'restored_self', 'restored_by'); + $this->addNotificationsForFileAction($node, Files::TYPE_SHARE_RESTORED, 'restored_self', 'restored_by'); } private function getFileChangeActivitySettings(int $fileId, array $users, string $type = Files::TYPE_FILE_CHANGED): array { @@ -133,20 +131,20 @@ private function getFileChangeActivitySettings(int $fileId, array $users, string } /** - * Creates the entries for file actions on $file_path + * Creates the entries for file actions on $node * - * @param string $filePath The file that is being changed + * @param Node $node The node that is being changed * @param string $activityType The activity type * @param string $subject The subject for the actor * @param string $subjectBy The subject for other users (with "by $actor") */ - protected function addNotificationsForFileAction($filePath, $activityType, $subject, $subjectBy) { + protected function addNotificationsForFileAction(Node $node, string $activityType, string $subject, string $subjectBy): void { // Do not add activities for .part-files - if (str_ends_with($filePath, '.part')) { + if (str_ends_with($node->getName(), '.part')) { return; } - [$filePath, $uidOwner, $fileId] = $this->getSourcePathAndOwner($filePath); + [$filePath, $uidOwner, $fileId] = $this->getSourcePathAndOwner($node); if ($fileId === 0) { // Could not find the file for the owner ... return; @@ -278,7 +276,7 @@ public function fileMove(Node $source, Node $target): void { } try { - [$this->oldParentPath, $this->oldParentOwner, $this->oldParentId] = $this->getSourcePathAndOwner($oldDir); + [$this->oldParentPath, $this->oldParentOwner, $this->oldParentId] = $this->getSourcePathAndOwner($source->getParent()); if ($this->oldParentId === 0) { // Could not find the file for the owner ... $this->moveCase = false; @@ -289,7 +287,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($oldPath); + [, , $oldFileId] = $this->getSourcePathAndOwner($source); $oldAccessList['users'] = array_merge($oldAccessList['users'], $this->getAffectedUsersFromCachedMounts($oldFileId)); } @@ -315,17 +313,14 @@ public function fileMovePost(Node $source, Node $target): void { return; } - $oldPath = $this->getVisiblePath($source->getPath()); - $newPath = $this->getVisiblePath($target->getPath()); - switch ($this->moveCase) { case 'rename': - $this->fileRenaming($oldPath, $newPath); + $this->fileRenaming($source, $target); break; case 'moveUp': case 'moveDown': case 'moveCross': - $this->fileMoving($oldPath, $newPath); + $this->fileMoving($source, $target); break; } @@ -335,16 +330,17 @@ public function fileMovePost(Node $source, Node $target): void { /** * Renaming a file inside the same folder (a/b to a/c) * - * @param string $oldPath - * @param string $newPath + * @param Node $source The node at its old location + * @param Node $target The node that has been renamed */ - protected function fileRenaming($oldPath, $newPath) { - $dirName = dirname($newPath); - $fileName = basename($newPath); - $oldFileName = basename($oldPath); + protected function fileRenaming(Node $source, Node $target): void { + $oldPath = $this->getVisiblePath($source->getPath()); + $newPath = $this->getVisiblePath($target->getPath()); + $fileName = $target->getName(); + $oldFileName = $source->getName(); - [, , $fileId] = $this->getSourcePathAndOwner($newPath); - [$parentPath, $parentOwner, $parentId] = $this->getSourcePathAndOwner($dirName); + [, , $fileId] = $this->getSourcePathAndOwner($target); + [$parentPath, $parentOwner, $parentId] = $this->getSourcePathAndOwner($target->getParent()); if ($fileId === 0 || $parentId === 0) { // Could not find the file for the owner ... return; @@ -399,22 +395,23 @@ protected function fileRenaming($oldPath, $newPath) { /** * Moving a file from one folder to another * - * @param string $oldPath - * @param string $newPath + * @param Node $source The node at its old location + * @param Node $target The node that has been moved */ - protected function fileMoving($oldPath, $newPath) { + protected function fileMoving(Node $source, Node $target): void { if (!is_array($this->oldAccessList)) { // fileMove() could not collect the old access list, so there is no // base to compute the activities from return; } - $dirName = dirname($newPath); - $fileName = basename($newPath); - $oldFileName = basename($oldPath); + $oldPath = $this->getVisiblePath($source->getPath()); + $newPath = $this->getVisiblePath($target->getPath()); + $fileName = $target->getName(); + $oldFileName = $source->getName(); - [, , $fileId] = $this->getSourcePathAndOwner($newPath); - [$parentPath, $parentOwner, $parentId] = $this->getSourcePathAndOwner($dirName); + [, , $fileId] = $this->getSourcePathAndOwner($target); + [$parentPath, $parentOwner, $parentId] = $this->getSourcePathAndOwner($target->getParent()); if ($fileId === 0 || $parentId === 0) { // Could not find the file for the owner ... return; @@ -642,45 +639,34 @@ protected function getVisiblePath(string $absolutePath): string { } /** - * Return the source + * Return the path relative to the owner's files folder, the owner and the file id of a node * - * @param string $path + * The file id is 0 when the node does not exist. + * + * @return array{0: string, 1: string, 2: int} + * @throws NotFoundException */ - protected function getSourcePathAndOwner($path): array { - $view = Filesystem::getView(); + protected function getSourcePathAndOwner(Node $node): array { try { - $owner = $view->getOwner($path); - $owner = $owner === '' ? null : $owner; + $owner = $node->getOwner()?->getUID(); + $fileId = $node->getId(); } catch (NotFoundException) { $owner = null; + $fileId = 0; } - $fileId = 0; - $currentUser = $this->currentUser->getUID(); - - if ($owner === null || $owner !== $currentUser) { - /** @var \OCP\Files\Storage\IStorage $storage */ - [$storage,] = $view->resolvePath($path); - if ($owner !== null && !$storage->instanceOfStorage(\OCA\Files_Sharing\External\Storage::class)) { - Filesystem::initMountPoints($owner); - } else { - // Probably a remote user, let's try to at least generate activities - // for the current user - if ($currentUser === null) { - [, $owner,] = explode('/', $view->getAbsolutePath($path), 3); - } else { - $owner = $currentUser; - } - } + if ($owner === null || $node->getStorage()->instanceOfStorage(\OCA\Files_Sharing\External\Storage::class)) { + // Probably a remote user, let's try to at least generate activities + // for the current user + [, $owner,] = explode('/', $node->getPath(), 3); + $owner = $this->currentUser->getUID() ?? $owner; } - $info = Filesystem::getFileInfo($path); - if ($info !== false) { - $fileId = (int)$info['fileid']; - $path = $this->getOwnerPathById($owner, $fileId); + if ($fileId === 0) { + return [$this->getVisiblePath($node->getPath()), $owner, 0]; } - return [$path, $owner, $fileId]; + return [$this->getOwnerPathById($owner, $fileId), $owner, $fileId]; } /** diff --git a/psalm.xml b/psalm.xml index ab7974dba..7882e8b5c 100644 --- a/psalm.xml +++ b/psalm.xml @@ -27,7 +27,6 @@ - diff --git a/tests/FilesHooksTest.php b/tests/FilesHooksTest.php index 890634dd9..a90a70df4 100644 --- a/tests/FilesHooksTest.php +++ b/tests/FilesHooksTest.php @@ -40,6 +40,7 @@ use OCP\Files\IUserFolder; use OCP\Files\Node; use OCP\Files\NotFoundException; +use OCP\Files\Storage\IStorage; use OCP\IConfig; use OCP\IDBConnection; use OCP\IGroup; @@ -101,14 +102,14 @@ protected function setUp(): void { $this->filesHooks = $this->getFilesHooks(); } - protected function getFilesHooks(array $mockedMethods = [], string $user = 'user'): FilesHooks { + protected function getFilesHooks(array $mockedMethods = [], ?string $user = 'user'): FilesHooks { $currentUser = $this->createMock(CurrentUser::class); $currentUser ->method('getUID') ->willReturn($user); $currentUser ->method('getUserIdentifier') - ->willReturn($user); + ->willReturn($user ?? ''); /** @var LoggerInterface $logger */ $logger = $this->createMock(LoggerInterface::class); @@ -202,11 +203,12 @@ public function testFileCreate(string $currentUser, bool $isPublicShare, string ->onlyMethods(['addNotificationsForFileAction']) ->getMock(); + $node = $this->getNodeMock(42, '/user/files/path'); $filesHooks->expects($this->once()) ->method('addNotificationsForFileAction') - ->with('/path', $type, $selfSubject, $othersSubject); + ->with($node, $type, $selfSubject, $othersSubject); - $filesHooks->fileCreate($this->getNodeMock(42, '/user/files/path')); + $filesHooks->fileCreate($node); } public static function dataFileCreateUser(): array { @@ -233,11 +235,12 @@ public function testFileUpdate(): void { 'addNotificationsForFileAction', ]); + $node = $this->getNodeMock(42, '/user/files/path'); $filesHooks->expects($this->once()) ->method('addNotificationsForFileAction') - ->with('/path', Files::TYPE_FILE_CHANGED, 'changed_self', 'changed_by'); + ->with($node, Files::TYPE_FILE_CHANGED, 'changed_self', 'changed_by'); - $filesHooks->fileUpdate($this->getNodeMock(42, '/user/files/path')); + $filesHooks->fileUpdate($node); } public function testFileDelete(): void { @@ -245,11 +248,12 @@ public function testFileDelete(): void { 'addNotificationsForFileAction', ]); + $node = $this->getNodeMock(42, '/user/files/path'); $filesHooks->expects($this->once()) ->method('addNotificationsForFileAction') - ->with('/path', Files::TYPE_SHARE_DELETED, 'deleted_self', 'deleted_by'); + ->with($node, Files::TYPE_SHARE_DELETED, 'deleted_self', 'deleted_by'); - $filesHooks->fileDelete($this->getNodeMock(42, '/user/files/path')); + $filesHooks->fileDelete($node); } public function testFileRestore(): void { @@ -257,11 +261,12 @@ public function testFileRestore(): void { 'addNotificationsForFileAction', ]); + $node = $this->getNodeMock(42, '/user/files/folder/file.txt'); $filesHooks->expects($this->once()) ->method('addNotificationsForFileAction') - ->with('/folder/file.txt', Files::TYPE_SHARE_RESTORED, 'restored_self', 'restored_by'); + ->with($node, Files::TYPE_SHARE_RESTORED, 'restored_self', 'restored_by'); - $filesHooks->fileRestore($this->getNodeMock(42, '/user/files/folder/file.txt')); + $filesHooks->fileRestore($node); } public function testAddNotificationsForFileActionPartFile(): void { @@ -272,7 +277,7 @@ public function testAddNotificationsForFileActionPartFile(): void { $filesHooks->expects($this->never()) ->method('getSourcePathAndOwner'); - self::invokePrivate($filesHooks, 'addNotificationsForFileAction', ['/test.txt.part', '', '', '']); + self::invokePrivate($filesHooks, 'addNotificationsForFileAction', [$this->getNodeMock(42, '/user/files/test.txt.part'), '', '', '']); } public static function dataAddNotificationsForFileAction(): array { @@ -389,9 +394,10 @@ public function testAddNotificationsForFileAction(array $filterUsers, bool $moun ->willReturn(['user', 'user1', 'user2']); } + $node = $this->getNodeMock(1337, '/user/files/path'); $filesHooks->expects($this->once()) ->method('getSourcePathAndOwner') - ->with('path') + ->with($node) ->willReturn(['/owner/path', 'owner', 1337]); $filesHooks->expects($this->once()) ->method('getUserPathsFromPath') @@ -476,7 +482,7 @@ public function testAddNotificationsForFileAction(array $filterUsers, bool $moun $receivedActivities[] = $params; }); - self::invokePrivate($filesHooks, 'addNotificationsForFileAction', ['path', Files::TYPE_SHARE_RESTORED, 'restored_self', 'restored_by']); + self::invokePrivate($filesHooks, 'addNotificationsForFileAction', [$node, Files::TYPE_SHARE_RESTORED, 'restored_self', 'restored_by']); $this->assertEquals($addCalls, array_slice($receivedActivities, 0, count($addCalls))); } @@ -487,16 +493,20 @@ public function testFileMoveCollectsOldAccessList(): void { 'getUserPathsFromPath', ]); + $parent = $this->getNodeMock(23, '/user/files/folder', false); + $source = $this->getNodeMock(42, '/user/files/folder/file.txt'); + $source->method('getParent') + ->willReturn($parent); $filesHooks->expects($this->once()) ->method('getSourcePathAndOwner') - ->with('/folder') + ->with($parent) ->willReturn(['/folder', 'owner', 23]); $filesHooks->expects($this->once()) ->method('getUserPathsFromPath') ->with('/folder', 'owner') ->willReturn(['users' => ['user' => '/folder'], 'remotes' => []]); - $filesHooks->fileMove($this->getNodeMock(42, '/user/files/folder/file.txt'), $this->getNodeMock(42, '/user/files/target/file.txt')); + $filesHooks->fileMove($source, $this->getNodeMock(42, '/user/files/target/file.txt')); $this->assertSame('moveCross', self::invokePrivate($filesHooks, 'moveCase')); $this->assertSame(['users' => ['user' => '/folder'], 'remotes' => []], self::invokePrivate($filesHooks, 'oldAccessList')); @@ -510,10 +520,11 @@ public function testFileMoveOldPathNotResolvable(): void { 'fileMoving', ]); - $filesHooks->expects($this->once()) - ->method('getSourcePathAndOwner') - ->with('/folder') + $source = $this->getNodeMock(42, '/user/files/folder/file.txt'); + $source->method('getParent') ->willThrowException(new NotFoundException('File with id "1337" has not been found.')); + $filesHooks->expects($this->never()) + ->method('getSourcePathAndOwner'); $filesHooks->expects($this->never()) ->method('getUserPathsFromPath'); $filesHooks->expects($this->never()) @@ -521,7 +532,7 @@ public function testFileMoveOldPathNotResolvable(): void { $filesHooks->expects($this->never()) ->method('fileMoving'); - $filesHooks->fileMove($this->getNodeMock(42, '/user/files/folder/file.txt'), $this->getNodeMock(42, '/user/files/target/file.txt')); + $filesHooks->fileMove($source, $this->getNodeMock(42, '/user/files/target/file.txt')); $this->assertFalse(self::invokePrivate($filesHooks, 'moveCase')); @@ -535,14 +546,14 @@ public function testFileMovePostRename(): void { 'fileMoving', ]); + $source = $this->getNodeMock(42, '/user/files/folder/old.txt'); + $target = $this->getNodeMock(42, '/user/files/folder/new.txt'); $filesHooks->expects($this->once()) ->method('fileRenaming') - ->with('/folder/old.txt', '/folder/new.txt'); + ->with($source, $target); $filesHooks->expects($this->never()) ->method('fileMoving'); - $source = $this->getNodeMock(42, '/user/files/folder/old.txt'); - $target = $this->getNodeMock(42, '/user/files/folder/new.txt'); $filesHooks->fileMove($source, $target); $filesHooks->fileMovePost($source, $target); @@ -557,7 +568,7 @@ public function testFileMovingWithoutOldAccessList(): void { $filesHooks->expects($this->never()) ->method('getSourcePathAndOwner'); - self::invokePrivate($filesHooks, 'fileMoving', ['/folder/file.txt', '/target/file.txt']); + self::invokePrivate($filesHooks, 'fileMoving', [$this->getNodeMock(42, '/user/files/folder/file.txt'), $this->getNodeMock(42, '/user/files/target/file.txt')]); } private function getNodeMock(int $fileId = 1337, string $path = 'path', bool $isFile = true): Node&MockObject { @@ -570,9 +581,68 @@ private function getNodeMock(int $fileId = 1337, string $path = 'path', bool $is ->willReturn($fileId); $node->method('getPath') ->willReturn($path); + $node->method('getName') + ->willReturn(basename($path)); return $node; } + public static function dataGetSourcePathAndOwner(): array { + return [ + 'own file' => ['user', 'user', false, 'user'], + 'file shared by another user' => ['user', 'owner', false, 'owner'], + 'node without owner' => ['user', null, false, 'user'], + 'federated share' => ['user', 'owner', true, 'user'], + 'no current user' => [null, null, false, 'pathuser'], + ]; + } + + #[DataProvider('dataGetSourcePathAndOwner')] + public function testGetSourcePathAndOwner(?string $currentUser, ?string $owner, bool $isExternalStorage, string $expectedOwner): void { + $filesHooks = $this->getFilesHooks(['getOwnerPathById'], $currentUser); + + $node = $this->getNodeMock(42, '/pathuser/files/folder/file.txt'); + $ownerUser = null; + if ($owner !== null) { + $ownerUser = $this->createMock(IUser::class); + $ownerUser->method('getUID') + ->willReturn($owner); + } + $node->method('getOwner') + ->willReturn($ownerUser); + $storage = $this->createMock(IStorage::class); + $storage->method('instanceOfStorage') + ->with(\OCA\Files_Sharing\External\Storage::class) + ->willReturn($isExternalStorage); + $node->method('getStorage') + ->willReturn($storage); + + $filesHooks->expects($this->once()) + ->method('getOwnerPathById') + ->with($expectedOwner, 42) + ->willReturn('/owner/path'); + + $this->assertSame(['/owner/path', $expectedOwner, 42], self::invokePrivate($filesHooks, 'getSourcePathAndOwner', [$node])); + } + + public function testGetSourcePathAndOwnerNodeNotFound(): void { + $filesHooks = $this->getFilesHooks(['getOwnerPathById']); + + $node = $this->createMock(File::class); + $node->method('getPath') + ->willReturn('/user/files/folder/file.txt'); + $node->method('getOwner') + ->willThrowException(new NotFoundException()); + $node->method('getId') + ->willThrowException(new NotFoundException()); + $node->method('getStorage') + ->willReturn($this->createMock(IStorage::class)); + + $filesHooks->expects($this->never()) + ->method('getOwnerPathById'); + + $this->assertSame(['/folder/file.txt', 'user', 0], self::invokePrivate($filesHooks, 'getSourcePathAndOwner', [$node])); + } + public static function dataGetOwnerPathById(): array { return [ ['/owner/files/folder/file.txt', '/folder/file.txt'], diff --git a/tests/stubs/oc_files.php b/tests/stubs/oc_files.php index 893a81200..623317a79 100644 --- a/tests/stubs/oc_files.php +++ b/tests/stubs/oc_files.php @@ -1,58 +1,5 @@