mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-03 14:27:20 +02:00
5972a21479
* feat: add createRole and deleteRole methods to role
* feat: add admin roles handler factory and Express routes
* fix: address convention violations in admin roles handlers
* fix: rename createRole/deleteRole to avoid AccessRole name collision
The existing accessRole.ts already exports createRole/deleteRole for the
AccessRole model. In createMethods index.ts, these are spread after
roleMethods, overwriting them. Renamed our Role methods to
createRoleByName/deleteRoleByName to match the existing pattern
(getRoleByName, updateRoleByName) and avoid the collision.
* feat: add description field to Role model
- Add description to IRole, CreateRoleRequest, UpdateRoleRequest types
- Add description field to Mongoose roleSchema (default: '')
- Wire description through createRoleHandler and updateRoleHandler
- Include description in listRoles select clause so it appears in list
* fix: address Copilot review findings in admin roles handlers
* test: add unit tests for admin roles and groups handlers
* test: add data-layer tests for createRoleByName, deleteRoleByName, listUsersByRole
* fix: allow system role updates when name is unchanged
The updateRoleHandler guard rejected any request where body.name matched
a system role, even when the name was not being changed. This blocked
editing a system role's description. Compare against the URL param to
only reject actual renames to reserved names.
* fix: address external review findings for admin roles
- Block renaming system roles (ADMIN/USER) and add user migration on rename
- Add input validation: name max-length, trim on update, duplicate name check
- Replace fragile String.includes error matching with prefix-based classification
- Catch MongoDB 11000 duplicate key in createRoleByName
- Add pagination (limit/offset/total) to getRoleMembersHandler
- Reverse delete order in deleteRoleByName — reassign users before deletion
- Add role existence check in removeRoleMember; drop unused createdAt select
- Add Array.isArray guard for permissions input; use consistent ?? coalescing
- Fix import ordering per AGENTS.md conventions
- Type-cast mongoose.models.User as Model<IUser> for proper TS inference
- Add comprehensive tests: rename guards, pagination, validation, 500 paths
* fix: address re-review findings for admin roles
- Gate deleteRoleByName on existence check — skip user reassignment and
cache invalidation when role doesn't exist (fixes test mismatch)
- Reverse rename order: migrate users before renaming role so a migration
failure leaves the system in a consistent state
- Add .sort({ _id: 1 }) to listUsersByRole for deterministic pagination
- Import shared AdminMember type from data-schemas instead of local copy;
make joinedAt optional since neither groups nor roles populate it
- Change IRole.description from optional to required to match schema default
- Add data-layer tests for updateUsersByRole and countUsersByRole
- Add handler test verifying users-first rename ordering and migration
failure safety
* fix: add rollback on rename failure and update PR description
- Roll back user migration if updateRoleByName returns null during a
rename (race: role deleted between existence check and update)
- Add test verifying rollback calls updateUsersByRole in reverse
- Update PR #12400 description to reflect current test counts (56
handler tests, 40 data-layer tests) and safety features
* fix: rollback on rename throw, description validation, delete/DRY cleanup
- Hoist isRename/trimmedName above try block so catch can roll back user
migration when updateRoleByName throws (not just returns null)
- Add description type + max-length (2000) validation in create and update,
consistent with groups handler
- Remove redundant getRoleByName existence check in deleteRoleHandler —
use deleteRoleByName return value directly
- Skip no-op name write when body.name equals current name (use isRename)
- Extract getUserModel() accessor to DRY repeated Model<IUser> casts
- Use name.trim() consistently in createRoleByName error messages
- Add tests: rename-throw rollback, description validation (create+update),
update delete test mocks to match simplified handler
* fix: guard spurious rollback, harden createRole error path, validate before DB calls
- Add migrationRan flag to prevent rollback of user migration that never ran
- Return generic message on 500 in createRoleHandler, specific only for 409
- Move description validation before DB queries in updateRoleHandler
- Return existing role early when update body has no changes
- Wrap cache.set in createRoleByName with try/catch to prevent masking DB success
- Add JSDoc on 11000 catch explaining compound unique index
- Add tests: spurious rollback guard, empty update body, description validation
ordering, listUsersByRole pagination
* fix: validate permissions in create, RoleConflictError, rollback safety, cache consistency
- Add permissions type/array validation in createRoleHandler
- Introduce RoleConflictError class replacing fragile string-prefix matching
- Wrap rollback in !role null path with try/catch for correct 404 response
- Wrap deleteRoleByName cache.set in try/catch matching createRoleByName
- Narrow updateRoleHandler body type to { name?, description? }
- Add tests: non-string description in create, rollback failure logging,
permissions array rejection, description max-length assertion fix
* feat: prevent removing the last admin user
Add guard in removeRoleMember that checks countUsersByRole before
demoting an ADMIN user, returning 400 if they are the last one.
* fix: move interleaved export below imports, add await to countUsersByRole
* fix: paginate listRoles, null-guard permissions handler, fix export ordering
- Add limit/offset/total pagination to listRoles matching the groups pattern
- Add countRoles data-layer method
- Omit permissions from listRoles select (getRole returns full document)
- Null-guard re-fetched role in updateRolePermissionsHandler
- Move interleaved export below all imports in methods/index.ts
* fix: address review findings — race safety, validation DRY, type accuracy, test coverage
- Add post-write admin count verification in removeRoleMember to prevent
zero-admin race condition (TOCTOU → rollback if count hits 0)
- Make IRole.description optional; backfill in initializeRoles for
pre-existing roles that lack the field (.lean() bypasses defaults)
- Extract parsePagination, validateNameParam, validateRoleName, and
validateDescription helpers to eliminate duplicated validation
- Add validateNameParam guard to all 7 handlers reading req.params.name
- Catch 11000 in updateRoleByName and surface as 409 via RoleConflictError
- Add idempotent skip in addRoleMember when user already has target role
- Verify updateRolePermissions test asserts response body
- Add data-layer tests: listRoles sort/pagination/projection, countRoles,
and createRoleByName 11000 duplicate key race
* fix: defensive rollback in removeRoleMember, type/style cleanup, test coverage
- Wrap removeRoleMember post-write admin rollback in try/catch so a
transient DB failure cannot leave the system with zero administrators
- Replace double `as unknown[] as IRole[]` cast with `.lean<IRole[]>()`
- Type parsePagination param explicitly; extract DEFAULT/MAX page constants
- Preserve original error cause in updateRoleByName re-throw
- Add test for rollback failure path in removeRoleMember (returns 400)
- Add test for pre-existing roles missing description field (.lean())
* chore: bump @librechat/data-schemas to 0.0.47
* fix: stale cache on rename, extract renameRole helper, shared pagination, cleanup
- Fix updateRoleByName cache bug: invalidate old key and populate new key
when updates.name differs from roleName (prevents stale cache after rename)
- Extract renameRole helper to eliminate mutable outer-scope state flags
(isRename, trimmedName, migrationRan) in updateRoleHandler
- Unify system-role protection to 403 for both rename-from and rename-to
- Extract parsePagination to shared admin/pagination.ts; use in both
roles.ts and groups.ts
- Extract name.trim() to local const in createRoleByName (was called 5×)
- Remove redundant findOne pre-check in deleteRoleByName
- Replace getUserModel closure with local const declarations
- Remove redundant description ?? '' in createRoleHandler (schema default)
- Add doc comment on updateRolePermissionsHandler noting cache dependency
- Add data-layer tests for cache rename behavior (old key null, new key set)
* fix: harden role guards, add User.role index, validate names, improve tests
- Add index on User.role field for efficient member queries at scale
- Replace fragile SystemRoles key lookup with value-based Set check (6 sites)
- Elevate rename rollback failure logging to CRITICAL (matches removeRoleMember)
- Guard removeRoleMember against non-ADMIN system roles (403 for USER)
- Fix parsePagination limit=0 gotcha: use parseInt + NaN check instead of ||
- Add control character and reserved path segment validation to role names
- Simplify validateRoleName: remove redundant casts and dead conditions
- Add JSDoc to deleteRoleByName documenting non-atomic window
- Split mixed value+type import in methods/index.ts per AGENTS.md
- Add 9 new tests: permissions assertion, combined rename+desc, createRole
with permissions, pagination edge cases, control char/reserved name
rejection, system role removeRoleMember guard
* fix: exact-case reserved name check, consistent validation, cleaner createRole
- Remove .toLowerCase() from reserved name check so only exact matches
(members, permissions) are rejected, not legitimate names like "Members"
- Extract trimmed const in validateRoleName for consistent validation
- Add control char check to validateNameParam for parity with body validation
- Build createRole roleData conditionally to avoid passing description: undefined
- Expand deleteRoleByName JSDoc documenting self-healing design and no-op trade-off
* fix: scope rename rollback to only migrated users, prevent cross-role corruption
Capture user IDs before forward migration so the rollback path only
reverts users this request actually moved. Previously the rollback called
updateUsersByRole(newName, currentName) which would sweep all users with
the new role — including any independently assigned by a concurrent admin
request — causing silent cross-role data corruption.
Adds findUserIdsByRole and updateUsersRoleByIds to the data layer.
Extracts rollbackMigratedUsers helper to deduplicate rollback sites.
* fix: guard last admin in addRoleMember to prevent zero-admin lockout
Since each user has exactly one role, addRoleMember implicitly removes
the user from their current role. Without a guard, reassigning the sole
admin to a non-admin role leaves zero admins and locks out admin
management. Adds the same countUsersByRole check used in removeRoleMember.
* fix: wire findUserIdsByRole and updateUsersRoleByIds into roles route
The scoped rollback deps added in c89b5db were missing from the route
DI wiring, causing renameRole to call undefined and return a 500.
* fix: post-write admin guard in addRoleMember, compound role index, review cleanup
- Add post-write admin count check + rollback to addRoleMember to match
removeRoleMember's two-phase TOCTOU protection (prevents zero-admin via
concurrent requests)
- Replace single-field User.role index with compound { role: 1, tenantId: 1 }
to align with existing multi-tenant index pattern (email, OAuth IDs)
- Narrow listRoles dep return type to RoleListItem (projected fields only)
- Refactor validateDescription to early-return style per AGENTS.md
- Remove redundant double .lean() in updateRoleByName
- Document rename snapshot race window in renameRole JSDoc
- Document cache null-set behavior in deleteRoleByName
- Add routing-coupling comment on RESERVED_ROLE_NAMES
- Add test for addRoleMember post-write rollback
* fix: review cleanup — system-role guard, type safety, JSDoc accuracy, tests
- Add system-role guard to addRoleMember: block direct assignment to
non-ADMIN system roles (403), symmetric with removeRoleMember
- Fix RESERVED_ROLE_NAMES comment: explain semantic URL ambiguity, not
a routing conflict (Express resolves single vs multi-segment correctly)
- Replace _id: unknown with Types.ObjectId | string per AGENTS.md
- Narrow listRoles data-layer return type to Pick<IRole, 'name' | 'description'>
to match the actual .select() projection
- Move updateRoleHandler param check inside try/catch for consistency
- Include user IDs in all CRITICAL rollback failure logs for operator recovery
- Clarify deleteRoleByName JSDoc: replace "self-healing" with "idempotent",
document that recovery requires caller retry
- Add tests: system-role guard, promote non-admin to ADMIN,
findUserIdsByRole throw prevents migration
* fix: include _id in listRoles return type to match RoleListItem
Pick<IRole, 'name' | 'description'> omits _id, making it incompatible
with the handler dep's RoleListItem which requires _id.
* fix: case-insensitive system role guard, reject null permissions, check updateUser result
- System role name checks now use case-insensitive comparison via
toUpperCase() — prevents creating 'admin' or 'user' which would
collide with the legacy roles route that uppercases params
- Reject permissions: null in createRole (typeof null === 'object'
was bypassing the validation)
- Check updateUser return in addRoleMember — return 404 if the user
was deleted between the findUser and updateUser calls
* fix: check updateUser return in removeRoleMember for concurrent delete safety
---------
Co-authored-by: Danny Avila <danny@librechat.ai>
43 lines
1.9 KiB
JavaScript
43 lines
1.9 KiB
JavaScript
const express = require('express');
|
|
const { createAdminRolesHandlers } = require('@librechat/api');
|
|
const { SystemCapabilities } = require('@librechat/data-schemas');
|
|
const { requireCapability } = require('~/server/middleware/roles/capabilities');
|
|
const { requireJwtAuth } = require('~/server/middleware');
|
|
const db = require('~/models');
|
|
|
|
const router = express.Router();
|
|
|
|
const requireAdminAccess = requireCapability(SystemCapabilities.ACCESS_ADMIN);
|
|
const requireReadRoles = requireCapability(SystemCapabilities.READ_ROLES);
|
|
const requireManageRoles = requireCapability(SystemCapabilities.MANAGE_ROLES);
|
|
|
|
const handlers = createAdminRolesHandlers({
|
|
listRoles: db.listRoles,
|
|
countRoles: db.countRoles,
|
|
getRoleByName: db.getRoleByName,
|
|
createRoleByName: db.createRoleByName,
|
|
updateRoleByName: db.updateRoleByName,
|
|
updateAccessPermissions: db.updateAccessPermissions,
|
|
deleteRoleByName: db.deleteRoleByName,
|
|
findUser: db.findUser,
|
|
updateUser: db.updateUser,
|
|
updateUsersByRole: db.updateUsersByRole,
|
|
findUserIdsByRole: db.findUserIdsByRole,
|
|
updateUsersRoleByIds: db.updateUsersRoleByIds,
|
|
listUsersByRole: db.listUsersByRole,
|
|
countUsersByRole: db.countUsersByRole,
|
|
});
|
|
|
|
router.use(requireJwtAuth, requireAdminAccess);
|
|
|
|
router.get('/', requireReadRoles, handlers.listRoles);
|
|
router.post('/', requireManageRoles, handlers.createRole);
|
|
router.get('/:name', requireReadRoles, handlers.getRole);
|
|
router.patch('/:name', requireManageRoles, handlers.updateRole);
|
|
router.delete('/:name', requireManageRoles, handlers.deleteRole);
|
|
router.patch('/:name/permissions', requireManageRoles, handlers.updateRolePermissions);
|
|
router.get('/:name/members', requireReadRoles, handlers.getRoleMembers);
|
|
router.post('/:name/members', requireManageRoles, handlers.addRoleMember);
|
|
router.delete('/:name/members/:userId', requireManageRoles, handlers.removeRoleMember);
|
|
|
|
module.exports = router;
|