From 977429c46fe0aa705ad861a868c6e0cc7c3478d9 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Thu, 7 Nov 2024 19:25:00 +0700
Subject: [PATCH] Email throttling config (#49477)
---
src/metabase/email.clj | 58 +++++++++++--
test/metabase/email_test.clj | 82 ++++++++++++++++++-
.../serialization_baseline/settings.yaml | 3 +-
3 files changed, 136 insertions(+), 7 deletions(-)
diff --git a/src/metabase/email.clj b/src/metabase/email.clj
index 4afd557915c..ec057bc5126 100644
--- a/src/metabase/email.clj
+++ b/src/metabase/email.clj
@@ -9,9 +9,11 @@
[metabase.util.malli.schema :as ms]
[metabase.util.retry :as retry]
[postal.core :as postal]
- [postal.support :refer [make-props]])
+ [postal.support :refer [make-props]]
+ [throttle.core :as throttle])
(:import
- (javax.mail Session)))
+ (javax.mail Session)
+ (throttle.core Throttler)))
(set! *warn-on-reflection* true)
@@ -95,12 +97,58 @@
(assert (#{:tls :ssl :none :starttls} (keyword new-value))))
(setting/set-value-of-type! :keyword :email-smtp-security new-value)))
+(defsetting email-max-recipients-per-second
+ (deferred-tru "The maximum number of recipients, summed across emails, that can be sent per second.
+ Note that the final email sent before reaching the limit is able to exceed it, if it has multiple recipients.")
+ :export? true
+ :type :integer
+ :visibility :settings-manager
+ :audit :getter)
+
+(defn- make-email-throttler
+ [rate-limit]
+ (throttle/make-throttler
+ :email
+ :attempt-ttl-ms 1000
+ :initial-delay-ms 1000
+ :attempts-threshold rate-limit))
+
+(defonce ^:private email-throttler (when-let [rate-limit (email-max-recipients-per-second)]
+ (make-email-throttler rate-limit)))
+
+(defn check-email-throttle
+ "Check if the email throttler is enabled and if so, throttle the email sending based on the total number of recipients.
+
+ We will allow multi-recipient emails to broach the limit, as long as the limit has not been reached yet.
+
+ We want two properties:
+ 1. All emails eventually get sent.
+ 2. Lowering the threshold must never cause more overflow."
+ [email]
+ (when email-throttler
+ (when-let [recipients (not-empty (into #{} (mapcat email) [:to :bcc]))]
+ (let [throttle-threshold (.attempts-threshold ^Throttler email-throttler)
+ check-one! #(throttle/check email-throttler true)]
+ (check-one!)
+ (try
+ (dotimes [_ (dec (count recipients))]
+ (throttle/check email-throttler true))
+ (catch Exception _e
+ (log/warn "Email throttling is enabled and the number of recipients exceeds the rate limit per second. Skip throttling."
+ {:email-subject (:subject email)
+ :recipients (count recipients)
+ :max-recipients throttle-threshold})))))))
+
;; ## PUBLIC INTERFACE
-(def ^{:arglists '([smtp-credentials email-details])} send-email!
+(defn send-email!
"Internal function used to send messages. Should take 2 args - a map of SMTP credentials, and a map of email details.
- Provided so you can swap this out with an \"inbox\" for test purposes."
- postal/send-message)
+ Provided so you can swap this out with an \"inbox\" for test purposes.
+
+ If email-rate-limit-per-second is set, this function will throttle the email sending based on the total number of recipients."
+ [smtp-credentials email-details]
+ (check-email-throttle email-details)
+ (postal/send-message smtp-credentials email-details))
(defsetting email-configured?
"Check if email is enabled and that the mandatory settings are configured."
diff --git a/test/metabase/email_test.clj b/test/metabase/email_test.clj
index 24d2ee23613..8f06ea6ae16 100644
--- a/test/metabase/email_test.clj
+++ b/test/metabase/email_test.clj
@@ -12,7 +12,9 @@
[metabase.util :as u :refer [prog1]]
[metabase.util.retry :as retry]
[metabase.util.retry-test :as rt]
- [postal.message :as message])
+ [postal.core :as postal]
+ [postal.message :as message]
+ [throttle.core :as throttle])
(:import
(java.io File)
(javax.activation MimeType)))
@@ -359,3 +361,81 @@
(is (re-find
#"(?s)Content-Disposition: attachment.+filename=.+this-is-quite-[\-\s?=0-9a-zA-Z]+-characters.csv"
(m/mapply email/send-message! params-with-problematic-file))))))))))
+
+(deftest throttle-test
+ (let [send-email (fn [recipients]
+ (with-redefs [postal/send-message (fn [& args] (last args))]
+ (email/send-email!
+ {}
+ (merge {:from "awesome@metabase.com"
+ :subject "101 Reasons to use Metabase"
+ :body "101. Metabase will make you a better person"}
+ recipients))))]
+ (tu/with-temporary-setting-values
+ [email-smtp-host "fake_smtp_host"
+ email-smtp-port 587]
+ (testing "throttle based on the number of recipients"
+ (testing "with 3 separate emails"
+ (with-redefs [email/email-throttler (#'email/make-email-throttler 3)]
+ (testing "ok if there is no recipient"
+ (is (some? (send-email {}))))
+ (is (some? (send-email {:to ["1@metabase.com"]})))
+ (is (some? (send-email {:bcc ["2@metabase.com"]})))
+ (is (some? (send-email {:to ["3@metabase.com"]})))
+ (is (thrown-with-msg?
+ Exception
+ #"Too many attempts!.*"
+ (send-email {:to ["4@metabase.com"]})))
+ (testing "still ok if there is no recipient"
+ (is (some? (send-email {})))))
+
+ (testing "with 1 small then 1 big event"
+ (with-redefs [email/email-throttler (#'email/make-email-throttler 3)]
+ (is (some? (send-email {:to ["1@metabase.com"]})))
+ (is (some? (send-email {:bcc ["2@metabase.com"]
+ :to ["3@metabase.com"]})))
+ (is (thrown-with-msg?
+ Exception
+ #"Too many attempts!.*"
+ (send-email {:to ["4@metabase.com"]})))))))
+
+ (testing "if an email has # of recipients greater than the limit"
+ (testing "we skip throttle check if we haven't reached the limit"
+ (with-redefs [email/email-throttler (#'email/make-email-throttler 3)]
+ (is (some? (send-email {:to ["1@metabase.com"]})))
+ ;; this one got through because we haven't reached the limit
+ (is (some? (send-email {:to ["2@metabase.com" "3@metabase.com"]
+ :bcc ["4@metabase.com" "5@metabase.com"]})))
+ (testing "senidng another will fail because we maxed-out the limit"
+ (is (thrown-with-msg?
+ Exception
+ #"Too many attempts!.*"
+ (send-email {:to ["6@metabase.com"]}))))))
+
+ (testing "still throttle if we already at limit"
+ (with-redefs [email/email-throttler (#'email/make-email-throttler 3)]
+ ;; mx otu the limit
+ (is (some? (send-email {:to ["1@metabase.com" "2@metabase.com" "3@metabase.com"]})))
+ (testing "but still max-out the limit"
+ (is (thrown-with-msg?
+ Exception
+ #"Too many attempts!.*"
+ (send-email {:to ["4@metabase.com" "5@metabase.com" "6@metabase.com" "7@metabase.com"]})))))))
+
+ (testing "keep retrying will eventually send the email"
+ (with-redefs [email/email-throttler (throttle/make-throttler
+ :email
+ :attempt-ttl-ms 100
+ :initial-delay-ms 100
+ :attempts-threshold 3)]
+ (is (some? (send-email {:to ["1@metabase.com" "2@metabase.com" "3@metabase.com"]})))
+ (is (thrown-with-msg?
+ Exception
+ #"Too many attempts!.*"
+ (send-email {:to ["4@metabase.com"]})))
+ (is (some? (u/poll {:thunk (fn [] (try (send-email {:to ["4@metabase.com"]})
+ (catch Exception _
+ nil)))
+ :done? some?
+ :timeout-ms 200
+ :interval-ms 10}))))))))
diff --git a/test_resources/serialization_baseline/settings.yaml b/test_resources/serialization_baseline/settings.yaml
index dcfd3399b55..da7e78b2cd2 100644
--- a/test_resources/serialization_baseline/settings.yaml
+++ b/test_resources/serialization_baseline/settings.yaml
@@ -1,4 +1,5 @@
aggregated-query-row-limit: null
+allowed-iframe-hosts: null
application-colors: null
application-favicon-url: null
application-font: null
@@ -13,6 +14,7 @@ custom-formatting: null
custom-geojson: null
custom-geojson-enabled: null
default-maps-enabled: null
+email-max-recipients-per-second: null
embedding-homepage: null
enable-embedding: null
enable-nested-queries: null
@@ -52,4 +54,3 @@ subscription-allowed-domains: null
synchronous-batch-updates: null
unaggregated-query-row-limit: null
update-channel: null
-allowed-iframe-hosts: null
--
GitLab