diff --git a/.changeset/accept-invitation-adopt-membership.md b/.changeset/accept-invitation-adopt-membership.md new file mode 100644 index 0000000000..0b8bb92cf1 --- /dev/null +++ b/.changeset/accept-invitation-adopt-membership.md @@ -0,0 +1,49 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): invitations can be accepted again — adopt the existing membership instead of colliding on the unique index (#7725) + +`POST /api/v1/auth/organization/accept-invitation` returned **HTTP 500 with an +empty body** and left the `sys_invitation` row `pending` **forever**. It was not +intermittent: on a single-organization deployment the flow in the docs — invite a +fresh email, invitee signs up through the link, invitee accepts — could never +complete at all, and the invitation was unrecoverable through the UI because +re-inviting an address that is already a member is refused too. + +Two correct platform decisions collided: + +- every user is auto-bound to the deployment's default organization at sign-up, + by the membership reconciler (ADR-0093 D1/D2), and +- `sys_member` declares `{ organization_id, user_id }` unique. + +better-auth's built-in accept-invitation route assumes an invitee is never +already a member: after flipping the invitation to `accepted` it inserts a +membership unconditionally, inside a transaction whose failure handler rolls the +invitation **back to `pending`** and rethrows. So the invitee's auto-bound row +made the insert fail, and the rollback erased the only evidence that acceptance +had been attempted. + +Acceptance now **adopts** that row rather than minting a second one. The declared +unique pair is the identity of a membership, so a create naming a pair that +already exists is that membership. The invitation ends `accepted`, and the +invitee holds exactly one membership in the target organization. + +**What adoption does to the role.** The invitation's role is written onto the +adopted row, so an invitation's intent is not silently replaced by the +reconciler's default `member` — accepting an `admin` invitation makes you an +admin even if you signed up first. One deliberate exception: **adoption never +lowers a grade.** If the existing membership already outranks the invitation's +role, the existing role is kept. Acceptance admits a person; demotion belongs to +`POST /organization/update-member-role`, which is the route the last-admin guard +stands on — without this exception, an organization's sole owner accepting a +`member` invitation would have been demoted past that guard, taking the +organization's last owner with it. + +The membership's `created_at` is not rewritten (the membership really did begin +at sign-up), and the adoption is recorded in `sys_member` history attributed to +the person who accepted. + +Unaffected: an invitee who is not yet a member of the target organization still +gets a membership created exactly as before, and the delegated-admin issuance +scope (ADR-0090 D12 / ADR-0105 D8) is untouched. diff --git a/packages/plugins/plugin-auth/src/accept-invitation-adopt-membership.test.ts b/packages/plugins/plugin-auth/src/accept-invitation-adopt-membership.test.ts new file mode 100644 index 0000000000..6706928781 --- /dev/null +++ b/packages/plugins/plugin-auth/src/accept-invitation-adopt-membership.test.ts @@ -0,0 +1,444 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7725] Regression suite for "accept-invitation returns a bodyless 500 and the +// invitation stays `pending` forever". +// +// These run the REAL better-auth pipeline through a REAL `AuthManager` — the +// organization plugin, the framework's `beforeCreateInvitation` role cap, the +// membership reconciler on `user.create.after`, and the ObjectQL adapter — the +// same shape as `auth-manager.jwt-eddsa-fallback.test.ts`. Nothing on the +// acceptance path is stubbed. +// +// ## The one thing the fake engine MUST do, or this whole file is theatre +// +// `sys_member` declares `{ organization_id, user_id }` UNIQUE +// (`platform-objects/src/identity/sys-member.object.ts`). A plain in-memory map +// happily stores two rows for one pair, so on such a fake the defect is +// invisible: acceptance "succeeds", the invitation flips to `accepted`, and the +// only symptom is a duplicate row nobody asserted about. The reported failure is +// the DATABASE refusing the second insert, so {@link createMemoryEngine} enforces +// that index and throws in SQLite's own words. That is what makes the ablation +// (revert the fix → these tests go red) mean anything. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { AuthManager } from './auth-manager'; + +const SECRET = 'test-secret-at-least-32-chars-long!!'; +const BASE = 'http://localhost:3000'; +const DEFAULT_ORG = 'org_default'; +const PARTNER_ORG = 'org_partner'; +const PASSWORD = 'S3cure!Passw0rd-7725'; + +/** + * In-memory engine that ENFORCES `sys_member`'s declared unique index. + * + * Everything else is the same minimal engine double the other end-to-end + * auth-manager suites use. + */ +const createMemoryEngine = () => { + const tables = new Map(); + const rows = (name: string) => { + if (!tables.has(name)) tables.set(name, []); + return tables.get(name)!; + }; + const eq = (a: any, b: any) => + a instanceof Date || b instanceof Date + ? new Date(a as any).getTime() === new Date(b as any).getTime() + : a === b; + const matches = (row: any, where: Record = {}) => + Object.entries(where).every(([k, v]) => { + const actual = row[k]; + if (v && typeof v === 'object' && !Array.isArray(v) && !(v instanceof Date)) { + if ('$ne' in v) return !eq(actual, v.$ne); + if ('$in' in v) return (v.$in as any[]).some((x) => eq(actual, x)); + if ('$gt' in v) return actual > v.$gt; + if ('$gte' in v) return actual >= v.$gte; + if ('$lt' in v) return actual < v.$lt; + if ('$lte' in v) return actual <= v.$lte; + if ('$regex' in v) return new RegExp(String(v.$regex)).test(String(actual ?? '')); + } + return eq(actual, v); + }); + + /** + * The declared index, in the driver's voice. The message is quoted from the + * server log on the issue so a future reader can match the two by eye. + */ + const assertMemberUnique = (name: string, candidate: any, ignoreId?: string) => { + if (name !== 'sys_member') return; + const clash = rows(name).some( + (r) => + r.id !== ignoreId && + eq(r.organization_id, candidate.organization_id) && + eq(r.user_id, candidate.user_id), + ); + if (clash) { + throw new Error( + 'insert into sys_member … UNIQUE constraint failed: ' + + 'sys_member.organization_id, sys_member.user_id', + ); + } + }; + + let seq = 0; + return { + tables, + async insert(name: string, data: any) { + const row = { id: data.id ?? `row_${++seq}`, ...data }; + assertMemberUnique(name, row); + rows(name).push(row); + return { ...row }; + }, + async findOne(name: string, q: any = {}) { + const found = rows(name).find((r) => matches(r, q.where)); + return found ? { ...found } : null; + }, + async find(name: string, q: any = {}) { + let out = rows(name).filter((r) => matches(r, q.where)); + const order = q.orderBy?.[0]; + if (order) { + out = [...out].sort( + (a, b) => (a[order.field] > b[order.field] ? 1 : -1) * (order.order === 'desc' ? -1 : 1), + ); + } + if (q.offset) out = out.slice(q.offset); + if (q.limit) out = out.slice(0, q.limit); + return out.map((r) => ({ ...r })); + }, + async count(name: string, q: any = {}) { + return rows(name).filter((r) => matches(r, q.where)).length; + }, + async update(name: string, patch: any, options?: any) { + // Pinned to ObjectQL.update's own dispatch predicate, for the same reason + // `delete` below is: this fake carries the ADOPTION write, so a fake looser + // than the real engine would let a by-id update that ObjectQL refuses look + // like a working adoption. `adopt-membership.ts` dispatches by scalar id. + assertEngineUpdateDispatch(patch, options); + const row = rows(name).find((r) => r.id === patch.id); + if (!row) return null; + assertMemberUnique(name, { ...row, ...patch }, row.id); + Object.assign(row, patch); + return { ...row }; + }, + async delete(name: string, q: any = {}) { + // [#4550] Pinned to ObjectQL.delete's own dispatch predicate rather than a + // hand-written approximation of it — a fake that accepts a call the real + // engine refuses is how a dead route ships with its suite green. + assertEngineDeleteDispatch(q); + const table = rows(name); + const keep = table.filter((r) => !matches(r, q.where)); + tables.set(name, keep); + return table.length - keep.length; + }, + }; +}; + +type MemoryEngine = ReturnType; + +/** Minimal tenancy stub — the reconciler only ever asks for `defaultOrgId()`. */ +const singleOrgTenancy = () => + ({ + posture: 'single', + requestedPosture: 'single', + isolationActive: false, + requested: false, + degraded: false, + defaultOrgId: async () => DEFAULT_ORG, + }) as any; + +const makeManager = (engine: MemoryEngine) => + new AuthManager({ + secret: SECRET, + baseUrl: BASE, + dataEngine: engine as any, + // Single-org, auto-bind — the ADR-0093 D1/D2 posture the defect lives on. + membershipPolicy: 'auto', + getTenancy: () => singleOrgTenancy(), + }); + +const cookieFrom = (response: Response): string => + (response.headers.getSetCookie?.() ?? [response.headers.get('set-cookie') ?? '']) + .map((c) => c.split(';')[0]) + .filter(Boolean) + .join('; '); + +const post = (manager: AuthManager, path: string, body: unknown, cookie?: string) => + manager.handleRequest( + new Request(`${BASE}/api/v1/auth${path}`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + ...(cookie ? { cookie } : {}), + }, + body: JSON.stringify(body), + }), + ); + +/** Sign a user up and return their session cookie + user id. */ +const signUp = async (manager: AuthManager, engine: MemoryEngine, email: string) => { + const res = await post(manager, '/sign-up/email', { email, password: PASSWORD, name: email }); + expect(res.status, await res.clone().text()).toBe(200); + const user = (engine.tables.get('sys_user') ?? []).find((u) => u.email === email); + expect(user, `sign-up did not create ${email}`).toBeDefined(); + return { cookie: cookieFrom(res), userId: String(user!.id) }; +}; + +const membersOf = (engine: MemoryEngine, organizationId: string, userId: string) => + (engine.tables.get('sys_member') ?? []).filter( + (m) => m.organization_id === organizationId && m.user_id === userId, + ); + +const invitationsFor = (engine: MemoryEngine, email: string) => + (engine.tables.get('sys_invitation') ?? []).filter((i) => i.email === email); + +/** Force a membership role directly, the way an operator's data would look. */ +const setRole = (engine: MemoryEngine, organizationId: string, userId: string, role: string) => { + const [row] = membersOf(engine, organizationId, userId); + expect(row, 'no membership to promote — the reconciler did not bind').toBeDefined(); + row!.role = role; +}; + +const seedOrganizations = (engine: MemoryEngine) => { + engine.tables.set('sys_organization', [ + { id: DEFAULT_ORG, name: 'Default', slug: 'default' }, + { id: PARTNER_ORG, name: 'Partner', slug: 'partner' }, + ]); +}; + +/** + * Bring up a deployment with an owner who can issue invitations. + * + * Order matters and mirrors the issue's reproduction: the invitation is created + * BEFORE the invitee exists, because better-auth's `invite-member` refuses an + * email that is already a member of the target org + * (`USER_IS_ALREADY_A_MEMBER_OF_THIS_ORGANIZATION`). The collision is created by + * the invitee then SIGNING UP, at which point the reconciler binds them to the + * default organization — the very org they were invited into. + */ +const bootWithOwner = async () => { + const engine = createMemoryEngine(); + seedOrganizations(engine); + const manager = makeManager(engine); + const owner = await signUp(manager, engine, 'owner@example.com'); + setRole(engine, DEFAULT_ORG, owner.userId, 'owner'); + return { engine, manager, owner }; +}; + +describe('#7725 — accepting an invitation when the invitee is already auto-bound', () => { + beforeEach(() => { + vi.spyOn(console, 'info').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('the reported bug: acceptance returns 2xx, the invitation becomes accepted, and there is exactly ONE membership', async () => { + const { engine, manager, owner } = await bootWithOwner(); + + const invite = await post( + manager, + '/organization/invite-member', + { email: 'invitee@example.com', role: 'member', organizationId: DEFAULT_ORG }, + owner.cookie, + ); + expect(invite.status, await invite.clone().text()).toBe(200); + const [invitation] = invitationsFor(engine, 'invitee@example.com'); + expect(invitation?.status).toBe('pending'); + + // The sign-up that creates the collision: the reconciler binds the invitee + // to the default org, which is the invitation's target org. + const invitee = await signUp(manager, engine, 'invitee@example.com'); + expect(membersOf(engine, DEFAULT_ORG, invitee.userId)).toHaveLength(1); + + const accept = await post( + manager, + '/organization/accept-invitation', + { invitationId: String(invitation.id) }, + invitee.cookie, + ); + + // Before the fix this was a 500 with an EMPTY body. + expect(accept.status, await accept.clone().text()).toBe(200); + const body: any = await accept.json(); + expect(body.member?.userId ?? body.member?.user_id).toBe(invitee.userId); + + // ... and the invitation no longer sits at `pending` forever. better-auth's + // transaction catch is what rolled it back, so this assertion is the one + // that pins the reported user-visible symptom, not merely the status code. + const [after] = invitationsFor(engine, 'invitee@example.com'); + expect(after.status).toBe('accepted'); + + // EXACTLY one membership — adoption, not a second row and not a lost one. + expect(membersOf(engine, DEFAULT_ORG, invitee.userId)).toHaveLength(1); + }); + + it('the adopted row carries the INVITATION’s role, not the reconciler’s default', async () => { + const { engine, manager, owner } = await bootWithOwner(); + + // An owner may invite an admin (the role cap only bounds issuers BELOW admin + // grade), so this is the case where invitation intent and the auto-bound + // default actually differ. + const invite = await post( + manager, + '/organization/invite-member', + { email: 'promoted@example.com', role: 'admin', organizationId: DEFAULT_ORG }, + owner.cookie, + ); + expect(invite.status, await invite.clone().text()).toBe(200); + const [invitation] = invitationsFor(engine, 'promoted@example.com'); + + const invitee = await signUp(manager, engine, 'promoted@example.com'); + // The reconciler's default, which adoption must not silently keep. + expect(membersOf(engine, DEFAULT_ORG, invitee.userId)[0]!.role).toBe('member'); + + const accept = await post( + manager, + '/organization/accept-invitation', + { invitationId: String(invitation.id) }, + invitee.cookie, + ); + expect(accept.status, await accept.clone().text()).toBe(200); + + const rows = membersOf(engine, DEFAULT_ORG, invitee.userId); + expect(rows).toHaveLength(1); + expect(rows[0]!.role).toBe('admin'); + }); + + it('adoption never DEMOTES: an owner accepting a member invitation keeps owner', async () => { + // Acceptance is an admission instrument. Demotion belongs to + // `update-member-role`, which is where `last-admin-guard` stands — routing a + // demotion through acceptance would take an organization's last owner away + // with every gate reporting success. + const { engine, manager, owner } = await bootWithOwner(); + + // Seeded directly, and it has to be: `invite-member` refuses an email that is + // already a member of the target org, so an owner can never be re-invited + // into their own organization through the route. The row is exactly what + // better-auth would have written — this pins what ACCEPTANCE does with such + // an invitation, however it came to exist (a stale invitation the invitee + // signed up against, an operator's import, a re-invite after a role change). + await engine.insert('sys_invitation', { + id: 'inv_demote', + email: 'owner@example.com', + role: 'member', + status: 'pending', + organization_id: DEFAULT_ORG, + inviter_id: owner.userId, + expires_at: new Date(Date.now() + 86_400_000), + }); + + const accept = await post( + manager, + '/organization/accept-invitation', + { invitationId: 'inv_demote' }, + owner.cookie, + ); + expect(accept.status, await accept.clone().text()).toBe(200); + + const rows = membersOf(engine, DEFAULT_ORG, owner.userId); + expect(rows).toHaveLength(1); + expect(rows[0]!.role).toBe('owner'); + }); + + it('an invitee who is NOT yet a member of the target org still gets a membership CREATED', async () => { + // The untouched half: adoption must not swallow the ordinary insert. The + // invitee here is auto-bound to the DEFAULT org and invited into a DIFFERENT + // one, so no pair collides and better-auth's own insert must still run. + const { engine, manager, owner } = await bootWithOwner(); + + // Give the owner a membership in the partner org so they may invite there. + await engine.insert('sys_member', { + id: 'mem_owner_partner', + organization_id: PARTNER_ORG, + user_id: owner.userId, + role: 'owner', + }); + + const invite = await post( + manager, + '/organization/invite-member', + { email: 'crossorg@example.com', role: 'member', organizationId: PARTNER_ORG }, + owner.cookie, + ); + expect(invite.status, await invite.clone().text()).toBe(200); + const [invitation] = invitationsFor(engine, 'crossorg@example.com'); + + const invitee = await signUp(manager, engine, 'crossorg@example.com'); + expect(membersOf(engine, DEFAULT_ORG, invitee.userId)).toHaveLength(1); + expect(membersOf(engine, PARTNER_ORG, invitee.userId)).toHaveLength(0); + + const accept = await post( + manager, + '/organization/accept-invitation', + { invitationId: String(invitation.id) }, + invitee.cookie, + ); + expect(accept.status, await accept.clone().text()).toBe(200); + + // A NEW row in the partner org, and the default-org one left alone. + expect(membersOf(engine, PARTNER_ORG, invitee.userId)).toHaveLength(1); + expect(membersOf(engine, DEFAULT_ORG, invitee.userId)).toHaveLength(1); + expect(invitationsFor(engine, 'crossorg@example.com')[0]!.status).toBe('accepted'); + }); +}); + +describe('#7725 — the delegated-admin issuance scope is UNCHANGED (ADR-0090 D12 / ADR-0105 D8)', () => { + // These are the pins that must stay GREEN through the ablation: the reported + // break was purely in acceptance, and the fix must not move the issuance gate + // a millimetre. + beforeEach(() => { + vi.spyOn(console, 'info').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + const bootWithDelegate = async () => { + const { engine, manager, owner } = await bootWithOwner(); + const delegate = await signUp(manager, engine, 'delegate@example.com'); + setRole(engine, DEFAULT_ORG, delegate.userId, 'delegated_admin'); + return { engine, manager, owner, delegate }; + }; + + it('a delegated_admin CAN invite a plain member — 200, exactly one attributed pending row', async () => { + const { engine, manager, delegate } = await bootWithDelegate(); + + const res = await post( + manager, + '/organization/invite-member', + { email: 'scoped.ok@example.com', role: 'member', organizationId: DEFAULT_ORG }, + delegate.cookie, + ); + expect(res.status, await res.clone().text()).toBe(200); + + const invitations = invitationsFor(engine, 'scoped.ok@example.com'); + expect(invitations).toHaveLength(1); + expect(invitations[0]!.status).toBe('pending'); + expect(invitations[0]!.role).toBe('member'); + expect(invitations[0]!.inviter_id).toBe(delegate.userId); + }); + + it('a delegated_admin CANNOT invite an admin — 403 with the ADR-0090 D12 message, and ZERO orphan rows', async () => { + const { engine, manager, delegate } = await bootWithDelegate(); + + const res = await post( + manager, + '/organization/invite-member', + { email: 'scoped.denied@example.com', role: 'admin', organizationId: DEFAULT_ORG }, + delegate.cookie, + ); + expect(res.status).toBe(403); + const body = await res.text(); + expect(body).toContain('ADR-0090 D12'); + expect(body).toContain('Access denied'); + + // The cap runs in `beforeCreateInvitation`, which better-auth invokes BEFORE + // `adapter.createInvitation` — so a refusal leaves nothing behind. + expect(invitationsFor(engine, 'scoped.denied@example.com')).toHaveLength(0); + }); +}); diff --git a/packages/plugins/plugin-auth/src/adopt-membership.test.ts b/packages/plugins/plugin-auth/src/adopt-membership.test.ts new file mode 100644 index 0000000000..1366db518d --- /dev/null +++ b/packages/plugins/plugin-auth/src/adopt-membership.test.ts @@ -0,0 +1,174 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7725] Unit pins for the adoption decision itself. +// +// The end-to-end proof lives in `accept-invitation-adopt-membership.test.ts` +// (real better-auth pipeline, an engine that enforces `sys_member`'s unique +// index). This file pins the two things that file exercises only along the +// paths it happens to drive: the role ladder, and the "not an adoption case" +// answers that must stay a plain insert. + +import { describe, it, expect, vi } from 'vitest'; +import { adoptExistingMembership, decideAdoptedRole } from './adopt-membership'; + +const silent = { logger: { info: () => {} } }; + +describe('decideAdoptedRole', () => { + it('applies the invitation’s role over the reconciler’s auto-bound default', () => { + expect(decideAdoptedRole('member', 'admin')).toEqual({ role: 'admin', verdict: 'applied' }); + expect(decideAdoptedRole('member', 'owner')).toEqual({ role: 'owner', verdict: 'applied' }); + }); + + it('takes a grade-FLAT invitation role too — intent is not dropped just because the grade is equal', () => { + // `delegated_admin` is reach, not authority (ADR-0105 D8), so `orgRoleGrade` + // puts it level with `member`. Keeping the default here is exactly the + // "silently keep the default" failure adoption exists to avoid. + expect(decideAdoptedRole('member', 'delegated_admin')).toEqual({ + role: 'delegated_admin', + verdict: 'applied', + }); + }); + + it('never LOWERS a grade — demotion belongs to update-member-role, where last-admin-guard stands', () => { + expect(decideAdoptedRole('owner', 'member')).toEqual({ role: 'owner', verdict: 'kept-higher' }); + expect(decideAdoptedRole('owner', 'admin')).toEqual({ role: 'owner', verdict: 'kept-higher' }); + expect(decideAdoptedRole('admin', 'member')).toEqual({ role: 'admin', verdict: 'kept-higher' }); + }); + + it('reports an identical role as unchanged, across better-auth’s spellings', () => { + expect(decideAdoptedRole('member', 'member').verdict).toBe('unchanged'); + // Comma-joined and array spellings are the same value to better-auth, so a + // pointless write must not be issued for them either. + expect(decideAdoptedRole('owner,admin', 'admin,owner').verdict).toBe('unchanged'); + expect(decideAdoptedRole('admin', ['admin']).verdict).toBe('unchanged'); + }); + + it('keeps the existing role when nothing was asked for', () => { + expect(decideAdoptedRole('admin', undefined)).toEqual({ role: 'admin', verdict: 'unchanged' }); + expect(decideAdoptedRole('admin', '')).toEqual({ role: 'admin', verdict: 'unchanged' }); + }); +}); + +describe('adoptExistingMembership — when it declines, the caller must insert normally', () => { + const engineWith = (row: any) => ({ + findOne: vi.fn().mockResolvedValue(row), + update: vi.fn().mockImplementation(async (_o: string, patch: any) => ({ ...row, ...patch })), + }); + + it('declines for any object other than sys_member', async () => { + const engine = engineWith({ id: 'm1', role: 'member' }); + expect(await adoptExistingMembership(engine, 'sys_user', { user_id: 'u1' }, silent)).toBeNull(); + expect(engine.findOne).not.toHaveBeenCalled(); + }); + + it('declines when either half of the declared unique key is missing', async () => { + const engine = engineWith({ id: 'm1', role: 'member' }); + expect( + await adoptExistingMembership(engine, 'sys_member', { user_id: 'u1' }, silent), + ).toBeNull(); + expect( + await adoptExistingMembership(engine, 'sys_member', { organization_id: 'o1' }, silent), + ).toBeNull(); + // A null organization_id is left alone on purpose: ADR-0120 D3 folds NULL + // into the `'__global__'` bucket for uniqueness and this function does not + // reproduce that folding. + expect( + await adoptExistingMembership( + engine, + 'sys_member', + { organization_id: null, user_id: 'u1' }, + silent, + ), + ).toBeNull(); + expect(engine.findOne).not.toHaveBeenCalled(); + }); + + it('declines when no row exists for the pair — the ordinary insert path', async () => { + const engine = engineWith(null); + expect( + await adoptExistingMembership( + engine, + 'sys_member', + { organization_id: 'o1', user_id: 'u1', role: 'member' }, + silent, + ), + ).toBeNull(); + expect(engine.findOne).toHaveBeenCalledWith('sys_member', { + where: { organization_id: 'o1', user_id: 'u1' }, + }); + expect(engine.update).not.toHaveBeenCalled(); + }); +}); + +describe('adoptExistingMembership — when it adopts', () => { + const engineWith = (row: any) => ({ + findOne: vi.fn().mockResolvedValue(row), + update: vi.fn().mockImplementation(async (_o: string, patch: any) => ({ ...row, ...patch })), + }); + + it('writes the invitation’s role onto the existing row and returns it', async () => { + const engine = engineWith({ + id: 'mem_1', + organization_id: 'o1', + user_id: 'u1', + role: 'member', + created_at: '2026-01-01T00:00:00.000Z', + }); + + const adopted = await adoptExistingMembership( + engine, + 'sys_member', + { id: 'discarded', organization_id: 'o1', user_id: 'u1', role: 'admin' }, + silent, + ); + + expect(engine.update).toHaveBeenCalledWith('sys_member', { id: 'mem_1', role: 'admin' }); + expect(adopted).toMatchObject({ id: 'mem_1', role: 'admin' }); + // The membership really did begin at sign-up — acceptance does not restart it. + expect(adopted!.created_at).toBe('2026-01-01T00:00:00.000Z'); + }); + + it('issues NO write when the role is already right, and still returns the row', async () => { + const engine = engineWith({ id: 'mem_1', organization_id: 'o1', user_id: 'u1', role: 'admin' }); + const adopted = await adoptExistingMembership( + engine, + 'sys_member', + { organization_id: 'o1', user_id: 'u1', role: 'admin' }, + silent, + ); + expect(engine.update).not.toHaveBeenCalled(); + expect(adopted).toMatchObject({ id: 'mem_1', role: 'admin' }); + }); + + it('issues no write — and keeps the higher grade — when the invitation would demote', async () => { + const engine = engineWith({ id: 'mem_1', organization_id: 'o1', user_id: 'u1', role: 'owner' }); + const adopted = await adoptExistingMembership( + engine, + 'sys_member', + { organization_id: 'o1', user_id: 'u1', role: 'member' }, + silent, + ); + expect(engine.update).not.toHaveBeenCalled(); + expect(adopted).toMatchObject({ id: 'mem_1', role: 'owner' }); + }); + + it('says so out loud — an adoption is never silent', async () => { + const info = vi.fn(); + const engine = engineWith({ id: 'mem_1', organization_id: 'o1', user_id: 'u1', role: 'member' }); + await adoptExistingMembership( + engine, + 'sys_member', + { organization_id: 'o1', user_id: 'u1', role: 'admin' }, + { logger: { info } }, + ); + expect(info).toHaveBeenCalledTimes(1); + expect(String(info.mock.calls[0]![0])).toContain('adopted the existing sys_member row'); + expect(info.mock.calls[0]![1]).toMatchObject({ + memberId: 'mem_1', + existingRole: 'member', + incomingRole: 'admin', + resultingRole: 'admin', + verdict: 'applied', + }); + }); +}); diff --git a/packages/plugins/plugin-auth/src/adopt-membership.ts b/packages/plugins/plugin-auth/src/adopt-membership.ts new file mode 100644 index 0000000000..e40c6eae13 --- /dev/null +++ b/packages/plugins/plugin-auth/src/adopt-membership.ts @@ -0,0 +1,247 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7725] Membership adoption — reconcile better-auth's insert-only membership + * write with the platform's own "every user is already bound" invariant. + * + * ## The defect this closes + * + * `POST /api/v1/auth/organization/accept-invitation` returned a **bodyless HTTP + * 500** and left the `sys_invitation` row `pending` forever. The server log + * named the cause exactly: + * + * ``` + * insert into sys_member … UNIQUE constraint failed: sys_member.organization_id, sys_member.user_id + * ``` + * + * Two platform decisions collide, and both are individually correct: + * + * 1. **ADR-0093 D1/D2** — every user is auto-bound to the deployment's default + * organization at sign-up, by the membership reconciler + * (`reconcile-membership.ts`, composed into `user.create.after`). + * 2. **`sys_member` declares `{ organization_id, user_id }` unique** — one + * membership per (org, user) pair, which is the platform's identity key for + * a membership, not merely a constraint. + * + * better-auth's built-in accept-invitation route (`crud-invites.mjs`) assumes an + * invitee is never already a member: after flipping the invitation to + * `accepted` it calls `adapter.createMember(...)` unconditionally, inside a + * transaction whose `catch` rolls the invitation **back to `pending`** and + * rethrows. So a single-org deployment — where the reconciler has already bound + * the invitee to the very org they are being invited into — could never accept + * an invitation at all. The reconciler yields to a pre-existing row + * (`insertMembership` checks first); better-auth's insert does not go through + * that seam. + * + * ## Why the seam is HERE, at the better-auth → ObjectQL adapter + * + * The three hook seams the framework already owns on this route all run at the + * wrong moment or with the wrong reach, verified against better-auth + * `1.7.0-rc.2`: + * + * - `organizationHooks.beforeAcceptInvitation` fires *before* `createMember` + * and can only throw or mutate. The one mutation that would make the insert + * succeed is deleting the pre-existing row — which destroys the membership's + * `created_at`/history, opens a window in which the invitee belongs to + * nothing, and reaches straight into `sys_member`'s `deleteBehavior`, which + * is a different card's decision surface (#7724). Refused. + * - `hooks.before` (the global route-boundary middleware in `auth-manager.ts`) + * has the endpoint ctx, but `ctx.context` is the **shared** `AuthContext` + * singleton (`to-auth-endpoints.mjs`: `context: authContext`, where + * `authContext` is the awaited instance-wide context), so per-request state + * parked there leaks across concurrent requests — an adapter swap parked + * there most of all. Refused. + * - Re-implementing the route (own the "already a member" branch at the route + * boundary) would duplicate better-auth's recipient / expiry / status / + * membership-limit / email-verification checks. Duplicated security checks + * are where bypasses live; an invitee who is already a member would be + * adjudicated by *our* copy of those rules. Refused. + * + * What is left — and what is actually the honest layer — is the adapter that + * bridges better-auth's `member` model onto `sys_member`. That adapter is + * already the place where better-auth's model is reconciled with the platform's + * object: it maps model and field names, normalises dates and identifiers. This + * adds one more reconciliation, and it is stated as the platform's own rule + * rather than as leniency: **the declared unique index IS the identity of a + * membership**, so a `create` naming an (org, user) pair that already exists is + * not a second membership — it is that membership. + * + * Blast radius, measured against better-auth 1.7.0-rc.2 rather than assumed — + * accept-invitation is the ONLY `member` create that can reach an existing pair: + * + * - `POST /organization/add-member` pre-checks and refuses first + * (`crud-members.mjs`: `findMemberByEmail` → + * `USER_IS_ALREADY_A_MEMBER_OF_THIS_ORGANIZATION`, a 400). + * - `POST /organization/create-organization` mints a fresh organization id, so + * no pair can pre-exist. + * - The reconciler and its backfill write through `engine.insert` directly, not + * through this adapter, and already yield to an existing row. + * + * ## What adoption does to the ROLE — the part that is not "don't throw" + * + * Not throwing is the smaller half. An invitation carries an intent, and the + * reconciler's auto-created row carries the deployment default (`member`); if + * adoption silently kept the default, a `delegated_admin` invitation would be + * accepted and confer nothing. So adoption WRITES the invitation's role onto the + * adopted row — with exactly one exception, {@link decideAdoptedRole}: + * + * **Adoption never lowers a grade.** If the existing membership outranks the + * invitation's role, the existing role is kept and the adoption is recorded as + * `kept-higher`. Acceptance is an ADMISSION instrument — `invitation-role-cap.ts` + * states the same posture from the other side ("an invitation may add a person, + * never authority above the issuer's own") — while DEMOTION has its own governed + * route, `POST /organization/update-member-role`, which is where + * `last-admin-guard.ts` stands. Without this exception, inviting an + * organization's sole `owner` as a `member` and having them accept would demote + * them through a path that guard never sees, and the organization would lose its + * last owner with every gate reporting success. Equal grades take the + * invitation's value (a lateral move — `member` → `delegated_admin` is grade-flat + * by `orgRoleGrade`, since `delegated_admin` is reach, not authority), which is + * what keeps the invitation's intent from being dropped in the ordinary case. + * + * ## Attribution + * + * Nothing extra to do, and that is deliberate. The adoption writes through the + * same `withSystemContext`-wrapped engine as every other adapter write, whose + * `update` carries `attributedUserId` (#4586) — here, the invitee who accepted. + * `sys_member` is `trackHistory: true`, so the adoption lands in history as a + * role change attributed to the acceptor, next to the `create` the reconciler + * recorded at sign-up. A fresh insert would have recorded a `create` attributed + * to the same person; the adopted row records an `update` instead. The + * membership's `created_at` is deliberately NOT rewritten — the membership + * really did begin at sign-up. + */ + +import { SystemObjectName } from '@objectstack/spec/system'; +import { orgRoleGrade, parseOrgRoles } from './invitation-role-cap.js'; + +/** What adoption decided to do with the incoming role. */ +export type MembershipAdoptionVerdict = + /** The incoming role was written onto the adopted row. */ + | 'applied' + /** Incoming and existing roles are the same value — nothing to write. */ + | 'unchanged' + /** + * The existing membership outranks the incoming role, so the existing role was + * KEPT. Acceptance admits; it does not demote (see the module doc). + */ + | 'kept-higher'; + +export interface MembershipAdoptionDecision { + /** The role the adopted row should end up carrying. */ + role: unknown; + verdict: MembershipAdoptionVerdict; +} + +/** Normalised comparable form of a better-auth role value (`"owner,admin"`). */ +function normaliseRole(raw: unknown): string { + return parseOrgRoles(raw).sort().join(','); +} + +/** + * Decide the role an adopted membership should carry. + * + * @param existingRole the role already on the `sys_member` row + * @param incomingRole the role better-auth's `createMember` was about to write — + * i.e. the invitation's role + */ +export function decideAdoptedRole( + existingRole: unknown, + incomingRole: unknown, +): MembershipAdoptionDecision { + const incoming = normaliseRole(incomingRole); + // Nothing was asked for (better-auth's body schema requires a role, so this is + // a caller that is not the invitation flow) — keep what is there. + if (incoming.length === 0) return { role: existingRole, verdict: 'unchanged' }; + + const existing = normaliseRole(existingRole); + if (existing === incoming) return { role: existingRole, verdict: 'unchanged' }; + + // The one refusal: never lower a grade through acceptance. `update-member-role` + // owns demotion, and `last-admin-guard.ts` stands on that route, not this one. + if (orgRoleGrade(incomingRole) < orgRoleGrade(existingRole)) { + return { role: existingRole, verdict: 'kept-higher' }; + } + + return { role: incomingRole, verdict: 'applied' }; +} + +/** + * The minimum engine surface adoption needs. Deliberately narrow so a caller + * cannot hand this function delete/insert reach it has no business having. + */ +export interface MembershipAdoptionEngine { + findOne(object: string, query?: any): Promise; + update(object: string, data: any, options?: any): Promise; +} + +export interface AdoptMembershipOptions { + /** Override for tests; defaults to `console`. */ + logger?: { info?: (msg: string, meta?: any) => void }; +} + +/** + * Adopt the `sys_member` row a `create` would have collided with, or return + * `null` to let the caller insert normally. + * + * `null` means "not an adoption case" and is the common answer: a different + * object, an unidentifiable pair, or simply no existing row. The caller must + * treat `null` as "carry on and insert" — this function never suppresses a + * genuine insert. + * + * @param engine a system-context engine (the adapter's `withSystemContext` one) + * @param object the protocol object name the create targets + * @param payload the snake_case row better-auth is about to insert + */ +export async function adoptExistingMembership( + engine: MembershipAdoptionEngine, + object: string, + payload: Record, + options: AdoptMembershipOptions = {}, +): Promise | null> { + if (object !== SystemObjectName.MEMBER) return null; + + const organizationId = payload?.organization_id; + const userId = payload?.user_id; + // Both halves of the declared unique key must be present and non-empty for the + // pair to identify anything. A null/absent `organization_id` is left alone on + // purpose: ADR-0120 D3 folds NULL into the `'__global__'` bucket for uniqueness + // and this function would have to reproduce that folding to be correct there — + // an unrelated surface, and the reconciler never mints such a row. + if (typeof organizationId !== 'string' || organizationId.length === 0) return null; + if (typeof userId !== 'string' || userId.length === 0) return null; + + const existing = await engine.findOne(SystemObjectName.MEMBER, { + where: { organization_id: organizationId, user_id: userId }, + }); + if (!existing?.id) return null; + + const decision = decideAdoptedRole(existing.role, payload.role); + let adopted: Record = existing; + if (decision.verdict === 'applied') { + const updated = await engine.update(SystemObjectName.MEMBER, { + id: existing.id, + role: decision.role, + }); + // ObjectQL returns the updated row; fall back to a local merge so adoption + // still reports the state it just wrote if an engine returns nothing. + adopted = updated ?? { ...existing, role: decision.role }; + } + + const log = options.logger?.info ?? ((msg: string, meta?: any) => console.info(msg, meta)); + log( + `[membership] adopted the existing sys_member row instead of inserting a second one ` + + `(${decision.verdict}) — the (organization_id, user_id) pair is unique by declaration [#7725]`, + { + memberId: existing.id, + organizationId, + userId, + existingRole: existing.role, + incomingRole: payload.role, + resultingRole: adopted.role, + verdict: decision.verdict, + }, + ); + + return adopted; +} diff --git a/packages/plugins/plugin-auth/src/index.ts b/packages/plugins/plugin-auth/src/index.ts index b61fbe33a4..fe7e55e8d5 100644 --- a/packages/plugins/plugin-auth/src/index.ts +++ b/packages/plugins/plugin-auth/src/index.ts @@ -55,6 +55,11 @@ export * from './auth-schema-config.js'; // ADR-0093 — membership reconciler + tenancy service (public host API: hosts // compose the reconciler into their own hooks; embeddings query tenancy mode). export * from './reconcile-membership.js'; +// [#7725] The other half of that invariant: what happens when better-auth tries +// to create a membership the reconciler already created. Exported alongside the +// reconciler because a host composing its own membership writes needs the same +// "the unique pair IS the membership, and acceptance never demotes" rule. +export * from './adopt-membership.js'; export * from './tenancy-service.js'; // [ADR-0108 / #3723] `./org-roles.js` is gone: there is no app-declared // organization-role vocabulary to collect, normalize or materialize. The four diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index 583358bf47..df2df55235 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -5,6 +5,7 @@ import { createAdapterFactory } from 'better-auth/adapters'; import type { CleanedWhere, WhereOperator } from 'better-auth/adapters'; import { SystemObjectName } from '@objectstack/spec/system'; import { resolveAttributedUserId } from './auth-actor-attribution.js'; +import { adoptExistingMembership } from './adopt-membership.js'; import { filterRevokedSessionRows, hideRevokedSessionRow, @@ -665,7 +666,17 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { const objectName = resolveProtocolName(model); const bridged = objectName !== model; const payload = normaliseIdentifierWrite(model, data); - const result = await dataEngine.insert(objectName, bridged ? remapKeys(payload, camelToSnake) : payload); + const row = bridged ? remapKeys(payload, camelToSnake) : payload; + // [#7725] `sys_member` declares `{organization_id, user_id}` unique, and + // the platform auto-binds every user at sign-up (ADR-0093 D1/D2), so + // better-auth's accept-invitation `createMember` collides on a pair that + // by declaration IS the membership it is trying to create. Adopt that row + // instead of minting a second one. Returns `null` for every other case — + // other object, unidentifiable pair, no existing row — and then this is a + // plain insert, byte for byte as before. See `adopt-membership.ts` for + // why the seam is here and what adoption does to the role. + const adopted = await adoptExistingMembership(dataEngine as any, objectName, row); + const result = adopted ?? (await dataEngine.insert(objectName, row)); const norm = normaliseLegacyDates(model, result); return (bridged ? remapKeys(norm, snakeToCamel) : norm) as T; },