Access Control Unit (#378)

The ACL unit allows you to combine an policy lists and conveniently test users and servers against them.
The main motivation for this work is provide access control on who can provision and continue to use mjolnir instances in the appservice component.
We include a new recommendation type org.matrix.mjolnir.allow which can be used with user and server entity types to create allow lists.
We have also replaced the destructing of policy lists in applyServerACL and applyMemberBans (in ProtectedRooms.ts) with calls to the AccessControlUnit.
Adding commands to add/remove allowed entities is not something i want to do at the moment.

src/ProtectedRooms.ts

@@ -18,10 +18,10 @@ import { LogLevel, LogService, MatrixClient, MatrixGlob, Permalinks, UserID } fr
 import { IConfig } from "./config";
 import ErrorCache, { ERROR_KIND_FATAL, ERROR_KIND_PERMISSION } from "./ErrorCache";
 import ManagementRoomOutput from "./ManagementRoomOutput";
+import AccessControlUnit, { Access } from "./models/AccessControlUnit";
 import { RULE_ROOM, RULE_SERVER, RULE_USER } from "./models/ListRule";
 import PolicyList, { ListRuleChange } from "./models/PolicyList";
 import { RoomUpdateError } from "./models/RoomUpdateError";
-import { ServerAcl } from "./models/ServerAcl";
 import { ProtectionManager } from "./protections/ProtectionManager";
 import { EventRedactionQueue, RedactUserInRoom } from "./queues/EventRedactionQueue";
 import { ProtectedRoomActivityTracker } from "./queues/ProtectedRoomActivityTracker";
@@ -82,6 +82,11 @@ export class ProtectedRooms {
      */
     private aclChain: Promise<void> = Promise.resolve();
 
+    /**
+     * A utility to test the access that users have in the set of protected rooms according to the policies of the watched lists.
+     */
+    private readonly accessControlUnit = new AccessControlUnit([]);
+
     constructor(
         private readonly client: MatrixClient,
         private readonly clientUserId: string,
@@ -136,11 +141,13 @@ export class ProtectedRooms {
     public watchList(policyList: PolicyList): void {
         if (!this.policyLists.includes(policyList)) {
             this.policyLists.push(policyList);
+            this.accessControlUnit.watchList(policyList);
         }
     }
 
     public unwatchList(policyList: PolicyList): void {
         this.policyLists = this.policyLists.filter(list => list.roomId !== policyList.roomId);
+        this.accessControlUnit.unwatchList(policyList);
     }
 
     /**
@@ -182,7 +189,7 @@ export class ProtectedRooms {
             // we cannot eagerly ban users (that is to ban them when they have never been a member)
             // as they can be force joined to a room they might not have known existed.
             // Only apply bans and then redactions in the room we are currently looking at.
-            const banErrors = await this.applyUserBans(this.policyLists, [roomId]);
+            const banErrors = await this.applyUserBans([roomId]);
             const redactionErrors = await this.processRedactionQueue(roomId);
             await this.printActionResult(banErrors);
             await this.printActionResult(redactionErrors);
@@ -202,7 +209,7 @@ export class ProtectedRooms {
         let hadErrors = false;
         const [aclErrors, banErrors] = await Promise.all([
             this.applyServerAcls(this.policyLists, this.protectedRoomsByActivity()),
-            this.applyUserBans(this.policyLists, this.protectedRoomsByActivity())
+            this.applyUserBans(this.protectedRoomsByActivity())
         ]);
         const redactionErrors = await this.processRedactionQueue();
         hadErrors = hadErrors || await this.printActionResult(aclErrors, "Errors updating server ACLs:");
@@ -250,7 +257,7 @@ export class ProtectedRooms {
         let hadErrors = false;
         const [aclErrors, banErrors] = await Promise.all([
             this.applyServerAcls(this.policyLists, this.protectedRoomsByActivity()),
-            this.applyUserBans(this.policyLists, this.protectedRoomsByActivity())
+            this.applyUserBans(this.protectedRoomsByActivity())
         ]);
         const redactionErrors = await this.processRedactionQueue();
         hadErrors = hadErrors || await this.printActionResult(aclErrors, "Errors updating server ACLs:");
@@ -293,19 +300,9 @@ export class ProtectedRooms {
         const serverName: string = new UserID(await this.client.getUserId()).domain;
 
         // Construct a server ACL first
-        const acl = new ServerAcl(serverName).denyIpAddresses().allowServer("*");
+        const acl = this.accessControlUnit.compileServerAcl(serverName);
-        for (const list of lists) {
-            for (const rule of list.serverRules) {
-                acl.denyServer(rule.entity);
-            }
-        }
-
         const finalAcl = acl.safeAclContent();
 
-        if (finalAcl.deny.length !== acl.literalAclContent().deny.length) {
-            this.managementRoomOutput.logMessage(LogLevel.WARN, "ApplyAcl", `Mjölnir has detected and removed an ACL that would exclude itself. Please check the ACL lists.`);
-        }
-
         if (this.config.verboseLogging) {
             // We specifically use sendNotice to avoid having to escape HTML
             await this.client.sendNotice(this.managementRoomId, `Constructed server ACL:\n${JSON.stringify(finalAcl, null, 2)}`);
@@ -346,11 +343,10 @@ export class ProtectedRooms {
     /**
     * Applies the member bans represented by the ban lists to the provided rooms, returning the
      * room IDs that could not be updated and their error.
-     * @param {PolicyList[]} lists The lists to determine bans from.
      * @param {string[]} roomIds The room IDs to apply the bans in.
      * @param {Mjolnir} mjolnir The Mjolnir client to apply the bans with.
      */
-    private async applyUserBans(lists: PolicyList[], roomIds: string[]): Promise<RoomUpdateError[]> {
+    private async applyUserBans(roomIds: string[]): Promise<RoomUpdateError[]> {
         // We can only ban people who are not already banned, and who match the rules.
         const errors: RoomUpdateError[] = [];
         for (const roomId of roomIds) {
@@ -377,29 +373,21 @@ export class ProtectedRooms {
                         continue; // user already banned
                     }
 
-                    let banned = false;
+                    // We don't want to ban people based on server ACL as this would flood the room with bans.
-                    for (const list of lists) {
+                    const memberAccess = this.accessControlUnit.getAccessForUser(member.userId, "IGNORE_SERVER");
-                        for (const userRule of list.userRules) {
+                    if (memberAccess.outcome === Access.Banned) {
-                            if (userRule.isMatch(member.userId)) {
+                        const reason = memberAccess.rule ? memberAccess.rule.reason : '<no reason supplied>';
-                                // User needs to be banned
-
                         // We specifically use sendNotice to avoid having to escape HTML
-                                await this.managementRoomOutput.logMessage(LogLevel.INFO, "ApplyBan", `Banning ${member.userId} in ${roomId} for: ${userRule.reason}`, roomId);
+                        await this.managementRoomOutput.logMessage(LogLevel.INFO, "ApplyBan", `Banning ${member.userId} in ${roomId} for: ${reason}`, roomId);
 
                         if (!this.config.noop) {
-                                    await this.client.banUser(member.userId, roomId, userRule.reason);
+                            await this.client.banUser(member.userId, roomId, memberAccess.rule!.reason);
-                                    if (this.automaticRedactGlobs.find(g => g.test(userRule.reason.toLowerCase()))) {
+                            if (this.automaticRedactGlobs.find(g => g.test(reason.toLowerCase()))) {
                                 this.redactUser(member.userId, roomId);
                             }
                         } else {
                             await this.managementRoomOutput.logMessage(LogLevel.WARN, "ApplyBan", `Tried to ban ${member.userId} in ${roomId} but Mjolnir is running in no-op mode`, roomId);
                         }
-
-                                banned = true;
-                                break;
-                            }
-                        }
-                        if (banned) break;
                     }
                 }
             } catch (e) {

src/models/AccessControlUnit.ts

@@ -0,0 +1,314 @@
+/*
+Copyright 2019-2022 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import PolicyList, { ChangeType, ListRuleChange } from "./PolicyList";
+import { EntityType, ListRule, Recommendation, RULE_SERVER, RULE_USER } from "./ListRule";
+import { LogService, UserID } from "matrix-bot-sdk";
+import { ServerAcl } from "./ServerAcl";
+
+/**
+ * The ListRuleCache is a cache for all the rules in a set of lists for a specific entity type and recommendation.
+ * The cache can then be used to quickly test against all the rules for that specific entity/recommendation.
+ * E.g. The cache can be used for all the m.ban rules for users in a set of lists to conveniently test members of a room.
+ * While some effort has been made to optimize the testing of entities, the main purpose of this class is to stop
+ * ad-hoc destructuring of policy lists to test rules against entities.
+ *
+ * Note: This cache should not be used to unban or introspect about the state of `PolicyLists`, for this
+ * see `PolicyList.unban` and `PolicyList.rulesMatchingEntity`, as these will make sure to account
+ * for unnormalized entity types.
+ */
+class ListRuleCache {
+    /**
+     * Glob rules always have to be scanned against every entity.
+     */
+    private readonly globRules: Map<string/** The entity that the rules specify */, ListRule[]> = new Map();
+    /**
+     * This table allows us to skip matching an entity against every literal.
+     */
+    private readonly literalRules: Map<string/* the string literal */, ListRule[]/* the rules matching this literal */> = new Map();
+    private readonly listUpdateListener: ((list: PolicyList, changes: ListRuleChange[]) => void);
+
+    constructor(
+        /**
+         * The entity type that this cache is for e.g. RULE_USER.
+         */
+        public readonly entityType: EntityType,
+        /**
+         * The recommendation that this cache is for e.g. m.ban (RECOMMENDATION_BAN).
+         */
+        public readonly recommendation: Recommendation,
+    ) {
+        this.listUpdateListener = (list: PolicyList, changes: ListRuleChange[]) => this.updateCache(changes);
+    }
+
+    /**
+     * Test the entitiy for the first matching rule out of all the watched lists.
+     * @param entity e.g. an mxid for a user, the server name for a server.
+     * @returns A single `ListRule` matching the entity.
+     */
+    public getAnyRuleForEntity(entity: string): ListRule|null {
+        const literalRule = this.literalRules.get(entity);
+        if (literalRule !== undefined) {
+            return literalRule[0];
+        }
+        for (const rule of this.globRules.values()) {
+            if (rule[0].isMatch(entity)) {
+                return rule[0];
+            }
+        }
+        return null;
+    }
+
+    /**
+     * Watch a list and add all its rules (and future rules) to the cache.
+     * Will automatically update with the list.
+     * @param list A PolicyList.
+     */
+    public watchList(list: PolicyList): void {
+        list.on('PolicyList.update', this.listUpdateListener);
+        const rules = list.rulesOfKind(this.entityType, this.recommendation);
+        rules.forEach(this.internRule, this);
+    }
+
+    /**
+     * Unwatch a list and remove all of its rules from the cache.
+     * Will stop updating the cache from this list.
+     * @param list A PolicyList.
+     */
+    public unwatchList(list: PolicyList): void {
+        list.removeListener('PolicyList.update', this.listUpdateListener);
+        const rules = list.rulesOfKind(this.entityType, this.recommendation);
+        rules.forEach(this.uninternRule, this);
+    }
+
+    /**
+     * @returns True when there are no rules in the cache.
+     */
+    public isEmpty(): boolean {
+        return this.globRules.size + this.literalRules.size === 0;
+    }
+
+    /**
+     * Returns all the rules in the cache, without duplicates from different lists.
+     */
+    public get allRules(): ListRule[] {
+        return [...this.literalRules.values(), ...this.globRules.values()].map(rules => rules[0]);
+    }
+
+    /**
+     * Remove a rule from the cache as it is now invalid. e.g. it was removed from a policy list.
+     * @param rule The rule to remove.
+     */
+    private uninternRule(rule: ListRule) {
+        /**
+         * Remove a rule from the map, there may be rules from different lists in the cache.
+         * We don't want to invalidate those.
+         * @param map A map of entities to rules.
+         */
+        const removeRuleFromMap = (map: Map<string, ListRule[]>) => {
+            const entry = map.get(rule.entity);
+            if (entry !== undefined) {
+                const newEntry = entry.filter(internedRule => internedRule.sourceEvent.event_id !== rule.sourceEvent.event_id);
+                if (newEntry.length === 0) {
+                    map.delete(rule.entity);
+                } else {
+                    map.set(rule.entity, newEntry);
+                }
+            }
+        };
+        if (rule.isGlob()) {
+            removeRuleFromMap(this.globRules);
+        } else {
+            removeRuleFromMap(this.literalRules);
+        }
+    }
+
+    /**
+     * Add a rule to the cache e.g. it was added to a policy list.
+     * @param rule The rule to add.
+     */
+    private internRule(rule: ListRule) {
+        /**
+         * Add a rule to the map, there might be duplicates of this rule in other lists.
+         * @param map A map of entities to rules.
+         */
+        const addRuleToMap = (map: Map<string, ListRule[]>) => {
+            const entry = map.get(rule.entity);
+            if (entry !== undefined) {
+                entry.push(rule);
+            } else {
+                map.set(rule.entity, [rule]);
+            }
+        }
+        if (rule.isGlob()) {
+            addRuleToMap(this.globRules);
+        } else {
+            addRuleToMap(this.literalRules);
+        }
+    }
+
+    /**
+     * Update the cache for a single `ListRuleChange`.
+     * @param change The change made to a rule that was present in the policy list.
+     */
+    private updateCacheForChange(change: ListRuleChange): void {
+        if (change.rule.kind !== this.entityType || change.rule.recommendation !== this.recommendation) {
+            return;
+        }
+        switch (change.changeType) {
+            case ChangeType.Added:
+            case ChangeType.Modified:
+                this.internRule(change.rule);
+                break;
+            case ChangeType.Removed:
+                this.uninternRule(change.rule);
+                break;
+            default:
+                throw new TypeError(`Uknown ListRule change type: ${change.changeType}`);
+        }
+    }
+
+    /**
+     * Update the cache for a change in a policy list.
+     * @param changes The changes that were made to list rules since the last update to this policy list.
+     */
+    private updateCache(changes: ListRuleChange[]) {
+        changes.forEach(this.updateCacheForChange, this);
+    }
+}
+
+export enum Access {
+    /// The entity was explicitly banned by a policy list.
+    Banned,
+    /// The entity did not match any allow rule.
+    NotAllowed,
+    /// The user was allowed and didn't match any ban.
+    Allowed,
+}
+
+/**
+ * A description of the access an entity has.
+ * If the access is `Banned`, then a single rule that bans the entity will be included.
+ */
+export interface EntityAccess {
+    readonly outcome: Access,
+    readonly rule?: ListRule,
+}
+
+/**
+ * This allows us to work out the access an entity has to some thing based on a set of watched/unwatched lists.
+ */
+export default class AccessControlUnit {
+    private readonly userBans = new ListRuleCache(RULE_USER, Recommendation.Ban);
+    private readonly serverBans = new ListRuleCache(RULE_SERVER, Recommendation.Ban);
+    private readonly userAllows = new ListRuleCache(RULE_USER, Recommendation.Allow);
+    private readonly serverAllows = new ListRuleCache(RULE_SERVER, Recommendation.Allow);
+    private readonly caches = [this.userBans, this.serverBans, this.userAllows, this.serverAllows]
+
+    constructor(policyLists: PolicyList[]) {
+        policyLists.forEach(this.watchList, this);
+    }
+
+    public watchList(list: PolicyList) {
+        for (const cache of this.caches) {
+            cache.watchList(list);
+        }
+    }
+
+    public unwatchList(list: PolicyList) {
+        for (const cache of this.caches) {
+            cache.watchList(list);
+        }
+    }
+
+    /**
+     * Test whether the server is allowed by the ACL unit.
+     * @param domain The server name to test.
+     * @returns A description of the access that the server has.
+     */
+    public getAccessForServer(domain: string): EntityAccess {
+        return this.getAccessForEntity(domain, this.serverAllows, this.serverBans);
+    }
+
+    /**
+     * Get the level of access the user has for the ACL unit.
+     * @param mxid The user id to test.
+     * @param policy Whether to check the server part of the user id against server rules.
+     * @returns A description of the access that the user has.
+     */
+    public getAccessForUser(mxid: string, policy: "CHECK_SERVER" | "IGNORE_SERVER"): EntityAccess {
+        const userAccess = this.getAccessForEntity(mxid, this.userAllows, this.userBans);
+        if (userAccess.outcome === Access.Allowed) {
+            if (policy === "IGNORE_SERVER") {
+                return userAccess;
+            } else {
+                const userId = new UserID(mxid);
+                return this.getAccessForServer(userId.domain);
+            }
+        } else {
+            return userAccess;
+        }
+    }
+
+    private getAccessForEntity(entity: string, allowCache: ListRuleCache, bannedCache: ListRuleCache): EntityAccess {
+        // Check if the entity is explicitly allowed.
+        // We have to infer that a rule exists for '*' if the allowCache is empty, otherwise you brick the ACL.
+        const allowRule = allowCache.getAnyRuleForEntity(entity);
+        if (allowRule === null && !allowCache.isEmpty()) {
+            return { outcome: Access.NotAllowed }
+        }
+        // Now check if the entity is banned.
+        const banRule = bannedCache.getAnyRuleForEntity(entity);
+        if (banRule !== null) {
+            return { outcome: Access.Banned, rule: banRule };
+        }
+        // If they got to this point, they're allowed!!
+        return { outcome: Access.Allowed };
+    }
+
+    /**
+     * Create a ServerAcl instance from the rules contained in this unit.
+     * @param serverName The name of the server that you are operating from, used to ensure you cannot brick yourself.
+     * @returns A new `ServerAcl` instance with deny and allow entries created from the rules in this unit.
+     */
+    public compileServerAcl(serverName: string): ServerAcl {
+        const acl = new ServerAcl(serverName).denyIpAddresses();
+        const allowedServers = this.serverAllows.allRules;
+        // Allowed servers (allow).
+        if (allowedServers.length === 0) {
+            acl.allowServer('*');
+        } else {
+            for (const rule of allowedServers) {
+                acl.allowServer(rule.entity);
+            }
+            if (this.getAccessForServer(serverName).outcome === Access.NotAllowed) {
+                acl.allowServer(serverName);
+                LogService.warn('AccessControlUnit', `The server ${serverName} we are operating from was not on the allowed when constructing the server ACL, so it will be injected it into the server acl. Please check the ACL lists.`)
+            }
+        }
+        // Banned servers (deny).
+        for (const rule of this.serverBans.allRules) {
+            if (rule.isMatch(serverName)) {
+                LogService.warn('AccessControlUnit', `The server ${serverName} we are operating from was found to be banned by ${rule.entity} by a rule from the event: ${rule.sourceEvent.event_id}, `
+                    + 'while constructing a server acl. Ignoring the rule. Please check the ACL lists.'
+                );
+            } else {
+                acl.denyServer(rule.entity);
+            }
+        }
+        return acl;
+    }
+}

src/models/ListRule.ts

@@ -55,6 +55,12 @@ export enum Recommendation {
     /// is considered absolutely absolutely perfect by whoever issued
     /// this ListRule.
     Opinion = "org.matrix.msc3845.opinion",
+
+    /**
+     * This is a rule that recommends allowing a user to participate.
+     * Used for the construction of allow lists.
+     */
+    Allow = "org.matrix.mjolnir.allow",
 }
 
 /**
@@ -75,6 +81,11 @@ const RECOMMENDATION_OPINION_VARIANTS: string[] = [
     Recommendation.Opinion
 ];
 
+const RECOMMENDATION_ALLOW_VARIANTS: string[] = [
+    // Unstable
+    Recommendation.Allow
+]
+
 export const OPINION_MIN = -100;
 export const OPINION_MAX = +100;
 
@@ -125,6 +136,13 @@ export abstract class ListRule {
         return this.glob.test(entity);
     }
 
+    /**
+     * @returns Whether the entity in he rule represents a Matrix glob (and not a literal).
+     */
+    public isGlob(): boolean {
+        return /[*?]/.test(this.entity);
+    }
+
     /**
      * Validate and parse an event into a ListRule.
      *
@@ -173,6 +191,8 @@ export abstract class ListRule {
                 return null;
             }
             return new ListRuleOpinion(event, entity, reason, kind, opinion);
+        } else if (RECOMMENDATION_ALLOW_VARIANTS.includes(recommendation)) {
+            return new ListRuleAllow(event, entity, reason, kind);
         } else {
             // As long as the `recommendation` is defined, we assume
             // that the rule is correct, just unknown.
@@ -207,6 +227,32 @@ export class ListRuleBan extends ListRule {
     }
 }
 
+/**
+ * A rule representing an "allow".
+ */
+ export class ListRuleAllow 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.
+         */
+        entity: string,
+        /**
+         * A human-readable reason for this rule, for audit purposes.
+         */
+        reason: string,
+        /**
+         * The type of entity for this rule, e.g. user, server domain, etc.
+         */
+        kind: EntityType,
+    ) {
+        super(sourceEvent, entity, reason, kind, Recommendation.Allow)
+    }
+}
+
 /**
  * A rule representing an "opinion"
  */

src/models/PolicyList.ts

@@ -86,6 +86,14 @@ class PolicyList extends EventEmitter {
     // 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 */>();
 
+    /**
+     * This is used to annotate state events we store with the rule they are associated with.
+     * If we refactor this, it is important to also refactor any listeners to 'PolicyList.update'
+     * which may assume `ListRule`s that are removed will be identital (Object.is) to when they were added.
+     * If you are adding new listeners, you should check the source event_id of the rule.
+     */
+    private static readonly EVENT_RULE_ANNOTATION_KEY = 'org.matrix.mjolnir.annotation.rule';
+
     /**
      * Construct a PolicyList, does not synchronize with the room.
      * @param roomId The id of the policy room, i.e. a room containing MSC2313 policies.
@@ -134,19 +142,21 @@ class PolicyList extends EventEmitter {
     /**
      * Return all the active rules of a given kind.
      * @param kind e.g. RULE_SERVER (m.policy.rule.server). Rule types are always normalised when they are interned into the PolicyList.
+     * @param recommendation A specific recommendation to filter for e.g. `m.ban`. Please remember recommendation varients are normalized.
      * @returns The active ListRules for the ban list of that kind.
      */
-    private rulesOfKind(kind: string): ListRule[] {
+    public rulesOfKind(kind: string, recommendation?: Recommendation): ListRule[] {
         const rules: ListRule[] = []
         const stateKeyMap = this.state.get(kind);
         if (stateKeyMap) {
             for (const event of stateKeyMap.values()) {
-                const rule = event?.unsigned?.rule;
+                const rule = event[PolicyList.EVENT_RULE_ANNOTATION_KEY];
-                // README! If you are refactoring this and/or introducing a mechanism to return the list of rules,
+                if (rule && rule.kind === kind) {
-                // please make sure that you *only* return rules with `m.ban` or create a different method
+                    if (recommendation === undefined) {
-                // (we don't want to accidentally ban entities).
-                if (rule && rule.kind === kind && rule.recommendation === Recommendation.Ban) {
                         rules.push(rule);
+                    } else if (rule.recommendation === recommendation) {
+                        rules.push(rule);
+                    }
                 }
             }
         }
@@ -353,10 +363,10 @@ class PolicyList extends EventEmitter {
 
             // 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) {
+            if (changeType === ChangeType.Removed && previousState?.[PolicyList.EVENT_RULE_ANNOTATION_KEY]) {
                 const sender = event.unsigned['redacted_because'] ? event.unsigned['redacted_because']['sender'] : event.sender;
                 changes.push({
-                    changeType, event, sender, rule: previousState.unsigned.rule,
+                    changeType, event, sender, rule: previousState[PolicyList.EVENT_RULE_ANNOTATION_KEY],
                     ...previousState ? { previousState } : {}
                 });
                 // Event has no content and cannot be parsed as a ListRule.
@@ -368,7 +378,7 @@ class PolicyList extends EventEmitter {
                 // Invalid/unknown rule, just skip it.
                 continue;
             }
-            event.unsigned.rule = rule;
+            event[PolicyList.EVENT_RULE_ANNOTATION_KEY] = rule;
             if (changeType) {
                 changes.push({ rule, changeType, event, sender: event.sender, ...previousState ? { previousState } : {} });
             }

test/integration/banListTest.ts

@@ -6,7 +6,8 @@ import { ServerAcl } from "../../src/models/ServerAcl";
 import { getFirstReaction } from "./commands/commandUtils";
 import { getMessagesByUserIn } from "../../src/utils";
 import { Mjolnir } from "../../src/Mjolnir";
-import { ALL_RULE_TYPES, RULE_SERVER, RULE_USER, SERVER_RULE_TYPES } from "../../src/models/ListRule";
+import { ALL_RULE_TYPES, Recommendation, RULE_SERVER, RULE_USER, SERVER_RULE_TYPES } from "../../src/models/ListRule";
+import AccessControlUnit, { Access, EntityAccess } from "../../src/models/AccessControlUnit";
 
 /**
  * Create a policy rule in a policy room.
@@ -26,6 +27,19 @@ async function createPolicyRule(client: MatrixClient, policyRoomId: string, poli
     });
 }
 
+/**
+ * Remove a policy rule from a list.
+ * @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 stateKey The key for the rule.
+ * @returns The event id of the void rule that was created to override the old one.
+ */
+async function removePolicyRule(client: MatrixClient, policyRoomId: string, policyType: string, entity: string, stateKey = `rule:${entity}`) {
+    return await client.sendStateEvent(policyRoomId, policyType, stateKey, {});
+}
+
 describe("Test: Updating the PolicyList", function() {
     it("Calculates what has changed correctly.", async function() {
         this.timeout(10000);
@@ -191,28 +205,13 @@ describe("Test: Updating the PolicyList", function() {
     })
 });
 
-describe('Test: We do not respond to recommendations other than m.ban in the PolicyList', function() {
-    it('Will not respond to a rule that has a different recommendation to m.ban (or the unstable equivalent).', async function() {
-        const mjolnir: Mjolnir = this.mjolnir!
-        const banListId = await mjolnir.client.createRoom();
-        const banList = new PolicyList(banListId, banListId, mjolnir.client);
-        await createPolicyRule(mjolnir.client, banListId, RULE_SERVER, 'exmaple.org', '', { recommendation: 'something that is not m.ban' });
-        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.client.getUserId());
-        // We really don't want things that aren't m.ban to end up being accessible in these APIs.
-        assert.equal(banList.serverRules.length, 0, `We should have an empty serverRules, got ${JSON.stringify(banList.serverRules)}`);
-        assert.equal(banList.allRules.length, 0, `We should have an empty allRules, got ${JSON.stringify(banList.allRules)}`);
-    })
-})
-
 describe('Test: We will not be able to ban ourselves via ACL.', function() {
     it('We do not ban ourselves when we put ourselves into the policy list.', async function() {
         const mjolnir: Mjolnir = this.mjolnir
         const serverName = new UserID(await mjolnir.client.getUserId()).domain;
         const banListId = await mjolnir.client.createRoom();
         const banList = new PolicyList(banListId, banListId, mjolnir.client);
+        const aclUnit = new AccessControlUnit([banList]);
         await createPolicyRule(mjolnir.client, banListId, RULE_SERVER, serverName, '');
         await createPolicyRule(mjolnir.client, banListId, RULE_SERVER, 'evil.com', '');
         await createPolicyRule(mjolnir.client, banListId, RULE_SERVER, '*', '');
@@ -224,6 +223,10 @@ describe('Test: We will not be able to ban ourselves via ACL.', function() {
         changes.forEach(change => acl.denyServer(change.rule.entity));
         assert.equal(acl.safeAclContent().deny.length, 1);
         assert.equal(acl.literalAclContent().deny.length, 3);
+
+        const aclUnitAcl = aclUnit.compileServerAcl(serverName);
+        assert.equal(aclUnitAcl.literalAclContent().deny.length, 1);
+
     })
 })
 
@@ -457,3 +460,107 @@ describe('Test: should apply bans to the most recently active rooms first', func
         }
     })
 })
+
+/**
+ * Assert that the AccessUnitOutcome entity test has the right access.
+ * @param expected The Access we expect the entity to have, See Access.
+ * @param query The result of a test on AccessControlUnit e.g. `unit.getAccessForUser'@meow:localhost')`
+ * @param message A message for the console if the entity doesn't have the expected access.
+ */
+function assertAccess(expected: Access, query: EntityAccess, message?: string) {
+    assert.equal(query.outcome, expected, message);
+}
+
+describe('Test: AccessControlUnit interaction with policy lists.', function() {
+    it('The AccessControlUnit correctly reflects the policies that have been set in its watched lists.', async function() {
+        const mjolnir: Mjolnir = this.mjolnir!
+        const policyListId = await mjolnir.client.createRoom();
+        const policyList = new PolicyList(policyListId, Permalinks.forRoom(policyListId), mjolnir.client);
+        const aclUnit = new AccessControlUnit([policyList]);
+        assertAccess(Access.Allowed, aclUnit.getAccessForUser('@anyone:anywhere.example.com', "CHECK_SERVER"), 'Empty lists should implicitly allow.');
+
+        await createPolicyRule(mjolnir.client, policyListId, RULE_SERVER, 'matrix.org', '', { recommendation: Recommendation.Allow });
+        // we want to imagine that the banned server was never taken off the allow after being banned.
+        await createPolicyRule(mjolnir.client, policyListId, RULE_SERVER, 'bad.example.com', '', { recommendation: Recommendation.Allow }, 'something-else');
+        await createPolicyRule(mjolnir.client, policyListId, RULE_SERVER, 'bad.example.com', '', { recommendation: Recommendation.Ban });
+        await createPolicyRule(mjolnir.client, policyListId, RULE_SERVER, '*.ddns.example.com', '', { recommendation: Recommendation.Ban });
+
+        await policyList.updateList();
+
+        assertAccess(Access.Allowed, aclUnit.getAccessForServer('matrix.org'));
+        assertAccess(Access.Allowed, aclUnit.getAccessForUser('@someone:matrix.org', "CHECK_SERVER"));
+        assertAccess(Access.NotAllowed, aclUnit.getAccessForServer('anywhere.else.example.com'));
+        assertAccess(Access.NotAllowed, aclUnit.getAccessForUser('@anyone:anywhere.else.example.com', "CHECK_SERVER"));
+        assertAccess(Access.Banned, aclUnit.getAccessForServer('bad.example.com'));
+        assertAccess(Access.Banned, aclUnit.getAccessForUser('@anyone:bad.example.com', "CHECK_SERVER"));
+        // They're not allowed in the first place, never mind that they are also banned.
+        assertAccess(Access.NotAllowed, aclUnit.getAccessForServer('meow.ddns.example.com'));
+        assertAccess(Access.NotAllowed, aclUnit.getAccessForUser('@anyone:meow.ddns.example.com', "CHECK_SERVER"));
+
+        assertAccess(Access.Allowed, aclUnit.getAccessForUser('@spam:matrix.org', "CHECK_SERVER"));
+        await createPolicyRule(mjolnir.client, policyListId, RULE_USER, '@spam:matrix.org', '', { recommendation: Recommendation.Ban });
+        await policyList.updateList();
+        assertAccess(Access.Banned, aclUnit.getAccessForUser('@spam:matrix.org', "CHECK_SERVER"));
+        assertAccess(Access.Allowed, aclUnit.getAccessForUser('@someone:matrix.org', "CHECK_SERVER"));
+
+        // protect a room and check that only bad.example.com, *.ddns.example.com are in the deny ACL and not matrix.org
+        await mjolnir.watchList(policyList.roomRef);
+        const protectedRoom = await mjolnir.client.createRoom();
+        await mjolnir.protectedRoomsTracker.addProtectedRoom(protectedRoom);
+        await mjolnir.protectedRoomsTracker.syncLists();
+        const roomAcl = await mjolnir.client.getRoomStateEvent(protectedRoom, "m.room.server_acl", "");
+        assert.equal(roomAcl?.deny?.length ?? 0, 2, 'There should be two entries in the deny ACL.');
+        for (const serverGlob of ["*.ddns.example.com", "bad.example.com"]) {
+            assert.equal((roomAcl?.deny ?? []).includes(serverGlob), true);
+        }
+        assert.equal(roomAcl.deny.includes("matrix.org"), false);
+        assert.equal(roomAcl.allow.includes("matrix.org"), true);
+
+        // Now we remove the rules and hope that everything functions noramally.
+        await removePolicyRule(mjolnir.client, policyListId, RULE_SERVER, 'matrix.org');
+        await removePolicyRule(mjolnir.client, policyListId, RULE_SERVER, 'bad.example.com', 'something-else');
+        await removePolicyRule(mjolnir.client, policyListId, RULE_SERVER, 'bad.example.com');
+        await removePolicyRule(mjolnir.client, policyListId, RULE_SERVER, '*.ddns.example.com');
+        await removePolicyRule(mjolnir.client, policyListId, RULE_USER, "@spam:matrix.org");
+        const changes = await policyList.updateList()
+        await mjolnir.protectedRoomsTracker.syncLists();
+
+        assert.equal(changes.length, 5, "The rules should have correctly been removed");
+        assertAccess(Access.Allowed, aclUnit.getAccessForServer('matrix.org'));
+        assertAccess(Access.Allowed, aclUnit.getAccessForUser('@someone:matrix.org', "CHECK_SERVER"));
+        assertAccess(Access.Allowed, aclUnit.getAccessForServer('anywhere.else.example.com'));
+        assertAccess(Access.Allowed, aclUnit.getAccessForUser('@anyone:anywhere.else.example.com', "CHECK_SERVER"));
+        assertAccess(Access.Allowed, aclUnit.getAccessForServer('bad.example.com'));
+        assertAccess(Access.Allowed, aclUnit.getAccessForUser('@anyone:bad.example.com', "CHECK_SERVER"));
+        assertAccess(Access.Allowed, aclUnit.getAccessForServer('meow.ddns.example.com'));
+        assertAccess(Access.Allowed, aclUnit.getAccessForUser('@anyone:meow.ddns.example.com', "CHECK_SERVER"));
+        assertAccess(Access.Allowed, aclUnit.getAccessForUser('@spam:matrix.org', "CHECK_SERVER"));
+
+        const roomAclAfter = await mjolnir.client.getRoomStateEvent(protectedRoom, "m.room.server_acl", "");
+        assert.equal(roomAclAfter.deny?.length ?? 0, 0, 'There should be no entries in the deny ACL.');
+        assert.equal(roomAclAfter.allow?.length ?? 0, 1, 'There should be 1 entry in the allow ACL.');
+        assert.equal(roomAclAfter.allow.includes("*"), true);
+    })
+    it('removing a rule from a different list will not clobber anything.', async function() {
+        const mjolnir: Mjolnir = this.mjolnir!
+        const policyLists = await Promise.all([...Array(2).keys()].map(async _ => {
+            const policyListId = await mjolnir.client.createRoom();
+            return new PolicyList(policyListId, Permalinks.forRoom(policyListId), mjolnir.client);
+        }));
+        const banMeServer = 'banme.example.com';
+        const aclUnit = new AccessControlUnit(policyLists);
+        await Promise.all(policyLists.map(policyList => {
+            return createPolicyRule(mjolnir.client, policyList.roomId, RULE_SERVER, banMeServer, '', { recommendation: Recommendation.Ban })
+        }));
+        await Promise.all(policyLists.map(list => list.updateList()));
+        assertAccess(Access.Banned, aclUnit.getAccessForServer(banMeServer));
+
+        // remove the rule that bans `banme.example.com` from just one of the lists.
+        await removePolicyRule(mjolnir.client, policyLists[0].roomId, RULE_SERVER, banMeServer);
+        await Promise.all(policyLists.map(list => list.updateList()));
+        assertAccess(Access.Banned, aclUnit.getAccessForServer(banMeServer), "Should still be banned at this point.");
+        await removePolicyRule(mjolnir.client, policyLists[1].roomId, RULE_SERVER, banMeServer);
+        await Promise.all(policyLists.map(list => list.updateList()));
+        assertAccess(Access.Allowed, aclUnit.getAccessForServer(banMeServer), "Should not longer be any rules");
+    })
+})