diff --git a/.server-changes/fix-invite-role-change-for-existing-members.md b/.server-changes/fix-invite-role-change-for-existing-members.md new file mode 100644 index 00000000000..ad4fb18b622 --- /dev/null +++ b/.server-changes/fix-invite-role-change-for-existing-members.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: fix +--- + +Accepting an old invitation could change the role of someone who was already in the organization. An invitation now leaves an existing member's role untouched, people who are already in an organization are no longer sent invitations to it, and the invite form now says which addresses it skipped instead of failing with an unhelpful error. diff --git a/apps/webapp/app/models/member.server.ts b/apps/webapp/app/models/member.server.ts index b786ce7e352..4167738413b 100644 --- a/apps/webapp/app/models/member.server.ts +++ b/apps/webapp/app/models/member.server.ts @@ -100,6 +100,17 @@ export async function inviteMembers({ throw new Error("User does not have access to this organization"); } + const uniqueEmails = new Set(emails); + + const existingMembers = await prisma.orgMember.findMany({ + where: { + organizationId: org.id, + user: { email: { in: [...uniqueEmails] } }, + }, + select: { user: { select: { email: true } } }, + }); + const existingMemberEmails = new Set(existingMembers.map((member) => member.user.email)); + // Create one invite per unique email and return ONLY the invites actually // created by this call. A P2002 means the email is already invited to this org // (unique org+email) — skip it so one duplicate can't fail the batch, and @@ -108,8 +119,15 @@ export async function inviteMembers({ const created: Prisma.OrgMemberInviteGetPayload<{ include: { organization: true; inviter: true }; }>[] = []; + const alreadyMembers: string[] = []; + const alreadyInvited: string[] = []; + + for (const email of uniqueEmails) { + if (existingMemberEmails.has(email)) { + alreadyMembers.push(email); + continue; + } - for (const email of new Set(emails)) { try { const invite = await prisma.orgMemberInvite.create({ data: { @@ -131,13 +149,14 @@ export async function inviteMembers({ error instanceof PrismaNamespace.PrismaClientKnownRequestError && error.code === "P2002" ) { + alreadyInvited.push(email); continue; } throw error; } } - return created; + return { created, alreadyMembers, alreadyInvited }; } export async function getInviteFromToken({ token }: { token: string }) { @@ -264,24 +283,41 @@ async function assignInviteRbacRole({ userId, organizationId, rbacRoleId, + onlyWhenUnassigned, }: { userId: string; organizationId: string; rbacRoleId: string; + onlyWhenUnassigned: boolean; }) { try { + if (onlyWhenUnassigned) { + const currentRole = await rbac.getUserRole({ userId, organizationId }); + if (currentRole !== null) { + return; + } + } + const roleResult = await rbac.setUserRole({ userId, organizationId, roleId: rbacRoleId, }); if (!roleResult.ok) { - logger.error("acceptInvite: skipped RBAC role assignment", { - organizationId, - userId, - rbacRoleId, - reason: roleResult.error, - }); + if (roleResult.code === "last_owner") { + logger.info("acceptInvite: kept last Owner, skipped RBAC role assignment", { + organizationId, + userId, + rbacRoleId, + }); + } else { + logger.warn("acceptInvite: skipped RBAC role assignment", { + organizationId, + userId, + rbacRoleId, + reason: roleResult.error, + }); + } } } catch (error) { logger.error("acceptInvite: RBAC role assignment threw", { @@ -415,6 +451,8 @@ export async function acceptInvite({ }, }); + let membershipCreated = false; + if (!member) { try { member = await prisma.orgMember.create({ @@ -424,6 +462,7 @@ export async function acceptInvite({ role: invite.role, }, }); + membershipCreated = true; } catch (error) { if ( error instanceof PrismaNamespace.PrismaClientKnownRequestError && @@ -473,16 +512,12 @@ export async function acceptInvite({ const remainingInvites = await getUsersInvites({ email: user.email }); - // If the invite carried an explicit RBAC role, assign it. Best-effort: the - // invite is already consumed and membership created above, so a failure here - // — a returned {ok:false} or a thrown error from the plugin — must not block - // joining the org. Swallow and log either way; without the catch a plugin - // throw escapes and turns the whole invite-accept into a 400. if (invite.rbacRoleId) { await assignInviteRbacRole({ userId: user.id, organizationId: invite.organization.id, rbacRoleId: invite.rbacRoleId, + onlyWhenUnassigned: !membershipCreated, }); } diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.invite/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.invite/route.tsx index 6a5a0e7a010..b6fe0b09c68 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.invite/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.invite/route.tsx @@ -28,7 +28,7 @@ import { $replica } from "~/db.server"; import { env } from "~/env.server"; import { useOrganization } from "~/hooks/useOrganizations"; import { inviteMembers } from "~/models/member.server"; -import { redirectWithSuccessMessage } from "~/models/message.server"; +import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/message.server"; import { resolveOrgIdFromSlug } from "~/models/organization.server"; import { TeamPresenter } from "~/presenters/TeamPresenter.server"; import { scheduleEmail } from "~/services/scheduleEmail.server"; @@ -127,6 +127,20 @@ const schema = z.object({ rbacRoleId: z.string().optional(), }); +function describeSkippedInvites(alreadyMembers: string[], alreadyInvited: string[]) { + const parts: string[] = []; + + if (alreadyMembers.length > 0) { + parts.push(simplur`${alreadyMembers.length} already [a member|members] of this organization`); + } + + if (alreadyInvited.length > 0) { + parts.push(simplur`${alreadyInvited.length} already invited`); + } + + return parts.join(" and "); +} + export const action = dashboardAction( { params: Params, @@ -201,7 +215,11 @@ export const action = dashboardAction( } try { - const invites = await inviteMembers({ + const { + created: invites, + alreadyMembers, + alreadyInvited, + } = await inviteMembers({ slug: organizationSlug, emails: submission.value.emails, userId, @@ -224,10 +242,19 @@ export const action = dashboardAction( } } + const teamPath = organizationTeamPath({ slug: organizationSlug }); + const skipped = describeSkippedInvites(alreadyMembers, alreadyInvited); + + if (invites.length === 0) { + return redirectWithErrorMessage(teamPath, request, `No invitations sent: ${skipped}.`); + } + return redirectWithSuccessMessage( - organizationTeamPath(invites[0].organization), + teamPath, request, - simplur`${submission.value.emails.length} member[|s] invited` + skipped + ? simplur`${invites.length} member[|s] invited. Skipped ${skipped}.` + : simplur`${invites.length} member[|s] invited` ); } catch (error: any) { return json({ errors: { body: error.message } }, { status: 400 }); diff --git a/apps/webapp/app/routes/api.v1.orgs.$orgParam.invites.ts b/apps/webapp/app/routes/api.v1.orgs.$orgParam.invites.ts index 76aeeb171b7..36e56e060eb 100644 --- a/apps/webapp/app/routes/api.v1.orgs.$orgParam.invites.ts +++ b/apps/webapp/app/routes/api.v1.orgs.$orgParam.invites.ts @@ -57,9 +57,7 @@ export const action = createActionPATApiRoute( return json({ error: "Membership is managed by Directory Sync" }, { status: 403 }); } - // Returns only the invites created by this call; already-invited emails are - // skipped (re-sending is the dashboard's dedicated resend flow, not this). - const created = await inviteMembers({ + const { created, alreadyMembers, alreadyInvited } = await inviteMembers({ slug: organization.slug, emails: body.emails, userId: authentication.userId, @@ -85,13 +83,11 @@ export const action = createActionPATApiRoute( // Report per-email outcome so callers aren't misled by an empty list on // re-invite. 201 when something was created, 200 when everything already // existed. - const createdEmails = new Set(created.map((invite) => invite.email)); - const alreadyInvited = [...new Set(body.emails)].filter((email) => !createdEmails.has(email)); - return json( { invited: created.map((invite) => ({ id: invite.id, email: invite.email })), alreadyInvited, + alreadyMembers, }, { status: created.length > 0 ? 201 : 200 } ); diff --git a/apps/webapp/test/member.server.test.ts b/apps/webapp/test/member.server.test.ts index 8d4a9128642..722c49aff18 100644 --- a/apps/webapp/test/member.server.test.ts +++ b/apps/webapp/test/member.server.test.ts @@ -1,31 +1,60 @@ import { randomBytes } from "node:crypto"; -import { describe, expect, vi } from "vitest"; +import { beforeEach, describe, expect, vi } from "vitest"; import type { PrismaClient } from "@trigger.dev/database"; const prismaHolder = vi.hoisted(() => ({ client: null as PrismaClient | null, })); +type SetUserRoleParams = { userId: string; organizationId: string; roleId: string }; +type SetUserRoleResult = { ok: true } | { ok: false; error: string; code?: "last_owner" }; +type CurrentRole = { id: string } | null; + +const rbacHolder = vi.hoisted(() => ({ + setUserRoleCalls: [] as SetUserRoleParams[], + setUserRoleResult: { ok: true } as SetUserRoleResult, + currentRole: null as CurrentRole, +})); + vi.mock("~/services/rbac.server", () => ({ rbac: { - setUserRole: async () => ({ ok: true as const }), + getUserRole: async () => rbacHolder.currentRole, + setUserRole: async (params: SetUserRoleParams) => { + rbacHolder.setUserRoleCalls.push(params); + return rbacHolder.setUserRoleResult; + }, }, })); -vi.mock("~/db.server", () => ({ - get prisma() { - if (!prismaHolder.client) { - throw new Error("test prisma not set"); - } - return prismaHolder.client; - }, - get $replica() { - if (!prismaHolder.client) { - throw new Error("test prisma not set"); - } - return prismaHolder.client; - }, -})); +function resetRbac(result: SetUserRoleResult = { ok: true }, currentRole: CurrentRole = null) { + rbacHolder.setUserRoleCalls.length = 0; + rbacHolder.setUserRoleResult = result; + rbacHolder.currentRole = currentRole; +} + +beforeEach(() => { + resetRbac(); +}); + +vi.mock("~/db.server", async () => { + const { Prisma } = await import("@trigger.dev/database"); + + return { + Prisma, + get prisma() { + if (!prismaHolder.client) { + throw new Error("test prisma not set"); + } + return prismaHolder.client; + }, + get $replica() { + if (!prismaHolder.client) { + throw new Error("test prisma not set"); + } + return prismaHolder.client; + }, + }; +}); import { postgresTest } from "@internal/testcontainers"; @@ -39,7 +68,7 @@ function randomHex(len = 12): string { async function seedInviteFixture( prisma: PrismaClient, - opts: { activeProjectCount: number; deletedProjectCount?: number } + opts: { activeProjectCount: number; deletedProjectCount?: number; rbacRoleId?: string } ) { const suffix = randomHex(8); const inviter = await prisma.user.create({ @@ -99,6 +128,7 @@ async function seedInviteFixture( organizationId: organization.id, inviterId: inviter.id, role: "MEMBER", + rbacRoleId: opts.rbacRoleId ?? null, }, }); @@ -253,6 +283,205 @@ describe("acceptInvite", () => { expect(member).toBeNull(); } ); + + postgresTest( + "applies the invite RBAC role when it creates the membership", + { timeout: 60_000 }, + async ({ prisma }) => { + prismaHolder.client = prisma; + const { acceptInvite } = await import("../app/models/member.server"); + + const { invitee, organization, invite } = await seedInviteFixture(prisma, { + activeProjectCount: 1, + rbacRoleId: "role_admin", + }); + + await acceptInvite({ + inviteId: invite.id, + organizationId: organization.id, + user: { id: invitee.id, email: invitee.email }, + }); + + expect(rbacHolder.setUserRoleCalls).toEqual([ + { userId: invitee.id, organizationId: organization.id, roleId: "role_admin" }, + ]); + } + ); + + postgresTest( + "does not apply the invite RBAC role to a member who already has a role", + { timeout: 60_000 }, + async ({ prisma }) => { + prismaHolder.client = prisma; + resetRbac({ ok: true }, { id: "role_owner" }); + const { acceptInvite } = await import("../app/models/member.server"); + + const { invitee, organization, invite } = await seedInviteFixture(prisma, { + activeProjectCount: 1, + rbacRoleId: "role_admin", + }); + + await prisma.orgMember.create({ + data: { + organizationId: organization.id, + userId: invitee.id, + role: "MEMBER", + }, + }); + + await acceptInvite({ + inviteId: invite.id, + organizationId: organization.id, + user: { id: invitee.id, email: invitee.email }, + }); + + expect(rbacHolder.setUserRoleCalls).toEqual([]); + + const remainingInvite = await prisma.orgMemberInvite.findFirst({ + where: { id: invite.id }, + }); + expect(remainingInvite).toBeNull(); + + const member = await prisma.orgMember.findFirst({ + where: { userId: invitee.id, organizationId: organization.id }, + }); + expect(member).not.toBeNull(); + } + ); + + postgresTest( + "applies the invite RBAC role to an existing member who has no role assigned", + { timeout: 60_000 }, + async ({ prisma }) => { + prismaHolder.client = prisma; + resetRbac({ ok: true }, null); + const { acceptInvite } = await import("../app/models/member.server"); + + const { invitee, organization, invite } = await seedInviteFixture(prisma, { + activeProjectCount: 1, + rbacRoleId: "role_admin", + }); + + await prisma.orgMember.create({ + data: { + organizationId: organization.id, + userId: invitee.id, + role: "MEMBER", + }, + }); + + await acceptInvite({ + inviteId: invite.id, + organizationId: organization.id, + user: { id: invitee.id, email: invitee.email }, + }); + + expect(rbacHolder.setUserRoleCalls).toEqual([ + { userId: invitee.id, organizationId: organization.id, roleId: "role_admin" }, + ]); + } + ); + + postgresTest( + "still joins the org when the RBAC role assignment is refused", + { timeout: 60_000 }, + async ({ prisma }) => { + prismaHolder.client = prisma; + resetRbac({ + ok: false, + error: "An organisation must have at least one Owner", + code: "last_owner", + }); + const { acceptInvite } = await import("../app/models/member.server"); + + const { invitee, organization, invite } = await seedInviteFixture(prisma, { + activeProjectCount: 1, + rbacRoleId: "role_admin", + }); + + const { organization: joinedOrg } = await acceptInvite({ + inviteId: invite.id, + organizationId: organization.id, + user: { id: invitee.id, email: invitee.email }, + }); + + expect(joinedOrg.id).toBe(organization.id); + expect(rbacHolder.setUserRoleCalls).toHaveLength(1); + + const member = await prisma.orgMember.findFirst({ + where: { userId: invitee.id, organizationId: organization.id }, + }); + expect(member).not.toBeNull(); + } + ); +}); + +describe("inviteMembers", () => { + postgresTest( + "skips emails that already belong to a member of the org", + { timeout: 60_000 }, + async ({ prisma }) => { + prismaHolder.client = prisma; + const { inviteMembers } = await import("../app/models/member.server"); + + const { inviter, invitee, organization, invite } = await seedInviteFixture(prisma, { + activeProjectCount: 0, + }); + + await prisma.orgMemberInvite.delete({ where: { id: invite.id } }); + + await prisma.orgMember.create({ + data: { + organizationId: organization.id, + userId: invitee.id, + role: "MEMBER", + }, + }); + + const newcomerEmail = `newcomer-${randomHex(8)}@test.local`; + + const { created, alreadyMembers, alreadyInvited } = await inviteMembers({ + slug: organization.slug, + emails: [invitee.email, newcomerEmail], + userId: inviter.id, + }); + + expect(created.map((row) => row.email)).toEqual([newcomerEmail]); + expect(alreadyMembers).toEqual([invitee.email]); + expect(alreadyInvited).toEqual([]); + + const invites = await prisma.orgMemberInvite.findMany({ + where: { organizationId: organization.id }, + select: { email: true }, + }); + expect(invites.map((pending) => pending.email)).toEqual([newcomerEmail]); + } + ); + + postgresTest( + "reports an email with a pending invitation as already invited", + { timeout: 60_000 }, + async ({ prisma }) => { + prismaHolder.client = prisma; + const { inviteMembers } = await import("../app/models/member.server"); + + const { inviter, organization, invite } = await seedInviteFixture(prisma, { + activeProjectCount: 0, + }); + + const newcomerEmail = `newcomer-${randomHex(8)}@test.local`; + + const { created, alreadyMembers, alreadyInvited } = await inviteMembers({ + slug: organization.slug, + emails: [invite.email, newcomerEmail], + userId: inviter.id, + }); + + expect(created.map((row) => row.email)).toEqual([newcomerEmail]); + expect(alreadyInvited).toEqual([invite.email]); + expect(alreadyMembers).toEqual([]); + } + ); }); describe("provisionMemberDevelopmentEnvironments", () => {