Merge branch...

Merge branch '26149-email-notifications-not-being-sent-on-important-security-event-like-disabling-2fa' into 'master'

Send email notification when 2FA is disabled

See merge request gitlab-org/gitlab!39572

app/controllers/admin/users_controller.rb

@@ -111,10 +111,14 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def disable_two_factor
-    update_user { |user| user.disable_two_factor! }
+    result = TwoFactor::DestroyService.new(current_user, user: user).execute
 
+    if result[:status] == :success
       redirect_to admin_user_path(user),
-      notice: _('Two-factor Authentication has been disabled for this user')
+        notice: _('Two-factor authentication has been disabled for this user')
+    else
+      redirect_to admin_user_path(user), alert: result[:message]
+    end
   end
 
   def create

app/controllers/profiles/two_factor_auths_controller.rb

@@ -73,9 +73,13 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
   end
 
   def destroy
-    current_user.disable_two_factor!
+    result = TwoFactor::DestroyService.new(current_user, user: current_user).execute
 
-    redirect_to profile_account_path, status: :found
+    if result[:status] == :success
+      redirect_to profile_account_path, status: :found, notice: s_('Two-factor authentication has been disabled successfully!')
+    else
+      redirect_to profile_account_path, status: :found, alert: result[:message]
+    end
   end
 
   def skip

app/helpers/emails_helper.rb

@@ -177,6 +177,27 @@ module EmailsHelper
     strip_tags(render_message(:footer_message, style: ''))
   end
 
+  def say_hi(user)
+    _('Hi %{username}!') % { username: sanitize_name(user.name) }
+  end
+
+  def two_factor_authentication_disabled_text
+    _('Two-factor authentication has been disabled for your GitLab account.')
+  end
+
+  def re_enable_two_factor_authentication_text(format: nil)
+    url = profile_two_factor_auth_url
+
+    case format
+    when :html
+      settings_link_to = link_to(_('two-factor authentication settings'), url, target: :_blank, rel: 'noopener noreferrer').html_safe
+      _("If you want to re-enable two-factor authentication, visit the %{settings_link_to} page.").html_safe % { settings_link_to: settings_link_to }
+    else
+      _('If you want to re-enable two-factor authentication, visit %{two_factor_link}') %
+        { two_factor_link: url }
+    end
+  end
+
   private
 
   def show_footer?

app/mailers/emails/profile.rb

@@ -72,6 +72,16 @@ module Emails
         end
       end
     end
+
+    def disabled_two_factor_email(user)
+      return unless user
+
+      @user = user
+
+      Gitlab::I18n.with_locale(@user.preferred_language) do
+        mail(to: @user.notification_email, subject: subject(_("Two-factor authentication disabled")))
+      end
+    end
   end
 end
 

app/policies/user_policy.rb

@@ -25,6 +25,7 @@ class UserPolicy < BasePolicy
 
   rule { default }.enable :read_user_profile
   rule { (private_profile | blocked_user) & ~(user_is_self | admin) }.prevent :read_user_profile
+  rule { user_is_self | admin }.enable :disable_two_factor
 end
 
 UserPolicy.prepend_if_ee('EE::UserPolicy')

app/services/notification_service.rb

@@ -35,6 +35,12 @@ class NotificationService
     @async ||= Async.new(self)
   end
 
+  def disabled_two_factor(user)
+    return unless user.can?(:receive_notifications)
+
+    mailer.disabled_two_factor_email(user).deliver_later
+  end
+
   # Always notify user about ssh key added
   # only if ssh key is not deploy key
   #

app/services/two_factor/base_service.rb

+# frozen_string_literal: true
+
+module TwoFactor
+  class BaseService
+    include BaseServiceUtility
+
+    attr_reader :current_user, :params, :user
+
+    def initialize(current_user, params = {})
+      @current_user, @params = current_user, params
+      @user = params.delete(:user)
+    end
+  end
+end

app/services/two_factor/destroy_service.rb

