Merge pull request #482 from matrix-org/hs/misc-fixes

Small tweaks for puppeting code

changelog.d/482.bugfix

@@ -0,0 +1 @@
+Reduce chance of duplicate messages arriving on Matrix when using puppeting

src/AllowDenyList.ts

@@ -31,8 +31,8 @@ export class AllowDenyList {
             // Ensure the exact string matches.
             return new RegExp(`^${str}$`);
         }
-        // Otherwise, it's a real regex
-        return new RegExp(str);
+        // Otherwise, it's a real regex. Remove the leading and trailing slash.
+        return new RegExp(str.slice(1, str.length - 1));
     }
 
     private dmAllow?: {
@@ -83,7 +83,6 @@ export class AllowDenyList {
             (slackUsername && e.test(slackUsername)))) {
             return DenyReason.SLACK; // Slack user was not on the allow list
         }
-
         const deny = this.dmDeny;
         if (deny && deny.matrix?.length > 0 && deny.matrix.some((e) => e.test(matrixUser))) {
             return DenyReason.MATRIX; // Matrix user was on the deny list

src/BridgedRoom.ts

@@ -589,7 +589,7 @@ export class BridgedRoom {
         await puppetedClient.conversations.invite({channel: this.slackChannelId!, users: ghost.slackId });
     }
 
-    public async onSlackMessage(message: ISlackMessageEvent, content?: Buffer) {
+    public async onSlackMessage(message: ISlackMessageEvent) {
         if (this.slackTeamId && message.user) {
             // This just checks if the user *could* be puppeted. If they are, delay handling their incoming messages.
             const hasPuppet = null !== await this.main.datastore.getPuppetTokenBySlackId(this.slackTeamId, message.user);
@@ -601,14 +601,16 @@ export class BridgedRoom {
             // We sent this, ignore.
             return;
         }
-        // Dedupe across RTM/Event streams
-        this.addRecentSlackMessage(message.ts);
         try {
             const ghost = await this.main.ghostStore.getForSlackMessage(message, this.slackTeamId!);
             await ghost.update(message, this);
             await ghost.cancelTyping(this.MatrixRoomId); // If they were typing, stop them from doing that.
             this.slackSendLock = this.slackSendLock.finally(async () => {
-                return this.handleSlackMessage(message, ghost, content);
+                // Check again
+                if (!this.recentSlackMessages.includes(message.ts)) {
+                    return this.handleSlackMessage(message, ghost);
+                }
+                // We sent this, ignore
             });
             await this.slackSendLock;
         } catch (err) {
@@ -776,10 +778,13 @@ export class BridgedRoom {
         );
     }
 
-    private async handleSlackMessage(message: ISlackMessageEvent, ghost: SlackGhost, content?: Buffer) {
+    private async handleSlackMessage(message: ISlackMessageEvent, ghost: SlackGhost) {
         const eventTS = message.event_ts || message.ts;
         const channelId = this.slackChannelId!;
 
+        // Dedupe across RTM/Event streams
+        this.addRecentSlackMessage(message.ts);
+
         ghost.bumpATime();
         this.slackATime = Date.now() / 1000;
 

src/Main.ts

@@ -712,7 +712,7 @@ export class Main {
                 "The admin of this Slack bridge has denied users to directly message this Slack user.",
                 msgtype: "m.notice",
             });
-            await intent.leave();
+            await intent.leave(roomId);
             return;
         }
 
@@ -966,7 +966,7 @@ export class Main {
         const entries = await this.datastore.getAllRooms();
         log.info(`Found ${entries.length} room entries in store`);
         await Promise.all(entries.map(async (entry, i) => {
-            log.info(`[${i}/${entries.length}] Loading room entry ${entry.matrix_id}`);
+            log.info(`[${i+1}/${entries.length}] Loading room entry ${entry.matrix_id}`);
             try {
                 await this.startupLoadRoomEntry(entry, joinedRooms as string[], teamClients);
             } catch (ex) {
@@ -1214,7 +1214,8 @@ export class Main {
         return userLevel >= requiresLevel;
     }
 
-    public async setUserAccessToken(userId: string, teamId: string, slackId: string, accessToken: string, puppeting: boolean) {
+    public async setUserAccessToken(userId: string, teamId: string, slackId: string, accessToken: string, puppeting: boolean,
+        botAccessToken?: string) {
         const existingTeam = await this.datastore.getTeam(teamId);
         await this.datastore.insertAccount(userId, slackId, teamId, accessToken);
         if (puppeting) {
@@ -1228,8 +1229,15 @@ export class Main {
             });
         }
         log.info(`Set new access token for ${userId} (team: ${teamId}, puppeting: ${puppeting})`);
+        if (botAccessToken) {
+            // Rather than upsert the values we were given, use the
+            // access token to validate and make additional requests
+            await this.clientFactory.upsertTeamByToken(
+                botAccessToken,
+            );
+        }
         if (!existingTeam && !puppeting && this.teamSyncer) {
-            log.info("This is a new team, so syncing members");
+            log.info("This is a new team, so syncing members and channels");
             try {
                 await this.teamSyncer.syncItems(
                     teamId,

src/SlackEventHandler.ts

@@ -315,24 +315,8 @@ export class SlackEventHandler extends BaseSlackHandler {
             return room.onSlackMessage(event);
         }
 
-        let content: Buffer|undefined;
-
-        if (msg.subtype === "file_share" && msg.file) {
-            // we need a user token to be able to enablePublicSharing
-            if (room.SlackClient) {
-                // TODO check is_public when matrix supports authenticated media
-                // https://github.com/matrix-org/matrix-doc/issues/701
-                try {
-                    msg.file = await this.enablePublicSharing(msg.file, room.SlackClient);
-                    content = await this.fetchFileContent(msg.file);
-                } catch {
-                    // Couldn't get a shareable URL for the file, oh well.
-                }
-            }
-        }
-
         msg.text = await this.doChannelUserReplacements(msg, msg.text!, room.SlackClient);
-        return room.onSlackMessage(msg, content);
+        return room.onSlackMessage(msg);
     }
 
     private async handleReaction(event: ISlackEventReaction, teamId: string) {

src/SlackHookHandler.ts

@@ -300,7 +300,7 @@ export class SlackHookHandler extends BaseSlackHandler {
         // them by now
         PRESERVE_KEYS.forEach((k) => lookupRes.message[k] = params[k]);
         lookupRes.message.text = await this.doChannelUserReplacements(lookupRes.message, text, room.SlackClient);
-        return room.onSlackMessage(lookupRes.message, lookupRes.content);
+        return room.onSlackMessage(lookupRes.message);
     }
 
     private async handleAuthorize(token: string, params: qs.ParsedUrlQuery) {
@@ -351,14 +351,8 @@ export class SlackHookHandler extends BaseSlackHandler {
                 response.user_id,
                 response.access_token,
                 response.bot === undefined,
+                response.bot?.bot_access_token,
             );
-            if (response.bot) {
-                // Rather than upsert the values we were given, use the
-                // access token to validate and make additional requests
-                await this.main.clientFactory.upsertTeamByToken(
-                    response.bot.bot_access_token,
-                );
-            }
         } catch (err) {
             log.error("Error during handling of an oauth token:", err);
             return {
@@ -384,7 +378,7 @@ export class SlackHookHandler extends BaseSlackHandler {
      *     formatted as a float.
      */
     private async lookupMessage(channelID: string, timestamp: string, client: WebClient): Promise<{
-        message: ISlackMessageEvent, content: Buffer|undefined}> {
+        message: ISlackMessageEvent}> {
         // Look up all messages at the exact timestamp we received.
         // This has microsecond granularity, so should return the message we want.
         const response = (await client.conversations.history({
@@ -411,17 +405,6 @@ export class SlackHookHandler extends BaseSlackHandler {
         const message = response.messages[0];
         log.debug("Looked up message from history as " + JSON.stringify(message));
 
-        if (message.subtype === "file_share") {
-            try {
-                message.file = await this.enablePublicSharing(message.file!, client);
-                const content = await this.fetchFileContent(message.file!);
-                return { message, content };
-            } catch (err) {
-                log.error("Failed to get file content: ", err);
-                // Fall through here and handle like a normal message.
-            }
-        }
-
-        return { message, content: undefined };
+        return { message };
     }
 }