Use MSC2313 m.policy.rule.* for rules and always prefer these types.

The reason for doing this is because otherwise there may be duplicate
rules under different state types for the same entity.
This simplifies the process of modifying or invalidating rules affecting
an entity because the rule with the most recent type will always be
preferred.

src/models/BanList.ts

@@ -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";
@@ -94,8 +98,8 @@ export default class BanList {
 
     /**
      * Lookup the current rules cached for the list.
-     * @param stateType The event type e.g. m.room.rule.user.
+     * @param stateType The event type e.g. m.policy.rule.user.
-     * @param stateKey The state key e.g. entity:@bad:matrix.org
+     * @param stateKey The state key e.g. rule:@bad:matrix.org
      * @returns A state event if present or null.
      */
     private getState(stateType: string, stateKey: string) {
@@ -104,8 +108,9 @@ export default class BanList {
 
     /**
      * Store this state event as part of the active room state for this BanList (used to cache rules).
-     * @param stateType The event type e.g. m.room.rule.user.
+     * 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 stateKey The state key e.g. entity:@bad:matrix.org
+     * @param stateType The event type e.g. m.room.policy.user.
+     * @param stateKey The state key e.g. rule:@bad:matrix.org
      * @param event A state event to store.
      */
     private setState(stateType: string, stateKey: string, event: any): void {
@@ -173,8 +178,33 @@ export default class BanList {
                 continue; // invalid/unknown
             }
 
-            const previousState = this.getState(event['type'], event['state_key']);
+            const previousState = this.getState(kind, event['state_key']);
-            this.setState(event['type'], event['state_key'], event);
+
+            // 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;

test/integration/banListTest.ts

@@ -31,26 +31,38 @@ describe("Test: Updating the BanList", function () {
         const banList = new BanList(banListId, banListId, mjolnir);
         mjolnir.setUserPowerLevel(await moderator.getUserId(), banListId, 100);
 
+        assert.equal(banList.allRules.length, 0);
+
         // Test adding a new rule
         await createPolicyRule(mjolnir, banListId, RULE_USER, '@added:localhost:9999', '');
         let changes: ListRuleChange[] = await banList.updateList();
         assert.equal(changes.length, 1, 'There should only be one change');
         assert.equal(changes[0].changeType, ChangeType.Added);
         assert.equal(changes[0].sender, await mjolnir.getUserId());
+        assert.equal(banList.userRules.length, 1);
+        assert.equal(banList.allRules.length, 1);
 
         // Test modifiying a rule
         let originalEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', '');
         await banList.updateList();
-        let nextEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', 'modified reason');
+        let modifyingEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', 'modified reason');
         changes = await banList.updateList();
         assert.equal(changes.length, 1);
         assert.equal(changes[0].changeType, ChangeType.Modified);
         assert.equal(changes[0].previousState['event_id'], originalEventId, 'There should be a previous state event for a modified rule');
-        assert.equal(changes[0].event['event_id'], nextEventId);
+        assert.equal(changes[0].event['event_id'], modifyingEventId);
+        let modifyingAgainEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@modified:localhost:9999', 'modified again');
+        changes = await banList.updateList();
+        assert.equal(changes.length, 1);
+        assert.equal(changes[0].changeType, ChangeType.Modified);
+        assert.equal(changes[0].previousState['event_id'], modifyingEventId, 'There should be a previous state event for a modified rule');
+        assert.equal(changes[0].event['event_id'], modifyingAgainEventId);
+        assert.equal(banList.userRules.length, 2, 'There should be two rules, one for @modified:localhost:9999 and one for @added:localhost:9999');
 
         // Test redacting a rule
         const redactThis = await createPolicyRule(mjolnir, banListId, RULE_USER, '@redacted:localhost:9999', '');
         await banList.updateList();
+        assert.equal(banList.userRules.filter(r => r.entity === '@redacted:localhost:9999').length, 1);
         await mjolnir.redactEvent(banListId, redactThis);
         changes = await banList.updateList();
         assert.equal(changes.length, 1);
@@ -59,22 +71,108 @@ describe("Test: Updating the BanList", function () {
         assert.equal(Object.keys(changes[0].event['content']).length, 0, 'Should show the new version of the event with redacted content');
         assert.notEqual(Object.keys(changes[0].previousState['content']), 0, 'Should have a copy of the unredacted state');
         assert.notEqual(changes[0].rule, undefined, 'The previous rule should be present');
+        assert.equal(banList.userRules.filter(r => r.entity === '@redacted:localhost:9999').length, 0, 'The rule should be removed.');
 
         // Test soft redaction of a rule
         const softRedactedEntity = '@softredacted:localhost:9999'
         await createPolicyRule(mjolnir, banListId, RULE_USER, softRedactedEntity, '');
         await banList.updateList();
-        await mjolnir.sendStateEvent(banListId, RULE_USER, softRedactedEntity, {});
+        assert.equal(banList.userRules.filter(r => r.entity === softRedactedEntity).length, 1);
+        await mjolnir.sendStateEvent(banListId, RULE_USER, `rule:${softRedactedEntity}`, {});
         changes = await banList.updateList();
         assert.equal(changes.length, 1);
         assert.equal(changes[0].changeType, ChangeType.Removed);
         assert.equal(Object.keys(changes[0].event['content']).length, 0, 'Should show the new version of the event with redacted content');
         assert.notEqual(Object.keys(changes[0].previousState['content']), 0, 'Should have a copy of the unredacted state');
         assert.notEqual(changes[0].rule, undefined, 'The previous rule should be present');
+        assert.equal(banList.userRules.filter(r => r.entity === softRedactedEntity).length, 0, 'The rule should have been removed');
 
         // Now test a double soft redaction just to make sure stuff doesn't explode
-        await mjolnir.sendStateEvent(banListId, RULE_USER, softRedactedEntity, {});
+        await mjolnir.sendStateEvent(banListId, RULE_USER, `rule:${softRedactedEntity}`, {});
         changes = await banList.updateList();
         assert.equal(changes.length, 0, "It shouldn't detect a double soft redaction as a change, it should be seen as adding an invalid rule.");
+        assert.equal(banList.userRules.filter(r => r.entity === softRedactedEntity).length, 0, 'The rule should have been removed');
+
+        // Test that different (old) rule types will be modelled as the latest event type.
+        originalEventId = await createPolicyRule(mjolnir, banListId, 'org.matrix.mjolnir.rule.user', '@old:localhost:9999', '');
+        changes = await banList.updateList();
+        assert.equal(changes.length, 1);
+        assert.equal(changes[0].changeType, ChangeType.Added);
+        assert.equal(banList.userRules.filter(r => r.entity === '@old:localhost:9999').length, 1);
+        modifyingEventId = await createPolicyRule(mjolnir, banListId, 'm.room.rule.user', '@old:localhost:9999', 'modified reason');
+        changes = await banList.updateList();
+        assert.equal(changes.length, 1);
+        assert.equal(changes[0].changeType, ChangeType.Modified);
+        assert.equal(changes[0].event['event_id'], modifyingEventId);
+        assert.equal(changes[0].previousState['event_id'], originalEventId, 'There should be a previous state event for a modified rule');
+        assert.equal(banList.userRules.filter(r => r.entity === '@old:localhost:9999').length, 1);
+        modifyingAgainEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, '@old:localhost:9999', 'changes again');
+        changes = await banList.updateList();
+        assert.equal(changes.length, 1);
+        assert.equal(changes[0].changeType, ChangeType.Modified);
+        assert.equal(changes[0].event['event_id'], modifyingAgainEventId);
+        assert.equal(changes[0].previousState['event_id'], modifyingEventId, 'There should be a previous state event for a modified rule');
+        assert.equal(banList.userRules.filter(r => r.entity === '@old:localhost:9999').length, 1);
+    })
+    it("Will remove rules with old types when they are 'soft redacted' with a different but more recent event type.", async function () {
+        this.timeout(3000);
+        const mjolnir = config.RUNTIME.client!
+        const moderator = await newTestUser(false, "moderator");
+        const banListId = await mjolnir.createRoom({ invite: [await moderator.getUserId()]});
+        const banList = new BanList(banListId, banListId, mjolnir);
+        mjolnir.setUserPowerLevel(await moderator.getUserId(), banListId, 100);
+
+        const entity = '@old:localhost:9999';
+        let originalEventId = await createPolicyRule(mjolnir, banListId, 'm.room.rule.user', entity, '');
+        let changes = await banList.updateList();
+        assert.equal(changes.length, 1);
+        assert.equal(changes[0].changeType, ChangeType.Added);
+        assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 1, 'There should be a rule stored that we just added...')
+        let softRedactingEventId = await mjolnir.sendStateEvent(banListId, RULE_USER, `rule:${entity}`, {});
+        changes = await banList.updateList();
+        assert.equal(changes.length, 1);
+        assert.equal(changes[0].changeType, ChangeType.Removed);
+        assert.equal(changes[0].event['event_id'], softRedactingEventId);
+        assert.equal(changes[0].previousState['event_id'], originalEventId, 'There should be a previous state event for a modified rule');
+        assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 0, 'The rule should no longer be stored.');
+    })
+    it("A rule of the most recent type won't be deleted when an old rule is deleted for the same entity.", async function () {
+        this.timeout(3000);
+        const mjolnir = config.RUNTIME.client!
+        const moderator = await newTestUser(false, "moderator");
+        const banListId = await mjolnir.createRoom({ invite: [await moderator.getUserId()]});
+        const banList = new BanList(banListId, banListId, mjolnir);
+        mjolnir.setUserPowerLevel(await moderator.getUserId(), banListId, 100);
+
+        const entity = '@old:localhost:9999';
+        let originalEventId = await createPolicyRule(mjolnir, banListId, 'm.room.rule.user', entity, '');
+        let changes = await banList.updateList();
+        assert.equal(changes.length, 1);
+        assert.equal(changes[0].changeType, ChangeType.Added);
+        assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 1, 'There should be a rule stored that we just added...')
+        let updatedEventId = await createPolicyRule(mjolnir, banListId, RULE_USER, entity, '');
+        changes = await banList.updateList();
+        // If in the future you change this and it fails, it's really subjective whether this constitutes a modification, since the only thing that has changed
+        // is the rule type. The actual content is identical.
+        assert.equal(changes.length, 1);
+        assert.equal(changes[0].changeType, ChangeType.Modified);
+        assert.equal(changes[0].event['event_id'], updatedEventId);
+        assert.equal(changes[0].previousState['event_id'], originalEventId, 'There should be a previous state event for a modified rule');
+        assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 1, 'Only the latest version of the rule gets returned.');
+
+        // Now we delete the old version of the rule without consequence.
+        await mjolnir.sendStateEvent(banListId, 'm.room.rule.user', `rule:${entity}`, {});
+        changes = await banList.updateList();
+        assert.equal(changes.length, 0);
+        assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 1, 'The rule should still be active.');
+
+        // And we can still delete the new version of the rule.
+        let softRedactingEventId = await mjolnir.sendStateEvent(banListId, RULE_USER, `rule:${entity}`, {});
+        changes = await banList.updateList();
+        assert.equal(changes.length, 1);
+        assert.equal(changes[0].changeType, ChangeType.Removed);
+        assert.equal(changes[0].event['event_id'], softRedactingEventId);
+        assert.equal(changes[0].previousState['event_id'], updatedEventId, 'There should be a previous state event for a modified rule');
+        assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 0, 'The rule should no longer be stored.');
     })
 });