Merge pull request #143 from matrix-org/gnuxie/banlist-change-watching

Provide notice showing how a BanList has changed after updating.

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;
+     * Pulls any changes to the rules that are in a policy room and updates all protected rooms
-        for (const list of this.banLists) {
+     * with those changes. Does not fail if there are errors updating the room, these are reported to the management room.
-            if (list.roomId !== roomId) continue;
+     * @param policyRoomId The room with a policy list which we will check for changes and apply them to all protected rooms.
-            await list.updateList();
+     * @returns When all of the protected rooms have been updated.
-            updated = true;
+     */
-        }
+    public async syncWithPolicyRoom(policyRoomId: string): Promise<void> {
-        if (!updated) return;
+        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<boolean> {
+        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 += `<a href="${htmlEscape(list.roomRef)}">${htmlEscape(list.roomId)}</a>${shortcodeInfo} ${changesInfo}<br/><ul>`;
+        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 += `<li>${change.changeType} ${htmlEscape(ruleKind)} (<code>${htmlEscape(rule.recommendation)}</code>): <code>${htmlEscape(rule.entity)}</code> (${htmlEscape(rule.reason)})</li>`;
+            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;
 

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_USER = "m.policy.rule.user";
-export const RULE_ROOM = "m.room.rule.room";
+export const RULE_ROOM = "m.policy.rule.room";
-export const RULE_SERVER = "m.room.rule.server";
+export const RULE_SERVER = "m.policy.rule.server";
 
-export const USER_RULE_TYPES = [RULE_USER, "org.matrix.mjolnir.rule.user"];
+// README! The order here matters for determining whether a type is obsolete, most recent should be first.
-export const ROOM_RULE_TYPES = [RULE_ROOM, "org.matrix.mjolnir.rule.room"];
+// These are the current and historical types for each type of rule which were used while MSC2313 was being developed
-export const SERVER_RULE_TYPES = [RULE_SERVER, "org.matrix.mjolnir.rule.server"];
+// 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";
@@ -35,17 +39,107 @@ export function ruleTypeToStable(rule: string, unstable = true): string|null {
     return null;
 }
 
-export default class BanList {
+export enum ChangeType {
-    private rules: ListRule[] = [];
+    Added    = "ADDED",
-    private shortcode: string|null = null;
+    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 shortcode: string|null = null;
+    // A map of state events indexed first by state type and then state keys.
+    private state: Map<string, Map<string, any>> = 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.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) {
+        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).
+     * 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 {
+        let typeTable = this.state.get(stateType);
+        if (typeTable) {
+            typeTable.set(stateKey, event);
+        } else {
+            this.state.set(stateType, new Map().set(stateKey, event));
+        }
+    }
+
+    /**
+     * 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;
@@ -56,23 +150,28 @@ 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[] {
         return [...this.serverRules, ...this.userRules, ...this.roomRules];
     }
 
-    public async updateList() {
+    /**
-        this.rules = [];
+     * 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<ListRuleChange[]> {
+        let changes: ListRuleChange[] = [];
 
         const state = await this.client.getRoomState(this.roomId);
         for (const event of state) {
@@ -96,6 +195,62 @@ export default class BanList {
                 continue; // invalid/unknown
             }
 
+            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;
+                } 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 +262,12 @@ export default class BanList {
             if (!entity || !recommendation) {
                 continue;
             }
-
+            const rule = new ListRule(entity, recommendation, reason, kind);
-            this.rules.push(new ListRule(entity, recommendation, reason, kind));
+            event.unsigned.rule = rule;
+            if (changeType) {
+                changes.push({rule, changeType, event, sender: event.sender, ... previousState ? {previousState} : {} });
+            }
         }
+        return changes;
     }
 }

test/integration/banListTest.ts

@@ -0,0 +1,178 @@
+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);
+
+        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 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'], 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);
+        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');
+        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();
+        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, `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.');
+    })
+});