From ceaddd14f01c808009dea2bbf8fc3d1f3cc13cdf Mon Sep 17 00:00:00 2001
From: Ryan Senior <ryan@metabase.com>
Date: Fri, 29 Jun 2018 11:14:42 -0500
Subject: [PATCH] Prevent google and ldap users from changing their email
address
Google and LDAP users shouldn't be able to change their own email
addresses as we get that from the source.
---
src/metabase/api/user.clj | 39 +++++++++++++++++++++++----------
test/metabase/api/user_test.clj | 22 +++++++++++++++++++
2 files changed, 49 insertions(+), 12 deletions(-)
diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj
index 62bdebac745..cee471889fa 100644
--- a/src/metabase/api/user.clj
+++ b/src/metabase/api/user.clj
@@ -86,6 +86,19 @@
(check-self-or-superuser id)
(api/check-404 (fetch-user :id id, :is_active true)))
+(defn- valid-email-update?
+ "This predicate tests whether or not the user is allowed to update the email address associated with this account."
+ [{:keys [google_auth ldap_auth email] :as foo } maybe-new-email]
+ (or
+ ;; Admin users can update
+ api/*is-superuser?*
+ ;; If the email address didn't change, let it through
+ (= email maybe-new-email)
+ ;; We should not allow a regular user to change their email address if they are a google/ldap user
+ (and
+ (not google_auth)
+ (not ldap_auth))))
+
(api/defendpoint PUT "/:id"
"Update an existing, active `User`."
[id :as {{:keys [email first_name last_name is_superuser login_attributes] :as body} :body}]
@@ -95,18 +108,20 @@
login_attributes (s/maybe user/LoginAttributes)}
(check-self-or-superuser id)
;; only allow updates if the specified account is active
- (api/check-404 (db/exists? User, :id id, :is_active true))
- ;; can't change email if it's already taken BY ANOTHER ACCOUNT
- (api/checkp (not (db/exists? User, :email email, :id [:not= id]))
- "email" (tru "Email address already associated to another user."))
- (api/check-500
- (db/update! User id
- (u/select-keys-when body
- :present (when api/*is-superuser?*
- #{:login_attributes})
- :non-nil (set (concat [:first_name :last_name :email]
- (when api/*is-superuser?*
- [:is_superuser]))))))
+ (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))
+ ;; can't change email if it's already taken BY ANOTHER ACCOUNT
+ (api/checkp (not (db/exists? User, :email email, :id [:not= id]))
+ "email" (tru "Email address already associated to another user."))
+ (api/check-500
+ (db/update! User id
+ (u/select-keys-when body
+ :present (when api/*is-superuser?*
+ #{:login_attributes})
+ :non-nil (set (concat [:first_name :last_name :email]
+ (when api/*is-superuser?*
+ [:is_superuser])))))))
(fetch-user :id id))
(api/defendpoint PUT "/:id/reactivate"
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index 91db0f9ad2d..d2e059683bf 100644
--- a/test/metabase/api/user_test.clj
+++ b/test/metabase/api/user_test.clj
@@ -322,6 +322,28 @@
((test-users/user->client :crowberto) :put 404 (str "user/" (test-users/user->id :trashbird))
{:email "toucan@metabase.com"}))
+;; Google auth users shouldn't be able to change their own password as we get that from Google
+(expect
+ "You don't have permissions to do that."
+ (tt/with-temp User [user {:email "anemail@metabase.com"
+ :password "def123"
+ :google_auth true}]
+ (let [creds {:username "anemail@metabase.com"
+ :password "def123"}]
+ (http/client creds :put 403 (format "user/%d" (u/get-id user))
+ {:email "adifferentemail@metabase.com"}))))
+
+;; Similar to Google auth accounts, we should not allow LDAP users to change their own email address as we get that
+;; from the LDAP server
+(expect
+ "You don't have permissions to do that."
+ (tt/with-temp User [user {:email "anemail@metabase.com"
+ :password "def123"
+ :ldap_auth true}]
+ (let [creds {:username "anemail@metabase.com"
+ :password "def123"}]
+ (http/client creds :put 403 (format "user/%d" (u/get-id user))
+ {:email "adifferentemail@metabase.com"}))))
;; ## PUT /api/user/:id/password
;; Test that a User can change their password (superuser and non-superuser)
--
GitLab