Make updateForEvent actually update PolicyLists. (#448)

For some reason we were relying on a mjolnir listening to
`'PolicyList.batch'` to update policy lists.

This was exposing an implementation detail to Mjolnir
and including it as part of the implementation of
`PolicyList.updateForEvent()` which is supposed to cause
the `PolicyList` to update (eventually).

I am confident this was because of a need before batching was
introduced to get the changes to a policy list directly
from the method call to `PolicyList.update()`, whereas
now you can just listen to `PolicyList.update`.

The `'PolicyList.batch'` event has now been removed
and the PolicyList event batcher (`UpdateBatcher`)
now calls `PolicyList.update()` internally.

src/Mjolnir.ts

@@ -436,7 +436,6 @@ export class Mjolnir {
     private async addPolicyList(roomId: string, roomRef: string): Promise<PolicyList> {
         const list = new PolicyList(roomId, roomRef, this.client);
         this.ruleServer?.watch(list);
-        list.on('PolicyList.batch', (...args) => this.protectedRoomsTracker.syncWithPolicyList(...args));
         await list.updateList();
         this.policyLists.push(list);
         this.protectedRoomsTracker.watchList(list);

src/ProtectedRoomsSet.ts

@@ -88,6 +88,12 @@ export class ProtectedRoomsSet {
      */
     private readonly accessControlUnit = new AccessControlUnit([]);
 
+    /**
+     * Intended to be `this.syncWithUpdatedPolicyList` so we can add it in `this.watchList` and remove it in `this.unwatchList`.
+     * Otherwise we would risk being informed about lists we no longer watch.
+     */
+    private readonly listUpdateListener: (list: PolicyList, changes: ListRuleChange[]) => void;
+
     constructor(
         private readonly client: MatrixSendClient,
         private readonly clientUserId: string,
@@ -108,6 +114,7 @@ export class ProtectedRoomsSet {
 
         // Setup room activity watcher
         this.protectedRoomActivityTracker = new ProtectedRoomActivityTracker();
+        this.listUpdateListener = this.syncWithUpdatedPolicyList.bind(this);
     }
 
     /**
@@ -143,12 +150,14 @@ export class ProtectedRoomsSet {
         if (!this.policyLists.includes(policyList)) {
             this.policyLists.push(policyList);
             this.accessControlUnit.watchList(policyList);
+            policyList.on('PolicyList.update', this.listUpdateListener);
         }
     }
 
     public unwatchList(policyList: PolicyList): void {
         this.policyLists = this.policyLists.filter(list => list.roomId !== policyList.roomId);
         this.accessControlUnit.unwatchList(policyList);
+        policyList.off('PolicyList.update', this.listUpdateListener)
     }
 
     /**
@@ -248,15 +257,13 @@ export class ProtectedRoomsSet {
     }
 
     /**
-     * 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.
+     * Updates all protected rooms with those any changes that have been made to a policy list.
+     * Does not fail if there are errors updating the room, these are reported to the management room.
+     * Do not use directly as a listener, use `this.listUpdateListener`.
      * @param policyList The `PolicyList` 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 syncWithPolicyList(policyList: PolicyList): Promise<void> {
-        // this bit can move away into a listener.
-        const changes = await policyList.updateList();
-
+    private async syncWithUpdatedPolicyList(policyList: PolicyList, changes: ListRuleChange[]): Promise<void> {
         let hadErrors = false;
         const [aclErrors, banErrors] = await Promise.all([
             this.applyServerAcls(this.policyLists, this.protectedRoomsByActivity()),

src/models/PolicyList.ts

@@ -56,9 +56,6 @@ declare interface PolicyList {
     // PolicyList.update is emitted when the PolicyList has pulled new rules from Matrix and informs listeners of any changes.
     on(event: 'PolicyList.update', listener: (list: PolicyList, changes: ListRuleChange[]) => void): this
     emit(event: 'PolicyList.update', list: PolicyList, changes: ListRuleChange[]): boolean
-    // PolicyList.batch is emitted when the PolicyList has created a batch from the events provided by `updateForEvent`.
-    on(event: 'PolicyList.batch', listener: (list: PolicyList) => void): this
-    emit(event: 'PolicyList.batch', list: PolicyList): boolean
 }
 
 /**
@@ -476,7 +473,8 @@ export default PolicyList;
 
 /**
  * Helper class that emits a batch event on a `PolicyList` when it has made a batch
- * out of the events given to `addToBatch`.
+ * out of the Matrix events given to `addToBatch` via `updateForEvent`.
+ * The `UpdateBatcher` will then call `list.update()` on the associated `PolicyList` once it has finished batching events.
  */
 class UpdateBatcher {
     // Whether we are waiting for more events to form a batch.
@@ -510,7 +508,8 @@ class UpdateBatcher {
             await new Promise(resolve => setTimeout(resolve, this.waitPeriodMS));
         } while ((Date.now() - start) < this.maxWaitMS && this.latestEventId !== eventId)
         this.reset();
-        this.banList.emit('PolicyList.batch', this.banList);
+        // batching finished, update the associated list.
+        await this.banList.updateList();
     }
 
     /**