From 72c20daa61adb2448caa02f7d4e807af35f6a84e Mon Sep 17 00:00:00 2001 From: zyssyz123 <916125788@qq.com> Date: Wed, 29 Jul 2026 20:39:06 +0800 Subject: [PATCH] fix(plugin): preserve credentials during local replacement (#39764) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --- api/controllers/console/workspace/plugin.py | 9 +- api/core/plugin/plugin_service.py | 104 ++++++++++-------- api/openapi/markdown/console-openapi.md | 1 + .../console/workspace/test_plugin.py | 15 ++- .../services/plugin/test_plugin_service.py | 9 ++ .../test_plugin_service_installation.py | 75 +++++++++++++ .../api/console/workspaces/types.gen.ts | 1 + .../api/console/workspaces/zod.gen.ts | 1 + .../steps/__tests__/install.spec.tsx | 6 +- .../steps/install.tsx | 4 +- web/app/components/plugins/types.ts | 4 - web/service/__tests__/use-plugins.spec.tsx | 57 +++++++++- web/service/plugins.spec.ts | 56 ++++++++++ web/service/plugins.ts | 14 ++- web/service/use-plugins.ts | 2 +- 15 files changed, 297 insertions(+), 61 deletions(-) create mode 100644 web/service/plugins.spec.ts diff --git a/api/controllers/console/workspace/plugin.py b/api/controllers/console/workspace/plugin.py index 98eb1192027..36ba2a621e1 100644 --- a/api/controllers/console/workspace/plugin.py +++ b/api/controllers/console/workspace/plugin.py @@ -150,6 +150,7 @@ class ParserGithubUpgrade(BaseModel): class ParserUninstall(BaseModel): plugin_installation_id: str + preserve_credentials: bool = False class ParserPermissionChange(BaseModel): @@ -971,7 +972,13 @@ class PluginUninstallApi(Resource): args = ParserUninstall.model_validate(console_ns.payload) try: - return {"success": PluginService.uninstall(tenant_id, args.plugin_installation_id)} + return { + "success": PluginService.uninstall( + tenant_id, + args.plugin_installation_id, + preserve_credentials=args.preserve_credentials, + ) + } except PluginDaemonClientSideError as e: return {"code": "plugin_error", "message": e.description}, 400 diff --git a/api/core/plugin/plugin_service.py b/api/core/plugin/plugin_service.py index 55738631891..08546c90845 100644 --- a/api/core/plugin/plugin_service.py +++ b/api/core/plugin/plugin_service.py @@ -1152,10 +1152,11 @@ class PluginService: return result @staticmethod - def uninstall(tenant_id: str, plugin_installation_id: str) -> bool: + def uninstall(tenant_id: str, plugin_installation_id: str, *, preserve_credentials: bool = False) -> bool: + """Uninstall a plugin and optionally retain model-provider credentials for replacement.""" manager = PluginInstaller() - # Get plugin info before uninstalling to delete associated credentials + # Resolve the trusted plugin ID before the daemon removes the installation record. plugins = manager.list_plugins(tenant_id) plugin = next((p for p in plugins if p.installation_id == plugin_installation_id), None) @@ -1172,59 +1173,74 @@ class PluginService: plugin_unique_identifier=plugin.plugin_unique_identifier, ) ) - with Session(db.engine) as session, session.begin(): + + result = manager.uninstall(tenant_id, plugin_installation_id) + if not result: + return False + + if preserve_credentials: + logger.info("Preserving credentials while replacing plugin: %s", plugin.plugin_id) + else: plugin_id = plugin.plugin_id - logger.info("Deleting credentials for plugin: %s", plugin_id) + logger.info("Deleting credentials after uninstalling plugin: %s", plugin_id) + provider_ids: Sequence[str] = [] - session.execute( - delete(TenantPreferredModelProvider).where( - TenantPreferredModelProvider.tenant_id == tenant_id, - TenantPreferredModelProvider.provider_name.like(f"{plugin_id}/%"), + with Session(db.engine) as session, session.begin(): + session.execute( + delete(TenantPreferredModelProvider).where( + TenantPreferredModelProvider.tenant_id == tenant_id, + TenantPreferredModelProvider.provider_name.like(f"{plugin_id}/%"), + ) ) - ) - # Delete provider credentials that match this plugin - credential_ids = session.scalars( - select(ProviderCredential.id).where( - ProviderCredential.tenant_id == tenant_id, - ProviderCredential.provider_name.like(f"{plugin_id}/%"), - ) - ).all() - - if not credential_ids: - logger.info("No credentials found for plugin: %s", plugin_id) - else: - provider_ids = session.scalars( - select(Provider.id).where( - Provider.tenant_id == tenant_id, - Provider.provider_name.like(f"{plugin_id}/%"), - Provider.credential_id.in_(credential_ids), + credential_ids = session.scalars( + select(ProviderCredential.id).where( + ProviderCredential.tenant_id == tenant_id, + ProviderCredential.provider_name.like(f"{plugin_id}/%"), ) ).all() - session.execute(update(Provider).where(Provider.id.in_(provider_ids)).values(credential_id=None)) + if not credential_ids: + logger.info("No credentials found for plugin: %s", plugin_id) + else: + provider_ids = session.scalars( + select(Provider.id).where( + Provider.tenant_id == tenant_id, + Provider.provider_name.like(f"{plugin_id}/%"), + Provider.credential_id.in_(credential_ids), + ) + ).all() - for provider_id in provider_ids: - ProviderCredentialsCache( - tenant_id=tenant_id, - identity_id=provider_id, - cache_type=ProviderCredentialsCacheType.PROVIDER, - ).delete() - - session.execute( - delete(ProviderCredential).where( - ProviderCredential.id.in_(credential_ids), + session.execute(update(Provider).where(Provider.id.in_(provider_ids)).values(credential_id=None)) + session.execute( + delete(ProviderCredential).where( + ProviderCredential.id.in_(credential_ids), + ) ) - ) - logger.info( - "Completed deleting credentials and cleaning provider associations for plugin: %s", - plugin_id, - ) + logger.info( + "Completed deleting credentials and cleaning provider associations for plugin: %s", + plugin_id, + ) - result = manager.uninstall(tenant_id, plugin_installation_id) - if result: - PluginService.invalidate_plugin_model_providers_cache(tenant_id) + for provider_id in provider_ids: + ProviderCredentialsCache( + tenant_id=tenant_id, + identity_id=provider_id, + cache_type=ProviderCredentialsCacheType.PROVIDER, + ).delete() + + from core.provider_manager import ProviderConfigurationCacheSource, ProviderManager + + ProviderManager.invalidate_configurations_cache( + tenant_id, + sources=( + ProviderConfigurationCacheSource.PREFERRED_MODEL_PROVIDERS, + ProviderConfigurationCacheSource.PROVIDER_CREDENTIALS, + ), + ) + + PluginService.invalidate_plugin_model_providers_cache(tenant_id) return result @staticmethod diff --git a/api/openapi/markdown/console-openapi.md b/api/openapi/markdown/console-openapi.md index e83c01ab291..681dfa266ee 100644 --- a/api/openapi/markdown/console-openapi.md +++ b/api/openapi/markdown/console-openapi.md @@ -20083,6 +20083,7 @@ Enum class for parameter type. | Name | Type | Description | Required | | ---- | ---- | ----------- | -------- | | plugin_installation_id | string | | Yes | +| preserve_credentials | boolean | | No | #### ParserUpdateCredential diff --git a/api/tests/unit_tests/controllers/console/workspace/test_plugin.py b/api/tests/unit_tests/controllers/console/workspace/test_plugin.py index 0cb336cfea8..e92c4651983 100644 --- a/api/tests/unit_tests/controllers/console/workspace/test_plugin.py +++ b/api/tests/unit_tests/controllers/console/workspace/test_plugin.py @@ -597,19 +597,28 @@ class TestPluginInstallFromPkgApi: class TestPluginUninstallApi: - def test_uninstall(self, app: Flask): + @pytest.mark.parametrize("preserve_credentials", [False, True]) + def test_uninstall(self, app: Flask, preserve_credentials: bool): api = PluginUninstallApi() method = unwrap(api.post) - payload = {"plugin_installation_id": "x"} + payload = { + "plugin_installation_id": "x", + "preserve_credentials": preserve_credentials, + } with ( app.test_request_context("/", json=payload), - patch("controllers.console.workspace.plugin.PluginService.uninstall", return_value=True), + patch("controllers.console.workspace.plugin.PluginService.uninstall", return_value=True) as uninstall_mock, ): result = method(api, "t1") assert result["success"] is True + uninstall_mock.assert_called_once_with( + "t1", + "x", + preserve_credentials=preserve_credentials, + ) class TestPluginChangePermissionApi: diff --git a/api/tests/unit_tests/services/plugin/test_plugin_service.py b/api/tests/unit_tests/services/plugin/test_plugin_service.py index 19bb2c0a359..4c063d30f4b 100644 --- a/api/tests/unit_tests/services/plugin/test_plugin_service.py +++ b/api/tests/unit_tests/services/plugin/test_plugin_service.py @@ -12,6 +12,7 @@ from sqlalchemy.orm import Session from core.helper.model_provider_cache import ProviderCredentialsCacheType from core.plugin.entities.plugin import PluginCategory, PluginInstallationSource from core.plugin.entities.plugin_daemon import PluginInstallTask, PluginInstallTaskStatus, PluginModelProviderEntity +from core.provider_manager import ProviderConfigurationCacheSource from graphon.model_runtime.entities.common_entities import I18nObject from graphon.model_runtime.entities.provider_entities import ConfigurateMethod, ProviderEntity from models.provider import Provider, ProviderCredential, ProviderType, TenantPreferredModelProvider @@ -1224,6 +1225,7 @@ class TestPluginModelProviderCacheInvalidation: patch(f"{MODULE}.PluginInstaller") as installer_cls, patch(f"{MODULE}.ProviderCredentialsCache") as credentials_cache, patch(f"{MODULE}.PluginService.invalidate_plugin_model_providers_cache") as invalidate_cache, + patch("core.provider_manager.ProviderManager.invalidate_configurations_cache") as invalidate_configurations, ): mock_config.ENTERPRISE_ENABLED = False installer = installer_cls.return_value @@ -1237,6 +1239,13 @@ class TestPluginModelProviderCacheInvalidation: assert result is True installer.uninstall.assert_called_once_with(TENANT_ID, "installation-1") invalidate_cache.assert_called_once_with(TENANT_ID) + invalidate_configurations.assert_called_once_with( + TENANT_ID, + sources=( + ProviderConfigurationCacheSource.PREFERRED_MODEL_PROVIDERS, + ProviderConfigurationCacheSource.PROVIDER_CREDENTIALS, + ), + ) credentials_cache.assert_called_once_with( tenant_id=TENANT_ID, identity_id=provider_id, diff --git a/api/tests/unit_tests/services/plugin/test_plugin_service_installation.py b/api/tests/unit_tests/services/plugin/test_plugin_service_installation.py index 9f6cf4f36f9..b75d469e5d5 100644 --- a/api/tests/unit_tests/services/plugin/test_plugin_service_installation.py +++ b/api/tests/unit_tests/services/plugin/test_plugin_service_installation.py @@ -452,3 +452,78 @@ class TestUninstall: ) ).all() assert len(remaining_prefs) == 0 + + @patch("core.plugin.plugin_service.PluginInstaller") + def test_preserves_credentials_when_replacing_plugin( + self, mock_installer_cls: MagicMock, plugin_db: Session + ) -> None: + tenant_id = str(uuid4()) + plugin_id = "org/myplugin" + provider_name = f"{plugin_id}/model-provider" + + credential = ProviderCredential( + tenant_id=tenant_id, + provider_name=provider_name, + credential_name="default", + encrypted_config="{}", + ) + plugin_db.add(credential) + plugin_db.flush() + + provider = Provider( + tenant_id=tenant_id, + provider_name=provider_name, + credential_id=credential.id, + ) + plugin_db.add(provider) + + preferred_provider = TenantPreferredModelProvider( + tenant_id=tenant_id, + provider_name=provider_name, + preferred_provider_type=ProviderType.CUSTOM, + ) + plugin_db.add(preferred_provider) + plugin_db.commit() + + plugin = MagicMock(installation_id="install-1", plugin_id=plugin_id) + installer = mock_installer_cls.return_value + installer.list_plugins.return_value = [plugin] + installer.uninstall.return_value = True + + with patch("core.plugin.plugin_service.dify_config") as mock_config: + mock_config.ENTERPRISE_ENABLED = False + result = PluginService.uninstall(tenant_id, "install-1", preserve_credentials=True) + + assert result is True + plugin_db.expire_all() + assert plugin_db.get(ProviderCredential, credential.id) is not None + assert plugin_db.get(Provider, provider.id).credential_id == credential.id + assert plugin_db.get(TenantPreferredModelProvider, preferred_provider.id) is not None + + @patch("core.plugin.plugin_service.PluginInstaller") + def test_preserves_credentials_when_daemon_uninstall_fails( + self, mock_installer_cls: MagicMock, plugin_db: Session + ) -> None: + tenant_id = str(uuid4()) + plugin_id = "org/myplugin" + credential = ProviderCredential( + tenant_id=tenant_id, + provider_name=f"{plugin_id}/model-provider", + credential_name="default", + encrypted_config="{}", + ) + plugin_db.add(credential) + plugin_db.commit() + + plugin = MagicMock(installation_id="install-1", plugin_id=plugin_id) + installer = mock_installer_cls.return_value + installer.list_plugins.return_value = [plugin] + installer.uninstall.return_value = False + + with patch("core.plugin.plugin_service.dify_config") as mock_config: + mock_config.ENTERPRISE_ENABLED = False + result = PluginService.uninstall(tenant_id, "install-1") + + assert result is False + plugin_db.expire_all() + assert plugin_db.get(ProviderCredential, credential.id) is not None diff --git a/packages/contracts/generated/api/console/workspaces/types.gen.ts b/packages/contracts/generated/api/console/workspaces/types.gen.ts index 350b46dd1a2..59b973b1ecd 100644 --- a/packages/contracts/generated/api/console/workspaces/types.gen.ts +++ b/packages/contracts/generated/api/console/workspaces/types.gen.ts @@ -475,6 +475,7 @@ export type PluginTaskResponse = { export type ParserUninstall = { plugin_installation_id: string + preserve_credentials?: boolean } export type ParserGithubUpgrade = { diff --git a/packages/contracts/generated/api/console/workspaces/zod.gen.ts b/packages/contracts/generated/api/console/workspaces/zod.gen.ts index 77bfc7dfe19..6f17f5c6d17 100644 --- a/packages/contracts/generated/api/console/workspaces/zod.gen.ts +++ b/packages/contracts/generated/api/console/workspaces/zod.gen.ts @@ -314,6 +314,7 @@ export const zPluginReadmeResponse = z.object({ */ export const zParserUninstall = z.object({ plugin_installation_id: z.string(), + preserve_credentials: z.boolean().optional().default(false), }) /** diff --git a/web/app/components/plugins/install-plugin/install-from-local-package/steps/__tests__/install.spec.tsx b/web/app/components/plugins/install-plugin/install-from-local-package/steps/__tests__/install.spec.tsx index 37739ea19e2..5e2efe6966b 100644 --- a/web/app/components/plugins/install-plugin/install-from-local-package/steps/__tests__/install.spec.tsx +++ b/web/app/components/plugins/install-plugin/install-from-local-package/steps/__tests__/install.spec.tsx @@ -372,7 +372,7 @@ describe('Install', () => { }) }) - it('should uninstall existing plugin before installing new version', async () => { + it('should preserve credentials when replacing an installed plugin', async () => { mockUseCheckInstalled.mockReturnValue({ installedInfo: { 'test-author/Test Plugin': { @@ -394,7 +394,9 @@ describe('Install', () => { fireEvent.click(screen.getByRole('button', { name: 'plugin.installModal.install' })) await waitFor(() => { - expect(mockUninstallPlugin).toHaveBeenCalledWith('installed-id-to-uninstall') + expect(mockUninstallPlugin).toHaveBeenCalledWith('installed-id-to-uninstall', { + preserveCredentials: true, + }) }) await waitFor(() => { diff --git a/web/app/components/plugins/install-plugin/install-from-local-package/steps/install.tsx b/web/app/components/plugins/install-plugin/install-from-local-package/steps/install.tsx index ae6c92956a3..77c64fd091d 100644 --- a/web/app/components/plugins/install-plugin/install-from-local-package/steps/install.tsx +++ b/web/app/components/plugins/install-plugin/install-from-local-package/steps/install.tsx @@ -69,7 +69,9 @@ const Installed: FC = ({ onStartToInstall?.() try { - if (hasInstalled) await uninstallPlugin(installedInfoPayload.installedId) + if (hasInstalled) { + await uninstallPlugin(installedInfoPayload.installedId, { preserveCredentials: true }) + } const response = await installPackageFromLocal(uniqueIdentifier) const { all_installed, task_id } = response diff --git a/web/app/components/plugins/types.ts b/web/app/components/plugins/types.ts index dea0689068d..2d301c56b28 100644 --- a/web/app/components/plugins/types.ts +++ b/web/app/components/plugins/types.ts @@ -452,10 +452,6 @@ export type InstalledPluginCategoryListResponse = { has_more: boolean } -export type UninstallPluginResponse = { - success: boolean -} - export type GitHubItemAndMarketPlaceDependency = { type: 'github' | 'marketplace' | 'package' value: { diff --git a/web/service/__tests__/use-plugins.spec.tsx b/web/service/__tests__/use-plugins.spec.tsx index 6e5ae5b0c07..ccae20709c3 100644 --- a/web/service/__tests__/use-plugins.spec.tsx +++ b/web/service/__tests__/use-plugins.spec.tsx @@ -19,15 +19,17 @@ import { renderHook } from '@/test/console/render' import { normalizeInstalledPluginDetail, useInstalledPluginList, + useInstallOrUpdate, useMutationPluginAutoUpgradeSettings, useMutationPluginPermissionSettings, usePluginAutoUpgradeSettings, usePluginTaskList, } from '../use-plugins' -const { mockGet, mockPost } = vi.hoisted(() => ({ +const { mockGet, mockPost, mockUninstallPlugin } = vi.hoisted(() => ({ mockGet: vi.fn(), mockPost: vi.fn(), + mockUninstallPlugin: vi.fn(), })) vi.mock('@/service/base', () => ({ @@ -37,6 +39,11 @@ vi.mock('@/service/base', () => ({ postMarketplace: vi.fn(), })) +vi.mock('@/service/plugins', () => ({ + fetchPluginInfoFromMarketPlace: vi.fn(), + uninstallPlugin: mockUninstallPlugin, +})) + vi.mock('@/app/components/plugins/install-plugin/hooks/use-refresh-plugin-list', () => ({ default: () => ({ refreshPluginList: vi.fn(), @@ -303,6 +310,54 @@ describe('use-plugins mutations', () => { resolvePost({}) }) + it('preserves credentials when replacing an installed bundle package', async () => { + const queryClient = createQueryClient() + mockUninstallPlugin.mockResolvedValue({ success: true }) + mockPost.mockResolvedValue({ all_installed: true, task_id: '' }) + const payload = [ + { + type: 'package' as const, + value: { + unique_identifier: 'langgenius/openai:0.0.2@new', + manifest: {}, + }, + }, + ] + const plugin = [ + { + org: 'langgenius', + name: 'openai', + }, + ] + const installedInfo = { + 'langgenius/openai': { + installedId: 'installation-id', + installedVersion: '0.0.1', + uniqueIdentifier: 'langgenius/openai:0.0.1@old', + }, + } + const { result } = renderHook(() => useInstallOrUpdate({}), { + wrapper: createWrapper(queryClient), + }) + + await act(async () => { + await result.current.mutateAsync({ + payload: payload as Parameters[0]['payload'], + plugin: plugin as Parameters[0]['plugin'], + installedInfo, + }) + }) + + expect(mockUninstallPlugin).toHaveBeenCalledWith('installation-id', { + preserveCredentials: true, + }) + expect(mockPost).toHaveBeenCalledWith('/workspaces/current/plugin/install/pkg', { + body: { + plugin_unique_identifiers: ['langgenius/openai:0.0.2@new'], + }, + }) + }) + it('optimistically updates plugin permission cache before the request finishes', async () => { const queryClient = createQueryClient() const queryKey = ['plugins', 'referenceSettings', 'permission'] diff --git a/web/service/plugins.spec.ts b/web/service/plugins.spec.ts new file mode 100644 index 00000000000..15aeb112b02 --- /dev/null +++ b/web/service/plugins.spec.ts @@ -0,0 +1,56 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { uninstallPlugin } from './plugins' + +const { mockUninstallPost } = vi.hoisted(() => ({ + mockUninstallPost: vi.fn(), +})) + +vi.mock('./base', () => ({ + get: vi.fn(), + getMarketplace: vi.fn(), + post: vi.fn(), + upload: vi.fn(), +})) + +vi.mock('./client', () => ({ + consoleClient: { + workspaces: { + current: { + plugin: { + uninstall: { + post: mockUninstallPost, + }, + }, + }, + }, + }, +})) + +describe('uninstallPlugin', () => { + beforeEach(() => { + vi.clearAllMocks() + mockUninstallPost.mockResolvedValue({ success: true }) + }) + + it('should delete credentials for an ordinary uninstall by default', async () => { + await uninstallPlugin('installation-id') + + expect(mockUninstallPost).toHaveBeenCalledWith({ + body: { + plugin_installation_id: 'installation-id', + preserve_credentials: false, + }, + }) + }) + + it('should preserve credentials when requested for a replacement', async () => { + await uninstallPlugin('installation-id', { preserveCredentials: true }) + + expect(mockUninstallPost).toHaveBeenCalledWith({ + body: { + plugin_installation_id: 'installation-id', + preserve_credentials: true, + }, + }) + }) +}) diff --git a/web/service/plugins.ts b/web/service/plugins.ts index ceb3c1bd291..452fba65c51 100644 --- a/web/service/plugins.ts +++ b/web/service/plugins.ts @@ -4,11 +4,11 @@ import type { InstallPackageResponse, PluginManifestInMarket, TaskStatusResponse, - UninstallPluginResponse, updatePackageResponse, uploadGitHubResponse, } from '@/app/components/plugins/types' import { get, getMarketplace, post, upload } from './base' +import { consoleClient } from './client' export const uploadFile = async (file: File, isBundle: boolean) => { const formData = new FormData() @@ -87,8 +87,14 @@ export const checkTaskStatus = async (taskId: string) => { return get(`/workspaces/current/plugin/tasks/${taskId}`) } -export const uninstallPlugin = async (pluginId: string) => { - return post('/workspaces/current/plugin/uninstall', { - body: { plugin_installation_id: pluginId }, +export const uninstallPlugin = async ( + pluginId: string, + options: { preserveCredentials?: boolean } = {}, +) => { + return consoleClient.workspaces.current.plugin.uninstall.post({ + body: { + plugin_installation_id: pluginId, + preserve_credentials: options.preserveCredentials ?? false, + }, }) } diff --git a/web/service/use-plugins.ts b/web/service/use-plugins.ts index baf40bb0a36..25fe6c6d4d9 100644 --- a/web/service/use-plugins.ts +++ b/web/service/use-plugins.ts @@ -919,7 +919,7 @@ export const useInstallOrUpdate = ({ } if (isInstalled) { if (item.type === 'package') { - await uninstallPlugin(installedPayload.installedId) + await uninstallPlugin(installedPayload.installedId, { preserveCredentials: true }) const response = await post( '/workspaces/current/plugin/install/pkg', {