diff --git a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
index ef15dd044dfdc1a41695a11a3a1d9538aab01936..ca1a4d995706543586a83b03fa21ceac95292ac7 100644
--- a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
+++ b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
@@ -45,7 +45,7 @@
(defn- attribute-synced-user
[{:keys [attributes first-name last-name email]}]
- (when-let [user (db/select-one [User :id :last_login :first_name :last_name :login_attributes]
+ (when-let [user (db/select-one [User :id :last_login :first_name :last_name :login_attributes :is_active]
:%lower.email (u/lower-case-en email))]
(let [syncable-attributes (syncable-user-attributes attributes)
old-first-name (:first_name user)
@@ -62,7 +62,7 @@
(if (seq user-changes)
(do
(db/update! User (:id user) user-changes)
- (db/select-one [User :id :last_login] :id (:id user))) ; Reload updated user
+ (db/select-one [User :id :last_login :is_active] :id (:id user))) ; Reload updated user
user))))
(s/defn ^:private find-user* :- (s/maybe EEUserInfo)
@@ -81,11 +81,11 @@
[{:keys [first-name last-name email groups attributes], :as user-info} :- EEUserInfo
{:keys [sync-groups?], :as settings} :- i/LDAPSettings]
(let [user (or (attribute-synced-user user-info)
- (user/create-new-ldap-auth-user!
- {:first_name (or first-name (trs "Unknown"))
- :last_name (or last-name (trs "Unknown"))
- :email email
- :login_attributes attributes}))]
+ (-> (user/create-new-ldap-auth-user! {:first_name (or first-name (trs "Unknown"))
+ :last_name (or last-name (trs "Unknown"))
+ :email email
+ :login_attributes attributes})
+ (assoc :is_active true)))]
(u/prog1 user
(when sync-groups?
(let [group-ids (default-impl/ldap-groups->mb-group-ids groups settings)
diff --git a/src/metabase/api/session.clj b/src/metabase/api/session.clj
index d35f38d8e08fd1e9e2cb7f2e7b0ddea65fde3ccf..23a5c6c5d5dc75e8203fb3cabaf8222c4bcfe558 100644
--- a/src/metabase/api/session.clj
+++ b/src/metabase/api/session.clj
@@ -92,19 +92,28 @@
;; Since LDAP knows about the user, fail here to prevent the local strategy to be tried with a possibly
;; outdated password
(throw (ex-info (str password-fail-message)
- {:status-code 401
- :errors {:password password-fail-snippet}})))
- ;; password is ok, return new session
- (create-session! :sso (ldap/fetch-or-create-user! user-info) device-info))
+ {:status-code 401
+ :errors {:password password-fail-snippet}})))
+ ;; password is ok, return new session if user is not deactivated
+ (let [user (ldap/fetch-or-create-user! user-info)]
+ (if (:is_active user)
+ (create-session! :sso (ldap/fetch-or-create-user! user-info) device-info)
+ (throw (ex-info (str disabled-account-message)
+ {:status-code 401
+ :errors {:_error disabled-account-snippet}})))))
(catch LDAPSDKException e
(log/error e (trs "Problem connecting to LDAP server, will fall back to local authentication"))))))
(s/defn ^:private email-login :- (s/maybe {:id UUID, s/Keyword s/Any})
"Find a matching `User` if one exists and return a new Session for them, or `nil` if they couldn't be authenticated."
[username password device-info :- request.u/DeviceInfo]
- (when-let [user (db/select-one [User :id :password_salt :password :last_login], :%lower.email (u/lower-case-en username), :is_active true)]
+ (when-let [user (db/select-one [User :id :password_salt :password :last_login :is_active], :%lower.email (u/lower-case-en username))]
(when (pass/verify-password password (:password_salt user) (:password user))
- (create-session! :password user device-info))))
+ (if (:is_active user)
+ (create-session! :password user device-info)
+ (throw (ex-info (str disabled-account-message)
+ {:status-code 401
+ :errors {:_error disabled-account-snippet}}))))))
(def ^:private throttling-disabled? (config/config-bool :mb-disable-session-throttle))
diff --git a/src/metabase/integrations/ldap/default_implementation.clj b/src/metabase/integrations/ldap/default_implementation.clj
index 41012e6120a75a7eeb80da01f3ed94fc2d88e44f..99be624a0c66fdcf36c266e48cc3c01b123f8666 100644
--- a/src/metabase/integrations/ldap/default_implementation.clj
+++ b/src/metabase/integrations/ldap/default_implementation.clj
@@ -125,7 +125,7 @@
(s/defn ^:private fetch-or-create-user!* :- (class User)
[{:keys [first-name last-name email groups]} :- i/UserInfo
{:keys [sync-groups?], :as settings} :- i/LDAPSettings]
- (let [user (db/select-one [User :id :last_login :first_name :last_name] :%lower.email (u/lower-case-en email))
+ (let [user (db/select-one [User :id :last_login :first_name :last_name :is_active] :%lower.email (u/lower-case-en email))
new-user (if user
(let [old-first-name (:first_name user)
old-last-name (:last_name user)
@@ -137,12 +137,12 @@
(if (seq user-changes)
(do
(db/update! User (:id user) user-changes)
- (db/select-one [User :id :last_login] :id (:id user))) ; Reload updated user
+ (db/select-one [User :id :last_login :is_active] :id (:id user))) ; Reload updated user
user))
- (user/create-new-ldap-auth-user!
- {:first_name (or first-name (trs "Unknown"))
- :last_name (or last-name (trs "Unknown"))
- :email email}))]
+ (-> (user/create-new-ldap-auth-user! {:first_name (or first-name (trs "Unknown"))
+ :last_name (or last-name (trs "Unknown"))
+ :email email})
+ (assoc :is_active true)))]
(u/prog1 new-user
(when sync-groups?
(let [group-ids (ldap-groups->mb-group-ids groups settings)
diff --git a/test/metabase/api/session_test.clj b/test/metabase/api/session_test.clj
index 4f1a663d2ec23f0062f439461cd9193c16969a50..fe9de1a9fbddeac12ff4659474c08bc232565dee 100644
--- a/test/metabase/api/session_test.clj
+++ b/test/metabase/api/session_test.clj
@@ -95,7 +95,7 @@
(testing "Test for inactive user (user shouldn't be able to login if :is_active = false)"
;; Return same error as incorrect password to avoid leaking existence of user
- (is (= {:errors {:password "did not match stored password"}}
+ (is (= {:errors {:_error "Your account is disabled."}}
(mt/client :post 401 "session" (mt/user->credentials :trashbird)))))
(testing "Test for password checking"
@@ -419,6 +419,15 @@
(is (= {:errors {:password "did not match stored password"}}
(mt/client :post 401 "session" (mt/user->credentials :lucky)))))
+ (testing "Test that a deactivated user cannot login with LDAP"
+ (let [user-id (test-users/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)))))
+
(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?)