From 23e725152f0e943547e83646ec836d7ba9dfe91c Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:42:58 +0200 Subject: [PATCH 1/4] feat(daemons): log deploy daemon changes to the audit log Registering, updating and unregistering a deploy daemon, and adding or removing a Docker registry mapping of a daemon, are now written to the admin audit log. The entries are written by DaemonConfigService, so they cover the admin settings, the occ app_api:daemon:* commands and the automatic AIO daemon registration. The HaRP shared key or HaProxy password is never logged. An update only logs whether a new secret was set, which is why the service now receives the stored configuration it replaces. The operations are collected in the new OCA\AppAPI\AuditLog\Operation enum, with a short description of each for the documentation. The new AuditLogger dispatches the CriticalActionPerformedEvent for all of them. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/AuditLogger.php | 37 +++++ lib/AuditLog/Operation.php | 38 +++++ lib/Controller/DaemonConfigController.php | 2 +- lib/Service/DaemonConfigService.php | 49 +++++- tests/php/AuditLog/AuditEvents.php | 21 +++ tests/php/AuditLog/AuditLoggerTest.php | 33 ++++ tests/php/AuditLog/OperationTest.php | 26 ++++ .../Controller/DaemonConfigControllerTest.php | 5 +- tests/php/Service/DaemonConfigServiceTest.php | 144 +++++++++++++++++- 9 files changed, 344 insertions(+), 11 deletions(-) create mode 100644 lib/AuditLog/AuditLogger.php create mode 100644 lib/AuditLog/Operation.php create mode 100644 tests/php/AuditLog/AuditEvents.php create mode 100644 tests/php/AuditLog/AuditLoggerTest.php create mode 100644 tests/php/AuditLog/OperationTest.php diff --git a/lib/AuditLog/AuditLogger.php b/lib/AuditLog/AuditLogger.php new file mode 100644 index 00000000..aab4597d --- /dev/null +++ b/lib/AuditLog/AuditLogger.php @@ -0,0 +1,37 @@ + $parameters named values for the placeholders of $message, in order, optionally followed by more context + */ + public function log(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)); + } +} diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php new file mode 100644 index 00000000..ee8f740d --- /dev/null +++ b/lib/AuditLog/Operation.php @@ -0,0 +1,38 @@ + 'A deploy daemon that ExApps can be installed on was registered', + self::DaemonUpdated => 'The configuration of a deploy daemon was changed', + self::DaemonUnregistered => 'A deploy daemon was unregistered', + self::DaemonRegistryAdded => 'A deploy daemon was set to pull the images of a Docker registry from another registry', + self::DaemonRegistryRemoved => 'A Docker registry mapping was removed from a deploy daemon', + }; + } +} diff --git a/lib/Controller/DaemonConfigController.php b/lib/Controller/DaemonConfigController.php index 9ad23357..96925475 100644 --- a/lib/Controller/DaemonConfigController.php +++ b/lib/Controller/DaemonConfigController.php @@ -144,7 +144,7 @@ public function updateDaemonConfig(string $name, array $daemonConfigParams): Res // Create and update DaemonConfig instance $updatedDaemonConfig = new DaemonConfig($daemonConfigParams); $updatedDaemonConfig->setId($daemonConfig->getId()); - $updatedDaemonConfig = $this->daemonConfigService->updateDaemonConfig($updatedDaemonConfig); + $updatedDaemonConfig = $this->daemonConfigService->updateDaemonConfig($updatedDaemonConfig, $daemonConfig); // Check if update was successful before proceeding if ($updatedDaemonConfig === null) { diff --git a/lib/Service/DaemonConfigService.php b/lib/Service/DaemonConfigService.php index aa920d64..592e070a 100644 --- a/lib/Service/DaemonConfigService.php +++ b/lib/Service/DaemonConfigService.php @@ -9,6 +9,8 @@ namespace OCA\AppAPI\Service; +use OCA\AppAPI\AuditLog\AuditLogger; +use OCA\AppAPI\AuditLog\Operation; use OCA\AppAPI\Db\DaemonConfig; use OCA\AppAPI\Db\DaemonConfigMapper; use OCA\AppAPI\DeployActions\DockerActions; @@ -36,6 +38,7 @@ public function __construct( private DaemonConfigMapper $mapper, private ExAppService $exAppService, private ICrypto $crypto, + private AuditLogger $auditLogger, ) { } @@ -134,7 +137,7 @@ public function registerDaemonConfig(array $params): ?DaemonConfig { if (isset($params['deploy_config']['haproxy_password']) && $params['deploy_config']['haproxy_password'] !== '') { $params['deploy_config']['haproxy_password'] = $this->crypto->encrypt($params['deploy_config']['haproxy_password']); } - return $this->mapper->insert(new DaemonConfig([ + $daemonConfig = $this->mapper->insert(new DaemonConfig([ 'name' => $params['name'], 'display_name' => $params['display_name'], 'accepts_deploy_id' => $params['accepts_deploy_id'], @@ -146,15 +149,25 @@ public function registerDaemonConfig(array $params): ?DaemonConfig { $this->logger->error('Failed to register daemon config. Error: ' . $e->getMessage(), ['exception' => $e]); return null; } + $this->auditLogger->log(Operation::DaemonRegistered, 'Deploy daemon %s was registered', [ + 'daemonName' => $daemonConfig->getName(), + 'acceptsDeployId' => $daemonConfig->getAcceptsDeployId(), + 'host' => $daemonConfig->getHost(), + ]); + return $daemonConfig; } public function unregisterDaemonConfig(DaemonConfig $daemonConfig): ?DaemonConfig { try { - return $this->mapper->delete($daemonConfig); + $daemonConfig = $this->mapper->delete($daemonConfig); } catch (Exception $e) { $this->logger->error('Failed to unregister daemon config. Error: ' . $e->getMessage(), ['exception' => $e]); return null; } + $this->auditLogger->log(Operation::DaemonUnregistered, 'Deploy daemon %s was unregistered', [ + 'daemonName' => $daemonConfig->getName(), + ]); + return $daemonConfig; } /** @@ -202,7 +215,10 @@ public function getDaemonConfigByName(string $name): ?DaemonConfig { } } - public function updateDaemonConfig(DaemonConfig $daemonConfig): ?DaemonConfig { + /** + * @param DaemonConfig $previousDaemonConfig the stored configuration before the update + */ + public function updateDaemonConfig(DaemonConfig $daemonConfig, DaemonConfig $previousDaemonConfig): ?DaemonConfig { $name = $daemonConfig->getName() ?? ''; if ($name === '' || $this->containsControlCharacters($name)) { $this->logger->error('Failed to update daemon configuration: `name` contains invalid characters or is empty.'); @@ -210,11 +226,20 @@ public function updateDaemonConfig(DaemonConfig $daemonConfig): ?DaemonConfig { } try { - return $this->mapper->update($daemonConfig); + $daemonConfig = $this->mapper->update($daemonConfig); } catch (Exception $e) { $this->logger->error('Failed to update DaemonConfig. Error: ' . $e->getMessage(), ['exception' => $e]); return null; } + // Only whether the secret changed is logged, never the secret itself + $secretChanged = ($daemonConfig->getDeployConfig()['haproxy_password'] ?? '') + !== ($previousDaemonConfig->getDeployConfig()['haproxy_password'] ?? ''); + $this->auditLogger->log(Operation::DaemonUpdated, 'Deploy daemon %s was updated', [ + 'daemonName' => $daemonConfig->getName(), + 'host' => $daemonConfig->getHost(), + 'secretChanged' => $secretChanged, + ]); + return $daemonConfig; } public function addDockerRegistry(DaemonConfig $daemonConfig, array $registryMap): DaemonConfig|array|null { @@ -246,11 +271,17 @@ public function addDockerRegistry(DaemonConfig $daemonConfig, array $registryMap $deployConfig['registries'] = [...array_values($deployConfig['registries']), ['from' => $from, 'to' => $to]]; $daemonConfig->setDeployConfig($deployConfig); - return $this->mapper->update($daemonConfig); + $daemonConfig = $this->mapper->update($daemonConfig); } catch (Exception $e) { $this->logger->error('Failed to add registry to DaemonConfig. Error: ' . $e->getMessage(), ['exception' => $e]); return null; } + $this->auditLogger->log(Operation::DaemonRegistryAdded, 'Docker registry mapping was added to deploy daemon %s: %s to %s', [ + 'daemonName' => $daemonConfig->getName(), + 'from' => $from, + 'to' => $to, + ]); + return $daemonConfig; } public function removeDockerRegistry(DaemonConfig $daemonConfig, array $registryMap): DaemonConfig|array|null { @@ -265,11 +296,17 @@ public function removeDockerRegistry(DaemonConfig $daemonConfig, array $registry })); $daemonConfig->setDeployConfig($deployConfig); - return $this->mapper->update($daemonConfig); + $daemonConfig = $this->mapper->update($daemonConfig); } catch (Exception $e) { $this->logger->error('Failed to remove registry from DaemonConfig. Error: ' . $e->getMessage(), ['exception' => $e]); return null; } + $this->auditLogger->log(Operation::DaemonRegistryRemoved, 'Docker registry mapping was removed from deploy daemon %s: %s to %s', [ + 'daemonName' => $daemonConfig->getName(), + 'from' => $registryMap['from'] ?? null, + 'to' => $registryMap['to'] ?? null, + ]); + return $daemonConfig; } /** diff --git a/tests/php/AuditLog/AuditEvents.php b/tests/php/AuditLog/AuditEvents.php new file mode 100644 index 00000000..ec87832c --- /dev/null +++ b/tests/php/AuditLog/AuditEvents.php @@ -0,0 +1,21 @@ +getOperation() === $operation; + } +} diff --git a/tests/php/AuditLog/AuditLoggerTest.php b/tests/php/AuditLog/AuditLoggerTest.php new file mode 100644 index 00000000..096880e1 --- /dev/null +++ b/tests/php/AuditLog/AuditLoggerTest.php @@ -0,0 +1,33 @@ +createMock(IEventDispatcher::class); + $eventDispatcher->expects(self::once()) + ->method('dispatchTyped') + ->with(self::callback(static function (CriticalActionPerformedEvent $event): bool { + self::assertSame('Deploy daemon %s was unregistered', $event->getLogMessage()); + self::assertSame(['daemonName' => 'harp_proxy'], $event->getParameters()); + self::assertFalse($event->getObfuscateParameters()); + self::assertTrue(AuditEvents::hasOperation($event, 'app_api.daemon.unregistered')); + return true; + })); + + (new AuditLogger($eventDispatcher))->log(Operation::DaemonUnregistered, 'Deploy daemon %s was unregistered', ['daemonName' => 'harp_proxy']); + } +} diff --git a/tests/php/AuditLog/OperationTest.php b/tests/php/AuditLog/OperationTest.php new file mode 100644 index 00000000..e24b4695 --- /dev/null +++ b/tests/php/AuditLog/OperationTest.php @@ -0,0 +1,26 @@ + [$operation], Operation::cases()); + } + + #[DataProvider('dataOperations')] + public function testOperationFormat(Operation $operation): void { + self::assertMatchesRegularExpression('/^app_api\.[a-z][a-z0-9_]*\.[a-z][a-z0-9_]*$/', $operation->value); + self::assertNotSame('', $operation->description()); + } +} diff --git a/tests/php/Controller/DaemonConfigControllerTest.php b/tests/php/Controller/DaemonConfigControllerTest.php index efe1a8ca..aaf1417d 100644 --- a/tests/php/Controller/DaemonConfigControllerTest.php +++ b/tests/php/Controller/DaemonConfigControllerTest.php @@ -119,12 +119,13 @@ public function testUpdateRejectsAnInvalidNewSecret(): void { public function testUpdateEncryptsAValidNewSecret(): void { $params = self::harpDaemonParams('a_long_enough_key'); + $storedDaemon = self::storedDaemon('encrypted:stored_secret_1', true); $this->daemonConfigService->method('getDaemonConfigByName') - ->willReturn(self::storedDaemon('encrypted:stored_secret_1', true)); + ->willReturn($storedDaemon); $this->daemonConfigService->expects(self::once())->method('validateNewSecret')->willReturn(null); $this->daemonConfigService->expects(self::once())->method('updateDaemonConfig') ->with(self::callback(static fn (DaemonConfig $daemonConfig): bool - => $daemonConfig->getDeployConfig()['haproxy_password'] === 'encrypted:a_long_enough_key')) + => $daemonConfig->getDeployConfig()['haproxy_password'] === 'encrypted:a_long_enough_key'), self::identicalTo($storedDaemon)) ->willReturnArgument(0); $response = $this->controller->updateDaemonConfig('harp_proxy_host', $params); diff --git a/tests/php/Service/DaemonConfigServiceTest.php b/tests/php/Service/DaemonConfigServiceTest.php index 8c6b0e24..a927b228 100644 --- a/tests/php/Service/DaemonConfigServiceTest.php +++ b/tests/php/Service/DaemonConfigServiceTest.php @@ -9,17 +9,28 @@ namespace OCA\AppAPI\Tests\php\Service; +use OCA\AppAPI\AuditLog\AuditLogger; +use OCA\AppAPI\AuditLog\Operation; use OCA\AppAPI\Db\DaemonConfig; use OCA\AppAPI\Db\DaemonConfigMapper; use OCA\AppAPI\Service\DaemonConfigService; use OCA\AppAPI\Service\ExAppService; +use OCP\DB\Exception; use OCP\Security\ICrypto; use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; class DaemonConfigServiceTest extends TestCase { + private AuditLogger&MockObject $auditLogger; + + protected function setUp(): void { + parent::setUp(); + $this->auditLogger = $this->createMock(AuditLogger::class); + } + public static function resolveImageRegistryProvider(): array { $mirror = ['from' => 'ghcr.io', 'to' => 'registry.example.com']; return [ @@ -104,6 +115,7 @@ private function createService(bool $expectUpdate = true): DaemonConfigService { $mapper, $this->createMock(ExAppService::class), $this->createMock(ICrypto::class), + $this->auditLogger, ); } @@ -133,6 +145,7 @@ public static function unusableRegistryMapProvider(): array { #[DataProvider('unusableRegistryMapProvider')] public function testAddDockerRegistryRejectsAnUnusableMap(array $registryMap): void { + $this->auditLogger->expects(self::never())->method('log'); $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => []]]); $result = $this->createService(expectUpdate: false)->addDockerRegistry($daemonConfig, $registryMap); @@ -142,6 +155,7 @@ public function testAddDockerRegistryRejectsAnUnusableMap(array $registryMap): v } public function testAddDockerRegistryRejectsADuplicateSource(): void { + $this->auditLogger->expects(self::never())->method('log'); $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => ['junk', ['from' => 'ghcr.io', 'to' => 'local']]]]); $result = $this->createService(expectUpdate: false) @@ -163,7 +177,12 @@ public function testAddDockerRegistryIgnoresAnUnusableStoredEntryOfTheSameSource } public function testAddDockerRegistryStoresNormalisedSourceAndTarget(): void { - $daemonConfig = new DaemonConfig(['deploy_config' => []]); + $daemonConfig = new DaemonConfig(['name' => 'harp_proxy', 'deploy_config' => []]); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::DaemonRegistryAdded, + 'Docker registry mapping was added to deploy daemon %s: %s to %s', + ['daemonName' => 'harp_proxy', 'from' => 'ghcr.io', 'to' => 'registry.example.com'], + ); $result = $this->createService() ->addDockerRegistry($daemonConfig, ['from' => ' ghcr.io/ ', 'to' => 'registry.example.com//', 'extra' => 'x']); @@ -182,6 +201,7 @@ public function testAddDockerRegistryStoresLocalWithoutATrailingSlash(): void { } public function testRemoveDockerRegistryWithoutAnyMapping(): void { + $this->auditLogger->expects(self::never())->method('log'); $daemonConfig = new DaemonConfig(['deploy_config' => ['net' => 'host']]); $result = $this->createService(expectUpdate: false) @@ -194,7 +214,12 @@ public function testRemoveDockerRegistryKeepsAList(): void { $service = $this->createService(); $first = ['from' => 'docker.io', 'to' => 'hub.example.com']; $second = ['from' => 'ghcr.io', 'to' => 'registry.example.com']; - $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => [$first, $second]]]); + $daemonConfig = new DaemonConfig(['name' => 'harp_proxy', 'deploy_config' => ['registries' => [$first, $second]]]); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::DaemonRegistryRemoved, + 'Docker registry mapping was removed from deploy daemon %s: %s to %s', + ['daemonName' => 'harp_proxy', 'from' => 'docker.io', 'to' => 'hub.example.com'], + ); $result = $service->removeDockerRegistry($daemonConfig, $first); @@ -219,6 +244,7 @@ private function createServiceWithLogger(LoggerInterface $logger): DaemonConfigS $this->createMock(DaemonConfigMapper::class), $this->createMock(ExAppService::class), $this->createMock(ICrypto::class), + $this->auditLogger, ); } @@ -281,4 +307,118 @@ public function testValidateNewSecretDoesNotWarnAboutAGeneratedSecret(): void { self::assertNull($result); } + + private function createServiceWithMapper(DaemonConfigMapper $mapper): DaemonConfigService { + $crypto = $this->createMock(ICrypto::class); + $crypto->method('encrypt')->willReturnCallback(static fn (string $value): string => 'encrypted:' . $value); + return new DaemonConfigService( + $this->createMock(LoggerInterface::class), + $mapper, + $this->createMock(ExAppService::class), + $crypto, + $this->auditLogger, + ); + } + + private static function harpRegisterParams(): array { + return [ + 'name' => 'harp_proxy', + 'display_name' => 'HaRP Proxy', + 'accepts_deploy_id' => 'docker-install', + 'protocol' => 'http', + 'host' => 'appapi-harp:8780', + 'deploy_config' => [ + 'net' => 'host', + 'nextcloud_url' => 'http://nextcloud.local/', + 'haproxy_password' => 'a_secret_shared_key', + 'harp' => ['frp_address' => 'appapi-harp:8782'], + ], + ]; + } + + public function testRegisterDaemonConfigLogsTheDaemonWithoutTheSecret(): void { + $mapper = $this->createMock(DaemonConfigMapper::class); + $mapper->expects(self::once())->method('insert')->willReturnArgument(0); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::DaemonRegistered, + 'Deploy daemon %s was registered', + ['daemonName' => 'harp_proxy', 'acceptsDeployId' => 'docker-install', 'host' => 'appapi-harp:8780'], + ); + + $result = $this->createServiceWithMapper($mapper)->registerDaemonConfig(self::harpRegisterParams()); + + self::assertInstanceOf(DaemonConfig::class, $result); + } + + public function testRegisterDaemonConfigDoesNotLogAnInvalidDaemon(): void { + $mapper = $this->createMock(DaemonConfigMapper::class); + $mapper->expects(self::never())->method('insert'); + $this->auditLogger->expects(self::never())->method('log'); + + $params = self::harpRegisterParams(); + $params['host'] = 'http://appapi-harp:8780'; + + self::assertNull($this->createServiceWithMapper($mapper)->registerDaemonConfig($params)); + } + + public function testRegisterDaemonConfigDoesNotLogADatabaseError(): void { + $mapper = $this->createMock(DaemonConfigMapper::class); + $mapper->method('insert')->willThrowException(new Exception('duplicate name')); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertNull($this->createServiceWithMapper($mapper)->registerDaemonConfig(self::harpRegisterParams())); + } + + public function testUnregisterDaemonConfigLogsTheDaemon(): void { + $mapper = $this->createMock(DaemonConfigMapper::class); + $mapper->expects(self::once())->method('delete')->willReturnArgument(0); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::DaemonUnregistered, + 'Deploy daemon %s was unregistered', + ['daemonName' => 'harp_proxy'], + ); + + $this->createServiceWithMapper($mapper)->unregisterDaemonConfig(new DaemonConfig(['name' => 'harp_proxy'])); + } + + public function testUnregisterDaemonConfigDoesNotLogADatabaseError(): void { + $mapper = $this->createMock(DaemonConfigMapper::class); + $mapper->method('delete')->willThrowException(new Exception('database is gone')); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertNull($this->createServiceWithMapper($mapper)->unregisterDaemonConfig(new DaemonConfig(['name' => 'harp_proxy']))); + } + + public static function updateDaemonConfigSecretProvider(): array { + return [ + 'stored secret kept' => ['encrypted:old_secret', 'encrypted:old_secret', false], + 'new secret set' => ['encrypted:old_secret', 'encrypted:new_secret', true], + 'secret removed' => ['encrypted:old_secret', '', true], + 'no secret before or after' => [null, '', false], + ]; + } + + #[DataProvider('updateDaemonConfigSecretProvider')] + public function testUpdateDaemonConfigLogsOnlyWhetherTheSecretChanged(?string $storedSecret, string $newSecret, bool $secretChanged): void { + $mapper = $this->createMock(DaemonConfigMapper::class); + $mapper->expects(self::once())->method('update')->willReturnArgument(0); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::DaemonUpdated, + 'Deploy daemon %s was updated', + ['daemonName' => 'harp_proxy', 'host' => 'appapi-harp:8780', 'secretChanged' => $secretChanged], + ); + $previous = new DaemonConfig(['name' => 'harp_proxy', 'host' => 'appapi-harp:8780', 'deploy_config' => $storedSecret === null ? [] : ['haproxy_password' => $storedSecret]]); + $updated = new DaemonConfig(['name' => 'harp_proxy', 'host' => 'appapi-harp:8780', 'deploy_config' => ['haproxy_password' => $newSecret]]); + + $this->createServiceWithMapper($mapper)->updateDaemonConfig($updated, $previous); + } + + public function testUpdateDaemonConfigDoesNotLogADatabaseError(): void { + $mapper = $this->createMock(DaemonConfigMapper::class); + $mapper->method('update')->willThrowException(new Exception('database is gone')); + $this->auditLogger->expects(self::never())->method('log'); + $daemonConfig = new DaemonConfig(['name' => 'harp_proxy', 'deploy_config' => []]); + + self::assertNull($this->createServiceWithMapper($mapper)->updateDaemonConfig($daemonConfig, clone $daemonConfig)); + } } From a8db189f4d41142ee3f664165beac95147dd148f Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:46:21 +0200 Subject: [PATCH 2/4] feat(exapps): log the ExApp lifecycle to the audit log Registering, updating, enabling, disabling and unregistering an ExApp, and deleting its data volume, are now written to the admin audit log. The entries are written where the change is stored, so every path is covered once: the web UI, the OCS API, the occ app_api:app:* commands and the removal of a deploy daemon with its ExApps. Installing and updating from the web UI run occ app_api:app:register or app_api:app:update in a separate process; the entry is written by that process, which is the one that knows whether the change was stored, and the web request itself writes nothing. - registered: logged when the ExApp and its secret are stored, before the deployment. A failed deployment that removes the ExApp again is logged as unregistered. - updated: logged with the old and new version when the new version is stored, before the new version is deployed. - enabled: logged once the ExApp is started and has accepted it, including when an ExApp finishes its initialization. - disabled: logged when the ExApp is disabled in the database, also when the ExApp itself could not be told about it, as it is disabled all the same. - data deleted: logged when the deploy daemon confirms the removal of the data volume, for HaRP, Docker Socket Proxy and Kubernetes. The ExApp secret is never logged. The automatic version change reported by an ExApp in its request headers is not logged, the update itself already is. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 14 ++ lib/Command/ExApp/Unregister.php | 15 +- lib/Controller/ExAppsPageController.php | 10 +- lib/Service/AppAPIService.php | 20 ++- lib/Service/ExAppService.php | 33 +++- tests/php/Command/ExApp/UnregisterTest.php | 126 +++++++++++++ .../Controller/ExAppsPageControllerTest.php | 40 ++++- tests/php/Service/AppAPIServiceTest.php | 124 ++++++++++++- .../php/Service/ExAppServiceAuditLogTest.php | 170 ++++++++++++++++++ .../Service/ExAppServiceGetAppInfoTest.php | 2 + 10 files changed, 541 insertions(+), 13 deletions(-) create mode 100644 tests/php/Command/ExApp/UnregisterTest.php create mode 100644 tests/php/Service/ExAppServiceAuditLogTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index ee8f740d..338a51c8 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -16,6 +16,14 @@ * so changing one is a breaking change for them. Case names may be renamed. */ enum Operation: string { + // ExApps + case ExAppRegistered = 'app_api.ex_app.registered'; + case ExAppUpdated = 'app_api.ex_app.updated'; + case ExAppEnabled = 'app_api.ex_app.enabled'; + case ExAppDisabled = 'app_api.ex_app.disabled'; + case ExAppUnregistered = 'app_api.ex_app.unregistered'; + case ExAppDataDeleted = 'app_api.ex_app_data.deleted'; + // Deploy daemons case DaemonRegistered = 'app_api.daemon.registered'; case DaemonUpdated = 'app_api.daemon.updated'; @@ -28,6 +36,12 @@ enum Operation: string { */ public function description(): string { return match ($this) { + self::ExAppRegistered => 'An ExApp was registered and given its secret to authenticate to Nextcloud, before its deployment', + self::ExAppUpdated => 'An ExApp was switched to a new version, before the deployment of that version', + self::ExAppEnabled => 'An ExApp was enabled and started', + self::ExAppDisabled => 'An ExApp was disabled', + self::ExAppUnregistered => 'An ExApp was removed together with everything it had registered', + self::ExAppDataDeleted => 'The data volume of an ExApp was deleted on its deploy daemon', self::DaemonRegistered => 'A deploy daemon that ExApps can be installed on was registered', self::DaemonUpdated => 'The configuration of a deploy daemon was changed', self::DaemonUnregistered => 'A deploy daemon was unregistered', diff --git a/lib/Command/ExApp/Unregister.php b/lib/Command/ExApp/Unregister.php index fc1bd877..000ce296 100644 --- a/lib/Command/ExApp/Unregister.php +++ b/lib/Command/ExApp/Unregister.php @@ -112,6 +112,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int return 1; } } else { + if ($rmData) { + $this->exAppService->logExAppDataDeleted($exApp); + } if (!$silent) { $output->writeln(sprintf('ExApp %s successfully removed', $appId)); } @@ -137,6 +140,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int $removeVolumeResult = $this->dockerActions->removeVolume( $this->dockerActions->buildDockerUrl($daemonConfig), $volumeName ); + if (!isset($removeVolumeResult['error'])) { + $this->exAppService->logExAppDataDeleted($exApp); + } if (!$silent) { if (isset($removeVolumeResult['error'])) { $output->writeln(sprintf('Failed to remove ExApp %s volume: %s', $appId, $volumeName)); @@ -171,8 +177,13 @@ protected function execute(InputInterface $input, OutputInterface $output): int if (!$force) { return 1; } - } elseif (!$silent) { - $output->writeln(sprintf('ExApp %s K8s resources successfully removed', $appId)); + } else { + if ($rmData) { + $this->exAppService->logExAppDataDeleted($exApp); + } + if (!$silent) { + $output->writeln(sprintf('ExApp %s K8s resources successfully removed', $appId)); + } } } } diff --git a/lib/Controller/ExAppsPageController.php b/lib/Controller/ExAppsPageController.php index 2c1133bb..94f669ba 100644 --- a/lib/Controller/ExAppsPageController.php +++ b/lib/Controller/ExAppsPageController.php @@ -491,12 +491,18 @@ public function uninstallApp(string $appId, bool $removeContainer = true, bool $ if ($daemonConfig->getAcceptsDeployId() === $this->dockerActions->getAcceptsDeployId()) { $this->dockerActions->initGuzzleClient($daemonConfig); if (boolval($exApp->getDeployConfig()['harp'] ?? false)) { - $this->dockerActions->removeExApp($this->dockerActions->buildDockerUrl($daemonConfig), $exApp->getAppid(), removeData: $removeData); + $removeError = $this->dockerActions->removeExApp($this->dockerActions->buildDockerUrl($daemonConfig), $exApp->getAppid(), removeData: $removeData); + if ($removeData && $removeError === '') { + $this->exAppService->logExAppDataDeleted($exApp); + } } else { if ($removeContainer) { $this->dockerActions->removeContainer($this->dockerActions->buildDockerUrl($daemonConfig), $this->dockerActions->buildExAppContainerName($appId)); if ($removeData) { - $this->dockerActions->removeVolume($this->dockerActions->buildDockerUrl($daemonConfig), $this->dockerActions->buildExAppVolumeName($appId)); + $removeVolumeResult = $this->dockerActions->removeVolume($this->dockerActions->buildDockerUrl($daemonConfig), $this->dockerActions->buildExAppVolumeName($appId)); + if (isset($removeVolumeResult['success'])) { + $this->exAppService->logExAppDataDeleted($exApp); + } } } } diff --git a/lib/Service/AppAPIService.php b/lib/Service/AppAPIService.php index 1d86f299..c94f2891 100644 --- a/lib/Service/AppAPIService.php +++ b/lib/Service/AppAPIService.php @@ -10,6 +10,8 @@ namespace OCA\AppAPI\Service; use OCA\AppAPI\AppInfo\Application; +use OCA\AppAPI\AuditLog\AuditLogger; +use OCA\AppAPI\AuditLog\Operation; use OCA\AppAPI\Db\DaemonConfig; use OCA\AppAPI\Db\ExApp; use OCA\AppAPI\DeployActions\DockerActions; @@ -53,6 +55,7 @@ public function __construct( private readonly DaemonConfigService $daemonConfigService, private readonly ExAppDeployOptionsService $exAppDeployOptionsService, private readonly HarpService $harpService, + private readonly AuditLogger $auditLogger, ) { $this->client = $clientService->newClient(); $this->exAppService->setAppAPIService($this); @@ -704,6 +707,9 @@ public function enableExApp(ExApp $exApp): bool { } $this->harpService->harpExAppUpdate($daemonConfig, $exApp, true); + $this->auditLogger->log(Operation::ExAppEnabled, 'ExApp %s was enabled', [ + 'appId' => $exApp->getAppid(), + ]); return true; } @@ -754,7 +760,12 @@ public function disableExApp(ExApp $exApp): bool { } } } - $this->exAppService->disableExAppInternal($exApp); + // The ExApp is disabled even when it could not be told about it + if ($this->exAppService->disableExAppInternal($exApp)) { + $this->auditLogger->log(Operation::ExAppDisabled, 'ExApp %s was disabled', [ + 'appId' => $exApp->getAppid(), + ]); + } if ($result) { $this->harpService->harpExAppUpdate($daemonConfig, $exApp, false); @@ -803,10 +814,13 @@ public function removeExAppsByDaemonConfigName(DaemonConfig $daemonConfig): void if ($daemonConfig->getAcceptsDeployId() === $this->dockerActions->getAcceptsDeployId()) { $this->dockerActions->initGuzzleClient($daemonConfig); if (boolval($exApp->getDeployConfig()['harp'] ?? false)) { - $this->dockerActions->removeExApp($this->dockerActions->buildDockerUrl($daemonConfig), $exApp->getAppid(), true); + $dataDeleted = $this->dockerActions->removeExApp($this->dockerActions->buildDockerUrl($daemonConfig), $exApp->getAppid(), true) === ''; } else { $this->dockerActions->removeContainer($this->dockerActions->buildDockerUrl($daemonConfig), $this->dockerActions->buildExAppContainerName($exApp->getAppid())); - $this->dockerActions->removeVolume($this->dockerActions->buildDockerUrl($daemonConfig), $this->dockerActions->buildExAppVolumeName($exApp->getAppid())); + $dataDeleted = isset($this->dockerActions->removeVolume($this->dockerActions->buildDockerUrl($daemonConfig), $this->dockerActions->buildExAppVolumeName($exApp->getAppid()))['success']); + } + if ($dataDeleted) { + $this->exAppService->logExAppDataDeleted($exApp); } } $this->exAppService->unregisterExApp($exApp->getAppid()); diff --git a/lib/Service/ExAppService.php b/lib/Service/ExAppService.php index d39fab8c..83d39b20 100644 --- a/lib/Service/ExAppService.php +++ b/lib/Service/ExAppService.php @@ -11,6 +11,8 @@ use InvalidArgumentException; use OCA\AppAPI\AppInfo\Application; +use OCA\AppAPI\AuditLog\AuditLogger; +use OCA\AppAPI\AuditLog\Operation; use OCA\AppAPI\Db\ExApp; use OCA\AppAPI\Db\ExAppMapper; use OCA\AppAPI\Fetcher\ExAppArchiveFetcher; @@ -58,6 +60,7 @@ public function __construct( private readonly ExAppDeployOptionsService $deployOptionsService, private readonly ExAppSetupCheckService $setupCheckService, private readonly IConfig $config, + private readonly AuditLogger $auditLogger, ) { if ($cacheFactory->isAvailable()) { $distributedCacheClass = ltrim($config->getSystemValueString('memcache.distributed', ''), '\\'); @@ -100,6 +103,12 @@ public function registerExApp(array $appInfo): ?ExApp { if (isset($appInfo['external-app']['routes'])) { $exApp->setRoutes($this->registerExAppRoutes($exApp, $appInfo['external-app']['routes'])->getRoutes() ?? []); } + $this->auditLogger->log(Operation::ExAppRegistered, 'ExApp %s (version %s) was registered on deploy daemon %s', [ + 'appId' => $exApp->getAppid(), + 'version' => $exApp->getVersion(), + 'daemonName' => $exApp->getDaemonConfigName(), + 'port' => $exApp->getPort(), + ]); return $exApp; } catch (Exception|MultipleObjectsReturnedException|DoesNotExistException $e) { $this->logger->error(sprintf('Error while registering ExApp %s: %s', $appInfo['id'], $e->getMessage())); @@ -128,6 +137,10 @@ public function unregisterExApp(string $appId): bool { $r = $this->exAppMapper->deleteExApp($appId); if ($r !== 1) { $this->logger->error(sprintf('Error while unregistering %s ExApp from the database.', $appId)); + } else { + $this->auditLogger->log(Operation::ExAppUnregistered, 'ExApp %s was unregistered', [ + 'appId' => $appId, + ]); } $rmRoutes = $this->removeExAppRoutes($exApp); if ($rmRoutes === null) { @@ -158,9 +171,19 @@ public function enableExAppInternal(ExApp $exApp): bool { return $this->updateExApp($exApp, ['enabled', 'status']); } - public function disableExAppInternal(ExApp $exApp): void { + public function disableExAppInternal(ExApp $exApp): bool { $exApp->setEnabled(0); - $this->updateExApp($exApp, ['enabled']); + return $this->updateExApp($exApp, ['enabled']); + } + + /** + * Logs the deletion of the data volume of an ExApp on its deploy daemon + */ + public function logExAppDataDeleted(ExApp $exApp): void { + $this->auditLogger->log(Operation::ExAppDataDeleted, 'Data of ExApp %s was deleted from deploy daemon %s', [ + 'appId' => $exApp->getAppid(), + 'daemonName' => $exApp->getDaemonConfigName(), + ]); } public function getExAppsByDaemonName(string $daemonName): array { @@ -203,11 +226,17 @@ public function getNCUsersList(): ?array { } public function updateExAppInfo(ExApp $exApp, array $appInfo): bool { + $oldVersion = $exApp->getVersion(); $exApp->setVersion($appInfo['version']); $exApp->setName($appInfo['name']); if (!$this->updateExApp($exApp, ['version', 'name'])) { return false; } + $this->auditLogger->log(Operation::ExAppUpdated, 'ExApp %s was updated from version %s to %s', [ + 'appId' => $exApp->getAppid(), + 'oldVersion' => $oldVersion, + 'newVersion' => $exApp->getVersion(), + ]); return true; } diff --git a/tests/php/Command/ExApp/UnregisterTest.php b/tests/php/Command/ExApp/UnregisterTest.php new file mode 100644 index 00000000..69a1da6a --- /dev/null +++ b/tests/php/Command/ExApp/UnregisterTest.php @@ -0,0 +1,126 @@ +daemonConfigService = $this->createMock(DaemonConfigService::class); + $this->dockerActions = $this->createMock(DockerActions::class); + $this->dockerActions->method('getAcceptsDeployId')->willReturn('docker-install'); + $this->kubernetesActions = $this->createMock(KubernetesActions::class); + $this->kubernetesActions->method('getAcceptsDeployId')->willReturn('kubernetes-install'); + $this->exAppService = $this->createMock(ExAppService::class); + $this->exAppService->method('unregisterExApp')->willReturn(true); + + $this->commandTester = new CommandTester(new Unregister( + $this->createMock(AppAPIService::class), + $this->daemonConfigService, + $this->dockerActions, + $this->kubernetesActions, + $this->exAppService, + $this->createMock(ExAppDeployOptionsService::class), + )); + } + + private function registerExApp(string $acceptsDeployId, bool $isHarp): ExApp { + $exApp = new ExApp([ + 'appid' => 'test_app', + 'daemon_config_name' => 'test_daemon', + 'enabled' => 0, + ]); + $exApp->setDeployConfig($isHarp ? ['harp' => ['frp_address' => 'appapi-harp:8782']] : []); + $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); + $this->daemonConfigService->method('getDaemonConfigByName') + ->willReturn(new DaemonConfig(['name' => 'test_daemon', 'accepts_deploy_id' => $acceptsDeployId])); + return $exApp; + } + + public static function harpProvider(): array { + return [ + 'removed with its data' => [true, '', true], + 'removed without its data' => [false, '', false], + 'removal failed' => [true, 'Failed to remove ExApp', false], + ]; + } + + #[DataProvider('harpProvider')] + public function testHarpExAppDataDeletionIsLogged(bool $rmData, string $removeError, bool $logged): void { + $exApp = $this->registerExApp('docker-install', true); + $this->dockerActions->method('removeExApp')->willReturn($removeError); + $this->exAppService->expects($logged ? self::once() : self::never())->method('logExAppDataDeleted')->with($exApp); + + $this->commandTester->execute(['appid' => 'test_app', '--rm-data' => $rmData, '--force' => true, '--silent' => true]); + } + + public static function dockerSocketProxyProvider(): array { + return [ + 'volume removed' => [['success' => true], true], + 'volume not removed' => [['error' => 'Volume is in use.'], false], + ]; + } + + #[DataProvider('dockerSocketProxyProvider')] + public function testDockerSocketProxyExAppDataDeletionIsLogged(array $removeVolumeResult, bool $logged): void { + $exApp = $this->registerExApp('docker-install', false); + $this->dockerActions->method('removeContainer')->willReturn(''); + $this->dockerActions->method('removeVolume')->willReturn($removeVolumeResult); + $this->exAppService->expects($logged ? self::once() : self::never())->method('logExAppDataDeleted')->with($exApp); + + $this->commandTester->execute(['appid' => 'test_app', '--rm-data' => true, '--silent' => true]); + } + + public function testDockerSocketProxyExAppDataIsKeptByDefault(): void { + $this->registerExApp('docker-install', false); + $this->dockerActions->method('removeContainer')->willReturn(''); + $this->dockerActions->expects(self::never())->method('removeVolume'); + $this->exAppService->expects(self::never())->method('logExAppDataDeleted'); + + $this->commandTester->execute(['appid' => 'test_app', '--silent' => true]); + } + + public static function kubernetesProvider(): array { + return [ + 'removed with its data' => [true, '', true], + 'removed without its data' => [false, '', false], + 'removal failed' => [true, 'K8s deployment not found', false], + ]; + } + + #[DataProvider('kubernetesProvider')] + public function testKubernetesExAppDataDeletionIsLogged(bool $rmData, string $removeError, bool $logged): void { + $exApp = $this->registerExApp('kubernetes-install', true); + $this->kubernetesActions->method('removeExApp')->willReturn($removeError); + $this->exAppService->expects($logged ? self::once() : self::never())->method('logExAppDataDeleted')->with($exApp); + + $this->commandTester->execute(['appid' => 'test_app', '--rm-data' => $rmData, '--force' => true, '--silent' => true]); + } +} diff --git a/tests/php/Controller/ExAppsPageControllerTest.php b/tests/php/Controller/ExAppsPageControllerTest.php index 0a8c366b..91633710 100644 --- a/tests/php/Controller/ExAppsPageControllerTest.php +++ b/tests/php/Controller/ExAppsPageControllerTest.php @@ -11,6 +11,8 @@ use OC\App\AppStore\Fetcher\CategoryFetcher; use OCA\AppAPI\Controller\ExAppsPageController; +use OCA\AppAPI\Db\DaemonConfig; +use OCA\AppAPI\Db\ExApp; use OCA\AppAPI\DeployActions\DockerActions; use OCA\AppAPI\Fetcher\ExAppFetcher; use OCA\AppAPI\Service\AppAPIService; @@ -23,6 +25,7 @@ use OCP\IL10N; use OCP\IRequest; use OCP\L10N\IFactory; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -36,6 +39,7 @@ class ExAppsPageControllerTest extends TestCase { private DaemonConfigService&MockObject $daemonConfigService; private IConfig&MockObject $config; private IAppManager&MockObject $appManager; + private DockerActions&MockObject $dockerActions; protected function setUp(): void { parent::setUp(); @@ -44,7 +48,7 @@ protected function setUp(): void { $this->config = $this->createMock(IConfig::class); $service = $this->createMock(AppAPIService::class); $this->daemonConfigService = $this->createMock(DaemonConfigService::class); - $dockerActions = $this->createMock(DockerActions::class); + $this->dockerActions = $this->createMock(DockerActions::class); $categoryFetcher = $this->createMock(CategoryFetcher::class); $this->l10nFactory = $this->createMock(IFactory::class); $this->exAppFetcher = $this->createMock(ExAppFetcher::class); @@ -59,7 +63,7 @@ protected function setUp(): void { $this->config, $service, $this->daemonConfigService, - $dockerActions, + $this->dockerActions, $categoryFetcher, $this->l10nFactory, $this->exAppFetcher, @@ -228,4 +232,36 @@ public function testForceRecoversFromNonArrayOverwriteValue(): void { self::assertInstanceOf(JSONResponse::class, $this->controller->force('my_exapp')); } + + public static function uninstallDataProvider(): array { + return [ + 'HaRP, data removed' => [true, true, '', null, true], + 'HaRP, data kept' => [true, false, '', null, false], + 'HaRP, removal failed' => [true, true, 'Failed to remove ExApp', null, false], + 'Docker Socket Proxy, volume removed' => [false, true, '', ['success' => true], true], + 'Docker Socket Proxy, volume not removed' => [false, true, '', ['error' => 'Volume is in use.'], false], + 'Docker Socket Proxy, data kept' => [false, false, '', null, false], + ]; + } + + #[DataProvider('uninstallDataProvider')] + public function testUninstallAppLogsTheDeletedData(bool $isHarp, bool $removeData, string $removeExAppError, ?array $removeVolumeResult, bool $logged): void { + $exApp = new ExApp([ + 'appid' => 'fake_app', + 'daemon_config_name' => 'docker_daemon', + 'enabled' => 0, + ]); + $exApp->setDeployConfig($isHarp ? ['harp' => ['frp_address' => 'appapi-harp:8782']] : []); + $this->exAppService->method('getExApp')->with('fake_app')->willReturn($exApp); + $this->daemonConfigService->method('getDaemonConfigByName') + ->willReturn(new DaemonConfig(['name' => 'docker_daemon', 'accepts_deploy_id' => 'docker-install'])); + $this->dockerActions->method('getAcceptsDeployId')->willReturn('docker-install'); + $this->dockerActions->method('removeExApp')->willReturn($removeExAppError); + $this->dockerActions->expects($removeData && !$isHarp ? self::once() : self::never()) + ->method('removeVolume')->willReturn($removeVolumeResult ?? []); + $this->exAppService->expects($logged ? self::once() : self::never())->method('logExAppDataDeleted')->with($exApp); + $this->exAppService->expects(self::once())->method('unregisterExApp')->with('fake_app'); + + $this->controller->uninstallApp('fake_app', true, $removeData); + } } diff --git a/tests/php/Service/AppAPIServiceTest.php b/tests/php/Service/AppAPIServiceTest.php index bc2ef7a8..9db8dd74 100644 --- a/tests/php/Service/AppAPIServiceTest.php +++ b/tests/php/Service/AppAPIServiceTest.php @@ -9,6 +9,9 @@ namespace OCA\AppAPI\Tests\php\Service; +use OCA\AppAPI\AuditLog\AuditLogger; +use OCA\AppAPI\AuditLog\Operation; +use OCA\AppAPI\Db\DaemonConfig; use OCA\AppAPI\Db\ExApp; use OCA\AppAPI\DeployActions\DockerActions; use OCA\AppAPI\DeployActions\KubernetesActions; @@ -21,12 +24,14 @@ use OCA\AppAPI\Service\HarpService; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; +use OCP\Http\Client\IResponse; use OCP\IConfig; use OCP\ISession; use OCP\IUserManager; use OCP\IUserSession; use OCP\L10N\IFactory; use OCP\Security\Bruteforce\IThrottler; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -48,6 +53,8 @@ class AppAPIServiceTest extends TestCase { private IUserSession&MockObject $userSession; private ISession&MockObject $session; private IUserManager&MockObject $userManager; + private DaemonConfigService&MockObject $daemonConfigService; + private AuditLogger&MockObject $auditLogger; protected function setUp(): void { parent::setUp(); @@ -68,7 +75,8 @@ protected function setUp(): void { $this->kubernetesActions = $this->createMock(KubernetesActions::class); $this->manualActions = $this->createMock(ManualActions::class); $this->commonService = $this->createMock(AppAPICommonService::class); - $daemonConfigService = $this->createMock(DaemonConfigService::class); + $this->daemonConfigService = $this->createMock(DaemonConfigService::class); + $this->auditLogger = $this->createMock(AuditLogger::class); $exAppDeployOptionsService = $this->createMock(ExAppDeployOptionsService::class); $harpService = $this->createMock(HarpService::class); @@ -87,9 +95,10 @@ protected function setUp(): void { $this->kubernetesActions, $this->manualActions, $this->commonService, - $daemonConfigService, + $this->daemonConfigService, $exAppDeployOptionsService, $harpService, + $this->auditLogger, ); } @@ -300,4 +309,115 @@ public function testValidateInvalidSecretRejected(): void { self::assertFalse($this->service->validateExAppRequestToNC($request)); } + + private function createResponse(string $body, int $statusCode = 200): IResponse&MockObject { + $response = $this->createMock(IResponse::class); + $response->method('getStatusCode')->willReturn($statusCode); + $response->method('getBody')->willReturn($body); + return $response; + } + + private function setupReachableManualExApp(ExApp $exApp, string $enabledResponseBody): void { + $this->setupExAppUrlMocks(); + $this->daemonConfigService->method('getDaemonConfigByName')->willReturn(new DaemonConfig(['name' => 'test_daemon'])); + $this->exAppService->method('getExApp')->willReturn($exApp); + $this->commonService->method('buildAppAPIAuthHeaders')->willReturn([]); + $this->client->method('get')->willReturn($this->createResponse('{"status":"ok"}')); + $this->client->method('put')->willReturn($this->createResponse($enabledResponseBody)); + } + + public function testEnableExAppLogsTheExApp(): void { + $exApp = $this->createExApp(enabled: false); + $this->setupReachableManualExApp($exApp, '{}'); + $this->exAppService->method('enableExAppInternal')->willReturn(true); + $this->auditLogger->expects(self::once())->method('log') + ->with(Operation::ExAppEnabled, 'ExApp %s was enabled', ['appId' => 'test_app']); + + self::assertTrue($this->service->enableExApp($exApp)); + } + + public function testEnableExAppDoesNotLogWhenTheExAppRefuses(): void { + $exApp = $this->createExApp(enabled: false); + $this->setupReachableManualExApp($exApp, '{"error":"missing configuration"}'); + $this->exAppService->method('enableExAppInternal')->willReturn(true); + $this->exAppService->expects(self::once())->method('disableExAppInternal')->with($exApp); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertFalse($this->service->enableExApp($exApp)); + } + + public function testEnableExAppDoesNotLogWhenItCannotBeStored(): void { + $exApp = $this->createExApp(enabled: false); + $this->exAppService->method('enableExAppInternal')->willReturn(false); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertFalse($this->service->enableExApp($exApp)); + } + + public function testDisableExAppLogsTheExApp(): void { + $exApp = $this->createExApp(); + $this->setupReachableManualExApp($exApp, '{}'); + $this->exAppService->method('disableExAppInternal')->willReturn(true); + $this->auditLogger->expects(self::once())->method('log') + ->with(Operation::ExAppDisabled, 'ExApp %s was disabled', ['appId' => 'test_app']); + + self::assertTrue($this->service->disableExApp($exApp)); + } + + public function testDisableExAppLogsAnExAppThatCouldNotBeToldAboutIt(): void { + $exApp = $this->createExApp(); + $this->setupReachableManualExApp($exApp, '{"error":"busy"}'); + $this->exAppService->expects(self::once())->method('disableExAppInternal')->with($exApp)->willReturn(true); + $this->auditLogger->expects(self::once())->method('log') + ->with(Operation::ExAppDisabled, 'ExApp %s was disabled', ['appId' => 'test_app']); + + self::assertFalse($this->service->disableExApp($exApp)); + } + + public function testDisableExAppDoesNotLogWhenItCannotBeStored(): void { + $exApp = $this->createExApp(); + $this->setupReachableManualExApp($exApp, '{}'); + $this->exAppService->method('disableExAppInternal')->willReturn(false); + $this->auditLogger->expects(self::never())->method('log'); + + $this->service->disableExApp($exApp); + } + + public function testDisableExAppDoesNotLogWithoutItsDaemon(): void { + $exApp = $this->createExApp(); + $this->setupExAppUrlMocks(); + $this->commonService->method('buildAppAPIAuthHeaders')->willReturn([]); + $this->client->method('put')->willReturn($this->createResponse('{}')); + $this->daemonConfigService->method('getDaemonConfigByName')->willReturn(null); + $this->exAppService->expects(self::never())->method('disableExAppInternal'); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertFalse($this->service->disableExApp($exApp)); + } + + public static function removeExAppsByDaemonProvider(): array { + return [ + 'HaRP, removed' => [true, '', null, true], + 'HaRP, failed' => [true, 'Failed to remove ExApp', null, false], + 'Docker Socket Proxy, volume removed' => [false, '', ['success' => true], true], + 'Docker Socket Proxy, volume not removed' => [false, '', ['error' => 'Volume is in use.'], false], + ]; + } + + #[DataProvider('removeExAppsByDaemonProvider')] + public function testRemoveExAppsByDaemonConfigNameLogsTheDeletedData(bool $isHarp, string $removeExAppError, ?array $removeVolumeResult, bool $logged): void { + $exApp = $this->createExApp(); + $exApp->setAcceptsDeployId('docker-install'); + $exApp->setDeployConfig($isHarp ? ['harp' => ['frp_address' => 'appapi-harp:8782']] : []); + $daemonConfig = new DaemonConfig(['name' => 'test_daemon', 'accepts_deploy_id' => 'docker-install']); + $this->setupReachableManualExApp($exApp, '{}'); + $this->exAppService->method('getExAppsByDaemonName')->with('test_daemon')->willReturn([$exApp]); + $this->exAppService->method('disableExAppInternal')->willReturn(true); + $this->dockerActions->method('removeExApp')->willReturn($removeExAppError); + $this->dockerActions->method('removeVolume')->willReturn($removeVolumeResult ?? []); + $this->exAppService->expects($logged ? self::once() : self::never())->method('logExAppDataDeleted')->with($exApp); + $this->exAppService->expects(self::once())->method('unregisterExApp')->with('test_app'); + + $this->service->removeExAppsByDaemonConfigName($daemonConfig); + } } diff --git a/tests/php/Service/ExAppServiceAuditLogTest.php b/tests/php/Service/ExAppServiceAuditLogTest.php new file mode 100644 index 00000000..221af76b --- /dev/null +++ b/tests/php/Service/ExAppServiceAuditLogTest.php @@ -0,0 +1,170 @@ +createMock(ICacheFactory::class); + $cacheFactory->method('isAvailable')->willReturn(false); + $this->exAppMapper = $this->createMock(ExAppMapper::class); + $this->auditLogger = $this->createMock(AuditLogger::class); + + $this->service = new ExAppService( + $this->createMock(LoggerInterface::class), + $cacheFactory, + $this->createMock(IUserManager::class), + $this->createMock(ExAppFetcher::class), + $this->createMock(ExAppArchiveFetcher::class), + $this->exAppMapper, + $this->createMock(TopMenuService::class), + $this->createMock(InitialStateService::class), + $this->createMock(ScriptsService::class), + $this->createMock(StylesService::class), + $this->createMock(FilesActionsMenuService::class), + $this->createMock(TaskProcessingService::class), + $this->createMock(TalkBotsService::class), + $this->createMock(SettingsService::class), + $this->createMock(ExAppOccService::class), + $this->createMock(ExAppDeployOptionsService::class), + $this->createMock(ExAppSetupCheckService::class), + $this->createMock(IConfig::class), + $this->auditLogger, + ); + } + + private static function appInfo(): array { + return [ + 'id' => 'test_app', + 'version' => '1.0.0', + 'name' => 'Test App', + 'daemon_config_name' => 'harp_proxy', + 'port' => 23000, + 'secret' => 'the_secret_of_the_exapp', + ]; + } + + private static function exApp(): ExApp { + return new ExApp([ + 'appid' => 'test_app', + 'version' => '1.0.0', + 'name' => 'Test App', + 'daemon_config_name' => 'harp_proxy', + 'port' => 23000, + 'secret' => 'the_secret_of_the_exapp', + ]); + } + + public function testRegisterExAppLogsTheExAppWithoutItsSecret(): void { + $this->exAppMapper->expects(self::once())->method('insert')->willReturnArgument(0); + $this->exAppMapper->method('findByAppId')->willReturn(self::exApp()); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::ExAppRegistered, + 'ExApp %s (version %s) was registered on deploy daemon %s', + ['appId' => 'test_app', 'version' => '1.0.0', 'daemonName' => 'harp_proxy', 'port' => 23000], + ); + + self::assertInstanceOf(ExApp::class, $this->service->registerExApp(self::appInfo())); + } + + public function testRegisterExAppDoesNotLogADatabaseError(): void { + $this->exAppMapper->method('insert')->willThrowException(new Exception('duplicate appid')); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertNull($this->service->registerExApp(self::appInfo())); + } + + public function testUnregisterExAppLogsTheExApp(): void { + $this->exAppMapper->method('findAll')->willReturn([self::exApp()]); + $this->exAppMapper->expects(self::once())->method('deleteExApp')->with('test_app')->willReturn(1); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::ExAppUnregistered, + 'ExApp %s was unregistered', + ['appId' => 'test_app'], + ); + + self::assertTrue($this->service->unregisterExApp('test_app')); + } + + public function testUnregisterExAppDoesNotLogWhenNothingWasDeleted(): void { + $this->exAppMapper->method('findAll')->willReturn([self::exApp()]); + $this->exAppMapper->method('deleteExApp')->willReturn(0); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertFalse($this->service->unregisterExApp('test_app')); + } + + public function testUnregisterExAppDoesNotLogAnUnknownExApp(): void { + $this->exAppMapper->method('findAll')->willReturn([]); + $this->exAppMapper->expects(self::never())->method('deleteExApp'); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertFalse($this->service->unregisterExApp('test_app')); + } + + public function testUpdateExAppInfoLogsBothVersions(): void { + $this->exAppMapper->expects(self::once())->method('updateExApp'); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::ExAppUpdated, + 'ExApp %s was updated from version %s to %s', + ['appId' => 'test_app', 'oldVersion' => '1.0.0', 'newVersion' => '1.1.0'], + ); + + self::assertTrue($this->service->updateExAppInfo(self::exApp(), ['version' => '1.1.0', 'name' => 'Test App'])); + } + + public function testUpdateExAppInfoDoesNotLogADatabaseError(): void { + $this->exAppMapper->method('updateExApp')->willThrowException(new Exception('database is gone')); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertFalse($this->service->updateExAppInfo(self::exApp(), ['version' => '1.1.0', 'name' => 'Test App'])); + } + + public function testLogExAppDataDeleted(): void { + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::ExAppDataDeleted, + 'Data of ExApp %s was deleted from deploy daemon %s', + ['appId' => 'test_app', 'daemonName' => 'harp_proxy'], + ); + + $this->service->logExAppDataDeleted(self::exApp()); + } +} diff --git a/tests/php/Service/ExAppServiceGetAppInfoTest.php b/tests/php/Service/ExAppServiceGetAppInfoTest.php index 8e052df8..f68454db 100644 --- a/tests/php/Service/ExAppServiceGetAppInfoTest.php +++ b/tests/php/Service/ExAppServiceGetAppInfoTest.php @@ -9,6 +9,7 @@ namespace OCA\AppAPI\Tests\php\Service; +use OCA\AppAPI\AuditLog\AuditLogger; use OCA\AppAPI\Db\ExAppMapper; use OCA\AppAPI\Fetcher\ExAppArchiveFetcher; use OCA\AppAPI\Fetcher\ExAppFetcher; @@ -65,6 +66,7 @@ protected function setUp(): void { $this->createMock(ExAppDeployOptionsService::class), $this->createMock(ExAppSetupCheckService::class), $this->createMock(IConfig::class), + $this->createMock(AuditLogger::class), ); } From 1fb786dd6ef14369279fed6c04da0556aa9f9add Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:47:00 +0200 Subject: [PATCH 3/4] feat(settings): log AppAPI admin settings changes to the audit log Changing an AppAPI admin setting, such as the default deploy daemon, the initialization timeout or the container restart policy, is now written to the admin audit log, once for each setting whose value actually changed. The endpoint accepts any key, so the value is only logged for the known settings of the admin page; for any other key only its name is logged, as it could hold a secret. The default deploy daemon is also set when a daemon is registered as the default one, and cleared when it is removed. Those writes are not logged as a setting change; the daemon registration and removal themselves are. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 4 ++ lib/Controller/ConfigController.php | 14 +++- tests/php/Controller/ConfigControllerTest.php | 68 +++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 tests/php/Controller/ConfigControllerTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 338a51c8..06988675 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -31,6 +31,9 @@ enum Operation: string { case DaemonRegistryAdded = 'app_api.daemon_registry.added'; case DaemonRegistryRemoved = 'app_api.daemon_registry.removed'; + // Settings + case AdminConfigUpdated = 'app_api.admin_config.updated'; + /** * Short description of when the operation is logged, used for the documentation */ @@ -47,6 +50,7 @@ public function description(): string { self::DaemonUnregistered => 'A deploy daemon was unregistered', self::DaemonRegistryAdded => 'A deploy daemon was set to pull the images of a Docker registry from another registry', self::DaemonRegistryRemoved => 'A Docker registry mapping was removed from a deploy daemon', + self::AdminConfigUpdated => 'An AppAPI admin setting, such as the default deploy daemon, was changed', }; } } diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index 37461339..2d0a2191 100644 --- a/lib/Controller/ConfigController.php +++ b/lib/Controller/ConfigController.php @@ -10,6 +10,8 @@ namespace OCA\AppAPI\Controller; use OCA\AppAPI\AppInfo\Application; +use OCA\AppAPI\AuditLog\AuditLogger; +use OCA\AppAPI\AuditLog\Operation; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; @@ -20,11 +22,14 @@ #[OpenAPI(OpenAPI::SCOPE_ADMINISTRATION)] class ConfigController extends Controller { + /** Settings whose value is written to the audit log; any other key could hold a secret, so only its name is */ + private const AUDITED_VALUE_KEYS = ['default_daemon_config', 'init_timeout', 'container_restart_policy', 'docker_api_version']; public function __construct( string $appName, IRequest $request, private readonly IAppConfig $appConfig, + private readonly AuditLogger $auditLogger, ) { parent::__construct($appName, $request); } @@ -41,7 +46,14 @@ public function __construct( #[NoCSRFRequired] public function setAdminConfig(array $values): DataResponse { foreach ($values as $key => $value) { - $this->appConfig->setValueString(Application::APP_ID, $key, $value, lazy: true); + if (!$this->appConfig->setValueString(Application::APP_ID, $key, $value, lazy: true)) { + continue; + } + if (in_array($key, self::AUDITED_VALUE_KEYS, true)) { + $this->auditLogger->log(Operation::AdminConfigUpdated, 'AppAPI setting %s was changed to %s', ['key' => $key, 'value' => $value]); + } else { + $this->auditLogger->log(Operation::AdminConfigUpdated, 'AppAPI setting %s was changed', ['key' => $key]); + } } return new DataResponse(1); } diff --git a/tests/php/Controller/ConfigControllerTest.php b/tests/php/Controller/ConfigControllerTest.php new file mode 100644 index 00000000..710368c5 --- /dev/null +++ b/tests/php/Controller/ConfigControllerTest.php @@ -0,0 +1,68 @@ +appConfig = $this->createMock(IAppConfig::class); + $this->auditLogger = $this->createMock(AuditLogger::class); + $this->controller = new ConfigController('app_api', $this->createMock(IRequest::class), $this->appConfig, $this->auditLogger); + } + + public function testSetAdminConfigLogsEachChangedSetting(): void { + $this->appConfig->expects(self::exactly(3))->method('setValueString') + ->willReturnCallback(static fn (string $app, string $key, string $value): bool => $key !== 'init_timeout'); + $logged = []; + $this->auditLogger->expects(self::exactly(2))->method('log') + ->willReturnCallback(static function (Operation $operation, string $message, array $parameters) use (&$logged): void { + $logged[] = [$operation, $message, $parameters]; + }); + + $this->controller->setAdminConfig([ + 'default_daemon_config' => 'harp_proxy', + 'init_timeout' => '40', + 'container_restart_policy' => 'always', + ]); + + self::assertSame([ + [Operation::AdminConfigUpdated, 'AppAPI setting %s was changed to %s', ['key' => 'default_daemon_config', 'value' => 'harp_proxy']], + [Operation::AdminConfigUpdated, 'AppAPI setting %s was changed to %s', ['key' => 'container_restart_policy', 'value' => 'always']], + ], $logged); + } + + public function testSetAdminConfigDoesNotLogTheValueOfAnUnknownSetting(): void { + $this->appConfig->method('setValueString')->willReturn(true); + $this->auditLogger->expects(self::once())->method('log') + ->with(Operation::AdminConfigUpdated, 'AppAPI setting %s was changed', ['key' => 'some_token']); + + $this->controller->setAdminConfig(['some_token' => 'a secret value']); + } + + public function testSetAdminConfigDoesNotLogAnUnchangedSetting(): void { + $this->appConfig->method('setValueString')->willReturn(false); + $this->auditLogger->expects(self::never())->method('log'); + + $this->controller->setAdminConfig(['init_timeout' => '40']); + } +} From c02350b13fa13e1679ca63c11a42a7a8ceac2b07 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:47:44 +0200 Subject: [PATCH 4/4] feat(auth): log failed AppAPI authentication to the audit log A request with AppAPI authentication that is rejected because the ExApp is unknown, the authorization header cannot be decoded or the secret is wrong is now written to the admin audit log, like a failed login. The entry names the ExApp and the user the request claims to act for, both as sent by the client, and the reason. The secret is never logged. Requests that authenticate correctly are not logged, also when they act for a user: they are already written to exapp_impersonation.log and would add an entry per request. Rejections of correctly authenticated requests, for example to a disabled ExApp, are not authentication failures and are not logged either. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 2 + lib/Service/AppAPIService.php | 17 +++++++- tests/php/Service/AppAPIServiceTest.php | 52 +++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 06988675..aebfce8e 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -23,6 +23,7 @@ enum Operation: string { case ExAppDisabled = 'app_api.ex_app.disabled'; case ExAppUnregistered = 'app_api.ex_app.unregistered'; case ExAppDataDeleted = 'app_api.ex_app_data.deleted'; + case ExAppAuthFailed = 'app_api.ex_app_auth.failed'; // Deploy daemons case DaemonRegistered = 'app_api.daemon.registered'; @@ -45,6 +46,7 @@ public function description(): string { self::ExAppDisabled => 'An ExApp was disabled', self::ExAppUnregistered => 'An ExApp was removed together with everything it had registered', self::ExAppDataDeleted => 'The data volume of an ExApp was deleted on its deploy daemon', + self::ExAppAuthFailed => 'A request with AppAPI authentication was rejected for an unknown ExApp or a wrong secret', self::DaemonRegistered => 'A deploy daemon that ExApps can be installed on was registered', self::DaemonUpdated => 'The configuration of a deploy daemon was changed', self::DaemonUnregistered => 'A deploy daemon was unregistered', diff --git a/lib/Service/AppAPIService.php b/lib/Service/AppAPIService.php index c94f2891..a8ceba5c 100644 --- a/lib/Service/AppAPIService.php +++ b/lib/Service/AppAPIService.php @@ -290,17 +290,20 @@ public function validateExAppRequestToNC(IRequest $request, bool $isDav = false) $exApp = $this->exAppService->getExApp($exAppId); if ($exApp === null) { $this->logger->error(sprintf('ExApp with appId %s not found.', $request->getHeader('EX-APP-ID'))); + $userId = explode(':', base64_decode($request->getHeader('AUTHORIZATION-APP-API')), 2)[0]; // Protection for guessing installed ExApps list $this->throttler->registerAttempt(Application::APP_ID, $request->getRemoteAddress(), [ 'appid' => $request->getHeader('EX-APP-ID'), - 'userid' => explode(':', base64_decode($request->getHeader('AUTHORIZATION-APP-API')), 2)[0], + 'userid' => $userId, ]); + $this->logAuthenticationFailure($exAppId, $userId, 'unknown_ex_app'); return false; } $authorization = base64_decode($request->getHeader('AUTHORIZATION-APP-API')); if ($authorization === false) { $this->logger->error('Failed to parse AUTHORIZATION-APP-API'); + $this->logAuthenticationFailure($exAppId, '', 'malformed_authorization'); return false; } $userId = explode(':', $authorization, 2)[0]; @@ -328,12 +331,24 @@ public function validateExAppRequestToNC(IRequest $request, bool $isDav = false) 'appid' => $request->getHeader('EX-APP-ID'), 'userid' => $userId, ]); + $this->logAuthenticationFailure($exApp->getAppid(), $userId, 'invalid_secret'); } $this->logger->error(sprintf('ExApp %s request to NC validation failed.', $exApp->getAppid())); return false; } + /** + * @param string $userId the user the request claims to act for, as sent by the client + */ + private function logAuthenticationFailure(string $exAppId, string $userId, string $reason): void { + $this->auditLogger->log(Operation::ExAppAuthFailed, 'Failed AppAPI authentication of ExApp %s for user %s', [ + 'exAppId' => $exAppId, + 'userId' => $userId, + 'reason' => $reason, + ]); + } + /** * Final step of AppAPI authentication request validation for Nextcloud: * - sets active user (null if not a user context) diff --git a/tests/php/Service/AppAPIServiceTest.php b/tests/php/Service/AppAPIServiceTest.php index 9db8dd74..6bec8557 100644 --- a/tests/php/Service/AppAPIServiceTest.php +++ b/tests/php/Service/AppAPIServiceTest.php @@ -278,6 +278,7 @@ public function testValidateDisabledExAppRejectedForNonExemptEndpoint(): void { $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + $this->auditLogger->expects(self::never())->method('log'); self::assertFalse($this->service->validateExAppRequestToNC($request)); } @@ -292,6 +293,7 @@ public function testValidateEnabledExAppAllowedEverywhere(): void { $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + $this->auditLogger->expects(self::never())->method('log'); self::assertTrue($this->service->validateExAppRequestToNC($request)); } @@ -306,6 +308,11 @@ public function testValidateInvalidSecretRejected(): void { $this->exAppService->method('getExApp')->with('test_app')->willReturn($exApp); $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::ExAppAuthFailed, + 'Failed AppAPI authentication of ExApp %s for user %s', + ['exAppId' => 'test_app', 'userId' => '', 'reason' => 'invalid_secret'], + ); self::assertFalse($this->service->validateExAppRequestToNC($request)); } @@ -420,4 +427,49 @@ public function testRemoveExAppsByDaemonConfigNameLogsTheDeletedData(bool $isHar $this->service->removeExAppsByDaemonConfigName($daemonConfig); } + + private function createRequestForUser(string $appId, string $userId, string $secret): MockObject&\OCP\IRequest { + $request = $this->createMock(\OCP\IRequest::class); + $request->method('getRemoteAddress')->willReturn('127.0.0.1'); + $request->method('getHeader')->willReturnCallback(static fn (string $name): string => match ($name) { + 'EX-APP-ID' => $appId, + 'EX-APP-VERSION' => '1.0.0', + 'AUTHORIZATION-APP-API' => base64_encode($userId . ':' . $secret), + default => '', + }); + $request->method('getPathInfo')->willReturn('/ocs/v1.php/apps/app_api/api/v1/log'); + return $request; + } + + public function testValidateLogsAWrongSecretWithTheClaimedUser(): void { + $this->exAppService->method('getExApp')->with('test_app')->willReturn($this->createExApp()); + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::ExAppAuthFailed, + 'Failed AppAPI authentication of ExApp %s for user %s', + ['exAppId' => 'test_app', 'userId' => 'alice', 'reason' => 'invalid_secret'], + ); + + self::assertFalse($this->service->validateExAppRequestToNC($this->createRequestForUser('test_app', 'alice', 'wrong_secret'))); + } + + public function testValidateLogsAnUnknownExApp(): void { + $this->exAppService->method('getExApp')->with('unknown_app')->willReturn(null); + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + $this->throttler->expects(self::once())->method('registerAttempt'); + $this->auditLogger->expects(self::once())->method('log')->with( + Operation::ExAppAuthFailed, + 'Failed AppAPI authentication of ExApp %s for user %s', + ['exAppId' => 'unknown_app', 'userId' => 'alice', 'reason' => 'unknown_ex_app'], + ); + + self::assertFalse($this->service->validateExAppRequestToNC($this->createRequestForUser('unknown_app', 'alice', 'any_secret'))); + } + + public function testValidateDoesNotLogARequestWithoutAnExAppId(): void { + $this->throttler->method('sleepDelayOrThrowOnMax')->willReturn(0); + $this->auditLogger->expects(self::never())->method('log'); + + self::assertFalse($this->service->validateExAppRequestToNC($this->createRequestForUser('', 'alice', 'any_secret'))); + } }