Support removing attachments from Slack messages

We now track which attachments was linked to which Matrix message, and support both removing them individually and dropping the whole event "tree".
matrix-org · Apr 5, 2024 · ae509b0 · ae509b0
1 parent a4fe71c
commit ae509b0
Show file tree

Hide file tree

Showing 5 changed files with 78 additions and 46 deletions.
diff --git a/src/BridgedRoom.ts b/src/BridgedRoom.ts
@@ -879,7 +879,7 @@ export class BridgedRoom {
                 formatted_body: `<a href="${link}">${file.name}</a>`,
                 msgtype: "m.text",
             };
-            await ghost.sendMessage(this.matrixRoomId, messageContent, channelId, slackEventId, { type: "attachment" });
+            await ghost.sendMessage(this.matrixRoomId, messageContent, channelId, slackEventId, { attachment_id: file.id });
             return;
         }
 
@@ -918,7 +918,7 @@ export class BridgedRoom {
                 formatted_body: htmlCode,
                 msgtype: "m.text",
             };
-            await ghost.sendMessage(this.matrixRoomId, messageContent, channelId, slackEventId, { type: "attachment" });
+            await ghost.sendMessage(this.matrixRoomId, messageContent, channelId, slackEventId, { attachment_id: file.id });
             return;
         }
 
@@ -955,7 +955,7 @@ export class BridgedRoom {
             slackFileToMatrixMessage(file, fileContentUri, thumbnailContentUri),
             channelId,
             slackEventId,
-            { type: "attachment" },
+            { attachment_id: file.id },
         );
     }
 
