diff --git a/api/api.go b/api/api.go index 791ab8e5b..cb5fdd14d 100644 --- a/api/api.go +++ b/api/api.go @@ -70,11 +70,10 @@ type MCPClientManager interface { GetConfig() mcp.Config RegisterPluginServer(cfg mcp.PluginServerConfig) - UpdatePluginServer(cfg mcp.PluginServerConfig) + UpdatePluginServerAdminFields(pluginID string, enabled bool, toolConfigs []mcp.ToolConfig) (mcp.PluginServerConfig, bool) UnregisterPluginServer(pluginID string) ListPluginServers() []mcp.PluginServerConfig GetPluginServer(pluginID string) (mcp.PluginServerConfig, bool) - IsPluginRegistered(pluginID string) bool DiscoverPluginServerTools(ctx context.Context, userID string, cfg mcp.PluginServerConfig) ([]mcp.ToolInfo, error) } @@ -83,6 +82,10 @@ type MCPClientManager interface { type ConfigStore interface { GetConfig() (*config.Config, error) SaveConfig(cfg config.Config) error + // UpdateConfig atomically reads the active config, applies transform, and + // persists the result under the config advisory lock. A transform error + // aborts the update and is returned as-is. + UpdateConfig(transform func(prev *config.Config) (config.Config, error)) (config.Config, error) } // AgentStore provides CRUD access to user-created agents in the database. diff --git a/api/api_admin.go b/api/api_admin.go index bd6ea99cf..8f357cc42 100644 --- a/api/api_admin.go +++ b/api/api_admin.go @@ -13,6 +13,7 @@ import ( "github.com/gin-gonic/gin" "github.com/mattermost/mattermost-plugin-agents/v2/audit" + "github.com/mattermost/mattermost-plugin-agents/v2/config" "github.com/mattermost/mattermost-plugin-agents/v2/indexer" "github.com/mattermost/mattermost-plugin-agents/v2/mcp" "github.com/mattermost/mattermost-plugin-agents/v2/mmapi" @@ -284,6 +285,8 @@ type MCPServerInfo struct { Enabled bool `json:"enabled"` // ToolConfigs is populated for plugin rows only. ToolConfigs []mcp.ToolConfig `json:"toolConfigs,omitempty"` + // ID is the stable ABAC policy identity when present. + ID string `json:"id,omitempty"` } // MCPToolsResponse represents the response structure for MCP tools endpoint @@ -362,6 +365,7 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis // Embedded MCP is always available after PR #617, even if older // configs still have the legacy toggle stored as false. Enabled: true, + ID: embeddedConfig.ID, }, discover: func() ([]MCPToolInfo, error) { return a.discoverEmbeddedServerTools(ctx, userID, embeddedConfig, embeddedServer) @@ -389,6 +393,7 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis Tools: []MCPToolInfo{}, ServerType: "remote", Enabled: serverConfig.Enabled, + ID: serverConfig.ID, }, } if message, conflicting := conflictMessages[i]; conflicting { @@ -402,12 +407,10 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis } // Render disabled plugin entries (with an empty tool list) so the admin UI - // can re-enable them. Skip orphans hydrated from persisted config without - // a live source-plugin registration; surfacing them as "session not found" - // rows is misleading, and their persisted policy is preserved on disk - // regardless of whether they appear here. + // can re-enable them. Skip config-only orphans without a live source-plugin + // registration; their persisted identity and policy remain on disk. for _, cfg := range a.mcpClientManager.ListPluginServers() { - if !a.mcpClientManager.IsPluginRegistered(cfg.PluginID) { + if !isPluginMCPServerRegistered(a.mcpClientManager, cfg.PluginID) { continue } @@ -419,6 +422,7 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis ServerType: "plugin", Enabled: cfg.Enabled, ToolConfigs: cfg.ToolConfigs, + ID: cfg.ID, }, } if cfg.Enabled { @@ -432,6 +436,21 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis return rows } +func isPluginMCPServerRegistered(manager MCPClientManager, pluginID string) bool { + registry, ok := manager.(interface { + IsPluginRegistered(string) bool + }) + if ok { + return registry.IsPluginRegistered(pluginID) + } + for _, cfg := range manager.ListPluginServers() { + if cfg.PluginID == pluginID { + return true + } + } + return false +} + func applyMCPDiscoveryResult(info *MCPServerInfo, tools []MCPToolInfo, err error) { if err == nil { info.Tools = tools @@ -556,7 +575,7 @@ func (a *API) handleUpdatePluginServer(c *gin.Context) { audit.AddParam(auditRec(c), "tool_configs_changed", req.ToolConfigs != nil) live, foundLive := a.mcpClientManager.GetPluginServer(pluginID) - if !foundLive { + if !foundLive || !isPluginMCPServerRegistered(a.mcpClientManager, pluginID) { c.AbortWithError(http.StatusNotFound, fmt.Errorf("plugin MCP server %q is not registered", pluginID)) return } @@ -569,40 +588,57 @@ func (a *API) handleUpdatePluginServer(c *gin.Context) { updated.ToolConfigs = *req.ToolConfigs } - // Effective final value after partial-update merge, not the raw request. + // Best-effort fallback; overwritten below when persisted state is available. audit.AddParam(auditRec(c), "enabled", updated.Enabled) - existing, getErr := a.configStore.GetConfig() - if getErr != nil { - c.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to load config for plugin-server save: %w", getErr)) - return - } - if existing == nil { - c.AbortWithError(http.StatusInternalServerError, errors.New("no plugin configuration available")) - return - } - // Clone to avoid mutating the store's cached pointer. - cfg := existing.Clone() - - // Merge by PluginID against the persisted list rather than overwriting - // with the in-memory snapshot, which would silently drop entries for - // plugins that are persisted but currently inactive in memory. - merged := append([]mcp.PluginServerConfig(nil), cfg.MCP.PluginServers...) - mergedIdx := -1 - for i := range merged { - if merged[i].PluginID == updated.PluginID { - mergedIdx = i - break + // Apply the request to the freshest persisted entry inside the UpdateConfig + // transform; rebasing onto the live snapshot read above would let two + // concurrent field updates overwrite each other. + saved, err := a.configStore.UpdateConfig(func(prev *config.Config) (config.Config, error) { + if prev == nil { + // Replacing a nil persisted config with a zero-value baseline + // would clobber unrelated settings on the next save. + return config.Config{}, errors.New("no plugin configuration available") } - } - if mergedIdx >= 0 { - merged[mergedIdx] = updated - } else { - merged = append(merged, updated) - } - cfg.MCP.PluginServers = merged + // Clone to avoid mutating the store's cached pointer. + cfg := prev.Clone() + + // Merge by PluginID against the persisted list rather than overwriting + // with the in-memory snapshot, which would silently drop entries for + // plugins that are persisted but currently inactive in memory. + merged := append([]mcp.PluginServerConfig(nil), cfg.MCP.PluginServers...) + mergedIdx := -1 + for i := range merged { + if merged[i].PluginID == pluginID { + mergedIdx = i + break + } + } + + // Live entry owns Name/Path/ExposeExternal; persisted admin fields + // (Enabled, ToolConfigs, ID) overlay via the shared merge helper. + base := live + if mergedIdx >= 0 { + base = mcp.ApplyPersistedPluginServerFields(base, merged[mergedIdx]) + } + if req.Enabled != nil { + base.Enabled = *req.Enabled + } + if req.ToolConfigs != nil { + base.ToolConfigs = *req.ToolConfigs + } + updated = base + audit.AddParam(auditRec(c), "enabled", updated.Enabled) - if err := a.configStore.SaveConfig(*cfg); err != nil { + if mergedIdx >= 0 { + merged[mergedIdx] = base + } else { + merged = append(merged, base) + } + cfg.MCP.PluginServers = merged + return *cfg, nil + }) + if err != nil { c.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to save plugin-server config: %w", err)) return } @@ -616,8 +652,13 @@ func (a *API) handleUpdatePluginServer(c *gin.Context) { return } - a.mcpClientManager.UpdatePluginServer(updated) - a.configUpdater.Update(cfg) + // Patch admin fields rather than replace the whole object, so plugin-owned + // fields survive a concurrent re-registration. A missing entry means the + // plugin unregistered; it is hydrated again on its next registration. + if patched, ok := a.mcpClientManager.UpdatePluginServerAdminFields(pluginID, updated.Enabled, updated.ToolConfigs); ok { + updated = patched + } + a.configUpdater.Update(&saved) // Rebuild when either old or new state was external so removed tools // disappear from the aggregate server. diff --git a/api/api_admin_test.go b/api/api_admin_test.go index efc9e71b5..6b3effc3f 100644 --- a/api/api_admin_test.go +++ b/api/api_admin_test.go @@ -29,6 +29,7 @@ import ( "github.com/mattermost/mattermost/server/public/plugin" "github.com/mattermost/mattermost/server/public/plugin/plugintest" "github.com/mattermost/mattermost/server/public/pluginapi" + gomcp "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -571,6 +572,92 @@ func TestHandleGetMCPTools_OmitsOrphanPluginServers(t *testing.T) { "orphan plugin must not be probed; probing would surface a misleading session-not-found error") } +type stubAdminEmbeddedServer struct{} + +func (s *stubAdminEmbeddedServer) CreateClientTransport(string, string, *pluginapi.Client) (*gomcp.InMemoryTransport, error) { + return nil, errors.New("stub: discovery not needed for ID assertions") +} + +func TestHandleGetMCPTools_ReturnsStableIDs(t *testing.T) { + api, mockAPI, _ := setupAdminTestEnvironment(t) + defer mockAPI.AssertExpectations(t) + + mockAPI.On("HasPermissionTo", "admin-user", model.PermissionManageSystem).Return(true).Maybe() + mockAPI.On("LogError", mock.Anything).Return().Maybe() + mockAPI.On("LogDebug", mock.Anything).Return().Maybe() + + const ( + embeddedID = "abcdefghijklmnopqrstuvwx01" + remoteID = "abcdefghijklmnopqrstuvwx02" + pluginID = "abcdefghijklmnopqrstuvwx03" + ) + + cfg := api.config.(*testConfigImpl) + cfg.mcpConfig = mcp.Config{ + Enabled: true, + EmbeddedServer: mcp.EmbeddedServerConfig{ + ID: embeddedID, + Enabled: true, + }, + Servers: []mcp.ServerConfig{{ + ID: remoteID, + Name: "Remote", + Enabled: true, + BaseURL: "https://mcp.example.com", + }}, + PluginServers: []mcp.PluginServerConfig{{ + ID: pluginID, + PluginID: "com.mattermost.demo", + Name: "Demo", + Path: "/mcp", + Enabled: true, + }}, + } + + mgr := api.mcpClientManager.(*mockMCPClientManager) + mgr.embeddedServer = &stubAdminEmbeddedServer{} + // Non-nil client so remote discovery fails cleanly instead of panicking. + mgr.httpClient = &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return nil, errors.New("stub: discovery not needed for ID assertions") + })} + // Live registry carries the effective ID after config sync. + mgr.pluginServers = []mcp.PluginServerConfig{{ + ID: pluginID, + PluginID: "com.mattermost.demo", + Name: "Demo", + Path: "/mcp", + Enabled: true, + }} + mgr.discoverPluginToolsResponse = []mcp.ToolInfo{{Name: "echo"}} + + req := httptest.NewRequest(http.MethodGet, "/admin/mcp/tools", nil) + req.Header.Set("Mattermost-User-Id", "admin-user") + + recorder := httptest.NewRecorder() + api.ServeHTTP(&plugin.Context{}, recorder, req) + + resp := recorder.Result() + require.Equal(t, http.StatusOK, resp.StatusCode) + + var body MCPToolsResponse + require.NoError(t, json.NewDecoder(resp.Body).Decode(&body)) + + byType := map[string]MCPServerInfo{} + for _, s := range body.Servers { + byType[s.ServerType] = s + } + + require.Equal(t, embeddedID, byType["embedded"].ID) + require.Equal(t, remoteID, byType["remote"].ID) + require.Equal(t, pluginID, byType["plugin"].ID) +} + +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { + return f(r) +} + func TestHandleUpdatePluginServer(t *testing.T) { tests := []struct { name string @@ -579,12 +666,13 @@ func TestHandleUpdatePluginServer(t *testing.T) { body string hasAdminPerm bool expectStatus int - expectRegistryCalls int + expectPatchCalls int expectEnabledAfter bool expectExposeAfter bool expectToolConfigsAfter []mcp.ToolConfig expectRebuildCalls int orphanPluginIDs map[string]bool + expectStateUnchanged bool }{ { name: "happy path: flips Enabled true->false", @@ -592,13 +680,13 @@ func TestHandleUpdatePluginServer(t *testing.T) { preRegistered: []mcp.PluginServerConfig{{ PluginID: "com.mattermost.demo", Name: "Demo", Path: "/mcp", Enabled: true, }}, - body: `{"enabled": false}`, - hasAdminPerm: true, - expectStatus: http.StatusOK, - expectRegistryCalls: 1, - expectEnabledAfter: false, - expectExposeAfter: false, - expectRebuildCalls: 0, + body: `{"enabled": false}`, + hasAdminPerm: true, + expectStatus: http.StatusOK, + expectPatchCalls: 1, + expectEnabledAfter: false, + expectExposeAfter: false, + expectRebuildCalls: 0, }, { name: "enabled update preserves existing ExposeExternal", @@ -607,13 +695,13 @@ func TestHandleUpdatePluginServer(t *testing.T) { PluginID: "com.mattermost.demo", Name: "Demo", Path: "/mcp", Enabled: true, ExposeExternal: true, }}, - body: `{"enabled": false}`, - hasAdminPerm: true, - expectStatus: http.StatusOK, - expectRegistryCalls: 1, - expectEnabledAfter: false, - expectExposeAfter: true, - expectRebuildCalls: 1, + body: `{"enabled": false}`, + hasAdminPerm: true, + expectStatus: http.StatusOK, + expectPatchCalls: 1, + expectEnabledAfter: false, + expectExposeAfter: true, + expectRebuildCalls: 1, }, { name: "expose_external field is ignored", @@ -622,13 +710,13 @@ func TestHandleUpdatePluginServer(t *testing.T) { PluginID: "com.mattermost.demo", Name: "Demo", Path: "/mcp", Enabled: true, ExposeExternal: false, }}, - body: `{"expose_external": true}`, - hasAdminPerm: true, - expectStatus: http.StatusOK, - expectRegistryCalls: 1, - expectEnabledAfter: true, - expectExposeAfter: false, - expectRebuildCalls: 0, + body: `{"expose_external": true}`, + hasAdminPerm: true, + expectStatus: http.StatusOK, + expectPatchCalls: 1, + expectEnabledAfter: true, + expectExposeAfter: false, + expectRebuildCalls: 0, }, { name: "empty body preserves both fields", @@ -637,26 +725,27 @@ func TestHandleUpdatePluginServer(t *testing.T) { PluginID: "com.mattermost.demo", Name: "Demo", Path: "/mcp", Enabled: true, ExposeExternal: true, }}, - body: `{}`, - hasAdminPerm: true, - expectStatus: http.StatusOK, - expectRegistryCalls: 1, - expectEnabledAfter: true, - expectExposeAfter: true, - expectRebuildCalls: 1, + body: `{}`, + hasAdminPerm: true, + expectStatus: http.StatusOK, + expectPatchCalls: 1, + expectEnabledAfter: true, + expectExposeAfter: true, + expectRebuildCalls: 1, }, { - name: "admin update keeps config-only orphan unregistered", + name: "admin update rejects config-only orphan", pluginID: "com.mattermost.demo", preRegistered: []mcp.PluginServerConfig{{ PluginID: "com.mattermost.demo", Name: "Demo", Path: "/mcp", Enabled: true, }}, - orphanPluginIDs: map[string]bool{"com.mattermost.demo": true}, - body: `{"enabled": false}`, - hasAdminPerm: true, - expectStatus: http.StatusOK, - expectRegistryCalls: 1, - expectEnabledAfter: false, + orphanPluginIDs: map[string]bool{"com.mattermost.demo": true}, + body: `{"enabled": false}`, + hasAdminPerm: true, + expectStatus: http.StatusNotFound, + expectPatchCalls: 0, + expectRebuildCalls: 0, + expectStateUnchanged: true, }, { name: "404 when pluginID not registered", @@ -681,10 +770,10 @@ func TestHandleUpdatePluginServer(t *testing.T) { preRegistered: []mcp.PluginServerConfig{{ PluginID: "com.mattermost.demo", Name: "Demo", Path: "/mcp", Enabled: true, }}, - body: `{"enabled": false}`, - hasAdminPerm: false, - expectStatus: http.StatusForbidden, - expectRegistryCalls: 0, + body: `{"enabled": false}`, + hasAdminPerm: false, + expectStatus: http.StatusForbidden, + expectPatchCalls: 0, }, { name: "tool_configs partial PUT sets policy, preserves enabled", @@ -693,12 +782,12 @@ func TestHandleUpdatePluginServer(t *testing.T) { PluginID: "com.mattermost.demo", Name: "Demo", Path: "/mcp", Enabled: true, ExposeExternal: false, }}, - body: `{"tool_configs": [{"name": "echo", "policy": "ask", "enabled": false}]}`, - hasAdminPerm: true, - expectStatus: http.StatusOK, - expectRegistryCalls: 1, - expectEnabledAfter: true, - expectExposeAfter: false, + body: `{"tool_configs": [{"name": "echo", "policy": "ask", "enabled": false}]}`, + hasAdminPerm: true, + expectStatus: http.StatusOK, + expectPatchCalls: 1, + expectEnabledAfter: true, + expectExposeAfter: false, expectToolConfigsAfter: []mcp.ToolConfig{ {Name: "echo", Policy: "ask", Enabled: false}, }, @@ -716,7 +805,7 @@ func TestHandleUpdatePluginServer(t *testing.T) { body: `{"tool_configs": []}`, hasAdminPerm: true, expectStatus: http.StatusOK, - expectRegistryCalls: 1, + expectPatchCalls: 1, expectEnabledAfter: true, expectExposeAfter: false, expectToolConfigsAfter: []mcp.ToolConfig{}, @@ -732,12 +821,12 @@ func TestHandleUpdatePluginServer(t *testing.T) { {Name: "echo", Policy: "auto_run_in_dm", Enabled: true}, }, }}, - body: `{"enabled": false}`, - hasAdminPerm: true, - expectStatus: http.StatusOK, - expectRegistryCalls: 1, - expectEnabledAfter: false, - expectExposeAfter: false, + body: `{"enabled": false}`, + hasAdminPerm: true, + expectStatus: http.StatusOK, + expectPatchCalls: 1, + expectEnabledAfter: false, + expectExposeAfter: false, expectToolConfigsAfter: []mcp.ToolConfig{ {Name: "echo", Policy: "auto_run_in_dm", Enabled: true}, }, @@ -756,6 +845,11 @@ func TestHandleUpdatePluginServer(t *testing.T) { mgr := api.mcpClientManager.(*mockMCPClientManager) mgr.pluginServers = tt.preRegistered mgr.orphanPluginIDs = tt.orphanPluginIDs + pluginServersBefore := append([]mcp.PluginServerConfig(nil), mgr.pluginServers...) + orphanPluginIDsBefore := make(map[string]bool, len(mgr.orphanPluginIDs)) + for pluginID, orphaned := range mgr.orphanPluginIDs { + orphanPluginIDsBefore[pluginID] = orphaned + } // Seed a baseline persisted config so the handler can clone it // instead of treating the store's nil as a 500. @@ -775,21 +869,26 @@ func TestHandleUpdatePluginServer(t *testing.T) { require.Equal(t, tt.expectStatus, resp.StatusCode) require.Empty(t, mgr.registerCalls) - require.Len(t, mgr.updateCalls, tt.expectRegistryCalls) + require.Len(t, mgr.adminPatchCalls, tt.expectPatchCalls) if tt.expectStatus == http.StatusOK { - require.Equal(t, tt.expectEnabledAfter, mgr.updateCalls[0].Enabled) - require.Equal(t, tt.expectExposeAfter, mgr.updateCalls[0].ExposeExternal) - require.Equal(t, "Demo", mgr.updateCalls[0].Name) - require.Equal(t, "/mcp", mgr.updateCalls[0].Path) - require.Equal(t, "com.mattermost.demo", mgr.updateCalls[0].PluginID) + require.Equal(t, tt.expectEnabledAfter, mgr.adminPatchCalls[0].Enabled) + require.Equal(t, tt.expectExposeAfter, mgr.adminPatchCalls[0].ExposeExternal) + require.Equal(t, "Demo", mgr.adminPatchCalls[0].Name) + require.Equal(t, "/mcp", mgr.adminPatchCalls[0].Path) + require.Equal(t, "com.mattermost.demo", mgr.adminPatchCalls[0].PluginID) if tt.expectToolConfigsAfter != nil { - require.Equal(t, tt.expectToolConfigsAfter, mgr.updateCalls[0].ToolConfigs, "ToolConfigs assertion") - } - if tt.orphanPluginIDs[tt.pluginID] { - require.False(t, mgr.IsPluginRegistered(tt.pluginID)) + require.Equal(t, tt.expectToolConfigsAfter, mgr.adminPatchCalls[0].ToolConfigs, "ToolConfigs assertion") } } require.Equal(t, tt.expectRebuildCalls, spy.callCount) + if tt.expectStateUnchanged { + require.Equal(t, pluginServersBefore, mgr.pluginServers) + require.Equal(t, orphanPluginIDsBefore, mgr.orphanPluginIDs) + require.Empty(t, mgr.unregisterCalls) + require.Equal(t, &config.Config{}, stores.configStore.cfg) + require.Zero(t, stores.configUpdater.callCount) + require.Zero(t, stores.clusterNotifier.callCount) + } }) } } @@ -808,7 +907,7 @@ func TestHandleUpdatePluginServer_PersistsToConfig(t *testing.T) { expectSaveCalls int expectUpdateCalls int expectPublishCalls int - expectRegistryCalls int + expectPatchCalls int expectUnregisterCalls int assertPersistedState func(t *testing.T, savedCfg *config.Config) }{ @@ -825,7 +924,7 @@ func TestHandleUpdatePluginServer_PersistsToConfig(t *testing.T) { expectSaveCalls: 1, expectUpdateCalls: 1, expectPublishCalls: 1, - expectRegistryCalls: 1, + expectPatchCalls: 1, expectUnregisterCalls: 0, assertPersistedState: func(t *testing.T, savedCfg *config.Config) { require.Len(t, savedCfg.MCP.PluginServers, 1) @@ -870,7 +969,7 @@ func TestHandleUpdatePluginServer_PersistsToConfig(t *testing.T) { expectSaveCalls: 1, expectUpdateCalls: 1, expectPublishCalls: 1, - expectRegistryCalls: 1, + expectPatchCalls: 1, expectUnregisterCalls: 0, assertPersistedState: func(t *testing.T, savedCfg *config.Config) { require.Len(t, savedCfg.MCP.PluginServers, 2, @@ -906,7 +1005,7 @@ func TestHandleUpdatePluginServer_PersistsToConfig(t *testing.T) { expectSaveCalls: 0, expectUpdateCalls: 0, expectPublishCalls: 0, - expectRegistryCalls: 0, + expectPatchCalls: 0, expectUnregisterCalls: 0, }, { @@ -920,7 +1019,7 @@ func TestHandleUpdatePluginServer_PersistsToConfig(t *testing.T) { expectSaveCalls: 1, expectUpdateCalls: 0, expectPublishCalls: 0, - expectRegistryCalls: 0, + expectPatchCalls: 0, expectUnregisterCalls: 0, }, { @@ -934,7 +1033,7 @@ func TestHandleUpdatePluginServer_PersistsToConfig(t *testing.T) { expectSaveCalls: 1, expectUpdateCalls: 0, expectPublishCalls: 1, - expectRegistryCalls: 0, + expectPatchCalls: 0, expectUnregisterCalls: 0, }, { @@ -951,7 +1050,7 @@ func TestHandleUpdatePluginServer_PersistsToConfig(t *testing.T) { expectSaveCalls: 0, expectUpdateCalls: 0, expectPublishCalls: 0, - expectRegistryCalls: 0, + expectPatchCalls: 0, expectUnregisterCalls: 0, }, } @@ -1006,7 +1105,7 @@ func TestHandleUpdatePluginServer_PersistsToConfig(t *testing.T) { require.Equal(t, tt.expectPublishCalls, stores.clusterNotifier.callCount) require.Empty(t, mgr.registerCalls) - require.Len(t, mgr.updateCalls, tt.expectRegistryCalls, "live plugin registry must not be mutated on failure paths") + require.Len(t, mgr.adminPatchCalls, tt.expectPatchCalls, "live plugin registry must not be mutated on failure paths") require.Len(t, mgr.unregisterCalls, tt.expectUnregisterCalls, "live plugin registry must not be mutated on failure paths") if tt.assertPersistedState != nil { @@ -1017,6 +1116,106 @@ func TestHandleUpdatePluginServer_PersistsToConfig(t *testing.T) { } } +// Lost-update regression: two updates to different fields that both read the +// same live state must both survive in the persisted config (the merge must +// rebase onto the freshest persisted entry inside the UpdateConfig transform). +func TestHandleUpdatePluginServer_ConcurrentFieldUpdatesBothSurvive(t *testing.T) { + api, mockAPI, stores := setupAdminTestEnvironment(t) + defer mockAPI.AssertExpectations(t) + + mockAPI.On("HasPermissionTo", "admin-user", model.PermissionManageSystem).Return(true).Maybe() + mockAPI.On("LogError", mock.Anything).Return().Maybe() + + // Fresh copies each time: the mock's RegisterPluginServer mutates the + // slice in place, and the reset below must restore the pristine state. + liveSnapshot := func() []mcp.PluginServerConfig { + return []mcp.PluginServerConfig{{ + PluginID: "com.mattermost.demo", Name: "Demo", Path: "/mcp", Enabled: true, + }} + } + mgr := api.mcpClientManager.(*mockMCPClientManager) + mgr.pluginServers = liveSnapshot() + stores.configStore.cfg = &config.Config{} + + put := func(body string) int { + req := httptest.NewRequest(http.MethodPut, "/admin/mcp/plugin-servers/com.mattermost.demo", strings.NewReader(body)) + req.Header.Set("Mattermost-User-Id", "admin-user") + req.Header.Set("Content-Type", "application/json") + recorder := httptest.NewRecorder() + api.ServeHTTP(&plugin.Context{}, recorder, req) + return recorder.Result().StatusCode + } + + require.Equal(t, http.StatusOK, put(`{"enabled": false}`)) + + // Pin the exact racing interleaving: the second request read the live + // registry before the first request's admin-field patch landed, so it + // sees the original Enabled:true snapshot. + mgr.pluginServers = liveSnapshot() + + require.Equal(t, http.StatusOK, put(`{"tool_configs": [{"name": "echo", "policy": "ask", "enabled": false}]}`)) + + require.Len(t, stores.configStore.cfg.MCP.PluginServers, 1) + persisted := stores.configStore.cfg.MCP.PluginServers[0] + require.False(t, persisted.Enabled, "first update's Enabled=false must survive the second update") + require.Len(t, persisted.ToolConfigs, 1, "second update's tool_configs must be applied") + require.Equal(t, "echo", persisted.ToolConfigs[0].Name) +} + +// Pins field ownership: the source plugin owns Name/Path/ExposeExternal and +// admins own Enabled/ToolConfigs. An admin update whose persisted entry +// predates a plugin re-registration must not resurrect stale plugin-owned +// values in the persisted config, the runtime registry, or the response. +func TestHandleUpdatePluginServer_PluginOwnedFieldsNotReverted(t *testing.T) { + api, mockAPI, stores := setupAdminTestEnvironment(t) + defer mockAPI.AssertExpectations(t) + + mockAPI.On("HasPermissionTo", "admin-user", model.PermissionManageSystem).Return(true).Maybe() + mockAPI.On("LogError", mock.Anything).Return().Maybe() + + // The plugin re-registered with new plugin-owned values after the entry + // below was persisted. + mgr := api.mcpClientManager.(*mockMCPClientManager) + mgr.pluginServers = []mcp.PluginServerConfig{{ + PluginID: "com.mattermost.demo", Name: "Demo v2", Path: "/mcp/v2", + Enabled: true, ExposeExternal: true, + }} + stores.configStore.cfg = &config.Config{} + stores.configStore.cfg.MCP.PluginServers = []config.PluginServerConfig{{ + PluginID: "com.mattermost.demo", Name: "Demo", Path: "/mcp", + Enabled: false, ExposeExternal: false, + ToolConfigs: []config.MCPToolConfig{{Name: "echo", Policy: config.MCPToolPolicyAsk, Enabled: false}}, + }} + + spy := &spyRebuilder{} + api.SetExternalRebuilderForTest(spy) + + req := httptest.NewRequest(http.MethodPut, "/admin/mcp/plugin-servers/com.mattermost.demo", strings.NewReader(`{"enabled": true}`)) + req.Header.Set("Mattermost-User-Id", "admin-user") + req.Header.Set("Content-Type", "application/json") + recorder := httptest.NewRecorder() + api.ServeHTTP(&plugin.Context{}, recorder, req) + require.Equal(t, http.StatusOK, recorder.Result().StatusCode) + + require.Len(t, stores.configStore.cfg.MCP.PluginServers, 1) + persisted := stores.configStore.cfg.MCP.PluginServers[0] + require.Equal(t, "Demo v2", persisted.Name, "persisted Name must come from the live registration") + require.Equal(t, "/mcp/v2", persisted.Path, "persisted Path must come from the live registration") + require.True(t, persisted.ExposeExternal, "persisted ExposeExternal must come from the live registration") + require.True(t, persisted.Enabled, "request patch must apply") + require.Equal(t, []config.MCPToolConfig{{Name: "echo", Policy: config.MCPToolPolicyAsk, Enabled: false}}, + persisted.ToolConfigs, "persisted admin-owned ToolConfigs must carry over") + + require.Len(t, mgr.adminPatchCalls, 1, "runtime registration must be an admin-field patch") + require.Empty(t, mgr.registerCalls, "runtime registration must not be a whole-object replacement") + patched := mgr.adminPatchCalls[0] + require.Equal(t, "Demo v2", patched.Name) + require.Equal(t, "/mcp/v2", patched.Path) + require.True(t, patched.ExposeExternal) + require.True(t, patched.Enabled) + require.Equal(t, []mcp.ToolConfig{{Name: "echo", Policy: config.MCPToolPolicyAsk, Enabled: false}}, patched.ToolConfigs) +} + // failingConfigStore is a testConfigStore variant with configurable error injection on Get/Save. type failingConfigStore struct { cfg *config.Config @@ -1039,6 +1238,20 @@ func (s *failingConfigStore) SaveConfig(cfg config.Config) error { return nil } +func (s *failingConfigStore) UpdateConfig(transform func(prev *config.Config) (config.Config, error)) (config.Config, error) { + if s.getErr != nil { + return config.Config{}, s.getErr + } + next, err := transform(s.cfg) + if err != nil { + return config.Config{}, err + } + if err := s.SaveConfig(next); err != nil { + return next, err + } + return next, nil +} + // setupAdminAuditTest wires the full-router environment with audit capture so // the audit middleware runs end to end for admin routes. func setupAdminAuditTest(t *testing.T) (*TestEnvironment, *[]*model.AuditRecord) { diff --git a/api/api_agents_test.go b/api/api_agents_test.go index 0f6fac352..2ab25b55f 100644 --- a/api/api_agents_test.go +++ b/api/api_agents_test.go @@ -66,6 +66,21 @@ func (m *mockConfigStore) SaveConfig(cfg config.Config) error { return nil } +func (m *mockConfigStore) UpdateConfig(transform func(prev *config.Config) (config.Config, error)) (config.Config, error) { + var prev *config.Config + if m.cfg != nil { + prev = m.cfg + } + next, err := transform(prev) + if err != nil { + return next, err + } + if err := m.SaveConfig(next); err != nil { + return next, err + } + return next, nil +} + // overrideLicenseMocks replaces any GetConfig/GetLicense expectations already // registered (e.g. by SetupTestEnvironment). Testify matches the first // registered expectation, so simply adding new ones would not take effect. diff --git a/api/api_bridge_mcp.go b/api/api_bridge_mcp.go index b96182c06..b72a77e65 100644 --- a/api/api_bridge_mcp.go +++ b/api/api_bridge_mcp.go @@ -4,13 +4,16 @@ package api import ( + "errors" "fmt" "net/http" "github.com/gin-gonic/gin" "github.com/mattermost/mattermost-plugin-agents/v2/audit" + "github.com/mattermost/mattermost-plugin-agents/v2/config" "github.com/mattermost/mattermost-plugin-agents/v2/mcp" "github.com/mattermost/mattermost-plugin-agents/v2/public/bridgeclient" + "github.com/mattermost/mattermost/server/public/model" ) // externalServerRebuilder rebuilds the external MCP aggregate after plugin changes. @@ -98,15 +101,29 @@ func (a *API) handleMCPRegister(c *gin.Context) { // Snapshot effective external exposure so we rebuild when it turns on or off. prevEffectiveExternal := a.pluginServerExternallyExposed(trustedPluginID) - // Preserve Enabled and ToolConfigs across re-registration, even after unregister. - // A first-time registration with no explicit enabled flag defaults to enabled. - persisted, hasPersisted := a.findPersistedPluginServer(trustedPluginID) + // Overlay admin-owned fields (Enabled, ToolConfigs, ID). Live entry first, + // then persisted config — so a re-register after unregister recovers from + // config, while a live-only ID is not rotated when the config row is absent. if existing, found := a.mcpClientManager.GetPluginServer(trustedPluginID); found { - cfg.Enabled = existing.Enabled - cfg.ToolConfigs = existing.ToolConfigs - } else if hasPersisted { - cfg.Enabled = persisted.Enabled - cfg.ToolConfigs = persisted.ToolConfigs + cfg = mcp.ApplyPersistedPluginServerFields(cfg, existing) + } + persisted, hasPersisted := a.findPersistedPluginServer(trustedPluginID) + if hasPersisted { + cfg = mcp.ApplyPersistedPluginServerFields(cfg, persisted) + } + + // Mint only when neither live nor persisted carried an ID. Persist when the + // config row is missing or ID-less so a live-only identity is written. + if cfg.ID == "" { + cfg.ID = model.NewId() + } + if !hasPersisted || persisted.ID == "" { + if err := a.persistPluginServerID(trustedPluginID, &cfg); err != nil { + c.JSON(http.StatusInternalServerError, bridgeclient.ErrorResponse{ + Error: fmt.Sprintf("failed to persist plugin server ID: %v", err), + }) + return + } } // Effective final value after the preserve-on-reregister merge, not the @@ -125,6 +142,66 @@ func (a *API) handleMCPRegister(c *gin.Context) { c.Status(http.StatusOK) } +// persistPluginServerID ensures cfg.MCP.PluginServers holds a stable ID for +// pluginID. Only mints when the entry is missing or ID-less; concurrent +// writers that already assigned an ID win (their ID is adopted onto +// registration). +func (a *API) persistPluginServerID(pluginID string, registration *mcp.PluginServerConfig) error { + if a.configStore == nil || registration == nil { + return nil + } + saved, err := a.configStore.UpdateConfig(func(prev *config.Config) (config.Config, error) { + if prev == nil { + return config.Config{}, errors.New("no plugin configuration available") + } + cfg := prev.Clone() + id := registration.ID + if id == "" { + id = model.NewId() + } + for i := range cfg.MCP.PluginServers { + if cfg.MCP.PluginServers[i].PluginID != pluginID { + continue + } + if cfg.MCP.PluginServers[i].ID == "" { + cfg.MCP.PluginServers[i].ID = id + } + return *cfg, nil + } + cfg.MCP.PluginServers = append(cfg.MCP.PluginServers, config.PluginServerConfig{ + ID: id, + PluginID: registration.PluginID, + Name: registration.Name, + Path: registration.Path, + Enabled: registration.Enabled, + ExposeExternal: registration.ExposeExternal, + ToolConfigs: registration.ToolConfigs, + }) + return *cfg, nil + }) + if err != nil { + return err + } + // Adopt the persisted ID in case another writer raced and minted first. + for i := range saved.MCP.PluginServers { + if saved.MCP.PluginServers[i].PluginID == pluginID && saved.MCP.PluginServers[i].ID != "" { + registration.ID = saved.MCP.PluginServers[i].ID + break + } + } + if a.configUpdater != nil { + a.configUpdater.Update(&saved) + } + // Propagation is best-effort; the ID is already persisted, so a failed + // notification must not abort this registration. + if a.clusterNotifier != nil { + if notifyErr := a.clusterNotifier.PublishConfigUpdate(); notifyErr != nil { + a.pluginAPI.Log.Warn("Failed to notify cluster of plugin-server ID", "error", notifyErr) + } + } + return nil +} + // pluginServerExternallyExposed reports whether the plugin should appear on the // external MCP server. func (a *API) pluginServerExternallyExposed(pluginID string) bool { diff --git a/api/api_bridge_mcp_test.go b/api/api_bridge_mcp_test.go index 1a14829c3..f8cb3e251 100644 --- a/api/api_bridge_mcp_test.go +++ b/api/api_bridge_mcp_test.go @@ -6,6 +6,7 @@ package api import ( "bytes" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" @@ -102,7 +103,10 @@ func TestHandleMCPRegister(t *testing.T) { wantStatus: http.StatusOK, assertMock: func(t *testing.T, m *mockMCPClientManager) { require.Len(t, m.registerCalls, 1) - require.Equal(t, validCfg, m.registerCalls[0]) + got := m.registerCalls[0] + require.True(t, model.IsValidId(got.ID), "register must mint a stable Mattermost ID") + got.ID = "" + require.Equal(t, validCfg, got) require.Empty(t, m.unregisterCalls) }, }, @@ -841,9 +845,130 @@ func TestHandleMCPRegister_PreservesAdminFieldsAfterUnregister(t *testing.T) { saved := e.mcp.registerCalls[0] require.Equal(t, true, saved.Enabled, "nil configStore: plugin payload preserved") require.Equal(t, false, saved.ExposeExternal, "nil configStore: plugin payload preserved") + require.True(t, model.IsValidId(saved.ID), "nil configStore: in-memory registration still receives a minted ID") }) } +func TestHandleMCPRegister_MintsStableIDAcrossReregister(t *testing.T) { + gin.SetMode(gin.ReleaseMode) + gin.DefaultWriter = io.Discard + + e := SetupTestEnvironment(t) + defer e.Cleanup(t) + + store := &testConfigStore{cfg: &config.Config{}} + e.api.configStore = store + e.api.configUpdater = &testConfigUpdater{} + e.api.clusterNotifier = &testClusterNotifier{} + + e.mockAPI.On("LogError", mock.Anything).Maybe() + e.mockAPI.On("LogError", mock.Anything, mock.Anything, mock.Anything).Maybe() + + body := mcp.PluginServerConfig{ + PluginID: testCallerPluginID, Name: "Playbooks MCP", Path: "/mcp", + Enabled: true, ExposeExternal: false, + } + req := mcpRegisterRequest(t, body) + req.Header.Set("Mattermost-Plugin-ID", testCallerPluginID) + resp := serveAndReturn(e, req) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Len(t, e.mcp.registerCalls, 1) + firstID := e.mcp.registerCalls[0].ID + require.True(t, model.IsValidId(firstID)) + require.Len(t, store.cfg.MCP.PluginServers, 1) + require.Equal(t, firstID, store.cfg.MCP.PluginServers[0].ID) + + // Unregister clears the in-memory registry but must leave the persisted ID. + unreg := mcpUnregisterRequest(t, struct{}{}) + unreg.Header.Set("Mattermost-Plugin-ID", testCallerPluginID) + require.Equal(t, http.StatusOK, serveAndReturn(e, unreg).StatusCode) + require.Equal(t, firstID, store.cfg.MCP.PluginServers[0].ID, "unregister must not remove the persisted ID") + + // Re-register with a different path: ID must be stable. + body.Path = "/mcp/v2" + req2 := mcpRegisterRequest(t, body) + req2.Header.Set("Mattermost-Plugin-ID", testCallerPluginID) + resp2 := serveAndReturn(e, req2) + require.Equal(t, http.StatusOK, resp2.StatusCode) + require.Len(t, e.mcp.registerCalls, 2) + require.Equal(t, firstID, e.mcp.registerCalls[1].ID, "re-register must reuse the persisted ID") + require.Equal(t, firstID, store.cfg.MCP.PluginServers[0].ID) + require.Len(t, store.cfg.MCP.PluginServers, 1, "re-register must not create a duplicate config entry") +} + +func TestHandleMCPRegister_ClusterNotifyFailureStillRegisters(t *testing.T) { + gin.SetMode(gin.ReleaseMode) + gin.DefaultWriter = io.Discard + + e := SetupTestEnvironment(t) + defer e.Cleanup(t) + + store := &testConfigStore{cfg: &config.Config{}} + updater := &testConfigUpdater{} + notifier := &testClusterNotifier{err: errors.New("cluster down")} + e.api.configStore = store + e.api.configUpdater = updater + e.api.clusterNotifier = notifier + + body := mcp.PluginServerConfig{ + PluginID: testCallerPluginID, Name: "Playbooks MCP", Path: "/mcp", + Enabled: true, ExposeExternal: false, + } + req := mcpRegisterRequest(t, body) + req.Header.Set("Mattermost-Plugin-ID", testCallerPluginID) + resp := serveAndReturn(e, req) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Len(t, e.mcp.registerCalls, 1, "registration must complete even when cluster notify fails") + require.True(t, model.IsValidId(e.mcp.registerCalls[0].ID)) + require.Len(t, store.cfg.MCP.PluginServers, 1) + require.Equal(t, 1, notifier.callCount) + require.Equal(t, 1, updater.callCount, "in-memory config must still be updated") +} + +// Live registry ID must win when the persisted PluginServers row is missing, +// and that ID must be written into config (not rotated). +func TestHandleMCPRegister_PreservesLiveIDWhenPersistedRowAbsent(t *testing.T) { + gin.SetMode(gin.ReleaseMode) + gin.DefaultWriter = io.Discard + + e := SetupTestEnvironment(t) + defer e.Cleanup(t) + + const liveID = "abcdefghijklmnopqrstuvwx0l" + store := &testConfigStore{cfg: &config.Config{}} + e.api.configStore = store + e.api.configUpdater = &testConfigUpdater{} + e.api.clusterNotifier = &testClusterNotifier{} + + e.mcp.pluginServers = []mcp.PluginServerConfig{{ + ID: liveID, PluginID: testCallerPluginID, Name: "Playbooks MCP", Path: "/mcp", + Enabled: true, ExposeExternal: false, + }} + + e.mockAPI.On("LogError", mock.Anything).Maybe() + e.mockAPI.On("LogError", mock.Anything, mock.Anything, mock.Anything).Maybe() + + body := mcp.PluginServerConfig{ + PluginID: testCallerPluginID, Name: "Playbooks MCP", Path: "/mcp/v2", + Enabled: false, ExposeExternal: true, + } + req := mcpRegisterRequest(t, body) + req.Header.Set("Mattermost-Plugin-ID", testCallerPluginID) + resp := serveAndReturn(e, req) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Len(t, e.mcp.registerCalls, 1) + + saved := e.mcp.registerCalls[0] + require.Equal(t, liveID, saved.ID, "live registry ID must not be rotated") + require.True(t, saved.Enabled, "Enabled preserved from live entry") + require.True(t, saved.ExposeExternal, "ExposeExternal comes from plugin payload") + require.Equal(t, "/mcp/v2", saved.Path) + + require.Len(t, store.cfg.MCP.PluginServers, 1) + require.Equal(t, liveID, store.cfg.MCP.PluginServers[0].ID, "live ID must be persisted") + require.Equal(t, testCallerPluginID, store.cfg.MCP.PluginServers[0].PluginID) +} + func TestAuditMCPRegister(t *testing.T) { gin.SetMode(gin.ReleaseMode) gin.DefaultWriter = io.Discard diff --git a/api/api_config.go b/api/api_config.go index 0c61f4687..93f05cb1e 100644 --- a/api/api_config.go +++ b/api/api_config.go @@ -4,6 +4,7 @@ package api import ( + "errors" "fmt" "net/http" @@ -12,6 +13,8 @@ import ( "github.com/mattermost/mattermost-plugin-agents/v2/config" "github.com/mattermost/mattermost-plugin-agents/v2/llm" "github.com/mattermost/mattermost-plugin-agents/v2/mcp" + "github.com/mattermost/mattermost-plugin-agents/v2/store" + "github.com/mattermost/mattermost/server/public/model" ) func normalizeAdminConfig(cfg config.Config) config.Config { @@ -27,6 +30,31 @@ func normalizeAdminConfig(cfg config.Config) config.Config { return cfg } +// mintEmptyAdminIDs assigns a stable ID to every service and MCP server +// (external, embedded, plugin) that arrived without one. Empty IDs are +// creates; this runs on write only so GET cannot invent unpersisted identities. +func mintEmptyAdminIDs(cfg config.Config) config.Config { + for i := range cfg.Services { + if cfg.Services[i].ID == "" { + cfg.Services[i].ID = model.NewId() + } + } + for i := range cfg.MCP.Servers { + if cfg.MCP.Servers[i].ID == "" { + cfg.MCP.Servers[i].ID = model.NewId() + } + } + if cfg.MCP.EmbeddedServer.ID == "" { + cfg.MCP.EmbeddedServer.ID = model.NewId() + } + for i := range cfg.MCP.PluginServers { + if cfg.MCP.PluginServers[i].ID == "" { + cfg.MCP.PluginServers[i].ID = model.NewId() + } + } + return cfg +} + // handleGetConfig returns the current plugin configuration from the database. // GET /admin/config func (a *API) handleGetConfig(c *gin.Context) { @@ -61,6 +89,8 @@ func (a *API) handleGetConfig(c *gin.Context) { // handleSaveConfig saves a new plugin configuration to the database, // updates the in-memory configuration, and notifies other cluster nodes. +// It responds with the normalized saved config so clients can adopt +// server-minted service/MCP server IDs without a refetch. // PUT /admin/config func (a *API) handleSaveConfig(c *gin.Context) { var cfg config.Config @@ -69,27 +99,46 @@ func (a *API) handleSaveConfig(c *gin.Context) { return } - cfg = normalizeAdminConfig(cfg) - - // Duplicate MCP server names or endpoints are rejected here rather than at - // activation: names key the per-user client map, the shared tools cache, - // and stored OAuth grants, so colliding entries silently shadow each other. + // Names key per-user clients and OAuth grants, while URLs key the shared + // tools cache, so duplicate names or endpoints cannot be persisted. if err := cfg.MCP.Validate(); err != nil { c.AbortWithError(http.StatusBadRequest, fmt.Errorf("invalid MCP configuration: %w", err)) return } - // Audit which top-level config sections change — never their values, - // since services/webSearch/mcp carry credentials. Best effort: a failed - // read of the prior config must not block the save, so the record then - // simply omits changed_keys. - if rec := auditRec(c); rec != nil { - if prev, err := a.configStore.GetConfig(); err == nil { - audit.AddParam(rec, "changed_keys", audit.ChangedJSONKeys(prev, cfg)) + // Read-previous → identity checks → mint empty IDs → save runs atomically + // under the config advisory lock. Duplicate IDs and embedded ID mismatch + // abort with 409. Empty list-item IDs represent creates and receive fresh + // IDs; they never reclaim an existing identity by name or origin. + var changedKeys []string + saved, err := a.configStore.UpdateConfig(func(prev *config.Config) (config.Config, error) { + next := cfg + if err := config.ValidateServiceIDUniqueness(next.Services); err != nil { + return config.Config{}, err } - } - - if err := a.configStore.SaveConfig(cfg); err != nil { + var prevMCP config.MCPConfig + if prev != nil { + prevMCP = prev.MCP + } + reconciledMCP, reconcileErr := config.ReconcileMCPConfigIDs(next.MCP, prevMCP) + if reconcileErr != nil { + return config.Config{}, reconcileErr + } + next.MCP = reconciledMCP + normalized := mintEmptyAdminIDs(normalizeAdminConfig(next)) + changedKeys = audit.ChangedJSONKeys(prev, normalized) + return normalized, nil + }) + switch { + case errors.Is(err, config.ErrServiceIDConflict), errors.Is(err, config.ErrMCPServerIDConflict): + // Duplicate payload IDs, or an embedded server ID that does not match storage. + c.AbortWithError(http.StatusConflict, fmt.Errorf("configuration payload has duplicate service or MCP server IDs, or an embedded server ID that does not match the stored identity: %w", err)) + return + case errors.Is(err, store.ErrLegacyUUIDServiceID): + // After the ABAC ID migration, a dashed UUID is an invalid service ID format. + c.AbortWithError(http.StatusBadRequest, fmt.Errorf("invalid service ID format: %w", err)) + return + case err != nil: c.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to save config: %w", err)) return } @@ -97,10 +146,11 @@ func (a *API) handleSaveConfig(c *gin.Context) { // From here on the config HAS changed in the database. If a later step // fails (cluster notify), the audit record's fail status would otherwise // hide a real mutation — mark it explicitly. + audit.AddParam(auditRec(c), "changed_keys", changedKeys) audit.AddParam(auditRec(c), "persisted", true) // Update in-memory config on this node - a.configUpdater.Update(&cfg) + a.configUpdater.Update(&saved) // Notify other cluster nodes to reload config from DB if err := a.clusterNotifier.PublishConfigUpdate(); err != nil { @@ -108,5 +158,5 @@ func (a *API) handleSaveConfig(c *gin.Context) { return } - c.Status(http.StatusOK) + c.JSON(http.StatusOK, saved) } diff --git a/api/api_config_ids_test.go b/api/api_config_ids_test.go new file mode 100644 index 000000000..95947226b --- /dev/null +++ b/api/api_config_ids_test.go @@ -0,0 +1,594 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package api + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/mattermost/mattermost-plugin-agents/v2/config" + "github.com/mattermost/mattermost-plugin-agents/v2/llm" + "github.com/mattermost/mattermost/server/public/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNormalizeAdminConfigDoesNotMintIDs(t *testing.T) { + result := normalizeAdminConfig(config.Config{ + Services: []llm.ServiceConfig{ + {Name: "no-id", Type: llm.ServiceTypeOpenAI}, + }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "srv-no-id", BaseURL: "https://one.example.com"}, + }, + PluginServers: []config.PluginServerConfig{ + {PluginID: "com.example.a", Name: "A", Path: "/mcp"}, + }, + }, + }) + + assert.Empty(t, result.Services[0].ID) + assert.True(t, result.Services[0].UseResponsesAPI) + assert.Empty(t, result.MCP.Servers[0].ID) + assert.Empty(t, result.MCP.EmbeddedServer.ID) + assert.Empty(t, result.MCP.PluginServers[0].ID) + assert.True(t, result.MCP.Enabled) + assert.True(t, result.MCP.EmbeddedServer.Enabled) +} + +func TestMintEmptyAdminIDsAssignsStableIDs(t *testing.T) { + tests := []struct { + name string + cfg config.Config + validate func(t *testing.T, result config.Config) + }{ + { + name: "empty service and MCP server IDs get fresh valid IDs", + cfg: config.Config{ + Services: []llm.ServiceConfig{ + {Name: "no-id"}, + }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "srv-no-id", BaseURL: "https://one.example.com"}, + }, + PluginServers: []config.PluginServerConfig{ + {PluginID: "com.example.a", Name: "A", Path: "/mcp"}, + }, + }, + }, + validate: func(t *testing.T, result config.Config) { + assert.True(t, model.IsValidId(result.Services[0].ID)) + assert.True(t, model.IsValidId(result.MCP.Servers[0].ID)) + assert.True(t, model.IsValidId(result.MCP.EmbeddedServer.ID)) + assert.True(t, model.IsValidId(result.MCP.PluginServers[0].ID)) + }, + }, + { + name: "non-empty IDs untouched", + cfg: config.Config{ + Services: []llm.ServiceConfig{ + {ID: "svc-existing", Name: "has-id"}, + }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "mcp-existing", Name: "srv-has-id", BaseURL: "https://one.example.com"}, + }, + EmbeddedServer: config.MCPEmbeddedServerConfig{ID: "embedded-existing", Enabled: true}, + PluginServers: []config.PluginServerConfig{ + {ID: "plugin-existing", PluginID: "com.example.a", Name: "A", Path: "/mcp"}, + }, + }, + }, + validate: func(t *testing.T, result config.Config) { + assert.Equal(t, "svc-existing", result.Services[0].ID) + assert.Equal(t, "mcp-existing", result.MCP.Servers[0].ID) + assert.Equal(t, "embedded-existing", result.MCP.EmbeddedServer.ID) + assert.Equal(t, "plugin-existing", result.MCP.PluginServers[0].ID) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.validate(t, mintEmptyAdminIDs(tt.cfg)) + }) + } +} + +// TestHandleSaveConfigMCPServerIDs: incoming uniqueness, then plugin overlay + +// embedded copy, then mint empty IDs. Duplicate incoming IDs return 409. +func TestHandleSaveConfigMCPServerIDs(t *testing.T) { + tests := []struct { + name string + storedCfg *config.Config + payloadCfg config.Config + expectedStatus int + validate func(t *testing.T, store *testConfigStore) + }{ + { + name: "payload without id mints a new remote server ID", + storedCfg: &config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "stable-id", Name: "Jira", BaseURL: "https://jira.example.com"}, + }, + EmbeddedServer: config.MCPEmbeddedServerConfig{ID: "embedded-stable", Enabled: true}, + PluginServers: []config.PluginServerConfig{ + {ID: "plugin-stable", PluginID: "com.example.a", Name: "A", Path: "/mcp"}, + }, + }, + }, + payloadCfg: config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "Jira", BaseURL: "https://jira.example.com"}, + }, + EmbeddedServer: config.MCPEmbeddedServerConfig{Enabled: true}, + PluginServers: []config.PluginServerConfig{ + {PluginID: "com.example.a", Name: "A", Path: "/mcp/v2"}, + }, + }, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.MCP.Servers, 1) + assert.True(t, model.IsValidId(store.cfg.MCP.Servers[0].ID)) + assert.NotEqual(t, "stable-id", store.cfg.MCP.Servers[0].ID) + assert.Equal(t, "embedded-stable", store.cfg.MCP.EmbeddedServer.ID) + require.Len(t, store.cfg.MCP.PluginServers, 1) + assert.Equal(t, "plugin-stable", store.cfg.MCP.PluginServers[0].ID) + }, + }, + { + name: "renamed ID-less server is treated as create", + storedCfg: &config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "stable-id", Name: "Old Name", BaseURL: "https://jira.example.com"}, + }, + }, + }, + payloadCfg: config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "New Name", BaseURL: "https://jira.example.com"}, + }, + }, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.MCP.Servers, 1) + assert.True(t, model.IsValidId(store.cfg.MCP.Servers[0].ID)) + assert.NotEqual(t, "stable-id", store.cfg.MCP.Servers[0].ID) + }, + }, + { + name: "add flow: existing ID kept and ID-less sibling minted", + storedCfg: &config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "stable-id", Name: "Jira", BaseURL: "https://jira.example.com"}, + }, + }, + }, + payloadCfg: config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "stable-id", Name: "Jira", BaseURL: "https://jira.example.com"}, + {Name: "Brand New", BaseURL: "https://new.example.com"}, + }, + }, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.MCP.Servers, 2) + assert.Equal(t, "stable-id", store.cfg.MCP.Servers[0].ID) + assert.True(t, model.IsValidId(store.cfg.MCP.Servers[1].ID)) + assert.NotEqual(t, "stable-id", store.cfg.MCP.Servers[1].ID) + }, + }, + { + name: "duplicate incoming remote IDs return 409", + storedCfg: &config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "stable-id", Name: "Jira", BaseURL: "https://jira.example.com"}, + }, + }, + }, + payloadCfg: config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "stable-id", Name: "Jira", BaseURL: "https://jira.example.com"}, + {ID: "stable-id", Name: "Copy", BaseURL: "https://copy.example.com"}, + }, + }, + }, + expectedStatus: http.StatusConflict, + validate: func(t *testing.T, store *testConfigStore) { + require.NotNil(t, store.cfg) + require.Len(t, store.cfg.MCP.Servers, 1) + assert.Equal(t, "stable-id", store.cfg.MCP.Servers[0].ID, "stored config must be untouched on rejection") + }, + }, + { + name: "no stored config still assigns ids", + storedCfg: nil, + payloadCfg: config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "Jira", BaseURL: "https://jira.example.com"}, + }, + }, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.MCP.Servers, 1) + assert.True(t, model.IsValidId(store.cfg.MCP.Servers[0].ID)) + }, + }, + { + name: "caller-chosen ID for a same-named stored server is kept", + storedCfg: &config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "stable-id", Name: "Jira", BaseURL: "https://jira.example.com"}, + }, + }, + }, + payloadCfg: config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "invented-by-client", Name: "Jira", BaseURL: "https://jira.example.com"}, + }, + }, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.MCP.Servers, 1) + assert.Equal(t, "invented-by-client", store.cfg.MCP.Servers[0].ID) + }, + }, + { + name: "caller-chosen ID on first write is kept", + storedCfg: nil, + payloadCfg: config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "seeded-by-client", Name: "Jira", BaseURL: "https://jira.example.com"}, + }, + }, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.MCP.Servers, 1) + assert.Equal(t, "seeded-by-client", store.cfg.MCP.Servers[0].ID) + }, + }, + { + name: "duplicate IDs on first write return 409", + storedCfg: nil, + payloadCfg: config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "seeded-by-client", Name: "Jira", BaseURL: "https://jira.example.com"}, + {ID: "seeded-by-client", Name: "Copy", BaseURL: "https://copy.example.com"}, + }, + }, + }, + expectedStatus: http.StatusConflict, + validate: func(t *testing.T, store *testConfigStore) { + assert.Nil(t, store.cfg, "nothing must be persisted on rejection") + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := &testConfigStore{cfg: tt.storedCfg} + updater := &testConfigUpdater{} + notifier := &testClusterNotifier{} + router := setupTestRouter(store, updater, notifier) + + body, err := json.Marshal(tt.payloadCfg) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPut, "/admin/config", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + require.Equal(t, tt.expectedStatus, w.Code) + if tt.expectedStatus == http.StatusConflict { + assert.Equal(t, 0, updater.callCount, "in-memory config must not be updated on rejection") + assert.Equal(t, 0, notifier.callCount, "cluster must not be notified on rejection") + } + if tt.validate != nil { + tt.validate(t, store) + } + }) + } +} + +// TestHandleSaveConfigPreservesOmittedPluginServers: full config save never +// accepts client-provided plugin_servers — omitted and stale non-empty lists +// both leave persisted plugin rows (and their IDs) untouched. +func TestHandleSaveConfigPreservesOmittedPluginServers(t *testing.T) { + pluginA := "abcdefghijklmnopqrstuvwx0a" + pluginB := "abcdefghijklmnopqrstuvwx0b" + embeddedID := "abcdefghijklmnopqrstuvwx0e" + remoteID := "abcdefghijklmnopqrstuvwx0r" + + storedPlugins := []config.PluginServerConfig{ + {ID: pluginA, PluginID: "com.example.a", Name: "A", Path: "/mcp", Enabled: true}, + {ID: pluginB, PluginID: "com.example.b", Name: "B", Path: "/other", Enabled: false}, + } + + tests := []struct { + name string + mcp map[string]any + wantIDs []string + }{ + { + name: "omitted plugin_servers preserves prev", + mcp: map[string]any{ + "enabled": true, + "servers": []map[string]any{ + {"id": remoteID, "name": "Remote", "baseURL": "https://mcp.example.com", "enabled": true}, + }, + "embeddedServer": map[string]any{"enabled": true}, + }, + wantIDs: []string{pluginA, pluginB}, + }, + { + name: "stale non-empty plugin_servers ignored", + mcp: map[string]any{ + "enabled": true, + "servers": []map[string]any{ + {"id": remoteID, "name": "Remote", "baseURL": "https://mcp.example.com", "enabled": true}, + }, + "embeddedServer": map[string]any{"enabled": true}, + "plugin_servers": []map[string]any{ + { + "id": "attackeridattackeridattack", "plugin_id": "com.example.a", + "name": "Hacked", "path": "/evil", "enabled": false, + }, + }, + }, + wantIDs: []string{pluginA, pluginB}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := &testConfigStore{cfg: &config.Config{ + MCP: config.MCPConfig{ + Enabled: true, + Servers: []config.MCPServerConfig{ + {ID: remoteID, Name: "Remote", BaseURL: "https://mcp.example.com", Enabled: true}, + }, + EmbeddedServer: config.MCPEmbeddedServerConfig{ID: embeddedID, Enabled: true}, + PluginServers: append([]config.PluginServerConfig(nil), storedPlugins...), + }, + }} + router := setupTestRouter(store, &testConfigUpdater{}, &testClusterNotifier{}) + + body, err := json.Marshal(map[string]any{"mcp": tt.mcp}) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPut, "/admin/config", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + require.Equal(t, http.StatusOK, w.Code) + + require.Len(t, store.cfg.MCP.PluginServers, len(tt.wantIDs)) + for i, id := range tt.wantIDs { + assert.Equal(t, id, store.cfg.MCP.PluginServers[i].ID) + assert.Equal(t, storedPlugins[i].PluginID, store.cfg.MCP.PluginServers[i].PluginID) + assert.Equal(t, storedPlugins[i].Name, store.cfg.MCP.PluginServers[i].Name) + assert.Equal(t, storedPlugins[i].Path, store.cfg.MCP.PluginServers[i].Path) + assert.Equal(t, storedPlugins[i].Enabled, store.cfg.MCP.PluginServers[i].Enabled) + } + assert.Equal(t, remoteID, store.cfg.MCP.Servers[0].ID) + assert.Equal(t, embeddedID, store.cfg.MCP.EmbeddedServer.ID) + }) + } +} + +func TestHandleSaveConfigRejectsCrossKindMCPIDConflict(t *testing.T) { + sharedID := "abcdefghijklmnopqrstuvwxzz" + store := &testConfigStore{cfg: &config.Config{ + MCP: config.MCPConfig{ + EmbeddedServer: config.MCPEmbeddedServerConfig{ID: sharedID, Enabled: true}, + }, + }} + router := setupTestRouter(store, &testConfigUpdater{}, &testClusterNotifier{}) + + payload := config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: sharedID, Name: "Remote", BaseURL: "https://mcp.example.com", Enabled: true}, + }, + EmbeddedServer: config.MCPEmbeddedServerConfig{ID: sharedID, Enabled: true}, + }, + } + body, err := json.Marshal(payload) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPut, "/admin/config", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + require.Equal(t, http.StatusConflict, w.Code) +} + +// TestHandleSaveConfigMintsServiceIDs: empty service ID means create +// (mintEmptyAdminIDs); explicit IDs are kept, including a new ID for +// a same-named stored service. Duplicate incoming IDs return 409. +func TestHandleSaveConfigMintsServiceIDs(t *testing.T) { + stored := func() *config.Config { + return &config.Config{ + Services: []llm.ServiceConfig{ + {ID: "stable-svc-id", Name: "OpenAI", Type: "openai"}, + }, + } + } + + tests := []struct { + name string + storedCfg *config.Config + payloadServices []llm.ServiceConfig + expectedStatus int + validate func(t *testing.T, store *testConfigStore) + }{ + { + name: "payload without id mints a new service ID", + storedCfg: stored(), + payloadServices: []llm.ServiceConfig{ + {Name: "OpenAI", Type: "openai"}, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.Services, 1) + assert.True(t, model.IsValidId(store.cfg.Services[0].ID)) + assert.NotEqual(t, "stable-svc-id", store.cfg.Services[0].ID) + }, + }, + { + name: "ID-less payload with field edits is treated as create", + storedCfg: stored(), + payloadServices: []llm.ServiceConfig{ + {Name: "OpenAI", Type: "anthropic", APIURL: "https://edited.example.com"}, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.Services, 1) + assert.True(t, model.IsValidId(store.cfg.Services[0].ID)) + assert.NotEqual(t, "stable-svc-id", store.cfg.Services[0].ID) + }, + }, + { + name: "add flow: ID-less sibling is minted", + storedCfg: stored(), + payloadServices: []llm.ServiceConfig{ + {ID: "stable-svc-id", Name: "OpenAI", Type: "openai"}, + {Name: "Brand New", Type: "anthropic"}, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.Services, 2) + assert.Equal(t, "stable-svc-id", store.cfg.Services[0].ID) + assert.True(t, model.IsValidId(store.cfg.Services[1].ID)) + }, + }, + { + name: "caller-chosen ID for a same-named service is kept", + storedCfg: stored(), + payloadServices: []llm.ServiceConfig{ + {ID: "invented-by-client", Name: "OpenAI", Type: "openai"}, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.Services, 1) + assert.Equal(t, "invented-by-client", store.cfg.Services[0].ID) + }, + }, + { + name: "duplicate incoming IDs return 409", + storedCfg: stored(), + payloadServices: []llm.ServiceConfig{ + {ID: "stable-svc-id", Name: "OpenAI", Type: "openai"}, + {ID: "stable-svc-id", Name: "Copy", Type: "openai"}, + }, + expectedStatus: http.StatusConflict, + }, + { + name: "no stored config still assigns ids", + storedCfg: nil, + payloadServices: []llm.ServiceConfig{ + {Name: "OpenAI", Type: "openai"}, + }, + expectedStatus: http.StatusOK, + validate: func(t *testing.T, store *testConfigStore) { + require.Len(t, store.cfg.Services, 1) + assert.True(t, model.IsValidId(store.cfg.Services[0].ID)) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := &testConfigStore{cfg: tt.storedCfg} + router := setupTestRouter(store, &testConfigUpdater{}, &testClusterNotifier{}) + + body, err := json.Marshal(config.Config{Services: tt.payloadServices}) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPut, "/admin/config", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + require.Equal(t, tt.expectedStatus, w.Code) + if tt.validate != nil { + tt.validate(t, store) + } + }) + } +} + +// TestHandleSaveConfigRejectsLegacyUUIDServiceIDs covers a save that carries +// dashed UUID service IDs after the ABAC ID migration: the save must fail with +// 400 and leave the migrated config untouched, while a valid payload is accepted. +func TestHandleSaveConfigRejectsLegacyUUIDServiceIDs(t *testing.T) { + migrated := &config.Config{ + Services: []llm.ServiceConfig{ + {ID: "migrated26charidmigrated26", Name: "OpenAI", Type: "openai"}, + }, + } + store := &testConfigStore{cfg: migrated, serviceIDMigrationDone: true} + updater := &testConfigUpdater{} + notifier := &testClusterNotifier{} + router := setupTestRouter(store, updater, notifier) + + put := func(cfg config.Config) *httptest.ResponseRecorder { + body, err := json.Marshal(cfg) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodPut, "/admin/config", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + return w + } + + // Payload with a dashed UUID service ID (invalid post-migration format). + stale := config.Config{ + Services: []llm.ServiceConfig{ + {ID: "550e8400-e29b-41d4-a716-446655440000", Name: "OpenAI", Type: "openai"}, + }, + } + w := put(stale) + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Equal(t, "migrated26charidmigrated26", store.cfg.Services[0].ID, "migrated ID must survive the rejected save") + assert.Equal(t, 0, updater.callCount, "in-memory config must not be updated on rejection") + assert.Equal(t, 0, notifier.callCount, "cluster must not be notified on rejection") + + // Fresh payload with the migrated ID is accepted. + fresh := config.Config{ + Services: []llm.ServiceConfig{ + {ID: "migrated26charidmigrated26", Name: "OpenAI Renamed", Type: "openai"}, + }, + } + w = put(fresh) + assert.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, "OpenAI Renamed", store.cfg.Services[0].Name) + assert.Equal(t, 1, updater.callCount) + assert.Equal(t, 1, notifier.callCount) +} diff --git a/api/api_config_test.go b/api/api_config_test.go index b3dbc1c52..ad8d805fa 100644 --- a/api/api_config_test.go +++ b/api/api_config_test.go @@ -15,6 +15,8 @@ import ( "github.com/mattermost/mattermost-plugin-agents/v2/config" "github.com/mattermost/mattermost-plugin-agents/v2/llm" "github.com/mattermost/mattermost-plugin-agents/v2/mcp" + "github.com/mattermost/mattermost-plugin-agents/v2/store" + "github.com/mattermost/mattermost/server/public/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -24,6 +26,10 @@ type testConfigStore struct { cfg *config.Config getErr error saveErr error + + // serviceIDMigrationDone mirrors the store's migration marker driving the + // post-migration UUID format rejection in UpdateConfig. + serviceIDMigrationDone bool } func (s *testConfigStore) GetConfig() (*config.Config, error) { @@ -42,6 +48,27 @@ func (s *testConfigStore) SaveConfig(cfg config.Config) error { return nil } +func (s *testConfigStore) UpdateConfig(transform func(prev *config.Config) (config.Config, error)) (config.Config, error) { + if s.getErr != nil { + return config.Config{}, s.getErr + } + next, err := transform(s.cfg) + if err != nil { + return config.Config{}, err + } + if s.serviceIDMigrationDone { + for i := range next.Services { + if len(next.Services[i].ID) == 36 { + return next, store.ErrLegacyUUIDServiceID + } + } + } + if err := s.SaveConfig(next); err != nil { + return next, err + } + return next, nil +} + // testConfigUpdater tracks whether Update was called and with what config. type testConfigUpdater struct { lastUpdate *config.Config @@ -129,6 +156,31 @@ func TestHandleGetConfig(t *testing.T) { assert.Empty(t, cfg.DefaultBotName) assert.True(t, cfg.MCP.Enabled) assert.True(t, cfg.MCP.EmbeddedServer.Enabled) + assert.Empty(t, cfg.MCP.EmbeddedServer.ID, "GET must not mint unpersisted IDs") + }, + }, + { + name: "does not mint IDs for ID-less stored rows", + storedConfig: &config.Config{ + Services: []llm.ServiceConfig{ + {Name: "OpenAI", Type: "openai"}, + }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "Jira", BaseURL: "https://jira.example.com"}, + }, + }, + }, + expectedStatus: http.StatusOK, + validateBody: func(t *testing.T, body []byte) { + var cfg config.Config + err := json.Unmarshal(body, &cfg) + require.NoError(t, err) + require.Len(t, cfg.Services, 1) + assert.Empty(t, cfg.Services[0].ID) + require.Len(t, cfg.MCP.Servers, 1) + assert.Empty(t, cfg.MCP.Servers[0].ID) + assert.Empty(t, cfg.MCP.EmbeddedServer.ID) }, }, { @@ -213,6 +265,7 @@ func TestHandleGetConfigDoesNotMutateStoredServices(t *testing.T) { func TestHandleSaveConfig(t *testing.T) { tests := []struct { name string + storedCfg *config.Config requestBody any clusterErr error expectedStatus int @@ -222,6 +275,11 @@ func TestHandleSaveConfig(t *testing.T) { }{ { name: "returns error when cluster notify fails after successful save", + storedCfg: &config.Config{ + Services: []llm.ServiceConfig{ + {ID: "svc-1", Name: "OpenAI", Type: "openai"}, + }, + }, requestBody: config.Config{ DefaultBotName: "ai", Services: []llm.ServiceConfig{ @@ -248,6 +306,11 @@ func TestHandleSaveConfig(t *testing.T) { }, { name: "saves valid config", + storedCfg: &config.Config{ + Services: []llm.ServiceConfig{ + {ID: "svc-1", Name: "OpenAI", Type: "openai"}, + }, + }, requestBody: config.Config{ DefaultBotName: "ai", Services: []llm.ServiceConfig{ @@ -390,7 +453,7 @@ func TestHandleSaveConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - store := &testConfigStore{} + store := &testConfigStore{cfg: tt.storedCfg} updater := &testConfigUpdater{} notifier := &testClusterNotifier{err: tt.clusterErr} @@ -426,6 +489,42 @@ func TestHandleSaveConfig(t *testing.T) { } } +// TestHandleSaveConfigReturnsNormalizedConfig verifies the PUT response body +// carries the normalized saved config, so the webapp can adopt server-minted +// service and MCP server IDs immediately instead of waiting for a reload. +func TestHandleSaveConfigReturnsNormalizedConfig(t *testing.T) { + store := &testConfigStore{} + router := setupTestRouter(store, &testConfigUpdater{}, &testClusterNotifier{}) + + payload := config.Config{ + Services: []llm.ServiceConfig{{Name: "OpenAI", Type: "openai"}}, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{{Name: "Jira", BaseURL: "https://jira.example.com"}}, + }, + } + body, err := json.Marshal(payload) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPut, "/admin/config", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + require.Equal(t, http.StatusOK, w.Code) + + var resp config.Config + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + + require.Len(t, resp.Services, 1) + assert.True(t, model.IsValidId(resp.Services[0].ID), "response must carry the server-minted service ID") + assert.Equal(t, store.cfg.Services[0].ID, resp.Services[0].ID, "response ID must match the persisted one") + + require.Len(t, resp.MCP.Servers, 1) + assert.True(t, model.IsValidId(resp.MCP.Servers[0].ID), "response must carry the server-minted MCP server ID") + assert.Equal(t, store.cfg.MCP.Servers[0].ID, resp.MCP.Servers[0].ID, "response ID must match the persisted one") + + assert.True(t, resp.Services[0].UseResponsesAPI, "response must reflect normalization") +} + func TestSaveAndGetConfigRoundTrip(t *testing.T) { store := &testConfigStore{} updater := &testConfigUpdater{} @@ -443,11 +542,12 @@ func TestSaveAndGetConfigRoundTrip(t *testing.T) { require.NoError(t, err) assert.Empty(t, emptyCfg.Services) - // Step 2: PUT a config + // Step 2: PUT a config. The new service arrives ID-less (the backend + // mints the stable ID). saveCfg := config.Config{ DefaultBotName: "ai", Services: []llm.ServiceConfig{ - {ID: "svc-1", Name: "OpenAI", Type: "openai", APIKey: "sk-test"}, + {Name: "OpenAI", Type: "openai", APIKey: "sk-test"}, }, Bots: []llm.BotConfig{ {ID: "bot-1", Name: "ai", ServiceID: "svc-1"}, diff --git a/api/api_test.go b/api/api_test.go index 694f95023..07f1ee20a 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -151,7 +151,7 @@ type mockMCPClientManager struct { ensureSessionCreated bool registerCalls []mcp.PluginServerConfig - updateCalls []mcp.PluginServerConfig + adminPatchCalls []mcp.PluginServerConfig unregisterCalls []string pluginServers []mcp.PluginServerConfig // orphanPluginIDs simulates entries present in pluginServers but with @@ -250,12 +250,9 @@ func (m *mockMCPClientManager) GetConfig() mcp.Config { func (m *mockMCPClientManager) RegisterPluginServer(cfg mcp.PluginServerConfig) { m.registerCalls = append(m.registerCalls, cfg) - delete(m.orphanPluginIDs, cfg.PluginID) - m.storePluginServer(cfg) -} - -func (m *mockMCPClientManager) UpdatePluginServer(cfg mcp.PluginServerConfig) { - m.updateCalls = append(m.updateCalls, cfg) + if m.orphanPluginIDs != nil { + delete(m.orphanPluginIDs, cfg.PluginID) + } m.storePluginServer(cfg) } @@ -269,6 +266,20 @@ func (m *mockMCPClientManager) storePluginServer(cfg mcp.PluginServerConfig) { m.pluginServers = append(m.pluginServers, cfg) } +func (m *mockMCPClientManager) UpdatePluginServerAdminFields(pluginID string, enabled bool, toolConfigs []mcp.ToolConfig) (mcp.PluginServerConfig, bool) { + // Mirror real ClientManager: patch only admin-owned fields on the live entry. + for i, existing := range m.pluginServers { + if existing.PluginID == pluginID { + existing.Enabled = enabled + existing.ToolConfigs = toolConfigs + m.pluginServers[i] = existing + m.adminPatchCalls = append(m.adminPatchCalls, existing) + return existing, true + } + } + return mcp.PluginServerConfig{}, false +} + func (m *mockMCPClientManager) UnregisterPluginServer(pluginID string) { m.unregisterCalls = append(m.unregisterCalls, pluginID) for i, existing := range m.pluginServers { @@ -280,8 +291,13 @@ func (m *mockMCPClientManager) UnregisterPluginServer(pluginID string) { } func (m *mockMCPClientManager) ListPluginServers() []mcp.PluginServerConfig { - out := make([]mcp.PluginServerConfig, len(m.pluginServers)) - copy(out, m.pluginServers) + out := make([]mcp.PluginServerConfig, 0, len(m.pluginServers)) + for _, cfg := range m.pluginServers { + if m.orphanPluginIDs != nil && m.orphanPluginIDs[cfg.PluginID] { + continue + } + out = append(out, cfg) + } return out } @@ -294,18 +310,6 @@ func (m *mockMCPClientManager) GetPluginServer(pluginID string) (mcp.PluginServe return mcp.PluginServerConfig{}, false } -func (m *mockMCPClientManager) IsPluginRegistered(pluginID string) bool { - if m.orphanPluginIDs[pluginID] { - return false - } - for _, existing := range m.pluginServers { - if existing.PluginID == pluginID { - return true - } - } - return false -} - func (m *mockMCPClientManager) DiscoverPluginServerTools(ctx context.Context, userID string, cfg mcp.PluginServerConfig) ([]mcp.ToolInfo, error) { m.discoverMu.Lock() m.discoverPluginToolsCallCount++ diff --git a/api/audit_middleware_test.go b/api/audit_middleware_test.go index 0037a3863..0933cf563 100644 --- a/api/audit_middleware_test.go +++ b/api/audit_middleware_test.go @@ -129,17 +129,22 @@ func TestAuditMiddlewareSaveConfig(t *testing.T) { }, }, { - name: "prior-config read failure still saves and audits, omitting changed_keys", + // Atomic UpdateConfig must read the prior config to reconcile + // stable service/MCP server IDs. A blind save would risk rotating + // identities and detaching ABAC policies, so fail closed. + name: "prior-config read failure records a 500 fail, omitting changed_keys", userID: "userid", isAdmin: true, body: requestBody, getErr: errors.New("kv read exploded"), - expectedStatus: http.StatusOK, + expectedStatus: http.StatusInternalServerError, validateRecord: func(t *testing.T, rec *model.AuditRecord) { - assert.Equal(t, model.AuditStatusSuccess, rec.Status) + assert.Equal(t, model.AuditStatusFail, rec.Status) + assert.Equal(t, http.StatusInternalServerError, rec.Error.Code) assert.NotContains(t, rec.EventData.Parameters, "changed_keys", - "best-effort diff must be omitted, not fabricated, when the prior config is unreadable") - assert.Equal(t, true, rec.EventData.Parameters["persisted"]) + "diff must be omitted when the prior config is unreadable") + assert.NotContains(t, rec.EventData.Parameters, "persisted", + "a save that never landed must not claim persistence") }, }, { diff --git a/config/legacy_migrations.go b/config/legacy_migrations.go index 5b865c843..114e29626 100644 --- a/config/legacy_migrations.go +++ b/config/legacy_migrations.go @@ -9,6 +9,7 @@ import ( "github.com/google/uuid" "github.com/mattermost/mattermost-plugin-agents/v2/llm" + "github.com/mattermost/mattermost/server/public/model" ) // LegacyServiceConfig represents the old config.json format with the legacy service fields. @@ -75,7 +76,7 @@ func MigrateServicesToBots(cfg Config, loadLegacyConfig func() (LegacyServiceCon existingConfig.Services = make([]llm.ServiceConfig, 0, len(oldConfig.Config.Services)) for _, service := range oldConfig.Config.Services { existingConfig.Services = append(existingConfig.Services, llm.ServiceConfig{ - ID: uuid.New().String(), + ID: model.NewId(), Name: service.Name, Type: service.ServiceName, DefaultModel: service.DefaultModel, @@ -149,7 +150,7 @@ func MigrateSeparateServicesFromBots(cfg Config) (Config, bool, error) { } // Generate service ID - serviceID := uuid.New().String() + serviceID := generateServiceID() // Check if similar service already exists (deduplication) existingID := findIdenticalService(serviceMap, bot.Service) @@ -211,6 +212,12 @@ func RunAllLegacyMigrations(cfg Config, loadLegacyConfig func() (LegacyServiceCo return cfg, changed, nil } +// generateServiceID returns a Mattermost-style 26-char service ID. Service IDs +// are ABAC policy identities; config-bot IDs (below) intentionally stay UUIDs. +func generateServiceID() string { + return model.NewId() +} + // findIdenticalService checks if a service with identical configuration already exists. func findIdenticalService(serviceMap map[string]llm.ServiceConfig, newSvc *llm.ServiceConfig) string { for id, existingSvc := range serviceMap { diff --git a/config/legacy_migrations_test.go b/config/legacy_migrations_test.go index 72ffcccba..1404797f0 100644 --- a/config/legacy_migrations_test.go +++ b/config/legacy_migrations_test.go @@ -9,10 +9,17 @@ import ( "testing" "github.com/mattermost/mattermost-plugin-agents/v2/llm" + "github.com/mattermost/mattermost/server/public/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestGenerateServiceID(t *testing.T) { + id := generateServiceID() + assert.True(t, model.IsValidId(id), "generated service ID %q must be a valid Mattermost ID", id) + assert.NotEqual(t, id, generateServiceID(), "IDs must be unique") +} + func TestMigrateSeparateServicesFromBots(t *testing.T) { tests := []struct { name string @@ -116,6 +123,7 @@ func TestMigrateSeparateServicesFromBots(t *testing.T) { assert.Equal(t, llm.ServiceTypeOpenAI, result.Services[0].Type) assert.Equal(t, "key1", result.Services[0].APIKey) assert.Equal(t, "gpt-4", result.Services[0].DefaultModel) + assert.True(t, model.IsValidId(result.Services[0].ID), "extracted service ID must be a valid Mattermost ID") require.Len(t, result.Bots, 1) assert.Equal(t, result.Services[0].ID, result.Bots[0].ServiceID) @@ -922,7 +930,7 @@ func TestMigrateServicesToBots(t *testing.T) { assert.Equal(t, "org-123", result.Services[0].OrgID) assert.Equal(t, "sk-test-key", result.Services[0].APIKey) assert.Equal(t, 4000, result.Services[0].InputTokenLimit) - assert.NotEmpty(t, result.Services[0].ID) + assert.True(t, model.IsValidId(result.Services[0].ID), "migrated service ID must be a valid Mattermost ID") require.Len(t, result.Bots, 1) assert.Equal(t, "ai1", result.Bots[0].Name) diff --git a/config/mcp_config.go b/config/mcp_config.go index 4805544ea..5c9d1be88 100644 --- a/config/mcp_config.go +++ b/config/mcp_config.go @@ -4,6 +4,8 @@ package config import ( + "errors" + "fmt" "net/textproto" "strings" ) @@ -12,8 +14,18 @@ const ( MCPToolPolicyAsk = "ask" MCPToolPolicyAutoRunInDM = "auto_run_in_dm" MCPToolPolicyAutoRunEverywhere = "auto_run_everywhere" + + // MCPEmbeddedServerOrigin is the runtime ServerOrigin for the embedded + // Mattermost MCP server (matches mcp.EmbeddedClientKey). + MCPEmbeddedServerOrigin = "embedded://mattermost" ) +// PluginServerOrigin returns the runtime ServerOrigin for a plugin-registered +// MCP server. Identity is keyed by PluginID only — Path is not part of the origin. +func PluginServerOrigin(pluginID string) string { + return "plugin://" + pluginID +} + // MCPToolConfig represents per-tool configuration for an MCP server. type MCPToolConfig struct { Name string `json:"name"` @@ -37,6 +49,10 @@ func IsToolPolicyAutoRunEverywhere(policy string) bool { // MCPEmbeddedServerConfig contains configuration for the embedded MCP server type MCPEmbeddedServerConfig struct { + // ID is the immutable plugin-assigned ABAC policy identity. It survives + // enablement/tool-config edits and is never reused. Runtime tool origin + // is MCPEmbeddedServerOrigin. + ID string `json:"id,omitempty"` Enabled bool `json:"enabled"` ToolConfigs []MCPToolConfig `json:"tool_configs,omitempty"` } @@ -53,6 +69,10 @@ type MCPConfig struct { // MCPServerConfig contains the configuration for a single MCP server type MCPServerConfig struct { + // ID is the immutable plugin-assigned ABAC policy identity: it survives + // Name/BaseURL edits and is never reused. OAuth keying (Name) and runtime + // tool origin (BaseURL) are separate identity systems. + ID string `json:"id,omitempty"` Name string `json:"name"` Enabled bool `json:"enabled"` BaseURL string `json:"baseURL"` @@ -153,8 +173,139 @@ func (s *MCPServerConfig) HasServiceAccountAuth() bool { return len(s.EffectiveServiceAccountHeaders()) > 0 } +// ServerIDByOrigin maps a runtime ServerOrigin to the stable server ID. +// Origins are BaseURL (external), MCPEmbeddedServerOrigin (embedded), or +// PluginServerOrigin(PluginID) (plugin). ID-less servers are omitted; on +// duplicate origins the last entry wins (matching filterToolsByConfig); +// disabled servers are included because policy CRUD needs the mapping +// regardless of enablement. +func (c *MCPConfig) ServerIDByOrigin() map[string]string { + out := make(map[string]string, len(c.Servers)+len(c.PluginServers)+1) + for i := range c.Servers { + if c.Servers[i].ID == "" { + continue + } + out[c.Servers[i].BaseURL] = c.Servers[i].ID + } + if c.EmbeddedServer.ID != "" { + out[MCPEmbeddedServerOrigin] = c.EmbeddedServer.ID + } + for i := range c.PluginServers { + if c.PluginServers[i].ID == "" || c.PluginServers[i].PluginID == "" { + continue + } + out[PluginServerOrigin(c.PluginServers[i].PluginID)] = c.PluginServers[i].ID + } + return out +} + +// OriginByServerID is the inverse of ServerIDByOrigin: stable ID -> current origin. +func (c *MCPConfig) OriginByServerID() map[string]string { + out := make(map[string]string, len(c.Servers)+len(c.PluginServers)+1) + for i := range c.Servers { + if c.Servers[i].ID == "" { + continue + } + out[c.Servers[i].ID] = c.Servers[i].BaseURL + } + if c.EmbeddedServer.ID != "" { + out[c.EmbeddedServer.ID] = MCPEmbeddedServerOrigin + } + for i := range c.PluginServers { + if c.PluginServers[i].ID == "" || c.PluginServers[i].PluginID == "" { + continue + } + out[c.PluginServers[i].ID] = PluginServerOrigin(c.PluginServers[i].PluginID) + } + return out +} + +// ErrMCPServerIDConflict is returned when the config being written contains +// duplicate non-empty MCP server IDs, or when an incoming embedded server ID +// differs from the stored ID. +var ErrMCPServerIDConflict = errors.New("MCP server identity conflict") + +// ReconcileMCPConfigIDs is the single entry point for MCP identity on config +// save. It: +// +// 1. Treats PluginServers as server-owned: prev is carried forward +// unconditionally. Full config save never accepts client-provided plugin +// rows (bridge register/unregister and PUT /admin/mcp/plugin-servers/:id +// are the only writers). +// 2. Copies the embedded server ID from prev when next is empty. A +// non-empty next ID that differs from a non-empty prev ID is a conflict. +// Empty remote IDs stay empty for the caller to mint. +// 3. Rejects any ID shared by two MCP resources of any kind on the config +// being written. Duplicate IDs in prev alone are not a conflict. +func ReconcileMCPConfigIDs(next, prev MCPConfig) (MCPConfig, error) { + next.PluginServers = append([]PluginServerConfig(nil), prev.PluginServers...) + + embedded, err := ReconcileEmbeddedMCPServerID(next.EmbeddedServer, prev.EmbeddedServer) + if err != nil { + return MCPConfig{}, err + } + next.EmbeddedServer = embedded + + if err := ValidateMCPServerIDUniqueness(next); err != nil { + return MCPConfig{}, err + } + return next, nil +} + +// ValidateMCPServerIDUniqueness returns ErrMCPServerIDConflict when any two +// MCP resources (remote, embedded, or plugin) share the same non-empty ID. +func ValidateMCPServerIDUniqueness(cfg MCPConfig) error { + seen := make(map[string]string, len(cfg.Servers)+len(cfg.PluginServers)+1) + claim := func(id, label string) error { + if id == "" { + return nil + } + if other, ok := seen[id]; ok { + return fmt.Errorf("%w: %s and %s share ID %q", ErrMCPServerIDConflict, other, label, id) + } + seen[id] = label + return nil + } + for i := range cfg.Servers { + label := fmt.Sprintf("remote server %q", cfg.Servers[i].Name) + if err := claim(cfg.Servers[i].ID, label); err != nil { + return err + } + } + if err := claim(cfg.EmbeddedServer.ID, "embedded server"); err != nil { + return err + } + for i := range cfg.PluginServers { + label := fmt.Sprintf("plugin server %q", cfg.PluginServers[i].PluginID) + if err := claim(cfg.PluginServers[i].ID, label); err != nil { + return err + } + } + return nil +} + +// ReconcileEmbeddedMCPServerID copies prev.ID onto an ID-less next (one +// embedded server, not a list heuristic). A non-empty next ID that differs +// from a non-empty prev ID is a conflict. Caller-chosen IDs on a previously +// ID-less embedded server are kept; both empty leaves next ID-less for the +// caller to mint. +func ReconcileEmbeddedMCPServerID(next, prev MCPEmbeddedServerConfig) (MCPEmbeddedServerConfig, error) { + if next.ID == "" { + next.ID = prev.ID + return next, nil + } + if prev.ID != "" && next.ID != prev.ID { + return MCPEmbeddedServerConfig{}, fmt.Errorf("%w: embedded server carries ID %q that differs from the stored ID %q", ErrMCPServerIDConflict, next.ID, prev.ID) + } + return next, nil +} + // PluginServerConfig describes an MCP server registered by another plugin. type PluginServerConfig struct { + // ID is the immutable plugin-assigned ABAC policy identity. Identity is + // keyed by PluginID: it survives re-registration and Path changes and is + // never reused. Runtime tool origin is PluginServerOrigin(PluginID). + ID string `json:"id,omitempty"` PluginID string `json:"plugin_id"` Name string `json:"name"` Path string `json:"path"` diff --git a/config/mcp_config_test.go b/config/mcp_config_test.go index a14ce3966..ba7127e1c 100644 --- a/config/mcp_config_test.go +++ b/config/mcp_config_test.go @@ -76,12 +76,9 @@ func TestMCPServerConfigServiceAccountHeaderFiltering(t *testing.T) { " X-Api-Key ": " secret-token\n", }, }, - // Untrimmed names/values make Go's HTTP transport reject the request. wantHeaders: map[string]string{"Authorization": "Bearer pat", "X-Api-Key": "secret-token"}, }, { - // Fail closed: an ambiguous pair has no deterministic winner, so the - // server ends up with no service account auth at all. name: "names colliding after trim are all dropped", serverConfig: &MCPServerConfig{ Enabled: true, @@ -97,8 +94,6 @@ func TestMCPServerConfigServiceAccountHeaderFiltering(t *testing.T) { wantHeaders: map[string]string{"X-Api-Key": "secret-token"}, }, { - // http.Header.Set canonicalizes names, so a case-only pair is the same - // header on the wire and just as ambiguous as a whitespace-only pair. name: "names colliding only by case are all dropped", serverConfig: &MCPServerConfig{ Enabled: true, @@ -138,7 +133,6 @@ func TestMCPServerConfigServiceAccountHeaderFiltering(t *testing.T) { } } -// A broken JSON tag would make Config.Clone silently drop SA credentials cluster-wide. func TestConfigClonePreservesServiceAccountHeaders(t *testing.T) { original := &Config{ MCP: MCPConfig{ @@ -154,7 +148,6 @@ func TestConfigClonePreservesServiceAccountHeaders(t *testing.T) { }, } - // No Config-wide equality: the JSON deep copy normalizes nil json.RawMessage fields to "null". clone := original.Clone() require.Equal(t, original.MCP.Servers, clone.MCP.Servers) @@ -162,6 +155,327 @@ func TestConfigClonePreservesServiceAccountHeaders(t *testing.T) { require.Equal(t, "Bearer service-pat", original.MCP.Servers[0].ServiceAccountHeaders["Authorization"]) } +func TestMCPConfigServerIDMaps(t *testing.T) { + tests := []struct { + name string + cfg MCPConfig + expectByOrigin map[string]string + expectByServerID map[string]string + }{ + { + name: "empty servers", + cfg: MCPConfig{}, + expectByOrigin: map[string]string{}, + expectByServerID: map[string]string{}, + }, + { + name: "servers without IDs omitted", + cfg: MCPConfig{ + Servers: []MCPServerConfig{ + {ID: "id-one", Name: "one", BaseURL: "https://one.example.com"}, + {Name: "no-id", BaseURL: "https://two.example.com"}, + }, + }, + expectByOrigin: map[string]string{"https://one.example.com": "id-one"}, + expectByServerID: map[string]string{"id-one": "https://one.example.com"}, + }, + { + name: "duplicate BaseURL last entry wins", + cfg: MCPConfig{ + Servers: []MCPServerConfig{ + {ID: "id-first", Name: "first", BaseURL: "https://dup.example.com"}, + {ID: "id-last", Name: "last", BaseURL: "https://dup.example.com"}, + }, + }, + expectByOrigin: map[string]string{"https://dup.example.com": "id-last"}, + expectByServerID: map[string]string{ + "id-first": "https://dup.example.com", + "id-last": "https://dup.example.com", + }, + }, + { + name: "disabled server included", + cfg: MCPConfig{ + Servers: []MCPServerConfig{ + {ID: "id-disabled", Name: "off", Enabled: false, BaseURL: "https://off.example.com"}, + }, + }, + expectByOrigin: map[string]string{"https://off.example.com": "id-disabled"}, + expectByServerID: map[string]string{"id-disabled": "https://off.example.com"}, + }, + { + name: "embedded and plugin origins included", + cfg: MCPConfig{ + Servers: []MCPServerConfig{ + {ID: "id-remote", Name: "remote", BaseURL: "https://remote.example.com"}, + }, + EmbeddedServer: MCPEmbeddedServerConfig{ID: "id-embedded", Enabled: true}, + PluginServers: []PluginServerConfig{ + {ID: "id-plugin", PluginID: "com.example.mcp", Name: "Plugin", Path: "/mcp"}, + {PluginID: "com.example.noid", Name: "No ID", Path: "/mcp"}, + }, + }, + expectByOrigin: map[string]string{ + "https://remote.example.com": "id-remote", + MCPEmbeddedServerOrigin: "id-embedded", + PluginServerOrigin("com.example.mcp"): "id-plugin", + }, + expectByServerID: map[string]string{ + "id-remote": "https://remote.example.com", + "id-embedded": MCPEmbeddedServerOrigin, + "id-plugin": PluginServerOrigin("com.example.mcp"), + }, + }, + { + name: "ID-less embedded omitted", + cfg: MCPConfig{ + EmbeddedServer: MCPEmbeddedServerConfig{Enabled: true}, + }, + expectByOrigin: map[string]string{}, + expectByServerID: map[string]string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.expectByOrigin, tt.cfg.ServerIDByOrigin()) + require.Equal(t, tt.expectByServerID, tt.cfg.OriginByServerID()) + }) + } +} + +func TestReconcileEmbeddedMCPServerID(t *testing.T) { + tests := []struct { + name string + next MCPEmbeddedServerConfig + prev MCPEmbeddedServerConfig + expectID string + expectErr bool + }{ + { + name: "empty incoming keeps prev ID", + next: MCPEmbeddedServerConfig{Enabled: true}, + prev: MCPEmbeddedServerConfig{ID: "embedded-prev", Enabled: true}, + expectID: "embedded-prev", + }, + { + name: "matching IDs kept", + next: MCPEmbeddedServerConfig{ID: "embedded-prev", Enabled: false}, + prev: MCPEmbeddedServerConfig{ID: "embedded-prev", Enabled: true}, + expectID: "embedded-prev", + }, + { + name: "conflicting IDs rejected", + next: MCPEmbeddedServerConfig{ID: "incoming-id", Enabled: true}, + prev: MCPEmbeddedServerConfig{ID: "embedded-prev", Enabled: true}, + expectErr: true, + }, + { + name: "caller-chosen ID on previously ID-less kept", + next: MCPEmbeddedServerConfig{ID: "seeded-id", Enabled: true}, + prev: MCPEmbeddedServerConfig{Enabled: true}, + expectID: "seeded-id", + }, + { + name: "both empty stays ID-less", + next: MCPEmbeddedServerConfig{Enabled: true}, + prev: MCPEmbeddedServerConfig{}, + expectID: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := ReconcileEmbeddedMCPServerID(tt.next, tt.prev) + if tt.expectErr { + require.ErrorIs(t, err, ErrMCPServerIDConflict) + return + } + require.NoError(t, err) + require.Equal(t, tt.expectID, result.ID) + }) + } +} + +func TestReconcileMCPConfigIDs(t *testing.T) { + const sharedID = "sharedidsharedidsharedidsha" + + prevPlugins := []PluginServerConfig{ + {ID: "plugin-a", PluginID: "com.example.a", Name: "A", Path: "/mcp", Enabled: true}, + {ID: "plugin-b", PluginID: "com.example.b", Name: "B", Path: "/other", Enabled: false}, + } + + tests := []struct { + name string + next MCPConfig + prev MCPConfig + expectErr bool + validate func(t *testing.T, got MCPConfig) + }{ + { + name: "happy path keeps explicit remote ID, copies embedded/plugin from prev", + next: MCPConfig{ + Servers: []MCPServerConfig{{ID: "remote-id", Name: "Jira", BaseURL: "https://jira.example.com"}}, + EmbeddedServer: MCPEmbeddedServerConfig{Enabled: true}, + PluginServers: []PluginServerConfig{{PluginID: "com.example.stale", Name: "Stale", Path: "/stale"}}, + }, + prev: MCPConfig{ + Servers: []MCPServerConfig{{ID: "remote-id", Name: "Jira", BaseURL: "https://jira.example.com"}}, + EmbeddedServer: MCPEmbeddedServerConfig{ID: "embedded-id", Enabled: true}, + PluginServers: []PluginServerConfig{{ID: "plugin-id", PluginID: "com.example.a", Name: "A", Path: "/mcp"}}, + }, + validate: func(t *testing.T, got MCPConfig) { + require.Equal(t, "remote-id", got.Servers[0].ID) + require.Equal(t, "embedded-id", got.EmbeddedServer.ID) + require.Len(t, got.PluginServers, 1) + require.Equal(t, "plugin-id", got.PluginServers[0].ID) + require.Equal(t, "com.example.a", got.PluginServers[0].PluginID) + }, + }, + { + name: "nil PluginServers carries prev wholesale", + next: MCPConfig{ + Servers: []MCPServerConfig{{ID: "remote-id", Name: "Jira", BaseURL: "https://jira.example.com"}}, + EmbeddedServer: MCPEmbeddedServerConfig{ID: "embedded-id", Enabled: true}, + PluginServers: nil, + }, + prev: MCPConfig{PluginServers: prevPlugins}, + validate: func(t *testing.T, got MCPConfig) { + require.Equal(t, prevPlugins, got.PluginServers) + }, + }, + { + name: "empty PluginServers carries prev wholesale", + next: MCPConfig{ + PluginServers: []PluginServerConfig{}, + }, + prev: MCPConfig{ + PluginServers: []PluginServerConfig{ + {ID: "plugin-a", PluginID: "com.example.a", Name: "A", Path: "/mcp"}, + }, + }, + validate: func(t *testing.T, got MCPConfig) { + require.Len(t, got.PluginServers, 1) + require.Equal(t, "plugin-a", got.PluginServers[0].ID) + }, + }, + { + name: "non-empty stale PluginServers in next ignored", + next: MCPConfig{ + PluginServers: []PluginServerConfig{ + {ID: "attacker-id", PluginID: "com.example.a", Name: "Hacked", Path: "/evil", Enabled: false}, + }, + }, + prev: MCPConfig{PluginServers: prevPlugins}, + validate: func(t *testing.T, got MCPConfig) { + require.Equal(t, prevPlugins, got.PluginServers) + }, + }, + { + name: "remote and embedded share ID", + next: MCPConfig{ + Servers: []MCPServerConfig{{ID: sharedID, Name: "Remote", BaseURL: "https://r.example.com"}}, + EmbeddedServer: MCPEmbeddedServerConfig{ID: sharedID, Enabled: true}, + }, + expectErr: true, + }, + { + name: "remote and carried-forward plugin share ID", + next: MCPConfig{ + Servers: []MCPServerConfig{{ID: sharedID, Name: "Remote", BaseURL: "https://r.example.com"}}, + }, + prev: MCPConfig{ + PluginServers: []PluginServerConfig{{ID: sharedID, PluginID: "com.example.a", Name: "A", Path: "/mcp"}}, + }, + expectErr: true, + }, + { + name: "embedded and carried-forward plugin share ID", + next: MCPConfig{ + EmbeddedServer: MCPEmbeddedServerConfig{ID: sharedID, Enabled: true}, + }, + prev: MCPConfig{ + PluginServers: []PluginServerConfig{{ID: sharedID, PluginID: "com.example.a", Name: "A", Path: "/mcp"}}, + }, + expectErr: true, + }, + { + name: "stale next plugin ID cannot free prev plugin ID for remote reuse", + next: MCPConfig{ + Servers: []MCPServerConfig{{ID: sharedID, Name: "Remote", BaseURL: "https://r.example.com"}}, + PluginServers: []PluginServerConfig{ + {ID: "other-id", PluginID: "com.example.a", Name: "A", Path: "/mcp"}, + }, + }, + prev: MCPConfig{ + PluginServers: []PluginServerConfig{{ID: sharedID, PluginID: "com.example.a", Name: "A", Path: "/mcp"}}, + }, + expectErr: true, + }, + { + name: "duplicate incoming remote IDs rejected", + next: MCPConfig{ + Servers: []MCPServerConfig{ + {ID: sharedID, Name: "srv", BaseURL: "https://one.example.com"}, + {ID: sharedID, Name: "copy", BaseURL: "https://two.example.com"}, + }, + }, + expectErr: true, + }, + { + name: "unique next succeeds even when prev has duplicate stored remote IDs", + next: MCPConfig{ + Servers: []MCPServerConfig{ + {ID: "unique-id", Name: "srv", BaseURL: "https://one.example.com"}, + }, + }, + prev: MCPConfig{ + Servers: []MCPServerConfig{ + {ID: sharedID, Name: "srv", BaseURL: "https://one.example.com"}, + {ID: sharedID, Name: "copy", BaseURL: "https://two.example.com"}, + }, + }, + validate: func(t *testing.T, got MCPConfig) { + require.Len(t, got.Servers, 1) + require.Equal(t, "unique-id", got.Servers[0].ID) + }, + }, + { + name: "ID-less remotes stay empty for the caller to mint", + next: MCPConfig{ + Servers: []MCPServerConfig{ + {Name: "srv", BaseURL: "https://one.example.com"}, + {Name: "brand-new", BaseURL: "https://new.example.com"}, + }, + }, + prev: MCPConfig{ + Servers: []MCPServerConfig{ + {ID: "prev-id", Name: "srv", BaseURL: "https://one.example.com"}, + }, + }, + validate: func(t *testing.T, got MCPConfig) { + require.Len(t, got.Servers, 2) + require.Empty(t, got.Servers[0].ID) + require.Empty(t, got.Servers[1].ID) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ReconcileMCPConfigIDs(tt.next, tt.prev) + if tt.expectErr { + require.ErrorIs(t, err, ErrMCPServerIDConflict) + return + } + require.NoError(t, err) + if tt.validate != nil { + tt.validate(t, got) + } + }) + } +} + func TestServerConfigGetToolPolicyIgnoresRetrievalOverride(t *testing.T) { serverConfig := &MCPServerConfig{ Enabled: true, diff --git a/config/service_ids.go b/config/service_ids.go new file mode 100644 index 000000000..7d0e8fee3 --- /dev/null +++ b/config/service_ids.go @@ -0,0 +1,32 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package config + +import ( + "errors" + "fmt" + + "github.com/mattermost/mattermost-plugin-agents/v2/llm" +) + +// ErrServiceIDConflict is returned when two non-empty service IDs in the +// incoming payload collide. +var ErrServiceIDConflict = errors.New("LLM service identity conflict") + +// ValidateServiceIDUniqueness reports ErrServiceIDConflict when two non-empty +// IDs in services collide. Empty IDs are ignored so the caller can mint. +func ValidateServiceIDUniqueness(services []llm.ServiceConfig) error { + seen := make(map[string]struct{}, len(services)) + for i := range services { + id := services[i].ID + if id == "" { + continue + } + if _, dup := seen[id]; dup { + return fmt.Errorf("%w: service %q duplicates the ID of another entry in the payload", ErrServiceIDConflict, services[i].Name) + } + seen[id] = struct{}{} + } + return nil +} diff --git a/config/service_ids_test.go b/config/service_ids_test.go new file mode 100644 index 000000000..e2ad522d7 --- /dev/null +++ b/config/service_ids_test.go @@ -0,0 +1,54 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost-plugin-agents/v2/llm" +) + +func TestValidateServiceIDUniqueness(t *testing.T) { + tests := []struct { + name string + services []llm.ServiceConfig + expectErr bool + }{ + { + name: "unique IDs", + services: []llm.ServiceConfig{ + {ID: "a", Name: "svc", Type: "openai"}, + {ID: "b", Name: "other", Type: "anthropic"}, + }, + }, + { + name: "empty IDs ignored", + services: []llm.ServiceConfig{ + {Name: "svc", Type: "openai"}, + {Name: "other", Type: "anthropic"}, + }, + }, + { + name: "duplicate non-empty IDs error", + services: []llm.ServiceConfig{ + {ID: "dup", Name: "svc", Type: "openai"}, + {ID: "dup", Name: "copy", Type: "openai"}, + }, + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateServiceIDUniqueness(tt.services) + if tt.expectErr { + require.ErrorIs(t, err, ErrServiceIDConflict) + return + } + require.NoError(t, err) + }) + } +} diff --git a/docs/admin_guide.md b/docs/admin_guide.md index 7d64478bc..d2c0e5c49 100644 --- a/docs/admin_guide.md +++ b/docs/admin_guide.md @@ -547,6 +547,8 @@ This separation allows multiple agents to share the same LLM service configurati } ``` +Service IDs are Mattermost-style 26-character IDs. Configurations created before this format was adopted used UUIDs; those are rewritten once on upgrade, so external automation that hard-coded UUID service IDs must re-read `GET /plugins/mattermost-ai/admin/config` to pick up the new IDs. + **Supported service types:** `openai`, `anthropic`, `azure`, `openaicompatible`, `asage`, `cohere`, `mistral`, `scale` **Legacy format:** Older configurations that stored bots in `config.bots`, or embedded service objects within bots, are migrated on plugin startup. After legacy bot migration completes, stored `config.bots` entries are removed to avoid duplicate bot registration. diff --git a/mcp/client.go b/mcp/client.go index 4eac54712..75a35df1f 100644 --- a/mcp/client.go +++ b/mcp/client.go @@ -26,7 +26,7 @@ import ( const ( MMUserIDHeader = "X-Mattermost-UserID" EmbeddedServerName = "Mattermost" - EmbeddedClientKey = "embedded://mattermost" + EmbeddedClientKey = config.MCPEmbeddedServerOrigin listToolsMethod = "tools/list" diff --git a/mcp/client_manager.go b/mcp/client_manager.go index e027eea97..129014d35 100644 --- a/mcp/client_manager.go +++ b/mcp/client_manager.go @@ -14,6 +14,7 @@ import ( "sync" "time" + "github.com/mattermost/mattermost-plugin-agents/v2/config" "github.com/mattermost/mattermost-plugin-agents/v2/llm" "github.com/mattermost/mattermost-plugin-agents/v2/mmapi" "github.com/mattermost/mattermost/server/public/model" @@ -25,6 +26,11 @@ const pluginRegistrationsKVKey = "mcp_plugin_registrations_v1" var ErrOAuthNotConfigured = errors.New("oauth not configured") +// ServerAccessChecker gates per-user visibility of MCP servers by stable ID. +type ServerAccessChecker interface { + CanUseMCPServer(ctx context.Context, userID, serverID string) error +} + // clientKind is the structural role of a pooled bag. Remote bags cannot // share sessions across user and service-account authentication modes. type clientKind int @@ -80,13 +86,20 @@ type ClientManager struct { // admission caps overlapping remote/plugin connection sequences on this // manager instance. It outlives ReInit and is closed only by Close. admission *connectionAdmission + // accessChecker filters servers for the invoking user (nil = no filtering). + accessChecker ServerAccessChecker // closed is set by Close and makes ReInit a no-op so shutdown stays permanent. closed bool } // NewClientManager creates a new MCP client manager. embeddedServer may be nil. // sourcePluginAPI routes PluginHTTP to source plugins; may be nil. -func NewClientManager(config Config, log pluginapi.LogService, pluginAPI *pluginapi.Client, oauthManager *OAuthManager, embeddedServer EmbeddedMCPServer, httpClient *http.Client, sourcePluginAPI mmapi.Client) *ClientManager { +// accessChecker filters external servers per user; nil disables filtering. +func NewClientManager(config Config, log pluginapi.LogService, pluginAPI *pluginapi.Client, oauthManager *OAuthManager, embeddedServer EmbeddedMCPServer, httpClient *http.Client, sourcePluginAPI mmapi.Client, accessCheckers ...ServerAccessChecker) *ClientManager { + var accessChecker ServerAccessChecker + if len(accessCheckers) > 0 { + accessChecker = accessCheckers[0] + } manager := &ClientManager{ log: log, pluginAPI: pluginAPI, @@ -97,6 +110,7 @@ func NewClientManager(config Config, log pluginapi.LogService, pluginAPI *plugin pluginRegistered: make(map[string]bool), sourcePluginAPI: sourcePluginAPI, admission: newConnectionAdmission(maxNodeConnections), + accessChecker: accessChecker, } manager.hydratePluginRegistrations() // PluginMCPHandlers is constructed later and builds the external aggregate @@ -287,7 +301,7 @@ func (m *ClientManager) snapshotRuntime() (Config, *EmbeddedServerClient) { return m.config, m.embeddedClient } -func (m *ClientManager) resolveEligibleServers(cfg Config, embeddedClient *EmbeddedServerClient, plugins []PluginServerConfig, selection ToolSelection, serviceAccount bool) eligibleServers { +func (m *ClientManager) resolveEligibleServers(cfg Config, embeddedClient *EmbeddedServerClient, plugins []PluginServerConfig, selection ToolSelection, deniedOrigins map[string]bool, serviceAccount bool) eligibleServers { resolved := eligibleServers{origins: make(map[string]bool)} // A duplicated name or endpoint makes every member of the group ambiguous: @@ -311,6 +325,8 @@ func (m *ClientManager) resolveEligibleServers(cfg Config, embeddedClient *Embed m.log.Debug("Skipping MCP server without service account headers in service account mode", "serverID", server.Name, "serverOrigin", server.BaseURL) continue + case deniedOrigins[llm.NormalizeMCPServerOrigin(server.BaseURL)]: + continue case !selection.Allows(server.BaseURL): continue } @@ -318,14 +334,14 @@ func (m *ClientManager) resolveEligibleServers(cfg Config, embeddedClient *Embed resolved.origins[llm.NormalizeMCPServerOrigin(server.BaseURL)] = true } - if embeddedClient != nil && cfg.EmbeddedServer.Enabled && selection.Allows(EmbeddedClientKey) { + if embeddedClient != nil && cfg.EmbeddedServer.Enabled && !deniedOrigins[EmbeddedClientKey] && selection.Allows(EmbeddedClientKey) { resolved.embedded = true resolved.origins[EmbeddedClientKey] = true } for _, pluginCfg := range plugins { origin := pluginServerOriginKey(pluginCfg.PluginID) - if !selection.Allows(origin) { + if deniedOrigins[origin] || !selection.Allows(origin) { continue } resolved.plugins = append(resolved.plugins, pluginCfg) @@ -401,7 +417,10 @@ func (m *ClientManager) getTools(ctx context.Context, req CatalogRequest, select cfg := m.config embeddedClient := m.embeddedClient plugins := m.snapshotEnabledPluginServers() - servers := m.resolveEligibleServers(cfg, embeddedClient, plugins, selection, req.ServiceAccount) + // Service-account remotes are pooled by the bot identity, but authorization + // and local MCP connections belong to the human invoking this request. + deniedOrigins := m.deniedMCPServerOrigins(ctx, req.InvokingUserID, cfg, embeddedClient, plugins) + servers := m.resolveEligibleServers(cfg, embeddedClient, plugins, selection, deniedOrigins, req.ServiceAccount) var localClients *UserClients if servers.embedded || len(servers.plugins) > 0 { @@ -459,6 +478,43 @@ func (m *ClientManager) getTools(ctx context.Context, req CatalogRequest, select return retainToolsFromOrigins(filtered, servers.origins), joinMCPErrors(remoteErrors, localErrors) } +// deniedMCPServerOrigins evaluates each configured stable identity once before +// connection planning. ID-less resources remain available until migration +// assigns their durable IDs. +func (m *ClientManager) deniedMCPServerOrigins(ctx context.Context, userID string, cfg Config, embeddedClient *EmbeddedServerClient, plugins []PluginServerConfig) map[string]bool { + if m.accessChecker == nil { + return nil + } + + var denied map[string]bool + check := func(origin, serverID string) { + if serverID == "" { + return + } + if err := m.accessChecker.CanUseMCPServer(ctx, userID, serverID); err == nil { + return + } + if denied == nil { + denied = make(map[string]bool) + } + denied[llm.NormalizeMCPServerOrigin(origin)] = true + m.log.Debug("Omitting MCP server for user by access policy", "userID", userID, "serverID", serverID) + } + + for _, server := range cfg.Servers { + if server.Enabled && server.BaseURL != "" { + check(server.BaseURL, server.ID) + } + } + if embeddedClient != nil { + check(EmbeddedClientKey, cfg.EmbeddedServer.ID) + } + for _, server := range plugins { + check(pluginServerOriginKey(server.PluginID), server.ID) + } + return denied +} + func joinMCPErrors(groups ...*Errors) *Errors { var joined *Errors for _, group := range groups { @@ -577,6 +633,14 @@ func (m *ClientManager) GetToolRetrievalOverrides() map[string]ToolRetrievalOver addOverride(pluginServerOriginKey(server.PluginID), toolConfig) } } + for _, server := range m.ListPluginServers() { + if !m.IsPluginRegistered(server.PluginID) || !server.Enabled { + continue + } + for _, toolConfig := range server.ToolConfigs { + addOverride(pluginServerOriginKey(server.PluginID), toolConfig) + } + } return overrides } @@ -742,6 +806,13 @@ func (m *ClientManager) pluginIdentityLocked(pluginID string) originIdentity { return pluginOriginIdentity(m.pluginServers[pluginID]) } +// MCPServerIDByOrigin snapshots the stable-ID mapping for the manager's current +// config. See config.MCPConfig.ServerIDByOrigin. +func (m *ClientManager) MCPServerIDByOrigin() map[string]string { + cfg, _ := m.snapshotRuntime() + return cfg.ServerIDByOrigin() +} + // RegisterPluginServer stores or overwrites a plugin-server registration. // Callers must ensure cfg.PluginID is non-empty. Identity-affecting changes // (name, path, enabled, registration) invalidate that origin immediately; @@ -756,7 +827,28 @@ func (m *ClientManager) RegisterPluginServer(cfg PluginServerConfig) { }) } -// UpdatePluginServer applies admin-owned fields without changing registration state. +// UpdatePluginServerAdminFields applies admin-owned fields without replacing +// plugin-owned registration identity. It reports false for a non-live entry. +func (m *ClientManager) UpdatePluginServerAdminFields(pluginID string, enabled bool, toolConfigs []ToolConfig) (PluginServerConfig, bool) { + var ( + updated PluginServerConfig + found bool + ) + m.updatePluginRegistry(pluginID, func() { + updated, found = m.pluginServers[pluginID] + if !found || !m.pluginRegistered[pluginID] { + found = false + return + } + updated.Enabled = enabled + updated.ToolConfigs = toolConfigs + m.pluginServers[pluginID] = updated + }) + return updated, found +} + +// UpdatePluginServer replaces a live registration and preserves the +// identity-change invalidation used by existing registry callers. func (m *ClientManager) UpdatePluginServer(cfg PluginServerConfig) { m.updatePluginRegistry(cfg.PluginID, func() { m.pluginServers[cfg.PluginID] = cfg @@ -824,14 +916,17 @@ func (m *ClientManager) snapshotUserClients() []*UserClients { return users } -// ListPluginServers returns a stable snapshot without holding the registry lock -// during caller work. +// ListPluginServers returns a stable snapshot of live-registered plugin MCP +// servers without holding the registry lock during caller work. func (m *ClientManager) ListPluginServers() []PluginServerConfig { m.pluginServersMu.RLock() defer m.pluginServersMu.RUnlock() out := make([]PluginServerConfig, 0, len(m.pluginServers)) for _, cfg := range m.pluginServers { + if !m.pluginRegistered[cfg.PluginID] { + continue + } out = append(out, cfg) } sort.Slice(out, func(i, j int) bool { @@ -937,32 +1032,36 @@ func (m *ClientManager) loadPersistedPluginRegistrationsLocked() (map[string]Plu return registrations, true } +// ApplyPersistedPluginServerFields overlays admin-owned persisted fields +// (Enabled, ToolConfigs, ID) onto a live registration. Name/Path/ExposeExternal +// remain plugin-owned. +func ApplyPersistedPluginServerFields(live, persisted PluginServerConfig) PluginServerConfig { + live.Enabled = persisted.Enabled + live.ToolConfigs = persisted.ToolConfigs + if persisted.ID != "" { + live.ID = persisted.ID + } + return live +} + // syncPluginServersFromConfig merges persisted admin-owned plugin-server fields -// onto live plugin registrations. Callers must not hold pluginServersMu. +// onto live-registered entries only. Config-only orphan rows keep their +// identity/policy in config but never become runtime registry members — +// hydratePluginRegistrations (KV) and RegisterPluginServer own membership. +// Callers must not hold pluginServersMu. func (m *ClientManager) syncPluginServersFromConfig(cfg Config) { m.pluginServersMu.Lock() defer m.pluginServersMu.Unlock() - if m.pluginServers == nil { - m.pluginServers = make(map[string]PluginServerConfig) - } - if m.pluginRegistered == nil { - m.pluginRegistered = make(map[string]bool) - } - for _, persisted := range cfg.PluginServers { if persisted.PluginID == "" { continue } - if existing, ok := m.pluginServers[persisted.PluginID]; ok { - // Merge admin-owned fields onto the live entry; keep runtime identity - // and the plugin-controlled external exposure flag. - existing.Enabled = persisted.Enabled - existing.ToolConfigs = persisted.ToolConfigs - m.pluginServers[persisted.PluginID] = existing + existing, ok := m.pluginServers[persisted.PluginID] + if !ok || !m.pluginRegistered[persisted.PluginID] { continue } - m.pluginServers[persisted.PluginID] = persisted + m.pluginServers[persisted.PluginID] = ApplyPersistedPluginServerFields(existing, persisted) } } @@ -1008,7 +1107,7 @@ func filterToolsByConfig(rawTools []llm.Tool, cfg Config, embeddedClient *Embedd if !ps.Enabled { continue } - origin := "plugin://" + ps.PluginID + origin := config.PluginServerOrigin(ps.PluginID) serverByOrigin[origin] = &ServerConfig{ Name: ps.Name, Enabled: true, diff --git a/mcp/client_manager_reinit_test.go b/mcp/client_manager_reinit_test.go index 4e5dd602d..c83469a38 100644 --- a/mcp/client_manager_reinit_test.go +++ b/mcp/client_manager_reinit_test.go @@ -403,8 +403,10 @@ func TestGetToolsForUserSkipsUnregisteredPluginServers(t *testing.T) { manager := harness.newManagerWithoutPluginRegister() require.False(t, manager.IsPluginRegistered(plugin.PluginID)) + require.Equal(t, []PluginServerConfig{plugin}, manager.GetConfig().PluginServers, + "config-only plugin row must remain persisted in config") _, ok := manager.GetPluginServer(plugin.PluginID) - require.True(t, ok, "config still hydrates the plugin row") + require.False(t, ok, "config-only plugin row must not be hydrated into the live registry") tools, mcpErrors := manager.GetToolsForUser(context.Background(), "alice", ToolSelection{}) require.Nil(t, mcpErrors) diff --git a/mcp/client_manager_test.go b/mcp/client_manager_test.go index a8585f2fb..ebc4aaa2e 100644 --- a/mcp/client_manager_test.go +++ b/mcp/client_manager_test.go @@ -33,6 +33,20 @@ type fakePluginHTTPClient struct { pluginHTTP func(*http.Request) *http.Response } +type serverAccessCall struct { + userID string + serverID string +} + +type recordingServerAccessChecker struct { + calls []serverAccessCall +} + +func (c *recordingServerAccessChecker) CanUseMCPServer(_ context.Context, userID, serverID string) error { + c.calls = append(c.calls, serverAccessCall{userID: userID, serverID: serverID}) + return assert.AnError +} + func (f *fakePluginHTTPClient) PluginHTTP(req *http.Request) *http.Response { return f.pluginHTTP(req) } @@ -163,11 +177,39 @@ func TestClientManagerReInitIdleTimeoutDefaulting(t *testing.T) { } } +func TestClientManagerServiceAccountAccessUsesInvokingUser(t *testing.T) { + pluginTestAPI := &plugintest.API{} + setupClientManagerTestAPI(t, pluginTestAPI) + client := pluginapi.NewClient(pluginTestAPI, nil) + checker := &recordingServerAccessChecker{} + + manager := NewClientManager(Config{ + IdleTimeoutMinutes: 30, + Servers: []ServerConfig{{ + ID: "abcdefghijklmnopqrstuvwxyz", + Name: "Denied", + Enabled: true, + BaseURL: "https://must-not-connect.invalid", + ServiceAccountHeaders: map[string]string{"Authorization": "Bearer secret"}, + }}, + }, client.Log, client, nil, nil, nil, nil, checker) + t.Cleanup(manager.Close) + + tools, mcpErrors := manager.GetTools(context.Background(), ServiceAccountCatalogRequest("bot-user", "invoking-user")) + + require.Empty(t, tools) + require.Nil(t, mcpErrors, "policy denial is silent and must prevent dialing") + require.Equal(t, []serverAccessCall{{ + userID: "invoking-user", + serverID: "abcdefghijklmnopqrstuvwxyz", + }}, checker.calls) +} + func TestClientManager_PluginServerRegistry_RegisterUnregisterList(t *testing.T) { pluginTestAPI := &plugintest.API{} setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) cfgA := PluginServerConfig{PluginID: "a", Name: "A", Path: "/mcp", Enabled: true} @@ -203,13 +245,77 @@ func TestClientManager_PluginServerRegistry_RegisterUnregisterList(t *testing.T) require.Len(t, m.ListPluginServers(), 1) } +func TestClientManager_UpdatePluginServerAdminFields(t *testing.T) { + tests := []struct { + name string + pluginID string + enabled bool + toolConfigs []ToolConfig + expectFound bool + }{ + { + name: "patches admin fields on a registered entry", + pluginID: "com.example.mcp", + enabled: false, + toolConfigs: []ToolConfig{{Name: "echo", Policy: "ask", Enabled: false}}, + expectFound: true, + }, + { + name: "unregistered plugin reports not found", + pluginID: "com.example.missing", + enabled: true, + expectFound: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + pluginTestAPI := &plugintest.API{} + setupClientManagerTestAPI(t, pluginTestAPI) + client := pluginapi.NewClient(pluginTestAPI, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) + t.Cleanup(m.Close) + + registered := PluginServerConfig{ + PluginID: "com.example.mcp", + Name: "Example v2", + Path: "/mcp/v2", + Enabled: true, + ExposeExternal: true, + } + m.RegisterPluginServer(registered) + + got, ok := m.UpdatePluginServerAdminFields(tc.pluginID, tc.enabled, tc.toolConfigs) + require.Equal(t, tc.expectFound, ok) + if !tc.expectFound { + require.Equal(t, PluginServerConfig{}, got) + stored, stillOK := m.GetPluginServer("com.example.mcp") + require.True(t, stillOK) + require.Equal(t, registered, stored, "a miss must not disturb other entries") + return + } + + // Plugin-owned fields survive; admin-owned fields are patched. + require.Equal(t, "Example v2", got.Name) + require.Equal(t, "/mcp/v2", got.Path) + require.True(t, got.ExposeExternal) + require.Equal(t, tc.enabled, got.Enabled) + require.Equal(t, tc.toolConfigs, got.ToolConfigs) + + stored, stillOK := m.GetPluginServer(tc.pluginID) + require.True(t, stillOK) + require.Equal(t, got, stored, "the patched entry must be stored back") + }) + } +} + func TestClientManager_PluginRegistrationPersistence(t *testing.T) { pluginTestAPI := &plugintest.API{} setupTestLogger(pluginTestAPI) fixture := setupPluginRegistrationKV(t, pluginTestAPI, nil) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) first := PluginServerConfig{PluginID: "com.example.first", Name: "First", Path: "/mcp", Enabled: true} @@ -267,20 +373,21 @@ func TestClientManager_HydratesLivePluginRegistrations(t *testing.T) { ExposeExternal: false, ToolConfigs: []ToolConfig{adminToolConfig}, }}, - }, client.Log, client, nil, nil, nil, nil) + }, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) got, ok := m.GetPluginServer(live.PluginID) require.True(t, ok) - require.True(t, m.IsPluginRegistered(live.PluginID)) require.Equal(t, live.Name, got.Name) require.Equal(t, live.Path, got.Path) require.True(t, got.ExposeExternal) require.True(t, got.Enabled) require.Equal(t, []ToolConfig{adminToolConfig}, got.ToolConfigs) - require.False(t, m.IsPluginRegistered(disabled.PluginID)) - require.False(t, m.IsPluginRegistered(absent.PluginID)) + _, ok = m.GetPluginServer(disabled.PluginID) + require.False(t, ok) + _, ok = m.GetPluginServer(absent.PluginID) + require.False(t, ok) require.Equal(t, map[string]PluginServerConfig{ live.PluginID: live, }, fixture.registrations(t)) @@ -300,32 +407,14 @@ func TestClientManager_HydrationKeepsRegistrationsWhenServerConfigUnavailable(t pluginTestAPI.On("GetConfig").Return((*model.Config)(nil)) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil) - t.Cleanup(m.Close) - - require.True(t, m.IsPluginRegistered(first.PluginID)) - require.True(t, m.IsPluginRegistered(second.PluginID)) - require.Equal(t, persisted, fixture.registrations(t)) - require.Zero(t, fixture.writeCount()) -} - -func TestClientManager_UpdatePluginServerPreservesRegistrationStateAndKV(t *testing.T) { - pluginTestAPI := &plugintest.API{} - setupTestLogger(pluginTestAPI) - fixture := setupPluginRegistrationKV(t, pluginTestAPI, nil) - client := pluginapi.NewClient(pluginTestAPI, nil) - - orphan := PluginServerConfig{PluginID: "com.example.orphan", Name: "Orphan", Path: "/mcp", Enabled: true} - m := NewClientManager(Config{IdleTimeoutMinutes: 30, PluginServers: []PluginServerConfig{orphan}}, client.Log, client, nil, nil, nil, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) - orphan.Enabled = false - m.UpdatePluginServer(orphan) - - got, ok := m.GetPluginServer(orphan.PluginID) + _, ok := m.GetPluginServer(first.PluginID) + require.True(t, ok) + _, ok = m.GetPluginServer(second.PluginID) require.True(t, ok) - require.Equal(t, orphan, got) - require.False(t, m.IsPluginRegistered(orphan.PluginID)) + require.Equal(t, persisted, fixture.registrations(t)) require.Zero(t, fixture.writeCount()) } @@ -333,7 +422,7 @@ func TestClientManager_GetPluginServer(t *testing.T) { pluginTestAPI := &plugintest.API{} setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) cfg, ok := m.GetPluginServer("missing") @@ -361,31 +450,23 @@ func TestClientManager_GetPluginServer(t *testing.T) { require.Equal(t, stored, again, "GetPluginServer must return an independent value copy") } -func TestClientManager_HydratesPluginServersFromConfig(t *testing.T) { +func TestClientManager_ConfigOnlyPluginServersAreNotRuntimeMembers(t *testing.T) { pluginTestAPI := &plugintest.API{} setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - persisted := []PluginServerConfig{ - { - PluginID: "com.example.a", - Name: "A", - Path: "/mcp", - Enabled: true, - ExposeExternal: false, - ToolConfigs: []ToolConfig{ - {Name: "tool_a1", Policy: ToolPolicyAsk, Enabled: true}, - {Name: "tool_a2", Policy: ToolPolicyAsk, Enabled: false}, - }, - }, - { - PluginID: "com.example.b", - Name: "B", - Path: "/mcp", - Enabled: false, - ExposeExternal: true, + const orphanID = "abcdefghijklmnopqrstuvwx0o" + persisted := []PluginServerConfig{{ + ID: orphanID, + PluginID: "com.example.orphan", + Name: "Orphan", + Path: "/mcp", + Enabled: true, + ExposeExternal: true, + ToolConfigs: []ToolConfig{ + {Name: "tool_a1", Policy: ToolPolicyAsk, Enabled: true}, }, - } + }} m := NewClientManager( Config{IdleTimeoutMinutes: 30, PluginServers: persisted}, @@ -395,32 +476,15 @@ func TestClientManager_HydratesPluginServersFromConfig(t *testing.T) { nil, nil, nil, + nil, ) t.Cleanup(m.Close) - got := m.ListPluginServers() - require.Len(t, got, 2, "both persisted entries must be hydrated synchronously") - - byID := map[string]PluginServerConfig{} - for _, c := range got { - byID[c.PluginID] = c - } - - a := byID["com.example.a"] - require.Equal(t, "A", a.Name) - require.Equal(t, "/mcp", a.Path) - require.True(t, a.Enabled) - require.False(t, a.ExposeExternal) - require.Len(t, a.ToolConfigs, 2) - require.Equal(t, "tool_a1", a.ToolConfigs[0].Name) - require.True(t, a.ToolConfigs[0].Enabled) - require.False(t, a.ToolConfigs[1].Enabled) - - b := byID["com.example.b"] - require.Equal(t, "B", b.Name) - require.False(t, b.Enabled) - require.True(t, b.ExposeExternal) - require.Empty(t, b.ToolConfigs) + require.Empty(t, m.ListPluginServers(), "config-only rows must not appear in the live registry") + _, ok := m.GetPluginServer("com.example.orphan") + require.False(t, ok) + require.Empty(t, m.snapshotEnabledPluginServers()) + require.Equal(t, orphanID, m.GetConfig().PluginServers[0].ID, "identity must survive in config") } // A config broadcast must merge persisted admin-owned fields (Enabled, @@ -432,7 +496,7 @@ func TestClientManager_ReInitSyncsPluginServerAdminFields(t *testing.T) { setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) m.RegisterPluginServer(PluginServerConfig{ @@ -443,9 +507,11 @@ func TestClientManager_ReInitSyncsPluginServerAdminFields(t *testing.T) { ExposeExternal: false, }) + const persistedID = "abcdefghijklmnopqrstuvwx0p" newCfg := Config{ IdleTimeoutMinutes: 30, PluginServers: []PluginServerConfig{{ + ID: persistedID, PluginID: "com.example.mcp", Name: "Stale Name From Config", // must be ignored on merge Path: "/stale-from-config", // must be ignored on merge @@ -467,39 +533,83 @@ func TestClientManager_ReInitSyncsPluginServerAdminFields(t *testing.T) { require.Len(t, got.ToolConfigs, 1, "ToolConfigs merged from config") require.Equal(t, "echo", got.ToolConfigs[0].Name) require.False(t, got.ToolConfigs[0].Enabled) + require.Equal(t, persistedID, got.ID, "stable ABAC ID synced from config onto live entry") require.Equal(t, "Live Name", got.Name) require.Equal(t, "/live-mcp", got.Path) } -func TestClientManager_ReInitInsertsConfigOnlyEntries(t *testing.T) { +func TestApplyPersistedPluginServerFields(t *testing.T) { + tests := []struct { + name string + live PluginServerConfig + persisted PluginServerConfig + wantID string + wantEnabled bool + }{ + { + name: "persisted ID overlays empty live ID", + live: PluginServerConfig{PluginID: "com.example.a", Name: "Live", Path: "/mcp", Enabled: false}, + persisted: PluginServerConfig{ + ID: "config-id", PluginID: "com.example.a", Enabled: true, + ToolConfigs: []ToolConfig{{Name: "t", Enabled: true}}, + }, + wantID: "config-id", wantEnabled: true, + }, + { + name: "persisted ID replaces stale live ID", + live: PluginServerConfig{ID: "stale", PluginID: "com.example.a", Enabled: true}, + persisted: PluginServerConfig{ID: "config-id", PluginID: "com.example.a", Enabled: false}, + wantID: "config-id", wantEnabled: false, + }, + { + name: "empty persisted ID keeps live ID", + live: PluginServerConfig{ID: "live-id", PluginID: "com.example.a", Enabled: true}, + persisted: PluginServerConfig{PluginID: "com.example.a", Enabled: false}, + wantID: "live-id", wantEnabled: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ApplyPersistedPluginServerFields(tt.live, tt.persisted) + assert.Equal(t, tt.wantID, got.ID) + assert.Equal(t, tt.wantEnabled, got.Enabled) + assert.Equal(t, tt.live.Name, got.Name) + assert.Equal(t, tt.live.Path, got.Path) + assert.Equal(t, tt.live.ExposeExternal, got.ExposeExternal) + }) + } +} + +func TestClientManager_ReInitDoesNotInsertConfigOnlyEntries(t *testing.T) { pluginTestAPI := &plugintest.API{} setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) require.Empty(t, m.ListPluginServers(), "precondition: empty registry") + const orphanID = "abcdefghijklmnopqrstuvwx0o" cfg := Config{ IdleTimeoutMinutes: 30, PluginServers: []PluginServerConfig{{ + ID: orphanID, PluginID: "com.example.mcp", Name: "From Config", Path: "/from-config", Enabled: true, - ExposeExternal: false, + ExposeExternal: true, }}, } m.ReInit(cfg, nil) - got, ok := m.GetPluginServer("com.example.mcp") - require.True(t, ok) - require.Equal(t, "From Config", got.Name) - require.Equal(t, "/from-config", got.Path) - require.True(t, got.Enabled) + require.Empty(t, m.ListPluginServers()) + _, ok := m.GetPluginServer("com.example.mcp") + require.False(t, ok, "ReInit must not fabricate registry entries from config") + require.Equal(t, orphanID, m.GetConfig().PluginServers[0].ID) } // Live registrations absent from config must survive config broadcasts. @@ -508,7 +618,7 @@ func TestClientManager_ReInitPreservesUnpersistedRuntimeEntries(t *testing.T) { setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) live := PluginServerConfig{ @@ -530,73 +640,38 @@ func TestClientManager_ReInitPreservesUnpersistedRuntimeEntries(t *testing.T) { } m.ReInit(cfg, nil) - require.Len(t, m.ListPluginServers(), 2) + require.Len(t, m.ListPluginServers(), 1, "config-only other must not join the registry") stillLive, ok := m.GetPluginServer("com.example.live") require.True(t, ok, "runtime registration must survive ReInit") require.Equal(t, live, stillLive) } -func TestClientManager_IsPluginRegistered(t *testing.T) { +func TestClientManager_SyncPluginServersFromConfig_SkipsEmptyPluginID(t *testing.T) { pluginTestAPI := &plugintest.API{} setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - cfg := Config{ - IdleTimeoutMinutes: 30, - PluginServers: []PluginServerConfig{{ - PluginID: "com.example.orphan", - Name: "Orphan", - Path: "/mcp", - Enabled: true, - }}, - } - m := NewClientManager(cfg, client.Log, client, nil, nil, nil, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) - require.False(t, m.IsPluginRegistered("com.example.orphan"), - "entry hydrated only from persisted config must not be reported as registered") - require.False(t, m.IsPluginRegistered("com.example.missing")) - - m.RegisterPluginServer(PluginServerConfig{ - PluginID: "com.example.live", - Name: "Live", - Path: "/live", - Enabled: true, - }) - require.True(t, m.IsPluginRegistered("com.example.live")) - m.RegisterPluginServer(PluginServerConfig{ - PluginID: "com.example.orphan", - Name: "Orphan", - Path: "/mcp", - Enabled: true, + PluginID: "com.example.valid", Name: "Valid", Path: "/mcp", Enabled: true, }) - require.True(t, m.IsPluginRegistered("com.example.orphan"), - "an explicit Register must mark a previously-orphan entry as registered") - - m.UnregisterPluginServer("com.example.live") - require.False(t, m.IsPluginRegistered("com.example.live")) -} - -func TestClientManager_SyncPluginServersFromConfig_SkipsEmptyPluginID(t *testing.T) { - pluginTestAPI := &plugintest.API{} - setupClientManagerTestAPI(t, pluginTestAPI) - client := pluginapi.NewClient(pluginTestAPI, nil) - cfg := Config{ + m.ReInit(Config{ IdleTimeoutMinutes: 30, PluginServers: []PluginServerConfig{ {PluginID: "", Name: "Empty ID", Path: "/x", Enabled: true}, - {PluginID: "com.example.valid", Name: "Valid", Path: "/mcp", Enabled: true}, + {PluginID: "com.example.valid", Name: "Stale", Path: "/stale", Enabled: false, ID: "abcdefghijklmnopqrstuvwx0v"}, }, - } - - m := NewClientManager(cfg, client.Log, client, nil, nil, nil, nil) - t.Cleanup(m.Close) + }, nil) got := m.ListPluginServers() - require.Len(t, got, 1, "empty-PluginID entry must be skipped; only valid entry hydrated") + require.Len(t, got, 1) require.Equal(t, "com.example.valid", got[0].PluginID) + require.Equal(t, "Valid", got[0].Name, "plugin-owned Name must survive sync") + require.False(t, got[0].Enabled, "admin Enabled merged from config") + require.Equal(t, "abcdefghijklmnopqrstuvwx0v", got[0].ID) } func TestClientManager_GetToolsForUser_PluginEnabled(t *testing.T) { @@ -609,7 +684,7 @@ func TestClientManager_GetToolsForUser_PluginEnabled(t *testing.T) { setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI, nil) t.Cleanup(m.Close) cfg := PluginServerConfig{ @@ -639,7 +714,7 @@ func TestClientManager_GetToolsForUser_PluginDisabled_ZeroTools(t *testing.T) { setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI, nil) t.Cleanup(m.Close) cfg := PluginServerConfig{ @@ -690,7 +765,7 @@ func TestClientManager_GetToolsForUser_PluginEnabled_HTTPFailure(t *testing.T) { setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI, nil) t.Cleanup(m.Close) m.RegisterPluginServer(PluginServerConfig{ @@ -739,7 +814,7 @@ func TestClientManager_GetToolsForUser_PluginConnectErrorsAreRequestScoped(t *te setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI, nil) t.Cleanup(m.Close) m.RegisterPluginServer(PluginServerConfig{ PluginID: "com.example.mcp", @@ -786,7 +861,7 @@ func TestClientManager_GetToolsForUser_MultiplePluginServers(t *testing.T) { }, } - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI, nil) t.Cleanup(m.Close) m.RegisterPluginServer(PluginServerConfig{PluginID: "com.example.a", Name: "A", Path: "/mcp", Enabled: true}) @@ -811,7 +886,7 @@ func TestClientManager_PluginServerRegistry_RaceSafe(t *testing.T) { pluginTestAPI := &plugintest.API{} setupClientManagerTestAPI(t, pluginTestAPI) client := pluginapi.NewClient(pluginTestAPI, nil) - m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil) + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, nil, nil) t.Cleanup(m.Close) const writers = 8 @@ -909,18 +984,17 @@ func TestClientManagerGetToolRetrievalOverridesEmbedded(t *testing.T) { func TestClientManagerGetToolRetrievalOverridesPlugin(t *testing.T) { manager := &ClientManager{ - config: Config{ - PluginServers: []PluginServerConfig{ - { - PluginID: "com.example.mcp", - Enabled: true, - ToolConfigs: []ToolConfig{ - {Name: "lookup", Policy: ToolPolicyAsk, Enabled: true, RetrievalDescriptionOverride: "Find plugin records"}, - }, - }, - }, + pluginServers: make(map[string]PluginServerConfig), + pluginRegistered: make(map[string]bool), + } + manager.pluginServers["com.example.mcp"] = PluginServerConfig{ + PluginID: "com.example.mcp", + Enabled: true, + ToolConfigs: []ToolConfig{ + {Name: "lookup", Policy: ToolPolicyAsk, Enabled: true, RetrievalDescriptionOverride: "Find plugin records"}, }, } + manager.pluginRegistered["com.example.mcp"] = true overrides := manager.GetToolRetrievalOverrides() diff --git a/mcp/testhelpers_test.go b/mcp/testhelpers_test.go index d210961d2..b7dedb05c 100644 --- a/mcp/testhelpers_test.go +++ b/mcp/testhelpers_test.go @@ -541,7 +541,7 @@ func (s *EmbeddedTestSuite) CreateClientManager(t *testing.T, session *model.Ses } // Create ClientManager with nil httpClient for tests (no remote requests in these tests) - manager := NewClientManager(config, pluginAPIClient.Log, pluginAPIClient, nil, wrapper, nil, nil) + manager := NewClientManager(config, pluginAPIClient.Log, pluginAPIClient, nil, wrapper, nil, nil, nil) require.NotNil(t, manager, "ClientManager should not be nil") return manager diff --git a/mcp/user_clients.go b/mcp/user_clients.go index 70538771c..1cdeb01dd 100644 --- a/mcp/user_clients.go +++ b/mcp/user_clients.go @@ -17,6 +17,7 @@ import ( "sync" "time" + "github.com/mattermost/mattermost-plugin-agents/v2/config" "github.com/mattermost/mattermost-plugin-agents/v2/llm" "github.com/mattermost/mattermost-plugin-agents/v2/mmapi" "github.com/mattermost/mattermost/server/public/pluginapi" @@ -909,5 +910,5 @@ func shortSlugHash(value string) string { // pluginServerOriginKey returns the synthetic origin string for plugin-server // tools. Must match the key used by filterToolsByConfig. func pluginServerOriginKey(pluginID string) string { - return "plugin://" + pluginID + return config.PluginServerOrigin(pluginID) } diff --git a/mcp/user_clients_test.go b/mcp/user_clients_test.go index 7fe9e0ce5..a64acab1e 100644 --- a/mcp/user_clients_test.go +++ b/mcp/user_clients_test.go @@ -512,6 +512,7 @@ func TestClientManagerGetToolsForUser_ReconnectsAfterStoredSessionRevoked(t *tes &sessionEchoEmbeddedMCPServer{ctx: runCtx}, http.DefaultClient, nil, + nil, ) t.Cleanup(manager.Close) diff --git a/server/abac_id_migrations.go b/server/abac_id_migrations.go new file mode 100644 index 000000000..99153f26b --- /dev/null +++ b/server/abac_id_migrations.go @@ -0,0 +1,48 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package main + +import ( + "fmt" + + "github.com/mattermost/mattermost-plugin-agents/v2/store" + "github.com/mattermost/mattermost/server/public/plugin" + "github.com/mattermost/mattermost/server/public/pluginapi" + "github.com/mattermost/mattermost/server/public/pluginapi/cluster" +) + +// runABACIDMigrations runs the one-time ABAC identity migrations: legacy UUID +// service IDs are rewritten to model.NewId() values, and MCP servers (external, +// embedded, and plugin-registered) get stable IDs assigned. All run in a +// single idempotent store transaction, guarded by one cluster mutex. Returns +// whether the migration wrote to the DB. Callers must load config from the +// store afterwards — Migrated=false means another node already wrote, not that +// this process's memory is current. +func runABACIDMigrations(api plugin.API, pluginAPI *pluginapi.Client, st *store.Store) (bool, error) { + mtx, err := cluster.NewMutex(api, "ai_abac_id_migration") + if err != nil { + return false, fmt.Errorf("failed to create ABAC ID migration mutex: %w", err) + } + mtx.Lock() + defer mtx.Unlock() + + report, err := st.MigrateABACIDs() + if err != nil { + return false, fmt.Errorf("failed to migrate ABAC IDs: %w", err) + } + + if report.Migrated { + pluginAPI.Log.Info("ABAC ID migrations applied", + "services_remapped", report.ServicesRemapped, + "agent_rows_updated", report.AgentRowsUpdated, + "mcp_ids_assigned", report.MCPServerIDsAssigned, + "embedded_plugin_mcp_ids_assigned", report.EmbeddedPluginServerIDsAssigned, + ) + } + for _, ref := range report.DanglingServiceRefs { + pluginAPI.Log.Warn("Dangling service reference left unchanged by service ID migration", "reference", ref) + } + + return report.Migrated, nil +} diff --git a/server/legacy_bot_migration.go b/server/legacy_bot_migration.go index 5693fc43f..89912ec34 100644 --- a/server/legacy_bot_migration.go +++ b/server/legacy_bot_migration.go @@ -4,7 +4,9 @@ package main import ( + "errors" "fmt" + "reflect" "github.com/mattermost/mattermost-plugin-agents/v2/config" "github.com/mattermost/mattermost-plugin-agents/v2/store" @@ -114,19 +116,36 @@ func migrateLegacyConfigBotsToUserAgents(api plugin.API, pluginAPI *pluginapi.Cl byUsername[bc.Name] = struct{}{} } - newCfg := *dbCfg - newCfg.Bots = nil - if saveErr := st.SaveConfig(newCfg); saveErr != nil { - return false, fmt.Errorf("failed to save config after legacy bot migration: %w", saveErr) + // Clear Bots atomically against the freshest persisted config: a blind + // SaveConfig of the dbCfg snapshot read above could overwrite a concurrent + // writer's changes (e.g. an admin save or the ID migration's remapped IDs). + // If Bots itself changed since the snapshot the agents were created from, + // defer instead of clearing a list that was never migrated; the created + // agents are deduplicated by username on the retry. + errBotsChanged := errors.New("config bots changed during legacy bot migration") + saved, err := st.UpdateConfig(func(prev *config.Config) (config.Config, error) { + if prev == nil { + return config.Config{}, fmt.Errorf("active config disappeared during legacy bot migration") + } + if !reflect.DeepEqual(prev.Bots, dbCfg.Bots) { + return config.Config{}, errBotsChanged + } + next := *prev + next.Bots = nil + return next, nil + }) + if errors.Is(err, errBotsChanged) { + // Soft defer (not an error): we retry on the next config update. + pluginAPI.Log.Warn("Deferring legacy bot migration: config bots changed concurrently") + return false, nil } - reloaded, err := st.GetConfig() if err != nil { - return false, fmt.Errorf("failed to reload config: %w", err) + return false, fmt.Errorf("failed to save config after legacy bot migration: %w", err) } - if reloaded != nil { - if err := cfg.StorePersistedConfigWithoutNotify(reloaded); err != nil { - return false, fmt.Errorf("failed to store config after legacy bot migration: %w", err) - } + // Deliberately without notify: the caller is already servicing a config + // update listener; notifying here would re-enter it. + if err := cfg.StorePersistedConfigWithoutNotify(&saved); err != nil { + return false, fmt.Errorf("failed to store config after legacy bot migration: %w", err) } if err := st.SetSystemValue(legacyConfigBotsMigratedKey, "true"); err != nil { diff --git a/server/main.go b/server/main.go index aa62bf654..86d8faa54 100644 --- a/server/main.go +++ b/server/main.go @@ -191,7 +191,15 @@ func (p *Plugin) OnActivate() error { } mtx2.Unlock() - // Load config from DB into memory and set migrated flag + // ABAC ID migrations must run after the config.json->DB migration and + // before runtime config is loaded. A follower that waited on the cluster + // lock must reload the winner's remapped IDs even when Migrated is false. + idsMigrated, err := runABACIDMigrations(p.API, pluginAPI, p.store) + if err != nil { + return fmt.Errorf("failed to run ABAC ID migrations: %w", err) + } + + // Load the fully migrated config from DB into memory and set migrated flag. dbConfig, err := p.store.GetConfig() if err != nil { return fmt.Errorf("failed to load config from database: %w", err) @@ -201,6 +209,15 @@ func (p *Plugin) OnActivate() error { } p.configMigrated = true + if idsMigrated { + if pubErr := p.PublishConfigUpdate(); pubErr != nil { + pluginAPI.Log.Error("Failed to publish config update after ID migration", "error", pubErr.Error()) + } + if pubErr := p.PublishAgentUpdate(); pubErr != nil { + pluginAPI.Log.Error("Failed to publish agent update after ID migration", "error", pubErr.Error()) + } + } + bots := bots.New(p.API, pluginAPI, licenseChecker, &p.configuration, p.store, llmUpstreamHTTPClient, metricsService) // migrateAndRefresh runs the one-time legacy bot migration, then forces diff --git a/store/config.go b/store/config.go index ca404051b..48f5c0758 100644 --- a/store/config.go +++ b/store/config.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" + "github.com/jmoiron/sqlx" "github.com/mattermost/mattermost-plugin-agents/v2/config" "github.com/mattermost/mattermost/server/public/model" ) @@ -18,6 +19,12 @@ const ( configSaveLockKey = int32(1) ) +// ErrLegacyUUIDServiceID is returned by any config write that contains a +// dashed UUID in Services[].ID after the ABAC ID migration marker is set. +// That format is invalid post-migration; the write is rejected, not reminted. +// Enforced inside insertActiveConfigTx so every writer is covered. +var ErrLegacyUUIDServiceID = errors.New("config contains invalid UUID service IDs after the ID migration") + // GetConfig retrieves the currently active configuration from the database. // Returns nil, nil if no active config exists (e.g., fresh install before migration). func (s *Store) GetConfig() (*config.Config, error) { @@ -41,12 +48,11 @@ func (s *Store) GetConfig() (*config.Config, error) { // SaveConfig persists a new configuration to the database with history. // The previous active config is deactivated and a new active row is inserted. // All prior configs are preserved with Active = false. +// +// SaveConfig is a blind write, only appropriate for bootstrap/first-write +// paths. Read-modify-write callers must use UpdateConfig or they can lose a +// concurrent writer's update. func (s *Store) SaveConfig(cfg config.Config) error { - configBytes, err := json.Marshal(cfg) - if err != nil { - return fmt.Errorf("failed to marshal config: %w", err) - } - tx, err := s.db.Beginx() if err != nil { return fmt.Errorf("failed to begin transaction: %w", err) @@ -63,13 +69,95 @@ func (s *Store) SaveConfig(cfg config.Config) error { return fmt.Errorf("failed to lock config save transaction: %w", err) } + if err = insertActiveConfigTx(tx, cfg); err != nil { + return err + } + + if err = tx.Commit(); err != nil { + return fmt.Errorf("failed to commit config save: %w", err) + } + + return nil +} + +// UpdateConfig atomically reads the active config, applies transform, and +// persists the result as the new active row — one transaction under the same +// advisory lock as SaveConfig, so no concurrent save or migration can +// interleave. transform receives nil when no active config exists; a +// transform error aborts with nothing written and is returned unwrapped so +// callers can map their own sentinel errors. +func (s *Store) UpdateConfig(transform func(prev *config.Config) (config.Config, error)) (config.Config, error) { + var next config.Config + + tx, err := s.db.Beginx() + if err != nil { + return next, fmt.Errorf("failed to begin transaction: %w", err) + } + defer func() { + if err != nil { + _ = tx.Rollback() + } + }() + + if _, err = tx.Exec("SELECT pg_advisory_xact_lock($1, $2)", configSaveLockNamespace, configSaveLockKey); err != nil { + return next, fmt.Errorf("failed to lock config update transaction: %w", err) + } + + prev, _, err := getActiveConfigTx(tx) + if err != nil { + return next, err + } + next, err = transform(prev) + if err != nil { + return next, err + } + + if err = insertActiveConfigTx(tx, next); err != nil { + return next, err + } + + if err = tx.Commit(); err != nil { + return next, fmt.Errorf("failed to commit config update: %w", err) + } + + return next, nil +} + +// configHasLegacyUUIDServiceIDs reports whether any service entry carries a +// pre-migration 36-char UUID as its ID. +func configHasLegacyUUIDServiceIDs(cfg *config.Config) bool { + for i := range cfg.Services { + if isLegacyUUID(cfg.Services[i].ID) { + return true + } + } + return false +} + +// insertActiveConfigTx deactivates the current active config-history row and +// inserts cfg as the new active one. After the ABAC ID migration marker is +// set, a dashed UUID in Services[].ID is an invalid ID format and the write +// is rejected (the migration itself rewrites UUIDs before inserting, so it +// never trips the guard). +func insertActiveConfigTx(tx *sqlx.Tx, cfg config.Config) error { + migrated, err := getSystemValueTx(tx, abacIDMigrationKey) + if err != nil { + return err + } + if migrated == "1" && configHasLegacyUUIDServiceIDs(&cfg) { + return ErrLegacyUUIDServiceID + } + + configBytes, err := json.Marshal(cfg) + if err != nil { + return fmt.Errorf("failed to marshal config: %w", err) + } + // Deactivate current active config (at most one row, indexed on Active) - if _, err = tx.Exec("UPDATE Agents_ConfigHistory SET Active = false WHERE Active = true"); err != nil { + if _, err := tx.Exec("UPDATE Agents_ConfigHistory SET Active = false WHERE Active = true"); err != nil { return fmt.Errorf("failed to deactivate current config: %w", err) } - - // Insert new active config - if _, err = tx.Exec( + if _, err := tx.Exec( "INSERT INTO Agents_ConfigHistory (ID, Config, CreateAt, Active) VALUES ($1, $2, $3, $4)", model.NewId(), string(configBytes), @@ -78,11 +166,6 @@ func (s *Store) SaveConfig(cfg config.Config) error { ); err != nil { return fmt.Errorf("failed to insert new config: %w", err) } - - if err = tx.Commit(); err != nil { - return fmt.Errorf("failed to commit config save: %w", err) - } - return nil } diff --git a/store/config_test.go b/store/config_test.go index b0d342e62..3d90cf2db 100644 --- a/store/config_test.go +++ b/store/config_test.go @@ -380,6 +380,107 @@ func TestSaveConfigConcurrent(t *testing.T) { assert.Equal(t, workerCount, totalCount) } +// TestSaveConfigRejectsLegacyUUIDsAfterMigration proves the post-migration +// legacy UUID guard lives inside the shared write primitive: even a direct +// SaveConfig (not just UpdateConfig) cannot reintroduce pre-migration UUID +// service IDs once the migration marker is set. +func TestSaveConfigRejectsLegacyUUIDsAfterMigration(t *testing.T) { + s := setupTestStore(t) + require.NoError(t, s.RunMigrations()) + + require.NoError(t, s.SaveConfig(config.Config{ + Services: []llm.ServiceConfig{{ID: testUUIDA, Name: "A"}}, + })) + report, err := s.MigrateABACIDs() + require.NoError(t, err) + require.True(t, report.Migrated) + migrated, err := s.GetConfig() + require.NoError(t, err) + + rowsBefore := configHistoryCount(t, s) + err = s.SaveConfig(config.Config{ + Services: []llm.ServiceConfig{{ID: testUUIDA, Name: "A"}}, + }) + require.ErrorIs(t, err, ErrLegacyUUIDServiceID) + assert.Equal(t, rowsBefore, configHistoryCount(t, s), "rejected save must not write a config row") + + current, err := s.GetConfig() + require.NoError(t, err) + assert.Equal(t, migrated.Services[0].ID, current.Services[0].ID, "migrated ID must survive the rejected save") + + // A payload without UUIDs still saves normally. + require.NoError(t, s.SaveConfig(*migrated)) +} + +// TestUpdateConfigConcurrentWritersNoLostUpdate is the read-modify-write race +// regression: two concurrent writers that each modify a different part of the +// config must both land — the advisory lock serializes them and each +// transform sees the other's committed write, so no update (including the +// migrated service IDs the base config carries) can be lost. +func TestUpdateConfigConcurrentWritersNoLostUpdate(t *testing.T) { + baseStore := setupTestStore(t) + require.NoError(t, baseStore.RunMigrations()) + + var schemaName string + require.NoError(t, baseStore.db.Get(&schemaName, "SELECT current_schema()")) + + // Base config as the ID migration left it: migrated 26-char service ID. + require.NoError(t, baseStore.SaveConfig(config.Config{ + Services: []llm.ServiceConfig{{ID: testUUIDA, Name: "A"}}, + })) + report, err := baseStore.MigrateABACIDs() + require.NoError(t, err) + require.True(t, report.Migrated) + migrated, err := baseStore.GetConfig() + require.NoError(t, err) + migratedID := migrated.Services[0].ID + + writerA := setupSchemaBoundStore(t, schemaName) + writerB := setupSchemaBoundStore(t, schemaName) + + start := make(chan struct{}) + errCh := make(chan error, 2) + var wg sync.WaitGroup + + wg.Add(2) + go func() { + defer wg.Done() + <-start + _, updateErr := writerA.UpdateConfig(func(prev *config.Config) (config.Config, error) { + next := *prev + next.DefaultBotName = "writer-a" + return next, nil + }) + errCh <- updateErr + }() + go func() { + defer wg.Done() + <-start + _, updateErr := writerB.UpdateConfig(func(prev *config.Config) (config.Config, error) { + next := *prev + next.MCP.Servers = append(append([]config.MCPServerConfig(nil), prev.MCP.Servers...), + config.MCPServerConfig{ID: "writerbserverwriterbserver", Name: "B", BaseURL: "https://b.example.com"}) + return next, nil + }) + errCh <- updateErr + }() + + close(start) + wg.Wait() + close(errCh) + for updateErr := range errCh { + require.NoError(t, updateErr) + } + + final, err := baseStore.GetConfig() + require.NoError(t, err) + assert.Equal(t, "writer-a", final.DefaultBotName, "writer A's change must survive") + require.Len(t, final.MCP.Servers, 1, "writer B's change must survive") + assert.Equal(t, "writerbserverwriterbserver", final.MCP.Servers[0].ID) + require.Len(t, final.Services, 1) + assert.Equal(t, migratedID, final.Services[0].ID, "migrated service ID must not be lost") +} + func TestSaveConfigWaitsForConfigLock(t *testing.T) { baseStore := setupTestStore(t) diff --git a/store/id_migrations.go b/store/id_migrations.go new file mode 100644 index 000000000..f51e665d8 --- /dev/null +++ b/store/id_migrations.go @@ -0,0 +1,265 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package store + +import ( + "database/sql" + "encoding/json" + "errors" + "fmt" + + "github.com/google/uuid" + "github.com/jmoiron/sqlx" + "github.com/mattermost/mattermost-plugin-agents/v2/config" + "github.com/mattermost/mattermost/server/public/model" +) + +// Agents_System marker for the one-time ABAC ID migration (value "1"). +const abacIDMigrationKey = "abac_id_migration_done" + +// isLegacyUUID reports whether s is a canonical dashed UUID, the legacy +// service-ID format that predates model.NewId()-style IDs. +func isLegacyUUID(s string) bool { + return len(s) == 36 && uuid.Validate(s) == nil +} + +// ABACIDMigrationReport summarizes what MigrateABACIDs did so the caller +// (server layer) can log it; the store has no logger. +type ABACIDMigrationReport struct { + // Migrated is true when a new active config row was written. + Migrated bool + // ServicesRemapped counts service entries whose ID was rewritten, + // including each occurrence of a duplicated legacy ID. + ServicesRemapped int + AgentRowsUpdated int64 + MCPServerIDsAssigned int + EmbeddedPluginServerIDsAssigned int + DanglingServiceRefs []string +} + +// MigrateABACIDs runs the one-time ABAC ID migration in a single transaction +// that writes at most one new active config row and sets the Agents_System +// marker atomically: +// +// - service IDs: legacy UUID service IDs in the active config and in +// Agents_UserAgents.ServiceID are rewritten to model.NewId() values. +// Dangling UUID references are left unchanged and reported. +// - MCP server IDs: every external MCP server entry with no ID gets a +// stable model.NewId(). +// - embedded/plugin MCP server IDs: EmbeddedServer.ID and every +// PluginServers entry with no ID get a stable model.NewId(). +// +// Idempotent: the marker short-circuits re-runs, and the rewrite is content-based +// (only UUID service IDs / empty MCP IDs are touched). Config and marker writes +// share one Postgres transaction; on failure the transaction is rolled back. +func (s *Store) MigrateABACIDs() (ABACIDMigrationReport, error) { + report := ABACIDMigrationReport{} + + // Fast path; the authoritative re-check happens inside the tx under the lock. + done, err := s.GetSystemValue(abacIDMigrationKey) + if err != nil { + return report, err + } + if done == "1" { + return report, nil + } + + tx, err := s.db.Beginx() + if err != nil { + return report, fmt.Errorf("failed to begin ABAC ID migration transaction: %w", err) + } + defer func() { + if err != nil { + _ = tx.Rollback() + } + }() + + // Same advisory lock as SaveConfig: serializes against concurrent admin + // saves and migration attempts from other nodes. + if _, err = tx.Exec("SELECT pg_advisory_xact_lock($1, $2)", configSaveLockNamespace, configSaveLockKey); err != nil { + return report, fmt.Errorf("failed to lock ABAC ID migration transaction: %w", err) + } + + done, err = getSystemValueTx(tx, abacIDMigrationKey) + if err != nil { + return report, err + } + if done == "1" { + // Another node finished between the fast path and the lock. + _ = tx.Rollback() + return report, nil + } + + cfg, found, err := getActiveConfigTx(tx) + if err != nil { + return report, err + } + + configChanged := false + if found { + if err = migrateServiceIDsTx(tx, cfg, &report); err != nil { + return report, err + } + configChanged = configChanged || report.ServicesRemapped > 0 + + for i := range cfg.MCP.Servers { + if cfg.MCP.Servers[i].ID == "" { + cfg.MCP.Servers[i].ID = model.NewId() + report.MCPServerIDsAssigned++ + } + } + configChanged = configChanged || report.MCPServerIDsAssigned > 0 + + if cfg.MCP.EmbeddedServer.ID == "" { + cfg.MCP.EmbeddedServer.ID = model.NewId() + report.EmbeddedPluginServerIDsAssigned++ + } + for i := range cfg.MCP.PluginServers { + if cfg.MCP.PluginServers[i].ID == "" { + cfg.MCP.PluginServers[i].ID = model.NewId() + report.EmbeddedPluginServerIDsAssigned++ + } + } + configChanged = configChanged || report.EmbeddedPluginServerIDsAssigned > 0 + } + + if configChanged { + if err = insertActiveConfigTx(tx, *cfg); err != nil { + return report, err + } + } + if err = setSystemValueTx(tx, abacIDMigrationKey, "1"); err != nil { + return report, err + } + if err = tx.Commit(); err != nil { + return report, fmt.Errorf("failed to commit ABAC ID migration: %w", err) + } + + report.Migrated = configChanged + return report, nil +} + +// migrateServiceIDsTx rewrites legacy UUID service IDs in cfg (in place) and +// in Agents_UserAgents rows, recording what it did in report. It does not +// write the config row; the caller commits everything in one transaction. +func migrateServiceIDsTx(tx *sqlx.Tx, cfg *config.Config, report *ABACIDMigrationReport) error { + // Duplicate legacy UUIDs: each occurrence gets its own new ID, but + // references remap to the FIRST occurrence's, mirroring GetServiceByID. + idMap := make(map[string]string) + for i := range cfg.Services { + oldID := cfg.Services[i].ID + if !isLegacyUUID(oldID) { + continue + } + newID := model.NewId() + if _, seen := idMap[oldID]; !seen { + idMap[oldID] = newID + } + cfg.Services[i].ID = newID + report.ServicesRemapped++ + } + + // Even when no service ID was remapped, keep going: dangling UUID + // references (fallbacks, bots, agent rows pointing at services absent + // from the active config) must still be detected and reported, or the + // one-time marker would permanently suppress the diagnosis. + for i := range cfg.Services { + fb := cfg.Services[i].FallbackServiceID + if fb == "" { + continue + } + if newID, ok := idMap[fb]; ok { + cfg.Services[i].FallbackServiceID = newID + } else if isLegacyUUID(fb) { + report.DanglingServiceRefs = append(report.DanglingServiceRefs, + fmt.Sprintf("service %q fallback references unknown service %q", cfg.Services[i].ID, fb)) + } + } + + for i := range cfg.Bots { + sid := cfg.Bots[i].ServiceID + if sid == "" { + continue + } + if newID, ok := idMap[sid]; ok { + cfg.Bots[i].ServiceID = newID + } else if isLegacyUUID(sid) { + report.DanglingServiceRefs = append(report.DanglingServiceRefs, + fmt.Sprintf("config bot %q references unknown service %q", cfg.Bots[i].ID, sid)) + } + } + + // Update every agent row, including soft-deleted ones. UpdateAt is + // deliberately not bumped (data migration, not a user edit). + for oldID, newID := range idMap { + res, err := tx.Exec("UPDATE Agents_UserAgents SET ServiceID = $1 WHERE ServiceID = $2", newID, oldID) + if err != nil { + return fmt.Errorf("failed to update agent service IDs: %w", err) + } + rows, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("failed to count updated agent rows: %w", err) + } + report.AgentRowsUpdated += rows + } + + // Any agent row still holding a UUID references a service that no longer + // exists in the active config; report it, leave it unchanged. + var remaining []string + if err := tx.Select(&remaining, "SELECT DISTINCT ServiceID FROM Agents_UserAgents WHERE LENGTH(ServiceID) = 36"); err != nil { + return fmt.Errorf("failed to check for dangling agent service IDs: %w", err) + } + for _, sid := range remaining { + if isLegacyUUID(sid) { + report.DanglingServiceRefs = append(report.DanglingServiceRefs, + fmt.Sprintf("agent rows reference unknown service %q", sid)) + } + } + return nil +} + +// getSystemValueTx reads an Agents_System value on the transaction. +func getSystemValueTx(tx *sqlx.Tx, key string) (string, error) { + var value string + err := tx.Get(&value, "SELECT SValue FROM Agents_System WHERE SKey = $1", key) + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + if err != nil { + return "", fmt.Errorf("failed to get system value for key %q: %w", key, err) + } + return value, nil +} + +// setSystemValueTx upserts an Agents_System value on the transaction. +func setSystemValueTx(tx *sqlx.Tx, key, value string) error { + _, err := tx.Exec( + `INSERT INTO Agents_System (SKey, SValue) VALUES ($1, $2) + ON CONFLICT (SKey) DO UPDATE SET SValue = $2`, + key, value, + ) + if err != nil { + return fmt.Errorf("failed to set system value for key %q: %w", key, err) + } + return nil +} + +// getActiveConfigTx reads the active config row on the transaction. +// Returns found == false when no active config exists (fresh install). +func getActiveConfigTx(tx *sqlx.Tx) (*config.Config, bool, error) { + var configJSON string + err := tx.Get(&configJSON, "SELECT Config FROM Agents_ConfigHistory WHERE Active = true LIMIT 1") + if errors.Is(err, sql.ErrNoRows) { + return nil, false, nil + } + if err != nil { + return nil, false, fmt.Errorf("failed to get active config: %w", err) + } + + var cfg config.Config + if err := json.Unmarshal([]byte(configJSON), &cfg); err != nil { + return nil, false, fmt.Errorf("failed to unmarshal config: %w", err) + } + return &cfg, true, nil +} diff --git a/store/id_migrations_test.go b/store/id_migrations_test.go new file mode 100644 index 000000000..346867466 --- /dev/null +++ b/store/id_migrations_test.go @@ -0,0 +1,740 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package store + +import ( + "encoding/json" + "sync" + "testing" + + "github.com/mattermost/mattermost-plugin-agents/v2/config" + "github.com/mattermost/mattermost-plugin-agents/v2/llm" + "github.com/mattermost/mattermost/server/public/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + testUUIDA = "550e8400-e29b-41d4-a716-446655440000" + testUUIDB = "550e8400-e29b-41d4-a716-446655440001" + testUUIDC = "550e8400-e29b-41d4-a716-446655440002" + // Valid UUID deliberately absent from every seeded service list. + testUUIDDangling = "550e8400-e29b-41d4-a716-446655449999" +) + +// seedConfigRow inserts a config history row directly, bypassing SaveConfig. +func seedConfigRow(t *testing.T, s *Store, cfg config.Config, active bool) { + t.Helper() + data, err := json.Marshal(cfg) + require.NoError(t, err) + _, err = s.db.Exec( + "INSERT INTO Agents_ConfigHistory (ID, Config, CreateAt, Active) VALUES ($1, $2, $3, $4)", + model.NewId(), string(data), model.GetMillis(), active, + ) + require.NoError(t, err) +} + +// seedAgentRow inserts an agent row directly so arbitrary ServiceIDs, +// soft-deletion, and timestamps can be seeded without CreateAgent's ID reset. +func seedAgentRow(t *testing.T, s *Store, id, serviceID string, updateAt, deleteAt int64) { + t.Helper() + _, err := s.db.Exec( + `INSERT INTO Agents_UserAgents ( + ID, BotUserID, CreatorID, DisplayName, Username, ServiceID, + CreateAt, UpdateAt, DeleteAt + ) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9)`, + id, "bot-"+id, "creator", "Agent "+id, "agent-"+id, serviceID, + int64(1), updateAt, deleteAt, + ) + require.NoError(t, err) +} + +func configHistoryCount(t *testing.T, s *Store) int { + t.Helper() + var count int + require.NoError(t, s.db.Get(&count, "SELECT COUNT(*) FROM Agents_ConfigHistory")) + return count +} + +func requireMarker(t *testing.T, s *Store, key string) { + t.Helper() + marker, err := s.GetSystemValue(key) + require.NoError(t, err) + assert.Equal(t, "1", marker, "marker %q must be set", key) +} + +type agentServiceRow struct { + ServiceID string `db:"serviceid"` + UpdateAt int64 `db:"updateat"` +} + +func getAgentServiceRow(t *testing.T, s *Store, id string) agentServiceRow { + t.Helper() + var row agentServiceRow + require.NoError(t, s.db.Get(&row, "SELECT ServiceID, UpdateAt FROM Agents_UserAgents WHERE ID = $1", id)) + return row +} + +func TestMigrateABACIDs(t *testing.T) { + tests := []struct { + name string + seed func(t *testing.T, s *Store) + validate func(t *testing.T, s *Store, report ABACIDMigrationReport) + }{ + { + name: "all legacy UUIDs remapped", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A"}, + {ID: testUUIDB, Name: "B"}, + }, + }, true) + seedAgentRow(t, s, "agent1", testUUIDA, 100, 0) + seedAgentRow(t, s, "agent2", testUUIDB, 200, 0) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + assert.Equal(t, 2, report.ServicesRemapped) + assert.Equal(t, int64(2), report.AgentRowsUpdated) + assert.Empty(t, report.DanglingServiceRefs) + + cfg, err := s.GetConfig() + require.NoError(t, err) + byName := map[string]string{} + for _, svc := range cfg.Services { + assert.True(t, model.IsValidId(svc.ID), "service %q ID %q should be a valid Mattermost ID", svc.Name, svc.ID) + byName[svc.Name] = svc.ID + } + assert.Equal(t, byName["A"], getAgentServiceRow(t, s, "agent1").ServiceID) + assert.Equal(t, byName["B"], getAgentServiceRow(t, s, "agent2").ServiceID) + }, + }, + { + name: "mixed IDs only UUIDs rewritten", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "legacy"}, + {ID: "kept26charidkept26charidke", Name: "modern"}, + }, + }, true) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + assert.Equal(t, 1, report.ServicesRemapped) + + cfg, err := s.GetConfig() + require.NoError(t, err) + byName := map[string]string{} + for _, svc := range cfg.Services { + byName[svc.Name] = svc.ID + } + assert.Equal(t, "kept26charidkept26charidke", byName["modern"], "non-UUID ID must be byte-identical") + assert.NotEqual(t, testUUIDA, byName["legacy"]) + assert.True(t, model.IsValidId(byName["legacy"])) + }, + }, + { + name: "fallback reference chain remapped", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A", FallbackServiceID: testUUIDB}, + {ID: testUUIDB, Name: "B", FallbackServiceID: testUUIDC}, + {ID: testUUIDC, Name: "C"}, + }, + }, true) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + assert.Equal(t, 3, report.ServicesRemapped) + assert.Empty(t, report.DanglingServiceRefs) + + cfg, err := s.GetConfig() + require.NoError(t, err) + byName := map[string]llm.ServiceConfig{} + for _, svc := range cfg.Services { + byName[svc.Name] = svc + } + assert.Equal(t, byName["B"].ID, byName["A"].FallbackServiceID) + assert.Equal(t, byName["C"].ID, byName["B"].FallbackServiceID) + assert.Empty(t, byName["C"].FallbackServiceID) + }, + }, + { + name: "config bot references remapped", + seed: func(t *testing.T, s *Store) { + // Pre-legacy-bot-migration state: bots still live in config. + // Proves the migration handles the state reachable only when it + // runs before migrateLegacyConfigBotsToUserAgents. + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A"}, + }, + Bots: []llm.BotConfig{ + {ID: "bot1", Name: "ai", ServiceID: testUUIDA}, + }, + }, true) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + + cfg, err := s.GetConfig() + require.NoError(t, err) + require.Len(t, cfg.Bots, 1) + assert.Equal(t, cfg.Services[0].ID, cfg.Bots[0].ServiceID) + assert.True(t, model.IsValidId(cfg.Bots[0].ServiceID)) + }, + }, + { + name: "duplicate legacy IDs: unique new IDs, references keep first occurrence", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "first"}, + {ID: testUUIDA, Name: "second"}, + {ID: testUUIDB, Name: "other", FallbackServiceID: testUUIDA}, + }, + Bots: []llm.BotConfig{ + {ID: "bot1", Name: "ai", ServiceID: testUUIDA}, + }, + }, true) + seedAgentRow(t, s, "agent1", testUUIDA, 100, 0) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + assert.Equal(t, 3, report.ServicesRemapped, "every occurrence counts, including duplicates") + assert.Empty(t, report.DanglingServiceRefs) + + cfg, err := s.GetConfig() + require.NoError(t, err) + byName := map[string]llm.ServiceConfig{} + for _, svc := range cfg.Services { + assert.True(t, model.IsValidId(svc.ID)) + byName[svc.Name] = svc + } + assert.NotEqual(t, byName["first"].ID, byName["second"].ID, + "duplicated services must each get their own unique new ID") + + // GetServiceByID first-match semantics: all references follow + // the FIRST occurrence's new ID. + firstID := byName["first"].ID + assert.Equal(t, firstID, byName["other"].FallbackServiceID) + assert.Equal(t, firstID, cfg.Bots[0].ServiceID) + assert.Equal(t, firstID, getAgentServiceRow(t, s, "agent1").ServiceID) + }, + }, + { + name: "active and soft-deleted agents both updated without UpdateAt bump", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A"}, + }, + }, true) + seedAgentRow(t, s, "active-agent", testUUIDA, 111, 0) + seedAgentRow(t, s, "deleted-agent", testUUIDA, 222, 999) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + assert.Equal(t, int64(2), report.AgentRowsUpdated) + + cfg, err := s.GetConfig() + require.NoError(t, err) + newID := cfg.Services[0].ID + + activeRow := getAgentServiceRow(t, s, "active-agent") + assert.Equal(t, newID, activeRow.ServiceID) + assert.Equal(t, int64(111), activeRow.UpdateAt) + + deletedRow := getAgentServiceRow(t, s, "deleted-agent") + assert.Equal(t, newID, deletedRow.ServiceID) + assert.Equal(t, int64(222), deletedRow.UpdateAt) + }, + }, + { + name: "dangling references left unchanged and reported", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A", FallbackServiceID: testUUIDDangling}, + }, + Bots: []llm.BotConfig{ + {ID: "bot1", Name: "ai", ServiceID: testUUIDDangling}, + }, + }, true) + seedAgentRow(t, s, "dangling-agent", testUUIDDangling, 100, 0) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + assert.Equal(t, 1, report.ServicesRemapped) + assert.Equal(t, int64(0), report.AgentRowsUpdated) + assert.Len(t, report.DanglingServiceRefs, 3) + + cfg, err := s.GetConfig() + require.NoError(t, err) + assert.Equal(t, testUUIDDangling, cfg.Services[0].FallbackServiceID) + assert.Equal(t, testUUIDDangling, cfg.Bots[0].ServiceID) + assert.Equal(t, testUUIDDangling, getAgentServiceRow(t, s, "dangling-agent").ServiceID) + }, + }, + { + name: "dangling references reported even when no service IDs remapped", + seed: func(t *testing.T, s *Store) { + // Services already carry modern IDs, so the rewrite is a + // no-op — dangling UUID references must still be diagnosed. + // Pre-seed embedded/plugin IDs so those migrations are also + // content no-ops and do not write a config row. + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: "modern26charidmodern26char", Name: "A", FallbackServiceID: testUUIDDangling}, + }, + Bots: []llm.BotConfig{ + {ID: "bot1", Name: "ai", ServiceID: testUUIDDangling}, + }, + MCP: config.MCPConfig{ + EmbeddedServer: config.MCPEmbeddedServerConfig{ID: "embedded26charidembedded26", Enabled: true}, + }, + }, true) + seedAgentRow(t, s, "dangling-agent", testUUIDDangling, 100, 0) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.False(t, report.Migrated, "no config rewrite happened") + assert.Equal(t, 0, report.ServicesRemapped) + assert.Equal(t, int64(0), report.AgentRowsUpdated) + assert.Zero(t, report.EmbeddedPluginServerIDsAssigned) + assert.Len(t, report.DanglingServiceRefs, 3) + + cfg, err := s.GetConfig() + require.NoError(t, err) + assert.Equal(t, testUUIDDangling, cfg.Services[0].FallbackServiceID) + assert.Equal(t, testUUIDDangling, cfg.Bots[0].ServiceID) + assert.Equal(t, testUUIDDangling, getAgentServiceRow(t, s, "dangling-agent").ServiceID) + }, + }, + { + name: "MCP servers get IDs only when missing, other fields preserved", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + MCP: config.MCPConfig{ + Enabled: true, + Servers: []config.MCPServerConfig{ + { + Name: "no-id", + Enabled: true, + BaseURL: "https://one.example.com", + Headers: map[string]string{"Authorization": "Bearer x"}, + ClientID: "client-1", + ToolConfigs: []config.MCPToolConfig{ + {Name: "get_issue", Policy: config.MCPToolPolicyAsk, Enabled: true}, + }, + }, + { + ID: "existing26charidexisting26", + Name: "has-id", + Enabled: false, + BaseURL: "https://two.example.com", + }, + }, + PluginServers: []config.PluginServerConfig{ + {PluginID: "com.example.plugin", Name: "Plugin Server", Enabled: true}, + }, + EmbeddedServer: config.MCPEmbeddedServerConfig{Enabled: true}, + }, + }, true) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + assert.Equal(t, 1, report.MCPServerIDsAssigned) + assert.Equal(t, 2, report.EmbeddedPluginServerIDsAssigned) + + cfg, err := s.GetConfig() + require.NoError(t, err) + require.Len(t, cfg.MCP.Servers, 2) + + noID := cfg.MCP.Servers[0] + assert.True(t, model.IsValidId(noID.ID)) + assert.Equal(t, "no-id", noID.Name) + assert.Equal(t, "https://one.example.com", noID.BaseURL) + assert.Equal(t, map[string]string{"Authorization": "Bearer x"}, noID.Headers) + assert.Equal(t, "client-1", noID.ClientID) + require.Len(t, noID.ToolConfigs, 1) + assert.Equal(t, "get_issue", noID.ToolConfigs[0].Name) + + hasID := cfg.MCP.Servers[1] + assert.Equal(t, "existing26charidexisting26", hasID.ID, "pre-existing ID must be preserved") + + require.Len(t, cfg.MCP.PluginServers, 1) + assert.Equal(t, "com.example.plugin", cfg.MCP.PluginServers[0].PluginID) + assert.True(t, model.IsValidId(cfg.MCP.PluginServers[0].ID)) + assert.True(t, cfg.MCP.EmbeddedServer.Enabled) + assert.True(t, model.IsValidId(cfg.MCP.EmbeddedServer.ID)) + requireMarker(t, s, abacIDMigrationKey) + }, + }, + { + name: "embedded and plugin IDs minted once; existing IDs untouched", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: "remote26charidremote26chari", Name: "remote", BaseURL: "https://one.example.com"}, + }, + EmbeddedServer: config.MCPEmbeddedServerConfig{Enabled: true}, + PluginServers: []config.PluginServerConfig{ + {PluginID: "com.example.a", Name: "A", Path: "/mcp", Enabled: true}, + { + ID: "plugin26charidplugin26char", + PluginID: "com.example.b", + Name: "B", + Path: "/mcp", + Enabled: false, + }, + }, + }, + }, true) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + assert.Zero(t, report.MCPServerIDsAssigned) + assert.Equal(t, 2, report.EmbeddedPluginServerIDsAssigned) + + cfg, err := s.GetConfig() + require.NoError(t, err) + assert.Equal(t, "remote26charidremote26chari", cfg.MCP.Servers[0].ID) + assert.True(t, model.IsValidId(cfg.MCP.EmbeddedServer.ID)) + require.Len(t, cfg.MCP.PluginServers, 2) + assert.True(t, model.IsValidId(cfg.MCP.PluginServers[0].ID)) + assert.Equal(t, "plugin26charidplugin26char", cfg.MCP.PluginServers[1].ID) + requireMarker(t, s, abacIDMigrationKey) + }, + }, + { + name: "second run does not remint embedded/plugin IDs", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + MCP: config.MCPConfig{ + EmbeddedServer: config.MCPEmbeddedServerConfig{Enabled: true}, + PluginServers: []config.PluginServerConfig{ + {PluginID: "com.example.a", Name: "A", Path: "/mcp"}, + }, + }, + }, true) + report, err := s.MigrateABACIDs() + require.NoError(t, err) + require.True(t, report.Migrated) + require.Equal(t, 2, report.EmbeddedPluginServerIDsAssigned) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.False(t, report.Migrated) + assert.Zero(t, report.EmbeddedPluginServerIDsAssigned) + assert.Equal(t, 2, configHistoryCount(t, s), "second run must not write another config row") + + cfg, err := s.GetConfig() + require.NoError(t, err) + assert.True(t, model.IsValidId(cfg.MCP.EmbeddedServer.ID)) + assert.True(t, model.IsValidId(cfg.MCP.PluginServers[0].ID)) + }, + }, + { + name: "service and MCP migrations commit as one config row", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A"}, + }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "srv", BaseURL: "https://one.example.com"}, + }, + }, + }, true) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + assert.Equal(t, 1, report.ServicesRemapped) + assert.Equal(t, 1, report.MCPServerIDsAssigned) + assert.Equal(t, 1, report.EmbeddedPluginServerIDsAssigned, "empty EmbeddedServer.ID is minted") + assert.Equal(t, 2, configHistoryCount(t, s), + "all migrations must write exactly one new config row together") + + cfg, err := s.GetConfig() + require.NoError(t, err) + assert.True(t, model.IsValidId(cfg.Services[0].ID)) + assert.True(t, model.IsValidId(cfg.MCP.Servers[0].ID)) + assert.True(t, model.IsValidId(cfg.MCP.EmbeddedServer.ID)) + requireMarker(t, s, abacIDMigrationKey) + }, + }, + { + name: "idempotency second run is a no-op", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A"}, + }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "srv", BaseURL: "https://one.example.com"}, + }, + EmbeddedServer: config.MCPEmbeddedServerConfig{ID: model.NewId(), Enabled: true}, + }, + }, true) + report, err := s.MigrateABACIDs() + require.NoError(t, err) + require.True(t, report.Migrated) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.False(t, report.Migrated) + assert.Zero(t, report.ServicesRemapped) + assert.Zero(t, report.MCPServerIDsAssigned) + assert.Zero(t, report.EmbeddedPluginServerIDsAssigned) + assert.Equal(t, 2, configHistoryCount(t, s), "second run must not write another config row") + }, + }, + { + name: "content-based no-op sets marker without config write", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: model.NewId(), Name: "modern"}, + }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {ID: model.NewId(), Name: "srv", BaseURL: "https://one.example.com"}, + }, + EmbeddedServer: config.MCPEmbeddedServerConfig{ID: model.NewId(), Enabled: true}, + PluginServers: []config.PluginServerConfig{ + {ID: model.NewId(), PluginID: "com.example.a", Name: "A", Path: "/mcp"}, + }, + }, + }, true) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.False(t, report.Migrated) + assert.Equal(t, 1, configHistoryCount(t, s)) + requireMarker(t, s, abacIDMigrationKey) + }, + }, + { + name: "no active config sets marker without error", + seed: func(t *testing.T, s *Store) {}, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.False(t, report.Migrated) + assert.Zero(t, configHistoryCount(t, s)) + requireMarker(t, s, abacIDMigrationKey) + }, + }, + { + name: "inactive history rows unchanged", + seed: func(t *testing.T, s *Store) { + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDB, Name: "old-snapshot"}, + }, + }, false) + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A"}, + }, + }, true) + }, + validate: func(t *testing.T, s *Store, report ABACIDMigrationReport) { + assert.True(t, report.Migrated) + + var inactiveConfigs []string + require.NoError(t, s.db.Select(&inactiveConfigs, "SELECT Config FROM Agents_ConfigHistory WHERE Active = false ORDER BY CreateAt")) + // The pre-seeded inactive row plus the row deactivated by the migration. + require.Len(t, inactiveConfigs, 2) + assert.Contains(t, inactiveConfigs[0], testUUIDB, "pre-existing inactive row must keep its UUIDs") + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := setupTestStore(t) + require.NoError(t, s.RunMigrations()) + + tt.seed(t, s) + + report, err := s.MigrateABACIDs() + require.NoError(t, err) + + tt.validate(t, s, report) + }) + } +} + +func TestMigrateABACIDsAtomicRollback(t *testing.T) { + tests := []struct { + name string + corrupt func(t *testing.T, s *Store) + }{ + { + name: "agent table missing rolls back config write and marker", + corrupt: func(t *testing.T, s *Store) { + _, err := s.db.Exec("DROP TABLE Agents_UserAgents") + require.NoError(t, err) + }, + }, + { + name: "corrupt active config JSON fails without writes", + corrupt: func(t *testing.T, s *Store) { + _, err := s.db.Exec("UPDATE Agents_ConfigHistory SET Config = 'not-json' WHERE Active = true") + require.NoError(t, err) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := setupTestStore(t) + require.NoError(t, s.RunMigrations()) + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A"}, + }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "srv", BaseURL: "https://one.example.com"}, + }, + }, + }, true) + seedAgentRow(t, s, "agent1", testUUIDA, 100, 0) + + tt.corrupt(t, s) + + _, err := s.MigrateABACIDs() + require.Error(t, err) + + marker, markerErr := s.GetSystemValue(abacIDMigrationKey) + require.NoError(t, markerErr) + assert.Empty(t, marker, "marker must not be set on failure") + + assert.Equal(t, 1, configHistoryCount(t, s), "no new config row on failure") + + var activeConfig string + require.NoError(t, s.db.Get(&activeConfig, "SELECT Config FROM Agents_ConfigHistory WHERE Active = true")) + if activeConfig != "not-json" { + assert.Contains(t, activeConfig, testUUIDA, "active config must keep its UUIDs on rollback") + } + }) + } +} + +func TestMigrateABACIDsConcurrentIdempotent(t *testing.T) { + s := setupTestStore(t) + require.NoError(t, s.RunMigrations()) + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A"}, + {ID: testUUIDB, Name: "B"}, + }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + {Name: "srv", BaseURL: "https://one.example.com"}, + }, + }, + }, true) + seedAgentRow(t, s, "agent1", testUUIDA, 100, 0) + + const goroutines = 8 + reports := make([]ABACIDMigrationReport, goroutines) + errs := make([]error, goroutines) + + var wg sync.WaitGroup + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + reports[idx], errs[idx] = s.MigrateABACIDs() + }(i) + } + wg.Wait() + + migratedCount := 0 + for i := 0; i < goroutines; i++ { + require.NoError(t, errs[i]) + if reports[i].Migrated { + migratedCount++ + } + } + assert.Equal(t, 1, migratedCount, "exactly one goroutine must perform the migration") + assert.Equal(t, 2, configHistoryCount(t, s), "exactly one new config row") + + requireMarker(t, s, abacIDMigrationKey) +} + +// TestUpdateConfigRejectsLegacyUUIDServiceIDs is the post-migration format +// guard: once the ABAC ID migration has run, a save with dashed UUID service +// IDs is rejected as an invalid ID format, while a payload with migrated IDs +// goes through. +func TestUpdateConfigRejectsLegacyUUIDServiceIDs(t *testing.T) { + s := setupTestStore(t) + require.NoError(t, s.RunMigrations()) + + seedConfigRow(t, s, config.Config{ + Services: []llm.ServiceConfig{ + {ID: testUUIDA, Name: "A"}, + }, + }, true) + uuidSnapshot, err := s.GetConfig() + require.NoError(t, err) + + report, err := s.MigrateABACIDs() + require.NoError(t, err) + require.True(t, report.Migrated) + migratedCfg, err := s.GetConfig() + require.NoError(t, err) + migratedID := migratedCfg.Services[0].ID + require.True(t, model.IsValidId(migratedID)) + + // UUID payload after migration: rejected as invalid format, active config untouched. + rowsBefore := configHistoryCount(t, s) + _, err = s.UpdateConfig(func(prev *config.Config) (config.Config, error) { + return *uuidSnapshot, nil + }) + require.ErrorIs(t, err, ErrLegacyUUIDServiceID) + assert.Equal(t, rowsBefore, configHistoryCount(t, s), "rejected save must not write a config row") + current, err := s.GetConfig() + require.NoError(t, err) + assert.Equal(t, migratedID, current.Services[0].ID, "migrated ID must survive the rejected save") + + // A fresh payload (post-migration IDs) is accepted. + fresh := *migratedCfg + fresh.Services[0].Name = "A-renamed" + saved, err := s.UpdateConfig(func(prev *config.Config) (config.Config, error) { + require.NotNil(t, prev) + assert.Equal(t, migratedID, prev.Services[0].ID, "transform must see the migrated config") + return fresh, nil + }) + require.NoError(t, err) + assert.Equal(t, "A-renamed", saved.Services[0].Name) + current, err = s.GetConfig() + require.NoError(t, err) + assert.Equal(t, "A-renamed", current.Services[0].Name) + assert.Equal(t, migratedID, current.Services[0].ID) +} + +// Before the migration marker is set, UpdateConfig must accept UUID service +// IDs: pre-migration saves are legitimate. +func TestUpdateConfigAllowsUUIDsBeforeMigration(t *testing.T) { + s := setupTestStore(t) + require.NoError(t, s.RunMigrations()) + + saved, err := s.UpdateConfig(func(prev *config.Config) (config.Config, error) { + assert.Nil(t, prev, "no active config yet") + return config.Config{ + Services: []llm.ServiceConfig{{ID: testUUIDA, Name: "A"}}, + }, nil + }) + require.NoError(t, err) + assert.Equal(t, testUUIDA, saved.Services[0].ID) + + current, err := s.GetConfig() + require.NoError(t, err) + assert.Equal(t, testUUIDA, current.Services[0].ID) +} diff --git a/webapp/src/client.tsx b/webapp/src/client.tsx index 2c8acd319..024934a63 100644 --- a/webapp/src/client.tsx +++ b/webapp/src/client.tsx @@ -949,7 +949,7 @@ export async function getPluginConfig(): Promise { }); } -export async function savePluginConfig(config: PluginConfig): Promise { +export async function savePluginConfig(config: PluginConfig): Promise { const url = `${baseRoute()}/admin/config`; const response = await fetch(url, Client4.getOptions({ method: 'PUT', @@ -958,7 +958,7 @@ export async function savePluginConfig(config: PluginConfig): Promise { })); if (response.ok) { - return; + return response.json(); } throw new ClientError(Client4.url, { diff --git a/webapp/src/components/system_console/config.test.tsx b/webapp/src/components/system_console/config.test.tsx new file mode 100644 index 000000000..89a61aee6 --- /dev/null +++ b/webapp/src/components/system_console/config.test.tsx @@ -0,0 +1,169 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {act, render, screen} from '@testing-library/react'; +import {IntlProvider} from 'react-intl'; + +import {getAIBots, getPluginConfig, savePluginConfig} from '@/client'; + +import Config from './config'; + +jest.mock('react-intl', () => { + const actual = jest.requireActual('react-intl'); + + // A stable intl instance: config.tsx keys effects on [intl], and a fresh + // object per render would re-run the config load effect forever. + const intl = { + formatMessage: ({defaultMessage}: {defaultMessage: string}) => defaultMessage, + }; + return { + ...actual, + useIntl: () => intl, + FormattedMessage: ({defaultMessage}: {defaultMessage: string}) => defaultMessage, + }; +}); + +jest.mock('@/client', () => ({ + getPluginConfig: jest.fn(), + getAIBots: jest.fn(), + savePluginConfig: jest.fn(), +})); + +// The heavy editors are not under test; expose the identity each entry is +// rendered with so ID propagation from the save response is observable. +jest.mock('./services', () => ({ + __esModule: true, + default: ({services}: {services: Array<{id: string; name: string}>}) => ( +
+ {services.map((s, i) => ( + {`service:${s.name}:${s.id || 'unsaved'}`} + ))} +
+ ), + firstNewService: {id: '', name: 'OpenAI Service'}, +})); + +jest.mock('./mcp_servers', () => ({ + __esModule: true, + default: ({mcpConfig}: {mcpConfig: {servers: Array<{id?: string; name: string}> | null}}) => ( +
+ {(mcpConfig.servers ?? []).map((s, i) => ( + {`mcp:${s.name}:${s.id || 'unsaved'}`} + ))} +
+ ), +})); + +jest.mock('./embedding_search/embedding_search_panel', () => ({ + __esModule: true, + default: () => null, +})); + +jest.mock('./web_search/web_search_panel', () => ({ + __esModule: true, + default: () => null, +})); + +jest.mock('./bots_moved_notice', () => ({ + __esModule: true, + default: () => null, +})); + +jest.mock('./no_services_page', () => ({ + __esModule: true, + default: () =>
{'no-services'}
, +})); + +type SaveAction = () => Promise<{error?: {message?: string}}>; + +const loadedConfig = { + services: [{id: '', name: 'My Service', type: 'openai'}], + mcp: { + enabled: true, + enablePluginServer: false, + servers: [{name: 'Jira', enabled: true, baseURL: 'https://jira.example.com', headers: {}}], + embeddedServer: {enabled: true}, + }, +}; + +function renderConfig() { + const registerSaveAction = jest.fn(); + const props = { + id: 'Config', + label: '', + helpText: null, + value: {} as never, + disabled: false, + config: {}, + currentState: {}, + license: {}, + setByEnv: false, + onChange: jest.fn(), + setSaveNeeded: jest.fn(), + registerSaveAction, + unRegisterSaveAction: jest.fn(), + }; + render( + + + , + ); + return {registerSaveAction}; +} + +describe('Config save flow', () => { + beforeEach(() => { + jest.clearAllMocks(); + (getPluginConfig as jest.Mock).mockResolvedValue(loadedConfig); + (getAIBots as jest.Mock).mockResolvedValue({bots: []}); + }); + + it('adopts server-minted IDs from the save response so ID-gated UI appears without a reload', async () => { + const {registerSaveAction} = renderConfig(); + + // Loaded config renders without IDs. + await screen.findByText('service:My Service:unsaved'); + expect(screen.getByText('mcp:Jira:unsaved')).toBeTruthy(); + + const savedConfig = { + ...loadedConfig, + services: [{id: 'serviceidaaaaaaaaaaaaaaaaa', name: 'My Service', type: 'openai'}], + mcp: { + ...loadedConfig.mcp, + servers: [{...loadedConfig.mcp.servers[0], id: 'mcpserveridbbbbbbbbbbbbbbb'}], + }, + }; + (savePluginConfig as jest.Mock).mockResolvedValue(savedConfig); + + const save = registerSaveAction.mock.calls.at(-1)?.[0] as SaveAction; + let result: {error?: {message?: string}} = {}; + await act(async () => { + result = await save(); + }); + + expect(result).toEqual({}); + expect(savePluginConfig).toHaveBeenCalledTimes(1); + + // The normalized response replaces local state: minted IDs are live. + await screen.findByText('service:My Service:serviceidaaaaaaaaaaaaaaaaa'); + expect(screen.getByText('mcp:Jira:mcpserveridbbbbbbbbbbbbbbb')).toBeTruthy(); + }); + + it('reports a save error and keeps local state when the save is rejected', async () => { + const {registerSaveAction} = renderConfig(); + await screen.findByText('service:My Service:unsaved'); + + (savePluginConfig as jest.Mock).mockRejectedValue(new Error('409')); + + const save = registerSaveAction.mock.calls.at(-1)?.[0] as SaveAction; + let result: {error?: {message?: string}} = {}; + await act(async () => { + result = await save(); + }); + + expect(result.error?.message).toBe('Failed to save configuration.'); + expect(screen.getByText('service:My Service:unsaved')).toBeTruthy(); + expect(screen.getByText('mcp:Jira:unsaved')).toBeTruthy(); + }); +}); diff --git a/webapp/src/components/system_console/config.tsx b/webapp/src/components/system_console/config.tsx index 227e5e72a..e1bbefd8c 100644 --- a/webapp/src/components/system_console/config.tsx +++ b/webapp/src/components/system_console/config.tsx @@ -230,7 +230,12 @@ const Config = (props: Props) => { useEffect(() => { const save = async () => { try { - await savePluginConfig(localConfig); + const saved = await savePluginConfig(localConfig); + + // Adopt the normalized saved config so server-minted + // service/MCP IDs (and the UI gated on them) appear + // immediately instead of after a page reload. + setLocalConfig({...defaultConfig, ...saved}); return {}; } catch (e: any) { return {error: {message: intl.formatMessage({defaultMessage: 'Failed to save configuration.'})}}; @@ -247,13 +252,11 @@ const Config = (props: Props) => { props.setSaveNeeded(); }, [props.setSaveNeeded]); + // No id is assigned client-side: the backend mints the stable service ID + // on save (normalizeAdminConfig). const addFirstService = () => { - const id = crypto.randomUUID(); updateConfig({ - services: [{ - ...firstNewService, - id, - }], + services: [{...firstNewService}], }); }; diff --git a/webapp/src/components/system_console/mcp_servers.test.tsx b/webapp/src/components/system_console/mcp_servers.test.tsx index d8231ffc8..495251d9b 100644 --- a/webapp/src/components/system_console/mcp_servers.test.tsx +++ b/webapp/src/components/system_console/mcp_servers.test.tsx @@ -8,13 +8,26 @@ import {fireEvent, render, screen, waitFor} from '@testing-library/react'; jest.mock('react-intl', () => { const React = require('react'); // eslint-disable-line @typescript-eslint/no-shadow, no-shadow, global-require + const formatMessage = ( + {defaultMessage}: {defaultMessage?: string}, + values?: Record, + ) => { + let message = defaultMessage ?? ''; + if (values) { + for (const [key, value] of Object.entries(values)) { + message = message.replace(new RegExp(`\\{${key}\\}`, 'g'), String(value)); + } + } + return message; + }; + return { __esModule: true, IntlProvider: ({children}: {children: React.ReactNode}) => React.createElement(React.Fragment, null, children), - FormattedMessage: ({defaultMessage}: {defaultMessage?: string}) => - React.createElement(React.Fragment, null, defaultMessage ?? ''), + FormattedMessage: ({defaultMessage, values}: {defaultMessage?: string; values?: Record}) => + React.createElement(React.Fragment, null, formatMessage({defaultMessage}, values)), useIntl: () => ({ - formatMessage: ({defaultMessage}: {defaultMessage?: string}) => defaultMessage ?? '', + formatMessage, }), }; }); @@ -27,6 +40,7 @@ jest.mock('react-bootstrap', () => ({ // The component reads SiteURL via useSelector; null falls back to window.location.origin. jest.mock('react-redux', () => ({ + __esModule: true, useSelector: jest.fn(() => null), })); @@ -42,22 +56,37 @@ jest.mock('../../client', () => ({ updatePluginServer: jest.fn().mockResolvedValue({}), })); +jest.mock('./mcp_tools_viewer', () => ({ + __esModule: true, + default: () => null, +})); + /* eslint-disable import/first, import/order */ import {IntlProvider} from 'react-intl'; import {useIsBasicsLicensed} from '@/license'; -import MCPServers, {MCPConfig, MCPServerConfig} from './mcp_servers'; +import {getMCPTools} from '../../client'; + +import MCPServers, {type MCPConfig, type MCPServerConfig} from './mcp_servers'; +import type {PluginServerConfig} from './mcp_types'; /* eslint-enable import/first, import/order */ const mockUseIsBasicsLicensed = useIsBasicsLicensed as jest.Mock; +const mockGetMCPTools = getMCPTools as jest.Mock; + +const STABLE_ID = 'abcdefghijklmnopqrstuvwxyz'; -function makeMCPConfig(servers: MCPServerConfig[] = []): MCPConfig { +function makeMCPConfig(servers: MCPServerConfig[] = [], embeddedId?: string): MCPConfig { return { enabled: true, enablePluginServer: false, servers, - embeddedServer: {enabled: true, tool_configs: []}, + embeddedServer: { + ...(embeddedId ? {id: embeddedId} : {}), + enabled: true, + tool_configs: [], + }, }; } @@ -86,48 +115,102 @@ function renderServers(mcpConfig: MCPConfig) { }; } -describe('MCPServers license gating', () => { +function lastChangedServers(onChange: jest.Mock): MCPServerConfig[] { + expect(onChange).toHaveBeenCalled(); + const config: MCPConfig = onChange.mock.calls[onChange.mock.calls.length - 1][0]; + return config.servers ?? []; +} + +const existingServer: MCPServerConfig = { + id: STABLE_ID, + name: 'Jira', + enabled: true, + baseURL: 'https://jira.example.com', + headers: {}, +}; + +describe('MCPServers stable ID handling', () => { beforeEach(() => { jest.clearAllMocks(); + + // The remote-server UI these tests drive is behind the license gate. + mockUseIsBasicsLicensed.mockReturnValue(true); + + // Never resolves: these assertions are synchronous, so a resolving + // prefetch would update state outside act(). + mockGetMCPTools.mockReturnValue(new Promise(() => null)); }); - test('unlicensed: remote server UI is hidden and the enterprise chip is shown', async () => { - mockUseIsBasicsLicensed.mockReturnValue(false); + it('preserves the server id through a name edit', () => { + const {onChange} = renderServers(makeMCPConfig([existingServer])); - renderServers(makeMCPConfig()); + fireEvent.click(screen.getByText('Jira')); + const nameInput = screen.getByPlaceholderText('Server name'); + fireEvent.change(nameInput, {target: {value: 'Jira Cloud'}}); + fireEvent.blur(nameInput); - expect(screen.queryByRole('button', {name: /Add Remote MCP Server/})).toBeNull(); - expect(screen.queryByText(/No remote MCP servers configured/)).toBeNull(); - expect(screen.queryByText('MCP OAuth Callback URL')).toBeNull(); - await waitFor(() => { - expect(screen.getByText('Use remote MCP servers on qualifying Mattermost plans')).not.toBeNull(); - }); + const servers = lastChangedServers(onChange); + expect(servers[0].name).toBe('Jira Cloud'); + expect(servers[0].id).toBe(STABLE_ID); }); - test('unlicensed with configured servers: server rows are hidden too', async () => { - mockUseIsBasicsLicensed.mockReturnValue(false); + it('preserves the server id through a URL edit', () => { + const {onChange} = renderServers(makeMCPConfig([existingServer])); - renderServers(makeMCPConfig([makeRemoteServer()])); + const urlInput = screen.getByPlaceholderText('https://mcp.example.com'); + fireEvent.change(urlInput, {target: {value: 'https://jira2.example.com'}}); - expect(screen.queryByText('Jira')).toBeNull(); - expect(screen.queryByRole('button', {name: /Add Remote MCP Server/})).toBeNull(); - await waitFor(() => { - expect(screen.getByText('Use remote MCP servers on qualifying Mattermost plans')).not.toBeNull(); - }); + const servers = lastChangedServers(onChange); + expect(servers[0].baseURL).toBe('https://jira2.example.com'); + expect(servers[0].id).toBe(STABLE_ID); }); - test('licensed: remote server UI is shown and no license UI appears', async () => { - mockUseIsBasicsLicensed.mockReturnValue(true); + it('adds a new server without an id so the backend mints the stable ID on save', () => { + const {onChange} = renderServers(makeMCPConfig([existingServer])); - renderServers(makeMCPConfig([makeRemoteServer()])); + fireEvent.click(screen.getByText('Add Remote MCP Server')); - const addButton = screen.getByRole('button', {name: /Add Remote MCP Server/}); - expect((addButton as HTMLButtonElement).disabled).toBe(false); - expect(screen.getByText('Jira')).not.toBeNull(); - expect(screen.getByText('MCP OAuth Callback URL')).not.toBeNull(); - await waitFor(() => { - expect(screen.queryByText('Use remote MCP servers on qualifying Mattermost plans')).toBeNull(); + const servers = lastChangedServers(onChange); + expect(servers).toHaveLength(2); + expect(servers[1].id).toBeUndefined(); + expect(servers[0].id).toBe(STABLE_ID); + }); + + it('preserves embeddedServer.id through enablePluginServer toggle', () => { + const {onChange} = renderServers(makeMCPConfig([], STABLE_ID)); + + // BooleanItem exposes true/false radios under the enablePluginServer row. + const trueRadios = screen.getAllByDisplayValue('true'); + fireEvent.click(trueRadios[0]); + + expect(onChange).toHaveBeenCalled(); + const config: MCPConfig = onChange.mock.calls[onChange.mock.calls.length - 1][0]; + expect(config.embeddedServer.id).toBe(STABLE_ID); + expect(config.embeddedServer.enabled).toBe(true); + }); + + it('preserves plugin_servers through enablePluginServer toggle', () => { + const pluginServers: PluginServerConfig[] = [{ + id: 'pluginstableidabcdefghijklm', + plugin_id: 'com.example.demo', + name: 'Demo Plugin', + path: '/mcp', + enabled: true, + expose_external: false, + tool_configs: [{name: 'echo', policy: 'ask', enabled: true}], + }]; + const {onChange} = renderServers({ + ...makeMCPConfig([], STABLE_ID), + plugin_servers: pluginServers, }); + + const trueRadios = screen.getAllByDisplayValue('true'); + fireEvent.click(trueRadios[0]); + + expect(onChange).toHaveBeenCalled(); + const config: MCPConfig = onChange.mock.calls[onChange.mock.calls.length - 1][0]; + expect(config.plugin_servers).toEqual(pluginServers); + expect(config.enablePluginServer).toBe(true); }); }); @@ -199,3 +282,49 @@ describe('MCPServers service account headers', () => { expect(screen.getByText(/Do not repeat the header name in the value/)).not.toBeNull(); }); }); + +describe('MCPServers license gating', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockGetMCPTools.mockResolvedValue({servers: []}); + }); + + test('unlicensed: remote server UI is hidden and the enterprise chip is shown', async () => { + mockUseIsBasicsLicensed.mockReturnValue(false); + + renderServers(makeMCPConfig()); + + expect(screen.queryByRole('button', {name: /Add Remote MCP Server/})).toBeNull(); + expect(screen.queryByText(/No remote MCP servers configured/)).toBeNull(); + expect(screen.queryByText('MCP OAuth Callback URL')).toBeNull(); + await waitFor(() => { + expect(screen.getByText('Use remote MCP servers on qualifying Mattermost plans')).not.toBeNull(); + }); + }); + + test('unlicensed with configured servers: server rows are hidden too', async () => { + mockUseIsBasicsLicensed.mockReturnValue(false); + + renderServers(makeMCPConfig([makeRemoteServer()])); + + expect(screen.queryByText('Jira')).toBeNull(); + expect(screen.queryByRole('button', {name: /Add Remote MCP Server/})).toBeNull(); + await waitFor(() => { + expect(screen.getByText('Use remote MCP servers on qualifying Mattermost plans')).not.toBeNull(); + }); + }); + + test('licensed: remote server UI is shown and no license UI appears', async () => { + mockUseIsBasicsLicensed.mockReturnValue(true); + + renderServers(makeMCPConfig([makeRemoteServer()])); + + const addButton = screen.getByRole('button', {name: /Add Remote MCP Server/}); + expect((addButton as HTMLButtonElement).disabled).toBe(false); + expect(screen.getByText('Jira')).not.toBeNull(); + expect(screen.getByText('MCP OAuth Callback URL')).not.toBeNull(); + await waitFor(() => { + expect(screen.queryByText('Use remote MCP servers on qualifying Mattermost plans')).toBeNull(); + }); + }); +}); diff --git a/webapp/src/components/system_console/mcp_servers.tsx b/webapp/src/components/system_console/mcp_servers.tsx index cc4c133b0..473dbdc27 100644 --- a/webapp/src/components/system_console/mcp_servers.tsx +++ b/webapp/src/components/system_console/mcp_servers.tsx @@ -16,43 +16,30 @@ import manifest from '@/manifest'; import {useIsBasicsLicensed} from '@/license'; import {CopyableTextItem} from './copyable_text_item'; -import MCPToolsViewer, {MCPToolsResponse} from './mcp_tools_viewer'; +import MCPToolsViewer from './mcp_tools_viewer'; +import type { + MCPConfig as BaseMCPConfig, + MCPEmbeddedServerConfig as BaseMCPEmbeddedServerConfig, + MCPServerConfig as BaseMCPServerConfig, + MCPToolConfig as BaseMCPToolConfig, + MCPToolsResponse, +} from './mcp_types'; import EnterpriseChip from './enterprise_chip'; import {BooleanItem, ItemList, TextItem} from './item'; -export type MCPToolConfig = { - name: string; - policy: 'auto_run_in_dm' | 'auto_run_everywhere' | 'ask'; - enabled: boolean; - retrieval_description_override?: string; -}; +export type MCPToolConfig = BaseMCPToolConfig; +export type MCPEmbeddedServerConfig = BaseMCPEmbeddedServerConfig; -export type MCPServerConfig = { - name: string; - enabled: boolean; - baseURL: string; - headers: {[key: string]: string}; +export type MCPServerConfig = BaseMCPServerConfig & { // Optional: the backend tag has omitempty, so pre-feature servers omit the key entirely. serviceAccountHeaders?: {[key: string]: string}; - tool_configs?: MCPToolConfig[]; - clientID?: string; - clientSecret?: string; -}; - -export type MCPEmbeddedServerConfig = { - enabled: boolean; - tool_configs?: MCPToolConfig[]; }; -export type MCPConfig = { - enabled: boolean; - enablePluginServer: boolean; - servers: MCPServerConfig[] | null; // server sends nil Go slice as JSON null - embeddedServer: MCPEmbeddedServerConfig; - idleTimeoutMinutes?: number; +export type MCPConfig = Omit & { + servers: MCPServerConfig[] | null; }; type Props = { @@ -167,8 +154,11 @@ const MCPServer = ({ const [serverName, setServerName] = useState(serverConfig.name); const [isOAuthExpanded, setIsOAuthExpanded] = useState(Boolean(serverConfig.clientID)); - // Ensure server config has all required properties + // Ensure server config has all required properties. + // id must be carried through: dropping it here would rotate the server's + // stable ID on every edit (the server backstop mints a new one per save). const config = { + id: serverConfig.id, name: serverConfig.name || '', enabled: serverConfig.enabled ?? false, baseURL: serverConfig.baseURL || '', @@ -449,7 +439,10 @@ const MCPServers = ({mcpConfig, onChange}: Props) => { // MCP client and embedded server are always enabled; users can still // disable individual tools but cannot turn off MCP entirely. + // Spread mcpConfig so fields like plugin_servers and embeddedServer.id + // survive rebuilds that only override a subset of keys. const config: MCPConfig = { + ...mcpConfig, enabled: true, enablePluginServer: mcpConfig?.enablePluginServer ?? false, servers: normalizedServers, @@ -475,7 +468,9 @@ const MCPServers = ({mcpConfig, onChange}: Props) => { return `${prefix}${counter}`; }; - // Add a new server + // Add a new server. No id is assigned client-side: the server treats + // ID-less entries with no identity match as new and mints the stable ID + // on save (client-invented IDs are rejected as fabricated). const addServer = () => { // Use the auto-generated name const serverName = generateServerName(); diff --git a/webapp/src/components/system_console/mcp_types.ts b/webapp/src/components/system_console/mcp_types.ts new file mode 100644 index 000000000..7c68ede74 --- /dev/null +++ b/webapp/src/components/system_console/mcp_types.ts @@ -0,0 +1,75 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +// Types mirror config/mcp_config.go JSON tags. + +export type MCPToolConfig = { + name: string; + policy: 'auto_run_in_dm' | 'auto_run_everywhere' | 'ask'; + enabled: boolean; + retrieval_description_override?: string; +}; + +export type MCPServerConfig = { + id?: string; // stable ABAC policy identity; may be absent until the server-side ID migration runs + name: string; + enabled: boolean; + baseURL: string; + headers: {[key: string]: string}; + tool_configs?: MCPToolConfig[]; + clientID?: string; + clientSecret?: string; +}; + +export type MCPEmbeddedServerConfig = { + id?: string; // stable ABAC policy identity; may be absent until the server-side ID migration runs + enabled: boolean; + tool_configs?: MCPToolConfig[]; +}; + +// Mirrors config.PluginServerConfig (json:"plugin_servers"). +export type PluginServerConfig = { + id?: string; + plugin_id: string; + name: string; + path: string; + enabled: boolean; + expose_external: boolean; + tool_configs?: MCPToolConfig[]; +}; + +export type MCPConfig = { + enabled: boolean; + enablePluginServer: boolean; + servers: MCPServerConfig[] | null; // server sends nil Go slice as JSON null + plugin_servers?: PluginServerConfig[] | null; + embeddedServer: MCPEmbeddedServerConfig; + idleTimeoutMinutes?: number; +}; + +export type MCPToolInfo = { + name: string; + description: string; + inputSchema: Record | null; +}; + +export type MCPServerInfo = { + name: string; + url: string; + tools: MCPToolInfo[]; + needsOAuth: boolean; + oauthURL?: string; + error: string | null; + + // Plugin-server fields; remote and embedded rows read state from mcpConfig. + serverType?: string; + enabled?: boolean; + toolConfigs?: MCPToolConfig[]; + + // Stable ABAC policy identity when present. + id?: string; +}; + +export type MCPToolsResponse = { + servers: MCPServerInfo[]; +}; diff --git a/webapp/src/components/system_console/plugin_config_types.tsx b/webapp/src/components/system_console/plugin_config_types.tsx index 9319f4bab..54859cdd1 100644 --- a/webapp/src/components/system_console/plugin_config_types.tsx +++ b/webapp/src/components/system_console/plugin_config_types.tsx @@ -3,7 +3,7 @@ import {LLMBotConfig} from './bot'; import {EmbeddingSearchConfig} from './embedding_search/types'; -import {MCPConfig} from './mcp_servers'; +import {MCPConfig} from './mcp_types'; import {LLMService} from './service'; import {WebSearchConfig as WebSearchSettings} from './web_search/web_search_panel'; diff --git a/webapp/src/components/system_console/service.tsx b/webapp/src/components/system_console/service.tsx index fe6c1aac8..b857fe145 100644 --- a/webapp/src/components/system_console/service.tsx +++ b/webapp/src/components/system_console/service.tsx @@ -426,7 +426,10 @@ export const ServiceFields = (props: ServiceFieldsProps) => { {intl.formatMessage({defaultMessage: 'No fallback'})} {(props.services ?? []). - filter((s) => s.id !== props.service.id). + + // ID-less entries were added this session and aren't + // addressable as fallbacks until the config is saved. + filter((s) => s.id && s.id !== props.service.id). map((s) => ( { + const actual = jest.requireActual('react-intl'); + return { + ...actual, + useIntl: () => ({ + formatMessage: ({defaultMessage}: {defaultMessage: string}, values?: Record) => { + if (!values) { + return defaultMessage; + } + return Object.entries(values).reduce( + (message, [key, value]) => message.replace(`{${key}}`, String(value)), + defaultMessage, + ); + }, + }), + FormattedMessage: ({defaultMessage}: {defaultMessage: string}) => defaultMessage, + }; +}); + +// The heavy per-service editor is not under test; expose identity and the +// delete hook so list-level behavior is observable. +jest.mock('./service', () => ({ + __esModule: true, + default: ({service, onDelete}: {service: {id: string; name: string}; onDelete: () => void}) => ( +
+ {`service:${service.name}:${service.id || 'unsaved'}`} + +
+ ), +})); + +const persistedService: LLMService = { + id: 'serviceidaaaaaaaaaaaaaaaaa', + name: 'Persisted', + type: 'openai', + apiKey: '', + apiURL: '', + orgId: '', + defaultModel: '', + tokenLimit: 0, + streamingTimeoutSeconds: 0, + outputTokenLimit: 0, + useResponsesAPI: true, + region: '', + awsAccessKeyID: '', + awsSecretAccessKey: '', + vertexProjectID: '', + vertexProjectNumber: '', + vertexAuthCredentials: '', + fallbackServiceID: '', +}; + +function renderServices(services: LLMService[], bots: LLMBotConfig[] = []) { + const onChange = jest.fn(); + render( + + + , + ); + return {onChange}; +} + +describe('Services', () => { + it('adds the first service without a client-side id', () => { + const {onChange} = renderServices([]); + + fireEvent.click(screen.getByText('Add an AI Service')); + + expect(onChange).toHaveBeenCalledTimes(1); + const added = onChange.mock.calls[0][0] as LLMService[]; + expect(added).toHaveLength(1); + expect(added[0].name).toBe('OpenAI Service'); + expect(added[0].id).toBe(''); + }); + + it('appends subsequent services without client-side ids', () => { + const {onChange} = renderServices([persistedService]); + + fireEvent.click(screen.getByText('Add an AI Service')); + + const next = onChange.mock.calls[0][0] as LLMService[]; + expect(next).toHaveLength(2); + expect(next[0]).toBe(persistedService); + expect(next[1].id).toBe(''); + }); + + it('deletes unsaved entries by position, not by (empty) id', () => { + const unsavedA = {...persistedService, id: '', name: 'Unsaved A'}; + const unsavedB = {...persistedService, id: '', name: 'Unsaved B'}; + const {onChange} = renderServices([unsavedA, unsavedB]); + + fireEvent.click(screen.getByText('delete-Unsaved B')); + + expect(onChange).toHaveBeenCalledWith([unsavedA]); + }); + + it('blocks deleting a persisted service that a bot uses', () => { + const bot = {id: 'bot1', name: 'bot', displayName: 'My Bot', serviceID: persistedService.id} as unknown as LLMBotConfig; + const {onChange} = renderServices([persistedService], [bot]); + + fireEvent.click(screen.getByText('delete-Persisted')); + + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByText('Cannot delete this service because it is being used by the following bot(s): My Bot')).toBeTruthy(); + }); + + it('clears dangling fallback links when deleting a persisted service', () => { + const dependent = {...persistedService, id: 'serviceidbbbbbbbbbbbbbbbbb', name: 'Dependent', fallbackServiceID: persistedService.id}; + const {onChange} = renderServices([persistedService, dependent]); + + fireEvent.click(screen.getByText('delete-Persisted')); + + expect(onChange).toHaveBeenCalledWith([{...dependent, fallbackServiceID: ''}]); + }); +}); diff --git a/webapp/src/components/system_console/services.tsx b/webapp/src/components/system_console/services.tsx index 91fc712fa..833e4b0b8 100644 --- a/webapp/src/components/system_console/services.tsx +++ b/webapp/src/components/system_console/services.tsx @@ -49,59 +49,60 @@ const Services = (props: Props) => { const [showErrorDialog, setShowErrorDialog] = useState(false); const [errorMessage, setErrorMessage] = useState(''); + // No id is assigned client-side: the backend mints the stable ID on save + // (normalizeAdminConfig); policy authoring needs a persisted id. const addNewService = (e: React.MouseEvent) => { e.preventDefault(); - const id = crypto.randomUUID(); if (props.services.length === 0) { - props.onChange([{ - ...firstNewService, - id, - }]); + props.onChange([{...firstNewService}]); } else { - props.onChange([...props.services, { - ...defaultNewService, - id, - }]); + props.onChange([...props.services, {...defaultNewService}]); } }; - const onChange = (newService: LLMService) => { - props.onChange(props.services.map((b) => (b.id === newService.id ? newService : b))); + // Entries added this session have no id yet, so services are addressed by + // index rather than by id. + const onChange = (index: number, newService: LLMService) => { + props.onChange(props.services.map((s, i) => (i === index ? newService : s))); }; - const onDelete = (id: string) => { - // Check if any bot is using this service - const botsUsingService = props.bots.filter((bot) => bot.serviceID === id); - - if (botsUsingService.length > 0) { - const botNames = botsUsingService.map((bot) => bot.displayName).join(', '); - const message = intl.formatMessage( - {defaultMessage: 'Cannot delete this service because it is being used by the following bot(s): {botNames}'}, - {botNames}, - ); - setErrorMessage(message); - setShowErrorDialog(true); - return; + const onDelete = (index: number) => { + const id = props.services[index].id; + + // Only persisted services can be referenced by bots or as fallbacks. + if (id) { + const botsUsingService = props.bots.filter((bot) => bot.serviceID === id); + + if (botsUsingService.length > 0) { + const botNames = botsUsingService.map((bot) => bot.displayName).join(', '); + const message = intl.formatMessage( + {defaultMessage: 'Cannot delete this service because it is being used by the following bot(s): {botNames}'}, + {botNames}, + ); + setErrorMessage(message); + setShowErrorDialog(true); + return; + } } // Drop the service and clear any remaining service's fallback link to it, // so deletion never leaves a dangling fallbackServiceID behind. const remaining = props.services. - filter((b) => b.id !== id). - map((b) => (b.fallbackServiceID === id ? {...b, fallbackServiceID: ''} : b)); + filter((_, i) => i !== index). + map((s) => (id && s.fallbackServiceID === id ? {...s, fallbackServiceID: ''} : s)); props.onChange(remaining); }; return ( <> - {props.services.map((service) => ( + {props.services.map((service, index) => ( onDelete(service.id)} + onChange={(updated) => onChange(index, updated)} + onDelete={() => onDelete(index)} /> ))}