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

--------------------------------------------------------------------------------
Setting the target version to 4.2.0.
--------------------------------------------------------------------------------
Thank you for posting the patch.

The patch attachment: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

--------------------------------------------------------------------------------
Go MAEDA wrote:
> Thank you for posting the patch.
>
> The patch attachment: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.
--------------------------------------------------------------------------------
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.

<pre><code class="diff">
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
</code></pre>
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Committed the patch. Thank you for your contribution.

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