Fix issue where XMPP members would not get new XMPP presence

changelog.d/179.bugfix

@@ -0,0 +1 @@
+Fix issue where XMPP users would not be informed of other XMPP users joining

package.json

@@ -9,7 +9,6 @@
     "lint": "tslint --project tsconfig.json --format stylish",
     "start": "node build/src/Program.js -c config.yaml",
     "genreg": "node build/src/Program.js -r -c config.yaml",
-    "pretest": "npm run build",
     "test": "mocha -r ts-node/register test/test.ts test/*.ts test/**/*.ts",
     "changelog": "scripts/towncrier.sh",
     "coverage": "nyc mocha -r ts-node/register test/test.ts test/*.ts test/**/*.ts"

src/xmppjs/GatewayMUCMembership.ts

@@ -41,11 +41,17 @@ export class GatewayMUCMembership {
         return this.getMatrixMembers(chatName).find((user) => user.matrixId === matrixId);
     }
 
+    public getXmppMemberByDevice(chatName: string, realJid: string|JID): IGatewayMemberXmpp|undefined {
+        const j = typeof(realJid) !== "string" ? realJid.toString() : realJid;
+        const member = this.getXmppMembers(chatName).find((user) => user.devices.has(j));
+        return member;
+    }
+
     public getXmppMemberByRealJid(chatName: string, realJid: string|JID): IGatewayMemberXmpp|undefined {
         // Strip the resource.
         const j = typeof(realJid) === "string" ? jid(realJid) : realJid;
         const strippedJid = `${j.local}@${j.domain}`;
-        const member = this.getXmppMembers(chatName).find((user) => user.realJid!.toString() === strippedJid);
+        const member = this.getXmppMembers(chatName).find((user) => user.realJid.toString() === strippedJid);
         return member;
     }
 

src/xmppjs/XJSGateway.ts

@@ -10,7 +10,7 @@ import { PresenceCache } from "./PresenceCache";
 import { XHTMLIM } from "./XHTMLIM";
 import { BifrostRemoteUser } from "../store/BifrostRemoteUser";
 import { StzaPresenceItem, StzaMessage, StzaMessageSubject,
-    StzaPresenceError, StzaBase, StzaPresenceKick, PresenceAffiliation, PresenceRole } from "./Stanzas";
+    StzaPresenceError, StzaBase, StzaPresenceKick, PresenceAffiliation, PresenceRole, StzaPresenceJoin } from "./Stanzas";
 import { IGateway } from "../bifrost/Gateway";
 import { GatewayMUCMembership, IGatewayMemberXmpp, IGatewayMemberMatrix } from "./GatewayMUCMembership";
 import { XMPPStatusCode } from "./XMPPConstants";
