From a7f0f9ba588e8ca63bc6e0c9baa81dc1545dd821 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 08:07:58 +0000 Subject: [PATCH 1/4] feat(plugin-auth): admin write surface for sys_user.manager_id WIP: endpoint, mount, ledger row and tests. Verification pending. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .../src/admin-set-user-manager.test.ts | 436 ++++++++++++++++ .../plugin-auth/src/admin-set-user-manager.ts | 476 ++++++++++++++++++ .../plugins/plugin-auth/src/auth-plugin.ts | 45 ++ .../src/auth-route-ledger.conformance.test.ts | 7 + .../plugin-auth/src/auth-route-ledger.ts | 1 + 5 files changed, 965 insertions(+) create mode 100644 packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts create mode 100644 packages/plugins/plugin-auth/src/admin-set-user-manager.ts diff --git a/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts b/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts new file mode 100644 index 0000000000..c17ac04291 --- /dev/null +++ b/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts @@ -0,0 +1,436 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `POST /api/v1/auth/admin/set-user-manager` (#16678). + * + * Two things this suite is built to prove, in order: + * + * 1. **The hole is closed, and closed the way the ruling says.** The column + * becomes writable through a product surface, and it does so under a + * SYSTEM context — so the ADR-0092 Tier-1 whitelist is asserted UNMOVED + * here as part of the fix, not merely left alone. A future change that + * "fixes" this by admitting `manager_id` to Tier 1 reds this file. + * 2. **Every refusal refuses, and refuses without writing.** Each refusal + * case asserts the ADR-0112 envelope (`error.code` + HTTP status + the + * `details.reason` discriminator) AND that the engine's `update` was + * never called. A bare status assertion is not a pin: "still 400" is what + * a handler that refuses everything also looks like, which is why the + * admissions below sit in the same file. + * + * The engine double pins its `update` to the real dispatch contract + * (`assertEngineUpdateDispatch`), so a handler that learned to write by + * predicate could not pass here while failing against `ObjectQL.update`. + */ + +import { describe, it, expect } from 'vitest'; +import { assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { + runSetUserManager, + MAX_MANAGER_CHAIN_DEPTH, + type SetUserManagerDeps, + type SetUserManagerEngine, +} from './admin-set-user-manager'; +import { SYS_USER_PROFILE_EDIT_FIELDS } from './sys-user-writable-fields'; +import type { AdminActor } from './admin-user-endpoints'; + +const ACTOR: AdminActor = { id: 'usr_admin', email: 'admin@example.com' }; + +type Row = Record; + +interface Recorded { + object: string; + data: Row; + options: Record | undefined; +} + +function makeEngine(tables: Record) { + const updates: Recorded[] = []; + const failReads = new Set(); + const engine: SetUserManagerEngine & { + updates: Recorded[]; + failReadsOn(object: string): void; + } = { + updates, + failReadsOn(object: string) { + failReads.add(object); + }, + async find(object: string, query?: unknown): Promise { + if (failReads.has(object)) throw new Error(`read of ${object} failed`); + const q = (query ?? {}) as { where?: Record; limit?: number }; + const rows = tables[object] ?? []; + const where = q.where ?? {}; + const out = rows.filter((r) => + Object.entries(where).every(([k, v]) => String(r[k] ?? '') === String(v ?? '')), + ); + return typeof q.limit === 'number' ? out.slice(0, q.limit) : out; + }, + async update(object: string, data: unknown, options?: unknown): Promise { + // The real engine's three-way dispatch — a double looser than this is + // no double at all. + assertEngineUpdateDispatch(data as Row, options as Record | undefined); + updates.push({ + object, + data: data as Row, + options: options as Record | undefined, + }); + const row = (tables[object] ?? []).find((r) => r.id === (data as Row).id); + if (row) Object.assign(row, data as Row); + return row ?? null; + }, + }; + return engine; +} + +function deps(engine: SetUserManagerEngine | undefined, warns: unknown[][] = []): SetUserManagerDeps { + return { + getDataEngine: () => engine, + logger: { warn: (msg, meta) => void warns.push([msg, meta]) }, + }; +} + +const post = (body: unknown): Request => + new Request('http://local/api/v1/auth/admin/set-user-manager', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }); + +/** A simple two-person org with no tenancy facts recorded. */ +const twoUsers = () => ({ + sys_user: [ + { id: 'u_report', manager_id: null, source: 'env_native' }, + { id: 'u_boss', manager_id: null, source: 'env_native' }, + ] as Row[], +}); + +describe('set-user-manager — the write surface the column never had (#16678)', () => { + it('the column had no product write surface, and the fix does NOT change that fact for Tier 1', () => { + // ⛔ The load-bearing half of the design: `manager_id` is reachable by + // CONTEXT, never by whitelist. ADR-0092 D5's amendment made Tier-1 + // membership imply SELF-editability, so admitting the column here would + // hand every member their own first-rung approver and a widening of their + // own `own_and_reports` read scope. ADR-0092 D4 depends on it too. + expect([...SYS_USER_PROFILE_EDIT_FIELDS].sort()).toEqual(['image', 'locale', 'name']); + expect(SYS_USER_PROFILE_EDIT_FIELDS).not.toContain('manager_id'); + }); + + it('SETS the manager, under a system context, attributed to the admin', async () => { + const engine = makeEngine(twoUsers()); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_report', managerId: 'u_boss' })); + + expect(res.status).toBe(200); + expect(res.body.success).toBe(true); + expect(res.body.data).toMatchObject({ userId: 'u_report', managerId: 'u_boss', setBy: 'usr_admin' }); + + // The preservation half: what the engine actually received. + expect(engine.updates).toHaveLength(1); + expect(engine.updates[0].object).toBe('sys_user'); + expect(engine.updates[0].data).toEqual({ id: 'u_report', manager_id: 'u_boss' }); + // `isSystem: true` is what carries this past the ADR-0092 identity write + // guard (`isUserContextWrite` is `Boolean(userId) && isSystem !== true`). + expect((engine.updates[0].options as { context?: { isSystem?: boolean } })?.context?.isSystem).toBe(true); + }); + + it('CLEARS the manager on an explicit null', async () => { + const engine = makeEngine({ + sys_user: [ + { id: 'u_report', manager_id: 'u_boss', source: 'env_native' }, + { id: 'u_boss', manager_id: null, source: 'env_native' }, + ], + }); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_report', managerId: null })); + + expect(res.status).toBe(200); + expect(engine.updates[0].data).toEqual({ id: 'u_report', manager_id: null }); + }); + + it('accepts the snake_case spellings the sibling ObjectStack mounts also read', async () => { + const engine = makeEngine(twoUsers()); + const res = await runSetUserManager(deps(engine), ACTOR, post({ user_id: 'u_report', manager_id: 'u_boss' })); + expect(res.status).toBe(200); + expect(engine.updates[0].data).toEqual({ id: 'u_report', manager_id: 'u_boss' }); + }); + + it('an ABSENT managerId is refused, never read as a clear', async () => { + // A payload that misspells the key must not silently unset an org chart. + const engine = makeEngine(twoUsers()); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_report', managerID: 'u_boss' })); + + expect(res.status).toBe(400); + expect(res.body.error?.code).toBe('INVALID_REQUEST'); + expect(res.body.error?.details?.reason).toBe('invalid_body'); + expect(engine.updates).toEqual([]); + }); + + it.each([ + ['no userId', { managerId: 'u_boss' }], + ['empty userId', { userId: '', managerId: 'u_boss' }], + ['non-string managerId', { userId: 'u_report', managerId: 42 }], + ['empty managerId', { userId: 'u_report', managerId: '' }], + ])('refuses a malformed body: %s', async (_label, body) => { + const engine = makeEngine(twoUsers()); + const res = await runSetUserManager(deps(engine), ACTOR, post(body)); + + expect(res.status).toBe(400); + expect(res.body.error?.code).toBe('INVALID_REQUEST'); + expect(res.body.error?.details?.reason).toBe('invalid_body'); + expect(engine.updates).toEqual([]); + }); + + it('refuses when no data engine is wired — loudly, not as a silent success', async () => { + const res = await runSetUserManager(deps(undefined), ACTOR, post({ userId: 'u_report', managerId: 'u_boss' })); + expect(res.status).toBe(503); + expect(res.body.error?.code).toBe('SERVICE_UNAVAILABLE'); + expect(res.body.error?.details?.reason).toBe('engine_unavailable'); + }); + + it('refuses an unknown target user', async () => { + const engine = makeEngine(twoUsers()); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_ghost', managerId: 'u_boss' })); + + expect(res.status).toBe(404); + expect(res.body.error?.code).toBe('RESOURCE_NOT_FOUND'); + expect(res.body.error?.details?.reason).toBe('user_not_found'); + expect(engine.updates).toEqual([]); + }); + + it('refuses an unknown proposed manager', async () => { + const engine = makeEngine(twoUsers()); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_report', managerId: 'u_ghost' })); + + expect(res.status).toBe(400); + expect(res.body.error?.code).toBe('INVALID_REFERENCE'); + expect(res.body.error?.details?.reason).toBe('manager_not_found'); + expect(engine.updates).toEqual([]); + }); +}); + +describe('set-user-manager — refusal 1: self-assignment', () => { + it('refuses a user as their own manager, and says why', async () => { + const engine = makeEngine(twoUsers()); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_report', managerId: 'u_report' })); + + expect(res.status).toBe(400); + expect(res.body.error?.code).toBe('INVALID_FIELD'); + expect(res.body.error?.details?.reason).toBe('self_assignment'); + // The reason is the whole point of the tier and is stated to the operator. + expect(res.body.error?.message).toContain('cannot be their own manager'); + expect(engine.updates).toEqual([]); + }); + + it('the refusal is about the TARGET, not about the admin editing himself', async () => { + // An admin setting his OWN manager to someone else is a legitimate write: + // the refusal is `userId === managerId`, never `userId === actor.id`. + const engine = makeEngine({ + sys_user: [ + { id: 'usr_admin', manager_id: null, source: 'env_native' }, + { id: 'u_boss', manager_id: null, source: 'env_native' }, + ], + }); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'usr_admin', managerId: 'u_boss' })); + expect(res.status).toBe(200); + }); +}); + +describe('set-user-manager — refusal 2: cycle, enforced AT THE WRITE', () => { + // Nothing downstream catches this: `ApprovalService.lookupManager` and + // `TeamGraphService.managerOf` each read ONE row, and the multi-hop + // resolver ships outside this repo. A loop admitted here is admitted + // permanently. + + it('refuses the two-hop loop (A -> B, then B -> A)', async () => { + const engine = makeEngine({ + sys_user: [ + { id: 'a', manager_id: 'b', source: 'env_native' }, + { id: 'b', manager_id: null, source: 'env_native' }, + ], + }); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'b', managerId: 'a' })); + + expect(res.status).toBe(409); + expect(res.body.error?.code).toBe('RESOURCE_CONFLICT'); + expect(res.body.error?.details?.reason).toBe('cycle'); + expect(engine.updates).toEqual([]); + }); + + it('refuses a loop closed several hops up (A -> B -> C, then C -> A)', async () => { + const engine = makeEngine({ + sys_user: [ + { id: 'a', manager_id: 'b', source: 'env_native' }, + { id: 'b', manager_id: 'c', source: 'env_native' }, + { id: 'c', manager_id: null, source: 'env_native' }, + ], + }); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'c', managerId: 'a' })); + + expect(res.status).toBe(409); + expect(res.body.error?.details?.reason).toBe('cycle'); + expect(engine.updates).toEqual([]); + }); + + it('ADMITS a diamond — two reports under one manager is not a cycle', async () => { + // The discriminating control: the walk must refuse loops, not every + // chain that reaches a shared ancestor. + const engine = makeEngine({ + sys_user: [ + { id: 'a', manager_id: 'top', source: 'env_native' }, + { id: 'b', manager_id: null, source: 'env_native' }, + { id: 'top', manager_id: null, source: 'env_native' }, + ], + }); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'b', managerId: 'top' })); + expect(res.status).toBe(200); + expect(engine.updates).toHaveLength(1); + }); + + it('terminates on a loop that ALREADY exists above the proposed manager', async () => { + // The walk carries its own `seen` set, so pre-existing corruption is + // reported as the data defect it is instead of hanging the request. + const engine = makeEngine({ + sys_user: [ + { id: 'x', manager_id: null, source: 'env_native' }, + { id: 'p', manager_id: 'q', source: 'env_native' }, + { id: 'q', manager_id: 'p', source: 'env_native' }, + ], + }); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'x', managerId: 'p' })); + + expect(res.status).toBe(409); + expect(res.body.error?.details?.reason).toBe('cycle'); + expect(res.body.error?.message).toContain('already contains a loop'); + expect(engine.updates).toEqual([]); + }); +}); + +describe('set-user-manager — refusal 3: depth', () => { + /** A straight chain `c0 -> c1 -> ... -> c{n-1}`, plus a free-standing `leaf`. */ + const chain = (n: number) => ({ + sys_user: [ + { id: 'leaf', manager_id: null, source: 'env_native' }, + ...Array.from({ length: n }, (_, i) => ({ + id: `c${i}`, + manager_id: i + 1 < n ? `c${i + 1}` : null, + source: 'env_native', + })), + ] as Row[], + }); + + it(`admits a chain of exactly ${MAX_MANAGER_CHAIN_DEPTH} links`, async () => { + // Attaching `leaf` under `c0` makes 1 + (MAX - 1) = MAX links. + const engine = makeEngine(chain(MAX_MANAGER_CHAIN_DEPTH - 1)); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'leaf', managerId: 'c0' })); + expect(res.status).toBe(200); + }); + + it('refuses the link that would cross the cap', async () => { + const engine = makeEngine(chain(MAX_MANAGER_CHAIN_DEPTH)); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'leaf', managerId: 'c0' })); + + expect(res.status).toBe(400); + expect(res.body.error?.code).toBe('VALUE_OUT_OF_RANGE'); + expect(res.body.error?.details?.reason).toBe('max_depth_exceeded'); + expect(res.body.error?.message).toContain(String(MAX_MANAGER_CHAIN_DEPTH)); + expect(engine.updates).toEqual([]); + }); +}); + +describe('set-user-manager — refusal 4: cross-organization', () => { + const membership = (rows: Array<{ user_id: string; organization_id: string }>) => ({ + sys_user: [ + { id: 'u_report', manager_id: null, source: 'env_native' }, + { id: 'u_boss', manager_id: null, source: 'env_native' }, + ] as Row[], + sys_member: rows as unknown as Row[], + }); + + it('refuses a manager whose memberships are DISJOINT from the user’s', async () => { + const engine = makeEngine( + membership([ + { user_id: 'u_report', organization_id: 'org_a' }, + { user_id: 'u_boss', organization_id: 'org_b' }, + ]), + ); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_report', managerId: 'u_boss' })); + + expect(res.status).toBe(400); + expect(res.body.error?.code).toBe('INVALID_REFERENCE'); + expect(res.body.error?.details?.reason).toBe('cross_organization'); + expect(engine.updates).toEqual([]); + }); + + it('ADMITS a manager who shares one organization, even while holding others', async () => { + const engine = makeEngine( + membership([ + { user_id: 'u_report', organization_id: 'org_a' }, + { user_id: 'u_boss', organization_id: 'org_b' }, + { user_id: 'u_boss', organization_id: 'org_a' }, + ]), + ); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_report', managerId: 'u_boss' })); + expect(res.status).toBe(200); + }); + + it('an ABSENT tenancy fact is not a negative one — the link is admitted', async () => { + // Mirrors `managerIsProvablyOutsideOrg`'s ruled posture: a stack that + // stamps organizations on requests but never materializes `sys_member` + // rows would otherwise lose every manager link at once. + const engine = makeEngine(membership([{ user_id: 'u_report', organization_id: 'org_a' }])); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_report', managerId: 'u_boss' })); + expect(res.status).toBe(200); + }); + + it('a FAILED tenancy read says so once, and does not invent an empty membership', async () => { + const engine = makeEngine( + membership([ + { user_id: 'u_report', organization_id: 'org_a' }, + { user_id: 'u_boss', organization_id: 'org_b' }, + ]), + ); + engine.failReadsOn('sys_member'); + const warns: unknown[][] = []; + const res = await runSetUserManager(deps(engine, warns), ACTOR, post({ userId: 'u_report', managerId: 'u_boss' })); + + // Degraded, not silently degraded: the link goes through (the screen could + // not answer) and the consequence plus the remedy are stated once. + expect(res.status).toBe(200); + expect(warns).toHaveLength(1); + expect(String(warns[0][0])).toContain('could not read sys_member'); + expect(String(warns[0][0])).toContain('Remedy'); + }); +}); + +describe('set-user-manager — refusal 5: the directory owns this identity', () => { + const provisioned = () => ({ + sys_user: [ + { id: 'u_sso', manager_id: 'u_old', source: 'idp_provisioned' }, + { id: 'u_boss', manager_id: null, source: 'env_native' }, + { id: 'u_old', manager_id: null, source: 'env_native' }, + ] as Row[], + }); + + it('refuses a SET on an idp_provisioned identity, naming the sync as the surface', async () => { + const engine = makeEngine(provisioned()); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_sso', managerId: 'u_boss' })); + + expect(res.status).toBe(403); + expect(res.body.error?.code).toBe('PERMISSION_DENIED'); + expect(res.body.error?.details?.reason).toBe('idp_provisioned'); + expect(res.body.error?.message).toContain('directory'); + expect(engine.updates).toEqual([]); + }); + + it('refuses the CLEAR too — a value the next sync reverts is the shape ADR-0049 refuses', async () => { + const engine = makeEngine(provisioned()); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_sso', managerId: null })); + + expect(res.status).toBe(403); + expect(res.body.error?.details?.reason).toBe('idp_provisioned'); + expect(engine.updates).toEqual([]); + }); + + it('an env_native identity is unaffected — the control for the screen above', async () => { + const engine = makeEngine(provisioned()); + const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'u_old', managerId: 'u_boss' })); + expect(res.status).toBe(200); + }); +}); diff --git a/packages/plugins/plugin-auth/src/admin-set-user-manager.ts b/packages/plugins/plugin-auth/src/admin-set-user-manager.ts new file mode 100644 index 0000000000..80f36ea1b4 --- /dev/null +++ b/packages/plugins/plugin-auth/src/admin-set-user-manager.ts @@ -0,0 +1,476 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `POST /api/v1/auth/admin/set-user-manager` — the admin write surface for + * `sys_user.manager_id` (#16678 Phase 3, ruled as option B on the Phase 2 + * design: the manager is the relation an ADMIN sets explicitly on the user, + * and `sys_business_unit.manager_user_id` — the unit head — stays independent + * of it; nothing here derives one from the other). + * + * ## The hole this closes, measured on the tree it was written against + * + * `sys_user.manager_id` drives two shipped behaviours — the approvals + * `{ type: 'manager' }` rung (`lookupManager`) and the `own_and_reports` read + * scope (ADR-0057) — and until this endpoint **no product surface could write + * it**: + * + * - the generic data path refuses it: `sys_user` is `managedBy: + * 'better-auth'` and the ADR-0092 D2 managed-update whitelist is + * `{name, image, locale}` (`SYS_USER_PROFILE_EDIT_FIELDS`); + * - the admin bulk import does not carry it: `admin-import-users.ts` matches + * `manager_id` 0 times, against a control of `phone_number` 8; + * - the Console renders no field for it (`readonly: true` on the column). + * + * So the rung expanded to nobody on every record in any install without a + * directory sync, and the only population route was a seed or another + * system-context write. + * + * ## Why a dedicated operation rather than a column on a profile payload + * + * The measured precedents make this write its own operation — a dedicated + * reference endpoint with the property read-only on the user resource. This + * repo already has that shape: `POST /api/v1/auth/admin/unlock-user` and + * `POST /api/v1/auth/admin/import-users`, both ObjectStack mounts on the raw + * app ahead of the better-auth catch-all, both platform-admin gated + * (ADR-0068), both ledgered in `auth-route-ledger.ts`. + * + * ⛔ It is NOT admitted to ADR-0092 Tier 1, and the design turns on that: + * since the ADR-0092 D5 amendment, Tier-1 membership implies SELF-editability + * (the `sys_user_self` policy widened from `select` to `all`), so admitting + * `manager_id` there would hand every member their own first-rung approver + * and a widening of their own `own_and_reports` read scope in one move. + * ⇒ `SYS_USER_PROFILE_EDIT_FIELDS` is untouched, + * `MANAGED_EXTENSION_EDITABLE_FIELDS.sys_user` stays `{locale}`, the column + * keeps `readonly: true`, and ADR-0092 D4 ("every non-Tier-1 field renders + * non-editable in the standard edit form") stays true by construction. This + * endpoint reaches the column **by system context**, the same way + * `admin-import-users` already reaches `phone_number` and `role`: both write + * guards gate on `isUserContextWrite`, spelled `Boolean(userId) && isSystem + * !== true`, so a system-context write bypasses the whitelist by + * construction and no whitelist entry is added anywhere. + * + * ## The refusals, and why each one is HERE + * + * Every refusal below is enforced AT THE WRITE. That is not belt-and-braces + * for the cycle check, it is the only enforcement that exists: the only + * manager-chain walkers in the open tree are single-hop + * (`ApprovalService.lookupManager` reads one row; `TeamGraphService.managerOf` + * reads one row), and the multi-hop `subordinate_user_ids` resolver ships + * only in `@objectstack/security-enterprise`, outside this repo. Nothing + * downstream will catch a loop this endpoint lets in. + * + * 1. **Self-assignment** (`userId === managerId`) — refused outright. An + * approver rung that resolves to the submitter is the whole failure this + * tier exists to prevent. + * 2. **Cycle** — the proposed manager's own chain is walked upward and the + * link is refused when it closes a loop. The walk is itself cycle-safe (a + * `seen` set), so a loop that ALREADY exists upstream is reported rather + * than hung on. + * 3. **Depth** — refused past {@link MAX_MANAGER_CHAIN_DEPTH}. ADR-0057 D3 + * requires the hierarchy rollups it feeds to be "bounded (hard cap + + * cache, mirroring the `org_user_ids` cap)"; an unbounded chain written + * here is an unbounded rollup there. The reference resolver in + * `packages/qa/dogfood` bounds itself at 20 levels, and this cap is the + * same number so a chain this endpoint accepts cannot exceed what that + * resolver walks. + * 4. **Cross-organization** — refused when the two identities are PROVABLY + * in disjoint organizations. `sys_user` carries no `organization_id` (it + * is a global identity table), so `sys_member` rows are the only tenancy + * fact either identity has; this mirrors `managerIsProvablyOutsideOrg`, + * which already drops such a manager at ROUTING time with a warning. Both + * halves are wanted: data drifts after a write, so the read-time screen + * stays the last line, and the write-time refusal is the only one an + * operator can act on at the moment they made the mistake. + * 5. **Directory-owned identity** — refused when the target is + * `source: 'idp_provisioned'`. The ruling picked "directory wins, per + * identity", keyed on the `sys_user.source` column the platform already + * stamps and already uses to hide three self-service identity actions. + * The rejected alternative was last-writer-wins, which stores a value the + * next sync silently reverts — a write that reports success and does not + * persist, which is exactly what ADR-0049 exists to refuse. + * + * ## Why these `error.code` values and not dedicated ones + * + * `error.code` is a CLOSED vocabulary — `StandardErrorCode` union + * `ERROR_CODE_LEDGER` — and both live in `packages/spec`, which this change + * is fenced out of. So every code below is one this package may already emit, + * and the machine-readable discrimination between refusals that share a code + * is carried by `error.details.reason`, whose values are pinned by this + * module's tests. A dedicated code per refusal would be the better shape and + * is recorded as a follow-up for the seat that owns the spec vocabulary; it + * is ⛔ not worth reaching over the fence for. + * + * | refusal | code | status | details.reason | + * |---------------------------|---------------------|--------|-----------------------| + * | body shape | INVALID_REQUEST | 400 | invalid_body | + * | target user missing | RESOURCE_NOT_FOUND | 404 | user_not_found | + * | proposed manager missing | INVALID_REFERENCE | 400 | manager_not_found | + * | self-assignment | INVALID_FIELD | 400 | self_assignment | + * | cycle | RESOURCE_CONFLICT | 409 | cycle | + * | depth cap | VALUE_OUT_OF_RANGE | 400 | max_depth_exceeded | + * | cross-organization | INVALID_REFERENCE | 400 | cross_organization | + * | directory-owned identity | PERMISSION_DENIED | 403 | idp_provisioned | + * + * Authorization is the caller's, not this module's: the mount runs the + * ADR-0068 platform-admin gate (`judgePlatformAdmin`) and hands the actor in + * already judged, identical to every other `/api/v1/auth/admin/*` route in + * the ledger. The finer "maintain the org chart in my subtree" axis is a + * recorded follow-up and ⛔ is deliberately NOT declared here: ADR-0049 + * forbids declaring a permission that is not enforced, and the three axes + * `delegated-admin-gate.ts` ships today express no such thing. + */ + +import { authSystemWriteContext } from './auth-actor-attribution.js'; +import type { AdminActor, EndpointResult } from './admin-user-endpoints.js'; + +/** + * Hard cap on the reporting chain this endpoint will create, counted in links + * from the edited user upward. See refusal 3 above for why a cap exists at + * all and why this is the number. + */ +export const MAX_MANAGER_CHAIN_DEPTH = 20; + +/** The machine-readable discriminator carried on every refusal. */ +export type SetUserManagerRefusalReason = + | 'invalid_body' + | 'user_not_found' + | 'manager_not_found' + | 'self_assignment' + | 'cycle' + | 'max_depth_exceeded' + | 'cross_organization' + | 'idp_provisioned' + | 'engine_unavailable'; + +/** The slice of the ObjectQL engine this endpoint needs. */ +export interface SetUserManagerEngine { + find(objectName: string, query?: unknown): Promise; + update(objectName: string, data: unknown, options?: unknown): Promise; +} + +export interface SetUserManagerDeps { + /** Resolves the live data engine; `undefined` when none is wired. */ + getDataEngine(): SetUserManagerEngine | undefined | null; + /** + * Functional-degradation channel. A tenancy read that FAILS leaves the + * cross-organization screen unable to answer, and this endpoint then routes + * as it would with no tenancy fact recorded — the same fail-open posture the + * routing-time screen states for itself. It says so once rather than + * degrading in silence. + */ + logger?: { warn(msg: string, meta?: unknown): void }; +} + +/** Result shape: `EndpointResult` plus the refusal discriminator. */ +export interface SetUserManagerResult { + status: number; + body: { + success: boolean; + data?: Record; + error?: { code: string; message: string; details?: { reason: SetUserManagerRefusalReason } }; + }; +} + +// A `SetUserManagerResult` is an `EndpointResult` with one optional extra +// field, so the mount can answer with either shape. Asserted structurally +// rather than declared, so the two cannot drift apart unnoticed. +const _assignableToEndpointResult: (r: SetUserManagerResult) => EndpointResult = (r) => r; +void _assignableToEndpointResult; + +type UserRow = { id?: unknown; manager_id?: unknown; source?: unknown }; + +function refuse( + status: number, + code: string, + reason: SetUserManagerRefusalReason, + message: string, +): SetUserManagerResult { + return { status, body: { success: false, error: { code, message, details: { reason } } } }; +} + +async function parseJson(request: Request): Promise> { + try { + const parsed = await request.json(); + return parsed && typeof parsed === 'object' ? (parsed as Record) : {}; + } catch { + return {}; + } +} + +/** + * Both spellings are read for each field, matching the sibling ObjectStack + * mounts: the Console posts the camelCase one (`recordIdParam: 'userId'`) and + * `unlock-user` has always also read the snake_case one. + */ +function readId(body: Record, camel: string, snake: string): unknown { + return body[camel] !== undefined ? body[camel] : body[snake]; +} + +const SYSTEM_READ_CTX = { isSystem: true, positions: [], permissions: [] } as const; + +async function findUser( + engine: SetUserManagerEngine, + userId: string, +): Promise { + const rows = await engine.find('sys_user', { + where: { id: userId }, + fields: ['id', 'manager_id', 'source'], + limit: 1, + context: SYSTEM_READ_CTX, + }); + const row = Array.isArray(rows) ? (rows[0] as UserRow | undefined) : undefined; + return row && row.id ? row : null; +} + +/** + * The organization ids an identity PROVABLY holds membership in. An empty + * array means "no tenancy fact recorded", which is not the same as "member of + * nothing" — see {@link screenCrossOrganization}. + */ +async function membershipOrgIds( + deps: SetUserManagerDeps, + engine: SetUserManagerEngine, + userId: string, +): Promise { + try { + const rows = await engine.find('sys_member', { + where: { user_id: userId }, + fields: ['user_id', 'organization_id'], + limit: 1000, + context: SYSTEM_READ_CTX, + }); + return (Array.isArray(rows) ? rows : []) + .map((r) => String((r as { organization_id?: unknown })?.organization_id ?? '')) + .filter(Boolean); + } catch (e) { + // Distinguishable by design: `null` is "the read did not happen", never an + // invented empty membership list that would read as a negative tenancy + // fact and refuse a legitimate link. + deps.logger?.warn( + 'AuthPlugin set-user-manager: the cross-organization screen could not read sys_member, so ' + + 'the link was accepted without it. The routing-time screen still applies at approval ' + + 'time. Remedy: make the sys_member read succeed and re-apply the link to have it ' + + 'screened at the write.', + { userId, error: e instanceof Error ? e.message : String(e) }, + ); + return null; + } +} + +/** + * Is the proposed manager PROVABLY outside every organization the edited user + * belongs to? + * + * "Provably" is the whole shape, and it is the routing-time screen's shape + * deliberately: both identities must have tenancy facts recorded, and those + * facts must be disjoint. Either side having none leaves the question + * unanswered, and an unanswered question routes as before — a stack that + * stamps organizations on requests but never materializes `sys_member` rows + * would otherwise have every manager link refused at once, which is a bigger + * behaviour change than the hole being closed. + */ +async function screenCrossOrganization( + deps: SetUserManagerDeps, + engine: SetUserManagerEngine, + userId: string, + managerId: string, +): Promise<{ outside: true; userOrgs: string[]; managerOrgs: string[] } | { outside: false }> { + const userOrgs = await membershipOrgIds(deps, engine, userId); + if (!userOrgs || userOrgs.length === 0) return { outside: false }; + const managerOrgs = await membershipOrgIds(deps, engine, managerId); + if (!managerOrgs || managerOrgs.length === 0) return { outside: false }; + const shared = managerOrgs.some((o) => userOrgs.includes(o)); + if (shared) return { outside: false }; + return { outside: true, userOrgs, managerOrgs }; +} + +/** + * Walk the proposed manager's own chain upward. + * + * Returns the ordered ancestor ids, or a verdict when the walk cannot end in + * an acceptable link. The walk carries its own `seen` set, so a loop that + * already exists upstream terminates the walk instead of hanging it, and is + * reported as the data defect it is rather than being silently accepted. + */ +async function walkChain( + engine: SetUserManagerEngine, + managerId: string, + userId: string, +): Promise< + | { kind: 'ok'; links: number } + | { kind: 'cycle'; closesOn: string } + | { kind: 'existing_loop'; repeated: string } + | { kind: 'too_deep' } +> { + const seen = new Set([managerId]); + let cursor: string | null = managerId; + // One link is the edge being written (userId -> managerId); each ancestor + // above the proposed manager adds another. + let links = 1; + + while (cursor) { + const row: UserRow | null = await findUser(engine, cursor); + const next: string = row?.manager_id ? String(row.manager_id) : ''; + if (!next) return { kind: 'ok', links }; + if (next === userId) return { kind: 'cycle', closesOn: next }; + if (seen.has(next)) return { kind: 'existing_loop', repeated: next }; + seen.add(next); + links += 1; + if (links > MAX_MANAGER_CHAIN_DEPTH) return { kind: 'too_deep' }; + cursor = next; + } + return { kind: 'ok', links }; +} + +/** + * `POST /api/v1/auth/admin/set-user-manager` — the caller is ALREADY gated by + * the mount's ADR-0068 platform-admin check, and `actor` is that judged admin. + * + * Body: `{ userId, managerId }`. `managerId: null` clears the link; the key + * being ABSENT is refused rather than read as a clear, so a payload that + * misspells it cannot silently unset an org chart. + */ +export async function runSetUserManager( + deps: SetUserManagerDeps, + actor: AdminActor, + request: Request, +): Promise { + const body = await parseJson(request); + + const rawUserId = readId(body, 'userId', 'user_id'); + if (typeof rawUserId !== 'string' || rawUserId.length === 0) { + return refuse(400, 'INVALID_REQUEST', 'invalid_body', 'userId is required'); + } + const userId = rawUserId; + + const rawManagerId = readId(body, 'managerId', 'manager_id'); + if (rawManagerId === undefined) { + return refuse( + 400, + 'INVALID_REQUEST', + 'invalid_body', + 'managerId is required — send null to clear the link, never omit the key', + ); + } + if (rawManagerId !== null && (typeof rawManagerId !== 'string' || rawManagerId.length === 0)) { + return refuse( + 400, + 'INVALID_REQUEST', + 'invalid_body', + 'managerId must be a non-empty user id, or null to clear the link', + ); + } + const managerId: string | null = rawManagerId; + + const engine = deps.getDataEngine(); + if (!engine) { + return refuse( + 503, + 'SERVICE_UNAVAILABLE', + 'engine_unavailable', + 'No data engine is wired, so the manager link cannot be written', + ); + } + + const user = await findUser(engine, userId); + if (!user) { + return refuse(404, 'RESOURCE_NOT_FOUND', 'user_not_found', 'User not found'); + } + + // Refusal 5 — the directory owns this identity. Applied to the CLEAR as + // well as the set: both are writes the next sync would overwrite. + if (String(user.source ?? '') === 'idp_provisioned') { + return refuse( + 403, + 'PERMISSION_DENIED', + 'idp_provisioned', + "This identity is provisioned by an external directory (sys_user.source is 'idp_provisioned'), " + + 'so its manager is maintained by that directory and a value written here would be ' + + 'reverted on the next sync. Set the manager in the directory instead.', + ); + } + + if (managerId !== null) { + // Refusal 1 — self-assignment. + if (managerId === userId) { + return refuse( + 400, + 'INVALID_FIELD', + 'self_assignment', + 'A user cannot be their own manager: an approval routed to the manager rung would resolve ' + + 'to the submitter, which is the approval this tier exists to prevent.', + ); + } + + const manager = await findUser(engine, managerId); + if (!manager) { + return refuse( + 400, + 'INVALID_REFERENCE', + 'manager_not_found', + 'The proposed manager does not exist', + ); + } + + // Refusal 4 — cross-organization. + const screen = await screenCrossOrganization(deps, engine, userId, managerId); + if (screen.outside) { + return refuse( + 400, + 'INVALID_REFERENCE', + 'cross_organization', + `The proposed manager holds membership in ${screen.managerOrgs.length} organization(s), ` + + `none of them an organization this user belongs to. Routing approvals to him would put ` + + `approval authority over the record outside its tenant. Grant him a membership in one ` + + `of this user's organizations, or pick a manager inside them.`, + ); + } + + // Refusals 2 and 3 — cycle and depth, in one walk. + const walk = await walkChain(engine, managerId, userId); + if (walk.kind === 'cycle') { + return refuse( + 409, + 'RESOURCE_CONFLICT', + 'cycle', + 'That link would close a loop in the reporting chain: this user already appears above the ' + + 'proposed manager, so the chain would never reach a top. Clear the intermediate link ' + + 'first, then set this one.', + ); + } + if (walk.kind === 'existing_loop') { + return refuse( + 409, + 'RESOURCE_CONFLICT', + 'cycle', + `The proposed manager's own reporting chain already contains a loop (it revisits user ` + + `'${walk.repeated}'), so attaching this user beneath it would produce a chain with no ` + + `top. Repair that loop first.`, + ); + } + if (walk.kind === 'too_deep') { + return refuse( + 400, + 'VALUE_OUT_OF_RANGE', + 'max_depth_exceeded', + `That link would make this user's reporting chain longer than the maximum of ` + + `${MAX_MANAGER_CHAIN_DEPTH} links. The hierarchy scopes that walk this chain are ` + + `bounded, so a longer chain is not resolved in full. Shorten the chain above the ` + + `proposed manager first.`, + ); + } + } + + // The write. `authSystemWriteContext()` builds both halves together: + // `isSystem: true` is the AUTHORIZATION half that carries this past the + // ADR-0092 identity write guard without any whitelist entry, and + // `attributedUserId` is the ATTRIBUTION half, so the row records the admin + // who made the change rather than the system. + const context = await authSystemWriteContext(); + await engine.update('sys_user', { id: userId, manager_id: managerId }, { context }); + + return { + status: 200, + body: { success: true, data: { userId, managerId, setBy: actor.id } }, + }; +} diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index b3f42b95c3..97a10fcc16 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -2322,6 +2322,51 @@ export class AuthPlugin implements Plugin { } }); + // ──────────────────────────────────────────────────────────────────── + // #16678 — admin: set (or clear) a user's manager. + // + // `sys_user.manager_id` drives the approvals `{ type: 'manager' }` rung + // and the `own_and_reports` read scope, and had no product write surface + // at all: the generic data path refuses it (ADR-0092 D2's managed-update + // whitelist is `{name, image, locale}`), the bulk import does not carry + // it, and the Console renders it read-only. So the rung expanded to + // nobody on every record in any install without a directory sync. + // + // Same family as `unlock-user` above: an ObjectStack mount on the raw app + // ahead of the catch-all, platform-admin gated (ADR-0068), ledgered in + // `auth-route-ledger.ts`. The handler runs under a SYSTEM context, so it + // reaches the column by context rather than by whitelist — exactly how + // `admin-import-users` already reaches `phone_number` and `role` — which + // is why no Tier-1 list moves and the column keeps `readonly: true`. + // Every refusal (self-assignment, cycle, depth, cross-organization, + // directory-owned identity) is enforced in the handler; see + // `admin-set-user-manager.ts` for why each one has to live at the write. + rawApp.post(`${basePath}/admin/set-user-manager`, async (c: any) => { + try { + const actor = await gateAdmin(c); + if (actor instanceof Response) return actor; + const { runSetUserManager } = await import('./admin-set-user-manager.js'); + // Attribution only — the route's own authorization already happened + // in `gateAdmin`. Opening the actor seam here credits the `sys_user` + // row to the admin instead of recording it as the system. + const { status, body } = await runAttributedToUser(actor.id, () => + runSetUserManager( + { + getDataEngine: () => this.authManager!.getDataEngine() as any, + logger: ctx.logger, + }, + actor, + c.req.raw, + ), + ); + return c.json(body, status as any); + } catch (error) { + const err = error instanceof Error ? error : new Error(String(error)); + ctx.logger.error('[AuthPlugin] set-user-manager failed', err); + return c.json({ success: false, error: { code: 'INTERNAL_ERROR', message: err.message } }, 500); + } + }); + // ──────────────────────────────────────────────────────────────────── // #2766 V1 — admin direct user management. `sys_user` CRUD is suppressed // (managedBy better-auth), and until now the only add-a-teammate path was diff --git a/packages/plugins/plugin-auth/src/auth-route-ledger.conformance.test.ts b/packages/plugins/plugin-auth/src/auth-route-ledger.conformance.test.ts index 042021e2df..e69a16ff88 100644 --- a/packages/plugins/plugin-auth/src/auth-route-ledger.conformance.test.ts +++ b/packages/plugins/plugin-auth/src/auth-route-ledger.conformance.test.ts @@ -217,6 +217,13 @@ describe('auth route ledger hygiene', () => { 'GET /api/v1/auth/config', 'POST /api/v1/auth/admin/import-users', 'POST /api/v1/auth/admin/oauth2/toggle-disabled', + // [#16678] The thirteenth, added on this pin's own terms exactly as the + // set-initial-password row below was: `auth-plugin.ts` mounts it itself + // (a `rawApp.post` on the `${basePath}/admin/set-user-manager` + // template, ahead of the catch-all), and the `live.has(route)` loop + // below holds it to the same proof as the other twelve — better-auth + // publishes no manager endpoint at all. + 'POST /api/v1/auth/admin/set-user-manager', 'POST /api/v1/auth/admin/sso/register', 'POST /api/v1/auth/admin/sso/register-saml', 'POST /api/v1/auth/admin/sso/request-domain-verification', diff --git a/packages/plugins/plugin-auth/src/auth-route-ledger.ts b/packages/plugins/plugin-auth/src/auth-route-ledger.ts index f88eb107ca..68895448f3 100644 --- a/packages/plugins/plugin-auth/src/auth-route-ledger.ts +++ b/packages/plugins/plugin-auth/src/auth-route-ledger.ts @@ -237,6 +237,7 @@ export const AUTH_ROUTE_LEDGER: readonly AuthRouteLedgerEntry[] = [ { route: 'POST /api/v1/auth/admin/sso/register-saml', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', requires: 'sso', note: 'no SDK method builds this URL — the sys_sso_provider register_saml_provider action posts flat fields the bridge reshapes into better-auth\'s nested samlConfig; platform-admin gated (ADR-0068 D4, #9653), ADR-0069 P3' }, { route: 'POST /api/v1/auth/admin/sso/request-domain-verification', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', requires: 'sso', note: 'no SDK method builds this URL — the sys_sso_provider action posts it and renders the returned DNS TXT record; ObjectStack bridge over @better-auth/sso, additionally gated on the opt-in ssoDomainVerification switch (OS_SSO_DOMAIN_VERIFICATION) — off means the inner endpoint 404s, the mount itself is unconditional; platform-admin gated (ADR-0068 D4), ADR-0024 ②' }, { route: 'POST /api/v1/auth/admin/sso/verify-domain', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', requires: 'sso', note: 'no SDK method builds this URL — the sys_sso_provider action posts it after the DNS TXT record is published; same opt-in ssoDomainVerification switch and platform-admin gate as request-domain-verification (ADR-0068 D4), ADR-0024 ②' }, + { route: 'POST /api/v1/auth/admin/set-user-manager', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', note: 'no SDK method builds this URL — the sys_user set_user_manager action posts it directly; sets or clears sys_user.manager_id, which drives the approvals `manager` rung and the own_and_reports scope and had no product write surface at all (the ADR-0092 D2 managed-update whitelist is {name,image,locale}). Runs under a system context so it reaches the column BY CONTEXT, not by whitelist — no Tier-1 list moves and the column keeps readonly: true; platform-admin gated (ADR-0068), ADR-0057 / ADR-0092, #16678' }, { route: 'POST /api/v1/auth/admin/unlock-user', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', note: 'no SDK method builds this URL — the sys_user unlock_user action posts it directly; clears a brute-force lockout (sys_user.locked_until / failed_login_count), a custom per-identity mechanism with no better-auth endpoint; platform-admin gated (ADR-0068), ADR-0069 D2' }, { route: 'POST /api/v1/auth/sys-oauth-application/register', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', note: 'no SDK method builds this URL — the sys_oauth_application create action posts it directly; session-required self-service wrapper over better-auth /oauth2/create-client that splits the Console\'s newline-separated redirect-URL textarea into the redirect_uris array the vendor schema requires' }, { route: 'POST /api/v1/auth/organization/accept-invitation', family: 'organization', source: 'better-auth', disposition: 'sdk', client: 'organizations.invitations.accept', requires: 'organization' }, From f62bc2d0ad8d7380824ee8d9ee6f9d973f07fad0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 08:25:09 +0000 Subject: [PATCH 2/4] feat(plugin-auth,lint): an admin can set sys_user.manager_id, and the remedy names the route Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .changeset/16678-admin-set-user-manager.md | 59 ++++++++++ content/docs/automation/approvals.mdx | 50 +++++--- .../src/validate-approval-approvers.test.ts | 49 +++++--- .../lint/src/validate-approval-approvers.ts | 110 +++++++++++------- .../src/admin-set-user-manager.test.ts | 12 +- 5 files changed, 202 insertions(+), 78 deletions(-) create mode 100644 .changeset/16678-admin-set-user-manager.md diff --git a/.changeset/16678-admin-set-user-manager.md b/.changeset/16678-admin-set-user-manager.md new file mode 100644 index 0000000000..6f903e4dcd --- /dev/null +++ b/.changeset/16678-admin-set-user-manager.md @@ -0,0 +1,59 @@ +--- +'@objectstack/plugin-auth': patch +'@objectstack/lint': patch +--- + +`sys_user.manager_id` gains an admin write surface: `POST /api/v1/auth/admin/set-user-manager` + +`{ type: 'manager' }` is the canonical first rung of a tiered approval ladder, +and it resolves `sys_user.manager_id` — a column **no product surface could +write**. Measured: the generic data path refuses it (the ADR-0092 D2 +managed-update whitelist for `sys_user` is `{name, image, locale}`), the admin +bulk import does not carry it (`admin-import-users.ts` matches `manager_id` 0 +times, against a control of `phone_number` 8), and the column is `readonly` on +the user form. So on any install without a directory sync the rung expanded to +nobody, the request opened on a slate no one could act on, and under the +default `lockRecord: true` the record stayed locked. + +**The endpoint.** A platform admin posts `{ userId, managerId }`; `managerId: +null` clears the link. It is an ObjectStack mount on the raw app ahead of the +better-auth catch-all — the same family as `POST /api/v1/auth/admin/unlock-user` +— platform-admin gated (ADR-0068) and ledgered in `auth-route-ledger.ts`. + +**It is not a new editable profile column, and that is the design.** The +handler runs under a **system context**, so it reaches the column by context +rather than by a whitelist entry — the same way `admin-import-users` already +reaches `phone_number` and `role`. `SYS_USER_PROFILE_EDIT_FIELDS` is +untouched, `MANAGED_EXTENSION_EDITABLE_FIELDS.sys_user` stays `{locale}`, and +`sys_user.manager_id` keeps `readonly: true`, so ADR-0092 D4 still holds by +construction. Since ADR-0092 D5's amendment made Tier-1 membership imply +self-editability, admitting the column to Tier 1 would have handed every member +their own first-rung approver and a widening of their own `own_and_reports` +read scope; it is not admitted. + +**Five refusals, every one enforced at the write** — the only manager-chain +walkers in the open tree are single-hop, so nothing downstream catches a bad +link: self-assignment; a link that closes a cycle (the walk is itself +cycle-safe, so a pre-existing loop is reported rather than hung on); a chain +past the depth cap that ADR-0057 D3's bounded rollups require; a manager +provably outside every organization the user belongs to (beside, not instead +of, the existing routing-time screen); and any identity whose `sys_user.source` +is `idp_provisioned`, where the directory stays the one authoring surface. + +**`@objectstack/lint`** keeps the `approval-approvers-may-resolve-empty` +advisory and its `stackWiresManagerChain` silencer — the dead end it reports +survives the write surface, because a static check still cannot read the +column; only its *cause* became recoverable. What changed is the remedy text, +which named a column with no route and now names the endpoint, its body, how to +clear the link, and what it refuses. The Approvals guide carries the same +rewrite in prose. + +**Why `patch` and not `minor`.** No new exported symbol is reachable from +either published entry: `admin-set-user-manager.ts` is deliberately not +re-exported from `plugin-auth/src/index.ts` and is not named in the package's +`exports` map, so none of `runSetUserManager`, `MAX_MANAGER_CHAIN_DEPTH`, +`SetUserManagerDeps`, `SetUserManagerEngine`, `SetUserManagerResult` or +`SetUserManagerRefusalReason` appears in the built `dist/index.d.ts`. No +already-published payload gains a key — the endpoint's response is a new +payload, not a new field on an old one. A new **route** is wire, and wire +compatibility is not the grading floor. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 311f13fe1c..2f309ae0cf 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -64,26 +64,40 @@ Authored `type: 'role'` on 15.x? That is the deprecated spelling of `org_members -**`manager` is a directory-sync dependency, not something you configure here.** -`{ type: 'manager' }` resolves at runtime from `sys_user.manager_id` on the record's owner, and -that column is not one an operator can fill in from inside the product. - -`sys_user.manager_id` has no product write surface — the data API's managed-update whitelist is -`{name, image, locale}`, the auth admin endpoints do not accept the column and the Console -renders no field for it, so it is never populated by editing the user in the Console. On this -platform the column is written by a seed, or by any other system-context write, which bypasses -the managed-update whitelist. SCIM provisioning and directory sync can populate it too, but only +**`manager` is set on the user by an admin, not configured here.** +`{ type: 'manager' }` resolves at runtime from `sys_user.manager_id` on the record's owner. That +column is not a profile field: it is `readonly` on the user form and the data API's +managed-update whitelist is `{name, image, locale}`, so it is never populated by editing the user +in the Console. + +It has a dedicated admin operation instead. A platform admin posts the link: + +```http +POST /api/v1/auth/admin/set-user-manager +Content-Type: application/json + +{ "userId": "usr_report", "managerId": "usr_boss" } +``` + +`"managerId": null` clears the link. The endpoint refuses a link that would make a user their own +manager, close a cycle in the reporting chain, run past the chain depth cap, or point at a manager +who is provably outside every organization the user belongs to — each with the reason in the +response. It is platform-admin gated, like every other `/api/v1/auth/admin/*` route. + +The column is also written by a seed, or by any other system-context write, which bypasses the +managed-update whitelist. SCIM provisioning and directory sync can populate it too, but only through a provisioning path your own deployment supplies: this platform declares the SCIM `manager` attribute without projecting it onto the column, and its admin bulk import does not -write it either. - -So where `manager_id` is unset the slate resolves to nobody: the request opens with an empty -`pending_approvers` and waits forever, and under the default `lockRecord: true` the record stays -locked with no in-product recovery. Populate the column for everyone who submits this request, -or add a fallback approver that cannot resolve empty — e.g. -`{ type: 'org_membership_level', value: 'owner' }`. `os lint` reports this at authoring time -(`approval-approvers-may-resolve-empty`, `info`) and carries the same remedy text, so the two -cannot drift apart. +write it either. Where an identity carries `source: 'idp_provisioned'` the admin operation +**refuses**, so that directory stays the one surface that authors its manager. + +Setting the column is still something you have to *do*. Where `manager_id` is unset the rung +expands to nobody, the request opens on a slate no one can act on, and under the default +`lockRecord: true` the record stays locked — recoverable only by a platform or tenant admin +override. So set it for everyone who submits this request, or add a fallback approver that cannot +resolve empty — e.g. `{ type: 'org_membership_level', value: 'owner' }`. `os lint` reports the +shape at authoring time (`approval-approvers-may-resolve-empty`, `info`) and carries the same +remedy text, so the two cannot drift apart. Approving is itself a gated action — model "may approve" as a capability (`approve_invoice`) the approver's permission set grants, and gate the approve action's `requiredPermissions` on it so the gate is enforced on **both** the UI and the server (ADR-0066 D4). diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index 9fe0c774e4..3a6f8a5d13 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -259,30 +259,50 @@ describe('unset-manager dead-end (#16748)', () => { expect(findings[0].message).toContain('locked'); // lockRecord defaults true }); - it('names the REAL remedy — provisioning, not the Console', () => { + it('names the REAL remedy — the admin operation, not the Console form', () => { const [finding] = validateApprovalApprovers(managerOnly()); - // The prescription an operator can actually carry out (#16678: the column - // has no product write surface). + // The prescription an operator can actually carry out. #16678 landed the + // write surface, so the first thing the hint owes the reader is the + // endpoint, its body and how to CLEAR the link. + expect(finding.hint).toContain('/api/v1/auth/admin/set-user-manager'); + expect(finding.hint).toContain('{ userId, managerId }'); + expect(finding.hint).toContain('managerId set to null'); expect(finding.hint).toContain('SCIM'); expect(finding.hint).toContain('import'); expect(finding.hint).toContain('directory sync'); - expect(finding.hint).toContain('no product write surface'); - // ⛔ And it must not send them to a surface that cannot write it. The word - // "Console" appears only inside that denial, never as an instruction. + + // ⛔ The assertion that went stale the day the endpoint landed. It must be + // gone from the string, not merely contradicted further down it: an author + // who reads "no product write surface" stops looking for the endpoint. + expect(finding.hint).not.toContain('no product write surface'); + + // ⛔ And it still must not send them to a surface that cannot write it — + // the column stays OUT of ADR-0092 Tier 1 and readonly on the form, so a + // dedicated operation is not the same thing as an editable profile column. + // The word "Console" appears only inside that denial, never as an + // instruction. expect(finding.hint).toContain('never populated by editing the user in the Console'); expect(finding.hint).not.toMatch(/[Ee]dit .{0,40}in the Console\b(?!.*NOT)/); - // It still offers the escape that does not depend on #16678 at all. + // It still offers the escape that needs no operator action at all. expect(finding.hint).toContain("org_membership_level', value: 'owner'"); }); it('GRADES the routes — an exact diagnosis whose remedy cannot be carried out is worse than none', () => { - // A remedy that names a route with no writer is the #17037 shape. The three - // routes are measured against this tree, so the hint must SEPARATE the one - // that works here from the ones that need the deployment's own provisioning. + // A remedy that names a route with no writer is the #17037 shape. The four + // routes are measured against this tree, so the hint must SEPARATE the ones + // that work here from the ones that need the deployment's own provisioning. const [finding] = validateApprovalApprovers(managerOnly()); - // The route with a demonstrated writer: a system-context write bypasses the - // managed-update whitelist (`isUserContextWrite` is `userId && !isSystem`). + // The route added by #16678, and the refusals that make it safe to point an + // operator at: naming an endpoint without naming what it declines is the + // same "prescription that cannot be carried out" defect one layer in. + expect(finding.hint).toContain('their own manager'); + expect(finding.hint).toContain('close a cycle'); + expect(finding.hint).toContain('organization boundary'); + + // The pre-existing route with a demonstrated writer: a system-context write + // bypasses the managed-update whitelist (`isUserContextWrite` is + // `userId && !isSystem`). Kept — the endpoint did not replace it. expect(finding.hint).toContain('written by a seed, or by any other system-context write'); expect(finding.hint).toContain('bypasses the managed-update whitelist'); @@ -294,8 +314,11 @@ describe('unset-manager dead-end (#16748)', () => { // ⛔ And they must not be deleted: a deployment running a real directory // sync may well populate the column, and the defect was presenting all - // three as equally available, never naming them at all. + // three as equally available, never naming them at all. They now also take + // PRECEDENCE — the endpoint refuses an `idp_provisioned` identity — so the + // hint must not read as "use the endpoint instead of your directory". expect(finding.hint).toContain('SCIM provisioning and directory sync can populate it'); + expect(finding.hint).toContain("source 'idp_provisioned' the admin operation refuses"); }); it('does not claim a runtime fact it did not read', () => { diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index 0430971303..0030359735 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -100,74 +100,98 @@ const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']); /** * How an operator actually populates `sys_user.manager_id` (#16748 / #16678). * - * ⚠️ NOT "edit the user in the Console". The column has NO product write - * surface today: `getManagedUpdateWhitelist('sys_user')` is exactly - * `{name, image, locale}` (ADR-0092 platform-object write narrowing), the auth - * admin endpoints do not accept it, and the Console renders no field for it. + * ⚠️ STILL NOT "edit the user in the Console". The column is `readonly: true` + * and stays out of ADR-0092's Tier 1, so `getManagedUpdateWhitelist('sys_user')` + * is still exactly `{name, image, locale}` and the standard edit form still + * renders no editable field for it. What changed in #16678 is that the column + * gained a DEDICATED admin operation of its own, which is a different thing + * from becoming an editable profile column — and the distinction is the design, + * not a nicety: ADR-0092 D5's amendment made Tier-1 membership imply + * SELF-editability, so a `manager_id` that reached the form would hand every + * member their own first-rung approver. * * ⛔ AND a remedy naming a route that does not exist is worse than no remedy — * an exact diagnosis whose prescription cannot be carried out (#17037). So the * routes are GRADED here rather than listed, and each grade is a measurement - * against this tree with a discriminating control beside it: + * against this tree with a discriminating control beside it. Re-taken on the + * change that landed the write surface: * - * - Seed, or any other system-context write — AVAILABLE HERE. Both write - * guards gate on `isUserContextWrite`, spelled identically in each: - * `Boolean(userId) && isSystem !== true` (plugin-security + * - The admin endpoint — AVAILABLE HERE, and now the route to name first. + * `POST /api/v1/auth/admin/set-user-manager`, body `{ userId, managerId }` + * with `managerId: null` as the clear, platform-admin gated (ADR-0068) and + * ledgered as an ObjectStack mount in plugin-auth's `auth-route-ledger.ts`. + * It reaches the column under a SYSTEM context — by context, ⛔ not by a + * whitelist entry — which is why the whitelist reading above is unchanged. + * It refuses self-assignment, a link that closes a cycle, a chain past its + * depth cap, a manager provably outside the user's organizations, and any + * identity whose `sys_user.source` is `idp_provisioned`. + * - Seed, or any other system-context write — AVAILABLE HERE, unchanged. + * Both write guards gate on `isUserContextWrite`, spelled identically in + * each: `Boolean(userId) && isSystem !== true` (plugin-security * `system-write-guard.ts`, plugin-auth `identity-write-guard.ts`). A * system-context write therefore bypasses the managed-update whitelist by - * construction. This is the one route with a demonstrated writer in-repo. - * - SCIM — NOT here. The Enterprise `manager` attribute IS declared - * (`scim.zod.ts`), but no non-test file under `packages/plugins` or - * `packages/runtime` projects it onto the column: measured 0, against a - * control (`SysScimGroup`) the same scan does find, so the scan - * discriminates. - * - Admin bulk import — NOT here. `admin-import-users.ts` matches - * `manager_id` 0 times against a control of `phone_number` 8, and - * `SYS_USER_IMPORT_UPDATE_FIELDS` is `{name, image, locale}` plus - * `phone_number` and `role`. `sys-user-writable-fields.ts` lists - * `manager_id` among the admin-surface-only columns, so the omission is - * deliberate and ⛔ not an oversight to route around. + * construction. + * - SCIM — NOT here, re-measured and unchanged. The Enterprise `manager` + * attribute IS declared (`scim.zod.ts`), but no non-test file under + * `packages/plugins` or `packages/runtime` projects it onto the column: + * measured 0, against a control (`SysScimGroup`) the same scan does find, + * so the scan discriminates. + * - Admin bulk import — NOT here, re-measured and unchanged. + * `admin-import-users.ts` matches `manager_id` 0 times against a control of + * `phone_number` 8, and `SYS_USER_IMPORT_UPDATE_FIELDS` is + * `{name, image, locale}` plus `phone_number` and `role`. Admitting the + * column to the import tier is ruled but is a separate change. * * ⇒ SCIM and directory sync stay NAMED, because a deployment running a real * one may well populate the column through it — but named as something the * operator's own provisioning supplies, ⛔ never as something this repo gives - * them. + * them. They also now have PRECEDENCE over the endpoint rather than merely + * sitting beside it: on an `idp_provisioned` identity the endpoint refuses, so + * the directory is the one authoring surface for those rows. * - * ⛔ DEPENDENCY — #16678 holds the open question of whether `manager_id` should - * GAIN a product write surface. If it ever does, these strings are the lines - * that go stale: it would then be wrong to tell an author their only route is - * a system-context write. Update them in the same change that opens the write - * surface — and re-take the three grades above, which are readings of this - * tree, not standing facts. + * ⛔ WHAT DID NOT CHANGE — and why this rule is not deleted. The dead end the + * finding reports SURVIVES the write surface: a static check still cannot read + * the column, so a slate that is entirely `manager` rungs can still resolve to + * nobody. Only its CAUSE became recoverable — "nobody can populate it" became + * "an operator can". So the finding stays, `stackWiresManagerChain` stays the + * silencer, and the remedy is what was rewritten (#16678). * - * Two other carriers assert the same fact and go stale with these strings, so - * the list is theirs too: the `manager` callout in - * `content/docs/automation/approvals.mdx`, and `ApproverType`'s `.describe()` - * in `packages/spec/src/automation/approval.zod.ts` (rendered verbatim into - * `content/docs/references/automation/approval.mdx`). Neither RESTATES the - * remedy — both point back here, which is why there is still exactly one copy - * to edit — but both assert that the column has no product write surface, and - * that is the sentence which stops being true. + * ⛔ DEPENDENCY — one carrier of the old assertion is still stale and is ⛔ NOT + * fixed here: `ApproverType`'s `.describe()` in + * `packages/spec/src/automation/approval.zod.ts` (rendered verbatim into the + * generated `content/docs/references/automation/approval.mdx`) still says the + * column "has no product write surface". That edit is `packages/spec`, which + * this change is fenced out of; it is reported to the PM for the `domain:spec` + * seat. The third carrier, the `manager` callout in + * `content/docs/automation/approvals.mdx`, IS updated in this same change. + * Neither RESTATES the remedy — both point back here, which is why there is + * still exactly one copy to edit. */ // ⛔ The tracker ids stay in the comments above and never in this string: // `check:doc-authoring` Rule 3 — a runtime string reaches authors, operators and // generated surfaces, none of whom can resolve `#NNNN`. The reader who can // resolve it is reading this source. const MANAGER_ONLY_REMEDY = - `sys_user.manager_id has no product write surface — the data API's managed-update whitelist is ` + - `{name, image, locale}, the auth admin endpoints do not accept the column and the Console ` + - `renders no field for it, so it is never populated by editing the user in the Console.`; + `sys_user.manager_id is not a profile column — the data API's managed-update whitelist is ` + + `{name, image, locale} and the column is readonly on the user form, so it is never populated by ` + + `editing the user in the Console. It has a dedicated admin operation instead: a platform admin ` + + `POSTs { userId, managerId } to /api/v1/auth/admin/set-user-manager, with managerId set to null ` + + `to clear the link.`; /** * The routes an operator can actually take, GRADED — the measurement behind * each grade is in {@link MANAGER_ONLY_REMEDY}'s docblock. */ const MANAGER_ONLY_ROUTES = - `On this platform the column is written by a seed, or by any other system-context write, which ` + - `bypasses the managed-update whitelist. SCIM provisioning and directory sync can populate it ` + - `too, but only through a provisioning path your own deployment supplies: this platform declares ` + - `the SCIM 'manager' attribute without projecting it onto the column, and its admin bulk import ` + - `does not write it either.`; + `That endpoint is the route this platform gives you, and it refuses a link that would make a ` + + `user their own manager, close a cycle, run past the chain depth cap, or point across an ` + + `organization boundary. The column is also written by a seed, or by any other system-context ` + + `write, which bypasses the managed-update whitelist. SCIM provisioning and directory sync can ` + + `populate it too, but only through a provisioning path your own deployment supplies: this ` + + `platform declares the SCIM 'manager' attribute without projecting it onto the column, and its ` + + `admin bulk import does not write it either — and where an identity carries ` + + `source 'idp_provisioned' the admin operation refuses, leaving that directory the one surface ` + + `that authors its manager.`; export type ApprovalApproverSeverity = 'error' | 'warning' | 'info'; diff --git a/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts b/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts index c17ac04291..ddf5a9ccbe 100644 --- a/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts +++ b/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts @@ -315,15 +315,19 @@ describe('set-user-manager — refusal 3: depth', () => { ] as Row[], }); + // `chain(n)` holds n-1 links among `c0..c{n-1}`, so attaching `leaf` under + // `c0` makes 1 + (n - 1) = n links. The cap is on the resulting chain, so + // n == MAX is the last admissible one and n == MAX + 1 is the first refused + // — the boundary is pinned from BOTH sides so an off-by-one cannot hide as + // "the refusal still fires". it(`admits a chain of exactly ${MAX_MANAGER_CHAIN_DEPTH} links`, async () => { - // Attaching `leaf` under `c0` makes 1 + (MAX - 1) = MAX links. - const engine = makeEngine(chain(MAX_MANAGER_CHAIN_DEPTH - 1)); + const engine = makeEngine(chain(MAX_MANAGER_CHAIN_DEPTH)); const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'leaf', managerId: 'c0' })); expect(res.status).toBe(200); }); - it('refuses the link that would cross the cap', async () => { - const engine = makeEngine(chain(MAX_MANAGER_CHAIN_DEPTH)); + it('refuses the first link that would cross the cap', async () => { + const engine = makeEngine(chain(MAX_MANAGER_CHAIN_DEPTH + 1)); const res = await runSetUserManager(deps(engine), ACTOR, post({ userId: 'leaf', managerId: 'c0' })); expect(res.status).toBe(400); From 5dae98aba3a25527facfe333f816d52ba04803e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 08:46:23 +0000 Subject: [PATCH 3/4] =?UTF-8?q?fix(plugin-auth,docs):=20gate=20findings=20?= =?UTF-8?q?=E2=80=94=20prose=20id,=20where-matcher=20refusal,=20tenant=20c?= =?UTF-8?q?ensus?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - auth-route-ledger note carried a tracker id (check:doc-authoring Rule 3) - the test engine double read a WHERE combinator as a field name - the write-call-site census moved 222 -> 223 with the new engine write Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .../docs/permissions/tenant-audit-census.mdx | 34 +++++++++---------- ...08-tenant-audit-write-call-sites.counts.md | 19 ++++++----- .../src/admin-set-user-manager.test.ts | 27 +++++++++++++-- .../plugin-auth/src/auth-route-ledger.ts | 5 ++- 4 files changed, 55 insertions(+), 30 deletions(-) diff --git a/content/docs/permissions/tenant-audit-census.mdx b/content/docs/permissions/tenant-audit-census.mdx index caa1d19199..544c7794c1 100644 --- a/content/docs/permissions/tenant-audit-census.mdx +++ b/content/docs/permissions/tenant-audit-census.mdx @@ -98,7 +98,7 @@ are reported as `undecidable` rather than assumed either way. The same holds twice over for the context. An options argument spelled as a literal can be read; one spelled `options`, `{ ...opts }`, or handed through a -forwarding shim cannot, and **67 of the 222 sites are spelled that way**. A +forwarding shim cannot, and **67 of the 223 sites are spelled that way**. A context resolved from an inline literal or a local `const` can be tested for `isSystem`; one arriving from a helper call cannot. @@ -147,10 +147,10 @@ reproduce them. Where it disagrees, it disagrees on the page: | carried figure | where it survives | this census | | :--- | :--- | ---: | -| 175 write call sites | quoted in the merged changeset | **222** | +| 175 write call sites | quoted in the merged changeset | **223** | | 24 carrying no tenant context | quoted in the merged changeset | **9** provable and tenancy-enabled; **32** more whose options argument is unreadable | -| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **148 of 222** decidable, **74** undecidable | -| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 104 decidably elevated, 0 decidably not, 101 undecidable | +| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **149 of 223** decidable, **74** undecidable | +| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 104 decidably elevated, 0 decidably not, 102 undecidable | | 141 and 132, two independent re-derivations | the card that filed this work | — | **The differences are not reconciled, and deliberately so.** The old census's @@ -167,11 +167,11 @@ would report a smaller number and would not say so. The fourth row is the one worth flagging to anyone citing it. **The 135 / 77% figure has no surviving corroboration anywhere in the tree.** This census reads -104 of 222 (47%) as decidably elevated, with 101 more whose elevation is a +104 of 223 (47%) as decidably elevated, with 102 more whose elevation is a run-time fact — so the claim is neither confirmed nor refuted, and the honest answer is that a static reading cannot settle it. -⇒ **Cite `9 / 222`, and say what it is**: the sites whose options argument was +⇒ **Cite `9 / 223`, and say what it is**: the sites whose options argument was READ and holds no tenant context, against a decidably tenancy-enabled object. That is the control's provable yield surface. ⛔ Do not cite it as "the sites without tenant context" — **32 further sites** have an options argument this @@ -183,28 +183,28 @@ cannot read, and they are neither in nor out. | what | count | | :--- | ---: | -| write call sites on the application surface | **222** | -| …whose object name is statically decidable | 148 | +| write call sites on the application surface | **223** | +| …whose object name is statically decidable | 149 | | …whose object name is chosen at run time | 74 | -| …against an object with tenancy ENABLED | 148 | +| …against an object with tenancy ENABLED | 149 | | …against an object that declares tenancy off | 0 | -| threading a tenant context | 138 | +| threading a tenant context | 139 | | PROVABLY carrying none (options read, no context key) | **17** | | …of those, against a decidably tenancy-enabled object | **9** | | options argument UNREADABLE — may or may not carry one | 67 | | …of those, against a decidably tenancy-enabled object | 32 | | threading a decidably ELEVATED (`isSystem`) context | 104 | | threading a context that is decidably NOT elevated | 0 | -| threading a context whose elevation is a run-time fact | 101 | +| threading a context whose elevation is a run-time fact | 102 | | how the instrument reached the site | count | | :--- | ---: | -| receiver carried a readable engine type | 178 | +| receiver carried a readable engine type | 179 | | receiver erased, placed by the object NAME | 18 | | receiver erased, placed by an `object: string` PARAMETER | 15 | | receiver erased, placed by an `UNTYPED_RECEIVERS` row | 11 | -| object name spelled inline | 108 | +| object name spelled inline | 109 | | object name spelled through a `const` | 40 | | object name is an `object: string` parameter | 19 | | object name is some other run-time expression | 55 | @@ -224,13 +224,13 @@ holds still. They are required to be HERE and to say WHEN they were true; their values are not compared. The reasoning, and the measurement behind it, are in `scripts/check-tenant-audit-census.mjs`. -Measured on 2026-09-12 at `bbeb190f6`. +Measured on 2026-09-13 at `f62bc2d0a`. | corpus scale (not enforced) | count | | :--- | ---: | -| tracked non-test sources scanned | 564 | -| engine-shaped types recognised | 59 | +| tracked non-test sources scanned | 567 | +| engine-shaped types recognised | 61 | | declared objects in the registry | 300 | -| same-named calls subtracted as non-engine | 137 | +| same-named calls subtracted as non-engine | 138 | {/* END GENERATED: tenant-audit-census */} diff --git a/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md b/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md index 111af3963c..af01467e5b 100644 --- a/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md +++ b/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md @@ -29,19 +29,19 @@ silent, and `node scripts/tenant-audit-census.mjs --write` is the resolution. | Measure | Value | |---|---:| -| Write call sites | 222 | -| Object name statically decidable | 148 | +| Write call sites | 223 | +| Object name statically decidable | 149 | | Object name chosen at run time | 74 | -| Against a tenancy-enabled object | 148 | +| Against a tenancy-enabled object | 149 | | Against an object declaring tenancy off | 0 | -| Threading a tenant context | 138 | +| Threading a tenant context | 139 | | Provably carrying none | 17 | | …and decidably tenancy-enabled | 9 | | Options argument unreadable | 67 | | …and decidably tenancy-enabled | 32 | | Threading a decidably elevated context | 104 | | Threading a decidably non-elevated context | 0 | -| Threading a context of undecidable elevation | 101 | +| Threading a context of undecidable elevation | 102 | ## Corpus scale — present and dated, ⛔ NOT enforced @@ -52,14 +52,14 @@ holds still. They are required to be HERE and to say WHEN they were true; their values are not compared. The reasoning, and the measurement behind it, are in `scripts/check-tenant-audit-census.mjs`. -Measured on 2026-09-12 at `bbeb190f6`. +Measured on 2026-09-13 at `f62bc2d0a`. | corpus scale (not enforced) | count | | :--- | ---: | -| tracked non-test sources scanned | 564 | -| engine-shaped types recognised | 59 | +| tracked non-test sources scanned | 567 | +| engine-shaped types recognised | 61 | | declared objects in the registry | 300 | -| same-named calls subtracted as non-engine | 137 | +| same-named calls subtracted as non-engine | 138 | ## Every site @@ -78,6 +78,7 @@ Measured on 2026-09-12 at `bbeb190f6`. | `packages/plugins/plugin-audit/src/read-audit.ts` | `insert` | `sys_audit_log` | enabled | options unreadable | 1 | | `packages/plugins/plugin-auth/src/admin-import-users.ts` | `insert` | `sys_audit_log` | enabled | options unreadable | 1 | | `packages/plugins/plugin-auth/src/admin-import-users.ts` | `update` | `sys_user` | enabled | options unreadable | 2 | +| `packages/plugins/plugin-auth/src/admin-set-user-manager.ts` | `update` | `sys_user` | enabled | context, elevation undecidable | 1 | | `packages/plugins/plugin-auth/src/admin-user-endpoints.ts` | `insert` | `sys_audit_log` | enabled | elevated | 1 | | `packages/plugins/plugin-auth/src/admin-user-endpoints.ts` | `update` | `sys_user` | enabled | elevated | 1 | | `packages/plugins/plugin-auth/src/adopt-membership.ts` | `update` | `SystemObjectName.MEMBER` | undecidable | PROVABLY NONE | 1 | diff --git a/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts b/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts index ddf5a9ccbe..b2ee5567dd 100644 --- a/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts +++ b/packages/plugins/plugin-auth/src/admin-set-user-manager.test.ts @@ -43,6 +43,29 @@ interface Recorded { options: Record | undefined; } +/** + * Equality on a field name — every predicate this module issues — and a LOUD + * refusal of everything else. A double that reads a combinator as a field name + * answers a question nobody asked, silently, and keeps the suite green while + * the real engine returns something else entirely. + * + * Lifted to module scope rather than closed over the fixtures on purpose: a + * matcher that closes over its own rows is unjudgeable by + * `check:where-matcher`, which is a worse answer than a wrong one. + */ +function matchesWhere(row: Row, where: Record): boolean { + for (const [key, value] of Object.entries(where)) { + if (key.startsWith('$') || key === 'and' || key === 'or' || key === 'not') { + throw new Error(`engineDouble: unsupported WHERE combinator '${key}' — implement it or stop issuing it`); + } + if (value !== null && typeof value === 'object') { + throw new Error(`engineDouble: unsupported operator object on '${key}' — implement it or stop issuing it`); + } + if (String(row[key] ?? '') !== String(value ?? '')) return false; + } + return true; +} + function makeEngine(tables: Record) { const updates: Recorded[] = []; const failReads = new Set(); @@ -59,9 +82,7 @@ function makeEngine(tables: Record) { const q = (query ?? {}) as { where?: Record; limit?: number }; const rows = tables[object] ?? []; const where = q.where ?? {}; - const out = rows.filter((r) => - Object.entries(where).every(([k, v]) => String(r[k] ?? '') === String(v ?? '')), - ); + const out = rows.filter((r) => matchesWhere(r, where)); return typeof q.limit === 'number' ? out.slice(0, q.limit) : out; }, async update(object: string, data: unknown, options?: unknown): Promise { diff --git a/packages/plugins/plugin-auth/src/auth-route-ledger.ts b/packages/plugins/plugin-auth/src/auth-route-ledger.ts index 68895448f3..52f7b7b9fe 100644 --- a/packages/plugins/plugin-auth/src/auth-route-ledger.ts +++ b/packages/plugins/plugin-auth/src/auth-route-ledger.ts @@ -237,7 +237,10 @@ export const AUTH_ROUTE_LEDGER: readonly AuthRouteLedgerEntry[] = [ { route: 'POST /api/v1/auth/admin/sso/register-saml', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', requires: 'sso', note: 'no SDK method builds this URL — the sys_sso_provider register_saml_provider action posts flat fields the bridge reshapes into better-auth\'s nested samlConfig; platform-admin gated (ADR-0068 D4, #9653), ADR-0069 P3' }, { route: 'POST /api/v1/auth/admin/sso/request-domain-verification', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', requires: 'sso', note: 'no SDK method builds this URL — the sys_sso_provider action posts it and renders the returned DNS TXT record; ObjectStack bridge over @better-auth/sso, additionally gated on the opt-in ssoDomainVerification switch (OS_SSO_DOMAIN_VERIFICATION) — off means the inner endpoint 404s, the mount itself is unconditional; platform-admin gated (ADR-0068 D4), ADR-0024 ②' }, { route: 'POST /api/v1/auth/admin/sso/verify-domain', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', requires: 'sso', note: 'no SDK method builds this URL — the sys_sso_provider action posts it after the DNS TXT record is published; same opt-in ssoDomainVerification switch and platform-admin gate as request-domain-verification (ADR-0068 D4), ADR-0024 ②' }, - { route: 'POST /api/v1/auth/admin/set-user-manager', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', note: 'no SDK method builds this URL — the sys_user set_user_manager action posts it directly; sets or clears sys_user.manager_id, which drives the approvals `manager` rung and the own_and_reports scope and had no product write surface at all (the ADR-0092 D2 managed-update whitelist is {name,image,locale}). Runs under a system context so it reaches the column BY CONTEXT, not by whitelist — no Tier-1 list moves and the column keeps readonly: true; platform-admin gated (ADR-0068), ADR-0057 / ADR-0092, #16678' }, + // #16678 — the tracker id stays HERE and never in the `note` string: + // `check:doc-authoring` Rule 3, and the ledgered cross-package leg it added, + // both read notes as prose that reaches authors and generated surfaces. + { route: 'POST /api/v1/auth/admin/set-user-manager', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', note: 'no SDK method builds this URL — the sys_user set_user_manager action posts it directly; sets or clears sys_user.manager_id, which drives the approvals `manager` rung and the own_and_reports scope and had no product write surface at all (the ADR-0092 D2 managed-update whitelist is {name,image,locale}). Runs under a system context so it reaches the column BY CONTEXT, not by whitelist — no Tier-1 list moves and the column keeps readonly: true; platform-admin gated (ADR-0068), ADR-0057 / ADR-0092' }, { route: 'POST /api/v1/auth/admin/unlock-user', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', note: 'no SDK method builds this URL — the sys_user unlock_user action posts it directly; clears a brute-force lockout (sys_user.locked_until / failed_login_count), a custom per-identity mechanism with no better-auth endpoint; platform-admin gated (ADR-0068), ADR-0069 D2' }, { route: 'POST /api/v1/auth/sys-oauth-application/register', family: 'objectstack-mount', source: 'objectstack', disposition: 'server-only', note: 'no SDK method builds this URL — the sys_oauth_application create action posts it directly; session-required self-service wrapper over better-auth /oauth2/create-client that splits the Console\'s newline-separated redirect-URL textarea into the redirect_uris array the vendor schema requires' }, { route: 'POST /api/v1/auth/organization/accept-invitation', family: 'organization', source: 'better-auth', disposition: 'sdk', client: 'organizations.invitations.accept', requires: 'organization' }, From 02a036ad344290dad8565776ec5016637e722635 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 10:25:04 +0000 Subject: [PATCH 4/4] test(dogfood): classify the new set-user-manager admin route in both sweeps Both /admin/ sweeps derive their population from the running stack, so the new mount is in scope the moment it exists and must be classified with the answer a non-admin actually gets. Measured against the live stack rather than assumed: this mount runs gateAdmin() as its first statement, ahead of every body read, so anon gets 401 UNAUTHENTICATED and a member 403 PERMISSION_DENIED on ANY body -- including an empty one, which is the opposite of unlock-user's ordering. The platform admin gets 200 from the idempotent clear. - admin-route-nonadmin-refusal: objectstack-gate, body { userId, managerId: null } - admin-platform-admin-standing: ADMITTED + the same sweep payload Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- ...in-platform-admin-standing.dogfood.test.ts | 17 ++++++++++ ...min-route-nonadmin-refusal.dogfood.test.ts | 31 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/packages/qa/dogfood/test/admin-platform-admin-standing.dogfood.test.ts b/packages/qa/dogfood/test/admin-platform-admin-standing.dogfood.test.ts index 81ae091f39..865518eb06 100644 --- a/packages/qa/dogfood/test/admin-platform-admin-standing.dogfood.test.ts +++ b/packages/qa/dogfood/test/admin-platform-admin-standing.dogfood.test.ts @@ -223,6 +223,13 @@ const ADMITTED = [ 'POST /api/v1/auth/admin/has-permission', 'POST /api/v1/auth/admin/create-user', 'POST /api/v1/auth/admin/set-user-password', + // #16678 — the admin write surface for `sys_user.manager_id`, the column the + // approvals `manager` rung and the `own_and_reports` scope both read and + // that no product surface could write. An ObjectStack raw mount carrying the + // ADR-0068 gate, and the gate is its FIRST statement — ahead of every body + // read, unlike `unlock-user`'s. Measured on this stack: the platform admin + // gets `200 {"success":true,...,"managerId":null}` from the sweep's payload. + 'POST /api/v1/auth/admin/set-user-manager', 'POST /api/v1/auth/admin/unlock-user', 'POST /api/v1/auth/admin/import-users', 'POST /api/v1/auth/admin/oauth2/toggle-disabled', @@ -661,6 +668,16 @@ describe('#9482: what an ObjectStack platform admin gets from every /admin/ rout body: { format: 'json', rows: [{ email: 'standing.sweep.imported@example.com', name: 'Sweep Imported' }] }, }, 'POST /api/v1/auth/admin/unlock-user': { body: { userId: targetUserId } }, + // #16678. The CLEAR, not a set: it is idempotent, so the sweep leaves + // `manager_id` as it found it, and it still reaches a 2xx rather than a + // semantic error — measured 200 `{"success":true,...,"managerId":null}`. + // ⛔ The payload is not optional here even though the gate runs ahead of + // the body read: with no entry the handler answers `400 INVALID_REQUEST` + // "userId is required", which is neither a gate refusal nor a match for + // the `diedBeforeTheGate` predicate (that reads `VALIDATION_ERROR` or an + // "Invalid input"/"body." message, and this is neither) — so a missing + // payload would slip through every assertion below as a silent void. + 'POST /api/v1/auth/admin/set-user-manager': { body: { userId: targetUserId, managerId: null } }, 'POST /api/v1/auth/admin/oauth2/toggle-disabled': { body: { client_id: 'standing-probe-client', disabled: false } }, 'POST /api/v1/auth/admin/sso/register': { body: { diff --git a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts index 90d1bc1a05..40db98adb4 100644 --- a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts +++ b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts @@ -241,6 +241,37 @@ function expectationsFor(targetUserId: string): Record body: { client_id: 'refusal-probe-client', disabled: true }, note: 'admin passes the gate and lands on RESOURCE_NOT_FOUND for the unknown client', }, + // ── #16678 — the admin write surface for `sys_user.manager_id` ────────── + // + // MEASURED against the running stack rather than assumed, because this + // file's sharpest edge cuts the OTHER way here and that difference is what + // makes the entry trustworthy. `unlock-user` and `oauth2/toggle-disabled` + // read and shape-check `body` BEFORE calling `getSession`; this mount runs + // `gateAdmin(c)` as its first statement and only then imports and calls the + // handler, so authorization is answered ahead of every body read. Fired + // three ways at the live stack: + // + // EMPTY body anon 401 UNAUTHENTICATED + // member 403 PERMISSION_DENIED + // admin 400 INVALID_REQUEST "userId is required" + // { userId, null } anon 401 UNAUTHENTICATED + // member 403 PERMISSION_DENIED + // admin 200 {"success":true,...,"managerId":null} + // + // ⇒ the member's 403 is a gate verdict on ANY body here, and the admin is + // not turned away: the full `objectstack-gate` contrast. + // + // The payload CLEARS rather than sets, for two reasons. The clear is + // idempotent, so the sweep leaves `manager_id` exactly as it found it + // (measured null on the freshly signed-up target); and it still reaches a + // 2xx, so the allowed side is a real reading rather than a semantic error. + // ⛔ Not a self-assignment body: the admin would get 400 INVALID_FIELD — + // still not a gate refusal, but a weaker reading and a worse example. + 'POST /api/v1/auth/admin/set-user-manager': { + bucket: 'objectstack-gate', + body: { userId: targetUserId, managerId: null }, + note: 'the gate runs ahead of any body read, so the member 403 holds even on an empty body; the admin gets 200 from the idempotent clear', + }, // ── #9653: the /admin/sso/* bridges, gated ahead of their delegation ──── // // The ADR-0068 gate runs BEFORE the bridge re-dispatches into better-auth,