From 0b853f5859ee5b4f5d56ea9444c1cb440547b676 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Fri, 14 Aug 2026 02:09:17 +0000 Subject: [PATCH 1/4] fix(#8577): scope two more tenant-scoped declared unique indexes per organization Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- ...ant-scoped-declared-unique-indexes-8577.md | 105 +++ content/docs/permissions/permission-sets.mdx | 6 +- ...8577-tenant-scoped-declared-unique.test.ts | 722 ++++++++++++++++++ packages/plugins/plugin-security/package.json | 2 + .../sys-audience-binding-suggestion.object.ts | 28 +- ...ing-suggestion.organization-unique.test.ts | 110 +++ ...ted-audience-bindings-install-path.test.ts | 368 +++++++++ .../notification-subscription.object.ts | 29 +- ...n-subscription.organization-unique.test.ts | 116 +++ pnpm-lock.yaml | 6 + 10 files changed, 1487 insertions(+), 5 deletions(-) create mode 100644 .changeset/tenant-scoped-declared-unique-indexes-8577.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-8577-tenant-scoped-declared-unique.test.ts create mode 100644 packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.organization-unique.test.ts create mode 100644 packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts create mode 100644 packages/services/service-messaging/src/objects/notification-subscription.organization-unique.test.ts diff --git a/.changeset/tenant-scoped-declared-unique-indexes-8577.md b/.changeset/tenant-scoped-declared-unique-indexes-8577.md new file mode 100644 index 0000000000..66b281ce21 --- /dev/null +++ b/.changeset/tenant-scoped-declared-unique-indexes-8577.md @@ -0,0 +1,105 @@ +--- +"@objectstack/plugin-security": patch +"@objectstack/service-messaging": patch +--- + +fix(plugin-security,service-messaging): two more tenant-scoped declared unique indexes become per-organization (#8577) + +Two platform 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 each +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`.) These are the fifth act of the class ruled on +2026-08-13, after `sys_user_preference` / `sys_capability` (#8461), +`sys_position` (#8556) and the five of #8554. + +| object | package | was | now | +|---|---|---|---| +| `sys_notification_subscription` | `service-messaging` | `[topic, principal]` global | same, per organization | +| `sys_audience_binding_suggestion` | `plugin-security` | `[package_id, permission_set_name, anchor]` global | same, per organization | + +Measured live on a real engine before the fix — two organizations, the same key, +`OS_TENANCY_POSTURE=isolated`, driving the real shipped declarations. Both +reproduced identically: + +``` +org_jia POST the key → 201 +org_yi POST the SAME → 409 UNIQUE_VIOLATION +org_yi POST an unused → 201 ← the control that makes it an oracle +org_yi GET the key → total 0 ← refused by a row it cannot see +``` + +`sys_notification_subscription` is the class's usual shape and the direct sibling +of `sys_notification_preference`: a user belonging to two organizations could not +subscribe to the same topic in both, and since `role:x` / `team:x` principal +names are themselves per-organization, the same string denoted different +subscribers while colliding on one installation-wide key. + +`sys_audience_binding_suggestion` is **more serious, and it is not a naming +collision at all.** Its key is the owning package's id, the package's own +permission-set name and the anchor — the same triple for every tenant that +installs the same package — while the row is per-tenant by construction +(ADR-0090 D5/D9: raised when a package's `isDefault` set is observed, resolved +when a tenant admin confirms). So the second and every later organization to +install a package never got its suggestion row: its admins were never prompted to +bind the package's default permission set, its users never received that set, and +nothing reported it — the reconciler cannot distinguish the cross-tenant UNIQUE +violation from the benign concurrent-sync race its `catch` was written for. Both +halves are now pinned end to end: two organizations installing the same package +each end up with their own pending row, and re-running one organization's sync +still adds nothing. + +## ⚠️ Operators: a migration is REQUIRED, and deploying this release is not it + +Respelling a declared index changes its generated **name**. On an existing +database `initObjects` is additive: it creates the new per-organization composite +at boot and **never drops the old global index**, which goes on enforcing. Until +the retirement is applied, a deployed installation that has taken this release +still refuses the second organization's row — that is asserted as a test, not +assumed. + +Run the migration: + +``` +os migrate plan # shows one `replace_unique_index` per object, categorised `safe` +os migrate apply # no --allow-destructive needed +``` + +Each object plans as **one pure relaxation**, not as two findings. That matters: +if it read as "composite missing" (safe) plus "old global index orphaned" +(destructive, opt-in), an operator applying only the safe half would keep the +global index — keep the defect — while the plan read as applied. The #8461 +`replace_unique_index` arm covers both unchanged (no driver change in this +release), applies CREATE-before-DROP so uniqueness is never unenforced in +between, drops the legacy index only once the replacement is confirmed present, +preserves every row, and converges to no drift. + +Two details worth an operator's attention: + +- **Both** replacement index names are **hash-suffixed**, because their natural + names are 66 and 90 characters against a 60-character limit: + `uniq_sys_notification_subscription_799a483c` and + `uniq_sys_audience_binding_suggestion_a736dc5a`. On + `sys_audience_binding_suggestion` the legacy name + (`uniq_sys_audience_binding_suggestion_79a05fef`) is hash-suffixed too, so the + two differ only in the hash. That is expected, not corruption. +- Rows with no `organization_id` (platform/seed rows) stay unique **among + themselves**: the organization key part is NULL-safe + (`COALESCE(organization_id, '__global__')`, ADR-0120 D3), so seeding by name + keeps working and a tenant may hold its own row of the same key. + +## Not breaking + +A relaxation admits key pairs that were previously refused and refuses nothing +that previously succeeded, so no caller that worked before fails now. Every read +path for these two objects goes through the tenant-scoped data API, so no +consumer resolves one of these keys across organizations expecting at most one +row. Shipped as `patch` for that reason — the same call #8556 and #8554 made for +the same shape. + +The one published uniqueness claim about either object — "one per package × set × +anchor" on the permission-sets guide — now reads "one per organization × package +× set × anchor". Neither object's field text made a uniqueness claim, so no +translation bundle changed. diff --git a/content/docs/permissions/permission-sets.mdx b/content/docs/permissions/permission-sets.mdx index ac8ad8deef..4ab604d5ca 100644 --- a/content/docs/permissions/permission-sets.mdx +++ b/content/docs/permissions/permission-sets.mdx @@ -194,9 +194,9 @@ bit for `everyone` — the platform's own `member_default` baseline is exactly that shape; the wildcard ban is the stricter `guest` tier's rule. Pending suggestions are materialized as `sys_audience_binding_suggestion` -rows (one per package × set × anchor, read-only over the data API) and -resolved through the security surface — both installing a package at runtime -and declaring the set in the stack produce them: +rows (one per organization × package × set × anchor, read-only over the data +API) and resolved through the security surface — both installing a package at +runtime and declaring the set in the stack produce them: ```http GET /api/v1/security/suggested-bindings?status=pending # list (reconciles first) diff --git a/packages/drivers/driver-sql/src/sql-driver-8577-tenant-scoped-declared-unique.test.ts b/packages/drivers/driver-sql/src/sql-driver-8577-tenant-scoped-declared-unique.test.ts new file mode 100644 index 0000000000..fbd8a91172 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-8577-tenant-scoped-declared-unique.test.ts @@ -0,0 +1,722 @@ +// 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'; + +/** + * #8577 — the FIFTH act of the #8323 class: two more tenant-scoped objects + * whose declared unique indexes were installation-wide. + * + * ## What was measured here, live, before the fix — per object + * + * The 2026-08-13 maintainer ruling makes the live probe the required first + * step, per object, with the prediction written down first. Both reproduced + * exactly as predicted, driving the REAL SHIPPED DECLARATIONS (imported from + * their source files, not hand-typed) through this driver on + * `OS_TENANCY_POSTURE=isolated`: + * + * ``` + * sys_notification_subscription uniq_sys_notification_subscription_topic_principal + * (topic, principal) + * sys_audience_binding_suggestion uniq_sys_audience_binding_suggestion_79a05fef + * (package_id, permission_set_name, anchor) + * + * org_jia POST the key → 201 + * org_yi POST the SAME → 409 UNIQUE_VIOLATION + * org_yi POST an unused → 201 ← the control that makes it an ORACLE + * org_yi GET the key → total 0 ← refused by a row it cannot see + * ``` + * + * ## The two objects are NOT the same severity, and the tests differ accordingly + * + * `sys_notification_subscription` is the ordinary class shape and the direct + * sibling of `sys_notification_preference` (#8554): same package, same + * directory, same ADR-0030 Layer 3, `topic` + `principal` authored from the + * Setup grid. Its symptom is the #8323 one — a user in two organizations could + * not subscribe to the same topic in both, and `role:x` / `team:x` names are + * per-organization since #8461/#8556 so the same string denoted different + * principals while colliding on one installation-wide key. + * + * `sys_audience_binding_suggestion` is worse than a naming oracle. Its key is + * `(package_id, permission_set_name, anchor)` — **the same triple for every + * tenant that installs the same package**, since the package's own manifest + * supplies all three. The row is per-tenant by construction, so the second and + * every later organization to install a package never got its suggestion row: + * its admins were never prompted and its users never received the package's + * default permission set (ADR-0090 D5/D9). The 409/201 oracle is the LESSER + * half of that object's story; the install path is pinned where the real + * `syncAudienceBindingSuggestions` lives, in + * `plugin-security/src/suggested-audience-bindings-install-path.test.ts`. + * + * ## Why the declarations were global + * + * The 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 it "the #4986 trap". + * + * ## 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 a fresh-database suite cannot see + * + * Respelling changes the index's generated NAME, so on a DEPLOYED database + * 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. Section 3 is + * therefore the load-bearing part of this file: it builds installations that + * ALREADY HAVE the old index and real rows, then migrates them. + */ + +/** 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; raw?: unknown }> { + try { + await driver.create(object, record as any); + return { status: 201 }; + } catch (error) { + if (isUniqueViolationError(error)) return { status: 409, code: 'UNIQUE_VIOLATION', raw: error }; + return { status: 500, code: 'INTERNAL_ERROR', raw: error }; + } +} + +/** + * One object under test. + * + * ⚠️ `preIndexes` / `fixedIndexes` are hand-copied from the shipped + * declarations, keeping the package boundary (the shape #8461, #8556 and #8599 + * used — `driver-sql` must not depend on a plugin or a service package). The + * copy is guarded in ONE direction only: each package carries a pin asserting + * its real `Xxx.indexes` against its own inline literal, so a change to a + * SHIPPED declaration that is not mirrored here goes red over there. The + * reverse is unguarded — edit a fixture below without touching its declaration + * and nothing compares them. + * + * The `unique: false` on the non-unique entries is not decoration: + * `ObjectSchema.create` normalizes an authored `{ fields: ['status'] }` into + * that shape, so this is what a driver is actually handed at registration. + */ +interface Subject { + table: string; + /** Physical columns the constraint and the rows need. */ + fields: Record; + preIndexes: Array>; + fixedIndexes: Array>; + /** The declared unique index's listed columns, in key order. */ + keyColumns: string[]; + /** The colliding value, column by column. */ + key: Record; + /** Differs from `key` in its FIRST column — the 201 control. */ + control: Record; + /** Differs from `key` in its LAST column. */ + trailingControl: Record; + /** Differs from `key` in a MIDDLE column. Three-column keys only. */ + middleControl?: Record; + /** + * A third distinct value, used for the organization-LESS (platform/seed) row. + * + * ⚠️ It must differ from `key`: the seeded database carries the PRE-FIX global + * index, under which an organization-less row and an organization's row + * sharing the key columns genuinely collide — that is the defect, not a + * harness accident (#8599 lost 30 tests at once to exactly this). + */ + platformKey: Record; + /** Other columns a row needs to be insertable. */ + filler?: Record; + /** Legacy (pre-fix) index name, as `buildIndexName` emits it. */ + legacyName: string; + /** Replacement index name — hash-suffixed when the base passes 60 chars. */ + replacementName: string; +} + +const s = { type: 'string' }; +const b = { type: 'boolean' }; + +const SUBJECTS: Subject[] = [ + { + table: 'sys_notification_subscription', + fields: { id: s, organization_id: s, topic: s, principal: s, enabled: b }, + preIndexes: [ + { fields: ['topic', 'principal'], unique: true }, // ← the defect + { fields: ['topic'], unique: false }, + ], + fixedIndexes: [ + { fields: ['topic', 'principal'], unique: 'organization' }, + { fields: ['topic'], unique: false }, + ], + keyColumns: ['topic', 'principal'], + key: { topic: 'billing.invoice', principal: 'role:sales_manager' }, + control: { topic: 'crm.lead', principal: 'role:sales_manager' }, + // `principal` is the trailing column: a DIFFERENT subscriber on the same + // topic was always accepted, which is what proves the installation-wide key + // was the composite rather than `topic` alone. + trailingControl: { topic: 'billing.invoice', principal: 'role:only_yi' }, + filler: {}, + platformKey: { topic: 'platform.broadcast', principal: 'role:platform_ops' }, + legacyName: 'uniq_sys_notification_subscription_topic_principal', + // ⚠️ HASH-SUFFIXED. The card flagged only the other object as landing on + // the truncation path; this one lands there too — see section 6. + replacementName: 'uniq_sys_notification_subscription_799a483c', + }, + { + table: 'sys_audience_binding_suggestion', + fields: { + id: s, + organization_id: s, + package_id: s, + permission_set_name: s, + anchor: s, + status: s, + }, + preIndexes: [ + { fields: ['package_id', 'permission_set_name', 'anchor'], unique: true }, // ← the defect + { fields: ['status'], unique: false }, + { fields: ['package_id'], unique: false }, + ], + fixedIndexes: [ + { fields: ['package_id', 'permission_set_name', 'anchor'], unique: 'organization' }, + { fields: ['status'], unique: false }, + { fields: ['package_id'], unique: false }, + ], + keyColumns: ['package_id', 'permission_set_name', 'anchor'], + key: { package_id: 'com.acme.crm', permission_set_name: 'sales_readonly', anchor: 'everyone' }, + control: { package_id: 'com.other.pkg', permission_set_name: 'sales_readonly', anchor: 'everyone' }, + trailingControl: { package_id: 'com.acme.crm', permission_set_name: 'sales_readonly', anchor: 'guest' }, + middleControl: { package_id: 'com.acme.crm', permission_set_name: 'other_set', anchor: 'everyone' }, + filler: { status: 'pending' }, + platformKey: { + package_id: 'com.objectstack.platform', + permission_set_name: 'platform_baseline', + anchor: 'everyone', + }, + // ⚠️ BOTH names are hash-suffixed on this object, and they share the same + // 37-character head — only the sha1 prefix separates them. Section 6. + legacyName: 'uniq_sys_audience_binding_suggestion_79a05fef', + replacementName: 'uniq_sys_audience_binding_suggestion_a736dc5a', + }, +]; + +describe('#8577 — two more tenant-scoped declared unique indexes become per-organization', () => { + let driver: SqlDriver | undefined; + let savedPosture: string | undefined; + let savedMultiOrg: string | undefined; + + const makeDriver = () => { + const d = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + (d as any).logger = { warn: vi.fn(), info: vi.fn(), error: vi.fn(), debug: vi.fn() }; + driver = d; + return d; + }; + + const app = (sub: Subject, which: 'pre' | 'fixed') => [ + { name: sub.table, fields: sub.fields, indexes: which === 'pre' ? sub.preIndexes : sub.fixedIndexes }, + ]; + + 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 — section 5 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; + } + + const row = (sub: Subject, id: string, org: string | undefined, value: Record) => ({ + id, + ...(org ? { organization_id: org } : {}), + ...(sub.filler ?? {}), + ...value, + }); + + for (const sub of SUBJECTS) { + describe(sub.table, () => { + // ─────────────────────────────────────────────────────────────────── + // 1. Materialized shape — both spellings, kept side by side + // ─────────────────────────────────────────────────────────────────── + + it('the fixed declaration keys on the NULL-safe organization part', async () => { + const d = makeDriver(); + await d.initObjects(app(sub, 'fixed') as any); + + expect(await uniqueKeyParts(sub.table)).toEqual({ + [sub.replacementName]: ['COALESCE(organization_id)', ...sub.keyColumns], + }); + }); + + it('the pre-fix declaration keyed on the bare business columns — 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(app(sub, 'pre') as any); + + expect(await uniqueKeyParts(sub.table)).toEqual({ [sub.legacyName]: sub.keyColumns }); + }); + + // ─────────────────────────────────────────────────────────────────── + // 2. The card's reproduction — the live probe the ruling required + // ─────────────────────────────────────────────────────────────────── + + it('BEFORE: a key held by another organization is refused — 409 on a row you cannot read', async () => { + const d = makeDriver(); + await d.initObjects(app(sub, 'pre') as any); + + expect((await createAsApi(d, sub.table, row(sub, 'a', 'org_jia', sub.key))).status).toBe(201); + expect(await createAsApi(d, sub.table, row(sub, 'b', 'org_yi', sub.key))).toMatchObject( + CONFLICT_ENVELOPE, + ); + + // The control that makes the refusal an ORACLE rather than a blanket + // rejection: an unused value from the same caller is accepted, so the + // 409 is a per-value answer about another tenant's data. + expect((await createAsApi(d, sub.table, row(sub, 'c', 'org_yi', sub.control))).status).toBe(201); + + // …and the other half of the oracle: the caller's own read of the + // colliding key returns nothing. It is refused by a row it cannot see. + const visible = (await d.find(sub.table, {})).filter( + (r: any) => + r.organization_id === 'org_yi' && Object.entries(sub.key).every(([k, v]) => r[k] === v), + ); + expect(visible).toHaveLength(0); + }); + + it('AFTER: the same cross-organization create is accepted — 409 flips to 201', async () => { + const d = makeDriver(); + await d.initObjects(app(sub, 'fixed') as any); + + expect((await createAsApi(d, sub.table, row(sub, 'a', 'org_jia', sub.key))).status).toBe(201); + expect((await createAsApi(d, sub.table, row(sub, 'b', 'org_yi', sub.key))).status).toBe(201); + + const held = (await d.find(sub.table, {})).filter((r: any) => + Object.entries(sub.key).every(([k, v]) => r[k] === v), + ); + 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(app(sub, 'fixed') as any); + + expect((await createAsApi(d, sub.table, row(sub, 'a', 'org_jia', sub.key))).status).toBe(201); + expect(await createAsApi(d, sub.table, row(sub, 'b', 'org_jia', sub.key))).toMatchObject( + CONFLICT_ENVELOPE, + ); + expect(await d.count(sub.table, {})).toBe(1); + }); + + it('AFTER: rows with no organization stay unique among THEMSELVES (ADR-0120 D3)', async () => { + // Platform/seed rows carry no organization. A bare + // `(organization_id, …)` composite would be NULL-DISTINCT under SQL, so + // every seeded row could be duplicated at will; the NULL-safe key part + // is what prevents that, and seed upsert-by-name relies on it. + // + // ⚠️ On `sys_audience_binding_suggestion` this is not a hypothetical + // bucket: the shipped `syncAudienceBindingSuggestions` writes with a + // hardcoded `{ isSystem: true }` context carrying no tenant, so every + // row it creates today lands organization-less. See + // `suggested-audience-bindings-install-path.test.ts` §3. + const d = makeDriver(); + await d.initObjects(app(sub, 'fixed') as any); + + expect((await createAsApi(d, sub.table, row(sub, 'seed1', undefined, sub.key))).status).toBe(201); + expect(await createAsApi(d, sub.table, row(sub, 'seed2', undefined, sub.key))).toMatchObject( + CONFLICT_ENVELOPE, + ); + + // A tenant may still hold its OWN row of the same key — the platform + // bucket and the organization buckets are separate namespaces. + expect((await createAsApi(d, sub.table, row(sub, 'own1', 'org_jia', sub.key))).status).toBe(201); + }); + + it('the key is the whole COMPOSITE — varying a trailing column was always accepted', async () => { + // Guards the fixture's own claim about which columns the constraint + // spans. Green BEFORE and AFTER by design: it describes behaviour this + // card does not change, which is exactly what makes it a control. If + // the key were silently narrowed to the leading column this goes red. + const d = makeDriver(); + await d.initObjects(app(sub, 'pre') as any); + + expect((await createAsApi(d, sub.table, row(sub, 'a', 'org_jia', sub.key))).status).toBe(201); + expect( + (await createAsApi(d, sub.table, row(sub, 'b', 'org_yi', sub.trailingControl))).status, + ).toBe(201); + }); + + if (sub.middleControl) { + it('…and varying the MIDDLE column too — a three-column key is three columns', async () => { + // `sys_audience_binding_suggestion` is the only three-column key in + // this card. Without this, "the key spans permission_set_name" would + // rest on the fixture's spelling rather than on a measurement. + const d = makeDriver(); + await d.initObjects(app(sub, 'pre') as any); + + expect((await createAsApi(d, sub.table, row(sub, 'a', 'org_jia', sub.key))).status).toBe(201); + expect( + (await createAsApi(d, sub.table, row(sub, 'b', 'org_yi', sub.middleControl!))).status, + ).toBe(201); + }); + } + + // ─────────────────────────────────────────────────────────────────── + // 3. The deployed-installation half — the #8323 trap, per 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(app(sub, 'pre') as any); + await d.create(sub.table, row(sub, 'r1', 'org_jia', sub.key) as any); + await d.create(sub.table, row(sub, 'r2', 'org_jia', sub.control) as any); + await d.create(sub.table, row(sub, 'r3', undefined, sub.platformKey) as any); + }; + + it('the seeded database really carries the pre-fix index, and the defect is live on it (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(sub.table)).toEqual({ [sub.legacyName]: sub.keyColumns }); + expect(await d.count(sub.table, {})).toBe(3); + expect(await createAsApi(d, sub.table, row(sub, 'x', 'org_yi', sub.key))).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(app(sub, 'fixed') as any); + + const drift = await d.detectManagedDrift(); + expect(drift.filter((e) => e.table === sub.table)).toHaveLength(1); + const entry = drift.find( + (e) => e.table === sub.table && 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: [sub.legacyName], + createIndexName: sub.replacementName, + createColumns: ['organization_id', ...sub.keyColumns], + 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. + expect( + drift.filter( + (e) => e.op.type === 'drop_index' && (e.op as any).indexName === sub.legacyName, + ), + ).toHaveLength(0); + }); + + it('applies WITHOUT --allow-destructive, keeps every row, and converges', async () => { + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(app(sub, 'fixed') 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(sub.table)).toEqual({ + [sub.replacementName]: ['COALESCE(organization_id)', ...sub.keyColumns], + }); + expect(await d.count(sub.table, {})).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(app(sub, 'fixed') as any); + await d.applyMigrationEntries(await d.detectManagedDrift(), { allowDestructive: false }); + + expect((await createAsApi(d, sub.table, row(sub, 'z1', 'org_yi', sub.key))).status).toBe(201); + + // The anti-vacuity arm, on the SAME migrated index. + expect(await createAsApi(d, sub.table, row(sub, 'z2', 'org_jia', sub.key))).toMatchObject( + CONFLICT_ENVELOPE, + ); + + // …and the organization-less bucket survived the migration intact: + // the seeded platform row still blocks a duplicate of ITSELF. + expect(await createAsApi(d, sub.table, row(sub, 'z3', undefined, sub.platformKey))).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 refusing the second organization's row — deploying the + // respelling is not, by itself, the fix. This is the sentence an + // operator needs, stated as an assertion, and it is what the + // changeset tells them in prose. + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(app(sub, 'fixed') as any); + + expect(Object.keys(await uniqueKeyParts(sub.table)).sort()).toEqual( + [sub.legacyName, sub.replacementName].sort(), + ); + expect(await createAsApi(d, sub.table, row(sub, 'y', 'org_yi', sub.key))).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(app(sub, 'fixed') as any); + const drift = await d.detectManagedDrift(); + + await (d as any).knex.raw(`DROP INDEX ${sub.replacementName}`); + (d as any).syncDeclaredIndexes = async () => undefined; + + const { applied, skipped } = await d.applyMigrationEntries(drift, { allowDestructive: false }); + const isReplace = (e: { table: string; op: { type: string } }) => + e.table === sub.table && e.op.type === 'replace_unique_index'; + expect(applied.some(isReplace)).toBe(false); + expect(skipped.some(isReplace)).toBe(true); + + // The pre-migration constraint is intact: never left with neither index. + expect(Object.keys(await uniqueKeyParts(sub.table))).toEqual([sub.legacyName]); + }); + }); + + // ─────────────────────────────────────────────────────────────────── + // 4. The #8461 arm and its guards, exercised on THIS object (A1) + // ─────────────────────────────────────────────────────────────────── + + describe('the declared-index replacement arm', () => { + const physicalColumns = () => new Set(Object.keys(sub.fields)); + + it('proposes exactly one retirement, keyed on the listed columns', () => { + const [entry, ...rest] = legacyUniqueReplacements({ + table: sub.table, + fields: {}, + tenantField: 'organization_id', + physicalColumns: physicalColumns(), + declaredIndexes: sub.fixedIndexes, + } as any); + expect(rest).toHaveLength(0); + expect(entry).toMatchObject({ + // ⚠️ `legacyColumns` is the whole listed key, not the leading + // column. `column` is the LEADING one and is reporting only. + legacyColumns: sub.keyColumns, + legacyNames: [sub.legacyName], + replacement: { + name: sub.replacementName, + columns: ['organization_id', ...sub.keyColumns], + 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: sub.table, + fields: {}, + tenantField: 'organization_id', + physicalColumns: physicalColumns(), + declaredIndexes: [{ name: 'uq_hand_named', fields: sub.keyColumns, 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 and the S6 objects the sweeps re-triaged. + expect( + legacyUniqueReplacements({ + table: sub.table, + fields: {}, + tenantField: 'organization_id', + physicalColumns: physicalColumns(), + declaredIndexes: [ + { fields: ['organization_id', ...sub.keyColumns], unique: 'organization' }, + ], + } as any), + ).toHaveLength(0); + }); + + it('claims nothing for the BARE spelling — an unrespelled declaration is untouched (#5082)', () => { + expect( + legacyUniqueReplacements({ + table: sub.table, + fields: {}, + tenantField: 'organization_id', + physicalColumns: physicalColumns(), + declaredIndexes: [{ fields: sub.keyColumns, 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, on both', async () => { + for (const sub of SUBJECTS) { + for (const posture of ['single', 'group', 'isolated']) { + process.env.OS_TENANCY_POSTURE = posture; + const d = makeDriver(); + await d.initObjects(app(sub, 'fixed') as any); + expect(await uniqueKeyParts(sub.table), `${sub.table} posture=${posture}`).toEqual({ + [sub.replacementName]: ['COALESCE(organization_id)', ...sub.keyColumns], + }); + await d.disconnect(); + driver = undefined; + } + } + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 6. The index-name truncation boundary — BOTH objects land on it + // ───────────────────────────────────────────────────────────────────────── + + /** + * `buildIndexName` truncates past `INDEX_NAME_MAX = 60` to + * `` `uniq_`.slice(0, 51) `` plus a sha1-8 of the FULL base name. This + * is where a name-based retirement goes wrong: if the legacy name and the + * replacement name ever collapsed to the same string, the + * `legacyName === replacement.name` guard would read the respelling as + * "nothing was superseded" and emit NO migration at all — the declaration + * would change, a fresh database would look right, and every deployed + * installation would keep the global index forever. + * + * ⚠️ The card flagged only `sys_audience_binding_suggestion` as hash-suffixed. + * Measured: BOTH objects on this card land on the truncation path, and on + * `sys_audience_binding_suggestion` BOTH names are truncated and share the + * same 37-character head — the narrowest gap between a legacy name and its + * replacement anywhere in this lineage so far, since only the sha1 prefix + * separates them. + */ + describe('the hash-suffixed replacement names', () => { + it('sys_notification_subscription: legacy verbatim (50 ch), replacement truncated', () => { + const sub = SUBJECTS.find((x) => x.table === 'sys_notification_subscription')!; + expect(sub.legacyName).toBe('uniq_sys_notification_subscription_topic_principal'); + expect(sub.legacyName.length).toBe(50); + // The un-truncated replacement, for the reader: 66 characters. + expect('uniq_sys_notification_subscription_organization_id_topic_principal'.length).toBe(66); + expect(sub.replacementName).toMatch(/^uniq_sys_notification_subscription_[0-9a-f]{8}$/); + expect(sub.replacementName).not.toBe(sub.legacyName); + }); + + it('sys_audience_binding_suggestion: BOTH names truncated, same head, different hash', () => { + const sub = SUBJECTS.find((x) => x.table === 'sys_audience_binding_suggestion')!; + // 74 and 90 characters respectively before truncation. + expect('uniq_sys_audience_binding_suggestion_package_id_permission_set_name_anchor'.length).toBe(74); + expect( + 'uniq_sys_audience_binding_suggestion_organization_id_package_id_permission_set_name_anchor'.length, + ).toBe(90); + const head = 'uniq_sys_audience_binding_suggestion_'; + expect(sub.legacyName.startsWith(head)).toBe(true); + expect(sub.replacementName.startsWith(head)).toBe(true); + expect(sub.legacyName).toMatch(/^uniq_sys_audience_binding_suggestion_[0-9a-f]{8}$/); + expect(sub.replacementName).toMatch(/^uniq_sys_audience_binding_suggestion_[0-9a-f]{8}$/); + // The whole point: the S6 guard must not swallow the retirement. + expect(sub.replacementName).not.toBe(sub.legacyName); + }); + + it('the truncated names are what actually materialize, and each migration converges', async () => { + for (const sub of SUBJECTS) { + const d = makeDriver(); + await d.initObjects(app(sub, 'pre') as any); + await d.create(sub.table, row(sub, 'r1', 'org_jia', sub.key) as any); + await d.initObjects(app(sub, 'fixed') as any); + await d.applyMigrationEntries(await d.detectManagedDrift(), { allowDestructive: false }); + + expect(Object.keys(await uniqueKeyParts(sub.table)), sub.table).toEqual([sub.replacementName]); + expect(await d.detectManagedDrift()).toHaveLength(0); + await d.disconnect(); + driver = undefined; + } + }); + }); +}); diff --git a/packages/plugins/plugin-security/package.json b/packages/plugins/plugin-security/package.json index 6a2b97d039..d34e02ec6a 100644 --- a/packages/plugins/plugin-security/package.json +++ b/packages/plugins/plugin-security/package.json @@ -25,6 +25,8 @@ "@objectstack/spec": "workspace:*" }, "devDependencies": { + "@objectstack/driver-sql": "workspace:*", + "@objectstack/objectql": "workspace:*", "@objectstack/plugin-sharing": "workspace:*", "@objectstack/service-i18n": "workspace:*", "@types/node": "^26.1.2", diff --git a/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.object.ts b/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.object.ts index 4732b4030e..4ec5ae4bc6 100644 --- a/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.object.ts +++ b/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.object.ts @@ -105,7 +105,33 @@ export const SysAudienceBindingSuggestion = ObjectSchema.create({ }, indexes: [ - { fields: ['package_id', 'permission_set_name', 'anchor'], unique: true }, + // [#8577] Scope spelled EXPLICITLY (ADR-0120 D1). On a DECLARED index bare + // `unique: true` is the positional spelling of `'global'` — the listed + // columns VERBATIM — so `(package_id, permission_set_name, anchor)` was an + // installation-wide key on a tenant-scoped object. + // + // ⚠️ This is not the class's usual naming oracle. The key is the owning + // package's id, the PACKAGE'S OWN permission-set name and the anchor — + // the SAME TRIPLE for every tenant that installs the same package — while + // the row is per-tenant by construction (produced when the declaration is + // observed, resolved when a TENANT ADMIN confirms). So the second and every + // later organization to install a package never got its suggestion row: + // its admins were never prompted and its users never received the package's + // default permission set. ADR-0090 D5/D9 exists so this is never + // auto-bound; the effect was that for every tenant after the first it was + // never bound AT ALL, and nothing said so — + // `syncAudienceBindingSuggestions` swallows the insert failure in a bare + // `catch` (read as a benign concurrent-sync race). + // + // Measured live before the fix (real SqlDriver, better-sqlite3, + // OS_TENANCY_POSTURE=isolated, this shipped declaration): + // org_jia creates (com.acme.crm, sales_readonly, everyone) 201 / org_yi the + // SAME triple 409 UNIQUE_VIOLATION / org_yi (com.acme.crm, other_set, + // everyone) 201 / org_yi (com.other.pkg, sales_readonly, everyone) 201 / + // org_yi (com.acme.crm, sales_readonly, guest) 201 / org_yi's own GET on + // the colliding triple 0 rows. And through the REAL sync on a real engine: + // org_jia created 1, org_yi created 0 with no throw and no log line. + { fields: ['package_id', 'permission_set_name', 'anchor'], unique: 'organization' }, { fields: ['status'] }, { fields: ['package_id'] }, ], diff --git a/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.organization-unique.test.ts b/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.organization-unique.test.ts new file mode 100644 index 0000000000..1ac4e8c354 --- /dev/null +++ b/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.organization-unique.test.ts @@ -0,0 +1,110 @@ +// 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 { SysAudienceBindingSuggestion } from './sys-audience-binding-suggestion.object.js'; + +/** + * #8577 — `sys_audience_binding_suggestion`'s declared uniqueness is + * organization-scoped. + * + * ## This one is not a naming oracle — it is a functional dead end + * + * The rest of the #8323 class is about two tenants wanting the same *name*. + * Here the key is `(package_id, permission_set_name, anchor)`, and all three + * come from the PACKAGE'S OWN manifest — so it is **the same triple for every + * tenant that installs the same package**, while the row is per-tenant by + * construction (produced when the declaration is observed, resolved when a + * TENANT ADMIN confirms). Under the installation-wide key, the second and every + * later organization to install a package never got its suggestion row: its + * admins were never prompted, and its users never received the package's + * default permission set. ADR-0090 D5/D9 exists so an install never + * auto-binds; the effect was that for every tenant after the first it was never + * bound at all, and nothing said so — `syncAudienceBindingSuggestions` swallows + * the insert failure in a bare `catch` it reads as a benign concurrent-sync + * race. + * + * Measured live BEFORE the fix, driving this shipped declaration through + * `SqlDriver` under `OS_TENANCY_POSTURE=isolated`: + * + * ``` + * CREATE UNIQUE INDEX uniq_sys_audience_binding_suggestion_79a05fef + * on sys_audience_binding_suggestion (package_id, permission_set_name, anchor) + * + * org_jia POST (com.acme.crm, sales_readonly, everyone) → 201 + * org_yi POST the SAME → 409 UNIQUE_VIOLATION + * org_yi POST an unused set → 201 ← the control that makes it an ORACLE + * org_yi GET the key → total 0 ← refused by a row it cannot see + * ``` + * + * ⚠️ `managedBy: 'engine-owned'` is not a reason to exempt it. The management + * mode says who WRITES the rows; the ruling's question is whose CONTENT they + * are, and these rows are one tenant's pending admin decision. + * + * ⚠️ BOTH index names are hash-suffixed here (74 and 90 characters before + * truncation, past `INDEX_NAME_MAX = 60`) and they share the same 37-character + * head — only the sha1 prefix separates + * `uniq_sys_audience_binding_suggestion_79a05fef` from + * `uniq_sys_audience_binding_suggestion_a736dc5a`. Had they collapsed to one + * string, `legacyName === replacement.name` would have read the respelling as + * "nothing was superseded" and emitted no migration at all. Pinned driver-side. + * + * The materialized shape, the anti-vacuity twin (a SAME-organization duplicate + * must still be refused), and the migration of an installation that already + * carries the old index are pinned in + * `driver-sql/src/sql-driver-8577-tenant-scoped-declared-unique.test.ts`; the + * install path — two organizations installing the same package, each ending up + * with its own row — is pinned through the REAL sync in + * `../suggested-audience-bindings-install-path.test.ts`. This test pins the + * declaration both of those rest on. + */ +describe('sys_audience_binding_suggestion — declared uniqueness is organization-scoped (#8577)', () => { + const uniqueIndexes = (SysAudienceBindingSuggestion.indexes ?? []).filter((i: any) => i.unique); + + it('declares exactly one unique index, on (package_id, permission_set_name, anchor)', () => { + expect(uniqueIndexes).toHaveLength(1); + expect((uniqueIndexes[0] as any).fields).toEqual([ + 'package_id', + 'permission_set_name', + 'anchor', + ]); + }); + + it("spells the scope 'organization' — NOT bare `true`", () => { + // ⛔ Asserted by EQUALITY, never by truthiness: `true` is truthy and the + // `.filter((i) => i.unique)` above still matches it, so a truthiness check + // here accepts the exact spelling that was the defect. + 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: ['package_id', 'permission_set_name', 'anchor'], + unique: 'organization', + }); + }); + + it('matches the fixture the driver suite copies, entry for entry', () => { + // One direction of drift only: a change HERE that is not mirrored in the + // driver fixture goes red; a change to the driver fixture alone does not. + // Asserted on the BUILT value — `ObjectSchema.create` normalizes an + // authored `{ fields: ['status'] }` into `{ fields: ['status'], unique: false }`, + // which is what the driver is actually handed. + expect(SysAudienceBindingSuggestion.indexes).toEqual([ + { fields: ['package_id', 'permission_set_name', 'anchor'], unique: 'organization' }, + { fields: ['status'], unique: false }, + { fields: ['package_id'], 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 silently inert. + const plan = resolveInjectedSystemColumns(SysAudienceBindingSuggestion); + expect(SysAudienceBindingSuggestion.tenancy).toBeUndefined(); + expect(plan.tenant).toBe(true); + expect(plan.names.has('organization_id')).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts b/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts new file mode 100644 index 0000000000..30c4783b89 --- /dev/null +++ b/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts @@ -0,0 +1,368 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +// The REAL shipped reconciler — not a re-implementation. A local copy would +// make this suite a test of the copy, which is exactly the failure mode the +// file exists to close. +import { syncAudienceBindingSuggestions } from './suggested-audience-bindings.js'; +import { SysAudienceBindingSuggestion } from './objects/sys-audience-binding-suggestion.object.js'; +// The other three tables the reconciler consults, as the REAL declarations — +// never hand-rolled stand-ins. With them registered the anchor lookup and the +// "is it already bound?" lookup resolve properly (to "anchor present, binding +// absent" — the genuine PENDING case), instead of throwing table-missing +// errors that `tryFind` swallows into `[]` and that reach the same verdict for +// the wrong reason. +import { SysPosition } from './objects/sys-position.object.js'; +import { SysPermissionSet } from './objects/sys-permission-set.object.js'; +import { SysPositionPermissionSet } from './objects/sys-position-permission-set.object.js'; + +/** + * #8577 — the INSTALL PATH of `sys_audience_binding_suggestion`, on a real + * engine, through the real reconciler. + * + * ## Why the 409/201 oracle is the LESSER half of this object's story + * + * The rest of the #8323 class is about two tenants wanting the same *name*. + * This object's key is `(package_id, permission_set_name, anchor)` and all + * three come from the package's own manifest — **the same triple for every + * tenant that installs the same package** — while the row is per-tenant by + * construction (ADR-0090 D5/D9: produced when the declaration is observed, + * resolved when a TENANT ADMIN confirms). + * + * So the question a driver-level "the second create now returns 201" assertion + * does NOT answer is the one that matters: *does the second organization's + * admin actually get prompted?* That is what this file measures, end to end: + * two organizations install the same package; each must end up with its own + * pending suggestion row. + * + * ## Why the failure was silent + * + * `syncAudienceBindingSuggestions` wraps its insert in a bare `catch` whose + * comment reads "unique-index race with a concurrent sync — benign". Under the + * pre-fix installation-wide index the UNIQUE violation raised for the SECOND + * organization is indistinguishable from that benign race, so it was swallowed: + * no throw, no warning, and `created` simply stayed 0. Section 1 asserts that + * silence explicitly — it is the reason nobody noticed. + * + * ## The harness + * + * A real `ObjectQL` engine over a real better-sqlite3 `SqlDriver`, registering + * the REAL shipped declaration (cloned only to flip `unique` back to the + * pre-fix spelling for the BEFORE cases). The one hand-built part is the `ql` + * facade: it threads ONE organization's execution context onto every call the + * reconciler makes, which is what a runtime serving that tenant does, and it + * supplies the installed-package manifest the reconciler reads through + * `registry.getAllPackages()`. + * + * ⚠️ Section 3 records a measured LIMIT of that faithfulness, and it is the + * finding this file exists to keep visible. + */ + +/** The package both organizations install. */ +const PACKAGE_MANIFEST = { + id: 'com.acme.crm', + permissions: [{ name: 'sales_readonly', isDefault: true }], +}; + +/** The two tenants of one installation. */ +const ORGANIZATIONS = ['org_jia', 'org_yi'] as const; + +const engines: ObjectQL[] = []; + +afterEach(async () => { + while (engines.length) { + try { + await engines.pop()?.destroy(); + } catch { + /* noop */ + } + } +}); + +/** The shipped declaration, or a clone respelled back to the pre-fix `true`. */ +function declaration(scope: true | 'organization'): any { + const decl: any = structuredClone(SysAudienceBindingSuggestion); + decl.indexes[0].unique = scope; + return decl; +} + +async function boot(scope: true | 'organization'): Promise { + const engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), + true, + ); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.security-objects', + name: 'Security Objects', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [declaration(scope), SysPosition, SysPermissionSet, SysPositionPermissionSet], + } as any); + await engine.syncSchemas(); + engines.push(engine); + + // Each organization has the `everyone` anchor seeded (bootstrapBuiltinRoles + // does this before the reconciler ever runs) and holds the package's set, + // but NO binding between them — which is exactly the state a package install + // leaves behind and the state a `pending` suggestion describes. + for (const org of ORGANIZATIONS) { + await (engine as any).insert( + 'sys_position', + { id: `pos_everyone_${org}`, name: 'everyone', label: 'Everyone' }, + { context: { isSystem: true, tenantId: org } }, + ); + await (engine as any).insert( + 'sys_permission_set', + { id: `ps_${org}`, name: 'sales_readonly', label: 'Sales Readonly', package_id: PACKAGE_MANIFEST.id }, + { context: { isSystem: true, tenantId: org } }, + ); + } + return engine; +} + +/** + * The engine as the runtime of ONE organization presents it: every call the + * reconciler makes carries that organization's tenant context, and + * `registry.getAllPackages()` answers with the installed manifest. + * + * `listItems` returns `[]` on purpose — this models a RUNTIME package install + * (`POST /api/v1/packages`), where the declaration reaches the reconciler + * through the registry rather than through boot-declared stack metadata. + */ +function runtimeOf(engine: ObjectQL, organizationId: string): any { + const withOrg = (o: any = {}) => ({ ...o, context: { ...(o.context ?? {}), tenantId: organizationId } }); + return { + find: (object: string, q: any = {}) => (engine as any).find(object, withOrg(q)), + insert: (object: string, data: any, opt: any = {}) => (engine as any).insert(object, data, withOrg(opt)), + update: (object: string, data: any, opt: any = {}) => (engine as any).update(object, data, withOrg(opt)), + delete: (object: string, opt: any = {}) => (engine as any).delete(object, withOrg(opt)), + registry: { + listItems: () => [], + getAllPackages: () => [{ enabled: true, manifest: PACKAGE_MANIFEST }], + }, + }; +} + +/** Every stored row, read past tenancy — the ground truth, not a tenant's view. */ +async function storedRows(engine: ObjectQL): Promise> { + const driver: any = (engine as any).getDriver('sys_audience_binding_suggestion'); + const rows: any[] = await driver.knex('sys_audience_binding_suggestion').select('*'); + return rows.map((r) => ({ + org: r.organization_id ?? null, + key: `${r.package_id}/${r.permission_set_name}/${r.anchor}`, + status: r.status, + })); +} + +/** What ONE organization's admin surface can see. */ +async function visibleTo(engine: ObjectQL, organizationId: string): Promise { + return (engine as any).find('sys_audience_binding_suggestion', { + context: { isSystem: true, tenantId: organizationId }, + }); +} + +/** Unique index names materialized on the table. */ +async function uniqueIndexNames(engine: ObjectQL): Promise { + const driver: any = (engine as any).getDriver('sys_audience_binding_suggestion'); + const list: any[] = await driver.knex.raw('PRAGMA index_list(sys_audience_binding_suggestion)'); + return list.filter((i) => i.origin !== 'pk' && i.unique === 1).map((i) => i.name).sort(); +} + +const KEY = 'com.acme.crm/sales_readonly/everyone'; + +describe('#8577 — the package-install path of sys_audience_binding_suggestion', () => { + // ───────────────────────────────────────────────────────────────────────── + // 1. BEFORE — the dead end, measured through the real reconciler + // ───────────────────────────────────────────────────────────────────────── + + describe('with the pre-fix installation-wide index', () => { + it('the harness really carries the pre-fix index (harness guard)', async () => { + // Without this the whole block could be exercising the FIXED schema and + // every assertion below would read as a description of the defect while + // measuring the fix. Named as a guard on purpose. + const engine = await boot(true); + expect(await uniqueIndexNames(engine)).toEqual(['uniq_sys_audience_binding_suggestion_79a05fef']); + }); + + it('the SECOND organization to install the package never gets its suggestion row', async () => { + const engine = await boot(true); + + const first = await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_jia')); + const second = await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_yi')); + + expect(first).toMatchObject({ created: 1 }); + // The measurement the card is about: not "an error", not "a warning" — + // nothing at all happened for the second tenant. + expect(second).toMatchObject({ created: 0, confirmedObserved: 0, pruned: 0 }); + + expect(await storedRows(engine)).toEqual([ + { org: 'org_jia', key: KEY, status: 'pending' }, + ]); + + // …and the consequence in the terms an admin experiences it: org_yi's + // suggestion surface is EMPTY, so its admin is never asked to bind the + // package's default permission set, and its users never receive it. + expect(await visibleTo(engine, 'org_yi')).toHaveLength(0); + }); + + it('and the reconciler reports NOTHING — no throw, no warning, only the first tenant is logged', async () => { + // The reason nobody noticed: the reconciler cannot tell the UNIQUE + // violation raised for a second TENANT from the benign concurrent-sync + // race its `catch` was written for. + // + // Precisely: the only trace anywhere is the ENGINE's own driver-level + // "Insert operation failed" line (visible in this suite's output on the + // two pre-fix cases). Nothing above the driver — not the reconciler's + // return value, not its logger, not the caller — ever learns that a + // tenant went unprompted. + const engine = await boot(true); + const lines: Array<[string, string]> = []; + const logger = { + info: (m: string) => lines.push(['info', m]), + warn: (m: string) => lines.push(['warn', m]), + }; + + await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_jia'), undefined, logger); + await expect( + syncAudienceBindingSuggestions(runtimeOf(engine, 'org_yi'), undefined, logger), + ).resolves.toMatchObject({ created: 0 }); + + expect(lines.filter(([level]) => level === 'warn')).toHaveLength(0); + // Exactly one reconciliation was reported — org_jia's. The second + // organization's produced no line of any kind, because from the + // reconciler's point of view nothing needed doing. + expect(lines).toHaveLength(1); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 2. AFTER — each organization is prompted for its own install + // ───────────────────────────────────────────────────────────────────────── + + describe('with the organization-scoped index', () => { + it('the harness carries the REPLACEMENT index (harness guard)', async () => { + const engine = await boot('organization'); + expect(await uniqueIndexNames(engine)).toEqual(['uniq_sys_audience_binding_suggestion_a736dc5a']); + }); + + it('two organizations installing the same package EACH get their own pending row', async () => { + const engine = await boot('organization'); + + const first = await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_jia')); + const second = await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_yi')); + + expect(first).toMatchObject({ created: 1 }); + expect(second).toMatchObject({ created: 1 }); + + expect((await storedRows(engine)).sort((a, b) => String(a.org).localeCompare(String(b.org)))).toEqual([ + { org: 'org_jia', key: KEY, status: 'pending' }, + { org: 'org_yi', key: KEY, status: 'pending' }, + ]); + }); + + it('…and each organization SEES exactly its own — the prompt actually reaches the admin', async () => { + // The end the card cares about, stated the way an admin experiences it. + const engine = await boot('organization'); + await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_jia')); + await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_yi')); + + for (const org of ['org_jia', 'org_yi']) { + const rows = await visibleTo(engine, org); + expect(rows, org).toHaveLength(1); + expect(rows[0].organization_id, org).toBe(org); + expect(rows[0].status, org).toBe('pending'); + } + }); + + it('ANTI-VACUITY: the reconciler is still idempotent WITHIN an organization', async () => { + // ⛔ The failure this guards against is a "fix" that removed uniqueness + // instead of scoping it — strictly worse than the defect, and + // indistinguishable from the real fix by the "each org gets a row" + // assertion alone. Re-running a tenant's sync must add nothing. + const engine = await boot('organization'); + + await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_jia')); + const again = await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_jia')); + const third = await syncAudienceBindingSuggestions(runtimeOf(engine, 'org_jia')); + + expect(again).toMatchObject({ created: 0, confirmedObserved: 0, pruned: 0 }); + expect(third).toMatchObject({ created: 0 }); + expect(await storedRows(engine)).toHaveLength(1); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 3. The remaining half — a MEASURED limit of this fix, kept visible + // ───────────────────────────────────────────────────────────────────────── + + /** + * ⚠️ Read this before concluding the install path is whole. + * + * Everything above threads a tenant context onto the reconciler's calls. + * The SHIPPED call sites do not: `suggested-audience-bindings.ts` writes with + * a module-level `SYSTEM_CTX = { isSystem: true }` that carries no tenant, and + * `security-plugin.ts` invokes it at boot and after a package-door publish + * with the bare engine. Measured on this engine: + * + * - an insert under `{ isSystem: true }` stores `organization_id` NULL; + * - a read under `{ isSystem: true }` sees every organization's rows; + * - a read under `{ isSystem: true, tenantId: X }` sees X's rows AND the + * NULL-organization rows (the driver expands the tenant predicate to + * `organization_id = :tenant OR organization_id IS NULL`). + * + * So on the shipped path the reconciler writes ONE organization-less row that + * every tenant reads, and the second run finds it and skips — the same dead + * end as the index, reached by a different road, and NOT repaired by + * respelling the index. The index fix is necessary (without it even a + * correctly tenant-scoped write is refused) and it is what this card was + * ruled to deliver; the reconciler's tenant blindness is filed separately. + * + * The two assertions below RECORD today's behaviour rather than endorse it. + * The follow-up that threads the tenant through must DELETE them, not update + * them — if they still pass afterwards, that fix did not work. + */ + describe('the shipped SYSTEM_CTX call path is tenant-blind (recorded, not endorsed)', () => { + /** The reconciler wired exactly as `security-plugin.ts` wires it. */ + const asShipped = (engine: ObjectQL): any => ({ + find: (object: string, q: any = {}) => (engine as any).find(object, q), + insert: (object: string, data: any, opt: any = {}) => (engine as any).insert(object, data, opt), + update: (object: string, data: any, opt: any = {}) => (engine as any).update(object, data, opt), + delete: (object: string, opt: any = {}) => (engine as any).delete(object, opt), + registry: { + listItems: () => [], + getAllPackages: () => [{ enabled: true, manifest: PACKAGE_MANIFEST }], + }, + }); + + it('writes ONE organization-less row, and the organization-scoped index does not change that', async () => { + const engine = await boot('organization'); + + const first = await syncAudienceBindingSuggestions(asShipped(engine)); + const second = await syncAudienceBindingSuggestions(asShipped(engine)); + + expect(first).toMatchObject({ created: 1 }); + expect(second).toMatchObject({ created: 0 }); + expect(await storedRows(engine)).toEqual([{ org: null, key: KEY, status: 'pending' }]); + }); + + it('every tenant reads that same organization-less row, so only one decision exists', async () => { + const engine = await boot('organization'); + await syncAudienceBindingSuggestions(asShipped(engine)); + + for (const org of ['org_jia', 'org_yi']) { + const rows = await visibleTo(engine, org); + expect(rows, org).toHaveLength(1); + expect(rows[0].organization_id ?? null, org).toBeNull(); + } + }); + }); +}); diff --git a/packages/services/service-messaging/src/objects/notification-subscription.object.ts b/packages/services/service-messaging/src/objects/notification-subscription.object.ts index acb3a982e8..cdb14c53b8 100644 --- a/packages/services/service-messaging/src/objects/notification-subscription.object.ts +++ b/packages/services/service-messaging/src/objects/notification-subscription.object.ts @@ -58,7 +58,34 @@ export const NotificationSubscription = ObjectSchema.create({ }, indexes: [ - { fields: ['topic', 'principal'], unique: true }, + // [#8577] Scope spelled EXPLICITLY (ADR-0120 D1). On a DECLARED index + // bare `unique: true` is the positional spelling of `'global'` — the + // listed columns VERBATIM — so `(topic, principal)` was an + // installation-wide key on a tenant-scoped object. Direct sibling of + // `sys_notification_preference` (#8554): same package, same directory, + // same ADR-0030 Layer 3, same archetype. + // Measured live before the fix (real SqlDriver, better-sqlite3, + // OS_TENANCY_POSTURE=isolated, this shipped declaration): + // org_jia creates (billing.invoice, role:sales_manager) 201 / org_yi + // the SAME pair 409 UNIQUE_VIOLATION / org_yi (billing.invoice, + // role:only_yi) 201 / org_yi (crm.lead, role:sales_manager) 201 / + // org_yi (billing.invoice, user:u1) 201 / org_yi's own GET on the + // colliding pair 0 rows. + // + // ⚠️ `principal` names are per-organization: `role:x` and `team:x` + // resolve against `sys_permission_set` / `sys_position` rows that + // #8461 and #8556 already scoped per organization, so `role:sales_manager` + // denoted a DIFFERENT principal in each organization while colliding on + // one installation-wide key. And a user who belongs to two + // organizations could not subscribe to the same topic in both — the + // symptom #8323 measured on `sys_user_preference`. + // + // ⚠️ `managedBy: 'system-data'` is NOT a reason to exempt this object; + // the already-ruled `sys_user_preference` is `system-data` too. The + // ruling's phrase is ADMIN-AUTHORED CONTENT — the provenance of the + // rows (this object's own header: authored from the Setup + // "Notification Subscriptions" grid), not the management mode. + { fields: ['topic', 'principal'], unique: 'organization' }, { fields: ['topic'] }, ], }); diff --git a/packages/services/service-messaging/src/objects/notification-subscription.organization-unique.test.ts b/packages/services/service-messaging/src/objects/notification-subscription.organization-unique.test.ts new file mode 100644 index 0000000000..a45214a56d --- /dev/null +++ b/packages/services/service-messaging/src/objects/notification-subscription.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'; +// Explicit `.js` extension: this package resolves under NodeNext, where an +// extensionless relative import does not typecheck (its sibling suites all +// spell it this way). +import { NotificationSubscription } from './notification-subscription.object.js'; + +/** + * #8577 — `sys_notification_subscription`'s declared uniqueness is + * organization-scoped. + * + * ## What the bare spelling cost + * + * A DECLARED index's `unique: true` is the positional spelling of `'global'` + * (the listed columns verbatim), so the composite `(topic, principal)` was an + * installation-wide key on a tenant-scoped object. Measured live on a real + * engine BEFORE the fix, driving this shipped declaration through `SqlDriver` + * under `OS_TENANCY_POSTURE=isolated`: + * + * ``` + * CREATE UNIQUE INDEX uniq_sys_notification_subscription_topic_principal + * on sys_notification_subscription (topic, principal) + * + * org_jia POST (billing.invoice, role:sales_manager) → 201 + * org_yi POST the SAME → 409 UNIQUE_VIOLATION + * org_yi POST an unused → 201 ← the control that makes it an ORACLE + * org_yi GET the key → total 0 ← refused by a row it cannot see + * ``` + * + * ## Why per-organization is the CORRECT boundary, not merely the safe one + * + * This is the direct sibling of `sys_notification_preference`, one of #8554's + * five: same package, same directory, same ADR-0030 Layer 3, same archetype, + * and its own header records that the rows are authored from the Setup + * "Notification Subscriptions" grid — admin-authored content by the ruling's + * own phrase. + * + * `principal` is `role:x` / `team:x` / `user:id` / a bare user id, and role and + * position names have been per-organization since #8461 / #8556 — so + * `role:sales_manager` denoted a DIFFERENT principal in each organization while + * colliding on one installation-wide key. The measured symptom is #8323's: a + * user who belongs to two organizations could not subscribe to the same topic + * in both. + * + * ⚠️ `managedBy: 'system-data'` is NOT a reason to exempt this object. The + * already-ruled `sys_user_preference` is `system-data` too; the ruling's phrase + * is ADMIN-AUTHORED CONTENT — the provenance of the ROWS, not the management + * mode of the object. + * + * ⚠️ The replacement index name is HASH-SUFFIXED + * (`uniq_sys_notification_subscription_799a483c`): its un-truncated form is 66 + * characters, past `INDEX_NAME_MAX = 60`. The card flagged only its sibling + * object as landing on that path; this one lands there too. Pinned driver-side. + * + * The materialized shape, the anti-vacuity twin (a SAME-organization duplicate + * must still be refused), and the migration of an installation that already + * carries the old index are pinned driver-side in + * `driver-sql/src/sql-driver-8577-tenant-scoped-declared-unique.test.ts`. This + * test pins the declaration that suite's fixture copies. + */ +describe('sys_notification_subscription — declared uniqueness is organization-scoped (#8577)', () => { + const uniqueIndexes = (NotificationSubscription.indexes ?? []).filter((i: any) => i.unique); + + it('declares exactly one unique index, on (topic, principal)', () => { + expect(uniqueIndexes).toHaveLength(1); + expect((uniqueIndexes[0] as any).fields).toEqual(['topic', 'principal']); + }); + + 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, + // which is how the equivalent pins on #8556 and #8554 stayed green under + // their own ablations. + 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: ['topic', 'principal'], + unique: 'organization', + }); + }); + + it('matches the fixture the driver suite copies, entry for entry', () => { + // The driver suite hand-copies this declaration to keep the package + // boundary. This assertion catches ONE direction of drift: + // + // caught — the shipped declaration changes and the driver fixture + // does not. This test goes red. + // NOT caught — the DRIVER fixture is edited and this declaration is not. + // Nothing compares the two copies directly; they are only + // ever checked against this third spelling. + // + // Asserted on the BUILT value, which is what a driver is handed: + // `ObjectSchema.create` normalizes an authored `{ fields: [...] }` into + // `{ fields: [...], unique: false }`. + expect(NotificationSubscription.indexes).toEqual([ + { fields: ['topic', 'principal'], unique: 'organization' }, + { fields: ['topic'], 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 silently inert — a failure the index assertions above cannot see. + const plan = resolveInjectedSystemColumns(NotificationSubscription); + expect(NotificationSubscription.tenancy).toBeUndefined(); + expect(plan.tenant).toBe(true); + expect(plan.names.has('organization_id')).toBe(true); + }); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ad2feb1982..89822ebc05 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1728,6 +1728,12 @@ importers: specifier: workspace:* version: link:../../spec devDependencies: + '@objectstack/driver-sql': + specifier: workspace:* + version: link:../../drivers/driver-sql + '@objectstack/objectql': + specifier: workspace:* + version: link:../../objectql '@objectstack/plugin-sharing': specifier: workspace:* version: link:../plugin-sharing From a4092bd52dc72b13c121eeaf3a3f68f7d2b08d8b Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Fri, 14 Aug 2026 02:33:30 +0000 Subject: [PATCH 2/4] docs(#8577): name the filed follow-up #8617 in the declaration, test and changeset Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- .changeset/tenant-scoped-declared-unique-indexes-8577.md | 9 +++++++++ .../objects/sys-audience-binding-suggestion.object.ts | 6 ++++++ .../src/suggested-audience-bindings-install-path.test.ts | 8 +++++--- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/.changeset/tenant-scoped-declared-unique-indexes-8577.md b/.changeset/tenant-scoped-declared-unique-indexes-8577.md index 66b281ce21..f8448780c2 100644 --- a/.changeset/tenant-scoped-declared-unique-indexes-8577.md +++ b/.changeset/tenant-scoped-declared-unique-indexes-8577.md @@ -51,6 +51,15 @@ halves are now pinned end to end: two organizations installing the same package each end up with their own pending row, and re-running one organization's sync still adds nothing. +### One caveat on `sys_audience_binding_suggestion` + +This release makes a per-organization suggestion row **possible**; it is not yet +what the platform writes. The reconciler still reads and writes through a +tenant-less system context, so on a shared-runtime multi-organization +installation the surface continues to hold one organization-less row that every +tenant reads — measured, recorded as a test, and tracked in #8617, which remains +open. Single-organization installations are unaffected either way. + ## ⚠️ Operators: a migration is REQUIRED, and deploying this release is not it Respelling a declared index changes its generated **name**. On an existing diff --git a/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.object.ts b/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.object.ts index 4ec5ae4bc6..609dfefa5e 100644 --- a/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.object.ts +++ b/packages/plugins/plugin-security/src/objects/sys-audience-binding-suggestion.object.ts @@ -131,6 +131,12 @@ export const SysAudienceBindingSuggestion = ObjectSchema.create({ // org_yi (com.acme.crm, sales_readonly, guest) 201 / org_yi's own GET on // the colliding triple 0 rows. And through the REAL sync on a real engine: // org_jia created 1, org_yi created 0 with no throw and no log line. + // + // ⚠️ This is the STORAGE half only. `syncAudienceBindingSuggestions` still + // reads and writes through a tenant-less `{ isSystem: true }` context, so + // the shipped path writes ONE organization-less row every tenant reads — + // measured, and filed as #8617. Until that lands, this respelling is what + // makes a per-organization row possible, not what produces one. { fields: ['package_id', 'permission_set_name', 'anchor'], unique: 'organization' }, { fields: ['status'] }, { fields: ['package_id'] }, diff --git a/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts b/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts index 30c4783b89..1911499e60 100644 --- a/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts +++ b/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts @@ -324,11 +324,13 @@ describe('#8577 — the package-install path of sys_audience_binding_suggestion' * end as the index, reached by a different road, and NOT repaired by * respelling the index. The index fix is necessary (without it even a * correctly tenant-scoped write is refused) and it is what this card was - * ruled to deliver; the reconciler's tenant blindness is filed separately. + * ruled to deliver; the reconciler's tenant blindness is filed as **#8617**, + * which also carries the measurements above and the tenancy question they + * raise about this object. * * The two assertions below RECORD today's behaviour rather than endorse it. - * The follow-up that threads the tenant through must DELETE them, not update - * them — if they still pass afterwards, that fix did not work. + * #8617's fix must DELETE them, not update them — if they still pass + * afterwards, that fix did not work. */ describe('the shipped SYSTEM_CTX call path is tenant-blind (recorded, not endorsed)', () => { /** The reconciler wired exactly as `security-plugin.ts` wires it. */ From 13b2ca6b12cbee3b724ce4b5ca2fa99fec5d7343 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Fri, 14 Aug 2026 02:56:12 +0000 Subject: [PATCH 3/4] test(#8577): alias driver-sql and objectql to SOURCE in plugin-security's vitest config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check:test-source-alias measured the new install-path suite resolving both workspace deps through dist/ — which would make its verdict a function of build state. A stale driver-sql would report the pre-fix installation-wide index as per-organization: the #8577 defect itself, passing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- .../plugins/plugin-security/vitest.config.ts | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 packages/plugins/plugin-security/vitest.config.ts diff --git a/packages/plugins/plugin-security/vitest.config.ts b/packages/plugins/plugin-security/vitest.config.ts new file mode 100644 index 0000000000..53a043376f --- /dev/null +++ b/packages/plugins/plugin-security/vitest.config.ts @@ -0,0 +1,50 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { defineConfig } from 'vitest/config'; +import path from 'path'; + +export default defineConfig({ + test: { + globals: true, + environment: 'node', + }, + resolve: { + // [#8577] Both entries exist for `suggested-audience-bindings-install-path.test.ts`, + // the one suite here that imports sibling packages as VALUES: it drives the real + // `syncAudienceBindingSuggestions` against a real `ObjectQL` engine over a real + // `SqlDriver`, because the question it answers — does the SECOND organization to + // install a package get its own binding-suggestion row? — cannot be asked of a + // fake engine at all. + // + // Unaliased, those two specifiers resolve through the workspace link to `dist/` — + // a BUILD ARTIFACT — which would make this suite's verdict a function of build + // state rather than of the source in the checkout. The loud failure (missing + // export) is the mild half; a dist merely BEHIND runs GREEN against the + // dependency's old behaviour and says nothing. That is the exact hazard here: + // this suite's subject is the interaction between a DECLARED index's scope and + // the driver that materializes it, so a stale `driver-sql` would report the + // pre-fix installation-wide index as per-organization — the #8577 defect itself, + // passing. + // + // Turbo already orders `test` after `^build`, so `turbo run test` was never the + // failing path. The paths it does not mediate are: `pnpm test` inside this + // package, `vitest run `, an editor runner, or an agent working in a tree + // built at an older commit — precisely the ways this pin gets re-run WHILE + // someone is changing the index scope or the drift arm. + // + // Array form with anchored patterns, deliberately: the object form matches by + // PREFIX, so a bare key with a FILE replacement would also swallow any subpath + // import and resolve it to `…/src/index.ts/` (ENOTDIR) at run time, in + // a config that looks right. + alias: [ + { + find: /^@objectstack\/driver-sql$/, + replacement: path.resolve(__dirname, '../../drivers/driver-sql/src/index.ts'), + }, + { + find: /^@objectstack\/objectql$/, + replacement: path.resolve(__dirname, '../../objectql/src/index.ts'), + }, + ], + }, +}); From 56f8f1e588cff86110ad034b78fb8c706a6b2b63 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Fri, 14 Aug 2026 03:25:06 +0000 Subject: [PATCH 4/4] test(#8577): route the install-path seams' write verbs through ObjectQL's dispatch predicates check:engine-double-contract counted the two ql handles as engine doubles whose delete()/update() did not route through the producer's predicates. Fixed at the doubles, never by growing the shrink-only baseline. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- ...ted-audience-bindings-install-path.test.ts | 46 +++++++++++++++++-- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts b/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts index 1911499e60..9a0fc5ba02 100644 --- a/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts +++ b/packages/plugins/plugin-security/src/suggested-audience-bindings-install-path.test.ts @@ -2,6 +2,18 @@ import { describe, it, expect, afterEach } from 'vitest'; import { ObjectQL } from '@objectstack/objectql'; +// [#5619] The producer's OWN write-verb dispatch decisions. The two `ql` handles +// below are seams in front of a real engine, not fakes — but they are still +// doubles by the shape the contract gate reads, and routing their write verbs +// through the producer's predicates is what keeps a seam from accepting a call +// `ObjectQL` refuses. Imported from `@objectstack/objectql` rather than +// `@objectstack/metadata-core` (its home since #5619) on purpose: objectql +// re-exports both, it is already a devDependency of this package, and — measured, +// not assumed — `@objectstack/plugin-security` is NOT in objectql's runtime +// closure (12 packages), so this direction is not the cycle turbo refuses. It is +// also the specifier this package's `vitest.config.ts` already aliases to SOURCE, +// so it adds no new artifact-resolved import. +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; // The REAL shipped reconciler — not a re-implementation. A local copy would // make this suite a test of the copy, which is exactly the failure mode the @@ -137,14 +149,28 @@ async function boot(scope: true | 'organization'): Promise { * `listItems` returns `[]` on purpose — this models a RUNTIME package install * (`POST /api/v1/packages`), where the declaration reaches the reconciler * through the registry rather than through boot-declared stack metadata. + * + * ⚠️ `update` and `delete` open with the producer's dispatch predicates. Both + * verbs are seams the reconciler really uses — `update` on the + * pending→confirmed-observed branch, `delete` on the prune branch — even though + * this suite's fixtures (declaration present, binding absent) reach neither. + * Forwarding to a real engine cannot make a seam LOOSER than that engine, but + * the gate reads shape rather than provenance and the assertion costs nothing: + * it pins that whatever this seam forwards is a call `ObjectQL` would accept. */ function runtimeOf(engine: ObjectQL, organizationId: string): any { const withOrg = (o: any = {}) => ({ ...o, context: { ...(o.context ?? {}), tenantId: organizationId } }); return { find: (object: string, q: any = {}) => (engine as any).find(object, withOrg(q)), insert: (object: string, data: any, opt: any = {}) => (engine as any).insert(object, data, withOrg(opt)), - update: (object: string, data: any, opt: any = {}) => (engine as any).update(object, data, withOrg(opt)), - delete: (object: string, opt: any = {}) => (engine as any).delete(object, withOrg(opt)), + update: (object: string, data: any, opt: any = {}) => { + assertEngineUpdateDispatch(data, opt); + return (engine as any).update(object, data, withOrg(opt)); + }, + delete: (object: string, opt: any = {}) => { + assertEngineDeleteDispatch(opt); + return (engine as any).delete(object, withOrg(opt)); + }, registry: { listItems: () => [], getAllPackages: () => [{ enabled: true, manifest: PACKAGE_MANIFEST }], @@ -333,12 +359,22 @@ describe('#8577 — the package-install path of sys_audience_binding_suggestion' * afterwards, that fix did not work. */ describe('the shipped SYSTEM_CTX call path is tenant-blind (recorded, not endorsed)', () => { - /** The reconciler wired exactly as `security-plugin.ts` wires it. */ + /** + * The reconciler wired exactly as `security-plugin.ts` wires it — the bare + * engine, no tenant threaded. Write verbs route through the producer's + * dispatch predicates for the same reason as `runtimeOf` above. + */ const asShipped = (engine: ObjectQL): any => ({ find: (object: string, q: any = {}) => (engine as any).find(object, q), insert: (object: string, data: any, opt: any = {}) => (engine as any).insert(object, data, opt), - update: (object: string, data: any, opt: any = {}) => (engine as any).update(object, data, opt), - delete: (object: string, opt: any = {}) => (engine as any).delete(object, opt), + update: (object: string, data: any, opt: any = {}) => { + assertEngineUpdateDispatch(data, opt); + return (engine as any).update(object, data, opt); + }, + delete: (object: string, opt: any = {}) => { + assertEngineDeleteDispatch(opt); + return (engine as any).delete(object, opt); + }, registry: { listItems: () => [], getAllPackages: () => [{ enabled: true, manifest: PACKAGE_MANIFEST }],