Skip to content

Commit 344d475

Browse files
os-salesclaude
andauthored
fix(plugin-auth)!: POST /admin/create-user reads the deployment membership policy instead of hard-coding auto (#16683) (#17443)
* fix(plugin-auth)!: admin/create-user reads the deployment membership policy `POST /api/v1/auth/admin/create-user` handed the membership reconciler a literal `policy: 'auto'`, making it the one membership-writing path that did not consult `AuthManager.getMembershipPolicy()`. On an `invite-only` deployment the `user.create.after` reconciler correctly bound nobody and this endpoint-side belt-and-suspenders bind then bound them anyway, answering `membershipCreated: true`. The failure direction was OPEN: it granted the membership the policy exists to withhold. ADR-0093 D1 enumerates the `invite-only` flows as a closed set and refuses "which endpoint created the user" as a determinant, so the endpoint now reads the live accessor through a new optional `AdminUserEndpointDeps.getMembershipPolicy` dep, wired at the mount in auth-plugin.ts. Absent dep falls back to `'auto'`, the accessor's own default, so unwired hosts are byte-identical. The accessor's docblock enumerated "both membership paths" while a third writer sat outside the accounting; it now names all three. Tests: invite-only binds nothing (single-org and multi-org), the org lookup is never reached so `policy-skip` stays distinguishable from `no-target-org`, and a negative control pins the `auto` path byte-identical across an explicit `'auto'`, an unwired dep and a dep returning `undefined`. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW * fix(plugin-auth): the new admin-endpoint assertions add no test-type debt `check:test-typecheck` ledgers this file's TS2493 / TS18048 counts and only ratchets DOWN. The invite-only and negative-control assertions read `engineInsert.mock.calls` directly, and `vi.fn(async () => ({}))` infers a ZERO-length parameter tuple, so every `c[0]` was a fresh TS2493 and the `![1]` audit-row read a fresh TS18048 — six new errors across three ledgered signatures. Read those calls through a typed `callsOf()` helper instead. No type is loosened, no ledger number is raised, no test is skipped: the recorded counts return to exactly what the ledger already holds. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3644fad commit 344d475

5 files changed

Lines changed: 217 additions & 7 deletions

File tree

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
"@objectstack/plugin-auth": minor
3+
---
4+
5+
fix(plugin-auth)!: `POST /admin/create-user` reads the deployment's membership policy instead of hard-coding `auto` (#16683)
6+
7+
**BREAKING** — the membership this published endpoint writes moves for existing inputs on `invite-only` deployments. The route, its request body, its response fields and every exported signature are byte-identical; what changes is what an existing call does on a deployment that declared a non-default policy, stated as a FROM/TO pair below.
8+
9+
ADR-0093 D1 makes the deployment's `membershipPolicy` the one answer to "does this new account get an organization membership", and enumerates the `invite-only` flows as a closed set — "which endpoint created the user" is explicitly not a determinant. The `user.create.after` reconciler and the D6 backfill both read it through `AuthManager.getMembershipPolicy()`. This endpoint did not: its belt-and-suspenders bind handed the reconciler a literal `'auto'`, so it was the one membership-writing path in the product that ignored the setting.
10+
11+
FROM: on a deployment declaring `membershipPolicy: 'invite-only'`, an account created through `POST /api/v1/auth/admin/create-user` was bound to the default organization anyway, and the 200 response answered `membershipCreated: true`. The `user.create.after` reconciler had already declined to bind it; this endpoint bound it afterwards.
12+
13+
TO: the same call creates the account and binds no membership. The response answers `membershipCreated: false` and omits `organizationId`, and the audit row records the same. The account is created and can sign in — `invite-only` withholds the membership, not the login.
14+
15+
Who is affected: only deployments that set `auth.membership_policy` (or `OS_AUTH_MEMBERSHIP_POLICY`) to `invite-only`. Under the default `auto` posture behaviour is unchanged in every observable respect — response body, `sys_member` write and audit metadata — and that equivalence is pinned by a test rather than asserted here.
16+
17+
If you relied on admin-created accounts acquiring a membership on an `invite-only` deployment, the supported way to keep it is to bind the membership explicitly (the `add_member` action / `POST /organization/add-member`), which is what `invite-only` means: memberships are granted deliberately, never as a side effect of account creation. Setting the deployment back to `auto` restores the old behaviour for every path at once, including sign-up.
18+
19+
The direction of the old defect was open, not closed: it GRANTED a membership the operator had configured the platform to withhold, and reported success while doing it. An operator who set `invite-only` specifically to keep a shared organization identity off their users got one anyway.
20+
21+
<!-- adr-0087: not-required (no-migration-prescription) nothing authorable changes shape: no spec key, no Zod schema, no stored metadata and no exported symbol is added, removed or renamed, so `os migrate meta` has no edit to make and no ledger id to carry. What moves is one runtime decision inside an HTTP handler, already governed by the `auth.membership_policy` setting an operator sets and can change back. -->

packages/plugins/plugin-auth/src/admin-user-endpoints.test.ts

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,18 @@ function makeDeps(overrides: Partial<Record<string, any>> = {}) {
5252
return { deps, createUser, engineUpdate, engineCreate, authCtx, warn, noteMustChangePasswordIssued };
5353
}
5454

55+
/**
56+
* Read a mock's recorded calls at their real arity.
57+
*
58+
* `vi.fn(async () => ({}))` infers a ZERO-length parameter tuple, so `c[0]` on
59+
* `mock.calls` is a type error (`TS2493`) even though every recorded call has
60+
* two arguments — the shape `scripts/check-test-typecheck.mts` ledgers for this
61+
* file. New assertions go through here so the ledger keeps ratcheting DOWN.
62+
*/
63+
function callsOf(fn: { mock: { calls: unknown[][] } }): Array<[string, any]> {
64+
return fn.mock.calls as unknown as Array<[string, any]>;
65+
}
66+
5567
/**
5668
* Security red line (#2766): no mock the endpoint touched may ever have seen
5769
* the plaintext password outside the better-auth hashing surface.
@@ -460,6 +472,130 @@ describe('runAdminCreateUser', () => {
460472
expect(data.membershipCreated).toBe(true);
461473
});
462474

475+
// ── ADR-0093 D1: the deployment membership policy governs THIS path too ──
476+
//
477+
// The endpoint used to hand the reconciler a literal `policy: 'auto'`, so an
478+
// `invite-only` deployment — where the `user.create.after` reconciler and the
479+
// D6 backfill both correctly bound nobody — got the membership bound anyway
480+
// by this belt-and-suspenders call, and a `membershipCreated: true` that read
481+
// as success. The failure direction was OPEN: it GRANTED what the policy is
482+
// set to withhold. Nothing pinned either direction before these tests, which
483+
// is why the literal survived.
484+
485+
it("invite-only: creates the account and binds NO membership (ADR-0093 D1)", async () => {
486+
const m = makeDepsWithOrgs({ orgs: [{ id: 'org_only' }] });
487+
m.deps.getMembershipPolicy = () => 'invite-only';
488+
const res = await runAdminCreateUser(
489+
m.deps,
490+
makeRequest({ email: 'a@b.co', generatePassword: true }),
491+
ACTOR,
492+
);
493+
494+
// The ACCOUNT is still created — `invite-only` withholds the membership,
495+
// not the login. That distinction is the whole ruling.
496+
expect(res.status).toBe(200);
497+
expect(m.createUser).toHaveBeenCalledTimes(1);
498+
499+
const data = res.body.data as any;
500+
expect(data.user.id).toBe('user-9');
501+
expect(data.membershipCreated).toBe(false);
502+
expect(data.organizationId).toBeUndefined();
503+
expect(callsOf(m.engineInsert).some((c) => c[0] === 'sys_member')).toBe(false);
504+
505+
// `policy-skip` is decided BEFORE any target-org resolution, so the org
506+
// lookup never runs. This separates "policy said no" from "no target org
507+
// was found" — two outcomes that would both show membershipCreated:false.
508+
expect(m.find.mock.calls.some((c) => c[0] === 'sys_organization')).toBe(false);
509+
510+
// The audit row records the refusal, so the trail shows the account was
511+
// created member-less on purpose rather than by a failed bind.
512+
const auditRow = callsOf(m.engineInsert).find((c) => c[0] === 'sys_audit_log')?.[1];
513+
expect(auditRow).toBeTruthy();
514+
const meta = JSON.parse(auditRow.metadata);
515+
expect(meta.membershipCreated).toBe(false);
516+
expect(meta.organizationId).toBeUndefined();
517+
});
518+
519+
it('invite-only in multi-org mode: also binds nothing (both reasons hold)', async () => {
520+
const m = makeDepsWithOrgs({
521+
orgs: [{ id: 'org_default', slug: 'default' }, { id: 'org_tenant_b' }],
522+
});
523+
m.deps.getTenancy = () => ({ defaultOrgId: async () => null });
524+
m.deps.getMembershipPolicy = () => 'invite-only';
525+
const res = await runAdminCreateUser(
526+
m.deps,
527+
makeRequest({ email: 'a@b.co', generatePassword: true }),
528+
ACTOR,
529+
);
530+
expect(res.status).toBe(200);
531+
const data = res.body.data as any;
532+
expect(data.membershipCreated).toBe(false);
533+
expect(data.organizationId).toBeUndefined();
534+
expect(callsOf(m.engineInsert).some((c) => c[0] === 'sys_member')).toBe(false);
535+
});
536+
537+
it('auto (explicit): still binds — the default posture is untouched', async () => {
538+
const m = makeDepsWithOrgs({ orgs: [{ id: 'org_only' }] });
539+
m.deps.getMembershipPolicy = () => 'auto';
540+
const res = await runAdminCreateUser(
541+
m.deps,
542+
makeRequest({ email: 'a@b.co', generatePassword: true }),
543+
ACTOR,
544+
);
545+
expect(res.status).toBe(200);
546+
const data = res.body.data as any;
547+
expect(data.organizationId).toBe('org_only');
548+
expect(data.membershipCreated).toBe(true);
549+
expect(callsOf(m.engineInsert).some((c) => c[0] === 'sys_member')).toBe(true);
550+
});
551+
552+
/**
553+
* NEGATIVE CONTROL (the ruling requires it, and it is not decorative).
554+
*
555+
* Under `auto` the behaviour must be BYTE-IDENTICAL to the pre-change
556+
* literal. A green `invite-only` assertion alone would also be green if the
557+
* endpoint had been changed to "never bind": that mistake passes the pin
558+
* above and silently breaks every default deployment. So compare the whole
559+
* observable surface — response body and every engine write — across the
560+
* three ways `auto` can be reached: an explicit `'auto'`, no policy dep at
561+
* all (unwired host: the accessor's own `?? 'auto'` default), and a dep that
562+
* is present but returns `undefined`.
563+
*
564+
* `sys_member.id` is a fresh random id per run, so it is normalized out;
565+
* everything else, including the audit metadata JSON, is compared verbatim.
566+
* The password is passed explicitly so no random temporary one is minted.
567+
*/
568+
async function autoPathSurface(
569+
policyDep: undefined | (() => any),
570+
): Promise<{ body: string; writes: string }> {
571+
const m = makeDepsWithOrgs({ orgs: [{ id: 'org_only' }] });
572+
if (policyDep) m.deps.getMembershipPolicy = policyDep as any;
573+
const res = await runAdminCreateUser(
574+
m.deps,
575+
makeRequest({ email: 'a@b.co', password: 'Sup3rSecret!x' }),
576+
ACTOR,
577+
);
578+
const writes = callsOf(m.engineInsert).map(([object, doc]) => [
579+
object,
580+
object === 'sys_member' ? { ...doc, id: '(generated)' } : doc,
581+
]);
582+
return { body: JSON.stringify(res.body), writes: JSON.stringify(writes) };
583+
}
584+
585+
it('auto: byte-identical across explicit / unwired / undefined policy deps', async () => {
586+
const explicitAuto = await autoPathSurface(() => 'auto');
587+
const unwired = await autoPathSurface(undefined);
588+
const returnsUndefined = await autoPathSurface(() => undefined);
589+
590+
expect(unwired).toEqual(explicitAuto);
591+
expect(returnsUndefined).toEqual(explicitAuto);
592+
593+
// And the shared surface is the BINDING one — a suite that agreed on
594+
// "nothing happened" three times would satisfy the equalities above.
595+
expect(explicitAuto.body).toContain('"membershipCreated":true');
596+
expect(explicitAuto.writes).toContain('sys_member');
597+
});
598+
463599
it('no-ops the bind (no throw) when the engine has no find surface', async () => {
464600
// Default makeDeps engine exposes only update/insert — the bind must be a
465601
// clean no-op, leaving exactly the audit insert.

packages/plugins/plugin-auth/src/admin-user-endpoints.ts

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,27 @@ export interface AdminUserEndpointDeps {
104104
* is registered.
105105
*/
106106
getTenancy?(): { defaultOrgId(): Promise<string | null> } | undefined;
107+
/**
108+
* ADR-0093 D1 — the deployment's membership policy, read LIVE through
109+
* `AuthManager.getMembershipPolicy()` (never a captured option).
110+
*
111+
* This endpoint used to hand the reconciler a literal `'auto'`, which made
112+
* it the one membership-writing path that did not consult the policy: an
113+
* `invite-only` deployment's `user.create.after` reconciler correctly bound
114+
* nobody, and then this endpoint-side belt-and-suspenders call bound them
115+
* anyway. The failure direction was OPEN — it GRANTED the membership the
116+
* policy exists to withhold, and answered `membershipCreated: true` while
117+
* doing it. ADR-0093 D1 enumerates the `invite-only` flows as a closed set
118+
* and refuses "which endpoint created the user" as a determinant, so the
119+
* endpoint reads the policy like every other consumer.
120+
*
121+
* Optional, and absent ⇒ `'auto'`: that is the accessor's OWN default
122+
* (`this.config.membershipPolicy ?? 'auto'`), so a host that wires no
123+
* policy dep — lean embeddings, the package's own mock-deps tests — keeps
124+
* byte-identical behaviour. The real mount in `auth-plugin.ts` always
125+
* wires it.
126+
*/
127+
getMembershipPolicy?(): MembershipPolicy;
107128
logger?: { warn(msg: string): void };
108129
}
109130

@@ -153,7 +174,7 @@ export interface EndpointResult {
153174

154175
import { CREDENTIAL_ISSUER } from './backfill-account-issuer.js';
155176
import { generatePlaceholderEmail } from './placeholder-email.js';
156-
import { reconcileMembership } from './reconcile-membership.js';
177+
import { reconcileMembership, type MembershipPolicy } from './reconcile-membership.js';
157178
import { resolveDefaultOrgId } from './tenancy-service.js';
158179

159180
const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] };
@@ -299,7 +320,8 @@ async function stampMustChangePassword(
299320
}
300321

301322
/**
302-
* Bind an admin-created user to the organization (single-org membership).
323+
* Bind an admin-created user to the organization (single-org membership),
324+
* SUBJECT TO the deployment's membership policy.
303325
*
304326
* ADR-0093 D2 — this now delegates to the shared membership reconciler, the
305327
* single owner of the "every new user gets a membership" invariant. The
@@ -311,10 +333,22 @@ async function stampMustChangePassword(
311333
* org is the single-org default (resolveDefaultOrgId); multi-org resolves to
312334
* none, so this no-ops there just as before.
313335
*
336+
* ADR-0093 D1 — the policy comes from {@link AdminUserEndpointDeps.getMembershipPolicy},
337+
* the same live accessor the sign-up and backfill seams read. It used to be a
338+
* literal `'auto'`, which made double-coverage anything but harmless under
339+
* `invite-only`: the hook skipped by policy and this call bound regardless, so
340+
* the endpoint GRANTED the membership the policy was set to withhold. D1
341+
* enumerates the `invite-only` flows as a closed set and refuses "which
342+
* endpoint created the user" as a determinant — an admin-created account is
343+
* not an implicit invitation.
344+
*
314345
* Returns the shape the response/audit consumed pre-ADR-0093:
315346
* `membershipCreated` is true only when THIS call inserted the row (a `bound`
316347
* outcome); a `yielded` outcome (the hook or a race already bound it) reports
317-
* the org with `membershipCreated: false`.
348+
* the org with `membershipCreated: false`. Under `invite-only` the reconciler
349+
* answers `policy-skip` before it looks at any org, so the caller reports no
350+
* organization and `membershipCreated: false` — the account is created, the
351+
* membership is not.
318352
*/
319353
async function bindUserToSoleOrganization(
320354
deps: AdminUserEndpointDeps,
@@ -327,8 +361,11 @@ async function bindUserToSoleOrganization(
327361
// in a multi-org deployment. Fallback (no tenancy wired: lean embeddings,
328362
// legacy mocks) keeps the single-org resolution.
329363
const tenancy = deps.getTenancy?.();
364+
// ADR-0093 D1 — read the live policy, never a literal. Absent dep ⇒ `auto`,
365+
// which is the accessor's own default, so unwired hosts are unchanged.
366+
const policy: MembershipPolicy = deps.getMembershipPolicy?.() ?? 'auto';
330367
const result = await reconcileMembership(engine, userId, {
331-
policy: 'auto',
368+
policy,
332369
resolveTargetOrg: () => (tenancy ? tenancy.defaultOrgId() : resolveDefaultOrgId(engine)),
333370
logger: deps.logger
334371
? { warn: (msg, meta) => deps.logger?.warn(`${msg} ${meta ? JSON.stringify(meta) : ''}`.trim()) }
@@ -535,7 +572,8 @@ export async function runAdminCreateUser(
535572

536573
// Match the invite / add-member flows: give the new user a membership so a
537574
// single-org deployment shows them under the Default Organization instead of
538-
// as a member-less account. No-op in multi-org (≥2 orgs) — see the helper.
575+
// as a member-less account. No-op in multi-org (≥2 orgs), and no-op under
576+
// `membershipPolicy: 'invite-only'` (ADR-0093 D1) — see the helper.
539577
const membership = await bindUserToSoleOrganization(deps, userId);
540578

541579
await writeAdminAudit(deps, {

packages/plugins/plugin-auth/src/auth-manager.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3996,10 +3996,19 @@ export class AuthManager {
39963996
/**
39973997
* ADR-0093 D1 — the deployment's membership policy **as it stands right now**.
39983998
*
3999-
* The ONE source both membership paths read (#5152):
3999+
* The ONE source EVERY membership-writing path reads (#5152):
40004000
* - sign-up: the reconciler composed into `user.create.after` (below);
40014001
* - backfill: `AuthPlugin`'s ADR-0093 D6 pass over pre-existing member-less
4002-
* users, which used to read the plugin's CONSTRUCTOR options instead.
4002+
* users, which used to read the plugin's CONSTRUCTOR options instead;
4003+
* - `POST /admin/create-user`: the endpoint-side belt-and-suspenders bind
4004+
* in `admin-user-endpoints.ts`, reached through
4005+
* `AdminUserEndpointDeps.getMembershipPolicy`.
4006+
*
4007+
* That list was written as "both membership paths" while the admin endpoint
4008+
* handed the reconciler a literal `'auto'` — a third writer, outside the
4009+
* accounting, binding under `invite-only` where the other two correctly did
4010+
* not. Enumerate every writer here: a path that is not in this list is a
4011+
* path that can disagree with the deployment's policy.
40034012
*
40044013
* That split mattered because `this.config` is what {@link applyConfigPatch}
40054014
* targets: once `auth.membership_policy` became a platform setting, the

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2410,6 +2410,12 @@ export class AuthPlugin implements Plugin {
24102410
// target org (never grab the bootstrap default org in a multi-tenant
24112411
// deployment); single-org resolves the default org.
24122412
getTenancy: () => this.tenancy ?? undefined,
2413+
// ADR-0093 D1 — the LIVE membership policy, read through the accessor
2414+
// per call (the deps factory itself runs per request). A captured
2415+
// value would keep the endpoint auto-binding after an admin switched
2416+
// the deployment to `invite-only`, which is the exact defect the
2417+
// accessor exists to prevent.
2418+
getMembershipPolicy: () => this.authManager!.getMembershipPolicy(),
24132419
logger: ctx.logger,
24142420
});
24152421
// Gate: the shared `gateAdmin` hoisted above the SSO mounts (#9653).

0 commit comments

Comments
 (0)