mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-03 22:37:20 +02:00
👥 feat: Admin Groups API Endpoints (#12387)
* feat: add listGroups and deleteGroup methods to userGroup
* feat: add admin groups handler factory and Express routes
* fix: address convention violations in admin groups handlers
* fix: address Copilot review findings in admin groups handlers
- Escape regex in listGroups to prevent injection/ReDoS
- Validate ObjectId format in all handlers accepting id/userId params
- Replace N+1 findUser loop with batched findUsers query
- Remove unused findGroupsByMemberId from dep interface
- Map Mongoose ValidationError to 400 in create/update handlers
- Validate name in updateGroupHandler (reject empty/whitespace)
- Handle null updateGroupById result (race condition)
- Tighten error message matching in add/remove member handlers
* test: add unit tests for admin groups handlers
* fix: address code review findings for admin groups
Atomic delete/update handlers (single DB trip), pass through
idOnTheSource, add removeMemberById for non-ObjectId members,
deduplicate member results, fix error message exposure, add hard
cap/sort to listGroups, replace GroupListFilter with Pick of
GroupFilterOptions, validate memberIds as array, trim name in
update, fix import order, and improve test hygiene with fresh
IDs per test.
* fix: cascade cleanup, pagination, and test coverage for admin groups
Add deleteGrantsForPrincipal to systemGrant data layer and wire cascade
cleanup (Config, AclEntry, SystemGrant) into deleteGroupHandler. Add
limit/offset pagination to getGroupMembers. Guard empty PATCH bodies with
400. Remove dead type guard and unnecessary type cast. Add 11 new tests
covering cascade delete, idempotent member removal, empty update, search
filter, 500 error paths, and pagination.
* fix: harden admin groups with cascade resilience, type safety, and fallback removal
Wrap cascade cleanup in inner try/catch so partial failure logs but still
returns 200 (group is already deleted). Replace Record<string, unknown> on
deleteAclEntries with proper typed filter. Log warning for unmapped user
ObjectIds in createGroup memberIds. Add removeMemberById fallback when
removeUserFromGroup throws User not found for ObjectId-format userId.
Extract VALID_GROUP_SOURCES constant. Add 3 new tests (60 total).
* refactor: add countGroups, pagination, and projection type to data layer
Extract buildGroupQuery helper, add countGroups method, support
limit/offset/skip in listGroups, standardize session handling to
.session(session ?? null), and tighten projection parameter from
Record<string, unknown> to Record<string, 0 | 1>.
* fix: cascade resilience, pagination, validation, and error clarity for admin groups
- Use Promise.allSettled for cascade cleanup so all steps run even if
one fails; log individual rejections
- Echo deleted group id in delete response
- Add countGroups dep and wire limit/offset pagination for listGroups
- Deduplicate memberIds before computing total in getGroupMembers
- Use { memberIds: 1 } projection in getGroupMembers
- Cap memberIds at 500 entries in createGroup
- Reject search queries exceeding 200 characters
- Clarify addGroupMember error for non-ObjectId userId
- Document deleted-user fallback limitation in removeGroupMember
* test: extend handler and DB-layer test coverage for admin groups
Handler tests: projection assertion, dedup total, memberIds cap,
search max length, non-ObjectId memberIds passthrough, cascade partial
failure resilience, dedup scenarios, echo id in delete response.
DB-layer tests: listGroups sort/filter/pagination, countGroups,
deleteGroup, removeMemberById, deleteGrantsForPrincipal.
* fix: cast group principalId to ObjectId for ACL entry cleanup
deleteAclEntries is a thin deleteMany wrapper with no type casting,
but grantPermission stores group principalId as ObjectId. Passing the
raw string from req.params would leave orphaned ACL entries on group
deletion.
* refactor: remove redundant pagination clamping from DB listGroups
Handler already clamps limit/offset at the API boundary. The DB
method is a general-purpose building block and should not re-validate.
* fix: add source and name validation, import order, and test coverage for admin groups
- Validate source against VALID_GROUP_SOURCES in createGroupHandler
- Cap name at 500 characters in both create and update handlers
- Document total as upper bound in getGroupMembers response
- Document ObjectId requirement for deleteAclEntries in cascade
- Fix import ordering in test file (local value after type imports)
- Add tests for updateGroup with description, email, avatar fields
- Add tests for invalid source and name max-length in both handlers
* fix: add field length caps, flatten nested try/catch, and fix logger level in admin groups
Add max-length validation for description, email, avatar, and
idOnTheSource in create/update handlers. Extract removeObjectIdMember
helper to flatten nested try/catch per never-nesting convention. Downgrade
unmapped-memberIds log from error to warn. Fix type import ordering and
add missing await in removeMemberById for consistency.
This commit is contained in:
parent
9f6d8c6e93
commit
2e3d66cfe2
11 changed files with 2216 additions and 3 deletions
|
|
@ -702,6 +702,68 @@ describe('systemGrant methods', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('deleteGrantsForPrincipal', () => {
|
||||
it('deletes all grants for a principal', async () => {
|
||||
const groupId = new Types.ObjectId();
|
||||
|
||||
await methods.grantCapability({
|
||||
principalType: PrincipalType.GROUP,
|
||||
principalId: groupId,
|
||||
capability: SystemCapabilities.READ_USERS,
|
||||
});
|
||||
await methods.grantCapability({
|
||||
principalType: PrincipalType.GROUP,
|
||||
principalId: groupId,
|
||||
capability: SystemCapabilities.READ_CONFIGS,
|
||||
});
|
||||
|
||||
await methods.deleteGrantsForPrincipal(PrincipalType.GROUP, groupId);
|
||||
|
||||
const remaining = await SystemGrant.countDocuments({
|
||||
principalType: PrincipalType.GROUP,
|
||||
principalId: groupId,
|
||||
});
|
||||
expect(remaining).toBe(0);
|
||||
});
|
||||
|
||||
it('is a no-op for principal with no grants', async () => {
|
||||
const groupId = new Types.ObjectId();
|
||||
|
||||
await expect(
|
||||
methods.deleteGrantsForPrincipal(PrincipalType.GROUP, groupId),
|
||||
).resolves.not.toThrow();
|
||||
});
|
||||
|
||||
it('does not affect other principals', async () => {
|
||||
const groupA = new Types.ObjectId();
|
||||
const groupB = new Types.ObjectId();
|
||||
|
||||
await methods.grantCapability({
|
||||
principalType: PrincipalType.GROUP,
|
||||
principalId: groupA,
|
||||
capability: SystemCapabilities.READ_USERS,
|
||||
});
|
||||
await methods.grantCapability({
|
||||
principalType: PrincipalType.GROUP,
|
||||
principalId: groupB,
|
||||
capability: SystemCapabilities.READ_USERS,
|
||||
});
|
||||
|
||||
await methods.deleteGrantsForPrincipal(PrincipalType.GROUP, groupA);
|
||||
|
||||
const remainingA = await SystemGrant.countDocuments({
|
||||
principalType: PrincipalType.GROUP,
|
||||
principalId: groupA,
|
||||
});
|
||||
const remainingB = await SystemGrant.countDocuments({
|
||||
principalType: PrincipalType.GROUP,
|
||||
principalId: groupB,
|
||||
});
|
||||
expect(remainingA).toBe(0);
|
||||
expect(remainingB).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe('schema validation', () => {
|
||||
it('rejects null tenantId at the schema level', async () => {
|
||||
await expect(
|
||||
|
|
|
|||
|
|
@ -246,12 +246,28 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')) {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete all system grants for a principal.
|
||||
* Used for cascade cleanup when a principal (group, role) is deleted.
|
||||
*/
|
||||
async function deleteGrantsForPrincipal(
|
||||
principalType: PrincipalType,
|
||||
principalId: string | Types.ObjectId,
|
||||
session?: ClientSession,
|
||||
): Promise<void> {
|
||||
const SystemGrant = mongoose.models.SystemGrant as Model<ISystemGrant>;
|
||||
const normalizedPrincipalId = normalizePrincipalId(principalId, principalType);
|
||||
const options = session ? { session } : {};
|
||||
await SystemGrant.deleteMany({ principalType, principalId: normalizedPrincipalId }, options);
|
||||
}
|
||||
|
||||
return {
|
||||
grantCapability,
|
||||
seedSystemGrants,
|
||||
revokeCapability,
|
||||
hasCapabilityForPrincipals,
|
||||
getCapabilitiesForPrincipal,
|
||||
deleteGrantsForPrincipal,
|
||||
};
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -44,6 +44,19 @@ export function createUserMethods(mongoose: typeof import('mongoose')) {
|
|||
return (await query.lean()) as IUser | null;
|
||||
}
|
||||
|
||||
async function findUsers(
|
||||
searchCriteria: FilterQuery<IUser>,
|
||||
fieldsToSelect?: string | string[] | null,
|
||||
): Promise<IUser[]> {
|
||||
const User = mongoose.models.User;
|
||||
const normalizedCriteria = normalizeEmailInCriteria(searchCriteria);
|
||||
const query = User.find(normalizedCriteria);
|
||||
if (fieldsToSelect) {
|
||||
query.select(fieldsToSelect);
|
||||
}
|
||||
return (await query.lean()) as IUser[];
|
||||
}
|
||||
|
||||
/**
|
||||
* Count the number of user documents in the collection based on the provided filter.
|
||||
*/
|
||||
|
|
@ -323,6 +336,7 @@ export function createUserMethods(mongoose: typeof import('mongoose')) {
|
|||
|
||||
return {
|
||||
findUser,
|
||||
findUsers,
|
||||
countUsers,
|
||||
createUser,
|
||||
updateUser,
|
||||
|
|
|
|||
|
|
@ -600,6 +600,155 @@ describe('UserGroup Methods - Detailed Tests', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('listGroups', () => {
|
||||
beforeEach(async () => {
|
||||
await Group.create([
|
||||
{ name: 'Beta', source: 'local', memberIds: [], email: 'beta@test.com' },
|
||||
{ name: 'Alpha', source: 'local', memberIds: [], description: 'first group' },
|
||||
{ name: 'Gamma', source: 'entra', idOnTheSource: 'ext-g', memberIds: [] },
|
||||
]);
|
||||
});
|
||||
|
||||
test('returns groups sorted by name', async () => {
|
||||
const groups = await methods.listGroups();
|
||||
|
||||
expect(groups).toHaveLength(3);
|
||||
expect(groups[0].name).toBe('Alpha');
|
||||
expect(groups[1].name).toBe('Beta');
|
||||
expect(groups[2].name).toBe('Gamma');
|
||||
});
|
||||
|
||||
test('filters by source', async () => {
|
||||
const groups = await methods.listGroups({ source: 'entra' });
|
||||
|
||||
expect(groups).toHaveLength(1);
|
||||
expect(groups[0].name).toBe('Gamma');
|
||||
});
|
||||
|
||||
test('filters by search (name)', async () => {
|
||||
const groups = await methods.listGroups({ search: 'alpha' });
|
||||
|
||||
expect(groups).toHaveLength(1);
|
||||
expect(groups[0].name).toBe('Alpha');
|
||||
});
|
||||
|
||||
test('filters by search (email)', async () => {
|
||||
const groups = await methods.listGroups({ search: 'beta@test' });
|
||||
|
||||
expect(groups).toHaveLength(1);
|
||||
expect(groups[0].name).toBe('Beta');
|
||||
});
|
||||
|
||||
test('filters by search (description)', async () => {
|
||||
const groups = await methods.listGroups({ search: 'first group' });
|
||||
|
||||
expect(groups).toHaveLength(1);
|
||||
expect(groups[0].name).toBe('Alpha');
|
||||
});
|
||||
|
||||
test('respects limit and offset', async () => {
|
||||
const groups = await methods.listGroups({ limit: 1, offset: 1 });
|
||||
|
||||
expect(groups).toHaveLength(1);
|
||||
expect(groups[0].name).toBe('Beta');
|
||||
});
|
||||
|
||||
test('returns empty for no matches', async () => {
|
||||
const groups = await methods.listGroups({ search: 'nonexistent' });
|
||||
|
||||
expect(groups).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('countGroups', () => {
|
||||
beforeEach(async () => {
|
||||
await Group.create([
|
||||
{ name: 'A', source: 'local', memberIds: [] },
|
||||
{ name: 'B', source: 'local', memberIds: [] },
|
||||
{ name: 'C', source: 'entra', idOnTheSource: 'ext-c', memberIds: [] },
|
||||
]);
|
||||
});
|
||||
|
||||
test('returns total count', async () => {
|
||||
const count = await methods.countGroups();
|
||||
|
||||
expect(count).toBe(3);
|
||||
});
|
||||
|
||||
test('respects source filter', async () => {
|
||||
const count = await methods.countGroups({ source: 'local' });
|
||||
|
||||
expect(count).toBe(2);
|
||||
});
|
||||
|
||||
test('respects search filter', async () => {
|
||||
const count = await methods.countGroups({ search: 'A' });
|
||||
|
||||
expect(count).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe('deleteGroup', () => {
|
||||
test('returns deleted group', async () => {
|
||||
const group = await Group.create({ name: 'ToDelete', source: 'local', memberIds: [] });
|
||||
|
||||
const deleted = await methods.deleteGroup(group._id as mongoose.Types.ObjectId);
|
||||
|
||||
expect(deleted).toBeDefined();
|
||||
expect(deleted?.name).toBe('ToDelete');
|
||||
const remaining = await Group.findById(group._id);
|
||||
expect(remaining).toBeNull();
|
||||
});
|
||||
|
||||
test('returns null for non-existent ID', async () => {
|
||||
const fakeId = new mongoose.Types.ObjectId();
|
||||
const result = await methods.deleteGroup(fakeId);
|
||||
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('removeMemberById', () => {
|
||||
test('removes member from memberIds array', async () => {
|
||||
const group = await Group.create({
|
||||
name: 'Test',
|
||||
source: 'local',
|
||||
memberIds: ['m1', 'm2', 'm3'],
|
||||
});
|
||||
|
||||
const updated = await methods.removeMemberById(
|
||||
group._id as mongoose.Types.ObjectId,
|
||||
'm2',
|
||||
);
|
||||
|
||||
expect(updated).toBeDefined();
|
||||
expect(updated?.memberIds).toEqual(['m1', 'm3']);
|
||||
});
|
||||
|
||||
test('is idempotent when memberId not present', async () => {
|
||||
const group = await Group.create({
|
||||
name: 'Test',
|
||||
source: 'local',
|
||||
memberIds: ['m1'],
|
||||
});
|
||||
|
||||
const updated = await methods.removeMemberById(
|
||||
group._id as mongoose.Types.ObjectId,
|
||||
'nonexistent',
|
||||
);
|
||||
|
||||
expect(updated).toBeDefined();
|
||||
expect(updated?.memberIds).toEqual(['m1']);
|
||||
});
|
||||
|
||||
test('returns null for non-existent group', async () => {
|
||||
const fakeId = new mongoose.Types.ObjectId();
|
||||
const result = await methods.removeMemberById(fakeId, 'any-id');
|
||||
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('sortPrincipalsByRelevance', () => {
|
||||
test('should sort principals by relevance score', async () => {
|
||||
const principals = [
|
||||
|
|
|
|||
|
|
@ -1,8 +1,9 @@
|
|||
import { Types } from 'mongoose';
|
||||
import { PrincipalType } from 'librechat-data-provider';
|
||||
import type { TUser, TPrincipalSearchResult } from 'librechat-data-provider';
|
||||
import type { Model, ClientSession } from 'mongoose';
|
||||
import type { Model, ClientSession, FilterQuery } from 'mongoose';
|
||||
import type { IGroup, IRole, IUser } from '~/types';
|
||||
import { escapeRegExp } from '~/utils/string';
|
||||
|
||||
export function createUserGroupMethods(mongoose: typeof import('mongoose')) {
|
||||
/**
|
||||
|
|
@ -14,7 +15,7 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) {
|
|||
*/
|
||||
async function findGroupById(
|
||||
groupId: string | Types.ObjectId,
|
||||
projection: Record<string, unknown> = {},
|
||||
projection: Record<string, 0 | 1> = {},
|
||||
session?: ClientSession,
|
||||
): Promise<IGroup | null> {
|
||||
const Group = mongoose.models.Group as Model<IGroup>;
|
||||
|
|
@ -36,7 +37,7 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) {
|
|||
async function findGroupByExternalId(
|
||||
idOnTheSource: string,
|
||||
source: 'entra' | 'local' = 'entra',
|
||||
projection: Record<string, unknown> = {},
|
||||
projection: Record<string, 0 | 1> = {},
|
||||
session?: ClientSession,
|
||||
): Promise<IGroup | null> {
|
||||
const Group = mongoose.models.Group as Model<IGroup>;
|
||||
|
|
@ -658,6 +659,97 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) {
|
|||
return Group.updateMany(filter, update, options || {});
|
||||
}
|
||||
|
||||
function buildGroupQuery(filter: {
|
||||
source?: 'local' | 'entra';
|
||||
search?: string;
|
||||
}): FilterQuery<IGroup> {
|
||||
const query: FilterQuery<IGroup> = {};
|
||||
if (filter.source) {
|
||||
query.source = filter.source;
|
||||
}
|
||||
if (filter.search) {
|
||||
const regex = new RegExp(escapeRegExp(filter.search), 'i');
|
||||
query.$or = [{ name: regex }, { email: regex }, { description: regex }];
|
||||
}
|
||||
return query;
|
||||
}
|
||||
|
||||
/**
|
||||
* List groups with optional source, search, and pagination filters.
|
||||
* Results are sorted by name.
|
||||
* @param filter - Optional filter with source, search, limit, and offset fields
|
||||
* @param session - Optional MongoDB session for transactions
|
||||
*/
|
||||
async function listGroups(
|
||||
filter: {
|
||||
source?: 'local' | 'entra';
|
||||
search?: string;
|
||||
limit?: number;
|
||||
offset?: number;
|
||||
} = {},
|
||||
session?: ClientSession,
|
||||
): Promise<IGroup[]> {
|
||||
const Group = mongoose.models.Group as Model<IGroup>;
|
||||
const query = buildGroupQuery(filter);
|
||||
const limit = filter.limit ?? 50;
|
||||
const offset = filter.offset ?? 0;
|
||||
return await Group.find(query)
|
||||
.sort({ name: 1 })
|
||||
.skip(offset)
|
||||
.limit(limit)
|
||||
.session(session ?? null)
|
||||
.lean();
|
||||
}
|
||||
|
||||
/**
|
||||
* Count groups matching optional source and search filters.
|
||||
* @param filter - Optional filter with source and search fields
|
||||
* @param session - Optional MongoDB session for transactions
|
||||
*/
|
||||
async function countGroups(
|
||||
filter: { source?: 'local' | 'entra'; search?: string } = {},
|
||||
session?: ClientSession,
|
||||
): Promise<number> {
|
||||
const Group = mongoose.models.Group as Model<IGroup>;
|
||||
const query = buildGroupQuery(filter);
|
||||
return await Group.countDocuments(query).session(session ?? null);
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete a group by its ID.
|
||||
* @param groupId - The group's ObjectId
|
||||
* @param session - Optional MongoDB session for transactions
|
||||
*/
|
||||
async function deleteGroup(
|
||||
groupId: string | Types.ObjectId,
|
||||
session?: ClientSession,
|
||||
): Promise<IGroup | null> {
|
||||
const Group = mongoose.models.Group as Model<IGroup>;
|
||||
const options = session ? { session } : {};
|
||||
return await Group.findByIdAndDelete(groupId, options).lean();
|
||||
}
|
||||
|
||||
/**
|
||||
* Remove a member from a group by raw memberId string ($pull from memberIds).
|
||||
* Unlike removeUserFromGroup, this does not look up the user first.
|
||||
* @param groupId - The group's ObjectId
|
||||
* @param memberId - The raw memberId string to remove (ObjectId or idOnTheSource)
|
||||
* @param session - Optional MongoDB session for transactions
|
||||
*/
|
||||
async function removeMemberById(
|
||||
groupId: string | Types.ObjectId,
|
||||
memberId: string,
|
||||
session?: ClientSession,
|
||||
): Promise<IGroup | null> {
|
||||
const Group = mongoose.models.Group as Model<IGroup>;
|
||||
const options = { new: true, ...(session ? { session } : {}) };
|
||||
return await Group.findByIdAndUpdate(
|
||||
groupId,
|
||||
{ $pull: { memberIds: memberId } },
|
||||
options,
|
||||
).lean();
|
||||
}
|
||||
|
||||
return {
|
||||
findGroupById,
|
||||
findGroupByExternalId,
|
||||
|
|
@ -677,6 +769,10 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) {
|
|||
searchPrincipals,
|
||||
calculateRelevanceScore,
|
||||
sortPrincipalsByRelevance,
|
||||
listGroups,
|
||||
countGroups,
|
||||
deleteGroup,
|
||||
removeMemberById,
|
||||
};
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue