FIX: In chat messages, filter uploads by UserUpload, not by Upload.user (#34596)

When creating or updating a chat message, uploads attached to the
message are filtered so as to only keep uploads created by the message
creator using `Upload.user`. This field, however, only points to the
_original_ user that created the upload, but since uploads are
de-duplicated, other users might have also uploaded the file. This PR
fixes this by looking at the `UserUpload`s instead (as suggested by
@SamSaffron).

Reported here: https://meta.discourse.org/t/chat-upload-bug/379253

plugins/chat/app/services/chat/create_message.rb

@@ -151,7 +151,10 @@ module Chat
 
     def fetch_uploads(params:, guardian:)
       return [] if !SiteSetting.chat_allow_uploads
-      guardian.user.uploads.where(id: params.upload_ids)
+      Upload
+        .where(id: params.upload_ids)
+        .joins(:user_uploads)
+        .where(user_uploads: { user: guardian.user })
     end
 
     def instantiate_message(channel:, guardian:, params:, uploads:, thread:, reply:, options:)

plugins/chat/app/services/chat/update_message.rb

@@ -90,7 +90,10 @@ module Chat
 
     def fetch_uploads(params:, guardian:)
       return if !SiteSetting.chat_allow_uploads
-      guardian.user.uploads.where(id: params.upload_ids)
+      Upload
+        .where(id: params.upload_ids)
+        .joins(:user_uploads)
+        .where(user_uploads: { user: guardian.user })
     end
 
     def can_modify_channel_message(guardian:, message:)

plugins/chat/spec/services/chat/create_message_spec.rb

@@ -47,15 +47,16 @@ RSpec.describe Chat::CreateMessage do
     let(:content) { "A new message @#{other_user.username_lower}" }
     let(:context_topic_id) { nil }
     let(:context_post_ids) { nil }
+    let(:upload_ids) { [upload.id] }
     let(:blocks) { nil }
     let(:params) do
       {
         chat_channel_id: channel.id,
         message: content,
-        upload_ids: [upload.id],
-        context_topic_id: context_topic_id,
-        context_post_ids: context_post_ids,
-        blocks: blocks,
+        upload_ids:,
+        context_topic_id:,
+        context_post_ids:,
+        blocks:,
       }
     end
     let(:options) { { enforce_membership: false, force_thread: false } }
@@ -514,6 +515,18 @@ RSpec.describe Chat::CreateMessage do
                       result
                     end
 
+                    context "when upload was created by another user" do
+                      fab!(:another_upload) do
+                        Fabricate(:upload, user: other_user, uploaders: [user])
+                      end
+
+                      let(:upload_ids) { [upload.id, another_upload.id] }
+
+                      it "attaches the upload created by the other user" do
+                        expect(message.uploads).to contain_exactly(upload, another_upload)
+                      end
+                    end
+
                     context "when client_created_at is provided" do
                       let(:client_timestamp) { 30.seconds.ago }
 

plugins/chat/spec/services/chat/update_message_spec.rb

@@ -628,6 +628,7 @@ RSpec.describe Chat::UpdateMessage do
     describe "uploads" do
       fab!(:upload1) { Fabricate(:upload, user: user1) }
       fab!(:upload2) { Fabricate(:upload, user: user1) }
+      fab!(:upload3) { Fabricate(:upload, user: user3, uploaders: [user1]) }
 
       it "does nothing if the passed in upload_ids match the existing upload_ids" do
         chat_message =
@@ -790,6 +791,20 @@ RSpec.describe Chat::UpdateMessage do
         )
         expect(chat_message.reload.message).to eq(new_message)
       end
+
+      it "adds upload even if created by another user" do
+        chat_message = create_chat_message(user1, "something", public_chat_channel)
+        expect {
+          described_class.call(
+            guardian: guardian,
+            params: {
+              message_id: chat_message.id,
+              message: "I guess this is different",
+              upload_ids: [upload3.id],
+            },
+          )
+        }.to change { UploadReference.where(target: chat_message).count }.by(1)
+      end
     end
 
     context "when the message is in a thread" do