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

Fix login history emails (#15606) 

* Much simpler impl

* Remove unused var

* Java 11+ test fixes :wrench:
parent 6cb3341c
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/email/messages.clj
+ 3
− 5
View file @ 9990c81f
......@@ -187,12 +187,10 @@
(defn send-login-from-new-device-email!
"Format and send an email informing the user that this is the first time we've seen a login from this device. Expects
login history infomation as returned by `metabase.models.login-history/human-friendly-infos`."
[{user-id :user_id, :keys [timestamp timezone], :as login-history}]
[{user-id :user_id, :keys [timestamp], :as login-history}]
(let [user-info (db/select-one ['User [:first_name :first-name] :email :locale] :id user-id)
timestamp (cond-> timestamp
timezone (u.date/with-time-zone-same-instant (t/zone-id timezone))
true (u.date/format-human-readable (or (:locale user-info)
(i18n/site-locale))))
user-locale (or (:locale user-info) (i18n/site-locale))
timestamp (u.date/format-human-readable timestamp user-locale)
context (merge (common-context)
{:first-name (:first-name user-info)
:device (:device_description login-history)
......
src/metabase/models/login_history.clj
+ 2
− 1
View file @ 9990c81f
(ns metabase.models.login-history
(:require [clojure.tools.logging :as log]
[java-time :as t]
[metabase.email.messages :as email.messages]
[metabase.models.setting :refer [defsetting]]
[metabase.server.request.util :as request.u]
......@@ -30,7 +31,7 @@
:timezone (timezone-display-name timezone))
(update :timestamp (fn [timestamp]
(if (and timestamp timezone)
(u.date/with-time-zone-same-instant timestamp timezone)
(t/zoned-date-time (u.date/with-time-zone-same-instant timestamp timezone) timezone)
timestamp)))
(update :device_description request.u/describe-user-agent)))))
......
src/metabase/util/date_2.clj
+ 13
− 4
View file @ 9990c81f
......@@ -13,7 +13,7 @@
[schema.core :as s])
(:import [java.time DayOfWeek Duration Instant LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime Period
ZonedDateTime]
[java.time.format DateTimeFormatter DateTimeFormatterBuilder FormatStyle]
[java.time.format DateTimeFormatter DateTimeFormatterBuilder FormatStyle TextStyle]
[java.time.temporal Temporal TemporalAdjuster WeekFields]
org.threeten.extra.PeriodDuration))
......@@ -106,13 +106,22 @@
(.toFormatter builder))
OffsetTime (let [builder (doto (DateTimeFormatterBuilder.)
(.append (DateTimeFormatter/ofLocalizedTime FormatStyle/MEDIUM))
(.appendPattern " (OOOO)"))]
(.appendLiteral " (")
(.appendLocalizedOffset TextStyle/FULL)
(.appendLiteral ")"))]
(.toFormatter builder))
OffsetDateTime (let [builder (doto (DateTimeFormatterBuilder.)
(.appendLocalized FormatStyle/LONG FormatStyle/MEDIUM)
(.appendPattern " (OOOO)"))]
(.appendLiteral " (")
(.appendLocalizedOffset TextStyle/FULL)
(.appendLiteral ")"))]
(.toFormatter builder))
ZonedDateTime (DateTimeFormatter/ofLocalizedDateTime FormatStyle/LONG)})
ZonedDateTime (let [builder (doto (DateTimeFormatterBuilder.)
(.appendLocalized FormatStyle/LONG FormatStyle/MEDIUM)
(.appendLiteral " (")
(.appendZoneText TextStyle/FULL)
(.appendLiteral ")"))]
(.toFormatter builder))})
(defn format-human-readable
"Format a temporal value `t` in a human-friendly way for `locale` (by default, the current User's locale).
......
test/metabase/models/login_history_test.clj
+ 58
− 45
View file @ 9990c81f
......@@ -2,11 +2,13 @@
(:require [clojure.string :as str]
[clojure.test :refer :all]
[environ.core :as env]
[metabase.db :as mdb]
[java-time :as t]
[metabase.models :refer [LoginHistory User]]
[metabase.models.login-history :as login-history]
[metabase.models.setting.cache :as setting.cache]
[metabase.server.request.util :as request.u]
[metabase.test :as mt]
[metabase.util.date-2 :as u.date]
[metabase.util.schema :as su]
[schema.core :as s]))
......@@ -34,51 +36,62 @@
(#'login-history/first-login-ever? history-1))))))))))
(deftest send-email-on-first-login-from-new-device-test
(mt/with-temp User [{user-id :id, email :email, first-name :first_name}]
(let [device (str (java.util.UUID/randomUUID))]
(testing "send email on first login from *new* device (but not first login ever)"
(mt/with-fake-inbox
(mt/with-temp* [LoginHistory [_ {:user_id user-id, :device_id (str (java.util.UUID/randomUUID))}]
LoginHistory [_ {:user_id user-id, :device_id device, :timestamp #t "2021-04-02T15:52:00-07:00[US/Pacific]"}]]
(is (schema= {(s/eq email)
[{:from su/Email
:to (s/eq [email])
:subject (s/eq (format "We've Noticed a New Metabase Login, %s" first-name))
:body [(s/one {:type (s/eq "text/html; charset=utf-8")
:content s/Str}
"HTML body")]}]}
@mt/inbox))
(let [message (-> @mt/inbox (get email) first :body first :content)]
(testing (format "\nMessage = %s" (pr-str message))
(is (string? message))
(when (string? message)
(is (str/includes? message "We've noticed a new login on your Metabase account."))
(is (str/includes? message "We noticed a login on your Metabase account from a new device."))
(is (str/includes? message "Browser (Chrome/Windows) - Unknown location"))
(if (= (mdb/db-type) :h2)
(str/includes? message "April 2 3:52 PM (GMT-07:00)")
(str/includes? message "April 2 10:52 PM (GMT)")))))
(testing "User should get an email the first time they log in from a new device (#14313, #15603)"
(mt/with-temp User [{user-id :id, email :email, first-name :first_name}]
(let [device (str (java.util.UUID/randomUUID))]
(testing "send email on first login from *new* device (but not first login ever)"
(mt/with-fake-inbox
;; mock out the IP address geocoding function so we can make sure it handles timezones like PST correctly
;; (#15603)
(with-redefs [request.u/geocode-ip-addresses (fn [ip-addresses]
(into {} (for [ip-address ip-addresses]
[ip-address
{:description "San Francisco, California, United States"
:timezone (t/zone-id "America/Los_Angeles")}])))]
(mt/with-temp* [LoginHistory [_ {:user_id user-id
:device_id (str (java.util.UUID/randomUUID))}]
LoginHistory [_ {:user_id user-id
:device_id device
:timestamp #t "2021-04-02T15:52:00-07:00[US/Pacific]"}]]
(testing "don't send email on subsequent login from same device"
(mt/reset-inbox!)
(mt/with-temp LoginHistory [_ {:user_id user-id, :device_id device}]
(is (= {}
@mt/inbox)))))))))
(is (schema= {(s/eq email)
[{:from su/Email
:to (s/eq [email])
:subject (s/eq (format "We've Noticed a New Metabase Login, %s" first-name))
:body [(s/one {:type (s/eq "text/html; charset=utf-8")
:content s/Str}
"HTML body")]}]}
@mt/inbox))
(let [message (-> @mt/inbox (get email) first :body first :content)]
(testing (format "\nMessage = %s" (pr-str message))
(is (string? message))
(when (string? message)
(doseq [expected-str ["We've noticed a new login on your Metabase account."
"We noticed a login on your Metabase account from a new device."
"Browser (Chrome/Windows) - San Francisco, California, United States"
;; `format-human-readable` has slightly different output on different JVMs
(u.date/format-human-readable #t "2021-04-02T15:52:00-07:00[US/Pacific]")]]
(is (str/includes? message expected-str))))))
(testing "don't send email on subsequent login from same device"
(mt/reset-inbox!)
(mt/with-temp LoginHistory [_ {:user_id user-id, :device_id device}]
(is (= {}
@mt/inbox)))))))))))
(testing "don't send email if the setting is disabled by setting MB_SEND_EMAIL_ON_FIRST_LOGIN_FROM_NEW_DEVICE=FALSE"
(mt/with-temp User [{user-id :id, email :email, first-name :first_name}]
(testing "send email on first login from new device"
(try
(mt/with-fake-inbox
;; can't use `mt/with-temporary-setting-values` here because it's a read-only setting
(with-redefs [env/env (assoc env/env :mb-send-email-on-first-login-from-new-device "FALSE")]
;; flush the Setting cache so it picks up the env var value for the `send-email-on-first-login-from-new-device` setting
(setting.cache/restore-cache!)
(mt/with-temp* [LoginHistory [_ {:user_id user-id, :device_id (str (java.util.UUID/randomUUID))}]
LoginHistory [_ {:user_id user-id, :device_id (str (java.util.UUID/randomUUID))}]]
(is (= {}
@mt/inbox)))))
(finally
;; flush the cache again so the original value of `send-email-on-first-login-from-new-device` gets
;; restored
(setting.cache/restore-cache!)))))))
(try
(mt/with-fake-inbox
;; can't use `mt/with-temporary-setting-values` here because it's a read-only setting
(with-redefs [env/env (assoc env/env :mb-send-email-on-first-login-from-new-device "FALSE")]
;; flush the Setting cache so it picks up the env var value for the `send-email-on-first-login-from-new-device` setting
(setting.cache/restore-cache!)
(mt/with-temp* [LoginHistory [_ {:user_id user-id, :device_id (str (java.util.UUID/randomUUID))}]
LoginHistory [_ {:user_id user-id, :device_id (str (java.util.UUID/randomUUID))}]]
(is (= {}
@mt/inbox)))))
(finally
;; flush the cache again so the original value of `send-email-on-first-login-from-new-device` gets
;; restored
(setting.cache/restore-cache!))))))
test/metabase/util/date_2_test.clj
+ 6
− 5
View file @ 9990c81f
......@@ -158,11 +158,12 @@
;; strings are localized slightly differently on different JVMs. For places where there are multiple possible
;; correct results, we'll use a set with all the possibilities below and check membership
(doseq [[t expected] {#t "2021-04-02T14:42:09.524392-07:00[US/Pacific]" ; ZonedDateTime
{:en-US #{"April 2, 2021 2:42:09 PM PDT"
"April 2, 2021 at 2:42:09 PM PDT"}
:es-MX #{"2 de abril de 2021 02:42:09 PM PDT"
"2 de abril de 2021, 14:42:09 PDT"
"2 de abril de 2021 a las 14:42:09 PDT"}}
{:en-US #{"April 2, 2021 2:42:09 PM (Pacific Daylight Time)"
"April 2, 2021, 2:42:09 PM (Pacific Daylight Time)"}
:es-MX #{"2 de abril de 2021 02:42:09 PM (Hora de verano del Pacífico)"
"2 de abril de 2021 14:42:09 (Hora de verano del Pacífico)"
"2 de abril de 2021 14:42:09 (hora de verano del Pacífico)"
"2 de abril de 2021, 14:42:09 (Hora de verano del Pacífico)"}}
#t "2021-04-02T14:42:09.524392-07:00" ; OffsetDateTime
{:en-US #{"April 2, 2021 2:42:09 PM (GMT-07:00)"
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment