diff --git a/api/api.go b/api/api.go index 484e5bcec..f4844e848 100644 --- a/api/api.go +++ b/api/api.go @@ -509,7 +509,10 @@ type AIBotInfo struct { UserIDs []string `json:"userIDs"` EnabledMCPTools []llm.EnabledMCPTool `json:"enabledMCPTools"` AutoEnableNewMCPTools bool `json:"autoEnableNewMCPTools"` - IsDefault bool `json:"isDefault,omitempty"` + // UseServiceAccountAuth is the effective mode, not the raw agent flag: it is + // false when the server is not licensed for remote MCP. + UseServiceAccountAuth bool `json:"useServiceAccountAuth"` + IsDefault bool `json:"isDefault,omitempty"` } type AIBotsResponse struct { @@ -518,6 +521,16 @@ type AIBotsResponse struct { AllowUnsafeLinks bool `json:"allowUnsafeLinks"` } +// usesServiceAccountAuth reports the effective service account mode for a bot: +// on an unlicensed server a service account agent runs in per-user mode, and the +// webapp keys its per-user MCP UI off this value. +func (a *API) usesServiceAccountAuth(bot *bots.Bot) bool { + if a.contextBuilder == nil { + return bot.GetConfig().UseServiceAccountAuth + } + return a.contextBuilder.UsesServiceAccountCatalog(bot) +} + // getAIBotsForUser returns all AI bots available to a user func (a *API) getAIBotsForUser(userID string) ([]AIBotInfo, error) { allBots := a.bots.GetAllBots() @@ -554,6 +567,7 @@ func (a *API) getAIBotsForUser(userID string) ([]AIBotInfo, error) { UserIDs: bot.GetConfig().UserIDs, EnabledMCPTools: bot.GetConfig().EnabledMCPTools, AutoEnableNewMCPTools: bot.GetConfig().AutoEnableNewMCPTools, + UseServiceAccountAuth: a.usesServiceAccountAuth(bot), IsDefault: isDefault, }) if isDefault { diff --git a/api/api_agents.go b/api/api_agents.go index 283cda8a0..daeb05b85 100644 --- a/api/api_agents.go +++ b/api/api_agents.go @@ -24,6 +24,10 @@ import ( var validUsernameRe = regexp.MustCompile(`^[a-z][a-z0-9._-]*$`) +// errServiceAccountAuthRequiresAdmin is returned when a caller without +// PermissionManageSystem tries to turn the service account flag on. +var errServiceAccountAuthRequiresAdmin = errors.New("only system administrators can enable service account authentication for an agent") + // WebsocketEventBotsInvalidate is the event name for PublishWebSocketEvent (webapp: custom_mattermost-ai_). const WebsocketEventBotsInvalidate = "bots_invalidate" @@ -72,6 +76,7 @@ type CreateAgentRequest struct { EnabledMCPTools []llm.EnabledMCPTool `json:"enabledMCPTools"` AutoEnableNewMCPTools bool `json:"autoEnableNewMCPTools"` MCPDynamicToolLoading bool `json:"mcpDynamicToolLoading"` + UseServiceAccountAuth bool `json:"useServiceAccountAuth"` Model string `json:"model"` EnableVision bool `json:"enableVision"` DisableTools bool `json:"disableTools"` @@ -99,6 +104,7 @@ type UpdateAgentRequest struct { EnabledMCPTools []llm.EnabledMCPTool `json:"enabledMCPTools"` AutoEnableNewMCPTools bool `json:"autoEnableNewMCPTools"` MCPDynamicToolLoading bool `json:"mcpDynamicToolLoading"` + UseServiceAccountAuth bool `json:"useServiceAccountAuth"` Model string `json:"model"` EnableVision bool `json:"enableVision"` DisableTools bool `json:"disableTools"` @@ -193,6 +199,13 @@ func canCreateAgent(client *pluginapi.Client, userID string) bool { return client.User.HasPermissionTo(userID, model.PermissionManageSystem) } +// canEnableServiceAccountAuth reports whether userID may turn the service account +// flag on. It grants the agent the admin-provisioned MCP credentials, so enabling +// it is restricted to system admins even when the caller can manage the agent. +func canEnableServiceAccountAuth(client *pluginapi.Client, userID string) bool { + return client.User.HasPermissionTo(userID, model.PermissionManageSystem) +} + // canConfigureAgentServices reports whether userID may list services or fetch models (ManageOwnAgent, ManageOthersAgent, or ManageSystem). func canConfigureAgentServices(client *pluginapi.Client, userID string) bool { if client.User.HasPermissionTo(userID, model.PermissionManageOwnAgent) { @@ -257,6 +270,7 @@ func buildAgentConfigForCreate(req CreateAgentRequest, userID, botUserID string) EnabledMCPTools: req.EnabledMCPTools, AutoEnableNewMCPTools: req.AutoEnableNewMCPTools, MCPDynamicToolLoading: req.MCPDynamicToolLoading, + UseServiceAccountAuth: req.UseServiceAccountAuth, Model: req.Model, EnableVision: req.EnableVision, DisableTools: req.DisableTools, @@ -284,6 +298,7 @@ func applyAgentUpdateRequest(cfg *llm.BotConfig, req UpdateAgentRequest) (displa cfg.EnabledMCPTools = req.EnabledMCPTools cfg.AutoEnableNewMCPTools = req.AutoEnableNewMCPTools cfg.MCPDynamicToolLoading = req.MCPDynamicToolLoading + cfg.UseServiceAccountAuth = req.UseServiceAccountAuth cfg.Model = req.Model cfg.EnableVision = req.EnableVision cfg.DisableTools = req.DisableTools @@ -346,6 +361,11 @@ func (a *API) handleCreateAgent(c *gin.Context) { return } + if req.UseServiceAccountAuth && !canEnableServiceAccountAuth(a.pluginAPI, userID) { + abortAgentRequest(c, http.StatusForbidden, errServiceAccountAuthRequiresAdmin) + return + } + if !validUsernameRe.MatchString(req.Username) { abortAgentRequest(c, http.StatusBadRequest, errors.New("invalid username: must start with a lowercase letter and contain only lowercase letters, numbers, dots, hyphens, or underscores")) return @@ -479,6 +499,13 @@ func (a *API) handleUpdateAgent(c *gin.Context) { return } + // Only the false→true transition escalates; an already-enabled flag was + // admin-granted, so keeping or clearing it needs no extra permission. + if req.UseServiceAccountAuth && !cfg.UseServiceAccountAuth && !canEnableServiceAccountAuth(a.pluginAPI, userID) { + abortAgentRequest(c, http.StatusForbidden, errServiceAccountAuthRequiresAdmin) + return + } + if req.usernameProvided && req.Username != cfg.Name { abortAgentRequest(c, http.StatusBadRequest, errors.New("username cannot be changed after the agent is created")) return diff --git a/api/api_agents_test.go b/api/api_agents_test.go index 475ae6217..d3ccf4ecc 100644 --- a/api/api_agents_test.go +++ b/api/api_agents_test.go @@ -148,6 +148,7 @@ func updateAgentBodyFromStored(cfg *llm.BotConfig, overrides map[string]any) map "enabledMCPTools": cfg.EnabledMCPTools, "autoEnableNewMCPTools": cfg.AutoEnableNewMCPTools, "mcpDynamicToolLoading": cfg.MCPDynamicToolLoading, + "useServiceAccountAuth": cfg.UseServiceAccountAuth, "model": cfg.Model, "enableVision": cfg.EnableVision, "disableTools": cfg.DisableTools, @@ -204,6 +205,8 @@ func TestCreateAgentPersistsExplicitRequestValues(t *testing.T) { mockLicensed(e.mockAPI) e.mockAPI.On("HasPermissionTo", testUserID, model.PermissionManageOwnAgent).Return(true) + // Enabling service account auth is system-admin only. + e.mockAPI.On("HasPermissionTo", testUserID, model.PermissionManageSystem).Return(true) e.mockAPI.On("CreateBot", mock.AnythingOfType("*model.Bot")).Return(&model.Bot{ UserId: "bot-user-id-created", Username: "my-agent", @@ -221,6 +224,7 @@ func TestCreateAgentPersistsExplicitRequestValues(t *testing.T) { "reasoningEnabled": false, "reasoningEffort": "high", "structuredOutputEnabled": false, + "useServiceAccountAuth": true, }) recorder := doRequest(e.api, http.MethodPost, "/agents", body, testUserID) @@ -234,6 +238,8 @@ func TestCreateAgentPersistsExplicitRequestValues(t *testing.T) { assert.Equal(t, "high", agent.ReasoningEffort) assert.False(t, agent.StructuredOutputEnabled) assert.Empty(t, agent.EnabledNativeTools) + assert.True(t, agent.UseServiceAccountAuth) + assert.True(t, e.agentStore.agents[agent.ID].UseServiceAccountAuth) } func TestCreateAgentMaxToolTurnsRoundTrip(t *testing.T) { @@ -590,10 +596,14 @@ func TestUpdateAgentAsCreator(t *testing.T) { stored := &llm.BotConfig{ ID: "agent-1", CreatorID: testUserID, BotUserID: "bot-1", DisplayName: "Original", Name: "original", ServiceID: "svc-1", + UseServiceAccountAuth: true, } e.agentStore.agents["agent-1"] = stored - body := updateAgentBodyFromStored(stored, map[string]any{"displayName": "Updated"}) + body := updateAgentBodyFromStored(stored, map[string]any{ + "displayName": "Updated", + "useServiceAccountAuth": false, + }) // Mock bot patch for display name sync e.mockAPI.On("PatchBot", "bot-1", mock.AnythingOfType("*model.BotPatch")).Return(&model.Bot{}, nil).Maybe() @@ -604,6 +614,8 @@ func TestUpdateAgentAsCreator(t *testing.T) { var agent llm.BotConfig require.NoError(t, json.NewDecoder(recorder.Result().Body).Decode(&agent)) assert.Equal(t, "Updated", agent.DisplayName) + assert.False(t, agent.UseServiceAccountAuth) + assert.False(t, e.agentStore.agents["agent-1"].UseServiceAccountAuth) } func TestUpdateAgentAsAdminUser(t *testing.T) { @@ -671,6 +683,122 @@ func TestUpdateAgentOwnedByOtherWithManageOthersPermission(t *testing.T) { assert.Equal(t, "Admin Renamed", agent.DisplayName) } +// Service account auth hands the agent the admin-provisioned MCP credentials, so +// only system admins may switch it on; anyone who can manage the agent may keep an +// already-granted flag or clear it. +func TestAgentServiceAccountAuthRequiresSystemAdmin(t *testing.T) { + tests := []struct { + name string + create bool + systemAdmin bool + storedValue bool + requestValue bool + expectedStatus int + expectStored bool + }{ + { + name: "non-admin cannot create with service account auth", + create: true, + requestValue: true, + expectedStatus: http.StatusForbidden, + }, + { + name: "admin can create with service account auth", + create: true, + systemAdmin: true, + requestValue: true, + expectedStatus: http.StatusCreated, + expectStored: true, + }, + { + name: "non-admin cannot turn service account auth on", + requestValue: true, + expectedStatus: http.StatusForbidden, + }, + { + name: "admin can turn service account auth on", + systemAdmin: true, + requestValue: true, + expectedStatus: http.StatusOK, + expectStored: true, + }, + { + name: "non-admin can update an agent that already uses service account auth", + storedValue: true, + requestValue: true, + expectedStatus: http.StatusOK, + expectStored: true, + }, + { + name: "non-admin can turn service account auth off", + storedValue: true, + expectedStatus: http.StatusOK, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + e := setupAgentTestEnvironment(t) + defer e.Cleanup(t) + + mockLicensed(e.mockAPI) + e.mockAPI.On("HasPermissionTo", testUserID, model.PermissionManageOwnAgent).Return(true).Maybe() + e.mockAPI.On("HasPermissionTo", testUserID, model.PermissionManageSystem).Return(tc.systemAdmin).Maybe() + e.mockAPI.On("LogError", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return().Maybe() + + if tc.create { + e.mockAPI.On("CreateBot", mock.AnythingOfType("*model.Bot")).Return(&model.Bot{ + UserId: "bot-user-id-created", + Username: "my-agent", + DisplayName: "My Agent", + }, nil).Maybe() + + body := createAgentBody(map[string]any{"useServiceAccountAuth": tc.requestValue}) + recorder := doRequest(e.api, http.MethodPost, "/agents", body, testUserID) + require.Equal(t, tc.expectedStatus, recorder.Result().StatusCode) + + if tc.expectedStatus != http.StatusCreated { + assert.Contains(t, decodeAgentError(t, recorder), "system administrators") + assert.Empty(t, e.agentStore.agents) + return + } + + var agent llm.BotConfig + require.NoError(t, json.NewDecoder(recorder.Body).Decode(&agent)) + assert.Equal(t, tc.expectStored, e.agentStore.agents[agent.ID].UseServiceAccountAuth) + return + } + + stored := &llm.BotConfig{ + ID: "agent-1", CreatorID: testUserID, BotUserID: "bot-1", + DisplayName: "Original", Name: "original", ServiceID: "svc-1", + UseServiceAccountAuth: tc.storedValue, + } + e.agentStore.agents["agent-1"] = stored + e.mockAPI.On("PatchBot", "bot-1", mock.AnythingOfType("*model.BotPatch")).Return(&model.Bot{}, nil).Maybe() + + body := updateAgentBodyFromStored(stored, map[string]any{ + "displayName": "Updated", + "useServiceAccountAuth": tc.requestValue, + }) + recorder := doRequest(e.api, http.MethodPut, "/agents/agent-1", body, testUserID) + require.Equal(t, tc.expectedStatus, recorder.Result().StatusCode) + if tc.expectedStatus == http.StatusForbidden { + assert.Contains(t, decodeAgentError(t, recorder), "system administrators") + } + assert.Equal(t, tc.expectStored, e.agentStore.agents["agent-1"].UseServiceAccountAuth) + }) + } +} + +// decodeAgentError returns the message from a JSON agent error response body. +func decodeAgentError(t *testing.T, recorder *httptest.ResponseRecorder) string { + t.Helper() + var payload agentErrorResponse + require.NoError(t, json.NewDecoder(recorder.Body).Decode(&payload)) + return payload.Error +} + func TestDeleteAgentDeactivatesBot(t *testing.T) { e := setupAgentTestEnvironment(t) defer e.Cleanup(t) diff --git a/api/api_channel_analysis_test.go b/api/api_channel_analysis_test.go index f16a42c5d..28613b112 100644 --- a/api/api_channel_analysis_test.go +++ b/api/api_channel_analysis_test.go @@ -35,6 +35,10 @@ func (p *channelAnalysisMCPProvider) GetToolsForUser(context.Context, string) ([ return p.tools, nil } +func (p *channelAnalysisMCPProvider) GetToolsForServiceAccount(context.Context, string) ([]llm.Tool, *mcp.Errors) { + return nil, nil +} + type channelAnalysisSequenceLLM struct { calls [][]llm.TextStreamEvent requests []llm.CompletionRequest diff --git a/api/api_config_test.go b/api/api_config_test.go index 3dc5262b8..62725ac6e 100644 --- a/api/api_config_test.go +++ b/api/api_config_test.go @@ -377,6 +377,17 @@ func TestSaveAndGetConfigRoundTrip(t *testing.T) { Bots: []llm.BotConfig{ {ID: "bot-1", Name: "ai", ServiceID: "svc-1"}, }, + MCP: config.MCPConfig{ + Servers: []config.MCPServerConfig{ + { + Name: "Jira", + Enabled: true, + BaseURL: "https://jira.example.com", + Headers: map[string]string{"X-Trace": "on"}, + ServiceAccountHeaders: map[string]string{"Authorization": "Bearer service-pat"}, + }, + }, + }, } body, err := json.Marshal(saveCfg) require.NoError(t, err) @@ -404,6 +415,9 @@ func TestSaveAndGetConfigRoundTrip(t *testing.T) { assert.Equal(t, "bot-1", loadedCfg.Bots[0].ID) assert.True(t, loadedCfg.MCP.Enabled) assert.True(t, loadedCfg.MCP.EmbeddedServer.Enabled) + require.Len(t, loadedCfg.MCP.Servers, 1) + assert.Equal(t, map[string]string{"X-Trace": "on"}, loadedCfg.MCP.Servers[0].Headers) + assert.Equal(t, map[string]string{"Authorization": "Bearer service-pat"}, loadedCfg.MCP.Servers[0].ServiceAccountHeaders) // Step 4: Verify side effects assert.Equal(t, 1, updater.callCount) diff --git a/api/api_llm_bridge.go b/api/api_llm_bridge.go index 3cd57d009..07aa121e7 100644 --- a/api/api_llm_bridge.go +++ b/api/api_llm_bridge.go @@ -137,11 +137,7 @@ func (a *API) buildLLMBridgeContext(bot *bots.Bot, req bridgeclient.CompletionRe } else { context = llm.NewContext() if bot != nil { - var botUserID string - if mmBot := bot.GetMMBot(); mmBot != nil { - botUserID = mmBot.UserId - } - context.SetBotFields(bot.GetConfig().DisplayName, bot.GetConfig().Name, botUserID, bot.GetService().DefaultModel, bot.GetService().Type, bot.GetConfig().CustomInstructions) + context.SetBotFields(bot.GetConfig().DisplayName, bot.GetConfig().Name, bot.BotUserID(), bot.GetService().DefaultModel, bot.GetService().Type, bot.GetConfig().CustomInstructions) } } @@ -171,6 +167,10 @@ func (a *API) convertAgentBridgeRequestToInternal(ctx stdcontext.Context, bot *b } bridgeContext := llm.NewContext() + if a.contextBuilder != nil { + // Populate bot identity for token-usage attribution and embedded MCP metadata. + a.contextBuilder.WithLLMContextBot(bot)(bridgeContext) + } bridgeContext.RequestingUser = &model.User{Id: req.UserID} if includeTools && a.contextBuilder != nil { a.contextBuilder.WithLLMContextConcreteTools(ctx, bot)(bridgeContext) diff --git a/api/api_llm_bridge_service_account_test.go b/api/api_llm_bridge_service_account_test.go new file mode 100644 index 000000000..32076f3f2 --- /dev/null +++ b/api/api_llm_bridge_service_account_test.go @@ -0,0 +1,162 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package api + +import ( + "io" + "testing" + + "github.com/gin-gonic/gin" + "github.com/mattermost/mattermost-plugin-agents/v2/llm" + "github.com/mattermost/mattermost-plugin-agents/v2/public/bridgeclient" + "github.com/stretchr/testify/require" +) + +const ( + // Each tool exists in only one catalog, so tests fail if the wrong catalog is in effect. + saToolName = "mattermost__sa_tool" + userToolName = "mattermost__user_tool" +) + +// AutoEnableNewMCPTools keeps every catalog tool in the store so allowed_tools is the only filter. +func (e *TestEnvironment) setupBridgeCatalogBot(useServiceAccountAuth bool) { + e.setupTestBot(llm.BotConfig{ + Name: "testbot", + DisplayName: "Test Bot", + UserAccessLevel: llm.UserAccessLevelAll, + AutoEnableNewMCPTools: true, + UseServiceAccountAuth: useServiceAccountAuth, + }) +} + +func (e *TestEnvironment) setBridgeFakeLLM(fakeLLM *FakeLLM) { + for _, bot := range e.bots.GetAllBots() { + bot.SetLLMForTest(fakeLLM) + } +} + +func TestBridgeAgentCompletionCatalogSelection(t *testing.T) { + gin.SetMode(gin.ReleaseMode) + gin.DefaultWriter = io.Discard + + testCases := []struct { + name string + serviceAccount bool + allowedTool string + wantSACalls []string + wantUserCalls []string + wantToolAuthMode string + }{ + { + name: "service account agent uses the agent bot's catalog", + serviceAccount: true, + allowedTool: saToolName, + wantSACalls: []string{testBotUserID}, + wantToolAuthMode: llm.ToolAuthModeServiceAccount, + }, + { + name: "normal agent uses the requesting user's catalog", + allowedTool: userToolName, + wantUserCalls: []string{testUserID}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := SetupTestEnvironment(t) + defer e.Cleanup(t) + + provider := e.setupBridgeMCPProviderSA( + []llm.Tool{bridgeMCPTool("mattermost", "user_tool", embeddedOrigin)}, + []llm.Tool{bridgeMCPTool("mattermost", "sa_tool", embeddedOrigin)}, + ) + e.setupBridgeCatalogBot(tc.serviceAccount) + + fakeLLM := NewFakeLLM("done") + fakeLLM.StreamEventSequence = fakeLLMAutoRunSequence("tc1", tc.allowedTool, "done") + e.setBridgeFakeLLM(fakeLLM) + + client := e.CreateBridgeClient() + result, err := client.AgentCompletion(testBotUserID, bridgeclient.CompletionRequest{ + Posts: []bridgeclient.Post{{Role: "user", Message: "use the tool"}}, + AllowedTools: []string{tc.allowedTool}, + UserID: testUserID, + }) + require.NoError(t, err) + require.Equal(t, "done", result) + + require.Equal(t, tc.wantSACalls, provider.saCalls) + require.Equal(t, tc.wantUserCalls, provider.userCalls) + + require.Len(t, fakeLLM.AllRequests, 2) + require.Equal(t, 1, findAutoApprovedToolUse(fakeLLM.AllRequests[1], tc.allowedTool)) + + llmContext := fakeLLM.LastRequest().Context + require.NotNil(t, llmContext) + require.Equal(t, testBotUserID, llmContext.BotUserID) + require.Equal(t, "testbot", llmContext.BotUsername) + require.Equal(t, "Test Bot", llmContext.BotName) + require.Equal(t, tc.wantToolAuthMode, llmContext.ToolAuthMode) + require.NotNil(t, llmContext.RequestingUser) + require.Equal(t, testUserID, llmContext.RequestingUser.Id) + }) + } +} + +// With no service-account-credentialed servers the catalog is empty, and never falls +// back to the requesting user's catalog. +func TestBridgeSAAgentCompletionFailsClosedWithoutSAServers(t *testing.T) { + gin.SetMode(gin.ReleaseMode) + gin.DefaultWriter = io.Discard + + e := SetupTestEnvironment(t) + defer e.Cleanup(t) + + // Non-empty user catalog proves fail-closed does not fall back to it. + provider := e.setupBridgeMCPProviderSA( + []llm.Tool{bridgeMCPTool("mattermost", "user_tool", embeddedOrigin)}, + nil, + ) + e.setupBridgeCatalogBot(true) + + fakeLLM := NewFakeLLM("done") + e.setBridgeFakeLLM(fakeLLM) + + client := e.CreateBridgeClient() + _, err := client.AgentCompletion(testBotUserID, bridgeclient.CompletionRequest{ + Posts: []bridgeclient.Post{{Role: "user", Message: "use the tool"}}, + AllowedTools: []string{saToolName}, + UserID: testUserID, + }) + require.Error(t, err) + require.Contains(t, err.Error(), "no eligible tools available for this agent") + require.Empty(t, provider.userCalls, "must not consult the user catalog") + require.Empty(t, fakeLLM.AllRequests, "must not call the LLM") +} + +func TestBridgeGetAgentToolsUsesServiceAccountCatalog(t *testing.T) { + gin.SetMode(gin.ReleaseMode) + gin.DefaultWriter = io.Discard + + e := SetupTestEnvironment(t) + defer e.Cleanup(t) + + provider := e.setupBridgeMCPProviderSA( + []llm.Tool{bridgeMCPTool("mattermost", "user_tool", embeddedOrigin)}, + []llm.Tool{bridgeMCPTool("mattermost", "sa_tool", embeddedOrigin)}, + ) + e.setupBridgeCatalogBot(true) + + client := e.CreateBridgeClient() + tools, err := client.GetAgentTools(testBotUserID, testUserID) + require.NoError(t, err) + + names := make([]string, 0, len(tools)) + for _, tool := range tools { + names = append(names, tool.Name) + } + require.Equal(t, []string{saToolName}, names) + require.Equal(t, []string{testBotUserID}, provider.saCalls) + require.Empty(t, provider.userCalls) +} diff --git a/api/api_llm_bridge_test.go b/api/api_llm_bridge_test.go index e3d2739b2..64068f5b6 100644 --- a/api/api_llm_bridge_test.go +++ b/api/api_llm_bridge_test.go @@ -1316,7 +1316,7 @@ func (e *TestEnvironment) setupMCPWithEligibleTools(t *testing.T, toolNames []st Enabled: true, Servers: []mcp.ServerConfig{ { - Name: "service-account-server", + Name: "static-header-server", Enabled: true, BaseURL: server.URL, Headers: map[string]string{"Authorization": "Bearer test-token"}, @@ -1392,7 +1392,7 @@ func TestBridgeGetAgentToolsReturnsEligibleOnly(t *testing.T) { Enabled: true, Servers: []mcp.ServerConfig{ { - Name: "service-account-server", + Name: "static-header-server", Enabled: true, BaseURL: server.URL, Headers: map[string]string{"Authorization": "Bearer test-token"}, @@ -1708,28 +1708,44 @@ func TestBridgeClientAgentCompletionAllowedToolsEnablesAutoRun(t *testing.T) { require.Len(t, fakeLLM.LastConversation.Context.Tools.GetTools(), 1) } -// fakeBridgeMCPToolProvider is a minimal llmcontext.MCPToolProvider that returns -// a fixed set of (namespaced) MCP tools regardless of user. Unlike +// fakeBridgeMCPToolProvider is a minimal llmcontext.MCPToolProvider with +// separate user-mode and service-account catalogs. Unlike // testLLMContextToolProvider (which feeds the built-in tool path), this exercises // the real MCP path: per-agent allowlist filtering and namespacing. type fakeBridgeMCPToolProvider struct { - tools []llm.Tool + tools []llm.Tool // user-mode catalog + saTools []llm.Tool // service-account catalog (fail-closed subset) + + userCalls []string + saCalls []string } -func (p *fakeBridgeMCPToolProvider) GetToolsForUser(_ context.Context, _ string) ([]llm.Tool, *mcp.Errors) { +func (p *fakeBridgeMCPToolProvider) GetToolsForUser(_ context.Context, userID string) ([]llm.Tool, *mcp.Errors) { + p.userCalls = append(p.userCalls, userID) return p.tools, nil } +func (p *fakeBridgeMCPToolProvider) GetToolsForServiceAccount(_ context.Context, botUserID string) ([]llm.Tool, *mcp.Errors) { + p.saCalls = append(p.saCalls, botUserID) + return p.saTools, nil +} + // setupBridgeMCPProvider wires the context builder with a real MCP tool provider // returning the given namespaced tools, so bridge discovery and completion run // through getToolsStoreForUser (namespacing + EnabledMCPTools filtering). -func (e *TestEnvironment) setupBridgeMCPProvider(tools []llm.Tool) { +func (e *TestEnvironment) setupBridgeMCPProvider(tools []llm.Tool) *fakeBridgeMCPToolProvider { + return e.setupBridgeMCPProviderSA(tools, nil) +} + +func (e *TestEnvironment) setupBridgeMCPProviderSA(userTools, saTools []llm.Tool) *fakeBridgeMCPToolProvider { + provider := &fakeBridgeMCPToolProvider{tools: userTools, saTools: saTools} e.api.contextBuilder = llmcontext.NewLLMContextBuilder( e.client, &testLLMContextToolProvider{}, - &fakeBridgeMCPToolProvider{tools: tools}, + provider, &testLLMContextConfigProvider{}, ) + return provider } // bridgeMCPTool builds a namespaced MCP tool (slug__bare) with the given origin. @@ -2415,7 +2431,7 @@ func TestBridgeClientAgentCompletionRejectsBuiltinToolInAllowedTools(t *testing. Enabled: true, Servers: []mcp.ServerConfig{ { - Name: "service-account-server", + Name: "static-header-server", Enabled: true, BaseURL: server.URL, Headers: map[string]string{"Authorization": "Bearer test-token"}, diff --git a/api/api_no_tools_test.go b/api/api_no_tools_test.go index 04ec3da4d..767512135 100644 --- a/api/api_no_tools_test.go +++ b/api/api_no_tools_test.go @@ -44,6 +44,11 @@ func (p *noToolsTestMCPProvider) GetToolsForUser(context.Context, string) ([]llm return p.tools, nil } +func (p *noToolsTestMCPProvider) GetToolsForServiceAccount(context.Context, string) ([]llm.Tool, *mcp.Errors) { + p.calls++ + return p.tools, nil +} + type noToolsTestContextConfigProvider struct{} func (p *noToolsTestContextConfigProvider) GetServiceByID(string) (llm.ServiceConfig, bool) { diff --git a/api/api_test.go b/api/api_test.go index ebaf0a65e..7a8180fec 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -956,12 +956,14 @@ func TestHandleGetAIBots(t *testing.T) { gin.DefaultWriter = io.Discard tests := []struct { - name string - searchService *search.Search - expectedSearchEnabled bool - expectedAllowUnsafeLinks bool - expectedStatus int - envSetup func(e *TestEnvironment) + name string + searchService *search.Search + useServiceAccountAuth bool + expectedUseServiceAccountAuth bool + expectedSearchEnabled bool + expectedAllowUnsafeLinks bool + expectedStatus int + envSetup func(e *TestEnvironment) }{ { name: "search enabled - non-nil service with non-nil embedding search", @@ -997,16 +999,34 @@ func TestHandleGetAIBots(t *testing.T) { }, }, { - name: "unsafe links enabled via config", - searchService: nil, - expectedSearchEnabled: false, - expectedAllowUnsafeLinks: true, - expectedStatus: http.StatusOK, + // The webapp reads useServiceAccountAuth to hide per-user MCP connect prompts. + name: "unsafe links enabled via config", + searchService: nil, + useServiceAccountAuth: true, + expectedUseServiceAccountAuth: true, + expectedSearchEnabled: false, + expectedAllowUnsafeLinks: true, + expectedStatus: http.StatusOK, envSetup: func(e *TestEnvironment) { e.config.allowUnsafeLinks = true e.mockAPI.On("GetChannelByName", "", mock.AnythingOfType("string"), false).Return(nil, &model.AppError{}) }, }, + { + // Unlicensed servers run service account agents in per-user mode, so the + // response must report the effective mode instead of the raw agent flag. + name: "service account agent reports user mode when unlicensed", + searchService: nil, + useServiceAccountAuth: true, + expectedUseServiceAccountAuth: false, + expectedSearchEnabled: false, + expectedAllowUnsafeLinks: false, + expectedStatus: http.StatusOK, + envSetup: func(e *TestEnvironment) { + e.OverrideLicense(nil) + e.mockAPI.On("GetChannelByName", "", mock.AnythingOfType("string"), false).Return(nil, &model.AppError{}) + }, + }, } for _, test := range tests { @@ -1019,8 +1039,9 @@ func TestHandleGetAIBots(t *testing.T) { // Setup a test bot e.setupTestBot(llm.BotConfig{ - Name: "test-bot", - DisplayName: "Test Bot", + Name: "test-bot", + DisplayName: "Test Bot", + UseServiceAccountAuth: test.useServiceAccountAuth, }) // Setup mock expectations @@ -1047,6 +1068,8 @@ func TestHandleGetAIBots(t *testing.T) { require.Equal(t, test.expectedSearchEnabled, response.SearchEnabled, "SearchEnabled field should match expected value") require.Equal(t, test.expectedAllowUnsafeLinks, response.AllowUnsafeLinks, "AllowUnsafeLinks field should match expected value") require.NotEmpty(t, response.Bots, "Should return at least one bot") + require.Equal(t, test.expectedUseServiceAccountAuth, response.Bots[0].UseServiceAccountAuth, + "UseServiceAccountAuth field should report the effective service account mode") } }) } diff --git a/bots/bot.go b/bots/bot.go index 18470f75e..0e0fed57c 100644 --- a/bots/bot.go +++ b/bots/bot.go @@ -35,6 +35,14 @@ func (b *Bot) GetMMBot() *model.Bot { return b.mmBot } +// BotUserID returns the Mattermost user ID of the bot's user account, or "" when no bot user exists. +func (b *Bot) BotUserID() string { + if b.mmBot == nil { + return "" + } + return b.mmBot.UserId +} + func (b *Bot) LLM() llm.LanguageModel { return b.llm } diff --git a/config/mcp_config.go b/config/mcp_config.go index ef2fa3ffb..4805544ea 100644 --- a/config/mcp_config.go +++ b/config/mcp_config.go @@ -3,6 +3,11 @@ package config +import ( + "net/textproto" + "strings" +) + const ( MCPToolPolicyAsk = "ask" MCPToolPolicyAutoRunInDM = "auto_run_in_dm" @@ -48,13 +53,18 @@ type MCPConfig struct { // MCPServerConfig contains the configuration for a single MCP server type MCPServerConfig struct { - Name string `json:"name"` - Enabled bool `json:"enabled"` - BaseURL string `json:"baseURL"` - Headers map[string]string `json:"headers,omitempty"` - ClientID string `json:"clientID,omitempty"` - ClientSecret string `json:"clientSecret,omitempty"` - ToolConfigs []MCPToolConfig `json:"tool_configs,omitempty"` + Name string `json:"name"` + Enabled bool `json:"enabled"` + BaseURL string `json:"baseURL"` + Headers map[string]string `json:"headers,omitempty"` + + // ServiceAccountHeaders are static headers (e.g. a PAT Authorization header) + // sent in place of per-user OAuth on service-account-mode connections. + ServiceAccountHeaders map[string]string `json:"serviceAccountHeaders,omitempty"` + + ClientID string `json:"clientID,omitempty"` + ClientSecret string `json:"clientSecret,omitempty"` + ToolConfigs []MCPToolConfig `json:"tool_configs,omitempty"` } // GetToolPolicy returns the policy and enabled state for a tool. @@ -101,6 +111,48 @@ func (s *MCPServerConfig) IsToolAutoRunInDM(toolName string) bool { return IsToolPolicyAutoRunInDM(policy) && enabled } +// EffectiveServiceAccountHeaders returns the ServiceAccountHeaders entries with a +// non-blank name and value, trimmed of surrounding whitespace and keyed by their +// canonical MIME form; blank or padded System Console rows would make Go's HTTP +// transport reject the whole request. Because http.Header canonicalizes names on +// the wire, collisions are case-insensitive: entries whose canonical names collide +// are ambiguous, so all of them are dropped instead of letting map iteration order +// pick the winner. +func (s *MCPServerConfig) EffectiveServiceAccountHeaders() map[string]string { + if s == nil { + return nil + } + + counts := make(map[string]int, len(s.ServiceAccountHeaders)) + for name, value := range s.ServiceAccountHeaders { + canonicalName := textproto.CanonicalMIMEHeaderKey(strings.TrimSpace(name)) + if canonicalName == "" || strings.TrimSpace(value) == "" { + continue + } + counts[canonicalName]++ + } + + var headers map[string]string + for name, value := range s.ServiceAccountHeaders { + canonicalName := textproto.CanonicalMIMEHeaderKey(strings.TrimSpace(name)) + trimmedValue := strings.TrimSpace(value) + if canonicalName == "" || trimmedValue == "" || counts[canonicalName] > 1 { + continue + } + if headers == nil { + headers = make(map[string]string, len(counts)) + } + headers[canonicalName] = trimmedValue + } + return headers +} + +// HasServiceAccountAuth reports whether this server has at least one usable +// service account header. Enabled is intentionally ignored. +func (s *MCPServerConfig) HasServiceAccountAuth() bool { + return len(s.EffectiveServiceAccountHeaders()) > 0 +} + // PluginServerConfig describes an MCP server registered by another plugin. type PluginServerConfig struct { PluginID string `json:"plugin_id"` diff --git a/config/mcp_config_test.go b/config/mcp_config_test.go index e96b4c1fa..a14ce3966 100644 --- a/config/mcp_config_test.go +++ b/config/mcp_config_test.go @@ -42,6 +42,126 @@ func TestMCPToolConfigEmptyRetrievalDescriptionOverrideOmitted(t *testing.T) { require.Empty(t, decoded.RetrievalDescriptionOverride) } +// headersFromPairs builds a header map from name/value pairs. Padded names are +// passed as arguments because a map literal cannot hold keys that only differ by +// whitespace without tripping the linter. +func headersFromPairs(pairs ...string) map[string]string { + headers := make(map[string]string, len(pairs)/2) + for i := 0; i+1 < len(pairs); i += 2 { + headers[pairs[i]] = pairs[i+1] + } + return headers +} + +// HasServiceAccountAuth must be true exactly when EffectiveServiceAccountHeaders is non-empty. +func TestMCPServerConfigServiceAccountHeaderFiltering(t *testing.T) { + tests := []struct { + name string + serverConfig *MCPServerConfig + wantHeaders map[string]string + }{ + { + name: "only blank entries", + serverConfig: &MCPServerConfig{Enabled: true, ServiceAccountHeaders: map[string]string{"": "Bearer pat", "Authorization": " "}}, + }, + { + name: "blank entries are dropped and kept entries are trimmed", + serverConfig: &MCPServerConfig{ + Enabled: true, + ServiceAccountHeaders: map[string]string{ + "": "", + " ": "Bearer pat", + "X-Blank-Value": " ", + "Authorization": "Bearer pat", + " 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, + ServiceAccountHeaders: headersFromPairs("Authorization", "Bearer first", " Authorization ", "Bearer second"), + }, + }, + { + name: "colliding names do not drop distinct entries", + serverConfig: &MCPServerConfig{ + Enabled: true, + ServiceAccountHeaders: headersFromPairs("Authorization", "Bearer first", "Authorization ", "Bearer second", "X-Api-Key", "secret-token"), + }, + 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, + ServiceAccountHeaders: map[string]string{"Authorization": "Bearer first", "authorization": "Bearer second"}, + }, + }, + { + name: "case-colliding names do not drop distinct entries", + serverConfig: &MCPServerConfig{ + Enabled: true, + ServiceAccountHeaders: map[string]string{"Authorization": "Bearer first", "authorization": "Bearer second", "X-Api-Key": "secret-token"}, + }, + wantHeaders: map[string]string{"X-Api-Key": "secret-token"}, + }, + { + name: "names are stored in canonical form", + serverConfig: &MCPServerConfig{ + Enabled: true, + ServiceAccountHeaders: map[string]string{"x-api-key": "secret-token"}, + }, + wantHeaders: map[string]string{"X-Api-Key": "secret-token"}, + }, + { + name: "base headers do not count as service account auth", + serverConfig: &MCPServerConfig{ + Enabled: true, + Headers: map[string]string{"Authorization": "Bearer shared"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.wantHeaders, tt.serverConfig.EffectiveServiceAccountHeaders()) + require.Equal(t, len(tt.wantHeaders) > 0, tt.serverConfig.HasServiceAccountAuth()) + }) + } +} + +// A broken JSON tag would make Config.Clone silently drop SA credentials cluster-wide. +func TestConfigClonePreservesServiceAccountHeaders(t *testing.T) { + original := &Config{ + MCP: MCPConfig{ + Servers: []MCPServerConfig{ + { + Name: "Jira", + Enabled: true, + BaseURL: "https://jira.example.com", + Headers: map[string]string{"X-Trace": "on"}, + ServiceAccountHeaders: map[string]string{"Authorization": "Bearer service-pat"}, + }, + }, + }, + } + + // 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) + + clone.MCP.Servers[0].ServiceAccountHeaders["Authorization"] = "Bearer tampered" + require.Equal(t, "Bearer service-pat", original.MCP.Servers[0].ServiceAccountHeaders["Authorization"]) +} + func TestServerConfigGetToolPolicyIgnoresRetrievalOverride(t *testing.T) { serverConfig := &MCPServerConfig{ Enabled: true, diff --git a/conversations/bot_channel_tool_filter_test.go b/conversations/bot_channel_tool_filter_test.go index b8d3888be..e0acbb90a 100644 --- a/conversations/bot_channel_tool_filter_test.go +++ b/conversations/bot_channel_tool_filter_test.go @@ -56,6 +56,10 @@ func (p *channelFollowUpTestMCPToolProvider) GetToolsForUser(context.Context, st return p.tools, nil } +func (p *channelFollowUpTestMCPToolProvider) GetToolsForServiceAccount(context.Context, string) ([]llm.Tool, *mcp.Errors) { + return nil, nil +} + type channelFollowUpTestConfig struct { enableChannelMentionToolCalling bool } diff --git a/conversations/conversations_test.go b/conversations/conversations_test.go index ffae7f4a9..f7714c926 100644 --- a/conversations/conversations_test.go +++ b/conversations/conversations_test.go @@ -50,6 +50,10 @@ func (m *mockMCPClientManager) GetToolsForUser(context.Context, string) ([]llm.T return []llm.Tool{}, nil } +func (m *mockMCPClientManager) GetToolsForServiceAccount(context.Context, string) ([]llm.Tool, *mcp.Errors) { + return []llm.Tool{}, nil +} + type mockConfigProvider struct{} func (m *mockConfigProvider) GetServiceByID(id string) (llm.ServiceConfig, bool) { diff --git a/conversations/dm_conversation_test.go b/conversations/dm_conversation_test.go index 67f28341f..5cc4ea790 100644 --- a/conversations/dm_conversation_test.go +++ b/conversations/dm_conversation_test.go @@ -495,6 +495,10 @@ func (m *testMCPClientManager) GetToolsForUser(context.Context, string) ([]llm.T return m.tools, m.errors } +func (m *testMCPClientManager) GetToolsForServiceAccount(context.Context, string) ([]llm.Tool, *mcp.Errors) { + return nil, nil +} + // --- Test: new DM creates conversation entity and returns stream ---------- func TestDMNewConversation_CreatesConversationAndTurns(t *testing.T) { diff --git a/conversations/handle_messages.go b/conversations/handle_messages.go index a79b68d94..684494554 100644 --- a/conversations/handle_messages.go +++ b/conversations/handle_messages.go @@ -127,7 +127,8 @@ func (c *Conversations) buildConversationContextWithTools( isDMOrGroup := channel != nil && (channel.Type == model.ChannelTypeDirect || channel.Type == model.ChannelTypeGroup) opts := make([]llm.ContextOption, 0, len(extraOpts)+4) - if isDMOrGroup && prefsLogMessage != "" && user != nil { + // Service account agents use one shared catalog; per-user MCP server preferences don't apply. + if isDMOrGroup && prefsLogMessage != "" && user != nil && !c.contextBuilder.UsesServiceAccountCatalog(bot) { opts = append(opts, c.userMCPPreferenceContextOptions(user.Id, prefsLogMessage)...) } opts = append(opts, extraOpts...) diff --git a/conversations/service_account_test.go b/conversations/service_account_test.go new file mode 100644 index 000000000..3ea610335 --- /dev/null +++ b/conversations/service_account_test.go @@ -0,0 +1,140 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package conversations + +import ( + "context" + "encoding/json" + "net/http" + "testing" + + "github.com/mattermost/mattermost-plugin-agents/v2/bots" + "github.com/mattermost/mattermost-plugin-agents/v2/conversation" + "github.com/mattermost/mattermost-plugin-agents/v2/llm" + "github.com/mattermost/mattermost-plugin-agents/v2/llmcontext" + "github.com/mattermost/mattermost-plugin-agents/v2/mmapi/mocks" + "github.com/mattermost/mattermost-plugin-agents/v2/store" + "github.com/mattermost/mattermost-plugin-agents/v2/streaming" + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/plugin/plugintest" + "github.com/mattermost/mattermost/server/public/pluginapi" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const ( + serviceAccountRemoteOrigin = "https://jira.example.com" + serviceAccountBotUserID = "bot-user-id" +) + +// serviceAccountTestBot returns an agent without dynamic MCP tool loading, so provided tools resolve immediately. +func serviceAccountTestBot(useServiceAccount bool) *bots.Bot { + return bots.NewBot( + llm.BotConfig{ + ID: "bot-id", + Name: "matty", + DisplayName: "Matty", + AutoEnableNewMCPTools: true, + UserAccessLevel: llm.UserAccessLevelAll, + ChannelAccessLevel: llm.ChannelAccessLevelAll, + UseServiceAccountAuth: useServiceAccount, + }, + llm.ServiceConfig{DefaultModel: "test-model", Type: llm.ServiceTypeOpenAI}, + &model.Bot{UserId: serviceAccountBotUserID, Username: "matty", DisplayName: "Matty"}, + &loadedStateLLM{}, + ) +} + +// The human initiator approves, but execution resolves against the re-derived SA catalog. +func TestHandleToolCallExecutesFromServiceAccountCatalog(t *testing.T) { + executed := 0 + saTool := channelFollowUpTestMCPTool("sa_jira__get_issue", serviceAccountRemoteOrigin, "service account Jira") + saTool.Resolver = func(_ context.Context, _ *llm.Context, _ llm.ToolArgumentGetter) (string, error) { + executed++ + return "mcp:sa_jira__get_issue", nil + } + provider := &countingMCPToolProvider{ + // A different Jira tool, so the assertions cannot pass by resolving the wrong catalog. + tools: []llm.Tool{channelFollowUpTestMCPTool("jira__get_issue", serviceAccountRemoteOrigin, "user OAuth Jira")}, + saTools: []llm.Tool{saTool}, + } + + convStore := newLoadedStateFlowStore() + conv := &store.Conversation{ + ID: "conv-id", + UserID: "user-id", + BotID: serviceAccountBotUserID, + SystemPrompt: "system", + Operation: "conversation", + } + require.NoError(t, convStore.CreateConversation(conv)) + + blocks := []conversation.ContentBlock{{ + Type: conversation.BlockTypeToolUse, + ID: "tool-use-1", + Name: "sa_jira__get_issue", + ServerOrigin: serviceAccountRemoteOrigin, + Input: json.RawMessage(`{}`), + Status: conversation.StatusPending, + }} + content, err := json.Marshal(blocks) + require.NoError(t, err) + approvalPostID := "approval-post-id" + require.NoError(t, convStore.CreateTurn(&store.Turn{ + ID: "assistant-turn", + ConversationID: conv.ID, + PostID: &approvalPostID, + Role: "assistant", + Content: content, + Sequence: 1, + })) + + c := serviceAccountConversations(t, convStore, provider) + + approvalPost := &model.Post{Id: approvalPostID, UserId: serviceAccountBotUserID} + approvalPost.AddProp(streaming.ConversationIDProp, conv.ID) + channel := &model.Channel{Id: "channel-id", TeamId: "team-id", Type: model.ChannelTypeOpen} + + require.NoError(t, c.HandleToolCall(context.Background(), "user-id", approvalPost, channel, []string{"tool-use-1"}, nil)) + + require.Equal(t, []string{serviceAccountBotUserID}, provider.SAIdentities(), + "the approval resume must re-derive the catalog for the agent bot identity") + require.Equal(t, 0, provider.Calls(), "service account agents never build the per-user catalog") + require.Equal(t, 1, executed, "the service account resolver must run exactly once") + + turns, err := convStore.GetTurnsForConversation(conv.ID) + require.NoError(t, err) + require.Len(t, turns, 2) + + var updatedBlocks []conversation.ContentBlock + require.NoError(t, json.Unmarshal(turns[0].Content, &updatedBlocks)) + require.Equal(t, conversation.StatusSuccess, updatedBlocks[0].Status) + + var resultBlocks []conversation.ContentBlock + require.NoError(t, json.Unmarshal(turns[1].Content, &resultBlocks)) + require.Equal(t, conversation.BlockTypeToolResult, resultBlocks[0].Type) + require.Equal(t, "mcp:sa_jira__get_issue", resultBlocks[0].Content) +} + +func serviceAccountConversations(t *testing.T, convStore *loadedStateFlowStore, provider llmcontext.MCPToolProvider) *Conversations { + t.Helper() + + mockAPI := &plugintest.API{} + pluginAPI := pluginapi.NewClient(mockAPI, nil) + licenseChecker := toolLicenseChecker(t, true) + botsService := bots.New(mockAPI, pluginAPI, licenseChecker, nil, nil, &http.Client{}, nil) + botsService.SetBotsForTesting([]*bots.Bot{serviceAccountTestBot(true)}) + + mmClient := mocks.NewMockClient(t) + mmClient.On("LogDebug", mock.Anything, mock.Anything).Maybe().Return() + mmClient.On("GetUser", "user-id").Return(&model.User{Id: "user-id", Username: "user"}, nil).Maybe() + + return &Conversations{ + mmClient: mmClient, + contextBuilder: newSingleBuildLLMContextBuilder(t, provider), + bots: botsService, + licenseChecker: licenseChecker, + convService: conversation.NewService(convStore, nil, nil, nil), + } +} diff --git a/conversations/single_build_test.go b/conversations/single_build_test.go index 468bd66ac..78e1ad8a3 100644 --- a/conversations/single_build_test.go +++ b/conversations/single_build_test.go @@ -5,12 +5,15 @@ package conversations import ( "context" + "sync" "sync/atomic" "testing" + "github.com/mattermost/mattermost-plugin-agents/v2/bots" "github.com/mattermost/mattermost-plugin-agents/v2/llm" "github.com/mattermost/mattermost-plugin-agents/v2/llmcontext" "github.com/mattermost/mattermost-plugin-agents/v2/mcp" + "github.com/mattermost/mattermost-plugin-agents/v2/mmapi/mocks" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/plugin/plugintest" "github.com/mattermost/mattermost/server/public/pluginapi" @@ -22,8 +25,12 @@ import ( // so single-build refactors can assert there is no second pipeline pass per // message. type countingMCPToolProvider struct { - calls int32 - tools []llm.Tool + calls int32 + tools []llm.Tool + saTools []llm.Tool + + mu sync.Mutex + saIdentities []string } func (p *countingMCPToolProvider) GetToolsForUser(context.Context, string) ([]llm.Tool, *mcp.Errors) { @@ -31,10 +38,23 @@ func (p *countingMCPToolProvider) GetToolsForUser(context.Context, string) ([]ll return append([]llm.Tool(nil), p.tools...), nil } +func (p *countingMCPToolProvider) GetToolsForServiceAccount(_ context.Context, botUserID string) ([]llm.Tool, *mcp.Errors) { + p.mu.Lock() + p.saIdentities = append(p.saIdentities, botUserID) + p.mu.Unlock() + return append([]llm.Tool(nil), p.saTools...), nil +} + func (p *countingMCPToolProvider) Calls() int { return int(atomic.LoadInt32(&p.calls)) } +func (p *countingMCPToolProvider) SAIdentities() []string { + p.mu.Lock() + defer p.mu.Unlock() + return append([]string(nil), p.saIdentities...) +} + func newSingleBuildLLMContextBuilder(t *testing.T, mcpProvider llmcontext.MCPToolProvider) *llmcontext.Builder { t.Helper() @@ -82,18 +102,62 @@ func TestBuildConversationContextWithTools_MentionShapeBuildsOnce(t *testing.T) // TestBuildConversationContextWithTools_DMShapeBuildsOnce mirrors the DM path: // the helper applies user MCP preferences (DM/group) and builds tools once. +// Service account agents share one catalog, so their preferences never apply. func TestBuildConversationContextWithTools_DMShapeBuildsOnce(t *testing.T) { - provider := &countingMCPToolProvider{} - builder := newSingleBuildLLMContextBuilder(t, provider) - - c := &Conversations{contextBuilder: builder} - bot := loadedStateBot(nil) - user := &model.User{Id: "user-id", Username: "user"} - channel := &model.Channel{Id: "dm-channel", Type: model.ChannelTypeDirect, Name: "bot-id__user-id"} + const disabledOrigin = "https://jira.example.com" - llmCtx := c.buildConversationContextWithTools(context.Background(), bot, user, channel, "Failed to load user tool preferences") - require.NotNil(t, llmCtx) - require.Equal(t, 1, provider.Calls(), "DM build should call GetToolsForUser exactly once") + tests := []struct { + name string + bot *bots.Bot + wantPrefsLoaded bool + wantUserCalls int + wantSAIdentities []string + wantDisabledOrigins []string + }{ + { + name: "normal agent applies per-user server preferences", + bot: loadedStateBot(nil), + wantPrefsLoaded: true, + wantUserCalls: 1, + wantDisabledOrigins: []string{disabledOrigin}, + }, + { + name: "service account agent ignores per-user server preferences", + bot: serviceAccountTestBot(true), + wantSAIdentities: []string{serviceAccountBotUserID}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + provider := &countingMCPToolProvider{} + + // A missing expectation fails the test if the preferences are read anyway. + mmClient := mocks.NewMockClient(t) + if tc.wantPrefsLoaded { + mmClient.On("KVGet", "user_tool_providers_user-id", mock.AnythingOfType("*mcp.UserToolProviderPreferences")). + Run(func(args mock.Arguments) { + prefs := args.Get(1).(*mcp.UserToolProviderPreferences) + prefs.DisabledServers = []string{disabledOrigin} + }). + Return(nil). + Once() + } + + c := &Conversations{ + mmClient: mmClient, + contextBuilder: newSingleBuildLLMContextBuilder(t, provider), + } + user := &model.User{Id: "user-id", Username: "user"} + channel := &model.Channel{Id: "dm-channel", Type: model.ChannelTypeDirect, Name: "bot-id__user-id"} + + llmCtx := c.buildConversationContextWithTools(context.Background(), tc.bot, user, channel, "Failed to load user tool preferences") + require.NotNil(t, llmCtx) + require.Equal(t, tc.wantUserCalls, provider.Calls(), "per-user catalog builds") + require.Equal(t, tc.wantSAIdentities, provider.SAIdentities(), "service account catalog builds") + require.ElementsMatch(t, tc.wantDisabledOrigins, llmCtx.ToolCatalog.DisabledMCPServerOrigins) + }) + } } // TestBuildConversationContextWithTools_DoesNotMaterializeDynamicMCPTools pins diff --git a/docs/admin_guide.md b/docs/admin_guide.md index 487ee4957..fc001395e 100644 --- a/docs/admin_guide.md +++ b/docs/admin_guide.md @@ -333,6 +333,8 @@ The Agents plugin can track token usage for all LLM interactions to support bill - **User ID**: The Mattermost user who initiated the request - **Team ID**: The team context for the request - **Bot Username**: Which agent was used for the interaction +- **Acting User ID**: The identity tool calls acted as — the requesting user, or the agent's bot user ID when the agent uses service account authentication +- **Tool Auth Mode**: `user` or `service_account`, recording which credential mode built the request's tool catalog - **Input Tokens**: Number of tokens in the request to the LLM - **Output Tokens**: Number of tokens in the LLM response - **Total Tokens**: Combined input and output token count @@ -356,8 +358,8 @@ jq -s '.' logs/agents/token_usage.log > token_usage.json **Convert to CSV format:** ```bash -echo "timestamp,user_id,team_id,bot_username,input_tokens,output_tokens,total_tokens,cached_read_tokens,cached_write_tokens,reasoning_tokens,cost" > token_usage.csv -jq -r '[.timestamp, .user_id, .team_id, .bot_username, .input_tokens, .output_tokens, .total_tokens, (.cached_read_tokens // 0), (.cached_write_tokens // 0), (.reasoning_tokens // 0), (.cost // 0)] | @csv' logs/agents/token_usage.log >> token_usage.csv +echo "timestamp,user_id,acting_user_id,tool_auth_mode,team_id,bot_username,input_tokens,output_tokens,total_tokens,cached_read_tokens,cached_write_tokens,reasoning_tokens,cost" > token_usage.csv +jq -r '[.timestamp, .user_id, .acting_user_id, .tool_auth_mode, .team_id, .bot_username, .input_tokens, .output_tokens, .total_tokens, (.cached_read_tokens // 0), (.cached_write_tokens // 0), (.reasoning_tokens // 0), (.cost // 0)] | @csv' logs/agents/token_usage.log >> token_usage.csv ``` ### Post indexing @@ -555,12 +557,12 @@ Remote and external MCP servers require a license (see [license requirements](#l - **Enable Mattermost MCP Server (HTTP)**: Optional HTTP endpoint for external MCP clients. See [Mattermost MCP Server](#mattermost-mcp-server). - **Connection Idle Timeout (minutes)**: Timeout for inactive user MCP connections (default: 30 minutes). - - Remote MCP servers, including URL, custom headers, OAuth client settings, and per-server enablement. + - Remote MCP servers, including URL, custom headers, OAuth client settings, service account headers, and per-server enablement. 3. Use the **Tools** tab to review discovered tools and set each tool's enabled state and approval policy. Expand a tool row to add an optional **Retrieval description override** for dynamic tool loading search; this helps the agent find the tool but does not change the tool schema sent after loading. Plugin-registered MCP servers appear as separate plugin rows in this tab. 4. When creating or editing an agent on the **Agents** page, use the **MCPs** tab to choose whether that agent can use all MCP tools automatically or only a selected set of tools, and whether MCP tool schemas are loaded dynamically or exposed up front. -Agent MCP access is filtered by admin tool policy, the agent's MCP allowlist or **Automatically enable all MCP tools** setting, user-disabled provider preferences, and any context restrictions for the current request. +Agent MCP access is filtered by admin tool policy, the agent's MCP allowlist or **Automatically enable all MCP tools** setting, user-disabled provider preferences, and any context restrictions for the current request. For agents using [service account authentication](#service-account-authentication), user-disabled provider preferences don't apply. The **Tools** tab refreshes automatically after the current user connects or disconnects an OAuth-backed MCP server. Because MCP OAuth connections are per-user, this live refresh applies only to the user who completed the connect or disconnect action. @@ -574,6 +576,7 @@ You can't disable MCP entirely from the System Console. To limit access, disable - **Server URL**: The endpoint URL for your MCP server. - **Custom Headers**: Additional headers required by your MCP server (optional). + - **Service Account Authentication**: Static headers used in place of per-user OAuth by agents with **Use service accounts for authentication** enabled (optional). See [Service account authentication](#service-account-authentication). - **Server Name**: Descriptive name for the server (auto-generated if not provided). 3. Select **Save** to add the server. @@ -600,6 +603,26 @@ The **Automatically enable all MCP tools** option remains the broadest setting. Enabling a server or tool for an agent controls what the agent is allowed to use, but it does not bypass tool approval policies. Tool execution still follows the policy configured in the **Tools** tab and each user's Mattermost and provider permissions. +### Service account authentication + +By default, MCP tool calls run with the credentials of the user who triggered the agent: per-user OAuth on external MCP servers, and the requesting user's own identity for embedded Mattermost tools. An agent can instead be switched to **service account authentication**, where every MCP tool call uses shared, admin-configured credentials. + +To set it up: + +1. Navigate to **System Console > Plugins > Agents > Model Context Protocol (MCP)**, open the remote MCP server on the **Configuration** tab, and add the static headers the server should receive from service account agents in the **Service Account Authentication** section — for example, an `Authorization` header carrying a personal access token. Rows with a blank name or value are ignored, so a server whose entries are all blank counts as having no service account credentials. Select **Save**. +2. On the agent's **MCPs** tab (Agents page), turn on **Use service accounts for authentication**. Only system administrators can turn this setting on; anyone who can manage the agent can turn it off. + +The agent setting is all-or-nothing: + +- **External MCP servers**: connections send the server's service account headers in place of per-user OAuth. The server's custom headers are still sent in both modes; when both define the same header, the service account value wins. Servers without service account headers are excluded from this agent entirely — it fails closed, with no fallback to any user's personal OAuth connection. To mix personal and service account access, use two agents or separate MCP server entries. +- **Embedded Mattermost tools and plugin-registered MCP servers**: tool calls run as the agent's **bot account** instead of the requesting user, so the bot's channel membership is the internal access boundary — reading posts, searching, and listing channel members return what the bot can read. Add the bot only to channels the agent should read. +- Users are never prompted to connect accounts for this agent, per-user tool provider preferences don't apply, and the Agents RHS **Tools** popover is hidden. +- Tool approval is unchanged: the person who triggered the agent still approves or rejects tool calls according to the configured tool policies. See [Multiplayer Tool Calling](features/multiplayer_tool_calling.md). + +> **Warning:** Service account authentication flattens permissions — **every user who can use the agent acts with the agent's shared access**. Restrict who can use the agent on its **Access** tab, and prefer a dedicated service account (and MCP server entry) per integration, scoped to the minimum permissions the agent needs. External systems attribute the agent's actions to the service account, not to the Mattermost user who triggered them; to correlate, enable [token usage tracking](#token-usage-tracking), where each record carries the triggering user (`user_id`), the acting identity (`acting_user_id`), and the auth mode (`tool_auth_mode`). Header values are stored in the plugin configuration and are visible to system admins, like the server's other credentials. + +Service account authentication requires a license, the same as remote and external MCP servers (see [license requirements](#license-requirements)). Without a license, the service account header configuration is not shown and the agent setting doesn't change how tool calls authenticate. + ### MCP dynamic tool loading When an agent's MCP dynamic tool loading setting is enabled, the model doesn't receive every full MCP tool schema at once. Instead, it sees `search_tools` and `load_tool` meta-tools, plus any preloaded or internal tools available for that request. @@ -608,7 +631,7 @@ When an agent's MCP dynamic tool loading setting is enabled, the model doesn't r After a tool is loaded successfully, it remains available for the rest of the conversation. On later turns, loaded tools are restored from retained conversation history when they are still authorized and available. -The `search_tools` and `load_tool` meta-tools run automatically without approval. The loaded business tool still follows its configured approval policy and the user's Mattermost and provider permissions. +The `search_tools` and `load_tool` meta-tools run automatically without approval. The loaded business tool still follows its configured approval policy and the user's Mattermost and provider permissions — or, for agents using [service account authentication](#service-account-authentication), the agent's bot account and service account permissions. Dynamic loading applies to normal agent conversation turns. Bridge integrations and direct tool catalog requests can still request concrete tool schemas directly. @@ -616,9 +639,9 @@ Dynamic loading applies to normal agent conversation turns. Bridge integrations - **Connection Management**: The system automatically manages user connections to MCP servers - **Idle Cleanup**: Inactive client connections are automatically closed after the configured timeout -- **Per-User Connections**: Each user gets their own connection to MCP servers for security and isolation +- **Per-User Connections**: Each user gets their own connection to MCP servers for security and isolation. Agents using service account authentication instead share one connection per agent, keyed to the agent's bot account - **Tool Policies**: Use the **Tools** tab to allow, require approval for, or disable individual tools, and to add optional retrieval description overrides used by dynamic tool loading search -- **Agent Scoping**: The RHS **Tools** popover only shows MCP providers allowed for the selected agent. Tool use is still subject to admin tool policies and the user's Mattermost permissions +- **Agent Scoping**: The RHS **Tools** popover only shows MCP providers allowed for the selected agent, and is hidden entirely for agents using service account authentication (there are no per-user connections or preferences to manage). Tool use is still subject to admin tool policies and the user's Mattermost permissions ### OAuth-backed MCP servers diff --git a/docs/features/managing_agents.md b/docs/features/managing_agents.md index f69c4a3b5..bcf21ecf4 100644 --- a/docs/features/managing_agents.md +++ b/docs/features/managing_agents.md @@ -131,6 +131,7 @@ The MCPs tab is available only when **Enable Tools** is on (Configuration tab). - **Automatically enable all MCP tools**: the agent has access to every MCP tool available in the server right now and to any MCP tools added later. This is the default for new agents and the setting used by all migrated legacy bots. - When the auto-grant is off, pick the specific MCP tools to enable. Tools that are no longer present on the server are dropped from the agent's allowlist when you save. - For OAuth-backed MCP servers, you can also start the per-user **Connect** flow directly from this tab. Enabling a server that is currently disconnected stores a wildcard grant — once you finish the OAuth flow, the agent gets every tool that server exposes. The tab refreshes automatically when you connect or disconnect (`mcp_connection_updated` websocket event). +- **Use service accounts for authentication** switches the agent from per-user credentials to admin-configured service account credentials for all MCP access. External MCP servers without service account headers configured in the System Console are excluded from the agent (fail closed), embedded Mattermost tools run as the agent's bot account, and users are never asked to connect accounts. Turning it on shows a warning because it flattens permissions — anyone who can use the agent acts with the agent's shared access — so restrict usage on the **Access** tab and only add the bot account to channels it should read. Requires the same license as remote MCP servers. See [Service account authentication](../admin_guide.md#service-account-authentication) in the Admin Guide. Dynamic tool loading and the auto-grant are separate controls: dynamic loading decides when schemas are shown to the model, while the auto-grant decides which MCP tools the agent is allowed to use. diff --git a/docs/features/multiplayer_tool_calling.md b/docs/features/multiplayer_tool_calling.md index 030f812bf..f1979b91c 100644 --- a/docs/features/multiplayer_tool_calling.md +++ b/docs/features/multiplayer_tool_calling.md @@ -22,7 +22,7 @@ Every tool call in a channel involves up to four distinct roles. These terms are |---|---| | **Initiator** | The human user who triggered the Agent — for example, by `@`-mentioning the bot or by sending a message in an Agents-pane thread. The initiator is recorded as `UserID` on the conversation row in the `LLM_Conversations` DB table. | | **Approver** | The user permitted to click **Accept** or **Reject** on a pending tool-call card. In multiplayer tool calling, **the approver is always the initiator** — never another channel member, never an admin. | -| **Executor** | The identity Mattermost uses when actually running the tool — opening the HTTP request, hitting the MCP server, reading channel posts, etc. The executor inherits the initiator's user identity and the initiator's per-user OAuth tokens for OAuth-backed MCP servers. | +| **Executor** | The identity Mattermost uses when actually running the tool — opening the HTTP request, hitting the MCP server, reading channel posts, etc. For agents using per-user authentication (the default), the executor inherits the initiator's user identity and the initiator's per-user OAuth tokens for OAuth-backed MCP servers. For agents configured with **service account authentication**, the executor is the agent itself: admin-configured service account credentials on external MCP servers and the agent's bot account inside Mattermost (§3). | | **Observer** (or **onlooker**) | Any other channel member who can read the channel post containing the Agent's response, but who did not trigger it. Observers are read-only with respect to the tool call: they cannot approve, cannot reject, and (by default) cannot see tool arguments or private results. | Concrete example: Maya `@`-mentions `@copilot` in `~team-eng` and asks it to look up a Jira ticket. Raj is also in `~team-eng` and watches the conversation unfold. @@ -36,21 +36,36 @@ These roles do not change mid-conversation. If Maya hands the keyboard to a team ## 3. The credential model +Every agent runs its tools in one of two authentication modes. The mode is a property of the agent — set by whoever manages the agent, on its **MCPs** tab — not a per-call choice. + +### Per-user authentication (the default) + When a tool runs after approval, Mattermost executes it **as the initiator**. Two specific things this means: 1. **Mattermost-side identity:** the user context passed into tool execution is built from the initiator's user record, channel membership, and team membership. Tools that read channel posts, search users, or call back into Mattermost see only what the initiator is permitted to see. 2. **OAuth-backed MCP servers:** for any MCP server that requires per-user OAuth, the OAuth token loaded for the call is the initiator's token, looked up by the initiator's user ID and the server name. -The Agent bot's own credentials are **not** used to run third-party tools. The bot is a delivery surface — it posts the response and the tool cards — but the side effects of tool execution belong to the initiator. This is intentional: it keeps audit trails meaningful, it prevents privilege escalation through the bot, and it makes per-user OAuth scopes the right place to enforce who can do what. +The Agent bot's own credentials are **not** used to run third-party tools in this mode. The bot is a delivery surface — it posts the response and the tool cards — but the side effects of tool execution belong to the initiator. This is intentional: it keeps audit trails meaningful, it prevents privilege escalation through the bot, and it makes per-user OAuth scopes the right place to enforce who can do what. -A few consequences fall out of this model: +A few consequences fall out of this mode: - Two different users in the same channel running the same tool against the same MCP server may get different results, because they have different OAuth scopes. That is correct behavior, not a bug. - A tool call that succeeds for one initiator may fail for another with a 401 or 403. OAuth 401 responses from MCP servers are caught and wrapped as `OAuthNeededError` in the client, surfacing as a tool auth error that prompts the initiator to re-authenticate. -- Service accounts are not a concept in this model. There is no "run this tool as the bot" path for OAuth-backed MCP servers. For MCP servers that do not use OAuth (for example, a server fronted by a shared API key configured by an admin), the credential is whatever the server itself was configured with. The initiator-as-executor rule still controls **whether** the call happens; the server's static credentials control **what** the call can do. +### Service account authentication + +An agent can instead be switched to **service account authentication** — the **Use service accounts for authentication** setting on the agent's MCPs tab. The setting is all-or-nothing for that agent and changes who the executor is: + +- **External MCP servers:** tool calls are sent with the admin-configured **Service Account Authentication** headers from the server's System Console configuration (for example, a personal access token) instead of the initiator's OAuth token. Servers with no service account headers configured are **excluded from the agent's tool catalog entirely** — the agent fails closed rather than falling back to anyone's personal credentials. +- **Embedded Mattermost tools and plugin-registered MCP servers:** tool calls run as the **agent's bot account**, not as the initiator. The bot's channel membership and permissions are the access boundary inside Mattermost. +- **No per-user OAuth anywhere:** users are never prompted to connect accounts for these agents, per-user tool provider preferences don't apply, and the per-user **Tools** menu is not shown. + +**The approval contract does not change: the human approves, the service account executes.** The initiator is still the only person who can accept or reject a pending tool call (§4), per-tool policies (§5) still decide which calls need approval, and the Share / Keep Private flow (§6) still belongs to the initiator. What changes is only whose credentials perform the side effect once consent is given. + +This mode deliberately flattens permissions: every user who can use the agent acts with the same service account access externally and the same bot access internally. The external system's audit log shows the service account, not the human; Mattermost's token usage logs record the triggering user alongside the acting identity (`acting_user_id`, `tool_auth_mode`) so the two can be correlated. It requires the same license as remote MCP servers; without that license the agent's tool calls keep using per-user credentials. Configuration and hardening guidance live in the [admin guide](../admin_guide.md#service-account-authentication). + ## 4. Approval ownership > **The conversation initiator is the only person who can approve or reject a pending tool call. No other channel member, including admins, can approve on the initiator's behalf.** @@ -59,10 +74,12 @@ This is enforced server-side, not just visually. When the **Accept** or **Reject The reasoning behind initiator-only approval: -- **Side effects belong to the requester.** Because tools run with the initiator's credentials and OAuth scopes (see §3), only the initiator has the standing to consent to side effects executed under their identity. +- **Side effects belong to the requester.** In per-user authentication mode, tools run with the initiator's credentials and OAuth scopes (see §3), so only the initiator has the standing to consent to side effects executed under their identity. In service account mode, initiator-only approval remains the consent contract even though the service account performs the action. - **Multiplayer approval would be confusing under partial trust.** If three members of a channel could each approve the same tool call, the question "whose Jira token did this run with?" becomes ambiguous and the audit trail becomes harder to reason about. - **Admins are not implicit approvers.** A workspace admin watching the channel does not automatically gain the ability to consent on the initiator's behalf. Admin authority lives in the per-tool policy (§5), not in the per-call approval. +Initiator-only approval applies in both credential modes (§3). For a service account agent, the initiator is still the person asking for the side effect, so their consent is still the one that matters; the admin consented to the credential itself when they enabled it on the agent. + ### What if the initiator is offline or never returns? Pending tool calls remain pending. They do not auto-approve after a timeout, and they do not transfer to anyone else. Practically, this means a tool call left unaddressed is a no-op: the Agent's response stops at the unapproved card, the tool never runs, and side effects never happen. The conversation can be re-triggered later, in which case the new conversation row creates a new initiator and a new approval flow. @@ -111,7 +128,7 @@ In a DM, when a tool runs, its arguments and results are visible to the only hum Multiplayer tool calling handles this with a two-step flow: -1. **Step 1: approve the call.** The initiator clicks **Accept** (or the policy auto-approves). The tool runs with the initiator's credentials. +1. **Step 1: approve the call.** The initiator clicks **Accept** (or the policy auto-approves). The tool runs with the executor's credentials — the initiator's by default, or the agent's service account (§3). 2. **Step 2: share the result.** Once the tool returns, the initiator sees a follow-up control with **Share** and **Keep Private** options. - **Share** marks the tool result as visible to the channel. Other channel members see the arguments and the result, and the Agent's follow-up response incorporates the result openly. - **Keep Private** marks the result as private to the initiator. Other channel members do not see the arguments or the result, and the Agent's follow-up response is generated without leaking that content into the channel-visible reply. @@ -176,7 +193,7 @@ Configuring the per-tool policy list is the main lever admins have over multipla - **Default new tools to `ask`.** It is the conservative choice and the easiest to relax later. Going the other direction — discovering a tool you wanted to require approval for has been auto-running in channels — is much worse. - **Reserve `auto_run_everywhere` for read-only tools with built-in permission enforcement.** The embedded Mattermost search tools ship at `auto_run_in_dm` by default. They fit the `auto_run_everywhere` profile in principle — Mattermost server enforces per-user permissions on every result — but the choice to promote them is left to the admin so that channel privacy expectations are explicit. Tools that hit external APIs almost never belong in this category unless you have a strong reason. - **Treat `auto_run_in_dm` as the "convenience in private, friction in public" mode.** Use it for tools that are safe for the initiator to see results from privately but where you don't want results splashed into a channel without explicit consent. -- **Don't use policy as a substitute for OAuth scope.** Policy controls the prompt; the underlying tool still runs as the initiator. Lock down scopes in the OAuth provider, not in the Agents policy. +- **Don't use policy as a substitute for credential scope.** Policy controls the prompt; the underlying tool still runs as the initiator — or, for service account agents, as the service account. Lock down scopes in the OAuth provider (and scope service account tokens minimally), not in the Agents policy. - **Audit which tools are seeded by default.** PR #520 adds vetted-provider seeding so that built-in MCP tools come pre-configured. Review the seeded list when you upgrade to v2 to make sure the defaults match your risk tolerance. The channel tool-calling capability itself is gated by the workspace setting **Enable Channel Mention Tool Calling**, which is experimental at the time of the v2 launch. Until that setting is enabled, multiplayer tool calling is effectively limited to `auto_run_*` policies in DMs and admins do not need to think about channel privacy at all. @@ -185,15 +202,15 @@ The channel tool-calling capability itself is gated by the workspace setting **E To make the model auditable, several things are deliberately **not** supported. These are not oversights; they are design choices. -- **No "any channel member can approve" mode.** Allowing onlookers to approve would mean a tool runs with the initiator's credentials but at someone else's request. That is a confused-deputy pattern. We do not offer it. +- **No "any channel member can approve" mode.** Allowing onlookers to approve would mean a side effect is committed against the initiator's request by someone who has no standing to consent to it — with the initiator's own credentials in the default mode, or with the agent's service account in the other. That is a confused-deputy pattern. We do not offer it. - **No per-channel admin override of the initiator-only rule.** Channel admins do not gain the ability to approve other people's pending tool calls just because they administer the channel. - **No timeout-based auto-approval.** A pending tool call left untouched does not silently fire. It just sits there until cleared. -- **No tool execution as the bot.** As noted in §3 and §9, tools always run as the initiator. There is no "service account" path for OAuth-backed MCP tools. +- **No implicit tool execution as the bot.** For per-user-authentication agents (§3), tools always run as the initiator; the bot never silently substitutes its own identity, and there is no fallback to a shared credential when the initiator's OAuth token is missing or rejected. Service account execution exists only as an explicit, admin-configured, per-agent opt-in — and even there, servers without service account credentials drop out of the agent's catalog rather than borrowing anyone else's. - **No retroactive privacy.** Once the initiator chooses **Share**, the result is visible to channel members and the Agent's follow-up response incorporates it openly. There is no "unshare" button. The same is true for `auto_run_everywhere` tools, which are auto-shared at the moment they execute — there is no Share / Keep Private prompt to retract. (Channel-level moderation tools — deleting posts, etc. — are still available but live outside Agents.) ## 12. Related docs - [User guide — Use tools](../user_guide.md#use-tools): the end-user-facing instructions for Accept / Reject and the Tools menu. -- [Admin guide](../admin_guide.md): per-tool policy configuration, agent management, and the **Enable Channel Mention Tool Calling** setting. +- [Admin guide](../admin_guide.md): per-tool policy configuration, agent management, [service account authentication](../admin_guide.md#service-account-authentication), and the **Enable Channel Mention Tool Calling** setting. - [Channel summaries](channel_summaries.md): a related channel-aware feature with its own privacy story. - [Providers](../providers.md): provider-side tool support, OAuth setup for MCP servers. diff --git a/docs/user_guide.md b/docs/user_guide.md index f8eea9671..9e8d6c7a7 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -106,6 +106,8 @@ Tool availability depends on your user permissions, provider connection status, Some MCP providers require each user to connect their own account before those tools become available. When that applies, open the **Tools** menu in the Agents pane or RHS, select **Connect** for the provider, and wait for the list to refresh with the newly available tools. +Some agents are configured by an admin to use **service account authentication** instead of per-user connections. When you chat with one of those agents, you're never asked to connect an account, the **Tools** menu isn't shown for that agent, and its tools run under admin-configured credentials — acting as the agent's bot account inside Mattermost — rather than your personal accounts. Tool approval works the same as with any other agent: if a tool call requires review, you still see the **Accept** and **Reject** options. + ## Analyze threads and channels ### Summarize discussion threads diff --git a/e2e/helpers/agent-api.ts b/e2e/helpers/agent-api.ts index 05ae0120b..215966b44 100644 --- a/e2e/helpers/agent-api.ts +++ b/e2e/helpers/agent-api.ts @@ -28,6 +28,7 @@ export interface CreateAgentRequest { adminUserIDs?: string[]; enabledMCPTools?: EnabledTool[]; autoEnableNewMCPTools: boolean; + useServiceAccountAuth?: boolean; enabledNativeTools?: string[]; model?: string; enableVision?: boolean; @@ -63,6 +64,7 @@ export interface AgentResponse { enabledNativeTools: string[]; enabledMCPTools?: EnabledTool[]; autoEnableNewMCPTools: boolean; + useServiceAccountAuth: boolean; reasoningEnabled: boolean; reasoningEffort: string; thinkingBudget: number; @@ -97,6 +99,7 @@ export function mergeAgentIntoUpdate( adminUserIDs: agent.adminUserIDs ?? [], enabledMCPTools: agent.enabledMCPTools ?? [], autoEnableNewMCPTools: agent.autoEnableNewMCPTools, + useServiceAccountAuth: agent.useServiceAccountAuth, enabledNativeTools: agent.enabledNativeTools, model: agent.model, enableVision: agent.enableVision, diff --git a/llm/configuration.go b/llm/configuration.go index 4ff000439..7e05975f2 100644 --- a/llm/configuration.go +++ b/llm/configuration.go @@ -153,6 +153,11 @@ type BotConfig struct { // It defaults to true for omitted legacy config. MCPDynamicToolLoading bool `json:"mcpDynamicToolLoading"` + // UseServiceAccountAuth switches all MCP access for this agent to the service + // account identity (admin ServiceAccountHeaders for external servers, the bot + // user for embedded/plugin servers) instead of per-user OAuth. + UseServiceAccountAuth bool `json:"useServiceAccountAuth"` + // ReasoningEnabled determines whether reasoning/thinking is enabled for this bot. // Applicable to OpenAI (with ResponsesAPI), Anthropic, and Gemini / Vertex AI. ReasoningEnabled bool `json:"reasoningEnabled"` diff --git a/llm/configuration_test.go b/llm/configuration_test.go index 98cf30ffd..72966db37 100644 --- a/llm/configuration_test.go +++ b/llm/configuration_test.go @@ -665,9 +665,11 @@ func TestBotConfigMCPDynamicToolLoadingDefaulting(t *testing.T) { }) } - raw, err := json.Marshal(BotConfig{MCPDynamicToolLoading: false}) + // The webapp relies on both flags being present even when false. + raw, err := json.Marshal(BotConfig{MCPDynamicToolLoading: false, UseServiceAccountAuth: false}) require.NoError(t, err) assert.Contains(t, string(raw), `"mcpDynamicToolLoading":false`) + assert.Contains(t, string(raw), `"useServiceAccountAuth":false`) } func TestServiceConfig_JSONRoundTrip_FallbackServiceID(t *testing.T) { diff --git a/llm/context.go b/llm/context.go index cf293f474..da936bfa4 100644 --- a/llm/context.go +++ b/llm/context.go @@ -44,6 +44,10 @@ type Context struct { BotServiceType string CustomInstructions string + // ToolAuthMode records the identity mode the tool catalog was built with + // (ToolAuthModeUser or ToolAuthModeServiceAccount); consumed by token usage attribution. + ToolAuthMode string + Tools *ToolStore DisabledToolsInfo []ToolInfo // Info about tools that are unavailable in the current context (e.g., DM-only tools in a channel) Parameters map[string]interface{} diff --git a/llm/token_tracking.go b/llm/token_tracking.go index 7ee63a0e5..32078a1be 100644 --- a/llm/token_tracking.go +++ b/llm/token_tracking.go @@ -127,6 +127,8 @@ func (w *TokenUsageLoggingWrapper) ChatCompletion(ctx context.Context, request C type tokenUsageDimensions struct { userID string + actingUserID string + toolAuthMode string teamID string channelID string channelType string @@ -175,6 +177,8 @@ func buildTokenUsageLogKeyValuePairs(dimensions tokenUsageDimensions, usage Toke // Temporary compatibility alias for existing dashboards/queries. "bot_username", dimensions.botUsername, "agent_user_id", dimensions.botUserID, + "acting_user_id", dimensions.actingUserID, + "tool_auth_mode", dimensions.toolAuthMode, "model", dimensions.model, "service_type", dimensions.serviceType, "operation", dimensions.operation, @@ -204,6 +208,8 @@ func tokenUsageKeyValuePairsToMlogFields(keyValuePairs []any) []mlog.Field { func extractTokenUsageDimensions(request CompletionRequest, fallbackBotUsername, optionModel string) tokenUsageDimensions { dimensions := tokenUsageDimensions{ userID: TokenUsageUnknown, + actingUserID: TokenUsageUnknown, + toolAuthMode: ToolAuthModeUser, teamID: TokenUsageUnknown, channelID: TokenUsageUnknown, channelType: TokenUsageUnknown, @@ -230,6 +236,17 @@ func extractTokenUsageDimensions(request CompletionRequest, fallbackBotUsername, dimensions.userID = request.Context.RequestingUser.Id } + // The acting identity is the requesting user in user mode, the agent's + // bot user in service-account mode. + dimensions.actingUserID = dimensions.userID + if request.Context.ToolAuthMode == ToolAuthModeServiceAccount { + dimensions.toolAuthMode = ToolAuthModeServiceAccount + dimensions.actingUserID = TokenUsageUnknown + if request.Context.BotUserID != "" { + dimensions.actingUserID = request.Context.BotUserID + } + } + if request.Context.Team != nil && request.Context.Team.Id != "" { dimensions.teamID = request.Context.Team.Id } else if request.Context.Channel != nil { diff --git a/llm/token_tracking_test.go b/llm/token_tracking_test.go index e254e1d67..0c0373f14 100644 --- a/llm/token_tracking_test.go +++ b/llm/token_tracking_test.go @@ -186,6 +186,8 @@ func TestTokenTrackingWrapper_ChatCompletion_TableDriven(t *testing.T) { "agent_username": "testbot", "bot_username": "testbot", "agent_user_id": "bot-user-id", + "acting_user_id": "user-123", + "tool_auth_mode": ToolAuthModeUser, "model": "override-model", "service_type": "openai", "operation": OperationConversation, @@ -195,6 +197,40 @@ func TestTokenTrackingWrapper_ChatCompletion_TableDriven(t *testing.T) { "total_tokens": int64(20), }, }, + { + name: "service account requests are attributed to the agent bot", + request: CompletionRequest{ + Context: &Context{ + RequestingUser: &model.User{Id: "user-123"}, + Channel: &model.Channel{Id: "channel-789", Type: model.ChannelTypeOpen}, + BotUsername: "testbot", + BotUserID: "bot-user-id", + ToolAuthMode: ToolAuthModeServiceAccount, + }, + Operation: OperationConversation, + OperationSubType: SubTypeStreaming, + }, + stream: makeStream( + TextStreamEvent{Type: EventTypeUsage, Value: TokenUsage{InputTokens: 4, OutputTokens: 6}}, + TextStreamEvent{Type: EventTypeEnd, Value: nil}, + ), + expectedEventTypes: []EventType{EventTypeEnd}, + expectedMetrics: []observedTokenUsage{ + { + botName: "testbot", + teamID: TokenUsageUnknown, + userID: "user-123", + inputTokens: 4, + outputTokens: 6, + }, + }, + expectedLogFields: map[string]any{ + "user_id": "user-123", + "agent_user_id": "bot-user-id", + "acting_user_id": "bot-user-id", + "tool_auth_mode": ToolAuthModeServiceAccount, + }, + }, { name: "uses unknown defaults for nil context with stream subtype default", request: CompletionRequest{ @@ -225,6 +261,8 @@ func TestTokenTrackingWrapper_ChatCompletion_TableDriven(t *testing.T) { "agent_username": "fallback-bot", "bot_username": "fallback-bot", "agent_user_id": TokenUsageUnknown, + "acting_user_id": TokenUsageUnknown, + "tool_auth_mode": ToolAuthModeUser, "model": TokenUsageUnknown, "service_type": TokenUsageUnknown, "operation": TokenUsageUnknown, @@ -322,6 +360,8 @@ func TestTokenTrackingWrapper_ChatCompletion_TableDriven(t *testing.T) { func TestBuildTokenUsageLogKeyValuePairs(t *testing.T) { dimensions := tokenUsageDimensions{ userID: "user-1", + actingUserID: "bot-user-1", + toolAuthMode: ToolAuthModeServiceAccount, teamID: "team-1", channelID: "channel-1", channelType: "open", @@ -381,6 +421,8 @@ func TestBuildTokenUsageLogKeyValuePairs(t *testing.T) { assert.Equal(t, TokenUsageLogEvent, keyed["event"]) assert.Equal(t, TokenUsageLogSchemaVersion, keyed["schema_version"]) assert.Equal(t, "user-1", keyed["user_id"]) + assert.Equal(t, "bot-user-1", keyed["acting_user_id"]) + assert.Equal(t, ToolAuthModeServiceAccount, keyed["tool_auth_mode"]) assert.Equal(t, "claude-sonnet-4-5", keyed["model"]) }) } diff --git a/llm/token_usage_fields.go b/llm/token_usage_fields.go index 7e5edcb53..15e77f9de 100644 --- a/llm/token_usage_fields.go +++ b/llm/token_usage_fields.go @@ -32,6 +32,12 @@ const ( OperationBridgeService = "bridge_service" ) +// tool_auth_mode identifies which auth mode the request's tool catalog was built with. +const ( + ToolAuthModeUser = "user" + ToolAuthModeServiceAccount = "service_account" +) + // operation_subtype is a low-cardinality detail for the operation. // Typical values represent modality or a small scenario class // (for example streaming vs non-streaming, tool calls, or chunk modes). diff --git a/llmcontext/llm_context.go b/llmcontext/llm_context.go index 90ac42a51..04dbc5459 100644 --- a/llmcontext/llm_context.go +++ b/llmcontext/llm_context.go @@ -23,9 +23,11 @@ type ToolProvider interface { GetTools(bot *bots.Bot, llmContext *llm.Context) []llm.Tool } -// MCPToolProvider provides MCP tools for a user +// MCPToolProvider provides MCP tools for a user or for a service-account agent type MCPToolProvider interface { GetToolsForUser(ctx stdcontext.Context, userID string) ([]llm.Tool, *mcp.Errors) + // GetToolsForServiceAccount returns the catalog for a service-account agent acting as botUserID. + GetToolsForServiceAccount(ctx stdcontext.Context, botUserID string) ([]llm.Tool, *mcp.Errors) } type MCPToolRetrievalOverrideProvider interface { @@ -213,6 +215,17 @@ func sanitizeUserProfileField(s string) string { // WithLLMContextSessionID removed: embedded MCP manages its own session lifecycle +// isRemoteMCPLicensed reports whether the licensed remote-MCP feature is available. +func (b *Builder) isRemoteMCPLicensed() bool { + return b.licenseChecker == nil || b.licenseChecker.IsBasicsLicensed() +} + +// UsesServiceAccountCatalog reports whether tool catalogs for this bot are built in +// service-account mode; on an unlicensed server SA agents behave like normal agents. +func (b *Builder) UsesServiceAccountCatalog(bot *bots.Bot) bool { + return bot != nil && bot.GetConfig().UseServiceAccountAuth && b.isRemoteMCPLicensed() +} + // getToolsStoreForUser returns a tool store for a specific user, including MCP tools. func (b *Builder) getToolsStoreForUser(ctx stdcontext.Context, c *llm.Context, bot *bots.Bot, userID string, forceConcrete bool) *llm.ToolStore { // Check for nil bot, which is unexpected @@ -227,6 +240,11 @@ func (b *Builder) getToolsStoreForUser(ctx stdcontext.Context, c *llm.Context, b return llm.NewNoTools() } + // useServiceAccount implies remoteMCPLicensed, so the license filter below + // never strips service account catalogs. + remoteMCPLicensed := b.isRemoteMCPLicensed() + useServiceAccount := b.UsesServiceAccountCatalog(bot) + // Check if tools are disabled for this bot if bot.GetConfig().DisableTools { return llm.NewNoTools() @@ -251,7 +269,19 @@ func (b *Builder) getToolsStoreForUser(ctx stdcontext.Context, c *llm.Context, b } // Get tools from all connected servers - mcpTools, mcpErrors = b.mcpToolProvider.GetToolsForUser(ctx, userID) + if useServiceAccount { + c.ToolAuthMode = llm.ToolAuthModeServiceAccount + botUserID := bot.BotUserID() + if botUserID == "" { + // Fail closed: no acting identity means no MCP catalog at all. + b.pluginAPI.Log.Error("Service account agent has no bot user ID; skipping MCP tools", + "bot_name", botCfg.Name, "bot_id", botCfg.ID) + } else { + mcpTools, mcpErrors = b.mcpToolProvider.GetToolsForServiceAccount(ctx, botUserID) + } + } else { + mcpTools, mcpErrors = b.mcpToolProvider.GetToolsForUser(ctx, userID) + } // Remote/external MCP servers are the licensed "MCP Support" feature. // Without a license their tools are never supplied to the LLM: they @@ -259,7 +289,6 @@ func (b *Builder) getToolsStoreForUser(ctx stdcontext.Context, c *llm.Context, b // loading registry is built, so the model cannot see, load, or call // them. Embedded Mattermost MCP tools are basic tool integrations // and are not filtered. - remoteMCPLicensed := b.licenseChecker == nil || b.licenseChecker.IsBasicsLicensed() if !remoteMCPLicensed { mcpTools = filterMCPToolsByPredicate(mcpTools, func(tool llm.Tool) bool { return !mcp.IsRemoteServerOrigin(tool.ServerOrigin) @@ -268,8 +297,9 @@ func (b *Builder) getToolsStoreForUser(ctx stdcontext.Context, c *llm.Context, b // Per-agent MCP tool filtering: unless the agent is configured to pick up // every MCP tool automatically, retain only tools listed in its allowlist. - // This runs AFTER admin policy (filterToolsByConfig inside GetToolsForUser) - // and BEFORE per-user/channel filtering and strict registry construction. + // This runs AFTER admin policy (filterToolsByConfig inside the MCP + // provider) and BEFORE per-user/channel filtering and strict registry + // construction. if !botCfg.AutoEnableNewMCPTools { mcpTools = llm.FilterMCPToolsByEnabledAllowlist(mcpTools, botCfg.EnabledMCPTools) } @@ -506,11 +536,7 @@ func (b *Builder) WithLLMContextParameters(params map[string]interface{}) llm.Co func (b *Builder) WithLLMContextBot(bot *bots.Bot) llm.ContextOption { return func(c *llm.Context) { - var botUserID string - if mmbot := bot.GetMMBot(); mmbot != nil { - botUserID = mmbot.UserId - } - c.SetBotFields(bot.GetConfig().DisplayName, bot.GetConfig().Name, botUserID, bot.GetService().DefaultModel, bot.GetService().Type, bot.GetConfig().CustomInstructions) + c.SetBotFields(bot.GetConfig().DisplayName, bot.GetConfig().Name, bot.BotUserID(), bot.GetService().DefaultModel, bot.GetService().Type, bot.GetConfig().CustomInstructions) c.ToolCatalog.MCPDynamicToolLoading = bot.GetConfig().MCPDynamicToolLoading c.ToolRuntime.MCPDynamicToolTelemetry = b.mcpDynamicToolTelemetry } diff --git a/llmcontext/llm_context_license_test.go b/llmcontext/llm_context_license_test.go index e2d772c6f..fc7b3c814 100644 --- a/llmcontext/llm_context_license_test.go +++ b/llmcontext/llm_context_license_test.go @@ -34,9 +34,15 @@ func newLicenseTestBuilder(t *testing.T, licensed bool, toolProvider ToolProvide } else { mockAPI.On("GetLicense").Return((*model.License)(nil)).Maybe() } - mockAPI.On("LogDebug", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe().Return() - mockAPI.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe().Return() - mockAPI.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe().Return() + for i := 1; i <= 10; i++ { + args := make([]interface{}, i) + for j := range args { + args[j] = mock.Anything + } + mockAPI.On("LogDebug", args...).Maybe().Return() + mockAPI.On("LogWarn", args...).Maybe().Return() + mockAPI.On("LogError", args...).Maybe().Return() + } return NewLLMContextBuilder( pluginapi.NewClient(mockAPI, nil), @@ -140,6 +146,35 @@ func TestUnlicensedBuilderDropsRemoteMCPToolsFromDynamicRegistry(t *testing.T) { } } +// SA auth inherits the remote-MCP enterprise gate: unlicensed SA-flagged agents behave like normal agents. +func TestServiceAccountModeFullyOffWhenUnlicensed(t *testing.T) { + provider := &staticMCPToolProvider{ + tools: []llm.Tool{ + testMCPTool("mattermost__read_channel", mcp.EmbeddedClientKey, "read channel posts"), + testMCPTool("jira__get_issue", licenseTestRemoteOrigin, "fetch Jira issue details"), + }, + saTools: []llm.Tool{testMCPTool("sa_jira__get_issue", licenseTestRemoteOrigin, "service account Jira")}, + } + builder := newLicenseTestBuilder(t, false, + &staticToolProvider{tools: []llm.Tool{testBuiltinTool("builtin")}}, + provider, + ) + bot := newTestBotWithConfig(llm.BotConfig{ + ID: "bot-id", + Name: "matty", + DisplayName: "Matty", + AutoEnableNewMCPTools: true, + UseServiceAccountAuth: true, + }) + + context := buildToolsContext(builder, bot) + + require.Equal(t, []string{"user-id"}, provider.userCalls, "unlicensed SA agents use the per-user catalog") + require.Empty(t, provider.saCalls, "unlicensed servers must never build a service account catalog") + require.ElementsMatch(t, []string{"builtin", "mattermost__read_channel"}, toolNames(context.Tools)) + require.Empty(t, context.ToolAuthMode, "unlicensed SA agents are attributed as user mode") +} + // TestUnlicensedBuilderDropsRemoteMCPAuthErrors pins that OAuth prompts for // remote servers are not surfaced when their tools cannot be used without a // license. diff --git a/llmcontext/llm_context_test.go b/llmcontext/llm_context_test.go index 44ff87d81..41f1c14a0 100644 --- a/llmcontext/llm_context_test.go +++ b/llmcontext/llm_context_test.go @@ -34,7 +34,8 @@ func (p *staticToolProvider) GetTools(*bots.Bot, *llm.Context) []llm.Tool { } type countingMCPToolProvider struct { - calls int + calls int + saCalls int } func (p *countingMCPToolProvider) GetToolsForUser(stdcontext.Context, string) ([]llm.Tool, *mcp.Errors) { @@ -48,16 +49,32 @@ func (p *countingMCPToolProvider) GetToolsForUser(stdcontext.Context, string) ([ }, nil } +func (p *countingMCPToolProvider) GetToolsForServiceAccount(stdcontext.Context, string) ([]llm.Tool, *mcp.Errors) { + p.saCalls++ + return nil, nil +} + +// staticMCPToolProvider serves a fixed catalog per auth mode and records the identity each mode was asked for. type staticMCPToolProvider struct { tools []llm.Tool + saTools []llm.Tool errors *mcp.Errors overrides map[string]mcp.ToolRetrievalOverride + + userCalls []string + saCalls []string } -func (p *staticMCPToolProvider) GetToolsForUser(stdcontext.Context, string) ([]llm.Tool, *mcp.Errors) { +func (p *staticMCPToolProvider) GetToolsForUser(_ stdcontext.Context, userID string) ([]llm.Tool, *mcp.Errors) { + p.userCalls = append(p.userCalls, userID) return p.tools, p.errors } +func (p *staticMCPToolProvider) GetToolsForServiceAccount(_ stdcontext.Context, botUserID string) ([]llm.Tool, *mcp.Errors) { + p.saCalls = append(p.saCalls, botUserID) + return p.saTools, nil +} + func (p *staticMCPToolProvider) GetToolRetrievalOverrides() map[string]mcp.ToolRetrievalOverride { return p.overrides } @@ -87,10 +104,14 @@ func newTestBot() *bots.Bot { } func newTestBotWithConfig(cfg llm.BotConfig) *bots.Bot { + return newTestBotWithMMBot(cfg, &model.Bot{UserId: "bot-id", Username: "matty", DisplayName: "Matty"}) +} + +func newTestBotWithMMBot(cfg llm.BotConfig, mmBot *model.Bot) *bots.Bot { return bots.NewBot( cfg, llm.ServiceConfig{DefaultModel: "test-model", Type: llm.ServiceTypeOpenAI}, - &model.Bot{UserId: "bot-id", Username: "matty", DisplayName: "Matty"}, + mmBot, nil, ) } @@ -232,6 +253,7 @@ func TestWithLLMContextDefaultToolsCallsMCPProvider(t *testing.T) { ) require.Equal(t, 1, mcpProvider.calls) + require.Equal(t, 0, mcpProvider.saCalls, "a normal agent must never use the service account catalog") require.Len(t, context.Tools.GetTools(), 1) } @@ -315,6 +337,42 @@ func TestWithLLMContextDefaultToolsRetainsAuthErrorsForWildcardAllowlist(t *test assert.Equal(t, "https://auth.example.com", authErrors[0].AuthURL) } +// A service account agent's catalog comes from the bot identity, not the requesting user. +func TestGetToolsStoreServiceAccountSelection(t *testing.T) { + const serviceAccountBotUserID = "bot-user-id" + + provider := &staticMCPToolProvider{ + tools: []llm.Tool{testMCPTool("jira__get_issue", "https://jira.example.com", "user OAuth Jira")}, + saTools: []llm.Tool{testMCPTool("sa_jira__get_issue", "https://jira.example.com", "service account Jira")}, + } + builder := newLicenseTestBuilder(t, true, + &staticToolProvider{tools: []llm.Tool{testBuiltinTool("builtin")}}, + provider, + ) + bot := newTestBotWithMMBot( + llm.BotConfig{ + ID: "bot-id", + Name: "matty", + DisplayName: "Matty", + AutoEnableNewMCPTools: true, + UseServiceAccountAuth: true, + }, + &model.Bot{UserId: serviceAccountBotUserID, Username: "matty", DisplayName: "Matty"}, + ) + + context := builder.BuildLLMContextUserRequest( + bot, + &model.User{Id: "user-id", Username: "test-user", Locale: "en"}, + testChannel(), + builder.WithLLMContextTools(stdcontext.Background(), bot), + ) + + require.Equal(t, []string{serviceAccountBotUserID}, provider.saCalls) + require.Empty(t, provider.userCalls, "the requesting user's catalog must not be consulted") + require.ElementsMatch(t, []string{"builtin", "sa_jira__get_issue"}, toolNames(context.Tools)) + require.Equal(t, llm.ToolAuthModeServiceAccount, context.ToolAuthMode) +} + func TestSanitizeUserProfileField(t *testing.T) { tests := []struct { name string diff --git a/mcp/client.go b/mcp/client.go index 8f29b8cf0..eee7ba695 100644 --- a/mcp/client.go +++ b/mcp/client.go @@ -67,6 +67,17 @@ type Client struct { toolsCache *ToolsCache embeddedClient *EmbeddedServerClient // for reconnection (nil for remote servers) sessionID string // session ID for embedded server reconnection + serviceAccount bool // auth via static ServiceAccountHeaders; userID is the bot user, oauthManager nil +} + +// clientParams bundles the dependencies for a remote MCP client connection. +type clientParams struct { + log pluginapi.LogService + oauthManager *OAuthManager // nil in service-account mode + httpClient *http.Client + toolsCache *ToolsCache + forceRefresh bool + serviceAccount bool } // staticOAuthCreds returns static OAuth credentials from a server config, or nil if not configured. @@ -80,10 +91,46 @@ func staticOAuthCreds(s ServerConfig) *StaticOAuthCredentials { } } -func shouldUseSharedToolsCache(serverConfig ServerConfig) bool { +// sharedToolsCacheAllowedForServer reports whether user-mode connections may use the +// shared tools cache; static OAuth credentials make the catalog user-specific. +func sharedToolsCacheAllowedForServer(serverConfig ServerConfig) bool { return staticOAuthCreds(serverConfig) == nil } +// serviceAccountToolsCacheID namespaces service-account tool lists away from the +// user-mode cache entry (keyed by the bare server name). +func serviceAccountToolsCacheID(serverName string) string { + return "sa:" + serverName +} + +func (c *Client) toolsCacheServerID() string { + if c.serviceAccount { + return serviceAccountToolsCacheID(c.config.Name) + } + return c.config.Name +} + +// useSharedToolsCache reports whether this client may read/write the shared tools +// cache. Service-account credentials are identical for every connection, so SA mode always may. +func (c *Client) useSharedToolsCache() bool { + if c.serviceAccount { + return true + } + return sharedToolsCacheAllowedForServer(c.config) +} + +// remoteConnectionHeaders builds the static headers for a remote MCP connection. +// Later layers win on key conflicts: X-Mattermost-UserID < admin Headers < ServiceAccountHeaders. +func remoteConnectionHeaders(userID string, serverConfig ServerConfig, serviceAccount bool) map[string]string { + headers := make(map[string]string) + headers[MMUserIDHeader] = userID + maps.Copy(headers, serverConfig.Headers) + if serviceAccount { + maps.Copy(headers, serverConfig.EffectiveServiceAccountHeaders()) + } + return headers +} + func invalidateSharedToolsCacheForOAuthDiscovery(toolsCache *ToolsCache, log Logger, userID, serverID string, serverConfig ServerConfig, hasStoredToken bool) { if toolsCache == nil || hasStoredToken { return @@ -102,7 +149,7 @@ func invalidateSharedToolsCacheForOAuthDiscovery(toolsCache *ToolsCache, log Log // server when the MCP server uses OAuth and the user has not completed OAuth yet. That avoids // ListTools reusing tools discovered before authentication (shared cache is only for non-OAuth servers). func maybeInvalidateSharedToolsBeforeOAuthListTools(userID string, serverConfig ServerConfig, log pluginapi.LogService, toolsCache *ToolsCache, oauthManager *OAuthManager) { - if shouldUseSharedToolsCache(serverConfig) || toolsCache == nil || oauthManager == nil { + if sharedToolsCacheAllowedForServer(serverConfig) || toolsCache == nil || oauthManager == nil { return } @@ -234,20 +281,33 @@ func (c *EmbeddedServerClient) CreateClient(ctx context.Context, userID, session return client, nil } -// NewClient creates a new MCP client for the given server and user and connects to the specified MCP server. +// NewClient creates a user-OAuth-mode MCP client for the given server and user and connects to it. // forceRefresh bypasses the shared tools cache read. Its sole purpose is to close the race where a concurrent // lookup repopulates the cache between a manual refresh's invalidation and this reconnect; a plain // post-invalidation rediscovery would otherwise cache-miss on its own. func NewClient(ctx context.Context, userID string, serverConfig ServerConfig, log pluginapi.LogService, oauthManager *OAuthManager, httpClient *http.Client, toolsCache *ToolsCache, forceRefresh bool) (*Client, error) { - c := &Client{ - session: nil, - config: serverConfig, - tools: make(map[string]*mcp.Tool), - userID: userID, + return newClient(ctx, userID, serverConfig, clientParams{ log: log, oauthManager: oauthManager, httpClient: httpClient, toolsCache: toolsCache, + forceRefresh: forceRefresh, + }) +} + +// newClient connects to a remote MCP server in either auth mode. In service-account +// mode p.oauthManager must be nil so no OAuth flow can occur. +func newClient(ctx context.Context, userID string, serverConfig ServerConfig, p clientParams) (*Client, error) { + c := &Client{ + session: nil, + config: serverConfig, + tools: make(map[string]*mcp.Tool), + userID: userID, + log: p.log, + oauthManager: p.oauthManager, + httpClient: p.httpClient, + toolsCache: p.toolsCache, + serviceAccount: p.serviceAccount, } session, err := c.createSession(ctx, serverConfig) @@ -255,19 +315,19 @@ func NewClient(ctx context.Context, userID string, serverConfig ServerConfig, lo return nil, fmt.Errorf("failed to create MCP session for server %s: %w", serverConfig.Name, err) } - useSharedToolsCache := shouldUseSharedToolsCache(serverConfig) - maybeInvalidateSharedToolsBeforeOAuthListTools(userID, serverConfig, log, toolsCache, oauthManager) - serverID := serverConfig.Name + sharedToolsCache := c.useSharedToolsCache() + maybeInvalidateSharedToolsBeforeOAuthListTools(userID, serverConfig, p.log, p.toolsCache, p.oauthManager) + serverID := c.toolsCacheServerID() // Try to get tools from global cache first. - if toolsCache != nil && useSharedToolsCache && !forceRefresh { - cachedTools := toolsCache.GetTools(serverID) + if p.toolsCache != nil && sharedToolsCache && !p.forceRefresh { + cachedTools := p.toolsCache.GetTools(serverID) if len(cachedTools) > 0 { // Cache hit - use cached tools c.toolsMu.Lock() c.tools = cachedTools c.toolsMu.Unlock() - log.Debug("Using cached tools for MCP server", + p.log.Debug("Using cached tools for MCP server", "userID", userID, "server", serverConfig.Name, "toolCount", len(cachedTools)) @@ -296,7 +356,7 @@ func NewClient(ctx context.Context, userID string, serverConfig ServerConfig, lo c.tools = discoveredTools c.toolsMu.Unlock() for _, tool := range discoveredTools { - log.Debug("Registered MCP tool", + p.log.Debug("Registered MCP tool", "userID", userID, "name", tool.Name, "description", tool.Description, @@ -304,9 +364,9 @@ func NewClient(ctx context.Context, userID string, serverConfig ServerConfig, lo } // Update the global cache with fetched tools. - if toolsCache != nil && useSharedToolsCache { - if err := toolsCache.SetTools(serverID, serverConfig.Name, serverConfig.BaseURL, discoveredTools, time.Now()); err != nil { - log.Warn("Failed to update tools cache", "server", serverConfig.Name, "error", err) + if p.toolsCache != nil && sharedToolsCache { + if err := p.toolsCache.SetTools(serverID, serverConfig.Name, serverConfig.BaseURL, discoveredTools, time.Now()); err != nil { + p.log.Warn("Failed to update tools cache", "server", serverConfig.Name, "error", err) } } @@ -427,6 +487,11 @@ func (c *Client) oauthNeededError(err error) error { return nil } + // Service-account mode has no per-user OAuth; never classify a failure as OAuth-needed. + if c.serviceAccount { + return nil + } + var mcpAuthErr *mcpUnauthorized if errors.As(err, &mcpAuthErr) { md := mcpAuthErr.MetadataURL() @@ -450,9 +515,7 @@ func (c *Client) oauthNeededError(err error) error { func (c *Client) createSession(ctx context.Context, serverConfig ServerConfig) (*mcp.ClientSession, error) { // Prepare headers for remote servers - headers := make(map[string]string) - headers[MMUserIDHeader] = c.userID - maps.Copy(headers, serverConfig.Headers) + headers := remoteConnectionHeaders(c.userID, serverConfig, c.serviceAccount) // TODO: Load and check cached authentication information @@ -616,8 +679,8 @@ func (c *Client) CallToolWithMetadata(ctx context.Context, toolName string, args c.tools = discoveredTools c.toolsMu.Unlock() - if c.toolsCache != nil && shouldUseSharedToolsCache(c.config) { - if cacheErr := c.toolsCache.SetTools(c.config.Name, c.config.Name, c.config.BaseURL, discoveredTools, time.Now()); cacheErr != nil { + if c.toolsCache != nil && c.useSharedToolsCache() { + if cacheErr := c.toolsCache.SetTools(c.toolsCacheServerID(), c.config.Name, c.config.BaseURL, discoveredTools, time.Now()); cacheErr != nil { c.log.Warn("Failed to update tools cache after MCP reconnect", "server", c.config.Name, "userID", c.userID, diff --git a/mcp/client_embedded_oauth_test.go b/mcp/client_embedded_oauth_test.go index 7a4f38a92..2b064cf3a 100644 --- a/mcp/client_embedded_oauth_test.go +++ b/mcp/client_embedded_oauth_test.go @@ -171,34 +171,57 @@ func TestExtractOAuthMetadataURL(t *testing.T) { } func TestClientOAuthNeededError(t *testing.T) { - client := &Client{ - config: ServerConfig{ - Name: "OAuth Server", - }, - oauthManager: &OAuthManager{ - callbackURL: "https://mattermost.example.com/plugins/mattermost-ai/oauth/callback", - }, - } - tests := []struct { - name string - err error + name string + err error + serviceAccount bool + wantOAuthError bool }{ { name: "mcp unauthorized error", err: &mcpUnauthorized{ metadataURL: "https://oauth.example.com/.well-known/oauth-protected-resource", }, + wantOAuthError: true, }, { - name: "string matched oauth error", - err: fmt.Errorf("OAuth authentication needed for resource at https://oauth.example.com/.well-known/oauth-protected-resource"), + name: "string matched oauth error", + err: fmt.Errorf("OAuth authentication needed for resource at https://oauth.example.com/.well-known/oauth-protected-resource"), + wantOAuthError: true, + }, + { + name: "service account mode ignores an unauthorized error", + err: &mcpUnauthorized{ + metadataURL: "https://oauth.example.com/.well-known/oauth-protected-resource", + }, + serviceAccount: true, + }, + { + name: "service account mode ignores oauth-shaped error text", + err: fmt.Errorf("OAuth authentication needed for resource at https://oauth.example.com/.well-known/oauth-protected-resource"), + serviceAccount: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Service account bags have no OAuth manager in production; keeping one here + // pins the mode guard rather than the nil. + client := &Client{ + config: ServerConfig{ + Name: "OAuth Server", + }, + oauthManager: &OAuthManager{ + callbackURL: "https://mattermost.example.com/plugins/mattermost-ai/oauth/callback", + }, + serviceAccount: tt.serviceAccount, + } + err := client.oauthNeededError(tt.err) + if !tt.wantOAuthError { + require.NoError(t, err, "service account mode must never ask the user to connect an account") + return + } require.Error(t, err) var oauthErr *OAuthNeededError @@ -228,7 +251,7 @@ func TestNilCacheHandling(t *testing.T) { require.Nil(t, tools) } -func TestShouldUseSharedToolsCache(t *testing.T) { +func TestSharedToolsCacheAllowedForServer(t *testing.T) { tests := []struct { name string serverConfig ServerConfig @@ -256,7 +279,7 @@ func TestShouldUseSharedToolsCache(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - require.Equal(t, tt.expected, shouldUseSharedToolsCache(tt.serverConfig)) + require.Equal(t, tt.expected, sharedToolsCacheAllowedForServer(tt.serverConfig)) }) } } diff --git a/mcp/client_manager.go b/mcp/client_manager.go index 3b0fb67f1..6c164149e 100644 --- a/mcp/client_manager.go +++ b/mcp/client_manager.go @@ -27,14 +27,21 @@ func cacheableContext(ctx context.Context) context.Context { return context.WithoutCancel(ctx) } +// clientKey identifies one pooled client bag. serviceAccount is part of the key so a +// user-OAuth bag for a bot user ID never collides with that bot's service-account bag. +type clientKey struct { + userID string + serviceAccount bool +} + // ClientManager manages MCP clients for multiple users type ClientManager struct { config Config log pluginapi.LogService pluginAPI *pluginapi.Client clientsMu sync.RWMutex - clients map[string]*UserClients // userID to UserClients - activity map[string]time.Time // userID to last activity time + clients map[clientKey]*UserClients // identity+auth mode to UserClients + activity map[clientKey]time.Time // identity+auth mode to last activity time cleanupTicker *time.Ticker closeChan chan struct{} clientTimeout time.Duration @@ -85,11 +92,12 @@ func (m *ClientManager) cleanupInactiveClients(closeChan <-chan struct{}, ticker case <-ticker.C: m.clientsMu.Lock() now := time.Now() - for userID, client := range m.clients { - if now.Sub(m.activity[userID]) > m.clientTimeout { - m.log.Debug("Closing inactive MCP client", "userID", userID) + for key, client := range m.clients { + if now.Sub(m.activity[key]) > m.clientTimeout { + m.log.Debug("Closing inactive MCP client", "userID", key.userID, "serviceAccount", key.serviceAccount) client.Close() - delete(m.clients, userID) + delete(m.clients, key) + delete(m.activity, key) } } m.clientsMu.Unlock() @@ -116,10 +124,10 @@ func (m *ClientManager) ReInit(config Config, embeddedServer EmbeddedMCPServer) } m.config = config - m.clients = make(map[string]*UserClients) + m.clients = make(map[clientKey]*UserClients) m.clientTimeout = time.Duration(config.IdleTimeoutMinutes) * time.Minute m.closeChan = make(chan struct{}) - m.activity = make(map[string]time.Time) + m.activity = make(map[clientKey]time.Time) m.cleanupTicker = time.NewTicker(5 * time.Minute) go m.cleanupInactiveClients(m.closeChan, m.cleanupTicker) @@ -149,26 +157,31 @@ func (m *ClientManager) Close() { } // Clear the clients map - m.clients = make(map[string]*UserClients) + m.clients = make(map[clientKey]*UserClients) } // createAndStoreUserClient creates a new UserClients instance and stores it in the manager. // When forceRefresh is true the remote connect bypasses the shared tools cache and any // existing cached client is replaced. -func (m *ClientManager) createAndStoreUserClient(ctx context.Context, userID string, forceRefresh bool) (*UserClients, *Errors) { +func (m *ClientManager) createAndStoreUserClient(ctx context.Context, key clientKey, forceRefresh bool) (*UserClients, *Errors) { // Unless forcing a refresh, reuse an already-cached client so we skip a // redundant remote connect when another goroutine cached one first. if !forceRefresh { m.clientsMu.Lock() - if client, exists := m.clients[userID]; exists { - m.activity[userID] = time.Now() + if client, exists := m.clients[key]; exists { + m.activity[key] = time.Now() m.clientsMu.Unlock() return client, client.InitialRemoteConnectErrors() } m.clientsMu.Unlock() } - userClients := NewUserClients(userID, m.log, m.oauthManager, m.httpClient, m.toolsCache) + var userClients *UserClients + if key.serviceAccount { + userClients = newServiceAccountClients(key.userID, m.log, m.httpClient, m.toolsCache) + } else { + userClients = NewUserClients(key.userID, m.log, m.oauthManager, m.httpClient, m.toolsCache) + } // Connect outside the manager lock so remote MCP handshakes do not block other users. // Cacheable client creation must not inherit request cancellation; a canceled @@ -181,10 +194,10 @@ func (m *ClientManager) createAndStoreUserClient(ctx context.Context, userID str // Check again in case another goroutine created the client while we were connecting. // On a forced refresh we intentionally replace (and close) any existing client. - if client, exists := m.clients[userID]; exists { + if client, exists := m.clients[key]; exists { if !forceRefresh { userClients.Close() - m.activity[userID] = time.Now() + m.activity[key] = time.Now() return client, client.InitialRemoteConnectErrors() } client.Close() @@ -192,30 +205,40 @@ func (m *ClientManager) createAndStoreUserClient(ctx context.Context, userID str // Store the client even if some servers failed to connect // This allows partial success - user gets tools from working servers - m.clients[userID] = userClients - m.activity[userID] = time.Now() + m.clients[key] = userClients + m.activity[key] = time.Now() return userClients, mcpErrors } -// getClientForUser gets or creates an MCP client for a specific user. -func (m *ClientManager) getClientForUser(ctx context.Context, userID string) (*UserClients, *Errors) { +// getClient gets or creates an MCP client bag for a specific identity and auth mode. +func (m *ClientManager) getClient(ctx context.Context, key clientKey) (*UserClients, *Errors) { m.clientsMu.Lock() - client, exists := m.clients[userID] + client, exists := m.clients[key] if exists { - m.activity[userID] = time.Now() + m.activity[key] = time.Now() m.clientsMu.Unlock() return client, client.InitialRemoteConnectErrors() } m.clientsMu.Unlock() - return m.createAndStoreUserClient(ctx, userID, false) + return m.createAndStoreUserClient(ctx, key, false) } // GetToolsForUser returns the tools available for a specific user, connecting to embedded server if session ID provided. func (m *ClientManager) GetToolsForUser(ctx context.Context, userID string) ([]llm.Tool, *Errors) { - // Get or create client for this user (connects to remote servers only) - userClient, initialErrors := m.getClientForUser(ctx, userID) + return m.getToolsForKey(ctx, clientKey{userID: userID}) +} + +// GetToolsForServiceAccount returns the tools for a service-account agent acting as +// botUserID. Servers without service account headers are excluded (fail closed). +func (m *ClientManager) GetToolsForServiceAccount(ctx context.Context, botUserID string) ([]llm.Tool, *Errors) { + return m.getToolsForKey(ctx, clientKey{userID: botUserID, serviceAccount: true}) +} + +func (m *ClientManager) getToolsForKey(ctx context.Context, key clientKey) ([]llm.Tool, *Errors) { + // Get or create the client bag for this identity (connects to remote servers only) + userClient, initialErrors := m.getClient(ctx, key) mcpErrors := cloneMCPErrors(initialErrors) // Embedded and plugin connects intentionally receive the raw cancelable ctx: @@ -223,12 +246,12 @@ func (m *ClientManager) GetToolsForUser(ctx context.Context, userID string) ([]l // them. Only the remote connect uses cacheableContext(ctx) (in // createAndStoreUserClient) because its result is cached across requests. if m.embeddedClient != nil { - ensuredSessionID, ensureErr := m.ensureEmbeddedSessionID(userID) + ensuredSessionID, ensureErr := m.ensureEmbeddedSessionID(key.userID) if ensureErr != nil { - m.log.Debug("Failed to ensure embedded session for user - embedded MCP tools will not be available", "userID", userID, "error", ensureErr) + m.log.Debug("Failed to ensure embedded session for user - embedded MCP tools will not be available", "userID", key.userID, "serviceAccount", key.serviceAccount, "error", ensureErr) } else if ensuredSessionID != "" { if embeddedErr := userClient.ConnectToEmbeddedServerIfAvailable(ctx, ensuredSessionID, m.embeddedClient, m.config.EmbeddedServer); embeddedErr != nil { - m.log.Debug("Failed to connect to embedded server for user - embedded MCP tools will not be available", "userID", userID, "sessionID", ensuredSessionID, "error", embeddedErr) + m.log.Debug("Failed to connect to embedded server for user - embedded MCP tools will not be available", "userID", key.userID, "serviceAccount", key.serviceAccount, "sessionID", ensuredSessionID, "error", embeddedErr) } } } @@ -237,7 +260,7 @@ func (m *ClientManager) GetToolsForUser(ctx context.Context, userID string) ([]l pluginSnap := m.snapshotEnabledPluginServers() for _, cfg := range pluginSnap { if connectErr := userClient.ConnectToPluginServer(ctx, cfg, m.sourcePluginAPI); connectErr != nil { - m.log.Error("Failed to connect to plugin MCP server", "userID", userID, "pluginID", cfg.PluginID, "error", connectErr) + m.log.Error("Failed to connect to plugin MCP server", "userID", key.userID, "serviceAccount", key.serviceAccount, "pluginID", cfg.PluginID, "error", connectErr) mcpErrors = appendMCPError(mcpErrors, connectErr) } } @@ -261,7 +284,7 @@ func (m *ClientManager) RefreshToolsForUser(ctx context.Context, userID string) m.InvalidateUserClients(userID) // Pre-warm the user client with a forced remote rediscovery; GetToolsForUser // then reuses this cached client rather than rebuilding it. - m.createAndStoreUserClient(ctx, userID, true) + m.createAndStoreUserClient(ctx, clientKey{userID: userID}, true) tools, mcpErrors := m.GetToolsForUser(ctx, userID) return tools, mcpErrors, nil @@ -273,12 +296,22 @@ func (m *ClientManager) invalidateSharedToolsCacheForRefresh() error { } var refreshErr error + invalidate := func(cacheID string) { + if err := m.toolsCache.InvalidateServer(cacheID); err != nil { + refreshErr = errors.Join(refreshErr, fmt.Errorf("failed to invalidate tools cache for server %s: %w", cacheID, err)) + } + } + for _, serverConfig := range m.config.Servers { - if !serverConfig.Enabled || serverConfig.BaseURL == "" || !shouldUseSharedToolsCache(serverConfig) { + if !serverConfig.Enabled || serverConfig.BaseURL == "" { continue } - if err := m.toolsCache.InvalidateServer(serverConfig.Name); err != nil { - refreshErr = errors.Join(refreshErr, fmt.Errorf("failed to invalidate tools cache for server %s: %w", serverConfig.Name, err)) + if sharedToolsCacheAllowedForServer(serverConfig) { + invalidate(serverConfig.Name) + } + // Service-account entries are always shared-cached, even for static-OAuth servers. + if serverConfig.HasServiceAccountAuth() { + invalidate(serviceAccountToolsCacheID(serverConfig.Name)) } } return refreshErr @@ -363,7 +396,7 @@ func (m *ClientManager) snapshotEnabledPluginServers() []PluginServerConfig { return out } -// InvalidateUserClients closes and removes cached MCP clients for a user. +// InvalidateUserClients closes and removes cached MCP clients for a user, in both auth modes. func (m *ClientManager) InvalidateUserClients(userID string) { if userID == "" { return @@ -372,11 +405,13 @@ func (m *ClientManager) InvalidateUserClients(userID string) { m.clientsMu.Lock() defer m.clientsMu.Unlock() - if uc, ok := m.clients[userID]; ok { - uc.Close() - delete(m.clients, userID) + for _, key := range []clientKey{{userID: userID}, {userID: userID, serviceAccount: true}} { + if uc, ok := m.clients[key]; ok { + uc.Close() + delete(m.clients, key) + } + delete(m.activity, key) } - delete(m.activity, userID) } // ProcessOAuthCallback processes the OAuth callback for a user diff --git a/mcp/client_manager_test.go b/mcp/client_manager_test.go index 7576ed5bb..8c99b5866 100644 --- a/mcp/client_manager_test.go +++ b/mcp/client_manager_test.go @@ -790,56 +790,54 @@ func TestClientManagerGetToolRetrievalOverridesDisabledServer(t *testing.T) { func TestClientManagerInvalidateUserClients(t *testing.T) { now := time.Now() + user1OAuth := clientKey{userID: "user-1"} + user1SA := clientKey{userID: "user-1", serviceAccount: true} + user2OAuth := clientKey{userID: "user-2"} + user2SA := clientKey{userID: "user-2", serviceAccount: true} + allKeys := []clientKey{user1OAuth, user1SA, user2OAuth, user2SA} + testCases := []struct { - name string - userID string - expectedClientKeys []string - expectedActivityKeys []string + name string + userID string + expectedKeys []clientKey }{ { - name: "removes existing user", - userID: "user-1", - expectedClientKeys: []string{"user-2"}, - expectedActivityKeys: []string{"user-2"}, + name: "removes both auth modes for the user", + userID: "user-1", + expectedKeys: []clientKey{user2OAuth, user2SA}, }, { - name: "ignores missing user", - userID: "missing-user", - expectedClientKeys: []string{"user-1", "user-2"}, - expectedActivityKeys: []string{"user-1", "user-2"}, + name: "ignores missing user", + userID: "missing-user", + expectedKeys: allKeys, }, { - name: "ignores empty user", - userID: "", - expectedClientKeys: []string{"user-1", "user-2"}, - expectedActivityKeys: []string{"user-1", "user-2"}, + name: "ignores empty user", + userID: "", + expectedKeys: allKeys, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { manager := &ClientManager{ - clients: map[string]*UserClients{ - "user-1": {clients: map[string]*Client{}}, - "user-2": {clients: map[string]*Client{}}, - }, - activity: map[string]time.Time{ - "user-1": now, - "user-2": now.Add(time.Minute), - }, + clients: map[clientKey]*UserClients{}, + activity: map[clientKey]time.Time{}, + } + for i, key := range allKeys { + manager.clients[key] = &UserClients{clients: map[string]*Client{}} + manager.activity[key] = now.Add(time.Duration(i) * time.Minute) } manager.InvalidateUserClients(tc.userID) - require.Len(t, manager.clients, len(tc.expectedClientKeys)) - for _, key := range tc.expectedClientKeys { + require.Len(t, manager.clients, len(tc.expectedKeys)) + require.Len(t, manager.activity, len(tc.expectedKeys)) + for _, key := range tc.expectedKeys { require.Contains(t, manager.clients, key) - } - require.Len(t, manager.activity, len(tc.expectedActivityKeys)) - for _, key := range tc.expectedActivityKeys { require.Contains(t, manager.activity, key) } - require.Equal(t, now.Add(time.Minute), manager.activity["user-2"]) + require.Equal(t, now.Add(2*time.Minute), manager.activity[user2OAuth]) }) } } @@ -850,8 +848,15 @@ func TestClientManagerInvalidateSharedToolsCacheForRefresh(t *testing.T) { cachedTools := map[string]*gomcp.Tool{ "tool": {Name: "tool"}, } - require.NoError(t, cache.SetTools("shared-server", "shared-server", "https://shared.example.com", cachedTools, time.Now())) - require.NoError(t, cache.SetTools("oauth-server", "oauth-server", "https://oauth.example.com", cachedTools, time.Now())) + saHeaders := map[string]string{"X-Service-Account-Token": "pat"} + for _, serverID := range []string{ + "shared-server", + "oauth-server", + serviceAccountToolsCacheID("sa-server"), + serviceAccountToolsCacheID("sa-oauth-server"), + } { + require.NoError(t, cache.SetTools(serverID, serverID, "https://"+serverID+".example.com", cachedTools, time.Now())) + } manager := &ClientManager{ config: Config{ @@ -859,6 +864,8 @@ func TestClientManagerInvalidateSharedToolsCacheForRefresh(t *testing.T) { {Name: "shared-server", BaseURL: "https://shared.example.com", Enabled: true}, {Name: "disabled-server", BaseURL: "https://disabled.example.com", Enabled: false}, {Name: "oauth-server", BaseURL: "https://oauth.example.com", Enabled: true, ClientID: "client-id"}, + {Name: "sa-server", BaseURL: "https://sa.example.com", Enabled: true, ServiceAccountHeaders: saHeaders}, + {Name: "sa-oauth-server", BaseURL: "https://sa-oauth.example.com", Enabled: true, ClientID: "client-id", ServiceAccountHeaders: saHeaders}, }, }, toolsCache: cache, @@ -868,6 +875,9 @@ func TestClientManagerInvalidateSharedToolsCacheForRefresh(t *testing.T) { require.Empty(t, cache.GetTools("shared-server")) require.NotEmpty(t, cache.GetTools("oauth-server")) + require.Empty(t, cache.GetTools(serviceAccountToolsCacheID("sa-server"))) + require.Empty(t, cache.GetTools(serviceAccountToolsCacheID("sa-oauth-server")), + "service account entries are invalidated even when static OAuth credentials are configured") } func TestClientManagerCreateAndStoreUserClientSetsInitialActivity(t *testing.T) { @@ -877,19 +887,19 @@ func TestClientManagerCreateAndStoreUserClientSetsInitialActivity(t *testing.T) manager := &ClientManager{ config: Config{}, log: client.Log, - clients: make(map[string]*UserClients), - activity: make(map[string]time.Time), + clients: make(map[clientKey]*UserClients), + activity: make(map[clientKey]time.Time), } before := time.Now() - userClients, mcpErrors := manager.createAndStoreUserClient(context.Background(), "user-1", false) + userClients, mcpErrors := manager.createAndStoreUserClient(context.Background(), clientKey{userID: "user-1"}, false) after := time.Now() require.NotNil(t, userClients) require.Nil(t, mcpErrors) - require.Contains(t, manager.clients, "user-1") + require.Contains(t, manager.clients, clientKey{userID: "user-1"}) - lastActivity, ok := manager.activity["user-1"] + lastActivity, ok := manager.activity[clientKey{userID: "user-1"}] require.True(t, ok) require.False(t, lastActivity.Before(before)) require.False(t, lastActivity.After(after)) @@ -904,15 +914,15 @@ func TestCacheableContextIgnoresParentCancellation(t *testing.T) { require.NoError(t, cacheCtx.Err()) } -func TestClientManagerGetClientForUserExistingClientConcurrent(t *testing.T) { +func TestClientManagerGetClientExistingClientConcurrent(t *testing.T) { before := time.Now() userClients := &UserClients{clients: map[string]*Client{}} manager := &ClientManager{ - clients: map[string]*UserClients{ - "user-1": userClients, + clients: map[clientKey]*UserClients{ + {userID: "user-1"}: userClients, }, - activity: map[string]time.Time{ - "user-1": before.Add(-time.Minute), + activity: map[clientKey]time.Time{ + {userID: "user-1"}: before.Add(-time.Minute), }, } @@ -927,9 +937,9 @@ func TestClientManagerGetClientForUserExistingClientConcurrent(t *testing.T) { defer wg.Done() <-start for range iterations { - got, errs := manager.getClientForUser(context.Background(), "user-1") + got, errs := manager.getClient(context.Background(), clientKey{userID: "user-1"}) if got != userClients || errs != nil { - t.Errorf("getClientForUser returned unexpected result: got=%p errs=%v", got, errs) + t.Errorf("getClient returned unexpected result: got=%p errs=%v", got, errs) return } } @@ -939,7 +949,7 @@ func TestClientManagerGetClientForUserExistingClientConcurrent(t *testing.T) { close(start) wg.Wait() - lastActivity, ok := manager.activity["user-1"] + lastActivity, ok := manager.activity[clientKey{userID: "user-1"}] require.True(t, ok) require.False(t, lastActivity.Before(before)) } @@ -959,11 +969,11 @@ func TestClientManagerMarkOAuthNeededInvalidatesUserClient(t *testing.T) { manager: func() *ClientManager { storeClient := &recordKVSetWithExpiryClient{} manager := &ClientManager{ - clients: map[string]*UserClients{ - "user-1": {clients: map[string]*Client{}}, + clients: map[clientKey]*UserClients{ + {userID: "user-1"}: {clients: map[string]*Client{}}, }, - activity: map[string]time.Time{ - "user-1": time.Now(), + activity: map[clientKey]time.Time{ + {userID: "user-1"}: time.Now(), }, } manager.oauthManager = NewOAuthManager(storeClient, "https://mattermost.example.com/plugins/mattermost-ai/oauth/callback", nil, nil) @@ -981,11 +991,11 @@ func TestClientManagerMarkOAuthNeededInvalidatesUserClient(t *testing.T) { setErr: model.NewAppError("test", "oauth_needed_store_failed", nil, "persist failed", http.StatusInternalServerError), } manager := &ClientManager{ - clients: map[string]*UserClients{ - "user-1": {clients: map[string]*Client{}}, + clients: map[clientKey]*UserClients{ + {userID: "user-1"}: {clients: map[string]*Client{}}, }, - activity: map[string]time.Time{ - "user-1": time.Now(), + activity: map[clientKey]time.Time{ + {userID: "user-1"}: time.Now(), }, } manager.oauthManager = NewOAuthManager(storeClient, "https://mattermost.example.com/plugins/mattermost-ai/oauth/callback", nil, nil) @@ -1000,11 +1010,11 @@ func TestClientManagerMarkOAuthNeededInvalidatesUserClient(t *testing.T) { { name: "still invalidates without oauth manager", manager: &ClientManager{ - clients: map[string]*UserClients{ - "user-1": {clients: map[string]*Client{}}, + clients: map[clientKey]*UserClients{ + {userID: "user-1"}: {clients: map[string]*Client{}}, }, - activity: map[string]time.Time{ - "user-1": time.Now(), + activity: map[clientKey]time.Time{ + {userID: "user-1"}: time.Now(), }, }, }, diff --git a/mcp/client_test.go b/mcp/client_test.go index e3fb9ab3b..709315675 100644 --- a/mcp/client_test.go +++ b/mcp/client_test.go @@ -194,9 +194,13 @@ func newTestPluginAPIWithSession(sessionID string) *pluginapi.Client { } func newTestPluginAPIForEmbeddedManager(userID, sessionID string) *pluginapi.Client { + return newTestPluginAPIForEmbeddedUser(&model.User{Id: userID, Roles: "system_user"}, sessionID) +} + +func newTestPluginAPIForEmbeddedUser(user *model.User, sessionID string) *pluginapi.Client { fakeAPI := &fixedPluginAPI{ kvGet: func(key string) ([]byte, *model.AppError) { - if key == buildEmbeddedSessionKey(userID) { + if key == buildEmbeddedSessionKey(user.Id) { return []byte(sessionID), nil } return nil, nil @@ -204,16 +208,13 @@ func newTestPluginAPIForEmbeddedManager(userID, sessionID string) *pluginapi.Cli sessionByID: map[string]*model.Session{ sessionID: { Id: sessionID, - UserId: userID, + UserId: user.Id, Token: "test-token", ExpiresAt: time.Now().Add(time.Hour).UnixMilli(), }, }, userByID: map[string]*model.User{ - userID: { - Id: userID, - Roles: "system_user", - }, + user.Id: user, }, } return pluginapi.NewClient(fakeAPI, nil) @@ -456,6 +457,67 @@ func TestNewClientDoesNotCachePartialPaginationOnError(t *testing.T) { require.Nil(t, cache.GetTools("paged")) } +func TestRemoteConnectionHeaders(t *testing.T) { + adminHeaders := map[string]string{"X-Admin": "admin-value"} + saHeaders := map[string]string{"Authorization": "Bearer service-account-pat"} + + testCases := []struct { + name string + userID string + serverConfig ServerConfig + serviceAccount bool + expectedHeaders map[string]string + }{ + { + name: "user mode ignores service account headers", + userID: "user-1", + serverConfig: ServerConfig{Name: "srv", Headers: adminHeaders, ServiceAccountHeaders: saHeaders}, + expectedHeaders: map[string]string{MMUserIDHeader: "user-1", "X-Admin": "admin-value"}, + }, + { + name: "service account headers layer on top of the bot ID and admin headers", + userID: "bot-1", + serverConfig: ServerConfig{ + Name: "srv", + Headers: map[string]string{"X-Admin": "admin-value", "Authorization": "Bearer admin"}, + ServiceAccountHeaders: saHeaders, + }, + serviceAccount: true, + expectedHeaders: map[string]string{ + MMUserIDHeader: "bot-1", + "X-Admin": "admin-value", + "Authorization": "Bearer service-account-pat", + }, + }, + { + // A blank header name or value must never reach the wire: net/http rejects the whole request. + name: "service account mode keeps valid headers alongside blank ones", + userID: "bot-1", + serverConfig: ServerConfig{ + Name: "srv", + Headers: adminHeaders, + ServiceAccountHeaders: map[string]string{ + "": "", + "X-Token": "", + "Authorization": "Bearer service-account-pat", + }, + }, + serviceAccount: true, + expectedHeaders: map[string]string{ + MMUserIDHeader: "bot-1", + "X-Admin": "admin-value", + "Authorization": "Bearer service-account-pat", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expectedHeaders, remoteConnectionHeaders(tc.userID, tc.serverConfig, tc.serviceAccount)) + }) + } +} + func TestNewClientErrorsOnEmptyRemoteToolCatalog(t *testing.T) { server := newEmptyToolsMCPServer() httpServer := startStreamableMCPServer(t, server) diff --git a/mcp/service_account_test.go b/mcp/service_account_test.go new file mode 100644 index 000000000..85b786b8d --- /dev/null +++ b/mcp/service_account_test.go @@ -0,0 +1,293 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package mcp + +import ( + "context" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/mattermost/mattermost/server/public/model" + plugintest "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/require" +) + +const ( + testSAHeaderName = "X-Service-Account-Token" + testSAHeaderValue = "service-account-pat" + testAdminHeader = "X-Admin" +) + +func testServiceAccountHeaders() map[string]string { + return map[string]string{testSAHeaderName: testSAHeaderValue} +} + +// requestHeaderRecorder captures the headers of every request reaching a test MCP server. +type requestHeaderRecorder struct { + mu sync.Mutex + headers []http.Header +} + +func (r *requestHeaderRecorder) record(h http.Header) { + r.mu.Lock() + defer r.mu.Unlock() + r.headers = append(r.headers, h) +} + +func (r *requestHeaderRecorder) snapshot() []http.Header { + r.mu.Lock() + defer r.mu.Unlock() + return append([]http.Header(nil), r.headers...) +} + +func startRecordingStreamableMCPServer(t *testing.T, server *gomcp.Server) (*httptest.Server, *requestHeaderRecorder) { + t.Helper() + + recorder := &requestHeaderRecorder{} + handler := gomcp.NewStreamableHTTPHandler(func(*http.Request) *gomcp.Server { + return server + }, nil) + httpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + recorder.record(r.Header.Clone()) + handler.ServeHTTP(w, r) + })) + t.Cleanup(httpServer.Close) + return httpServer, recorder +} + +// deadServerURL returns a local URL guaranteed to refuse connections. +func deadServerURL(t *testing.T) string { + t.Helper() + + httpServer := httptest.NewServer(http.NotFoundHandler()) + url := httpServer.URL + httpServer.Close() + return url +} + +func TestNewClientServiceAccountSendsStaticHeaders(t *testing.T) { + server := newTestMCPServer(0, "sa_tool") + httpServer, recorder := startRecordingStreamableMCPServer(t, server) + + client, err := newClient(context.Background(), "bot-1", ServerConfig{ + Name: "sa-server", + BaseURL: httpServer.URL, + Enabled: true, + Headers: map[string]string{testAdminHeader: "admin-value"}, + ServiceAccountHeaders: testServiceAccountHeaders(), + }, clientParams{ + log: newTestLogService(), + httpClient: httpServer.Client(), + toolsCache: newTestToolsCache(), + serviceAccount: true, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = client.Close() }) + + recorded := recorder.snapshot() + require.NotEmpty(t, recorded, "the service account connect must have reached the server") + for _, headers := range recorded { + require.Equal(t, testSAHeaderValue, headers.Get(testSAHeaderName)) + require.Equal(t, "admin-value", headers.Get(testAdminHeader)) + require.Equal(t, "bot-1", headers.Get(MMUserIDHeader), "X-Mattermost-UserID must be the acting bot user ID") + require.Empty(t, headers.Get("Authorization"), "service account mode must not attach an OAuth token") + } +} + +// Servers without service account headers are excluded, never dialed with anyone else's credentials. +func TestServiceAccountConnectToRemoteServersFailClosed(t *testing.T) { + liveHTTP := startStreamableMCPServer(t, newTestMCPServer(0, "sa_tool")) + + bag := newServiceAccountClients("bot-1", newTestLogService(), liveHTTP.Client(), newTestToolsCache()) + t.Cleanup(bag.Close) + + mcpErrors := bag.ConnectToRemoteServers(context.Background(), []ServerConfig{ + { + Name: "sa-server", + BaseURL: liveHTTP.URL, + Enabled: true, + ServiceAccountHeaders: testServiceAccountHeaders(), + }, + // A refused URL: any dial attempt would surface as a connect error below. + {Name: "no-sa-server", BaseURL: deadServerURL(t), Enabled: true}, + }, false) + + connectedIDs := make([]string, 0, 1) + for _, entry := range bag.snapshotClients() { + connectedIDs = append(connectedIDs, entry.serverID) + } + require.ElementsMatch(t, []string{"sa-server"}, connectedIDs) + require.Nil(t, mcpErrors, "servers excluded by fail-closed filtering are not failures") +} + +// The two auth modes keep separate tools cache entries for the same server. +func TestServiceAccountToolsCacheIsolation(t *testing.T) { + const serverName = "sa-server" + const sentinelTool = "sentinel_tool" + saCacheID := serviceAccountToolsCacheID(serverName) + + httpServer := startStreamableMCPServer(t, newTestMCPServer(0, "live_tool")) + cache := newTestToolsCache() + require.NoError(t, cache.SetTools(serverName, serverName, httpServer.URL, map[string]*gomcp.Tool{ + sentinelTool: {Name: sentinelTool, Description: "Stale cached tool"}, + }, time.Now())) + + client, err := newClient(context.Background(), "bot-1", ServerConfig{ + Name: serverName, + BaseURL: httpServer.URL, + Enabled: true, + ServiceAccountHeaders: testServiceAccountHeaders(), + }, clientParams{ + log: newTestLogService(), + httpClient: httpServer.Client(), + toolsCache: cache, + serviceAccount: true, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = client.Close() }) + + require.ElementsMatch(t, []string{"live_tool"}, cachedToolNames(client.Tools())) + require.ElementsMatch(t, []string{sentinelTool}, cachedToolNames(cache.GetTools(serverName))) + require.ElementsMatch(t, []string{"live_tool"}, cachedToolNames(cache.GetTools(saCacheID))) +} + +func TestServiceAccountUsesSharedCacheDespiteStaticOAuthCreds(t *testing.T) { + var listCalls atomic.Int32 + server := newStaticToolListMCPServer(0, "sa_tool") + server.AddReceivingMiddleware(func(next gomcp.MethodHandler) gomcp.MethodHandler { + return func(ctx context.Context, method string, req gomcp.Request) (gomcp.Result, error) { + if method == testListToolsMethod { + listCalls.Add(1) + } + return next(ctx, method, req) + } + }) + httpServer := startStreamableMCPServer(t, server) + cache := newTestToolsCache() + + // Static OAuth creds disable the shared cache in user mode, but SA creds are identical per connection. + serverConfig := ServerConfig{ + Name: "sa-server", + BaseURL: httpServer.URL, + Enabled: true, + ClientID: "client-id", + ClientSecret: "client-secret", + ServiceAccountHeaders: testServiceAccountHeaders(), + } + params := clientParams{ + log: newTestLogService(), + httpClient: httpServer.Client(), + toolsCache: cache, + serviceAccount: true, + } + + first, err := newClient(context.Background(), "bot-1", serverConfig, params) + require.NoError(t, err) + t.Cleanup(func() { _ = first.Close() }) + require.Equal(t, int32(1), listCalls.Load()) + require.ElementsMatch(t, []string{"sa_tool"}, cachedToolNames(cache.GetTools(serviceAccountToolsCacheID(serverConfig.Name)))) + + second, err := newClient(context.Background(), "bot-1", serverConfig, params) + require.NoError(t, err) + t.Cleanup(func() { _ = second.Close() }) + + require.Equal(t, int32(1), listCalls.Load(), "the second service account connect must be served from the cache") + require.ElementsMatch(t, []string{"sa_tool"}, cachedToolNames(second.Tools())) +} + +// The two auth modes must pool separately even for the same user ID. +func TestClientManagerModeIsolationPooling(t *testing.T) { + pluginAPI := newTestPluginAPIForEmbeddedManager("bot-1", "session-1") + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, pluginAPI.Log, pluginAPI, newTestOAuthManager(), nil, http.DefaultClient, nil) + t.Cleanup(m.Close) + + _, userErrors := m.GetToolsForUser(context.Background(), "bot-1") + require.Nil(t, userErrors) + _, saErrors := m.GetToolsForServiceAccount(context.Background(), "bot-1") + require.Nil(t, saErrors) + + require.Len(t, m.clients, 2) + userBag := m.clients[clientKey{userID: "bot-1"}] + saBag := m.clients[clientKey{userID: "bot-1", serviceAccount: true}] + require.NotNil(t, userBag) + require.NotNil(t, saBag) + require.NotSame(t, userBag, saBag) + + require.False(t, userBag.serviceAccount) + require.NotNil(t, userBag.oauthManager) + require.True(t, saBag.serviceAccount) + require.Nil(t, saBag.oauthManager, "service account bags must have no OAuth machinery") +} + +func TestClientManagerServiceAccountEmbeddedSessionAsBot(t *testing.T) { + runCtx, cancelRun := context.WithCancel(context.Background()) + t.Cleanup(cancelRun) + + pluginAPI := newTestPluginAPIForEmbeddedUser(&model.User{Id: "bot-1", Roles: "system_user", IsBot: true}, "session-1") + embeddedServer := &fakeEmbeddedMCPServer{ctx: runCtx, server: newTestMCPServer(0, "search_users")} + m := NewClientManager(Config{ + IdleTimeoutMinutes: 30, + EmbeddedServer: EmbeddedServerConfig{ + Enabled: true, + ToolConfigs: []ToolConfig{{Name: "search_users", Policy: ToolPolicyAsk, Enabled: true}}, + }, + }, pluginAPI.Log, pluginAPI, nil, embeddedServer, http.DefaultClient, nil) + t.Cleanup(m.Close) + + tools, mcpErrors := m.GetToolsForServiceAccount(context.Background(), "bot-1") + require.Nil(t, mcpErrors) + requireToolNames(t, tools, "mattermost__search_users") +} + +func TestClientManagerServiceAccountPluginServerGetsBotUserIDHeader(t *testing.T) { + target := newFakePluginMCPServer(t, 1) + t.Cleanup(target.Close) + + var mu sync.Mutex + var recordedUserIDs []string + mockAPI := &fakePluginHTTPClient{ + pluginHTTP: func(req *http.Request) *http.Response { + mu.Lock() + recordedUserIDs = append(recordedUserIDs, req.Header.Get(MMUserIDHeader)) + mu.Unlock() + + rec := httptest.NewRecorder() + target.Config.Handler.ServeHTTP(rec, req) + return rec.Result() + }, + } + + pluginTestAPI := &plugintest.API{} + setupTestLogger(pluginTestAPI) + client := pluginapi.NewClient(pluginTestAPI, nil) + + m := NewClientManager(Config{IdleTimeoutMinutes: 30}, client.Log, client, nil, nil, nil, mockAPI) + t.Cleanup(m.Close) + m.RegisterPluginServer(PluginServerConfig{PluginID: "com.example.mcp", Name: "Example", Path: "/mcp", Enabled: true}) + + tools, mcpErrors := m.GetToolsForServiceAccount(context.Background(), "bot-1") + require.Nil(t, mcpErrors) + require.Len(t, tools, 1) + + mu.Lock() + defer mu.Unlock() + require.NotEmpty(t, recordedUserIDs) + for _, userID := range recordedUserIDs { + require.Equal(t, "bot-1", userID, "plugin MCP servers must see the acting bot user ID") + } +} + +func cachedToolNames(tools map[string]*gomcp.Tool) []string { + names := make([]string, 0, len(tools)) + for name := range tools { + names = append(names, name) + } + return names +} diff --git a/mcp/user_clients.go b/mcp/user_clients.go index 1d72119c5..522215395 100644 --- a/mcp/user_clients.go +++ b/mcp/user_clients.go @@ -42,6 +42,9 @@ type UserClients struct { // user client is cached; otherwise callers only see those errors once (first // GetToolsForUser) and lose stable auth-required state on subsequent requests. initialRemoteConnectErrors *Errors + // serviceAccount marks a service-account bag: userID is the acting bot's user ID + // and oauthManager is nil. + serviceAccount bool } type userClientSnapshot struct { @@ -60,6 +63,14 @@ func NewUserClients(userID string, log pluginapi.LogService, oauthManager *OAuth } } +// newServiceAccountClients creates a client bag for service-account mode acting as +// botUserID; it has no OAuthManager, so no OAuth flow can occur. +func newServiceAccountClients(botUserID string, log pluginapi.LogService, httpClient *http.Client, toolsCache *ToolsCache) *UserClients { + userClients := NewUserClients(botUserID, log, nil, httpClient, toolsCache) + userClients.serviceAccount = true + return userClients +} + // ConnectToRemoteServers initializes connections to remote MCP servers. func (c *UserClients) ConnectToRemoteServers(ctx context.Context, servers []ServerConfig, forceRefresh bool) *Errors { if len(servers) == 0 { @@ -76,6 +87,14 @@ func (c *UserClients) ConnectToRemoteServers(ctx context.Context, servers []Serv continue } + // Fail closed: no service account credential means the server is excluded, + // never a fallback to user OAuth. + if c.serviceAccount && !serverConfig.HasServiceAccountAuth() { + c.log.Debug("Skipping MCP server without service account headers in service account mode", + "userID", c.userID, "serverID", serverConfig.Name) + continue + } + if err := c.connectToServer(ctx, serverConfig.Name, serverConfig, forceRefresh); err != nil { // Initialize errors struct if needed if mcpErrors == nil { @@ -141,7 +160,14 @@ func (c *UserClients) ConnectToEmbeddedServerIfAvailable(ctx context.Context, se // connectToServer establishes a connection to a single server func (c *UserClients) connectToServer(ctx context.Context, serverID string, serverConfig ServerConfig, forceRefresh bool) error { - serverClient, err := NewClient(ctx, c.userID, serverConfig, c.log, c.oauthManager, c.httpClient, c.toolsCache, forceRefresh) + serverClient, err := newClient(ctx, c.userID, serverConfig, clientParams{ + log: c.log, + oauthManager: c.oauthManager, + httpClient: c.httpClient, + toolsCache: c.toolsCache, + forceRefresh: forceRefresh, + serviceAccount: c.serviceAccount, + }) if err != nil { return err } diff --git a/public/bridgeclient/README.md b/public/bridgeclient/README.md index 89078425d..7028a60dd 100644 --- a/public/bridgeclient/README.md +++ b/public/bridgeclient/README.md @@ -156,6 +156,24 @@ response, err := client.AgentCompletion("bot-user-id", request) If not using built-in permission checks, your plugin must verify permissions before making requests. +## Service Account Agents + +An agent can be configured (on its MCPs tab) to use **service account authentication**: MCP tool +calls run with admin-configured service account credentials and the agent's bot identity instead +of per-user credentials. For bridge callers this changes what `UserID` means: + +- **The tool catalog is the agent's, not the user's.** For a service account agent, + `GetAgentTools` and `AllowedTools` resolution use the agent's service account catalog, which is + identical for every caller. External MCP servers without service account headers configured are + excluded (fail closed) — they never appear in discovery and never execute. +- **`UserID` is still used for permission checks and attribution.** Passing `UserID` still + enforces the agent's user and channel access rules and is recorded in token usage logs; it just + no longer selects credentials. +- **`UserID` is still required for `AllowedTools`**, in both modes. + +Service account authentication requires a license. Without one, an agent flagged for it behaves +like any other agent: the caller-asserted `user_id` selects per-user MCP credentials. + ## Token Usage Dimensions Bridge callers can optionally provide `Operation` and `OperationSubType` in `CompletionRequest` to customize token usage categorization in logs. diff --git a/store/agents.go b/store/agents.go index 7bd1dbacb..bf28b1bed 100644 --- a/store/agents.go +++ b/store/agents.go @@ -20,7 +20,7 @@ const agentSelectColumns = `ID, BotUserID, CreatorID, DisplayName, Username, Ser EnabledTools, AutoEnableNewMCPTools, mcp_dynamic_tool_loading, Model, EnableVision, DisableTools, EnabledNativeTools, ReasoningEnabled, ReasoningEffort, ThinkingBudget, StructuredOutputEnabled, - MaxToolTurns, + MaxToolTurns, UseServiceAccountAuth, CreateAt, UpdateAt, DeleteAt` // mustMarshalSlice marshals a string slice to JSON, returning "[]" on nil/empty or error. @@ -120,6 +120,7 @@ type agentRow struct { ThinkingBudget int `db:"thinkingbudget"` StructuredOutputEnabled bool `db:"structuredoutputenabled"` MaxToolTurns int `db:"maxtoolturns"` + UseServiceAccountAuth bool `db:"useserviceaccountauth"` CreateAt int64 `db:"createat"` UpdateAt int64 `db:"updateat"` DeleteAt int64 `db:"deleteat"` @@ -147,6 +148,7 @@ func (r *agentRow) toBotConfig() (*llm.BotConfig, error) { ThinkingBudget: r.ThinkingBudget, StructuredOutputEnabled: r.StructuredOutputEnabled, MaxToolTurns: r.MaxToolTurns, + UseServiceAccountAuth: r.UseServiceAccountAuth, CreateAt: r.CreateAt, UpdateAt: r.UpdateAt, DeleteAt: r.DeleteAt, @@ -191,9 +193,9 @@ func (s *Store) CreateAgent(cfg *llm.BotConfig) error { EnabledTools, AutoEnableNewMCPTools, mcp_dynamic_tool_loading, Model, EnableVision, DisableTools, EnabledNativeTools, ReasoningEnabled, ReasoningEffort, ThinkingBudget, StructuredOutputEnabled, - MaxToolTurns, + MaxToolTurns, UseServiceAccountAuth, CreateAt, UpdateAt, DeleteAt - ) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24,$25,$26,$27,$28)`, + ) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24,$25,$26,$27,$28,$29)`, cfg.ID, cfg.BotUserID, cfg.CreatorID, @@ -219,6 +221,7 @@ func (s *Store) CreateAgent(cfg *llm.BotConfig) error { cfg.ThinkingBudget, cfg.StructuredOutputEnabled, cfg.MaxToolTurns, + cfg.UseServiceAccountAuth, cfg.CreateAt, cfg.UpdateAt, cfg.DeleteAt, @@ -345,8 +348,9 @@ func (s *Store) UpdateAgent(cfg *llm.BotConfig) error { ThinkingBudget = $20, StructuredOutputEnabled = $21, MaxToolTurns = $22, - UpdateAt = $23 - WHERE ID = $24 AND DeleteAt = 0`, + UseServiceAccountAuth = $23, + UpdateAt = $24 + WHERE ID = $25 AND DeleteAt = 0`, cfg.DisplayName, cfg.Name, cfg.ServiceID, @@ -369,6 +373,7 @@ func (s *Store) UpdateAgent(cfg *llm.BotConfig) error { cfg.ThinkingBudget, cfg.StructuredOutputEnabled, cfg.MaxToolTurns, + cfg.UseServiceAccountAuth, cfg.UpdateAt, cfg.ID, ) diff --git a/store/agents_test.go b/store/agents_test.go index 9beb39ba0..4dec66cfb 100644 --- a/store/agents_test.go +++ b/store/agents_test.go @@ -43,6 +43,7 @@ func testAgent(creatorID, username, displayName string) *llm.BotConfig { ThinkingBudget: 10000, StructuredOutputEnabled: true, MaxToolTurns: 42, + UseServiceAccountAuth: true, } } @@ -102,6 +103,7 @@ func TestAgentCreateAndGet(t *testing.T) { assert.Equal(t, 10000, fetched.ThinkingBudget) assert.True(t, fetched.StructuredOutputEnabled) assert.Equal(t, 42, fetched.MaxToolTurns) + assert.True(t, fetched.UseServiceAccountAuth) } // TestAgentMaxToolTurnsDefaultsToThirty verifies that the SQL DEFAULT 30 supplied @@ -222,6 +224,7 @@ func TestAgentUpdate(t *testing.T) { agent.ChannelIDs = []string{"ch-3"} agent.EnabledMCPTools = nil agent.ServiceID = "svc-2" + agent.UseServiceAccountAuth = false require.NoError(t, s.UpdateAgent(agent)) @@ -238,6 +241,7 @@ func TestAgentUpdate(t *testing.T) { assert.Equal(t, []string{"ch-3"}, fetched.ChannelIDs) assert.Nil(t, fetched.EnabledMCPTools) assert.Equal(t, "svc-2", fetched.ServiceID) + assert.False(t, fetched.UseServiceAccountAuth) // Immutable fields should not change assert.Equal(t, agent.CreatorID, fetched.CreatorID) diff --git a/store/migrations/000010_user_agent_service_account_auth.down.sql b/store/migrations/000010_user_agent_service_account_auth.down.sql new file mode 100644 index 000000000..8ea16addd --- /dev/null +++ b/store/migrations/000010_user_agent_service_account_auth.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE Agents_UserAgents + DROP COLUMN IF EXISTS UseServiceAccountAuth; diff --git a/store/migrations/000010_user_agent_service_account_auth.up.sql b/store/migrations/000010_user_agent_service_account_auth.up.sql new file mode 100644 index 000000000..a627765c4 --- /dev/null +++ b/store/migrations/000010_user_agent_service_account_auth.up.sql @@ -0,0 +1,2 @@ +ALTER TABLE Agents_UserAgents + ADD COLUMN IF NOT EXISTS UseServiceAccountAuth BOOLEAN NOT NULL DEFAULT false; diff --git a/store/migrations/reviews/000010_user_agent_service_account_auth.md b/store/migrations/reviews/000010_user_agent_service_account_auth.md new file mode 100644 index 000000000..27233606e --- /dev/null +++ b/store/migrations/reviews/000010_user_agent_service_account_auth.md @@ -0,0 +1,57 @@ +# Schema Migration Review: 000010 — Add UseServiceAccountAuth to Agents_UserAgents + +> **Context:** Persists the per-agent, all-or-nothing Service Account authentication flag: when set, the agent reaches external MCP servers with admin-configured service-account headers and acts as its own bot user for embedded/plugin MCP access. `Agents_UserAgents` is admin-configured and bounded (typically tens of rows). + +## Schema Changes +- [ ] New table(s): — +- [x] New column(s) on `Agents_UserAgents`: `UseServiceAccountAuth BOOLEAN NOT NULL DEFAULT false` +- [ ] New index(es): — +- [ ] Modified column(s): — +- [ ] Dropped object(s): — + +## Safety Analysis + +| Check | Status | Notes | +|-------|--------|-------| +| No ALTER COLUMN TYPE | ✅ | Only ADD COLUMN. | +| CREATE INDEX uses CONCURRENTLY | N/A | No indexes. | +| DROP INDEX uses CONCURRENTLY | N/A | No DROP INDEX. | +| No FOREIGN KEY via ALTER TABLE | ✅ | No FKs. | +| No full-table DELETE/UPDATE | ✅ | No backfill UPDATE; column default supplies the value for existing rows. | +| morph:nontransactional where needed | N/A | No CONCURRENTLY. | +| Down migration exists | ✅ | Drops the column. | +| Transactional/nontransactional split correct | ✅ | All-transactional. | + +## Postgres-Specific Notes +- `ADD COLUMN ... NOT NULL DEFAULT false` is metadata-only on PostgreSQL 11+ (constant default → no table rewrite). ✅ + +## Backwards Compatibility +- Compatible with previous ESR: Yes (plugin-owned). +- Can previous Mattermost version run with new schema: Yes — older plugin code paths simply ignore the column. +- Impact if not compatible: N/A. + +## Table Locks & Impact +- Tables affected: `Agents_UserAgents`. +- Lock types acquired: + - `ALTER TABLE … ADD COLUMN`: ACCESS EXCLUSIVE on `Agents_UserAgents`. Metadata-only because the default is constant, so the lock is held for a negligible amount of work — but the lock is still requested up front and waits for any transaction already touching the table, and readers arriving during that wait queue behind it. +- Impact to concurrent operations: Negligible once the lock is granted; bounded by lock-wait duration if a long-running transaction holds a conflicting lock on `Agents_UserAgents`. + +## Zero Downtime +- Possible: Yes. +- Reason: Metadata-only ADD COLUMN on an admin-managed table; no table rewrite, so the ACCESS EXCLUSIVE lock is acquired once all preceding conflicting transactions on the table finish, then released almost immediately. + +## Large-Dataset Testing Recommendation +- **Recommended: No** +- Reason: `Agents_UserAgents` is admin-configured and small. + +## Test Results + +| DB | Table Size | Row Count | Duration | Instance | +|----|-----------|-----------|----------|----------| +| PostgreSQL | | | | | + +## SQL Queries +```sql +ALTER TABLE Agents_UserAgents + ADD COLUMN IF NOT EXISTS UseServiceAccountAuth BOOLEAN NOT NULL DEFAULT false; +``` diff --git a/store/store_test.go b/store/store_test.go index 7f356aef2..b41152979 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -176,7 +176,7 @@ func TestRunMigrations(t *testing.T) { err := s.db.Get(&count, ` SELECT COUNT(*) FROM Agents_DB_Migrations`) require.NoError(t, err) - assert.Equal(t, 9, count, "Should have 9 migration records") + assert.Equal(t, 10, count, "Should have 10 migration records") }, }, { diff --git a/webapp/src/bots.tsx b/webapp/src/bots.tsx index aae7ad3b8..13df43db4 100644 --- a/webapp/src/bots.tsx +++ b/webapp/src/bots.tsx @@ -32,6 +32,9 @@ export interface LLMBot { enabledMCPTools: EnabledMCPTool[] | null; autoEnableNewMCPTools: boolean; + // Optional so the UI degrades to the non-SA UX when the server predates the field. + useServiceAccountAuth?: boolean; + // isDefault marks the system-wide default agent. Optional: older servers // omit it, in which case we fall back to list ordering. isDefault?: boolean; diff --git a/webapp/src/components/agents/agent_config_view.test.tsx b/webapp/src/components/agents/agent_config_view.test.tsx index ee9f296ca..f4f97c01e 100644 --- a/webapp/src/components/agents/agent_config_view.test.tsx +++ b/webapp/src/components/agents/agent_config_view.test.tsx @@ -93,11 +93,21 @@ jest.mock('./tabs/access_tab', () => ({ jest.mock('./tabs/mcps_tab', () => ({ __esModule: true, default: ({ + useServiceAccountAuth, + onChange, onReconcileEnabledTools, }: { + useServiceAccountAuth: boolean; + onChange: (updates: {useServiceAccountAuth?: boolean}) => void; onReconcileEnabledTools?: (cleaned: EnabledTool[]) => void; }) => ( <> + onChange({useServiceAccountAuth: e.target.checked})} + />