diff --git a/api/server/controllers/UserController.js b/api/server/controllers/UserController.js index cdfc4601c5..710b2f3f6a 100644 --- a/api/server/controllers/UserController.js +++ b/api/server/controllers/UserController.js @@ -7,7 +7,6 @@ const { MCPTokenStorage, normalizeHttpError, extractWebSearchEnvVars, - ReauthenticationRequiredError, } = require('@librechat/api'); const { Tools, @@ -376,9 +375,48 @@ const resendVerificationController = async (req, res) => { } }; -/** - * OAuth MCP specific uninstall logic - */ +/** Best-effort cleanup of stored MCP OAuth tokens and flow state. */ +const clearStoredMCPOAuthState = async (userId, serverName) => { + try { + await MCPTokenStorage.deleteUserTokens({ + userId, + serverName, + deleteToken: async (filter) => { + await db.deleteTokens(filter); + }, + }); + } catch (error) { + logger.warn( + `[clearStoredMCPOAuthState] Failed to delete MCP OAuth tokens for ${serverName}:`, + error, + ); + } + + try { + const flowsCache = getLogStores(CacheKeys.FLOWS); + const flowManager = getFlowStateManager(flowsCache); + const flowId = MCPOAuthHandler.generateFlowId(userId, serverName); + const results = await Promise.allSettled([ + flowManager.deleteFlow(flowId, 'mcp_get_tokens'), + flowManager.deleteFlow(flowId, 'mcp_oauth'), + ]); + for (const result of results) { + if (result.status === 'rejected') { + logger.warn( + `[clearStoredMCPOAuthState] Failed to clear MCP OAuth flow state for ${serverName}:`, + result.reason, + ); + } + } + } catch (error) { + logger.warn( + `[clearStoredMCPOAuthState] Failed to clear MCP OAuth flow state for ${serverName}:`, + error, + ); + } +}; + +/** Revokes MCP OAuth tokens at the provider when possible, then clears local state. */ const maybeUninstallOAuthMCP = async (userId, pluginKey, appConfig) => { if (!pluginKey.startsWith(Constants.mcp_prefix)) { // this is not an MCP server, so nothing to do here @@ -390,29 +428,37 @@ const maybeUninstallOAuthMCP = async (userId, pluginKey, appConfig) => { (await getMCPServersRegistry().getServerConfig(serverName, userId)) ?? appConfig?.mcpServers?.[serverName]; const oauthServers = await getMCPServersRegistry().getOAuthServers(userId); - if (!oauthServers.has(serverName)) { - // this server does not use OAuth, so nothing to do here as well + if (!oauthServers.has(serverName) || !serverConfig) { + await clearStoredMCPOAuthState(userId, serverName); return; } // 1. get client info used for revocation (client id, secret) - const clientTokenData = await MCPTokenStorage.getClientInfoAndMetadata({ - userId, - serverName, - findToken: db.findToken, - }); + let clientTokenData = null; + try { + clientTokenData = await MCPTokenStorage.getClientInfoAndMetadata({ + userId, + serverName, + findToken: db.findToken, + }); + } catch (error) { + logger.warn( + `[maybeUninstallOAuthMCP] Unable to load OAuth client metadata for ${serverName}; clearing local MCP OAuth state only.`, + error, + ); + await clearStoredMCPOAuthState(userId, serverName); + return; + } if (clientTokenData == null) { + logger.info( + `[maybeUninstallOAuthMCP] Missing OAuth client metadata for ${serverName}; clearing local MCP OAuth state only.`, + ); + await clearStoredMCPOAuthState(userId, serverName); return; } const { clientInfo, clientMetadata } = clientTokenData; - // 2. get decrypted tokens before deletion. - // Token retrieval can throw ReauthenticationRequiredError (or other - // errors) when the refresh token is missing/expired — exactly the - // state that triggers a user-initiated revoke. Swallow it here so - // the DB and flow-state cleanup below always runs. Revocation is - // best-effort and the individual calls already wrap their own - // try/catch. + // 2. get decrypted tokens before deletion let tokens = null; try { tokens = await MCPTokenStorage.getTokens({ @@ -421,16 +467,10 @@ const maybeUninstallOAuthMCP = async (userId, pluginKey, appConfig) => { findToken: db.findToken, }); } catch (error) { - if (error instanceof ReauthenticationRequiredError) { - logger.info( - `[maybeUninstallOAuthMCP] No usable tokens for ${serverName} — skipping revocation, continuing cleanup`, - ); - } else { - logger.warn( - `[maybeUninstallOAuthMCP] Unexpected error retrieving tokens for ${serverName}:`, - error, - ); - } + logger.warn( + `[maybeUninstallOAuthMCP] Unable to load OAuth tokens for ${serverName}; clearing local token state.`, + error, + ); } // 3. revoke OAuth tokens at the provider @@ -459,7 +499,10 @@ const maybeUninstallOAuthMCP = async (userId, pluginKey, appConfig) => { allowedDomains, ); } catch (error) { - logger.error(`Error revoking OAuth access token for ${serverName}:`, error); + logger.error( + `[maybeUninstallOAuthMCP] Error revoking OAuth access token for ${serverName}:`, + error, + ); } } @@ -480,25 +523,15 @@ const maybeUninstallOAuthMCP = async (userId, pluginKey, appConfig) => { allowedDomains, ); } catch (error) { - logger.error(`Error revoking OAuth refresh token for ${serverName}:`, error); + logger.error( + `[maybeUninstallOAuthMCP] Error revoking OAuth refresh token for ${serverName}:`, + error, + ); } } - // 4. delete tokens from the DB after revocation attempts - await MCPTokenStorage.deleteUserTokens({ - userId, - serverName, - deleteToken: async (filter) => { - await db.deleteTokens(filter); - }, - }); - - // 5. clear the flow state for the OAuth tokens - const flowsCache = getLogStores(CacheKeys.FLOWS); - const flowManager = getFlowStateManager(flowsCache); - const flowId = MCPOAuthHandler.generateFlowId(userId, serverName); - await flowManager.deleteFlow(flowId, 'mcp_get_tokens'); - await flowManager.deleteFlow(flowId, 'mcp_oauth'); + // 4. delete tokens from the DB and clear the flow state after revocation attempts + await clearStoredMCPOAuthState(userId, serverName); }; module.exports = { diff --git a/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js b/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js new file mode 100644 index 0000000000..986ab712a1 --- /dev/null +++ b/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js @@ -0,0 +1,419 @@ +const mockUpdateUserPlugins = jest.fn(); +const mockFindToken = jest.fn(); +const mockDeleteUserPluginAuth = jest.fn(); +const mockGetAppConfig = jest.fn(); +const mockInvalidateCachedTools = jest.fn(); +const mockGetLogStores = jest.fn(); +const mockGetMCPManager = jest.fn(); +const mockGetFlowStateManager = jest.fn(); +const mockGetMCPServersRegistry = jest.fn(); + +jest.mock('@librechat/data-schemas', () => ({ + logger: { error: jest.fn(), info: jest.fn(), warn: jest.fn() }, + webSearchKeys: [], +})); + +jest.mock('librechat-data-provider', () => ({ + Tools: {}, + CacheKeys: { FLOWS: 'flows' }, + Constants: { mcp_delimiter: '_mcp_', mcp_prefix: 'mcp_' }, + FileSources: {}, +})); + +jest.mock('@librechat/api', () => ({ + MCPOAuthHandler: { + generateFlowId: jest.fn(() => 'user-1:test-server'), + revokeOAuthToken: jest.fn(), + }, + MCPTokenStorage: { + getClientInfoAndMetadata: jest.fn(), + getTokens: jest.fn(), + deleteUserTokens: jest.fn().mockResolvedValue(undefined), + }, + normalizeHttpError: jest.fn((error) => error), + extractWebSearchEnvVars: jest.fn((params) => params.keys), + needsRefresh: jest.fn(), + getNewS3URL: jest.fn(), +})); + +jest.mock('~/models', () => ({ + updateUserPlugins: (...args) => mockUpdateUserPlugins(...args), + findToken: mockFindToken, + deleteTokens: jest.fn(), +})); + +jest.mock('~/server/services/PluginService', () => ({ + updateUserPluginAuth: jest.fn(), + deleteUserPluginAuth: (...args) => mockDeleteUserPluginAuth(...args), +})); + +jest.mock('~/server/services/twoFactorService', () => ({ + verifyOTPOrBackupCode: jest.fn(), +})); + +jest.mock('~/server/services/AuthService', () => ({ + verifyEmail: jest.fn(), + resendVerificationEmail: jest.fn(), +})); + +jest.mock('~/config', () => ({ + getMCPManager: (...args) => mockGetMCPManager(...args), + getFlowStateManager: (...args) => mockGetFlowStateManager(...args), + getMCPServersRegistry: (...args) => mockGetMCPServersRegistry(...args), +})); + +jest.mock('~/server/services/Config/getCachedTools', () => ({ + invalidateCachedTools: (...args) => mockInvalidateCachedTools(...args), +})); + +jest.mock('~/server/services/Files/process', () => ({ + processDeleteRequest: jest.fn(), +})); + +jest.mock('~/server/services/Config', () => ({ + getAppConfig: (...args) => mockGetAppConfig(...args), +})); + +jest.mock('~/cache', () => ({ + getLogStores: (...args) => mockGetLogStores(...args), +})); + +const { logger } = require('@librechat/data-schemas'); +const { MCPTokenStorage, MCPOAuthHandler } = require('@librechat/api'); +const { updateUserPluginsController } = require('~/server/controllers/UserController'); + +function createResponse() { + const res = {}; + res.status = jest.fn().mockReturnValue(res); + res.json = jest.fn().mockReturnValue(res); + res.send = jest.fn().mockReturnValue(res); + return res; +} + +function createRequest() { + return { + user: { + id: 'user-1', + _id: 'user-1', + plugins: [], + role: 'USER', + }, + body: { + pluginKey: 'mcp_test-server', + action: 'uninstall', + auth: {}, + }, + }; +} + +function setupMCPMocks() { + const flowManager = { + deleteFlow: jest.fn().mockResolvedValue(true), + }; + const mcpManager = { + disconnectUserConnection: jest.fn().mockResolvedValue(), + }; + const registry = { + getServerConfig: jest.fn().mockResolvedValue({ + url: 'https://example.com/mcp', + oauth: {}, + oauth_headers: {}, + }), + getOAuthServers: jest.fn().mockResolvedValue(new Set(['test-server'])), + getAllowedDomains: jest.fn().mockReturnValue([]), + }; + + mockGetAppConfig.mockResolvedValue({}); + mockUpdateUserPlugins.mockResolvedValue(); + mockDeleteUserPluginAuth.mockResolvedValue(); + mockInvalidateCachedTools.mockResolvedValue(); + mockGetLogStores.mockReturnValue({}); + mockGetFlowStateManager.mockReturnValue(flowManager); + mockGetMCPManager.mockReturnValue(mcpManager); + mockGetMCPServersRegistry.mockReturnValue(registry); + + return { flowManager, mcpManager, registry }; +} + +beforeEach(() => { + jest.clearAllMocks(); +}); + +describe('updateUserPluginsController MCP OAuth cleanup', () => { + it('clears stored OAuth token state when client metadata is missing', async () => { + const { flowManager, mcpManager } = setupMCPMocks(); + MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue(null); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(MCPTokenStorage.getClientInfoAndMetadata).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + findToken: mockFindToken, + }); + expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + deleteToken: expect.any(Function), + }); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); + expect(mcpManager.disconnectUserConnection).toHaveBeenCalledWith('user-1', 'test-server'); + }); + + it('still clears OAuth flow state when stored token deletion fails', async () => { + const { flowManager } = setupMCPMocks(); + const cleanupError = new Error('DB down'); + MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue(null); + MCPTokenStorage.deleteUserTokens.mockRejectedValueOnce(cleanupError); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(logger.warn).toHaveBeenCalledWith( + '[clearStoredMCPOAuthState] Failed to delete MCP OAuth tokens for test-server:', + cleanupError, + ); + }); + + it('logs all flow cleanup failures without failing MCP OAuth cleanup', async () => { + const { flowManager } = setupMCPMocks(); + const getTokensFlowError = new Error('get tokens flow cache down'); + const oauthFlowError = new Error('oauth flow cache down'); + MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue(null); + flowManager.deleteFlow + .mockRejectedValueOnce(getTokensFlowError) + .mockRejectedValueOnce(oauthFlowError); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(logger.warn).toHaveBeenCalledWith( + '[clearStoredMCPOAuthState] Failed to clear MCP OAuth flow state for test-server:', + getTokensFlowError, + ); + expect(logger.warn).toHaveBeenCalledWith( + '[clearStoredMCPOAuthState] Failed to clear MCP OAuth flow state for test-server:', + oauthFlowError, + ); + }); + + it('clears stored OAuth token state when client metadata cannot be loaded', async () => { + const { flowManager } = setupMCPMocks(); + MCPTokenStorage.getClientInfoAndMetadata.mockRejectedValue(new Error('invalid client info')); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(logger.warn).toHaveBeenCalledWith( + '[maybeUninstallOAuthMCP] Unable to load OAuth client metadata for test-server; clearing local MCP OAuth state only.', + expect.any(Error), + ); + expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + deleteToken: expect.any(Function), + }); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPTokenStorage.getTokens).not.toHaveBeenCalled(); + expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); + }); + + it('clears stored OAuth token state when server config is missing', async () => { + const { flowManager, registry } = setupMCPMocks(); + registry.getServerConfig.mockResolvedValue(undefined); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + deleteToken: expect.any(Function), + }); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPTokenStorage.getClientInfoAndMetadata).not.toHaveBeenCalled(); + expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); + }); + + it('clears stored OAuth token state when server no longer requires OAuth', async () => { + const { flowManager, registry } = setupMCPMocks(); + registry.getOAuthServers.mockResolvedValue(new Set()); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + deleteToken: expect.any(Function), + }); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPTokenStorage.getClientInfoAndMetadata).not.toHaveBeenCalled(); + expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); + }); + + it('clears stored OAuth token state when token loading fails before provider revocation', async () => { + const { flowManager } = setupMCPMocks(); + MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue({ + clientInfo: { client_id: 'client-1' }, + clientMetadata: {}, + }); + MCPTokenStorage.getTokens.mockRejectedValue(new Error('token lookup failed')); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(MCPTokenStorage.getTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + findToken: mockFindToken, + }); + expect(logger.warn).toHaveBeenCalledWith( + '[maybeUninstallOAuthMCP] Unable to load OAuth tokens for test-server; clearing local token state.', + expect.any(Error), + ); + expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + deleteToken: expect.any(Function), + }); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens'); + expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth'); + expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled(); + }); + + it('revokes provider tokens before clearing local token state when token data is available', async () => { + setupMCPMocks(); + MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue({ + clientInfo: { client_id: 'client-1', client_secret: 'secret-1' }, + clientMetadata: { revocation_endpoint: 'https://example.com/revoke' }, + }); + MCPTokenStorage.getTokens.mockResolvedValue({ + access_token: 'access-token', + refresh_token: 'refresh-token', + }); + MCPOAuthHandler.revokeOAuthToken.mockResolvedValue(); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(MCPTokenStorage.getTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + findToken: mockFindToken, + }); + expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledWith( + 'test-server', + 'access-token', + 'access', + { + serverUrl: 'https://example.com/mcp', + clientId: 'client-1', + clientSecret: 'secret-1', + revocationEndpoint: 'https://example.com/revoke', + revocationEndpointAuthMethodsSupported: undefined, + }, + {}, + [], + ); + expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledWith( + 'test-server', + 'refresh-token', + 'refresh', + { + serverUrl: 'https://example.com/mcp', + clientId: 'client-1', + clientSecret: 'secret-1', + revocationEndpoint: 'https://example.com/revoke', + revocationEndpointAuthMethodsSupported: undefined, + }, + {}, + [], + ); + expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + deleteToken: expect.any(Function), + }); + }); + + it('revokes only the access token when refresh token data is absent', async () => { + setupMCPMocks(); + MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue({ + clientInfo: { client_id: 'client-1', client_secret: 'secret-1' }, + clientMetadata: {}, + }); + MCPTokenStorage.getTokens.mockResolvedValue({ + access_token: 'access-token', + }); + MCPOAuthHandler.revokeOAuthToken.mockResolvedValue(); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledTimes(1); + expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledWith( + 'test-server', + 'access-token', + 'access', + expect.objectContaining({ clientId: 'client-1' }), + {}, + [], + ); + expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + deleteToken: expect.any(Function), + }); + }); + + it('revokes only the refresh token when access token data is absent', async () => { + setupMCPMocks(); + MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue({ + clientInfo: { client_id: 'client-1', client_secret: 'secret-1' }, + clientMetadata: {}, + }); + MCPTokenStorage.getTokens.mockResolvedValue({ + refresh_token: 'refresh-token', + }); + MCPOAuthHandler.revokeOAuthToken.mockResolvedValue(); + + const res = createResponse(); + await updateUserPluginsController(createRequest(), res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledTimes(1); + expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledWith( + 'test-server', + 'refresh-token', + 'refresh', + expect.objectContaining({ clientId: 'client-1' }), + {}, + [], + ); + expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({ + userId: 'user-1', + serverName: 'test-server', + deleteToken: expect.any(Function), + }); + }); +}); diff --git a/api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js b/api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js index c478b80e1a..65e12cb5b9 100644 --- a/api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js +++ b/api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js @@ -19,12 +19,6 @@ jest.mock('@librechat/data-schemas', () => ({ })); jest.mock('@librechat/api', () => { - class ReauthenticationRequiredError extends Error { - constructor(serverName, reason) { - super(`Re-authentication required for "${serverName}": ${reason}`); - this.name = 'ReauthenticationRequiredError'; - } - } return { MCPOAuthHandler: { revokeOAuthToken: (...args) => mockRevokeOAuthToken(...args), @@ -35,7 +29,6 @@ jest.mock('@librechat/api', () => { getClientInfoAndMetadata: (...args) => mockGetClientInfoAndMetadata(...args), deleteUserTokens: (...args) => mockDeleteUserTokens(...args), }, - ReauthenticationRequiredError, normalizeHttpError: jest.fn(), extractWebSearchEnvVars: jest.fn(), needsRefresh: jest.fn(), @@ -124,7 +117,6 @@ jest.mock('~/models', () => ({ })); const { maybeUninstallOAuthMCP } = require('~/server/controllers/UserController'); -const { ReauthenticationRequiredError } = require('@librechat/api'); const userId = 'user-123'; const pluginKey = 'mcp_acme'; @@ -167,7 +159,7 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockDeleteFlow).not.toHaveBeenCalled(); }); - test('is a no-op when the MCP server is not an OAuth server', async () => { + test('clears stored state when the MCP server is not an OAuth server', async () => { mockGetServerConfig.mockResolvedValue(serverConfig); mockGetOAuthServers.mockResolvedValue(new Set(['other'])); @@ -175,18 +167,39 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockGetClientInfoAndMetadata).not.toHaveBeenCalled(); expect(mockGetTokens).not.toHaveBeenCalled(); - expect(mockDeleteUserTokens).not.toHaveBeenCalled(); + expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); + expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName }); + expect(mockDeleteFlow).toHaveBeenCalledTimes(2); }); - test('returns early when client info is missing', async () => { + test('clears stored state when client info is missing', async () => { setupOAuthServerFound(); mockGetClientInfoAndMetadata.mockResolvedValue(null); await maybeUninstallOAuthMCP(userId, pluginKey, appConfig); expect(mockGetTokens).not.toHaveBeenCalled(); - expect(mockDeleteUserTokens).not.toHaveBeenCalled(); - expect(mockDeleteFlow).not.toHaveBeenCalled(); + expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); + expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName }); + expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + }); + + test('clears stored state when client info cannot be loaded', async () => { + setupOAuthServerFound(); + mockGetClientInfoAndMetadata.mockRejectedValue(new Error('bad client data')); + mockDeleteUserTokens.mockResolvedValue(undefined); + mockDeleteFlow.mockResolvedValue(undefined); + + await maybeUninstallOAuthMCP(userId, pluginKey, appConfig); + + expect(mockGetTokens).not.toHaveBeenCalled(); + expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); + expect(mockDeleteUserTokens.mock.calls[0][0]).toMatchObject({ userId, serverName }); + expect(mockDeleteFlow).toHaveBeenCalledTimes(2); + expect(mockLoggerWarn).toHaveBeenCalledWith( + `[maybeUninstallOAuthMCP] Unable to load OAuth client metadata for ${serverName}; clearing local MCP OAuth state only.`, + expect.any(Error), + ); }); test('revokes both tokens and runs cleanup on happy path', async () => { @@ -215,9 +228,9 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockDeleteFlow.mock.calls[1][1]).toBe('mcp_oauth'); }); - test('skips revocation but still runs cleanup when getTokens throws ReauthenticationRequiredError', async () => { + test('skips revocation but still runs cleanup when token retrieval fails', async () => { setupOAuthServerFound(); - mockGetTokens.mockRejectedValue(new ReauthenticationRequiredError(serverName, 'missing')); + mockGetTokens.mockRejectedValue(new Error('missing')); mockDeleteUserTokens.mockResolvedValue(undefined); mockDeleteFlow.mockResolvedValue(undefined); @@ -226,7 +239,10 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockRevokeOAuthToken).not.toHaveBeenCalled(); expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); expect(mockDeleteFlow).toHaveBeenCalledTimes(2); - expect(mockLoggerInfo).toHaveBeenCalledWith(expect.stringContaining('No usable tokens')); + expect(mockLoggerWarn).toHaveBeenCalledWith( + `[maybeUninstallOAuthMCP] Unable to load OAuth tokens for ${serverName}; clearing local token state.`, + expect.any(Error), + ); }); test('skips revocation, logs warn, and still runs cleanup on unexpected token-retrieval error', async () => { @@ -241,7 +257,7 @@ describe('maybeUninstallOAuthMCP', () => { expect(mockDeleteUserTokens).toHaveBeenCalledTimes(1); expect(mockDeleteFlow).toHaveBeenCalledTimes(2); expect(mockLoggerWarn).toHaveBeenCalledWith( - expect.stringContaining('Unexpected error retrieving tokens'), + `[maybeUninstallOAuthMCP] Unable to load OAuth tokens for ${serverName}; clearing local token state.`, expect.any(Error), ); });