From 261941c05f3929b0ae93c51173584130c117be8a Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 3 Apr 2026 10:24:11 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=A8=20fix:=20Custom=20Role=20Permissio?= =?UTF-8?q?ns=20(#12528)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Resolve custom role permissions not loading in frontend Users assigned to custom roles (non-USER/ADMIN) had all permission checks fail because AuthContext only fetched system role permissions. The roles map keyed by USER/ADMIN never contained the custom role name, so useHasAccess returned false for every feature gate. - Fetch the user's custom role in AuthContext and include it in the roles map so useHasAccess can resolve permissions correctly - Use encodeURIComponent instead of toLowerCase for role name URLs to preserve custom role casing through the API roundtrip - Only uppercase system role names on the backend GET route; pass custom role names through as-is for exact DB lookup - Allow users to fetch their own assigned role without READ_ROLES capability * refactor: Normalize all role names to uppercase Custom role names were stored in original casing, causing case-sensitivity bugs across the stack — URL lowercasing, route uppercasing, and case-sensitive DB lookups all conflicted for mixed-case custom roles. Enforce uppercase normalization at every boundary: - createRoleByName trims and uppercases the name before storage - createRoleHandler uppercases before passing to createRoleByName - All admin route handlers (get, update, delete, members, permissions) uppercase the :name URL param before DB lookups - addRoleMemberHandler uppercases before setting user.role - Startup migration (normalizeRoleNames) finds non-uppercase custom roles, renames them, and updates affected user.role values with collision detection Legacy GET /api/roles/:roleName retains always-uppercase behavior. Tests updated to expect uppercase role names throughout. * fix: Use case-preserved role names with strict equality Remove uppercase normalization — custom role names are stored and compared exactly as the user sets them, with only trimming applied. USER and ADMIN remain reserved case-insensitively via isSystemRoleName. - Remove toUpperCase from createRoleByName, createRoleHandler, and all admin route handlers (get, update, delete, members, permissions) - Remove toUpperCase from legacy GET and PUT routes in roles.js; the frontend now sends exact casing via encodeURIComponent - Remove normalizeRoleNames startup migration - Revert test expectations to original casing * fix: Format useMemo dependency array for Prettier * feat: Add custom role support to admin settings + review fixes - Add backend tests for isOwnRole authorization gate on GET /api/roles/:roleName - Add frontend tests for custom role detection and fetching in AuthContext - Fix transient null permission flash by only spreading custom role once loaded - Add isSystemRoleName helper to data-provider for case-insensitive system role detection - Use sentinel value in useGetRole to avoid ghost cache entry from empty string - Add useListRoles hook and listRoles data service for fetching all roles - Update AdminSettingsDialog and PeoplePickerAdminSettings to dynamically list custom roles in the role dropdown, with proper fallback defaults * fix: Address review findings for custom role permissions - Add assertions to AuthContext test verifying custom role in roles map - Fix empty array bypassing nullish coalescing fallback in role dropdowns - Add null/undefined guard to isSystemRoleName helper - Memoize role dropdown items to avoid unnecessary re-renders - Apply sentinel pattern to useGetRole in admin settings for consistency - Mark ListRolesResponse description as required to match schema * fix: Prevent prototype pollution in role authorization gate - Replace roleDefaults[roleName] with Object.hasOwn to prevent prototype chain bypass for names like constructor or __proto__ - Add dedicated rolesList query key to avoid cache collision when a custom role is named 'list' - Add regression test for prototype property name authorization * fix: Resolve Prettier formatting and unused variable lint errors * fix: Address review findings for custom role permissions - Add ADMIN self-read test documenting isOwnRole bypass behavior - Guard save button while custom role data loads to prevent data loss - Extract useRoleSelector hook eliminating ~55 lines of duplication - Unify defaultValues/useEffect permission resolution (fixes inconsistency) - Make ListRolesResponse.description and _id optional to match schema - Fix vacuous test assertions to verify sentinel calls exist - Only fetch userRole when user.role === USER (avoid unnecessary requests) - Remove redundant empty string guard in custom role detection * fix: Revert USER role fetch restriction to preserve admin settings Admins need the USER role loaded in AuthContext.roles so the admin settings dialog shows persisted USER permissions instead of defaults. * fix: Remove unused useEffect import from useRoleSelector * fix: Clean up useRoleSelector hook - Use existing isCustom variable instead of re-calling isSystemRoleName - Remove unused roles and availableRoleNames from return object * fix: Address review findings for custom role permissions - Use Set-based isSystemRoleName to auto-expand with future SystemRoles - Add isCustomRoleError handling: guard useEffect reset and disable Save - Remove resolvePermissions from hook return; use defaultValues in useEffect to eliminate redundant computation and stale-closure reset race - Rename customRoleName to userRoleName in AuthContext for clarity * fix: Request server-max roles for admin dropdown listRoles now passes limit=200 (the server's MAX_PAGE_LIMIT) so the admin role selector shows all roles instead of silently truncating at the default page size of 50. --------- Co-authored-by: Danny Avila --- api/server/routes/__tests__/roles.spec.js | 155 ++++++++++++++++++ api/server/routes/roles.js | 12 +- .../Sharing/PeoplePickerAdminSettings.tsx | 60 +++---- .../src/components/ui/AdminSettingsDialog.tsx | 59 +++---- client/src/data-provider/roles.ts | 12 ++ client/src/hooks/AuthContext.tsx | 20 ++- .../src/hooks/__tests__/AuthContext.spec.tsx | 136 ++++++++++++++- client/src/hooks/index.ts | 1 + client/src/hooks/useRoleSelector.ts | 64 ++++++++ packages/data-provider/src/api-endpoints.ts | 3 +- packages/data-provider/src/data-service.ts | 4 + packages/data-provider/src/keys.ts | 1 + packages/data-provider/src/roles.ts | 10 ++ packages/data-provider/src/types/queries.ts | 7 + 14 files changed, 462 insertions(+), 82 deletions(-) create mode 100644 api/server/routes/__tests__/roles.spec.js create mode 100644 client/src/hooks/useRoleSelector.ts diff --git a/api/server/routes/__tests__/roles.spec.js b/api/server/routes/__tests__/roles.spec.js new file mode 100644 index 0000000000..e78965d460 --- /dev/null +++ b/api/server/routes/__tests__/roles.spec.js @@ -0,0 +1,155 @@ +const express = require('express'); +const request = require('supertest'); +const { SystemRoles, roleDefaults } = require('librechat-data-provider'); + +const mockGetRoleByName = jest.fn(); +const mockHasCapability = jest.fn(); + +jest.mock('~/server/middleware', () => ({ + requireJwtAuth: (_req, _res, next) => next(), +})); + +jest.mock('~/server/middleware/roles/capabilities', () => ({ + hasCapability: (...args) => mockHasCapability(...args), + requireCapability: () => (_req, _res, next) => next(), +})); + +jest.mock('~/models', () => ({ + getRoleByName: (...args) => mockGetRoleByName(...args), + updateRoleByName: jest.fn(), +})); + +const rolesRouter = require('../roles'); + +function createApp(user) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.user = user; + next(); + }); + app.use('/api/roles', rolesRouter); + return app; +} + +const staffRole = { + name: 'STAFF', + permissions: { + PROMPTS: { USE: true, CREATE: false }, + }, +}; + +const userRole = roleDefaults[SystemRoles.USER]; +const adminRole = roleDefaults[SystemRoles.ADMIN]; + +beforeEach(() => { + jest.clearAllMocks(); + mockHasCapability.mockResolvedValue(false); + mockGetRoleByName.mockResolvedValue(null); +}); + +describe('GET /api/roles/:roleName — isOwnRole authorization', () => { + it('allows a custom role user to fetch their own role', async () => { + mockGetRoleByName.mockResolvedValue(staffRole); + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/STAFF'); + + expect(res.status).toBe(200); + expect(res.body.name).toBe('STAFF'); + expect(mockGetRoleByName).toHaveBeenCalledWith('STAFF', '-_id -__v'); + }); + + it('returns 403 when a custom role user requests a different custom role', async () => { + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/MANAGER'); + + expect(res.status).toBe(403); + expect(mockGetRoleByName).not.toHaveBeenCalled(); + }); + + it('returns 403 when a custom role user requests ADMIN', async () => { + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/ADMIN'); + + expect(res.status).toBe(403); + expect(mockGetRoleByName).not.toHaveBeenCalled(); + }); + + it('allows USER to fetch the USER role (roleDefaults key)', async () => { + mockGetRoleByName.mockResolvedValue(userRole); + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get(`/api/roles/${SystemRoles.USER}`); + + expect(res.status).toBe(200); + }); + + it('returns 403 when USER requests the ADMIN role', async () => { + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get(`/api/roles/${SystemRoles.ADMIN}`); + + expect(res.status).toBe(403); + }); + + it('allows ADMIN user to fetch their own ADMIN role via isOwnRole', async () => { + mockHasCapability.mockResolvedValue(false); + mockGetRoleByName.mockResolvedValue(adminRole); + const app = createApp({ id: 'u1', role: SystemRoles.ADMIN }); + + const res = await request(app).get(`/api/roles/${SystemRoles.ADMIN}`); + + expect(res.status).toBe(200); + }); + + it('allows any user with READ_ROLES capability to fetch any role', async () => { + mockHasCapability.mockResolvedValue(true); + mockGetRoleByName.mockResolvedValue(staffRole); + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get('/api/roles/STAFF'); + + expect(res.status).toBe(200); + expect(res.body.name).toBe('STAFF'); + }); + + it('returns 404 when the requested role does not exist', async () => { + mockGetRoleByName.mockResolvedValue(null); + const app = createApp({ id: 'u1', role: 'GHOST' }); + + const res = await request(app).get('/api/roles/GHOST'); + + expect(res.status).toBe(404); + }); + + it('returns 500 when getRoleByName throws', async () => { + mockGetRoleByName.mockRejectedValue(new Error('db error')); + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get(`/api/roles/${SystemRoles.USER}`); + + expect(res.status).toBe(500); + }); + + it('returns 403 for prototype property names like constructor (no prototype pollution)', async () => { + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/constructor'); + + expect(res.status).toBe(403); + expect(mockGetRoleByName).not.toHaveBeenCalled(); + }); + + it('treats hasCapability failure as no capability (does not 500)', async () => { + mockHasCapability.mockRejectedValue(new Error('capability check failed')); + const app = createApp({ id: 'u1', role: 'STAFF' }); + mockGetRoleByName.mockResolvedValue(staffRole); + + const res = await request(app).get('/api/roles/STAFF'); + + expect(res.status).toBe(200); + }); +}); diff --git a/api/server/routes/roles.js b/api/server/routes/roles.js index 25ee47854d..456d144af1 100644 --- a/api/server/routes/roles.js +++ b/api/server/routes/roles.js @@ -71,9 +71,7 @@ const createPermissionUpdateHandler = (permissionKey) => { const config = permissionConfigs[permissionKey]; return async (req, res) => { - const { roleName: _r } = req.params; - // TODO: TEMP, use a better parsing for roleName - const roleName = _r.toUpperCase(); + const { roleName } = req.params; const updates = req.body; try { @@ -110,9 +108,7 @@ const createPermissionUpdateHandler = (permissionKey) => { * Get a specific role by name */ router.get('/:roleName', async (req, res) => { - const { roleName: _r } = req.params; - // TODO: TEMP, use a better parsing for roleName - const roleName = _r.toUpperCase(); + const { roleName } = req.params; try { let hasReadRoles = false; @@ -121,7 +117,9 @@ router.get('/:roleName', async (req, res) => { } catch (err) { logger.warn(`[GET /roles/:roleName] capability check failed: ${err.message}`); } - if (!hasReadRoles && (roleName === SystemRoles.ADMIN || !roleDefaults[roleName])) { + const isOwnRole = req.user?.role === roleName; + const isDefaultRole = Object.hasOwn(roleDefaults, roleName); + if (!hasReadRoles && !isOwnRole && (roleName === SystemRoles.ADMIN || !isDefaultRole)) { return res.status(403).send({ message: 'Unauthorized' }); } diff --git a/client/src/components/Sharing/PeoplePickerAdminSettings.tsx b/client/src/components/Sharing/PeoplePickerAdminSettings.tsx index 38d44ad311..5f334f3764 100644 --- a/client/src/components/Sharing/PeoplePickerAdminSettings.tsx +++ b/client/src/components/Sharing/PeoplePickerAdminSettings.tsx @@ -1,8 +1,8 @@ -import { useMemo, useEffect, useState } from 'react'; +import { useEffect, useState } from 'react'; import * as Ariakit from '@ariakit/react'; import { ShieldEllipsis } from 'lucide-react'; import { useForm, Controller } from 'react-hook-form'; -import { Permissions, SystemRoles, roleDefaults, PermissionTypes } from 'librechat-data-provider'; +import { Permissions, SystemRoles, PermissionTypes } from 'librechat-data-provider'; import { Button, Switch, @@ -15,7 +15,7 @@ import { } from '@librechat/client'; import type { Control, UseFormSetValue, UseFormGetValues } from 'react-hook-form'; import { useUpdatePeoplePickerPermissionsMutation } from '~/data-provider'; -import { useLocalize, useAuthContext } from '~/hooks'; +import { useLocalize, useAuthContext, useRoleSelector } from '~/hooks'; type FormValues = { [Permissions.VIEW_USERS]: boolean; @@ -70,7 +70,7 @@ const LabelController: React.FC = ({ const PeoplePickerAdminSettings = () => { const localize = useLocalize(); const { showToast } = useToastContext(); - const { user, roles } = useAuthContext(); + const { user } = useAuthContext(); const { mutate, isLoading } = useUpdatePeoplePickerPermissionsMutation({ onSuccess: () => { showToast({ status: 'success', message: localize('com_ui_saved') }); @@ -81,15 +81,14 @@ const PeoplePickerAdminSettings = () => { }); const [isRoleMenuOpen, setIsRoleMenuOpen] = useState(false); - const [selectedRole, setSelectedRole] = useState(SystemRoles.USER); - - const defaultValues = useMemo(() => { - const rolePerms = roles?.[selectedRole]?.permissions; - if (rolePerms) { - return rolePerms[PermissionTypes.PEOPLE_PICKER]; - } - return roleDefaults[selectedRole].permissions[PermissionTypes.PEOPLE_PICKER]; - }, [roles, selectedRole]); + const { + selectedRole, + isSelectedCustomRole, + isCustomRoleLoading, + isCustomRoleError, + defaultValues, + roleDropdownItems, + } = useRoleSelector(PermissionTypes.PEOPLE_PICKER); const { reset, @@ -100,17 +99,15 @@ const PeoplePickerAdminSettings = () => { formState: { isSubmitting }, } = useForm({ mode: 'onChange', - defaultValues, + defaultValues: defaultValues as FormValues, }); useEffect(() => { - const value = roles?.[selectedRole]?.permissions?.[PermissionTypes.PEOPLE_PICKER]; - if (value) { - reset(value); - } else { - reset(roleDefaults[selectedRole].permissions[PermissionTypes.PEOPLE_PICKER]); + if (isSelectedCustomRole && (isCustomRoleLoading || isCustomRoleError)) { + return; } - }, [roles, selectedRole, reset]); + reset(defaultValues as FormValues); + }, [isSelectedCustomRole, isCustomRoleLoading, isCustomRoleError, defaultValues, reset]); if (user?.role !== SystemRoles.ADMIN) { return null; @@ -138,21 +135,6 @@ const PeoplePickerAdminSettings = () => { mutate({ roleName: selectedRole, updates: data }); }; - const roleDropdownItems = [ - { - label: SystemRoles.USER, - onClick: () => { - setSelectedRole(SystemRoles.USER); - }, - }, - { - label: SystemRoles.ADMIN, - onClick: () => { - setSelectedRole(SystemRoles.ADMIN); - }, - }, - ]; - return ( @@ -179,7 +161,7 @@ const PeoplePickerAdminSettings = () => { isOpen={isRoleMenuOpen} setIsOpen={setIsRoleMenuOpen} trigger={ - + {selectedRole} } @@ -207,7 +189,11 @@ const PeoplePickerAdminSettings = () => {