Skip to content

Feat/admin audit logging #1130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
GhassenKefi wants to merge 4 commits into
base: main
Choose a base branch
from
+1,040 −26
Open

Feat/admin audit logging #1130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23e7251
feat(daemons): log deploy daemon changes to the audit log
GhassenKefi Oct 19, 2026
a8db189
feat(exapps): log the ExApp lifecycle to the audit log
GhassenKefi Oct 19, 2026
1fb786d
feat(settings): log AppAPI admin settings changes to the audit log
GhassenKefi Oct 19, 2026
c02350b
feat(auth): log failed AppAPI authentication to the audit log
GhassenKefi Oct 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions lib/AuditLog/AuditLogger.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\AppAPI\AuditLog;

use OCP\EventDispatcher\IEventDispatcher;
use OCP\Log\Audit\CriticalActionPerformedEvent;

/**
* Writes entries to the admin audit log
*/
class AuditLogger {
public function __construct(
private readonly IEventDispatcher $eventDispatcher,
) {
}

/**
* The operation is only passed on to servers whose CriticalActionPerformedEvent supports it
*
* @param string $message message with a `%s` placeholder for each of the first parameters
* @param array<string, mixed> $parameters named values for the placeholders of $message, in order, optionally followed by more context

@coderabbitai coderabbitai Bot Oct 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

sed -n '1,110p' lib/AuditLog/AuditLogger.php
rg -n 'passwordChanged|auditLogger->log|auditLogger\?->log|AuditLogger' lib/Controller lib/Service

Repository: nextcloud/app_api

Length of output: 3870

🏁 Script executed:

set -o pipefail
printf '%s\n' '--- PR diff for AuditLogger and callers ---'
git diff --no-ext-diff --unified=20 38a8a35fa20c6d060b477920ed9c36590e0367b5 c02350b13fa13e1679ca63c11a42a7a8ceac2b07 -- lib/AuditLog/AuditLogger.php lib/Controller lib/Service

printf '%s\n' '--- all AuditLogger references and passwordChanged ---'
rg -n -F --glob '*.php' -e '->auditLogger->log(' -e 'AuditLogger' -e 'passwordChanged' . || test "$?" -eq 1

printf '%s\n' '--- caller context ---'
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('lib').rglob('*.php'):
    text = p.read_text()
    if '->auditLogger->log(' not in text and 'passwordChanged' not in text:
        continue
    lines = text.splitlines()
    for i, line in enumerate(lines):
        if '->auditLogger->log(' in line or 'passwordChanged' in line:
            lo=max(0,i-4); hi=min(len(lines),i+14)
            print(f'--- {p}:{lo+1}-{hi} ---')
            for n in range(lo,hi):
                print(f'{n+1}: {lines[n]}')
PY

printf '%s\n' '--- Nextcloud 35 listener ---'
curl -L --fail --silent --show-error --max-time 20 https://raw.githubusercontent.com/nextcloud/server/stable35/apps/admin_audit/lib/Listener/CriticalActionPerformedEventListener.php

Repository: nextcloud/app_api

Length of output: 41766

🏁 Script executed:

curl -L --fail --silent --show-error --max-time 20 https://raw.githubusercontent.com/nextcloud/server/stable35/apps/admin_audit/lib/Actions/Action.php

Repository: nextcloud/app_api

Length of output: 1683

Preserve additional audit context in the written log entry.

On Nextcloud 35, admin_audit passes all parameters to vsprintf(), but persists only formatted placeholders and {name} context fields. Extra values can disappear from the audit entry.

Current callers pass extra values such as reason, acceptsDeployId, host, secretChanged, and port. Include each audit-relevant value in its message, or use a logging path that persists additional 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));
}
}
58 changes: 58 additions & 0 deletions lib/AuditLog/Operation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\AppAPI\AuditLog;

/**
* Operations AppAPI writes to the admin audit log, in the form `app.entity.action`
*
* The values are matched by log consumers (filters, alert rules, SIEM imports),
* 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';
case ExAppAuthFailed = 'app_api.ex_app_auth.failed';

// Deploy daemons
case DaemonRegistered = 'app_api.daemon.registered';
case DaemonUpdated = 'app_api.daemon.updated';
case DaemonUnregistered = 'app_api.daemon.unregistered';
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
*/
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::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',
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',
};
}
}
15 changes: 13 additions & 2 deletions lib/Command/ExApp/Unregister.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand All @@ -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));
Expand Down Expand Up @@ -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));
}
}
}
}
Expand Down
14 changes: 13 additions & 1 deletion lib/Controller/ConfigController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Controller/DaemonConfigController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
10 changes: 8 additions & 2 deletions lib/Controller/ExAppsPageController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
}
Expand Down
37 changes: 33 additions & 4 deletions lib/Service/AppAPIService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -287,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];
Expand Down Expand Up @@ -325,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)
Expand Down Expand Up @@ -704,6 +722,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;
}

Expand Down Expand Up @@ -754,7 +775,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);
Expand Down Expand Up @@ -803,10 +829,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());
Expand Down
Loading
Loading