-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for ban syncing #1821
base: develop
Are you sure you want to change the base?
Support for ban syncing #1821
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Initial support for bridging bans to IRC. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1254,18 +1254,20 @@ export class IrcBridge { | |
else if (event.content.membership === "join") { | ||
await this.matrixHandler.onJoin(request, memberEvent as unknown as OnMemberEventData, target); | ||
} | ||
else if (["ban", "leave"].includes(event.content.membership as string)) { | ||
// Given a "self-kick" is a leave, and you can't ban yourself, | ||
// if the 2 IDs are different then we know it is either a kick | ||
// or a ban (or a rescinded invite) | ||
const isKickOrBan = target.getId() !== sender.getId(); | ||
if (isKickOrBan) { | ||
else if (event.content.membership === "leave") { | ||
// Given a "self-kick" is a leave, if the 2 IDs are different then | ||
// we know it is a kick (or a rescinded invite) | ||
const isKick = target.getId() !== sender.getId(); | ||
if (isKick) { | ||
await this.matrixHandler.onKick(request, memberEvent as unknown as MatrixEventKick, sender, target); | ||
} | ||
else { | ||
await this.matrixHandler.onLeave(request, memberEvent, target); | ||
} | ||
} | ||
else if (event.content.membership === "ban") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do unban? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not implemented because I didn't wrap my head on how unbanning works on Matrix and how to check that. |
||
await this.matrixHandler.onBan(request, memberEvent as unknown as MatrixEventKick, sender, target); | ||
} | ||
} | ||
else if (event.type === "m.room.power_levels" && event.state_key === "") { | ||
this.ircHandler.roomAccessSyncer.onMatrixPowerlevelEvent(event); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -652,7 +652,7 @@ export class MatrixHandler { | |
|
||
private async _onKick(req: BridgeRequest, event: MatrixEventKick, kicker: MatrixUser, kickee: MatrixUser) { | ||
req.log.info( | ||
"onKick %s is kicking/banning %s from %s (reason: %s)", | ||
"onKick %s is kicking %s from %s (reason: %s)", | ||
kicker.getId(), kickee.getId(), event.room_id, event.content.reason || "none" | ||
); | ||
this._onMemberEvent(req, event); | ||
|
@@ -741,7 +741,81 @@ export class MatrixHandler { | |
// If we aren't joined this will no-op. | ||
await client.leaveChannel( | ||
ircRoom.channel, | ||
`Kicked by ${kicker.getId()} ` + | ||
`Kicked by ${kicker.getId()}` + | ||
(event.content.reason ? ` : ${event.content.reason}` : "") | ||
); | ||
}))); | ||
} | ||
} | ||
|
||
private async _onBan(req: BridgeRequest, event: MatrixEventKick, sender: MatrixUser, banned: MatrixUser) { | ||
req.log.info( | ||
"onBan %s is banning %s from %s (reason: %s)", | ||
sender.getId(), banned.getId(), event.room_id, event.content.reason || "none" | ||
); | ||
this._onMemberEvent(req, event); | ||
|
||
const ircRooms = await this.ircBridge.getStore().getIrcChannelsForRoomId(event.room_id); | ||
// do we have an active connection for the banned? This tells us if they are real | ||
// or virtual. | ||
const bannedClients = this.ircBridge.getBridgedClientsForUserId(banned.getId()); | ||
|
||
if (bannedClients.length === 0) { | ||
// Matrix on IRC banning, work out which IRC user to ban. | ||
let server = null; | ||
for (let i = 0; i < ircRooms.length; i++) { | ||
if (ircRooms[i].server.claimsUserId(banned.getId())) { | ||
server = ircRooms[i].server; | ||
break; | ||
} | ||
} | ||
if (!server) { | ||
return; // kicking a bogus user | ||
} | ||
const bannedNick = server.getNickFromUserId(banned.getId()); | ||
if (!bannedNick) { | ||
return; // bogus virtual user ID | ||
} | ||
// work out which client will do the kicking | ||
const senderClient = this.ircBridge.getIrcUserFromCache(server, sender.getId()); | ||
if (!senderClient) { | ||
// well this is awkward.. whine about it and bail. | ||
req.log.warn( | ||
"%s has no client instance to send kick from. Cannot kick.", | ||
sender.getId() | ||
); | ||
return; | ||
} | ||
// we may be bridging this matrix room into many different IRC channels, and we want | ||
// to kick this user from all of them. | ||
for (let i = 0; i < ircRooms.length; i++) { | ||
if (ircRooms[i].server.domain !== server.domain) { | ||
return; | ||
} | ||
senderClient.ban(bannedNick, ircRooms[i].channel); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to await either of these? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because you're doing two operations here, and I'm not sure if one should proceed if the other fails? At the very least, run them both concurrently but await the result so we can throw an error. Node typically crashes the process these days if a unawaited promise throws. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but sure I'll look into that |
||
senderClient.kick( | ||
bannedNick, ircRooms[i].channel, | ||
`Banned by ${sender.getId()}` + | ||
(event.content.reason ? ` : ${event.content.reason}` : "") | ||
); | ||
} | ||
} | ||
else { | ||
// Matrix on Matrix banning: part the channel. | ||
const bannedServerLookup: {[serverDomain: string]: BridgedClient} = {}; | ||
bannedClients.forEach((ircClient) => { | ||
bannedServerLookup[ircClient.server.domain] = ircClient; | ||
}); | ||
await Promise.all(ircRooms.map((async (ircRoom) => { | ||
// Make the connected IRC client leave the channel. | ||
const client = bannedServerLookup[ircRoom.server.domain]; | ||
if (!client) { | ||
return; // not connected to this server | ||
} | ||
// If we aren't joined this will no-op. | ||
await client.leaveChannel( | ||
ircRoom.channel, | ||
`Banned by ${sender.getId()}` + | ||
(event.content.reason ? ` : ${event.content.reason}` : "") | ||
); | ||
}))); | ||
|
@@ -1464,6 +1538,10 @@ export class MatrixHandler { | |
return reqHandler(req, this._onKick(req, event, kicker, kickee)); | ||
} | ||
|
||
public onBan(req: BridgeRequest, event: MatrixEventKick, sender: MatrixUser, banned: MatrixUser) { | ||
return reqHandler(req, this._onBan(req, event, sender, banned)); | ||
} | ||
|
||
public onMessage(req: BridgeRequest, event: MatrixMessageEvent) { | ||
return reqHandler(req, this._onMessage(req, event)); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -510,6 +510,26 @@ export class BridgedClient extends EventEmitter { | |
await c.send("KICK", channel, nick, reason); | ||
} | ||
|
||
public async ban(nick: string, channel: string): Promise<void> { | ||
if (this.state.status !== BridgedClientStatus.CONNECTED) { | ||
return; // we were never connected to the network. | ||
} | ||
if (!this.state.client.chans.has(channel)) { | ||
// we were never joined to it. We need to be joined to it to kick people. | ||
return; | ||
} | ||
if (!channel.startsWith("#")) { | ||
return; // PM room | ||
} | ||
|
||
const c = this.state.client; | ||
|
||
this.log.debug("Banning %s from channel %s", nick, channel); | ||
|
||
// best effort ban | ||
await c.send("MODE", channel, "+b", nick + "!*@*"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm always a bit skeptical about these. I presume we're sure this is reasonably implementation independent. We don't want to offer a ban command config option or anything like that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would this be implementation dependent? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd love to know how we could offer ban config when banning from Matrix. |
||
} | ||
|
||
public sendAction(room: IrcRoom, action: IrcAction) { | ||
this.keepAlive(); | ||
let expiryTs = 0; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file says kicking
test says banning
new test file time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming
kicking.spec.js
tokicking-banning.spec.js
? I think they go well in one file.