ABAC: share TableEditor/CELEditor with plugins via window.Components - #37510
Conversation
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
…Event clicks Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
📝 WalkthroughWalkthroughAdds injectable validation and user-search actions to ABAC editors, exposes the editors through lazy-loaded plugin components, and adds tests for fallback behavior, schema handling, rendering, and ChangesABAC editor integration
Lazy plugin editor exports
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CELEditor
participant ValidationAction
participant TestResultsModal
participant SearchAction
CELEditor->>ValidationAction: validate CEL expression
ValidationAction-->>CELEditor: return validation errors
CELEditor->>TestResultsModal: open access rule test
TestResultsModal->>SearchAction: search matching users
SearchAction-->>TestResultsModal: return access test results
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx (1)
482-493: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated searchUsers thunk-wrapping into a shared helper. Both editors implement the identical "use injected
searchUserswrapped as a zero-arg thunk, else return thesearchUsersForExpressionthunk" logic; a future signature change to this pattern would need to be updated in both places consistently.
webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx#L482-L493: extract this branch (usingeditorState.expression) into a shared helper, e.g.wrapSearchUsersAction(injected, fallback).webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx#L679-L692: extract this branch (usingvalue) into the same shared helper.♻️ Proposed shared helper
// e.g. in a shared module such as editors/search_users_action.ts import {searchUsersForExpression} from 'mattermost-redux/actions/access_control'; import type {AccessControlTestResult} from '`@mattermost/types/access_control`'; import type {ActionResult} from 'mattermost-redux/types/actions'; export function wrapSearchUsersAction( injected: ((expression: string, term: string, after: string, limit: number) => Promise<ActionResult<AccessControlTestResult>>) | undefined, expression: string, channelId?: string, teamId?: string, ) { return (term: string, after: string, limit: number) => { if (injected) { return () => injected(expression, term, after, limit); } return searchUsersForExpression(expression, term, after, limit, channelId, teamId); }; }🤖 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/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx` around lines 482 - 493, The duplicated searchUsers thunk-wrapping logic should be centralized in a shared wrapSearchUsersAction helper. Update webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx lines 482-493 to call it with editorState.expression, and update webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx lines 679-692 to call it with value; the helper must preserve the injected zero-argument thunk behavior and searchUsersForExpression fallback, including channelId and teamId.
🤖 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.
Nitpick comments:
In
`@webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx`:
- Around line 482-493: The duplicated searchUsers thunk-wrapping logic should be
centralized in a shared wrapSearchUsersAction helper. Update
webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx
lines 482-493 to call it with editorState.expression, and update
webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx
lines 679-692 to call it with value; the helper must preserve the injected
zero-argument thunk behavior and searchUsersForExpression fallback, including
channelId and teamId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cae552c4-81b1-4e29-b50c-49e790a192e7
📒 Files selected for processing (8)
webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.test.tsxwebapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsxwebapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsxwebapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor_channel_admin.test.tsxwebapp/channels/src/plugins/access_control_editors.test.tsxwebapp/channels/src/plugins/access_control_editors.tswebapp/channels/src/plugins/export.test.tswebapp/channels/src/plugins/export.ts
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
|
@pvev this lays the groundwork for the upcoming ABAC in agents as a whole. We need to have it so that the CEL funnels through the Agents backend so it knows the rules exist for the resources, and the easiest way was to add props that we can override on the window. The Agents side is still in draft here: mattermost/mattermost-plugin-agents#886 |
| import {lazy} from 'react'; | ||
|
|
||
| // Lazy-wrapped so monaco-editor stays out of the main channels bundle; consumers render inside <Suspense>. | ||
| export const AccessControlTableEditor = lazy(() => import(/* webpackChunkName: 'access-control-editors' */ 'components/admin_console/access_control/editors/table_editor/table_editor')); |
There was a problem hiding this comment.
question: is the lazy call directly here intentional? I asked because I checked how other published components do and they provide their own suspense, like published_modals.ts for example.
There was a problem hiding this comment.
Yep, intentional. Same code-splitting pattern as published_modals/published_editor so Monaco stays out of the main channels bundle. Difference is we leave the Suspense boundary to the plugin consumer rather than wrapping it here (those helpers add one because ModalController doesn’t).
Summary
Makes the ABAC policy editors reusable by plugins (first consumer: the Agents plugin, for agent/service/MCP resource policies) without changing any existing admin-console or channel-settings behavior.
CELEditorgains an optionalactionsprop (checkExpression,searchUsers). When provided, the editor makes zero directClient4/redux calls — validation and the test-results modal go through the injected callbacks. When omitted, behavior is exactly as today (all 6 existing caller sites verified unchanged).TableEditor's existingactionsprop gains an optionalsearchUserswith the same fallback discipline;TestResultsModaland child components needed no changes (injected callbacks are wrapped in a pass-through thunk).plugins/access_control_editors.tsexports both editors asReact.lazycomponents onwindow.Components(AccessControlTableEditor,AccessControlCELEditor). Lazy export keeps Monaco out of the main bundle — verified via production webpack stats (zero Monaco modules in initial chunks); consuming plugins render inside their ownSuspenseand never bundle Monaco.Client4/redux are not hit on the injected path), export presence assertions, and a lazy-export smoke test.Ticket: https://mattermost.atlassian.net/browse/MM-69921
Screenshot of the shared editors rendering inside the Agents plugin (simple/table mode with attribute autocomplete, backed entirely by injected callbacks):


Release Note
To show artifacts inline, enable in settings.
Change Impact: 🟡 Medium
Regression Risk: Changes span shared editor components and public plugin exports in authorization-related flows, though injected and fallback paths are covered by automated tests.
QA Recommendation: Manual QA can be skipped; automated coverage is sufficient.
Generated by CodeRabbitAI