Rework the banning and unbanning of entities in PolicyLists. (#345)

* Rework the banning and unbanning of entities in PolicyLists.

1. We keep track of the event that created a list rule so that we
can remove the rule by having a way to determine the original state key for the rule.
This is because the state key of rules can be anything and should not be
relied on by Mjolnir to unban things (which it was doing).

2. The old scheme for producing a state key was causing for some entities to escape bans
https://github.com/matrix-org/mjolnir/issues/322.

We could have used a hash or something similar, but we know that
the reason for the `rule:${entity}` scheme existed was for ease of debugging
and finding rules in devtools. So instead we have followed a scheme simalar to
bridges where the first character of an mxid is replaced with an underscore.
Everything else just gets put into the state key. Since domains can't have '@'
and room ids, aliases can't either.

3. We have stopped the need for Mjolnir to wait for the next response from sync after banning,
unbanning an entity so that we can apply ACL's sooner.

* Use PolicyList's `banEntity` method to create imported rules.

src/Mjolnir.ts

@@ -1009,7 +1009,7 @@ export class Mjolnir {
         const policyList = this.policyLists.find(list => list.roomId === roomId);
         if (policyList !== undefined) {
             if (ALL_BAN_LIST_RULE_TYPES.includes(event['type']) || event['type'] === 'm.room.redaction') {
-                policyList.updateForEvent(event)
+                policyList.updateForEvent(event.event_id)
             }
         }
 

src/commands/ImportCommand.ts

@@ -16,12 +16,13 @@ limitations under the License.
 
 import { Mjolnir } from "../Mjolnir";
 import { RichReply } from "matrix-bot-sdk";
-import { EntityType, Recommendation } from "../models/ListRule";
+import { EntityType } from "../models/ListRule";
+import PolicyList from "../models/PolicyList";
 
 // !mjolnir import <room ID> <shortcode>
 export async function execImportCommand(roomId: string, event: any, mjolnir: Mjolnir, parts: string[]) {
     const importRoomId = await mjolnir.client.resolveRoom(parts[2]);
-    const list = mjolnir.lists.find(b => b.listShortcode === parts[3]);
+    const list = mjolnir.lists.find(b => b.listShortcode === parts[3]) as PolicyList;
     if (!list) {
         const errMessage = "Unable to find list - check your shortcode.";
         const errReply = RichReply.createFor(roomId, event, errMessage, errMessage);
@@ -43,17 +44,7 @@ export async function execImportCommand(roomId: string, event: any, mjolnir: Mjo
                 const reason = content['reason'] || '<no reason>';
 
                 await mjolnir.client.sendNotice(mjolnir.managementRoomId, `Adding user ${stateEvent['state_key']} to ban list`);
-
+                await list.banEntity(EntityType.RULE_USER, stateEvent['state_key'], reason);
-                const ruleContent = {
-                    entity: stateEvent['state_key'],
-                    recommendation: Recommendation.Ban,
-                    reason: reason,
-                };
-                const stateKey = `rule:${ruleContent.entity}`;
-                let stableRule = EntityType.RULE_USER;
-                if (stableRule) {
-                    await mjolnir.client.sendStateEvent(list.roomId, stableRule, stateKey, ruleContent);
-                }
                 importedRules++;
             }
         } else if (stateEvent['type'] === 'm.room.server_acl' && stateEvent['state_key'] === '') {
@@ -64,16 +55,7 @@ export async function execImportCommand(roomId: string, event: any, mjolnir: Mjo
 
                 await mjolnir.client.sendNotice(mjolnir.managementRoomId, `Adding server ${server} to ban list`);
 
-                const ruleContent = {
+                await list.banEntity(EntityType.RULE_SERVER, server, reason);
-                    entity: server,
-                    recommendation: Recommendation.Ban,
-                    reason: reason,
-                };
-                const stateKey = `rule:${ruleContent.entity}`;
-                let stableRule = EntityType.RULE_SERVER;
-                if (stableRule) {
-                    await mjolnir.client.sendStateEvent(list.roomId, stableRule, stateKey, ruleContent);
-                }
                 importedRules++;
             }
         }

src/commands/UnbanBanCommand.ts

@@ -17,7 +17,7 @@ limitations under the License.
 import { Mjolnir } from "../Mjolnir";
 import PolicyList from "../models/PolicyList";
 import { extractRequestError, LogLevel, LogService, MatrixGlob, RichReply } from "matrix-bot-sdk";
-import { Recommendation, RULE_ROOM, RULE_SERVER, RULE_USER, USER_RULE_TYPES } from "../models/ListRule";
+import { RULE_ROOM, RULE_SERVER, RULE_USER, USER_RULE_TYPES } from "../models/ListRule";
 import { DEFAULT_LIST_EVENT_TYPE } from "./SetDefaultBanListCommand";
 
 interface Arguments {
@@ -118,14 +118,7 @@ export async function execBanCommand(roomId: string, event: any, mjolnir: Mjolni
     const bits = await parseArguments(roomId, event, mjolnir, parts);
     if (!bits) return; // error already handled
 
-    const ruleContent = {
+    await bits.list!.banEntity(bits.ruleType!, bits.entity, bits.reason);
-        entity: bits.entity,
-        recommendation: Recommendation.Ban,
-        reason: bits.reason || '<no reason supplied>',
-    };
-    const stateKey = `rule:${bits.entity}`;
-
-    await mjolnir.client.sendStateEvent(bits.list!.roomId, bits.ruleType!, stateKey, ruleContent);
     await mjolnir.client.unstableApis.addReactionToEvent(roomId, event['event_id'], '✅');
 }
 

src/models/ListRule.ts

@@ -78,6 +78,13 @@ const RECOMMENDATION_OPINION_VARIANTS: string[] = [
 export const OPINION_MIN = -100;
 export const OPINION_MAX = +100;
 
+interface MatrixStateEvent {
+    type: string,
+    content: any,
+    event_id: string,
+    state_key: string,
+}
+
 /**
  * Representation of a rule within a Policy List.
  */
@@ -87,6 +94,10 @@ export abstract class ListRule {
      */
     private glob: MatrixGlob;
     constructor(
+        /**
+         * The event source for the rule.
+         */
+        public readonly sourceEvent: MatrixStateEvent,
         /**
          * The entity covered by this rule, e.g. a glob user ID, a room ID, a server domain.
          */
@@ -120,7 +131,7 @@ export abstract class ListRule {
      * @param event An *untrusted* event.
      * @returns null if the ListRule is invalid or not recognized by Mjölnir.
      */
-    public static parse(event: {type: string, content: any}): ListRule | null {
+    public static parse(event: MatrixStateEvent): ListRule | null {
         // Parse common fields.
         // If a field is ill-formed, discard the rule.
         const content = event['content'];
@@ -155,17 +166,17 @@ export abstract class ListRule {
 
         // From this point, we may need specific fields.
         if (RECOMMENDATION_BAN_VARIANTS.includes(recommendation)) {
-            return new ListRuleBan(entity, reason, kind);
+            return new ListRuleBan(event, entity, reason, kind);
         } else if (RECOMMENDATION_OPINION_VARIANTS.includes(recommendation)) {
             let opinion = content['opinion'];
             if (!Number.isInteger(opinion)) {
                 return null;
             }
-            return new ListRuleOpinion(entity, reason, kind, opinion);
+            return new ListRuleOpinion(event, entity, reason, kind, opinion);
         } else {
             // As long as the `recommendation` is defined, we assume
             // that the rule is correct, just unknown.
-            return new ListRuleUnknown(entity, reason, kind, content);
+            return new ListRuleUnknown(event, entity, reason, kind, content);
         }
     }
 }
@@ -175,6 +186,10 @@ export abstract class ListRule {
  */
 export class ListRuleBan extends ListRule {
     constructor(
+        /**
+         * The event source for the rule.
+         */
+        sourceEvent: MatrixStateEvent,
         /**
          * The entity covered by this rule, e.g. a glob user ID, a room ID, a server domain.
          */
@@ -188,7 +203,7 @@ export class ListRuleBan extends ListRule {
          */
         kind: EntityType,
     ) {
-        super(entity, reason, kind, Recommendation.Ban)
+        super(sourceEvent, entity, reason, kind, Recommendation.Ban)
     }
 }
 
@@ -197,6 +212,10 @@ export class ListRuleBan extends ListRule {
  */
 export class ListRuleOpinion extends ListRule {
     constructor(
+        /**
+         * The event source for the rule.
+         */
+        sourceEvent: MatrixStateEvent,
         /**
          * The entity covered by this rule, e.g. a glob user ID, a room ID, a server domain.
          */
@@ -216,7 +235,7 @@ export class ListRuleOpinion extends ListRule {
          */
         public readonly opinion: number
     ) {
-        super(entity, reason, kind, Recommendation.Opinion);
+        super(sourceEvent, entity, reason, kind, Recommendation.Opinion);
         if (!Number.isInteger(opinion)) {
             throw new TypeError(`The opinion must be an integer, got ${opinion}`);
         }
@@ -231,6 +250,10 @@ export class ListRuleOpinion extends ListRule {
  */
 export class ListRuleUnknown extends ListRule {
     constructor(
+        /**
+         * The event source for the rule.
+         */
+        sourceEvent: MatrixStateEvent,
         /**
          * The entity covered by this rule, e.g. a glob user ID, a room ID, a server domain.
          */
@@ -248,6 +271,6 @@ export class ListRuleUnknown extends ListRule {
          */
         public readonly content: any,
     ) {
-        super(entity, reason, kind, null);
+        super(sourceEvent, entity, reason, kind, null);
     }
 }

src/models/PolicyList.ts

@@ -63,13 +63,28 @@ declare interface PolicyList {
 /**
  * The PolicyList 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.
+ *
+ * The policy list needs to be updated manually, it has no way of knowing about new events in it's modelled matrix room on its own.
+ * You can inform the PolicyList about new events in the matrix side of policy room with the `updateForEvent`, this will eventually
+ * cause the PolicyList to update its view of the room (via `updateList`) if it doesn't know about that state event.
+ * Each time the PolicyList has finished updating, it will emit the `'PolicyList.update'` event on itself as an EventEmitter.
+ *
+ * Implementation note: The reason why the PolicyList has to update via a call to `/state` is because
+ * you cannot rely on the timeline portion of `/sync` to provide a consistent view of the room state as you
+ * receive events in stream order.
  */
 class PolicyList extends EventEmitter {
     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();
+    /**
+     * Allow us to detect whether we have updated the state for this event.
+     */
+    private stateByEventId: Map<string /* event id */, any> = new Map();
     // Batches new events from sync together before starting the process to update the list.
     private readonly batcher: UpdateBatcher;
+    // Events that we have already informed the batcher about, that we haven't loaded from the room state yet.
+    private batchedEvents = new Set<string /* event id */>();
 
     /**
      * Construct a PolicyList, does not synchronize with the room.
@@ -113,6 +128,7 @@ class PolicyList extends EventEmitter {
         } else {
             this.state.set(stateType, new Map().set(stateKey, event));
         }
+        this.stateByEventId.set(event.event_id, event);
     }
 
     /**
@@ -194,6 +210,23 @@ class PolicyList extends EventEmitter {
         }
     }
 
+    /**
+     * Ban an entity with Recommendation.Ban from the list.
+     * @param ruleType The type of rule e.g. RULE_USER.
+     * @param entity The entity to ban.
+     * @param reason A reason we are banning them.
+     */
+    public async banEntity(ruleType: string, entity: string, reason?: string): Promise<void> {
+        // '@' at the beginning of state keys is reserved.
+        const stateKey = ruleType === RULE_USER ? '_' + entity.substring(1) : entity;
+        const event_id = await this.client.sendStateEvent(this.roomId, ruleType, stateKey, {
+            entity,
+            recommendation: Recommendation.Ban,
+            reason: reason || '<no reason supplied>',
+        });
+        this.updateForEvent(event_id);
+    }
+
     /**
      * 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.
@@ -202,7 +235,6 @@ class PolicyList extends EventEmitter {
      * @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<boolean> {
-        const stateKey = `rule:${entity}`;
         let typesToCheck = [ruleType];
         switch (ruleType) {
             case RULE_USER:
@@ -215,17 +247,26 @@ class PolicyList extends EventEmitter {
                 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 sendNullState = async (stateType: string, stateKey: string) => {
-        const typesToRemove = (await Promise.all(
+            const event_id = await this.client.sendStateEvent(this.roomId, stateType, stateKey, {});
-            typesToCheck.map(stateType => this.client.getRoomStateEvent(this.roomId, stateType, stateKey)
+            this.updateForEvent(event_id);
-                .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, {})));
+        const removeRule = async (rule: ListRule): Promise<void> => {
-        return true;
+            const stateKey = rule.sourceEvent.state_key;
+            // 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;
+            }
+            await Promise.all(typesToRemove.map(stateType => sendNullState(stateType!, stateKey)));
+        }
+        const rules = this.rulesMatchingEntity(entity, ruleType);
+        await Promise.all(rules.map(removeRule));
+        return rules.length > 0;
     }
 
     /**
@@ -305,6 +346,11 @@ class PolicyList extends EventEmitter {
                 }
             })();
 
+            // Clear out any events that we were informed about via updateForEvent.
+            if (changeType !== null) {
+                this.batchedEvents.delete(event.event_id)
+            }
+
             // 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) {
@@ -328,6 +374,13 @@ class PolicyList extends EventEmitter {
             }
         }
         this.emit('PolicyList.update', this, changes);
+        if (this.batchedEvents.keys.length !== 0) {
+            // The only reason why this isn't a TypeError is because we need to know about this when it happens, because it means
+            // we're probably doing something wrong, on the other hand, if someone messes with a server implementation and
+            // strange things happen where events appear in /sync sooner than they do in /state (which would be outrageous)
+            // we don't want Mjolnir to stop working properly. Though, I am not confident a burried warning is going to alert us.
+            LogService.warn("PolicyList", "The policy list is being informed about events that it cannot find in the room state, this is really bad and you should seek help.");
+        }
         return changes;
     }
 
@@ -335,8 +388,12 @@ class PolicyList extends EventEmitter {
      * Inform the `PolicyList` about a new event from the room it is modelling.
      * @param event An event from the room the `PolicyList` models to inform an instance about.
      */
-    public updateForEvent(event: { event_id: string }): void {
+    public updateForEvent(eventId: string): void {
-        this.batcher.addToBatch(event.event_id)
+        if (this.stateByEventId.has(eventId) || this.batchedEvents.has(eventId)) {
+            return; // we already know about this event.
+        }
+        this.batcher.addToBatch(eventId);
+        this.batchedEvents.add(eventId);
     }
 }
 

test/integration/banListTest.ts

@@ -18,8 +18,8 @@ import { ALL_RULE_TYPES, RULE_SERVER, RULE_USER, SERVER_RULE_TYPES } from "../..
  * @param template The template to use for the policy rule event.
  * @returns The event id of the newly created policy rule.
  */
-async function createPolicyRule(client: MatrixClient, policyRoomId: string, policyType: string, entity: string, reason: string, template = { recommendation: 'm.ban' }) {
+async function createPolicyRule(client: MatrixClient, policyRoomId: string, policyType: string, entity: string, reason: string, template = { recommendation: 'm.ban' }, stateKey = `rule:${entity}`) {
-    return await client.sendStateEvent(policyRoomId, policyType, `rule:${entity}`, {
+    return await client.sendStateEvent(policyRoomId, policyType, stateKey, {
         entity,
         reason,
         ...template,
@@ -327,18 +327,19 @@ describe('Test: unbaning entities via the PolicyList.', function() {
         const banList = new PolicyList(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 olderBadServer = "old.evil.example"
-        const newerBadServer = "new.evil.com"
+        const newerBadServer = "new.evil.example"
         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.');
+        await createPolicyRule(moderator, banListId, RULE_SERVER, newerBadServer, 'hidden with a non-standard state key', undefined, "rule_1");
         // 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.
+        // rules are normalized by rule type, that's why there should only be 3.
-        assert.equal(banList.allRules.length, 2);
+        assert.equal(banList.allRules.length, 3);
 
-        // Check that we have setup our test properly and therefore evil.com is banned.
+        // Check that we have setup our test properly and therefore evil.example is banned.
         const acl = new ServerAcl(serverName).denyIpAddresses().allowServer("*").denyServer(olderBadServer).denyServer(newerBadServer);
         const protectedAcl = await mjolnir.client.getRoomStateEvent(protectedRoom, "m.room.server_acl", "");
         if (!acl.matches(protectedAcl)) {