Fix logic in DeviceListener (#30230)

* remove incorrect check for cross-signing

SETUP_ENCRYPTION tries to set up everything (4S, cross-signing and key backup),
rather than just setting up encryption, as its name would imply.
crossSigningReady == false happens when the user's device isn't verified, so it
should trigger VERIFY_THIS_SESSION rather than SETUP_ENCRYPTION

* reorder conditions in allSystemsReady to match the order in the if statements

* explicitly handle secrets missing from 4S

rather than falling back to the SETUP_ENCRYPTION catch-all.  Also, remove
SETUP_ENCRYPTION since it is no longer used.

* convert button handlers to switch statements for consistency

(almost) all the other functions that use make decisions based on Kind use
switch statements

* update i18n (remove obsolete string)

src/DeviceListener.ts

@@ -362,7 +362,7 @@ export default class DeviceListener {
         // said we are OK with that.
         const keyBackupIsOk = keyBackupUploadActive || backupDisabled;
 
-        const allSystemsReady = crossSigningReady && keyBackupIsOk && recoveryIsOk && allCrossSigningSecretsCached;
+        const allSystemsReady = isCurrentDeviceTrusted && allCrossSigningSecretsCached && keyBackupIsOk && recoveryIsOk;
 
         await this.reportCryptoSessionStateToAnalytics(cli);
 
@@ -375,13 +375,8 @@ export default class DeviceListener {
             // make sure our keys are finished downloading
             await crypto.getUserDeviceInfo([cli.getSafeUserId()]);
 
-            if (!crossSigningReady) {
+            if (!isCurrentDeviceTrusted) {
-                // This account is legacy and doesn't have cross-signing set up at all.
+                // the current device is not trusted: prompt the user to verify
-                // Prompt the user to set it up.
-                logSpan.info("Cross-signing not ready: showing SET_UP_ENCRYPTION toast");
-                showSetupEncryptionToast(SetupKind.SET_UP_ENCRYPTION);
-            } else if (!isCurrentDeviceTrusted) {
-                // cross signing is ready but the current device is not trusted: prompt the user to verify
                 logSpan.info("Current device not verified: showing VERIFY_THIS_SESSION toast");
                 showSetupEncryptionToast(SetupKind.VERIFY_THIS_SESSION);
             } else if (!allCrossSigningSecretsCached) {
@@ -410,16 +405,11 @@ export default class DeviceListener {
                     hideSetupEncryptionToast();
                 }
             } else {
-                // some other condition... yikes! Show the 'set up encryption' toast: this is what we previously did
+                // If we get here, then we are verified, have key backup, and
-                // in 'other' situations. Possibly we should consider prompting for a full reset in this case?
+                // 4S, but crypto.isCrossSigningReady returned false, which
-                logSpan.warn("Couldn't match encryption state to a known case: showing 'setup encryption' prompt", {
+                // means that 4S doesn't have all the secrets.
-                    crossSigningReady,
+                logSpan.warn("4S is missing secrets");
-                    secretStorageReady,
+                showSetupEncryptionToast(SetupKind.KEY_STORAGE_OUT_OF_SYNC_STORE);
-                    allCrossSigningSecretsCached,
-                    isCurrentDeviceTrusted,
-                    defaultKeyId,
-                });
-                showSetupEncryptionToast(SetupKind.SET_UP_ENCRYPTION);
             }
         } else {
             logSpan.info("Not yet ready, but shouldShowSetupEncryptionToast==false");

src/i18n/strings/en_EN.json

@@ -969,7 +969,6 @@
         "reset_all_button": "Forgotten or lost all recovery methods? <a>Reset all</a>",
         "set_up_recovery": "Set up recovery",
         "set_up_recovery_toast_description": "Generate a recovery key that can be used to restore your encrypted message history in case you lose access to your devices.",
-        "set_up_toast_description": "Safeguard against losing access to encrypted messages & data",
         "set_up_toast_title": "Set up Secure Backup",
         "setup_secure_backup": {
             "explainer": "Back up your keys before signing out to avoid losing them."

src/toasts/SetupEncryptionToast.ts

@@ -30,13 +30,12 @@ const TOAST_KEY = "setupencryption";
 
 const getTitle = (kind: Kind): string => {
     switch (kind) {
-        case Kind.SET_UP_ENCRYPTION:
-            return _t("encryption|set_up_toast_title");
         case Kind.SET_UP_RECOVERY:
             return _t("encryption|set_up_recovery");
         case Kind.VERIFY_THIS_SESSION:
             return _t("encryption|verify_toast_title");
         case Kind.KEY_STORAGE_OUT_OF_SYNC:
+        case Kind.KEY_STORAGE_OUT_OF_SYNC_STORE:
             return _t("encryption|key_storage_out_of_sync");
         case Kind.TURN_ON_KEY_STORAGE:
             return _t("encryption|turn_on_key_storage");
@@ -45,12 +44,11 @@ const getTitle = (kind: Kind): string => {
 
 const getIcon = (kind: Kind): string | undefined => {
     switch (kind) {
-        case Kind.SET_UP_ENCRYPTION:
-            return "secure_backup";
         case Kind.SET_UP_RECOVERY:
             return undefined;
         case Kind.VERIFY_THIS_SESSION:
         case Kind.KEY_STORAGE_OUT_OF_SYNC:
+        case Kind.KEY_STORAGE_OUT_OF_SYNC_STORE:
             return "verification_warning";
         case Kind.TURN_ON_KEY_STORAGE:
             return "key_storage";
@@ -59,13 +57,12 @@ const getIcon = (kind: Kind): string | undefined => {
 
 const getSetupCaption = (kind: Kind): string => {
     switch (kind) {
-        case Kind.SET_UP_ENCRYPTION:
-            return _t("action|continue");
         case Kind.SET_UP_RECOVERY:
             return _t("action|continue");
         case Kind.VERIFY_THIS_SESSION:
             return _t("action|verify");
         case Kind.KEY_STORAGE_OUT_OF_SYNC:
+        case Kind.KEY_STORAGE_OUT_OF_SYNC_STORE:
             return _t("encryption|enter_recovery_key");
         case Kind.TURN_ON_KEY_STORAGE:
             return _t("action|continue");
@@ -79,6 +76,7 @@ const getSetupCaption = (kind: Kind): string => {
 const getPrimaryButtonIcon = (kind: Kind): ComponentType<React.SVGAttributes<SVGElement>> | undefined => {
     switch (kind) {
         case Kind.KEY_STORAGE_OUT_OF_SYNC:
+        case Kind.KEY_STORAGE_OUT_OF_SYNC_STORE:
             return KeyIcon;
         default:
             return;
@@ -89,10 +87,10 @@ const getSecondaryButtonLabel = (kind: Kind): string => {
     switch (kind) {
         case Kind.SET_UP_RECOVERY:
             return _t("action|dismiss");
-        case Kind.SET_UP_ENCRYPTION:
         case Kind.VERIFY_THIS_SESSION:
             return _t("encryption|verification|unverified_sessions_toast_reject");
         case Kind.KEY_STORAGE_OUT_OF_SYNC:
+        case Kind.KEY_STORAGE_OUT_OF_SYNC_STORE:
             return _t("encryption|forgot_recovery_key");
         case Kind.TURN_ON_KEY_STORAGE:
             return _t("action|dismiss");
@@ -101,13 +99,12 @@ const getSecondaryButtonLabel = (kind: Kind): string => {
 
 const getDescription = (kind: Kind): string => {
     switch (kind) {
-        case Kind.SET_UP_ENCRYPTION:
-            return _t("encryption|set_up_toast_description");
         case Kind.SET_UP_RECOVERY:
             return _t("encryption|set_up_recovery_toast_description");
         case Kind.VERIFY_THIS_SESSION:
             return _t("encryption|verify_toast_description");
         case Kind.KEY_STORAGE_OUT_OF_SYNC:
+        case Kind.KEY_STORAGE_OUT_OF_SYNC_STORE:
             return _t("encryption|key_storage_out_of_sync_description");
         case Kind.TURN_ON_KEY_STORAGE:
             return _t("encryption|turn_on_key_storage_description");
@@ -118,10 +115,6 @@ const getDescription = (kind: Kind): string => {
  * The kind of toast to show.
  */
 export enum Kind {
-    /**
-     *  Prompt the user to set up encryption
-     */
-    SET_UP_ENCRYPTION = "set_up_encryption",
     /**
      * Prompt the user to set up a recovery key
      */
@@ -131,9 +124,13 @@ export enum Kind {
      */
     VERIFY_THIS_SESSION = "verify_this_session",
     /**
-     * Prompt the user to enter their recovery key
+     * Prompt the user to enter their recovery key, to retrieve secrets
      */
     KEY_STORAGE_OUT_OF_SYNC = "key_storage_out_of_sync",
+    /**
+     * Prompt the user to enter their recovery key, to store secrets
+     */
+    KEY_STORAGE_OUT_OF_SYNC_STORE = "key_storage_out_of_sync_store",
     /**
      * Prompt the user to turn on key storage
      */
@@ -156,16 +153,22 @@ export const showToast = (kind: Kind): void => {
     }
 
     const onPrimaryClick = async (): Promise<void> => {
-        if (kind === Kind.VERIFY_THIS_SESSION) {
+        switch (kind) {
-            Modal.createDialog(SetupEncryptionDialog, {}, undefined, /* priority = */ false, /* static = */ true);
+            case Kind.TURN_ON_KEY_STORAGE: {
-        } else if (kind == Kind.TURN_ON_KEY_STORAGE) {
                 // Open the user settings dialog to the encryption tab
                 const payload: OpenToTabPayload = {
                     action: Action.ViewUserSettings,
                     initialTabId: UserTab.Encryption,
                 };
                 defaultDispatcher.dispatch(payload);
-        } else {
+                break;
+            }
+            case Kind.VERIFY_THIS_SESSION:
+                Modal.createDialog(SetupEncryptionDialog, {}, undefined, /* priority = */ false, /* static = */ true);
+                break;
+            case Kind.SET_UP_RECOVERY:
+            case Kind.KEY_STORAGE_OUT_OF_SYNC:
+            case Kind.KEY_STORAGE_OUT_OF_SYNC_STORE: {
                 const modal = Modal.createDialog(
                     Spinner,
                     undefined,
@@ -176,15 +179,25 @@ export const showToast = (kind: Kind): void => {
                 try {
                     await accessSecretStorage();
                 } catch (error) {
-                onAccessSecretStorageFailed(error as Error);
+                    onAccessSecretStorageFailed(kind, error as Error);
                 } finally {
                     modal.close();
                 }
+                break;
+            }
         }
     };
 
     const onSecondaryClick = async (): Promise<void> => {
-        if (kind === Kind.KEY_STORAGE_OUT_OF_SYNC) {
+        switch (kind) {
+            case Kind.SET_UP_RECOVERY: {
+                // Record that the user doesn't want to set up recovery
+                const deviceListener = DeviceListener.sharedInstance();
+                await deviceListener.recordRecoveryDisabled();
+                deviceListener.dismissEncryptionSetup();
+                break;
+            }
+            case Kind.KEY_STORAGE_OUT_OF_SYNC: {
                 // Open the user settings dialog to the encryption tab and start the flow to reset encryption
                 const payload: OpenToTabPayload = {
                     action: Action.ViewUserSettings,
@@ -192,21 +205,34 @@ export const showToast = (kind: Kind): void => {
                     props: { initialEncryptionState: "reset_identity_forgot" },
                 };
                 defaultDispatcher.dispatch(payload);
-        } else if (kind === Kind.TURN_ON_KEY_STORAGE) {
+                break;
+            }
+            case Kind.KEY_STORAGE_OUT_OF_SYNC_STORE: {
+                // Open the user settings dialog to the encryption tab and start the flow to reset 4S
+                const payload: OpenToTabPayload = {
+                    action: Action.ViewUserSettings,
+                    initialTabId: UserTab.Encryption,
+                    props: { initialEncryptionState: "change_recovery_key" },
+                };
+                defaultDispatcher.dispatch(payload);
+                break;
+            }
+            case Kind.TURN_ON_KEY_STORAGE: {
                 // The user clicked "Dismiss": offer them "Are you sure?"
-            const modal = Modal.createDialog(ConfirmKeyStorageOffDialog, undefined, "mx_ConfirmKeyStorageOffDialog");
+                const modal = Modal.createDialog(
+                    ConfirmKeyStorageOffDialog,
+                    undefined,
+                    "mx_ConfirmKeyStorageOffDialog",
+                );
                 const [dismissed] = await modal.finished;
                 if (dismissed) {
                     const deviceListener = DeviceListener.sharedInstance();
                     await deviceListener.recordKeyBackupDisabled();
                     deviceListener.dismissEncryptionSetup();
                 }
-        } else if (kind === Kind.SET_UP_RECOVERY) {
+                break;
-            // Record that the user doesn't want to set up recovery
+            }
-            const deviceListener = DeviceListener.sharedInstance();
+            default:
-            await deviceListener.recordRecoveryDisabled();
-            deviceListener.dismissEncryptionSetup();
-        } else {
                 DeviceListener.sharedInstance().dismissEncryptionSetup();
         }
     };
@@ -215,10 +241,16 @@ export const showToast = (kind: Kind): void => {
      * We tried to accessSecretStorage, which triggered us to ask for the
      * recovery key, but this failed. If the user just gave up, that is fine,
      * but if not, that means downloading encryption info from 4S did not fix
-     * the problem we identified. Presumably, something is wrong with what
+     * the problem we identified. Presumably, something is wrong with what they
-     * they have in 4S: we tell them to reset their identity.
+     * have in 4S.  If we were trying to fetch secrets from 4S, we tell them to
+     * reset their identity, to reset everything.  If we were trying to store
+     * secrets in 4S, or set up recovery, we tell them to change their recovery
+     * key, to create a new 4S that we can store the secrets in.
      */
-    const onAccessSecretStorageFailed = (error: Error): void => {
+    const onAccessSecretStorageFailed = (
+        kind: Kind.SET_UP_RECOVERY | Kind.KEY_STORAGE_OUT_OF_SYNC | Kind.KEY_STORAGE_OUT_OF_SYNC_STORE,
+        error: Error,
+    ): void => {
         if (error instanceof AccessCancelledError) {
             // The user cancelled the dialog - just allow it to close
         } else {
@@ -226,7 +258,10 @@ export const showToast = (kind: Kind): void => {
             const payload: OpenToTabPayload = {
                 action: Action.ViewUserSettings,
                 initialTabId: UserTab.Encryption,
-                props: { initialEncryptionState: "reset_identity_sync_failed" },
+                props: {
+                    initialEncryptionState:
+                        kind === Kind.KEY_STORAGE_OUT_OF_SYNC ? "reset_identity_sync_failed" : "change_recovery_key",
+                },
             };
             defaultDispatcher.dispatch(payload);
         }

test/unit-tests/DeviceListener-test.ts

@@ -307,15 +307,6 @@ describe("DeviceListener", () => {
                 jest.spyOn(mockClient.getCrypto()!, "isEncryptionEnabledInRoom").mockResolvedValue(true);
             });
 
-            it("hides setup encryption toast when cross signing and secret storage are ready", async () => {
-                mockCrypto!.isCrossSigningReady.mockResolvedValue(true);
-                mockCrypto!.isSecretStorageReady.mockResolvedValue(true);
-                mockCrypto!.getActiveSessionBackupVersion.mockResolvedValue("1");
-
-                await createAndStart();
-                expect(SetupEncryptionToast.hideToast).toHaveBeenCalled();
-            });
-
             it("hides setup encryption toast when it is dismissed", async () => {
                 const instance = await createAndStart();
                 instance.dismissEncryptionSetup();
@@ -360,7 +351,15 @@ describe("DeviceListener", () => {
                     );
                 });
 
-                it("shows an out-of-sync toast when one of the secrets is missing", async () => {
+                it("hides setup encryption toast when cross signing and secret storage are ready", async () => {
+                    mockCrypto!.isSecretStorageReady.mockResolvedValue(true);
+                    mockCrypto!.getActiveSessionBackupVersion.mockResolvedValue("1");
+
+                    await createAndStart();
+                    expect(SetupEncryptionToast.hideToast).toHaveBeenCalled();
+                });
+
+                it("shows an out-of-sync toast when one of the secrets is missing locally", async () => {
                     mockCrypto!.getCrossSigningStatus.mockResolvedValue({
                         publicKeysOnDevice: true,
                         privateKeysInSecretStorage: true,
@@ -378,7 +377,7 @@ describe("DeviceListener", () => {
                     );
                 });
 
-                it("hides the out-of-sync toast when one of the secrets is missing", async () => {
+                it("hides the out-of-sync toast after we receive the missing secrets", async () => {
                     mockCrypto!.isSecretStorageReady.mockResolvedValue(true);
                     mockCrypto!.getActiveSessionBackupVersion.mockResolvedValue("1");
 
@@ -427,6 +426,18 @@ describe("DeviceListener", () => {
                         SetupEncryptionToast.Kind.SET_UP_RECOVERY,
                     );
                 });
+
+                it("shows an out-of-sync toast when one of the secrets is missing from 4S", async () => {
+                    mockCrypto.getKeyBackupInfo.mockResolvedValue({} as unknown as KeyBackupInfo);
+                    mockCrypto.getActiveSessionBackupVersion.mockResolvedValue("1");
+                    mockClient.secretStorage.getDefaultKeyId.mockResolvedValue("foo");
+
+                    await createAndStart();
+
+                    expect(SetupEncryptionToast.showToast).toHaveBeenCalledWith(
+                        SetupEncryptionToast.Kind.KEY_STORAGE_OUT_OF_SYNC_STORE,
+                    );
+                });
             });
         });
 
@@ -448,6 +459,16 @@ describe("DeviceListener", () => {
             });
 
             it("dispatches keybackup event when key backup is not enabled", async () => {
+                mockCrypto!.isCrossSigningReady.mockResolvedValue(true);
+
+                // current device is verified
+                mockCrypto!.getDeviceVerificationStatus.mockResolvedValue(
+                    new DeviceVerificationStatus({
+                        trustCrossSignedDevices: true,
+                        crossSigningVerified: true,
+                    }),
+                );
+
                 mockCrypto.getActiveSessionBackupVersion.mockResolvedValue(null);
                 mockClient.getAccountDataFromServer.mockImplementation((eventType) =>
                     eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY ? ({ disabled: true } as any) : null,

test/unit-tests/toasts/SetupEncryptionToast-test.tsx

@@ -50,7 +50,7 @@ describe("SetupEncryptionToast", () => {
         });
     });
 
-    describe("Key storage out of sync", () => {
+    describe("Key storage out of sync (retrieve secrets)", () => {
         it("should render the toast", async () => {
             showToast(Kind.KEY_STORAGE_OUT_OF_SYNC);
 
@@ -88,6 +88,44 @@ describe("SetupEncryptionToast", () => {
         });
     });
 
+    describe("Key storage out of sync (store secrets)", () => {
+        it("should render the toast", async () => {
+            showToast(Kind.KEY_STORAGE_OUT_OF_SYNC_STORE);
+
+            await expect(screen.findByText("Your key storage is out of sync.")).resolves.toBeInTheDocument();
+        });
+
+        it("should open settings to the reset flow when 'forgot recovery key' clicked", async () => {
+            showToast(Kind.KEY_STORAGE_OUT_OF_SYNC_STORE);
+
+            const user = userEvent.setup();
+            await user.click(await screen.findByText("Forgot recovery key?"));
+
+            expect(dis.dispatch).toHaveBeenCalledWith({
+                action: "view_user_settings",
+                initialTabId: "USER_ENCRYPTION_TAB",
+                props: { initialEncryptionState: "change_recovery_key" },
+            });
+        });
+
+        it("should open settings to the reset flow when recovering fails", async () => {
+            jest.spyOn(SecurityManager, "accessSecretStorage").mockImplementation(async () => {
+                throw new Error("Something went wrong while recovering!");
+            });
+
+            showToast(Kind.KEY_STORAGE_OUT_OF_SYNC_STORE);
+
+            const user = userEvent.setup();
+            await user.click(await screen.findByText("Enter recovery key"));
+
+            expect(dis.dispatch).toHaveBeenCalledWith({
+                action: "view_user_settings",
+                initialTabId: "USER_ENCRYPTION_TAB",
+                props: { initialEncryptionState: "change_recovery_key" },
+            });
+        });
+    });
+
     describe("Turn on key storage", () => {
         it("should render the toast", async () => {
             showToast(Kind.TURN_ON_KEY_STORAGE);