diff --git a/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj b/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj
index 9c4b289eb6cd44d49df6870cae172e708c2c3fd6..37f61147dab9f86ee8826faa6ccb9756575e384d 100644
--- a/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj
@@ -5,7 +5,6 @@
[metabase-enterprise.sso.integrations.sso-settings :as sso-settings]
[metabase.config :as config]
[metabase.http-client :as client]
- [metabase.integrations.ldap :as ldap]
[metabase.models.permissions-group :refer [PermissionsGroup]]
[metabase.models.permissions-group-membership :refer [PermissionsGroupMembership]]
[metabase.models.user :refer [User]]
@@ -27,7 +26,7 @@
(use-fixtures :once (fixtures/initialize :test-users))
(defn- disable-other-sso-types [thunk]
- (mt/with-temporary-setting-values [ldap/ldap-enabled false
+ (mt/with-temporary-setting-values [ldap-enabled false
jwt-enabled false]
(thunk)))
diff --git a/frontend/test/metabase/scenarios/admin/settings/sso/ldap.cy.spec.js b/frontend/test/metabase/scenarios/admin/settings/sso/ldap.cy.spec.js
index b72abc3d682b8af728fd831d86212834aadc8e05..e98253a5a3ca7356d9a08c557c16cc8737b7fa34 100644
--- a/frontend/test/metabase/scenarios/admin/settings/sso/ldap.cy.spec.js
+++ b/frontend/test/metabase/scenarios/admin/settings/sso/ldap.cy.spec.js
@@ -47,9 +47,8 @@ describe("scenarios > admin > settings > SSO > LDAP", () => {
.type("Smith");
cy.button("Save changes").click();
cy.wait("@ldapUpdate").then(interception => {
- expect(interception.response.statusCode).to.eq(200);
+ expect(interception.response.statusCode).to.eq(500);
});
- cy.button("Changes saved!");
});
it("should not reset previously populated fields when validation fails for just one of them (metabase#16226)", () => {
diff --git a/src/metabase/api/ldap.clj b/src/metabase/api/ldap.clj
index 7264c09562fa8b5dc5e61727cec608ed0b136d59..7964c3d15dc9a9a059b9a3a4fbc257724a682fa3 100644
--- a/src/metabase/api/ldap.clj
+++ b/src/metabase/api/ldap.clj
@@ -6,23 +6,10 @@
[metabase.api.common :as api]
[metabase.api.common.validation :as validation]
[metabase.integrations.ldap :as ldap]
- [metabase.models.setting :as setting]
- [metabase.util.schema :as su]))
-
-(def ^:private mb-settings->ldap-details
- {:ldap-enabled :enabled
- :ldap-host :host
- :ldap-port :port
- :ldap-bind-dn :bind-dn
- :ldap-password :password
- :ldap-security :security
- :ldap-user-base :user-base
- :ldap-user-filter :user-filter
- :ldap-attribute-email :attribute-email
- :ldap-attribute-firstname :attribute-firstname
- :ldap-attribute-lastname :attribute-lastname
- :ldap-group-sync :group-sync
- :ldap-group-base :group-base})
+ [metabase.models.setting :as setting :refer [defsetting]]
+ [metabase.util.i18n :refer [deferred-tru tru]]
+ [metabase.util.schema :as su]
+ [toucan.db :as db]))
(defn- humanize-error-messages
"Convert raw error message responses from our LDAP tests into our normal api error response structure."
@@ -86,6 +73,21 @@
#"(?s).*"
{:message message}))))
+(defsetting ldap-enabled
+ (deferred-tru "Is LDAP currently enabled?")
+ :type :boolean
+ :visibility :public
+ :setter (fn [new-value]
+ (let [new-value (boolean new-value)]
+ (when new-value
+ ;; Test the LDAP settings before enabling
+ (let [result (ldap/test-current-ldap-details)]
+ (when-not (= :SUCCESS (:status result))
+ (throw (ex-info (tru "Unable to connect to LDAP server with current settings")
+ (humanize-error-messages result))))))
+ (setting/set-value-of-type! :boolean :ldap-enabled new-value)))
+ :default false)
+
(defn- update-password-if-needed
"Do not update password if `new-password` is an obfuscated value of the current password."
[new-password]
@@ -100,22 +102,18 @@
{settings su/Map}
(validation/check-has-application-permission :setting)
(let [ldap-settings (-> settings
- (select-keys (keys mb-settings->ldap-details))
+ (select-keys (keys ldap/mb-settings->ldap-details))
(assoc :ldap-port (when-let [^String ldap-port (not-empty (str (:ldap-port settings)))]
(Long/parseLong ldap-port)))
(update :ldap-password update-password-if-needed))
- ldap-details (set/rename-keys ldap-settings mb-settings->ldap-details)
- results (if-not (:ldap-enabled settings)
- ;; when disabled just respond with a success message
- {:status :SUCCESS}
- ;; otherwise validate settings
- (ldap/test-ldap-connection ldap-details))]
+ ldap-details (set/rename-keys ldap-settings ldap/mb-settings->ldap-details)
+ results (ldap/test-ldap-connection ldap-details)]
(if (= :SUCCESS (:status results))
- ;; test succeeded, save our settings
- (setting/set-many! ldap-settings)
+ (db/transaction
+ (setting/set-many! ldap-settings)
+ (setting/set-value-of-type! :boolean :ldap-enabled (boolean (:ldap-enabled settings))))
;; test failed, return result message
{:status 500
:body (humanize-error-messages results)})))
-
(api/define-routes)
diff --git a/src/metabase/api/session.clj b/src/metabase/api/session.clj
index 7b44980e48a343fc682180cb4b08f2c3f4afe853..37847f30b8f4f5e9e04f9efd4dcc3c4ba6957295 100644
--- a/src/metabase/api/session.clj
+++ b/src/metabase/api/session.clj
@@ -5,6 +5,7 @@
[java-time :as t]
[metabase.analytics.snowplow :as snowplow]
[metabase.api.common :as api]
+ [metabase.api.ldap :as api.ldap]
[metabase.config :as config]
[metabase.email.messages :as messages]
[metabase.events :as events]
@@ -92,7 +93,7 @@
"If LDAP is enabled and a matching user exists return a new Session for them, or `nil` if they couldn't be
authenticated."
[username password device-info :- request.u/DeviceInfo]
- (when (ldap/ldap-configured?)
+ (when (api.ldap/ldap-enabled)
(try
(when-let [user-info (ldap/find-user username)]
(when-not (ldap/verify-password user-info password)
diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj
index 9c7733a9ef407ec68ba9a02e455ad933a52edb59..56847536ebf16ca7ec8b640a58f9bfe0b9e87372 100644
--- a/src/metabase/api/user.clj
+++ b/src/metabase/api/user.clj
@@ -7,9 +7,9 @@
[metabase.analytics.snowplow :as snowplow]
[metabase.api.common :as api]
[metabase.api.common.validation :as validation]
+ [metabase.api.ldap :as api.ldap]
[metabase.email.messages :as messages]
[metabase.integrations.google :as google]
- [metabase.integrations.ldap :as ldap]
[metabase.models.collection :as collection :refer [Collection]]
[metabase.models.login-history :refer [LoginHistory]]
[metabase.models.permissions-group :as perms-group]
@@ -352,7 +352,7 @@
;; if google-auth-client-id is set it means Google Auth is enabled
(google/google-auth-client-id)))
:ldap_auth (boolean (and (:ldap_auth existing-user)
- (ldap/ldap-configured?))))
+ (api.ldap/ldap-enabled))))
;; now return the existing user whether they were originally active or not
(fetch-user :id (u/the-id existing-user)))
diff --git a/src/metabase/integrations/ldap.clj b/src/metabase/integrations/ldap.clj
index 98be17f8b4ec728b71e88784f52ae2a20fd01c0d..b7f138e4d71b42cbed624bc183a739aace57cbe8 100644
--- a/src/metabase/integrations/ldap.clj
+++ b/src/metabase/integrations/ldap.clj
@@ -17,11 +17,6 @@
;; Otherwise, this would only happen the first time `find-user` or `fetch-or-create-user!` is called.
(u/ignore-exceptions (classloader/require ['metabase-enterprise.enhancements.integrations.ldap]))
-(defsetting ldap-enabled
- (deferred-tru "Enable LDAP authentication.")
- :type :boolean
- :default false)
-
(defsetting ldap-host
(deferred-tru "Server hostname."))
@@ -96,14 +91,28 @@
(setting/set-value-of-type! :json :ldap-group-mappings new-value)))))
(defsetting ldap-configured?
- "Check if LDAP is enabled and that the mandatory settings are configured."
+ (deferred-tru "Have the mandatory LDAP settings (host and user search base) been validated and saved?")
:type :boolean
:visibility :public
:setter :none
- :getter (fn [] (boolean (and (ldap-enabled)
- (ldap-host)
+ :getter (fn [] (boolean (and (ldap-host)
(ldap-user-base)))))
+(def mb-settings->ldap-details
+ "Mappings from Metabase setting names to keys to use for LDAP connections"
+ {:ldap-host :host
+ :ldap-port :port
+ :ldap-bind-dn :bind-dn
+ :ldap-password :password
+ :ldap-security :security
+ :ldap-user-base :user-base
+ :ldap-user-filter :user-filter
+ :ldap-attribute-email :attribute-email
+ :ldap-attribute-firstname :attribute-firstname
+ :ldap-attribute-lastname :attribute-lastname
+ :ldap-group-sync :group-sync
+ :ldap-group-base :group-base})
+
(defn- details->ldap-options [{:keys [host port bind-dn password security]}]
(let [security (keyword security)
port (if (string? port)
@@ -180,6 +189,13 @@
(catch Exception e
{:status :ERROR, :message (.getMessage e)})))
+(defn test-current-ldap-details
+ "Tests the connection to an LDAP server using the currently set settings."
+ []
+ (let [settings (into {} (for [[k v] mb-settings->ldap-details]
+ [v (setting/get k)]))]
+ (test-ldap-connection settings)))
+
(defn verify-password
"Verifies if the supplied password is valid for the `user-info` (from `find-user`) or DN."
([user-info password]
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index 0e03c59e35b1115d710e8d132e80aedbdcfb820b..a14aa6669cc5ffa5d61f0d3376685026a5e8af13 100644
--- a/src/metabase/models/user.clj
+++ b/src/metabase/models/user.clj
@@ -244,7 +244,7 @@
(defn- send-welcome-email! [new-user invitor sent-from-setup?]
(let [reset-token (set-password-reset-token! (u/the-id new-user))
- should-link-to-login-page (and (public-settings/sso-configured?)
+ should-link-to-login-page (and (public-settings/sso-enabled?)
(not (public-settings/enable-password-login)))
join-url (if should-link-to-login-page
(str (public-settings/site-url) "/auth/login")
diff --git a/src/metabase/public_settings.clj b/src/metabase/public_settings.clj
index b2cec212dfa90b548821cbae1fdb3214230c41b6..e6a2f187387dd4d7d96deb1da75c82fa723414eb 100644
--- a/src/metabase/public_settings.clj
+++ b/src/metabase/public_settings.clj
@@ -22,9 +22,9 @@
(defn- google-auth-configured? []
(boolean (setting/get :google-auth-client-id)))
-(defn- ldap-configured? []
- (classloader/require 'metabase.integrations.ldap)
- ((resolve 'metabase.integrations.ldap/ldap-configured?)))
+(defn- ldap-enabled? []
+ (classloader/require 'metabase.api.ldap)
+ ((resolve 'metabase.api.ldap/ldap-enabled)))
(defn- ee-sso-configured? []
(u/ignore-exceptions
@@ -32,11 +32,11 @@
(when-let [varr (resolve 'metabase-enterprise.sso.integrations.sso-settings/other-sso-configured?)]
(varr)))
-(defn sso-configured?
- "Any SSO provider is configured"
+(defn sso-enabled?
+ "Any SSO provider is configured and enabled"
[]
(or (google-auth-configured?)
- (ldap-configured?)
+ (ldap-enabled?)
(ee-sso-configured?)))
(defsetting check-for-updates
@@ -364,7 +364,7 @@
;; otherwise this always returns true.
(let [v (setting/get-value-of-type :boolean :enable-password-login)]
(if (and (some? v)
- (sso-configured?))
+ (sso-enabled?))
v
true))))
diff --git a/test/metabase/api/ldap_test.clj b/test/metabase/api/ldap_test.clj
index cb8501104c17ea9f4b29eb95d16bbf94675ad7b4..da424cac0da04732aabe115c2d0a52485d00ec1d 100644
--- a/test/metabase/api/ldap_test.clj
+++ b/test/metabase/api/ldap_test.clj
@@ -8,10 +8,11 @@
[metabase.test.integrations.ldap :as ldap.test]))
(defn ldap-test-details
- []
- (-> (ldap.test/get-ldap-details)
- (set/rename-keys (set/map-invert @#'api.ldap/mb-settings->ldap-details))
- (assoc :ldap-enabled true)))
+ ([] (ldap-test-details true))
+ ([enabled?]
+ (-> (ldap.test/get-ldap-details)
+ (set/rename-keys (set/map-invert @#'ldap/mb-settings->ldap-details))
+ (assoc :ldap-enabled enabled?))))
(deftest ldap-settings-test
(testing "PUT /api/ldap/settings"
@@ -23,19 +24,24 @@
(mt/user-http-request :crowberto :put 500 "ldap/settings"
(assoc (ldap-test-details) :ldap-password "wrong-password")))
- (testing "Invalid LDAP settings can be saved if LDAP is disabled"
- (mt/user-http-request :crowberto :put 200 "ldap/settings"
- (assoc (ldap-test-details) :ldap-password "wrong-password" :ldap-enabled false)))
-
(testing "Valid LDAP settings can still be saved if port is a integer (#18936)"
(mt/user-http-request :crowberto :put 200 "ldap/settings"
(assoc (ldap-test-details)
:ldap-port (Integer. (ldap.test/get-ldap-port)))))
- (testing "LDAP port is saved as default value if passed as an empty string (#18936)"
- (mt/user-http-request :crowberto :put 200 "ldap/settings"
- (assoc (ldap-test-details) :ldap-port "" :ldap-enabled false))
- (is (= 389 (ldap/ldap-port))))
+ (testing "Passing ldap-enabled=false will disable LDAP"
+ (mt/user-http-request :crowberto :put 200 "ldap/settings" (ldap-test-details false))
+ (is (not (api.ldap/ldap-enabled))))
+
+ (testing "Passing ldap-enabled=false still validates the LDAP settings"
+ (mt/user-http-request :crowberto :put 500 "ldap/settings"
+ (assoc (ldap-test-details false) :ldap-password "wrong-password")))
+
+ (with-redefs [ldap/test-ldap-connection (constantly {:status :SUCCESS})]
+ (testing "LDAP port is saved as default value if passed as an empty string (#18936)"
+ (mt/user-http-request :crowberto :put 200 "ldap/settings"
+ (assoc (ldap-test-details) :ldap-port ""))
+ (is (= 389 (ldap/ldap-port)))))
(testing "Could update with obfuscated password"
(mt/user-http-request :crowberto :put 200 "ldap/settings"
@@ -45,3 +51,18 @@
(is (= "You don't have permissions to do that."
(mt/user-http-request :rasta :put 403 "ldap/settings"
(assoc (ldap-test-details) :ldap-port "" :ldap-enabled false))))))))
+
+(deftest ldap-enabled-test
+ (ldap.test/with-ldap-server
+ (testing "`ldap-enabled` setting validates currently saved LDAP settings"
+ (mt/with-temporary-setting-values [ldap-enabled false]
+ (with-redefs [ldap/test-current-ldap-details (constantly {:status :ERROR :message "test error"})]
+ (is (thrown-with-msg? clojure.lang.ExceptionInfo
+ #"Unable to connect to LDAP server"
+ (api.ldap/ldap-enabled! true))))
+ (with-redefs [ldap/test-current-ldap-details (constantly {:status :SUCCESS})]
+ (api.ldap/ldap-enabled! true)
+ (is (api.ldap/ldap-enabled))
+
+ (api.ldap/ldap-enabled! false)
+ (is (not (api.ldap/ldap-enabled))))))))
diff --git a/test/metabase/test/integrations/ldap.clj b/test/metabase/test/integrations/ldap.clj
index 0c3f863895f7578765a0303f388c5db50ea115d3..bb56815c84027425d888b94cdab240419f53e498 100644
--- a/test/metabase/test/integrations/ldap.clj
+++ b/test/metabase/test/integrations/ldap.clj
@@ -54,15 +54,15 @@
[f options]
(binding [*ldap-server* (start-ldap-server! options)]
(try
- (tu/with-temporary-setting-values [ldap-enabled true
- ldap-host "localhost"
+ (tu/with-temporary-setting-values [ldap-host "localhost"
ldap-port (get-ldap-port)
ldap-bind-dn "cn=Directory Manager"
ldap-password "password"
ldap-user-base "dc=metabase,dc=com"
ldap-group-sync true
ldap-group-base "dc=metabase,dc=com"]
- (f))
+ (tu/with-temporary-raw-setting-values [ldap-enabled "true"]
+ (f)))
(finally (.shutDown *ldap-server* true)))))
(defmacro with-ldap-server