Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Second batch: Replace MatrixClient.isRoomEncrypted by MatrixClient.CryptoApi.isEncryptionEnabledInRoom #28466

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
8 changes: 4 additions & 4 deletions src/ScalarMessaging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ function getWidgets(event: MessageEvent<any>, roomId: string | null): void {
sendResponse(event, widgetStateEvents);
}

function getRoomEncState(event: MessageEvent<any>, roomId: string): void {
async function getRoomEncState(event: MessageEvent<any>, roomId: string): Promise<void> {
const client = MatrixClientPeg.get();
if (!client) {
sendError(event, _t("widget|error_need_to_be_logged_in"));
Expand All @@ -525,7 +525,7 @@ function getRoomEncState(event: MessageEvent<any>, roomId: string): void {
sendError(event, _t("scalar|error_room_unknown"));
return;
}
const roomIsEncrypted = MatrixClientPeg.safeGet().isRoomEncrypted(roomId);
const roomIsEncrypted = Boolean(await client.getCrypto()?.isEncryptionEnabledInRoom(roomId));

sendResponse(event, roomIsEncrypted);
}
Expand Down Expand Up @@ -841,7 +841,7 @@ async function readEvents(
}
}

const onMessage = function (event: MessageEvent<any>): void {
const onMessage = async function (event: MessageEvent<any>): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this method can be async? It could make events be handled out-of-order unless we add something to serialise the data flow

if (!event.origin) {
// @ts-ignore - stupid chrome
event.origin = event.originalEvent.origin;
Expand Down Expand Up @@ -928,7 +928,7 @@ const onMessage = function (event: MessageEvent<any>): void {
getMembershipCount(event, roomId);
return;
} else if (event.data.action === Action.GetRoomEncryptionState) {
getRoomEncState(event, roomId);
await getRoomEncState(event, roomId);
return;
} else if (event.data.action === Action.CanSendEvent) {
canSendEvent(event, roomId);
Expand Down
24 changes: 12 additions & 12 deletions src/components/views/rooms/SendMessageComposer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ export class SendMessageComposer extends React.Component<ISendMessageComposerPro
public static contextType = RoomContext;
public declare context: React.ContextType<typeof RoomContext>;

private readonly prepareToEncrypt?: DebouncedFunc<() => void>;
private prepareToEncrypt?: DebouncedFunc<() => void>;
private readonly editorRef = createRef<BasicMessageComposer>();
private model: EditorModel;
private currentlyComposedEditorState: SerializedPart[] | null = null;
Expand All @@ -253,25 +253,25 @@ export class SendMessageComposer extends React.Component<ISendMessageComposerPro
public constructor(props: ISendMessageComposerProps, context: React.ContextType<typeof RoomContext>) {
super(props, context);

if (this.props.mxClient.getCrypto() && this.props.mxClient.isRoomEncrypted(this.props.room.roomId)) {
this.prepareToEncrypt = throttle(
() => {
this.props.mxClient.getCrypto()?.prepareToEncrypt(this.props.room);
},
60000,
{ leading: true, trailing: false },
);
}

const partCreator = new CommandPartCreator(this.props.room, this.props.mxClient);
const parts = this.restoreStoredEditorState(partCreator) || [];
this.model = new EditorModel(parts, partCreator);
this.sendHistoryManager = new SendHistoryManager(this.props.room.roomId, "mx_cider_history_");
}

public componentDidMount(): void {
public async componentDidMount(): Promise<void> {
window.addEventListener("beforeunload", this.saveStoredEditorState);
this.dispatcherRef = dis.register(this.onAction);

if (await this.props.mxClient.getCrypto()?.isEncryptionEnabledInRoom(this.props.room.roomId)) {
this.prepareToEncrypt = throttle(
() => {
this.props.mxClient.getCrypto()?.prepareToEncrypt(this.props.room);
},
60000,
{ leading: true, trailing: false },
);
}
Comment on lines +266 to +274
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if isEncryptionEnabledInRoom is still resolving by the time the user types something, encryption won't be prepared it looks like, this is undesirable and a regression

}

public componentDidUpdate(prevProps: ISendMessageComposerProps): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,27 @@ interface IProps {
room: Room;
}

export default class RolesRoomSettingsTab extends React.Component<IProps> {
interface RolesRoomSettingsTabState {
isRoomEncrypted: boolean;
}

export default class RolesRoomSettingsTab extends React.Component<IProps, RolesRoomSettingsTabState> {
public static contextType = MatrixClientContext;
public declare context: React.ContextType<typeof MatrixClientContext>;

public componentDidMount(): void {
public constructor(props: IProps) {
super(props);
this.state = {
isRoomEncrypted: false,
};
}

public async componentDidMount(): Promise<void> {
this.context.on(RoomStateEvent.Update, this.onRoomStateUpdate);
this.setState({
isRoomEncrypted:
(await this.context.getCrypto()?.isEncryptionEnabledInRoom(this.props.room.roomId)) || false,
});
}

public componentWillUnmount(): void {
Expand Down Expand Up @@ -416,7 +431,7 @@ export default class RolesRoomSettingsTab extends React.Component<IProps> {
.filter(Boolean);

// hide the power level selector for enabling E2EE if it the room is already encrypted
if (client.isRoomEncrypted(this.props.room.roomId)) {
if (this.state.isRoomEncrypted) {
delete eventsLevels[EventType.RoomEncryption];
}
Comment on lines 433 to 436
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it'll cause a UI flicker, where a row disappears after all rooms are checked, this is undesirable, we should not show UI until we are sure it is relevant.


Expand Down
9 changes: 4 additions & 5 deletions src/indexing/EventIndex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import { MatrixClientPeg } from "../MatrixClientPeg";
import SettingsStore from "../settings/SettingsStore";
import { SettingLevel } from "../settings/SettingLevel";
import { ICrawlerCheckpoint, IEventAndProfile, IIndexStats, ILoadArgs, ISearchArgs } from "./BaseEventIndexManager";
import { asyncFilter } from "../utils/arrays.ts";

// The time in ms that the crawler will wait loop iterations if there
// have not been any checkpoints to consume in the last iteration.
Expand Down Expand Up @@ -103,13 +104,11 @@ export default class EventIndex extends EventEmitter {
const client = MatrixClientPeg.safeGet();
const rooms = client.getRooms();

const isRoomEncrypted = (room: Room): boolean => {
return client.isRoomEncrypted(room.roomId);
};

// We only care to crawl the encrypted rooms, non-encrypted
// rooms can use the search provided by the homeserver.
const encryptedRooms = rooms.filter(isRoomEncrypted);
const encryptedRooms = await asyncFilter(rooms, async (room) =>
Boolean(await client.getCrypto()?.isEncryptionEnabledInRoom(room.roomId)),
);

logger.log("EventIndex: Adding initial crawler checkpoints");

Expand Down
6 changes: 3 additions & 3 deletions src/stores/MemberListStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export class MemberListStore {
return [];
}

if (!this.isLazyLoadingEnabled(roomId) || this.loadedRooms.has(roomId)) {
if (!(await this.isLazyLoadingEnabled(roomId)) || this.loadedRooms.has(roomId)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reverse this if condition so that the synchronous path is not blocked behind the async one?

// nice and easy, we must already have all the members so just return them.
return this.loadMembersInRoom(room);
}
Expand Down Expand Up @@ -121,10 +121,10 @@ export class MemberListStore {
* @param roomId The room to check if lazy loading is enabled
* @returns True if enabled
*/
private isLazyLoadingEnabled(roomId: string): boolean {
private async isLazyLoadingEnabled(roomId: string): Promise<boolean> {
if (SettingsStore.getValue("feature_sliding_sync")) {
// only unencrypted rooms use lazy loading
return !this.stores.client!.isRoomEncrypted(roomId);
return !(await this.stores.client?.getCrypto()?.isEncryptionEnabledInRoom(roomId));
}
return this.stores.client!.hasLazyLoadMembersEnabled();
}
Expand Down
10 changes: 10 additions & 0 deletions src/utils/arrays.ts
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,16 @@ export async function asyncSome<T>(values: Iterable<T>, predicate: (value: T) =>
return false;
}

/**
* Async version of Array.filter.
* @param values
* @param predicate
Comment on lines +332 to +334
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc could do with clarification on what happens when a rejection occurs

*/
export async function asyncFilter<T>(values: Array<T>, predicate: (value: T) => Promise<boolean>): Promise<Array<T>> {
const results = await Promise.all(values.map(predicate));
return values.filter((_, i) => results[i]);
}

export function filterBoolean<T>(values: Array<T | null | undefined>): T[] {
return values.filter(Boolean) as T[];
}
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,7 @@ describe("<SendMessageComposer/>", () => {

it("should call prepareToEncrypt when the user is typing", async () => {
const cli = stubClient();
cli.isRoomEncrypted = jest.fn().mockReturnValue(true);
jest.spyOn(cli.getCrypto()!, "isEncryptionEnabledInRoom").mockResolvedValue(true);
const room = mkStubRoom("!roomId:server", "Room", cli);

expect(cli.getCrypto()!.prepareToEncrypt).not.toHaveBeenCalled();
Expand Down
3 changes: 1 addition & 2 deletions test/unit-tests/stores/MemberListStore-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,7 @@ describe("MemberListStore", () => {
});

it("does not use lazy loading on encrypted rooms", async () => {
client.isRoomEncrypted = jest.fn();
mocked(client.isRoomEncrypted).mockReturnValue(true);
jest.spyOn(client.getCrypto()!, "isEncryptionEnabledInRoom").mockResolvedValue(true);

const { joined } = await store.loadMemberList(roomId);
expect(joined).toEqual([room.getMember(alice)]);
Expand Down
12 changes: 12 additions & 0 deletions test/unit-tests/utils/arrays-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
concat,
asyncEvery,
asyncSome,
asyncFilter,
} from "../../../src/utils/arrays";

type TestParams = { input: number[]; output: number[] };
Expand Down Expand Up @@ -460,4 +461,15 @@ describe("arrays", () => {
expect(predicate).toHaveBeenCalledWith(2);
});
});

describe("asyncFilter", () => {
it("when called with an empty array, it should return an empty array", async () => {
expect(await asyncFilter([], jest.fn().mockResolvedValue(true))).toEqual([]);
});

it("should filter the content", async () => {
const predicate = jest.fn().mockImplementation((value) => Promise.resolve(value === 2));
expect(await asyncFilter([1, 2, 3], predicate)).toEqual([2]);
});
});
});
Loading