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.

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;

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
+ * @returns `null` if the device is unknown or has not published encryption keys; otherwise a boolean
- *    cross-signed. `null` if there was an error fetching the device info.
+ *    indicating whether the device has been cross-signed by a cross-signing key we trust.
  */
 export const isDeviceVerified = async (client: MatrixClient, deviceId: string): Promise<boolean | null> => {
-    try {
+    const trustLevel = await client.getCrypto()?.getDeviceVerificationStatus(client.getSafeUserId(), deviceId);
-        const trustLevel = await client.getCrypto()?.getDeviceVerificationStatus(client.getSafeUserId(), deviceId);
+    if (!trustLevel) {
-        return trustLevel?.crossSigningVerified ?? false;
+        // either no crypto, or an unknown/no-e2e device
-    } catch (e) {
-        console.error("Error getting device cross-signing info", e);
         return null;
     }
+    return trustLevel.crossSigningVerified;
 };

test/components/views/settings/DevicesPanel-test.tsx

@@ -25,18 +25,40 @@ import MatrixClientContext from "../../../../src/contexts/MatrixClientContext";
 
 describe("<DevicesPanel />", () => {
     const userId = "@alice:server.org";
-    const device1 = { device_id: "device_1" };
+
-    const device2 = { device_id: "device_2" };
+    // the local device
-    const device3 = { device_id: "device_3" };
+    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({
+        getDeviceVerificationStatus: jest.fn().mockImplementation((_userId, deviceId) => {
-            crossSigningVerified: false,
+            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("<DevicesPanel />", () => {
     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("<DevicesPanel />", () => {
         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("<DevicesPanel />", () => {
             });
 
             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("<DevicesPanel />", () => {
                 .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("<DevicesPanel />", () => {
             // 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("<DevicesPanel />", () => {
             // 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("<DevicesPanel />", () => {
             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("<DevicesPanel />", () => {
                 .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("<DevicesPanel />", () => {
             // 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"));

test/components/views/settings/__snapshots__/DevicesPanel-test.tsx.snap

@@ -104,7 +104,7 @@ exports[`<DevicesPanel /> renders device panel with devices 1`] = `
         class="mx_DevicesPanel_deviceTrust"
       >
         <span
-          class="mx_DevicesPanel_icon mx_E2EIcon mx_E2EIcon_warning"
+          class="mx_DevicesPanel_icon mx_E2EIcon mx_E2EIcon_verified"
         />
       </div>
       <div
@@ -124,8 +124,8 @@ exports[`<DevicesPanel /> renders device panel with devices 1`] = `
             />
           </div>
           <div
-            aria-label="Unverified"
+            aria-label="Verified"
-            class="mx_DeviceTypeIcon_verificationIcon unverified"
+            class="mx_DeviceTypeIcon_verificationIcon verified"
             role="img"
           />
         </div>
@@ -143,7 +143,7 @@ exports[`<DevicesPanel /> renders device panel with devices 1`] = `
             <span
               data-testid="device-metadata-isVerified"
             >
-              Unverified
+              Verified
             </span>
              · 
             <span
@@ -163,13 +163,6 @@ exports[`<DevicesPanel /> renders device panel with devices 1`] = `
           >
             Sign Out
           </div>
-          <div
-            class="mx_AccessibleButton mx_AccessibleButton_hasKind mx_AccessibleButton_kind_primary"
-            role="button"
-            tabindex="0"
-          >
-            Verify
-          </div>
           <div
             class="mx_AccessibleButton mx_AccessibleButton_hasKind mx_AccessibleButton_kind_primary_outline"
             role="button"
@@ -188,24 +181,13 @@ exports[`<DevicesPanel /> renders device panel with devices 1`] = `
         class="mx_DevicesPanel_header_trust"
       >
         <span
-          class="mx_DevicesPanel_header_icon mx_E2EIcon mx_E2EIcon_warning"
+          class="mx_DevicesPanel_header_icon mx_E2EIcon mx_E2EIcon_verified"
         />
       </div>
       <div
         class="mx_DevicesPanel_header_title"
       >
-        Unverified devices
+        Verified devices
-      </div>
-      <div
-        class="mx_DevicesPanel_header_button"
-      >
-        <div
-          class="mx_AccessibleButton mx_DevicesPanel_selectButton mx_AccessibleButton_hasKind mx_AccessibleButton_kind_secondary"
-          role="button"
-          tabindex="0"
-        >
-          Select all
-        </div>
       </div>
     </div>
     <div
@@ -250,8 +232,8 @@ exports[`<DevicesPanel /> renders device panel with devices 1`] = `
                     />
                   </div>
                   <div
-                    aria-label="Unverified"
+                    aria-label="Verified"
-                    class="mx_DeviceTypeIcon_verificationIcon unverified"
+                    class="mx_DeviceTypeIcon_verificationIcon verified"
                     role="img"
                   />
                 </div>
@@ -269,7 +251,7 @@ exports[`<DevicesPanel /> renders device panel with devices 1`] = `
                     <span
                       data-testid="device-metadata-isVerified"
                     >
-                      Unverified
+                      Verified
                     </span>
                      · 
                     <span
@@ -296,6 +278,23 @@ exports[`<DevicesPanel /> renders device panel with devices 1`] = `
         </span>
       </div>
     </div>
+    <hr />
+    <div
+      class="mx_DevicesPanel_header"
+    >
+      <div
+        class="mx_DevicesPanel_header_trust"
+      >
+        <span
+          class="mx_DevicesPanel_header_icon mx_E2EIcon mx_E2EIcon_warning"
+        />
+      </div>
+      <div
+        class="mx_DevicesPanel_header_title"
+      >
+        Unverified devices
+      </div>
+    </div>
     <div
       class="mx_DevicesPanel_device"
     >
@@ -367,6 +366,114 @@ exports[`<DevicesPanel /> renders device panel with devices 1`] = `
                     </span>
                   </div>
                 </div>
+                <div
+                  class="mx_DeviceTile_actions"
+                >
+                  <div
+                    class="mx_AccessibleButton mx_AccessibleButton_hasKind mx_AccessibleButton_kind_primary"
+                    role="button"
+                    tabindex="0"
+                  >
+                    Verify
+                  </div>
+                  <div
+                    class="mx_AccessibleButton mx_AccessibleButton_hasKind mx_AccessibleButton_kind_primary_outline"
+                    role="button"
+                    tabindex="0"
+                  >
+                    Rename
+                  </div>
+                </div>
+              </div>
+            </div>
+          </label>
+        </span>
+      </div>
+    </div>
+    <hr />
+    <div
+      class="mx_DevicesPanel_header"
+    >
+      <div
+        class="mx_DevicesPanel_header_trust"
+      />
+      <div
+        class="mx_DevicesPanel_header_title"
+      >
+        Devices without encryption support
+      </div>
+    </div>
+    <div
+      class="mx_DevicesPanel_device"
+    >
+      <div
+        class="mx_SelectableDeviceTile"
+      >
+        <span
+          class="mx_Checkbox mx_SelectableDeviceTile_checkbox mx_Checkbox_hasKind mx_Checkbox_kind_solid"
+        >
+          <input
+            data-testid="device-tile-checkbox-non_crypto"
+            id="device-tile-checkbox-non_crypto"
+            type="checkbox"
+          />
+          <label
+            for="device-tile-checkbox-non_crypto"
+          >
+            <div
+              class="mx_Checkbox_background"
+            >
+              <div
+                class="mx_Checkbox_checkmark"
+              />
+            </div>
+            <div>
+              <div
+                class="mx_DeviceTile"
+                data-testid="device-tile-non_crypto"
+              >
+                <div
+                  class="mx_DeviceTypeIcon"
+                >
+                  <div
+                    class="mx_DeviceTypeIcon_deviceIconWrapper"
+                  >
+                    <div
+                      aria-label="Unknown session type"
+                      class="mx_DeviceTypeIcon_deviceIcon"
+                      role="img"
+                    />
+                  </div>
+                  <div
+                    aria-label="Unverified"
+                    class="mx_DeviceTypeIcon_verificationIcon unverified"
+                    role="img"
+                  />
+                </div>
+                <div
+                  class="mx_DeviceTile_info"
+                >
+                  <h4
+                    class="mx_Heading_h4"
+                  >
+                    non_crypto
+                  </h4>
+                  <div
+                    class="mx_DeviceTile_metadata"
+                  >
+                    <span
+                      data-testid="device-metadata-isVerified"
+                    >
+                      Unverified
+                    </span>
+                     · 
+                    <span
+                      data-testid="device-metadata-deviceId"
+                    >
+                      non_crypto
+                    </span>
+                  </div>
+                </div>
                 <div
                   class="mx_DeviceTile_actions"
                 >

test/components/views/settings/tabs/user/SessionManagerTab-test.tsx

@@ -232,27 +232,6 @@ describe("<SessionManagerTab />", () => {
         expect(container.getElementsByClassName("mx_Spinner").length).toBeFalsy();
     });
 
-    it("does not fail when checking device verification fails", async () => {
-        const logSpy = jest.spyOn(console, "error").mockImplementation((e) => {});
-        mockClient.getDevices.mockResolvedValue({
-            devices: [alicesDevice, alicesMobileDevice],
-        });
-        const failError = new Error("non-specific failure");
-        mockCrypto.getDeviceVerificationStatus.mockImplementation(() => {
-            throw failError;
-        });
-        render(getComponent());
-
-        await act(async () => {
-            await flushPromises();
-        });
-
-        // called for each device despite error
-        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 () => {
         mockClient.getDevices.mockResolvedValue({
             devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice],
@@ -268,7 +247,7 @@ describe("<SessionManagerTab />", () => {
                 return new DeviceVerificationStatus({});
             }
             // alicesOlderMobileDevice does not support encryption
-            throw new Error("encryption not supported");
+            return null;
         });
 
         const { getByTestId } = render(getComponent());
@@ -567,8 +546,7 @@ describe("<SessionManagerTab />", () => {
                     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.
+                return null;
-                throw new Error("encryption not supported");
             });
 
             const { getByTestId, queryByTestId } = render(getComponent());