Merge branch 'develop' into release-1.9.0-rc3

changelog.d/624.bugfix

@@ -0,0 +1 @@
+Fix a bug where typing in an admin command in the bridge admin room would cause the bridge to immediately exit.

package.json

@@ -49,7 +49,7 @@
     "uuid": "^8.3.2",
     "winston-daily-rotate-file": "^4.5.5",
     "winston": "^3.3.3",
-    "yargs": "^17.0.1"
+    "yargs": "^17.2.1"
   },
   "devDependencies": {
     "@types/chai": "^4.2.21",
@@ -62,7 +62,7 @@
     "@types/randomstring": "^1.1.7",
     "@types/uuid": "^8.3.1",
     "@types/yargs-parser": "^20.2.1",
-    "@types/yargs": "^17.0.2",
+    "@types/yargs": "^17.0.3",
     "@typescript-eslint/eslint-plugin": "^4.28.4",
     "@typescript-eslint/parser": "^4.28.4",
     "chai": "^4.3.4",

src/AdminCommand.ts

@@ -19,7 +19,6 @@ import { Options } from "yargs";
 export type ResponseCallback = (response: string) => void;
 interface IHandlerArgs {
     matched: () =>  void;
-    completed: (err: Error|null) =>  void;
     respond: ResponseCallback;
     // yargs annoyingly puts the string parameters in with the more complex types
     // above. To save lots of if|else|other types, unknown is being used here.
@@ -39,9 +38,7 @@ export class AdminCommand {
         argv.matched();
         try {
             await this.cb(argv);
-            argv.completed(null);
         } catch (ex) {
-            argv.completed(ex as Error);
         }
     }
 

src/AdminCommands.ts

@@ -49,12 +49,13 @@ export class AdminCommands {
             .help(false); // We provide our own help, and version is not required.
 
         this.commands.forEach((cmd) => {
-            this.yargs = this.yargs.command(cmd.command, cmd.description, ((yg) => {
-                if (cmd.options) {
-                    return yg.options(cmd.options);
-                }
-            // TODO: Fix typing
-            }) as any, cmd.handler.bind(cmd));
+            this.yargs = this.yargs.command(
+                cmd.command,
+                cmd.description,
+                (yg: yargs.Argv) => void yg.options(cmd.options || {}),
+                cmd.handler.bind(cmd)
+            ).exitProcess(false)
+                .showHelpOnFail(false);
         });
     }
 
@@ -150,7 +151,7 @@ export class AdminCommands {
                 respond("  Inbound ID: " + bridgedRoom.InboundId);
                 respond("  Inbound URL: " + this.main.getInboundUrlForRoom(bridgedRoom));
                 respond("  Matrix room ID: " + bridgedRoom.MatrixRoomId);
-                respond("  Using RTM: " + this.main.teamIsUsingRtm(bridgedRoom.SlackTeamId!));
+                respond("  Using RTM: " + (bridgedRoom.SlackTeamId ? this.main.teamIsUsingRtm(bridgedRoom.SlackTeamId) : false).toString());
             },
             {
                 channel_id: {
@@ -175,8 +176,12 @@ export class AdminCommands {
                 team_id?: string,
             }) => {
                 try {
+                    if (!room) {
+                        respond("Room not provided");
+                        return;
+                    }
                     const r = await this.main.actionLink({
-                        matrix_room_id: room!,
+                        matrix_room_id: room,
                         slack_bot_token,
                         team_id,
                         slack_channel_id: channel_id,
@@ -227,11 +232,13 @@ export class AdminCommands {
                 respond: ResponseCallback,
                 room?: string,
             }) => {
+                if (!room) {
+                    respond("Room not provided");
+                    return;
+                }
                 try {
                     await this.main.actionUnlink({
-                        matrix_room_id: room!,
-                        // slack_channel_name: channel,
-                        // slack_channel_id: channel_id,
+                        matrix_room_id: room,
                     });
                     respond("Unlinked");
                 } catch (ex) {
@@ -273,11 +280,14 @@ export class AdminCommands {
                 respond: ResponseCallback,
                 room?: string,
             }) => {
-                const roomId: string = room!;
-                const userIds = await this.main.listGhostUsers(roomId);
-                respond(`Draining ${userIds.length} ghosts from ${roomId}`);
-                await Promise.all(userIds.map(async (userId) => this.main.getIntent(userId).leave(roomId)));
-                await this.main.botIntent.leave(roomId);
+                if (!room) {
+                    respond("Room not provided");
+                    return;
+                }
+                const userIds = await this.main.listGhostUsers(room);
+                respond(`Draining ${userIds.length} ghosts from ${room}`);
+                await Promise.all(userIds.map(async (userId) => this.main.getIntent(userId).leave(room)));
+                await this.main.botIntent.leave(room);
                 respond("Drained");
             },
             {
@@ -316,7 +326,11 @@ export class AdminCommands {
                     respond("Oauth is not configured on this bridge");
                     return;
                 }
-                const token = this.main.oauth2.getPreauthToken(userId!);
+                if (!userId) {
+                    respond("userId not provided");
+                    return;
+                }
+                const token = this.main.oauth2.getPreauthToken(userId);
                 const authUri = this.main.oauth2.makeAuthorizeURL(
                     token,
                     token,
@@ -370,7 +384,7 @@ export class AdminCommands {
         // yargs has no way to tell us if a command matched, so we have this
         // slightly whacky function to manage it.
         let matched = false;
-        await this.yargs.parseAsync(argv, {
+        await this.yargs.parse(argv, {
             matched: () => { matched = true; },
             respond,
         });

src/Main.ts

@@ -907,8 +907,6 @@ export class Main {
             return;
         }
 
-        log.info("Admin: " + cmd);
-
         const response: string[] = [];
         const respond = (responseMsg: string) => {
             if (!response) {
@@ -928,16 +926,16 @@ export class Main {
                 respond("Done");
             }
         } catch (ex) {
-            log.debug(`Command '${cmd}' failed to complete:`, ex);
-            respond("Command failed: " + ex);
+            const error = ex as {message: string; name?: "YError"};
+            log.warn(`Command '${cmd}' failed to complete:`, ex);
+            // YErrors are yargs errors when the user inputs the command wrong.
+            respond(`${error.name === "YError" ? error.message : "Command failed: See the logs for details."}`);
         }
 
         const message = response.join("\n");
 
         await this.botIntent.sendEvent(ev.room_id, "m.room.message", {
             body: message,
-            format: "org.matrix.custom.html",
-            formatted_body: `<pre>${message}</pre>`,
             msgtype: "m.notice",
         });
     }

src/OAuth2.ts

@@ -47,7 +47,7 @@ const TOKEN_EXPIRE_MS = 5 * 60 * 1000; // 5 minutes
 
 export class OAuth2 {
     private readonly main: Main;
-    private readonly userTokensWaiting: Map<string, string>;
+    private readonly userTokensWaiting: Map<string, {userId: string; expireAfter: number}>;
     private readonly clientId: string;
     private readonly clientSecret: string;
     private readonly redirectPrefix: string;
@@ -116,20 +116,17 @@ export class OAuth2 {
         // NOTE: We use 32 because we need to use it into SlackEventHandler which
         // expects inbound roomIds to be 32 chars.
         const token = uuid().substr(0, INTERNAL_ID_LEN);
-        this.userTokensWaiting.set(token, userId);
-        setTimeout(() => {
-            if (this.userTokensWaiting.delete(token)) {
-                log.info(`Token for ${userId} has expired`);
-                this.main.incCounter(METRIC_OAUTH_SESSIONS, {result: "failed", reason: "timeout"});
-            }
-        }, TOKEN_EXPIRE_MS);
+        this.userTokensWaiting.set(token, {userId, expireAfter: TOKEN_EXPIRE_MS + Date.now()});
         return token;
     }
 
     public getUserIdForPreauthToken(token: string): string|null {
         const v = this.userTokensWaiting.get(token);
         this.userTokensWaiting.delete(token);
-        return v || null;
+        if (v && v.expireAfter >= Date.now()) {
+            return v.userId;
+        }
+        return null;
     }
 
 

tests/unit/AdminCommandTest.ts

@@ -62,7 +62,6 @@ describe("AdminCommand", () => {
         );
         await command.handler({
             matched: () => {},
-            completed: () => {},
         } as any);
         expect(wasCalledTimes).to.equal(1);
     });
@@ -81,7 +80,6 @@ describe("AdminCommand", () => {
             },
         );
         await command.handler({
-            completed: () => {},
             matched: () => {},
             respond: respondMock,
         } as any);