FIX: prevents user to restore message deleted by staff (#22571)

It could only occur on message created by the user itself and deleted while the user was looking at the channel.

It more generally fix the trash service which was not correctly setting the author of the delete.

plugins/chat/app/services/chat/publisher.rb

@@ -174,6 +174,7 @@ module Chat
           type: "delete",
           deleted_id: chat_message.id,
           deleted_at: chat_message.deleted_at,
+          deleted_by_id: chat_message.deleted_by_id,
           latest_not_deleted_message_id: latest_not_deleted_message_id,
         },
       )

plugins/chat/app/services/chat/trash_message.rb

@@ -50,8 +50,8 @@ module Chat
       guardian.can_delete_chat?(message, message.chat_channel.chatable)
     end
 
-    def trash_message(message:, **)
-      message.trash!
+    def trash_message(message:, guardian:, **)
+      message.trash!(guardian.user)
     end
 
     def destroy_notifications(message:, **)

plugins/chat/assets/javascripts/discourse/lib/chat-message-interactor.js

@@ -96,8 +96,10 @@ export default class ChatMessageInteractor {
 
   get canRestoreMessage() {
     return (
-      this.canDelete &&
       this.message?.deletedAt &&
+      (this.currentUser.staff ||
+        (this.message?.user?.id === this.currentUser.id &&
+          this.message?.deletedById === this.currentUser.id)) &&
       this.message.channel?.canModifyMessages?.(this.currentUser)
     );
   }
@@ -118,7 +120,7 @@ export default class ChatMessageInteractor {
 
   get canFlagMessage() {
     return (
-      this.currentUser?.id !== this.message?.user?.id &&
+      this.currentUser.id !== this.message?.user?.id &&
       this.message?.userFlagStatus === undefined &&
       this.message.channel?.canFlag &&
       !this.message?.chatWebhookEvent &&
@@ -128,7 +130,7 @@ export default class ChatMessageInteractor {
 
   get canRebakeMessage() {
     return (
-      this.currentUser?.staff &&
+      this.currentUser.staff &&
       this.message.channel?.canModifyMessages?.(this.currentUser)
     );
   }
@@ -142,7 +144,7 @@ export default class ChatMessageInteractor {
   }
 
   get canDelete() {
-    return this.currentUser?.id === this.message.user.id
+    return this.currentUser.id === this.message.user.id
       ? this.message.channel?.canDeleteSelf
       : this.message.channel?.canDeleteOthers;
   }

plugins/chat/assets/javascripts/discourse/models/chat-message.js

@@ -52,6 +52,7 @@ export default class ChatMessage {
   @tracked thread;
   @tracked manager;
   @tracked threadTitle;
+  @tracked deletedById;
 
   @tracked _deletedAt;
   @tracked _cooked;
@@ -69,6 +70,7 @@ export default class ChatMessage {
     this.hidden = args.hidden || false;
     this.chatWebhookEvent = args.chatWebhookEvent || args.chat_webhook_event;
     this.createdAt = args.createdAt || args.created_at;
+    this.deletedById = args.deletedById || args.deleted_by_id;
     this._deletedAt = args.deletedAt || args.deleted_at;
     this.expanded =
       this.hidden || this._deletedAt ? false : args.expanded || true;

plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js

@@ -177,6 +177,7 @@ export default class ChatPaneBaseSubscriptionsManager extends Service {
 
     if (this.currentUser.staff || this.currentUser.id === targetMsg.user.id) {
       targetMsg.deletedAt = data.deleted_at;
+      targetMsg.deletedById = data.deleted_by_id;
       targetMsg.expanded = false;
     } else {
       this.messagesManager.removeMessage(targetMsg);

plugins/chat/lib/chat/guardian_extensions.rb

@@ -172,9 +172,9 @@ module Chat
       if message.user_id == current_user.id
         case chatable
         when Category
-          return can_see_category?(chatable)
+          return message.deleted_by_id == current_user.id || can_see_category?(chatable)
         when Chat::DirectMessage
-          return true
+          return message.deleted_by_id == current_user.id || is_staff?
         end
       end
 

plugins/chat/spec/lib/chat/guardian_extensions_spec.rb

@@ -236,10 +236,16 @@ RSpec.describe Chat::GuardianExtensions do
       context "when chatable is a direct message" do
         fab!(:chatable) { Chat::DirectMessage.create! }
 
-        it "allows owner to restore" do
+        it "allows owner to restore when deleted by owner" do
+          message.trash!(guardian.user)
           expect(guardian.can_restore_chat?(message, chatable)).to eq(true)
         end
 
+        it "disallow owner to restore when deleted by staff" do
+          message.trash!(staff_guardian.user)
+          expect(guardian.can_restore_chat?(message, chatable)).to eq(false)
+        end
+
         it "allows staff to restore" do
           expect(staff_guardian.can_restore_chat?(message, chatable)).to eq(true)
         end
@@ -323,9 +329,15 @@ RSpec.describe Chat::GuardianExtensions do
             expect(staff_guardian.can_restore_chat?(message, chatable)).to eq(true)
           end
 
-          it "allows owner to restore" do
+          it "allows owner to restore when deleted by owner" do
+            message.trash!(guardian.user)
             expect(guardian.can_restore_chat?(message, chatable)).to eq(true)
           end
+
+          it "disallow owner to restore when deleted by staff" do
+            message.trash!(staff_guardian.user)
+            expect(guardian.can_restore_chat?(message, chatable)).to eq(false)
+          end
         end
       end
     end

plugins/chat/spec/requests/chat/api/messages_controller_spec.rb

@@ -110,91 +110,91 @@ RSpec.describe Chat::Api::ChannelMessagesController do
   describe "#restore" do
     RSpec.shared_examples "chat_message_restoration" do
       it "doesn't allow a user to restore another user's message" do
-        sign_in(other_user)
+        another_user = Fabricate(:user)
+        message = Fabricate(:chat_message, chat_channel: chat_channel, user: another_user)
+        message.trash!(another_user)
 
-        put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json"
+        sign_in(current_user)
+
+        put "/chat/api/channels/#{chat_channel.id}/messages/#{message.id}/restore.json"
         expect(response.status).to eq(403)
       end
 
       it "allows a user to restore their own messages" do
+        message = Fabricate(:chat_message, chat_channel: chat_channel, user: current_user)
+        message.trash!(current_user)
+
         sign_in(current_user)
 
-        put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json"
+        put "/chat/api/channels/#{chat_channel.id}/messages/#{message.id}/restore.json"
         expect(response.status).to eq(200)
-        expect(deleted_message.reload.deleted_at).to be_nil
+        expect(message.reload.deleted_at).to be_nil
       end
 
       it "allows admin to restore others' messages" do
+        message = Fabricate(:chat_message, chat_channel: chat_channel, user: current_user)
+        message.trash!(current_user)
+
         sign_in(admin)
 
-        put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json"
+        put "/chat/api/channels/#{chat_channel.id}/messages/#{message.id}/restore.json"
         expect(response.status).to eq(200)
-        expect(deleted_message.reload.deleted_at).to be_nil
+        expect(message.reload.deleted_at).to be_nil
       end
 
       it "does not allow message restore when channel is read_only" do
+        message = Fabricate(:chat_message, chat_channel: chat_channel, user: current_user)
+        message.trash!(current_user)
+
         sign_in(current_user)
 
         chat_channel.update!(status: :read_only)
 
-        put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json"
+        put "/chat/api/channels/#{chat_channel.id}/messages/#{message.id}/restore.json"
         expect(response.status).to eq(403)
-        expect(deleted_message.reload.deleted_at).not_to be_nil
+        expect(message.reload.deleted_at).not_to be_nil
 
         sign_in(admin)
-        put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json"
+        put "/chat/api/channels/#{chat_channel.id}/messages/#{message.id}/restore.json"
         expect(response.status).to eq(403)
       end
 
       it "only allows admin to restore when channel is closed" do
+        message = Fabricate(:chat_message, chat_channel: chat_channel, user: current_user)
+        message.trash!(current_user)
+
         sign_in(admin)
 
         chat_channel.update!(status: :read_only)
 
-        put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json"
+        put "/chat/api/channels/#{chat_channel.id}/messages/#{message.id}/restore.json"
         expect(response.status).to eq(403)
-        expect(deleted_message.reload.deleted_at).not_to be_nil
+        expect(message.reload.deleted_at).not_to be_nil
 
         chat_channel.update!(status: :closed)
-        put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json"
+        put "/chat/api/channels/#{chat_channel.id}/messages/#{message.id}/restore.json"
         expect(response.status).to eq(200)
-        expect(deleted_message.reload.deleted_at).to be_nil
+        expect(message.reload.deleted_at).to be_nil
       end
     end
 
     fab!(:admin) { Fabricate(:admin) }
-    fab!(:second_user) { Fabricate(:user) }
-
-    before do
-      message =
-        Chat::Message.create(
-          user: current_user,
-          message: "this is a message",
-          chat_channel: chat_channel,
-        )
-      message.trash!
-    end
-
-    let(:deleted_message) do
-      Chat::Message.unscoped.where(user: current_user, chat_channel: chat_channel).last
-    end
+    fab!(:another_user) { Fabricate(:user) }
 
     describe "for category" do
       fab!(:category) { Fabricate(:category) }
       fab!(:chat_channel) { Fabricate(:category_channel, chatable: category) }
 
-      it_behaves_like "chat_message_restoration" do
-        let(:other_user) { second_user }
-      end
+      it_behaves_like "chat_message_restoration"
     end
 
     describe "for dm channel" do
       fab!(:user_2) { Fabricate(:user) }
-      fab!(:chat_channel) { Fabricate(:direct_message_channel, users: [current_user, user_2]) }
-
-      it_behaves_like "chat_message_restoration" do
-        let(:other_user) { user_2 }
+      fab!(:chat_channel) do
+        Fabricate(:direct_message_channel, users: [current_user, another_user])
       end
+
+      it_behaves_like "chat_message_restoration"
     end
   end
 end

plugins/chat/spec/services/chat/publisher_spec.rb

@@ -15,6 +15,7 @@ describe Chat::Publisher do
         MessageBus.track_publish { described_class.publish_delete!(channel, message_2) }[0].data
 
       expect(data["deleted_at"]).to eq(message_2.deleted_at.iso8601(3))
+      expect(data["deleted_by_id"]).to eq(message_2.deleted_by_id)
       expect(data["deleted_id"]).to eq(message_2.id)
       expect(data["latest_not_deleted_message_id"]).to eq(message_1.id)
       expect(data["type"]).to eq("delete")

plugins/chat/spec/services/chat/trash_message_spec.rb

@@ -41,6 +41,10 @@ RSpec.describe Chat::TrashMessage do
         it "trashes the message" do
           result
           expect(Chat::Message.find_by(id: message.id)).to be_nil
+
+          deleted_message = Chat::Message.unscoped.find_by(id: message.id)
+          expect(deleted_message.deleted_by_id).to eq(current_user.id)
+          expect(deleted_message.deleted_at).to be_within(1.minute).of(Time.zone.now)
         end
 
         it "destroys notifications for mentions" do
@@ -65,6 +69,7 @@ RSpec.describe Chat::TrashMessage do
             {
               "type" => "delete",
               "deleted_id" => message.id,
+              "deleted_by_id" => current_user.id,
               "deleted_at" => message.reload.deleted_at.iso8601(3),
               "latest_not_deleted_message_id" => nil,
             },

plugins/chat/spec/system/page_objects/chat/chat_channel.rb

@@ -120,6 +120,12 @@ module PageObjects
         find("[data-value='delete']").click
       end
 
+      def restore_message(message)
+        hover_message(message)
+        click_more_button
+        find("[data-value='restore']").click
+      end
+
       def open_edit_message(message)
         hover_message(message)
         click_more_button

plugins/chat/spec/system/page_objects/chat/components/message.rb

@@ -18,7 +18,16 @@ module PageObjects
         end
 
         def hover
-          message_by_id(message.id).hover
+          component.hover
+        end
+
+        def open_more_menu
+          hover
+          click_more_button
+        end
+
+        def expand
+          component.find(".chat-message-expand").click
         end
 
         def select(shift: false)
@@ -37,7 +46,7 @@ module PageObjects
             component.click(delay: 0.6)
             page.find(".chat-message-actions [data-id=\"select\"]").click
           else
-            component.hover
+            hover
             click_more_button
             page.find("[data-value='select']").click
           end

plugins/chat/spec/system/page_objects/chat/components/messages.rb

@@ -16,6 +16,22 @@ module PageObjects
           page.find(context)
         end
 
+        def has_action?(action, **args)
+          message = find(args)
+          message.open_more_menu
+          page.has_css?("[data-value='#{action}']")
+        end
+
+        def has_no_action?(action, **args)
+          message = find(args)
+          message.open_more_menu
+          page.has_no_css?("[data-value='#{action}']")
+        end
+
+        def expand(**args)
+          find(args).expand
+        end
+
         def select(args)
           find(args).select
         end

plugins/chat/spec/system/restore_message_spec.rb

@@ -0,0 +1,76 @@
+# frozen_string_literal: true
+
+RSpec.describe "Restore message", type: :system do
+  fab!(:admin_user) { Fabricate(:admin) }
+  fab!(:regular_user) { Fabricate(:user) }
+  fab!(:another_user) { Fabricate(:user) }
+  fab!(:channel_1) { Fabricate(:category_channel) }
+
+  let(:chat_page) { PageObjects::Pages::Chat.new }
+  let(:chat_channel_page) { PageObjects::Pages::ChatChannel.new }
+
+  before do
+    chat_system_bootstrap
+    channel_1.add(admin_user)
+    channel_1.add(regular_user)
+    channel_1.add(another_user)
+  end
+
+  context "when user deletes its own message" do
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: regular_user) }
+
+    it "can be restored by the owner" do
+      sign_in(regular_user)
+      chat_page.visit_channel(channel_1)
+
+      chat_channel_page.delete_message(message_1)
+
+      expect(chat_channel_page.messages).to have_deleted_message(message_1, count: 1)
+      expect(chat_channel_page.messages).to have_action("restore", id: message_1.id)
+    end
+
+    it "can't be restored by another user" do
+      using_session(:another_user) do
+        sign_in(another_user)
+        chat_page.visit_channel(channel_1)
+      end
+
+      using_session(:regular_user) do |session|
+        sign_in(regular_user)
+        chat_page.visit_channel(channel_1)
+        chat_channel_page.delete_message(message_1)
+        session.quit
+      end
+
+      using_session(:another_user) do |session|
+        expect(chat_channel_page.messages).to have_no_message(id: message_1.id)
+        session.quit
+      end
+    end
+  end
+
+  context "when staff deletes user message" do
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: regular_user) }
+
+    it "can't be restored by owner" do
+      using_session(:regular_user) do
+        sign_in(regular_user)
+        chat_page.visit_channel(channel_1)
+      end
+
+      using_session(:admin_user) do |session|
+        sign_in(admin_user)
+        chat_page.visit_channel(channel_1)
+        chat_channel_page.delete_message(message_1)
+        session.quit
+      end
+
+      using_session(:regular_user) do |session|
+        expect(chat_channel_page.messages).to have_deleted_message(message_1, count: 1)
+        chat_channel_page.messages.expand(id: message_1.id)
+        expect(chat_channel_page.messages).to have_no_action("restore", id: message_1.id)
+        session.quit
+      end
+    end
+  end
+end