feat: Add option to delete or keep API keys when uninstalling plugin #28201
Conversation
dosubot
bot
added
size:XXL
Summary of ChangesHello @UMDKyle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the plugin uninstallation experience by providing users with explicit control over the lifecycle of their API keys and credentials. Previously, these sensitive details would remain in the database even after a plugin was removed, leading to potential security concerns and user confusion upon reinstallation. The new implementation introduces a clear, user-friendly mechanism to either securely delete these credentials or intentionally retain them, directly resolving a critical user feedback and improving data hygiene. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for managing plugin credentials during uninstallation. The backend and frontend changes are comprehensive and well-structured. However, I've identified several critical issues in the backend tests that need immediate attention, as they are not correctly testing the implementation and could lead to a false sense of security. There are also significant performance inefficiencies in the database operations for deleting credentials, which should be refactored to use bulk operations for better performance. On the frontend, there's an opportunity to improve maintainability by reducing code duplication with a custom hook, and there are some UI and internationalization inconsistencies that should be addressed.
api/tests/unit_tests/services/plugin/test_plugin_credential_service.py
Outdated
Show resolved
Hide resolved
api/tests/unit_tests/services/plugin/test_plugin_credential_service.py
Outdated
Show resolved
Hide resolved
api/tests/unit_tests/services/plugin/test_plugin_service_uninstall.py
Outdated
Show resolved
Hide resolved
api/tests/unit_tests/services/plugin/test_plugin_service_uninstall.py
Outdated
Show resolved
Hide resolved
| def delete_plugin_credentials(tenant_id: str, credential_ids: Sequence[str]) -> int: | ||
| """ | ||
| Delete plugin credentials by IDs. | ||
| Args: | ||
| tenant_id: Tenant ID | ||
| credential_ids: List of credential IDs to delete | ||
| Returns: | ||
| Number of credentials deleted | ||
| """ | ||
| logger.info(f"Deleting credentials: {credential_ids} for tenant: {tenant_id}") | ||
| deleted_count = 0 | ||
|
|
||
| for credential_id in credential_ids: | ||
| # Try deleting from provider_credentials | ||
| provider_cred = db.session.scalars( | ||
| select(ProviderCredential).where( | ||
| ProviderCredential.id == credential_id, ProviderCredential.tenant_id == tenant_id | ||
| ) | ||
| ).first() | ||
|
|
||
| if provider_cred: | ||
| db.session.delete(provider_cred) | ||
| deleted_count += 1 | ||
| continue | ||
|
|
||
| # Try deleting from provider_model_credentials | ||
| model_cred = db.session.scalars( | ||
| select(ProviderModelCredential).where( | ||
| ProviderModelCredential.id == credential_id, ProviderModelCredential.tenant_id == tenant_id | ||
| ) | ||
| ).first() | ||
|
|
||
| if model_cred: | ||
| db.session.delete(model_cred) | ||
| deleted_count += 1 | ||
|
|
||
| db.session.commit() | ||
| logger.info(f"Deleted {deleted_count} plugin credentials for tenant {tenant_id}") | ||
| return deleted_count |
There was a problem hiding this comment.
The current implementation of delete_plugin_credentials is inefficient. It iterates through each credential_id and performs up to two SELECT queries inside the loop to find the credential before deleting it. This leads to an N+1 query problem, which can cause performance issues when deleting many credentials at once.
def delete_plugin_credentials(tenant_id: str, credential_ids: Sequence[str]) -> int:
"""
Delete plugin credentials by IDs.
Args:
tenant_id: Tenant ID
credential_ids: List of credential IDs to delete
Returns:
Number of credentials deleted
"""
if not credential_ids:
return 0
logger.info(f"Deleting credentials: {credential_ids} for tenant: {tenant_id}")
# Bulk delete from ProviderCredential
provider_creds_deleted = db.session.query(ProviderCredential).filter(
ProviderCredential.tenant_id == tenant_id,
ProviderCredential.id.in_(credential_ids)
).delete(synchronize_session=False)
# Bulk delete from ProviderModelCredential
model_creds_deleted = db.session.query(ProviderModelCredential).filter(
ProviderModelCredential.tenant_id == tenant_id,
ProviderModelCredential.id.in_(credential_ids)
).delete(synchronize_session=False)
deleted_count = provider_creds_deleted + model_creds_deleted
db.session.commit()
logger.info(f"Deleted {deleted_count} plugin credentials for tenant {tenant_id}")
return deleted_count| def delete_all_plugin_credentials(tenant_id: str, plugin_id: str) -> int: | ||
| """ | ||
| Delete all credentials associated with a plugin. | ||
| Args: | ||
| tenant_id: Tenant ID | ||
| plugin_id: Plugin ID in format "organization/plugin_name" | ||
| Returns: | ||
| Number of credentials deleted | ||
| """ | ||
| credentials = PluginCredentialService.get_plugin_credentials(tenant_id, plugin_id) | ||
| credential_ids = [cred.credential_id for cred in credentials] | ||
| return PluginCredentialService.delete_plugin_credentials(tenant_id, credential_ids) |
There was a problem hiding this comment.
This method is inefficient. It first fetches all credential objects and their IDs, and then calls delete_plugin_credentials, which in its current form loops through them one by one. This results in multiple database roundtrips.
def delete_all_plugin_credentials(tenant_id: str, plugin_id: str) -> int:
"""
Delete all credentials associated with a plugin.
Args:
tenant_id: Tenant ID
plugin_id: Plugin ID in format "organization/plugin_name"
Returns:
Number of credentials deleted
"""
logger.info(f"Deleting all credentials for plugin_id: {plugin_id}, tenant_id: {tenant_id}")
# Bulk delete from ProviderCredential
provider_creds_deleted = db.session.query(ProviderCredential).filter(
ProviderCredential.tenant_id == tenant_id,
ProviderCredential.provider_name.like(f"{plugin_id}/%")
).delete(synchronize_session=False)
# Bulk delete from ProviderModelCredential
model_creds_deleted = db.session.query(ProviderModelCredential).filter(
ProviderModelCredential.tenant_id == tenant_id,
ProviderModelCredential.provider_name.like(f"{plugin_id}/%")
).delete(synchronize_session=False)
deleted_count = provider_creds_deleted + model_creds_deleted
db.session.commit()
logger.info(f"Deleted {deleted_count} plugin credentials for tenant {tenant_id}")
return deleted_count| manager = PluginInstaller() | ||
| plugins = manager.list_plugins(tenant_id) | ||
| plugin = next((p for p in plugins if p.installation_id == plugin_installation_id), None) |
There was a problem hiding this comment.
In both check_plugin_credentials and uninstall, you are fetching the list of all plugins for a tenant and then iterating in Python to find the one with the matching plugin_installation_id. This can be inefficient if a tenant has a large number of installed plugins. A similar pattern is repeated in the uninstall method.
| manager = PluginInstaller() | |
| plugins = manager.list_plugins(tenant_id) | |
| plugin = next((p for p in plugins if p.installation_id == plugin_installation_id), None) | |
| # Get plugin installation to extract plugin_id | |
| manager = PluginInstaller() | |
| plugin = manager.get_plugin_by_installation_id(tenant_id, plugin_installation_id) |
| {credentials.length > 0 && ( | ||
| <div className='space-y-2'> | ||
| <div className='system-sm-semibold text-text-secondary'> | ||
| {t(`${i18nPrefix}.credentialsWarning`)} | ||
| </div> | ||
| <div className='max-h-32 overflow-y-auto space-y-1 rounded-lg border border-divider-subtle bg-background-section-burn p-2'> | ||
| {credentials.map(cred => ( | ||
| <div key={cred.credential_id} className='system-xs-regular text-text-tertiary'> | ||
| • {cred.credential_name} ({cred.credential_type}) | ||
| </div> | ||
| ))} | ||
| </div> | ||
| <label className='flex items-center space-x-2 cursor-pointer'> | ||
| <input | ||
| type='checkbox' | ||
| checked={deleteCredentials} | ||
| onChange={e => setDeleteCredentials(e.target.checked)} | ||
| className='rounded border-divider-regular' | ||
| /> | ||
| <span className='system-sm-regular text-text-secondary'> | ||
| {t(`${i18nPrefix}.deleteCredentials`)} | ||
| </span> | ||
| </label> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
This component uses a single checkbox for the credential deletion option, while plugin-item/action.tsx uses a pair of radio-button-style options. The PR description also mentions "radio button options". Using different UI controls for the same action can be confusing for users.
For UI consistency, consider replacing the checkbox with the same radio button group used in plugin-item/action.tsx. This will provide a consistent user experience across the application.
| <div className='space-y-2 pt-2'> | ||
| <div className='system-sm-semibold text-text-secondary'> | ||
| What should happen to these API keys? | ||
| </div> | ||
| <div className='flex flex-col gap-2'> | ||
| <label | ||
| className='flex items-center gap-2 p-2 rounded cursor-pointer hover:bg-background-default-hover transition-colors' | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| setDeleteCredentials(true) | ||
| }} | ||
| > | ||
| <div className={`w-4 h-4 rounded-full border-2 flex items-center justify-center ${deleteCredentials ? 'border-blue-600' : 'border-gray-300'}`}> | ||
| {deleteCredentials && <div className='w-2 h-2 rounded-full bg-blue-600' />} | ||
| </div> | ||
| <span className='system-sm-regular'>Delete these API keys</span> | ||
| </label> | ||
| <label | ||
| className='flex items-center gap-2 p-2 rounded cursor-pointer hover:bg-background-default-hover transition-colors' | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| setDeleteCredentials(false) | ||
| }} | ||
| > | ||
| <div className={`w-4 h-4 rounded-full border-2 flex items-center justify-center ${!deleteCredentials ? 'border-blue-600' : 'border-gray-300'}`}> | ||
| {!deleteCredentials && <div className='w-2 h-2 rounded-full bg-blue-600' />} | ||
| </div> | ||
| <span className='system-sm-regular'>Keep these API keys (reuse if reinstalling plugin)</span> | ||
| </label> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The text for the credential deletion options ("Delete these API keys", "Keep these API keys...") is hardcoded in this component. This will prevent them from being translated into other languages.
alterxyz
requested review from
Yeuoly and
crazywoola
and removed request for
Yeuoly and
crazywoola
- Add PluginCredentialService for credential management - Add check_plugin_credentials() and update uninstall() in PluginService - Add API endpoints for credential checking and deletion options - Update frontend with credential deletion UI and radio buttons - Add i18n strings for credential management Fixes langgenius#27531
UMDKyle
force-pushed
the
feature/plugin-credential-deletion-option
branch
from
c734463 to
78a5274
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature that allows users to decide whether to delete or retain API keys upon uninstalling a plugin. The implementation is solid, with both backend and frontend changes to support this new functionality. I've identified a couple of areas for improvement: one related to database performance and another concerning internationalization of new UI strings. Overall, this is a well-executed feature addition.
| deleted_count = 0 | ||
|
|
||
| for credential_id in credential_ids: | ||
| # Try deleting from provider_credentials | ||
| provider_cred = db.session.scalars( | ||
| select(ProviderCredential).where( | ||
| ProviderCredential.id == credential_id, ProviderCredential.tenant_id == tenant_id | ||
| ) | ||
| ).first() | ||
|
|
||
| if provider_cred: | ||
| db.session.delete(provider_cred) | ||
| deleted_count += 1 | ||
| continue | ||
|
|
||
| # Try deleting from provider_model_credentials | ||
| model_cred = db.session.scalars( | ||
| select(ProviderModelCredential).where( | ||
| ProviderModelCredential.id == credential_id, ProviderModelCredential.tenant_id == tenant_id | ||
| ) | ||
| ).first() | ||
|
|
||
| if model_cred: | ||
| db.session.delete(model_cred) | ||
| deleted_count += 1 | ||
|
|
There was a problem hiding this comment.
The current implementation iterates through each credential_id and executes individual database queries within a loop. This can lead to an N+1 query problem, which is inefficient if a plugin has many credentials. A more performant approach would be to use a single bulk DELETE statement for each credential table.
deleted_count = 0
if not credential_ids:
return deleted_count
from sqlalchemy import delete
# Delete from provider_credentials
provider_stmt = delete(ProviderCredential).where(
ProviderCredential.tenant_id == tenant_id,
ProviderCredential.id.in_(credential_ids)
)
provider_result = db.session.execute(provider_stmt)
# Delete from provider_model_credentials
model_stmt = delete(ProviderModelCredential).where(
ProviderModelCredential.tenant_id == tenant_id,
ProviderModelCredential.id.in_(credential_ids)
)
model_result = db.session.execute(model_stmt)
deleted_count = provider_result.rowcount + model_result.rowcount| <div className='space-y-2 pt-2'> | ||
| <div className='system-sm-semibold text-text-secondary'> | ||
| What should happen to these API keys? | ||
| </div> | ||
| <div className='flex flex-col gap-2'> | ||
| <label | ||
| className='flex items-center gap-2 p-2 rounded cursor-pointer hover:bg-background-default-hover transition-colors' | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| setDeleteCredentials(true) | ||
| }} | ||
| > | ||
| <div className={`w-4 h-4 rounded-full border-2 flex items-center justify-center ${deleteCredentials ? 'border-blue-600' : 'border-gray-300'}`}> | ||
| {deleteCredentials && <div className='w-2 h-2 rounded-full bg-blue-600' />} | ||
| </div> | ||
| <span className='system-sm-regular'>Delete these API keys</span> | ||
| </label> | ||
| <label | ||
| className='flex items-center gap-2 p-2 rounded cursor-pointer hover:bg-background-default-hover transition-colors' | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| setDeleteCredentials(false) | ||
| }} | ||
| > | ||
| <div className={`w-4 h-4 rounded-full border-2 flex items-center justify-center ${!deleteCredentials ? 'border-blue-600' : 'border-gray-300'}`}> | ||
| {!deleteCredentials && <div className='w-2 h-2 rounded-full bg-blue-600' />} | ||
| </div> | ||
| <span className='system-sm-regular'>Keep these API keys (reuse if reinstalling plugin)</span> | ||
| </label> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
There are several hardcoded strings in the UI for managing credentials upon plugin deletion. These should be internationalized to support multiple languages and improve maintainability.
Please add the following keys to web/i18n/en-US/plugin.ts under action:
whatToDoWithKeys: 'What should happen to these API keys?',
deleteApiKeys: 'Delete these API keys',
keepApiKeys: 'Keep these API keys (reuse if reinstalling plugin)',Then, use the t function to render these strings in the component.
<div className='space-y-2 pt-2'>
<div className='system-sm-semibold text-text-secondary'>
{t(`${i18nPrefix}.whatToDoWithKeys`)}
</div>
<div className='flex flex-col gap-2'>
<label
className='flex items-center gap-2 p-2 rounded cursor-pointer hover:bg-background-default-hover transition-colors'
onClick={(e) => {
e.stopPropagation()
setDeleteCredentials(true)
}}
>
<div className={`w-4 h-4 rounded-full border-2 flex items-center justify-center ${deleteCredentials ? 'border-blue-600' : 'border-gray-300'}`}>
{deleteCredentials && <div className='w-2 h-2 rounded-full bg-blue-600' />}
</div>
<span className='system-sm-regular'>{t(`${i18nPrefix}.deleteApiKeys`)}</span>
</label>
<label
className='flex items-center gap-2 p-2 rounded cursor-pointer hover:bg-background-default-hover transition-colors'
onClick={(e) => {
e.stopPropagation()
setDeleteCredentials(false)
}}
>
<div className={`w-4 h-4 rounded-full border-2 flex items-center justify-center ${!deleteCredentials ? 'border-blue-600' : 'border-gray-300'}`}>
{!deleteCredentials && <div className='w-2 h-2 rounded-full bg-blue-600' />}
</div>
<span className='system-sm-regular'>{t(`${i18nPrefix}.keepApiKeys`)}</span>
</label>
</div>
</div>
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality allowing users to choose whether to delete or keep API keys/credentials when uninstalling plugins, directly addressing issue #27531 where credentials persisted after plugin deletion without user awareness.
Key Changes:
- Added backend service (
PluginCredentialService) to query and delete plugin credentials - Enhanced uninstall API endpoint with credential management parameters
- Implemented frontend UI showing credentials and providing delete/keep options before uninstallation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
api/services/plugin/plugin_credential_service.py |
New service to retrieve and delete plugin credentials from database |
api/services/plugin/plugin_service.py |
Enhanced uninstall method with credential deletion logic |
api/controllers/console/workspace/plugin.py |
Added credential check endpoint and updated uninstall endpoint with new parameters |
web/service/plugins.ts |
Added credential check function and updated uninstall function signature |
web/app/components/plugins/plugin-item/action.tsx |
Implemented credential check and deletion UI with radio buttons |
web/app/components/plugins/plugin-detail-panel/detail-header.tsx |
Implemented credential check and deletion UI with checkbox |
web/i18n/en-US/plugin.ts |
Added English translations for credential management UI |
Comments suppressed due to low confidence (3)
web/app/components/plugins/plugin-item/action.tsx:12
- Unused import CredentialCheckbox.
import CredentialCheckbox from './credential-checkbox'
web/i18n/en-US/plugin.ts:196
- This property is overwritten by another property in the same object literal.
action: {
checkForUpdates: 'Check for updates',
pluginInfo: 'Plugin info',
delete: 'Remove plugin',
deleteContentLeft: 'Would you like to remove ',
deleteContentRight: ' plugin?',
usedInApps: 'This plugin is being used in {{num}} apps.',
},
api/services/plugin/plugin_service.py:551
- This import of module logging is redundant, as it was previously imported on line 1.
import logging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import Tooltip from '../../base/tooltip' | ||
| import Confirm from '../../base/confirm' | ||
| import { uninstallPlugin } from '@/service/plugins' | ||
| import CredentialCheckbox from './credential-checkbox' |
There was a problem hiding this comment.
There's an unused import CredentialCheckbox on line 12. This component is imported but never used in the file. The credential UI is implemented inline with custom radio buttons instead.
| import CredentialCheckbox from './credential-checkbox' |
| <label className='flex items-center space-x-2 cursor-pointer'> | ||
| <input | ||
| type='checkbox' | ||
| checked={deleteCredentials} | ||
| onChange={e => setDeleteCredentials(e.target.checked)} | ||
| className='rounded border-divider-regular' | ||
| /> | ||
| <span className='system-sm-regular text-text-secondary'> | ||
| {t(`${i18nPrefix}.deleteCredentials`)} | ||
| </span> | ||
| </label> |
There was a problem hiding this comment.
[nitpick] The default credential deletion behavior differs between the two components:
- In
plugin-item/action.tsx(line 107): Defaults totrue(delete credentials) when credentials exist - In
plugin-detail-panel/detail-header.tsx(line 212): Also defaults totruewhen credentials exist, but uses a checkbox instead of radio buttons
The checkbox pattern in detail-header.tsx is confusing because the label says "Also delete these API keys (if unchecked, they will remain...)" but the default is checked=true. With a checkbox, users might interpret this as an opt-in rather than understanding the default behavior.
Suggestion: Use radio buttons consistently in both locations to make the choice clearer.
| <label className='flex items-center space-x-2 cursor-pointer'> | |
| <input | |
| type='checkbox' | |
| checked={deleteCredentials} | |
| onChange={e => setDeleteCredentials(e.target.checked)} | |
| className='rounded border-divider-regular' | |
| /> | |
| <span className='system-sm-regular text-text-secondary'> | |
| {t(`${i18nPrefix}.deleteCredentials`)} | |
| </span> | |
| </label> | |
| <div className='flex flex-col space-y-2'> | |
| <label className='flex items-center space-x-2 cursor-pointer'> | |
| <input | |
| type='radio' | |
| name='delete-credentials' | |
| value='delete' | |
| checked={deleteCredentials === true} | |
| onChange={() => setDeleteCredentials(true)} | |
| className='rounded border-divider-regular' | |
| /> | |
| <span className='system-sm-regular text-text-secondary'> | |
| {t(`${i18nPrefix}.deleteCredentials`)} | |
| </span> | |
| </label> | |
| <label className='flex items-center space-x-2 cursor-pointer'> | |
| <input | |
| type='radio' | |
| name='delete-credentials' | |
| value='keep' | |
| checked={deleteCredentials === false} | |
| onChange={() => setDeleteCredentials(false)} | |
| className='rounded border-divider-regular' | |
| /> | |
| <span className='system-sm-regular text-text-secondary'> | |
| {t(`${i18nPrefix}.keepCredentials`)} | |
| </span> | |
| </label> | |
| </div> |
| parser_uninstall = reqparse.RequestParser().add_argument( | ||
| "plugin_installation_id", type=str, required=True, location="json" | ||
| ).add_argument("delete_credentials", type=bool, required=False, default=False, location="json").add_argument( | ||
| "credential_ids", type=list, required=False, location="json" |
There was a problem hiding this comment.
The parameter type on line 560 uses type=list which is not specific enough for proper validation. The API expects a list of strings (credential IDs), but the current parser doesn't enforce this.
Suggestion: Consider using a more specific type like action='append' or validating the list contents are strings, to prevent potential type errors when non-string values are passed.
| parser_uninstall = reqparse.RequestParser().add_argument( | |
| "plugin_installation_id", type=str, required=True, location="json" | |
| ).add_argument("delete_credentials", type=bool, required=False, default=False, location="json").add_argument( | |
| "credential_ids", type=list, required=False, location="json" | |
| def list_of_strings(value): | |
| if not isinstance(value, list): | |
| raise ValueError("credential_ids must be a list of strings") | |
| if not all(isinstance(item, str) for item in value): | |
| raise ValueError("All credential_ids must be strings") | |
| return value | |
| parser_uninstall = reqparse.RequestParser().add_argument( | |
| "plugin_installation_id", type=str, required=True, location="json" | |
| ).add_argument("delete_credentials", type=bool, required=False, default=False, location="json").add_argument( | |
| "credential_ids", type=list_of_strings, required=False, location="json" |
| tenant_id, | ||
| args["plugin_installation_id"], | ||
| delete_credentials=args.get("delete_credentials", False), | ||
| credential_ids=args.get("credential_ids", []), |
There was a problem hiding this comment.
On line 581, when credential_ids is not provided, the code passes an empty list [] instead of None. However, the service method uninstall() signature uses Optional[Sequence[str]] for this parameter to distinguish between "no IDs provided" (None) and "empty list provided" ([]).
The logic in lines 569-576 of plugin_service.py shows that when credential_ids is provided (even if empty), it tries to delete specific credentials, but an empty list would do nothing. When credential_ids is None, it deletes all credentials.
Suggestion: Change line 581 to credential_ids=args.get("credential_ids") (without the default []) to properly pass None when not provided, allowing the service to delete all credentials as intended.
| credential_ids=args.get("credential_ids", []), | |
| credential_ids=args.get("credential_ids"), |
| result = PluginService.uninstall( | ||
| tenant_id, | ||
| args["plugin_installation_id"], | ||
| delete_credentials=args.get("delete_credentials", False), |
There was a problem hiding this comment.
[nitpick] The delete_credentials parameter on line 580 uses .get() with a default of False, which is already defined in the parser on line 559. This creates redundant default value handling. While not harmful, it's unnecessary duplication.
Suggestion: Since the parser already has default=False, you can use args["delete_credentials"] directly instead of args.get("delete_credentials", False).
| delete_credentials=args.get("delete_credentials", False), | |
| delete_credentials=args["delete_credentials"], |
| <label className='flex items-center space-x-2 cursor-pointer'> | ||
| <input | ||
| type='checkbox' | ||
| checked={deleteCredentials} | ||
| onChange={e => setDeleteCredentials(e.target.checked)} | ||
| className='rounded border-divider-regular' | ||
| /> | ||
| <span className='system-sm-regular text-text-secondary'> | ||
| {t(`${i18nPrefix}.deleteCredentials`)} | ||
| </span> | ||
| </label> | ||
| </div> |
There was a problem hiding this comment.
The UI implementation differs between the two deletion dialogs in the codebase:
- In
plugin-item/action.tsx(lines 158-183): Uses custom radio buttons - In
plugin-detail-panel/detail-header.tsx(lines 399-410): Uses a checkbox
This inconsistency creates a confusing user experience. Both locations should use the same UI pattern for deleting credentials.
Suggestion: Standardize on one approach (preferably radio buttons as they better represent the mutually exclusive choice).
| """Service for managing plugin credentials.""" | ||
|
|
||
| @staticmethod | ||
| def get_plugin_credentials(tenant_id: str, plugin_id: str) -> list[PluginCredentialInfo]: |
There was a problem hiding this comment.
The return type annotation on line 37 uses list[PluginCredentialInfo] which is Python 3.9+ syntax, but this requires from __future__ import annotations at the top of the file to work in Python 3.9. Without this import, the code will fail at runtime in Python 3.9.
Since the file already imports Optional from typing (line 4 in plugin_service.py shows this pattern), this file should consistently use List[PluginCredentialInfo] from the typing module or add from __future__ import annotations.
Suggestion: Change to from typing import List and use List[PluginCredentialInfo], or add from __future__ import annotations at the top of the file.
| import logging | ||
| from services.plugin.plugin_credential_service import PluginCredentialService | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
The variable logger is defined on line 554 using logging.getLogger(__name__), but there's already a module-level logger imported on line 1 and typically available as a global. The local logger definition is unnecessary and creates confusion about which logger instance is being used.
Additionally, there's a redundant import statement on line 551 (import logging) when logging is already imported at the module level (line 1).
Suggestion: Remove the redundant import on line 551 and the local logger definition on line 554, and use the module-level logger instead.
| import logging | |
| from services.plugin.plugin_credential_service import PluginCredentialService | |
| logger = logging.getLogger(__name__) | |
| from services.plugin.plugin_credential_service import PluginCredentialService |
| } | ||
| catch (error) { | ||
| console.error('checkPluginCredentials error', error) | ||
| setCredentials([]) |
There was a problem hiding this comment.
Similar to the issue in action.tsx, the error handling on lines 214-217 catches and logs errors from checkPluginCredentials(), but then proceeds without any user notification. If the credentials check fails, the user won't know that credential information couldn't be loaded.
Additionally, when an error occurs, deleteCredentials remains false (line 206 initialization), which means credentials won't be deleted even if they exist.
Suggestion: Add user-facing error notification (toast) when the credential check fails, or default deleteCredentials to true to ensure credentials are cleaned up even if the check fails.
| setCredentials([]) | |
| setCredentials([]) | |
| setDeleteCredentials(true) | |
| Toast.notify({ | |
| type: 'error', | |
| message: 'Failed to load plugin credentials. Credentials will be deleted if present.', | |
| }) |
|
|
||
| from extensions.ext_database import db | ||
| from models.provider import ProviderCredential, ProviderModelCredential | ||
| from models.provider_ids import GenericProviderID |
There was a problem hiding this comment.
Import of 'GenericProviderID' is not used.
| from models.provider_ids import GenericProviderID |
2 participants
Description
Summary
This PR adds a user-facing option to delete or retain API keys/credentials when uninstalling a plugin, directly solving the issue where credentials persist after plugin deletion.
Problem (Issue #27531)
When users deleted a plugin, the associated API keys and credentials remained in the database without any notification or option to manage them. As reported in #27531:
This caused:
Solution
Implemented a two-step deletion process with user choice:
Users can now:
Changes
Backend Changes
New Service:
PluginCredentialServiceget_plugin_credentials(): Retrieves all credentials for a plugin (bothProviderCredentialandProviderModelCredential)delete_plugin_credentials(): Deletes specific credentials by IDdelete_all_plugin_credentials(): Deletes all credentials for a pluginUpdated Service:
PluginServicecheck_plugin_credentials(): Check credentials before uninstalluninstall(): Addeddelete_credentialsandcredential_idsparametersAPI Endpoints:
GET /workspaces/current/plugin/uninstall/check-credentialsPOST /workspaces/current/plugin/uninstallwith credential optionsFrontend Changes
Components:
plugin-item/action.tsxwith credential checking and deletion UIplugin-detail-panel/detail-header.tsxwith same functionalityService Layer:
checkPluginCredentials()functionuninstallPlugin()with credential parametersUI:
Internationalization:
Tests
Comprehensive test coverage with 41 tests:
Backend Tests (26 tests):
PluginCredentialServicePluginService.uninstall()Frontend Tests (15 tests):
Test Documentation:
api/tests/unit_tests/services/plugin/README.md- Backend test guideweb/__tests__/README.plugin-delete-tests.md- Frontend test guideHow This Solves Issue #27531
Before: API keys persisted after deletion with no way to clear them
After: Users explicitly choose what happens to credentials (only if credential exist)
The issue reporter can now:
Database Queries
The implementation queries two tables with
plugin_id/%pattern:provider_credential(provider-level credentials)provider_model_credential(model-specific credentials)Both credential types are shown and can be deleted.
Testing
All 41 tests passing:
Type Safety
Fixed Python 3.9 compatibility:
list[str] | None→Optional[Sequence[str]]Backward Compatibility
✅ Fully backward compatible
Security
tenant_id(no cross-tenant access)Performance
tenant_idandprovider)Files Changed
New Files (5):
api/services/plugin/plugin_credential_service.pyapi/tests/unit_tests/services/plugin/test_plugin_credential_service.pyapi/tests/unit_tests/services/plugin/test_plugin_service_uninstall.pyapi/tests/unit_tests/controllers/console/workspace/test_plugin_uninstall_api.pyweb/__tests__/plugin-delete-credential-options.test.tsxModified Files (6):
api/services/plugin/plugin_service.pyapi/controllers/console/workspace/plugin.pyweb/app/components/plugins/plugin-item/action.tsxweb/app/components/plugins/plugin-detail-panel/detail-header.tsxweb/service/plugins.tsweb/i18n/en-US/plugin.tsDocumentation (2):
api/tests/unit_tests/services/plugin/README.mdweb/__tests__/README.plugin-delete-tests.mdTotal: 15 files, 2161+ insertions, 23 deletions
Checklist
Related Issue
Fixes #27531