Further PNP fixes

package.json

@@ -201,7 +201,7 @@
     "@electron/notarize": "2.1.0",
     "@formatjs/intl": "2.6.7",
     "@mixer/parallel-prettier": "2.0.3",
-    "@signalapp/mock-server": "5.0.0",
+    "@signalapp/mock-server": "5.0.1",
     "@storybook/addon-a11y": "7.4.5",
     "@storybook/addon-actions": "7.4.5",
     "@storybook/addon-controls": "7.4.5",

ts/ConversationController.ts

@@ -102,7 +102,6 @@ function applyChangeToConversation(
 
 export type CombineConversationsParams = Readonly<{
   current: ConversationModel;
-  fromPniSignature?: boolean;
   obsolete: ConversationModel;
   obsoleteTitleInfo?: ConversationRenderInfoType;
 }>;
@@ -495,6 +494,9 @@ export class ConversationController {
     if (providedPni) {
       dataProvided.push(`pni=${providedPni}`);
     }
+    if (fromPniSignature) {
+      dataProvided.push(`fromPniSignature=${fromPniSignature}`);
+    }
     const logId = `maybeMergeContacts/${reason}/${dataProvided.join(',')}`;
 
     const aci = providedAci
@@ -677,7 +679,6 @@ export class ConversationController {
           mergePromises.push(
             mergeOldAndNew({
               current: targetConversation,
-              fromPniSignature,
               logId,
               obsolete: match,
               obsoleteTitleInfo,
@@ -1021,7 +1022,6 @@ export class ConversationController {
     current,
     obsolete,
     obsoleteTitleInfo,
-    fromPniSignature,
   }: CombineConversationsParams): Promise<void> {
     const logId = `combineConversations/${obsolete.id}->${current.id}`;
 
@@ -1047,12 +1047,13 @@ export class ConversationController {
 
     const obsoleteActiveAt = obsolete.get('active_at');
     const currentActiveAt = current.get('active_at');
-    const activeAt =
+    let activeAt: number | null | undefined;
-      !obsoleteActiveAt ||
+
-      !currentActiveAt ||
+    if (obsoleteActiveAt && currentActiveAt) {
-      currentActiveAt > obsoleteActiveAt
+      activeAt = Math.max(obsoleteActiveAt, currentActiveAt);
-        ? currentActiveAt
+    } else {
-        : obsoleteActiveAt;
+      activeAt = obsoleteActiveAt || currentActiveAt;
+    }
     current.set('active_at', activeAt);
 
     const dataToCopy: Partial<ConversationAttributesType> = pick(
@@ -1195,12 +1196,7 @@ export class ConversationController {
     const titleIsUseful = Boolean(
       obsoleteTitleInfo && getTitleNoDefault(obsoleteTitleInfo)
     );
-    if (
+    if (obsoleteTitleInfo && titleIsUseful && obsoleteHadMessages) {
-      obsoleteTitleInfo &&
-      titleIsUseful &&
-      !fromPniSignature &&
-      obsoleteHadMessages
-    ) {
       drop(current.addConversationMerge(obsoleteTitleInfo));
     }
 

ts/SignalProtocolStore.ts

@@ -2635,7 +2635,7 @@ export class SignalProtocolStore extends EventEmitter {
     this.ourRegistrationIds.set(pni, registrationId);
 
     // Update database
-    await Promise.all([
+    await Promise.all<void>([
       storage.put('identityKeyMap', {
         ...(storage.get('identityKeyMap') || {}),
         [pni]: {
@@ -2647,11 +2647,11 @@ export class SignalProtocolStore extends EventEmitter {
         ...(storage.get('registrationIdMap') || {}),
         [pni]: registrationId,
       }),
-      async () => {
+      (async () => {
         const newId = signedPreKey.id() + 1;
         log.warn(`${logId}: Updating next signed pre key id to ${newId}`);
         await storage.put(SIGNED_PRE_KEY_ID_KEY[ServiceIdKind.PNI], newId);
-      },
+      })(),
       this.storeSignedPreKey(
         pni,
         signedPreKey.id(),
@@ -2662,14 +2662,14 @@ export class SignalProtocolStore extends EventEmitter {
         true,
         signedPreKey.timestamp()
       ),
-      async () => {
+      (async () => {
         if (!lastResortKyberPreKey) {
           return;
         }
         const newId = lastResortKyberPreKey.id() + 1;
         log.warn(`${logId}: Updating next kyber pre key id to ${newId}`);
         await storage.put(KYBER_KEY_ID_KEY[ServiceIdKind.PNI], newId);
-      },
+      })(),
       lastResortKyberPreKeyBytes && lastResortKyberPreKey
         ? this.storeKyberPreKeys(pni, [
             {

ts/test-mock/pnp/merge_test.ts

@@ -214,7 +214,7 @@ describe('pnp/merge', function (this: Mocha.Suite) {
         const notifications = window.locator('.SystemMessage');
         assert.strictEqual(
           await notifications.count(),
-          withPNIMessage && !pniSignatureVerified ? 1 : 0,
+          withPNIMessage ? 1 : 0,
           'notification count'
         );
 

ts/test-mock/pnp/pni_signature_test.ts

@@ -377,9 +377,15 @@ describe('pnp/PNI Signature', function (this: Mocha.Suite) {
       const messages = window.locator('.module-message__text');
       assert.strictEqual(await messages.count(), 3, 'messages');
 
-      // No notifications
+      // Merge notification
       const notifications = window.locator('.SystemMessage');
-      assert.strictEqual(await notifications.count(), 0, 'notifications');
+      assert.strictEqual(await notifications.count(), 1, 'notifications');
+
+      const first = await notifications.first();
+      assert.match(
+        await first.innerText(),
+        /Your message history with ACI Contact and their number .* has been merged./
+      );
 
       assert.isEmpty(await phone.getOrphanedStorageKeys());
     }

ts/textsecure/AccountManager.ts

@@ -47,6 +47,7 @@ import {
   isUntaggedPniString,
 } from '../types/ServiceId';
 import { normalizeAci } from '../util/normalizeAci';
+import { drop } from '../util/drop';
 import { isMoreRecentThan, isOlderThan } from '../util/timestamp';
 import { ourProfileKeyService } from '../services/ourProfileKey';
 import { strictAssert } from '../util/assert';
@@ -558,7 +559,10 @@ export default class AccountManager extends EventTarget {
     return toUpload;
   }
 
-  async maybeUpdateKeys(serviceIdKind: ServiceIdKind): Promise<void> {
+  async maybeUpdateKeys(
+    serviceIdKind: ServiceIdKind,
+    forceUpdate = false
+  ): Promise<void> {
     const logId = `maybeUpdateKeys(${serviceIdKind})`;
     await this.queueTask(async () => {
       const { storage } = window.textsecure;
@@ -583,7 +587,7 @@ export default class AccountManager extends EventTarget {
         await this.server.getMyKeyCounts(serviceIdKind);
 
       let preKeys: Array<UploadPreKeyType> | undefined;
-      if (preKeyCount < PRE_KEY_MINIMUM) {
+      if (preKeyCount < PRE_KEY_MINIMUM || forceUpdate) {
         log.info(
           `${logId}: Server prekey count is ${preKeyCount}, generating a new set`
         );
@@ -591,7 +595,7 @@ export default class AccountManager extends EventTarget {
       }
 
       let pqPreKeys: Array<UploadKyberPreKeyType> | undefined;
-      if (kyberPreKeyCount < PRE_KEY_MINIMUM) {
+      if (kyberPreKeyCount < PRE_KEY_MINIMUM || forceUpdate) {
         log.info(
           `${logId}: Server kyber prekey count is ${kyberPreKeyCount}, generating a new set`
         );
@@ -599,9 +603,13 @@ export default class AccountManager extends EventTarget {
       }
 
       const pqLastResortPreKey = await this.maybeUpdateLastResortKyberKey(
-        serviceIdKind
+        serviceIdKind,
+        forceUpdate
+      );
+      const signedPreKey = await this.maybeUpdateSignedPreKey(
+        serviceIdKind,
+        forceUpdate
       );
-      const signedPreKey = await this.maybeUpdateSignedPreKey(serviceIdKind);
 
       if (
         !preKeys?.length &&
@@ -615,7 +623,7 @@ export default class AccountManager extends EventTarget {
 
       const keySummary: Array<string> = [];
       if (preKeys?.length) {
-        keySummary.push(`${!preKeys?.length || 0} prekeys`);
+        keySummary.push(`${preKeys.length} prekeys`);
       }
       if (signedPreKey) {
         keySummary.push('a signed prekey');
@@ -624,7 +632,7 @@ export default class AccountManager extends EventTarget {
         keySummary.push('a last-resort kyber prekey');
       }
       if (pqPreKeys?.length) {
-        keySummary.push(`${!pqPreKeys?.length || 0} kyber prekeys`);
+        keySummary.push(`${pqPreKeys.length} kyber prekeys`);
       }
       log.info(`${logId}: Uploading with ${keySummary.join(', ')}`);
 
@@ -699,7 +707,8 @@ export default class AccountManager extends EventTarget {
   }
 
   private async maybeUpdateSignedPreKey(
-    serviceIdKind: ServiceIdKind
+    serviceIdKind: ServiceIdKind,
+    forceUpdate = false
   ): Promise<UploadSignedPreKeyType | undefined> {
     const ourServiceId =
       window.textsecure.storage.user.getCheckedServiceId(serviceIdKind);
@@ -713,7 +722,10 @@ export default class AccountManager extends EventTarget {
     const mostRecent = confirmedKeys[0];
 
     const lastUpdate = mostRecent?.created_at;
-    if (isMoreRecentThan(lastUpdate || 0, SIGNED_PRE_KEY_ROTATION_AGE)) {
+    if (
+      !forceUpdate &&
+      isMoreRecentThan(lastUpdate || 0, SIGNED_PRE_KEY_ROTATION_AGE)
+    ) {
       log.warn(
         `${logId}: ${confirmedKeys.length} confirmed keys, ` +
           `most recent was created ${lastUpdate}. No need to update.`
@@ -765,7 +777,8 @@ export default class AccountManager extends EventTarget {
   }
 
   private async maybeUpdateLastResortKyberKey(
-    serviceIdKind: ServiceIdKind
+    serviceIdKind: ServiceIdKind,
+    forceUpdate = false
   ): Promise<UploadSignedPreKeyType | undefined> {
     const ourServiceId =
       window.textsecure.storage.user.getCheckedServiceId(serviceIdKind);
@@ -779,7 +792,10 @@ export default class AccountManager extends EventTarget {
     const mostRecent = confirmedKeys[0];
 
     const lastUpdate = mostRecent?.createdAt;
-    if (isMoreRecentThan(lastUpdate || 0, LAST_RESORT_KEY_ROTATION_AGE)) {
+    if (
+      !forceUpdate &&
+      isMoreRecentThan(lastUpdate || 0, LAST_RESORT_KEY_ROTATION_AGE)
+    ) {
       log.warn(
         `${logId}: ${confirmedKeys.length} confirmed keys, ` +
           `most recent was created ${lastUpdate}. No need to update.`
@@ -1389,17 +1405,20 @@ export default class AccountManager extends EventTarget {
       await storage.protocol.updateOurPniKeyMaterial(pni, keyMaterial);
 
       // Intentionally not awaiting since this is processed on encrypted queue
-      // of MessageReceiver.
+      // of MessageReceiver. Note that `maybeUpdateKeys` runs on the queue so
-      void this.queueTask(async () => {
+      // we don't have to wrap it with `queueTask`.
-        try {
+      drop(
-          await this.maybeUpdateKeys(ServiceIdKind.PNI);
+        (async () => {
-        } catch (error) {
+          try {
-          log.error(
+            await this.maybeUpdateKeys(ServiceIdKind.PNI, true);
-            `${logId}: Failed to upload PNI prekeys. Moving on`,
+          } catch (error) {
-            Errors.toLogFormat(error)
+            log.error(
-          );
+              `${logId}: Failed to upload PNI prekeys. Moving on`,
-        }
+              Errors.toLogFormat(error)
-      });
+            );
+          }
+        })()
+      );
 
       // PNI has changed and credentials are no longer valid
       await storage.put('groupCredentials', []);

yarn.lock

@@ -3973,10 +3973,10 @@
     type-fest "^3.5.0"
     uuid "^8.3.0"
 
-"@signalapp/mock-server@5.0.0":
+"@signalapp/mock-server@5.0.1":
-  version "5.0.0"
+  version "5.0.1"
-  resolved "https://registry.yarnpkg.com/@signalapp/mock-server/-/mock-server-5.0.0.tgz#2534925b0e248b6211cda420641508d92c5a97cb"
+  resolved "https://registry.yarnpkg.com/@signalapp/mock-server/-/mock-server-5.0.1.tgz#648eb44b91f1ecbb0b3dc1791101fb5d67ac0e73"
-  integrity sha512-HlEgiBKmPp1Rl5ltfg/FmIfH2DQeCzb7lML8SscRvZOQSE83+MRf7JZtwIlDQxg3YmhQo0RmReQNv8yz5KtbVg==
+  integrity sha512-KuaHznpxubFib9LJiIP6lT8Oynuni4yzrItetxVwN9YPMTlFa0TYvaf+InODnH7tnM1bY1NCku9SYy+0H2ejMQ==
   dependencies:
     "@signalapp/libsignal-client" "^0.39.2"
     debug "^4.3.2"