diff --git a/.changeset/enforce-active-on-grant-catalogues.md b/.changeset/enforce-active-on-grant-catalogues.md new file mode 100644 index 0000000000..6c05ba5fb1 --- /dev/null +++ b/.changeset/enforce-active-on-grant-catalogues.md @@ -0,0 +1,79 @@ +--- +"@objectstack/core": minor +"@objectstack/plugin-security": minor +"@objectstack/plugin-auth": minor +--- + +fix(security): `sys_permission_set.active` and `sys_position.active` now actually stop granting access (#8613) + + + +**BREAKING for deployments that already switched a permission set or position +off.** Both objects ship a Deactivate action whose confirmation dialog promises, +in all four locales, that access stops: + +> Deactivate this permission set? Existing assignments stay in place but stop +> granting access until re-activated. +> Deactivate this position? Users keep their assignment but the position stops +> granting permissions until re-activated. + +Nothing read the column. Measured on the real resolver: a position seeded +`active: false` still granted its permission sets, and a permission set seeded +`active: false` still returned `posture: PLATFORM_ADMIN` with its system +permissions. Deactivation moved a badge in Setup and nothing else — while the +admin who had just revoked a compromised or over-broad grant was told the +opposite, and whose likely next step was therefore *not* the action that would +have worked (delete the set, or remove the assignments). + +**What changes at runtime.** `resolveAuthzContext` / `resolveUserAuthzGrants` +(`@objectstack/core`) — the single seam every transport resolves authorization +through — now drop a deactivated row **before** any derivation: + +- a deactivated `sys_position` no longer contributes its + `sys_position_permission_set` grants, and its name leaves `positions` (so the + name-reuse path cannot resolve the same grant one layer down); +- a deactivated `sys_permission_set` contributes no name, no + `system_permissions`, no `tab_permissions`, **and no `PLATFORM_ADMIN` + posture** — the flag is applied before the posture is derived, not after; +- the `plugin-security` DB loader applies the same predicate, which is what + judges a set reached by NAME through an active position of the same name. + +Both tables were already read at that seam, so this costs **zero new hot-path +queries**. + +**⚠️ Read this before upgrading.** Any `sys_permission_set` or `sys_position` +row currently carrying `active: false` **stops granting the moment this +lands** — on live data, with no migration step to notice. That is the correct +direction (it is what the dialog said when someone clicked Deactivate), but on +an installation that used the switch believing it was inert it is a real +revocation. Before upgrading, list the deactivated rows and re-activate any that +are still meant to grant: + +``` +GET /api/v1/data/sys_permission_set?filters=[["active","=",false]] +GET /api/v1/data/sys_position?filters=[["active","=",false]] +``` + +A row whose `active` column is **absent or NULL** is unaffected: the predicate +is "explicitly deactivated", never "explicitly active", so rows that predate the +column keep granting exactly as before. + +**Break-glass, closed in the same change** (`@objectstack/plugin-auth`). +Enforcing the flag opened a one-click, installation-wide lockout: deactivating +`admin_full_access` un-makes every platform admin at once, through a payload +that touches neither `name` nor any identity table, and re-activating requires +the permission the click just took away (the seeders deliberately never +reconcile `active`, so no restart restores it). The last-administrator guard now +judges that write like the delete and rename spellings it already refused, and +an environment whose break-glass set is *already* off is read as emptied rather +than as a bootstrap window — so it does not silently disarm the guard for every +other identity write. Re-activation itself stays permitted, or the refusal would +have no way out from inside the product. diff --git a/content/docs/permissions/authorization.mdx b/content/docs/permissions/authorization.mdx index a765ddd1cd..8b886a3cc5 100644 --- a/content/docs/permissions/authorization.mdx +++ b/content/docs/permissions/authorization.mdx @@ -330,6 +330,48 @@ product — see ADR-0091 D4–D7 for the open-core line; their community *shapes (a time-boxed direct grant with a reason; certification stamps) are the L1 substrate above. +## Grant lifecycle: the `active` switch (ADR-0049) + +Validity windows above date a **user's grant row**. The second lifecycle +control dates the **catalogue row itself**: `sys_permission_set.active` and +`sys_position.active`, the switch behind the Deactivate action on both objects. +It answers a different question — "switch this grant off for everyone, without +deleting it or unwinding the assignments" — and it is enforced in the same +place, by the same discipline: **resolution-time filtering, fail-closed**, in +`resolveAuthzContext`, with no cleanup job involved. + +- A deactivated **permission set** contributes nothing: not its name, not its + `system_permissions`, not its `tab_permissions`. As with an expired grant, a + deactivated unscoped `admin_full_access` no longer derives `platform_admin` + — the flag is applied *before* the posture is derived, not after. +- A deactivated **position** stops carrying its permission sets, and its name + stops appearing in `positions`, so a permission set that merely shares the + position's name cannot resolve through it either. +- Assignments are untouched. `sys_user_position` and + `sys_user_permission_set` rows stay exactly as they were, and re-activating + the catalogue row restores every grant it carried, at the next resolution. + +**Absent is ACTIVE.** Only a stored value that really reads false takes a grant +away, so a row that predates the column keeps granting. The same predicate +(`isRowActive`) is used by every reader, including the last-administrator +guard's simulation — a guard that modelled "deactivated" differently from the +resolver would permit exactly the write it exists to refuse. + +**Deactivating the break-glass set is refused.** `admin_full_access` is what +makes the environment's platform admins, so switching it off would un-make all +of them in one write — and re-activating it needs the permission just lost. +That write is judged like deleting or renaming the row (ADR-0024 D5.2): it is +refused while it would leave the environment with no administrator who can +sign in. Re-activation is never refused. + +Deactivation is an incident-response control, so what it does **not** touch is +deliberate. Administration surfaces keep listing and editing a deactivated +position — an admin must still be able to unbind and clean up what they just +switched off — and the write gates that judge audience-anchor bindings and a +delegated administrator's blast radius keep reading every row, deactivated +included: dropping rows there would make a refused binding *permitted* and a +delegate's boundary *narrower*, which is the opposite of switching access off. + ## Governance: how "declared = enforced" is kept true Five mechanisms — four CI-time, one runtime — make the security posture a diff --git a/packages/core/src/security/index.ts b/packages/core/src/security/index.ts index d2bd7b1a29..a9afe5e46c 100644 --- a/packages/core/src/security/index.ts +++ b/packages/core/src/security/index.ts @@ -137,6 +137,11 @@ export { // ADR-0091 D1/D2 — grant validity windows, the shared resolution-time predicate. export { isGrantActive, isGrantExpired, type GrantValidityWindow } from './grant-validity.js'; +// ADR-0049 enforce-or-remove — the `active` flag on the RBAC grant catalogues +// (`sys_permission_set` / `sys_position`). One predicate for the resolver that +// enforces it and the break-glass guard that simulates a write to it. +export { isRowActive, type ActivatableRow } from './row-active.js'; + // [#7678] ADR-0090 D5/D9 — the audience-binding suggestion `?status=` vocabulary, // shared by the runtime dispatcher's `/security` domain and the live REST route. export { diff --git a/packages/core/src/security/resolve-authz-context.test.ts b/packages/core/src/security/resolve-authz-context.test.ts index 82f672c585..0d22b1ef01 100644 --- a/packages/core/src/security/resolve-authz-context.test.ts +++ b/packages/core/src/security/resolve-authz-context.test.ts @@ -466,3 +466,228 @@ describe('resolveUserAuthzGrants — userId-driven authz for non-HTTP surfaces ( }); }); + +/** + * [#8613 / ADR-0049] `sys_permission_set.active` and `sys_position.active` — + * enforce-or-remove, enforced. + * + * Both objects ship a Deactivate action whose dialog promises, in four locales, + * that access stops. Nothing read the column, so the promise was false: the + * assignments kept granting and the admin who trusted the dialog did not take + * the action that would actually have worked. + * + * This is the ONLY seam where either flag is enforceable. Downstream in + * plugin-security the position → permission-set linkage is already collapsed + * into a flat `permissions` list, so a set held via a deactivated position is + * indistinguishable there from one granted directly — filtering there would + * over-revoke a set the user also holds in their own right. + * + * The predicate is "explicitly deactivated", not "explicitly active": absent + * means ACTIVE, so a row that predates the column keeps working. Every fixture + * above this block carries no `active` key at all and is the pin for that + * direction — requiring `true` would have turned this file red wholesale, which + * is what it would do to deployed data. + */ +describe('[#8613] the `active` flag on the grant catalogues (ADR-0049)', () => { + const withActive = (v: unknown) => ({ + sys_user: [{ id: 'u1' }], + sys_member: [], + sys_user_position: [{ user_id: 'u1', position: 'contributor', organization_id: null }], + sys_user_permission_set: [], + sys_position: [{ id: 'r1', name: 'contributor', active: v }], + sys_position_permission_set: [{ position_id: 'r1', permission_set_id: 'ps1' }], + sys_permission_set: [{ id: 'ps1', name: 'contributor_ps', system_permissions: ['cap_x'] }], + }); + + // ── sys_position ────────────────────────────────────────────────────────── + + it('a DEACTIVATED position stops granting its permission sets', async () => { + const ctx = await resolveAuthzContext({ + ql: makeQl(withActive(false)), + headers: H(), + getSession: session('u1'), + }); + expect(ctx.permissions).not.toContain('contributor_ps'); + expect(ctx.systemPermissions).not.toContain('cap_x'); + }); + + it('…and its NAME leaves `positions` too, or the name alone would resolve the set', async () => { + // `resolvePermissionSetsForContext` requests `context.positions` as + // permission-set names (position names are commonly reused as set names), + // so a name left standing would resolve the same grant one layer down. + const ctx = await resolveAuthzContext({ + ql: makeQl(withActive(false)), + headers: H(), + getSession: session('u1'), + }); + expect(ctx.positions).not.toContain('contributor'); + // The audience anchor is untouched — it is not the deactivated row. + expect(ctx.positions).toContain('everyone'); + }); + + it('an ACTIVE position still grants (the flag is not a blanket revocation)', async () => { + const ctx = await resolveAuthzContext({ + ql: makeQl(withActive(true)), + headers: H(), + getSession: session('u1'), + }); + expect(ctx.positions).toContain('contributor'); + expect(ctx.permissions).toContain('contributor_ps'); + expect(ctx.systemPermissions).toContain('cap_x'); + }); + + it('an ABSENT `active` column grants — deployed rows are not mass-revoked', async () => { + const ctx = await resolveAuthzContext({ + ql: makeQl(withActive(undefined)), + headers: H(), + getSession: session('u1'), + }); + expect(ctx.positions).toContain('contributor'); + expect(ctx.permissions).toContain('contributor_ps'); + }); + + it('the 0/1 storage shape deactivates too — what the primary driver returns', async () => { + const off = await resolveAuthzContext({ + ql: makeQl(withActive(0)), + headers: H(), + getSession: session('u1'), + }); + expect(off.permissions).not.toContain('contributor_ps'); + const on = await resolveAuthzContext({ + ql: makeQl(withActive(1)), + headers: H(), + getSession: session('u1'), + }); + expect(on.permissions).toContain('contributor_ps'); + }); + + it('a position name with NO `sys_position` row is untouched (org roles, memberships)', async () => { + const ql = makeQl({ + sys_user: [{ id: 'u1' }], + sys_member: [{ user_id: 'u1', role: 'owner', organization_id: 'o1' }], + sys_user_position: [], + sys_user_permission_set: [], + // `org_owner` is projected from the membership and has no catalogue row — + // there is no flag to read, so nothing may be inferred from its absence. + sys_position: [{ id: 'r9', name: 'something_else', active: false }], + }); + const ctx = await resolveAuthzContext({ ql, headers: H(), getSession: session('u1', { org: 'o1' }) }); + expect(ctx.positions).toContain('org_owner'); + }); + + it('deactivating ONE position leaves the others granting', async () => { + const ql = makeQl({ + sys_user: [{ id: 'u1' }], + sys_member: [], + sys_user_position: [ + { user_id: 'u1', position: 'contributor', organization_id: null }, + { user_id: 'u1', position: 'reviewer', organization_id: null }, + ], + sys_user_permission_set: [], + sys_position: [ + { id: 'r1', name: 'contributor', active: false }, + { id: 'r2', name: 'reviewer', active: true }, + ], + sys_position_permission_set: [ + { position_id: 'r1', permission_set_id: 'ps1' }, + { position_id: 'r2', permission_set_id: 'ps2' }, + ], + sys_permission_set: [ + { id: 'ps1', name: 'contributor_ps' }, + { id: 'ps2', name: 'reviewer_ps' }, + ], + }); + const ctx = await resolveAuthzContext({ ql, headers: H(), getSession: session('u1') }); + expect(ctx.permissions).not.toContain('contributor_ps'); + expect(ctx.permissions).toContain('reviewer_ps'); + expect(ctx.positions).not.toContain('contributor'); + expect(ctx.positions).toContain('reviewer'); + }); + + // ── sys_permission_set ──────────────────────────────────────────────────── + + it('a DEACTIVATED permission set grants nothing — name, capabilities and tabs', async () => { + const ql = makeQl({ + sys_user: [{ id: 'u1' }], + sys_member: [], + sys_user_position: [], + sys_user_permission_set: [{ user_id: 'u1', permission_set_id: 'ps1', organization_id: null }], + sys_permission_set: [{ + id: 'ps1', + name: 'crm_full', + active: false, + system_permissions: ['cap_x'], + tab_permissions: { crm: 'visible' }, + }], + }); + const ctx = await resolveAuthzContext({ ql, headers: H(), getSession: session('u1') }); + expect(ctx.permissions).not.toContain('crm_full'); + expect(ctx.systemPermissions).not.toContain('cap_x'); + expect(ctx.tabPermissions?.crm).toBeUndefined(); + }); + + it('THE HIGH-BLAST-RADIUS CASE: a deactivated admin_full_access confers no PLATFORM_ADMIN', async () => { + const ql = makeQl({ + sys_user: [{ id: 'u1' }], + sys_member: [], + sys_user_position: [], + sys_user_permission_set: [{ user_id: 'u1', permission_set_id: 'psA', organization_id: null }], + sys_permission_set: [{ + id: 'psA', + name: 'admin_full_access', + active: false, + system_permissions: ['manage_users'], + }], + }); + const ctx = await resolveAuthzContext({ ql, headers: H(), getSession: session('u1') }); + // Dropped BEFORE the derivation, so the posture cannot be read off a set + // that no longer grants — the whole point of filtering at §6b rather than + // after it. + expect(ctx.permissions).not.toContain('admin_full_access'); + expect(ctx.posture).not.toBe('PLATFORM_ADMIN'); + expect(ctx.positions).not.toContain('platform_admin'); + expect(ctx.systemPermissions).not.toContain('manage_users'); + }); + + it('deactivating ONE set leaves the others granting', async () => { + const ql = makeQl({ + sys_user: [{ id: 'u1' }], + sys_member: [], + sys_user_position: [], + sys_user_permission_set: [ + { user_id: 'u1', permission_set_id: 'ps1', organization_id: null }, + { user_id: 'u1', permission_set_id: 'ps2', organization_id: null }, + ], + sys_permission_set: [ + { id: 'ps1', name: 'crm_full', active: false }, + { id: 'ps2', name: 'crm_read', active: true }, + ], + }); + const ctx = await resolveAuthzContext({ ql, headers: H(), getSession: session('u1') }); + expect(ctx.permissions).not.toContain('crm_full'); + expect(ctx.permissions).toContain('crm_read'); + }); + + it('a set held via BOTH a deactivated position and a direct grant still resolves', async () => { + // The over-revocation this seam is chosen to avoid: the direct grant is a + // separate authority and the position's deactivation may not touch it. + const ql = makeQl({ + sys_user: [{ id: 'u1' }], + sys_member: [], + sys_user_position: [{ user_id: 'u1', position: 'contributor', organization_id: null }], + sys_user_permission_set: [{ user_id: 'u1', permission_set_id: 'ps1', organization_id: null }], + sys_position: [{ id: 'r1', name: 'contributor', active: false }], + sys_position_permission_set: [{ position_id: 'r1', permission_set_id: 'ps1' }], + sys_permission_set: [{ id: 'ps1', name: 'crm_full' }], + }); + const ctx = await resolveAuthzContext({ ql, headers: H(), getSession: session('u1') }); + expect(ctx.permissions).toContain('crm_full'); + expect(ctx.positions).not.toContain('contributor'); + }); + + it('resolveUserAuthzGrants enforces it too — the non-HTTP surfaces share the seam', async () => { + const grants = await resolveUserAuthzGrants(makeQl(withActive(false)), 'u1'); + expect(grants.permissions).not.toContain('contributor_ps'); + expect(grants.positions).not.toContain('contributor'); + }); +}); diff --git a/packages/core/src/security/resolve-authz-context.ts b/packages/core/src/security/resolve-authz-context.ts index a155caa65c..86821d0da6 100644 --- a/packages/core/src/security/resolve-authz-context.ts +++ b/packages/core/src/security/resolve-authz-context.ts @@ -36,6 +36,7 @@ import type { AuthzPosture } from '@objectstack/spec/security'; import { resolveApiKeyPrincipal } from './api-key.js'; import { isGrantActive } from './grant-validity.js'; import { derivePosture } from './posture-ladder.js'; +import { isRowActive } from './row-active.js'; /** The transport-agnostic authorization envelope produced from a request. */ export interface ResolvedAuthzContext { @@ -367,9 +368,31 @@ export async function resolveUserAuthzGrants( // 6a. Position-bound permission sets (sys_position_permission_set): a position // carries its permission sets. + // + // [ADR-0049] A DEACTIVATED position grants nothing — the `deactivate_position` + // dialog's promise ("users keep their assignment but the position stops + // granting permissions"), enforced at the ONE place it is enforceable. + // Downstream in plugin-security the position→set linkage is already + // collapsed into a flat `permissions` list, so a set held via a + // deactivated position is indistinguishable there from one granted + // directly and filtering there would over-revoke. + // + // The name is dropped from `positions` too, not merely from the junction + // read: `resolvePermissionSetsForContext` requests `positions` as + // permission-set NAMES (position names are commonly reused as set names), + // so a name left standing would resolve the same grant one layer down. + // Only a name whose row is explicitly deactivated is dropped — a name + // with no `sys_position` row at all (`org_owner`, a membership-derived + // role) has no flag to read and is untouched. if (grants.positions.length > 0) { const positionRows = await tryFind(ql, 'sys_position', { name: { $in: grants.positions } }, 100); - const positionIds = positionRows.map((r) => r.id).filter(Boolean); + const deactivatedNames = new Set( + positionRows.filter((r) => !isRowActive(r)).map((r) => r.name).filter(Boolean), + ); + if (deactivatedNames.size > 0) { + grants.positions = grants.positions.filter((n) => !deactivatedNames.has(n)); + } + const positionIds = positionRows.filter((r) => isRowActive(r)).map((r) => r.id).filter(Boolean); if (positionIds.length > 0) { const rpsRows = await tryFind(ql, 'sys_position_permission_set', { position_id: { $in: positionIds } }, 500); for (const r of rpsRows) { @@ -382,7 +405,14 @@ export async function resolveUserAuthzGrants( // 6b. Resolve permission-set details (names → grants.permissions; system_permissions; // tab_permissions merged by highest visibility). if (psIds.size > 0) { - const psRows = await tryFind(ql, 'sys_permission_set', { id: { $in: Array.from(psIds) } }, 500); + const psRowsAll = await tryFind(ql, 'sys_permission_set', { id: { $in: Array.from(psIds) } }, 500); + // [ADR-0049] A DEACTIVATED permission set grants nothing — the + // `deactivate_permission_set` dialog's promise ("existing assignments stay + // in place but stop granting access"). Dropped BEFORE any derivation, the + // same discipline the validity window gets at §6, so `hasPlatformAdminGrant` + // cannot be derived from a set that no longer grants either: a deactivated + // `admin_full_access` must not keep conferring PLATFORM_ADMIN. + const psRows = psRowsAll.filter((r) => isRowActive(r)); const tabRank: Record = { hidden: 0, default_off: 1, default_on: 2, visible: 3 }; const mergedTabs: Record = {}; for (const ps of psRows) { diff --git a/packages/core/src/security/row-active.test.ts b/packages/core/src/security/row-active.test.ts new file mode 100644 index 0000000000..83118f95b3 --- /dev/null +++ b/packages/core/src/security/row-active.test.ts @@ -0,0 +1,48 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { isRowActive } from './row-active.js'; + +/** + * [#8613 / ADR-0049] The ONE predicate three readers share — the core resolver + * that enforces `active`, the plugin-security loader that judges the same flag + * on the name-reuse path, and the break-glass guard that SIMULATES a write to + * it. The reason it is one function is that a guard modelling "deactivated" + * differently from the resolver would permit exactly the write it exists to + * refuse. + * + * Both directions are pinned deliberately, because both have a real failure + * mode: reading absent as inactive mass-revokes deployed rows, and reading only + * `=== false` as inactive misses the shape SQLite actually stores. + */ +describe('[#8613] isRowActive — explicitly deactivated, not explicitly active', () => { + it('treats the stored spellings of OFF as deactivated', () => { + for (const off of [false, 0, '0', 'false']) { + expect(isRowActive({ active: off })).toBe(false); + } + }); + + it('treats the stored spellings of ON as active', () => { + for (const on of [true, 1, '1', 'true']) { + expect(isRowActive({ active: on })).toBe(true); + } + }); + + it('ABSENT means active — a row predating the column keeps granting', () => { + expect(isRowActive({})).toBe(true); + expect(isRowActive({ active: undefined })).toBe(true); + expect(isRowActive({ active: null })).toBe(true); + }); + + it('an unrecognised value is NOT a deactivation — junk may not revoke', () => { + // Nothing writes these. Inventing a revocation out of unreadable data is + // the direction that ends in a lockout nobody ordered. + expect(isRowActive({ active: 'yes' } as never)).toBe(true); + expect(isRowActive({ active: {} } as never)).toBe(true); + }); + + it('a missing row is not active — a filter over nothing grants nothing', () => { + expect(isRowActive(undefined)).toBe(false); + expect(isRowActive(null)).toBe(false); + }); +}); diff --git a/packages/core/src/security/row-active.ts b/packages/core/src/security/row-active.ts new file mode 100644 index 0000000000..23a8a33e7a --- /dev/null +++ b/packages/core/src/security/row-active.ts @@ -0,0 +1,69 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The `active` flag on the RBAC grant catalogues — `sys_permission_set` and + * `sys_position` (ADR-0049 enforce-or-remove). + * + * Both objects ship a Deactivate action whose confirmation dialog promises, in + * four locales, that access stops: + * + * > Deactivate this permission set? Existing assignments stay in place but stop + * > granting access until re-activated. + * > Deactivate this position? Users keep their assignment but the position stops + * > granting permissions until re-activated. + * + * Nothing read the column, so the promise was false: deactivation changed a + * badge in Setup and the assignments kept granting. Correctness lives at + * RESOLUTION time — the same placement ADR-0091 chose for validity windows, + * and for the same reason: a catalogue flag that is only honoured by a cleanup + * job is an unenforced security property. + * + * ## Why the predicate is "explicitly deactivated", not "explicitly active" + * + * Absent means ACTIVE. Only a stored value that really reads false takes the + * grant away. Two measured reasons, and they point the same way: + * + * 1. **Deployed data.** `active` carries `defaultValue: true`, but a row that + * predates the column, arrived through a migration, or was projected with a + * `fields` list that omitted it carries no value at all. Requiring `true` + * would revoke every such row's grants the moment this lands — a silent + * mass revocation nobody asked for, which is the opposite of the one thing + * the Deactivate dialog promises. `isGrantActive` reads absent bounds as + * unbounded for exactly this reason (ADR-0091 D2). + * 2. **Storage shapes.** SQLite stores booleans as 1/0 and presents them back + * as numbers unless the driver knows the column is boolean; the memory + * driver round-trips real booleans; a JSON/text column can hand back + * `'false'`. `row.active === false` alone therefore misses the deactivated + * row on the primary driver — an enforcement hole shaped exactly like the + * bug this predicate closes. Pushing `active: true` into a driver `where` + * has the same problem from the other side AND drops the NULL rows of (1), + * so the filter runs in memory over rows the resolver already fetched: + * zero new queries, identical answer on every driver. + * + * The false-set below is closed on purpose. An unrecognised value (a stray + * string, an object) is NOT a deactivation — nothing writes one, and inventing + * a revocation out of junk data is the failing-open-into-a-lockout direction. + */ + +/** The stored spellings of a deactivated row, across every driver in the tree. */ +const DEACTIVATED_VALUES: readonly unknown[] = [false, 0, '0', 'false']; + +/** A catalogue row that may carry the `active` flag (`sys_permission_set`, `sys_position`). */ +export interface ActivatableRow { + active?: unknown; +} + +/** + * True unless the row carries an `active` column that is explicitly OFF. + * + * The ONE predicate every reader of `sys_permission_set.active` / + * `sys_position.active` uses, so the resolver that enforces the flag and the + * break-glass guard that simulates a write to it can never disagree about what + * "deactivated" means. + */ +export function isRowActive(row: ActivatableRow | null | undefined): boolean { + if (!row) return false; + const value = (row as { active?: unknown }).active; + if (value === undefined || value === null) return true; + return !DEACTIVATED_VALUES.includes(value); +} diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts index 68454def79..804d51fca9 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts @@ -106,6 +106,12 @@ const sysPermissionSet = { // (`permissionSetRowFields`), so it is the column the "costs no reads" // pin drives. label: { name: 'label', type: 'text' as const }, + // [#8613 / ADR-0049] The Deactivate switch. Declared `boolean`, so on this + // real sqlite database it stores as 0/1 and the guard's flag handling is + // exercised against the shape the primary driver actually returns — not + // against a hand-written `false`. Seeded rows leave it NULL, which is the + // deployed shape of a row that predates the column: absent means ACTIVE. + active: { name: 'active', type: 'boolean' as const }, }, }; @@ -1997,3 +2003,255 @@ describe('[#6084] reverse verification: one unguarded write takes the admins AND expect(await bannedFlag(engine, 'usr_platform')).toBeTruthy(); }); }); + +// --------------------------------------------------------------------------- +// [#8613 / ADR-0049] Write shape (4), third spelling: DEACTIVATING the row +// +// `sys_permission_set.active` used to be inert — a badge in Setup and nothing +// else — so the #6084 standing-key list could exclude it in writing, and did. +// Enforcing the flag at the resolution seam makes `active: false` on +// `admin_full_access` un-make every platform admin at once, by a payload that +// touches neither `name` nor any identity table, through a row action that +// carries no visibility or condition guard. Unguarded, that is one click and an +// installation-wide lockout with no path back: the seeders deliberately never +// reconcile `active`, and re-activating needs the permission just lost. +// +// Deactivation also leaves NO dangling grant, so the #6084 bootstrap predicate +// cannot see it — the set row is still there, still correctly named. Hence the +// second half of this block: the same emptiness, its own evidence, its own +// remedy, and an exemption for the write that IS the remedy. +// --------------------------------------------------------------------------- + +describe('[#8613] path 4, third spelling — deactivating the admin_full_access permission set', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = await boot(); + await seedAdminPermissionSet(engine); + }); + + /** Exactly what the `deactivate_permission_set` row action PATCHes. */ + const deactivate = (id: string) => + engine.update('sys_permission_set', { id, active: false }, SYSTEM); + + /** + * Whether the STORED row reads as deactivated, in whichever shape sqlite + * hands back — `0`, `false`, or the NULL a never-written column keeps. The + * predicate under test treats absent as ACTIVE, so this asserts the write did + * not land rather than asserting one particular spelling of "off". + */ + const isDeactivated = async (id: string): Promise => { + const row = await engine.findOne('sys_permission_set', { where: { id } }, SYSTEM); + const flag = row?.active as unknown; + return flag === 0 || flag === false || flag === '0'; + }; + + it('THE ONE-CLICK LOCKOUT: deactivating it is refused and the row stays active', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect(deactivate(PS_ADMIN)).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + object: 'sys_permission_set', + }); + // Nothing was written — the row is still there and still grants. + expect(await isDeactivated(PS_ADMIN)).toBe(false); + expect(await rowExists(engine, 'sys_permission_set', PS_ADMIN)).toBe(true); + }); + + it('no identity table is touched, exactly as in the delete and rename spellings', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true, accountProvider: 'oidc' }); + + await expect(deactivate(PS_ADMIN)).rejects.toThrow(/ADR-0024 D5\.2/); + await expectUserRowUntouched(engine, 'usr_platform'); + expect(await rowExists(engine, 'sys_user_permission_set', 'ups_usr_platform')).toBe(true); + }); + + it('the refusal names the user who would lose standing', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect(deactivate(PS_ADMIN)).rejects.toThrow(/'usr_platform'/); + await expect(deactivate(PS_ADMIN)).rejects.toThrow(/last administrator/i); + }); + + it('RE-activating is never refused — the payload is simulated, not pattern-matched', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + // Same column, same standing-key hit, opposite direction: the simulation + // finds the administrator still there afterwards, so it proceeds. + await expect( + engine.update('sys_permission_set', { id: PS_ADMIN, active: true }, SYSTEM), + ).resolves.toBeTruthy(); + }); + + it('an org admin elsewhere keeps the deactivation legal — the invariant is the ENVIRONMENT\'s', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + await expect(deactivate(PS_ADMIN)).resolves.toBeTruthy(); + }); + + it('deactivating ANOTHER permission set is unaffected', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect(deactivate('ps_member')).resolves.toBeTruthy(); + expect(await isDeactivated('ps_member')).toBe(true); + expect(await isDeactivated(PS_ADMIN)).toBe(false); + }); + + it('a predicate write that sweeps every set at once is refused', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect( + engine.update( + 'sys_permission_set', + { active: false }, + { multi: true, where: { id: { $in: [PS_ADMIN, 'ps_member'] } }, ...SYSTEM }, + ), + ).rejects.toThrow(/last administrator/i); + }); + + it('a payload that touches NEITHER `name` nor `active` still costs no reads at all', async () => { + const quiet = await boot({ + readThrough: (real) => ({ + registerHook: (event, handler, options) => real.registerHook(event, handler, options), + find: async () => { + throw new Error('the guard must not read anything for a row-state-free payload'); + }, + }), + }); + await seedAdminPermissionSet(quiet); + await seedUser(quiet, 'usr_platform', { platformAdmin: true }); + + // Adding `active` to the standing keys must not walk back the #6084 + // read-freeness: the projection is facets-only and never re-flips the + // switch, so every projection pass and `os meta resync` still skips this + // guard statically. Any read would surface as a refusal. + await expect( + quiet.update('sys_permission_set', { id: PS_ADMIN, label: 'Full Access (edited)' }, SYSTEM), + ).resolves.toBeTruthy(); + }); +}); + +describe('[#8613] a DEACTIVATED break-glass set is an emptied environment, not a fresh one', () => { + /** + * The state the third spelling leaves behind, reachable the same way #6084's + * is: the deactivation lands while the guard is not registered (a pre-#8613 + * deployment that clicked Deactivate while the flag was inert, a migration, a + * direct database edit), and the platform then boots with the guard on. + * + * This is the population the behaviour flip lands hardest on, which is why it + * is pinned rather than reasoned about: on those installations `active:false` + * was a no-op until this change, so the row can already be off on upgrade. + */ + async function deactivatedEnvironment(): Promise { + const engine = await boot({ unguarded: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_platform', { platformAdmin: true, accountProvider: 'oidc' }); + await engine.update('sys_permission_set', { id: PS_ADMIN, active: false }, SYSTEM); + registerLastAdminGuard(engine as unknown as LastAdminGuardEngine, { + packageId: 'test.last-admin-guard', + }); + return engine; + } + + it('THE AMPLIFIER PIN: the ban the bootstrap exemption would have waved through is refused', async () => { + const engine = await deactivatedEnvironment(); + + await expect(ban(engine, 'usr_platform')).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + }); + expect(await bannedFlag(engine, 'usr_platform')).toBeFalsy(); + }); + + it('…and the user delete and the grant revoke with it — all three halves stay on', async () => { + const engine = await deactivatedEnvironment(); + + await expect(removeUser(engine, 'usr_platform')).rejects.toThrow(/DEACTIVATED/); + await expect( + engine.delete('sys_user_permission_set', { where: { id: 'ups_usr_platform' }, ...SYSTEM }), + ).rejects.toThrow(/DEACTIVATED/); + expect(await userExists(engine, 'usr_platform')).toBe(true); + }); + + it('the refusal names the evidence, the cause and the way back', async () => { + const engine = await deactivatedEnvironment(); + + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/recognises NO administrator/); + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/DEACTIVATED/); + // The remedy is the one that actually works here — re-activate, NOT the + // "restore the deleted row" sentence the #6084 wipe prescribes. + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/Re-activate/); + await expect(ban(engine, 'usr_platform')).rejects.toThrow(new RegExp(ADMIN_FULL_ACCESS)); + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/ADR-0024 D5\.2/); + }); + + it('THE WAY BACK IS OPEN: re-activating the set is permitted from inside that environment', async () => { + const engine = await deactivatedEnvironment(); + + // Every other guarded write is refused above. If the remedy the refusal + // prescribes were refused too, the guard itself would be the lockout. + await expect( + engine.update('sys_permission_set', { id: PS_ADMIN, active: true }, SYSTEM), + ).resolves.toBeTruthy(); + // …and the environment is whole again: the ordinary verdict is back. + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/last administrator/i); + }); + + it('a deactivated set nobody holds an unscoped grant to is NOT evidence', async () => { + const engine = await boot(); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_a', { grant: { permission_set_id: 'ps_member' } }); + // No unscoped grant points at `admin_full_access`, so switching it off + // strands nobody — this is an ordinary pre-first-admin environment. + await engine.update('sys_permission_set', { id: PS_ADMIN, active: false }, SYSTEM); + + await expect(ban(engine, 'usr_a')).resolves.toBeTruthy(); + }); + + it('a genuinely fresh environment is untouched — no set is deactivated at all', async () => { + const engine = await boot(); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_first', { role: 'member' }); + + await expect(ban(engine, 'usr_first')).resolves.toBeTruthy(); + }); +}); + +// --------------------------------------------------------------------------- +// [#8613] Reverse verification — the same fixtures with the guard NOT registered +// +// Direction, decided before running: RED, the usual one. Without +// `registerLastAdminGuard` the deactivation succeeds, every platform admin +// evaporates while every row survives untouched, and the ban that follows +// succeeds too. The guarded halves above are measured against exactly this. +// --------------------------------------------------------------------------- + +describe('[#8613] reverse verification: unguarded, one click takes the admins AND the guard', () => { + it('the unguarded engine deactivates the row and leaves every other row intact', async () => { + const engine = await boot({ unguarded: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_platform', { platformAdmin: true, accountProvider: 'oidc' }); + + await expect( + engine.update('sys_permission_set', { id: PS_ADMIN, active: false }, SYSTEM), + ).resolves.toBeTruthy(); + expect(await userExists(engine, 'usr_platform')).toBe(true); + expect(await bannedFlag(engine, 'usr_platform')).toBeFalsy(); + expect(await rowExists(engine, 'sys_user_permission_set', 'ups_usr_platform')).toBe(true); + // The row is STILL THERE and still correctly named — which is exactly why + // the #6084 dangling-grant predicate cannot see this state. + expect(await rowExists(engine, 'sys_permission_set', PS_ADMIN)).toBe(true); + }); + + it('THE AMPLIFICATION: on that same engine the ban of the last administrator then succeeds', async () => { + const engine = await boot({ unguarded: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await engine.update('sys_permission_set', { id: PS_ADMIN, active: false }, SYSTEM); + await expect(ban(engine, 'usr_platform')).resolves.toBeTruthy(); + expect(await bannedFlag(engine, 'usr_platform')).toBeTruthy(); + }); +}); diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.ts b/packages/plugins/plugin-auth/src/last-admin-guard.ts index 9858e5d397..28a1635ffc 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.ts @@ -26,18 +26,21 @@ * ADR-0091 validity window). The end state is identical to (2): everyone is * still there, nobody can administer anything, and there is no recovery * path from inside the product. - * 4. **deleting — or RENAMING — the `admin_full_access` `sys_permission_set` - * row** (#6084) — the one table the enumeration reads that is not itself an - * identity table. "Who is a platform admin" is resolved by NAME: the first - * step of `resolveAdminUserIds` looks the permission set up as - * `where: { name: 'admin_full_access' }` and only then reads the grants - * pointing at its id. Remove that row, or call it something else, and every - * grant, every `sys_user` row and every `sys_member` row survives untouched - * while nobody is a platform admin any more — one write, the whole - * platform-admin population. Unlike (3) this one is not driven by an IdP at - * all: it is written by a metadata delete, an `os meta` run or a package - * uninstall, which is why it needs its own two hooks rather than a wider - * filter on the three tables above. + * 4. **deleting, RENAMING — or DEACTIVATING — the `admin_full_access` + * `sys_permission_set` row** (#6084) — the one table the enumeration reads + * that is not itself an identity table. "Who is a platform admin" is + * resolved by NAME: the first step of `resolveAdminUserIds` looks the + * permission set up as `where: { name: 'admin_full_access' }` and only then + * reads the grants pointing at its id. Remove that row, call it something + * else, or (ADR-0049, since `active` became a resolution-time predicate) + * switch it off, and every grant, every `sys_user` row and every + * `sys_member` row survives untouched while nobody is a platform admin any + * more — one write, the whole platform-admin population. Unlike (3) this one + * is not driven by an IdP at all: it is written by a metadata delete, an + * `os meta` run, a package uninstall — or, for the deactivation spelling, by + * one click on a Setup row action that carries no visibility or condition + * guard — which is why it needs its own two hooks rather than a wider filter + * on the three tables above. * * In the case that matters both are driven by an EXTERNAL system: nobody reads * the payload before it commits, so one mis-scoped IdP group or one over-broad @@ -146,6 +149,15 @@ * - **an environment that was EMPTIED** — an unscoped, in-window * `sys_user_permission_set` grant still points at a `sys_permission_set` row * that no longer exists. Refused, loudly, naming the dangling grants. + * - **an environment whose break-glass set was switched OFF** (ADR-0049) — the + * `admin_full_access` row is present and named correctly, and unscoped, + * in-window grants still point at it, but `active` is false so it confers + * nothing. Refused too, with its own remedy: re-activate the row. This state + * became reachable the moment `active` became a resolution-time predicate, + * and it produces the identical emptiness while leaving no dangling grant to + * read. A write that RESTORES standing — re-activation itself — is exempt, + * measured through the same simulation, or the refusal would have no way out + * from inside the product. * * The evidence is chosen so the FRESH-INSTALL answer cannot change: a dangling * grant is unreachable on the happy path in either direction. Every writer @@ -261,7 +273,7 @@ import { MEMBERSHIP_ROLE_OWNER, } from '@objectstack/spec/identity'; import { SystemObjectName, SystemUserId } from '@objectstack/spec/system'; -import { isGrantActive } from '@objectstack/core'; +import { isGrantActive, isRowActive } from '@objectstack/core'; import { isOrgAdminGrade } from './invitation-role-cap.js'; @@ -556,22 +568,47 @@ const GRANT_STANDING_KEYS = [ ] as const; /** - * [#6084] Same, for `sys_permission_set` — and it is a one-element list, - * because the platform-admin half of the enumeration reads exactly one column - * of that table: the `name` it looks the set up by. Everything else a - * permission-set write touches (`label`, `description`, the four permission - * JSON blobs, `active`, provenance) is invisible to "who is an administrator" — - * `resolveAuthzContext` derives `platform_admin` from the NAME, not from the - * capabilities the set carries — so those writes, which is every projection - * pass and every Setup edit, cost this guard no reads at all. + * [#6084] Same, for `sys_permission_set` — the two columns of that table the + * platform-admin half of the enumeration reads: + * + * - `name`, the column it looks the set up by. Renaming the row takes the + * standing away from everyone holding a grant to it, in one write. + * - `active` [ADR-0049]. This column USED to be inert, and this comment used + * to say so: `resolveAuthzContext` derived `platform_admin` from the name + * alone and read no flag. It now drops a DEACTIVATED set before any + * derivation, so `active: false` on `admin_full_access` un-makes every + * platform admin at once — the same end state as renaming or deleting the + * row, reached by a payload that touches neither. Enforcing the flag without + * listing it here would have left exactly one unguarded route to an + * installation-wide lockout: the action is offered on every row with no + * visibility or condition guard, the seeders deliberately never reconcile + * `active`, and re-activating requires the permission the click just took + * away. + * + * Everything else a permission-set write touches (`label`, `description`, the + * four permission JSON blobs, provenance) is still invisible to "who is an + * administrator" — `resolveAuthzContext` derives `platform_admin` from the NAME + * of an ACTIVE set, never from the capabilities it carries — so those writes + * still cost this guard no reads at all. Adding `active` does not walk that + * back: the projection is FACETS ONLY and deliberately never re-flips a + * record's on/off switch (`permission-set-projection.ts`, #4669), so every + * projection pass, every `os meta resync` and every ordinary Setup edit still + * misses this list entirely. What now pays for an enumeration is the write that + * actually toggles the switch — which is the write this list exists to judge. * * `id` is deliberately NOT here even though the enumeration reads it. On this * engine `data.id` on an update ADDRESSES the row (it is what * `resolveTargetIds` resolves the target from) rather than proposing a new * primary key, so a key rewrite is not expressible through this write path; the * two standing key lists above exclude `id` for the same reason. + * + * `sys_position` gets no analogous list because it has no route into this + * enumeration to guard: platform-admin standing is read from UNSCOPED + * `sys_user_permission_set` grants only (a position-bound `admin_full_access` + * never conferred it, in the resolver or here), and org-administrator standing + * is read from `sys_member.role`. Deactivating a position cannot empty either. */ -const PERMISSION_SET_STANDING_KEYS = ['name'] as const; +const PERMISSION_SET_STANDING_KEYS = ['name', 'active'] as const; function touchesAny(data: Record, keys: readonly string[]): boolean { return keys.some((k) => k in data); @@ -633,19 +670,28 @@ export function registerLastAdminGuard( // // [#6084] The set row is simulated exactly like the grant rows below it: a // pending write on `sys_permission_set` can DELETE this row (it drops out of - // `adminSetIds`, and with it every grant that pointed at it) or RENAME it, - // and the name is RE-TESTED for the same reason the grant's + // `adminSetIds`, and with it every grant that pointed at it), RENAME it, or + // DEACTIVATE it, and each is RE-TESTED for the same reason the grant's // `permission_set_id` is — the scan's own `where` only proved what the row // was called BEFORE the write. + // + // [ADR-0049] `active` rides the projection because the flag is now read at + // resolution time: `fields` is a projection, so a column left out of this + // list would read as absent here and absent means ACTIVE — the guard would + // model an environment in which no set is ever deactivated and permit the + // one write that empties it. const sets = await scan(op, SystemObjectName.PERMISSION_SET, { where: { name: ADMIN_FULL_ACCESS }, - fields: ['id', 'name'], + fields: ['id', 'name', 'active'], }); const adminSetIds: string[] = []; for (const rawSet of sets) { const set = applyPending(rawSet, pending, SystemObjectName.PERMISSION_SET); if (!set) continue; // removed outright by the pending write if (set.name !== ADMIN_FULL_ACCESS) continue; // renamed away — the row survives, the meaning does not + // Deactivated — the row survives under its own name and grants nothing, + // judged by the SAME predicate `resolveAuthzContext` resolves with. + if (!isRowActive(set)) continue; const sid = toId(set.id); if (sid) adminSetIds.push(sid); } @@ -741,13 +787,56 @@ export function registerLastAdminGuard( * guard cannot tell the two apart (the name it would compare went away with * the row), and refusing in an environment that has zero administrators AND a * grant pointing into nowhere is the fail-closed direction. + * + * [ADR-0049] DEACTIVATION is the second way to reach the same emptiness, and + * it leaves NO dangling grant to read — the row is still there, still named + * `admin_full_access`, and simply grants nothing. Left unhandled, enforcing + * the flag would turn this exemption into the amplifier the paragraph above + * exists to prevent: one deactivation empties the administrator population + * and every later ban, delete and downgrade sails through unguarded. So the + * deactivated row with unscoped, in-window grants still pointing at it is + * read as the SAME evidence, with its own remedy — re-activate it. */ const refuseIfEmptiedRatherThanFresh = async (op: GuardedOp): Promise => { - const sets = await scan(op, SystemObjectName.PERMISSION_SET, { fields: ['id'] }); + const sets = await scan(op, SystemObjectName.PERMISSION_SET, { fields: ['id', 'name', 'active'] }); const known = new Set(); + const deactivatedAdminSetIds: string[] = []; for (const row of sets) { const sid = toId(row.id); - if (sid) known.add(sid); + if (!sid) continue; + known.add(sid); + if (row.name === ADMIN_FULL_ACCESS && !isRowActive(row)) deactivatedAdminSetIds.push(sid); + } + + // The deactivated-break-glass case, checked before the dangling one: it has + // a precise diagnosis and a one-click remedy, so it must not be reported as + // the vaguer "something was deleted" story. + if (deactivatedAdminSetIds.length > 0) { + const held = await scan(op, USER_PERMISSION_SET, { + where: { permission_set_id: { $in: deactivatedAdminSetIds } }, + }); + const nowMs = Date.now(); + const stranded = held.filter( + (link) => !(link.organization_id ?? link.organizationId) && isGrantActive(link, nowMs), + ); + if (stranded.length > 0) { + const words = OP_WORDS[op]; + logger?.warn( + `[LastAdminGuard] refused a ${words.noun} in an environment whose '${ADMIN_FULL_ACCESS}' ` + + `permission set is DEACTIVATED — ${stranded.length} unscoped grant(s) confer nothing`, + ); + throw refuse( + `Refusing this ${words.noun}: this environment recognises NO administrator because its ` + + `'${ADMIN_FULL_ACCESS}' '${SystemObjectName.PERMISSION_SET}' row is DEACTIVATED — ` + + `${stranded.length} unscoped, in-window '${USER_PERMISSION_SET}' grant(s) still point ` + + 'at it and confer nothing while it is off. That is not the bootstrap window, and ' + + 'reading the resulting emptiness as "no administrator to protect" would switch this ' + + `guard off for every other write too (${BREAK_GLASS_CITATION}). Re-activate the ` + + `'${ADMIN_FULL_ACCESS}' permission set (set 'active' back to true) — the grants naming ` + + 'it are still there — before writing the identity tables again.', + words.table, + ); + } } // With no permission set at all every grant is dangling, and `$nin: []` is // not a predicate every driver agrees on — so that case reads unfiltered @@ -984,6 +1073,23 @@ export function registerLastAdminGuard( // break-glass account to protect — unless the environment got there by // being emptied rather than by being new. if (before.size === 0) { + // [ADR-0049] …and unless THIS write is the way back. A write that + // RESTORES standing can never be the write that takes the last one + // away, and re-activating a deactivated `admin_full_access` row is + // exactly the remedy the refusal below prescribes — judged by the same + // simulation as everything else, so the exemption is a measurement and + // not a special case for one column. Without it the refusal would be + // unrecoverable from inside the product: the only fix is an update, and + // every update would be refused. + const restoringIds = await resolveTargetIds(op, table, input?.id, input?.options, input?.data); + if (restoringIds.size > 0) { + const restored = await resolveAdminUserIds(op, { + table, + ids: restoringIds, + ...(patch ? { patch } : {}), + }); + if (restored.size > 0) return; + } await refuseIfEmptiedRatherThanFresh(op); return; } diff --git a/packages/plugins/plugin-security/src/permission-set-active.test.ts b/packages/plugins/plugin-security/src/permission-set-active.test.ts new file mode 100644 index 0000000000..123edf77fb --- /dev/null +++ b/packages/plugins/plugin-security/src/permission-set-active.test.ts @@ -0,0 +1,150 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8613 / ADR-0049] `sys_permission_set.active`, at the DB loader. + * + * The dominant enforcement point for this flag is `resolveAuthzContext` in + * `@objectstack/core`, which drops a deactivated set before `context.permissions` + * is built. This loader is the SECOND reader, and it is not defence in depth + * that nothing reaches — the reachability case below is the reason it exists: + * + * `resolvePermissionSetsForContext` requests `context.positions` as + * permission-set NAMES too (a position name is commonly reused as a set name, + * which the evaluator's own doc comment calls out). So an ACTIVE position + * whose name matches a DEACTIVATED `sys_permission_set` row arrives here with + * that name still standing — core filtered the position catalogue and the + * sets reached by id, and neither of those judged THIS row. + * + * The filter runs in memory over rows the loader already fetched rather than as + * an `active: true` `where` predicate: `true` would also drop rows whose column + * is NULL (rows predating the field — a silent mass revocation on deployed + * data), and boolean `where` coercion differs per driver. `isRowActive` is the + * same predicate core and the break-glass guard use. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { SecurityPlugin } from './security-plugin.js'; +import type { PermissionSet } from '@objectstack/spec/security'; +import type { ISecurityService } from '@objectstack/spec/contracts'; + +/** The metadata-declared baseline every authenticated caller resolves additively. */ +const MEMBER_DEFAULT: PermissionSet = { + name: 'member_default', + label: 'Member', + objects: { deal: { allowRead: true } }, + fields: {}, + systemPermissions: [], +} as any; + +/** A DB-authored set, as it sits in `sys_permission_set`. `active` varies per case. */ +const salesManagerRow = (active: unknown) => ({ + name: 'sales_manager', + label: 'Sales Manager', + object_permissions: JSON.stringify({ deal: { allowRead: true, allowEdit: true } }), + system_permissions: JSON.stringify(['setup.access']), + ...(active === undefined ? {} : { active }), +}); + +/** + * Resolve the security service the way a cross-package consumer does. The fake + * engine matches on `where.name.$in` ONLY — deliberately, because that is the + * predicate the loader has always sent. A filter expressed as an extra `where` + * key would be invisible to this fixture (and to any driver that ignores an + * unknown column), so the assertion below measures the product's own judgment. + */ +async function locateSecurityService( + dbRows: Array>, +): Promise> { + const schema: any = { + name: 'deal', + label: 'Deal', + systemFields: false, + fields: { id: { name: 'id' }, amount: { name: 'amount' } }, + }; + const ql: any = { + registerMiddleware: () => {}, + getSchema: (name: string) => (name === 'deal' ? schema : null), + findOne: async () => null, + find: async (object: string, query: any) => { + if (object !== 'sys_permission_set') return []; + const wanted: string[] = query?.where?.name?.$in ?? []; + return dbRows.filter((r) => wanted.includes(String(r.name))); + }, + }; + const metadata: any = { + get: async (_type: string, name: string) => (name === 'deal' ? schema : null), + list: async () => [MEMBER_DEFAULT], + }; + const services: Record = { manifest: { register: vi.fn() }, objectql: ql, metadata }; + const ctx: any = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + registerService: vi.fn(), + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' } as any); + await plugin.init(ctx); + await plugin.start(ctx); + return ctx.registerService.mock.calls.find((c: any[]) => c[0] === 'security')?.[1]; +} + +const namesFor = async ( + rows: Array>, + context: Record, +): Promise => { + const svc = await locateSecurityService(rows); + const sets = await svc.resolvePermissionSetsForContext?.({ userId: 'u1', ...context } as any); + return (sets ?? []).map((s) => s.name); +}; + +describe('[#8613] the DB loader drops DEACTIVATED permission sets', () => { + it('a deactivated set requested by name resolves to nothing', async () => { + const names = await namesFor([salesManagerRow(false)], { permissions: ['sales_manager'] }); + expect(names).not.toContain('sales_manager'); + // The baseline still applies — deactivating one set is not a blanket denial. + expect(names).toContain('member_default'); + }); + + it('an ACTIVE set still resolves, whole', async () => { + const svc = await locateSecurityService([salesManagerRow(true)]); + const sets = await svc.resolvePermissionSetsForContext?.({ + userId: 'u1', + permissions: ['sales_manager'], + } as any); + const found: any = (sets ?? []).find((s) => s.name === 'sales_manager'); + expect(found).toBeDefined(); + expect(found.objects).toEqual({ deal: { allowRead: true, allowEdit: true } }); + expect(found.systemPermissions).toEqual(['setup.access']); + }); + + it('a row with NO `active` column still resolves — deployed rows are not mass-revoked', async () => { + const names = await namesFor([salesManagerRow(undefined)], { permissions: ['sales_manager'] }); + expect(names).toContain('sales_manager'); + }); + + it('the 0/1 storage shape is judged too, not only a literal `false`', async () => { + expect(await namesFor([salesManagerRow(0)], { permissions: ['sales_manager'] })).not.toContain( + 'sales_manager', + ); + expect(await namesFor([salesManagerRow(1)], { permissions: ['sales_manager'] })).toContain( + 'sales_manager', + ); + }); + + it('THE REACHABILITY CASE: a POSITION name reaching a deactivated set of the same name', async () => { + // Core cannot judge this row: it filtered the `sys_position` catalogue and + // the sets it reached by id, and this name arrives as a POSITION the caller + // legitimately holds. Without this loader's filter the deactivated set + // would grant `deal` edit rights through the name-reuse path — the wall + // that looks enforced and is not. + const names = await namesFor([salesManagerRow(false)], { positions: ['sales_manager'] }); + expect(names).not.toContain('sales_manager'); + }); + + it('…and the same request with the set ACTIVE does resolve — the case is real, not vacuous', async () => { + const names = await namesFor([salesManagerRow(true)], { positions: ['sales_manager'] }); + expect(names).toContain('sales_manager'); + }); +}); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index cf93196aa4..3fff9b173e 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -1,6 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import { Plugin, PluginContext, POSTURE_LADDER } from '@objectstack/core'; +import { Plugin, PluginContext, POSTURE_LADDER, isRowActive } from '@objectstack/core'; import type { PermissionSet, RowLevelSecurityPolicy } from '@objectstack/spec/security'; import { describeHighPrivilegeBits, describeAnchorForbiddenBits, PUBLIC_FORM_SERVER_MANAGED_FIELDS } from '@objectstack/spec/security'; import { MCP_AGENT_PERMISSION_SET_RESTRICTED } from '@objectstack/spec/ai'; @@ -826,7 +826,19 @@ export class SecurityPlugin implements Plugin { } catch { rows = []; } - const list = Array.isArray(rows) ? rows : rows?.records ?? []; + const all = Array.isArray(rows) ? rows : rows?.records ?? []; + // [ADR-0049] A DEACTIVATED set grants nothing. Not defence in depth + // that nothing reaches: `resolvePermissionSetsForContext` requests + // `context.positions` as permission-set NAMES too (a position name is + // commonly reused as a set name), so an ACTIVE position whose name + // matches a DEACTIVATED `sys_permission_set` row arrives here with + // that name still standing — this loader is the only place that read + // is judged. Filtered in memory rather than by a `where` predicate: + // `active: true` would also drop rows whose column is NULL (rows + // predating the field), and boolean `where` coercion differs per + // driver. `isRowActive` is the same predicate the core resolver and + // the break-glass guard use, so the three cannot drift. + const list = all.filter((r: any) => isRowActive(r)); const parseJson = (v: any, fallback: any) => { if (typeof v !== 'string') return v ?? fallback; try { return JSON.parse(v || JSON.stringify(fallback)); } catch { return fallback; }