Drop GV1 records when GV2 records are present

Co-authored-by: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com>

package.json

@@ -186,7 +186,7 @@
     "@babel/preset-typescript": "7.16.0",
     "@chanzuckerberg/axe-storybook-testing": "3.0.2",
     "@electron/fuses": "1.5.0",
-    "@signalapp/mock-server": "1.0.1",
+    "@signalapp/mock-server": "1.1.0",
     "@storybook/addon-actions": "5.1.11",
     "@storybook/addon-knobs": "5.1.11",
     "@storybook/addons": "5.1.11",

ts/services/storage.ts

@@ -12,6 +12,7 @@ import {
   deriveStorageManifestKey,
   encryptProfile,
   decryptProfile,
+  deriveMasterKeyFromGroupV1,
 } from '../Crypto';
 import {
   mergeAccountRecord,
@@ -1046,26 +1047,86 @@ async function processRemoteRecords(
       `count=${missingKeys.size}`
   );
 
+  const ITEM_TYPE = Proto.ManifestRecord.Identifier.Type;
   const droppedKeys = new Set<string>();
 
-  // Merge Account records last since it contains the pinned conversations
-  // and we need all other records merged first before we can find the pinned
-  // records in our db
-  const ITEM_TYPE = Proto.ManifestRecord.Identifier.Type;
-  const sortedStorageItems = decryptedStorageItems.sort((_, b) =>
-    b.itemType === ITEM_TYPE.ACCOUNT ? -1 : 1
-  );
+  // Drop all GV1 records for which we have GV2 record in the same manifest
+  const masterKeys = new Map<string, string>();
+  for (const { itemType, storageID, storageRecord } of decryptedStorageItems) {
+    if (itemType === ITEM_TYPE.GROUPV2 && storageRecord.groupV2?.masterKey) {
+      masterKeys.set(
+        Bytes.toBase64(storageRecord.groupV2.masterKey),
+        storageID
+      );
+    }
+  }
+
+  let accountItem: MergeableItemType | undefined;
+
+  const prunedStorageItems = decryptedStorageItems.filter(item => {
+    const { itemType, storageID, storageRecord } = item;
+    if (itemType === ITEM_TYPE.ACCOUNT) {
+      if (accountItem !== undefined) {
+        log.warn(
+          `storageService.process(${storageVersion}): duplicate account ` +
+            `record=${redactStorageID(storageID, storageVersion)} ` +
+            `previous=${redactStorageID(accountItem.storageID, storageVersion)}`
+        );
+        droppedKeys.add(accountItem.storageID);
+      }
+
+      accountItem = item;
+      return false;
+    }
+
+    if (itemType !== ITEM_TYPE.GROUPV1 || !storageRecord.groupV1?.id) {
+      return true;
+    }
+
+    const masterKey = deriveMasterKeyFromGroupV1(storageRecord.groupV1.id);
+    const gv2StorageID = masterKeys.get(Bytes.toBase64(masterKey));
+    if (!gv2StorageID) {
+      return true;
+    }
+
+    log.warn(
+      `storageService.process(${storageVersion}): dropping ` +
+        `GV1 record=${redactStorageID(storageID, storageVersion)} ` +
+        `GV2 record=${redactStorageID(gv2StorageID, storageVersion)} ` +
+        'is in the same manifest'
+    );
+    droppedKeys.add(storageID);
+
+    return false;
+  });
 
   try {
     log.info(
       `storageService.process(${storageVersion}): ` +
-        `attempting to merge records=${sortedStorageItems.length}`
-    );
-    const mergedRecords = await pMap(
-      sortedStorageItems,
-      (item: MergeableItemType) => mergeRecord(storageVersion, item),
-      { concurrency: 5 }
+        `attempting to merge records=${prunedStorageItems.length}`
     );
+    if (accountItem === undefined) {
+      log.warn(`storageService.process(${storageVersion}): no account record`);
+    } else {
+      log.info(
+        `storageService.process(${storageVersion}): account ` +
+          `record=${redactStorageID(accountItem.storageID, storageVersion)}`
+      );
+    }
+
+    const mergedRecords = [
+      ...(await pMap(
+        prunedStorageItems,
+        (item: MergeableItemType) => mergeRecord(storageVersion, item),
+        { concurrency: 5 }
+      )),
+
+      // Merge Account records last since it contains the pinned conversations
+      // and we need all other records merged first before we can find the pinned
+      // records in our db
+      ...(accountItem ? [await mergeRecord(storageVersion, accountItem)] : []),
+    ];
+
     log.info(
       `storageService.process(${storageVersion}): ` +
         `processed records=${mergedRecords.length}`
@@ -1190,7 +1251,9 @@ async function sync(
     throw new Error('storageService.sync: Cannot start; no storage key!');
   }
 
-  log.info('storageService.sync: starting...');
+  log.info(
+    `storageService.sync: starting... ignoreConflicts=${ignoreConflicts}`
+  );
 
   let manifest: Proto.ManifestRecord | undefined;
   try {
@@ -1329,6 +1392,9 @@ async function upload(fromSync = false): Promise<void> {
       false
     );
     await uploadManifest(version, generatedManifest);
+
+    // Clear pending delete keys after successful upload
+    await window.storage.put('storage-service-pending-deletes', []);
   } catch (err) {
     if (err.code === 409) {
       await sleep(conflictBackOff.getAndIncrement());

ts/test-mock/storage/drop_test.ts

@@ -0,0 +1,140 @@
+// Copyright 2022 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import { assert } from 'chai';
+import { Proto } from '@signalapp/mock-server';
+
+import * as durations from '../../util/durations';
+import type { App, Bootstrap } from './fixtures';
+import { initStorage, debug } from './fixtures';
+
+const IdentifierType = Proto.ManifestRecord.Identifier.Type;
+
+describe('storage service', function needsName() {
+  this.timeout(durations.MINUTE);
+
+  let bootstrap: Bootstrap;
+  let app: App;
+
+  beforeEach(async () => {
+    ({ bootstrap, app } = await initStorage());
+  });
+
+  afterEach(async () => {
+    await app.close();
+    await bootstrap.teardown();
+  });
+
+  it('should drop gv1 record if there is a matching gv2 record', async () => {
+    const { phone } = bootstrap;
+
+    debug('adding both records');
+    {
+      const state = await phone.expectStorageState('consistency check');
+
+      const groupV1Id = Buffer.from('Wi9258rCEp7AdSdp+jCMlQ==', 'base64');
+      const masterKey = Buffer.from(
+        '2+rdvzFGCOJI8POHcPNZHrYQWS/JXmT63R5OXKxhrPk=',
+        'base64'
+      );
+
+      const updatedState = await phone.setStorageState(
+        state
+          .addRecord({
+            type: IdentifierType.GROUPV1,
+            record: {
+              groupV1: {
+                id: groupV1Id,
+              },
+            },
+          })
+          .addRecord({
+            type: IdentifierType.GROUPV2,
+            record: {
+              groupV2: {
+                masterKey,
+              },
+            },
+          })
+      );
+
+      debug('sending fetch storage');
+      await phone.sendFetchStorage({
+        timestamp: bootstrap.getTimestamp(),
+      });
+
+      debug('waiting for next storage state');
+      const nextState = await phone.waitForStorageState({
+        after: updatedState,
+      });
+
+      assert.isFalse(
+        nextState.hasRecord(({ type }) => {
+          return type === IdentifierType.GROUPV1;
+        }),
+        'should not have gv1 record'
+      );
+
+      assert.isTrue(
+        nextState.hasRecord(({ type, record }) => {
+          if (type !== IdentifierType.GROUPV2) {
+            return false;
+          }
+
+          if (!record.groupV2?.masterKey) {
+            return false;
+          }
+          return Buffer.from(masterKey).equals(record.groupV2.masterKey);
+        }),
+        'should have gv2 record'
+      );
+    }
+  });
+
+  it('should drop duplicate account record', async () => {
+    const { phone } = bootstrap;
+
+    debug('duplicating account record');
+    {
+      const state = await phone.expectStorageState('consistency check');
+
+      const oldAccount = state.findRecord(({ type }) => {
+        return type === IdentifierType.ACCOUNT;
+      });
+      if (oldAccount === undefined) {
+        throw new Error('should have initial account record');
+      }
+
+      const updatedState = await phone.setStorageState(
+        state.addRecord({
+          type: IdentifierType.ACCOUNT,
+          record: oldAccount.record,
+        })
+      );
+
+      debug('sending fetch storage');
+      await phone.sendFetchStorage({
+        timestamp: bootstrap.getTimestamp(),
+      });
+
+      debug('waiting for next storage state');
+      const nextState = await phone.waitForStorageState({
+        after: updatedState,
+      });
+
+      assert.isFalse(
+        nextState.hasRecord(({ type, key }) => {
+          return type === IdentifierType.ACCOUNT && key.equals(oldAccount.key);
+        }),
+        'should not have old account record'
+      );
+
+      assert.isTrue(
+        nextState.hasRecord(({ type }) => {
+          return type === IdentifierType.ACCOUNT;
+        }),
+        'should have new account record'
+      );
+    }
+  });
+});

yarn.lock

@@ -1342,10 +1342,10 @@
     "@react-spring/shared" "~9.4.0"
     "@react-spring/types" "~9.4.0"
 
-"@signalapp/mock-server@1.0.1":
-  version "1.0.1"
-  resolved "https://registry.yarnpkg.com/@signalapp/mock-server/-/mock-server-1.0.1.tgz#ae461528ca18218cf34366d5afa1c672b0ddabe0"
-  integrity sha512-9XYIFZwwGnFEg/WSffn3KWOHHe/ooL44+UQ3cFX68jEtgOk575EeRZaTFge+XNxzciAbDuCtkWivYCODPBJISA==
+"@signalapp/mock-server@1.1.0":
+  version "1.1.0"
+  resolved "https://registry.yarnpkg.com/@signalapp/mock-server/-/mock-server-1.1.0.tgz#1b6ca0f4c89fb2f0b803af06bb6571780edebf10"
+  integrity sha512-dsrxPhvbNM5kdaGzQSwLc2jgDNkNi93DvccWwSl8P28Wfir/1IVt+8/9alaTen0xr0UBbscfreZ2sJ8junNLjA==
   dependencies:
     "@signalapp/signal-client" "0.12.1"
     debug "^4.3.2"