diff --git a/src/app/admin/audit/page.tsx b/src/app/admin/audit/page.tsx index d5ead1b..c40c969 100644 --- a/src/app/admin/audit/page.tsx +++ b/src/app/admin/audit/page.tsx @@ -682,6 +682,7 @@ export default async function AdminAuditPage({ roleNames, tierLabels, new Map(Object.entries(r.detailAccountNames)), + new Map(Object.entries(r.detailCharacterNames)), )} /> ) : ( diff --git a/src/app/admin/audit/summarize.ts b/src/app/admin/audit/summarize.ts index e19f9c7..e2d9b3f 100644 --- a/src/app/admin/audit/summarize.ts +++ b/src/app/admin/audit/summarize.ts @@ -27,6 +27,7 @@ type Render = ( roleNames: ReadonlyMap, labels: Record, accountNames: ReadonlyMap, + characterNames: ReadonlyMap, ) => string; /** @@ -101,7 +102,9 @@ function scalar(key: string): Part { return part([key], (d) => (d[key] === undefined ? "" : fmt(d[key]))); } -/** A payload value behind a fixed word, e.g. `character 90000001`. */ +/** A payload value behind a fixed word, e.g. `detected by token-health`. For a + * value that is a character id, use `characterRef` — it renders exactly this on + * the miss path but resolves a name when there is one. */ function labelled(word: string, key: string): Part { return part([key], (d) => (d[key] === undefined ? "" : `${word} ${fmt(d[key])}`)); } @@ -164,6 +167,27 @@ function accountRef(word: string, key: string): Part { }); } +/** A character id resolved to its name, e.g. `main Probe Kid`. Unlike + * `accountRef`'s uuid or `roleRef`'s snowflake, a miss degrades to the raw id + * via `fmt`, not to a shortened form: a 9-10 digit character id is already + * legible, so truncating it would destroy information for no gain, and + * rendering it whole keeps the miss path byte-identical to what this column + * showed before names were resolved -- an unresolvable row is no worse than + * it is today. `characterNames` is keyed by the `details` field name, the + * same convention `accountNames` uses -- see `resolveAuditIdentities`'s + * `detailCharacterNames`. The miss path is deliberately `labelled`'s exactly + * -- render on any defined value, nothing only when the key is absent -- so a + * row this cannot resolve reads exactly as it did before, whatever shape the + * unenforced jsonb column turns out to hold. */ +function characterRef(word: string, key: string): Part { + return part([key], (d, _roleNames, _labels, _accountNames, characterNames) => { + const raw = d[key]; + if (raw === undefined) return ""; + const name = characterNames.get(key); + return `${word} ${name ?? fmt(raw)}`; + }); +} + /** A single Discord role id, resolved to its configured tier name where one * exists. The id in `roleId` (role_strip_failed, role_sync_failed) is always * one of `discord.roleIds`' values -- the same set `roleNames` is built from @@ -263,7 +287,7 @@ const PARTS: Record = { "tier.approved": [tierTransition("from", "to"), flag("locked", "locked")], "account.merged": [ shortRef("absorbed", "sourceAccountId"), - labelled("character", "characterId"), + characterRef("character", "characterId"), ], // The only action in the repo whose payload exceeds FALLBACK_KEYS, and the // key the fallback dropped was the price — the reason the row exists. @@ -293,17 +317,22 @@ const PARTS: Record = { // noteChange's doc. "payout.notes_changed": [noteChange("had", "has")], "status.changed": [transition("from", "to"), flag("self", "self-service")], - "admin.bootstrap_granted": [labelled("character", "characterId")], - "account.created": [labelled("main", "mainCharacterId")], - "account.main_changed": [labelled("main →", "mainCharacterId")], + "admin.bootstrap_granted": [characterRef("character", "characterId")], + "account.created": [characterRef("main", "mainCharacterId")], + "account.main_changed": [characterRef("main →", "mainCharacterId")], // Same shape as the member-driven row above; the two are separate actions // only so the log can say who drove it (services/accounts.ts). - "admin.main_changed": [labelled("main →", "mainCharacterId")], + "admin.main_changed": [characterRef("main →", "mainCharacterId")], "character.reclaimed": [accountRef("from", "fromAccount")], "character.unlinked": [scalar("name"), flag("wasMain", "was main")], "token.invalidated": [scalar("reason")], "token.verify_failed": [scalar("error")], - "token.subject_mismatch": [labelled("subject", "subjectCharacterId")], + // The subject is by construction a different character from the row's + // target (jobs/token-health.ts:61 -- the mismatch is the whole point). It + // names whichever character the token's EVE subject belongs to, which this + // app need never have held a row for, so a raw id here is expected rather + // than a sign the lookup is broken. + "token.subject_mismatch": [characterRef("subject", "subjectCharacterId")], // Two writers, two payload shapes. token-health computes a shortfall against // config and sends `missingScopes`; the location job sends the single scope // whose read ESI refused. Each renders nothing for the other's keys, so the @@ -316,10 +345,10 @@ const PARTS: Record = { "tier.unlocked": [tierLabelled("was", "tier")], "status.note_changed": [noteChange("had", "has")], "character.owner_mismatch": [labelled("detected by", "detectedBy")], - "access_list.holder_designated": [labelled("character", "characterId")], + "access_list.holder_designated": [characterRef("character", "characterId")], "access_list.holder_replaced": [ - labelled("character", "characterId"), - labelled("was", "previousCharacterId"), + characterRef("character", "characterId"), + characterRef("was", "previousCharacterId"), ], "access_list.watch_added": [accessListRef("name", "accessListId")], "access_list.watch_removed": [accessListRef("name", "accessListId")], @@ -379,8 +408,10 @@ const FALLBACK_KEYS = 3; * tier value to this deployment's configured label. `accountNames` maps a * `details` field name (not a uuid) to the account it resolved to -- see * `resolveAuditIdentities`'s `detailAccountNames` and `accountRef` above. - * All three passed in rather than imported so this module stays a pure - * function of its arguments and needs no env to test. + * `characterNames` is the same idea for character ids -- see + * `detailCharacterNames` and `characterRef` above. All four passed in rather + * than imported so this module stays a pure function of its arguments and + * needs no env to test. */ export function summarizeDetails( action: string, @@ -388,6 +419,7 @@ export function summarizeDetails( roleNames: ReadonlyMap = new Map(), labels: Record = {}, accountNames: ReadonlyMap = new Map(), + characterNames: ReadonlyMap = new Map(), ): string { const d = (details && typeof details === "object" ? details : {}) as Record< string, @@ -397,7 +429,7 @@ export function summarizeDetails( const parts = PARTS[action]; if (parts) { const rendered = parts - .map((p) => p(d, roleNames, labels, accountNames)) + .map((p) => p(d, roleNames, labels, accountNames, characterNames)) .filter(Boolean); const declared = new Set(parts.flatMap((p) => p.keys)); const hidden = Object.keys(d).filter((k) => !declared.has(k)).length; diff --git a/src/services/audit.ts b/src/services/audit.ts index 4e41b4b..72b41ad 100644 --- a/src/services/audit.ts +++ b/src/services/audit.ts @@ -84,6 +84,13 @@ export type ResolvedAuditRow = typeof auditLog.$inferSelect & { * without knowing which action wrote it. Empty for every row that carries * no such field — see `DETAIL_ACCOUNT_KEYS`. */ detailAccountNames: Record; + /** Character ids embedded in `details`, resolved to that character's name — + * see `DETAIL_CHARACTER_KEYS`. Keyed by the `details` field name rather than + * by the id: `access_list.holder_replaced` carries two character ids on the + * same row (`characterId` and `previousCharacterId`), and an id-keyed map + * could not tell a caller which field either name came from — nor hold both + * when a re-designation of the sitting holder makes the two ids equal. */ + detailCharacterNames: Record; }; const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; @@ -218,6 +225,64 @@ const DETAIL_ACCOUNT_KEYS: Readonly> = { "character.reclaimed": ["fromAccount"], }; +/** + * `details` keys that hold a character id, per action — same idea as + * `DETAIL_ACCOUNT_KEYS`, one join fewer. Resolution is best-effort in both + * directions: a character row is HARD-deleted the moment it is unlinked or + * reclaimed (accounts.ts, both the reclaim and unlink paths), and + * `token.subject_mismatch` below can name a character this app never held a + * row for at all. Either way the renderer falls back to the raw id, same as + * any other unresolved detail. + * + * `access_list.watch_added`/`watch_removed`'s `accessListId` is deliberately + * NOT listed here — it is an EVE access-list id, not a character id, and is + * exactly the false-positive a bare key-name heuristic would produce. The + * `NAMESPACE_TARGET_KIND` comment above flags the same namespace but reaches + * the opposite conclusion, and deliberately: there the collision is tolerated + * because a ~6-digit list id will not match a ~9-10-digit character id in + * practice, and nothing user-facing reads that resolution anyway. Here the + * result is rendered, so the key is excluded outright rather than left to a + * size argument. + * + * `token.subject_mismatch`'s `subjectCharacterId` is, by construction, a + * DIFFERENT character from the row's own target (src/jobs/token-health.ts:61 — + * the mismatch is the whole point): it names whichever character the token's + * EVE subject actually belongs to, which need never have been linked here. + * A raw id on those rows is expected, not a sign the lookup is broken. + */ +const DETAIL_CHARACTER_KEYS: Readonly> = { + "account.created": ["mainCharacterId"], + "account.main_changed": ["mainCharacterId"], + "admin.main_changed": ["mainCharacterId"], + "account.merged": ["characterId"], + "admin.bootstrap_granted": ["characterId"], + "access_list.holder_designated": ["characterId"], + "access_list.holder_replaced": ["characterId", "previousCharacterId"], + "token.subject_mismatch": ["subjectCharacterId"], +}; + +/** + * `jsonb` gives back a genuine number for anything written as one, but the + * column's shape is unenforced by the type system — a hand-inserted or + * legacy row could hold the same id as a digit string instead. Accept either + * and reject everything else, rather than assuming today's writer shape is + * the only one that will ever be read. + * + * The string branch round-trips through `String` rather than trusting + * `Number`, because this path renders a NAME: `"0090000001"` and a 17-digit + * string both coerce to some other character's id, and the summary would then + * confidently name a character the payload never referred to. Failing the + * round-trip degrades to the raw-id render, which is honest. + */ +function detailCharacterId(raw: unknown): number | null { + if (typeof raw === "number") return Number.isInteger(raw) ? raw : null; + if (typeof raw === "string" && DIGITS_RE.test(raw)) { + const id = Number(raw); + return String(id) === raw ? id : null; + } + return null; +} + /** * Resolves actor/target ids to human (main character) names in a fixed, * small number of batched queries, independent of row count: @@ -229,8 +294,8 @@ const DETAIL_ACCOUNT_KEYS: Readonly> = { * 2b. names of deleted payout operations, recovered from the `payout.deleted` * audit row's own details — skipped entirely if step 1 resolved every * payout target already - * 3. every character name needed (target characters + all main characters - * collected above), in one shot + * 3. every character name needed (target characters + character ids read out + * of `details` + all main characters collected above), in one shot * Anything that doesn't resolve is left as `null`/`"unresolved"`; the raw * `actor`/`target` strings on the row are always preserved unchanged. */ @@ -244,6 +309,7 @@ export async function resolveAuditIdentities( const targetCharacterIds = new Set(); const targetDiscordIds = new Set(); const targetPayoutIds = new Set(); + const detailCharacterIds = new Set(); for (const r of rows) { if (r.actor !== "system" && UUID_RE.test(r.actor)) accountIds.add(r.actor); @@ -264,6 +330,14 @@ export async function resolveAuditIdentities( const raw = r.details?.[key]; if (typeof raw === "string" && UUID_RE.test(raw)) accountIds.add(raw); } + // Same idea, one join fewer: a character id living inside `details`. + // Collected separately only because the name lookup needs to know which + // ids came from here; unioned into `characterIds` at step 3 so it shares + // that one query rather than paying for a new one. + for (const key of DETAIL_CHARACTER_KEYS[r.action] ?? []) { + const id = detailCharacterId(r.details?.[key]); + if (id !== null) detailCharacterIds.add(id); + } } const [directAccounts, links, payoutOperations] = await Promise.all([ @@ -332,7 +406,7 @@ export async function resolveAuditIdentities( } } - const characterIds = new Set(targetCharacterIds); + const characterIds = new Set([...targetCharacterIds, ...detailCharacterIds]); for (const a of accountById.values()) { if (a.mainCharacterId !== null) characterIds.add(a.mainCharacterId); } @@ -410,7 +484,22 @@ export async function resolveAuditIdentities( if (name !== null) detailAccountNames[key] = name; } - return { ...r, actorName, actorKind, targetName, targetKind, detailAccountNames }; + const detailCharacterNames: Record = {}; + for (const key of DETAIL_CHARACTER_KEYS[r.action] ?? []) { + const id = detailCharacterId(r.details?.[key]); + const name = id !== null ? (nameByCharacterId.get(id) ?? null) : null; + if (name !== null) detailCharacterNames[key] = name; + } + + return { + ...r, + actorName, + actorKind, + targetName, + targetKind, + detailAccountNames, + detailCharacterNames, + }; }); } diff --git a/tests/audit-resolve.test.ts b/tests/audit-resolve.test.ts index b247f54..774ebcd 100644 --- a/tests/audit-resolve.test.ts +++ b/tests/audit-resolve.test.ts @@ -251,6 +251,111 @@ describe("resolveAuditIdentities / queryAuditLog resolution", () => { expect(row.detailAccountNames).toEqual({}); }); + it("resolves a detail character id (account.main_changed's mainCharacterId)", async () => { + const acc = await seedAccount(ctx.db); + await seedCharacter(ctx.db, cfg, { + id: 90008, + accountId: acc.id, + name: "New Main", + main: true, + }); + await logAudit(ctx.db, { + actor: acc.id, + action: "account.main_changed", + target: acc.id, + details: { mainCharacterId: 90008 }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({ mainCharacterId: "New Main" }); + }); + + it("resolves both characterId and previousCharacterId on a holder_replaced row to distinct names", async () => { + const acc1 = await seedAccount(ctx.db); + const acc2 = await seedAccount(ctx.db); + await seedCharacter(ctx.db, cfg, { + id: 90009, + accountId: acc1.id, + name: "New Holder", + }); + await seedCharacter(ctx.db, cfg, { + id: 90010, + accountId: acc2.id, + name: "Old Holder", + }); + await logAudit(ctx.db, { + actor: "system", + action: "access_list.holder_replaced", + target: "some-list", + details: { characterId: 90009, previousCharacterId: 90010 }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({ + characterId: "New Holder", + previousCharacterId: "Old Holder", + }); + }); + + it("leaves detailCharacterNames empty when the detail character id doesn't exist", async () => { + await logAudit(ctx.db, { + actor: "system", + action: "account.main_changed", + target: "all", + details: { mainCharacterId: 424242 }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({}); + }); + + it("leaves detailCharacterNames empty for actions that don't declare a character-id detail key", async () => { + await logAudit(ctx.db, { + actor: "system", + action: "tier.changed", + target: "all", + details: { from: "member", to: "alumni" }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({}); + }); + + it("resolves a character id the payload carried as a digit string", async () => { + const acc = await seedAccount(ctx.db); + await seedCharacter(ctx.db, cfg, { + id: 90011, + accountId: acc.id, + name: "String Main", + main: true, + }); + await logAudit(ctx.db, { + actor: acc.id, + action: "account.main_changed", + target: acc.id, + details: { mainCharacterId: "90011" }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({ mainCharacterId: "String Main" }); + }); + + it("refuses a digit string that is not this id's own decimal form", async () => { + const acc = await seedAccount(ctx.db); + await seedCharacter(ctx.db, cfg, { + id: 90012, + accountId: acc.id, + name: "Zero Padded", + main: true, + }); + // `Number("0090012")` is 90012, so a naive coercion would render the + // summary as "main → Zero Padded" for a payload that never held that id. + // Naming the wrong character is worse than showing the raw value. + await logAudit(ctx.db, { + actor: acc.id, + action: "account.main_changed", + target: acc.id, + details: { mainCharacterId: "0090012" }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({}); + }); + it("resolves a full page of 200+ rows with a small, constant number of queries (no N+1)", async () => { const accounts = await Promise.all( Array.from({ length: 20 }, () => seedAccount(ctx.db)), diff --git a/tests/audit-summarize.test.ts b/tests/audit-summarize.test.ts index c32fb70..6989bce 100644 --- a/tests/audit-summarize.test.ts +++ b/tests/audit-summarize.test.ts @@ -534,6 +534,131 @@ describe("summarizeDetails with configured tier labels", () => { ).toBe("character 90000002, was 90000001"); }); + it("renders a resolved character name in place of a raw id", () => { + const names = new Map([["mainCharacterId", "Probe Kid"]]); + expect( + summarizeDetails( + "account.main_changed", + { mainCharacterId: 90000001 }, + new Map(), + {}, + new Map(), + names, + ), + ).toBe("main → Probe Kid"); + expect( + summarizeDetails( + "account.created", + { mainCharacterId: 90000001 }, + new Map(), + {}, + new Map(), + names, + ), + ).toBe("main Probe Kid"); + expect( + summarizeDetails( + "account.merged", + { + sourceAccountId: "7f3a2b1c-0000-4000-8000-000000000001", + characterId: 90000001, + }, + new Map(), + {}, + new Map(), + new Map([["characterId", "Probe Kid"]]), + ), + ).toBe("absorbed 7f3a2b…, character Probe Kid"); + expect( + summarizeDetails( + "admin.bootstrap_granted", + { characterId: 90000001 }, + new Map(), + {}, + new Map(), + new Map([["characterId", "Probe Kid"]]), + ), + ).toBe("character Probe Kid"); + expect( + summarizeDetails( + "access_list.holder_designated", + { characterId: 90000001 }, + new Map(), + {}, + new Map(), + new Map([["characterId", "Probe Kid"]]), + ), + ).toBe("character Probe Kid"); + expect( + summarizeDetails( + "token.subject_mismatch", + { subjectCharacterId: 90000003 }, + new Map(), + {}, + new Map(), + new Map([["subjectCharacterId", "Some Alt"]]), + ), + ).toBe("subject Some Alt"); + }); + + it("renders both sides of a holder replacement by their own field-name key", () => { + // The case that proves keying by details field name, not by id: the same + // action names two different characters under two different keys, and + // each must resolve to its own name rather than the other's. + expect( + summarizeDetails( + "access_list.holder_replaced", + { characterId: 90000002, previousCharacterId: 90000001 }, + new Map(), + {}, + new Map(), + new Map([ + ["characterId", "New Holder"], + ["previousCharacterId", "Old Holder"], + ]), + ), + ).toBe("character New Holder, was Old Holder"); + }); + + it("degrades a character id to the raw value when no name resolves", () => { + // No characterNames map at all -- the default parameter, exercised the + // way a caller that hasn't been updated for this field still would be. + // Output must be byte-identical to what this line rendered before names + // were resolved. + expect(summarizeDetails("account.main_changed", { mainCharacterId: 90000001 })).toBe( + "main → 90000001", + ); + }); + + it("does not turn a declared character key into a hidden-key count", () => { + expect( + summarizeDetails( + "access_list.holder_designated", + { characterId: 90000001 }, + new Map(), + {}, + new Map(), + new Map([["characterId", "Probe Kid"]]), + ), + ).not.toContain("more"); + }); + + // The jsonb column's shape is unenforced, so a hand-inserted or legacy row + // could hold something that is not an id at all. Whatever that is, the miss + // path must render it the way this column always has (`labelled` -> `fmt`) + // rather than silently dropping the part and leaving the row blank. + it("falls back like the plain renderer for a value that is not an id", () => { + expect(summarizeDetails("account.main_changed", { mainCharacterId: null })).toBe( + "main → ?", + ); + expect(summarizeDetails("account.created", { mainCharacterId: true })).toBe( + "main true", + ); + // Absent is the one case the part renders nothing for, leaving the row + // with no summary at all rather than a labelled `?`. + expect(summarizeDetails("account.created", {})).toBe("—"); + }); + it("renders a watch change with the list's name, and without it", () => { expect( summarizeDetails("access_list.watch_added", {