Skip to content

Commit 17facc5

Browse files
committed
fix(security): AuditTrailController — admin-only on clearAll + export
`clearAll()` and `export()` carried `@NoAdminRequired`. Two paths: - `clearAll`: any authenticated caller could wipe every row in `oc_openregister_audit_trails`, breaking the AVG/GDPR Art 30 audit chain that operators rely on for supervisor review. - `export`: any authenticated caller could dump every audit row in bulk (CSV/etc) without an organisation filter — cross-tenant recon with one request. Add a `requireAdmin()` body-level gate (returns 401/403 JSONResponse or null). Keep `@NoAdminRequired` at the framework level so existing non-admin UI flows that touch other endpoints (`index`, `show`, `objects`, hash-chain verification) still load — those don't trip this gate. `index` cross-tenant scoping is a deeper RBAC question (audit rows have an `organisation` column but the existing LogService doesn't filter by it); left for follow-up. Refs: #1419 review (off-diff blocker — top-level comment 4378205122)
1 parent ec84a5d commit 17facc5

1 file changed

Lines changed: 43 additions & 1 deletion

File tree

lib/Controller/AuditTrailController.php

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,45 @@ public function __construct(
6464
IRequest $request,
6565
private readonly LogService $logService,
6666
private readonly AuditTrailMapper $auditTrailMapper,
67-
private readonly AuditHashService $auditHashService
67+
private readonly AuditHashService $auditHashService,
68+
private readonly \OCP\IUserSession $userSession,
69+
private readonly \OCP\IGroupManager $groupManager
6870
) {
6971
parent::__construct(appName: $appName, request: $request);
7072
}//end __construct()
7173

74+
/**
75+
* Gate destructive / bulk-export audit operations on admin membership.
76+
*
77+
* SECURITY: `clearAll` wipes the entire audit table — a chain of
78+
* trust for AVG/GDPR Art 30 reviews. `export` dumps every row in
79+
* bulk and is an obvious recon path across tenants. Both surfaces
80+
* stay `@NoAdminRequired` at the framework level so the existing
81+
* UI keeps loading, but reject non-admin callers in the body.
82+
*
83+
* @return JSONResponse|null 401/403 response when blocked, null when allowed.
84+
*/
85+
private function requireAdmin(): ?JSONResponse
86+
{
87+
$user = $this->userSession->getUser();
88+
if ($user === null) {
89+
return new JSONResponse(
90+
data: ['error' => 'Authentication required'],
91+
statusCode: 401
92+
);
93+
}
94+
95+
if ($this->groupManager->isAdmin($user->getUID()) === false) {
96+
return new JSONResponse(
97+
data: ['error' => 'Forbidden: this audit-trail operation is admin-only'],
98+
statusCode: 403
99+
);
100+
}
101+
102+
return null;
103+
104+
}//end requireAdmin()
105+
72106
/**
73107
* Extract pagination, filter, and search parameters from request
74108
*
@@ -345,6 +379,10 @@ public function objects(string $register, string $schema, string $id): JSONRespo
345379
*/
346380
public function export(): JSONResponse
347381
{
382+
if (($denial = $this->requireAdmin()) !== null) {
383+
return $denial;
384+
}
385+
348386
// Extract request parameters.
349387
$params = $this->extractRequestParameters();
350388

@@ -453,6 +491,10 @@ public function destroyMultiple(): JSONResponse
453491
*/
454492
public function clearAll(): JSONResponse
455493
{
494+
if (($denial = $this->requireAdmin()) !== null) {
495+
return $denial;
496+
}
497+
456498
try {
457499
// Use the clearAllLogs method from the mapper.
458500
$result = $this->auditTrailMapper->clearAllLogs();

0 commit comments

Comments
 (0)