diff --git a/server/channels/app/properties/access_control_attribute_validation.go b/server/channels/app/properties/access_control_attribute_validation.go index c645e54eb9c..b485fe8cc05 100644 --- a/server/channels/app/properties/access_control_attribute_validation.go +++ b/server/channels/app/properties/access_control_attribute_validation.go @@ -35,7 +35,7 @@ type PermissionChecker func(userID string, permission *model.Permission) bool // - trims whitespace on string attrs // - applies the visibility default when unset // - clears attrs that don't apply to the field type (options on non-select, -// ldap/saml on non-text or admin-managed fields) +// ldap/saml on non-text fields) // - auto-assigns IDs to options that lack one and validates option shape // - validates visibility, value_type, managed, display_name, and sort_order // - validates property values for text fields against value_type @@ -92,9 +92,8 @@ func (h *AccessControlAttributeValidationHook) sanitizeAndValidateFieldAttrs(fie field.Attrs[model.PropertyFieldAttrVisibility] = model.PropertyFieldVisibilityWhenSet } - // Type-based attr clearing: select-shaped fields keep options, only text - // supports external sync, and admin-managed fields can never be synced - // (mutual exclusivity). + // Type-based attr clearing: select-shaped fields keep options and only + // text fields support external sync. isSelect := field.Type == model.PropertyFieldTypeSelect || field.Type == model.PropertyFieldTypeMultiselect isText := field.Type == model.PropertyFieldTypeText managed, _ := field.Attrs[model.PropertyFieldAttrManaged].(string) @@ -102,7 +101,7 @@ func (h *AccessControlAttributeValidationHook) sanitizeAndValidateFieldAttrs(fie if !isSelect { delete(field.Attrs, model.PropertyFieldAttributeOptions) } - if !isText || managed == "admin" { + if !isText { delete(field.Attrs, model.PropertyFieldAttrLDAP) delete(field.Attrs, model.PropertyFieldAttrSAML) } diff --git a/server/channels/app/properties/access_control_attribute_validation_test.go b/server/channels/app/properties/access_control_attribute_validation_test.go index 90a93e57d46..005fa754056 100644 --- a/server/channels/app/properties/access_control_attribute_validation_test.go +++ b/server/channels/app/properties/access_control_attribute_validation_test.go @@ -1121,3 +1121,195 @@ func TestAccessControlAttributeValidationHookManagedAuthorization(t *testing.T) assert.Contains(t, createErr.Error(), "managed=admin") }) } + +func TestAccessControlAttributeValidationHookSync(t *testing.T) { + th := Setup(t) + + group, err := th.service.RegisterPropertyGroup(&model.PropertyGroup{Name: "test_attr_sync", Version: model.PropertyGroupVersionV2}) + require.NoError(t, err) + + adminUserID := model.NewId() + permChecker := func(userID string, perm *model.Permission) bool { + return userID == adminUserID && perm.Id == model.PermissionManageSystem.Id + } + + hook := NewAccessControlAttributeValidationHook(th.service, permChecker, group.ID) + th.service.AddHook(hook) + + adminRctx := RequestContextWithCallerID(th.Context, adminUserID) + + t.Run("user-editable text field keeps the ldap sync attr", func(t *testing.T) { + field := &model.PropertyField{ + GroupID: group.ID, + Name: "field_" + model.NewId(), + Type: model.PropertyFieldTypeText, + TargetType: "system", + ObjectType: "user", + Attrs: model.StringInterface{ + model.PropertyFieldAttrLDAP: "employeeID", + }, + } + created, createErr := th.service.CreatePropertyField(th.Context, field) + require.NoError(t, createErr) + assert.Equal(t, "employeeID", created.Attrs[model.PropertyFieldAttrLDAP]) + }) + + t.Run("admin-managed text field keeps the ldap sync attr", func(t *testing.T) { + field := &model.PropertyField{ + GroupID: group.ID, + Name: "field_" + model.NewId(), + Type: model.PropertyFieldTypeText, + TargetType: "system", + ObjectType: "user", + Attrs: model.StringInterface{ + model.PropertyFieldAttrManaged: "admin", + model.PropertyFieldAttrLDAP: "employeeID", + }, + } + created, createErr := th.service.CreatePropertyField(adminRctx, field) + require.NoError(t, createErr) + assert.Equal(t, "employeeID", created.Attrs[model.PropertyFieldAttrLDAP]) + assert.Equal(t, "admin", created.Attrs[model.PropertyFieldAttrManaged]) + }) + + t.Run("admin-managed text field keeps the saml sync attr", func(t *testing.T) { + field := &model.PropertyField{ + GroupID: group.ID, + Name: "field_" + model.NewId(), + Type: model.PropertyFieldTypeText, + TargetType: "system", + ObjectType: "user", + Attrs: model.StringInterface{ + model.PropertyFieldAttrManaged: "admin", + model.PropertyFieldAttrSAML: "position", + }, + } + created, createErr := th.service.CreatePropertyField(adminRctx, field) + require.NoError(t, createErr) + assert.Equal(t, "position", created.Attrs[model.PropertyFieldAttrSAML]) + }) + + t.Run("linking an existing admin-managed field keeps the ldap sync attr on update", func(t *testing.T) { + field := &model.PropertyField{ + GroupID: group.ID, + Name: "field_" + model.NewId(), + Type: model.PropertyFieldTypeText, + TargetType: "system", + ObjectType: "user", + Attrs: model.StringInterface{ + model.PropertyFieldAttrManaged: "admin", + }, + } + created, createErr := th.service.CreatePropertyField(adminRctx, field) + require.NoError(t, createErr) + require.Empty(t, created.Attrs[model.PropertyFieldAttrLDAP]) + + created.Attrs[model.PropertyFieldAttrLDAP] = "employeeID" + updated, _, updateErr := th.service.UpdatePropertyField(adminRctx, group.ID, created) + require.NoError(t, updateErr) + assert.Equal(t, "employeeID", updated.Attrs[model.PropertyFieldAttrLDAP]) + }) + + t.Run("adding managed to an ldap-synced field keeps the ldap sync attr on update", func(t *testing.T) { + field := &model.PropertyField{ + GroupID: group.ID, + Name: "field_" + model.NewId(), + Type: model.PropertyFieldTypeText, + TargetType: "system", + ObjectType: "user", + Attrs: model.StringInterface{ + model.PropertyFieldAttrLDAP: "employeeID", + }, + } + created, createErr := th.service.CreatePropertyField(th.Context, field) + require.NoError(t, createErr) + + created.Attrs[model.PropertyFieldAttrManaged] = "admin" + updated, _, updateErr := th.service.UpdatePropertyField(adminRctx, group.ID, created) + require.NoError(t, updateErr) + assert.Equal(t, "employeeID", updated.Attrs[model.PropertyFieldAttrLDAP]) + assert.Equal(t, "admin", updated.Attrs[model.PropertyFieldAttrManaged]) + }) + + t.Run("clearing ldap on an unmanaged field keeps managed unset", func(t *testing.T) { + field := &model.PropertyField{ + GroupID: group.ID, + Name: "field_" + model.NewId(), + Type: model.PropertyFieldTypeText, + TargetType: "system", + ObjectType: "user", + Attrs: model.StringInterface{ + model.PropertyFieldAttrLDAP: "employeeID", + }, + } + created, createErr := th.service.CreatePropertyField(th.Context, field) + require.NoError(t, createErr) + + created.Attrs[model.PropertyFieldAttrLDAP] = "" + updated, _, updateErr := th.service.UpdatePropertyField(th.Context, group.ID, created) + require.NoError(t, updateErr) + assert.Equal(t, "", updated.Attrs[model.PropertyFieldAttrLDAP]) + assert.NotContains(t, updated.Attrs, model.PropertyFieldAttrManaged) + }) + + t.Run("clearing ldap on an admin-managed field keeps managed admin", func(t *testing.T) { + field := &model.PropertyField{ + GroupID: group.ID, + Name: "field_" + model.NewId(), + Type: model.PropertyFieldTypeText, + TargetType: "system", + ObjectType: "user", + Attrs: model.StringInterface{ + model.PropertyFieldAttrManaged: "admin", + model.PropertyFieldAttrLDAP: "employeeID", + }, + } + created, createErr := th.service.CreatePropertyField(adminRctx, field) + require.NoError(t, createErr) + + created.Attrs[model.PropertyFieldAttrLDAP] = "" + updated, _, updateErr := th.service.UpdatePropertyField(adminRctx, group.ID, created) + require.NoError(t, updateErr) + assert.Equal(t, "", updated.Attrs[model.PropertyFieldAttrLDAP]) + assert.Equal(t, "admin", updated.Attrs[model.PropertyFieldAttrManaged]) + }) + + t.Run("linking an existing admin-managed field keeps the saml sync attr on update", func(t *testing.T) { + field := &model.PropertyField{ + GroupID: group.ID, + Name: "field_" + model.NewId(), + Type: model.PropertyFieldTypeText, + TargetType: "system", + ObjectType: "user", + Attrs: model.StringInterface{ + model.PropertyFieldAttrManaged: "admin", + }, + } + created, createErr := th.service.CreatePropertyField(adminRctx, field) + require.NoError(t, createErr) + require.Empty(t, created.Attrs[model.PropertyFieldAttrSAML]) + + created.Attrs[model.PropertyFieldAttrSAML] = "position" + updated, _, updateErr := th.service.UpdatePropertyField(adminRctx, group.ID, created) + require.NoError(t, updateErr) + assert.Equal(t, "position", updated.Attrs[model.PropertyFieldAttrSAML]) + }) + + t.Run("non-text field strips ldap and saml sync attrs", func(t *testing.T) { + field := &model.PropertyField{ + GroupID: group.ID, + Name: "field_" + model.NewId(), + Type: model.PropertyFieldTypeSelect, + TargetType: "system", + ObjectType: "user", + Attrs: model.StringInterface{ + model.PropertyFieldAttrLDAP: "employeeID", + model.PropertyFieldAttrSAML: "position", + }, + } + created, createErr := th.service.CreatePropertyField(th.Context, field) + require.NoError(t, createErr) + assert.NotContains(t, created.Attrs, model.PropertyFieldAttrLDAP) + assert.NotContains(t, created.Attrs, model.PropertyFieldAttrSAML) + }) +} diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.test.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.test.tsx index d95062fa138..de405e15b74 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.test.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.test.tsx @@ -10,7 +10,7 @@ import {Client4} from 'mattermost-redux/client'; import ModalController from 'components/modal_controller'; -import {renderWithContext, screen, userEvent, waitFor} from 'tests/react_testing_utils'; +import {renderWithContext, screen, userEvent, waitFor, within} from 'tests/react_testing_utils'; import DotMenu from './user_properties_dot_menu'; import {useUserPropertyFields} from './user_properties_utils'; @@ -186,6 +186,55 @@ describe('UserPropertyDotMenu', () => { expect(screen.getByText('Edit SAML link')).toBeInTheDocument(); }); + it('sets ldap from the modal without adding managed to an unmanaged field', async () => { + renderComponent(); + + const menuButton = screen.getByTestId(`user-property-field_dotmenu-${baseField.id}`); + await userEvent.click(menuButton); + await userEvent.click(screen.getByText('Link attribute to AD/LDAP')); + + await userEvent.type(await screen.findByRole('textbox'), 'employeeID'); + await userEvent.click(screen.getByRole('button', {name: 'Save'})); + + expect(updateField).toHaveBeenCalledWith({ + ...baseField, + type: 'text', + attrs: { + ...baseField.attrs, + ldap: 'employeeID', + }, + }); + }); + + it('sets ldap from the modal without changing managed on an admin-managed field', async () => { + const adminManagedField: UserPropertyField = { + ...baseField, + id: 'admin-managed-ldap-modal', + attrs: { + ...baseField.attrs, + managed: 'admin', + }, + }; + + renderComponent(adminManagedField); + + const menuButton = screen.getByTestId(`user-property-field_dotmenu-${adminManagedField.id}`); + await userEvent.click(menuButton); + await userEvent.click(screen.getByText('Link attribute to AD/LDAP')); + + await userEvent.type(await screen.findByRole('textbox'), 'employeeID'); + await userEvent.click(screen.getByRole('button', {name: 'Save'})); + + expect(updateField).toHaveBeenCalledWith({ + ...adminManagedField, + type: 'text', + attrs: { + ...adminManagedField.attrs, + ldap: 'employeeID', + }, + }); + }); + it('clears admin-managed by setting managed to empty string, not by removing the key', async () => { const adminManagedField: UserPropertyField = { ...baseField, @@ -218,6 +267,112 @@ describe('UserPropertyDotMenu', () => { }); }); + it('keeps the "Editable by users" toggle enabled for an admin-managed field that is not synced', async () => { + const adminManagedField: UserPropertyField = { + ...baseField, + id: 'admin-managed-unsynced', + attrs: { + ...baseField.attrs, + managed: 'admin', + }, + }; + + renderComponent(adminManagedField); + + const menuButton = screen.getByTestId(`user-property-field_dotmenu-${adminManagedField.id}`); + await userEvent.click(menuButton); + + const editableItem = screen.getByRole('menuitemcheckbox', {name: /Editable by users/}); + expect(editableItem).toHaveAttribute('aria-checked', 'false'); + expect(within(editableItem).getByRole('button')).toBeEnabled(); + expect(screen.queryByText('Synced attributes are managed by AD/LDAP or SAML')).not.toBeInTheDocument(); + }); + + it('disables the "Editable by users" toggle and reports it off when the field is synced via LDAP', async () => { + const ldapSyncedField: UserPropertyField = { + ...baseField, + id: 'ldap-synced-field', + attrs: { + ...baseField.attrs, + ldap: 'employeeID', + }, + }; + + renderComponent(ldapSyncedField); + + const menuButton = screen.getByTestId(`user-property-field_dotmenu-${ldapSyncedField.id}`); + await userEvent.click(menuButton); + + const editableItem = screen.getByRole('menuitemcheckbox', {name: /Editable by users/}); + expect(editableItem).toHaveAttribute('aria-checked', 'false'); + expect(within(editableItem).getByRole('button')).toBeDisabled(); + expect(screen.getByText('Synced attributes are managed by AD/LDAP or SAML')).toBeInTheDocument(); + }); + + it('does not update a synced field when clicking the "Editable by users" toggle', async () => { + const ldapSyncedField: UserPropertyField = { + ...baseField, + id: 'ldap-synced-toggle-click', + attrs: { + ...baseField.attrs, + ldap: 'employeeID', + }, + }; + + renderComponent(ldapSyncedField); + + const menuButton = screen.getByTestId(`user-property-field_dotmenu-${ldapSyncedField.id}`); + await userEvent.click(menuButton); + + const editableItem = screen.getByRole('menuitemcheckbox', {name: /Editable by users/}); + editableItem.click(); + + expect(updateField).not.toHaveBeenCalled(); + }); + + it('disables the "Editable by users" toggle when the field is synced via SAML', async () => { + const samlSyncedField: UserPropertyField = { + ...baseField, + id: 'saml-synced-field', + attrs: { + ...baseField.attrs, + saml: 'position', + }, + }; + + renderComponent(samlSyncedField); + + const menuButton = screen.getByTestId(`user-property-field_dotmenu-${samlSyncedField.id}`); + await userEvent.click(menuButton); + + const editableItem = screen.getByRole('menuitemcheckbox', {name: /Editable by users/}); + expect(editableItem).toHaveAttribute('aria-checked', 'false'); + expect(within(editableItem).getByRole('button')).toBeDisabled(); + expect(screen.getByText('Synced attributes are managed by AD/LDAP or SAML')).toBeInTheDocument(); + }); + + it('disables the "Editable by users" toggle when the field is both admin-managed and synced', async () => { + const adminManagedSyncedField: UserPropertyField = { + ...baseField, + id: 'admin-managed-synced-field', + attrs: { + ...baseField.attrs, + managed: 'admin', + ldap: 'employeeID', + }, + }; + + renderComponent(adminManagedSyncedField); + + const menuButton = screen.getByTestId(`user-property-field_dotmenu-${adminManagedSyncedField.id}`); + await userEvent.click(menuButton); + + const editableItem = screen.getByRole('menuitemcheckbox', {name: /Editable by users/}); + expect(editableItem).toHaveAttribute('aria-checked', 'false'); + expect(within(editableItem).getByRole('button')).toBeDisabled(); + expect(screen.getByText('Synced attributes are managed by AD/LDAP or SAML')).toBeInTheDocument(); + }); + it('handles field duplication', async () => { renderComponent(); diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.tsx index eb9d08fc653..af064926ad3 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.tsx @@ -120,6 +120,9 @@ const DotMenu = ({ const isProtected = Boolean(field.attrs?.protected); + const isSynced = Boolean(field.attrs.ldap || field.attrs.saml); + const isEditableByUsers = !isSynced && field.attrs.managed !== 'admin'; + const handleDuplicate = () => { const name = `${slugifyForCEL(field.name)}_copy`; createField({...field, attrs: {...field.attrs}, name}); @@ -139,6 +142,10 @@ const DotMenu = ({ }; const handleEditableByUsersToggle = () => { + if (isSynced) { + return; + } + const newAttrs = {...field.attrs}; if (field.attrs.managed === 'admin') { @@ -276,10 +283,26 @@ const DotMenu = ({ } - labels={( + labels={isSynced ? ( + <> + + + + + + + + ) : ( diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index b01d8c77baa..806874c2bb5 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -3236,6 +3236,7 @@ "admin.system_properties.user_properties.dotmenu.delete.label": "Delete attribute", "admin.system_properties.user_properties.dotmenu.duplicate.label": "Duplicate attribute", "admin.system_properties.user_properties.dotmenu.editable_by_users.label": "Editable by users", + "admin.system_properties.user_properties.dotmenu.editable_by_users.synced_help": "Synced attributes are managed by AD/LDAP or SAML", "admin.system_properties.user_properties.dotmenu.saml.edit_link.label": "Edit SAML link", "admin.system_properties.user_properties.dotmenu.saml.link_property.label": "Link attribute to SAML", "admin.system_properties.user_properties.dotmenu.saml.modal.helpText": "The attribute in the SAML server used to sync as a custom attribute in user's profile in Mattermost.",