From ba6cb653bf5016a9c877ac6c3db1529a9c901f7b Mon Sep 17 00:00:00 2001 From: Josh Perez <60019601+josh-signal@users.noreply.github.com> Date: Mon, 6 Jul 2020 20:39:55 -0400 Subject: [PATCH] Drop group messages that don't change group --- js/background.js | 1 + js/models/messages.js | 105 ++++++++++++++++++++++------- js/modules/attachment_downloads.js | 87 +++++------------------- ts/textsecure.d.ts | 22 ++++++ ts/textsecure/MessageReceiver.ts | 26 ++----- ts/util/downloadAttachment.ts | 32 +++++++++ ts/util/index.ts | 2 + 7 files changed, 158 insertions(+), 117 deletions(-) create mode 100644 ts/util/downloadAttachment.ts diff --git a/js/background.js b/js/background.js index e37464c5d46a..b86834aa0508 100644 --- a/js/background.js +++ b/js/background.js @@ -1614,6 +1614,7 @@ mySignalingKey, options ); + window.textsecure.messageReceiver = messageReceiver; function addQueuedEventListener(name, handler) { messageReceiver.addEventListener(name, (...args) => diff --git a/js/models/messages.js b/js/models/messages.js index a3303211a9bd..40ae2cd301de 100644 --- a/js/models/messages.js +++ b/js/models/messages.js @@ -1881,22 +1881,6 @@ }; } - let group = this.get('group_update'); - if (group && group.avatar) { - window.log.info( - `Queueing group avatar download for message ${this.idForLogging()}` - ); - count += 1; - group = { - ...group, - avatar: await window.Signal.AttachmentDownloads.addJob(group.avatar, { - messageId, - type: 'group-avatar', - index: 0, - }), - }; - } - let sticker = this.get('sticker'); if (sticker) { window.log.info( @@ -1957,7 +1941,6 @@ preview, contact, quote, - group_update: group, sticker, }); @@ -2259,7 +2242,7 @@ ...conversation.attributes, }; if (dataMessage.group) { - let groupUpdate = null; + const pendingGroupUpdate = []; const memberConversations = await Promise.all( ( dataMessage.group.members || dataMessage.group.membersE164 @@ -2289,20 +2272,83 @@ members: _.union(members, conversation.get('members')), }; - groupUpdate = {}; if (dataMessage.group.name !== conversation.get('name')) { - groupUpdate.name = dataMessage.group.name; + pendingGroupUpdate.push(['name', dataMessage.group.name]); } - // Note: used and later cleared by background attachment downloader - groupUpdate.avatar = dataMessage.group.avatar; + const avatarAttachment = dataMessage.group.avatar; + + let downloadedAvatar; + let hash; + if (avatarAttachment) { + try { + downloadedAvatar = await window.Signal.Util.downloadAttachment( + avatarAttachment + ); + + if (downloadedAvatar) { + const loadedAttachment = await Signal.Migrations.loadAttachmentData( + downloadedAvatar + ); + + hash = await Signal.Types.Conversation.computeHash( + loadedAttachment.data + ); + } + } catch (err) { + window.log.info( + 'handleDataMessage: group avatar download failed' + ); + } + } + + const existingAvatar = conversation.get('avatar'); + + if ( + // Avatar added + !existingAvatar || + // Avatar changed + (existingAvatar && existingAvatar.hash !== hash) || + // Avatar removed + avatarAttachment === null + ) { + // Removes existing avatar from disk + if (existingAvatar && existingAvatar.path) { + await Signal.Migrations.deleteAttachmentData( + existingAvatar.path + ); + } + + let avatar = null; + if (downloadedAvatar && avatarAttachment !== null) { + const onDiskAttachment = await window.Signal.Types.Attachment.migrateDataToFileSystem( + downloadedAvatar, + { + writeNewAttachmentData: + window.Signal.Migrations.writeNewAttachmentData, + } + ); + avatar = { + ...onDiskAttachment, + hash, + }; + } + + attributes.avatar = avatar; + + pendingGroupUpdate.push(['avatarUpdated', true]); + } else { + window.log.info( + 'handleDataMessage: Group avatar hash matched; not replacing group avatar' + ); + } const difference = _.difference( members, conversation.get('members') ); if (difference.length > 0) { - groupUpdate.joined = difference; + pendingGroupUpdate.push(['joined', difference]); } if (conversation.get('left')) { window.log.warn('re-added to a left group'); @@ -2324,9 +2370,9 @@ if (sender.isMe()) { attributes.left = true; - groupUpdate = { left: 'You' }; + pendingGroupUpdate.push(['left', 'You']); } else { - groupUpdate = { left: sender.get('id') }; + pendingGroupUpdate.push(['left', sender.get('id')]); } attributes.members = _.without( conversation.get('members'), @@ -2334,7 +2380,14 @@ ); } - if (groupUpdate !== null) { + if (pendingGroupUpdate.length) { + const groupUpdate = pendingGroupUpdate.reduce( + (acc, [key, value]) => { + acc[key] = value; + return acc; + }, + {} + ); message.set({ group_update: groupUpdate }); } } diff --git a/js/modules/attachment_downloads.js b/js/modules/attachment_downloads.js index 764db737f519..28a34a4dfd86 100644 --- a/js/modules/attachment_downloads.js +++ b/js/modules/attachment_downloads.js @@ -1,5 +1,4 @@ /* global - ConversationController, Whisper, Signal, setTimeout, @@ -8,7 +7,6 @@ */ const { isFunction, isNumber, omit } = require('lodash'); -const { computeHash } = require('./types/conversation'); const getGuid = require('uuid/v4'); const { getMessageById, @@ -19,6 +17,7 @@ const { saveMessage, setAttachmentDownloadJobPending, } = require('../../ts/sql/Client').default; +const { downloadAttachment } = require('../../ts/util/downloadAttachment'); const { stringFromBytes } = require('../../ts/Crypto'); module.exports = { @@ -186,38 +185,28 @@ async function _runJob(job) { const pending = true; await setAttachmentDownloadJobPending(id, pending); - let downloaded; const messageReceiver = getMessageReceiver(); if (!messageReceiver) { throw new Error('_runJob: messageReceiver not found'); } - try { - if (attachment.id) { - // eslint-disable-next-line no-param-reassign - attachment.cdnId = attachment.id; - } - downloaded = await messageReceiver.downloadAttachment(attachment); - } catch (error) { - // Attachments on the server expire after 30 days, then start returning 404 - if (error && error.code === 404) { - logger.warn( - `_runJob: Got 404 from server for CDN ${ - attachment.cdnNumber - }, marking attachment ${attachment.cdnId || - attachment.cdnKey} from message ${message.idForLogging()} as permanent error` - ); + const downloaded = await downloadAttachment(attachment); - await _finishJob(message, id); - await _addAttachmentToMessage( - message, - _markAttachmentAsError(attachment), - { type, index } - ); + if (!downloaded) { + logger.warn( + `_runJob: Got 404 from server for CDN ${ + attachment.cdnNumber + }, marking attachment ${attachment.cdnId || + attachment.cdnKey} from message ${message.idForLogging()} as permanent error` + ); - return; - } - throw error; + await _finishJob(message, id); + await _addAttachmentToMessage( + message, + _markAttachmentAsError(attachment), + { type, index } + ); + return; } const upgradedAttachment = await Signal.Migrations.processNewAttachment( @@ -425,50 +414,6 @@ async function _addAttachmentToMessage(message, attachment, { type, index }) { return; } - if (type === 'group-avatar') { - const conversationId = message.get('conversationId'); - const conversation = ConversationController.get(conversationId); - if (!conversation) { - logger.warn("_addAttachmentToMessage: conversation didn't exist"); - return; - } - - const loadedAttachment = await Signal.Migrations.loadAttachmentData( - attachment - ); - const hash = await computeHash(loadedAttachment.data); - const existingAvatar = conversation.get('avatar'); - - if (existingAvatar) { - if (existingAvatar.hash === hash) { - logger.info( - '_addAttachmentToMessage: Group avatar hash matched; not replacing group avatar' - ); - return; - } - - await Signal.Migrations.deleteAttachmentData(existingAvatar.path); - } - - conversation.set({ - avatar: { - ...attachment, - hash, - }, - }); - Signal.Data.updateConversation(conversation.attributes); - - message.set({ - group_update: { - ...message.get('group_update'), - avatar: null, - avatarUpdated: true, - }, - }); - - return; - } - if (type === 'sticker') { const sticker = message.get('sticker'); if (!sticker) { diff --git a/ts/textsecure.d.ts b/ts/textsecure.d.ts index 0c4b4bbfa858..da2556dfb516 100644 --- a/ts/textsecure.d.ts +++ b/ts/textsecure.d.ts @@ -62,6 +62,11 @@ export type TextSecureType = { remove: (key: string | Array) => Promise; protocol: StorageProtocolType; }; + messageReceiver: { + downloadAttachment: ( + attachment: AttachmentPointerClass + ) => Promise; + }; messaging: { sendStickerPackSync: ( operations: Array<{ @@ -172,6 +177,23 @@ export declare class AttachmentPointerClass { cdnNumber?: number; } +export type DownloadAttachmentType = { + data: ArrayBuffer; + cdnId?: ProtoBigNumberType; + cdnKey?: string; + contentType?: string; + size?: number; + thumbnail?: ProtoBinaryType; + fileName?: string; + flags?: number; + width?: number; + height?: number; + caption?: string; + blurHash?: string; + uploadTimestamp?: ProtoBigNumberType; + cdnNumber?: number; +}; + export declare class ContactDetailsClass { static decode: ( data: ArrayBuffer | ByteBufferClass, diff --git a/ts/textsecure/MessageReceiver.ts b/ts/textsecure/MessageReceiver.ts index 801b95bdf414..80b34ec5bd53 100644 --- a/ts/textsecure/MessageReceiver.ts +++ b/ts/textsecure/MessageReceiver.ts @@ -20,8 +20,8 @@ import { IncomingIdentityKeyError } from './Errors'; import { AttachmentPointerClass, DataMessageClass, + DownloadAttachmentType, EnvelopeClass, - ProtoBigNumberType, ReceiptMessageClass, SyncMessageClass, TypingMessageClass, @@ -62,22 +62,6 @@ declare global { } } -type AttachmentType = { - cdnId?: string; - cdnKey?: string; - data: ArrayBuffer; - contentType?: string; - size?: number; - fileName?: string; - flags?: number; - width?: number; - height?: number; - caption?: string; - blurHash?: string; - uploadTimestamp?: ProtoBigNumberType; - cdnNumber?: number; -}; - type CacheAddItemType = { envelope: EnvelopeClass; data: UnprocessedType; @@ -1591,7 +1575,9 @@ class MessageReceiverInner extends EventTarget { digest: attachment.digest ? attachment.digest.toString('base64') : null, }; } - async downloadAttachment(attachment: AttachmentPointerClass) { + async downloadAttachment( + attachment: AttachmentPointerClass + ): Promise { const encrypted = await this.server.getAttachment( attachment.cdnId || attachment.cdnKey, attachment.cdnNumber || 0 @@ -1623,7 +1609,7 @@ class MessageReceiverInner extends EventTarget { } async handleAttachment( attachment: AttachmentPointerClass - ): Promise { + ): Promise { const cleaned = this.cleanAttachment(attachment); return this.downloadAttachment(cleaned); } @@ -1866,7 +1852,7 @@ export default class MessageReceiver { close: () => Promise; downloadAttachment: ( attachment: AttachmentPointerClass - ) => Promise; + ) => Promise; stopProcessing: () => Promise; unregisterBatchers: () => void; diff --git a/ts/util/downloadAttachment.ts b/ts/util/downloadAttachment.ts new file mode 100644 index 000000000000..5a52a455e350 --- /dev/null +++ b/ts/util/downloadAttachment.ts @@ -0,0 +1,32 @@ +import { + AttachmentPointerClass, + DownloadAttachmentType, +} from '../textsecure.d'; + +type AttachmentData = AttachmentPointerClass & { + id?: string; +}; + +export async function downloadAttachment( + attachmentData: AttachmentData +): Promise { + let downloaded; + try { + if (attachmentData.id) { + // eslint-disable-next-line no-param-reassign + attachmentData.cdnId = attachmentData.id; + } + downloaded = await window.textsecure.messageReceiver.downloadAttachment( + attachmentData + ); + } catch (error) { + // Attachments on the server expire after 30 days, then start returning 404 + if (error && error.code === 404) { + return null; + } + + throw error; + } + + return downloaded; +} diff --git a/ts/util/index.ts b/ts/util/index.ts index f767c2ba4019..8600b428e5ca 100644 --- a/ts/util/index.ts +++ b/ts/util/index.ts @@ -4,6 +4,7 @@ import { arrayBufferToObjectURL } from './arrayBufferToObjectURL'; import { combineNames } from './combineNames'; import { createBatcher } from './batcher'; import { createWaitBatcher } from './waitBatcher'; +import { downloadAttachment } from './downloadAttachment'; import { hasExpired } from './hasExpired'; import { isFileDangerous } from './isFileDangerous'; import { makeLookup } from './makeLookup'; @@ -16,6 +17,7 @@ export { combineNames, createBatcher, createWaitBatcher, + downloadAttachment, GoogleChrome, hasExpired, isFileDangerous,