From 9c47fc917acfbf20d108c61f5ada7140cc83b471 Mon Sep 17 00:00:00 2001 From: gnuxie Date: Fri, 12 Nov 2021 18:35:44 +0000 Subject: [PATCH 1/4] Provide notice showing how a BanList has changed after updating. Only shows changes to lists made by other accounts (than the one used by Mjolnir). Displays when rules are added, removed and modified by either replacing the state event or redacting them. --- src/Mjolnir.ts | 73 +++++++++++++++++++++----- src/models/BanList.ts | 119 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 177 insertions(+), 15 deletions(-) diff --git a/src/Mjolnir.ts b/src/Mjolnir.ts index bf6ce93..97afbc0 100644 --- a/src/Mjolnir.ts +++ b/src/Mjolnir.ts @@ -26,7 +26,7 @@ import { UserID } from "matrix-bot-sdk"; -import BanList, { ALL_RULE_TYPES } from "./models/BanList"; +import BanList, { ALL_RULE_TYPES, ListRuleChange, RULE_ROOM, RULE_SERVER, RULE_USER } from "./models/BanList"; import { applyServerAcls } from "./actions/ApplyAcl"; import { RoomUpdateError } from "./models/RoomUpdateError"; import { COMMAND_PREFIX, handleCommand } from "./commands/CommandHandler"; @@ -614,7 +614,8 @@ export class Mjolnir { */ public async syncLists(verbose = true) { for (const list of this.banLists) { - await list.updateList(); + const changes = await list.updateList(); + await this.printBanlistChanges(changes, list, true); } let hadErrors = false; @@ -638,17 +639,19 @@ export class Mjolnir { } } - public async syncListForRoom(roomId: string) { - let updated = false; - for (const list of this.banLists) { - if (list.roomId !== roomId) continue; - await list.updateList(); - updated = true; - } - if (!updated) return; + /** + * Pulls any changes to the rules that are in a policy room and updates all protected rooms + * with those changes. Does not fail if there are errors updating the room, these are reported to the management room. + * @param policyRoomId The room with a policy list which we will check for changes and apply them to all protected rooms. + * @returns When all of the protected rooms have been updated. + */ + public async syncWithPolicyRoom(policyRoomId: string): Promise { + const banList = this.banLists.find(list => list.roomId === policyRoomId); + if (banList === undefined) return; + const changes = await banList.updateList(); + await this.printBanlistChanges(changes, banList, true); let hadErrors = false; - const aclErrors = await applyServerAcls(this.banLists, Object.keys(this.protectedRooms), this); const banErrors = await applyUserBans(this.banLists, Object.keys(this.protectedRooms), this); const redactionErrors = await this.processRedactionQueue(); @@ -685,7 +688,7 @@ export class Mjolnir { // themselves. if (this.banLists.map(b => b.roomId).includes(roomId)) { if (ALL_RULE_TYPES.includes(event['type'])) { - await this.syncListForRoom(roomId); + await this.syncWithPolicyRoom(roomId); } } @@ -732,6 +735,52 @@ export class Mjolnir { } } + /** + * Print the changes to a banlist to the management room. + * @param changes A list of changes that have been made to a particular ban list. + * @param ignoreSelf Whether to exclude changes that have been made by Mjolnir. + * @returns true if the message was sent, false if it wasn't (because there there were no changes to report). + */ + private async printBanlistChanges(changes: ListRuleChange[], list: BanList, ignoreSelf = false): Promise { + if (ignoreSelf) { + const sender = await this.client.getUserId(); + changes = changes.filter(change => change.sender !== sender); + } + if (changes.length <= 0) return false; + + let html = ""; + let text = ""; + + const changesInfo = `updated with ${changes.length} ` + (changes.length === 1 ? 'change:' : 'changes:'); + const shortcodeInfo = list.listShortcode ? ` (shortcode: ${htmlEscape(list.listShortcode)})` : ''; + + html += `${htmlEscape(list.roomId)}${shortcodeInfo} ${changesInfo}
    `; + text += `${list.roomRef}${shortcodeInfo} ${changesInfo}:\n`; + + for (const change of changes) { + const rule = change.rule; + let ruleKind: string = rule.kind; + if (ruleKind === RULE_USER) { + ruleKind = 'user'; + } else if (ruleKind === RULE_SERVER) { + ruleKind = 'server'; + } else if (ruleKind === RULE_ROOM) { + ruleKind = 'room'; + } + html += `
  • ${change.changeType} ${htmlEscape(ruleKind)} (${htmlEscape(rule.recommendation)}): ${htmlEscape(rule.entity)} (${htmlEscape(rule.reason)})
    • `; + text += `* ${change.changeType} ${ruleKind} (${rule.recommendation}): ${rule.entity} (${rule.reason})\n`; + } + + const message = { + msgtype: "m.notice", + body: text, + format: "org.matrix.custom.html", + formatted_body: html, + }; + await this.client.sendMessage(config.managementRoom, message); + return true; + } + private async printActionResult(errors: RoomUpdateError[], title: string | null = null, logAnyways = false) { if (errors.length <= 0) return false; diff --git a/src/models/BanList.ts b/src/models/BanList.ts index d25c6d8..f4079ef 100644 --- a/src/models/BanList.ts +++ b/src/models/BanList.ts @@ -35,17 +35,88 @@ export function ruleTypeToStable(rule: string, unstable = true): string|null { return null; } +export enum ChangeType { + Added = "ADDED", + Removed = "REMOVED", + Modified = "MODIFIED" +} + +export interface ListRuleChange { + readonly changeType: ChangeType, + /** + * State event that caused the change. + * If the rule was redacted, this will be the redacted version of the event. + */ + readonly event: any, + /** + * The sender that caused the change. + * The original event sender unless the change is because `event` was redacted. When the change is `event` being redacted + * this will be the user who caused the redaction. + */ + readonly sender: string, + /** + * The current rule represented by the event. + * If the rule has been removed, then this will show what the rule was. + */ + readonly rule: ListRule, + /** + * The previous state that has been changed. Only (and always) provided when the change type is `ChangeType.Removed` or `Modified`. + * This will be a copy of the same event as `event` when a redaction has occurred and this will show its unredacted state. + */ + readonly previousState?: any, +} + +/** + * The BanList caches all of the rules that are active in a policy room so Mjolnir can refer to when applying bans etc. + * This cannot be used to update events in the modeled room, it is a readonly model of the policy room. + */ export default class BanList { private rules: ListRule[] = []; private shortcode: string|null = null; + // A map of state events indexed first by state type and then state keys. + private state: Map> = new Map(); + /** + * Construct a BanList, does not synchronize with the room. + * @param roomId The id of the policy room, i.e. a room containing MSC2313 policies. + * @param roomRef A sharable/clickable matrix URL that refers to the room. + * @param client A matrix client that is used to read the state of the room when `updateList` is called. + */ constructor(public readonly roomId: string, public readonly roomRef, private client: MatrixClient) { } + /** + * The code that can be used to refer to this banlist in Mjolnir commands. + */ public get listShortcode(): string { return this.shortcode || ''; } + /** + * Lookup the current rules cached for the list. + * @param stateType The event type e.g. m.room.rule.user. + * @param stateKey The state key e.g. entity:@bad:matrix.org + * @returns A state event if present or null. + */ + private getState(stateType: string, stateKey: string) { + return this.state.get(stateType)?.get(stateKey); + } + + /** + * Store this state event as part of the active room state for this BanList (used to cache rules). + * @param stateType The event type e.g. m.room.rule.user. + * @param stateKey The state key e.g. entity:@bad:matrix.org + * @param event A state event to store. + */ + private setState(stateType: string, stateKey: string, event: any): void { + let typeTable = this.state.get(stateType); + if (typeTable) { + typeTable.set(stateKey, event); + } else { + this.state.set(stateType, new Map().set(stateKey, event)); + } + } + public set listShortcode(newShortcode: string) { const currentShortcode = this.shortcode; this.shortcode = newShortcode; @@ -71,8 +142,14 @@ export default class BanList { return [...this.serverRules, ...this.userRules, ...this.roomRules]; } - public async updateList() { + /** + * 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. + * @returns A description of any rules that were added, modified or removed from the list as a result of this update. + */ + public async updateList(): Promise { this.rules = []; + let changes: ListRuleChange[] = []; const state = await this.client.getRoomState(this.roomId); for (const event of state) { @@ -96,6 +173,37 @@ export default class BanList { continue; // invalid/unknown } + const previousState = this.getState(event['type'], event['state_key']); + this.setState(event['type'], event['state_key'], event); + const changeType: null|ChangeType = (() => { + if (!previousState) { + return ChangeType.Added; + } else if (previousState['event_id'] === event['event_id']) { + if (event['unsigned']?.['redacted_because']) { + return ChangeType.Removed; + } else { + // Nothing has changed. + return null; + } + } else { + // Then the policy has been modified in some other way, possibly 'soft' redacted by a new event with empty content... + if (Object.keys(event['content']).length === 0) { + return ChangeType.Removed; + } else { + return ChangeType.Modified; + } + } + })(); + + // If we haven't got any information about what the rule used to be, then it wasn't a valid rule to begin with + // and so will not have been used. Removing a rule like this therefore results in no change. + if (changeType === ChangeType.Removed && previousState?.unsigned?.rule) { + const sender = event.unsigned['redacted_because'] ? event.unsigned['redacted_because']['sender'] : event.sender; + changes.push({changeType, event, sender, rule: previousState.unsigned.rule, + ... previousState ? {previousState} : {} }); + // Event has no content and cannot be parsed as a ListRule. + continue; + } // It's a rule - parse it const content = event['content']; if (!content) continue; @@ -107,8 +215,13 @@ export default class BanList { if (!entity || !recommendation) { continue; } - - this.rules.push(new ListRule(entity, recommendation, reason, kind)); + const rule = new ListRule(entity, recommendation, reason, kind); + event.unsigned.rule = rule; + if (changeType) { + changes.push({rule, changeType, event, sender: event.sender, ... previousState ? {previousState} : {} }); + } + this.rules.push(rule); } + return changes; } } From 7aee14bf5cf1a7c95dc9a802aa07d30101ef6b6c Mon Sep 17 00:00:00 2001 From: gnuxie Date: Fri, 19 Nov 2021 16:44:48 +0000 Subject: [PATCH 2/4] Add test for banList changes --- test/integration/banListTest.ts | 80 +++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 test/integration/banListTest.ts diff --git a/test/integration/banListTest.ts b/test/integration/banListTest.ts new file mode 100644 index 0000000..dad9d5b --- /dev/null +++ b/test/integration/banListTest.ts @@ -0,0 +1,80 @@ +import { strict as assert } from "assert"; + +import config from "../../src/config"; +import { newTestUser } from "./clientHelper"; +import { MatrixClient } from "matrix-bot-sdk"; +import BanList, { ChangeType, ListRuleChange, RULE_USER } from "../../src/models/BanList"; + +/** + * Create a policy rule in a policy room. + * @param client A matrix client that is logged in + * @param policyRoomId The room id to add the policy to. + * @param policyType The type of policy to add e.g. m.policy.rule.user. (Use RULE_USER though). + * @param entity The entity to ban e.g. @foo:example.org + * @param reason A reason for the rule e.g. 'Wouldn't stop posting spam links' + * @returns The event id of the newly created policy rule. + */ +async function createPolicyRule(client: MatrixClient, policyRoomId: string, policyType: string, entity: string, reason: string) { + return await client.sendStateEvent(policyRoomId, policyType, `rule:${entity}`, { + entity, + reason, + recommendation: 'm.ban' + }); +} + +describe("Test: Updating the BanList", function () { + it("Calculates what has changed correctly.", async function () { + this.timeout(10000); + const mjolnir = config.RUNTIME.client! + const moderator = await newTestUser(false, "moderator"); + const banListId = await mjolnir.createRoom({ invite: [await moderator.getUserId()]}); + const banList = new BanList(banListId, banListId, mjolnir); + mjolnir.setUserPowerLevel(await moderator.getUserId(), banListId, 100); + + // Test adding a new rule + await createPolicyRule(mjolnir, banListId, RULE_USER, '@added:localhost:9999', ''); + let changes: ListRuleChange[] = await banList.updateList(); + assert.equal(changes.length, 1, 'There should only be one change'); + assert.equal(changes[0].changeType, ChangeType.Added); + assert.equal(changes[0].sender, await mjolnir.getUserId()); + + // Test modifiying a rule + let originalEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', ''); + await banList.updateList(); + let nextEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', 'modified reason'); + changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Modified); + assert.equal(changes[0].previousState['event_id'], originalEventId, 'There should be a previous state event for a modified rule'); + assert.equal(changes[0].event['event_id'], nextEventId); + + // Test redacting a rule + const redactThis = await createPolicyRule(mjolnir, banListId, RULE_USER, '@redacted:localhost:9999', ''); + await banList.updateList(); + await mjolnir.redactEvent(banListId, redactThis); + changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Removed); + assert.equal(changes[0].event['event_id'], redactThis, 'Should show the new version of the event with redacted content'); + assert.equal(Object.keys(changes[0].event['content']).length, 0, 'Should show the new version of the event with redacted content'); + assert.notEqual(Object.keys(changes[0].previousState['content']), 0, 'Should have a copy of the unredacted state'); + assert.notEqual(changes[0].rule, undefined, 'The previous rule should be present'); + + // Test soft redaction of a rule + const softRedactedEntity = '@softredacted:localhost:9999' + await createPolicyRule(mjolnir, banListId, RULE_USER, softRedactedEntity, ''); + await banList.updateList(); + await mjolnir.sendStateEvent(banListId, RULE_USER, softRedactedEntity, {}); + changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Removed); + assert.equal(Object.keys(changes[0].event['content']).length, 0, 'Should show the new version of the event with redacted content'); + assert.notEqual(Object.keys(changes[0].previousState['content']), 0, 'Should have a copy of the unredacted state'); + assert.notEqual(changes[0].rule, undefined, 'The previous rule should be present'); + + // Now test a double soft redaction just to make sure stuff doesn't explode + await mjolnir.sendStateEvent(banListId, RULE_USER, softRedactedEntity, {}); + changes = await banList.updateList(); + assert.equal(changes.length, 0, "It shouldn't detect a double soft redaction as a change, it should be seen as adding an invalid rule."); + }) +}); From 0bbfe93a4b992417287ee699fb3604bf92366f82 Mon Sep 17 00:00:00 2001 From: gnuxie Date: Mon, 22 Nov 2021 15:41:12 +0000 Subject: [PATCH 3/4] Use MSC2313 m.policy.rule.* for rules and always prefer these types. The reason for doing this is because otherwise there may be duplicate rules under different state types for the same entity. This simplifies the process of modifying or invalidating rules affecting an entity because the rule with the most recent type will always be preferred. --- src/models/BanList.ts | 54 ++++++++++++---- test/integration/banListTest.ts | 106 ++++++++++++++++++++++++++++++-- 2 files changed, 144 insertions(+), 16 deletions(-) diff --git a/src/models/BanList.ts b/src/models/BanList.ts index f4079ef..4be1c35 100644 --- a/src/models/BanList.ts +++ b/src/models/BanList.ts @@ -17,13 +17,17 @@ limitations under the License. import { extractRequestError, LogService, MatrixClient } from "matrix-bot-sdk"; import { ListRule } from "./ListRule"; -export const RULE_USER = "m.room.rule.user"; -export const RULE_ROOM = "m.room.rule.room"; -export const RULE_SERVER = "m.room.rule.server"; +export const RULE_USER = "m.policy.rule.user"; +export const RULE_ROOM = "m.policy.rule.room"; +export const RULE_SERVER = "m.policy.rule.server"; -export const USER_RULE_TYPES = [RULE_USER, "org.matrix.mjolnir.rule.user"]; -export const ROOM_RULE_TYPES = [RULE_ROOM, "org.matrix.mjolnir.rule.room"]; -export const SERVER_RULE_TYPES = [RULE_SERVER, "org.matrix.mjolnir.rule.server"]; +// README! The order here matters for determining whether a type is obsolete, most recent should be first. +// These are the current and historical types for each type of rule which were used while MSC2313 was being developed +// and were left as an artifact for some time afterwards. +// Most rules (as of writing) will have the prefix `m.room.rule.*` as this has been in use for roughly 2 years. +export const USER_RULE_TYPES = [RULE_USER, "m.room.rule.user", "org.matrix.mjolnir.rule.user"]; +export const ROOM_RULE_TYPES = [RULE_ROOM, "m.room.rule.room", "org.matrix.mjolnir.rule.room"]; +export const SERVER_RULE_TYPES = [RULE_SERVER, "m.room.rule.server", "org.matrix.mjolnir.rule.server"]; export const ALL_RULE_TYPES = [...USER_RULE_TYPES, ...ROOM_RULE_TYPES, ...SERVER_RULE_TYPES]; export const SHORTCODE_EVENT_TYPE = "org.matrix.mjolnir.shortcode"; @@ -94,8 +98,8 @@ export default class BanList { /** * Lookup the current rules cached for the list. - * @param stateType The event type e.g. m.room.rule.user. - * @param stateKey The state key e.g. entity:@bad:matrix.org + * @param stateType The event type e.g. m.policy.rule.user. + * @param stateKey The state key e.g. rule:@bad:matrix.org * @returns A state event if present or null. */ private getState(stateType: string, stateKey: string) { @@ -104,8 +108,9 @@ export default class BanList { /** * Store this state event as part of the active room state for this BanList (used to cache rules). - * @param stateType The event type e.g. m.room.rule.user. - * @param stateKey The state key e.g. entity:@bad:matrix.org + * The state type should be normalised if it is obsolete e.g. m.room.rule.user should be stored as m.policy.rule.user. + * @param stateType The event type e.g. m.room.policy.user. + * @param stateKey The state key e.g. rule:@bad:matrix.org * @param event A state event to store. */ private setState(stateType: string, stateKey: string, event: any): void { @@ -173,8 +178,33 @@ export default class BanList { continue; // invalid/unknown } - const previousState = this.getState(event['type'], event['state_key']); - this.setState(event['type'], event['state_key'], event); + const previousState = this.getState(kind, event['state_key']); + + // Now we need to figure out if the current event is of an obsolete type + // (e.g. org.matrix.mjolnir.rule.user) when compared to the previousState (which might be m.policy.rule.user). + // We do not want to overwrite a rule of a newer type with an older type even if the event itself is supposedly more recent + // as it may be someone deleting the older versions of the rules. + if (previousState) { + const logObsoleteRule = () => { + LogService.info('BanList', `In BanList ${this.roomRef}, conflict between rules ${event['event_id']} (with obsolete type ${event['type']}) ` + + `and ${previousState['event_id']} (with standard type ${previousState['type']}). Ignoring rule with obsolete type.`); + } + if (kind === RULE_USER && USER_RULE_TYPES.indexOf(event['type']) > USER_RULE_TYPES.indexOf(previousState['type'])) { + logObsoleteRule(); + continue; + } else if (kind === RULE_ROOM && ROOM_RULE_TYPES.indexOf(event['type']) > ROOM_RULE_TYPES.indexOf(previousState['type'])) { + logObsoleteRule(); + continue; + } else if (kind === RULE_SERVER && SERVER_RULE_TYPES.indexOf(event['type']) > SERVER_RULE_TYPES.indexOf(previousState['type'])) { + logObsoleteRule(); + continue; + } + } + + // The reason we set the state at this point is because it is valid to want to set the state to an invalid rule + // in order to mark a rule as deleted. + // We always set state with the normalised state type via `kind` to de-duplicate rules. + this.setState(kind, event['state_key'], event); const changeType: null|ChangeType = (() => { if (!previousState) { return ChangeType.Added; diff --git a/test/integration/banListTest.ts b/test/integration/banListTest.ts index dad9d5b..4eb6476 100644 --- a/test/integration/banListTest.ts +++ b/test/integration/banListTest.ts @@ -31,26 +31,38 @@ describe("Test: Updating the BanList", function () { const banList = new BanList(banListId, banListId, mjolnir); mjolnir.setUserPowerLevel(await moderator.getUserId(), banListId, 100); + assert.equal(banList.allRules.length, 0); + // Test adding a new rule await createPolicyRule(mjolnir, banListId, RULE_USER, '@added:localhost:9999', ''); let changes: ListRuleChange[] = await banList.updateList(); assert.equal(changes.length, 1, 'There should only be one change'); assert.equal(changes[0].changeType, ChangeType.Added); assert.equal(changes[0].sender, await mjolnir.getUserId()); + assert.equal(banList.userRules.length, 1); + assert.equal(banList.allRules.length, 1); // Test modifiying a rule let originalEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', ''); await banList.updateList(); - let nextEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', 'modified reason'); + let modifyingEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', 'modified reason'); changes = await banList.updateList(); assert.equal(changes.length, 1); assert.equal(changes[0].changeType, ChangeType.Modified); assert.equal(changes[0].previousState['event_id'], originalEventId, 'There should be a previous state event for a modified rule'); - assert.equal(changes[0].event['event_id'], nextEventId); + assert.equal(changes[0].event['event_id'], modifyingEventId); + let modifyingAgainEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', 'modified again'); + changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Modified); + assert.equal(changes[0].previousState['event_id'], modifyingEventId, 'There should be a previous state event for a modified rule'); + assert.equal(changes[0].event['event_id'], modifyingAgainEventId); + assert.equal(banList.userRules.length, 2, 'There should be two rules, one for @modified:localhost:9999 and one for @added:localhost:9999'); // Test redacting a rule const redactThis = await createPolicyRule(mjolnir, banListId, RULE_USER, '@redacted:localhost:9999', ''); await banList.updateList(); + assert.equal(banList.userRules.filter(r => r.entity === '@redacted:localhost:9999').length, 1); await mjolnir.redactEvent(banListId, redactThis); changes = await banList.updateList(); assert.equal(changes.length, 1); @@ -59,22 +71,108 @@ describe("Test: Updating the BanList", function () { assert.equal(Object.keys(changes[0].event['content']).length, 0, 'Should show the new version of the event with redacted content'); assert.notEqual(Object.keys(changes[0].previousState['content']), 0, 'Should have a copy of the unredacted state'); assert.notEqual(changes[0].rule, undefined, 'The previous rule should be present'); + assert.equal(banList.userRules.filter(r => r.entity === '@redacted:localhost:9999').length, 0, 'The rule should be removed.'); // Test soft redaction of a rule const softRedactedEntity = '@softredacted:localhost:9999' await createPolicyRule(mjolnir, banListId, RULE_USER, softRedactedEntity, ''); await banList.updateList(); - await mjolnir.sendStateEvent(banListId, RULE_USER, softRedactedEntity, {}); + assert.equal(banList.userRules.filter(r => r.entity === softRedactedEntity).length, 1); + await mjolnir.sendStateEvent(banListId, RULE_USER, `rule:${softRedactedEntity}`, {}); changes = await banList.updateList(); assert.equal(changes.length, 1); assert.equal(changes[0].changeType, ChangeType.Removed); assert.equal(Object.keys(changes[0].event['content']).length, 0, 'Should show the new version of the event with redacted content'); assert.notEqual(Object.keys(changes[0].previousState['content']), 0, 'Should have a copy of the unredacted state'); assert.notEqual(changes[0].rule, undefined, 'The previous rule should be present'); + assert.equal(banList.userRules.filter(r => r.entity === softRedactedEntity).length, 0, 'The rule should have been removed'); // Now test a double soft redaction just to make sure stuff doesn't explode - await mjolnir.sendStateEvent(banListId, RULE_USER, softRedactedEntity, {}); + await mjolnir.sendStateEvent(banListId, RULE_USER, `rule:${softRedactedEntity}`, {}); changes = await banList.updateList(); assert.equal(changes.length, 0, "It shouldn't detect a double soft redaction as a change, it should be seen as adding an invalid rule."); + assert.equal(banList.userRules.filter(r => r.entity === softRedactedEntity).length, 0, 'The rule should have been removed'); + + // Test that different (old) rule types will be modelled as the latest event type. + originalEventId = await createPolicyRule(mjolnir, banListId, 'org.matrix.mjolnir.rule.user', '@old:localhost:9999', ''); + changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Added); + assert.equal(banList.userRules.filter(r => r.entity === '@old:localhost:9999').length, 1); + modifyingEventId = await createPolicyRule(mjolnir, banListId, 'm.room.rule.user', '@old:localhost:9999', 'modified reason'); + changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Modified); + assert.equal(changes[0].event['event_id'], modifyingEventId); + assert.equal(changes[0].previousState['event_id'], originalEventId, 'There should be a previous state event for a modified rule'); + assert.equal(banList.userRules.filter(r => r.entity === '@old:localhost:9999').length, 1); + modifyingAgainEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@old:localhost:9999', 'changes again'); + changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Modified); + assert.equal(changes[0].event['event_id'], modifyingAgainEventId); + assert.equal(changes[0].previousState['event_id'], modifyingEventId, 'There should be a previous state event for a modified rule'); + assert.equal(banList.userRules.filter(r => r.entity === '@old:localhost:9999').length, 1); + }) + it("Will remove rules with old types when they are 'soft redacted' with a different but more recent event type.", async function () { + this.timeout(3000); + const mjolnir = config.RUNTIME.client! + const moderator = await newTestUser(false, "moderator"); + const banListId = await mjolnir.createRoom({ invite: [await moderator.getUserId()]}); + const banList = new BanList(banListId, banListId, mjolnir); + mjolnir.setUserPowerLevel(await moderator.getUserId(), banListId, 100); + + const entity = '@old:localhost:9999'; + let originalEventId = await createPolicyRule(mjolnir, banListId, 'm.room.rule.user', entity, ''); + let changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Added); + assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 1, 'There should be a rule stored that we just added...') + let softRedactingEventId = await mjolnir.sendStateEvent(banListId, RULE_USER, `rule:${entity}`, {}); + changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Removed); + assert.equal(changes[0].event['event_id'], softRedactingEventId); + assert.equal(changes[0].previousState['event_id'], originalEventId, 'There should be a previous state event for a modified rule'); + assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 0, 'The rule should no longer be stored.'); + }) + it("A rule of the most recent type won't be deleted when an old rule is deleted for the same entity.", async function () { + this.timeout(3000); + const mjolnir = config.RUNTIME.client! + const moderator = await newTestUser(false, "moderator"); + const banListId = await mjolnir.createRoom({ invite: [await moderator.getUserId()]}); + const banList = new BanList(banListId, banListId, mjolnir); + mjolnir.setUserPowerLevel(await moderator.getUserId(), banListId, 100); + + const entity = '@old:localhost:9999'; + let originalEventId = await createPolicyRule(mjolnir, banListId, 'm.room.rule.user', entity, ''); + let changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Added); + assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 1, 'There should be a rule stored that we just added...') + let updatedEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, entity, ''); + changes = await banList.updateList(); + // If in the future you change this and it fails, it's really subjective whether this constitutes a modification, since the only thing that has changed + // is the rule type. The actual content is identical. + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Modified); + assert.equal(changes[0].event['event_id'], updatedEventId); + assert.equal(changes[0].previousState['event_id'], originalEventId, 'There should be a previous state event for a modified rule'); + assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 1, 'Only the latest version of the rule gets returned.'); + + // Now we delete the old version of the rule without consequence. + await mjolnir.sendStateEvent(banListId, 'm.room.rule.user', `rule:${entity}`, {}); + changes = await banList.updateList(); + assert.equal(changes.length, 0); + assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 1, 'The rule should still be active.'); + + // And we can still delete the new version of the rule. + let softRedactingEventId = await mjolnir.sendStateEvent(banListId, RULE_USER, `rule:${entity}`, {}); + changes = await banList.updateList(); + assert.equal(changes.length, 1); + assert.equal(changes[0].changeType, ChangeType.Removed); + assert.equal(changes[0].event['event_id'], softRedactingEventId); + assert.equal(changes[0].previousState['event_id'], updatedEventId, 'There should be a previous state event for a modified rule'); + assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 0, 'The rule should no longer be stored.'); }) }); From 33011ddb04cd0d2ac0f86fd87a951c77c1d695c1 Mon Sep 17 00:00:00 2001 From: gnuxie Date: Wed, 24 Nov 2021 12:29:14 +0000 Subject: [PATCH 4/4] Store BanList rules only in the room state cache. We do this so that there is only one source of truth for which rules are active and it simplifies de-duplicating rules of conflicting event types (e.g. m.room.rule.user vs m.policy.rule.user). --- src/models/BanList.ts | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/models/BanList.ts b/src/models/BanList.ts index 4be1c35..c78a6ce 100644 --- a/src/models/BanList.ts +++ b/src/models/BanList.ts @@ -75,7 +75,6 @@ export interface ListRuleChange { * This cannot be used to update events in the modeled room, it is a readonly model of the policy room. */ export default class BanList { - private rules: ListRule[] = []; private shortcode: string|null = null; // A map of state events indexed first by state type and then state keys. private state: Map> = new Map(); @@ -122,6 +121,25 @@ export default class BanList { } } + /** + * Return all the active rules of a given kind. + * @param kind e.g. RULE_SERVER (m.policy.rule.server) + * @returns The active ListRules for the ban list of that kind. + */ + private rulesOfKind(kind: string): ListRule[] { + const rules: ListRule[] = [] + const stateKeyMap = this.state.get(kind); + if (stateKeyMap) { + for (const event of stateKeyMap.values()) { + const rule = event?.unsigned?.rule; + if (rule && rule.kind === kind) { + rules.push(rule); + } + } + } + return rules; + } + public set listShortcode(newShortcode: string) { const currentShortcode = this.shortcode; this.shortcode = newShortcode; @@ -132,15 +150,15 @@ export default class BanList { } public get serverRules(): ListRule[] { - return this.rules.filter(r => r.kind === RULE_SERVER); + return this.rulesOfKind(RULE_SERVER); } public get userRules(): ListRule[] { - return this.rules.filter(r => r.kind === RULE_USER); + return this.rulesOfKind(RULE_USER); } public get roomRules(): ListRule[] { - return this.rules.filter(r => r.kind === RULE_ROOM); + return this.rulesOfKind(RULE_ROOM); } public get allRules(): ListRule[] { @@ -153,7 +171,6 @@ export default class BanList { * @returns A description of any rules that were added, modified or removed from the list as a result of this update. */ public async updateList(): Promise { - this.rules = []; let changes: ListRuleChange[] = []; const state = await this.client.getRoomState(this.roomId); @@ -250,7 +267,6 @@ export default class BanList { if (changeType) { changes.push({rule, changeType, event, sender: event.sender, ... previousState ? {previousState} : {} }); } - this.rules.push(rule); } return changes; }