From ee134cf91d6457eb3ecb8fb39c2c3f6d829315e3 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Wed, 29 Sep 2021 12:20:33 -0700
Subject: [PATCH] Enforce allowed domains when sending test Pulses (#18122)
---
.../advanced_config/models/pulse_channel.clj | 3 +-
.../advanced_config/api/pulse_test.clj | 55 +++++++++++++++++++
src/metabase/api/pulse.clj | 6 +-
src/metabase/models/pulse_channel.clj | 2 +-
src/metabase/pulse.clj | 2 +-
test/metabase/api/pulse_test.clj | 33 +++++++++--
6 files changed, 92 insertions(+), 9 deletions(-)
create mode 100644 enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj
diff --git a/enterprise/backend/src/metabase_enterprise/advanced_config/models/pulse_channel.clj b/enterprise/backend/src/metabase_enterprise/advanced_config/models/pulse_channel.clj
index 754c819686d..58f544822b3 100644
--- a/enterprise/backend/src/metabase_enterprise/advanced_config/models/pulse_channel.clj
+++ b/enterprise/backend/src/metabase_enterprise/advanced_config/models/pulse_channel.clj
@@ -39,4 +39,5 @@
(pr-str domain)
(str/join ", " allowed-domains))
{:email email
- :allowed-domains allowed-domains})))))))
+ :allowed-domains allowed-domains
+ :status-code 403})))))))
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
new file mode 100644
index 00000000000..e921d47231a
--- /dev/null
+++ b/enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj
@@ -0,0 +1,55 @@
+(ns metabase-enterprise.advanced-config.api.pulse-test
+ (:require [clojure.test :refer :all]
+ [metabase.models :refer [Card]]
+ [metabase.public-settings.premium-features-test :as premium-features-test]
+ [metabase.test :as mt]
+ [metabase.util :as u]))
+
+(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")))))))))))
diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj
index 00edc389ca0..0a7a26cc3cc 100644
--- a/src/metabase/api/pulse.clj
+++ b/src/metabase/api/pulse.clj
@@ -10,7 +10,7 @@
[metabase.models.dashboard :refer [Dashboard]]
[metabase.models.interface :as mi]
[metabase.models.pulse :as pulse :refer [Pulse]]
- [metabase.models.pulse-channel :refer [channel-types PulseChannel]]
+ [metabase.models.pulse-channel :as pulse-channel :refer [channel-types PulseChannel]]
[metabase.models.pulse-channel-recipient :refer [PulseChannelRecipient]]
[metabase.plugins.classloader :as classloader]
[metabase.pulse :as p]
@@ -205,6 +205,10 @@
collection_position (s/maybe su/IntGreaterThanZero)
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)))
(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 38545d2b117..55030a382da 100644
--- a/src/metabase/models/pulse_channel.clj
+++ b/src/metabase/models/pulse_channel.clj
@@ -140,7 +140,7 @@
(resolve 'metabase-enterprise.advanced-config.models.pulse-channel/validate-email-domains))
(constantly nil)))
-(defn- validate-email-domains
+(defn validate-email-domains
"For channels that are being sent to raw email addresses: check that the domains in the emails are allowed by
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
diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj
index c812077531d..8c510324247 100644
--- a/src/metabase/pulse.clj
+++ b/src/metabase/pulse.clj
@@ -277,7 +277,7 @@
[(alert-or-pulse pulse) (keyword channel_type)]))
(defmethod notification [:pulse :email]
- [{pulse-id :id, pulse-name :name, dashboard-id :dashboard_id, :as pulse} results {:keys [recipients] :as channel}]
+ [{pulse-id :id, pulse-name :name, dashboard-id :dashboard_id, :as pulse} results {:keys [recipients], :as channel}]
(log/debug (u/format-color 'cyan (trs "Sending Pulse ({0}: {1}) with {2} Cards via email"
pulse-id (pr-str pulse-name) (count results))))
(let [email-recipients (filterv u/email? (map :email recipients))
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index 0c4395b3201..2f42cc6846a 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -8,6 +8,7 @@
[metabase.models :refer [Card Collection Dashboard Pulse PulseCard PulseChannel PulseChannelRecipient]]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
+ [metabase.models.pulse-channel :as pulse-channel]
[metabase.models.pulse-test :as pulse-test]
[metabase.pulse.render.png :as png]
[metabase.server.middleware.util :as middleware.u]
@@ -813,12 +814,9 @@
(with-pulses-in-readable-collection [pulse-id]
(mt/user-http-request :rasta :get 404 (str "pulse/" pulse-id)))))))))
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | POST /api/pulse/test |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
(deftest send-test-pulse-test
+ ;; see [[metabase-enterprise.advanced-config.api.pulse-test/test-pulse-endpoint-should-respect-email-domain-allow-list-test]]
+ ;; for additional EE-specific tests
(testing "POST /api/pulse/test"
(mt/with-non-admin-groups-no-root-collection-perms
(mt/with-fake-inbox
@@ -845,6 +843,31 @@
:body {"Daily Sad Toucans" true}})
(mt/regex-email-bodies #"Daily Sad Toucans"))))))))))
+(deftest send-test-pulse-validate-emails-test
+ (testing (str "POST /api/pulse/test should call " `pulse-channel/validate-email-domains)
+ (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")))))))))
+
;; 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
;; of the same format that the pulse GET returns
--
GitLab