diff --git a/.changeset/auto-org-admin-revoke-delete-signature.md b/.changeset/auto-org-admin-revoke-delete-signature.md new file mode 100644 index 0000000000..9a5ec4c563 --- /dev/null +++ b/.changeset/auto-org-admin-revoke-delete-signature.md @@ -0,0 +1,57 @@ +--- +"@objectstack/plugin-security": minor +--- + +fix(plugin-security): the org-admin auto-grant can actually revoke — demoted admins really do lose tenant admin (#4640) + +`auto-org-admin-grant`'s only delete channel called +`ql.delete(object, id, { context })`. The engine's signature is two arguments — +`delete(object, options?: EngineDeleteOptions)` — so the id landed in the option +bag, `rejectUnknownEngineOptions` read its character indices (`'0'`, `'1'`, …) +as unknown option keys and threw, and `tryDelete`'s `catch` swallowed it. The +system context in the discarded third argument went with it. + +That wrapper is the module's **only** delete channel, so all three revoke paths +were silent no-ops for the module's entire life: + +1. **Demotion and member removal did not take the capability back.** + `organization/update-member-role` moving someone from `owner`/`admin` back to + `member` reconciled, deleted nothing, and returned + `{ action: 'skipped', reason: 'delete_failed' }` while the + `sys_user_permission_set` row stayed put. That row carries wildcard + `viewAllRecords`/`modifyAllRecords` → `isTenantAdmin()`, so the demoted user + remained a **tenant admin**. +2. **The ADR-0105 D4 superseded-variant convergence never converged.** A posture + change left the old `organization_admin` / `organization_admin_no_bypass` row + in force — on a wall-less deployment, that is the unbounded variant. +3. **The `kernel:ready` orphan sweep never swept** (membership deleted, grant + left behind). + +The call now matches every other `ql.delete` call site in the repo: +`ql.delete(object, { where: { id }, context: SYSTEM_CTX })`. + +## ⚠️ Behaviour change: people will lose tenant admin on upgrade — that is the fix working + +Existing deployments have accumulated `sys_user_permission_set` rows that should +have been revoked when someone was demoted or removed from an organization. +After this release the `kernel:ready` backfill reconciles them, and every one of +those grants is deleted on the first boot. Concretely, on upgrade: + +- users demoted from `owner`/`admin` to `member` at any point in the past + **stop being tenant admins**; +- users whose membership was deleted lose their orphaned org-scoped grant; +- deployments that changed `tenancy.posture` converge on the posture's variant + instead of keeping both. + +Nobody loses access they were *supposed* to have: the grade that qualified them +was already taken away, and only the capability row outlived it. If a specific +person should keep blanket visibility, grant it deliberately — +`admin_full_access` or an explicitly authored permission set — rather than +through a better-auth membership grade. Expect `[security] revoked org-admin +capability` lines in the boot log naming each one. + +Failed revokes are no longer silent either: a delete the datastore rejects logs +`[security] org-admin grant revoke FAILED — capability still in force`, and a +reconcile that found grant rows and removed none logs that it left them behind. +A capability the platform decided to withdraw and could not is exactly the +outcome that must reach an operator. diff --git a/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts b/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts index 6a8f166948..3c85849d0a 100644 --- a/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts +++ b/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts @@ -8,9 +8,92 @@ import { autoOrgAdminGrantReason, } from './auto-org-admin-grant.js'; +// --------------------------------------------------------------------------- +// [#4640] The double speaks the ENGINE's signatures — or it proves nothing. +// +// The previous stub implemented `delete(object, id)`, a signature ObjectQL has +// never had. The module called `ql.delete(object, id, ctx)`; the stub happily +// deleted the row, every revoke test in this file went green, and in production +// the id landed in the option-bag slot where `rejectUnknownEngineOptions` reads its +// character indices as unknown keys and throws — straight into a swallowing +// `catch`. So for this module's entire life NOTHING was ever revoked: demoted +// admins kept `organization_admin`, hence tenant admin. +// +// A double looser than the real thing is not a weaker test — it is a test of a +// different program. This one therefore mirrors the engine's entry-point +// contract (`packages/objectql/src/engine.ts`) on both axes that matter: +// +// 1. ARITY AND ARGUMENT ROLES, which is where this bug lived: +// find(object, query: EngineQueryOptions, options?: EngineReadOptions) +// insert(object, data, options?) ← context in the 3rd arg +// delete(object, options?) ← context in the 2nd arg +// 2. `rejectUnknownEngineOptions`'s rule that an option key the engine does +// not execute is an ERROR — never something to quietly ignore. A +// positional argument in the bag slot fails this the same way it fails in +// the engine, so the same drift is loud here next time. +// --------------------------------------------------------------------------- + +/** Mirrors `ENGINE_FIND_OPTION_KEYS` in `packages/objectql/src/engine.ts`. */ +const FIND_QUERY_KEYS = new Set([ + 'context', 'where', 'fields', 'orderBy', 'limit', 'offset', 'search', 'searchFields', 'expand', +]); +/** Mirrors `ENGINE_DELETE_OPTION_KEYS` — note `where`, and note NO id argument. */ +const DELETE_OPTION_KEYS = new Set(['context', 'where', 'multi']); +/** The trailing read/write options bag (`EngineReadOptions` and friends). */ +const TRAILING_OPTION_KEYS = new Set(['context']); + +/** + * The engine's own unknown-key rule, applied to a double. + * + * Rejecting a non-object bag is the half that catches a positional argument: + * `Object.entries('ups_1')` yields `'0'/'1'/'2'…`, which is exactly how the + * real engine reports a mis-shaped call — the message just reads better here. + */ +function assertOptionBag( + operation: string, + object: string, + bag: unknown, + legal: ReadonlySet, +): void { + if (bag === undefined || bag === null) return; + if (typeof bag !== 'object' || Array.isArray(bag)) { + throw new Error( + `${operation}('${object}') takes an OPTION BAG in this position, got ${typeof bag} ` + + `(${String(bag)}). The engine names rows by \`where\`, never positionally — ` + + `e.g. delete(object, { where: { id }, context }).`, + ); + } + const unknown = Object.entries(bag as Record) + .filter(([k, v]) => v != null && !legal.has(k)) + .map(([k]) => k); + if (unknown.length > 0) { + throw new Error( + `${operation}('${object}') does not recognise option${unknown.length > 1 ? 's' : ''} ` + + `${unknown.map((k) => `'${k}'`).join(', ')}. The engine executes none of them, so the ` + + `call would succeed with the option silently ignored (#4371). ` + + `Legal keys for ${operation}: ${[...legal].sort().join(', ')}.`, + ); + } +} + /** - * Tiny in-memory ObjectQL stub: just enough surface for the reconciler - * (find / insert / delete) with isSystem context passthrough. + * This module's writes must run as the system (better-auth's identity tables + * refuse user-context writes — ADR-0092 D2). Dropping the context was the + * *other* casualty of the three-arg delete, so the double checks for it too. + */ +function assertSystemContext(operation: string, object: string, context: any): void { + if (!context || context.isSystem !== true) { + throw new Error( + `${operation}('${object}') reached the datastore without a system context ` + + `(got ${JSON.stringify(context) ?? 'undefined'}). The reconciler's own writes are ` + + `system writes; a dropped context is how a call shape silently loses its privileges.`, + ); + } +} + +/** + * Tiny in-memory ObjectQL double: just enough surface for the reconciler + * (find / insert / delete), with the engine's call shapes ENFORCED. */ function makeStub(seed: { sys_permission_set?: any[]; @@ -22,6 +105,8 @@ function makeStub(seed: { sys_member: seed.sys_member ?? [], sys_user_permission_set: seed.sys_user_permission_set ?? [], }; + /** Every delete the module issued, as the engine received it. */ + const deleteCalls: Array<{ object: string; options: any }> = []; const matches = (row: any, where: any) => { for (const [k, v] of Object.entries(where ?? {})) { @@ -37,19 +122,47 @@ function makeStub(seed: { return { tables, - async find(object: string, args: any) { - const rows = tables[object] ?? []; - return rows.filter((r) => matches(r, args?.where)); + deleteCalls, + // find(object, query, options) — `where`/`limit` in the query, execution + // context in either bag (`options.context` wins, as in the engine). + async find(object: string, query?: any, options?: any) { + assertOptionBag('find', object, query, FIND_QUERY_KEYS); + assertOptionBag('find', object, options, TRAILING_OPTION_KEYS); + assertSystemContext('find', object, options?.context ?? query?.context); + const rows = (tables[object] ?? []).filter((r) => matches(r, query?.where)); + return typeof query?.limit === 'number' ? rows.slice(0, query.limit) : rows; }, - async insert(object: string, data: any) { + // insert(object, data, options) — context in the TRAILING bag. + async insert(object: string, data: any, options?: any) { + assertOptionBag('insert', object, options, TRAILING_OPTION_KEYS); + assertSystemContext('insert', object, options?.context); + if (!data || typeof data !== 'object' || Array.isArray(data)) { + throw new Error(`insert('${object}') takes a record object as its second argument.`); + } const id = data.id ?? `${object}_${tables[object].length + 1}`; const row = { ...data, id }; tables[object] = [...(tables[object] ?? []), row]; return row; }, - async delete(object: string, id: string) { - tables[object] = (tables[object] ?? []).filter((r) => r.id !== id); - return true; + // delete(object, options) — TWO arguments. The row is named by + // `where.id`; there is no positional id and no third argument. + async delete(object: string, options?: any) { + assertOptionBag('delete', object, options, DELETE_OPTION_KEYS); + assertSystemContext('delete', object, options?.context); + deleteCalls.push({ object, options }); + const where = options?.where; + const id = where && typeof where === 'object' ? (where as any).id : undefined; + const scalarId = typeof id === 'string' || typeof id === 'number' ? id : undefined; + if (scalarId === undefined && options?.multi !== true) { + // The engine's own refusal — an unscoped delete never runs by accident. + throw new Error('Delete requires an ID or options.multi=true'); + } + const before = tables[object] ?? []; + tables[object] = + scalarId !== undefined + ? before.filter((r) => r.id !== scalarId) + : before.filter((r) => !matches(r, where)); + return before.length - tables[object].length; }, }; } @@ -369,3 +482,97 @@ describe('[#4586] the auto-grant records its provenance', () => { expect(row.reason).toContain('mem_7'); }); }); + +// --------------------------------------------------------------------------- +// [#4640] The revoke channel, pinned at the call SHAPE. +// +// Every `revoked` assertion in this file was already green while production +// revoked nothing, because the double implemented the wrong signature. The +// tests below pin the two things that green-ness depended on and nobody was +// checking: the exact call the module hands the engine, and the double's +// refusal to accept anything else. +// --------------------------------------------------------------------------- +describe('[#4640] revoke speaks the engine\'s delete signature', () => { + const seedDemoted = () => + makeStub({ + sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'member' }], + sys_user_permission_set: [ + { id: 'ups1', user_id: 'u1', organization_id: 'o1', permission_set_id: 'ps_org_admin' }, + ], + }); + + it('names the row by `where.id` in a TWO-argument call carrying the system context', async () => { + const stub = seedDemoted(); + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + + expect(res.action).toBe('revoked'); + expect(stub.deleteCalls).toHaveLength(1); + const [call] = stub.deleteCalls; + expect(call.object).toBe('sys_user_permission_set'); + // The whole bug in one assertion: the id belongs INSIDE the option bag. + expect(call.options).toEqual({ where: { id: 'ups1' }, context: { isSystem: true } }); + }); + + it('the double refuses the three-argument call the module used to make', async () => { + // The drift guard. If a future edit reverts the call shape — or loosens + // this double back toward `delete(object, id)` — this is what goes red + // instead of the whole feature going silently inert. + const stub = seedDemoted(); + await expect( + (stub as any).delete('sys_user_permission_set', 'ups1', { context: { isSystem: true } }), + ).rejects.toThrow(/takes an OPTION BAG/); + expect(stub.tables.sys_user_permission_set).toHaveLength(1); + }); + + it('a delete the datastore rejects is REPORTED — never a silent no-op', async () => { + // The other half of why this survived: the wrapper's `catch {}` turned a + // throwing revoke into `false` and told nobody. The capability is still in + // force, so that has to reach an operator. + const stub = seedDemoted(); + stub.delete = async () => { + throw new Error('driver exploded'); + }; + const warnings: Array<{ msg: string; meta?: any }> = []; + const logger = { warn: (msg: string, meta?: any) => warnings.push({ msg, meta }) }; + + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', { ...WALLED, logger }); + + expect(res).toEqual({ action: 'skipped', reason: 'delete_failed' }); + // The grant row is still there — the state the warning is about. + expect(stub.tables.sys_user_permission_set).toHaveLength(1); + expect(warnings.map((w) => w.msg)).toEqual([ + '[security] org-admin grant revoke FAILED — capability still in force', + '[security] org-admin capability could NOT be revoked — grant rows remain', + ]); + expect(warnings[0].meta.error).toBe('driver exploded'); + }); + + it('"nothing to revoke" stays distinguishable from "revoke failed"', async () => { + // `noop` and `skipped/delete_failed` are different facts about the + // platform's state; collapsing them is how the failure hid. + const stub = makeStub({ + sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'member' }], + sys_user_permission_set: [], + }); + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + expect(res).toEqual({ action: 'noop' }); + expect(stub.deleteCalls).toHaveLength(0); + }); + + it('membership removal revokes through the same channel', async () => { + // The `sys_member` delete path: no membership row at all, grant still there. + const stub = makeStub({ + sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_member: [], + sys_user_permission_set: [ + { id: 'ups1', user_id: 'u1', organization_id: 'o1', permission_set_id: 'ps_org_admin' }, + ], + }); + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + expect(res.action).toBe('revoked'); + expect(stub.tables.sys_user_permission_set).toHaveLength(0); + expect(stub.deleteCalls[0].options.where).toEqual({ id: 'ups1' }); + }); +}); diff --git a/packages/plugins/plugin-security/src/auto-org-admin-grant.ts b/packages/plugins/plugin-security/src/auto-org-admin-grant.ts index fb5da61260..bb3e8943b0 100644 --- a/packages/plugins/plugin-security/src/auto-org-admin-grant.ts +++ b/packages/plugins/plugin-security/src/auto-org-admin-grant.ts @@ -74,28 +74,69 @@ function genId(prefix: string): string { return `${prefix}_${ts}${rand}`; } -async function tryFind(ql: any, object: string, where: any, limit = 50): Promise { +/** + * [#4640] The engine call shapes this module speaks, spelled out because + * getting one wrong here is silent: every wrapper below swallows the throw, + * so a call that no longer matches `ObjectQL`'s signature degrades into a + * no-op that still reports "nothing to do". + * + * `packages/objectql/src/engine.ts` — the only signatures that exist: + * + * find(object, query: EngineQueryOptions, options?: EngineReadOptions) + * insert(object, data, options?: DataEngineInsertOptions) + * delete(object, options?: EngineDeleteOptions) ← TWO args; the row is + * named by `where`, and + * the context rides in + * the same bag + * + * `delete` is the odd one out: reads and inserts take their execution context + * in a THIRD argument, deletes take it in the second. A `delete(object, id, + * ctx)` call therefore hands the id in as the option bag, where + * `rejectUnknownEngineOptions` reads its character indices as unknown option + * keys and throws — and drops the system context on the floor along the way. + * That was this module's only revoke channel for its whole life (#4640). + */ + +async function tryFind(ql: any, object: string, where: any, limit = 50, logger?: MaybeLogger): Promise { try { const rows = await ql.find(object, { where, limit }, { context: SYSTEM_CTX }); return Array.isArray(rows) ? rows : Array.isArray(rows?.records) ? rows.records : []; - } catch { + } catch (e) { + // Reads legitimately fail before the tables exist (boot ordering), so this + // is debug rather than warn — but it is no longer nothing (#4640). + logger?.debug?.('[security] org-admin reconcile read failed — treated as no rows', { + object, + error: (e as Error)?.message, + }); return []; } } -async function tryInsert(ql: any, object: string, data: any): Promise { +async function tryInsert(ql: any, object: string, data: any, logger?: MaybeLogger): Promise { try { return await ql.insert(object, data, { context: SYSTEM_CTX }); - } catch { + } catch (e) { + logger?.warn?.('[security] org-admin grant insert failed — capability NOT granted', { + object, + error: (e as Error)?.message, + }); return null; } } -async function tryDelete(ql: any, object: string, id: string): Promise { +async function tryDelete(ql: any, object: string, id: string, logger?: MaybeLogger): Promise { try { - await ql.delete(object, id, { context: SYSTEM_CTX }); + await ql.delete(object, { where: { id }, context: SYSTEM_CTX }); return true; - } catch { + } catch (e) { + // [#4640] A failed revoke means a capability the platform decided to take + // away is still in force — the one failure in this module that must never + // be silent, whatever the caller does with the `false`. + logger?.warn?.('[security] org-admin grant revoke FAILED — capability still in force', { + object, + id, + error: (e as Error)?.message, + }); return false; } } @@ -167,7 +208,11 @@ export function autoOrgAdminGrantReason( */ const permissionSetIdCache = new WeakMap>(); -async function resolvePermissionSetId(ql: any, name: string): Promise { +async function resolvePermissionSetId( + ql: any, + name: string, + logger?: MaybeLogger, +): Promise { let perQl = permissionSetIdCache.get(ql); if (!perQl) { perQl = new Map(); @@ -175,7 +220,7 @@ async function resolvePermissionSetId(ql: any, name: string): Promise 0) { perQl.set(name, id); @@ -234,7 +279,7 @@ export async function reconcileOrgAdminGrant( const grantSetName = orgAdminSetNameForPosture(posture); const supersededSetName = supersededOrgAdminSetName(posture); - const permSetId = await resolvePermissionSetId(ql, grantSetName); + const permSetId = await resolvePermissionSetId(ql, grantSetName, logger); if (!permSetId) { // The permission set isn't seeded yet (boot ordering) — caller can retry // later (e.g. via kernel:ready backfill). @@ -250,6 +295,7 @@ export async function reconcileOrgAdminGrant( 'sys_member', { user_id: userId, organization_id: orgId }, 10, + logger, ); // The row that QUALIFIES is also the row the grant is provenance-linked to // (#4586) — "this capability exists because of that membership". @@ -259,16 +305,17 @@ export async function reconcileOrgAdminGrant( // 1b. [ADR-0105 D4] Revoke the OTHER variant for this pair, always. A posture // change (or a downgrade after F2) must converge on exactly one org-admin // grant; leaving the superseded row would keep the old bits in force. - const supersededSetId = await resolvePermissionSetId(ql, supersededSetName); + const supersededSetId = await resolvePermissionSetId(ql, supersededSetName, logger); if (supersededSetId) { const stale = await tryFind( ql, 'sys_user_permission_set', { user_id: userId, organization_id: orgId, permission_set_id: supersededSetId }, 5, + logger, ); for (const row of stale) { - if (row?.id && (await tryDelete(ql, 'sys_user_permission_set', String(row.id)))) { + if (row?.id && (await tryDelete(ql, 'sys_user_permission_set', String(row.id), logger))) { logger?.info?.('[security] revoked superseded org-admin grant', { userId, orgId, @@ -285,29 +332,35 @@ export async function reconcileOrgAdminGrant( 'sys_user_permission_set', { user_id: userId, organization_id: orgId, permission_set_id: permSetId }, 5, + logger, ); if (shouldGrant) { if (existingGrants.length > 0) { // Deduplicate stale duplicates if any slipped through. for (const extra of existingGrants.slice(1)) { - if (extra?.id) await tryDelete(ql, 'sys_user_permission_set', String(extra.id)); + if (extra?.id) await tryDelete(ql, 'sys_user_permission_set', String(extra.id), logger); } return { action: 'noop' }; } - const created = await tryInsert(ql, 'sys_user_permission_set', { - id: genId('ups'), - user_id: userId, - permission_set_id: permSetId, - organization_id: orgId, - // [#4586] The provenance the row already had a column for. `granted_by` - // is a `sys_user` lookup: the human whose better-auth call triggered the - // grade change when one was in scope, else `null` = the system (ADR-0118 - // D1 — an id or null, never a sentinel). The machine marker and the - // triggering membership row live in `reason`, where free text belongs. - granted_by: options.attributedUserId ?? null, - reason: autoOrgAdminGrantReason(qualifyingMembership, grantSetName), - }); + const created = await tryInsert( + ql, + 'sys_user_permission_set', + { + id: genId('ups'), + user_id: userId, + permission_set_id: permSetId, + organization_id: orgId, + // [#4586] The provenance the row already had a column for. `granted_by` + // is a `sys_user` lookup: the human whose better-auth call triggered the + // grade change when one was in scope, else `null` = the system (ADR-0118 + // D1 — an id or null, never a sentinel). The machine marker and the + // triggering membership row live in `reason`, where free text belongs. + granted_by: options.attributedUserId ?? null, + reason: autoOrgAdminGrantReason(qualifyingMembership, grantSetName), + }, + logger, + ); if (created) { logger?.info?.('[security] granted org-admin capability', { userId, @@ -327,7 +380,7 @@ export async function reconcileOrgAdminGrant( } let removed = 0; for (const row of existingGrants) { - if (row?.id && (await tryDelete(ql, 'sys_user_permission_set', String(row.id)))) { + if (row?.id && (await tryDelete(ql, 'sys_user_permission_set', String(row.id), logger))) { removed += 1; } } @@ -340,6 +393,16 @@ export async function reconcileOrgAdminGrant( }); return { action: 'revoked' }; } + // [#4640] Rows were found and none could be removed: the user keeps an + // org-admin capability the platform just decided they should not have. The + // `skipped` return already said so; nothing was reading it, which is how the + // broken call shape survived — so say it out loud as well. + logger?.warn?.('[security] org-admin capability could NOT be revoked — grant rows remain', { + userId, + orgId, + set: grantSetName, + remaining: existingGrants.length, + }); return { action: 'skipped', reason: 'delete_failed' }; } @@ -359,7 +422,7 @@ export async function backfillOrgAdminGrants( const summary = { scanned: 0, granted: 0, revoked: 0, skipped: 0 }; if (!ql || typeof ql.find !== 'function') return summary; - const permSetId = await resolvePermissionSetId(ql, orgAdminSetNameForPosture(posture)); + const permSetId = await resolvePermissionSetId(ql, orgAdminSetNameForPosture(posture), logger); if (!permSetId) { logger?.debug?.('[security] org-admin backfill skipped — permission set missing'); return summary; @@ -367,9 +430,13 @@ export async function backfillOrgAdminGrants( // [ADR-0105 D4] The orphan sweep below must see BOTH variants: a boot that // changed posture leaves grants of the superseded set behind, and those are // exactly the rows whose bits must stop applying. - const supersededId = await resolvePermissionSetId(ql, supersededOrgAdminSetName(posture)); + const supersededId = await resolvePermissionSetId( + ql, + supersededOrgAdminSetName(posture), + logger, + ); - const members = await tryFind(ql, 'sys_member', {}, limit); + const members = await tryFind(ql, 'sys_member', {}, limit, logger); // De-duplicate by (user_id, organization_id) pair — a user with two // membership rows (e.g. legacy duplicates) only needs one reconcile. const seen = new Set(); @@ -396,6 +463,7 @@ export async function backfillOrgAdminGrants( 'sys_user_permission_set', { permission_set_id: { $in: grantSetIds } }, limit, + logger, ); for (const g of allGrants) { const userId = String(g?.user_id ?? ''); diff --git a/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts b/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts index 2fce49a32b..a2cdb4c22a 100644 --- a/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts +++ b/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts @@ -297,14 +297,26 @@ describe('#4586: the better-auth actor reaches sys_member history and the grant' expect(String(promotion.id)).not.toBe(String(demotion.id)); expect(promotion.user_id).toBe(adminUserId); - // NOTE: whether the org-admin GRANT is revoked on demotion is deliberately - // not asserted here. It currently is not — `auto-org-admin-grant`'s only - // delete channel calls `ql.delete(object, id, ctx)` while the engine takes - // `(object, { where, context })`, so every revoke throws into a swallowing - // catch. That is a separate, pre-existing defect (its unit stub implements - // the wrong signature, so the suite never saw it) filed as #4640 — a - // security behaviour change that deserves its own review and changeset, not - // a rider on an attribution PR. + // [#4640] …and the CAPABILITY goes with the grade. The grant W2 asserted + // above must be gone: that row is what `isTenantAdmin()` reads, so a + // demotion that leaves it behind leaves a tenant admin behind. This is the + // assertion that could only ever have been made here — the unit suite's + // double implemented `delete(object, id)`, a signature the engine has never + // had, so it reported every revoke as successful while the real engine + // threw on all of them. The reconcile runs in an ObjectQL middleware after + // the route's write, which better-auth may settle after the response, so + // poll rather than read once. + let grants: any[] = [{ pending: true }]; + for (let i = 0; i < 20 && grants.length > 0; i++) { + grants = await findRows( + ql, + 'sys_user_permission_set', + { user_id: memberUserId, organization_id: orgId }, + 5, + ); + if (grants.length > 0) await new Promise((r) => setTimeout(r, 250)); + } + expect(grants).toHaveLength(0); }, 60_000); // ── The hard constraint ─────────────────────────────────────────────────