add support for slack Events api #66

ewingrj · 2018-01-13T23:58:14Z

This PR updates the bridge to use slacks events api & a bot user. This allows bridging of as many channels as you would like with only 1 slack integration per slack team/workspace.

This is fully backwards compatible with the incoming/outgoing webhooks.

Also improvea slack -> matrix file uploads

- abstracted out a BaseSlackHandler class which the SlackEventHandler & SlackHookHandler extend. - fix bug with room linking - enable slack msg transformations in SlackEventHandler - provide better file bridging support from slack -> matrix

erdnaxeli

Great PR, thanks a lot!

I need to test it, but it looks good.

erdnaxeli · 2018-01-14T14:08:10Z

README.md

+
+### Recommended 
+
+1. add a custom app to your slack team/workspace by visiting https://api.slack.com/apps


'Add' with a capital.

erdnaxeli · 2018-01-14T14:08:38Z

README.md

+
+   Note: media uploaded to matrix is currently not permissioned, and anyone with the link
+   can access the file. In order to make slack files visible to matrix users, this bridge
+   will set make the slack file visible to anyone with the url (including files in private channels).


'will make' no?

erdnaxeli · 2018-01-14T14:11:17Z

README.md

+       the channel. Unfortunately, it is not easily obtained from the Slack UI. The
+       easiest way to do this is to send a message from Slack to the bridge; the
+       bridge will log the channel ID as part of the unrecognised message output.
+       You can then take note of the `channel_id` field.


Actually the channel ID is in the channel URL: https://XXX.slack.com/messages/<channel id>/.

erdnaxeli · 2018-01-14T14:15:14Z

README.md

+
+   - files:write:user
+
+   Note: media uploaded to matrix is currently not permissioned, and anyone with the link


This note is confusing. The 'files:write:user' permission allow the bot to upload file to slack right? But in your note you talk about the other way.

erdnaxeli · 2018-01-15T23:19:51Z

lib/BridgedRoom.js

+        },
+        json: true,
+    }).then((response) => {
+        if (!response.team) return;


Why not Promise.resolve()?

there's no need, since we're already in a promise

Ok, but your are doing it line 447.

oops. removed.

erdnaxeli · 2018-01-15T23:32:55Z

lib/SlackEventHandler.js

+ *
+ * @param {Object} params The event request emitted.
+ * @param {string} params.event.user Slack user ID of user sending the message.
+ // * @param {string} params.user_name Slack user name of the user sending the message.


If it's not needed anymore, just remove it.

erdnaxeli · 2018-01-15T23:38:05Z

lib/SlackEventHandler.js

+    var room = this._main.getRoomBySlackChannelId(params.event.channel);
+    if (!room) throw new UnknownChannel(params.event.channel);
+
+    if (params.event.subtype === 'bot_message' && params.event.bot_id === room.getSlackBotId()) {


If there is not slack_bot_token, lookupAndSetUserInfo will never set slack_bot_id, so this condition is always true, and I'm afraid we have a loop.

The condition should be something like:

params.event.subtype === 'bot_message' && (!room.getSlackBotToken || params.event.bot_id === room.getSlackBotId())

I will let you answer, but if I understand your code we got here only if we are using the Event API, so slack_bot_id is defined.

yes, the SlackEventHandler should only be used if slack_bot_id is set. I added your check anyways, as it should save some loops when incorrectly setup

erdnaxeli · 2018-01-15T23:43:29Z

lib/SlackGhost.js

+                }
+                return undefined;
+            }) :
+        Promise.resolve(display_name);


It's not a one liner anymore, please use

var getDisplayName; if (!display_name) { … } else { result = Promise.resolve(display_name); }

erdnaxeli · 2018-01-15T23:44:16Z

lib/SlackGhost.js

