Proxy support for Discord#109
Conversation
Added changelog notes for other tweaks
|
Sorry, right now I am also out of ideas what the cause might be. Maybe @selfhoster1312 (who set up the CI and is generally more familiar with Github quirks than me) will have time to look at it after the holidays. |
|
The CI fails at the |
|
I ran, and... it produced unrelated changes that weren't supposed to change. Actually, the error is absolutely unobvious. Maybe inject an echo that tells an advice if such thing fails? VSCode did these changes too, but I reverted them since they are unrelated to the work that I done. |
|
And ye, now I see the thing is fixed now. Actually, the CI error should be obvious. Especially for people who aren't sure for this. |
|
From my side I see no issue in merging this if it is a purely optional feature. But I have not tested it and no easy way to do so right now. @selfhoster1312 any objections from the point of view of your http file handling changes? |
Yes, feature is fully optional, by default it's just unused, and everything works as usually, but when used, then Discord connects through HTTP proxy. |
Also! If you want to test, I can provide temporary login credentials at my own proxy server, but that's better if I send credentials via email. My email you can find just at any source file at one of my projects, for example https://github.com/Wohlstand/libADLMIDI. |
| - xmpp | ||
| - New and revised advanced authentication settings `UseDirectTLS`, `NoStartTls`, `NoPlain`, and `Mechanism` ([#77](https://github.com/matterbridge-org/matterbridge/pull/77)) | ||
| - discord | ||
| - Added a setting `AlwaysDownloadFiles` that allows to download and re-upload sent files from Discord CDN to other messengers or mediaproxy if configured |
There was a problem hiding this comment.
The debate about this setting is still ongoing here: #37 (comment)
There was a problem hiding this comment.
I already added that setting, but I didn't documented at changelog at all, and I done that just here.
| - matrix | ||
| - attachments received from matrix are working again, with authenticated media (MSC3916) implemented ([#61](https://github.com/matterbridge-org/matterbridge/pull/61)) | ||
| - image attachments are now send as images with more metadata ([#61](https://github.com/matterbridge-org/matterbridge/pull/61)) | ||
| - audio attachments are now sent as documents, not as a voice messages, otherwise they won't be delivered |
There was a problem hiding this comment.
Is this because it was forgotten in another PR? If so, can you link to the relevant PR here please?
There was a problem hiding this comment.
Is this because it was forgotten in another PR?
Absolutely!
If so, can you link to the relevant PR here please?
Sure...
| req, err := http.NewRequest("GET", fileurl, nil) //nolint:usestdlibvars | ||
|
|
||
| req.Header.Add("X-Auth-Token", token) | ||
| req.Header.Add("X-User-Id", userID) |
There was a problem hiding this comment.
You should probably not touch helper.go at all. We now have http helpers which respect the http_proxy global or bridge setting: https://github.com/matterbridge-org/matterbridge/blob/master/docs/development/protocol.md#handling-http-requests
There was a problem hiding this comment.
I touched it because I wanted to implement the file handling that will use the custom proxy, I extended API, because it was totally missing here.
There was a problem hiding this comment.
The new helpers were added in #59 specifically because you needed them here, and because they're useful in other bridges too. No need to add custom logic to discord bridge, except to make sure the discord API client itself uses the proxy of course :)
| return | ||
| } | ||
| data, err = helper.DownloadFile(url) | ||
| data, err = helper.DownloadFileProxy(url, b.httpProxy) |
There was a problem hiding this comment.
I'm guessing this should be b.AddAttachmentFromURL(&rmsg, attach.Filename, attach.Filename, caption , url). If there's an attachment ID somewhere, you can use it instead of the second filename argument.
Also, this method replaces HandleDownloadSize, DownloadFileProxy, and HandleDownloadData making the whole code simpler.
| } | ||
| b.Log.Info("Connection succeeded") | ||
|
|
||
| if b.httpProxy != "" { |
There was a problem hiding this comment.
This should be b.GetString("http_proxy") so it's the same across all bridges.
There was a problem hiding this comment.
When it became such? When I developed the thing initially, it was like I done already.
There was a problem hiding this comment.
Just a note: THIS and other changes I done a while ago before the matterbridge got been forked, and then I contributing my stuff, so, I already learned some changes and tweaked.
| b.nickMemberMap = make(map[string]*discordgo.Member) | ||
| b.channelInfoMap = make(map[string]*config.ChannelInfo) | ||
|
|
||
| b.httpProxy = b.GetString("HttpProxy") |
There was a problem hiding this comment.
It's not really useful to store this here. You can just remove it.
There was a problem hiding this comment.
I stored it here for convenience to track all bridge-specific settings at the same place, as it was done before me when I started to tweak this. And that's guarantee that it will be always valid config field requested, and such dupes would lead human factor errors like typo in the field name, and result - it works at one place but fails at another. With a variable it's impossible, compilation will fail.
|
|
||
| if b.httpProxy != "" { | ||
| b.Log.Info("Using HTTP proxy to connect Discord") | ||
| proxyURL, _ := url.Parse(b.httpProxy) |
There was a problem hiding this comment.
It's ok to discard the error here because in bridge.go:NewHttpClient, failing to parse http_proxy produces an error, which is considered fatal in gateway.go:AddBridge. You should still add a comment saying if the URL was invalid, AddBridge would panic and you would never reach this point.
There was a problem hiding this comment.
A question: DID that unrelated http_proxy implemented just recently? Actually, why not to just take my thing and refactor it together with everything so the full integrity is guaranteed? I feeling just tired of this bueroracy led by unexpected changes are doing just now, and as result, my not-yet-taken-changes yesterday was actual, and now they are so ancient as the dinosaur skeleton. (Just my expressing, sorry for that).
There was a problem hiding this comment.
No worry i understand your confusion. It is indeed very recent. The discussion started on your previous PR #48 but since you closed it you probably didn't read everything. I added the new helpers to help you and other bridge maintainers. If the new helpers are not helping, feel free to provide feedback so we can improve them.
| AutoWebhooks=true | ||
| ``` | ||
|
|
||
| ## HttpProxy |
There was a problem hiding this comment.
This should be http_proxy
There was a problem hiding this comment.
I made this field following format that was already defined by other fields here.
|
That looks good, thanks. Just a few changes here and there (especially using the new HTTP helpers) and we'll be good to go! 👍 |
These new HTTP helpers are just unexpected for me, especially because I am tired of recent CI fighting. I would take a while before I will apply all the new thingies without breaking of everything. |
If you feel like this is too much for you just say so. I'm OK to take the time to edit your PR or open a new one with the changes. |
Ye, you can try just remake my code if you can, I am fine, just it's need to make sure the final thing will just work. For my side that proxy is needed, otherwise Discord just doesn't work at all. |
|
So apparently i can't push directly to your branch, even if you have clicked "allow edits from maintainers". Github is really stupid. Maybe that's because i'm admin of the matterbridge-org org, and not of the matterbridge repo directly? Maybe you explicitly need to allow me to push to your fork, but i can't find documentation about this behavior. Anyway i'll just open a new PR, sorry. |
|
I'll just grant access to my copy of repo. |
That will be appreciated for next time :-) For now i already opened #113 so we can head over there for review. Thanks for your patience! |
Recreation of #48, CI still fails, and tells nothing why it fails. All the stuff refered related to downloads.