From 3795b56cc45670b5fb4941954ca9a2c5b0234d50 Mon Sep 17 00:00:00 2001
From: adam-james <21064735+adam-james-v@users.noreply.github.com>
Date: Mon, 25 Jul 2022 13:02:29 -0700
Subject: [PATCH] Add a check to PUT /user/:id to disallow name edits if an SSO
user (#23752)
* Add a check to PUT /user/:id to disallow name edits if an SSO user
* Clean up After SAML SSO tests
The `:sso_source` key is set for the Rasta user in some SAML tests, but is expeted to be nil in subsequent tests, so
we clean up in the SAML test ns.
* Add a test to ensure SSO user names can't be changed via API
* Missed a change I had made while adjusting tests
* valid-name-update? take 1 name, to allow better error messages
Let's the user know that first or last name is the cause of a problem, rather than just 'names'.
* Remove unneeded thread macro
* Use partial=
* slight change to local fn for reusability
---
.../sso/integrations/saml_test.clj | 2 +-
src/metabase/api/user.clj | 18 ++++++++++++++++
src/metabase/models/user.clj | 2 +-
test/metabase/api/user_test.clj | 21 +++++++++++++++++++
4 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj b/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj
index eee76be8342..948c2539fc7 100644
--- a/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj
@@ -128,7 +128,7 @@
(f)
(finally
(u/ignore-exceptions (do (db/update-where! User {} :login_attributes nil)
- (db/update-where! User {:email "rasta@metabase.com"} :first_name "Rasta" :last_name "Toucan"))))))
+ (db/update-where! User {:email "rasta@metabase.com"} :first_name "Rasta" :last_name "Toucan" :sso_source nil))))))
(defmacro ^:private with-saml-default-setup [& body]
`(with-valid-premium-features-token
diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj
index 1f6466746d0..4cafce2bc28 100644
--- a/src/metabase/api/user.clj
+++ b/src/metabase/api/user.clj
@@ -275,6 +275,17 @@
(not google_auth)
(not ldap_auth))))
+(defn- valid-name-update?
+ "This predicate tests whether or not the user is allowed to update the first/last name associated with this account.
+ If the user is an SSO user, no name edits are allowed, but we accept if the new names are equal to the existing names."
+ [{:keys [google_auth ldap_auth sso_source] :as user} name-key new-name]
+ (or
+ (= (get user name-key) new-name)
+ (and
+ (not sso_source)
+ (not google_auth)
+ (not ldap_auth))))
+
(api/defendpoint PUT "/:id"
"Update an existing, active `User`.
Self or superusers can update user info and groups.
@@ -298,6 +309,13 @@
(api/let-404 [user-before-update (fetch-user :id id, :is_active true)]
;; Google/LDAP non-admin users can't change their email to prevent account hijacking
(api/check-403 (valid-email-update? user-before-update email))
+ ;; SSO users (JWT, SAML, LDAP, Google) can't change their first/last names
+ (when (contains? body :first_name)
+ (api/checkp (valid-name-update? user-before-update :first_name first_name)
+ "first_name" (tru "Editing first name is not allowed for SSO users.")))
+ (when (contains? body :last_name)
+ (api/checkp (valid-name-update? user-before-update :last_name last_name)
+ "last_name" (tru "Editing last name is not allowed for SSO users.")))
;; can't change email if it's already taken BY ANOTHER ACCOUNT
(api/checkp (not (db/exists? User, :%lower.email (if email (u/lower-case-en email) email), :id [:not= id]))
"email" (tru "Email address already associated to another user."))
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index f8b30792e3d..b4b290feb08 100644
--- a/src/metabase/models/user.clj
+++ b/src/metabase/models/user.clj
@@ -121,7 +121,7 @@
(def admin-or-self-visible-columns
"Sequence of columns that we can/should return for admins fetching a list of all Users, or for the current user
fetching themselves. Needed to power the admin page."
- (into default-user-columns [:google_auth :ldap_auth :is_active :updated_at :login_attributes :locale]))
+ (into default-user-columns [:google_auth :ldap_auth :sso_source :is_active :updated_at :login_attributes :locale]))
(def non-admin-or-self-visible-columns
"Sequence of columns that we will allow non-admin Users to see when fetching a list of Users. Why can non-admins see
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index 2becb4faea3..08461e87145 100644
--- a/test/metabase/api/user_test.clj
+++ b/test/metabase/api/user_test.clj
@@ -35,6 +35,7 @@
:is_active true
:last_login false
:ldap_auth false
+ :sso_source nil
:login_attributes nil
:updated_at true
:locale nil})))
@@ -641,6 +642,26 @@
(change-user-via-api! {:first_name "Blue"
:last_name nil})))))))))
+(deftest update-sso-user-test
+ (testing "PUT /api/user/:id"
+ (testing "Test that we do not update a user's first and last names if they are an SSO user."
+ (mt/with-temp User [{user-id :id} {:first_name "SSO"
+ :last_name "User"
+ :email "sso-user@metabase.com"
+ :sso_source "jwt"
+ :is_superuser true}]
+ (letfn [(change-user-via-api! [expected-status m]
+ (mt/user-http-request :crowberto :put expected-status (str "user/" user-id) m))]
+ (testing "`:first_name` changes are rejected"
+ (is (= {:errors {:first_name "Editing first name is not allowed for SSO users."}}
+ (change-user-via-api! 400 {:first_name "NOT-SSO"}))))
+ (testing "`:last_name` changes are rejected"
+ (is (= {:errors {:last_name "Editing last name is not allowed for SSO users."}}
+ (change-user-via-api! 400 {:last_name "USER"}))))
+ (testing "New names that are the same as existing names succeed because there is no change."
+ (is (partial= {:first_name "SSO" :last_name "User"}
+ (change-user-via-api! 200 {:first_name "SSO" :last_name "User"})))))))))
+
(deftest update-email-check-if-already-used-test
(testing "PUT /api/user/:id"
(testing "test that updating a user's email to an existing inactive user's email fails"
--
GitLab