Handle XMPP uploads#23
Conversation
|
So right now the status is:
I need to implement OOB file upload when sending the attachments. Here's the status of that:
Currently this depends on unreleased go-xmpp because of missing fields on the |
2075019 to
8a266da
Compare
|
Not sure why Thank you CI gods <3 |
|
Like discussed in the XMPP chat, it seems a bit odd to use the xmpp http_upload service for something the built in media-proxy was designed for, but on the other hand having the bot use the existing xmpp server features isn't bad per se and does simplify the setup significantly in some cases. For example some people run Matterbridge from their home networks with no option to publicly expose a http endpoint via the media-proxy. |
So if the xmpp bridge is about to send a file with an attachment, if that file already has a URL, it will not reupload it. However, on the receiving side i am not transmitting the URL. Maybe i should? There's a tradeoff involved:
I'm not sure what's the best approach here. Does anybody have a strong opinion? Otherwise, maybe this should just be a setting so that the bridge operator can choose for themselves. Also noting that attachment handling is a very undocumented part of the codebase. I've added a few comments here and there in this PR, but i think this deserves a deeper conversation in the future so we don't ask those unfortunate questions on a bridge-by-bridge basis. |
|
A setting is good, but it should probably default to what requires the least additional setup, i.e. using the external http_upload in this case. |
Sorry if i wasn't clear. I meant when the xmpp bridge receives a remote HTTP attachment, it reuploads it on the bridge's file upload server. I was proposing a setting to disable that, which has nothing to do with the mediaserver. Or i could just disable the reupload entirely. Opinions? The mediaserver integration is also a good question but i think it's a more general question of architecture with all the different bridges. I'll open an issue about this but i'm proposing we don't really answer it in this PR because it requires more overall thought. |
|
Ah ok. Yes that seems like a good privacy feature and is probably also useful to avoid some automated checks against hotloading images from the Discord cdn for example. |
92b241d to
e4303a4
Compare
e4303a4 to
f69f19e
Compare
|
Pushed a new commit using the new |
f69f19e to
55b475f
Compare
|
Quick question: did you the disguesting the fact the URL is owned by XMPP's HTTP file server or it's just a random URL shared from nowhere? |
55b475f to
85cb317
Compare
Not sure what you mean sorry :D :D If the question is about attachments from other bridges:
If the question is about attachments coming from the XMPP channel:
EDIT: no actually files are uploaded to the bot's HTTP upload server, not the chatroom's. |
85cb317 to
dc2c653
Compare
cd9877f to
c178663
Compare
|
Ready for testing/review @poVoq You can probably test the native file upload by using Discord's Looks good to me. There's still plenty of corner cases and profound questions about attachments and interop with other networks but i think it's deeper than XMPP integration and we should merge this then think about the bigger picture. |
|
Am I missing some newly necessary config option or so? Because right now if I try the version of this PR it just fails entirely to send files to XMPP. Files uploaded on Matrix only show the filename and an empty message on XMPP, and files uploaded on Discord Nothing visible in the regular logs about this. It should probably show a warning or so when an upload fails. (The version of this PR you shared a few weeks ago was working fine with this exact setup). Also: please add a changelog entry and at least a note in the xmpp protocol documentation that a http_upload component for the bot account is required now. |
Very good point!!!!
Yeah discord attachments are broken. I don't think it's the fault of this PR. See #114 for discord attachment improvements (work in progress).
That's not expected. I may have broken something by accident. Can you share debug logs maybe? I tested again xmpp<->xmpp is working fine on this branch, so except if it's a problem with captions (wihch we don't really support in XMPP), it might indicate a bug on the other bridge instead. A quick read at how matrix handles upload indicates:
|
c178663 to
f70727f
Compare
f70727f to
4be0fac
Compare
|
Rebased for testing. This will be the final round of review. Concerns about blocking uploads/downloads and message ID tracking will be addressed further, as deep discussions have taken place about these topics and work is under way. |
|
Sending image files from Discord or Matrix still result in an empty message on the XMPP side. |
|
Further testing: With the media-server turned off it is a bit better. There is still the empty message, but images shared from Matrix get uploaded to the XMPP http_upload correctly and shared as another message. However for images shared from Discord the situation is unchanged even with the media-server off. The debug logs are hard to parse, but there is no error or warning as far as I can see. |
|
So there seem to be two separate issues the debug message for the failing Discord image share is this:
Which is odd because it just repeats the Discord CDN link, while the working message for the Matrix image share is:
Which uses the http_upload service as expected (as long as the media-server is off). Edit: ok the lack of an Discord URL link, XMPP side seems to be unrelated to this PR. |
|
So trying to reproduce the missing file uploads towards XMPP. Looks like it only happens on @poVoq 's server so far. I could not reproduce with two different prosody instance, nor with another ejabberd instance. We have ruled out:
|
|
The problem was that my server had So it looks like this PR is finally good to merge. |
|
Looks like the culprit was rate-limiting configured on @poVoq's server. No problem on this PR, everything is good!!!!!! <3 |
Current status:
Learned along the way:
RemoteNickFormatin config, you're gonna have a bad timexmpp.Client.SendOOBactually ignores anyTextbody set, which is bad for OOB file uploads where the body must match the OOB urlSome gotchas:
RemoteNickFormatstyleuploading HTTP files happens in the background so we don't block the task, but downloading files is still done in the foreground, which means the client could hangfixed