Merge pull request #46368 from nextcloud/fix/task-processing

TaskProcessing follow-up

4 files changed, 183 insertions, 55 deletions

diff --git a/lib/private/TaskProcessing/Manager.php b/lib/private/TaskProcessing/Manager.php
index f720776a239..d5a09ff2472 100644
--- a/lib/private/TaskProcessing/Manager.php
+++ b/lib/private/TaskProcessing/Manager.php
@@ -18,10 +18,13 @@ use OCP\BackgroundJob\IJobList;
 use OCP\DB\Exception;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\AppData\IAppDataFactory;
+use OCP\Files\Config\IUserMountCache;
 use OCP\Files\File;
 use OCP\Files\GenericFileException;
 use OCP\Files\IAppData;
+use OCP\Files\InvalidPathException;
 use OCP\Files\IRootFolder;
+use OCP\Files\Node;
 use OCP\Files\NotPermittedException;
 use OCP\Files\SimpleFS\ISimpleFile;
 use OCP\IConfig;
@@ -79,7 +82,7 @@ class Manager implements IManager {
 		private \OCP\TextProcessing\IManager $textProcessingManager,
 		private \OCP\TextToImage\IManager $textToImageManager,
 		private \OCP\SpeechToText\ISpeechToTextManager $speechToTextManager,
-		private \OCP\Share\IManager $shareManager,
+		private IUserMountCache $userMountCache,
 	) {
 		$this->appData = $appDataFactory->get('core');
 	}
