From dd5b66039daea79907db6b8ce258e2bc5ec5e0e6 Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Fri, 16 Feb 2024 12:39:58 -0800 Subject: [PATCH] Further PNP fixes --- package.json | 2 +- ts/ConversationController.ts | 26 +++++------ ts/SignalProtocolStore.ts | 10 ++-- ts/test-mock/pnp/merge_test.ts | 2 +- ts/test-mock/pnp/pni_signature_test.ts | 10 +++- ts/textsecure/AccountManager.ts | 63 +++++++++++++++++--------- yarn.lock | 8 ++-- 7 files changed, 71 insertions(+), 50 deletions(-) diff --git a/package.json b/package.json index 61bdb627d2d0..e5994adb2e33 100644 --- a/package.json +++ b/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", diff --git a/ts/ConversationController.ts b/ts/ConversationController.ts index bfe91a9050cd..197ddcff8827 100644 --- a/ts/ConversationController.ts +++ b/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 { 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 = - !obsoleteActiveAt || - !currentActiveAt || - currentActiveAt > obsoleteActiveAt - ? currentActiveAt - : obsoleteActiveAt; + let activeAt: number | null | undefined; + + if (obsoleteActiveAt && currentActiveAt) { + activeAt = Math.max(obsoleteActiveAt, currentActiveAt); + } else { + activeAt = obsoleteActiveAt || currentActiveAt; + } current.set('active_at', activeAt); const dataToCopy: Partial = pick( @@ -1195,12 +1196,7 @@ export class ConversationController { const titleIsUseful = Boolean( obsoleteTitleInfo && getTitleNoDefault(obsoleteTitleInfo) ); - if ( - obsoleteTitleInfo && - titleIsUseful && - !fromPniSignature && - obsoleteHadMessages - ) { + if (obsoleteTitleInfo && titleIsUseful && obsoleteHadMessages) { drop(current.addConversationMerge(obsoleteTitleInfo)); } diff --git a/ts/SignalProtocolStore.ts b/ts/SignalProtocolStore.ts index e167e44d969e..5fff48642d39 100644 --- a/ts/SignalProtocolStore.ts +++ b/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([ 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, [ { diff --git a/ts/test-mock/pnp/merge_test.ts b/ts/test-mock/pnp/merge_test.ts index 39d243140b1f..7a4d375cbc1c 100644 --- a/ts/test-mock/pnp/merge_test.ts +++ b/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' ); diff --git a/ts/test-mock/pnp/pni_signature_test.ts b/ts/test-mock/pnp/pni_signature_test.ts index e31681a76c0f..0bcd084172ed 100644 --- a/ts/test-mock/pnp/pni_signature_test.ts +++ b/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()); } diff --git a/ts/textsecure/AccountManager.ts b/ts/textsecure/AccountManager.ts index a4a7d7a3a769..b3033402c059 100644 --- a/ts/textsecure/AccountManager.ts +++ b/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 { + async maybeUpdateKeys( + serviceIdKind: ServiceIdKind, + forceUpdate = false + ): Promise { 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 | 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 | 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 = []; 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 { 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 { 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. - void this.queueTask(async () => { - try { - await this.maybeUpdateKeys(ServiceIdKind.PNI); - } catch (error) { - log.error( - `${logId}: Failed to upload PNI prekeys. Moving on`, - Errors.toLogFormat(error) - ); - } - }); + // of MessageReceiver. Note that `maybeUpdateKeys` runs on the queue so + // we don't have to wrap it with `queueTask`. + drop( + (async () => { + try { + await this.maybeUpdateKeys(ServiceIdKind.PNI, true); + } catch (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', []); diff --git a/yarn.lock b/yarn.lock index f235b7aecf6d..a2cdf4cfe4bd 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3973,10 +3973,10 @@ type-fest "^3.5.0" uuid "^8.3.0" -"@signalapp/mock-server@5.0.0": - version "5.0.0" - resolved "https://registry.yarnpkg.com/@signalapp/mock-server/-/mock-server-5.0.0.tgz#2534925b0e248b6211cda420641508d92c5a97cb" - integrity sha512-HlEgiBKmPp1Rl5ltfg/FmIfH2DQeCzb7lML8SscRvZOQSE83+MRf7JZtwIlDQxg3YmhQo0RmReQNv8yz5KtbVg== +"@signalapp/mock-server@5.0.1": + version "5.0.1" + resolved "https://registry.yarnpkg.com/@signalapp/mock-server/-/mock-server-5.0.1.tgz#648eb44b91f1ecbb0b3dc1791101fb5d67ac0e73" + integrity sha512-KuaHznpxubFib9LJiIP6lT8Oynuni4yzrItetxVwN9YPMTlFa0TYvaf+InODnH7tnM1bY1NCku9SYy+0H2ejMQ== dependencies: "@signalapp/libsignal-client" "^0.39.2" debug "^4.3.2"