From 4e7379a8611e593deda8bf64fb1877987ed8b7ff Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 3 Aug 2025 23:23:59 -0400 Subject: [PATCH] WIP: cover edge cases for string vs ObjectId handling in permission granting and checking --- api/server/routes/prompts.test.js | 6 +- api/server/services/PermissionService.js | 37 +- api/server/services/PermissionService.spec.js | 340 ++++++++++++++++++ .../data-schemas/src/methods/aclEntry.spec.ts | 186 ++++++++++ packages/data-schemas/src/methods/aclEntry.ts | 18 +- .../src/methods/userGroup.spec.ts | 55 +++ .../data-schemas/src/methods/userGroup.ts | 7 +- 7 files changed, 625 insertions(+), 24 deletions(-) diff --git a/api/server/routes/prompts.test.js b/api/server/routes/prompts.test.js index 812320fd6b..530d5d67fa 100644 --- a/api/server/routes/prompts.test.js +++ b/api/server/routes/prompts.test.js @@ -214,8 +214,6 @@ describe('Prompt Routes - ACL Permissions', () => { console.log('Console errors:', consoleErrorSpy.mock.calls); } - console.log('POST response:', response.body); - expect(response.status).toBe(200); expect(response.body.prompt).toBeDefined(); expect(response.body.prompt.prompt).toBe(promptData.prompt.prompt); @@ -303,8 +301,8 @@ describe('Prompt Routes - ACL Permissions', () => { grantedBy: testUsers.owner._id, }); - const response = await request(app).get(`/api/prompts/${testPrompt._id}`).expect(200); - + const response = await request(app).get(`/api/prompts/${testPrompt._id}`); + expect(response.status).toBe(200); expect(response.body._id).toBe(testPrompt._id.toString()); expect(response.body.prompt).toBe(testPrompt.prompt); }); diff --git a/api/server/services/PermissionService.js b/api/server/services/PermissionService.js index 240c532b60..16374fc0dc 100644 --- a/api/server/services/PermissionService.js +++ b/api/server/services/PermissionService.js @@ -46,8 +46,8 @@ const validateResourceType = (resourceType) => { /** * Grant a permission to a principal for a resource using a role * @param {Object} params - Parameters for granting role-based permission - * @param {string} params.principalType - 'user', 'group', or 'public' - * @param {string|mongoose.Types.ObjectId|null} params.principalId - The ID of the principal (null for 'public') + * @param {string} params.principalType - PrincipalType.USER, PrincipalType.GROUP, or PrincipalType.PUBLIC + * @param {string|mongoose.Types.ObjectId|null} params.principalId - The ID of the principal (null for PrincipalType.PUBLIC) * @param {string} params.resourceType - Type of resource (e.g., 'agent') * @param {string|mongoose.Types.ObjectId} params.resourceId - The ID of the resource * @param {string} params.accessRoleId - The ID of the role (e.g., AccessRoleIds.AGENT_VIEWER, AccessRoleIds.AGENT_EDITOR) @@ -267,7 +267,7 @@ const getAvailableRoles = async ({ resourceType }) => { * Ensures a principal exists in the database based on TPrincipal data * Creates user if it doesn't exist locally (for Entra ID users) * @param {Object} principal - TPrincipal object from frontend - * @param {string} principal.type - 'user', 'group', or 'public' + * @param {string} principal.type - PrincipalType.USER, PrincipalType.GROUP, or PrincipalType.PUBLIC * @param {string} [principal.id] - Local database ID (null for Entra ID principals not yet synced) * @param {string} principal.name - Display name * @param {string} [principal.email] - Email address @@ -276,7 +276,7 @@ const getAvailableRoles = async ({ resourceType }) => { * @returns {Promise} Returns the principalId for database operations, null for public */ const ensurePrincipalExists = async function (principal) { - if (principal.type === 'public') { + if (principal.type === PrincipalType.PUBLIC) { return null; } @@ -284,7 +284,7 @@ const ensurePrincipalExists = async function (principal) { return principal.id; } - if (principal.type === 'user' && principal.source === 'entra') { + if (principal.type === PrincipalType.USER && principal.source === 'entra') { if (!principal.email || !principal.idOnTheSource) { throw new Error('Entra ID user principals must have email and idOnTheSource'); } @@ -317,7 +317,7 @@ const ensurePrincipalExists = async function (principal) { return userId.toString(); } - if (principal.type === 'group') { + if (principal.type === PrincipalType.GROUP) { throw new Error('Group principals should be handled by group-specific methods'); } @@ -329,7 +329,7 @@ const ensurePrincipalExists = async function (principal) { * Creates group if it doesn't exist locally (for Entra ID groups) * For Entra ID groups, always synchronizes member IDs when authentication context is provided * @param {Object} principal - TPrincipal object from frontend - * @param {string} principal.type - Must be 'group' + * @param {string} principal.type - Must be PrincipalType.GROUP * @param {string} [principal.id] - Local database ID (null for Entra ID principals not yet synced) * @param {string} principal.name - Display name * @param {string} [principal.email] - Email address @@ -342,8 +342,8 @@ const ensurePrincipalExists = async function (principal) { * @returns {Promise} Returns the groupId for database operations */ const ensureGroupPrincipalExists = async function (principal, authContext = null) { - if (principal.type !== 'group') { - throw new Error(`Invalid principal type: ${principal.type}. Expected 'group'`); + if (principal.type !== PrincipalType.GROUP) { + throw new Error(`Invalid principal type: ${principal.type}. Expected '${PrincipalType.GROUP}'`); } if (principal.source === 'entra') { @@ -626,8 +626,11 @@ const bulkUpdateResourcePermissions = async ({ resourceId, }; - if (principal.type !== 'public') { - query.principalId = principal.id; + if (principal.type !== PrincipalType.PUBLIC) { + query.principalId = + principal.type === PrincipalType.ROLE + ? principal.id + : new mongoose.Types.ObjectId(principal.id); } const principalModelMap = { @@ -648,7 +651,10 @@ const bulkUpdateResourcePermissions = async ({ resourceType, resourceId, ...(principal.type !== PrincipalType.PUBLIC && { - principalId: principal.id, + principalId: + principal.type === PrincipalType.ROLE + ? principal.id + : new mongoose.Types.ObjectId(principal.id), principalModel: principalModelMap[principal.type], }), }, @@ -696,8 +702,11 @@ const bulkUpdateResourcePermissions = async ({ resourceId, }; - if (principal.type !== 'public') { - query.principalId = principal.id; + if (principal.type !== PrincipalType.PUBLIC) { + query.principalId = + principal.type === PrincipalType.ROLE + ? principal.id + : new mongoose.Types.ObjectId(principal.id); } deleteQueries.push(query); diff --git a/api/server/services/PermissionService.spec.js b/api/server/services/PermissionService.spec.js index 9836859198..5772f3b909 100644 --- a/api/server/services/PermissionService.spec.js +++ b/api/server/services/PermissionService.spec.js @@ -1264,4 +1264,344 @@ describe('PermissionService', () => { ); }); }); + + describe('String vs ObjectId Edge Cases', () => { + const stringUserId = new mongoose.Types.ObjectId().toString(); + const objectIdUserId = new mongoose.Types.ObjectId(); + const stringGroupId = new mongoose.Types.ObjectId().toString(); + const objectIdGroupId = new mongoose.Types.ObjectId(); + const testResourceId = new mongoose.Types.ObjectId(); + + beforeEach(async () => { + // Clear any existing ACL entries + await AclEntry.deleteMany({}); + getUserPrincipals.mockReset(); + }); + + test('should handle string userId in grantPermission', async () => { + const entry = await grantPermission({ + principalType: PrincipalType.USER, + principalId: stringUserId, // Pass string + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_VIEWER, + grantedBy: grantedById, + }); + + expect(entry).toBeDefined(); + expect(entry.principalType).toBe(PrincipalType.USER); + // Should be stored as ObjectId + expect(entry.principalId).toBeInstanceOf(mongoose.Types.ObjectId); + expect(entry.principalId.toString()).toBe(stringUserId); + }); + + test('should handle string groupId in grantPermission', async () => { + const entry = await grantPermission({ + principalType: PrincipalType.GROUP, + principalId: stringGroupId, // Pass string + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_EDITOR, + grantedBy: grantedById, + }); + + expect(entry).toBeDefined(); + expect(entry.principalType).toBe(PrincipalType.GROUP); + // Should be stored as ObjectId + expect(entry.principalId).toBeInstanceOf(mongoose.Types.ObjectId); + expect(entry.principalId.toString()).toBe(stringGroupId); + }); + + test('should handle string roleId in grantPermission for ROLE type', async () => { + const roleString = 'moderator'; + + const entry = await grantPermission({ + principalType: PrincipalType.ROLE, + principalId: roleString, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_VIEWER, + grantedBy: grantedById, + }); + + expect(entry).toBeDefined(); + expect(entry.principalType).toBe(PrincipalType.ROLE); + // Should remain as string for ROLE type + expect(typeof entry.principalId).toBe('string'); + expect(entry.principalId).toBe(roleString); + expect(entry.principalModel).toBe(PrincipalModel.ROLE); + }); + + test('should check permissions correctly when permission granted with string userId', async () => { + // Grant permission with string userId + await grantPermission({ + principalType: PrincipalType.USER, + principalId: stringUserId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_EDITOR, + grantedBy: grantedById, + }); + + // Mock getUserPrincipals to return ObjectId (as it should after our fix) + getUserPrincipals.mockResolvedValue([ + { + principalType: PrincipalType.USER, + principalId: new mongoose.Types.ObjectId(stringUserId), + }, + { principalType: PrincipalType.PUBLIC }, + ]); + + // Check permission with string userId + const hasPermission = await checkPermission({ + userId: stringUserId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + requiredPermission: 1, // VIEW + }); + + expect(hasPermission).toBe(true); + expect(getUserPrincipals).toHaveBeenCalledWith({ userId: stringUserId, role: undefined }); + }); + + test('should check permissions correctly when permission granted with ObjectId', async () => { + // Grant permission with ObjectId + await grantPermission({ + principalType: PrincipalType.USER, + principalId: objectIdUserId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: grantedById, + }); + + // Mock getUserPrincipals to return ObjectId + getUserPrincipals.mockResolvedValue([ + { principalType: PrincipalType.USER, principalId: objectIdUserId }, + { principalType: PrincipalType.PUBLIC }, + ]); + + // Check permission with ObjectId + const hasPermission = await checkPermission({ + userId: objectIdUserId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + requiredPermission: 7, // Full permissions + }); + + expect(hasPermission).toBe(true); + expect(getUserPrincipals).toHaveBeenCalledWith({ userId: objectIdUserId, role: undefined }); + }); + + test('should handle bulkUpdateResourcePermissions with string IDs', async () => { + const updatedPrincipals = [ + { + type: PrincipalType.USER, + id: stringUserId, // String ID + accessRoleId: AccessRoleIds.AGENT_VIEWER, + }, + { + type: PrincipalType.GROUP, + id: stringGroupId, // String ID + accessRoleId: AccessRoleIds.AGENT_EDITOR, + }, + { + type: PrincipalType.ROLE, + id: 'admin', // String ID (should remain string) + accessRoleId: AccessRoleIds.AGENT_OWNER, + }, + ]; + + const results = await bulkUpdateResourcePermissions({ + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + updatedPrincipals, + grantedBy: grantedById, + }); + + expect(results.granted).toHaveLength(3); + expect(results.errors).toHaveLength(0); + + // Verify USER entry has ObjectId + const userEntry = await AclEntry.findOne({ + principalType: PrincipalType.USER, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + }); + expect(userEntry.principalId).toBeInstanceOf(mongoose.Types.ObjectId); + expect(userEntry.principalId.toString()).toBe(stringUserId); + + // Verify GROUP entry has ObjectId + const groupEntry = await AclEntry.findOne({ + principalType: PrincipalType.GROUP, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + }); + expect(groupEntry.principalId).toBeInstanceOf(mongoose.Types.ObjectId); + expect(groupEntry.principalId.toString()).toBe(stringGroupId); + + // Verify ROLE entry has string + const roleEntry = await AclEntry.findOne({ + principalType: PrincipalType.ROLE, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + }); + expect(typeof roleEntry.principalId).toBe('string'); + expect(roleEntry.principalId).toBe('admin'); + }); + + test('should handle revoking permissions with string IDs in bulkUpdateResourcePermissions', async () => { + // First grant permissions with ObjectIds + await grantPermission({ + principalType: PrincipalType.USER, + principalId: objectIdUserId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: grantedById, + }); + + await grantPermission({ + principalType: PrincipalType.GROUP, + principalId: objectIdGroupId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_EDITOR, + grantedBy: grantedById, + }); + + // Revoke using string IDs + const revokedPrincipals = [ + { + type: PrincipalType.USER, + id: objectIdUserId.toString(), // String version of ObjectId + }, + { + type: PrincipalType.GROUP, + id: objectIdGroupId.toString(), // String version of ObjectId + }, + ]; + + const results = await bulkUpdateResourcePermissions({ + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + revokedPrincipals, + grantedBy: grantedById, + }); + + expect(results.revoked).toHaveLength(2); + expect(results.errors).toHaveLength(0); + + // Verify permissions were actually revoked + const remainingEntries = await AclEntry.find({ + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + }); + expect(remainingEntries).toHaveLength(0); + }); + + test('should find accessible resources when permissions granted with mixed ID types', async () => { + const resource1 = new mongoose.Types.ObjectId(); + const resource2 = new mongoose.Types.ObjectId(); + const resource3 = new mongoose.Types.ObjectId(); + + // Grant with string userId + await grantPermission({ + principalType: PrincipalType.USER, + principalId: stringUserId, + resourceType: ResourceType.AGENT, + resourceId: resource1, + accessRoleId: AccessRoleIds.AGENT_VIEWER, + grantedBy: grantedById, + }); + + // Grant with ObjectId userId (same user) + await grantPermission({ + principalType: PrincipalType.USER, + principalId: new mongoose.Types.ObjectId(stringUserId), + resourceType: ResourceType.AGENT, + resourceId: resource2, + accessRoleId: AccessRoleIds.AGENT_EDITOR, + grantedBy: grantedById, + }); + + // Grant to role + await grantPermission({ + principalType: PrincipalType.ROLE, + principalId: 'admin', + resourceType: ResourceType.AGENT, + resourceId: resource3, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: grantedById, + }); + + // Mock getUserPrincipals to return user with admin role + getUserPrincipals.mockResolvedValue([ + { + principalType: PrincipalType.USER, + principalId: new mongoose.Types.ObjectId(stringUserId), + }, + { principalType: PrincipalType.ROLE, principalId: 'admin' }, + { principalType: PrincipalType.PUBLIC }, + ]); + + const accessibleResources = await findAccessibleResources({ + userId: stringUserId, + role: 'admin', + resourceType: ResourceType.AGENT, + requiredPermissions: 1, // VIEW + }); + + // Should find all three resources + expect(accessibleResources).toHaveLength(3); + const resourceIds = accessibleResources.map((id) => id.toString()); + expect(resourceIds).toContain(resource1.toString()); + expect(resourceIds).toContain(resource2.toString()); + expect(resourceIds).toContain(resource3.toString()); + }); + + test('should get effective permissions with mixed ID types', async () => { + // Grant VIEW permission with string userId + await grantPermission({ + principalType: PrincipalType.USER, + principalId: stringUserId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_VIEWER, + grantedBy: grantedById, + }); + + // Grant EDIT permission to a group with string groupId + await grantPermission({ + principalType: PrincipalType.GROUP, + principalId: stringGroupId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_EDITOR, + grantedBy: grantedById, + }); + + // Mock getUserPrincipals to return ObjectIds (as it should after our fix) + getUserPrincipals.mockResolvedValue([ + { + principalType: PrincipalType.USER, + principalId: new mongoose.Types.ObjectId(stringUserId), + }, + { + principalType: PrincipalType.GROUP, + principalId: new mongoose.Types.ObjectId(stringGroupId), + }, + { principalType: PrincipalType.PUBLIC }, + ]); + + const effectivePermissions = await getEffectivePermissions({ + userId: stringUserId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + }); + + // Should combine VIEW (1) and EDIT (3) permissions + expect(effectivePermissions).toBe(3); // EDITOR includes VIEW + }); + }); }); diff --git a/packages/data-schemas/src/methods/aclEntry.spec.ts b/packages/data-schemas/src/methods/aclEntry.spec.ts index 69983f369f..be18a81c1b 100644 --- a/packages/data-schemas/src/methods/aclEntry.spec.ts +++ b/packages/data-schemas/src/methods/aclEntry.spec.ts @@ -394,6 +394,192 @@ describe('AclEntry Model Tests', () => { }); }); + describe('String vs ObjectId Edge Cases', () => { + test('should handle string userId in grantPermission', async () => { + const userIdString = userId.toString(); + + const entry = await methods.grantPermission( + PrincipalType.USER, + userIdString, // Pass string instead of ObjectId + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + grantedById, + ); + + expect(entry).toBeDefined(); + expect(entry?.principalType).toBe(PrincipalType.USER); + // Should be stored as ObjectId + expect(entry?.principalId).toBeInstanceOf(mongoose.Types.ObjectId); + expect(entry?.principalId?.toString()).toBe(userIdString); + }); + + test('should handle string groupId in grantPermission', async () => { + const groupIdString = groupId.toString(); + + const entry = await methods.grantPermission( + PrincipalType.GROUP, + groupIdString, // Pass string instead of ObjectId + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + grantedById, + ); + + expect(entry).toBeDefined(); + expect(entry?.principalType).toBe(PrincipalType.GROUP); + // Should be stored as ObjectId + expect(entry?.principalId).toBeInstanceOf(mongoose.Types.ObjectId); + expect(entry?.principalId?.toString()).toBe(groupIdString); + }); + + test('should handle string roleId in grantPermission for ROLE type', async () => { + const roleString = 'admin'; + + const entry = await methods.grantPermission( + PrincipalType.ROLE, + roleString, + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + grantedById, + ); + + expect(entry).toBeDefined(); + expect(entry?.principalType).toBe(PrincipalType.ROLE); + // Should remain as string for ROLE type + expect(typeof entry?.principalId).toBe('string'); + expect(entry?.principalId).toBe(roleString); + expect(entry?.principalModel).toBe(PrincipalModel.ROLE); + }); + + test('should handle string principalId in revokePermission', async () => { + // First grant permission with ObjectId + await methods.grantPermission( + PrincipalType.USER, + userId, + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + grantedById, + ); + + // Then revoke with string ID + const result = await methods.revokePermission( + PrincipalType.USER, + userId.toString(), // Pass string + ResourceType.AGENT, + resourceId, + ); + + expect(result.deletedCount).toBe(1); + + // Verify it's actually deleted + const entries = await methods.findEntriesByPrincipal(PrincipalType.USER, userId); + expect(entries).toHaveLength(0); + }); + + test('should handle string principalId in modifyPermissionBits', async () => { + // First grant permission with ObjectId + await methods.grantPermission( + PrincipalType.USER, + userId, + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + grantedById, + ); + + // Then modify with string ID + const updated = await methods.modifyPermissionBits( + PrincipalType.USER, + userId.toString(), // Pass string + ResourceType.AGENT, + resourceId, + PermissionBits.EDIT, + null, + ); + + expect(updated).toBeDefined(); + expect(updated?.permBits).toBe(PermissionBits.VIEW | PermissionBits.EDIT); + }); + + test('should handle mixed string and ObjectId in hasPermission', async () => { + // Grant permission with string ID + await methods.grantPermission( + PrincipalType.USER, + userId.toString(), + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + grantedById, + ); + + // Check permission with ObjectId in principals list + const hasPermWithObjectId = await methods.hasPermission( + [{ principalType: PrincipalType.USER, principalId: userId }], + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + ); + expect(hasPermWithObjectId).toBe(true); + + // Check permission with string in principals list + const hasPermWithString = await methods.hasPermission( + [{ principalType: PrincipalType.USER, principalId: userId.toString() }], + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + ); + expect(hasPermWithString).toBe(false); // This should fail because hasPermission doesn't convert + + // Check with converted ObjectId + const hasPermWithConvertedId = await methods.hasPermission( + [ + { + principalType: PrincipalType.USER, + principalId: new mongoose.Types.ObjectId(userId.toString()), + }, + ], + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + ); + expect(hasPermWithConvertedId).toBe(true); + }); + + test('should update existing permission when granting with string ID', async () => { + // First grant with ObjectId + await methods.grantPermission( + PrincipalType.USER, + userId, + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW, + grantedById, + ); + + // Grant again with string ID and different permissions + const updated = await methods.grantPermission( + PrincipalType.USER, + userId.toString(), + ResourceType.AGENT, + resourceId, + PermissionBits.VIEW | PermissionBits.EDIT | PermissionBits.DELETE, + grantedById, + ); + + expect(updated).toBeDefined(); + expect(updated?.permBits).toBe( + PermissionBits.VIEW | PermissionBits.EDIT | PermissionBits.DELETE, + ); + + // Should still only be one entry + const entries = await methods.findEntriesByPrincipal(PrincipalType.USER, userId); + expect(entries).toHaveLength(1); + }); + }); + describe('Resource Access Queries', () => { test('should find accessible resources', async () => { /** Create multiple resources with different permissions */ diff --git a/packages/data-schemas/src/methods/aclEntry.ts b/packages/data-schemas/src/methods/aclEntry.ts index eed20deaa4..67e91ad018 100644 --- a/packages/data-schemas/src/methods/aclEntry.ts +++ b/packages/data-schemas/src/methods/aclEntry.ts @@ -1,5 +1,6 @@ +import { Types } from 'mongoose'; import { PrincipalType, PrincipalModel } from 'librechat-data-provider'; -import type { Model, Types, DeleteResult, ClientSession } from 'mongoose'; +import type { Model, DeleteResult, ClientSession } from 'mongoose'; import type { IAclEntry } from '~/types'; export function createAclEntryMethods(mongoose: typeof import('mongoose')) { @@ -147,7 +148,10 @@ export function createAclEntryMethods(mongoose: typeof import('mongoose')) { }; if (principalType !== PrincipalType.PUBLIC) { - query.principalId = principalId; + query.principalId = + typeof principalId === 'string' && principalType !== PrincipalType.ROLE + ? new Types.ObjectId(principalId) + : principalId; if (principalType === PrincipalType.USER) { query.principalModel = PrincipalModel.USER; } else if (principalType === PrincipalType.GROUP) { @@ -199,7 +203,10 @@ export function createAclEntryMethods(mongoose: typeof import('mongoose')) { }; if (principalType !== PrincipalType.PUBLIC) { - query.principalId = principalId; + query.principalId = + typeof principalId === 'string' && principalType !== PrincipalType.ROLE + ? new Types.ObjectId(principalId) + : principalId; } const options = session ? { session } : {}; @@ -235,7 +242,10 @@ export function createAclEntryMethods(mongoose: typeof import('mongoose')) { }; if (principalType !== PrincipalType.PUBLIC) { - query.principalId = principalId; + query.principalId = + typeof principalId === 'string' && principalType !== PrincipalType.ROLE + ? new Types.ObjectId(principalId) + : principalId; } const update: Record = {}; diff --git a/packages/data-schemas/src/methods/userGroup.spec.ts b/packages/data-schemas/src/methods/userGroup.spec.ts index 92a957792a..9de8eaf912 100644 --- a/packages/data-schemas/src/methods/userGroup.spec.ts +++ b/packages/data-schemas/src/methods/userGroup.spec.ts @@ -362,6 +362,61 @@ describe('User Group Methods Tests', () => { expect(principals[1].principalType).toBe(PrincipalType.PUBLIC); expect(principals[1].principalId).toBeUndefined(); }); + + test('should convert string userId to ObjectId in getUserPrincipals', async () => { + /** Add user to a group */ + await methods.addUserToGroup( + testUser1._id as mongoose.Types.ObjectId, + testGroup._id as mongoose.Types.ObjectId, + ); + + /** Get user principals with string userId */ + const principals = await methods.getUserPrincipals({ + userId: (testUser1._id as mongoose.Types.ObjectId).toString(), + }); + + /** Should include user, role (default USER), group, and public principals */ + expect(principals).toHaveLength(4); + + /** Check that USER principal has ObjectId */ + const userPrincipal = principals.find((p) => p.principalType === PrincipalType.USER); + expect(userPrincipal).toBeDefined(); + expect(userPrincipal?.principalId).toBeInstanceOf(mongoose.Types.ObjectId); + expect(userPrincipal?.principalId?.toString()).toBe( + (testUser1._id as mongoose.Types.ObjectId).toString(), + ); + + /** Check that GROUP principal has ObjectId */ + const groupPrincipal = principals.find((p) => p.principalType === PrincipalType.GROUP); + expect(groupPrincipal).toBeDefined(); + expect(groupPrincipal?.principalId).toBeInstanceOf(mongoose.Types.ObjectId); + expect(groupPrincipal?.principalId?.toString()).toBe(testGroup._id.toString()); + }); + + test('should include role principal as string in getUserPrincipals', async () => { + /** Create user with specific role */ + const userWithRole = await User.create({ + name: 'Admin User', + email: 'admin@example.com', + password: 'password123', + provider: 'local', + role: 'ADMIN', + }); + + /** Get user principals */ + const principals = await methods.getUserPrincipals({ + userId: userWithRole._id as mongoose.Types.ObjectId, + }); + + /** Should include user, role, and public principals */ + expect(principals).toHaveLength(3); + + /** Check that ROLE principal has string ID */ + const rolePrincipal = principals.find((p) => p.principalType === PrincipalType.ROLE); + expect(rolePrincipal).toBeDefined(); + expect(typeof rolePrincipal?.principalId).toBe('string'); + expect(rolePrincipal?.principalId).toBe('ADMIN'); + }); }); describe('Entra ID Synchronization', () => { diff --git a/packages/data-schemas/src/methods/userGroup.ts b/packages/data-schemas/src/methods/userGroup.ts index f61469d825..2464e35c9e 100644 --- a/packages/data-schemas/src/methods/userGroup.ts +++ b/packages/data-schemas/src/methods/userGroup.ts @@ -1,6 +1,7 @@ +import { Types } from 'mongoose'; import { PrincipalType } from 'librechat-data-provider'; import type { TUser, TPrincipalSearchResult } from 'librechat-data-provider'; -import type { Model, Types, ClientSession } from 'mongoose'; +import type { Model, ClientSession } from 'mongoose'; import type { IGroup, IRole, IUser } from '~/types'; export function createUserGroupMethods(mongoose: typeof import('mongoose')) { @@ -251,8 +252,10 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { session?: ClientSession, ): Promise> { const { userId, role } = params; + /** `userId` must be an `ObjectId` for USER principal since ACL entries store `ObjectId`s */ + const userObjectId = typeof userId === 'string' ? new Types.ObjectId(userId) : userId; const principals: Array<{ principalType: string; principalId?: string | Types.ObjectId }> = [ - { principalType: PrincipalType.USER, principalId: userId }, + { principalType: PrincipalType.USER, principalId: userObjectId }, ]; // If role is not provided, query user to get it