WIP: cover edge cases for string vs ObjectId handling in permission granting and checking

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 */

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<string, unknown> = {};

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', () => {

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<Array<{ principalType: string; principalId?: string | Types.ObjectId }>> {
     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