feat(tenant): adopt OR Organisation as tenant identity (closes #405) by rubenvdlinde · Pull Request #411 · ConductionNL/procest

rubenvdlinde · 2026-05-11T21:51:32Z

Implements migrate-tenant-to-or-tenant — the procest subset of consume-or-tenant-fleet-wide.

Investigation finding

The spec assumed procest had a parallel tenant schema. It doesn't. No schema in procest_register.json, no tenant_schema config key set, no tenant objects ever written. The old code path short-circuited to null in practice. Procest's tenant identity has always been NC groups with tenant_ prefix.

What

TenantService rewritten to consume OCA\OpenRegister\Db\OrganisationMapper + OCA\OpenRegister\Service\TenantLifecycleService. Public method signatures preserved so existing callers (TenantController, TenantMiddleware) are unchanged.
TenantMiddleware now reads the resolved tenant's status and blocks non-active organisations with HTTP 403 + {success: false, error: 'Organisation is <state>', status: '<state>'}. afterException() surfaces the envelope.
All OR dependencies resolve gracefully — null if openregister absent.

Not done (no-op for this codebase)

OCC migration command — no source data.
Schema deprecation — no schema exists.
Row-migration integration tests — nothing to migrate.

Scope inline in tasks.md.

Closes #405.

Implements migrate-tenant-to-or-tenant — the procest subset of consume-or-tenant-fleet-wide. Investigation: procest's tenant concept is vestigial. No tenant schema in procest_register.json, tenant_schema config key never set, no tenant objects ever written. Functional tenant identity is just NC groups with tenant_ prefix. This is therefore an adoption, not a migration. - lib/Service/TenantService.php — rewritten to consume OR's OrganisationMapper + TenantLifecycleService. Public method signatures preserved. - lib/Middleware/TenantMiddleware.php — blocks non-active organisations with HTTP 403 + body { status: <state> }; afterException() surfaces the envelope. No OCC migration (no source data), no schema deprecation (no schema), no row migration tests (no rows). Scope adjustment documented in tasks.md. Closes #405

github-actions · 2026-05-11T21:56:27Z

Quality Report — ConductionNL/procest @ `3ffed81`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	❌
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		❌
composer			✅	✅ 100/100
npm			✅	✅ 419/419
PHPUnit					⏭️
Newman					⏭️
Playwright					❌

Spec coverage: 5% (21 tests / 456 specs)

Quality workflow — 2026-05-11 21:56 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse · 2026-05-12T15:36:19Z

[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.

WilcoLouwerse · 2026-05-12T15:36:21Z

[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.

WilcoLouwerse · 2026-05-12T15:36:22Z

[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.

WilcoLouwerse · 2026-05-12T15:36:24Z

[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.

WilcoLouwerse · 2026-05-12T15:36:25Z

+            return null;
+        }
+    }//end getTenantStatus()
+


[BLOCKER] provisionTenant: $org->getOwner() may not exist on OR entity — silent escalation to admin

$mapper->findByUuid($tenantId) returns an OR Organisation entity. The code calls $org->getOwner() which may not exist on that entity. If it doesn't, the call is caught by the wrapping try/catch and returns ['error' => '...']. More critically, $org->getOwner() ?? '' passed to $this->groupManager->isAdmin() means an org without an owner field will always fall back to a default that may silently escalate provisioning to the admin identity. Fix: verify getOwner() exists on OR's entity; if not, remove this check or use a safe explicit default.

WilcoLouwerse · 2026-05-12T15:36:27Z

+                $userCount += count($group->getUsers());
+            }
        }



[CONCERN] getTenantByGroupId: linear O(n) scan with findAll(limit:500) may miss tenants in large deployments

The hard limit of 500 means any OR instance with >500 organisations will silently fail to find tenants beyond that count, causing auth failures. Fix: query OR directly by groupId property if the mapper supports it, or log a warning when result count equals the limit.

WilcoLouwerse · 2026-05-12T15:36:28Z

                ['tenantId' => $tenantId, 'exception' => $e->getMessage()]
            );
-            return ['error' => 'Failed to create tenant register: '.$e->getMessage()];
+            return ['error' => 'Failed to provision tenant: '.$e->getMessage()];


[CONCERN] getTenantForUser returns first org without validating user membership — potential IDOR

findOrganisationsByUserId($userId) returns $orgs[0]->jsonSerialize() — the first org — without secondary membership validation. If OR's findByUserId returns orgs by indirect association (contact, not just member), the first result may not be the correct tenant for access scoping. Fix: add an assertion or comment documenting the OR contract, or add a cross-check against NC groups.

WilcoLouwerse · 2026-05-12T15:36:29Z

[CONCERN] findOrganisationsByUserId silently swallows all Throwables — no logging on OR errors

The catch (Throwable $e) block returns [] with no log entry, triggering NC-group fallback. A real OR database error will silently fall back to old group-based lookup, creating split-brain scenarios. Fix: log the exception at warning level before returning [].

WilcoLouwerse · 2026-05-12T15:36:31Z

