Skip to content
Snippets Groups Projects
Unverified Commit 34d089ce authored by adam-james's avatar adam-james Committed by GitHub
Browse files

LDAP now also properly allows `nil` in names. (#23716) 

* LDAP now also properly allows `nil` in names.

There were still places where "Unknown" was substituted in to name keys, which we don't need to do anymore as 'nil' is
valid for first/last names of users.

* Remove unused namespaces/refers

* Adjust LDAP so that even nil name values update the Metabase user

Also changed a test in both the OSS and EE LDAP tests to make sure this is indeed true that 'nil' values are correctly
set in the app-db when LDAP does not send `givenName` and/or `sn`.
parent 3f294234
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
+ 7
− 9
View file @ 34d089ce
......@@ -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)))]
......
enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj
+ 8
− 8
View file @ 34d089ce
......@@ -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"))))))
src/metabase/integrations/ldap/default_implementation.clj
+ 6
− 18
View file @ 34d089ce
......@@ -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
......
src/metabase/models/user.clj
+ 2
− 2
View file @ 34d089ce
......@@ -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)
......
test/metabase/integrations/ldap_test.clj
+ 8
− 8
View file @ 34d089ce
......@@ -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
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment