From 5d45197fe240d3a5f509811f07a05a0c8de31709 Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Tue, 9 Aug 2022 16:46:01 -0700 Subject: [PATCH] Remove restriction on maybeMergeContacts, combineConversations fixes --- ts/ConversationController.ts | 142 +++++++++++------- .../ConversationController_test.ts | 52 ++++--- ts/util/MessageController.ts | 9 ++ 3 files changed, 129 insertions(+), 74 deletions(-) diff --git a/ts/ConversationController.ts b/ts/ConversationController.ts index 2b04e2581e9a..a6ddb7c7cceb 100644 --- a/ts/ConversationController.ts +++ b/ts/ConversationController.ts @@ -1,27 +1,29 @@ // Copyright 2020-2022 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only -import { debounce, uniq, without } from 'lodash'; +import { debounce, pick, uniq, without } from 'lodash'; import PQueue from 'p-queue'; -import dataInterface from './sql/Client'; import type { ConversationModelCollectionType, ConversationAttributesType, ConversationAttributesTypeType, } from './model-types.d'; import type { ConversationModel } from './models/conversations'; +import type { MessageModel } from './models/messages'; +import type { UUIDStringType } from './types/UUID'; + +import dataInterface from './sql/Client'; +import * as log from './logging/log'; +import * as Errors from './types/errors'; import { getContactId } from './messages/helpers'; import { maybeDeriveGroupV2Id } from './groups'; import { assert, strictAssert } from './util/assert'; import { isGroupV1, isGroupV2 } from './util/whatTypeOfConversation'; import { getConversationUnreadCountForAppBadge } from './util/getConversationUnreadCountForAppBadge'; import { UUID, isValidUuid, UUIDKind } from './types/UUID'; -import type { UUIDStringType } from './types/UUID'; import { Address } from './types/Address'; import { QualifiedAddress } from './types/QualifiedAddress'; -import * as log from './logging/log'; -import * as Errors from './types/errors'; import { sleep } from './util/sleep'; import { isNotNil } from './util/isNotNil'; import { MINUTE, SECOND } from './util/durations'; @@ -87,7 +89,7 @@ function applyChangeToConversation( // Note: we don't do a conversation.set here, because change is limited to these fields } -async function mergeConversations({ +async function safeCombineConversations({ logId, oldConversation, newConversation, @@ -101,12 +103,6 @@ async function mergeConversations({ newConversation, oldConversation ); - - // If the old conversation was currently displayed, we load the new one - window.Whisper.events.trigger('refreshConversation', { - newId: newConversation.get('id'), - oldId: oldConversation.get('id'), - }); } catch (error) { log.warn( `${logId}: error combining contacts: ${Errors.toLogFormat(error)}` @@ -421,7 +417,7 @@ export class ConversationController { e164, pni: providedPni, reason, - mergeOldAndNew = mergeConversations, + mergeOldAndNew = safeCombineConversations, }: { aci?: string; e164?: string; @@ -455,10 +451,6 @@ export class ConversationController { ); } - if (pni && !e164) { - throw new Error(`${logId}: Cannot provide pni without an e164`); - } - const identifier = aci || e164 || pni; strictAssert(identifier, `${logId}: identifier must be truthy!`); @@ -575,7 +567,7 @@ export class ConversationController { } } else if (targetConversation && !targetConversation?.get(key)) { // This is mostly for the situation where PNI was erased when updating e164 - // log.debug(`${logId}: Re-adding ${key} on target conversation`); + log.debug(`${logId}: Re-adding ${key} on target conversation`); applyChangeToConversation(targetConversation, { [key]: value, }); @@ -821,36 +813,61 @@ export class ConversationController { current: ConversationModel, obsolete: ConversationModel ): Promise { + const logId = `combineConversations/${obsolete.id}->${current.id}`; + return this._combineConversationsQueue.add(async () => { const conversationType = current.get('type'); if (!this.get(obsolete.id)) { - log.warn( - `combineConversations: Already combined obsolete conversation ${obsolete.id}` - ); + log.warn(`${logId}: Already combined obsolete conversation`); } if (obsolete.get('type') !== conversationType) { assert( false, - 'combineConversations cannot combine a private and group conversation. Doing nothing' + `${logId}: cannot combine a private and group conversation. Doing nothing` ); return; } + const dataToCopy: Partial = pick( + obsolete.attributes, + [ + 'conversationColor', + 'customColor', + 'customColorId', + 'draftAttachments', + 'draftBodyRanges', + 'draftTimestamp', + 'messageCount', + 'messageRequestResponseType', + 'quotedMessageId', + 'sentMessageCount', + ] + ); + + const keys = Object.keys(dataToCopy) as Array< + keyof ConversationAttributesType + >; + keys.forEach(key => { + if (current.get(key) === undefined) { + current.set(key, dataToCopy[key]); + + // To ensure that any files on disk don't get deleted out from under us + if (key === 'draftAttachments') { + obsolete.set(key, undefined); + } + } + }); + const obsoleteId = obsolete.get('id'); const obsoleteUuid = obsolete.getUuid(); const currentId = current.get('id'); - log.warn('combineConversations: Combining two conversations', { - obsolete: obsoleteId, - current: currentId, - }); + log.warn(`${logId}: Combining two conversations...`); if (conversationType === 'private' && obsoleteUuid) { if (!current.get('profileKey') && obsolete.get('profileKey')) { - log.warn( - 'combineConversations: Copying profile key from old to new contact' - ); + log.warn(`${logId}: Copying profile key from old to new contact`); const profileKey = obsolete.get('profileKey'); @@ -859,28 +876,33 @@ export class ConversationController { } } - log.warn( - 'combineConversations: Delete all sessions tied to old conversationId' - ); - const ourUuid = window.textsecure.storage.user.getCheckedUuid(); - const deviceIds = await window.textsecure.storage.protocol.getDeviceIds( - { - ourUuid, - identifier: obsoleteUuid.toString(), - } - ); + log.warn(`${logId}: Delete all sessions tied to old conversationId`); + const ourACI = window.textsecure.storage.user.getUuid(UUIDKind.ACI); + const ourPNI = window.textsecure.storage.user.getUuid(UUIDKind.PNI); await Promise.all( - deviceIds.map(async deviceId => { - const addr = new QualifiedAddress( - ourUuid, - new Address(obsoleteUuid, deviceId) + [ourACI, ourPNI].map(async ourUuid => { + if (!ourUuid) { + return; + } + const deviceIds = + await window.textsecure.storage.protocol.getDeviceIds({ + ourUuid, + identifier: obsoleteUuid.toString(), + }); + await Promise.all( + deviceIds.map(async deviceId => { + const addr = new QualifiedAddress( + ourUuid, + new Address(obsoleteUuid, deviceId) + ); + await window.textsecure.storage.protocol.removeSession(addr); + }) ); - await window.textsecure.storage.protocol.removeSession(addr); }) ); log.warn( - 'combineConversations: Delete all identity information tied to old conversationId' + `${logId}: Delete all identity information tied to old conversationId` ); if (obsoleteUuid) { @@ -890,7 +912,7 @@ export class ConversationController { } log.warn( - 'combineConversations: Ensure that all V1 groups have new conversationId instead of old' + `${logId}: Ensure that all V1 groups have new conversationId instead of old` ); const groups = await this.getAllGroupsInvolvingUuid(obsoleteUuid); groups.forEach(group => { @@ -907,24 +929,34 @@ export class ConversationController { // Note: we explicitly don't want to update V2 groups - log.warn( - 'combineConversations: Delete the obsolete conversation from the database' - ); + log.warn(`${logId}: Delete the obsolete conversation from the database`); await removeConversation(obsoleteId); - log.warn('combineConversations: Update messages table'); + log.warn(`${logId}: Update cached messages in MessageController`); + window.MessageController.update((message: MessageModel) => { + if (message.get('conversationId') === obsoleteId) { + message.set({ conversationId: currentId }); + } + }); + + log.warn(`${logId}: Update messages table`); await migrateConversationMessages(obsoleteId, currentId); log.warn( - 'combineConversations: Eliminate old conversation from ConversationController lookups' + `${logId}: Emit refreshConversation event to close old/open new` + ); + window.Whisper.events.trigger('refreshConversation', { + newId: currentId, + oldId: obsoleteId, + }); + + log.warn( + `${logId}: Eliminate old conversation from ConversationController lookups` ); this._conversations.remove(obsolete); this._conversations.resetLookups(); - log.warn('combineConversations: Complete!', { - obsolete: obsoleteId, - current: currentId, - }); + log.warn(`${logId}: Complete!`); }); } diff --git a/ts/test-electron/ConversationController_test.ts b/ts/test-electron/ConversationController_test.ts index 49f22d45ee40..56818b2ae859 100644 --- a/ts/test-electron/ConversationController_test.ts +++ b/ts/test-electron/ConversationController_test.ts @@ -43,25 +43,13 @@ describe('ConversationController', () => { }; }); - // Verifying incoming data - describe('data validation', () => { - it('throws when provided no data', () => { - assert.throws(() => { - window.ConversationController.maybeMergeContacts({ - mergeOldAndNew, - reason, - }); - }, 'Need to provide at least one'); - }); - it('throws when provided a pni with no e164', () => { - assert.throws(() => { - window.ConversationController.maybeMergeContacts({ - mergeOldAndNew, - pni: PNI_1, - reason, - }); - }, 'Cannot provide pni without an e164'); - }); + it('throws when provided no data', () => { + assert.throws(() => { + window.ConversationController.maybeMergeContacts({ + mergeOldAndNew, + reason, + }); + }, 'Need to provide at least one'); }); function create( @@ -300,6 +288,32 @@ describe('ConversationController', () => { assert.strictEqual(initial?.id, result?.id, 'result and initial match'); }); + it('adds ACI (via ACI+PNI) to conversation with e164+PNI', () => { + const initial = create('initial', { + uuid: PNI_1, + e164: E164_1, + }); + + expectPropsAndLookups(initial, 'initial', { + uuid: PNI_1, + e164: E164_1, + }); + + const result = window.ConversationController.maybeMergeContacts({ + mergeOldAndNew, + aci: ACI_1, + pni: PNI_1, + reason, + }); + expectPropsAndLookups(result, 'result', { + uuid: ACI_1, + e164: E164_1, + pni: PNI_1, + }); + + assert.strictEqual(initial?.id, result?.id, 'result and initial match'); + }); + it('adds e164+PNI to conversation with just ACI', () => { const initial = create('initial', { uuid: ACI_1, diff --git a/ts/util/MessageController.ts b/ts/util/MessageController.ts index e67e99f39339..bf41d454aa07 100644 --- a/ts/util/MessageController.ts +++ b/ts/util/MessageController.ts @@ -3,6 +3,7 @@ import type { MessageModel } from '../models/messages'; import * as durations from './durations'; +import * as log from '../logging/log'; import { map, filter } from './iterables'; import { isNotNil } from './isNotNil'; import type { MessageAttributesType } from '../model-types.d'; @@ -121,6 +122,14 @@ export class MessageController { return this.getById(id); } + update(predicate: (message: MessageModel) => void): void { + const values = Object.values(this.messageLookup); + log.info( + `MessageController.update: About to process ${values.length} messages` + ); + values.forEach(({ message }) => predicate(message)); + } + _get(): LookupType { return this.messageLookup; }