From 5e49508d62d97849900438ab519913279671fb88 Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Fri, 15 Jul 2022 13:40:06 -0500
Subject: [PATCH] Handle email password obfuscation (#23925)
The frontend receives obfuscated values for settings marked sensitive:
```clojure
(defsetting email-smtp-password
(deferred-tru "SMTP password.")
:sensitive? true)
```
and over the wire:
```javascript
{
"key": "email-smtp-password",
"value": "**********ig",
"is_env_setting": false,
"env_name": "MB_EMAIL_SMTP_PASSWORD",
"description": "SMTP password.",
"default": null
},
```
So the frontend form never has a valid password in it. When you edit
email settings, we check if those are valid, and if so, commit the
settings. But with the wrong password email settings are almost always
wrong. You have to re-type the email password just to change other
settings, like the friendly name, reply-to email address, etc.
So we recognize that we've received the obfuscated value, swap in the
real value for the email connection test, and then obfuscate the
password in the response. If we do not receive an obfuscated value, just
leave it alone and return the value anyways (they typed it in, so seems
safe to send it back).
Note that there is a function in models.setting called
`obfuscated-value?` that checks strings against a regex of
`#"^\*{10}.{2}$"`. This is used to never set settings to an obfuscated
value. But its a bit less sensitive than our purposes need. We have a
real value and an obfuscated value so we can check if the obfuscated
value is based on the real value. (obfuscate-value "password") ->
"**********rd" . Whereas we might recognize "**********AA" as the
obfuscated value if we reused that helper function.
NOTE this could be weird if anyone's password changes from "password" to
"**********rd" but the chances of that happening are miniscule.
Had thought to have the FE not send a value at all if it is unchanged
but this had some far-reaching implications that we don't want to tackle
so close to the release. There's an associated issue for LDAP settings
that is largely the same as this:
https://github.com/metabase/metabase/issues/16708 But it is not clear to
me if these are the only two places or if there could be others. Until
that research is done we can just accept this patch as is and come up
with a systematic approach in the future.
But it does seem only three settings are sensitive:
```clojure
setting=> (->> (vals @registered-settings)
(filter :sensitive?)
(map :name))
(:saml-keystore-password :email-smtp-password :ldap-password)
```
The direct setting apis won't write setting values which appear as
obfuscated, but we have other endpoints that take collections of
settings as a unit (for instance, the endpoint /api/email that takes all
of the email settings here to test if they work together).
---
src/metabase/api/email.clj | 29 ++++++++++++++++++-----------
test/metabase/api/email_test.clj | 31 +++++++++++++++++++++++++++++--
2 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/src/metabase/api/email.clj b/src/metabase/api/email.clj
index 55745dc22f2..32d3e62fc36 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 d55c7a02a2a..b3f17e284f8 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"
--
GitLab