Password changed emails must specify that password was changed by admin

This change sends out a new email ‘Password changed by administrator’, when the password of a user was changed by the administrator via the UI/API.

Password changed emails must specify that password was changed by admin
This change sends out a new email ‘Password changed by administrator’, when the password of a user was changed by the administrator via the UI/API.
577f1604 · manojmj · 194eee15 · 577f1604 · 577f1604 · 577f1604
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -149,7 +149,7 @@ def update
        password_confirmation: params[:user][:password_confirmation]
      }

-      password_params[:password_expires_at] = Time.current unless changing_own_password?
+      password_params[:password_expires_at] = Time.current if admin_making_changes_for_another_user?

      user_params_with_pass.merge!(password_params)
    end
@@ -157,6 +157,7 @@ def update
    respond_to do |format|
      result = Users::UpdateService.new(current_user, user_params_with_pass.merge(user: user)).execute do |user|
        user.skip_reconfirmation!
+        user.send_only_admin_changed_your_password_notification! if admin_making_changes_for_another_user?
      end

      if result[:status] == :success
@@ -197,8 +198,8 @@ def remove_email

  protected

-  def changing_own_password?
-    user == current_user
+  def admin_making_changes_for_another_user?
+    user != current_user
  end

  def user

--- a/app/helpers/emails_helper.rb
+++ b/app/helpers/emails_helper.rb
@@ -181,6 +181,10 @@ def say_hi(user)
    _('Hi %{username}!') % { username: sanitize_name(user.name) }
  end

+  def say_hello(user)
+    _('Hello, %{username}!') % { username: sanitize_name(user.name) }
+  end
+
  def two_factor_authentication_disabled_text
    _('Two-factor authentication has been disabled for your GitLab account.')
  end
@@ -190,7 +194,7 @@ def re_enable_two_factor_authentication_text(format: nil)

    case format
    when :html
-      settings_link_to = link_to(_('two-factor authentication settings'), url, target: :_blank, rel: 'noopener noreferrer').html_safe
+      settings_link_to = generate_link(_('two-factor authentication settings'), url).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}') %
@@ -198,8 +202,28 @@ def re_enable_two_factor_authentication_text(format: nil)
    end
  end

+  def admin_changed_password_text(format: nil)
+    url = Gitlab.config.gitlab.url
+
+    case format
+    when :html
+      link_to = generate_link(url, url).html_safe
+      _('An administrator changed the password for your GitLab account on %{link_to}.').html_safe % { link_to: link_to }
+    else
+      _('An administrator changed the password for your GitLab account on %{link_to}.') % { link_to: url }
+    end
+  end
+
+  def contact_your_administrator_text
+    _('Please contact your administrator with any questions.')
+  end
+
  private

+  def generate_link(text, url)
+    link_to(text, url, target: :_blank, rel: 'noopener noreferrer')
+  end
+
  def show_footer?
    email_header_and_footer_enabled? && current_appearance&.show_footer?
  end

--- a/app/mailers/devise_mailer.rb
+++ b/app/mailers/devise_mailer.rb
@@ -9,6 +9,10 @@ class DeviseMailer < Devise::Mailer
  helper EmailsHelper
  helper ApplicationHelper

+  def password_change_by_admin(record, opts = {})
+    devise_mail(record, :password_change_by_admin, opts)
+  end
+
  protected

  def subject_for(key)

