Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.

Commit 91a8e5d

Browse files
committed
fix(security): AggregationRunner — RBAC + multitenancy on native PG path
The Postgres-native fast path built raw SQL against `oc_<table>` with only `_deleted IS NULL` in the WHERE. `loadSchema/loadRegister` passed `_multitenancy: false`, the controller is `#[NoAdminRequired]`, and the PHP fallback's RBAC was unreachable for the common metric set (count/sum/avg/min/max). Net effect: any authed user could compute aggregates over any schema in any tenant. Changes: - Inject PermissionHandler + IUserSession + OrganisationService. - Gate run() on `hasPermission(schema, 'list', userId)` — fail-closed. - Drop `_multitenancy: false` on loadSchema / loadRegister so the slug lookup itself is tenant-scoped. - Add `"organisation" = ?` predicate to tryNativeAggregation's WHERE, mirroring MagicRbacHandler. Active org `null` ⇒ no rows. - Include userId + active-org UUID in the AggregationCache key so two callers with different RBAC verdicts can't read each other's cached results (also addresses concern r3187494499). Refs: #1419 review (blocker 2 + concern 8) — discussion_r3187494411, discussion_r3187494499
1 parent 1f71fcd commit 91a8e5d

1 file changed

Lines changed: 46 additions & 6 deletions

File tree

lib/Service/Aggregation/AggregationRunner.php

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,12 @@
3434
use OCA\OpenRegister\Db\Schema;
3535
use OCA\OpenRegister\Db\SchemaMapper;
3636
use OCA\OpenRegister\Service\Index\SearchBackendInterface;
37+
use OCA\OpenRegister\Service\Object\PermissionHandler;
38+
use OCA\OpenRegister\Service\OrganisationService;
3739
use OCA\OpenRegister\Service\Search\PlaceholderResolver;
38-
use ReflectionClass;
3940
use OCP\IDBConnection;
41+
use OCP\IUserSession;
42+
use ReflectionClass;
4043
use RuntimeException;
4144

4245
/**
@@ -67,6 +70,9 @@ public function __construct(
6770
private readonly PlaceholderResolver $placeholders,
6871
private readonly IDBConnection $db,
6972
private readonly AggregationCache $cache,
73+
private readonly PermissionHandler $permissionHandler,
74+
private readonly IUserSession $userSession,
75+
private readonly OrganisationService $organisationService,
7076
private readonly ?SearchBackendInterface $searchBackend=null
7177
) {
7278
}//end __construct()
@@ -95,8 +101,23 @@ public function __construct(
95101
*/
96102
public function run(string $registerRef, string $schemaRef, string $name): array
97103
{
98-
$schema = $this->loadSchema(schemaRef: $schemaRef);
99-
$register = $this->loadRegister(registerRef: $registerRef);
104+
$schema = $this->loadSchema(schemaRef: $schemaRef);
105+
$register = $this->loadRegister(registerRef: $registerRef);
106+
107+
// SECURITY: gate aggregation behind list-permission on the schema
108+
// before any native or fallback path executes. Without this gate,
109+
// the native PG path would compute COUNT/SUM/AVG over rows the
110+
// caller has no read permission for (cross-tenant + cross-RBAC
111+
// statistics leak).
112+
$userId = $this->userSession->getUser()?->getUID();
113+
if ($this->permissionHandler->hasPermission(
114+
schema: $schema,
115+
action: 'list',
116+
userId: $userId
117+
) === false) {
118+
throw new RuntimeException('Forbidden: caller lacks list permission on the requested schema.');
119+
}
120+
100121
$annotation = $this->getAnnotation(schema: $schema);
101122
if ($annotation === null) {
102123
throw new RuntimeException(
@@ -118,11 +139,17 @@ public function run(string $registerRef, string $schemaRef, string $name): array
118139

119140
// Cache lookup: the resolved filter (with placeholders concrete)
120141
// is the cache key together with the user's RBAC scope. 60s TTL.
121-
$cacheKey = [
142+
// SECURITY: cache key MUST include userId + active organisation —
143+
// two callers with different RBAC verdicts on the same (metric,
144+
// field, filter) tuple would otherwise read each other's results.
145+
$activeOrg = $this->organisationService->getActiveOrganisation();
146+
$cacheKey = [
122147
'metric' => $metric,
123148
'field' => $field,
124149
'filter' => $resolvedFilter,
125150
'groupBy' => $groupBy,
151+
'userId' => $userId,
152+
'org' => $activeOrg?->getUuid(),
126153
];
127154
$cached = $this->cache->get(
128155
registerSlug: (string) $register->getSlug(),
@@ -528,6 +555,15 @@ private function tryNativeAggregation(
528555

529556
$whereParts = ["(_deleted IS NULL OR _deleted = 'null'::jsonb)"];
530557
$bindings = [];
558+
559+
// SECURITY: mirror MagicRbacHandler's multi-tenancy predicate. The
560+
// native fast path bypasses MagicMapper entirely, so without this
561+
// filter any authed caller could compute aggregates over rows in
562+
// other tenants. Active org of `null` ⇒ no rows (fail-closed).
563+
$activeOrg = $this->organisationService->getActiveOrganisation();
564+
$whereParts[] = '"organisation" = ?';
565+
$bindings[] = $activeOrg?->getUuid() ?? '__no_active_org__';
566+
531567
foreach ($filter as $f => $v) {
532568
$col = $this->sanitizeColumnName(name: (string) $f);
533569
if (is_array($v) === false) {
@@ -688,7 +724,10 @@ private function sanitizeColumnName(string $name): string
688724
private function loadSchema(string $schemaRef): Schema
689725
{
690726
try {
691-
return $this->schemaMapper->find($schemaRef, _multitenancy: false);
727+
// SECURITY: keep multitenancy filter on so a tenant user cannot
728+
// resolve schemas owned by another tenant simply by knowing
729+
// the slug.
730+
return $this->schemaMapper->find($schemaRef);
692731
} catch (\Throwable $e) {
693732
throw new RuntimeException(sprintf('Schema "%s" not found.', $schemaRef), 0, $e);
694733
}
@@ -706,7 +745,8 @@ private function loadSchema(string $schemaRef): Schema
706745
private function loadRegister(string $registerRef): \OCA\OpenRegister\Db\Register
707746
{
708747
try {
709-
return $this->registerMapper->find($registerRef, _multitenancy: false);
748+
// SECURITY: keep multitenancy filter on (see loadSchema).
749+
return $this->registerMapper->find($registerRef);
710750
} catch (\Throwable $e) {
711751
throw new RuntimeException(sprintf('Register "%s" not found.', $registerRef), 0, $e);
712752
}

0 commit comments

Comments
 (0)