Fix LDAP requires `uid` to do group sync (#22368)

* fix ldap requires uid to do group sync

* fix ee test

enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj

@@ -44,12 +44,11 @@
                 :first-name "Fred"
                 :last-name  "Taylor"
                 :email      "fred.taylor@metabase.com"
-                :attributes {:uid       "ftaylor300"
-                             :mail      "fred.taylor@metabase.com"
+                :attributes {:mail      "fred.taylor@metabase.com"
                              :cn        "Fred Taylor"
                              :givenname "Fred"
                              :sn        "Taylor"}
-                :groups     []}
+                :groups     ["cn=Accounting,ou=Groups,dc=metabase,dc=com"]}
                (ldap/find-user "fred.taylor@metabase.com"))))
 
       (testing "find by email, no givenName"

src/metabase/integrations/ldap/default_implementation.clj

@@ -49,7 +49,7 @@
   "Retrieve groups for a supplied DN."
   [ldap-connection         :- LDAPConnectionPool
    dn                      :- su/NonBlankString
-   uid                     :- su/NonBlankString
+   uid                     :- (s/maybe su/NonBlankString)
    {:keys [group-base]}    :- i/LDAPSettings
    group-membership-filter :- su/NonBlankString]
   (when group-base

test/metabase/api/session_test.clj

@@ -6,10 +6,8 @@
             [metabase.api.session :as api.session]
             [metabase.driver.h2 :as h2]
             [metabase.http-client :as client]
-            [metabase.models :refer [LoginHistory]]
-            [metabase.models.session :refer [Session]]
+            [metabase.models :refer [LoginHistory PermissionsGroup PermissionsGroupMembership Session User]]
             [metabase.models.setting :as setting]
-            [metabase.models.user :refer [User]]
             [metabase.public-settings :as public-settings]
             [metabase.server.middleware.session :as mw.session]
             [metabase.test :as mt]
@@ -401,15 +399,11 @@
 (deftest ldap-login-test
   (ldap.test/with-ldap-server
     (testing "Test that we can login with LDAP"
-      (let [user-id (mt/user->id :rasta)]
-        (try
-          ;; TODO -- it's not so nice to go around permanently deleting stuff like Sessions like this in tests. We
-          ;; should just create a temp User instead for this test
-          (db/simple-delete! Session :user_id user-id)
-          (is (schema= SessionResponse
-                       (mt/client :post 200 "session" (mt/user->credentials :rasta))))
-          (finally
-            (db/update! User user-id :login_attributes nil)))))
+      (mt/with-temp User [_ {:email    "ngoc@metabase.com"
+                             :password "securedpassword"}]
+        (is (schema= SessionResponse
+                     (mt/client :post 200 "session" {:username "ngoc@metabase.com"
+                                                     :password "securedpassword"})))))
 
     (testing "Test that login will fallback to local for users not in LDAP"
       (mt/with-temporary-setting-values [enable-password-login true]
@@ -426,24 +420,20 @@
              (mt/client :post 401 "session" (mt/user->credentials :lucky)))))
 
     (testing "Test that a deactivated user cannot login with LDAP"
-      (let [user-id (mt/user->id :rasta)]
-        (try
-          (db/update! User user-id :is_active false)
-          (is (= {:errors {:_error "Your account is disabled."}}
-                 (mt/client :post 401 "session" (mt/user->credentials :rasta))))
-          (finally
-            (db/update! User user-id :is_active true)))))
+      (mt/with-temp User [_ {:email    "ngoc@metabase.com"
+                             :password "securedpassword"
+                             :is_active false}]
+        (is (= {:errors {:_error "Your account is disabled."}}
+               (mt/client :post 401 "session" {:username "ngoc@metabase.com"
+                                               :password "securedpassword"})))))
 
     (testing "Test that login will fallback to local for broken LDAP settings"
       (mt/with-temporary-setting-values [ldap-user-base "cn=wrong,cn=com"]
-        ;; delete all other sessions for the bird first, otherwise test doesn't seem to work (TODO - why?)
-        (let [user-id (mt/user->id :rasta)]
-          (try
-            (db/simple-delete! Session :user_id user-id)
+        (mt/with-temp User [_ {:email    "ngoc@metabase.com"
+                               :password "securedpassword"}]
             (is (schema= SessionResponse
-                         (mt/client :post 200 "session" (mt/user->credentials :rasta))))
-            (finally
-              (db/update! User user-id :login_attributes nil))))))
+                         (mt/client :post 200 "session" {:username "ngoc@metabase.com"
+                                                         :password "securedpassword"}))))))
 
     (testing "Test that we can login with LDAP with new user"
       (try
@@ -462,7 +452,16 @@
              SessionResponse
              (mt/client :post 200 "session" {:username "John.Smith@metabase.com", :password "strongpassword"})))
         (finally
-          (db/delete! User :email "John.Smith@metabase.com"))))))
+          (db/delete! User :email "John.Smith@metabase.com"))))
+
+    (testing "test that group sync works even if ldap doesn't return uid (#22014)"
+      (mt/with-temp PermissionsGroup [group {:name "Accounting"}]
+        (mt/with-temporary-raw-setting-values
+          [ldap-group-mappings (json/generate-string {"cn=Accounting,ou=Groups,dc=metabase,dc=com" [(:id group)]})]
+          (is (schema= SessionResponse
+                    (mt/client :post 200 "session" {:username "fred.taylor@metabase.com", :password "pa$$word"})))
+          (let [user-id (db/select-one-id User :email "fred.taylor@metabase.com")]
+            (is (= true (db/exists? PermissionsGroupMembership :group_id (:id group) (:user_id user-id))))))))))
 
 (deftest no-password-no-login-test
   (testing "A user with no password should not be able to do password-based login"

test/metabase/integrations/ldap_test.clj

@@ -89,7 +89,7 @@
                 :first-name "Fred"
                 :last-name  "Taylor"
                 :email      "fred.taylor@metabase.com"
-                :groups     []}
+                :groups     ["cn=Accounting,ou=Groups,dc=metabase,dc=com"]}
                (ldap/find-user "fred.taylor@metabase.com"))))
 
       (testing "find by email, no givenName"

test_resources/ldap.ldif

@@ -43,7 +43,7 @@ cn: Fred Taylor
 # intetionally having some attributes not properly cased to test case-insensitive when querying users
 sN: Taylor
 givenname: Fred
-uid: ftaylor300
+# no uid for this user to test login without uid
 mAiL: fred.taylor@metabase.com
 userpassword: pa$$word
 
@@ -99,6 +99,7 @@ objectClass: groupOfNames
 cn: Accounting
 member: cn=John Smith,ou=People,dc=metabase,dc=com
 member: cn=Rasta Toucan,ou=Birds,dc=metabase,dc=com
+member: cn=Fred Taylor,ou=People,dc=metabase,dc=com
 
 dn: cn=Engineering,ou=Groups,dc=metabase,dc=com
 objectClass: posixGroup