From 269c87a42e3280f53a9704cda1454464bc1fcc6f Mon Sep 17 00:00:00 2001 From: Mikunj Date: Fri, 3 Jul 2020 13:07:27 +1000 Subject: [PATCH 1/4] Fix closed group issues --- js/background.js | 8 +-- js/models/conversations.js | 69 ++++++++++++------- js/models/messages.js | 14 +++- .../session/SessionGroupSettings.tsx | 2 +- ts/receiver/dataMessage.ts | 2 +- 5 files changed, 63 insertions(+), 32 deletions(-) diff --git a/js/background.js b/js/background.js index 26e72a198..4f921b706 100644 --- a/js/background.js +++ b/js/background.js @@ -639,13 +639,15 @@ confirm: () => {}, }; - await window.NewReceiver.onGroupReceived(ev); - const convo = await ConversationController.getOrCreateAndWait( groupId, 'group' ); + const recipients = _.union(convo.get('members'), members); + + await window.NewReceiver.onGroupReceived(ev); + if (convo.isPublic()) { const API = await convo.getPublicSendData(); @@ -703,8 +705,6 @@ } const options = {}; - const recipients = _.union(convo.get('members'), members); - const isMediumGroup = convo.isMediumGroup(); const updateObj = { diff --git a/js/models/conversations.js b/js/models/conversations.js index be10cd40f..d4d78f16b 100644 --- a/js/models/conversations.js +++ b/js/models/conversations.js @@ -1239,12 +1239,12 @@ recipients, }); - if (this.isPrivate()) { - messageWithSchema.destination = destination; - } else if (this.isPublic()) { + if (this.isPublic()) { // Public chats require this data to detect duplicates messageWithSchema.source = textsecure.storage.user.getNumber(); messageWithSchema.sourceDevice = 1; + } else { + messageWithSchema.destination = destination; } const { sessionRestoration = false } = otherOptions; @@ -1368,6 +1368,7 @@ } if (conversationType === Message.GROUP) { + const members = this.get('members'); if (this.isMediumGroup()) { const mediumGroupChatMessage = new libsession.Messages.Outgoing.MediumGroupChatMessage( { @@ -1375,7 +1376,6 @@ groupId: destination, } ); - const members = this.get('members'); await Promise.all( members.map(async m => { const memberPubKey = new libsession.Types.PubKey(m); @@ -1391,6 +1391,16 @@ groupId: destination, } ); + + // Special-case the self-send case - we send only a sync message + if (members.length === 1) { + const isOurDevice = await libsession.Protocols.MultiDeviceProtocol.isOurDevice(members[0]); + if (isOurDevice) { + await message.sendSyncMessageOnly(closedGroupChatMessage); + return true; + } + } + await libsession .getMessageQueue() .sendToGroup(closedGroupChatMessage); @@ -1815,13 +1825,16 @@ ); message.set({ id: messageId }); + // Difference between `recipients` and `members` is that `recipients` includes the members which were removed in this update + const { id, name, members, avatar, recipients } = groupUpdate; + if (groupUpdate.is_medium_group) { + const { secretKey, senderKey } = groupUpdate; // Constructing a "create group" message - const { id, name, secretKey, senderKey, members } = groupUpdate; const { chainKey, keyIdx } = senderKey; const createParams = { - timestamp: Date.now(), + timestamp: now, groupId: id, identifier: messageId, groupSecretKey: secretKey, @@ -1849,17 +1862,18 @@ const updateParams = { // if we do set an identifier here, be sure to not sync the message two times in msg.handleMessageSentSuccess() - timestamp: Date.now(), - groupId: this.id, - name: this.get('name'), - avatar: this.get('avatar'), - members: this.get('members'), + timestamp: now, + groupId: id, + name, + avatar, + members, admins: this.get('groupAdmins'), }; const groupUpdateMessage = new libsession.Messages.Outgoing.ClosedGroupUpdateMessage( updateParams ); - await this.sendClosedGroupMessageWithSync(groupUpdateMessage); + + await this.sendClosedGroupMessageWithSync(groupUpdateMessage, recipients); }, sendGroupInfo(recipient) { @@ -1935,7 +1949,7 @@ } }, - async sendClosedGroupMessageWithSync(message) { + async sendClosedGroupMessageWithSync(message, recipients) { const { ClosedGroupMessage, ClosedGroupChatMessage, @@ -1951,19 +1965,28 @@ ); } + const members = recipients || this.get('members'); + try { - await libsession.getMessageQueue().sendToGroup(message); + // Exclude our device from members and send them the message + const ourNumber = textsecure.storage.user.getNumber(); + const primary = await libsession.Protocols.MultiDeviceProtocol.getPrimaryDevice(ourNumber); + const otherMembers = (members || []).filter(member => !primary.isEqual(member)); + const sendPromises = otherMembers.map(member => { + const memberPubKey = libsession.Types.PubKey.cast(member); + return libsession.getMessageQueue().sendUsingMultiDevice(memberPubKey, message); + }); + await Promise.all(sendPromises); - const syncMessage = libsession.Utils.SyncMessageUtils.getSentSyncMessage( - { - destination: message.groupId, - message, - } - ); + // Send the sync message to our devices + const syncMessage = new libsession.Messages.Outgoing.SentSyncMessage({ + timestamp: Date.now(), + identifier: message.identifier, + destination: message.groupId, + dataMessage: message.dataProto(), + }); - if (syncMessage) { - await libsession.getMessageQueue().sendSyncMessage(syncMessage); - } + await libsession.getMessageQueue().sendSyncMessage(syncMessage); } catch (e) { window.log.error(e); } diff --git a/js/models/messages.js b/js/models/messages.js index e1ff2bfde..3c402d9f3 100644 --- a/js/models/messages.js +++ b/js/models/messages.js @@ -1100,8 +1100,11 @@ }); // Special-case the self-send case - we send only a sync message - if (recipients.length === 1 && recipients[0] === this.OUR_NUMBER) { - return this.sendSyncMessageOnly(chatMessage); + if (recipients.length === 1) { + const isOurDevice = await libsession.Protocols.MultiDeviceProtocol.isOurDevice(recipients[0]); + if (isOurDevice) { + return this.sendSyncMessageOnly(chatMessage); + } } if (conversation.isPrivate()) { @@ -1433,10 +1436,15 @@ return; } + const data = + dataMessage instanceof libsession.Messages.Outgoing.DataMessage + ? dataMessage.dataProto() + : dataMessage; + const syncMessage = new libsession.Messages.Outgoing.SentSyncMessage({ timestamp: this.get('sent_at'), identifier: this.id, - dataMessage, + dataMessage: data, destination: this.get('destination'), expirationStartTimestamp: this.get('expirationStartTimestamp'), sent_to: this.get('sent_to'), diff --git a/ts/components/session/SessionGroupSettings.tsx b/ts/components/session/SessionGroupSettings.tsx index 5a1c13c8d..826b66939 100644 --- a/ts/components/session/SessionGroupSettings.tsx +++ b/ts/components/session/SessionGroupSettings.tsx @@ -222,7 +222,7 @@ export class SessionGroupSettings extends React.Component { const leaveGroupString = isPublic ? window.i18n('leaveOpenGroup') : isKickedFromGroup - ? window.i18n('youAreKickedFromThisGroup') + ? window.i18n('youGotKickedFromThisGroup') : window.i18n('leaveClosedGroup'); const disappearingMessagesOptions = timerOptions.map(option => { diff --git a/ts/receiver/dataMessage.ts b/ts/receiver/dataMessage.ts index 2bdf50f06..beffafcbe 100644 --- a/ts/receiver/dataMessage.ts +++ b/ts/receiver/dataMessage.ts @@ -533,7 +533,7 @@ export async function handleMessageEvent(event: any): Promise { let { source } = data; - const isGroupMessage = message.group; + const isGroupMessage = Boolean(message.group); const type = isGroupMessage ? ConversationType.GROUP From fd547941d66f09c2778c14afe6223ae5c724ff88 Mon Sep 17 00:00:00 2001 From: Mikunj Date: Fri, 3 Jul 2020 13:53:17 +1000 Subject: [PATCH 2/4] Fix sync message issue --- js/views/inbox_view.js | 5 ++++ .../outgoing/content/sync/SentSyncMessage.ts | 3 +- ts/session/utils/Protobuf.ts | 28 +++++++++++++++++++ ts/session/utils/index.ts | 2 ++ 4 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 ts/session/utils/Protobuf.ts diff --git a/js/views/inbox_view.js b/js/views/inbox_view.js index 0e4c89681..8ec8e024b 100644 --- a/js/views/inbox_view.js +++ b/js/views/inbox_view.js @@ -297,6 +297,11 @@ const msg = conv.messageCollection.models.find( convMsg => convMsg.id === tmpMsg.id ); + + if (!msg) { + return null; + } + return { msg }; }, diff --git a/ts/session/messages/outgoing/content/sync/SentSyncMessage.ts b/ts/session/messages/outgoing/content/sync/SentSyncMessage.ts index 328fd9958..8778e9938 100644 --- a/ts/session/messages/outgoing/content/sync/SentSyncMessage.ts +++ b/ts/session/messages/outgoing/content/sync/SentSyncMessage.ts @@ -2,6 +2,7 @@ import { SyncMessage } from './SyncMessage'; import { SignalService } from '../../../../../protobuf'; import { MessageParams } from '../../Message'; import { PubKey } from '../../../../types'; +import { ProtobufUtils } from '../../../../utils'; interface SentSyncMessageParams extends MessageParams { dataMessage: SignalService.IDataMessage; @@ -21,7 +22,7 @@ export class SentSyncMessage extends SyncMessage { constructor(params: SentSyncMessageParams) { super({ timestamp: params.timestamp, identifier: params.identifier }); - this.dataMessage = params.dataMessage; + this.dataMessage = ProtobufUtils.convertToTS(params.dataMessage); this.expirationStartTimestamp = params.expirationStartTimestamp; this.sentTo = params.sentTo; this.unidentifiedDeliveries = params.unidentifiedDeliveries; diff --git a/ts/session/utils/Protobuf.ts b/ts/session/utils/Protobuf.ts new file mode 100644 index 000000000..ac9023803 --- /dev/null +++ b/ts/session/utils/Protobuf.ts @@ -0,0 +1,28 @@ +import ByteBuffer from 'bytebuffer'; + +/** + * Converts any object to a valid ts protobuf object. + * + * This is needed because there's a very jarring difference between `protobufjs` and `protobufts`. + * `protobufjs` returns all `bytes` as `ByteBuffer` where as `protobufts` returns all `bytes` as `Uint8Array`. + */ +export function convertToTS(object: any): any { + // No idea why js `ByteBuffer` and ts `ByteBuffer` differ ... + if (object && object.constructor && object.constructor.name === 'ByteBuffer') { + return new Uint8Array(object.toArrayBuffer()); + } else if (object instanceof ByteBuffer || object instanceof Buffer || object instanceof ArrayBuffer || object instanceof SharedArrayBuffer) { + const arrayBuffer = ByteBuffer.wrap(object).toArrayBuffer(); + return new Uint8Array(arrayBuffer); + } else if (Array.isArray(object)) { + return object.map(convertToTS); + } else if (object && typeof object === 'object') { + const keys = Object.keys(object); + const values: { [key: string]: any } = {}; + for (const key of keys) { + values[key] = convertToTS(object[key]); + } + return values; + } + + return object; +} diff --git a/ts/session/utils/index.ts b/ts/session/utils/index.ts index 73a11d4de..2db75fa65 100644 --- a/ts/session/utils/index.ts +++ b/ts/session/utils/index.ts @@ -3,6 +3,7 @@ import * as GroupUtils from './Groups'; import * as SyncMessageUtils from './SyncMessage'; import * as StringUtils from './String'; import * as PromiseUtils from './Promise'; +import * as ProtobufUtils from './Protobuf'; export * from './Attachments'; export * from './TypedEmitter'; @@ -14,4 +15,5 @@ export { GroupUtils, StringUtils, PromiseUtils, + ProtobufUtils }; From c422c9e2ade5ae951f19d1f2afb52008c4f82efe Mon Sep 17 00:00:00 2001 From: Mikunj Date: Fri, 3 Jul 2020 13:59:14 +1000 Subject: [PATCH 3/4] Lint --- js/models/conversations.js | 16 ++++++++++++---- js/models/messages.js | 4 +++- ts/receiver/dataMessage.ts | 2 +- .../data/group/ClosedGroupUpdateMessage.ts | 3 --- ts/session/utils/Protobuf.ts | 13 +++++++++++-- ts/session/utils/index.ts | 2 +- 6 files changed, 28 insertions(+), 12 deletions(-) diff --git a/js/models/conversations.js b/js/models/conversations.js index d4d78f16b..987817412 100644 --- a/js/models/conversations.js +++ b/js/models/conversations.js @@ -1394,7 +1394,9 @@ // Special-case the self-send case - we send only a sync message if (members.length === 1) { - const isOurDevice = await libsession.Protocols.MultiDeviceProtocol.isOurDevice(members[0]); + const isOurDevice = await libsession.Protocols.MultiDeviceProtocol.isOurDevice( + members[0] + ); if (isOurDevice) { await message.sendSyncMessageOnly(closedGroupChatMessage); return true; @@ -1970,11 +1972,17 @@ try { // Exclude our device from members and send them the message const ourNumber = textsecure.storage.user.getNumber(); - const primary = await libsession.Protocols.MultiDeviceProtocol.getPrimaryDevice(ourNumber); - const otherMembers = (members || []).filter(member => !primary.isEqual(member)); + const primary = await libsession.Protocols.MultiDeviceProtocol.getPrimaryDevice( + ourNumber + ); + const otherMembers = (members || []).filter( + member => !primary.isEqual(member) + ); const sendPromises = otherMembers.map(member => { const memberPubKey = libsession.Types.PubKey.cast(member); - return libsession.getMessageQueue().sendUsingMultiDevice(memberPubKey, message); + return libsession + .getMessageQueue() + .sendUsingMultiDevice(memberPubKey, message); }); await Promise.all(sendPromises); diff --git a/js/models/messages.js b/js/models/messages.js index 3c402d9f3..ae60a3283 100644 --- a/js/models/messages.js +++ b/js/models/messages.js @@ -1101,7 +1101,9 @@ // Special-case the self-send case - we send only a sync message if (recipients.length === 1) { - const isOurDevice = await libsession.Protocols.MultiDeviceProtocol.isOurDevice(recipients[0]); + const isOurDevice = await libsession.Protocols.MultiDeviceProtocol.isOurDevice( + recipients[0] + ); if (isOurDevice) { return this.sendSyncMessageOnly(chatMessage); } diff --git a/ts/receiver/dataMessage.ts b/ts/receiver/dataMessage.ts index beffafcbe..b6ec395c9 100644 --- a/ts/receiver/dataMessage.ts +++ b/ts/receiver/dataMessage.ts @@ -508,7 +508,7 @@ function createMessage( } function sendDeliveryReceipt(source: string, timestamp: any) { -// FIXME audric + // FIXME audric // const receiptMessage = new DeliveryReceiptMessage({ // timestamp: Date.now(), // timestamps: [timestamp], diff --git a/ts/session/messages/outgoing/content/data/group/ClosedGroupUpdateMessage.ts b/ts/session/messages/outgoing/content/data/group/ClosedGroupUpdateMessage.ts index 3c4aa14c0..dfcbf14c2 100644 --- a/ts/session/messages/outgoing/content/data/group/ClosedGroupUpdateMessage.ts +++ b/ts/session/messages/outgoing/content/data/group/ClosedGroupUpdateMessage.ts @@ -1,12 +1,9 @@ -import { DataMessage } from '../DataMessage'; import { SignalService } from '../../../../../../protobuf'; -import { StringUtils } from '../../../../../utils'; import { ClosedGroupMessage, ClosedGroupMessageParams, } from './ClosedGroupMessage'; import { AttachmentPointer } from '../ChatMessage'; -import { PubKey } from '../../../../../types'; export interface ClosedGroupUpdateMessageParams extends ClosedGroupMessageParams { diff --git a/ts/session/utils/Protobuf.ts b/ts/session/utils/Protobuf.ts index ac9023803..59f6a0e91 100644 --- a/ts/session/utils/Protobuf.ts +++ b/ts/session/utils/Protobuf.ts @@ -8,9 +8,18 @@ import ByteBuffer from 'bytebuffer'; */ export function convertToTS(object: any): any { // No idea why js `ByteBuffer` and ts `ByteBuffer` differ ... - if (object && object.constructor && object.constructor.name === 'ByteBuffer') { + if ( + object && + object.constructor && + object.constructor.name === 'ByteBuffer' + ) { return new Uint8Array(object.toArrayBuffer()); - } else if (object instanceof ByteBuffer || object instanceof Buffer || object instanceof ArrayBuffer || object instanceof SharedArrayBuffer) { + } else if ( + object instanceof ByteBuffer || + object instanceof Buffer || + object instanceof ArrayBuffer || + object instanceof SharedArrayBuffer + ) { const arrayBuffer = ByteBuffer.wrap(object).toArrayBuffer(); return new Uint8Array(arrayBuffer); } else if (Array.isArray(object)) { diff --git a/ts/session/utils/index.ts b/ts/session/utils/index.ts index 2db75fa65..5548d7125 100644 --- a/ts/session/utils/index.ts +++ b/ts/session/utils/index.ts @@ -15,5 +15,5 @@ export { GroupUtils, StringUtils, PromiseUtils, - ProtobufUtils + ProtobufUtils, }; From 2ff177d814c09fd98032849ed08d237a6e496481 Mon Sep 17 00:00:00 2001 From: Mikunj Date: Fri, 3 Jul 2020 14:32:57 +1000 Subject: [PATCH 4/4] Fix promise test --- ts/test/session/utils/Promise_test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ts/test/session/utils/Promise_test.ts b/ts/test/session/utils/Promise_test.ts index c4ca24057..476a2ff6a 100644 --- a/ts/test/session/utils/Promise_test.ts +++ b/ts/test/session/utils/Promise_test.ts @@ -69,8 +69,8 @@ describe('Promise Utils', () => { it('will recur according to interval option', async () => { const expectedRecurrences = 4; - const timeout = 1000; - const interval = timeout / expectedRecurrences; + const timeout = 3000; + const interval = 50; const recurrenceSpy = sandbox.spy(); const task = (done: any) => {