From d77ce23de2d265125496fa709d18613b94d57400 Mon Sep 17 00:00:00 2001 From: Navin Date: Mon, 8 Jul 2013 11:53:22 +0200 Subject: [PATCH 1/2] Log all changes of user trust level by an admin --- app/controllers/admin/users_controller.rb | 3 ++- app/models/admin_log.rb | 2 +- lib/admin_logger.rb | 12 ++++++++- lib/boost_trust_level.rb | 8 +++--- spec/components/admin_logger_spec.rb | 26 +++++++++++++++++++ spec/components/boost_trust_level_spec.rb | 22 ++++++++++++---- .../admin/users_controller_spec.rb | 2 ++ 7 files changed, 64 insertions(+), 11 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 6c1e1de5ca7..b097ed82573 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -72,7 +72,8 @@ class Admin::UsersController < Admin::AdminController def trust_level guardian.ensure_can_change_trust_level!(@user) - BoostTrustLevel.new(@user, params[:level]).save! + logger = AdminLogger.new(current_user) + BoostTrustLevel.new(user: @user, level: params[:level], logger: logger).save! render_serialized(@user, AdminUserSerializer) end diff --git a/app/models/admin_log.rb b/app/models/admin_log.rb index 71dd383a12a..405bac7f79e 100644 --- a/app/models/admin_log.rb +++ b/app/models/admin_log.rb @@ -9,7 +9,7 @@ class AdminLog < ActiveRecord::Base validates_presence_of :action def self.actions - @actions ||= Enum.new(:delete_user) + @actions ||= Enum.new(:delete_user, :change_trust_level) end end diff --git a/lib/admin_logger.rb b/lib/admin_logger.rb index 53684172846..4adbf90d56d 100644 --- a/lib/admin_logger.rb +++ b/lib/admin_logger.rb @@ -13,4 +13,14 @@ class AdminLogger details: [:id, :username, :name, :created_at, :trust_level, :last_seen_at, :last_emailed_at].map { |x| "#{x}: #{deleted_user.send(x)}" }.join(', ') ) end -end \ No newline at end of file + + def log_trust_level_change(user, new_trust_level) + raise Discourse::InvalidParameters.new('user is nil') unless user and user.is_a?(User) + raise Discourse::InvalidParameters.new('new trust level is invalid') unless TrustLevel.levels.values.include? new_trust_level + AdminLog.create!( + action: AdminLog.actions[:change_trust_level], + admin_id: @admin.id, + details: [:id, :username, :name, :created_at, :trust_level, :last_seen_at, :last_emailed_at].map { |x| "#{x}: #{user.send(x)}" }.join(', ') + "new trust level: #{new_trust_level}" + ) + end +end diff --git a/lib/boost_trust_level.rb b/lib/boost_trust_level.rb index 597f380866e..8691c1a1b06 100644 --- a/lib/boost_trust_level.rb +++ b/lib/boost_trust_level.rb @@ -2,14 +2,16 @@ require_dependency 'promotion' class BoostTrustLevel - def initialize(user, level) - @user = user - @level = level.to_i + def initialize(args) + @user = args[:user] + @level = args[:level].to_i @promotion = Promotion.new(@user) @trust_levels = TrustLevel.levels + @logger = args[:logger] end def save! + @logger.log_trust_level_change(@user, @level) if @level < @user.trust_level demote! else diff --git a/spec/components/admin_logger_spec.rb b/spec/components/admin_logger_spec.rb index 5162cf9bc23..f98517e6b5e 100644 --- a/spec/components/admin_logger_spec.rb +++ b/spec/components/admin_logger_spec.rb @@ -32,4 +32,30 @@ describe AdminLogger do end end + describe 'log_trust_level_change' do + let(:admin) { Fabricate(:admin) } + let(:user) { Fabricate(:user) } + let(:new_trust_level) { TrustLevel.levels[:basic] } + + subject(:log_trust_level_change) { AdminLogger.new(admin).log_trust_level_change(user, new_trust_level) } + + it 'raises an error when user or trust level is nil' do + expect { AdminLogger.new(admin).log_trust_level_change(nil, new_trust_level) }.to raise_error(Discourse::InvalidParameters) + expect { AdminLogger.new(admin).log_trust_level_change(user, nil) }.to raise_error(Discourse::InvalidParameters) + end + + it 'raises an error when user is not a User' do + expect { AdminLogger.new(admin).log_trust_level_change(1, new_trust_level) }.to raise_error(Discourse::InvalidParameters) + end + + it 'raises an error when new trust level is not a Trust Level' do + max_level = TrustLevel.levels.values.max + expect { AdminLogger.new(admin).log_trust_level_change(user, max_level + 1) }.to raise_error(Discourse::InvalidParameters) + end + + it 'creates a new AdminLog record' do + expect { log_trust_level_change }.to change { AdminLog.count }.by(1) + AdminLog.last.details.should include "new trust level: #{new_trust_level}" + end + end end diff --git a/spec/components/boost_trust_level_spec.rb b/spec/components/boost_trust_level_spec.rb index 26412079337..b7fd528f7ef 100644 --- a/spec/components/boost_trust_level_spec.rb +++ b/spec/components/boost_trust_level_spec.rb @@ -1,16 +1,25 @@ require 'spec_helper' require 'boost_trust_level' +require 'admin_logger' describe BoostTrustLevel do let(:user) { Fabricate(:user) } + let(:logger) { AdminLogger.new(Fabricate(:admin)) } + it "should upgrade the trust level of a user" do - boostr = BoostTrustLevel.new(user, TrustLevel.levels[:basic]) + boostr = BoostTrustLevel.new(user: user, level: TrustLevel.levels[:basic], logger: logger) boostr.save!.should be_true user.trust_level.should == TrustLevel.levels[:basic] end + it "should log the action" do + AdminLogger.any_instance.expects(:log_trust_level_change).with(user, TrustLevel.levels[:basic]).once + boostr = BoostTrustLevel.new(user: user, level: TrustLevel.levels[:basic], logger: logger) + boostr.save! + end + describe "demotions" do before { user.update_attributes(trust_level: TrustLevel.levels[:newuser]) } @@ -21,8 +30,9 @@ describe BoostTrustLevel do user.update_attributes(trust_level: TrustLevel.levels[:basic]) end - it "should demote the user" do - boostr = BoostTrustLevel.new(user, TrustLevel.levels[:newuser]) + it "should demote the user and log the action" do + AdminLogger.any_instance.expects(:log_trust_level_change).with(user, TrustLevel.levels[:newuser]).once + boostr = BoostTrustLevel.new(user: user, level: TrustLevel.levels[:newuser], logger: logger) boostr.save!.should be_true user.trust_level.should == TrustLevel.levels[:newuser] end @@ -38,11 +48,13 @@ describe BoostTrustLevel do user.update_attributes(trust_level: TrustLevel.levels[:basic]) end - it "should not demote the user" do - boostr = BoostTrustLevel.new(user, TrustLevel.levels[:newuser]) + it "should not demote the user but log the action anyway" do + AdminLogger.any_instance.expects(:log_trust_level_change).with(user, TrustLevel.levels[:newuser]).once + boostr = BoostTrustLevel.new(user: user, level: TrustLevel.levels[:newuser], logger: logger) expect { boostr.save! }.to raise_error(Discourse::InvalidAccess, "You attempted to demote #{user.name} to 'newuser'. However their trust level is already 'basic'. #{user.name} will remain at 'basic'") user.trust_level.should == TrustLevel.levels[:basic] end + end end end diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 35fbaab5887..239a5a88f3a 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -137,12 +137,14 @@ describe Admin::UsersController do end it "upgrades the user's trust level" do + AdminLogger.any_instance.expects(:log_trust_level_change).with(@another_user, 2).once xhr :put, :trust_level, user_id: @another_user.id, level: 2 @another_user.reload @another_user.trust_level.should == 2 end it "raises an error when demoting a user below their current trust level" do + AdminLogger.any_instance.expects(:log_trust_level_change).with(@another_user, TrustLevel.levels[:newuser]).once @another_user.topics_entered = SiteSetting.basic_requires_topics_entered + 1 @another_user.posts_read_count = SiteSetting.basic_requires_read_posts + 1 @another_user.time_read = SiteSetting.basic_requires_time_spent_mins * 60 From 45d85f40546bcb971277dff3c801cfc46e35974b Mon Sep 17 00:00:00 2001 From: Navin Date: Mon, 8 Jul 2013 12:51:35 +0200 Subject: [PATCH 2/2] If the change doesn't go through, don't log anything --- lib/boost_trust_level.rb | 11 ++++++----- spec/components/boost_trust_level_spec.rb | 4 ++-- spec/controllers/admin/users_controller_spec.rb | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/boost_trust_level.rb b/lib/boost_trust_level.rb index 8691c1a1b06..50785270ea0 100644 --- a/lib/boost_trust_level.rb +++ b/lib/boost_trust_level.rb @@ -11,12 +11,13 @@ class BoostTrustLevel end def save! + success = if @level < @user.trust_level + demote! + else + @user.update_attributes!(trust_level: @level) + end @logger.log_trust_level_change(@user, @level) - if @level < @user.trust_level - demote! - else - @user.update_attributes!(trust_level: @level) - end + success end protected diff --git a/spec/components/boost_trust_level_spec.rb b/spec/components/boost_trust_level_spec.rb index b7fd528f7ef..3974aecbb62 100644 --- a/spec/components/boost_trust_level_spec.rb +++ b/spec/components/boost_trust_level_spec.rb @@ -48,8 +48,8 @@ describe BoostTrustLevel do user.update_attributes(trust_level: TrustLevel.levels[:basic]) end - it "should not demote the user but log the action anyway" do - AdminLogger.any_instance.expects(:log_trust_level_change).with(user, TrustLevel.levels[:newuser]).once + it "should not demote the user and not log the action" do + AdminLogger.any_instance.expects(:log_trust_level_change).with(user, TrustLevel.levels[:newuser]).never boostr = BoostTrustLevel.new(user: user, level: TrustLevel.levels[:newuser], logger: logger) expect { boostr.save! }.to raise_error(Discourse::InvalidAccess, "You attempted to demote #{user.name} to 'newuser'. However their trust level is already 'basic'. #{user.name} will remain at 'basic'") user.trust_level.should == TrustLevel.levels[:basic] diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 239a5a88f3a..036e557bb0a 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -144,7 +144,7 @@ describe Admin::UsersController do end it "raises an error when demoting a user below their current trust level" do - AdminLogger.any_instance.expects(:log_trust_level_change).with(@another_user, TrustLevel.levels[:newuser]).once + AdminLogger.any_instance.expects(:log_trust_level_change).with(@another_user, TrustLevel.levels[:newuser]).never @another_user.topics_entered = SiteSetting.basic_requires_topics_entered + 1 @another_user.posts_read_count = SiteSetting.basic_requires_read_posts + 1 @another_user.time_read = SiteSetting.basic_requires_time_spent_mins * 60