diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 78bbddf9fb2..a8e49aaee86 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -46,7 +46,11 @@ class Admin::UsersController < Admin::StaffController @user = User.find_by(id: params[:id]) raise Discourse::NotFound unless @user - similar_users = User.real.where.not(id: @user.id).where(ip_address: @user.ip_address) + similar_users = + User + .real + .where.not(id: @user.id) + .where(ip_address: @user.ip_address, admin: false, moderator: false) render_serialized( @user, @@ -144,44 +148,20 @@ class Admin::UsersController < Admin::StaffController user_history = nil + all_users.each { |user| raise Discourse::InvalidAccess.new if !guardian.can_suspend?(user) } + all_users.each do |user| - user.suspended_till = params[:suspend_until] - user.suspended_at = DateTime.now - - message = params[:message] - - User.transaction do - user.save! - - user_history = - StaffActionLogger.new(current_user).log_user_suspend( - user, - params[:reason], - message: message, - post_id: params[:post_id], - ) - end - user.logged_out - - if message.present? - Jobs.enqueue( - :critical_user_email, - type: "account_suspended", - user_id: user.id, - user_history_id: user_history.id, + suspender = + UserSuspender.new( + user, + suspended_till: params[:suspend_until], + reason: params[:reason], + by_user: current_user, + message: params[:message], + post_id: params[:post_id], ) - end - - DiscourseEvent.trigger( - :user_suspended, - user: user, - reason: params[:reason], - message: message, - user_history: user_history, - post_id: params[:post_id], - suspended_till: params[:suspend_until], - suspended_at: DateTime.now, - ) + suspender.suspend + user_history = suspender.user_history end perform_post_action @@ -189,7 +169,7 @@ class Admin::UsersController < Admin::StaffController render_json_dump( suspension: { suspend_reason: params[:reason], - full_suspend_reason: user_history.try(:details), + full_suspend_reason: user_history&.details, suspended_till: @user.suspended_till, suspended_at: @user.suspended_at, suspended_by: BasicUserSerializer.new(current_user, root: false).as_json, @@ -369,7 +349,6 @@ class Admin::UsersController < Admin::StaffController end def silence - guardian.ensure_can_silence_user! @user reason = params[:reason] if reason && (!reason.is_a?(String) || reason.size > 300) @@ -404,6 +383,10 @@ class Admin::UsersController < Admin::StaffController user_history = nil + all_users.each do |user| + raise Discourse::InvalidAccess.new if !guardian.can_silence_user?(user) + end + all_users.each do |user| silencer = UserSilencer.new( diff --git a/app/services/user_suspender.rb b/app/services/user_suspender.rb new file mode 100644 index 00000000000..8488b0dadb7 --- /dev/null +++ b/app/services/user_suspender.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +class UserSuspender + attr_reader :user_history + + def initialize(user, suspended_till:, reason:, by_user:, message: nil, post_id: nil) + @user = user + @suspended_till = suspended_till + @reason = reason + @by_user = by_user + @message = message + @post_id = post_id + end + + def suspend + suspended_at = DateTime.now + + @user.suspended_till = @suspended_till + @user.suspended_at = suspended_at + + @user.transaction do + @user.save! + + @user_history = + StaffActionLogger.new(@by_user).log_user_suspend( + @user, + @reason, + message: @message, + post_id: @post_id, + ) + end + @user.logged_out + + if @message.present? + Jobs.enqueue( + Jobs::CriticalUserEmail, + type: "account_suspended", + user_id: @user.id, + user_history_id: @user_history.id, + ) + end + + DiscourseEvent.trigger( + :user_suspended, + user: @user, + reason: @reason, + message: @message, + user_history: @user_history, + post_id: @post_id, + suspended_till: @suspended_till, + suspended_at: suspended_at, + ) + nil + end +end diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index 8114fe9791e..ea73e76036f 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -5,6 +5,7 @@ require "rotp" RSpec.describe Admin::UsersController do fab!(:admin) + fab!(:another_admin) { Fabricate(:admin) } fab!(:moderator) fab!(:user) fab!(:coding_horror) @@ -108,8 +109,10 @@ RSpec.describe Admin::UsersController do expect(response.parsed_body["id"]).to eq(user.id) end - it "returns similar users" do + it "includes similar users who aren't admin or mods" do Fabricate(:user, ip_address: "88.88.88.88") + Fabricate(:admin, ip_address: user.ip_address) + Fabricate(:moderator, ip_address: user.ip_address) similar_user = Fabricate(:user, ip_address: user.ip_address) get "/admin/users/#{user.id}.json" @@ -278,10 +281,37 @@ RSpec.describe Admin::UsersController do end end + shared_examples "suspension of staff users" do + it "doesn't allow suspending a staff user" do + put "/admin/users/#{another_admin.id}/suspend.json", + params: { + suspend_until: 5.hours.from_now, + reason: "naughty boy", + } + + expect(response.status).to eq(403) + expect(another_admin.reload).not_to be_suspended + end + + it "doesn't allow suspending a staff user via other_user_ids" do + put "/admin/users/#{user.id}/suspend.json", + params: { + suspend_until: 5.hours.from_now, + reason: "naughty boy", + other_user_ids: [another_admin.id], + } + + expect(response.status).to eq(403) + expect(user.reload).not_to be_suspended + expect(another_admin.reload).not_to be_suspended + end + end + context "when logged in as an admin" do before { sign_in(admin) } include_examples "suspension of active user possible" + include_examples "suspension of staff users" it "checks if user is suspended" do put "/admin/users/#{user.id}/suspend.json", @@ -505,6 +535,7 @@ RSpec.describe Admin::UsersController do before { sign_in(moderator) } include_examples "suspension of active user possible" + include_examples "suspension of staff users" end context "when logged in as a non-staff user" do @@ -1517,6 +1548,22 @@ RSpec.describe Admin::UsersController do expect(response.status).to eq(404) end + it "doesn't allow silencing another admin" do + put "/admin/users/#{another_admin.id}/silence.json" + expect(response.status).to eq(403) + expect(another_admin.reload).to_not be_silenced + end + + it "doesn't allow silencing another admin via other_user_ids" do + put "/admin/users/#{reg_user.id}/silence.json", + params: { + other_user_ids: [another_admin.id], + } + expect(response.status).to eq(403) + expect(another_admin.reload).to_not be_silenced + expect(reg_user.reload).to_not be_silenced + end + it "punishes the user for spamming" do put "/admin/users/#{reg_user.id}/silence.json" expect(response.status).to eq(200) @@ -1633,6 +1680,22 @@ RSpec.describe Admin::UsersController do expect(reg_user).to be_silenced expect(reg_user.silenced_record).to be_present end + + it "doesn't allow silencing another admin" do + put "/admin/users/#{another_admin.id}/silence.json" + expect(response.status).to eq(403) + expect(another_admin.reload).to_not be_silenced + end + + it "doesn't allow silencing another admin via other_user_ids" do + put "/admin/users/#{reg_user.id}/silence.json", + params: { + other_user_ids: [another_admin.id], + } + expect(response.status).to eq(403) + expect(another_admin.reload).to_not be_silenced + expect(reg_user.reload).to_not be_silenced + end end context "when logged in as a non-staff user" do diff --git a/spec/services/user_suspender_spec.rb b/spec/services/user_suspender_spec.rb new file mode 100644 index 00000000000..1ca55870215 --- /dev/null +++ b/spec/services/user_suspender_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +RSpec.describe UserSuspender do + fab!(:user) { Fabricate(:user, trust_level: 0) } + fab!(:post) { Fabricate(:post, user: user) } + fab!(:admin) + + describe "suspend" do + subject(:suspend_user) { suspender.suspend } + + let(:suspender) do + UserSuspender.new( + user, + suspended_till: 5.hours.from_now, + reason: "because", + by_user: admin, + post_id: post.id, + message: "you have been suspended", + ) + end + + it "suspends the user correctly" do + freeze_time + suspend_user + expect(user.reload).to be_suspended + expect(user.suspended_till).to be_within_one_second_of(5.hours.from_now) + expect(user.suspended_at).to be_within_one_second_of(Time.zone.now) + end + + it "creates a staff action log" do + expect do suspend_user end.to change { + UserHistory.where( + action: UserHistory.actions[:suspend_user], + acting_user_id: admin.id, + target_user_id: user.id, + ).count + }.from(0).to(1) + end + + it "logs the user out" do + messages = MessageBus.track_publish("/logout/#{user.id}") { suspend_user } + expect(messages.size).to eq(1) + expect(messages[0].user_ids).to eq([user.id]) + expect(messages[0].data).to eq(user.id) + end + + it "fires a user_suspended event" do + freeze_time + events = DiscourseEvent.track_events(:user_suspended) { suspend_user } + expect(events.size).to eq(1) + + params = events[0][:params].first + expect(params[:user].id).to eq(user.id) + expect(params[:reason]).to eq("because") + expect(params[:message]).to eq("you have been suspended") + expect(params[:suspended_till]).to be_within_one_second_of(5.hours.from_now) + expect(params[:suspended_at]).to eq(Time.zone.now) + end + + context "when a message is provided" do + it "enqueues a critical user email job" do + expect do suspend_user end.to change { Jobs::CriticalUserEmail.jobs.size }.from(0).to(1) + + job = Jobs::CriticalUserEmail.jobs.first + expect(job["args"].first["user_id"]).to eq(user.id) + expect(job["args"].first["user_history_id"]).to eq(suspender.user_history.id) + end + end + + context "when a message is not provided" do + let(:suspender) do + UserSuspender.new( + user, + suspended_till: 5.hours.from_now, + reason: "because", + by_user: admin, + post_id: post.id, + message: nil, + ) + end + + it "doesn't enqueue a critical user email job" do + expect do suspend_user end.not_to change { Jobs::CriticalUserEmail.jobs.size }.from(0) + end + end + end +end