Fix allowing multiple puppets on the same team for the same user (#705)

* Add new schema to prevent multiple puppets on the same team

* Test

* Fix tests

* Notify users of account violations

* Notice

* Fix NEDB test

* Check error message in test

* Cleanup schema

* Fix type bug

* spell sql out

* fix sql

changelog.d/705.bugfix

@@ -0,0 +1,3 @@
+Disallowed the accidental behaviour of puppeting two or more Slack accounts from the same team to the same Matrix user. This would cause all puppeting for the
+user to stop working. The bridge will now **automatically delete** any puppeted connections which violate this rule. You will be notified via the bridge when
+this happens, so you can relink your accounts.

src/Main.ts

@@ -982,12 +982,20 @@ export class Main {
 
         await UserAdminRoom.compileTemplates();
 
-        const dbEngine = this.config.db ? this.config.db.engine.toLowerCase() : "nedb";
-        if (dbEngine === "postgres") {
+        if (this.datastore instanceof PgDatastore) {
             // We create this in the constructor because we need it for encryption
             // support.
-            await (this.datastore as PgDatastore).ensureSchema();
-        } else if (dbEngine === "nedb") {
+            const userMessages = await this.datastore.ensureSchema();
+            for (const message of userMessages) {
+                let roomId = await this.datastore.getUserAdminRoom(message.matrixId);
+                if (!roomId) {
+                    // Unexpected, they somehow set up a puppet without creating an admin room.
+                    roomId = (await UserAdminRoom.inviteAndCreateAdminRoom(message.matrixId, this)).roomId;
+                }
+                log.info(`Sending one-time notice from schema to ${message.matrixId} (${roomId})`);
+                await this.botIntent.sendText(roomId, message.message);
+            }
+        } else if (!this.config.db || this.config.db.engine === "nedb") {
             await this.bridge.loadDatabases();
             log.info("Loading teams.db");
             // eslint-disable-next-line @typescript-eslint/no-var-requires

src/datastore/postgres/PgDatastore.ts

@@ -48,8 +48,15 @@ interface ClientSessionSchema {
     sync_token?: string;
 }
 
+export interface SchemaRunUserMessage {
+    matrixId: string,
+    message: string
+}
+
+type SchemaRunFn = (db: IDatabase<unknown>) => Promise<void|{userMessages: SchemaRunUserMessage[]}>;
+
 export class PgDatastore implements Datastore, ClientEncryptionStore {
-    public static readonly LATEST_SCHEMA = 14;
+    public static readonly LATEST_SCHEMA = 15;
     public readonly postgresDb: IDatabase<any>;
 
     constructor(connectionString: string) {
@@ -246,14 +253,18 @@ export class PgDatastore implements Datastore, ClientEncryptionStore {
         );
     }
 
-    public async ensureSchema(): Promise<void> {
+    public async ensureSchema(): Promise<SchemaRunUserMessage[]> {
+        const userMessages: SchemaRunUserMessage[] = [];
         let currentVersion = await this.getSchemaVersion();
         while (currentVersion < PgDatastore.LATEST_SCHEMA) {
             log.info(`Updating schema to v${currentVersion + 1}`);
             // eslint-disable-next-line @typescript-eslint/no-var-requires
-            const runSchema = require(`./schema/v${currentVersion + 1}`).runSchema;
+            const runSchema: SchemaRunFn = require(`./schema/v${currentVersion + 1}`).runSchema;
             try {
-                await runSchema(this.postgresDb);
+                const result = await runSchema(this.postgresDb);
+                if (result?.userMessages) {
+                    userMessages.push(...result.userMessages);
+                }
                 currentVersion++;
                 await this.updateSchemaVersion(currentVersion);
             } catch (ex) {
@@ -262,6 +273,7 @@ export class PgDatastore implements Datastore, ClientEncryptionStore {
             }
         }
         log.info(`Database schema is at version v${currentVersion}`);
+        return userMessages;
     }
 
     public async upsertRoom(room: BridgedRoom): Promise<null> {

src/datastore/postgres/schema/v15.ts

@@ -0,0 +1,32 @@
+import { IDatabase } from "pg-promise";
+
+import { SchemaRunUserMessage } from "../PgDatastore";
+
+export const runSchema = async(db: IDatabase<unknown>): Promise<{userMessages: SchemaRunUserMessage[]}> => {
+    // Disallow multiple puppets for the same team for the same user.
+    const cases =(await db.manyOrNone(
+        `SELECT matrixuser, name FROM (
+            SELECT slackteam, matrixuser FROM puppets
+            GROUP BY slackteam, matrixuser HAVING COUNT(*) > 1
+        ) entry
+        LEFT JOIN teams ON entry.slackteam = teams.id;`)).map((puppet => ({
+        matrixId: puppet.matrixuser,
+        // It's possible the team name might not exist (rarely), so just hide it.
+        teamName: puppet.name ? ` (${puppet.name})` : '',
+    })));
+    // Delete any cases where this has happened
+    await db.none(`
+        DELETE FROM puppets puppetsOuter USING (
+            SELECT matrixuser, slackteam FROM puppets GROUP BY slackteam, matrixuser HAVING COUNT(*) > 1
+        ) puppetsInner WHERE puppetsOuter.matrixuser = puppetsInner.matrixuser AND puppetsOuter.slackteam = puppetsInner.slackteam;
+        ALTER TABLE puppets ADD CONSTRAINT puppets_slackteam_matrixuser_unique UNIQUE(slackteam, matrixuser);`);
+    return {
+        userMessages: cases.map(u => ({
+            matrixId: u.matrixId,
+            message: `Hello. Your Matrix account was puppeted to two or more Slack accounts from the same team${u.teamName}, which` +
+            ` is not valid. As a precaution, the bridge has unlinked all Slack accounts where two or more were present from the same team.` +
+            ` You can use the \`whoami\` command to find out if any accounts are still linked. To relink your accounts, simply run \`login\`.
+            `
+        })),
+    };
+};

src/rooms/UserAdminRoom.ts

@@ -39,7 +39,7 @@ export class UserAdminRoom {
         return adminRoom;
     }
 
-    constructor(private roomId: string, private userId: string, private main: Main) {
+    constructor(public readonly roomId: string, private userId: string, private main: Main) {
 
     }
 

tests/integration/PgDatastoreTest.ts

@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Matrix.org Foundation C.I.C.
+Copyright 2019,2022 The Matrix.org Foundation C.I.C.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -131,6 +131,28 @@ describeFnl("PgDatastore", () => {
         });
     });
 
+    describe("puppets", () => {
+        afterEach(async () => {
+            await ds.postgresDb.none("DELETE FROM puppets");
+        });
+        it("should allow two puppets on different teams for the same user", async () => {
+            await ds.setPuppetToken("MY_TEAM_1", "MY_SLACK_USER_1", "@myuser:id", "MY_TOKEN");
+            await ds.setPuppetToken("MY_TEAM_2", "MY_SLACK_USER_2", "@myuser:id", "DIFF_TOKEN");
+        });
+        it("should not allow two puppets on the same team for the same user", async () => {
+            await ds.setPuppetToken("MY_TEAM_1", "MY_SLACK_USER_1", "@myuser:id", "MY_TOKEN");
+            // Allow a different matrix user to have someone on the same team.
+            await ds.setPuppetToken("MY_TEAM_1", "MY_SLACK_USER_2", "@diff_user:id", "DIFF_TOKEN");
+            try {
+                await ds.setPuppetToken("MY_TEAM_1", "MY_SLACK_USER_2", "@myuser:id", "DIFF_TOKEN_2");
+            } catch (ex) {
+                expect((ex as Error).message).to.equal('duplicate key value violates unique constraint "puppets_slackuser_key"');
+                return;
+            }
+            throw Error('Expected to fail');
+        });
+    });
+
     after(async () => {
         Logging.configure({console: "off"});
     });