diff --git a/.changeset/24583-7a3-sharing-null-readers.md b/.changeset/24583-7a3-sharing-null-readers.md new file mode 100644 index 00000000000..afff86eb22f --- /dev/null +++ b/.changeset/24583-7a3-sharing-null-readers.md @@ -0,0 +1,23 @@ +--- +'@objectstack/spec': minor +'@objectstack/plugin-sharing': minor +'@objectstack/plugin-approvals': minor +'@objectstack/metadata-protocol': minor +--- + +feat(plugin-sharing,plugin-approvals,metadata-protocol)!: the organization-less readers of sharing rules, approval units and the seed loader retire (ADR-0131 D2/D8, 18.x step 7) + +Clause-②: yes (narrowing): a declared sharing rule served under `single` gains the `deploymentWide: true` key on the wire in place of `organization_id: null` (widening); the sharing-rule admin reads, the approval unit screen and the scoped seed pre-load stop reading rows with no organization (narrowing). A declared rule keeps applying deployment-wide; nothing an author declares is refused. + + + +**BREAKING** — readers narrow. On a certified database (every 18.x database: a fresh install, or one the organization-ownership ceremony upgraded) none of these readers has a row left to find; they matter only to a database that still holds organization-less rows, which 18.x refuses at boot. + +- **A declared sharing rule is deployment-wide by a typed marker, never a NULL organization.** Under `single` it is served as one projection carrying `deploymentWide: true` and no `organization_id` key (was `organization_id: null`); its criteria sweep carries no tenant (the predicate ADR-0131 D8 computes under `single` is none) and each grant is stamped with its record's organization. Under `group` / `isolated` it is served once per organization, exactly as before. FROM `rule.organization_id === null` TO `rule.deploymentWide === true` for any client that told a declared rule apart by its NULL organization (`GET /api/v1/sharing/rules`, `GET /api/v1/sharing/rules/:idOrName`). +- **The contract names the marker.** `SharingRuleRow` (`@objectstack/spec/contracts`) declares `deploymentWide?: true`, documented: set only on a declared rule's `single` projection, which carries no `organization_id`. +- **A stored sharing rule with no organization sweeps nothing and grants nothing** where the deployment registers the organization object, reported once at `error` with the remedy (run the organization-ownership ceremony, or re-author the rule in its organization). A lean embedding with no organization object keeps sweeping the deployment: it has no organization to scope by. +- **Sharing-rule administration reads the caller's own organization only.** `listRules`, `getRule` (by id and by name), `evaluateRule` and `deleteRule` no longer reach an `organization_id = null` row for an organization-bound admin; system contexts and platform operators keep the unfiltered read. Deleting a "platform-global" rule no longer needs platform authority, because that rule class no longer exists: the guard is removed. +- **The approval unit screen is a strict equality.** A `department` approver resolves a `sys_business_unit` in the request's organization only (was: that organization or no organization). The ceremony attributes `sys_business_unit`, and a fresh install stamps it. +- **The scoped seed pre-load reads one thing: its organization's rows.** The second read that collected keys held by organization-less rows, and refused to twin or adopt them, is deleted; a scoped upsert/ignore/update issues no `organization_id IS NULL` read on any path. + +Remedy for a database that still holds organization-less rows: `os migrate organization-ownership --out PLAN`, review it, then `os migrate organization-ownership --apply --plan-file PLAN --backup-taken`. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index c82007f2aa7..8c25d831558 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -159,7 +159,7 @@ One of the largest consumers of the flag; | 37 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `packages/plugins/plugin-sharing/src/sharing-plugin.ts#buildSharingMiddleware` | | 38 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link **creation** while the policy is off — resolution is **not** bypassed since #14033 (`publicSharing.enabled` is a standing policy held at every redemption): a link minted this way does not resolve until the block is enabled | `packages/plugins/plugin-sharing/src/share-link-service.ts#createLink`, `#revokeLink`, `#listLinks` | | 39 | Sharing-rule provenance stamp skipped | Lose: the row is not marked as an admin customization — seeder / `defineRule` / boot reconcilers are "the package door" | `packages/plugins/plugin-sharing/src/sharing-rule-provenance.ts#bindRuleProvenanceStamp` | -| 40 | Sharing-rule service write + delete paths return early | Lose: the manage-rules gate on the service surface, the platform-global-rule delete guard, and the declared-name guard on `sys_sharing_rule` writes (a new row or a rename taking a declared rule's name, 409) — `defineRule` makes that check itself before its own system write, and the other system writers are the platform's machinery (the ownership ceremony, migrations) | `packages/plugins/plugin-sharing/src/sharing-rule-service.ts#assertCanManageRules`, `#assertCanDeletePlatformGlobalRule`, `#bindDeclaredRuleNameGuard` | +| 40 | Sharing-rule service write + delete paths return early | Lose: the manage-rules gate on the service surface and the declared-name guard on `sys_sharing_rule` writes (a new row or a rename taking a declared rule's name, 409) — `defineRule` makes that check itself before its own system write, and the other system writers are the platform's machinery (the ownership ceremony, migrations) | `packages/plugins/plugin-sharing/src/sharing-rule-service.ts#assertCanManageRules`, `#bindDeclaredRuleNameGuard` | ### 4. Approvals, attachments, comments, knowledge diff --git a/packages/metadata-protocol/src/seed-loader-null-organization-holder.test.ts b/packages/metadata-protocol/src/seed-loader-null-organization-holder.test.ts index 54a75980ebd..0e1837eac43 100644 --- a/packages/metadata-protocol/src/seed-loader-null-organization-holder.test.ts +++ b/packages/metadata-protocol/src/seed-loader-null-organization-holder.test.ts @@ -6,20 +6,23 @@ import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts' import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core'; /** - * [#24301] An organization-scoped load refuses a seed row whose natural key a - * row with NO organization already holds, as a per-row error, instead of - * inserting a twin of that row; and it never adopts the row. + * [ADR-0131 D8] An organization-scoped load reads ITS OWN rows, and only them: + * one read per decision, `organization_id = ORG`, with no NULL arm on any of + * the three readers of `loadExistingRecords` (its #8896 note): * - * These pins hold the three readers of `loadExistingRecords` (its #8896 note) - * apart, which a real-driver file cannot reach: + * 1. the upsert/ignore/update pre-load; + * 2. `writeBatchPartial`'s retry recheck; + * 3. `writeOne`'s per-row recheck. * - * 1. the upsert/ignore pre-load sees the unowned holder and refuses; - * 2. `writeBatchPartial`'s retry recheck, and - * 3. `writeOne`'s recheck read this organization's rows only: a row an - * earlier attempt committed carries the organization, so an unowned row - * is never taken for one, and the refused row never reaches either. + * Until step 7 of the organization-ownership line the pre-load also read the + * keys held by rows with NO organization and refused to twin or adopt them + * (#24301). Certification leaves no such row (the ownership ceremony + * attributes every one, D10) and a new write without an organization is + * stamped or refused (D9), so that second read retired. These pins hold the + * retirement: no `{ organization_id: null }` read is issued, whatever the + * mode or path, and the scoped load behaves as the organization's own. * - * The real drivers' NULL predicate and the stored rows are pinned in + * The real driver's side is pinned in * `packages/runtime/src/seed-loader-null-organization-holder-real-driver.integration.test.ts`. */ @@ -112,76 +115,47 @@ const widgets = (mode: string) => [{ records: [{ sku: 'W-HELD', label: 'held' }, { sku: 'W-NEW', label: 'new' }], }] as any[]; -/** A row no organization owns, holding the natural key `W-HELD`. */ -function plantUnowned(store: Record) { - store.tw_widget = [{ id: 'w_unowned', sku: 'W-HELD', label: 'old', organization_id: null }]; +/** This organization's own row, holding the natural key `W-HELD`. */ +function plantOwn(store: Record) { + store.tw_widget = [{ id: 'w_own', sku: 'W-HELD', label: 'old', organization_id: ORG }]; } -const refusalsOf = (result: { errors: E[] }) => - result.errors.filter((e) => e.field === 'organization_id' && e.targetObject === e.sourceObject); +const nullReads = (finds: any[]) => finds.filter((f) => f.where && 'organization_id' in f.where && f.where.organization_id == null); -describe('[#24301] a scoped load over a NULL-organization holder of a natural key', () => { - it.each(['upsert', 'ignore'])('%s: refuses the held key by name, inserts the rest, and leaves the holder alone', async (mode) => { - const { engine, store } = createFaithfulEngine(); - plantUnowned(store); +describe('[ADR-0131 D8] a scoped load reads only its organization\'s rows', () => { + it.each(['upsert', 'ignore', 'update'])('%s: one scoped pre-load, no NULL read, its own row matched', async (mode) => { + const { engine, store, finds } = createFaithfulEngine(); + plantOwn(store); const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ seeds: widgets(mode), config: config(ORG) }); - expect(store.tw_widget.map((r) => r.sku).sort()).toEqual(['W-HELD', 'W-NEW']); - expect(store.tw_widget[0]).toEqual({ id: 'w_unowned', sku: 'W-HELD', label: 'old', organization_id: null }); - expect(store.tw_widget.find((r) => r.sku === 'W-NEW')?.organization_id).toBe(ORG); - expect(result.summary.totalInserted).toBe(1); - expect(result.summary.totalErrored).toBe(1); - expect(engine.update).not.toHaveBeenCalled(); - const [refusal] = refusalsOf(result); - expect(refusal).toMatchObject({ sourceObject: 'tw_widget', targetField: 'sku', attemptedValue: 'W-HELD', recordIndex: 0 }); - expect(refusal.message).toContain("'w_unowned'"); - // The ceremony is the remedy, with its reviewed plan; the first sentences - // (what was refused, and why) stay. - expect(refusal.message).toContain('which has no organization (organization_id is NULL)'); - expect(refusal.message).toContain('decided by the organization-ownership ceremony, not by a seed load.'); - expect(refusal.message).toContain( - 'Attribute the unowned rows first (`os migrate organization-ownership --out PLAN`, then the same command with ' - + '--apply --plan-file PLAN --backup-taken), then re-run this seed', - ); - expect(refusal.message).not.toMatch(/fresh installs only|on a 17\.x release|carries the upgrade path/); - }); - - it("the organization's own row wins: a key held both by it and by an unowned row is matched, not refused", async () => { - const { engine, store } = createFaithfulEngine(); - plantUnowned(store); - store.tw_widget.push({ id: 'w_own', sku: 'W-HELD', label: 'held', organization_id: ORG }); - const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ seeds: widgets('upsert'), config: config(ORG) }); - expect(result.errors).toEqual([]); - expect(result.summary.totalSkipped).toBe(1); - expect(store.tw_widget.filter((r) => r.sku === 'W-HELD')).toHaveLength(2); + expect(nullReads(finds)).toEqual([]); + expect(finds.filter((f) => f.objectName === 'tw_widget').map((f) => f.where)).toEqual([{ organization_id: ORG }]); + // The held key is this organization's: matched, never twinned. + expect(store.tw_widget.filter((r) => r.sku === 'W-HELD')).toHaveLength(1); + if (mode === 'update') { + expect(store.tw_widget).toHaveLength(1); + } else { + expect(store.tw_widget.find((r) => r.sku === 'W-NEW')?.organization_id).toBe(ORG); + expect(result.summary.totalInserted).toBe(1); + } }); - it('update mode writes no new row, so it keeps its behaviour: the held key is skipped, nothing refused', async () => { + it('the self-referencing (sequential) path reads the same way', async () => { const { engine, store, finds } = createFaithfulEngine(); - plantUnowned(store); - const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ seeds: widgets('update'), config: config(ORG) }); - - expect(result.errors).toEqual([]); - expect(result.summary.totalSkipped).toBe(2); - expect(store.tw_widget).toHaveLength(1); - expect(finds.some((f) => f.where?.organization_id === null)).toBe(false); - }); - - it('the self-referencing (sequential) path refuses the same way', async () => { - const { engine, store } = createFaithfulEngine(); - store.tw_unit = [{ id: 'u_unowned', code: 'ROOT', parent: null, organization_id: null }]; + store.tw_unit = [{ id: 'u_own', code: 'ROOT', parent: null, organization_id: ORG }]; const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ seeds: [{ object: 'tw_unit', externalId: 'code', mode: 'upsert', env: ['prod', 'dev', 'test'], records: [{ code: 'ROOT' }, { code: 'LEAF' }] }] as any[], config: config(ORG), }); + expect(result.errors).toEqual([]); expect(store.tw_unit.map((r) => r.code).sort()).toEqual(['LEAF', 'ROOT']); - expect(refusalsOf(result).map((e) => e.attemptedValue)).toEqual(['ROOT']); + expect(nullReads(finds)).toEqual([]); }); - it('writeBatchPartial retry recheck: reads only the organization\'s rows, and the refused row never reaches an insert', async () => { + it('writeBatchPartial retry recheck: reads only the organization\'s rows, and inserts nothing twice', async () => { const { engine, store, finds } = createFaithfulEngine(); - plantUnowned(store); + plantOwn(store); // The first array insert commits, then loses its response: the retry's // recheck must find W-NEW as already written and insert nothing again. const realInsert = (engine.insert as any).getMockImplementation(); @@ -195,24 +169,19 @@ describe('[#24301] a scoped load over a NULL-organization holder of a natural ke }); const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ seeds: widgets('upsert'), config: config(ORG) }); + expect(calls).toBe(1); expect(payloads.flat().map((r) => r.sku)).toEqual(['W-NEW']); expect(store.tw_widget.map((r) => r.sku).sort()).toEqual(['W-HELD', 'W-NEW']); - expect(refusalsOf(result)).toHaveLength(1); expect(result.summary.totalInserted).toBe(1); - // One NULL read, the pre-load's; every recheck after the first insert is scoped. - const nullReads = finds.filter((f) => f.where?.organization_id === null); - expect(nullReads).toHaveLength(1); - const afterPreload = finds.slice(finds.findIndex((f) => f.where?.organization_id === null) + 1) - .filter((f) => f.objectName === 'tw_widget'); - // The retry happened, and its recheck is the one scoped read after the pre-load. - expect(calls).toBe(1); - expect(afterPreload.map((f) => f.where)).toEqual([{ organization_id: ORG }]); + expect(nullReads(finds)).toEqual([]); + // The pre-load and the retry's recheck: both scoped. + expect(finds.filter((f) => f.objectName === 'tw_widget').map((f) => f.where)) + .toEqual([{ organization_id: ORG }, { organization_id: ORG }]); }); it('writeOne recheck (an engine with no insertMany, an uncertain batch): the same, per row', async () => { const { engine, store, finds } = createFaithfulEngine(); - plantUnowned(store); - store.tw_widget.push({ id: 'w_unowned_2', sku: 'W-HELD-2', label: 'old', organization_id: null }); + plantOwn(store); const realInsert = (engine.insert as any).getMockImplementation(); const payloads: any[][] = []; (engine.insert as any).mockImplementation(async (obj: string, data: any, opts: any) => { @@ -224,27 +193,26 @@ describe('[#24301] a scoped load over a NULL-organization holder of a natural ke const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ seeds: [{ object: 'tw_widget', externalId: 'sku', mode: 'upsert', env: ['prod', 'dev', 'test'], - records: [{ sku: 'W-HELD' }, { sku: 'W-NEW' }, { sku: 'W-HELD-2' }, { sku: 'W-NEW-2' }], + records: [{ sku: 'W-HELD' }, { sku: 'W-NEW' }, { sku: 'W-NEW-2' }], }] as any[], config: config(ORG), }); // One whole-array attempt, then the per-row degradation: two single inserts. expect(payloads.map((p) => p.map((r) => r.sku))).toEqual([['W-NEW', 'W-NEW-2'], ['W-NEW'], ['W-NEW-2']]); - expect(store.tw_widget.map((r) => r.sku).sort()).toEqual(['W-HELD', 'W-HELD-2', 'W-NEW', 'W-NEW-2']); - expect(refusalsOf(result).map((e) => e.attemptedValue)).toEqual(['W-HELD', 'W-HELD-2']); + expect(store.tw_widget.map((r) => r.sku).sort()).toEqual(['W-HELD', 'W-NEW', 'W-NEW-2']); expect(result.summary.totalInserted).toBe(2); - expect(finds.filter((f) => f.where?.organization_id === null)).toHaveLength(1); + expect(nullReads(finds)).toEqual([]); }); - it('an unscoped load reads no NULL holders and keeps matching the unowned row', async () => { + it('an unscoped load reads no NULL holders either, and matches by key alone', async () => { const { engine, store, finds } = createFaithfulEngine(); - plantUnowned(store); + plantOwn(store); const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ seeds: widgets('upsert'), config: config() }); - expect(refusalsOf(result)).toEqual([]); + expect(result.errors).toEqual([]); expect(store.tw_widget.filter((r) => r.sku === 'W-HELD')).toHaveLength(1); expect(store.tw_widget.find((r) => r.sku === 'W-HELD')?.label).toBe('held'); - expect(finds.some((f) => f.where?.organization_id === null)).toBe(false); + expect(nullReads(finds)).toEqual([]); }); }); diff --git a/packages/metadata-protocol/src/seed-loader.ts b/packages/metadata-protocol/src/seed-loader.ts index 3b0cb448f35..c1b1195750d 100644 --- a/packages/metadata-protocol/src/seed-loader.ts +++ b/packages/metadata-protocol/src/seed-loader.ts @@ -1129,24 +1129,9 @@ export class SeedLoaderService implements ISeedLoaderService { // own copy (otherwise upsert would clobber other tenants' rows // that share the same natural key — e.g. `name: 'Acme Corp'`). let existingRecords: Map | undefined; - /** - * [#24301] Natural key → the row holding it with NO organization, read - * only on an organization-scoped upsert/ignore: the two modes whose - * unmatched key is written as a new row. Kept apart from - * `existingRecords` so such a row is never matched as this - * organization's own (see {@link decideWriteAction}'s `refuse`). - */ - let unownedHolders: Map | undefined; if ((mode === 'upsert' || mode === 'update' || mode === 'ignore') && !config.dryRun) { - if (config.organizationId && mode !== 'update') unownedHolders = new Map(); - existingRecords = await this.loadExistingRecords( - objectName, - externalId, - config.organizationId, - unownedHolders, - ); + existingRecords = await this.loadExistingRecords(objectName, externalId, config.organizationId); } - let unownedHolderRefusalLogged = false; // [#21665] Per-organization row identity. On a per-organization replay, // decide which id each row authored with an `id` holds in THIS @@ -1824,47 +1809,6 @@ export class SeedLoaderService implements ISeedLoaderService { continue; } - // [#24301] A natural key this organization does not hold, held by a row - // that has NO organization: refused by name, before either write path. - // Inserting would put a twin beside that row; updating it would adopt - // it, and which organization an unowned row belongs to is the - // organization-ownership ceremony's attribution, never the loader's. - if (unownedHolders && unownedHolders.size > 0) { - const decision = this.decideWriteAction(record, mode, externalId, existingRecords, comparands, unownedHolders); - if (decision.action === 'refuse') { - errored++; - const label = this.externalIdLabel(externalId); - const keyValue = this.externalIdKey(record, externalId); - const error: ReferenceResolutionError = { - sourceObject: objectName, - field: 'organization_id', - targetObject: objectName, - targetField: label, - attemptedValue: keyValue, - recordIndex: i, - message: - `Seed record #${i} of '${objectName}' was REFUSED: its natural key ` + - `(${label}=${this.externalIdDisplay(keyValue)}) is already held by row '${String(decision.holderId)}', ` + - `which has no organization (organization_id is NULL), and this seed load writes for organization ` + - `${config.organizationId}. Inserting the record would put a twin beside that row, and updating the ` + - `row would hand it to this organization: which organization an unowned row belongs to is decided by ` + - `the organization-ownership ceremony, not by a seed load. Attribute the unowned rows first ` + - `(\`os migrate organization-ownership --out PLAN\`, then the same command with ` + - `--apply --plan-file PLAN --backup-taken), then re-run this seed: the row is this organization's own ` + - `by then, and the upsert matches it.`, - }; - errors.push(error); - allErrors.push(error); - // `error`, not `warn` (#4632): the record is dropped. Said once per - // dataset; every refused row is still counted in `allErrors`. - if (!unownedHolderRefusalLogged) { - unownedHolderRefusalLogged = true; - this.logger.error(`[SeedLoader] ${error.message}`, undefined, { object: objectName, recordIndex: i }); - } - continue; - } - } - // [commit 1cba33f16] EARLY SIGNAL — emitted here, before this row reaches the // engine, for the deferrals it took on columns the write contract // requires on INSERT. @@ -2825,12 +2769,6 @@ export class SeedLoaderService implements ISeedLoaderService { * should do with a record — mirrors {@link writeRecord}'s mode/existing * logic exactly, but WITHOUT performing the write, so insert decisions can * be buffered and flushed as a batch instead of one call per record. - * - * [#24301] `unownedHolders` (an organization-scoped upsert/ignore only, see - * {@link loadExistingRecords}) turns an unmatched key that a row with NO - * organization holds into `refuse`: never `insert` (a twin beside that - * row) and never `update`/`skip` against it (adopting it). The - * organization's own row always wins over an unowned one. */ private decideWriteAction( record: Record, @@ -2838,16 +2776,12 @@ export class SeedLoaderService implements ISeedLoaderService { externalId: string | string[], existingRecords: Map | undefined, comparands: ReplayComparands, - unownedHolders?: Map, ): | { action: 'insert' } | { action: 'update'; id: string } - | { action: 'skip'; id?: string; uncompared?: readonly string[] } - | { action: 'refuse'; holderId?: string } { + | { action: 'skip'; id?: string; uncompared?: readonly string[] } { const key = this.externalIdKey(record, externalId); const existing = existingRecords?.get(key); - const unowned = !existing && (mode === 'upsert' || mode === 'ignore') ? unownedHolders?.get(key) : undefined; - if (unowned) return { action: 'refuse', holderId: this.extractId(unowned) }; switch (mode) { case 'update': @@ -3417,21 +3351,17 @@ export class SeedLoaderService implements ISeedLoaderService { * Natural key → the existing row, for the write decision (see the #8896 * note in the catch). Scoped to `organizationId`'s own rows when one is set. * - * [#24301] `unownedHolders`, passed by the scoped upsert/ignore pre-load - * only, also collects every key held by a row with NO organization and not - * by this organization: a second read, `organization_id` NULL, kept OUT of - * the returned map so no caller ever matches such a row as this - * organization's own. The two rechecks pass none: a row an earlier attempt - * of this load committed carries the organization, so an unowned row is - * never one of them. Another organization's row stays invisible to both - * reads, and the NULL read keeps only rows that really carry no - * organization, whatever the driver answers. + * [ADR-0131 D8] One read, no NULL arm. A second read that collected the + * keys held by rows with NO organization (so an organization-scoped + * upsert/ignore refused to twin or adopt them) retired with those rows: the + * organization-ownership ceremony attributes every one before a deployment + * is certified (D10), and a new write without an organization is stamped or + * refused (D9). */ private async loadExistingRecords( objectName: string, externalId: string | string[], organizationId?: string, - unownedHolders?: Map, ): Promise> { const map = new Map(); try { @@ -3451,18 +3381,6 @@ export class SeedLoaderService implements ISeedLoaderService { map.set(key, record); } } - if (organizationId && unownedHolders) { - const unownedRead: EngineQueryOptions = { - where: { organization_id: null }, - context: { isSystem: true }, - }; - const unowned = await this.engine.find(objectName, unownedRead); - for (const record of unowned || []) { - if (record?.organization_id != null) continue; - const key = this.externalIdKey(record, externalId); - if (key && !map.has(key) && !unownedHolders.has(key)) unownedHolders.set(key, record); - } - } } catch (error) { // [#8896] Discriminate by error TYPE. This map is not a convenience — // it IS the decision, in all three of its callers, and an empty map is diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 25141f4021e..372abb24975 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -953,11 +953,10 @@ describe('ApprovalService (node era)', () => { expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']); }); - // #3807 — an app's org tree is normally SEEDED, and a seed cannot know the - // organization id the runtime mints at boot, so those rows carry - // `organization_id = null` while every approval request carries an org. The - // old strict equality made each of them invisible and every `department` - // approver resolved to the dead `department:` literal. + // [ADR-0131 D1/D8] The unit screen is a strict equality. It admitted a NULL + // organization for seeded units until certification made every unit an + // organization's (#3807's reason is gone: the ceremony attributes + // `sys_business_unit`, and a new unit is stamped or refused, D9). const departmentInput = (value: string) => positionInput({ config: { approvers: [{ type: 'department' as const, value }], @@ -966,22 +965,19 @@ describe('ApprovalService (node era)', () => { }, }); - it('department approver: an env-wide (null-org) business unit still resolves (#3807)', async () => { + it('department approver: an organization-less unit is no organization’s — it does not resolve (ADR-0131 D8)', async () => { engine._tables['sys_business_unit'] = [ { id: 'bu_seeded', organization_id: null, active: true }, { id: 'bu_seeded_child', parent_business_unit_id: 'bu_seeded', organization_id: null, active: true }, ]; - // #14946: the SEEDED rows are the units; the membership rows are stamped, - // as the session write path leaves them. An org-less membership row on a - // seeded unit is pinned on its own in - // `business-unit-member-org-screen.test.ts` (B3) — it does NOT route. + // The membership rows are the request organization's, so a slate could only + // come from the unit screen admitting the org-less unit — the arm retired. engine._tables['sys_business_unit_member'] = [ { id: 'bm1', business_unit_id: 'bu_seeded', user_id: 'u5', organization_id: 't1' }, { id: 'bm2', business_unit_id: 'bu_seeded_child', user_id: 'u6', organization_id: 't1' }, ]; const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX); - // Both the seed check AND the subtree descent must see the null-org rows. - expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']); + expect(req.pending_approvers).toEqual(['department:bu_seeded']); }); it('department approver: another organization’s unit stays invisible (#3807 keeps the wall)', async () => { @@ -997,9 +993,9 @@ describe('ApprovalService (node era)', () => { expect(req.pending_approvers).toEqual(['department:bu_other']); }); - it('department approver: a null-org subtree does not drag in another org’s child unit (#3807)', async () => { + it('department approver: the subtree descent does not drag in another org’s child unit', async () => { engine._tables['sys_business_unit'] = [ - { id: 'bu_seeded', organization_id: null, active: true }, + { id: 'bu_seeded', organization_id: 't1', active: true }, { id: 'bu_mine', parent_business_unit_id: 'bu_seeded', organization_id: 't1', active: true }, { id: 'bu_theirs', parent_business_unit_id: 'bu_seeded', organization_id: 't2', active: true }, ]; diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 3382971460c..26deed17208 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -2582,8 +2582,8 @@ export class ApprovalService implements IApprovalService { * ⇒ the tenancy fact is ABSENT ⇒ leave routing exactly as it was. * * The `organization_id = null` limb is not timidity — it is the reading - * {@link businessUnitOrgScope} settled on one screen below, for the identical - * shape: null on a platform object means "owned by no organization", which is + * {@link businessUnitOrgScope} held for the identical shape until ADR-0131 + * D8 retired its NULL arm: null on a platform object meant "owned by no organization", which is * what a seed writes because a seed cannot know the organization id the * runtime mints at boot. Treating null as "not mine" would delete every * seeded team approver at once — a larger behaviour change than the hole @@ -2734,43 +2734,39 @@ export class ApprovalService implements IApprovalService { } /** - * Tenant scope for a `sys_business_unit` read that may legitimately be - * env-wide (#3807). - * - * `organization_id = null` on a platform object means "owned by no - * organization" — a row written by a seed, the file layer, or bootstrap, - * i.e. before (or outside) any org exists. A strict - * `organization_id = ` equality made every such row invisible: - * the seed check below found nothing, the whole expansion returned `[]`, and - * the approver fell back to the dead `department:` literal that routes to - * nobody. That is not an edge case — an app's org tree is normally seeded - * (a seed cannot know the org id the runtime mints at boot) while the - * approval request always carries one, so EVERY department approver a - * designer could pick resolved to nobody. - * - * Widen to "this org ∪ env-wide", the same predicate `sys_metadata`'s - * pending-draft listing settled on for the identical reason. Another org's - * unit still fails the match, so the wall between two organizations is - * unchanged — only rows belonging to no org at all become visible. + * Tenant scope for a `sys_business_unit` read: the request's organization, + * a STRICT equality. + * + * [ADR-0131 D1/D8] This was "this org ∪ env-wide" (#3807): an `$or` with an + * `organization_id = null` arm, because an app's org tree was seeded before + * the runtime minted the organization id, so every seeded unit carried no + * organization and a strict equality resolved every department approver to + * nobody. The organization-ownership ceremony attributes `sys_business_unit` + * before a deployment is certified (D10), and from then on a write with no + * organization is stamped with the derivable one or refused (D9) — a fresh + * install's seed included. No layer keeps a NULL arm (D8), so the unit + * screen is the same equality as the member screen below. + * + * No `organizationId` (a request with no directory organization) keeps the + * unfiltered read: there is no organization to screen by. */ private businessUnitOrgScope( filter: Record, organizationId?: string | null, ): Record { if (!organizationId) return filter; - return { ...filter, $or: [{ organization_id: organizationId }, { organization_id: null }] }; + return { ...filter, organization_id: organizationId }; } /** - * Tenant scope for the `sys_business_unit_member` read — a STRICT equality, - * deliberately NOT {@link businessUnitOrgScope} (#14946). - * - * The two screens answer different questions. The UNIT is the anchor the - * approver NAMES, and a seeded unit carries `organization_id = null` by - * construction (a seed cannot know the id the runtime mints at boot), so - * #3807 admits the null there on purpose. The MEMBER rows are the SET BEING - * ROUTED TO — enumerated by the platform, never named by anyone — and a - * seeded unit id exists identically in every tenant. Before this screen the + * Tenant scope for the `sys_business_unit_member` read — a STRICT equality + * (#14946). + * + * The two screens answer different questions, and both are strict now. The + * UNIT is the anchor the approver NAMES ({@link businessUnitOrgScope}, which + * admitted a NULL organization until ADR-0131 retired that arm). The MEMBER + * rows are the SET BEING ROUTED TO — enumerated by the platform, never named + * by anyone — and a seeded unit id exists identically in every tenant. Before this screen the * member read carried no organization predicate at all, under * {@link SYSTEM_CTX} which carries no tenant either, so tenant A's request * resolved the shared unit and then collected EVERY tenant's membership rows @@ -2794,9 +2790,9 @@ export class ApprovalService implements IApprovalService { * see. That is not silent — the graph-type fallback in `expandApprover` * warns `expanded to nobody` (#3807) and `onEmptyApprovers` governs the * request as for any unstaffed target — and the repair is to stamp the - * membership rows, never to widen this screen. ⛔ Do not "unify" the two - * screens: one method serving both re-opens whichever half it does not - * implement. + * membership rows, never to widen this screen. ⛔ Keep the two screens two + * methods: they guard different reads, and a change to one must not + * silently move the other. */ private businessUnitMemberScope( filter: Record, @@ -2809,9 +2805,8 @@ export class ApprovalService implements IApprovalService { /** * Recursive department — walks `sys_business_unit.parent_business_unit_id`. * - * Two tenant screens, and they are different on purpose: the UNIT rows - * (seed check and descent) go through the null-inclusive - * {@link businessUnitOrgScope}; the MEMBER read goes through the strict + * Two tenant screens, both strict: the UNIT rows (seed check and descent) + * go through {@link businessUnitOrgScope}; the MEMBER read goes through * {@link businessUnitMemberScope}. `organizationId` is the DIRECTORY * organization the approver resolves in (ADR-0105 D9), for both. */ @@ -2851,7 +2846,7 @@ export class ApprovalService implements IApprovalService { rows = await this.engine.find('sys_business_unit_member', { // #14946: tenant-screened — {@link businessUnitMemberScope} is STRICT // on purpose and is not {@link businessUnitOrgScope}. The units above - // proved their tenancy (or are seeded); these rows have not, and the + // proved their tenancy; these rows have not, and the // shared seeded unit id is exactly where other tenants' rows sit. where: this.businessUnitMemberScope({ business_unit_id: { $in: Array.from(seen) } }, organizationId), fields: ['user_id'], diff --git a/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts b/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts index b0b3a1e9946..e079f7e0a3f 100644 --- a/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts +++ b/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts @@ -4,15 +4,15 @@ * organization, with a STRICT equality. * * `expandBusinessUnitUsers` screens the `sys_business_unit` rows with - * `businessUnitOrgScope` — null-inclusive since #3807, because a seeded unit - * carries `organization_id = null` by construction and is admitted on purpose. + * `businessUnitOrgScope` — a strict equality since ADR-0131 D8 retired the + * NULL arm #3807 gave it for seeded units. * The `sys_business_unit_member` read that follows carried NO organization * predicate at all, under `SYSTEM_CTX`, which carries no tenant either. A * seeded unit id exists identically in every tenant, so tenant A's request * resolved that unit and then collected EVERY tenant's membership rows off it. * - * Why the member screen is strict where the unit screen is not — measured on - * this tree, not inherited from the sibling card: + * Why the member screen is strict — measured on this tree, not inherited from + * the sibling card: * * - `sys_business_unit_member` declares no `organization_id` * (`packages/platform-objects/src/identity/sys-business-unit-member.object.ts`); @@ -29,13 +29,13 @@ * tenancy is the same cross-tenant hole by the other door. The screen fails * CLOSED, exactly as `plugin-sharing`'s `memberScope` does for the same rows. * - * Every anchor unit below is SEEDED (`organization_id: null`) unless a case - * says otherwise. That is load-bearing: on an org-stamped unit the assertions - * would hold even if the member screen were deleted, because the unit screen - * would answer first. Anchoring on a seeded unit is what makes each case a pin - * on the MEMBER screen. + * Every anchor unit below belongs to the REQUEST's organization (`org_a`) — + * re-stamped from `organization_id: null` when the unit screen's NULL arm + * retired (ADR-0131 D8), so the unit screen admits it and each case stays a + * pin on the MEMBER screen: the other tenant's membership rows hang off the + * same unit ids, and only the member screen drops them. * - * B1 — THE LEAK: a seeded unit with two tenants' membership rows resolves + * B1 — THE LEAK: a unit with two tenants' membership rows resolves * ONLY the request organization's users, at both depths of the walk. * B2 — THE CONTROL: an org-stamped unit still routes its own members — * strict, not broken — and still drops the other tenant's row. @@ -138,13 +138,13 @@ const CTX_NO_ORG = { userId: 'u_sub', positions: [], permissions: [] } as any; const DEPT_SEEDED = { type: 'department', value: 'bu_seeded' }; /** - * The org chart the card describes: a SEEDED unit tree (`organization_id: - * null`, which `businessUnitOrgScope` admits by design) — the same unit ids - * in every tenant. + * The org chart the card describes, attributed to the request's organization + * as the ownership ceremony leaves a seeded tree (ADR-0131 D10). The ids keep + * their `seeded` spelling: the other tenant's membership rows still name them. */ const SEEDED_TREE = [ - { id: 'bu_seeded', organization_id: null, active: true }, - { id: 'bu_seeded_child', parent_business_unit_id: 'bu_seeded', organization_id: null, active: true }, + { id: 'bu_seeded', organization_id: ORG_A, active: true }, + { id: 'bu_seeded_child', parent_business_unit_id: 'bu_seeded', organization_id: ORG_A, active: true }, ]; /** @@ -249,8 +249,8 @@ describe('#14946 business-unit MEMBER org screen', () => { const where = reads[0].options?.where ?? reads[0].options?.filter ?? {}; console.log('[PROBE B5] sys_business_unit_member where =', JSON.stringify(where)); expect(where.organization_id).toBe(ORG_A); - // ⛔ Not `businessUnitOrgScope`'s `$or: [{organization_id}, {organization_id: null}]`. - // A null arm here would re-admit every org-less row — the B3 population — + // ⛔ No `$or: [{organization_id}, {organization_id: null}]` (the arm the unit + // screen carried until ADR-0131 D8). A null arm here would re-admit every org-less row — the B3 population — // and re-open the hole for the elevated-write case. expect(where).not.toHaveProperty('$or'); expect([...(where.business_unit_id?.$in ?? [])].sort()).toEqual(['bu_seeded', 'bu_seeded_child']); diff --git a/packages/plugins/plugin-sharing/src/rule-criteria-org-scope.test.ts b/packages/plugins/plugin-sharing/src/rule-criteria-org-scope.test.ts index ad0d6df2b2d..f48d3dc6333 100644 --- a/packages/plugins/plugin-sharing/src/rule-criteria-org-scope.test.ts +++ b/packages/plugins/plugin-sharing/src/rule-criteria-org-scope.test.ts @@ -2,7 +2,7 @@ /** * [#10119] A sharing rule's CRITERIA sweep is scoped to the rule's own - * organization — and a platform-global rule's is still not. + * organization — and a declared rule's deployment-wide projection is not. * * ## The defect * @@ -43,27 +43,29 @@ * * A suite that only pinned "an org-stamped rule stops sweeping other orgs" * would stay green against an implementation that scoped EVERY rule — silently - * killing the platform-global sweep that `organization_id = null` rules are - * declared to perform (#7795, documented at the `deleteRule` guard). So each - * org-stamped case is stated beside the null-org case it must not become. + * killing the deployment-wide sweep a DECLARED rule performs under `single`. + * So each org-stamped case is stated beside the deployment-wide case it must + * not become. * - * The NULL-org RECORD is here for the third direction. [ADR-0131 D1, D14] - * NULL is not an owner: `applyTenantScope` emits `field = ?` alone, since 18.x - * retired the v17 `OR field IS NULL` arm after the upgrade ceremony, so an - * org-stamped sweep routed through the chokepoint does NOT reach `deal_p1`, - * while the platform-global sweep, which carries no scope, still does. A sweep - * that kept its own copy of the retired arm would grant it. + * ## [ADR-0131 D2/D8] Deployment-wide is a typed marker, never a NULL * - * ## What `deal_p1` does and does NOT assert (#10103) + * A declared rule is deployment-wide by nature (D2) and carries a typed + * marker on its `single` projection (`deploymentWide`); under a wall it is + * served once per organization, each projection inside that organization's + * one predicate (D8). A STORED row with no organization, in a deployment that + * has organizations, is invalid state (certification leaves none): it sweeps + * nothing and grants nothing, said once, loudly. Only a composition with no + * organization object at all (a lean embedding) has no organization to scope + * by, and there every rule is the deployment's. * - * It used to be justified as the platform bucket: a NULL organization marked a - * platform row that had to stay visible to every tenant (#2734, ADR-0120 D3). - * The ruling on #10103 superseded that reading (a row belonging to no - * organization is INVALID STATE, never a platform-wide default) and kept the - * arm as a driver-level compatibility note; ADR-0131 D14 retired the note with - * the ceremony that removes the rows it served. `deal_p1` stays as the record - * no organization's sweep reaches: a certified database holds no such row, so - * this is the shape a stray one would take, not a population a tenant relies on. + * ## The organization-less RECORD, `deal_p1` + * + * [ADR-0131 D1] NULL is not an owner: the driver's tenant predicate is + * `field = ?` alone, so no sweep scoped to an organization reaches `deal_p1`, + * and a sweep that kept a copy of its own wider predicate would grant it. Only + * the deployment-wide sweep, which carries no tenant, reaches it, and stamps + * nothing on its grant. A certified database holds no such record: this is + * the shape a stray one would take, not a population a tenant relies on. */ import { describe, it, expect, afterEach } from 'vitest'; @@ -73,6 +75,7 @@ import type { ExecutionContext } from '@objectstack/spec/kernel'; import { SharingService } from './sharing-service.js'; import { SharingRuleService } from './sharing-rule-service.js'; +import type { DeclaredSharingRule } from './declared-sharing-rules.js'; const OBJECT = 'os10119_deal'; @@ -141,6 +144,30 @@ interface Booted { rules: SharingRuleService; /** Every `sys_record_share` row in the database, read straight off the driver. */ sharedRecordIds: () => Promise; + /** `[record_id, organization_id]` of every `sys_record_share` row, sorted. */ + grantsAtRest: () => Promise>; + /** What the rule service logged at `error`. */ + errors: string[]; +} + +/** A DECLARED rule, as the registry serves it: `won` deals to `u_decl`. */ +const DECLARED: DeclaredSharingRule = { + name: 'os10119_declared_won', + label: 'Declared won deals', + description: null, + object: OBJECT, + criteria: { stage: 'won' }, + recipientType: 'user', + recipientId: 'u_decl', + accessLevel: 'read', + active: true, +}; + +interface BootOptions { + /** Register `sys_organization`: a deployment WITH organizations, not a lean embedding. */ + organizations?: boolean; + /** Declared rules the registry serves. */ + declared?: DeclaredSharingRule[]; } const open: SqlDriver[] = []; @@ -155,9 +182,9 @@ const open: SqlDriver[] = []; * deal_b2 ORG_B stage=won <- second one, so a count cannot pass by luck * deal_p1 (null) stage=won <- organization-less row: no org-stamped * sweep reaches it (ADR-0131 D1), the - * platform-global one does — see the header. + * deployment-wide one does — see the header. */ -async function boot(posture?: 'single' | 'isolated' | 'group'): Promise { +async function boot(posture?: 'single' | 'isolated' | 'group', opts: BootOptions = {}): Promise { const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, @@ -187,6 +214,14 @@ async function boot(posture?: 'single' | 'isolated' | 'group'): Promise isSystem: true, fields: RULE_FIELDS, } as never); + if (opts.organizations) { + ql.registerObject({ + name: 'sys_organization', + label: 'Organization', + isSystem: true, + fields: { id: { type: 'text', name: 'id', label: 'Id', primary: true }, name: { type: 'text', name: 'name', label: 'Name' } }, + } as never); + } await driver.initObjects([ { name: OBJECT, fields: DEAL_FIELDS } as never, { name: 'sys_record_share', fields: SHARE_FIELDS } as never, @@ -203,16 +238,29 @@ async function boot(posture?: 'single' | 'isolated' | 'group'): Promise await driver.create(OBJECT, { id: 'deal_p1', stage: 'won', owner_id: 'u_p' } as never); const sharing = new SharingService({ engine: ql as never }); - const rules = new SharingRuleService({ engine: ql as never, sharing }); + const errors: string[] = []; + const rules = new SharingRuleService({ + engine: ql as never, + sharing, + ...(opts.declared ? { declaredRules: { list: async () => opts.declared! } } : {}), + logger: { warn: () => {}, error: (m: unknown) => { errors.push(String(m)); } }, + }); return { driver, ql, rules, + errors, sharedRecordIds: async () => { const rows = await driver.find('sys_record_share', {} as never); return rows.map((r: any) => String(r.record_id)).sort(); }, + grantsAtRest: async () => { + const rows = await driver.find('sys_record_share', {} as never); + return rows + .map((r: any): [string, unknown] => [String(r.record_id), r.organization_id ?? null]) + .sort((a, b) => (a[0] < b[0] ? -1 : a[0] > b[0] ? 1 : 0)); + }, }; } @@ -261,15 +309,70 @@ describe('[#10119] sharing-rule criteria sweep is scoped to the rule\'s organiza expect(result.matchedRecords).toBe(1); }); - it('a NULL-ORG (platform-global) rule still sweeps EVERY organization', async () => { - const { rules, sharedRecordIds } = await boot(); + it('a DECLARED rule under `single` is deployment-wide: it sweeps every organization, each grant its record\'s', async () => { + // [ADR-0131 D2/D8] The typed marker's sweep carries no tenant — the + // predicate D8 computes under `single` is none — and each grant is + // stamped with the organization of the record it shares. + const { rules, grantsAtRest } = await boot(undefined, { declared: [DECLARED] }); + + const result = await rules.evaluateRule(DECLARED.name, SYSTEM); + + expect(result.matchedRecords).toBe(4); + expect(await grantsAtRest()).toEqual([ + ['deal_a1', ORG_A], + ['deal_b1', ORG_B], + ['deal_b2', ORG_B], + ['deal_p1', null], + ]); + }); + + it('a DECLARED rule under a wall is served per organization: each projection sweeps only its own', async () => { + // [ADR-0131 D8] No projection sweeps the deployment under a wall: one + // per organization, each inside that organization's one predicate, its + // grants stamped with it. No organization's projection reaches the + // organization-less RECORD `deal_p1` (ADR-0131 D1, see the header). + const { rules, grantsAtRest } = await boot('isolated', { declared: [DECLARED] }); + rules.setDeclaredRuleScopes([ORG_A, ORG_B]); + + await rules.evaluateRule(DECLARED.name, SYSTEM); + + expect(await grantsAtRest()).toEqual([ + ['deal_a1', ORG_A], + ['deal_b1', ORG_B], + ['deal_b2', ORG_B], + ]); + }); + + it('a STORED rule with NO organization, in a deployment with organizations, sweeps nothing and says so once', async () => { + // Invalid state — certification leaves no such row, and the engine + // stamps or refuses every new one (D9) — so it is planted through the + // driver. ⛔ It must not become the deployment-wide sweep: that is the + // organization-less branch the marker replaced. + const { driver, rules, sharedRecordIds, errors } = await boot(undefined, { organizations: true }); + await driver.create('sys_sharing_rule', { + id: 'srule_orgless', name: 'os10119_orgless_won', label: 'Org-less won deals', object_name: OBJECT, + criteria_json: JSON.stringify(WON), recipient_type: 'user', recipient_id: 'u_plat', access_level: 'read', + active: true, managed_by: 'admin', customized: false, + } as never); + + const result = await rules.evaluateRule('srule_orgless', SYSTEM); + await rules.evaluateRule('srule_orgless', SYSTEM); + + expect(result.matchedRecords).toBe(0); + expect(result.grantsCreated).toBe(0); + expect(await sharedRecordIds()).toEqual([]); + expect(errors.filter((m) => m.includes("'os10119_orgless_won' is a stored row with no organization"))).toHaveLength(1); + }); + + it('CONTROL: a composition with NO organization object (a lean embedding) sweeps the deployment for that row', async () => { + // The one composition with no organization to scope by + // (`no-organization-object`): there every rule is the deployment's. + const { rules, sharedRecordIds, errors } = await boot(); - // Defined under the system context, which is what stamps - // `organization_id: null` — the platform-global class (#7795). const rule = await rules.defineRule( { - name: 'os10119_platform_won', - label: 'Platform won deals', + name: 'os10119_lean_won', + label: 'Lean won deals', object: OBJECT, criteria: WON, recipientType: 'user', @@ -278,15 +381,11 @@ describe('[#10119] sharing-rule criteria sweep is scoped to the rule\'s organiza } as never, SYSTEM, ); - expect(rule.organization_id).toBeNull(); const result = await rules.evaluateRule(rule.id, SYSTEM); - const granted = await sharedRecordIds(); - - // The declared platform-global behaviour, pinned so scoping the - // org-stamped case above cannot quietly take it away. - expect(granted).toEqual(['deal_a1', 'deal_b1', 'deal_b2', 'deal_p1']); expect(result.matchedRecords).toBe(4); + expect(await sharedRecordIds()).toEqual(['deal_a1', 'deal_b1', 'deal_b2', 'deal_p1']); + expect(errors).toEqual([]); }); }); @@ -317,27 +416,29 @@ describe('[#10119] sharing-rule criteria sweep is scoped to the rule\'s organiza expect(await sharedRecordIds()).toEqual([]); }); - it('a NULL-ORG rule still matches every organization\'s record', async () => { - const { rules, sharedRecordIds } = await boot(); - - await rules.defineRule( - { - name: 'os10119_platform_won_hook', - label: 'Platform won deals', - object: OBJECT, - criteria: WON, - recipientType: 'user', - recipientId: 'u_plat', - accessLevel: 'read', - } as never, - SYSTEM, - ); + it('a DECLARED rule under `single` matches every organization\'s record', async () => { + const { rules, grantsAtRest } = await boot(undefined, { declared: [DECLARED] }); const results = await rules.evaluateAllForRecord(OBJECT, 'deal_b1', SYSTEM); expect(results).toHaveLength(1); expect(results[0]!.grantsCreated).toBe(1); - expect(await sharedRecordIds()).toEqual(['deal_b1']); + expect(await grantsAtRest()).toEqual([['deal_b1', ORG_B]]); + }); + + it('a STORED rule with NO organization matches no record where the deployment has organizations', async () => { + const { driver, rules, sharedRecordIds } = await boot(undefined, { organizations: true }); + await driver.create('sys_sharing_rule', { + id: 'srule_orgless_hook', name: 'os10119_orgless_won_hook', label: 'Org-less won deals', object_name: OBJECT, + criteria_json: JSON.stringify(WON), recipient_type: 'user', recipient_id: 'u_plat', access_level: 'read', + active: true, managed_by: 'admin', customized: false, + } as never); + + const results = await rules.evaluateAllForRecord(OBJECT, 'deal_b1', SYSTEM); + + expect(results).toHaveLength(1); + expect(results[0]!.grantsCreated).toBe(0); + expect(await sharedRecordIds()).toEqual([]); }); }); }); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-positions-name-authority.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule-positions-name-authority.test.ts index fe3ca93f7eb..176364bce3c 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-positions-name-authority.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-positions-name-authority.test.ts @@ -30,7 +30,8 @@ * expose every tenant's rules". The D4 name-read was the bypass: it satisfied * that gate, `adminOrgScope` then returned the UNFILTERED `where`, and * `listRules` answered with every organization's rows. The same read also - * carried `deleteRule` past `assertCanDeletePlatformGlobalRule`. + * carried `deleteRule` past the platform-global delete guard, which retired + * with that rule class (ADR-0131 D1/D2); the scope gate is what refuses it now. * * ## POPULATION OF THIS PIN — stated because a pin proves only what it covers * @@ -158,7 +159,7 @@ async function resolve(shape: 'name-only' | 'genuine') { return { context, grants, rung: await hasPlatformAdminStanding(ql as any, USER) }; } -/** Two tenants' rules plus one platform-global row, so a cross-tenant read is VISIBLE. */ +/** Two tenants' rules plus one organization-less row, so a cross-tenant read is VISIBLE. */ const RULE_ROWS = [ { id: 'srule_home', name: 'home_rule', object_name: 'account', organization_id: HOME_ORG, active: true }, { id: 'srule_victim', name: 'victim_rule', object_name: 'account', organization_id: OTHER_ORG, active: true }, @@ -244,7 +245,7 @@ describe('[#15981] a D4 `sys_user_position` row spelling `platform_admin` confer expect(engine.finds).toEqual([]); }); - it('`deleteRule` refuses to destroy a platform-global rule', async () => { + it('`deleteRule` refuses the org-less caller before it reaches any rule — the organization-less row included', async () => { const { context } = await resolve('name-only'); const engine = makeSharingEngine(); const service = makeService(engine); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 44e4b2b9ea4..26f18e1e7c3 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -57,6 +57,9 @@ import type { DeclaredSharingRule, DeclaredSharingRuleSource } from './declared- */ const SYSTEM_CTX: ExecutionContext = { isSystem: true, positions: [], permissions: [] }; +/** The organization object, whose registration tells a lean embedding from a deployment with organizations. */ +const ORGANIZATION_OBJECT = 'sys_organization'; + /** * [ADR-0131 D6 regime C] The `sys_metadata_activation.metadata_type` a declared * sharing rule's switch is recorded under — its registry type. @@ -143,11 +146,55 @@ function rowFromRule(row: any): SharingRuleRow { }; } +/** + * [ADR-0131 D2/D8] The one scope a declared rule is served in under `single`: + * the deployment. A typed marker, never an organization id and never NULL — + * "no organization" is not a state (D1), and no layer has a NULL arm (D8). + */ +const DEPLOYMENT_WIDE_SCOPE: unique symbol = Symbol('sharing-rule.deployment-wide'); + +/** [ADR-0131 D2] Where a declared rule is served: the deployment, or one organization. */ +type DeclaredRuleScope = string | typeof DEPLOYMENT_WIDE_SCOPE; + +/** + * [ADR-0131 D2] A declared rule's projection that sweeps the deployment — the + * typed marker that replaced the organization-less (`organization_id: null`) + * projection, in the shape of the declared webhooks' `deploymentWide` (the + * epic's ruling A on the walled shape of a declared webhook, record + * 6278609968). It carries no `organization_id` at all. The marker itself is + * declared on the spec contract (`SharingRuleRow.deploymentWide`); this type + * narrows it to present. + */ +export interface DeploymentWideSharingRule extends SharingRuleRow { + readonly deploymentWide: true; +} + +/** + * Does `rule` sweep the whole deployment? Only a declared rule's `single` + * projection does ({@link projectDeclaredRule}); a stored row is never one, + * because {@link rowFromRule} builds its fields one by one and the marker is + * not a column. + */ +export function isDeploymentWideSharingRule(rule: SharingRuleRow): rule is DeploymentWideSharingRule { + return (rule as Partial).deploymentWide === true && isDeclaredSharingRule(rule); +} + /** * [ADR-0131 D2] A DECLARED rule as the rule set serves it: the declaration * (read from the registry, {@link DeclaredSharingRuleSource}) projected into - * the runtime row shape for ONE scope — `null` under `single`, one - * organization under a wall. It is not a row: nothing is written for it. + * the runtime row shape for ONE scope. It is not a row: nothing is written for + * it. + * + * A declaration is deployment-wide under every posture — every organization + * runs the same code (D2). How it is SERVED follows the wall: + * + * - under `single`, ONE projection marked {@link DeploymentWideSharingRule}: + * its criteria sweep carries no tenant, which is the predicate D8 computes + * there (none), and each grant is stamped with its record's organization; + * - under a wall, one projection PER ORGANIZATION, stamped with it: the sweep + * runs inside that organization's one predicate, recipients expand inside + * it, and its grants carry it. A deployment-wide sweep under a wall would + * be a second predicate beside Layer 0's, which D8 forbids. * * Its `id` is its NAME. That is what its grants carry as * `sys_record_share.source_id` (ruling β item 5 (c) on the C3 epic: the @@ -156,10 +203,10 @@ function rowFromRule(row: any): SharingRuleRow { * reconcile below reads and writes a declared rule's grants by name with no * second code path. An organization-authored rule keeps its row id. */ -function projectDeclaredRule(rule: DeclaredSharingRule, organizationId: string | null): SharingRuleRow { +function projectDeclaredRule(rule: DeclaredSharingRule, scope: DeclaredRuleScope): SharingRuleRow { return { id: rule.name, - organization_id: organizationId, + ...(scope === DEPLOYMENT_WIDE_SCOPE ? { deploymentWide: true } : { organization_id: scope }), name: rule.name, label: rule.label, description: rule.description, @@ -310,7 +357,7 @@ export function bindSharingRuleActivationReconcile( */ function ruleGrantScope(rule: SharingRuleRow): Record { const where: Record = { source: 'rule', source_id: rule.id }; - if (isDeclaredSharingRule(rule) && rule.organization_id != null) where.organization_id = rule.organization_id; + if (isDeclaredSharingRule(rule) && !isDeploymentWideSharingRule(rule)) where.organization_id = rule.organization_id; return where; } @@ -453,9 +500,9 @@ export class SharingRuleService implements ISharingRuleService { /** [ADR-0131 D2] Where declared rules are read. */ private readonly declaredSource?: DeclaredSharingRuleSource; /** - * [ADR-0131 D2] The scopes a declared rule is served in: `[null]` under - * `single` (one organization-less projection, the platform-global class), - * every organization under a wall. Set by the plugin from the posture + * [ADR-0131 D2] The scopes a declared rule is served in: the deployment + * under `single` (one {@link DeploymentWideSharingRule} projection), every + * organization under a wall. Set by the plugin from the posture * ({@link setDeclaredRuleScopes}). * * `null` is UNKNOWN — the organizations could not be read @@ -464,11 +511,13 @@ export class SharingRuleService implements ISharingRuleService { * rule", and the orphaned-grant sweep would revoke every declared rule's * grants on that reading. Unknown serves no declared rule and revokes none. */ - private declaredScopes: ReadonlyArray | null = [null]; + private declaredScopes: ReadonlyArray | null = [DEPLOYMENT_WIDE_SCOPE]; /** [ADR-0131 D2] The held-back sweep under unknown scopes says so once per process. */ private unknownScopesSweepWarned = false; /** [ADR-0131 D6 regime C] The declaration-over-row conflicts already reported, `name::scope`. */ private readonly declarationOverRowReported = new Set(); + /** [ADR-0131 D1/D8] Stored rules refused for carrying no organization, reported once each. */ + private readonly organizationlessRuleReported = new Set(); constructor(opts: SharingRuleServiceOptions) { this.engine = opts.engine; @@ -478,13 +527,17 @@ export class SharingRuleService implements ISharingRuleService { } /** - * [ADR-0131 D2] Serve declared rules in these scopes — `[null]` under + * [ADR-0131 D2] Serve declared rules in these scopes — the deployment under * `single`, the organizations under a wall. The same passes the retired - * seeder made, as reads instead of rows: one organization-less pass under + * seeder made, as reads instead of rows: one deployment-wide pass under * `single`, one per organization under a wall (#10103). + * + * `null` is how the plugin's posture read (`resolveDeclaredRuleScopes`) + * spells `single`. It is translated here, at the one door, into the + * deployment-wide scope, so no NULL travels past this line. */ setDeclaredRuleScopes(scopes: ReadonlyArray): void { - this.declaredScopes = [...new Set(scopes)]; + this.declaredScopes = [...new Set(scopes.map((s): DeclaredRuleScope => (s === null ? DEPLOYMENT_WIDE_SCOPE : s)))]; } /** @@ -506,7 +559,9 @@ export class SharingRuleService implements ISharingRuleService { */ addDeclaredRuleScope(organizationId: string): void { const scopes = this.declaredScopes; - if (!organizationId || scopes === null || scopes.includes(null) || scopes.includes(organizationId)) return; + if (!organizationId || scopes === null || scopes.includes(DEPLOYMENT_WIDE_SCOPE) || scopes.includes(organizationId)) { + return; + } this.declaredScopes = [...scopes, organizationId]; } @@ -559,16 +614,18 @@ export class SharingRuleService implements ISharingRuleService { /** * [#8158] PLATFORM authority — the two spellings, one predicate. * - * Extracted from {@link assertCanDeletePlatformGlobalRule} (#7795), whose - * doc block is the authority on WHY both spellings are accepted and why - * accepting either is the fail-safe reading: they are two independent - * channels by which the same unscoped `admin_full_access` grant reaches an + * Both spellings are accepted because they are two independent channels by + * which the same unscoped `admin_full_access` grant reaches an * `ExecutionContext` (a `scope: 'platform'` capability on - * `systemPermissions`; the ADR-0095 posture RUNG). + * `systemPermissions`; the ADR-0095 posture RUNG): a context built by the + * shared authz resolver carries both, a hand-built one may carry only one, + * so accepting either is the fail-safe reading. * - * It is asked TWICE now — once to authorize destroying a platform-global - * rule, once to decide whether an org-less caller may read across tenants — - * so it is one predicate rather than two spellings that can drift apart. + * It decides whether an org-less caller may read across tenants. Its other + * question — may this caller DELETE a platform-global (`organization_id = + * null`) rule (#7795) — retired with that rule class (ADR-0131 D1/D2): a + * stored rule belongs to the organization that authored it, and a declared + * rule is code, which no caller deletes. * * ⛔ [#15981] The second spelling is the RUNG, never * `positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN)`. The two stopped @@ -640,8 +697,8 @@ export class SharingRuleService implements ISharingRuleService { * * Asserted in {@link assertCanManageRules}, which every verb already calls * first — so `defineRule` is covered too, and an org-less caller can no - * longer mint an `organization_id: null` rule (a platform-global one, whose - * grants reach every tenant and which #7795 then forbids them to delete). + * longer write a rule with no organization of its own (the engine stamps or + * refuses such a write, ADR-0131 D9). * * ## Two classes keep the unfiltered read, deliberately * @@ -671,110 +728,6 @@ export class SharingRuleService implements ISharingRuleService { ); } - /** - * [#7795] DELETING a platform-global (`organization_id = null`) rule requires - * PLATFORM authority. Org-scoped `manage_sharing` does not authorize it. - * - * Maintainer ruling, 2026-08-12 (方向 B), quoted verbatim and untranslated: - * - * > **裁定:方向 B —— read/evaluate 保持开放,delete 需要平台级权限。** - * > - * > - `deleteRule` 对 `organization_id = null` 的行,要求调用者持有平台级权限 - * > (`manage_platform_settings` 或 `platform_admin` 位置);仅持 org 级 - * > `manage_sharing` 者拒绝。 - * > - 错误面用 **403 `PERMISSION_DENIED`**,不是 404 —— 该行是有意可见的, - * > 404 会撒谎。 - * > - #7760 开放的能力(列出、查看、评估种子规则)全部保持不动。 - * - * ## Why only DELETE, and only this row class - * - * `manage_sharing` is declared `scope: 'org'` in the spec's capability - * registry, but a null-org rule belongs to no organization: its criteria - * query runs unscoped under {@link SYSTEM_CTX}, so {@link deleteRule}'s - * grant purge revokes EVERY tenant's `sys_record_share` rows, not just the - * caller's. That is the one act on this surface an org-level capability - * should not reach, and the two measurements the ruling rests on say why — - * both re-verified against this build before the guard was written: - * - * 1. **The delete is a revocation wearing removal's clothes.** - * `bootstrapDeclaredSharingRules` re-seeds declared rules on every boot, - * and {@link defineRule}'s existence lookup under a null org is `{name}` — - * which matches nothing once the row is deleted, so the insert branch - * mints a fresh `uid('srule')`. Measured: the rule returns after a - * restart under a DIFFERENT id, with its grants re-materialised. The - * profile of an outage, not of an administrative change. - * 2. **The safe lever is unavailable while the destructive one is not.** - * An org admin cannot deactivate this row: `defineRule`'s existence - * lookup is deliberately strict (`{name, organization_id: orgId}`, held - * that way by #7676 so one org cannot upsert over a row other orgs read), - * so `active: false` from an org admin creates a SECOND, org-stamped row - * and leaves the shared one running. Measured: two rows, the null-org one - * still `active: true`. Scoped + reversible refused, cross-tenant + - * irreversible-until-reboot permitted — the inverse of the safe - * arrangement, and closing the destructive lever is the structural fix. - * - * ## Two spellings of platform authority, and why BOTH are accepted - * - * They are not synonyms — they are two independent channels by which the - * SAME underlying grant (an unscoped `admin_full_access`) reaches an - * `ExecutionContext`: - * - * - `manage_platform_settings` — a `scope: 'platform'` CAPABILITY, arriving - * on `context.systemPermissions`. `admin_full_access` carries it; - * `organization_admin` deliberately withholds it (it gets only - * `manage_org_users` / `setup.access` / `setup.write`), which is exactly - * what makes it a discriminator between a platform operator and a tenant - * admin — the same reasoning plugin-security's - * `PLATFORM_ADMIN_ONLY_CAPABILITIES` probe encodes. - * - `platform_admin` — a built-in POSITION (ADR-0068 D2), arriving on - * `context.positions`, DERIVED by the shared resolver from the unscoped - * `admin_full_access` user grant (never a stored boolean). - * - * A context built by the shared authz resolver carries both. A HAND-BUILT - * context — the population ADR-0096 D3 is still eliminating, and which - * plugin-security's probe comment names the sharing service as part of — - * may carry only one. Accepting either is therefore the fail-safe reading of - * a ruling that names both, and refusing on the absence of both cannot - * silently over-refuse a genuine platform operator. - * - * ## The error surface is 403 `PERMISSION_DENIED`, deliberately not 404 - * - * The row is DELIBERATELY visible: #7760 opened listing, reading and - * evaluating seeded rules to org admins on purpose, and this guard leaves - * all three untouched. A 404 here would be the platform lying about a row - * the caller could list and read one call earlier. The message prefix is - * what `rest-server.ts`'s sharing-rule `handleError` maps to HTTP 403 + - * `{code: 'PERMISSION_DENIED'}`, which is also the pairing the spec's own - * `HttpStatusErrorCodeMap[403]` records. ⛔ Do not "harden" this into a 404 - * or a silent no-op — both re-open the lie this shape exists to avoid. - * - * Placed AFTER {@link getRule} resolves, so a row the caller cannot see at - * all keeps its existing silent-no-op behaviour rather than gaining a new - * refusal that would disclose the row's existence. - * - * ⚠️ Recorded consequence, accepted by the ruling: with delete closed, an org - * admin has NO lever at all over a platform-global rule. The ruling - * explicitly declines to pre-build a per-org suppression mechanism - * (「⛔ 不做 D」) absent measured demand — do not add one here. - */ - private assertCanDeletePlatformGlobalRule(row: SharingRuleRow, context: ExecutionContext): void { - // Only the platform-global class is gated — an org's own rows are - // untouched, and so is every read verb. - if (row.organization_id != null) return; - // Boot seeding, hooks, backfills and the plugin machinery, as everywhere else. - if (context?.isSystem) return; - // [#8158] Both spellings, now through the shared predicate — see - // {@link hasPlatformAuthority}, which carries this block's reasoning. - if (this.hasPlatformAuthority(context)) return; - throw new Error( - 'PERMISSION_DENIED: deleting a platform-global sharing rule requires platform authority — ' + - 'the manage_platform_settings capability or the platform_admin position. Org-scoped ' + - 'manage_sharing does not authorize it, because this rule belongs to no organization and ' + - 'deleting it revokes every tenant’s grants under it. It remains listable, ' + - 'readable and evaluable.', - ); - } - async defineRule(input: DefineSharingRuleInput, context: ExecutionContext): Promise { this.assertCanManageRules(context); if (!input.name) throw new Error('VALIDATION_FAILED: name is required'); @@ -811,11 +764,11 @@ export class SharingRuleService implements ISharingRuleService { } // [#7136 / #8158] One spelling of this read for the whole service — see - // {@link callerOrgId}. `null` (not `undefined`) is what a null-org row is - // STAMPED with, and only a system context or a platform operator reaches - // this line without an org id (`assertCanManageRules` above refuses the - // authenticated org-less caller, so a `manage_sharing` holder can no - // longer mint a platform-global rule by accident). + // {@link callerOrgId}. Only a system context or a platform operator + // reaches this line without an org id (`assertCanManageRules` above + // refuses the authenticated org-less caller). Their insert carries no + // organization, and the engine stamps it with the derivable one under + // `single` or refuses it under a wall (ADR-0131 D9) — it never lands NULL. const orgId = this.callerOrgId(context) ?? null; const now = new Date().toISOString(); // Authoring path — `full` normalises to `edit`, anything unrecognised is a @@ -903,41 +856,26 @@ export class SharingRuleService implements ISharingRuleService { } /** - * [#7676] Tenant scope for a sharing-rule ADMIN read: "this org ∪ - * platform-global". - * - * `organization_id = null` on `sys_sharing_rule` means "owned by no - * organization" — a row the package/app seeder wrote - * (`bootstrapDeclaredSharingRules`, which defined under `SYSTEM_CTX` and so - * stamped `organization_id: null`) before any org id existed, or, since that - * seeder retired (ADR-0131 D2), a declared rule's `single`-posture - * projection ({@link projectDeclaredRule}). A strict - * `organization_id = ` equality made every such row invisible to - * the admin API while enforcement kept reading them under `SYSTEM_CTX`: on a - * stock boot `GET /api/v1/sharing/rules` answered `{data: []}` over four - * active seeded rules, by-name GET and evaluate 404'd `RULE_NOT_FOUND`, and - * only the org-unfiltered by-id branch of {@link getRule} still worked. Rules - * that grant access but cannot be listed, inspected or deactivated are the - * worst half of both properties. - * - * Widening the READ leaks nothing across tenants: another org's row still - * fails the match, and a null-org row is platform-global by construction — - * every org already receives the grants it materialises. This is the same - * predicate, for the same reason, that `sys_business_unit` approver expansion - * settled on in #3807 and that `sys_metadata`'s pending-draft listing uses. - * - * ⚠️ It is deliberately NOT applied to {@link defineRule}'s existence lookup. - * That lookup decides UPSERT-or-insert, so widening it would let one org's - * admin overwrite the label, criteria, recipient and access level of a row - * every OTHER org reads — a cross-tenant WRITE, which is a different act from - * a cross-tenant read of a platform-global row. A same-named POST therefore - * still creates an org-stamped row of the tenant's own, and - * {@link findRuleRowByName} prefers it. - * - * [#7761] It IS applied to {@link getRule}'s by-**id** branch, which the - * second paragraph above records as the one read that still worked while - * everything else was over-scoped. That was never a feature: unfiltered - * meant an org admin could resolve — and, through {@link deleteRule}, + * Tenant scope for a sharing-rule ADMIN read: the caller's OWN organization. + * + * [ADR-0131 D1/D8] This was "this org ∪ platform-global" (#7676): a + * `$or` with an `organization_id = null` arm, because the retired boot + * seeder (`bootstrapDeclaredSharingRules`) defined declared rules under the + * system context and so stamped them with no organization, and a strict + * equality made those rows invisible to the admin API while enforcement + * kept reading them. Both halves of that reason are gone: declared rules are + * read from the registry and never written (D2, {@link declaredRules}, which + * serves them beside the rows below), and the organization-ownership + * ceremony attributes every stored row before the deployment is certified + * (D10), after which a write with no organization is stamped or refused + * (D9). No layer keeps a NULL arm (D8), so this one is an equality. + * + * ⚠️ {@link defineRule}'s existence lookup is the same equality, and must + * stay one: it decides UPSERT-or-insert, so a wider lookup would let one + * organization's admin overwrite a row another organization reads. + * + * [#7761] It IS applied to {@link getRule}'s by-**id** branch. Unfiltered, + * that branch let an org admin resolve — and, through {@link deleteRule}, * destroy — another organization's rule from its id alone. * * [#8158] Takes the CONTEXT, not a bare org id. The unfiltered branch is @@ -952,7 +890,7 @@ export class SharingRuleService implements ISharingRuleService { private adminOrgScope(where: Record, context: ExecutionContext): Record { const orgId = this.callerOrgId(context); if (!orgId) return where; // system context / platform operator — asserted upstream - return { ...where, $or: [{ organization_id: orgId }, { organization_id: null }] }; + return { ...where, organization_id: orgId }; } async listRules( @@ -1008,7 +946,7 @@ export class SharingRuleService implements ISharingRuleService { if (off.size > 0) rules = rules.filter((r) => !off.has(r.name)); if (rules.length === 0) return []; - const single = this.declaredScopes?.includes(null) === true; + const single = this.declaredScopes?.includes(DEPLOYMENT_WIDE_SCOPE) === true; const holders = await this.engine.find('sys_sharing_rule', { where: { name: { $in: rules.map((r) => r.name) } }, fields: ['name', 'organization_id'], @@ -1017,12 +955,12 @@ export class SharingRuleService implements ISharingRuleService { }); const held = new Set(); for (const row of (Array.isArray(holders) ? holders : []) as any[]) { - held.add(single ? String(row.name) : `${String(row.name)}::${String(row.organization_id ?? '')}`); + held.add(single ? String(row.name) : `${String(row.name)}::${String(row.organization_id)}`); } const out: SharingRuleRow[] = []; for (const scope of scopes) { for (const rule of rules) { - if (held.has(single ? rule.name : `${rule.name}::${scope}`)) { + if (held.has(single ? rule.name : `${rule.name}::${String(scope)}`)) { this.reportDeclarationOverAuthoredRow(rule, scope); continue; } @@ -1042,17 +980,18 @@ export class SharingRuleService implements ISharingRuleService { * re-grant what a customised copy switched off — and that is said loudly, * once per name and scope per process, with the remedy. */ - private reportDeclarationOverAuthoredRow(rule: DeclaredSharingRule, scope: string | null): void { - const key = `${rule.name}::${scope ?? ''}`; + private reportDeclarationOverAuthoredRow(rule: DeclaredSharingRule, scope: DeclaredRuleScope): void { + const organization = scope === DEPLOYMENT_WIDE_SCOPE ? undefined : scope; + const key = `${rule.name}::${organization ?? ''}`; if (this.declarationOverRowReported.has(key)) return; this.declarationOverRowReported.add(key); const message = `[sharing-rule] declared sharing rule '${rule.name}' arrives over a stored sys_sharing_rule row of the same ` + - `name${scope ? ` in organization '${scope}'` : ''}, so the declared rule is NOT in force there and grants nothing ` + + `name${organization ? ` in organization '${organization}'` : ''}, so the declared rule is NOT in force there and grants nothing ` + 'while the row keeps answering as its own rule. Remedy: rename the stored rule, which makes it a clone of its ' + `own, and switch the declared rule off (POST /api/v1/security/_activation/sharing_rule/${rule.name} with ` + '{ "enabled": false }) if it must not grant.'; - const meta = { rule: rule.name, organization: scope }; + const meta = organization ? { rule: rule.name, organization } : { rule: rule.name, deploymentWide: true }; if (this.logger?.error) this.logger.error(message, meta); else this.logger?.warn?.(message, meta); } @@ -1088,16 +1027,15 @@ export class SharingRuleService implements ISharingRuleService { } /** - * [ADR-0131 D2] Which scopes a caller reads declared rules in: the one - * organization-less scope under `single` (platform-global, every - * organization reads it — the {@link adminOrgScope} reasoning); under a wall, - * the caller's own organization, or every organization for a system context - * or a platform operator. + * [ADR-0131 D2] Which scopes a caller reads declared rules in: the + * deployment under `single` (every organization reads the one + * deployment-wide projection); under a wall, the caller's own organization, + * or every organization for a system context or a platform operator. */ - private declaredScopesFor(context: ExecutionContext): ReadonlyArray { + private declaredScopesFor(context: ExecutionContext): ReadonlyArray { const scopes = this.declaredScopes; if (scopes === null) return []; // unknown: served nowhere - if (scopes.includes(null)) return [null]; + if (scopes.includes(DEPLOYMENT_WIDE_SCOPE)) return [DEPLOYMENT_WIDE_SCOPE]; const orgId = this.callerOrgId(context); if (!orgId) return scopes; return scopes.includes(orgId) ? [orgId] : []; @@ -1174,8 +1112,7 @@ export class SharingRuleService implements ISharingRuleService { // `manage_sharing` is an org-level capability (`scope: 'org'` in the spec's // capability registry) and an id is not a tenant boundary: ids leak through // logs, exports, support tickets and the evaluate response's `{ruleId}`. - // A platform-global (`organization_id = null`) row stays reachable, for - // symmetry with the by-name path — see {@link adminOrgScope}. + // Own organization only, like the by-name path — see {@link adminOrgScope}. const byId = await this.engine.find('sys_sharing_rule', { where: this.adminOrgScope({ id: idOrName }, context), limit: 1, @@ -1188,15 +1125,15 @@ export class SharingRuleService implements ISharingRuleService { } /** - * [#7676] Resolve a rule by NAME for an admin read: this org first, the - * platform-global (`organization_id IS NULL`) row second. + * Resolve a rule by NAME for an admin read: the caller's own organization's + * row of that name. * - * Two sequenced lookups rather than one `$or` with `limit: 1`, because when - * BOTH rows exist the answer must be the caller's own: a single disjunctive - * query with a row cap picks whichever row the driver happened to reach - * first, so an org that had authored its own `share_red_projects_with_execs` - * could get the platform row back on one dialect and its own on another. - * Preference is a decision, so it is written as one. + * [ADR-0131 D1/D8] It used to fall back to the platform-global + * (`organization_id IS NULL`) row of that name (#7676) — the seeded copy of + * a declared rule. Declared rules are read from the registry now + * ({@link getRule} resolves them by name after this lookup misses), and a + * certified deployment holds no organization-less row, so the fallback is + * gone: the one row this caller can name is its organization's. * * No `orgId` (SYSTEM_CTX — boot seeding, hooks, backfills) keeps the * unfiltered by-name lookup it has always had. @@ -1215,7 +1152,7 @@ export class SharingRuleService implements ISharingRuleService { return Array.isArray(rows) && rows[0] ? rows[0] : null; }; if (!orgId) return first({ name }); // system context / platform operator — asserted upstream - return (await first({ name, organization_id: orgId })) ?? (await first({ name, organization_id: null })); + return first({ name, organization_id: orgId }); } async deleteRule(idOrName: string, context: ExecutionContext): Promise { @@ -1236,9 +1173,6 @@ export class SharingRuleService implements ISharingRuleService { } return; } - // [#7795] A platform-global row is visible to an org admin by design - // (#7760) but is NOT theirs to destroy — 403, never 404. - this.assertCanDeletePlatformGlobalRule(row, context); // Drop materialised grants first so we don't orphan them. // // [#4434] This used to be a predicate-shaped `engine.delete` on @@ -1344,13 +1278,14 @@ export class SharingRuleService implements ISharingRuleService { const live = new Set(); for (const r of (ruleRows ?? [])) live.add(String((r as any).id)); // [ADR-0131 D2] A declared rule's grants carry its NAME, and they are live - // exactly where it is served: organization-less (every organization's - // records) under `single`, its organization's own grants under a wall. A - // declaration removed from the code, or answered in an organization by a - // row of its name, leaves grants no projection claims — swept here. + // exactly where it is served: deployment-wide (every organization's + // records — {@link DeploymentWideSharingRule}) under `single`, its + // organization's own grants under a wall. A declaration removed from the + // code, or answered in an organization by a row of its name, leaves grants + // no projection claims — swept here. const liveDeclared = new Set(); for (const rule of await this.declaredRules(SYSTEM_CTX, {})) { - if (rule.organization_id == null) live.add(rule.id); + if (isDeploymentWideSharingRule(rule)) live.add(rule.id); else liveDeclared.add(`${rule.id}::${rule.organization_id}`); } // Unknown scopes: where a declared rule is served is not known, so none of @@ -1383,7 +1318,7 @@ export class SharingRuleService implements ISharingRuleService { // A `source: 'rule'` row with no `source_id` names no rule that could // ever re-grant it — equally unreachable, equally void. if (sourceId != null && live.has(String(sourceId))) continue; - if (sourceId != null && liveDeclared.has(`${String(sourceId)}::${String((g as any).organization_id ?? '')}`)) continue; + if (sourceId != null && liveDeclared.has(`${String(sourceId)}::${String((g as any).organization_id)}`)) continue; await this.sharing.revoke(String((g as any).id), SYSTEM_CTX); revoked += 1; } @@ -1769,7 +1704,8 @@ export class SharingRuleService implements ISharingRuleService { /** * [#10119] The context the rule's CRITERIA query runs under — system - * elevation, plus the rule's OWN organization when it has one. + * elevation, plus the rule's OWN organization when it has one; `null` when + * the rule may sweep nowhere. * * ## What was wrong * @@ -1793,22 +1729,35 @@ export class SharingRuleService implements ISharingRuleService { * * `tenantId` is the platform's ONE spelling of tenant scope on a read: the * engine threads it to `DriverOptions.tenantId` and the driver's own - * chokepoint (`SqlDriver.applyTenantScope`) emits - * `(organization_id = ? OR organization_id IS NULL)`. Open-coding an - * `organization_id` equality into `filter` would be a second, worse copy — - * it would collide with a rule whose criteria already name that column, it - * would not know the object's declared tenant field, and it would drop the - * NULL arm that keeps platform-seeded rows visible to every tenant (#2734). - * - * ## The null-org rule keeps its unscoped sweep - * - * `organization_id = null` means "owned by no organization" — the - * platform-global class documented at - * {@link assertCanDeletePlatformGlobalRule} (#7795), whose whole point is - * that it grants across every tenant. It gets {@link SYSTEM_CTX} unchanged. - * ⛔ Do not "simplify" this into scoping every rule: that silently retires a - * declared behaviour, and a one-directional test suite would not notice. - * Both directions are pinned in `rule-criteria-org-scope.test.ts`. + * chokepoint (`SqlDriver.applyTenantScope`) applies the posture's one + * predicate (ADR-0131 D8). Open-coding an `organization_id` equality into + * `filter` would be a second, worse copy — it would collide with a rule + * whose criteria already name that column, and it would not know the + * object's declared tenant field. + * + * ## [ADR-0131 D2/D8] Deployment-wide is a typed marker, never a NULL + * + * The organization-less branch that answered "no organization ⇒ unscoped + * sweep" is retired. What it served is now one of three named cases: + * + * - **The rule has an organization** — an organization-authored row, or a + * declared rule's projection under a wall: the sweep runs inside that + * organization's one predicate. + * - **A {@link DeploymentWideSharingRule}** — a declared rule under + * `single`: no tenant, which is exactly the predicate D8 computes under + * `single` (none), so Layer 0 is not widened by a single row. Under a wall + * no projection carries the marker ({@link projectDeclaredRule}). + * - **A composition with no organization object** (a lean embedding, + * `resolveSystemWriteOrganization`'s `no-organization-object`): there are + * no organizations to scope by, so every rule is the deployment's. + * + * Anything else is a STORED row with no organization in a deployment that + * has organizations. Certification leaves none and the engine stamps or + * refuses every new one (D9/D10), so it is invalid state: `null` here, and + * the caller sweeps nothing and grants nothing ({@link sweepContextOrReport} + * says so once). ⛔ Never default it to the deployment-wide sweep — that is + * the NULL arm this replaced, and it is how one row came to grant across + * every organization. * * `isSystem` is retained either way. The elevation and the tenant are * different axes (`ObjectQLEngine.buildDriverOptions` reads them separately): @@ -1822,13 +1771,53 @@ export class SharingRuleService implements ISharingRuleService { * from it (ruled 2026-09-02: a rule-materialised grant carries the rule's * organization). One context for the sweep and for the write is what keeps * the two agreeing — the grant lands in the organization whose records the - * rule was allowed to sweep. A platform-global rule threads none, and its + * rule was allowed to sweep. A deployment-wide rule threads none, and its * grants belong where each record does; `grant` derives that from the record. + * + * Both postures of a declared rule, and the refused row, are pinned in + * `rule-criteria-org-scope.test.ts`. */ - private criteriaContext(rule: SharingRuleRow): ExecutionContext { + private criteriaContext(rule: SharingRuleRow): ExecutionContext | null { const orgId = rule.organization_id; - if (orgId == null || orgId === '') return SYSTEM_CTX; - return { ...SYSTEM_CTX, tenantId: String(orgId) }; + if (typeof orgId === 'string' && orgId !== '') return { ...SYSTEM_CTX, tenantId: orgId }; + if (isDeploymentWideSharingRule(rule) || !this.organizationsComposed()) return SYSTEM_CTX; + return null; + } + + /** + * Does this composition register the organization object? Read from the + * engine's schema registry — the same fact `resolveSystemWriteOrganization` + * separates `no-organization-object` by. An engine that cannot answer is + * read as composing organizations: the reading that refuses, never the one + * that sweeps every organization. + */ + private organizationsComposed(): boolean { + if (typeof this.engine.getSchema !== 'function') return true; + return !!this.engine.getSchema(ORGANIZATION_OBJECT); + } + + /** + * {@link criteriaContext}, or `null` with the refusal reported — once per + * rule per process, at `error`: the rule looks configured and grants + * nothing, which is the degradation an operator cannot see otherwise. + */ + private sweepContextOrReport(rule: SharingRuleRow): ExecutionContext | null { + const ctx = this.criteriaContext(rule); + if (ctx) return ctx; + const key = String(rule.id ?? rule.name); + if (!this.organizationlessRuleReported.has(key)) { + this.organizationlessRuleReported.add(key); + const message = + `[sharing-rule] sharing rule '${rule.name ?? rule.id}' is a stored row with no organization, so its ` + + 'criteria are not swept and it grants nothing: a stored rule belongs to the organization that authored ' + + 'it, and only a rule declared in the application is deployment-wide. Remedy: run the organization-' + + 'ownership upgrade ceremony, which attributes the row to its organization, or delete the row and ' + + 'author the rule in its organization.'; + const meta = { rule: rule.name ?? rule.id, id: rule.id, object: rule.object_name }; + if (this.logger?.error) this.logger.error(message, meta); + else this.logger?.warn?.(message, meta); + } + return null; } private async findMatchingRecords(rule: SharingRuleRow): Promise { @@ -1844,14 +1833,17 @@ export class SharingRuleService implements ISharingRuleService { */ private async findMatchingRows(rule: SharingRuleRow, fields: string[]): Promise[]> { if (this.isInertMatchAll(rule)) return []; + // [#10119] The rule's own organization, when it has one — and nowhere + // when it may sweep nowhere (ADR-0131 D8, {@link criteriaContext}). + const context = this.sweepContextOrReport(rule); + if (!context) return []; const filter = (rule.criteria ?? {}) as any; try { const rows = await this.engine.find(rule.object_name, { filter, fields, limit: 5000, - // [#10119] The rule's own organization, when it has one. - context: this.criteriaContext(rule), + context, }); return Array.isArray(rows) ? rows : []; } catch (err: any) { @@ -1871,6 +1863,8 @@ export class SharingRuleService implements ISharingRuleService { */ private async matchRecord(rule: SharingRuleRow, recordId: string): Promise | null> { if (this.isInertMatchAll(rule)) return null; + const context = this.sweepContextOrReport(rule); + if (!context) return null; const filter = { ...((rule.criteria ?? {}) as any), id: recordId }; const fields = rule.recipient_type === 'field' ? ['id', rule.recipient_id] : ['id']; try { @@ -1882,7 +1876,7 @@ export class SharingRuleService implements ISharingRuleService { // {@link criteriaContext}. A per-record hook pass that stayed // unscoped would re-mint, one write at a time, exactly the cross-org // rows the sweep no longer creates. - context: this.criteriaContext(rule), + context, }); return Array.isArray(rows) && rows[0] ? (rows[0] as Record) : null; } catch (err: any) { @@ -2256,9 +2250,9 @@ export class SharingRuleService implements ISharingRuleService { * `sys_record_share` is tenant-scoped in the #13491 ledger (commit 3f64fe6c6), so on a * walled install an organization-less system insert on it is refused loudly * with {@link SYSTEM_WRITE_ORGANIZATION_REQUIRED_CODE}. `SharingService.grant` - * resolves the organization on every path that can; a platform-global rule - * (`organization_id = null`, its sweep unscoped) materialising onto an - * organization-less record resolves none, and meets the refusal. + * resolves the organization on every path that can; a rule whose sweep + * carries no tenant (a {@link DeploymentWideSharingRule}) materialising + * onto an organization-less record resolves none, and meets the refusal. * * Without this catch the refusal propagated out of the reconcile loop and * that rule's pass ABORTED mid-loop. The grants already written stayed; the @@ -2292,6 +2286,10 @@ export class SharingRuleService implements ISharingRuleService { recordId: string, recipientId: string, ): Promise { + // A rule that may sweep nowhere matched nothing, so it never asks for a + // grant; refused here too, so no caller can write one around the sweep. + const context = this.sweepContextOrReport(rule); + if (!context) return false; try { await this.sharing.grant( { @@ -2304,7 +2302,7 @@ export class SharingRuleService implements ISharingRuleService { sourceId: rule.id, reason: `rule:${rule.name}`, } as any, - this.criteriaContext(rule), + context, ); return true; } catch (err: any) { diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index 0fa446e2d96..4533d8c3655 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -2,11 +2,17 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { assertEngineDeleteDispatch } from '@objectstack/objectql'; -// [#7795] The platform's own code↔status pairing, so the delete-refusal tests -// assert an HTTP status they SOURCE rather than one they restate. +// [#8158] The platform's own code↔status pairing, so the refusal tests assert +// an HTTP status they SOURCE rather than one they restate. import { HttpStatusErrorCodeMap } from '@objectstack/spec/api'; import { SharingService } from './sharing-service.js'; -import { SharingRuleService, isDeclaredSharingRule, bindDeclaredRuleNameGuard, bindSharingRuleActivationReconcile } from './sharing-rule-service.js'; +import { + SharingRuleService, + isDeclaredSharingRule, + isDeploymentWideSharingRule, + bindDeclaredRuleNameGuard, + bindSharingRuleActivationReconcile, +} from './sharing-rule-service.js'; import { TeamGraphService, expandPrincipal } from './team-graph.js'; import { BusinessUnitGraphService } from './business-unit-graph.js'; // [commit 04d03c3a0] The ADDRESSING primitive, imported so the negative half of the @@ -962,15 +968,11 @@ describe('[ADR-0111 D6] sharing-rule management gate', () => { let rules: SharingRuleService; const MALLORY = { userId: 'mallory', systemPermissions: [] } as any; /** - * [#7795] `organizationId` added — this persona is a TENANT sharing admin, - * and the REST layer never builds one without an org. Omitting it made - * `defineRule` stamp `organization_id: null`, so the rule this block created - * to exercise the CAPABILITY gate was incidentally a PLATFORM-GLOBAL row — - * a shape no org admin can author through the API at all. That was invisible - * while every verb shared one gate; it stops being invisible now that delete - * judges the row's class too (see the #7795 block at the end of this file). - * The subject here is unchanged: `manage_sharing` authorizes the full - * surface for the caller's OWN organization. + * `organizationId` is set — this persona is a TENANT sharing admin, and the + * REST layer never builds one without an org. Omitted, `defineRule` would + * write a rule with no organization, a shape no org admin can author through + * the API at all. The subject here is unchanged: `manage_sharing` authorizes + * the full surface for the caller's OWN organization. */ const RULE_ADMIN = { userId: 'admin', organizationId: 'org1', systemPermissions: ['manage_sharing'] } as any; const LEGACY_ADMIN = { userId: 'admin', systemPermissions: ['manage_platform_settings'] } as any; @@ -1020,20 +1022,18 @@ describe('[ADR-0111 D6] sharing-rule management gate', () => { }); // ───────────────────────────────────────────────────────────────────── -// [#7676] Package-seeded rules (`organization_id = null`) are visible and -// addressable BY NAME to an org-scoped admin. +// [ADR-0131 D1/D8] An admin read is the caller's OWN organization — no NULL arm. // -// The QA run's finding: on a stock boot `GET /api/v1/sharing/rules` answered -// `{data: []}` over four active seeded rules, by-name GET/evaluate 404'd -// `RULE_NOT_FOUND`, and only the org-unfiltered by-id branch still worked — -// because the seeder defines under SYSTEM_CTX (`organization_id: null`) while -// an authenticated admin's context carries `organizationId: 'org_…'`, and a -// strict equality can never match. Enforcement was unaffected (boot reconcile -// reads under SYSTEM_CTX, no org filter), which is exactly why it stayed -// invisible: rules that grant access but cannot be listed or deactivated. +// It used to be "this org ∪ platform-global" (#7676): the retired boot seeder +// defined declared rules under SYSTEM_CTX, which stamped them with no +// organization, so a strict equality hid them from the admin API. Declared +// rules are read from the registry now (the ADR-0131 D2 block below serves +// them beside an organization's rows), and certification leaves no stored row +// without an organization. So the org-less row this fixture plants is INVALID +// STATE, and an organization's admin never reads, resolves or evaluates it. // ───────────────────────────────────────────────────────────────────── -describe('[#7676] admin visibility of package-seeded (org-null) sharing rules', () => { +describe('[ADR-0131 D8] an admin read is own-organization only (the org-less arm retired)', () => { let engine: ReturnType; let rules: SharingRuleService; @@ -1087,28 +1087,25 @@ describe('[#7676] admin visibility of package-seeded (org-null) sharing rules', // ── visibility (the defect) ────────────────────────────────────────── - it('listRules shows the seeded rule to an org-scoped admin', async () => { + it('listRules shows an org-scoped admin its OWN rows, never the organization-less one', async () => { const names = (await rules.listRules({}, ORG1_ADMIN)).map((r) => r.name); - expect(names).toContain(SEEDED); - // Exact set — "this org ∪ platform-global", and nothing else. Kept HERE - // rather than on the isolation pins below so that each test has ONE - // predicted direction under the ablation: this one flips red, they do not. - expect(names.sort()).toEqual([SEEDED, 'org1_rule'].sort()); + expect(names).not.toContain(SEEDED); + // Exact set — the caller's organization, and nothing else. Kept HERE rather + // than on the isolation pins below so that each test has ONE predicted + // direction under the ablation (restore the NULL arm): this one flips red, + // they do not. + expect(names).toEqual(['org1_rule']); }); - it('getRule resolves the seeded rule BY NAME for an org-scoped admin', async () => { - const row = await rules.getRule(SEEDED, ORG1_ADMIN); - expect(row?.name).toBe(SEEDED); - expect(row?.organization_id).toBeNull(); + it('getRule does not resolve the organization-less row BY NAME, nor BY ID', async () => { + expect(await rules.getRule(SEEDED, ORG1_ADMIN)).toBeNull(); + const seededId = engine._tables.sys_sharing_rule.find((r) => r.name === SEEDED)!.id; + expect(await rules.getRule(seededId, ORG1_ADMIN)).toBeNull(); }); - it('evaluateRule works BY NAME for the seeded rule (no RULE_NOT_FOUND)', async () => { - const res = await rules.evaluateRule(SEEDED, ORG1_ADMIN); - expect(res.matchedRecords).toBe(1); - expect(res.grantsCreated).toBe(1); - // The control the QA run used: by-id already worked, and must still agree. - const byId = await rules.getRule(SEEDED, ORG1_ADMIN); - expect((await rules.evaluateRule(byId!.id, ORG1_ADMIN)).ruleId).toBe(byId!.id); + it('evaluateRule refuses the organization-less row (RULE_NOT_FOUND) and writes no grant', async () => { + await expect(rules.evaluateRule(SEEDED, ORG1_ADMIN)).rejects.toThrow(/RULE_NOT_FOUND/); + expect(engine._tables.sys_record_share ?? []).toEqual([]); }); // ── isolation pins (must stay green under the ablation) ────────────── @@ -1164,7 +1161,7 @@ describe('[#7676] admin visibility of package-seeded (org-null) sharing rules', expect((await rules.getRule('other_org_rule', BOOT))?.organization_id).toBe('org2'); }); - it('when both a platform-global and an own-org row share a name, by-name resolves the OWN row', async () => { + it('when both an organization-less and an own-org row share a name, by-name resolves the OWN row', async () => { // `defineRule` is deliberately NOT widened, so a same-named POST from an // org admin creates its own row instead of overwriting the shared seed. const own = await rules.defineRule({ @@ -1198,8 +1195,8 @@ describe('[#7676] admin visibility of package-seeded (org-null) sharing rules', // would satisfy a row-only assertion while leaving the damage intact. // // Ablation (predicted in advance): restore `where: {id: idOrName}` and the -// three "unreachable by id" tests flip red, while the platform-global, -// own-org and BOOT pins below stay green. +// three "unreachable by id" tests flip red, while the own-org and BOOT pins +// below stay green. // ───────────────────────────────────────────────────────────────────── describe('[#7761] getRule by-id is scoped to the caller organization', () => { @@ -1235,7 +1232,8 @@ describe('[#7761] getRule by-id is scoped to the caller organization', () => { ]; rules = new SharingRuleService({ engine: engine as any, sharing: new SharingService({ engine: engine as any }) }); - // Platform-global package seed — defined with no org, as the boot seeder does. + // An organization-less row, as the retired boot seeder left one — invalid + // state on a certified database, planted so the scope has a row to refuse. seededId = (await rules.defineRule({ name: SEEDED, label: 'Red projects → execs', object: 'project', criteria: { status: 'red' }, recipientType: 'user', recipientId: 'exec', @@ -1290,19 +1288,24 @@ describe('[#7761] getRule by-id is scoped to the caller organization', () => { expect(grantsOf(otherOrgRuleId)).toHaveLength(1); }); - // ── platform-global stays reachable (what #7760 enabled by name) ───── + // ── [ADR-0131 D8] an organization-less row is no organization's ─────── - it('a platform-global (organization_id = null) rule stays reachable BY ID — read', async () => { - const row = await rules.getRule(seededId, ORG1_ADMIN); - expect(row?.name).toBe(SEEDED); - expect(row?.organization_id).toBeNull(); + it('an organization-less row is unreachable BY ID — read and evaluate', async () => { + expect(await rules.getRule(seededId, ORG1_ADMIN)).toBeNull(); + await expect(rules.evaluateRule(seededId, ORG1_ADMIN)).rejects.toThrow(/RULE_NOT_FOUND/); + expect(grantsOf(seededId)).toEqual([]); }); - it('a platform-global rule stays reachable BY ID — evaluate', async () => { - const res = await rules.evaluateRule(seededId, ORG1_ADMIN); - expect(res.ruleId).toBe(seededId); - expect(res.matchedRecords).toBe(1); - expect(res.grantsCreated).toBe(1); + it('an organization-less row is unreachable BY ID — delete is a no-op that leaves the row AND its grants intact', async () => { + // The successor of the retired #7795 delete guard: the row is no longer + // refused by class, it is simply not the org admin's to address. Grants + // are materialised under BOOT (this double registers no organization + // object, so the row sweeps the deployment) so the no-op has something to lose. + await rules.evaluateRule(seededId, BOOT); + expect(grantsOf(seededId)).toHaveLength(1); + await rules.deleteRule(seededId, ORG1_ADMIN); + expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeTruthy(); + expect(grantsOf(seededId)).toHaveLength(1); }); // ── unchanged behaviour ────────────────────────────────────────────── @@ -1324,275 +1327,6 @@ describe('[#7761] getRule by-id is scoped to the caller organization', () => { }); }); -// ───────────────────────────────────────────────────────────────────── -// [#7795] DELETING a platform-global (`organization_id = null`) rule requires -// PLATFORM authority. Maintainer ruling 2026-08-12, 方向 B — quoted verbatim -// and untranslated in `sharing-rule-service.ts`'s -// `assertCanDeletePlatformGlobalRule` docblock, which is the authority on the -// decision; this file measures it. -// -// The split, in one line: read/list/evaluate stay OPEN (that is what #7760 -// deliberately unlocked), delete alone closes to org-scoped callers. -// -// ## The persona is the whole test -// -// Every refusal below is driven as an ORG-LEVEL admin **holding -// `manage_sharing`** — the exact caller the ruling refuses, and the only one -// that can fail against the pre-fix build. Written as an unprivileged user it -// would pass before AND after the fix (the ADR-0111 D6 gate refuses that -// caller several lines earlier) and would prove nothing at all. The -// `ORG1_ADMIN` fixture therefore carries `manage_sharing` on purpose, and -// `the ADR-0111 D6 gate still fires FIRST` below pins that the two refusals -// stay distinguishable. -// -// ## Why `code` and `status` are asserted separately -// -// The harm this closes and the harm a 404 would cause are different bugs, so a -// loose `.toThrow()` — or even `/PERMISSION_DENIED/` as a substring — cannot -// tell them apart. `refusalCodeOf` parses the ADR-0112 code TOKEN (the prefix -// `rest-server.ts`'s sharing-rule `handleError` dispatches on to pick the HTTP -// status) and the assertions pair it against the spec's own -// `HttpStatusErrorCodeMap`: it must be the code the platform pairs with **403** -// and must not be either 404-shaped code. A regression that answered 404 fails -// as a NAMED, different assertion rather than sliding through. -// -// And the 404 is not merely a worse status, it is a false statement — which is -// why `the 404 a stricter reading would have chosen would be a LIE` asserts -// that the very same refused caller can still READ the row it may not delete. -// -// ## Ablation (predicted in advance, then measured) -// -// Remove the `assertCanDeletePlatformGlobalRule` call from `deleteRule` and: -// the four refusal tests flip RED; every permitted-side and #7760 read-surface -// pin below stays GREEN. Plain red, not "more diagnostics" and not inverted — -// the guard is a new refusal on a path that previously succeeded, and the pins -// sit on the refusal itself. -// ───────────────────────────────────────────────────────────────────── - -describe('[#7795] deleting a platform-global rule requires platform authority', () => { - let engine: ReturnType; - let rules: SharingRuleService; - - /** - * THE REFUSED PERSONA: an org-scoped sharing admin. Holds `manage_sharing` - * (so it clears the ADR-0111 D6 gate and reaches the new one) and carries an - * `organizationId` (so it is org-scoped, not platform). This is the shape the - * REST layer builds for a tenant admin. - */ - const ORG1_ADMIN = { userId: 'admin', organizationId: 'org1', systemPermissions: ['manage_sharing'] } as any; - /** Platform authority, spelling 1: the `scope: 'platform'` CAPABILITY. */ - const PLATFORM_SETTINGS_ADMIN = { - userId: 'ops', organizationId: 'org1', - systemPermissions: ['manage_sharing', 'manage_platform_settings'], - } as any; - /** - * Platform authority, spelling 2: the ADR-0095 posture RUNG. - * - * [#15981] This fixture used to spell the second channel as the built-in - * POSITION NAME alone, on the reading that the name is only ever projected - * from the unscoped `admin_full_access` grant. `positions[]` is now the - * security axis and also carries ADR-0057 D4 `sys_user_position` names, so - * the name no longer implies the grant — the rung does. A genuine operator's - * resolved context carries BOTH (the projection is still emitted), which is - * why this keeps `positions` as well: dropping it would make the fixture - * unrepresentative in the opposite direction. - */ - const PLATFORM_ADMIN_RUNG = { - userId: 'root', organizationId: 'org1', - systemPermissions: ['manage_sharing'], positions: ['platform_admin'], - posture: 'PLATFORM_ADMIN', - } as any; - /** - * [#15981] The ESCALATION shape: the same name with no rung behind it — what - * a tenant can mint through the `apiEnabled` `sys_user_position` surface. - * Refused. Driven end-to-end through the real resolver in - * `sharing-rule-positions-name-authority.test.ts`; this arm keeps the - * boundary visible in the suite that owns the gate. - */ - const PLATFORM_ADMIN_NAME_ONLY = { - userId: 'impostor', organizationId: 'org1', - systemPermissions: ['manage_sharing'], positions: ['platform_admin'], - posture: 'MEMBER', - } as any; - /** No `manage_sharing` at all — must still be refused by the OLDER gate. */ - const MALLORY = { userId: 'mallory', organizationId: 'org1', systemPermissions: [] } as any; - /** The seeder's / boot context — carries no org, which is what stamps `organization_id: null`. */ - const BOOT = { isSystem: true, positions: [], permissions: [] } as any; - - const SEEDED = 'share_red_projects_with_execs'; - let seededId = ''; - let org1RuleId = ''; - - const grantsOf = (ruleId: string): Row[] => - (engine._tables.sys_record_share ?? []).filter((g) => g.source === 'rule' && g.source_id === ruleId); - - /** - * The ADR-0112 code TOKEN carried by a refused call, or a failure if the call - * was not refused at all. Deliberately the leading token rather than a - * substring match: `PERMISSION_DENIED` and `RULE_NOT_FOUND` must be - * distinguishable, because they are the two different bugs this card sits - * between. - */ - const refusalCodeOf = async (call: Promise): Promise => { - try { - await call; - } catch (err: any) { - return String(err?.message ?? err ?? '').split(':')[0].trim(); - } - throw new Error('expected the call to be REFUSED, but it resolved successfully'); - }; - - beforeEach(async () => { - engine = makeEngine(); - engine._tables.project = [ - { id: 'p_red', status: 'red', owner_id: 'someone' }, - { id: 'p_green', status: 'green', owner_id: 'someone' }, - ]; - rules = new SharingRuleService({ engine: engine as any, sharing: new SharingService({ engine: engine as any }) }); - - // The platform-global package seed — defined with no org, exactly as - // `bootstrapDeclaredSharingRules` does on every boot. - seededId = (await rules.defineRule({ - name: SEEDED, label: 'Red projects → execs', object: 'project', - criteria: { status: 'red' }, recipientType: 'user', recipientId: 'exec', - managedBy: 'package', - } as any, BOOT)).id; - // The caller's OWN org rule — the control that proves a refusal below comes - // from the row's platform-global class and not from delete breaking wholesale. - org1RuleId = (await rules.defineRule({ - name: 'org1_rule', label: 'Org1 own', object: 'project', - criteria: { status: 'red' }, recipientType: 'user', recipientId: 'alice', - } as any, ORG1_ADMIN)).id; - - // Materialise both rules' grants under BOOT, so the fixture never depends - // on the very gate these tests measure. - await rules.evaluateRule(seededId, BOOT); - await rules.evaluateRule(org1RuleId, BOOT); - }); - - it('the fixture really is platform-global, org-stamped, and carrying live grants', () => { - const rows = engine._tables.sys_sharing_rule; - expect(rows.find((r) => r.id === seededId)?.organization_id).toBeNull(); - expect(rows.find((r) => r.id === seededId)?.managed_by).toBe('package'); - expect(rows.find((r) => r.id === org1RuleId)?.organization_id).toBe('org1'); - // Without live grants the "grants survive" assertions below could pass over - // a rule that never had any to lose. - expect(grantsOf(seededId)).toHaveLength(1); - expect(grantsOf(org1RuleId)).toHaveLength(1); - // And the refused persona really does clear the OLDER gate, or the refusal - // under test would be indistinguishable from ADR-0111 D6's. - expect(ORG1_ADMIN.systemPermissions).toContain('manage_sharing'); - }); - - // ── the refusal ────────────────────────────────────────────────────── - - it('an org admin holding manage_sharing is refused — 403 PERMISSION_DENIED, by id', async () => { - const code = await refusalCodeOf(rules.deleteRule(seededId, ORG1_ADMIN)); - // `code`: the exact ADR-0112 token, not a substring. - expect(code).toBe('PERMISSION_DENIED'); - // `status`: sourced from the platform's own code↔status pairing rather than - // restated, so this cannot agree with itself. - expect(HttpStatusErrorCodeMap[403]).toBe(code); - // …and distinctly NOT 404-shaped, in either of that status's spellings — - // the standard catalog's, and this route's own `RULE_NOT_FOUND`. - expect(code).not.toBe(HttpStatusErrorCodeMap[404]); - expect(code).not.toBe('RULE_NOT_FOUND'); - }); - - it('an org admin holding manage_sharing is refused — by NAME as well as by id', async () => { - const code = await refusalCodeOf(rules.deleteRule(SEEDED, ORG1_ADMIN)); - expect(code).toBe('PERMISSION_DENIED'); - expect(HttpStatusErrorCodeMap[403]).toBe(code); - expect(code).not.toBe('RULE_NOT_FOUND'); - }); - - it('the refusal leaves the rule row AND every tenant’s grants intact', async () => { - await expect(rules.deleteRule(seededId, ORG1_ADMIN)).rejects.toThrow(); - expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeTruthy(); - // The grants are the assertion that matters: the harm in this defect is the - // cross-tenant grant purge, so a "fix" that threw but still purged — or - // that purged before throwing — would satisfy a row-only check while - // leaving the damage done. - expect(grantsOf(seededId)).toHaveLength(1); - }); - - it('the 404 a stricter reading would have chosen would be a LIE — the caller can still READ the row', async () => { - // This is the ruling's stated reason for 403 over 404, made executable: - // the row is DELIBERATELY visible (#7760), so answering "no such rule" to - // the very caller that can list and read it one call earlier would be the - // platform contradicting itself. - const readable = await rules.getRule(seededId, ORG1_ADMIN); - expect(readable?.id).toBe(seededId); - expect(readable?.organization_id).toBeNull(); - expect(await refusalCodeOf(rules.deleteRule(seededId, ORG1_ADMIN))).toBe('PERMISSION_DENIED'); - }); - - it('the ADR-0111 D6 gate still fires FIRST for a caller without manage_sharing', async () => { - // Ordering pin: the new guard resolves the row before judging it, so it - // must not have displaced the capability gate that runs before any read. - // Both refusals are PERMISSION_DENIED; the messages keep them apart. - await expect(rules.deleteRule(seededId, MALLORY)).rejects.toThrow(/manage_sharing capability/); - expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeTruthy(); - }); - - // ── the permitted side ─────────────────────────────────────────────── - - it('the manage_platform_settings capability authorizes the delete', async () => { - await expect(rules.deleteRule(seededId, PLATFORM_SETTINGS_ADMIN)).resolves.toBeUndefined(); - expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeUndefined(); - expect(grantsOf(seededId)).toHaveLength(0); - }); - - it('the PLATFORM_ADMIN posture rung authorizes the delete', async () => { - await expect(rules.deleteRule(seededId, PLATFORM_ADMIN_RUNG)).resolves.toBeUndefined(); - expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeUndefined(); - expect(grantsOf(seededId)).toHaveLength(0); - }); - - it('[#15981] the built-in position NAME without the rung does NOT', async () => { - expect(await refusalCodeOf(rules.deleteRule(seededId, PLATFORM_ADMIN_NAME_ONLY))) - .toBe('PERMISSION_DENIED'); - // The row survives: a refusal that still destroyed the rule would be none. - expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeTruthy(); - }); - - it('a system context (boot seeding, hooks, backfills) still deletes', async () => { - await expect(rules.deleteRule(seededId, BOOT)).resolves.toBeUndefined(); - expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeUndefined(); - }); - - it('the org admin can still delete its OWN organization’s rule', async () => { - // The control: delete did not simply stop working for org admins. - await expect(rules.deleteRule(org1RuleId, ORG1_ADMIN)).resolves.toBeUndefined(); - expect(engine._tables.sys_sharing_rule.find((r) => r.id === org1RuleId)).toBeUndefined(); - expect(grantsOf(org1RuleId)).toHaveLength(0); - }); - - // ── #7760's read/evaluate surface is a REGRESSION surface, not collateral ── - - it('#7760 stays whole: the org admin can still LIST the platform-global rule', async () => { - const names = (await rules.listRules({}, ORG1_ADMIN)).map((r) => r.name); - expect(names).toContain(SEEDED); - expect(names).toContain('org1_rule'); - }); - - it('#7760 stays whole: the org admin can still GET it, by id and by name', async () => { - expect((await rules.getRule(seededId, ORG1_ADMIN))?.name).toBe(SEEDED); - expect((await rules.getRule(SEEDED, ORG1_ADMIN))?.id).toBe(seededId); - }); - - it('#7760 stays whole: the org admin can still EVALUATE it — grants reconcile', async () => { - const res = await rules.evaluateRule(seededId, ORG1_ADMIN); - expect(res.ruleId).toBe(seededId); - expect(res.matchedRecords).toBe(1); - // Evaluate is a WRITE. It must still reach the grant table, not merely - // return a shape — a gate applied to the whole null-org class instead of - // to `delete` alone would show up right here. - expect(grantsOf(seededId)).toHaveLength(1); - expect(res.grantsRevoked).toBe(0); - }); -}); - // ───────────────────────────────────────────────────────────────────── // [#8158] An authenticated `manage_sharing` holder whose session resolves NO // organization is REFUSED — it used to get the system read scope. @@ -1609,8 +1343,8 @@ describe('[#7795] deleting a platform-global rule requires platform authority', // // ## Anti-vacuity // -// The fixture seeds TWO organizations, each with its own rule, plus the -// platform-global seed. A single-tenant fixture would pass on the BROKEN build +// The fixture seeds TWO organizations, each with its own rule, plus an +// organization-less row (invalid state on a certified database). A single-tenant fixture would pass on the BROKEN build // (nothing else to leak), so every refusal below is paired with a positive // half naming the OTHER tenant's row, and the two org-bound personas assert // their EXACT visible set. @@ -1685,7 +1419,7 @@ describe('[#8158] a non-system caller with NO organization does not get the syst (engine._tables.sys_record_share ?? []).filter((g) => g.source === 'rule' && g.source_id === ruleId); const namesOf = (rows: { name: string }[]): string[] => rows.map((r) => r.name).sort(); - /** The leading ADR-0112 token of a refusal — see the #7795 block's copy. */ + /** The leading ADR-0112 token of a refusal: the prefix `rest-server.ts` dispatches its HTTP status on. */ const refusalCodeOf = async (call: Promise): Promise => { try { await call; @@ -1722,7 +1456,7 @@ describe('[#8158] a non-system caller with NO organization does not get the syst await rules.evaluateRule(org2RuleId, BOOT); }); - it('PRECONDITION: two organizations, two rules, one platform-global row, live grants', () => { + it('PRECONDITION: two organizations, two rules, one organization-less row, live grants', () => { const rows = engine._tables.sys_sharing_rule; expect(rows).toHaveLength(3); expect(rows.find((r) => r.id === seededId)?.organization_id).toBeNull(); @@ -1744,7 +1478,7 @@ describe('[#8158] a non-system caller with NO organization does not get the syst it('listRules is refused — it used to return BOTH tenants’ rules', async () => { const code = await refusalCodeOf(rules.listRules({}, ORG_LESS_SHARING_ADMIN)); expect(code).toBe('PERMISSION_DENIED'); - // Sourced from the platform's own pairing rather than restated (#7795). + // Sourced from the platform's own pairing rather than restated. expect(HttpStatusErrorCodeMap[403]).toBe(code); expect(code).not.toBe('RULE_NOT_FOUND'); // The rows it used to answer with are really there to be leaked — this is @@ -1792,9 +1526,8 @@ describe('[#8158] a non-system caller with NO organization does not get the syst expect(grantsOf(org2RuleId)).toHaveLength(1); }); - it('defineRule is refused — no accidental platform-global rule', async () => { - // An org-less caller stamps `organization_id: null`, i.e. a rule whose - // grants reach EVERY tenant and which #7795 then forbids them to delete. + it('defineRule is refused — no rule written for an org-less caller', async () => { + // An org-less caller has no organization for the rule to belong to. expect(await refusalCodeOf(rules.defineRule({ name: 'mallory_rule', label: 'Mallory', object: 'project', criteria: { status: 'red' }, recipientType: 'user', recipientId: 'mallory', @@ -1817,16 +1550,17 @@ describe('[#8158] a non-system caller with NO organization does not get the syst expect((await rules.getRule('org1_rule', BOOT))?.id).toBe(org1RuleId); }); - it('a SYSTEM context still SEEDS platform-global rows (boot seeding unbroken)', async () => { - // `bootstrapDeclaredSharingRules` runs on every boot with exactly this + it('a SYSTEM context still defines and evaluates rules (the plugin machinery unbroken)', async () => { + // The reconcile hooks, backfills and migrations run with exactly this // context; a guard that refused it would take the deployment down rather - // than leak anything. - const row = await rules.defineRule({ + // than leak anything. (Its insert carries no organization: a real engine + // stamps or refuses it, ADR-0131 D9; this double has no organization + // object, the lean-embedding composition, so the row sweeps the deployment.) + await rules.defineRule({ name: 'seeded_on_boot', label: 'Seeded', object: 'project', criteria: { status: 'red' }, recipientType: 'user', recipientId: 'exec', managedBy: 'package', } as any, BOOT); - expect(row.organization_id).toBeNull(); expect(await rules.evaluateRule('seeded_on_boot', BOOT)).toMatchObject({ matchedRecords: 1 }); }); @@ -1853,12 +1587,12 @@ describe('[#8158] a non-system caller with NO organization does not get the syst // ── the anti-vacuity control: scoped ≠ single-tenant ───────────────── - it('each org-bound admin sees EXACTLY its own org ∪ platform-global', async () => { + it('each org-bound admin sees EXACTLY its own organization', async () => { // The pair that distinguishes "scoped correctly" from "there was only one - // tenant's data anyway": two personas, two disjoint answers, one shared - // platform-global row, over the same three-row table. - expect(namesOf(await rules.listRules({}, ORG1_ADMIN))).toEqual(['org1_rule', SEEDED].sort()); - expect(namesOf(await rules.listRules({}, ORG2_ADMIN))).toEqual(['org2_rule', SEEDED].sort()); + // tenant's data anyway": two personas, two disjoint answers, over the same + // three-row table — and the organization-less row in neither (ADR-0131 D8). + expect(namesOf(await rules.listRules({}, ORG1_ADMIN))).toEqual(['org1_rule']); + expect(namesOf(await rules.listRules({}, ORG2_ADMIN))).toEqual(['org2_rule']); expect(await rules.getRule('org2_rule', ORG1_ADMIN)).toBeNull(); expect(await rules.getRule(org1RuleId, ORG2_ADMIN)).toBeNull(); }); @@ -2147,7 +1881,7 @@ describe('[ADR-0131 D2] declared sharing rules are read from the registry, never return { engine, rules, grants }; } - describe('`single`: one organization-less projection', () => { + describe('`single`: one deployment-wide projection', () => { let h: ReturnType; beforeEach(() => { h = harness([DECLARED], [ @@ -2159,11 +1893,14 @@ describe('[ADR-0131 D2] declared sharing rules are read from the registry, never it('a declared rule is listed and enforces — and no `sys_sharing_rule` row is written for it', async () => { const listed = await h.rules.listRules({}, BOOT); expect(listed).toEqual([expect.objectContaining({ - id: DECLARED.name, name: DECLARED.name, organization_id: null, object_name: 'project', + id: DECLARED.name, name: DECLARED.name, deploymentWide: true, object_name: 'project', criteria: { status: 'red' }, recipient_type: 'user', recipient_id: 'exec', access_level: 'read', active: true, managed_by: 'package', customized: false, })]); + // [ADR-0131 D2/D8] The typed marker, never a NULL organization. + expect(listed[0]).not.toHaveProperty('organization_id'); expect(isDeclaredSharingRule(listed[0])).toBe(true); + expect(isDeploymentWideSharingRule(listed[0])).toBe(true); const pass = await h.rules.evaluateRule(DECLARED.name, BOOT); expect(pass).toMatchObject({ ruleId: DECLARED.name, matchedRecords: 1, grantsCreated: 1 }); @@ -2266,6 +2003,8 @@ describe('[ADR-0131 D2] declared sharing rules are read from the registry, never it('lists one projection per organization, each sweeping only its organization’s records', async () => { const listed = await h.rules.listRules({}, BOOT); expect(listed.map((r) => [r.id, r.organization_id])).toEqual([[DECLARED.name, 'org_a'], [DECLARED.name, 'org_b']]); + // [ADR-0131 D8] Under a wall no projection sweeps the deployment: each is its organization's. + expect(listed.filter((r) => isDeploymentWideSharingRule(r))).toEqual([]); const pass = await h.rules.evaluateRule(DECLARED.name, BOOT); expect(pass).toMatchObject({ ruleId: DECLARED.name, matchedRecords: 2, grantsCreated: 2 }); expect(h.grants().map((g) => [g.record_id, g.organization_id, g.source_id]).sort()).toEqual([ @@ -2507,7 +2246,8 @@ describe('[ADR-0131 D2] declared sharing rules are read from the registry, never expect((await h.rules.listRules({}, BOOT)).map((r) => r.id)).toEqual(['srule_authored']); await h.rules.listRules({}, BOOT); expect(errors).toHaveLength(1); - expect(errors[0][1]).toEqual({ rule: DECLARED.name, organization: null }); + // [ADR-0131 D8] The deployment-wide scope is named, never a NULL organization. + expect(errors[0][1]).toEqual({ rule: DECLARED.name, deploymentWide: true }); // The declaration grants nothing beside it: the boot sweep revokes its grants. expect(await h.rules.sweepOrphanedRuleGrants()).toBe(2); // The stored row stays editable as itself (the upsert of a row that holds the name already). diff --git a/packages/qa/dogfood/test/org-scoped-sharing-rule-listing.dogfood.test.ts b/packages/qa/dogfood/test/org-scoped-sharing-rule-listing.dogfood.test.ts index a864e78f8ae..a3c3317bfdf 100644 --- a/packages/qa/dogfood/test/org-scoped-sharing-rule-listing.dogfood.test.ts +++ b/packages/qa/dogfood/test/org-scoped-sharing-rule-listing.dogfood.test.ts @@ -7,8 +7,9 @@ // #7676: `sys_sharing_rule` rows seeded by the package/app seeder carried // `organization_id = null` (`bootstrapDeclaredSharingRules` defined under // `SYSTEM_CTX`, before any org id existed; ADR-0131 D2 has since retired it, -// and a declared rule is now read from the registry, served organization-less -// under `single` — the same platform-global class). The admin read path scoped with a +// and a declared rule is now read from the registry, served under `single` as +// ONE deployment-wide projection — a typed `deploymentWide` marker, never a +// NULL organization, ADR-0131 D8). The admin read path scoped with a // strict `organization_id = ` equality, so on a stock boot // `GET /api/v1/sharing/rules` answered `{data: []}` over four active seeded // rules, by-name GET and evaluate 404'd, and only the org-unfiltered by-id @@ -34,7 +35,7 @@ // organization wall (SecurityPlugin strips the wildcard `organization_id` RLS // policies with no `org-scoping` service present) — see the option's doc block // in `packages/verify/src/harness.ts`. This file proves that an org-scoped READ -// FILTER engages and admits the platform-global rows it must; it proves nothing +// FILTER engages and serves the declared rules beside it; it proves nothing // about tenant B's rows, and must never be extended to claim that. // // @proof: org-scoped-sharing-rule-listing @@ -47,7 +48,7 @@ const RULES = '/sharing/rules'; const SYS = { isSystem: true } as const; /** - * The rules `examples/app-showcase` declares, all served org-less. + * The rules `examples/app-showcase` declares, all served deployment-wide. * * [#9237] Two of the former four were retired rather than re-homed: they sat * on `showcase_project` / `showcase_task`, whose `public_read_write` OWD @@ -63,7 +64,8 @@ const SEEDED_RULE_NAMES = [ interface RuleRow { id: string; name: string; - organization_id: string | null; + organization_id?: string | null; + deploymentWide?: boolean; } describe('#7676 — package-seeded (org-less) sharing rules stay visible to an ORG-BOUND admin', () => { @@ -124,11 +126,14 @@ describe('#7676 — package-seeded (org-less) sharing rules stay visible to an O for (const name of SEEDED_RULE_NAMES) { expect(names, `seeded rule ${name} must be listed`).toContain(name); } - // They really are the platform-global rows, not org-stamped copies — which - // is what makes them invisible to a strict `organization_id = orgId`. + // They really are the declarations, not org-stamped copies: the + // deployment-wide marker and NO organization key — never a NULL one + // (ADR-0131 D8). The org-scoped row read cannot see them; the registry + // read serves them beside it. for (const name of SEEDED_RULE_NAMES) { const row = body.data.find((r) => r.name === name); - expect(row?.organization_id, `${name} is seeded org-less`).toBeNull(); + expect(row?.deploymentWide, `${name} is served deployment-wide`).toBe(true); + expect(row, `${name} carries no organization key`).not.toHaveProperty('organization_id'); } }); @@ -137,7 +142,8 @@ describe('#7676 — package-seeded (org-less) sharing rules stay visible to an O expect(res.status).toBe(200); const row = (await res.json()) as RuleRow; expect(row.name).toBe(SEEDED_RULE_NAMES[0]); - expect(row.organization_id).toBeNull(); + expect(row.deploymentWide).toBe(true); + expect(row).not.toHaveProperty('organization_id'); }); it('a declared org-less rule can be EVALUATED by name — read from the registry, never stored', async () => { @@ -157,8 +163,8 @@ describe('#7676 — package-seeded (org-less) sharing rules stay visible to an O it('the scope still SCOPES — another organization\'s rule is not listed', async () => { // The counterweight, and the reason this is a scope rather than a hole: - // widening the read to `this org ∪ platform-global` must not also admit a - // THIRD org's row. Written at the storage seam because the harness mints + // serving the declared rules beside the organization's rows must not also + // admit a THIRD org's row. Written at the storage seam because the harness mints // exactly one organization — this is a read-filter assertion, NOT a // tenant-isolation proof (see the header). const foreignId = 'srule_7762_foreign'; diff --git a/packages/qa/dogfood/test/sharing-rule-org-less-caller.dogfood.test.ts b/packages/qa/dogfood/test/sharing-rule-org-less-caller.dogfood.test.ts index 18fc74ea469..1b8d463cd9c 100644 --- a/packages/qa/dogfood/test/sharing-rule-org-less-caller.dogfood.test.ts +++ b/packages/qa/dogfood/test/sharing-rule-org-less-caller.dogfood.test.ts @@ -321,7 +321,7 @@ describe('#8158 — a manage_sharing holder with NO active organization cannot r expect((await ql.findOne('sys_sharing_rule', { where: { name: RULE_B }, context: SYS }))?.id).toBeTruthy(); }); - it('CREATE is refused — no org-less caller mints a platform-global rule', async () => { + it('CREATE is refused — no org-less caller writes a rule', async () => { const res = await stack.apiAs(orgLess, 'POST', RULES, { name: 'rule_8158_minted_by_orgless', label: 'Minted with no organization', diff --git a/packages/runtime/src/seed-loader-null-organization-holder-real-driver.integration.test.ts b/packages/runtime/src/seed-loader-null-organization-holder-real-driver.integration.test.ts index 6dd3470bef2..554802486dc 100644 --- a/packages/runtime/src/seed-loader-null-organization-holder-real-driver.integration.test.ts +++ b/packages/runtime/src/seed-loader-null-organization-holder-real-driver.integration.test.ts @@ -1,34 +1,29 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * An organization-scoped seed load over a row whose natural key a - * NULL-organization row already holds REFUSES that seed row, by name, in the - * seed result. It never inserts a twin beside the unowned row, and it never - * adopts it. + * [ADR-0131 D8] An organization-scoped seed load on a CERTIFIED database reads + * its own organization's rows and nothing else: no `organization_id IS NULL` + * read on any path, no twin of a row it already holds, and another + * organization's row invisible to it. * - * ## The defect, as measured + * ## What retired * - * A database whose rows predate organization stamping (a single-posture 17.x - * install, every business row `organization_id` NULL) took an - * organization-scoped load: the per-organization replay, the walled install or - * the reseed, all of which ask `SeedLoaderService` for `defaultMode: 'upsert'` - * with `config.organizationId`. The scoped pre-load read only - * `organization_id = ORG`, so the NULL row holding each natural key was - * invisible, the upsert decided `insert`, and every such row got a twin that - * carried the organization: 0 errors, nothing logged. On this file's fixture - * (two NULL events, one NULL task), before the fix, the scoped load inserted - * all five seed rows, a twin for each of the three NULL-held keys - * (`tw_event` 2 → 5, `tw_task` 1 → 3). + * Until step 7 of the organization-ownership line the scoped pre-load also + * read the natural keys held by rows with NO organization — a 17.x + * single-posture install's rows — and refused to twin or adopt them (#24301). + * A database is certified before it boots now: the ownership ceremony + * attributes every such row (D10), an uncertified database is refused at boot, + * and a new write without an organization is stamped or refused (D9). So the + * NULL read had no row left to find, and it retired. What these cases hold is + * its absence, and the behaviour a certified database keeps without it. * * ## Why a real engine and real drivers * - * The scoped lookup's NULL comparison is a driver's answer, not the loader's: - * `{ organization_id: null }` is `IS NULL` on the SQL family. An engine double - * would encode the very predicate under test. So this file runs the real - * `SeedLoaderService` behind a real `ObjectQL`, over a real `SqlDriver` on a - * better-sqlite3 FILE, and asserts on the stored rows. The - * per-organization replay is the production closure AppPlugin registers on a - * walled boot; the walled install and the reseed are pinned with the request + * The scoped lookup's comparison is a driver's answer, not the loader's. So + * this file runs the real `SeedLoaderService` behind a real `ObjectQL`, over a + * real `SqlDriver` on a better-sqlite3 FILE, and asserts on the stored rows. + * The per-organization replay is the production closure AppPlugin registers on + * a walled boot; the walled install and the reseed are pinned with the request * `MarketplaceInstallLocalPlugin.runInlineSeed` builds for both. */ @@ -137,11 +132,11 @@ async function boot() { return { engine, all, plant }; } -/** The 17.x shape: every row of the seed already present, `organization_id` NULL. */ -async function plantUnowned(plant: (o: string, r: Record) => Promise) { - await plant('tw_event', { id: 'ev_kickoff', subject: 'Kickoff', notes: 'first', organization_id: null }); - await plant('tw_event', { id: 'ev_review', subject: 'Review', notes: 'second', organization_id: null }); - await plant('tw_task', { id: 'tk_room', title: 'Book room', event_id: 'ev_kickoff', organization_id: null }); +/** A certified database: part of the seed already present, stamped with the organization. */ +async function plantOwn(plant: (o: string, r: Record) => Promise) { + await plant('tw_event', { id: 'ev_kickoff', subject: 'Kickoff', notes: 'first', organization_id: ORG }); + await plant('tw_event', { id: 'ev_review', subject: 'Review', notes: 'second', organization_id: ORG }); + await plant('tw_task', { id: 'tk_room', title: 'Book room', event_id: 'ev_kickoff', organization_id: ORG }); } /** The request the per-organization replay and `runInlineSeed` (install, reseed) both build. */ @@ -152,72 +147,40 @@ function scopedRequest(organizationId: string, seeds: unknown = SEEDS) { }); } -/** - * The per-row refusal, told apart structurally: the existing - * `ReferenceResolutionError` shape with `field: 'organization_id'` searched on - * the object itself (the unowned-row refusal of an unscoped load searches - * `sys_organization` instead). - */ -const isNullHolderRefusal = (e: { field: string; sourceObject: string; targetObject: string }) => - e.field === 'organization_id' && e.targetObject === e.sourceObject; +/** Every engine read that names `organization_id` with a NULL value — the retired arm. */ +function spyNullReads(engine: ObjectQL): () => unknown[] { + const seen: unknown[] = []; + const realFind = engine.find.bind(engine); + vi.spyOn(engine, 'find').mockImplementation(async (object: string, query?: any) => { + const where = query?.where; + if (where && typeof where === 'object' && 'organization_id' in where && where.organization_id == null) seen.push({ object, where }); + return realFind(object, query); + }); + return () => seen; +} -describe('scoped seed load over NULL-organization holders (sqlite-file)', () => { - it('refuses each NULL-held natural key by name, inserts no twin, adopts nothing, and still loads the rest', async () => { +describe('scoped seed load on a certified database (sqlite-file)', () => { + it('matches its own rows, inserts no twin, and issues no NULL read', async () => { const { engine, all, plant } = await boot(); - await plantUnowned(plant); - const eventsBefore = await all('tw_event'); - const tasksBefore = await all('tw_task'); - expect(eventsBefore).toHaveLength(2); - expect(tasksBefore).toHaveLength(1); + await plantOwn(plant); + const nullReads = spyNullReads(engine); - const logger = createLogger(); - const svc = new SeedLoaderService(engine as never, metadataFor(OBJECTS), logger as never); + const svc = new SeedLoaderService(engine as never, metadataFor(OBJECTS), createLogger() as never); const result = await svc.load(scopedRequest(ORG) as never); + expect(result.errors).toEqual([]); + expect(nullReads()).toEqual([]); const events = await all('tw_event'); const tasks = await all('tw_task'); - // No twin: the two NULL-held events and the NULL-held task were refused; - // only the keys nothing held ('Retro', 'Send notes') were inserted. + // Only the keys the organization did not hold ('Retro', 'Send notes') were inserted. + expect(result.summary.totalInserted).toBe(2); expect(events).toHaveLength(3); expect(tasks).toHaveLength(2); - expect(result.summary.totalInserted).toBe(2); expect(events.filter((r) => r.subject === 'Kickoff')).toHaveLength(1); - expect(events.filter((r) => r.subject === 'Review')).toHaveLength(1); expect(tasks.filter((r) => r.title === 'Book room')).toHaveLength(1); - // The rows that loaded are the organization's. - expect(events.find((r) => r.subject === 'Retro')?.organization_id).toBe(ORG); - expect(tasks.find((r) => r.title === 'Send notes')?.organization_id).toBe(ORG); - - // Never adopted: each unowned row is byte-for-byte what was planted. - for (const before of [...eventsBefore, ...tasksBefore]) { - const after = [...events, ...tasks].find((r) => r.id === before.id); - expect(after?.organization_id ?? null).toBeNull(); - expect(after).toEqual(before); - } - - // Each refusal is a named per-row error in the seed result. - const refusals = result.errors.filter(isNullHolderRefusal); - expect(refusals.map((e) => `${e.sourceObject}#${e.recordIndex}`).sort()).toEqual(['tw_event#0', 'tw_event#1', 'tw_task#0']); - // The only other errors are the cascade the refusals cause: both tasks - // name an event this organization does not hold ('Kickoff' and 'Review' - // were refused), and a reference never resolves to an unowned row, so - // pass 2 reports each link as dropped. - expect(result.errors.filter((e) => !isNullHolderRefusal(e)).map((e) => `${e.sourceObject}.${e.field}#${e.recordIndex}`).sort()) - .toEqual(['tw_task.event_id#0', 'tw_task.event_id#1']); - expect(tasks.find((r) => r.title === 'Send notes')?.event_id ?? null).toBeNull(); - const kickoff = refusals.find((e) => e.sourceObject === 'tw_event' && e.recordIndex === 0)!; - expect(kickoff.targetObject).toBe('tw_event'); - expect(kickoff.targetField).toBe('subject'); - expect(kickoff.attemptedValue).toBe('Kickoff'); - // The message names the key, the unowned holder and the remedy's command. - expect(kickoff.message).toContain('subject=Kickoff'); - expect(kickoff.message).toContain("'ev_kickoff'"); - expect(kickoff.message).toContain('os migrate organization-ownership --out PLAN'); - expect(kickoff.message).not.toMatch(/fresh installs only|on a 17\.x release|carries the upgrade path/); - expect(result.success).toBe(false); - // Loud: said at `error`, once per dataset. - const lines = logger.error.mock.calls.map((c) => String(c[0])).filter((m) => refusals.some((e) => m.includes(e.message))); - expect(lines).toHaveLength(2); + expect([...events, ...tasks].every((r) => r.organization_id === ORG)).toBe(true); + // The new task's reference resolved to the organization's own event. + expect(tasks.find((r) => r.title === 'Send notes')?.event_id).toBe('ev_review'); }); it('a fully stamped database still loads with 0 inserts and 0 errors', async () => { @@ -249,24 +212,13 @@ describe('scoped seed load over NULL-organization holders (sqlite-file)', () => expect(kickoffs.map((r) => r.organization_id).sort()).toEqual([OTHER_ORG, ORG].sort()); expect(kickoffs.find((r) => r.id === 'ev_other')).toEqual(theirsBefore); }); - - it('an unscoped (single-posture) load keeps its behaviour: it matches the NULL rows and inserts no twin', async () => { - const { engine, all, plant } = await boot(); - await plantUnowned(plant); - const result = await new SeedLoaderService(engine as never, metadataFor(OBJECTS), createLogger() as never) - .load(SeedLoaderRequestSchema.parse({ seeds: SEEDS, config: { defaultMode: 'upsert', multiPass: true } }) as never); - - expect(result.errors).toEqual([]); - expect(result.summary.totalInserted).toBe(2); - expect((await all('tw_event')).filter((r) => r.subject === 'Kickoff')).toHaveLength(1); - expect((await all('tw_task')).filter((r) => r.title === 'Book room')).toHaveLength(1); - }); }); -describe('every scoped caller refuses the NULL-held row (sqlite-file)', () => { +describe('every scoped caller reads the same way (sqlite-file)', () => { it('per-organization replay: the replayer a walled boot registers', async () => { const { engine, all, plant } = await boot(); - await plantUnowned(plant); + await plantOwn(plant); + const nullReads = spyNullReads(engine); const services = new Map([['tenancy', { posture: 'isolated' }]]); const metadata = metadataFor(OBJECTS); const ctx = { @@ -282,9 +234,9 @@ describe('every scoped caller refuses the NULL-held row (sqlite-file)', () => { expect(typeof replayer).toBe('function'); const out = await replayer(ORG); + expect(out.errors).toEqual([]); expect(out.inserted).toBe(2); - expect(out.errors.filter(isNullHolderRefusal).map((e) => `${e.sourceObject}#${e.recordIndex}`).sort()) - .toEqual(['tw_event#0', 'tw_event#1', 'tw_task#0']); + expect(nullReads()).toEqual([]); expect(await all('tw_event')).toHaveLength(3); expect(await all('tw_task')).toHaveLength(2); }); @@ -294,11 +246,13 @@ describe('every scoped caller refuses the NULL-held row (sqlite-file)', () => { // caller's organization. it('walled install and reseed: the request runInlineSeed builds for both', async () => { const { engine, all, plant } = await boot(); - await plantUnowned(plant); + await plantOwn(plant); + const nullReads = spyNullReads(engine); const result = await new SeedLoaderService(engine as never, metadataFor(OBJECTS), createLogger() as never) .load(scopedRequest(ORG) as never); + expect(result.errors).toEqual([]); expect(result.summary.totalInserted).toBe(2); - expect(result.errors.filter(isNullHolderRefusal)).toHaveLength(3); + expect(nullReads()).toEqual([]); expect(await all('tw_event')).toHaveLength(3); expect(await all('tw_task')).toHaveLength(2); }); diff --git a/packages/runtime/src/standalone-stack-seeder-declaration-copy.test.ts b/packages/runtime/src/standalone-stack-seeder-declaration-copy.test.ts index 0558a2b2bb9..b1251cdfd47 100644 --- a/packages/runtime/src/standalone-stack-seeder-declaration-copy.test.ts +++ b/packages/runtime/src/standalone-stack-seeder-declaration-copy.test.ts @@ -489,9 +489,11 @@ describe('#14491 Q1 + Q4 — which copy the seeders consume, and which copy wins active: true, managed_by: 'package', customized: false, - organization_id: null, + // [ADR-0131 D2/D8] The typed deployment-wide marker, never a NULL organization. + deploymentWide: true, }); expect(byName(served, 'share_legacy_deals').criteria).toEqual({ status: 'open' }); + expect(byName(served, 'share_legacy_deals')).not.toHaveProperty('organization_id'); // Three declared rules in, three served, each exactly once: the legacy // rule is served ONE time, not a second time beside a modern spelling. @@ -502,7 +504,7 @@ describe('#14491 Q1 + Q4 — which copy the seeders consume, and which copy wins expect(rows.sys_sharing_rule).toEqual([]); }); - it('the rules served carry the reader-derived shape, organization-less under the `single` posture', () => { + it('the rules served carry the reader-derived shape, deployment-wide under the `single` posture', () => { const modern = byName(served, 'share_modern_deals'); expect(modern).toMatchObject({ name: 'share_modern_deals', @@ -513,8 +515,10 @@ describe('#14491 Q1 + Q4 — which copy the seeders consume, and which copy wins active: true, managed_by: 'package', customized: false, - organization_id: null, + // [ADR-0131 D2/D8] The typed deployment-wide marker, never a NULL organization. + deploymentWide: true, }); expect(modern.criteria).toEqual({ stage: 'won' }); + expect(modern).not.toHaveProperty('organization_id'); }); }); diff --git a/packages/spec/src/contracts/sharing-service.ts b/packages/spec/src/contracts/sharing-service.ts index 6f0eee2ee11..93ab729cacc 100644 --- a/packages/spec/src/contracts/sharing-service.ts +++ b/packages/spec/src/contracts/sharing-service.ts @@ -481,6 +481,15 @@ export type SharingRuleRecipientType = 'user' | 'team' | 'business_unit' | 'posi export interface SharingRuleRow { id: string; organization_id?: string | null; + /** + * [ADR-0131 D2/D8] `true` on a DECLARED rule's projection that sweeps the + * whole deployment — the one projection a declared rule is served as under + * the `single` posture. Such a projection carries no `organization_id`: + * deployment-wide is this typed marker, never a NULL organization. Absent on + * every stored `sys_sharing_rule` row, and on a declared rule's + * per-organization projections under a walled posture. + */ + deploymentWide?: true; name: string; label: string; description?: string | null;