ABAC resource policies for agents, LLM services, and MCP servers#886
Open
nickmisasi wants to merge 77 commits into
Open
ABAC resource policies for agents, LLM services, and MCP servers#886nickmisasi wants to merge 77 commits into
nickmisasi wants to merge 77 commits into
Conversation
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…d contract Co-authored-by: nick.misasi <[email protected]>
…ig-row primitive Co-authored-by: nick.misasi <[email protected]>
…e IDs Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…d decision tables Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…sting entry points Co-authored-by: nick.misasi <[email protected]>
… availability Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…ervices, and MCP servers Co-authored-by: nick.misasi <[email protected]>
…d webapp editor hosts Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…Config Co-authored-by: nick.misasi <[email protected]>
… re-minting IDs Co-authored-by: nick.misasi <[email protected]>
…tion-time MCP resolver Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…s/fetch for non-admins Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 6 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
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:
📝 WalkthroughWalkthroughThis PR adds ABAC support for agents, services, and MCP servers, including policy APIs, runtime enforcement, stable resource IDs, migration safeguards, webapp policy editors, and editor-contract drift validation. ChangesABAC platform integration
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/api.go (1)
175-251: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake missing ABAC dependencies fail closed. The checker is optional at construction and the MCP listing also skips authorization when either the checker or stable resource ID is missing.
api/api.go#L175-L251: reject a nilaccessCheckerduring API construction.api/api_mcp.go#L94-L101: omit the server unless a stable ID exists andCanUseMCPServersucceeds.🤖 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.go` around lines 175 - 251, Make ABAC fail closed at both affected sites: in api/api.go within New, reject a nil accessChecker during API construction; in api/api_mcp.go within the MCP listing logic, include a server only when it has a stable resource ID and CanUseMCPServer succeeds, omitting it when either requirement is missing.api/api_admin.go (1)
521-539: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate a committed configuration locally before reporting publication failure. Persistence succeeds before cluster notification, so returning early leaves the current node stale despite the committed database state.
api/api_admin.go#L521-L539: applyUpdatePluginServerAdminFieldsandconfigUpdater.Updateimmediately afterUpdateConfig, before publishing.api/api_admin_test.go#L876-L887: expect the local updater and runtime patch after a successful save even when publication fails.api/api_admin_test.go#L951-L958: distinguish pre-commit failures from post-commit notification failures in the mutation assertions.🤖 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_admin.go` around lines 521 - 539, Move the local runtime patch via UpdatePluginServerAdminFields and configUpdater.Update immediately after the successful UpdateConfig persistence, before PublishConfigUpdate; retain the publication error response afterward. In api/api_admin_test.go lines 876-887, expect both local updates when publication fails, and in lines 951-958, distinguish pre-commit failures from post-commit notification failures in mutation assertions.
🧹 Nitpick comments (5)
e2e/tests/agents/access-control.spec.ts (1)
4-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse snake_case for the planned spec filenames.
Rename the listed files to
abac_agent_access.spec.ts,abac_service_mcp.spec.ts, andabac_authoring_authz.spec.ts.As per coding guidelines,
**/*.{go,ts,tsx}files must usesnake_case.🤖 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 `@e2e/tests/agents/access-control.spec.ts` around lines 4 - 10, Rename the planned spec files to snake_case: abac_agent_access.spec.ts, abac_service_mcp.spec.ts, and abac_authoring_authz.spec.ts, updating any references to their current kebab-case names while preserving their test contents.Source: Coding guidelines
api/api_search.go (1)
31-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated usage-gate + request-validation logic.
handleRunSearchandhandleSearchQuerynow both start with an identicalCheckUsageRestrictionsForUsergate, on top of the pre-existing duplicated query/MaxResultsvalidation block. Consider factoring the shared preamble (gate + decode + validate) into one helper returning(SearchRequest, error)(or a bool to abort), used by both handlers.♻️ Sketch of a shared helper
func (a *API) prepareSearchRequest(c *gin.Context, userID string, bot *bots.Bot) (*SearchRequest, bool) { if err := a.bots.CheckUsageRestrictionsForUser(c.Request.Context(), bot, userID); err != nil { c.AbortWithError(http.StatusForbidden, err) return nil, false } if !a.searchService.Enabled() { c.AbortWithError(http.StatusBadRequest, fmt.Errorf("search functionality is not configured")) return nil, false } var req SearchRequest if err := json.NewDecoder(c.Request.Body).Decode(&req); err != nil { c.AbortWithError(http.StatusBadRequest, fmt.Errorf("invalid request: %w", err)) return nil, false } req.Query = strings.TrimSpace(req.Query) if req.Query == "" { c.AbortWithError(http.StatusBadRequest, fmt.Errorf("query cannot be empty")) return nil, false } if len(req.Query) > maxSearchQueryLength { c.AbortWithError(http.StatusBadRequest, fmt.Errorf("query exceeds maximum length of %d characters", maxSearchQueryLength)) return nil, false } if req.MaxResults <= 0 { req.MaxResults = defaultMaxResults } else if req.MaxResults > maxMaxResults { req.MaxResults = maxMaxResults } return &req, true }🤖 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_search.go` around lines 31 - 125, Extract the duplicated usage gate, search-enabled check, request decoding, query validation, and MaxResults normalization from handleRunSearch and handleSearchQuery into a shared prepareSearchRequest helper. Have the helper abort the Gin context and report failure consistently, return the prepared SearchRequest on success, and update both handlers to use it while preserving their distinct RunSearch and SearchQuery calls.bots/bots.go (1)
78-92: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winEnforce the documented non-nil
accessCheckerinvariant.
MMBotsdereferencesm.accessCheckerunconditionally inbots/permissions.go:
m.accessChecker.CanUseAgent(...)m.accessChecker.CanUseService(...)If
Newis ever called withnil, the panic happens later in a permission check instead of at construction time. Add an explicit nil check inNewto fail fast.🤖 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 `@bots/bots.go` around lines 78 - 92, Update New to explicitly reject a nil accessChecker during construction, failing immediately before creating and returning MMBots. Preserve the existing initialization flow for non-nil accessChecker values and use the project’s established failure mechanism.accesscontrol/reconciler.go (1)
41-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPotential key-collision in dedup/cooldown maps.
Concatenating
resourceType + "/" + resourceIDas a map key can collide across different(resourceType, resourceID)pairs if either component contains"/". Using thereconcileRequeststruct directly as the map key sidesteps this entirely.♻️ Proposed fix: use struct keys instead of string concatenation
- mu sync.Mutex - pending map[string]struct{} - lastRun map[string]time.Time + mu sync.Mutex + pending map[reconcileRequest]struct{} + lastRun map[reconcileRequest]time.Time @@ func newReconciler(cooldown time.Duration) *reconciler { return &reconciler{ ch: make(chan reconcileRequest, reconcileQueueSize), stop: make(chan struct{}), done: make(chan struct{}), cooldown: cooldown, - pending: map[string]struct{}{}, - lastRun: map[string]time.Time{}, + pending: map[reconcileRequest]struct{}{}, + lastRun: map[reconcileRequest]time.Time{}, } } @@ func (r *reconciler) enqueue(resourceType, resourceID string) { - key := resourceType + "/" + resourceID + key := reconcileRequest{resourceType: resourceType, resourceID: resourceID}Also applies to: 82-82, 130-130
🤖 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 `@accesscontrol/reconciler.go` around lines 41 - 44, Update the deduplication and cooldown maps to use reconcileRequest directly as their key instead of concatenating resourceType and resourceID with “/”. Replace all corresponding key construction and lookups, including the additional occurrences, while preserving the existing request values and map behavior.webapp/src/components/system_console/mcp_servers.tsx (1)
358-364: 📐 Maintainability & Code Quality | 🔵 TrivialRoute the fallback display name through
intl.formatMessagefor i18n coverage.
Server ${serverIndex + 1}is a hardcoded English fallback feedingresourceDisplayName.intlis already in scope here; the siblingServicecomponent uses an i18n-aware fallback (serviceTypeToDisplayName) for the equivalent case.♻️ Proposed fix
{config.id && ( <ConsolePolicySection resourceType='mcp' resourceId={config.id} - resourceDisplayName={config.name || `Server ${serverIndex + 1}`} + resourceDisplayName={config.name || intl.formatMessage({defaultMessage: 'Server {number}'}, {number: serverIndex + 1})} /> )}As per coding guidelines,
webapp/**/*.{ts,tsx}: 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 `@webapp/src/components/system_console/mcp_servers.tsx` around lines 358 - 364, Update the resourceDisplayName fallback in the MCP server ConsolePolicySection usage to call intl.formatMessage with an extracted message for the server label, while preserving config.name when available and the existing serverIndex-based numbering.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.
Inline comments:
In `@accesscontrol/checker.go`:
- Around line 81-89: Update the Checker constructor New to accept
context.Context as its first parameter, then derive lifecycleCtx with
context.WithCancel(ctx) instead of context.Background(). Thread this
caller-provided context through the related LLM call path while preserving
existing cancellation behavior.
- Around line 24-26: Route access-control errors through typed internal details
and localized API responses instead of exposing literal English. In
accesscontrol/checker.go lines 24-26 keep ErrABACUnavailable internal; at lines
388-390 and 420-422 use typed errors/details for service and MCP denials. In
api/api_agents.go lines 381-386, 521-526, and 728-733 map validation and
model-fetch errors to extracted i18n messages before abortAgentRequest, rather
than passing err.Error() directly.
In `@accesscontrol/index_rebuild.go`:
- Around line 169-195: Prevent canceled heals from performing reads after
waiting for mutationMu: in Checker.healDivergenceUnderLock, check ctx.Err()
immediately after acquiring the mutex and return false before calling index.Has
or policyMarkerTruth, preserving the existing cancellation logging behavior. In
accesscontrol/index_heal_test.go lines 507-535, add a scenario where the heal
waits on mutationMu, the context is canceled, and then the lock is released;
assert that no under-lock policy fetch occurs.
In `@api/api_access_control.go`:
- Around line 338-349: Cap request-body size for all CEL proxy routes before
JSON binding. Update bindCELExpressionRequest, used by handleCELCheck and
handleCELVisualAST, and the inline binding in handleCELTest to wrap the request
body with the same http.MaxBytesReader limit and handling pattern used by
bindPolicyBody, preserving the existing validation and bad-request responses.
In `@api/api_admin.go`:
- Around line 472-476: Update the nil persisted-config branch in the relevant
admin configuration handler to use the project’s i18n extraction mechanism for
the user-facing “no plugin configuration available” error, rather than
constructing a hardcoded string. Preserve the existing error return behavior and
localization conventions used by nearby HTTP error paths.
- Around line 465-520: Synchronize the plugin-owned fields used by the
UpdateConfig transform with the registration path so a re-registration between
the initial read and transform cannot persist stale Name, Path, or
ExposeExternal values. Update the logic around live and UpdateConfig to read or
reconcile the latest registered entry while preserving the admin-owned patch
behavior. Add a test that re-registers the plugin between the initial read and
transform and verifies the persisted fields use the newer registration.
In `@api/api_agents_test.go`:
- Around line 66-71: Update mockConfigStore.UpdateConfig to capture the result
of transform, assign the transformed config to m.cfg when no error occurs, and
return the persisted config and error. Preserve the existing getErr handling and
propagate transform errors without overwriting the stored configuration.
In `@api/api_config.go`:
- Around line 117-122: Update the conflict responses in the configuration
handler around the ErrStaleLegacyServiceIDs and preceding identity-conflict case
to use the project’s existing i18n translation mechanism instead of hardcoded
admin-facing messages. Ensure both 409 error messages are registered for
extraction and preserve their current retry guidance and wrapped error details.
- Around line 29-35: Preserve IDs for existing services during the API
configuration save flow: before normalizeAdminConfig, reconcile ID-less services
against prev using stable service identity, or reject them when no match exists,
instead of assigning model.NewId() unconditionally. Update the normalization
logic around the service loop and add coverage proving repeated saves of an
ID-less existing service retain its original ID and associated ABAC policy.
In `@api/api_test.go`:
- Around line 530-540: Update newPassthroughAccessChecker to accept testing.TB,
create the checker, and register checker.Close with t.Cleanup before returning
it. Update the createTestBots call sites in api/api_test.go:640-640 and
api/api_admin_test.go:55-55 to pass t to this cleanup-aware helper.
In `@api/api.go`:
- Line 175: Update the API construction path that initializes accessChecker to
reject a nil *accesscontrol.Checker immediately, returning the existing
constructor error type or startup failure mechanism. Ensure all construction
paths enforce this required dependency before authorization-sensitive handlers
can run, while preserving normal initialization for non-nil checkers.
In `@config/mcp_config.go`:
- Around line 249-277: Reject duplicate MCP runtime origins instead of assigning
a new policy identity: in config/mcp_config.go:249-277, detect an unclaimed
entry whose BaseURL matches an already-claimed origin and return
ErrMCPServerIDConflict; in config/mcp_config.go:115-129, enforce unique origins
rather than last-entry-wins selection; update config/mcp_config_test.go:137-146
to expect the duplicate-BaseURL conflict and config/mcp_config_test.go:68-77 to
stop treating last-entry-wins as valid ABAC behavior.
In `@conversations/test_helpers_test.go`:
- Around line 21-25: Ensure every test-created accesscontrol.Checker is closed
via test cleanup: update newPassthroughAccessChecker in
conversations/test_helpers_test.go, the corresponding helper in
bots/bots_test.go, and newTestChecker in accesscontrol/checker_test.go to accept
testing.TB and register checker.Close with t.Cleanup; also register
t.Cleanup(c.Close) at accesscontrol/checker_test.go lines 302, 390, 425, 460,
and 629.
In `@mcp/client_manager.go`:
- Around line 282-306: Update deniedExternalOrigins and its GetToolsForUser call
path to avoid repeating direct CanUseMCPServer PDP evaluations on every request,
using request-scoped caching or an available batched access-check mechanism
while preserving one decision per eligible server and the existing denial
filtering behavior.
- Around line 187-196: Synchronize access to ClientManager.config in ReInit and
the request-path helpers, including the server filtering flow shown here, so
reads cannot race with config replacement. Use the manager’s existing
synchronization mechanism if available, or publish and consistently read an
immutable config snapshot while preserving the current deniedOrigins filtering
behavior.
In `@server/legacy_bot_migration.go`:
- Around line 120-126: Update the legacy bot migration flow around the
st.UpdateConfig callback so it cannot clear a newer Bots list than the
dbCfg.Bots snapshot used for agent creation. Either hold the configuration lock
across migration and agent creation, or compare prev.Bots with dbCfg.Bots and
abort/retry when they differ; only clear Bots after confirming they still match.
In `@store/id_migrations.go`:
- Around line 172-176: Remove the early return on an empty idMap in the
migration flow so dangling UUID fallback, bot, and agent references are still
detected when no service IDs are remapped. Preserve the no-op behavior only
after all reference checks complete, and ensure the one-time marker is not
committed before those checks run.
In `@webapp/scripts/check_editor_contract.mjs`:
- Around line 142-160: Update the snapshot validation block guarded by
fs.existsSync(snapshotPath) to skip the pinned-snapshot probe when
--update-snapshot mode is active, allowing the later regeneration flow to run.
Keep the live-host probe and normal snapshot validation unchanged for non-update
invocations.
In `@webapp/scripts/editor_contract/probe.ts`:
- Around line 64-82: Update the searchUsers contract probes to compare the
callbacks’ awaited return types, not only their inner result aliases. In
webapp/scripts/editor_contract/probe.ts lines 64-82 and
webapp/scripts/editor_contract/probe_snapshot.ts lines 53-71, add bidirectional
Awaited<ReturnType<...>> checks for the host and mirror callbacks while
preserving the existing users/total comparisons.
In `@webapp/src/components/access_control/policy_editor.tsx`:
- Around line 130-133: Update the Promise.all flow in the policy editor around
getAccessControlFields so request failures are no longer converted to an empty
AccessControlPropertyField array. Propagate the failure into the component’s
error state and render the existing localized retry state, while preserving
normal rendering when fields load successfully.
---
Outside diff comments:
In `@api/api_admin.go`:
- Around line 521-539: Move the local runtime patch via
UpdatePluginServerAdminFields and configUpdater.Update immediately after the
successful UpdateConfig persistence, before PublishConfigUpdate; retain the
publication error response afterward. In api/api_admin_test.go lines 876-887,
expect both local updates when publication fails, and in lines 951-958,
distinguish pre-commit failures from post-commit notification failures in
mutation assertions.
In `@api/api.go`:
- Around line 175-251: Make ABAC fail closed at both affected sites: in
api/api.go within New, reject a nil accessChecker during API construction; in
api/api_mcp.go within the MCP listing logic, include a server only when it has a
stable resource ID and CanUseMCPServer succeeds, omitting it when either
requirement is missing.
---
Nitpick comments:
In `@accesscontrol/reconciler.go`:
- Around line 41-44: Update the deduplication and cooldown maps to use
reconcileRequest directly as their key instead of concatenating resourceType and
resourceID with “/”. Replace all corresponding key construction and lookups,
including the additional occurrences, while preserving the existing request
values and map behavior.
In `@api/api_search.go`:
- Around line 31-125: Extract the duplicated usage gate, search-enabled check,
request decoding, query validation, and MaxResults normalization from
handleRunSearch and handleSearchQuery into a shared prepareSearchRequest helper.
Have the helper abort the Gin context and report failure consistently, return
the prepared SearchRequest on success, and update both handlers to use it while
preserving their distinct RunSearch and SearchQuery calls.
In `@bots/bots.go`:
- Around line 78-92: Update New to explicitly reject a nil accessChecker during
construction, failing immediately before creating and returning MMBots. Preserve
the existing initialization flow for non-nil accessChecker values and use the
project’s established failure mechanism.
In `@e2e/tests/agents/access-control.spec.ts`:
- Around line 4-10: Rename the planned spec files to snake_case:
abac_agent_access.spec.ts, abac_service_mcp.spec.ts, and
abac_authoring_authz.spec.ts, updating any references to their current
kebab-case names while preserving their test contents.
In `@webapp/src/components/system_console/mcp_servers.tsx`:
- Around line 358-364: Update the resourceDisplayName fallback in the MCP server
ConsolePolicySection usage to call intl.formatMessage with an extracted message
for the server label, while preserving config.name when available and the
existing serverIndex-based numbering.
🪄 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: bf8c98df-8d34-42b8-9a12-218acbb96aea
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (106)
.github/workflows/ci.ymlMakefileaccesscontrol/checker.goaccesscontrol/checker_test.goaccesscontrol/client.goaccesscontrol/doc.goaccesscontrol/index_heal_test.goaccesscontrol/index_rebuild.goaccesscontrol/kv_policy_index.goaccesscontrol/kv_policy_index_test.goaccesscontrol/logger.goaccesscontrol/outcome.goaccesscontrol/pap.goaccesscontrol/pap_test.goaccesscontrol/passthrough.goaccesscontrol/pdp_client.goaccesscontrol/pdp_client_test.goaccesscontrol/policy_index.goaccesscontrol/reconciler.goapi/api.goapi/api_access_control.goapi/api_access_control_test.goapi/api_admin.goapi/api_admin_test.goapi/api_agents.goapi/api_agents_test.goapi/api_channel.goapi/api_config.goapi/api_config_test.goapi/api_llm_bridge.goapi/api_mcp.goapi/api_post.goapi/api_search.goapi/api_search_test.goapi/api_test.gobots/bots.gobots/bots_test.gobots/permissions.gobots/permissions_test.goconfig/legacy_migrations.goconfig/legacy_migrations_test.goconfig/mcp_config.goconfig/mcp_config_test.goconversations/agent_mention_reminder.goconversations/agent_mention_reminder_test.goconversations/ask_user_question_flow_test.goconversations/conversations_test.goconversations/direct_message_eval_test.goconversations/dm_conversation_test.goconversations/handle_messages.goconversations/handle_messages_test.goconversations/loaded_state_flow_test.goconversations/test_helpers_test.godocs/admin_guide.mde2e/tests/agents/access-control.spec.tsgo.modllm/configuration.gollm/configuration_test.gollmcontext/llm_context.gollmcontext/llm_context_strict_test.gomcp/client_manager.gomcp/client_manager_access_test.gomcp/client_manager_test.gomcp/testhelpers_test.gomcpserver/test_helpers_test.goplugin.jsonserver/abac_id_migrations.goserver/legacy_bot_migration.goserver/main.gostore/config.gostore/config_test.gostore/id_migrations.gostore/id_migrations_test.gotelemetry/attributes.gowebapp/package.jsonwebapp/scripts/check_editor_contract.mjswebapp/scripts/editor_contract/generate_snapshot.mjswebapp/scripts/editor_contract/probe.tswebapp/scripts/editor_contract/probe_snapshot.tswebapp/scripts/editor_contract/shims.d.tswebapp/src/client.tsxwebapp/src/client/access_control.tswebapp/src/components/access_control/console_policy_section.test.tsxwebapp/src/components/access_control/console_policy_section.tsxwebapp/src/components/access_control/policy_editor.test.tsxwebapp/src/components/access_control/policy_editor.tsxwebapp/src/components/agents/agent_config_view.test.tsxwebapp/src/components/agents/agent_config_view.tsxwebapp/src/components/agents/tabs/access_tab.test.tsxwebapp/src/components/agents/tabs/access_tab.tsxwebapp/src/components/system_console/bot.tsxwebapp/src/components/system_console/config.test.tsxwebapp/src/components/system_console/config.tsxwebapp/src/components/system_console/llm_access.tsxwebapp/src/components/system_console/mcp_servers.test.tsxwebapp/src/components/system_console/mcp_servers.tsxwebapp/src/components/system_console/service.test.tsxwebapp/src/components/system_console/service.tsxwebapp/src/components/system_console/services.test.tsxwebapp/src/components/system_console/services.tsxwebapp/src/i18n/en.jsonwebapp/src/types/access_control.tswebapp/src/types/access_control_editors.tswebapp/src/types/host_editor_contract.snapshot.d.tswebapp/src/utils/access_control.test.tsxwebapp/src/utils/access_control.ts
Comment on lines
+24
to
+26
| // ErrABACUnavailable is returned by ValidateAgentWrite when an agent is being | ||
| // saved into attribute-based mode while ABAC is not available on the server. | ||
| var ErrABACUnavailable = errors.New("attribute-based access requires Attribute-Based Access Control to be licensed and enabled on this server") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Route the new access-control responses through i18n.
These literal English errors are returned to clients through abortAgentRequest.
accesscontrol/checker.go#L24-L26: keep the sentinel internal and select a translated public message at the API boundary.accesscontrol/checker.go#L388-L390: represent service denial with a typed error rather than public English text.accesscontrol/checker.go#L420-L422: represent MCP denial with typed details for localized formatting.api/api_agents.go#L381-L386: map create-validation errors to extracted i18n messages instead of exposingerr.Error().api/api_agents.go#L521-L526: apply the same mapping for updates.api/api_agents.go#L728-L733: extract and translate the model-fetch denial message.
As per coding guidelines, all new user-facing strings must go through i18n extraction instead of being hardcoded.
📍 Affects 2 files
accesscontrol/checker.go#L24-L26(this comment)accesscontrol/checker.go#L388-L390accesscontrol/checker.go#L420-L422api/api_agents.go#L381-L386api/api_agents.go#L521-L526api/api_agents.go#L728-L733
🤖 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 `@accesscontrol/checker.go` around lines 24 - 26, Route access-control errors
through typed internal details and localized API responses instead of exposing
literal English. In accesscontrol/checker.go lines 24-26 keep ErrABACUnavailable
internal; at lines 388-390 and 420-422 use typed errors/details for service and
MCP denials. In api/api_agents.go lines 381-386, 521-526, and 728-733 map
validation and model-fetch errors to extracted i18n messages before
abortAgentRequest, rather than passing err.Error() directly.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Disagree — the repo's convention returns plain-English errors from the Go API layer (many pre-existing examples in api/); server-side i18n/en.json is hand-curated and not wired to these paths. Webapp-surfaced strings are localized.
@coderabbitai resolve
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
Comment on lines
+81
to
+89
| func New(client DecisionClient, papi plugin.API, index PolicyIndex, mcpIDsByOrigin func() map[string]string, mutationMutex sync.Locker, log Logger) *Checker { | ||
| if mcpIDsByOrigin == nil { | ||
| panic("accesscontrol: New requires a non-nil MCP server ID resolver (use NoMCPServerIDs)") | ||
| } | ||
| if mutationMutex == nil { | ||
| mutationMutex = &sync.Mutex{} | ||
| } | ||
| lifecycleCtx, lifecycleCancel := context.WithCancel(context.Background()) | ||
| c := &Checker{ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Derive the checker lifecycle from a caller-provided context.
Line 88 detaches reconciliation from parent cancellation by using context.Background(). Accept ctx context.Context as the first New parameter and call context.WithCancel(ctx).
As per coding guidelines, production code must avoid context.Background() and thread context as the first parameter through the LLM call path.
🤖 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 `@accesscontrol/checker.go` around lines 81 - 89, Update the Checker
constructor New to accept context.Context as its first parameter, then derive
lifecycleCtx with context.WithCancel(ctx) instead of context.Background().
Thread this caller-provided context through the related LLM call path while
preserving existing cancellation behavior.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Addressed by the Option B redesign (63555580): the checker no longer runs a background reconciler — accesscontrol.New spawns no goroutine and has no internal context.Background() lifecycle; decision calls thread the request-scoped ctx.
@coderabbitai resolve
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
Comment on lines
+169
to
+195
| func (c *Checker) healDivergenceUnderLock(ctx context.Context, ref rebuildRef) bool { | ||
| c.mutationMu.Lock() | ||
| defer c.mutationMu.Unlock() | ||
|
|
||
| has, err := c.index.Has(ref.resourceType, ref.resourceID) | ||
| if err != nil { | ||
| logWarn(c.log, "ABAC policy index heal could not re-read the policy index under the lock", | ||
| "resource_type", ref.resourceType, "resource_id", ref.resourceID, "error", err.Error()) | ||
| return false | ||
| } | ||
| want, readable := c.policyMarkerTruth(ref.resourceID) | ||
| if !readable { | ||
| logDebug(c.log, "ABAC policy index heal could not re-read the policy under the lock; leaving the marker unchanged", | ||
| "resource_type", ref.resourceType, "resource_id", ref.resourceID) | ||
| return false | ||
| } | ||
| if has == want { | ||
| return true | ||
| } | ||
|
|
||
| // Last gate before mutating: a context canceled between the truth fetch | ||
| // and the write (plugin shutdown) must make the heal a no-op. | ||
| if ctx.Err() != nil { | ||
| logDebug(c.log, "ABAC policy index heal abandoned before its write: lifecycle context canceled", | ||
| "resource_type", ref.resourceType, "resource_id", ref.resourceID) | ||
| return false | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent new policy reads after lifecycle cancellation.
A heal cancelled while waiting for mutationMu can acquire the lock later and begin new index and plugin API reads.
accesscontrol/index_rebuild.go#L169-L195: checkctx.Err()immediately after acquiringmutationMu.accesscontrol/index_heal_test.go#L507-L535: add the mutex-wait cancellation scenario and assert no under-lock policy fetch occurs.
📍 Affects 2 files
accesscontrol/index_rebuild.go#L169-L195(this comment)accesscontrol/index_heal_test.go#L507-L535
🤖 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 `@accesscontrol/index_rebuild.go` around lines 169 - 195, Prevent canceled
heals from performing reads after waiting for mutationMu: in
Checker.healDivergenceUnderLock, check ctx.Err() immediately after acquiring the
mutex and return false before calling index.Has or policyMarkerTruth, preserving
the existing cancellation logging behavior. In accesscontrol/index_heal_test.go
lines 507-535, add a scenario where the heal waits on mutationMu, the context is
canceled, and then the lock is released; assert that no under-lock policy fetch
occurs.
Contributor
There was a problem hiding this comment.
No longer applicable — this file and the whole heal/rebuild machinery (including mutationMu) were deleted in 63555580; unavailable/error outcomes now simply deny.
@coderabbitai resolve
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
Comment on lines
+465
to
+520
| // The request's fields are applied to the freshest persisted entry inside | ||
| // the UpdateConfig transform, which runs atomically under the config | ||
| // advisory lock. Deriving the merged entry from the live snapshot read | ||
| // above would let two concurrent field updates overwrite each other: both | ||
| // would rebase onto the same stale in-memory state. | ||
| var updated mcp.PluginServerConfig | ||
| saved, err := a.configStore.UpdateConfig(func(prev *config.Config) (config.Config, error) { | ||
| if prev == nil { | ||
| // A nil persisted config must not be silently replaced by a | ||
| // zero-value baseline; doing so would clobber unrelated settings | ||
| // (services, bots, MCP flags) on the next save. | ||
| return config.Config{}, errors.New("no plugin configuration available") | ||
| } | ||
| // Clone to avoid mutating the store's cached pointer. | ||
| cfg := prev.Clone() | ||
|
|
||
| // Merge by PluginID against the persisted list rather than overwriting | ||
| // with the in-memory snapshot, which would silently drop entries for | ||
| // plugins that are persisted but currently inactive in memory. | ||
| merged := append([]mcp.PluginServerConfig(nil), cfg.MCP.PluginServers...) | ||
| mergedIdx := -1 | ||
| for i := range merged { | ||
| if merged[i].PluginID == pluginID { | ||
| mergedIdx = i | ||
| break | ||
| } | ||
| } | ||
|
|
||
| existing, getErr := a.configStore.GetConfig() | ||
| if getErr != nil { | ||
| c.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to load config for plugin-server save: %w", getErr)) | ||
| return | ||
| } | ||
| if existing == nil { | ||
| c.AbortWithError(http.StatusInternalServerError, errors.New("no plugin configuration available")) | ||
| return | ||
| } | ||
| // Clone to avoid mutating the store's cached pointer. | ||
| cfg := existing.Clone() | ||
|
|
||
| // Merge by PluginID against the persisted list rather than overwriting | ||
| // with the in-memory snapshot, which would silently drop entries for | ||
| // plugins that are persisted but currently inactive in memory. | ||
| merged := append([]mcp.PluginServerConfig(nil), cfg.MCP.PluginServers...) | ||
| mergedIdx := -1 | ||
| for i := range merged { | ||
| if merged[i].PluginID == updated.PluginID { | ||
| mergedIdx = i | ||
| break | ||
| // Field ownership: the live registry entry is authoritative for the | ||
| // plugin-owned fields (Name, Path, ExposeExternal) — the source | ||
| // plugin may have re-registered with new values since the entry was | ||
| // last persisted, and adopting the persisted object wholesale would | ||
| // resurrect the stale ones. Only the admin-owned fields (Enabled, | ||
| // ToolConfigs) are carried over from the freshest persisted entry, | ||
| // with the request patch applied on top. | ||
| base := live | ||
| if mergedIdx >= 0 { | ||
| base.Enabled = merged[mergedIdx].Enabled | ||
| base.ToolConfigs = merged[mergedIdx].ToolConfigs | ||
| } | ||
| } | ||
| if mergedIdx >= 0 { | ||
| merged[mergedIdx] = updated | ||
| } else { | ||
| merged = append(merged, updated) | ||
| } | ||
| cfg.MCP.PluginServers = merged | ||
| if req.Enabled != nil { | ||
| base.Enabled = *req.Enabled | ||
| } | ||
| if req.ToolConfigs != nil { | ||
| base.ToolConfigs = *req.ToolConfigs | ||
| } | ||
| updated = base | ||
|
|
||
| if err := a.configStore.SaveConfig(*cfg); err != nil { | ||
| if mergedIdx >= 0 { | ||
| merged[mergedIdx] = base | ||
| } else { | ||
| merged = append(merged, base) | ||
| } | ||
| cfg.MCP.PluginServers = merged | ||
| return *cfg, nil | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Synchronize the plugin-owned snapshot with re-registration.
live is captured before UpdateConfig; a concurrent re-registration can therefore persist stale Name, Path, or ExposeExternal values even though the later runtime patch preserves the newer entry. Read these fields under synchronization shared with registration, or reconcile the persisted entry when the patched runtime result differs. Add a test that re-registers between the initial read and the transform.
🤖 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_admin.go` around lines 465 - 520, Synchronize the plugin-owned fields
used by the UpdateConfig transform with the registration path so a
re-registration between the initial read and transform cannot persist stale
Name, Path, or ExposeExternal values. Update the logic around live and
UpdateConfig to read or reconcile the latest registered entry while preserving
the admin-owned patch behavior. Add a test that re-registers the plugin between
the initial read and transform and verifies the persisted fields use the newer
registration.
Contributor
There was a problem hiding this comment.
Fixed in b45f4951/7712c03f — updates now rebase on the freshest persisted entry inside the UpdateConfig transform; the live snapshot only supplies plugin-owned fields (Name, Path, ExposeExternal).
@coderabbitai resolve
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
…Option B) The server now resolves policy existence during ABAC unavailability (mattermost 4d1c1816fc): unavailable is returned only when a policy row exists at the resource ID, else no_policy. That makes no_policy trustworthy during outages, so the plugin-local fail-closed machinery is deleted wholesale: KV policy index, cluster mutation mutex, activation rebuild, decision-time reconciler, and the Checker lifecycle that managed them. - Amended decision tables: unavailable and call errors deny unconditionally, for all resources and all agent modes; no_policy keeps legacy behavior (incl. the deliberate attribute-based fail-open); the local invalid-ID -> no_policy short-circuit stays. - IsAvailable redesigned to probe the PAP read path (GetAccessControlPolicy on a fresh ID; 404 => available) because the old decision probe reports no_policy whether ABAC is up or down under the new server semantics; 30s cache kept, userID param dropped. - outcome.go deleted; model.AccessDecisionOutcome used directly, with unknown future values passed through to the checkers' deny default. - SavePolicy/DeletePolicy reduced to single plugin-API calls. - admin_guide.md: new outage semantics; lease-loss/self-healing notes removed. Co-authored-by: nick.misasi <[email protected]>
Replaces the DEV-ONLY local-path replace with the pseudo-version of the pushed mattermost cursor/agents-abac-platform-229e head (4d1c1816fc) so plugin CI can build without a sibling checkout. TEMPORARY: must be repointed to a master pseudo-version of server/public containing the merged API before this PR merges. Co-authored-by: nick.misasi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
accesscontrol/pdp_client.go (1)
39-47: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail closed on a nil decision. If
EvaluateAccessControlreturns(nil, nil),decision.Outcomewill panic here; return an error instead so callers deny access rather than crashing.🤖 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 `@accesscontrol/pdp_client.go` around lines 39 - 47, Update the access-control evaluation flow around EvaluateAccessControl to detect a nil decision when no error is returned. Return a descriptive error before accessing decision.Outcome, preserving the existing error recording and successful outcome handling so callers fail closed and deny access.server/main.go (1)
213-219: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon’t make the ID-migration broadcasts best-effort. The ABAC ID migration is already committed before these publishes run; if either send fails, peers can stay on stale config/agent state until another config change or restart, and there’s no retry path because the next activation will see
idsMigrated == false. Persist a retryable notification or reload peers from the migrated state before returning.🤖 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 `@server/main.go` around lines 213 - 219, Update the idsMigrated handling around PublishConfigUpdate and PublishAgentUpdate so migration broadcasts are not best-effort: when either publish fails, persist a retryable notification or reload peers from the migrated state before the activation returns. Ensure the committed ABAC ID migration cannot leave peers stale, and retain the existing error context while making the failure recoverable.
🧹 Nitpick comments (1)
accesscontrol/pap_test.go (1)
95-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the combined not-found checks table-driven.
This test exercises two operations as separate cases. Represent Get and Delete through named table rows so future error-mapping cases remain consistent.
As per coding guidelines, Go tests must be table-driven when they contain more than one case.
🤖 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 `@accesscontrol/pap_test.go` around lines 95 - 112, Refactor TestGetAndDeletePolicyNotFound into a named table-driven test with separate rows for GetPolicy and DeletePolicy, capturing each operation’s setup, invocation, and expected ErrPolicyNotFound result. Reuse shared IDs and API setup where appropriate, and run the rows through a subtest loop while preserving the existing not-found behavior and expectations.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.
Inline comments:
In `@docs/admin_guide.md`:
- Line 313: Update the ABAC engine availability sentence in the documented
policy behavior to add a comma after “for example,” while preserving the rest of
the wording and behavior description.
In `@go.mod`:
- Around line 236-241: Replace the dev-only pseudo-version in the server/public
replace directive with the released or master pseudo-version containing the
merged ABAC API. Update the dependency target for
github.com/mattermost/mattermost/server/public and remove the temporary
branch-pin state before merging; do not use a branch pin or local-path
replacement.
---
Outside diff comments:
In `@accesscontrol/pdp_client.go`:
- Around line 39-47: Update the access-control evaluation flow around
EvaluateAccessControl to detect a nil decision when no error is returned. Return
a descriptive error before accessing decision.Outcome, preserving the existing
error recording and successful outcome handling so callers fail closed and deny
access.
In `@server/main.go`:
- Around line 213-219: Update the idsMigrated handling around
PublishConfigUpdate and PublishAgentUpdate so migration broadcasts are not
best-effort: when either publish fails, persist a retryable notification or
reload peers from the migrated state before the activation returns. Ensure the
committed ABAC ID migration cannot leave peers stale, and retain the existing
error context while making the failure recoverable.
---
Nitpick comments:
In `@accesscontrol/pap_test.go`:
- Around line 95-112: Refactor TestGetAndDeletePolicyNotFound into a named
table-driven test with separate rows for GetPolicy and DeletePolicy, capturing
each operation’s setup, invocation, and expected ErrPolicyNotFound result. Reuse
shared IDs and API setup where appropriate, and run the rows through a subtest
loop while preserving the existing not-found behavior and expectations.
🪄 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: 0a614996-37b8-4fa5-ad2f-9efe2e95d079
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
accesscontrol/checker.goaccesscontrol/checker_test.goaccesscontrol/client.goaccesscontrol/doc.goaccesscontrol/logger.goaccesscontrol/pap.goaccesscontrol/pap_test.goaccesscontrol/passthrough.goaccesscontrol/pdp_client.goaccesscontrol/pdp_client_test.goapi/api_access_control.goapi/api_access_control_test.goapi/api_test.gobots/bots_test.gobots/permissions_test.goconversations/handle_messages_test.goconversations/test_helpers_test.godocs/admin_guide.mdgo.modserver/main.go
💤 Files with no reviewable changes (1)
- accesscontrol/logger.go
🚧 Files skipped from review as they are similar to previous changes (8)
- accesscontrol/doc.go
- accesscontrol/passthrough.go
- conversations/handle_messages_test.go
- conversations/test_helpers_test.go
- bots/permissions_test.go
- accesscontrol/client.go
- bots/bots_test.go
- api/api_access_control.go
Comment on lines
+236
to
+241
| // DEV-ONLY: branch pin for the in-flight ABAC plugin API (EvaluateAccessControl + PAP/CEL | ||
| // methods + Option B existence-resolving semantics). Currently the pseudo-version of the | ||
| // mattermost branch cursor/agents-abac-platform-229e head (4d1c1816fc). Before merge: repoint | ||
| // to a MASTER pseudo-version of server/public containing the merged API — never merge a | ||
| // branch pin, and never merge a local-path replace. | ||
| replace github.com/mattermost/mattermost/server/public => github.com/mattermost/mattermost/server/public v0.4.4-0.20260715162756-4d1c1816fcff |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '220,250p' go.modRepository: mattermost/mattermost-plugin-agents
Length of output: 1567
🏁 Script executed:
python3 - <<'PY'
print("need evidence")
PYRepository: mattermost/mattermost-plugin-agents
Length of output: 187
🏁 Script executed:
rg -n "server/public|ABAC|EvaluateAccessControl|PAP/CEL|Option B" go.mod . -g '!**/node_modules/**'Repository: mattermost/mattermost-plugin-agents
Length of output: 50393
Replace the temporary server/public branch pin before merging. go.mod:236-241 still points at a branch-head pseudo-version, so this would merge a dev-only dependency instead of the released server/public revision.
🤖 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 `@go.mod` around lines 236 - 241, Replace the dev-only pseudo-version in the
server/public replace directive with the released or master pseudo-version
containing the merged ABAC API. Update the dependency target for
github.com/mattermost/mattermost/server/public and remove the temporary
branch-pin state before merging; do not use a branch pin or local-path
replacement.
Contributor
There was a problem hiding this comment.
The server/public pseudo-version pin is deliberate while this PR consumes APIs from mattermost/mattermost#37509; it will be dropped once that merges.
@coderabbitai resolve
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
…lure The generated plugin RPC client logs and swallows transport errors for (*T, *model.AppError) methods, returning (nil, nil). Two consumers mishandled that: - PluginAPIClient.EvaluateAccessRequest would dereference a nil decision; it now returns an explicit transport-failure error, which the checkers' error path turns into a deny. - IsAvailable counted (nil, nil) as available; the probe now requires either the expected fresh-ID 404 or a non-nil policy, so a transport failure reads as unavailable. Also sharpen the no_policy wording in doc.go and admin_guide.md: legacy modes run the legacy allow/block checks, while attribute-based agents deliberately fail open without them. Co-authored-by: nick.misasi <[email protected]>
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)
docs/admin_guide.md (1)
305-305: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
manage-others'-agentsauthoring permission consistently.Line 321 grants this permission policy read/write/delete access, but Line 305 says only creators and agent admins can author policies. Clarify whether these users also receive the simplified editor access.
Also applies to: 317-321
🤖 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/admin_guide.md` at line 305, Update the Agents access-policy documentation around the simplified and advanced editor permissions to explicitly describe the `manage-others'-agents` permission consistently with the policy read/write/delete access described nearby. Clarify whether users with this permission can author policies through the simplified table editor, while preserving the existing distinctions for agent creators, agent admins, and system admins.
🤖 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 `@docs/admin_guide.md`:
- Line 305: Update the Agents access-policy documentation around the simplified
and advanced editor permissions to explicitly describe the
`manage-others'-agents` permission consistently with the policy
read/write/delete access described nearby. Clarify whether users with this
permission can author policies through the simplified table editor, while
preserving the existing distinctions for agent creators, agent admins, and
system admins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 944b3189-1fc2-4cbb-b235-228741b71e94
📒 Files selected for processing (6)
accesscontrol/checker.goaccesscontrol/checker_test.goaccesscontrol/doc.goaccesscontrol/pdp_client.goaccesscontrol/pdp_client_test.godocs/admin_guide.md
🚧 Files skipped from review as they are similar to previous changes (5)
- accesscontrol/doc.go
- accesscontrol/pdp_client_test.go
- accesscontrol/pdp_client.go
- accesscontrol/checker.go
- accesscontrol/checker_test.go
Co-authored-by: nick.misasi <[email protected]>
…servers Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…ode) Co-authored-by: nick.misasi <[email protected]>
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 `@bots/permissions_test.go`:
- Around line 388-447: Convert TestCheckUsageRestrictionsDowngradedServer’s
three sequential scenarios into a []struct{} table with case names and per-case
test functions or inputs, then execute them through a single t.Run loop.
Preserve the existing setup, assertions, and coverage for attribute-based
denial, legacy-mode behavior, and unrestricted service/MCP access, following the
table-driven pattern used by TestLegacyOnlyModeDecisionTable.
🪄 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: 5d4b84af-e881-48d4-b6e1-95a669ca7530
📒 Files selected for processing (11)
accesscontrol/checker.goaccesscontrol/doc.goaccesscontrol/passthrough.goaccesscontrol/version.goaccesscontrol/version_test.gobots/bots.gobots/permissions_test.godocs/admin_guide.mdgo.modserver/main.gowebapp/scripts/check_editor_contract.mjs
🚧 Files skipped from review as they are similar to previous changes (6)
- accesscontrol/passthrough.go
- bots/bots.go
- accesscontrol/doc.go
- webapp/scripts/check_editor_contract.mjs
- go.mod
- docs/admin_guide.md
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…ass stale snapshot gate Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…vers Co-authored-by: nick.misasi <[email protected]>
Contributor
|
@coderabbitai full review |
nickmisasi
marked this pull request as ready for review
July 21, 2026 04:04
![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: e668803695
ℹ️ 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".
Comment on lines
+124
to
+125
| if err := m.accessChecker.CanUseService(ctx, requestingUserID, cfg.ServiceID); err != nil { | ||
| return wrapDeny(err) |
There was a problem hiding this comment.
When an allowed primary service has a policy-denied service in its fallback chain, this gate checks only cfg.ServiceID; bots.EnsureBots still attaches the complete fallback chain and Bifrost invokes it when the primary fails. Consequently, a user denied access to the fallback service can still send completions through it during a primary outage. Evaluate every service in the resolved fallback chain for the requesting user before permitting the completion.
Useful? React with 👍 / 👎.
Comment on lines
+33
to
+34
| if report.Migrated { | ||
| reloaded, reloadErr := st.GetConfig() |
There was a problem hiding this comment.
During concurrent cluster activation, a node can load the pre-migration config before waiting on this mutex, then observe that another node already set the migration markers. In that case MigrateABACIDs returns report.Migrated == false, so this branch skips reloading and the node continues with legacy service IDs while agent rows in the database contain the remapped IDs; bots may fail service lookup and legacy IDs also short-circuit ABAC evaluation. Reload the active config after acquiring/running the migration regardless of which node performed the write.
Useful? React with 👍 / 👎.
Comment on lines
+243
to
+246
| deniedOrigins := m.deniedExternalOrigins(ctx, userID) | ||
|
|
||
| // Get or create client for this user (connects to remote servers only) | ||
| userClient, initialErrors := m.getClientForUser(ctx, userID) | ||
| mcpErrors := cloneMCPErrors(initialErrors) | ||
| userClient, initialErrors := m.getClientForUser(ctx, userID, deniedOrigins) |
There was a problem hiding this comment.
If a user's first request occurs while an MCP server is denied, the newly cached UserClients is created without that server. After the policy changes to allow it, this call still reuses the same client; the final filtering can remove newly denied tools but cannot add tools from a server that was never connected. Active users can therefore remain unable to use the newly allowed server indefinitely because each request refreshes the client activity timestamp, unless they explicitly refresh tools or become idle long enough for eviction.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Attribute-based access control for the three Agents resources, built on the new server plugin APIs (mattermost/mattermost#37509, mattermost/mattermost#37510, mattermost/enterprise#2231). The server owns policies (PAP), evaluation (PDP), and policy-existence resolution; this plugin is the enforcement point (PEP) — it stores no CEL, never evaluates locally, keeps no policy bookkeeping, and does not bundle cel-go.
Enforcement (JIT, no membership sync)
accesscontrolpackage:CheckerwithCanUseAgent/CanUseService/CanUseMCPServer/ValidateAgentWrite, calling the server decision API with explicitallow/deny/no_policy/unavailableoutcomes.no_policypreserves legacy behavior (deliberate channel-style fail-open for attribute-based agents).unavailableand transport errors simply always fail closed — outages deny exactly the resources a policy gates, with no client-side policy index.useare re-checked on every completion (DM, mention, regen, tool follow-up, bridge, search) via the composite gate inbots.CheckUsageRestrictionsForUserConfig.search_tools/load_toolare omitted when no authorized catalog remains. Filtering is silent, server-level.ValidateAgentWriterejects unauthorized service/MCP assignment, checking only newly-assigned resources on update).Authoring
actingUserID(no browser-to-core calls). Agent policies: agent creators/admins + system admins; service/MCP policies: system admins only (System Console).window.Components(feature-detected): simplified table editor for agent creators, advanced CEL editor for system admins; inline policy sections on the agent Access tab and the System Console service/MCP panels. Resources with legacy (non-26-char) IDs show an explanatory note instead of an editor. A generated host-contract snapshot + CI probe guards the cross-repo prop contract against drift.Migrations (one-time, activation, cluster-mutex + marker, single transaction)
services[].id,fallbackServiceID, config bots) andAgents_UserAgents.ServiceID(incl. soft-deleted; no DDL — 26 fits VARCHAR(36)). Future service IDs are server-minted. Note for support: restoring an old config-history row by hand also requires clearing theabac_service_id_migration_donemarker.UpdateConfig; the write primitive itself rejects reintroduction of legacy UUID service IDs post-migration.Not in scope (per spec): membership-sync jobs, per-tool MCP policies, share-time warnings, permission-policy scoping of
manage_own_agent/manage_others_agent.go.modpinsserver/publicto a pseudo-version of the open server PR branch (mattermost/mattermost#37509) so CI can build. Before merging, that PR must merge and the pin must be repointed to a master pseudo-version (commit subject marks it).min_server_versionstays 11.9.0 — the version gate handles older servers explicitly. Playwright e2e specs for ABAC flows are deferred until a server image containing the new plugin APIs exists for testcontainers; an outline ships ine2e/tests/agents/access-control.spec.ts, and the existing access-control e2e suite passes against the released 11.9 image (legacy-only mode).Screenshots
All demos recorded against a locally built server+enterprise (Enterprise Advanced license, ABAC enabled), with CPA attributes
department=Engineering|Saleson two test users, a mock OpenAI-compatible LLM, and a real external MCP server.demo1_author_agent_attribute_policy.mp4
Demo 1 — authoring an agent policy: Attribute-based access level, simple table editor, advanced CEL editor, test-rule modal, save + persistence.
demo2_agent_policy_enforcement_eng_allowed_sales_denied.mp4
Demo 2 — agent enforcement: Engineering user gets replies; Sales user has the agent filtered from lists and gets silent denial; an unrestricted agent still replies to Sales.
demo3_service_policy_review_and_per_completion_enforcement.mp4
Demo 3 — LLM service policy: admin-only console editor, legacy-ID note, and per-completion enforcement flipping the same agent between deny (Sales) and allow (Engineering).
demo4_mcp_policy_runtime_tool_filtering.mp4
Demo 4 — MCP server policy: console authoring and runtime tool filtering — Engineering sees the external server's tools, Sales sees it silently removed from the Tool Providers list.
Engineering: external MCP server present
Sales: external MCP server filtered out
Legacy-ID service note instead of policy editor
Release Note
To show artifacts inline, enable in settings.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation