diff --git a/src/metabase/api/session.clj b/src/metabase/api/session.clj
index 3fa8592d55fcf88548935bfd02d1a8265b0424b5..49cb9fe3567880b16461acfde6565c721e36d9eb 100644
--- a/src/metabase/api/session.clj
+++ b/src/metabase/api/session.clj
@@ -33,7 +33,7 @@
:user_id (:id user))
(events/publish-event! :user-login {:user_id (:id user), :session_id <>, :first_login (not (boolean (:last_login user)))})))
-(defn- ldap-auth-fetch-or-create-user! [first-name last-name email password]
+(defn- ldap-auth-fetch-or-create-user! [first-name last-name email password groups]
(when-let [user (or (db/select-one [User :id :last_login] :email email)
(user/create-new-ldap-auth-user! first-name last-name email password))]
(u/prog1 {:id (create-session! user)}
@@ -57,9 +57,9 @@
;; First try LDAP if it's enabled
(when (ldap/ldap-configured?)
(try
- (when-let [{:keys [first-name last-name email], :as user-info} (ldap/find-user username)]
+ (when-let [{:keys [first-name last-name email groups], :as user-info} (ldap/find-user username)]
(if (ldap/verify-password user-info password)
- (ldap-auth-fetch-or-create-user! first-name last-name email password)
+ (ldap-auth-fetch-or-create-user! first-name last-name email password groups)
;; Since LDAP knows about our user, fail here to prevent the local strategy to be tried with an outdated password
(throw (ex-info "Password did not match stored password." {:status-code 400
:errors {:password "did not match stored password"}}))))
diff --git a/src/metabase/integrations/ldap.clj b/src/metabase/integrations/ldap.clj
index 9b210bcafaf7c9c59502f92fa6c4430cba53ac1a..1d4978e23dfe8a1c2ac425c348b81faab9ffbb2d 100644
--- a/src/metabase/integrations/ldap.clj
+++ b/src/metabase/integrations/ldap.clj
@@ -71,6 +71,31 @@
:password (ldap-password)
:security (ldap-security)}))
+(defn- escape-value [value]
+ "Escapes a value for use in an LDAP filter expression."
+ (s/replace value #"[\*\(\)\\\\0]" (comp (partial format "\\%02X") int first)))
+
+(defn- get-connection []
+ "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 and `args`"
+ (with-open [conn (get-connection)]
+ (apply f conn args)))
+
+(defn- get-user-groups
+ "Retrieve groups for a supplied DN."
+ ([dn]
+ (with-connection get-user-groups dn))
+ ([conn dn]
+ (let [results (ldap/search conn (ldap-base) {:scope :sub
+ :filter (str "member=" (escape-value dn))
+ :attributes [:dn :distinguishedName]})]
+ (filter some?
+ (for [result results]
+ (or (:dn result) (:distinguishedName result)))))))
+
(defn test-ldap-connection
"Test the connection to an LDAP server to determine if we can find the search base.
@@ -88,23 +113,10 @@
{:status :SUCCESS}
{:status :ERROR
:message "Search base does not exist or is unreadable"}))
- (catch com.unboundid.util.LDAPSDKException e
+ (catch Exception e
{:status :ERROR
:message (.getMessage e)})))
-(defn- get-ldap-connection []
- "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`"
- (with-open [conn (get-ldap-connection)]
- (apply f conn args)))
-
-(defn- escape-value [value]
- "Escapes a value for use in an LDAP filter expression."
- (s/replace value #"[\*\(\)\\\\0]" (comp (partial format "\\%02X") int first)))
-
(defn find-user
"Gets user information for the supplied username."
([username]
@@ -115,18 +127,21 @@
email-attr (keyword (ldap-attribute-email))]
(when-let [[result] (ldap/search conn (ldap-base) {:scope :sub
:filter (s/replace (ldap-user-filter) "{login}" (escape-value username))
- :attributes [:dn :distinguishedName :membderOf fname-attr lname-attr email-attr]
+ :attributes [:dn :distinguishedName fname-attr lname-attr email-attr :memberOf]
:size-limit 1})]
(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) [])}))))))
+ ;; ActiveDirectory (and others?) will supply a `memberOf` overlay attribute for groups
+ ;; Otherwise we have to make the inverse query to get them
+ (let [groups (or (:memberOf result) (get-user-groups dn) [])]
+ {:dn dn
+ :first-name fname
+ :last-name lname
+ :email email
+ :groups groups})))))))
(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 5b4090cd888199a7829f0784451f7f82a1aef56d..58a1a83d7ec09816170bd21b96e6ec64f3d8b58a 100644
--- a/test/metabase/integrations/ldap_test.clj
+++ b/test/metabase/integrations/ldap_test.clj
@@ -4,7 +4,7 @@
(metabase.test [util :refer [resolve-private-vars]])
(metabase.test.integrations [ldap :refer [expect-with-ldap-server get-ldap-port]])))
-(resolve-private-vars metabase.integrations.ldap escape-value settings->ldap-options get-ldap-connection)
+(resolve-private-vars metabase.integrations.ldap escape-value settings->ldap-options get-connection get-user-groups)
(defn- get-ldap-details []
@@ -47,7 +47,7 @@
;; Make sure the basic connection stuff works, this will throw otherwise
(expect-with-ldap-server
nil
- (.close (get-ldap-connection)))
+ (.close (get-connection)))
;; Login with everything right should succeed
(expect-with-ldap-server
@@ -67,27 +67,27 @@
;; Login for regular users should also work
(expect-with-ldap-server
true
- (ldap/verify-password "cn=Sally Brown,ou=people,dc=metabase,dc=com" "1234"))
+ (ldap/verify-password "cn=Sally Brown,ou=People,dc=metabase,dc=com" "1234"))
;; Login for regular users should also fail for the wrong password
(expect-with-ldap-server
false
- (ldap/verify-password "cn=Sally Brown,ou=people,dc=metabase,dc=com" "password"))
+ (ldap/verify-password "cn=Sally Brown,ou=People,dc=metabase,dc=com" "password"))
;; Find by username should work (given the default LDAP filter and test fixtures)
(expect-with-ldap-server
- {:dn "cn=Sally Brown,ou=people,dc=metabase,dc=com"
- :first-name "Sally"
- :last-name "Brown"
- :email "sally.brown@metabase.com"
- :groups []}
- (ldap/find-user "sbrown20"))
+ {:dn "cn=John Smith,ou=People,dc=metabase,dc=com"
+ :first-name "John"
+ :last-name "Smith"
+ :email "John.Smith@metabase.com"
+ :groups ["cn=Accounting,ou=Groups,dc=metabase,dc=com"]}
+ (ldap/find-user "jsmith1"))
;; 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"
- :last-name "Brown"
- :email "sally.brown@metabase.com"
- :groups []}
- (ldap/find-user "sally.brown@metabase.com"))
+ {:dn "cn=John Smith,ou=People,dc=metabase,dc=com"
+ :first-name "John"
+ :last-name "Smith"
+ :email "John.Smith@metabase.com"
+ :groups ["cn=Accounting,ou=Groups,dc=metabase,dc=com"]}
+ (ldap/find-user "John.Smith@metabase.com"))
diff --git a/test_resources/ldap.ldif b/test_resources/ldap.ldif
index 7481a9df7fd6dcad7ea83db3929f75a3b358a9d9..9d86c6f417da87d19368da755fdc2b3f303445a0 100644
--- a/test_resources/ldap.ldif
+++ b/test_resources/ldap.ldif
@@ -5,12 +5,12 @@ objectClass: dcObject
dc: metabase
o: Metabase Corporation
-dn: ou=people,dc=metabase,dc=com
+dn: ou=People,dc=metabase,dc=com
objectClass: top
objectClass: organizationalUnit
-ou: people
+ou: People
-dn: cn=John Smith,ou=people,dc=metabase,dc=com
+dn: cn=John Smith,ou=People,dc=metabase,dc=com
objectClass: top
objectClass: person
objectClass: organizationalPerson
@@ -22,7 +22,7 @@ uid: jsmith1
mail: John.Smith@metabase.com
userPassword: strongpassword
-dn: cn=Sally Brown,ou=people,dc=metabase,dc=com
+dn: cn=Sally Brown,ou=People,dc=metabase,dc=com
objectClass: top
objectClass: person
objectClass: organizationalPerson
@@ -34,12 +34,12 @@ uid: sbrown20
mail: sally.brown@metabase.com
userPassword: 1234
-dn: ou=birds,dc=metabase,dc=com
+dn: ou=Birds,dc=metabase,dc=com
objectClass: top
objectClass: organizationalUnit
-ou: birds
+ou: Birds
-dn: cn=Rasta Toucan,ou=birds,dc=metabase,dc=com
+dn: cn=Rasta Toucan,ou=Birds,dc=metabase,dc=com
objectClass: top
objectClass: person
objectClass: organizationalPerson
@@ -51,7 +51,7 @@ uid: rasta
mail: rasta@metabase.com
userPassword: blueberries
-dn: cn=Lucky Pigeon,ou=birds,dc=metabase,dc=com
+dn: cn=Lucky Pigeon,ou=Birds,dc=metabase,dc=com
objectClass: top
objectClass: person
objectClass: organizationalPerson
@@ -62,3 +62,15 @@ sn: Pigeon
uid: lucky
mail: lucky@metabase.com
userPassword: notalmonds
+
+dn: ou=Groups,dc=metabase,dc=com
+objectClass: top
+objectClass: organizationalUnit
+ou: Groups
+
+dn: cn=Accounting,ou=Groups,dc=metabase,dc=com
+objectClass: top
+objectClass: groupOfNames
+cn: Accounting
+member: cn=John Smith,ou=People,dc=metabase,dc=com
+member: cn=Rasta Toucan,ou=Birds,dc=metabase,dc=com