From 815fcb3c803ea5ba939252897ba42c3bc3a048e8 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Thu, 10 Jun 2021 18:24:47 -0700
Subject: [PATCH] Allow LDAP logins even if givenName or sn attributes are
missing (#16139)
---
.../administration-guide/10-single-sign-on.md | 10 +---
docs/troubleshooting-guide/ldap.md | 1 -
.../enhancements/integrations/ldap.clj | 34 +++++++----
.../sso/integrations/jwt.clj | 6 +-
.../enhancements/integrations/ldap_test.clj | 50 +++++++++++++++-
.../ldap/default_implementation.clj | 60 ++++++++++++-------
src/metabase/integrations/ldap/interface.clj | 4 +-
test/metabase/integrations/ldap_test.clj | 53 +++++++++++++++-
test_resources/ldap.ldif | 11 ++++
9 files changed, 181 insertions(+), 48 deletions(-)
diff --git a/docs/administration-guide/10-single-sign-on.md b/docs/administration-guide/10-single-sign-on.md
index 77de0f3e1ff..c21b9899971 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 67c588949a6..d9c572f5026 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 9af9f71abc8..6023d63ff15 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 4db2f877176..28e01c173b7 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 e6cf447f269..d2f00114cd6 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 0adf998168c..4f4d61a1ad8 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 535c0b5ab29..3049314b973 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 9e6b58adedc..28eda6abb39 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 fd6aacfb24c..7f2c75b4444 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
--
GitLab