From f1606cb2bdbb7fe69af9a0baf9d979691df46f4a Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Fri, 3 Dec 2021 10:37:35 -0500
Subject: [PATCH] Fix LDAP port validation and add LDAP API tests (#19185)
---
src/metabase/api/ldap.clj | 9 ++++---
test/metabase/api/ldap_test.clj | 34 ++++++++++++++++++++++++
test/metabase/integrations/ldap_test.clj | 25 ++++++-----------
test/metabase/test/integrations/ldap.clj | 9 +++++++
4 files changed, 56 insertions(+), 21 deletions(-)
create mode 100644 test/metabase/api/ldap_test.clj
diff --git a/src/metabase/api/ldap.clj b/src/metabase/api/ldap.clj
index e26175a04d6..05ddb388859 100644
--- a/src/metabase/api/ldap.clj
+++ b/src/metabase/api/ldap.clj
@@ -90,10 +90,11 @@
[:as {settings :body}]
{settings su/Map}
(api/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 (when-let [^String ldap-port (not-empty (:ldap-port settings))]
- (Long/parseLong ldap-port))))
+ (let [ldap-settings (-> settings
+ (select-keys (keys mb-settings->ldap-details))
+ (assoc :ldap-port (when-let [^String ldap-port (not-empty (str (:ldap-port settings)))]
+ (Long/parseLong ldap-port))))
+ 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}
diff --git a/test/metabase/api/ldap_test.clj b/test/metabase/api/ldap_test.clj
new file mode 100644
index 00000000000..157743a3059
--- /dev/null
+++ b/test/metabase/api/ldap_test.clj
@@ -0,0 +1,34 @@
+(ns metabase.api.ldap-test
+ (:require [clojure.set :as set]
+ [clojure.test :refer :all]
+ [metabase.api.ldap :as ldap-api]
+ [metabase.integrations.ldap :as ldap]
+ [metabase.test :as mt]
+ [metabase.test.integrations.ldap :as ldap.test]))
+
+(deftest ldap-settings-test
+ (testing "PUT /api/ldap/settings"
+ (ldap.test/with-ldap-server
+ (let [ldap-test-details (-> (ldap.test/get-ldap-details)
+ (set/rename-keys (set/map-invert @#'ldap-api/mb-settings->ldap-details))
+ (assoc :ldap-enabled true))]
+ (testing "Valid LDAP settings can be saved via an API call"
+ (mt/user-http-request :crowberto :put 200 "ldap/settings" ldap-test-details))
+
+ (testing "Invalid LDAP settings return a server error"
+ (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))))))))
diff --git a/test/metabase/integrations/ldap_test.clj b/test/metabase/integrations/ldap_test.clj
index b4150106aab..6e1206a165d 100644
--- a/test/metabase/integrations/ldap_test.clj
+++ b/test/metabase/integrations/ldap_test.clj
@@ -9,15 +9,6 @@
[toucan.db :as db])
(:import com.unboundid.ldap.sdk.LDAPConnectionPool))
-(defn- get-ldap-details []
- {:host "localhost"
- :port (ldap.test/get-ldap-port)
- :bind-dn "cn=Directory Manager"
- :password "password"
- :security :none
- :user-base "dc=metabase,dc=com"
- :group-base "dc=metabase,dc=com"})
-
;; See test_resources/ldap.ldif for fixtures
;; The connection test should pass with valid settings
@@ -26,24 +17,24 @@
(testing "anonymous binds"
(testing "successfully connect to IPv4 host"
(is (= {:status :SUCCESS}
- (ldap/test-ldap-connection (get-ldap-details))))))
+ (ldap/test-ldap-connection (ldap.test/get-ldap-details))))))
(testing "invalid user search base"
(is (= :ERROR
- (:status (ldap/test-ldap-connection (assoc (get-ldap-details)
+ (:status (ldap/test-ldap-connection (assoc (ldap.test/get-ldap-details)
:user-base "dc=example,dc=com"))))))
(testing "invalid group search base"
(is (= :ERROR
- (:status (ldap/test-ldap-connection (assoc (get-ldap-details) :group-base "dc=example,dc=com"))))))
+ (:status (ldap/test-ldap-connection (assoc (ldap.test/get-ldap-details) :group-base "dc=example,dc=com"))))))
(testing "invalid bind DN"
(is (= :ERROR
- (:status (ldap/test-ldap-connection (assoc (get-ldap-details) :bind-dn "cn=Not Directory Manager"))))))
+ (:status (ldap/test-ldap-connection (assoc (ldap.test/get-ldap-details) :bind-dn "cn=Not Directory Manager"))))))
(testing "invalid bind password"
(is (= :ERROR
- (:status (ldap/test-ldap-connection (assoc (get-ldap-details) :password "wrong"))))))
+ (:status (ldap/test-ldap-connection (assoc (ldap.test/get-ldap-details) :password "wrong"))))))
(testing "basic get-connection works, will throw otherwise"
(is (= nil
@@ -107,7 +98,7 @@
:last-name "Miller"
:email "jane.miller@metabase.com"
:groups []}
- (ldap/find-user "jane.miller@metabase.com")))
+ (ldap/find-user "jane.miller@metabase.com")))))
;; Test group lookups for directory servers that use the memberOf attribute overlay, such as Active Directory
(ldap.test/with-active-directory-ldap-server
@@ -126,7 +117,7 @@
:email "sally.brown@metabase.com"
:groups ["cn=Accounting,ou=Groups,dc=metabase,dc=com",
"cn=Engineering,ou=Groups,dc=metabase,dc=com"]}
- (ldap/find-user "sbrown20")))))))))
+ (ldap/find-user "sbrown20")))))))
(deftest fetch-or-create-user-test
;; there are EE-specific versions of this test in `metabase-enterprise.enhancements.integrations.ldap-test`
@@ -181,7 +172,7 @@
(deftest ipv6-test
(testing "successfully connect to IPv6 host"
(let [actual (ldap.test/with-ldap-server
- (ldap/test-ldap-connection (assoc (get-ldap-details)
+ (ldap/test-ldap-connection (assoc (ldap.test/get-ldap-details)
:host "[::1]")))]
(if (= (:status actual) :ERROR)
(is (re-matches #"An error occurred while attempting to connect to server \[::1].*" (:message actual)))
diff --git a/test/metabase/test/integrations/ldap.clj b/test/metabase/test/integrations/ldap.clj
index b361dc8c0b2..0c3f863895f 100644
--- a/test/metabase/test/integrations/ldap.clj
+++ b/test/metabase/test/integrations/ldap.clj
@@ -33,6 +33,15 @@
[]
(.getListenPort *ldap-server*))
+(defn get-ldap-details []
+ {:host "localhost"
+ :port (get-ldap-port)
+ :bind-dn "cn=Directory Manager"
+ :password "password"
+ :security :none
+ :user-base "dc=metabase,dc=com"
+ :group-base "dc=metabase,dc=com"})
+
(defn get-default-schema
"Get the default schema for the directory server."
[]
--
GitLab