diff --git a/ts/services/storage.ts b/ts/services/storage.ts index 3f3d790e9..8e9fb5d25 100644 --- a/ts/services/storage.ts +++ b/ts/services/storage.ts @@ -41,6 +41,11 @@ import { SignalService as Proto } from '../protobuf'; import * as log from '../logging/log'; import { singleProtoJobQueue } from '../jobs/singleProtoJobQueue'; import * as Errors from '../types/errors'; +import type { + ExtendedStorageID, + RemoteRecord, + UnknownRecord, +} from '../types/StorageService.d'; type IManifestRecordIdentifier = Proto.ManifestRecord.IIdentifier; @@ -80,23 +85,10 @@ function redactStorageID( return `${version ?? '?'}:${storageID.substring(0, 3)}${convoId}`; } -type RemoteRecord = { - itemType: number; - storageID: string; - storageVersion?: number; -}; - -type UnknownRecord = RemoteRecord; - -type ProcessRemoteRecordsResultType = Readonly<{ - conflictCount: number; - deleteKeys: Array; -}>; - -function unknownRecordToRedactedID({ +function redactExtendedStorageID({ storageID, storageVersion, -}: UnknownRecord): string { +}: ExtendedStorageID): string { return redactStorageID(storageID, storageVersion); } @@ -148,12 +140,11 @@ type GeneratedManifestType = { async function generateManifest( version: number, previousManifest?: Proto.IManifestRecord, - isNewManifest = false, - extraDeleteKeys = new Array() + isNewManifest = false ): Promise { log.info( `storageService.upload(${version}): generating manifest ` + - `new=${isNewManifest} extraDeleteKeys=${extraDeleteKeys.length}` + `new=${isNewManifest}` ); await window.ConversationController.checkForConflicts(); @@ -166,10 +157,6 @@ async function generateManifest( const manifestRecordKeys: Set = new Set(); const newItems: Set = new Set(); - for (const key of extraDeleteKeys) { - deleteKeys.push(Bytes.fromBase64(key)); - } - const conversations = window.getConversations(); for (let i = 0; i < conversations.length; i += 1) { const conversation = conversations.models[i]; @@ -300,7 +287,7 @@ async function generateManifest( window.storage.get('storage-service-unknown-records') || [] ).filter((record: UnknownRecord) => !validRecordTypes.has(record.itemType)); - const redactedUnknowns = unknownRecordsArray.map(unknownRecordToRedactedID); + const redactedUnknowns = unknownRecordsArray.map(redactExtendedStorageID); log.info( `storageService.upload(${version}): adding unknown ` + @@ -322,7 +309,7 @@ async function generateManifest( 'storage-service-error-records', new Array() ); - const redactedErrors = recordsWithErrors.map(unknownRecordToRedactedID); + const redactedErrors = recordsWithErrors.map(redactExtendedStorageID); log.info( `storageService.upload(${version}): adding error ` + @@ -339,6 +326,24 @@ async function generateManifest( manifestRecordKeys.add(identifier); }); + // Delete keys that we wanted to drop during the processing of the manifest. + const storedPendingDeletes = window.storage.get( + 'storage-service-pending-deletes', + [] + ); + const redactedPendingDeletes = storedPendingDeletes.map( + redactExtendedStorageID + ); + log.info( + `storageService.upload(${version}): ` + + `deleting extra keys=${JSON.stringify(redactedPendingDeletes)} ` + + `count=${redactedPendingDeletes.length}` + ); + + for (const { storageID } of storedPendingDeletes) { + deleteKeys.push(Bytes.fromBase64(storageID)); + } + // Validate before writing const rawDuplicates = new Set(); @@ -827,7 +832,7 @@ async function mergeRecord( async function processManifest( manifest: Proto.IManifestRecord, version: number -): Promise { +): Promise { if (!window.textsecure.messaging) { throw new Error('storageService.processManifest: We are offline!'); } @@ -907,12 +912,8 @@ async function processManifest( }); let conflictCount = 0; - let deleteKeys = new Array(); if (remoteOnlyRecords.size) { - ({ conflictCount, deleteKeys } = await processRemoteRecords( - version, - remoteOnlyRecords - )); + conflictCount = await processRemoteRecords(version, remoteOnlyRecords); } // Post-merge, if our local records contain any storage IDs that were not @@ -940,17 +941,16 @@ async function processManifest( }); log.info( - `storageService.process(${version}): conflictCount=${conflictCount} ` + - `deleteKeys=${deleteKeys.length}` + `storageService.process(${version}): conflictCount=${conflictCount}` ); - return { conflictCount, deleteKeys }; + return conflictCount; } async function processRemoteRecords( storageVersion: number, remoteOnlyRecords: Map -): Promise { +): Promise { const storageKeyBase64 = window.storage.get('storageKey'); if (!storageKeyBase64) { throw new Error('No storage key'); @@ -1102,7 +1102,7 @@ async function processRemoteRecords( }); } - if (mergedRecord.hasConflict || mergedRecord.shouldDrop) { + if (mergedRecord.hasConflict) { conflictCount += 1; } @@ -1116,7 +1116,7 @@ async function processRemoteRecords( ); log.info( `storageService.process(${storageVersion}): ` + - `droppedKeys=${JSON.stringify(redactedDroppedKeys)} ` + + `dropped keys=${JSON.stringify(redactedDroppedKeys)} ` + `count=${redactedDroppedKeys.length}` ); @@ -1124,9 +1124,7 @@ async function processRemoteRecords( const newUnknownRecords = Array.from(unknownRecords.values()).filter( (record: UnknownRecord) => !validRecordTypes.has(record.itemType) ); - const redactedNewUnknowns = newUnknownRecords.map( - unknownRecordToRedactedID - ); + const redactedNewUnknowns = newUnknownRecords.map(redactExtendedStorageID); log.info( `storageService.process(${storageVersion}): ` + @@ -1139,7 +1137,7 @@ async function processRemoteRecords( ); const redactedErrorRecords = newRecordsWithErrors.map( - unknownRecordToRedactedID + redactExtendedStorageID ); log.info( `storageService.process(${storageVersion}): ` + @@ -1154,14 +1152,25 @@ async function processRemoteRecords( newRecordsWithErrors ); + // Store/overwrite keys pending deletion, but use them only when we have to + // upload a new manifest to avoid oscillation. + const pendingDeletes = [...missingKeys, ...droppedKeys].map(storageID => ({ + storageID, + storageVersion, + })); + const redactedPendingDeletes = pendingDeletes.map(redactExtendedStorageID); + log.info( + `storageService.process(${storageVersion}): ` + + `pending deletes=${JSON.stringify(redactedPendingDeletes)} ` + + `count=${redactedPendingDeletes.length}` + ); + await window.storage.put('storage-service-pending-deletes', pendingDeletes); + if (conflictCount === 0) { conflictBackOff.reset(); } - return { - conflictCount, - deleteKeys: [...missingKeys, ...droppedKeys], - }; + return conflictCount; } catch (err) { log.error( `storageService.process(${storageVersion}): ` + @@ -1170,7 +1179,8 @@ async function processRemoteRecords( ); } - return { conflictCount: 0, deleteKeys: [] }; + // conflictCount + return 0; } async function sync( @@ -1218,21 +1228,18 @@ async function sync( `version=${localManifestVersion}` ); - const { conflictCount, deleteKeys } = await processManifest( - manifest, - version - ); + const conflictCount = await processManifest(manifest, version); log.info( `storageService.sync: updated to version=${version} ` + - `conflicts=${conflictCount} deleteKeys=${deleteKeys.length}` + `conflicts=${conflictCount}` ); await window.storage.put('manifestVersion', version); - const hasConflicts = conflictCount !== 0 || deleteKeys.length !== 0; + const hasConflicts = conflictCount !== 0; if (hasConflicts && !ignoreConflicts) { - await upload(true, deleteKeys); + await upload(true); } // We now know that we've successfully completed a storage service fetch @@ -1249,10 +1256,7 @@ async function sync( return manifest; } -async function upload( - fromSync = false, - extraDeleteKeys = new Array() -): Promise { +async function upload(fromSync = false): Promise { if (!window.textsecure.messaging) { throw new Error('storageService.upload: We are offline!'); } @@ -1322,8 +1326,7 @@ async function upload( const generatedManifest = await generateManifest( version, previousManifest, - false, - extraDeleteKeys + false ); await uploadManifest(version, generatedManifest); } catch (err) { diff --git a/ts/services/storageRecordOps.ts b/ts/services/storageRecordOps.ts index 4317fb7c7..20610cdfb 100644 --- a/ts/services/storageRecordOps.ts +++ b/ts/services/storageRecordOps.ts @@ -558,7 +558,9 @@ export async function mergeGroupV1Record( details.push('GV1 record for GV2 group, dropping'); return { - hasConflict: true, + // Note: conflicts cause immediate uploads, but we should upload + // only in response to user's action. + hasConflict: false, shouldDrop: true, conversation, oldStorageID, @@ -771,11 +773,11 @@ export async function mergeContactRecord( // All contacts must have UUID if (!uuid) { - return { hasConflict: false, details: ['no uuid'] }; + return { hasConflict: false, shouldDrop: true, details: ['no uuid'] }; } if (!isValidUuid(uuid)) { - return { hasConflict: false, details: ['invalid uuid'] }; + return { hasConflict: false, shouldDrop: true, details: ['invalid uuid'] }; } const id = window.ConversationController.ensureContactIds({ diff --git a/ts/test-both/util/normalizeUuid_test.ts b/ts/test-both/util/normalizeUuid_test.ts index af840ccac..833b0b003 100644 --- a/ts/test-both/util/normalizeUuid_test.ts +++ b/ts/test-both/util/normalizeUuid_test.ts @@ -3,19 +3,36 @@ import { assert } from 'chai'; import { v4 as generateUuid } from 'uuid'; +import * as sinon from 'sinon'; +import type { LoggerType } from '../../types/Logging'; import { normalizeUuid } from '../../util/normalizeUuid'; describe('normalizeUuid', () => { - it('converts uuid to lower case', () => { - const uuid = generateUuid(); - assert.strictEqual(normalizeUuid(uuid, 'context 1'), uuid); - assert.strictEqual(normalizeUuid(uuid.toUpperCase(), 'context 2'), uuid); + let warn: sinon.SinonStub; + let logger: Pick; + + beforeEach(() => { + warn = sinon.stub(); + logger = { warn }; }); - it("throws if passed a string that's not a UUID", () => { - assert.throws( - () => normalizeUuid('not-UUID-at-all', 'context 3'), + it('converts uuid to lower case', () => { + const uuid = generateUuid(); + assert.strictEqual(normalizeUuid(uuid, 'context 1', logger), uuid); + assert.strictEqual( + normalizeUuid(uuid.toUpperCase(), 'context 2', logger), + uuid + ); + + sinon.assert.notCalled(warn); + }); + + it("warns if passed a string that's not a UUID", () => { + normalizeUuid('not-UUID-at-all', 'context 3', logger); + sinon.assert.calledOnce(warn); + sinon.assert.calledWith( + warn, 'Normalizing invalid uuid: not-UUID-at-all to not-uuid-at-all in ' + 'context "context 3"' ); diff --git a/ts/types/Storage.d.ts b/ts/types/Storage.d.ts index 30927b3d4..ad4e9f1fc 100644 --- a/ts/types/Storage.d.ts +++ b/ts/types/Storage.d.ts @@ -11,7 +11,12 @@ import type { PhoneNumberDiscoverability } from '../util/phoneNumberDiscoverabil import type { PhoneNumberSharingMode } from '../util/phoneNumberSharingMode'; import type { RetryItemType } from '../util/retryPlaceholders'; import type { ConfigMapType as RemoteConfigType } from '../RemoteConfig'; -import { SystemTraySetting } from './SystemTraySetting'; +import type { SystemTraySetting } from './SystemTraySetting'; +import type { + ExtendedStorageID, + RemoteRecord, + UnknownRecord, +} from './StorageService'; import type { GroupCredentialType } from '../textsecure/WebAPI'; import type { @@ -105,14 +110,9 @@ export type StorageAccessType = { avatarUrl: string; manifestVersion: number; storageCredentials: StorageServiceCredentials; - 'storage-service-error-records': Array<{ - itemType: number; - storageID: string; - }>; - 'storage-service-unknown-records': Array<{ - itemType: number; - storageID: string; - }>; + 'storage-service-error-records': Array; + 'storage-service-unknown-records': Array; + 'storage-service-pending-deletes': Array; 'preferred-video-input-device': string; 'preferred-audio-input-device': AudioDevice; 'preferred-audio-output-device': AudioDevice; diff --git a/ts/types/StorageService.d.ts b/ts/types/StorageService.d.ts new file mode 100644 index 000000000..f794bcefc --- /dev/null +++ b/ts/types/StorageService.d.ts @@ -0,0 +1,13 @@ +// Copyright 2020-2022 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +export type ExtendedStorageID = { + storageID: string; + storageVersion?: number; +}; + +export type RemoteRecord = ExtendedStorageID & { + itemType: number; +}; + +export type UnknownRecord = RemoteRecord; diff --git a/ts/util/normalizeUuid.ts b/ts/util/normalizeUuid.ts index ca24ced58..a093a7139 100644 --- a/ts/util/normalizeUuid.ts +++ b/ts/util/normalizeUuid.ts @@ -3,15 +3,24 @@ import type { UUIDStringType } from '../types/UUID'; import { isValidUuid } from '../types/UUID'; -import { assert } from './assert'; +import type { LoggerType } from '../types/Logging'; +import * as log from '../logging/log'; -export function normalizeUuid(uuid: string, context: string): UUIDStringType { +export function normalizeUuid( + uuid: string, + context: string, + logger: Pick = log +): UUIDStringType { const result = uuid.toLowerCase(); - assert( - isValidUuid(uuid) && isValidUuid(result), - `Normalizing invalid uuid: ${uuid} to ${result} in context "${context}"` - ); + if (!isValidUuid(uuid) || !isValidUuid(result)) { + logger.warn( + `Normalizing invalid uuid: ${uuid} to ${result} in context "${context}"` + ); + + // Cast anyway we don't want to throw here + return result as unknown as UUIDStringType; + } return result; }