Repository navigation
fix(plugin-auth)!: POST /admin/create-user reads the deployment membership policy instead of hard-coding auto (#16683) - #17443
Conversation
…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
📓 Docs Drift CheckThis PR changes 1 package(s): 3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 13 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin b65447a1a412c372a7579c32a0d6d90179997141 && git checkout b65447a1a412c372a7579c32a0d6d90179997141
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin ab56ea3a1ea6fde6161781c93a013fd494d39193 7c54bcbfcbd64d42bc47b512d35c133dce6cb631 && git checkout -B drift-repro ab56ea3a1ea6fde6161781c93a013fd494d39193 && git merge --no-ff 7c54bcbfcbd64d42bc47b512d35c133dce6cb631
node scripts/docs-audit/affected-docs.mjs --json ab56ea3a1ea6fde6161781c93a013fd494d39193
|
…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
Fixes #16683
Implements the director-seat ruling of decision batch #86 (option A,
#issuecomment-5581954386):POST /api/v1/auth/admin/create-userreads the deployment's membership policy instead of hard-codingauto.Source-app reading that started this: steedos-labs/ats#38 — an app that declared
membershipPolicy: 'invite-only'at author time and got admin-created accounts bound to the Default Organization anyway.What moved
bindUserToSoleOrganization()handed the shared reconciler a literalpolicy: 'auto', which made this endpoint the one membership-writing path in the product that never consultedAuthManager.getMembershipPolicy(). On aninvite-onlydeployment theuser.create.afterreconciler correctly declined to bind, and this endpoint-side belt-and-suspenders bind then bound the user anyway, answeringmembershipCreated: true. The failure direction was OPEN: it granted the membership the policy exists to withhold.The endpoint is a pure
runXxx(deps, request)handler with nothis, so the accessor arrives as a dep:admin-user-endpoints.ts— new optionalAdminUserEndpointDeps.getMembershipPolicy?(): MembershipPolicy; the bind readsdeps.getMembershipPolicy?.() ?? 'auto'. The fallback is the accessor's own default (this.config.membershipPolicy ?? 'auto'), so a host that wires no policy dep is byte-identical.auth-plugin.ts— the mount's deps factory wiresgetMembershipPolicy: () => this.authManager!.getMembershipPolicy(). The factory runs per request, so the read is live; a captured value would reproduce the exact defect the accessor exists to prevent.auth-manager.ts— acceptance item 1. The accessor's docblock enumerated "The ONE source both membership paths read" while a third writer sat outside the accounting; it now names all three (sign-up, D6 backfill, admin create-user) and says why the list has to be exhaustive. Docblock only, new-side lines 3996-4015.Under
invite-onlythe reconciler answerspolicy-skipbefore it looks at any organization, so the response omitsorganizationIdand answersmembershipCreated: false, and the audit row records the same. The account is still created and can sign in —invite-onlywithholds the membership, not the login.Verification
All readings below are on head
7c54bcbfcwith a clean working tree.Package suite + typecheck —
pnpm --filter @objectstack/plugin-auth typecheck && pnpm --filter @objectstack/plugin-auth test,VERDICT command-exit 0:Test Files 106 passed (106)/Tests 2249 passed (2249), andcheck:test-typecheckclean.Repo-wide lint —
pnpm exec eslint . --no-inline-config --format json, exit 0: 6557 files linted, 0 errors, 0 warnings. Population read from eslint's own run rather than estimated, so this is the full union and not a narrowed sample.Derived gates —
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --ran <record>on this head: 63 derived, 61 run green, 2 NOT MEASURED, 0 UNRUN. Every exit code was captured before any pipe. The two not-measured arepnpm check:dual-build-cjs-loadsandpnpm check:type-check-debt, both exit 3 = PREREQUISITE NOT MET (they read built output for the whole workspace and demand a fullpnpm buildfirst). ⛔ Recorded as not measured, not as passes; CI builds the tree and runs them for real. Four artifact-roster families whose roster directory contains one of these paths were also run out of caution —check-changeset-fixed,check:authz-resolver,check:error-code-casing,check:filter-alias-parity— all exit 0.New pins in
admin-user-endpoints.test.ts:invite-onlybinds nothing (single-org): 200, account created, nosys_memberinsert,membershipCreated: false, noorganizationId, and the audit metadata agrees.invite-only(find('sys_organization')uncalled) — this keepspolicy-skipdistinguishable fromno-target-org, two outcomes that both surface asmembershipCreated: false.invite-onlyin multi-org mode: also binds nothing.auto(explicit) still binds.autopath is compared byte-for-byte — full response body plus every engine write, with only the randomsys_member.idnormalized — across an explicit'auto', no policy dep at all, and a dep returningundefined. It also asserts the shared surface is the BINDING one, so "all three agree that nothing happened" cannot pass. A greeninvite-onlyassertion alone would also be green if the endpoint had been changed to never bind, which would silently break every default deployment.Ablation — the
invite-onlypin is proven able to failThe pin is a negative assertion, so it is worthless until it has been shown to go red. Run from the committed state, with a restoring
trapon absolute paths:deps.getMembershipPolicy?.() ?? 'auto'went 1 → 0 andpolicy: 'auto',went 0 → 1, and the blob hash moved67e14b5d→f3a65b74.VERDICT command-exit 1:invite-onlytest went red; the multi-org one stayed green under the mutation, because multi-org resolves no target org andno-target-orgproduces the samemembershipCreated: false. That confound is exactly why pin 2 exists, and it is reported rather than papered over.git checkout HEAD -- <path>(pointing atHEAD, not a bare checkout that would re-read the poisoned index), then on-diskgit hash-object== the HEAD blob67e14b5d30f5e17f90c4a926ce95cfcae7f5a095ANDgit diff HEAD --name-onlyempty.No dist leg is owed here: the test imports
./admin-user-endpoints.jsrelatively inside its own package, so the subject resolves to TypeScript source and never throughdist/.Acceptance notes
Carried from the triage seat's acceptance list (
#issuecomment-5578477253) as amended by the ruling:auth-manager.ts:3999(the ruling names:3879, the triage comment:3865; both are stale, the text moved). Located by text, not by line.admin-user-endpoints.test.tsgains theinvite-onlyassertion — item 1 above, in the refusing direction the ruling chose.auto, which DESIGN.md §03 forbids steedos-labs/ats#38, linked above by issue number rather than paraphrased./admin/import-usersparity — CHECKED, unchangedThe ruling asks for
/admin/import-usersto be checked for parity. It is correct as it stands and this PR does not touch it.admin-import-users.tswrites no membership at all: it has noreconcileMembershipcall and nosys_memberinsert. Thepolicyidentifier in that file isImportPasswordPolicy(auto/none/invite/temporary), an unrelated per-row PASSWORD policy, not the deployment membership policy. Imported users get their membership from the shareduser.create.afterreconciler, which already reads the accessor. So the import path was never the third writer, and after this PR the accounting is closed.The published
invite-onlyenumerations were read — reading (a), nothing falsifiedcontent/docs/permissions/authentication.mdx:126lists 「an admin adding them」 among the explicit acts that DO grant membership underinvite-only. Read against this change, that sentence still holds, and the disambiguator is a named identifier rather than a judgement call:content/docs/deployment/tenancy-modes.mdx:185spells the same slot in the same closed set asadd-member— the endpoint, not prose.add-memberisPOST /api/v1/auth/organization/add-member, documented on that very sameauthentication.mdxpage: it takes an existinguserIdand attaches it to an organization. That is an admin "adding them";POST /admin/create-usermints a login account, which is a different act.⇒ Nothing in the three cited pages is falsified by this change, and no page is edited here. This is the class the drift tool states it can never detect (a page that states a rule by its inputs shares no identifier with the emitter), so it is written down here rather than left to be rediscovered.
One page is now incomplete, not wrong, and is named for a follow-up rather than fixed here (
content/docs/**isdomain:devx, not this lane):content/docs/deployment/environment-variables.mdx:84says the policy "Applies to sign-up and to the backfill of pre-existing member-less users." After this change it also applies toPOST /admin/create-user. That is the documentation mirror of theauth-manager.tsdocblock corrected above — an enumeration short by one member.The docs-drift advisory's three
AuthManager-anchored pages were checked and are noise, as suspected:kernel/contracts/auth-service.mdxandkernel/services-checklist.mdxcontain no mention of membership policy,create-userormembershipCreatedat all, andpermissions/authentication.mdxmentions admin create-user only at:366, about phone-only placeholder emails, which this diff does not touch.Noted, not filed
invite-only,reconcileMembershipdecidespolicy-skipbefore the "yield to any existing membership" branch. So if a host hook (e.g. a cloud personal-org provisioner) had already bound the user, this endpoint reportsorganizationId: undefined/membershipCreated: falseeven though a membership exists — where underautoit would reportyieldedwith the org. That is the reconciler's own long-standing decision order, shared with the sign-up seam, and is unchanged by this PR. Not a defect of this diff, and the response field is honest about what THIS call did. No PR or person is heading for this file, so: successor — none.Response-surface reading (
Clause-②)The card declares
Clause-②: yeson the grounds that the response gains amembershipCreatedfield. Measured:membershipCreatedis already in the response body and in the audit metadata onorigin/main— this diff adds no field and widens no accept set. What moves is the VALUE that field carries underinvite-only, plus a new OPTIONAL dep on the exportedAdminUserEndpointDepsinterface (additive; nothing that type-checks today stops doing so).needs:contract-reviewis hung as instructed and the declaration is left exactly as the PM set it — this paragraph is the reading, not a correction.Generated by Claude Code