diff --git a/src/metabase/api/email.clj b/src/metabase/api/email.clj index 55745dc22f2c623d0ab8763dcb4882d80111bf99..32d3e62fc3671443db0b54769594226526a37e2d 100644 --- a/src/metabase/api/email.clj +++ b/src/metabase/api/email.clj @@ -73,20 +73,27 @@ [:as {settings :body}] {settings su/Map} (validation/check-has-application-permission :setting) - (let [settings (-> settings - (select-keys (keys mb-to-smtp-settings)) - (set/rename-keys mb-to-smtp-settings)) - settings (cond-> settings - (string? (:port settings)) (update :port #(Long/parseLong ^String %)) - (string? (:security settings)) (update :security keyword)) + ;; the frontend has access to an obfuscated version of the password. Watch for whether it sent us a new password or + ;; the obfuscated version + (let [obfuscated? (and (:email-smtp-password settings) (email/email-smtp-password) + (= (:email-smtp-password settings) (setting/obfuscate-value (email/email-smtp-password)))) + settings (-> (cond-> settings + obfuscated? + (assoc :email-smtp-password (email/email-smtp-password))) + (select-keys (keys mb-to-smtp-settings)) + (set/rename-keys mb-to-smtp-settings)) + settings (cond-> settings + (string? (:port settings)) (update :port #(Long/parseLong ^String %)) + (string? (:security settings)) (update :security keyword)) response (email/test-smtp-connection settings)] (if-not (::email/error response) ;; test was good, save our settings - (assoc (setting/set-many! (set/rename-keys response (set/map-invert mb-to-smtp-settings))) - :with-corrections (let [[_ corrections] (data/diff settings response)] - (-> corrections - (set/rename-keys (set/map-invert mb-to-smtp-settings)) - humanize-email-corrections))) + (cond-> (assoc (setting/set-many! (set/rename-keys response (set/map-invert mb-to-smtp-settings))) + :with-corrections (let [[_ corrections] (data/diff settings response)] + (-> corrections + (set/rename-keys (set/map-invert mb-to-smtp-settings)) + humanize-email-corrections))) + obfuscated? (update :email-smtp-password setting/obfuscate-value)) ;; test failed, return response message {:status 400 :body (humanize-error-messages response)}))) diff --git a/test/metabase/api/email_test.clj b/test/metabase/api/email_test.clj index d55c7a02a2a9be67bc7441addc3390cad030aa38..b3f17e284f8580439adb5e7aa54c636bcaec6da9 100644 --- a/test/metabase/api/email_test.clj +++ b/test/metabase/api/email_test.clj @@ -1,5 +1,6 @@ (ns metabase.api.email-test - (:require [clojure.test :refer :all] + (:require [clojure.string :as str] + [clojure.test :refer :all] [metabase.api.email :as api.email] [metabase.email :as email] [metabase.models.setting :as setting] @@ -90,7 +91,33 @@ (is (= (if success? default-email-settings original-values) - (email-settings))))))))))) + (email-settings)))))))))) + (testing "Updating values with obfuscated password (#23919)" + (mt/with-temporary-setting-values [email-from-address "notifications@metabase.com" + email-from-name "Sender Name" + email-reply-to ["reply-to@metabase.com"] + email-smtp-host "www.test.com" + email-smtp-password "preexisting"] + (with-redefs [email/test-smtp-connection (fn [settings] + (let [obfuscated? (str/starts-with? (:pass settings) "****")] + (is (not obfuscated?) "We received an obfuscated password!") + (if obfuscated? + {::email/error (ex-info "Sent obfuscated password" {})} + settings)))] + (testing "If we don't change the password we don't see the password" + (let [payload (-> (email-settings) + ;; user changes one property + (assoc :email-from-name "notifications") + ;; the FE will have an obfuscated value + (update :email-smtp-password setting/obfuscate-value)) + response (mt/user-http-request :crowberto :put 200 "email" payload)] + (is (= (setting/obfuscate-value "preexisting") (:email-smtp-password response))))) + (testing "If we change the password we can receive the password" + (let [payload (-> (email-settings) + ;; user types in a new password + (assoc :email-smtp-password "new-password")) + response (mt/user-http-request :crowberto :put 200 "email" payload)] + (is (= "new-password" (:email-smtp-password response))))))))) (deftest clear-email-settings-test (testing "DELETE /api/email"