Split ACI contact when it is unregistered

package.json

@@ -191,7 +191,7 @@
     "@babel/preset-typescript": "7.17.12",
     "@electron/fuses": "1.5.0",
     "@mixer/parallel-prettier": "2.0.1",
-    "@signalapp/mock-server": "2.12.1",
+    "@signalapp/mock-server": "2.13.0",
     "@storybook/addon-a11y": "6.5.6",
     "@storybook/addon-actions": "6.5.6",
     "@storybook/addon-controls": "6.5.6",

ts/ConversationController.ts

@@ -381,7 +381,7 @@ export class ConversationController {
       reason: 'getOurConversationId',
     });
 
-    return conversation?.id;
+    return conversation.id;
   }
 
   getOurConversationIdOrThrow(): string {
@@ -465,7 +465,7 @@ export class ConversationController {
     fromPniSignature?: boolean;
     mergeOldAndNew?: (options: SafeCombineConversationsParams) => Promise<void>;
   }): {
-    conversation: ConversationModel | undefined;
+    conversation: ConversationModel;
     mergePromises: Array<Promise<void>>;
   } {
     const dataProvided = [];

ts/background.ts

@@ -2615,16 +2615,12 @@ export async function startApp(): Promise<void> {
     if (!conversation && groupId) {
       conversation = window.ConversationController.get(groupId);
     }
-    if (!groupV2Id && !groupId && senderConversation) {
+    if (!groupV2Id && !groupId) {
       conversation = senderConversation;
     }
 
     const ourId = window.ConversationController.getOurConversationId();
 
-    if (!senderConversation) {
-      log.warn('onTyping: maybeMergeContacts returned falsey sender!');
-      return;
-    }
     if (!ourId) {
       log.warn("onTyping: Couldn't get our own id!");
       return;
@@ -2867,7 +2863,6 @@ export async function startApp(): Promise<void> {
     const { data, confirm } = event;
 
     const messageDescriptor = getMessageDescriptor({
-      confirm,
       message: data.message,
       source: data.source,
       sourceUuid: data.sourceUuid,
@@ -3012,16 +3007,6 @@ export async function startApp(): Promise<void> {
       reason: 'onProfileKeyUpdate',
     });
 
-    if (!conversation) {
-      log.error(
-        'onProfileKeyUpdate: could not find conversation',
-        data.source,
-        data.sourceUuid
-      );
-      confirm();
-      return;
-    }
-
     if (!data.profileKey) {
       log.error('onProfileKeyUpdate: missing profileKey', data.profileKey);
       confirm();
@@ -3155,14 +3140,12 @@ export async function startApp(): Promise<void> {
   // Works with 'sent' and 'message' data sent from MessageReceiver, with a little massage
   //   at callsites to make sure both source and destination are populated.
   const getMessageDescriptor = ({
-    confirm,
     message,
     source,
     sourceUuid,
     destination,
     destinationUuid,
   }: {
-    confirm: () => unknown;
     message: ProcessedDataMessage;
     source?: string;
     sourceUuid?: string;
@@ -3236,7 +3219,7 @@ export async function startApp(): Promise<void> {
         });
 
       const conversationId = window.ConversationController.ensureGroup(id, {
-        addedBy: fromContact?.id,
+        addedBy: fromContact.id,
       });
 
       return {
@@ -3250,12 +3233,6 @@ export async function startApp(): Promise<void> {
       e164: destination,
       reason: `getMessageDescriptor(${message.timestamp}): private`,
     });
-    if (!conversation) {
-      confirm();
-      throw new Error(
-        `getMessageDescriptor/${message.timestamp}: maybeMergeContacts returned falsey conversation`
-      );
-    }
 
     return {
       type: Message.PRIVATE,
@@ -3274,7 +3251,6 @@ export async function startApp(): Promise<void> {
     strictAssert(source && sourceUuid, 'Missing user number and uuid');
 
     const messageDescriptor = getMessageDescriptor({
-      confirm,
       ...data,
 
       // 'sent' event: the sender is always us!
@@ -3692,10 +3668,6 @@ export async function startApp(): Promise<void> {
 
     event.confirm();
 
-    if (!sourceConversation) {
-      return;
-    }
-
     strictAssert(
       isValidUuid(sourceUuid),
       'onReadOrViewReceipt: Missing sourceUuid'
@@ -3825,11 +3797,6 @@ export async function startApp(): Promise<void> {
       `wasSentEncrypted=${wasSentEncrypted}`
     );
 
-    if (!sourceConversation) {
-      log.info('onDeliveryReceipt: no conversation for', source, sourceUuid);
-      return;
-    }
-
     strictAssert(
       envelopeTimestamp,
       'onDeliveryReceipt: missing envelopeTimestamp'

ts/models/conversations.ts

@@ -795,8 +795,8 @@ export class ConversationModel extends window.Backbone
     shouldSave?: boolean;
   } = {}): void {
     log.info(
-      `Conversation ${this.idForLogging()} is now unregistered, ` +
+      `setUnregistered(${this.idForLogging()}): conversation is now ` +
-        `timestamp=${timestamp}`
+        `unregistered, timestamp=${timestamp}`
     );
 
     const oldFirstUnregisteredAt = this.get('firstUnregisteredAt');
@@ -821,6 +821,26 @@ export class ConversationModel extends window.Backbone
       window.Signal.Data.updateConversation(this.attributes);
     }
 
+    const e164 = this.get('e164');
+    const pni = this.get('pni');
+    const aci = this.get('uuid');
+    if (e164 && pni && aci && pni !== aci) {
+      this.updateE164(undefined);
+      this.updatePni(undefined);
+
+      const { conversation: split } =
+        window.ConversationController.maybeMergeContacts({
+          pni,
+          e164,
+          reason: `ConversationModel.setUnregistered(${aci})`,
+        });
+
+      log.info(
+        `setUnregistered(${this.idForLogging()}): splitting pni ${pni} and ` +
+          `e164 ${e164} into a separate conversation ${split.idForLogging()}`
+      );
+    }
+
     if (
       !fromStorageService &&
       oldFirstUnregisteredAt !== this.get('firstUnregisteredAt')

ts/services/contactSync.ts

@@ -11,7 +11,6 @@ import * as Errors from '../types/errors';
 import type { ValidateConversationType } from '../model-types.d';
 import type { ConversationModel } from '../models/conversations';
 import { validateConversation } from '../util/validateConversation';
-import { strictAssert } from '../util/assert';
 import { isDirectConversation, isMe } from '../util/whatTypeOfConversation';
 import * as log from '../logging/log';
 
@@ -110,7 +109,6 @@ async function doContactSync({
       aci: details.uuid,
       reason: logId,
     });
-    strictAssert(conversation, 'need conversation to queue the job!');
 
     // It's important to use queueJob here because we might update the expiration timer
     //   and we don't want conflicts with incoming message processing happening on the

ts/services/storage.ts

@@ -1469,7 +1469,7 @@ async function processRemoteRecords(
 
   let accountItem: MergeableItemType | undefined;
 
-  const prunedStorageItems = decryptedItems.filter(item => {
+  let prunedStorageItems = decryptedItems.filter(item => {
     const { itemType, storageID, storageRecord } = item;
     if (itemType === ITEM_TYPE.ACCOUNT) {
       if (accountItem !== undefined) {
@@ -1506,6 +1506,36 @@ async function processRemoteRecords(
     return false;
   });
 
+  // Find remote contact records that:
+  // - Have `remote.pni === remote.serviceUuid` and have `remote.serviceE164`
+  // - Match local contact that has `local.serviceUuid != remote.pni`.
+  const splitPNIContacts = new Array<MergeableItemType>();
+  prunedStorageItems = prunedStorageItems.filter(item => {
+    const { itemType, storageRecord } = item;
+    const { contact } = storageRecord;
+    if (itemType !== ITEM_TYPE.CONTACT || !contact) {
+      return true;
+    }
+
+    if (
+      !contact.serviceE164 ||
+      !contact.pni ||
+      contact.pni !== contact.serviceUuid
+    ) {
+      return true;
+    }
+
+    const localUuid = window.ConversationController.get(contact.pni)?.get(
+      'uuid'
+    );
+    if (!localUuid || localUuid === contact.pni) {
+      return true;
+    }
+
+    splitPNIContacts.push(item);
+    return false;
+  });
+
   try {
     log.info(
       `storageService.process(${storageVersion}): ` +
@@ -1517,13 +1547,31 @@ async function processRemoteRecords(
           `record=${redactStorageID(accountItem.storageID, storageVersion)}`
       );
     }
+    if (splitPNIContacts.length !== 0) {
+      log.info(
+        `storageService.process(${storageVersion}): ` +
+          `split pni contacts=${splitPNIContacts.length}`
+      );
+    }
 
-    const mergedRecords = [
+    const mergeWithConcurrency = (
-      ...(await pMap(
+      items: ReadonlyArray<MergeableItemType>
-        prunedStorageItems,
+    ): Promise<Array<MergedRecordType>> => {
+      return pMap(
+        items,
         (item: MergeableItemType) => mergeRecord(storageVersion, item),
         { concurrency: 32 }
-      )),
+      );
+    };
+
+    const mergedRecords = [
+      ...(await mergeWithConcurrency(prunedStorageItems)),
+
+      // Merge split PNI contacts after processing remote records. If original
+      // e164+ACI+PNI contact is unregistered - it is going to be split so we
+      // have to make that happen first. Otherwise we will ignore ContactRecord
+      // changes on these since there is already a parent "merged" contact.
+      ...(await mergeWithConcurrency(splitPNIContacts)),
 
       // Merge Account records last since it contains the pinned conversations
       // and we need all other records merged first before we can find the pinned

ts/services/storageRecordOps.ts

@@ -964,10 +964,6 @@ export async function mergeContactRecord(
     reason: 'mergeContactRecord',
   });
 
-  if (!conversation) {
-    throw new Error(`No conversation for ${storageID}`);
-  }
-
   // We're going to ignore this; it's likely a PNI-only contact we've already merged
   if (conversation.get('uuid') !== uuid) {
     log.warn(

ts/state/ducks/accounts.ts

@@ -104,7 +104,7 @@ function checkForAccount(
             e164: phoneNumber,
             reason: 'checkForAccount',
           });
-        uuid = maybeMerged?.get('uuid');
+        uuid = maybeMerged.get('uuid');
       }
     } catch (error) {
       log.error('checkForAccount:', Errors.toLogFormat(error));

ts/test-electron/updateConversationsWithUuidLookup_test.ts

@@ -36,7 +36,7 @@ describe('updateConversationsWithUuidLookup', () => {
       aci?: string | null;
       reason?: string;
     }): {
-      conversation: ConversationModel | undefined;
+      conversation: ConversationModel;
       mergePromises: Array<Promise<void>>;
     } {
       assert(
@@ -75,8 +75,7 @@ describe('updateConversationsWithUuidLookup', () => {
         return { conversation: convoE164, mergePromises: [] };
       }
 
-      assert.fail('FakeConversationController should never get here');
+      throw new Error('FakeConversationController should never get here');
-      return { conversation: undefined, mergePromises: [] };
     }
 
     lookupOrCreate({

ts/test-mock/pnp/merge_test.ts

@@ -5,6 +5,7 @@ import { assert } from 'chai';
 import { UUIDKind, Proto, StorageState } from '@signalapp/mock-server';
 import type { PrimaryDevice } from '@signalapp/mock-server';
 import createDebug from 'debug';
+import Long from 'long';
 
 import * as durations from '../../util/durations';
 import { uuidToBytes } from '../../util/uuidToBytes';
@@ -26,7 +27,7 @@ describe('pnp/merge', function needsName() {
   let aciIdentityKey: Uint8Array;
 
   beforeEach(async () => {
-    bootstrap = new Bootstrap();
+    bootstrap = new Bootstrap({ contactCount: 0 });
     await bootstrap.init();
 
     const { server, phone } = bootstrap;
@@ -64,7 +65,7 @@ describe('pnp/merge', function needsName() {
 
       serviceE164: undefined,
       identityKey: aciIdentityKey,
-      profileKey: pniContact.profileKey.serialize(),
+      givenName: pniContact.profileName,
     });
 
     // Put both contacts in left pane
@@ -109,7 +110,7 @@ describe('pnp/merge', function needsName() {
 
       debug('opening conversation with the aci contact');
       await leftPane
-        .locator(`[data-testid="${pniContact.toContact().uuid}"]`)
+        .locator(`[data-testid="${pniContact.device.uuid}"]`)
         .click();
 
       await window.locator('.module-conversation-hero').waitFor();
@@ -166,7 +167,7 @@ describe('pnp/merge', function needsName() {
       if (finalContact === UUIDKind.ACI) {
         debug('switching back to ACI conversation');
         await leftPane
-          .locator(`[data-testid="${pniContact.toContact().uuid}"]`)
+          .locator(`[data-testid="${pniContact.device.uuid}"]`)
           .click();
 
         await window.locator('.module-conversation-hero').waitFor();
@@ -178,24 +179,12 @@ describe('pnp/merge', function needsName() {
       {
         const state = await phone.expectStorageState('consistency check');
         await phone.setStorageState(
-          state
+          state.mergeContact(pniContact, {
-            .removeRecord(
+            identityState: Proto.ContactRecord.IdentityState.DEFAULT,
-              item =>
+            whitelisted: true,
-                item.record.contact?.serviceUuid ===
+            identityKey: pniContact.publicKey.serialize(),
-                pniContact.device.getUUIDByKind(UUIDKind.ACI)
+            profileKey: pniContact.profileKey.serialize(),
-            )
+          })
-            .removeRecord(
-              item =>
-                item.record.contact?.serviceUuid ===
-                pniContact.device.getUUIDByKind(UUIDKind.PNI)
-            )
-            .addContact(pniContact, {
-              identityState: Proto.ContactRecord.IdentityState.DEFAULT,
-              whitelisted: true,
-              pni: pniContact.device.getUUIDByKind(UUIDKind.PNI),
-              identityKey: pniContact.publicKey.serialize(),
-              profileKey: pniContact.profileKey.serialize(),
-            })
         );
         await phone.sendFetchStorage({
           timestamp: bootstrap.getTimestamp(),
@@ -230,4 +219,223 @@ describe('pnp/merge', function needsName() {
       }
     });
   }
+
+  it('accepts storage service contact splitting', async () => {
+    const { phone } = bootstrap;
+
+    debug(
+      'removing both contacts from storage service, adding one combined contact'
+    );
+    {
+      const state = await phone.expectStorageState('consistency check');
+      await phone.setStorageState(
+        state.mergeContact(pniContact, {
+          identityState: Proto.ContactRecord.IdentityState.DEFAULT,
+          whitelisted: true,
+          identityKey: pniContact.publicKey.serialize(),
+          profileKey: pniContact.profileKey.serialize(),
+        })
+      );
+      await phone.sendFetchStorage({
+        timestamp: bootstrap.getTimestamp(),
+      });
+    }
+
+    const window = await app.getWindow();
+    const leftPane = window.locator('.left-pane-wrapper');
+
+    debug('opening conversation with the merged contact');
+    await leftPane
+      .locator(
+        `[data-testid="${pniContact.device.uuid}"] >> ` +
+          `"${pniContact.profileName}"`
+      )
+      .click();
+
+    await window.locator('.module-conversation-hero').waitFor();
+
+    debug('Send message to merged contact');
+    {
+      const composeArea = window.locator(
+        '.composition-area-wrapper, .conversation .ConversationView'
+      );
+      const compositionInput = composeArea.locator(
+        '[data-testid=CompositionInput]'
+      );
+
+      await compositionInput.type('Hello merged');
+      await compositionInput.press('Enter');
+    }
+
+    debug('Split contact and mark ACI as unregistered');
+    {
+      let state = await phone.expectStorageState('consistency check');
+
+      state = state.updateContact(pniContact, {
+        pni: undefined,
+        serviceE164: undefined,
+        unregisteredAtTimestamp: Long.fromNumber(bootstrap.getTimestamp()),
+      });
+
+      state = state.addContact(
+        pniContact,
+        {
+          identityState: Proto.ContactRecord.IdentityState.DEFAULT,
+          whitelisted: true,
+
+          identityKey: pniIdentityKey,
+
+          serviceE164: pniContact.device.number,
+          givenName: 'PNI Contact',
+        },
+        UUIDKind.PNI
+      );
+
+      state = state.pin(pniContact, UUIDKind.PNI);
+
+      await phone.setStorageState(state);
+      await phone.sendFetchStorage({
+        timestamp: bootstrap.getTimestamp(),
+      });
+    }
+
+    debug('Wait for pni contact to appear');
+    await leftPane
+      .locator(`[data-testid="${pniContact.device.pni}"]`)
+      .waitFor();
+
+    debug('Verify that the message is in the ACI conversation');
+    {
+      // Should have both PNI and ACI messages
+      await window.locator('.module-message__text >> "Hello merged"').waitFor();
+
+      const messages = window.locator('.module-message__text');
+      assert.strictEqual(await messages.count(), 1, 'message count');
+    }
+
+    debug('Open PNI conversation');
+    await leftPane.locator(`[data-testid="${pniContact.device.pni}"]`).click();
+
+    debug('Verify absence of messages in the PNI conversation');
+    {
+      const messages = window.locator('.module-message__text');
+      assert.strictEqual(await messages.count(), 0, 'message count');
+    }
+  });
+
+  it('splits contact when ACI becomes unregistered', async () => {
+    const { phone, server } = bootstrap;
+
+    debug(
+      'removing both contacts from storage service, adding one combined contact'
+    );
+    {
+      const state = await phone.expectStorageState('consistency check');
+      await phone.setStorageState(
+        state.mergeContact(pniContact, {
+          identityState: Proto.ContactRecord.IdentityState.DEFAULT,
+          whitelisted: true,
+          identityKey: pniContact.publicKey.serialize(),
+          profileKey: pniContact.profileKey.serialize(),
+        })
+      );
+      await phone.sendFetchStorage({
+        timestamp: bootstrap.getTimestamp(),
+      });
+    }
+
+    const window = await app.getWindow();
+    const leftPane = window.locator('.left-pane-wrapper');
+
+    debug('opening conversation with the merged contact');
+    await leftPane
+      .locator(
+        `[data-testid="${pniContact.device.uuid}"] >> ` +
+          `"${pniContact.profileName}"`
+      )
+      .click();
+
+    await window.locator('.module-conversation-hero').waitFor();
+
+    debug('Unregistering ACI');
+    server.unregister(pniContact);
+
+    const state = await phone.expectStorageState('initial state');
+
+    debug('Send message to merged contact');
+    {
+      const composeArea = window.locator(
+        '.composition-area-wrapper, .conversation .ConversationView'
+      );
+      const compositionInput = composeArea.locator(
+        '[data-testid=CompositionInput]'
+      );
+
+      await compositionInput.type('Hello merged');
+      await compositionInput.press('Enter');
+    }
+
+    debug('Verify that contact is split in storage service');
+    {
+      const newState = await phone.waitForStorageState({
+        after: state,
+      });
+
+      const { added, removed } = newState.diff(state);
+      assert.strictEqual(added.length, 2, 'only two records must be added');
+      assert.strictEqual(removed.length, 1, 'only one record must be removed');
+
+      let pniContacts = 0;
+      let aciContacts = 0;
+      for (const { contact } of added) {
+        if (!contact) {
+          throw new Error('Invalid record');
+        }
+
+        const { serviceUuid, serviceE164, pni } = contact;
+        if (serviceUuid === pniContact.device.uuid) {
+          aciContacts += 1;
+          assert.strictEqual(pni, '');
+          assert.strictEqual(serviceE164, '');
+        } else if (serviceUuid === pniContact.device.pni) {
+          pniContacts += 1;
+          assert.strictEqual(pni, serviceUuid);
+          assert.strictEqual(serviceE164, pniContact.device.number);
+        }
+      }
+      assert.strictEqual(aciContacts, 1);
+      assert.strictEqual(pniContacts, 1);
+
+      assert.strictEqual(removed[0].contact?.pni, pniContact.device.pni);
+      assert.strictEqual(
+        removed[0].contact?.serviceUuid,
+        pniContact.device.uuid
+      );
+
+      // Pin PNI so that it appears in the left pane
+      const updated = newState.pin(pniContact, UUIDKind.PNI);
+      await phone.setStorageState(updated);
+      await phone.sendFetchStorage({
+        timestamp: bootstrap.getTimestamp(),
+      });
+    }
+
+    debug('Verify that the message is in the ACI conversation');
+    {
+      // Should have both PNI and ACI messages
+      await window.locator('.module-message__text >> "Hello merged"').waitFor();
+
+      const messages = window.locator('.module-message__text');
+      assert.strictEqual(await messages.count(), 1, 'message count');
+    }
+
+    debug('Open PNI conversation');
+    await leftPane.locator(`[data-testid="${pniContact.device.pni}"]`).click();
+
+    debug('Verify absence of messages in the PNI conversation');
+    {
+      const messages = window.locator('.module-message__text');
+      assert.strictEqual(await messages.count(), 0, 'message count');
+    }
+  });
 });

yarn.lock

@@ -2178,10 +2178,10 @@
     node-gyp-build "^4.2.3"
     uuid "^8.3.0"
 
-"@signalapp/mock-server@2.12.1":
+"@signalapp/mock-server@2.13.0":
-  version "2.12.1"
+  version "2.13.0"
-  resolved "https://registry.yarnpkg.com/@signalapp/mock-server/-/mock-server-2.12.1.tgz#456ee3c9458363d333bd803910f874d388f28d04"
+  resolved "https://registry.yarnpkg.com/@signalapp/mock-server/-/mock-server-2.13.0.tgz#928c95a4890b21c811d29b3b0765aa3a35d28006"
-  integrity sha512-c8ndwTtDoRPRZAWjbBeVH79DQLA4UvKPztgJUqtDOqFCju1+NMKbjxrK+v1PJQvhhOMANbG6Z3qTCl4UGcm/zQ==
+  integrity sha512-YZZdEYhVoWT4Ih4ZJLu1AaLtn+i8RFO/Tr55/72TvrtcWLjolik3YTnGqHnAcxNR+6mfeCfhpp7+rtexYnnefw==
   dependencies:
     "@signalapp/libsignal-client" "^0.20.0"
     debug "^4.3.2"