Project

General

Profile

Actions

Defect #32199

closed

Security notification is not sent when an admin changes the password of a user

Added by Go MAEDA over 4 years ago. Updated almost 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Email notifications
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:

Description

Security notifications should be sent when admin changes a user's password in order to prevent admins from changing a user's password for malicious purposes.

See the table below. It describes the current behavior. Security notifications for change of email address are sent even when the change is made by admins. However, security notifications for change of password are not sent if the change is made by admins. The behavior is inconsistent.

by the user by admins
Change of password -
Change of email address

Files

Actions #1

Updated by Yuichi HARADA over 4 years ago

Go MAEDA wrote:

Security notifications should be sent when admin changes a user's password in order to prevent admins from changing a user's password for malicious purposes.

+1
Security notification is send when an admin changes the password of users.
I attached a patch.

Actions #2

Updated by Go MAEDA over 4 years ago

  • Target version set to Candidate for next major release
Actions #3

Updated by Go MAEDA about 4 years ago

  • Target version changed from Candidate for next major release to 4.2.0

Setting the target version to 4.2.0.

Actions #4

Updated by Go MAEDA over 3 years ago

Thank you for posting the patch.

The patch 32199_change_password_by_admin.patch does not send a security notification if one of the following conditions is met:

  • The user is not active (e.g. locked)
  • The current user changes their own password on /users/:id/edit page
  • The checkbox "Send account information to the user" is checked

But I think a security notification should always be sent when an user's password has been changed. Assume the following situations. If security notifications are skipped in some conditions, a malicious person can secretly change someone's password:

  • If a notification is not sent for a locked user, a malicious admin can secretly change an active user's password while temporarily lock the user
  • If a notification is not sent when the current user's password is updated via /users/:id/edit, a malicious person can update the password secretly if the user has gone somewhere with the screen open
  • If a notification is not sent when the checkbox "Send account information to the user" is checked, the behavior is inconsistent with the case the user's email is updated
Actions #5

Updated by Yuichi HARADA over 3 years ago

Go MAEDA wrote:

Thank you for posting the patch.

The patch 32199_change_password_by_admin.patch does not send a security notification if one of the following conditions is met:

  • The user is not active (e.g. locked)
  • The current user changes their own password on /users/:id/edit page
  • The checkbox "Send account information to the user" is checked

But I think a security notification should always be sent when an user's password has been changed. Assume the following situations. If security notifications are skipped in some conditions, a malicious person can secretly change someone's password:

  • If a notification is not sent for a locked user, a malicious admin can secretly change an active user's password while temporarily lock the user
  • If a notification is not sent when the current user's password is updated via /users/:id/edit, a malicious person can update the password secretly if the user has gone somewhere with the screen open
  • If a notification is not sent when the checkbox "Send account information to the user" is checked, the behavior is inconsistent with the case the user's email is updated

Thank you for pointing out. I will revise the patch. Please wait for a while.

Actions #6

Updated by Yuichi HARADA over 3 years ago

Yuichi HARADA wrote:

Thank you for pointing out. I will revise the patch. Please wait for a while.

I remade the patch. Unconditionally send a security notification when admin changes a user password.

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 2fb297874..a1c224f7a 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -145,7 +145,8 @@ class UsersController < ApplicationController
   end

   def update
-    if params[:user][:password].present? && (@user.auth_source_id.nil? || params[:user][:auth_source_id].blank?)
+    update_password = params[:user][:password].present? && (@user.auth_source_id.nil? || params[:user][:auth_source_id].blank?)
+    if update_password
       @user.password, @user.password_confirmation = params[:user][:password], params[:user][:password_confirmation]
     end
     @user.safe_attributes = params[:user]
@@ -157,6 +158,7 @@ class UsersController < ApplicationController
     if @user.save
       @user.pref.save

+      Mailer.deliver_password_updated(@user, User.current) if update_password
       if was_activated
         Mailer.deliver_account_activated(@user)
       elsif @user.active? && params[:send_information] && @user != User.current
Actions #7

Updated by Marius BĂLTEANU about 3 years ago

  • Target version changed from 4.2.0 to 5.0.0
Actions #8

Updated by Go MAEDA almost 3 years ago

  • Subject changed from Security notification is not sent when an admin changes the password of users to Security notification is not sent when an admin changes the password of a user
  • Status changed from New to Closed
  • Assignee set to Go MAEDA
  • Resolution set to Fixed

Committed the patch. Thank you for your contribution.

Security notifications will now also be sent when an admin changes a user's password.

Actions

Also available in: Atom PDF