From 7643dd457ef53bb60604f64c2c1247aa91e2fd2e Mon Sep 17 00:00:00 2001 From: wizardchen Date: Sun, 17 May 2026 15:09:30 +0800 Subject: [PATCH] refactor(credentials): introduce /credentials subresource pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the legacy "redacted placeholder + Clear* boolean" pattern with dedicated per-resource credential subresources across MCP services, Models, Web search providers, and Data sources. Why --- The previous design had three problems: 1. Main PUT body carried secret fields. The frontend echoed back a redacted "***" placeholder, and a fragile MergeUpdate / IsRedactedOrEmpty defense in the service layer tried to detect "user did not change this" vs "user wants to clear this". A regression in that defense (or a new frontend forgetting it) silently overwrites the stored secret with the placeholder. 2. The "remove this credential" UX was a red checkbox under a pre-filled password input. Three intents (preserve / replace / clear) collapsed onto one field, and credential changes were bundled with unrelated config edits in the same submit. Users wiped working keys by mistake. 3. Secret presence was inferred from "did the response come back with a '***' placeholder", which couples the contract to a magic string. Design ------ Each of the four resources now exposes: PUT /{resource}/{id}/credentials # write one or more fields DELETE /{resource}/{id}/credentials/{field} # clear a single field "Is this configured?" metadata travels on the main resource response as a typed map (dto.MCPServiceResponse.Credentials etc.) — no separate GET endpoint. The frontend reads the configured boolean from the main GET and never sees secret values at all. Main PUT endpoints now ignore any secret fields in the request body and log a deprecation warning if they appear, so legacy clients fail loudly rather than overwriting silently. Frontend -------- - New reusable component renders a three-state card (unconfigured / configured / editing) and drives the dedicated endpoints. Used by MCP, Model, Web search; DataSource has a bespoke card with the same behaviour because its credentials are a single atomic per-connector map. - Cancel from edit mode now restores state synchronously from the meta prop. The previous async refresh() was a no-op while state was still 'editing', leaving the input frozen open. - Remove is single-click + toast. The danger-themed button is the deterrent; a modal confirm adds friction without adding safety (the plaintext is irrecoverable client-side either way — recovery means re-typing). DTOs (internal/handler/dto/) are deliberately separate from the GORM models so "no secret in response" is a compile-time invariant: a future contributor cannot leak a secret without explicitly adding the field to the DTO, which is review-able in a single diff. Storage is unchanged — credentials still live in the existing jsonb / parameters columns. No schema migration. Cleanup ------- - types.MCPService / types.Model / types.WebSearchProviderEntity / types.DataSourceConfig: drop ClearAPIKey / ClearToken / ClearAppSecret / ClearCredentials boolean fields, MergeUpdate(), RedactSensitiveData(). - utils/types/secret.go: drop PreserveIfRedacted / IsRedactedOrEmpty. RedactedSecretPlaceholder constant is retained because VectorStore still uses the old pattern and is out of scope here. - Frontend hasExistingApiKey / clearApiKey / convertToLegacyFormat redaction handling removed; i18n keys renamed secret -> credential. --- client/mcp_service.go | 12 +- frontend/src/api/datasource/index.ts | 27 + frontend/src/api/mcp-service.ts | 47 +- frontend/src/api/model/index.ts | 37 +- frontend/src/api/web-search-provider.ts | 33 +- frontend/src/components/ModelEditorDialog.vue | 778 ++++++++---------- .../credentials/CredentialResource.vue | 293 +++++++ frontend/src/i18n/locales/en-US.ts | 28 +- frontend/src/i18n/locales/ko-KR.ts | 21 +- frontend/src/i18n/locales/ru-RU.ts | 21 +- frontend/src/i18n/locales/zh-CN.ts | 21 +- .../settings/DataSourceEditorDialog.vue | 434 ++++++---- frontend/src/views/settings/ModelSettings.vue | 77 +- .../src/views/settings/WebSearchSettings.vue | 134 +-- .../settings/components/McpServiceDialog.vue | 330 +++----- .../application/service/datasource_service.go | 122 ++- internal/application/service/mcp_service.go | 207 +++-- .../application/service/mcp_service_test.go | 470 +++++------ internal/application/service/model.go | 78 ++ .../service/session_knowledge_qa_test.go | 10 + .../service/web_search_provider.go | 49 ++ internal/container/container.go | 4 + internal/handler/datasource.go | 15 +- internal/handler/datasource_credentials.go | 115 +++ internal/handler/dto/datasource.go | 103 +++ internal/handler/dto/datasource_test.go | 62 ++ internal/handler/dto/mcp.go | 122 +++ internal/handler/dto/mcp_test.go | 79 ++ internal/handler/dto/model.go | 111 +++ internal/handler/dto/model_test.go | 74 ++ internal/handler/dto/web_search_provider.go | 68 ++ .../handler/dto/web_search_provider_test.go | 41 + internal/handler/mcp_credentials.go | 155 ++++ internal/handler/mcp_service.go | 69 +- internal/handler/model.go | 116 +-- internal/handler/model_credentials.go | 109 +++ internal/handler/web_search_provider.go | 39 +- .../web_search_provider_credentials.go | 104 +++ internal/router/router.go | 52 +- internal/types/datasource.go | 100 +-- internal/types/datasource_test.go | 206 ----- internal/types/interfaces/datasource.go | 12 + internal/types/interfaces/mcp_service.go | 17 + internal/types/interfaces/model.go | 9 + .../types/interfaces/web_search_provider.go | 10 + internal/types/mcp.go | 99 +-- internal/types/mcp_test.go | 52 -- internal/types/model.go | 55 +- internal/types/model_test.go | 139 ---- internal/types/secret.go | 67 +- internal/types/secret_test.go | 51 -- internal/types/web_search_provider.go | 47 +- internal/types/web_search_provider_test.go | 87 -- 53 files changed, 3271 insertions(+), 2347 deletions(-) create mode 100644 frontend/src/components/credentials/CredentialResource.vue create mode 100644 internal/handler/datasource_credentials.go create mode 100644 internal/handler/dto/datasource.go create mode 100644 internal/handler/dto/datasource_test.go create mode 100644 internal/handler/dto/mcp.go create mode 100644 internal/handler/dto/mcp_test.go create mode 100644 internal/handler/dto/model.go create mode 100644 internal/handler/dto/model_test.go create mode 100644 internal/handler/dto/web_search_provider.go create mode 100644 internal/handler/dto/web_search_provider_test.go create mode 100644 internal/handler/mcp_credentials.go create mode 100644 internal/handler/model_credentials.go create mode 100644 internal/handler/web_search_provider_credentials.go delete mode 100644 internal/types/datasource_test.go delete mode 100644 internal/types/mcp_test.go delete mode 100644 internal/types/model_test.go delete mode 100644 internal/types/secret_test.go delete mode 100644 internal/types/web_search_provider_test.go diff --git a/client/mcp_service.go b/client/mcp_service.go index c76a1c97..abc18600 100644 --- a/client/mcp_service.go +++ b/client/mcp_service.go @@ -37,17 +37,15 @@ type MCPService struct { // MCPAuthConfig represents authentication configuration for MCP service. // -// ClearAPIKey and ClearToken are write-only flags used in Update requests to -// explicitly remove a stored credential. The server never returns these -// fields in responses. +// Secret fields (APIKey, Token) are accepted on create but are never returned +// by the server. To mutate credentials on an existing service, use the +// dedicated /credentials subresource — see the MCP credentials API for the +// PUT / DELETE shape. Sending secret fields in a main PUT body is silently +// ignored server-side. type MCPAuthConfig struct { APIKey string `json:"api_key,omitempty"` Token string `json:"token,omitempty"` CustomHeaders map[string]string `json:"custom_headers,omitempty"` - - // Write-only clear flags. - ClearAPIKey bool `json:"clear_api_key,omitempty"` - ClearToken bool `json:"clear_token,omitempty"` } // MCPAdvancedConfig represents advanced configuration for MCP service diff --git a/frontend/src/api/datasource/index.ts b/frontend/src/api/datasource/index.ts index a5a47895..b1cb2424 100644 --- a/frontend/src/api/datasource/index.ts +++ b/frontend/src/api/datasource/index.ts @@ -17,6 +17,9 @@ export interface DataSource { last_sync_at: string | null last_sync_result: any error_message: string + // Single-field "credentials" map from the main response — DataSource + // credentials are a per-connector atomic set. + credentials?: { credentials: { configured: boolean } } created_at: string updated_at: string latest_sync_log?: SyncLog @@ -111,3 +114,27 @@ export function resumeDataSource(id: string) { export function getSyncLogs(id: string, limit = 20, offset = 0) { return get(`/api/v1/datasource/${id}/logs?limit=${limit}&offset=${offset}`) } + +// ---------------------------------------------------------------------------- +// Data source credential subresource. Unlike the other three resources, +// DataSource exposes a single logical field "credentials" because connector +// auth is a per-connector atomic map. See internal/handler/dto/datasource.go. +// ---------------------------------------------------------------------------- + +export interface DataSourceCredentialsResponse { + fields: { + credentials: { configured: boolean } + } +} + +export async function putDataSourceCredentials( + id: string, + credentials: Record, +): Promise { + const response: any = await put(`/api/v1/datasource/${id}/credentials`, { credentials }) + return (response.data ?? response) as DataSourceCredentialsResponse +} + +export async function deleteDataSourceCredentials(id: string): Promise { + await del(`/api/v1/datasource/${id}/credentials/credentials`) +} diff --git a/frontend/src/api/mcp-service.ts b/frontend/src/api/mcp-service.ts index 2b969dda..8af1a1a7 100644 --- a/frontend/src/api/mcp-service.ts +++ b/frontend/src/api/mcp-service.ts @@ -10,13 +10,13 @@ export interface MCPService { url?: string // Optional: required for SSE/HTTP Streamable headers?: Record auth_config?: { + // Secret fields (api_key, token) are NEVER returned by the server in + // this shape — they live behind the /credentials subresource. The + // optional-property typing remains so create-mode payloads can still + // carry them in the initial POST body. api_key?: string token?: string custom_headers?: Record - // Write-only flags — set to true in Update requests to explicitly remove - // the corresponding credential. The server never returns these fields. - clear_api_key?: boolean - clear_token?: boolean } advanced_config?: { timeout?: number @@ -29,6 +29,10 @@ export interface MCPService { } env_vars?: Record // Environment variables for stdio transport is_builtin?: boolean // Whether this is a builtin MCP service + // Per-field "configured?" map embedded on the main response (server-side + // dto.MCPServiceResponse.Credentials). Drives the CredentialResource card + // without a follow-up GET. Absent for builtin services. + credentials?: Record created_at?: string updated_at?: string } @@ -127,6 +131,41 @@ export async function setMCPToolApproval(serviceId: string, toolName: string, re }) } +// ---------------------------------------------------------------------------- +// Credential subresource (issue #988 follow-up). +// +// Secrets travel through a dedicated /credentials endpoint instead of the +// main MCP PUT body. "Is this configured?" metadata is embedded on the main +// MCPService response (MCPService.credentials), so there is no GET on this +// endpoint — only PUT (write) and DELETE (clear). Both trigger an MCP +// client reconnect server-side. +// ---------------------------------------------------------------------------- + +export type McpCredentialField = 'api_key' | 'token' + +export interface CredentialFieldMetadata { + configured: boolean +} + +export interface McpCredentialsResponse { + fields: Record +} + +export async function putMCPCredentials( + serviceId: string, + body: Partial> +): Promise { + const response: any = await put(`/api/v1/mcp-services/${serviceId}/credentials`, body) + return (response.data ?? response) as McpCredentialsResponse +} + +export async function deleteMCPCredentialField( + serviceId: string, + field: McpCredentialField +): Promise { + await del(`/api/v1/mcp-services/${serviceId}/credentials/${field}`) +} + export async function resolveToolApproval( pendingId: string, body: { decision: 'approve' | 'reject'; modified_args?: Record; reason?: string } diff --git a/frontend/src/api/model/index.ts b/frontend/src/api/model/index.ts index 3ca7ac7a..1102adc2 100644 --- a/frontend/src/api/model/index.ts +++ b/frontend/src/api/model/index.ts @@ -27,15 +27,18 @@ export interface ModelConfig { custom_headers?: Record; supports_vision?: boolean; // Whether the model accepts image/multimodal input app_id?: string; + // Secret fields (api_key, app_secret) are never returned by the server in + // this shape — they live behind the /credentials subresource. They are + // kept on the type so create-mode payloads can still carry them in the + // initial POST body. app_secret?: string; - // Write-only flags — set to true in Update requests to explicitly remove - // the corresponding credential. The server never returns these fields. - clear_api_key?: boolean; - clear_app_secret?: boolean; }; is_default?: boolean; is_builtin?: boolean; status?: string; + // Per-field configured? metadata from the main response. Absent for + // builtin models. + credentials?: Record; created_at?: string; updated_at?: string; deleted_at?: string | null; @@ -135,6 +138,32 @@ export function deleteModel(id: string): Promise { }); } +// ---------------------------------------------------------------------------- +// Model credential subresource. See mcp-service.ts for the matching MCP API +// shape and the design notes in internal/handler/dto/mcp.go. +// ---------------------------------------------------------------------------- + +export type ModelCredentialField = 'api_key' | 'app_secret' + +export interface ModelCredentialsResponse { + fields: Record +} + +export async function putModelCredentials( + id: string, + body: Partial>, +): Promise { + const response: any = await put(`/api/v1/models/${id}/credentials`, body) + return (response.data ?? response) as ModelCredentialsResponse +} + +export async function deleteModelCredentialField( + id: string, + field: ModelCredentialField, +): Promise { + await del(`/api/v1/models/${id}/credentials/${field}`) +} + export interface InitializeWeKnoraCloudRequest { app_id: string app_secret: string diff --git a/frontend/src/api/web-search-provider.ts b/frontend/src/api/web-search-provider.ts index 5c0c8c95..c99c73e9 100644 --- a/frontend/src/api/web-search-provider.ts +++ b/frontend/src/api/web-search-provider.ts @@ -8,16 +8,18 @@ export interface WebSearchProviderEntity { provider: 'bing' | 'google' | 'duckduckgo' | 'tavily' | 'ollama' | 'baidu' | 'searxng' description?: string parameters: { + // api_key is never returned by the server in this shape; it lives behind + // the /credentials subresource. Kept on the type so the initial create + // POST can still include it. api_key?: string engine_id?: string base_url?: string proxy_url?: string extra_config?: Record - // Write-only flag — set to true in Update requests to explicitly remove - // the stored API key. The server never returns this field. - clear_api_key?: boolean } is_default?: boolean + // Per-field configured? metadata from the main response. + credentials?: Record created_at?: string updated_at?: string } @@ -69,6 +71,31 @@ export function listWebSearchProviderTypes(): Promise +} + +export async function putWebSearchProviderCredentials( + id: string, + body: Partial>, +): Promise { + const response: any = await put(`/api/v1/web-search-providers/${id}/credentials`, body) + return (response.data ?? response) as WebSearchCredentialsResponse +} + +export async function deleteWebSearchProviderCredentialField( + id: string, + field: WebSearchCredentialField, +): Promise { + await del(`/api/v1/web-search-providers/${id}/credentials/${field}`) +} + // Test a web search provider connection. // If id is provided, tests the existing saved provider. // If data is provided, tests with raw credentials (no persistence). diff --git a/frontend/src/components/ModelEditorDialog.vue b/frontend/src/components/ModelEditorDialog.vue index 6b694ed8..a997cce3 100644 --- a/frontend/src/components/ModelEditorDialog.vue +++ b/frontend/src/components/ModelEditorDialog.vue @@ -1,339 +1,240 @@