From abc546664bf54e159e6d99a6bb246dd19e8efaa2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 07:15:27 +0000 Subject: [PATCH 1/2] wip: thread refusal reporting into drift + overlay-discard --- .../src/per-organization-catalog.ts | 12 +- .../src/permission-set-drift.ts | 113 ++++++++++++++++-- .../src/permission-set-overlay-discard.ts | 60 +++++++++- 3 files changed, 173 insertions(+), 12 deletions(-) diff --git a/packages/plugins/plugin-security/src/per-organization-catalog.ts b/packages/plugins/plugin-security/src/per-organization-catalog.ts index 16868ccfc3..43c591cab1 100644 --- a/packages/plugins/plugin-security/src/per-organization-catalog.ts +++ b/packages/plugins/plugin-security/src/per-organization-catalog.ts @@ -145,8 +145,18 @@ export type SeedLogger = { * * The `?.` on `warn` is the backstop for hosts the TYPE cannot reach (a * plain-JS embedder, or a cast), not doubt about the declaration. + * + * EXPORTED rather than module-private, for the reason its own doc gives — the + * fallback "lives in {@link logSeedDurabilityFailure} so no site can forget + * it". Two sites outside the catalog seed now report a refused write and owe + * the identical fallback: `permission-set-drift.ts` (a refused drift-diagnostic + * write) and `permission-set-overlay-discard.ts` (a refused resync after a + * sanctioned overlay discard). They reuse this spelling and NOT + * {@link reportSeedWriteRefusals}, whose PROSE is catalog-seed-specific — see + * the deviation note in each of those call sites. Deliberately absent from the + * package's `index.ts`: this is an intra-package helper, not public API. */ -function logSeedDurabilityFailure( +export function logSeedDurabilityFailure( logger: SeedLogger | undefined, message: string, meta?: Record, diff --git a/packages/plugins/plugin-security/src/permission-set-drift.ts b/packages/plugins/plugin-security/src/permission-set-drift.ts index c844cc519a..63a6c9db0b 100644 --- a/packages/plugins/plugin-security/src/permission-set-drift.ts +++ b/packages/plugins/plugin-security/src/permission-set-drift.ts @@ -62,6 +62,10 @@ import { tryUpdate, type ProjectionLogger, } from './permission-set-projection.js'; +import { + createSeedWriteRefusals, + logSeedDurabilityFailure, +} from './per-organization-catalog.js'; import { readDeclared } from './bootstrap-declared-permissions.js'; import { buildExistingByName } from './seed-name-lookup.js'; @@ -189,6 +193,75 @@ export async function computePermissionSetDriftDiagnostics( return out; } +/** + * Report, ONCE per pass, the drift-diagnostic writes the store refused. + * + * ## Why this is not {@link reportSeedWriteRefusals} + * + * The shared ACCUMULATOR (`createSeedWriteRefusals`) is reused as-is — it + * carries the shipped `isUniqueViolationError` classification and the + * value-free `code`/`errno` channel, and re-deriving either here is exactly + * the local-regex defect that module was written to retire. The shared + * REPORTER is not, and deliberately: every sentence it prints is about + * seeding the RBAC catalog — "the catalog is INCOMPLETE", "this pass's + * 'seeded' count", and a remedy naming the legacy PLATFORM-WIDE unique index + * on the catalog name column. None of that is true here. This pass seeds + * nothing; it writes two diagnostic columns onto rows that already exist, by + * id. Printing that text over this failure would send an operator to + * `os migrate` for a defect that is not there — the same "a confident wrong + * answer is worse than no answer" reasoning that makes `other` its own class + * in `reportSeedWriteRefusals` rather than a relabelled unique violation. + * + * ## Why ONE line and not one per class + * + * `reportSeedWriteRefusals` splits its classes because their REMEDIES differ. + * Here they do not: an update-by-id of two nullable diagnostic columns has no + * unique constraint to violate, so the class is a fact for `meta` (it travels + * there, per (object, class), with the driver codes) and not a reason to print + * a second sentence. + * + * ## Why the durability channel + * + * AGENTS.md "Degradation log levels", one question — after the degradation, + * does the system still look normal while something it claims is persisted + * has not landed? Yes, precisely: `drift_status` / `drift_detail` are what + * Setup's "Needs Attention" surface reads, so a refused write leaves that + * screen showing a CLEAN environment over sets that are still drifted. + * ⚠️ `check:durability-log-level` does not vouch for this choice — `ql.update` + * is not in its `DURABILITY_CRITICAL_CALLEES` vocabulary, so its green here + * means the site is outside the gate's reach (NOT MEASURED), never approval. + */ +function reportDriftWriteRefusals( + logger: ProjectionLogger | undefined, + refusals: ReturnType, + organizationId?: string, +): void { + const entries = refusals.report(); + if (entries.length === 0) return; + logSeedDurabilityFailure( + logger, + `[security] ${refusals.total} package-declared permission set drift diagnostic(s) were REFUSED by the ` + + `store — the "declared ≠ enforced" verdict this pass computed did NOT persist, so those rows keep ` + + `their PREVIOUS drift_status/drift_detail (usually none at all), Setup's "Needs Attention" surface goes ` + + `on showing them as CLEAN, and THE DEPLOYMENT WILL GO ON LOOKING HEALTHY while those sets keep ` + + `enforcing grants that differ from the shipped artifact. The drifted set NAMES are not lost with the ` + + `write: the "[security] package-declared permission set(s) enforcing grants that differ from the ` + + `shipped artifact" line is logged alongside this one and names every one of them — it is no longer ` + + `gated behind the count of writes that landed. What the store actually said is in the query engine's ` + + `"Update operation failed" entries logged just before this one, which keep the driver's own identifier ` + + `with the bound statement and its values cut. Remedy: read those entries — a sys_permission_set ` + + `missing its drift_status/drift_detail columns is a deployment SCHEMA defect, reported by ` + + `"os migrate plan" and fixed by "os migrate apply"; anything else is a store outage, which this pass ` + + `re-computes and re-attempts on the next boot. Either way nothing is lost, and nothing is recorded ` + + `either until a write lands.`, + { + refused: refusals.total, + refusals: entries, + ...(organizationId ? { organization: organizationId } : {}), + }, + ); +} + /** * Write the computed diagnostics onto their `sys_permission_set` rows. * EQUALITY-GATED (#10946 discipline): a row whose stored `drift_status` / @@ -200,40 +273,66 @@ export async function computePermissionSetDriftDiagnostics( * fact, not merely a filtered view: a quiet set carries no `drift_status` * value at all, so a client reading the raw record (not only the Setup * "Needs Attention" view) sees nothing to worry about either. + * + * Answers `refused` alongside `updated`. ⚠️ They are not two spellings of the + * same pass: `updated` counts the verdicts that LANDED and `refused` counts + * the ones the store rejected, and a caller asking "was there drift?" while + * reading only `updated` reads a wholly refused pass as a clean one — the + * defect {@link reportDriftWriteRefusals} and the gate in + * {@link runPermissionSetDriftDiagnostics} exist to close. */ export async function persistPermissionSetDriftDiagnostics( ql: any, diagnostics: readonly PermissionSetDriftDiagnostic[], opts: DriftDiagnosticsOptions = {}, -): Promise<{ updated: number }> { +): Promise<{ updated: number; refused: number }> { + // ⛔ The refusal LOG is what makes a refused write distinguishable from + // "nothing to write". `tryUpdate` answers `false` for both, and this + // function's only output used to be a count of the writes that LANDED — so + // a pass whose every write was refused returned `{ updated: 0 }`, which is + // byte-identical to the steady-state boot the equality gate above is built + // to produce. See the report below for what that silence cost. + const refusals = createSeedWriteRefusals(); let updated = 0; for (const d of diagnostics) { const status: string | null = d.status === 'in_sync' ? null : d.status; const detail: string | null = d.status === 'in_sync' ? null : d.detail; if (d.priorStatus === status && d.priorDetail === detail) continue; - if (await tryUpdate(ql, 'sys_permission_set', { id: d.id, drift_status: status, drift_detail: detail }, opts.organizationId)) { + if (await tryUpdate(ql, 'sys_permission_set', { id: d.id, drift_status: status, drift_detail: detail }, opts.organizationId, refusals)) { updated += 1; } } - return { updated }; + reportDriftWriteRefusals(opts.logger, refusals, opts.organizationId); + return { updated, refused: refusals.total }; } /** Compute + persist in one call — what boot wiring uses. */ export async function runPermissionSetDriftDiagnostics( ql: any, opts: DriftDiagnosticsOptions = {}, -): Promise<{ diagnostics: PermissionSetDriftDiagnostic[]; updated: number }> { +): Promise<{ diagnostics: PermissionSetDriftDiagnostic[]; updated: number; refused: number }> { const diagnostics = await computePermissionSetDriftDiagnostics(ql, opts); - const { updated } = await persistPermissionSetDriftDiagnostics(ql, diagnostics, opts); - if (updated > 0) { + const { updated, refused } = await persistPermissionSetDriftDiagnostics(ql, diagnostics, opts); + // ⛔ `updated > 0` ALONE was the suppressor. A boot on which every drift + // write is refused computes the drift correctly, persists none of it, and — + // under the old gate — printed nothing at all, which is byte-identical to a + // deployment with no drift. `refused > 0` re-opens exactly that case and + // nothing else: a steady-state boot (equality-gated, nothing to write, + // nothing refused) stays as quiet as it was. + if (updated > 0 || refused > 0) { opts.logger?.warn?.( '[security] package-declared permission set(s) enforcing grants that differ from the shipped artifact', { updated, + // Present ONLY when non-zero, so the steady-state line is unchanged + // byte-for-byte for anything reading it. Read it together with + // `updated`: `updated` counts the verdicts that LANDED, never the + // verdicts that were reached. + ...(refused > 0 ? { refused } : {}), drifted: diagnostics.filter((d) => d.status !== 'in_sync').map((d) => ({ name: d.name, status: d.status })), ...(opts.organizationId ? { organization: opts.organizationId } : {}), }, ); } - return { diagnostics, updated }; + return { diagnostics, updated, refused }; } diff --git a/packages/plugins/plugin-security/src/permission-set-overlay-discard.ts b/packages/plugins/plugin-security/src/permission-set-overlay-discard.ts index 12f343cab1..414da8010a 100644 --- a/packages/plugins/plugin-security/src/permission-set-overlay-discard.ts +++ b/packages/plugins/plugin-security/src/permission-set-overlay-discard.ts @@ -63,6 +63,10 @@ import { type ProjectionDeps, type ProjectionLogger, } from './permission-set-projection.js'; +import { + createSeedWriteRefusals, + logSeedDurabilityFailure, +} from './per-organization-catalog.js'; import { readDeclared } from './bootstrap-declared-permissions.js'; import { PermissionDeniedError } from './errors.js'; import { isTenantAdmin } from './delegated-admin-gate.js'; @@ -208,7 +212,9 @@ export async function discardPermissionSetOverlay( // registry echo (`syncEvaluatorRegistry`'s "overlay gone" branch). const protocol = deps.getProtocol?.(); const projectionDeps: ProjectionDeps = { ql, metadata: deps.metadata, logger }; + const refusals = createSeedWriteRefusals(); let healedRow: any; + let resyncRefused = false; if (protocol && typeof protocol.getMetaItemLayered === 'function') { await projectPermissionMutation(protocol, projectionDeps, { type: 'permission', name: String(row.name), state: 'active', organizationId: organizationId ?? null, @@ -218,23 +224,69 @@ export async function discardPermissionSetOverlay( // Degraded kernel with no metadata protocol: project the declared // artifact's facets directly, same shape `upsertEnvPermissionSet` writes, // clearing `customized` (there is no overlay left to badge). - await tryUpdate( + // + // ⛔ The result is READ. Discarding it is what let the audit line below + // assert a completed operator action over a write the store refused: on + // refusal `healedRow` is re-read as the UNCHANGED row, `objectGrantsAfter` + // equals `objectGrantsBefore`, and every field of the entry stays + // individually true while the entry as a whole is false. An audit record + // of a sanctioned action is the one record that may not be optimistic. + resyncRefused = !(await tryUpdate( ql, 'sys_permission_set', { id, ...permissionSetRowFields(declaredItem), customized: false }, organizationId, - ); + refusals, + )); healedRow = (await tryFind(ql, 'sys_permission_set', { id }, 1, organizationId))[0] ?? row; } const afterCount = countGrantedObjects(healedRow); // Audited: who, what, before/after — the "supported, audited action" ask. - logger?.info?.('[security] package-declared permission set overlay discarded (sanctioned operator action)', { + const audit = { id, name: row.name, packageId: declaredItem._packageId ?? declaredItem.packageId, by: callerCtx?.userId, organization: organizationId, overlaysDiscarded: overlays.length, objectGrantsBefore: beforeCount, objectGrantsAfter: afterCount, - }); + }; + if (resyncRefused) { + // ⛔ EXACTLY ONE audit entry per action, and when the resync was refused + // it is THIS one — the success line above is withheld, never emitted + // alongside. Two entries for one action would leave a reader to decide + // which is authoritative, and the optimistic one is the one that reads + // like the rest of the ledger. + // + // Emitted-with-the-failure-stated rather than withheld outright, because + // the destructive HALF of this operator action DID land: the sys_metadata + // overlay row is deleted and gone (that leg rethrows, so reaching here + // means it succeeded). Withholding the entry would erase the record of a + // deletion that actually happened — a worse audit defect than the + // optimistic one this repairs. + // + // Durability channel (AGENTS.md "Degradation log levels"): the caller is + // answered normally by ruling, the API returns 200, and the row silently + // goes on enforcing its pre-discard grants — the "still looks normal from + // the outside" shape, which is `error` with the mandatory `warn` fallback + // for hosts that inject a sink without one. + logSeedDurabilityFailure( + logger, + `[security] package-declared permission set overlay discarded BUT THE RESYNC WRITE WAS REFUSED ` + + `(sanctioned operator action, PARTIALLY applied) — the stale sys_metadata overlay row(s) WERE ` + + `deleted and are gone, and the follow-up write that re-projects the declared artifact onto ` + + `sys_permission_set was refused by the store, so the row STILL ENFORCES ITS PRE-DISCARD GRANTS. ` + + `"objectGrantsAfter" below is therefore the UN-HEALED count, and the caller was answered with that ` + + `same number as "healedObjectGrantCount" and a 200 — NOTHING LOOKS BROKEN from the API. This entry ` + + `replaces the "overlay discarded" success line, which is NOT also emitted for this action. ` + + `Recovery: the overlay is gone, so the ADR-0094 env-door reconciler ` + + `("reconcilePermissionSetProjection") re-projects this row on its own on the NEXT BOOT — restart, ` + + `or re-run this action once the store accepts writes again, to converge sooner. What the store ` + + `actually said is in the query engine's "Update operation failed" entry logged just before this ` + + `one, which keeps the driver's own identifier with the bound statement and its values cut.`, + { ...audit, resyncWriteRefused: true, refusals: refusals.report() }, + ); + } else { + logger?.info?.('[security] package-declared permission set overlay discarded (sanctioned operator action)', audit); + } return { permissionSet: healedRow, healedObjectGrantCount: afterCount, overlaysDiscarded: overlays.length }; } From fc6df0b2b0dbc8ed58324268337c8d96b8d015f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 07:22:30 +0000 Subject: [PATCH 2/2] fix(plugin-security): report the two swallowed tryUpdate refusals outside the catalog seed --- .../permission-set-refusal-visibility.md | 44 ++++++ .../src/permission-set-drift.test.ts | 121 +++++++++++++++- .../permission-set-overlay-discard.test.ts | 130 +++++++++++++++++- .../src/permission-set-overlay-discard.ts | 27 +++- 4 files changed, 317 insertions(+), 5 deletions(-) create mode 100644 .changeset/permission-set-refusal-visibility.md diff --git a/.changeset/permission-set-refusal-visibility.md b/.changeset/permission-set-refusal-visibility.md new file mode 100644 index 0000000000..4e8e8206d9 --- /dev/null +++ b/.changeset/permission-set-refusal-visibility.md @@ -0,0 +1,44 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): report the two swallowed `tryUpdate` refusals outside the catalog seed (#12970) + +Both sites call the shared `tryUpdate` in `permission-set-projection.ts`, which +answers `false` on refusal. That answer is byte-identical to "nothing to do", +and neither caller passed the optional refusal log the helper already accepts — +so a refused write was indistinguishable from a clean pass. + +**`permission-set-drift.ts` — a refused diagnostic write silenced its own +report.** `persistPermissionSetDriftDiagnostics` counted only the writes that +landed, and `runPermissionSetDriftDiagnostics` reported only when that count was +non-zero. A boot on which every drift write was refused computed the drift +correctly, persisted none of it, and printed nothing at all — indistinguishable +from a deployment with no drift, while the sets kept enforcing grants that +differ from the shipped artifact. The pass now records refusals, answers a +`refused` count beside `updated`, reports them once per pass on the durability +channel, and emits the drifted-set line when writes were refused as well as when +they landed. A steady-state boot (nothing to write, nothing refused) stays +exactly as quiet as before. + +**`permission-set-overlay-discard.ts` — the audit line could describe a discard +that did not happen.** On the degraded-kernel branch the resync write's result +was discarded entirely. On refusal the row was re-read unchanged, so +`objectGrantsAfter` equalled `objectGrantsBefore` while the `info` entry still +announced a completed "sanctioned operator action": every field individually +true, the entry as a whole false. The result is now read, and a refused resync +emits one entry stating what did and did not land — the overlay row deletion +(which had already succeeded) and the refused resync, with the un-healed grant +count named as such — **instead of** the success line, never alongside it. + +Both new lines go through the shared durability channel with its mandatory +`warn` fallback, so they still print against a host sink that has no `error`. +They reuse the shared refusal *accumulator* (`createSeedWriteRefusals`, with its +cross-dialect classification and value-free driver-code channel) but not +`reportSeedWriteRefusals`, whose prose is specific to seeding the RBAC catalog +and would misdiagnose either of these paths. + +No API is removed or narrowed. `persistPermissionSetDriftDiagnostics` and +`runPermissionSetDriftDiagnostics` answer one additional field (`refused`), and +what `discardPermissionSetOverlay` returns to its caller is deliberately +unchanged. diff --git a/packages/plugins/plugin-security/src/permission-set-drift.test.ts b/packages/plugins/plugin-security/src/permission-set-drift.test.ts index 2c86b1c7ff..5ece433659 100644 --- a/packages/plugins/plugin-security/src/permission-set-drift.test.ts +++ b/packages/plugins/plugin-security/src/permission-set-drift.test.ts @@ -24,8 +24,18 @@ import { } from './permission-set-drift.js'; import { permissionSetRowFields } from './permission-set-projection.js'; -/** Minimal in-memory ql: sys_permission_set + sys_metadata + $in support. */ -function makeQl(declared: any[] = []) { +/** + * Minimal in-memory ql: sys_permission_set + sys_metadata + $in support. + * + * `refuseUpdatesWith` makes every `update` throw AFTER the dispatch predicate + * has accepted the call shape — the real failure ORDER (the engine validates, + * the store refuses), and the only one `tryUpdate`'s catch ever sees. Placing + * the throw first would let a call shape the real engine rejects pass this + * suite. Extended in place rather than added as a second double, deliberately: + * this file's `update` double is pinned 1-per-file in + * `scripts/engine-double-contract.pinned.json`. + */ +function makeQl(declared: any[] = [], refusal: { refuseUpdatesWith?: Error } = {}) { const permRows: any[] = []; const metaRows: any[] = []; const tableFor = (object: string) => @@ -63,6 +73,7 @@ function makeQl(declared: any[] = []) { async update(object: string, data: any, options?: any) { const rows = tableFor(object); const dispatch = assertEngineUpdateDispatch(data, options); + if (refusal.refuseUpdatesWith) throw refusal.refuseUpdatesWith; const targets = dispatch.kind === 'by-id' ? (rows ?? []).filter((r: any) => r.id === dispatch.id) : (rows ?? []).filter((r: any) => matches(r, options?.where)); @@ -207,3 +218,109 @@ describe('persistPermissionSetDriftDiagnostics — writes are equality-gated', ( expect(second.updated).toBe(0); // nothing changed — no round trip }); }); + + +/* ------------------------------------------------------------------------- * + * pin 6 — a REFUSED diagnostic write must be LOUD + * + * The defect: `persistPermissionSetDriftDiagnostics` counted only the writes + * that LANDED, and `runPermissionSetDriftDiagnostics` reported only when that + * count was non-zero. So a boot on which every drift write was refused + * computed the drift correctly, persisted none of it, and printed NOTHING — + * byte-identical to a deployment with no drift, while the drifted sets kept + * enforcing grants that differ from the shipped artifact. + * + * Driver-error spellings are taken from the shipped classifier's own measured + * fixtures (see `seed-write-refusal.test.ts`), never invented here. + * ------------------------------------------------------------------------- */ + +/** Records every channel separately, with `error`'s real 3-arg shape. */ +function recordingLogger() { + const info: any[] = []; + const warn: any[] = []; + const error: any[] = []; + return { + info, warn, error, + sink: { + info: (m: string, meta?: any) => { info.push({ m, meta }); }, + warn: (m: string, meta?: any) => { warn.push({ m, meta }); }, + error: (m: string, e?: any, meta?: any) => { error.push({ m, e, meta }); }, + }, + }; +} + +/** NOT a unique violation — the realistic refusal for an update-by-id. */ +const connectionFailure = () => + Object.assign(new Error('connect ECONNREFUSED 127.0.0.1:5432'), { code: 'ECONNREFUSED' }); + +describe('runPermissionSetDriftDiagnostics — pin 6: a refused diagnostic write is visible', () => { + it('a pass whose every write is REFUSED still reports — the drifted set is named, and the refusal is reported on the durability channel', async () => { + const artifact = declaredSet(); + const ql = makeQl([artifact], { refuseUpdatesWith: connectionFailure() }); + ql.permRows.push(inSyncRow({ + managed_by: 'admin', // provenance_skip — real drift + object_permissions: JSON.stringify({ obj_a: { allowRead: true } }), + })); + const log = recordingLogger(); + + const result = await runPermissionSetDriftDiagnostics(ql, { logger: log.sink }); + + // The drift was computed correctly and NONE of it landed. + expect(result.diagnostics[0].status).toBe('provenance_skip'); + expect(result.updated).toBe(0); + expect(result.refused).toBe(1); + expect(ql.permRows[0].drift_status).toBeUndefined(); + + // ⭐ The boot is no longer indistinguishable from a clean one. + expect(log.error).toHaveLength(1); + expect(log.error[0].m).toContain('REFUSED'); + expect(log.error[0].meta.refused).toBe(1); + expect(log.error[0].meta.refusals[0]).toMatchObject({ + object: 'sys_permission_set', class: 'other', count: 1, + }); + // The value-free code channel, never the bound statement. + expect(log.error[0].meta.refusals[0].driverCodes).toContain('ECONNREFUSED'); + // Never the catalog-seed prose: this pass seeds nothing, and sending an + // operator after a legacy platform-wide catalog index would be a + // confident wrong answer. See `reportDriftWriteRefusals`. + expect(log.error[0].m).not.toContain('RBAC catalog'); + + // …and the line that NAMES the drifted set is no longer gated behind the + // counter the refusal suppressed. + const drift = log.warn.find((l) => l.m.includes('differ from the shipped artifact')); + expect(drift).toBeDefined(); + expect(drift!.meta.drifted).toEqual([{ name: 'ehr_quality_inspector', status: 'provenance_skip' }]); + expect(drift!.meta.updated).toBe(0); + expect(drift!.meta.refused).toBe(1); + }); + + it('⭐ counter-direction: a steady-state pass (nothing to write, nothing refused) still prints NOTHING', async () => { + const ql = makeQl([declaredSet()]); + ql.permRows.push(inSyncRow({ drift_status: null, drift_detail: null })); + const log = recordingLogger(); + + const result = await runPermissionSetDriftDiagnostics(ql, { logger: log.sink }); + + expect(result.updated).toBe(0); + expect(result.refused).toBe(0); + // The new `refused > 0` limb re-opens the refusal case and nothing else — + // a quiet boot stays exactly as quiet as it was. + expect(log.error).toHaveLength(0); + expect(log.warn).toHaveLength(0); + }); + + it('against a REDUCED sink with no `error`, the refusal still prints — at `warn`, never nowhere', async () => { + const ql = makeQl([declaredSet()], { refuseUpdatesWith: connectionFailure() }); + ql.permRows.push(inSyncRow({ + managed_by: 'admin', + object_permissions: JSON.stringify({ obj_a: { allowRead: true } }), + })); + const warn: any[] = []; + + // `{ warn }` alone is a legal ProjectionLogger — hosts do inject reduced + // sinks, which is exactly why the durability fallback is mandatory. + await runPermissionSetDriftDiagnostics(ql, { logger: { warn: (m: string, meta?: any) => { warn.push({ m, meta }); } } }); + + expect(warn.some((l) => l.m.includes('REFUSED'))).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-security/src/permission-set-overlay-discard.test.ts b/packages/plugins/plugin-security/src/permission-set-overlay-discard.test.ts index b729c73117..4be7745825 100644 --- a/packages/plugins/plugin-security/src/permission-set-overlay-discard.test.ts +++ b/packages/plugins/plugin-security/src/permission-set-overlay-discard.test.ts @@ -24,8 +24,16 @@ import { import { PermissionDeniedError } from './errors.js'; import { permissionSetRowFields } from './permission-set-projection.js'; -/** Minimal in-memory ql: sys_permission_set + sys_metadata + delete support. */ -function makeQl(declared: any[] = []) { +/** + * Minimal in-memory ql: sys_permission_set + sys_metadata + delete support. + * + * `refuseUpdatesWith` makes every `update` throw AFTER the dispatch predicate + * has accepted the call shape — the real failure ORDER (the engine validates, + * the store refuses), and the only one `tryUpdate`'s catch ever sees. Extended + * in place rather than added as a second double: this file's `update`/`delete` + * doubles are pinned 1-per-file in `scripts/engine-double-contract.pinned.json`. + */ +function makeQl(declared: any[] = [], refusal: { refuseUpdatesWith?: Error } = {}) { const permRows: any[] = []; const metaRows: any[] = []; const tableFor = (object: string) => @@ -57,6 +65,7 @@ function makeQl(declared: any[] = []) { async update(object: string, data: any, options?: any) { const rows = tableFor(object); const dispatch = assertEngineUpdateDispatch(data, options); + if (refusal.refuseUpdatesWith) throw refusal.refuseUpdatesWith; const targets = dispatch.kind === 'by-id' ? (rows ?? []).filter((r: any) => r.id === dispatch.id) : (rows ?? []).filter((r: any) => matches(r, options?.where)); @@ -230,3 +239,120 @@ describe('discardPermissionSetOverlay — pin 5: refuses what it must', () => { expect(ql.metaRows).toHaveLength(1); // untouched }); }); + + +/* ------------------------------------------------------------------------- * + * pin 6 — the audit entry may never assert a write the store refused + * + * The defect: on the degraded-kernel branch (no metadata protocol) the resync + * `tryUpdate`'s result was discarded — not assigned, not tested. On refusal + * the row was re-read UNCHANGED, so `objectGrantsAfter` equalled + * `objectGrantsBefore` and the `info` line still announced a completed + * "sanctioned operator action". Every field was individually true; the entry + * as a whole was false. That is the one record that may not be optimistic. + * ------------------------------------------------------------------------- */ + +function recordingLogger() { + const info: any[] = []; + const warn: any[] = []; + const error: any[] = []; + return { + info, warn, error, + sink: { + info: (m: string, meta?: any) => { info.push({ m, meta }); }, + warn: (m: string, meta?: any) => { warn.push({ m, meta }); }, + error: (m: string, e?: any, meta?: any) => { error.push({ m, e, meta }); }, + }, + }; +} + +const connectionFailure = () => + Object.assign(new Error('connect ECONNREFUSED 127.0.0.1:5432'), { code: 'ECONNREFUSED' }); + +/** The degraded kernel this branch exists for: no metadata protocol at all. */ +function degradedDeps(ql: ReturnType, logger: any): PermissionSetOverlayDiscardDeps { + return { ...deps(ql), logger, getProtocol: () => undefined }; +} + +describe('discardPermissionSetOverlay — pin 6: a refused resync is never audited as a success', () => { + it('degraded kernel + REFUSED resync: the success line is WITHHELD and one entry stating the failure is emitted instead', async () => { + const artifact = declaredSet(); // ships 3 objects + const ql = makeQl([artifact], { refuseUpdatesWith: connectionFailure() }); + ql.permRows.push({ + id: 'ps_1', name: 'ehr_quality_inspector', managed_by: 'package', package_id: 'com.example.ehr', + ...permissionSetRowFields(artifact), + object_permissions: JSON.stringify({ obj_a: { allowRead: true }, obj_b: { allowRead: true } }), // 2 — stale + }); + ql.metaRows.push(overlayRow({ obj_a: {}, obj_b: {} })); + const log = recordingLogger(); + + const result = await discardPermissionSetOverlay(degradedDeps(ql, log.sink), tenantAdminCtx, 'ps_1'); + + // The destructive half DID land — which is why the entry is emitted with + // the failure stated rather than withheld outright. + expect(ql.metaRows).toHaveLength(0); + // The row was NOT healed: it still enforces its pre-discard grants. + expect(result.healedObjectGrantCount).toBe(2); + + // ⭐ No optimistic audit line, at any level. + expect(log.info.some((l) => l.m.includes('overlay discarded (sanctioned operator action)'))).toBe(false); + expect(log.warn.some((l) => l.m.includes('(sanctioned operator action)'))).toBe(false); + + // Exactly ONE entry, and it states what did and did not land. + expect(log.error).toHaveLength(1); + expect(log.error[0].m).toContain('RESYNC WRITE WAS REFUSED'); + expect(log.error[0].m).toContain('PARTIALLY applied'); + expect(log.error[0].meta).toMatchObject({ + id: 'ps_1', + name: 'ehr_quality_inspector', + by: 'u_admin', + overlaysDiscarded: 1, + objectGrantsBefore: 2, + objectGrantsAfter: 2, // un-healed, and the message says so + resyncWriteRefused: true, + }); + expect(log.error[0].meta.refusals[0]).toMatchObject({ object: 'sys_permission_set', class: 'other', count: 1 }); + expect(log.error[0].meta.refusals[0].driverCodes).toContain('ECONNREFUSED'); + }); + + it('⭐ counter-direction: the same degraded branch with a write the store ACCEPTS still emits the unchanged success line, and nothing on the failure channel', async () => { + const artifact = declaredSet(); + const ql = makeQl([artifact]); // updates land + ql.permRows.push({ + id: 'ps_1', name: 'ehr_quality_inspector', managed_by: 'package', package_id: 'com.example.ehr', + ...permissionSetRowFields(artifact), + object_permissions: JSON.stringify({ obj_a: { allowRead: true }, obj_b: { allowRead: true } }), + }); + ql.metaRows.push(overlayRow({ obj_a: {}, obj_b: {} })); + const log = recordingLogger(); + + const result = await discardPermissionSetOverlay(degradedDeps(ql, log.sink), tenantAdminCtx, 'ps_1'); + + expect(result.healedObjectGrantCount).toBe(3); // genuinely healed to the artifact + expect(log.error).toHaveLength(0); + const success = log.info.find((l) => l.m.includes('overlay discarded (sanctioned operator action)')); + expect(success).toBeDefined(); + expect(success!.meta).toMatchObject({ objectGrantsBefore: 2, objectGrantsAfter: 3, overlaysDiscarded: 1 }); + expect(success!.meta.resyncWriteRefused).toBeUndefined(); + }); + + it('against a REDUCED sink with no `error`, the refused resync still prints — at `warn`, never nowhere', async () => { + const artifact = declaredSet(); + const ql = makeQl([artifact], { refuseUpdatesWith: connectionFailure() }); + ql.permRows.push({ + id: 'ps_1', name: 'ehr_quality_inspector', managed_by: 'package', package_id: 'com.example.ehr', + ...permissionSetRowFields(artifact), + object_permissions: JSON.stringify({ obj_a: { allowRead: true } }), + }); + ql.metaRows.push(overlayRow({ obj_a: {} })); + const warn: any[] = []; + + await discardPermissionSetOverlay( + degradedDeps(ql, { warn: (m: string, meta?: any) => { warn.push({ m, meta }); } }), + tenantAdminCtx, + 'ps_1', + ); + + expect(warn.some((l) => l.m.includes('RESYNC WRITE WAS REFUSED'))).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-security/src/permission-set-overlay-discard.ts b/packages/plugins/plugin-security/src/permission-set-overlay-discard.ts index 414da8010a..341a90a675 100644 --- a/packages/plugins/plugin-security/src/permission-set-overlay-discard.ts +++ b/packages/plugins/plugin-security/src/permission-set-overlay-discard.ts @@ -51,6 +51,21 @@ * environment-authored and refused — the maintainer's cited hazard verbatim: * "a name collision with a genuinely env-authored set would be destroyed * without a trace". + * + * ## The audit entry may never be optimistic + * + * This action's whole point is that it is AUDITED, so the one thing its log + * entry may not do is assert an action that did not fully land. The + * degraded-kernel branch below discarded its `tryUpdate` result, and on a + * refused resync every field of the success entry stayed individually true — + * the overlay count was right, `objectGrantsBefore`/`objectGrantsAfter` were + * both read off real rows — while the entry as a whole claimed a completed + * sanctioned operator action over a write the store had rejected. The result + * is now read, and a refused resync gets its own entry stating the failure + * INSTEAD of the success line (never alongside it): the overlay deletion DID + * land and must stay on the record, and the un-healed grant count must stop + * being reported as a healed one. What the CALLER is told is deliberately + * unchanged — see {@link PermissionSetOverlayDiscardResult}. */ import type { PermissionSet } from '@objectstack/spec/security'; @@ -103,7 +118,17 @@ export interface PermissionSetOverlayDiscardDeps { export interface PermissionSetOverlayDiscardResult { permissionSet: any; - /** Object grants the row now carries (post-reconcile) — the "healed" number, for callers to assert on. */ + /** + * Object grants the row now carries (post-reconcile) — the "healed" number, + * for callers to assert on. + * + * ⚠️ Literally "what the row carries NOW", which is not always a healed + * count: on the degraded-kernel branch a REFUSED resync write leaves this + * equal to the pre-discard count. That case is reported on the audit + * channel (an entry naming the refusal replaces the success line) and + * deliberately NOT on this contract — propagating it to the caller would + * widen a declared surface and is out of scope here. + */ healedObjectGrantCount: number; overlaysDiscarded: number; }