From 332da15d0d523f427a2c5c5e14eac8b1460e587a Mon Sep 17 00:00:00 2001 From: Gnuxie <50846879+Gnuxie@users.noreply.github.com> Date: Mon, 21 Feb 2022 16:51:14 +0000 Subject: [PATCH] Remove old rules when unbanning entities from BanLists. (#227) * Remove old rules when unbanning entities from BanLists. Fixes #220 --- src/commands/UnbanBanCommand.ts | 5 +-- src/models/BanList.ts | 34 +++++++++++++++ test/integration/banListTest.ts | 76 ++++++++++++++++++++++++++++++++- 3 files changed, 109 insertions(+), 6 deletions(-) diff --git a/src/commands/UnbanBanCommand.ts b/src/commands/UnbanBanCommand.ts index 7cdf47d..cfac205 100644 --- a/src/commands/UnbanBanCommand.ts +++ b/src/commands/UnbanBanCommand.ts @@ -136,10 +136,7 @@ export async function execUnbanCommand(roomId: string, event: any, mjolnir: Mjol const bits = await parseArguments(roomId, event, mjolnir, parts); if (!bits) return; // error already handled - const ruleContent = {}; // empty == clear/unban - const stateKey = `rule:${bits.entity}`; - - await mjolnir.client.sendStateEvent(bits.list!.roomId, bits.ruleType!, stateKey, ruleContent); + await bits.list!.unbanEntity(bits.ruleType!, bits.entity); if (USER_RULE_TYPES.includes(bits.ruleType!) && bits.reason === 'true') { const rule = new MatrixGlob(bits.entity); diff --git a/src/models/BanList.ts b/src/models/BanList.ts index df6004a..3b0d0a4 100644 --- a/src/models/BanList.ts +++ b/src/models/BanList.ts @@ -182,6 +182,40 @@ class BanList extends EventEmitter { return [...this.serverRules, ...this.userRules, ...this.roomRules]; } + /** + * Remove all rules in the banList for this entity that have the same state key (as when we ban them) + * by searching for rules that have legacy state types. + * @param ruleType The normalized (most recent) type for this rule e.g. `RULE_USER`. + * @param entity The entity to unban from this list. + * @returns true if any rules were removed and the entity was unbanned, otherwise false because there were no rules. + */ + public async unbanEntity(ruleType: string, entity: string): Promise { + const stateKey = `rule:${entity}`; + let typesToCheck = [ruleType]; + switch (ruleType) { + case RULE_USER: + typesToCheck = USER_RULE_TYPES; + break; + case RULE_SERVER: + typesToCheck = SERVER_RULE_TYPES; + break; + case RULE_ROOM: + typesToCheck = ROOM_RULE_TYPES; + break; + } + // We can't cheat and check our state cache because we normalize the event types to the most recent version. + const typesToRemove = (await Promise.all( + typesToCheck.map(stateType => this.client.getRoomStateEvent(this.roomId, stateType, stateKey) + .then(_ => stateType) // We need the state type as getRoomState only returns the content, not the top level. + .catch(e => e.statusCode === 404 ? null : Promise.reject(e)))) + ).filter(e => e); // remove nulls. I don't know why TS still thinks there can be nulls after this?? + if (typesToRemove.length === 0) { + return false; + } + await Promise.all(typesToRemove.map(stateType => this.client.sendStateEvent(this.roomId, stateType!, stateKey, {}))); + return true; + } + /** * Synchronise the model with the room representing the ban list by reading the current state of the room * and updating the model to reflect the room. diff --git a/test/integration/banListTest.ts b/test/integration/banListTest.ts index 322c923..4a7110f 100644 --- a/test/integration/banListTest.ts +++ b/test/integration/banListTest.ts @@ -3,9 +3,9 @@ import { strict as assert } from "assert"; import config from "../../src/config"; import { newTestUser, noticeListener } from "./clientHelper"; import { LogService, MatrixClient, Permalinks, UserID } from "matrix-bot-sdk"; -import BanList, { ALL_RULE_TYPES, ChangeType, ListRuleChange, RULE_SERVER, RULE_USER } from "../../src/models/BanList"; +import BanList, { ALL_RULE_TYPES, ChangeType, ListRuleChange, RULE_SERVER, RULE_USER, SERVER_RULE_TYPES } from "../../src/models/BanList"; import { ServerAcl, ServerAclContent } from "../../src/models/ServerAcl"; -import { createBanList } from "./commands/commandUtils"; +import { createBanList, getFirstReaction } from "./commands/commandUtils"; import { getMessagesByUserIn } from "../../src/utils"; /** @@ -290,3 +290,75 @@ describe('Test: ACL updates will batch when rules are added in succession.', fun })); }) }) + +describe('Test: unbaning entities via the BanList.', function () { + afterEach(function() { this.moderator?.stop(); }); + it('Will remove rules that have legacy types', async function () { + this.timeout(20000) + const mjolnir = config.RUNTIME.client! + const serverName: string = new UserID(await mjolnir.getUserId()).domain + const moderator = await newTestUser({ name: { contains: "moderator" }}); + this.moderator = moderator; + moderator.joinRoom(this.mjolnir.managementRoomId); + const mjolnirId = await mjolnir.getUserId(); + + // We'll make 1 protected room to test ACLs in. + const protectedRoom = await moderator.createRoom({ invite: [mjolnirId]}); + await mjolnir.joinRoom(protectedRoom); + await moderator.setUserPowerLevel(mjolnirId, protectedRoom, 100); + await this.mjolnir!.addProtectedRoom(protectedRoom); + + // If a previous test hasn't cleaned up properly, these rooms will be populated by bogus ACLs at this point. + await this.mjolnir!.syncLists(); + const roomAcl = await mjolnir.getRoomStateEvent(protectedRoom, "m.room.server_acl", ""); + assert.equal(roomAcl?.deny?.length ?? 0, 0, 'There should be no entries in the deny ACL.'); + + // Create some legacy rules on a BanList. + const banListId = await moderator.createRoom({ invite: [mjolnirId] }); + await moderator.setUserPowerLevel(await mjolnir.getUserId(), banListId, 100); + await moderator.sendStateEvent(banListId, 'org.matrix.mjolnir.shortcode', '', { shortcode: "unban-test"}); + await mjolnir.joinRoom(banListId); + this.mjolnir!.watchList(Permalinks.forRoom(banListId)); + // we use this to compare changes. + const banList = new BanList(banListId, banListId, moderator); + // we need two because we need to test the case where an entity has all rule types in the list + // and another one that only has one (so that we would hit 404 while looking up state) + const olderBadServer = "old.evil.com" + const newerBadServer = "new.evil.com" + await Promise.all(SERVER_RULE_TYPES.map(type => createPolicyRule(moderator, banListId, type, olderBadServer, 'gregg rulz ok'))); + await createPolicyRule(moderator, banListId, RULE_SERVER, newerBadServer, 'this is bad sort it out.'); + // Wait for the ACL event to be applied to our protected room. + await this.mjolnir!.syncLists(); + + await banList.updateList(); + // rules are normalized, that's why there should only be 2. + assert.equal(banList.allRules.length, 2); + + // Check that we have setup our test properly and therefore evil.com is banned. + const acl = new ServerAcl(serverName).denyIpAddresses().allowServer("*").denyServer(olderBadServer).denyServer(newerBadServer); + const protectedAcl = await mjolnir.getRoomStateEvent(protectedRoom, "m.room.server_acl", ""); + if (!acl.matches(protectedAcl)) { + assert.fail(`Room ${protectedRoom} doesn't have the correct ACL: ${JSON.stringify(roomAcl, null, 2)}`); + } + + // Now unban the servers, we will go via the unban command for completeness sake. + try { + await moderator.start(); + for (const server of [olderBadServer, newerBadServer]) { + await getFirstReaction(moderator, this.mjolnir.managementRoomId, '✅', async () => { + return await moderator.sendMessage(this.mjolnir.managementRoomId, {msgtype: 'm.text', body: `!mjolnir unban unban-test server ${server}`}); + }); + } + } finally { + moderator.stop(); + } + + // Wait for mjolnir to sync protected rooms to update ACL. + await this.mjolnir!.syncLists(); + // Confirm that the server is unbanned. + await banList.updateList(); + assert.equal(banList.allRules.length, 0); + const aclAfter = await mjolnir.getRoomStateEvent(protectedRoom, "m.room.server_acl", ""); + assert.equal(aclAfter.deny.length, 0, 'Should be no servers denied anymore'); + }) +})