Skip to content

fix(plugin-auth)!: POST /admin/create-user reads the deployment membership policy instead of hard-coding auto (#16683) - #17443

Merged
os-sales merged 2 commits into
mainfrom
claude/issue-16683-create-user-membership-policy
Sep 10, 2026
Merged

os-sales merged 2 commits into
mainfrom
claude/issue-16683-create-user-membership-policy

Conversation

@os-sales

@os-sales os-sales commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #16683

Implements the director-seat ruling of decision batch #86 (option A, #issuecomment-5581954386): POST /api/v1/auth/admin/create-user reads the deployment's membership policy instead of hard-coding auto.

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 literal policy: 'auto', which made this endpoint the one membership-writing path in the product that never consulted AuthManager.getMembershipPolicy(). On an invite-only deployment the user.create.after reconciler correctly declined to bind, and this endpoint-side belt-and-suspenders bind then bound the user anyway, answering membershipCreated: 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 no this, so the accessor arrives as a dep:

  • admin-user-endpoints.ts — new optional AdminUserEndpointDeps.getMembershipPolicy?(): MembershipPolicy; the bind reads deps.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 wires getMembershipPolicy: () => 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-only the reconciler answers policy-skip before it looks at any organization, so the response omits organizationId and answers membershipCreated: false, and the audit row records the same. The account is still created and can sign in — invite-only withholds the membership, not the login.

Verification

All readings below are on head 7c54bcbfc with 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), and check:test-typecheck clean.

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 are pnpm check:dual-build-cjs-loads and pnpm check:type-check-debt, both exit 3 = PREREQUISITE NOT MET (they read built output for the whole workspace and demand a full pnpm build first). ⛔ 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:

  1. invite-only binds nothing (single-org): 200, account created, no sys_member insert, membershipCreated: false, no organizationId, and the audit metadata agrees.
  2. The org lookup is never reached under invite-only (find('sys_organization') uncalled) — this keeps policy-skip distinguishable from no-target-org, two outcomes that both surface as membershipCreated: false.
  3. invite-only in multi-org mode: also binds nothing.
  4. auto (explicit) still binds.
  5. Negative control (ruling-mandated). The auto path is compared byte-for-byte — full response body plus every engine write, with only the random sys_member.id normalized — across an explicit 'auto', no policy dep at all, and a dep returning undefined. It also asserts the shared surface is the BINDING one, so "all three agree that nothing happened" cannot pass. A green invite-only assertion alone would also be green if the endpoint had been changed to never bind, which would silently break every default deployment.

Ablation — the invite-only pin is proven able to fail

The 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 trap on absolute paths:

  • Mutation reached disk — proven by anchored counts, not by an editor's exit code: deps.getMembershipPolicy?.() ?? 'auto' went 1 → 0 and policy: 'auto', went 0 → 1, and the blob hash moved 67e14b5d → f3a65b74.
  • RED leg — VERDICT command-exit 1:
× invite-only: creates the account and binds NO membership (ADR-0093 D1)
AssertionError: expected true to be false // Object.is equality
- Expected  false
+ Received  true
  src/admin-user-endpoints.test.ts:501  expect(data.membershipCreated).toBe(false);
  • Honest reading of the other legs. Only the single-org invite-only test went red; the multi-org one stayed green under the mutation, because multi-org resolves no target org and no-target-org produces the same membershipCreated: false. That confound is exactly why pin 2 exists, and it is reported rather than papered over.
  • Restore proven BY STATE, never by exit code: git checkout HEAD -- <path> (pointing at HEAD, not a bare checkout that would re-read the poisoned index), then on-disk git hash-object == the HEAD blob 67e14b5d30f5e17f90c4a926ce95cfcae7f5a095 AND git diff HEAD --name-only empty.

No dist leg is owed here: the test imports ./admin-user-endpoints.js relatively inside its own package, so the subject resolves to TypeScript source and never through dist/.

Acceptance notes

