خطأ في تحميل الدردشة

خلل
,
1

Hi, I want to report a problem I found when uploading files or images in chat DMs.

  • When User A uploads File A in a post or chat, it works fine and the file is registered correctly.

  • But when User B downloads File A and then tries to upload it again in another chat:

    • If User B sends the file without text, they get an error:

      Message is too short, must have a minimum of 1 character.

    • If User B sends the file with text, only the text is sent — the attachment is missing.

However, if User A re-uploads the same file again, it works fine and the file is included in the message.

My question:
Is this the intended behavior, where only the original uploader can re-use an uploaded file? Or should other users also be able to upload and send the same file in chat?

image
image1096×578 35.9 KB

إعجاب واحد (1)
3

Yeah this sounds like a bit of a glitch, someone will have a look over the next few weeks, happy to put a PR welcome on this in the interim.

إعجاب واحد (1)
5

It took me a while to dig through this, but I think I find the cause. I think it has to do with this code:

def fetch_uploads(params:, guardian:)
  return [] if !SiteSetting.chat_allow_uploads
  guardian.user.uploads.where(id: params.upload_ids) # Specifically, here
end

When a new chat message is created, this is called to fetch the uploads to attach to the message. Possibly to ensure ownership, the method goes through the Guardian’s user to fetch the uploads so as to only allow uploads belonging to that user.

The problem is that uploads are deduplicated, as seen here:

# do we already have that upload?
@upload = Upload.find_by(sha1: sha1)

# ...

# return the previous upload if any
if @upload
  add_metadata!
  UserUpload.find_or_create_by!(user_id: user_id, upload_id: @upload.id) if user_id
  return @upload
end

I think that a possible fix would be to go through the UserUploads instead of just considering the uploads belonging to the user. UserUpload links uploads to multiple users, which seems to be what we need. I’m not yet 100% sure how to do it properly; it’s getting late, so I’ll tuck in, but if no one else fixes it, I’ll try to come back later and work on a PR. :slightly_smiling_face:

3 إعجابات
6

This is a fantastic catch and spot on. :hugs:

Uploads have a user_id, but that is just the original user that created the upload.

Converting that code to:

Upload.where(id: params.upload_ids).joins(:user_uploads).where(user_uploads: { user_id: guardian.user.id })

will do the trick

Can you send a PR through!

7

Thanks, I wasn’t sure how to best build that query (I’m fairly new here :slightly_smiling_face: ). I’ll work on a PR.

إعجابَين (2)
8

Ok, I finally finished the PR. Sorry for the delay, life issues intruded.

FIX: in chat messages, filter uploads by `UserUpload`, not by `Upload.user` by clechasseur · Pull Request #34596 · discourse/discourse

9

I see that some of the tests have failed, but they’re not tests I’ve touched. I’ve been having issues running the tests in these files locally as well - I got the same duplicate key errors I see here… I’m not sure if it’s me that broke something or if the tests are sometimes flaky…