Skip to content

Commit fec2f0e

Browse files
os-zhuangclaude
andauthored
feat(audit): write login/logout rows to sys_audit_log and attribute the last_login_at update (#8244)
* feat(audit): write login/logout rows to sys_audit_log and attribute the last_login_at update (#8144) sys_audit_log.action declares `login` and `logout`, the shipped `auth_events` list view filters on them, and two System Overview widgets chart them — but nothing ever wrote either row: the audit writers subscribe to the ObjectQL CRUD lifecycle, so create/update/delete/restore were the only actions that could materialize. The whole trace a sign-in left behind was an unattributed `update sys_user` row (user_id null) diffing last_login_at. - plugin-audit registers the `audit` service — the ledger's ingress for events that are not CRUD. The row shape stays owned by plugin-audit; the caller hands over an EVENT with a closed `login | logout` union, which is the only structural protection available on an object whose action enum nothing validates in either direction (#8203). - plugin-auth emits from better-auth's session lifecycle hooks: session.create.after => login (covers every sign-in method, not just /sign-in/email), session.delete.after under /sign-out => logout. Revokes, bans, erasure and expired-row collection are deliberately NOT logout — they already carry their cause on the ADR-0069 D4 tombstone, and naming them logout would be a wrong record rather than a vague one. - stampLastLogin now carries attributedUserId (#4586), so the last_login_at diff row names the person who signed in. Attributed rather than excluded: the write still authorizes as the system, and suppressing it would delete the last_login_ip trail repo-wide. Neither package depends on the other; a stack without plugin-audit writes no auth rows, exactly as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 * test(dogfood): read the auth_events filter from the running registry (#8144) #8200 retired `permission_change` / `export` from the action enum and narrowed the `auth_events` view in the same PR. A hard-coded copy of the old filter kept querying a value nothing can hold while still reporting success — the view has exactly the shape that hides it, since the login rows alone satisfy the assertion. Read the shipped filter instead, so the test tracks the view. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 * test(plugin-auth): type the audit sink spy so its call log is a real tuple (#8144) `vi.fn(async () => undefined)` types `mock.calls` as `[][]` — a zero-length tuple — so every `calls[0][0]` in this file reached past the end of a tuple the type system believed was empty (TS2493 x3), and the dereference that followed was `possibly undefined` (TS18048 x3). Reading the argument back is the entire point of these cases, so the fix is to declare what the spy receives rather than to soften the read: the spy's implementation now names its parameter, and the call log is pulled through `recordedEvents` / `firstEvent`, which name the "never called" case instead of letting it surface as a TypeError. Pinning the element type to `AuthSessionAuditEventInput` also makes these assertions type-check against the real event surface instead of `any`: a renamed field now fails at compile time rather than quietly comparing `undefined` to `undefined`. One `(c: any[])` map goes away with it. Behaviour unchanged — 1113/1113 plugin-auth tests pass, same 20 cases. Why now: #8225 lowered this package's TEST_DEBT ceiling 131 -> 111 after this branch was cut, so these six errors stopped being slack and became a violation in the merge queue. Measured at 117 against the merged tree, 111 after this commit — exactly the ceiling, ledger untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 6500ebb commit fec2f0e

13 files changed

Lines changed: 1562 additions & 43 deletions
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
---
2+
"@objectstack/plugin-audit": minor
3+
"@objectstack/plugin-auth": minor
4+
---
5+
6+
feat(audit): sign-in and sign-out are recorded in `sys_audit_log`, with the actor and the tenant (#8144)
7+
8+
`sys_audit_log.action` declares `login` and `logout`, the shipped `auth_events`
9+
list view filters on them, and two System Overview dashboard widgets chart them —
10+
but **nothing in the platform ever wrote either row**. The audit writers subscribe
11+
to the ObjectQL CRUD lifecycle, so `create`/`update`/`delete`/`restore` were the
12+
only actions that could ever materialize. On a fresh boot, signing in and then
13+
querying `GET /api/v1/data/sys_audit_log?$filter={"action":"login"}` returned
14+
**total 0**, and the `auth_events` view was empty by construction.
15+
16+
The whole trace a sign-in left behind was one **unattributed** `update sys_user`
17+
row (`user_id` null) diffing `last_login_at` — a compliance ledger recording that
18+
somebody, unknown, had signed in.
19+
20+
Both halves are fixed:
21+
22+
- **`login` on every session creation.** The writer is wired to better-auth's
23+
`session.create` database hook rather than to the `/sign-in/email` endpoint, so
24+
it covers every way a session is minted — email sign-in, sign-up auto-sign-in,
25+
SSO, OAuth callback, magic link, email OTP, passkey. The row carries the actor
26+
(`user_id`), the tenant (`tenant_id` + the RLS `organization_id`), the session
27+
it is about, and the client fingerprint better-auth recorded (IP, user agent).
28+
An impersonation session keeps the subject on `user_id` and names the
29+
impersonating admin on `actor`, so it cannot be misread as a self-service login.
30+
- **`logout` on sign-out.** Scoped to `POST /sign-out` deliberately: a session row
31+
is also deleted by admin revokes, `/revoke-session`, bans, user erasure and
32+
better-auth's own collection of expired rows, and recording any of those as
33+
`logout` would name an action the user never took. Those revocations already
34+
carry their own cause on the ADR-0069 D4 session tombstone.
35+
- **The `last_login_at` write is now attributed.** It goes out through the
36+
platform's existing attribution channel (`ExecutionContext.attributedUserId`),
37+
so the row names the person who signed in. It is attribution only — the write
38+
still authorizes as the system, and nothing about who may touch `sys_user`
39+
changes. The row is kept rather than suppressed: a login from a new address is
40+
exactly what a compliance ledger is read for.
41+
42+
The audit plugin now registers the `audit` service, the ledger's write ingress
43+
for events that are not CRUD. `@objectstack/plugin-auth` resolves it lazily and
44+
takes no dependency on the audit package — a deployment without the audit plugin
45+
installed writes no auth rows, exactly as before.
46+
47+
No API, schema or enum changes: `login`/`logout` were already declared members of
48+
the `action` enum, and `sys_audit_log` remains `get`/`list`-only over HTTP.

packages/plugins/plugin-audit/src/audit-plugin.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { SysAuditLog, SysActivity, SysComment } from './objects/index.js';
1414
// @objectstack/service-storage for the same ownership reason (ADR-0052 §3: a
1515
// file↔record link belongs with storage, not the compliance ledger).
1616
import { installAuditWriters, type AuditI18nSurface, type MessagingEmitSurface } from './audit-writers.js';
17+
import { createAuthEventAuditSink } from './auth-event-audit.js';
1718
import { installCommentAccessHooks, installCommentReadVisibility } from './comment-access-hooks.js';
1819

1920
/**
@@ -30,6 +31,13 @@ export class AuditPlugin implements Plugin {
3031
type = 'standard';
3132
version = '1.0.0';
3233
dependencies = ['com.objectstack.engine.objectql'];
34+
/**
35+
* [#8144] The `audit` slot — the ledger's WRITE ingress for events that are
36+
* not CRUD (`login`/`logout` today). Declared here because `init()` registers
37+
* it unconditionally, which is what ADR-0116 / `plugin-order.ts` reads this
38+
* field to mean.
39+
*/
40+
providesServices = ['audit'];
3341

3442
async init(ctx: PluginContext): Promise<void> {
3543
// Register audit system objects via the manifest service.
@@ -56,6 +64,31 @@ export class AuditPlugin implements Plugin {
5664
],
5765
});
5866

67+
// [#8144] The non-CRUD write ingress. Registered in init() — plugin-auth
68+
// resolves it lazily and calls it from better-auth's session lifecycle
69+
// hooks, i.e. at request time, so the engine is resolved per call rather
70+
// than captured: the service exists from init() while `objectql` only
71+
// resolves at kernel:ready, and every caller arrives long after both.
72+
ctx.registerService(
73+
'audit',
74+
createAuthEventAuditSink({
75+
getEngine: () => {
76+
try {
77+
return ctx.getService<IDataEngine>('objectql');
78+
} catch {
79+
// Same fallback alias `start()` uses below — some kernels register
80+
// the engine as `data`.
81+
try {
82+
return ctx.getService<IDataEngine>('data');
83+
} catch {
84+
return undefined;
85+
}
86+
}
87+
},
88+
logger: ctx.logger,
89+
}),
90+
);
91+
5992
// ADR-0029 D8 — contribute this plugin's object translations to the i18n
6093
// service on kernel:ready (the i18n plugin may register after this one).
6194
if (typeof (ctx as any).hook === 'function') {

packages/plugins/plugin-audit/src/audit-writers.ts

Lines changed: 52 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,54 @@ const NOISE_FIELDS = new Set<string>([
193193
'created_by',
194194
]);
195195

196+
/**
197+
* "Does this object's REGISTERED schema declare this field?", memoized per
198+
* object.
199+
*
200+
* Extracted to module scope (#8144) so the CRUD writer below and the auth-event
201+
* writer (`auth-event-audit.ts`) ask the question ONE way. Both stamp the same
202+
* two conditional columns on the same table, and a second hand-rolled probe
203+
* would answer differently on the day one of them is fixed.
204+
*
205+
* Why the probe exists at all: the SchemaRegistry auto-injects
206+
* `organization_id` only in multi-tenant mode (`applySystemFields({
207+
* multiTenant })`), so on single-tenant stacks the `sys_audit_log` /
208+
* `sys_activity` tables have no such column. Unconditionally stamping it there
209+
* made every audit INSERT fail with "table sys_audit_log has no column named
210+
* organization_id" — and the error was swallowed, so audit logging was silently
211+
* non-functional. Resolve the field set lazily from the engine schema and cache
212+
* it; object schemas are static after registration.
213+
*
214+
* Best-effort in both directions: an engine with no `getSchema` (an in-memory
215+
* test double) reports every field absent, which skips the stamp rather than
216+
* failing the write.
217+
*/
218+
export function createFieldPresenceProbe(
219+
engine: unknown,
220+
): (objectName: string, field: string) => boolean {
221+
const fieldSetCache = new Map<string, Set<string> | null>();
222+
return (objectName: string, field: string): boolean => {
223+
let set = fieldSetCache.get(objectName);
224+
if (set === undefined) {
225+
set = null;
226+
try {
227+
const schema: any =
228+
typeof (engine as any)?.getSchema === 'function' ? (engine as any).getSchema(objectName) : null;
229+
const fields = schema?.fields;
230+
if (fields && typeof fields === 'object' && !Array.isArray(fields)) {
231+
set = new Set<string>(Object.keys(fields));
232+
} else if (Array.isArray(fields)) {
233+
set = new Set<string>(fields.map((f: any) => f?.name).filter(Boolean));
234+
}
235+
} catch {
236+
/* ignore — best-effort; absence just means we skip the stamp */
237+
}
238+
fieldSetCache.set(objectName, set);
239+
}
240+
return set != null && set.has(field);
241+
};
242+
}
243+
196244
/** Action name produced from a HookContext.event string. */
197245
function actionFor(event: string): 'create' | 'update' | 'delete' | null {
198246
if (event === 'afterInsert') return 'create';
@@ -777,36 +825,10 @@ export function installAuditWriters(
777825
engine.unregisterHooksByPackage(packageId);
778826
}
779827

780-
// Whether a given object's *registered* schema declares a field. The
781-
// SchemaRegistry auto-injects `organization_id` only in multi-tenant mode
782-
// (`applySystemFields({ multiTenant })`), so on single-tenant stacks the
783-
// `sys_audit_log` / `sys_activity` tables have no `organization_id` column.
784-
// Unconditionally stamping it there made every audit INSERT fail with
785-
// "table sys_audit_log has no column named organization_id" (the error was
786-
// swallowed, so audit logging was silently non-functional). Resolve the
787-
// field set lazily from the engine schema and cache it — object schemas are
788-
// static after registration.
789-
const fieldSetCache = new Map<string, Set<string> | null>();
790-
const objectHasField = (objectName: string, field: string): boolean => {
791-
let set = fieldSetCache.get(objectName);
792-
if (set === undefined) {
793-
set = null;
794-
try {
795-
const schema: any =
796-
typeof (engine as any).getSchema === 'function' ? (engine as any).getSchema(objectName) : null;
797-
const fields = schema?.fields;
798-
if (fields && typeof fields === 'object' && !Array.isArray(fields)) {
799-
set = new Set<string>(Object.keys(fields));
800-
} else if (Array.isArray(fields)) {
801-
set = new Set<string>(fields.map((f: any) => f?.name).filter(Boolean));
802-
}
803-
} catch {
804-
/* ignore — best-effort; absence just means we skip the stamp */
805-
}
806-
fieldSetCache.set(objectName, set);
807-
}
808-
return set != null && set.has(field);
809-
};
828+
// Whether a given object's *registered* schema declares a field — see
829+
// `createFieldPresenceProbe` for why the conditional stamp exists. Shared
830+
// with the auth-event writer so both stamp on the same answer.
831+
const objectHasField = createFieldPresenceProbe(engine);
810832

811833
// Cached full field-definition map per object (for ADR-0052 §5b trackHistory
812834
// rendering — needs labels/options, not just field names).

0 commit comments

Comments
 (0)