fix config.protectAllJoinedRooms leaking into explicitly protected rooms

package.json

@@ -36,6 +36,7 @@
     "typescript-formatter": "^7.2"
   },
   "dependencies": {
+    "await-lock": "^2.2.2",
     "express": "^4.17",
     "html-to-text": "^8.0.0",
     "humanize-duration": "^3.27.1",

src/ProtectedRoomsConfig.ts

@@ -0,0 +1,129 @@
+/*
+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.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import AwaitLock from 'await-lock';
+import { extractRequestError, LogService, MatrixClient, Permalinks } from "matrix-bot-sdk";
+import { IConfig } from "./config";
+const PROTECTED_ROOMS_EVENT_TYPE = "org.matrix.mjolnir.protected_rooms";
+
+/**
+ * Manages the set of rooms that the user has EXPLICITLY asked to be protected.
+ */
+export default class ProtectedRoomsConfig {
+
+    /**
+     * These are rooms that we EXPLICITLY asked Mjolnir to protect, usually via the `rooms add` command.
+     * These are NOT all of the rooms that mjolnir is protecting as with `config.protectAllJoinedRooms`.
+     */
+    private explicitlyProtectedRooms = new Set</*room id*/string>();
+    /** This is to prevent clobbering the account data for the protected rooms if several rooms are explicitly protected concurrently. */
+    private accountDataLock = new AwaitLock();
+
+    constructor(private readonly client: MatrixClient) {
+
+    }
+
+    /**
+     * Load any rooms that have been explicitly protected from a Mjolnir config.
+     * Will also ensure we are able to join all of the rooms.
+     * @param config The config to load the rooms from under `config.protectedRooms`.
+     */
+    public async loadProtectedRoomsFromConfig(config: IConfig): Promise<void> {
+        // Ensure we're also joined to the rooms we're protecting
+        LogService.info("ProtectedRoomsConfig", "Resolving protected rooms...");
+        const joinedRooms = await this.client.getJoinedRooms();
+        for (const roomRef of config.protectedRooms) {
+            const permalink = Permalinks.parseUrl(roomRef);
+            if (!permalink.roomIdOrAlias) continue;
+
+            let roomId = await this.client.resolveRoom(permalink.roomIdOrAlias);
+            if (!joinedRooms.includes(roomId)) {
+                roomId = await this.client.joinRoom(permalink.roomIdOrAlias, permalink.viaServers);
+            }
+            this.explicitlyProtectedRooms.add(roomId);
+        }
+    }
+
+    /**
+     * Load any rooms that have been explicitly protected from the account data of the mjolnir user.
+     * Will not ensure we can join all the rooms. This so mjolnir can continue to operate if bogus rooms have been persisted to the account data.
+     */
+    public async loadProtectedRoomsFromAccountData(): Promise<void> {
+        LogService.debug("ProtectedRoomsConfig", "Loading protected rooms...");
+        try {
+            const data: { rooms?: string[] } | null = await this.client.getAccountData(PROTECTED_ROOMS_EVENT_TYPE);
+            if (data && data['rooms']) {
+                for (const roomId of data['rooms']) {
+                    this.explicitlyProtectedRooms.add(roomId);
+                }
+            }
+        } catch (e) {
+            if (e.statusCode === 404) {
+                LogService.warn("ProtectedRoomsConfig", "Couldn't find any explicitly protected rooms from Mjolnir's account data, assuming first start.", extractRequestError(e));
+            } else {
+                throw e;
+            }
+        }
+    }
+
+    /**
+     * Save the room as explicitly protected.
+     * @param roomId The room to persist as explicitly protected.
+     */
+    public async addProtectedRoom(roomId: string): Promise<void> {
+        this.explicitlyProtectedRooms.add(roomId);
+        await this.saveProtectedRoomsToAccountData();
+    }
+
+    /**
+     * Remove the room from the explicitly protected set of rooms.
+     * @param roomId The room that should no longer be persisted as protected.
+     */
+    public async removeProtectedRoom(roomId: string): Promise<void> {
+        this.explicitlyProtectedRooms.delete(roomId);
+        await this.saveProtectedRoomsToAccountData([roomId]);
+    }
+
+    /**
+     * Get the set of explicitly protected rooms.
+     * This will NOT be the complete set of protected rooms, if `config.protectAllJoinedRooms` is true and should never be treated as the complete set.
+     * @returns The rooms that are marked as explicitly protected in both the config and Mjolnir's account data.
+     */
+    public getExplicitlyProtectedRooms(): string[] {
+        return [...this.explicitlyProtectedRooms.keys()]
+    }
+
+    /**
+     * Persist the set of explicitly protected rooms to the client's account data.
+     * @param excludeRooms Rooms that should not be persisted to the account data, and removed if already present.
+     */
+    private async saveProtectedRoomsToAccountData(excludeRooms: string[] = []): Promise<void> {
+        // NOTE: this stops Mjolnir from racing with itself when saving the config
+        //       but it doesn't stop a third party client on the same account racing with us instead.
+        await this.accountDataLock.acquireAsync();
+        try {
+            const additionalProtectedRooms: string[] = await this.client.getAccountData(PROTECTED_ROOMS_EVENT_TYPE)
+                .then((rooms: {rooms?: string[]}) => Array.isArray(rooms?.rooms) ? rooms.rooms : [])
+                .catch(e => (LogService.warn("ProtectedRoomsConfig", "Could not load protected rooms from account data", extractRequestError(e)), []));
+
+            const roomsToSave = new Set([...this.explicitlyProtectedRooms.keys(), ...additionalProtectedRooms]);
+            excludeRooms.forEach(roomsToSave.delete, roomsToSave);
+            await this.client.setAccountData(PROTECTED_ROOMS_EVENT_TYPE, { rooms: Array.from(roomsToSave.keys()) });
+        } finally {
+            this.accountDataLock.release();
+        }
+    }
+}

src/ProtectedRooms.ts → src/ProtectedRoomsSet.ts

@@ -42,7 +42,7 @@ import { htmlEscape } from "./utils";
  * It is also important not to tie this to the one group of rooms that a mjolnir may watch
  * as in future we might want to borrow this class to represent a space https://github.com/matrix-org/mjolnir/issues/283.
  */
-export class ProtectedRooms {
+export class ProtectedRoomsSet {
 
     private protectedRooms = new Set</* room id */string>();
 
@@ -173,6 +173,9 @@ export class ProtectedRooms {
         if (event['sender'] === this.clientUserId) {
             throw new TypeError("`ProtectedRooms::handleEvent` should not be used to inform about events sent by mjolnir.");
         }
+        if (!this.protectedRooms.has(roomId)) {
+            return; // We're not protecting this room.
+        }
         this.protectedRoomActivityTracker.handleEvent(roomId, event);
         if (event['type'] === 'm.room.power_levels' && event['state_key'] === '') {
             // power levels were updated - recheck permissions
@@ -228,15 +231,14 @@ export class ProtectedRooms {
         }
     }
 
-    public async addProtectedRoom(roomId: string): Promise<void> {
+    public addProtectedRoom(roomId: string): void {
         if (this.protectedRooms.has(roomId)) {
             // we need to protect ourselves form syncing all the lists unnecessarily
             // as Mjolnir does call this method repeatedly.
             return;
         }
         this.protectedRooms.add(roomId);
         this.protectedRoomActivityTracker.addProtectedRoom(roomId);
-        await this.syncLists(this.config.verboseLogging);
     }
 
     public removeProtectedRoom(roomId: string): void {

src/commands/CreateBanListCommand.ts

@@ -32,6 +32,7 @@ export async function execCreateListCommand(roomId: string, event: any, mjolnir:
 
     const roomRef = Permalinks.forRoom(listRoomId);
     await mjolnir.watchList(roomRef);
+    await mjolnir.addProtectedRoom(listRoomId);
 
     const html = `Created new list (<a href="${roomRef}">${listRoomId}</a>). This list is now being watched.`;
     const text = `Created new list (${roomRef}). This list is now being watched.`;

src/commands/ListProtectedRoomsCommand.ts

@@ -24,7 +24,7 @@ export async function execListProtectedRooms(roomId: string, event: any, mjolnir
     let text = `Protected rooms (${rooms.length}):\n`;
 
     let hasRooms = false;
-    for (const protectedRoomId in rooms) {
+    for (const protectedRoomId of rooms) {
         hasRooms = true;
 
         const roomUrl = Permalinks.forRoom(protectedRoomId);

src/commands/StatusCommand.ts

@@ -18,6 +18,7 @@ import { Mjolnir, STATE_CHECKING_PERMISSIONS, STATE_NOT_STARTED, STATE_RUNNING,
 import { RichReply } from "matrix-bot-sdk";
 import { htmlEscape, parseDuration } from "../utils";
 import { HumanizeDurationLanguage, HumanizeDuration } from "humanize-duration-ts";
+import PolicyList from "../models/PolicyList";
 
 const HUMANIZE_LAG_SERVICE: HumanizeDurationLanguage = new HumanizeDurationLanguage();
 const HUMANIZER: HumanizeDuration = new HumanizeDuration(HUMANIZE_LAG_SERVICE);
@@ -67,22 +68,28 @@ async function showMjolnirStatus(roomId: string, event: any, mjolnir: Mjolnir) {
             break;
     }
 
-    html += `<b>Protected rooms: </b> ${Object.keys(mjolnir.protectedRooms).length}<br/>`;
-    text += `Protected rooms: ${Object.keys(mjolnir.protectedRooms).length}\n`;
+    html += `<b>Protected rooms: </b> ${mjolnir.protectedRoomsTracker.getProtectedRooms().length}<br/>`;
+    text += `Protected rooms: ${mjolnir.protectedRoomsTracker.getProtectedRooms().length}\n`;
 
     // Append list information
-    html += "<b>Subscribed ban lists:</b><br><ul>";
-    text += "Subscribed ban lists:\n";
-    for (const list of mjolnir.lists) {
-        const ruleInfo = `rules: ${list.serverRules.length} servers, ${list.userRules.length} users, ${list.roomRules.length} rooms`;
-        html += `<li>${htmlEscape(list.listShortcode)} @ <a href="${list.roomRef}">${list.roomId}</a> (${ruleInfo})</li>`;
-        text += `* ${list.listShortcode} @ ${list.roomRef} (${ruleInfo})\n`;
-    }
-    if (mjolnir.lists.length === 0) {
-        html += "<li><i>None</i></li>";
-        text += "* None\n";
+    const renderPolicyLists = (header: string, lists: PolicyList[]) => {
+        html += `<b>${header}:</b><br><ul>`;
+        text += `${header}:\n`;
+        for (const list of lists) {
+            const ruleInfo = `rules: ${list.serverRules.length} servers, ${list.userRules.length} users, ${list.roomRules.length} rooms`;
+            html += `<li>${htmlEscape(list.listShortcode)} @ <a href="${list.roomRef}">${list.roomId}</a> (${ruleInfo})</li>`;
+            text += `* ${list.listShortcode} @ ${list.roomRef} (${ruleInfo})\n`;
+        }
+        if (lists.length === 0) {
+            html += "<li><i>None</i></li>";
+            text += "* None\n";
+        }
+        html += "</ul>";
     }
-    html += "</ul>";
+    const subscribedLists = mjolnir.lists.filter(list => !mjolnir.explicitlyProtectedRooms.includes(list.roomId));
+    renderPolicyLists("Subscribed policy lists", subscribedLists);
+    const subscribedAndProtectedLists = mjolnir.lists.filter(list => mjolnir.explicitlyProtectedRooms.includes(list.roomId));
+    renderPolicyLists("Subscribed and protected policy lists", subscribedAndProtectedLists);
 
     const reply = RichReply.createFor(roomId, event, text, html);
     reply["msgtype"] = "m.notice";

test/integration/protectedRoomsConfigTest.ts

@@ -0,0 +1,64 @@
+
+import { strict as assert } from "assert";
+import { MatrixClient, Permalinks, UserID } from "matrix-bot-sdk";
+import { Mjolnir } from "../../src/Mjolnir";
+import PolicyList from "../../src/models/PolicyList";
+import { newTestUser } from "./clientHelper";
+import { createBanList, getFirstReaction } from "./commands/commandUtils";
+
+async function createPolicyList(client: MatrixClient): Promise<PolicyList> {
+    const serverName = new UserID(await client.getUserId()).domain;
+    const policyListId = await client.createRoom({ preset: "public_chat" });
+    return new PolicyList(policyListId, Permalinks.forRoom(policyListId), client);
+}
+
+async function getProtectedRoomsFromAccountData(client: MatrixClient): Promise<string[]> {
+    const rooms: { rooms?: string[] } = await client.getAccountData("org.matrix.mjolnir.protected_rooms");
+    return rooms.rooms!;
+}
+
+describe('Test: config.protectAllJoinedRooms behaves correctly.', function() {
+    it('does not clobber the account data.', async function() {
+        // set up account data for a protected room with your own list and a watched list.
+        const mjolnir: Mjolnir = this.mjolnir!;
+
+        // moderator sets up some rooms, that aren't explicitly protected
+        const moderator = await newTestUser(this.config.homeserverUrl, { name: { contains: "moderator" } });
+        await moderator.joinRoom(mjolnir.managementRoomId);
+        const implicitlyProtectedRooms = await Promise.all(
+            [...Array(2).keys()].map(_ => moderator.createRoom({ preset: "public_chat" }))
+        );
+        await Promise.all(
+            implicitlyProtectedRooms.map(roomId => mjolnir.client.joinRoom(roomId))
+        );
+
+        // we sync and check that none of them end up in account data
+        await mjolnir.protectedRoomsTracker.syncLists();
+        (await getProtectedRoomsFromAccountData(mjolnir.client))
+            .forEach(roomId => assert.equal(implicitlyProtectedRooms.includes(roomId), false));
+
+        // ... but they are protected
+        mjolnir.protectedRoomsTracker.getProtectedRooms()
+            .forEach(roomId => assert.equal(implicitlyProtectedRooms.includes(roomId), true));
+
+        // We create one policy list with Mjolnir, and we watch another that is maintained by someone else.
+        const policyListShortcode = await createBanList(mjolnir.managementRoomId, mjolnir.client, moderator);
+        const unprotectedWatchedList = await createPolicyList(moderator);
+        await mjolnir.watchList(unprotectedWatchedList.roomRef);
+        await mjolnir.protectedRoomsTracker.syncLists();
+
+        // We expect that the watched list will not be protected, despite config.protectAllJoinedRooms being true
+        // this is necessary so that it doesn't try change acl, ban users etc in someone else's list.
+        assert.equal(mjolnir.protectedRoomsTracker.getProtectedRooms().includes(unprotectedWatchedList.roomId), false);
+        const accountDataAfterListSetup = await getProtectedRoomsFromAccountData(mjolnir.client);
+        assert.equal(accountDataAfterListSetup.includes(unprotectedWatchedList.roomId), false);
+        // But our own list should be protected AND stored in account data
+        assert.equal(accountDataAfterListSetup.length, 1);
+        const policyListId = accountDataAfterListSetup[0];
+        assert.equal(mjolnir.protectedRoomsTracker.getProtectedRooms().includes(policyListId), true);
+        // Confirm that it is the right room, since we only get the shortcode back when using the command to create a list.
+        const shortcodeInfo = await mjolnir.client.getRoomStateEvent(policyListId, "org.matrix.mjolnir.shortcode", "");
+        assert.equal(shortcodeInfo.shortcode, policyListShortcode);
+    })
+});
+