+# frozen_string_literal: true
+
+module TwoFactor
+  class DestroyService < ::TwoFactor::BaseService
+    def execute
+      return error(_('You are not authorized to perform this action')) unless can?(current_user, :disable_two_factor, user)
+      return error(_('Two-factor authentication is not enabled for this user')) unless user.two_factor_enabled?
+
+      result = disable_two_factor
+
+      notification_service.disabled_two_factor(user) if result[:status] == :success
+
+      result
+    end
+
+    private
+
+    def disable_two_factor
+      ::Users::UpdateService.new(current_user, user: user).execute do |user|
+        user.disable_two_factor!
+      end
+    end
+  end
+end

app/views/notify/disabled_two_factor_email.html.haml

+%p
+  = say_hi(@user)
+%p
+  = two_factor_authentication_disabled_text
+%p
+  = re_enable_two_factor_authentication_text(format: :html)

app/views/notify/disabled_two_factor_email.text.erb

+<%= say_hi(@user) %>
+
+<%= two_factor_authentication_disabled_text %>
+
+<%= re_enable_two_factor_authentication_text %>

changelogs/unreleased/26149-email-notifications-not-being-sent-on-important-security-event-like.yml

+---
+title: Send email notification on disabling 2FA
+merge_request: 39572
+author:
+type: added

doc/user/profile/notifications.md

@@ -144,6 +144,7 @@ Users will be notified of the following events:
 | New email added              | User                | Security email, always sent. |
 | Email changed                | User                | Security email, always sent. |
 | Password changed             | User                | Security email, always sent. |
+| Two-factor authentication disabled | User          | Security email, always sent. |
 | New user created             | User                | Sent on user creation, except for OmniAuth (LDAP)|
 | User added to project        | User                | Sent when user is added to project |
 | Project access level changed | User                | Sent when user project access level is changed |

locale/gitlab.pot

@@ -12746,6 +12746,12 @@ msgstr ""
 msgid "If you remove this license, GitLab will fall back on the previous license, if any."
 msgstr ""
 
+msgid "If you want to re-enable two-factor authentication, visit %{two_factor_link}"
+msgstr ""
+
+msgid "If you want to re-enable two-factor authentication, visit the %{settings_link_to} page."
+msgstr ""
+
 msgid "If your HTTP repository is not publicly accessible, add your credentials."
 msgstr ""
 
@@ -26076,10 +26082,22 @@ msgstr ""
 msgid "Two-factor Authentication Recovery codes"
 msgstr ""
 
-msgid "Two-factor Authentication has been disabled for this user"
+msgid "Two-factor authentication"
 msgstr ""
 
-msgid "Two-factor authentication"
+msgid "Two-factor authentication disabled"
+msgstr ""
+
+msgid "Two-factor authentication has been disabled for this user"
+msgstr ""
+
+msgid "Two-factor authentication has been disabled for your GitLab account."
+msgstr ""
+
+msgid "Two-factor authentication has been disabled successfully!"
+msgstr ""
+
+msgid "Two-factor authentication is not enabled for this user"
 msgstr ""
 
 msgid "Type"
@@ -29910,6 +29928,9 @@ msgstr ""
 msgid "triggered"
 msgstr ""
 
+msgid "two-factor authentication settings"
+msgstr ""
+
 msgid "unicode domains should use IDNA encoding"
 msgstr ""
 

spec/controllers/admin/users_controller_spec.rb

@@ -218,28 +218,44 @@ RSpec.describe Admin::UsersController do
   end
 
   describe 'PATCH disable_two_factor' do
+    subject { patch :disable_two_factor, params: { id: user.to_param } }
+
+    context 'for a user that has 2FA enabled' do
+      let(:user) { create(:user, :two_factor) }
+
       it 'disables 2FA for the user' do
-      expect(user).to receive(:disable_two_factor!)
-      allow(subject).to receive(:user).and_return(user)
+        subject
 
-      go
+        expect(user.reload.two_factor_enabled?).to eq(false)
       end
 
       it 'redirects back' do
-      go
+        subject
 
         expect(response).to redirect_to(admin_user_path(user))
       end
 
-    it 'displays an alert' do
-      go
+      it 'displays a notice on success' do
+        subject
 
         expect(flash[:notice])
