Skip to content

Commit

Permalink
Merge pull request #179 from matrix-org/hs/fix-non-reflective-membership
Browse files Browse the repository at this point in the history 
Fix issue where XMPP members would not get new XMPP presence
  • Loading branch information
@Half-Shot
Half-Shot authored Oct 22, 2020
2 parents cf76d8e + 11b9bd4 commit 9763044
Show file tree
Hide file tree
Showing 10 changed files with 176 additions and 95 deletions.
1 change: 1 addition & 0 deletions changelog.d/179.bugfix
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix issue where XMPP users would not be informed of other XMPP users joining
1 change: 0 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
8 changes: 7 additions & 1 deletion src/xmppjs/GatewayMUCMembership.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
50 changes: 24 additions & 26 deletions src/xmppjs/XJSGateway.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand All @@ -567,6 +563,8 @@ export class XmppJsGateway implements IGateway {
if (lastDevice) {
leaveStza.self = false;
this.reflectXMPPStanza(chatName, leaveStza);
return true;
}
return false;
}
}
6 changes: 6 additions & 0 deletions src/xmppjs/XJSInstance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 9 additions & 2 deletions test/mocks/XJSInstance.ts
View file
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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"));
Expand Down
12 changes: 12 additions & 0 deletions test/xmppjs/fixtures.ts
View file
Original file line number Diff line number Diff line change
@@ -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"
60 changes: 25 additions & 35 deletions test/xmppjs/test_GatewayMUCMembership.ts
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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);
Expand Down
Loading

0 comments on commit 9763044

Please sign in to comment.