From 93952f06b42e35fe98069de6bc5e2621379c27e9 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 19 Mar 2026 15:15:10 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AF=20fix:=20Remove=20Revoked=20Agents?= =?UTF-8?q?=20from=20User=20Favorites=20(#12296)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🧯 fix: Remove revoked agents from user favorites When agent access is revoked, the agent remained in the user's favorites causing repeated 403 errors on page load. Backend now cleans up favorites on permission revocation; frontend treats 403 like 404 and auto-removes stale agent references. * 🧪 fix: Address review findings for stale agent favorites cleanup - Guard cleanup effect with ref to prevent infinite loop on mutation failure (Finding 1) - Use validated results.revoked instead of raw request payload for revokedUserIds (Finding 3) - Stabilize staleAgentIds memo with string key to avoid spurious re-evaluation during drag-drop (Finding 5) - Add JSDoc with param types to removeRevokedAgentFromFavorites (Finding 7) - Return promise from removeRevokedAgentFromFavorites for testability - Add 7 backend tests covering revocation cleanup paths - Add 3 frontend tests for 403 handling and stale cleanup persistence --- .../controllers/PermissionsController.js | 34 ++- .../__tests__/PermissionsController.spec.js | 268 ++++++++++++++++++ .../Nav/Favorites/FavoritesList.tsx | 29 +- .../Favorites/tests/FavoritesList.spec.tsx | 81 ++++++ 4 files changed, 409 insertions(+), 3 deletions(-) create mode 100644 api/server/controllers/__tests__/PermissionsController.spec.js 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); + }); }); });