Starts adding group fetching logic

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"}}))))

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."

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"))

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