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

@@ -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"

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)