diff --git a/apps/api/src/people/people-invite.service.spec.ts b/apps/api/src/people/people-invite.service.spec.ts index c35c4bf780..f8fc9ce542 100644 --- a/apps/api/src/people/people-invite.service.spec.ts +++ b/apps/api/src/people/people-invite.service.spec.ts @@ -356,6 +356,97 @@ describe('PeopleInviteService', () => { ); }); + // Regression (CS): promoting an EXISTING ACTIVE member must upgrade their + // role in place. Previously an active member's role was left unchanged, so + // adding admin to an existing employee never granted app access and the + // user hit "Access Denied" after accepting. + it('upgrades an existing active member in place when promoted (no invitation, no email)', async () => { + (mockDb.user.findFirst as jest.Mock).mockResolvedValue({ + id: 'user_existing', + email: 'zub@example.com', + }); + (mockDb.member.findFirst as jest.Mock).mockResolvedValue({ + id: 'member_existing', + role: 'employee', + deactivated: false, + isActive: true, + }); + (mockDb.member.update as jest.Mock).mockResolvedValue({ + id: 'member_existing', + }); + + const results = await service.inviteMembers({ + ...baseParams, + invites: [{ email: 'zub@example.com', roles: ['admin', 'employee'] }], + }); + + expect(results[0].success).toBe(true); + // Role is unioned (sorted, de-duped) onto the existing membership. + expect(mockDb.member.update).toHaveBeenCalledWith( + expect.objectContaining({ + where: { id: 'member_existing' }, + data: { role: 'admin,employee' }, + }), + ); + // An already-active member is upgraded directly — no re-invitation, no email. + expect(mockDb.invitation.create).not.toHaveBeenCalled(); + expect(mockTriggerEmail).not.toHaveBeenCalled(); + }); + + it('does not rewrite an active member who already holds the invited roles', async () => { + (mockDb.user.findFirst as jest.Mock).mockResolvedValue({ + id: 'user_existing', + email: 'a@example.com', + }); + (mockDb.member.findFirst as jest.Mock).mockResolvedValue({ + id: 'member_existing', + role: 'admin,employee', + deactivated: false, + isActive: true, + }); + + const results = await service.inviteMembers({ + ...baseParams, + invites: [{ email: 'a@example.com', roles: ['admin'] }], + }); + + expect(results[0].success).toBe(true); + expect(mockDb.member.update).not.toHaveBeenCalled(); + expect(mockDb.invitation.create).not.toHaveBeenCalled(); + }); + + it('unions roles for an active member re-added via the employee path', async () => { + (mockDb.organization.findUnique as jest.Mock).mockResolvedValue({ + name: 'Test Org', + }); + (mockDb.user.findFirst as jest.Mock).mockResolvedValue({ + id: 'user_existing', + email: 'c@example.com', + }); + (mockDb.member.findFirst as jest.Mock).mockResolvedValue({ + id: 'member_existing', + role: 'contractor', + deactivated: false, + isActive: true, + }); + (mockDb.member.update as jest.Mock).mockResolvedValue({ + id: 'member_existing', + }); + + const results = await service.inviteMembers({ + ...baseParams, + invites: [{ email: 'c@example.com', roles: ['employee'] }], + }); + + expect(results[0].success).toBe(true); + expect(mockDb.member.update).toHaveBeenCalledWith( + expect.objectContaining({ + where: { id: 'member_existing' }, + data: { role: 'contractor,employee' }, + }), + ); + }); + it('should handle multiple invites', async () => { (mockDb.organization.findUnique as jest.Mock).mockResolvedValue({ name: 'Test Org', diff --git a/apps/api/src/people/people-invite.service.ts b/apps/api/src/people/people-invite.service.ts index 0ed1f2586a..ef5feab296 100644 --- a/apps/api/src/people/people-invite.service.ts +++ b/apps/api/src/people/people-invite.service.ts @@ -193,7 +193,17 @@ export class PeopleInviteService { data: { deactivated: false, isActive: true, role: roleString }, }); } else { - member = existingMember; + // Active member re-added: union the new roles into their existing roles + // so we never strip a role they already have, and so adding a role + // actually takes effect instead of silently no-op'ing. + const mergedRole = this.mergeRoleString(existingMember.role, roles); + member = + mergedRole === this.normalizeRoleString(existingMember.role) + ? existingMember + : await db.member.update({ + where: { id: existingMember.id }, + data: { role: mergedRole }, + }); } } else { member = await db.member.create({ @@ -276,14 +286,18 @@ export class PeopleInviteService { return; } - await this.sendInvitationEmailToExistingMember({ - email, - roles, - organizationId, - inviterId: currentUserId, - sendPortalEmail, - sendAppEmail, - }); + // Already an active member: an invitation/accept round-trip can't grant + // new roles to someone who is already in the org (and historically left + // their role unchanged, so promoting an employee to admin silently + // failed and the user hit "Access Denied"). Upgrade their role in place + // by unioning the new roles into their existing roles. + const mergedRole = this.mergeRoleString(existingMember.role, roles); + if (mergedRole !== this.normalizeRoleString(existingMember.role)) { + await db.member.update({ + where: { id: existingMember.id }, + data: { role: mergedRole }, + }); + } return; } } @@ -319,51 +333,36 @@ export class PeopleInviteService { }); } - private async sendInvitationEmailToExistingMember(params: { - email: string; - roles: string[]; - organizationId: string; - inviterId: string; - sendPortalEmail?: boolean; - sendAppEmail?: boolean; - }): Promise { - const { - email, - roles, - organizationId, - inviterId, - sendPortalEmail, - sendAppEmail, - } = params; - - const organization = await db.organization.findUnique({ - where: { id: organizationId }, - select: { name: true }, - }); - - if (!organization) { - throw new BadRequestException('Organization not found.'); - } - - const invitation = await db.invitation.create({ - data: { - email: email.toLowerCase(), - organizationId, - role: roles.length === 1 ? roles[0] : roles.join(','), - status: 'pending', - expiresAt: new Date(Date.now() + 7 * 24 * 60 * 60 * 1000), - inviterId, - }, - }); + /** Sort + de-dupe a comma-separated role string into a canonical form. */ + private normalizeRoleString(role: string | null | undefined): string { + return [ + ...new Set( + (role ?? '') + .split(',') + .map((r) => r.trim()) + .filter(Boolean), + ), + ] + .sort() + .join(','); + } - await this.sendInviteEmails({ - email: email.toLowerCase(), - organizationName: organization.name, - sendPortalEmail, - sendAppEmail, - portalLink: this.buildPortalUrl(organizationId), - appLink: this.buildInviteLink(invitation.id), - }); + /** Union new roles into an existing comma-separated role string. */ + private mergeRoleString( + existingRole: string | null | undefined, + addedRoles: string[], + ): string { + return [ + ...new Set([ + ...(existingRole ?? '') + .split(',') + .map((r) => r.trim()) + .filter(Boolean), + ...addedRoles.map((r) => r.trim()).filter(Boolean), + ]), + ] + .sort() + .join(','); } async resendPortalInvite(params: { diff --git a/apps/app/src/actions/organization/accept-invitation.ts b/apps/app/src/actions/organization/accept-invitation.ts index ce7d7f98c7..6ea927fa0d 100644 --- a/apps/app/src/actions/organization/accept-invitation.ts +++ b/apps/app/src/actions/organization/accept-invitation.ts @@ -1,6 +1,7 @@ 'use server'; import { createTrainingVideoEntries } from '@/lib/db/employee'; +import { mergeRoleStrings, normalizeRoleString } from '@/lib/permissions'; import { auth } from '@/utils/auth'; import { db } from '@db/server'; import { revalidatePath, revalidateTag } from 'next/cache'; @@ -74,8 +75,14 @@ export const completeInvitation = authActionClientWithoutOrg }); if (existingMembership) { - // Reactivate member before setting active org, since better-auth - // validates membership status when setting the active organization. + // Ensure the member ends up with at least the invited roles. + // Reactivate first since better-auth validates membership status when + // setting the active organization. + // - Deactivated members are reactivated with the invited roles. + // - Active members have the invited roles UNIONed into their existing + // roles. Previously this branch left an active member's role + // untouched, so promoting e.g. an employee to admin via an invite + // never granted app access and the user hit "Access Denied". if (existingMembership.deactivated) { await db.member.update({ where: { id: existingMembership.id }, @@ -84,6 +91,17 @@ export const completeInvitation = authActionClientWithoutOrg role: invitation.role, }, }); + } else { + const mergedRole = mergeRoleStrings( + existingMembership.role, + invitation.role, + ); + if (mergedRole !== normalizeRoleString(existingMembership.role)) { + await db.member.update({ + where: { id: existingMembership.id }, + data: { role: mergedRole }, + }); + } } if (ctx.session.activeOrganizationId !== invitation.organizationId) { diff --git a/apps/app/src/lib/permissions.test.ts b/apps/app/src/lib/permissions.test.ts index 180db8bf85..b0a4790cad 100644 --- a/apps/app/src/lib/permissions.test.ts +++ b/apps/app/src/lib/permissions.test.ts @@ -6,6 +6,8 @@ import { canAccessRoute, getDefaultRoute, hasPermission, + mergeRoleStrings, + normalizeRoleString, type UserPermissions, } from './permissions'; @@ -48,6 +50,47 @@ describe('canAccessApp', () => { }); }); +describe('normalizeRoleString', () => { + it('sorts and de-dupes a role string', () => { + expect(normalizeRoleString('employee,admin,employee')).toBe('admin,employee'); + }); + + it('trims whitespace and ignores empty segments', () => { + expect(normalizeRoleString(' admin , , employee ')).toBe('admin,employee'); + }); + + it('returns empty string for null/undefined/empty', () => { + expect(normalizeRoleString(null)).toBe(''); + expect(normalizeRoleString(undefined)).toBe(''); + expect(normalizeRoleString('')).toBe(''); + }); +}); + +describe('mergeRoleStrings', () => { + it('unions invited roles into existing roles (employee promoted to admin)', () => { + expect(mergeRoleStrings('employee', 'admin')).toBe('admin,employee'); + }); + + it('preserves existing roles not present in the invite (never strips a role)', () => { + // An owner re-invited as employee keeps owner. + expect(mergeRoleStrings('owner', 'employee')).toBe('employee,owner'); + }); + + it('is a no-op (set-equal) when the member already holds the invited roles', () => { + const existing = 'admin,employee'; + expect(mergeRoleStrings(existing, 'admin')).toBe(normalizeRoleString(existing)); + expect(mergeRoleStrings(existing, 'employee,admin')).toBe(normalizeRoleString(existing)); + }); + + it('handles multi-role invite strings and de-dupes', () => { + expect(mergeRoleStrings('employee', 'admin,employee')).toBe('admin,employee'); + }); + + it('handles a missing existing role (acts like the invited roles)', () => { + expect(mergeRoleStrings(null, 'admin,employee')).toBe('admin,employee'); + }); +}); + describe('canAccessRoute', () => { it('allows access to penetration-tests with pentest:read', () => { const permissions: UserPermissions = { pentest: ['read'] }; diff --git a/apps/app/src/lib/permissions.ts b/apps/app/src/lib/permissions.ts index 761764fe41..8780d29358 100644 --- a/apps/app/src/lib/permissions.ts +++ b/apps/app/src/lib/permissions.ts @@ -21,6 +21,25 @@ export function parseRolesString(rolesStr: string | null | undefined): string[] .filter((r) => r.length > 0); } +/** Sort + de-dupe a comma-separated role string into a canonical form. */ +export function normalizeRoleString(rolesStr: string | null | undefined): string { + return [...new Set(parseRolesString(rolesStr))].sort().join(','); +} + +/** + * Union the invited roles into an existing comma-separated role string. + * Used when an existing member is (re-)invited: we add the new roles rather + * than replacing, so a member is never stripped of a role they already hold. + */ +export function mergeRoleStrings( + existingRoles: string | null | undefined, + invitedRoles: string | null | undefined, +): string { + return [...new Set([...parseRolesString(existingRoles), ...parseRolesString(invitedRoles)])] + .sort() + .join(','); +} + /** * Check if a role name is a built-in role. */