Rework kick command for ACL clean+glob kick.

config/harness.yaml

@@ -96,6 +96,8 @@ protectedRooms: []
 # Manually add these rooms to the protected rooms list if you want them protected.
 protectAllJoinedRooms: false
 
+backgroundDelayMS: 10
+
 # Server administration commands, these commands will only work if Mjolnir is
 # a global server administrator
 admin:

src/commands/CommandHandler.ts

@@ -36,7 +36,7 @@ import { execAddRoomToDirectoryCommand, execRemoveRoomFromDirectoryCommand } fro
 import { execSetPowerLevelCommand } from "./SetPowerLevelCommand";
 import { execShutdownRoomCommand } from "./ShutdownRoomCommand";
 import { execAddAliasCommand, execMoveAliasCommand, execRemoveAliasCommand, execResolveCommand } from "./AliasCommands";
-import { execKickCommand } from "./KickCommand";
+import { execKickCommand, execServerAclCleanCommand } from "./KickCommand";
 import { execMakeRoomAdminCommand } from "./MakeRoomAdminCommand";
 import { parse as tokenize } from "shell-quote";
 import { execSinceCommand } from "./SinceCommand";
@@ -119,6 +119,8 @@ export async function handleCommand(roomId: string, event: { content: { body: st
             return await execSinceCommand(roomId, event, mjolnir, tokens);
         } else if (parts[1] === 'kick' && parts.length > 2) {
             return await execKickCommand(roomId, event, mjolnir, parts);
+        } else if (parts[1] === 'acl-clean') {
+            return await execServerAclCleanCommand(roomId, event, mjolnir, parts);
         } else if (parts[1] === 'make' && parts[2] === 'admin' && parts.length > 3) {
             return await execMakeRoomAdminCommand(roomId, event, mjolnir, parts);
         } else {
@@ -132,6 +134,7 @@ export async function handleCommand(roomId: string, event: { content: { body: st
                 "!mjolnir redact <user ID> [room alias/ID] [limit]                   - Redacts messages by the sender in the target room (or all rooms), up to a maximum number of events in the backlog (default 1000)\n" +
                 "!mjolnir redact <event permalink>                                   - Redacts a message by permalink\n" +
                 "!mjolnir kick <glob> [room alias/ID] [reason]                    - Kicks a user or all of those matching a glob in a particular room or all protected rooms\n" +
+                "!mjolnir acl-clean [room alias/ID]                                  - Kicks all of the users from the room or all protected rooms that whose servers are banned by the room's server ACL.\n" +
                 "!mjolnir rules                                                      - Lists the rules currently in use by Mjolnir\n" +
                 "!mjolnir sync                                                       - Force updates of all lists and re-apply rules\n" +
                 "!mjolnir verify                                                     - Ensures Mjolnir can moderate all your rooms\n" +

src/commands/KickCommand.ts

@@ -15,65 +15,156 @@ limitations under the License.
 */
 
 import { Mjolnir } from "../Mjolnir";
-import { LogLevel, MatrixGlob, RichReply } from "matrix-bot-sdk";
+import { LogLevel, MatrixGlob, MembershipEvent, RichReply, UserID } from "matrix-bot-sdk";
 import config from "../config";
+import { ServerAcl } from "../models/ServerAcl";
 
 // !mjolnir kick <user|filter> [room] [reason]
-export async function execKickCommand(roomId: string, event: any, mjolnir: Mjolnir, parts: string[]) {
-    let force = false;
-
+export async function execKickCommand(commandRoomId: string, event: any, mjolnir: Mjolnir, parts: string[]) {
+    const force = parts[parts.length - 1] === "--force";
     const glob = parts[2];
-    let rooms = [...Object.keys(mjolnir.protectedRooms)];
+    const kickRule = new MatrixGlob(glob);
+    const kickRuleHasGlob = /[*?]/.test(glob);
+    const rooms = await (async () => {
+        // if they provide a room then use that, otherwise use all protected rooms.
+        if (parts.length > 3) {
+            if (parts[3].startsWith("#") || parts[3].startsWith("!")) {
+                return [await mjolnir.client.resolveRoom(parts[3])];
+            }
+        }
+        return [...Object.keys(mjolnir.protectedRooms)];
+    })();
+    const reason = (rooms.length === 1 ?
+            // we don't forget to remove the `--force` argument from the reason.
+            parts.slice(4, force ? -1 : undefined).join(' ') :
+            parts.slice(3, force ? -1 : undefined).join(' ')
+        )
+        || '<no reason supplied>';
 
-    if (parts[parts.length - 1] === "--force") {
-        force = true;
-        parts.pop();
+    for (const protectedRoomId of rooms) {
+        const membersToKick = await filterMembers(
+            mjolnir,
+            protectedRoomId,
+            membership => kickRule.test(membership.membershipFor) ? KickOutcome.Remove : KickOutcome.Keep
+        );
+        if (kickRuleHasGlob && (!config.commands.confirmWildcardBan || !force)) {
+            let replyMessage = `The wildcard command would have removed ${membersToKick.length} ${membersToKick.length === 1 ? 'member' : 'members'} from ${protectedRoomId}`;
+            replyMessage += "Wildcard bans need to be explicitly enabled in the config and require an addition `--force` argument to confirm";
+            const reply = RichReply.createFor(commandRoomId, event, replyMessage, replyMessage);
+            reply["msgtype"] = "m.notice";
+            await mjolnir.client.sendMessage(commandRoomId, reply);
+            // We don't want to even tell them who is being kicked if it hasn't been enabled.
+            if (!config.commands.confirmWildcardBan) {
+                return;
+            }
+        }
+        await kickMembers(mjolnir, protectedRoomId, membersToKick, force, reason);
     }
 
-    if (config.commands.confirmWildcardBan && /[*?]/.test(glob) && !force) {
-        let replyMessage = "Wildcard bans require an addition `--force` argument to confirm";
-        const reply = RichReply.createFor(roomId, event, replyMessage, replyMessage);
-        reply["msgtype"] = "m.notice";
-        await mjolnir.client.sendMessage(roomId, reply);
-        return;
-    }
+    return mjolnir.client.unstableApis.addReactionToEvent(commandRoomId, event['event_id'], '✅');
+}
 
-    const kickRule = new MatrixGlob(glob);
+/**
+ * A command to remove users whose server is banned by server ACL from a room.
+ * @param commandRoomId The room the command was sent from.
+ * @param event The event containing the command.
+ * @param mjolnir A mjolnir instance.
+ * @param parts The parts of the command.
+ * @returns When the users have been removed and the command has been marked as complete.
+ */
+export async function execServerAclCleanCommand(commandRoomId: string, event: any, mjolnir: Mjolnir, parts: string[]) {
+    const force = parts[parts.length - 1] === "--force";
+    const serverName: string = new UserID(await mjolnir.client.getUserId()).domain;
+    // If they say all, clean all protected rooms, otherwise they gave a room id/alias/pill.
+    const roomsToClean = parts[2] === 'all' ? [...Object.keys(mjolnir.protectedRooms)] : [await mjolnir.client.resolveRoom(parts[2])]
+    for (const roomToClean of roomsToClean) {
+        const currentAcl = new ServerAcl(serverName).fromACL(await mjolnir.client.getRoomStateEvent(roomToClean, "m.room.server_acl", ""));
+        const membersToKick = await filterMembers(
+            mjolnir,
+            roomToClean,
+            membership => {
+                const memberId = new UserID(membership.membershipFor);
+                // check the user's server isn't on the deny list.
+                for (const deny of currentAcl.safeAclContent().deny) {
+                    const rule = new MatrixGlob(deny);
+                    if (rule.test(memberId.domain)) {
+                        return KickOutcome.Remove;
+                    }
+                }
+                // check the user's server is allowed.
+                for (const allow of currentAcl.safeAclContent().allow) {
+                    const rule = new MatrixGlob(allow);
+                    if (rule.test(memberId.domain)) {
+                        return KickOutcome.Keep;
+                    }
+                }
+                // if they got here it means their server was not explicitly allowed.
+                return KickOutcome.Remove;
+            }
+        );
 
-    let reason: string | undefined;
-    if (parts.length > 3) {
-        let reasonIndex = 3;
-        if (parts[3].startsWith("#") || parts[3].startsWith("!")) {
-            rooms = [await mjolnir.client.resolveRoom(parts[3])];
-            reasonIndex = 4;
+        /// Instead of having --force on commands like these were confirmation is required after some context,
+        /// wouldn't it be better if we showed what would happen and then ask yes/no to confirm?
+        if (!force) {
+            let replyMessage = `The ACL clean command would have removed ${membersToKick.length} ${membersToKick.length === 1 ? 'member' : 'members'} from ${roomToClean}`;
+            replyMessage += "The ACL clean command needs an additional `--force` argument to confirm";
+            const reply = RichReply.createFor(commandRoomId, event, replyMessage, replyMessage);
+            reply["msgtype"] = "m.notice";
+            await mjolnir.client.sendMessage(commandRoomId, reply);
         }
-        reason = parts.slice(reasonIndex).join(' ') || '<no reason supplied>';
+        await kickMembers(mjolnir, roomToClean, membersToKick, force, "User's server is banned by the room's server ACL event.")
     }
-    if (!reason) reason = '<none supplied>';
-
-    for (const protectedRoomId of rooms) {
-        const members = await mjolnir.client.getRoomMembers(protectedRoomId, undefined, ["join"], ["ban", "leave"]);
+    return mjolnir.client.unstableApis.addReactionToEvent(commandRoomId, event['event_id'], '✅');
+}
 
-        for (const member of members) {
-            const victim = member.membershipFor;
+/**
+ * Filter room members using a user specified predicate.
+ * @param mjolnir Mjolnir instance to fetch room members with.
+ * @param roomId The room to fetch members from.
+ * @param predicate A function accepting a membership event's content and returns a `KickOutcome`. See `MembershipEvent`.
+ * @returns A list of user ids who are members of the room who have been marked as `KickOutcome.Remove`.
+ */
+async function filterMembers(
+    mjolnir: Mjolnir,
+    roomId: string,
+    predicate: (member: MembershipEvent) => KickOutcome
+): Promise<string[]> {
+    const members = await mjolnir.client.getRoomMembers(roomId, undefined, ["join"], ["ban", "leave"]);
+    const filteredMembers = [];
+    for (const member of members) {
+        if (predicate(member) === KickOutcome.Remove) {
+            filteredMembers.push(member.membershipFor);
+        }
+    }
+    return filteredMembers;
+}
 
-            if (kickRule.test(victim)) {
-                await mjolnir.logMessage(LogLevel.DEBUG, "KickCommand", `Removing ${victim} in ${protectedRoomId}`, protectedRoomId);
+/**
+ * Whether to remove a user from a room or not.
+ */
+enum KickOutcome {
+    Remove,
+    Keep,
+}
 
-                if (!config.noop) {
-                    try {
-                        await mjolnir.taskQueue.push(async () => {
-                            return mjolnir.client.kickUser(victim, protectedRoomId, reason);
-                        });
-                    } catch (e) {
-                        await mjolnir.logMessage(LogLevel.WARN, "KickCommand", `An error happened while trying to kick ${victim}: ${e}`);
-                    }
-                } else {
-                    await mjolnir.logMessage(LogLevel.WARN, "KickCommand", `Tried to kick ${victim} in ${protectedRoomId} but the bot is running in no-op mode.`, protectedRoomId);
-                }
+async function kickMembers(mjolnir: Mjolnir, roomId: string, membersToKick: string[], force: boolean, reason: string) {
+    // I do not think it makes much sense to notify who was kicked like this.
+    // It should really be reconsidered with https://github.com/matrix-org/mjolnir/issues/294
+    // and whether we want to produce reports or something like that.
+    for (const member of membersToKick) {
+        if (config.noop) {
+            await mjolnir.logMessage(LogLevel.WARN, "KickCommand", `Tried to kick ${member} in ${roomId} but the bot is running in no-op mode.`);
+        } else if (!force) {
+            await mjolnir.logMessage(LogLevel.DEBUG, "KickCommand", `Would have kicked ${member} in ${roomId} but --force was not given with the command.`);
+        } else {
+            await mjolnir.logMessage(LogLevel.DEBUG, "KickCommand", `Removing ${member} in ${roomId}`);
+            try {
+                await mjolnir.taskQueue.push(async () => {
+                    return mjolnir.client.kickUser(member, roomId, reason);
+                });
+            } catch (e) {
+                await mjolnir.logMessage(LogLevel.WARN, "KickCommand", `An error happened while trying to kick ${member}: ${e}`);
             }
         }
     }
-
-    return mjolnir.client.unstableApis.addReactionToEvent(roomId, event['event_id'], '✅');
 }

src/models/ServerAcl.ts

@@ -32,6 +32,30 @@ export class ServerAcl {
 
     }
 
+    /**
+     * Initialize the ServerACL with the rules from an existing ACL object retrieved from the room state.
+     * @param acl The content of a `m.room.server_acl` event.
+     */
+    public fromACL(acl: any): ServerAcl {
+        if (this.allowedServers.size !== 0 || this.deniedServers.size !== 0) {
+            throw new TypeError(`Expected this ACL to be uninitialized.`);
+        }
+        const allow = acl['allow'];
+        const deny = acl['deny'];
+        const ips = acl['allow_ip_literals'];
+
+        if (Array.isArray(allow)) {
+            allow.forEach(this.allowServer, this);
+        }
+        if (Array.isArray(deny)) {
+            deny.forEach(this.denyServer, this);
+        }
+        if (Boolean(ips)) {
+            this.allowIpAddresses();
+        }
+        return this;
+    }
+
     /**
      * Checks the ACL for any entries that might ban ourself.
      * @returns A list of deny entries that will not ban our own homeserver.

test/integration/commands/kickCommandTest.ts

@@ -0,0 +1,46 @@
+import { strict as assert } from "assert";
+import { newTestUser } from "../clientHelper";
+import { getFirstReaction } from "./commandUtils";
+import { Mjolnir } from "../../../src/Mjolnir";
+
+ describe("Test: The redaction command", function () {
+    // If a test has a timeout while awaitng on a promise then we never get given control back.
+    afterEach(function() { this.moderator?.stop(); });
+
+    it("Kicks users matching ACL", async function () {
+        // How tf
+    })
+
+    it("Kicks users matching a glob", async function () {
+        this.timeout(120000)
+        // create a bunch of users with a pattern in the name.
+        const usersToRemove = await Promise.all([...Array(20)].map(_ => newTestUser({ name: { contains: "remove-me"}})));
+        const usersToKeep = await Promise.all([...Array(20)].map(_ => newTestUser({ name: { contains: "keep-me"}})));
+        // FIXME: Does our kick command kick from all protected rooms or just one room???
+        const protectedRooms: string[] = [];
+        for (let i = 0; i < 10; i++) {
+            const room = await this.mjolnir.client.createRoom({ preset: "public_chat" });
+            await this.mjolnir!.addProtectedRoom(room);
+            protectedRooms.push(room);
+            await Promise.all([...usersToKeep, ...usersToRemove].map(client => {
+                return Promise.all(protectedRooms.map(r => client.joinRoom(r)));
+            }));
+        }
+        // issue the glob kick
+        await getFirstReaction(this.mjolnir.client, this.mjolnir.managementRoomId, '✅', async () => {
+            return await this.mjolnir.client.sendMessage(this.mjolnir.managementRoomId, { msgtype: 'm.text', body: `!mjolnir kick *remove-me* --force` });
+        });
+        // make sure no one else got kicked
+        await Promise.all(protectedRooms.map(async room => {
+            const mjolnir: Mjolnir = this.mjolnir!;
+            const members = await mjolnir.client.getJoinedRoomMembers(room);
+            await Promise.all(usersToKeep.map(async client => {
+                assert.equal(members.includes(await client.getUserId()), true);
+            }));
+            await Promise.all(usersToRemove.map(async client => {
+                assert.equal(members.includes(await client.getUserId()), false);
+            }));
+        }));
+    })
+
+});

tsconfig.json

@@ -22,6 +22,7 @@
         "./src/**/*",
         "./test/integration/manualLaunchScript.ts",
         "./test/integration/roomMembersTest.ts",
-        "./test/integration/banListTest.ts"
+        "./test/integration/banListTest.ts",
+        "./test/integration/commands/kickCommandTest.ts"
     ]
 }