@@ -1020,6 +1020,30 @@ export class BridgedRoom {
             const newMessageRich = substitutions.slackToMatrix(message.text!);
             const newMessage = ghost.prepareBody(newMessageRich);
 
+            // Check if any of the attachments have been deleted.
+            // Slack unfortunately puts a "tombstone" in both message versions in this event,
+            // so let's try to remove every single one even if we may have deleted it before.
+            for (const file of message.message?.files ?? []) {
+                if (file.mode === 'tombstone') {
+                    const events = await this.main.datastore.getEventsBySlackId(channelId, message.previous_message!.ts);
+                    const event = events.find(e => e._extras.attachment_id === file.id);
+                    if (event) {
+                        const team = message.team_id ? await this.main.datastore.getTeam(message.team_id) : null;
+                        if (!team) {
+                            log.warn("Cannot determine team for message", message, "so we cannot delete attachment", file.id);
+                            continue;
+                        }
+                        try {
+                            await this.deleteMessage(message, event, team);
+                        } catch (err) {
+                            log.warn(`Failed to delete attachment ${file.id}:`, err);
+                        }
+                    } else {
+                        log.warn(`Tried to remove tombstoned attachmend ${file.id} but we didn't find a Matrix event for it`);
+                    }
+                }
+            }
+
             // The substitutions might make the messages the same
             if (previousMessage === newMessage) {
                 log.debug("Ignoring edit message because messages are the same post-substitutions.");
@@ -1240,6 +1264,39 @@ export class BridgedRoom {
             this.recentSlackMessages.shift();
         }
     }
+
+    public async deleteMessage(msg: ISlackMessageEvent, event: EventEntry, team: TeamEntry): Promise<void> {
+        const previousMessage = msg.previous_message;
+        if (!previousMessage) {
+            throw new Error(`Cannot delete message with no previous_message: ${JSON.stringify(msg)}`);
+        }
+
+        // Try to determine the Matrix user responsible for deleting the message, fallback to our main bot if all else fails
+        if (!previousMessage.user) {
+            log.warn("We don't know the original sender of", previousMessage, "will try to remove with our bot");
+        }
+
+        const isOurMessage = previousMessage.subtype === 'bot_message' && (previousMessage.bot_id === team.bot_id);
+
+        if (previousMessage.user && !isOurMessage) {
+            try {
+                const ghost = await this.main.ghostStore.get(previousMessage.user, previousMessage.team_domain, previousMessage.team);
+                await ghost.redactEvent(event.roomId, event.eventId);
+                return;
+            } catch (err) {
+                log.warn(`Failed to remove message on behalf of ${previousMessage.user}, falling back to our bot`);
+            }
+        }
+
+        try {
+            const botClient = this.main.botIntent.matrixClient;
+            await botClient.redactEvent(event.roomId, event.eventId, "Deleted on Slack");
+        } catch (err) {
+            throw new Error(
+                `Failed to remove message ${JSON.stringify(previousMessage)} with our Matrix bot. insufficient power level? Error: ${err}`
+            );
+        }
+    }
 }
 
 /**

diff --git a/src/SlackEventHandler.ts b/src/SlackEventHandler.ts
@@ -18,7 +18,7 @@ import { BaseSlackHandler, ISlackEvent, ISlackMessageEvent, ISlackUser } from ".
 import { BridgedRoom } from "./BridgedRoom";
 import { Main, METRIC_RECEIVED_MESSAGE } from "./Main";
 import { Logger } from "matrix-appservice-bridge";
-import { TeamEntry } from "./datastore/Models";
+import { EventEntry, TeamEntry } from "./datastore/Models";
 const log = new Logger("SlackEventHandler");
 
 /**
@@ -292,7 +292,8 @@ export class SlackEventHandler extends BaseSlackHandler {
             }
         } else if (msg.subtype === "message_deleted" && msg.deleted_ts) {
             try {
-                await this.deleteMessage(msg, team);
+                const events = await this.main.datastore.getEventsBySlackId(msg.channel, msg.deleted_ts!);
+                await Promise.all(events.map(event => room.deleteMessage(msg, event, team)));
             } catch (err) {
                 log.error(err);
             }
@@ -316,42 +317,6 @@ export class SlackEventHandler extends BaseSlackHandler {
         return room.onSlackMessage(msg);
     }
 
-    private async deleteMessage(msg: ISlackMessageEvent, team: TeamEntry): Promise<void> {
-        const originalEvent = await this.main.datastore.getEventBySlackId(msg.channel, msg.deleted_ts!);
-        if (originalEvent) {
-            const previousMessage = msg.previous_message;
-            if (!previousMessage) {
-                throw new Error(`Cannot delete message with no previous_message: ${JSON.stringify(msg)}`);
-            }
-
-            // Try to determine the Matrix user responsible for deleting the message, fallback to our main bot if all else fails
-            if (!previousMessage.user) {
-                log.warn("We don't know the original sender of", previousMessage, "will try to remove with our bot");
-            }
-
-            const isOurMessage = previousMessage.subtype === 'bot_message' && (previousMessage.bot_id === team.bot_id);
-
-            if (previousMessage.user && !isOurMessage) {
-                try {
-                    const ghost = await this.main.ghostStore.get(previousMessage.user, previousMessage.team_domain, previousMessage.team);
-                    await ghost.redactEvent(originalEvent.roomId, originalEvent.eventId);
-                    return;
-                } catch (err) {
-                    log.warn(`Failed to remove message on behalf of ${previousMessage.user}, falling back to our bot`);
-                }
-            }
-
-            try {
-                const botClient = this.main.botIntent.matrixClient;
-                await botClient.redactEvent(originalEvent.roomId, originalEvent.eventId, "Deleted on Slack");
-            } catch (err) {
-                throw new Error(
-                    `Failed to remove message ${JSON.stringify(previousMessage)} with our Matrix bot. insufficient power level? Error: ${err}`
-                );
-            }
-        }
-    }
-
     private async handleReaction(event: ISlackEventReaction, teamId: string) {
         // Reactions store the channel in the item
         const channel = event.item.channel;

diff --git a/src/datastore/Models.ts b/src/datastore/Models.ts
@@ -50,7 +50,7 @@ export interface EventEntry {
 }
 
 export interface EventEntryExtra {
-    type?: 'attachment';
+    attachment_id?: string;
     slackThreadMessages?: string[];
 }
 
@@ -114,6 +114,7 @@ export interface Datastore extends ProvisioningStore {
     upsertEvent(roomId: string, eventId: string, channelId: string, ts: string, extras?: EventEntryExtra): Promise<null>;
     upsertEvent(roomIdOrEntry: EventEntry): Promise<null>;
     getEventByMatrixId(roomId: string, eventId: string): Promise<EventEntry|null>;
+    getEventsBySlackId(channelId: string, ts: string): Promise<EventEntry[]>;
     getEventBySlackId(channelId: string, ts: string): Promise<EventEntry|null>;
     deleteEventByMatrixId(roomId: string, eventId: string): Promise<null>;
 

diff --git a/src/datastore/NedbDatastore.ts b/src/datastore/NedbDatastore.ts
@@ -241,6 +241,10 @@ export class NedbDatastore implements Datastore {
         return this.storedEventToEventEntry(storedEvent);
     }
 
+    public async getEventsBySlackId(channelId: string, ts: string): Promise<EventEntry[]> {
+        return this.getEventBySlackId(channelId, ts).then(e => e ? [e] : []);
+    }
+
     public async getEventBySlackId(channelId: string, ts: string): Promise<EventEntry|null> {
         const storedEvent = await this.eventStore.getEntryByRemoteId(channelId, ts);
         if (!storedEvent) {

diff --git a/src/datastore/postgres/PgDatastore.ts b/src/datastore/postgres/PgDatastore.ts
@@ -178,9 +178,8 @@ export class PgDatastore implements Datastore, ClientEncryptionStore, Provisioni
             });
     }
 
-    public async getEventBySlackId(slackChannel: string, slackTs: string): Promise<EventEntry|null> {
-        log.debug(`getEventBySlackId: ${slackChannel} ${slackTs}`);
-        const events = await this.postgresDb.manyOrNone(
+    public async getEventsBySlackId(slackChannel: string, slackTs: string): Promise<EventEntry[]> {
+        return this.postgresDb.manyOrNone(
             "SELECT * FROM events WHERE slackChannel = ${slackChannel} AND slackTs = ${slackTs}",
             { slackChannel, slackTs }
         ).then(entries => entries.map(e => ({
@@ -190,8 +189,14 @@ export class PgDatastore implements Datastore, ClientEncryptionStore, Provisioni
             slackTs,
             _extras: JSON.parse(e.extras) as EventEntryExtra,
         })));
+    }
 
-        return events.find(e => e._extras.type !== 'attachment') ?? null;
+    /**
+     * @deprecated One Slack event may map to many Matrix events -- use getEventsBySlackId()
+     */
+    public async getEventBySlackId(slackChannel: string, slackTs: string): Promise<EventEntry|null> {
+        const events = await this.getEventsBySlackId(slackChannel, slackTs);
+        return events.find(e => !e._extras.attachment_id) ?? null;
     }
 
     public async deleteEventByMatrixId(roomId: string, eventId: string): Promise<null> {