From 177a6b345ada1681060f3359a238d22dd7166d72 Mon Sep 17 00:00:00 2001 From: shaun Date: Fri, 8 Dec 2023 09:57:26 -0600 Subject: [PATCH] Migrate SettingsLdapForm to formik (#36054) --- .../scenarios/admin-2/sso/ldap.cy.spec.js | 28 +- .../src/metabase-enterprise/auth/index.js | 7 - .../settings/components/SettingsLdapForm.jsx | 97 ------ .../components/SettingsLdapForm.styled.tsx | 6 - .../settings/components/SettingsLdapForm.tsx | 214 ++++++++++++ .../components/SettingsLdapForm.unit.spec.tsx | 320 ++++++++++++++++++ .../GroupMappingsWidget.jsx | 5 +- .../src/metabase/plugins/builtin/auth/ldap.js | 2 +- 8 files changed, 556 insertions(+), 123 deletions(-) delete mode 100644 frontend/src/metabase/admin/settings/components/SettingsLdapForm.jsx delete mode 100644 frontend/src/metabase/admin/settings/components/SettingsLdapForm.styled.tsx create mode 100644 frontend/src/metabase/admin/settings/components/SettingsLdapForm.tsx create mode 100644 frontend/src/metabase/admin/settings/components/SettingsLdapForm.unit.spec.tsx diff --git a/e2e/test/scenarios/admin-2/sso/ldap.cy.spec.js b/e2e/test/scenarios/admin-2/sso/ldap.cy.spec.js index ec5eb47d7a94e..5663d10ccfbc5 100644 --- a/e2e/test/scenarios/admin-2/sso/ldap.cy.spec.js +++ b/e2e/test/scenarios/admin-2/sso/ldap.cy.spec.js @@ -91,22 +91,28 @@ describe( it("shouldn't be possible to save a non-numeric port (#13313)", () => { cy.visit("/admin/settings/authentication/ldap"); + cy.findByLabelText("LDAP Port").parent().parent().as("portSection"); + enterLdapSettings(); enterLdapPort("asd"); - // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.findByText("That's not a valid port number").should("exist"); + cy.get("@portSection") + .findByText("That's not a valid port number") + .should("exist"); enterLdapPort("21.3"); - cy.button("Save and enable").click(); - cy.wait("@updateLdapSettings"); - // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.findByText('For input string: "21.3"').should("exist"); + cy.get("@portSection") + .findByText("That's not a valid port number") + .should("exist"); + + enterLdapPort("389 "); + cy.get("@portSection") + .findByText("That's not a valid port number") + .should("not.exist"); - enterLdapPort("123 "); - cy.button("Save failed").click(); + cy.button("Save and enable").click(); cy.wait("@updateLdapSettings"); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.findByText('For input string: "123 "').should("exist"); + cy.findByText("Success").should("exist"); }); it("should allow user login on OSS when LDAP is enabled", () => { @@ -211,9 +217,9 @@ const enterLdapPort = value => { }; const enterLdapSettings = () => { - typeAndBlurUsingLabel("LDAP Host", "localhost"); + typeAndBlurUsingLabel(/LDAP Host/, "localhost"); typeAndBlurUsingLabel("LDAP Port", "389"); typeAndBlurUsingLabel("Username or DN", "cn=admin,dc=example,dc=org"); typeAndBlurUsingLabel("Password", "adminpass"); - typeAndBlurUsingLabel("User search base", "ou=users,dc=example,dc=org"); + typeAndBlurUsingLabel(/User search base/, "ou=users,dc=example,dc=org"); }; diff --git a/enterprise/frontend/src/metabase-enterprise/auth/index.js b/enterprise/frontend/src/metabase-enterprise/auth/index.js index 03a4cfb1b9181..96b14b902cd09 100644 --- a/enterprise/frontend/src/metabase-enterprise/auth/index.js +++ b/enterprise/frontend/src/metabase-enterprise/auth/index.js @@ -253,13 +253,6 @@ if (hasPremiumFeature("sso_ldap")) { key: "ldap-group-membership-filter", display_name: t`Group membership filter`, type: "string", - validations: [ - value => - (value.match(/\(/g) || []).length !== - (value.match(/\)/g) || []).length - ? t`Check your parentheses` - : null, - ], }, { key: "ldap-sync-admin-group", diff --git a/frontend/src/metabase/admin/settings/components/SettingsLdapForm.jsx b/frontend/src/metabase/admin/settings/components/SettingsLdapForm.jsx deleted file mode 100644 index 169924026cbb9..0000000000000 --- a/frontend/src/metabase/admin/settings/components/SettingsLdapForm.jsx +++ /dev/null @@ -1,97 +0,0 @@ -import { useCallback } from "react"; -import PropTypes from "prop-types"; -import { t } from "ttag"; -import { connect } from "react-redux"; -import { updateLdapSettings } from "metabase/admin/settings/settings"; -import SettingsBatchForm from "./SettingsBatchForm"; -import { FormButton } from "./SettingsLdapForm.styled"; - -const propTypes = { - settingValues: PropTypes.object.isRequired, - onSubmit: PropTypes.func.isRequired, -}; - -const SettingsLdapForm = ({ settingValues, onSubmit, ...props }) => { - const isEnabled = settingValues["ldap-enabled"]; - const layout = getLayout(settingValues); - const breadcrumbs = getBreadcrumbs(); - - const handleSubmit = useCallback( - values => { - return onSubmit({ ...values, "ldap-enabled": true }); - }, - [onSubmit], - ); - - return ( - ( - - )} - /> - ); -}; - -SettingsLdapForm.propTypes = propTypes; - -const getLayout = settingValues => { - return [ - { - title: t`Server Settings`, - settings: [ - "ldap-host", - "ldap-port", - "ldap-security", - "ldap-bind-dn", - "ldap-password", - ], - }, - { - title: t`User Schema`, - settings: ["ldap-user-base", "ldap-user-filter"], - }, - { - title: t`Attributes`, - collapse: true, - settings: [ - "ldap-attribute-email", - "ldap-attribute-firstname", - "ldap-attribute-lastname", - ], - }, - { - title: t`Group Schema`, - settings: [ - "ldap-group-sync", - "ldap-group-base", - "ldap-group-membership-filter" in settingValues - ? "ldap-group-membership-filter" - : null, - "ldap-sync-admin-group" in settingValues - ? "ldap-sync-admin-group" - : null, - ].filter(Boolean), - }, - ]; -}; - -const getBreadcrumbs = () => { - return [[t`Authentication`, "/admin/settings/authentication"], [t`LDAP`]]; -}; - -const mapDispatchToProps = { - onSubmit: updateLdapSettings, -}; - -export default connect(null, mapDispatchToProps)(SettingsLdapForm); diff --git a/frontend/src/metabase/admin/settings/components/SettingsLdapForm.styled.tsx b/frontend/src/metabase/admin/settings/components/SettingsLdapForm.styled.tsx deleted file mode 100644 index 81085a54677bc..0000000000000 --- a/frontend/src/metabase/admin/settings/components/SettingsLdapForm.styled.tsx +++ /dev/null @@ -1,6 +0,0 @@ -import styled from "@emotion/styled"; -import ActionButton from "metabase/components/ActionButton"; - -export const FormButton = styled(ActionButton)` - margin-right: 0.5rem; -`; diff --git a/frontend/src/metabase/admin/settings/components/SettingsLdapForm.tsx b/frontend/src/metabase/admin/settings/components/SettingsLdapForm.tsx new file mode 100644 index 0000000000000..3fe4c3302f675 --- /dev/null +++ b/frontend/src/metabase/admin/settings/components/SettingsLdapForm.tsx @@ -0,0 +1,214 @@ +import { useCallback, useMemo } from "react"; +import { t } from "ttag"; +import _ from "underscore"; +import { connect } from "react-redux"; +import * as Yup from "yup"; +import type { TestConfig } from "yup"; + +import { updateLdapSettings } from "metabase/admin/settings/settings"; + +import { Stack, Group, Radio } from "metabase/ui"; +import { + Form, + FormErrorMessage, + FormProvider, + FormRadioGroup, + FormSubmitButton, + FormSwitch, + FormTextInput, +} from "metabase/forms"; +import Breadcrumbs from "metabase/components/Breadcrumbs"; +import { FormSection } from "metabase/containers/FormikForm"; +import GroupMappingsWidget from "metabase/admin/settings/containers/GroupMappingsWidget"; +import type { SettingValue } from "metabase-types/api"; +import type { SettingElement } from "metabase/admin/settings/types"; + +const testParentheses: TestConfig = { + name: "test-parentheses", + message: "Check your parentheses", + test: value => + (value?.match(/\(/g) || []).length === (value?.match(/\)/g) || []).length, +}; + +const testPort: TestConfig = { + name: "test-port", + message: "That's not a valid port number", + test: value => Boolean((value || "").trim().match(/^\d*$/)), +}; + +const LDAP_SCHEMA = Yup.object({ + "ldap-port": Yup.string().nullable().test(testPort), + "ldap-user-filter": Yup.string().nullable().test(testParentheses), + "ldap-group-membership-filter": Yup.string().nullable().test(testParentheses), +}); + +export type SettingValues = { [key: string]: SettingValue }; + +type LdapFormSettingElement = Omit & { + key: string; // ensuring key is required + is_env_setting?: boolean; + env_name?: string; + default?: any; +}; + +type Props = { + elements: LdapFormSettingElement[]; + settingValues: SettingValues; + onSubmit: (values: SettingValues) => void; +}; + +export const SettingsLdapFormView = ({ + elements = [], + settingValues, + onSubmit, +}: Props) => { + const isEnabled = settingValues["ldap-enabled"]; + + const settings = useMemo(() => { + return _.indexBy(elements, "key"); + }, [elements]); + + const fields = useMemo(() => { + return _.mapObject(settings, setting => ({ + name: setting.key, + label: setting.display_name, + description: setting.description, + placeholder: setting.is_env_setting + ? t`Using ${setting.env_name}` + : setting.placeholder || setting.default, + required: setting.required, + autoFocus: setting.autoFocus, + })); + }, [settings]); + + const attributeValues = useMemo(() => { + return getAttributeValues(settingValues); + }, [settingValues]); + + const handleSubmit = useCallback( + values => { + return onSubmit({ + ...values, + "ldap-port": values["ldap-port"]?.trim(), + "ldap-enabled": true, + }); + }, + [onSubmit], + ); + + return ( + + {({ dirty }) => ( +
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + {"ldap-group-membership-filter" in fields && + "ldap-group-membership-filter" in settingValues && ( + + )} + {"ldap-sync-admin-group" in fields && + "ldap-sync-admin-group" in settingValues && ( + + )} + + + + + + + + )} +
+ ); +}; + +const LDAP_ATTRS = [ + // Server Settings + "ldap-host", + "ldap-port", + "ldap-security", + "ldap-bind-dn", + "ldap-password", + + // User Schema + "ldap-user-base", + "ldap-user-filter", + + // Attributes + "ldap-attribute-email", + "ldap-attribute-firstname", + "ldap-attribute-lastname", + + // Group Schema + "ldap-group-sync", + "ldap-group-base", + "ldap-group-membership-filter", + "ldap-sync-admin-group", +]; + +const getAttributeValues = (values: SettingValues) => { + return Object.fromEntries(LDAP_ATTRS.map(key => [key, values[key]])); +}; + +const mapDispatchToProps = { + onSubmit: updateLdapSettings, +}; + +export const SettingsLdapForm = connect( + null, + mapDispatchToProps, +)(SettingsLdapFormView); diff --git a/frontend/src/metabase/admin/settings/components/SettingsLdapForm.unit.spec.tsx b/frontend/src/metabase/admin/settings/components/SettingsLdapForm.unit.spec.tsx new file mode 100644 index 0000000000000..4d290e6c695d8 --- /dev/null +++ b/frontend/src/metabase/admin/settings/components/SettingsLdapForm.unit.spec.tsx @@ -0,0 +1,320 @@ +import fetchMock from "fetch-mock"; + +import userEvent from "@testing-library/user-event"; +import { renderWithProviders, screen, waitFor } from "__support__/ui"; +import { createMockGroup } from "metabase-types/api/mocks"; + +import { SettingsLdapFormView } from "./SettingsLdapForm"; +import type { SettingValues } from "./SettingsLdapForm"; + +const GROUPS = [ + createMockGroup(), + createMockGroup({ id: 2, name: "Administrators" }), + createMockGroup({ id: 3, name: "foo" }), + createMockGroup({ id: 4, name: "bar" }), + createMockGroup({ id: 5, name: "flamingos" }), +]; + +const elements = [ + { + // placeholder: false, + key: "ldap-enabled", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_ENABLED", + // description: null, + default: false, + originalValue: null, + display_name: "LDAP Authentication", + type: "boolean", + }, + { + placeholder: "ldap.yourdomain.org", + key: "ldap-host", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_HOST", + description: "Server hostname.", + default: null, + originalValue: null, + display_name: "LDAP Host", + type: "string", + required: true, + autoFocus: true, + }, + { + placeholder: "389", + key: "ldap-port", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_PORT", + description: "Server port, usually 389 or 636 if SSL is used.", + default: 389, + originalValue: null, + display_name: "LDAP Port", + type: "string", + }, + { + placeholder: "none", + key: "ldap-security", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_SECURITY", + // description: null, + default: "none", + originalValue: null, + display_name: "LDAP Security", + type: "radio", + defaultValue: "none", + }, + { + // placeholder: null, + key: "ldap-bind-dn", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_BIND_DN", + description: + "The Distinguished Name to bind as (if any), this user will be used to lookup information about other users.", + default: null, + originalValue: null, + display_name: "Username or DN", + type: "string", + }, + { + // placeholder: null, + key: "ldap-password", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_PASSWORD", + description: "The password to bind with for the lookup user.", + default: null, + originalValue: null, + display_name: "Password", + type: "password", + }, + { + // placeholder: null, + key: "ldap-user-base", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_USER_BASE", + description: "Search base for users. (Will be searched recursively)", + default: null, + originalValue: null, + display_name: "User search base", + type: "string", + required: true, + }, + { + placeholder: "(&(objectClass=inetOrgPerson)(|(uid={login})(mail={login})))", + key: "ldap-user-filter", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_USER_FILTER", + description: + "User lookup filter. The placeholder {login} will be replaced by the user supplied login.", + default: "(&(objectClass=inetOrgPerson)(|(uid={login})(mail={login})))", + originalValue: null, + display_name: "User filter", + type: "string", + }, + { + placeholder: "mail", + key: "ldap-attribute-email", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_ATTRIBUTE_EMAIL", + description: + "Attribute to use for the user's email. (usually 'mail', 'email' or 'userPrincipalName')", + default: "mail", + originalValue: null, + display_name: "Email attribute", + type: "string", + }, + { + placeholder: "givenName", + key: "ldap-attribute-firstname", + value: "givenname", + is_env_setting: false, + env_name: "MB_LDAP_ATTRIBUTE_FIRSTNAME", + description: + "Attribute to use for the user's first name. (usually 'givenName')", + default: "givenName", + originalValue: "givenname", + display_name: "First name attribute", + type: "string", + }, + { + placeholder: "sn", + key: "ldap-attribute-lastname", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_ATTRIBUTE_LASTNAME", + description: "Attribute to use for the user's last name. (usually 'sn')", + default: "sn", + originalValue: null, + display_name: "Last name attribute", + type: "string", + }, + { + // placeholder: false, + key: "ldap-group-sync", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_GROUP_SYNC", + // description: null, + default: false, + originalValue: null, + }, + { + // placeholder: null, + key: "ldap-group-base", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_GROUP_BASE", + description: + "Search base for groups. Not required for LDAP directories that provide a 'memberOf' overlay, such as Active Directory. (Will be searched recursively)", + default: null, + originalValue: null, + display_name: "Group search base", + type: "string", + }, + { + // placeholder: {}, + key: "ldap-group-mappings", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_GROUP_MAPPINGS", + description: "JSON containing LDAP to Metabase group mappings.", + default: {}, + originalValue: null, + }, + { + placeholder: "(member={dn})", + key: "ldap-group-membership-filter", + value: null, + is_env_setting: false, + env_name: "MB_LDAP_GROUP_MEMBERSHIP_FILTER", + description: + "Group membership lookup filter. The placeholders {dn} and {uid} will be replaced by the user's Distinguished Name and UID, respectively.", + default: "(member={dn})", + originalValue: null, + display_name: "Group membership filter", + type: "string", + }, + { + key: "ldap-sync-admin-group", + display_name: "Sync Administrator group", + type: "boolean", + }, +]; + +const setup = (settingValues: SettingValues) => { + const onSubmit = jest.fn(); + + fetchMock.get("path:/api/permissions/group", GROUPS); + + renderWithProviders( + , + {}, + ); + + return { + onSubmit, + }; +}; + +describe("SettingsLdapForm", () => { + it("should submit the correct payload", async () => { + const { onSubmit } = setup({ + "ldap-enabled": true, + "ldap-group-membership-filter": null, + "ldap-sync-admin-group": null, + }); + + const ATTRS = { + "ldap-host": "example.com", + "ldap-port": "123", + "ldap-security": "ssl", + "ldap-user-base": "user-base", + "ldap-user-filter": "(filter1)", + "ldap-bind-dn": "username", + "ldap-password": "password", + "ldap-attribute-email": "john@example.com", + "ldap-attribute-firstname": "John", + "ldap-attribute-lastname": "Doe", + "ldap-enabled": true, + "ldap-group-sync": true, + "ldap-group-base": "group-base", + "ldap-group-membership-filter": "(filter2)", + "ldap-sync-admin-group": true, + }; + + userEvent.type( + await screen.findByRole("textbox", { name: /LDAP Host/ }), + ATTRS["ldap-host"], + ); + userEvent.type( + await screen.findByRole("textbox", { name: /LDAP Port/ }), + ATTRS["ldap-port"], + ); + userEvent.click(screen.getByRole("radio", { name: /SSL/ })); + userEvent.type( + await screen.findByRole("textbox", { name: /Username or DN/ }), + ATTRS["ldap-bind-dn"], + ); + userEvent.type(screen.getByLabelText(/Password/), ATTRS["ldap-password"]); + userEvent.type( + await screen.findByRole("textbox", { name: /User search base/ }), + ATTRS["ldap-user-base"], + ); + userEvent.type( + await screen.findByRole("textbox", { name: /User filter/ }), + ATTRS["ldap-user-filter"], + ); + userEvent.type( + await screen.findByRole("textbox", { name: /Email attribute/ }), + ATTRS["ldap-attribute-email"], + ); + userEvent.type( + await screen.findByRole("textbox", { name: /First name attribute/ }), + ATTRS["ldap-attribute-firstname"], + ); + userEvent.type( + await screen.findByRole("textbox", { name: /Last name attribute/ }), + ATTRS["ldap-attribute-lastname"], + ); + userEvent.click(screen.getByTestId("group-sync-switch")); // checkbox for "ldap-group-sync" + userEvent.type( + await screen.findByRole("textbox", { name: /Group search base/ }), + ATTRS["ldap-group-base"], + ); + userEvent.type( + await screen.findByRole("textbox", { name: /Group membership filter/ }), + ATTRS["ldap-group-membership-filter"], + ); + userEvent.click( + screen.getByRole("checkbox", { name: /Sync Administrator group/ }), + ); + + userEvent.click(await screen.findByRole("button", { name: /Save/ })); + + await waitFor(() => { + expect(onSubmit).toHaveBeenCalledWith(ATTRS); + }); + }); + + it("should hide group membership and sync admin group fields when appropriate", async () => { + setup({ "ldap-enabled": true }); + expect( + screen.queryByRole("textbox", { name: /Group membership filter/ }), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole("checkbox", { name: /Sync Administrator group/ }), + ).not.toBeInTheDocument(); + }); +}); diff --git a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupMappingsWidget.jsx b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupMappingsWidget.jsx index 69a2b99266bd0..721a552eccf89 100644 --- a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupMappingsWidget.jsx +++ b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupMappingsWidget.jsx @@ -108,7 +108,10 @@ function GroupMappingsWidget({ {t`Synchronize Group Memberships`} {isFormik ? ( // temporary until SettingsJWTForm and SettingsLdapForm are migrated to formik - + ) : ( )} diff --git a/frontend/src/metabase/plugins/builtin/auth/ldap.js b/frontend/src/metabase/plugins/builtin/auth/ldap.js index 43636408486c5..e6bbcc70830a4 100644 --- a/frontend/src/metabase/plugins/builtin/auth/ldap.js +++ b/frontend/src/metabase/plugins/builtin/auth/ldap.js @@ -6,7 +6,7 @@ import { PLUGIN_IS_PASSWORD_USER, } from "metabase/plugins"; -import SettingsLdapForm from "metabase/admin/settings/components/SettingsLdapForm"; +import { SettingsLdapForm } from "metabase/admin/settings/components/SettingsLdapForm"; import LdapAuthCard from "metabase/admin/settings/auth/containers/LdapAuthCard"; import GroupMappingsWidget from "metabase/admin/settings/containers/GroupMappingsWidget";