You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Nextcloud 36 asks every app to write its administrative and security relevant actions to the admin audit log, each with a stable operation id. AppAPI wrote nothing: installing, updating or removing ExApps, changing deploy daemons or AppAPI settings, and failed ExApp authentication left no trace in audit.log.
Changes
ExApps (app_api.ex_app.*): registered, updated, enabled, disabled, unregistered and data deleted, written where the change is stored, so occ, the web interface and the OCS API are each covered once. Installs, updates and the test deploy started in the web interface run occ in a separate process without a user, so the web request also writes install_requested, update_requested or uninstall_requested as the administrator, with the names of the environment variables and the mounts.
Deploy daemons (app_api.daemon.*, app_api.daemon_registry.*): registered, updated with the names of the changed settings, unregistered, registry mapping added and removed. A change of the default daemon is an app_api.admin_config.updated entry.
AppAPI settings (app_api.admin_config.updated): one entry per changed key, the value only for the known AppAPI settings.
Failed authentication: AppAPI requests for an unknown ExApp or with a wrong secret, HaRP requests with a missing or wrong shared key (now throttled like the other AppAPI checks), and Talk bot messages with a wrong signature.
The entries never contain secrets. Values sent by a client or an ExApp stay out of the messages, and {placeholders} in all other values are escaped, as the logger interpolates them in the message.
With --rm-data, a Kubernetes ExApp whose deployment is already gone now has its volume removed too.
nextcloud/ocp is bumped for the operation parameter; an unused method that still referenced the removed OCP\IServerContainer is dropped.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
The change adds audit operation identifiers and an AuditLogger. It records selected configuration, daemon, ExApp lifecycle, authentication, and data-deletion events, with tests for event contents and logging conditions. Kubernetes removal now sends a data-removal request when the deployment is absent and data removal is requested. The change also removes ExApp TaskProcessing provider registration.
Merge Risk: Low · up to 219f5
The audit logging changes are mostly sound. Before merging, fix the registry-removal endpoint so a non-matching mapping returns a JSON error instead of a 500. Two smaller issues, unregistration audit ordering and enable rollback, are bounded and can follow up.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title "feat: expand audit logging" matches the main change. The PR adds audit log entries for AppAPI ExApp lifecycle actions, deploy daemon and registry changes, AppAPI settings, and failed authen…
Description check
The description explains the purpose (Nextcloud 36 audit log requirement) and lists the changes: ExApp lifecycle events, deploy daemon and registry changes, AppAPI settings, failed authentication, sec…
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.
Full details: Title check
Explanation
The title "feat: expand audit logging" matches the main change. The PR adds audit log entries for AppAPI ExApp lifecycle actions, deploy daemon and registry changes, AppAPI settings, and failed authentication. The title is short and specific enough for a reader scanning history. It omits the specific scope (AppAPI, admin audit log), but that is an acceptable level of detail.
Full details: Description check
Explanation
The description explains the purpose (Nextcloud 36 audit log requirement) and lists the changes: ExApp lifecycle events, deploy daemon and registry changes, AppAPI settings, failed authentication, secret handling, the Kubernetes volume removal fix, and the nextcloud/ocp bump. It matches the changeset closely.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
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 reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: b6037887-7eb4-4096-8f3e-1432c885e56c
📥 Commits
Reviewing files that changed from the base of the PR and between 853c618 and c02350b.
📒 Files selected for processing: 3
lib/AuditLog/AuditLogger.php
tests/php/AuditLog/AuditEvents.php
tests/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 reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
Minor · Emit ExAppUnregistered after cleanup succeeds. · ExAppService.php:141
lib/Service/ExAppService.php:141 Minor · Data Integrity & Integration · Quick win
Emit ExAppUnregistered after cleanup succeeds.
The audit entry is written after the ExApp row is deleted but before removeExAppRoutes() and the cache removal run. Two failure cases follow:
If route removal fails, the audit log still records a successful unregistration, but the method returns false.
If an audit listener throws, the exception can leave unregisterExApp() before route removal and cache invalidation. The ExApp row is already deleted at that point.
Move the audit call after the cleanup steps and gate it on the final result. A throwing listener then no longer skips route removal or cache invalidation. The exception still reaches the caller after cleanup finishes. A failed route removal produces no success entry; the existing error log records that case.
Suggested fix
$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) {
$this->logger->error(sprintf('Error while unregistering %s ExApp routes from the database.', $appId));
}
$this->cache?->remove('/ex_apps');
- return $r === 1 && $rmRoutes !== null;+ $unregistered = $r === 1 && $rmRoutes !== null;+ if ($unregistered) {+ $this->auditLogger->log(Operation::ExAppUnregistered, 'ExApp %s was unregistered', [+ 'appId' => $appId,+ ]);+ }+ return $unregistered;
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: dc69a60e-646f-4c2f-a524-2e4ccdf606ee
Commits
Reviewing files that changed from the base of the PR and between c02350b and d856a75.
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 reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: c1e77785-2bfb-4f19-b951-fb56a3276b66
Commits
Reviewing files that changed from the base of the PR and between d856a75 and bfcb389.
Files selected for processing: 3
lib/Service/DaemonConfigService.php
lib/Service/ExAppService.php
tests/php/Service/DaemonConfigServiceTest.php
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
Minor · Return a controller error when registry removal fails. · DaemonConfigService.php:370-371
lib/Service/DaemonConfigService.php:370-371 Minor · Stability & Availability · Quick win
Return a controller error when registry removal fails.
removeDockerRegistry() returns an error array when it removes no mapping. removeDaemonDockerRegistry() handles only null, so it calls jsonSerialize() on that array. PHP throws an Error, and the request fails with a 500 instead of a JSON error response.
A JSON body with boolean from and to values can reach this path. The loose in_array check accepts it when the stored mapping has non-empty string endpoints. The strict filter then removes nothing. Before this change, the same request succeeded as a no-op. A mapping that does not exist already triggers the same crash.
Suggested fix
- $daemonConfig = $this->daemonConfigService->removeDockerRegistry($daemonConfig, $registryMap);- if ($daemonConfig === null) {+ $result = $this->daemonConfigService->removeDockerRegistry($daemonConfig, $registryMap);+ if (is_array($result)) {+ return new JSONResponse($result, Http::STATUS_NOT_FOUND);+ }+ if ($result === null) {
return new JSONResponse(['error' => $this->l10n->t('Error removing Docker registry')], Http::STATUS_INTERNAL_SERVER_ERROR);
}
- return new JSONResponse($daemonConfig->jsonSerialize(), Http::STATUS_OK);+ return new JSONResponse($result->jsonSerialize(), Http::STATUS_OK);
Add a controller test that sends ['from' => true, 'to' => true] against a stored string mapping and expects a 404 response. No controller test covers this method.
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 1c5b55c1-0955-4fe8-a6de-ed7aef6aa7d8
Commits
Reviewing files that changed from the base of the PR and between bfcb389 and 219f511.
Files ignored due to path filters: 1
composer.lock is excluded by !**/*.lock
Files selected for processing: 3
CHANGELOG.md
lib/Service/DaemonConfigService.php
tests/php/Service/DaemonConfigServiceTest.php
Files skipped from review as they are similar to previous changes: 1
CHANGELOG.md
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Nextcloud 36 asks every app to write its administrative and security relevant actions to the admin audit log, each with a stable operation id. AppAPI wrote nothing: installing, updating or removing ExApps, changing deploy daemons or AppAPI settings, and failed ExApp authentication left no trace in
audit.log.Changes
app_api.ex_app.*): registered, updated, enabled, disabled, unregistered and data deleted, written where the change is stored, so occ, the web interface and the OCS API are each covered once. Installs, updates and the test deploy started in the web interface run occ in a separate process without a user, so the web request also writesinstall_requested,update_requestedoruninstall_requestedas the administrator, with the names of the environment variables and the mounts.app_api.daemon.*,app_api.daemon_registry.*): registered, updated with the names of the changed settings, unregistered, registry mapping added and removed. A change of the default daemon is anapp_api.admin_config.updatedentry.app_api.admin_config.updated): one entry per changed key, the value only for the known AppAPI settings.{placeholders}in all other values are escaped, as the logger interpolates them in the message.--rm-data, a Kubernetes ExApp whose deployment is already gone now has its volume removed too.nextcloud/ocpis bumped for the operation parameter; an unused method that still referenced the removedOCP\IServerContaineris dropped.The operations are listed in
lib/AuditLog/Operation.phpand documented in nextcloud/documentation#16039.🤖 AI (if applicable)