🐛 Address code review feedback on password notification

- Send password-changed notification from recover-profile too (forgot-password reset path was missing the email). - Strengthen test assertions to verify email factory, recipient, and name via :call-args-list instead of just call-count. - Add negative test: no email sent when old-password is wrong. - Wrap pre-existing update-profile-password test with send! mock to keep its scope focused. Ref: PR #11393 AI-assisted-by: longcat-2.0
✨ Send notification email on password change
2026-09-11 13:20:03 -04:00 · 2026-08-27 12:39:33 +02:00 · 2026-08-27 12:39:33 +02:00
7 changed files with 340 additions and 20 deletions
@@ -0,0 +1,233 @@
+<!doctype html>
+<html xmlns="http://www.w3.org/1999/xhtml" xmlns:v="urn:schemas-microsoft-com:vml"
+  xmlns:o="urn:schemas-microsoft-com:office:office">
+
+<head>
+  <title>
+  </title>
+  <!--[if !mso]><!-- -->
+  <meta http-equiv="X-UA-Compatible" content="IE=edge">
+  <!--<![endif]-->
+  <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
+  <meta name="viewport" content="width=device-width, initial-scale=1">
+  <style type="text/css">
+    #outlook a {
+      padding: 0;
+    }
+
+    body {
+      margin: 0;
+      padding: 0;
+      -webkit-text-size-adjust: 100%;
+      -ms-text-size-adjust: 100%;
+    }
+
+    table,
+    td {
+      border-collapse: collapse;
+      mso-table-lspace: 0pt;
+      mso-table-rspace: 0pt;
+    }
+
+    img {
+      border: 0;
+      height: auto;
+      line-height: 100%;
+      outline: none;
+      text-decoration: none;
+      -ms-interpolation-mode: bicubic;
+    }
+
+    p {
+      display: block;
+      margin: 13px 0;
+    }
+  </style>
+  <!--[if mso]>
+        <xml>
+        <o:OfficeDocumentSettings>
+          <o:AllowPNG/>
+          <o:PixelsPerInch>96</o:PixelsPerInch>
+        </o:OfficeDocumentSettings>
+        </xml>
+        <![endif]-->
+  <!--[if lte mso 11]>
+        <style type="text/css">
+          .mj-outlook-group-fix { width:100% !important; }
+        </style>
+        <![endif]-->
+  <!--[if !mso]><!-->
+  <link href="https://fonts.googleapis.com/css?family=Source%20Sans%20Pro" rel="stylesheet" type="text/css">
+  <style type="text/css">
+    @import url(https://fonts.googleapis.com/css?family=Source%20Sans%20Pro);
+  </style>
+  <!--<![endif]-->
+  <style type="text/css">
+    @media only screen and (min-width:480px) {
+      .mj-column-per-100 {
+        width: 100% !important;
+        max-width: 100%;
+      }
+
+      .mj-column-px-425 {
+        width: 425px !important;
+        max-width: 425px;
+      }
+    }
+  </style>
+  <style type="text/css">
+    @media only screen and (max-width:480px) {
+      table.mj-full-width-mobile {
+        width: 100% !important;
+      }
+
+      td.mj-full-width-mobile {
+        width: auto !important;
+      }
+    }
+  </style>
+</head>
+
+<body style="background-color:#E5E5E5;">
+  <div style="background-color:#E5E5E5;">
+    <!--[if mso | IE]>
+      <table
+         align="center" border="0" cellpadding="0" cellspacing="0" class="" style="width:600px;" width="600"
+      >
+        <tr>
+          <td style="line-height:0px;font-size:0px;mso-line-height-rule:exactly;">
+      <![endif]-->
+    <div style="margin:0px auto;max-width:600px;">
+      <table align="center" border="0" cellpadding="0" cellspacing="0" role="presentation" style="width:100%;">
+        <tbody>
+          <tr>
+            <td style="direction:ltr;font-size:0px;padding:0;text-align:center;">
+              <!--[if mso | IE]>
+                  <table role="presentation" border="0" cellpadding="0" cellspacing="0">
+
+        <tr>
+
+            <td
+               class="" style="vertical-align:top;width:600px;"
+            >
+          <![endif]-->
+              <div class="mj-column-per-100 mj-outlook-group-fix"
+                style="font-size:0px;text-align:left;direction:ltr;display:inline-block;vertical-align:top;width:100%;">
+                <table border="0" cellpadding="0" cellspacing="0" role="presentation" style="vertical-align:top;"
+                  width="100%">
+                  <tr>
+                    <td align="left" style="font-size:0px;padding:16px;word-break:break-word;">
+                      <table border="0" cellpadding="0" cellspacing="0" role="presentation"
+                        style="border-collapse:collapse;border-spacing:0px;">
+                        <tbody>
+                          <tr>
+                            <td style="width:97px;">
+                              <img height="32" src="{{ public-uri }}/images/email/logo-penpot.svg"
+                                style="border:0;display:block;outline:none;text-decoration:none;height:32px;width:100%;font-size:13px;"
+                                width="97" />
+                            </td>
+                          </tr>
+                        </tbody>
+                      </table>
+                    </td>
+                  </tr>
+                </table>
+              </div>
+              <!--[if mso | IE]>
+            </td>
+
+        </tr>
+
+                  </table>
+                </td>
+            </tr>
+        </tbody>
+      </table>
+    </div>
+    <!--[if mso | IE]>
+          </td>
+        </tr>
+      </table>
+
+      <table
+         align="center" border="0" cellpadding="0" cellspacing="0" class="" style="width:600px;" width="600"
+      >
+        <tr>
+          <td style="line-height:0px;font-size:0px;mso-line-height-rule:exactly;">
+      <![endif]-->
+    <div style="background:#FFFFFF;background-color:#FFFFFF;margin:0px auto;max-width:600px;">
+      <table align="center" border="0" cellpadding="0" cellspacing="0" role="presentation"
+        style="background:#FFFFFF;background-color:#FFFFFF;width:100%;">
+        <tbody>
+          <tr>
+            <td style="direction:ltr;font-size:0px;padding:20px 0;text-align:center;">
+              <!--[if mso | IE]>
+                  <table role="presentation" border="0" cellpadding="0" cellspacing="0">
+
+        <tr>
+
+            <td
+               class="" style="vertical-align:top;width:600px;"
+            >
+          <![endif]-->
+              <div class="mj-column-per-100 mj-outlook-group-fix"
+                style="font-size:0px;text-align:left;direction:ltr;display:inline-block;vertical-align:top;width:100%;">
+                <table border="0" cellpadding="0" cellspacing="0" role="presentation" style="vertical-align:top;"
+                  width="100%">
+                  <tr>
+                    <td align="left" style="font-size:0px;padding:10px 25px;word-break:break-word;">
+                      <div
+                        style="font-family:Source Sans Pro, sans-serif;font-size:24px;font-weight:600;line-height:150%;text-align:left;color:#000000;">
+                        Hello {{name|abbreviate:25}}!</div>
+                    </td>
+                  </tr>
+                  <tr>
+                    <td align="left" style="font-size:0px;padding:10px 25px;word-break:break-word;">
+                      <div
+                        style="font-family:Source Sans Pro, sans-serif;font-size:16px;line-height:150%;text-align:left;color:#000000;">
+                        Your password has been changed.
+                      </div>
+                    </td>
+                  </tr>
+                  <tr>
+                    <td align="left" style="font-size:0px;padding:10px 25px;word-break:break-word;">
+                      <div
+                        style="font-family:Source Sans Pro, sans-serif;font-size:16px;line-height:150%;text-align:left;color:#000000;">
+                        If you did not make this change, please contact support immediately or reset your password.
+                      </div>
+                    </td>
+                  </tr>
+                  <tr>
+                    <td align="left" style="font-size:0px;padding:10px 25px;word-break:break-word;">
+                      <div
+                        style="font-family:Source Sans Pro, sans-serif;font-size:16px;line-height:150%;text-align:left;color:#000000;">
+                        Enjoy!</div>
+                    </td>
+                  </tr>
+                  <tr>
+                    <td align="left" style="font-size:0px;padding:10px 25px;word-break:break-word;">
+                      <div
+                        style="font-family:Source Sans Pro, sans-serif;font-size:16px;line-height:150%;text-align:left;color:#000000;">
+                        The Penpot team.</div>
+                    </td>
+                  </tr>
+                </table>
+              </div>
+              <!--[if mso | IE]>
+            </td>
+
+        </tr>
+
+                  </table>
+                </td>
+            </tr>
+        </tbody>
+      </table>
+    </div>
+
+    {% include "app/email/includes/footer.html" %}
+
+  </div>
+</body>
+
+</html>
@@ -0,0 +1 @@
+Password changed
@@ -0,0 +1,8 @@
+Hello {{name|abbreviate:25}}!
+
+Your password has been changed.
+
+If you did not make this change, please contact support immediately or reset your password.
+
+Enjoy!
+The Penpot team.
@@ -407,6 +407,16 @@
   :id ::password-recovery
   :schema schema:password-recovery))

