Don't mark conversations as unregistered unless there's no UUID

ts/background.ts

@@ -13,6 +13,7 @@ import { isMoreRecentThan, isOlderThan } from './util/timestamp';
 import { isValidReactionEmoji } from './reactions/isValidReactionEmoji';
 import { ConversationModel } from './models/conversations';
 import { createBatcher } from './util/batcher';
+import { updateConversationsWithUuidLookup } from './updateConversationsWithUuidLookup';
 
 const MAX_ATTACHMENT_DOWNLOAD_AGE = 3600 * 72 * 1000;
 
@@ -1755,7 +1756,7 @@ export async function startApp(): Promise<void> {
         }
 
         try {
-          const lonelyE164s = window
+          const lonelyE164Conversations = window
             .getConversations()
             .filter(c =>
               Boolean(
@@ -1764,30 +1765,12 @@ export async function startApp(): Promise<void> {
                   !c.get('uuid') &&
                   !c.isEverUnregistered()
               )
-            )
-            .map(c => c.get('e164'))
-            .filter(Boolean) as Array<string>;
-
-          if (lonelyE164s.length > 0) {
-            const lookup = await window.textsecure.messaging.getUuidsForE164s(
-              lonelyE164s
             );
-            const e164s = Object.keys(lookup);
-            e164s.forEach(e164 => {
-              const uuid = lookup[e164];
-              if (!uuid) {
-                const byE164 = window.ConversationController.get(e164);
-                if (byE164) {
-                  byE164.setUnregistered();
-                }
-              }
-              window.ConversationController.ensureContactIds({
-                e164,
-                uuid,
-                highTrust: true,
+          await updateConversationsWithUuidLookup({
+            conversationController: window.ConversationController,
+            conversations: lonelyE164Conversations,
+            messaging: window.textsecure.messaging,
           });
-            });
-          }
         } catch (error) {
           window.log.error(
             'connect: Error fetching UUIDs for lonely e164s:',

ts/test-electron/updateConversationsWithUuidLookup_test.ts

@@ -0,0 +1,211 @@
+// Copyright 2021 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+/* eslint-disable @typescript-eslint/no-non-null-assertion */
+
+import { assert } from 'chai';
+import sinon from 'sinon';
+import { v4 as uuid } from 'uuid';
+import { ConversationModel } from '../models/conversations';
+import { ConversationAttributesType } from '../model-types.d';
+import SendMessage from '../textsecure/SendMessage';
+
+import { updateConversationsWithUuidLookup } from '../updateConversationsWithUuidLookup';
+
+describe('updateConversationsWithUuidLookup', () => {
+  class FakeConversationController {
+    constructor(
+      private readonly conversations: Array<ConversationModel> = []
+    ) {}
+
+    get(id?: string | null): ConversationModel | undefined {
+      return this.conversations.find(
+        conversation =>
+          conversation.id === id ||
+          conversation.get('e164') === id ||
+          conversation.get('uuid') === id
+      );
+    }
+
+    ensureContactIds({
+      e164,
+      uuid: uuidFromServer,
+      highTrust,
+    }: {
+      e164?: string | null;
+      uuid?: string | null;
+      highTrust?: boolean;
+    }): string | undefined {
+      assert(
+        e164,
+        'FakeConversationController is not set up for this case (E164 must be provided)'
+      );
+      assert(
+        uuid,
+        'FakeConversationController is not set up for this case (UUID must be provided)'
+      );
+      assert(
+        highTrust,
+        'FakeConversationController is not set up for this case (must be "high trust")'
+      );
+      const normalizedUuid = uuidFromServer!.toLowerCase();
+
+      const convoE164 = this.get(e164);
+      const convoUuid = this.get(normalizedUuid);
+      assert(
+        convoE164 || convoUuid,
+        'FakeConversationController is not set up for this case (at least one conversation should be found)'
+      );
+
+      if (convoE164 && convoUuid) {
+        if (convoE164 === convoUuid) {
+          return convoUuid.get('id');
+        }
+
+        convoE164.unset('e164');
+        convoUuid.updateE164(e164);
+        return convoUuid.get('id');
+      }
+
+      if (convoE164 && !convoUuid) {
+        convoE164.updateUuid(normalizedUuid);
+        return convoE164.get('id');
+      }
+
+      assert.fail('FakeConversationController should never get here');
+      return undefined;
+    }
+  }
+
+  function createConversation(
+    attributes: Readonly<Partial<ConversationAttributesType>> = {}
+  ): ConversationModel {
+    return new ConversationModel({
+      id: uuid(),
+      inbox_position: 0,
+      isPinned: false,
+      lastMessageDeletedForEveryone: false,
+      markedUnread: false,
+      messageCount: 1,
+      profileSharing: true,
+      sentMessageCount: 0,
+      type: 'private' as const,
+      version: 0,
+      ...attributes,
+    });
+  }
+
+  let sinonSandbox: sinon.SinonSandbox;
+
+  let fakeGetUuidsForE164s: sinon.SinonStub;
+  let fakeMessaging: Pick<SendMessage, 'getUuidsForE164s'>;
+
+  beforeEach(() => {
+    sinonSandbox = sinon.createSandbox();
+
+    sinonSandbox.stub(window.Signal.Data, 'updateConversation');
+
+    fakeGetUuidsForE164s = sinonSandbox.stub().resolves({});
+    fakeMessaging = { getUuidsForE164s: fakeGetUuidsForE164s };
+  });
+
+  afterEach(() => {
+    sinonSandbox.restore();
+  });
+
+  it('does nothing when called with an empty array', async () => {
+    await updateConversationsWithUuidLookup({
+      conversationController: new FakeConversationController(),
+      conversations: [],
+      messaging: fakeMessaging,
+    });
+
+    sinon.assert.notCalled(fakeMessaging.getUuidsForE164s as sinon.SinonStub);
+  });
+
+  it('does nothing when called with an array of conversations that lack E164s', async () => {
+    await updateConversationsWithUuidLookup({
+      conversationController: new FakeConversationController(),
+      conversations: [
+        createConversation(),
+        createConversation({ uuid: uuid() }),
+      ],
+      messaging: fakeMessaging,
+    });
+
+    sinon.assert.notCalled(fakeMessaging.getUuidsForE164s as sinon.SinonStub);
+  });
+
+  it('updates conversations with their UUID', async () => {
+    const conversation1 = createConversation({ e164: '+13215559876' });
+    const conversation2 = createConversation({
+      e164: '+16545559876',
+      uuid: 'should be overwritten',
+    });
+
+    const uuid1 = uuid();
+    const uuid2 = uuid();
+
+    fakeGetUuidsForE164s.resolves({
+      '+13215559876': uuid1,
+      '+16545559876': uuid2,
+    });
+
+    await updateConversationsWithUuidLookup({
+      conversationController: new FakeConversationController([
+        conversation1,
+        conversation2,
+      ]),
+      conversations: [conversation1, conversation2],
+      messaging: fakeMessaging,
+    });
+
+    assert.strictEqual(conversation1.get('uuid'), uuid1);
+    assert.strictEqual(conversation2.get('uuid'), uuid2);
+  });
+
+  it("marks conversations unregistered if we didn't have a UUID for them and the server also doesn't have one", async () => {
+    const conversation = createConversation({ e164: '+13215559876' });
+    assert.isUndefined(
+      conversation.get('discoveredUnregisteredAt'),
+      'Test was not set up correctly'
+    );
+
+    fakeGetUuidsForE164s.resolves({ '+13215559876': null });
+
+    await updateConversationsWithUuidLookup({
+      conversationController: new FakeConversationController([conversation]),
+      conversations: [conversation],
+      messaging: fakeMessaging,
+    });
+
+    assert.approximately(
+      conversation.get('discoveredUnregisteredAt') || 0,
+      Date.now(),
+      5000
+    );
+  });
+
+  it("doesn't mark conversations unregistered if we already had a UUID for them, even if the server doesn't return one", async () => {
+    const existingUuid = uuid();
+    const conversation = createConversation({
+      e164: '+13215559876',
+      uuid: existingUuid,
+    });
+    assert.isUndefined(
+      conversation.get('discoveredUnregisteredAt'),
+      'Test was not set up correctly'
+    );
+
+    fakeGetUuidsForE164s.resolves({ '+13215559876': null });
+
+    await updateConversationsWithUuidLookup({
+      conversationController: new FakeConversationController([conversation]),
+      conversations: [conversation],
+      messaging: fakeMessaging,
+    });
+
+    assert.strictEqual(conversation.get('uuid'), existingUuid);
+    assert.isUndefined(conversation.get('discoveredUnregisteredAt'));
+  });
+});

ts/textsecure/OutgoingMessage.ts

@@ -1,4 +1,4 @@
-// Copyright 2020 Signal Messenger, LLC
+// Copyright 2020-2021 Signal Messenger, LLC
 // SPDX-License-Identifier: AGPL-3.0-only
 
 /* eslint-disable guard-for-in */
@@ -38,6 +38,7 @@ import {
 } from './Errors';
 import { isValidNumber } from '../types/PhoneNumber';
 import { Sessions, IdentityKeys } from '../LibSignalStores';
+import { updateConversationsWithUuidLookup } from '../updateConversationsWithUuidLookup';
 
 export const enum SenderCertificateMode {
   WithE164,
@@ -652,29 +653,26 @@ export default class OutgoingMessage {
             'sendToIdentifier: window.textsecure.messaging is not available!'
           );
         }
-        try {
-          const lookup = await window.textsecure.messaging.getUuidsForE164s([
-            identifier,
-          ]);
-          const uuid = lookup[identifier];
-          if (uuid) {
-            window.ConversationController.ensureContactIds({
-              uuid,
-              e164: identifier,
-              highTrust: true,
-            });
-            identifier = uuid;
-          } else {
-            const c = window.ConversationController.get(identifier);
-            if (c) {
-              c.setUnregistered();
-            }
 
+        try {
+          await updateConversationsWithUuidLookup({
+            conversationController: window.ConversationController,
+            conversations: [
+              window.ConversationController.getOrCreate(identifier, 'private'),
+            ],
+            messaging: window.textsecure.messaging,
+          });
+
+          const uuid = window.ConversationController.get(identifier)?.get(
+            'uuid'
+          );
+          if (!uuid) {
             throw new UnregisteredUserError(
               identifier,
               new Error('User is not registered')
             );
           }
+          identifier = uuid;
         } catch (error) {
           window.log.error(
             `sentToIdentifier: Failed to fetch UUID for identifier ${identifier}`,

ts/updateConversationsWithUuidLookup.ts

@@ -0,0 +1,63 @@
+// Copyright 2021 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import { ConversationController } from './ConversationController';
+import { ConversationModel } from './models/conversations';
+import SendMessage from './textsecure/SendMessage';
+import { assert } from './util/assert';
+import { getOwn } from './util/getOwn';
+import { isNotNil } from './util/isNotNil';
+
+export async function updateConversationsWithUuidLookup({
+  conversationController,
+  conversations,
+  messaging,
+}: Readonly<{
+  conversationController: Pick<
+    ConversationController,
+    'ensureContactIds' | 'get'
+  >;
+  conversations: ReadonlyArray<ConversationModel>;
+  messaging: Pick<SendMessage, 'getUuidsForE164s'>;
+}>): Promise<void> {
+  const e164s = conversations
+    .map(conversation => conversation.get('e164'))
+    .filter(isNotNil);
+  if (!e164s.length) {
+    return;
+  }
+
+  const serverLookup = await messaging.getUuidsForE164s(e164s);
+
+  conversations.forEach(conversation => {
+    const e164 = conversation.get('e164');
+    if (!e164) {
+      return;
+    }
+
+    let finalConversation: ConversationModel;
+
+    const uuidFromServer = getOwn(serverLookup, e164);
+    if (uuidFromServer) {
+      const finalConversationId = conversationController.ensureContactIds({
+        e164,
+        uuid: uuidFromServer,
+        highTrust: true,
+      });
+      const maybeFinalConversation = conversationController.get(
+        finalConversationId
+      );
+      assert(
+        maybeFinalConversation,
+        'updateConversationsWithUuidLookup: expected a conversation to be found or created'
+      );
+      finalConversation = maybeFinalConversation;
+    } else {
+      finalConversation = conversation;
+    }
+
+    if (!finalConversation.get('e164') || !finalConversation.get('uuid')) {
+      finalConversation.setUnregistered();
+    }
+  });
+}