--- a/app/models/concerns/admin_changed_password_notifier.rb
+++ b/app/models/concerns/admin_changed_password_notifier.rb
+# frozen_string_literal: true
+
+module AdminChangedPasswordNotifier
+  # This module is responsible for triggering the `Password changed by administrator` emails
+  # when a GitLab administrator changes the password of another user.
+
+  # Usage
+  # These emails are disabled by default and are never trigerred after updating the password, unless
+  # explicitly specified.
+
+  # To explicitly trigger this email, the `send_only_admin_changed_your_password_notification!`
+  # method should be called, so like:
+
+  # user = User.find_by(email: 'hello@example.com')
+  # user.send_only_admin_changed_your_password_notification!
+  # user.password = user.password_confirmation = 'new_password'
+  # user.save!
+
+  # The `send_only_admin_changed_your_password_notification` has 2 responsibilities.
+  # It prevents triggering Devise's default `Password changed` email.
+  # It trigggers the `Password changed by administrator` email.
+
+  # It is important to skip sending the default Devise email when sending out `Password changed by administrator`
+  # email because we should not be sending 2 emails for the same event,
+  # hence the only public API made available from this module is `send_only_admin_changed_your_password_notification!`
+
+  # There is no public API made available to send the `Password changed by administrator` email,
+  # *without* skipping the default `Password changed` email, to prevent the problem mentioned above.
+
+  extend ActiveSupport::Concern
+
+  included do
+    after_update :send_admin_changed_your_password_notification, if: :send_admin_changed_your_password_notification?
+  end
+
+  def initialize(*args, &block)
+    @allow_admin_changed_your_password_notification = false # These emails are off by default
+    super
+  end
+
+  def send_only_admin_changed_your_password_notification!
+    skip_password_change_notification! # skip sending the default Devise 'password changed' notification
+    allow_admin_changed_your_password_notification!
+  end
+
+  private
+
+  def send_admin_changed_your_password_notification
+    send_devise_notification(:password_change_by_admin)
+  end
+
+  def allow_admin_changed_your_password_notification!
+    @allow_admin_changed_your_password_notification = true # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  end
+
+  def send_admin_changed_your_password_notification?
+    self.class.send_password_change_notification && saved_change_to_encrypted_password? &&
+    @allow_admin_changed_your_password_notification # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  end
+end
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -58,6 +58,8 @@ class User < ApplicationRecord
  devise :lockable, :recoverable, :rememberable, :trackable,
         :validatable, :omniauthable, :confirmable, :registerable

+  include AdminChangedPasswordNotifier
+
  # This module adds async behaviour to Devise emails
  # and should be added after Devise modules are initialized.
  include AsyncDeviseEmail

--- a/app/views/devise/mailer/password_change_by_admin.html.haml
+++ b/app/views/devise/mailer/password_change_by_admin.html.haml
+= email_default_heading(say_hello(@resource))
+
+%p
+  = admin_changed_password_text(format: :html)
+%p
+  = contact_your_administrator_text
--- a/app/views/devise/mailer/password_change_by_admin.text.erb
+++ b/app/views/devise/mailer/password_change_by_admin.text.erb
+<%= say_hello(@resource) %>
+
+<%= admin_changed_password_text %>
+
+<%= contact_your_administrator_text %>
--- a/changelogs/unreleased/27284-indicate-that-password-reset-was-initiated-by-an-admin-when-an-admi.yml
+++ b/changelogs/unreleased/27284-indicate-that-password-reset-was-initiated-by-an-admin-when-an-admi.yml
+---
+title: Password changed emails must specify that password was changed by admin
+merge_request: 40342
+author:
+type: added
--- a/config/locales/devise.en.yml
+++ b/config/locales/devise.en.yml
@@ -25,6 +25,8 @@ en:
        subject: "Unlock instructions"
      password_change:
        subject: "Password Changed"
+      password_change_by_admin:
+        subject: "Password changed by administrator"
    omniauth_callbacks:
      failure: "Could not authenticate you from %{kind} because \"%{reason}\"."
      success: "Successfully authenticated from %{kind} account."

--- a/doc/security/README.md
+++ b/doc/security/README.md
@@ -12,7 +12,7 @@ type: index
 - [Rate limits](rate_limits.md)
 - [Webhooks and insecure internal web services](webhooks.md)
 - [Information exclusivity](information_exclusivity.md)