Carried from the triage seat's acceptance list (#issuecomment-5578477253) as amended by the ruling:

  1. The universal statement is made true — at auth-manager.ts:3999 (the ruling names :3879, the triage comment :3865; both are stale, the text moved). Located by text, not by line.
  2. admin-user-endpoints.test.ts gains the invite-only assertion — item 1 above, in the refusing direction the ruling chose.
  3. Negative control measured — item 5 above.
  4. Multi-org covered — item 3 above, on top of the pre-existing multi-org tests.
  5. Source-app reading back-linked — Membership backfill binds all 80 job seekers and both platform staff to the Default Organization: the auth membership policy is left at auto, which DESIGN.md §03 forbids steedos-labs/ats#38, linked above by issue number rather than paraphrased.

/admin/import-users parity — CHECKED, unchanged

The ruling asks for /admin/import-users to be checked for parity. It is correct as it stands and this PR does not touch it. admin-import-users.ts writes no membership at all: it has no reconcileMembership call and no sys_member insert. The policy identifier in that file is ImportPasswordPolicy (auto / none / invite / temporary), an unrelated per-row PASSWORD policy, not the deployment membership policy. Imported users get their membership from the shared user.create.after reconciler, 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-only enumerations were read — reading (a), nothing falsified

content/docs/permissions/authentication.mdx:126 lists 「an admin adding them」 among the explicit acts that DO grant membership under invite-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:185 spells the same slot in the same closed set as add-member — the endpoint, not prose.
  • add-member is POST /api/v1/auth/organization/add-member, documented on that very same authentication.mdx page: it takes an existing userId and attaches it to an organization. That is an admin "adding them"; POST /admin/create-user mints a login account, which is a different act.
  • ADR-0093 D1 independently refuses "which endpoint created the user" as a determinant, so a create endpoint could not join a set of membership-granting acts merely by being a create endpoint.

⇒ 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/** is domain:devx, not this lane): content/docs/deployment/environment-variables.mdx:84 says the policy "Applies to sign-up and to the backfill of pre-existing member-less users." After this change it also applies to POST /admin/create-user. That is the documentation mirror of the auth-manager.ts docblock 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.mdx and kernel/services-checklist.mdx contain no mention of membership policy, create-user or membershipCreated at all, and permissions/authentication.mdx mentions admin create-user only at :366, about phone-only placeholder emails, which this diff does not touch.

Noted, not filed

  • Under invite-only, reconcileMembership decides policy-skip before 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 reports organizationId: undefined / membershipCreated: false even though a membership exists — where under auto it would report yielded with 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.
  • ADR-0131:570's tension with this area is explicitly out of scope per the ruling ("noted, not resolved here"). Recorded, not acted on.

Response-surface reading (Clause-②)

The card declares Clause-②: yes on the grounds that the response gains a membershipCreated field. Measured: membershipCreated is already in the response body and in the audit metadata on origin/main — this diff adds no field and widens no accept set. What moves is the VALUE that field carries under invite-only, plus a new OPTIONAL dep on the exported AdminUserEndpointDeps interface (additive; nothing that type-checks today stops doing so). needs:contract-review is 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

…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
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 10, 2026
@os-sales os-sales added needs:contract-review and removed documentation Improvements or additions to documentation tests tooling labels Sep 10, 2026 — with Claude
@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-auth, touching 6 documentable anchor(s).

3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/kernel/contracts/auth-service.mdx (via AuthManager (symbol, a top-level class))
  • content/docs/kernel/services-checklist.mdx (via AuthManager (symbol, a top-level class))
  • content/docs/permissions/authentication.mdx (via AuthManager (symbol, a top-level class))
What this run could not see
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 13 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json ab56ea3a1ea6fde6161781c93a013fd494d39193 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from b65447a1a412c372a7579c32a0d6d90179997141 — the merge of head 7c54bcbfcbd64d42bc47b512d35c133dce6cb631 into base ab56ea3a1ea6fde6161781c93a013fd494d39193, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# 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

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs ab56ea3a1ea6fde6161781c93a013fd494d39193 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

…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
@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 10, 2026
@os-sales
os-sales marked this pull request as ready for review September 10, 2026 15:32
@os-sales
os-sales enabled auto-merge September 10, 2026 15:33
@os-sales
os-sales added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit 344d475 Sep 10, 2026
44 checks passed
@os-sales
os-sales deleted the claude/issue-16683-create-user-membership-policy branch September 10, 2026 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants