diff --git a/README.md b/README.md index d03667f..dc5f954 100644 --- a/README.md +++ b/README.md @@ -189,6 +189,7 @@ Name | Description -----|------------ Processor cores | Defines the number of processor cores to use for OCR processing. When the input is a PDF file, this corresponds to the [`ocrmypdf` CPU limit](https://ocrmypdf.readthedocs.io/en/latest/pdfsecurity.html?highlight=%22-j%22#limiting-cpu-usage). This setting can be especially useful if you have a small backend system which has only limited power. Request timeout | Defines a global timeout (in seconds) when using the `workflow_ocr_backend` app. Use this to limit how long the app will wait for OCR responses. Default value is 60 seconds. | +OCR user | Defines a dedicated user account which is used to run the OCR processing. If this setting is left empty (default), the app impersonates the owner of the processed file, which means that the resulting file version and the file activity are attributed to that user. By configuring a dedicated user you can make automated OCR operations distinguishable from manual user edits in the audit trail. Please note that the configured user needs write access to all files which should be OCR processed (for example via group shares) - otherwise the processing will fail. If the configured user does not exist, the app logs a warning and falls back to the file owner. Notifications are always sent to the owner of the processed file. | ### Testing your configuration diff --git a/lib/Controller/GlobalSettingsController.php b/lib/Controller/GlobalSettingsController.php index e9fe9fe..6e1f8fe 100644 --- a/lib/Controller/GlobalSettingsController.php +++ b/lib/Controller/GlobalSettingsController.php @@ -61,6 +61,8 @@ public function setGlobalSettings(array $globalSettings) : JSONResponse { $globalSettingsObject = new GlobalSettings(); $globalSettingsObject->processorCount = isset($globalSettings['processorCount']) ? (int)$globalSettings['processorCount'] : null; $globalSettingsObject->timeout = isset($globalSettings['timeout']) ? (int)$globalSettings['timeout'] : null; + $processingUserId = isset($globalSettings['processingUserId']) ? trim((string)$globalSettings['processingUserId']) : ''; + $globalSettingsObject->processingUserId = $processingUserId !== '' ? $processingUserId : null; $this->globalSettingsService->setGlobalSettings($globalSettingsObject); return $this->globalSettingsService->getGlobalSettings(); diff --git a/lib/Model/GlobalSettings.php b/lib/Model/GlobalSettings.php index 29c8135..e70b627 100644 --- a/lib/Model/GlobalSettings.php +++ b/lib/Model/GlobalSettings.php @@ -29,4 +29,10 @@ class GlobalSettings { public ?int $processorCount = null; public ?int $timeout = null; + /** + * Optional uid of a dedicated user which is used to run the OCR + * processing instead of impersonating the file owner. If this is + * null, the file owner is used (default behaviour). + */ + public ?string $processingUserId = null; } diff --git a/lib/Service/GlobalSettingsService.php b/lib/Service/GlobalSettingsService.php index 3bea562..204d009 100644 --- a/lib/Service/GlobalSettingsService.php +++ b/lib/Service/GlobalSettingsService.php @@ -54,13 +54,16 @@ public function getGlobalSettings() : GlobalSettings { foreach ($this->getProperties($settings) as $prop) { $key = $prop->getName(); $type = $prop->getType(); + $isNullable = $type !== null && $type->allowsNull(); if ($type instanceof \ReflectionNamedType && $type->getName() === 'int') { $intValue = $this->config->getValueInt(Application::APP_NAME, $key, 0); // 0 is used as the "not configured" sentinel: none of the integer settings // have a meaningful zero value, so 0 → null (not set). $settings->$key = $intValue > 0 ? $intValue : null; } else { - $settings->$key = $this->config->getValueString(Application::APP_NAME, $key); + $stringValue = $this->config->getValueString(Application::APP_NAME, $key); + // The empty string is used as the "not configured" sentinel for string settings. + $settings->$key = $stringValue === '' && $isNullable ? null : $stringValue; } } diff --git a/lib/Service/OcrService.php b/lib/Service/OcrService.php index b9b0112..9cb9cf3 100644 --- a/lib/Service/OcrService.php +++ b/lib/Service/OcrService.php @@ -32,6 +32,7 @@ use OCA\Files_Versions\Versions\IVersionManager; use OCA\WorkflowOcr\Exception\OcrNotPossibleException; use OCA\WorkflowOcr\Helper\IProcessingFileAccessor; +use OCA\WorkflowOcr\Model\GlobalSettings; use OCA\WorkflowOcr\Model\WorkflowSettings; use OCA\WorkflowOcr\OcrProcessors\IOcrProcessorFactory; use OCA\WorkflowOcr\OcrProcessors\OcrProcessorResult; @@ -136,7 +137,10 @@ public function runOcrProcessWithJobArgument($argument): void { /** @inheritdoc */ public function runOcrProcess(int $fileId, string $uid, WorkflowSettings $settings) : void { // TODO :: make private try { - $this->initUserEnvironment($uid); + $globalSettings = $this->globalSettingsService->getGlobalSettings(); + $processingUid = $this->determineProcessingUid($uid, $globalSettings); + + $this->initUserEnvironment($processingUid); $file = $this->getNode($fileId); @@ -155,7 +159,6 @@ public function runOcrProcess(int $fileId, string $uid, WorkflowSettings $settin } $ocrProcessor = $this->ocrProcessorFactory->create($file->getMimeType()); - $globalSettings = $this->globalSettingsService->getGlobalSettings(); $result = $ocrProcessor->ocrFile($file, $settings, $globalSettings); @@ -197,7 +200,7 @@ public function runOcrProcess(int $fileId, string $uid, WorkflowSettings $settin return; } - $this->doPostProcessing($file, $uid, $settings, $result, $fileMtime); + $this->doPostProcessing($file, $processingUid, $uid, $settings, $result, $fileMtime); } finally { $this->shutdownUserEnvironment(); } @@ -228,7 +231,33 @@ private function parseArguments($argument) : array { } /** - * * @param string $uid The owners userId of the file to be processed + * Determines which user the OCR process should be run with. By default this is + * the owner of the file to be processed. If an admin configured a dedicated + * processing user globally, this user is used instead (#385). + * + * @param string $uid The owners userId of the file to be processed + * @param GlobalSettings $globalSettings The globally configured settings + * @return string The userId to run the OCR process with + */ + private function determineProcessingUid(string $uid, GlobalSettings $globalSettings) : string { + $configuredUid = trim($globalSettings->processingUserId ?? ''); + + if ($configuredUid === '' || $configuredUid === $uid) { + return $uid; + } + + if (!$this->userManager->userExists($configuredUid)) { + $this->logger->warning('Configured OCR processing user \'{configuredUid}\' does not exist. Falling back to file owner \'{uid}\'.', ['configuredUid' => $configuredUid, 'uid' => $uid]); + return $uid; + } + + $this->logger->debug('Running OCR process as configured processing user {configuredUid} instead of file owner {uid}', ['configuredUid' => $configuredUid, 'uid' => $uid]); + + return $configuredUid; + } + + /** + * * @param string $uid The userId to run the OCR process with */ private function initUserEnvironment(string $uid) : void { /** @var IUser */ @@ -315,7 +344,7 @@ private function createNewFileVersion(string $filePath, string $ocrContent, ?int /** * @param File $file The file to set the label for - * @param string $uid The userId of the file owner + * @param string $uid The userId of the user running the OCR process * @param string $label The label to set */ private function setFileVersionsLabel(File $file, string $uid, string $label): void { @@ -352,7 +381,12 @@ private function setFileVersionsLabel(File $file, string $uid, string $label): v } } - private function doPostProcessing(Node $file, string $uid, WorkflowSettings $settings, OcrProcessorResult $result, ?int $fileMtime = null): void { + /** + * @param Node $file The OCR processed file + * @param string $processingUid The userId the OCR process is running with + * @param string $ownerUid The userId of the file owner (receiver of notifications) + */ + private function doPostProcessing(Node $file, string $processingUid, string $ownerUid, WorkflowSettings $settings, OcrProcessorResult $result, ?int $fileMtime = null): void { $this->processTagsAfterSuccessfulOcr($file, $settings); $fileId = $file->getId(); @@ -363,7 +397,7 @@ private function doPostProcessing(Node $file, string $uid, WorkflowSettings $set if ($result->getRecognizedText() !== '') { if ($settings->getKeepOriginalFileVersion() && $file->isUpdateable()) { // Add label to original file to prevent its expiry - $this->setFileVersionsLabel($file, $uid, self::FILE_VERSION_LABEL_VALUE); + $this->setFileVersionsLabel($file, $processingUid, self::FILE_VERSION_LABEL_VALUE); } $newFilePath = $this->determineNewFilePath($file, $originalFileExtension); @@ -378,7 +412,7 @@ private function doPostProcessing(Node $file, string $uid, WorkflowSettings $set $this->eventService->textRecognized($result, $file); if ($settings->getSendSuccessNotification()) { - $this->notificationService->createSuccessNotification($uid, $fileId); + $this->notificationService->createSuccessNotification($ownerUid, $fileId); } } diff --git a/src/components/GlobalSettings.vue b/src/components/GlobalSettings.vue index de399a1..0cdf8e7 100644 --- a/src/components/GlobalSettings.vue +++ b/src/components/GlobalSettings.vue @@ -57,6 +57,22 @@ @input="settings.timeout = Number($event.target.value) || null; save()"> +
+
+ {{ translate('OCR user:') }} +
+ {{ translate('User account which is used to run the OCR processing. If not set, the owner of the processed file is used.') }} +
+ {{ translate('Note:') }} {{ translate('This user needs write access to all files which should be OCR processed.') }} +
+
+ +
+
diff --git a/src/test/components/GlobalSettings.spec.js b/src/test/components/GlobalSettings.spec.js index fa1c6a2..f3df3c9 100644 --- a/src/test/components/GlobalSettings.spec.js +++ b/src/test/components/GlobalSettings.spec.js @@ -91,6 +91,52 @@ describe('Interaction tests', () => { })) }) + test('Should update settings when processingUserId is changed', async () => { + const initialMockSettings = { processingUserId: null } + getGlobalSettings.mockResolvedValueOnce(initialMockSettings) + + const afterSaveMockSettings = { processingUserId: 'ocruser' } + setGlobalSettings.mockResolvedValueOnce(afterSaveMockSettings) + + const wrapper = mount(GlobalSettings, mountOptions) + await new Promise(process.nextTick) + + const processingUserId = wrapper.find('input[name="processingUserId"]') + expect(processingUserId.element.value).toBe('') + + processingUserId.element.value = ' ocruser ' + await processingUserId.trigger('change') + + expect(wrapper.vm.settings.processingUserId).toBe('ocruser') + expect(setGlobalSettings).toHaveBeenCalledTimes(1) + expect(setGlobalSettings).toHaveBeenCalledWith(expect.objectContaining({ + processingUserId: 'ocruser', + })) + }) + + test('Should convert empty processingUserId to null', async () => { + const initialMockSettings = { processingUserId: 'ocruser' } + getGlobalSettings.mockResolvedValueOnce(initialMockSettings) + + const afterSaveMockSettings = { processingUserId: null } + setGlobalSettings.mockResolvedValueOnce(afterSaveMockSettings) + + const wrapper = mount(GlobalSettings, mountOptions) + await new Promise(process.nextTick) + + const processingUserId = wrapper.find('input[name="processingUserId"]') + expect(processingUserId.element.value).toBe('ocruser') + + processingUserId.element.value = ' ' + await processingUserId.trigger('change') + + expect(wrapper.vm.settings.processingUserId).toBeNull() + expect(setGlobalSettings).toHaveBeenCalledTimes(1) + expect(setGlobalSettings).toHaveBeenCalledWith(expect.objectContaining({ + processingUserId: null, + })) + }) + test('Should show error when save fails', async () => { const initialMockSettings = { processorCount: 2 } getGlobalSettings.mockResolvedValueOnce(initialMockSettings) diff --git a/tests/Unit/Controller/GlobalSettingsControllerTest.php b/tests/Unit/Controller/GlobalSettingsControllerTest.php index 51a8d4d..f8e50a5 100644 --- a/tests/Unit/Controller/GlobalSettingsControllerTest.php +++ b/tests/Unit/Controller/GlobalSettingsControllerTest.php @@ -29,6 +29,7 @@ use OCA\WorkflowOcr\Service\IGlobalSettingsService; use OCP\AppFramework\Http\JSONResponse; use OCP\IRequest; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -79,12 +80,53 @@ public function testSetSettingsCallsService() { $settings = [ 'processorCount' => 42, 'timeout' => 120, + 'processingUserId' => 'ocruser', ]; $this->globalSettingsService->expects($this->once()) ->method('setGlobalSettings') ->with($this->callback(function (GlobalSettings $settings) { - return $settings->processorCount === 42 && $settings->timeout === 120; + return $settings->processorCount === 42 + && $settings->timeout === 120 + && $settings->processingUserId === 'ocruser'; + })); + + $this->controller->setGlobalSettings($settings); + } + + #[DataProvider('dataProvider_EmptyProcessingUserIds')] + public function testSetSettingsMapsEmptyProcessingUserIdToNull($processingUserId) { + $settings = [ + 'processorCount' => 42, + 'processingUserId' => $processingUserId, + ]; + + $this->globalSettingsService->expects($this->once()) + ->method('setGlobalSettings') + ->with($this->callback(function (GlobalSettings $settings) { + return $settings->processingUserId === null; + })); + + $this->controller->setGlobalSettings($settings); + } + + public static function dataProvider_EmptyProcessingUserIds() { + return [ + [null], + [''], + [' '], + ]; + } + + public function testSetSettingsTrimsProcessingUserId() { + $settings = [ + 'processingUserId' => ' ocruser ', + ]; + + $this->globalSettingsService->expects($this->once()) + ->method('setGlobalSettings') + ->with($this->callback(function (GlobalSettings $settings) { + return $settings->processingUserId === 'ocruser'; })); $this->controller->setGlobalSettings($settings); diff --git a/tests/Unit/Service/GlobalSettingsServiceTest.php b/tests/Unit/Service/GlobalSettingsServiceTest.php index 41014fd..e63d5f6 100644 --- a/tests/Unit/Service/GlobalSettingsServiceTest.php +++ b/tests/Unit/Service/GlobalSettingsServiceTest.php @@ -50,27 +50,36 @@ public function testGetSettings_ReturnsCorrectSettings() { [Application::APP_NAME, 'processorCount', 0, 2], [Application::APP_NAME, 'timeout', 0, 30], ]); + $this->config->expects($this->any()) + ->method('getValueString') + ->willReturn('ocruser'); $settings = $this->globalSettingsService->getGlobalSettings(); $this->assertInstanceOf(GlobalSettings::class, $settings); $this->assertEquals(2, $settings->processorCount); $this->assertEquals(30, $settings->timeout); + $this->assertEquals('ocruser', $settings->processingUserId); } public function testGetSettings_ReturnsNullForUnsetValues() { // getValueInt returns 0 for keys that have never been stored. // 0 is not a valid value for processorCount or timeout, so it is - // treated as "not configured" and mapped to null. + // treated as "not configured" and mapped to null. The same applies + // to the empty string for string based settings. $this->config->expects($this->any()) ->method('getValueInt') ->willReturn(0); + $this->config->expects($this->any()) + ->method('getValueString') + ->willReturn(''); $settings = $this->globalSettingsService->getGlobalSettings(); $this->assertInstanceOf(GlobalSettings::class, $settings); $this->assertNull($settings->processorCount); $this->assertNull($settings->timeout); + $this->assertNull($settings->processingUserId); } public function testSetSettings_CallsConfigSetValueInt() { @@ -96,20 +105,41 @@ function (string $appName, string $key, int $value) use ($settings) { $this->globalSettingsService->setGlobalSettings($settings); } + public function testSetSettings_CallsConfigSetValueString() { + $settings = new GlobalSettings(); + $settings->processingUserId = 'ocruser'; + + $this->config->expects($this->once()) + ->method('setValueString') + ->willReturnCallback( + function (string $appName, string $key, string $value) { + $this->assertEquals(Application::APP_NAME, $appName); + $this->assertEquals('processingUserId', $key); + $this->assertEquals('ocruser', $value); + return true; + } + ); + + $this->globalSettingsService->setGlobalSettings($settings); + } + public function testSetSettings_DeletesKeyForNullValues() { $settings = new GlobalSettings(); $settings->processorCount = null; $settings->timeout = null; + $settings->processingUserId = null; $this->config->expects($this->never()) ->method('setValueInt'); + $this->config->expects($this->never()) + ->method('setValueString'); - $this->config->expects($this->exactly(2)) + $this->config->expects($this->exactly(3)) ->method('deleteKey') ->willReturnCallback( function (string $appName, string $key) { $this->assertEquals(Application::APP_NAME, $appName); - $this->assertContains($key, ['processorCount', 'timeout']); + $this->assertContains($key, ['processorCount', 'timeout', 'processingUserId']); } ); diff --git a/tests/Unit/Service/OcrServiceTest.php b/tests/Unit/Service/OcrServiceTest.php index 6904d0e..a76c9f0 100644 --- a/tests/Unit/Service/OcrServiceTest.php +++ b/tests/Unit/Service/OcrServiceTest.php @@ -485,6 +485,123 @@ public function testThrowsNoUserException_OnNonExistingUser() { $this->assertTrue($thrown); } + public function testRunsOcrProcessWithConfiguredProcessingUser() { + // #385: If an admin configured a dedicated OCR user, the whole processing + // (and therefore the resulting file version) has to run with this user + // instead of impersonating the file owner. + $settings = new WorkflowSettings('{"keepOriginalFileVersion": true, "sendSuccessNotification": true}'); + $globalSettings = new GlobalSettings(); + $globalSettings->processingUserId = 'ocruser'; + + $this->globalSettingsService->expects($this->once()) + ->method('getGlobalSettings') + ->willReturn($globalSettings); + + /** @var IUser|MockObject */ + $ocrUser = $this->createMock(IUser::class); + /** @var IUserManager|MockObject */ + $userManager = $this->createMock(IUserManager::class); + $userManager->method('userExists') + ->with('ocruser') + ->willReturn(true); + $userManager->method('get') + ->willReturnMap([ + ['ocruser', $ocrUser], + ['usr', $this->user], + ]); + + $this->userSession->expects($this->exactly(2)) + ->method('setUser') + ->willReturnCallback(function ($user) use ($ocrUser) { + // First call sets the OCR user, the last one resets the session + $this->assertTrue($user === $ocrUser || $user === null); + }); + + $this->filesystem->expects($this->once()) + ->method('init') + ->with('ocruser', '/ocruser/files'); + + // The file versions have to be read as the configured OCR user + $this->versionManager->expects($this->once()) + ->method('getVersionsForFile') + ->with($ocrUser, $this->rootFolderGetFirstNodeById42ReturnValue) + ->willReturn([]); + + // The file owner still has to be the receiver of the notification + $this->notificationService->expects($this->once()) + ->method('createSuccessNotification') + ->with('usr', 42); + + $this->ocrProcessor->expects($this->once()) + ->method('ocrFile') + ->willReturn(new OcrProcessorResult(true, 'someOcrProcessedFile', 'some recognized text')); + + $ocrService = new OcrService( + $this->ocrProcessorFactory, + $this->globalSettingsService, + $this->versionManager, + $this->systemTagObjectMapper, + $userManager, + $this->filesystem, + $this->userSession, + $this->rootFolder, + $this->eventService, + $this->viewFactory, + $this->processingFileAccessor, + $this->notificationService, + $this->logger); + + $ocrService->runOcrProcess(42, 'usr', $settings); + } + + public function testFallsBackToFileOwnerIfConfiguredProcessingUserDoesNotExist() { + $settings = new WorkflowSettings(); + $globalSettings = new GlobalSettings(); + $globalSettings->processingUserId = 'nonexistinguser'; + + $this->globalSettingsService->expects($this->once()) + ->method('getGlobalSettings') + ->willReturn($globalSettings); + + /** @var IUserManager|MockObject */ + $userManager = $this->createMock(IUserManager::class); + $userManager->method('userExists') + ->with('nonexistinguser') + ->willReturn(false); + $userManager->method('get') + ->with('usr') + ->willReturn($this->user); + + $this->filesystem->expects($this->once()) + ->method('init') + ->with('usr', '/usr/files'); + + $this->logger->expects($this->once()) + ->method('warning') + ->with($this->stringContains('does not exist'), ['configuredUid' => 'nonexistinguser', 'uid' => 'usr']); + + $this->ocrProcessor->expects($this->once()) + ->method('ocrFile') + ->willReturn(new OcrProcessorResult(true, 'someOcrProcessedFile', 'some recognized text')); + + $ocrService = new OcrService( + $this->ocrProcessorFactory, + $this->globalSettingsService, + $this->versionManager, + $this->systemTagObjectMapper, + $userManager, + $this->filesystem, + $this->userSession, + $this->rootFolder, + $this->eventService, + $this->viewFactory, + $this->processingFileAccessor, + $this->notificationService, + $this->logger); + + $ocrService->runOcrProcess(42, 'usr', $settings); + } + public function testCallsProcessingFileAccessor() { $settings = new WorkflowSettings(); $mimeType = 'application/pdf';