FIX: prevents user to restore message deleted by staff

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

@@ -171,6 +171,7 @@ def self.publish_delete!(chat_channel, chat_message)
           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

@@ -49,8 +49,8 @@ def invalid_access(guardian:, message:, **)
       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 @@ def can_restore_chat?(message, chatable)
       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 @@
       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 @@
             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 @@
   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)
+
+        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(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 @@
         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 @@
         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 @@
             {
               "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 @@ def delete_message(message)
         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 @@ def does_not_exist?(**args)
         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 @@ def select(shift: false)
             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 @@ def component
           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