Add service account authentication for external MCP servers - #937
Add service account authentication for external MCP servers#937nickmisasi wants to merge 9 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 7 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
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. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
7. 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. |
📝 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 configuration and persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AgentConfig
participant ContextBuilder
participant ClientManager
participant MCPServer
participant LLM
AgentConfig->>ContextBuilder: enable service-account authentication
ContextBuilder->>ClientManager: request service-account catalog
ClientManager->>MCPServer: connect with static headers and bot identity
MCPServer-->>ClientManager: eligible tools
ClientManager-->>ContextBuilder: service-account tool catalog
ContextBuilder->>LLM: provide catalog and tool auth mode
LLM-->>ContextBuilder: tool request
🚥 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
| func (s *MCPServerConfig) EffectiveServiceAccountHeaders() map[string]string { | ||
| if s == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var headers map[string]string | ||
| for name, value := range s.ServiceAccountHeaders { | ||
| if strings.TrimSpace(name) == "" || strings.TrimSpace(value) == "" { | ||
| continue | ||
| } | ||
| if headers == nil { | ||
| headers = make(map[string]string, len(s.ServiceAccountHeaders)) | ||
| } | ||
| headers[name] = value | ||
| } | ||
| return headers | ||
| } | ||
|
|
||
| // HasServiceAccountAuth reports whether this server has Service Account | ||
| // authentication configured, i.e. at least one usable header. An SA-flagged | ||
| // agent therefore never connects to a server that would effectively send no | ||
| // service credential (fail closed). Enabled is intentionally ignored: | ||
| // enablement is filtered where catalogs are built, this predicate answers only | ||
| // "is SA auth configured". | ||
| func (s *MCPServerConfig) HasServiceAccountAuth() bool { | ||
| return len(s.EffectiveServiceAccountHeaders()) > 0 | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Padded header names still slip through and break the whole request.
The blank check trims, but the stored key/value keep surrounding whitespace. An admin entry like " Authorization" (or a value with a trailing newline pasted from a token file) is non-blank, so it passes the filter and is then rejected by Go's HTTP transport with invalid header field name, failing every SA connection to that server — the exact failure mode the doc comment says this function prevents. Trimming what you keep closes the gap and matches the intent.
🛠️ Proposed fix: store the trimmed name/value
var headers map[string]string
for name, value := range s.ServiceAccountHeaders {
- if strings.TrimSpace(name) == "" || strings.TrimSpace(value) == "" {
+ trimmedName := strings.TrimSpace(name)
+ trimmedValue := strings.TrimSpace(value)
+ if trimmedName == "" || trimmedValue == "" {
continue
}
if headers == nil {
headers = make(map[string]string, len(s.ServiceAccountHeaders))
}
- headers[name] = value
+ headers[trimmedName] = trimmedValue
}
return headersNote the companion table in config/mcp_config_test.go (lines 122-206) would need a row for a padded name/value if you take this.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *MCPServerConfig) EffectiveServiceAccountHeaders() map[string]string { | |
| if s == nil { | |
| return nil | |
| } | |
| var headers map[string]string | |
| for name, value := range s.ServiceAccountHeaders { | |
| if strings.TrimSpace(name) == "" || strings.TrimSpace(value) == "" { | |
| continue | |
| } | |
| if headers == nil { | |
| headers = make(map[string]string, len(s.ServiceAccountHeaders)) | |
| } | |
| headers[name] = value | |
| } | |
| return headers | |
| } | |
| // HasServiceAccountAuth reports whether this server has Service Account | |
| // authentication configured, i.e. at least one usable header. An SA-flagged | |
| // agent therefore never connects to a server that would effectively send no | |
| // service credential (fail closed). Enabled is intentionally ignored: | |
| // enablement is filtered where catalogs are built, this predicate answers only | |
| // "is SA auth configured". | |
| func (s *MCPServerConfig) HasServiceAccountAuth() bool { | |
| return len(s.EffectiveServiceAccountHeaders()) > 0 | |
| } | |
| func (s *MCPServerConfig) EffectiveServiceAccountHeaders() map[string]string { | |
| if s == nil { | |
| return nil | |
| } | |
| var headers map[string]string | |
| for name, value := range s.ServiceAccountHeaders { | |
| trimmedName := strings.TrimSpace(name) | |
| trimmedValue := strings.TrimSpace(value) | |
| if trimmedName == "" || trimmedValue == "" { | |
| continue | |
| } | |
| if headers == nil { | |
| headers = make(map[string]string, len(s.ServiceAccountHeaders)) | |
| } | |
| headers[trimmedName] = trimmedValue | |
| } | |
| return headers | |
| } | |
| // HasServiceAccountAuth reports whether this server has Service Account | |
| // authentication configured, i.e. at least one usable header. An SA-flagged | |
| // agent therefore never connects to a server that would effectively send no | |
| // service credential (fail closed). Enabled is intentionally ignored: | |
| // enablement is filtered where catalogs are built, this predicate answers only | |
| // "is SA auth configured". | |
| func (s *MCPServerConfig) HasServiceAccountAuth() bool { | |
| return len(s.EffectiveServiceAccountHeaders()) > 0 | |
| } |
🤖 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 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.
| func (m *ClientManager) closeIdleClients(now time.Time) { | ||
| m.clientsMu.Lock() | ||
| defer m.clientsMu.Unlock() | ||
| 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, key) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
closeIdleClients leaves orphaned activity entries.
The bag is deleted from m.clients, but its timestamp stays in m.activity forever, so the map grows unbounded with distinct (user, auth mode) pairs that never reconnect. Now that each user can occupy two keys, this doubles.
🧹 Proposed fix
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, key)
+ delete(m.activity, key)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *ClientManager) closeIdleClients(now time.Time) { | |
| m.clientsMu.Lock() | |
| defer m.clientsMu.Unlock() | |
| 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, key) | |
| } | |
| } | |
| } | |
| func (m *ClientManager) closeIdleClients(now time.Time) { | |
| m.clientsMu.Lock() | |
| defer m.clientsMu.Unlock() | |
| 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, key) | |
| delete(m.activity, key) | |
| } | |
| } | |
| } |
🤖 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 `@mcp/client_manager.go` around lines 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.
| ## 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 — returns instantly. | ||
| - Impact to concurrent operations: Negligible. | ||
|
|
||
| ## Zero Downtime | ||
| - Possible: Yes. | ||
| - Reason: Metadata-only ADD COLUMN on an admin-managed table. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' store/migrations/reviews/000010_user_agent_service_account_auth.mdRepository: mattermost/mattermost-plugin-agents
Length of output: 2539
🌐 Web query:
PostgreSQL ALTER TABLE ADD COLUMN ACCESS EXCLUSIVE lock waits behind active transactions metadata-only constant default lock semantics
💡 Result:
In PostgreSQL, an ALTER TABLE ADD COLUMN statement always acquires an ACCESS EXCLUSIVE lock on the target table [1][2]. This lock is the most restrictive level in PostgreSQL and conflicts with all other lock modes, meaning it will block all concurrent access to the table, including simple SELECT statements, until the transaction completes [3][4]. While the lock is always required, its duration and the impact on the system have been significantly reduced by changes introduced in PostgreSQL 11 [5][6]. Metadata-Only and Fast Default Mechanism: When a column is added with a non-volatile DEFAULT value, PostgreSQL now uses a "fast default" mechanism [5][6]. Instead of rewriting the entire table to populate the new column for existing rows, the database evaluates the default value once and stores it in the table's metadata (specifically within the system catalog pg_attribute, using the attmissingval and atthasmissing fields) [5][6]. When existing rows are subsequently read, the database synthesizes the default value on the fly using this metadata [5][6]. Because the table data itself is not modified during the ALTER TABLE command, the operation completes extremely quickly, effectively minimizing the duration for which the ACCESS EXCLUSIVE lock must be held [5][6][4]. Key Considerations: 1. Volatile Defaults: If the default value is volatile (e.g., clock_timestamp or random), the fast default mechanism cannot be used [5]. In such cases, the database must update every existing row, which may result in a much longer hold time for the ACCESS EXCLUSIVE lock [7][5]. 2. Lock Queuing: Even with the fast path, because the ACCESS EXCLUSIVE lock is requested at the start of the command, it can still lead to "lock queuing" in high-traffic environments [4][8]. If there are long-running transactions active on the table when the ALTER TABLE statement is issued, the ALTER TABLE command will wait behind them, and subsequent incoming queries will, in turn, be queued behind the pending ALTER TABLE request [4][8]. 3. No Default/Null: If no DEFAULT is specified (defaulting to NULL) or a simple constant is used, the operation is metadata-only and highly efficient [1][6]. In summary, while the ACCESS EXCLUSIVE lock is mandatory, modern PostgreSQL versions ensure the operation is near-instant for non-volatile constants, preventing the heavy performance penalty associated with full table rewrites [5][6][4].
Citations:
- 1: https://www.postgresql.org/docs/18/sql-altertable.html
- 2: https://www.postgresql.org/docs/19/sql-altertable.html
- 3: https://www.postgresql.org/docs/18/explicit-locking.html
- 4: https://thedba.net/does-add-column-lock-a-table/
- 5: https://brandur.org/postgres-default
- 6: https://hgryoo.dev/knowledge-docs-site/code-analysis/postgres/postgres-alter-table/
- 7: https://www.postgresql.org/docs/18/ddl-alter.html
- 8: https://stackoverflow.com/questions/43972656/why-does-postgres-start-enforcing-access-exclusive-lock-for-alter-table-add-colu
Clarify lock wait behaviour. ADD COLUMN is metadata-only with a constant default, but it still takes ACCESS EXCLUSIVE and can wait behind active transactions; avoid calling it “instant” or the impact “negligible” without that caveat.
🤖 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 `@store/migrations/reviews/000010_user_agent_service_account_auth.md` around
lines 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.
| const updateHeader = (oldKey: string, newKey: string, value: string) => { | ||
| const updated = {...headers}; | ||
| if (oldKey !== newKey) { | ||
| delete updated[oldKey]; | ||
| } | ||
| updated[newKey] = value; | ||
| onChange(updated); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Renaming a header reorders the rows while key={index} stays fixed.
updateHeader does delete updated[oldKey] then updated[newKey] = value, which moves the entry to the end of the object's insertion order. Rows are rendered from Object.entries(headers) keyed by position, so with two or more headers, typing in the name field of any non-last row swaps that row's rendered content with the one below it on the first keystroke while focus stays put — the admin ends up editing the wrong header. Now that this editor also backs the service-account credential map, that's a credential typed into the wrong slot.
Rebuilding the map in the original order keeps the row stable.
🐛 Proposed fix: preserve entry order on rename
const updateHeader = (oldKey: string, newKey: string, value: string) => {
- const updated = {...headers};
- if (oldKey !== newKey) {
- delete updated[oldKey];
- }
- updated[newKey] = value;
- onChange(updated);
+ const updated: {[key: string]: string} = {};
+ for (const [k, v] of Object.entries(headers)) {
+ if (k === oldKey) {
+ updated[newKey] = value;
+ } else {
+ updated[k] = v;
+ }
+ }
+ onChange(updated);
};Also applies to: 119-120
🤖 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 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.
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
| 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. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the approval rationale to per-user authentication.
The new clarification correctly says initiator-only approval applies in both modes, but the preceding rationale says tools run with the initiator’s credentials. That is inaccurate for service-account agents and should be qualified as applying only to per-user authentication.
Suggested wording
- 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.
+ 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.🧰 Tools
🪛 LanguageTool
[uncategorized] ~81-~81: Possible missing comma found.
Context: ...ntial modes (§3). For a service account agent the initiator is still the person askin...
(AI_HYDRA_LEO_MISSING_COMMA)
🤖 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 `@docs/features/multiplayer_tool_calling.md` around lines 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.
3 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.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).QA (manual, local server + real Anthropic LLM + PAT-gated external MCP server)
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.mp4
normal_agent_keeps_per_user_tools_ux.mp4
Release Note
To show artifacts inline, enable in settings.
Summary by CodeRabbit