From c66743ee3d3e76e946292f9bc514cf2e5afc9af2 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Tue, 8 Nov 2022 09:04:18 +1000
Subject: [PATCH] FIX: Make ChatMessageUpdater check editing access for
 guardian (#18902)

Follow up to 766bcbc6840c9d665055441bcd77616b3a96e10e

This fixes a gaffe from that commit where I passed in the
guardian to ChatMessageUpdater but then forgot to remove
the old way of setting the guardian and user instance variables
from the chat_message that was passed in.

Also, it moves the ensure_can_edit_message! check from the
controller into ChatMessageUpdater so all the access
checks are in the same place.
---
 .../chat/app/controllers/chat_controller.rb    |  1 -
 plugins/chat/lib/chat_message_updater.rb       |  7 +------
 .../components/chat_message_updater_spec.rb    | 18 ++++++++++++++----
 .../chat/spec/requests/chat_controller_spec.rb |  6 +++---
 4 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/plugins/chat/app/controllers/chat_controller.rb b/plugins/chat/app/controllers/chat_controller.rb
index f84ce9b2739..e272188e720 100644
--- a/plugins/chat/app/controllers/chat_controller.rb
+++ b/plugins/chat/app/controllers/chat_controller.rb
@@ -144,7 +144,6 @@ class Chat::ChatController < Chat::ChatBaseController
   end
 
   def edit_message
-    guardian.ensure_can_edit_chat!(@message)
     chat_message_updater =
       Chat::ChatMessageUpdater.update(
         guardian: guardian,
diff --git a/plugins/chat/lib/chat_message_updater.rb b/plugins/chat/lib/chat_message_updater.rb
index 2ae108a22e7..e72bdb3d938 100644
--- a/plugins/chat/lib/chat_message_updater.rb
+++ b/plugins/chat/lib/chat_message_updater.rb
@@ -15,8 +15,6 @@ class Chat::ChatMessageUpdater
     @chat_message = chat_message
     @old_message_content = chat_message.message
     @chat_channel = @chat_message.chat_channel
-    @user = @chat_message.user
-    @guardian = Guardian.new(@user)
     @new_content = new_content
     @upload_ids = upload_ids
     @error = nil
@@ -25,6 +23,7 @@ class Chat::ChatMessageUpdater
   def update
     begin
       validate_channel_status!
+      @guardian.ensure_can_edit_chat!(@chat_message)
       @chat_message.message = @new_content
       @chat_message.last_editor_id = @user.id
       upload_info = get_upload_info
@@ -48,10 +47,6 @@ class Chat::ChatMessageUpdater
 
   private
 
-  # TODO (martin) Since we have guardian here now we should move
-  # guardian.ensure_can_edit_chat!(@message) from the controller into
-  # this class.
-
   def validate_channel_status!
     return if @guardian.can_modify_channel_message?(@chat_channel)
     raise StandardError.new(
diff --git a/plugins/chat/spec/components/chat_message_updater_spec.rb b/plugins/chat/spec/components/chat_message_updater_spec.rb
index 1b6c9989658..d31e4bae8e1 100644
--- a/plugins/chat/spec/components/chat_message_updater_spec.rb
+++ b/plugins/chat/spec/components/chat_message_updater_spec.rb
@@ -31,10 +31,7 @@ describe Chat::ChatMessageUpdater do
     end
     Group.refresh_automatic_groups!
     @direct_message_channel =
-      Chat::DirectMessageChannelCreator.create!(
-        acting_user: user1,
-        target_users: [user1, user2],
-      )
+      Chat::DirectMessageChannelCreator.create!(acting_user: user1, target_users: [user1, user2])
   end
 
   def create_chat_message(user, message, channel, upload_ids: nil)
@@ -71,6 +68,19 @@ describe Chat::ChatMessageUpdater do
     expect(chat_message.reload.message).to eq(og_message)
   end
 
+  it "errors if a user other than the message user is trying to edit the message" do
+    og_message = "This won't be changed!"
+    chat_message = create_chat_message(user1, og_message, public_chat_channel)
+    new_message = "2 short"
+    updater = Chat::ChatMessageUpdater.update(
+      guardian: Guardian.new(Fabricate(:user)),
+      chat_message: chat_message,
+      new_content: new_message,
+    )
+    expect(updater.failed?).to eq(true)
+    expect(updater.error).to match(Discourse::InvalidAccess)
+  end
+
   it "it updates a messages content" do
     chat_message = create_chat_message(user1, "This will be changed", public_chat_channel)
     new_message = "Change to this!"
diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb
index 12f582cfd56..78dc47157f8 100644
--- a/plugins/chat/spec/requests/chat_controller_spec.rb
+++ b/plugins/chat/spec/requests/chat_controller_spec.rb
@@ -532,7 +532,7 @@ RSpec.describe Chat::ChatController do
 
       it "raises an invalid request" do
         put "/chat/#{chat_channel.id}/edit/#{chat_message.id}.json", params: { new_message: "Hi" }
-        expect(response.status).to eq(403)
+        expect(response.status).to eq(422)
       end
     end
 
@@ -540,7 +540,7 @@ RSpec.describe Chat::ChatController do
       sign_in(Fabricate(:user))
 
       put "/chat/#{chat_channel.id}/edit/#{chat_message.id}.json", params: { new_message: "edit!" }
-      expect(response.status).to eq(403)
+      expect(response.status).to eq(422)
     end
 
     it "errors when staff tries to edit another user's message" do
@@ -551,7 +551,7 @@ RSpec.describe Chat::ChatController do
           params: {
             new_message: new_message,
           }
-      expect(response.status).to eq(403)
+      expect(response.status).to eq(422)
     end
 
     it "allows a user to edit their own messages" do