From 718e32e03035c0491061704afa2c3e36a44dd437 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 19:07:40 +0000 Subject: [PATCH 1/2] fix(plugin-security,spec): scope sys_position.name uniqueness per organization (#8468) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The declared index carried bare `unique: true` — the positional spelling of `'global'` on a DECLARED index — so `name` was an installation-wide key on a tenant-scoped object. Measured live before the fix (two organizations, same name): 201 / 409 UNIQUE_VIOLATION / 201 control, with the caller's own GET on the colliding name returning zero rows. A per-value refusal on an unreadable row is a cross-tenant existence oracle, and a plain dead end for an admin who simply wanted to name a position `sales_manager`. Third instance of the class ruled on 2026-08-13, after sys_user_preference and sys_capability (#8461). The hierarchy counter-argument does not arise: positions are deliberately flat (ADR-0090 D3) and this object has no parent_id. Also corrects the published text — the spec `describe()` said "Unique position name" and the reference page is generated from it, so the accident had reached authors as contract — and regenerates the page from source. Migration reuses #8461's `replace_unique_index` arm unchanged: one finding, categorised safe, CREATE before DROP. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- .../sys-position-organization-unique.md | 69 +++ content/docs/references/identity/position.mdx | 2 +- ...r-sys-position-organization-unique.test.ts | 585 ++++++++++++++++++ .../src/objects/sys-position.object.ts | 50 +- .../sys-position.organization-unique.test.ts | 116 ++++ packages/spec/src/identity/position.zod.ts | 10 +- 6 files changed, 827 insertions(+), 5 deletions(-) create mode 100644 .changeset/sys-position-organization-unique.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-sys-position-organization-unique.test.ts create mode 100644 packages/plugins/plugin-security/src/objects/sys-position.organization-unique.test.ts diff --git a/.changeset/sys-position-organization-unique.md b/.changeset/sys-position-organization-unique.md new file mode 100644 index 0000000000..e0d05969af --- /dev/null +++ b/.changeset/sys-position-organization-unique.md @@ -0,0 +1,69 @@ +--- +"@objectstack/plugin-security": patch +"@objectstack/spec": patch +--- + +fix(plugin-security,spec): `sys_position.name` uniqueness is per organization (#8468) + +`sys_position` declared its uniqueness as a table-level index with bare +`unique: true`. At the DECLARED-index level that is the positional spelling of +`'global'` — the listed columns verbatim — so on a tenant-scoped object it +materialized an **installation-wide** unique index. (Field-level `unique: true` +means the opposite, per-organization, and has since #3696; `packages/lint` names +that divergence "the #4986 trap" and warns on it via +`unique/unscoped-declared-index`.) This is the third instance of the class +ruled on 2026-08-13, after `sys_user_preference` and `sys_capability` (#8461). + +Measured live on a real engine before the fix — two organizations, same name, +`OS_TENANCY_POSTURE=isolated`: + +``` +CREATE UNIQUE INDEX uniq_sys_position_name on sys_position (name) + +org_jia POST name=sales_manager → 201 +org_yi POST the SAME name → 409 UNIQUE_VIOLATION +org_yi POST an unused name → 201 +org_yi GET that name → total 0 +``` + +Two consequences. **An organization could not name a position that any other +organization had already used** — `sales_manager` taken installation-wide meant +a permanent, unexplained 409 on a perfectly ordinary name. And because the +refusal is per-value on a row the caller cannot read, it was a **cross-tenant +existence oracle**: an admin could enumerate other organizations' position +vocabulary by reading 409-vs-201. + +The declaration now says `unique: 'organization'` (ADR-0120 D1), materializing +`(COALESCE(organization_id,'__global__'), name)`. Platform-seeded rows +(`bootstrapBuiltinRoles` — `platform_admin`, `org_*`, and the ADR-0090 D9 +audience anchors) carry no organization and the key part is NULL-safe (ADR-0120 +D3), so they stay unique among themselves and the bootstrap upsert-by-name is +unaffected. Same-organization duplicates are still refused — the constraint is +scoped, not removed. + +Positions are deliberately flat (ADR-0090 D3, finalizing ADR-0057 D5), so the +"a hierarchy implies a shared namespace" argument does not arise here: there is +no `parent_id` on this object. + +**Published text.** The field's spec `describe()` said "Unique position name", +and `content/docs/references/identity/position.mdx` is generated from it, so the +docs asserted installation-uniqueness as though it had been intended. The +`describe()` now reads "Position name, unique per organization" and the +reference page is regenerated from it; the object's own field description and +the `clone_position` dialog's help text are corrected to match. + +**Migration.** No new machinery: the `replace_unique_index` retirement that +#8461 generalized to declared indexes covers this object unchanged. Respelling a +declared index changes its generated name, which on a deployed database would +otherwise read as two unrelated findings — the composite missing (safe) and the +old global index orphaned (**destructive**) — letting an operator who applies +only the safe half keep the defect while the plan reads as applied. Instead it +plans as ONE `replace_unique_index` entry categorised `safe`, CREATE before +DROP, with the legacy index dropped only once the replacement is confirmed +present. + +Operators upgrading a deployed database should run `os migrate plan` / `os +migrate apply` — no `--allow-destructive` required. Note that **deploying the +new code is not by itself the fix**: `initObjects` is additive, so until the +retirement is applied the old installation-wide index keeps enforcing (and the +constraint is never unenforced at any point in the migration). diff --git a/content/docs/references/identity/position.mdx b/content/docs/references/identity/position.mdx index 173fc671be..65bedadb78 100644 --- a/content/docs/references/identity/position.mdx +++ b/content/docs/references/identity/position.mdx @@ -61,7 +61,7 @@ const result = PositionSchema.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **name** | `string` | ✅ | Unique position name (lowercase snake_case) | +| **name** | `string` | ✅ | Position name, unique per organization (lowercase snake_case) | | **label** | `string` | ✅ | Display label (e.g. VP of Sales) | | **description** | `string` | optional | | | **delegatable** | `boolean` | ✅ | ADR-0091 D3: holders may self-service delegate this position, time-boxed (default false). | diff --git a/packages/drivers/driver-sql/src/sql-driver-sys-position-organization-unique.test.ts b/packages/drivers/driver-sql/src/sql-driver-sys-position-organization-unique.test.ts new file mode 100644 index 0000000000..c8553cd565 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-sys-position-organization-unique.test.ts @@ -0,0 +1,585 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, afterEach, beforeEach, vi } from 'vitest'; +import { isUniqueViolationError } from '@objectstack/types'; +import { SqlDriver, classifyIndexKeyPart, parseIndexDdl, legacyUniqueReplacements } from '../src/index.js'; + +/** + * #8468 — `sys_position.name`, the THIRD instance of the #8323 class. + * + * ## What was measured here, live, before the fix + * + * The card was filed from a STATIC read and said so; the maintainer ruling of + * 2026-08-13 made the fix conditional on a live probe reproducing the oracle + * first. It does. Driving the real shipped declaration through this driver on + * `OS_TENANCY_POSTURE=isolated`: + * + * ``` + * CREATE UNIQUE INDEX uniq_sys_position_name on sys_position (name) ← installation-wide + * + * org_jia POST name=probe_pos_xtenant → 201 + * org_yi POST the SAME name → 409 UNIQUE_VIOLATION + * org_yi POST an unused name → 201 + * org_yi GET that name → total 0 + * ``` + * + * A per-value refusal on a row the caller cannot read is a cross-tenant + * existence oracle; the 201 control is what makes it an oracle rather than a + * blanket refusal. It is also a plain dead end — the second organization could + * never name a position `sales_manager` if any other organization already had, + * and the 409 does not say why. + * + * ## Why the declaration was global + * + * The same deliberate divergence #8323 documents: a DECLARED index's bare + * `unique: true` is the positional spelling of `'global'` and takes the listed + * columns VERBATIM, while FIELD-level `unique: true` has meant per-organization + * since #3696. `packages/lint/src/data-model-rules.ts` calls this "the #4986 + * trap"; `sys_position` was its third instance in the platform's own metadata, + * after `sys_user_preference` and `sys_capability` (#8461). + * + * The hierarchy counter-argument raised during triage does not apply to this + * object at all: positions are deliberately FLAT (ADR-0090 D3, finalizing + * ADR-0057 D5). There is no `parent_id` on `sys_position` and no position tree, + * so there is no shared-namespace argument to weigh. + * + * ## Why this suite is at the DRIVER level + * + * The driver is the only layer that materializes a unique constraint and the + * only layer that can insert the violating row. `rest-server.ts` maps any error + * satisfying `isUniqueViolationError` to `409 UNIQUE_VIOLATION` and a + * successful create to `201`, so each case asserts the ENVELOPE the API would + * put on the wire — never a bare `.toThrow()`, which a driver throwing a plain + * `Error` would satisfy while REST answered 500. + * + * ## The half that a fresh-database suite cannot see + * + * #8323's most expensive finding was that changing the declaration alone passes + * every behavioural test on a fresh database and leaves every DEPLOYED + * installation still enumerable: respelling changes the index's generated NAME, + * so drift reads as two findings — composite missing (safe, auto-applied) and + * old global index orphaned (destructive, opt-in) — and an operator applying + * only the safe half keeps the defect while the plan reads as applied. The + * `migration on a database built before the respelling` block below is + * therefore the load-bearing part of this file, not an addendum: it builds an + * installation that ALREADY HAS `uniq_sys_position_name` and real rows, then + * migrates it. + */ + +/** The wire shape a duplicate insert must produce, per `rest-server.ts`. */ +const CONFLICT_ENVELOPE = { status: 409, code: 'UNIQUE_VIOLATION' } as const; + +async function createAsApi( + driver: SqlDriver, + object: string, + record: Record, +): Promise<{ status: number; code?: string; row?: any; raw?: unknown }> { + try { + const row = await driver.create(object, record as any); + return { status: 201, row }; + } catch (error) { + if (isUniqueViolationError(error)) return { status: 409, code: 'UNIQUE_VIOLATION', raw: error }; + return { status: 500, code: 'INTERNAL_ERROR', raw: error }; + } +} + +/** Physical columns of `sys_position` that carry the constraint. */ +const POSITION_FIELDS = { + id: { type: 'string' }, + organization_id: { type: 'string' }, + name: { type: 'string' }, + label: { type: 'string' }, + managed_by: { type: 'string' }, + active: { type: 'boolean' }, +} as const; + +/** + * The shipped declaration, reduced to the entry that carries the constraint. + * `sys-position.organization-unique.test.ts` in `plugin-security` asserts the + * real `SysPosition.indexes` is byte-identical to `FIXED_APP`'s, so this + * fixture and the real metadata cannot drift apart silently. (Copying rather + * than importing keeps the package boundary — the same shape #8461 used.) + * + * The `unique: false` on the second entry is not decoration: `ObjectSchema.create` + * normalizes the authored `{ fields: ['active'] }` into that shape, so this is + * what a driver is actually handed at registration. + */ +const PRE_FIX_APP = [ + { + name: 'sys_position', + fields: POSITION_FIELDS, + indexes: [ + { fields: ['name'], unique: true }, // ← the defect + { fields: ['active'], unique: false }, + ], + }, +] as const; + +const FIXED_APP = [ + { + name: 'sys_position', + fields: POSITION_FIELDS, + indexes: [ + { fields: ['name'], unique: 'organization' }, + { fields: ['active'], unique: false }, + ], + }, +] as const; + +describe('#8468 — sys_position.name is unique per organization, not per installation', () => { + let driver: SqlDriver | undefined; + let savedPosture: string | undefined; + let savedMultiOrg: string | undefined; + + const makeDriver = (opts: any = {}) => { + const d = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + ...opts, + }); + (d as any).logger = { warn: vi.fn(), info: vi.fn(), error: vi.fn() }; + driver = d; + return d; + }; + + beforeEach(() => { + savedPosture = process.env.OS_TENANCY_POSTURE; + savedMultiOrg = process.env.OS_MULTI_ORG_ENABLED; + // The posture the probe was run on. ADR-0120's invariant is that no index + // shape reads the posture — `postureIndependence` below pins that — so this + // is context for the reader, not an input the assertions depend on. + process.env.OS_TENANCY_POSTURE = 'isolated'; + process.env.OS_MULTI_ORG_ENABLED = 'true'; + }); + + afterEach(async () => { + await driver?.disconnect(); + driver = undefined; + if (savedPosture === undefined) delete process.env.OS_TENANCY_POSTURE; + else process.env.OS_TENANCY_POSTURE = savedPosture; + if (savedMultiOrg === undefined) delete process.env.OS_MULTI_ORG_ENABLED; + else process.env.OS_MULTI_ORG_ENABLED = savedMultiOrg; + }); + + /** Unique index name → canonical key parts, COALESCE literal elided. */ + async function uniqueKeyParts(table: string): Promise> { + const k = (driver as any).knex; + const list: any = await k.raw(`PRAGMA index_list(${table})`); + const master: any = await k.raw( + `SELECT name, sql FROM sqlite_master WHERE type = 'index' AND tbl_name = ?`, + [table], + ); + const ddlByName = new Map(); + for (const r of Array.isArray(master) ? master : (master?.rows ?? [])) { + if (typeof r?.sql === 'string' && r.sql) ddlByName.set(r.name, r.sql); + } + const out: Record = {}; + for (const idx of list) { + if (idx.origin === 'pk' || idx.unique !== 1) continue; + const parsed = parseIndexDdl(ddlByName.get(idx.name) ?? ''); + if (parsed) { + out[idx.name] = parsed.keyParts.map((p) => { + const part = classifyIndexKeyPart(p); + if (part.kind === 'column') return part.column; + return part.column === null ? p : `COALESCE(${part.column})`; + }); + } else { + const info: any = await k.raw(`PRAGMA index_info("${idx.name}")`); + out[idx.name] = info.map((c: any) => c.name); + } + } + return out; + } + + // ─────────────────────────────────────────────────────────────────────────── + // 1. Materialized shape — both spellings, kept side by side + // ─────────────────────────────────────────────────────────────────────────── + + describe('materialized shape', () => { + it('the fixed declaration keys on the NULL-safe organization part', async () => { + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + + expect(await uniqueKeyParts('sys_position')).toEqual({ + uniq_sys_position_organization_id_name: ['COALESCE(organization_id)', 'name'], + }); + }); + + it('the pre-fix declaration keyed on the bare business column — installation-wide', async () => { + // Kept permanently rather than measured once: this is the contrast that + // makes every "AFTER" assertion below mean something. + const d = makeDriver(); + await d.initObjects(PRE_FIX_APP as any); + + expect(await uniqueKeyParts('sys_position')).toEqual({ + uniq_sys_position_name: ['name'], + }); + }); + }); + + // ─────────────────────────────────────────────────────────────────────────── + // 2. The card's reproduction — the live probe the ruling required + // ─────────────────────────────────────────────────────────────────────────── + + describe('the cross-organization existence oracle', () => { + it('BEFORE: a name held by another organization is refused — 409 on a row you cannot read', async () => { + const d = makeDriver(); + await d.initObjects(PRE_FIX_APP as any); + + const first = await createAsApi(d, 'sys_position', { + id: 'pos1', + organization_id: 'org_jia', + name: 'sales_manager', + label: 'Sales Manager', + }); + expect(first.status).toBe(201); + + const crossOrg = await createAsApi(d, 'sys_position', { + id: 'pos2', + organization_id: 'org_yi', + name: 'sales_manager', + label: 'Sales Manager', + }); + expect(crossOrg).toMatchObject(CONFLICT_ENVELOPE); + + // The control that makes the refusal an ORACLE rather than a blanket + // rejection: an unused name from the same caller is accepted, so the 409 + // is a per-value answer about another tenant's data. + const control = await createAsApi(d, 'sys_position', { + id: 'pos3', + organization_id: 'org_yi', + name: 'position_only_in_b', + label: 'Other', + }); + expect(control.status).toBe(201); + + // …and the other half of the oracle: the caller's own read of the + // colliding name returns nothing. It is refused by a row it cannot see. + const visible = (await d.find('sys_position', {})).filter( + (r: any) => r.organization_id === 'org_yi' && r.name === 'sales_manager', + ); + expect(visible).toHaveLength(0); + }); + + it('AFTER: the same cross-organization create is accepted — 409 flips to 201', async () => { + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + + expect( + (await createAsApi(d, 'sys_position', { + id: 'pos1', organization_id: 'org_jia', name: 'sales_manager', label: 'Sales Manager', + })).status, + ).toBe(201); + expect( + (await createAsApi(d, 'sys_position', { + id: 'pos2', organization_id: 'org_yi', name: 'sales_manager', label: 'Sales Manager', + })).status, + ).toBe(201); + + const held = (await d.find('sys_position', {})).filter((r: any) => r.name === 'sales_manager'); + expect(held.map((r: any) => r.organization_id).sort()).toEqual(['org_jia', 'org_yi']); + }); + + it('AFTER (anti-vacuity): a SAME-organization duplicate is still refused', async () => { + // If this ever goes green-by-acceptance the constraint was REMOVED, not + // scoped — strictly worse than the defect being fixed, and indistinguishable + // from the fix by the 409-flips-to-201 assertion alone. + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + + expect( + (await createAsApi(d, 'sys_position', { + id: 'pos1', organization_id: 'org_jia', name: 'sales_manager', label: 'Sales Manager', + })).status, + ).toBe(201); + const sameOrg = await createAsApi(d, 'sys_position', { + id: 'pos2', organization_id: 'org_jia', name: 'sales_manager', label: 'Duplicate', + }); + expect(sameOrg).toMatchObject(CONFLICT_ENVELOPE); + expect(await d.count('sys_position', {})).toBe(1); + }); + + it('AFTER: platform-seeded rows carry no organization and stay unique among THEMSELVES (D3)', async () => { + // `bootstrapBuiltinRoles` seeds the framework-reserved identity positions + // (platform_admin / org_*) and the ADR-0090 D9 audience anchors + // (everyone / guest) with `managed_by: 'platform'` and no organization. + // A bare `(organization_id, name)` composite would be NULL-DISTINCT under + // SQL, so every seeded position could be duplicated at will; the NULL-safe + // key part is what prevents that, and the bootstrap upsert-by-name relies + // on it. + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + + expect( + (await createAsApi(d, 'sys_position', { + id: 'seed1', name: 'platform_admin', label: 'Platform Admin', managed_by: 'platform', + })).status, + ).toBe(201); + const duplicateSeed = await createAsApi(d, 'sys_position', { + id: 'seed2', name: 'platform_admin', label: 'Platform Admin', managed_by: 'platform', + }); + expect(duplicateSeed).toMatchObject(CONFLICT_ENVELOPE); + + // A tenant may still author its OWN position of the same name — the + // platform bucket and the organization buckets are separate namespaces. + expect( + (await createAsApi(d, 'sys_position', { + id: 'own1', organization_id: 'org_jia', name: 'platform_admin', label: 'Local', managed_by: 'admin', + })).status, + ).toBe(201); + }); + }); + + // ─────────────────────────────────────────────────────────────────────────── + // 3. The deployed-installation half — the #8323 trap, re-run for this object + // ─────────────────────────────────────────────────────────────────────────── + + describe('migration on a database built before the respelling', () => { + /** + * An installation that ALREADY HAS the old global index and real rows — + * i.e. every deployment in the field. A fresh-provision test cannot reach + * any of the assertions in this block. + */ + const seedDeployed = async (d: SqlDriver) => { + await d.initObjects(PRE_FIX_APP as any); + await d.create('sys_position', { + id: 'pos1', organization_id: 'org_jia', name: 'sales_manager', label: 'Sales Manager', managed_by: 'admin', + } as any); + await d.create('sys_position', { + id: 'pos2', organization_id: 'org_jia', name: 'hr_specialist', label: 'HR Specialist', managed_by: 'admin', + } as any); + await d.create('sys_position', { + id: 'seed1', name: 'platform_admin', label: 'Platform Admin', managed_by: 'platform', + } as any); + }; + + it('the seeded database really carries the pre-fix index (harness guard)', async () => { + // Without this the whole block could be exercising a fresh schema and + // every assertion below would still pass. Named as a guard on purpose. + const d = makeDriver(); + await seedDeployed(d); + + expect(await uniqueKeyParts('sys_position')).toEqual({ uniq_sys_position_name: ['name'] }); + expect(await d.count('sys_position', {})).toBe(3); + // …and the defect is live on it, which is what makes migrating it matter. + expect( + await createAsApi(d, 'sys_position', { + id: 'x', organization_id: 'org_yi', name: 'sales_manager', label: 'Sales Manager', + }), + ).toMatchObject(CONFLICT_ENVELOPE); + }); + + it('is planned as ONE pure relaxation, categorised safe — not a destructive orphan drop', async () => { + const d = makeDriver(); + await seedDeployed(d); + // The new metadata arrives (a deploy), same database. + await d.initObjects(FIXED_APP as any); + + const drift = await d.detectManagedDrift(); + expect(drift.filter((e) => e.table === 'sys_position')).toHaveLength(1); + const entry = drift.find( + (e) => e.table === 'sys_position' && e.op.type === 'replace_unique_index', + ); + expect(entry, 'the respelling must be a replacement, not two unrelated findings').toBeDefined(); + expect(entry!.category).toBe('safe'); + expect(entry!.op).toMatchObject({ + dropIndexNames: ['uniq_sys_position_name'], + createIndexName: 'uniq_sys_position_organization_id_name', + createColumns: ['organization_id', 'name'], + nullSafeColumns: ['organization_id'], + }); + + // ⛔ The old index must NOT ALSO surface as an orphan. An orphan drop is + // `destructive`, so an operator applying only the safe half would keep the + // global index — keep the defect — while the plan read as applied. That + // is the exact failure #8323 measured, and it is what this assertion + // exists to prevent from recurring on this object. + expect( + drift.filter( + (e) => e.op.type === 'drop_index' && (e.op as any).indexName === 'uniq_sys_position_name', + ), + ).toHaveLength(0); + }); + + it('applies WITHOUT --allow-destructive, keeps every row, and converges', async () => { + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(FIXED_APP as any); + + const drift = await d.detectManagedDrift(); + const { applied, skipped } = await d.applyMigrationEntries(drift, { allowDestructive: false }); + expect(applied.some((e) => e.op.type === 'replace_unique_index')).toBe(true); + expect(skipped).toHaveLength(0); + + expect(await uniqueKeyParts('sys_position')).toEqual({ + uniq_sys_position_organization_id_name: ['COALESCE(organization_id)', 'name'], + }); + expect(await d.count('sys_position', {})).toBe(3); + + // Re-running finds nothing: the plan is not a drop/create cycle. + expect(await d.detectManagedDrift()).toHaveLength(0); + }); + + it('after applying, BOTH halves hold on the MIGRATED database', async () => { + // The assertion the card is actually about: the fix reaches a deployed + // installation, not merely a freshly provisioned one. + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(FIXED_APP as any); + await d.applyMigrationEntries(await d.detectManagedDrift(), { allowDestructive: false }); + + expect( + (await createAsApi(d, 'sys_position', { + id: 'pos3', organization_id: 'org_yi', name: 'sales_manager', label: 'Sales Manager', + })).status, + ).toBe(201); + + // The anti-vacuity arm, on the SAME migrated index. + expect( + await createAsApi(d, 'sys_position', { + id: 'pos4', organization_id: 'org_jia', name: 'sales_manager', label: 'Duplicate', + }), + ).toMatchObject(CONFLICT_ENVELOPE); + + // …and the platform seed bucket survived the migration intact. + expect( + await createAsApi(d, 'sys_position', { + id: 'seed2', name: 'platform_admin', label: 'Platform Admin', managed_by: 'platform', + }), + ).toMatchObject(CONFLICT_ENVELOPE); + }); + + it('boot creates the replacement ADDITIVELY, so the defect is STILL LIVE until the plan runs', async () => { + // `initObjects` is additive-only: it materializes the newly-declared + // composite at boot and never drops anything. So a deployed installation + // that has taken the new code but not run the plan is still enumerable — + // deploying the respelling is not, by itself, the fix. This is the + // sentence an operator needs, stated as an assertion. + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(FIXED_APP as any); + + expect(Object.keys(await uniqueKeyParts('sys_position')).sort()).toEqual([ + 'uniq_sys_position_name', + 'uniq_sys_position_organization_id_name', + ]); + expect( + await createAsApi(d, 'sys_position', { + id: 'pos3', organization_id: 'org_yi', name: 'sales_manager', label: 'Sales Manager', + }), + ).toMatchObject(CONFLICT_ENVELOPE); + }); + + it('DROP happens only once the replacement is confirmed present', async () => { + // The safety argument in the direction that can actually go wrong: if the + // replacement is not there, the legacy index must be left alone rather + // than dropped into a gap with no uniqueness at all. + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(FIXED_APP as any); + const drift = await d.detectManagedDrift(); + + const k = (d as any).knex; + await k.raw('DROP INDEX uniq_sys_position_organization_id_name'); + (d as any).syncDeclaredIndexes = async () => undefined; + + const { applied, skipped } = await d.applyMigrationEntries(drift, { allowDestructive: false }); + const isPositionReplace = (e: { table: string; op: { type: string } }) => + e.table === 'sys_position' && e.op.type === 'replace_unique_index'; + expect(applied.some(isPositionReplace)).toBe(false); + expect(skipped.some(isPositionReplace)).toBe(true); + + // The pre-migration constraint is intact: never left with neither index. + expect(Object.keys(await uniqueKeyParts('sys_position'))).toEqual(['uniq_sys_position_name']); + }); + }); + + // ─────────────────────────────────────────────────────────────────────────── + // 4. The #8461 guards still hold for this object (A3) + // ─────────────────────────────────────────────────────────────────────────── + + describe('the declared-index replacement arm, exercised on sys_position', () => { + const physicalColumns = new Set(['organization_id', 'id', 'name', 'label', 'managed_by', 'active']); + + it('proposes exactly one retirement, keyed on the listed column', () => { + const [entry, ...rest] = legacyUniqueReplacements({ + table: 'sys_position', + fields: {}, + tenantField: 'organization_id', + physicalColumns, + declaredIndexes: [{ fields: ['name'], unique: 'organization' }, { fields: ['active'] }], + } as any); + expect(rest).toHaveLength(0); + expect(entry).toMatchObject({ + column: 'name', + legacyColumns: ['name'], + legacyNames: ['uniq_sys_position_name'], + replacement: { + name: 'uniq_sys_position_organization_id_name', + columns: ['organization_id', 'name'], + unique: true, + nullSafeColumns: ['organization_id'], + }, + }); + }); + + it('claims nothing for an EXPLICITLY NAMED index — that transition is a recreate', () => { + // #8461 guard 1. If this ever starts proposing a replacement it would ask + // to drop the very index `recreate_index` is rebuilding. + expect( + legacyUniqueReplacements({ + table: 'sys_position', + fields: {}, + tenantField: 'organization_id', + physicalColumns, + declaredIndexes: [{ name: 'uq_position_name', fields: ['name'], unique: 'organization' }], + } as any), + ).toHaveLength(0); + }); + + it('claims nothing when the legacy name IS the replacement name (the S6 composite)', () => { + // #8461 guard 2 — what protects sys_team / sys_business_unit / sys_member. + expect( + legacyUniqueReplacements({ + table: 'sys_position', + fields: {}, + tenantField: 'organization_id', + physicalColumns, + declaredIndexes: [{ fields: ['organization_id', 'name'], unique: 'organization' }], + } as any), + ).toHaveLength(0); + }); + + it('claims nothing for the BARE spelling — an unrespelled declaration is untouched (#5082)', () => { + expect( + legacyUniqueReplacements({ + table: 'sys_position', + fields: {}, + tenantField: 'organization_id', + physicalColumns, + declaredIndexes: [{ fields: ['name'], unique: true }], + } as any), + ).toHaveLength(0); + }); + }); + + // ─────────────────────────────────────────────────────────────────────────── + // 5. ADR-0120: no index shape reads the posture + // ─────────────────────────────────────────────────────────────────────────── + + describe('postureIndependence', () => { + it('materializes the same key parts under single / group / isolated', async () => { + for (const posture of ['single', 'group', 'isolated']) { + process.env.OS_TENANCY_POSTURE = posture; + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + expect(await uniqueKeyParts('sys_position'), `posture=${posture}`).toEqual({ + uniq_sys_position_organization_id_name: ['COALESCE(organization_id)', 'name'], + }); + await d.disconnect(); + driver = undefined; + } + }); + }); +}); diff --git a/packages/plugins/plugin-security/src/objects/sys-position.object.ts b/packages/plugins/plugin-security/src/objects/sys-position.object.ts index bbfed749ff..48182db2cb 100644 --- a/packages/plugins/plugin-security/src/objects/sys-position.object.ts +++ b/packages/plugins/plugin-security/src/objects/sys-position.object.ts @@ -109,7 +109,11 @@ export const SysPosition = ObjectSchema.create({ refreshAfter: true, params: [ { name: 'label', label: 'New Display Name', type: 'text', required: true }, - { name: 'name', label: 'New API Name', type: 'text', required: true, helpText: 'Unique snake_case machine name' }, + // [#8468] The clone dialog is where an admin types a NEW position name, + // so it is the one place the scope has to be right at the moment of + // authoring: the name must be free within THIS organization, not across + // the installation. + { name: 'name', label: 'New API Name', type: 'text', required: true, helpText: 'snake_case machine name, unique per organization' }, { field: 'description', defaultFromRow: true }, { field: 'permissions', defaultFromRow: true }, ], @@ -173,7 +177,12 @@ export const SysPosition = ObjectSchema.create({ required: true, searchable: true, maxLength: 100, - description: 'Unique machine name for the position (e.g. sales_manager, hr_specialist)', + // [#8468] "unique per organization", not "unique". The bare wording + // asserted the installation-wide reading the declared index accidentally + // materialized, so the accident reached admins as contract. + description: + 'Machine name for the position, unique per organization ' + + '(e.g. sales_manager, hr_specialist)', group: 'Identity', }), @@ -267,7 +276,42 @@ export const SysPosition = ObjectSchema.create({ }, indexes: [ - { fields: ['name'], unique: true }, + // [ADR-0120 D1, #8468] `'organization'`, NOT bare `true`. + // + // Positions are admin-authored (`managed_by: 'admin'` by default, created + // in Setup and by the `clone_position` action above), and the object takes + // no `tenancy` opt-out, so `organization_id` is injected. A position name + // is therefore one holder per ORGANIZATION, not one across the whole + // installation. + // + // Bare `true` on a DECLARED index is the positional spelling of `'global'` + // — the listed columns VERBATIM (see `normalizeDeclaredIndex`, which + // prepends the organization key part only for the explicit spelling). It is + // NOT the field-level `true`, which has meant per-organization since #3696; + // that divergence is "the #4986 trap" named in + // `packages/lint/src/data-model-rules.ts`, and this declaration was the + // third instance of it in the platform's own objects. + // + // Measured on a real engine before the fix (two organizations, same name): + // org_jia POST name=probe_pos_xtenant → 201 + // org_yi POST the SAME name → 409 UNIQUE_VIOLATION + // org_yi POST an unused name → 201 + // org_yi GET that name → total 0 + // A per-value refusal on a row the caller cannot read is a cross-tenant + // existence oracle, plus a dead end: the second organization could never + // name a position `sales_manager` if any other organization already had. + // + // The maintainer ruling of 2026-08-13 settles the family: an admin-authored + // name on a tenant-scoped object is scoped per organization. The hierarchy + // counter-argument does not apply here in any case — positions are + // deliberately FLAT (ADR-0090 D3, finalizing ADR-0057 D5); there is no + // `parent_id` on this object and no position tree to share a namespace. + // + // Platform-seeded rows (`bootstrapBuiltinRoles`, `managed_by: 'platform'`) + // carry no organization, and the organization key part is NULL-safe + // (`COALESCE(organization_id, '__global__')`, ADR-0120 D3), so they remain + // unique among THEMSELVES and the bootstrap upsert-by-name is unaffected. + { fields: ['name'], unique: 'organization' }, { fields: ['active'] }, ], diff --git a/packages/plugins/plugin-security/src/objects/sys-position.organization-unique.test.ts b/packages/plugins/plugin-security/src/objects/sys-position.organization-unique.test.ts new file mode 100644 index 0000000000..e82bb0eaa7 --- /dev/null +++ b/packages/plugins/plugin-security/src/objects/sys-position.organization-unique.test.ts @@ -0,0 +1,116 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { IndexSchema } from '@objectstack/spec/data'; +import { resolveInjectedSystemColumns } from '@objectstack/spec/data'; +import { SysPosition } from './sys-position.object'; + +/** + * #8468 — `sys_position.name` is unique per ORGANIZATION, not per installation. + * + * ## What the bare spelling cost + * + * A DECLARED index's `unique: true` is the positional spelling of `'global'` + * (the listed columns verbatim), so `name` was an installation-wide key on a + * tenant-scoped object. Measured live on a real engine before the fix, two + * organizations and the same name: + * + * ``` + * CREATE UNIQUE INDEX uniq_sys_position_name on sys_position (name) + * + * org_jia POST name=sales_manager → 201 + * org_yi POST the SAME name → 409 UNIQUE_VIOLATION + * org_yi POST an unused name → 201 + * org_yi GET that name → total 0 + * ``` + * + * A per-value refusal on a row the caller cannot read is a cross-tenant + * existence oracle; it was also a plain dead end, since the second organization + * could never name a position `sales_manager` and the 409 does not say why. + * + * ## Why per-organization is the CORRECT boundary, not merely the safe one + * + * Positions are admin-authored — `managed_by` defaults to `'admin'`, admins + * create them in Setup and via the `clone_position` action — so two + * organizations naming a position `sales_manager` are not in conflict. The + * maintainer ruling of 2026-08-13 settles the family this way. + * + * The hierarchy counter-argument weighed during triage does not apply to this + * object at all: positions are deliberately FLAT (ADR-0090 D3, finalizing + * ADR-0057 D5). `noPositionHierarchy` below pins that, because the argument for + * an installation-wide namespace would have to come from somewhere, and a + * `parent_id` appearing here later is exactly when someone would revisit it. + * + * Platform-seeded rows (`bootstrapBuiltinRoles`) carry no organization and the + * organization key part is NULL-safe (`COALESCE(organization_id,'__global__')`, + * ADR-0120 D3), so they stay unique among themselves. That behaviour, the + * materialized shape, and the migration of an installation that already carries + * the old index are pinned driver-side in + * `driver-sql/src/sql-driver-sys-position-organization-unique.test.ts`; this + * test pins the declaration that suite's fixture copies. + */ +describe('sys_position — declared uniqueness is organization-scoped (#8468)', () => { + const uniqueIndexes = (SysPosition.indexes ?? []).filter((i: any) => i.unique); + + it('declares exactly one unique index, on (name)', () => { + expect(uniqueIndexes).toHaveLength(1); + expect((uniqueIndexes[0] as any).fields).toEqual(['name']); + }); + + it("spells the scope 'organization' — NOT bare `true`", () => { + // ⛔ Bare `true` here is `'global'` — the installation-wide key that made + // the 409 an oracle over other tenants' rows. Asserted by equality, never + // by truthiness: a truthy check accepts the very spelling that was the bug. + expect((uniqueIndexes[0] as any).unique).toBe('organization'); + }); + + it('is a valid IndexSchema — the spec accepts the explicit vocabulary', () => { + expect(IndexSchema.parse(uniqueIndexes[0])).toMatchObject({ + fields: ['name'], + unique: 'organization', + }); + }); + + it('leaves the non-unique indexes alone', () => { + // Scope discipline: #8468 changes uniqueness scope, nothing else. + expect((SysPosition.indexes ?? []).map((i: any) => [i.fields, i.unique ?? false])).toEqual([ + [['name'], 'organization'], + [['active'], false], + ]); + }); + + it('matches the fixture the driver suite copies, entry for entry', () => { + // The driver suite hand-copies this declaration to keep the package + // boundary; without this assertion the two could drift apart silently and + // the driver suite would go on proving something about a fixture nobody + // ships. Keep both sides in step. + // + // Asserted on the BUILT value, which is what a driver is handed: + // `ObjectSchema.create` normalizes the authored `{ fields: ['active'] }` + // into `{ fields: ['active'], unique: false }`. The source spelling and the + // runtime spelling are not the same object, and a fixture copied from the + // source text alone would be subtly wrong about what the driver sees. + expect(SysPosition.indexes).toEqual([ + { fields: ['name'], unique: 'organization' }, + { fields: ['active'], unique: false }, + ]); + }); + + it('takes no tenancy opt-out, so organization_id is injected (the scope has a column)', () => { + // Derived from the built value, not a regex over source. If this ever goes + // false the `'organization'` spelling has no column to key on and the whole + // fix is inert — a silent failure the index assertions above cannot see. + const plan = resolveInjectedSystemColumns(SysPosition); + expect(SysPosition.tenancy).toBeUndefined(); + expect(plan.tenant).toBe(true); + expect(plan.names.has('organization_id')).toBe(true); + }); + + it('noPositionHierarchy: there is no parent_id, so no shared-namespace argument', () => { + // ADR-0090 D3 / ADR-0057 D5. The triage that produced the ruling treated a + // position hierarchy as the one candidate reason to want an + // installation-wide namespace; this object has none. + expect(Object.keys(SysPosition.fields)).not.toContain('parent_id'); + expect(Object.keys(SysPosition.fields)).not.toContain('parent'); + }); +}); diff --git a/packages/spec/src/identity/position.zod.ts b/packages/spec/src/identity/position.zod.ts index d623667a78..36fa41f9d1 100644 --- a/packages/spec/src/identity/position.zod.ts +++ b/packages/spec/src/identity/position.zod.ts @@ -69,7 +69,15 @@ export const PositionSchema = lazySchema(() => strictObject( }, { /** Identity */ - name: SnakeCaseIdentifierSchema.describe('Unique position name (lowercase snake_case)'), + // [#8468] "unique per organization", not bare "unique". This `describe()` is + // the source of the generated reference page's `name` row + // (`content/docs/references/identity/position.mdx`), so the bare wording + // published the installation-wide reading that `sys_position`'s declared + // index accidentally materialized — as if it had been intended. The ruling of + // 2026-08-13 scopes the name per organization; the text now says so. + name: SnakeCaseIdentifierSchema.describe( + 'Position name, unique per organization (lowercase snake_case)', + ), label: z.string().describe('Display label (e.g. VP of Sales)'), /** Description */ From ad0d564c1e8adf3454b69cdc724a27fdc50834c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 19:53:35 +0000 Subject: [PATCH 2/2] test(plugin-security,driver-sql): state what the fixture pin actually guarantees (#8468) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PM review Q2: the "matches the fixture the driver suite copies" pin claimed the two copies "cannot drift apart silently". It guards ONE direction — shipped declaration moves, driver fixture does not. A driver-side edit is unguarded, because nothing compares the pin's literal to FIXED_APP. Both comments now say which direction is covered and which is not, rather than implying the loop is closed. Comment-only; no assertion changed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- ...r-sys-position-organization-unique.test.ts | 14 +++++++++---- .../sys-position.organization-unique.test.ts | 20 ++++++++++++++++--- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/packages/drivers/driver-sql/src/sql-driver-sys-position-organization-unique.test.ts b/packages/drivers/driver-sql/src/sql-driver-sys-position-organization-unique.test.ts index c8553cd565..ea6087bdb2 100644 --- a/packages/drivers/driver-sql/src/sql-driver-sys-position-organization-unique.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-sys-position-organization-unique.test.ts @@ -95,10 +95,16 @@ const POSITION_FIELDS = { /** * The shipped declaration, reduced to the entry that carries the constraint. - * `sys-position.organization-unique.test.ts` in `plugin-security` asserts the - * real `SysPosition.indexes` is byte-identical to `FIXED_APP`'s, so this - * fixture and the real metadata cannot drift apart silently. (Copying rather - * than importing keeps the package boundary — the same shape #8461 used.) + * Copying rather than importing keeps the package boundary — the shape #8461 + * used. + * + * ⚠️ The copy is guarded in ONE direction only. `sys-position.organization-unique.test.ts` + * in `plugin-security` pins the real `SysPosition.indexes` against its own inline + * literal, so a change to the SHIPPED DECLARATION that is not mirrored here goes + * red over there. The reverse is unguarded: if `FIXED_APP` below is edited and the + * declaration is not, nothing compares them and this suite will go on proving + * something about a fixture nobody ships. Treat this block as hand-maintained, + * and change it only together with the declaration. * * The `unique: false` on the second entry is not decoration: `ObjectSchema.create` * normalizes the authored `{ fields: ['active'] }` into that shape, so this is diff --git a/packages/plugins/plugin-security/src/objects/sys-position.organization-unique.test.ts b/packages/plugins/plugin-security/src/objects/sys-position.organization-unique.test.ts index e82bb0eaa7..1bdeb91f62 100644 --- a/packages/plugins/plugin-security/src/objects/sys-position.organization-unique.test.ts +++ b/packages/plugins/plugin-security/src/objects/sys-position.organization-unique.test.ts @@ -81,9 +81,23 @@ describe('sys_position — declared uniqueness is organization-scoped (#8468)', it('matches the fixture the driver suite copies, entry for entry', () => { // The driver suite hand-copies this declaration to keep the package - // boundary; without this assertion the two could drift apart silently and - // the driver suite would go on proving something about a fixture nobody - // ships. Keep both sides in step. + // boundary (the shape #8461 used). This assertion catches ONE direction of + // drift, and it is worth being exact about which: + // + // caught — the shipped declaration changes and the driver fixture does + // not. This test goes red. Measured: reverting the declaration + // to bare `true` leaves the entire driver suite green at 17/17, + // because that suite never imports `SysPosition`. This is the + // only thing standing between that edit and a silent pass. + // NOT caught — the DRIVER fixture is edited and this declaration is not. + // Nothing compares the literal below to `FIXED_APP`; the two + // copies are only ever checked against this third spelling. + // A driver-side edit drifts silently and this test cannot see + // it. Closing that direction means importing across the + // package boundary, which #8461 deliberately declined. + // + // So: keep both sides in step by hand, and treat the driver fixture as the + // copy that has no guard rather than assuming this pin covers it. // // Asserted on the BUILT value, which is what a driver is handed: // `ObjectSchema.create` normalizes the authored `{ fields: ['active'] }`