Send a security notification when users gain or loose admin (#21421).

Patch by Jan Schulz-Hofen.

git-svn-id: http://svn.redmine.org/redmine/trunk@15265 e93f8b46-1217-0410-a6f0-8f06a7374b81

diff --git a/app/models/user.rb b/app/models/user.rb
index e2a7c4559720a73d9297050ae6f69a4cca7b6089..dca47281052370fcf6bdd8e28216a69a4d67701f 100644 (file)
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -123,7 +123,8 @@ class User < Principal
   before_create :set_mail_notification
   before_save   :generate_password_if_needed, :update_hashed_password
   before_destroy :remove_references_before_destroy
-  after_save :update_notified_project_ids, :destroy_tokens
+  after_save :update_notified_project_ids, :destroy_tokens, :deliver_security_notification
+  after_destroy :deliver_security_notification
 
   scope :in_group, lambda {|group|
     group_id = group.is_a?(Group) ? group.id : group.to_i
@@ -835,6 +836,34 @@ class User < Principal
   def self.generate_salt
     Redmine::Utils.random_hex(16)
   end
+  # Send a security notification to all admins if the user has gained/lost admin privileges
+  def deliver_security_notification
+    options = {
+      field: :field_admin,
+      value: login,
+      title: :label_user_plural,
+      url: {controller: 'users', action: 'index'}
+    }
+    deliver = false
+    if (admin? && id_changed? && active?) ||    # newly created admin
+       (admin? && admin_changed? && active?) || # regular user became admin
+       (admin? && status_changed? && active?)   # locked admin became active again
+
+       deliver = true
+       options[:message] = :mail_body_security_notification_add
+
+    elsif (admin? && destroyed? && active?) ||      # active admin user was deleted
+          (!admin? && admin_changed? && active?) || # admin is no longer admin
+          (admin? && status_changed? && !active?)   # admin was locked
+
+          deliver = true
+          options[:message] = :mail_body_security_notification_remove
+    end
+
+    User.where(admin: true, status: Principal::STATUS_ACTIVE).each{|u| Mailer.security_notification(u, options).deliver} if deliver
+  end
+
+
 
 end
 

diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb
index 86c92e937eb831a2877fb28c91c7ccc31a808eb4..7c22dedf20d8c7c2d5efd30d3906b14589835f28 100644 (file)
--- a/test/functional/users_controller_test.rb
+++ b/test/functional/users_controller_test.rb
@@ -280,6 +280,48 @@ class UsersControllerTest < ActionController::TestCase
     assert_select 'input#pref_no_self_notified[value="1"][checked=checked]'
   end
 
+  def test_create_admin_should_send_security_notification
+    ActionMailer::Base.deliveries.clear
+    post :create,
+      :user => {
+        :firstname => 'Edgar',
+        :lastname => 'Schmoe',
+        :login => 'eschmoe',
+        :password => 'secret123',
+        :password_confirmation => 'secret123',
+        :mail => 'eschmoe@example.foo',
+        :admin => '1'
+      }
+
+    assert_not_nil (mail = ActionMailer::Base.deliveries.last)
+    assert_mail_body_match '0.0.0.0', mail
+    assert_mail_body_match I18n.t(:mail_body_security_notification_add, field: I18n.t(:field_admin), value: 'eschmoe'), mail
+    assert_select_email do
+      assert_select 'a[href^=?]', 'http://localhost:3000/users', :text => 'Users'
+    end
+
+    # All admins should receive this
+    User.where(admin: true, status: Principal::STATUS_ACTIVE).each do |admin|
+      assert_not_nil ActionMailer::Base.deliveries.detect{|mail| [mail.bcc, mail.cc].flatten.include?(admin.mail) }
+    end
+  end
+
+  def test_create_non_admin_should_not_send_security_notification
+    ActionMailer::Base.deliveries.clear
+    post :create,
+      :user => {
+        :firstname => 'Edgar',
+        :lastname => 'Schmoe',
+        :login => 'eschmoe',
+        :password => 'secret123',
+        :password_confirmation => 'secret123',
+        :mail => 'eschmoe@example.foo',
+        :admin => '0'
+      }
+    assert_nil ActionMailer::Base.deliveries.last
+  end
+
+
   def test_edit
     get :edit, :id => 2
     assert_response :success
@@ -426,6 +468,92 @@ class UsersControllerTest < ActionController::TestCase
     assert_equal '1', user.pref[:no_self_notified]
   end
 
+  def test_update_assign_admin_should_send_security_notification
+    ActionMailer::Base.deliveries.clear
+    put :update, :id => 2, :user => {
+      :admin => 1
+    }
+
+    assert_not_nil (mail = ActionMailer::Base.deliveries.last)
+    assert_mail_body_match I18n.t(:mail_body_security_notification_add, field: I18n.t(:field_admin), value: User.find(2).login), mail
+
+    # All admins should receive this
+    User.where(admin: true, status: Principal::STATUS_ACTIVE).each do |admin|
+      assert_not_nil ActionMailer::Base.deliveries.detect{|mail| [mail.bcc, mail.cc].flatten.include?(admin.mail) }
+    end
+  end
+
+  def test_update_unassign_admin_should_send_security_notification
+    user = User.find(2)
+    user.admin = true
+    user.save!
+
+    ActionMailer::Base.deliveries.clear
+    put :update, :id => user.id, :user => {
+      :admin => 0
+    }
+
+    assert_not_nil (mail = ActionMailer::Base.deliveries.last)
+    assert_mail_body_match I18n.t(:mail_body_security_notification_remove, field: I18n.t(:field_admin), value: user.login), mail
+
+    # All admins should receive this
+    User.where(admin: true, status: Principal::STATUS_ACTIVE).each do |admin|
+      assert_not_nil ActionMailer::Base.deliveries.detect{|mail| [mail.bcc, mail.cc].flatten.include?(admin.mail) }
+    end
+  end
+
+  def test_update_lock_admin_should_send_security_notification
+    user = User.find(2)
+    user.admin = true
+    user.save!
+
+    ActionMailer::Base.deliveries.clear
+    put :update, :id => 2, :user => {
+      :status => Principal::STATUS_LOCKED
+    }
+
+    assert_not_nil (mail = ActionMailer::Base.deliveries.last)
+    assert_mail_body_match I18n.t(:mail_body_security_notification_remove, field: I18n.t(:field_admin), value: User.find(2).login), mail
+
+    # All admins should receive this
+    User.where(admin: true, status: Principal::STATUS_ACTIVE).each do |admin|
+      assert_not_nil ActionMailer::Base.deliveries.detect{|mail| [mail.bcc, mail.cc].flatten.include?(admin.mail) }
+    end
+
+    # if user is already locked, destroying should not send a second mail
+    # (for active admins see furtherbelow)
+    ActionMailer::Base.deliveries.clear
+    delete :destroy, :id => 1
+    assert_nil ActionMailer::Base.deliveries.last
+
+  end
+
+  def test_update_unlock_admin_should_send_security_notification
+    user = User.find(5) # already locked
+    user.admin = true
+    user.save!
+    ActionMailer::Base.deliveries.clear
+    put :update, :id => user.id, :user => {
+      :status => Principal::STATUS_ACTIVE
+    }
+
+    assert_not_nil (mail = ActionMailer::Base.deliveries.last)
+    assert_mail_body_match I18n.t(:mail_body_security_notification_add, field: I18n.t(:field_admin), value: user.login), mail
+
+    # All admins should receive this
+    User.where(admin: true, status: Principal::STATUS_ACTIVE).each do |admin|
+      assert_not_nil ActionMailer::Base.deliveries.detect{|mail| [mail.bcc, mail.cc].flatten.include?(admin.mail) }
+    end
+  end
+
+  def test_update_admin_unrelated_property_should_not_send_security_notification
+    ActionMailer::Base.deliveries.clear
+    put :update, :id => 1, :user => {
+      :firstname => 'Jimmy'
+    }
+    assert_nil ActionMailer::Base.deliveries.last
+  end
+
   def test_destroy
     assert_difference 'User.count', -1 do
       delete :destroy, :id => 2
@@ -449,4 +577,20 @@ class UsersControllerTest < ActionController::TestCase
     end
     assert_redirected_to '/users?name=foo'
   end
+
+  def test_destroy_active_admin_should_send_security_notification
+    user = User.find(2)
+    user.admin = true
+    user.save!
+    ActionMailer::Base.deliveries.clear
+    delete :destroy, :id => user.id
+
+    assert_not_nil (mail = ActionMailer::Base.deliveries.last)
+    assert_mail_body_match I18n.t(:mail_body_security_notification_remove, field: I18n.t(:field_admin), value: user.login), mail
+
+    # All admins should receive this
+    User.where(admin: true, status: Principal::STATUS_ACTIVE).each do |admin|
+      assert_not_nil ActionMailer::Base.deliveries.detect{|mail| [mail.bcc, mail.cc].flatten.include?(admin.mail) }
+    end
+  end
 end