From 8f4d867bc486221490d92f650f14e9d14e50c666 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Tue, 5 Oct 2021 13:10:52 -0700
Subject: [PATCH] Fix Dashboard Subscription email validation (second pass)
(#18237)
* Fix Dashboard Subscription email validation (second pass)
* Always validate Pulse channels in test endpoint
---
.../advanced_config/api/pulse_test.clj | 96 ++++++++++---------
src/metabase/api/pulse.clj | 5 +-
src/metabase/models/pulse_channel.clj | 32 ++++++-
test/metabase/api/pulse_test.clj | 43 +++++----
test/metabase/models/pulse_channel_test.clj | 24 +++++
5 files changed, 131 insertions(+), 69 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj
index e921d47231a..47b0c02c02c 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj
@@ -8,48 +8,54 @@
(deftest test-pulse-endpoint-should-respect-email-domain-allow-list-test
(testing "POST /api/pulse/test"
(mt/with-temp Card [card {:dataset_query (mt/mbql-query venues)}]
- (letfn [(send! [expected-status-code]
- (let [pulse-name (mt/random-name)]
- (mt/with-fake-inbox
- {:response (mt/user-http-request
- :rasta :post expected-status-code "pulse/test"
- {:name pulse-name
- :cards [{:id (u/the-id card)
- :include_csv false
- :include_xls false
- :dashboard_card_id nil}]
- :channels [{:enabled true
- :channel_type "email"
- :schedule_type "daily"
- :schedule_hour 12
- :schedule_day nil
- :details {:emails ["test@metabase.com"]}}]
- :skip_if_empty false})
- :recipients (set (keys (mt/regex-email-bodies (re-pattern pulse-name))))})))]
- (testing "allowed email -- should pass"
- (mt/with-temporary-setting-values [subscription-allowed-domains "metabase.com"]
- (premium-features-test/with-premium-features #{:advanced-config}
- (let [{:keys [response recipients]} (send! 200)]
- (is (= {:ok true}
- response))
- (is (contains? recipients "test@metabase.com"))))
- (testing "No :advanced-config token"
- (premium-features-test/with-premium-features #{}
- (let [{:keys [response recipients]} (send! 200)]
- (is (= {:ok true}
- response))
- (is (contains? recipients "test@metabase.com")))))))
- (testing "disallowed email"
- (mt/with-temporary-setting-values [subscription-allowed-domains "example.com"]
- (testing "should fail when :advanced-config is enabled"
- (premium-features-test/with-premium-features #{:advanced-config}
- (let [{:keys [response recipients]} (send! 403)]
- (is (= "You cannot create new subscriptions for the domain \"metabase.com\". Allowed domains are: example.com"
- (:message response)))
- (is (not (contains? recipients "test@metabase.com"))))))
- (testing "No :advanced-config token -- should still pass"
- (premium-features-test/with-premium-features #{}
- (let [{:keys [response recipients]} (send! 200)]
- (is (= {:ok true}
- response))
- (is (contains? recipients "test@metabase.com")))))))))))
+ ;; make sure we validate raw emails whether they're part of `:details` or part of `:recipients` -- we
+ ;; technically allow either right now
+ (doseq [channel [{:details {:emails ["test@metabase.com"]}}
+ {:recipients [{:email "test@metabase.com"}]
+ :details {}}]]
+ (testing (format "\nChannel = %s\n" (u/pprint-to-str channel))
+ (letfn [(send! [expected-status-code]
+ (let [pulse-name (mt/random-name)]
+ (mt/with-fake-inbox
+ {:response (mt/user-http-request
+ :rasta :post expected-status-code "pulse/test"
+ {:name pulse-name
+ :cards [{:id (u/the-id card)
+ :include_csv false
+ :include_xls false
+ :dashboard_card_id nil}]
+ :channels [(merge {:enabled true
+ :channel_type "email"
+ :schedule_type "daily"
+ :schedule_hour 12
+ :schedule_day nil}
+ channel)]
+ :skip_if_empty false})
+ :recipients (set (keys (mt/regex-email-bodies (re-pattern pulse-name))))})))]
+ (testing "allowed email -- should pass"
+ (mt/with-temporary-setting-values [subscription-allowed-domains "metabase.com"]
+ (premium-features-test/with-premium-features #{:advanced-config}
+ (let [{:keys [response recipients]} (send! 200)]
+ (is (= {:ok true}
+ response))
+ (is (contains? recipients "test@metabase.com"))))
+ (testing "No :advanced-config token"
+ (premium-features-test/with-premium-features #{}
+ (let [{:keys [response recipients]} (send! 200)]
+ (is (= {:ok true}
+ response))
+ (is (contains? recipients "test@metabase.com")))))))
+ (testing "disallowed email"
+ (mt/with-temporary-setting-values [subscription-allowed-domains "example.com"]
+ (testing "should fail when :advanced-config is enabled"
+ (premium-features-test/with-premium-features #{:advanced-config}
+ (let [{:keys [response recipients]} (send! 403)]
+ (is (= "You cannot create new subscriptions for the domain \"metabase.com\". Allowed domains are: example.com"
+ (:message response)))
+ (is (not (contains? recipients "test@metabase.com"))))))
+ (testing "No :advanced-config token -- should still pass"
+ (premium-features-test/with-premium-features #{}
+ (let [{:keys [response recipients]} (send! 200)]
+ (is (= {:ok true}
+ response))
+ (is (contains? recipients "test@metabase.com")))))))))))))
diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj
index 0a7a26cc3cc..f0ac21013c3 100644
--- a/src/metabase/api/pulse.clj
+++ b/src/metabase/api/pulse.clj
@@ -206,9 +206,8 @@
dashboard_id (s/maybe su/IntGreaterThanZero)}
(check-card-read-permissions cards)
;; make sure any email addresses that are specified are allowed before sending the test Pulse.
- (doseq [{{:keys [emails]} :details, :as channel} channels]
- (when (seq emails)
- (pulse-channel/validate-email-domains channel)))
+ (doseq [channel channels]
+ (pulse-channel/validate-email-domains channel))
(p/send-pulse! (assoc body :creator_id api/*current-user-id*))
{:ok true})
diff --git a/src/metabase/models/pulse_channel.clj b/src/metabase/models/pulse_channel.clj
index 55030a382da..a33ebfd8e1b 100644
--- a/src/metabase/models/pulse_channel.clj
+++ b/src/metabase/models/pulse_channel.clj
@@ -7,6 +7,7 @@
[metabase.models.user :as user :refer [User]]
[metabase.plugins.classloader :as classloader]
[metabase.util :as u]
+ [metabase.util.i18n :refer [tru]]
[schema.core :as s]
[toucan.db :as db]
[toucan.models :as models]))
@@ -145,9 +146,36 @@
the [[metabase-enterprise.advanced-config.models.pulse-channel/subscription-allowed-domains]] Setting, if set. This
will no-op if `subscription-allowed-domains` is unset or if we do not have a premium token with the
`:advanced-config` feature."
- [{{:keys [emails]} :details, :as pulse-channel}]
+ [{{:keys [emails]} :details, :keys [recipients], :as pulse-channel}]
+ ;; Raw email addresses can be in either `[:details :emails]` or in `:recipients`, depending on who is invoking this
+ ;; function. Make sure we handle both situations.
+ ;;
+ ;; {:details {:emails [\"email@example.com\" ...]}}
+ ;;
+ ;; The Dashboard Subscription FE currently sends raw email address recipients in this format:
+ ;;
+ ;; {:recipients [{:email \"email@example.com\"} ...]}
+ ;;
(u/prog1 pulse-channel
- (validate-email-domains* emails)))
+ (let [raw-email-recipients (remove :id recipients)
+ user-recipients (filter :id recipients)
+ emails (concat emails (map :email raw-email-recipients))]
+ (validate-email-domains* emails)
+ ;; validate User `:id` & `:email` match up for User recipients. This is mostly to make sure people don't try to
+ ;; be sneaky and pass in a valid User ID but different email so they can send test Pulses out to arbitrary email
+ ;; addresses
+ (when-let [user-ids (not-empty (into #{} (comp (filter some?) (map :id)) user-recipients))]
+ (let [user-id->email (db/select-id->field :email User, :id [:in user-ids])]
+ (doseq [{:keys [id email]} user-recipients
+ :let [correct-email (get user-id->email id)]]
+ (when-not correct-email
+ (throw (ex-info (tru "User {0} does not exist." id)
+ {:status-code 404})))
+ ;; only validate the email address if it was explicitly specified, which is not explicitly required.
+ (when (and email
+ (not= email correct-email))
+ (throw (ex-info (tru "Wrong email address for User {0}." id)
+ {:status-code 403})))))))))
(u/strict-extend (class PulseChannel)
models/IModel
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index 2f42cc6846a..8c29130c785 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -848,25 +848,30 @@
(mt/with-temp Card [card {:dataset_query (mt/mbql-query venues)}]
(with-redefs [pulse-channel/validate-email-domains (fn [& _]
(throw (ex-info "Nope!" {:status-code 403})))]
- (let [pulse-name (mt/random-name)]
- (mt/with-fake-inbox
- (is (= "Nope!"
- (mt/user-http-request
- :rasta :post 403 "pulse/test"
- {:name pulse-name
- :cards [{:id (u/the-id card)
- :include_csv false
- :include_xls false
- :dashboard_card_id nil}]
- :channels [{:enabled true
- :channel_type "email"
- :schedule_type "daily"
- :schedule_hour 12
- :schedule_day nil
- :details {:emails ["test@metabase.com"]}}]
- :skip_if_empty false})))
- (is (not (contains? (set (keys (mt/regex-email-bodies (re-pattern pulse-name))))
- "test@metabase.com")))))))))
+ ;; make sure we validate raw emails whether they're part of `:details` or part of `:recipients` -- we
+ ;; technically allow either right now
+ (doseq [channel [{:details {:emails ["test@metabase.com"]}}
+ {:recipients [{:email "test@metabase.com"}]
+ :details {}}]
+ :let [pulse-name (mt/random-name)
+ request-body {:name pulse-name
+ :cards [{:id (u/the-id card)
+ :include_csv false
+ :include_xls false
+ :dashboard_card_id nil}]
+ :channels [(assoc channel
+ :enabled true
+ :channel_type "email"
+ :schedule_type "daily"
+ :schedule_hour 12
+ :schedule_day nil)]
+ :skip_if_empty false}]]
+ (testing (format "\nchannel =\n%s" (u/pprint-to-str channel))
+ (mt/with-fake-inbox
+ (is (= "Nope!"
+ (mt/user-http-request :rasta :post 403 "pulse/test" request-body)))
+ (is (not (contains? (set (keys (mt/regex-email-bodies (re-pattern pulse-name))))
+ "test@metabase.com"))))))))))
;; This test follows a flow that the user/UI would follow by first creating a pulse, then making a small change to
;; that pulse and testing it. The primary purpose of this test is to ensure tha the pulse/test endpoint accepts data
diff --git a/test/metabase/models/pulse_channel_test.clj b/test/metabase/models/pulse_channel_test.clj
index 893bc8081ed..afdf38a7356 100644
--- a/test/metabase/models/pulse_channel_test.clj
+++ b/test/metabase/models/pulse_channel_test.clj
@@ -6,6 +6,7 @@
[metabase.models.pulse-channel-recipient :as pcr :refer [PulseChannelRecipient]]
[metabase.models.user :refer [User]]
[metabase.test :as mt]
+ [metabase.util :as u]
[toucan.db :as db]
[toucan.hydrate :refer [hydrate]]))
@@ -415,3 +416,26 @@
:last_name "Toucan"
:common_name "Rasta Toucan"}]))
(:recipients (hydrate channel :recipients)))))))
+
+(deftest validate-email-domains-check-user-ids-match-emails
+ (testing `pc/validate-email-domains
+ (testing "should check that User `:id` and `:email`s match for User `:recipients`"
+ (let [input {:recipients [{:email "rasta@metabase.com"
+ :id (mt/user->id :rasta)}]}]
+ (is (= input
+ (pc/validate-email-domains input))))
+ (testing "Throw Exception if User does not exist"
+ ;; should validate even if `:email` isn't specified
+ (doseq [input [{:id Integer/MAX_VALUE}
+ {:email "rasta@example.com"
+ :id Integer/MAX_VALUE}]]
+ (testing (format "\ninput = %s" (u/pprint-to-str input))
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"User [\d,]+ does not exist"
+ (pc/validate-email-domains {:recipients [input]}))))))
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"Wrong email address for User [\d,]+"
+ (pc/validate-email-domains {:recipients [{:email "rasta@example.com"
+ :id (mt/user->id :rasta)}]}))))))
--
GitLab