From 5593872b7a7a0e709e2b289b99f7a0fb938861cc Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 25 May 2023 17:12:01 +0100 Subject: [PATCH] Fix display of devices without encryption support in Settings dialog (#10977) * Update tests to demonstrate broken behaviour * Fixes and comments * Remove exception swallowing This seems like it causes more problems than it solves. --- .../views/settings/devices/types.ts | 8 +- src/utils/device/isDeviceVerified.ts | 13 +- .../views/settings/DevicesPanel-test.tsx | 64 ++++--- .../__snapshots__/DevicesPanel-test.tsx.snap | 161 +++++++++++++++--- .../tabs/user/SessionManagerTab-test.tsx | 26 +-- 5 files changed, 193 insertions(+), 79 deletions(-) diff --git a/src/components/views/settings/devices/types.ts b/src/components/views/settings/devices/types.ts index 36343cf22c0..5b073831733 100644 --- a/src/components/views/settings/devices/types.ts +++ b/src/components/views/settings/devices/types.ts @@ -18,7 +18,13 @@ import { IMyDevice } from "matrix-js-sdk/src/matrix"; import { ExtendedDeviceInformation } from "../../../../utils/device/parseUserAgent"; -export type DeviceWithVerification = IMyDevice & { isVerified: boolean | null }; +export type DeviceWithVerification = IMyDevice & { + /** + * `null` if the device is unknown or has not published encryption keys; otherwise a boolean + * indicating whether the device has been cross-signed by a cross-signing key we trust. + */ + isVerified: boolean | null; +}; export type ExtendedDeviceAppInfo = { // eg Element Web appName?: string; diff --git a/src/utils/device/isDeviceVerified.ts b/src/utils/device/isDeviceVerified.ts index 1671fa01a30..a139069fe27 100644 --- a/src/utils/device/isDeviceVerified.ts +++ b/src/utils/device/isDeviceVerified.ts @@ -22,15 +22,14 @@ import { MatrixClient } from "matrix-js-sdk/src/matrix"; * @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. + * @returns `null` if the device is unknown or has not published encryption keys; otherwise a boolean + * indicating whether the device has been cross-signed by a cross-signing key we trust. */ export const isDeviceVerified = async (client: MatrixClient, deviceId: string): Promise => { - try { - const trustLevel = await client.getCrypto()?.getDeviceVerificationStatus(client.getSafeUserId(), deviceId); - return trustLevel?.crossSigningVerified ?? false; - } catch (e) { - console.error("Error getting device cross-signing info", e); + const trustLevel = await client.getCrypto()?.getDeviceVerificationStatus(client.getSafeUserId(), deviceId); + if (!trustLevel) { + // either no crypto, or an unknown/no-e2e device return null; } + return trustLevel.crossSigningVerified; }; diff --git a/test/components/views/settings/DevicesPanel-test.tsx b/test/components/views/settings/DevicesPanel-test.tsx index dd334d89475..2f25242c73b 100644 --- a/test/components/views/settings/DevicesPanel-test.tsx +++ b/test/components/views/settings/DevicesPanel-test.tsx @@ -25,18 +25,40 @@ import MatrixClientContext from "../../../../src/contexts/MatrixClientContext"; describe("", () => { const userId = "@alice:server.org"; - const device1 = { device_id: "device_1" }; - const device2 = { device_id: "device_2" }; - const device3 = { device_id: "device_3" }; + + // the local device + const ownDevice = { device_id: "device_1" }; + + // a device which we have verified via cross-signing + const verifiedDevice = { device_id: "device_2" }; + + // a device which we have *not* verified via cross-signing + const unverifiedDevice = { device_id: "device_3" }; + + // a device which is returned by `getDevices` but getDeviceVerificationStatus returns `null` for + // (as it would for a device with no E2E keys). + const nonCryptoDevice = { device_id: "non_crypto" }; + const mockCrypto = { - getDeviceVerificationStatus: jest.fn().mockResolvedValue({ - crossSigningVerified: false, + getDeviceVerificationStatus: jest.fn().mockImplementation((_userId, deviceId) => { + if (_userId !== userId) { + throw new Error(`bad user id ${_userId}`); + } + if (deviceId === ownDevice.device_id || deviceId === verifiedDevice.device_id) { + return { crossSigningVerified: true }; + } else if (deviceId === unverifiedDevice.device_id) { + return { + crossSigningVerified: false, + }; + } else { + return null; + } }), }; const mockClient = getMockClientWithEventEmitter({ ...mockClientMethodsUser(userId), getDevices: jest.fn(), - getDeviceId: jest.fn().mockReturnValue(device1.device_id), + getDeviceId: jest.fn().mockReturnValue(ownDevice.device_id), deleteMultipleDevices: jest.fn(), getStoredDevice: jest.fn().mockReturnValue(new DeviceInfo("id")), generateClientSecret: jest.fn(), @@ -54,12 +76,14 @@ describe("", () => { beforeEach(() => { jest.clearAllMocks(); - mockClient.getDevices.mockReset().mockResolvedValue({ devices: [device1, device2, device3] }); + mockClient.getDevices + .mockReset() + .mockResolvedValue({ devices: [ownDevice, verifiedDevice, unverifiedDevice, nonCryptoDevice] }); mockClient.getPushers.mockReset().mockResolvedValue({ pushers: [ mkPusher({ - [PUSHER_DEVICE_ID.name]: device1.device_id, + [PUSHER_DEVICE_ID.name]: ownDevice.device_id, [PUSHER_ENABLED.name]: true, }), ], @@ -88,16 +112,16 @@ describe("", () => { it("deletes selected devices when interactive auth is not required", async () => { mockClient.deleteMultipleDevices.mockResolvedValue({}); mockClient.getDevices - .mockResolvedValueOnce({ devices: [device1, device2, device3] }) + .mockResolvedValueOnce({ devices: [ownDevice, verifiedDevice, unverifiedDevice] }) // pretend it was really deleted on refresh - .mockResolvedValueOnce({ devices: [device1, device3] }); + .mockResolvedValueOnce({ devices: [ownDevice, unverifiedDevice] }); const { container, getByTestId } = render(getComponent()); await flushPromises(); expect(container.getElementsByClassName("mx_DevicesPanel_device").length).toEqual(3); - toggleDeviceSelection(container, device2.device_id); + toggleDeviceSelection(container, verifiedDevice.device_id); mockClient.getDevices.mockClear(); @@ -106,7 +130,7 @@ describe("", () => { }); expect(container.getElementsByClassName("mx_Spinner").length).toBeTruthy(); - expect(mockClient.deleteMultipleDevices).toHaveBeenCalledWith([device2.device_id], undefined); + expect(mockClient.deleteMultipleDevices).toHaveBeenCalledWith([verifiedDevice.device_id], undefined); await flushPromises(); @@ -124,9 +148,9 @@ describe("", () => { .mockResolvedValueOnce({}); mockClient.getDevices - .mockResolvedValueOnce({ devices: [device1, device2, device3] }) + .mockResolvedValueOnce({ devices: [ownDevice, verifiedDevice, unverifiedDevice] }) // pretend it was really deleted on refresh - .mockResolvedValueOnce({ devices: [device1, device3] }); + .mockResolvedValueOnce({ devices: [ownDevice, unverifiedDevice] }); const { container, getByTestId, getByLabelText } = render(getComponent()); @@ -135,7 +159,7 @@ describe("", () => { // reset mock count after initial load mockClient.getDevices.mockClear(); - toggleDeviceSelection(container, device2.device_id); + toggleDeviceSelection(container, verifiedDevice.device_id); act(() => { fireEvent.click(getByTestId("sign-out-devices-btn")); @@ -145,7 +169,7 @@ describe("", () => { // modal rendering has some weird sleeps await sleep(100); - expect(mockClient.deleteMultipleDevices).toHaveBeenCalledWith([device2.device_id], undefined); + expect(mockClient.deleteMultipleDevices).toHaveBeenCalledWith([verifiedDevice.device_id], undefined); const modal = document.getElementsByClassName("mx_Dialog"); expect(modal).toMatchSnapshot(); @@ -159,7 +183,7 @@ describe("", () => { await flushPromises(); // called again with auth - expect(mockClient.deleteMultipleDevices).toHaveBeenCalledWith([device2.device_id], { + expect(mockClient.deleteMultipleDevices).toHaveBeenCalledWith([verifiedDevice.device_id], { identifier: { type: "m.id.user", user: userId, @@ -182,9 +206,9 @@ describe("", () => { .mockResolvedValueOnce({}); mockClient.getDevices - .mockResolvedValueOnce({ devices: [device1, device2, device3] }) + .mockResolvedValueOnce({ devices: [ownDevice, verifiedDevice, unverifiedDevice] }) // pretend it was really deleted on refresh - .mockResolvedValueOnce({ devices: [device1, device3] }); + .mockResolvedValueOnce({ devices: [ownDevice, unverifiedDevice] }); const { container, getByTestId } = render(getComponent()); @@ -193,7 +217,7 @@ describe("", () => { // reset mock count after initial load mockClient.getDevices.mockClear(); - toggleDeviceSelection(container, device2.device_id); + toggleDeviceSelection(container, verifiedDevice.device_id); act(() => { fireEvent.click(getByTestId("sign-out-devices-btn")); diff --git a/test/components/views/settings/__snapshots__/DevicesPanel-test.tsx.snap b/test/components/views/settings/__snapshots__/DevicesPanel-test.tsx.snap index e8aca7063cf..ad12a99c7c5 100644 --- a/test/components/views/settings/__snapshots__/DevicesPanel-test.tsx.snap +++ b/test/components/views/settings/__snapshots__/DevicesPanel-test.tsx.snap @@ -104,7 +104,7 @@ exports[` renders device panel with devices 1`] = ` class="mx_DevicesPanel_deviceTrust" >
renders device panel with devices 1`] = ` />
@@ -143,7 +143,7 @@ exports[` renders device panel with devices 1`] = ` - Unverified + Verified · renders device panel with devices 1`] = ` > Sign Out -
- Verify -
renders device panel with devices 1`] = ` class="mx_DevicesPanel_header_trust" >
- Unverified devices -
-
-
- Select all -
+ Verified devices
renders device panel with devices 1`] = ` />
@@ -269,7 +251,7 @@ exports[` renders device panel with devices 1`] = ` - Unverified + Verified · renders device panel with devices 1`] = ` +
+
+
+ +
+
+ Unverified devices +
+
@@ -367,6 +366,114 @@ exports[` renders device panel with devices 1`] = `
+
+
+ Verify +
+
+ Rename +
+
+ + + + + + +
+
+
+
+ Devices without encryption support +
+
+
+
+ + +