From 4cb8d8940332b80a5cfec976154c9f328afbf02f Mon Sep 17 00:00:00 2001 From: Mattermost Build Date: Wed, 23 Jul 2025 08:28:42 +0300 Subject: [PATCH] [MM-64516] Do not allow user editable attributes to be used in ABAC table editor (#32522) (#33524) Automatic Merge --- .../lib/src/server/default_config.ts | 1 + server/public/model/config.go | 9 ++- .../table_editor/attribute_selector_menu.tsx | 18 +++++- .../table_editor/table_editor.test.tsx | 62 ++++++++++++++++++- .../editors/table_editor/table_editor.tsx | 33 +++++++++- .../policy_details.test.tsx.snap | 2 + .../access_control/policy_details/index.ts | 4 +- .../policy_details/policy_details.test.tsx | 5 ++ .../policy_details/policy_details.tsx | 19 ++++-- webapp/channels/src/i18n/en.json | 1 + .../src/selectors/entities/access_control.ts | 5 ++ webapp/platform/types/src/config.ts | 1 + 12 files changed, 148 insertions(+), 12 deletions(-) diff --git a/e2e-tests/playwright/lib/src/server/default_config.ts b/e2e-tests/playwright/lib/src/server/default_config.ts index 835fa769f6..8887dc41dc 100644 --- a/e2e-tests/playwright/lib/src/server/default_config.ts +++ b/e2e-tests/playwright/lib/src/server/default_config.ts @@ -803,6 +803,7 @@ const defaultServerConfig: AdminConfig = { AccessControlSettings: { EnableAttributeBasedAccessControl: false, EnableChannelScopeAccessControl: false, + EnableUserManagedAttributes: false, }, ContentFlaggingSettings: { NotificationSettings: { diff --git a/server/public/model/config.go b/server/public/model/config.go index 41f4312f89..b6cdc74bf7 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -3783,8 +3783,9 @@ func (s *ExportSettings) SetDefaults() { } type AccessControlSettings struct { - EnableAttributeBasedAccessControl *bool `access:"write_restrictable,cloud_restrictable"` - EnableChannelScopeAccessControl *bool `access:"cloud_restrictable"` + EnableAttributeBasedAccessControl *bool `access:"write_restrictable"` + EnableChannelScopeAccessControl *bool `access:"write_restrictable"` + EnableUserManagedAttributes *bool `access:"write_restrictable"` } func (s *AccessControlSettings) SetDefaults() { @@ -3795,6 +3796,10 @@ func (s *AccessControlSettings) SetDefaults() { if s.EnableChannelScopeAccessControl == nil { s.EnableChannelScopeAccessControl = NewPointer(false) } + + if s.EnableUserManagedAttributes == nil { + s.EnableUserManagedAttributes = NewPointer(false) + } } type ConfigFunc func() *Config diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/attribute_selector_menu.tsx b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/attribute_selector_menu.tsx index 0fccd5f95c..69cddd8c9b 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/attribute_selector_menu.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/attribute_selector_menu.tsx @@ -15,6 +15,7 @@ import { PoundIcon, InformationOutlineIcon, SyncIcon, + ShieldAlertOutlineIcon, } from '@mattermost/compass-icons/components'; import type IconProps from '@mattermost/compass-icons/components/props'; import type {UserPropertyField} from '@mattermost/types/properties'; @@ -62,9 +63,10 @@ interface AttributeSelectorProps { buttonId: string; autoOpen?: boolean; onMenuOpened?: () => void; + enableUserManagedAttributes: boolean; } -const AttributeSelectorMenu = ({currentAttribute, availableAttributes, disabled, onChange, menuId, buttonId, autoOpen = false, onMenuOpened}: AttributeSelectorProps) => { +const AttributeSelectorMenu = ({currentAttribute, availableAttributes, disabled, onChange, menuId, buttonId, autoOpen = false, onMenuOpened, enableUserManagedAttributes}: AttributeSelectorProps) => { const {formatMessage} = useIntl(); const [filter, setFilter] = useState(''); const prevAutoOpen = useRef(false); @@ -134,6 +136,7 @@ const AttributeSelectorMenu = ({currentAttribute, availableAttributes, disabled, const {name} = option; const hasSpaces = name.includes(' '); const isSynced = option.attrs?.ldap || option.attrs?.saml; + const allowed = isSynced || enableUserManagedAttributes; const menuItem = ( handleAttributeChange(name)} labels={{name}} - disabled={hasSpaces} + disabled={hasSpaces || !allowed} leadingElement={ )} + {!allowed && !isSynced && ( + + )} {isSynced && ( { test('handles "==" operator mapping to "is"', () => { @@ -143,3 +144,62 @@ describe('parseExpression', () => { ]); }); }); + +describe('findFirstAvailableAttributeFromList', () => { + const createMockAttribute = (name: string, attrs: Partial = {}): UserPropertyField => ({ + id: `id-${name}`, + group_id: 'custom_profile_attributes', + name, + type: 'text', + create_at: 0, + update_at: 0, + delete_at: 0, + attrs: { + sort_order: 1, + visibility: 'when_set', + value_type: '', + ...attrs, + }, + }); + + test('returns first attribute that is synced from LDAP', () => { + const attributes = [ + createMockAttribute('invalid attribute'), // Has spaces + createMockAttribute('unsafe_attribute'), // Not synced + createMockAttribute('ldap_attribute', {ldap: 'ldap_field'}), // Synced from LDAP + ]; + + const result = findFirstAvailableAttributeFromList(attributes, false); + expect(result?.name).toBe('ldap_attribute'); + }); + + test('returns first attribute that is synced from SAML', () => { + const attributes = [ + createMockAttribute('invalid attribute'), // Has spaces + createMockAttribute('saml_attribute', {saml: 'saml_field'}), // Synced from SAML + ]; + + const result = findFirstAvailableAttributeFromList(attributes, false); + expect(result?.name).toBe('saml_attribute'); + }); + + test('returns first user-managed attribute when enableUserManagedAttributes is true', () => { + const attributes = [ + createMockAttribute('invalid attribute'), // Has spaces - still skipped + createMockAttribute('user_managed_attribute'), // User managed + ]; + + const result = findFirstAvailableAttributeFromList(attributes, true); + expect(result?.name).toBe('user_managed_attribute'); + }); + + test('skips attributes with spaces even when synced', () => { + const attributes = [ + createMockAttribute('synced attribute', {ldap: 'ldap_field'}), // Has spaces but synced + createMockAttribute('valid_synced_attribute', {ldap: 'ldap_field'}), // Valid and synced + ]; + + const result = findFirstAvailableAttributeFromList(attributes, false); + expect(result?.name).toBe('valid_synced_attribute'); + }); +}); diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx index 28134ad447..99b71c56b0 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx @@ -27,12 +27,28 @@ interface TableEditorProps { onValidate?: (isValid: boolean) => void; disabled?: boolean; userAttributes: UserPropertyField[]; + enableUserManagedAttributes: boolean; onParseError: (error: string) => void; actions: { getVisualAST: (expr: string) => Promise; }; } +// Finds the first available (non-disabled) attribute from a list of user attributes. +// An attribute is considered available if it doesn't have spaces in its name (CEL incompatible) +// and is considered "safe" (synced from LDAP/SAML OR enableUserManagedAttributes is true). +export const findFirstAvailableAttributeFromList = ( + userAttributes: UserPropertyField[], + enableUserManagedAttributes: boolean, +): UserPropertyField | undefined => { + return userAttributes.find((attr) => { + const hasSpaces = attr.name.includes(' '); + const isSynced = attr.attrs?.ldap || attr.attrs?.saml; + const allowed = isSynced || enableUserManagedAttributes; + return !hasSpaces && allowed; + }); +}; + // Parses a CEL (Common Expression Language) string into a structured array of TableRow objects. // This allows the expression to be displayed and edited in a user-friendly table format. export const parseExpression = (visualAST: AccessControlVisualAST): TableRow[] => { @@ -86,6 +102,7 @@ function TableEditor({ onValidate, disabled = false, userAttributes, + enableUserManagedAttributes, onParseError, actions, }: TableEditorProps): JSX.Element { @@ -156,14 +173,25 @@ function TableEditor({ } }, [onChange, onValidate]); + // Helper function to find the first available (non-disabled) attribute + const findFirstAvailableAttribute = useCallback(() => { + return findFirstAvailableAttributeFromList(userAttributes, enableUserManagedAttributes); + }, [userAttributes, enableUserManagedAttributes]); + // Row Manipulation Handlers const addRow = useCallback(() => { if (userAttributes.length === 0) { return; // Do not add a row if no attributes are available } + + const firstAvailableAttribute = findFirstAvailableAttribute(); + if (!firstAvailableAttribute) { + return; // Do not add a row if no attributes are available + } + setRows((currentRows) => { const newRow = { - attribute: userAttributes[0]?.name || '', // Default to the first available attribute + attribute: firstAvailableAttribute.name, // Default to the first available attribute operator: OperatorLabel.IS, // Default operator values: [], }; @@ -172,7 +200,7 @@ function TableEditor({ setAutoOpenAttributeMenuForRow(newRows.length - 1); // Set for the new row return newRows; }); - }, [userAttributes, updateExpression]); + }, [userAttributes, updateExpression, findFirstAvailableAttribute]); const removeRow = useCallback((index: number) => { setRows((currentRows) => { @@ -295,6 +323,7 @@ function TableEditor({ buttonId={`attribute-selector-button-${index}`} autoOpen={index === autoOpenAttributeMenuForRow} onMenuOpened={() => setAutoOpenAttributeMenuForRow(null)} + enableUserManagedAttributes={enableUserManagedAttributes} /> diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap b/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap index ac5864f187..04d05777f4 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap @@ -100,6 +100,7 @@ exports[`components/admin_console/access_control/policy_details/PolicyDetails sh "getVisualAST": [MockFunction], } } + enableUserManagedAttributes={false} onChange={[Function]} onParseError={[Function]} onValidate={[Function]} @@ -303,6 +304,7 @@ exports[`components/admin_console/access_control/policy_details/PolicyDetails sh "getVisualAST": [MockFunction], } } + enableUserManagedAttributes={false} onChange={[Function]} onParseError={[Function]} onValidate={[Function]} diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/index.ts b/webapp/channels/src/components/admin_console/access_control/policy_details/index.ts index 97d8302be0..b33097addd 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/index.ts +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/index.ts @@ -7,7 +7,7 @@ import type {Dispatch} from 'redux'; import {getAccessControlPolicy as fetchPolicy, createAccessControlPolicy as createPolicy, deleteAccessControlPolicy as deletePolicy, searchAccessControlPolicyChannels as searchChannels, assignChannelsToAccessControlPolicy, unassignChannelsFromAccessControlPolicy, getAccessControlFields, updateAccessControlPolicyActive, getVisualAST} from 'mattermost-redux/actions/access_control'; import {createJob} from 'mattermost-redux/actions/jobs'; -import {getAccessControlPolicy as getPolicy} from 'mattermost-redux/selectors/entities/access_control'; +import {getAccessControlSettings, getAccessControlPolicy as getPolicy} from 'mattermost-redux/selectors/entities/access_control'; import {setNavigationBlocked} from 'actions/admin_actions.jsx'; @@ -26,9 +26,11 @@ type OwnProps = { function mapStateToProps(state: GlobalState, ownProps: OwnProps) { const policyId = ownProps.match.params.policy_id; const policy = getPolicy(state, policyId); + const config = getAccessControlSettings(state); return { policy, policyId, + accessControlSettings: config, }; } diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.test.tsx b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.test.tsx index 9c86172b1b..7a07756b45 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.test.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.test.tsx @@ -36,6 +36,11 @@ describe('components/admin_console/access_control/policy_details/PolicyDetails', const mockGetVisualAST = jest.fn(); const defaultProps = { policyId: 'policy1', + accessControlSettings: { + EnableAttributeBasedAccessControl: true, + EnableChannelScopeAccessControl: true, + EnableUserManagedAttributes: false, + }, channels: [ {id: 'channel1', name: 'Channel 1', display_name: 'Channel 1', team_display_name: 'Team 1', type: 'O'} as ChannelWithTeamData, {id: 'channel2', name: 'channel2', display_name: 'Channel 2', team_display_name: 'Team 2', type: 'P'} as ChannelWithTeamData, diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx index c9a52e4f9d..7f93359df1 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx @@ -8,6 +8,7 @@ import {FormattedMessage, useIntl} from 'react-intl'; import {GenericModal} from '@mattermost/components'; import type {AccessControlPolicy, AccessControlPolicyRule} from '@mattermost/types/access_control'; import type {ChannelSearchOpts, ChannelWithTeamData} from '@mattermost/types/channels'; +import type {AccessControlSettings} from '@mattermost/types/config'; import type {JobTypeBase} from '@mattermost/types/jobs'; import type {UserPropertyField} from '@mattermost/types/properties'; @@ -53,6 +54,7 @@ interface PolicyActions { export interface PolicyDetailsProps { policy?: AccessControlPolicy; policyId?: string; + accessControlSettings: AccessControlSettings; actions: PolicyActions; } @@ -66,6 +68,7 @@ function PolicyDetails({ policy, policyId, actions, + accessControlSettings, }: PolicyDetailsProps): JSX.Element { const [policyName, setPolicyName] = useState(policy?.name || ''); const [expression, setExpression] = useState(policy?.rules?.[0]?.expression || ''); @@ -457,10 +460,17 @@ function PolicyDetails({ setSaveNeeded(true); }} onValidate={() => {}} - userAttributes={autocompleteResult.map((attr) => ({ - attribute: attr.name, - values: [], - }))} + userAttributes={autocompleteResult. + filter((attr) => { + if (accessControlSettings.EnableUserManagedAttributes) { + return true; + } + return attr.attrs?.ldap || attr.attrs?.saml; + }). + map((attr) => ({ + attribute: attr.name, + values: [], + }))} /> ) : ( { setEditorMode('cel'); }} + enableUserManagedAttributes={accessControlSettings.EnableUserManagedAttributes} actions={{ getVisualAST: actions.getVisualAST, }} diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index 69d58dc87d..a739e02b44 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -323,6 +323,7 @@ "admin.access_control.table_editor.create_value": "Create \"{value}\"", "admin.access_control.table_editor.help_text": "Each row is a single condition that must be met for a user to comply with the policy. All rules are combined with logical AND operator (`&&`).", "admin.access_control.table_editor.learnMore": "Learn more about creating access expressions with examples.", + "admin.access_control.table_editor.not_safe_to_use": "Values for this attribute are managed by users and should not be used for access control. Please link attribute to AD/LDAP for use in access policies.", "admin.access_control.table_editor.operator": "Operator", "admin.access_control.table_editor.operator.contains": "contains", "admin.access_control.table_editor.operator.ends_with": "ends with", diff --git a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/access_control.ts b/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/access_control.ts index a56c194b21..52f41d9808 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/access_control.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/access_control.ts @@ -2,6 +2,7 @@ // See LICENSE.txt for license information. import type {Channel, ChannelWithTeamData, ChannelSearchOpts} from '@mattermost/types/channels'; +import type {AccessControlSettings} from '@mattermost/types/config'; import type {GlobalState} from '@mattermost/types/store'; import {filterChannelsMatchingTerm} from 'mattermost-redux/utils/channel_utils'; @@ -10,6 +11,10 @@ import {filterChannelList} from './channels'; import {createSelector} from '../create_selector'; +export function getAccessControlSettings(state: GlobalState): AccessControlSettings { + return state.entities.admin.config.AccessControlSettings as AccessControlSettings; +} + export function getAccessControlPolicy(state: GlobalState, id: string) { return state.entities.admin.accessControlPolicies[id]; } diff --git a/webapp/platform/types/src/config.ts b/webapp/platform/types/src/config.ts index cf5d805c5b..02bcc5a30f 100644 --- a/webapp/platform/types/src/config.ts +++ b/webapp/platform/types/src/config.ts @@ -990,6 +990,7 @@ export type ExportSettings = { export type AccessControlSettings = { EnableAttributeBasedAccessControl: boolean; EnableChannelScopeAccessControl: boolean; + EnableUserManagedAttributes: boolean; }; export type ContentFlaggingNotificationSettings = {