From bb0a51348a09b06dec00d1a89f2beb4e0015cefc Mon Sep 17 00:00:00 2001
From: Sorunome <mail@sorunome.de>
Date: Thu, 16 May 2019 17:34:49 +0200
Subject: [PATCH] improve how channel aliases are fetched

---
 src/bot.ts                           |  2 +-
 src/channelsyncroniser.ts            | 22 ++++++++++++
 src/discordmessageprocessor.ts       | 32 +++++++-----------
 test/test_channelsyncroniser.ts      | 48 +++++++++++++++++++++++++-
 test/test_discordmessageprocessor.ts | 50 +++++++++++++---------------
 5 files changed, 106 insertions(+), 48 deletions(-)

diff --git a/src/bot.ts b/src/bot.ts
index db1cd29..3f55abc 100644
--- a/src/bot.ts
+++ b/src/bot.ts
@@ -93,7 +93,7 @@ export class DiscordBot {
         this.provisioner = new Provisioner(store.roomStore);
         this.clientFactory = new DiscordClientFactory(store, config.auth);
         this.discordMsgProcessor = new DiscordMessageProcessor(
-            new DiscordMessageProcessorOpts(config.bridge.domain, this, this.bridge),
+            new DiscordMessageProcessorOpts(config.bridge.domain, this),
         );
         this.presenceHandler = new PresenceHandler(this);
         this.roomHandler = new MatrixRoomHandler(this, config, this.provisioner, bridge, store.roomStore);
diff --git a/src/channelsyncroniser.ts b/src/channelsyncroniser.ts
index c948f35..fcb2f3b 100644
--- a/src/channelsyncroniser.ts
+++ b/src/channelsyncroniser.ts
@@ -150,6 +150,28 @@ export class ChannelSyncroniser {
         return rooms.map((room) => room.matrix!.getId() as string);
     }
 
+    public async GetAliasFromChannel(channel: Discord.Channel): Promise<string | null> {
+        let rooms: string[] = [];
+        try {
+            rooms = await this.GetRoomIdsFromChannel(channel);
+        } catch (err) { } // do nothing, our rooms array will just be empty
+        for (const room of rooms) {
+            try {
+                const al = (await this.bridge.getIntent().getClient()
+                    .getStateEvent(room, "m.room.canonical_alias")).alias;
+                if (al) {
+                    return al; // we are done, we found an alias
+                }
+            } catch (err) { } // do nothing, as if we error we just roll over to the next entry
+        }
+        const guildChannel = channel as Discord.TextChannel;
+        if (!guildChannel.guild) {
+            return null; // we didn't pass a guild, so we have no way of bridging this room, thus no alias
+        }
+        // at last, no known canonical aliases and we are ag uild....so we know an alias!
+        return `#_discord_${guildChannel.guild.id}_${channel.id}:${this.config.bridge.domain}`;
+    }
+
     public async GetChannelUpdateState(channel: Discord.TextChannel, forceUpdate = false): Promise<IChannelState> {
         log.verbose(`State update request for ${channel.id}`);
         const channelState: IChannelState = Object.assign({}, DEFAULT_CHANNEL_STATE, {
diff --git a/src/discordmessageprocessor.ts b/src/discordmessageprocessor.ts
index 690d8f9..a43d8c5 100644
--- a/src/discordmessageprocessor.ts
+++ b/src/discordmessageprocessor.ts
@@ -39,7 +39,7 @@ const CHANNEL_INSERT_REGEX = /\x01chan\x01([0-9]*)\x01/;
 const ID_CHANNEL_INSERT_REGEX = 1;
 
 export class DiscordMessageProcessorOpts {
-    constructor(readonly domain: string, readonly bot?: DiscordBot, readonly bridge?: Bridge) {
+    constructor(readonly domain: string, readonly bot?: DiscordBot) {
 
     }
 }
@@ -64,7 +64,7 @@ export class DiscordMessageProcessor {
     constructor(opts: DiscordMessageProcessorOpts, bot: DiscordBot | null = null) {
         // Backwards compat
         if (bot !== null) {
-            this.opts = new DiscordMessageProcessorOpts(opts.domain, bot, opts.bridge);
+            this.opts = new DiscordMessageProcessorOpts(opts.domain, bot);
         } else {
             this.opts = opts;
         }
@@ -268,10 +268,11 @@ export class DiscordMessageProcessor {
             const animated = results[ANIMATED_MXC_INSERT_REGEX_GROUP] === "1";
             const id = results[ID_MXC_INSERT_REGEX_GROUP];
             let replace = "";
+            const nameHtml = escapeHtml(name);
             try {
                 const mxcUrl = await this.opts.bot!.GetEmoji(name, animated, id);
                 if (html) {
-                    replace = `<img alt="${escapeHtml(name)}" title="${escapeHtml(name)}" ` +
+                    replace = `<img alt="${nameHtml}" title="${nameHtml}" ` +
                         `height="${EMOJI_SIZE}" src="${mxcUrl}" />`;
                 } else {
                     replace = `:${name}:`;
@@ -281,7 +282,7 @@ export class DiscordMessageProcessor {
                     `Could not insert emoji ${id} for msg ${msg.id} in guild ${msg.guild.id}: ${ex}`,
                 );
                 if (html) {
-                    replace = `&lt;${animated ? "a" : ""}:${escapeHtml(name)}:${id}&gt;`;
+                    replace = `&lt;${animated ? "a" : ""}:${nameHtml}:${id}&gt;`;
                 } else {
                     replace = `<${animated ? "a" : ""}:${name}:${id}>`;
                 }
@@ -299,22 +300,13 @@ export class DiscordMessageProcessor {
             let replace = "";
             const channel = msg.guild.channels.get(id);
             if (channel) {
-                const name = "#" + channel.name;
-                let alias = `#_discord_${msg.guild.id}_${id}:${this.opts.domain}`;
-                try {
-                    // we don't bother here much of checking if objects exists, as the error is catched
-                    // and we use the default alias then, as we wanted to anyways
-                    const matrixRoom = (await this.opts.bot!.ChannelSyncroniser.GetRoomIdsFromChannel(channel))[0];
-                    if (matrixRoom) {
-                        const al = (await this.opts.bridge.getIntent().getClient()
-                            .getStateEvent(matrixRoom, "m.room.canonical_alias")).alias;
-                        if (al) {
-                            alias = al;
-                        }
-                    }
-                } catch (err) { } // no rooms found, just use default
-                replace = html ? `<a href="${MATRIX_TO_LINK}${escapeHtml(alias)}">${escapeHtml(name)}</a>` : name;
-            } else {
+                const alias = await this.opts.bot!.ChannelSyncroniser.GetAliasFromChannel(channel);
+                if (alias) {
+                    const name = "#" + channel.name;
+                    replace = html ? `<a href="${MATRIX_TO_LINK}${escapeHtml(alias)}">${escapeHtml(name)}</a>` : name;
+                }
+            }
+            if (!replace) {
                 replace = html ? `&lt;#${escapeHtml(id)}&gt;` : `<#${id}>`;
             }
             content = content.replace(results[0], replace);
diff --git a/test/test_channelsyncroniser.ts b/test/test_channelsyncroniser.ts
index 7d90f6e..cbc338f 100644
--- a/test/test_channelsyncroniser.ts
+++ b/test/test_channelsyncroniser.ts
@@ -1,5 +1,5 @@
 /*
-Copyright 2018 matrix-appservice-discord
+Copyright 2018, 2019 matrix-appservice-discord
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -83,6 +83,15 @@ function CreateChannelSync(remoteChannels: any[] = []): ChannelSyncroniser {
                             ALIAS_DELETED = true;
                         },
                         getStateEvent: async (mxid, event) => {
+                            if (event === "m.room.canonical_alias") {
+                                if (mxid === "!valid:localhost") {
+                                    return {
+                                        alias: "#alias:localhost",
+                                    };
+                                } else {
+                                    return null;
+                                }
+                            }
                             return event;
                         },
                         sendStateEvent: async (mxid, event, data) => {
@@ -278,6 +287,43 @@ describe("ChannelSyncroniser", () => {
             }
         });
     });
+    describe("GetAliasFromChannel", () => {
+        const getIds = async (chan) => {
+            if (chan.id === "678") {
+                return ["!valid:localhost"];
+            }
+            throw new Error("invalid");
+        };
+        it("Should get one canonical alias for a room", async () => {
+            const chan = new MockChannel();
+            chan.id = "678";
+            const channelSync = CreateChannelSync();
+            channelSync.GetRoomIdsFromChannel = getIds;
+            const alias = await channelSync.GetAliasFromChannel(chan as any);
+
+            expect(alias).to.equal("#alias:localhost");
+        });
+        it("Should return null if no alias found and no guild present", async () => {
+            const chan = new MockChannel();
+            chan.id = "123";
+            const channelSync = CreateChannelSync();
+            channelSync.GetRoomIdsFromChannel = getIds;
+            const alias = await channelSync.GetAliasFromChannel(chan as any);
+
+            expect(alias).to.equal(null);
+        });
+        it("Should return a #_discord_ alias if a guild is present", async () => {
+            const chan = new MockChannel();
+            const guild = new MockGuild("123");
+            chan.id = "123";
+            chan.guild = guild;
+            const channelSync = CreateChannelSync();
+            channelSync.GetRoomIdsFromChannel = getIds;
+            const alias = await channelSync.GetAliasFromChannel(chan as any);
+
+            expect(alias).to.equal("#_discord_123_123:localhost");
+        });
+    });
     describe("GetChannelUpdateState", () => {
         it("will do nothing on no rooms", async () => {
             const chan = new MockChannel();
diff --git a/test/test_discordmessageprocessor.ts b/test/test_discordmessageprocessor.ts
index 9b4b1ea..ab59077 100644
--- a/test/test_discordmessageprocessor.ts
+++ b/test/test_discordmessageprocessor.ts
@@ -28,11 +28,11 @@ import { MockRole } from "./mocks/role";
 
 const bot = {
     ChannelSyncroniser: {
-        GetRoomIdsFromChannel: async (chan) => {
-            if (chan.id === "678") {
-                return ["!valid:localhost"];
+        GetAliasFromChannel: async (chan) => {
+            if (chan.id === "456") {
+                return "#_discord_123_456:localhost";
             }
-            throw new Error("invalid");
+            return null;
         },
     },
     GetEmoji: async (name: string, animated: boolean, id: string): Promise<string> => {
@@ -43,24 +43,6 @@ const bot = {
         }
     },
 };
-const bridge = {
-    getIntent: () => {
-        return {
-            getClient: () => {
-                return {
-                    getStateEvent: async (roomId, state) => {
-                        if (roomId === "!valid:localhost" && state === "m.room.canonical_alias") {
-                            return {
-                                alias: "#alias:localhost",
-                            };
-                        }
-                        return null;
-                    },
-                };
-            },
-        };
-    },
-};
 
 describe("DiscordMessageProcessor", () => {
     describe("init", () => {
@@ -487,19 +469,35 @@ describe("DiscordMessageProcessor", () => {
             Chai.assert.equal(reply, "Hello <a href=\"https://matrix.to/#/#_discord_123" +
                 "_456:localhost\">#TestChannel</a>");
         });
-        it("uses the canonical alias, if applicable", async () => {
+        it("processes multiple channels correctly", async () => {
+            const processor = new DiscordMessageProcessor(
+                new DiscordMessageProcessorOpts("localhost"), bot as DiscordBot);
+            const guild: any = new MockGuild("123", []);
+            const channel = new Discord.TextChannel(guild, {id: "456", name: "TestChannel"});
+            guild.channels.set("456", channel);
+            const msg = new MockMessage(channel) as any;
+            const content = "Hello \x01chan\x01456\x01 \x01chan\x01456\x01";
+            let reply = await processor.InsertChannelPills(content, msg);
+            Chai.assert.equal(reply, "Hello #TestChannel #TestChannel");
+
+            reply = await processor.InsertChannelPills(content, msg, true);
+            Chai.assert.equal(reply, "Hello <a href=\"https://matrix.to/#/#_discord_123" +
+                "_456:localhost\">#TestChannel</a> <a href=\"https://matrix.to/#/#_discord_123" +
+                "_456:localhost\">#TestChannel</a>");
+        });
+        it("processes channels without alias correctly", async () => {
             const processor = new DiscordMessageProcessor(
-                new DiscordMessageProcessorOpts("localhost", bot as DiscordBot, bridge as any));
+                new DiscordMessageProcessorOpts("localhost"), bot as DiscordBot);
             const guild: any = new MockGuild("123", []);
             const channel = new Discord.TextChannel(guild, {id: "678", name: "TestChannel"});
             guild.channels.set("678", channel);
             const msg = new MockMessage(channel) as any;
             const content = "Hello \x01chan\x01678\x01";
             let reply = await processor.InsertChannelPills(content, msg);
-            Chai.assert.equal(reply, "Hello #TestChannel");
+            Chai.assert.equal(reply, "Hello <#678>");
 
             reply = await processor.InsertChannelPills(content, msg, true);
-            Chai.assert.equal(reply, "Hello <a href=\"https://matrix.to/#/#alias:localhost\">#TestChannel</a>");
+            Chai.assert.equal(reply, "Hello &lt;#678&gt;");
         });
     });
     describe("InsertEmbeds", () => {
-- 
GitLab