From 1264e6da2b78632b3eebddb7febcf065637f1c71 Mon Sep 17 00:00:00 2001 From: Josh Perez <60019601+josh-signal@users.noreply.github.com> Date: Tue, 30 Mar 2021 20:16:28 -0400 Subject: [PATCH] Retain protections on gv1 records that match gv2 ids --- ts/models/conversations.ts | 22 ++++++++--- ts/services/storage.ts | 70 +++------------------------------ ts/services/storageRecordOps.ts | 15 ++++++- 3 files changed, 36 insertions(+), 71 deletions(-) diff --git a/ts/models/conversations.ts b/ts/models/conversations.ts index 77bdc13d1..59c2cbd0d 100644 --- a/ts/models/conversations.ts +++ b/ts/models/conversations.ts @@ -3725,7 +3725,9 @@ export class ConversationModel extends window.Backbone.Model< if (Boolean(before) !== Boolean(after)) { if (after) { - this.unpin(); + // we're capturing a storage sync below so + // we don't need to capture it twice + this.unpin({ stopStorageSync: true }); } this.captureChange('isArchived'); } @@ -5105,6 +5107,10 @@ export class ConversationModel extends window.Backbone.Model< } pin(): void { + if (this.get('isPinned')) { + return; + } + window.log.info('pinning', this.idForLogging()); const pinnedConversationIds = new Set( window.storage.get>('pinnedConversationIds', []) @@ -5115,14 +5121,18 @@ export class ConversationModel extends window.Backbone.Model< this.writePinnedConversations([...pinnedConversationIds]); this.set('isPinned', true); - window.Signal.Data.updateConversation(this.attributes); if (this.get('isArchived')) { - this.setArchived(false); + this.set({ isArchived: false }); } + window.Signal.Data.updateConversation(this.attributes); } - unpin(): void { + unpin({ stopStorageSync = false } = {}): void { + if (!this.get('isPinned')) { + return; + } + window.log.info('un-pinning', this.idForLogging()); const pinnedConversationIds = new Set( @@ -5131,7 +5141,9 @@ export class ConversationModel extends window.Backbone.Model< pinnedConversationIds.delete(this.id); - this.writePinnedConversations([...pinnedConversationIds]); + if (!stopStorageSync) { + this.writePinnedConversations([...pinnedConversationIds]); + } this.set('isPinned', false); window.Signal.Data.updateConversation(this.attributes); diff --git a/ts/services/storage.ts b/ts/services/storage.ts index 5f089a7bc..e4cf9e5df 100644 --- a/ts/services/storage.ts +++ b/ts/services/storage.ts @@ -131,8 +131,6 @@ async function generateManifest( await window.ConversationController.checkForConflicts(); - await repairUnknownAndErroredRecords(); - const ITEM_TYPE = window.textsecure.protobuf.ManifestRecord.Identifier.Type; const conversationsToUpdate = []; @@ -351,54 +349,6 @@ async function generateManifest( }; } -async function repairUnknownAndErroredRecords() { - const unknownRecordsArray: ReadonlyArray = - window.storage.get('storage-service-unknown-records') || []; - - const recordsWithErrors: ReadonlyArray = - window.storage.get('storage-service-error-records') || []; - - const remoteRecords = unknownRecordsArray.concat(recordsWithErrors); - - // No repair necessary - if (remoteRecords.length === 0) { - return; - } - - // Process unknown and records with records from the past sync to see - // if they can be merged - const remoteRecordsMap: Map = new Map(); - remoteRecords.forEach(record => { - remoteRecordsMap.set(record.storageID, record); - }); - - window.log.info( - 'storageService.repairUnknownAndErroredRecords: found ' + - `${unknownRecordsArray.length} unknown records and ` + - `${recordsWithErrors.length} errored records, attempting repair` - ); - const conflictCount = await processRemoteRecords(remoteRecordsMap); - if (conflictCount !== 0) { - window.log.info( - 'storageService.repairUnknownAndErroredRecords: fixed ' + - `${conflictCount} conflicts` - ); - } - - const newUnknownCount = ( - window.storage.get('storage-service-unknown-records') || [] - ).length; - const newErroredCount = ( - window.storage.get('storage-service-error-records') || [] - ).length; - - window.log.info( - 'storageService.repairUnknownAndErroredRecords: ' + - `${newUnknownCount} unknown records and ` + - `${newErroredCount} errored records after repair` - ); -} - async function uploadManifest( version: number, { @@ -647,8 +597,9 @@ async function mergeRecord( } catch (err) { hasError = true; window.log.error( - 'storageService.mergeRecord: merging record failed', - err && err.stack ? err.stack : String(err) + 'storageService.mergeRecord: Error with', + storageID, + itemType ); } @@ -691,14 +642,6 @@ async function processManifest( } }); - const recordsWithErrors: ReadonlyArray = - window.storage.get('storage-service-error-records') || []; - - // Do not fetch any records that we failed to merge in the previous fetch - recordsWithErrors.forEach((record: UnknownRecord) => { - localKeys.push(record.storageID); - }); - window.log.info( 'storageService.processManifest: local keys:', localKeys.length @@ -708,10 +651,6 @@ async function processManifest( 'storageService.processManifest: incl. unknown records:', unknownRecordsArray.length ); - window.log.info( - 'storageService.processManifest: incl. records with errors:', - recordsWithErrors.length - ); const remoteKeys = Array.from(remoteKeysTypeMap.keys()); @@ -1062,6 +1001,9 @@ async function upload(fromSync = false): Promise { } if (!fromSync) { + // Syncing before we upload so that we repair any unknown records and + // records with errors as well as ensure that we have the latest up to date + // manifest. await sync(); } diff --git a/ts/services/storageRecordOps.ts b/ts/services/storageRecordOps.ts index 032627e97..e336bfef2 100644 --- a/ts/services/storageRecordOps.ts +++ b/ts/services/storageRecordOps.ts @@ -388,7 +388,20 @@ export async function mergeGroupV1Record( // Attempt to fetch an existing group pertaining to the `groupId` or create // a new group and populate it with the attributes from the record. let conversation = window.ConversationController.get(groupId); + + // Because ConversationController.get retrieves all types of records we + // may sometimes have a situation where we get a record of groupv1 type + // where the binary representation of its ID matches a v2 record in memory. + // Here we ensure that the record we're about to process is GV1 otherwise + // we drop the update. + if (conversation && !conversation.isGroupV1()) { + throw new Error(`Record has group type mismatch ${conversation.debugID()}`); + } + if (!conversation) { + // It's possible this group was migrated to a GV2 if so we attempt to + // retrieve the master key and find the conversation locally. If we + // are successful then we continue setting and applying state. const masterKeyBuffer = await deriveMasterKeyFromGroupV1(groupId); const fields = deriveGroupFields(masterKeyBuffer); const derivedGroupV2Id = arrayBufferToBase64(fields.id); @@ -415,8 +428,6 @@ export async function mergeGroupV1Record( ); } - // If we receive a group V1 record, remote data should take precendence - // even if the group is actually V2 on our end. conversation.set({ isArchived: Boolean(groupV1Record.archived), markedUnread: Boolean(groupV1Record.markedUnread),