Skip to content

Commit 097364b

Browse files
authored
Merge pull request #3291 from trycompai/main
[comp] Production Deploy
2 parents 896e4ef + 86e4354 commit 097364b

5 files changed

Lines changed: 225 additions & 55 deletions

File tree

‎apps/api/src/people/people-invite.service.spec.ts‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,97 @@ describe('PeopleInviteService', () => {
356356
);
357357
});
358358

359+
// Regression (CS): promoting an EXISTING ACTIVE member must upgrade their
360+
// role in place. Previously an active member's role was left unchanged, so
361+
// adding admin to an existing employee never granted app access and the
362+
// user hit "Access Denied" after accepting.
363+
it('upgrades an existing active member in place when promoted (no invitation, no email)', async () => {
364+
(mockDb.user.findFirst as jest.Mock).mockResolvedValue({
365+
id: 'user_existing',
366+
email: 'zub@example.com',
367+
});
368+
(mockDb.member.findFirst as jest.Mock).mockResolvedValue({
369+
id: 'member_existing',
370+
role: 'employee',
371+
deactivated: false,
372+
isActive: true,
373+
});
374+
(mockDb.member.update as jest.Mock).mockResolvedValue({
375+
id: 'member_existing',
376+
});
377+
378+
const results = await service.inviteMembers({
379+
...baseParams,
380+
invites: [{ email: 'zub@example.com', roles: ['admin', 'employee'] }],
381+
});
382+
383+
expect(results[0].success).toBe(true);
384+
// Role is unioned (sorted, de-duped) onto the existing membership.
385+
expect(mockDb.member.update).toHaveBeenCalledWith(
386+
expect.objectContaining({
387+
where: { id: 'member_existing' },
388+
data: { role: 'admin,employee' },
389+
}),
390+
);
391+
// An already-active member is upgraded directly — no re-invitation, no email.
392+
expect(mockDb.invitation.create).not.toHaveBeenCalled();
393+
expect(mockTriggerEmail).not.toHaveBeenCalled();
394+
});
395+
396+
it('does not rewrite an active member who already holds the invited roles', async () => {
397+
(mockDb.user.findFirst as jest.Mock).mockResolvedValue({
398+
id: 'user_existing',
399+
email: 'a@example.com',
400+
});
401+
(mockDb.member.findFirst as jest.Mock).mockResolvedValue({
402+
id: 'member_existing',
403+
role: 'admin,employee',
404+
deactivated: false,
405+
isActive: true,
406+
});
407+
408+
const results = await service.inviteMembers({
409+
...baseParams,
410+
invites: [{ email: 'a@example.com', roles: ['admin'] }],
411+
});
412+
413+
expect(results[0].success).toBe(true);
414+
expect(mockDb.member.update).not.toHaveBeenCalled();
415+
expect(mockDb.invitation.create).not.toHaveBeenCalled();
416+
});
417+
418+
it('unions roles for an active member re-added via the employee path', async () => {
419+
(mockDb.organization.findUnique as jest.Mock).mockResolvedValue({
420+
name: 'Test Org',
421+
});
422+
(mockDb.user.findFirst as jest.Mock).mockResolvedValue({
423+
id: 'user_existing',
424+
email: 'c@example.com',
425+
});
426+
(mockDb.member.findFirst as jest.Mock).mockResolvedValue({
427+
id: 'member_existing',
428+
role: 'contractor',
429+
deactivated: false,
430+
isActive: true,
431+
});
432+
(mockDb.member.update as jest.Mock).mockResolvedValue({
433+
id: 'member_existing',
434+
});
435+
436+
const results = await service.inviteMembers({
437+
...baseParams,
438+
invites: [{ email: 'c@example.com', roles: ['employee'] }],
439+
});
440+
441+
expect(results[0].success).toBe(true);
442+
expect(mockDb.member.update).toHaveBeenCalledWith(
443+
expect.objectContaining({
444+
where: { id: 'member_existing' },
445+
data: { role: 'contractor,employee' },
446+
}),
447+
);
448+
});
449+
359450
it('should handle multiple invites', async () => {
360451
(mockDb.organization.findUnique as jest.Mock).mockResolvedValue({
361452
name: 'Test Org',

‎apps/api/src/people/people-invite.service.ts‎

Lines changed: 52 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,17 @@ export class PeopleInviteService {
193193
data: { deactivated: false, isActive: true, role: roleString },
194194
});
195195
} else {
196-
member = existingMember;
196+
// Active member re-added: union the new roles into their existing roles
197+
// so we never strip a role they already have, and so adding a role
198+
// actually takes effect instead of silently no-op'ing.
199+
const mergedRole = this.mergeRoleString(existingMember.role, roles);
200+
member =
201+
mergedRole === this.normalizeRoleString(existingMember.role)
202+
? existingMember
203+
: await db.member.update({
204+
where: { id: existingMember.id },
205+
data: { role: mergedRole },
206+
});
197207
}
198208
} else {
199209
member = await db.member.create({
@@ -276,14 +286,18 @@ export class PeopleInviteService {
276286
return;
277287
}
278288

279-
await this.sendInvitationEmailToExistingMember({
280-
email,
281-
roles,
282-
organizationId,
283-
inviterId: currentUserId,
284-
sendPortalEmail,
285-
sendAppEmail,
286-
});
289+
// Already an active member: an invitation/accept round-trip can't grant
290+
// new roles to someone who is already in the org (and historically left
291+
// their role unchanged, so promoting an employee to admin silently
292+
// failed and the user hit "Access Denied"). Upgrade their role in place
293+
// by unioning the new roles into their existing roles.
294+
const mergedRole = this.mergeRoleString(existingMember.role, roles);
295+
if (mergedRole !== this.normalizeRoleString(existingMember.role)) {
296+
await db.member.update({
297+
where: { id: existingMember.id },
298+
data: { role: mergedRole },
299+
});
300+
}
287301
return;
288302
}
289303
}
@@ -319,51 +333,36 @@ export class PeopleInviteService {
319333
});
320334
}
321335

