diff --git a/src/metabase/api/session.clj b/src/metabase/api/session.clj
index 91cb2d72edd0161ff967cb64924a206f917d60c6..6513b5a1aa5f8de66bc725ea8487eb077abe9626 100644
--- a/src/metabase/api/session.clj
+++ b/src/metabase/api/session.clj
@@ -33,10 +33,11 @@
:user_id (:id user))
(events/publish-event! :user-login {:user_id (:id user), :session_id <>, :first_login (not (boolean (:last_login user)))})))
-(defn- ldap-fetch-or-create-user! [first-name last-name email password]
- (if-let [user (or (db/select-one [User :id :last_login] :email email) ; TODO - Update the user's password
- (user/create-new-ldap-auth-user! first-name last-name email password))]
- {:id (create-session! user)}))
+(defn- ldap-auth-fetch-or-create-user! [first-name last-name email password]
+ (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)}
+ (user/set-password! (:id user) password))))
;;; ## API Endpoints
@@ -51,16 +52,19 @@
password su/NonBlankString}
(throttle/check (login-throttlers :ip-address) remote-address)
(throttle/check (login-throttlers :username) username)
+ ;; Primitive "strategy implementation", should be reworked for #3210
(or
;; First try LDAP if it's enabled
(when (ldap/ldap-configured?)
- (when-let [{:keys [first-name last-name email], :as user-info} (ldap/find-user username)]
- (println user-info)
- (if (ldap/verify-password user-info password)
- (ldap-fetch-or-create-user! first-name last-name email password)
- ;; Since LDAP knows about our user, fail fast here to prevent the local strategy to be tried with a potentially outdated password
- (throw (ex-info "Password did not match stored password." {:status-code 400
- :errors {:password "did not match stored password"}})))))
+ (try
+ (when-let [{:keys [first-name last-name email], :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)
+ ;; 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"}}))))
+ (catch com.unboundid.util.LDAPSDKException e
+ (log/error (u/format-color 'red "Unexpected LDAP error, 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/test/metabase/api/session_test.clj b/test/metabase/api/session_test.clj
index 09595a2b3fadae7f0f8ff49abbafc16cb173864b..50eb6602c56582a6fc388bbe479eca743320062a 100644
--- a/test/metabase/api/session_test.clj
+++ b/test/metabase/api/session_test.clj
@@ -11,8 +11,9 @@
[metabase.public-settings :as public-settings]
[metabase.test.data :refer :all]
[metabase.test.data.users :refer :all]
- [metabase.util :as u]
- [metabase.test.util :refer [random-name resolve-private-vars with-temporary-setting-values], :as tu]))
+ [metabase.test.integrations.ldap :refer [expect-with-ldap-server]]
+ [metabase.test.util :refer [random-name resolve-private-vars with-temporary-setting-values], :as tu]
+ [metabase.util :as u]))
;; ## POST /api/session
;; Test that we can login
@@ -249,3 +250,40 @@
admin-email "rasta@toucans.com"]
(u/prog1 (is-session? (google-auth-fetch-or-create-user! "Rasta" "Toucan" "rasta@sf-toucannery.com"))
(db/delete! User :email "rasta@sf-toucannery.com")))) ; clean up after ourselves
+
+
+;;; ------------------------------------------------------------ TESTS FOR LDAP AUTH STUFF ------------------------------------------------------------
+
+;; Test that we can login with LDAP
+(expect-with-ldap-server
+ true
+ ;; delete all other sessions for the bird first, otherwise test doesn't seem to work (TODO - why?)
+ (do (db/simple-delete! Session, :user_id (user->id :rasta))
+ (tu/is-uuid-string? (:id (client :post 200 "session" (user->credentials :rasta))))))
+
+;; Test that login will fallback to local for users not in LDAP
+(expect-with-ldap-server
+ true
+ ;; delete all other sessions for the bird first, otherwise test doesn't seem to work (TODO - why?)
+ (do (db/simple-delete! Session, :user_id (user->id :crowberto))
+ (tu/is-uuid-string? (:id (client :post 200 "session" (user->credentials :crowberto))))))
+
+;; Test that login will NOT fallback for users in LDAP but with an invalid password
+(expect-with-ldap-server
+ {:errors {:password "did not match stored password"}}
+ (client :post 400 "session" (user->credentials :lucky))) ; NOTE: there's a different password in LDAP for Lucky
+
+;; Test that login will fallback to local for broken LDAP settings
+;; NOTE: This will ERROR out in the logs, it's normal
+(expect-with-ldap-server
+ true
+ (tu/with-temporary-setting-values [ldap-base "cn=wrong,cn=com"]
+ ;; delete all other sessions for the bird first, otherwise test doesn't seem to work (TODO - why?)
+ (do (db/simple-delete! Session, :user_id (user->id :rasta))
+ (tu/is-uuid-string? (:id (client :post 200 "session" (user->credentials :rasta)))))))
+
+;; Test that we can login with LDAP with new user
+(expect-with-ldap-server
+ true
+ (u/prog1 (tu/is-uuid-string? (:id (client :post 200 "session" {:username "sbrown20", :password "1234"})))
+ (db/delete! User :email "sally.brown@metabase.com"))) ; clean up
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index a1c84a43466088a06129ff4510fbe311a029eb74..3bc33b6b87d6fa50d05146e1265a5247d43f5be1 100644
--- a/test/metabase/api/user_test.clj
+++ b/test/metabase/api/user_test.clj
@@ -31,7 +31,8 @@
:last_login $
:first_name "Crowberto"
:email "crowberto@metabase.com"
- :google_auth false})
+ :google_auth false
+ :ldap_auth false})
(match-$ (fetch-user :lucky)
{:common_name "Lucky Pigeon"
:last_name "Pigeon"
@@ -40,7 +41,8 @@
:last_login $
:first_name "Lucky"
:email "lucky@metabase.com"
- :google_auth false})
+ :google_auth false
+ :ldap_auth false})
(match-$ (fetch-user :rasta)
{:common_name "Rasta Toucan"
:last_name "Toucan"
@@ -49,7 +51,8 @@
:last_login $
:first_name "Rasta"
:email "rasta@metabase.com"
- :google_auth false})}
+ :google_auth false
+ :ldap_auth false})}
(do
;; Delete all the other random Users we've created so far
(let [user-ids (set (map user->id [:crowberto :rasta :lucky :trashbird]))]
@@ -131,6 +134,7 @@
:is_superuser false
:is_qbnewb true
:google_auth false
+ :ldap_auth false
:id $})
((user->client :rasta) :get 200 "user/current"))
diff --git a/test/metabase/integrations/ldap_test.clj b/test/metabase/integrations/ldap_test.clj
index 56e1f2768458c36acb4ee1fee89d6fb534140cdf..6587d0f8ca37331dcf1d01d8564ef75d0db81a40 100644
--- a/test/metabase/integrations/ldap_test.clj
+++ b/test/metabase/integrations/ldap_test.clj
@@ -13,51 +13,50 @@
"\\2AJohn \\28Dude\\29 Doe\\5C"
(escape-value "*John (Dude) Doe\\"))
+;; Make sure the basic connection stuff works, this will throw otherwise
(expect-with-ldap-server
- ;; Make sure the basic connection stuff works, this will throw otherwise
- ;; TODO: not sure if there's an "expect no throw"
nil
(.close (get-ldap-connection)))
+;; Login with everything right should succeed
(expect-with-ldap-server
- ;; Login with everything right should succeed
true
(ldap/verify-password "cn=Directory Manager" "password"))
+;; Login with wrong password should fail
(expect-with-ldap-server
- ;; Login with wrong password should fail
false
(ldap/verify-password "cn=Directory Manager" "wrongpassword"))
+;; Login with invalid DN should fail
(expect-with-ldap-server
- ;; Login with invalid DN should fail
false
(ldap/verify-password "cn=Nobody,ou=people,dc=metabase,dc=com" "password"))
+;; Login for regular users should also work
(expect-with-ldap-server
- ;; Login for regular users should also work
true
(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
- ;; Login for regular users should also fail for the wrong password
false
(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
- ;; Find by username should work (given the default LDAP filter and test fixtures)
{:dn "cn=Sally Brown,ou=people,dc=metabase,dc=com"
:first-name "Sally"
:last-name "Brown"
- :email "sally.brown@example.com"
+ :email "sally.brown@metabase.com"
:groups []}
(ldap/find-user "sbrown20"))
+;; Find by email should also work (also given our test setup)
(expect-with-ldap-server
- ;; Find by email should also work (also given our test setup)
{:dn "cn=Sally Brown,ou=people,dc=metabase,dc=com"
:first-name "Sally"
:last-name "Brown"
- :email "sally.brown@example.com"
+ :email "sally.brown@metabase.com"
:groups []}
- (ldap/find-user "sally.brown@example.com"))
+ (ldap/find-user "sally.brown@metabase.com"))
diff --git a/test_resources/ldap.ldif b/test_resources/ldap.ldif
index 75c16483d09979ddc29869508ffc1f0a22e05d4b..7481a9df7fd6dcad7ea83db3929f75a3b358a9d9 100644
--- a/test_resources/ldap.ldif
+++ b/test_resources/ldap.ldif
@@ -19,7 +19,7 @@ cn: John Smith
sn: Smith
givenName: John
uid: jsmith1
-mail: John.Smith@example.com
+mail: John.Smith@metabase.com
userPassword: strongpassword
dn: cn=Sally Brown,ou=people,dc=metabase,dc=com
@@ -31,5 +31,34 @@ cn: Sally Brown
sn: Brown
givenName: Sally
uid: sbrown20
-mail: sally.brown@example.com
+mail: sally.brown@metabase.com
userPassword: 1234
+
+dn: ou=birds,dc=metabase,dc=com
+objectClass: top
+objectClass: organizationalUnit
+ou: birds
+
+dn: cn=Rasta Toucan,ou=birds,dc=metabase,dc=com
+objectClass: top
+objectClass: person
+objectClass: organizationalPerson
+objectClass: inetOrgPerson
+cn: Rasta Toucan
+givenName: Rasta
+sn: Toucan
+uid: rasta
+mail: rasta@metabase.com
+userPassword: blueberries
+
+dn: cn=Lucky Pigeon,ou=birds,dc=metabase,dc=com
+objectClass: top
+objectClass: person
+objectClass: organizationalPerson
+objectClass: inetOrgPerson
+cn: Lucky Pigeon
+givenName: Lucky
+sn: Pigeon
+uid: lucky
+mail: lucky@metabase.com
+userPassword: notalmonds