diff --git a/docs/administration-guide/10-single-sign-on.md b/docs/administration-guide/10-single-sign-on.md
index 77de0f3e1ff6537e1c061eca587ba762ac678719..c21b9899971550012320b532c35556628af1adce 100644
--- a/docs/administration-guide/10-single-sign-on.md
+++ b/docs/administration-guide/10-single-sign-on.md
@@ -41,13 +41,9 @@ Metabase will pull out three main attributes from your LDAP directory - email (d
-Metabase needs to pull out three main attributes from your LDAP directory.
-
-- email (defaults to the `mail` attribute)
-- first name (defaults to the `givenName` attribute)
-- last name (defaults to the `sn` attribute).
-
-Your LDAP directory must have these three fields populated or Metabase won't be able to create or log in the user. If your LDAP setup uses different values for defaults, you must specify these under the "Attributes" portion of the form.
+Your LDAP directory must have the email field populated or Metabase won't be able to create or log in the user. If
+either name field is missing, Metabase will use a default of "Unknown", and the name can be changed manually in the
+user's account settings.
If you have user groups in Metabase you are using to control access, it is often tedious to have to manually assign a user to a group after they're logged in via SSO. You can take advantage of the groups your LDAP directory uses by enabling Group Mappings, and specifying which LDAP group corresponds to which user group on your Metabase server.
diff --git a/docs/troubleshooting-guide/ldap.md b/docs/troubleshooting-guide/ldap.md
index 67c588949a6df5ee5572946a524ee0c421001ca0..d9c572f5026b6ab86be371fd6ea598b6230ac5f6 100644
--- a/docs/troubleshooting-guide/ldap.md
+++ b/docs/troubleshooting-guide/ldap.md
@@ -62,5 +62,4 @@ If you run into an issue, check that you can login and use your LDAP directory w
### Current limitations
-- Metabase will populate the user profile with the name and surname a user has on LDAP on the first login. In case the user changes the name on the directory, it won't be automatically updated on Metabase.
- When using Metabase Enterprise with a MySQL database and LDAP enabled, make sure that you disable the sync of binary fields from your LDAP directory by using the `MB_LDAP_SYNC_USER_ATTRIBUTES_BLACKLIST` environment variable, as you may hit the 60K field size limitation of the text field in MySQL, which will prevent the creation or log-in of your users.
diff --git a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
index 9af9f71abc8390acc6ca8d5c497e7e3fb6148768..6023d63ff15d090e667d1eaa93aecf649f869706 100644
--- a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
+++ b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
@@ -8,7 +8,7 @@
[metabase.models.user :as user :refer [User]]
[metabase.public-settings.metastore :as settings.metastore]
[metabase.util :as u]
- [metabase.util.i18n :refer [deferred-tru]]
+ [metabase.util.i18n :refer [deferred-tru trs]]
[metabase.util.schema :as su]
[pretty.core :refer [PrettyPrintable]]
[schema.core :as s]
@@ -40,14 +40,26 @@
(apply dissoc m :objectclass (map (comp keyword u/lower-case-en) (ldap-sync-user-attributes-blacklist)))))
(defn- attribute-synced-user
- [{:keys [attributes email]}]
- (when-let [user (db/select-one [User :id :last_login :login_attributes] :%lower.email (u/lower-case-en email))]
- (let [syncable-attributes (syncable-user-attributes attributes)]
- ;; Update User's `login_attributes` if needed
- (if (and (not= (:login_attributes user) syncable-attributes)
- (db/update! User (:id user) :login_attributes syncable-attributes))
- (db/select-one [User :id :last_login] :id (:id user)) ; Reload updated user
- user))))
+ [{:keys [attributes first-name last-name email]}]
+ (when-let [user (db/select-one [User :id :last_login :first_name :last_name :login_attributes]
+ :%lower.email (u/lower-case-en email))]
+ (let [syncable-attributes (syncable-user-attributes attributes)
+ old-first-name (:first_name user)
+ old-last-name (:last_name user)
+ new-first-name (default-impl/updated-name-part first-name old-first-name)
+ new-last-name (default-impl/updated-name-part last-name old-last-name)
+ user-changes (merge
+ (when-not (= syncable-attributes (:login_attributes user))
+ {:login_attributes syncable-attributes})
+ (when-not (= new-first-name old-first-name)
+ {:first_name new-first-name})
+ (when-not (= new-last-name old-last-name)
+ {:last_name new-last-name}))]
+ (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
+ user))))
(s/defn ^:private find-user* :- (s/maybe EEUserInfo)
[ldap-connection :- LDAPConnectionPool
@@ -62,8 +74,8 @@
{:keys [sync-groups?], :as settings} :- i/LDAPSettings]
(let [user (or (attribute-synced-user user-info)
(user/create-new-ldap-auth-user!
- {:first_name first-name
- :last_name last-name
+ {:first_name (or first-name (trs "Unknown"))
+ :last_name (or last-name (trs "Unknown"))
:email email
:login_attributes attributes}))]
(u/prog1 user
diff --git a/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj b/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
index 4db2f877176e6cc615b9e0cca421879c94101b53..28e01c173b7ffedeab164a0faf59b202e7196933 100644
--- a/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
+++ b/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
@@ -9,7 +9,7 @@
[metabase.integrations.common :as integrations.common]
[metabase.server.middleware.session :as mw.session]
[metabase.server.request.util :as request.u]
- [metabase.util.i18n :refer [tru]]
+ [metabase.util.i18n :refer [trs tru]]
[ring.util.response :as resp])
(:import java.net.URLEncoder))
@@ -63,8 +63,8 @@
e))))
login-attrs (jwt-data->login-attributes jwt-data)
email (get jwt-data (jwt-attribute-email))
- first-name (get jwt-data (jwt-attribute-firstname) "Unknown")
- last-name (get jwt-data (jwt-attribute-lastname) "Unknown")
+ first-name (get jwt-data (jwt-attribute-firstname) (trs "Unknown"))
+ last-name (get jwt-data (jwt-attribute-lastname) (trs "Unknown"))
user (fetch-or-create-user! first-name last-name email login-attrs)
session (session/create-session! :sso user (request.u/device-info request))
redirect-url (or redirect (URLEncoder/encode "/"))]
diff --git a/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj b/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj
index e6cf447f2696ae18be362c720331ef2855829c56..d2f00114cd66605d37a0418f68ea925df4064e8a 100644
--- a/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj
@@ -50,7 +50,19 @@
:givenname "Fred"
:sn "Taylor"}
:groups []}
- (ldap/find-user "fred.taylor@metabase.com")))))))
+ (ldap/find-user "fred.taylor@metabase.com"))))
+
+ (testing "find by email, no givenName"
+ (is (= {:dn "cn=Jane Miller,ou=People,dc=metabase,dc=com"
+ :first-name nil
+ :last-name "Miller"
+ :email "jane.miller@metabase.com"
+ :attributes {:uid "jmiller"
+ :mail "jane.miller@metabase.com"
+ :cn "Jane Miller"
+ :sn "Miller"}
+ :groups []}
+ (ldap/find-user "jane.miller@metabase.com")))))))
(deftest attribute-sync-test
(with-redefs [metastore/enable-enhancements? (constantly true)]
@@ -173,3 +185,39 @@
:email "john.smith@metabase.com"))))))
(finally
(db/delete! User :%lower.email "john.smith@metabase.com")))))))
+
+(deftest fetch-or-create-user-test
+ (with-redefs [metastore/enable-enhancements? (constantly true)]
+ (ldap.test/with-ldap-server
+ (testing "a new user is created when they don't already exist"
+ (try
+ (ldap/fetch-or-create-user! (ldap/find-user "jsmith1"))
+ (is (= {:first_name "John"
+ :last_name "Smith"
+ :common_name "John Smith"
+ :email "john.smith@metabase.com"}
+ (into {} (db/select-one [User :first_name :last_name :email] :email "john.smith@metabase.com"))))
+ (finally (db/delete! User :email "john.smith@metabase.com"))))
+
+ (try
+ (testing "a user without a givenName attribute defaults to Unknown"
+ (ldap/fetch-or-create-user! (ldap/find-user "jmiller"))
+ (is (= {:first_name "Unknown"
+ :last_name "Miller"
+ :common_name "Unknown Miller"}
+ (into {} (db/select-one [User :first_name :last_name] :email "jane.miller@metabase.com")))))
+
+ (testing "when givenName or sn attributes change in LDAP, they are updated in Metabase on next login"
+ (ldap/fetch-or-create-user! (assoc (ldap/find-user "jmiller") :first-name "Jane" :last-name "Doe"))
+ (is (= {:first_name "Jane"
+ :last_name "Doe"
+ :common_name "Jane Doe"}
+ (into {} (db/select-one [User :first_name :last_name] :email "jane.miller@metabase.com")))))
+
+ (testing "if givenName or sn attributes are removed, values stored in Metabase are not overwritten on next login"
+ (ldap/fetch-or-create-user! (assoc (ldap/find-user "jmiller") :first-name nil :last-name nil))
+ (is (= {:first_name "Jane"
+ :last_name "Doe"
+ :common_name "Jane Doe"}
+ (into {} (db/select-one [User :first_name :last_name] :email "jane.miller@metabase.com")))))
+ (finally (db/delete! User :email "jane.miller@metabase.com"))))))
diff --git a/src/metabase/integrations/ldap/default_implementation.clj b/src/metabase/integrations/ldap/default_implementation.clj
index 0adf998168c9140382df1d5cb395a28eaf090b98..4f4d61a1ad8193dbc6c64a1d890dc71cc30a4f6d 100644
--- a/src/metabase/integrations/ldap/default_implementation.clj
+++ b/src/metabase/integrations/ldap/default_implementation.clj
@@ -6,6 +6,7 @@
[metabase.integrations.ldap.interface :as i]
[metabase.models.user :as user :refer [User]]
[metabase.util :as u]
+ [metabase.util.i18n :as ui18n :refer [trs]]
[metabase.util.schema :as su]
[pretty.core :refer [PrettyPrintable]]
[schema.core :as s]
@@ -58,18 +59,16 @@
(let [{first-name (keyword first-name-attribute)
last-name (keyword last-name-attribute)
email (keyword email-attribute)} result]
- ;; Make sure we got everything as these are all required for new accounts
- (when-not (some empty? [dn first-name last-name email])
- {:dn dn
- :first-name first-name
- :last-name last-name
- :email email
- :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 (u/one-or-many (:memberof result))
- (user-groups ldap-connection dn settings)
- []))})))
+ {:dn dn
+ :first-name first-name
+ :last-name last-name
+ :email email
+ :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 (u/one-or-many (:memberof result))
+ (user-groups ldap-connection dn settings)
+ []))}))
(s/defn ^:private find-user* :- (s/maybe i/UserInfo)
[ldap-connection :- LDAPConnectionPool
@@ -91,19 +90,40 @@
flatten
set))
+(defn updated-name-part
+ "Given a first or last name returned by LDAP, and the equivalent name currently stored by Metabase, return the new
+ name that should be stored by Metabase."
+ [ldap-name mb-name]
+ (if (and mb-name (nil? ldap-name))
+ ;; Don't overwrite a stored name if no name was returned by LDAP
+ mb-name
+ (or ldap-name (trs "Unknown"))))
+
(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 (or (db/select-one [User :id :last_login] :email (u/lower-case-en email))
- (user/create-new-ldap-auth-user!
- {:first_name first-name
- :last_name last-name
- :email email}))]
- (u/prog1 user
+ (let [user (db/select-one [User :id :last_login :first_name :last_name] :%lower.email (u/lower-case-en email))
+ new-user (if user
+ (let [old-first-name (:first_name user)
+ old-last-name (:last_name user)
+ new-first-name (updated-name-part first-name old-first-name)
+ new-last-name (updated-name-part last-name old-last-name)
+ user-changes (merge
+ (when-not (= new-first-name old-first-name) {:first_name new-first-name})
+ (when-not (= new-last-name old-last-name) {:last_name new-last-name}))]
+ (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
+ user))
+ (user/create-new-ldap-auth-user!
+ {:first_name (or first-name (trs "Unknown"))
+ :last_name (or last-name (trs "Unknown"))
+ :email email}))]
+ (u/prog1 new-user
(when sync-groups?
(let [group-ids (ldap-groups->mb-group-ids groups settings)]
- (integrations.common/sync-group-memberships! user group-ids false))))))
-
+ (integrations.common/sync-group-memberships! new-user group-ids false))))))
;;; ------------------------------------------------------ impl ------------------------------------------------------
diff --git a/src/metabase/integrations/ldap/interface.clj b/src/metabase/integrations/ldap/interface.clj
index 535c0b5ab2902ff48e4502756c22efb4ac3b0925..3049314b973e95561f984bd26fa90953f3c3096f 100644
--- a/src/metabase/integrations/ldap/interface.clj
+++ b/src/metabase/integrations/ldap/interface.clj
@@ -9,8 +9,8 @@
(def UserInfo
"Schema for LDAP User info as returned by `user-info` and used as input to `fetch-or-create-user!`."
{:dn su/NonBlankString
- :first-name su/NonBlankString
- :last-name su/NonBlankString
+ :first-name (s/maybe su/NonBlankString)
+ :last-name (s/maybe su/NonBlankString)
:email su/Email
:groups [su/NonBlankString]
s/Keyword s/Any})
diff --git a/test/metabase/integrations/ldap_test.clj b/test/metabase/integrations/ldap_test.clj
index 9e6b58adedccabf53e39c55fdf16661c3fab5bc3..28eda6abb39852c588acf107c2902c1c78ab6346 100644
--- a/test/metabase/integrations/ldap_test.clj
+++ b/test/metabase/integrations/ldap_test.clj
@@ -2,9 +2,11 @@
(:require [clojure.test :refer :all]
[metabase.integrations.ldap :as ldap]
[metabase.integrations.ldap.default-implementation :as default-impl]
+ [metabase.models.user :as user :refer [User]]
[metabase.public-settings.metastore :as metastore]
[metabase.test :as mt]
- [metabase.test.integrations.ldap :as ldap.test]))
+ [metabase.test.integrations.ldap :as ldap.test]
+ [toucan.db :as db]))
(defn- get-ldap-details []
{:host "localhost"
@@ -96,7 +98,15 @@
:last-name "Taylor"
:email "fred.taylor@metabase.com"
:groups []}
- (ldap/find-user "fred.taylor@metabase.com")))))
+ (ldap/find-user "fred.taylor@metabase.com"))))
+
+ (testing "find by email, no givenName"
+ (is (= {:dn "cn=Jane Miller,ou=People,dc=metabase,dc=com"
+ :first-name nil
+ :last-name "Miller"
+ :email "jane.miller@metabase.com"
+ :groups []}
+ (ldap/find-user "jane.miller@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
@@ -115,7 +125,44 @@
: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")))))))
+ (ldap/find-user "sbrown20")))))))))
+
+(deftest fetch-or-create-user-test
+ ;; there are EE-specific versions of this test in `metabase-enterprise.enhancements.integrations.ldap-test`
+ (with-redefs [metastore/enable-enhancements? (constantly false)]
+ (ldap.test/with-ldap-server
+ (testing "a new user is created when they don't already exist"
+ (try
+ (ldap/fetch-or-create-user! (ldap/find-user "jsmith1"))
+ (is (= {:first_name "John"
+ :last_name "Smith"
+ :common_name "John Smith"
+ :email "john.smith@metabase.com"}
+ (into {} (db/select-one [User :first_name :last_name :email] :email "john.smith@metabase.com"))))
+ (finally (db/delete! User :email "john.smith@metabase.com"))))
+
+ (try
+ (testing "a user without a givenName attribute defaults to Unknown"
+ (ldap/fetch-or-create-user! (ldap/find-user "jmiller"))
+ (is (= {:first_name "Unknown"
+ :last_name "Miller"
+ :common_name "Unknown Miller"}
+ (into {} (db/select-one [User :first_name :last_name] :email "jane.miller@metabase.com")))))
+
+ (testing "when givenName or sn attributes change in LDAP, they are updated in Metabase on next login"
+ (ldap/fetch-or-create-user! (assoc (ldap/find-user "jmiller") :first-name "Jane" :last-name "Doe"))
+ (is (= {:first_name "Jane"
+ :last_name "Doe"
+ :common_name "Jane Doe"}
+ (into {} (db/select-one [User :first_name :last_name] :email "jane.miller@metabase.com")))))
+
+ (testing "if givenName or sn attributes are removed, values stored in Metabase are not overwritten on next login"
+ (ldap/fetch-or-create-user! (assoc (ldap/find-user "jmiller") :first-name nil :last-name nil))
+ (is (= {:first_name "Jane"
+ :last_name "Doe"
+ :common_name "Jane Doe"}
+ (into {} (db/select-one [User :first_name :last_name] :email "jane.miller@metabase.com")))))
+ (finally (db/delete! User :email "jane.miller@metabase.com"))))))
(deftest group-matching-test
(testing "LDAP group matching should identify Metabase groups using DN equality rules"
diff --git a/test_resources/ldap.ldif b/test_resources/ldap.ldif
index fd6aacfb24ce1781542e27fa4f5463eca15d756e..7f2c75b4444c6a1843ad577cd843814836da0b07 100644
--- a/test_resources/ldap.ldif
+++ b/test_resources/ldap.ldif
@@ -46,6 +46,17 @@ uid: ftaylor300
mAiL: fred.taylor@metabase.com
userpassword: pa$$word
+dn: cn=Jane Miller,ou=People,dc=metabase,dc=com
+objectClass: top
+objectClass: person
+objectClass: organizationalPerson
+objectClass: inetOrgPerson
+cn: Jane Miller
+sn: Miller
+uid: jmiller
+mail: jane.miller@metabase.com
+userPassword: n0peeking
+
dn: ou=Birds,dc=metabase,dc=com
objectClass: top
objectClass: organizationalUnit