Add logic to detect sync conflicts and trigger merge (#1352)

This is part of addressing #1342. This is the app-framework part of
adding merge to E2EE sync. The merge algorithm itself is stubbed-out for
separate implementation. There are two major cases covered here:
background sync and password change. The behavior is summarized below.
This conflict-resolution model is based on a pattern I used at Dropbox,
so I hope it's clear to people who haven't seen it before.

### Background sync behavior

- Pre-existing logic lets the server revision to be used to detect
whether this client has seen the latest state on the server.
- Pre-existing logic lets the storage hash be used to detect whether
this client's state has changed from the most recent server state.
- New upload behavior: Send the previous known revision to the server,
which will abort rather than clobbering a new revision.
- Client responds by triggering an extra download in order to merge,
after which another upload will be attempted.
- New download behavior: When a new revision is downloaded, recalculate
the hash of data from AppState, and compare it to the hash of the
**previous** server revision to see if this client has made any changes.
  - If so, trigger a merge and keep the merged state.
- The download can trigger a merge on its own if it's the first point
that a conflict is discovered. The case of upload aborting and
triggering download is simply the common case in a quiescent system,
because uploads are attempted eagerly after changes.

### Password change behavior

- Password change (changeBlobKey) behaves like an upload, because it
uploads the latest data using the new encryption key.
- The known revision is added to the changeBlobKey request to detect
conflicts and abort, same as an upload.
  - This requires some new react hooks to fetch this info from AppState.
- When an abort happens, the user sees a message asking them to wait for
sync to complete.
  - An extra download is triggered to help this happen.
- Making that possible required adding `useSyncE2EEStorage()` to some
more
- Also added found #1351 in the process of testing, and added tweak to
the behavior of background tab behavior to trigger an extra download and
merge.

### Testing

I tested the background sync and password change cases manually against
my dev server. I don't think there's a way to write automated tests for
them.

I didn't test the non-standard password change cases (where a password
is set for an existing account) because I don't know how to set up those
cases. I'm hopeful that the relevant logic is all identical and will
just work. Maybe our QA can set up those cases to be sure.

### Notes

If this PR is pushed without a real merge algorithm, it ends up always
preferring the remote copy, such that whichever change hits the server
first wins. That's quite close to the current behavior (given the eager
nature of uploads), though not identical because the current behavior
doesn't ensure a consistent decision.

apps/passport-client/components/modals/RequireAddPasswordModal.tsx

@@ -1,9 +1,15 @@
 import { sleep } from "@pcd/util";
 import { useCallback, useState } from "react";
 import styled from "styled-components";
-import { useDispatch, useSelf, useUpdate } from "../../src/appHooks";
+import {
+  useDispatch,
+  useSelf,
+  useServerStorageRevision,
+  useUpdate
+} from "../../src/appHooks";
 import { loadEncryptionKey } from "../../src/localstorage";
 import { setPassword } from "../../src/password";
+import { useSyncE2EEStorage } from "../../src/useSyncE2EEStorage";
 import { BigInput, H2, Spacer } from "../core";
 import { NewPasswordForm } from "../shared/NewPasswordForm";
 import { ScreenLoader } from "../shared/ScreenLoader";
