Repository navigation
Feat/admin audit logging - #1130
GhassenKefi wants to merge 4 commits into
Conversation
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 <ghassen.kefi.dev@gmail.com>
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 <ghassen.kefi.dev@gmail.com>
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 <ghassen.kefi.dev@gmail.com>
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 <ghassen.kefi.dev@gmail.com>
📝 Walkthrough
Priority: ➖ Normal Pre-merge checks:
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description only identifies AI involvement and does not describe the admin audit logging changes. | Add a concise summary of the audit events, affected AppAPI components, success conditions, and secret-handling behavior. |
| Docstring Coverage | |
Docstring coverage is 13.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 104 functions across 19 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main change: adding admin audit logging. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 PHPStan (2.2.16)
-
PHPStan reported 10 non-file analysis error(s); analysis is incomplete
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
cff23f7f-ef6d-4d59-a3d8-bd265278c6f2
📒 Files selected for processing: 19
lib/AuditLog/AuditLogger.phplib/AuditLog/Operation.phplib/Command/ExApp/Unregister.phplib/Controller/ConfigController.phplib/Controller/DaemonConfigController.phplib/Controller/ExAppsPageController.phplib/Service/AppAPIService.phplib/Service/DaemonConfigService.phplib/Service/ExAppService.phptests/php/AuditLog/AuditLoggerTest.phptests/php/AuditLog/OperationTest.phptests/php/Command/ExApp/UnregisterTest.phptests/php/Controller/ConfigControllerTest.phptests/php/Controller/DaemonConfigControllerTest.phptests/php/Controller/ExAppsPageControllerTest.phptests/php/Service/AppAPIServiceTest.phptests/php/Service/DaemonConfigServiceTest.phptests/php/Service/ExAppServiceAuditLogTest.phptests/php/Service/ExAppServiceGetAppInfoTest.php
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
| * @param array<string, mixed> $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 { | ||
| $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent($message, $parameters, false, $operation->value)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n '"nextcloud/ocp"' composer.json
rg -n -A3 '"name": "nextcloud/ocp"' composer.lock | head -20
rg -n 'nextcloud.*min-version|max-version' appinfo/info.xml
fd CriticalActionPerformedEvent.php vendor 2>/dev/null --exec sed -n '1,80p' {}Repository: nextcloud/app_api
Length of output: 388
🏁 Script executed:
set -u
printf '%s\n' '--- changed file ---'
git show 853c6184a47787978070c6fcc10d38e63b6f05c6:lib/AuditLog/AuditLogger.php
printf '%s\n' '--- dependency and version declarations ---'
sed -n '1,90p' composer.json
sed -n '150,195p' composer.lock
sed -n '50,68p' appinfo/info.xml
printf '%s\n' '--- event references and local stubs/config ---'
rg -n -F --glob '!vendor/**' --glob '!node_modules/**' -e 'CriticalActionPerformedEvent' -e 'php-psalm-analysis' -e 'TooManyArguments' -e 'nextcloud/ocp' .
printf '%s\n' '--- PR diff for directly relevant files ---'
git diff --stat 38a8a35fa20c6d060b477920ed9c36590e0367b5 853c6184a47787978070c6fcc10d38e63b6f05c6 -- lib/AuditLog/AuditLogger.php composer.json composer.lock appinfo/info.xml
git diff --unified=20 38a8a35fa20c6d060b477920ed9c36590e0367b5 853c6184a47787978070c6fcc10d38e63b6f05c6 -- lib/AuditLog/AuditLogger.php composer.json composer.lock appinfo/info.xmlRepository: nextcloud/app_api
Length of output: 8544
🏁 Script executed:
set -u
printf '%s\n' '--- AuditLoggerTest ---'
nl -ba tests/php/AuditLog/AuditLoggerTest.php
printf '%s\n' '--- TooManyArguments baseline context ---'
nl -ba tests/psalm-baseline.xml | sed -n '38,58p'
printf '%s\n' '--- locked OCP event source ---'
url='https://raw.githubusercontent.com/nextcloud-deps/ocp/ddc90a88f4ec0166a1c0384cf6d0370e3dc53898/OCP/Log/Audit/CriticalActionPerformedEvent.php'
curl -L --fail --silent --show-error "$url" | nl -baRepository: nextcloud/app_api
Length of output: 4304
Use a constructor supported by the declared OCP API.
nextcloud/ocp is locked to a CriticalActionPerformedEvent constructor with three parameters. This call passes four parameters, which produces a TooManyArguments error in static analysis. AuditLoggerTest also calls getOperation(), but the locked event class does not define that method.
If Nextcloud 36 remains supported, use the three-parameter constructor and preserve the operation in the event parameters. If the four-parameter API is required, update nextcloud/ocp and raise the minimum Nextcloud version together.
🧰 Tools
🪛 GitHub Actions: Lint / 4_php-psalm-analysis (8.3).txt
[error] 29-29: Psalm TooManyArguments: Too many arguments for OCP\Log\Audit\CriticalActionPerformedEvent::__construct; expected 3 but received 4. The composer run psalm step failed with exit code 2.
🪛 GitHub Actions: Lint / php-psalm-analysis (8.3)
[error] 29-29: Psalm (composer run psalm) reported TooManyArguments: OCP\Log\Audit\CriticalActionPerformedEvent::__construct expects 3 arguments but received 4.
🪛 GitHub Check: php-psalm-analysis (8.3)
[failure] 29-29: TooManyArguments
lib/AuditLog/AuditLogger.php:29:41: TooManyArguments: Too many arguments for OCP\Log\Audit\CriticalActionPerformedEvent::__construct - expecting 3 but saw 4 (see https://psalm.dev/026)
🪛 PHPStan (2.2.16)
[error] 29-29: Class OCP\Log\Audit\CriticalActionPerformedEvent constructor invoked with 4 parameters, 1-3 required.
(arguments.count)
Sources: Linters/SAST tools, Pipeline failures
This branch has not been deployed
1 participant
🤖 AI (if applicable)