From 65bed0209b0e2fc9c631cf6e168a9d68b101b244 Mon Sep 17 00:00:00 2001 From: Robin Windey Date: Wed, 11 May 2022 07:42:10 +0200 Subject: [PATCH] Use uid from filepath (#116) * Read user id from filepath, don't use fileowner * Use new \OCP\BackgroundJob\QueuedJob base class * Closing #110 --- lib/BackgroundJobs/ProcessFileJob.php | 25 ++++++---- lib/Operation.php | 49 +++++++------------ .../BackgroundJobs/ProcessFileJobTest.php | 21 ++++---- tests/Unit/OperationTest.php | 10 ++-- tests/psalm-baseline.xml | 2 +- 5 files changed, 51 insertions(+), 56 deletions(-) diff --git a/lib/BackgroundJobs/ProcessFileJob.php b/lib/BackgroundJobs/ProcessFileJob.php index 106c7f8..19e492d 100644 --- a/lib/BackgroundJobs/ProcessFileJob.php +++ b/lib/BackgroundJobs/ProcessFileJob.php @@ -37,6 +37,7 @@ use OCA\WorkflowOcr\Service\IOcrService; use OCA\WorkflowOcr\Wrapper\IFilesystem; use OCA\WorkflowOcr\Wrapper\IViewFactory; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\FileInfo; use OCP\Files\Node; use OCP\IUserManager; @@ -48,7 +49,7 @@ * Represents a QuedJob which processes * a OCR on a single file. */ -class ProcessFileJob extends \OC\BackgroundJob\QueuedJob { +class ProcessFileJob extends \OCP\BackgroundJob\QueuedJob { /** @var LoggerInterface */ protected $logger; @@ -75,7 +76,9 @@ public function __construct( IFilesystem $filesystem, IUserManager $userManager, IUserSession $userSession, - IProcessingFileAccessor $processingFileAccessor) { + IProcessingFileAccessor $processingFileAccessor, + ITimeFactory $timeFactory) { + parent::__construct($timeFactory); $this->logger = $logger; $this->rootFolder = $rootFolder; $this->ocrService = $ocrService; @@ -121,21 +124,23 @@ private function tryParseArguments($argument) : array { } $filePath = null; + $uid = null; $filePathKey = 'filePath'; if (array_key_exists($filePathKey, $argument)) { $filePath = $argument[$filePathKey]; + // '', admin, 'files', 'path/to/file.pdf' + $splitted = explode('/', $filePath, 4); + if (count($splitted) < 4) { + $this->logger->warning('File path "' . $filePath . '" is not valid in ' . self::class . ' method \'tryParseArguments\'.'); + return [ + false + ]; + } + $uid = $splitted[1]; } else { $this->logVariableKeyNotSet($filePathKey, 'tryParseArguments'); } - $uid = null; - $uidKey = 'uid'; - if (array_key_exists($uidKey, $argument)) { - $uid = $argument[$uidKey]; - } else { - $this->logVariableKeyNotSet($uidKey, 'tryParseArguments'); - } - $settings = null; $settingsKey = 'settings'; if (array_key_exists($settingsKey, $argument)) { diff --git a/lib/Operation.php b/lib/Operation.php index 3ab0a43..73bf61b 100644 --- a/lib/Operation.php +++ b/lib/Operation.php @@ -105,30 +105,17 @@ public function isAvailableForScope(int $scope): bool { public function onEvent(string $eventName, Event $event, IRuleMatcher $ruleMatcher): void { $this->logger->debug('onEvent: ' . $eventName); - if (!($match = $this->getMatch($ruleMatcher))) { + // $node and $argsArray will be passed by reference + if (!($match = $this->getMatch($ruleMatcher)) || + !$this->tryGetFile($eventName, $event, $node) || + $this->eventTriggeredByOcrProcess($node) || + !$this->tryGetJobArgs($node, $match, $argsArray)) { return; } - - // $node will be passed by reference - if (!$this->tryGetFile($eventName, $event, $node)) { - return; - } - - if (!$this->pathIsValid($node) || - !$this->ownerExists($node) || - $this->eventTriggeredByOcrProcess($node)) { - return; - } - - $args = [ - 'filePath' => $node->getPath(), - 'uid' => $node->getOwner()->getUID(), - 'settings' => $match['operation'] - ]; - $this->logger->debug('Adding file to jobqueue: ' . json_encode($args)); + $this->logger->debug('Adding file to jobqueue: ' . json_encode($argsArray)); - $this->jobList->add(ProcessFileJob::class, $args); + $this->jobList->add(ProcessFileJob::class, $argsArray); } public function getEntityId(): string { @@ -205,7 +192,12 @@ private function getMatch(IRuleMatcher $ruleMatcher) : array { return $match; } - private function pathIsValid(Node $node) : bool { + /** + * @param Node $node + * @param array $match + * @param array $argsArray + */ + private function tryGetJobArgs(Node $node, $match, & $argsArray) : bool { // Check path has valid structure $filePath = $node->getPath(); // '', admin, 'files', 'path/to/file.pdf' @@ -216,17 +208,10 @@ private function pathIsValid(Node $node) : bool { return false; } - return true; - } - - private function ownerExists(Node $node) : bool { - // Check owner of file exists - $owner = $node->getOwner(); - if ($owner === null) { - $this->logger->debug('Not processing event because file with path \'{path}\' has no owner.', - ['path' => $node->getPath()]); - return false; - } + $argsArray = [ + 'filePath' => $filePath, + 'settings' => $match['operation'] + ]; return true; } diff --git a/tests/Unit/BackgroundJobs/ProcessFileJobTest.php b/tests/Unit/BackgroundJobs/ProcessFileJobTest.php index 118e984..73642c3 100644 --- a/tests/Unit/BackgroundJobs/ProcessFileJobTest.php +++ b/tests/Unit/BackgroundJobs/ProcessFileJobTest.php @@ -105,8 +105,10 @@ public function setUp() : void { $this->filesystem, $this->userManager, $this->userSession, - $this->processingFileAccessor + $this->processingFileAccessor, + $this->createMock(ITimeFactory::class) ); + $this->processFileJob->setId(111); /** @var IConfig */ $configMock = $this->createMock(IConfig::class); @@ -335,9 +337,11 @@ public function testThrowsNoUserException_OnNonExistingUser() { $this->filesystem, $userManager, $this->userSession, - $this->processingFileAccessor + $this->processingFileAccessor, + $this->createMock(ITimeFactory::class) ); - $arguments = ['filePath' => '/admin/files/someInvalidStuff', 'uid' => 'nonexistinguser', 'settings' => '{}']; + $processFileJob->setId(111); + $arguments = ['filePath' => '/nonexistinguser/files/someInvalidStuff', 'settings' => '{}']; $processFileJob->setArgument($arguments); $processFileJob->execute($this->jobList); @@ -393,18 +397,17 @@ public function testCallsProcessingFileAccessor(array $arguments, string $user, public function dataProvider_InvalidArguments() { $arr = [ [null, 1], - [['mykey' => 'myvalue'], 3], - [['someotherkey' => 'someothervalue', 'k2' => 'v2'], 3], - [['uid' => 'someuser'], 2], - [['filePath' => 'somepath'], 2] + [['mykey' => 'myvalue'], 2], + [['someotherkey' => 'someothervalue', 'k2' => 'v2'], 2], + [['filePath' => 'someInvalidPath'], 1] ]; return $arr; } public function dataProvider_ValidArguments() { $arr = [ - [['filePath' => '/admin/files/somefile.pdf', 'uid' => 'admin', 'settings' => '{}'], 'admin', '/admin/files'], - [['filePath' => '/myuser/files/subfolder/someotherfile.docx', 'uid' => 'myuser', 'settings' => '{}'], 'myuser', '/myuser/files'] + [['filePath' => '/admin/files/somefile.pdf', 'settings' => '{}'], 'admin', '/admin/files'], + [['filePath' => '/myuser/files/subfolder/someotherfile.docx', 'settings' => '{}'], 'myuser', '/myuser/files'] ]; return $arr; } diff --git a/tests/Unit/OperationTest.php b/tests/Unit/OperationTest.php index 378e4c7..3119466 100644 --- a/tests/Unit/OperationTest.php +++ b/tests/Unit/OperationTest.php @@ -176,6 +176,8 @@ public function testDoesNothingOnInvalidFilePath(string $filePath) { ->willReturn(FileInfo::TYPE_FILE); $fileMock->method('getPath') ->willReturn($filePath); + $fileMock->method('getId') + ->willReturn(42); $event = new GenericEvent($fileMock); $eventName = '\OCP\Files::postCreate'; @@ -212,13 +214,13 @@ public function testAddWithCorrectFilePathAndUser() { $uid = 'admin'; $this->jobList->expects($this->once()) ->method('add') - ->with(ProcessFileJob::class, ['filePath' => $filePath, 'uid' => $uid, 'settings' => self::SETTINGS]); + ->with(ProcessFileJob::class, ['filePath' => $filePath, 'settings' => self::SETTINGS]); $operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder); /** @var MockObject|IUser */ $userMock = $this->createMock(IUser::class); - $userMock->expects($this->once()) + $userMock->expects($this->never()) ->method('getUID') ->willReturn($uid); /** @var MockObject|Node */ @@ -402,11 +404,11 @@ public function testFileAddedToQueueOnTagAssignedEvent() { $this->jobList->expects($this->once()) ->method('add') - ->with(ProcessFileJob::class, ['filePath' => $filePath, 'uid' => $uid, 'settings' => self::SETTINGS]); + ->with(ProcessFileJob::class, ['filePath' => $filePath, 'settings' => self::SETTINGS]); /** @var MockObject|IUser */ $userMock = $this->createMock(IUser::class); - $userMock->expects($this->once()) + $userMock->expects($this->never()) ->method('getUID') ->willReturn($uid); /** @var MockObject|\OCP\Files\File */ diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml index 2ed75b1..2c72907 100644 --- a/tests/psalm-baseline.xml +++ b/tests/psalm-baseline.xml @@ -8,7 +8,7 @@ - \OC\BackgroundJob\QueuedJob + \OCP\BackgroundJob\QueuedJob