Skip to content
Permalink
Browse files Browse the repository at this point in the history
Refactor password reset login for SSO users (#25819) (#25826)
* draft fix

* test

* revert change to email testing helper to fix tests

Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>
  • Loading branch information
metabase-bot[bot] and noahmoss committed Oct 6, 2022
1 parent 057e2d6 commit edadf73
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 42 deletions.
21 changes: 15 additions & 6 deletions src/metabase/api/session.clj
Expand Up @@ -199,12 +199,21 @@
(defn- forgot-password-impl
[email]
(future
(when-let [{user-id :id, google-auth? :google_auth, is-active? :is_active}
(db/select-one [User :id :google_auth :is_active] :%lower.email (u/lower-case-en email))]
(let [reset-token (user/set-password-reset-token! user-id)
password-reset-url (str (public-settings/site-url) "/auth/reset_password/" reset-token)]
(log/info password-reset-url)
(messages/send-password-reset-email! email google-auth? password-reset-url is-active?)))))
(when-let [{user-id :id
google-auth? :google_auth
ldap-auth? :ldap_auth
sso-source :sso_source
is-active? :is_active}
(db/select-one [User :id :google_auth :ldap_auth :sso_source :is_active]
:%lower.email
(u/lower-case-en email))]
(if (or google-auth? ldap-auth? sso-source)
;; If user uses any SSO method to log in, no need to generate a reset token
(messages/send-password-reset-email! email google-auth? (boolean (or ldap-auth? sso-source)) nil is-active?)
(let [reset-token (user/set-password-reset-token! user-id)
password-reset-url (str (public-settings/site-url) "/auth/reset_password/" reset-token)]
(log/info password-reset-url)
(messages/send-password-reset-email! email false false password-reset-url is-active?))))))

(api/defendpoint POST "/forgot_password"
"Send a reset email when user has forgotten their password."
Expand Down
27 changes: 14 additions & 13 deletions src/metabase/email/messages.clj
Expand Up @@ -56,12 +56,11 @@
(cond
(= url "app/assets/img/logo.svg") "http://static.metabase.com/email_logo.png"

:else nil
:else nil)))
;; NOTE: disabling whitelabeled URLs for now since some email clients don't render them correctly
;; We need to extract them and embed as attachments like we do in metabase.pulse.render.image-bundle
;; (data-uri-svg? url) (themed-image-url url color)
;; :else url
)))

