Separate toasts for existing & new device verification

Separate device verification toasts into ones for devices that were
there when the app loaded and a separate toast for each device that
has appeared since.

Reverts part of https://github.com/matrix-org/matrix-react-sdk/pull/4506
(clicking a device from your own UserInfo now triggers the legacy
verification flow again).

Fixes https://github.com/vector-im/riot-web/issues/13422
Fixes https://github.com/vector-im/riot-web/issues/13418

src/DeviceListener.js

@@ -24,6 +24,10 @@ const KEY_BACKUP_POLL_INTERVAL = 5 * 60 * 1000;
 const THIS_DEVICE_TOAST_KEY = 'setupencryption';
 const OTHER_DEVICES_TOAST_KEY = 'reviewsessions';
 
+function toastKey(deviceId) {
+    return "unverified_session_" + deviceId;
+}
+
 export default class DeviceListener {
     static sharedInstance() {
         if (!global.mx_DeviceListener) global.mx_DeviceListener = new DeviceListener();
@@ -39,9 +43,18 @@ export default class DeviceListener {
         // cache of the key backup info
         this._keyBackupInfo = null;
         this._keyBackupFetchedAt = null;
+
+        // We keep a list of our own device IDs so we can batch ones that were already
+        // there the last time the app launched into a single toast, but display new
+        // ones in their own toasts.
+        this._ourDeviceIdsAtStart = null;
+
+        // The set of device IDs we're currently displaying toasts for
+        this._displayingToastsForDeviceIds = new Set();
     }
 
     start() {
+        MatrixClientPeg.get().on('crypto.willUpdateDevices', this._onWillUpdateDevices);
         MatrixClientPeg.get().on('crypto.devicesUpdated', this._onDevicesUpdated);
         MatrixClientPeg.get().on('deviceVerificationChanged', this._onDeviceVerificationChanged);
         MatrixClientPeg.get().on('userTrustStatusChanged', this._onUserTrustStatusChanged);
@@ -53,6 +66,7 @@ export default class DeviceListener {
 
     stop() {
         if (MatrixClientPeg.get()) {
+            MatrixClientPeg.get().removeListener('crypto.willUpdateDevices', this._onWillUpdateDevices);
             MatrixClientPeg.get().removeListener('crypto.devicesUpdated', this._onDevicesUpdated);
             MatrixClientPeg.get().removeListener('deviceVerificationChanged', this._onDeviceVerificationChanged);
             MatrixClientPeg.get().removeListener('userTrustStatusChanged', this._onUserTrustStatusChanged);
@@ -66,10 +80,15 @@ export default class DeviceListener {
         this._keyBackupFetchedAt = null;
     }
 
-    async dismissVerifications() {
+    /**
-        const cli = MatrixClientPeg.get();
+     * Dismiss notifications about our own unverified devices
-        const devices = await cli.getStoredDevicesForUser(cli.getUserId());
+     *
-        this._dismissed = new Set(devices.filter(d => d.deviceId !== cli.deviceId).map(d => d.deviceId));
+     * @param {String[]} deviceIds List of device IDs to dismiss notifications for
+     */
+    async dismissUnverifiedSessions(deviceIds) {
+        for (const d of deviceIds) {
+            this._dismissed.add(d);
+        }
 
         this._recheck();
     }
@@ -79,6 +98,20 @@ export default class DeviceListener {
         this._recheck();
     }
 
+    _ensureDeviceIdsAtStartPopulated() {
+        if (this._ourDeviceIdsAtStart === null) {
+            const cli = MatrixClientPeg.get();
+            this._ourDeviceIdsAtStart = new Set(
+                cli.getStoredDevicesForUser(cli.getUserId()).map(d => d.deviceId),
+            );
+        }
+    }
+
+    _onWillUpdateDevices = async (users) => {
+        const myUserId = MatrixClientPeg.get().getUserId();
+        if (users.includes(myUserId)) this._ensureDeviceIdsAtStartPopulated();
+    }
+
     _onDevicesUpdated = (users) => {
         if (!users.includes(MatrixClientPeg.get().getUserId())) return;
         this._recheck();
@@ -143,6 +176,8 @@ export default class DeviceListener {
 
         const crossSigningReady = await cli.isCrossSigningReady();
 
+        this._ensureDeviceIdsAtStartPopulated();
+
         if (this._dismissedThisDeviceToast) {
             ToastStore.sharedInstance().dismissToast(THIS_DEVICE_TOAST_KEY);
         } else {
@@ -197,32 +232,65 @@ export default class DeviceListener {
             }
         }
 
+        // Unverified devices that were there last time the app ran
+        // (technically could just be a boolean: we don't actually
+        // need to remember the device IDs, but for the sake of
+        // symmetry...).
+        const oldUnverifiedDeviceIds = new Set();
+        // Unverified devices that have appeared since then
+        const newUnverifiedDeviceIds = new Set();
+
         // as long as cross-signing isn't ready,
         // you can't see or dismiss any device toasts
         if (crossSigningReady) {
-            let haveUnverifiedDevices = false;
+            const devices = cli.getStoredDevicesForUser(cli.getUserId());
-
-            const devices = await cli.getStoredDevicesForUser(cli.getUserId());
             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)) {
-                    haveUnverifiedDevices = true;
+                    if (this._ourDeviceIdsAtStart.has(device.deviceId)) {
-                    break;
+                        oldUnverifiedDeviceIds.add(device.deviceId);
+                    } else {
+                        newUnverifiedDeviceIds.add(device.deviceId);
+                    }
                 }
             }
+        }
 
-            if (haveUnverifiedDevices) {
+        // Display or hide the batch toast for old unverified sessions
-                ToastStore.sharedInstance().addOrReplaceToast({
+        if (oldUnverifiedDeviceIds.size > 0) {
-                    key: OTHER_DEVICES_TOAST_KEY,
+            ToastStore.sharedInstance().addOrReplaceToast({
-                    title: _t("Review where you’re logged in"),
+                key: OTHER_DEVICES_TOAST_KEY,
-                    icon: "verification_warning",
+                title: _t("Review where you’re logged in"),
-                    component: sdk.getComponent("toasts.UnverifiedSessionToast"),
+                icon: "verification_warning",
-                });
+                props: {
-            } else {
+                    deviceIds: oldUnverifiedDeviceIds,
-                ToastStore.sharedInstance().dismissToast(OTHER_DEVICES_TOAST_KEY);
+                },
+                component: sdk.getComponent("toasts.BulkUnverifiedSessionsToast"),
+            });
+        } else {
+            ToastStore.sharedInstance().dismissToast(OTHER_DEVICES_TOAST_KEY);
+        }
+
+        // Show toasts for new unverified devices if they aren't already there
+        for (const deviceId of newUnverifiedDeviceIds) {
+            ToastStore.sharedInstance().addOrReplaceToast({
+                key: toastKey(deviceId),
+                title: _t("Unverified login. Was this you?"),
+                icon: "verification_warning",
+                props: { deviceId },
+                component: sdk.getComponent("toasts.UnverifiedSessionToast"),
+            });
+        }
+
+        // ...and hide any we don't need any more
+        for (const deviceId of this._displayingToastsForDeviceIds) {
+            if (!newUnverifiedDeviceIds.has(deviceId)) {
+                ToastStore.sharedInstance().dismissToast(toastKey(deviceId));
             }
         }
+
+        this._displayingToastsForDeviceIds = newUnverifiedDeviceIds;
     }
 }

src/SlashCommands.tsx

@@ -799,7 +799,7 @@ export const Commands = [
                     const fingerprint = matches[3];
 
                     return success((async () => {
-                        const device = await cli.getStoredDevice(userId, deviceId);
+                        const device = cli.getStoredDevice(userId, deviceId);
                         if (!device) {
                             throw new Error(_t('Unknown (user, session) pair:') + ` (${userId}, ${deviceId})`);
                         }

src/components/views/right_panel/EncryptionPanel.js

@@ -22,7 +22,6 @@ import VerificationPanel from "./VerificationPanel";
 import {MatrixClientPeg} from "../../../MatrixClientPeg";
 import {ensureDMExists} from "../../../createRoom";
 import {useEventEmitter} from "../../../hooks/useEventEmitter";
-import {useAsyncMemo} from "../../../hooks/useAsyncMemo";
 import Modal from "../../../Modal";
 import {PHASE_REQUESTED, PHASE_UNSENT} from "matrix-js-sdk/src/crypto/verification/request/VerificationRequest";
 import * as sdk from "../../../index";
@@ -47,10 +46,7 @@ const EncryptionPanel = (props) => {
     }, [verificationRequest]);
 
     const deviceId = request && request.channel.deviceId;
-    const device = useAsyncMemo(() => {
+    const device = MatrixClientPeg.get().getStoredDevice(MatrixClientPeg.get().getUserId(), deviceId);
-        const cli = MatrixClientPeg.get();
-        return cli.getStoredDevice(cli.getUserId(), deviceId);
-    }, [deviceId]);
 
     useEffect(() => {
         async function awaitPromise() {

src/components/views/right_panel/UserInfo.js

@@ -1110,7 +1110,7 @@ export const useDevices = (userId) => {
         async function _downloadDeviceList() {
             try {
                 await cli.downloadKeys([userId], true);
-                const devices = await cli.getStoredDevicesForUser(userId);
+                const devices = cli.getStoredDevicesForUser(userId);
 
                 if (cancelled) {
                     // we got cancelled - presumably a different user now
@@ -1135,7 +1135,7 @@ export const useDevices = (userId) => {
     useEffect(() => {
         let cancel = false;
         const updateDevices = async () => {
-            const newDevices = await cli.getStoredDevicesForUser(userId);
+            const newDevices = cli.getStoredDevicesForUser(userId);
             if (cancel) return;
             setDevices(newDevices);
         };

src/components/views/rooms/MemberInfo.js

@@ -160,13 +160,10 @@ export default createReactClass({
             // no need to re-download the whole thing; just update our copy of
             // the list.
 
-            // Promise.resolve to handle transition from static result to promise; can be removed
+            const devices = this.context.getStoredDevicesForUser(userId);
-            // in future
+            this.setState({
-            Promise.resolve(this.context.getStoredDevicesForUser(userId)).then((devices) => {
+                devices: devices,
-                this.setState({
+                e2eStatus: this._getE2EStatus(devices),
-                    devices: devices,
-                    e2eStatus: this._getE2EStatus(devices),
-                });
             });
         }
     },

src/components/views/toasts/BulkUnverifiedSessionsToast.js

@@ -15,6 +15,7 @@ limitations under the License.
 */
 
 import React from 'react';
+import PropTypes from 'prop-types';
 import { _t } from '../../../languageHandler';
 import dis from "../../../dispatcher";
 import { MatrixClientPeg } from '../../../MatrixClientPeg';
@@ -22,14 +23,18 @@ import DeviceListener from '../../../DeviceListener';
 import FormButton from '../elements/FormButton';
 import { replaceableComponent } from '../../../utils/replaceableComponent';
 
-@replaceableComponent("views.toasts.UnverifiedSessionToast")
+@replaceableComponent("views.toasts.BulkUnverifiedSessionsToast")
-export default class UnverifiedSessionToast extends React.PureComponent {
+export default class BulkUnverifiedSessionsToast extends React.PureComponent {
+    static propTypes = {
+        deviceIds: PropTypes.array,
+    }
+
     _onLaterClick = () => {
-        DeviceListener.sharedInstance().dismissVerifications();
+        DeviceListener.sharedInstance().dismissUnverifiedSessions(this.props.deviceIds);
     };
 
     _onReviewClick = async () => {
-        DeviceListener.sharedInstance().dismissVerifications();
+        DeviceListener.sharedInstance().dismissUnverifiedSessions(this.props.deviceIds);
 
         dis.dispatch({
             action: 'view_user_info',

src/i18n/strings/en_EN.json

@@ -106,6 +106,7 @@
     "Encryption upgrade available": "Encryption upgrade available",
     "Set up encryption": "Set up encryption",
     "Review where you’re logged in": "Review where you’re logged in",
+    "Unverified login. Was this you?": "Unverified login. Was this you?",
     "Who would you like to add to this community?": "Who would you like to add to this community?",
     "Warning: any person you add to a community will be publicly visible to anyone who knows the community ID": "Warning: any person you add to a community will be publicly visible to anyone who knows the community ID",
     "Invite new community members": "Invite new community members",
@@ -555,15 +556,15 @@
     "Headphones": "Headphones",
     "Folder": "Folder",
     "Pin": "Pin",
+    "Verify your other sessions": "Verify your other sessions",
+    "Later": "Later",
+    "Review": "Review",
     "Verify yourself & others to keep your chats safe": "Verify yourself & others to keep your chats safe",
     "Other users may not trust it": "Other users may not trust it",
     "Update your secure storage": "Update your secure storage",
     "Set up": "Set up",
     "Upgrade": "Upgrade",
     "Verify": "Verify",
-    "Later": "Later",
-    "Verify your other sessions": "Verify your other sessions",
-    "Review": "Review",
     "From %(deviceName)s (%(deviceId)s)": "From %(deviceName)s (%(deviceId)s)",
     "Decline (%(counter)s)": "Decline (%(counter)s)",
     "Accept <policyLink /> to continue:": "Accept <policyLink /> to continue:",

src/utils/ShieldUtils.ts

@@ -7,7 +7,7 @@ interface Client {
         isCrossSigningVerified: () => boolean
         wasCrossSigningVerified: () => boolean
     };
-    getStoredDevicesForUser: (userId: string) => Promise<[{ deviceId: string }]>;
+    getStoredDevicesForUser: (userId: string) => [{ deviceId: string }];
     checkDeviceTrust: (userId: string, deviceId: string) => {
         isVerified: () => boolean
     }
@@ -45,7 +45,7 @@ export async function shieldStatusForRoom(client: Client, room: Room): Promise<s
                         (members.length === 1);     // Do alarm for self if we're alone in a room
     const targets = includeUser ? [...verified, client.getUserId()] : verified;
     for (const userId of targets) {
-        const devices = await client.getStoredDevicesForUser(userId);
+        const devices = client.getStoredDevicesForUser(userId);
         const anyDeviceNotVerified = devices.some(({deviceId}) => {
             return !client.checkDeviceTrust(userId, deviceId).isVerified();
         });

src/verification.js

@@ -23,7 +23,6 @@ import {RIGHT_PANEL_PHASES} from "./stores/RightPanelStorePhases";
 import {findDMForUser} from './createRoom';
 import {accessSecretStorage} from './CrossSigningManager';
 import SettingsStore from './settings/SettingsStore';
-import NewSessionReviewDialog from './components/views/dialogs/NewSessionReviewDialog';
 import {verificationMethods} from 'matrix-js-sdk/src/crypto';
 
 async function enable4SIfNeeded() {
@@ -70,41 +69,34 @@ export async function verifyDevice(user, device) {
         }
     }
 
-    if (user.userId === cli.getUserId()) {
+    Modal.createTrackedDialog("Verification warning", "unverified session", UntrustedDeviceDialog, {
-        Modal.createTrackedDialog('New Session Review', 'Starting dialog', NewSessionReviewDialog, {
+        user,
-            userId: user.userId,
+        device,
-            device,
+        onFinished: async (action) => {
-        });
+            if (action === "sas") {
-    } else {
+                const verificationRequestPromise = cli.legacyDeviceVerification(
-        Modal.createTrackedDialog("Verification warning", "unverified session", UntrustedDeviceDialog, {
+                    user.userId,
-            user,
+                    device.deviceId,
-            device,
+                    verificationMethods.SAS,
-            onFinished: async (action) => {
+                );
-                if (action === "sas") {
+                dis.dispatch({
-                    const verificationRequestPromise = cli.legacyDeviceVerification(
+                    action: "set_right_panel_phase",
-                        user.userId,
+                    phase: RIGHT_PANEL_PHASES.EncryptionPanel,
-                        device.deviceId,
+                    refireParams: {member: user, verificationRequestPromise},
-                        verificationMethods.SAS,
+                });
-                    );
+            } else if (action === "legacy") {
-                    dis.dispatch({
+                const ManualDeviceKeyVerificationDialog =
-                        action: "set_right_panel_phase",
+                    sdk.getComponent("dialogs.ManualDeviceKeyVerificationDialog");
-                        phase: RIGHT_PANEL_PHASES.EncryptionPanel,
+                Modal.createTrackedDialog("Legacy verify session", "legacy verify session",
-                        refireParams: {member: user, verificationRequestPromise},
+                    ManualDeviceKeyVerificationDialog,
-                    });
+                    {
-                } else if (action === "legacy") {
+                        userId: user.userId,
-                    const ManualDeviceKeyVerificationDialog =
+                        device,
-                        sdk.getComponent("dialogs.ManualDeviceKeyVerificationDialog");
+                    },
-                    Modal.createTrackedDialog("Legacy verify session", "legacy verify session",
+                );
-                        ManualDeviceKeyVerificationDialog,
+            }
-                        {
+        },
-                            userId: user.userId,
+    });
-                            device,
-                        },
-                    );
-                }
-            },
-        });
-    }
 }
 
 export async function legacyVerifyUser(user) {

test/utils/ShieldUtils-test.js

@@ -11,7 +11,7 @@ function mkClient(selfTrust) {
         checkDeviceTrust: (userId, deviceId) => ({
             isVerified: () => userId === "@self:localhost" ? selfTrust : userId[2] == "T",
         }),
-        getStoredDevicesForUser: async (userId) => ["DEVICE"],
+        getStoredDevicesForUser: (userId) => ["DEVICE"],
     };
 }