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 Sep 8, 2022
1 parent 8490f72 commit e1e0996
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 51 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",
"browser-request": "^0.3.3",
"bs58": "^5.0.0",
"content-type": "^1.0.4",
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 @@ -1509,12 +1509,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 @@ -1526,6 +1539,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 @@ -1615,7 +1629,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 @@ -1678,8 +1691,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 @@ -71,6 +72,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 @@ -83,6 +94,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 @@ -119,11 +136,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 @@ -133,7 +149,6 @@ export class IgnoredInvites {
ignoreInvitesPolicies.sources = sources;
});

// Race ends.
return true;
}

Expand All @@ -144,10 +159,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 @@ -158,7 +173,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 @@ -227,21 +242,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 @@ -258,40 +282,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 @@ -313,10 +318,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
6 changes: 5 additions & 1 deletion yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1410,7 +1410,6 @@

"@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":
Expand Down Expand Up @@ -2052,6 +2051,11 @@ available-typed-arrays@^1.0.5:
resolved "https://registry.yarnpkg.com/available-typed-arrays/-/available-typed-arrays-1.0.5.tgz#92f95616501069d07d10edb2fc37d3e1c65123b7"
integrity sha512-DMD0KiN46eipeziST1LPP/STfDU0sufISXmjSgvVsoU2tqxctQeASejWcfNtxYKqETM1UxQ8sp2OrSBWpHY6sw==

await-lock@^2.2.2:
version "2.2.2"
resolved "https://registry.yarnpkg.com/await-lock/-/await-lock-2.2.2.tgz#a95a9b269bfd2f69d22b17a321686f551152bcef"
integrity sha512-aDczADvlvTGajTDjcjpJMqRkOF6Qdz3YbPZm/PyW6tKPkx2hlYBzxMhEywM/tU72HrVZjgl5VCdRuMlA7pZ8Gw==

aws-sign2@~0.7.0:
version "0.7.0"
resolved "https://registry.yarnpkg.com/aws-sign2/-/aws-sign2-0.7.0.tgz#b46e890934a9591f2d2f6f86d7e6a9f1b3fe76a8"
Expand Down

0 comments on commit e1e0996

Please sign in to comment.