From 4b4e2fe48ff618f0c068f3f8eb4d6da666a0fef0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 14:09:52 +0000 Subject: [PATCH 1/4] feat(auth): thread the real better-auth actor into identity writes for attribution (#4586) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit better-auth owns every write to the identity tables and its ObjectQL adapter runs them `isSystem: true` on purpose — the route already authorized the action under better-auth's own ACL. The human who clicked *make admin* was known exactly once, in the hook layer, then discarded, so every `trackHistory` transition on `sys_member` recorded "system" as its actor. W1 — a general seam, not a `sys_member` special case: a request-scoped attribution store opened at `AuthManager.handleRequest`, filled lazily from better-auth's global before-hook, surfaced as `ExecutionContext.attributedUserId` → `HookContext.provenance.attributedUserId` and read by the audit writer. W2 — `auto-org-admin-grant` stamps the attributed human into the `granted_by` column it always wrote null into, plus a machine-provenance `reason` naming the writer and the triggering `sys_member` row. W3 — covered at the real routes (invite-accept, update-member-role, the reconciler bind, demotion) in a dogfood test over the live HTTP stack. ATTRIBUTION ONLY: the threaded actor never becomes the authorization subject. It rides `provenance`, which no security middleware reads; `isSystem` stays the unconditional authorization half. Re-authorizing as the human would open the second adjudication track ADR-0095 D3 closed — pinned by tests at the engine seam, the adapter, and the live route. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012C2cd7tL8QDoZ2QKN3djJ5 --- content/docs/references/data/data-engine.mdx | 24 +- content/docs/references/data/hook.mdx | 2 +- .../references/kernel/execution-context.mdx | 1 + packages/objectql/src/engine.test.ts | 76 +++++ packages/objectql/src/engine.ts | 15 +- .../plugin-audit/src/audit-writers.test.ts | 57 ++++ .../plugins/plugin-audit/src/audit-writers.ts | 21 +- .../src/auth-actor-attribution.test.ts | 206 ++++++++++++ .../plugin-auth/src/auth-actor-attribution.ts | 173 ++++++++++ .../plugins/plugin-auth/src/auth-manager.ts | 30 +- .../plugins/plugin-auth/src/auth-plugin.ts | 19 +- packages/plugins/plugin-auth/src/index.ts | 6 + .../plugin-auth/src/objectql-adapter.ts | 21 +- .../src/reconcile-membership.test.ts | 35 ++ .../plugin-auth/src/reconcile-membership.ts | 11 +- .../src/auto-org-admin-grant.test.ts | 99 ++++++ .../src/auto-org-admin-grant.ts | 72 ++++- packages/plugins/plugin-security/src/index.ts | 6 + .../plugin-security/src/security-plugin.ts | 17 +- ...mbership-actor-attribution.dogfood.test.ts | 306 ++++++++++++++++++ packages/spec/authorable-surface.json | 1 + packages/spec/src/data/hook.zod.ts | 22 +- .../spec/src/kernel/execution-context.zod.ts | 40 +++ 23 files changed, 1227 insertions(+), 33 deletions(-) create mode 100644 packages/plugins/plugin-auth/src/auth-actor-attribution.test.ts create mode 100644 packages/plugins/plugin-auth/src/auth-actor-attribution.ts create mode 100644 packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts diff --git a/content/docs/references/data/data-engine.mdx b/content/docs/references/data/data-engine.mdx index 6476a8e847..3567cdb162 100644 --- a/content/docs/references/data/data-engine.mdx +++ b/content/docs/references/data/data-engine.mdx @@ -39,7 +39,7 @@ const result = BaseEngineOptionsSchema.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | --- @@ -52,7 +52,7 @@ Options for DataEngine.aggregate operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | | **groupBy** | `string[]` | optional | | | **aggregations** | `{ field: string; method: Enum<'count' \| 'sum' \| 'avg' \| 'min' \| 'max' \| 'count_distinct'>; alias?: string }[]` | optional | | @@ -94,7 +94,7 @@ Options for DataEngine.count operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | @@ -121,7 +121,7 @@ Options for DataEngine.delete operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | | **multi** | `boolean` | optional | | @@ -212,7 +212,7 @@ Options for DataEngine.insert operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **returning** | `boolean` | optional | | @@ -240,7 +240,7 @@ Query options for IDataEngine.find() operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | | **select** | `string[]` | optional | | | **sort** | `Record> \| Record \| { field: string; order: Enum<'asc' \| 'desc'> }[]` | optional | Sort order definition | @@ -428,7 +428,7 @@ Options for DataEngine.update operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | | **upsert** | `boolean` | optional | | | **multi** | `boolean` | optional | | @@ -492,7 +492,7 @@ QueryAST-aligned options for DataEngine.aggregate operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | | **groupBy** | `string[]` | optional | | | **aggregations** | `{ function: Enum<'count' \| 'sum' \| 'avg' \| 'min' \| 'max' \| 'count_distinct' \| 'array_agg' \| 'string_agg'>; field?: string; alias: string; distinct?: boolean; … }[]` | optional | | @@ -510,7 +510,7 @@ QueryAST-aligned options for DataEngine.count operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | @@ -524,7 +524,7 @@ QueryAST-aligned options for DataEngine.delete operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | | **multi** | `boolean` | optional | | @@ -539,7 +539,7 @@ QueryAST-aligned query options for IDataEngine.find() operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | | **fields** | `string[]` | optional | | | **orderBy** | `{ field: string; order: Enum<'asc' \| 'desc'> }[]` | optional | | @@ -563,7 +563,7 @@ QueryAST-aligned options for DataEngine.update operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | | **upsert** | `boolean` | optional | | | **multi** | `boolean` | optional | | diff --git a/content/docs/references/data/hook.mdx b/content/docs/references/data/hook.mdx index 726210e48f..8fa503212b 100644 --- a/content/docs/references/data/hook.mdx +++ b/content/docs/references/data/hook.mdx @@ -38,7 +38,7 @@ const result = HookContextSchema.parse(data); | **result** | `any` | optional | Operation result (After hooks only) | | **previous** | `Record` | optional | Record state before operation | | **session** | `{ userId?: string; actor?: string; organizationId?: string; roles?: string[]; … }` | optional | Current session context | -| **provenance** | `{ flowRunId?: string }` | optional | Server-stamped write provenance (never client-supplied, never an authorization input) | +| **provenance** | `{ flowRunId?: string; attributedUserId?: string }` | optional | Server-stamped write provenance (never client-supplied, never an authorization input) | | **transaction** | `any` | optional | Database transaction handle | | **ql** | `any` | ✅ | ObjectQL Engine Reference | | **api** | `any` | optional | Cross-object data access (ScopedContext) | diff --git a/content/docs/references/kernel/execution-context.mdx b/content/docs/references/kernel/execution-context.mdx index 7db9e05eb4..c6485511ab 100644 --- a/content/docs/references/kernel/execution-context.mdx +++ b/content/docs/references/kernel/execution-context.mdx @@ -49,6 +49,7 @@ const result = ExecutionContextSchema.parse(data); | :--- | :--- | :--- | :--- | | **userId** | `string` | optional | | | **actor** | `string` | optional | | +| **attributedUserId** | `string` | optional | | | **email** | `string` | optional | | | **tenantId** | `string` | optional | | | **timezone** | `string` | optional | | diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index c8960f56e9..b9cafe5c55 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -632,6 +632,82 @@ describe('ObjectQL Engine', () => { }); }); + /** + * #4586 — the ATTRIBUTED human rides provenance, never the session. + * + * better-auth owns every write to the identity tables and runs them + * `isSystem: true` ON PURPOSE: the route already authorized the action + * under better-auth's own ACL. Threading the real human through so + * `sys_member` history stops saying "system" must therefore change exactly + * one thing — who the write is CREDITED to — and nothing about who it is + * AUTHORIZED as. Re-authorizing as the human would open a second + * adjudication track at the boundary ADR-0095 D3 closed. + * + * These are the pins for that constraint at the engine seam, where the + * context is split into the envelopes hooks and middleware actually read. + */ + describe('attributed actor is attribution, never authorization (#4586)', () => { + beforeEach(async () => { + engine.registerDriver(mockDriver, true); + await engine.init(); + vi.mocked(SchemaRegistry.getObject).mockReturnValue({ name: 'task', fields: {} } as any); + }); + + const capture = () => { + const seen: { session?: any; provenance?: any; user?: any } = {}; + engine.registerHook('beforeInsert', async (ctx: any) => { + seen.session = ctx.session; + seen.provenance = ctx.provenance; + seen.user = ctx.user; + }, { object: 'task' }); + return seen; + }; + + it('a better-auth write surfaces the human on provenance and stays a SYSTEM session', async () => { + const seen = capture(); + + await engine.insert('task', { title: 'grade change' }, { + context: { isSystem: true, attributedUserId: 'usr_admin' } as any, + }); + + expect(seen.provenance).toEqual({ attributedUserId: 'usr_admin' }); + // The authorization half is untouched: still system, still no caller. + expect(seen.session).toMatchObject({ isSystem: true }); + expect(seen.session.userId).toBeUndefined(); + // And the attributed human must NOT leak into any channel that a + // hook or middleware reads as "the acting user". + expect(seen.session).not.toHaveProperty('attributedUserId'); + expect(seen.user).toBeUndefined(); + }); + + it('attribution ALONE authorizes exactly like no context at all (ADR-0118 D2)', async () => { + // "Absence is never system": a context that names only who to credit + // establishes no principal, so it must not become one. Anything else + // would make forgetting `isSystem` an accidental elevation. + const seen = capture(); + + await engine.insert('task', { title: 'no authority' }, { + context: { attributedUserId: 'usr_admin' } as any, + }); + + expect(seen.provenance).toEqual({ attributedUserId: 'usr_admin' }); + expect(seen.session).toBeUndefined(); + expect(seen.user).toBeUndefined(); + }); + + it('a real caller keeps their own session; the two envelopes never merge', async () => { + const seen = capture(); + + await engine.insert('task', { title: 'both' }, { + context: { userId: 'u1', attributedUserId: 'usr_admin', flowRunId: 'run_9' } as any, + }); + + expect(seen.session).toMatchObject({ userId: 'u1' }); + expect(seen.user).toMatchObject({ id: 'u1' }); + expect(seen.provenance).toEqual({ flowRunId: 'run_9', attributedUserId: 'usr_admin' }); + }); + }); + describe('execution context via the trailing options arg (read methods)', () => { // Regression: reads took context inside the query while writes took it in // a trailing options arg — so `find(obj, q, { context })` silently dropped diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 60cb801716..5a1658727a 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1181,17 +1181,28 @@ export class ObjectQL implements IObjectQLEngine { } /** - * Build the HookContext.provenance envelope — WHAT produced this write. + * Build the HookContext.provenance envelope — WHERE this write came from. * * Deliberately separate from {@link buildSession}: provenance is server- * stamped, evaluated by no security middleware, and can exist with no * identity beside it. A schedule-triggered flow run resolves no principal * yet still owns its writes, and that is the case the approvals record lock * needs to recognize (#3456 / #3712). + * + * `attributedUserId` rides the SAME envelope for the same reason (#4586): + * a better-auth-originated write authorizes as the system, so the human who + * triggered it must reach the audit writer WITHOUT appearing in `session` — + * where every caller-gating hook would read them as the caller. Attribution + * here, authorization in `session`/`isSystem`, never the two mixed. */ private buildProvenance(execCtx?: ExecutionContextInput): HookContext['provenance'] { const flowRunId = (execCtx as any)?.flowRunId; - return flowRunId ? { flowRunId: String(flowRunId) } : undefined; + const attributedUserId = (execCtx as any)?.attributedUserId; + if (!flowRunId && !attributedUserId) return undefined; + return { + ...(flowRunId ? { flowRunId: String(flowRunId) } : {}), + ...(attributedUserId ? { attributedUserId: String(attributedUserId) } : {}), + }; } /** diff --git a/packages/plugins/plugin-audit/src/audit-writers.test.ts b/packages/plugins/plugin-audit/src/audit-writers.test.ts index 297b3d8b79..aaea8cd36c 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.test.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.test.ts @@ -183,6 +183,63 @@ describe('audit writers — actor attribution (ADR-0014 D2, cloud#340)', () => { expect(audit?.row.actor).toBeNull(); expect(audit?.row.user_id).toBeNull(); }); + + /** + * [#4586] The `sys_member` case the issue is about: better-auth authorizes + * identity writes as the SYSTEM on purpose, so the session names no caller + * and every grade change used to record as "system". The human arrives on + * PROVENANCE instead — attribution, never authorization. + */ + it('credits the attributed human when the write authorized as the system', async () => { + const { engine, fire, created } = makeEngine(SINGLE_TENANT); + installAuditWriters(engine as any, 'test.audit'); + await fire('afterUpdate', { + object: 'sys_member', + input: { id: 'mem-1' }, + __previous: { id: 'mem-1', role: 'member' }, + result: { id: 'mem-1', role: 'admin' }, + // Exactly the envelope `withSystemContext` produces for an + // `organization/update-member-role` call. + session: { isSystem: true }, + provenance: { attributedUserId: 'user-admin' }, + }); + const audit = created.find((c) => c.object === 'sys_audit_log'); + expect(audit?.row.action).toBe('update'); + // WHO changed the grade — a real sys_user id, so the lookup still joins + // (ADR-0118 D1: an id or null, never a sentinel like 'system'). + expect(audit?.row.user_id).toBe('user-admin'); + expect(audit?.row.actor).toBe('user-admin'); + }); + + it('a genuinely machine-originated write still records as the system (null)', async () => { + // Boot sync / migration / the kernel:ready backfill: no scope, no actor. + // Absence must stay absence — never upgraded into some ambient user. + const { engine, fire, created } = makeEngine(SINGLE_TENANT); + installAuditWriters(engine as any, 'test.audit'); + await fire('afterInsert', { + object: 'sys_member', + input: { id: 'mem-2' }, + result: { id: 'mem-2', role: 'member' }, + session: { isSystem: true }, + }); + const audit = created.find((c) => c.object === 'sys_audit_log'); + expect(audit?.row.user_id).toBeNull(); + expect(audit?.row.actor).toBeNull(); + }); + + it('a real caller outranks attribution — the session subject wins', async () => { + const { engine, fire, created } = makeEngine(SINGLE_TENANT); + installAuditWriters(engine as any, 'test.audit'); + await fire('afterInsert', { + object: 'crm_lead', + input: { id: 'lead-3' }, + result: { id: 'lead-3', name: 'Gamma' }, + session: { userId: 'user-7' }, + provenance: { attributedUserId: 'user-admin' }, + }); + const audit = created.find((c) => c.object === 'sys_audit_log'); + expect(audit?.row.user_id).toBe('user-7'); + }); }); describe('audit writers — declarative trackHistory activity (ADR-0052 §5b)', () => { diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index ed4d4d7870..914c11e584 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -476,7 +476,26 @@ export function installAuditWriters( if (recordId !== undefined) recordId = String(recordId); const sess: any = (ctx as any).session ?? {}; - const userId: string | undefined = sess.userId; + // [#4586] Two channels can name a human, and they mean different things: + // + // session.userId — the subject the write was AUTHORIZED as. + // provenance.attributedUserId — the human CREDITED for a write the + // system authorized on their behalf. + // + // The second exists because better-auth owns every identity-table write + // and runs them `isSystem` on purpose (the route already authorized under + // its own ACL), which left every `sys_member` grade change recorded as + // "system". Reading it here is what makes the history row name the admin + // who clicked *make admin*. The session subject still WINS when present — + // attribution never overrides who actually acted — and neither channel + // widens what the write may touch (no security middleware reads + // provenance). + const attributedUserId: string | undefined = + typeof (ctx as any).provenance?.attributedUserId === 'string' && + (ctx as any).provenance.attributedUserId + ? (ctx as any).provenance.attributedUserId + : undefined; + const userId: string | undefined = sess.userId ?? attributedUserId; // Principal label for attribution. Prefer the real user id; otherwise fall // back to a service/automation principal the host put on the context // (`ExecutionContext.actor`, e.g. `svc:`). This is what makes a diff --git a/packages/plugins/plugin-auth/src/auth-actor-attribution.test.ts b/packages/plugins/plugin-auth/src/auth-actor-attribution.test.ts new file mode 100644 index 0000000000..c7e2362ec1 --- /dev/null +++ b/packages/plugins/plugin-auth/src/auth-actor-attribution.test.ts @@ -0,0 +1,206 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4586] The better-auth actor seam. + * + * These cover the seam's own mechanics — scope lifetime, laziness, the + * re-entrancy guard, and the two-part context it builds. The proof that the + * REAL routes reach it (invite-accept, update-member-role, the reconciler bind, + * demotion revoke) lives at the call sites, in + * `packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts` — + * the #3106 lesson: a function that works in isolation is not a seam anyone + * actually crosses. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IDataEngine } from '@objectstack/core'; +import { + runWithAuthActorScope, + setAuthActorResolver, + resolveAttributedUserId, + authSystemWriteContext, +} from './auth-actor-attribution'; +import { withSystemContext } from './objectql-adapter'; + +describe('auth actor attribution scope (#4586)', () => { + it('resolves the actor registered for the current request', async () => { + const seen = await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + return resolveAttributedUserId(); + }); + expect(seen).toBe('usr_admin'); + }); + + it('is LAZY — registering a resolver does not run it', async () => { + const resolver = vi.fn(async () => 'usr_admin'); + await runWithAuthActorScope(async () => { + setAuthActorResolver(resolver); + // A read-only auth request never asks who to credit, so it must not pay + // for a session lookup. This is what keeps the general shape cheap + // enough to leave on for every endpoint. + expect(resolver).not.toHaveBeenCalled(); + }); + expect(resolver).not.toHaveBeenCalled(); + }); + + it('runs the resolver at most once, shared by concurrent writes', async () => { + const resolver = vi.fn(async () => 'usr_admin'); + const results = await runWithAuthActorScope(async () => { + setAuthActorResolver(resolver); + return Promise.all([ + resolveAttributedUserId(), + resolveAttributedUserId(), + resolveAttributedUserId(), + ]); + }); + expect(results).toEqual(['usr_admin', 'usr_admin', 'usr_admin']); + expect(resolver).toHaveBeenCalledTimes(1); + }); + + it('does not deadlock when resolving the session itself writes (re-entrancy)', async () => { + // Real shape: resolving the actor goes back through better-auth, which may + // refresh the session row — a WRITE, which asks who to credit. That inner + // ask must return immediately rather than await the resolution producing + // it. A sibling write started afterwards still gets the answer. + let innerSawActor: string | undefined = 'not-run'; + const scoped = runWithAuthActorScope(async () => { + setAuthActorResolver(async () => { + innerSawActor = await resolveAttributedUserId(); + return 'usr_admin'; + }); + return resolveAttributedUserId(); + }); + await expect(scoped).resolves.toBe('usr_admin'); + expect(innerSawActor).toBeUndefined(); + }); + + it('a failing or empty resolver attributes nothing — never throws', async () => { + const boom = await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => { + throw new Error('session store down'); + }); + return resolveAttributedUserId(); + }); + expect(boom).toBeUndefined(); + + const anonymous = await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => null); + return resolveAttributedUserId(); + }); + expect(anonymous).toBeUndefined(); + }); + + it('outside any request scope there is no actor — absence is the system, not a caller', async () => { + // Programmatic `auth.api.*` calls, boot sync, scheduled jobs. ADR-0118 D1: + // that records as `null`, never as some ambient user. + expect(await resolveAttributedUserId()).toBeUndefined(); + expect(await authSystemWriteContext()).toEqual({ isSystem: true }); + }); + + it('does not leak across sibling requests', async () => { + const [a, b] = await Promise.all([ + runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_a'); + await new Promise((r) => setTimeout(r, 5)); + return resolveAttributedUserId(); + }), + runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_b'); + return resolveAttributedUserId(); + }), + ]); + expect(a).toBe('usr_a'); + expect(b).toBe('usr_b'); + }); + + it('builds a context whose AUTHORIZATION half is system and ATTRIBUTION half is the human', async () => { + const ctx = await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + return authSystemWriteContext(); + }); + // The two halves are separate fields on purpose: `isSystem` decides what + // the write may touch, `attributedUserId` decides only who it is credited + // to. Nothing here may name the human as `userId`. + expect(ctx).toEqual({ isSystem: true, attributedUserId: 'usr_admin' }); + expect(ctx).not.toHaveProperty('userId'); + }); +}); + +describe('withSystemContext carries attribution on WRITES only (#4586)', () => { + const mockEngine = () => + ({ + insert: vi.fn().mockResolvedValue({ id: '1' }), + update: vi.fn().mockResolvedValue({ id: '1' }), + delete: vi.fn().mockResolvedValue(undefined), + find: vi.fn().mockResolvedValue([]), + findOne: vi.fn().mockResolvedValue({ id: '1' }), + count: vi.fn().mockResolvedValue(0), + }) as unknown as IDataEngine; + + it('stamps the attributed human on insert / update / delete, beside isSystem', async () => { + const engine = mockEngine(); + await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + const e = withSystemContext(engine); + await e.insert('sys_member', { id: 'm1' } as any); + await e.update('sys_member', { id: 'm1' } as any); + await e.delete('sys_member', { where: { id: 'm1' } } as any); + }); + + const expected = { context: { attributedUserId: 'usr_admin', isSystem: true } }; + expect(engine.insert).toHaveBeenCalledWith('sys_member', { id: 'm1' }, expected); + expect(engine.update).toHaveBeenCalledWith('sys_member', { id: 'm1' }, expected); + expect(engine.delete).toHaveBeenCalledWith( + 'sys_member', + expect.objectContaining(expected), + ); + }); + + it('never puts the human on `userId` — the write still authorizes as the system', async () => { + const engine = mockEngine(); + await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + await withSystemContext(engine).insert('sys_member', { id: 'm1' } as any); + }); + const ctx = (engine.insert as any).mock.calls[0][2].context; + // The hard constraint of #4586, pinned at the seam that could break it: + // better-auth already authorized this write under its own ACL. Re-running + // it as the human would open a second adjudication track (ADR-0095 D3). + expect(ctx.userId).toBeUndefined(); + expect(ctx.isSystem).toBe(true); + expect(ctx.attributedUserId).toBe('usr_admin'); + }); + + it('READS carry no attribution — a read changes nothing to attribute', async () => { + const engine = mockEngine(); + await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + const e = withSystemContext(engine); + await e.find('sys_member', { where: {} } as any); + await e.findOne('sys_member', { where: {} } as any); + await e.count('sys_member', { where: {} } as any); + }); + expect(engine.find).toHaveBeenCalledWith('sys_member', expect.objectContaining({ context: { isSystem: true } })); + expect(engine.findOne).toHaveBeenCalledWith('sys_member', expect.objectContaining({ context: { isSystem: true } })); + expect(engine.count).toHaveBeenCalledWith('sys_member', expect.objectContaining({ context: { isSystem: true } })); + }); + + it('outside a request scope writes are unchanged — plain isSystem', async () => { + const engine = mockEngine(); + await withSystemContext(engine).insert('sys_member', { id: 'm1' } as any); + expect(engine.insert).toHaveBeenCalledWith('sys_member', { id: 'm1' }, { context: { isSystem: true } }); + }); + + it('an explicit caller-supplied context still wins on every key', async () => { + const engine = mockEngine(); + await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + await withSystemContext(engine).insert('sys_member', { id: 'm1' } as any, { + context: { transaction: 'tx1', attributedUserId: 'usr_explicit' }, + } as any); + }); + expect(engine.insert).toHaveBeenCalledWith('sys_member', { id: 'm1' }, { + context: { attributedUserId: 'usr_explicit', isSystem: true, transaction: 'tx1' }, + }); + }); +}); diff --git a/packages/plugins/plugin-auth/src/auth-actor-attribution.ts b/packages/plugins/plugin-auth/src/auth-actor-attribution.ts new file mode 100644 index 0000000000..5742bf66f1 --- /dev/null +++ b/packages/plugins/plugin-auth/src/auth-actor-attribution.ts @@ -0,0 +1,173 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4586] The better-auth actor seam — carry the real human into the write + * context for ATTRIBUTION, never for authorization. + * + * ## The gap this closes + * + * better-auth is the identity authority: every write to `sys_member`, + * `sys_user`, `sys_invitation` … goes through its routes, and the ObjectQL + * adapter runs them `isSystem: true` **on purpose** (see `withSystemContext` + * in `objectql-adapter.ts` — the route already authorized the action under + * better-auth's own ACL, and ADR-0092 D2 refuses user-context writes to those + * tables outright). `sys_member` is `trackHistory: true`, so its role + * transitions were already recorded — but with "system" as the actor, because + * the human who clicked *make admin* was known exactly once, in the hook layer + * where the session exists, and discarded before the write reached ObjectQL. + * + * This module is that one hop. It is deliberately GENERAL: it attributes every + * better-auth-originated write, not `sys_member` alone, so the next + * better-auth-managed table inherits the fix instead of re-filing the bug. + * + * ## The invariant, and it is the whole point + * + * The threaded actor is **attribution only**. It travels as + * `ExecutionContext.attributedUserId`, which no security middleware reads, and + * it never becomes `ExecutionContext.userId` — the subject the engine + * authorizes AS. Promoting it would re-adjudicate, under the platform's RBAC, + * a decision better-auth already made under its own — the second adjudication + * track ADR-0095 D3 closed. `isSystem: true` stays exactly where it is. + * + * ## Shape + * + * A request-scoped {@link AsyncLocalStorage} store, opened once at the auth + * request boundary (`AuthManager.handleRequest`) and filled LAZILY: + * + * 1. `runWithAuthActorScope` opens an empty scope around the whole request — + * O(1), no I/O, no session lookup; + * 2. better-auth's global `hooks.before` (which runs for EVERY endpoint, + * `matcher: () => true`, including the organization plugin's) hands the + * scope a RESOLVER over its own endpoint ctx — still no I/O; + * 3. a write that actually needs attribution awaits + * {@link resolveAttributedUserId}, which runs the resolver at most once + * per request and memoizes it. + * + * So a read-only auth request pays nothing, and a writing request pays one + * session resolution — the same one better-auth's own `sessionMiddleware` + * memoizes on `ctx.context.session`. + * + * No scope (a programmatic `auth.api.*` call, a boot-time sync, a scheduled + * job) resolves to `undefined`, which records as `null` — the platform's one + * representation for "the system did this" (ADR-0118 D1). Absence is never + * upgraded into a caller. + * + * WebContainer caveat: its `node:async_hooks` does not propagate a store + * across `await` (the same defect `auth-manager.ts` polyfills for better-auth's + * request state). There, attribution degrades to absent — i.e. to today's + * behaviour — which is the safe direction: a lost actor records as the system, + * it never mis-attributes one write to another request's user. + */ + +import { AsyncLocalStorage } from 'node:async_hooks'; + +/** + * Resolves the acting user id for the current auth request, or `null` when no + * session can be established. Supplied by the hook layer, which is the only + * place the session actor exists. + */ +export type AuthActorResolver = () => Promise; + +interface AuthActorScope { + /** Set by the before-hook; run at most once, on first demand. */ + resolver?: AuthActorResolver; + /** Memoized resolution for this request (shared by concurrent writes). */ + pending?: Promise; + /** + * True only INSIDE the resolver's own async subtree. Session resolution goes + * back through better-auth and can itself write (session refresh); that write + * must not await the resolution producing it. A concurrent SIBLING write is + * unaffected — it sees the outer scope and simply awaits `pending`. + */ + suspended?: boolean; +} + +const scopeStore = new AsyncLocalStorage(); + +/** + * Open an attribution scope for one auth request. Everything the request does + * — hooks, endpoint handler, adapter writes — runs inside it. + */ +export function runWithAuthActorScope(fn: () => Promise): Promise { + return scopeStore.run({}, fn); +} + +/** + * Open an attribution scope for an actor that is ALREADY known — the shape a + * framework route takes when it authorized the caller itself and then drives + * better-auth programmatically (`/auth/admin/create-user`, + * `/auth/admin/import-users`: `gateAdmin` resolves the platform admin, then + * `auth.api.createUser` runs server-side, bypassing `handleRequest`). + * + * Without this, those paths would write identity rows — and fire the membership + * reconciler — with no actor at all, recording an admin's deliberate action as + * the system. Same attribution-only semantics: the write still authorizes as + * the system, and this route's own authorization already happened, above. + */ +export function runAttributedToUser( + userId: string | undefined, + fn: () => Promise, +): Promise { + return runWithAuthActorScope(async () => { + if (userId) setAuthActorResolver(async () => userId); + return fn(); + }); +} + +/** + * Hand the current scope a way to resolve the acting user. Cheap and + * idempotent: the resolver is stored, not run. Called from better-auth's + * global before-hook, which has the endpoint ctx the session hangs off. + * + * A no-op outside a scope (programmatic `auth.api.*` calls), and after the + * resolution has already started — every dispatch inside one request resolves + * the same user, so the first resolver wins and re-registration cannot flip + * an in-flight attribution. + */ +export function setAuthActorResolver(resolver: AuthActorResolver): void { + const scope = scopeStore.getStore(); + if (!scope || scope.suspended || scope.pending || scope.resolver) return; + scope.resolver = resolver; +} + +/** + * The acting user id for the current auth request, or `undefined` when there + * is none (no scope, no resolver, no session, or a failed lookup). + * + * Never throws: attribution is observability, and a failure to name the actor + * must never fail the write it was describing. + */ +export async function resolveAttributedUserId(): Promise { + const scope = scopeStore.getStore(); + if (!scope || scope.suspended) return undefined; + if (!scope.pending) { + const resolver = scope.resolver; + if (!resolver) return undefined; + scope.pending = scopeStore.run({ suspended: true }, async () => { + try { + const resolved = await resolver(); + return typeof resolved === 'string' && resolved.length > 0 ? resolved : undefined; + } catch { + return undefined; + } + }); + } + return scope.pending; +} + +/** + * The execution context for a better-auth-owned write: system authorization + * plus — when one is in scope — the human it is attributed to. + * + * `isSystem: true` is the AUTHORIZATION half and is unconditional; + * `attributedUserId` is the ATTRIBUTION half and is purely additive. Use this + * anywhere plugin-auth writes an identity table on better-auth's behalf, so + * the two halves are constructed together and neither can drift into the other. + */ +export async function authSystemWriteContext(): Promise<{ + isSystem: true; + attributedUserId?: string; +}> { + const attributedUserId = await resolveAttributedUserId(); + return attributedUserId ? { isSystem: true, attributedUserId } : { isSystem: true }; +} diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 2a4b68d9f7..146b39f814 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -20,6 +20,7 @@ import { } from '@objectstack/spec'; import { MCP_OAUTH_SCOPES } from '@objectstack/spec/ai'; import { createObjectQLAdapterFactory, withSystemReadContext } from './objectql-adapter.js'; +import { runWithAuthActorScope, setAuthActorResolver } from './auth-actor-attribution.js'; import { invitationRoleCapFailure, isPlainMemberInvitation } from './invitation-role-cap.js'; import { isPlaceholderEmail } from './placeholder-email.js'; import { reconcileMembership, type MembershipPolicy } from './reconcile-membership.js'; @@ -953,6 +954,24 @@ export class AuthManager { // sees `userCount > 0` and the toggle is enforced again. hooks: { before: createAuthMiddleware(async (ctx: any) => { + // ── #4586: hand the attribution scope its actor resolver ───── + // FIRST, and unconditionally: this global before-hook is the one + // seam every better-auth endpoint passes through (`matcher: () => + // true`, plugin routes included), and it is the only layer where + // the session actor exists at all. Registering here is O(1) — the + // resolver is STORED, not run; the session lookup happens only if + // a write later asks who to credit, and at most once per request. + // + // What travels is `attributedUserId`, which no security middleware + // reads. The adapter's writes stay `isSystem` (see + // `withSystemContext`): better-auth already authorized them under + // its own ACL, and re-authorizing as the human would open the + // second adjudication track ADR-0095 D3 closed. + setAuthActorResolver(async () => { + const actor = await this.resolveActor(ctx); + return actor?.userId ?? null; + }); + // ── #2780: per-number OTP send guard (admission control) ───── // MUST run BEFORE the phone-number endpoints: better-auth's // send-otp handler stores a fresh code and only THEN invokes @@ -2673,7 +2692,16 @@ export class AuthManager { // auto-wrap. We establish the ALS store here so all downstream endpoint // calls inherit a valid request-state WeakMap. const { runWithRequestState } = await import('@better-auth/core/context'); - const response = await runWithRequestState(new WeakMap(), () => auth.handler(request)); + // [#4586] Open the actor-attribution scope around the WHOLE request, so + // every write better-auth makes on the way — adapter writes, the + // membership reconciler in `user.create.after`, anything a plugin hook + // triggers — can be credited to the human who made the request. Opening it + // costs nothing: the scope starts empty, the before-hook drops a resolver + // in, and the session is looked up only if some write asks. Attribution + // only — the authorization subject of those writes is unchanged (system). + const response = await runWithAuthActorScope(() => + runWithRequestState(new WeakMap(), () => auth.handler(request)), + ); if (response.status >= 500) { try { diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 0ff943fb0a..9bba9fbf61 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -27,6 +27,7 @@ import { type AuthManagerOptions, } from './auth-manager.js'; import { ensureDefaultOrganization } from './ensure-default-organization.js'; +import { runAttributedToUser } from './auth-actor-attribution.js'; import type { ResolvedSocialProvider } from './backfill-account-issuer.js'; import { createTenancyService, type TenancyService } from './tenancy-service.js'; import { backfillMemberships, type MembershipPolicy } from './reconcile-membership.js'; @@ -1690,7 +1691,17 @@ export class AuthPlugin implements Plugin { ); } const { runAdminCreateUser } = await import('./admin-user-endpoints.js'); - const { status, body } = await runAdminCreateUser(adminUserDeps(), c.req.raw, actor); + // [#4586] This route authorized the admin itself (`gateAdmin`) and + // then drives better-auth SERVER-SIDE, so it never passes through + // `AuthManager.handleRequest` and the request-scoped actor seam is + // not open. Open it here with the actor already in hand, so the + // identity rows this creates — and the membership the reconciler + // binds in `user.create.after` — are credited to the admin instead + // of recorded as the system. Attribution only: the route's own + // authorization already happened above, and the writes stay system. + const { status, body } = await runAttributedToUser(actor.id, () => + runAdminCreateUser(adminUserDeps(), c.req.raw, actor), + ); return c.json(body, status as any); } catch (error) { const err = error instanceof Error ? error : new Error(String(error)); @@ -1735,7 +1746,10 @@ export class AuthPlugin implements Plugin { const metaReader = (() => { try { return ctx.getService?.('protocol'); } catch { return undefined; } })(); - const { status, body } = await runAdminImportUsers( + // [#4586] Same seam as create-user: server-side better-auth calls, + // credited to the admin who ran the import. + const { status, body } = await runAttributedToUser(actor.id, () => + runAdminImportUsers( { getAuthApi: () => this.authManager!.getApi() as any, getDataEngine: () => this.authManager!.getDataEngine(), @@ -1754,6 +1768,7 @@ export class AuthPlugin implements Plugin { }, c.req.raw, actor, + ), ); return c.json(body, status as any); } catch (error) { diff --git a/packages/plugins/plugin-auth/src/index.ts b/packages/plugins/plugin-auth/src/index.ts index 3ca836f65a..354e9b0462 100644 --- a/packages/plugins/plugin-auth/src/index.ts +++ b/packages/plugins/plugin-auth/src/index.ts @@ -25,6 +25,12 @@ export * from './otp-send-guard.js'; export * from './register-sso-provider.js'; export * from './send-verification-email.js'; export * from './objectql-adapter.js'; +// [#4586] The better-auth actor seam. Exported because a host that writes an +// identity table on better-auth's behalf (a control-plane provisioning hook, +// an SSO JIT path) must construct the SAME two-part context — +// `isSystem` for authorization, `attributedUserId` for attribution — rather +// than inventing a second way to say "the system did this, for that person". +export * from './auth-actor-attribution.js'; export * from './auth-schema-config.js'; // ADR-0093 — membership reconciler + tenancy service (public host API: hosts // compose the reconciler into their own hooks; embeddings query tenancy mode). diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index 325a74b8c8..eccd7fe52f 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -4,6 +4,7 @@ import type { IDataEngine } from '@objectstack/core'; import { createAdapterFactory } from 'better-auth/adapters'; import type { CleanedWhere } from 'better-auth/adapters'; import { SystemObjectName } from '@objectstack/spec/system'; +import { resolveAttributedUserId } from './auth-actor-attribution.js'; /** * Mapping from better-auth model names to ObjectStack protocol object names. @@ -241,14 +242,28 @@ export function withValidationErrorMapping>(adapte * writes; user-context writes to `managedBy: 'better-auth'` tables are already * rejected upstream by the identity write guard (ADR-0092 D2), so this path only * ever carries better-auth's internal writes. + * + * WRITES additionally carry `attributedUserId` when an auth request is in scope + * (#4586) — the human whose click better-auth is executing. That is the + * ATTRIBUTION half and nothing more: `isSystem` remains the AUTHORIZATION half, + * unconditionally, so what a write may touch is byte-for-byte what it could + * touch before. Reads never carry it — attribution describes a change, and a + * read changes nothing. */ export function withSystemContext(engine: IDataEngine): IDataEngine { const e = engine as any; const asSystem = (q: any) => ({ ...(q ?? {}), context: { isSystem: true, ...(q?.context ?? {}) } }); + // The attributed human is spread FIRST so an explicit caller-supplied context + // still wins on every key — the same precedence `isSystem` already had. + const asAttributedSystem = async (q: any) => { + const attributedUserId = await resolveAttributedUserId(); + if (!attributedUserId) return asSystem(q); + return { ...(q ?? {}), context: { attributedUserId, isSystem: true, ...(q?.context ?? {}) } }; + }; return { - insert: (m: string, d: any, o?: any) => e.insert(m, d, asSystem(o)), - update: (m: string, d: any, o?: any) => e.update(m, d, asSystem(o)), - delete: (m: string, q?: any) => e.delete(m, asSystem(q)), + insert: async (m: string, d: any, o?: any) => e.insert(m, d, await asAttributedSystem(o)), + update: async (m: string, d: any, o?: any) => e.update(m, d, await asAttributedSystem(o)), + delete: async (m: string, q?: any) => e.delete(m, await asAttributedSystem(q)), find: (m: string, q?: any) => e.find(m, asSystem(q)), findOne: (m: string, q?: any) => e.findOne(m, asSystem(q)), count: (m: string, q?: any) => e.count(m, asSystem(q)), diff --git a/packages/plugins/plugin-auth/src/reconcile-membership.test.ts b/packages/plugins/plugin-auth/src/reconcile-membership.test.ts index 1b3be99cc4..1c491fde17 100644 --- a/packages/plugins/plugin-auth/src/reconcile-membership.test.ts +++ b/packages/plugins/plugin-auth/src/reconcile-membership.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi } from 'vitest'; import { reconcileMembership, backfillMemberships } from './reconcile-membership.js'; +import { runAttributedToUser } from './auth-actor-attribution.js'; /** * In-memory engine over sys_member (+ optional sys_user) with the find/insert @@ -158,3 +159,37 @@ describe('backfillMemberships', () => { expect(res.skipped).toBe(1); }); }); + +/** + * [#4586] The reconciler bind is the third `sys_member` writer (after the + * better-auth adapter and the invite-accept path), and it runs INSIDE + * better-auth's `user.create.after` — so when the creation was an admin + * action, the acting human is in scope and the membership row should name them. + */ +describe('reconcileMembership — actor attribution (#4586)', () => { + it('credits the acting admin when a request scope names one', async () => { + const engine = makeEngine(); + await runAttributedToUser('usr_admin', () => + reconcileMembership(engine, 'user-1', { + policy: 'auto', + resolveTargetOrg: async () => 'org_default', + }), + ); + const [, , options] = engine.insert.mock.calls[0]; + // Both halves, side by side and separate: system AUTHORIZES the write to a + // better-auth-managed table, the admin is merely CREDITED for it. + expect(options).toEqual({ context: { isSystem: true, attributedUserId: 'usr_admin' } }); + }); + + it('a self sign-up has no actor — the bind records as the system', async () => { + // Nobody else acted, and there is no session on the sign-up request. ADR-0118 + // D1: that is `null`, not a fabricated caller. + const engine = makeEngine(); + await reconcileMembership(engine, 'user-2', { + policy: 'auto', + resolveTargetOrg: async () => 'org_default', + }); + const [, , options] = engine.insert.mock.calls[0]; + expect(options).toEqual({ context: { isSystem: true } }); + }); +}); diff --git a/packages/plugins/plugin-auth/src/reconcile-membership.ts b/packages/plugins/plugin-auth/src/reconcile-membership.ts index f86e2ffe5e..12ee0bdce2 100644 --- a/packages/plugins/plugin-auth/src/reconcile-membership.ts +++ b/packages/plugins/plugin-auth/src/reconcile-membership.ts @@ -26,6 +26,8 @@ * kernel:ready backfill is the self-healing net). */ +import { authSystemWriteContext } from './auth-actor-attribution.js'; + export type MembershipPolicy = 'auto' | 'invite-only'; export type ReconcileOutcome = @@ -81,7 +83,14 @@ async function insertMembership(engine: any, organizationId: string, userId: str await engine.insert( 'sys_member', { id: genMemberId(), organization_id: organizationId, user_id: userId, role: 'member' }, - { context: SYSTEM_CTX }, + // [#4586] The bind runs inside better-auth's `user.create.after`, so when + // the creation was an ADMIN action (`/admin/create-user`, bulk import) the + // acting human is in scope and the membership row's history names them + // instead of "system". Self sign-up resolves nothing (no session yet) and + // records as the system — correct, nobody else acted. `isSystem` is + // constructed here regardless: this is a platform write to a + // better-auth-managed table, and attribution never changes that. + { context: await authSystemWriteContext() }, ); } diff --git a/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts b/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts index aa8cd8ed95..6a8f166948 100644 --- a/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts +++ b/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts @@ -4,6 +4,8 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { reconcileOrgAdminGrant, backfillOrgAdminGrants, + AUTO_ORG_ADMIN_GRANT_REASON_PREFIX, + autoOrgAdminGrantReason, } from './auto-org-admin-grant.js'; /** @@ -270,3 +272,100 @@ describe('[ADR-0105 D4] posture selects the org-admin variant', () => { expect(grants.every((g) => g.permission_set_id === 'ps_org_admin_nb')).toBe(true); }); }); + +// --------------------------------------------------------------------------- +// [#4586] Hop 2 of the elevation chain stops discarding provenance. +// +// The chain is `sys_member.role` → this reconciler → `sys_user_permission_set` +// → `isTenantAdmin()`. The grant row had a `granted_by` column and wrote `null` +// into it unconditionally, so "why is X a tenant admin" dead-ended one hop in. +// Now the row records BOTH halves of the answer: the human whose better-auth +// call changed the grade (`granted_by`), and the machine writer + the exact +// membership row that triggered it (`reason`). +// --------------------------------------------------------------------------- +describe('[#4586] the auto-grant records its provenance', () => { + const seed = () => + makeStub({ + sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_member: [{ id: 'mem_42', user_id: 'u1', organization_id: 'o1', role: 'admin' }], + sys_user_permission_set: [], + }); + + it('stamps the attributed human into granted_by', async () => { + const stub = seed(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { ...WALLED, attributedUserId: 'usr_boss' }); + expect(stub.tables.sys_user_permission_set[0].granted_by).toBe('usr_boss'); + }); + + it('names the machine writer and the triggering sys_member row in reason', async () => { + const stub = seed(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { ...WALLED, attributedUserId: 'usr_boss' }); + const reason: string = stub.tables.sys_user_permission_set[0].reason; + // The marker a reader matches on — one constant, not a re-derived string. + expect(reason.startsWith(AUTO_ORG_ADMIN_GRANT_REASON_PREFIX)).toBe(true); + // …and the rest of the chain: which membership, at which grade, to which set. + expect(reason).toContain('mem_42'); + expect(reason).toContain('admin'); + expect(reason).toContain('organization_admin'); + expect(reason).toBe( + autoOrgAdminGrantReason({ id: 'mem_42', role: 'admin' }, 'organization_admin'), + ); + }); + + it('a machine-originated grade change leaves granted_by NULL — never a sentinel', async () => { + // ADR-0118 D1: `granted_by` is a `sys_user` lookup, so the only two legal + // values are a real id and null. The kernel:ready backfill and any boot + // bind have no human, and writing 'system' there would break the join and + // force every reader to special-case it. + const stub = seed(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + const row = stub.tables.sys_user_permission_set[0]; + expect(row.granted_by).toBeNull(); + // The machine provenance is still recorded — in the column that takes text. + expect(row.reason.startsWith(AUTO_ORG_ADMIN_GRANT_REASON_PREFIX)).toBe(true); + }); + + it('attribution changes nothing about WHETHER the grant happens', async () => { + // The threaded human is attribution, not authority: a member-grade row does + // not become grantable because an admin triggered the write. + const stub = makeStub({ + sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_member: [{ id: 'mem_9', user_id: 'u1', organization_id: 'o1', role: 'member' }], + sys_user_permission_set: [], + }); + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', { + ...WALLED, + attributedUserId: 'usr_boss', + }); + expect(res.action).toBe('noop'); + expect(stub.tables.sys_user_permission_set).toHaveLength(0); + }); + + it('demotion still revokes — with or without an attributed actor', async () => { + const stub = seed(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { ...WALLED, attributedUserId: 'usr_boss' }); + expect(stub.tables.sys_user_permission_set).toHaveLength(1); + + stub.tables.sys_member = [ + { id: 'mem_42', user_id: 'u1', organization_id: 'o1', role: 'member' }, + ]; + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', { + ...WALLED, + attributedUserId: 'usr_boss', + }); + expect(res.action).toBe('revoked'); + expect(stub.tables.sys_user_permission_set).toHaveLength(0); + }); + + it('the backfill grants with no human — it is machine-originated by construction', async () => { + const stub = makeStub({ + sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_member: [{ id: 'mem_7', user_id: 'u1', organization_id: 'o1', role: 'owner' }], + sys_user_permission_set: [], + }); + await backfillOrgAdminGrants(stub, WALLED); + const row = stub.tables.sys_user_permission_set[0]; + expect(row.granted_by).toBeNull(); + expect(row.reason).toContain('mem_7'); + }); +}); diff --git a/packages/plugins/plugin-security/src/auto-org-admin-grant.ts b/packages/plugins/plugin-security/src/auto-org-admin-grant.ts index 3ecd73bced..fb5da61260 100644 --- a/packages/plugins/plugin-security/src/auto-org-admin-grant.ts +++ b/packages/plugins/plugin-security/src/auto-org-admin-grant.ts @@ -118,6 +118,48 @@ function isAdminRole(raw: unknown): boolean { return roles.includes('owner') || roles.includes('admin'); } +/** + * [#4586] Machine-provenance marker for rows this module writes. + * + * The auto-grant is the second hop of the elevation chain + * (`sys_member.role` → here → `sys_user_permission_set` → `isTenantAdmin()`), + * and it is the hop with no human in it: no operator ever asked for THIS row, + * a membership grade did. Stamping that fact in a stable, greppable prefix is + * what lets "why is X a tenant admin" be answered from the data — the row says + * which writer minted it and which membership triggered it, so the chain is + * followable instead of inferred. + */ +export const AUTO_ORG_ADMIN_GRANT_REASON_PREFIX = 'auto-org-admin-grant'; + +/** + * The `reason` text for an auto-granted org-admin row: the machine marker, the + * `sys_member` row that triggered it, and the grade that qualified. + * + * Deliberately NOT in `granted_by`: that column is a `sys_user` lookup, and + * ADR-0118 D1 admits exactly two values there — a real user id, or `null` for + * "the system did this". A marker string in a lookup column is the sentinel + * that ADR forbids (it breaks the join and forces every reader to special-case + * it). The repo's own vocabulary already splits these roles — + * `validate-security-posture` states it as "granted_by = writer, + * delegated_from = authority source, reason = why" — so the human goes to + * `granted_by` and the why goes here. + */ +export function autoOrgAdminGrantReason( + member: { id?: unknown; role?: unknown } | undefined, + setName: string, +): string { + const memberId = typeof member?.id === 'string' || typeof member?.id === 'number' + ? String(member.id) + : ''; + const role = parseRoles(member?.role).join(','); + return ( + `${AUTO_ORG_ADMIN_GRANT_REASON_PREFIX}: granted from membership grade` + + (role ? ` '${role}'` : '') + + (memberId ? ` (sys_member ${memberId})` : '') + + ` → ${setName}` + ); +} + /** * Resolve the `sys_permission_set.id` for `organization_admin`. Cached * across calls per ObjectQL instance via a WeakMap so repeated @@ -158,7 +200,21 @@ export async function reconcileOrgAdminGrant( ql: any, userId: string, orgId: string, - options: { logger?: MaybeLogger; posture?: TenancyPosture } = {}, + options: { + logger?: MaybeLogger; + posture?: TenancyPosture; + /** + * [#4586] The human the TRIGGERING `sys_member` write was attributed to + * (`ExecutionContext.attributedUserId`), stamped into `granted_by` so the + * grant names the admin who caused it rather than nobody. Attribution + * only: this reconciler's own writes stay `SYSTEM_CTX`, and nothing here + * authorizes against this value. Absent — the kernel:ready backfill, a + * boot-time bind, any machine-originated grade change — leaves + * `granted_by: null`, the platform's one representation for "the system + * did this" (ADR-0118 D1). + */ + attributedUserId?: string; + } = {}, ): Promise<{ action: 'granted' | 'revoked' | 'noop' | 'skipped'; reason?: string; @@ -195,7 +251,10 @@ export async function reconcileOrgAdminGrant( { user_id: userId, organization_id: orgId }, 10, ); - const shouldGrant = memberships.some((m: any) => isAdminRole(m?.role)); + // The row that QUALIFIES is also the row the grant is provenance-linked to + // (#4586) — "this capability exists because of that membership". + const qualifyingMembership = memberships.find((m: any) => isAdminRole(m?.role)); + const shouldGrant = qualifyingMembership !== undefined; // 1b. [ADR-0105 D4] Revoke the OTHER variant for this pair, always. A posture // change (or a downgrade after F2) must converge on exactly one org-admin @@ -241,7 +300,13 @@ export async function reconcileOrgAdminGrant( user_id: userId, permission_set_id: permSetId, organization_id: orgId, - granted_by: null, + // [#4586] The provenance the row already had a column for. `granted_by` + // is a `sys_user` lookup: the human whose better-auth call triggered the + // grade change when one was in scope, else `null` = the system (ADR-0118 + // D1 — an id or null, never a sentinel). The machine marker and the + // triggering membership row live in `reason`, where free text belongs. + granted_by: options.attributedUserId ?? null, + reason: autoOrgAdminGrantReason(qualifyingMembership, grantSetName), }); if (created) { logger?.info?.('[security] granted org-admin capability', { @@ -249,6 +314,7 @@ export async function reconcileOrgAdminGrant( orgId, set: grantSetName, posture, + grantedBy: options.attributedUserId ?? null, }); return { action: 'granted' }; } diff --git a/packages/plugins/plugin-security/src/index.ts b/packages/plugins/plugin-security/src/index.ts index c5dd223575..e4b097b51e 100644 --- a/packages/plugins/plugin-security/src/index.ts +++ b/packages/plugins/plugin-security/src/index.ts @@ -23,6 +23,12 @@ export { export { reconcileOrgAdminGrant, backfillOrgAdminGrants, + // [#4586] The auto-grant's machine-provenance marker + its `reason` builder. + // Exported so a reader of the grant table (explain surfaces, compliance + // exports, tests) matches the ONE prefix this writer stamps instead of + // re-deriving the string. + AUTO_ORG_ADMIN_GRANT_REASON_PREFIX, + autoOrgAdminGrantReason, } from './auto-org-admin-grant.js'; export { bootstrapPlatformAdmin } from './bootstrap-platform-admin.js'; // [ADR-0105 D8] Scoped-invitation placement — issuance gate + accept-time apply. diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 07e6fdd7ce..4eef31d1f2 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -2090,9 +2090,24 @@ export class SecurityPlugin implements Plugin { return; } const pairs = extractMemberPairs(opCtx); + // [#4586] Carry the triggering write's ATTRIBUTED human into the grant. + // better-auth's membership writes authorize as the system on purpose, so + // `opCtx.context.userId` is empty by construction and `granted_by` was + // always null; `attributedUserId` is the channel that does name the + // admin who clicked. Read as attribution only — it decides nothing about + // whether this reconcile may run, and the reconciler's own writes stay + // system-context. + const attributedUserId = + typeof opCtx?.context?.attributedUserId === 'string' && opCtx.context.attributedUserId + ? opCtx.context.attributedUserId + : undefined; for (const { userId, orgId } of pairs) { try { - await reconcileOrgAdminGrant(ql, userId, orgId, { logger: ctx.logger, posture: this.tenancyPosture }); + await reconcileOrgAdminGrant(ql, userId, orgId, { + logger: ctx.logger, + posture: this.tenancyPosture, + ...(attributedUserId ? { attributedUserId } : {}), + }); } catch (e) { ctx.logger.warn?.('[security] org_admin reconcile failed', { userId, diff --git a/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts b/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts new file mode 100644 index 0000000000..7fe6a701e1 --- /dev/null +++ b/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts @@ -0,0 +1,306 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4586] `sys_member` grade changes keep the true actor — proven at the REAL + * better-auth routes, not against the seam in isolation. + * + * The gap: `sys_member` is `trackHistory: true`, so every role transition was + * already recorded — with "system" as the actor. Every writer is a better-auth + * path, the adapter runs them `isSystem: true` on purpose (the route already + * authorized under better-auth's own ACL, and ADR-0092 D2 refuses user-context + * writes to identity tables), and the human who clicked *make admin* was known + * exactly once, in the hook layer, then discarded. Second hop, same story: + * `auto-org-admin-grant` wrote `granted_by: null` into a column that exists, so + * "why is X a tenant admin" dead-ended one row in. + * + * Why this file exists rather than more unit tests (#3106): the seam's own + * mechanics are covered in `plugin-auth/src/auth-actor-attribution.test.ts`, + * and a seam that works when called directly proves nothing about whether the + * routes cross it. So these drive the actual endpoints — `invite-member` → + * `accept-invitation`, `update-member-role` up and back down — over the real + * HTTP stack, and read the rows the platform actually wrote. + * + * The one constraint the whole change hangs on is pinned here too: the threaded + * actor is ATTRIBUTION. It must never become the write's authorization subject + * — that would put a second adjudication track at exactly the boundary + * ADR-0095 D3 closed — so the promotion is asserted to be refused for a + * caller better-auth does not authorize, no matter who the write would be + * credited to. + * + * Harness note: `bootStack` disables the default-org bootstrap and installs no + * audit plugin, so this file mints the Default Organization itself (system + * context — exactly what the bootstrap would do) and adds `AuditPlugin`, which + * is what turns `trackHistory` into rows. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { AuditPlugin } from '@objectstack/plugin-audit'; +import { AUTO_ORG_ADMIN_GRANT_REASON_PREFIX } from '@objectstack/plugin-security'; + +const SYSTEM_CTX = { isSystem: true }; + +async function findRows(ql: any, object: string, where: any, limit = 50): Promise { + const rows = await ql.find(object, { where, limit }, { context: SYSTEM_CTX }); + return Array.isArray(rows) ? rows : (rows?.records ?? []); +} + +/** The sign-up reconciler's membership row lands asynchronously (better-auth + * defers `user.create.after` past the signup transaction). */ +async function waitForMembership(ql: any, userId: string): Promise { + for (let i = 0; i < 40; i++) { + const rows = await findRows(ql, 'sys_member', { user_id: userId }, 5); + if (rows.length > 0) return rows[0]; + await new Promise((r) => setTimeout(r, 250)); + } + throw new Error(`no sys_member row appeared for ${userId}`); +} + +/** Audit rows this stack wrote for one `sys_member` row, newest last. */ +async function memberHistory(ql: any, memberId: string, action?: string): Promise { + const rows = await findRows( + ql, + 'sys_audit_log', + { object_name: 'sys_member', record_id: memberId }, + 100, + ); + return action ? rows.filter((r: any) => r.action === action) : rows; +} + +async function waitForHistory(ql: any, memberId: string, action: string): Promise { + for (let i = 0; i < 20; i++) { + const rows = await memberHistory(ql, memberId, action); + if (rows.length > 0) return rows[rows.length - 1]; + await new Promise((r) => setTimeout(r, 250)); + } + throw new Error(`no '${action}' history row appeared for sys_member ${memberId}`); +} + +describe('#4586: the better-auth actor reaches sys_member history and the grant', () => { + let stack: VerifyStack; + let ql: any; + let orgId: string; + let partnerOrgId: string; + let adminToken: string; + let adminUserId: string; + let memberToken: string; + let memberUserId: string; + let memberRowId: string; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, { extraPlugins: [new AuditPlugin()] }); + adminToken = await stack.signIn(); // the seeded dev admin + ql = await stack.kernel.getServiceAsync('objectql'); + + const org = await ql.insert( + 'sys_organization', + { name: 'Default Organization', slug: 'default' }, + { context: SYSTEM_CTX }, + ); + orgId = String(org.id); + + // A second org, so invite-accept has somewhere to add a membership that the + // single-org reconciler has not already created. + const partner = await ql.insert( + 'sys_organization', + { name: 'Partner Organization', slug: 'partner' }, + { context: SYSTEM_CTX }, + ); + partnerOrgId = String(partner.id); + + // The dev admin predates the org rows — give them the owner membership the + // single-org bootstrap would have, in both orgs. + const [adminUser] = await findRows(ql, 'sys_user', { email: 'admin@objectos.ai' }, 1); + adminUserId = String(adminUser.id); + const adminMembers = await findRows(ql, 'sys_member', { user_id: adminUserId }, 5); + if (adminMembers.length > 0) { + await ql.update( + 'sys_member', + { id: adminMembers[0].id, organization_id: orgId, role: 'owner' }, + { context: SYSTEM_CTX }, + ); + } else { + await ql.insert( + 'sys_member', + { user_id: adminUserId, organization_id: orgId, role: 'owner' }, + { context: SYSTEM_CTX }, + ); + } + await ql.insert( + 'sys_member', + { user_id: adminUserId, organization_id: partnerOrgId, role: 'owner' }, + { context: SYSTEM_CTX }, + ); + + // The subject: an ordinary member the admin will promote and demote. + memberToken = await stack.signUp('member.4586@example.com', 'Member!Pass123', 'Member 4586'); + const [memberUser] = await findRows(ql, 'sys_user', { email: 'member.4586@example.com' }, 1); + memberUserId = String(memberUser.id); + const bound = await waitForMembership(ql, memberUserId); + memberRowId = String(bound.id); + expect(bound.role).toBe('member'); + }, 180_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + // ── W1: the grade change names the human ──────────────────────────────── + + it('update-member-role: the history row names the admin who promoted, not "system"', async () => { + const res = await stack.apiAs(adminToken, 'POST', '/auth/organization/update-member-role', { + memberId: memberRowId, + role: 'admin', + organizationId: orgId, + }); + expect(res.status, await res.clone().text()).toBe(200); + + const [row] = await findRows(ql, 'sys_member', { id: memberRowId }, 1); + expect(row.role).toBe('admin'); + + const history = await waitForHistory(ql, memberRowId, 'update'); + // Before this change the write reached ObjectQL as bare `{ isSystem: true }` + // and this column was null — the transition was recorded, the actor was not. + expect(history.user_id).toBe(adminUserId); + // A real `sys_user` id, so the lookup still joins (ADR-0118 D1: an id or + // null, never a sentinel string like 'system'). + expect(await findRows(ql, 'sys_user', { id: history.user_id }, 1)).toHaveLength(1); + // The diff itself is unchanged — this adds WHO, it does not alter WHAT. + expect(String(history.new_value)).toContain('admin'); + }, 60_000); + + // ── W2: the capability grant records its whole provenance ─────────────── + + it('the auto-granted capability names the admin in granted_by and the membership in reason', async () => { + // The elevation chain's second hop: sys_member.role → auto-org-admin-grant + // → sys_user_permission_set → isTenantAdmin(). + let grants: any[] = []; + for (let i = 0; i < 20 && grants.length === 0; i++) { + grants = await findRows( + ql, + 'sys_user_permission_set', + { user_id: memberUserId, organization_id: orgId }, + 5, + ); + if (grants.length === 0) await new Promise((r) => setTimeout(r, 250)); + } + expect(grants).toHaveLength(1); + const grant = grants[0]; + + // WHO: the human whose better-auth call caused the elevation. + expect(grant.granted_by).toBe(adminUserId); + // WHY: the machine writer that minted the row, and the membership that + // triggered it — so `explain` can walk the chain instead of inferring it. + expect(String(grant.reason).startsWith(AUTO_ORG_ADMIN_GRANT_REASON_PREFIX)).toBe(true); + expect(String(grant.reason)).toContain(memberRowId); + }, 60_000); + + // ── W1 again, on the OTHER membership-creating route ──────────────────── + + it('accept-invitation: the new membership is attributed to the person who accepted', async () => { + const invite = await stack.apiAs(adminToken, 'POST', '/auth/organization/invite-member', { + email: 'member.4586@example.com', + role: 'member', + organizationId: partnerOrgId, + }); + expect(invite.status, await invite.clone().text()).toBe(200); + const [invitation] = await findRows( + ql, + 'sys_invitation', + { email: 'member.4586@example.com', organization_id: partnerOrgId }, + 1, + ); + expect(invitation).toBeDefined(); + + const accept = await stack.apiAs(memberToken, 'POST', '/auth/organization/accept-invitation', { + invitationId: String(invitation.id), + }); + expect(accept.status, await accept.clone().text()).toBe(200); + + const partnerMembers = await findRows( + ql, + 'sys_member', + { user_id: memberUserId, organization_id: partnerOrgId }, + 5, + ); + expect(partnerMembers).toHaveLength(1); + + // The ACCEPTOR is the actor here, not the inviter — which is the point of + // threading the real session actor rather than hard-coding one route's idea + // of who is responsible. + const created = await waitForHistory(ql, String(partnerMembers[0].id), 'create'); + expect(created.user_id).toBe(memberUserId); + }, 60_000); + + // ── W1 + W2 on the way back down ──────────────────────────────────────── + + it('demotion: the grade change back down is attributed to the admin too', async () => { + const res = await stack.apiAs(adminToken, 'POST', '/auth/organization/update-member-role', { + memberId: memberRowId, + role: 'member', + organizationId: orgId, + }); + expect(res.status, await res.clone().text()).toBe(200); + + const [row] = await findRows(ql, 'sys_member', { id: memberRowId }, 1); + expect(row.role).toBe('member'); + + const updates = await memberHistory(ql, memberRowId, 'update'); + const demotion = updates[updates.length - 1]; + expect(String(demotion.new_value)).toContain('member'); + // Attribution works in both directions — taking authority away is exactly + // as answerable as handing it out. + expect(demotion.user_id).toBe(adminUserId); + + // NOTE: whether the org-admin GRANT is revoked on demotion is deliberately + // not asserted here. It currently is not — `auto-org-admin-grant`'s only + // delete channel calls `ql.delete(object, id, ctx)` while the engine takes + // `(object, { where, context })`, so every revoke throws into a swallowing + // catch. That is a separate, pre-existing defect (its unit stub implements + // the wrong signature, so the suite never saw it) filed as #4640 — a + // security behaviour change that deserves its own review and changeset, not + // a rider on an attribution PR. + }, 60_000); + + // ── The hard constraint ───────────────────────────────────────────────── + + it('attribution is not authority: a plain member cannot promote themselves', async () => { + // The failure mode this change must not create. If the threaded actor had + // become the write's authorization subject, the platform would be + // adjudicating membership writes a second time, beside better-auth's own + // route ACL — the boundary ADR-0095 D3 closed. better-auth still owns the + // decision, and it says no. + const before = (await memberHistory(ql, memberRowId, 'update')).length; + + const res = await stack.apiAs(memberToken, 'POST', '/auth/organization/update-member-role', { + memberId: memberRowId, + role: 'admin', + organizationId: orgId, + }); + expect(res.status).toBeGreaterThanOrEqual(400); + + // Refused at the door: no grade change, and therefore nothing to attribute. + const [row] = await findRows(ql, 'sys_member', { id: memberRowId }, 1); + expect(row.role).toBe('member'); + expect((await memberHistory(ql, memberRowId, 'update')).length).toBe(before); + }, 60_000); + + it('a machine-originated membership write still records as the system', async () => { + // ADR-0118 D1/D2: no actor in scope means `null` — absence is never + // upgraded into a caller, and never written as a sentinel. + const orphan = await ql.insert( + 'sys_user', + { name: 'Orphan 4586', email: 'orphan.4586@example.com' }, + { context: SYSTEM_CTX }, + ); + const row = await ql.insert( + 'sys_member', + { user_id: String(orphan.id), organization_id: partnerOrgId, role: 'member' }, + { context: SYSTEM_CTX }, + ); + const created = await waitForHistory(ql, String(row.id), 'create'); + expect(created.user_id).toBeNull(); + expect(created.actor).toBeNull(); + }, 60_000); +}); diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index 4dda8f322a..3114ffeaf5 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -4464,6 +4464,7 @@ "kernel/ExecutionContext:accessToken", "kernel/ExecutionContext:accessible_org_ids", "kernel/ExecutionContext:actor", + "kernel/ExecutionContext:attributedUserId", "kernel/ExecutionContext:audience", "kernel/ExecutionContext:currency", "kernel/ExecutionContext:email", diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index eba3cae85a..2c1726e4b4 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -380,21 +380,31 @@ export const HookContextSchema = lazySchema(() => z.object({ /** * Write Provenance - * WHAT produced this write, as opposed to WHO is calling ({@link session}). + * WHERE this write came from, as opposed to WHO is calling ({@link session}). * - * Server-stamped and never client-supplied. It is not an identity: nothing - * here is evaluated by any security middleware, and it neither widens nor - * narrows what the write may touch. It exists so a hook can tell "the actor - * that OWNS some externalized state" from "an unrelated caller". + * Server-stamped and never client-supplied. **Nothing here is an + * authorization input**: no security middleware evaluates any of it, and it + * neither widens nor narrows what the write may touch. It exists so a hook — + * or the audit writer — can tell "the run / the person this write belongs + * to" from "an unrelated caller". + * + * Two marks, both non-authorizing, for two different questions: + * - `flowRunId` — WHAT produced the write (a machine origin, no person); + * - `attributedUserId` — WHO is CREDITED for a write the system authorized + * (a person, but never the subject the write was authorized as). * * Kept OUT of `session` on purpose. A writer can have provenance and no * identity at all — a schedule-triggered flow run resolves no principal — and * folding the two together would have forced such a run to present an empty * session, silently turning "no caller" into "an anonymous caller" for every - * hook that gates on `session` being absent (#3712). + * hook that gates on `session` being absent (#3712). The same reasoning + * keeps `attributedUserId` out: it names a human, but the write authorized + * as the SYSTEM, and a hook reading `session.userId` must keep seeing the + * truth — there was no caller (#4586). */ provenance: z.object({ flowRunId: z.string().optional().describe('Id of the automation flow run performing this write, when it originates from a flow data node. Lets a hook recognize the run that OWNS state that run itself opened — the approvals record lock exempts the run holding the pending request (#3456).'), + attributedUserId: z.string().optional().describe('The real human credited for a write whose authorization subject was the SYSTEM — e.g. the admin whose better-auth `update-member-role` call the identity adapter executes as `isSystem` (#4586). ATTRIBUTION ONLY: the audit writer records it as `sys_audit_log.user_id`; no security middleware reads it, and it never becomes the subject the write is authorized as.'), }).optional().describe('Server-stamped write provenance (never client-supplied, never an authorization input)'), diff --git a/packages/spec/src/kernel/execution-context.zod.ts b/packages/spec/src/kernel/execution-context.zod.ts index e7f7af53d3..d0031d2b24 100644 --- a/packages/spec/src/kernel/execution-context.zod.ts +++ b/packages/spec/src/kernel/execution-context.zod.ts @@ -34,6 +34,46 @@ export const ExecutionContextSchema = lazySchema(() => z.object({ */ actor: z.string().optional(), + /** + * [#4586] The real HUMAN behind a write whose authorization subject is the + * SYSTEM — **attribution only, never authorization**. + * + * The case it exists for: better-auth owns every write to the identity + * tables (`sys_member`, `sys_user`, …) and its adapter runs them + * `isSystem: true` **on purpose** — the route already authorized the action + * under better-auth's own ACL (ADR-0092 D2 refuses user-context writes to + * those tables outright). So the person who clicked *make admin* was known + * exactly once, in the hook layer where the session exists, and was then + * discarded: every `sys_member` role transition recorded "system" as its + * actor. This field is the one hop that carries them through. + * + * **The invariant, and it is load-bearing:** nothing in the authorization + * path reads this. It is not {@link userId} — that is the subject the + * engine authorizes AS (RLS `current_user`, ownership stamps, permission + * resolution). Promoting the attributed human to the write's authorization + * subject would re-adjudicate a decision better-auth already made, opening + * the second adjudication track ADR-0095 D3 closed. A context carrying only + * this field authorizes exactly like a context carrying nothing — + * ANONYMOUS, per ADR-0118 D2 ("absence is never system") — and a context + * carrying it beside `isSystem: true` authorizes exactly like `isSystem` + * alone. Attribution and authorization are separate fields on purpose. + * + * Relationship to {@link actor}: same intent (audit attribution), different + * principal. `actor` is a LABEL for a caller that is not a user at all + * (`svc:`) and lands on `sys_audit_log.actor`; this is a real + * `sys_user` id and lands on `sys_audit_log.user_id`, keeping that lookup + * column joinable (ADR-0118 D1 — a user-lookup column holds an id or null, + * never a sentinel). Absent = the write is genuinely machine-originated + * (boot sync, migration, scheduled job) and records as `null`. + * + * Server-constructed only, never client-supplied — exactly like + * {@link isSystem} and {@link flowRunId}. Surfaced to hooks as + * `HookContext.provenance.attributedUserId`, deliberately NOT folded into + * `session`: a hook that gates on `session.userId` must keep seeing "no + * caller" here, because there is none. + */ + attributedUserId: z.string().optional(), + /** * Current user's unique email (resolved from session, falling back to a * `sys_user` lookup). Exposed to RLS as `current_user.email` for seedable, From be6967ef47172c47dead928f196121541ead1385 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 14:09:52 +0000 Subject: [PATCH 2/4] feat(auth): thread the real better-auth actor into identity writes for attribution (#4586) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit better-auth owns every write to the identity tables and its ObjectQL adapter runs them `isSystem: true` on purpose — the route already authorized the action under better-auth's own ACL. The human who clicked *make admin* was known exactly once, in the hook layer, then discarded, so every `trackHistory` transition on `sys_member` recorded "system" as its actor. W1 — a general seam, not a `sys_member` special case: a request-scoped attribution store opened at `AuthManager.handleRequest`, filled lazily from better-auth's global before-hook, surfaced as `ExecutionContext.attributedUserId` → `HookContext.provenance.attributedUserId` and read by the audit writer. W2 — `auto-org-admin-grant` stamps the attributed human into the `granted_by` column it always wrote null into, plus a machine-provenance `reason` naming the writer and the triggering `sys_member` row. W3 — covered at the real routes (invite-accept, update-member-role, the reconciler bind, demotion) in a dogfood test over the live HTTP stack. ATTRIBUTION ONLY: the threaded actor never becomes the authorization subject. It rides `provenance`, which no security middleware reads; `isSystem` stays the unconditional authorization half. Re-authorizing as the human would open the second adjudication track ADR-0095 D3 closed — pinned by tests at the engine seam, the adapter, and the live route. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012C2cd7tL8QDoZ2QKN3djJ5 --- content/docs/references/data/data-engine.mdx | 24 +- content/docs/references/data/hook.mdx | 2 +- .../references/kernel/execution-context.mdx | 1 + packages/objectql/src/engine.test.ts | 76 +++++ packages/objectql/src/engine.ts | 15 +- .../plugin-audit/src/audit-writers.test.ts | 57 ++++ .../plugins/plugin-audit/src/audit-writers.ts | 21 +- .../src/auth-actor-attribution.test.ts | 206 ++++++++++++ .../plugin-auth/src/auth-actor-attribution.ts | 173 ++++++++++ .../plugins/plugin-auth/src/auth-manager.ts | 30 +- .../plugins/plugin-auth/src/auth-plugin.ts | 19 +- packages/plugins/plugin-auth/src/index.ts | 6 + .../plugin-auth/src/objectql-adapter.ts | 21 +- .../src/reconcile-membership.test.ts | 35 ++ .../plugin-auth/src/reconcile-membership.ts | 11 +- .../src/auto-org-admin-grant.test.ts | 99 ++++++ .../src/auto-org-admin-grant.ts | 72 ++++- packages/plugins/plugin-security/src/index.ts | 6 + .../plugin-security/src/security-plugin.ts | 17 +- ...mbership-actor-attribution.dogfood.test.ts | 306 ++++++++++++++++++ packages/spec/authorable-surface.json | 1 + packages/spec/src/data/hook.zod.ts | 22 +- .../spec/src/kernel/execution-context.zod.ts | 40 +++ 23 files changed, 1227 insertions(+), 33 deletions(-) create mode 100644 packages/plugins/plugin-auth/src/auth-actor-attribution.test.ts create mode 100644 packages/plugins/plugin-auth/src/auth-actor-attribution.ts create mode 100644 packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts diff --git a/content/docs/references/data/data-engine.mdx b/content/docs/references/data/data-engine.mdx index 6476a8e847..3567cdb162 100644 --- a/content/docs/references/data/data-engine.mdx +++ b/content/docs/references/data/data-engine.mdx @@ -39,7 +39,7 @@ const result = BaseEngineOptionsSchema.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | --- @@ -52,7 +52,7 @@ Options for DataEngine.aggregate operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | | **groupBy** | `string[]` | optional | | | **aggregations** | `{ field: string; method: Enum<'count' \| 'sum' \| 'avg' \| 'min' \| 'max' \| 'count_distinct'>; alias?: string }[]` | optional | | @@ -94,7 +94,7 @@ Options for DataEngine.count operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | @@ -121,7 +121,7 @@ Options for DataEngine.delete operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | | **multi** | `boolean` | optional | | @@ -212,7 +212,7 @@ Options for DataEngine.insert operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **returning** | `boolean` | optional | | @@ -240,7 +240,7 @@ Query options for IDataEngine.find() operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | | **select** | `string[]` | optional | | | **sort** | `Record> \| Record \| { field: string; order: Enum<'asc' \| 'desc'> }[]` | optional | Sort order definition | @@ -428,7 +428,7 @@ Options for DataEngine.update operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **filter** | `Record \| any` | optional | Data Engine query filter conditions | | **upsert** | `boolean` | optional | | | **multi** | `boolean` | optional | | @@ -492,7 +492,7 @@ QueryAST-aligned options for DataEngine.aggregate operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | | **groupBy** | `string[]` | optional | | | **aggregations** | `{ function: Enum<'count' \| 'sum' \| 'avg' \| 'min' \| 'max' \| 'count_distinct' \| 'array_agg' \| 'string_agg'>; field?: string; alias: string; distinct?: boolean; … }[]` | optional | | @@ -510,7 +510,7 @@ QueryAST-aligned options for DataEngine.count operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | @@ -524,7 +524,7 @@ QueryAST-aligned options for DataEngine.delete operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | | **multi** | `boolean` | optional | | @@ -539,7 +539,7 @@ QueryAST-aligned query options for IDataEngine.find() operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | | **fields** | `string[]` | optional | | | **orderBy** | `{ field: string; order: Enum<'asc' \| 'desc'> }[]` | optional | | @@ -563,7 +563,7 @@ QueryAST-aligned options for DataEngine.update operations | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **context** | `{ userId?: string; actor?: string; email?: string; tenantId?: string; … }` | optional | | +| **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional | | | **where** | `Record \| any` | optional | | | **upsert** | `boolean` | optional | | | **multi** | `boolean` | optional | | diff --git a/content/docs/references/data/hook.mdx b/content/docs/references/data/hook.mdx index 726210e48f..8fa503212b 100644 --- a/content/docs/references/data/hook.mdx +++ b/content/docs/references/data/hook.mdx @@ -38,7 +38,7 @@ const result = HookContextSchema.parse(data); | **result** | `any` | optional | Operation result (After hooks only) | | **previous** | `Record` | optional | Record state before operation | | **session** | `{ userId?: string; actor?: string; organizationId?: string; roles?: string[]; … }` | optional | Current session context | -| **provenance** | `{ flowRunId?: string }` | optional | Server-stamped write provenance (never client-supplied, never an authorization input) | +| **provenance** | `{ flowRunId?: string; attributedUserId?: string }` | optional | Server-stamped write provenance (never client-supplied, never an authorization input) | | **transaction** | `any` | optional | Database transaction handle | | **ql** | `any` | ✅ | ObjectQL Engine Reference | | **api** | `any` | optional | Cross-object data access (ScopedContext) | diff --git a/content/docs/references/kernel/execution-context.mdx b/content/docs/references/kernel/execution-context.mdx index 7db9e05eb4..c6485511ab 100644 --- a/content/docs/references/kernel/execution-context.mdx +++ b/content/docs/references/kernel/execution-context.mdx @@ -49,6 +49,7 @@ const result = ExecutionContextSchema.parse(data); | :--- | :--- | :--- | :--- | | **userId** | `string` | optional | | | **actor** | `string` | optional | | +| **attributedUserId** | `string` | optional | | | **email** | `string` | optional | | | **tenantId** | `string` | optional | | | **timezone** | `string` | optional | | diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index c8960f56e9..b9cafe5c55 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -632,6 +632,82 @@ describe('ObjectQL Engine', () => { }); }); + /** + * #4586 — the ATTRIBUTED human rides provenance, never the session. + * + * better-auth owns every write to the identity tables and runs them + * `isSystem: true` ON PURPOSE: the route already authorized the action + * under better-auth's own ACL. Threading the real human through so + * `sys_member` history stops saying "system" must therefore change exactly + * one thing — who the write is CREDITED to — and nothing about who it is + * AUTHORIZED as. Re-authorizing as the human would open a second + * adjudication track at the boundary ADR-0095 D3 closed. + * + * These are the pins for that constraint at the engine seam, where the + * context is split into the envelopes hooks and middleware actually read. + */ + describe('attributed actor is attribution, never authorization (#4586)', () => { + beforeEach(async () => { + engine.registerDriver(mockDriver, true); + await engine.init(); + vi.mocked(SchemaRegistry.getObject).mockReturnValue({ name: 'task', fields: {} } as any); + }); + + const capture = () => { + const seen: { session?: any; provenance?: any; user?: any } = {}; + engine.registerHook('beforeInsert', async (ctx: any) => { + seen.session = ctx.session; + seen.provenance = ctx.provenance; + seen.user = ctx.user; + }, { object: 'task' }); + return seen; + }; + + it('a better-auth write surfaces the human on provenance and stays a SYSTEM session', async () => { + const seen = capture(); + + await engine.insert('task', { title: 'grade change' }, { + context: { isSystem: true, attributedUserId: 'usr_admin' } as any, + }); + + expect(seen.provenance).toEqual({ attributedUserId: 'usr_admin' }); + // The authorization half is untouched: still system, still no caller. + expect(seen.session).toMatchObject({ isSystem: true }); + expect(seen.session.userId).toBeUndefined(); + // And the attributed human must NOT leak into any channel that a + // hook or middleware reads as "the acting user". + expect(seen.session).not.toHaveProperty('attributedUserId'); + expect(seen.user).toBeUndefined(); + }); + + it('attribution ALONE authorizes exactly like no context at all (ADR-0118 D2)', async () => { + // "Absence is never system": a context that names only who to credit + // establishes no principal, so it must not become one. Anything else + // would make forgetting `isSystem` an accidental elevation. + const seen = capture(); + + await engine.insert('task', { title: 'no authority' }, { + context: { attributedUserId: 'usr_admin' } as any, + }); + + expect(seen.provenance).toEqual({ attributedUserId: 'usr_admin' }); + expect(seen.session).toBeUndefined(); + expect(seen.user).toBeUndefined(); + }); + + it('a real caller keeps their own session; the two envelopes never merge', async () => { + const seen = capture(); + + await engine.insert('task', { title: 'both' }, { + context: { userId: 'u1', attributedUserId: 'usr_admin', flowRunId: 'run_9' } as any, + }); + + expect(seen.session).toMatchObject({ userId: 'u1' }); + expect(seen.user).toMatchObject({ id: 'u1' }); + expect(seen.provenance).toEqual({ flowRunId: 'run_9', attributedUserId: 'usr_admin' }); + }); + }); + describe('execution context via the trailing options arg (read methods)', () => { // Regression: reads took context inside the query while writes took it in // a trailing options arg — so `find(obj, q, { context })` silently dropped diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 60cb801716..5a1658727a 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1181,17 +1181,28 @@ export class ObjectQL implements IObjectQLEngine { } /** - * Build the HookContext.provenance envelope — WHAT produced this write. + * Build the HookContext.provenance envelope — WHERE this write came from. * * Deliberately separate from {@link buildSession}: provenance is server- * stamped, evaluated by no security middleware, and can exist with no * identity beside it. A schedule-triggered flow run resolves no principal * yet still owns its writes, and that is the case the approvals record lock * needs to recognize (#3456 / #3712). + * + * `attributedUserId` rides the SAME envelope for the same reason (#4586): + * a better-auth-originated write authorizes as the system, so the human who + * triggered it must reach the audit writer WITHOUT appearing in `session` — + * where every caller-gating hook would read them as the caller. Attribution + * here, authorization in `session`/`isSystem`, never the two mixed. */ private buildProvenance(execCtx?: ExecutionContextInput): HookContext['provenance'] { const flowRunId = (execCtx as any)?.flowRunId; - return flowRunId ? { flowRunId: String(flowRunId) } : undefined; + const attributedUserId = (execCtx as any)?.attributedUserId; + if (!flowRunId && !attributedUserId) return undefined; + return { + ...(flowRunId ? { flowRunId: String(flowRunId) } : {}), + ...(attributedUserId ? { attributedUserId: String(attributedUserId) } : {}), + }; } /** diff --git a/packages/plugins/plugin-audit/src/audit-writers.test.ts b/packages/plugins/plugin-audit/src/audit-writers.test.ts index 297b3d8b79..aaea8cd36c 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.test.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.test.ts @@ -183,6 +183,63 @@ describe('audit writers — actor attribution (ADR-0014 D2, cloud#340)', () => { expect(audit?.row.actor).toBeNull(); expect(audit?.row.user_id).toBeNull(); }); + + /** + * [#4586] The `sys_member` case the issue is about: better-auth authorizes + * identity writes as the SYSTEM on purpose, so the session names no caller + * and every grade change used to record as "system". The human arrives on + * PROVENANCE instead — attribution, never authorization. + */ + it('credits the attributed human when the write authorized as the system', async () => { + const { engine, fire, created } = makeEngine(SINGLE_TENANT); + installAuditWriters(engine as any, 'test.audit'); + await fire('afterUpdate', { + object: 'sys_member', + input: { id: 'mem-1' }, + __previous: { id: 'mem-1', role: 'member' }, + result: { id: 'mem-1', role: 'admin' }, + // Exactly the envelope `withSystemContext` produces for an + // `organization/update-member-role` call. + session: { isSystem: true }, + provenance: { attributedUserId: 'user-admin' }, + }); + const audit = created.find((c) => c.object === 'sys_audit_log'); + expect(audit?.row.action).toBe('update'); + // WHO changed the grade — a real sys_user id, so the lookup still joins + // (ADR-0118 D1: an id or null, never a sentinel like 'system'). + expect(audit?.row.user_id).toBe('user-admin'); + expect(audit?.row.actor).toBe('user-admin'); + }); + + it('a genuinely machine-originated write still records as the system (null)', async () => { + // Boot sync / migration / the kernel:ready backfill: no scope, no actor. + // Absence must stay absence — never upgraded into some ambient user. + const { engine, fire, created } = makeEngine(SINGLE_TENANT); + installAuditWriters(engine as any, 'test.audit'); + await fire('afterInsert', { + object: 'sys_member', + input: { id: 'mem-2' }, + result: { id: 'mem-2', role: 'member' }, + session: { isSystem: true }, + }); + const audit = created.find((c) => c.object === 'sys_audit_log'); + expect(audit?.row.user_id).toBeNull(); + expect(audit?.row.actor).toBeNull(); + }); + + it('a real caller outranks attribution — the session subject wins', async () => { + const { engine, fire, created } = makeEngine(SINGLE_TENANT); + installAuditWriters(engine as any, 'test.audit'); + await fire('afterInsert', { + object: 'crm_lead', + input: { id: 'lead-3' }, + result: { id: 'lead-3', name: 'Gamma' }, + session: { userId: 'user-7' }, + provenance: { attributedUserId: 'user-admin' }, + }); + const audit = created.find((c) => c.object === 'sys_audit_log'); + expect(audit?.row.user_id).toBe('user-7'); + }); }); describe('audit writers — declarative trackHistory activity (ADR-0052 §5b)', () => { diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index ed4d4d7870..914c11e584 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -476,7 +476,26 @@ export function installAuditWriters( if (recordId !== undefined) recordId = String(recordId); const sess: any = (ctx as any).session ?? {}; - const userId: string | undefined = sess.userId; + // [#4586] Two channels can name a human, and they mean different things: + // + // session.userId — the subject the write was AUTHORIZED as. + // provenance.attributedUserId — the human CREDITED for a write the + // system authorized on their behalf. + // + // The second exists because better-auth owns every identity-table write + // and runs them `isSystem` on purpose (the route already authorized under + // its own ACL), which left every `sys_member` grade change recorded as + // "system". Reading it here is what makes the history row name the admin + // who clicked *make admin*. The session subject still WINS when present — + // attribution never overrides who actually acted — and neither channel + // widens what the write may touch (no security middleware reads + // provenance). + const attributedUserId: string | undefined = + typeof (ctx as any).provenance?.attributedUserId === 'string' && + (ctx as any).provenance.attributedUserId + ? (ctx as any).provenance.attributedUserId + : undefined; + const userId: string | undefined = sess.userId ?? attributedUserId; // Principal label for attribution. Prefer the real user id; otherwise fall // back to a service/automation principal the host put on the context // (`ExecutionContext.actor`, e.g. `svc:`). This is what makes a diff --git a/packages/plugins/plugin-auth/src/auth-actor-attribution.test.ts b/packages/plugins/plugin-auth/src/auth-actor-attribution.test.ts new file mode 100644 index 0000000000..c7e2362ec1 --- /dev/null +++ b/packages/plugins/plugin-auth/src/auth-actor-attribution.test.ts @@ -0,0 +1,206 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4586] The better-auth actor seam. + * + * These cover the seam's own mechanics — scope lifetime, laziness, the + * re-entrancy guard, and the two-part context it builds. The proof that the + * REAL routes reach it (invite-accept, update-member-role, the reconciler bind, + * demotion revoke) lives at the call sites, in + * `packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts` — + * the #3106 lesson: a function that works in isolation is not a seam anyone + * actually crosses. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IDataEngine } from '@objectstack/core'; +import { + runWithAuthActorScope, + setAuthActorResolver, + resolveAttributedUserId, + authSystemWriteContext, +} from './auth-actor-attribution'; +import { withSystemContext } from './objectql-adapter'; + +describe('auth actor attribution scope (#4586)', () => { + it('resolves the actor registered for the current request', async () => { + const seen = await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + return resolveAttributedUserId(); + }); + expect(seen).toBe('usr_admin'); + }); + + it('is LAZY — registering a resolver does not run it', async () => { + const resolver = vi.fn(async () => 'usr_admin'); + await runWithAuthActorScope(async () => { + setAuthActorResolver(resolver); + // A read-only auth request never asks who to credit, so it must not pay + // for a session lookup. This is what keeps the general shape cheap + // enough to leave on for every endpoint. + expect(resolver).not.toHaveBeenCalled(); + }); + expect(resolver).not.toHaveBeenCalled(); + }); + + it('runs the resolver at most once, shared by concurrent writes', async () => { + const resolver = vi.fn(async () => 'usr_admin'); + const results = await runWithAuthActorScope(async () => { + setAuthActorResolver(resolver); + return Promise.all([ + resolveAttributedUserId(), + resolveAttributedUserId(), + resolveAttributedUserId(), + ]); + }); + expect(results).toEqual(['usr_admin', 'usr_admin', 'usr_admin']); + expect(resolver).toHaveBeenCalledTimes(1); + }); + + it('does not deadlock when resolving the session itself writes (re-entrancy)', async () => { + // Real shape: resolving the actor goes back through better-auth, which may + // refresh the session row — a WRITE, which asks who to credit. That inner + // ask must return immediately rather than await the resolution producing + // it. A sibling write started afterwards still gets the answer. + let innerSawActor: string | undefined = 'not-run'; + const scoped = runWithAuthActorScope(async () => { + setAuthActorResolver(async () => { + innerSawActor = await resolveAttributedUserId(); + return 'usr_admin'; + }); + return resolveAttributedUserId(); + }); + await expect(scoped).resolves.toBe('usr_admin'); + expect(innerSawActor).toBeUndefined(); + }); + + it('a failing or empty resolver attributes nothing — never throws', async () => { + const boom = await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => { + throw new Error('session store down'); + }); + return resolveAttributedUserId(); + }); + expect(boom).toBeUndefined(); + + const anonymous = await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => null); + return resolveAttributedUserId(); + }); + expect(anonymous).toBeUndefined(); + }); + + it('outside any request scope there is no actor — absence is the system, not a caller', async () => { + // Programmatic `auth.api.*` calls, boot sync, scheduled jobs. ADR-0118 D1: + // that records as `null`, never as some ambient user. + expect(await resolveAttributedUserId()).toBeUndefined(); + expect(await authSystemWriteContext()).toEqual({ isSystem: true }); + }); + + it('does not leak across sibling requests', async () => { + const [a, b] = await Promise.all([ + runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_a'); + await new Promise((r) => setTimeout(r, 5)); + return resolveAttributedUserId(); + }), + runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_b'); + return resolveAttributedUserId(); + }), + ]); + expect(a).toBe('usr_a'); + expect(b).toBe('usr_b'); + }); + + it('builds a context whose AUTHORIZATION half is system and ATTRIBUTION half is the human', async () => { + const ctx = await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + return authSystemWriteContext(); + }); + // The two halves are separate fields on purpose: `isSystem` decides what + // the write may touch, `attributedUserId` decides only who it is credited + // to. Nothing here may name the human as `userId`. + expect(ctx).toEqual({ isSystem: true, attributedUserId: 'usr_admin' }); + expect(ctx).not.toHaveProperty('userId'); + }); +}); + +describe('withSystemContext carries attribution on WRITES only (#4586)', () => { + const mockEngine = () => + ({ + insert: vi.fn().mockResolvedValue({ id: '1' }), + update: vi.fn().mockResolvedValue({ id: '1' }), + delete: vi.fn().mockResolvedValue(undefined), + find: vi.fn().mockResolvedValue([]), + findOne: vi.fn().mockResolvedValue({ id: '1' }), + count: vi.fn().mockResolvedValue(0), + }) as unknown as IDataEngine; + + it('stamps the attributed human on insert / update / delete, beside isSystem', async () => { + const engine = mockEngine(); + await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + const e = withSystemContext(engine); + await e.insert('sys_member', { id: 'm1' } as any); + await e.update('sys_member', { id: 'm1' } as any); + await e.delete('sys_member', { where: { id: 'm1' } } as any); + }); + + const expected = { context: { attributedUserId: 'usr_admin', isSystem: true } }; + expect(engine.insert).toHaveBeenCalledWith('sys_member', { id: 'm1' }, expected); + expect(engine.update).toHaveBeenCalledWith('sys_member', { id: 'm1' }, expected); + expect(engine.delete).toHaveBeenCalledWith( + 'sys_member', + expect.objectContaining(expected), + ); + }); + + it('never puts the human on `userId` — the write still authorizes as the system', async () => { + const engine = mockEngine(); + await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + await withSystemContext(engine).insert('sys_member', { id: 'm1' } as any); + }); + const ctx = (engine.insert as any).mock.calls[0][2].context; + // The hard constraint of #4586, pinned at the seam that could break it: + // better-auth already authorized this write under its own ACL. Re-running + // it as the human would open a second adjudication track (ADR-0095 D3). + expect(ctx.userId).toBeUndefined(); + expect(ctx.isSystem).toBe(true); + expect(ctx.attributedUserId).toBe('usr_admin'); + }); + + it('READS carry no attribution — a read changes nothing to attribute', async () => { + const engine = mockEngine(); + await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + const e = withSystemContext(engine); + await e.find('sys_member', { where: {} } as any); + await e.findOne('sys_member', { where: {} } as any); + await e.count('sys_member', { where: {} } as any); + }); + expect(engine.find).toHaveBeenCalledWith('sys_member', expect.objectContaining({ context: { isSystem: true } })); + expect(engine.findOne).toHaveBeenCalledWith('sys_member', expect.objectContaining({ context: { isSystem: true } })); + expect(engine.count).toHaveBeenCalledWith('sys_member', expect.objectContaining({ context: { isSystem: true } })); + }); + + it('outside a request scope writes are unchanged — plain isSystem', async () => { + const engine = mockEngine(); + await withSystemContext(engine).insert('sys_member', { id: 'm1' } as any); + expect(engine.insert).toHaveBeenCalledWith('sys_member', { id: 'm1' }, { context: { isSystem: true } }); + }); + + it('an explicit caller-supplied context still wins on every key', async () => { + const engine = mockEngine(); + await runWithAuthActorScope(async () => { + setAuthActorResolver(async () => 'usr_admin'); + await withSystemContext(engine).insert('sys_member', { id: 'm1' } as any, { + context: { transaction: 'tx1', attributedUserId: 'usr_explicit' }, + } as any); + }); + expect(engine.insert).toHaveBeenCalledWith('sys_member', { id: 'm1' }, { + context: { attributedUserId: 'usr_explicit', isSystem: true, transaction: 'tx1' }, + }); + }); +}); diff --git a/packages/plugins/plugin-auth/src/auth-actor-attribution.ts b/packages/plugins/plugin-auth/src/auth-actor-attribution.ts new file mode 100644 index 0000000000..5742bf66f1 --- /dev/null +++ b/packages/plugins/plugin-auth/src/auth-actor-attribution.ts @@ -0,0 +1,173 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4586] The better-auth actor seam — carry the real human into the write + * context for ATTRIBUTION, never for authorization. + * + * ## The gap this closes + * + * better-auth is the identity authority: every write to `sys_member`, + * `sys_user`, `sys_invitation` … goes through its routes, and the ObjectQL + * adapter runs them `isSystem: true` **on purpose** (see `withSystemContext` + * in `objectql-adapter.ts` — the route already authorized the action under + * better-auth's own ACL, and ADR-0092 D2 refuses user-context writes to those + * tables outright). `sys_member` is `trackHistory: true`, so its role + * transitions were already recorded — but with "system" as the actor, because + * the human who clicked *make admin* was known exactly once, in the hook layer + * where the session exists, and discarded before the write reached ObjectQL. + * + * This module is that one hop. It is deliberately GENERAL: it attributes every + * better-auth-originated write, not `sys_member` alone, so the next + * better-auth-managed table inherits the fix instead of re-filing the bug. + * + * ## The invariant, and it is the whole point + * + * The threaded actor is **attribution only**. It travels as + * `ExecutionContext.attributedUserId`, which no security middleware reads, and + * it never becomes `ExecutionContext.userId` — the subject the engine + * authorizes AS. Promoting it would re-adjudicate, under the platform's RBAC, + * a decision better-auth already made under its own — the second adjudication + * track ADR-0095 D3 closed. `isSystem: true` stays exactly where it is. + * + * ## Shape + * + * A request-scoped {@link AsyncLocalStorage} store, opened once at the auth + * request boundary (`AuthManager.handleRequest`) and filled LAZILY: + * + * 1. `runWithAuthActorScope` opens an empty scope around the whole request — + * O(1), no I/O, no session lookup; + * 2. better-auth's global `hooks.before` (which runs for EVERY endpoint, + * `matcher: () => true`, including the organization plugin's) hands the + * scope a RESOLVER over its own endpoint ctx — still no I/O; + * 3. a write that actually needs attribution awaits + * {@link resolveAttributedUserId}, which runs the resolver at most once + * per request and memoizes it. + * + * So a read-only auth request pays nothing, and a writing request pays one + * session resolution — the same one better-auth's own `sessionMiddleware` + * memoizes on `ctx.context.session`. + * + * No scope (a programmatic `auth.api.*` call, a boot-time sync, a scheduled + * job) resolves to `undefined`, which records as `null` — the platform's one + * representation for "the system did this" (ADR-0118 D1). Absence is never + * upgraded into a caller. + * + * WebContainer caveat: its `node:async_hooks` does not propagate a store + * across `await` (the same defect `auth-manager.ts` polyfills for better-auth's + * request state). There, attribution degrades to absent — i.e. to today's + * behaviour — which is the safe direction: a lost actor records as the system, + * it never mis-attributes one write to another request's user. + */ + +import { AsyncLocalStorage } from 'node:async_hooks'; + +/** + * Resolves the acting user id for the current auth request, or `null` when no + * session can be established. Supplied by the hook layer, which is the only + * place the session actor exists. + */ +export type AuthActorResolver = () => Promise; + +interface AuthActorScope { + /** Set by the before-hook; run at most once, on first demand. */ + resolver?: AuthActorResolver; + /** Memoized resolution for this request (shared by concurrent writes). */ + pending?: Promise; + /** + * True only INSIDE the resolver's own async subtree. Session resolution goes + * back through better-auth and can itself write (session refresh); that write + * must not await the resolution producing it. A concurrent SIBLING write is + * unaffected — it sees the outer scope and simply awaits `pending`. + */ + suspended?: boolean; +} + +const scopeStore = new AsyncLocalStorage(); + +/** + * Open an attribution scope for one auth request. Everything the request does + * — hooks, endpoint handler, adapter writes — runs inside it. + */ +export function runWithAuthActorScope(fn: () => Promise): Promise { + return scopeStore.run({}, fn); +} + +/** + * Open an attribution scope for an actor that is ALREADY known — the shape a + * framework route takes when it authorized the caller itself and then drives + * better-auth programmatically (`/auth/admin/create-user`, + * `/auth/admin/import-users`: `gateAdmin` resolves the platform admin, then + * `auth.api.createUser` runs server-side, bypassing `handleRequest`). + * + * Without this, those paths would write identity rows — and fire the membership + * reconciler — with no actor at all, recording an admin's deliberate action as + * the system. Same attribution-only semantics: the write still authorizes as + * the system, and this route's own authorization already happened, above. + */ +export function runAttributedToUser( + userId: string | undefined, + fn: () => Promise, +): Promise { + return runWithAuthActorScope(async () => { + if (userId) setAuthActorResolver(async () => userId); + return fn(); + }); +} + +/** + * Hand the current scope a way to resolve the acting user. Cheap and + * idempotent: the resolver is stored, not run. Called from better-auth's + * global before-hook, which has the endpoint ctx the session hangs off. + * + * A no-op outside a scope (programmatic `auth.api.*` calls), and after the + * resolution has already started — every dispatch inside one request resolves + * the same user, so the first resolver wins and re-registration cannot flip + * an in-flight attribution. + */ +export function setAuthActorResolver(resolver: AuthActorResolver): void { + const scope = scopeStore.getStore(); + if (!scope || scope.suspended || scope.pending || scope.resolver) return; + scope.resolver = resolver; +} + +/** + * The acting user id for the current auth request, or `undefined` when there + * is none (no scope, no resolver, no session, or a failed lookup). + * + * Never throws: attribution is observability, and a failure to name the actor + * must never fail the write it was describing. + */ +export async function resolveAttributedUserId(): Promise { + const scope = scopeStore.getStore(); + if (!scope || scope.suspended) return undefined; + if (!scope.pending) { + const resolver = scope.resolver; + if (!resolver) return undefined; + scope.pending = scopeStore.run({ suspended: true }, async () => { + try { + const resolved = await resolver(); + return typeof resolved === 'string' && resolved.length > 0 ? resolved : undefined; + } catch { + return undefined; + } + }); + } + return scope.pending; +} + +/** + * The execution context for a better-auth-owned write: system authorization + * plus — when one is in scope — the human it is attributed to. + * + * `isSystem: true` is the AUTHORIZATION half and is unconditional; + * `attributedUserId` is the ATTRIBUTION half and is purely additive. Use this + * anywhere plugin-auth writes an identity table on better-auth's behalf, so + * the two halves are constructed together and neither can drift into the other. + */ +export async function authSystemWriteContext(): Promise<{ + isSystem: true; + attributedUserId?: string; +}> { + const attributedUserId = await resolveAttributedUserId(); + return attributedUserId ? { isSystem: true, attributedUserId } : { isSystem: true }; +} diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 2a4b68d9f7..146b39f814 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -20,6 +20,7 @@ import { } from '@objectstack/spec'; import { MCP_OAUTH_SCOPES } from '@objectstack/spec/ai'; import { createObjectQLAdapterFactory, withSystemReadContext } from './objectql-adapter.js'; +import { runWithAuthActorScope, setAuthActorResolver } from './auth-actor-attribution.js'; import { invitationRoleCapFailure, isPlainMemberInvitation } from './invitation-role-cap.js'; import { isPlaceholderEmail } from './placeholder-email.js'; import { reconcileMembership, type MembershipPolicy } from './reconcile-membership.js'; @@ -953,6 +954,24 @@ export class AuthManager { // sees `userCount > 0` and the toggle is enforced again. hooks: { before: createAuthMiddleware(async (ctx: any) => { + // ── #4586: hand the attribution scope its actor resolver ───── + // FIRST, and unconditionally: this global before-hook is the one + // seam every better-auth endpoint passes through (`matcher: () => + // true`, plugin routes included), and it is the only layer where + // the session actor exists at all. Registering here is O(1) — the + // resolver is STORED, not run; the session lookup happens only if + // a write later asks who to credit, and at most once per request. + // + // What travels is `attributedUserId`, which no security middleware + // reads. The adapter's writes stay `isSystem` (see + // `withSystemContext`): better-auth already authorized them under + // its own ACL, and re-authorizing as the human would open the + // second adjudication track ADR-0095 D3 closed. + setAuthActorResolver(async () => { + const actor = await this.resolveActor(ctx); + return actor?.userId ?? null; + }); + // ── #2780: per-number OTP send guard (admission control) ───── // MUST run BEFORE the phone-number endpoints: better-auth's // send-otp handler stores a fresh code and only THEN invokes @@ -2673,7 +2692,16 @@ export class AuthManager { // auto-wrap. We establish the ALS store here so all downstream endpoint // calls inherit a valid request-state WeakMap. const { runWithRequestState } = await import('@better-auth/core/context'); - const response = await runWithRequestState(new WeakMap(), () => auth.handler(request)); + // [#4586] Open the actor-attribution scope around the WHOLE request, so + // every write better-auth makes on the way — adapter writes, the + // membership reconciler in `user.create.after`, anything a plugin hook + // triggers — can be credited to the human who made the request. Opening it + // costs nothing: the scope starts empty, the before-hook drops a resolver + // in, and the session is looked up only if some write asks. Attribution + // only — the authorization subject of those writes is unchanged (system). + const response = await runWithAuthActorScope(() => + runWithRequestState(new WeakMap(), () => auth.handler(request)), + ); if (response.status >= 500) { try { diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 0ff943fb0a..9bba9fbf61 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -27,6 +27,7 @@ import { type AuthManagerOptions, } from './auth-manager.js'; import { ensureDefaultOrganization } from './ensure-default-organization.js'; +import { runAttributedToUser } from './auth-actor-attribution.js'; import type { ResolvedSocialProvider } from './backfill-account-issuer.js'; import { createTenancyService, type TenancyService } from './tenancy-service.js'; import { backfillMemberships, type MembershipPolicy } from './reconcile-membership.js'; @@ -1690,7 +1691,17 @@ export class AuthPlugin implements Plugin { ); } const { runAdminCreateUser } = await import('./admin-user-endpoints.js'); - const { status, body } = await runAdminCreateUser(adminUserDeps(), c.req.raw, actor); + // [#4586] This route authorized the admin itself (`gateAdmin`) and + // then drives better-auth SERVER-SIDE, so it never passes through + // `AuthManager.handleRequest` and the request-scoped actor seam is + // not open. Open it here with the actor already in hand, so the + // identity rows this creates — and the membership the reconciler + // binds in `user.create.after` — are credited to the admin instead + // of recorded as the system. Attribution only: the route's own + // authorization already happened above, and the writes stay system. + const { status, body } = await runAttributedToUser(actor.id, () => + runAdminCreateUser(adminUserDeps(), c.req.raw, actor), + ); return c.json(body, status as any); } catch (error) { const err = error instanceof Error ? error : new Error(String(error)); @@ -1735,7 +1746,10 @@ export class AuthPlugin implements Plugin { const metaReader = (() => { try { return ctx.getService?.('protocol'); } catch { return undefined; } })(); - const { status, body } = await runAdminImportUsers( + // [#4586] Same seam as create-user: server-side better-auth calls, + // credited to the admin who ran the import. + const { status, body } = await runAttributedToUser(actor.id, () => + runAdminImportUsers( { getAuthApi: () => this.authManager!.getApi() as any, getDataEngine: () => this.authManager!.getDataEngine(), @@ -1754,6 +1768,7 @@ export class AuthPlugin implements Plugin { }, c.req.raw, actor, + ), ); return c.json(body, status as any); } catch (error) { diff --git a/packages/plugins/plugin-auth/src/index.ts b/packages/plugins/plugin-auth/src/index.ts index 3ca836f65a..354e9b0462 100644 --- a/packages/plugins/plugin-auth/src/index.ts +++ b/packages/plugins/plugin-auth/src/index.ts @@ -25,6 +25,12 @@ export * from './otp-send-guard.js'; export * from './register-sso-provider.js'; export * from './send-verification-email.js'; export * from './objectql-adapter.js'; +// [#4586] The better-auth actor seam. Exported because a host that writes an +// identity table on better-auth's behalf (a control-plane provisioning hook, +// an SSO JIT path) must construct the SAME two-part context — +// `isSystem` for authorization, `attributedUserId` for attribution — rather +// than inventing a second way to say "the system did this, for that person". +export * from './auth-actor-attribution.js'; export * from './auth-schema-config.js'; // ADR-0093 — membership reconciler + tenancy service (public host API: hosts // compose the reconciler into their own hooks; embeddings query tenancy mode). diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index 325a74b8c8..eccd7fe52f 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -4,6 +4,7 @@ import type { IDataEngine } from '@objectstack/core'; import { createAdapterFactory } from 'better-auth/adapters'; import type { CleanedWhere } from 'better-auth/adapters'; import { SystemObjectName } from '@objectstack/spec/system'; +import { resolveAttributedUserId } from './auth-actor-attribution.js'; /** * Mapping from better-auth model names to ObjectStack protocol object names. @@ -241,14 +242,28 @@ export function withValidationErrorMapping>(adapte * writes; user-context writes to `managedBy: 'better-auth'` tables are already * rejected upstream by the identity write guard (ADR-0092 D2), so this path only * ever carries better-auth's internal writes. + * + * WRITES additionally carry `attributedUserId` when an auth request is in scope + * (#4586) — the human whose click better-auth is executing. That is the + * ATTRIBUTION half and nothing more: `isSystem` remains the AUTHORIZATION half, + * unconditionally, so what a write may touch is byte-for-byte what it could + * touch before. Reads never carry it — attribution describes a change, and a + * read changes nothing. */ export function withSystemContext(engine: IDataEngine): IDataEngine { const e = engine as any; const asSystem = (q: any) => ({ ...(q ?? {}), context: { isSystem: true, ...(q?.context ?? {}) } }); + // The attributed human is spread FIRST so an explicit caller-supplied context + // still wins on every key — the same precedence `isSystem` already had. + const asAttributedSystem = async (q: any) => { + const attributedUserId = await resolveAttributedUserId(); + if (!attributedUserId) return asSystem(q); + return { ...(q ?? {}), context: { attributedUserId, isSystem: true, ...(q?.context ?? {}) } }; + }; return { - insert: (m: string, d: any, o?: any) => e.insert(m, d, asSystem(o)), - update: (m: string, d: any, o?: any) => e.update(m, d, asSystem(o)), - delete: (m: string, q?: any) => e.delete(m, asSystem(q)), + insert: async (m: string, d: any, o?: any) => e.insert(m, d, await asAttributedSystem(o)), + update: async (m: string, d: any, o?: any) => e.update(m, d, await asAttributedSystem(o)), + delete: async (m: string, q?: any) => e.delete(m, await asAttributedSystem(q)), find: (m: string, q?: any) => e.find(m, asSystem(q)), findOne: (m: string, q?: any) => e.findOne(m, asSystem(q)), count: (m: string, q?: any) => e.count(m, asSystem(q)), diff --git a/packages/plugins/plugin-auth/src/reconcile-membership.test.ts b/packages/plugins/plugin-auth/src/reconcile-membership.test.ts index 1b3be99cc4..1c491fde17 100644 --- a/packages/plugins/plugin-auth/src/reconcile-membership.test.ts +++ b/packages/plugins/plugin-auth/src/reconcile-membership.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi } from 'vitest'; import { reconcileMembership, backfillMemberships } from './reconcile-membership.js'; +import { runAttributedToUser } from './auth-actor-attribution.js'; /** * In-memory engine over sys_member (+ optional sys_user) with the find/insert @@ -158,3 +159,37 @@ describe('backfillMemberships', () => { expect(res.skipped).toBe(1); }); }); + +/** + * [#4586] The reconciler bind is the third `sys_member` writer (after the + * better-auth adapter and the invite-accept path), and it runs INSIDE + * better-auth's `user.create.after` — so when the creation was an admin + * action, the acting human is in scope and the membership row should name them. + */ +describe('reconcileMembership — actor attribution (#4586)', () => { + it('credits the acting admin when a request scope names one', async () => { + const engine = makeEngine(); + await runAttributedToUser('usr_admin', () => + reconcileMembership(engine, 'user-1', { + policy: 'auto', + resolveTargetOrg: async () => 'org_default', + }), + ); + const [, , options] = engine.insert.mock.calls[0]; + // Both halves, side by side and separate: system AUTHORIZES the write to a + // better-auth-managed table, the admin is merely CREDITED for it. + expect(options).toEqual({ context: { isSystem: true, attributedUserId: 'usr_admin' } }); + }); + + it('a self sign-up has no actor — the bind records as the system', async () => { + // Nobody else acted, and there is no session on the sign-up request. ADR-0118 + // D1: that is `null`, not a fabricated caller. + const engine = makeEngine(); + await reconcileMembership(engine, 'user-2', { + policy: 'auto', + resolveTargetOrg: async () => 'org_default', + }); + const [, , options] = engine.insert.mock.calls[0]; + expect(options).toEqual({ context: { isSystem: true } }); + }); +}); diff --git a/packages/plugins/plugin-auth/src/reconcile-membership.ts b/packages/plugins/plugin-auth/src/reconcile-membership.ts index f86e2ffe5e..12ee0bdce2 100644 --- a/packages/plugins/plugin-auth/src/reconcile-membership.ts +++ b/packages/plugins/plugin-auth/src/reconcile-membership.ts @@ -26,6 +26,8 @@ * kernel:ready backfill is the self-healing net). */ +import { authSystemWriteContext } from './auth-actor-attribution.js'; + export type MembershipPolicy = 'auto' | 'invite-only'; export type ReconcileOutcome = @@ -81,7 +83,14 @@ async function insertMembership(engine: any, organizationId: string, userId: str await engine.insert( 'sys_member', { id: genMemberId(), organization_id: organizationId, user_id: userId, role: 'member' }, - { context: SYSTEM_CTX }, + // [#4586] The bind runs inside better-auth's `user.create.after`, so when + // the creation was an ADMIN action (`/admin/create-user`, bulk import) the + // acting human is in scope and the membership row's history names them + // instead of "system". Self sign-up resolves nothing (no session yet) and + // records as the system — correct, nobody else acted. `isSystem` is + // constructed here regardless: this is a platform write to a + // better-auth-managed table, and attribution never changes that. + { context: await authSystemWriteContext() }, ); } diff --git a/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts b/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts index aa8cd8ed95..6a8f166948 100644 --- a/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts +++ b/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts @@ -4,6 +4,8 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { reconcileOrgAdminGrant, backfillOrgAdminGrants, + AUTO_ORG_ADMIN_GRANT_REASON_PREFIX, + autoOrgAdminGrantReason, } from './auto-org-admin-grant.js'; /** @@ -270,3 +272,100 @@ describe('[ADR-0105 D4] posture selects the org-admin variant', () => { expect(grants.every((g) => g.permission_set_id === 'ps_org_admin_nb')).toBe(true); }); }); + +// --------------------------------------------------------------------------- +// [#4586] Hop 2 of the elevation chain stops discarding provenance. +// +// The chain is `sys_member.role` → this reconciler → `sys_user_permission_set` +// → `isTenantAdmin()`. The grant row had a `granted_by` column and wrote `null` +// into it unconditionally, so "why is X a tenant admin" dead-ended one hop in. +// Now the row records BOTH halves of the answer: the human whose better-auth +// call changed the grade (`granted_by`), and the machine writer + the exact +// membership row that triggered it (`reason`). +// --------------------------------------------------------------------------- +describe('[#4586] the auto-grant records its provenance', () => { + const seed = () => + makeStub({ + sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_member: [{ id: 'mem_42', user_id: 'u1', organization_id: 'o1', role: 'admin' }], + sys_user_permission_set: [], + }); + + it('stamps the attributed human into granted_by', async () => { + const stub = seed(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { ...WALLED, attributedUserId: 'usr_boss' }); + expect(stub.tables.sys_user_permission_set[0].granted_by).toBe('usr_boss'); + }); + + it('names the machine writer and the triggering sys_member row in reason', async () => { + const stub = seed(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { ...WALLED, attributedUserId: 'usr_boss' }); + const reason: string = stub.tables.sys_user_permission_set[0].reason; + // The marker a reader matches on — one constant, not a re-derived string. + expect(reason.startsWith(AUTO_ORG_ADMIN_GRANT_REASON_PREFIX)).toBe(true); + // …and the rest of the chain: which membership, at which grade, to which set. + expect(reason).toContain('mem_42'); + expect(reason).toContain('admin'); + expect(reason).toContain('organization_admin'); + expect(reason).toBe( + autoOrgAdminGrantReason({ id: 'mem_42', role: 'admin' }, 'organization_admin'), + ); + }); + + it('a machine-originated grade change leaves granted_by NULL — never a sentinel', async () => { + // ADR-0118 D1: `granted_by` is a `sys_user` lookup, so the only two legal + // values are a real id and null. The kernel:ready backfill and any boot + // bind have no human, and writing 'system' there would break the join and + // force every reader to special-case it. + const stub = seed(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + const row = stub.tables.sys_user_permission_set[0]; + expect(row.granted_by).toBeNull(); + // The machine provenance is still recorded — in the column that takes text. + expect(row.reason.startsWith(AUTO_ORG_ADMIN_GRANT_REASON_PREFIX)).toBe(true); + }); + + it('attribution changes nothing about WHETHER the grant happens', async () => { + // The threaded human is attribution, not authority: a member-grade row does + // not become grantable because an admin triggered the write. + const stub = makeStub({ + sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_member: [{ id: 'mem_9', user_id: 'u1', organization_id: 'o1', role: 'member' }], + sys_user_permission_set: [], + }); + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', { + ...WALLED, + attributedUserId: 'usr_boss', + }); + expect(res.action).toBe('noop'); + expect(stub.tables.sys_user_permission_set).toHaveLength(0); + }); + + it('demotion still revokes — with or without an attributed actor', async () => { + const stub = seed(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { ...WALLED, attributedUserId: 'usr_boss' }); + expect(stub.tables.sys_user_permission_set).toHaveLength(1); + + stub.tables.sys_member = [ + { id: 'mem_42', user_id: 'u1', organization_id: 'o1', role: 'member' }, + ]; + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', { + ...WALLED, + attributedUserId: 'usr_boss', + }); + expect(res.action).toBe('revoked'); + expect(stub.tables.sys_user_permission_set).toHaveLength(0); + }); + + it('the backfill grants with no human — it is machine-originated by construction', async () => { + const stub = makeStub({ + sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_member: [{ id: 'mem_7', user_id: 'u1', organization_id: 'o1', role: 'owner' }], + sys_user_permission_set: [], + }); + await backfillOrgAdminGrants(stub, WALLED); + const row = stub.tables.sys_user_permission_set[0]; + expect(row.granted_by).toBeNull(); + expect(row.reason).toContain('mem_7'); + }); +}); diff --git a/packages/plugins/plugin-security/src/auto-org-admin-grant.ts b/packages/plugins/plugin-security/src/auto-org-admin-grant.ts index 3ecd73bced..fb5da61260 100644 --- a/packages/plugins/plugin-security/src/auto-org-admin-grant.ts +++ b/packages/plugins/plugin-security/src/auto-org-admin-grant.ts @@ -118,6 +118,48 @@ function isAdminRole(raw: unknown): boolean { return roles.includes('owner') || roles.includes('admin'); } +/** + * [#4586] Machine-provenance marker for rows this module writes. + * + * The auto-grant is the second hop of the elevation chain + * (`sys_member.role` → here → `sys_user_permission_set` → `isTenantAdmin()`), + * and it is the hop with no human in it: no operator ever asked for THIS row, + * a membership grade did. Stamping that fact in a stable, greppable prefix is + * what lets "why is X a tenant admin" be answered from the data — the row says + * which writer minted it and which membership triggered it, so the chain is + * followable instead of inferred. + */ +export const AUTO_ORG_ADMIN_GRANT_REASON_PREFIX = 'auto-org-admin-grant'; + +/** + * The `reason` text for an auto-granted org-admin row: the machine marker, the + * `sys_member` row that triggered it, and the grade that qualified. + * + * Deliberately NOT in `granted_by`: that column is a `sys_user` lookup, and + * ADR-0118 D1 admits exactly two values there — a real user id, or `null` for + * "the system did this". A marker string in a lookup column is the sentinel + * that ADR forbids (it breaks the join and forces every reader to special-case + * it). The repo's own vocabulary already splits these roles — + * `validate-security-posture` states it as "granted_by = writer, + * delegated_from = authority source, reason = why" — so the human goes to + * `granted_by` and the why goes here. + */ +export function autoOrgAdminGrantReason( + member: { id?: unknown; role?: unknown } | undefined, + setName: string, +): string { + const memberId = typeof member?.id === 'string' || typeof member?.id === 'number' + ? String(member.id) + : ''; + const role = parseRoles(member?.role).join(','); + return ( + `${AUTO_ORG_ADMIN_GRANT_REASON_PREFIX}: granted from membership grade` + + (role ? ` '${role}'` : '') + + (memberId ? ` (sys_member ${memberId})` : '') + + ` → ${setName}` + ); +} + /** * Resolve the `sys_permission_set.id` for `organization_admin`. Cached * across calls per ObjectQL instance via a WeakMap so repeated @@ -158,7 +200,21 @@ export async function reconcileOrgAdminGrant( ql: any, userId: string, orgId: string, - options: { logger?: MaybeLogger; posture?: TenancyPosture } = {}, + options: { + logger?: MaybeLogger; + posture?: TenancyPosture; + /** + * [#4586] The human the TRIGGERING `sys_member` write was attributed to + * (`ExecutionContext.attributedUserId`), stamped into `granted_by` so the + * grant names the admin who caused it rather than nobody. Attribution + * only: this reconciler's own writes stay `SYSTEM_CTX`, and nothing here + * authorizes against this value. Absent — the kernel:ready backfill, a + * boot-time bind, any machine-originated grade change — leaves + * `granted_by: null`, the platform's one representation for "the system + * did this" (ADR-0118 D1). + */ + attributedUserId?: string; + } = {}, ): Promise<{ action: 'granted' | 'revoked' | 'noop' | 'skipped'; reason?: string; @@ -195,7 +251,10 @@ export async function reconcileOrgAdminGrant( { user_id: userId, organization_id: orgId }, 10, ); - const shouldGrant = memberships.some((m: any) => isAdminRole(m?.role)); + // The row that QUALIFIES is also the row the grant is provenance-linked to + // (#4586) — "this capability exists because of that membership". + const qualifyingMembership = memberships.find((m: any) => isAdminRole(m?.role)); + const shouldGrant = qualifyingMembership !== undefined; // 1b. [ADR-0105 D4] Revoke the OTHER variant for this pair, always. A posture // change (or a downgrade after F2) must converge on exactly one org-admin @@ -241,7 +300,13 @@ export async function reconcileOrgAdminGrant( user_id: userId, permission_set_id: permSetId, organization_id: orgId, - granted_by: null, + // [#4586] The provenance the row already had a column for. `granted_by` + // is a `sys_user` lookup: the human whose better-auth call triggered the + // grade change when one was in scope, else `null` = the system (ADR-0118 + // D1 — an id or null, never a sentinel). The machine marker and the + // triggering membership row live in `reason`, where free text belongs. + granted_by: options.attributedUserId ?? null, + reason: autoOrgAdminGrantReason(qualifyingMembership, grantSetName), }); if (created) { logger?.info?.('[security] granted org-admin capability', { @@ -249,6 +314,7 @@ export async function reconcileOrgAdminGrant( orgId, set: grantSetName, posture, + grantedBy: options.attributedUserId ?? null, }); return { action: 'granted' }; } diff --git a/packages/plugins/plugin-security/src/index.ts b/packages/plugins/plugin-security/src/index.ts index c5dd223575..e4b097b51e 100644 --- a/packages/plugins/plugin-security/src/index.ts +++ b/packages/plugins/plugin-security/src/index.ts @@ -23,6 +23,12 @@ export { export { reconcileOrgAdminGrant, backfillOrgAdminGrants, + // [#4586] The auto-grant's machine-provenance marker + its `reason` builder. + // Exported so a reader of the grant table (explain surfaces, compliance + // exports, tests) matches the ONE prefix this writer stamps instead of + // re-deriving the string. + AUTO_ORG_ADMIN_GRANT_REASON_PREFIX, + autoOrgAdminGrantReason, } from './auto-org-admin-grant.js'; export { bootstrapPlatformAdmin } from './bootstrap-platform-admin.js'; // [ADR-0105 D8] Scoped-invitation placement — issuance gate + accept-time apply. diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 07e6fdd7ce..4eef31d1f2 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -2090,9 +2090,24 @@ export class SecurityPlugin implements Plugin { return; } const pairs = extractMemberPairs(opCtx); + // [#4586] Carry the triggering write's ATTRIBUTED human into the grant. + // better-auth's membership writes authorize as the system on purpose, so + // `opCtx.context.userId` is empty by construction and `granted_by` was + // always null; `attributedUserId` is the channel that does name the + // admin who clicked. Read as attribution only — it decides nothing about + // whether this reconcile may run, and the reconciler's own writes stay + // system-context. + const attributedUserId = + typeof opCtx?.context?.attributedUserId === 'string' && opCtx.context.attributedUserId + ? opCtx.context.attributedUserId + : undefined; for (const { userId, orgId } of pairs) { try { - await reconcileOrgAdminGrant(ql, userId, orgId, { logger: ctx.logger, posture: this.tenancyPosture }); + await reconcileOrgAdminGrant(ql, userId, orgId, { + logger: ctx.logger, + posture: this.tenancyPosture, + ...(attributedUserId ? { attributedUserId } : {}), + }); } catch (e) { ctx.logger.warn?.('[security] org_admin reconcile failed', { userId, diff --git a/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts b/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts new file mode 100644 index 0000000000..7fe6a701e1 --- /dev/null +++ b/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts @@ -0,0 +1,306 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4586] `sys_member` grade changes keep the true actor — proven at the REAL + * better-auth routes, not against the seam in isolation. + * + * The gap: `sys_member` is `trackHistory: true`, so every role transition was + * already recorded — with "system" as the actor. Every writer is a better-auth + * path, the adapter runs them `isSystem: true` on purpose (the route already + * authorized under better-auth's own ACL, and ADR-0092 D2 refuses user-context + * writes to identity tables), and the human who clicked *make admin* was known + * exactly once, in the hook layer, then discarded. Second hop, same story: + * `auto-org-admin-grant` wrote `granted_by: null` into a column that exists, so + * "why is X a tenant admin" dead-ended one row in. + * + * Why this file exists rather than more unit tests (#3106): the seam's own + * mechanics are covered in `plugin-auth/src/auth-actor-attribution.test.ts`, + * and a seam that works when called directly proves nothing about whether the + * routes cross it. So these drive the actual endpoints — `invite-member` → + * `accept-invitation`, `update-member-role` up and back down — over the real + * HTTP stack, and read the rows the platform actually wrote. + * + * The one constraint the whole change hangs on is pinned here too: the threaded + * actor is ATTRIBUTION. It must never become the write's authorization subject + * — that would put a second adjudication track at exactly the boundary + * ADR-0095 D3 closed — so the promotion is asserted to be refused for a + * caller better-auth does not authorize, no matter who the write would be + * credited to. + * + * Harness note: `bootStack` disables the default-org bootstrap and installs no + * audit plugin, so this file mints the Default Organization itself (system + * context — exactly what the bootstrap would do) and adds `AuditPlugin`, which + * is what turns `trackHistory` into rows. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { AuditPlugin } from '@objectstack/plugin-audit'; +import { AUTO_ORG_ADMIN_GRANT_REASON_PREFIX } from '@objectstack/plugin-security'; + +const SYSTEM_CTX = { isSystem: true }; + +async function findRows(ql: any, object: string, where: any, limit = 50): Promise { + const rows = await ql.find(object, { where, limit }, { context: SYSTEM_CTX }); + return Array.isArray(rows) ? rows : (rows?.records ?? []); +} + +/** The sign-up reconciler's membership row lands asynchronously (better-auth + * defers `user.create.after` past the signup transaction). */ +async function waitForMembership(ql: any, userId: string): Promise { + for (let i = 0; i < 40; i++) { + const rows = await findRows(ql, 'sys_member', { user_id: userId }, 5); + if (rows.length > 0) return rows[0]; + await new Promise((r) => setTimeout(r, 250)); + } + throw new Error(`no sys_member row appeared for ${userId}`); +} + +/** Audit rows this stack wrote for one `sys_member` row, newest last. */ +async function memberHistory(ql: any, memberId: string, action?: string): Promise { + const rows = await findRows( + ql, + 'sys_audit_log', + { object_name: 'sys_member', record_id: memberId }, + 100, + ); + return action ? rows.filter((r: any) => r.action === action) : rows; +} + +async function waitForHistory(ql: any, memberId: string, action: string): Promise { + for (let i = 0; i < 20; i++) { + const rows = await memberHistory(ql, memberId, action); + if (rows.length > 0) return rows[rows.length - 1]; + await new Promise((r) => setTimeout(r, 250)); + } + throw new Error(`no '${action}' history row appeared for sys_member ${memberId}`); +} + +describe('#4586: the better-auth actor reaches sys_member history and the grant', () => { + let stack: VerifyStack; + let ql: any; + let orgId: string; + let partnerOrgId: string; + let adminToken: string; + let adminUserId: string; + let memberToken: string; + let memberUserId: string; + let memberRowId: string; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, { extraPlugins: [new AuditPlugin()] }); + adminToken = await stack.signIn(); // the seeded dev admin + ql = await stack.kernel.getServiceAsync('objectql'); + + const org = await ql.insert( + 'sys_organization', + { name: 'Default Organization', slug: 'default' }, + { context: SYSTEM_CTX }, + ); + orgId = String(org.id); + + // A second org, so invite-accept has somewhere to add a membership that the + // single-org reconciler has not already created. + const partner = await ql.insert( + 'sys_organization', + { name: 'Partner Organization', slug: 'partner' }, + { context: SYSTEM_CTX }, + ); + partnerOrgId = String(partner.id); + + // The dev admin predates the org rows — give them the owner membership the + // single-org bootstrap would have, in both orgs. + const [adminUser] = await findRows(ql, 'sys_user', { email: 'admin@objectos.ai' }, 1); + adminUserId = String(adminUser.id); + const adminMembers = await findRows(ql, 'sys_member', { user_id: adminUserId }, 5); + if (adminMembers.length > 0) { + await ql.update( + 'sys_member', + { id: adminMembers[0].id, organization_id: orgId, role: 'owner' }, + { context: SYSTEM_CTX }, + ); + } else { + await ql.insert( + 'sys_member', + { user_id: adminUserId, organization_id: orgId, role: 'owner' }, + { context: SYSTEM_CTX }, + ); + } + await ql.insert( + 'sys_member', + { user_id: adminUserId, organization_id: partnerOrgId, role: 'owner' }, + { context: SYSTEM_CTX }, + ); + + // The subject: an ordinary member the admin will promote and demote. + memberToken = await stack.signUp('member.4586@example.com', 'Member!Pass123', 'Member 4586'); + const [memberUser] = await findRows(ql, 'sys_user', { email: 'member.4586@example.com' }, 1); + memberUserId = String(memberUser.id); + const bound = await waitForMembership(ql, memberUserId); + memberRowId = String(bound.id); + expect(bound.role).toBe('member'); + }, 180_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + // ── W1: the grade change names the human ──────────────────────────────── + + it('update-member-role: the history row names the admin who promoted, not "system"', async () => { + const res = await stack.apiAs(adminToken, 'POST', '/auth/organization/update-member-role', { + memberId: memberRowId, + role: 'admin', + organizationId: orgId, + }); + expect(res.status, await res.clone().text()).toBe(200); + + const [row] = await findRows(ql, 'sys_member', { id: memberRowId }, 1); + expect(row.role).toBe('admin'); + + const history = await waitForHistory(ql, memberRowId, 'update'); + // Before this change the write reached ObjectQL as bare `{ isSystem: true }` + // and this column was null — the transition was recorded, the actor was not. + expect(history.user_id).toBe(adminUserId); + // A real `sys_user` id, so the lookup still joins (ADR-0118 D1: an id or + // null, never a sentinel string like 'system'). + expect(await findRows(ql, 'sys_user', { id: history.user_id }, 1)).toHaveLength(1); + // The diff itself is unchanged — this adds WHO, it does not alter WHAT. + expect(String(history.new_value)).toContain('admin'); + }, 60_000); + + // ── W2: the capability grant records its whole provenance ─────────────── + + it('the auto-granted capability names the admin in granted_by and the membership in reason', async () => { + // The elevation chain's second hop: sys_member.role → auto-org-admin-grant + // → sys_user_permission_set → isTenantAdmin(). + let grants: any[] = []; + for (let i = 0; i < 20 && grants.length === 0; i++) { + grants = await findRows( + ql, + 'sys_user_permission_set', + { user_id: memberUserId, organization_id: orgId }, + 5, + ); + if (grants.length === 0) await new Promise((r) => setTimeout(r, 250)); + } + expect(grants).toHaveLength(1); + const grant = grants[0]; + + // WHO: the human whose better-auth call caused the elevation. + expect(grant.granted_by).toBe(adminUserId); + // WHY: the machine writer that minted the row, and the membership that + // triggered it — so `explain` can walk the chain instead of inferring it. + expect(String(grant.reason).startsWith(AUTO_ORG_ADMIN_GRANT_REASON_PREFIX)).toBe(true); + expect(String(grant.reason)).toContain(memberRowId); + }, 60_000); + + // ── W1 again, on the OTHER membership-creating route ──────────────────── + + it('accept-invitation: the new membership is attributed to the person who accepted', async () => { + const invite = await stack.apiAs(adminToken, 'POST', '/auth/organization/invite-member', { + email: 'member.4586@example.com', + role: 'member', + organizationId: partnerOrgId, + }); + expect(invite.status, await invite.clone().text()).toBe(200); + const [invitation] = await findRows( + ql, + 'sys_invitation', + { email: 'member.4586@example.com', organization_id: partnerOrgId }, + 1, + ); + expect(invitation).toBeDefined(); + + const accept = await stack.apiAs(memberToken, 'POST', '/auth/organization/accept-invitation', { + invitationId: String(invitation.id), + }); + expect(accept.status, await accept.clone().text()).toBe(200); + + const partnerMembers = await findRows( + ql, + 'sys_member', + { user_id: memberUserId, organization_id: partnerOrgId }, + 5, + ); + expect(partnerMembers).toHaveLength(1); + + // The ACCEPTOR is the actor here, not the inviter — which is the point of + // threading the real session actor rather than hard-coding one route's idea + // of who is responsible. + const created = await waitForHistory(ql, String(partnerMembers[0].id), 'create'); + expect(created.user_id).toBe(memberUserId); + }, 60_000); + + // ── W1 + W2 on the way back down ──────────────────────────────────────── + + it('demotion: the grade change back down is attributed to the admin too', async () => { + const res = await stack.apiAs(adminToken, 'POST', '/auth/organization/update-member-role', { + memberId: memberRowId, + role: 'member', + organizationId: orgId, + }); + expect(res.status, await res.clone().text()).toBe(200); + + const [row] = await findRows(ql, 'sys_member', { id: memberRowId }, 1); + expect(row.role).toBe('member'); + + const updates = await memberHistory(ql, memberRowId, 'update'); + const demotion = updates[updates.length - 1]; + expect(String(demotion.new_value)).toContain('member'); + // Attribution works in both directions — taking authority away is exactly + // as answerable as handing it out. + expect(demotion.user_id).toBe(adminUserId); + + // NOTE: whether the org-admin GRANT is revoked on demotion is deliberately + // not asserted here. It currently is not — `auto-org-admin-grant`'s only + // delete channel calls `ql.delete(object, id, ctx)` while the engine takes + // `(object, { where, context })`, so every revoke throws into a swallowing + // catch. That is a separate, pre-existing defect (its unit stub implements + // the wrong signature, so the suite never saw it) filed as #4640 — a + // security behaviour change that deserves its own review and changeset, not + // a rider on an attribution PR. + }, 60_000); + + // ── The hard constraint ───────────────────────────────────────────────── + + it('attribution is not authority: a plain member cannot promote themselves', async () => { + // The failure mode this change must not create. If the threaded actor had + // become the write's authorization subject, the platform would be + // adjudicating membership writes a second time, beside better-auth's own + // route ACL — the boundary ADR-0095 D3 closed. better-auth still owns the + // decision, and it says no. + const before = (await memberHistory(ql, memberRowId, 'update')).length; + + const res = await stack.apiAs(memberToken, 'POST', '/auth/organization/update-member-role', { + memberId: memberRowId, + role: 'admin', + organizationId: orgId, + }); + expect(res.status).toBeGreaterThanOrEqual(400); + + // Refused at the door: no grade change, and therefore nothing to attribute. + const [row] = await findRows(ql, 'sys_member', { id: memberRowId }, 1); + expect(row.role).toBe('member'); + expect((await memberHistory(ql, memberRowId, 'update')).length).toBe(before); + }, 60_000); + + it('a machine-originated membership write still records as the system', async () => { + // ADR-0118 D1/D2: no actor in scope means `null` — absence is never + // upgraded into a caller, and never written as a sentinel. + const orphan = await ql.insert( + 'sys_user', + { name: 'Orphan 4586', email: 'orphan.4586@example.com' }, + { context: SYSTEM_CTX }, + ); + const row = await ql.insert( + 'sys_member', + { user_id: String(orphan.id), organization_id: partnerOrgId, role: 'member' }, + { context: SYSTEM_CTX }, + ); + const created = await waitForHistory(ql, String(row.id), 'create'); + expect(created.user_id).toBeNull(); + expect(created.actor).toBeNull(); + }, 60_000); +}); diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index c9857043d7..a886cbf3cb 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -4450,6 +4450,7 @@ "kernel/ExecutionContext:accessToken", "kernel/ExecutionContext:accessible_org_ids", "kernel/ExecutionContext:actor", + "kernel/ExecutionContext:attributedUserId", "kernel/ExecutionContext:audience", "kernel/ExecutionContext:currency", "kernel/ExecutionContext:email", diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index eba3cae85a..2c1726e4b4 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -380,21 +380,31 @@ export const HookContextSchema = lazySchema(() => z.object({ /** * Write Provenance - * WHAT produced this write, as opposed to WHO is calling ({@link session}). + * WHERE this write came from, as opposed to WHO is calling ({@link session}). * - * Server-stamped and never client-supplied. It is not an identity: nothing - * here is evaluated by any security middleware, and it neither widens nor - * narrows what the write may touch. It exists so a hook can tell "the actor - * that OWNS some externalized state" from "an unrelated caller". + * Server-stamped and never client-supplied. **Nothing here is an + * authorization input**: no security middleware evaluates any of it, and it + * neither widens nor narrows what the write may touch. It exists so a hook — + * or the audit writer — can tell "the run / the person this write belongs + * to" from "an unrelated caller". + * + * Two marks, both non-authorizing, for two different questions: + * - `flowRunId` — WHAT produced the write (a machine origin, no person); + * - `attributedUserId` — WHO is CREDITED for a write the system authorized + * (a person, but never the subject the write was authorized as). * * Kept OUT of `session` on purpose. A writer can have provenance and no * identity at all — a schedule-triggered flow run resolves no principal — and * folding the two together would have forced such a run to present an empty * session, silently turning "no caller" into "an anonymous caller" for every - * hook that gates on `session` being absent (#3712). + * hook that gates on `session` being absent (#3712). The same reasoning + * keeps `attributedUserId` out: it names a human, but the write authorized + * as the SYSTEM, and a hook reading `session.userId` must keep seeing the + * truth — there was no caller (#4586). */ provenance: z.object({ flowRunId: z.string().optional().describe('Id of the automation flow run performing this write, when it originates from a flow data node. Lets a hook recognize the run that OWNS state that run itself opened — the approvals record lock exempts the run holding the pending request (#3456).'), + attributedUserId: z.string().optional().describe('The real human credited for a write whose authorization subject was the SYSTEM — e.g. the admin whose better-auth `update-member-role` call the identity adapter executes as `isSystem` (#4586). ATTRIBUTION ONLY: the audit writer records it as `sys_audit_log.user_id`; no security middleware reads it, and it never becomes the subject the write is authorized as.'), }).optional().describe('Server-stamped write provenance (never client-supplied, never an authorization input)'), diff --git a/packages/spec/src/kernel/execution-context.zod.ts b/packages/spec/src/kernel/execution-context.zod.ts index e7f7af53d3..d0031d2b24 100644 --- a/packages/spec/src/kernel/execution-context.zod.ts +++ b/packages/spec/src/kernel/execution-context.zod.ts @@ -34,6 +34,46 @@ export const ExecutionContextSchema = lazySchema(() => z.object({ */ actor: z.string().optional(), + /** + * [#4586] The real HUMAN behind a write whose authorization subject is the + * SYSTEM — **attribution only, never authorization**. + * + * The case it exists for: better-auth owns every write to the identity + * tables (`sys_member`, `sys_user`, …) and its adapter runs them + * `isSystem: true` **on purpose** — the route already authorized the action + * under better-auth's own ACL (ADR-0092 D2 refuses user-context writes to + * those tables outright). So the person who clicked *make admin* was known + * exactly once, in the hook layer where the session exists, and was then + * discarded: every `sys_member` role transition recorded "system" as its + * actor. This field is the one hop that carries them through. + * + * **The invariant, and it is load-bearing:** nothing in the authorization + * path reads this. It is not {@link userId} — that is the subject the + * engine authorizes AS (RLS `current_user`, ownership stamps, permission + * resolution). Promoting the attributed human to the write's authorization + * subject would re-adjudicate a decision better-auth already made, opening + * the second adjudication track ADR-0095 D3 closed. A context carrying only + * this field authorizes exactly like a context carrying nothing — + * ANONYMOUS, per ADR-0118 D2 ("absence is never system") — and a context + * carrying it beside `isSystem: true` authorizes exactly like `isSystem` + * alone. Attribution and authorization are separate fields on purpose. + * + * Relationship to {@link actor}: same intent (audit attribution), different + * principal. `actor` is a LABEL for a caller that is not a user at all + * (`svc:`) and lands on `sys_audit_log.actor`; this is a real + * `sys_user` id and lands on `sys_audit_log.user_id`, keeping that lookup + * column joinable (ADR-0118 D1 — a user-lookup column holds an id or null, + * never a sentinel). Absent = the write is genuinely machine-originated + * (boot sync, migration, scheduled job) and records as `null`. + * + * Server-constructed only, never client-supplied — exactly like + * {@link isSystem} and {@link flowRunId}. Surfaced to hooks as + * `HookContext.provenance.attributedUserId`, deliberately NOT folded into + * `session`: a hook that gates on `session.userId` must keep seeing "no + * caller" here, because there is none. + */ + attributedUserId: z.string().optional(), + /** * Current user's unique email (resolved from session, falling back to a * `sys_user` lookup). Exposed to RLS as `current_user.email` for seedable, From 37e02eafb75de07bd86efe375218b1bbadf37416 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 14:37:25 +0000 Subject: [PATCH 3/4] test(dogfood): identify the demotion history row by what it records, not its position (#4586) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Dogfood Regression Gate failed on this file's own new assertion: expected '{"role":"admin"}' to contain 'member' Not a defect in what `trackHistory` stores — the audit writer's `update` leg writes a CHANGED-FIELDS DIFF, `old_value` the before-state and `new_value` the after-state (`diff()` in `audit-writers.ts` fills both halves), so a demotion really is recorded as `{"role":"admin"}` → `{"role":"member"}`. The audit question "who changed X from member to admin" was already answerable; #4586 adds WHO to a row that already knew WHAT. The defect was the TEST's row SELECTION. It took `rows[rows.length - 1]` as "the newest row", which is wrong twice over: 1. `find` without an explicit sort is unordered, so the last element is not the newest anything; 2. the audit row lands ASYNCHRONOUSLY after the endpoint returns, so at the moment the demotion test polled, the only `update` row present was the PROMOTION from the earlier test — and it asserted the demotion's expectation against it. Locally green, red in CI, because `packages/qa/dogfood` sits outside the `--filter` scope the change was verified under. `waitForHistoryMatching` now waits for the row that says the thing under test, so both hazards are gone. The demotion case additionally pins the pair (`old_value` admin → `new_value` member) and asserts the promotion survives as a DISTINCT row keeping its own actor. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012C2cd7tL8QDoZ2QKN3djJ5 --- ...mbership-actor-attribution.dogfood.test.ts | 58 ++++++++++++++++--- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts b/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts index 7fe6a701e1..2fce49a32b 100644 --- a/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts +++ b/packages/qa/dogfood/test/membership-actor-attribution.dogfood.test.ts @@ -57,7 +57,7 @@ async function waitForMembership(ql: any, userId: string): Promise { throw new Error(`no sys_member row appeared for ${userId}`); } -/** Audit rows this stack wrote for one `sys_member` row, newest last. */ +/** Audit rows this stack wrote for one `sys_member` row. */ async function memberHistory(ql: any, memberId: string, action?: string): Promise { const rows = await findRows( ql, @@ -68,13 +68,38 @@ async function memberHistory(ql: any, memberId: string, action?: string): Promis return action ? rows.filter((r: any) => r.action === action) : rows; } -async function waitForHistory(ql: any, memberId: string, action: string): Promise { +/** + * Wait for the history row that records a SPECIFIC transition, identified by + * what it says rather than by its position. + * + * Selecting "the last row" is wrong twice over here: `find` is unordered + * without an explicit sort, and — the trap this file walked into — the audit + * row lands ASYNCHRONOUSLY after the endpoint returns, so a promote/demote + * pair polled by count alone happily returns the *previous* transition and + * asserts against it. Matching on the row's own `new_value` is immune to both. + * + * On `action: 'update'` the audit writer stores a CHANGED-FIELDS DIFF: the + * before-state in `old_value`, the after-state in `new_value` (a demotion is + * `old_value {"role":"admin"}` → `new_value {"role":"member"}`). Both halves + * are recorded — this change adds WHO to a row that already knew WHAT. + */ +async function waitForHistoryMatching( + ql: any, + memberId: string, + action: string, + matches: (row: any) => boolean, + what = action, +): Promise { for (let i = 0; i < 20; i++) { - const rows = await memberHistory(ql, memberId, action); - if (rows.length > 0) return rows[rows.length - 1]; + const hit = (await memberHistory(ql, memberId, action)).find(matches); + if (hit) return hit; await new Promise((r) => setTimeout(r, 250)); } - throw new Error(`no '${action}' history row appeared for sys_member ${memberId}`); + throw new Error(`no '${what}' history row appeared for sys_member ${memberId}`); +} + +async function waitForHistory(ql: any, memberId: string, action: string): Promise { + return waitForHistoryMatching(ql, memberId, action, () => true); } describe('#4586: the better-auth actor reaches sys_member history and the grant', () => { @@ -246,12 +271,31 @@ describe('#4586: the better-auth actor reaches sys_member history and the grant' const [row] = await findRows(ql, 'sys_member', { id: memberRowId }, 1); expect(row.role).toBe('member'); - const updates = await memberHistory(ql, memberRowId, 'update'); - const demotion = updates[updates.length - 1]; + // Identify the demotion row by what it RECORDS, not by its position: the + // promotion row is already there, and the new row lands asynchronously. + const demotion = await waitForHistoryMatching( + ql, + memberRowId, + 'update', + (r) => String(r.new_value).includes('member'), + 'demotion', + ); + // The pair the diff actually stores: admin → member. + expect(String(demotion.old_value)).toContain('admin'); expect(String(demotion.new_value)).toContain('member'); // Attribution works in both directions — taking authority away is exactly // as answerable as handing it out. expect(demotion.user_id).toBe(adminUserId); + // …and it is a DISTINCT row from the promotion, which keeps its own actor. + const promotion = await waitForHistoryMatching( + ql, + memberRowId, + 'update', + (r) => String(r.new_value).includes('admin'), + 'promotion', + ); + expect(String(promotion.id)).not.toBe(String(demotion.id)); + expect(promotion.user_id).toBe(adminUserId); // NOTE: whether the org-admin GRANT is revoked on demotion is deliberately // not asserted here. It currently is not — `auto-org-admin-grant`'s only From 631ba442fdf223b0798e68ad8cc443a0e9a9e683 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 14:37:36 +0000 Subject: [PATCH 4/4] docs(changeset): add the changeset for the #4586 actor-attribution seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Check Changeset gate was red: a user-visible behaviour change (identity-table writes now carry the true actor; `granted_by` / `reason` stop being null) with no `.changeset/*.md`. Minor across the five published packages whose surface moves — `spec` gains an authorable `ExecutionContext.attributedUserId` and the `HookContext.provenance` envelope, so this is additive API, not a pure fix. The body states the hard constraint the reviewer must be able to find later: the threaded actor is attribution ONLY and never the authorization subject. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012C2cd7tL8QDoZ2QKN3djJ5 --- .changeset/actor-attribution-seam.md | 60 ++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 .changeset/actor-attribution-seam.md diff --git a/.changeset/actor-attribution-seam.md b/.changeset/actor-attribution-seam.md new file mode 100644 index 0000000000..8891469c97 --- /dev/null +++ b/.changeset/actor-attribution-seam.md @@ -0,0 +1,60 @@ +--- +"@objectstack/spec": minor +"@objectstack/objectql": minor +"@objectstack/plugin-auth": minor +"@objectstack/plugin-audit": minor +"@objectstack/plugin-security": minor +--- + +feat(auth,objectql,audit,security,spec): identity-table writes carry the real actor, so `sys_member` history stops saying "system" (#4586) + +better-auth owns every write to the identity tables (`sys_member`, `sys_user`, +`sys_invitation`, …) and its ObjectQL adapter runs them `isSystem: true` **on +purpose** — the route already authorized the action under better-auth's own ACL, +and ADR-0092 D2 refuses user-context writes to those tables outright. The +consequence was that the human who clicked *make admin* was known exactly once, +in the hook layer where the session exists, and then discarded: every +`trackHistory` transition on `sys_member` recorded `user_id: null` / "system", +and `sys_user_permission_set.granted_by` was written null by the auto-grant. +"Who made this person an org admin?" had no answer in the platform's own audit +log. + +**What changed** + +A request-scoped attribution seam, general rather than a `sys_member` special +case: + +| Layer | Before | After | +|:--|:--|:--| +| `ExecutionContext` | `userId` / `actor` only | new optional `attributedUserId` — the human CREDITED for a write the system AUTHORIZED | +| `HookContext` | `session`, `user` | new `provenance.attributedUserId`, split off the context beside `session` | +| better-auth ObjectQL adapter | `{ isSystem: true }` | `{ isSystem: true, attributedUserId }` when a request scope is open | +| audit writer | `user_id = session.userId ?? null` | falls back to `provenance.attributedUserId` when the session names nobody | +| `auto-org-admin-grant` | `granted_by: null`, no `reason` | the attributed human in `granted_by`, plus a machine-provenance `reason` naming the writer and the triggering `sys_member` row | + +Outside a request scope nothing changes: writes stay bare `{ isSystem: true }` +and audit rows keep recording `null`. Absence is still never upgraded into a +caller, and never written as a sentinel string (ADR-0118 D1/D2). + +**Hard constraint — attribution is not authority** + +`attributedUserId` is read by exactly one consumer, the audit writer, and by no +security middleware. It never becomes `ExecutionContext.userId`, so it is never +the subject the engine authorizes as: not RLS `current_user`, not the ownership +stamp, not permission resolution. A context carrying only `attributedUserId` +authorizes exactly like an empty context (ANONYMOUS), and a context carrying it +beside `isSystem: true` authorizes exactly like `isSystem` alone. Re-authorizing +identity writes as the human would re-adjudicate a decision better-auth already +made — the second adjudication track ADR-0095 D3 closed. The constraint is +pinned by tests at three layers: the engine seam +(`packages/objectql/src/engine.test.ts`), the better-auth adapter +(`packages/plugins/plugin-auth/src/auth-actor-attribution.test.ts`), and the +live HTTP route (a plain member still cannot promote themselves). + +**For authors and plugin developers** + +`attributedUserId` is authorable on `ExecutionContext` and readable as +`ctx.provenance?.attributedUserId` in hooks. Use it to answer *who is +responsible*; keep using `ctx.session` / `ctx.user` to decide *what is +permitted*. The two are separate fields precisely so the distinction cannot be +blurred by accident.