@@ -71,13 +71,13 @@ export class XmppJsGateway implements IGateway {
             if (wasKicked && wasKicked.kicker) {
                 kicker = `${convName}/${wasKicked.kicker}`;
             }
-            const member = this.members.getXmppMemberByRealJid(convName, stanza.attrs.from);
-            this.remoteLeft(stanza);
+            const member = this.members.getXmppMemberByDevice(convName, stanza.attrs.from);
+            const lastDevice = this.remoteLeft(stanza);
             if (!member) {
                 log.warn("User has gone offline, but we don't have a member for them");
                 return;
             }
-            if (member.devices.size) {
+            if (!lastDevice) {
                 // User still has other devices, not leaving.
                 log.info(`User has ${member.devices.size} other devices, not leaving.`);
                 return;
@@ -177,19 +177,15 @@ export class XmppJsGateway implements IGateway {
             kick.statusCodes.add(XMPPStatusCode.SelfKickedTechnical);
             await this.xmpp.xmppSend(kick);
             return false;
-        } else if (!member) {
-            return false;
         }
         const preserveFrom = stanza.attrs.from;
         try {
             stanza.attrs.from = member!.anonymousJid;
-            const xmppMembers = this.members.getXmppMembers(chatName);
-            await Promise.all(xmppMembers.map((xmppUser) => {
-                [...xmppUser.devices!].map((device) => {
-                    stanza.attrs.to = device;
-                    this.xmpp.xmppWriteToStream(stanza);
-                });
-            }).flat());
+            const devices = this.members.getXmppMembersDevices(chatName);
+            for (const deviceJid of devices) {
+                stanza.attrs.to = deviceJid;
+                this.xmpp.xmppWriteToStream(stanza);
+            }
         } catch (err) {
             log.warn("Failed to reflect XMPP message:", err);
             stanza.attrs.from = preserveFrom;
@@ -386,7 +382,7 @@ export class XmppJsGateway implements IGateway {
         const selfPresence = new StzaPresenceItem(
             stanza.attrs.to,
             stanza.attrs.from,
-            undefined,
+            stanza.attrs.id,
             PresenceAffiliation.Member,
             PresenceRole.Participant,
             true,
@@ -396,18 +392,18 @@ export class XmppJsGateway implements IGateway {
         selfPresence.statusCodes.add(XMPPStatusCode.RoomNonAnonymous);
         selfPresence.statusCodes.add(XMPPStatusCode.RoomLoggingEnabled);
         await this.xmpp.xmppSend(selfPresence);
-        // Send everyone else the users new presence.
-        await this.reflectXMPPMessage(chatName, x("presence", {
-                from: stanza.attrs.from,
-                to: null,
-                id: stanza.attrs.id,
-            }, x("x", {
-                    xmlns: "http://jabber.org/protocol/muc#user",
-                }, [
-                    x("item", {affiliation: "member", role: "participant"}),
-                ]),
-        ), false);
 
+        // Send everyone else the users new presence.
+        const reflectedPresence = new StzaPresenceItem(
+            stanza.attrs.to,
+            "",
+            undefined,
+            PresenceAffiliation.Member,
+            PresenceRole.Participant,
+            false,
+            stanza.attrs.from,
+        );
+        this.reflectXMPPStanza(chatName, reflectedPresence);
         // FROM THIS POINT ON, WE CONSIDER THE USER JOINED.
 
         this.members.addXmppMember(
@@ -548,7 +544,7 @@ export class XmppJsGateway implements IGateway {
         const user = this.members.getXmppMemberByRealJid(chatName, stanza.attrs.from);
         if (!user) {
             log.error(`User tried to leave room, but they aren't in the member list`);
-            return;
+            return false;
         }
         const lastDevice = this.members.removeXmppMember(chatName, stanza.attrs.from);
         const leaveStza = new StzaPresenceItem(
@@ -567,6 +563,8 @@ export class XmppJsGateway implements IGateway {
         if (lastDevice) {
             leaveStza.self = false;
             this.reflectXMPPStanza(chatName, leaveStza);
+            return true;
         }
+        return false;
     }
 }

src/xmppjs/XJSInstance.ts

@@ -131,6 +131,12 @@ export class XmppJsInstance extends EventEmitter implements IBifrostInstance {
         return this.xmppSend(xml);
     }
 
+    /**
+     * Send an XML stanza to the stream. It's safe to modify
+     * the Stanza object after calling this, as the object
+     * is immediately converted to an XML string.
+     * @param xmlMsg The XML stanza or string to send
+     */
     public xmppSend(xmlMsg: IStza|string): Promise<unknown> {
         const xml = typeof(xmlMsg) === "string" ? xmlMsg : xmlMsg.xml;
         if (this.canWrite) {

test/mocks/XJSInstance.ts

@@ -1,4 +1,4 @@
-import { IStza, StzaIqPing, StzaPresenceJoin } from "../../src/xmppjs/Stanzas";
+import { IStza, StzaIqPing, StzaPresenceItem, StzaPresenceJoin } from "../../src/xmppjs/Stanzas";
 import { EventEmitter } from "events";
 import { x } from "@xmpp/xml";
 import { IChatJoined } from "../../src/bifrost/Events";
@@ -18,7 +18,14 @@ export class MockXJSInstance extends EventEmitter {
     }
 
     public xmppSend(msg: IStza) {
-        this.sentMessages.push(msg);
+        if (msg instanceof StzaPresenceItem) {
+            // Hack for tests, clone this.
+            const item = new StzaPresenceItem(msg.from, msg.to, msg.id, msg.affiliation, msg.role, msg.self, msg.jid, msg.presenceType);
+            msg.statusCodes.forEach(i => item.statusCodes.add(i));
+            this.sentMessages.push(item);
+        } else {
+            this.sentMessages.push(msg);
+        }
         if (msg instanceof StzaIqPing) {
             if (this.selfPingResponse === "respond-ok") {
                 this.emit("iq." + msg.id, x("iq"));

test/xmppjs/fixtures.ts

@@ -0,0 +1,12 @@
+import { jid } from "@xmpp/jid";
+
+export const XMPP_CHAT_NAME = "mychatname#matrix.org";
+export const XMPP_MEMBER_JID = jid("xmpp_bob", "xmpp.example.com", "myresource");
+export const XMPP_MEMBER_JID_STRIPPED = jid("xmpp_bob", "xmpp.example.com");
+export const XMPP_MEMBER_JID_SECOND_DEVICE = jid("xmpp_bob", "xmpp.example.com", "myresource2");
+export const XMPP_MEMBER_ANONYMOUS = jid(XMPP_CHAT_NAME, "xmpp.example.com", "bob");
+export const XMPP_MEMBER_MXID = "@_x_xmpp_bob:matrix.example.com";
+
+export const MATRIX_MEMBER_MXID = "@alice:matrix.example.com";
+export const MATRIX_MEMBER_ANONYMOUS = jid(XMPP_CHAT_NAME, "xmpp.example.com", "alice");
+export const MATRIX_ALIAS = "#mychatname:matrix.org"

test/xmppjs/test_GatewayMUCMembership.ts

@@ -1,16 +1,6 @@
-import { jid } from "@xmpp/jid";
 import { expect } from "chai";
 import { GatewayMUCMembership } from "../../src/xmppjs/GatewayMUCMembership";
-
-const CHAT_NAME = "mychatname";
-const XMPP_MEMBER_JID = jid("xmpp_bob", "xmpp.example.com", "myresource");
-const XMPP_MEMBER_JID_STRIPPED = jid("xmpp_bob", "xmpp.example.com");
-const XMPP_MEMBER_JID_SECOND_DEVICE = jid("xmpp_bob", "xmpp.example.com", "myresource2");
-const XMPP_MEMBER_ANONYMOUS = jid("mychatname", "xmpp.example.com", "bob");
-const XMPP_MEMBER_MXID = "@_x_xmpp_bob:matrix.example.com";
-
-const MATRIX_MEMBER_MXID = "@alice:matrix.example.com";
-const MATRIX_MEMBER_ANONYMOUS = jid("mychatname", "xmpp.example.com", "alice");
+import { XMPP_CHAT_NAME, MATRIX_MEMBER_ANONYMOUS, MATRIX_MEMBER_MXID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_JID, XMPP_MEMBER_JID_SECOND_DEVICE, XMPP_MEMBER_JID_STRIPPED, XMPP_MEMBER_MXID } from "./fixtures";
 
 describe("GatewayMUCMembership", () => {
     let members: GatewayMUCMembership;
@@ -19,77 +9,77 @@ describe("GatewayMUCMembership", () => {
     })
     describe("adding members", () => {
         it("can add a XMPP member", () => {
-            const firstDevice = members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            const firstDevice = members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
             expect(firstDevice).to.be.true;
         });
         it("can add another device for the same XMPP member", () => {
-            members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
-            const firstDevice = members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID_SECOND_DEVICE, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            const firstDevice = members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID_SECOND_DEVICE, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
             expect(firstDevice).to.be.false;
         });
         it("can add a Matrix member", () => {
-            const firstDevice = members.addMatrixMember(CHAT_NAME, MATRIX_MEMBER_MXID, MATRIX_MEMBER_ANONYMOUS);
+            const firstDevice = members.addMatrixMember(XMPP_CHAT_NAME, MATRIX_MEMBER_MXID, XMPP_MEMBER_ANONYMOUS);
             expect(firstDevice).to.be.true;
         });
         it("can add another device for the same Matrix member", () => {
-            members.addMatrixMember(CHAT_NAME, MATRIX_MEMBER_MXID, MATRIX_MEMBER_ANONYMOUS);
-            const firstDevice = members.addMatrixMember(CHAT_NAME, MATRIX_MEMBER_MXID, MATRIX_MEMBER_ANONYMOUS);
+            members.addMatrixMember(XMPP_CHAT_NAME, MATRIX_MEMBER_MXID, MATRIX_MEMBER_ANONYMOUS);
+            const firstDevice = members.addMatrixMember(XMPP_CHAT_NAME, MATRIX_MEMBER_MXID, MATRIX_MEMBER_ANONYMOUS);
             expect(firstDevice).to.be.false;
         });
     });
     describe("removing members", () => {
         it("can remove a XMPP member", () => {
-            members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
-            const lastDevice = members.removeXmppMember(CHAT_NAME, XMPP_MEMBER_JID.toString());
+            members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            const lastDevice = members.removeXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID.toString());
             expect(lastDevice).to.be.true;
         });
         it("can add two devices, and remove one", () => {
-            members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
-            members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID_SECOND_DEVICE, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
-            let lastDevice = members.removeXmppMember(CHAT_NAME, XMPP_MEMBER_JID.toString());
+            members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID_SECOND_DEVICE, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            let lastDevice = members.removeXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID.toString());
             expect(lastDevice).to.be.false;
-            lastDevice = members.removeXmppMember(CHAT_NAME, XMPP_MEMBER_JID_SECOND_DEVICE.toString());
+            lastDevice = members.removeXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID_SECOND_DEVICE.toString());
             expect(lastDevice).to.be.true;
         });
         it("can add two devices, and remove all if the JID is stripped", () => {
-            members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
-            members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID_SECOND_DEVICE, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
-            const lastDevice = members.removeXmppMember(CHAT_NAME, XMPP_MEMBER_JID_STRIPPED);
+            members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID_SECOND_DEVICE, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            const lastDevice = members.removeXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID_STRIPPED);
             expect(lastDevice).to.be.true;
         });
         it("can remove a Matrix member", () => {
-            members.addMatrixMember(CHAT_NAME, MATRIX_MEMBER_MXID, MATRIX_MEMBER_ANONYMOUS);
-            const removed = members.removeMatrixMember(CHAT_NAME, MATRIX_MEMBER_MXID);
+            members.addMatrixMember(XMPP_CHAT_NAME, MATRIX_MEMBER_MXID, MATRIX_MEMBER_ANONYMOUS);
+            const removed = members.removeMatrixMember(XMPP_CHAT_NAME, MATRIX_MEMBER_MXID);
             expect(removed).to.be.true;
         });
         it("will return false if the Matrix member was not removed", () => {
-            const removed = members.removeMatrixMember(CHAT_NAME, MATRIX_MEMBER_MXID);
+            const removed = members.removeMatrixMember(XMPP_CHAT_NAME, MATRIX_MEMBER_MXID);
             expect(removed).to.be.false;
         });
 
     });
     describe("finding members", () => {
         it("can find an XMPP member by real JID", () => {
-            members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
-            const member = members.getXmppMemberByRealJid(CHAT_NAME, XMPP_MEMBER_JID);
+            members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            const member = members.getXmppMemberByRealJid(XMPP_CHAT_NAME, XMPP_MEMBER_JID);
             expect(member?.realJid.toString()).to.be.equal(XMPP_MEMBER_JID_STRIPPED.toString());
             expect(member?.anonymousJid.toString()).to.be.equal(XMPP_MEMBER_ANONYMOUS.toString());
             expect(member?.matrixId).to.be.equal(XMPP_MEMBER_MXID);
             expect(member?.devices.size).to.equal(1);
             expect(member?.devices.values().next().value).to.equal(XMPP_MEMBER_JID.toString())
         });
         it("can find an XMPP member by real JID", () => {
-            members.addXmppMember(CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
-            const member = members.getXmppMemberByMatrixId(CHAT_NAME, XMPP_MEMBER_MXID);
+            members.addXmppMember(XMPP_CHAT_NAME, XMPP_MEMBER_JID, XMPP_MEMBER_ANONYMOUS, XMPP_MEMBER_MXID);
+            const member = members.getXmppMemberByMatrixId(XMPP_CHAT_NAME, XMPP_MEMBER_MXID);
             expect(member?.realJid.toString()).to.be.equal(XMPP_MEMBER_JID_STRIPPED.toString());
             expect(member?.anonymousJid.toString()).to.be.equal(XMPP_MEMBER_ANONYMOUS.toString());
             expect(member?.matrixId).to.be.equal(XMPP_MEMBER_MXID);
             expect(member?.devices.size).to.equal(1);
             expect(member?.devices.values().next().value).to.equal(XMPP_MEMBER_JID.toString())
         });
         it("can find a Matrix member by mxId", () => {
-            members.addMatrixMember(CHAT_NAME, MATRIX_MEMBER_MXID, MATRIX_MEMBER_ANONYMOUS);
-            const member = members.getMatrixMemberByMatrixId(CHAT_NAME, MATRIX_MEMBER_MXID);
+            members.addMatrixMember(XMPP_CHAT_NAME, MATRIX_MEMBER_MXID, MATRIX_MEMBER_ANONYMOUS);
+            const member = members.getMatrixMemberByMatrixId(XMPP_CHAT_NAME, MATRIX_MEMBER_MXID);
             expect(member?.type).to.equal("matrix");
             expect(member?.anonymousJid.toString()).to.be.equal(MATRIX_MEMBER_ANONYMOUS.toString());
             expect(member?.matrixId).to.be.equal(MATRIX_MEMBER_MXID);