@@ -13,10 +19,12 @@ import { ScreenLoader } from "../shared/ScreenLoader";
  * and have never created a password. It asks them to create a password.
  */
 export function RequireAddPasswordModal() {
+  useSyncE2EEStorage();
   const [loading, setLoading] = useState(false);
   const dispatch = useDispatch();
   const update = useUpdate();
   const self = useSelf();
+  const serverStorageRevision = useServerStorageRevision();
 
   const [newPassword, setNewPassword] = useState("");
   const [confirmPassword, setConfirmPassword] = useState("");
@@ -29,18 +37,25 @@ export function RequireAddPasswordModal() {
     await sleep();
     try {
       const currentEncryptionKey = loadEncryptionKey();
-      await setPassword(newPassword, currentEncryptionKey, dispatch, update);
+      await setPassword(
+        newPassword,
+        currentEncryptionKey,
+        serverStorageRevision,
+        dispatch,
+        update
+      );
 
       dispatch({
         type: "set-modal",
         modal: { modalType: "none" }
       });
     } catch (e) {
-      setError("Couldn't set a password - try again later");
+      console.log("error setting password", e);
+      setError(e.message);
     } finally {
       setLoading(false);
     }
-  }, [loading, newPassword, dispatch, update]);
+  }, [loading, newPassword, serverStorageRevision, dispatch, update]);
 
   if (loading) {
     return <ScreenLoader text="Adding your password..." />;

apps/passport-client/components/modals/UpgradeAccountModal.tsx

@@ -1,9 +1,15 @@
 import { sleep } from "@pcd/util";
 import { useCallback, useState } from "react";
 import styled from "styled-components";
-import { useDispatch, useSelf, useUpdate } from "../../src/appHooks";
+import {
+  useDispatch,
+  useSelf,
+  useServerStorageRevision,
+  useUpdate
+} from "../../src/appHooks";
 import { loadEncryptionKey } from "../../src/localstorage";
 import { setPassword } from "../../src/password";
+import { useSyncE2EEStorage } from "../../src/useSyncE2EEStorage";
 import { BigInput, H2, Spacer } from "../core";
 import { NewPasswordForm } from "../shared/NewPasswordForm";
 import { ScreenLoader } from "../shared/ScreenLoader";
@@ -13,10 +19,12 @@ import { ScreenLoader } from "../shared/ScreenLoader";
  * and have never created a password. It asks them to create a password.
  */
 export function UpgradeAccountModal() {
+  useSyncE2EEStorage();
   const [loading, setLoading] = useState(false);
   const dispatch = useDispatch();
   const update = useUpdate();
   const self = useSelf();
+  const serverStorageRevision = useServerStorageRevision();
 
   const [newPassword, setNewPassword] = useState("");
   const [confirmPassword, setConfirmPassword] = useState("");
@@ -29,18 +37,25 @@ export function UpgradeAccountModal() {
     await sleep();
     try {
       const currentEncryptionKey = loadEncryptionKey();
-      await setPassword(newPassword, currentEncryptionKey, dispatch, update);
+      await setPassword(
+        newPassword,
+        currentEncryptionKey,
+        serverStorageRevision,
+        dispatch,
+        update
+      );
 
       dispatch({
         type: "set-modal",
         modal: { modalType: "changed-password" }
       });
     } catch (e) {
-      setError("Couldn't set a password - try again later");
+      console.log("error setting password", e);
+      setError(e.message);
     } finally {
       setLoading(false);
     }
-  }, [loading, newPassword, dispatch, update]);
+  }, [loading, newPassword, serverStorageRevision, dispatch, update]);
 
   if (loading) {
     return <ScreenLoader text="Adding your password..." />;

apps/passport-client/components/screens/ChangePasswordScreen.tsx

@@ -7,10 +7,12 @@ import {
   useDispatch,
   useHasSetupPassword,
   useSelf,
+  useServerStorageRevision,
   useUpdate
 } from "../../src/appHooks";
 import { loadEncryptionKey } from "../../src/localstorage";
 import { setPassword } from "../../src/password";
+import { useSyncE2EEStorage } from "../../src/useSyncE2EEStorage";
 import { CenterColumn, H2, HR, Spacer, TextCenter } from "../core";
 import { LinkButton } from "../core/Button";
 import { RippleLoader } from "../core/RippleLoader";
@@ -20,8 +22,11 @@ import { NewPasswordForm } from "../shared/NewPasswordForm";
 import { PasswordInput } from "../shared/PasswordInput";
 
 export function ChangePasswordScreen() {
+  useSyncE2EEStorage();
   const self = useSelf();
   const hasSetupPassword = useHasSetupPassword();
+  const serverStorageRevision = useServerStorageRevision();
+
   // We want the `isChangePassword` state to persist on future renders,
   // otherwise we may show the invalid copy on the "finished" screen
   // after a password is set for the first time.
@@ -66,7 +71,13 @@ export function ChangePasswordScreen() {
           saltResult.value
         );
       }
-      await setPassword(newPassword, currentEncryptionKey, dispatch, update);
+      await setPassword(
+        newPassword,
+        currentEncryptionKey,
+        serverStorageRevision,
+        dispatch,
+        update
+      );
 
       setFinished(true);
 
@@ -79,6 +90,7 @@ export function ChangePasswordScreen() {
   }, [
     currentPassword,
     newPassword,
+    serverStorageRevision,
     dispatch,
     update,
     loading,

apps/passport-client/src/appHooks.ts

@@ -106,6 +106,10 @@ export function useIsDownloaded(): boolean | undefined {
   return useSelector<boolean | undefined>((s) => s.downloadedPCDs, []);
 }
 
+export function useServerStorageRevision(): string | undefined {
+  return useSelector<string | undefined>((s) => s.serverStorageRevision, []);
+}
+
 export function useUserForcedToLogout(): boolean {
   const userForcedToLogout = useSelector<boolean>(
     (s) => !!s.userInvalid || !!s.anotherDeviceChangedPassword,

apps/passport-client/src/dispatch.ts

@@ -47,7 +47,7 @@ import { hasPendingRequest } from "./sessionStorage";
 import { AppError, AppState, GetState, StateEmitter } from "./state";
 import { hasSetupPassword } from "./user";
 import {
-  downloadStorage,
+  downloadAndMergeStorage,
   uploadSerializedStorage,
   uploadStorage
 } from "./useSyncE2EEStorage";
@@ -354,12 +354,16 @@ async function finishAccountCreation(
     return; // Don't save the bad identity.  User must reset account.
   }
 
-  // Save PCDs to E2EE storage.
+  // Save PCDs to E2EE storage.  knownRevision=undefined is the way to create
+  // a new entry.  It would also overwrite any conflicting data which may
+  // already exist, but that should be impossible for a new account with
+  // a new encryption key.
   console.log("[ACCOUNT] Upload initial PCDs");
   const uploadResult = await uploadStorage(
     user,
     state.pcds,
-    state.subscriptions
+    state.subscriptions,
+    undefined // knownRevision
   );
   if (uploadResult.success) {
     update({
@@ -531,6 +535,7 @@ async function loadAfterLogin(
     modal = { modalType: "upgrade-account-modal" };
   }
 
+  console.log(`[SYNC] saving state at login: revision ${storage.revision}`);
   await savePCDs(pcds);
   await saveSubscriptions(subscriptions);
   savePersistentSyncStatus({
@@ -569,7 +574,13 @@ async function handlePasswordChangeOnOtherTab(update: ZuUpdate) {
   const encryptionKey = loadEncryptionKey();
   return update({
     self,
-    encryptionKey
+    encryptionKey,
+
+    // Extra download helps to get our in-memory sync state up-to-date faster.
+    // The password change on the other tab is an upload of a new revision so
+    // a download is necessary.  Otherwise loading our new self object above
+    // will make this tab think it needs to upload and cause a conflict.
+    extraDownloadRequested: true
   });
 }
 
@@ -656,9 +667,16 @@ let syncInProgress = false;
 let skippedSyncUpdates = 0;
 
 /**
- * Does the real work of sync(), inside of reentrancy protection.
- * Returns the changes to be made to AppState.  If changes were made,
- * this function should be run again.
+ * Does the real work of sync(), inside of reentrancy protection.  If action
+ * is needed, this function takes one action and returns, expecting to be
+ * run again until no further action is needed.
+ *
+ * Returns the changes to be made to AppState, which the caller is expected
+ * to applly via update().  If the result is defined, this function should be
+ * run again.
+ *
+ * Further calls to update() will also occur inside of this function, to update
+ * fields which allow the UI to track progress.
  */
 async function doSync(
   state: AppState,
@@ -686,15 +704,21 @@ async function doSync(
     // Download user's E2EE storage, which includes both PCDs and subscriptions.
     // We'll skip this if it fails, or if the server indicates no changes based
     // on the last revision we downloaded.
-    const dlRes = await downloadStorage(state.serverStorageRevision);
+    const dlRes = await downloadAndMergeStorage(
+      state.serverStorageRevision,
+      state.serverStorageHash,
+      state.self,
+      state.pcds,
+      state.subscriptions
+    );
     if (dlRes.success && dlRes.value != null) {
-      const { pcds, subscriptions, revision, storageHash } = dlRes.value;
+      const { pcds, subscriptions, serverRevision, serverHash } = dlRes.value;
       return {
         downloadedPCDs: true,
         pcds,
         subscriptions,
-        serverStorageRevision: revision,
-        serverStorageHash: storageHash,
+        serverStorageRevision: serverRevision,
+        serverStorageHash: serverHash,
         extraDownloadRequested: false
       };
     } else {
@@ -758,18 +782,24 @@ async function doSync(
   );
   if (state.serverStorageHash !== appStorage.storageHash) {
     console.log("[SYNC] sync action: upload");
-    // TODO(artwyman): Add serverStorageRevision input as knownRevision here,
-    // but only after we're able to respond to a conflict by downloading.
     const upRes = await uploadSerializedStorage(
       appStorage.serializedStorage,
-      appStorage.storageHash
+      appStorage.storageHash,
+      state.serverStorageRevision
     );
     if (upRes.success) {
       return {
         serverStorageRevision: upRes.value.revision,
         serverStorageHash: upRes.value.storageHash
       };
+    } else if (upRes.error.name === "Conflict") {
+      // Conflicts are resolved at download time, so ensure another download.
+      return {
+        completedFirstSync: true,
+        extraDownloadRequested: true
+      };
     } else {
+      // We completed a first attempt at sync, even if it failed.  We'll retry.
       return {
         completedFirstSync: true
       };

apps/passport-client/src/password.ts

@@ -23,6 +23,7 @@ export const PASSWORD_MINIMUM_LENGTH = 8;
 export const setPassword = async (
   newPassword: string,
   currentEncryptionKey: HexString,
+  knownServerStorageRevision: string | undefined,
   dispatch: Dispatcher,
   update: ZuUpdate
 ) => {
@@ -32,18 +33,24 @@ export const setPassword = async (
   const res = await updateBlobKeyForEncryptedStorage(
     currentEncryptionKey,
     newEncryptionKey,
-    newSalt
+    newSalt,
+    knownServerStorageRevision
   );
 
-  // Meaning password is incorrect, as old row is not found
-  if (!res.success && res.error.name === "PasswordIncorrect") {
-    throw new Error(
-      "Incorrect password. If you've lost your password, reset your account below."
-    );
-  }
-
   if (!res.success) {
-    throw new Error(`Request failed with message ${res.error}`);
+    if (res.error.name === "PasswordIncorrect") {
+      throw new Error(
+        "Incorrect password. If you've lost your password, reset your account below."
+      );
+    } else if (res.error.name === "Conflict") {
+      update({ extraDownloadRequested: true });
+      throw new Error(`Cannot change password while PCDs are syncing.  
+        Wait for download to complete or reload the page and try again.`);
+    } else {
+      throw new Error(
+        `Failed to set password - try again later.  Request failed with message ${res.error}`
+      );
+    }
   }
 
   dispatch({