Adds group search base to panel. Fixes settings testing.

frontend/src/metabase/admin/settings/components/SettingsLdapForm.jsx

@@ -72,19 +72,25 @@ export default class SettingsLdapForm extends Component {
         let valid = true,
             validationErrors = {};
 
-        elements.forEach(function(element) {
-            // test for required elements
-            if (element.required && MetabaseUtils.isEmpty(formData[element.key])) {
-                valid = false;
-            }
-
-            if (element.validations) {
-                element.validations.forEach(function(validation) {
-                    validationErrors[element.key] = this.validateElement(validation, formData[element.key], element);
-                    if (validationErrors[element.key]) valid = false;
-                }, this);
-            }
-        }, this);
+        if (formData['ldap-enabled']) {
+            elements.forEach(function(element) {
+                if (element.key === 'ldap-group-base' && !formData['ldap-group-sync']) {
+                    return;
+                }
+
+                // test for required elements
+                if (element.required && MetabaseUtils.isEmpty(formData[element.key])) {
+                    valid = false;
+                }
+
+                if (element.validations) {
+                    element.validations.forEach(function(validation) {
+                        validationErrors[element.key] = this.validateElement(validation, formData[element.key], element);
+                        if (validationErrors[element.key]) valid = false;
+                    }, this);
+                }
+            }, this);
+        }
 
         if (this.state.valid !== valid || !_.isEqual(this.state.validationErrors, validationErrors)) {
             this.setState({ valid, validationErrors });
@@ -117,14 +123,14 @@ export default class SettingsLdapForm extends Component {
     updateLdapSettings(e) {
         e.preventDefault();
 
-        this.setState({
-            formErrors: null,
-            submitting: "working"
-        });
-
         let { formData, valid } = this.state;
 
         if (valid) {
+            this.setState({
+                formErrors: null,
+                submitting: "working"
+            });
+
             this.props.updateLdapSettings(formData).then(() => {
                 this.setState({
                     dirty: false,
@@ -151,19 +157,6 @@ export default class SettingsLdapForm extends Component {
             let errorMessage = (formErrors && formErrors.elements) ? formErrors.elements[element.key] : validationErrors[element.key];
             let value = formData[element.key] == null ? element.defaultValue : formData[element.key];
 
-            if (element.key === "ldap-enabled") {
-                let configuredEnough = formData["ldap-host"] && formData['ldap-bind-dn'] && formData['ldap-password'] && formData['ldap-user-base'];
-                return (
-                    <SettingsSetting
-                        key={element.key}
-                        setting={{ ...element, value }}
-                        updateSetting={(value) => this.handleChangeEvent(element, value)}
-                        errorMessage={errorMessage}
-                        disabled={!configuredEnough}
-                    />
-                );
-            }
-
             return (
                 <SettingsSetting
                     key={element.key}

frontend/src/metabase/admin/settings/selectors.js

@@ -202,7 +202,7 @@ const SECTIONS = [
             },
             {
                 key: "ldap-user-base",
-                display_name: "Search base",
+                display_name: "User search base",
                 type: "string",
                 required: true
             },
@@ -232,6 +232,12 @@ const SECTIONS = [
                 display_name: "Synchronize groups",
                 description: null,
                 type: "boolean"
+            },
+            {
+                key: "ldap-group-base",
+                display_name: "Group search base",
+                type: "string",
+                required: true
             }
         ]
     },

src/metabase/api/ldap.clj

@@ -27,7 +27,6 @@
 (defn- humanize-error-messages
   "Convert raw error message responses from our LDAP tests into our normal api error response structure."
   [{:keys [status message]}]
-  (println message)
   (when (not= :SUCCESS status)
     (log/warn "Problem connecting to LDAP server:" message)
     (let [conn-error       {:errors {:ldap-host "Wrong host or port"
@@ -73,7 +72,9 @@
   (check-superuser)
   (let [ldap-settings (select-keys settings (keys mb-settings->ldap-details))
         ldap-details  (-> (set/rename-keys ldap-settings mb-settings->ldap-details)
-                          (assoc :port (Integer/parseInt (:ldap-port settings))))
+                          (assoc :port
+                            (when-not (empty? (:ldap-port settings))
+                              (Integer/parseInt (:ldap-port settings)))))
         results       (if (or config/is-test? (not (:ldap-enabled settings)))
                         ;; for unit testing or disabled status just respond with a success message
                         {:status :SUCCESS}

src/metabase/integrations/ldap.clj

@@ -128,6 +128,7 @@
         {:status  :ERROR
          :message "User search base does not exist or is unreadable"}))
     (catch Exception e
+      ;; ActiveDirectory annoyingly throws for every little thing
       {:status  :ERROR
        :message (.getMessage e)})))
 
@@ -147,6 +148,7 @@
               fname (get result fname-attr)
               lname (get result lname-attr)
               email (get result email-attr)]
+          ;; Make sure we got everything as these are all required for new accounts
           (when-not (or (empty? dn) (empty? fname) (empty? lname) (empty? email))
             ;; ActiveDirectory (and others?) will supply a `memberOf` overlay attribute for groups
             ;; Otherwise we have to make the inverse query to get them