Skip to content
Snippets Groups Projects
Unverified Commit 8f4d867b authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

Fix Dashboard Subscription email validation (second pass) (#18237) 

* Fix Dashboard Subscription email validation (second pass)

* Always validate Pulse channels in test endpoint
parent a7e1d254
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj
+ 51
− 45
View file @ 8f4d867b
......@@ -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")))))))))))))
src/metabase/api/pulse.clj
+ 2
− 3
View file @ 8f4d867b
......@@ -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})
......
src/metabase/models/pulse_channel.clj
+ 30
− 2
View file @ 8f4d867b
......@@ -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
......
test/metabase/api/pulse_test.clj
+ 24
− 19
View file @ 8f4d867b
......@@ -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
......
test/metabase/models/pulse_channel_test.clj
+ 24
− 0
View file @ 8f4d867b
......@@ -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)}]}))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment