Merge branch 'dblessing-new-sign-in-email-beautification' into 'master'

New/unknown sign-in email email styling

See merge request gitlab-org/gitlab!32808

app/controllers/concerns/known_sign_in.rb

@@ -26,6 +26,6 @@ module KnownSignIn
   end
 
   def notify_user
-    current_user.notification_service.unknown_sign_in(current_user, request.remote_ip)
+    current_user.notification_service.unknown_sign_in(current_user, request.remote_ip, current_user.current_sign_in_at)
   end
 end

app/mailers/emails/profile.rb

@@ -45,13 +45,20 @@ module Emails
       end
     end
 
-    def unknown_sign_in_email(user, ip)
+    def unknown_sign_in_email(user, ip, time)
       @user = user
       @ip = ip
+      @time = time
       @target_url = edit_profile_password_url
 
       Gitlab::I18n.with_locale(@user.preferred_language) do
-        mail(to: @user.notification_email, subject: subject(_("Unknown sign-in from new location")))
+        mail(
+          to: @user.notification_email,
+          subject: subject(_("%{host} sign-in from new location") % { host: Gitlab.config.gitlab.host })
+        ) do |format|
+          format.html { render layout: 'mailer' }
+          format.text { render layout: 'mailer' }
+        end
       end
     end
   end

app/mailers/previews/notify_preview.rb

@@ -162,7 +162,7 @@ class NotifyPreview < ActionMailer::Preview
   end
 
   def unknown_sign_in_email
-    Notify.unknown_sign_in_email(user, '127.0.0.1').message
+    Notify.unknown_sign_in_email(user, '127.0.0.1', Time.current).message
   end
 
   private

app/models/active_session.rb

@@ -36,7 +36,7 @@ class ActiveSession
       timestamp = Time.current
 
       active_user_session = new(
-        ip_address: request.ip,
+        ip_address: request.remote_ip,
         browser: client.name,
         os: client.os_name,
         device_name: client.device_name,

app/services/notification_service.rb

@@ -68,10 +68,10 @@ class NotificationService
 
   # Notify a user when a previously unknown IP or device is used to
   # sign in to their account
-  def unknown_sign_in(user, ip)
+  def unknown_sign_in(user, ip, time)
     return unless user.can?(:receive_notifications)
 
-    mailer.unknown_sign_in_email(user, ip).deliver_later
+    mailer.unknown_sign_in_email(user, ip, time).deliver_later
   end
 
   # When create an issue we should send an email to:

app/views/notify/unknown_sign_in_email.html.haml

-%p
-  = _('Hi %{username}!') % { username: sanitize_name(@user.name) }
-%p
-  = _('A sign-in to your account has been made from the following IP address: %{ip}.') % { ip: @ip }
-%p
-  - password_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: 'https://docs.gitlab.com/ee/user/profile/#changing-your-password' }
-  = _('If you recently signed in and recognize the IP address, you may disregard this email.')
-  = _('If you did not recently sign in, you should immediately %{password_link_start}change your password%{password_link_end}.').html_safe % { password_link_start: password_link_start, password_link_end: '</a>'.html_safe }
-  = _('Passwords should be unique and not used for any other sites or services.')
+- default_font = "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;"
+- default_style = "#{default_font}font-size:15px;line-height:1.4;color:#8c8c8c;font-weight:300;padding:14px 0;margin:0;"
+- spacer_style = "#{default_font};height:18px;font-size:18px;line-height:18px;"
 
