diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 06d03a7146e..b2ff46cecda 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -88,7 +88,8 @@ class UsersController < ApplicationController user = fetch_user_from_params guardian.ensure_can_edit_username!(user) - result = user.change_username(params[:new_username]) + # TODO proper error surfacing (result is a Model#save call) + result = user.change_username(params[:new_username], current_user) raise Discourse::InvalidParameters.new(:new_username) unless result render json: { diff --git a/app/models/user.rb b/app/models/user.rb index f661b825fc3..8b597298f6c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -185,7 +185,11 @@ class User < ActiveRecord::Base Jobs.enqueue(:send_system_message, user_id: id, message_type: message_type) end - def change_username(new_username) + def change_username(new_username, actor=nil) + if actor && actor != self + StaffActionLogger.new(actor).log_username_change(self, self.username, new_username) + end + self.username = new_username save end diff --git a/app/models/user_history.rb b/app/models/user_history.rb index b4bfcad6a82..37722e4e579 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -34,7 +34,8 @@ class UserHistory < ActiveRecord::Base :delete_post, :delete_topic, :impersonate, - :roll_up) + :roll_up, + :change_username) end # Staff actions is a subset of all actions, used to audit actions taken by staff users. @@ -52,7 +53,8 @@ class UserHistory < ActiveRecord::Base :delete_post, :delete_topic, :impersonate, - :roll_up] + :roll_up, + :change_username] end def self.staff_action_ids diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index fc6bfdf100b..620c1138f50 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -100,6 +100,16 @@ class StaffActionLogger })) end + def log_username_change(user, old_username, new_username, opts={}) + raise Discourse::InvalidParameters.new('user is nil') unless user + UserHistory.create( params(opts).merge({ + action: UserHistory.actions[:change_username], + target_user_id: user.id, + previous_value: old_username, + new_value: new_username + })) + end + def log_user_suspend(user, reason, opts={}) raise Discourse::InvalidParameters.new('user is nil') unless user UserHistory.create( params(opts).merge({ diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 3159cf6efda..a9686319ee6 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1863,6 +1863,7 @@ en: actions: delete_user: "delete user" change_trust_level: "change trust level" + change_username: "change username" change_site_setting: "change site setting" change_site_customization: "change site customization" delete_site_customization: "delete site customization" diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index abd8cb76c64..0b3cfef5f4c 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -612,28 +612,59 @@ describe UsersController do end context 'while logged in' do - let!(:user) { log_in } - let(:new_username) { "#{user.username}1234" } + let(:old_username) { "OrigUsrname" } + let(:new_username) { "#{old_username}1234" } + let(:user) { Fabricate(:user, username: old_username) } + + before do + user.username = old_username + log_in_user(user) + end it 'raises an error without a new_username param' do expect { xhr :put, :username, username: user.username }.to raise_error(ActionController::ParameterMissing) + user.reload.username.should == old_username end it 'raises an error when you don\'t have permission to change the username' do Guardian.any_instance.expects(:can_edit_username?).with(user).returns(false) xhr :put, :username, username: user.username, new_username: new_username expect(response).to be_forbidden + user.reload.username.should == old_username end + # Bad behavior, this should give a real JSON error, not an InvalidParameters it 'raises an error when change_username fails' do - User.any_instance.expects(:change_username).with(new_username).returns(false) + User.any_instance.expects(:save).returns(false) expect { xhr :put, :username, username: user.username, new_username: new_username }.to raise_error(Discourse::InvalidParameters) + user.reload.username.should == old_username end - it 'should succeed when the change_username returns true' do - User.any_instance.expects(:change_username).with(new_username).returns(true) + it 'should succeed in normal circumstances' do xhr :put, :username, username: user.username, new_username: new_username - expect(response).to be_success + response.should be_success + user.reload.username.should == new_username + end + + pending 'should fail if the user is old', 'ensure_can_edit_username! is not throwing' do + # Older than the change period and >1 post + user.created_at = Time.now - (SiteSetting.username_change_period + 1).days + user.stubs(:post_count).returns(200) + Guardian.new(user).can_edit_username?(user).should == false + + xhr :put, :username, username: user.username, new_username: new_username + + response.should be_forbidden + user.reload.username.should == old_username + end + + it 'should create a staff action log when a staff member changes the username' do + acting_user = Fabricate(:admin) + log_in_user(acting_user) + xhr :put, :username, username: user.username, new_username: new_username + response.should be_success + UserHistory.where(action: UserHistory.actions[:change_username], target_user_id: user.id, acting_user_id: acting_user.id).should be_present + user.reload.username.should == new_username end it 'should return a JSON response with the updated username' do