From be55067ba07eb01e617838079d030e62b80dd998 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 13:15:32 +0000 Subject: [PATCH 1/4] feat(plugin-auth): admit manager_id to the bulk import, resolved in a second pass The admin write surface's five refusals are extracted behind one seam, applyUserManagerLink, and the importer's second pass calls it row-wise instead of carrying a copy of the predicates. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj --- .../plugin-auth/src/admin-import-users.ts | 262 +++++++++++++++++- .../plugin-auth/src/admin-set-user-manager.ts | 97 +++++-- 2 files changed, 337 insertions(+), 22 deletions(-) diff --git a/packages/plugins/plugin-auth/src/admin-import-users.ts b/packages/plugins/plugin-auth/src/admin-import-users.ts index a371c6d7c7..a5e69888b0 100644 --- a/packages/plugins/plugin-auth/src/admin-import-users.ts +++ b/packages/plugins/plugin-auth/src/admin-import-users.ts @@ -63,6 +63,40 @@ * - Upsert updates only touch profile fields (UPDATE_ALLOWED_FIELDS); * credentials and the email identity are never modified on update, so a * re-imported CSV can never silently reset an existing user's password. + * + * ## `manager_id` — admitted, and resolved in a SECOND pass (#18028) + * + * Ruling `5651634638` row 7: the import tier admits `manager_id`, resolved in a + * second pass keyed on the importer's identity key, with every row-level + * refusal of the admin write surface applied and an unresolved key reported as + * a PER-ROW error. + * + * - **What the column holds is an identity key, not a user id.** A CSV author + * has the manager's email or phone, never their `usr_…` id, so the cell is + * read with the same key this importer already keys rows by — one key, ⛔ not + * a second one invented for this column. + * - **Why the pass is SECOND and not inline.** A manager named in row 40 may + * be created by row 90. Resolving inside `createData` would refuse valid + * input whenever the file is not topologically ordered, which no export + * guarantees. So the column is stripped from the row before the write + * (⛔ it never reaches `createData` / `updateData`, and ⛔ it is not added to + * UPDATE_ALLOWED_FIELDS) and the links are applied after `runImport` has + * returned, when every row in the batch exists. + * - **Where the refusals come from.** `applyUserManagerLink` — the same + * derivation `POST /admin/set-user-manager` runs, imported, ⛔ never copied. + * Self-assignment, cycle, depth cap, cross-organization and directory-owned + * identity are that function's to answer, and the refusal it returns is + * reported on the row rather than re-worded here. + * - **A manager problem never costs the row its identity.** The user is + * created either way and the failure rides `rows[].code` / `rows[].error`, + * exactly like the sibling post-write `INVITE_EMAIL_FAILED`; `rows[].manager` + * carries the machine-readable outcome in `rows[].delivery`'s shape. ⛔ Not a + * whole-import failure, and ⛔ not a silent skip. + * - **The pass does not run on `dryRun`.** It is a post-write pass like + * delivery: with nothing created there are no ids to link, and the five + * refusals cannot be evaluated against rows that do not exist. A dry run + * reports `manager: { linked: 0, unresolved: 0, refused: 0 }` and says + * nothing about the links — ⛔ rather than half-answering. */ import type { @@ -74,9 +108,24 @@ import { prepareImportRequest, runImport } from '@objectstack/rest'; import { generatePlaceholderEmail, isPlaceholderEmail } from './placeholder-email.js'; import { generateTemporaryPassword, normalizePhoneNumber, isLikelyEmail, type AdminActor, type EndpointResult } from './admin-user-endpoints.js'; import { SYS_USER_IMPORT_UPDATE_FIELDS } from './sys-user-writable-fields.js'; +import { + applyUserManagerLink, + type SetUserManagerDeps, + type SetUserManagerRefusalReason, +} from './admin-set-user-manager.js'; export const IMPORT_USERS_MAX_ROWS = 500; +/** + * The column a row names its manager in. Its value is the MANAGER'S IDENTITY + * KEY — an email or a phone number, the same two the importer keys rows by. + * + * One spelling, deliberately. The phone column's three historical aliases + * (`phone_number` / `phoneNumber` / `phone`) are debt this key does not + * inherit: Prime Directive #12 refuses a new dialect on an internal contract. + */ +const MANAGER_COLUMN = 'manager_id'; + /** * Profile fields an upsert row may modify on an EXISTING user — shared with * the identity write guard's Tier-1 whitelist via sys-user-writable-fields.ts @@ -249,6 +298,88 @@ function identityKey(email?: string, phone?: string): string { return ''; } +/** + * A row's manager outcome — the machine-readable half, carried in + * `rows[].delivery`'s shape rather than dug out of a message. + * + * `'linked'` and `'unresolved'` are this importer's two answers; every other + * member is a {@link SetUserManagerRefusalReason} produced by the shared + * derivation and passed through verbatim, so the importer and + * `POST /admin/set-user-manager` discriminate refusals identically. + */ +export type ImportManagerOutcome = 'linked' | 'unresolved' | SetUserManagerRefusalReason; + +/** + * One row of the endpoint's `data.rows[]` — the generic import row plus the + * three things this identity surface adds to it. Named because callers read + * `manager` and `delivery` to decide what to show an operator, and an inline + * type on one `const` is not something a Console can import. + */ +export interface IdentityImportRowResult extends ImportRowResult { + /** Returned ONCE, never persisted — `temporary` rows only. */ + temporaryPassword?: string; + /** How this created row's credential was delivered. */ + delivery?: 'email' | 'sms' | 'temporary'; + /** + * Present only on rows that named a manager. Absent means the row's + * {@link MANAGER_COLUMN} cell was empty — ⛔ never "the link silently failed". + */ + manager?: ImportManagerOutcome; +} + +/** + * Stamp a row's manager failure. + * + * `manager` is ALWAYS set — that is this outcome's own channel. `code`/`error` + * is the SHARED row error channel the sibling `INVITE_EMAIL_FAILED` also writes + * to, so it is claimed only when free: a row whose invitation already failed + * keeps that report and still carries its manager verdict on `manager`, + * ⛔ rather than one of the two failures overwriting the other into silence. + */ +function noteManagerFailure( + row: IdentityImportRowResult, + outcome: ImportManagerOutcome, + code: string, + message: string, +): void { + row.manager = outcome; + if (row.code === undefined) { + row.code = code; + row.error = message; + } +} + +/** + * A manager cell, read as an identity key. + * + * `'absent'` = the cell was empty, which is not a problem and is not reported. + * `'unreadable'` = it held something that is neither an email nor (where the + * phoneNumber plugin is wired) a phone number — reported per row, because a + * typo in a manager cell must never pass for "this user has no manager". + */ +interface ManagerKeyRef { + /** The `identityKey()` spelling — `e:` or `p:`. */ + key: string; + email?: string; + phone?: string; +} + +function resolveManagerKey( + raw: unknown, + phoneEnabled: boolean, +): ManagerKeyRef | 'absent' | 'unreadable' { + const value = typeof raw === 'string' ? raw.trim() : ''; + if (value.length === 0) return 'absent'; + if (isLikelyEmail(value)) { + const email = value.toLowerCase(); + return { key: identityKey(email, undefined), email }; + } + if (!phoneEnabled) return 'unreadable'; + const phone = normalizePhoneNumber(value); + if (!phone) return 'unreadable'; + return { key: identityKey(undefined, phone), phone }; +} + export async function runAdminImportUsers( deps: IdentityImportDeps, request: Request, @@ -317,15 +448,25 @@ export async function runAdminImportUsers( // ── Identity pre-validation (runs for dryRun too) ──────────────────── const phoneEnabled = deps.phoneNumberEnabled(); - const results: Array = new Array(prepared.rows.length); + const results: IdentityImportRowResult[] = new Array(prepared.rows.length); const validRows: Array> = []; const validIndex: number[] = []; // Per-row delivery plan, keyed by identity so createData (which sees a // coerced COPY of the row, not the original object) can look it up. `auto` // decides each row here; every other policy resolves the same plan for all. const planByKey = new Map(); + // [#18028] The manager cell each row named, and that row's own identity — + // both indexed by ORIGINAL row number, which is what the second pass walks. + // `undefined` in `managerWanted` means the row named nobody. + const managerWanted: Array = new Array(prepared.rows.length); + const rowIdentities: Array = new Array(prepared.rows.length); for (let i = 0; i < prepared.rows.length; i++) { const row = { ...prepared.rows[i] }; + // [#18028] Read the manager cell and STRIP it before anything else sees the + // row: it holds an identity key, so leaving it in place would hand a lookup + // column an email address. The second pass owns it from here. + const wanted = resolveManagerKey(row[MANAGER_COLUMN], phoneEnabled); + delete row[MANAGER_COLUMN]; const identity = resolveRowIdentity(row, { policy, phoneEnabled, emailInviteOk, smsInviteOk }); if (identity.invalid) { results[i] = { row: i + 1, ok: false, action: 'failed', code: identity.invalid.code, error: identity.invalid.error }; @@ -336,6 +477,8 @@ export async function runAdminImportUsers( if (identity.email) row.email = identity.email; else delete row.email; if (identity.phone) row.phone_number = identity.phone; else delete row.phone_number; if (identity.plan) planByKey.set(identityKey(identity.email, identity.phone), identity.plan); + if (wanted !== 'absent') managerWanted[i] = wanted; + rowIdentities[i] = identity; validRows.push(row); validIndex.push(i); } @@ -487,6 +630,10 @@ export async function runAdminImportUsers( // ── Post-write phases (skipped on dryRun) ───────────────────────────── const delivery = { emailInvite: 0, smsInvite: 0, temporary: 0 }; + // [#18028] How the second pass went. Declared out here so the summary + // reports the same three keys on a dry run — where the pass does not run — + // as it does on a real one. + const managerLinks = { linked: 0, unresolved: 0, refused: 0 }; if (!prepared.dryRun) { // One pass over every created row — each is in exactly one of the two // maps (or neither, for `none` and updated rows). This single loop serves @@ -534,6 +681,110 @@ export async function runAdminImportUsers( } } + // ── Second pass — the manager links (#18028, ruling row 7) ────────── + // It runs HERE, after `runImport` has returned, and that is the whole + // point: a manager named in row 40 may be created by row 90, so every row + // in the batch has to exist before any key is resolved. A resolve inside + // `createData` would refuse exactly that input, and would happen to work + // only on a file whose rows were already topologically ordered. + if (managerWanted.some((w) => w !== undefined)) { + // This batch's landed rows, indexed under BOTH spellings a manager cell + // may name them by — a row carrying an email AND a phone is reachable + // through either. `identityKey`'s `e:` / `p:` prefixes keep the two + // namespaces from colliding. + const idByKey = new Map(); + for (let i = 0; i < results.length; i++) { + const r = results[i]; + const ident = rowIdentities[i]; + if (!r || !r.id || !ident) continue; + if (r.action !== 'created' && r.action !== 'updated') continue; + if (ident.email) idByKey.set(identityKey(ident.email, undefined), r.id); + if (ident.phone) idByKey.set(identityKey(undefined, ident.phone), r.id); + } + + // A manager who is NOT in this file is still a valid manager — an org + // chart is grown one batch at a time. Memoized per key: a file where 200 + // rows report to the same person is the ordinary case, not the exotic one. + const fromTree = new Map(); + const managerDeps: SetUserManagerDeps = { + getDataEngine: () => engine, + ...(deps.logger ? { logger: deps.logger } : {}), + }; + + for (let i = 0; i < results.length; i++) { + const wanted = managerWanted[i]; + if (wanted === undefined) continue; + const r = results[i]; + // A row that failed its own write has no id to hang a link on. Its + // failure is already reported; the manager cell adds nothing. + if (!r || !r.id || (r.action !== 'created' && r.action !== 'updated')) continue; + + if (wanted === 'unreadable') { + noteManagerFailure( + r, + 'unresolved', + 'MANAGER_UNRESOLVED', + `The ${MANAGER_COLUMN} cell is neither an email address nor a phone number this deployment ` + + 'can read, so it names no identity. This row landed; only its manager link did not.', + ); + managerLinks.unresolved++; + continue; + } + + let managerId = idByKey.get(wanted.key) ?? null; + if (!managerId) { + if (!fromTree.has(wanted.key)) { + try { + const where = wanted.email ? { email: wanted.email } : { phone_number: wanted.phone }; + const found = await engine.find('sys_user', { + where, fields: ['id'], limit: 1, context: SYSTEM_CTX, + }); + const hit = Array.isArray(found) ? found[0] : undefined; + fromTree.set(wanted.key, hit?.id != null ? String(hit.id) : null); + } catch (e) { + // A read that FAILED is not the fact "no such user", and it must + // not be spelled like one silently. The row is reported as + // unresolved either way — the link genuinely was not written — + // and the reason it could not be answered is said once, here. + fromTree.set(wanted.key, null); + deps.logger?.warn( + `[AuthPlugin] import-users: the sys_user lookup for manager key '${wanted.key}' FAILED, so ` + + 'that row is reported as an unresolved manager rather than a refused one. The user ' + + 'itself was created. Remedy: restore read access to sys_user and re-run the manager ' + + `link for the affected rows. Cause: ${(e as Error)?.message ?? e}`, + ); + } + } + managerId = fromTree.get(wanted.key) ?? null; + } + + if (!managerId) { + noteManagerFailure( + r, + 'unresolved', + 'MANAGER_UNRESOLVED', + `No user matches this row's ${MANAGER_COLUMN} key, in this import or already in the ` + + 'directory, so the manager link was not written. The rest of this row landed.', + ); + managerLinks.unresolved++; + continue; + } + + // ⛔ The five refusals are NOT re-derived here. `applyUserManagerLink` + // is the same derivation `POST /admin/set-user-manager` runs, and its + // refusal — status, code and the `reason` discriminator — is reported + // as it came back. + const refusal = await applyUserManagerLink(managerDeps, r.id, managerId); + if (refusal) { + noteManagerFailure(r, refusal.reason, 'MANAGER_REFUSED', refusal.message); + managerLinks.refused++; + continue; + } + r.manager = 'linked'; + managerLinks.linked++; + } + } + // Run-level audit. Best-effort; NO password material. // // Corrected rationale (#4940): this used to read "better-auth writes @@ -575,6 +826,10 @@ export async function runAdminImportUsers( skipped: summary.skipped, errors: summary.errors + preErrors, // How `auto` (and the fixed policies) split the batch across channels. delivery, + // [#18028] And how the second pass's manager links went — a run + // that linked nobody because every key was unresolved is the shape + // an operator most needs to find later. + manager: managerLinks, }), }, { context: SYSTEM_CTX } as any); } catch (e) { @@ -622,6 +877,11 @@ export async function runAdminImportUsers( // Per-channel split of the created rows — the value of `auto`: how // many rows were invited vs. fell back to a temporary password. delivery, + // [#18028] The second pass's split. `unresolved` and `refused` are + // both per-row failures on rows that otherwise LANDED, so they are + // reported here rather than folded into `errors`, which would make + // `created` and `errors` disagree about the same row. + manager: managerLinks, mode, matchBy, }, diff --git a/packages/plugins/plugin-auth/src/admin-set-user-manager.ts b/packages/plugins/plugin-auth/src/admin-set-user-manager.ts index 80f36ea1b4..513ad5d0ac 100644 --- a/packages/plugins/plugin-auth/src/admin-set-user-manager.ts +++ b/packages/plugins/plugin-auth/src/admin-set-user-manager.ts @@ -179,13 +179,39 @@ void _assignableToEndpointResult; type UserRow = { id?: unknown; manager_id?: unknown; source?: unknown }; -function refuse( +/** + * A refusal BEFORE it is dressed as an HTTP envelope. + * + * {@link applyUserManagerLink} answers in this shape so a caller that is not an + * HTTP request — the bulk importer's second pass (#18028) — routes the very + * same predicates onto its own per-row channel. ⛔ A second copy of the five + * refusals inside the importer is the drift #15706 already cost this platform + * once; there is ONE derivation and this is it. + */ +export interface SetUserManagerRefusal { + status: number; + code: string; + reason: SetUserManagerRefusalReason; + message: string; +} + +function deny( status: number, code: string, reason: SetUserManagerRefusalReason, message: string, -): SetUserManagerResult { - return { status, body: { success: false, error: { code, message, details: { reason } } } }; +): SetUserManagerRefusal { + return { status, code, reason, message }; +} + +function envelope(refusal: SetUserManagerRefusal): SetUserManagerResult { + return { + status: refusal.status, + body: { + success: false, + error: { code: refusal.code, message: refusal.message, details: { reason: refusal.reason } }, + }, + }; } async function parseJson(request: Request): Promise> { @@ -339,32 +365,64 @@ export async function runSetUserManager( const rawUserId = readId(body, 'userId', 'user_id'); if (typeof rawUserId !== 'string' || rawUserId.length === 0) { - return refuse(400, 'INVALID_REQUEST', 'invalid_body', 'userId is required'); + return envelope(deny(400, 'INVALID_REQUEST', 'invalid_body', 'userId is required')); } const userId = rawUserId; const rawManagerId = readId(body, 'managerId', 'manager_id'); if (rawManagerId === undefined) { - return refuse( + return envelope(deny( 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( + return envelope(deny( 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 refusal = await applyUserManagerLink(deps, userId, managerId); + if (refusal) return envelope(refusal); + + return { + status: 200, + body: { success: true, data: { userId, managerId, setBy: actor.id } }, + }; +} + +/** + * The ruled refusals and — when they all pass — the write, applied to ONE + * `(user, manager)` pair. The seam #18028 needed: `runSetUserManager` is this + * function plus body parsing and an HTTP envelope, and the bulk importer's + * second pass is this function called once per row. + * + * Answers `null` when the link was written, or the {@link SetUserManagerRefusal} + * that stopped it. Every refusal keeps its `status` and `error.code` so a + * caller with an HTTP envelope to fill can render it unchanged, and every + * caller that has some other channel — a `rows[]` report, say — still gets the + * one discriminator that matters (`reason`) without restating a predicate. + * + * ⚠️ The caller is responsible for AUTHORIZATION. This function has no view of + * who is asking: `runSetUserManager` is reached only through the mount's + * ADR-0068 platform-admin gate, and the importer is reached only through the + * identical gate on `/admin/import-users`. ⛔ Never call it from a surface that + * is not already admin-gated. + */ +export async function applyUserManagerLink( + deps: SetUserManagerDeps, + userId: string, + managerId: string | null, +): Promise { const engine = deps.getDataEngine(); if (!engine) { - return refuse( + return deny( 503, 'SERVICE_UNAVAILABLE', 'engine_unavailable', @@ -374,13 +432,13 @@ export async function runSetUserManager( const user = await findUser(engine, userId); if (!user) { - return refuse(404, 'RESOURCE_NOT_FOUND', 'user_not_found', 'User not found'); + return deny(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( + return deny( 403, 'PERMISSION_DENIED', 'idp_provisioned', @@ -393,7 +451,7 @@ export async function runSetUserManager( if (managerId !== null) { // Refusal 1 — self-assignment. if (managerId === userId) { - return refuse( + return deny( 400, 'INVALID_FIELD', 'self_assignment', @@ -404,7 +462,7 @@ export async function runSetUserManager( const manager = await findUser(engine, managerId); if (!manager) { - return refuse( + return deny( 400, 'INVALID_REFERENCE', 'manager_not_found', @@ -415,7 +473,7 @@ export async function runSetUserManager( // Refusal 4 — cross-organization. const screen = await screenCrossOrganization(deps, engine, userId, managerId); if (screen.outside) { - return refuse( + return deny( 400, 'INVALID_REFERENCE', 'cross_organization', @@ -429,7 +487,7 @@ export async function runSetUserManager( // Refusals 2 and 3 — cycle and depth, in one walk. const walk = await walkChain(engine, managerId, userId); if (walk.kind === 'cycle') { - return refuse( + return deny( 409, 'RESOURCE_CONFLICT', 'cycle', @@ -439,7 +497,7 @@ export async function runSetUserManager( ); } if (walk.kind === 'existing_loop') { - return refuse( + return deny( 409, 'RESOURCE_CONFLICT', 'cycle', @@ -449,7 +507,7 @@ export async function runSetUserManager( ); } if (walk.kind === 'too_deep') { - return refuse( + return deny( 400, 'VALUE_OUT_OF_RANGE', 'max_depth_exceeded', @@ -469,8 +527,5 @@ export async function runSetUserManager( 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 } }, - }; + return null; } From a54a1c60484d2e30791582bfea1351aba6f26013 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 13:22:14 +0000 Subject: [PATCH 2/4] test(plugin-auth): pin the manager second pass, its per-row errors and the refusal fence Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj --- .../admin-import-users-manager-pass.test.ts | 550 ++++++++++++++++++ .../plugin-auth/src/admin-import-users.ts | 20 +- 2 files changed, 569 insertions(+), 1 deletion(-) create mode 100644 packages/plugins/plugin-auth/src/admin-import-users-manager-pass.test.ts diff --git a/packages/plugins/plugin-auth/src/admin-import-users-manager-pass.test.ts b/packages/plugins/plugin-auth/src/admin-import-users-manager-pass.test.ts new file mode 100644 index 0000000000..c397713a39 --- /dev/null +++ b/packages/plugins/plugin-auth/src/admin-import-users-manager-pass.test.ts @@ -0,0 +1,550 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `POST /api/v1/auth/admin/import-users` — ruling row 7 (#18028): `manager_id` + * admitted to the import tier, resolved in a SECOND pass keyed on the + * importer's identity key, every row-2 refusal applied per row, and an + * unresolved key reported as a per-row error. + * + * Four things this suite is built to prove, and three of them are built so a + * lesser implementation FAILS them rather than merely not being tested: + * + * 1. **The pass is genuinely second.** A pin that only checks "a manager got + * resolved" passes against a single-pass resolve that happens to work + * because the fixture was written in dependency order. So the fixture here + * is deliberately out of order — a row whose manager is created by a LATER + * row — and the discrimination is asserted twice over: the harness records + * what `sys_user` held at the moment each row was written (the manager was + * NOT there), and the link's `update` is asserted to land after the last + * `createUser`. A single-pass implementation cannot produce either. + * 2. **A per-row error is per-row.** Both directions, because a test that + * asserts only the first passes against an implementation that aborts the + * whole import: the offending row reports, AND every other row still lands. + * 3. **The five refusals are the shared derivation's, not a copy.** Four of + * them are driven through the importer and asserted to surface with the + * endpoint's own `reason` discriminator, and the file is read to assert it + * spells none of those predicates itself — with a positive control, so a + * rename cannot turn the absence into a vacuous pass. + * 4. **Tier 1 did not move.** `manager_id` is reached by system context, so + * `SYS_USER_PROFILE_EDIT_FIELDS` and `SYS_USER_IMPORT_UPDATE_FIELDS` are + * asserted UNCHANGED here as part of the fix, exactly as #16678's own + * delivery pinned them. + * + * The engine double pins `update` to the real dispatch contract + * (`assertEngineUpdateDispatch`) and its WHERE matcher REFUSES combinators by + * throwing rather than answering them wrongly — the cheap correct answer for a + * double that only ever sees scalar equality. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { runAdminImportUsers, type IdentityImportDeps } from './admin-import-users.js'; +import { SYS_USER_PROFILE_EDIT_FIELDS, SYS_USER_IMPORT_UPDATE_FIELDS } from './sys-user-writable-fields.js'; +import type { AdminActor } from './admin-user-endpoints.js'; + +const ACTOR: AdminActor = { id: 'usr_admin', email: 'admin@example.com' }; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const IMPORT_USERS_SOURCE = readFileSync(resolve(HERE, 'admin-import-users.ts'), 'utf8'); + +type Row = Record; + +function makeRequest(body: unknown): Request { + return new Request('http://localhost/api/v1/auth/admin/import-users', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }); +} + +/** + * Equality on a field name — every predicate this endpoint and its delegate + * issue — and a LOUD refusal of everything else. A double that reads `$or` 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 makeHarness(opts: { + seedUsers?: Row[]; + seedMembers?: Row[]; + phoneEnabled?: boolean; + emailAvailable?: boolean; + smsInviteAvailable?: boolean; + resetFails?: boolean; + /** Make every `sys_user` READ throw, to drive the failed-lookup branch. */ + failUserReads?: boolean; +} = {}) { + const tables: Record = { + sys_user: (opts.seedUsers ?? []).map((u) => ({ manager_id: null, source: 'env_native', ...u })), + sys_member: [...(opts.seedMembers ?? [])], + sys_audit_log: [], + }; + + /** + * What `sys_user` held at the instant each row was written. This is the + * discrimination for the forward reference: if the manager's email is not in + * the snapshot taken when its report was created, then no single-pass + * implementation could have resolved that row. + */ + const snapshotsAtCreate: string[][] = []; + + let nextId = 1; + let readsFail = opts.failUserReads === true; + const createUser = vi.fn(async ({ body }: any) => { + snapshotsAtCreate.push(tables.sys_user.map((u) => String(u.email ?? ''))); + const id = `u-${nextId++}`; + tables.sys_user.push({ + id, + email: body.email, + name: body.name, + phone_number: body?.data?.phoneNumber ?? null, + manager_id: null, + source: 'env_native', + }); + return { user: { id, email: body.email, name: body.name } }; + }); + const requestPasswordReset = vi.fn(async () => { + if (opts.resetFails) throw new Error('smtp down'); + return { status: true }; + }); + + const find = vi.fn(async (object: string, query?: any) => { + if (readsFail && object === 'sys_user') throw new Error(`read of ${object} failed`); + const q = query ?? {}; + const rows = tables[object] ?? []; + const out = rows.filter((r) => matchesWhere(r, q.where ?? {})); + return typeof q.limit === 'number' ? out.slice(0, q.limit) : out; + }); + const update = vi.fn(async (object: string, data: any, options?: any) => { + // The real engine's three-way dispatch — a double looser than this is no + // double at all. + assertEngineUpdateDispatch(data, options); + const row = (tables[object] ?? []).find((r) => r.id === data.id); + if (row) Object.assign(row, data); + return row ?? null; + }); + const insert = vi.fn(async (object: string, data: any) => { + (tables[object] ??= []).push({ ...data }); + return {}; + }); + + const warn = vi.fn(); + const sendInviteSms = vi.fn(async () => {}); + const noteMustChangePasswordIssued = vi.fn(); + + const deps: IdentityImportDeps = { + getAuthApi: async () => ({ createUser, requestPasswordReset }), + getDataEngine: () => ({ find, update, insert }), + phoneNumberEnabled: () => opts.phoneEnabled ?? false, + emailServiceAvailable: () => opts.emailAvailable ?? true, + smsInviteAvailable: () => opts.smsInviteAvailable ?? false, + sendInviteSms, + noteMustChangePasswordIssued, + logger: { warn }, + }; + + return { + deps, tables, snapshotsAtCreate, + createUser, requestPasswordReset, find, update, insert, warn, + failUserReadsFrom: () => { readsFail = true; }, + userBy: (email: string) => tables.sys_user.find((u) => u.email === email), + }; +} + +/** The `data` half of a 200, typed loosely on purpose — it is a wire payload. */ +function payload(res: { body: { data?: unknown } }): any { + return res.body.data as any; +} + +/** The index of the `update` call that wrote a manager link, or -1. */ +function managerWriteIndex(update: ReturnType): number { + return update.mock.calls.findIndex( + ([object, data]: any[]) => object === 'sys_user' && data && Object.hasOwn(data, 'manager_id'), + ); +} + +describe('import-users manager pass — the SECOND pass is genuinely second (#18028)', () => { + const forwardReference = { + passwordPolicy: 'none', + format: 'json', + rows: [ + // Row 1 names a manager that row 2 creates. A single-pass resolve + // refuses this; the ruling's second pass is what admits it. + { email: 'report@x.co', name: 'Report', manager_id: 'boss@x.co' }, + { email: 'boss@x.co', name: 'Boss' }, + ], + }; + + it('links a row to a manager CREATED BY A LATER ROW', async () => { + const h = makeHarness(); + const res = await runAdminImportUsers(h.deps, makeRequest(forwardReference), ACTOR); + + expect(res.status).toBe(200); + const data = payload(res); + expect(data.summary.created).toBe(2); + expect(data.summary.errors).toBe(0); + expect(data.summary.manager).toEqual({ linked: 1, unresolved: 0, refused: 0 }); + expect(data.rows[0].manager).toBe('linked'); + expect(data.rows[0].code).toBeUndefined(); + // The link is on the row, pointing at the id the LATER row minted. + expect(h.userBy('report@x.co')?.manager_id).toBe(h.userBy('boss@x.co')?.id); + }); + + it('CONTROL — the manager did not exist when its report was written', async () => { + // Without this the test above is explainable by a fixture that happened to + // be in dependency order. It was not: at the instant row 1 was created, + // `sys_user` did not contain the manager at all. + const h = makeHarness(); + await runAdminImportUsers(h.deps, makeRequest(forwardReference), ACTOR); + + expect(h.snapshotsAtCreate).toHaveLength(2); + expect(h.snapshotsAtCreate[0]).not.toContain('boss@x.co'); + expect(h.snapshotsAtCreate[1]).toContain('report@x.co'); + }); + + it('CONTROL — the link is written AFTER the last row was created', async () => { + const h = makeHarness(); + await runAdminImportUsers(h.deps, makeRequest(forwardReference), ACTOR); + + const idx = managerWriteIndex(h.update); + expect(idx).toBeGreaterThanOrEqual(0); + const linkOrder = h.update.mock.invocationCallOrder[idx]; + const createOrders = h.createUser.mock.invocationCallOrder; + expect(createOrders).toHaveLength(2); + // Ordering a single-pass implementation structurally cannot produce: the + // link postdates EVERY creation in the batch, not just its own row's. + expect(linkOrder).toBeGreaterThan(createOrders[createOrders.length - 1]); + }); + + it('resolves a manager ALREADY IN THE TREE, not named by this file', async () => { + const h = makeHarness({ + seedUsers: [{ id: 'u_boss', email: 'boss@x.co', name: 'Boss' }], + }); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [{ email: 'new@x.co', name: 'New', manager_id: 'boss@x.co' }], + }), ACTOR); + + expect(res.status).toBe(200); + expect(payload(res).rows[0].manager).toBe('linked'); + expect(h.userBy('new@x.co')?.manager_id).toBe('u_boss'); + }); + + it('keys on PHONE as well as email — the ruling names both', async () => { + const h = makeHarness({ phoneEnabled: true }); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [ + { email: 'report@x.co', name: 'Report', manager_id: '+15550001111' }, + { phone_number: '+1 555 000 1111', name: 'Boss' }, + ], + }), ACTOR); + + expect(res.status).toBe(200); + const data = payload(res); + expect(data.summary.manager).toEqual({ linked: 1, unresolved: 0, refused: 0 }); + // The phone-only row's email is a minted placeholder, so the ONLY key that + // could have matched here is the phone one. + const boss = h.tables.sys_user.find((u) => u.phone_number === '+15550001111'); + expect(boss).toBeTruthy(); + expect(h.userBy('report@x.co')?.manager_id).toBe(boss?.id); + }); +}); + +describe('import-users manager pass — an unresolved key is PER-ROW (#18028)', () => { + it('reports the offending row AND lands every other row', async () => { + const h = makeHarness(); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [ + { email: 'orphan@x.co', name: 'Orphan', manager_id: 'ghost@x.co' }, + { email: 'fine@x.co', name: 'Fine' }, + ], + }), ACTOR); + + // Direction 1 — the row reports. + expect(res.status).toBe(200); + const data = payload(res); + expect(data.rows[0].manager).toBe('unresolved'); + expect(data.rows[0].code).toBe('MANAGER_UNRESOLVED'); + expect(String(data.rows[0].error)).toContain('manager_id'); + + // Direction 2 — and it is not a whole-import failure. Asserting only the + // first would pass against an implementation that aborts everything. + expect(h.createUser).toHaveBeenCalledTimes(2); + expect(data.summary.created).toBe(2); + expect(data.summary.errors).toBe(0); + expect(data.rows[1].code).toBeUndefined(); + expect(data.rows[1].manager).toBeUndefined(); + expect(data.summary.manager).toEqual({ linked: 0, unresolved: 1, refused: 0 }); + + // ⛔ Not a silent skip either: the identity landed, unlinked. + expect(h.userBy('orphan@x.co')).toBeTruthy(); + expect(h.userBy('orphan@x.co')?.manager_id ?? null).toBeNull(); + }); + + it('a cell that is neither an email nor a phone is unresolved, not ignored', async () => { + const h = makeHarness(); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [{ email: 'a@x.co', name: 'A', manager_id: 'Jane Doe' }], + }), ACTOR); + + const data = payload(res); + expect(data.rows[0].manager).toBe('unresolved'); + expect(data.rows[0].code).toBe('MANAGER_UNRESOLVED'); + expect(data.summary.manager.unresolved).toBe(1); + }); + + it('an EMPTY manager cell is not a finding', async () => { + const h = makeHarness(); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [{ email: 'a@x.co', name: 'A', manager_id: ' ' }], + }), ACTOR); + + const data = payload(res); + expect(data.rows[0].manager).toBeUndefined(); + expect(data.rows[0].code).toBeUndefined(); + expect(data.summary.manager).toEqual({ linked: 0, unresolved: 0, refused: 0 }); + }); + + it('a manager failure does not overwrite a delivery failure, or vice versa', async () => { + // Both post-write phases have something to report about the SAME row. The + // shared `code`/`error` slot belongs to whoever claimed it first; the + // manager verdict is still readable on its own field, so neither failure + // is lost into silence. + const h = makeHarness({ resetFails: true }); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'invite', format: 'json', + rows: [{ email: 'a@x.co', name: 'A', manager_id: 'ghost@x.co' }], + }), ACTOR); + + const data = payload(res); + expect(data.rows[0].code).toBe('INVITE_EMAIL_FAILED'); + expect(data.rows[0].manager).toBe('unresolved'); + expect(data.summary.manager.unresolved).toBe(1); + }); + + it('an engine fault DURING the link stays a per-row error, not a 500', async () => { + // By this point every identity in the batch is written. A fault here must + // not turn a 200 that created N users into a 500 that reports none of them. + const h = makeHarness({ seedUsers: [{ id: 'u_boss', email: 'boss@x.co', name: 'Boss' }] }); + h.update.mockImplementation(async () => { throw new Error('driver offline'); }); + + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [{ email: 'a@x.co', name: 'A', manager_id: 'boss@x.co' }], + }), ACTOR); + + expect(res.status).toBe(200); + const data = payload(res); + expect(data.summary.created).toBe(1); + expect(data.rows[0].manager).toBe('unresolved'); + expect(String(data.rows[0].error)).toContain('driver offline'); + }); + + it('a FAILED sys_user lookup is reported as unresolved AND said out loud', async () => { + // "the read did not happen" and "no such user" are different facts. The + // link genuinely was not written either way, so the row reads the same — + // but the reason is stated once rather than degrading in silence. + const h = makeHarness(); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [{ email: 'a@x.co', name: 'A', manager_id: 'boss@x.co' }], + }), ACTOR); + void res; + + const h2 = makeHarness(); + // Fail reads only once the identity writes are done, so the row still lands. + h2.createUser.mockImplementationOnce(async ({ body }: any) => { + const created = { user: { id: 'u-1', email: body.email, name: body.name } }; + h2.tables.sys_user.push({ id: 'u-1', email: body.email, name: body.name, manager_id: null, source: 'env_native' }); + h2.failUserReadsFrom(); + return created; + }); + const res2 = await runAdminImportUsers(h2.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [{ email: 'a@x.co', name: 'A', manager_id: 'boss@x.co' }], + }), ACTOR); + + const data = payload(res2); + expect(data.rows[0].manager).toBe('unresolved'); + expect(h2.warn.mock.calls.some(([m]: any[]) => String(m).includes('FAILED'))).toBe(true); + expect(h2.warn.mock.calls.some(([m]: any[]) => String(m).includes('Remedy'))).toBe(true); + }); +}); + +describe('import-users manager pass — the refusals are the DELEGATE\'s (#18028)', () => { + it('self-assignment — a row naming itself surfaces the endpoint\'s reason', async () => { + const h = makeHarness(); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [{ email: 'a@x.co', name: 'A', manager_id: 'a@x.co' }], + }), ACTOR); + + const data = payload(res); + expect(data.rows[0].manager).toBe('self_assignment'); + expect(data.rows[0].code).toBe('MANAGER_REFUSED'); + expect(data.summary.manager).toEqual({ linked: 0, unresolved: 0, refused: 1 }); + expect(h.userBy('a@x.co')?.manager_id ?? null).toBeNull(); + }); + + it('cycle — refused against the links THIS PASS has already written', async () => { + // Row 1's link is in place by the time row 2 is judged, so the loop is + // caught inside one import rather than left for a later read to trip over. + const h = makeHarness(); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [ + { email: 'a@x.co', name: 'A', manager_id: 'b@x.co' }, + { email: 'b@x.co', name: 'B', manager_id: 'a@x.co' }, + ], + }), ACTOR); + + const data = payload(res); + expect(data.rows[0].manager).toBe('linked'); + expect(data.rows[1].manager).toBe('cycle'); + expect(data.rows[1].code).toBe('MANAGER_REFUSED'); + expect(data.summary.manager).toEqual({ linked: 1, unresolved: 0, refused: 1 }); + expect(h.userBy('b@x.co')?.manager_id ?? null).toBeNull(); + }); + + it('directory-owned identity — an upsert row the IdP owns is refused', async () => { + const h = makeHarness({ + seedUsers: [ + { id: 'u_sso', email: 'sso@x.co', name: 'SSO', source: 'idp_provisioned' }, + { id: 'u_boss', email: 'boss@x.co', name: 'Boss' }, + ], + }); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', mode: 'upsert', matchBy: 'email', format: 'json', + rows: [{ email: 'sso@x.co', name: 'SSO', manager_id: 'boss@x.co' }], + }), ACTOR); + + const data = payload(res); + expect(data.rows[0].action).toBe('updated'); + expect(data.rows[0].manager).toBe('idp_provisioned'); + expect(data.summary.manager.refused).toBe(1); + expect(h.userBy('sso@x.co')?.manager_id ?? null).toBeNull(); + }); + + it('cross-organization — the delegate\'s sys_member screen really runs', async () => { + const h = makeHarness({ + seedUsers: [ + { id: 'u_report', email: 'report@x.co', name: 'Report' }, + { id: 'u_boss', email: 'boss@x.co', name: 'Boss' }, + ], + seedMembers: [ + { user_id: 'u_report', organization_id: 'org_a' }, + { user_id: 'u_boss', organization_id: 'org_b' }, + ], + }); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', mode: 'upsert', matchBy: 'email', format: 'json', + rows: [{ email: 'report@x.co', name: 'Report', manager_id: 'boss@x.co' }], + }), ACTOR); + + const data = payload(res); + expect(data.rows[0].manager).toBe('cross_organization'); + expect(data.summary.manager.refused).toBe(1); + // The screen is only reachable through the delegate — nothing in the + // importer reads sys_member. + expect(h.find.mock.calls.some(([object]: any[]) => object === 'sys_member')).toBe(true); + }); + + it('⛔ the importer carries NO second copy of the five predicates', () => { + // POSITIVE CONTROL — the delegation is really in this file, so the + // absences below are readings rather than a vacuous pass after a rename. + expect(IMPORT_USERS_SOURCE).toContain('applyUserManagerLink'); + expect(IMPORT_USERS_SOURCE).toContain('admin-set-user-manager.js'); + + // Each refusal reason is the delegate's to name. A fork would spell them. + for (const reason of [ + 'self_assignment', 'cycle', 'max_depth_exceeded', 'cross_organization', 'idp_provisioned', + ]) { + expect(IMPORT_USERS_SOURCE).not.toContain(`'${reason}'`); + } + // And none of the predicates' own machinery. + expect(IMPORT_USERS_SOURCE).not.toContain('MAX_MANAGER_CHAIN_DEPTH'); + expect(IMPORT_USERS_SOURCE).not.toContain('sys_member'); + }); +}); + +describe('import-users manager pass — the fences (#18028)', () => { + it('the manager cell never reaches the identity write path', async () => { + const h = makeHarness(); + await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', format: 'json', + rows: [ + { email: 'report@x.co', name: 'Report', manager_id: 'boss@x.co' }, + { email: 'boss@x.co', name: 'Boss' }, + ], + }), ACTOR); + + // better-auth never sees it (it would be a lookup column handed an email). + expect(JSON.stringify(h.createUser.mock.calls)).not.toContain('manager_id'); + // And the ONLY `manager_id` write is the delegate's, one per link. + const managerWrites = h.update.mock.calls.filter( + ([object, data]: any[]) => object === 'sys_user' && data && Object.hasOwn(data, 'manager_id'), + ); + expect(managerWrites).toHaveLength(1); + }); + + it('an upsert patch still cannot carry manager_id', async () => { + const h = makeHarness({ seedUsers: [{ id: 'u_a', email: 'a@x.co', name: 'A' }] }); + await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', mode: 'upsert', matchBy: 'email', format: 'json', + rows: [{ email: 'a@x.co', name: 'A2', manager_id: 'Jane Doe' }], + }), ACTOR); + + const patches = h.update.mock.calls.filter(([object]: any[]) => object === 'sys_user'); + for (const [, data] of patches) expect(Object.hasOwn(data, 'manager_id')).toBe(false); + }); + + it('⛔ Tier 1 did not move — the column is reached by system context', () => { + // #16678's delivery pinned this ABSENCE; admitting the column to the + // import tier must leave it exactly as pinned. + expect([...SYS_USER_PROFILE_EDIT_FIELDS].sort()).toEqual(['image', 'locale', 'name']); + expect([...SYS_USER_IMPORT_UPDATE_FIELDS].sort()).toEqual( + ['image', 'locale', 'name', 'phone_number', 'role'], + ); + expect(SYS_USER_IMPORT_UPDATE_FIELDS.has('manager_id')).toBe(false); + }); + + it('dryRun does not run the pass, and does not half-answer about it', async () => { + const h = makeHarness(); + const res = await runAdminImportUsers(h.deps, makeRequest({ + passwordPolicy: 'none', dryRun: true, format: 'json', + rows: [ + { email: 'report@x.co', name: 'Report', manager_id: 'boss@x.co' }, + { email: 'boss@x.co', name: 'Boss' }, + ], + }), ACTOR); + + const data = payload(res); + expect(data.summary.dryRun).toBe(true); + expect(data.summary.manager).toEqual({ linked: 0, unresolved: 0, refused: 0 }); + expect(managerWriteIndex(h.update)).toBe(-1); + expect(data.rows.every((r: Row) => r.manager === undefined)).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-auth/src/admin-import-users.ts b/packages/plugins/plugin-auth/src/admin-import-users.ts index a5e69888b0..295593f154 100644 --- a/packages/plugins/plugin-auth/src/admin-import-users.ts +++ b/packages/plugins/plugin-auth/src/admin-import-users.ts @@ -111,6 +111,7 @@ import { SYS_USER_IMPORT_UPDATE_FIELDS } from './sys-user-writable-fields.js'; import { applyUserManagerLink, type SetUserManagerDeps, + type SetUserManagerRefusal, type SetUserManagerRefusalReason, } from './admin-set-user-manager.js'; @@ -774,7 +775,24 @@ export async function runAdminImportUsers( // is the same derivation `POST /admin/set-user-manager` runs, and its // refusal — status, code and the `reason` discriminator — is reported // as it came back. - const refusal = await applyUserManagerLink(managerDeps, r.id, managerId); + let refusal: SetUserManagerRefusal | null; + try { + refusal = await applyUserManagerLink(managerDeps, r.id, managerId); + } catch (e) { + // Every identity in this batch is ALREADY written by now. An engine + // fault while linking them must not turn a 200 that created N users + // into a 500 reporting none of them — that is exactly the + // whole-import failure the ruling refuses. Per row, and loudly. + noteManagerFailure( + r, + 'unresolved', + 'MANAGER_UNRESOLVED', + `The manager link could not be written: ${(e as Error)?.message ?? String(e)}. ` + + 'This row itself landed.', + ); + managerLinks.unresolved++; + continue; + } if (refusal) { noteManagerFailure(r, refusal.reason, 'MANAGER_REFUSED', refusal.message); managerLinks.refused++; From fcaa95d536241b6c446fff924eee6f223a3af356 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 13:28:38 +0000 Subject: [PATCH 3/4] chore: changeset for the import manager second pass Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj --- .../18028-import-users-manager-second-pass.md | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 .changeset/18028-import-users-manager-second-pass.md diff --git a/.changeset/18028-import-users-manager-second-pass.md b/.changeset/18028-import-users-manager-second-pass.md new file mode 100644 index 0000000000..cdf795bdb6 --- /dev/null +++ b/.changeset/18028-import-users-manager-second-pass.md @@ -0,0 +1,58 @@ +--- +'@objectstack/plugin-auth': minor +--- + +The bulk identity import admits `manager_id`, resolved in a second pass keyed on the importer's identity key + +`POST /api/v1/auth/admin/import-users` now reads a `manager_id` column. Until +this change it matched `manager_id` **0** times — against a positive control of +`email` at 73 — so a CSV naming everyone's manager built the org chart for +nobody, silently: the column was dropped on create (the identity write path +composes its own better-auth body) and filtered out on upsert (it is not in +`SYS_USER_IMPORT_UPDATE_FIELDS`). With +`POST /api/v1/auth/admin/set-user-manager` shipped, the import surface was the +one remaining route that could populate the column at scale and did not. + +**What the cell holds is an identity key, not a user id.** A CSV author has the +manager's email or phone number, never their `usr_…` id, so the cell is read +with the same key the importer already keys rows by. One spelling, `manager_id` +— the phone column's three historical aliases are debt this key does not +inherit. + +**The pass is SECOND, and that is load-bearing.** A manager named in row 40 may +be created by row 90, so the links are applied after the row engine has +returned and every row in the batch exists. A resolve inside the per-row write +would refuse exactly that input and would appear to work only on a file whose +rows happened to arrive in dependency order. A manager who is *not* in the file +is resolved against the directory instead, so an org chart can be grown one +batch at a time. + +**Every refusal is the write surface's, applied per row.** The importer calls +`applyUserManagerLink` — the same derivation `POST /admin/set-user-manager` +runs — so self-assignment, a link that closes a cycle, a chain past the depth +cap, a manager provably outside every organization the user belongs to, and any +identity whose `sys_user.source` is `idp_provisioned` are refused on import +exactly as they are on the endpoint, with the endpoint's own `reason` +discriminator carried through. There is no second copy of those predicates. + +**A manager problem never costs the row its identity.** The user is created +either way; the failure is reported on that row — `rows[].code` is +`MANAGER_UNRESOLVED` or `MANAGER_REFUSED` and `rows[].manager` carries the +machine-readable outcome, in the shape `rows[].delivery` already uses. It is +⛔ not a whole-import failure and ⛔ not a silent skip, and an engine fault +while linking is reported the same way rather than turning a 200 that created +N users into a 500 that reports none of them. + +**New on the response.** `data.summary.manager` is +`{ linked, unresolved, refused }`, beside `data.summary.delivery`, and the +run-level `sys_audit_log` row records the same split. Row objects are typed as +the newly exported `IdentityImportRowResult`, whose `manager` member is an +`ImportManagerOutcome`. + +**Unchanged, deliberately.** `SYS_USER_PROFILE_EDIT_FIELDS` and +`SYS_USER_IMPORT_UPDATE_FIELDS` are untouched — the import reaches the column +by system context, the same way it already reaches `phone_number` and `role`, +and the same way the admin endpoint does. `manager_id` keeps `readonly: true` +on the column. Nothing derives a manager from org-unit membership. A dry run +does not run the pass at all and reports zeroes rather than half-answering +about links it could not evaluate. From 2be67d2d4a7bdfc43fe674ab343ea462ba0ac885 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 14:07:16 +0000 Subject: [PATCH 4/4] fix(plugin-auth): keep the manager outcome off rows[].code, which the spec ledger does not register Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj --- .../18028-import-users-manager-second-pass.md | 16 ++++--- .../admin-import-users-manager-pass.test.ts | 19 ++++++-- .../plugin-auth/src/admin-import-users.ts | 47 ++++++++++++------- scripts/engine-double-contract.pinned.json | 5 ++ 4 files changed, 59 insertions(+), 28 deletions(-) diff --git a/.changeset/18028-import-users-manager-second-pass.md b/.changeset/18028-import-users-manager-second-pass.md index cdf795bdb6..54d056a737 100644 --- a/.changeset/18028-import-users-manager-second-pass.md +++ b/.changeset/18028-import-users-manager-second-pass.md @@ -36,12 +36,16 @@ exactly as they are on the endpoint, with the endpoint's own `reason` discriminator carried through. There is no second copy of those predicates. **A manager problem never costs the row its identity.** The user is created -either way; the failure is reported on that row — `rows[].code` is -`MANAGER_UNRESOLVED` or `MANAGER_REFUSED` and `rows[].manager` carries the -machine-readable outcome, in the shape `rows[].delivery` already uses. It is -⛔ not a whole-import failure and ⛔ not a silent skip, and an engine fault -while linking is reported the same way rather than turning a 200 that created -N users into a 500 that reports none of them. +either way; the failure is reported on that row — `rows[].manager` carries the +machine-readable outcome in the shape `rows[].delivery` already uses +(`unresolved`, or the refusal's own `reason`), and `rows[].error` carries the +sentence. It is ⛔ not a whole-import failure and ⛔ not a silent skip, and an +engine fault while linking is reported the same way rather than turning a 200 +that created N users into a 500 that reports none of them. No `rows[].code` is +stamped for a manager outcome: a row-level code would have to be registered in +the `packages/spec` error-code ledger, which this change is fenced out of, so +the machine-readable half lives on `rows[].manager` instead of on a code the +vocabulary does not carry. **New on the response.** `data.summary.manager` is `{ linked, unresolved, refused }`, beside `data.summary.delivery`, and the diff --git a/packages/plugins/plugin-auth/src/admin-import-users-manager-pass.test.ts b/packages/plugins/plugin-auth/src/admin-import-users-manager-pass.test.ts index c397713a39..f7c5a0c763 100644 --- a/packages/plugins/plugin-auth/src/admin-import-users-manager-pass.test.ts +++ b/packages/plugins/plugin-auth/src/admin-import-users-manager-pass.test.ts @@ -285,8 +285,13 @@ describe('import-users manager pass — an unresolved key is PER-ROW (#18028)', expect(res.status).toBe(200); const data = payload(res); expect(data.rows[0].manager).toBe('unresolved'); - expect(data.rows[0].code).toBe('MANAGER_UNRESOLVED'); expect(String(data.rows[0].error)).toContain('manager_id'); + // ⛔ No row-level `code`: the obvious `MANAGER_UNRESOLVED` symmetry with + // `INVITE_EMAIL_FAILED` needs a `packages/spec` error-code ledger entry + // this lane is fenced out of, and `check:dispatcher-error-vocabulary` + // refuses an unregistered one. Pinned so the symmetry cannot be restored + // without the registration that makes it legal. + expect(data.rows[0].code).toBeUndefined(); // Direction 2 — and it is not a whole-import failure. Asserting only the // first would pass against an implementation that aborts everything. @@ -311,7 +316,8 @@ describe('import-users manager pass — an unresolved key is PER-ROW (#18028)', const data = payload(res); expect(data.rows[0].manager).toBe('unresolved'); - expect(data.rows[0].code).toBe('MANAGER_UNRESOLVED'); + expect(String(data.rows[0].error)).toContain('phone number'); + expect(data.rows[0].code).toBeUndefined(); expect(data.summary.manager.unresolved).toBe(1); }); @@ -341,6 +347,9 @@ describe('import-users manager pass — an unresolved key is PER-ROW (#18028)', const data = payload(res); expect(data.rows[0].code).toBe('INVITE_EMAIL_FAILED'); + expect(String(data.rows[0].error)).toContain('invitation email failed'); + // The delivery report keeps the shared `error` slot; the manager verdict is + // still readable on its own field, so neither failure is lost to silence. expect(data.rows[0].manager).toBe('unresolved'); expect(data.summary.manager.unresolved).toBe(1); }); @@ -404,7 +413,9 @@ describe('import-users manager pass — the refusals are the DELEGATE\'s (#18028 const data = payload(res); expect(data.rows[0].manager).toBe('self_assignment'); - expect(data.rows[0].code).toBe('MANAGER_REFUSED'); + // The delegate's own message, carried through rather than re-worded. + expect(String(data.rows[0].error)).toContain('cannot be their own manager'); + expect(data.rows[0].code).toBeUndefined(); expect(data.summary.manager).toEqual({ linked: 0, unresolved: 0, refused: 1 }); expect(h.userBy('a@x.co')?.manager_id ?? null).toBeNull(); }); @@ -424,7 +435,7 @@ describe('import-users manager pass — the refusals are the DELEGATE\'s (#18028 const data = payload(res); expect(data.rows[0].manager).toBe('linked'); expect(data.rows[1].manager).toBe('cycle'); - expect(data.rows[1].code).toBe('MANAGER_REFUSED'); + expect(String(data.rows[1].error)).toContain('loop'); expect(data.summary.manager).toEqual({ linked: 1, unresolved: 0, refused: 1 }); expect(h.userBy('b@x.co')?.manager_id ?? null).toBeNull(); }); diff --git a/packages/plugins/plugin-auth/src/admin-import-users.ts b/packages/plugins/plugin-auth/src/admin-import-users.ts index 295593f154..0488d3b17e 100644 --- a/packages/plugins/plugin-auth/src/admin-import-users.ts +++ b/packages/plugins/plugin-auth/src/admin-import-users.ts @@ -88,10 +88,11 @@ * identity are that function's to answer, and the refusal it returns is * reported on the row rather than re-worded here. * - **A manager problem never costs the row its identity.** The user is - * created either way and the failure rides `rows[].code` / `rows[].error`, - * exactly like the sibling post-write `INVITE_EMAIL_FAILED`; `rows[].manager` - * carries the machine-readable outcome in `rows[].delivery`'s shape. ⛔ Not a - * whole-import failure, and ⛔ not a silent skip. + * created either way; `rows[].manager` carries the machine-readable outcome + * in `rows[].delivery`'s shape and `rows[].error` carries the sentence. + * ⛔ Not a whole-import failure, and ⛔ not a silent skip. ⛔ And no + * `rows[].code` — see {@link noteManagerFailure} for why that half is fenced + * out of this lane rather than forgotten. * - **The pass does not run on `dryRun`.** It is a post-write pass like * delivery: with nothing created there are no ids to link, and the five * refusals cannot be evaluated against rows that do not exist. A dry run @@ -331,23 +332,36 @@ export interface IdentityImportRowResult extends ImportRowResult { /** * Stamp a row's manager failure. * - * `manager` is ALWAYS set — that is this outcome's own channel. `code`/`error` - * is the SHARED row error channel the sibling `INVITE_EMAIL_FAILED` also writes - * to, so it is claimed only when free: a row whose invitation already failed - * keeps that report and still carries its manager verdict on `manager`, - * ⛔ rather than one of the two failures overwriting the other into silence. + * `manager` is ALWAYS set — that is this outcome's own channel, and it is the + * machine-readable one: `'unresolved'` and each refusal `reason` are distinct + * members of {@link ImportManagerOutcome}, so a caller discriminates on one + * field without parsing a sentence. + * + * ⛔ NO `rows[].code` IS STAMPED, and that is a fence rather than an oversight. + * The sibling post-write failure writes `code: 'INVITE_EMAIL_FAILED'`, and a + * matching `MANAGER_UNRESOLVED` / `MANAGER_REFUSED` pair would read as the + * obvious symmetry — but `check:dispatcher-error-vocabulary` refuses a code + * this package's `packages/spec` ledger entry does not register, and + * registering one is a `packages/spec` edit this lane is fenced out of + * (the closed-vocabulary question for this endpoint's refusals is already + * carried by #17995). So the failure rides `error` — the human half — and + * `manager` — the machine half — and the row-level code is left to the seat + * that owns the vocabulary. ⛔ Reaching for an already-registered code whose + * meaning is something else would be the lenient alias Prime Directive #12 + * refuses. + * + * `error` is the SHARED row channel that sibling also writes to, so it is + * claimed only when free: a row whose invitation already failed keeps that + * report and still carries its manager verdict on `manager`, ⛔ rather than one + * of the two failures overwriting the other into silence. */ function noteManagerFailure( row: IdentityImportRowResult, outcome: ImportManagerOutcome, - code: string, message: string, ): void { row.manager = outcome; - if (row.code === undefined) { - row.code = code; - row.error = message; - } + if (row.error === undefined) row.error = message; } /** @@ -724,7 +738,6 @@ export async function runAdminImportUsers( noteManagerFailure( r, 'unresolved', - 'MANAGER_UNRESOLVED', `The ${MANAGER_COLUMN} cell is neither an email address nor a phone number this deployment ` + 'can read, so it names no identity. This row landed; only its manager link did not.', ); @@ -763,7 +776,6 @@ export async function runAdminImportUsers( noteManagerFailure( r, 'unresolved', - 'MANAGER_UNRESOLVED', `No user matches this row's ${MANAGER_COLUMN} key, in this import or already in the ` + 'directory, so the manager link was not written. The rest of this row landed.', ); @@ -786,7 +798,6 @@ export async function runAdminImportUsers( noteManagerFailure( r, 'unresolved', - 'MANAGER_UNRESOLVED', `The manager link could not be written: ${(e as Error)?.message ?? String(e)}. ` + 'This row itself landed.', ); @@ -794,7 +805,7 @@ export async function runAdminImportUsers( continue; } if (refusal) { - noteManagerFailure(r, refusal.reason, 'MANAGER_REFUSED', refusal.message); + noteManagerFailure(r, refusal.reason, refusal.message); managerLinks.refused++; continue; } diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 68b8a906d9..696c17522f 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2376,6 +2376,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-auth/src/admin-import-users-manager-pass.test.ts", + "verb": "update", + "pinned": 2 + }, { "file": "packages/plugins/plugin-auth/src/admin-import-users.test.ts", "verb": "update",