From 9438b7b3fef6e6dec215ba88398cd61777daf386 Mon Sep 17 00:00:00 2001 From: Josh Perez <60019601+josh-signal@users.noreply.github.com> Date: Thu, 8 Apr 2021 12:27:20 -0700 Subject: [PATCH] Fixes pinned conversations sync --- ts/services/storage.ts | 93 ++++++++++---- ts/services/storageRecordOps.ts | 60 +++++++-- .../util/arePinnedConversationsEqual_test.ts | 120 ++++++++++++++++++ ts/util/arePinnedConversationsEqual.ts | 46 +++++++ ts/window.d.ts | 3 + 5 files changed, 281 insertions(+), 41 deletions(-) create mode 100644 ts/test-both/util/arePinnedConversationsEqual_test.ts create mode 100644 ts/util/arePinnedConversationsEqual.ts diff --git a/ts/services/storage.ts b/ts/services/storage.ts index c3cc0b756..4fdda71e8 100644 --- a/ts/services/storage.ts +++ b/ts/services/storage.ts @@ -1,7 +1,7 @@ // Copyright 2020-2021 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only -import { debounce, isNumber, partition } from 'lodash'; +import { debounce, isNumber } from 'lodash'; import pMap from 'p-map'; import Crypto from '../textsecure/Crypto'; @@ -654,46 +654,60 @@ async function processManifest( ); }); - const localKeys = window - .getConversations() - .map((conversation: ConversationModel) => conversation.get('storageID')) - .filter(Boolean); + const remoteKeys = new Set(remoteKeysTypeMap.keys()); + const localKeys: Set = new Set(); + + const conversations = window.getConversations(); + conversations.forEach((conversation: ConversationModel) => { + const storageID = conversation.get('storageID'); + if (storageID) { + localKeys.add(storageID); + } + }); const unknownRecordsArray: ReadonlyArray = window.storage.get('storage-service-unknown-records') || []; - unknownRecordsArray.forEach((record: UnknownRecord) => { + const stillUnknown = unknownRecordsArray.filter((record: UnknownRecord) => { // Do not include any unknown records that we already support if (!validRecordTypes.has(record.itemType)) { - localKeys.push(record.storageID); + localKeys.add(record.storageID); + return false; } + return true; }); window.log.info( - 'storageService.processManifest: local keys:', - localKeys.length + 'storageService.processManifest: local records:', + conversations.length ); - window.log.info( - 'storageService.processManifest: incl. unknown records:', - unknownRecordsArray.length + 'storageService.processManifest: local keys:', + localKeys.size + ); + window.log.info( + 'storageService.processManifest: unknown records:', + stillUnknown.length + ); + window.log.info( + 'storageService.processManifest: remote keys:', + remoteKeys.size ); - - const remoteKeys = Array.from(remoteKeysTypeMap.keys()); const remoteOnlySet: Set = new Set(); remoteKeys.forEach((key: string) => { - if (!localKeys.includes(key)) { + if (!localKeys.has(key)) { remoteOnlySet.add(key); } }); + window.log.info( + 'storageService.processManifest: remote ids:', + Array.from(remoteOnlySet).map(redactStorageID).join(',') + ); + const remoteOnlyRecords = new Map(); remoteOnlySet.forEach(storageID => { - window.log.info( - 'storageService.processManifest: remote key', - redactStorageID(storageID) - ); remoteOnlyRecords.set(storageID, { storageID, itemType: remoteKeysTypeMap.get(storageID), @@ -703,13 +717,36 @@ async function processManifest( // if the remote only keys are larger or equal to our local keys then it // was likely a forced push of storage service. We keep track of these // merges so that we can detect possible infinite loops - const isForcePushed = remoteOnlyRecords.size >= localKeys.length; + const isForcePushed = remoteOnlyRecords.size >= localKeys.size; const conflictCount = await processRemoteRecords( remoteOnlyRecords, isForcePushed ); - const hasConflicts = conflictCount !== 0; + + let hasConflicts = conflictCount !== 0; + + // Post-merge, if our local records contain any storage IDs that were not + // present in the remote manifest then we'll need to clear it, generate a + // new storageID for that record, and upload. + // This might happen if a device pushes a manifest which doesn't contain + // the keys that we have in our local database. + window.getConversations().forEach((conversation: ConversationModel) => { + const storageID = conversation.get('storageID'); + if (storageID && !remoteKeys.has(storageID)) { + window.log.info( + 'storageService.processManifest: local key was not in remote manifest', + redactStorageID(storageID), + conversation.idForLogging() + ); + conversation.set({ + needsStorageServiceSync: true, + storageID: undefined, + }); + updateConversation(conversation.attributes); + hasConflicts = true; + } + }); return hasConflicts; } @@ -722,7 +759,7 @@ async function processRemoteRecords( const storageKey = base64ToArrayBuffer(storageKeyBase64); window.log.info( - 'storageService.processRemoteRecords: remote keys', + 'storageService.processRemoteRecords: remote only keys', remoteOnlyRecords.size ); @@ -809,12 +846,12 @@ async function processRemoteRecords( { concurrency: 5 } ); - // Merge Account records last - const sortedStorageItems = ([] as Array).concat( - ...partition( - decryptedStorageItems, - storageRecord => storageRecord.storageRecord.account === undefined - ) + // Merge Account records last since it contains the pinned conversations + // and we need all other records merged first before we can find the pinned + // records in our db + const ITEM_TYPE = window.textsecure.protobuf.ManifestRecord.Identifier.Type; + const sortedStorageItems = decryptedStorageItems.sort((_, b) => + b.itemType === ITEM_TYPE.ACCOUNT ? -1 : 1 ); try { diff --git a/ts/services/storageRecordOps.ts b/ts/services/storageRecordOps.ts index ad1abccfc..1a5ff7398 100644 --- a/ts/services/storageRecordOps.ts +++ b/ts/services/storageRecordOps.ts @@ -32,8 +32,8 @@ import { PhoneNumberDiscoverability, parsePhoneNumberDiscoverability, } from '../util/phoneNumberDiscoverability'; +import { arePinnedConversationsEqual } from '../util/arePinnedConversationsEqual'; import { ConversationModel } from '../models/conversations'; -import { ConversationAttributesTypeType } from '../model-types.d'; const { updateConversation } = dataInterface; @@ -364,6 +364,48 @@ function doRecordsConflict( const localValue = localRecord[key]; const remoteValue = remoteRecord[key]; + // Sometimes we have a ByteBuffer and an ArrayBuffer, this ensures that we + // are comparing them both equally by converting them into base64 string. + if (Object.prototype.toString.call(localValue) === '[object ArrayBuffer]') { + const areEqual = + arrayBufferToBase64(localValue) === arrayBufferToBase64(remoteValue); + if (!areEqual) { + window.log.info( + 'storageService.doRecordsConflict: Conflict found for ArrayBuffer', + key, + idForLogging + ); + } + return hasConflict || !areEqual; + } + + // If both types are Long we can use Long's equals to compare them + if ( + window.dcodeIO.Long.isLong(localValue) && + window.dcodeIO.Long.isLong(remoteValue) + ) { + const areEqual = localValue.equals(remoteValue); + if (!areEqual) { + window.log.info( + 'storageService.doRecordsConflict: Conflict found for Long', + key, + idForLogging + ); + } + return hasConflict || !areEqual; + } + + if (key === 'pinnedConversations') { + const areEqual = arePinnedConversationsEqual(localValue, remoteValue); + if (!areEqual) { + window.log.info( + 'storageService.doRecordsConflict: Conflict found for pinnedConversations', + idForLogging + ); + } + return hasConflict || !areEqual; + } + if (localValue === remoteValue) { return hasConflict || false; } @@ -373,7 +415,10 @@ function doRecordsConflict( // conflicting. if ( remoteValue === null && - (localValue === false || localValue === '' || localValue === 0) + (localValue === false || + localValue === '' || + localValue === 0 || + (window.dcodeIO.Long.isLong(localValue) && localValue.toNumber() === 0)) ) { return hasConflict || false; } @@ -805,7 +850,6 @@ export async function mergeAccountRecord( const remotelyPinnedConversationPromises = pinnedConversations.map( async pinnedConversation => { let conversationId; - let conversationType: ConversationAttributesTypeType = 'private'; switch (pinnedConversation.identifier) { case 'contact': { @@ -815,7 +859,6 @@ export async function mergeAccountRecord( conversationId = window.ConversationController.ensureContactIds( pinnedConversation.contact ); - conversationType = 'private'; break; } case 'legacyGroupId': { @@ -823,7 +866,6 @@ export async function mergeAccountRecord( throw new Error('mergeAccountRecord: no legacyGroupId found'); } conversationId = pinnedConversation.legacyGroupId.toBinary(); - conversationType = 'group'; break; } case 'groupMasterKey': { @@ -835,7 +877,6 @@ export async function mergeAccountRecord( const groupId = arrayBufferToBase64(groupFields.id); conversationId = groupId; - conversationType = 'group'; break; } default: { @@ -853,13 +894,6 @@ export async function mergeAccountRecord( return undefined; } - if (conversationType === 'private') { - return window.ConversationController.getOrCreateAndWait( - conversationId, - conversationType - ); - } - return window.ConversationController.get(conversationId); } ); diff --git a/ts/test-both/util/arePinnedConversationsEqual_test.ts b/ts/test-both/util/arePinnedConversationsEqual_test.ts new file mode 100644 index 000000000..3be7ba60f --- /dev/null +++ b/ts/test-both/util/arePinnedConversationsEqual_test.ts @@ -0,0 +1,120 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { assert } from 'chai'; +import { arePinnedConversationsEqual } from '../../util/arePinnedConversationsEqual'; +import { PinnedConversationClass } from '../../textsecure.d'; + +describe('arePinnedConversationsEqual', () => { + it('is equal if both have same values at same indices', () => { + const localValue = [ + { + identifier: 'contact' as const, + contact: { + uuid: '72313cde-2784-4a6f-a92a-abbe23763a60', + e164: '+13055551234', + }, + toArrayBuffer: () => new ArrayBuffer(0), + }, + { + identifier: 'groupMasterKey' as const, + groupMasterKey: new ArrayBuffer(32), + toArrayBuffer: () => new ArrayBuffer(0), + }, + ]; + const remoteValue = [ + { + identifier: 'contact' as const, + contact: { + uuid: '72313cde-2784-4a6f-a92a-abbe23763a60', + e164: '+13055551234', + }, + toArrayBuffer: () => new ArrayBuffer(0), + }, + { + identifier: 'groupMasterKey' as const, + groupMasterKey: new ArrayBuffer(32), + toArrayBuffer: () => new ArrayBuffer(0), + }, + ]; + + assert.isTrue(arePinnedConversationsEqual(localValue, remoteValue)); + }); + + it('is not equal if values are mixed', () => { + const localValue = [ + { + identifier: 'contact' as const, + contact: { + uuid: '72313cde-2784-4a6f-a92a-abbe23763a60', + e164: '+13055551234', + }, + toArrayBuffer: () => new ArrayBuffer(0), + }, + { + identifier: 'contact' as const, + contact: { + uuid: 'f59a9fed-9e91-4bb4-a015-d49e58b47e25', + e164: '+17865554321', + }, + toArrayBuffer: () => new ArrayBuffer(0), + }, + ]; + const remoteValue = [ + { + identifier: 'contact' as const, + contact: { + uuid: 'f59a9fed-9e91-4bb4-a015-d49e58b47e25', + e164: '+17865554321', + }, + toArrayBuffer: () => new ArrayBuffer(0), + }, + { + identifier: 'contact' as const, + contact: { + uuid: '72313cde-2784-4a6f-a92a-abbe23763a60', + e164: '+13055551234', + }, + toArrayBuffer: () => new ArrayBuffer(0), + }, + ]; + + assert.isFalse(arePinnedConversationsEqual(localValue, remoteValue)); + }); + + it('is not equal if lengths are not same', () => { + const localValue = [ + { + identifier: 'contact' as const, + contact: { + uuid: '72313cde-2784-4a6f-a92a-abbe23763a60', + e164: '+13055551234', + }, + toArrayBuffer: () => new ArrayBuffer(0), + }, + ]; + const remoteValue: Array = []; + assert.isFalse(arePinnedConversationsEqual(localValue, remoteValue)); + }); + + it('is not equal if content does not match', () => { + const localValue = [ + { + identifier: 'contact' as const, + contact: { + uuid: '72313cde-2784-4a6f-a92a-abbe23763a60', + e164: '+13055551234', + }, + toArrayBuffer: () => new ArrayBuffer(0), + }, + ]; + const remoteValue = [ + { + identifier: 'groupMasterKey' as const, + groupMasterKey: new ArrayBuffer(32), + toArrayBuffer: () => new ArrayBuffer(0), + }, + ]; + assert.isFalse(arePinnedConversationsEqual(localValue, remoteValue)); + }); +}); diff --git a/ts/util/arePinnedConversationsEqual.ts b/ts/util/arePinnedConversationsEqual.ts new file mode 100644 index 000000000..253bf7ce0 --- /dev/null +++ b/ts/util/arePinnedConversationsEqual.ts @@ -0,0 +1,46 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { arrayBufferToBase64 } from '../Crypto'; +import { PinnedConversationClass } from '../textsecure.d'; + +export function arePinnedConversationsEqual( + localValue: Array, + remoteValue: Array +): boolean { + if (localValue.length !== remoteValue.length) { + return false; + } + return localValue.every( + (localPinnedConversation: PinnedConversationClass, index: number) => { + const remotePinnedConversation = remoteValue[index]; + if ( + localPinnedConversation.identifier !== + remotePinnedConversation.identifier + ) { + return false; + } + switch (localPinnedConversation.identifier) { + case 'contact': + return ( + localPinnedConversation.contact && + remotePinnedConversation.contact && + localPinnedConversation.contact.uuid === + remotePinnedConversation.contact.uuid + ); + case 'groupMasterKey': + return ( + arrayBufferToBase64(localPinnedConversation.groupMasterKey) === + arrayBufferToBase64(remotePinnedConversation.groupMasterKey) + ); + case 'legacyGroupId': + return ( + arrayBufferToBase64(localPinnedConversation.legacyGroupId) === + arrayBufferToBase64(remotePinnedConversation.legacyGroupId) + ); + default: + return false; + } + } + ); +} diff --git a/ts/window.d.ts b/ts/window.d.ts index 2641a71dd..eebfe55a0 100644 --- a/ts/window.d.ts +++ b/ts/window.d.ts @@ -572,8 +572,11 @@ export type DCodeIOType = { Long: DCodeIOType['Long']; }; Long: Long & { + equals: (other: Long | number | string) => boolean; fromBits: (low: number, high: number, unsigned: boolean) => number; + fromNumber: (value: number, unsigned?: boolean) => Long; fromString: (str: string | null) => Long; + isLong: (obj: unknown) => obj is Long; }; };