@@ -456,7 +459,7 @@ class Manager implements IManager {
 	 * @return void
 	 * @throws ValidationException
 	 */
-	private function validateOutput(array $spec, array $io, bool $optional = false): void {
+	private function validateOutputWithFileIds(array $spec, array $io, bool $optional = false): void {
 		foreach ($spec as $key => $descriptor) {
 			$type = $descriptor->getShapeType();
 			if (!isset($io[$key])) {
@@ -466,7 +469,31 @@ class Manager implements IManager {
 				throw new ValidationException('Missing key: "' . $key . '"');
 			}
 			try {
-				$type->validateOutput($io[$key]);
+				$type->validateOutputWithFileIds($io[$key]);
+			} catch (ValidationException $e) {
+				throw new ValidationException('Failed to validate output key "' . $key . '": ' . $e->getMessage());
+			}
+		}
+	}
+
+	/**
+	 * @param ShapeDescriptor[] $spec
+	 * @param array $io
+	 * @param bool $optional
+	 * @return void
+	 * @throws ValidationException
+	 */
+	private function validateOutputWithFileData(array $spec, array $io, bool $optional = false): void {
+		foreach ($spec as $key => $descriptor) {
+			$type = $descriptor->getShapeType();
+			if (!isset($io[$key])) {
+				if ($optional) {
+					continue;
+				}
+				throw new ValidationException('Missing key: "' . $key . '"');
+			}
+			try {
+				$type->validateOutputWithFileData($io[$key]);
 			} catch (ValidationException $e) {
 				throw new ValidationException('Failed to validate output key "' . $key . '": ' . $e->getMessage());
 			}
@@ -575,19 +602,8 @@ class Manager implements IManager {
 			}
 		}
 		foreach ($ids as $fileId) {
-			$node = $this->rootFolder->getFirstNodeById($fileId);
-			if ($node === null) {
-				$node = $this->rootFolder->getFirstNodeByIdInPath($fileId, '/' . $this->rootFolder->getAppDataDirectoryName() . '/');
-				if ($node === null) {
-					throw new ValidationException('Could not find file ' . $fileId);
-				}
-			}
-			/** @var array{users:array<string,array{node_id:int, node_path: string}>, remote: array<string,array{node_id:int, node_path: string}>, mail: array<string,array{node_id:int, node_path: string}>} $accessList */
-			$accessList = $this->shareManager->getAccessList($node, true, true);
-			$userIds = array_map(fn ($id) => strval($id), array_keys($accessList['users']));
-			if (!in_array($task->getUserId(), $userIds)) {
-				throw new UnauthorizedException('User ' . $task->getUserId() . ' does not have access to file ' . $fileId);
-			}
+			$this->validateFileId($fileId);
+			$this->validateUserAccessToFile($fileId, $task->getUserId());
 		}
 		// remove superfluous keys and set input
 		$task->setInput($this->removeSuperfluousArrayKeys($task->getInput(), $inputShape, $optionalInputShape));
@@ -657,7 +673,7 @@ class Manager implements IManager {
 		return true;
 	}
 
-	public function setTaskResult(int $id, ?string $error, ?array $result): void {
+	public function setTaskResult(int $id, ?string $error, ?array $result, bool $isUsingFileIds = false): void {
 		// TODO: Not sure if we should rather catch the exceptions of getTask here and fail silently
 		$task = $this->getTask($id);
 		if ($task->getStatus() === Task::STATUS_CANCELLED) {
@@ -674,11 +690,29 @@ class Manager implements IManager {
 			$optionalOutputShape = $taskTypes[$task->getTaskTypeId()]['optionalOutputShape'];
 			try {
 				// validate output
-				$this->validateOutput($outputShape, $result);
-				$this->validateOutput($optionalOutputShape, $result, true);
+				if (!$isUsingFileIds) {
+					$this->validateOutputWithFileData($outputShape, $result);
+					$this->validateOutputWithFileData($optionalOutputShape, $result, true);
+				} else {
+					$this->validateOutputWithFileIds($outputShape, $result);
+					$this->validateOutputWithFileIds($optionalOutputShape, $result, true);
+				}
 				$output = $this->removeSuperfluousArrayKeys($result, $outputShape, $optionalOutputShape);
 				// extract raw data and put it in files, replace it with file ids
-				$output = $this->encapsulateOutputFileData($output, $outputShape, $optionalOutputShape);
+				if (!$isUsingFileIds) {
+					$output = $this->encapsulateOutputFileData($output, $outputShape, $optionalOutputShape);
+				} else {
+					$this->validateOutputFileIds($output, $outputShape, $optionalOutputShape);
+				}
+				// Turn file objects into IDs
+				foreach ($output as $key => $value) {
+					if ($value instanceof Node) {
+						$output[$key] = $value->getId();
+					}
+					if (is_array($value) && $value[0] instanceof Node) {
+						$output[$key] = array_map(fn ($node) => $node->getId(), $value);
+					}
+				}
 				$task->setOutput($output);
 				$task->setProgress(1);
 				$task->setStatus(Task::STATUS_SUCCESSFUL);
@@ -694,7 +728,12 @@ class Manager implements IManager {
 				$error = 'The task was processed successfully but storing the output in a file failed';
 				$task->setErrorMessage($error);
 				$this->logger->error($error, ['exception' => $e]);
-
+			} catch (InvalidPathException|\OCP\Files\NotFoundException $e) {
+				$task->setProgress(1);
+				$task->setStatus(Task::STATUS_FAILED);
+				$error = 'The task was processed successfully but the result file could not be found';
+				$task->setErrorMessage($error);
+				$this->logger->error($error, ['exception' => $e]);
 			}
 		}
 		$taskEntity = \OC\TaskProcessing\Db\Task::fromPublicTask($task);
@@ -725,16 +764,13 @@ class Manager implements IManager {
 	}
 
 	/**
-	 * Takes task input or output data and replaces fileIds with base64 data
+	 * Takes task input data and replaces fileIds with File objects
 	 *
 	 * @param string|null $userId
 	 * @param array<array-key, list<numeric|string>|numeric|string> $input
 	 * @param ShapeDescriptor[] ...$specs the specs
 	 * @return array<array-key, list<File|numeric|string>|numeric|string|File>
-	 * @throws GenericFileException
-	 * @throws LockedException
-	 * @throws NotPermittedException
-	 * @throws ValidationException
+	 * @throws GenericFileException|LockedException|NotPermittedException|ValidationException|UnauthorizedException
 	 */
 	public function fillInputFileData(?string $userId, array $input, ...$specs): array {
 		if ($userId !== null) {
@@ -751,31 +787,17 @@ class Manager implements IManager {
 				$newInputOutput[$key] = $input[$key];
 				continue;
 			}
-			if ($type->value < 10) {
-				$node = $this->rootFolder->getFirstNodeById((int)$input[$key]);
-				if ($node === null) {
-					$node = $this->rootFolder->getFirstNodeByIdInPath((int)$input[$key], '/' . $this->rootFolder->getAppDataDirectoryName() . '/');
-					if (!$node instanceof File) {
-						throw new ValidationException('File id given for key "' . $key . '" is not a file');
-					}
-				} elseif (!$node instanceof File) {
-					throw new ValidationException('File id given for key "' . $key . '" is not a file');
-				}
-				// TODO: Validate if userId has access to this file
+			if (EShapeType::getScalarType($type) === $type) {
+				// is scalar
+				$node = $this->validateFileId((int)$input[$key]);
+				$this->validateUserAccessToFile($input[$key], $userId);
 				$newInputOutput[$key] = $node;
 			} else {
+				// is list
 				$newInputOutput[$key] = [];
 				foreach ($input[$key] as $item) {
-					$node = $this->rootFolder->getFirstNodeById((int)$item);
-					if ($node === null) {
-						$node = $this->rootFolder->getFirstNodeByIdInPath((int)$item, '/' . $this->rootFolder->getAppDataDirectoryName() . '/');
-						if (!$node instanceof File) {
-							throw new ValidationException('File id given for key "' . $key . '" is not a file');
-						}
-					} elseif (!$node instanceof File) {
-						throw new ValidationException('File id given for key "' . $key . '" is not a file');
-					}
-					// TODO: Validate if userId has access to this file
+					$node = $this->validateFileId((int)$item);
+					$this->validateUserAccessToFile($item, $userId);
 					$newInputOutput[$key][] = $node;
 				}
 			}
@@ -843,15 +865,15 @@ class Manager implements IManager {
 				$newOutput[$key] = $output[$key];
 				continue;
 			}
-			if ($type->value < 10) {
+			if (EShapeType::getScalarType($type) === $type) {
 				/** @var SimpleFile $file */
-				$file = $folder->newFile((string) rand(0, 10000000), $output[$key]);
+				$file = $folder->newFile(time() . '-' . rand(1, 100000), $output[$key]);
 				$newOutput[$key] = $file->getId(); // polymorphic call to SimpleFile
 			} else {
 				$newOutput = [];
 				foreach ($output[$key] as $item) {
 					/** @var SimpleFile $file */
-					$file = $folder->newFile((string) rand(0, 10000000), $item);
+					$file = $folder->newFile(time() . '-' . rand(1, 100000), $item);
 					$newOutput[$key][] = $file->getId();
 				}
 			}
@@ -865,7 +887,7 @@ class Manager implements IManager {
 	 * @throws GenericFileException
 	 * @throws LockedException
 	 * @throws NotPermittedException
-	 * @throws ValidationException
+	 * @throws ValidationException|UnauthorizedException
 	 */
 	public function prepareInputData(Task $task): array {
 		$taskTypes = $this->getAvailableTaskTypes();
@@ -898,4 +920,73 @@ class Manager implements IManager {
 		$taskEntity = \OC\TaskProcessing\Db\Task::fromPublicTask($task);
 		$this->taskMapper->update($taskEntity);
 	}
+
+	/**
+	 * @param array $output
+	 * @param ShapeDescriptor[] ...$specs the specs that define which keys to keep
+	 * @return array
+	 * @throws NotPermittedException
+	 */
+	private function validateOutputFileIds(array $output, ...$specs): array {
+		$newOutput = [];
+		$spec = array_reduce($specs, fn ($carry, $spec) => $carry + $spec, []);
+		foreach($spec as $key => $descriptor) {
+			$type = $descriptor->getShapeType();
+			if (!isset($output[$key])) {
+				continue;
+			}
+			if (!in_array(EShapeType::getScalarType($type), [EShapeType::Image, EShapeType::Audio, EShapeType::Video, EShapeType::File], true)) {
+				$newOutput[$key] = $output[$key];
+				continue;
+			}
+			if (EShapeType::getScalarType($type) === $type) {
+				// Is scalar file ID
+				$newOutput[$key] = $this->validateFileId($output[$key]);
+			} else {
+				// Is list of file IDs
+				$newOutput = [];
+				foreach ($output[$key] as $item) {
+					$newOutput[$key][] = $this->validateFileId($item);
+				}
+			}
+		}
+		return $newOutput;
+	}
+
+	/**
+	 * @param mixed $id
+	 * @return File
+	 * @throws ValidationException
+	 */
+	private function validateFileId(mixed $id): File {
+		$node = $this->rootFolder->getFirstNodeById($id);
+		if ($node === null) {
+			$node = $this->rootFolder->getFirstNodeByIdInPath($id, '/' . $this->rootFolder->getAppDataDirectoryName() . '/');
+			if ($node === null) {
+				throw new ValidationException('Could not find file ' . $id);
+			} elseif (!$node instanceof File) {
+				throw new ValidationException('File with id "' . $id . '" is not a file');
+			}
+		} elseif (!$node instanceof File) {
+			throw new ValidationException('File with id "' . $id . '" is not a file');
+		}
+		return $node;
+	}
+
+	/**
+	 * @param mixed $fileId
+	 * @param string $userId
+	 * @return void
+	 * @throws UnauthorizedException
+	 */
+	private function validateUserAccessToFile(mixed $fileId, ?string $userId): void {
+		if ($userId === null) {
+			throw new UnauthorizedException('User does not have access to file ' . $fileId);
+		}
+		$mounts = $this->userMountCache->getMountsForFileId($fileId);
+		$userIds = array_map(fn ($mount) => $mount->getUser()->getUID(), $mounts);
+		if (!in_array($userId, $userIds)) {
+			throw new UnauthorizedException('User ' . $userId . ' does not have access to file ' . $fileId);
+		}
+	}
 }
diff --git a/lib/private/TaskProcessing/SynchronousBackgroundJob.php b/lib/private/TaskProcessing/SynchronousBackgroundJob.php
index 7f1ab623190..093882d4c1e 100644
--- a/lib/private/TaskProcessing/SynchronousBackgroundJob.php
+++ b/lib/private/TaskProcessing/SynchronousBackgroundJob.php
@@ -15,6 +15,7 @@ use OCP\Lock\LockedException;
 use OCP\TaskProcessing\Exception\Exception;
 use OCP\TaskProcessing\Exception\NotFoundException;
 use OCP\TaskProcessing\Exception\ProcessingException;
+use OCP\TaskProcessing\Exception\UnauthorizedException;
 use OCP\TaskProcessing\Exception\ValidationException;
 use OCP\TaskProcessing\IManager;
 use OCP\TaskProcessing\ISynchronousProvider;
@@ -54,7 +55,7 @@ class SynchronousBackgroundJob extends QueuedJob {
 			try {
 				try {
 					$input = $this->taskProcessingManager->prepareInputData($task);
-				} catch (GenericFileException|NotPermittedException|LockedException|ValidationException $e) {
+				} catch (GenericFileException|NotPermittedException|LockedException|ValidationException|UnauthorizedException $e) {
 					$this->logger->warning('Failed to prepare input data for a TaskProcessing task with synchronous provider ' . $provider->getId(), ['exception' => $e]);
 					$this->taskProcessingManager->setTaskResult($task->getId(), $e->getMessage(), null);
 					// Schedule again
diff --git a/lib/public/TaskProcessing/EShapeType.php b/lib/public/TaskProcessing/EShapeType.php
index d66de6e01a8..059f9d0c3c7 100644
--- a/lib/public/TaskProcessing/EShapeType.php
+++ b/lib/public/TaskProcessing/EShapeType.php
@@ -89,7 +89,7 @@ enum EShapeType: int {
 	 * @throws ValidationException
 	 * @since 30.0.0
 	 */
-	public function validateOutput(mixed $value) {
+	public function validateOutputWithFileData(mixed $value): void {
 		$this->validateNonFileType($value);
 		if ($this === EShapeType::Image && !is_string($value)) {
 			throw new ValidationException('Non-image item provided for Image slot');
@@ -118,6 +118,40 @@ enum EShapeType: int {
 	}
 
 	/**
+	 * @param mixed $value
+	 * @return void
+	 * @throws ValidationException
+	 * @since 30.0.0
+	 */
+	public function validateOutputWithFileIds(mixed $value): void {
+		$this->validateNonFileType($value);
+		if ($this === EShapeType::Image && !is_numeric($value)) {
+			throw new ValidationException('Non-image item provided for Image slot');
+		}
+		if ($this === EShapeType::ListOfImages && (!is_array($value) || count(array_filter($value, fn ($item) => !is_numeric($item))) > 0)) {
+			throw new ValidationException('Non-image list item provided for ListOfImages slot');
+		}
+		if ($this === EShapeType::Audio && !is_string($value)) {
+			throw new ValidationException('Non-audio item provided for Audio slot');
+		}
+		if ($this === EShapeType::ListOfAudios && (!is_array($value) || count(array_filter($value, fn ($item) => !is_numeric($item))) > 0)) {
+			throw new ValidationException('Non-audio list item provided for ListOfAudio slot');
+		}
+		if ($this === EShapeType::Video && !is_string($value)) {
+			throw new ValidationException('Non-video item provided for Video slot');
+		}
+		if ($this === EShapeType::ListOfVideos && (!is_array($value) || count(array_filter($value, fn ($item) => !is_numeric($item))) > 0)) {
+			throw new ValidationException('Non-video list item provided for ListOfTexts slot');
+		}
+		if ($this === EShapeType::File && !is_string($value)) {
+			throw new ValidationException('Non-file item provided for File slot');
+		}
+		if ($this === EShapeType::ListOfFiles && (!is_array($value) || count(array_filter($value, fn ($item) => !is_numeric($item))) > 0)) {
+			throw new ValidationException('Non-audio list item provided for ListOfFiles slot');
+		}
+	}
+
+	/**
 	 * @param EShapeType $type
 	 * @return EShapeType
 	 * @since 30.0.0
diff --git a/lib/public/TaskProcessing/IManager.php b/lib/public/TaskProcessing/IManager.php
index 599bd244d8a..c68ad1afbac 100644
--- a/lib/public/TaskProcessing/IManager.php
+++ b/lib/public/TaskProcessing/IManager.php
@@ -91,11 +91,12 @@ interface IManager {
 	 * @param int $id The id of the task
 	 * @param string|null $error
 	 * @param array|null $result
+	 * @param bool $isUsingFileIds
 	 * @throws Exception If the query failed
 	 * @throws NotFoundException If the task could not be found
 	 * @since 30.0.0
 	 */
-	public function setTaskResult(int $id, ?string $error, ?array $result): void;
+	public function setTaskResult(int $id, ?string $error, ?array $result, bool $isUsingFileIds = false): void;
 
 	/**
 	 * @param int $id
@@ -152,7 +153,7 @@ interface IManager {
 
 	/**
 	 * Prepare the task's input data, so it can be processed by the provider
-	 * ie. this replaces file ids with base64 data
+	 * ie. this replaces file ids with File objects
 	 *
 	 * @param Task $task
 	 * @return array<array-key, list<numeric|string|File>|numeric|string|File>
@@ -160,6 +161,7 @@ interface IManager {
 	 * @throws GenericFileException
 	 * @throws LockedException
 	 * @throws ValidationException
+	 * @throws UnauthorizedException
 	 * @since 30.0.0
 	 */
 	public function prepareInputData(Task $task): array;