From 36fef45a508e80ed9a449d1929c8e98b22890e1f Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:44:12 +0200 Subject: [PATCH 1/5] feat(audit): write audit log entries through the public audit event The existing audit log entries (app password created, replaced and deleted, project folder setup deactivated, integration reset, OpenProject URL and OAuth client set, Nextcloud OAuth client created) are now dispatched as CriticalActionPerformedEvent with named parameters and, on servers that support it, an operation. The messages keep their meaning; the misplaced quote in the app password messages is fixed. The operations are collected in the new OCA\OpenProject\AuditLog\Operation enum, with a short description of each for the documentation. Before, OpenProjectAPIService::logToAuditFile() created admin_audit's internal AuditLogger itself. Because that bypasses the admin_audit app, it had to guess whether the app was installed and configured, and only wrote when the audit log was the default data/audit.log file, log.condition listed admin_audit and the log level was at least 1. With a custom logfile_audit, a syslog or systemd audit log, or without log.condition, nothing was logged. AuditLogger is also not a public API: since Nextcloud 35 its constructor takes IAppConfig, so the direct construction fails there. Now admin_audit decides itself: its listener only runs when the app is enabled and writes to the audit log as configured, like every other audit entry. The ILogFactory dependency and the psalm suppression for AuditLogger are gone. isAdminAuditConfigSetCorrectly() is now only used for the hint in the admin settings and only checks that admin_audit is enabled, as the file path and log.condition no longer matter. The app password entry is now written by generateAppPasswordTokenForUser() itself, which is told whether it replaces an existing app password. The app password, the OpenProject client secret and the Nextcloud client secret are never logged. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- CHANGELOG.md | 1 + bootstrap.php | 1 + lib/AuditLog/Operation.php | 38 ++++ lib/Controller/ConfigController.php | 38 ++-- lib/Service/OpenProjectAPIService.php | 55 +++-- psalm.xml | 1 - tests/lib/AuditLog/AuditEvents.php | 22 ++ tests/lib/AuditLog/OperationTest.php | 25 +++ tests/lib/Controller/ConfigControllerTest.php | 161 ++++++++++++++ .../lib/Service/OpenProjectAPIServiceTest.php | 199 +++++++++++------- 10 files changed, 412 insertions(+), 129 deletions(-) create mode 100644 lib/AuditLog/Operation.php create mode 100644 tests/lib/AuditLog/AuditEvents.php create mode 100644 tests/lib/AuditLog/OperationTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index d671af15b..175cc3380 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Make OAuth2 Client class usage compatible with older Nextcloud versions [#1148](https://github.com/nextcloud/integration_openproject/pull/1148) - Add owner_file_name property in filesInfo endpoint response [#1189](https://github.com/nextcloud/integration_openproject/pull/1189) +- Write the audit log entries through the admin_audit app, with an operation and named parameters, whenever the app is enabled ### Removed diff --git a/bootstrap.php b/bootstrap.php index 050e0e925..c78427811 100644 --- a/bootstrap.php +++ b/bootstrap.php @@ -29,4 +29,5 @@ $classLoader = new ClassLoader(); $classLoader->addPsr4("OCA\\OpenProject\\", $rootDir . '/lib', true); +$classLoader->addPsr4("OCA\\OpenProject\\Tests\\", $rootDir . '/tests/lib', true); $classLoader->register(); diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php new file mode 100644 index 000000000..4496b7234 --- /dev/null +++ b/lib/AuditLog/Operation.php @@ -0,0 +1,38 @@ + 'An admin changed the OpenProject integration settings, such as the OpenProject URL or the OpenProject OAuth client', + self::IntegrationReset => 'An admin reset the OpenProject integration, which disconnects all users from OpenProject', + self::OauthClientCreated => 'An admin created or replaced the Nextcloud OAuth client used by OpenProject', + self::AppPasswordCreated => 'An app password was created or replaced for the OpenProject system user', + self::AppPasswordDeleted => 'The app password of the OpenProject system user was deleted', + self::ProjectFolderDeactivated => 'An admin deactivated the project folder setup', + }; + } +} diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index a0cc0db8b..022bc9b47 100755 --- a/lib/Controller/ConfigController.php +++ b/lib/Controller/ConfigController.php @@ -14,6 +14,7 @@ use OCA\OAuth2\Exceptions\ClientNotFoundException; use OCA\OAuth2\Service\ClientService; use OCA\OpenProject\AppInfo\Application; +use OCA\OpenProject\AuditLog\Operation; use OCA\OpenProject\Exception\OpenprojectGroupfolderSetupConflictException; use OCA\OpenProject\Service\OauthService; use OCA\OpenProject\Service\OpenProjectAPIService; @@ -185,16 +186,7 @@ private function setIntegrationConfig(array $values): array { if (!$this->userManager->userExists(Application::OPEN_PROJECT_ENTITIES_NAME)) { throw new NoUserException('User "' . Application::OPEN_PROJECT_ENTITIES_NAME . '" does not exists to create application password'); } - $appPassword = $this->openprojectAPIService->generateAppPasswordTokenForUser(); - if ($isAppPasswordBeingReplaced) { - $this->openprojectAPIService->logToAuditFile( - "Application password for user 'OpenProject has been replaced' with new password in application " . Application::APP_ID - ); - } else { - $this->openprojectAPIService->logToAuditFile( - "New application password for user 'OpenProject' has been created in application " . Application::APP_ID - ); - } + $appPassword = $this->openprojectAPIService->generateAppPasswordTokenForUser($isAppPasswordBeingReplaced); } $oldClientId = $oldClientSecret = ''; $oldOpenProjectOauthUrl = $this->config->getAppValue( @@ -293,8 +285,9 @@ private function setIntegrationConfig(array $values): array { if (key_exists('setup_app_password', $values) && $values['setup_app_password'] === false) { $this->openprojectAPIService->deleteAppPassword(); if (!$runningFullReset) { - $this->openprojectAPIService->logToAuditFile( - "Project folder setup has been deactivated in application " . Application::APP_ID + $this->openprojectAPIService->logToAuditLog( + Operation::ProjectFolderDeactivated, + 'Project folder setup has been deactivated in application ' . Application::APP_ID, ); } } @@ -321,8 +314,9 @@ private function setIntegrationConfig(array $values): array { // so setting `fresh_project_folder_setup` as true if ($runningFullReset) { $this->config->setAppValue(Application::APP_ID, 'fresh_project_folder_setup', "1"); - $this->openprojectAPIService->logToAuditFile( - "OpenProject Integration admin configuration has been reset in application " . Application::APP_ID + $this->openprojectAPIService->logToAuditLog( + Operation::IntegrationReset, + 'OpenProject Integration admin configuration has been reset in application ' . Application::APP_ID, ); } elseif (key_exists('setup_app_password', $values) && key_exists('setup_project_folder', $values)) { // for other cases when api has key 'setup_app_password' and 'setup_project_folder' we set it to false @@ -363,13 +357,16 @@ public function setAdminConfig(array $values): DataResponse { if (key_exists('openproject_instance_url', $values) && $values['openproject_instance_url'] && !$isOPOAuthCrdentialSet) { // sending admin audit log if admin has changed or added the openproject host url - $this->openprojectAPIService->logToAuditFile( - "OpenProject host url has been set to '" . $values['openproject_instance_url'] . "' in application " . Application::APP_ID + $this->openprojectAPIService->logToAuditLog( + Operation::IntegrationConfigured, + "OpenProject host url has been set to '%s' in application " . Application::APP_ID, + ['openprojectUrl' => (string)$values['openproject_instance_url']], ); } if ($isOPOAuthCrdentialSet) { - $this->openprojectAPIService->logToAuditFile( - "OpenProject OAuth credential 'openproject_client_id' and 'openproject_client_secret' has been set in application " . Application::APP_ID + $this->openprojectAPIService->logToAuditLog( + Operation::IntegrationConfigured, + "OpenProject OAuth credential 'openproject_client_id' and 'openproject_client_secret' has been set in application " . Application::APP_ID, ); } return new DataResponse($result); @@ -510,8 +507,9 @@ public function oauthRedirect(string $code = '', string $state = ''): RedirectRe */ public function autoOauthCreation(): DataResponse { $result = $this->recreateOauthClientInformation(); - $this->openprojectAPIService->logToAuditFile( - "Nextcloud OAuth credential has been set in application " . Application::APP_ID + $this->openprojectAPIService->logToAuditLog( + Operation::OauthClientCreated, + 'Nextcloud OAuth credential has been set in application ' . Application::APP_ID, ); return new DataResponse($result); } diff --git a/lib/Service/OpenProjectAPIService.php b/lib/Service/OpenProjectAPIService.php index 080ea97cb..7ca88a6aa 100644 --- a/lib/Service/OpenProjectAPIService.php +++ b/lib/Service/OpenProjectAPIService.php @@ -18,10 +18,10 @@ use OC\Authentication\Events\AppPasswordCreatedEvent; use OC\Authentication\Token\IProvider; use OC\User\NoUserException; -use OCA\AdminAudit\AuditLogger; use OCA\GroupFolders\Folder\FolderManager; use OCA\OIDCIdentityProvider\Exceptions\ClientNotFoundException; use OCA\OpenProject\AppInfo\Application; +use OCA\OpenProject\AuditLog\Operation; use OCA\OpenProject\Exception\OpenprojectAvatarErrorException; use OCA\OpenProject\Exception\OpenprojectErrorException; use OCA\OpenProject\Exception\OpenprojectGroupfolderSetupConflictException; @@ -56,7 +56,7 @@ use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; -use OCP\Log\ILogFactory; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\PreConditionNotMetException; use OCP\Security\ISecureRandom; use OCP\Server; @@ -126,7 +126,6 @@ class OpenProjectAPIService { */ private ISubAdmin $subAdminManager; private IDBConnection $db; - private ILogFactory $logFactory; private IManager $encryptionManager; /** @@ -143,7 +142,6 @@ class OpenProjectAPIService { private IProvider $tokenProvider; private ISecureRandom $random; private IEventDispatcher $eventDispatcher; - private AuditLogger $auditLogger; private TokenEventFactory $tokenEventFactory; public function __construct( @@ -164,7 +162,6 @@ public function __construct( IEventDispatcher $eventDispatcher, ISubAdmin $subAdminManager, IDBConnection $db, - ILogFactory $logFactory, IManager $encryptionManager, TokenEventFactory $tokenEventFactory, IUserSession $userSession, @@ -187,7 +184,6 @@ public function __construct( $this->random = $random; $this->eventDispatcher = $eventDispatcher; $this->db = $db; - $this->logFactory = $logFactory; $this->encryptionManager = $encryptionManager; $this->tokenEventFactory = $tokenEventFactory; $this->userSession = $userSession; @@ -1162,30 +1158,23 @@ public function isGroupfoldersAppSupported(): bool { } /** - * @param $auditLogMessage - * @return void + * Writes an entry to the admin audit log, if the admin_audit app is enabled + * + * The operation is only passed on to servers whose CriticalActionPerformedEvent supports it + * + * @param string $message format string whose %s placeholders are filled by the first parameters, in order + * @param array $parameters */ - public function logToAuditFile($auditLogMessage): void { - if ($this->isAdminAuditConfigSetCorrectly()) { - $this->auditLogger = new AuditLogger($this->logFactory, $this->config); - $this->auditLogger->info($auditLogMessage, - ['app' => 'admin_audit'] - ); + public function logToAuditLog(Operation $operation, string $message, array $parameters = []): void { + $arguments = [$message, $parameters, false]; + if (method_exists(CriticalActionPerformedEvent::class, 'getOperation')) { + $arguments[] = $operation->value; } + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent(...$arguments)); } public function isAdminAuditConfigSetCorrectly(): bool { - $logLevel = $this->config->getSystemValue('loglevel'); - $configAuditFile = $this->config->getSystemValue('logfile_audit'); - $logCondition = $this->config->getSystemValue('log.condition'); - // All the above config should be satisfied in order for admin audit log for the integration application - // if any of the config is failed to be set then we are not able to send the admin audit logging in the audit.log file - return ( - $this->appManager->isInstalled('admin_audit') && - $configAuditFile === $this->config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . '/audit.log' && - (isset($logCondition["apps"]) && in_array('admin_audit', $logCondition["apps"])) && - $logLevel >= 1 - ); + return $this->appManager->isInstalled('admin_audit'); } public function isTermsOfServiceAppEnabled(): bool { @@ -1330,9 +1319,10 @@ public function getPasswordLength(): int { } /** + * @param bool $replacesExisting whether an existing app password was deleted to create this one * @return string */ - public function generateAppPasswordTokenForUser(): string { + public function generateAppPasswordTokenForUser(bool $replacesExisting = false): string { $user = $this->userManager->get(Application::OPEN_PROJECT_ENTITIES_NAME); $userID = $user->getUID(); $token = $this->random->generate(self::getPasswordLength(), ISecureRandom::CHAR_ALPHANUMERIC.ISecureRandom::CHAR_SYMBOLS); @@ -1347,6 +1337,13 @@ public function generateAppPasswordTokenForUser(): string { $this->eventDispatcher->dispatchTyped( new AppPasswordCreatedEvent($generatedToken) ); + $this->logToAuditLog( + Operation::AppPasswordCreated, + $replacesExisting + ? "Application password for user '%s' has been replaced with new password in application " . Application::APP_ID + : "New application password for user '%s' has been created in application " . Application::APP_ID, + ['userId' => $userID, 'replaced' => $replacesExisting], + ); return $token; } @@ -1361,8 +1358,10 @@ public function deleteAppPassword(): void { foreach ($tokens as $token) { if ($token->getName() === Application::OPEN_PROJECT_ENTITIES_NAME) { $this->tokenProvider->invalidateTokenById(Application::OPEN_PROJECT_ENTITIES_NAME, $token->getId()); - $this->logToAuditFile( - "Application password for user 'OpenProject has been deleted' in application " . Application::APP_ID + $this->logToAuditLog( + Operation::AppPasswordDeleted, + "Application password for user '%s' has been deleted in application " . Application::APP_ID, + ['userId' => Application::OPEN_PROJECT_ENTITIES_NAME], ); } } diff --git a/psalm.xml b/psalm.xml index b954657c1..9030a1c5f 100644 --- a/psalm.xml +++ b/psalm.xml @@ -65,7 +65,6 @@ - diff --git a/tests/lib/AuditLog/AuditEvents.php b/tests/lib/AuditLog/AuditEvents.php new file mode 100644 index 000000000..1f8a0beb4 --- /dev/null +++ b/tests/lib/AuditLog/AuditEvents.php @@ -0,0 +1,22 @@ +getOperation() === $operation->value; + } +} diff --git a/tests/lib/AuditLog/OperationTest.php b/tests/lib/AuditLog/OperationTest.php new file mode 100644 index 000000000..25104ae24 --- /dev/null +++ b/tests/lib/AuditLog/OperationTest.php @@ -0,0 +1,25 @@ + [$operation], Operation::cases()); + } + + /** + * @dataProvider dataOperations + */ + public function testOperationFormat(Operation $operation): void { + $this->assertMatchesRegularExpression('/^integration_openproject\.[a-z][a-z0-9_]*\.[a-z][a-z0-9_]*$/', $operation->value); + $this->assertNotSame('', $operation->description()); + } +} diff --git a/tests/lib/Controller/ConfigControllerTest.php b/tests/lib/Controller/ConfigControllerTest.php index dc53c90f1..a8f5e4730 100644 --- a/tests/lib/Controller/ConfigControllerTest.php +++ b/tests/lib/Controller/ConfigControllerTest.php @@ -13,6 +13,7 @@ use OCA\OAuth2\Db\ClientMapper; use OCA\OAuth2\Service\ClientService; use OCA\OpenProject\AppInfo\Application; +use OCA\OpenProject\AuditLog\Operation; use OCA\OpenProject\Service\OauthService; use OCA\OpenProject\Service\OpenProjectAPIService; use OCA\OpenProject\Service\SettingsService; @@ -1852,4 +1853,164 @@ public function testUpdateIntegrationSuccess(string $authMethod, int $oauthClien $this->assertArrayHasKey('openproject_redirect_uri', $data); } } + + /** + * @param list}> $auditEntries filled with the audit log entries written through the service + */ + private function getApiServiceMockCollectingAuditEntries(array &$auditEntries): OpenProjectAPIService&MockObject { + $apiService = $this->createMock(OpenProjectAPIService::class); + $apiService->method('logToAuditLog') + ->willReturnCallback(function (Operation $operation, string $message, array $parameters = []) use (&$auditEntries): void { + $auditEntries[] = [$operation, $message, $parameters]; + }); + return $apiService; + } + + /** + * @param array $appValues + */ + private function getConfigMockWithAppValues(array $appValues): IConfig&MockObject { + $configMock = $this->createMock(IConfig::class); + $configMock->method('getAppValue') + ->willReturnCallback(fn (string $app, string $key, string $default = '') => $appValues[$key] ?? $default); + return $configMock; + } + + /** + * @param array $constructParams + */ + private function getConfigControllerForAuditLog(array &$auditEntries, array $appValues, array $constructParams = []): ConfigController { + $constructArgs = $this->getConfigControllerConstructArgs(array_merge([ + 'config' => $this->getConfigMockWithAppValues($appValues), + 'userManager' => $this->createMock(IUserManager::class), + 'openprojectAPIService' => $this->getApiServiceMockCollectingAuditEntries($auditEntries), + ], $constructParams)); + return new ConfigController(...$constructArgs); + } + + public function testSetAdminConfigLogsOpenProjectUrl(): void { + $auditEntries = []; + $controller = $this->getConfigControllerForAuditLog($auditEntries, [ + 'openproject_instance_url' => 'https://old.example.com', + 'authorization_method' => Application::AUTH_METHOD_OAUTH, + 'nc_oauth_client_id' => '12', + ]); + + $result = $controller->setAdminConfig(['openproject_instance_url' => 'https://openproject.example.com']); + + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + $this->assertSame([[ + Operation::IntegrationConfigured, + "OpenProject host url has been set to '%s' in application integration_openproject", + ['openprojectUrl' => 'https://openproject.example.com'], + ]], $auditEntries); + } + + public function testSetAdminConfigLogsOpenProjectOauthCredentialsWithoutSecret(): void { + $auditEntries = []; + $controller = $this->getConfigControllerForAuditLog($auditEntries, [ + 'openproject_instance_url' => 'https://openproject.example.com', + 'authorization_method' => Application::AUTH_METHOD_OAUTH, + ]); + + $result = $controller->setAdminConfig([ + 'openproject_instance_url' => 'https://openproject.example.com', + 'openproject_client_id' => 'op-client-id', + 'openproject_client_secret' => 'op-client-secret', + ]); + + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + $this->assertSame([[ + Operation::IntegrationConfigured, + "OpenProject OAuth credential 'openproject_client_id' and 'openproject_client_secret' has been set in application integration_openproject", + [], + ]], $auditEntries); + } + + public function testSetAdminConfigLogsProjectFolderDeactivation(): void { + $auditEntries = []; + $controller = $this->getConfigControllerForAuditLog($auditEntries, [ + 'openproject_instance_url' => 'https://openproject.example.com', + 'authorization_method' => Application::AUTH_METHOD_OAUTH, + ]); + + $result = $controller->setAdminConfig(['setup_app_password' => false]); + + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + $this->assertSame([[ + Operation::ProjectFolderDeactivated, + 'Project folder setup has been deactivated in application integration_openproject', + [], + ]], $auditEntries); + } + + public function testSetAdminConfigWithUnknownSettingDoesNotLog(): void { + $auditEntries = []; + $controller = $this->getConfigControllerForAuditLog($auditEntries, []); + + $result = $controller->setAdminConfig(['openproject_instance_url' => 'https://openproject.example.com', 'unknown' => 'value']); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $result->getStatus()); + $this->assertSame([], $auditEntries); + } + + public function testResetIntegrationLogsReset(): void { + $auditEntries = []; + $controller = $this->getConfigControllerForAuditLog($auditEntries, [ + 'openproject_instance_url' => 'https://openproject.example.com', + 'authorization_method' => Application::AUTH_METHOD_OAUTH, + ]); + + $result = $controller->resetIntegration(); + + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + $this->assertSame([[ + Operation::IntegrationReset, + 'OpenProject Integration admin configuration has been reset in application integration_openproject', + [], + ]], $auditEntries); + } + + public function testAutoOauthCreationLogsOauthClientCreation(): void { + $auditEntries = []; + $oauthServiceMock = $this->createMock(OauthService::class); + $oauthServiceMock->method('createNcOauthClient')->willReturn([ + 'id' => 34, + 'nextcloud_oauth_client_name' => 'OpenProject client', + 'openproject_redirect_uri' => 'https://openproject.example.com/oauth_clients/client-id/callback', + 'nextcloud_client_id' => 'client-id', + 'nextcloud_client_secret' => 'client-secret', + ]); + $controller = $this->getConfigControllerForAuditLog($auditEntries, [ + 'openproject_instance_url' => 'https://openproject.example.com', + ], ['oauthService' => $oauthServiceMock]); + + $controller->autoOauthCreation(); + + $this->assertSame([[ + Operation::OauthClientCreated, + 'Nextcloud OAuth credential has been set in application integration_openproject', + [], + ]], $auditEntries); + } + + public function testSetAdminConfigPassesAppPasswordReplacementToService(): void { + $auditEntries = []; + $apiService = $this->getApiServiceMockCollectingAuditEntries($auditEntries); + $apiService->method('hasAppPassword')->willReturn(true); + $apiService->expects($this->once()) + ->method('generateAppPasswordTokenForUser') + ->with(true) + ->willReturn('app-password'); + $userManagerMock = $this->createMock(IUserManager::class); + $userManagerMock->method('userExists')->willReturn(true); + $controller = $this->getConfigControllerForAuditLog($auditEntries, [ + 'openproject_instance_url' => 'https://openproject.example.com', + 'authorization_method' => Application::AUTH_METHOD_OAUTH, + ], ['openprojectAPIService' => $apiService, 'userManager' => $userManagerMock]); + + $result = $controller->setAdminConfig(['setup_app_password' => true]); + + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + } } diff --git a/tests/lib/Service/OpenProjectAPIServiceTest.php b/tests/lib/Service/OpenProjectAPIServiceTest.php index 7abae88f7..77c5c0a30 100644 --- a/tests/lib/Service/OpenProjectAPIServiceTest.php +++ b/tests/lib/Service/OpenProjectAPIServiceTest.php @@ -20,9 +20,11 @@ use OCA\GroupFolders\Folder\FolderManager; use OCA\OIDCIdentityProvider\Db\Client as OIDCClient; use OCA\OpenProject\AppInfo\Application; +use OCA\OpenProject\AuditLog\Operation; use OCA\OpenProject\Exception\OpenprojectErrorException; use OCA\OpenProject\Exception\OpenprojectResponseException; use OCA\OpenProject\OIDCClientMapper; +use OCA\OpenProject\Tests\AuditLog\AuditEvents; use OCA\OpenProject\TokenEventFactory; use OCA\TermsOfService\Db\Mapper\SignatoryMapper; use OCA\UserOIDC\Event\ExchangedTokenRequestedEvent; @@ -33,6 +35,7 @@ use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\Encryption\IManager; +use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Folder; use OCP\Files\IRootFolder; @@ -56,7 +59,7 @@ use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; -use OCP\Log\ILogFactory; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\ISecureRandom; use OCP\UserInterface; use phpmock\phpunit\PHPMock; @@ -690,7 +693,6 @@ private function getOpenProjectAPIServiceConstructArgs(array $constructParams = 'eventDispatcher' => $this->createMock(IEventDispatcher::class), 'subAdmin' => $this->createMock(ISubAdmin::class), 'dbConnection' => $this->createMock(IDBConnection::class), - 'logFactory' => $this->createMock(ILogFactory::class), 'manager' => $this->createMock(IManager::class), 'tokenEventFactory' => $this->createMock(TokenEventFactory::class), 'userSession' => $this->createMock(IUserSession::class), @@ -2499,6 +2501,91 @@ public function testIsGroupFolderNotSetup( $this->assertFalse($service->isProjectFoldersSetupComplete()); } + /** + * @param list $auditEvents filled with the audit log events dispatched through the returned mock + */ + private function getEventDispatcherMockCollectingAuditEvents(array &$auditEvents): IEventDispatcher&MockObject { + $eventDispatcherMock = $this->createMock(IEventDispatcher::class); + $eventDispatcherMock->method('dispatchTyped') + ->willReturnCallback(function (Event $event) use (&$auditEvents): void { + if ($event instanceof CriticalActionPerformedEvent) { + $auditEvents[] = $event; + } + }); + return $eventDispatcherMock; + } + + /** + * @param list}> $expected + * @param list $auditEvents + */ + private function assertAuditEvents(array $expected, array $auditEvents): void { + $this->assertSame( + array_map(static fn (array $entry): array => [$entry[1], $entry[2]], $expected), + array_map( + static fn (CriticalActionPerformedEvent $event): array => [$event->getLogMessage(), $event->getParameters()], + $auditEvents, + ), + ); + foreach ($auditEvents as $index => $event) { + $this->assertTrue(AuditEvents::hasOperation($event, $expected[$index][0])); + $this->assertFalse($event->getObfuscateParameters()); + } + } + + public function testLogToAuditLog(): void { + $auditEvents = []; + $service = $this->getOpenProjectAPIServiceMock([], [ + 'eventDispatcher' => $this->getEventDispatcherMockCollectingAuditEvents($auditEvents), + ]); + + $service->logToAuditLog(Operation::IntegrationReset, 'Message with %s', ['value' => 'parameter', 'extra' => true]); + + $this->assertAuditEvents([ + [Operation::IntegrationReset, 'Message with %s', ['value' => 'parameter', 'extra' => true]], + ], $auditEvents); + } + + public function generateAppPasswordAuditLogDataProvider(): array { + return [ + 'new app password' => [ + false, + "New application password for user '%s' has been created in application integration_openproject", + ], + 'replaced app password' => [ + true, + "Application password for user '%s' has been replaced with new password in application integration_openproject", + ], + ]; + } + + /** + * @dataProvider generateAppPasswordAuditLogDataProvider + */ + public function testGenerateAppPasswordTokenForUserLogsWithoutPassword(bool $replacesExisting, string $expectedMessage): void { + $userMock = $this->createMock(IUser::class); + $userMock->method('getUID')->willReturn(Application::OPEN_PROJECT_ENTITIES_NAME); + $userManagerMock = $this->createMock(IUserManager::class); + $userManagerMock->method('get')->willReturn($userMock); + $secureRandomMock = $this->createMock(ISecureRandom::class); + $secureRandomMock->method('generate')->willReturn('generated-app-password'); + $tokenProviderMock = $this->createMock(IProvider::class); + $tokenProviderMock->method('generateToken')->willReturn($this->createMock(IToken::class)); + $auditEvents = []; + $service = $this->getOpenProjectAPIServiceMock([], [ + 'userManager' => $userManagerMock, + 'secureRandom' => $secureRandomMock, + 'provider' => $tokenProviderMock, + 'eventDispatcher' => $this->getEventDispatcherMockCollectingAuditEvents($auditEvents), + ]); + + $this->assertSame('generated-app-password', $service->generateAppPasswordTokenForUser($replacesExisting)); + + $this->assertAuditEvents([ + [Operation::AppPasswordCreated, $expectedMessage, ['userId' => Application::OPEN_PROJECT_ENTITIES_NAME, 'replaced' => $replacesExisting]], + ], $auditEvents); + } + public function testGenerateAppPasswordTokenForUser(): void { $userMock = $this->getMockBuilder(IUser::class)->getMock(); $userMock @@ -2812,10 +2899,12 @@ public function testProjectFolderDeleteAppPassword(): void { ->method('getTokenByUser') ->with(Application::OPEN_PROJECT_ENTITIES_NAME) ->willReturn([$tokenMock1,$tokenMock2,$tokenMock3,$tokenMock4,$tokenMock5]); + $auditEvents = []; $service = $this->getOpenProjectAPIServiceMock( ['hasAppPassword'], [ 'provider' => $tokenProviderMock, + 'eventDispatcher' => $this->getEventDispatcherMockCollectingAuditEvents($auditEvents), ], ); $service->method('hasAppPassword')->willReturn(true); @@ -2828,6 +2917,30 @@ public function testProjectFolderDeleteAppPassword(): void { ] ); $service->deleteAppPassword(); + $deletedEntry = [ + Operation::AppPasswordDeleted, + "Application password for user '%s' has been deleted in application integration_openproject", + ['userId' => Application::OPEN_PROJECT_ENTITIES_NAME], + ]; + $this->assertAuditEvents([$deletedEntry, $deletedEntry], $auditEvents); + } + + public function testDeleteAppPasswordWithoutAppPasswordDoesNotLog(): void { + $tokenProviderMock = $this->createMock(IProvider::class); + $tokenProviderMock->expects($this->never())->method('invalidateTokenById'); + $auditEvents = []; + $service = $this->getOpenProjectAPIServiceMock( + ['hasAppPassword'], + [ + 'provider' => $tokenProviderMock, + 'eventDispatcher' => $this->getEventDispatcherMockCollectingAuditEvents($auditEvents), + ], + ); + $service->method('hasAppPassword')->willReturn(false); + + $service->deleteAppPassword(); + + $this->assertSame([], $auditEvents); } /** @@ -4389,99 +4502,25 @@ public function testGetWorkPackageInfoForNoUserAccessToken(): void { public function auditLogDataProvider(): array { return [ - [ - // log level less than 1 - 0, - \OC::$SERVERROOT . '/data/audit.log', - [], - true, - false - ], - [ - // wrong path to audit file - 1, - 'wrong-path-to-audit-log/audit.log', - [], - true, - false - ], - [ - // no audit apps in apps section - 1, - '/audit.log', - ['apps' => ['no_audit_app']], - true, - false - ], - [ - // multiple values including apps with no audit apps in apps section - 1, - '/audit.log', - ['apps' => ['no_audit_app'], 'others' => []], - true, - false - ], - [ - // all values configured correctly - 1, - '/audit.log', - ['apps' => ['admin_audit']], - true, - true - ], - [ - // admin_audit app not installed - 1, - '/audit.log', - ['apps' => ['admin_audit']], - false, - false - ], + 'admin_audit app enabled' => [true], + 'admin_audit app not enabled' => [false], ]; } /** * @dataProvider auditLogDataProvider - * @param int $logLevel - * @param string $pathToAuditLog - * @param array $logCondition - * @param bool $isAdminAuditAppInstalled - * @param bool $expectedResult - * - * @return void */ - - public function testIsAdminAuditConfigSetCorrectly( - int $logLevel, - string $pathToAuditLog, - array $logCondition, - bool $isAdminAuditAppInstalled, - bool $expectedResult - ): void { - $configMock = $this->getMockBuilder(IConfig::class)->getMock(); + public function testIsAdminAuditConfigSetCorrectly(bool $isAdminAuditAppInstalled): void { $iAppManagerMock = $this->getMockBuilder(IAppManager::class)->getMock(); - $configMock - ->method('getSystemValue') - ->willReturnMap([ - ['loglevel', '', $logLevel], - ['logfile_audit', '', $pathToAuditLog], - ['log.condition', '', $logCondition] - ] - ); - $userManagerMock = $this->getMockBuilder(IUserManager::class) - ->getMock(); $iAppManagerMock->method('isInstalled')->with('admin_audit') ->willReturn($isAdminAuditAppInstalled); $service = $this->getOpenProjectAPIServiceMock( [], [ - 'userManager' => $userManagerMock, 'appManager' => $iAppManagerMock, - 'config' => $configMock, ], ); - $actualResult = $service->isAdminAuditConfigSetCorrectly(); - $this->assertEquals($expectedResult, $actualResult); + $this->assertSame($isAdminAuditAppInstalled, $service->isAdminAuditConfigSetCorrectly()); } public function serverSideEncryptionEnabledDataProvider(): array { From f037f3d19e21f4ed6630b8eb81b27e2bdce00013 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:46:00 +0200 Subject: [PATCH 2/5] feat(settings): log integration setup, updates and OAuth clients Saving the integration settings is now logged for every way they are saved: the admin settings form, the setup API (POST, PATCH and DELETE /setup) and the reset. The entry lists the names of the saved settings, the OpenProject URL, the authorization method and whether all users were disconnected from OpenProject because the OAuth client or the authorization method changed. The reset entry also tells whether users were disconnected. Setting values other than the URL and the authorization method are not logged, so the OpenProject client secret never ends up in the audit log. This replaces the two entries written only by the admin settings form, "OpenProject host url has been set" and "OpenProject OAuth credential ... has been set", which missed the setup API and changes of the OIDC settings. The saved setting names are logged instead of comparing each value with the stored one, which would need an extra app config read per setting. Creating the Nextcloud OAuth client used by OpenProject is now logged wherever it happens (the admin settings button and the setup API), with the ID of the client and whether it replaced the previous one, instead of only from the admin settings button. The client secret is not logged. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- CHANGELOG.md | 1 + lib/Controller/ConfigController.php | 59 ++++---- tests/lib/Controller/ConfigControllerTest.php | 132 ++++++++++++++++-- 3 files changed, 157 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 175cc3380..5ccc4041e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Added - Support Nextcloud 35 +- Log integration settings saved through the setup API and every new Nextcloud OAuth client for OpenProject to the audit log ### Fixed - Keep the admin settings when Nextcloud replays the migrations to check the database schema (`occ db:schema:check`, every `occ upgrade` on Nextcloud 35) diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index 022bc9b47..e6a2fea44 100755 --- a/lib/Controller/ConfigController.php +++ b/lib/Controller/ConfigController.php @@ -295,10 +295,12 @@ private function setIntegrationConfig(array $values): array { $resetOpenProjectClient = ((key_exists('openproject_client_id', $values) && $values['openproject_client_id'] !== $oldClientId) || (key_exists('openproject_client_secret', $values) && $values['openproject_client_secret'] !== $oldClientSecret)); + $usersDisconnected = false; if ($resetOpenProjectClient || $runningFullResetWithOAuth2Auth || $switchingOAuthToOIDC) { $this->userManager->callForAllUsers(function (IUser $user) { $this->clearUserInfo($user->getUID()); }); + $usersDisconnected = true; if ($switchingOAuthToOIDC) { $this->resetOauth2Configs(); @@ -308,6 +310,7 @@ private function setIntegrationConfig(array $values): array { $this->userManager->callForAllUsers(function (IUser $user) { $this->clearUserInfo($user->getUID()); }); + $usersDisconnected = true; } // whenever doing full reset we want the project folder switch state to be "on" in the UI @@ -317,6 +320,7 @@ private function setIntegrationConfig(array $values): array { $this->openprojectAPIService->logToAuditLog( Operation::IntegrationReset, 'OpenProject Integration admin configuration has been reset in application ' . Application::APP_ID, + ['usersDisconnected' => $usersDisconnected], ); } elseif (key_exists('setup_app_password', $values) && key_exists('setup_project_folder', $values)) { // for other cases when api has key 'setup_app_password' and 'setup_project_folder' we set it to false @@ -324,6 +328,21 @@ private function setIntegrationConfig(array $values): array { $this->config->setAppValue(Application::APP_ID, 'fresh_project_folder_setup', "0"); } + $savedSettings = array_values(array_diff(array_keys($values), ['setup_project_folder', 'setup_app_password'])); + if (!$runningFullReset && $savedSettings !== []) { + // only the names of the settings are logged, never the OpenProject client secret + $this->openprojectAPIService->logToAuditLog( + Operation::IntegrationConfigured, + "OpenProject integration settings %s have been saved for the OpenProject instance '%s' in application " . Application::APP_ID, + [ + 'settings' => implode(', ', $savedSettings), + 'openprojectUrl' => key_exists('openproject_instance_url', $values) ? (string)$values['openproject_instance_url'] : $oldOpenProjectOauthUrl, + 'authorizationMethod' => key_exists('authorization_method', $values) ? (string)$values['authorization_method'] : $oldAuthMethod, + 'usersDisconnected' => $usersDisconnected, + ], + ); + } + return [ "status" => OpenProjectAPIService::isAdminConfigOk($this->config), "oPUserAppPassword" => $appPassword, @@ -349,26 +368,6 @@ public function setAdminConfig(array $values): DataResponse { } } $result = $this->setIntegrationConfig($values); - $isOPOAuthCrdentialSet = key_exists('openproject_client_id', $values) && - key_exists('openproject_client_secret', $values) && - $values['openproject_client_id'] && - $values['openproject_client_secret']; - - if (key_exists('openproject_instance_url', $values) && - $values['openproject_instance_url'] && !$isOPOAuthCrdentialSet) { - // sending admin audit log if admin has changed or added the openproject host url - $this->openprojectAPIService->logToAuditLog( - Operation::IntegrationConfigured, - "OpenProject host url has been set to '%s' in application " . Application::APP_ID, - ['openprojectUrl' => (string)$values['openproject_instance_url']], - ); - } - if ($isOPOAuthCrdentialSet) { - $this->openprojectAPIService->logToAuditLog( - Operation::IntegrationConfigured, - "OpenProject OAuth credential 'openproject_client_id' and 'openproject_client_secret' has been set in application " . Application::APP_ID, - ); - } return new DataResponse($result); } catch (OpenprojectGroupfolderSetupConflictException $e) { return new DataResponse([ @@ -507,14 +506,13 @@ public function oauthRedirect(string $code = '', string $state = ''): RedirectRe */ public function autoOauthCreation(): DataResponse { $result = $this->recreateOauthClientInformation(); - $this->openprojectAPIService->logToAuditLog( - Operation::OauthClientCreated, - 'Nextcloud OAuth credential has been set in application ' . Application::APP_ID, - ); return new DataResponse($result); } - private function deleteOauthClient(): void { + /** + * @return bool whether an OAuth client was configured + */ + private function deleteOauthClient(): bool { $oauthClientInternalId = $this->config->getAppValue( Application::APP_ID, 'nc_oauth_client_id', '' ); @@ -525,7 +523,9 @@ private function deleteOauthClient(): void { } catch (ClientNotFoundException $e) { } $this->config->deleteAppValue(Application::APP_ID, 'nc_oauth_client_id'); + return true; } + return false; } /** @@ -699,10 +699,17 @@ public function resetIntegration(): DataResponse { * @return array */ private function recreateOauthClientInformation(): array { - $this->deleteOauthClient(); + $replaced = $this->deleteOauthClient(); $opUrl = $this->config->getAppValue(Application::APP_ID, 'openproject_instance_url', ''); $clientInfo = $this->oauthService->createNcOauthClient('OpenProject client', rtrim($opUrl, '/') .'/oauth_clients/%s/callback'); $this->config->setAppValue(Application::APP_ID, 'nc_oauth_client_id', $clientInfo['id']); + $this->openprojectAPIService->logToAuditLog( + Operation::OauthClientCreated, + $replaced + ? 'Nextcloud OAuth client %s for OpenProject has replaced the previous one in application ' . Application::APP_ID + : 'Nextcloud OAuth client %s for OpenProject has been created in application ' . Application::APP_ID, + ['oauthClientId' => (int)$clientInfo['id'], 'replaced' => $replaced], + ); unset($clientInfo['id']); return $clientInfo; } diff --git a/tests/lib/Controller/ConfigControllerTest.php b/tests/lib/Controller/ConfigControllerTest.php index a8f5e4730..9f40fbdde 100644 --- a/tests/lib/Controller/ConfigControllerTest.php +++ b/tests/lib/Controller/ConfigControllerTest.php @@ -1888,7 +1888,7 @@ private function getConfigControllerForAuditLog(array &$auditEntries, array $app return new ConfigController(...$constructArgs); } - public function testSetAdminConfigLogsOpenProjectUrl(): void { + public function testSetAdminConfigLogsSavedSettings(): void { $auditEntries = []; $controller = $this->getConfigControllerForAuditLog($auditEntries, [ 'openproject_instance_url' => 'https://old.example.com', @@ -1901,17 +1901,26 @@ public function testSetAdminConfigLogsOpenProjectUrl(): void { $this->assertSame(Http::STATUS_OK, $result->getStatus()); $this->assertSame([[ Operation::IntegrationConfigured, - "OpenProject host url has been set to '%s' in application integration_openproject", - ['openprojectUrl' => 'https://openproject.example.com'], + "OpenProject integration settings %s have been saved for the OpenProject instance '%s' in application integration_openproject", + [ + 'settings' => 'openproject_instance_url', + 'openprojectUrl' => 'https://openproject.example.com', + 'authorizationMethod' => Application::AUTH_METHOD_OAUTH, + 'usersDisconnected' => false, + ], ]], $auditEntries); } public function testSetAdminConfigLogsOpenProjectOauthCredentialsWithoutSecret(): void { $auditEntries = []; + $userManagerMock = $this->createMock(IUserManager::class); + $userManagerMock->expects($this->once())->method('callForAllUsers'); $controller = $this->getConfigControllerForAuditLog($auditEntries, [ 'openproject_instance_url' => 'https://openproject.example.com', 'authorization_method' => Application::AUTH_METHOD_OAUTH, - ]); + 'openproject_client_id' => 'old-client-id', + 'openproject_client_secret' => 'old-client-secret', + ], ['userManager' => $userManagerMock]); $result = $controller->setAdminConfig([ 'openproject_instance_url' => 'https://openproject.example.com', @@ -1922,9 +1931,15 @@ public function testSetAdminConfigLogsOpenProjectOauthCredentialsWithoutSecret() $this->assertSame(Http::STATUS_OK, $result->getStatus()); $this->assertSame([[ Operation::IntegrationConfigured, - "OpenProject OAuth credential 'openproject_client_id' and 'openproject_client_secret' has been set in application integration_openproject", - [], + "OpenProject integration settings %s have been saved for the OpenProject instance '%s' in application integration_openproject", + [ + 'settings' => 'openproject_instance_url, openproject_client_id, openproject_client_secret', + 'openprojectUrl' => 'https://openproject.example.com', + 'authorizationMethod' => Application::AUTH_METHOD_OAUTH, + 'usersDisconnected' => true, + ], ]], $auditEntries); + $this->assertStringNotContainsString('op-client-secret', json_encode($auditEntries)); } public function testSetAdminConfigLogsProjectFolderDeactivation(): void { @@ -1934,6 +1949,7 @@ public function testSetAdminConfigLogsProjectFolderDeactivation(): void { 'authorization_method' => Application::AUTH_METHOD_OAUTH, ]); + // only the project folder flag is sent, so no settings are logged as saved $result = $controller->setAdminConfig(['setup_app_password' => false]); $this->assertSame(Http::STATUS_OK, $result->getStatus()); @@ -1967,10 +1983,106 @@ public function testResetIntegrationLogsReset(): void { $this->assertSame([[ Operation::IntegrationReset, 'OpenProject Integration admin configuration has been reset in application integration_openproject', - [], + ['usersDisconnected' => true], ]], $auditEntries); } + public function testSetUpIntegrationLogsSettingsAndOauthClient(): void { + $auditEntries = []; + $oauthServiceMock = $this->createMock(OauthService::class); + $oauthServiceMock->method('createNcOauthClient')->willReturn([ + 'id' => 34, + 'nextcloud_oauth_client_name' => 'OpenProject client', + 'openproject_redirect_uri' => 'https://openproject.example.com/oauth_clients/client-id/callback', + 'nextcloud_client_id' => 'client-id', + 'nextcloud_client_secret' => 'nc-client-secret', + ]); + $settingsServiceMock = $this->createMock(SettingsService::class); + $controller = $this->getConfigControllerForAuditLog($auditEntries, [], [ + 'oauthService' => $oauthServiceMock, + 'settingsService' => $settingsServiceMock, + ]); + + $result = $controller->setUpIntegration([ + 'openproject_instance_url' => 'https://openproject.example.com', + 'authorization_method' => Application::AUTH_METHOD_OAUTH, + 'openproject_client_id' => 'op-client-id', + 'openproject_client_secret' => 'op-client-secret', + 'default_enable_navigation' => false, + 'default_enable_unified_search' => false, + 'setup_project_folder' => false, + 'setup_app_password' => false, + ]); + + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + $this->assertSame([ + [ + Operation::ProjectFolderDeactivated, + 'Project folder setup has been deactivated in application integration_openproject', + [], + ], + [ + Operation::IntegrationConfigured, + "OpenProject integration settings %s have been saved for the OpenProject instance '%s' in application integration_openproject", + [ + 'settings' => 'openproject_instance_url, authorization_method, openproject_client_id, openproject_client_secret, default_enable_navigation, default_enable_unified_search', + 'openprojectUrl' => 'https://openproject.example.com', + 'authorizationMethod' => Application::AUTH_METHOD_OAUTH, + 'usersDisconnected' => true, + ], + ], + [ + Operation::OauthClientCreated, + 'Nextcloud OAuth client %s for OpenProject has been created in application integration_openproject', + ['oauthClientId' => 34, 'replaced' => false], + ], + ], $auditEntries); + $encodedEntries = json_encode($auditEntries); + $this->assertStringNotContainsString('op-client-secret', $encodedEntries); + $this->assertStringNotContainsString('nc-client-secret', $encodedEntries); + } + + public function testUpdateIntegrationLogsSavedSettings(): void { + $auditEntries = []; + $oauthServiceMock = $this->createMock(OauthService::class); + $oauthServiceMock->method('getClientInfo')->willReturn(['id' => 12]); + $oauthServiceMock->expects($this->never())->method('createNcOauthClient'); + $controller = $this->getConfigControllerForAuditLog($auditEntries, [ + 'openproject_instance_url' => 'https://openproject.example.com', + 'authorization_method' => Application::AUTH_METHOD_OAUTH, + 'nc_oauth_client_id' => '12', + ], [ + 'oauthService' => $oauthServiceMock, + 'settingsService' => $this->createMock(SettingsService::class), + ]); + + $result = $controller->updateIntegration(['default_enable_navigation' => true]); + + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + $this->assertSame([[ + Operation::IntegrationConfigured, + "OpenProject integration settings %s have been saved for the OpenProject instance '%s' in application integration_openproject", + [ + 'settings' => 'default_enable_navigation', + 'openprojectUrl' => 'https://openproject.example.com', + 'authorizationMethod' => Application::AUTH_METHOD_OAUTH, + 'usersDisconnected' => false, + ], + ]], $auditEntries); + } + + public function testUpdateIntegrationWithInvalidSettingsDoesNotLog(): void { + $auditEntries = []; + $settingsServiceMock = $this->createMock(SettingsService::class); + $settingsServiceMock->method('validateAdminSettingsForm')->willThrowException(new \InvalidArgumentException('invalid')); + $controller = $this->getConfigControllerForAuditLog($auditEntries, [], ['settingsService' => $settingsServiceMock]); + + $result = $controller->updateIntegration(['openproject_instance_url' => 'invalid']); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $result->getStatus()); + $this->assertSame([], $auditEntries); + } + public function testAutoOauthCreationLogsOauthClientCreation(): void { $auditEntries = []; $oauthServiceMock = $this->createMock(OauthService::class); @@ -1983,15 +2095,17 @@ public function testAutoOauthCreationLogsOauthClientCreation(): void { ]); $controller = $this->getConfigControllerForAuditLog($auditEntries, [ 'openproject_instance_url' => 'https://openproject.example.com', + 'nc_oauth_client_id' => '12', ], ['oauthService' => $oauthServiceMock]); $controller->autoOauthCreation(); $this->assertSame([[ Operation::OauthClientCreated, - 'Nextcloud OAuth credential has been set in application integration_openproject', - [], + 'Nextcloud OAuth client %s for OpenProject has replaced the previous one in application integration_openproject', + ['oauthClientId' => 34, 'replaced' => true], ]], $auditEntries); + $this->assertStringNotContainsString('client-secret', json_encode($auditEntries)); } public function testSetAdminConfigPassesAppPasswordReplacementToService(): void { From 9703f32ef39f1b75e4d3f185beb9230b290b9459 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:48:09 +0200 Subject: [PATCH 3/5] feat(project-folder): log project folder setup and signed terms of service Setting up the project folder is now written to the audit log with the groupfolder ID, the OpenProject system user and its two groups. Creating the user and the groups is already logged by admin_audit, but making the user group admin of them and creating the groupfolder whose advanced permissions the user manages were not. createGroupfolder() now returns the ID of the groupfolder for this. Each terms of service signed on behalf of the OpenProject system user is logged with the terms ID. This covers terms signed by an admin, by the project folder setup and automatically by the app when the terms change. Terms that were already signed are not logged again. Logging happens in signTermsOfServiceForUserOpenProject() instead of the admin endpoint to cover all of these. To test it, the signatory mapper can be passed in, as for isAllTermsOfServiceSignedForUserOpenProject(), and the signatory is created in its own method, as the terms_of_service app is not available in the unit tests. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- CHANGELOG.md | 1 + lib/AuditLog/Operation.php | 4 ++ lib/Service/OpenProjectAPIService.php | 32 ++++++++++++---- lib/Service/SettingsService.php | 12 +++++- .../lib/Service/OpenProjectAPIServiceTest.php | 38 +++++++++++++++++++ tests/lib/Service/SettingsServiceTest.php | 19 +++++++++- 6 files changed, 96 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ccc4041e..8f0d1909f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Added - Support Nextcloud 35 - Log integration settings saved through the setup API and every new Nextcloud OAuth client for OpenProject to the audit log +- Log the project folder setup and terms of service signed for the OpenProject user to the audit log ### Fixed - Keep the admin settings when Nextcloud replays the migrations to check the database schema (`occ db:schema:check`, every `occ upgrade` on Nextcloud 35) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 4496b7234..c07ee2619 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -20,7 +20,9 @@ enum Operation: string { case OauthClientCreated = 'integration_openproject.oauth_client.created'; case AppPasswordCreated = 'integration_openproject.app_password.created'; case AppPasswordDeleted = 'integration_openproject.app_password.deleted'; + case ProjectFolderCreated = 'integration_openproject.project_folder.created'; case ProjectFolderDeactivated = 'integration_openproject.project_folder.deactivated'; + case TermsOfServiceSigned = 'integration_openproject.terms_of_service.signed'; /** * Short description of when the operation is logged, used for the documentation @@ -32,7 +34,9 @@ public function description(): string { self::OauthClientCreated => 'An admin created or replaced the Nextcloud OAuth client used by OpenProject', self::AppPasswordCreated => 'An app password was created or replaced for the OpenProject system user', self::AppPasswordDeleted => 'The app password of the OpenProject system user was deleted', + self::ProjectFolderCreated => 'The project folder setup created the OpenProject system user and groups, made the user their group admin and created the project groupfolder', self::ProjectFolderDeactivated => 'An admin deactivated the project folder setup', + self::TermsOfServiceSigned => 'Terms of service were signed on behalf of the OpenProject system user, by an admin, during the project folder setup or automatically when the terms change', }; } } diff --git a/lib/Service/OpenProjectAPIService.php b/lib/Service/OpenProjectAPIService.php index 7ca88a6aa..5318aa37f 100644 --- a/lib/Service/OpenProjectAPIService.php +++ b/lib/Service/OpenProjectAPIService.php @@ -1072,11 +1072,12 @@ public function getProjectFolderSetupInformation(): array { } /** + * @return int the ID of the created groupfolder * @throws NotPermittedException * @throws NotFoundException * @throws NoUserException */ - public function createGroupfolder(): void { + public function createGroupfolder(): int { $groupfoldersFolderManager = Server::get(FolderManager::class); $folderId = $groupfoldersFolderManager->createFolder( Application::OPEN_PROJECT_ENTITIES_NAME @@ -1096,6 +1097,7 @@ public function createGroupfolder(): void { Application::OPEN_PROJECT_ENTITIES_NAME, true ); + return $folderId; } public function getGroupFolderManager(): FolderManager { @@ -1247,8 +1249,10 @@ public function isAllTermsOfServiceSignedForUserOpenProject(?SignatoryMapper $si /** * @throws \OCP\DB\Exception */ - public function signTermsOfServiceForUserOpenProject(): void { - $signatoryMapper = new SignatoryMapper($this->db); + public function signTermsOfServiceForUserOpenProject(?SignatoryMapper $signatoryMapper = null): void { + if ($signatoryMapper === null) { + $signatoryMapper = new SignatoryMapper($this->db); + } // get all the available terms of services $terms = $this->getAllTermsOfServiceAvailable(); $alreadySignedTermsIdForUserOpenProject = $this->getAllTermsOfServiceSignedByUserOpenProject($signatoryMapper); @@ -1256,15 +1260,27 @@ public function signTermsOfServiceForUserOpenProject(): void { $termId = $term->id; // sign only not signed TOS for user "OpenProject" if (!in_array($termId, $alreadySignedTermsIdForUserOpenProject)) { - $signatory = new Signatory(); - $signatory->setUserId(Application::OPEN_PROJECT_ENTITIES_NAME); - $signatory->setTermsId($termId); - $signatory->setTimestamp(time()); - $signatoryMapper->insert($signatory); + $signatoryMapper->insert($this->createSignatoryForUserOpenProject($termId)); + $this->logToAuditLog( + Operation::TermsOfServiceSigned, + "Terms of service %s have been signed for user '%s' in application " . Application::APP_ID, + ['termsId' => $termId, 'userId' => Application::OPEN_PROJECT_ENTITIES_NAME], + ); } } } + /** + * @param int $termId + */ + protected function createSignatoryForUserOpenProject($termId) { + $signatory = new Signatory(); + $signatory->setUserId(Application::OPEN_PROJECT_ENTITIES_NAME); + $signatory->setTermsId($termId); + $signatory->setTimestamp(time()); + return $signatory; + } + public function isUserPartOfAndAdminOfGroup():bool { if ($this->groupManager->isInGroup( Application::OPEN_PROJECT_ENTITIES_NAME, diff --git a/lib/Service/SettingsService.php b/lib/Service/SettingsService.php index 6ef591bf8..861d26540 100644 --- a/lib/Service/SettingsService.php +++ b/lib/Service/SettingsService.php @@ -9,6 +9,7 @@ use InvalidArgumentException; use OCA\OpenProject\AppInfo\Application; +use OCA\OpenProject\AuditLog\Operation; use OCP\Group\ISubAdmin; use OCP\IGroupManager; use OCP\IUserManager; @@ -257,7 +258,16 @@ public function setupProjectFolder(): void { $allGroup->addUser($user); $this->subAdmin->createSubAdmin($user, $group); $this->subAdmin->createSubAdmin($user, $allGroup); - $this->openprojectAPIService->createGroupfolder(); + $groupfolderId = $this->openprojectAPIService->createGroupfolder(); + $this->openprojectAPIService->logToAuditLog( + Operation::ProjectFolderCreated, + "Project folder %s has been set up for user '%s', who manages the groups %s, in application " . Application::APP_ID, + [ + 'groupfolderId' => $groupfolderId, + 'userId' => Application::OPEN_PROJECT_ENTITIES_NAME, + 'groupIds' => Application::OPEN_PROJECT_ENTITIES_NAME . ', ' . Application::OPENPROJECT_ALL_GROUP_NAME, + ], + ); if ($this->openprojectAPIService->isTermsOfServiceAppEnabled() && $this->userManager->userExists(Application::OPEN_PROJECT_ENTITIES_NAME)) { $this->openprojectAPIService->signTermsOfServiceForUserOpenProject(); } diff --git a/tests/lib/Service/OpenProjectAPIServiceTest.php b/tests/lib/Service/OpenProjectAPIServiceTest.php index 77c5c0a30..d0b4d095b 100644 --- a/tests/lib/Service/OpenProjectAPIServiceTest.php +++ b/tests/lib/Service/OpenProjectAPIServiceTest.php @@ -4386,6 +4386,44 @@ public function testIsAllTermsOfServiceSignedForUserOpenProject(array $available $this->assertSame($expectedResult, $result); } + public function signTermsOfServiceDataProvider(): array { + return [ + 'one term not signed yet' => [[1], [2]], + 'no term signed yet' => [[], [1, 2]], + 'all terms already signed' => [[1, 2], []], + ]; + } + + /** + * @dataProvider signTermsOfServiceDataProvider + * @param list $alreadySignedTermIds + * @param list $expectedSignedTermIds + */ + public function testSignTermsOfServiceForUserOpenProjectLogsSignedTerms(array $alreadySignedTermIds, array $expectedSignedTermIds): void { + $signatoryMapperBuilder = $this->getMockBuilder(SignatoryMapper::class)->disableOriginalConstructor(); + // the terms_of_service app is optional, so its mapper might not exist + $signatoryMapperMock = method_exists(SignatoryMapper::class, 'insert') + ? $signatoryMapperBuilder->onlyMethods(['insert'])->getMock() + : $signatoryMapperBuilder->addMethods(['insert'])->getMock(); + $signatoryMapperMock->expects($this->exactly(count($expectedSignedTermIds)))->method('insert'); + $auditEvents = []; + $service = $this->getOpenProjectAPIServiceMock( + ['getAllTermsOfServiceAvailable', 'getAllTermsOfServiceSignedByUserOpenProject', 'createSignatoryForUserOpenProject'], + ['eventDispatcher' => $this->getEventDispatcherMockCollectingAuditEvents($auditEvents)], + ); + $service->method('getAllTermsOfServiceAvailable')->willReturn([(object)['id' => 1], (object)['id' => 2]]); + $service->method('getAllTermsOfServiceSignedByUserOpenProject')->willReturn($alreadySignedTermIds); + $service->method('createSignatoryForUserOpenProject')->willReturn(new \stdClass()); + + $service->signTermsOfServiceForUserOpenProject($signatoryMapperMock); + + $this->assertAuditEvents(array_map(static fn (int $termId): array => [ + Operation::TermsOfServiceSigned, + "Terms of service %s have been signed for user '%s' in application integration_openproject", + ['termsId' => $termId, 'userId' => Application::OPEN_PROJECT_ENTITIES_NAME], + ], $expectedSignedTermIds), $auditEvents); + } + public function testGetSubline(): void { $configMock = $this->getMockBuilder(IConfig::class)->getMock(); $configMock->method('getUserValue') diff --git a/tests/lib/Service/SettingsServiceTest.php b/tests/lib/Service/SettingsServiceTest.php index 19f4b1b15..f5666ff18 100644 --- a/tests/lib/Service/SettingsServiceTest.php +++ b/tests/lib/Service/SettingsServiceTest.php @@ -9,6 +9,7 @@ use InvalidArgumentException; use OCA\OpenProject\AppInfo\Application; +use OCA\OpenProject\AuditLog\Operation; use OCP\Group\ISubAdmin; use OCP\IGroup; use OCP\IGroupManager; @@ -437,7 +438,20 @@ public function testSetupProjectFolder(bool $systemReady, bool $tosEnabled): voi $opApiService ->expects($this->once()) - ->method('createGroupfolder'); + ->method('createGroupfolder') + ->willReturn(7); + $opApiService + ->expects($this->once()) + ->method('logToAuditLog') + ->with( + Operation::ProjectFolderCreated, + "Project folder %s has been set up for user '%s', who manages the groups %s, in application integration_openproject", + [ + 'groupfolderId' => 7, + 'userId' => Application::OPEN_PROJECT_ENTITIES_NAME, + 'groupIds' => Application::OPEN_PROJECT_ENTITIES_NAME . ', ' . Application::OPENPROJECT_ALL_GROUP_NAME, + ], + ); $opApiService ->expects($this->once()) ->method('isTermsOfServiceAppEnabled') @@ -458,6 +472,9 @@ public function testSetupProjectFolder(bool $systemReady, bool $tosEnabled): voi $opApiService ->expects($this->never()) ->method('createGroupfolder'); + $opApiService + ->expects($this->never()) + ->method('logToAuditLog'); $opApiService ->expects($this->never()) ->method('isTermsOfServiceAppEnabled'); From b6d04a7ae99bcb583e379076ee87aa949413a319 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:49:13 +0200 Subject: [PATCH 4/5] feat(accounts): log connecting and disconnecting OpenProject accounts A user connecting their OpenProject account, through the OAuth flow or with a personal access token, is now written to the audit log with the user ID and the OpenProject user ID. Removing the token in the personal settings is logged as a disconnection. The access and refresh tokens are never logged. Failed connections are not logged. Users of the OIDC authorization method are not logged: they never connect explicitly, their token is fetched from the identity provider whenever needed. Disconnecting all users because the OpenProject OAuth client or the authorization method changed is part of the integration settings entries. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- CHANGELOG.md | 1 + lib/AuditLog/Operation.php | 4 ++ lib/Controller/ConfigController.php | 23 ++++++ tests/lib/Controller/ConfigControllerTest.php | 72 +++++++++++++++++++ 4 files changed, 100 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f0d1909f..79cdaee99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Support Nextcloud 35 - Log integration settings saved through the setup API and every new Nextcloud OAuth client for OpenProject to the audit log - Log the project folder setup and terms of service signed for the OpenProject user to the audit log +- Log users connecting and disconnecting their OpenProject account to the audit log ### Fixed - Keep the admin settings when Nextcloud replays the migrations to check the database schema (`occ db:schema:check`, every `occ upgrade` on Nextcloud 35) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index c07ee2619..93caa7c51 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -23,6 +23,8 @@ enum Operation: string { case ProjectFolderCreated = 'integration_openproject.project_folder.created'; case ProjectFolderDeactivated = 'integration_openproject.project_folder.deactivated'; case TermsOfServiceSigned = 'integration_openproject.terms_of_service.signed'; + case AccountConnected = 'integration_openproject.account.connected'; + case AccountDisconnected = 'integration_openproject.account.disconnected'; /** * Short description of when the operation is logged, used for the documentation @@ -37,6 +39,8 @@ public function description(): string { self::ProjectFolderCreated => 'The project folder setup created the OpenProject system user and groups, made the user their group admin and created the project groupfolder', self::ProjectFolderDeactivated => 'An admin deactivated the project folder setup', self::TermsOfServiceSigned => 'Terms of service were signed on behalf of the OpenProject system user, by an admin, during the project folder setup or automatically when the terms change', + self::AccountConnected => 'A user connected their OpenProject account through OAuth or a personal access token', + self::AccountDisconnected => 'A user disconnected their OpenProject account', }; } } diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index e6a2fea44..ce8b1522f 100755 --- a/lib/Controller/ConfigController.php +++ b/lib/Controller/ConfigController.php @@ -114,6 +114,20 @@ public function clearUserInfo(?string $userId = null) { $this->config->deleteUserValue($userId, Application::APP_ID, 'token_expires_at'); } + /** + * Only the OpenProject user ID is logged, never the access or refresh token + */ + private function logAccountConnected(): void { + $this->openprojectAPIService->logToAuditLog( + Operation::AccountConnected, + 'User %s has connected the OpenProject account %s in application ' . Application::APP_ID, + [ + 'userId' => $this->userId, + 'openprojectUserId' => (string)$this->config->getUserValue($this->userId, Application::APP_ID, 'user_id'), + ], + ); + } + /** * @return void */ @@ -149,8 +163,16 @@ public function setConfig(array $values): DataResponse { if (isset($values['token'])) { if ($values['token']) { $result = $this->openprojectAPIService->initUserInfo($this->userId, $values['token']); + if (!isset($result['error'])) { + $this->logAccountConnected(); + } } else { $this->clearUserInfo(); + $this->openprojectAPIService->logToAuditLog( + Operation::AccountDisconnected, + 'User %s has disconnected the OpenProject account in application ' . Application::APP_ID, + ['userId' => $this->userId], + ); $result = [ 'user_name' => '', ]; @@ -472,6 +494,7 @@ public function oauthRedirect(string $code = '', string $state = ''): RedirectRe $this->config->setUserValue( $this->userId, Application::APP_ID, 'oauth_connection_result', 'success' ); + $this->logAccountConnected(); } return new RedirectResponse($newUrl); } diff --git a/tests/lib/Controller/ConfigControllerTest.php b/tests/lib/Controller/ConfigControllerTest.php index 9f40fbdde..180d893be 100644 --- a/tests/lib/Controller/ConfigControllerTest.php +++ b/tests/lib/Controller/ConfigControllerTest.php @@ -99,6 +99,7 @@ private function getConfigMock( ['testUser', 'integration_openproject', 'code_verifier', false, $codeVerifier], ['testUser', 'integration_openproject', 'oauth_journey_starting_page', '', $startingPage], ['testUser', 'integration_openproject', 'refresh_token', false, 'oAuthRefreshToken'], + ['testUser', 'integration_openproject', 'user_id', '', '7'], ]); return $configMock; } @@ -238,6 +239,13 @@ public function testOauthRedirectSuccess():void { $apiServiceMock ->method('requestOAuthAccessToken') ->willReturn(['access_token' => 'oAuthAccessToken', 'refresh_token' => 'oAuthRefreshToken']); + $apiServiceMock->expects($this->once()) + ->method('logToAuditLog') + ->with( + Operation::AccountConnected, + 'User %s has connected the OpenProject account %s in application integration_openproject', + ['userId' => 'testUser', 'openprojectUserId' => '7'], + ); $constructArgs = $this->getConfigControllerConstructArgs([ 'config' => $configMock, @@ -322,8 +330,12 @@ public function testOauthRedirectWrongState() { [['testUser', 'integration_openproject', 'oauth_connection_error_message', 'Error during OAuth exchanges', null], null], ]); + $apiServiceMock = $this->createMock(OpenProjectAPIService::class); + $apiServiceMock->expects($this->never())->method('logToAuditLog'); + $constructArgs = $this->getConfigControllerConstructArgs([ 'config' => $configMock, + 'openprojectAPIService' => $apiServiceMock, ]); $configController = new ConfigController(...$constructArgs); @@ -482,6 +494,7 @@ public function testOauthNoAccessTokenInResponse($oauthResponse, $expectedErrorM $apiServiceMock ->method('requestOAuthAccessToken') ->willReturn($oauthResponse); + $apiServiceMock->expects($this->never())->method('logToAuditLog'); $configMock = $this->getConfigMock( str_repeat("A", 128), str_repeat("S", 50) ); @@ -2127,4 +2140,63 @@ public function testSetAdminConfigPassesAppPasswordReplacementToService(): void $this->assertSame(Http::STATUS_OK, $result->getStatus()); } + + public function testSetConfigWithTokenLogsConnectedAccountWithoutToken(): void { + $auditEntries = []; + $apiService = $this->getApiServiceMockCollectingAuditEntries($auditEntries); + $apiService->method('initUserInfo')->with('testUser', 'personal-token')->willReturn(['user_name' => 'Some User']); + $configMock = $this->createMock(IConfig::class); + $configMock->method('getUserValue')->willReturnMap([ + ['testUser', Application::APP_ID, 'user_id', '', '7'], + ]); + $controller = $this->getConfigControllerForAuditLog($auditEntries, [], [ + 'config' => $configMock, + 'openprojectAPIService' => $apiService, + ]); + + $result = $controller->setConfig(['token' => 'personal-token']); + + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + $this->assertSame([[ + Operation::AccountConnected, + 'User %s has connected the OpenProject account %s in application integration_openproject', + ['userId' => 'testUser', 'openprojectUserId' => '7'], + ]], $auditEntries); + $this->assertStringNotContainsString('personal-token', json_encode($auditEntries)); + } + + public function testSetConfigWithInvalidTokenDoesNotLog(): void { + $auditEntries = []; + $apiService = $this->getApiServiceMockCollectingAuditEntries($auditEntries); + $apiService->method('initUserInfo')->willReturn(['error' => 'Unauthorized']); + $controller = $this->getConfigControllerForAuditLog($auditEntries, [], ['openprojectAPIService' => $apiService]); + + $result = $controller->setConfig(['token' => 'invalid-token']); + + $this->assertSame(Http::STATUS_UNAUTHORIZED, $result->getStatus()); + $this->assertSame([], $auditEntries); + } + + public function testSetConfigWithEmptyTokenLogsDisconnectedAccount(): void { + $auditEntries = []; + $controller = $this->getConfigControllerForAuditLog($auditEntries, []); + + $result = $controller->setConfig(['token' => '']); + + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + $this->assertSame([[ + Operation::AccountDisconnected, + 'User %s has disconnected the OpenProject account in application integration_openproject', + ['userId' => 'testUser'], + ]], $auditEntries); + } + + public function testSetConfigWithoutTokenDoesNotLog(): void { + $auditEntries = []; + $controller = $this->getConfigControllerForAuditLog($auditEntries, []); + + $controller->setConfig(['search_enabled' => '1']); + + $this->assertSame([], $auditEntries); + } } From 92b2404847b97c537eb16ac11cfc5702d098158b Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:50:52 +0200 Subject: [PATCH 5/5] feat(file-links): log linking files to OpenProject work packages Linking a file to an OpenProject work package is now written to the audit log, one entry per file, with the file ID, the work package ID, the user and the ID of the file link OpenProject created. Linking sends the file ID, name and type to OpenProject, where the members of the project can see them. Deleting a file link is logged with the file link ID and the user. Only IDs are logged, not the file names. Nothing is logged when OpenProject rejects the request or answers with a malformed response. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- CHANGELOG.md | 1 + lib/AuditLog/Operation.php | 4 ++ lib/Service/OpenProjectAPIService.php | 17 +++++ .../lib/Service/OpenProjectAPIServiceTest.php | 63 ++++++++++++++++++- 4 files changed, 83 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79cdaee99..3bb803342 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Log integration settings saved through the setup API and every new Nextcloud OAuth client for OpenProject to the audit log - Log the project folder setup and terms of service signed for the OpenProject user to the audit log - Log users connecting and disconnecting their OpenProject account to the audit log +- Log files linked to and unlinked from OpenProject work packages to the audit log ### Fixed - Keep the admin settings when Nextcloud replays the migrations to check the database schema (`occ db:schema:check`, every `occ upgrade` on Nextcloud 35) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 93caa7c51..dc303873b 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -25,6 +25,8 @@ enum Operation: string { case TermsOfServiceSigned = 'integration_openproject.terms_of_service.signed'; case AccountConnected = 'integration_openproject.account.connected'; case AccountDisconnected = 'integration_openproject.account.disconnected'; + case FileLinkCreated = 'integration_openproject.file_link.created'; + case FileLinkDeleted = 'integration_openproject.file_link.deleted'; /** * Short description of when the operation is logged, used for the documentation @@ -41,6 +43,8 @@ public function description(): string { self::TermsOfServiceSigned => 'Terms of service were signed on behalf of the OpenProject system user, by an admin, during the project folder setup or automatically when the terms change', self::AccountConnected => 'A user connected their OpenProject account through OAuth or a personal access token', self::AccountDisconnected => 'A user disconnected their OpenProject account', + self::FileLinkCreated => 'A user linked a file to an OpenProject work package, which shares the file ID, name and type with OpenProject', + self::FileLinkDeleted => 'A user removed the link between a file and an OpenProject work package', }; } } diff --git a/lib/Service/OpenProjectAPIService.php b/lib/Service/OpenProjectAPIService.php index 5318aa37f..a09a5af95 100644 --- a/lib/Service/OpenProjectAPIService.php +++ b/lib/Service/OpenProjectAPIService.php @@ -816,6 +816,18 @@ public function linkWorkPackageToFile( } $fileIds [] = $result['_embedded']['elements'][$i]['id']; } + foreach (array_values($fileInfos) as $i => $fileInfo) { + $this->logToAuditLog( + Operation::FileLinkCreated, + 'File %s has been linked to OpenProject work package %s by user %s in application ' . Application::APP_ID, + [ + 'fileId' => $fileInfo['id'], + 'workPackageId' => $values['workpackageId'], + 'userId' => $userId, + 'fileLinkId' => $fileIds[$i], + ], + ); + } return $fileIds; } @@ -886,6 +898,11 @@ public function deleteFileLink(int $fileLinkId, string $userId): array { if (!isset($result['success'])) { throw new OpenprojectResponseException('Malformed response'); } + $this->logToAuditLog( + Operation::FileLinkDeleted, + 'OpenProject file link %s has been deleted by user %s in application ' . Application::APP_ID, + ['fileLinkId' => $fileLinkId, 'userId' => $userId], + ); return $result; } diff --git a/tests/lib/Service/OpenProjectAPIServiceTest.php b/tests/lib/Service/OpenProjectAPIServiceTest.php index d0b4d095b..65b53e6ed 100644 --- a/tests/lib/Service/OpenProjectAPIServiceTest.php +++ b/tests/lib/Service/OpenProjectAPIServiceTest.php @@ -1924,6 +1924,42 @@ public function testLinkWorkPackageToFileRequest(): void { $this->assertSame([2456], $result); } + public function testLinkWorkPackageToFileLogsEachLinkedFile(): void { + $auditEvents = []; + $service = $this->getOpenProjectAPIServiceMock(['request', 'getNode'], [ + 'eventDispatcher' => $this->getEventDispatcherMockCollectingAuditEvents($auditEvents), + ]); + $service->method('getNode')->willReturn($this->getNodeMock()); + $service->method('request') + ->willReturn(['_type' => 'Collection', '_embedded' => ['elements' => [['id' => 2456], ['id' => 2457], ['id' => 2458]]]]); + + $service->linkWorkPackageToFile($this->multipleFileInformation, 'user'); + + $message = 'File %s has been linked to OpenProject work package %s by user %s in application integration_openproject'; + $this->assertAuditEvents([ + [Operation::FileLinkCreated, $message, ['fileId' => 5503, 'workPackageId' => 123, 'userId' => 'user', 'fileLinkId' => 2456]], + [Operation::FileLinkCreated, $message, ['fileId' => 5504, 'workPackageId' => 123, 'userId' => 'user', 'fileLinkId' => 2457]], + [Operation::FileLinkCreated, $message, ['fileId' => 5505, 'workPackageId' => 123, 'userId' => 'user', 'fileLinkId' => 2458]], + ], $auditEvents); + } + + public function testLinkWorkPackageToFileWithMalformedResponseDoesNotLog(): void { + $auditEvents = []; + $service = $this->getOpenProjectAPIServiceMock(['request', 'getNode'], [ + 'eventDispatcher' => $this->getEventDispatcherMockCollectingAuditEvents($auditEvents), + ]); + $service->method('getNode')->willReturn($this->getNodeMock()); + $service->method('request') + ->willReturn(['_type' => 'Collection', '_embedded' => ['elements' => [['id' => 2456]]]]); + + try { + $service->linkWorkPackageToFile($this->multipleFileInformation, 'user'); + $this->fail('Expected OpenprojectResponseException'); + } catch (OpenprojectResponseException $e) { + } + $this->assertSame([], $auditEvents); + } + /** * @return void */ @@ -3860,13 +3896,36 @@ public function testDeleteFileLinkResponse(): void { * @return void */ public function testDeleteFileLinkErrorResponse(): void { - $service = $this->getOpenProjectAPIServiceMock(); + $auditEvents = []; + $service = $this->getOpenProjectAPIServiceMock(['request'], [ + 'eventDispatcher' => $this->getEventDispatcherMockCollectingAuditEvents($auditEvents), + ]); $service->method('request') ->willReturn([ 'error' => 'something went wrong', ]); - $this->expectException(OpenprojectErrorException::class); + try { + $service->deleteFileLink(7, 'user'); + $this->fail('Expected OpenprojectErrorException'); + } catch (OpenprojectErrorException $e) { + } + $this->assertSame([], $auditEvents); + } + + public function testDeleteFileLinkLogsDeletion(): void { + $auditEvents = []; + $service = $this->getOpenProjectAPIServiceMock(['request'], [ + 'eventDispatcher' => $this->getEventDispatcherMockCollectingAuditEvents($auditEvents), + ]); + $service->method('request')->willReturn(['success' => true]); + $service->deleteFileLink(7, 'user'); + + $this->assertAuditEvents([[ + Operation::FileLinkDeleted, + 'OpenProject file link %s has been deleted by user %s in application integration_openproject', + ['fileLinkId' => 7, 'userId' => 'user'], + ]], $auditEvents); } /**