From 7015c8eae96883bb40e9fa112453ad16f6f32554 Mon Sep 17 00:00:00 2001
From: William Turner <william.turner@aero.bombardier.com>
Date: Wed, 29 Mar 2017 14:25:11 -0400
Subject: [PATCH] Adds settings validation and connection testing
---
src/metabase/api/ldap.clj | 74 +++++++++++++++++++++++-
src/metabase/api/session.clj | 2 +-
src/metabase/integrations/ldap.clj | 65 ++++++++++++++++-----
test/metabase/integrations/ldap_test.clj | 39 +++++++++++--
test/metabase/test/integrations/ldap.clj | 7 +--
5 files changed, 158 insertions(+), 29 deletions(-)
diff --git a/src/metabase/api/ldap.clj b/src/metabase/api/ldap.clj
index 73e109aa279..3e15fa26846 100644
--- a/src/metabase/api/ldap.clj
+++ b/src/metabase/api/ldap.clj
@@ -1,16 +1,84 @@
(ns metabase.api.ldap
"/api/ldap endpoints"
- (:require [compojure.core :refer [PUT]]
+ (:require [clojure.tools.logging :as log]
+ [clojure.set :as set]
+ [compojure.core :refer [PUT]]
[metabase.api.common :refer :all]
+ [metabase.config :as config]
+ [metabase.integrations.ldap :as ldap]
[metabase.models.setting :as setting]
[metabase.util.schema :as su]))
+(def ^:private ^:const 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-base :base
+ :ldap-user-filter :user-filter
+ :ldap-attribute-email :attribute-email
+ :ldap-attribute-firstname :attribute-firstname
+ :ldap-attribute-lastname :attribute-lastname})
+
+(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"
+ :ldap-port "Wrong host or port"}}
+ security-error {:errors {:ldap-port "Wrong port or security setting"
+ :ldap-security "Wrong port or security setting"}}
+ creds-error {:errors {:ldap-bind-dn "Wrong bind DN or password"
+ :ldap-password "Wrong bind DN or password"}}
+ base-error {:errors {:ldap-base "Search base does not exist or is unreadable"}}]
+ (condp re-matches message
+ #".*UnknownHostException.*"
+ conn-error
+
+ #".*ConnectException.*"
+ conn-error
+
+ #".*SocketException.*"
+ security-error
+
+ #"^80090308:.*"
+ creds-error
+
+ #"^Unable to bind as user .*"
+ creds-error
+
+ #"(?s)^0000202B:.*"
+ base-error
+
+ #"^Search base does not exist .*"
+ base-error
+
+ ;; everything else :(
+ #"(?s).*"
+ {:message "Sorry, something went wrong. Please try again."}))))
+
(defendpoint PUT "/settings"
"Update LDAP related settings. You must be a superuser to do this."
[:as {settings :body}]
{settings su/Map}
(check-superuser)
- (setting/set-many! settings)
- {:ok true})
+ (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))))
+ results (if (or config/is-test? (not (:ldap-enabled settings)))
+ ;; for unit testing or disabled status just respond with a success message
+ {:status :SUCCESS}
+ ;; in normal conditions, validate connection
+ (ldap/test-ldap-connection ldap-details))]
+ (if (= :SUCCESS (:status results))
+ ;; test was good, save our settings
+ (setting/set-many! ldap-settings)
+ ;; test failed, return result message
+ {:status 500
+ :body (humanize-error-messages results)})))
(define-routes)
diff --git a/src/metabase/api/session.clj b/src/metabase/api/session.clj
index 6513b5a1aa5..3fa8592d55f 100644
--- a/src/metabase/api/session.clj
+++ b/src/metabase/api/session.clj
@@ -64,7 +64,7 @@
(throw (ex-info "Password did not match stored password." {:status-code 400
:errors {:password "did not match stored password"}}))))
(catch com.unboundid.util.LDAPSDKException e
- (log/error (u/format-color 'red "Unexpected LDAP error, will fallback to local authentication") (.getMessage e)))))
+ (log/error (u/format-color 'red "Problem connecting to LDAP server, will fallback to local authentication") (.getMessage e)))))
;; Then try local authentication
(when-let [user (db/select-one [User :id :password_salt :password :last_login], :email username, :is_active true)]
diff --git a/src/metabase/integrations/ldap.clj b/src/metabase/integrations/ldap.clj
index 8d716b18792..9b210bcafaf 100644
--- a/src/metabase/integrations/ldap.clj
+++ b/src/metabase/integrations/ldap.clj
@@ -9,7 +9,7 @@
:default false)
(defsetting ldap-host
- "LDAP server hostname.")
+ "Server hostname.")
(defsetting ldap-port
"Server port, usually 389 or 636 if SSL is used."
@@ -57,19 +57,49 @@
(boolean (ldap-password))
(boolean (ldap-base))))
+(defn- details->ldap-options [{:keys [host port bind-dn password security]}]
+ {:host (str host ":" port)
+ :bind-dn bind-dn
+ :password password
+ :ssl? (= security "ssl")
+ :startTLS? (= security "starttls")})
+
+(defn- settings->ldap-options []
+ (details->ldap-options {:host (ldap-host)
+ :port (ldap-port)
+ :bind-dn (ldap-bind-dn)
+ :password (ldap-password)
+ :security (ldap-security)}))
+
+(defn test-ldap-connection
+ "Test the connection to an LDAP server to determine if we can find the search base.
+
+ Takes in a dictionary of properties such as:
+ {:host \"localhost\"
+ :port 389
+ :bind-dn \"cn=Directory Manager\"
+ :password \"password\"
+ :security \"none\"
+ :base \"ou=people,dc=metabase,dc=com\"}"
+ [{:keys [base], :as details}]
+ (try
+ (with-open [conn (ldap/connect (details->ldap-options details))]
+ (if-let [_ (ldap/get conn base)]
+ {:status :SUCCESS}
+ {:status :ERROR
+ :message "Search base does not exist or is unreadable"}))
+ (catch com.unboundid.util.LDAPSDKException e
+ {:status :ERROR
+ :message (.getMessage e)})))
+
(defn- get-ldap-connection []
- (ldap/connect {:host (str (ldap-host) ":" (ldap-port))
- :bind-dn (ldap-bind-dn)
- :password (ldap-password)
- :ssl? (= (ldap-security) "ssl")
- :startTLS? (= (ldap-security) "starttls")}))
+ "Connects to LDAP with the currently set settings and returns the connection."
+ (ldap/connect (settings->ldap-options)))
(defn- with-connection [f & args]
"Applies `f` with a connection pool followed by `args`"
- (let [conn (get-ldap-connection)]
- (try
- (apply f conn args)
- (finally (ldap/close conn)))))
+ (with-open [conn (get-ldap-connection)]
+ (apply f conn args)))
(defn- escape-value [value]
"Escapes a value for use in an LDAP filter expression."
@@ -87,11 +117,16 @@
:filter (s/replace (ldap-user-filter) "{login}" (escape-value username))
:attributes [:dn :distinguishedName :membderOf fname-attr lname-attr email-attr]
:size-limit 1})]
- {:dn (or (:dn result) (:distinguishedName result))
- :first-name (get result fname-attr)
- :last-name (get result lname-attr)
- :email (get result email-attr)
- :groups (or (:membderOf result) [])}))))
+ (let [dn (or (:dn result) (:distinguishedName result))
+ fname (get result fname-attr)
+ lname (get result lname-attr)
+ email (get result email-attr)]
+ (when-not (or (empty? dn) (empty? fname) (empty? lname) (empty? email))
+ {:dn dn
+ :first-name fname
+ :last-name lname
+ :email email
+ :groups (or (:membderOf result) [])}))))))
(defn verify-password
"Verifies if the password supplied is valid for the supplied `user-info` (from `find-user`) or DN."
diff --git a/test/metabase/integrations/ldap_test.clj b/test/metabase/integrations/ldap_test.clj
index 6587d0f8ca3..5b4090cd888 100644
--- a/test/metabase/integrations/ldap_test.clj
+++ b/test/metabase/integrations/ldap_test.clj
@@ -2,17 +2,48 @@
(:require [expectations :refer :all]
[metabase.integrations.ldap :as ldap]
(metabase.test [util :refer [resolve-private-vars]])
- (metabase.test.integrations [ldap :refer [expect-with-ldap-server]])))
+ (metabase.test.integrations [ldap :refer [expect-with-ldap-server get-ldap-port]])))
-(resolve-private-vars metabase.integrations.ldap escape-value get-ldap-connection)
+(resolve-private-vars metabase.integrations.ldap escape-value settings->ldap-options get-ldap-connection)
+(defn- get-ldap-details []
+ {:host "localhost"
+ :port (get-ldap-port)
+ :bind-dn "cn=Directory Manager"
+ :password "password"
+ :security "none"
+ :base "dc=metabase,dc=com"})
+
;; See test_resources/ldap.ldif for fixtures
(expect
"\\2AJohn \\28Dude\\29 Doe\\5C"
(escape-value "*John (Dude) Doe\\"))
+;; The connection test should pass with valid settings
+(expect-with-ldap-server
+ {:status :SUCCESS}
+ (ldap/test-ldap-connection (get-ldap-details)))
+
+;; The connection test should fail with an invalid search base
+(expect-with-ldap-server
+ {:status :ERROR
+ :message "Search base does not exist or is unreadable"}
+ (ldap/test-ldap-connection (assoc (get-ldap-details) :base "dc=example,dc=com")))
+
+;; The connection test should fail with an invalid bind DN
+(expect-with-ldap-server
+ {:status :ERROR
+ :message "Unable to bind as user 'cn=Not Directory Manager' because no such entry exists in the server."}
+ (ldap/test-ldap-connection (assoc (get-ldap-details) :bind-dn "cn=Not Directory Manager")))
+
+;; The connection test should fail with an invalid bind password
+(expect-with-ldap-server
+ {:status :ERROR
+ :message "Unable to bind as user 'cn=Directory Manager' because the provided password was incorrect."}
+ (ldap/test-ldap-connection (assoc (get-ldap-details) :password "wrong")))
+
;; Make sure the basic connection stuff works, this will throw otherwise
(expect-with-ldap-server
nil
@@ -31,7 +62,7 @@
;; Login with invalid DN should fail
(expect-with-ldap-server
false
- (ldap/verify-password "cn=Nobody,ou=people,dc=metabase,dc=com" "password"))
+ (ldap/verify-password "cn=Nobody,ou=nowhere,dc=metabase,dc=com" "password"))
;; Login for regular users should also work
(expect-with-ldap-server
@@ -52,7 +83,7 @@
:groups []}
(ldap/find-user "sbrown20"))
-;; Find by email should also work (also given our test setup)
+;; Find by email should also work (also given our default settings and fixtures)
(expect-with-ldap-server
{:dn "cn=Sally Brown,ou=people,dc=metabase,dc=com"
:first-name "Sally"
diff --git a/test/metabase/test/integrations/ldap.clj b/test/metabase/test/integrations/ldap.clj
index b92e1bc8397..bd11be205fc 100644
--- a/test/metabase/test/integrations/ldap.clj
+++ b/test/metabase/test/integrations/ldap.clj
@@ -32,11 +32,6 @@
[]
(.getListenPort *ldap-server*))
-(defn get-ldap-base
- "Get the base DN for the bound in-memory LDAP testing server."
- []
- (.toNormalizedString (first (.getBaseDNs *ldap-server*))))
-
(defn do-with-ldap-server
"Bind `*ldap-server*` and the relevant settings to an in-memory LDAP testing server and executes `f`."
[f]
@@ -47,7 +42,7 @@
ldap-port (str (get-ldap-port))
ldap-bind-dn "cn=Directory Manager"
ldap-password "password"
- ldap-base (get-ldap-base)]
+ ldap-base "dc=metabase,dc=com"]
(f))
(finally (.shutDown *ldap-server* true)))))
--
GitLab