-- unless @user.two_factor_enabled?
-  %p
-    - mfa_link_start = '<a href="https://docs.gitlab.com/ee/user/profile/account/two_factor_authentication.html" target="_blank">'.html_safe
-    = _('To further protect your account, consider configuring a %{mfa_link_start}two-factor authentication%{mfa_link_end} method.').html_safe % { mfa_link_start: mfa_link_start, mfa_link_end: '</a>'.html_safe }
+%tr.alert
+  %td{ style: "#{default_font}padding:10px;border-radius:3px;font-size:14px;line-height:1.3;text-align:center;overflow:hidden;color:#ffffff;background-color:#FC6D26;" }
+    %table.img{ border: "0", cellpadding: "0", cellspacing: "0", style: "border-collapse:collapse;margin:0 auto;" }
+      %tbody
+        %tr
+          %td{ style: "#{default_font}vertical-align:middle;color:#ffffff;text-align:center;" }
+            %span
+              = _("Your %{host} account was signed in to from a new location") % { host: Gitlab.config.gitlab.host }
+%tr.spacer
+  %td{ style: spacer_style }
+    &nbsp;
+%tr.section
+  %td{ style: "#{default_font};padding:0 15px;border:1px solid #ededed;border-radius:3px;overflow:hidden;" }
+    %table.info{ border: "0", cellpadding: "0", cellspacing: "0", style: "width:100%;" }
+      %tbody
+        %tr
+          %td{ style: default_style }
+            = _('Hostname')
+          %td{ style: "#{default_style}color:#333333;font-weight:400;width:75%;padding-left:5px;" }
+            = Gitlab.config.gitlab.host
+        %tr
+          %td{ style: "#{default_style}border-top:1px solid #ededed;" }
+            = _('IP Address')
+          %td{ style: "#{default_style}color:#333333;font-weight:400;width:75%;padding-left:5px;border-top:1px solid #ededed;" }
+            %span.muted{ style: "color:#333333;text-decoration:none;" }
+              = @ip
+        %tr
+          %td{ style: "#{default_style}border-top:1px solid #ededed;" }
+            = _('Time')
+          %td{ style: "#{default_style}color:#333333;font-weight:400;width:75%;padding-left:5px;border-top:1px solid #ededed;" }
+            = @time.strftime('%Y-%m-%d %l:%M:%S %p %Z')
+%tr.spacer
+  %td{ style: spacer_style }
+    &nbsp;
+%tr.section
+  %td{ style: "#{default_font};line-height:1.4;text-align:center;padding:0 15px;overflow:hidden;" }
+    %table.img{ border: "0", cellpadding: "0", cellspacing: "0", style: "border-collapse:collapse;width:100%;" }
+      %tbody
+        %tr{ style: 'width:100%;' }
+          %td{ style: "#{default_style}text-align:center;" }
+            - password_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: 'https://docs.gitlab.com/ee/user/profile/#changing-your-password' }
+            = _('If you recently signed in and recognize the IP address, you may disregard this email.')
+            %p
+              = _('If you did not recently sign in, you should immediately %{password_link_start}change your password%{password_link_end}.').html_safe % { password_link_start: password_link_start, password_link_end: '</a>'.html_safe }
+              = _('Passwords should be unique and not used for any other sites or services.')
+
+            - unless @user.two_factor_enabled?
+              %p
+                - mfa_link_start = '<a href="https://docs.gitlab.com/ee/user/profile/account/two_factor_authentication.html" target="_blank">'.html_safe
+                = _('To further protect your account, consider configuring a %{mfa_link_start}two-factor authentication%{mfa_link_end} method.').html_safe % { mfa_link_start: mfa_link_start, mfa_link_end: '</a>'.html_safe }

changelogs/unreleased/dblessing-new-sign-in-email-beautification.yml

+---
+title: Improve new/unknown sign-in email styling
+merge_request: 32808
+author:
+type: changed

doc/user/profile/unknown_sign_in_notification.md

 # Email notification for unknown sign-ins
 
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/27211) in GitLab 13.0.
+
 When a user successfully signs in from a previously unknown IP address,
 GitLab notifies the user by email. In this way, GitLab proactively alerts users of potentially
 malicious or unauthorized sign-ins.
@@ -13,4 +15,4 @@ There are two methods used to identify a known sign-in:
 
 ## Example email
 
-![Unknown sign in email](./img/unknown_sign_in_email_v13_0.png)
+![Unknown sign in email](./img/unknown_sign_in_email_v13_1.png)

locale/gitlab.pot

@@ -358,6 +358,9 @@ msgstr ""
 msgid "%{group_name} uses group managed accounts. You need to create a new GitLab account which will be managed by %{group_name}."
 msgstr ""
 
+msgid "%{host} sign-in from new location"
+msgstr ""
+
 msgid "%{icon}You are about to add %{usersTag} people to the discussion. Proceed with caution."
 msgstr ""
 
@@ -968,9 +971,6 @@ msgstr ""
 msgid "A sign-in to your account has been made from the following IP address: %{ip}"
 msgstr ""
 
-msgid "A sign-in to your account has been made from the following IP address: %{ip}."
-msgstr ""
-
 msgid "A subscription will trigger a new pipeline on the default branch of this project when a pipeline successfully completes for a new tag on the %{default_branch_docs} of the subscribed project."
 msgstr ""
 
@@ -11405,6 +11405,9 @@ msgstr ""
 msgid "Hook was successfully updated."
 msgstr ""
 
