diff --git a/src/models/BanList.ts b/src/models/BanList.ts index f4079ef..4be1c35 100644 --- a/src/models/BanList.ts +++ b/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_ROOM = "m.room.rule.room"; -export const RULE_SERVER = "m.room.rule.server"; +export const RULE_USER = "m.policy.rule.user"; +export const RULE_ROOM = "m.policy.rule.room"; +export const RULE_SERVER = "m.policy.rule.server"; -export const USER_RULE_TYPES = [RULE_USER, "org.matrix.mjolnir.rule.user"]; -export const ROOM_RULE_TYPES = [RULE_ROOM, "org.matrix.mjolnir.rule.room"]; -export const SERVER_RULE_TYPES = [RULE_SERVER, "org.matrix.mjolnir.rule.server"]; +// README! The order here matters for determining whether a type is obsolete, most recent should be first. +// These are the current and historical types for each type of rule which were used while MSC2313 was being developed +// 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 stateKey The state key e.g. entity:@bad:matrix.org + * @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) { @@ -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. - * @param stateKey The state key e.g. entity:@bad:matrix.org + * 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 { @@ -173,8 +178,33 @@ export default class BanList { continue; // invalid/unknown } - const previousState = this.getState(event['type'], event['state_key']); - this.setState(event['type'], event['state_key'], event); + 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; diff --git a/test/integration/banListTest.ts b/test/integration/banListTest.ts index dad9d5b..4eb6476 100644 --- a/test/integration/banListTest.ts +++ b/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.'); }) });