-        .to eq _('Two-factor Authentication has been disabled for this user')
+          .to eq _('Two-factor authentication has been disabled for this user')
+      end
+    end
+
+    context 'for a user that does not have 2FA enabled' do
+      it 'redirects back' do
+        subject
+
+        expect(response).to redirect_to(admin_user_path(user))
       end
 
-    def go
-      patch :disable_two_factor, params: { id: user.to_param }
+      it 'displays an alert on failure' do
+        subject
+
+        expect(flash[:alert])
+          .to eq _('Two-factor authentication is not enabled for this user')
+      end
     end
   end
 

spec/controllers/profiles/two_factor_auths_controller_spec.rb

@@ -107,18 +107,46 @@ RSpec.describe Profiles::TwoFactorAuthsController do
   end
 
   describe 'DELETE destroy' do
+    subject { delete :destroy }
+
+    context 'for a user that has 2FA enabled' do
       let(:user) { create(:user, :two_factor) }
 
       it 'disables two factor' do
-      expect(user).to receive(:disable_two_factor!)
+        subject
+
+        expect(user.reload.two_factor_enabled?).to eq(false)
+      end
+
+      it 'redirects to profile_account_path' do
+        subject
+
+        expect(response).to redirect_to(profile_account_path)
+      end
 
-      delete :destroy
+      it 'displays a notice on success' do
+        subject
+
+        expect(flash[:notice])
+          .to eq _('Two-factor authentication has been disabled successfully!')
+      end
     end
 
+    context 'for a user that does not have 2FA enabled' do
+      let(:user) { create(:user) }
+
       it 'redirects to profile_account_path' do
-      delete :destroy
+        subject
 
         expect(response).to redirect_to(profile_account_path)
       end
+
+      it 'displays an alert on failure' do
+        subject
+
+        expect(flash[:alert])
+          .to eq _('Two-factor authentication is not enabled for this user')
+      end
+    end
   end
 end

spec/helpers/emails_helper_spec.rb

@@ -31,7 +31,7 @@ RSpec.describe EmailsHelper do
 
         context "and format is unknown" do
           it "returns plain text" do
-            expect(helper.closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})")
+            expect(helper.closure_reason_text(merge_request, format: 'unknown')).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})")
           end
         end
       end
@@ -110,6 +110,41 @@ RSpec.describe EmailsHelper do
     end
   end
 
+  describe '#say_hi' do
+    let(:user) { create(:user, name: 'John') }
+
+    it 'returns the greeting message for the given user' do
+      expect(say_hi(user)).to eq('Hi John!')
+    end
+  end
+
+  describe '#two_factor_authentication_disabled_text' do
+    it 'returns the message that 2FA is disabled' do
+      expect(two_factor_authentication_disabled_text).to eq(
+        _('Two-factor authentication has been disabled for your GitLab account.')
+      )
+    end
+  end
+
+  describe '#re_enable_two_factor_authentication_text' do
+    context 'format is html' do
+      it 'returns HTML' do
+        expect(re_enable_two_factor_authentication_text(format: :html)).to eq(
+          "If you want to re-enable two-factor authentication, visit the " \
+          "#{link_to('two-factor authentication settings', profile_two_factor_auth_url, target: :_blank, rel: 'noopener noreferrer')} page."
+        )
+      end
+    end
+
+    context 'format is not specified' do
+      it 'returns text' do
+        expect(re_enable_two_factor_authentication_text).to eq(
+          "If you want to re-enable two-factor authentication, visit #{profile_two_factor_auth_url}"
+        )
+      end
+    end
+  end
+
   describe 'password_reset_token_valid_time' do
     def validate_time_string(time_limit, expected_string)
       Devise.reset_password_within = time_limit

spec/mailers/emails/profile_spec.rb

@@ -256,4 +256,26 @@ RSpec.describe Emails::Profile do
       end
     end
   end
+
+  describe 'disabled two-factor authentication email' do
+    let_it_be(:user) { create(:user) }
+
+    subject { Notify.disabled_two_factor_email(user) }
+
+    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
+      is_expected.to deliver_to user.email
+    end
+
+    it 'has the correct subject' do
+      is_expected.to have_subject /^Two-factor authentication disabled$/i
+    end
+
+    it 'includes a link to two-factor authentication settings page' do
+      is_expected.to have_body_text /#{profile_two_factor_auth_path}/
+    end
+  end
 end

