Skip to content

Commit

Permalink
Rework the banning and unbanning of entities in PolicyLists. (#345)
Browse files Browse the repository at this point in the history 
* 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
#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.
  • Loading branch information
@Gnuxie
Gnuxie authored Aug 19, 2022
1 parent 8bafa16 commit 4d5447c
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 60 deletions.
2 changes: 1 addition & 1 deletion src/Mjolnir.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down
28 changes: 5 additions & 23 deletions src/commands/ImportCommand.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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`);

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);
}
await list.banEntity(EntityType.RULE_USER, stateEvent['state_key'], reason);
importedRules++;
}
} else if (stateEvent['type'] === 'm.room.server_acl' && stateEvent['state_key'] === '') {
Expand All @@ -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 = {
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);
}
await list.banEntity(EntityType.RULE_SERVER, server, reason);
importedRules++;
}
}
Expand Down
11 changes: 2 additions & 9 deletions src/commands/UnbanBanCommand.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 = {
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 bits.list!.banEntity(bits.ruleType!, bits.entity, bits.reason);
await mjolnir.client.unstableApis.addReactionToEvent(roomId, event['event_id'], '✅');
}

Expand Down
37 changes: 30 additions & 7 deletions src/models/ListRule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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'];
Expand Down Expand Up @@ -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);
}
}
}
Expand All @@ -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.
*/
Expand All @@ -188,7 +203,7 @@ export class ListRuleBan extends ListRule {
*/
kind: EntityType,
) {
super(entity, reason, kind, Recommendation.Ban)
super(sourceEvent, entity, reason, kind, Recommendation.Ban)
}
}

Expand All @@ -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.
*/
Expand All @@ -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}`);
}
Expand All @@ -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.
*/
Expand All @@ -248,6 +271,6 @@ export class ListRuleUnknown extends ListRule {
*/
public readonly content: any,
) {
super(entity, reason, kind, null);
super(sourceEvent, entity, reason, kind, null);
}
}
83 changes: 70 additions & 13 deletions src/models/PolicyList.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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.
Expand All @@ -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:
Expand All @@ -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 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 false;
const sendNullState = async (stateType: string, stateKey: string) => {
const event_id = await this.client.sendStateEvent(this.roomId, stateType, stateKey, {});
this.updateForEvent(event_id);
}
await Promise.all(typesToRemove.map(stateType => this.client.sendStateEvent(this.roomId, stateType!, stateKey, {})));
return true;
const removeRule = async (rule: ListRule): Promise<void> => {
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;
}

/**
Expand Down Expand Up @@ -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) {
Expand All @@ -328,15 +374,26 @@ 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;
}

/**
* 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 {
this.batcher.addToBatch(event.event_id)
public updateForEvent(eventId: string): void {
if (this.stateByEventId.has(eventId) || this.batchedEvents.has(eventId)) {
return; // we already know about this event.
}
this.batcher.addToBatch(eventId);
this.batchedEvents.add(eventId);
}
}

Expand Down
15 changes: 8 additions & 7 deletions test/integration/banListTest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' }) {
return await client.sendStateEvent(policyRoomId, policyType, `rule:${entity}`, {
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, stateKey, {
entity,
reason,
...template,
Expand Down Expand Up @@ -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 newerBadServer = "new.evil.com"
const olderBadServer = "old.evil.example"
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.
assert.equal(banList.allRules.length, 2);
// rules are normalized by rule type, that's why there should only be 3.
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)) {
Expand Down

0 comments on commit 4d5447c

Please sign in to comment.