From b38260ff448b75a7a4c1bf87a07c960ec72b97a5 Mon Sep 17 00:00:00 2001 From: Jan Schulz-Hofen Date: Wed, 2 Dec 2015 18:47:24 +0800 Subject: [PATCH 7/7] Send a security notification when users gain or loose admin privileges --- app/models/user.rb | 31 ++++++- test/functional/users_controller_test.rb | 144 +++++++++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index cfffd7c..8bb44f9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -118,7 +118,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 @@ -831,6 +832,34 @@ class User < Principal 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 class AnonymousUser < User diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index b34c809..94b2e21 100644 --- 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 -- 2.4.0