Fix wrong user problem when using tag assignment in TeamFolders

lib/Operation.php

@@ -46,32 +46,15 @@
 use Psr\Log\LoggerInterface;
 
 class Operation implements ISpecificOperation {
-	/** @var IJobList */
-	private $jobList;
-	/** @var IL10N */
-	private $l;
-	/** @var LoggerInterface */
-	private $logger;
-	/** @var IURLGenerator */
-	private $urlGenerator;
-	/** @var IProcessingFileAccessor */
-	private $processingFileAccessor;
-	/** @var IRootFolder */
-	private $rootFolder;
 
 	public function __construct(
-		IJobList $jobList,
-		IL10N $l,
-		LoggerInterface $logger,
-		IURLGenerator $urlGenerator,
-		IProcessingFileAccessor $processingFileAccessor,
-		IRootFolder $rootFolder) {
-		$this->jobList = $jobList;
-		$this->l = $l;
-		$this->logger = $logger;
-		$this->urlGenerator = $urlGenerator;
-		$this->processingFileAccessor = $processingFileAccessor;
-		$this->rootFolder = $rootFolder;
+		private IJobList $jobList,
+		private IL10N $l,
+		private LoggerInterface $logger,
+		private IURLGenerator $urlGenerator,
+		private IProcessingFileAccessor $processingFileAccessor,
+		private IRootFolder $rootFolder,
+	) {
 	}
 
 	/**
@@ -178,16 +161,18 @@ private function tryGetFileFromMapperEvent(string $eventName, MapperEvent $event
 			return false;
 		}
 
-		$files = $this->rootFolder->getById($fileId);
-		if (empty($files) || !($files[0] instanceof \OCP\Files\File)) {
+		// #324: The root folder object can potentially return multiple nodes with the same id when using 'getById'.
+		// Threfore we use 'getFirstNodeById' here to ensure we only get one node which belongs to the current user.
+		$file = $this->rootFolder->getFirstNodeById($fileId);
+		if (!$file || !($file instanceof \OCP\Files\File)) {
 			$this->logger->warning(
 				'Not processing event {eventname} because node with id  \'{fileid}\' could not be found or is not a file.',
 				['eventname' => $eventName],
 				['fileid' => $fileId]);
 			return false;
 		}
 
-		$node = $files[0];
+		$node = $file;
 		return true;
 	}
 

lib/Service/OcrService.php

@@ -217,14 +217,14 @@ private function shutdownUserEnvironment() : void {
 	}
 
 	private function getNode(int $fileId) : Node {
-		/** @var File[] */
-		$nodeArr = $this->rootFolder->getById($fileId);
-		if (count($nodeArr) === 0) {
+		// #324: The root folder object can potentially return multiple nodes with the same id when using 'getById'.
+		// Therefore we use 'getFirstNodeById' here to ensure we only get one node which belongs to the current user.
+		$node = $this->rootFolder->getFirstNodeById($fileId);
+
+		if ($node === null) {
 			throw new NotFoundException('Could not process file with id \'' . $fileId . '\'. File was not found');
 		}
 
-		$node = array_shift($nodeArr);
-
 		if (!$node instanceof Node || $node->getType() !== FileInfo::TYPE_FILE) {
 			throw new \InvalidArgumentException('Skipping process for file with id \'' . $fileId . '\'. It is not a file');
 		}

tests/Unit/OperationTest.php

@@ -323,9 +323,9 @@ public function testDoesNothingOnMapperEventIfFileNotFound() {
 		/** @var MockObject|IRootFolder */
 		$this->rootFolder = $this->createMock(IRootFolder::class);
 		$this->rootFolder->expects($this->once())
-			->method('getById')
+			->method('getFirstNodeById')
 			->with(42)
-			->willReturn([]);
+			->willReturn(null);
 
 		$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder);
 
@@ -344,9 +344,9 @@ public function testDoesNothingOnMapperEventIfObjectIdIsFolder() {
 		/** @var MockObject|IRootFolder */
 		$this->rootFolder = $this->createMock(IRootFolder::class);
 		$this->rootFolder->expects($this->once())
-			->method('getById')
+			->method('getFirstNodeById')
 			->with(42)
-			->willReturn([$this->createMock(Folder::class)]);
+			->willReturn($this->createMock(Folder::class));
 
 		$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder);
 
@@ -375,10 +375,11 @@ public function testFileAddedToQueueOnTagAssignedEvent() {
 			->willReturn($fileId);
 		/** @var MockObject|IRootFolder */
 		$rootFolder = $this->createMock(IRootFolder::class);
+		$rootFolder->expects($this->never())->method('getById');
 		$rootFolder->expects($this->once())
-			->method('getById')
+			->method('getFirstNodeById')
 			->with($fileId)
-			->willReturn([$fileMock]);
+			->willReturn($fileMock);
 
 		$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $rootFolder);
 

tests/Unit/Service/OcrServiceTest.php

@@ -94,7 +94,7 @@ class OcrServiceTest extends TestCase {
 	/** @var INotificationService|MockObject */
 	private $notificationService;
 	/** @var File[] */
-	private $rootFolderGetById42ReturnValue;
+	private $rootFolderGetFirstNodeById42ReturnValue;
 	/** @var OcrService */
 	private $ocrService;
 	/** @var File|MockObject */
@@ -124,12 +124,12 @@ public function setUp() : void {
 
 		/** @var MockObject|IRootFolder */
 		$this->rootFolder = $this->createMock(IRootFolder::class);
-		$this->rootFolderGetById42ReturnValue = [$this->createValidFileMock()];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $this->createValidFileMock();
 		$this->rootFolder->expects($this->any())
-			->method('getById')
+			->method('getFirstNodeById')
 			->with(42)
 			->willReturnCallback(function () {
-				return $this->rootFolderGetById42ReturnValue;
+				return $this->rootFolderGetFirstNodeById42ReturnValue;
 			});
 
 		/** @var MockObject|IUserManager */
@@ -348,7 +348,7 @@ public function testCallsInitFilesystem() {
 	public function testCallsGetOnRootFolder() {
 		$settings = new WorkflowSettings();
 		$this->rootFolder->expects($this->once())
-			->method('getById')
+			->method('getFirstNodeById')
 			->with(42);
 
 		$this->ocrService->runOcrProcess(42, 'usr', $settings);
@@ -361,7 +361,7 @@ public function testCallsOcrProcessorWithFile() {
 		$mimeType = 'application/pdf';
 		$content = 'someFileContent';
 		$fileMock = $this->createValidFileMock($mimeType, $content);
-		$this->rootFolderGetById42ReturnValue = [$fileMock];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
 
 		$this->globalSettingsService->expects($this->once())
 			->method('getGlobalSettings')
@@ -385,7 +385,7 @@ public function testCreatesNewFileVersionAndEmitsTextRecognizedEvent(string $ori
 		$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors
 		$originalFileMock = $this->createValidFileMock($mimeType, $content, $rootFolderPath, $originalFilename);
 
-		$this->rootFolderGetById42ReturnValue = [$originalFileMock];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $originalFileMock;
 
 		$this->ocrProcessor->expects($this->once())
 			->method('ocrFile')
@@ -410,17 +410,18 @@ public function testCreatesNewFileVersionAndEmitsTextRecognizedEvent(string $ori
 
 	public function testThrowsNotFoundExceptionWhenFileNotFound() {
 		$settings = new WorkflowSettings();
-		$this->rootFolderGetById42ReturnValue = [];
+		$this->rootFolderGetFirstNodeById42ReturnValue = null;
 
 		$this->expectException(NotFoundException::class);
 		$this->ocrService->runOcrProcess(42, 'usr', $settings);
 	}
 
-	#[DataProvider('dataProvider_InvalidNodes')]
-	public function testDoesNotCallOcr_OnNonFile(callable $invalidNodeCallback) {
-		$invalidNode = $invalidNodeCallback($this);
+	public function testDoesNotCallOcr_OnNonFile() {
+		$invalidNode = $this->createMock(Node::class);
+		$invalidNode->method('getType')
+			->willReturn(FileInfo::TYPE_FOLDER);
 		$settings = new WorkflowSettings();
-		$this->rootFolderGetById42ReturnValue = [$invalidNode];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $invalidNode;
 
 		$this->ocrProcessor->expects($this->never())
 			->method('ocrFile');
@@ -473,7 +474,7 @@ public function testCallsProcessingFileAccessor() {
 		$filePath = '/admin/files/somefile.pdf';
 		$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors
 
-		$this->rootFolderGetById42ReturnValue = [$this->createValidFileMock($mimeType, $content)];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $this->createValidFileMock($mimeType, $content);
 
 		$this->ocrProcessor->expects($this->once())
 			->method('ocrFile')
@@ -517,10 +518,12 @@ public function testDoesNotCreateNewFileVersionIfOcrContentWasEmpty() {
 		$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent);
 		$fileId = 42;
 
+		$this->rootFolder->expects($this->never())->method('getById');
+
 		$this->rootFolder->expects($this->once())
-			->method('getById')
+			->method('getFirstNodeById')
 			->with($fileId)
-			->willReturn([$this->createValidFileMock($mimeType, $content)]);
+			->willReturn($this->createValidFileMock($mimeType, $content));
 
 		$this->ocrProcessor->expects($this->once())
 			->method('ocrFile')
@@ -600,7 +603,7 @@ public function testRestoreOriginalFileModificationDate() {
 		$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors
 
 		$fileMock = $this->createValidFileMock($mimeType, $content);
-		$this->rootFolderGetById42ReturnValue = [$fileMock];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
 
 		$this->ocrProcessor->expects($this->once())
 			->method('ocrFile')
@@ -648,7 +651,8 @@ public function testRunOcrProcessWithJobArgumentLogsErrorAndDoesNothingOnInvalid
 	}
 
 	public function testRunOcrProcessWithJobArgumentLogsErrorAndSendsNotificationOnNotFound() {
-		$this->rootFolder->method('getById')
+		// This will never be thrown in real live, it's just to test the error handling
+		$this->rootFolder->method('getFirstNodeById')
 			->willThrowException(new NotFoundException('File was not found'));
 		$this->logger->expects($this->once())
 			->method('error')
@@ -681,7 +685,7 @@ public function testCreatesNewFileVersionWithSuffixIfNodeIsNotUpdateable() {
 		$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors
 
 		$fileMock = $this->createValidFileMock($mimeType, $content, '/admin/files', 'somefile.pdf', false);
-		$this->rootFolderGetById42ReturnValue = [$fileMock];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
 
 		$this->ocrProcessor->expects($this->once())
 			->method('ocrFile')
@@ -707,7 +711,7 @@ public function testSetsFileVersionsLabelIfKeepOriginalFileVersionIsTrue() {
 		$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent);
 
 		$fileMock = $this->createValidFileMock($mimeType, $content);
-		$this->rootFolderGetById42ReturnValue = [$fileMock];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
 
 		$this->ocrProcessor->expects($this->once())
 			->method('ocrFile')
@@ -756,7 +760,7 @@ public function testRunOcrProcessThrowsNonOcrAlreadyDoneExceptionIfModeIsNotSkip
 		$content = 'someFileContent';
 
 		$fileMock = $this->createValidFileMock($mimeType, $content);
-		$this->rootFolderGetById42ReturnValue = [$fileMock];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
 
 		$ex = new OcrAlreadyDoneException('oops');
 		$this->ocrProcessor->expects($this->once())
@@ -777,7 +781,7 @@ public function testRunOcrProcessWorksWithoutIVersionManager() {
 		$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent);
 
 		$fileMock = $this->createValidFileMock($mimeType, $content);
-		$this->rootFolderGetById42ReturnValue = [$fileMock];
+		$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
 
 		$this->ocrProcessor->expects($this->once())
 			->method('ocrFile')
@@ -822,24 +826,6 @@ public function testRunOcrProcessWorksWithoutIVersionManager() {
 		$this->assertTrue($logged, 'Expected debug log message not found');
 	}
 
-	public static function dataProvider_InvalidNodes() {
-		$folderMockCallable = function (self $testClass) {
-			/** @var MockObject|Node */
-			$folderMock = $testClass->createMock(Node::class);
-			$folderMock->method('getType')
-				->willReturn(FileInfo::TYPE_FOLDER);
-			return $folderMock;
-		};
-		$fileInfoMockCallable = fn (self $testClass) => $testClass->createMock(FileInfo::class);
-		$arr = [
-			[$folderMockCallable],
-			[$fileInfoMockCallable],
-			[fn () => null],
-			[fn () => (object)['someField' => 'someValue']]
-		];
-		return $arr;
-	}
-
 	public static function dataProvider_OcrModesThrowOnEmptyResult() {
 		return [
 			[WorkflowSettings::OCR_MODE_FORCE_OCR],

tests/psalm-baseline.xml

@@ -207,23 +207,14 @@
   <file src="lib/Operation.php">
     <MissingDependency>
       <code><![CDATA[$this->rootFolder]]></code>
-      <code><![CDATA[IRootFolder]]></code>
-      <code><![CDATA[IRootFolder]]></code>
+      <code><![CDATA[private]]></code>
     </MissingDependency>
     <NullReference>
       <code><![CDATA[$argsArray]]></code>
     </NullReference>
     <PossiblyUnusedMethod>
       <code><![CDATA[__construct]]></code>
     </PossiblyUnusedMethod>
-    <PossiblyUnusedParam>
-      <code><![CDATA[$jobList]]></code>
-      <code><![CDATA[$l]]></code>
-      <code><![CDATA[$logger]]></code>
-      <code><![CDATA[$processingFileAccessor]]></code>
-      <code><![CDATA[$rootFolder]]></code>
-      <code><![CDATA[$urlGenerator]]></code>
-    </PossiblyUnusedParam>
     <UndefinedClass>
       <code><![CDATA[File]]></code>
     </UndefinedClass>