diff --git a/.changeset/auth-admin-audit-swallow-batch-6.md b/.changeset/auth-admin-audit-swallow-batch-6.md new file mode 100644 index 0000000000..bd7019ae25 --- /dev/null +++ b/.changeset/auth-admin-audit-swallow-batch-6.md @@ -0,0 +1,47 @@ +--- +'@objectstack/plugin-auth': minor +--- + +Report the refused admin-audit writes two `catch { }` sites swallowed (#12981 batch 6) + +The two tier-1 DARK durability swallows on `plugin-auth`'s admin surface: an +administrative action landed, its audit row was refused, and the endpoint +answered `200` with nothing written anywhere. Control flow is unchanged at both +sites — an admin operation must never fail over its own audit — but the refusal +is no longer silent. + +Both `catch` blocks were doing two jobs and were only right about one of them: + +- **plugin-audit UNINSTALLED** — there is no `sys_audit_log` object at all, so + nothing ever claimed the action would be audited. Silence is correct, and + reporting here would put a line on every admin action in every deployment that + does not run plugin-audit. +- **plugin-audit INSTALLED, the write REFUSED** — the action happened, the audit + record did not, and nothing retries or reconstructs it. + +Both spelled `catch { }`. Each site now asks `getSchema('sys_audit_log')` — the +registry that owns the answer — instead of reading the driver's error text, +which would decide the same question by guessing. `getSchema` is declared +**optional** on `AdminUserDataEngine` and `IdentityImportEngine`, so it is +additive and no host that type-checks today stops doing so; where it is absent +the site cannot measure the difference and therefore reports, because an +unmeasurable write must not be a silent one. + +What was hiding in the silence: + +- `admin-user-endpoints.ts :: writeAdminAudit` — `sys_account` is in + plugin-audit's `SKIP_OBJECTS`, so for `/admin/set-user-password` its generic + writer emits **zero** rows and the row refused here was the only record that a + password was ever administratively reset. +- `admin-import-users.ts` run-level row — `action: 'import'` with a null + `record_id` is a shape plugin-audit's `actionFor` structurally cannot emit. The + per-row `create` rows still land, which is what made this dangerous: the trail + looked complete while who ran the import, under which password policy, and what + it did in aggregate was gone. + +Both sinks (`AdminUserEndpointDeps.logger`, `IdentityImportDeps.logger`) are +`{ warn(msg: string): void }` and both are re-exported from the package +`index.ts`. Neither declares `error`, so the LEVEL stays `warn` and remains +#13398's question; only the SILENCE is repaired here. Each seam is pinned by a +test that fails if it goes quiet again, plus absence-asserting cases so a seam +that warns unconditionally cannot pass. diff --git a/packages/plugins/plugin-auth/src/admin-import-users.ts b/packages/plugins/plugin-auth/src/admin-import-users.ts index 9fe377f9a2..cc79147d8a 100644 --- a/packages/plugins/plugin-auth/src/admin-import-users.ts +++ b/packages/plugins/plugin-auth/src/admin-import-users.ts @@ -89,6 +89,19 @@ export interface IdentityImportEngine { find(objectName: string, query?: any): Promise; update(objectName: string, data: any, options?: any): Promise; insert(objectName: string, data: any, options?: any): Promise; + /** + * [#12981] Optional registry probe — `ObjectQL.getSchema`, which answers + * `undefined` for an object no package registered. + * + * Separates the two outcomes the run-level audit `catch` used to spell + * identically: plugin-audit UNINSTALLED (no `sys_audit_log` object — nothing + * was ever claimed, so silence is correct) from plugin-audit INSTALLED AND + * THE WRITE REFUSED (the import ran, its only run-level record did not land, + * and the endpoint still answers 200). Optional, therefore additive: a host + * or mock without it keeps type-checking, and a site that cannot measure the + * difference REPORTS rather than going quiet. + */ + getSchema?(objectName: string): unknown; } export interface IdentityImportDeps { @@ -510,23 +523,56 @@ export async function runAdminImportUsers( // `sys_audit_log` table, and an import must not fail over its own audit. // Both facts are pinned in // `packages/qa/dogfood/test/admin-identity-audit-trail.dogfood.test.ts`. - try { - await engine.insert('sys_audit_log', { - action: 'import', - user_id: actor.id, - actor: actor.id, - object_name: 'sys_user', - metadata: JSON.stringify({ - event: 'user.import_run', - mode, matchBy, passwordPolicy: policy, - total: prepared.rows.length, - created: summary.created, updated: summary.updated, - skipped: summary.skipped, errors: summary.errors + preErrors, - // How `auto` (and the fixed policies) split the batch across channels. - delivery, - }), - }, { context: SYSTEM_CTX } as any); - } catch { /* audit table may not exist — never fail the import */ } + // + // [#12981] "Best-effort" was doing two jobs and only one of them was + // right. plugin-audit UNINSTALLED means no `sys_audit_log` object, so + // nothing ever claimed the run would be audited — a declared skip, taken + // silently by the probe below. A REFUSED write is the other thing + // entirely, and it was wearing the same `catch`. + const auditRegistered = !engine.getSchema || Boolean(engine.getSchema('sys_audit_log')); + if (auditRegistered) { + try { + await engine.insert('sys_audit_log', { + action: 'import', + user_id: actor.id, + actor: actor.id, + object_name: 'sys_user', + metadata: JSON.stringify({ + event: 'user.import_run', + mode, matchBy, passwordPolicy: policy, + total: prepared.rows.length, + created: summary.created, updated: summary.updated, + skipped: summary.skipped, errors: summary.errors + preErrors, + // How `auto` (and the fixed policies) split the batch across channels. + delivery, + }), + }, { context: SYSTEM_CTX } as any); + } catch (e) { + // [#12981] An import must not fail over its own audit — control flow is + // unchanged and the run still answers 200 with its summary. It must not + // be SILENT either. `sys_audit_log` is registered (checked above), so + // this is a refused write, and the run-level row is the ONLY record of + // it: plugin-audit's `actionFor` maps afterInsert/Update/Delete to + // create/update/delete and nothing else, so `action: 'import'` with a + // null `record_id` is a shape its writer structurally cannot emit. The + // per-row `create` rows survive, which is what makes this dangerous — + // the trail looks populated while WHO ran the import, under WHICH + // policy, and WHAT the run did overall is simply gone. + deps.logger?.warn( + `[AuthPlugin] the run-level sys_audit_log row for this user import was NOT written — ` + + `the import itself SUCCEEDED (created ${summary.created}, updated ${summary.updated}, ` + + `skipped ${summary.skipped}) and the endpoint answers 200, so nothing looks wrong. ` + + 'plugin-audit is installed (sys_audit_log is registered), so this is a REFUSED ' + + "write, not an absent plugin. plugin-audit's per-row create rows still landed, so " + + 'the audit trail LOOKS complete while the only record of who ran this import, under ' + + 'which password policy, and what it did in aggregate is absent. Nothing retries it ' + + 'and no later boot reconstructs it. Remedy: restore write access to sys_audit_log ' + + `(permissions, driver connectivity) before the next import. Cause: ${ + (e as Error)?.message ?? e + }`, + ); + } + } } const errors = summary.errors + preErrors; diff --git a/packages/plugins/plugin-auth/src/admin-user-endpoints.ts b/packages/plugins/plugin-auth/src/admin-user-endpoints.ts index 498d2d8db6..ea168649be 100644 --- a/packages/plugins/plugin-auth/src/admin-user-endpoints.ts +++ b/packages/plugins/plugin-auth/src/admin-user-endpoints.ts @@ -116,6 +116,24 @@ export interface AdminUserDataEngine { * isn't wired, in which case the org bind simply no-ops. */ find?(object: string, query?: unknown, opts?: unknown): Promise; + /** + * [#12981] Optional registry probe — `ObjectQL.getSchema`, which answers + * `undefined` for an object no package registered. + * + * It is here to separate two outcomes the audit `catch` below used to spell + * identically: plugin-audit UNINSTALLED (no `sys_audit_log` object, so + * nothing was ever claimed and silence is correct) from plugin-audit + * INSTALLED AND THE WRITE REFUSED (the admin action landed, its audit row did + * not, and the endpoint still answers 200). Reading the driver's error text + * would decide the same question by guessing; this asks the registry that + * owns the answer. + * + * Optional because lean mocks and hosts that wire no ObjectQL engine do not + * carry it — additive, so nothing that type-checks today stops doing so. When + * it is absent the site cannot tell the two apart and therefore REPORTS: an + * unmeasurable write must never be a silent one. + */ + getSchema?(objectName: string): unknown; } /** The gated caller, passed by the route after its ADR-0068 check. */ @@ -376,6 +394,12 @@ async function writeAdminAudit( ): Promise { const engine = deps.getDataEngine(); if (!engine) return; + // plugin-audit is OPTIONAL, and with it uninstalled there is no + // `sys_audit_log` object at all. That case is a DECLARED skip, not a + // swallow: nothing ever claimed this action would be audited, so there is + // nothing to report and the channel stays quiet — which is what keeps the + // `warn` below meaningful instead of one more line nobody reads. + if (engine.getSchema && !engine.getSchema('sys_audit_log')) return; try { await engine.insert( 'sys_audit_log', @@ -389,9 +413,31 @@ async function writeAdminAudit( }, { context: SYSTEM_CTX }, ); - } catch { - // plugin-audit may not be installed (no sys_audit_log table) — audit is - // best-effort by design here; the operation itself must not fail. + } catch (error) { + // [#12981] The operation itself must NOT fail over its own audit — control + // flow is unchanged and the endpoint still answers 200. But it must not be + // SILENT either, and this site is the sharpest case in the family: the + // header above records that `sys_account` is in plugin-audit's + // `SKIP_OBJECTS`, so for `/admin/set-user-password` the generic writer + // emits ZERO rows and the row refused here is the ONLY record that a + // password was administratively reset. Nothing retries it and no later + // boot reconstructs it — the reset simply has no trail, while the admin + // who performed it reads `success: true`. `sys_audit_log` is registered + // (checked above), so this is a refused write, not an absent plugin. + deps.logger?.warn( + `[AuthPlugin] the sys_audit_log row for this administrative '${entry.action}' on sys_user ` + + `${entry.recordId} was NOT written — the operation itself SUCCEEDED and the endpoint ` + + 'answers 200, so nothing looks wrong. plugin-audit is installed (sys_audit_log is ' + + 'registered), so this is a REFUSED write, not an absent plugin. This row carries the ' + + "admin's decisions (event, passwordGenerated, mustChangePassword, placeholderEmail, " + + 'membershipCreated), none of which is derivable from the stored row, and for ' + + '/admin/set-user-password it is the only audit record that exists at all because ' + + "sys_account is in plugin-audit's SKIP_OBJECTS. Nothing retries this write, so the " + + 'action stays permanently untrailed. Remedy: restore write access to sys_audit_log ' + + `(permissions, driver connectivity), then treat this line as the audit record. Cause: ${ + (error as Error)?.message ?? error + }`, + ); } } diff --git a/packages/plugins/plugin-auth/src/durability-swallow-repair.test.ts b/packages/plugins/plugin-auth/src/durability-swallow-repair.test.ts index 2cee394082..d97eb44a0e 100644 --- a/packages/plugins/plugin-auth/src/durability-swallow-repair.test.ts +++ b/packages/plugins/plugin-auth/src/durability-swallow-repair.test.ts @@ -461,3 +461,258 @@ describe('#12981 batch 5 — plugin-auth durability swallows report instead of v }); }); }); + +/** + * [#12981 batch 6] The two `plugin-auth` admin-AUDIT swallows, pinned. + * + * ## What is different about these two, and why the probe is the repair + * + * The eight batch-5 seams above swallowed a bookkeeping write outright. These + * two swallowed something subtler: a `catch` that was doing TWO jobs and was + * only right about one of them. + * + * - plugin-audit UNINSTALLED -> there is no `sys_audit_log` object at all, + * nothing ever claimed the action would be audited, and silence is the + * CORRECT answer. Reporting here would put a line on every admin action in + * every deployment that does not run plugin-audit — the "warn nobody + * reads" that #4420 is the historical accident for. + * - plugin-audit INSTALLED, write REFUSED -> the admin action landed, the + * endpoint answered 200, and its audit row did not exist. That is #12981's + * shape exactly. + * + * Both spelled `catch { }`. The repair asks `getSchema('sys_audit_log')` — + * the registry that owns the answer — instead of reading the driver's error + * text, which would decide the same question by guessing. + * + * ⚠️ So each site needs BOTH directions pinned, and the silent direction is + * the load-bearing one: a repair that warned unconditionally would pass every + * "it reports" case in this file while making the channel worthless. + * + * ## Level + * + * `warn`, and NOT because the consequence is small — an administrative + * password reset losing its only audit record is the distorted-audit class + * #12970 named. `AdminUserEndpointDeps.logger` and `IdentityImportDeps.logger` + * are both `{ warn(msg: string): void }` and both PUBLISHED (`index.ts` carries + * `export * from './admin-user-endpoints.js'` and `export *` for + * `./admin-import-users.js`). Neither declares `error`, so raising the level + * means widening a published sink — refused as actively harmful by the + * maintainer's #13398 ruling, which routes that question there and leaves the + * SILENCE here. The assertions below pin the channel, so a later level change + * is a deliberate edit rather than a drift. + */ +describe('#12981 batch 6 — the plugin-auth admin-audit swallows report instead of vanishing', () => { + const AUDIT_REFUSAL = new Error('write refused: no permission on sys_audit_log'); + + /** + * The engine the admin endpoints see. + * + * `getSchema` is the discriminator under test, so it is a real member here + * rather than a convenience: `registered` decides whether the double is a + * deployment that runs plugin-audit. `update` is pinned to ObjectQL's own + * dispatch predicate BEFORE any refusal branch, for the reason the batch-5 + * double states — a case must not be able to pass by handing the engine a + * call the real engine would have thrown on. + */ + const createAuditEngine = (opts: { registered: boolean; refuseAudit?: boolean }) => { + const insert = vi.fn(async (object: string) => { + if (object === 'sys_audit_log' && opts.refuseAudit) throw AUDIT_REFUSAL; + return { id: 'row-1' }; + }); + return { + insert, + update: vi.fn(async (_object: string, doc: Record, options?: unknown) => { + assertEngineUpdateDispatch(doc, options as never); + return { id: String(doc.id ?? 'updated') }; + }), + find: vi.fn(async () => []), + // `undefined` is exactly what ObjectQL answers for an object no package + // registered — the uninstalled-plugin case, not an error. + getSchema: vi.fn((object: string) => + opts.registered && object === 'sys_audit_log' ? { name: object } : undefined, + ), + }; + }; + + const auditInserts = (engine: { insert: ReturnType }): unknown[][] => + engine.insert.mock.calls.filter((c) => c[0] === 'sys_audit_log'); + + describe('admin-user-endpoints :: writeAdminAudit (`warn` — level is #13398\'s)', () => { + const ACTOR = { id: 'admin-1', email: 'admin@example.com' }; + + const makeDeps = (engine: ReturnType, logger: Capture) => ({ + getAuthApi: async () => + ({ + createUser: vi.fn(async ({ body }: never) => ({ + user: { id: 'user-9', email: (body as { email: string }).email }, + })), + }) as never, + getAuthContext: async () => + ({ + password: { + hash: vi.fn(async (pw: string) => `hashed(${pw})`), + config: { minPasswordLength: 8, maxPasswordLength: 128 }, + }, + internalAdapter: { + findUserById: vi.fn(async () => ({ id: 'user-9' })), + findAccounts: vi.fn(async () => [{ providerId: 'credential' }]), + updatePassword: vi.fn(async () => ({})), + createAccount: vi.fn(async () => ({})), + }, + }) as never, + getDataEngine: () => engine as never, + assertPasswordComplexity: vi.fn(async () => undefined), + noteMustChangePasswordIssued: vi.fn(), + logger: logger as never, + }); + + const createUserRequest = () => + new Request('http://localhost/api/v1/auth/admin/create-user', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ email: 'new@example.com', generatePassword: true }), + }); + + it('a refused audit row is reported, and names the action that still succeeded', async () => { + const logger = createLogger(); + const engine = createAuditEngine({ registered: true, refuseAudit: true }); + const { runAdminCreateUser } = await import('./admin-user-endpoints.js'); + + const res = await runAdminCreateUser(makeDeps(engine, logger) as never, createUserRequest(), ACTOR as never); + + // Control flow is UNCHANGED: the account exists and the caller sees 200. + // That is precisely why the line below is the operator's only evidence. + expect(res.status).toBe(200); + expect(res.body.success).toBe(true); + expect(auditInserts(engine)).toHaveLength(1); + + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(warnText(logger)).toContain('sys_audit_log'); + // The two facts that make the line actionable: the row did NOT land, and + // the operation it describes DID. + expect(warnText(logger)).toContain('NOT written'); + expect(warnText(logger)).toContain('SUCCEEDED'); + // The consequence, not merely the failure — this is the record that has + // no substitute, because sys_account is in plugin-audit's SKIP_OBJECTS. + expect(warnText(logger)).toContain('sys_account'); + // The driver's own text survives, so the line is greppable against the + // datasource log instead of being prose about a failure nobody can find. + expect(warnText(logger)).toContain('no permission on sys_audit_log'); + }); + + it('plugin-audit UNINSTALLED stays silent, and does not attempt the write', async () => { + const logger = createLogger(); + const engine = createAuditEngine({ registered: false }); + const { runAdminCreateUser } = await import('./admin-user-endpoints.js'); + + const res = await runAdminCreateUser(makeDeps(engine, logger) as never, createUserRequest(), ACTOR as never); + + expect(res.status).toBe(200); + // The declared skip: nothing claimed an audit row, so none is attempted + // and nothing is reported. A repair that warned here would fire on every + // admin action in every deployment without plugin-audit. + expect(engine.getSchema).toHaveBeenCalledWith('sys_audit_log'); + expect(auditInserts(engine)).toHaveLength(0); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it('a healthy audit write reports nothing on this channel', async () => { + const logger = createLogger(); + const engine = createAuditEngine({ registered: true }); + const { runAdminCreateUser } = await import('./admin-user-endpoints.js'); + + const res = await runAdminCreateUser(makeDeps(engine, logger) as never, createUserRequest(), ACTOR as never); + + expect(res.status).toBe(200); + expect(auditInserts(engine)).toHaveLength(1); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it('an engine without `getSchema` cannot measure the difference, so it REPORTS', async () => { + const logger = createLogger(); + const engine = createAuditEngine({ registered: true, refuseAudit: true }); + // A lean host/mock: the probe is optional, and its absence must fail + // toward the loud answer, never toward the silent one. + const lean = { insert: engine.insert, update: engine.update, find: engine.find }; + const { runAdminCreateUser } = await import('./admin-user-endpoints.js'); + + const res = await runAdminCreateUser(makeDeps(lean as never, logger) as never, createUserRequest(), ACTOR as never); + + expect(res.status).toBe(200); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(warnText(logger)).toContain('sys_audit_log'); + }); + }); + + describe('admin-import-users :: the run-level audit row (`warn` — level is #13398\'s)', () => { + const ACTOR = { id: 'admin-1', email: 'admin@example.com' }; + + const makeDeps = (engine: ReturnType, logger: Capture) => ({ + getAuthApi: async () => ({ + createUser: vi.fn(async ({ body }: never) => ({ + user: { id: 'u-1', email: (body as { email: string }).email }, + })), + requestPasswordReset: vi.fn(async () => ({ status: true })), + }), + getDataEngine: () => engine as never, + phoneNumberEnabled: () => false, + emailServiceAvailable: () => true, + smsInviteAvailable: () => false, + sendInviteSms: vi.fn(async () => undefined), + noteMustChangePasswordIssued: vi.fn(), + logger: logger as never, + }); + + const importRequest = () => + new Request('http://localhost/api/v1/auth/admin/import-users', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ format: 'json', rows: [{ email: 'a@b.co' }] }), + }); + + it('a refused run-level row is reported, and says the per-row trail survived', async () => { + const logger = createLogger(); + const engine = createAuditEngine({ registered: true, refuseAudit: true }); + const { runAdminImportUsers } = await import('./admin-import-users.js'); + + const res = await runAdminImportUsers(makeDeps(engine, logger) as never, importRequest(), ACTOR as never); + + expect(res.status).toBe(200); + expect(auditInserts(engine)).toHaveLength(1); + + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(warnText(logger)).toContain('sys_audit_log'); + expect(warnText(logger)).toContain('NOT written'); + // The dangerous part, and the one an operator cannot infer: the per-row + // `create` rows DID land, so the trail looks complete while the only + // record of who ran the import and under which policy is gone. + expect(warnText(logger)).toContain('per-row'); + expect(warnText(logger)).toContain('no permission on sys_audit_log'); + }); + + it('plugin-audit UNINSTALLED stays silent, and does not attempt the write', async () => { + const logger = createLogger(); + const engine = createAuditEngine({ registered: false }); + const { runAdminImportUsers } = await import('./admin-import-users.js'); + + const res = await runAdminImportUsers(makeDeps(engine, logger) as never, importRequest(), ACTOR as never); + + expect(res.status).toBe(200); + expect(engine.getSchema).toHaveBeenCalledWith('sys_audit_log'); + expect(auditInserts(engine)).toHaveLength(0); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it('a healthy run reports nothing on this channel', async () => { + const logger = createLogger(); + const engine = createAuditEngine({ registered: true }); + const { runAdminImportUsers } = await import('./admin-import-users.js'); + + const res = await runAdminImportUsers(makeDeps(engine, logger) as never, importRequest(), ACTOR as never); + + expect(res.status).toBe(200); + expect(auditInserts(engine)).toHaveLength(1); + expect(logger.warn).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 3056333405..9bb62ca1d6 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2114,7 +2114,7 @@ { "file": "packages/plugins/plugin-auth/src/durability-swallow-repair.test.ts", "verb": "update", - "pinned": 1 + "pinned": 2 }, { "file": "packages/plugins/plugin-auth/src/first-session-membership-ordering.test.ts",