SECURITY: Don't allow suspending staff users via other_user_ids param

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(

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

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

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