diff --git a/.changeset/first-session-membership-ordering.md b/.changeset/first-session-membership-ordering.md new file mode 100644 index 0000000000..ddc96ded1c --- /dev/null +++ b/.changeset/first-session-membership-ordering.md @@ -0,0 +1,48 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(auth): a user's first session no longer predates their membership, so its audit rows carry a tenant (#8245, #8247) + +`session.create.before` resolves a session's `activeOrganizationId` from the +caller's `sys_member` row. The ADR-0093 D2 reconciler that **writes** that row is +composed into `user.create.after`, and better-auth defers it past the sign-up +transaction — so the session sign-up mints ran first, found no membership, and +carried no active organization. Structurally, for every new user, on every +deployment. + +That first session was not a harmless intermediate. Its `login` audit row takes +its tenant from `session.activeOrganizationId`, so the row landed with a NULL +tenant and the SecurityPlugin's RLS predicate (`organization_id = +current_user.organization_id`) hid it from every reader **permanently** — +nothing back-fills a written ledger row, and the rows lost this way are exactly +the ones describing account creation. + +The membership now settles at the seam that needs it: when the active-org lookup +finds nothing, the reconciler runs and the lookup is repeated, so the first +session mints *with* its organization. + +**This changes ordering, not policy.** It calls the same reconciler with the same +membership policy and the same target-organization resolution that +`user.create.after` uses — both now share one assembly point on the manager — so +the outcome is exactly what would have happened a moment later: + +- `invite-only` binds nobody, and those sessions still mint with no active + organization; +- a multi-organization deployment resolves no unambiguous target and binds + nobody, unchanged; +- a user who already holds a membership never reaches the new branch, and no + second membership is ever written; +- owner-preference in the active-org selection is unchanged, because the + selection is one function called on both sides of the settle. + +Cost is paid only where there is something to fix. A deployment that binds nobody +stops at the reconciler's own policy check without touching the store, and the +repeat lookup is gated on an outcome meaning a membership now exists — so an +ordinary sign-in issues no extra query. + +Unchanged: `user.create.after` still reconciles (the creation paths that mint no +session at all — admin create-user, bulk import, SSO JIT — are untouched), the +host `session.create.before` hook still chains first and still wins, +`autoActiveOrganization: false` still opts out entirely, and a failing engine +still never breaks session creation. diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index b93934449b..cfa1b0b67c 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -48,7 +48,11 @@ import { removalBlockedByOwnerTarget, } from './remove-member-permission-guard.js'; import { isPlaceholderEmail } from './placeholder-email.js'; -import { reconcileMembership, type MembershipPolicy } from './reconcile-membership.js'; +import { + reconcileMembership, + type MembershipPolicy, + type ReconcileOutcome, +} from './reconcile-membership.js'; import type { TenancyService } from './tenancy-service.js'; import { OtpSendGuard, assertOtpCooldownSeconds } from './otp-send-guard.js'; import type { CounterStore } from './rate-limit-storage.js'; @@ -3019,6 +3023,58 @@ export class AuthManager { return this.config.membershipPolicy ?? 'auto'; } + /** + * [ADR-0093 D2] Run the membership reconciler for one user — the ONE place + * this manager assembles its inputs. + * + * Two seams call it, and the whole point is that they cannot disagree: + * + * - `user.create.after`, the creation seam every path flows through (email + * signup, admin create-user, bulk import, SSO JIT); + * - `session.create.before`, which settles the membership before resolving + * the session's active organization so a user's FIRST session is not + * minted tenant-less (#8247 rule 2 / #8245). + * + * Assembling the deps at each call site instead would let the two drift on + * the axis that matters most: the POLICY. `getMembershipPolicy()` reads a + * live platform setting (#5152) — a captured constructor option would keep + * one seam auto-binding after an admin switched the deployment to + * `invite-only`, which is the exact defect that made the accessor exist. The + * target-org resolution is shared for the same reason: "which organization" + * must never be answered two ways. + * + * Never throws — `reconcileMembership` already guarantees that, and the guard + * stands anyway because both callers are hooks where a bookkeeping failure + * must not fail user creation or sign-in. The OUTCOME is returned (rather + * than swallowed) so the session seam can tell "a membership now exists" from + * "policy says there will never be one" and skip a pointless re-read; + * `undefined` means the reconciler could not be consulted at all. + */ + private async settleMembership(userId: unknown): Promise { + try { + const result = await reconcileMembership( + this.config.dataEngine, + typeof userId === 'string' && userId ? userId : undefined, + { + // #5152 — read through the accessor, not `this.config` directly: it is + // the single source the backfill path reads too. + policy: this.getMembershipPolicy(), + resolveTargetOrg: async () => { + const tenancy = this.config.getTenancy?.(); + // Single-org → default org; multi-org → none (invite/JIT own it). + return tenancy ? await tenancy.defaultOrgId() : null; + }, + logger: this.config.logger, + }, + ); + return result.outcome; + } catch { + // reconcileMembership never throws, but guard regardless — membership + // bookkeeping must never break user creation or session creation. + return undefined; + } + } + /** * Inject (or replace) the outbound email service used by better-auth * callbacks. Safe to call after construction but BEFORE the first @@ -4370,6 +4426,30 @@ export class AuthManager { // never fails on this bookkeeping. Opt out via `autoActiveOrganization: // false`. const hostSessionBefore = (host as any)?.session?.create?.before; + + /** + * The membership → active-org selection, in ONE place: owner-preferred, + * else the oldest row. It is called twice below and both calls must select + * identically — a second, "simpler" lookup after the settle would silently + * make a freshly-bound user's active org depend on which path found it. + */ + const selectActiveOrg = async (reader: any, userId: string): Promise => { + let row: any; + try { + row = await reader.findOne('sys_member', { where: { user_id: userId, role: 'owner' } }); + } catch { + row = undefined; + } + if (!row?.organization_id) { + try { + row = await reader.findOne('sys_member', { where: { user_id: userId } }); + } catch { + row = undefined; + } + } + return row?.organization_id; + }; + const defaultActiveOrg = async (session: any) => { try { if (!session || session.activeOrganizationId) return; @@ -4380,22 +4460,51 @@ export class AuthManager { // sys_member is org/user-scoped in host stacks — read with the system // context so the pre-session lookup (no org on the caller yet) works. const reader = withSystemReadContext(engine); - let row: any; - try { - row = await reader.findOne('sys_member', { - where: { user_id: userId, role: 'owner' }, - }); - } catch { - row = undefined; - } - if (!row?.organization_id) { - try { - row = await reader.findOne('sys_member', { where: { user_id: userId } }); - } catch { - row = undefined; + let orgId = await selectActiveOrg(reader, userId); + + // [#8247 rule 2 / #8245] SETTLE THE MEMBERSHIP, THEN LOOK AGAIN. + // + // The ADR-0093 D2 reconciler is composed into `user.create.after`, and + // better-auth DEFERS that past the sign-up transaction. This hook runs + // inside it. So a user's very FIRST session is minted BEFORE the + // reconciler has bound them to anything, the lookup above finds no + // `sys_member` row, and the session carries no active organization — + // for every new user, on every deployment, structurally. + // + // That first session is not a harmless intermediate. Everything it + // writes is tenant-less: its `login` audit row is derived from + // `session.activeOrganizationId` (`auth-session-audit.ts`), so it lands + // with a NULL tenant and the SecurityPlugin's RLS predicate hides it + // from every reader FOREVER — nothing back-fills a written ledger row, + // and the rows lost this way are exactly the ones describing account + // creation (#8245). + // + // So the settle is hoisted HERE, to the seam that actually needs it, + // rather than the ordering being left to better-auth's hook scheduling. + // + // ⛔ THIS DOES NOT WIDEN WHO GETS BOUND, and that is the property to + // preserve if this is ever touched: it calls the SAME reconciler with + // the SAME policy and the SAME target-org resolution that + // `user.create.after` uses (one owner — `settleMembership`), so the + // outcome is byte-for-byte what would have happened a moment later. + // `invite-only` still binds nobody; multi-org still resolves no + // unambiguous target and binds nobody. Those users keep minting + // sessions with no active organization, which is the LEGAL state the + // #8247 ruling declares — this removes a race, never a policy. + // + // Cost is paid only where there is something to fix: a caller who + // already holds a membership never reaches this branch, and a + // deployment that binds nobody stops at the reconciler's own policy / + // target-org check without touching the store. The re-read is gated on + // an outcome that means a membership now EXISTS, so the common + // no-bind login costs no extra query at all. + if (!orgId) { + const outcome = await this.settleMembership(userId); + if (outcome === 'bound' || outcome === 'yielded') { + orgId = await selectActiveOrg(reader, userId); } } - const orgId = row?.organization_id; + if (!orgId) return; return { data: { ...session, activeOrganizationId: orgId } }; } catch { @@ -4455,22 +4564,7 @@ export class AuthManager { // double bind. Best-effort — never fails user creation. const hostUserAfter = (host as any)?.user?.create?.after; const membershipReconciler = async (user: any) => { - try { - await reconcileMembership(this.config.dataEngine, user?.id, { - // #5152 — read through the accessor, not `this.config` directly: it is - // the single source the backfill path reads too. - policy: this.getMembershipPolicy(), - resolveTargetOrg: async () => { - const tenancy = this.config.getTenancy?.(); - // Single-org → default org; multi-org → none (invite/JIT own it). - return tenancy ? await tenancy.defaultOrgId() : null; - }, - logger: this.config.logger, - }); - } catch { - // reconcileMembership never throws, but guard the hook regardless — - // membership bookkeeping must never break user creation. - } + await this.settleMembership(user?.id); }; const userAfter = hostUserAfter ? async (user: any, ctx: any) => { diff --git a/packages/plugins/plugin-auth/src/first-session-membership-ordering.test.ts b/packages/plugins/plugin-auth/src/first-session-membership-ordering.test.ts new file mode 100644 index 0000000000..9af52775c0 --- /dev/null +++ b/packages/plugins/plugin-auth/src/first-session-membership-ordering.test.ts @@ -0,0 +1,267 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8247 rule 2 / #8245] A user's FIRST session must not be minted before their + * membership settles. + * + * ## What was open + * + * `session.create.before` resolves a session's `activeOrganizationId` from the + * caller's `sys_member` row. The ADR-0093 D2 reconciler that WRITES that row is + * composed into `user.create.after`, and better-auth defers it past the sign-up + * transaction. So the session sign-up mints runs first, finds no membership, and + * carries no active organization — for every new user, on every deployment. + * + * That first session is not a harmless intermediate. Its `login` audit row takes + * its tenant from `session.activeOrganizationId` (`auth-session-audit.ts`), so + * the row lands with a NULL tenant and the SecurityPlugin's RLS predicate hides + * it from every reader forever. Nothing back-fills a written ledger row, and the + * rows lost this way are exactly the ones describing account creation. + * + * ## Anti-vacuity + * + * The fix is an ORDERING change, so the trap is a test that would pass on the + * broken build because it never establishes that the membership was absent when + * the session was minted. Every case below therefore starts from a store with + * **no `sys_member` row for the user** and asserts on the FIRST session — the + * one that used to be tenant-less. `PRECONDITION` cases pin that starting state + * rather than assuming it. + * + * The second trap is the opposite over-reach: an ordering fix must not become a + * POLICY change. The `invite-only` and no-target-org cases are not decoration — + * they are the property that makes this safe to land, and they assert on the + * store (`insert` never called), not merely on the returned draft. A user whom + * policy says must not be bound still mints a session with no active + * organization, which is the legal state the #8247 ruling declares. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { AuthManager } from './auth-manager.js'; +import { loginEventFor } from './auth-session-audit.js'; + +const USER = 'u_first_session'; +const DEFAULT_ORG = 'org_default'; + +interface MemberRow { + id: string; + organization_id: string; + user_id: string; + role?: string; +} + +/** + * A minimal ObjectQL double over an in-memory `sys_member` table. + * + * `find` / `findOne` / `insert` only — the three verbs this path uses. It + * deliberately declares no `update` / `delete`: there is no dispatch contract + * for a verb the double does not offer, and adding stubs would put a looser + * copy of a write verb in front of the real one. + */ +function makeEngine(seed: MemberRow[] = []) { + const rows: MemberRow[] = [...seed]; + const match = (r: MemberRow, where: Record = {}) => + (where.user_id === undefined || r.user_id === where.user_id) && + (where.role === undefined || r.role === where.role) && + (where.organization_id === undefined || r.organization_id === where.organization_id); + const insert = vi.fn(async (model: string, data: MemberRow) => { + if (model !== 'sys_member') return data; + rows.push(data); + return data; + }); + return { + rows, + insert, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + find: vi.fn(async (model: string, q: any) => + model === 'sys_member' ? rows.filter((r) => match(r, q?.where)).slice(0, q?.limit ?? 100) : [], + ), + // eslint-disable-next-line @typescript-eslint/no-explicit-any + findOne: vi.fn(async (model: string, q: any) => + model === 'sys_member' ? (rows.find((r) => match(r, q?.where)) ?? null) : null, + ), + }; +} + +interface ManagerOpts { + engine: ReturnType; + /** `undefined` → the `auto` default. */ + membershipPolicy?: 'auto' | 'invite-only'; + /** What `tenancy.defaultOrgId()` answers. `null` = no unambiguous target. */ + defaultOrgId?: string | null; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + databaseHooks?: any; +} + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function hooksFor(opts: ManagerOpts): any { + const manager = new AuthManager({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: 'http://localhost:3000', + dataEngine: opts.engine, + ...(opts.membershipPolicy ? { membershipPolicy: opts.membershipPolicy } : {}), + getTenancy: () => ({ defaultOrgId: async () => opts.defaultOrgId ?? null }), + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (manager as any).composeDatabaseHooks(opts.databaseHooks); +} + +describe("[#8247 rule 2] a user's FIRST session settles the membership before resolving its active org", () => { + it('PRECONDITION: the store really does hold no membership for the user before sign-in', async () => { + // Without this the whole file could be measuring a user who was already + // bound, which is the pre-existing (and always-worked) path. + const engine = makeEngine(); + expect(engine.rows).toHaveLength(0); + const hooks = hooksFor({ engine, defaultOrgId: DEFAULT_ORG }); + expect(typeof hooks.session.create.before).toBe('function'); + }); + + it('the FIRST session carries the active organization — the reconciler is run, then re-read', async () => { + const engine = makeEngine(); + const hooks = hooksFor({ engine, defaultOrgId: DEFAULT_ORG }); + const result = await hooks.session.create.before({ userId: USER }); + expect(result?.data?.activeOrganizationId).toBe(DEFAULT_ORG); + // …and it is a real membership, not a value invented for the draft. The + // difference matters: a stamped-but-unbacked org would satisfy the session + // and still leave the user missing from the Members list and outside every + // RLS predicate that reads `sys_member`. + expect(engine.rows).toHaveLength(1); + expect(engine.rows[0]).toMatchObject({ organization_id: DEFAULT_ORG, user_id: USER }); + }); + + it("the login audit event for that first session carries the tenant — #8245's chain, end to end", async () => { + // The consequence the card is actually about. `loginEventFor` reads + // `session.activeOrganizationId`; with the ordering fixed there is one to + // read, so the ledger row is written INTO a tenant instead of into NULL + // where no RLS reader could ever see it again. + const engine = makeEngine(); + const hooks = hooksFor({ engine, defaultOrgId: DEFAULT_ORG }); + const result = await hooks.session.create.before({ userId: USER, id: 'sess_1' }); + const event = loginEventFor(result?.data); + expect(event?.organizationId).toBe(DEFAULT_ORG); + expect(event?.userId).toBe(USER); + }); + + it('an already-bound user is unchanged, and NO second membership is written', async () => { + const engine = makeEngine([{ id: 'm1', organization_id: 'org_existing', user_id: USER, role: 'member' }]); + const hooks = hooksFor({ engine, defaultOrgId: DEFAULT_ORG }); + const result = await hooks.session.create.before({ userId: USER }); + expect(result?.data?.activeOrganizationId).toBe('org_existing'); + expect(engine.insert).not.toHaveBeenCalled(); + expect(engine.rows).toHaveLength(1); + }); + + it('owner-preference survives the settle path (the selection is one function, called twice)', async () => { + // A bound user with two memberships takes the OWNER one. This is the pin + // that goes red if the post-settle re-read is ever replaced by a "simpler" + // lookup: the two calls must select identically, or a freshly-bound user's + // active org would depend on which path found it. + const engine = makeEngine([ + { id: 'm1', organization_id: 'org_member', user_id: USER, role: 'member' }, + { id: 'm2', organization_id: 'org_owner', user_id: USER, role: 'owner' }, + ]); + const hooks = hooksFor({ engine, defaultOrgId: DEFAULT_ORG }); + const result = await hooks.session.create.before({ userId: USER }); + expect(result?.data?.activeOrganizationId).toBe('org_owner'); + }); + + describe('⛔ the ordering fix removes a RACE, never a POLICY', () => { + it('`invite-only`: nothing is bound, and the session is legitimately org-less', async () => { + const engine = makeEngine(); + const hooks = hooksFor({ engine, membershipPolicy: 'invite-only', defaultOrgId: DEFAULT_ORG }); + const result = await hooks.session.create.before({ userId: USER }); + expect(result?.data?.activeOrganizationId).toBeUndefined(); + // Asserted on the STORE, not just on the draft: a policy that says "no + // auto-bind" must not be satisfied by binding and then declining to stamp. + expect(engine.insert).not.toHaveBeenCalled(); + expect(engine.rows).toHaveLength(0); + }); + + it('no unambiguous target organization (multi-org): nothing is bound, session org-less', async () => { + const engine = makeEngine(); + const hooks = hooksFor({ engine, defaultOrgId: null }); + const result = await hooks.session.create.before({ userId: USER }); + expect(result?.data?.activeOrganizationId).toBeUndefined(); + expect(engine.insert).not.toHaveBeenCalled(); + expect(engine.rows).toHaveLength(0); + }); + + it('a no-bind sign-in costs NO extra membership read — the re-read is gated on the outcome', async () => { + const engine = makeEngine(); + const hooks = hooksFor({ engine, membershipPolicy: 'invite-only', defaultOrgId: DEFAULT_ORG }); + await hooks.session.create.before({ userId: USER }); + // The two owner-preferred lookups of the ORIGINAL selection, and nothing + // more: `invite-only` is refused by the reconciler before it touches the + // store, and the re-read never runs because nothing was bound. + expect(engine.findOne).toHaveBeenCalledTimes(2); + expect(engine.find).not.toHaveBeenCalled(); + }); + }); + + describe('the pre-existing contract of this hook is untouched', () => { + it('a draft that already carries an org is left alone, and nothing is read or written', async () => { + const engine = makeEngine(); + const hooks = hooksFor({ engine, defaultOrgId: DEFAULT_ORG }); + const result = await hooks.session.create.before({ + userId: USER, + activeOrganizationId: 'org_explicit', + }); + expect(result?.data?.activeOrganizationId ?? 'org_explicit').toBe('org_explicit'); + expect(engine.findOne).not.toHaveBeenCalled(); + expect(engine.insert).not.toHaveBeenCalled(); + }); + + it('the HOST session hook still chains first and still wins', async () => { + const engine = makeEngine(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const hostHook = vi.fn(async (session: any) => ({ + data: { ...session, activeOrganizationId: 'org_host' }, + })); + const hooks = hooksFor({ + engine, + defaultOrgId: DEFAULT_ORG, + databaseHooks: { session: { create: { before: hostHook } } }, + }); + const result = await hooks.session.create.before({ userId: USER }); + expect(hostHook).toHaveBeenCalledTimes(1); + expect(result?.data?.activeOrganizationId).toBe('org_host'); + // The host owned it, so the settle never ran. + expect(engine.insert).not.toHaveBeenCalled(); + }); + + it('a broken engine never breaks session create', async () => { + const engine = { + findOne: vi.fn(async () => { throw new Error('db down'); }), + find: vi.fn(async () => { throw new Error('db down'); }), + insert: vi.fn(async () => { throw new Error('db down'); }), + }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const hooks = hooksFor({ engine: engine as any, defaultOrgId: DEFAULT_ORG }); + const result = await hooks.session.create.before({ userId: USER }); + expect(result?.data?.activeOrganizationId).toBeUndefined(); + }); + + it('`user.create.after` still binds — the creation seam is unchanged', async () => { + // The other half of "one owner": hoisting the settle to the session seam + // must not have quietly removed it from the seam every creation path + // flows through (admin create-user, bulk import, SSO JIT — none of which + // mint a session at all). + const engine = makeEngine(); + const hooks = hooksFor({ engine, defaultOrgId: DEFAULT_ORG }); + await hooks.user.create.after({ id: USER }); + expect(engine.rows).toHaveLength(1); + expect(engine.rows[0]).toMatchObject({ organization_id: DEFAULT_ORG, user_id: USER }); + }); + + it('both seams read the SAME policy — neither can auto-bind while the other does not', async () => { + // The drift `getMembershipPolicy()` exists to prevent (#5152), now that a + // second caller shares it. One manager, one policy, two seams: both must + // decline. + const engine = makeEngine(); + const hooks = hooksFor({ engine, membershipPolicy: 'invite-only', defaultOrgId: DEFAULT_ORG }); + await hooks.user.create.after({ id: USER }); + await hooks.session.create.before({ userId: USER }); + expect(engine.insert).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/scripts/adr-anchors/packages__plugins__plugin-auth__src__auth-manager.ts.json b/scripts/adr-anchors/packages__plugins__plugin-auth__src__auth-manager.ts.json index 026d720bbb..2eac6595f7 100644 --- a/scripts/adr-anchors/packages__plugins__plugin-auth__src__auth-manager.ts.json +++ b/scripts/adr-anchors/packages__plugins__plugin-auth__src__auth-manager.ts.json @@ -1,7 +1,8 @@ { "file": "packages/plugins/plugin-auth/src/auth-manager.ts", "adrs": [ + "ADR-0093", "ADR-0108" ], - "invariant": "better-auth's organization roles map registers the closed framework vocabulary ONLY. App-declared `position` / `permission` names are not organization roles — registering one makes it storable in `sys_member.role`, which `resolve-authz-context` projects into `current_user.positions`." + "invariant": "ADR-0108 — better-auth's organization roles map registers the closed framework vocabulary ONLY. App-declared `position` / `permission` names are not organization roles — registering one makes it storable in `sys_member.role`, which `resolve-authz-context` projects into `current_user.positions`.\n\nADR-0093 D2 — the membership reconciler is the SINGLE owner of the \"every user gets a membership\" invariant, and `settleMembership` is this manager's single assembly of its inputs. TWO seams call it and must never disagree: `user.create.after` (the creation seam every path flows through — email signup, admin create-user, bulk import, SSO JIT, none of which need a session) and `session.create.before` (which settles BEFORE resolving the session's active organization, so a user's first session is not minted tenant-less — #8245). Assembling the deps per call site would let them drift on the policy, which is a live platform setting read through `getMembershipPolicy()` (#5152): a captured option leaves one seam auto-binding after an admin switched the deployment to `invite-only`. The session-seam call must also stay a pure ORDERING change — same reconciler, same policy, same target-org resolution — so `invite-only` and multi-org still bind nobody and those sessions still mint with no active organization, which is the legal state the #8247 ruling declares." }