From 6fa8b5e6d0bda6101a80f0003ff3d0449f99b5e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 13:32:13 +0000 Subject: [PATCH] fix(spec)!: HierarchyScopeContext names organizationId as the tenancy authority and requires it (#5858) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `HierarchyScopeContext` declared `organizationId?` and `tenantId?` side by side with no doc saying which one carries the caller's active organization. The one in-repo producer filled `organizationId` from a `SharingExecutionContext` whose only tenancy member is `tenantId` (structurally always null), while the real consumer reads `organizationId` and skipped tenant isolation on null — two individually contract-compliant ends adding up to a reachable cross-org read (#5852). - `organizationId` documented as AUTHORITATIVE (null = platform/unscoped, matching `EvalUser.organizationId`), per the #3280/#3290 naming convention that `scripts/check-org-identifier.mjs` gates. - `organizationId?: string | null` -> `organizationId: string | null`: a producer that omits the caller's org now fails to compile instead of handing every resolver an `undefined`. - `tenantId` retained as a `@deprecated` alias (NOT removed) with the explicit "a resolver must not depend on it alone" obligation. - `IHierarchyScopeResolver.resolveOwnerIds` documents the fail-closed rule: a null organization is "no org", never "every org". Pins: two compile-time probes (`@ts-expect-error` on the omitted field and on tenantId-only; a RequiredKeys pin in both directions) plus an AST prose pin over the three doc obligations, in a file with a zero budget in test-typecheck-debt.json. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011M7UwH25Unfi73UHim7ajY --- .../hierarchy-scope-organization-authority.md | 49 +++++++ .../src/contracts/sharing-service.test.ts | 133 +++++++++++++++++- .../spec/src/contracts/sharing-service.ts | 40 +++++- 3 files changed, 220 insertions(+), 2 deletions(-) create mode 100644 .changeset/hierarchy-scope-organization-authority.md diff --git a/.changeset/hierarchy-scope-organization-authority.md b/.changeset/hierarchy-scope-organization-authority.md new file mode 100644 index 0000000000..273fcfbec6 --- /dev/null +++ b/.changeset/hierarchy-scope-organization-authority.md @@ -0,0 +1,49 @@ +--- +"@objectstack/spec": major +--- + +fix(spec)!: `HierarchyScopeContext.organizationId` is the authoritative tenancy field, and it is now required (#5858) + +`HierarchyScopeContext` declared `organizationId?: string | null` and +`tenantId?: string | null` side by side with no doc saying which one carries the +caller's active organization. That silence had a measured cost (#5852): the +single in-repo producer (`@objectstack/plugin-sharing`) filled `organizationId` +from a `SharingExecutionContext` whose only tenancy member is `tenantId`, so the +read was structurally always `null`, while the real consumer — the enterprise +`hierarchy-scope-resolver` — reads `organizationId`, saw `null`, and skipped +tenant isolation. Both ends were individually contract-compliant; the pair was a +reachable cross-organization read. + +The contract now says which field is authoritative, and enforces it structurally +rather than in prose: + +- **`organizationId` is the authority** — the caller's active organization, + `null` = platform/unscoped, the same meaning `EvalUser.organizationId` + already carries. This is the repo's settled name for the concept, not a new + one: #3280 blessed `organizationId` as the developer-facing name for the + caller's active org (matching the `organization_id` column and + `current_user.organizationId` in RLS), #3290 removed the `session.tenantId` + alias in v11, and `scripts/check-org-identifier.mjs` gates it in CI. +- **It is REQUIRED** — `organizationId: string | null`, never omitted. A + producer that forgets to state the caller's org now fails to compile instead + of handing every resolver an `undefined` it will read as "unscoped". Stating + "no org" is still allowed, but it must be stated: `null` is a value, not an + omission. +- **`tenantId` is retained as a deprecated alias**, not removed. It stays the + generic driver-layer tenancy knob (a database-per-tenant kernel legitimately + puts an *environment* id there), and its doc now says a resolver must not + depend on it alone or use it as a stand-in for a `null` `organizationId`. + Its removal is a separate, deliberate retirement. +- **`IHierarchyScopeResolver.resolveOwnerIds` documents the fail-closed + obligation**: when `organizationId` is `null`, an implementation must not + build the owner set as though no tenancy constraint applied — "no org" is + never "every org". Return owner-only (or throw, which the sharing layer + treats the same way); never widen. + +**Migration.** Breaking for *producers* of the context only — implementers of +`IHierarchyScopeResolver` are unaffected (a required property is strictly easier +to consume). A producer that omitted the key adds `organizationId: `; a producer that was passing the org under `tenantId` must +move it, which is the bug this closes rather than a rename to absorb. The only +in-repo producer already supplied the key, so nothing in this repo changed +shape. diff --git a/packages/spec/src/contracts/sharing-service.test.ts b/packages/spec/src/contracts/sharing-service.test.ts index 3ec4956931..5e54064738 100644 --- a/packages/spec/src/contracts/sharing-service.test.ts +++ b/packages/spec/src/contracts/sharing-service.test.ts @@ -1,7 +1,11 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; -import type { RecordShareRecipientType, SharingRuleRecipientType } from './sharing-service'; +import type { + HierarchyScopeContext, + RecordShareRecipientType, + SharingRuleRecipientType, +} from './sharing-service'; import { ShareRecipientType } from '../security/sharing.zod'; /** @@ -126,3 +130,130 @@ describe('[#5125] ISharingService write-gate bypass documentation parity', () => expect(docOf.get('buildReadFilter')).not.toContain('modifyAllRecords'); }); }); + +/** + * [#5858] `HierarchyScopeContext` names ONE authoritative tenancy field. + * + * The interface declared `organizationId?` and `tenantId?` side by side with no + * doc saying which one carries the caller's active organization. The single + * in-repo producer filled `organizationId` from a `SharingExecutionContext` + * that only has `tenantId`, so the read was structurally always `null`; the + * real consumer (cloud `security-enterprise`) reads `organizationId`, saw + * `null`, and skipped tenant isolation. Both ends were "contract-compliant" and + * the pair leaked across organizations (#5852). + * + * The fix is on the contract, not on either consumer: `organizationId` is the + * authority (repo convention — #3280 blessed the name, #3290 removed the + * `session.tenantId` alias, `scripts/check-org-identifier.mjs` gates it) and is + * now REQUIRED, so a producer that forgets it fails to compile rather than + * handing every resolver an `undefined` org. `tenantId` survives as a + * deprecated alias — removal is its own retirement, not a rider here. + * + * Two kinds of pin below, because the change has two halves: type-level ones + * that tsc evaluates (`tsconfig.test.json` compiles this file — #5286), and + * prose ones, because prose is the other half of what was missing. + */ +describe('[#5858] HierarchyScopeContext tenancy authority', () => { + it('requires `organizationId` and keeps `tenantId` optional (compile-time)', () => { + // A caller with no active org states so EXPLICITLY. `null` is a value the + // contract carries (platform/unscoped), never an omission. + const platformScoped: HierarchyScopeContext = { userId: 'usr_1', organizationId: null }; + const orgScoped: HierarchyScopeContext = { + userId: 'usr_1', + organizationId: 'org_east', + tenantId: 'env_prod', + }; + + // THE pin for the required-ness. Omitting the authoritative field is the + // exact producer bug #5852 measured; it must not compile. Deleting the `?` + // again turns this directive into an unused-`@ts-expect-error` error, and + // this file carries no entry in `test-typecheck-debt.json` — so its budget + // is zero and the gate goes red. + // @ts-expect-error `organizationId` is REQUIRED — omitting it must not compile (#5858) + const missingOrg: HierarchyScopeContext = { userId: 'usr_1' }; + + // The deprecated alias is NOT a substitute: supplying only `tenantId` + // leaves the authoritative field unstated, which is the same defect. + // @ts-expect-error `tenantId` does not satisfy the authoritative field (#5858) + const tenantOnly: HierarchyScopeContext = { userId: 'usr_1', tenantId: 'env_prod' }; + + expect(platformScoped.organizationId).toBeNull(); + expect(orgScoped.organizationId).toBe('org_east'); + expect(missingOrg.userId).toBe('usr_1'); + expect(tenantOnly.tenantId).toBe('env_prod'); + }); + + it('pins WHICH keys are mandatory, in both directions (compile-time)', () => { + // `-?` strips optionality, then `object extends Pick` is true exactly + // when K was optional — so the union is the mandatory keys. + type RequiredKeys = { + [K in keyof T]-?: object extends Pick ? never : K; + }[keyof T]; + + const mandatory: Array> = ['userId', 'organizationId']; + + // The other direction, and the ⛔-not-deleted guard in one: `tenantId` is + // still a member (a removal breaks the reference below) and still OPTIONAL + // (making the deprecated alias mandatory would be the mirror mistake). + // @ts-expect-error `tenantId` stays optional — it is a deprecated alias, not a second authority (#5858) + const notMandatory: RequiredKeys = 'tenantId'; + + expect(mandatory).toEqual(['userId', 'organizationId']); + expect(notMandatory).toBe('tenantId'); + }); + + it('documents the authority, the deprecation, and the fail-closed obligation', async () => { + const ts = (await import('typescript')).default; + const { readFileSync } = await import('node:fs'); + const { dirname, resolve } = await import('node:path'); + const { fileURLToPath } = await import('node:url'); + + const file = resolve(dirname(fileURLToPath(import.meta.url)), 'sharing-service.ts'); + const source = ts.createSourceFile( + file, + readFileSync(file, 'utf8'), + ts.ScriptTarget.Latest, + /* setParentNodes */ true, + ); + + const memberDocs = (name: string): Map => { + const iface = source.statements.find( + (s): s is import('typescript').InterfaceDeclaration => + ts.isInterfaceDeclaration(s) && s.name.text === name, + ); + expect(iface, `${name} must still be an interface in this file`).toBeDefined(); + const out = new Map(); + for (const member of iface!.members) { + if (!member.name || !ts.isIdentifier(member.name)) continue; + // `getFullText` carries leading trivia — the doc comment an IDE shows + // on hover — with no assumption about how it is wrapped. + out.set(member.name.text, member.getFullText(source)); + } + return out; + }; + + const ctx = memberDocs('HierarchyScopeContext'); + // Anti-vacuity 1: the enumeration found the real members, so a rename or a + // deletion cannot quietly empty the assertions. `tenantId` being listed IS + // the "not removed" pin — its retirement is a separate, deliberate change. + expect([...ctx.keys()]).toEqual(['userId', 'organizationId', 'tenantId']); + + expect(ctx.get('organizationId')).toContain('AUTHORITATIVE'); + expect(ctx.get('organizationId')).toContain('platform/unscoped'); + expect(ctx.get('organizationId')).toContain('MUST scope its owner set by this field'); + + expect(ctx.get('tenantId')).toContain('@deprecated'); + expect(ctx.get('tenantId')).toContain('Not the authority for hierarchy scoping'); + + // Anti-vacuity 2: the search DISCRIMINATES. `userId` is the honest negative + // — it is identity, not tenancy, and calling it authoritative would itself + // be drift. + expect(ctx.get('userId')).not.toContain('AUTHORITATIVE'); + + const resolver = memberDocs('IHierarchyScopeResolver'); + expect([...resolver.keys()]).toEqual(['resolveOwnerIds']); + // A `null` organization is "no org", never "every org". + expect(resolver.get('resolveOwnerIds')).toContain('Fail CLOSED'); + expect(resolver.get('resolveOwnerIds')).toContain('never widen'); + }); +}); diff --git a/packages/spec/src/contracts/sharing-service.ts b/packages/spec/src/contracts/sharing-service.ts index ff46c0ca0f..256221a6f2 100644 --- a/packages/spec/src/contracts/sharing-service.ts +++ b/packages/spec/src/contracts/sharing-service.ts @@ -393,9 +393,40 @@ export interface IBusinessUnitGraphService { */ export type HierarchyScope = 'unit' | 'unit_and_below' | 'own_and_reports'; +/** + * Caller identity a {@link IHierarchyScopeResolver} resolves a + * {@link HierarchyScope} against. + * + * **`organizationId` is the AUTHORITATIVE tenancy field** — the one a resolver + * scopes its owner-set query by. It is REQUIRED (`string | null`, never + * omitted) so a producer that forgets to supply it fails to compile instead of + * silently handing every resolver an `undefined` org: two sides can each look + * contract-compliant while the pair leaks across organizations (#5852/#5858). + * The name follows the repo-wide convention: #3280 made `organizationId` the + * blessed developer-facing name for the caller's active org (matching the + * `organization_id` column and `current_user.organizationId` in RLS) and #3290 + * removed the `session.tenantId` alias in v11; `scripts/check-org-identifier.mjs` + * keeps it that way. + */ export interface HierarchyScopeContext { userId: string; - organizationId?: string | null; + /** + * AUTHORITATIVE. Active organization ID of the caller (`null` = + * platform/unscoped) — same meaning as `EvalUser.organizationId`. A resolver + * MUST scope its owner set by this field; see + * {@link IHierarchyScopeResolver.resolveOwnerIds} for the `null` obligation. + */ + organizationId: string | null; + /** + * Generic driver-layer tenancy knob, carried through for kernels that key + * isolation off something other than the organization (database-per-tenant + * deployments legitimately put an *environment* id here). + * + * @deprecated Not the authority for hierarchy scoping — a resolver MUST NOT + * depend on it alone, and MUST NOT treat it as a substitute for a `null` + * {@link HierarchyScopeContext.organizationId}. Read `organizationId`. + * Retained for compatibility only; removal goes through the retirement flow. + */ tenantId?: string | null; } @@ -420,6 +451,13 @@ export interface IHierarchyScopeResolver { /** * Owner ids whose records the caller may see under `scope` (must include the * caller). Empty/throw → caller falls back to owner-only. + * + * **Fail CLOSED on a missing organization.** When the authoritative + * {@link HierarchyScopeContext.organizationId} is `null`, an implementation + * MUST NOT build the owner set as though no tenancy constraint applied — + * "no org" is not "every org". Return owner-only (or throw, which the sharing + * layer treats the same way); never widen. Falling back to + * {@link HierarchyScopeContext.tenantId} instead is not fail-closed either. */ resolveOwnerIds(context: HierarchyScopeContext, scope: HierarchyScope): Promise; }