322-
private async sendInvitationEmailToExistingMember(params: {
323-
email: string;
324-
roles: string[];
325-
organizationId: string;
326-
inviterId: string;
327-
sendPortalEmail?: boolean;
328-
sendAppEmail?: boolean;
329-
}): Promise<void> {
330-
const {
331-
email,
332-
roles,
333-
organizationId,
334-
inviterId,
335-
sendPortalEmail,
336-
sendAppEmail,
337-
} = params;
338-
339-
const organization = await db.organization.findUnique({
340-
where: { id: organizationId },
341-
select: { name: true },
342-
});
343-
344-
if (!organization) {
345-
throw new BadRequestException('Organization not found.');
346-
}
347-
348-
const invitation = await db.invitation.create({
349-
data: {
350-
email: email.toLowerCase(),
351-
organizationId,
352-
role: roles.length === 1 ? roles[0] : roles.join(','),
353-
status: 'pending',
354-
expiresAt: new Date(Date.now() + 7 * 24 * 60 * 60 * 1000),
355-
inviterId,
356-
},
357-
});
336+
/** Sort + de-dupe a comma-separated role string into a canonical form. */
337+
private normalizeRoleString(role: string | null | undefined): string {
338+
return [
339+
...new Set(
340+
(role ?? '')
341+
.split(',')
342+
.map((r) => r.trim())
343+
.filter(Boolean),
344+
),
345+
]
346+
.sort()
347+
.join(',');
348+
}
358349

359-
await this.sendInviteEmails({
360-
email: email.toLowerCase(),
361-
organizationName: organization.name,
362-
sendPortalEmail,
363-
sendAppEmail,
364-
portalLink: this.buildPortalUrl(organizationId),
365-
appLink: this.buildInviteLink(invitation.id),
366-
});
350+
/** Union new roles into an existing comma-separated role string. */
351+
private mergeRoleString(
352+
existingRole: string | null | undefined,
353+
addedRoles: string[],
354+
): string {
355+
return [
356+
...new Set([
357+
...(existingRole ?? '')
358+
.split(',')
359+
.map((r) => r.trim())
360+
.filter(Boolean),
361+
...addedRoles.map((r) => r.trim()).filter(Boolean),
362+
]),
363+
]
364+
.sort()
365+
.join(',');
367366
}
368367

369368
async resendPortalInvite(params: {

‎apps/app/src/actions/organization/accept-invitation.ts‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use server';
22

33
import { createTrainingVideoEntries } from '@/lib/db/employee';
4+
import { mergeRoleStrings, normalizeRoleString } from '@/lib/permissions';
45
import { auth } from '@/utils/auth';
56
import { db } from '@db/server';
67
import { revalidatePath, revalidateTag } from 'next/cache';
@@ -74,8 +75,14 @@ export const completeInvitation = authActionClientWithoutOrg
7475
});
7576

7677
if (existingMembership) {
77-
// Reactivate member before setting active org, since better-auth
78-
// validates membership status when setting the active organization.
78+
// Ensure the member ends up with at least the invited roles.
79+
// Reactivate first since better-auth validates membership status when
80+
// setting the active organization.
81+
// - Deactivated members are reactivated with the invited roles.
82+
// - Active members have the invited roles UNIONed into their existing
83+
// roles. Previously this branch left an active member's role
84+
// untouched, so promoting e.g. an employee to admin via an invite
85+
// never granted app access and the user hit "Access Denied".
7986
if (existingMembership.deactivated) {
8087
await db.member.update({
8188
where: { id: existingMembership.id },
@@ -84,6 +91,17 @@ export const completeInvitation = authActionClientWithoutOrg
8491
role: invitation.role,
8592
},
8693
});
94+
} else {
95+
const mergedRole = mergeRoleStrings(
96+
existingMembership.role,
97+
invitation.role,
98+
);
99+
if (mergedRole !== normalizeRoleString(existingMembership.role)) {
100+
await db.member.update({
101+
where: { id: existingMembership.id },
102+
data: { role: mergedRole },
103+
});
104+
}
87105
}
88106

89107
if (ctx.session.activeOrganizationId !== invitation.organizationId) {

‎apps/app/src/lib/permissions.test.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import {
66
canAccessRoute,
77
getDefaultRoute,
88
hasPermission,
9+
mergeRoleStrings,
10+
normalizeRoleString,
911
type UserPermissions,
1012
} from './permissions';
1113

@@ -48,6 +50,47 @@ describe('canAccessApp', () => {
4850
});
4951
});
5052

