Add support for socket mode Events API for Slack bridge#149
Conversation
|
Thanks, that looks good at first look. I dont use slack so i can't test, we'll see what others have to say. If you can address the linter concerns i think we can merge this as a BETA (like you already say in the warning log) since it looks like new users have no alternative ways to connect to Slack. Other opinions welcome. |
asdfzdfj
force-pushed
the
exp/slack-events-socket
branch
2 times, most recently
from
5f226c7 to
58ee80c
Compare
There was a problem hiding this comment.
Been running this code (or at least a version from before it was rebased) and it works for me using the new bot token setup (which also requires an app token as documented). I'm not even using the full list of scopes in the docs. Haven't reviewed the code quality in much detail as im not familiar with this codebase but it seems that was already reviewed.
My main struggle was specifying the channel ID in the right format/with the ID: prefix. The rest of it was reading the docs and comments from OP basically
|
I do have a legacy classic slack app i could maybe test with if thats what you mean by the old RTM API. If so, should I expect this to be a drop-in replacement of running the unchanged config with this new version? or would i need to set up the additional socket mode token too? |
I'd like to check that the existing slack bridge setup w/ classic apps still works as expected just like before, without any changes in matterbridge config or slack app settings (it would still use RTM under the hood but it shouldn't break) mainly because I have edited some event handlers such that it can be shared between both RTM and Events API, and I'm not sure if I remapped the incoming event data to feed into the common handlers properly, |
|
looks like bridging images from slack to other platforms (new bot token method) isnt working (could be my fault with weird permissions things, slacks portal seems to get glitchy when you add this many token scopes): |
| b.cache.Add(cfileDownloadChannel+ev.Files[i].ID, ev.Channel) | ||
| if err := b.handleDownloadFile(rmsg, &ev.Files[i], false); err != nil { | ||
| b.cache.Add(cfileDownloadChannel+msg.Files[i].ID, msg.Channel) | ||
| if err := b.handleDownloadFile(rmsg, &msg.Files[i], false); err != nil { |
There was a problem hiding this comment.
It looks like for some reason slack.File.URLPrivateDownload == "", at least according to this report: #145 (comment)
EDIT: that report was reposted in this PR too, it's the same error reported by @MoralCode
There was a problem hiding this comment.
if we know what permission scope governs this i can tell you if its probbaly a self-inflicted permissions issue or not
There was a problem hiding this comment.
the related scope should be around files:{read,write} and remote_files:{read,share,write}
the minimum required scope seems to be files:read going by this doc but you should add the other file scopes anyway so the bridge can upload files from other places into slack
also I have no idea why that field would be empty, that's a first (hopefully it's just the missing scope)
There was a problem hiding this comment.
@MoralCode was this issue fixed for you somehow?
| @@ -666,6 +665,11 @@ PreserveThreading=false | |||
| #REQUIRED (when not using webhooks) | |||
There was a problem hiding this comment.
Is it still required with the new AppToken? If not we should indicate that it's been deprecated, linking to the FAQ warning about classic slack apps.
There was a problem hiding this comment.
It's still required, you need *both* bot token (this) and the new app token (below) in order to operate the bridge with socket mode events api
you need app token to open socket mode events api connection, and bot token to tell slack to do things using web api
There was a problem hiding this comment.
Hmm does that mean, hypothetically if i used Discord, i need to follow two different token creation steps? If so i think we can make that a lot clearer. But that's not gonna block this PR which we need yesterday already ^^
There was a problem hiding this comment.
I'm not sure I understand what you mean but if someone were to setup a new slack bridge bridging to literally anywhere else today they would need to create a slack app. generate 2 types of token and put them in matterbridge config in order to get a working slack bridge, but if they already got a working matterbridge config using bot token and classic apps then it should work just as before (i.e. they would only have a single bot token belonging to a classic slack apps configured)
discord bridge config should only concerns discord's side and not slack's
There was a problem hiding this comment.
Sorry i meant "slack" not "discord". If i understand your docs correctly, that token is called Bot User OAuth Token in the slack UI. Maybe this could be explained in the comment above Token, something like:
# `Token` is required both for new socket mode (recommended in 2026) and classical app setups.
# See the matterbridge slack docs for how to obtain this token:
# https://github.com/asdfzdfj/matterbridge/blob/master/docs/protocols/slack/account.md
#REQUIRED (when not using webhooks) <--- is that still a thing?
|
I deployed my (maybe somewhat old) build of this branch with one of my classic slack apps and at least message bridging seems to work |
thanks, that's good to hear |
asdfzdfj
force-pushed
the
exp/slack-events-socket
branch
from
772e9a7 to
f65b024
Compare
|
From your branch's docs: 
We should probably remove any mention of "legacy" since it's entirely broken now if i understand correctly? 
If i understand the classic token you still have to set is given in the steps before that. We should just remove any text below, for posterity we have git 😄. I think this page should not have subsections, for clarity it should only have the bot setup instructions (with the deprecation warning for other methods at the top), and the "issues" section should be moved to the FAQ, if it is at all still relevant after updating both code and docs. If editing the docs is too much burden for you, i'd be happy to deal with that after merging : i don't consider that a blocker. Also, you could add yourself as a slack maintainer in |
i think those docs were there from the main branch. If it were me, id want to also add something under the "bot token setup" column to say that token prefix is The note about legacy configs could link to the git tree at the last commit to main before the docs got changed, so we could say
|
asdfzdfj
force-pushed
the
exp/slack-events-socket
branch
2 times, most recently
from
2399b5e to
38287a1
Compare
|
ok so I've updated the docs a bit from the suggestions, in particular:
if thing seems ok then I'll start combine those commits down to just a few patches to tidy things up a bit |
|
That looks great, thanks! I would still remove the outdated setup information in
Great plan, good luck with that! |
asdfzdfj
force-pushed
the
exp/slack-events-socket
branch
2 times, most recently
from
e075acf to
90b6953
Compare
|
just a heads up but i have squashed those commits down, and also remove the mention of old account setup methods as requested, the top important note and the new method is all that's left side note: consider enabling PR squash merge for the project? idk where it is the toggle in the setting but it should be there somewhere |
|
Thanks. So from your side you consider it good to merge now, or is it still WIP? |
yes it should be good now |
asdfzdfj
changed the title
|
Thank you for keeping matterbridge alive @asdfzdfj ! 👏 |
|
Ok, so I think if you can still add a changelog entry then this should be good to merge. Of the current core maintainers no one seems to be using Slack, so we will just have to see how well it works when others try it. |
| // all messages will be logged with level debug | ||
| // | ||
| // CAVEAT: due to logrus' limitation, the logged source location is inaccurate | ||
| // and it's impossible to inject custom caller location information for logging |
There was a problem hiding this comment.
I'm not 100% sure what you mean. Is all location information reported as smlog.Output?
Could we maybe do something like SetOutput(b.Log.WriterLevel(Level.DebugLevel))?
There was a problem hiding this comment.
I'm currently away from the dev/test machine so I couldn't give good examples but basically logrus can determine where the code calls the logging function and output the source location to log lines e.g. [handleSlackClientEAPI:bridge/slack/handlers.go:58] in:
[0000] WARN slack: [handleSlackClientEAPI:bridge/slack/handlers.go:58] Socket mode Events API is currently WORK IN PROGRESS
the problem here is that when this smlog.Output() is being called in the target code, the log location shown in the log would pointed to where the l.Debug() is, rather than where the outside code has called us, which is most likely what we'd like to know,
and it looked like logrus didn't provide a good way to allow caller to supply their own source location for logging (see log/slog docs on wrapping output and its wrapping example, what this lacked here is the ability to do the equivalent wrapping for logrus)
though in practice, the only place this smlog being used is in socketmode client in the bridge, most of its output message is for diagnostic purposes, and I have it logged at debug level so it shouldn't be too big of a problem for usual bridge operations
I have no idea what your suggestions would do but I doubt that will help with the above
| If you get the message: | ||
|
|
||
| ``` | ||
| ERROR slack: Connection failed "not_allowed_token_type" &errors.errorString{s:"not_allowed_token_type"} |
There was a problem hiding this comment.
Is this still relevant? The issues are pretty old by now. And it looks like the recommendation was update to event API?
There was a problem hiding this comment.
I only lifted this faq point out of the account doc and put them here, I kinda don't have enough context or info to update the answers properly...
though it seems like this could still happen when trying to start the bridge using bot token from a modern slack app but then it tries to connect using RTM for whatever reason (now most likely because they didn't configure app token, or that the the bot token once belonged to a classic slack app but the app behind it got upgraded to modern app somehow), like what happened in #145 (comment)
|
All looks good. Just minor questions before merging. Would you like to be added as maintainer in Thank you so much for working hard on this 👍 |
adds support for using socket mode Events API to receive messages for bridging instead of legacy RTM system, however this will require an additional App Token to be configured for future slack bridge setup this is the only way forward
- add new bot setup w/ modern slack app + socket mode events api - remove old account setup steps - bot token + _classic_ slack apps - legacy webhook thing - remove auth comparison table (no meaningful comparison)
asdfzdfj
force-pushed
the
exp/slack-events-socket
branch
from
90b6953 to
2509c33
Compare
changelog entry added
at this point, might as well |
Thank you! Just to make sure i understand: what was previously called "legacy" is just a specific token type not dedicated code, so we only removed docs about something that did not exist? If so, we don't need to specify it in the changelog.
then feel free to add your username as maintainer in Apart from this minor nitpick, looks good to me! Feel free to merge after adding maintainer info :) |
there seems to be quite a bit of "legacy" components around on both side, us and slack's, and tbh I'm not sure I get it all either, but to the best that I understand: in the beginning there was legacy tester tokens from slack, and on our side there is perhaps the earliest iteration of slack bridge which consume these token, now relegated to a bridge named sometimes later, (classic) slack apps and its bot users become a thing, and the primary slack bridge (and now, modern/granular scopes slack apps is now a thing, which is what this patch is about, because these new apps cannot use RTM and classic slack apps is also marked "legacy" by slack) what I've removed here was setup info for
|
5 participants
the current slack bridge with bot token setup relies on legacy RTM API/system to receive messages for bridging, which is only available to classic slack apps, but as of now it's not possible to create new classic slack apps anymore (not even with the link in the current slack documents)
this patch adds support for using socket mode Events API to receive messages for bridging instead of RTM, however this will require an additional App Token with
connections:writescope to be suppliedfrom initial testing it seems to be working, but I have put this up as a WIP for now as it could use some polish and additional testing, especially with existing RTM based setup and confirm that it didn't break things over there