From 4dffcf6835512ba5eb07bd5a9fb15ee6aa39f7e0 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 14 Apr 2023 17:15:30 +0100 Subject: [PATCH 01/12] Use `fetchDevicesWithVerification` in DevicesPanel We have a method called `fetchDevicesWithVerification` which is currently used in the new Session manager. Let's use it for the (old) Devices Panel too, because it allows us to check the device verification upfront, rather than on the fly, which is going to be handy when that operation becomes async --- .../views/settings/DevicesPanel.tsx | 46 ++++++++----------- .../views/settings/devices/useOwnDevices.ts | 12 +++-- .../views/settings/DevicesPanel-test.tsx | 8 +++- 3 files changed, 35 insertions(+), 31 deletions(-) diff --git a/src/components/views/settings/DevicesPanel.tsx b/src/components/views/settings/DevicesPanel.tsx index 603438e7e5f..06bdc5fea99 100644 --- a/src/components/views/settings/DevicesPanel.tsx +++ b/src/components/views/settings/DevicesPanel.tsx @@ -26,14 +26,15 @@ import Spinner from "../elements/Spinner"; import AccessibleButton from "../elements/AccessibleButton"; import { deleteDevicesWithInteractiveAuth } from "./devices/deleteDevices"; import MatrixClientContext from "../../../contexts/MatrixClientContext"; -import { isDeviceVerified } from "../../../utils/device/isDeviceVerified"; +import { fetchExtendedDeviceInformation } from "./devices/useOwnDevices"; +import { DevicesDictionary, ExtendedDevice } from "./devices/types"; interface IProps { className?: string; } interface IState { - devices: IMyDevice[]; + devices?: DevicesDictionary; deviceLoadError?: string; selectedDevices: string[]; deleting?: boolean; @@ -47,7 +48,6 @@ export default class DevicesPanel extends React.Component { public constructor(props: IProps) { super(props); this.state = { - devices: [], selectedDevices: [], }; this.loadDevices = this.loadDevices.bind(this); @@ -70,18 +70,16 @@ export default class DevicesPanel extends React.Component { private loadDevices(): void { const cli = this.context; - cli.getDevices().then( - (resp) => { + fetchExtendedDeviceInformation(cli).then( + (devices) => { if (this.unmounted) { return; } this.setState((state, props) => { - const deviceIds = resp.devices.map((device) => device.device_id); - const selectedDevices = state.selectedDevices.filter((deviceId) => deviceIds.includes(deviceId)); return { - devices: resp.devices || [], - selectedDevices, + devices: devices, + selectedDevices: state.selectedDevices.filter((deviceId) => devices.hasOwnProperty(deviceId)), }; }); }, @@ -119,10 +117,6 @@ export default class DevicesPanel extends React.Component { return idA < idB ? -1 : idA > idB ? 1 : 0; } - private isDeviceVerified(device: IMyDevice): boolean | null { - return isDeviceVerified(this.context, device.device_id); - } - private onDeviceSelectionToggled = (device: IMyDevice): void => { if (this.unmounted) { return; @@ -205,15 +199,15 @@ export default class DevicesPanel extends React.Component { } }; - private renderDevice = (device: IMyDevice): JSX.Element => { - const myDeviceId = this.context.getDeviceId(); - const myDevice = this.state.devices.find((device) => device.device_id === myDeviceId); + private renderDevice = (device: ExtendedDevice): JSX.Element => { + const myDeviceId = this.context.getDeviceId()!; + const myDevice = this.state.devices?.[myDeviceId]; const isOwnDevice = device.device_id === myDeviceId; // If our own device is unverified, it can't verify other // devices, it can only request verification for itself - const canBeVerified = (myDevice && this.isDeviceVerified(myDevice)) || isOwnDevice; + const canBeVerified = (myDevice && myDevice.isVerified) || isOwnDevice; return ( { device={device} selected={this.state.selectedDevices.includes(device.device_id)} isOwnDevice={isOwnDevice} - verified={this.isDeviceVerified(device)} + verified={device.isVerified} canBeVerified={canBeVerified} onDeviceChange={this.loadDevices} onDeviceToggled={this.onDeviceSelectionToggled} @@ -242,21 +236,21 @@ export default class DevicesPanel extends React.Component { return ; } - const myDeviceId = this.context.getDeviceId(); - const myDevice = devices.find((device) => device.device_id === myDeviceId); + const myDeviceId = this.context.getDeviceId()!; + const myDevice = devices[myDeviceId]; if (!myDevice) { return loadError; } - const otherDevices = devices.filter((device) => device.device_id !== myDeviceId); + const otherDevices = Object.values(devices).filter((device) => device.device_id !== myDeviceId); otherDevices.sort(this.deviceCompare); - const verifiedDevices: IMyDevice[] = []; - const unverifiedDevices: IMyDevice[] = []; - const nonCryptoDevices: IMyDevice[] = []; + const verifiedDevices: ExtendedDevice[] = []; + const unverifiedDevices: ExtendedDevice[] = []; + const nonCryptoDevices: ExtendedDevice[] = []; for (const device of otherDevices) { - const verified = this.isDeviceVerified(device); + const verified = device.isVerified; if (verified === true) { verifiedDevices.push(device); } else if (verified === false) { @@ -266,7 +260,7 @@ export default class DevicesPanel extends React.Component { } } - const section = (trustIcon: JSX.Element, title: string, deviceList: IMyDevice[]): JSX.Element => { + const section = (trustIcon: JSX.Element, title: string, deviceList: ExtendedDevice[]): JSX.Element => { if (deviceList.length === 0) { return ; } diff --git a/src/components/views/settings/devices/useOwnDevices.ts b/src/components/views/settings/devices/useOwnDevices.ts index ffd0e00eba2..209c35085da 100644 --- a/src/components/views/settings/devices/useOwnDevices.ts +++ b/src/components/views/settings/devices/useOwnDevices.ts @@ -50,7 +50,13 @@ const parseDeviceExtendedInformation = (matrixClient: MatrixClient, device: IMyD }; }; -const fetchDevicesWithVerification = async (matrixClient: MatrixClient): Promise => { +/** + * Fetch extended details of the user's own devices + * + * @param matrixClient - Matrix Client + * @returns A dictionary mapping from device ID to ExtendedDevice + */ +export async function fetchExtendedDeviceInformation(matrixClient: MatrixClient): Promise { const { devices } = await matrixClient.getDevices(); const devicesDict = devices.reduce( @@ -67,7 +73,7 @@ const fetchDevicesWithVerification = async (matrixClient: MatrixClient): Promise ); return devicesDict; -}; +} export enum OwnDevicesError { Unsupported = "Unsupported", @@ -112,7 +118,7 @@ export const useOwnDevices = (): DevicesState => { const refreshDevices = useCallback(async (): Promise => { setIsLoadingDeviceList(true); try { - const devices = await fetchDevicesWithVerification(matrixClient); + const devices = await fetchExtendedDeviceInformation(matrixClient); setDevices(devices); const { pushers } = await matrixClient.getPushers(); diff --git a/test/components/views/settings/DevicesPanel-test.tsx b/test/components/views/settings/DevicesPanel-test.tsx index b7a0ec6a257..dd334d89475 100644 --- a/test/components/views/settings/DevicesPanel-test.tsx +++ b/test/components/views/settings/DevicesPanel-test.tsx @@ -18,7 +18,6 @@ import { act, fireEvent, render } from "@testing-library/react"; import { DeviceInfo } from "matrix-js-sdk/src/crypto/deviceinfo"; import { sleep } from "matrix-js-sdk/src/utils"; import { PUSHER_DEVICE_ID, PUSHER_ENABLED } from "matrix-js-sdk/src/@types/event"; -import { DeviceTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; import DevicesPanel from "../../../../src/components/views/settings/DevicesPanel"; import { flushPromises, getMockClientWithEventEmitter, mkPusher, mockClientMethodsUser } from "../../../test-utils"; @@ -29,16 +28,21 @@ describe("", () => { const device1 = { device_id: "device_1" }; const device2 = { device_id: "device_2" }; const device3 = { device_id: "device_3" }; + const mockCrypto = { + getDeviceVerificationStatus: jest.fn().mockResolvedValue({ + crossSigningVerified: false, + }), + }; const mockClient = getMockClientWithEventEmitter({ ...mockClientMethodsUser(userId), getDevices: jest.fn(), getDeviceId: jest.fn().mockReturnValue(device1.device_id), deleteMultipleDevices: jest.fn(), - checkDeviceTrust: jest.fn().mockReturnValue(new DeviceTrustLevel(false, false, false, false)), getStoredDevice: jest.fn().mockReturnValue(new DeviceInfo("id")), generateClientSecret: jest.fn(), getPushers: jest.fn(), setPusher: jest.fn(), + getCrypto: jest.fn().mockReturnValue(mockCrypto), }); const getComponent = () => ( From f706559a55a528303c08b40770d9ef7177af9671 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 14 Apr 2023 18:24:25 +0100 Subject: [PATCH 02/12] A couple of simple uses of `checkDeviceTrust` --- src/DeviceListener.ts | 12 +++++++++--- src/SlashCommands.tsx | 4 ++-- test/DeviceListener-test.ts | 30 ++++++++++++++++++------------ 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/DeviceListener.ts b/src/DeviceListener.ts index 3281c43267f..e53386b0a0d 100644 --- a/src/DeviceListener.ts +++ b/src/DeviceListener.ts @@ -310,7 +310,11 @@ export default class DeviceListener { const newUnverifiedDeviceIds = new Set(); const isCurrentDeviceTrusted = - crossSigningReady && (await cli.checkDeviceTrust(cli.getUserId()!, cli.deviceId!).isCrossSigningVerified()); + crossSigningReady && + Boolean( + (await cli.getCrypto()?.getDeviceVerificationStatus(cli.getUserId()!, cli.deviceId!)) + ?.crossSigningVerified, + ); // as long as cross-signing isn't ready, // you can't see or dismiss any device toasts @@ -319,8 +323,10 @@ export default class DeviceListener { for (const device of devices) { if (device.deviceId === cli.deviceId) continue; - const deviceTrust = await cli.checkDeviceTrust(cli.getUserId()!, device.deviceId!); - if (!deviceTrust.isCrossSigningVerified() && !this.dismissed.has(device.deviceId)) { + const deviceTrust = await cli + .getCrypto() + ?.getDeviceVerificationStatus(cli.getUserId()!, device.deviceId!); + if (!deviceTrust?.crossSigningVerified && !this.dismissed.has(device.deviceId)) { if (this.ourDeviceIdsAtStart?.has(device.deviceId)) { oldUnverifiedDeviceIds.add(device.deviceId); } else { diff --git a/src/SlashCommands.tsx b/src/SlashCommands.tsx index d06e6858bc1..f3355388f28 100644 --- a/src/SlashCommands.tsx +++ b/src/SlashCommands.tsx @@ -1073,9 +1073,9 @@ export const Commands = [ }, ); } - const deviceTrust = await cli.checkDeviceTrust(userId, deviceId); + const deviceTrust = await cli.getCrypto()?.getDeviceVerificationStatus(userId, deviceId); - if (deviceTrust.isVerified()) { + if (deviceTrust?.isVerified()) { if (device.getFingerprint() === fingerprint) { throw new UserFriendlyError("Session already verified!"); } else { diff --git a/test/DeviceListener-test.ts b/test/DeviceListener-test.ts index c0470200d7b..fe6a61c90ae 100644 --- a/test/DeviceListener-test.ts +++ b/test/DeviceListener-test.ts @@ -15,10 +15,10 @@ limitations under the License. */ import { Mocked, mocked } from "jest-mock"; -import { MatrixEvent, Room, MatrixClient } from "matrix-js-sdk/src/matrix"; +import { MatrixEvent, Room, MatrixClient, DeviceVerificationStatus, CryptoApi } from "matrix-js-sdk/src/matrix"; import { logger } from "matrix-js-sdk/src/logger"; import { DeviceInfo } from "matrix-js-sdk/src/crypto/deviceinfo"; -import { CrossSigningInfo, DeviceTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; +import { CrossSigningInfo } from "matrix-js-sdk/src/crypto/CrossSigning"; import { CryptoEvent } from "matrix-js-sdk/src/crypto"; import { IKeyBackupInfo } from "matrix-js-sdk/src/crypto/keybackup"; @@ -60,6 +60,7 @@ const flushPromises = async () => await new Promise(process.nextTick); describe("DeviceListener", () => { let mockClient: Mocked | undefined; + let mockCrypto: Mocked | undefined; // spy on various toasts' hide and show functions // easier than mocking @@ -75,6 +76,11 @@ describe("DeviceListener", () => { mockPlatformPeg({ getAppVersion: jest.fn().mockResolvedValue("1.2.3"), }); + mockCrypto = { + getDeviceVerificationStatus: jest.fn().mockResolvedValue({ + crossSigningVerified: false, + }), + } as unknown as Mocked; mockClient = getMockClientWithEventEmitter({ isGuest: jest.fn(), getUserId: jest.fn().mockReturnValue(userId), @@ -97,7 +103,7 @@ describe("DeviceListener", () => { setAccountData: jest.fn(), getAccountData: jest.fn(), deleteAccountData: jest.fn(), - checkDeviceTrust: jest.fn().mockReturnValue(new DeviceTrustLevel(false, false, false, false)), + getCrypto: jest.fn().mockReturnValue(mockCrypto), }); jest.spyOn(MatrixClientPeg, "get").mockReturnValue(mockClient); jest.spyOn(SettingsStore, "getValue").mockReturnValue(false); @@ -391,14 +397,14 @@ describe("DeviceListener", () => { const device2 = new DeviceInfo("d2"); const device3 = new DeviceInfo("d3"); - const deviceTrustVerified = new DeviceTrustLevel(true, false, false, false); - const deviceTrustUnverified = new DeviceTrustLevel(false, false, false, false); + const deviceTrustVerified = new DeviceVerificationStatus({ crossSigningVerified: true }); + const deviceTrustUnverified = new DeviceVerificationStatus({}); beforeEach(() => { mockClient!.isCrossSigningReady.mockResolvedValue(true); mockClient!.getStoredDevicesForUser.mockReturnValue([currentDevice, device2, device3]); // all devices verified by default - mockClient!.checkDeviceTrust.mockReturnValue(deviceTrustVerified); + mockCrypto!.getDeviceVerificationStatus.mockResolvedValue(deviceTrustVerified); mockClient!.deviceId = currentDevice.deviceId; jest.spyOn(SettingsStore, "getValue").mockImplementation( (settingName) => settingName === UIFeature.BulkUnverifiedSessionsReminder, @@ -423,7 +429,7 @@ describe("DeviceListener", () => { jest.spyOn(SettingsStore, "getValue").mockReturnValue(false); // currentDevice, device2 are verified, device3 is unverified // ie if reminder was enabled it should be shown - mockClient!.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto!.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { switch (deviceId) { case currentDevice.deviceId: case device2.deviceId: @@ -438,7 +444,7 @@ describe("DeviceListener", () => { it("hides toast when current device is unverified", async () => { // device2 verified, current and device3 unverified - mockClient!.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto!.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { switch (deviceId) { case device2.deviceId: return deviceTrustVerified; @@ -454,7 +460,7 @@ describe("DeviceListener", () => { it("hides toast when reminder is snoozed", async () => { mocked(isBulkUnverifiedDeviceReminderSnoozed).mockReturnValue(true); // currentDevice, device2 are verified, device3 is unverified - mockClient!.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto!.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { switch (deviceId) { case currentDevice.deviceId: case device2.deviceId: @@ -470,7 +476,7 @@ describe("DeviceListener", () => { it("shows toast with unverified devices at app start", async () => { // currentDevice, device2 are verified, device3 is unverified - mockClient!.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto!.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { switch (deviceId) { case currentDevice.deviceId: case device2.deviceId: @@ -488,7 +494,7 @@ describe("DeviceListener", () => { it("hides toast when unverified sessions at app start have been dismissed", async () => { // currentDevice, device2 are verified, device3 is unverified - mockClient!.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto!.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { switch (deviceId) { case currentDevice.deviceId: case device2.deviceId: @@ -510,7 +516,7 @@ describe("DeviceListener", () => { it("hides toast when unverified sessions are added after app start", async () => { // currentDevice, device2 are verified, device3 is unverified - mockClient!.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto!.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { switch (deviceId) { case currentDevice.deviceId: case device2.deviceId: From 6c6fe7fad076377c6f2aab4fa324a00e1a4791f5 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 14 Apr 2023 18:28:45 +0100 Subject: [PATCH 03/12] cases of `checkDeviceTrust` that need `asyncSome` these are in an async method, but currently use `some`. We need to invent an `asyncSome` (we already have an `asyncEvery`) and use that --- src/components/views/rooms/MemberTile.tsx | 7 ++++--- src/utils/ShieldUtils.ts | 6 ++++-- src/utils/arrays.ts | 10 ++++++++++ test/utils/ShieldUtils-test.ts | 19 +++++++++++------- test/utils/arrays-test.ts | 24 +++++++++++++++++++++++ 5 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/components/views/rooms/MemberTile.tsx b/src/components/views/rooms/MemberTile.tsx index cac793066b3..7ff46c922f7 100644 --- a/src/components/views/rooms/MemberTile.tsx +++ b/src/components/views/rooms/MemberTile.tsx @@ -33,6 +33,7 @@ import MemberAvatar from "./../avatars/MemberAvatar"; import DisambiguatedProfile from "../messages/DisambiguatedProfile"; import UserIdentifierCustomisations from "../../../customisations/UserIdentifier"; import { E2EState } from "./E2EIcon"; +import { asyncSome } from "../../../utils/arrays"; interface IProps { member: RoomMember; @@ -127,15 +128,15 @@ export default class MemberTile extends React.Component { } const devices = cli.getStoredDevicesForUser(userId); - const anyDeviceUnverified = devices.some((device) => { + const anyDeviceUnverified = await asyncSome(devices, async (device) => { const { deviceId } = device; // For your own devices, we use the stricter check of cross-signing // verification to encourage everyone to trust their own devices via // cross-signing so that other users can then safely trust you. // For other people's devices, the more general verified check that // includes locally verified devices can be used. - const deviceTrust = cli.checkDeviceTrust(userId, deviceId); - return isMe ? !deviceTrust.isCrossSigningVerified() : !deviceTrust.isVerified(); + const deviceTrust = await cli.getCrypto()?.getDeviceVerificationStatus(userId, deviceId); + return !deviceTrust || (isMe ? !deviceTrust.crossSigningVerified : !deviceTrust.isVerified()); }); this.setState({ e2eStatus: anyDeviceUnverified ? E2EState.Warning : E2EState.Verified, diff --git a/src/utils/ShieldUtils.ts b/src/utils/ShieldUtils.ts index 089cf3feeb7..a9efe4584f7 100644 --- a/src/utils/ShieldUtils.ts +++ b/src/utils/ShieldUtils.ts @@ -18,6 +18,7 @@ import { MatrixClient } from "matrix-js-sdk/src/client"; import { Room } from "matrix-js-sdk/src/models/room"; import DMRoomMap from "./DMRoomMap"; +import { asyncSome } from "./arrays"; export enum E2EStatus { Warning = "warning", @@ -54,8 +55,9 @@ export async function shieldStatusForRoom(client: MatrixClient, room: Room): Pro const targets = includeUser ? [...verified, client.getUserId()!] : verified; for (const userId of targets) { const devices = client.getStoredDevicesForUser(userId); - const anyDeviceNotVerified = devices.some(({ deviceId }) => { - return !client.checkDeviceTrust(userId, deviceId).isVerified(); + const anyDeviceNotVerified = await asyncSome(devices, async ({ deviceId }) => { + const verificationStatus = await client.getCrypto()?.getDeviceVerificationStatus(userId, deviceId); + return !verificationStatus?.isVerified(); }); if (anyDeviceNotVerified) { return E2EStatus.Warning; diff --git a/src/utils/arrays.ts b/src/utils/arrays.ts index bbc75fa4f7e..d9c7c3735ea 100644 --- a/src/utils/arrays.ts +++ b/src/utils/arrays.ts @@ -324,6 +324,16 @@ export async function asyncEvery(values: T[], predicate: (value: T) => Promis return true; } +/** + * Async version of Array.some. + */ +export async function asyncSome(values: T[], predicate: (value: T) => Promise): Promise { + for (const value of values) { + if (await predicate(value)) return true; + } + return false; +} + export function filterBoolean(values: Array): T[] { return values.filter(Boolean) as T[]; } diff --git a/test/utils/ShieldUtils-test.ts b/test/utils/ShieldUtils-test.ts index f971cea7798..8759252727b 100644 --- a/test/utils/ShieldUtils-test.ts +++ b/test/utils/ShieldUtils-test.ts @@ -22,21 +22,25 @@ import DMRoomMap from "../../src/utils/DMRoomMap"; function mkClient(selfTrust = false) { return { getUserId: () => "@self:localhost", + getCrypto: () => ({ + getDeviceVerificationStatus: (userId: string, deviceId: string) => + Promise.resolve({ + isVerified: () => (userId === "@self:localhost" ? selfTrust : userId[2] == "T"), + }), + }), checkUserTrust: (userId: string) => ({ isCrossSigningVerified: () => userId[1] == "T", wasCrossSigningVerified: () => userId[1] == "T" || userId[1] == "W", }), - checkDeviceTrust: (userId: string, deviceId: string) => ({ - isVerified: () => (userId === "@self:localhost" ? selfTrust : userId[2] == "T"), - }), getStoredDevicesForUser: (userId: string) => ["DEVICE"], } as unknown as MatrixClient; } describe("mkClient self-test", function () { - test.each([true, false])("behaves well for self-trust=%s", (v) => { + test.each([true, false])("behaves well for self-trust=%s", async (v) => { const client = mkClient(v); - expect(client.checkDeviceTrust("@self:localhost", "DEVICE").isVerified()).toBe(v); + const status = await client.getCrypto()!.getDeviceVerificationStatus("@self:localhost", "DEVICE"); + expect(status?.isVerified()).toBe(v); }); test.each([ @@ -53,8 +57,9 @@ describe("mkClient self-test", function () { ["@TF:h", false], ["@FT:h", true], ["@FF:h", false], - ])("behaves well for device trust %s", (userId, trust) => { - expect(mkClient().checkDeviceTrust(userId, "device").isVerified()).toBe(trust); + ])("behaves well for device trust %s", async (userId, trust) => { + const status = await mkClient().getCrypto()!.getDeviceVerificationStatus(userId, "device"); + expect(status?.isVerified()).toBe(trust); }); }); diff --git a/test/utils/arrays-test.ts b/test/utils/arrays-test.ts index 558b85e0b5c..f843210dc4c 100644 --- a/test/utils/arrays-test.ts +++ b/test/utils/arrays-test.ts @@ -30,6 +30,7 @@ import { GroupedArray, concat, asyncEvery, + asyncSome, } from "../../src/utils/arrays"; type TestParams = { input: number[]; output: number[] }; @@ -444,4 +445,27 @@ describe("arrays", () => { expect(predicate).toHaveBeenCalledWith(2); }); }); + + describe("asyncSome", () => { + it("when called with an empty array, it should return false", async () => { + expect(await asyncSome([], jest.fn().mockResolvedValue(true))).toBe(false); + }); + + it("when called with some items and the predicate resolves to false for all of them, it should return false", async () => { + const predicate = jest.fn().mockResolvedValue(false); + expect(await asyncSome([1, 2, 3], predicate)).toBe(false); + expect(predicate).toHaveBeenCalledTimes(3); + expect(predicate).toHaveBeenCalledWith(1); + expect(predicate).toHaveBeenCalledWith(2); + expect(predicate).toHaveBeenCalledWith(3); + }); + + it("when called with some items and the predicate resolves to true, it should short-circuit and return true", async () => { + const predicate = jest.fn().mockResolvedValueOnce(false).mockResolvedValueOnce(true); + expect(await asyncSome([1, 2, 3], predicate)).toBe(true); + expect(predicate).toHaveBeenCalledTimes(2); + expect(predicate).toHaveBeenCalledWith(1); + expect(predicate).toHaveBeenCalledWith(2); + }); + }); }); From 432e29344ae9b22399f3ed766f8f778cfae356e3 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 14 Apr 2023 18:32:26 +0100 Subject: [PATCH 04/12] `isDeviceVerified`: switch to `getDeviceVerificationStatus` ... which means making it async --- .../views/settings/devices/useOwnDevices.ts | 22 +++----- src/toasts/UnverifiedSessionToast.tsx | 2 +- src/utils/device/isDeviceVerified.ts | 6 +- .../tabs/user/SessionManagerTab-test.tsx | 56 +++++++++++-------- test/toasts/UnverifiedSessionToast-test.tsx | 9 ++- 5 files changed, 52 insertions(+), 43 deletions(-) diff --git a/src/components/views/settings/devices/useOwnDevices.ts b/src/components/views/settings/devices/useOwnDevices.ts index 209c35085da..e4753cad43f 100644 --- a/src/components/views/settings/devices/useOwnDevices.ts +++ b/src/components/views/settings/devices/useOwnDevices.ts @@ -59,19 +59,15 @@ const parseDeviceExtendedInformation = (matrixClient: MatrixClient, device: IMyD export async function fetchExtendedDeviceInformation(matrixClient: MatrixClient): Promise { const { devices } = await matrixClient.getDevices(); - const devicesDict = devices.reduce( - (acc, device: IMyDevice) => ({ - ...acc, - [device.device_id]: { - ...device, - isVerified: isDeviceVerified(matrixClient, device.device_id), - ...parseDeviceExtendedInformation(matrixClient, device), - ...parseUserAgent(device[UNSTABLE_MSC3852_LAST_SEEN_UA.name]), - }, - }), - {}, - ); - + const devicesDict: DevicesDictionary = {}; + for (const device of devices) { + devicesDict[device.device_id] = { + ...device, + isVerified: await isDeviceVerified(matrixClient, device.device_id), + ...parseDeviceExtendedInformation(matrixClient, device), + ...parseUserAgent(device[UNSTABLE_MSC3852_LAST_SEEN_UA.name]), + }; + } return devicesDict; } diff --git a/src/toasts/UnverifiedSessionToast.tsx b/src/toasts/UnverifiedSessionToast.tsx index 8d619d87675..b3e9a63b591 100644 --- a/src/toasts/UnverifiedSessionToast.tsx +++ b/src/toasts/UnverifiedSessionToast.tsx @@ -48,7 +48,7 @@ export const showToast = async (deviceId: string): Promise => { const device = await cli.getDevice(deviceId); const extendedDevice = { ...device, - isVerified: isDeviceVerified(cli, deviceId), + isVerified: await isDeviceVerified(cli, deviceId), deviceType: DeviceType.Unknown, }; diff --git a/src/utils/device/isDeviceVerified.ts b/src/utils/device/isDeviceVerified.ts index 0f8fdb6082e..1671fa01a30 100644 --- a/src/utils/device/isDeviceVerified.ts +++ b/src/utils/device/isDeviceVerified.ts @@ -25,10 +25,10 @@ import { MatrixClient } from "matrix-js-sdk/src/matrix"; * @returns `true` if the device has been correctly cross-signed. `false` if the device is unknown or not correctly * cross-signed. `null` if there was an error fetching the device info. */ -export const isDeviceVerified = (client: MatrixClient, deviceId: string): boolean | null => { +export const isDeviceVerified = async (client: MatrixClient, deviceId: string): Promise => { try { - const trustLevel = client.checkDeviceTrust(client.getSafeUserId(), deviceId); - return trustLevel.isCrossSigningVerified(); + const trustLevel = await client.getCrypto()?.getDeviceVerificationStatus(client.getSafeUserId(), deviceId); + return trustLevel?.crossSigningVerified ?? false; } catch (e) { console.error("Error getting device cross-signing info", e); return null; diff --git a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx index b1648271869..6e982147bf5 100644 --- a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -18,7 +18,6 @@ import React from "react"; import { act, fireEvent, render, RenderResult } from "@testing-library/react"; import { DeviceInfo } from "matrix-js-sdk/src/crypto/deviceinfo"; import { logger } from "matrix-js-sdk/src/logger"; -import { DeviceTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; import { VerificationRequest } from "matrix-js-sdk/src/crypto/verification/request/VerificationRequest"; import { defer, sleep } from "matrix-js-sdk/src/utils"; import { @@ -30,7 +29,10 @@ import { PUSHER_ENABLED, IAuthData, UNSTABLE_MSC3882_CAPABILITY, + CryptoApi, + DeviceVerificationStatus, } from "matrix-js-sdk/src/matrix"; +import { mocked } from "jest-mock"; import { clearAllModals } from "../../../../../test-utils"; import SessionManagerTab from "../../../../../../src/components/views/settings/tabs/user/SessionManagerTab"; @@ -78,9 +80,14 @@ describe("", () => { cancel: jest.fn(), on: jest.fn(), } as unknown as VerificationRequest; + + const mockCrypto = mocked({ + getDeviceVerificationStatus: jest.fn(), + } as unknown as CryptoApi); + const mockClient = getMockClientWithEventEmitter({ ...mockClientMethodsUser(aliceId), - checkDeviceTrust: jest.fn(), + getCrypto: jest.fn().mockReturnValue(mockCrypto), getDevices: jest.fn(), getStoredDevice: jest.fn(), getDeviceId: jest.fn().mockReturnValue(deviceId), @@ -171,7 +178,7 @@ describe("", () => { const device = [alicesDevice, alicesMobileDevice].find((device) => device.device_id === id); return device ? new DeviceInfo(device.device_id) : null; }); - mockClient.checkDeviceTrust.mockReset().mockReturnValue(new DeviceTrustLevel(false, false, false, false)); + mockCrypto.getDeviceVerificationStatus.mockReset().mockResolvedValue(new DeviceVerificationStatus({})); mockClient.getDevices.mockReset().mockResolvedValue({ devices: [alicesDevice, alicesMobileDevice] }); @@ -221,13 +228,13 @@ describe("", () => { }); it("does not fail when checking device verification fails", async () => { - const logSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + const logSpy = jest.spyOn(console, "error").mockImplementation((e) => {}); mockClient.getDevices.mockResolvedValue({ devices: [alicesDevice, alicesMobileDevice], }); - const noCryptoError = new Error("End-to-end encryption disabled"); - mockClient.checkDeviceTrust.mockImplementation(() => { - throw noCryptoError; + const failError = new Error("non-specific failure"); + mockCrypto.getDeviceVerificationStatus.mockImplementation(() => { + throw failError; }); render(getComponent()); @@ -236,9 +243,9 @@ describe("", () => { }); // called for each device despite error - expect(mockClient.checkDeviceTrust).toHaveBeenCalledWith(aliceId, alicesDevice.device_id); - expect(mockClient.checkDeviceTrust).toHaveBeenCalledWith(aliceId, alicesMobileDevice.device_id); - expect(logSpy).toHaveBeenCalledWith("Error getting device cross-signing info", noCryptoError); + expect(mockCrypto.getDeviceVerificationStatus).toHaveBeenCalledWith(aliceId, alicesDevice.device_id); + expect(mockCrypto.getDeviceVerificationStatus).toHaveBeenCalledWith(aliceId, alicesMobileDevice.device_id); + expect(logSpy).toHaveBeenCalledWith("Error getting device cross-signing info", failError); }); it("sets device verification status correctly", async () => { @@ -246,14 +253,14 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice], }); mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); - mockClient.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { // alices device is trusted if (deviceId === alicesDevice.device_id) { - return new DeviceTrustLevel(true, true, false, false); + return new DeviceVerificationStatus({ crossSigningVerified: true, localVerified: true }); } // alices mobile device is not if (deviceId === alicesMobileDevice.device_id) { - return new DeviceTrustLevel(false, false, false, false); + return new DeviceVerificationStatus({}); } // alicesOlderMobileDevice does not support encryption throw new Error("encryption not supported"); @@ -265,7 +272,7 @@ describe("", () => { await flushPromises(); }); - expect(mockClient.checkDeviceTrust).toHaveBeenCalledTimes(3); + expect(mockCrypto.getDeviceVerificationStatus).toHaveBeenCalledTimes(3); expect( getByTestId(`device-tile-${alicesDevice.device_id}`).querySelector('[aria-label="Verified"]'), ).toBeTruthy(); @@ -418,7 +425,9 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice], }); mockClient.getStoredDevice.mockImplementation(() => new DeviceInfo(alicesDevice.device_id)); - mockClient.checkDeviceTrust.mockReturnValue(new DeviceTrustLevel(true, true, false, false)); + mockCrypto.getDeviceVerificationStatus.mockResolvedValue( + new DeviceVerificationStatus({ crossSigningVerified: true, localVerified: true }), + ); const { getByTestId } = render(getComponent()); @@ -520,11 +529,11 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice], }); mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); - mockClient.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { if (deviceId === alicesDevice.device_id) { - return new DeviceTrustLevel(true, true, false, false); + return new DeviceVerificationStatus({ crossSigningVerified: true, localVerified: true }); } - return new DeviceTrustLevel(false, false, false, false); + return new DeviceVerificationStatus({}); }); const { getByTestId } = render(getComponent()); @@ -547,12 +556,13 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice], }); mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); - mockClient.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { // current session verified = able to verify other sessions if (deviceId === alicesDevice.device_id) { - return new DeviceTrustLevel(true, true, false, false); + return new DeviceVerificationStatus({ crossSigningVerified: true, localVerified: true }); } // but alicesMobileDevice doesn't support encryption + // XXX this is not what happens if a device doesn't support encryption. throw new Error("encryption not supported"); }); @@ -581,11 +591,11 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice], }); mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); - mockClient.checkDeviceTrust.mockImplementation((_userId, deviceId) => { + mockCrypto.getDeviceVerificationStatus.mockImplementation(async (_userId, deviceId) => { if (deviceId === alicesDevice.device_id) { - return new DeviceTrustLevel(true, true, false, false); + return new DeviceVerificationStatus({ crossSigningVerified: true, localVerified: true }); } - return new DeviceTrustLevel(false, false, false, false); + return new DeviceVerificationStatus({}); }); const { getByTestId } = render(getComponent()); diff --git a/test/toasts/UnverifiedSessionToast-test.tsx b/test/toasts/UnverifiedSessionToast-test.tsx index a65f7dec79f..2799f0f7916 100644 --- a/test/toasts/UnverifiedSessionToast-test.tsx +++ b/test/toasts/UnverifiedSessionToast-test.tsx @@ -18,9 +18,8 @@ import React from "react"; import { render, RenderResult, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { mocked, Mocked } from "jest-mock"; -import { IMyDevice, MatrixClient } from "matrix-js-sdk/src/matrix"; +import { CryptoApi, DeviceVerificationStatus, IMyDevice, MatrixClient } from "matrix-js-sdk/src/matrix"; import { DeviceInfo } from "matrix-js-sdk/src/crypto/deviceinfo"; -import { DeviceTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; import dis from "../../src/dispatcher/dispatcher"; import { showToast } from "../../src/toasts/UnverifiedSessionToast"; @@ -55,7 +54,11 @@ describe("UnverifiedSessionToast", () => { return null; }); - client.checkDeviceTrust.mockReturnValue(new DeviceTrustLevel(true, false, false, false)); + client.getCrypto.mockReturnValue({ + getDeviceVerificationStatus: jest + .fn() + .mockResolvedValue(new DeviceVerificationStatus({ crossSigningVerified: true })), + } as unknown as CryptoApi); jest.spyOn(dis, "dispatch"); jest.spyOn(DeviceListener.sharedInstance(), "dismissUnverifiedSessions"); }); From ef47246b0d0522b2be6c604354ad70713aef7695 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 14 Apr 2023 18:38:17 +0100 Subject: [PATCH 05/12] EventTile: use `getDeviceVerificationStatus` This is ok except we need an "unmounted" guard because it's an old-style componment --- src/components/views/rooms/EventTile.tsx | 14 +++++++-- .../components/views/rooms/EventTile-test.tsx | 31 ++++++++++++------- test/test-utils/test-utils.ts | 1 + 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/components/views/rooms/EventTile.tsx b/src/components/views/rooms/EventTile.tsx index fb44993553a..a6393b7c825 100644 --- a/src/components/views/rooms/EventTile.tsx +++ b/src/components/views/rooms/EventTile.tsx @@ -265,6 +265,8 @@ export class UnwrappedEventTile extends React.Component public static contextType = RoomContext; public context!: React.ContextType; + private unmounted = false; + public constructor(props: EventTileProps, context: React.ContextType) { super(props, context); @@ -420,6 +422,7 @@ export class UnwrappedEventTile extends React.Component this.props.mxEvent.removeListener(MatrixEventEvent.RelationsCreated, this.onReactionsCreated); } this.props.mxEvent.off(ThreadEvent.Update, this.updateThread); + this.unmounted = false; } public componentDidUpdate(prevProps: Readonly, prevState: Readonly): void { @@ -561,7 +564,7 @@ export class UnwrappedEventTile extends React.Component this.verifyEvent(); }; - private verifyEvent(): void { + private async verifyEvent(): Promise { // if the event was edited, show the verification info for the edit, not // the original const mxEvent = this.props.mxEvent.replacingEvent() ?? this.props.mxEvent; @@ -590,7 +593,14 @@ export class UnwrappedEventTile extends React.Component } const eventSenderTrust = - encryptionInfo.sender && MatrixClientPeg.get().checkDeviceTrust(senderId, encryptionInfo.sender.deviceId); + senderId && + encryptionInfo.sender && + (await MatrixClientPeg.get() + .getCrypto() + ?.getDeviceVerificationStatus(senderId, encryptionInfo.sender.deviceId)); + + if (this.unmounted) return; + if (!eventSenderTrust) { this.setState({ verified: E2EState.Unknown }); return; diff --git a/test/components/views/rooms/EventTile-test.tsx b/test/components/views/rooms/EventTile-test.tsx index abbb3f70b49..5f067088aad 100644 --- a/test/components/views/rooms/EventTile-test.tsx +++ b/test/components/views/rooms/EventTile-test.tsx @@ -19,7 +19,7 @@ import { render, waitFor, screen, act, fireEvent } from "@testing-library/react" import { mocked } from "jest-mock"; import { EventType } from "matrix-js-sdk/src/@types/event"; import { MatrixClient, PendingEventOrdering } from "matrix-js-sdk/src/client"; -import { TweakName } from "matrix-js-sdk/src/matrix"; +import { CryptoApi, TweakName } from "matrix-js-sdk/src/matrix"; import { MatrixEvent } from "matrix-js-sdk/src/models/event"; import { NotificationCountType, Room } from "matrix-js-sdk/src/models/room"; import { DeviceTrustLevel, UserTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; @@ -30,7 +30,7 @@ import EventTile, { EventTileProps } from "../../../../src/components/views/room import MatrixClientContext from "../../../../src/contexts/MatrixClientContext"; import RoomContext, { TimelineRenderingType } from "../../../../src/contexts/RoomContext"; import { MatrixClientPeg } from "../../../../src/MatrixClientPeg"; -import { getRoomContext, mkEncryptedEvent, mkEvent, mkMessage, stubClient } from "../../../test-utils"; +import { flushPromises, getRoomContext, mkEncryptedEvent, mkEvent, mkMessage, stubClient } from "../../../test-utils"; import { mkThread } from "../../../test-utils/threads"; import DMRoomMap from "../../../../src/utils/DMRoomMap"; import dis from "../../../../src/dispatcher/dispatcher"; @@ -221,13 +221,16 @@ describe("EventTile", () => { // a version of checkDeviceTrust which says that TRUSTED_DEVICE is trusted, and others are not. const trustedDeviceTrustLevel = DeviceTrustLevel.fromUserTrustLevel(trustedUserTrustLevel, true, false); const untrustedDeviceTrustLevel = DeviceTrustLevel.fromUserTrustLevel(trustedUserTrustLevel, false, false); - client.checkDeviceTrust = (userId, deviceId) => { - if (deviceId === TRUSTED_DEVICE.deviceId) { - return trustedDeviceTrustLevel; - } else { - return untrustedDeviceTrustLevel; - } - }; + const mockCrypto = { + getDeviceVerificationStatus: async (userId: string, deviceId: string) => { + if (deviceId === TRUSTED_DEVICE.deviceId) { + return trustedDeviceTrustLevel; + } else { + return untrustedDeviceTrustLevel; + } + }, + } as unknown as CryptoApi; + client.getCrypto = () => mockCrypto; }); it("shows a warning for an event from an unverified device", async () => { @@ -243,6 +246,7 @@ describe("EventTile", () => { } as IEncryptedEventInfo); const { container } = getComponent(); + await act(flushPromises); const eventTiles = container.getElementsByClassName("mx_EventTile"); expect(eventTiles).toHaveLength(1); @@ -270,6 +274,7 @@ describe("EventTile", () => { } as IEncryptedEventInfo); const { container } = getComponent(); + await act(flushPromises); const eventTiles = container.getElementsByClassName("mx_EventTile"); expect(eventTiles).toHaveLength(1); @@ -295,6 +300,7 @@ describe("EventTile", () => { } as IEncryptedEventInfo); const { container } = getComponent(); + await act(flushPromises); const eventTiles = container.getElementsByClassName("mx_EventTile"); expect(eventTiles).toHaveLength(1); @@ -317,8 +323,9 @@ describe("EventTile", () => { sender: UNTRUSTED_DEVICE, } as IEncryptedEventInfo); - act(() => { + await act(async () => { mxEvent.makeReplaced(replacementEvent); + flushPromises(); }); // check it was updated @@ -345,6 +352,7 @@ describe("EventTile", () => { } as IEncryptedEventInfo); const { container } = getComponent(); + await act(flushPromises); const eventTiles = container.getElementsByClassName("mx_EventTile"); expect(eventTiles).toHaveLength(1); @@ -363,8 +371,9 @@ describe("EventTile", () => { event: true, }); - act(() => { + await act(async () => { mxEvent.makeReplaced(replacementEvent); + await flushPromises(); }); // check it was updated diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index ddd6b091257..c4b21e00e11 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -234,6 +234,7 @@ export function createTestClient(): MatrixClient { }), searchUserDirectory: jest.fn().mockResolvedValue({ limited: false, results: [] }), + getCrypto: jest.fn(), } as unknown as MatrixClient; client.reEmitter = new ReEmitter(client); From 9d2d47b7b2d93c9d3537415aecdead4604f7fc24 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 14 Apr 2023 18:43:40 +0100 Subject: [PATCH 06/12] UserInfo step 1: `getE2EStatus` --- src/components/views/right_panel/UserInfo.tsx | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/components/views/right_panel/UserInfo.tsx b/src/components/views/right_panel/UserInfo.tsx index 270e837a703..a69afac91d5 100644 --- a/src/components/views/right_panel/UserInfo.tsx +++ b/src/components/views/right_panel/UserInfo.tsx @@ -79,6 +79,7 @@ import PosthogTrackers from "../../../PosthogTrackers"; import { ViewRoomPayload } from "../../../dispatcher/payloads/ViewRoomPayload"; import { DirectoryMember, startDmOnFirstMessage } from "../../../utils/direct-messages"; import { SdkContextClass } from "../../../contexts/SDKContext"; +import { asyncSome } from "../../../utils/arrays"; export interface IDevice extends DeviceInfo { ambiguous?: boolean; @@ -101,22 +102,22 @@ export const disambiguateDevices = (devices: IDevice[]): void => { } }; -export const getE2EStatus = (cli: MatrixClient, userId: string, devices: IDevice[]): E2EStatus => { +export const getE2EStatus = async (cli: MatrixClient, userId: string, devices: IDevice[]): Promise => { const isMe = userId === cli.getUserId(); const userTrust = cli.checkUserTrust(userId); if (!userTrust.isCrossSigningVerified()) { return userTrust.wasCrossSigningVerified() ? E2EStatus.Warning : E2EStatus.Normal; } - const anyDeviceUnverified = devices.some((device) => { + const anyDeviceUnverified = await asyncSome(devices, async (device) => { const { deviceId } = device; // For your own devices, we use the stricter check of cross-signing // verification to encourage everyone to trust their own devices via // cross-signing so that other users can then safely trust you. // For other people's devices, the more general verified check that // includes locally verified devices can be used. - const deviceTrust = cli.checkDeviceTrust(userId, deviceId); - return isMe ? !deviceTrust.isCrossSigningVerified() : !deviceTrust.isVerified(); + const deviceTrust = await cli.getCrypto()?.getDeviceVerificationStatus(userId, deviceId); + return isMe ? !deviceTrust?.crossSigningVerified : !deviceTrust?.isVerified(); }); return anyDeviceUnverified ? E2EStatus.Warning : E2EStatus.Verified; }; @@ -1611,10 +1612,13 @@ const UserInfo: React.FC = ({ user, room, onClose, phase = RightPanelPha const isRoomEncrypted = useIsEncrypted(cli, room); const devices = useDevices(user.userId) ?? []; - let e2eStatus: E2EStatus | undefined; - if (isRoomEncrypted && devices) { - e2eStatus = getE2EStatus(cli, user.userId, devices); - } + const e2eStatus = useAsyncMemo(async () => { + if (!isRoomEncrypted || !devices) { + return undefined; + } else { + return await getE2EStatus(cli, user.userId, devices); + } + }, [cli, isRoomEncrypted, user.userId, devices]); const classes = ["mx_UserInfo"]; From edfad2b602efe5db9014ab0e571d8870ea60411b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 14 Apr 2023 18:48:53 +0100 Subject: [PATCH 07/12] UserInfo step 2: `DeviceItem` --- src/components/views/right_panel/UserInfo.tsx | 25 +++++++---- .../views/right_panel/UserInfo-test.tsx | 42 ++++++++++++++----- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/src/components/views/right_panel/UserInfo.tsx b/src/components/views/right_panel/UserInfo.tsx index a69afac91d5..ebaa1cbf9d8 100644 --- a/src/components/views/right_panel/UserInfo.tsx +++ b/src/components/views/right_panel/UserInfo.tsx @@ -162,14 +162,20 @@ function useHasCrossSigningKeys( export function DeviceItem({ userId, device }: { userId: string; device: IDevice }): JSX.Element { const cli = useContext(MatrixClientContext); const isMe = userId === cli.getUserId(); - const deviceTrust = cli.checkDeviceTrust(userId, device.deviceId); const userTrust = cli.checkUserTrust(userId); - // For your own devices, we use the stricter check of cross-signing - // verification to encourage everyone to trust their own devices via - // cross-signing so that other users can then safely trust you. - // For other people's devices, the more general verified check that - // includes locally verified devices can be used. - const isVerified = isMe ? deviceTrust.isCrossSigningVerified() : deviceTrust.isVerified(); + + /** is the device verified? */ + const isVerified = useAsyncMemo(async () => { + const deviceTrust = await cli.getCrypto()?.getDeviceVerificationStatus(userId, device.deviceId); + if (!deviceTrust) return false; + + // For your own devices, we use the stricter check of cross-signing + // verification to encourage everyone to trust their own devices via + // cross-signing so that other users can then safely trust you. + // For other people's devices, the more general verified check that + // includes locally verified devices can be used. + return isMe ? deviceTrust.crossSigningVerified : deviceTrust.isVerified(); + }, [cli, userId, device]); const classes = classNames("mx_UserInfo_device", { mx_UserInfo_device_verified: isVerified, @@ -200,7 +206,10 @@ export function DeviceItem({ userId, device }: { userId: string; device: IDevice let trustedLabel: string | undefined; if (userTrust.isVerified()) trustedLabel = isVerified ? _t("Trusted") : _t("Not trusted"); - if (isVerified) { + if (isVerified === undefined) { + // we're still deciding if the device is verified + return
; + } else if (isVerified) { return (
diff --git a/test/components/views/right_panel/UserInfo-test.tsx b/test/components/views/right_panel/UserInfo-test.tsx index 8e538577233..7b4a9047205 100644 --- a/test/components/views/right_panel/UserInfo-test.tsx +++ b/test/components/views/right_panel/UserInfo-test.tsx @@ -18,7 +18,16 @@ import React from "react"; import { fireEvent, render, screen, waitFor, cleanup, act, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { Mocked, mocked } from "jest-mock"; -import { Room, User, MatrixClient, RoomMember, MatrixEvent, EventType } from "matrix-js-sdk/src/matrix"; +import { + Room, + User, + MatrixClient, + RoomMember, + MatrixEvent, + EventType, + CryptoApi, + DeviceVerificationStatus, +} from "matrix-js-sdk/src/matrix"; import { Phase, VerificationRequest } from "matrix-js-sdk/src/crypto/verification/request/VerificationRequest"; import { DeviceTrustLevel, UserTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; import { DeviceInfo } from "matrix-js-sdk/src/crypto/deviceinfo"; @@ -79,6 +88,7 @@ const defaultUser = new User(defaultUserId); let mockRoom: Mocked; let mockSpace: Mocked; let mockClient: Mocked; +let mockCrypto: Mocked; beforeEach(() => { mockRoom = mocked({ @@ -115,6 +125,10 @@ beforeEach(() => { getEventReadUpTo: jest.fn(), } as unknown as Room); + mockCrypto = mocked({ + getDeviceVerificationStatus: jest.fn(), + } as unknown as CryptoApi); + mockClient = mocked({ getUser: jest.fn(), isGuest: jest.fn().mockReturnValue(false), @@ -141,6 +155,7 @@ beforeEach(() => { setPowerLevel: jest.fn(), downloadKeys: jest.fn(), getStoredDevicesForUser: jest.fn(), + getCrypto: jest.fn().mockReturnValue(mockCrypto), } as unknown as MatrixClient); jest.spyOn(MatrixClientPeg, "get").mockReturnValue(mockClient); @@ -370,10 +385,10 @@ describe("", () => { mockClient.checkUserTrust.mockReturnValue({ isVerified: () => isVerified } as UserTrustLevel); }; const setMockDeviceTrust = (isVerified = false, isCrossSigningVerified = false) => { - mockClient.checkDeviceTrust.mockReturnValue({ + mockCrypto.getDeviceVerificationStatus.mockResolvedValue({ isVerified: () => isVerified, - isCrossSigningVerified: () => isCrossSigningVerified, - } as DeviceTrustLevel); + crossSigningVerified: isCrossSigningVerified, + } as DeviceVerificationStatus); }; const mockVerifyDevice = jest.spyOn(mockVerification, "verifyDevice"); @@ -384,7 +399,7 @@ describe("", () => { }); afterEach(() => { - mockClient.checkDeviceTrust.mockReset(); + mockCrypto.getDeviceVerificationStatus.mockReset(); mockClient.checkUserTrust.mockReset(); mockVerifyDevice.mockClear(); }); @@ -393,32 +408,36 @@ describe("", () => { mockVerifyDevice.mockRestore(); }); - it("with unverified user and device, displays button without a label", () => { + it("with unverified user and device, displays button without a label", async () => { renderComponent(); + await act(flushPromises); expect(screen.getByRole("button", { name: device.getDisplayName()! })).toBeInTheDocument; expect(screen.queryByText(/trusted/i)).not.toBeInTheDocument(); }); - it("with verified user only, displays button with a 'Not trusted' label", () => { + it("with verified user only, displays button with a 'Not trusted' label", async () => { setMockUserTrust(true); renderComponent(); + await act(flushPromises); expect(screen.getByRole("button", { name: `${device.getDisplayName()} Not trusted` })).toBeInTheDocument; }); - it("with verified device only, displays no button without a label", () => { + it("with verified device only, displays no button without a label", async () => { setMockDeviceTrust(true); renderComponent(); + await act(flushPromises); expect(screen.getByText(device.getDisplayName()!)).toBeInTheDocument(); expect(screen.queryByText(/trusted/)).not.toBeInTheDocument(); }); - it("when userId is the same as userId from client, uses isCrossSigningVerified to determine if button is shown", () => { + it("when userId is the same as userId from client, uses isCrossSigningVerified to determine if button is shown", async () => { mockClient.getSafeUserId.mockReturnValueOnce(defaultUserId); mockClient.getUserId.mockReturnValueOnce(defaultUserId); renderComponent(); + await act(flushPromises); // set trust to be false for isVerified, true for isCrossSigningVerified setMockDeviceTrust(false, true); @@ -428,10 +447,11 @@ describe("", () => { expect(screen.getByText(device.getDisplayName()!)).toBeInTheDocument(); }); - it("with verified user and device, displays no button and a 'Trusted' label", () => { + it("with verified user and device, displays no button and a 'Trusted' label", async () => { setMockUserTrust(true); setMockDeviceTrust(true); renderComponent(); + await act(flushPromises); expect(screen.queryByRole("button")).not.toBeInTheDocument; expect(screen.getByText(device.getDisplayName()!)).toBeInTheDocument(); @@ -441,6 +461,7 @@ describe("", () => { it("does not call verifyDevice if client.getUser returns null", async () => { mockClient.getUser.mockReturnValueOnce(null); renderComponent(); + await act(flushPromises); const button = screen.getByRole("button", { name: device.getDisplayName()! }); expect(button).toBeInTheDocument; @@ -455,6 +476,7 @@ describe("", () => { // even more mocking mockClient.isGuest.mockReturnValueOnce(true); renderComponent(); + await act(flushPromises); const button = screen.getByRole("button", { name: device.getDisplayName()! }); expect(button).toBeInTheDocument; From 74d56967cc3c3797c4966abac78a7f60676b6699 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 14 Apr 2023 18:50:21 +0100 Subject: [PATCH 08/12] UserInfo step 3: `DevicesSection` --- src/components/views/right_panel/UserInfo.tsx | 14 ++++++++------ src/i18n/strings/en_EN.json | 1 - .../components/views/right_panel/UserInfo-test.tsx | 4 +--- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/components/views/right_panel/UserInfo.tsx b/src/components/views/right_panel/UserInfo.tsx index ebaa1cbf9d8..49f9b5ab3a6 100644 --- a/src/components/views/right_panel/UserInfo.tsx +++ b/src/components/views/right_panel/UserInfo.tsx @@ -242,15 +242,17 @@ function DevicesSection({ const [isExpanded, setExpanded] = useState(false); - if (loading) { + const deviceTrusts = useAsyncMemo(() => { + const cryptoApi = cli.getCrypto(); + if (!cryptoApi) return Promise.resolve(undefined); + return Promise.all(devices.map((d) => cryptoApi.getDeviceVerificationStatus(userId, d.deviceId))); + }, [cli, userId, devices]); + + if (loading || deviceTrusts === undefined) { // still loading return ; } - if (devices === null) { - return

{_t("Unable to load session list")}

; - } const isMe = userId === cli.getUserId(); - const deviceTrusts = devices.map((d) => cli.checkDeviceTrust(userId, d.deviceId)); let expandSectionDevices: IDevice[] = []; const unverifiedDevices: IDevice[] = []; @@ -268,7 +270,7 @@ function DevicesSection({ // cross-signing so that other users can then safely trust you. // For other people's devices, the more general verified check that // includes locally verified devices can be used. - const isVerified = isMe ? deviceTrust.isCrossSigningVerified() : deviceTrust.isVerified(); + const isVerified = deviceTrust && (isMe ? deviceTrust.crossSigningVerified : deviceTrust.isVerified()); if (isVerified) { expandSectionDevices.push(device); diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index dd9afaf5d75..caca153edb0 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -2252,7 +2252,6 @@ "Room settings": "Room settings", "Trusted": "Trusted", "Not trusted": "Not trusted", - "Unable to load session list": "Unable to load session list", "%(count)s verified sessions|other": "%(count)s verified sessions", "%(count)s verified sessions|one": "1 verified session", "Hide verified sessions": "Hide verified sessions", diff --git a/test/components/views/right_panel/UserInfo-test.tsx b/test/components/views/right_panel/UserInfo-test.tsx index 7b4a9047205..e7f3feb70cf 100644 --- a/test/components/views/right_panel/UserInfo-test.tsx +++ b/test/components/views/right_panel/UserInfo-test.tsx @@ -29,7 +29,7 @@ import { DeviceVerificationStatus, } from "matrix-js-sdk/src/matrix"; import { Phase, VerificationRequest } from "matrix-js-sdk/src/crypto/verification/request/VerificationRequest"; -import { DeviceTrustLevel, UserTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; +import { UserTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; import { DeviceInfo } from "matrix-js-sdk/src/crypto/deviceinfo"; import { defer } from "matrix-js-sdk/src/utils"; @@ -148,7 +148,6 @@ beforeEach(() => { currentState: { on: jest.fn(), }, - checkDeviceTrust: jest.fn(), checkUserTrust: jest.fn(), getRoom: jest.fn(), credentials: {}, @@ -266,7 +265,6 @@ describe("", () => { beforeEach(() => { mockClient.isCryptoEnabled.mockReturnValue(true); mockClient.checkUserTrust.mockReturnValue(new UserTrustLevel(false, false, false)); - mockClient.checkDeviceTrust.mockReturnValue(new DeviceTrustLevel(false, false, false, false)); const device1 = DeviceInfo.fromStorage( { From 3c88d94cf5660a77a2204c1530b5b7b6578918f4 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 17 Apr 2023 23:23:04 +0100 Subject: [PATCH 09/12] Add some comments on the last use of checkDeviceTrust --- src/stores/SetupEncryptionStore.ts | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/stores/SetupEncryptionStore.ts b/src/stores/SetupEncryptionStore.ts index d2d2ee0a4d8..46c0193f94f 100644 --- a/src/stores/SetupEncryptionStore.ts +++ b/src/stores/SetupEncryptionStore.ts @@ -109,14 +109,20 @@ export class SetupEncryptionStore extends EventEmitter { const dehydratedDevice = await cli.getDehydratedDevice(); const ownUserId = cli.getUserId()!; const crossSigningInfo = cli.getStoredCrossSigningForUser(ownUserId); - this.hasDevicesToVerifyAgainst = cli - .getStoredDevicesForUser(ownUserId) - .some( - (device) => - device.getIdentityKey() && - (!dehydratedDevice || device.deviceId != dehydratedDevice.device_id) && - crossSigningInfo?.checkDeviceTrust(crossSigningInfo, device, false, true).isCrossSigningVerified(), - ); + this.hasDevicesToVerifyAgainst = cli.getStoredDevicesForUser(ownUserId).some((device) => { + if (!device.getIdentityKey() || (dehydratedDevice && device.deviceId == dehydratedDevice?.device_id)) { + return false; + } + // check if the device is signed by the cross-signing key stored for our user. Note that this is + // *different* to calling `cryptoApi.getDeviceVerificationStatus`, because even if we have stored + // a cross-signing key for our user, we don't necessarily trust it yet (In legacy Crypto, we have not + // yet imported it into `Crypto.crossSigningInfo`, which for maximal confusion is a different object to + // `Crypto.getStoredCrossSigningForUser(ownUserId)`). + // + // TODO: figure out wtf to to here for rust-crypto + const verificationStatus = crossSigningInfo?.checkDeviceTrust(crossSigningInfo, device, false, true); + return !!verificationStatus?.isCrossSigningVerified(); + }); this.phase = Phase.Intro; this.emit("update"); From 1cc6e40d414e2d1cd711d4df512185398a39f60d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 17 Apr 2023 13:41:54 +0100 Subject: [PATCH 10/12] remove unsed stub from createTestClient --- test/test-utils/test-utils.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index c4b21e00e11..58668fa470b 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -99,7 +99,6 @@ export function createTestClient(): MatrixClient { getDevice: jest.fn(), getDeviceId: jest.fn().mockReturnValue("ABCDEFGHI"), getStoredCrossSigningForUser: jest.fn(), - checkDeviceTrust: jest.fn(), getStoredDevice: jest.fn(), requestVerification: jest.fn(), deviceId: "ABCDEFGHI", From 8b72ebfd4977c17e6a854d815cb9cdacfbc358e1 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 24 Apr 2023 12:51:40 +0100 Subject: [PATCH 11/12] Apply suggestions from code review --- src/DeviceListener.ts | 2 +- src/components/views/right_panel/UserInfo.tsx | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/DeviceListener.ts b/src/DeviceListener.ts index e53386b0a0d..ba3fed3d5f4 100644 --- a/src/DeviceListener.ts +++ b/src/DeviceListener.ts @@ -325,7 +325,7 @@ export default class DeviceListener { const deviceTrust = await cli .getCrypto() - ?.getDeviceVerificationStatus(cli.getUserId()!, device.deviceId!); + !.getDeviceVerificationStatus(cli.getUserId()!, device.deviceId!); if (!deviceTrust?.crossSigningVerified && !this.dismissed.has(device.deviceId)) { if (this.ourDeviceIdsAtStart?.has(device.deviceId)) { oldUnverifiedDeviceIds.add(device.deviceId); diff --git a/src/components/views/right_panel/UserInfo.tsx b/src/components/views/right_panel/UserInfo.tsx index 49f9b5ab3a6..f6f90313191 100644 --- a/src/components/views/right_panel/UserInfo.tsx +++ b/src/components/views/right_panel/UserInfo.tsx @@ -1626,9 +1626,8 @@ const UserInfo: React.FC = ({ user, room, onClose, phase = RightPanelPha const e2eStatus = useAsyncMemo(async () => { if (!isRoomEncrypted || !devices) { return undefined; - } else { - return await getE2EStatus(cli, user.userId, devices); } + return await getE2EStatus(cli, user.userId, devices); }, [cli, isRoomEncrypted, user.userId, devices]); const classes = ["mx_UserInfo"]; From 02f8263f116cfbdd557c94b585906d3c440e38d8 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 24 Apr 2023 13:34:26 +0100 Subject: [PATCH 12/12] Update DeviceListener.ts --- src/DeviceListener.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/DeviceListener.ts b/src/DeviceListener.ts index ba3fed3d5f4..afec1f62f47 100644 --- a/src/DeviceListener.ts +++ b/src/DeviceListener.ts @@ -324,8 +324,8 @@ export default class DeviceListener { if (device.deviceId === cli.deviceId) continue; const deviceTrust = await cli - .getCrypto() - !.getDeviceVerificationStatus(cli.getUserId()!, device.deviceId!); + .getCrypto()! + .getDeviceVerificationStatus(cli.getUserId()!, device.deviceId!); if (!deviceTrust?.crossSigningVerified && !this.dismissed.has(device.deviceId)) { if (this.ourDeviceIdsAtStart?.has(device.deviceId)) { oldUnverifiedDeviceIds.add(device.deviceId);