Merge branch...

Merge branch '27284-indicate-that-password-reset-was-initiated-by-an-admin-when-an-admin-resets-a-user-password' into 'master'

Resolve "Send a specific `Password changed by Admin` email when an admin resets the password of another user"

See merge request gitlab-org/gitlab!40342

app/controllers/admin/users_controller.rb

@@ -149,7 +149,7 @@ class Admin::UsersController < Admin::ApplicationController
         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 @@ class Admin::UsersController < Admin::ApplicationController
     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 @@ class Admin::UsersController < Admin::ApplicationController
 
   protected
 
-  def changing_own_password?
-    user == current_user
+  def admin_making_changes_for_another_user?
+    user != current_user
   end
 
   def user

app/helpers/emails_helper.rb

@@ -181,6 +181,10 @@ module EmailsHelper
     _('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 @@ module EmailsHelper
 
     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 @@ module EmailsHelper
     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

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)

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

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

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

app/views/devise/mailer/password_change_by_admin.text.erb

+<%= say_hello(@resource) %>
+
+<%= admin_changed_password_text %>
+
+<%= contact_your_administrator_text %>

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

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."

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)

doc/security/reset_root_password.md → doc/security/reset_user_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

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 |

lib/api/users.rb

@@ -218,9 +218,15 @@ module API
                 .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

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 ""
 

spec/controllers/admin/users_controller_spec.rb

@@ -286,82 +286,111 @@ RSpec.describe Admin::UsersController do
 
   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
+      context 'when admin changes their own password' do
+        context 'when password is valid' do
           it 'updates the password' do
-          expect { update_password(admin, 'AValidPassword1') }
+            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, 'AValidPassword1') }
-            .not_to change { admin.reload.password_expires_at }
+            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 'enqueues the `password changed` email' do
+            expect { update_password(admin) }
+              .to have_enqueued_mail(DeviseMailer, :password_change)
+          end
         end
       end
 
+      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, 'AValidPassword1')
+            update_password(user)
 
             expect(response).to redirect_to(admin_user_path(user))
           end
 
           it 'updates the password' do
-          expect { update_password(user, 'AValidPassword1') }
+            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)
+            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

spec/helpers/emails_helper_spec.rb

@@ -118,6 +118,14 @@ RSpec.describe EmailsHelper do
     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 @@ RSpec.describe EmailsHelper do
     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

spec/mailers/devise_mailer_spec.rb

@@ -4,6 +4,9 @@ require 'spec_helper'
 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 @@ RSpec.describe DeviseMailer do
       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

spec/models/user_spec.rb

@@ -180,6 +180,58 @@ RSpec.describe User do
         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

spec/requests/api/users_spec.rb

@@ -914,6 +914,50 @@ RSpec.describe API::Users, :do_not_mock_admin_mode do
       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 @@ RSpec.describe API::Users, :do_not_mock_admin_mode do
       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 @@ RSpec.describe API::Users, :do_not_mock_admin_mode do
     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)