53+
describe('normalizeRoleString', () => {
54+
it('sorts and de-dupes a role string', () => {
55+
expect(normalizeRoleString('employee,admin,employee')).toBe('admin,employee');
56+
});
57+
58+
it('trims whitespace and ignores empty segments', () => {
59+
expect(normalizeRoleString(' admin , , employee ')).toBe('admin,employee');
60+
});
61+
62+
it('returns empty string for null/undefined/empty', () => {
63+
expect(normalizeRoleString(null)).toBe('');
64+
expect(normalizeRoleString(undefined)).toBe('');
65+
expect(normalizeRoleString('')).toBe('');
66+
});
67+
});
68+
69+
describe('mergeRoleStrings', () => {
70+
it('unions invited roles into existing roles (employee promoted to admin)', () => {
71+
expect(mergeRoleStrings('employee', 'admin')).toBe('admin,employee');
72+
});
73+
74+
it('preserves existing roles not present in the invite (never strips a role)', () => {
75+
// An owner re-invited as employee keeps owner.
76+
expect(mergeRoleStrings('owner', 'employee')).toBe('employee,owner');
77+
});
78+
79+
it('is a no-op (set-equal) when the member already holds the invited roles', () => {
80+
const existing = 'admin,employee';
81+
expect(mergeRoleStrings(existing, 'admin')).toBe(normalizeRoleString(existing));
82+
expect(mergeRoleStrings(existing, 'employee,admin')).toBe(normalizeRoleString(existing));
83+
});
84+
85+
it('handles multi-role invite strings and de-dupes', () => {
86+
expect(mergeRoleStrings('employee', 'admin,employee')).toBe('admin,employee');
87+
});
88+
89+
it('handles a missing existing role (acts like the invited roles)', () => {
90+
expect(mergeRoleStrings(null, 'admin,employee')).toBe('admin,employee');
91+
});
92+
});
93+
5194
describe('canAccessRoute', () => {
5295
it('allows access to penetration-tests with pentest:read', () => {
5396
const permissions: UserPermissions = { pentest: ['read'] };

‎apps/app/src/lib/permissions.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,25 @@ export function parseRolesString(rolesStr: string | null | undefined): string[]
2121
.filter((r) => r.length > 0);
2222
}
2323

24+
/** Sort + de-dupe a comma-separated role string into a canonical form. */
25+
export function normalizeRoleString(rolesStr: string | null | undefined): string {
26+
return [...new Set(parseRolesString(rolesStr))].sort().join(',');
27+
}
28+
29+
/**
30+
* Union the invited roles into an existing comma-separated role string.
31+
* Used when an existing member is (re-)invited: we add the new roles rather
32+
* than replacing, so a member is never stripped of a role they already hold.
33+
*/
34+
export function mergeRoleStrings(
35+
existingRoles: string | null | undefined,
36+
invitedRoles: string | null | undefined,
37+
): string {
38+
return [...new Set([...parseRolesString(existingRoles), ...parseRolesString(invitedRoles)])]
39+
.sort()
40+
.join(',');
41+
}
42+
2443
/**
2544
* Check if a role name is a built-in role.
2645
*/

0 commit comments

Comments
 (0)