- [Reset your root password](reset_root_password.md)
+- [Reset user password](reset_user_password.md)
 - [Unlock a locked user](unlock_user.md)
 - [User File Uploads](user_file_uploads.md)
 - [How we manage the CRIME vulnerability](crime_vulnerability.md)

--- a/doc/security/reset_root_password.md
+++ b/doc/security/reset_root_password.md
@@ -2,9 +2,9 @@
 type: howto
 ---

-# How to reset your root password
+# How to reset user password

-To reset your root password, first log into your server with root privileges.
+To reset the password of a user, first log into your server with root privileges.

 Start a Ruby on Rails console with this command:

@@ -14,18 +14,22 @@ gitlab-rails console -e production

 Wait until the console has loaded.

-There are multiple ways to find your user. You can search for email or username.
+## Find the user
+
+There are multiple ways to find your user. You can search by email or user ID number.

 ```shell
-user = User.where(id: 1).first
+user = User.where(id: 7).first
 ```

 or

 ```shell
-user = User.find_by(email: 'admin@example.com')
+user = User.find_by(email: 'user@example.com')
 ```

+## Reset the password
+
 Now you can change your password:

 ```shell
@@ -35,6 +39,14 @@ user.password_confirmation = 'secret_pass'

 It's important that you change both password and password_confirmation to make it work.

+When using this method instead of the [Users API](../api/users.md#user-modification), GitLab sends an email to the user stating that the user changed their password.
+
+If the password was changed by an administrator, execute the following command to notify the user by email:
+
+```shell
+user.send_only_admin_changed_your_password_notification!
+```
+
 Don't forget to save the changes.

 ```shell
@@ -43,6 +55,19 @@ user.save!

 Exit the console and try to login with your new password.

