Skip to content

Commit 382f619

Browse files
Marfuenclaude
Marfuen
and
claude
authored
fix(sync): validate employee.role before persisting (limbo-role guard) (#2690)
* fix(sync): validate employee.role against known roles before persisting The generic employee sync service wrote employee.role straight into member.role with no validation. A misconfigured DSL (notably the Microsoft sync, which mapped Entra Graph's jobTitle into role) could plant strings like "Senior Front End Engineer" in member.role with no matching organization_role row. The Roles page correctly showed nothing for these "limbo" roles, but the People list rendered them as Badges (getRoleLabel falls back to title-case for unknown roles), and any RBAC check against them silently denied permissions because the value matched neither a built-in role nor a custom role. Sanitize employee.role before insert: each comma-separated token must be either a built-in role (BUILT_IN_ROLE_PERMISSIONS) or an existing organization_role.name in the same org. Unknown tokens are dropped; if every token is invalid (or role is empty), fall back to defaultRole. Log a warning when this happens so a misconfigured DSL is visible in sync logs instead of silently corrupting data. Note: this is a forward fix only. Customers with already-polluted member.role values need a one-time data scrub plus a fix to their DynamicIntegration.syncDefinition JSON in the database (separate operations on the DB side, not in this PR). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sync): self-heal existing members' limbo roles on re-sync The previous commit guarded *new* member creation against unknown role strings. This extends the same sanitizer to *existing* members the sync re-encounters, so customers who already have polluted member.role values get cleaned up automatically the next time they import — no manual DB scrub required. The heal is conservative: it only ever shrinks the role string (drops unknown tokens, falls back to defaultRole if every token is invalid). It never overwrites a member's currently-valid role with whatever the provider sent, so a manually-assigned admin won't get downgraded by a DSL still mis-mapping jobTitle. Both the "already a member" and the "reactivate" paths heal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 926a905 commit 382f619

2 files changed

Lines changed: 298 additions & 3 deletions

File tree

‎apps/api/src/integration-platform/services/generic-employee-sync.service.spec.ts‎

Lines changed: 244 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,244 @@
1+
import type { SyncEmployee } from '@trycompai/integration-platform';
2+
3+
const mockUserFindUnique = jest.fn();
4+
const mockUserCreate = jest.fn();
5+
const mockMemberFindFirst = jest.fn();
6+
const mockMemberCreate = jest.fn();
7+
const mockMemberFindMany = jest.fn();
8+
const mockMemberUpdate = jest.fn();
9+
const mockOrgRoleFindMany = jest.fn();
10+
11+
jest.mock('@trycompai/auth', () => ({
12+
BUILT_IN_ROLE_PERMISSIONS: {
13+
owner: {},
14+
admin: {},
15+
auditor: {},
16+
employee: {},
17+
contractor: {},
18+
},
19+
}));
20+
21+
jest.mock('@db', () => ({
22+
db: {
23+
user: {
24+
findUnique: (...args: unknown[]) => mockUserFindUnique(...args),
25+
create: (...args: unknown[]) => mockUserCreate(...args),
26+
},
27+
member: {
28+
findFirst: (...args: unknown[]) => mockMemberFindFirst(...args),
29+
create: (...args: unknown[]) => mockMemberCreate(...args),
30+
findMany: (...args: unknown[]) => mockMemberFindMany(...args),
31+
update: (...args: unknown[]) => mockMemberUpdate(...args),
32+
},
33+
organizationRole: {
34+
findMany: (...args: unknown[]) => mockOrgRoleFindMany(...args),
35+
},
36+
},
37+
}));
38+
39+
import { GenericEmployeeSyncService } from './generic-employee-sync.service';
40+
41+
describe('GenericEmployeeSyncService role validation', () => {
42+
let service: GenericEmployeeSyncService;
43+
44+
const baseEmployee = (
45+
overrides: Partial<SyncEmployee> = {},
46+
): SyncEmployee => ({
47+
email: 'new-hire@example.com',
48+
name: 'New Hire',
49+
status: 'active',
50+
...overrides,
51+
});
52+
53+
beforeEach(() => {
54+
jest.clearAllMocks();
55+
service = new GenericEmployeeSyncService();
56+
57+
// Default: user does not exist (will be created), no existing member,
58+
// no other org members (skip phase 2).
59+
mockUserFindUnique.mockResolvedValue(null);
60+
mockUserCreate.mockResolvedValue({
61+
id: 'user_1',
62+
email: 'new-hire@example.com',
63+
});
64+
mockMemberFindFirst.mockResolvedValue(null);
65+
mockMemberCreate.mockResolvedValue({ id: 'mem_1' });
66+
mockMemberFindMany.mockResolvedValue([]);
67+
mockOrgRoleFindMany.mockResolvedValue([]);
68+
});
69+
70+
it('persists a built-in role from the provider as-is', async () => {
71+
await service.processEmployees({
72+
organizationId: 'org_1',
73+
employees: [baseEmployee({ role: 'admin' })],
74+
});
75+
76+
expect(mockMemberCreate).toHaveBeenCalledWith(
77+
expect.objectContaining({
78+
data: expect.objectContaining({ role: 'admin' }),
79+
}),
80+
);
81+
});
82+
83+
it('persists a known custom role from the provider as-is', async () => {
84+
mockOrgRoleFindMany.mockResolvedValue([
85+
{ name: 'security-engineer' },
86+
]);
87+
88+
await service.processEmployees({
89+
organizationId: 'org_1',
90+
employees: [baseEmployee({ role: 'security-engineer' })],
91+
});
92+
93+
expect(mockMemberCreate).toHaveBeenCalledWith(
94+
expect.objectContaining({
95+
data: expect.objectContaining({ role: 'security-engineer' }),
96+
}),
97+
);
98+
});
99+
100+
it('falls back to defaultRole when provider sends an unknown role (e.g. Microsoft jobTitle)', async () => {
101+
await service.processEmployees({
102+
organizationId: 'org_1',
103+
employees: [baseEmployee({ role: 'Senior Front End Engineer' })],
104+
options: { defaultRole: 'employee' },
105+
});
106+
107+
expect(mockMemberCreate).toHaveBeenCalledWith(
108+
expect.objectContaining({
109+
data: expect.objectContaining({ role: 'employee' }),
110+
}),
111+
);
112+
});
113+
114+
it('falls back to defaultRole when provider sends an empty role', async () => {
115+
await service.processEmployees({
116+
organizationId: 'org_1',
117+
employees: [baseEmployee({ role: '' })],
118+
options: { defaultRole: 'contractor' },
119+
});
120+
121+
expect(mockMemberCreate).toHaveBeenCalledWith(
122+
expect.objectContaining({
123+
data: expect.objectContaining({ role: 'contractor' }),
124+
}),
125+
);
126+
});
127+
128+
it('looks up custom roles scoped to the org being synced', async () => {
129+
await service.processEmployees({
130+
organizationId: 'org_42',
131+
employees: [baseEmployee({ role: 'employee' })],
132+
});
133+
134+
expect(mockOrgRoleFindMany).toHaveBeenCalledWith(
135+
expect.objectContaining({
136+
where: { organizationId: 'org_42' },
137+
select: { name: true },
138+
}),
139+
);
140+
});
141+
142+
it('keeps only the valid tokens when role is comma-separated', async () => {
143+
await service.processEmployees({
144+
organizationId: 'org_1',
145+
employees: [baseEmployee({ role: 'admin,Senior Front End Engineer' })],
146+
options: { defaultRole: 'employee' },
147+
});
148+
149+
expect(mockMemberCreate).toHaveBeenCalledWith(
150+
expect.objectContaining({
151+
data: expect.objectContaining({ role: 'admin' }),
152+
}),
153+
);
154+
});
155+
156+
describe('limbo role self-heal on re-sync', () => {
157+
it('heals an existing member whose role is entirely invalid', async () => {
158+
mockUserFindUnique.mockResolvedValue({
159+
id: 'user_1',
160+
email: 'mc@example.com',
161+
});
162+
mockMemberFindFirst.mockResolvedValue({
163+
id: 'mem_1',
164+
role: 'Senior Front End Engineer',
165+
deactivated: false,
166+
});
167+
168+
await service.processEmployees({
169+
organizationId: 'org_1',
170+
employees: [baseEmployee({ email: 'mc@example.com', role: 'employee' })],
171+
options: { defaultRole: 'employee' },
172+
});
173+
174+
expect(mockMemberUpdate).toHaveBeenCalledWith({
175+
where: { id: 'mem_1' },
176+
data: { role: 'employee' },
177+
});
178+
});
179+
180+
it('heals an existing member whose role mixes valid + invalid tokens', async () => {
181+
mockUserFindUnique.mockResolvedValue({
182+
id: 'user_1',
183+
email: 'mc@example.com',
184+
});
185+
mockMemberFindFirst.mockResolvedValue({
186+
id: 'mem_1',
187+
role: 'admin,Senior Front End Engineer',
188+
deactivated: false,
189+
});
190+
191+
await service.processEmployees({
192+
organizationId: 'org_1',
193+
employees: [baseEmployee({ email: 'mc@example.com' })],
194+
});
195+
196+
expect(mockMemberUpdate).toHaveBeenCalledWith({
197+
where: { id: 'mem_1' },
198+
data: { role: 'admin' },
199+
});
200+
});
201+
202+
it('does not touch an existing member whose role is already valid', async () => {
203+
mockUserFindUnique.mockResolvedValue({
204+
id: 'user_1',
205+
email: 'mc@example.com',
206+
});
207+
mockMemberFindFirst.mockResolvedValue({
208+
id: 'mem_1',
209+
role: 'admin',
210+
deactivated: false,
211+
});
212+
213+
await service.processEmployees({
214+
organizationId: 'org_1',
215+
employees: [baseEmployee({ email: 'mc@example.com' })],
216+
});
217+
218+
expect(mockMemberUpdate).not.toHaveBeenCalled();
219+
});
220+
221+
it('heals AND reactivates a deactivated member with a limbo role', async () => {
222+
mockUserFindUnique.mockResolvedValue({
223+
id: 'user_1',
224+
email: 'mc@example.com',
225+
});
226+
mockMemberFindFirst.mockResolvedValue({
227+
id: 'mem_1',
228+
role: 'Senior Front End Engineer',
229+
deactivated: true,
230+
});
231+
232+
await service.processEmployees({
233+
organizationId: 'org_1',
234+
employees: [baseEmployee({ email: 'mc@example.com' })],
235+
options: { allowReactivation: true, defaultRole: 'employee' },
236+
});
237+
238+
expect(mockMemberUpdate).toHaveBeenCalledWith({
239+
where: { id: 'mem_1' },
240+
data: { deactivated: false, isActive: true, role: 'employee' },
241+
});
242+
});
243+
});
244+
});

‎apps/api/src/integration-platform/services/generic-employee-sync.service.ts‎

Lines changed: 54 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { Injectable, Logger } from '@nestjs/common';
22
import { db } from '@db';
33
import type { SyncEmployee } from '@trycompai/integration-platform';
4+
import { BUILT_IN_ROLE_PERMISSIONS } from '@trycompai/auth';
45

56
// ============================================================================
67
// Types
@@ -76,6 +77,28 @@ export class GenericEmployeeSyncService {
7677
const protectedRoles = options.protectedRoles ?? DEFAULT_PROTECTED_ROLES;
7778
const providerName = options.providerName ?? 'provider';
7879

80+
// Build the set of role identifiers we'll accept on this sync. Anything
81+
// outside this set is dropped (e.g. a Microsoft DSL that mis-maps
82+
// jobTitle into role would otherwise plant "Senior Front End Engineer"
83+
// strings into member.role with no matching organization_role row).
84+
const customRoles: { name: string }[] = await db.organizationRole.findMany({
85+
where: { organizationId },
86+
select: { name: true },
87+
});
88+
const validRoleNames = new Set<string>([
89+
...Object.keys(BUILT_IN_ROLE_PERMISSIONS),
90+
...customRoles.map((r) => r.name),
91+
]);
92+
93+
const sanitizeRole = (raw: string | undefined | null): string => {
94+
if (!raw) return defaultRole;
95+
const tokens = raw
96+
.split(',')
97+
.map((t) => t.trim())
98+
.filter((t) => t.length > 0 && validRoleNames.has(t));
99+
return tokens.length > 0 ? tokens.join(',') : defaultRole;
100+
};
101+
79102
const results: SyncResult = {
80103
success: true,
81104
totalFound: employees.length,
@@ -149,18 +172,40 @@ export class GenericEmployeeSyncService {
149172
});
150173

151174
if (existingMember) {
175+
// Self-heal limbo roles: if the persisted member.role contains
176+
// tokens that don't map to any valid role today (e.g. an Entra
177+
// jobTitle planted by a misconfigured DSL pre-fix), drop them.
178+
// We only ever shrink the role string here — never overwrite a
179+
// valid assignment with whatever the provider sent.
180+
const healedRole = sanitizeRole(existingMember.role);
181+
const needsHeal = healedRole !== existingMember.role;
182+
if (needsHeal) {
183+
this.logger.warn(
184+
`[GenericSync] Healing limbo role for ${normalizedEmail}: "${existingMember.role}" → "${healedRole}"`,
185+
);
186+
}
187+
152188
if (existingMember.deactivated && allowReactivation) {
153-
// Reactivate the member
154189
await db.member.update({
155190
where: { id: existingMember.id },
156-
data: { deactivated: false, isActive: true },
191+
data: {
192+
deactivated: false,
193+
isActive: true,
194+
...(needsHeal ? { role: healedRole } : {}),
195+
},
157196
});
158197
results.reactivated++;
159198
results.details.push({
160199
email: normalizedEmail,
161200
status: 'reactivated',
162201
});
163202
} else {
203+
if (needsHeal) {
204+
await db.member.update({
205+
where: { id: existingMember.id },
206+
data: { role: healedRole },
207+
});
208+
}
164209
results.skipped++;
165210
results.details.push({
166211
email: normalizedEmail,
@@ -174,11 +219,17 @@ export class GenericEmployeeSyncService {
174219
}
175220

176221
// Create new member
222+
const sanitizedRole = sanitizeRole(employee.role);
223+
if (employee.role && sanitizedRole !== employee.role) {
224+
this.logger.warn(
225+
`[GenericSync] Provider "${providerName}" sent unrecognized role "${employee.role}" for ${normalizedEmail}; falling back to "${sanitizedRole}"`,
226+
);
227+
}
177228
await db.member.create({
178229
data: {
179230
organizationId,
180231
userId: existingUser.id,
181-
role: employee.role || defaultRole,
232+
role: sanitizedRole,
182233
isActive: true,
183234
},
184235
});

0 commit comments

Comments
 (0)