From 5eccea4c3dc9740dd7a2d0ad12a5572b08655738 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 20:44:28 +0000 Subject: [PATCH] feat: allow configuring a dedicated OCR user (#385) Automated OCR operations were always attributed to the owner of the processed file, because the app impersonates that user to get proper filesystem permissions. This makes automated OCR indistinguishable from a manual user edit in the file version history and the activity log. Add an optional global setting "OCR user". If it is set, the whole OCR processing (user session, filesystem and therefore the resulting file version) runs with that account instead of the file owner. If it is not set, nothing changes and the file owner is impersonated as before. Notifications are still sent to the file owner, and if the configured user does not exist the app logs a warning and falls back to the owner. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PjP5Wsw6uiKSBPiXp9N7sf --- README.md | 1 + lib/Controller/GlobalSettingsController.php | 2 + lib/Model/GlobalSettings.php | 6 + lib/Service/GlobalSettingsService.php | 5 +- lib/Service/OcrService.php | 50 ++++++-- src/components/GlobalSettings.vue | 16 +++ src/test/components/GlobalSettings.spec.js | 46 +++++++ .../GlobalSettingsControllerTest.php | 44 ++++++- .../Service/GlobalSettingsServiceTest.php | 36 +++++- tests/Unit/Service/OcrServiceTest.php | 117 ++++++++++++++++++ 10 files changed, 310 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index d03667f8..dc5f954b 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 e9fe9fed..6e1f8fec 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 29c8135c..e70b627a 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 3bea5625..204d0099 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 b9b0112c..9cb9cf3a 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 de399a1f..0cdf8e76 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 fa1c6a2f..f3df3c97 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 51a8d4d2..f8e50a56 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 41014fd6..e63d5f60 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 6904d0e1..a76c9f03 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';