Fix broken LDAP group lookups for Active Directory (#16331)

* adapt ldap test helpers to be able to test AD memberOf attribute

* fix ldap search to handle memberOf attributes with a single group

* use one-or-many instead of if statement

* fix tests

src/metabase/integrations/ldap/default_implementation.clj

@@ -67,7 +67,7 @@
        :groups     (when sync-groups?
                      ;; Active Directory and others (like FreeIPA) will supply a `memberOf` overlay attribute for
                      ;; groups. Otherwise we have to make the inverse query to get them.
-                     (or (:memberof result)
+                     (or (u/one-or-many (:memberof result))
                          (user-groups ldap-connection dn settings)
                          []))})))
 

test/metabase/integrations/ldap_test.clj

@@ -96,7 +96,26 @@
                 :last-name  "Taylor"
                 :email      "fred.taylor@metabase.com"
                 :groups     []}
-               (ldap/find-user "fred.taylor@metabase.com")))))))
+               (ldap/find-user "fred.taylor@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
+      (testing "find user with one group using memberOf attribute"
+        (is (= {: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"))))
+
+      (testing "find user with two groups using memberOf attribute"
+        (is (= {:dn         "cn=Sally Brown,ou=People,dc=metabase,dc=com"
+                :first-name "Sally"
+                :last-name  "Brown"
+                :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")))))))
 
 (deftest group-matching-test
   (testing "LDAP group matching should identify Metabase groups using DN equality rules"

test/metabase/test/integrations/ldap.clj

@@ -10,18 +10,19 @@
   "An in-memory LDAP testing server."
   nil)
 
-(defn- get-server-config []
+(defn- get-server-config
+  [schema]
   (doto (InMemoryDirectoryServerConfig. (into-array String ["dc=metabase,dc=com"]))
     (.addAdditionalBindCredentials "cn=Directory Manager" "password")
-    (.setSchema (Schema/getDefaultStandardSchema))
+    (.setSchema schema)
     (.setListenerConfigs (into-array InMemoryListenerConfig [(InMemoryListenerConfig/createLDAPConfig "LDAP" 0)]))))
 
-(defn- start-ldap-server! []
-  (with-open [ldif (LDIFReader. (or (io/file (io/resource "ldap.ldif"))
-                                    (io/file "test_resources/ldap.ldif")
+(defn- start-ldap-server!
+  [{:keys [ldif-resource schema]}]
+  (with-open [ldif (LDIFReader. (or (io/file (str "test_resources/" ldif-resource))
                                     (throw
-                                     (FileNotFoundException. "ldap.ldif does not exist!"))))]
-    (doto (InMemoryDirectoryServer. (get-server-config))
+                                     (FileNotFoundException. (str ldif-resource " does not exist!")))))]
+    (doto (InMemoryDirectoryServer. (get-server-config schema))
       (.importFromLDIF true ldif)
       (.startListening))))
 
@@ -30,10 +31,15 @@
   []
   (.getListenPort *ldap-server*))
 
+(defn get-default-schema
+  "Get the default schema for the directory server."
+  []
+  (Schema/getDefaultStandardSchema))
+
 (defn do-with-ldap-server
   "Bind `*ldap-server*` and the relevant settings to an in-memory LDAP testing server and executes `f`."
-  [f]
-  (binding [*ldap-server* (start-ldap-server!)]
+  [f options]
+  (binding [*ldap-server* (start-ldap-server! options)]
     (try
       (tu/with-temporary-setting-values [ldap-enabled    true
                                          ldap-host       "localhost"
@@ -49,4 +55,14 @@
 (defmacro with-ldap-server
   "Bind `*ldap-server*` and the relevant settings to an in-memory LDAP testing server and executes `body`."
   [& body]
-  `(do-with-ldap-server (fn [] ~@body)))
+  `(do-with-ldap-server (fn [] ~@body)
+                        {:ldif-resource "ldap.ldif"
+                         :schema        (get-default-schema)}))
+
+(defmacro with-active-directory-ldap-server
+  "Bind `*ldap-server*` and the relevant settings to an in-memory LDAP testing server and executes `body`.
+  This version of the macro uses options that simulate an Active Directory server with memberOf attributes."
+  [& body]
+  `(do-with-ldap-server (fn [] ~@body)
+                        {:ldif-resource "active_directory.ldif"
+                         :schema        nil}))

test_resources/active_directory.ldif

+dn: dc=metabase,dc=com
+objectClass: top
+objectClass: organization
+objectClass: dcObject
+dc: metabase
+o: Metabase Corporation
+
+dn: ou=People,dc=metabase,dc=com
+objectClass: top
+objectClass: organizationalUnit
+ou: People
+
+dn: cn=John Smith,ou=People,dc=metabase,dc=com
+objectClass: top
+objectClass: person
+objectClass: organizationalPerson
+objectClass: inetOrgPerson
+cn: John Smith
+sn: Smith
+givenName: John
+uid: jsmith1
+mail: John.Smith@metabase.com
+userPassword: strongpassword
+memberOf: cn=Accounting,ou=Groups,dc=metabase,dc=com
+
+dn: cn=Sally Brown,ou=People,dc=metabase,dc=com
+objectClass: top
+objectClass: person
+objectClass: organizationalPerson
+objectClass: inetOrgPerson
+cn: Sally Brown
+sn: Brown
+givenName: Sally
+uid: sbrown20
+mail: sally.brown@metabase.com
+userPassword: 1234
+memberOf: cn=Accounting,ou=Groups,dc=metabase,dc=com
+memberOf: cn=Engineering,ou=Groups,dc=metabase,dc=com
+
+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=Sally Brown,ou=People,dc=metabase,dc=com
+
+dn: cn=Engineering,ou=Groups,dc=metabase,dc=com
+objectClass: top
+objectClass: groupOfNames
+cn: Accounting
+member: cn=Sally Brown,ou=People,dc=metabase,dc=com