+
+    return getDisplayName.then(display_name => {
+        if (!display_name) return Promise.resolve();
+        if (this._display_name === display_name) return Promise.resolve();


You can do both with one if.

erdnaxeli · 2018-01-15T23:48:28Z

lib/SlackHookHandler.js

@@ -53,9 +59,15 @@ SlackHookHandler.prototype.startAndListen = function(port, tls_config) {
            });

            request.on("end", () => {
-                var params = qs.parse(body);
+                var isEvent = request.headers['content-type'] === 'application/json' && request.method === 'POST';


If isEvent is true, this means it's an event from the Event API right? Could you just add a comment please?

ewingrj · 2018-01-16T04:19:36Z

Thanks for the 2nd pair of eyes and catching all the things I forgot to cleanup :)

erdnaxeli · 2018-01-16T08:14:08Z

lib/BridgedRoom.js

+            sendMessageParams.body.as_user = false;
+            sendMessageParams.body.channel = this._slack_channel_id;
+        }
+
        sendMessageParams.body.username = user.getDisplaynameForRoom(message.room_id);


Actually I am wondering if this will still work with a bot user. See here the part about "Authoring messages": https://api.slack.com/changelog/2017-09-the-one-about-usernames#changing.

If I am correct, a bot user cannot change the username anymore :(

I did some reading and this page seemed to suggest you could: https://api.slack.com/methods/chat.postMessage (I have no idea just providing links 😆 )

Hmm, it looks like maybe they are phasing out that support, which would be disasterous for this AS, both in webhook and bot mode.

The username & avatar changes definitely work as of now, not sure what will happen in the future.

erdnaxeli · 2018-01-18T23:06:29Z

lib/Main.js

+            // this was throwing an error when the bot was invited to a room by a user on
+            // a different homeserver ({"errcode":"M_FORBIDDEN","error":"You are not invited to this room."})
+            Promise.delay(30 * 1000).then(() => this.getBotIntent().join(ev.room_id));
+


Empty line.

And this feels like a bug in Synapse :s. I wonder if it can be reproduced with a normal client or if it is related to the appservice API.

erdnaxeli · 2018-01-18T23:30:42Z

The in / out webhooks still work, that's good. I test to link a room with event API tomorrow.

erdnaxeli · 2018-01-24T22:50:15Z

When issuing a link command, I got this message: @erdnaxeli:cervoi.se: Cannot link - TypeError: Cannot read property 'profile' of undefined.

erdnaxeli · 2018-01-24T23:08:05Z

lib/BridgedRoom.js

+    }).then((response) => {
+        if (!response.user && response.user.profile) return;
+
+        if (!this._slack_bot_id !== response.user.profile.bot_id) {


The error came from here, because response his:

{ ok: false, error: 'missing_scope', needed: 'users:read', provided: 'identify,bot,channels:history,groups:history,team:read,files:write:user' }

I added "users:read" in oauth permission, and "user_change" in events, but now when I add the link, I got the message "Row is now pending-name" but the room does not show up in rooms list and link is not working.

are you providing both the bot & user tokens? --slack_bot_token xoxb-xxxxxxxxxx-xxxxxxxxxxxxxxxxxxxx --slack_user_token xoxp-xxxxxxxx-xxxxxxxxx-xxxxxxxx-xxxxxxxx

and did you have the files:write:user permission. I don't think you would need this, but it would help me debug if I knew how your's was set

erdnaxeli · 2018-01-26T08:06:33Z

I provided both, and I added the file write permission. Le 25 janv. 2018 00:22, "perissology" <[email protected]> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In lib/BridgedRoom.js <#66 (comment)> : > + this._slack_user_id = response.user_id; + this._dirty = true; + } + + return rp({ + uri: 'https://slack.com/api/users.info', + qs: { + token: this._slack_bot_token, + user: response.user_id, + }, + json: true, + }); + }).then((response) => { + if (!response.user && response.user.profile) return; + + if (!this._slack_bot_id !== response.user.profile.bot_id) { and did you have the files:write:user permission. I don't think you would need this, but it would help me debug if I knew how your's was set — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#66 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdQ0b0YOU47q4NtJmL9WkjqKbYqGBUyks5tN7skgaJpZM4Rdamr> .

Cadair · 2018-02-17T20:16:09Z

Any progress on this one? I have a slack team that's run out of integrations, and I could really do with this!

pedropombeiro · 2018-03-01T17:11:43Z

README.md

+   the rooms.
+
+4. Click on `Event Subscriptions` and enable them. At this point, the bridge needs to be
+   started as slack will do some verification of the request rul. The request url should be


typo rul=url

Cadair · 2018-05-11T09:19:46Z

For what it is worth, I have been running this on a couple of slack teams for a few weeks now and everything seems to be working perfectly.

geleeroyale · 2018-07-31T18:20:36Z

I want to bring this PR to attention again - maybe it can be merged? Contributor perissology (Giveth) made this version and is running it for our bridge since January and it works really great.

Many other communities come to us and ask for bridging - it would be great to give them a detailed how-to with the appservice, but as of now we have to say "It's custom, changes to official service are pending review".

ara4n · 2018-09-17T16:51:17Z

@Half-Shot when you rejoin us next week please can you put this at the top of your todo list?

Half-Shot · 2018-09-17T17:17:03Z

Sure.

Cadair · 2018-09-18T10:02:23Z

Once this is in I can update my branches and send a couple more PRs with support for gitter-as style edits etc.

Half-Shot · 2018-09-18T10:08:09Z

Ooh, yes that would be exciting @Cadair

Half-Shot · 2018-09-24T14:04:25Z

Thanks @perissology for the hard work in doing this! I've continued the PR on #89.

ewingrj added 6 commits January 12, 2018 14:51

add support for send msgs as bot user

c5094d1

got basic slack event bridging

2139a7c

update readme

5797761

fix bug when failed to enablePublicSharing

a241862

small slack event handler refactor

d050846

erdnaxeli mentioned this pull request Jan 15, 2018

Managing integration not possible in non-public rooms #40

Closed

erdnaxeli suggested changes Jan 15, 2018

View reviewed changes

a bit more cleanup

9c692b3

ewingrj force-pushed the master branch from 95ffa5d to 9c692b3 Compare January 16, 2018 04:18

erdnaxeli reviewed Jan 16, 2018

View reviewed changes

ewingrj added 2 commits January 16, 2018 05:28

be consistent w/ returns

64b1f8d

fix bug when inviting bot with a user on another HS

beb4aac

erdnaxeli reviewed Jan 18, 2018

View reviewed changes

erdnaxeli approved these changes Jan 24, 2018

View reviewed changes

erdnaxeli reviewed Jan 24, 2018

View reviewed changes

fix some typos in if statements

6be1c78

ewingrj mentioned this pull request Jan 26, 2018

How to deal with threads #45

Closed

pedropombeiro reviewed Mar 1, 2018

View reviewed changes

Half-Shot mentioned this pull request Sep 17, 2018

Slack Events API matrix-org/issues-bridging#14

Closed

Half-Shot self-assigned this Sep 24, 2018

Half-Shot mentioned this pull request Sep 24, 2018

Slack Event API Support v2 #89

Merged

Half-Shot closed this Sep 24, 2018


		### Recommended

		1. add a custom app to your slack team/workspace by visiting https://api.slack.com/apps


		- files:write:user

		Note: media uploaded to matrix is currently not permissioned, and anyone with the link

add support for slack Events api #66

add support for slack Events api #66

Conversation

ewingrj commented Jan 13, 2018

erdnaxeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdnaxeli Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewingrj commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdnaxeli commented Jan 18, 2018

erdnaxeli commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdnaxeli commented Jan 26, 2018 via email

Cadair commented Feb 17, 2018

Choose a reason for hiding this comment

Cadair commented May 11, 2018

geleeroyale commented Jul 31, 2018

ara4n commented Sep 17, 2018

Half-Shot commented Sep 17, 2018

Cadair commented Sep 18, 2018

Half-Shot commented Sep 18, 2018

Half-Shot commented Sep 24, 2018

erdnaxeli Jan 15, 2018 •

edited

Loading