From bddd55d57415e4173caff3eaedea09d5aec65b8e Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Mon, 21 Mar 2022 14:19:37 -0700 Subject: [PATCH] Challenge: Save conversationIds and start queues --- app/main.ts | 17 +- ts/background.ts | 94 +++++---- ts/challenge.ts | 305 ++++++++++----------------- ts/components/CaptchaDialog.tsx | 2 + ts/jobs/conversationJobQueue.ts | 75 +++++-- ts/jobs/helpers/sendNormalMessage.ts | 17 +- ts/jobs/removeStorageKeyJobQueue.ts | 2 +- ts/models/messages.ts | 13 -- ts/test-both/challenge_test.ts | 254 +++++++--------------- ts/textsecure/Errors.ts | 4 +- ts/types/Storage.d.ts | 9 +- 11 files changed, 316 insertions(+), 476 deletions(-) diff --git a/app/main.ts b/app/main.ts index b8ecda4c8..2474c00ef 100644 --- a/app/main.ts +++ b/app/main.ts @@ -1225,7 +1225,7 @@ async function showDebugLogWindow() { let permissionsPopupWindow: BrowserWindow | undefined; function showPermissionsPopupWindow(forCalling: boolean, forCamera: boolean) { // eslint-disable-next-line no-async-promise-executor - return new Promise(async (resolve, reject) => { + return new Promise(async (resolveFn, reject) => { if (permissionsPopupWindow) { permissionsPopupWindow.show(); reject(new Error('Permission window already showing')); @@ -1276,7 +1276,7 @@ function showPermissionsPopupWindow(forCalling: boolean, forCamera: boolean) { removeDarkOverlay(); permissionsPopupWindow = undefined; - resolve(); + resolveFn(); }); permissionsPopupWindow.once('ready-to-show', () => { @@ -1501,7 +1501,9 @@ app.on('ready', async () => { // If the sql initialization takes more than three seconds to complete, we // want to notify the user that things are happening - const timeout = new Promise(resolve => setTimeout(resolve, 3000, 'timeout')); + const timeout = new Promise(resolveFn => + setTimeout(resolveFn, 3000, 'timeout') + ); // eslint-disable-next-line more/no-then Promise.race([sqlInitPromise, timeout]).then(maybeTimeout => { if (maybeTimeout !== 'timeout') { @@ -1691,11 +1693,11 @@ async function requestShutdown() { } getLogger().info('requestShutdown: Requesting close of mainWindow...'); - const request = new Promise((resolve, reject) => { + const request = new Promise((resolveFn, reject) => { let timeout: NodeJS.Timeout | undefined; if (!mainWindow) { - resolve(); + resolveFn(); return; } @@ -1707,7 +1709,7 @@ async function requestShutdown() { } clearTimeoutIfNecessary(timeout); - resolve(); + resolveFn(); }); mainWindow.webContents.send('get-ready-for-shutdown'); @@ -1720,7 +1722,7 @@ async function requestShutdown() { getLogger().error( 'requestShutdown: Response never received; forcing shutdown.' ); - resolve(); + resolveFn(); }, 2 * 60 * 1000); }); @@ -1792,6 +1794,7 @@ app.on( app.setAsDefaultProtocolClient('sgnl'); app.setAsDefaultProtocolClient('signalcaptcha'); + app.on('will-finish-launching', () => { // open-url must be set from within will-finish-launching for macOS // https://stackoverflow.com/a/43949291 diff --git a/ts/background.ts b/ts/background.ts index 39c965ed2..72e38b18f 100644 --- a/ts/background.ts +++ b/ts/background.ts @@ -47,7 +47,6 @@ import { isMoreRecentThan, isOlderThan, toDayMillis } from './util/timestamp'; import { isValidReactionEmoji } from './reactions/isValidReactionEmoji'; import type { ConversationModel } from './models/conversations'; import { getContact } from './messages/helpers'; -import { getMessageById } from './messages/getMessageById'; import { createBatcher } from './util/batcher'; import { updateConversationsWithUuidLookup } from './updateConversationsWithUuidLookup'; import { initializeAllJobQueues } from './jobs/initializeAllJobQueues'; @@ -139,6 +138,7 @@ import { updateOurUsername } from './util/updateOurUsername'; import { ReactionSource } from './reactions/ReactionSource'; import { singleProtoJobQueue } from './jobs/singleProtoJobQueue'; import { getInitialState } from './state/getInitialState'; +import { conversationJobQueue } from './jobs/conversationJobQueue'; const MAX_ATTACHMENT_DOWNLOAD_AGE = 3600 * 72 * 1000; @@ -194,16 +194,54 @@ export async function startApp(): Promise { // Initialize WebAPI as early as possible let server: WebAPIType | undefined; let messageReceiver: MessageReceiver | undefined; + let challengeHandler: ChallengeHandler | undefined; + window.storage.onready(() => { server = window.WebAPI.connect( window.textsecure.storage.user.getWebAPICredentials() ); window.textsecure.server = server; - initializeAllJobQueues({ - server, + challengeHandler = new ChallengeHandler({ + storage: window.storage, + + startQueue(conversationId: string) { + conversationJobQueue.resolveVerificationWaiter(conversationId); + }, + + requestChallenge(request) { + window.sendChallengeRequest(request); + }, + + async sendChallengeResponse(data) { + await window.textsecure.messaging.sendChallengeResponse(data); + }, + + onChallengeFailed() { + // TODO: DESKTOP-1530 + // Display humanized `retryAfter` + showToast(ToastCaptchaFailed); + }, + + onChallengeSolved() { + showToast(ToastCaptchaSolved); + }, + + setChallengeStatus(challengeStatus) { + window.reduxActions.network.setChallengeStatus(challengeStatus); + }, }); + window.Whisper.events.on('challengeResponse', response => { + if (!challengeHandler) { + throw new Error('Expected challenge handler to be there'); + } + + challengeHandler.onResponse(response); + }); + + window.Signal.challengeHandler = challengeHandler; + log.info('Initializing MessageReceiver'); messageReceiver = new MessageReceiver({ server, @@ -709,6 +747,11 @@ export async function startApp(): Promise { } if (window.isBeforeVersion(lastVersion, 'v5.37.0-alpha')) { + const legacyChallengeKey = 'challenge:retry-message-ids'; + await removeStorageKeyJobQueue.add({ + key: legacyChallengeKey, + }); + await window.Signal.Data.clearAllErrorStickerPackAttempts(); } @@ -1569,49 +1612,16 @@ export async function startApp(): Promise { } } - let challengeHandler: ChallengeHandler | undefined; - async function start() { - challengeHandler = new ChallengeHandler({ - storage: window.storage, - - getMessageById, - - requestChallenge(request) { - window.sendChallengeRequest(request); - }, - - async sendChallengeResponse(data) { - await window.textsecure.messaging.sendChallengeResponse(data); - }, - - onChallengeFailed() { - // TODO: DESKTOP-1530 - // Display humanized `retryAfter` - showToast(ToastCaptchaFailed); - }, - - onChallengeSolved() { - showToast(ToastCaptchaSolved); - }, - - setChallengeStatus(challengeStatus) { - window.reduxActions.network.setChallengeStatus(challengeStatus); - }, - }); - - window.Whisper.events.on('challengeResponse', response => { - if (!challengeHandler) { - throw new Error('Expected challenge handler to be there'); - } - - challengeHandler.onResponse(response); - }); - // Storage is ready because `start()` is called from `storage.onready()` + + strictAssert(challengeHandler, 'start: challengeHandler'); await challengeHandler.load(); - window.Signal.challengeHandler = challengeHandler; + strictAssert(server, 'start: server'); + initializeAllJobQueues({ + server, + }); if (!window.storage.user.getNumber()) { const ourConversation = diff --git a/ts/challenge.ts b/ts/challenge.ts index c0c521b2f..9232853ad 100644 --- a/ts/challenge.ts +++ b/ts/challenge.ts @@ -12,15 +12,14 @@ // are not immediately retried, however, until `.onOnline()` is called from // when we are actually online. -import type { MessageModel } from './models/messages'; import { assert } from './util/assert'; -import { isNotNil } from './util/isNotNil'; import { isOlderThan } from './util/timestamp'; import { parseRetryAfter } from './util/parseRetryAfter'; import { clearTimeoutIfNecessary } from './util/clearTimeoutIfNecessary'; import { getEnvironment, Environment } from './environment'; import type { StorageInterface } from './types/Storage.d'; import { HTTPError } from './textsecure/Errors'; +import type { SendMessageChallengeData } from './textsecure/Errors'; import * as log from './logging/log'; export type ChallengeResponse = { @@ -36,11 +35,6 @@ export type IPCResponse = { readonly data: ChallengeResponse; }; -export enum RetryMode { - Retry = 'Retry', - NoImmediateRetry = 'NoImmediateRetry', -} - type Handler = { readonly token: string | undefined; @@ -54,22 +48,12 @@ export type ChallengeData = { readonly captcha: string; }; -export type MinimalMessage = Pick< - MessageModel, - 'id' | 'idForLogging' | 'getLastChallengeError' | 'retrySend' -> & { - isNormalBubble(): boolean; - get(name: 'sent_at'): number; - on(event: 'sent', callback: () => void): void; - off(event: 'sent', callback: () => void): void; -}; - export type Options = { readonly storage: Pick; requestChallenge(request: IPCRequest): void; - getMessageById(messageId: string): Promise; + startQueue(conversationId: string): void; sendChallengeResponse(data: ChallengeData): Promise; @@ -81,25 +65,22 @@ export type Options = { expireAfter?: number; }; -export type StoredEntity = { - readonly messageId: string; - readonly createdAt: number; -}; +export const STORAGE_KEY = 'challenge:conversations'; -type TrackedEntry = { - readonly message: MinimalMessage; - readonly createdAt: number; -}; +export type RegisteredChallengeType = Readonly<{ + conversationId: string; + createdAt: number; + retryAt: number; + token?: string; +}>; const DEFAULT_EXPIRE_AFTER = 24 * 3600 * 1000; // one day -const MAX_RETRIES = 5; const CAPTCHA_URL = 'https://signalcaptchas.org/challenge/generate.html'; const CAPTCHA_STAGING_URL = 'https://signalcaptchas.org/staging/challenge/generate.html'; -function shouldRetrySend(message: MinimalMessage): boolean { - const error = message.getLastChallengeError(); - if (!error || error.retryAfter <= Date.now()) { +function shouldStartQueue(registered: RegisteredChallengeType): boolean { + if (!registered.retryAt || registered.retryAt <= Date.now()) { return true; } @@ -117,6 +98,8 @@ export function getChallengeURL(): string { // `ChallengeHandler` should be in memory at the same time because they could // overwrite each others storage data. export class ChallengeHandler { + private solving = 0; + private isLoaded = false; private challengeToken: string | undefined; @@ -127,13 +110,14 @@ export class ChallengeHandler { private readonly responseHandlers = new Map(); - private readonly trackedMessages = new Map(); + private readonly registeredConversations = new Map< + string, + RegisteredChallengeType + >(); - private readonly retryTimers = new Map(); + private readonly startTimers = new Map(); - private readonly pendingRetries = new Set(); - - private readonly retryCountById = new Map(); + private readonly pendingStarts = new Set(); constructor(private readonly options: Options) {} @@ -143,43 +127,18 @@ export class ChallengeHandler { } this.isLoaded = true; - const stored: ReadonlyArray = - this.options.storage.get('challenge:retry-message-ids') || []; + const challenges: ReadonlyArray = + this.options.storage.get(STORAGE_KEY) || []; - log.info(`challenge: loading ${stored.length} messages`); - - const entityMap = new Map(); - for (const entity of stored) { - entityMap.set(entity.messageId, entity); - } - - const retryIds = new Set(stored.map(({ messageId }) => messageId)); - - const maybeMessages: ReadonlyArray = - await Promise.all( - Array.from(retryIds).map(async messageId => - this.options.getMessageById(messageId) - ) - ); - - const messages: Array = maybeMessages.filter(isNotNil); - - log.info(`challenge: loaded ${messages.length} messages`); + log.info(`challenge: loading ${challenges.length} challenges`); await Promise.all( - messages.map(async message => { - const entity = entityMap.get(message.id); - if (!entity) { - log.error( - 'challenge: unexpected missing entity ' + - `for ${message.idForLogging()}` - ); - return; - } - + challenges.map(async challenge => { const expireAfter = this.options.expireAfter || DEFAULT_EXPIRE_AFTER; - if (isOlderThan(entity.createdAt, expireAfter)) { - log.info(`challenge: expired entity for ${message.idForLogging()}`); + if (isOlderThan(challenge.createdAt, expireAfter)) { + log.info( + `challenge: expired challenge for conversation ${challenge.conversationId}` + ); return; } @@ -190,7 +149,7 @@ export class ChallengeHandler { // // Wait for `.onOnline()` to trigger the retries instead of triggering // them here immediately (if the message is ready to be retried). - await this.register(message, RetryMode.NoImmediateRetry, entity); + await this.register(challenge); }) ); } @@ -204,89 +163,88 @@ export class ChallengeHandler { public async onOnline(): Promise { this.isOnline = true; - const pending = Array.from(this.pendingRetries.values()); - this.pendingRetries.clear(); + const pending = Array.from(this.pendingStarts.values()); + this.pendingStarts.clear(); - log.info(`challenge: online, retrying ${pending.length} messages`); + log.info(`challenge: online, starting ${pending.length} queues`); - // Retry messages that matured while we were offline - await Promise.all(pending.map(message => this.retryOne(message))); + // Start queues for challenges that matured while we were offline + await Promise.all( + pending.map(conversationId => this.startQueue(conversationId)) + ); - await this.retrySend(); + await this.startAllQueues(); + } + + public maybeSolve(conversationId: string): void { + const challenge = this.registeredConversations.get(conversationId); + if (!challenge) { + return; + } + + if (this.solving > 0) { + return; + } + + if (challenge.token) { + this.solve(challenge.token); + } } public async register( - message: MinimalMessage, - retry = RetryMode.Retry, - entity?: StoredEntity + challenge: RegisteredChallengeType, + data?: SendMessageChallengeData ): Promise { - if (this.isRegistered(message)) { - log.info( - `challenge: message already registered ${message.idForLogging()}` - ); + const { conversationId } = challenge; + + if (this.isRegistered(conversationId)) { + log.info(`challenge: conversation ${conversationId} already registered`); return; } - this.trackedMessages.set(message.id, { - message, - createdAt: entity ? entity.createdAt : Date.now(), - }); + this.registeredConversations.set(conversationId, challenge); await this.persist(); - // Message is already retryable - initiate new send - if (retry === RetryMode.Retry && shouldRetrySend(message)) { + // Challenge is already retryable - start the queue + if (shouldStartQueue(challenge)) { log.info( - `challenge: sending message immediately ${message.idForLogging()}` + `challenge: starting conversation ${conversationId} immediately` ); - await this.retryOne(message); + await this.startQueue(conversationId); return; } - const error = message.getLastChallengeError(); - if (!error) { - log.error('Unexpected message without challenge error'); - return; - } - - const waitTime = Math.max(0, error.retryAfter - Date.now()); - const oldTimer = this.retryTimers.get(message.id); + const waitTime = Math.max(0, challenge.retryAt - Date.now()); + const oldTimer = this.startTimers.get(conversationId); if (oldTimer) { clearTimeoutIfNecessary(oldTimer); } - this.retryTimers.set( - message.id, + this.startTimers.set( + conversationId, setTimeout(() => { - this.retryTimers.delete(message.id); + this.startTimers.delete(conversationId); - this.retryOne(message); + this.startQueue(conversationId); }, waitTime) ); - log.info( - `challenge: tracking ${message.idForLogging()} ` + - `with waitTime=${waitTime}` - ); + log.info(`challenge: tracking ${conversationId} with waitTime=${waitTime}`); - if (!error.data.options || !error.data.options.includes('recaptcha')) { + if (data && !data.options?.includes('recaptcha')) { log.error( - `challenge: unexpected options ${JSON.stringify(error.data.options)}` + `challenge: unexpected options ${JSON.stringify(data.options)}` ); } - if (!error.data.token) { + if (!challenge.token) { + const dataString = JSON.stringify(data); log.error( - `challenge: no token in challenge error ${JSON.stringify(error.data)}` + `challenge: ${conversationId} is waiting; no token in data ${dataString}` ); - } else if (message.isNormalBubble()) { - // Display challenge dialog only for core messages - // (e.g. text, attachment, embedded contact, or sticker) - // - // Note: not waiting on this call intentionally since it waits for - // challenge to be fully completed. - this.solve(error.data.token); - } else { - log.info(`challenge: not a bubble message ${message.idForLogging()}`); + return; } + + this.solve(challenge.token); } public onResponse(response: IPCResponse): void { @@ -299,13 +257,13 @@ export class ChallengeHandler { handler.resolve(response.data); } - public async unregister(message: MinimalMessage): Promise { - log.info(`challenge: unregistered ${message.idForLogging()}`); - this.trackedMessages.delete(message.id); - this.pendingRetries.delete(message); + public async unregister(conversationId: string): Promise { + log.info(`challenge: unregistered conversation ${conversationId}`); + this.registeredConversations.delete(conversationId); + this.pendingStarts.delete(conversationId); - const timer = this.retryTimers.get(message.id); - this.retryTimers.delete(message.id); + const timer = this.startTimers.get(conversationId); + this.startTimers.delete(conversationId); clearTimeoutIfNecessary(timer); await this.persist(); @@ -330,95 +288,45 @@ export class ChallengeHandler { 'ChallengeHandler has to be loaded before persisting new data' ); await this.options.storage.put( - 'challenge:retry-message-ids', - Array.from(this.trackedMessages.entries()).map( - ([messageId, { createdAt }]) => { - return { messageId, createdAt }; - } - ) + STORAGE_KEY, + Array.from(this.registeredConversations.values()) ); } - private isRegistered(message: MinimalMessage): boolean { - return this.trackedMessages.has(message.id); + public isRegistered(conversationId: string): boolean { + return this.registeredConversations.has(conversationId); } - private async retrySend(force = false): Promise { - log.info(`challenge: retrySend force=${force}`); + private startAllQueues({ + force = false, + }: { + force?: boolean; + } = {}): void { + log.info(`challenge: startAllQueues force=${force}`); - const retries = Array.from(this.trackedMessages.values()) - .map(({ message }) => message) - // Sort messages in `sent_at` order - .sort((a, b) => a.get('sent_at') - b.get('sent_at')) - .filter(message => force || shouldRetrySend(message)) - .map(message => this.retryOne(message)); - - await Promise.all(retries); + Array.from(this.registeredConversations.values()) + .filter(challenge => force || shouldStartQueue(challenge)) + .forEach(challenge => this.startQueue(challenge.conversationId)); } - private async retryOne(message: MinimalMessage): Promise { - // Send is already pending - if (!this.isRegistered(message)) { - return; - } - - // We are not online + private async startQueue(conversationId: string): Promise { if (!this.isOnline) { - this.pendingRetries.add(message); + this.pendingStarts.add(conversationId); return; } - const retryCount = this.retryCountById.get(message.id) || 0; - log.info( - `challenge: retrying sending ${message.idForLogging()}, ` + - `retry count: ${retryCount}` - ); + await this.unregister(conversationId); - if (retryCount === MAX_RETRIES) { - log.info( - `challenge: dropping message ${message.idForLogging()}, ` + - 'too many failed retries' - ); - - // Keep the message registered so that we'll retry sending it on app - // restart. - return; + if (this.registeredConversations.size === 0) { + this.options.setChallengeStatus('idle'); } - await this.unregister(message); - - let sent = false; - const onSent = () => { - sent = true; - }; - message.on('sent', onSent); - - try { - await message.retrySend(); - } catch (error) { - log.error( - `challenge: failed to send ${message.idForLogging()} due to ` + - `error: ${error && error.stack}` - ); - } finally { - message.off('sent', onSent); - } - - if (sent) { - log.info(`challenge: message ${message.idForLogging()} sent`); - this.retryCountById.delete(message.id); - if (this.trackedMessages.size === 0) { - this.options.setChallengeStatus('idle'); - } - } else { - log.info(`challenge: message ${message.idForLogging()} not sent`); - - this.retryCountById.set(message.id, retryCount + 1); - await this.register(message, RetryMode.NoImmediateRetry); - } + log.info(`startQueue: starting queue ${conversationId}`); + this.options.startQueue(conversationId); } private async solve(token: string): Promise { + this.solving += 1; this.options.setChallengeStatus('required'); this.challengeToken = token; @@ -426,6 +334,7 @@ export class ChallengeHandler { // Another `.solve()` has completed earlier than us if (this.challengeToken === undefined) { + this.solving -= 1; return; } @@ -445,6 +354,7 @@ export class ChallengeHandler { } catch (error) { log.error(`challenge: challenge failure, error: ${error && error.stack}`); this.options.setChallengeStatus('required'); + this.solving -= 1; return; } @@ -452,7 +362,8 @@ export class ChallengeHandler { this.options.setChallengeStatus('idle'); - this.retrySend(true); + this.startAllQueues({ force: true }); + this.solving -= 1; } private async sendChallengeResponse(data: ChallengeData): Promise { diff --git a/ts/components/CaptchaDialog.tsx b/ts/components/CaptchaDialog.tsx index 9d73dd1c3..cb3df1958 100644 --- a/ts/components/CaptchaDialog.tsx +++ b/ts/components/CaptchaDialog.tsx @@ -40,6 +40,7 @@ export function CaptchaDialog(props: Readonly): JSX.Element { i18n={i18n} title={i18n('CaptchaDialog--can-close__title')} onClose={() => setIsClosing(false)} + key="skip" >

{i18n('CaptchaDialog--can-close__body')}

@@ -76,6 +77,7 @@ export function CaptchaDialog(props: Readonly): JSX.Element { title={i18n('CaptchaDialog__title')} hasXButton onClose={() => setIsClosing(true)} + key="primary" >

{i18n('CaptchaDialog__first-paragraph')}

diff --git a/ts/jobs/conversationJobQueue.ts b/ts/jobs/conversationJobQueue.ts index db39a1643..e62acdbd0 100644 --- a/ts/jobs/conversationJobQueue.ts +++ b/ts/jobs/conversationJobQueue.ts @@ -22,14 +22,17 @@ import { sendReaction } from './helpers/sendReaction'; import type { LoggerType } from '../types/Logging'; import { ConversationVerificationState } from '../state/ducks/conversationsEnums'; import { sleep } from '../util/sleep'; -import { SECOND } from '../util/durations'; +import { MINUTE } from '../util/durations'; import { OutgoingIdentityKeyError, + SendMessageChallengeError, SendMessageProtoError, } from '../textsecure/Errors'; import { strictAssert } from '../util/assert'; import { missingCaseError } from '../util/missingCaseError'; import { explodePromise } from '../util/explodePromise'; +import type { Job } from './Job'; +import type { ParsedJob } from './types'; // Note: generally, we only want to add to this list. If you do need to change one of // these values, you'll likely need to write a database migration. @@ -135,6 +138,16 @@ export class ConversationJobQueue extends JobQueue { } >(); + public override async add( + data: Readonly, + insert?: (job: ParsedJob) => Promise + ): Promise> { + const { conversationId } = data; + window.Signal.challengeHandler.maybeSolve(conversationId); + + return super.add(data, insert); + } + protected parseData(data: unknown): ConversationQueueJobData { return conversationQueueJobDataSchema.parse(data); } @@ -215,6 +228,18 @@ export class ConversationJobQueue extends JobQueue { break; } + if (window.Signal.challengeHandler.isRegistered(conversationId)) { + log.info( + 'captcha challenge is pending for this conversation; waiting at most 5m...' + ); + // eslint-disable-next-line no-await-in-loop + await Promise.race([ + this.startVerificationWaiter(conversation.id), + sleep(5 * MINUTE), + ]); + continue; + } + const verificationData = window.reduxStore.getState().conversations .verificationDataByConversation[conversationId]; @@ -228,12 +253,12 @@ export class ConversationJobQueue extends JobQueue { ConversationVerificationState.PendingVerification ) { log.info( - 'verification is pending for this conversation; waiting at most 30s...' + 'verification is pending for this conversation; waiting at most 5m...' ); // eslint-disable-next-line no-await-in-loop await Promise.race([ this.startVerificationWaiter(conversation.id), - sleep(30 * SECOND), + sleep(5 * MINUTE), ]); continue; } @@ -302,25 +327,31 @@ export class ConversationJobQueue extends JobQueue { } } catch (error: unknown) { const untrustedConversationIds: Array = []; - if (error instanceof OutgoingIdentityKeyError) { - const failedConversation = window.ConversationController.getOrCreate( - error.identifier, - 'private' - ); - strictAssert(failedConversation, 'Conversation should be created'); - untrustedConversationIds.push(failedConversation.id); - } else if (error instanceof SendMessageProtoError) { - (error.errors || []).forEach(innerError => { - if (innerError instanceof OutgoingIdentityKeyError) { - const failedConversation = - window.ConversationController.getOrCreate( - innerError.identifier, - 'private' - ); - strictAssert(failedConversation, 'Conversation should be created'); - untrustedConversationIds.push(failedConversation.id); - } - }); + + const processError = (toProcess: unknown) => { + if (toProcess instanceof OutgoingIdentityKeyError) { + const failedConversation = window.ConversationController.getOrCreate( + toProcess.identifier, + 'private' + ); + strictAssert(failedConversation, 'Conversation should be created'); + untrustedConversationIds.push(failedConversation.id); + } else if (toProcess instanceof SendMessageChallengeError) { + window.Signal.challengeHandler.register( + { + conversationId, + createdAt: Date.now(), + retryAt: toProcess.retryAt, + token: toProcess.data?.token, + }, + toProcess.data + ); + } + }; + + processError(error); + if (error instanceof SendMessageProtoError) { + (error.errors || []).forEach(processError); } if (untrustedConversationIds.length) { diff --git a/ts/jobs/helpers/sendNormalMessage.ts b/ts/jobs/helpers/sendNormalMessage.ts index d095bdf6c..9cf90818d 100644 --- a/ts/jobs/helpers/sendNormalMessage.ts +++ b/ts/jobs/helpers/sendNormalMessage.ts @@ -13,10 +13,7 @@ import { SignalService as Proto } from '../../protobuf'; import { handleMessageSend } from '../../util/handleMessageSend'; import type { CallbackResultType } from '../../textsecure/Types.d'; import { isSent } from '../../messages/MessageSendState'; -import { - getLastChallengeError, - isOutgoing, -} from '../../state/selectors/message'; +import { isOutgoing } from '../../state/selectors/message'; import type { AttachmentType } from '../../textsecure/SendMessage'; import type { LinkPreviewType } from '../../types/message/LinkPreviews'; import type { BodyRangesType, StoryContextType } from '../../types/Util'; @@ -286,18 +283,6 @@ export async function sendNormalMessage( await messageSendPromise; - if ( - getLastChallengeError({ - errors: messageSendErrors, - }) - ) { - log.info( - `message ${messageId} hit a spam challenge. Not retrying any more` - ); - await message.saveErrors(messageSendErrors); - return; - } - const didFullySend = !messageSendErrors.length || didSendToEveryone(message); if (!didFullySend) { diff --git a/ts/jobs/removeStorageKeyJobQueue.ts b/ts/jobs/removeStorageKeyJobQueue.ts index 85c64b0a3..eb5ccb752 100644 --- a/ts/jobs/removeStorageKeyJobQueue.ts +++ b/ts/jobs/removeStorageKeyJobQueue.ts @@ -7,7 +7,7 @@ import { JobQueue } from './JobQueue'; import { jobQueueDatabaseStore } from './JobQueueDatabaseStore'; const removeStorageKeyJobDataSchema = z.object({ - key: z.enum(['senderCertificateWithUuid']), + key: z.enum(['senderCertificateWithUuid', 'challenge:retry-message-ids']), }); type RemoveStorageKeyJobData = z.infer; diff --git a/ts/models/messages.ts b/ts/models/messages.ts index f25cb36ac..c3c4eee34 100644 --- a/ts/models/messages.ts +++ b/ts/models/messages.ts @@ -7,7 +7,6 @@ import type { GroupV1Update, MessageAttributesType, MessageReactionType, - ShallowChallengeError, QuotedMessageType, WhatIsThis, } from '../model-types.d'; @@ -78,7 +77,6 @@ import { handleMessageSend } from '../util/handleMessageSend'; import { getSendOptions } from '../util/getSendOptions'; import { findAndFormatContact } from '../util/findAndFormatContact'; import { - getLastChallengeError, getMessagePropStatus, getPropsForCallHistory, getPropsForMessage, @@ -1151,13 +1149,6 @@ export class MessageModel extends window.Backbone.Model { this.set({ errors }); - if ( - !this.doNotSave && - errors.some(error => error.name === 'SendMessageChallengeError') - ) { - await window.Signal.challengeHandler.register(this); - } - if (!skipSave && !this.doNotSave) { await window.Signal.Data.saveMessage(this.attributes, { ourUuid: window.textsecure.storage.user.getCheckedUuid().toString(), @@ -1683,10 +1674,6 @@ export class MessageModel extends window.Backbone.Model { return false; } - getLastChallengeError(): ShallowChallengeError | undefined { - return getLastChallengeError(this.attributes); - } - hasAttachmentDownloads(): boolean { return hasAttachmentDownloads(this.attributes); } diff --git a/ts/test-both/challenge_test.ts b/ts/test-both/challenge_test.ts index b98625211..d3b4a3bbd 100644 --- a/ts/test-both/challenge_test.ts +++ b/ts/test-both/challenge_test.ts @@ -7,27 +7,21 @@ import { assert } from 'chai'; import { noop } from 'lodash'; import * as sinon from 'sinon'; -import type { MinimalMessage } from '../challenge'; -import { ChallengeHandler } from '../challenge'; - -type CreateMessageOptions = { - readonly sentAt?: number; - readonly retryAfter?: number; - readonly isNormalBubble?: boolean; -}; +import { STORAGE_KEY, ChallengeHandler } from '../challenge'; +import type { RegisteredChallengeType } from '../challenge'; +import { DAY, SECOND } from '../util/durations'; type CreateHandlerOptions = { readonly autoSolve?: boolean; readonly challengeError?: Error; readonly expireAfter?: number; readonly onChallengeSolved?: () => void; - readonly onChallengeFailed?: (retryAfter?: number) => void; + readonly onChallengeFailed?: (retryAt?: number) => void; }; const NOW = Date.now(); -const ONE_DAY = 24 * 3600 * 1000; -const NEVER_RETRY = NOW + ONE_DAY; -const IMMEDIATE_RETRY = NOW - ONE_DAY; +const NEVER_RETRY = NOW + DAY; +const IMMEDIATE_RETRY = NOW - DAY; // Various timeouts in milliseconds const DEFAULT_RETRY_AFTER = 25; @@ -35,15 +29,13 @@ const SOLVE_AFTER = 5; describe('ChallengeHandler', () => { const storage = new Map(); - const messageStorage = new Map(); let challengeStatus = 'idle'; - let sent: Array = []; + let queuesStarted: Array = []; beforeEach(function beforeEach() { storage.clear(); - messageStorage.clear(); challengeStatus = 'idle'; - sent = []; + queuesStarted = []; this.sandbox = sinon.createSandbox(); this.clock = this.sandbox.useFakeTimers({ @@ -55,56 +47,16 @@ describe('ChallengeHandler', () => { this.sandbox.restore(); }); - const createMessage = ( - id: string, - options: CreateMessageOptions = {} - ): MinimalMessage => { - const { - sentAt = 0, - isNormalBubble = true, - retryAfter = NOW + DEFAULT_RETRY_AFTER, - } = options; - - const testLocalSent = sent; - - const events = new Map void>(); - + const createChallenge = ( + conversationId: string, + options: Partial = {} + ): RegisteredChallengeType => { return { - id, - idForLogging: () => id, - isNormalBubble() { - return isNormalBubble; - }, - getLastChallengeError() { - return { - name: 'Ignored', - message: 'Ignored', - retryAfter, - data: { token: 'token', options: ['recaptcha'] }, - }; - }, - get(name) { - assert.equal(name, 'sent_at'); - return sentAt; - }, - on(name, handler) { - if (events.get(name)) { - throw new Error('Duplicate event'); - } - events.set(name, handler); - }, - off(name, handler) { - assert.equal(events.get(name), handler); - events.delete(name); - }, - async retrySend() { - const handler = events.get('sent'); - if (!handler) { - throw new Error('Expected handler'); - } - handler(); - testLocalSent.push(this.id); - }, + conversationId, + token: '1', + retryAt: NOW + DEFAULT_RETRY_AFTER, + createdAt: NOW - SECOND, + ...options, }; }; @@ -127,6 +79,10 @@ describe('ChallengeHandler', () => { }, }, + startQueue(conversationId: string) { + queuesStarted.push(conversationId); + }, + onChallengeSolved, onChallengeFailed, @@ -143,10 +99,6 @@ describe('ChallengeHandler', () => { }, SOLVE_AFTER); }, - async getMessageById(messageId) { - return messageStorage.get(messageId); - }, - async sendChallengeResponse() { if (challengeError) { throw challengeError; @@ -162,200 +114,156 @@ describe('ChallengeHandler', () => { return handler; }; - const isInStorage = (messageId: string) => { - return (storage.get('challenge:retry-message-ids') || []).some( - ({ messageId: storageId }: { messageId: string }) => { - return storageId === messageId; + const isInStorage = (conversationId: string) => { + return (storage.get(STORAGE_KEY) || []).some( + ({ conversationId: storageId }: { conversationId: string }) => { + return storageId === conversationId; } ); }; - it('should automatically retry after timeout', async function test() { + it('should automatically start queue after timeout', async function test() { const handler = await createHandler(); - const one = createMessage('1'); - messageStorage.set('1', one); - + const one = createChallenge('1'); await handler.register(one); - assert.isTrue(isInStorage(one.id)); + assert.isTrue(isInStorage(one.conversationId)); assert.equal(challengeStatus, 'required'); await this.clock.nextAsync(); - assert.deepEqual(sent, ['1']); + assert.deepEqual(queuesStarted, [one.conversationId]); assert.equal(challengeStatus, 'idle'); - assert.isFalse(isInStorage(one.id)); + assert.isFalse(isInStorage(one.conversationId)); }); it('should send challenge response', async function test() { const handler = await createHandler({ autoSolve: true }); - const one = createMessage('1', { retryAfter: NEVER_RETRY }); - messageStorage.set('1', one); - + const one = createChallenge('1', { + retryAt: NEVER_RETRY, + }); await handler.register(one); assert.equal(challengeStatus, 'required'); await this.clock.nextAsync(); - assert.deepEqual(sent, ['1']); - assert.isFalse(isInStorage(one.id)); + assert.deepEqual(queuesStarted, [one.conversationId]); + assert.isFalse(isInStorage(one.conversationId)); assert.equal(challengeStatus, 'idle'); }); - it('should send old messages', async function test() { + it('should send old challenges', async function test() { const handler = await createHandler(); - // Put messages in reverse order to validate that the send order is correct - const messages = [ - createMessage('3', { sentAt: 3 }), - createMessage('2', { sentAt: 2 }), - createMessage('1', { sentAt: 1 }), + const challenges = [ + createChallenge('1'), + createChallenge('2'), + createChallenge('3'), ]; - for (const message of messages) { - messageStorage.set(message.id, message); - await handler.register(message); + for (const challenge of challenges) { + await handler.register(challenge); } assert.equal(challengeStatus, 'required'); - assert.deepEqual(sent, []); + assert.deepEqual(queuesStarted, []); - for (const message of messages) { + for (const challenge of challenges) { assert.isTrue( - isInStorage(message.id), - `${message.id} should be in storage` + isInStorage(challenge.conversationId), + `${challenge.conversationId} should be in storage` ); } await handler.onOffline(); - // Wait for messages to mature + // Wait for challenges to mature await this.clock.nextAsync(); - // Create new handler to load old messages from storage + // Create new handler to load old challenges from storage; it will start up online await createHandler(); - for (const message of messages) { - await handler.unregister(message); + + for (const challenge of challenges) { + await handler.unregister(challenge.conversationId); } - for (const message of messages) { + for (const challenge of challenges) { assert.isFalse( - isInStorage(message.id), - `${message.id} should not be in storage` + isInStorage(challenge.conversationId), + `${challenge.conversationId} should not be in storage` ); } // The order has to be correct - assert.deepEqual(sent, ['1', '2', '3']); + assert.deepEqual(queuesStarted, ['1', '2', '3']); assert.equal(challengeStatus, 'idle'); }); - it('should send message immediately if it is ready', async () => { + it('should send challenge immediately if it is ready', async () => { const handler = await createHandler(); - const one = createMessage('1', { retryAfter: IMMEDIATE_RETRY }); - await handler.register(one); - - assert.equal(challengeStatus, 'idle'); - assert.deepEqual(sent, ['1']); - }); - - it('should not change challenge status on non-bubble messages', async function test() { - const handler = await createHandler(); - - const one = createMessage('1', { - isNormalBubble: false, + const one = createChallenge('1', { + retryAt: IMMEDIATE_RETRY, }); await handler.register(one); assert.equal(challengeStatus, 'idle'); - assert.deepEqual(sent, []); - - await this.clock.nextAsync(); - - assert.deepEqual(sent, ['1']); + assert.deepEqual(queuesStarted, [one.conversationId]); }); - it('should not retry expired messages', async function test() { + it('should not retry expired challenges', async function test() { const handler = await createHandler(); - const bubble = createMessage('1'); - messageStorage.set('1', bubble); - await handler.register(bubble); - assert.isTrue(isInStorage(bubble.id)); + const one = createChallenge('1'); + await handler.register(one); + assert.isTrue(isInStorage(one.conversationId)); const newHandler = await createHandler({ autoSolve: true, expireAfter: -1, }); - await handler.unregister(bubble); + await handler.unregister(one.conversationId); challengeStatus = 'idle'; await newHandler.load(); assert.equal(challengeStatus, 'idle'); - assert.deepEqual(sent, []); + assert.deepEqual(queuesStarted, []); await this.clock.nextAsync(); assert.equal(challengeStatus, 'idle'); - assert.deepEqual(sent, []); - assert.isFalse(isInStorage(bubble.id)); + assert.deepEqual(queuesStarted, []); + assert.isFalse(isInStorage(one.conversationId)); }); - it('should send messages that matured while we were offline', async function test() { + it('should send challenges that matured while we were offline', async function test() { const handler = await createHandler(); - const one = createMessage('1'); - messageStorage.set('1', one); + const one = createChallenge('1'); await handler.register(one); - assert.isTrue(isInStorage(one.id)); - assert.deepEqual(sent, []); + assert.isTrue(isInStorage(one.conversationId)); + assert.deepEqual(queuesStarted, []); assert.equal(challengeStatus, 'required'); await handler.onOffline(); - // Let messages mature + // Let challenges mature await this.clock.nextAsync(); - assert.isTrue(isInStorage(one.id)); - assert.deepEqual(sent, []); + assert.isTrue(isInStorage(one.conversationId)); + assert.deepEqual(queuesStarted, []); assert.equal(challengeStatus, 'required'); // Go back online await handler.onOnline(); - assert.isFalse(isInStorage(one.id)); - assert.deepEqual(sent, [one.id]); + assert.isFalse(isInStorage(one.conversationId)); + assert.deepEqual(queuesStarted, [one.conversationId]); assert.equal(challengeStatus, 'idle'); }); - it('should not retry more than 5 times', async function test() { - const handler = await createHandler(); - - const one = createMessage('1', { retryAfter: IMMEDIATE_RETRY }); - const retrySend = sinon.stub(one, 'retrySend'); - - messageStorage.set('1', one); - await handler.register(one); - - assert.isTrue(isInStorage(one.id)); - assert.deepEqual(sent, []); - assert.equal(challengeStatus, 'required'); - - // Wait more than 5 times - for (let i = 0; i < 6; i += 1) { - await this.clock.nextAsync(); - } - - assert.isTrue(isInStorage(one.id)); - assert.deepEqual(sent, []); - assert.equal(challengeStatus, 'required'); - - sinon.assert.callCount(retrySend, 5); - }); - it('should trigger onChallengeSolved', async function test() { const onChallengeSolved = sinon.stub(); @@ -364,8 +272,9 @@ describe('ChallengeHandler', () => { onChallengeSolved, }); - const one = createMessage('1', { retryAfter: NEVER_RETRY }); - messageStorage.set('1', one); + const one = createChallenge('1', { + retryAt: NEVER_RETRY, + }); await handler.register(one); // Let the challenge go through @@ -383,8 +292,9 @@ describe('ChallengeHandler', () => { onChallengeFailed, }); - const one = createMessage('1', { retryAfter: NEVER_RETRY }); - messageStorage.set('1', one); + const one = createChallenge('1', { + retryAt: NEVER_RETRY, + }); await handler.register(one); // Let the challenge go through diff --git a/ts/textsecure/Errors.ts b/ts/textsecure/Errors.ts index d70bb2b8b..39f4e7ecc 100644 --- a/ts/textsecure/Errors.ts +++ b/ts/textsecure/Errors.ts @@ -156,7 +156,7 @@ export class SendMessageChallengeError extends ReplayableError { public readonly data: SendMessageChallengeData | undefined; - public readonly retryAfter: number; + public readonly retryAt: number; constructor(identifier: string, httpError: HTTPError) { super({ @@ -171,7 +171,7 @@ export class SendMessageChallengeError extends ReplayableError { const headers = httpError.responseHeaders || {}; - this.retryAfter = Date.now() + parseRetryAfter(headers['retry-after']); + this.retryAt = Date.now() + parseRetryAfter(headers['retry-after']); appendStack(this, httpError); } diff --git a/ts/types/Storage.d.ts b/ts/types/Storage.d.ts index 76914b334..a4acc64a4 100644 --- a/ts/types/Storage.d.ts +++ b/ts/types/Storage.d.ts @@ -24,6 +24,8 @@ import type { SessionResetsType, StorageServiceCredentials, } from '../textsecure/Types.d'; +import { UUIDStringType } from './UUID'; +import { RegisteredChallengeType } from '../challenge'; export type SerializedCertificateType = { expires: number; @@ -124,10 +126,8 @@ export type StorageAccessType = { preferredReactionEmoji: Array; skinTone: number; unreadCount: number; - 'challenge:retry-message-ids': ReadonlyArray<{ - messageId: string; - createdAt: number; - }>; + 'challenge:conversations': ReadonlyArray; + deviceNameEncrypted: boolean; 'indexeddb-delete-needed': boolean; senderCertificate: SerializedCertificateType; @@ -144,6 +144,7 @@ export type StorageAccessType = { // Deprecated senderCertificateWithUuid: never; signaling_key: never; + 'challenge:retry-message-ids': never; }; export interface StorageInterface {