Skip to content

Commit

Permalink
Making IgnoredInvites mostly sync.
Browse files Browse the repository at this point in the history 
Experience and feedback from matrix-org/matrix-react-sdk#9255
indicates that we cannot integrate an async `IgnoredInvites.getRuleForInvite`. This
PR:

- rewrites `getRuleForInvite` to be sync;
- adds some locking within `IgnoredInvites` to avoid race conditions noticed by @t3chguy.
  • Loading branch information
@Yoric
Yoric committed Nov 3, 2022
1 parent 81c3668 commit 5d9f920
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 54 deletions.
1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
"dependencies": {
"@babel/runtime": "^7.12.5",
"another-json": "^0.2.0",
"await-lock": "^2.2.2",
"bs58": "^5.0.0",
"content-type": "^1.0.4",
"loglevel": "^1.7.1",
Expand Down
25 changes: 19 additions & 6 deletions spec/unit/matrix-client.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1500,12 +1500,25 @@ describe("MatrixClient", function() {
expect(target1).toBe(target2);
});

it("should initialize and return the same `sources` consistently", async () => {
const sources1 = await client.ignoredInvites.getOrCreateSourceRooms();
const sources2 = await client.ignoredInvites.getOrCreateSourceRooms();
expect(sources1).toBeTruthy();
it("if there are no source rooms, it should return an empty list consistently", async () => {
const sources1 = client.ignoredInvites.getSourceRooms();
const sources2 = client.ignoredInvites.getSourceRooms();
expect(sources1).toHaveLength(0);
expect(sources1).toEqual(sources2);
});

it("if there are any source rooms, it should the same list consistently", async () => {
const NEW_SOURCE_ROOM_ID = "!another-source:example.org";

// Make sure that everything is initialized.
await client.joinRoom(NEW_SOURCE_ROOM_ID);
await client.ignoredInvites.addSource(NEW_SOURCE_ROOM_ID);

const sources1 = client.ignoredInvites.getSourceRooms();
const sources2 = client.ignoredInvites.getSourceRooms();
expect(sources1).toHaveLength(1);
expect(sources1).toEqual(sources2);
expect(sources1.map(room => room.roomId)).toContain(NEW_SOURCE_ROOM_ID);
});

it("should initially not reject any invite", async () => {
Expand All @@ -1517,6 +1530,7 @@ describe("MatrixClient", function() {
});

it("should reject invites once we have added a matching rule in the target room (scope: user)", async () => {
await client.ignoredInvites.getOrCreateTargetRoom();
await client.ignoredInvites.addRule(PolicyScope.User, "*:example.org", "just a test");

// We should reject this invite.
Expand Down Expand Up @@ -1606,7 +1620,6 @@ describe("MatrixClient", function() {
const NEW_SOURCE_ROOM_ID = "!another-source:example.org";

// Make sure that everything is initialized.
await client.ignoredInvites.getOrCreateSourceRooms();
await client.joinRoom(NEW_SOURCE_ROOM_ID);
await client.ignoredInvites.addSource(NEW_SOURCE_ROOM_ID);

Expand Down Expand Up @@ -1669,8 +1682,8 @@ describe("MatrixClient", function() {
const NEW_SOURCE_ROOM_ID = "!another-source:example.org";

// Make sure that everything is initialized.
await client.ignoredInvites.getOrCreateSourceRooms();
await client.joinRoom(NEW_SOURCE_ROOM_ID);
await client.ignoredInvites.getOrCreateTargetRoom();
const newSourceRoom = client.getRoom(NEW_SOURCE_ROOM_ID);

// Fetch the list of sources and check that we do not have the new room yet.
Expand Down
98 changes: 54 additions & 44 deletions src/models/invites-ignorer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import AwaitLock from "await-lock";
import { UnstableValue } from "matrix-events-sdk";

import { MatrixClient } from "../client";
Expand Down Expand Up @@ -73,6 +74,16 @@ export enum PolicyScope {
* our data structures.
*/
export class IgnoredInvites {
// A lock around method `getOrCreateTargetRoom`.
// Used to ensure that only one async task of this class
// is creating a new target room and modifying the
// `target` property of account key `IGNORE_INVITES_POLICIES`.
private _getOrCreateTargetRoomLock = new AwaitLock();

// A lock around method `withIgnoreInvitesPoliciesLock`.
// Used to ensure that only one async task of this class is
// modifying `IGNORE_INVITES_POLICIES` at any point in time.
private _withIgnoreInvitesPoliciesLock = new AwaitLock();
constructor(
private readonly client: MatrixClient,
) {
Expand All @@ -85,6 +96,12 @@ export class IgnoredInvites {
* @param entity The entity covered by this rule. Globs are supported.
* @param reason A human-readable reason for introducing this new rule.
* @return The event id for the new rule.
*
* # Safety
*
* This method will rewrite the `Policies` object in the user's account data.
* This rewrite is inherently racy and could overwrite or be overwritten by
* other concurrent rewrites of the same object.
*/
public async addRule(scope: PolicyScope, entity: string, reason: string): Promise<string> {
const target = await this.getOrCreateTargetRoom();
Expand Down Expand Up @@ -121,11 +138,10 @@ export class IgnoredInvites {
*/
public async addSource(roomId: string): Promise<boolean> {
// We attempt to join the room *before* calling
// `await this.getOrCreateSourceRooms()` to decrease the duration
// `await this.getSourceRooms()` to decrease the duration
// of the racy section.
await this.client.joinRoom(roomId);
// Race starts.
const sources = (await this.getOrCreateSourceRooms())
const sources = this.getSourceRooms()
.map(room => room.roomId);
if (sources.includes(roomId)) {
return false;
Expand All @@ -135,7 +151,6 @@ export class IgnoredInvites {
ignoreInvitesPolicies.sources = sources;
});

// Race ends.
return true;
}

Expand All @@ -146,10 +161,10 @@ export class IgnoredInvites {
* @param roomId The room to which the user is invited.
* @returns A rule matching the entity, if any was found, `null` otherwise.
*/
public async getRuleForInvite({ sender, roomId }: {
public getRuleForInvite({ sender, roomId }: {
sender: string;
roomId: string;
}): Promise<Readonly<MatrixEvent | null>> {
}): Readonly<MatrixEvent | null> {
// In this implementation, we perform a very naive lookup:
// - search in each policy room;
// - turn each (potentially glob) rule entity into a regexp.
Expand All @@ -160,7 +175,7 @@ export class IgnoredInvites {
// - match several entities per go;
// - pre-compile each rule entity into a regexp;
// - pre-compile entire rooms into a single regexp.
const policyRooms = await this.getOrCreateSourceRooms();
const policyRooms = this.getSourceRooms();
const senderServer = sender.split(":")[1];
const roomServer = roomId.split(":")[1];
for (const room of policyRooms) {
Expand Down Expand Up @@ -229,21 +244,30 @@ export class IgnoredInvites {
const room = this.client.getRoom(target);
if (room) {
return room;
} else {
target = null;
}
}
// We need to create our own policy room for ignoring invites.
target = (await this.client.createRoom({
name: "Individual Policy Room",
preset: Preset.PrivateChat,
})).room_id;
await this.withIgnoreInvitesPolicies(ignoreInvitesPolicies => {
ignoreInvitesPolicies.target = target;
});
try {
// We need to create our own policy room for ignoring invites.
await this._getOrCreateTargetRoomLock.acquireAsync();
target = (await this.client.createRoom({
name: "Individual Policy Room",
preset: Preset.PrivateChat,
})).room_id;
await this.withIgnoreInvitesPolicies(ignoreInvitesPolicies => {
ignoreInvitesPolicies.target = target;
if (!("sources" in ignoreInvitesPolicies)) {
// `[target]` is a reasonable default for `sources`.
ignoreInvitesPolicies.sources = [target];
}
});

// Since we have just called `createRoom`, `getRoom` should not be `null`.
return this.client.getRoom(target)!;
// Since we have just called `createRoom`, `getRoom` should not be `null`.
// Note that this is unavoidably racy, e.g. another client could have left
// the room during the call to `this.withIgnoreInvitesPolicies`.
return this.client.getRoom(target)!;
} finally {
this._getOrCreateTargetRoomLock.release();
}
}

/**
Expand All @@ -260,40 +284,21 @@ export class IgnoredInvites {
* This rewrite is inherently racy and could overwrite or be overwritten by
* other concurrent rewrites of the same object.
*/
public async getOrCreateSourceRooms(): Promise<Room[]> {
public getSourceRooms(): Room[] {
const ignoreInvitesPolicies = this.getIgnoreInvitesPolicies();
let sources = ignoreInvitesPolicies.sources;

// Validate `sources`. If it is invalid, trash out the current `sources`
// and create a new list of sources from `target`.
let hasChanges = false;
if (!Array.isArray(sources)) {
// `sources` could not be an array.
hasChanges = true;
sources = [];
}
let sourceRooms: Room[] = sources
const sourceRooms: Room[] = sources
// `sources` could contain non-string / invalid room ids
.filter(roomId => typeof roomId === "string")
.map(roomId => this.client.getRoom(roomId))
.filter(room => !!room);
if (sourceRooms.length != sources.length) {
hasChanges = true;
}
if (sourceRooms.length == 0) {
// `sources` could be empty (possibly because we've removed
// invalid content)
const target = await this.getOrCreateTargetRoom();
hasChanges = true;
sourceRooms = [target];
}
if (hasChanges) {
// Reload `policies`/`ignoreInvitesPolicies` in case it has been changed
// during or by our call to `this.getTargetRoom()`.
await this.withIgnoreInvitesPolicies(ignoreInvitesPolicies => {
ignoreInvitesPolicies.sources = sources;
});
}
return sourceRooms;
}

Expand All @@ -315,10 +320,15 @@ export class IgnoredInvites {
* Modify in place the `IGNORE_INVITES_POLICIES` object from account data.
*/
private async withIgnoreInvitesPolicies(cb: (ignoreInvitesPolicies: {[key: string]: any}) => void) {
const { policies, ignoreInvitesPolicies } = this.getPoliciesAndIgnoreInvitesPolicies();
cb(ignoreInvitesPolicies);
policies[IGNORE_INVITES_ACCOUNT_EVENT_KEY.name] = ignoreInvitesPolicies;
await this.client.setAccountData(POLICIES_ACCOUNT_EVENT_TYPE.name, policies);
await this._withIgnoreInvitesPoliciesLock.acquireAsync();
try {
const { policies, ignoreInvitesPolicies } = this.getPoliciesAndIgnoreInvitesPolicies();
cb(ignoreInvitesPolicies);
policies[IGNORE_INVITES_ACCOUNT_EVENT_KEY.name] = ignoreInvitesPolicies;
await this.client.setAccountData(POLICIES_ACCOUNT_EVENT_TYPE.name, policies);
} finally {
this._withIgnoreInvitesPoliciesLock.release();
}
}

/**
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1367,10 +1367,10 @@
"@jridgewell/resolve-uri" "3.1.0"
"@jridgewell/sourcemap-codec" "1.4.14"

"@matrix-org/olm@https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.13.tgz":
version "3.2.13"
uid "0109fde93bcc61def851f79826c9384c073b5175"
resolved "https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.13.tgz#0109fde93bcc61def851f79826c9384c073b5175"
"@matrix-org/olm@https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.12.tgz":
version "3.2.12"
uid "0bce3c86f9d36a4984d3c3e07df1c3fb4c679bd9"
resolved "https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.12.tgz#0bce3c86f9d36a4984d3c3e07df1c3fb4c679bd9"

"@nicolo-ribaudo/chokidar-2@2.1.8-no-fsevents.3":
version "2.1.8-no-fsevents.3"
Expand Down

0 comments on commit 5d9f920

Please sign in to comment.