Skip to content

Commit f3e2bf2

Browse files
authored
🧲 fix: Match Directory Users to Existing Principals (#15855)
* fix: Preserve Directory Identity Ownership * fix: Fail Closed on Identity Conflicts * fix: Resolve Existing Sharing Principals Safely
1 parent fe351ee commit f3e2bf2

7 files changed

Lines changed: 180 additions & 33 deletions

File tree

api/server/services/AuthService.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1005,8 +1005,8 @@ const resendVerificationEmail = async (req) => {
10051005
} catch (error) {
10061006
logger.error(`[resendVerificationEmail] Error resending verification email: ${error.message}`);
10071007
return {
1008-
status: 500,
1009-
message: 'Something went wrong.',
1008+
status: 200,
1009+
message: genericVerificationMessage,
10101010
};
10111011
}
10121012
};

api/server/services/AuthService.spec.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1227,6 +1227,26 @@ describe('resendVerificationEmail', () => {
12271227
}),
12281228
);
12291229
});
1230+
1231+
it('returns the generic response when delivery fails', async () => {
1232+
const user = {
1233+
_id: 'user-delivery-failure',
1234+
email: 'delivery-failure@example.com',
1235+
name: 'Delivery Failure',
1236+
};
1237+
findUser.mockResolvedValue(user);
1238+
sendEmail.mockRejectedValue(new Error('mail transport unavailable'));
1239+
1240+
const result = await resendVerificationEmail({ body: { email: user.email } });
1241+
1242+
expect(result).toEqual({
1243+
status: 200,
1244+
message: 'Please check your email to verify your email address.',
1245+
});
1246+
expect(logger.error).toHaveBeenCalledWith(
1247+
'[resendVerificationEmail] Error resending verification email: mail transport unavailable',
1248+
);
1249+
});
12301250
});
12311251

