diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj index 62bdebac745030d887337415d1a645d567e1abc7..cee471889faf679b4d329c730ee2f6f1b1b30e80 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 91db0f9ad2dfdb01b10cc6167ea18a261ab62ddb..d2e059683bf5f43ee1a4de3f6674288459eccefc 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)