diff --git a/api/server/controllers/PermissionsController.js b/api/server/controllers/PermissionsController.js index 51993d083c..16930c5139 100644 --- a/api/server/controllers/PermissionsController.js +++ b/api/server/controllers/PermissionsController.js @@ -24,7 +24,7 @@ const { entraIdPrincipalFeatureEnabled, searchEntraIdPrincipals, } = require('~/server/services/GraphApiService'); -const { AclEntry, AccessRole } = require('~/db/models'); +const { Agent, AclEntry, AccessRole, User } = require('~/db/models'); /** * Generic controller for resource permission endpoints @@ -43,6 +43,28 @@ const validateResourceType = (resourceType) => { } }; +/** + * Removes an agent from the favorites of specified users (fire-and-forget). + * Both AGENT and REMOTE_AGENT resource types share the Agent collection. + * @param {string} resourceId - The agent's MongoDB ObjectId hex string + * @param {string[]} userIds - User ObjectId strings whose favorites should be cleaned + */ +const removeRevokedAgentFromFavorites = (resourceId, userIds) => + Agent.findOne({ _id: resourceId }, { id: 1 }) + .lean() + .then((agent) => { + if (!agent) { + return; + } + return User.updateMany( + { _id: { $in: userIds }, 'favorites.agentId': agent.id }, + { $pull: { favorites: { agentId: agent.id } } }, + ); + }) + .catch((err) => { + logger.error('[removeRevokedAgentFromFavorites] Error cleaning up favorites', err); + }); + /** * Bulk update permissions for a resource (grant, update, remove) * @route PUT /api/{resourceType}/{resourceId}/permissions @@ -155,6 +177,16 @@ const updateResourcePermissions = async (req, res) => { grantedBy: userId, }); + const isAgentResource = + resourceType === ResourceType.AGENT || resourceType === ResourceType.REMOTE_AGENT; + const revokedUserIds = results.revoked + .filter((p) => p.type === PrincipalType.USER && p.id) + .map((p) => p.id); + + if (isAgentResource && revokedUserIds.length > 0) { + removeRevokedAgentFromFavorites(resourceId, revokedUserIds); + } + /** @type {TUpdateResourcePermissionsResponse} */ const response = { message: 'Permissions updated successfully', diff --git a/api/server/controllers/__tests__/PermissionsController.spec.js b/api/server/controllers/__tests__/PermissionsController.spec.js new file mode 100644 index 0000000000..840eaf0c30 --- /dev/null +++ b/api/server/controllers/__tests__/PermissionsController.spec.js @@ -0,0 +1,268 @@ +const mongoose = require('mongoose'); + +const mockLogger = { error: jest.fn(), warn: jest.fn(), info: jest.fn(), debug: jest.fn() }; + +jest.mock('@librechat/data-schemas', () => ({ + logger: mockLogger, +})); + +const { ResourceType, PrincipalType } = jest.requireActual('librechat-data-provider'); + +jest.mock('librechat-data-provider', () => ({ + ...jest.requireActual('librechat-data-provider'), +})); + +jest.mock('@librechat/api', () => ({ + enrichRemoteAgentPrincipals: jest.fn(), + backfillRemoteAgentPermissions: jest.fn(), +})); + +const mockBulkUpdateResourcePermissions = jest.fn(); + +jest.mock('~/server/services/PermissionService', () => ({ + bulkUpdateResourcePermissions: (...args) => mockBulkUpdateResourcePermissions(...args), + ensureGroupPrincipalExists: jest.fn(), + getEffectivePermissions: jest.fn(), + ensurePrincipalExists: jest.fn(), + getAvailableRoles: jest.fn(), + findAccessibleResources: jest.fn(), + getResourcePermissionsMap: jest.fn(), +})); + +jest.mock('~/models', () => ({ + searchPrincipals: jest.fn(), + sortPrincipalsByRelevance: jest.fn(), + calculateRelevanceScore: jest.fn(), +})); + +jest.mock('~/server/services/GraphApiService', () => ({ + entraIdPrincipalFeatureEnabled: jest.fn(() => false), + searchEntraIdPrincipals: jest.fn(), +})); + +const mockAgentFindOne = jest.fn(); +const mockUserUpdateMany = jest.fn(); + +jest.mock('~/db/models', () => ({ + Agent: { + findOne: (...args) => mockAgentFindOne(...args), + }, + AclEntry: {}, + AccessRole: {}, + User: { + updateMany: (...args) => mockUserUpdateMany(...args), + }, +})); + +const { updateResourcePermissions } = require('../PermissionsController'); + +const createMockReq = (overrides = {}) => ({ + params: { resourceType: ResourceType.AGENT, resourceId: '507f1f77bcf86cd799439011' }, + body: { updated: [], removed: [], public: false }, + user: { id: 'user-1', role: 'USER' }, + headers: { authorization: '' }, + ...overrides, +}); + +const createMockRes = () => { + const res = {}; + res.status = jest.fn().mockReturnValue(res); + res.json = jest.fn().mockReturnValue(res); + return res; +}; + +const flushPromises = () => new Promise((resolve) => setImmediate(resolve)); + +describe('PermissionsController', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('updateResourcePermissions — favorites cleanup', () => { + const agentObjectId = new mongoose.Types.ObjectId().toString(); + const revokedUserId = new mongoose.Types.ObjectId().toString(); + + beforeEach(() => { + mockBulkUpdateResourcePermissions.mockResolvedValue({ + granted: [], + updated: [], + revoked: [{ type: PrincipalType.USER, id: revokedUserId, name: 'Revoked User' }], + errors: [], + }); + + mockAgentFindOne.mockReturnValue({ + lean: () => Promise.resolve({ _id: agentObjectId, id: 'agent_abc123' }), + }); + mockUserUpdateMany.mockResolvedValue({ modifiedCount: 1 }); + }); + + it('removes agent from revoked users favorites on AGENT resource type', async () => { + const req = createMockReq({ + params: { resourceType: ResourceType.AGENT, resourceId: agentObjectId }, + body: { + updated: [], + removed: [{ type: PrincipalType.USER, id: revokedUserId }], + public: false, + }, + }); + const res = createMockRes(); + + await updateResourcePermissions(req, res); + await flushPromises(); + + expect(res.status).toHaveBeenCalledWith(200); + expect(mockAgentFindOne).toHaveBeenCalledWith({ _id: agentObjectId }, { id: 1 }); + expect(mockUserUpdateMany).toHaveBeenCalledWith( + { _id: { $in: [revokedUserId] }, 'favorites.agentId': 'agent_abc123' }, + { $pull: { favorites: { agentId: 'agent_abc123' } } }, + ); + }); + + it('removes agent from revoked users favorites on REMOTE_AGENT resource type', async () => { + const req = createMockReq({ + params: { resourceType: ResourceType.REMOTE_AGENT, resourceId: agentObjectId }, + body: { + updated: [], + removed: [{ type: PrincipalType.USER, id: revokedUserId }], + public: false, + }, + }); + const res = createMockRes(); + + await updateResourcePermissions(req, res); + await flushPromises(); + + expect(mockAgentFindOne).toHaveBeenCalledWith({ _id: agentObjectId }, { id: 1 }); + expect(mockUserUpdateMany).toHaveBeenCalled(); + }); + + it('uses results.revoked (validated) not raw request payload', async () => { + const validId = new mongoose.Types.ObjectId().toString(); + const invalidId = 'not-a-valid-id'; + + mockBulkUpdateResourcePermissions.mockResolvedValue({ + granted: [], + updated: [], + revoked: [{ type: PrincipalType.USER, id: validId }], + errors: [{ principal: { type: PrincipalType.USER, id: invalidId }, error: 'Invalid ID' }], + }); + + const req = createMockReq({ + params: { resourceType: ResourceType.AGENT, resourceId: agentObjectId }, + body: { + updated: [], + removed: [ + { type: PrincipalType.USER, id: validId }, + { type: PrincipalType.USER, id: invalidId }, + ], + public: false, + }, + }); + const res = createMockRes(); + + await updateResourcePermissions(req, res); + await flushPromises(); + + expect(mockUserUpdateMany).toHaveBeenCalledWith( + expect.objectContaining({ _id: { $in: [validId] } }), + expect.any(Object), + ); + }); + + it('skips cleanup when no USER principals are revoked', async () => { + mockBulkUpdateResourcePermissions.mockResolvedValue({ + granted: [], + updated: [], + revoked: [{ type: PrincipalType.GROUP, id: 'group-1' }], + errors: [], + }); + + const req = createMockReq({ + params: { resourceType: ResourceType.AGENT, resourceId: agentObjectId }, + body: { + updated: [], + removed: [{ type: PrincipalType.GROUP, id: 'group-1' }], + public: false, + }, + }); + const res = createMockRes(); + + await updateResourcePermissions(req, res); + await flushPromises(); + + expect(mockAgentFindOne).not.toHaveBeenCalled(); + expect(mockUserUpdateMany).not.toHaveBeenCalled(); + }); + + it('skips cleanup for non-agent resource types', async () => { + mockBulkUpdateResourcePermissions.mockResolvedValue({ + granted: [], + updated: [], + revoked: [{ type: PrincipalType.USER, id: revokedUserId }], + errors: [], + }); + + const req = createMockReq({ + params: { resourceType: ResourceType.PROMPTGROUP, resourceId: agentObjectId }, + body: { + updated: [], + removed: [{ type: PrincipalType.USER, id: revokedUserId }], + public: false, + }, + }); + const res = createMockRes(); + + await updateResourcePermissions(req, res); + await flushPromises(); + + expect(res.status).toHaveBeenCalledWith(200); + expect(mockAgentFindOne).not.toHaveBeenCalled(); + }); + + it('handles agent not found gracefully', async () => { + mockAgentFindOne.mockReturnValue({ + lean: () => Promise.resolve(null), + }); + + const req = createMockReq({ + params: { resourceType: ResourceType.AGENT, resourceId: agentObjectId }, + body: { + updated: [], + removed: [{ type: PrincipalType.USER, id: revokedUserId }], + public: false, + }, + }); + const res = createMockRes(); + + await updateResourcePermissions(req, res); + await flushPromises(); + + expect(mockAgentFindOne).toHaveBeenCalled(); + expect(mockUserUpdateMany).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(200); + }); + + it('logs error when User.updateMany fails without blocking response', async () => { + mockUserUpdateMany.mockRejectedValue(new Error('DB connection lost')); + + const req = createMockReq({ + params: { resourceType: ResourceType.AGENT, resourceId: agentObjectId }, + body: { + updated: [], + removed: [{ type: PrincipalType.USER, id: revokedUserId }], + public: false, + }, + }); + const res = createMockRes(); + + await updateResourcePermissions(req, res); + await flushPromises(); + + expect(res.status).toHaveBeenCalledWith(200); + expect(mockLogger.error).toHaveBeenCalledWith( + '[removeRevokedAgentFromFavorites] Error cleaning up favorites', + expect.any(Error), + ); + }); + }); +}); diff --git a/client/src/components/Nav/Favorites/FavoritesList.tsx b/client/src/components/Nav/Favorites/FavoritesList.tsx index 82225733fd..0ca23f8853 100644 --- a/client/src/components/Nav/Favorites/FavoritesList.tsx +++ b/client/src/components/Nav/Favorites/FavoritesList.tsx @@ -198,7 +198,8 @@ export default function FavoritesList({ } catch (error) { if (error && typeof error === 'object' && 'response' in error) { const axiosError = error as { response?: { status?: number } }; - if (axiosError.response?.status === 404) { + const status = axiosError.response?.status; + if (status === 404 || status === 403) { return { found: false }; } } @@ -206,10 +207,34 @@ export default function FavoritesList({ } }, staleTime: 1000 * 60 * 5, - enabled: missingAgentIds.length > 0, })), }); + const staleAgentIdsKey = useMemo(() => { + const ids: string[] = []; + for (let i = 0; i < missingAgentIds.length; i++) { + const query = missingAgentQueries[i]; + if (query.data && !query.data.found) { + ids.push(missingAgentIds[i]); + } + } + return ids.sort().join(','); + }, [missingAgentIds, missingAgentQueries]); + + const cleanupAttemptedRef = useRef(''); + + useEffect(() => { + if (!staleAgentIdsKey || cleanupAttemptedRef.current === staleAgentIdsKey) { + return; + } + const staleSet = new Set(staleAgentIdsKey.split(',')); + const cleaned = safeFavorites.filter((f) => !f.agentId || !staleSet.has(f.agentId)); + if (cleaned.length < safeFavorites.length) { + cleanupAttemptedRef.current = staleAgentIdsKey; + reorderFavorites(cleaned, true); + } + }, [staleAgentIdsKey, safeFavorites, reorderFavorites]); + const combinedAgentsMap = useMemo(() => { if (agentsMap === undefined) { return undefined; diff --git a/client/src/components/Nav/Favorites/tests/FavoritesList.spec.tsx b/client/src/components/Nav/Favorites/tests/FavoritesList.spec.tsx index ed71221de3..74228dc169 100644 --- a/client/src/components/Nav/Favorites/tests/FavoritesList.spec.tsx +++ b/client/src/components/Nav/Favorites/tests/FavoritesList.spec.tsx @@ -188,5 +188,86 @@ describe('FavoritesList', () => { // No favorite items should be rendered (deleted agent is filtered out) expect(queryAllByTestId('favorite-item')).toHaveLength(0); }); + + it('should treat 403 the same as 404 — agent not rendered', async () => { + const validAgent: t.Agent = { + id: 'valid-agent', + name: 'Valid Agent', + author: 'test-author', + } as t.Agent; + + mockFavorites.push({ agentId: 'valid-agent' }, { agentId: 'revoked-agent' }); + + (dataService.getAgentById as jest.Mock).mockImplementation( + ({ agent_id }: { agent_id: string }) => { + if (agent_id === 'valid-agent') { + return Promise.resolve(validAgent); + } + if (agent_id === 'revoked-agent') { + return Promise.reject({ response: { status: 403 } }); + } + return Promise.reject(new Error('Unknown agent')); + }, + ); + + const { findAllByTestId } = renderWithProviders(); + + const favoriteItems = await findAllByTestId('favorite-item'); + expect(favoriteItems).toHaveLength(1); + expect(favoriteItems[0]).toHaveTextContent('Valid Agent'); + }); + + it('should call reorderFavorites to persist removal of stale agents', async () => { + const mockReorderFavorites = jest.fn().mockResolvedValue(undefined); + mockUseFavorites.mockReturnValue({ + favorites: [{ agentId: 'revoked-agent' }], + reorderFavorites: mockReorderFavorites, + isLoading: false, + }); + + (dataService.getAgentById as jest.Mock).mockRejectedValue({ response: { status: 403 } }); + + renderWithProviders(); + + await waitFor(() => { + expect(mockReorderFavorites).toHaveBeenCalledWith([], true); + }); + }); + + it('should only attempt cleanup once even when favorites revert to stale state', async () => { + const mockReorderFavorites = jest.fn().mockResolvedValue(undefined); + + mockUseFavorites.mockReturnValue({ + favorites: [{ agentId: 'revoked-agent' }], + reorderFavorites: mockReorderFavorites, + isLoading: false, + }); + + (dataService.getAgentById as jest.Mock).mockRejectedValue({ response: { status: 403 } }); + + const { rerender } = renderWithProviders(); + + await waitFor(() => { + expect(mockReorderFavorites).toHaveBeenCalledWith([], true); + }); + + expect(mockReorderFavorites).toHaveBeenCalledTimes(1); + + rerender( + + + + + + + + + , + ); + + await new Promise((r) => setTimeout(r, 50)); + + expect(mockReorderFavorites).toHaveBeenCalledTimes(1); + }); }); });