+msgid "Hostname"
+msgstr ""
+
 msgid "Hour (UTC)"
 msgstr ""
 
@@ -23531,9 +23534,6 @@ msgstr ""
 msgid "Unknown response text"
 msgstr ""
 
-msgid "Unknown sign-in from new location"
-msgstr ""
-
 msgid "Unlimited"
 msgstr ""
 
@@ -25535,6 +25535,9 @@ msgstr ""
 msgid "YouTube"
 msgstr ""
 
+msgid "Your %{host} account was signed in to from a new location"
+msgstr ""
+
 msgid "Your %{strong}%{plan_name}%{strong_close} subscription for %{strong}%{namespace_name}%{strong_close} will expire on %{strong}%{expires_on}%{strong_close}."
 msgstr ""
 

spec/mailers/emails/profile_spec.rb

@@ -160,38 +160,48 @@ describe Emails::Profile do
   describe 'user unknown sign in email' do
     let_it_be(:user) { create(:user) }
     let_it_be(:ip) { '169.0.0.1' }
+    let_it_be(:current_time) { Time.current }
+    let_it_be(:email) { Notify.unknown_sign_in_email(user, ip, current_time) }
 
-    subject { Notify.unknown_sign_in_email(user, ip) }
+    subject { email }
 
     it_behaves_like 'an email sent from GitLab'
     it_behaves_like 'it should not have Gmail Actions links'
     it_behaves_like 'a user cannot unsubscribe through footer link'
 
     it 'is sent to the user' do
-      expect(subject).to deliver_to user.email
+      is_expected.to deliver_to user.email
     end
 
     it 'has the correct subject' do
-      expect(subject).to have_subject /^Unknown sign-in from new location$/
+      is_expected.to have_subject "#{Gitlab.config.gitlab.host} sign-in from new location"
+    end
+
+    it 'mentions the new sign-in IP' do
+      is_expected.to have_body_text ip
     end
 
-    it 'mentions the unknown sign-in IP' do
-      expect(subject).to have_body_text /A sign-in to your account has been made from the following IP address: #{ip}./
+    it 'mentioned the time' do
+      is_expected.to have_body_text current_time.strftime('%Y-%m-%d %l:%M:%S %p %Z')
     end
 
-    it 'includes a link to the change password page' do
-      expect(subject).to have_body_text /#{edit_profile_password_path}/
+    it 'includes a link to the change password documentation' do
+      is_expected.to have_body_text 'https://docs.gitlab.com/ee/user/profile/#changing-your-password'
     end
 
     it 'mentions two factor authentication when two factor is not enabled' do
-      expect(subject).to have_body_text /two-factor authentication/
+      is_expected.to have_body_text 'two-factor authentication'
+    end
+
+    it 'includes a link to two-factor authentication documentation' do
+      is_expected.to have_body_text 'https://docs.gitlab.com/ee/user/profile/account/two_factor_authentication.html'
     end
 
     context 'when two factor authentication is enabled' do
-      it 'does not mention two factor authentication' do
-        two_factor_user = create(:user, :two_factor)
+      let(:user) { create(:user, :two_factor) }
 
-        expect( Notify.unknown_sign_in_email(two_factor_user, ip) )
+      it 'does not mention two factor authentication' do
+        expect( Notify.unknown_sign_in_email(user, ip, current_time) )
           .not_to have_body_text /two-factor authentication/
       end
     end

spec/models/active_session_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     double(:request, {
       user_agent: 'Mozilla/5.0 (iPhone; CPU iPhone OS 8_1_3 like Mac OS X) AppleWebKit/600.1.4 ' \
         '(KHTML, like Gecko) Mobile/12B466 [FBDV/iPhone7,2]',
-      ip: '127.0.0.1',
+      remote_ip: '127.0.0.1',
       session: session
     })
   end

spec/services/notification_service_spec.rb

@@ -243,11 +243,12 @@ describe NotificationService, :mailer do
   describe '#unknown_sign_in' do
     let_it_be(:user) { create(:user) }
     let_it_be(:ip) { '127.0.0.1' }
+    let_it_be(:time) { Time.current }
 
-    subject { notification.unknown_sign_in(user, ip) }
+    subject { notification.unknown_sign_in(user, ip, time) }
 
     it 'sends email to the user' do
-      expect { subject }.to have_enqueued_email(user, ip, mail: 'unknown_sign_in_email')
+      expect { subject }.to have_enqueued_email(user, ip, time, mail: 'unknown_sign_in_email')
     end
   end