From 70b87f8bde7323949ac1dd5417cf5795f1e02a98 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 14 Apr 2023 10:46:37 +0100 Subject: [PATCH] Simplify `isDeviceVerified` definitions (#10594) * Simplify `isDeviceVerified` definitions Currently, we have two similar but different definitions of `isDeviceVerified`, and they both do a lot of wrangling that relies on js-sdk internals. We can simplify it a lot by just calling `MatrixClientPeg.checkDeviceTrust`. * fix tests * more test fixes --- .../views/settings/DevicesPanel.tsx | 2 +- .../views/settings/devices/useOwnDevices.ts | 36 +++---------------- src/toasts/UnverifiedSessionToast.tsx | 2 +- src/utils/device/isDeviceVerified.ts | 22 +++++++----- .../views/settings/DevicesPanel-test.tsx | 4 +-- .../tabs/user/SessionManagerTab-test.tsx | 29 +++++++-------- test/test-utils/test-utils.ts | 1 + test/toasts/UnverifiedSessionToast-test.tsx | 8 ++--- 8 files changed, 37 insertions(+), 67 deletions(-) diff --git a/src/components/views/settings/DevicesPanel.tsx b/src/components/views/settings/DevicesPanel.tsx index cf7506caa5..603438e7e5 100644 --- a/src/components/views/settings/DevicesPanel.tsx +++ b/src/components/views/settings/DevicesPanel.tsx @@ -120,7 +120,7 @@ export default class DevicesPanel extends React.Component { } private isDeviceVerified(device: IMyDevice): boolean | null { - return isDeviceVerified(device, this.context); + return isDeviceVerified(this.context, device.device_id); } private onDeviceSelectionToggled = (device: IMyDevice): void => { diff --git a/src/components/views/settings/devices/useOwnDevices.ts b/src/components/views/settings/devices/useOwnDevices.ts index 77b8dd9663..ffd0e00eba 100644 --- a/src/components/views/settings/devices/useOwnDevices.ts +++ b/src/components/views/settings/devices/useOwnDevices.ts @@ -26,7 +26,6 @@ import { PUSHER_ENABLED, UNSTABLE_MSC3852_LAST_SEEN_UA, } from "matrix-js-sdk/src/matrix"; -import { CrossSigningInfo } from "matrix-js-sdk/src/crypto/CrossSigning"; import { VerificationRequest } from "matrix-js-sdk/src/crypto/verification/request/VerificationRequest"; import { MatrixError } from "matrix-js-sdk/src/http-api"; import { logger } from "matrix-js-sdk/src/logger"; @@ -39,27 +38,7 @@ import { getDeviceClientInformation, pruneClientInformation } from "../../../../ import { DevicesDictionary, ExtendedDevice, ExtendedDeviceAppInfo } from "./types"; import { useEventEmitter } from "../../../../hooks/useEventEmitter"; import { parseUserAgent } from "../../../../utils/device/parseUserAgent"; - -const isDeviceVerified = ( - matrixClient: MatrixClient, - crossSigningInfo: CrossSigningInfo, - device: IMyDevice, -): boolean | null => { - try { - const userId = matrixClient.getUserId(); - if (!userId) { - throw new Error("No user id"); - } - const deviceInfo = matrixClient.getStoredDevice(userId, device.device_id); - if (!deviceInfo) { - throw new Error("No device info available"); - } - return crossSigningInfo.checkDeviceTrust(crossSigningInfo, deviceInfo, false, true).isCrossSigningVerified(); - } catch (error) { - logger.error("Error getting device cross-signing info", error); - return null; - } -}; +import { isDeviceVerified } from "../../../../utils/device/isDeviceVerified"; const parseDeviceExtendedInformation = (matrixClient: MatrixClient, device: IMyDevice): ExtendedDeviceAppInfo => { const { name, version, url } = getDeviceClientInformation(matrixClient, device.device_id); @@ -71,20 +50,15 @@ const parseDeviceExtendedInformation = (matrixClient: MatrixClient, device: IMyD }; }; -const fetchDevicesWithVerification = async ( - matrixClient: MatrixClient, - userId: string, -): Promise => { +const fetchDevicesWithVerification = async (matrixClient: MatrixClient): Promise => { const { devices } = await matrixClient.getDevices(); - const crossSigningInfo = matrixClient.getStoredCrossSigningForUser(userId); - const devicesDict = devices.reduce( (acc, device: IMyDevice) => ({ ...acc, [device.device_id]: { ...device, - isVerified: isDeviceVerified(matrixClient, crossSigningInfo, device), + isVerified: isDeviceVerified(matrixClient, device.device_id), ...parseDeviceExtendedInformation(matrixClient, device), ...parseUserAgent(device[UNSTABLE_MSC3852_LAST_SEEN_UA.name]), }, @@ -138,7 +112,7 @@ export const useOwnDevices = (): DevicesState => { const refreshDevices = useCallback(async (): Promise => { setIsLoadingDeviceList(true); try { - const devices = await fetchDevicesWithVerification(matrixClient, userId); + const devices = await fetchDevicesWithVerification(matrixClient); setDevices(devices); const { pushers } = await matrixClient.getPushers(); @@ -165,7 +139,7 @@ export const useOwnDevices = (): DevicesState => { } setIsLoadingDeviceList(false); } - }, [matrixClient, userId]); + }, [matrixClient]); useEffect(() => { refreshDevices(); diff --git a/src/toasts/UnverifiedSessionToast.tsx b/src/toasts/UnverifiedSessionToast.tsx index 7efa592919..8d619d8767 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(device, cli), + isVerified: isDeviceVerified(cli, deviceId), deviceType: DeviceType.Unknown, }; diff --git a/src/utils/device/isDeviceVerified.ts b/src/utils/device/isDeviceVerified.ts index 4f600b785d..0f8fdb6082 100644 --- a/src/utils/device/isDeviceVerified.ts +++ b/src/utils/device/isDeviceVerified.ts @@ -14,17 +14,21 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { IMyDevice, MatrixClient } from "matrix-js-sdk/src/matrix"; +import { MatrixClient } from "matrix-js-sdk/src/matrix"; -export const isDeviceVerified = (device: IMyDevice, client: MatrixClient): boolean | null => { +/** + * Check if one of our own devices is verified via cross signing + * + * @param client - reference to the MatrixClient + * @param deviceId - ID of the device to be checked + * + * @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 => { try { - const crossSigningInfo = client.getStoredCrossSigningForUser(client.getSafeUserId()); - const deviceInfo = client.getStoredDevice(client.getSafeUserId(), device.device_id); - - // no cross-signing or device info available - if (!crossSigningInfo || !deviceInfo) return false; - - return crossSigningInfo.checkDeviceTrust(crossSigningInfo, deviceInfo, false, true).isCrossSigningVerified(); + const trustLevel = client.checkDeviceTrust(client.getSafeUserId(), deviceId); + return trustLevel.isCrossSigningVerified(); } catch (e) { console.error("Error getting device cross-signing info", e); return null; diff --git a/test/components/views/settings/DevicesPanel-test.tsx b/test/components/views/settings/DevicesPanel-test.tsx index 391472f027..b7a0ec6a25 100644 --- a/test/components/views/settings/DevicesPanel-test.tsx +++ b/test/components/views/settings/DevicesPanel-test.tsx @@ -15,10 +15,10 @@ limitations under the License. */ import React from "react"; import { act, fireEvent, render } from "@testing-library/react"; -import { CrossSigningInfo } from "matrix-js-sdk/src/crypto/CrossSigning"; 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"; @@ -34,7 +34,7 @@ describe("", () => { getDevices: jest.fn(), getDeviceId: jest.fn().mockReturnValue(device1.device_id), deleteMultipleDevices: jest.fn(), - getStoredCrossSigningForUser: jest.fn().mockReturnValue(new CrossSigningInfo(userId, {}, {})), + checkDeviceTrust: jest.fn().mockReturnValue(new DeviceTrustLevel(false, false, false, false)), getStoredDevice: jest.fn().mockReturnValue(new DeviceInfo("id")), generateClientSecret: jest.fn(), getPushers: jest.fn(), diff --git a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx index bb96fe4e00..b164827186 100644 --- a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -74,16 +74,13 @@ describe("", () => { last_seen_ts: Date.now() - (INACTIVE_DEVICE_AGE_MS + 1000), }; - const mockCrossSigningInfo = { - checkDeviceTrust: jest.fn(), - }; const mockVerificationRequest = { cancel: jest.fn(), on: jest.fn(), } as unknown as VerificationRequest; const mockClient = getMockClientWithEventEmitter({ ...mockClientMethodsUser(aliceId), - getStoredCrossSigningForUser: jest.fn().mockReturnValue(mockCrossSigningInfo), + checkDeviceTrust: jest.fn(), getDevices: jest.fn(), getStoredDevice: jest.fn(), getDeviceId: jest.fn().mockReturnValue(deviceId), @@ -174,9 +171,7 @@ describe("", () => { const device = [alicesDevice, alicesMobileDevice].find((device) => device.device_id === id); return device ? new DeviceInfo(device.device_id) : null; }); - mockCrossSigningInfo.checkDeviceTrust - .mockReset() - .mockReturnValue(new DeviceTrustLevel(false, false, false, false)); + mockClient.checkDeviceTrust.mockReset().mockReturnValue(new DeviceTrustLevel(false, false, false, false)); mockClient.getDevices.mockReset().mockResolvedValue({ devices: [alicesDevice, alicesMobileDevice] }); @@ -226,12 +221,12 @@ describe("", () => { }); it("does not fail when checking device verification fails", async () => { - const logSpy = jest.spyOn(logger, "error").mockImplementation(() => {}); + const logSpy = jest.spyOn(console, "error").mockImplementation(() => {}); mockClient.getDevices.mockResolvedValue({ devices: [alicesDevice, alicesMobileDevice], }); const noCryptoError = new Error("End-to-end encryption disabled"); - mockClient.getStoredDevice.mockImplementation(() => { + mockClient.checkDeviceTrust.mockImplementation(() => { throw noCryptoError; }); render(getComponent()); @@ -241,8 +236,8 @@ describe("", () => { }); // called for each device despite error - expect(mockClient.getStoredDevice).toHaveBeenCalledWith(aliceId, alicesDevice.device_id); - expect(mockClient.getStoredDevice).toHaveBeenCalledWith(aliceId, alicesMobileDevice.device_id); + 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); }); @@ -251,7 +246,7 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice], }); mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); - mockCrossSigningInfo.checkDeviceTrust.mockImplementation((_userId, { deviceId }) => { + mockClient.checkDeviceTrust.mockImplementation((_userId, deviceId) => { // alices device is trusted if (deviceId === alicesDevice.device_id) { return new DeviceTrustLevel(true, true, false, false); @@ -270,7 +265,7 @@ describe("", () => { await flushPromises(); }); - expect(mockCrossSigningInfo.checkDeviceTrust).toHaveBeenCalledTimes(3); + expect(mockClient.checkDeviceTrust).toHaveBeenCalledTimes(3); expect( getByTestId(`device-tile-${alicesDevice.device_id}`).querySelector('[aria-label="Verified"]'), ).toBeTruthy(); @@ -423,7 +418,7 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice], }); mockClient.getStoredDevice.mockImplementation(() => new DeviceInfo(alicesDevice.device_id)); - mockCrossSigningInfo.checkDeviceTrust.mockReturnValue(new DeviceTrustLevel(true, true, false, false)); + mockClient.checkDeviceTrust.mockReturnValue(new DeviceTrustLevel(true, true, false, false)); const { getByTestId } = render(getComponent()); @@ -525,7 +520,7 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice], }); mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); - mockCrossSigningInfo.checkDeviceTrust.mockImplementation((_userId, { deviceId }) => { + mockClient.checkDeviceTrust.mockImplementation((_userId, deviceId) => { if (deviceId === alicesDevice.device_id) { return new DeviceTrustLevel(true, true, false, false); } @@ -552,7 +547,7 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice], }); mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); - mockCrossSigningInfo.checkDeviceTrust.mockImplementation((_userId, { deviceId }) => { + mockClient.checkDeviceTrust.mockImplementation((_userId, deviceId) => { // current session verified = able to verify other sessions if (deviceId === alicesDevice.device_id) { return new DeviceTrustLevel(true, true, false, false); @@ -586,7 +581,7 @@ describe("", () => { devices: [alicesDevice, alicesMobileDevice], }); mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); - mockCrossSigningInfo.checkDeviceTrust.mockImplementation((_userId, { deviceId }) => { + mockClient.checkDeviceTrust.mockImplementation((_userId, deviceId) => { if (deviceId === alicesDevice.device_id) { return new DeviceTrustLevel(true, true, false, false); } diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index 92ba173aa9..ddd6b09125 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -99,6 +99,7 @@ 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", diff --git a/test/toasts/UnverifiedSessionToast-test.tsx b/test/toasts/UnverifiedSessionToast-test.tsx index 8d540baf07..7a6dae4079 100644 --- a/test/toasts/UnverifiedSessionToast-test.tsx +++ b/test/toasts/UnverifiedSessionToast-test.tsx @@ -20,7 +20,7 @@ import userEvent from "@testing-library/user-event"; import { mocked, Mocked } from "jest-mock"; import { IMyDevice, MatrixClient } from "matrix-js-sdk/src/matrix"; import { DeviceInfo } from "matrix-js-sdk/src/crypto/deviceinfo"; -import { CrossSigningInfo } from "matrix-js-sdk/src/crypto/CrossSigning"; +import { DeviceTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; import dis from "../../src/dispatcher/dispatcher"; import { showToast } from "../../src/toasts/UnverifiedSessionToast"; @@ -55,11 +55,7 @@ describe("UnverifiedSessionToast", () => { return null; }); - client.getStoredCrossSigningForUser.mockReturnValue({ - checkDeviceTrust: jest.fn().mockReturnValue({ - isCrossSigningVerified: jest.fn().mockReturnValue(true), - }), - } as unknown as CrossSigningInfo); + client.checkDeviceTrust.mockReturnValue(new DeviceTrustLevel(true, false, false, false)); jest.spyOn(dis, "dispatch"); jest.spyOn(DeviceListener.sharedInstance(), "dismissUnverifiedSessions"); });