Merge pull request #2797 from nextcloud/backport/2784/stable33

[stable33] fix: do not fail moves when the old location cannot be resolved

Author: solracsf

lib/FilesHooks.php

@@ -43,7 +43,7 @@ class FilesHooks {
 
 	/** @var string|bool */
 	protected $moveCase = false;
-	/** @var array */
+	/** @var array|null */
 	protected $oldAccessList;
 	/** @var string */
 	protected $oldParentPath;
@@ -275,22 +275,30 @@ public function fileMove($oldPath, $newPath) {
 			$this->moveCase = 'moveCross';
 		}
 
-		[$this->oldParentPath, $this->oldParentOwner, $this->oldParentId] = $this->getSourcePathAndOwner($oldDir);
-		if ($this->oldParentId === 0) {
-			// Could not find the file for the owner ...
-			$this->moveCase = false;
-			return;
-		}
+		try {
+			[$this->oldParentPath, $this->oldParentOwner, $this->oldParentId] = $this->getSourcePathAndOwner($oldDir);
+			if ($this->oldParentId === 0) {
+				// Could not find the file for the owner ...
+				$this->moveCase = false;
+				return;
+			}
 
-		$oldAccessList = $this->getUserPathsFromPath($this->oldParentPath, $this->oldParentOwner);
+			$oldAccessList = $this->getUserPathsFromPath($this->oldParentPath, $this->oldParentOwner);
 
-		// file can be shared using GroupFolders, including ACL check
-		if ($this->config->getSystemValueBool('activity_use_cached_mountpoints', false)) {
-			[, , $oldFileId] = $this->getSourcePathAndOwner($oldPath);
-			$oldAccessList['users'] = array_merge($oldAccessList['users'], $this->getAffectedUsersFromCachedMounts($oldFileId));
-		}
+			// file can be shared using GroupFolders, including ACL check
+			if ($this->config->getSystemValueBool('activity_use_cached_mountpoints', false)) {
+				[, , $oldFileId] = $this->getSourcePathAndOwner($oldPath);
+				$oldAccessList['users'] = array_merge($oldAccessList['users'], $this->getAffectedUsersFromCachedMounts($oldFileId));
+			}
 
-		$this->oldAccessList = $oldAccessList;
+			$this->oldAccessList = $oldAccessList;
+		} catch (NotFoundException $e) {
+			// The old location cannot be resolved (e.g. inconsistent mount or file
+			// cache state), so fileMovePost() would have no valid data to build
+			// activities from. Skip it instead of failing the move half-way.
+			$this->logger->warning('Could not resolve the old location of "' . $oldPath . '", no move activities will be created', ['exception' => $e]);
+			$this->moveCase = false;
+		}
 	}
 
 
@@ -392,6 +400,12 @@ protected function fileRenaming($oldPath, $newPath) {
 	 * @param string $newPath
 	 */
 	protected function fileMoving($oldPath, $newPath) {
+		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);

tests/FilesHooksTest.php

@@ -481,6 +481,65 @@ public function testAddNotificationsForFileAction(array $filterUsers, bool $moun
 		$this->assertEquals($addCalls, array_slice($receivedActivities, 0, count($addCalls)));
 	}
 
+	public function testFileMoveCollectsOldAccessList(): void {
+		$filesHooks = $this->getFilesHooks([
+			'getSourcePathAndOwner',
+			'getUserPathsFromPath',
+		]);
+
+		$filesHooks->expects($this->once())
+			->method('getSourcePathAndOwner')
+			->with('/folder')
+			->willReturn(['/folder', 'owner', 23]);
+		$filesHooks->expects($this->once())
+			->method('getUserPathsFromPath')
+			->with('/folder', 'owner')
+			->willReturn(['users' => ['user' => '/folder'], 'remotes' => []]);
+
+		$filesHooks->fileMove('/folder/file.txt', '/target/file.txt');
+
+		$this->assertSame('moveCross', self::invokePrivate($filesHooks, 'moveCase'));
+		$this->assertSame(['users' => ['user' => '/folder'], 'remotes' => []], self::invokePrivate($filesHooks, 'oldAccessList'));
+	}
+
+	public function testFileMoveOldPathNotResolvable(): void {
+		$filesHooks = $this->getFilesHooks([
+			'getSourcePathAndOwner',
+			'getUserPathsFromPath',
+			'fileRenaming',
+			'fileMoving',
+		]);
+
+		$filesHooks->expects($this->once())
+			->method('getSourcePathAndOwner')
+			->with('/folder')
+			->willThrowException(new NotFoundException('File with id "1337" has not been found.'));
+		$filesHooks->expects($this->never())
+			->method('getUserPathsFromPath');
+		$filesHooks->expects($this->never())
+			->method('fileRenaming');
+		$filesHooks->expects($this->never())
+			->method('fileMoving');
+
+		$filesHooks->fileMove('/folder/file.txt', '/target/file.txt');
+
+		$this->assertFalse(self::invokePrivate($filesHooks, 'moveCase'));
+
+		// the post hook must be a no-op instead of failing on the missing state
+		$filesHooks->fileMovePost('/folder/file.txt', '/target/file.txt');
+	}
+
+	public function testFileMovingWithoutOldAccessList(): void {
+		$filesHooks = $this->getFilesHooks([
+			'getSourcePathAndOwner',
+		]);
+
+		$filesHooks->expects($this->never())
+			->method('getSourcePathAndOwner');
+
+		self::invokePrivate($filesHooks, 'fileMoving', ['/folder/file.txt', '/target/file.txt']);
+	}
+
 	private function getNodeMock(int $fileId = 1337, string $path = 'path', bool $isFile = true): Node&MockObject {
 		if ($isFile) {
 			$node = $this->createMock(File::class);