Add service account authentication for external MCP servers - #937
Add service account authentication for external MCP servers#937nickmisasi wants to merge 17 commits into
Conversation
… flag Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
…context Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 6 failuresOPENAI1. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
6. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection
Anthropic
❌ Failed EvaluationsShow 7 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
2. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection
This comment was automatically generated by the eval CI pipeline. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAgents now support opt-in service-account authentication across persistence, MCP execution, bridge and conversation flows, administration, UI configuration, and token attribution. External MCP servers without valid service-account headers are excluded, while agent identity is preserved for execution and logging. ChangesService-account authentication
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
webapp/src/components/system_console/mcp_servers.tsx (1)
126-130: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winService-account credentials render in cleartext.
The section help text advertises this field for a personal access token, yet the value input is plain text while the adjacent OAuth Client Secret (line 359) uses
type='password'. Consider threading amaskValuesprop throughHeaderMapEditorand enabling it for the service-account map so the credential isn't shoulder-surfable in the System Console.🔒 Sketch
const HeaderMapEditor = ({ headers, onChange, + maskValues = false, }: { headers: {[key: string]: string}; onChange: (headers: {[key: string]: string}) => void; + maskValues?: boolean; }) => {<HeaderInput + type={maskValues ? 'password' : 'text'} placeholder={intl.formatMessage({defaultMessage: 'Value'})}<HeaderMapEditor headers={config.serviceAccountHeaders} + maskValues={true} onChange={(serviceAccountHeaders) => onChange(serverIndex, {...config, serviceAccountHeaders})} />Note this would also require updating the positional
Valueplaceholder lookups inwebapp/src/components/system_console/mcp_servers.test.tsx(lines 142-143) if you switch input types.Also applies to: 303-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/system_console/mcp_servers.tsx` around lines 126 - 130, Update HeaderMapEditor and its service-account header map usage to accept and apply a maskValues prop, rendering credential value inputs with password masking when enabled while preserving plain-text behavior elsewhere. Enable masking for the service-account credentials, and update the affected mcp_servers tests to locate the Value inputs without relying on their changed input type or position.webapp/src/components/agents/tabs/mcps_tab.tsx (1)
149-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
aria-checkedis ignored on an implicitrole='button'.Assistive tech drops
aria-checkedunless the element exposes a checkable role, so the on/off state isn't announced. Thearia-labelstill flips between "Enable"/"Disable", so the control remains usable — hence optional — butrole='switch'makes the state programmatically correct.♻️ Proposed tweak
<ServerToggle type='button' + role='switch' aria-label={serverToggleLabel} aria-checked={allOn}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/agents/tabs/mcps_tab.tsx` around lines 149 - 158, Update the ServerToggle element to expose a checkable switch role so its existing aria-checked={allOn} state is announced by assistive technology. Preserve the current label, toggle handler, disabled behavior, and visual state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/mcp_config.go`:
- Around line 122-148: Update EffectiveServiceAccountHeaders to trim each header
name and value before storing them, while continuing to skip entries whose
trimmed name or value is empty. Use the trimmed strings as the map key and value
so HasServiceAccountAuth only exposes valid credentials, and add coverage in the
existing table-driven tests for padded headers.
In `@mcp/client_manager.go`:
- Around line 105-115: Update ClientManager.closeIdleClients to delete the
matching m.activity entry whenever an idle client is removed from m.clients,
using the same key and preserving the existing timeout and client-closing
behavior.
In `@store/migrations/reviews/000010_user_agent_service_account_auth.md`:
- Around line 33-41: Update the “Table Locks & Impact” and “Zero Downtime”
sections to state that the metadata-only ADD COLUMN still acquires ACCESS
EXCLUSIVE and may wait for active transactions or conflicting locks. Remove
“returns instantly” and unqualified “Negligible” claims, and describe the impact
as dependent on lock-wait duration.
In `@webapp/src/components/system_console/mcp_servers.tsx`:
- Around line 101-108: Update updateHeader so renaming a key rebuilds the
headers map in its existing Object.entries order, replacing oldKey with newKey
at the original position rather than deleting and appending it. Preserve the
current value update and onChange behavior, including the unchanged-key path, so
rows rendered by index remain stable.
---
Nitpick comments:
In `@webapp/src/components/agents/tabs/mcps_tab.tsx`:
- Around line 149-158: Update the ServerToggle element to expose a checkable
switch role so its existing aria-checked={allOn} state is announced by assistive
technology. Preserve the current label, toggle handler, disabled behavior, and
visual state.
In `@webapp/src/components/system_console/mcp_servers.tsx`:
- Around line 126-130: Update HeaderMapEditor and its service-account header map
usage to accept and apply a maskValues prop, rendering credential value inputs
with password masking when enabled while preserving plain-text behavior
elsewhere. Enable masking for the service-account credentials, and update the
affected mcp_servers tests to locate the Value inputs without relying on their
changed input type or position.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7c279556-1ca7-4c98-9fd5-3d2103f02588
📒 Files selected for processing (58)
api/api.goapi/api_agents.goapi/api_agents_test.goapi/api_channel_analysis_test.goapi/api_config_test.goapi/api_llm_bridge.goapi/api_llm_bridge_service_account_test.goapi/api_llm_bridge_test.goapi/api_no_tools_test.goapi/api_test.gobots/bot.goconfig/mcp_config.goconfig/mcp_config_test.goconversations/bot_channel_tool_filter_test.goconversations/conversations_test.goconversations/dm_conversation_test.goconversations/handle_messages.goconversations/service_account_test.goconversations/single_build_test.godocs/admin_guide.mddocs/features/managing_agents.mddocs/features/multiplayer_tool_calling.mddocs/user_guide.mde2e/helpers/agent-api.tsllm/configuration.gollm/configuration_test.gollm/context.gollm/token_tracking.gollm/token_tracking_test.gollm/token_usage_fields.gollmcontext/llm_context.gollmcontext/llm_context_license_test.gollmcontext/llm_context_test.gomcp/client.gomcp/client_embedded_oauth_test.gomcp/client_manager.gomcp/client_manager_test.gomcp/client_test.gomcp/service_account_test.gomcp/user_clients.gopublic/bridgeclient/README.mdstore/agents.gostore/agents_test.gostore/migrations/000010_user_agent_service_account_auth.down.sqlstore/migrations/000010_user_agent_service_account_auth.up.sqlstore/migrations/reviews/000010_user_agent_service_account_auth.mdstore/store_test.gowebapp/src/bots.tsxwebapp/src/components/agents/agent_config_view.test.tsxwebapp/src/components/agents/agent_config_view.tsxwebapp/src/components/agents/tabs/mcps_tab.test.tsxwebapp/src/components/agents/tabs/mcps_tab.tsxwebapp/src/components/rhs/rhs_header.test.tsxwebapp/src/components/rhs/rhs_header.tsxwebapp/src/components/system_console/mcp_servers.test.tsxwebapp/src/components/system_console/mcp_servers.tsxwebapp/src/i18n/en.jsonwebapp/src/types/agents.ts
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
llmcontext/llm_context.go (1)
272-292: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRecord service-account mode outside the optional MCP-provider branch.
mcpToolProvideris nullable, butc.ToolAuthModeis set only when that provider exists. A licensed service-account agent without an MCP provider therefore reaches token tracking with the default user mode, causingacting_user_idto be attributed to the requesting user instead of the bot account. Setc.ToolAuthModeimmediately afteruseServiceAccountis computed, before the provider check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llmcontext/llm_context.go` around lines 272 - 292, Set c.ToolAuthMode immediately after useServiceAccount is computed, before any nullable mcpToolProvider check, assigning service-account mode when enabled and preserving user mode otherwise. Remove the later service-account assignment inside the provider branch while keeping the existing tool-fetch behavior unchanged.api/api_llm_bridge.go (1)
318-323: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the new bridge error through i18n.
Line 321 adds a client-visible error as a hardcoded English string. Route it through the repository’s i18n extraction or established API error-key mechanism.
As per coding guidelines: “All new user-facing strings must go through i18n extraction instead of being hardcoded.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/api_llm_bridge.go` around lines 318 - 323, Update the new error returned by the tool-hook validation in the request bridge to use the repository’s established i18n extraction or API error-key mechanism instead of a hardcoded English message. Preserve the existing BadRequest status and validation behavior in the service-account catalog check.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api/api_llm_bridge.go`:
- Around line 318-323: Update the new error returned by the tool-hook validation
in the request bridge to use the repository’s established i18n extraction or API
error-key mechanism instead of a hardcoded English message. Preserve the
existing BadRequest status and validation behavior in the service-account
catalog check.
In `@llmcontext/llm_context.go`:
- Around line 272-292: Set c.ToolAuthMode immediately after useServiceAccount is
computed, before any nullable mcpToolProvider check, assigning service-account
mode when enabled and preserving user mode otherwise. Remove the later
service-account assignment inside the provider branch while keeping the existing
tool-fetch behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1a2cf38f-75d0-4f24-b82b-b890dac696f9
📒 Files selected for processing (34)
api/api_llm_bridge.goapi/api_llm_bridge_service_account_test.goapi/api_llm_bridge_test.goapi/api_test.gobots/bot.goconfig/mcp_config.goconfig/mcp_config_test.goconversations/bot_channel_tool_filter_test.goconversations/handle_messages.goconversations/service_account_test.goconversations/single_build_test.gollm/configuration.gollm/configuration_test.gollm/context.gollm/token_tracking.gollm/token_tracking_test.gollm/token_usage_fields.gollmcontext/llm_context.gollmcontext/llm_context_license_test.gollmcontext/llm_context_test.gomcp/client.gomcp/client_manager.gomcp/client_test.gomcp/service_account_test.gomcp/user_clients.gowebapp/src/bots.tsxwebapp/src/components/agents/agent_config_view.test.tsxwebapp/src/components/agents/tabs/mcps_tab.test.tsxwebapp/src/components/agents/tabs/mcps_tab.tsxwebapp/src/components/rhs/rhs_header.test.tsxwebapp/src/components/rhs/rhs_header.tsxwebapp/src/components/system_console/mcp_servers.test.tsxwebapp/src/components/system_console/mcp_servers.tsxwebapp/src/types/agents.ts
💤 Files with no reviewable changes (3)
- conversations/bot_channel_tool_filter_test.go
- conversations/single_build_test.go
- llm/token_tracking_test.go
🚧 Files skipped from review as they are similar to previous changes (27)
- llm/context.go
- webapp/src/components/rhs/rhs_header.tsx
- llm/configuration.go
- webapp/src/bots.tsx
- webapp/src/types/agents.ts
- conversations/handle_messages.go
- bots/bot.go
- webapp/src/components/agents/agent_config_view.test.tsx
- webapp/src/components/rhs/rhs_header.test.tsx
- mcp/client_test.go
- webapp/src/components/system_console/mcp_servers.test.tsx
- llm/token_usage_fields.go
- conversations/service_account_test.go
- mcp/user_clients.go
- webapp/src/components/agents/tabs/mcps_tab.test.tsx
- config/mcp_config_test.go
- llmcontext/llm_context_license_test.go
- config/mcp_config.go
- webapp/src/components/agents/tabs/mcps_tab.tsx
- api/api_test.go
- api/api_llm_bridge_service_account_test.go
- webapp/src/components/system_console/mcp_servers.tsx
- llmcontext/llm_context_test.go
- api/api_llm_bridge_test.go
- mcp/client.go
- mcp/client_manager.go
- mcp/service_account_test.go
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
|
Test server destroyed |
…account-auth-a475 Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
… tests Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/components/agents/tabs/mcps_tab.tsx (1)
313-375: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftBypass per-user OAuth controls in service-account mode.
When
useServiceAccountAuthis enabled, this still renders Connect, connection badges, and instructions telling a user to authenticate. Hide these controls and render the service-account catalogue/state instead; otherwise the UI contradicts the selected authentication mode.Based on PR objectives: per-user OAuth flows and related UI are bypassed.
Also applies to: 394-402
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/agents/tabs/mcps_tab.tsx` around lines 313 - 375, Update the MCP server rendering around ServerHeaderButton and ConnectButton to branch on useServiceAccountAuth: when service-account authentication is enabled, suppress the per-user Connect control, authentication/connection badges, and related user-authentication instructions, and render the existing service-account catalogue/state instead. Preserve the current OAuth UI behavior when useServiceAccountAuth is disabled, including the applicable tools metadata and server expansion controls.
🧹 Nitpick comments (1)
llm/configuration_test.go (1)
668-672: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd read-side and true-value coverage for the new flag.
This assertion covers only marshalling
false. Add table-driven unmarshal/round-trip cases for bothtrueandfalse; otherwise a regression while loading persisted or API configuration could silently disable service-account authentication while this test still passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/configuration_test.go` around lines 668 - 672, Extend the BotConfig coverage in the existing configuration tests with table-driven cases that unmarshal and round-trip both true and false values for UseServiceAccountAuth, verifying the loaded value is preserved and serialized correctly. Retain the current false-field presence assertions and use the existing MCPDynamicToolLoading/UseServiceAccountAuth symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/multiplayer_tool_calling.md`:
- Around line 81-82: Update the approval rationale in the multiplayer
tool-calling documentation to qualify that tools run with the initiator’s
credentials only for per-user authentication. Keep the existing initiator-only
approval behavior and service-account explanation unchanged.
---
Outside diff comments:
In `@webapp/src/components/agents/tabs/mcps_tab.tsx`:
- Around line 313-375: Update the MCP server rendering around ServerHeaderButton
and ConnectButton to branch on useServiceAccountAuth: when service-account
authentication is enabled, suppress the per-user Connect control,
authentication/connection badges, and related user-authentication instructions,
and render the existing service-account catalogue/state instead. Preserve the
current OAuth UI behavior when useServiceAccountAuth is disabled, including the
applicable tools metadata and server expansion controls.
---
Nitpick comments:
In `@llm/configuration_test.go`:
- Around line 668-672: Extend the BotConfig coverage in the existing
configuration tests with table-driven cases that unmarshal and round-trip both
true and false values for UseServiceAccountAuth, verifying the loaded value is
preserved and serialized correctly. Retain the current false-field presence
assertions and use the existing MCPDynamicToolLoading/UseServiceAccountAuth
symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 30bcc64f-1b71-4c69-95a7-39965355eecf
📒 Files selected for processing (26)
api/api_agents_test.goapi/api_config_test.goapi/api_llm_bridge.goapi/api_llm_bridge_service_account_test.goapi/api_llm_bridge_test.goapi/api_test.goconfig/mcp_config_test.goconversations/service_account_test.goconversations/single_build_test.godocs/admin_guide.mddocs/features/multiplayer_tool_calling.mdllm/configuration_test.gollm/token_tracking_test.gollmcontext/llm_context.gollmcontext/llm_context_test.gomcp/client_embedded_oauth_test.gomcp/client_manager.gomcp/client_test.gomcp/service_account_test.gomcp/user_clients.gopublic/bridgeclient/README.mdstore/agents_test.gowebapp/src/components/agents/agent_config_view.test.tsxwebapp/src/components/agents/tabs/mcps_tab.test.tsxwebapp/src/components/agents/tabs/mcps_tab.tsxwebapp/src/components/system_console/mcp_servers.test.tsx
💤 Files with no reviewable changes (2)
- api/api_llm_bridge.go
- api/api_llm_bridge_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- webapp/src/components/agents/tabs/mcps_tab.test.tsx
- conversations/single_build_test.go
- api/api_test.go
- docs/admin_guide.md
- webapp/src/components/agents/agent_config_view.test.tsx
- mcp/client_manager.go
- llmcontext/llm_context.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52bf800312
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rve header row order, doc fixes Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/mcp_config.go`:
- Around line 120-129: Update the ServiceAccountHeaders normalization loop to
detect duplicate trimmed names before inserting into headers, including
collisions caused by whitespace. Return or propagate a validation error for any
duplicate instead of overwriting an existing value; retain skipping of blank
names and values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5ce082db-2018-4b2f-be3c-8d6381dc4e65
📒 Files selected for processing (7)
config/mcp_config.goconfig/mcp_config_test.godocs/features/multiplayer_tool_calling.mdmcp/client_manager.gostore/migrations/reviews/000010_user_agent_service_account_auth.mdwebapp/src/components/system_console/mcp_servers.test.tsxwebapp/src/components/system_console/mcp_servers.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- store/migrations/reviews/000010_user_agent_service_account_auth.md
- webapp/src/components/system_console/mcp_servers.test.tsx
- config/mcp_config_test.go
- webapp/src/components/system_console/mcp_servers.tsx
- mcp/client_manager.go
…t review findings Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/mcp_config.go (1)
114-142: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCanonicalize header names before collision checks in
config/mcp_config.go:114-142strings.TrimSpace(name)still treatsAuthorizationandauthorizationas distinct, but the downstream request code applies headers withreq.Header.Set, which folds them to the same wire header. That leaves case-only duplicates to clobber each other by map iteration order; usetextproto.CanonicalMIMEHeaderKey(or equivalent case-folding) when counting and storing these headers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/mcp_config.go` around lines 114 - 142, Update EffectiveServiceAccountHeaders to canonicalize each trimmed header name with textproto.CanonicalMIMEHeaderKey before collision counting and storage. Use the same canonical form in both loops so case-only duplicates such as Authorization and authorization are treated as collisions and omitted, while preserving existing trimming and empty-value filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@config/mcp_config.go`:
- Around line 114-142: Update EffectiveServiceAccountHeaders to canonicalize
each trimmed header name with textproto.CanonicalMIMEHeaderKey before collision
counting and storage. Use the same canonical form in both loops so case-only
duplicates such as Authorization and authorization are treated as collisions and
omitted, while preserving existing trimming and empty-value filtering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 71a4c31c-86f6-4ee9-9871-ca8370916ee9
📒 Files selected for processing (10)
api/api.goapi/api_agents.goapi/api_agents_test.goapi/api_test.goconfig/mcp_config.goconfig/mcp_config_test.godocs/admin_guide.mdwebapp/src/components/agents/tabs/mcps_tab.test.tsxwebapp/src/components/agents/tabs/mcps_tab.tsxwebapp/src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- webapp/src/i18n/en.json
- docs/admin_guide.md
…l closed Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
|
@cursor fix this PR's conflicts with master. |
|
The merge conflicts with master are resolved and pushed. I merged
Verification all passed:   |
…account-auth-a475 # Conflicts: # webapp/src/components/agents/agent_config_view.test.tsx # webapp/src/i18n/en.json Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
|
Test server destroyed |
edgarbellot
left a comment
edgarbellot
left a comment
There was a problem hiding this comment.
Great work! Let me know if you have any questions about my comments
|
|
||
| // Get tools from all connected servers | ||
| mcpTools, mcpErrors = b.mcpToolProvider.GetToolsForUser(ctx, userID) | ||
| if useServiceAccount { |
There was a problem hiding this comment.
In SA mode this branch builds the embedded tool catalog for the bot user, not the requesting user, and ensureEmbeddedSessionID(key.userID) mints the Mattermost session for that bot. Every embedded tool call then runs with the bot's own permissions. Nothing downstream re-checks the tool's output against what the requesting user is actually allowed to see - the code only asks "can the bot do this", never "can this user see this".
Confirmed it live: a user who was not a member of a private channel asked the SA agent to summarize it. The agent called read_channel as the bot (which is a member) and returned the full message history to someone who should never have seen it.
Suggested fix: the missing piece is a permission check between the tool call and its result - something that, for channel-scoped reads, intersects the target channel against the requesting user's own membership before returning content, not just the bot's. Right now that check doesn't exist anywhere in the SA path.
Same root cause also affects write tools like add_channel_member (unconfigured tools default to enabled/ask per GetToolPolicy, config/mcp_config.go) - it only didn't fire in my test because the bot lacked manage_private_channel_members, which is Mattermost's own guard, not this code's. If a bot ever holds a higher channel/team role, the same missing check lets that write through too.
There was a problem hiding this comment.
This is something I am aware of and have done intentionally. If we were to implement your suggested fix where permissions are intersected, you'd end up in a state where the service account has less access than if the invoker's permissions were just being used themselves.
Channel membership being required is the thing that is gating access. It's actually something that's already in the product: #611 - bot invocations of an Agent use the bot's access. I have an incoming commit to fix your finding here: #937 (comment) - after which only trusted system administrators will be able to set or manage these agents (non system administrator maintainers will be read only)
Long term as possible scoped tokens, or even ABAC for bot accounts (because they're just users at the end of the day) come into play for channel access, this'll get more secure. But for now it's a deliberate, documented thing: https://github.com/mattermost/mattermost-plugin-agents/pull/937/changes#diff-358388706355238989c5323624130cf04d7e4b827ea00f92e298c1ea23d6d19dR57-R67
| return | ||
| } | ||
|
|
||
| // Only the false→true transition escalates; an already-enabled flag was |
There was a problem hiding this comment.
Once a system admin turns on useServiceAccountAuth for an agent, anyone who can manage that agent - even without manage_system - can freely widen it afterwards: open it to every user on the instance, auto-enable every tool, or point it at a different LLM provider.
Why: the admin-only check only fires on the false→true transition of the flag. Every field after that - userAccessLevel, autoEnableNewMCPTools, adminUserIDs, serviceID - gets overwritten under the same check as any normal agent edit: canManageAgent, which passes for the agent's own creator or anyone in adminUserIDs. Neither of those requires being a system admin. So the code checks "is this person allowed to touch this agent at all", never "is this specific change one that needs admin rights".
Confirmed in my test instance: a system admin enabled SA on an agent created by, and scoped to, a regular member. That same member - still with no manage_system permission - then opened the agent to everyone and auto-enabled every tool, in one request that returned 200.
Is this intentional? If not, once useServiceAccountAuth is true, I would suggest that changes that widen its reach (userAccessLevel, userIDs, channelIDs, teamIDs, adminUserIDs, autoEnableNewMCPTools, enabledMCPTools) should require the same canEnableServiceAccountAuth check as enabling the flag itself, not just canManageAgent.
| headers := make(map[string]string) | ||
| headers[MMUserIDHeader] = c.userID | ||
| maps.Copy(headers, serverConfig.Headers) | ||
| headers := remoteConnectionHeaders(c.userID, serverConfig, c.serviceAccount) |
There was a problem hiding this comment.
The service-account header (the shared, admin-provisioned credential) gets resent to a different host if the configured MCP server responds with an HTTP redirect.
Why: remoteConnectionHeaders builds the SA header, and createSession hands it to httpClientForMCP, which wires it into a custom http.RoundTripper that re-adds the header on every outgoing request - including the one Go automatically sends when following a redirect. Go's http.Client normally strips Authorization on a cross-host redirect, but that only removes what's already set on the request; it can't stop a custom transport from putting the same header back one line later.
Reproduced in my test instance: pointed this client at a test server configured with a fake SA header, made it respond with a redirect to a different host, and watched that header arrive intact at the redirect target.
Suggested fix: the http.Client built in createSession/httpClientForMCP needs a CheckRedirect that either blocks redirects to a different origin than serverConfig.BaseURL, or drops the injected headers before the redirected request goes out - without that, re-adding the header unconditionally on every RoundTrip defeats any redirect check added elsewhere.
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
…account-auth-a475 # Conflicts: # docs/user_guide.md Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
|
/update-branch |
|
Error trying to update the PR. |
crspeller
left a comment
crspeller
left a comment
There was a problem hiding this comment.
Looks mostly good, but I think we need to allow non-system admins to make service accounts.
| // Any save that keeps service account auth enabled is admin-only, since every | ||
| // field shapes what the admin-granted credentials can do; anyone who can | ||
| // manage the agent may still turn it off. | ||
| if req.UseServiceAccountAuth && !canSaveServiceAccountAuth(a.pluginAPI, userID) { |
There was a problem hiding this comment.
This seems clunky and involves system admins. Can we have these service accounts created on the user side and grant access to them? (This breaks Agents being managed by users, on Hub that means we are back to begging to update the PDE intake agent)
6 participants
Summary
Adds service account authentication for MCP servers, so automation-oriented agents can call external MCP servers (e.g. Jira) with admin-provisioned shared credentials instead of per-user OAuth.
What's new
EffectiveServiceAccountHeaders()).mcp_oauth_neededstate, per-user server preferences, RHS Tools popover) is bypassed/hidden./ai_botsreports the effective mode, so the UI degrades to per-user UX on unlicensed servers.acting_user_idandtool_auth_mode.user_id(which still drives usage ACLs and attribution); discovery uses the same catalog as execution.user_idsemantics.docs/features/multiplayer_tool_calling.md(the "service accounts are not a concept" contract is replaced with per-user vs service-account modes),docs/admin_guide.md,public/bridgeclient/README.md, user guide, managing-agents.Implementation notes
(bot user ID, service-account mode)inmcp.ClientManager, with an isolated shared tools cache namespace (sa:<server>);X-Mattermost-UserIDis the agent bot's ID. The "SA never surfaces OAuth prompts" invariant is enforced structurally inClient.oauthNeededError.llmcontext.Builder.getToolsStoreForUser) viaUsesServiceAccountCatalog, so conversations, the bridge, discovery, and channel analysis all inherit it with no per-site branching.000010addsUseServiceAccountAuthtoAgents_UserAgents.tool_hooksare deliberately not accounted for in SA mode (their only consumer, the Channel Automation plugin, is being retired).Security review follow-up (2026-08-06)
Addresses two findings from @edgarbellot's review:
useServiceAccountAuthrequired system admin, so after an admin enabled it, any agent manager could widen the agent (user access, tool auto-grant, service, instructions) under the admin-granted credentials. Now any save whose resulting state keeps the flag on requires system admin. Non-admin managers keep full read access to the config and retain two escape hatches: turning the setting off (optionally editing anything else in that same request, since the result is a normal agent) or deleting the agent. Re-enabling requires admin again. The webapp shows non-admin managers a read-only editor (banner + disabled Save) while the flag stays on, with the MCPs tab kept reachable as the off-switch even when tools are disabled.Authorizationstripping when a server issued a redirect. The fix covers all three credential types (SA headers, admin-configured headers, per-user OAuth tokens) with two layers: aCheckRedirectpolicy that rejects redirects leaving the configured server origin, and origin-gated injection insideheaderTransportandauthenticationTransport(fail-closed on unparsable base URLs; in-process plugin transports are unaffected). OAuth discovery/token flows to a different authorization-server origin are untouched since they don't route through these transports.Demo — a non-admin agent maintainer gets a read-only editor for an SA agent, with turning the setting off as the only way to re-enable saving:
non_admin_maintainer_read_only_sa_agent_editor.mp4
QA (manual, local server + real Anthropic LLM + PAT-gated external MCP server)
console_mcp_server_service_account_section.webpandagent_mcps_tab_sa_checkbox_warning.webpin the run's artifact history.)get_metool returns the service account's username (not the signed-in user), after human approval, with no OAuth prompt; the RHS Tools popover is hidden. (sa_agent_tool_call_runs_as_service_account.mp4in the run's artifact history.)normal_agent_keeps_per_user_tools_ux.mp4in the run's artifact history.)go test ./api/... ./mcp/..., webapp type-check + jest, eslint, i18n drift check, and the recorded manual demo of the read-only maintainer editor.Release Note
To show artifacts inline, enable in settings.
Summary by CodeRabbit