diff --git a/.changeset/action-ctx-user-display-name.md b/.changeset/action-ctx-user-display-name.md new file mode 100644 index 0000000000..08da1eac1f --- /dev/null +++ b/.changeset/action-ctx-user-display-name.md @@ -0,0 +1,61 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): `ctx.user.name` is the acting user's display name, on every dispatch path (#5372) + +An action body reading `ctx.user.name` got the raw user id back — a *declared* +key delivering a plausible **wrong value**, which is the failure mode +"declared = enforced" exists to prevent. Nothing downstream can detect it: the +value is a perfectly good string, so no `??` chain and no consumer-side guard +tells it apart from a real name. Apps that trusted the declaration wrote opaque +ids into user-facing surfaces (an activity timeline rendering +`usr_01j…` as its actor for every logged activity). + +Three dispatchers built the caller's `user` object three different ways, and +all three landed on the id: + +- **REST `/actions`** hardcoded `name: ec.userId`. +- **MCP `run_action`** read `ec.userName ?? ec.userDisplayName ?? ec.userId`. + Neither alias is declared on `ExecutionContextSchema` and nothing ever + assigned either, so the chain's only reachable arm was the id. +- **The AI routes** spelled the key `displayName` (same dead chain behind it) + and read the caller's address off `ec.userEmail` — the declared field is + `ec.email` — so `req.user.email` there was permanently `undefined`. + +**What changes.** One shared producer builds the user envelope for all three +paths. `name` now carries `sys_user.name`, the platform's own profile +display-name column, resolved once per request (a memo keyed on the request's +ExecutionContext, so N action dispatches in one request cost one indexed read +— ~0.22 ms measured against real SQLite — and nothing is cached across +requests, so a rename takes effect on the user's next request). + +Resolution is **quiet**: no `sys_user` row, no engine, a failing read or a blank +name falls back to the id. A missing display name never fails an action. So +`name === id` now means exactly one thing — *this user has no resolvable display +name* — which is what makes the fix detectable from application code: any +workaround of the form "if `ctx.user.name` differs from `ctx.user.id`, trust +it; otherwise look the name up myself" **self-retires** the moment this lands, +with no coordinated deploy. + +**One shape, and it is the spec's.** [ADR-0068 D1] declares `EvalUser` as the +one user-context contract, mounted under `current_user` / `user` / `ctx.user` +on the predicate surface — with `name` on it, meaning "display name". The +dispatch envelope's identity core is now built through that same +`createEvalUser` factory, so an action's `visible` predicate and its `body` — +both spelled `ctx.user` — see one object: `id`, `name`, `email`, `positions`, +`isPlatformAdmin`, `organizationId`. On top of that core the dispatch surfaces +keep publishing what they already published: `userId` and `displayName` +(aliases of `id` / `name`, same values), `roles` (the pre-ADR-0090 alias of +`positions`), and the two authority channels `permissions` (permission-set +names) and `systemPermissions` (capabilities), still side by side and never +merged. Additive for every existing reader; no key was removed. + +The AI routes' second `req.user` producer (the concrete per-route mounts) is +built by the same function, so the two can no longer drift apart by hand. Its +display name comes from the session's own `user.name`, needing no extra read; +its former `?? user.email` middle arm is gone so that `name === id` means the +same thing on every producer — the address is still served under `email`. + +`buildActionSandboxContext` is unchanged: it passed the user through verbatim +all along, and was never where the name was lost. diff --git a/packages/runtime/src/action-ctx-user-shape.test.ts b/packages/runtime/src/action-ctx-user-shape.test.ts new file mode 100644 index 0000000000..f0b4996093 --- /dev/null +++ b/packages/runtime/src/action-ctx-user-shape.test.ts @@ -0,0 +1,314 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5372] `ctx.user.name` is the acting user's DISPLAY NAME, on every path. + * + * The defect these tests pin is not a missing key — it is a declared key + * delivering a plausible WRONG value, which is strictly worse: `ctx.user.name` + * read as a perfectly good string, so no consumer-side `??` could tell that it + * was the raw user id, and app code that trusted the declaration wrote opaque + * ids into user-facing surfaces (objectstack-ai/hotcrm#673's activity + * timeline). + * + * Three dispatchers built the user object three different ways: + * + * - REST `/actions` hardcoded `name: ec.userId`; + * - MCP `run_action` read `ec.userName ?? ec.userDisplayName ?? ec.userId` + * — neither alias is declared on `ExecutionContextSchema` and nothing in + * the repo ever assigned either, so the only reachable arm was the id; + * - the AI routes spelled the key `displayName` (same dead chain) and read + * the caller's address off `ec.userEmail`, which is not the declared field + * (`ec.email`), so `user.email` there was permanently `undefined`. + * + * So the assertions come in three families: + * 1. the VALUE — `name` is `sys_user.name`, and `name === id` happens if and + * only if the user has no resolvable display name (both directions); + * 2. the SHAPE — all three paths, plus the AI routes' second producer, emit + * ONE key set, so a body/handler never branches on which door it came in; + * 3. the FAILURE MODE — a name that cannot be resolved is quiet: the + * dispatch still succeeds and `name` falls back to the id. A display name + * is not worth failing an action over. + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { HttpDispatcher } from './http-dispatcher.js'; +import { invokeBusinessAction } from './action-execution.js'; +import { handleAIRequest } from './domains/ai.js'; +import { actionBodyRunnerFactory } from './sandbox/body-runner.js'; +import { QuickJSScriptRunner } from './sandbox/quickjs-runner.js'; +import type { DomainHandlerDeps } from './domain-handler-registry.js'; +import type { HttpProtocolContext } from './http-dispatcher.js'; + +const ACTION = { + name: 'close_case', + label: 'Close', + objectName: 'crm_case', + type: 'script', + target: 'closeCase', + ai: { exposed: true, description: 'Close a case.' }, +}; +const OBJECT_DEF = { name: 'crm_case', actions: [ACTION] }; + +/** The acting principal, as `resolveExecutionContext` actually builds one. */ +function makeEc(overrides: Record = {}) { + return { + userId: 'usr_admin', + email: 'admin@objectos.ai', + tenantId: 'org_1', + positions: ['platform_admin'], + permissions: ['admin_full_access'], + systemPermissions: ['manage_metadata'], + ...overrides, + }; +} + +/** + * An engine whose `sys_user` read answers with `row`. `undefined` = the row is + * not there at all (a service principal, a deleted account); `throws: true` = + * the read itself fails. + */ +function makeQl(row: Record | undefined, opts: { throws?: boolean } = {}) { + const executeAction = vi.fn(async () => ({ ok: true })); + const userReads: any[] = []; + const schemaOf = (n: string) => (n === OBJECT_DEF.name ? OBJECT_DEF : undefined); + const ql: any = { + executeAction, + userReads, + getSchema: schemaOf, + registry: { getObject: schemaOf, getItem: () => undefined }, + find: vi.fn(async (object: string, options?: any) => { + if (object === 'sys_user') { + userReads.push(options); + if (opts.throws) throw new Error('sys_user unavailable'); + return row ? [row] : []; + } + return [{ id: 'case_1', status: 'open' }]; + }), + insert: vi.fn(), update: vi.fn(), delete: vi.fn(), + }; + return ql; +} + +/** REST — `POST /actions/crm_case/close_case/case_1`. Returns the body ctx. */ +async function dispatchRest(ec: any, ql: any, context?: HttpProtocolContext) { + const kernel: any = { + context: { getService: (n: string) => (n === 'objectql' || n === 'data' ? ql : null) }, + }; + const ctx = context ?? ({ request: {}, environmentId: 'platform', executionContext: ec } as any); + const res: any = await new HttpDispatcher(kernel).handleActions( + '/crm_case/close_case/case_1', 'POST', {}, ctx, + ); + return { response: res.response, actionCtx: ql.executeAction.mock.calls[0]?.[2] }; +} + +/** MCP — `run_action`. Returns the body ctx. */ +async function dispatchMcp(ec: any, ql: any) { + const deps: any = { resolveService: async () => null, getObjectQL: async () => ql }; + await invokeBusinessAction(deps, { request: {} } as any, 'close_case', { recordId: 'case_1' }, { + driver: undefined, + envId: 'platform', + ec, + getMeta: () => ({ listObjects: async () => [OBJECT_DEF] }), + callData: async () => ({ record: { id: 'case_1' } }), + }); + return { actionCtx: ql.executeAction.mock.calls[0]?.[2] }; +} + +const AI_ROUTE = '/api/v1/ai/tools/:toolName/execute'; + +/** AI route — `POST /ai/tools/create_object/execute`. Returns the handler's `req.user`. */ +async function dispatchAi(ec: any, ql: any) { + const seen: { req?: any } = {}; + const deps = { + resolveService: (async (_c: any, name: string) => (name === 'ai' ? { chat: async () => ({}) } : undefined)) as any, + getObjectQL: async () => ql, + getRegisteredAiRoutes: () => [{ + method: 'POST', path: AI_ROUTE, auth: true, + handler: async (req: any) => { seen.req = req; return { status: 200, body: { success: true, data: {} } }; }, + }], + success: (data: any) => ({ status: 200, body: { success: true, data } }), + error: (message: string, httpStatus = 500) => ({ status: httpStatus, body: { success: false, error: { message } } }), + routeNotFound: (route: string) => ({ status: 404, body: { success: false, error: { route } } }), + } as unknown as DomainHandlerDeps; + await handleAIRequest( + deps, '/ai/tools/create_object/execute', 'POST', {}, {}, + { executionContext: ec } as unknown as HttpProtocolContext, + ); + return seen.req?.user; +} + +const DEV_ADMIN = { id: 'usr_admin', name: 'Dev Admin', email: 'admin@objectos.ai' }; + +describe('#5372 — the VALUE: ctx.user.name is sys_user.name, not the id', () => { + it('REST /actions — the path that was hardcoded to the id', async () => { + const { actionCtx } = await dispatchRest(makeEc(), makeQl(DEV_ADMIN)); + + expect(actionCtx.user.name).toBe('Dev Admin'); + expect(actionCtx.user.name).not.toBe(actionCtx.user.id); + expect(actionCtx.user.id).toBe('usr_admin'); + // The alias carries the SAME value — one name, two spellings, never two + // different answers. + expect(actionCtx.user.displayName).toBe('Dev Admin'); + }); + + it('MCP run_action — same value through the other dispatcher', async () => { + const { actionCtx } = await dispatchMcp(makeEc(), makeQl(DEV_ADMIN)); + + expect(actionCtx.user.name).toBe('Dev Admin'); + expect(actionCtx.user.displayName).toBe('Dev Admin'); + expect(actionCtx.user.id).toBe('usr_admin'); + }); + + it('AI route req.user — same value again', async () => { + const user = await dispatchAi(makeEc(), makeQl(DEV_ADMIN)); + + expect(user.name).toBe('Dev Admin'); + expect(user.displayName).toBe('Dev Admin'); + // `email` used to read `ec.userEmail`, a field ExecutionContext does + // not declare — permanently undefined. It reads the declared one now. + expect(user.email).toBe('admin@objectos.ai'); + }); + + it('a real sandboxed body reads it — end to end, dispatcher → VM', async () => { + // `buildActionSandboxContext` was never where the name was lost (it + // passes `actionCtx.user` through verbatim), so this closes the loop + // on the OTHER end: what an author actually writes in a body. + const ql = makeQl(DEV_ADMIN); + const { actionCtx } = await dispatchRest(makeEc(), ql); + const fn = actionBodyRunnerFactory(new QuickJSScriptRunner(), { ql, appId: 'crm' })({ + name: 'close_case', + object: 'crm_case', + type: 'script', + body: { language: 'js', source: 'return ctx.user.name;', capabilities: [] }, + } as any); + + await expect(fn!(actionCtx)).resolves.toBe('Dev Admin'); + }, 60_000); +}); + +describe('#5372 — the VALUE, other direction: name === id iff there is no display name', () => { + it('a sys_user row with no name falls back to the id', async () => { + const { actionCtx } = await dispatchRest(makeEc(), makeQl({ id: 'usr_admin', email: 'a@b.c' })); + + expect(actionCtx.user.name).toBe('usr_admin'); + expect(actionCtx.user.name).toBe(actionCtx.user.id); + }); + + it('a blank/whitespace name is no display name', async () => { + const { actionCtx } = await dispatchRest(makeEc(), makeQl({ id: 'usr_admin', name: ' ' })); + + expect(actionCtx.user.name).toBe('usr_admin'); + }); + + it('no sys_user row at all (a principal with no profile) falls back to the id', async () => { + const { actionCtx } = await dispatchRest(makeEc(), makeQl(undefined)); + + expect(actionCtx.user.name).toBe('usr_admin'); + }); + + it('an anonymous / self-invoked dispatch is the `system` principal, unchanged (#2701)', async () => { + const { actionCtx } = await dispatchRest(undefined, makeQl(DEV_ADMIN)); + + expect(actionCtx.user.id).toBe('system'); + expect(actionCtx.user.name).toBe('system'); + // …and it still carries the empty authority arrays, so a body reads + // "holds nothing" rather than needing a `?? []`. + expect(actionCtx.user.permissions).toEqual([]); + expect(actionCtx.user.systemPermissions).toEqual([]); + }); +}); + +describe('#5372 — the FAILURE MODE: an unresolvable name is quiet', () => { + it('a failing sys_user read falls back to the id and the action still runs', async () => { + const ql = makeQl(DEV_ADMIN, { throws: true }); + const { response, actionCtx } = await dispatchRest(makeEc(), ql); + + expect(response.status).toBe(200); + expect(actionCtx.user.name).toBe('usr_admin'); + }); + + it('an engine with no `find` at all does not break the dispatch', async () => { + const ql = makeQl(DEV_ADMIN); + delete (ql as any).find; + // The record pre-load needs `find` too, so this also proves the name + // resolution is not what turns a degraded engine into a 500. + const { response, actionCtx } = await dispatchRest(makeEc(), ql); + + expect(response.status).toBe(200); + expect(actionCtx.user.name).toBe('usr_admin'); + }); + + it('the read is system-elevated — resolving WHO the caller is cannot depend on their own grants', async () => { + const ql = makeQl(DEV_ADMIN); + await dispatchRest(makeEc(), ql); + + expect(ql.userReads[0]).toMatchObject({ + where: { id: 'usr_admin' }, limit: 1, context: { isSystem: true }, + }); + }); + + it('resolves ONCE per request, however many actions the request dispatches', async () => { + const ql = makeQl(DEV_ADMIN); + const ec = makeEc(); + // One ExecutionContext object = one inbound request. The memo is keyed + // on its identity, so nothing is cached across requests and a renamed + // user is correct on their very next one. + const context: any = { request: {}, environmentId: 'platform', executionContext: ec }; + await dispatchRest(ec, ql, context); + await dispatchRest(ec, ql, context); + + expect(ql.userReads.length).toBe(1); + expect(ql.executeAction.mock.calls.length).toBe(2); + }); +}); + +describe('#5372 — the SHAPE: one key set across every producer', () => { + it('REST, MCP and the AI route agree key-for-key', async () => { + const ec = makeEc(); + const rest = (await dispatchRest(ec, makeQl(DEV_ADMIN))).actionCtx.user; + const mcp = (await dispatchMcp(makeEc(), makeQl(DEV_ADMIN))).actionCtx.user; + const ai = await dispatchAi(makeEc(), makeQl(DEV_ADMIN)); + + const keys = (u: any) => Object.keys(u).sort(); + expect(keys(mcp)).toEqual(keys(rest)); + expect(keys(ai)).toEqual(keys(rest)); + // The EvalUser core (ADR-0068 D1: id/name/email/positions/ + // isPlatformAdmin/organizationId) plus the two transport channels and + // the id/name aliases the dispatch surfaces already published. + expect(keys(rest)).toEqual([ + 'displayName', 'email', 'id', 'isPlatformAdmin', 'name', 'organizationId', + 'permissions', 'positions', 'roles', 'systemPermissions', 'userId', + ]); + }); + + it('and value-for-value, for one and the same caller', async () => { + const rest = (await dispatchRest(makeEc(), makeQl(DEV_ADMIN))).actionCtx.user; + const mcp = (await dispatchMcp(makeEc(), makeQl(DEV_ADMIN))).actionCtx.user; + const ai = await dispatchAi(makeEc(), makeQl(DEV_ADMIN)); + + expect(mcp).toEqual(rest); + expect(ai).toEqual(rest); + expect(rest).toEqual({ + id: 'usr_admin', + userId: 'usr_admin', + name: 'Dev Admin', + displayName: 'Dev Admin', + email: 'admin@objectos.ai', + positions: ['platform_admin'], + roles: ['platform_admin'], + // Derived by `createEvalUser`, never stored — ADR-0068 D2. + isPlatformAdmin: true, + permissions: ['admin_full_access'], + systemPermissions: ['manage_metadata'], + organizationId: 'org_1', + }); + }); + + it('the ADR-0090 position aliases stay in lockstep (`roles` is `positions`)', async () => { + const { actionCtx } = await dispatchRest(makeEc({ positions: ['sales_rep'] }), makeQl(DEV_ADMIN)); + + expect(actionCtx.user.positions).toEqual(['sales_rep']); + expect(actionCtx.user.roles).toEqual(actionCtx.user.positions); + }); +}); diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index 97653f08c0..4e23305a20 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -19,6 +19,7 @@ import { validateActionParams, type ResolvedActionParam } from '@objectstack/spe import type { ExecutionContext } from '@objectstack/spec/kernel'; import type { IObjectQLEngine, ServiceSlotContract, ServiceSlotContracts } from '@objectstack/spec/contracts'; import { checkApiExposure } from './api-exposure.js'; +import { actorUserFromExecutionContext, resolveActorDisplayName } from './security/actor-user.js'; import type { HttpProtocolContext } from './http-dispatcher.js'; import { GLOBAL_ACTION_OBJECT_KEY, @@ -872,17 +873,23 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, } if (record && (record as any).id == null && recordId) (record as any).id = recordId; - const user = ec?.userId - ? { - id: ec.userId, - name: ec.userName ?? ec.userDisplayName ?? ec.userId, - // `organizationId` is the blessed name for the caller's active - // org (matches columns + `current_user.organizationId`); the - // action body executes TRUSTED (RLS-bypassing), so a body that - // wants to scope by org must read it here (#3280). - ...(ec.tenantId != null ? { organizationId: String(ec.tenantId) } : {}), - } - : { id: 'system', name: 'system' }; + // [#5372] One shared producer for the user shape (`security/actor-user.ts`), + // the same one the REST `/actions` route and the AI routes use. What stood + // here was `name: ec.userName ?? ec.userDisplayName ?? ec.userId` — a `??` + // chain over two fields `ExecutionContextSchema` never declared and nothing + // in the repo ever assigned, so its only reachable arm was the id (#4984's + // dead-limb family). The name now comes from `sys_user.name`, resolved once + // per request. `organizationId` is the blessed name for the caller's active + // org (matches columns + `current_user.organizationId`); the action body + // executes TRUSTED (RLS-bypassing), so a body that wants to scope by org + // must read it here (#3280). + const user = actorUserFromExecutionContext( + ec, + await resolveActorDisplayName( + async () => driver ?? await deps.getObjectQL(requestContext, envId), + ec, + ), + ); // ── flow dispatch ── (shared with the REST /actions route, #3915) if (action.type === 'flow') { diff --git a/packages/runtime/src/dispatcher-plugin.ts b/packages/runtime/src/dispatcher-plugin.ts index b0eb3955d9..b56963bf55 100644 --- a/packages/runtime/src/dispatcher-plugin.ts +++ b/packages/runtime/src/dispatcher-plugin.ts @@ -19,6 +19,7 @@ import { type SecurityHeadersOptions, } from './security/index.js'; import { resolveSessionData, resolveSessionPrincipalId } from './security/resolve-session-principal.js'; +import { buildActorUser } from './security/actor-user.js'; import { NoopMetricsRegistry, NoopErrorReporter, @@ -1518,16 +1519,24 @@ export function createDispatcherPlugin(config: DispatcherPluginConfig = {}): Plu // `dispatcher.dispatch()` EARLIER in this same `start()`, so // the ExecutionContext-backed path is the one that answers a // real `/api/v1/ai/...` request. - return { - userId, + // + // [#5372] Built by the shared producer so the two AI-route + // `req.user` producers agree on the key set by + // CONSTRUCTION rather than by two literals being kept in + // sync by hand — that is exactly how the three dispatch + // paths drifted. The display name is the session's own + // `user.name` (better-auth's projection of `sys_user.name`, + // the same authority the ExecutionContext-backed producer + // reads), so no extra lookup happens here. Its former + // `?? user.email` middle arm is gone: `name === id` now + // means "no display name" on EVERY path, and the address + // is still served under its own `email` key. + return buildActorUser({ id: userId, - displayName: sessionData?.user?.name ?? sessionData?.user?.email ?? userId, + displayName: sessionData?.user?.name, email: sessionData?.user?.email, - positions: [], - permissions: [], - systemPermissions: [], organizationId: sessionData?.session?.activeOrganizationId, - }; + }); } catch { return undefined; } diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index bfec89c0c5..a81605a113 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -49,6 +49,7 @@ */ import * as actionExec from '../action-execution.js'; +import { actorUserFromExecutionContext, resolveActorDisplayName } from '../security/actor-user.js'; import { validationFailureDetails } from '../validation-failure.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -272,21 +273,22 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string // roles (ADR-0090 `positions`, formerly `roles`) so a handler can branch // on identity and enforce ownership. Falls back to a `system` principal // only for a genuinely anonymous / self-invoked call (#2701). + // + // [#5372] The SHAPE is built by the one shared producer + // (`security/actor-user.ts`) that the MCP `run_action` and AI-route paths + // also use — three hand-rolled literals had drifted into three different + // user shapes. `name` in particular was hardcoded to `ec.userId` here: a + // declared key delivering a plausible WRONG value, which no consumer-side + // fallback can detect. It now carries `sys_user.name`, resolved once per + // request (falling back to the id, quietly, when there is none). + // `organizationId` remains the blessed developer-facing name for the + // caller's active org (matches columns + `current_user.organizationId`); + // the deprecated `tenantId` alias (#3280) was removed in v11 (#3290). const ec: any = _context?.executionContext; - const userFromAuth = ec?.userId - ? { - id: ec.userId, - name: ec.userId, - email: ec.email, - roles: Array.isArray(ec.positions) ? ec.positions : [], - positions: Array.isArray(ec.positions) ? ec.positions : [], - permissions: Array.isArray(ec.permissions) ? ec.permissions : [], - // `organizationId` is the blessed developer-facing name for the - // caller's active org (matches columns + `current_user.organizationId`). - // The deprecated `tenantId` alias (#3280) was removed in v11 (#3290). - organizationId: ec.tenantId, - } - : { id: 'system', name: 'system', roles: [], positions: [], permissions: [] }; + const userFromAuth = actorUserFromExecutionContext( + ec, + await resolveActorDisplayName(() => ql, ec), + ); const actionContext: any = { record, diff --git a/packages/runtime/src/domains/ai-request-user-capability-channel.test.ts b/packages/runtime/src/domains/ai-request-user-capability-channel.test.ts index ebd37b9959..8a0d7970bc 100644 --- a/packages/runtime/src/domains/ai-request-user-capability-channel.test.ts +++ b/packages/runtime/src/domains/ai-request-user-capability-channel.test.ts @@ -95,7 +95,11 @@ describe('#4705 — /ai/* req.user carries the capability channel', () => { // `manage_metadata` / `studio.access` / `setup.access` there). const { user, result } = await dispatchToolExecute({ userId: 'usr_admin', - userEmail: 'admin@objectos.ai', + // [#5372] Was `userEmail:` — a spelling `ExecutionContextSchema` + // does not declare and nothing ever assigned, matching the equally + // undeclared key `domains/ai.ts` used to read. The declared field + // is `email`; the fixture states a real ExecutionContext now. + email: 'admin@objectos.ai', positions: ['platform_admin'], permissions: ['admin_full_access', 'ai_seat'], systemPermissions: ['manage_users', 'manage_metadata', 'studio.access', 'setup.access'], @@ -183,7 +187,7 @@ function makeFakeServer() { }; } -function makeCtx(fakeServer: any, aiRoutes: any[], onRequest: (req: any) => void) { +function makeCtx(fakeServer: any, aiRoutes: any[], onRequest: (req: any) => void, session?: any) { const kernel: any = { getService: () => undefined, getServiceAsync: async () => undefined, @@ -196,7 +200,7 @@ function makeCtx(fakeServer: any, aiRoutes: any[], onRequest: (req: any) => void }; const authService: any = { api: { - getSession: async () => ({ + getSession: async () => session ?? ({ user: { id: 'usr_admin', name: 'Admin', email: 'admin@objectos.ai' }, session: { activeOrganizationId: 'org_1' }, }), @@ -235,6 +239,40 @@ describe('#4705 — the concrete-mount producer agrees on the shape', () => { // shape as the dispatch path, so a consumer needs no `?? []`. expect(seen.user.permissions).toEqual([]); expect(seen.user.systemPermissions).toEqual([]); + // [#5372] …and the SAME key set, including the identity keys. This + // producer reads the session's own `user.name` (better-auth's + // projection of `sys_user.name`), so it needs no lookup of its own — + // but it must answer under both spellings, like every other path. + expect(seen.user.name).toBe('Admin'); + expect(seen.user.displayName).toBe('Admin'); + expect(Object.keys(seen.user).sort()).toEqual([ + 'displayName', 'email', 'id', 'isPlatformAdmin', 'name', 'organizationId', + 'permissions', 'positions', 'roles', 'systemPermissions', 'userId', + ]); + }); + + it('[#5372] a session with no display name falls back to the id, not the email', async () => { + // The biconditional the dispatch paths hold to: `name === id` means + // "no display name", on every producer. The address stays reachable + // under its own key — it is not a stand-in for a name. + const { server, handlers } = makeFakeServer(); + let seen: any; + const ctx = makeCtx( + server, + [{ method: 'POST', path: '/ai/tools/:toolName/execute', description: 'x', auth: true }], + (req) => { seen = req; }, + { user: { id: 'usr_admin', email: 'admin@objectos.ai' }, session: {} }, + ); + const plugin = createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false }); + await plugin.start?.(ctx); + + const res = { status() { return res; }, header() { return res; }, json() { return res; } } as any; + await handlers[`POST ${TOOL_ROUTE}`]( + { headers: {}, body: {}, params: { toolName: 'create_object' }, query: {} }, res, + ); + + expect(seen.user.name).toBe('usr_admin'); + expect(seen.user.email).toBe('admin@objectos.ai'); }); it('mounts the /ai/* dispatch wildcard BEFORE the concrete AI routes', async () => { diff --git a/packages/runtime/src/domains/ai.ts b/packages/runtime/src/domains/ai.ts index ab864bfc75..f2ef25f559 100644 --- a/packages/runtime/src/domains/ai.ts +++ b/packages/runtime/src/domains/ai.ts @@ -14,6 +14,7 @@ import { shouldDenyAnonymous, ANONYMOUS_DENY_STATUS, ANONYMOUS_DENY_CODE, ANONYMOUS_DENY_MESSAGE, } from '@objectstack/core'; import { isServiceServeable } from '../service-serveable.js'; +import { actorUserFromExecutionContext, resolveActorDisplayName } from '../security/actor-user.js'; import { capabilityUnavailable } from './unavailable.js'; import type { IAIService } from '@objectstack/spec/contracts'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; @@ -171,17 +172,30 @@ export async function handleAIRequest(deps: DomainHandlerDeps, subPath: string, // absent — `ExecutionContext.systemPermissions` is optional) becomes // `[]`, never `undefined`, so a consumer reads "holds nothing" instead // of having to tolerate a missing field. + // + // [#5372] The shape itself now comes from the ONE producer + // (`security/actor-user.ts`) shared with the REST `/actions` and MCP + // `run_action` dispatch paths, and it fixes two silent wrong values + // this literal carried: `displayName` read a `??` chain over + // `ec.userDisplayName` / `ec.userName`, neither of which + // `ExecutionContextSchema` declares and neither of which anything ever + // assigned (so it always served the raw id), and `email` read + // `ec.userEmail` — the declared field is `ec.email`, so `user.email` + // here was permanently `undefined`. `name` joins `displayName` (same + // value) so all three paths answer to one key set. + // + // Anonymous stays `undefined` rather than the action paths' `system` + // principal: an AI route handler distinguishes "no caller" by the + // absence of `user`, and this route only reaches anonymous when the + // deployment does not require auth. const user = ec?.userId - ? { - userId: ec.userId, - id: ec.userId, - displayName: ec.userDisplayName ?? ec.userName ?? ec.userId, - email: ec.userEmail, - roles: Array.isArray(ec.positions) ? ec.positions : [], - permissions: Array.isArray(ec.permissions) ? ec.permissions : [], - systemPermissions: Array.isArray(ec.systemPermissions) ? ec.systemPermissions : [], - organizationId: ec.tenantId, - } + ? actorUserFromExecutionContext( + ec, + await resolveActorDisplayName( + () => deps.getObjectQL(context, context?.environmentId), + ec, + ), + ) : undefined; const result = await route.handler({ diff --git a/packages/runtime/src/security/actor-user.ts b/packages/runtime/src/security/actor-user.ts new file mode 100644 index 0000000000..37e4f30614 --- /dev/null +++ b/packages/runtime/src/security/actor-user.ts @@ -0,0 +1,231 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * actor-user — the ONE producer of the caller's `user` shape for the three + * dispatch paths that hand it to customer code (#5372). + * + * Before this module the shape was open-coded three times and the three + * disagreed, in the worst possible way — not by omitting a key, but by + * DELIVERING A PLAUSIBLE WRONG VALUE under a declared one: + * + * - REST `/actions` (domains/actions.ts) hardcoded `name: ec.userId`; + * - MCP `run_action` (action-execution.ts) read + * `ec.userName ?? ec.userDisplayName ?? ec.userId` — neither alias is + * declared on `ExecutionContextSchema` and nothing in the repo ever + * assigned either, so the chain always landed on the id (the #4984 + * dead-limb family: a `??` chain whose only reachable arm is the last); + * - the AI routes (domains/ai.ts) spelled the key `displayName` (same dead + * chain behind it) and read the caller's address off `ec.userEmail`, + * which is not the declared field either (it is `ec.email`), so + * `user.email` on that path was permanently `undefined`. + * + * `ctx.user.name` reads as a plausible string, so no consumer-side `??` can + * detect that it is the wrong string — an app that trusts the declared key + * writes opaque ids into user-facing surfaces (hotcrm's activity timeline did + * exactly that, objectstack-ai/hotcrm#673). One producer, one shape, one + * resolution of the display name is the structural fix; a fallback in each + * consumer would have been the #12 anti-pattern. + * + * `sys_user.name` is the authority: it is the platform's own profile + * display-name column (`required: true` on the object, and the one field + * besides `image` that plugin-auth's identity write guard lets a user edit). + * Resolution is quiet by construction — a missing row, an unavailable engine + * or a blank name falls back to the id. A dispatch must not fail because a + * display name could not be read. + * + * [ADR-0068 D1] The shape is not invented here. `EvalUser` is the spec's one + * user-context contract, mounted under `current_user` / `user` / `ctx.user` on + * the predicate surface (`formula/stdlib.ts` `buildScope`) — and `name` is + * declared on it as "Display name". So the REST hardcode was not merely + * inconsistent with its sibling dispatchers, it was serving something else + * under a key the spec had already defined. This module builds the identity + * core through the SAME `createEvalUser` factory and extends it with the two + * transport channels the dispatch surfaces publish, so an author's `ctx.user` + * means one thing whether they are writing a predicate or a body. + */ + +import { createEvalUser, type EvalUser } from '@objectstack/spec/identity'; + +/** + * The user envelope handed to an action body (`ctx.user`) and to AI route + * handlers (`req.user`). + * + * [ADR-0068 D1] Its identity core IS an `EvalUser` — the spec's one + * user-context contract, built through the same `createEvalUser` factory the + * formula/predicate surface mounts under `current_user` / `user` / `ctx.user`. + * That is deliberate and load-bearing: an action declares a `visible` + * predicate and a `body` side by side, both spelled `ctx.user`, and ADR-0068 + * exists precisely because the platform once had three different user shapes + * under one name. A body reading `ctx.user.name` (or `.positions`, or + * `.isPlatformAdmin`) must see what the predicate one line above sees. + * + * The extra keys are the TRANSPORT's, not the contract's: the id/name aliases + * the two dispatch surfaces already published, and the two authority channels + * (`permissions` = permission-SET names, `systemPermissions` = CAPABILITIES — + * separate channels, never merged, #4705). They extend the EvalUser core; they + * never contradict it. + * + * Every key is always present with a defined value except `email` / + * `organizationId`, which are genuinely optional facts about the caller. The + * arrays in particular are never `undefined`, so a consumer reads "holds + * nothing" instead of needing a `?? []` to tell absence from emptiness. + */ +export interface ActorUser extends EvalUser { + /** The acting user's id (`sys_user.id`), or `system` for a self-invoked call. */ + id: string; + /** + * Alias of {@link id}. Pre-existing on the AI-route shape (both producers + * emitted it), carried into the unified shape rather than dropped — this + * change fixes a wrong VALUE, it does not get to retire a key consumers + * may already read. + */ + userId: string; + /** + * The acting user's display name (`sys_user.name`). Falls back to the id + * when the platform cannot resolve one — so `name === id` means exactly + * "this user has no resolvable display name", never "the dispatcher + * forgot to look". (`EvalUser.name` is optional; on this surface it is + * always populated, because a body that has to handle `undefined` will + * print `undefined` into a timeline exactly once and then grow a + * workaround.) + */ + name: string; + /** Alias of {@link name} — the spelling AI route handlers already read. */ + displayName: string; + /** ADR-0090 position names held by the caller (canonical; `EvalUser.positions`). */ + positions: string[]; + /** Legacy alias of {@link positions} (pre-ADR-0090 spelling, kept for the REST/AI shapes). */ + roles: string[]; + /** Permission-SET names (`admin_full_access`, `ai_seat`, …). */ + permissions: string[]; + /** CAPABILITIES (`manage_metadata`, `studio.access`, …) — a separate channel (#4705). */ + systemPermissions: string[]; +} + +/** The principal a genuinely anonymous / self-invoked dispatch runs as (#2701). */ +const SYSTEM_ACTOR_ID = 'system'; + +/** + * Per-request memo for the display-name read, keyed on the request's + * ExecutionContext object identity — the envelope `resolveExecutionContext` + * builds once per inbound request. So N action dispatches inside one request + * cost ONE `sys_user` read, and nothing is cached across requests (a renamed + * user is correct on their very next request, with no invalidation hook to + * forget). Entries die with the context. + */ +const displayNameByContext = new WeakMap>(); + +/** A lazily-resolved data engine — the callers reach theirs differently. */ +export type QlGetter = () => Promise | any; + +async function readDisplayName(getQl: QlGetter, userId: string): Promise { + try { + const ql: any = await getQl(); + if (!ql || typeof ql.find !== 'function') return undefined; + // System-elevated, exactly like the shared authz resolver's own + // `sys_user` read (core/security/resolve-authz-context.ts): resolving + // WHO the caller is must not depend on the caller holding read + // permission on the identity table. + let rows: any = await ql.find('sys_user', { + where: { id: userId }, + limit: 1, + context: { isSystem: true }, + }); + if (rows && rows.value) rows = rows.value; + if (!Array.isArray(rows)) return undefined; + const row = rows.find((r: any) => r?.id === userId) ?? rows[0]; + const name = row?.name; + return typeof name === 'string' && name.trim().length > 0 ? name.trim() : undefined; + } catch { + // Quiet by design — see the module header. + return undefined; + } +} + +/** + * Resolve the acting user's display name from `sys_user.name`, once per + * request. Returns `undefined` when there is no user, no engine, no row, or + * no name — callers fall back to the id via {@link actorUserFromExecutionContext}. + */ +export async function resolveActorDisplayName(getQl: QlGetter, ec: any): Promise { + const userId = ec?.userId; + if (typeof userId !== 'string' || userId.length === 0) return undefined; + const cacheKey: object | undefined = ec && typeof ec === 'object' ? ec : undefined; + if (cacheKey) { + const cached = displayNameByContext.get(cacheKey); + if (cached) return cached; + } + const pending = readDisplayName(getQl, userId); + if (cacheKey) displayNameByContext.set(cacheKey, pending); + return pending; +} + +/** Normalize an unknown into a string array (never `undefined`). */ +function strings(v: unknown): string[] { + return Array.isArray(v) ? v.filter((x): x is string => typeof x === 'string') : []; +} + +/** + * Build the unified user envelope. Pass no `id` (an anonymous / self-invoked + * dispatch) to get the `system` principal in the SAME shape, so a body never + * has to branch on which principal it got. + */ +export function buildActorUser(input?: { + id?: string; + displayName?: string; + email?: string; + organizationId?: string; + positions?: unknown; + permissions?: unknown; + systemPermissions?: unknown; +}): ActorUser { + const id = + typeof input?.id === 'string' && input.id.length > 0 ? input.id : SYSTEM_ACTOR_ID; + const anonymous = id === SYSTEM_ACTOR_ID && !input?.id; + const name = + !anonymous && typeof input?.displayName === 'string' && input.displayName.trim().length > 0 + ? input.displayName.trim() + : id; + // [ADR-0068 D1] The identity core comes from the spec's ONE factory, so + // `positions` normalization and the `isPlatformAdmin` derivation can never + // disagree with the predicate surface's copy of the same user. + const core = createEvalUser({ + id, + name, + email: anonymous ? undefined : (typeof input?.email === 'string' && input.email.length > 0 ? input.email : undefined), + positions: anonymous ? [] : strings(input?.positions), + organizationId: + !anonymous && typeof input?.organizationId === 'string' && input.organizationId.length > 0 + ? input.organizationId + : undefined, + }); + return { + ...core, + name, + userId: id, + displayName: name, + positions: core.positions, + roles: core.positions, + permissions: anonymous ? [] : strings(input?.permissions), + systemPermissions: anonymous ? [] : strings(input?.systemPermissions), + }; +} + +/** + * Map a resolved {@link ExecutionContext} (plus the display name resolved by + * {@link resolveActorDisplayName}) onto the unified shape. This is the single + * place that decides which ExecutionContext field feeds which user key — + * `email` (declared) rather than the `userEmail` that never existed, + * `tenantId` → `organizationId` (the blessed developer-facing name, #3280). + */ +export function actorUserFromExecutionContext(ec: any, displayName?: string): ActorUser { + return buildActorUser({ + id: typeof ec?.userId === 'string' ? ec.userId : undefined, + displayName, + email: typeof ec?.email === 'string' ? ec.email : undefined, + organizationId: ec?.tenantId != null ? String(ec.tenantId) : undefined, + positions: ec?.positions, + permissions: ec?.permissions, + systemPermissions: ec?.systemPermissions, + }); +}