spec/policies/user_policy_spec.rb

@@ -82,4 +82,24 @@ RSpec.describe UserPolicy do
   describe "updating a user" do
     it_behaves_like 'changing a user', :update_user
   end
+
+  describe 'disabling two-factor authentication' do
+    context 'disabling their own two-factor authentication' do
+      let(:user) { current_user }
+
+      it { is_expected.to be_allowed(:disable_two_factor) }
+    end
+
+    context 'disabling the two-factor authentication of another user' do
+      context 'when the executor is an admin', :enable_admin_mode do
+        let(:current_user) { create(:user, :admin) }
+
+        it { is_expected.to be_allowed(:disable_two_factor) }
+      end
+
+      context 'when the executor is not an admin' do
+        it { is_expected.not_to be_allowed(:disable_two_factor) }
+      end
+    end
+  end
 end

spec/services/notification_service_spec.rb

@@ -272,6 +272,16 @@ RSpec.describe NotificationService, :mailer do
     end
   end
 
+  describe '#disabled_two_factor' do
+    let_it_be(:user) { create(:user) }
+
+    subject { notification.disabled_two_factor(user) }
+
+    it 'sends email to the user' do
+      expect { subject }.to have_enqueued_email(user, mail: 'disabled_two_factor_email')
+    end
+  end
+
   describe 'Notes' do
     context 'issue note' do
       let(:project) { create(:project, :private) }

spec/services/two_factor/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe TwoFactor::DestroyService do
+  let_it_be(:current_user) { create(:user) }
+
+  subject { described_class.new(current_user, user: user).execute }
+
+  context 'disabling two-factor authentication' do
+    shared_examples_for 'does not send notification email' do
+      context 'notification', :mailer do
+        it 'does not send a notification' do
+          perform_enqueued_jobs do
+            subject
+          end
+
+          should_not_email(user)
+        end
+      end
+    end
+
+    context 'when the user does not have two-factor authentication enabled' do
+      let(:user) { current_user }
+
+      it 'returns error' do
+        expect(subject).to eq(
+          {
+            status: :error,
+            message: 'Two-factor authentication is not enabled for this user'
+          }
+        )
+      end
+
+      it_behaves_like 'does not send notification email'
+    end
+
+    context 'when the user has two-factor authentication enabled' do
+      context 'when the executor is not authorized to disable two-factor authentication' do
+        context 'disabling the two-factor authentication of another user' do
+          let(:user) { create(:user, :two_factor) }
+
+          it 'returns error' do
+            expect(subject).to eq(
+              {
+                status: :error,
+                message: 'You are not authorized to perform this action'
+              }
+            )
+          end
+
+          it 'does not disable two-factor authentication' do
+            expect { subject }.not_to change { user.reload.two_factor_enabled? }.from(true)
+          end
+
+          it_behaves_like 'does not send notification email'
+        end
+      end
+
+      context 'when the executor is authorized to disable two-factor authentication' do
+        shared_examples_for 'disables two-factor authentication' do
+          it 'returns success' do
+            expect(subject).to eq({ status: :success })
+          end
+
+          it 'disables the two-factor authentication of the user' do
+            expect { subject }.to change { user.reload.two_factor_enabled? }.from(true).to(false)
+          end
+
+          context 'notification', :mailer do
+            it 'sends a notification' do
+              perform_enqueued_jobs do
+                subject
+              end
+
+              should_email(user)
+            end
+          end
+        end
+
+        context 'disabling their own two-factor authentication' do
+          let(:current_user) { create(:user, :two_factor) }
+          let(:user) { current_user }
+
+          it_behaves_like 'disables two-factor authentication'
+        end
+
+        context 'admin disables the two-factor authentication of another user' do
+          let(:current_user) { create(:admin) }
+          let(:user) { create(:user, :two_factor) }
+
+          it_behaves_like 'disables two-factor authentication'
+        end
+      end
+    end
+  end
+end