From 7cb7573c1e9d173bde61cb650ee65a39f64a8bf0 Mon Sep 17 00:00:00 2001 From: gnuxie Date: Tue, 28 Jun 2022 10:34:09 +0100 Subject: [PATCH 1/3] Rework kick command for ACL clean+glob kick. https://github.com/matrix-org/mjolnir/issues/279 This isn't going to be the end, i don't like it. --- config/harness.yaml | 2 + src/commands/KickCommand.ts | 179 ++++++++++++++----- src/models/ServerAcl.ts | 24 +++ test/integration/commands/kickCommandTest.ts | 46 +++++ tsconfig.json | 3 +- 5 files changed, 209 insertions(+), 45 deletions(-) create mode 100644 test/integration/commands/kickCommandTest.ts diff --git a/config/harness.yaml b/config/harness.yaml index e3c3a9db..7e4a83a4 100644 --- a/config/harness.yaml +++ b/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: diff --git a/src/commands/KickCommand.ts b/src/commands/KickCommand.ts index f841ef4a..95878161 100644 --- a/src/commands/KickCommand.ts +++ b/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 [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(' ') + ) + || ''; - 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(' ') || ''; + await kickMembers(mjolnir, roomToClean, membersToKick, force, "User's server is banned by the room's server ACL event.") } - if (!reason) reason = ''; - - 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 { + 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'], '✅'); } diff --git a/src/models/ServerAcl.ts b/src/models/ServerAcl.ts index f9d6eca8..7d12b906 100644 --- a/src/models/ServerAcl.ts +++ b/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. diff --git a/test/integration/commands/kickCommandTest.ts b/test/integration/commands/kickCommandTest.ts new file mode 100644 index 00000000..45fee7ee --- /dev/null +++ b/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); + })); + })); + }) + +}); diff --git a/tsconfig.json b/tsconfig.json index d4c654ff..649e0a51 100644 --- a/tsconfig.json +++ b/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" ] } From bf732494127e35e1b44b9af46b4535cee10d671f Mon Sep 17 00:00:00 2001 From: gnuxie Date: Fri, 1 Jul 2022 11:32:56 +0100 Subject: [PATCH 2/3] oops forgot the command handler --- src/commands/CommandHandler.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/commands/CommandHandler.ts b/src/commands/CommandHandler.ts index ea121676..1836608f 100644 --- a/src/commands/CommandHandler.ts +++ b/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 [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 - Redacts a message by permalink\n" + "!mjolnir kick [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" + From 03602b7f5821b7b00800bb36ff2a410b3716b524 Mon Sep 17 00:00:00 2001 From: gnuxie Date: Tue, 5 Jul 2022 12:29:52 +0100 Subject: [PATCH 3/3] improve doc --- config/harness.yaml | 5 +++++ src/commands/CommandHandler.ts | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/config/harness.yaml b/config/harness.yaml index 7e4a83a4..1ab9203f 100644 --- a/config/harness.yaml +++ b/config/harness.yaml @@ -96,6 +96,11 @@ protectedRooms: [] # Manually add these rooms to the protected rooms list if you want them protected. protectAllJoinedRooms: false +# Increase this delay to have Mjölnir wait longer between two consecutive backgrounded +# operations. The total duration of operations will be longer, but the homeserver won't +# be affected as much. Conversely, decrease this delay to have Mjölnir chain operations +# faster. The total duration of operations will generally be shorter, but the performance +# of the homeserver may be more impacted. backgroundDelayMS: 10 # Server administration commands, these commands will only work if Mjolnir is diff --git a/src/commands/CommandHandler.ts b/src/commands/CommandHandler.ts index 1836608f..6d13c3a9 100644 --- a/src/commands/CommandHandler.ts +++ b/src/commands/CommandHandler.ts @@ -134,7 +134,7 @@ export async function handleCommand(roomId: string, event: { content: { body: st "!mjolnir redact [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 - Redacts a message by permalink\n" + "!mjolnir kick [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 acl-clean [room alias/ID] - Kicks all of the users from a room or all protected rooms (when no arguments are given) whose servers are banned by the room's server ACL event.\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" +