+(def ^:private schema:password-changed
+  [:map
+   [:name ::sm/text]])
+
+(def password-changed
+  "A password changed notification email."
+  (template-factory
+   :id ::password-changed
+   :schema schema:password-changed))
+
 (def ^:private schema:change-email
  [:map
   [:name ::sm/text]
@@ -181,11 +181,17 @@
          (update-password [conn profile-id]
            (let [pwd (auth/derive-password password)]
              (db/update! conn :profile {:password pwd :is-active true} {:id profile-id})
-              nil))]
+              (db/get-by-id conn :profile profile-id)))]

    (passwords/validate-password password)
-    (->> (validate-token token)
-         (update-password conn))
+
+    (let [profile (->> (validate-token token)
+                       (update-password conn))]
+      (eml/send! {::eml/conn conn
+                  ::eml/factory eml/password-changed
+                  :public-uri (cf/get :public-uri)
+                  :to (:email profile)
+                  :name (:fullname profile)}))

    nil))

@@ -222,6 +222,12 @@

    (update-profile-password! cfg (assoc profile :password password))

+    (eml/send! {::eml/conn (::db/conn cfg)
+                ::eml/factory eml/password-changed
+                :public-uri (cf/get :public-uri)
+                :to (:email profile)
+                :name (:fullname profile)})
+
    (->> (rph/get-request params)
         (session/get-session)
         (session/invalidate-others cfg))