(defn- icon-bundle
[icon-name]
Expand Down Expand Up @@ -172,15 +171,17 @@

(defn send-password-reset-email!
"Format and send an email informing the user how to reset their password."
[email google-auth? password-reset-url is-active?]
[email google-auth? non-google-sso? password-reset-url is-active?]
{:pre [(m/boolean? google-auth?)
(m/boolean? non-google-sso?)
(u/email? email)
(string? password-reset-url)]}
((some-fn string? nil?) password-reset-url)]}
(let [message-body (stencil/render-file
"metabase/email/password_reset"
(merge (common-context)
{:emailType "password_reset"
:sso google-auth?
:google google-auth?
:nonGoogleSSO non-google-sso?
:passwordResetUrl password-reset-url
:logoHeader true
:isActive is-active?
Expand Down Expand Up @@ -497,14 +498,14 @@
filters)
rows (partition 2 2 nil cells)]
(html
[:table {:style (style/style {:table-layout :fixed
:border-collapse :collapse
:cellpadding "0"
:cellspacing "0"
:width "100%"
:font-size "12px"
:font-weight 700
:margin-top "8px"})}
[:table {:style (style/style {:table-layout :fixed
:border-collapse :collapse
:cellpadding "0"
:cellspacing "0"
:width "100%"
:font-size "12px"
:font-weight 700
:margin-top "8px"})}
(for [row rows]
[:tr {} row])])))

Expand Down
43 changes: 25 additions & 18 deletions src/metabase/email/password_reset.mustache
@@ -1,24 +1,31 @@
{{> metabase/email/_header }}
<div>
{{#sso}}
{{#google}}
<p>You're using Google to log in to {{applicationName}}, so you don't have a password. You can log in to {{applicationName}} by clicking "Sign in with Google"</p>
<a href="{{siteUrl}}">Go to {{applicationName}}</a>
{{/sso}}
{{^sso}}
{{#isActive}}
<div style="text-align: center">
<p>Click the button below to reset the password for your {{applicationName}} account at {{siteUrl}}.</p>
<a style="display: inline-block; box-sizing: border-box; font-size: 18px; padding: 8px 22px; cursor: pointer; text-decoration: none; border-radius: 4px; background-color: #4990E2; border-color: #4990E2; color: #fff;" href="{{passwordResetUrl}}">Reset password</a>
<p style="padding-top: 2em; font-size: small;">Didn't request this password reset? It's safe to ignore it.</p>
</div>
{{/isActive}}
{{^isActive}}
<p>Someone requested a password reset for your {{applicationName}} account at {{siteUrl}}, but your account
has been deactivated. Contact an administrator for further assistance.</p>
{{#adminEmailSet}}
<a href="mailto:{{adminEmail}}">Contact your administrator</a>
{{/adminEmailSet}}
{{/isActive}}
{{/sso}}
{{/google}}
{{^google}}
{{#nonGoogleSSO}}
<p>We can't reset your password becase you're using single sign-on to log in to {{applicationName}}. Use the
"Sign in with SSO" button on the log in page. To change your password, you'll need to contact an administrator.</p>
<a href="{{siteUrl}}">Go to {{applicationName}}</a>
{{/nonGoogleSSO}}
{{^nonGoogleSSO}}
{{#isActive}}
<div style="text-align: center">
<p>Click the button below to reset the password for your {{applicationName}} account at {{siteUrl}}.</p>
<a style="display: inline-block; box-sizing: border-box; font-size: 18px; padding: 8px 22px; cursor: pointer; text-decoration: none; border-radius: 4px; background-color: #4990E2; border-color: #4990E2; color: #fff;" href="{{passwordResetUrl}}">Reset password</a>
<p style="padding-top: 2em; font-size: small;">Didn't request this password reset? It's safe to ignore it.</p>
</div>
{{/isActive}}
{{^isActive}}
<p>Someone requested a password reset for your {{applicationName}} account at {{siteUrl}}, but your account
has been deactivated. Contact an administrator for further assistance.</p>
{{#adminEmailSet}}
<a href="mailto:{{adminEmail}}">Contact your administrator</a>
{{/adminEmailSet}}
{{/isActive}}
{{/nonGoogleSSO}}
{{/google}}
</div>
{{> metabase/email/_footer }}
12 changes: 9 additions & 3 deletions test/metabase/email/messages_test.clj
Expand Up @@ -23,7 +23,7 @@
(deftest password-reset-email
(testing "password reset email can be sent successfully"
(et/with-fake-inbox
(messages/send-password-reset-email! "test@test.com" false "http://localhost/some/url" true)
(messages/send-password-reset-email! "test@test.com" false false "http://localhost/some/url" true)
(is (= [{:from "notifications@metabase.com",
:to ["test@test.com"],
:subject "[Metabase] Password Reset Request",
Expand All @@ -34,13 +34,19 @@
;; that the contents changed in the tests below.
(testing "password reset email tells user if they should log in with Google Sign-In"
(et/with-fake-inbox
(messages/send-password-reset-email! "test@test.com" true "http://localhost/some/url" true)
(messages/send-password-reset-email! "test@test.com" true false "http://localhost/some/url" true)
(is (-> (@et/inbox "test@test.com")
(get-in [0 :body 0 :content])
(str/includes? "Google")))))
(testing "password reset email tells user if they should log in with (non-Google) SSO"
(et/with-fake-inbox
(messages/send-password-reset-email! "test@test.com" false true nil true)
(is (-> (@et/inbox "test@test.com")
(get-in [0 :body 0 :content])
(str/includes? "SSO")))))
(testing "password reset email tells user if their account is inactive"
(et/with-fake-inbox
(messages/send-password-reset-email! "test@test.com" false "http://localhost/some/url" false)
(messages/send-password-reset-email! "test@test.com" false false "http://localhost/some/url" false)
(is (-> (@et/inbox "test@test.com")
(get-in [0 :body 0 :content])
(str/includes? "deactivated"))))))
Expand Down
4 changes: 2 additions & 2 deletions test/metabase/email_test.clj
Expand Up @@ -66,8 +66,8 @@
[f]
(with-redefs [email/send-email! fake-inbox-email-fn]
(reset-inbox!)
(tu/with-temporary-setting-values [email-smtp-host "fake_smtp_host"
email-smtp-port 587]
(tu/with-temporary-setting-values [email-smtp-host "fake_smtp_host"
email-smtp-port 587]
(f))))

(defmacro with-fake-inbox
Expand Down

0 comments on commit edadf73

Please sign in to comment.