|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | + |
| 3 | +/** |
| 4 | + * [#4940] What `sys_audit_log` actually holds after an admin identity |
| 5 | + * operation — measured on the real routes, with plugin-audit installed. |
| 6 | + * |
| 7 | + * Why this file exists. Two comments in `plugin-auth` justified their explicit |
| 8 | + * `sys_audit_log` insert with a mechanism claim: *"better-auth writes bypass |
| 9 | + * the ObjectQL lifecycle hooks that plugin-audit subscribes to, so admin |
| 10 | + * identity operations would otherwise leave no compliance trail."* That is the |
| 11 | + * same stale claim #4802 refuted one layer up (`AuthManagerOptions. |
| 12 | + * databaseHooks`): better-auth's adapter writes through the ordinary |
| 13 | + * `dataEngine.insert(...)`, `ObjectQL.insert()` fires `triggerHooks( |
| 14 | + * 'afterInsert')` inside `executeWithMiddleware()`, and plugin-audit registers |
| 15 | + * `writeAudit` as a bare `engine.registerHook('afterInsert', …)` with no |
| 16 | + * object filter. The hooks fire. |
| 17 | + * |
| 18 | + * The explicit rows are nevertheless CORRECT — for different reasons, which is |
| 19 | + * exactly why the claim being wrong is dangerous rather than harmless: the next |
| 20 | + * person to read it either deletes a row that is load-bearing, or keeps one for |
| 21 | + * a reason that will not survive contact with the code. So the reasons are |
| 22 | + * pinned here as measurements rather than left as prose: |
| 23 | + * |
| 24 | + * W1 `sys_user` is NOT in plugin-audit's `SKIP_OBJECTS` → the generic writer |
| 25 | + * DOES produce a row for a better-auth-created user. This is the |
| 26 | + * refutation: an assertion that fails if the bypass claim were true. |
| 27 | + * W2 …and the explicit row is a SECOND row on the same record. The overlap |
| 28 | + * is real, measured, and accepted — the two carry disjoint payloads. |
| 29 | + * W3 `sys_account` IS in `SKIP_OBJECTS` → the credential write behind |
| 30 | + * `/admin/set-user-password` produces NO generic row. The explicit row is |
| 31 | + * the only trail that a password was administratively reset. This is the |
| 32 | + * reason that would be silently destroyed by "the hook covers it, drop |
| 33 | + * the explicit insert". |
| 34 | + * W4 The import's run-level row is `action: 'import'` with `record_id: null` |
| 35 | + * — a shape plugin-audit's `actionFor` structurally cannot emit (it maps |
| 36 | + * afterInsert/Update/Delete → create/update/delete and nothing else), |
| 37 | + * alongside the per-row rows the hook does write. |
| 38 | + * |
| 39 | + * Harness notes: |
| 40 | + * - `bootStack` installs no audit plugin, so `AuditPlugin` is added here — |
| 41 | + * that is what turns better-auth's writes into generic audit rows at all. |
| 42 | + * - The admin identity routes 501 unless better-auth's `admin` plugin is on, |
| 43 | + * and `bootStack` exposes no auth-plugin override. `OS_SCIM_ENABLED` is the |
| 44 | + * one env knob that reaches it: `AuthManager.buildPluginList` resolves |
| 45 | + * `admin: pluginConfig.admin ?? scimEffective` (SCIM forces admin on), so |
| 46 | + * this is a deliberate, documented derivation rather than a coincidence. |
| 47 | + * Same shape as `two-factor-lockout.dogfood.test.ts`'s `OS_AUTH_TWO_FACTOR`: |
| 48 | + * read when the auth manager is constructed, so it must precede `bootStack`. |
| 49 | + * - Generic audit rows land ASYNCHRONOUSLY (better-auth may settle its writes |
| 50 | + * after the response), so every read polls rather than reading once. |
| 51 | + */ |
| 52 | + |
| 53 | +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; |
| 54 | +import showcaseStack from '@objectstack/example-showcase'; |
| 55 | +import { bootStack, type VerifyStack } from '@objectstack/verify'; |
| 56 | +import { AuditPlugin } from '@objectstack/plugin-audit'; |
| 57 | + |
| 58 | +const SYSTEM_CTX = { isSystem: true }; |
| 59 | + |
| 60 | +async function findRows(ql: any, object: string, where: any, limit = 200): Promise<any[]> { |
| 61 | + const rows = await ql.find(object, { where, limit }, { context: SYSTEM_CTX }); |
| 62 | + return Array.isArray(rows) ? rows : (rows?.records ?? []); |
| 63 | +} |
| 64 | + |
| 65 | +/** Audit rows recorded against one `sys_user` record. */ |
| 66 | +async function userAudit(ql: any, userId: string): Promise<any[]> { |
| 67 | + return findRows(ql, 'sys_audit_log', { object_name: 'sys_user', record_id: userId }); |
| 68 | +} |
| 69 | + |
| 70 | +/** Poll until `predicate` holds over the rows, then return them. */ |
| 71 | +async function waitForRows( |
| 72 | + load: () => Promise<any[]>, |
| 73 | + predicate: (rows: any[]) => boolean, |
| 74 | + what: string, |
| 75 | +): Promise<any[]> { |
| 76 | + let rows: any[] = []; |
| 77 | + for (let i = 0; i < 40; i++) { |
| 78 | + rows = await load(); |
| 79 | + if (predicate(rows)) return rows; |
| 80 | + await new Promise((r) => setTimeout(r, 250)); |
| 81 | + } |
| 82 | + throw new Error(`${what} — last saw ${rows.length} row(s): ${JSON.stringify(rows.map((r) => ({ a: r.action, m: r.metadata })))}`); |
| 83 | +} |
| 84 | + |
| 85 | +/** The row this endpoint writes itself: structured `metadata`, no field diff. */ |
| 86 | +const isExplicit = (row: any, event: string): boolean => |
| 87 | + typeof row.metadata === 'string' && row.metadata.includes(event); |
| 88 | + |
| 89 | +/** plugin-audit's generic row: a field diff, never a structured `metadata`. */ |
| 90 | +const isGeneric = (row: any): boolean => row.metadata == null; |
| 91 | + |
| 92 | +describe('#4940: what an admin identity operation leaves in sys_audit_log', () => { |
| 93 | + let stack: VerifyStack; |
| 94 | + let ql: any; |
| 95 | + let adminToken: string; |
| 96 | + let adminUserId: string; |
| 97 | + let priorScim: string | undefined; |
| 98 | + |
| 99 | + beforeAll(async () => { |
| 100 | + priorScim = process.env.OS_SCIM_ENABLED; |
| 101 | + process.env.OS_SCIM_ENABLED = 'true'; |
| 102 | + stack = await bootStack(showcaseStack, { extraPlugins: [new AuditPlugin()] }); |
| 103 | + adminToken = await stack.signIn(); // the seeded dev admin (platform admin) |
| 104 | + ql = await stack.kernel.getServiceAsync<any>('objectql'); |
| 105 | + const [admin] = await findRows(ql, 'sys_user', { email: 'admin@objectos.ai' }, 1); |
| 106 | + adminUserId = String(admin.id); |
| 107 | + }, 180_000); |
| 108 | + |
| 109 | + afterAll(async () => { |
| 110 | + await stack?.stop?.(); |
| 111 | + if (priorScim === undefined) delete process.env.OS_SCIM_ENABLED; |
| 112 | + else process.env.OS_SCIM_ENABLED = priorScim; |
| 113 | + }); |
| 114 | + |
| 115 | + // ── W1 + W2: create-user writes TWO create rows, not one ──────────────── |
| 116 | + |
| 117 | + it('POST /admin/create-user: plugin-audit\'s hook fires for the better-auth write, alongside the explicit row', async () => { |
| 118 | + const email = 'audited.4940@example.com'; |
| 119 | + const res = await stack.apiAs(adminToken, 'POST', '/auth/admin/create-user', { |
| 120 | + email, |
| 121 | + name: 'Audited 4940', |
| 122 | + password: 'Audited!Pass123', |
| 123 | + }); |
| 124 | + expect(res.status, await res.clone().text()).toBe(200); |
| 125 | + const userId = String((await res.json()).data.user.id); |
| 126 | + |
| 127 | + const creates = await waitForRows( |
| 128 | + async () => (await userAudit(ql, userId)).filter((r) => r.action === 'create'), |
| 129 | + (rows) => rows.length >= 2, |
| 130 | + 'expected both a generic and an explicit create row', |
| 131 | + ); |
| 132 | + |
| 133 | + // W1 — THE REFUTATION. If better-auth's writes really bypassed the |
| 134 | + // lifecycle hooks plugin-audit subscribes to, this row could not exist: |
| 135 | + // nothing but `engine.registerHook('afterInsert', writeAudit)` writes it. |
| 136 | + // It carries a full row snapshot in `new_value` and no `metadata`. |
| 137 | + const generic = creates.filter(isGeneric); |
| 138 | + expect(generic).toHaveLength(1); |
| 139 | + expect(String(generic[0].new_value)).toContain(email); |
| 140 | + |
| 141 | + // W2 — and the endpoint's own row is a SECOND row on the same record. |
| 142 | + // Kept deliberately: it records the admin's DECISIONS, none of which is |
| 143 | + // derivable from a field diff of the created row. |
| 144 | + const explicit = creates.filter((r) => isExplicit(r, 'user.admin_created')); |
| 145 | + expect(explicit).toHaveLength(1); |
| 146 | + expect(explicit[0].user_id).toBe(adminUserId); |
| 147 | + expect(String(explicit[0].metadata)).toContain('"mustChangePassword":true'); |
| 148 | + expect(String(explicit[0].metadata)).toContain('"passwordGenerated":false'); |
| 149 | + // The overlap is exactly two — measured, so a third writer appearing on |
| 150 | + // this path is a finding rather than a silent extra ledger row. |
| 151 | + expect(creates).toHaveLength(2); |
| 152 | + |
| 153 | + // Neither row is allowed to carry password material (the red line). |
| 154 | + for (const row of creates) { |
| 155 | + const blob = `${row.metadata ?? ''}${row.new_value ?? ''}${row.old_value ?? ''}`; |
| 156 | + expect(blob).not.toContain('Audited!Pass123'); |
| 157 | + } |
| 158 | + }, 120_000); |
| 159 | + |
| 160 | + // ── W3: the credential write is invisible to plugin-audit ─────────────── |
| 161 | + |
| 162 | + it('POST /admin/set-user-password: sys_account is in SKIP_OBJECTS, so the explicit row is the ONLY trail', async () => { |
| 163 | + const email = 'audited.pw.4940@example.com'; |
| 164 | + const created = await stack.apiAs(adminToken, 'POST', '/auth/admin/create-user', { |
| 165 | + email, |
| 166 | + name: 'Audited PW 4940', |
| 167 | + password: 'Audited!Pass123', |
| 168 | + }); |
| 169 | + expect(created.status, await created.clone().text()).toBe(200); |
| 170 | + const userId = String((await created.json()).data.user.id); |
| 171 | + await waitForRows( |
| 172 | + async () => (await userAudit(ql, userId)).filter((r) => r.action === 'create'), |
| 173 | + (rows) => rows.length >= 2, |
| 174 | + 'create rows for the password subject', |
| 175 | + ); |
| 176 | + |
| 177 | + const res = await stack.apiAs(adminToken, 'POST', '/auth/admin/set-user-password', { |
| 178 | + userId, |
| 179 | + newPassword: 'Rotated!Pass456', |
| 180 | + }); |
| 181 | + expect(res.status, await res.clone().text()).toBe(200); |
| 182 | + |
| 183 | + // The endpoint's own row — `action: 'update'` on `sys_user`, naming the |
| 184 | + // admin who reset the password. |
| 185 | + const [explicit] = await waitForRows( |
| 186 | + async () => (await userAudit(ql, userId)).filter((r) => isExplicit(r, 'user.admin_password_set')), |
| 187 | + (rows) => rows.length === 1, |
| 188 | + 'the explicit password-reset row', |
| 189 | + ); |
| 190 | + expect(explicit.action).toBe('update'); |
| 191 | + expect(explicit.user_id).toBe(adminUserId); |
| 192 | + expect(String(explicit.metadata)).not.toContain('Rotated!Pass456'); |
| 193 | + |
| 194 | + // W3 — the write this endpoint actually made is on `sys_account` (the |
| 195 | + // credential row), and `sys_account` is in plugin-audit's SKIP_OBJECTS. |
| 196 | + // Measured: ZERO generic rows for it. Delete the explicit insert on the |
| 197 | + // strength of "the hook covers it" and an administrative password reset |
| 198 | + // becomes untraceable — which is the concrete cost of the wrong comment. |
| 199 | + const accounts = await findRows(ql, 'sys_account', { user_id: userId }); |
| 200 | + expect(accounts.length).toBeGreaterThan(0); |
| 201 | + expect(await findRows(ql, 'sys_audit_log', { object_name: 'sys_account' })).toHaveLength(0); |
| 202 | + for (const account of accounts) { |
| 203 | + expect(await findRows(ql, 'sys_audit_log', { record_id: String(account.id) })).toHaveLength(0); |
| 204 | + } |
| 205 | + }, 120_000); |
| 206 | + |
| 207 | + // ── W4: the run-level row is a shape the generic writer cannot emit ───── |
| 208 | + |
| 209 | + it('POST /admin/import-users: the run-level row complements the per-row rows the hook writes', async () => { |
| 210 | + const email = 'imported.4940@example.com'; |
| 211 | + const res = await stack.apiAs(adminToken, 'POST', '/auth/admin/import-users', { |
| 212 | + format: 'json', |
| 213 | + rows: [{ email, name: 'Imported 4940' }], |
| 214 | + passwordPolicy: 'temporary', |
| 215 | + }); |
| 216 | + expect(res.status, await res.clone().text()).toBe(200); |
| 217 | + const body = await res.json(); |
| 218 | + expect(body.data.summary.created).toBe(1); |
| 219 | + const importedId = String(body.data.rows[0].id); |
| 220 | + |
| 221 | + // The hook DOES cover each imported row — same refutation as W1, on the |
| 222 | + // second of the two call sites whose comment claimed otherwise. |
| 223 | + const perRow = await waitForRows( |
| 224 | + async () => (await userAudit(ql, importedId)).filter((r) => r.action === 'create' && isGeneric(r)), |
| 225 | + (rows) => rows.length === 1, |
| 226 | + "plugin-audit's per-row create row for the imported user", |
| 227 | + ); |
| 228 | + expect(String(perRow[0].new_value)).toContain(email); |
| 229 | + // The import writes NO explicit per-row audit row of its own. |
| 230 | + expect((await userAudit(ql, importedId)).filter((r) => !isGeneric(r))).toHaveLength(0); |
| 231 | + |
| 232 | + // W4 — what the import writes instead: one run-level row per run. |
| 233 | + // `action: 'import'` with `record_id: null` is outside plugin-audit's |
| 234 | + // vocabulary entirely (`actionFor` emits only create/update/delete), so |
| 235 | + // this is complement, not duplication — and it answers a question no |
| 236 | + // per-row ledger can: who ran which import, and what did it do overall. |
| 237 | + const runRows = await waitForRows( |
| 238 | + () => findRows(ql, 'sys_audit_log', { action: 'import' }), |
| 239 | + (rows) => rows.length === 1, |
| 240 | + 'the run-level import row', |
| 241 | + ); |
| 242 | + expect(runRows[0].object_name).toBe('sys_user'); |
| 243 | + expect(runRows[0].record_id).toBeNull(); |
| 244 | + expect(runRows[0].user_id).toBe(adminUserId); |
| 245 | + const metadata = JSON.parse(String(runRows[0].metadata)); |
| 246 | + expect(metadata.event).toBe('user.import_run'); |
| 247 | + expect(metadata).toMatchObject({ total: 1, created: 1, updated: 0, skipped: 0, errors: 0 }); |
| 248 | + expect(metadata.delivery).toMatchObject({ temporary: 1 }); |
| 249 | + }, 120_000); |
| 250 | +}); |
0 commit comments