@@ -10,6 +10,7 @@
   [app.common.uuid :as uuid]
   [app.config :as cf]
   [app.db :as db]
+   [app.email :as eml]
   [app.email.blacklist :as email.blacklist]
   [app.email.whitelist :as email.whitelist]
   [app.nitrate :as nitrate]
@@ -1164,26 +1165,29 @@


 (t/deftest update-profile-password
-  (let [profile (th/create-profile* 1)
-        data  {::th/type :update-profile-password
-               ::rpc/profile-id (:id profile)
-               :old-password "Test123!"
-               :password "Foobar12!"}
-        out   (th/command! data)]
-    (t/is (nil? (:error out)))
-    (t/is (nil? (:result out)))))
+  (with-mocks [_ {:target 'app.email/send! :return nil}]
+    (let [profile (th/create-profile* 1)
+          data  {::th/type :update-profile-password
+                 ::rpc/profile-id (:id profile)
+                 :old-password "Test123!"
+                 :password "Foobar12!"}
+          out   (th/command! data)]
+      (t/is (nil? (:error out)))
+      (t/is (nil? (:result out))))))


 (t/deftest update-profile-password-bad-old-password
-  (let [profile (th/create-profile* 1)
-        data  {::th/type :update-profile-password
-               ::rpc/profile-id (:id profile)
-               :old-password "badpassword"
-               :password "Foobar12!"}
-        {:keys [result error] :as out} (th/command! data)]
-    (t/is (th/ex-info? error))
-    (t/is (th/ex-of-type? error :validation))
-    (t/is (th/ex-of-code? error :old-password-not-match))))
+  (with-mocks [mock {:target 'app.email/send! :return nil}]
+    (let [profile (th/create-profile* 1)
+          data  {::th/type :update-profile-password
+                 ::rpc/profile-id (:id profile)
+                 :old-password "badpassword"
+                 :password "Foobar12!"}
+          {:keys [result error] :as out} (th/command! data)]
+      (t/is (th/ex-info? error))
+      (t/is (th/ex-of-type? error :validation))
+      (t/is (th/ex-of-code? error :old-password-not-match))
+      (t/is (= 0 (:call-count @mock))))))


 (t/deftest update-profile-password-email-as-password
@@ -1353,3 +1357,55 @@
    (t/is (th/ex-info? (:error out)))
    (t/is (th/ex-of-type? (:error out) :validation))
    (t/is (th/ex-of-code? (:error out) :weak-password))))
+
+
+(t/deftest update-profile-password-sends-notification
+  (with-mocks [mock {:target 'app.email/send! :return nil}]
+    (let [profile (th/create-profile* 1)
+          data    {::th/type :update-profile-password
+                   ::rpc/profile-id (:id profile)
+                   :old-password "Test123!"
+                   :password "Foobar12!"}
+          out     (th/command! data)]
+      (t/is (nil? (:error out)))
+      (t/is (nil? (:result out)))
+      (t/is (= 1 (:call-count @mock)))
+      (let [{:keys [::eml/factory :to :name]} (first (:call-args-list @mock))]
+        (t/is (= eml/password-changed factory))
+        (t/is (= (:email profile) to))
+        (t/is (= (:fullname profile) name))))))
+
+
+(t/deftest update-profile-password-sends-notification-for-first-password
+  (with-mocks [mock {:target 'app.email/send! :return nil}]
+    (let [profile (th/create-profile* 1 {:password "!"})
+          data    {::th/type :update-profile-password
+                   ::rpc/profile-id (:id profile)
+                   :password "Foobar12!"}
+          out     (th/command! data)]
+      (t/is (nil? (:error out)))
+      (t/is (nil? (:result out)))
+      (t/is (= 1 (:call-count @mock)))
+      (let [{:keys [::eml/factory :to :name]} (first (:call-args-list @mock))]
+        (t/is (= eml/password-changed factory))
+        (t/is (= (:email profile) to))
+        (t/is (= (:fullname profile) name))))))
+
+
+(t/deftest recover-profile-sends-notification
+  (with-mocks [mock {:target 'app.email/send! :return nil}]
+    (let [profile (th/create-profile* 1)
+          token   (tokens/generate th/*system*
+                                   {:iss :password-recovery
+                                    :exp (ct/in-future "15m")
+                                    :profile-id (:id profile)})
+          data    {::th/type :recover-profile
+                   :token token
+                   :password "Foobar12!"}
+          out     (th/command! data)]
+      (t/is (nil? (:error out)))
+      (t/is (= 1 (:call-count @mock)))
+      (let [{:keys [::eml/factory :to :name]} (first (:call-args-list @mock))]
+        (t/is (= eml/password-changed factory))
+        (t/is (= (:email profile) to))
+        (t/is (= (:fullname profile) name))))))