+NOTE: **Note:**
+Passwords can also be reset via the [Users API](../api/users.md#user-modification)
+
+### Reset your root password
+
+The steps described above can also be used to reset the root password. But first, identify the root user, with an `id` of `1`. To do so, run the following command:
+
+```shell
+user = User.where(id: 1).first
+```
+
+After finding the user, follow the steps mentioned in the [Reset the password](#reset-the-password) section to reset the password of the root user.
+
 <!-- ## Troubleshooting

 Include any troubleshooting steps that you can foresee. If you know beforehand what issues

--- a/doc/user/profile/notifications.md
+++ b/doc/user/profile/notifications.md
@@ -143,7 +143,8 @@ Users will be notified of the following events:
 | New SSH key added            | User                | Security email, always sent. |
 | New email added              | User                | Security email, always sent. |
 | Email changed                | User                | Security email, always sent. |
-| Password changed             | User                | Security email, always sent. |
+| Password changed             | User                | Security email, always sent when user changes their own password |
+| Password changed by administrator | User | Security email, always sent when an adminstrator changes the password of another user |
 | 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 |

--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -218,9 +218,15 @@ def reorder_users(users)
                .where.not(id: user.id).exists?

        user_params = declared_params(include_missing: false)
+        admin_making_changes_for_another_user = (current_user != user)

-        user_params[:password_expires_at] = Time.current if user_params[:password].present?
-        result = ::Users::UpdateService.new(current_user, user_params.merge(user: user)).execute
+        if user_params[:password].present?
+          user_params[:password_expires_at] = Time.current if admin_making_changes_for_another_user
+        end
+
+        result = ::Users::UpdateService.new(current_user, user_params.merge(user: user)).execute do |user|
+          user.send_only_admin_changed_your_password_notification! if admin_making_changes_for_another_user
+        end

        if result[:status] == :success
          present user, with: Entities::UserWithAdmin, current_user: current_user

--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -2540,6 +2540,9 @@ msgstr ""
 msgid "An %{link_start}alert%{link_end} with the same fingerprint is already open. To change the status of this alert, resolve the linked alert."
 msgstr ""
  
+msgid "An administrator changed the password for your GitLab account on %{link_to}."
+msgstr ""
+
 msgid "An alert has been triggered in %{project_path}."
 msgstr ""
  
@@ -12565,6 +12568,9 @@ msgstr ""
 msgid "Hello there"
 msgstr ""
  
+msgid "Hello, %{username}!"
+msgstr ""
+
 msgid "Help"
 msgstr ""
  
@@ -18304,6 +18310,9 @@ msgstr ""
 msgid "Please complete your profile with email address"
 msgstr ""
  
+msgid "Please contact your administrator with any questions."
+msgstr ""
+
 msgid "Please contact your administrator."
 msgstr ""
  

--- a/spec/controllers/admin/users_controller_spec.rb
+++ b/spec/controllers/admin/users_controller_spec.rb
@@ -286,82 +286,111 @@

  describe 'POST update' do
    context 'when the password has changed' do
-      def update_password(user, password, password_confirmation = nil)
+      def update_password(user, password = User.random_password, password_confirmation = password)
        params = {
          id: user.to_param,
          user: {
            password: password,
-            password_confirmation: password_confirmation || password
+            password_confirmation: password_confirmation
          }
        }

        post :update, params: params
      end

-      context 'when the admin changes their own password' do
-        it 'updates the password' do
-          expect { update_password(admin, 'AValidPassword1') }
-            .to change { admin.reload.encrypted_password }
-        end
+      context 'when admin changes their own password' do
+        context 'when password is valid' do
+          it 'updates the password' do
+            expect { update_password(admin) }
+              .to change { admin.reload.encrypted_password }
+          end
+
+          it 'does not set the new password to expire immediately' do
+            expect { update_password(admin) }
+              .not_to change { admin.reload.password_expired? }
+          end
+
+          it 'does not enqueue the `admin changed your password` email' do
+            expect { update_password(admin) }
+              .not_to have_enqueued_mail(DeviseMailer, :password_change_by_admin)
+          end

-        it 'does not set the new password to expire immediately' do
-          expect { update_password(admin, 'AValidPassword1') }
-            .not_to change { admin.reload.password_expires_at }
+          it 'enqueues the `password changed` email' do
+            expect { update_password(admin) }
+              .to have_enqueued_mail(DeviseMailer, :password_change)
+          end
        end
      end

-      context 'when the new password is valid' do
-        it 'redirects to the user' do
-          update_password(user, 'AValidPassword1')
+      context 'when admin changes the password of another user' do
+        context 'when the new password is valid' do
+          it 'redirects to the user' do
+            update_password(user)

-          expect(response).to redirect_to(admin_user_path(user))
-        end
+            expect(response).to redirect_to(admin_user_path(user))
+          end

-        it 'updates the password' do
-          expect { update_password(user, 'AValidPassword1') }
-            .to change { user.reload.encrypted_password }
-        end
+          it 'updates the password' do
+            expect { update_password(user) }
+              .to change { user.reload.encrypted_password }
+          end

-        it 'sets the new password to expire immediately' do
-          expect { update_password(user, 'AValidPassword1') }
-            .to change { user.reload.password_expires_at }.to be_within(2.seconds).of(Time.current)
+          it 'sets the new password to expire immediately' do
+            expect { update_password(user) }
+              .to change { user.reload.password_expired? }.from(false).to(true)
+          end
+
+          it 'enqueues the `admin changed your password` email' do
+            expect { update_password(user) }
+              .to have_enqueued_mail(DeviseMailer, :password_change_by_admin)
+          end
+
+          it 'does not enqueue the `password changed` email' do
+            expect { update_password(user) }
+              .not_to have_enqueued_mail(DeviseMailer, :password_change)
+          end
        end
      end

      context 'when the new password is invalid' do
+        let(:password) { 'invalid' }
+
        it 'shows the edit page again' do
-          update_password(user, 'invalid')
+          update_password(user, password)

          expect(response).to render_template(:edit)
        end

        it 'returns the error message' do
-          update_password(user, 'invalid')
+          update_password(user, password)

          expect(assigns[:user].errors).to contain_exactly(a_string_matching(/too short/))
        end

        it 'does not update the password' do
-          expect { update_password(user, 'invalid') }
+          expect { update_password(user, password) }
            .not_to change { user.reload.encrypted_password }
        end
      end

      context 'when the new password does not match the password confirmation' do
+        let(:password) { 'some_password' }
+        let(:password_confirmation) { 'not_same_as_password' }
+
        it 'shows the edit page again' do
-          update_password(user, 'AValidPassword1', 'AValidPassword2')
+          update_password(user, password, password_confirmation)

          expect(response).to render_template(:edit)
        end

        it 'returns the error message' do
-          update_password(user, 'AValidPassword1', 'AValidPassword2')
+          update_password(user, password, password_confirmation)

          expect(assigns[:user].errors).to contain_exactly(a_string_matching(/doesn't match/))
        end

        it 'does not update the password' do
-          expect { update_password(user, 'AValidPassword1', 'AValidPassword2') }
+          expect { update_password(user, password, password_confirmation) }
            .not_to change { user.reload.encrypted_password }
        end
      end

--- a/spec/helpers/emails_helper_spec.rb
+++ b/spec/helpers/emails_helper_spec.rb
@@ -118,6 +118,14 @@
    end
  end

+  describe '#say_hello' do
+    let(:user) { build(:user, name: 'John') }
+
+    it 'returns the greeting message for the given user' do
+      expect(say_hello(user)).to eq('Hello, 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(
@@ -145,6 +153,33 @@
    end
  end

+  describe '#admin_changed_password_text' do
+    context 'format is html' do
+      it 'returns HTML' do
+        expect(admin_changed_password_text(format: :html)).to eq(
+          "An administrator changed the password for your GitLab account on " \
+          "#{link_to(Gitlab.config.gitlab.url, Gitlab.config.gitlab.url, target: :_blank, rel: 'noopener noreferrer')}."
+        )
+      end
+    end
+
+    context 'format is not specified' do
+      it 'returns text' do
+        expect(admin_changed_password_text).to eq(
+          "An administrator changed the password for your GitLab account on #{Gitlab.config.gitlab.url}."
+        )
+      end
+    end
+  end
+
+  describe '#contact_your_administrator_text' do
+    it 'returns the message to contact the administrator' do
+      expect(contact_your_administrator_text).to eq(
+        _('Please contact your administrator with any questions.')
+      )
+    end
+  end
+
  describe 'password_reset_token_valid_time' do
    def validate_time_string(time_limit, expected_string)
      Devise.reset_password_within = time_limit

--- a/spec/mailers/devise_mailer_spec.rb
+++ b/spec/mailers/devise_mailer_spec.rb
@@ -4,6 +4,9 @@
 require 'email_spec'

 RSpec.describe DeviseMailer do
+  include EmailSpec::Matchers
+  include_context 'gitlab email notification'
+
  describe "#confirmation_instructions" do
    subject { described_class.confirmation_instructions(user, 'faketoken', {}) }

@@ -35,4 +38,30 @@
      end
    end
  end
+
+  describe '#password_change_by_admin' do
+    subject { described_class.password_change_by_admin(user) }
+
+    let_it_be(:user) { create(: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 /^Password changed by administrator$/i
+    end
+
+    it 'includes the correct content' do
+      is_expected.to have_body_text /An administrator changed the password for your GitLab account/
+    end
+
+    it 'includes a link to GitLab' do
+      is_expected.to have_body_text /#{Gitlab.config.gitlab.url}/
+    end
+  end
 end
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -180,6 +180,58 @@
        end.to have_enqueued_job.on_queue('mailers').exactly(:twice)
      end
    end
+
+    context 'emails sent on changing password' do
+      context 'when password is updated' do
+        context 'default behaviour' do
+          it 'enqueues the `password changed` email' do
+            user.password = User.random_password
+
+            expect { user.save! }.to have_enqueued_mail(DeviseMailer, :password_change)
+          end
+
+          it 'does not enqueue the `admin changed your password` email' do
+            user.password = User.random_password
+
+            expect { user.save! }.not_to have_enqueued_mail(DeviseMailer, :password_change_by_admin)
+          end
+        end
+
+        context '`admin changed your password` email' do
+          it 'is enqueued only when explicitly allowed' do
+            user.password = User.random_password
+            user.send_only_admin_changed_your_password_notification!
+
+            expect { user.save! }.to have_enqueued_mail(DeviseMailer, :password_change_by_admin)
+          end
+
+          it '`password changed` email is not enqueued if it is explicitly allowed' do
+            user.password = User.random_password
+            user.send_only_admin_changed_your_password_notification!
+
+            expect { user.save! }.not_to have_enqueued_mail(DeviseMailer, :password_changed)
+          end
+
+          it 'is not enqueued if sending notifications on password updates is turned off as per Devise config' do
+            user.password = User.random_password
+            user.send_only_admin_changed_your_password_notification!
+
+            allow(Devise).to receive(:send_password_change_notification).and_return(false)
+
+            expect { user.save! }.not_to have_enqueued_mail(DeviseMailer, :password_change_by_admin)
+          end
+        end
+      end
+
+      context 'when password is not updated' do
+        it 'does not enqueue the `admin changed your password` email even if explicitly allowed' do
+          user.name = 'John'
+          user.send_only_admin_changed_your_password_notification!
+
+          expect { user.save! }.not_to have_enqueued_mail(DeviseMailer, :password_change_by_admin)
+        end
+      end
+    end
  end

  describe 'validations' do

--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -914,6 +914,50 @@
      expect(response).to have_gitlab_http_status(:ok)
    end

+    context 'updating password' do
+      def update_password(user, admin, password = User.random_password)
+        put api("/users/#{user.id}", admin), params: { password: password }
+      end
+
+      context 'admin updates their own password' do
+        it 'does not force reset on next login' do
+          update_password(admin, admin)
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(user.reload.password_expired?).to eq(false)
+        end
+
+        it 'does not enqueue the `admin changed your password` email' do
+          expect { update_password(admin, admin) }
+            .not_to have_enqueued_mail(DeviseMailer, :password_change_by_admin)
+        end
+
+        it 'enqueues the `password changed` email' do
+          expect { update_password(admin, admin) }
+            .to have_enqueued_mail(DeviseMailer, :password_change)
+        end
+      end
+
+      context 'admin updates the password of another user' do
+        it 'forces reset on next login' do
+          update_password(user, admin)
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(user.reload.password_expired?).to eq(true)
+        end
+
+        it 'enqueues the `admin changed your password` email' do
+          expect { update_password(user, admin) }
+            .to have_enqueued_mail(DeviseMailer, :password_change_by_admin)
+        end
+
+        it 'does not enqueue the `password changed` email' do
+          expect { update_password(user, admin) }
+            .not_to have_enqueued_mail(DeviseMailer, :password_change)
+        end
+      end
+    end
+
    it "updates user with new bio" do
      put api("/users/#{user.id}", admin), params: { bio: 'new test bio' }

@@ -940,13 +984,6 @@
      expect(user.reload.bio).to eq('')
    end

-    it "updates user with new password and forces reset on next login" do
-      put api("/users/#{user.id}", admin), params: { password: '12345678' }
-
-      expect(response).to have_gitlab_http_status(:ok)
-      expect(user.reload.password_expires_at).to be <= Time.now
-    end
-
    it "updates user with organization" do
      put api("/users/#{user.id}", admin), params: { organization: 'GitLab' }

@@ -1397,7 +1434,7 @@
    end
  end

-  describe 'POST /users/:id/keys' do
+  describe 'POST /users/:id/gpg_keys' do
    it 'does not create invalid GPG key' do
      post api("/users/#{user.id}/gpg_keys", admin)