[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.

WilcoLouwerse · 2026-05-12T15:36:32Z

@@ -0,0 +1,193 @@
+# procest-tenant-migration Specification


[CONCERN] spec.md uses YAML frontmatter status block — violates Conduction spec schema

The spec opens with ---\nstatus: proposed\n--- as YAML frontmatter. The Conduction writing-specs guide requires status as a bold inline field: **Status**: idea | planned | in-progress | done. The spec is also missing required fields **Scope** and **OpenSpec changes**, and required sections ## Non-Functional Requirements, ## Acceptance Criteria, and ## Notes.

WilcoLouwerse · 2026-05-12T15:36:33Z

@@ -0,0 +1,193 @@
+# procest-tenant-migration Specification


[CONCERN] spec.md placed inside change directory, not canonical openspec/specs/ tree

The file is at openspec/changes/.../specs/procest-tenant-migration/spec.md. Per the writing-specs guide, specs belong at openspec/specs/{domain}/spec.md. Change directories contain design.md, proposal.md, and tasks.md, not spec definitions. Fix: move to openspec/specs/tenant-identity/spec.md.

WilcoLouwerse · 2026-05-12T15:36:35Z

[CONCERN] MW-1.1 task marked open but implementation already merged — task tracker misleading

Tasks MW-1.1 through MW-1.3 and SVC-1 through SVC-3 are left as open checkboxes even though they are implemented in this PR. Fix: tick the completed tasks or add a scope note.

WilcoLouwerse

Review

🔴 Blockers (6)

TenantMiddleware never registered — middleware is completely inert (lib/AppInfo/Application.php:63)
Application.php only registers ZgwAuthMiddleware. TenantMiddleware is not registered via $context->registerMiddleware() anywhere in the diff. Until it is registered, every beforeController() guard — including the new 403 status block — is dead code. Any call to a procest endpoint bypasses tenant resolution entirely. Fix: add $context->registerMiddleware(class: TenantMiddleware::class); in Application::register() and the corresponding use import.
Generic \Exception(code=403) used as control flow — afterException over-matches — lib/Middleware/TenantMiddleware.php:27
Empty-string _tenantId set when neither uuid nor id key present — potential cross-tenant IDOR — lib/Middleware/TenantMiddleware.php:20
Null/empty status silently bypasses lifecycle check — provisioning orgs pass through — lib/Middleware/TenantMiddleware.php:22
Implementation diverges from spec: TenantLifecycleService::isActive() is never called — lib/Middleware/TenantMiddleware.php:22
provisionTenant: $org->getOwner() may not exist on OR entity — silent escalation to admin — lib/Service/TenantService.php:280

🟡 Concerns (7)

getTenantByGroupId: linear O(n) scan with findAll(limit:500) may miss tenants in large deployments — lib/Service/TenantService.php:213
getTenantForUser returns first org without validating user membership — potential IDOR — lib/Service/TenantService.php:171
findOrganisationsByUserId silently swallows all Throwables — no logging on OR errors — lib/Service/TenantService.php:453
TenantMiddleware.php SPDX header status unknown — diff starts mid-file — lib/Middleware/TenantMiddleware.php:1
spec.md uses YAML frontmatter status block — violates Conduction spec schema — openspec/changes/migrate-tenant-to-or-tenant/specs/procest-tenant-migration/spec.md:1
spec.md placed inside change directory, not canonical openspec/specs/ tree — openspec/changes/migrate-tenant-to-or-tenant/specs/procest-tenant-migration/spec.md:1
MW-1.1 task marked open but implementation already merged — task tracker misleading — openspec/changes/migrate-tenant-to-or-tenant/tasks.md:970

🟢 Minor (2)

getOrganisationMapper and getTenantLifecycleService duplicate the installed-apps check (lib/Service/TenantService.php:466)
Both private helpers call in_array('openregister', $this->appManager->getInstalledApps(), true) independently. Each call may trigger a DB read. Extract the check into a private bool $orAvailable property set once lazily.
Regex in afterException message extraction is fragile — non-word characters in status silently fall back (lib/Middleware/TenantMiddleware.php:43)
preg_match('/Organisation is (\w+)/', $message, $m) fails for status values with hyphens or underscores (e.g., de-provisioning), silently falling back to 'inactive' and misreporting the actual status. Fix: pass the status value directly on the exception class rather than re-parsing the message string.

Reviewed by WilcoLouwerse via automated batch review.

rubenvdlinde requested review from Rem-Dam, WilcoLouwerse, bbrands02 and rjzondervan as code owners May 11, 2026 21:51

rubenvdlinde mentioned this pull request May 12, 2026

fix(procest): repair OR-API drift + dangling seed references #417

Open

2 tasks

WilcoLouwerse reviewed May 12, 2026

View reviewed changes

WilcoLouwerse requested changes May 12, 2026

View reviewed changes

Conversation

rubenvdlinde commented May 11, 2026

Investigation finding

What

Not done (no-op for this codebase)

Uh oh!

github-actions Bot commented May 11, 2026

Quality Report — ConductionNL/procest @ 3ffed81

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

Review

🔴 Blockers (6)

🟡 Concerns (7)

🟢 Minor (2)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Quality Report — ConductionNL/procest @ `3ffed81`