diff --git a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
index 3eb8d1fe28c303d41591d41c2b6db81ad2aa4d16..486f3552a3eb3dbbab907bd0cf65b8351b88184f 100644
--- a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
+++ b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
@@ -7,7 +7,7 @@
[metabase.models.user :as user :refer [User]]
[metabase.public-settings.premium-features :as premium-features :refer [defenterprise-schema]]
[metabase.util :as u]
- [metabase.util.i18n :refer [deferred-tru trs]]
+ [metabase.util.i18n :refer [deferred-tru]]
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db])
@@ -42,15 +42,13 @@
(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}))]
+ (when (not= first-name old-first-name)
+ {:first_name first-name})
+ (when (not= last-name old-last-name)
+ {:last_name last-name}))]
(if (seq user-changes)
(do
(db/update! User (:id user) user-changes)
@@ -77,8 +75,8 @@
[{: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"))
+ (-> (user/create-new-ldap-auth-user! {:first_name first-name
+ :last_name last-name
:email email
:login_attributes attributes})
(assoc :is_active true)))]
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 03b2813d9f9596ff68b367164a0b1314966eafd1..9fbe632284c0b3788f438ee5bcb1f66f7ca392c4 100644
--- a/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj
@@ -226,11 +226,11 @@
(finally (db/delete! User :email "john.smith@metabase.com"))))
(try
- (testing "a user without a givenName attribute defaults to Unknown"
+ (testing "a user without a givenName attribute has `nil` for that attribute"
(ldap/fetch-or-create-user! (ldap/find-user "jmiller"))
- (is (= {:first_name "Unknown"
+ (is (= {:first_name nil
:last_name "Miller"
- :common_name "Unknown Miller"}
+ :common_name "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"
@@ -240,10 +240,10 @@
: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"
+ (testing "if givenName or sn attributes are removed, values stored in Metabase are updated to `nil` to respect the IdP response."
(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")))))
+ (is (= {:first_name nil
+ :last_name nil
+ :common_name "jane.miller@metabase.com"}
+ (select-keys (db/select-one User :email "jane.miller@metabase.com") [:first_name :last_name :common_name]))))
(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 70b188375a13c5bddf43f2eea6d3bf19c3f4c683..ed474955fa45f8df521b7b09937e34ef23ba2361 100644
--- a/src/metabase/integrations/ldap/default_implementation.clj
+++ b/src/metabase/integrations/ldap/default_implementation.clj
@@ -7,7 +7,6 @@
[metabase.models.user :as user :refer [User]]
[metabase.public-settings.premium-features :refer [defenterprise-schema]]
[metabase.util :as u]
- [metabase.util.i18n :refer [trs]]
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db])
@@ -114,37 +113,26 @@
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"))))
-
(defenterprise-schema fetch-or-create-user! :- (class User)
"Using the `user-info` (from `find-user`) get the corresponding Metabase user, creating it if necessary."
metabase-enterprise.enhancements.integrations.ldap
[{: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 :is_active]
- :%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)
- 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}))]
+ (when (not= first-name old-first-name) {:first_name first-name})
+ (when (not= last-name old-last-name) {:last_name last-name}))]
(if (seq user-changes)
(do
(db/update! User (:id user) user-changes)
(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"))
+ (-> (user/create-new-ldap-auth-user! {:first_name first-name
+ :last_name last-name
:email email})
(assoc :is_active true)))]
(u/prog1 new-user
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index f697630519f00898ee940d8e3c282135de2a6d50..f8b30792e3de6e2a5bedbbd3cd1ab93d11f47c72 100644
--- a/src/metabase/models/user.clj
+++ b/src/metabase/models/user.clj
@@ -261,8 +261,8 @@
(def NewUser
"Required/optionals parameters needed to create a new user (for any backend)"
- {(s/optional-key :first_name) su/NonBlankString
- (s/optional-key :last_name) su/NonBlankString
+ {(s/optional-key :first_name) (s/maybe su/NonBlankString)
+ (s/optional-key :last_name) (s/maybe su/NonBlankString)
:email su/Email
(s/optional-key :password) (s/maybe su/NonBlankString)
(s/optional-key :login_attributes) (s/maybe LoginAttributes)
diff --git a/test/metabase/integrations/ldap_test.clj b/test/metabase/integrations/ldap_test.clj
index 971d92938c1f53a3170153b90ecc99ee9825d735..d1f13c99fd77c718ea2c7cbfe4eb9ed84496c07e 100644
--- a/test/metabase/integrations/ldap_test.clj
+++ b/test/metabase/integrations/ldap_test.clj
@@ -134,11 +134,11 @@
(finally (db/delete! User :email "john.smith@metabase.com"))))
(try
- (testing "a user without a givenName attribute defaults to Unknown"
+ (testing "a user without a givenName attribute has `nil` for that attribute"
(ldap/fetch-or-create-user! (ldap/find-user "jmiller"))
- (is (= {:first_name "Unknown"
+ (is (= {:first_name nil
:last_name "Miller"
- :common_name "Unknown Miller"}
+ :common_name "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"
@@ -148,12 +148,12 @@
: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"
+ (testing "if givenName or sn attributes are removed, values stored in Metabase are updated to `nil` to respect the IdP response."
(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")))))
+ (is (= {:first_name nil
+ :last_name nil
+ :common_name "jane.miller@metabase.com"}
+ (select-keys (db/select-one User :email "jane.miller@metabase.com") [:first_name :last_name :common_name]))))
(finally (db/delete! User :email "jane.miller@metabase.com"))))))
(deftest group-matching-test