From b083ff1c668585fb8eec47f9e11d6570cb9ae846 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 12:55:31 +0000 Subject: [PATCH 1/2] fix(platform-objects,plugin-security,driver-sql): scope sys_user_preference and sys_capability uniqueness per organization Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- .../drivers/driver-sql/src/schema-drift.ts | 92 +- ...ared-index-organization-respelling.test.ts | 799 ++++++++++++++++++ .../src/sql-driver-index-drift.test.ts | 1 + ...driver-index-introspection-failure.test.ts | 1 + .../identity/sys-user-preference.object.ts | 19 +- .../src/objects/sys-capability.object.ts | 16 +- 6 files changed, 920 insertions(+), 8 deletions(-) create mode 100644 packages/drivers/driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts diff --git a/packages/drivers/driver-sql/src/schema-drift.ts b/packages/drivers/driver-sql/src/schema-drift.ts index 6a8bbb1ec5..01b23dbca5 100644 --- a/packages/drivers/driver-sql/src/schema-drift.ts +++ b/packages/drivers/driver-sql/src/schema-drift.ts @@ -1042,6 +1042,20 @@ export interface LegacyUniqueReplacement { column: string; legacyNames: string[]; replacement: ExpectedIndex; + /** + * The EXACT physical key columns the superseded index must have, in key + * order — the shape the replacement relaxes away from. + * + * For a field-level unique this is `[column]`: the pre-#3696 single-column + * global index. For a DECLARED index respelled from the global spelling to + * `'organization'` (#8323) it is the index's listed columns, which may be + * several — `sys_user_preference`'s `(user_id, key)` is the case that put + * this here. Matching on the name alone is not enough (an unrelated index + * may collide with the generated spelling), and matching on a single leading + * column is not enough either: `(user_id, key)` and `(user_id, tenant)` share + * one, and only one of them is the index being replaced. + */ + legacyColumns: string[]; } /** @@ -1092,6 +1106,7 @@ export function legacyUniqueReplacements(args: { const columns = [tenantField, name]; out.push({ column: name, + legacyColumns: [name], legacyNames, // NULL-safe organization key part (ADR-0120 D3). Still a pure // relaxation to create from under the legacy GLOBAL single-column @@ -1105,6 +1120,59 @@ export function legacyUniqueReplacements(args: { }, }); } + + // ── Declared indexes respelled from the global spelling to 'organization' ── + // + // #8323: the same retirement, one level up. A declared index's bare + // `unique: true` is the positional spelling of `'global'` — the listed + // columns VERBATIM — so respelling it `'organization'` changes the + // materialized shape from `(…listed)` to `(COALESCE(tenant,'__global__'), + // …listed)`, and with it the generated NAME. On a deployed database that + // reads as two unrelated findings: the composite is missing (create, safe) + // and the old global index is an orphan (drop, DESTRUCTIVE, opt-in). An + // operator who applies only the safe half keeps the global index — and the + // global index is the defect, so the migration would look applied while the + // cross-organization refusal it exists to remove is still enforced. + // + // Routing it through the SAME `replace_unique_index` op the field-level + // retirement uses states it as what it is: one pure relaxation, categorised + // `safe`, applied CREATE-before-DROP so uniqueness is never unenforced in + // between, and dropping the old index only once the replacement is confirmed + // present. Any two rows colliding on `(tenant, …listed)` already collided on + // `(…listed)`, so the create cannot fail on existing data and no data is lost. + for (const idx of Array.isArray(declaredIndexes) ? declaredIndexes : []) { + if (idx?.unique !== 'organization') continue; + // An EXPLICITLY NAMED index keeps its name across the respelling, so there + // is no second name to retire — same name, new definition, which is + // `recreate_index`'s job (drop-then-create under one name). Emitting a + // replacement here as well would propose dropping the very index the + // recreate is rebuilding. + if (typeof idx?.name === 'string' && idx.name.trim()) continue; + const listed = Array.isArray(idx?.fields) + ? idx.fields.filter((f): f is string => typeof f === 'string' && f.length > 0) + : []; + if (listed.length === 0) continue; + // Every listed column must exist physically, or there is no index to match + // and nothing the replacement could be created from. + if (!listed.every((c) => physicalColumns.has(c))) continue; + const replacement = normalizeDeclaredIndex(table, idx, tenantField); + if (!replacement) continue; + const legacyName = buildIndexName(table, listed, true); + // The S6 hand-written composite already lists the tenant column, so + // `normalizeDeclaredIndex` prepends nothing and the "legacy" name IS the + // current name. Nothing was superseded; the D4 NULL-safe tightening path + // owns that transition. + if (legacyName === replacement.name) continue; + // An index the CURRENT metadata declares is by definition not legacy + // (#3955) — the same guard the field-level arm applies. + if (declaredNames.has(legacyName)) continue; + out.push({ + column: listed[0], + legacyColumns: listed, + legacyNames: [legacyName], + replacement, + }); + } return out; } @@ -1197,13 +1265,25 @@ export function diffManagedIndexes(args: { // ── 1. Legacy platform-wide unique superseded by a tenant composite ── for (const l of legacy) { - // Only a *single-column unique on that very column* is the legacy shape. - // Matching on the name alone would let an unrelated index that happens to - // collide with the legacy spelling be dropped. + // Only a *plain unique on exactly those columns, in key order* is the + // legacy shape. Matching on the name alone would let an unrelated index + // that happens to collide with the legacy spelling be dropped. + // + // `legacyColumns` is `[column]` for the field-level retirement and the + // declared index's listed columns for the #8323 respelling — the same + // question either way, asked once. The plainness guards matter for the + // multi-column arm: an index carrying an expression key part, a NULL-safe + // organization part or a WHERE predicate is NOT the verbatim global shape + // being relaxed, whatever its column identities read as. const present = l.legacyNames.filter((n) => { const p = byName.get(n); if (!p || p.primary || isRuntimeManagedIndex(p, runtimeCreated, tenantField)) return false; - return p.unique && p.columns.length === 1 && p.columns[0] === l.column; + if (!p.unique || p.partial === true) return false; + if ((p.expressions?.length ?? 0) > 0 || (p.nullSafeColumns?.length ?? 0) > 0) return false; + return ( + p.columns.length === l.legacyColumns.length && + p.columns.every((c, i) => c === l.legacyColumns[i]) + ); }); if (present.length === 0) continue; for (const n of present) explained.add(n); @@ -1213,7 +1293,7 @@ export function diffManagedIndexes(args: { table, column: l.column, expected: indexSignature(l.replacement.columns, true, l.replacement.nullSafeColumns), - actual: indexSignature([l.column], true), + actual: indexSignature(l.legacyColumns, true), severity: 'warning', category: 'safe', op: { @@ -1226,7 +1306,7 @@ export function diffManagedIndexes(args: { ...(l.replacement.nullSafeColumns ? { nullSafeColumns: l.replacement.nullSafeColumns } : {}), }, message: - `${table}.${l.column}: a legacy platform-wide UNIQUE index (${present.join(', ')}) still enforces ` + + `${table}.${l.legacyColumns.join('+')}: a legacy platform-wide UNIQUE index (${present.join(', ')}) still enforces ` + `uniqueness across ALL tenants, but metadata scopes it per '${l.replacement.columns[0]}' — a second ` + `tenant reusing the value is rejected on insert (#3696). Replacing it with ${indexSignature(l.replacement.columns, true, l.replacement.nullSafeColumns)} ` + `is a pure relaxation: run "os migrate apply".`, diff --git a/packages/drivers/driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts b/packages/drivers/driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts new file mode 100644 index 0000000000..a1306c44ed --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts @@ -0,0 +1,799 @@ +// 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, + normalizeDeclaredIndex, + uniqueIndexesFromFields, + legacyUniqueReplacements, +} from '../src/index.js'; + +/** + * #8323 — a tenant-scoped object's DECLARED unique index materialized GLOBALLY, + * and the platform's own objects were declared that way. + * + * ## What was measured in the field + * + * On a deployment running `OS_TENANCY_POSTURE=isolated`, with one user who + * belongs to two organizations: + * + * ``` + * as 乙: POST /data/sys_user_preference {user_id, key:"ui.recent"} → 409 UNIQUE_VIOLATION + * as 甲: GET /data/sys_user_preference?filter=["key","=","ui.recent"] → total 0 + * ``` + * + * The row it collided with was invisible to the caller — so the refusal was an + * existence oracle over another tenant's data, and (the live half) the console's + * own "recent items" preference could never persist in a user's SECOND + * organization. `data-objectstack`'s `userState.save()` swallows the failure by + * design, so it failed silently, forever. + * + * ## Why the declaration was global + * + * Not a regression in #4986 — a DELIBERATE divergence between two spellings, + * pinned in `spellings` below. Field-level `unique: true` has meant "per + * organization" since #3696; a DECLARED index's `unique: true` is the positional + * spelling of `'global'` and takes the listed columns VERBATIM. Only the + * explicit `'organization'` prepends the NULL-safe organization key part. + * `packages/lint/src/data-model-rules.ts` calls this "the #4986 trap" and warns + * on it (`unique/unscoped-declared-index`); `sys_user_preference` and + * `sys_capability` were two instances of it in the platform's own metadata. + * + * ## Why this suite is at the DRIVER level + * + * The same argument `adr0120-three-posture-conformance.test.ts` makes: the + * driver is the only layer that materializes a unique constraint, and it is the + * only layer that can insert the violating row. The REST status codes the issue + * reports are a pure function of what happens here — `rest-server.ts` maps any + * error satisfying `isUniqueViolationError` to `409 UNIQUE_VIOLATION` and a + * successful create to `201`. So each case below asserts the ENVELOPE the API + * would produce (`isUniqueViolationError` ⇒ 409 + `code: 'UNIQUE_VIOLATION'`), + * not merely "it threw". + * + * ## Both directions are pinned, deliberately + * + * A fix that made the cross-organization insert succeed by REMOVING uniqueness + * would be a far worse defect than the one being fixed, and it would look + * identical from the 409-flips-to-201 side alone. Every scenario therefore + * carries its anti-vacuity twin: the same-organization duplicate must still be + * refused. `old` fixtures (the pre-#8323 spelling) are kept alongside `new` ones + * so the contrast is a permanent assertion rather than a one-off measurement. + */ + +/** The wire shape a duplicate insert must produce, per `rest-server.ts`. */ +const CONFLICT_ENVELOPE = { status: 409, code: 'UNIQUE_VIOLATION' } as const; + +/** + * Drive a create the way the REST layer does and report the envelope it would + * put on the wire — `{ status: 201 }` for an accepted row, or the + * `409 UNIQUE_VIOLATION` body `rest-server.ts` derives from + * `isUniqueViolationError`. Asserting on this rather than on `.toThrow()` is + * what makes a green run mean the API contract holds: a driver that threw a + * bare `Error` would satisfy `toThrow()` while REST answered `500`. + */ +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 }; + } +} + +describe('#8323 — declared unique indexes on the platform’s tenant-scoped objects', () => { + 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 issue was measured on. ADR-0120's invariant is that no + // index shape reads the posture, so this is context for the reader rather + // than an input the assertions depend on — `postureIndependence` pins that. + 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. WHY the #4986 fix did not cover the table-level declaration + // ───────────────────────────────────────────────────────────────────────── + + describe('the two `unique` spellings diverge by design (the #4986 answer)', () => { + it('a DECLARED index’s bare `true` takes the listed columns verbatim — no tenant column', () => { + const norm = normalizeDeclaredIndex( + 'sys_user_preference', + { fields: ['user_id', 'key'], unique: true }, + 'organization_id', + ); + // This is the pre-#8323 declaration, and this is what it materialized: + // a GLOBAL unique index. The tenant column is offered and not taken. + expect(norm).toEqual({ + name: 'uniq_sys_user_preference_user_id_key', + columns: ['user_id', 'key'], + unique: true, + }); + expect(norm!.nullSafeColumns).toBeUndefined(); + }); + + it('`unique: "global"` is the same shape — bare `true` is its positional spelling', () => { + const bare = normalizeDeclaredIndex('t', { fields: ['a', 'b'], unique: true }, 'organization_id'); + const explicit = normalizeDeclaredIndex('t', { fields: ['a', 'b'], unique: 'global' }, 'organization_id'); + expect(bare).toEqual(explicit); + }); + + it('only the explicit `"organization"` spelling prepends the NULL-safe organization key part', () => { + const norm = normalizeDeclaredIndex( + 'sys_user_preference', + { fields: ['user_id', 'key'], unique: 'organization' }, + 'organization_id', + ); + expect(norm).toEqual({ + name: 'uniq_sys_user_preference_organization_id_user_id_key', + columns: ['organization_id', 'user_id', 'key'], + unique: true, + nullSafeColumns: ['organization_id'], + }); + }); + + it('FIELD-level bare `true` DOES scope per organization — the divergence itself', () => { + // Same two characters, opposite meaning, one level up. An author who + // reads the field-level rule and writes the table-level declaration gets + // the global index silently: that is the whole content of #8323, and it + // is why the fix is a respelling rather than a driver change. + const [fieldLevel] = uniqueIndexesFromFields( + 'sys_user_preference', + { key: { type: 'string', unique: true } }, + 'organization_id', + ); + expect(fieldLevel).toEqual({ + name: 'uniq_sys_user_preference_organization_id_key', + columns: ['organization_id', 'key'], + unique: true, + nullSafeColumns: ['organization_id'], + }); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 2. The platform declarations, materialized + // ───────────────────────────────────────────────────────────────────────── + + /** + * The two objects the ruling names, reduced to the columns and the declared + * `indexes[]` entry that carry the constraint. The `indexes[]` entries are + * byte-identical to the shipped declarations; `sys-user-preference.object.ts` + * and `sys-capability.object.ts` carry pin tests asserting that, so the + * fixture and the real metadata cannot drift apart silently. + */ + const PRE_FIX_APP = [ + { + name: 'sys_user_preference', + fields: { + id: { type: 'string' }, + organization_id: { type: 'string' }, + user_id: { type: 'string' }, + key: { type: 'string' }, + }, + indexes: [ + { fields: ['user_id', 'key'], unique: true }, // ← the defect + { fields: ['user_id'], unique: false }, + ], + }, + { + name: 'sys_capability', + fields: { + id: { type: 'string' }, + organization_id: { type: 'string' }, + name: { type: 'string' }, + }, + indexes: [{ fields: ['name'], unique: true }], // ← the defect + }, + ] as const; + + const FIXED_APP = [ + { + name: 'sys_user_preference', + fields: { + id: { type: 'string' }, + organization_id: { type: 'string' }, + user_id: { type: 'string' }, + key: { type: 'string' }, + }, + indexes: [ + { fields: ['user_id', 'key'], unique: 'organization' }, + { fields: ['user_id'], unique: false }, + ], + }, + { + name: 'sys_capability', + fields: { + id: { type: 'string' }, + organization_id: { type: 'string' }, + name: { type: 'string' }, + }, + indexes: [{ fields: ['name'], unique: 'organization' }], + }, + ] as const; + + describe('materialized shape', () => { + it('the fixed declarations key on the NULL-safe organization part', async () => { + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + + expect(await uniqueKeyParts('sys_user_preference')).toEqual({ + uniq_sys_user_preference_organization_id_user_id_key: [ + 'COALESCE(organization_id)', + 'user_id', + 'key', + ], + }); + expect(await uniqueKeyParts('sys_capability')).toEqual({ + uniq_sys_capability_organization_id_name: ['COALESCE(organization_id)', 'name'], + }); + }); + + it('the pre-fix declarations keyed on the bare business columns — installation-wide', async () => { + const d = makeDriver(); + await d.initObjects(PRE_FIX_APP as any); + + expect(await uniqueKeyParts('sys_user_preference')).toEqual({ + uniq_sys_user_preference_user_id_key: ['user_id', 'key'], + }); + expect(await uniqueKeyParts('sys_capability')).toEqual({ + uniq_sys_capability_name: ['name'], + }); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 3. The card's reproduction — both halves + // ───────────────────────────────────────────────────────────────────────── + + describe('§1/§2 the cross-organization existence oracle', () => { + it('BEFORE: a value 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_capability', { + id: 'c1', + organization_id: 'org_jia', + name: 'probe_cap_xtenant', + }); + expect(first.status).toBe(201); + + const crossOrg = await createAsApi(d, 'sys_capability', { + id: 'c2', + organization_id: 'org_yi', + name: 'probe_cap_xtenant', + }); + expect(crossOrg).toMatchObject(CONFLICT_ENVELOPE); + + // …and the control from the issue: an unused name is accepted, so the + // refusal above is a per-VALUE answer. That is what makes it an oracle. + const control = await createAsApi(d, 'sys_capability', { + id: 'c3', + organization_id: 'org_yi', + name: 'probe_cap_only_in_b', + }); + expect(control.status).toBe(201); + }); + + 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_capability', { id: 'c1', organization_id: 'org_jia', name: 'probe_cap_xtenant' })) + .status, + ).toBe(201); + expect( + (await createAsApi(d, 'sys_capability', { id: 'c2', organization_id: 'org_yi', name: 'probe_cap_xtenant' })) + .status, + ).toBe(201); + + // Both rows exist, each stamped to its own organization. + expect(await d.count('sys_capability', { name: 'probe_cap_xtenant' })).toBe(2); + }); + + 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 — a strictly worse defect than the one #8323 reports. + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + + expect( + (await createAsApi(d, 'sys_capability', { id: 'c1', organization_id: 'org_jia', name: 'manage_users' })) + .status, + ).toBe(201); + const sameOrg = await createAsApi(d, 'sys_capability', { + id: 'c2', + organization_id: 'org_jia', + name: 'manage_users', + }); + expect(sameOrg).toMatchObject(CONFLICT_ENVELOPE); + expect(await d.count('sys_capability', {})).toBe(1); + }); + + it('AFTER: platform-seeded rows carry no organization and stay unique among THEMSELVES (D3)', async () => { + // The NULL-safe key part is what makes this hold: a bare + // `(organization_id, name)` composite would be NULL-distinct under SQL, + // so every platform-seeded capability could be duplicated at will. + // `bootstrapSystemCapabilities` upserts by name and depends on it. + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + + expect((await createAsApi(d, 'sys_capability', { id: 'p1', name: 'manage_metadata' })).status).toBe(201); + const duplicateSeed = await createAsApi(d, 'sys_capability', { id: 'p2', name: 'manage_metadata' }); + expect(duplicateSeed).toMatchObject(CONFLICT_ENVELOPE); + + // An organization may still define its own row of the same name — that + // is the ADR-0066 "admins EXTEND the registry" case, and the reason the + // platform bucket and the organization buckets are separate. + expect( + (await createAsApi(d, 'sys_capability', { id: 'o1', organization_id: 'org_jia', name: 'manage_metadata' })) + .status, + ).toBe(201); + }); + }); + + describe('§3 end to end — a two-organization user’s preferences persist in BOTH', () => { + it('BEFORE: the second organization can never hold a key the first already used', async () => { + const d = makeDriver(); + await d.initObjects(PRE_FIX_APP as any); + + expect( + (await createAsApi(d, 'sys_user_preference', { + id: 'p1', + organization_id: 'org_jia', + user_id: 'zhangsan', + key: 'ui.recent', + })).status, + ).toBe(201); + + const secondOrg = await createAsApi(d, 'sys_user_preference', { + id: 'p2', + organization_id: 'org_yi', + user_id: 'zhangsan', + key: 'ui.recent', + }); + // The measured symptom: refused, invisibly — `userState.save()` swallows + // it, so the preference simply never persisted in the second workspace. + expect(secondOrg).toMatchObject(CONFLICT_ENVELOPE); + }); + + it('AFTER: the same user holds an independent `ui.recent` in each organization', async () => { + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + + const jia = await createAsApi(d, 'sys_user_preference', { + id: 'p1', + organization_id: 'org_jia', + user_id: 'zhangsan', + key: 'ui.recent', + }); + const yi = await createAsApi(d, 'sys_user_preference', { + id: 'p2', + organization_id: 'org_yi', + user_id: 'zhangsan', + key: 'ui.recent', + }); + expect([jia.status, yi.status]).toEqual([201, 201]); + + // Two independent rows, one per organization — the console feature works. + const rows = await d.find('sys_user_preference', { filters: [['key', '=', 'ui.recent']] } as any); + expect(rows.map((r: any) => r.organization_id).sort()).toEqual(['org_jia', 'org_yi']); + }); + + it('AFTER (anti-vacuity): the same key twice in ONE organization is still refused', async () => { + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + + expect( + (await createAsApi(d, 'sys_user_preference', { + id: 'p1', + organization_id: 'org_jia', + user_id: 'zhangsan', + key: 'ui.recent', + })).status, + ).toBe(201); + const duplicate = await createAsApi(d, 'sys_user_preference', { + id: 'p2', + organization_id: 'org_jia', + user_id: 'zhangsan', + key: 'ui.recent', + }); + expect(duplicate).toMatchObject(CONFLICT_ENVELOPE); + + // …and a DIFFERENT user in the same organization is unaffected: the + // constraint still keys on `user_id`, not on the organization alone. + expect( + (await createAsApi(d, 'sys_user_preference', { + id: 'p3', + organization_id: 'org_jia', + user_id: 'lisi', + key: 'ui.recent', + })).status, + ).toBe(201); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 4. The migration — deployed indexes, staged + // ───────────────────────────────────────────────────────────────────────── + + describe('migration on a database built before the respelling', () => { + /** A database carrying the PRE-fix physical shape plus real rows. */ + const seedDeployed = async (d: SqlDriver) => { + await d.initObjects(PRE_FIX_APP as any); + await d.create('sys_user_preference', { + id: 'p1', + organization_id: 'org_jia', + user_id: 'zhangsan', + key: 'ui.recent', + } as any); + await d.create('sys_user_preference', { + id: 'p2', + organization_id: 'org_jia', + user_id: 'zhangsan', + key: 'ui.theme', + } as any); + }; + + 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(); + // ONE finding for the table, not two. The composite was already + // materialized additively at boot, so nothing reports it missing, and the + // legacy index is `explained` by the replacement rather than falling + // through to orphan detection. + expect(drift.filter((e) => e.table === 'sys_user_preference')).toHaveLength(1); + const entry = drift.find( + (e) => e.table === 'sys_user_preference' && 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_user_preference_user_id_key'], + createIndexName: 'uniq_sys_user_preference_organization_id_user_id_key', + createColumns: ['organization_id', 'user_id', 'key'], + nullSafeColumns: ['organization_id'], + }); + + // The old index must NOT be reported as an orphan as well. An orphan drop + // is `destructive`, so an operator applying only the safe half would keep + // the global index — i.e. keep the defect — while the plan read as + // applied. This is the whole reason the declared-index arm exists. + expect( + drift.filter( + (e) => + e.op.type === 'drop_index' && + (e.op as any).indexName === 'uniq_sys_user_preference_user_id_key', + ), + ).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_user_preference')).toEqual({ + uniq_sys_user_preference_organization_id_user_id_key: [ + 'COALESCE(organization_id)', + 'user_id', + 'key', + ], + }); + expect(await d.count('sys_user_preference', {})).toBe(2); + + // 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 () => { + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(FIXED_APP as any); + await d.applyMigrationEntries(await d.detectManagedDrift(), { allowDestructive: false }); + + // The fix: the second organization can now hold the colliding key. + expect( + (await createAsApi(d, 'sys_user_preference', { + id: 'p3', + organization_id: 'org_yi', + user_id: 'zhangsan', + key: 'ui.recent', + })).status, + ).toBe(201); + + // The anti-vacuity arm, on the SAME migrated index. + expect( + await createAsApi(d, 'sys_user_preference', { + id: 'p4', + organization_id: 'org_jia', + user_id: 'zhangsan', + key: 'ui.recent', + }), + ).toMatchObject(CONFLICT_ENVELOPE); + }); + + it('boot creates the replacement ADDITIVELY, so both indexes stand until the plan runs', async () => { + // The staging, stated as an assertion. `initObjects` is additive-only: it + // materializes the newly-declared composite at boot and never drops + // anything. The composite is a pure relaxation of the index already + // there, so the create cannot fail on existing data. What the PLAN then + // owns is only the retirement of the superseded global index — which is + // why the operator-visible step is `safe` and why the constraint is + // continuously enforced across the whole migration. + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(FIXED_APP as any); + + expect(Object.keys(await uniqueKeyParts('sys_user_preference')).sort()).toEqual([ + 'uniq_sys_user_preference_organization_id_user_id_key', + 'uniq_sys_user_preference_user_id_key', + ]); + // Until the retirement is applied the OLD index is still enforcing, so + // the defect is still live — the fix is not complete at boot. + expect( + await createAsApi(d, 'sys_user_preference', { + id: 'p3', + organization_id: 'org_yi', + user_id: 'zhangsan', + key: 'ui.recent', + }), + ).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(); + + // Simulate a replacement that is not present and cannot be created (the + // real cause is `syncDeclaredIndexes` skipping an index whose column was + // never materialized). + const k = (d as any).knex; + await k.raw('DROP INDEX uniq_sys_user_preference_organization_id_user_id_key'); + (d as any).syncDeclaredIndexes = async () => undefined; + + const { applied, skipped } = await d.applyMigrationEntries(drift, { allowDestructive: false }); + const isPreferenceReplace = (e: { table: string; op: { type: string } }) => + e.table === 'sys_user_preference' && e.op.type === 'replace_unique_index'; + expect(applied.some(isPreferenceReplace)).toBe(false); + expect(skipped.some(isPreferenceReplace)).toBe(true); + + // The pre-migration constraint is intact: the database is never left + // with neither index. + expect(Object.keys(await uniqueKeyParts('sys_user_preference'))).toEqual([ + 'uniq_sys_user_preference_user_id_key', + ]); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 5. Guards on the new declared-index arm + // ───────────────────────────────────────────────────────────────────────── + + describe('what the declared-index replacement must NOT claim', () => { + const physicalColumns = new Set(['organization_id', 'user_id', 'key', 'name']); + + it('proposes the retirement for an unnamed `organization` index', () => { + const [entry, ...rest] = legacyUniqueReplacements({ + table: 'sys_user_preference', + fields: {}, + tenantField: 'organization_id', + physicalColumns, + declaredIndexes: [{ fields: ['user_id', 'key'], unique: 'organization' }], + }); + expect(rest).toHaveLength(0); + expect(entry).toMatchObject({ + legacyColumns: ['user_id', 'key'], + legacyNames: ['uniq_sys_user_preference_user_id_key'], + replacement: { name: 'uniq_sys_user_preference_organization_id_user_id_key' }, + }); + }); + + it('claims nothing for an EXPLICITLY NAMED index — that transition is a recreate', () => { + // The name does not change, so there is no second index to retire. + // Proposing one would ask to drop the very index being rebuilt. + expect( + legacyUniqueReplacements({ + table: 'sys_user_preference', + fields: {}, + tenantField: 'organization_id', + physicalColumns, + declaredIndexes: [ + { name: 'uniq_pref_user_key', fields: ['user_id', 'key'], unique: 'organization' }, + ], + }), + ).toEqual([]); + }); + + it('claims nothing for the S6 hand-written composite (the legacy name IS the current name)', () => { + expect( + legacyUniqueReplacements({ + table: 'sys_user_preference', + fields: {}, + tenantField: 'organization_id', + physicalColumns, + declaredIndexes: [{ fields: ['user_id', 'organization_id'], unique: 'organization' }], + }), + ).toEqual([]); + }); + + it('claims nothing when metadata ALSO declares the global index under that name (#3955)', () => { + // Declaring both scopes is a lint contradiction, not a licence to drop + // the one the author is still asking for. + expect( + legacyUniqueReplacements({ + table: 'sys_user_preference', + fields: {}, + tenantField: 'organization_id', + physicalColumns, + declaredIndexes: [ + { fields: ['user_id', 'key'], unique: 'organization' }, + { fields: ['user_id', 'key'], unique: 'global' }, + ], + }), + ).toEqual([]); + }); + + it('claims nothing for a bare `true` declaration — the bare spelling is untouched (#5082)', () => { + // #8323 respells two platform objects; it does NOT reinterpret bare + // `true`. That question belongs to #5082 and stays there. + expect( + legacyUniqueReplacements({ + table: 'sys_user_preference', + fields: {}, + tenantField: 'organization_id', + physicalColumns, + declaredIndexes: [{ fields: ['user_id', 'key'], unique: true }], + }), + ).toEqual([]); + }); + + it('does not fire on a table with no tenant column', () => { + expect( + legacyUniqueReplacements({ + table: 'sys_user_preference', + fields: {}, + tenantField: null, + physicalColumns, + declaredIndexes: [{ fields: ['user_id', 'key'], unique: 'organization' }], + }), + ).toEqual([]); + }); + + it('leaves a same-named index alone when its COLUMNS are not the shape being replaced', async () => { + // Name matching alone is not enough: a physical index that happens to + // carry the generated spelling but keys on other columns is somebody + // else's index, and dropping it would be a pure mistake. + const d = makeDriver(); + await d.initObjects([ + { + name: 'sys_user_preference', + fields: { + id: { type: 'string' }, + organization_id: { type: 'string' }, + user_id: { type: 'string' }, + key: { type: 'string' }, + }, + }, + ] as any); + const k = (d as any).knex; + await k.raw('CREATE UNIQUE INDEX uniq_sys_user_preference_user_id_key ON sys_user_preference (user_id)'); + await d.initObjects(FIXED_APP as any); + + const drift = await d.detectManagedDrift(); + expect( + drift.filter((e) => e.table === 'sys_user_preference' && e.op.type === 'replace_unique_index'), + ).toHaveLength(0); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 6. ADR-0120's invariant still holds for the new shape + // ───────────────────────────────────────────────────────────────────────── + + describe('postureIndependence', () => { + it('materializes the same key parts under single / group / isolated', async () => { + const shapes: Record> = {}; + for (const posture of ['single', 'group', 'isolated'] as const) { + process.env.OS_TENANCY_POSTURE = posture; + if (posture === 'single') delete process.env.OS_MULTI_ORG_ENABLED; + else process.env.OS_MULTI_ORG_ENABLED = 'true'; + + const d = makeDriver(); + await d.initObjects(FIXED_APP as any); + shapes[posture] = await uniqueKeyParts('sys_user_preference'); + await d.disconnect(); + driver = undefined; + } + // Positively asserted, not merely "all three agree" — three empty maps + // would satisfy sameness alone. + expect(shapes.single).toEqual({ + uniq_sys_user_preference_organization_id_user_id_key: [ + 'COALESCE(organization_id)', + 'user_id', + 'key', + ], + }); + expect(shapes.group).toEqual(shapes.single); + expect(shapes.isolated).toEqual(shapes.single); + }); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver-index-drift.test.ts b/packages/drivers/driver-sql/src/sql-driver-index-drift.test.ts index 4c363bae7e..6d72cd14c6 100644 --- a/packages/drivers/driver-sql/src/sql-driver-index-drift.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-index-drift.test.ts @@ -483,6 +483,7 @@ describe('SqlDriver index drift (#3728)', () => { legacy: [ { column: 'code', + legacyColumns: ['code'], legacyNames: ['product_code_unique'], replacement: { name: 'uniq_product_organization_id_code', columns: ['organization_id', 'code'], unique: true }, }, diff --git a/packages/drivers/driver-sql/src/sql-driver-index-introspection-failure.test.ts b/packages/drivers/driver-sql/src/sql-driver-index-introspection-failure.test.ts index 4fbc065a0a..321892ad40 100644 --- a/packages/drivers/driver-sql/src/sql-driver-index-introspection-failure.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-index-introspection-failure.test.ts @@ -199,6 +199,7 @@ describe('index introspection failure is not "no indexes" (#7332)', () => { legacy: [ { column: 'code', + legacyColumns: ['code'], legacyNames: ['product_code_unique'], replacement: { name: 'uniq_product_organization_id_code', columns: ['organization_id', 'code'], unique: true as const }, }, diff --git a/packages/platform-objects/src/identity/sys-user-preference.object.ts b/packages/platform-objects/src/identity/sys-user-preference.object.ts index 7b75a1979f..49874eceea 100644 --- a/packages/platform-objects/src/identity/sys-user-preference.object.ts +++ b/packages/platform-objects/src/identity/sys-user-preference.object.ts @@ -104,7 +104,24 @@ export const SysUserPreference = ObjectSchema.create({ }, indexes: [ - { fields: ['user_id', 'key'], unique: true }, + // [ADR-0120 D1, #8323] `'organization'`, NOT bare `true`. + // + // A preference belongs to a (user, organization) pair, not to a user + // globally: the same person in two organizations keeps two independent + // `ui.recent` rows. 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 one of its two instances in the platform's own objects. + // + // Measured consequence of the bare spelling: a user belonging to two + // organizations could never persist a key they already used in the first + // one — the write was refused by an index whose colliding row they cannot + // read, and `data-objectstack`'s `userState.save()` swallows the failure by + // design, so the preference silently stopped persisting. + { fields: ['user_id', 'key'], unique: 'organization' }, { fields: ['user_id'], unique: false }, ], diff --git a/packages/plugins/plugin-security/src/objects/sys-capability.object.ts b/packages/plugins/plugin-security/src/objects/sys-capability.object.ts index b7593d60b8..8f7e80ffb8 100644 --- a/packages/plugins/plugin-security/src/objects/sys-capability.object.ts +++ b/packages/plugins/plugin-security/src/objects/sys-capability.object.ts @@ -203,7 +203,21 @@ export const SysCapability = ObjectSchema.create({ }, indexes: [ - { fields: ['name'], unique: true }, + // [ADR-0120 D1, #8323] `'organization'`, NOT bare `true`. + // + // Admins EXTEND this registry in Setup (`managed_by: 'admin'`, `scope: + // 'org'`), so a capability name is one holder per organization, not one + // across the installation. Bare `true` on a DECLARED index is the + // positional spelling of `'global'` (listed columns verbatim), which made + // `name` an installation-wide key: an organization could probe whether ANY + // other organization — or the platform seed — already held a name, by + // reading 409-vs-201 on a row it has no permission to see. + // + // Platform-seeded rows 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 `bootstrapSystemCapabilities`' + // upsert-by-name is unaffected. + { fields: ['name'], unique: 'organization' }, { fields: ['scope'] }, { fields: ['active'] }, // [ADR-0086 D3] uninstall/upgrade query: "this package's own capabilities". From b531b65ead9113359b27b866482fbc89339419c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 13:34:48 +0000 Subject: [PATCH 2/2] test(platform-objects,plugin-security): pin the organization-scoped declarations; changeset Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- .../tenant-scoped-platform-object-uniques.md | 54 ++++++++++++ ...ared-index-organization-respelling.test.ts | 12 ++- ...ser-preference.organization-unique.test.ts | 83 +++++++++++++++++++ .../src/objects/rbac-objects.test.ts | 11 ++- ...sys-capability.organization-unique.test.ts | 66 +++++++++++++++ 5 files changed, 221 insertions(+), 5 deletions(-) create mode 100644 .changeset/tenant-scoped-platform-object-uniques.md create mode 100644 packages/platform-objects/src/identity/sys-user-preference.organization-unique.test.ts create mode 100644 packages/plugins/plugin-security/src/objects/sys-capability.organization-unique.test.ts diff --git a/.changeset/tenant-scoped-platform-object-uniques.md b/.changeset/tenant-scoped-platform-object-uniques.md new file mode 100644 index 0000000000..56ea658ef4 --- /dev/null +++ b/.changeset/tenant-scoped-platform-object-uniques.md @@ -0,0 +1,54 @@ +--- +"@objectstack/platform-objects": patch +"@objectstack/plugin-security": patch +"@objectstack/driver-sql": patch +--- + +fix(platform-objects,plugin-security,driver-sql): `sys_user_preference` and `sys_capability` uniqueness is per organization (#8323) + +Both objects declared their 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`.) Measured on a deployment running +`OS_TENANCY_POSTURE=isolated`: + +- **A user in two organizations could never persist a preference key they had + already used in the first one.** `sys_user_preference`'s `(user_id, key)` was + installation-wide, so the second organization's write was refused by a row the + caller cannot read — and `data-objectstack`'s `userState.save()` swallows the + failure by design, so "recent items" and similar preferences silently stopped + persisting in a user's second workspace, with no error anywhere. +- **`sys_capability.name` refusals were an existence oracle across tenants.** An + organization could POST a name and read `409` vs `201` to learn whether some + other organization — or the platform seed — already held it, while its own + `GET` on that name returned zero rows. + +Both declarations now say `unique: 'organization'` (ADR-0120 D1), materializing +`(COALESCE(organization_id,'__global__'), …)`. Platform-seeded rows carry no +organization and the key part is NULL-safe (ADR-0120 D3), so they stay unique +among themselves and `bootstrapSystemCapabilities`' upsert-by-name is unaffected. +Same-organization duplicates are still refused — the constraint is scoped, not +removed. + +The bare `unique: true` spelling itself is **unchanged**; whether it should be +reinterpreted is #5082 (v18), and the publish-time authoring advisory is #8379. + +**Migration (`@objectstack/driver-sql`).** Respelling a declared index changes +its generated name, which on a deployed database read as two unrelated findings: +the composite missing (`create_index`, safe) and the old global index orphaned +(`drop_index`, **destructive**). An operator applying only the safe half would +have kept the global index — i.e. kept the defect — while the plan read as +applied. The declared-index respelling now routes through the same +`replace_unique_index` retirement the field-level `unique` migration has used +since #3728: one finding, categorised `safe`, CREATE before DROP, and the legacy +index dropped only once the replacement is confirmed present. Any two rows +colliding on `(organization, …fields)` already collided on `(…fields)`, so the +replacement can neither fail on existing data nor lose any. + +Operators upgrading a deployed database should run `os migrate plan` / `os +migrate apply` — no `--allow-destructive` is required. Until the retirement is +applied the old index keeps enforcing, so the constraint is never unenforced at +any point in the migration. diff --git a/packages/drivers/driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts b/packages/drivers/driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts index a1306c44ed..7bf1b751b2 100644 --- a/packages/drivers/driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts @@ -352,7 +352,8 @@ describe('#8323 — declared unique indexes on the platform’s tenant-scoped ob ).toBe(201); // Both rows exist, each stamped to its own organization. - expect(await d.count('sys_capability', { name: 'probe_cap_xtenant' })).toBe(2); + const held = (await d.find('sys_capability', {})).filter((r: any) => r.name === 'probe_cap_xtenant'); + 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 () => { @@ -440,8 +441,13 @@ describe('#8323 — declared unique indexes on the platform’s tenant-scoped ob expect([jia.status, yi.status]).toEqual([201, 201]); // Two independent rows, one per organization — the console feature works. - const rows = await d.find('sys_user_preference', { filters: [['key', '=', 'ui.recent']] } as any); - expect(rows.map((r: any) => r.organization_id).sort()).toEqual(['org_jia', 'org_yi']); + const rows = await d.find('sys_user_preference', {}); + expect( + rows + .filter((r: any) => r.key === 'ui.recent') + .map((r: any) => r.organization_id) + .sort(), + ).toEqual(['org_jia', 'org_yi']); }); it('AFTER (anti-vacuity): the same key twice in ONE organization is still refused', async () => { diff --git a/packages/platform-objects/src/identity/sys-user-preference.organization-unique.test.ts b/packages/platform-objects/src/identity/sys-user-preference.organization-unique.test.ts new file mode 100644 index 0000000000..8ea45966da --- /dev/null +++ b/packages/platform-objects/src/identity/sys-user-preference.organization-unique.test.ts @@ -0,0 +1,83 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { lintUnscopedDeclaredIndexes, UNIQUE_UNSCOPED_DECLARED_INDEX } from '@objectstack/lint'; +import { IndexSchema } from '@objectstack/spec/data'; +import { SysUserPreference } from './sys-user-preference.object'; + +/** + * #8323 — `sys_user_preference`'s `(user_id, key)` uniqueness is per + * ORGANIZATION, and the spelling that says so is the explicit one. + * + * ## Why this pin exists as a literal + * + * The two `unique` spellings mean opposite things at the two levels: a + * FIELD-level `unique: true` has been per-organization since #3696, while a + * DECLARED index's `unique: true` is the positional spelling of `'global'` and + * materializes the listed columns verbatim. `packages/lint` names that "the + * #4986 trap", and this declaration was one of its two instances in the + * platform's own metadata — measured in production as: a user belonging to two + * organizations could never persist a preference key they had already used in + * the first one, silently, because `userState.save()` swallows the refusal. + * + * Reverting this one word would restore that bug with no other visible change, + * so it is pinned by literal rather than by "is unique in some sense". + * + * The driver-side behaviour — that this declaration materializes + * `(COALESCE(organization_id,'__global__'), user_id, key)`, that a + * cross-organization write is accepted and a same-organization duplicate is + * still refused — is pinned in + * `driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts`, + * whose fixture copies the `indexes[]` entry asserted here. This test is what + * keeps the copy honest. + */ +describe('sys_user_preference — declared uniqueness is organization-scoped (#8323)', () => { + const uniqueIndexes = (SysUserPreference.indexes ?? []).filter((i: any) => i.unique); + + it('declares exactly one unique index, on (user_id, key)', () => { + expect(uniqueIndexes).toHaveLength(1); + expect(uniqueIndexes[0].fields).toEqual(['user_id', 'key']); + }); + + it("spells the scope 'organization' — NOT bare `true`", () => { + // ⛔ Bare `true` here is `'global'`: one holder across the whole + // installation, which is the defect #8323 reports. `'global'` is equally + // wrong for this object and equally rejected by this assertion. + expect(uniqueIndexes[0].unique).toBe('organization'); + }); + + it('is a valid IndexSchema — the spec accepts the explicit vocabulary', () => { + expect(IndexSchema.parse(uniqueIndexes[0])).toMatchObject({ + fields: ['user_id', 'key'], + unique: 'organization', + }); + }); + + it('reports no `unique/unscoped-declared-index` finding', () => { + // The rule that would have caught this at authoring time. It fires on the + // bare spelling alone, so a green result here IS the statement that no + // declared index on this object leaves its scope unstated. + const findings = lintUnscopedDeclaredIndexes([SysUserPreference as any]); + expect(findings.filter((f) => f.rule === UNIQUE_UNSCOPED_DECLARED_INDEX)).toEqual([]); + }); + + it('the rule DOES fire on the pre-fix spelling — this pin is not vacuous', () => { + // Anti-vacuity: proves the assertion above can fail. A lint rule that + // reported nothing for every input would make the previous test green + // while the trap was wide open. + const preFix = { + ...(SysUserPreference as any), + indexes: [{ fields: ['user_id', 'key'], unique: true }, { fields: ['user_id'], unique: false }], + }; + const findings = lintUnscopedDeclaredIndexes([preFix]); + expect(findings.filter((f) => f.rule === UNIQUE_UNSCOPED_DECLARED_INDEX)).toHaveLength(1); + }); + + it('leaves the non-unique index alone', () => { + // Scope discipline: #8323 changes uniqueness scope, nothing else. + expect((SysUserPreference.indexes ?? []).map((i: any) => [i.fields, i.unique ?? false])).toEqual([ + [['user_id', 'key'], 'organization'], + [['user_id'], false], + ]); + }); +}); diff --git a/packages/plugins/plugin-security/src/objects/rbac-objects.test.ts b/packages/plugins/plugin-security/src/objects/rbac-objects.test.ts index fc046f455c..608ab4cbf1 100644 --- a/packages/plugins/plugin-security/src/objects/rbac-objects.test.ts +++ b/packages/plugins/plugin-security/src/objects/rbac-objects.test.ts @@ -240,8 +240,15 @@ describe('sys_capability — ADR-0066 D1 capability registry', () => { expect(mbOpts).toEqual(['admin', 'package', 'platform']); }); - it('enforces a unique index on name', () => { + it('enforces a unique index on name, scoped per organization', () => { const nameIdx = (SysCapability.indexes ?? []).find((i: any) => Array.isArray(i.fields) && i.fields.includes('name')); - expect(nameIdx?.unique).toBe(true); + // [#8323] Was `toBe(true)`. On a DECLARED index bare `true` is the + // positional spelling of `'global'` — the listed columns verbatim — which + // made `name` an installation-wide key on a tenant-scoped object and turned + // its 409 into a cross-tenant existence oracle. The assertion is respelled + // rather than relaxed: a truthiness check here would accept the very + // spelling that was the defect. Full contract in + // `sys-capability.organization-unique.test.ts`. + expect(nameIdx?.unique).toBe('organization'); }); }); diff --git a/packages/plugins/plugin-security/src/objects/sys-capability.organization-unique.test.ts b/packages/plugins/plugin-security/src/objects/sys-capability.organization-unique.test.ts new file mode 100644 index 0000000000..91397d8be4 --- /dev/null +++ b/packages/plugins/plugin-security/src/objects/sys-capability.organization-unique.test.ts @@ -0,0 +1,66 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { IndexSchema } from '@objectstack/spec/data'; +import { SysCapability } from './sys-capability.object'; + +/** + * #8323 — `sys_capability.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 on an `isolated` deployment: an organization + * could POST a capability name and read `409` vs `201` to learn whether ANY + * other organization — or the platform seed — already held it, while its own + * `GET` on that name returned `total 0`. A refusal on a row you are not + * permitted to see is an existence oracle, and under the posture sold as + * "legal-entity / sovereignty isolation" it crosses the wall it advertises. + * + * ADR-0066 D1 is the reason per-organization is the CORRECT boundary and not + * merely the safe one: the platform DEFINES capabilities and admins EXTEND them + * in Setup (`managed_by: 'admin'`, `scope: 'org'`), so two organizations + * naming a capability the same way are not in conflict. + * + * Platform-seeded rows carry no organization and the organization key part is + * NULL-safe (`COALESCE(organization_id,'__global__')`, ADR-0120 D3), so they + * remain unique among themselves — which is what + * `bootstrapSystemCapabilities`' upsert-by-name relies on. That behaviour is + * pinned driver-side in + * `driver-sql/src/sql-driver-declared-index-organization-respelling.test.ts`; + * this test pins the declaration its fixture copies. + */ +describe('sys_capability — declared uniqueness is organization-scoped (#8323)', () => { + const uniqueIndexes = (SysCapability.indexes ?? []).filter((i: any) => i.unique); + + it('declares exactly one unique index, on (name)', () => { + expect(uniqueIndexes).toHaveLength(1); + expect(uniqueIndexes[0].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. + expect(uniqueIndexes[0].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: #8323 changes uniqueness scope, nothing else. The + // `package_id` index is ADR-0086 D3's uninstall/upgrade query. + expect((SysCapability.indexes ?? []).map((i: any) => [i.fields, i.unique ?? false])).toEqual([ + [['name'], 'organization'], + [['scope'], false], + [['active'], false], + [['package_id'], false], + ]); + }); +});