12321252
describe('CloudFront cookie integration', () => {

api/server/services/PermissionService.js

Lines changed: 15 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
const mongoose = require('mongoose');
2-
const { AccessControlService, isEnabled } = require('@librechat/api');
2+
const { AccessControlService, isEnabled, ensureDirectoryPrincipalUser } = require('@librechat/api');
33
const {
44
tenantStorage,
55
getTenantId,
@@ -337,36 +337,20 @@ const ensurePrincipalExists = async function (principal) {
337337
}
338338

339339
if (principal.type === PrincipalType.USER && principal.source === 'entra') {
340-
if (!principal.email || !principal.idOnTheSource) {
341-
throw new Error('Entra ID user principals must have email and idOnTheSource');
342-
}
343-
344-
let existingUser = await db.findUser({ idOnTheSource: principal.idOnTheSource });
345-
346-
if (!existingUser) {
347-
existingUser = await db.findUser({ email: principal.email });
348-
}
349-
350-
if (existingUser) {
351-
if (!existingUser.idOnTheSource && principal.idOnTheSource) {
352-
await db.updateUser(existingUser._id, {
353-
idOnTheSource: principal.idOnTheSource,
354-
provider: 'openid',
355-
});
356-
}
357-
return existingUser._id.toString();
358-
}
359-
360-
const userData = {
361-
name: principal.name,
362-
email: principal.email.toLowerCase(),
363-
emailVerified: false,
364-
provider: 'openid',
365-
idOnTheSource: principal.idOnTheSource,
366-
};
367-
368-
const userId = await db.createUser(userData, true, true);
369-
return userId.toString();
340+
return ensureDirectoryPrincipalUser(principal, {
341+
findUserBySourceId: async (idOnTheSource) => {
342+
const user = await db.findUser({ idOnTheSource });
343+
return user ? { id: user._id.toString() } : null;
344+
},
345+
findUserByEmail: async (email) => {
346+
const user = await db.findUser({ email });
347+
return user ? { id: user._id.toString() } : null;
348+
},
349+
createUser: async (userData) => {
350+
const userId = await db.createUser(userData, true, true);
351+
return userId.toString();
352+
},
353+
});
370354
}
371355

372356
if (principal.type === PrincipalType.GROUP) {

api/server/services/PermissionService.spec.js

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,46 @@ describe('PermissionService', () => {
296296
expect(principalId).toBe(currentUser._id.toString());
297297
});
298298

299+
test('accepts a directory user already linked to the supplied source id', async () => {
300+
const directoryUser = await User.create({
301+
name: 'ACL Principal Directory User',
302+
email: 'acl-principal-directory-user@example.com',
303+
provider: 'openid',
304+
idOnTheSource: 'directory-user-id',
305+
});
306+
307+
const principalId = await ensurePrincipalExists({
308+
type: PrincipalType.USER,
309+
name: directoryUser.name,
310+
email: directoryUser.email,
311+
source: 'entra',
312+
idOnTheSource: directoryUser.idOnTheSource,
313+
});
314+
315+
expect(principalId).toBe(directoryUser._id.toString());
316+
});
317+
318+
test('uses an existing user found only by email without linking its identity', async () => {
319+
const existingUser = await User.create({
320+
name: 'ACL Principal Existing User',
321+
email: 'acl-principal-existing-user@example.com',
322+
provider: 'local',
323+
});
324+
325+
const principalId = await ensurePrincipalExists({
326+
type: PrincipalType.USER,
327+
name: existingUser.name,
328+
email: existingUser.email,
329+
source: 'entra',
330+
idOnTheSource: 'unlinked-directory-id',
331+
});
332+
333+
const unchangedUser = await User.findById(existingUser._id).lean();
334+
expect(principalId).toBe(existingUser._id.toString());
335+
expect(unchangedUser.provider).toBe('local');
336+
expect(unchangedUser.idOnTheSource).toBeUndefined();
337+
});
338+
299339
test('rejects a local group id outside the current request context', async () => {
300340
const outsideGroup = await Group.create({
301341
name: 'ACL Principal Outside Group',
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
import { ensureDirectoryPrincipalUser } from './principals';
2+
3+
const createMethods = () => ({
4+
findUserBySourceId: jest.fn().mockResolvedValue(null),
5+
findUserByEmail: jest.fn().mockResolvedValue(null),
6+
createUser: jest.fn().mockResolvedValue('created-user'),
7+
});
8+
9+
const principal = {
10+
name: 'Directory User',
11+
email: 'Directory-User@Example.com',
12+
idOnTheSource: 'directory-user-id',
13+
};
14+
15+
describe('ensureDirectoryPrincipalUser', () => {
16+
it('returns a user already linked to the directory source ID without an email lookup', async () => {
17+
const methods = createMethods();
18+
methods.findUserBySourceId.mockResolvedValue({ id: 'source-user' });
19+
20+
await expect(ensureDirectoryPrincipalUser(principal, methods)).resolves.toBe('source-user');
21+
expect(methods.findUserByEmail).not.toHaveBeenCalled();
22+
expect(methods.createUser).not.toHaveBeenCalled();
23+
});
24+
25+
it('returns an existing user found by email without creating a placeholder', async () => {
26+
const methods = createMethods();
27+
methods.findUserByEmail.mockResolvedValue({ id: 'email-user' });
28+
29+
await expect(ensureDirectoryPrincipalUser(principal, methods)).resolves.toBe('email-user');
30+
expect(methods.createUser).not.toHaveBeenCalled();
31+
});
32+
33+
it('creates a normalized directory placeholder when neither identifier matches', async () => {
34+
const methods = createMethods();
35+
36+
await expect(ensureDirectoryPrincipalUser(principal, methods)).resolves.toBe('created-user');
37+
expect(methods.createUser).toHaveBeenCalledWith({
38+
name: principal.name,
39+
email: 'directory-user@example.com',
40+
emailVerified: false,
41+
provider: 'openid',
42+
idOnTheSource: principal.idOnTheSource,
43+
});
44+
});
45+
46+
it('rejects incomplete directory principals before database access', async () => {
47+
const methods = createMethods();
48+
49+
await expect(ensureDirectoryPrincipalUser({ name: 'Incomplete' }, methods)).rejects.toThrow(
50+
'Directory user principals must have email and idOnTheSource',
51+
);
52+
expect(methods.findUserBySourceId).not.toHaveBeenCalled();
53+
});
54+
});

packages/api/src/acl/principals.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import type { TPrincipal } from 'librechat-data-provider';
2+
3+
export interface DirectoryPrincipalUser {
4+
id: string;
5+
}
6+
7+
export interface DirectoryPrincipalUserData {
8+
name?: string;
9+
email: string;
10+
emailVerified: false;
11+
provider: 'openid';
12+
idOnTheSource: string;
13+
}
14+
15+
export interface DirectoryPrincipalUserMethods {
16+
findUserBySourceId: (idOnTheSource: string) => Promise<DirectoryPrincipalUser | null>;
17+
findUserByEmail: (email: string) => Promise<DirectoryPrincipalUser | null>;
18+
createUser: (user: DirectoryPrincipalUserData) => Promise<string>;
19+
}
20+
21+
type DirectoryPrincipal = Pick<TPrincipal, 'name' | 'email' | 'idOnTheSource'>;
22+
23+
export const ensureDirectoryPrincipalUser = async (
24+
principal: DirectoryPrincipal,
25+
methods: DirectoryPrincipalUserMethods,
26+
): Promise<string> => {
27+
if (!principal.email || !principal.idOnTheSource) {
28+
throw new Error('Directory user principals must have email and idOnTheSource');
29+
}
30+
31+
const userBySourceId = await methods.findUserBySourceId(principal.idOnTheSource);
32+
if (userBySourceId) {
33+
return userBySourceId.id;
34+
}
35+
36+
const userByEmail = await methods.findUserByEmail(principal.email);
37+
if (userByEmail) {
38+
return userByEmail.id;
39+
}
40+
41+
return methods.createUser({
42+
name: principal.name,
43+
email: principal.email.toLowerCase(),
44+
emailVerified: false,
45+
provider: 'openid',
46+
idOnTheSource: principal.idOnTheSource,
47+
});
48+
};

packages/api/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ export * from './app';
22
export * from './acl/accessControlService';
33
export * from './acl/insightsPermissions';
44
export * from './acl/middleware';
5+
export * from './acl/principals';
56
export * from './credentials';
67
/* Artifacts */
78
export * from './artifacts';

0 commit comments

Comments
 (0)