Merge pull request #1192 from Mikunj/send-to-group

Handle typing and expiration messages when sending to group

js/models/conversations.js

@@ -417,13 +417,13 @@
         libsession
           .getMessageQueue()
           .sendUsingMultiDevice(device, typingMessage)
-          .ignore();
+          .catch(log.error);
       } else {
         // the recipients on the case of a group are found by the messageQueue using message.groupId
         libsession
           .getMessageQueue()
           .sendToGroup(typingMessage)
-          .ignore();
+          .catch(log.error);
       }
     },
 
@@ -1925,7 +1925,7 @@
       libsession
         .getMessageQueue()
         .sendToGroup(groupUpdateMessage)
-        .ignore();
+        .catch(log.error);
     },
 
     sendGroupInfo(recipient) {
@@ -1949,7 +1949,7 @@
         libsession
           .getMessageQueue()
           .send(recipientPubKey, groupUpdateMessage)
-          .ignore();
+          .catch(log.error);
       }
     },
 

test/models/conversations_test.js

@@ -96,7 +96,7 @@ describe('Conversation', () => {
   it('adds conversation to message collection upon leaving group', async () => {
     const convo = new Whisper.ConversationCollection().add({
       type: 'group',
-      id: 'a random string',
+      id: '052d11d01e56d9bfc3d74115c33225a632321b509ac17a13fdeac71165d09b94ab',
     });
     await convo.leaveGroup();
     assert.notEqual(convo.messageCollection.length, 0);

ts/session/messages/outgoing/content/TypingMessage.ts

@@ -3,23 +3,26 @@ import { SignalService } from '../../../../protobuf';
 import { TextEncoder } from 'util';
 import { MessageParams } from '../Message';
 import { StringUtils } from '../../../utils';
+import { PubKey } from '../../../types';
 
 interface TypingMessageParams extends MessageParams {
   isTyping: boolean;
   typingTimestamp?: number;
-  groupId?: string;
+  groupId?: string | PubKey;
 }
 
 export class TypingMessage extends ContentMessage {
-  private readonly isTyping: boolean;
+  public readonly isTyping: boolean;
-  private readonly typingTimestamp?: number;
+  public readonly typingTimestamp?: number;
-  private readonly groupId?: string;
+  public readonly groupId?: PubKey;
 
   constructor(params: TypingMessageParams) {
     super({ timestamp: params.timestamp, identifier: params.identifier });
     this.isTyping = params.isTyping;
     this.typingTimestamp = params.typingTimestamp;
-    this.groupId = params.groupId;
+
+    const { groupId } = params;
+    this.groupId = groupId ? PubKey.cast(groupId) : undefined;
   }
 
   public ttl(): number {
@@ -41,7 +44,7 @@ export class TypingMessage extends ContentMessage {
     const typingMessage = new SignalService.TypingMessage();
     if (this.groupId) {
       typingMessage.groupId = new Uint8Array(
-        StringUtils.encode(this.groupId, 'utf8')
+        StringUtils.encode(this.groupId.key, 'utf8')
       );
     }
     typingMessage.action = action;

ts/session/messages/outgoing/content/data/ExpirationTimerUpdateMessage.ts

@@ -3,23 +3,26 @@ import { SignalService } from '../../../../../protobuf';
 import { MessageParams } from '../../Message';
 import { StringUtils } from '../../../../utils';
 import { DataMessage } from './DataMessage';
+import { PubKey } from '../../../../types';
 
 interface ExpirationTimerUpdateMessageParams extends MessageParams {
-  groupId?: string;
+  groupId?: string | PubKey;
   expireTimer: number | null;
   profileKey?: Uint8Array;
 }
 
 export class ExpirationTimerUpdateMessage extends DataMessage {
-  private readonly groupId?: string;
+  public readonly groupId?: PubKey;
-  private readonly expireTimer: number | null;
+  public readonly expireTimer: number | null;
-  private readonly profileKey?: Uint8Array;
+  public readonly profileKey?: Uint8Array;
 
   constructor(params: ExpirationTimerUpdateMessageParams) {
     super({ timestamp: params.timestamp, identifier: params.identifier });
-    this.groupId = params.groupId;
     this.expireTimer = params.expireTimer;
     this.profileKey = params.profileKey;
+
+    const { groupId } = params;
+    this.groupId = groupId ? PubKey.cast(groupId) : undefined;
   }
 
   public ttl(): number {
@@ -32,7 +35,7 @@ export class ExpirationTimerUpdateMessage extends DataMessage {
     const groupMessage = new SignalService.GroupContext();
     if (this.groupId) {
       groupMessage.id = new Uint8Array(
-        StringUtils.encode(this.groupId, 'utf8')
+        StringUtils.encode(this.groupId.key, 'utf8')
       );
       groupMessage.type = SignalService.GroupContext.Type.DELIVER;
     }

ts/session/messages/outgoing/content/data/group/ClosedGroupMessage.ts

@@ -16,8 +16,7 @@ export abstract class ClosedGroupMessage extends DataMessage {
       timestamp: params.timestamp,
       identifier: params.identifier,
     });
-    const { groupId } = params;
+    this.groupId = PubKey.cast(params.groupId);
-    this.groupId = typeof groupId === 'string' ? new PubKey(groupId) : groupId;
   }
 
   public ttl(): number {

ts/session/messages/outgoing/content/data/index.ts

@@ -3,3 +3,4 @@ export * from './DeviceUnlinkMessage';
 export * from './GroupInvitationMessage';
 export * from './ChatMessage';
 export * from './group';
+export * from './ExpirationTimerUpdateMessage';

ts/session/protocols/MultiDeviceProtocol.ts

@@ -140,7 +140,7 @@ export class MultiDeviceProtocol {
   public static async getPairingAuthorisations(
     device: PubKey | string
   ): Promise<Array<PairingAuthorisation>> {
-    const pubKey = typeof device === 'string' ? new PubKey(device) : device;
+    const pubKey = PubKey.cast(device);
     await this.fetchPairingAuthorisationsIfNeeded(pubKey);
 
     return getPairingAuthorisationsFor(pubKey.key);
@@ -153,7 +153,7 @@ export class MultiDeviceProtocol {
   public static async removePairingAuthorisations(
     device: PubKey | string
   ): Promise<void> {
-    const pubKey = typeof device === 'string' ? new PubKey(device) : device;
+    const pubKey = PubKey.cast(device);
 
     return removePairingAuthorisationsFor(pubKey.key);
   }
@@ -166,7 +166,7 @@ export class MultiDeviceProtocol {
   public static async getAllDevices(
     user: PubKey | string
   ): Promise<Array<PubKey>> {
-    const pubKey = typeof user === 'string' ? new PubKey(user) : user;
+    const pubKey = PubKey.cast(user);
     const authorisations = await this.getPairingAuthorisations(pubKey);
     if (authorisations.length === 0) {
       return [pubKey];
@@ -190,7 +190,7 @@ export class MultiDeviceProtocol {
   public static async getPrimaryDevice(
     user: PubKey | string
   ): Promise<PrimaryPubKey> {
-    const pubKey = typeof user === 'string' ? new PubKey(user) : user;
+    const pubKey = PubKey.cast(user);
     const authorisations = await this.getPairingAuthorisations(pubKey);
     if (authorisations.length === 0) {
       return pubKey;
@@ -237,7 +237,7 @@ export class MultiDeviceProtocol {
    * @param device The device to check.
    */
   public static async isOurDevice(device: PubKey | string): Promise<boolean> {
-    const pubKey = typeof device === 'string' ? new PubKey(device) : device;
+    const pubKey = PubKey.cast(device);
     try {
       const ourDevices = await this.getOurDevices();
 

ts/session/sending/MessageQueue.ts

@@ -6,9 +6,11 @@ import {
 import {
   ClosedGroupMessage,
   ContentMessage,
+  ExpirationTimerUpdateMessage,
   OpenGroupMessage,
   SessionRequestMessage,
   SyncMessage,
+  TypingMessage,
 } from '../messages/outgoing';
 import { PendingMessageCache } from './PendingMessageCache';
 import {
@@ -33,13 +35,16 @@ export class MessageQueue implements MessageQueueInterface {
     void this.processAllPending();
   }
 
-  public async sendUsingMultiDevice(user: PubKey, message: ContentMessage) {
+  public async sendUsingMultiDevice(
+    user: PubKey,
+    message: ContentMessage
+  ): Promise<void> {
     const userDevices = await MultiDeviceProtocol.getAllDevices(user.key);
 
     await this.sendMessageToDevices(userDevices, message);
   }
 
-  public async send(device: PubKey, message: ContentMessage) {
+  public async send(device: PubKey, message: ContentMessage): Promise<void> {
     await this.sendMessageToDevices([device], message);
   }
 
@@ -75,26 +80,8 @@ export class MessageQueue implements MessageQueueInterface {
   }
 
   public async sendToGroup(
-    message: OpenGroupMessage | ClosedGroupMessage
+    message: OpenGroupMessage | ContentMessage
-  ): Promise<boolean> {
+  ): Promise<void> {
-    // Closed groups
-    if (message instanceof ClosedGroupMessage) {
-      // Get devices in closed group
-      const recipients = await GroupUtils.getGroupMembers(message.groupId);
-      if (recipients.length === 0) {
-        return false;
-      }
-
-      // Send to all devices of members
-      await Promise.all(
-        recipients.map(async recipient =>
-          this.sendUsingMultiDevice(recipient, message)
-        )
-      );
-
-      return true;
-    }
-
     // Open groups
     if (message instanceof OpenGroupMessage) {
       // No queue needed for Open Groups; send directly
@@ -108,20 +95,42 @@ export class MessageQueue implements MessageQueueInterface {
         } else {
           this.events.emit('fail', message, error);
         }
-
-        return result;
       } catch (e) {
         console.warn(
           `Failed to send message to open group: ${message.group.server}`,
           e
         );
         this.events.emit('fail', message, error);
+      }
+
+      return;
+    }
 
-        return false;
+    let groupId: PubKey | undefined;
+    if (message instanceof ClosedGroupMessage) {
+      groupId = message.groupId;
+    } else if (message instanceof TypingMessage) {
+      groupId = message.groupId;
+    } else if (message instanceof ExpirationTimerUpdateMessage) {
+      groupId = message.groupId;
+    }
+
+    if (!groupId) {
+      throw new Error('Invalid group message passed in sendToGroup.');
     }
+
+    // Get devices in group
+    const recipients = await GroupUtils.getGroupMembers(groupId);
+    if (recipients.length === 0) {
+      return;
     }
 
-    return false;
+    // Send to all devices of members
+    await Promise.all(
+      recipients.map(async recipient =>
+        this.sendUsingMultiDevice(recipient, message)
+      )
+    );
   }
 
   public async sendSyncMessage(message: SyncMessage | undefined): Promise<any> {

ts/session/sending/MessageQueueInterface.ts

@@ -17,8 +17,8 @@ export interface MessageQueueInterfaceEvents {
 
 export interface MessageQueueInterface {
   events: TypedEventEmitter<MessageQueueInterfaceEvents>;
-  sendUsingMultiDevice(user: PubKey, message: ContentMessage): void;
+  sendUsingMultiDevice(user: PubKey, message: ContentMessage): Promise<void>;
-  send(device: PubKey, message: ContentMessage): void;
+  send(device: PubKey, message: ContentMessage): Promise<void>;
-  sendToGroup(message: GroupMessageType): void;
+  sendToGroup(message: GroupMessageType): Promise<void>;
   sendSyncMessage(message: SyncMessage | undefined): Promise<any>;
 }

ts/session/types/PubKey.ts

@@ -5,11 +5,35 @@ export class PubKey {
   );
   public readonly key: string;
 
+  /**
+   * A PubKey object.
+   * If `pubKeyString` is not valid then this will throw an `Error`.
+   *
+   * @param pubkeyString The public key string.
+   */
   constructor(pubkeyString: string) {
-    PubKey.validate(pubkeyString);
+    if (!PubKey.validate(pubkeyString)) {
+      throw new Error(`Invalid pubkey string passed: ${pubkeyString}`);
+    }
     this.key = pubkeyString;
   }
 
+  /**
+   * Cast a `value` to a `PubKey`.
+   * If `value` is not valid then this will throw.
+   *
+   * @param value The value to cast.
+   */
+  public static cast(value: string | PubKey): PubKey {
+    return typeof value === 'string' ? new PubKey(value) : value;
+  }
+
+  /**
+   * Try convert `pubKeyString` to `PubKey`.
+   *
+   * @param pubkeyString The public key string.
+   * @returns `PubKey` if valid otherwise returns `undefined`.
+   */
   public static from(pubkeyString: string): PubKey | undefined {
     // Returns a new instance if the pubkey is valid
     if (PubKey.validate(pubkeyString)) {

ts/test/session/messages/ClosedGroupChatMessage_test.ts

@@ -6,22 +6,32 @@ import {
 } from '../../../session/messages/outgoing';
 import { SignalService } from '../../../protobuf';
 import { TextEncoder } from 'util';
+import { TestUtils } from '../../test-utils';
+import { StringUtils } from '../../../session/utils';
+import { PubKey } from '../../../session/types';
 
 describe('ClosedGroupChatMessage', () => {
+  let groupId: PubKey;
+  beforeEach(() => {
+    groupId = TestUtils.generateFakePubKey();
+  });
   it('can create empty message with timestamp, groupId and chatMessage', () => {
     const chatMessage = new ChatMessage({
       timestamp: Date.now(),
       body: 'body',
     });
     const message = new ClosedGroupChatMessage({
-      groupId: '12',
+      groupId,
       chatMessage,
     });
     const plainText = message.plainTextBuffer();
     const decoded = SignalService.Content.decode(plainText);
     expect(decoded.dataMessage)
       .to.have.property('group')
-      .to.have.deep.property('id', new TextEncoder().encode('12'));
+      .to.have.deep.property(
+        'id',
+        new Uint8Array(StringUtils.encode(groupId.key, 'utf8'))
+      );
     expect(decoded.dataMessage)
       .to.have.property('group')
       .to.have.deep.property('type', SignalService.GroupContext.Type.DELIVER);
@@ -39,7 +49,7 @@ describe('ClosedGroupChatMessage', () => {
       timestamp: Date.now(),
     });
     const message = new ClosedGroupChatMessage({
-      groupId: '12',
+      groupId,
       chatMessage,
     });
     expect(message.ttl()).to.equal(24 * 60 * 60 * 1000);
@@ -50,7 +60,7 @@ describe('ClosedGroupChatMessage', () => {
       timestamp: Date.now(),
     });
     const message = new ClosedGroupChatMessage({
-      groupId: '12',
+      groupId,
       chatMessage,
     });
     expect(message.identifier).to.not.equal(null, 'identifier cannot be null');
@@ -67,7 +77,7 @@ describe('ClosedGroupChatMessage', () => {
       identifier: 'chatMessage',
     });
     const message = new ClosedGroupChatMessage({
-      groupId: '12',
+      groupId,
       chatMessage,
       identifier: 'closedGroupMessage',
     });
@@ -81,7 +91,7 @@ describe('ClosedGroupChatMessage', () => {
       identifier: 'chatMessage',
     });
     const message = new ClosedGroupChatMessage({
-      groupId: '12',
+      groupId,
       chatMessage,
     });
     expect(message.identifier).to.be.equal('chatMessage');

ts/test/session/messages/TypingMessage_test.ts

@@ -5,6 +5,8 @@ import { SignalService } from '../../../protobuf';
 import { TextEncoder } from 'util';
 import Long from 'long';
 import { toNumber } from 'lodash';
+import { StringUtils } from '../../../session/utils';
+import { TestUtils } from '../../test-utils';
 
 describe('TypingMessage', () => {
   it('has Action.STARTED if isTyping = true', () => {
@@ -60,7 +62,7 @@ describe('TypingMessage', () => {
   });
 
   it('has groupId set if a value given', () => {
-    const groupId = '6666666666';
+    const groupId = TestUtils.generateFakePubKey();
     const message = new TypingMessage({
       timestamp: Date.now(),
       isTyping: true,
@@ -68,7 +70,9 @@ describe('TypingMessage', () => {
     });
     const plainText = message.plainTextBuffer();
     const decoded = SignalService.Content.decode(plainText);
-    const manuallyEncodedGroupId = new TextEncoder().encode(groupId);
+    const manuallyEncodedGroupId = new Uint8Array(
+      StringUtils.encode(groupId.key, 'utf8')
+    );
 
     expect(decoded.typingMessage?.groupId).to.be.deep.equal(
       manuallyEncodedGroupId

ts/test/session/protocols/SessionProtocol_test.ts

@@ -12,7 +12,7 @@ import { PubKey } from '../../../session/types';
 describe('SessionProtocol', () => {
   const sandbox = sinon.createSandbox();
   const ourNumber = 'ourNumber';
-  const pubkey = new PubKey('deviceid');
+  const pubkey = TestUtils.generateFakePubKey();
   let getItemById: sinon.SinonStub;
   let send: sinon.SinonStub;
 
@@ -88,14 +88,14 @@ describe('SessionProtocol', () => {
 
     it('protocol: sendSessionRequest should add the deviceID to the sentMap', async () => {
       expect(SessionProtocol.getSentSessionsTimestamp())
-        .to.have.property('deviceid')
+        .to.have.property(pubkey.key)
         .to.be.approximately(Date.now(), 100);
     });
 
     it('protocol: sendSessionRequest should not have pendingSend set after', async () => {
       expect(
         SessionProtocol.getPendingSendSessionTimestamp()
-      ).to.not.have.property('deviceid');
+      ).to.not.have.property(pubkey.key);
     });
   });
 
@@ -107,34 +107,32 @@ describe('SessionProtocol', () => {
 
     it('protocol: onSessionEstablished should remove the device in sentTimestamps', async () => {
       expect(SessionProtocol.getSentSessionsTimestamp()).to.have.property(
-        'deviceid'
+        pubkey.key
       );
       await SessionProtocol.onSessionEstablished(pubkey);
       expect(SessionProtocol.getSentSessionsTimestamp()).to.not.have.property(
-        'deviceid'
+        pubkey.key
       );
     });
 
     it('protocol: onSessionEstablished should remove the device in sentTimestamps and ONLY that one', async () => {
       // add a second item to the map
-      await SessionProtocol.sendSessionRequest(
+      const anotherPubKey = TestUtils.generateFakePubKey();
-        resetMessage,
+      await SessionProtocol.sendSessionRequest(resetMessage, anotherPubKey);
-        new PubKey('deviceid2')
-      );
 
       expect(SessionProtocol.getSentSessionsTimestamp()).to.have.property(
-        'deviceid'
+        pubkey.key
       );
       expect(SessionProtocol.getSentSessionsTimestamp()).to.have.property(
-        'deviceid2'
+        anotherPubKey.key
       );
 
       await SessionProtocol.onSessionEstablished(pubkey);
       expect(SessionProtocol.getSentSessionsTimestamp()).to.not.have.property(
-        'deviceid'
+        pubkey.key
       );
       expect(SessionProtocol.getSentSessionsTimestamp()).to.have.property(
-        'deviceid2'
+        anotherPubKey.key
       );
     });
   });
@@ -144,7 +142,7 @@ describe('SessionProtocol', () => {
       const hasSent = await SessionProtocol.hasSentSessionRequest(pubkey);
       expect(hasSent).to.be.equal(
         false,
-        'hasSent should be false for `deviceid`'
+        `hasSent should be false for ${pubkey.key}`
       );
     });
 
@@ -154,7 +152,7 @@ describe('SessionProtocol', () => {
       const hasSent = await SessionProtocol.hasSentSessionRequest(pubkey);
       expect(hasSent).to.be.equal(
         true,
-        'hasSent should be true for `deviceid`'
+        `hasSent should be true for ${pubkey.key}`
       );
     });
 
@@ -174,7 +172,7 @@ describe('SessionProtocol', () => {
       const hasSent = await SessionProtocol.hasSentSessionRequest(pubkey);
       expect(hasSent).to.be.equal(
         true,
-        'hasSent should be true for `deviceid`'
+        `hasSent should be true for ${pubkey.key}`
       );
     });
 
@@ -189,7 +187,7 @@ describe('SessionProtocol', () => {
       const hasSent = await SessionProtocol.hasSentSessionRequest(pubkey);
       expect(hasSent).to.be.equal(
         true,
-        'hasSent should be true for `deviceid`'
+        `hasSent should be true for ${pubkey.key}`
       );
       send.resetHistory();
 
@@ -207,7 +205,7 @@ describe('SessionProtocol', () => {
       // trigger the requestProcessed and check the map is updated
       await SessionProtocol.onSessionRequestProcessed(pubkey);
       expect(SessionProtocol.getProcessedSessionsTimestamp())
-        .to.have.property('deviceid')
+        .to.have.property(pubkey.key)
         .to.be.approximately(Date.now(), 5);
     });
 
@@ -217,14 +215,15 @@ describe('SessionProtocol', () => {
 
       await SessionProtocol.onSessionRequestProcessed(pubkey);
       expect(SessionProtocol.getProcessedSessionsTimestamp())
-        .to.have.property('deviceid')
+        .to.have.property(pubkey.key)
         .to.be.approximately(Date.now(), 5);
       await TestUtils.timeout(5);
-      const oldTimestamp = SessionProtocol.getProcessedSessionsTimestamp()
+      const oldTimestamp = SessionProtocol.getProcessedSessionsTimestamp()[
-        .deviceid;
+        pubkey.key
+      ];
       await SessionProtocol.onSessionRequestProcessed(pubkey);
       expect(SessionProtocol.getProcessedSessionsTimestamp())
-        .to.have.property('deviceid')
+        .to.have.property(pubkey.key)
         .to.be.approximately(Date.now(), 5)
         .to.not.be.equal(oldTimestamp);
     });

ts/test/session/sending/MessageQueue_test.ts

@@ -312,6 +312,13 @@ describe('MessageQueue', () => {
   });
 
   describe('sendToGroup', () => {
+    it('should throw an error if invalid non-group message was passed', async () => {
+      const chatMessage = TestUtils.generateChatMessage();
+      await expect(
+        messageQueueStub.sendToGroup(chatMessage)
+      ).to.be.rejectedWith('Invalid group message passed in sendToGroup.');
+    });
+
     describe('closed groups', async () => {
       it('can send to closed group', async () => {
         const members = TestUtils.generateFakePubKeys(4).map(
@@ -324,8 +331,7 @@ describe('MessageQueue', () => {
           .resolves();
 
         const message = TestUtils.generateClosedGroupMessage();
-        const success = await messageQueueStub.sendToGroup(message);
+        await messageQueueStub.sendToGroup(message);
-        expect(success).to.equal(true, 'sending to group failed');
         expect(sendUsingMultiDeviceStub.callCount).to.equal(members.length);
 
         const arg = sendUsingMultiDeviceStub.getCall(0).args;
@@ -342,12 +348,7 @@ describe('MessageQueue', () => {
           .resolves();
 
         const message = TestUtils.generateClosedGroupMessage();
-        const response = await messageQueueStub.sendToGroup(message);
+        await messageQueueStub.sendToGroup(message);
-
-        expect(response).to.equal(
-          false,
-          'sendToGroup sent a message to an empty group'
-        );
         expect(sendUsingMultiDeviceStub.callCount).to.equal(0);
       });
     });
@@ -365,9 +366,8 @@ describe('MessageQueue', () => {
 
       it('can send to open group', async () => {
         const message = TestUtils.generateOpenGroupMessage();
-        const success = await messageQueueStub.sendToGroup(message);
+        await messageQueueStub.sendToGroup(message);
         expect(sendToOpenGroupStub.callCount).to.equal(1);
-        expect(success).to.equal(true, 'Sending to open group failed');
       });
 
       it('should emit a success event when send was successful', async () => {