From 9ce0746f5bcca45dbc19ae44d034dc8afe7fa066 Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Wed, 19 Apr 2023 09:13:48 -0700 Subject: [PATCH] Fix flakey mock test --- ts/SignalProtocolStore.ts | 317 ++++++++++++--------- ts/models/conversations.ts | 4 +- ts/services/profiles.ts | 1 + ts/test-mock/pnp/accept_gv2_invite_test.ts | 2 +- 4 files changed, 185 insertions(+), 139 deletions(-) diff --git a/ts/SignalProtocolStore.ts b/ts/SignalProtocolStore.ts index 2b4130a68b..3fde993d45 100644 --- a/ts/SignalProtocolStore.ts +++ b/ts/SignalProtocolStore.ts @@ -241,6 +241,8 @@ export class SignalProtocolStore extends EventEmitter { sessionQueues = new Map(); + private readonly identityQueues = new Map(); + private currentZone?: Zone; private currentZoneDepth = 0; @@ -730,6 +732,27 @@ export class SignalProtocolStore extends EventEmitter { return freshQueue; } + // Identity Queue + + private _createIdentityQueue(): PQueue { + return new PQueue({ + concurrency: 1, + timeout: MINUTE * 30, + throwOnTimeout: true, + }); + } + + private _getIdentityQueue(uuid: UUID): PQueue { + const cachedQueue = this.identityQueues.get(uuid.toString()); + if (cachedQueue) { + return cachedQueue; + } + + const freshQueue = this._createIdentityQueue(); + this.identityQueues.set(uuid.toString(), freshQueue); + return freshQueue; + } + // Sessions // Re-entrant session transaction routine. Only one session transaction could @@ -1686,94 +1709,97 @@ export class SignalProtocolStore extends EventEmitter { nonblockingApproval = false; } - const identityRecord = await this.getOrMigrateIdentityRecord( - encodedAddress.uuid - ); - - const id = encodedAddress.uuid.toString(); - - if (!identityRecord || !identityRecord.publicKey) { - // Lookup failed, or the current key was removed, so save this one. - log.info('saveIdentity: Saving new identity...'); - await this._saveIdentityKey({ - id, - publicKey, - firstUse: true, - timestamp: Date.now(), - verified: VerifiedStatus.DEFAULT, - nonblockingApproval, - }); - - this.checkPreviousKey(encodedAddress.uuid, publicKey, 'saveIdentity'); - - return false; - } - - const identityKeyChanged = !constantTimeEqual( - identityRecord.publicKey, - publicKey - ); - - if (identityKeyChanged) { - const isOurIdentifier = window.textsecure.storage.user.isOurUuid( + return this._getIdentityQueue(encodedAddress.uuid).add(async () => { + const identityRecord = await this.getOrMigrateIdentityRecord( encodedAddress.uuid ); - if (isOurIdentifier && identityKeyChanged) { - log.warn('saveIdentity: ignoring identity for ourselves'); + const id = encodedAddress.uuid.toString(); + const logId = `saveIdentity(${id})`; + + if (!identityRecord || !identityRecord.publicKey) { + // Lookup failed, or the current key was removed, so save this one. + log.info(`${logId}: Saving new identity...`); + await this._saveIdentityKey({ + id, + publicKey, + firstUse: true, + timestamp: Date.now(), + verified: VerifiedStatus.DEFAULT, + nonblockingApproval, + }); + + this.checkPreviousKey(encodedAddress.uuid, publicKey, 'saveIdentity'); + return false; } - log.info('saveIdentity: Replacing existing identity...'); - const previousStatus = identityRecord.verified; - let verifiedStatus; - if ( - previousStatus === VerifiedStatus.VERIFIED || - previousStatus === VerifiedStatus.UNVERIFIED - ) { - verifiedStatus = VerifiedStatus.UNVERIFIED; - } else { - verifiedStatus = VerifiedStatus.DEFAULT; - } + const identityKeyChanged = !constantTimeEqual( + identityRecord.publicKey, + publicKey + ); - await this._saveIdentityKey({ - id, - publicKey, - firstUse: false, - timestamp: Date.now(), - verified: verifiedStatus, - nonblockingApproval, - }); - - // See `addKeyChange` in `ts/models/conversations.ts` for sender key info - // update caused by this. - try { - this.emit('keychange', encodedAddress.uuid, 'saveIdentity - change'); - } catch (error) { - log.error( - 'saveIdentity: error triggering keychange:', - Errors.toLogFormat(error) + if (identityKeyChanged) { + const isOurIdentifier = window.textsecure.storage.user.isOurUuid( + encodedAddress.uuid ); + + if (isOurIdentifier && identityKeyChanged) { + log.warn(`${logId}: ignoring identity for ourselves`); + return false; + } + + log.info(`${logId}: Replacing existing identity...`); + const previousStatus = identityRecord.verified; + let verifiedStatus; + if ( + previousStatus === VerifiedStatus.VERIFIED || + previousStatus === VerifiedStatus.UNVERIFIED + ) { + verifiedStatus = VerifiedStatus.UNVERIFIED; + } else { + verifiedStatus = VerifiedStatus.DEFAULT; + } + + await this._saveIdentityKey({ + id, + publicKey, + firstUse: false, + timestamp: Date.now(), + verified: verifiedStatus, + nonblockingApproval, + }); + + // See `addKeyChange` in `ts/models/conversations.ts` for sender key info + // update caused by this. + try { + this.emit('keychange', encodedAddress.uuid, 'saveIdentity - change'); + } catch (error) { + log.error( + `${logId}: error triggering keychange:`, + Errors.toLogFormat(error) + ); + } + + // Pass the zone to facilitate transactional session use in + // MessageReceiver.ts + await this.archiveSiblingSessions(encodedAddress, { + zone, + }); + + return true; } + if (this.isNonBlockingApprovalRequired(identityRecord)) { + log.info(`${logId}: Setting approval status...`); - // Pass the zone to facilitate transactional session use in - // MessageReceiver.ts - await this.archiveSiblingSessions(encodedAddress, { - zone, - }); + identityRecord.nonblockingApproval = nonblockingApproval; + await this._saveIdentityKey(identityRecord); - return true; - } - if (this.isNonBlockingApprovalRequired(identityRecord)) { - log.info('saveIdentity: Setting approval status...'); - - identityRecord.nonblockingApproval = nonblockingApproval; - await this._saveIdentityKey(identityRecord); + return false; + } return false; - } - - return false; + }); } // https://github.com/signalapp/Signal-Android/blob/fc3db538bcaa38dc149712a483d3032c9c1f3998/app/src/main/java/org/thoughtcrime/securesms/crypto/storage/SignalBaseIdentityKeyStore.java#L257 @@ -1790,6 +1816,15 @@ export class SignalProtocolStore extends EventEmitter { async saveIdentityWithAttributes( uuid: UUID, attributes: Partial + ): Promise { + return this._getIdentityQueue(uuid).add(async () => { + return this.saveIdentityWithAttributesOnQueue(uuid, attributes); + }); + } + + private async saveIdentityWithAttributesOnQueue( + uuid: UUID, + attributes: Partial ): Promise { if (uuid == null) { throw new Error('saveIdentityWithAttributes: uuid was undefined/null'); @@ -1823,14 +1858,16 @@ export class SignalProtocolStore extends EventEmitter { throw new Error('setApproval: Invalid approval status'); } - const identityRecord = await this.getOrMigrateIdentityRecord(uuid); + return this._getIdentityQueue(uuid).add(async () => { + const identityRecord = await this.getOrMigrateIdentityRecord(uuid); - if (!identityRecord) { - throw new Error(`setApproval: No identity record for ${uuid}`); - } + if (!identityRecord) { + throw new Error(`setApproval: No identity record for ${uuid}`); + } - identityRecord.nonblockingApproval = nonblockingApproval; - await this._saveIdentityKey(identityRecord); + identityRecord.nonblockingApproval = nonblockingApproval; + await this._saveIdentityKey(identityRecord); + }); } // https://github.com/signalapp/Signal-Android/blob/fc3db538bcaa38dc149712a483d3032c9c1f3998/app/src/main/java/org/thoughtcrime/securesms/crypto/storage/SignalBaseIdentityKeyStore.java#L215 @@ -1847,19 +1884,23 @@ export class SignalProtocolStore extends EventEmitter { throw new Error('setVerified: Invalid verified status'); } - const identityRecord = await this.getOrMigrateIdentityRecord(uuid); + return this._getIdentityQueue(uuid).add(async () => { + const identityRecord = await this.getOrMigrateIdentityRecord(uuid); - if (!identityRecord) { - throw new Error(`setVerified: No identity record for ${uuid.toString()}`); - } + if (!identityRecord) { + throw new Error( + `setVerified: No identity record for ${uuid.toString()}` + ); + } - if (validateIdentityKey(identityRecord)) { - await this._saveIdentityKey({ - ...identityRecord, - ...extra, - verified: verifiedStatus, - }); - } + if (validateIdentityKey(identityRecord)) { + await this._saveIdentityKey({ + ...identityRecord, + ...extra, + verified: verifiedStatus, + }); + } + }); } async getVerified(uuid: UUID): Promise { @@ -1922,54 +1963,56 @@ export class SignalProtocolStore extends EventEmitter { `Invalid verified status: ${verifiedStatus}` ); - const identityRecord = await this.getOrMigrateIdentityRecord(uuid); - const hadEntry = identityRecord !== undefined; - const keyMatches = Boolean( - identityRecord?.publicKey && - constantTimeEqual(publicKey, identityRecord.publicKey) - ); - const statusMatches = - keyMatches && verifiedStatus === identityRecord?.verified; + return this._getIdentityQueue(uuid).add(async () => { + const identityRecord = await this.getOrMigrateIdentityRecord(uuid); + const hadEntry = identityRecord !== undefined; + const keyMatches = Boolean( + identityRecord?.publicKey && + constantTimeEqual(publicKey, identityRecord.publicKey) + ); + const statusMatches = + keyMatches && verifiedStatus === identityRecord?.verified; - if (!keyMatches || !statusMatches) { - await this.saveIdentityWithAttributes(uuid, { - publicKey, - verified: verifiedStatus, - firstUse: !hadEntry, - timestamp: Date.now(), - nonblockingApproval: true, - }); - } - if (!hadEntry) { - this.checkPreviousKey(uuid, publicKey, 'updateIdentityAfterSync'); - } else if (hadEntry && !keyMatches) { - try { - this.emit('keychange', uuid, 'updateIdentityAfterSync - change'); - } catch (error) { - log.error( - 'updateIdentityAfterSync: error triggering keychange:', - Errors.toLogFormat(error) - ); + if (!keyMatches || !statusMatches) { + await this.saveIdentityWithAttributesOnQueue(uuid, { + publicKey, + verified: verifiedStatus, + firstUse: !hadEntry, + timestamp: Date.now(), + nonblockingApproval: true, + }); + } + if (!hadEntry) { + this.checkPreviousKey(uuid, publicKey, 'updateIdentityAfterSync'); + } else if (hadEntry && !keyMatches) { + try { + this.emit('keychange', uuid, 'updateIdentityAfterSync - change'); + } catch (error) { + log.error( + 'updateIdentityAfterSync: error triggering keychange:', + Errors.toLogFormat(error) + ); + } } - } - // See: https://github.com/signalapp/Signal-Android/blob/fc3db538bcaa38dc149712a483d3032c9c1f3998/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt#L921-L936 - if ( - verifiedStatus === VerifiedStatus.VERIFIED && - (!hadEntry || identityRecord?.verified !== VerifiedStatus.VERIFIED) - ) { - // Needs a notification. - return true; - } - if ( - verifiedStatus !== VerifiedStatus.VERIFIED && - hadEntry && - identityRecord?.verified === VerifiedStatus.VERIFIED - ) { - // Needs a notification. - return true; - } - return false; + // See: https://github.com/signalapp/Signal-Android/blob/fc3db538bcaa38dc149712a483d3032c9c1f3998/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt#L921-L936 + if ( + verifiedStatus === VerifiedStatus.VERIFIED && + (!hadEntry || identityRecord?.verified !== VerifiedStatus.VERIFIED) + ) { + // Needs a notification. + return true; + } + if ( + verifiedStatus !== VerifiedStatus.VERIFIED && + hadEntry && + identityRecord?.verified === VerifiedStatus.VERIFIED + ) { + // Needs a notification. + return true; + } + return false; + }); } isUntrusted(uuid: UUID, timestampThreshold = TIMESTAMP_THRESHOLD): boolean { diff --git a/ts/models/conversations.ts b/ts/models/conversations.ts index b0ae97044b..efc5ec9a75 100644 --- a/ts/models/conversations.ts +++ b/ts/models/conversations.ts @@ -1032,7 +1032,9 @@ export class ConversationModel extends window.Backbone } if (this.get('removalStage') === undefined) { - log.warn(`${logId}: not removed`); + if (!viaStorageServiceSync) { + log.warn(`${logId}: not removed`); + } return; } diff --git a/ts/services/profiles.ts b/ts/services/profiles.ts index d5be677a0b..2b69c0236e 100644 --- a/ts/services/profiles.ts +++ b/ts/services/profiles.ts @@ -584,6 +584,7 @@ export async function updateIdentityKey( false ); if (changed) { + log.info(`updateIdentityKey(${uuid.toString()}): changed`); // save identity will close all sessions except for .1, so we // must close that one manually. const ourUuid = window.textsecure.storage.user.getCheckedUuid(); diff --git a/ts/test-mock/pnp/accept_gv2_invite_test.ts b/ts/test-mock/pnp/accept_gv2_invite_test.ts index cfec47dbbd..cc9c0601d3 100644 --- a/ts/test-mock/pnp/accept_gv2_invite_test.ts +++ b/ts/test-mock/pnp/accept_gv2_invite_test.ts @@ -192,7 +192,7 @@ describe('pnp/accept gv2 invite', function needsName() { debug('Checking final notification'); await window .locator( - 'text=You accepted an invitation to the group from ' + + '.SystemMessage >> text=You accepted an invitation to the group from ' + `${second.profileName}.` ) .waitFor();