Enforce allowed domains when sending test Pulses (#18122)

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})))))))

enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj

+(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")))))))))))

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})
 

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

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))

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