Skip to content

feat(tenant): adopt OR Organisation as tenant identity (closes #405) #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rubenvdlinde wants to merge 1 commit into
base: development
Choose a base branch
from
Open

feat(tenant): adopt OR Organisation as tenant identity (closes #405) #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions lib/Middleware/TenantMiddleware.php
View file
Open in desktop
Copy link
Copy Markdown

@WilcoLouwerse WilcoLouwerse May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] Generic \Exception(code=403) used as control flow — afterException over-matches

The status block throws new \Exception('Organisation is '.$status, 403). The afterException handler intercepts any \Exception with code 403, including exceptions from unrelated code paths. This rewrites unrelated 403 exceptions as organisation-status responses, breaking Nextcloud's exception handling chain. Fix: define and throw a dedicated TenantNotActiveException class that afterException checks with instanceof, not by code equality.

Copy link
Copy Markdown

@WilcoLouwerse WilcoLouwerse May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] Empty-string _tenantId set when neither uuid nor id key present — potential cross-tenant IDOR

$tenantUuid = ($tenant['uuid'] ?? $tenant['id'] ?? '') defaults to empty string '' when the serialized Organisation array contains neither key. The empty string is then stored as _tenantId. Controllers trusting _tenantId to scope queries will receive empty string and may return all objects (depending on their null/empty guard) or match objects whose stored tenantId is also empty. Fix: validate $tenantUuid is non-empty after extraction; if empty, treat as missing-tenant condition.

Copy link
Copy Markdown

@WilcoLouwerse WilcoLouwerse May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] Null/empty status silently bypasses lifecycle check — provisioning orgs pass through

Guard if ($status !== null && $status !== '' && $status !== 'active') only blocks non-empty, non-active values. An org with status: null or absent status field (e.g., a newly-created provisioning org) passes the check. Per spec, status: 'provisioning' MUST be blocked. Fix: change to an allowlist: if ($status !== 'active') — only explicitly allow active tenants through, treating null/missing as non-active.

Copy link
Copy Markdown

@WilcoLouwerse WilcoLouwerse May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] Implementation diverges from spec: TenantLifecycleService::isActive() is never called

Both spec.md and design.md require calling $tenantLifecycleService->isActive($organisationUuid). The actual implementation reads $tenant['status'] from the serialized array — TenantLifecycleService is never injected into TenantMiddleware and never called. This violates the normative MUST in the spec, means tasks MW-1.1 and MW-1.2 are not fulfilled, and bypasses any side-effects isActive() may enforce (e.g., quota re-evaluation, audit logging). Fix: inject TenantLifecycleService as a constructor argument and call isActive() as specified.

Copy link
Copy Markdown

@WilcoLouwerse WilcoLouwerse May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CONCERN] TenantMiddleware.php SPDX header status unknown — diff starts mid-file

The diff for TenantMiddleware.php starts at line 98 (inside beforeController). The file header is not shown, so it is unclear whether the file has SPDX-License-Identifier and SPDX-FileCopyrightText headers. Gate 1 requires all lib/ PHP files to carry SPDX headers. Verify and add if missing.

Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public function beforeController($controller, $methodName): void
return;
}

// Resolve tenant for the current user.
// Resolve tenant for the current user (delegates to OR Organisation).
$tenant = $this->tenantService->getTenantForUser($userId);
if ($tenant === null) {
$this->logger->warning(
Expand All @@ -109,8 +109,20 @@ public function beforeController($controller, $methodName): void
return;
}

// Per consume-or-tenant-fleet-wide: lifecycle status enforcement lives
// in OR's tenant-lifecycle. Block requests scoped to a non-active tenant.
$tenantUuid = ($tenant['uuid'] ?? $tenant['id'] ?? '');
$status = ($tenant['status'] ?? null);
if ($status !== null && $status !== '' && $status !== 'active') {
$this->logger->info(
'Procest: Request blocked because tenant is not active',
['userId' => $userId, 'tenantUuid' => $tenantUuid, 'status' => $status]
);
throw new \Exception('Organisation is '.$status, 403);
}

// Store tenant context for controllers to use.
$this->request->setParameter('_tenantId', $tenant['uuid'] ?? $tenant['id'] ?? '');
$this->request->setParameter('_tenantId', $tenantUuid);
$this->request->setParameter('_tenantRegisterId', $tenant['registerId'] ?? '');
$this->request->setParameter('_tenantSlug', $tenant['slug'] ?? '');
}//end beforeController()
Expand All @@ -135,6 +147,16 @@ public function afterException($controller, $methodName, \Exception $exception):
);
}

if ($exception->getCode() === 403) {
// Surface OR-Organisation status block to the caller.
$message = $exception->getMessage();
$status = (preg_match('/Organisation is (\\w+)/', $message, $m) === 1) ? $m[1] : 'inactive';
return new JSONResponse(
['success' => false, 'error' => $message, 'status' => $status],
403
);
}

throw $exception;
}//end afterException()
}//end class
Loading
Loading