Repository navigation
Feat/admin audit logging - #1130
GhassenKefi wants to merge 4 commits into
Conversation
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 105 functions across 20 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Description check |  |
The description only states that AI contributed to the PR. It does not describe the administrative audit logging changes or their tests. | Add a concise summary of the audit logging implementation, affected services and controllers, and the tests included. |
-
Check name Status Explanation Title check The title clearly identifies the main change: adding administrative 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
- 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 1 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
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>
853c618 to
c02350b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b6037887-7eb4-4096-8f3e-1432c885e56c
📒 Files selected for processing: 3
lib/AuditLog/AuditLogger.phptests/php/AuditLog/AuditEvents.phptests/php/AuditLog/AuditLoggerTest.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.
| * 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 |
There was a problem hiding this comment.
🗄️ 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/ServiceRepository: 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.phpRepository: 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.phpRepository: 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.
| use OCP\Log\Audit\CriticalActionPerformedEvent; | ||
|
|
||
| final class AuditEvents { | ||
| /** | ||
| * Servers before the operation was added to CriticalActionPerformedEvent do not keep it | ||
| */ | ||
| public static function hasOperation(CriticalActionPerformedEvent $event, string $operation): bool { | ||
| return !method_exists($event, 'getOperation') || $event->getOperation() === $operation; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,90p' tests/php/AuditLog/AuditEvents.php
sed -n '1,100p' tests/php/AuditLog/AuditLoggerTest.php
rg -n 'hasOperation|nextcloud/ocp|getOperation' tests/php composer.jsonRepository: nextcloud/app_api
Length of output: 2455
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- PR diff ---'
git diff --find-renames 38a8a35fa20c6d060b477920ed9c36590e0367b5 c02350b13fa13e1679ca63c11a42a7a8ceac2b07 -- tests/php/AuditLog composer.json .github 2>/dev/null || true
printf '%s\n' '--- all relevant references ---'
rg -n -F --glob '*.php' -e 'AuditEvents::hasOperation' -e 'CriticalActionPerformedEvent' -e 'getOperation' tests lib || test "$?" -eq 1
printf '%s\n' '--- logger and operation definitions ---'
rg -n -F --glob '*.php' -e 'class AuditLogger' -e 'function log' -e 'enum Operation' -e 'class Operation' lib tests || test "$?" -eq 1
printf '%s\n' '--- dependency and supported-version configuration ---'
rg -n -i --glob '!vendor/**' -e 'nextcloud/ocp|ocp|supported|compatib|server-version|matrix|minimum.*nextcloud|nextcloud.*version' composer.json composer.lock .github tests phpunit.xml* 2>/dev/null || true
printf '%s\n' '--- local OCP event declaration if present ---'
rg -n -F --glob '*.php' 'class CriticalActionPerformedEvent' . vendor/nextcloud/ocp 2>/dev/null || trueRepository: nextcloud/app_api
Length of output: 20562
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- AuditLogger and event declarations ---'
nl -ba lib/AuditLog/AuditLogger.php
nl -ba vendor/nextcloud/ocp/OCP/Log/Audit/CriticalActionPerformedEvent.php 2>/dev/null || true
printf '%s\n' '--- package and app compatibility metadata ---'
nl -ba composer.json | sed -n '1,90p'
nl -ba composer.lock | sed -n '150,225p'
rg -n -i --glob '!vendor/**' --glob '!composer.lock' -e 'nextcloud min version|nextcloud.*version|version.*nextcloud|min-version|minVersion|dependencies|server' appinfo .github phpunit.xml* tests 2>/dev/null || true
printf '%s\n' '--- PHPUnit workflow and test bootstrap/config ---'
nl -ba .github/workflows/phpunit.yml
find . -maxdepth 3 -type f \( -name 'phpunit.xml' -o -name 'phpunit.xml.dist' -o -name 'info.xml' \) -printRepository: nextcloud/app_api
Length of output: 30871
Exercise the operation assertion on an OCP version that supports it.
AuditLoggerTest checks the operation only through AuditEvents::hasOperation(). In the current supported test setup, CriticalActionPerformedEvent has no getOperation(), so AuditLogger omits the operation and hasOperation() returns true for any expected value. The test can therefore pass when no operation is emitted. Add a test configuration with getOperation() support and make the operation assertion fail, rather than pass, when that method is unavailable.
This branch has not been deployed
1 participant
🤖 AI (if applicable)