Support for ban syncing #1821

tadzik wants to merge 3 commits into develop from 1807-split/ban-syncing
tadzik
tadzik312 days ago

Split off from #1807, credit to @funderscore1

Initial support for bridging Matrix bans to IRC
ac7311df
spec/integ/kicking: add ban tests
ce67a19a
tadzik tadzik requested a review 312 days ago
tadzik changelog
b2add730
tadzik
tadzik235 days ago

Looks good to me, and I've heard it's been successfully running on pixie.town for a while.

Half-Shot
Half-Shot requested changes on 2024-11-28
Half-Shot235 days ago

This is fine, but this isn't ban syncing. This needs to handle the case of unbanning too, or at the very least some technical documentation to explain why we don't do that.

And actually I'd love some documentation on our docs site on what users can expect from kicks / bans.

spec/integ/kicking.spec.js
176176});
177177
178178
179
describe("Banning", () => {
Half-Shot235 days ago

file says kicking
test says banning

new test file time?

funderscore1232 days ago

How about renaming kicking.spec.js to kicking-banning.spec.js? I think they go well in one file.

src/bridge/MatrixHandler.ts
792 if (ircRooms[i].server.domain !== server.domain) {
793 return;
794 }
795
senderClient.ban(bannedNick, ircRooms[i].channel);
Half-Shot235 days ago

Do we need to await either of these?

funderscore1235 days ago

Why?

Half-Shot234 days ago

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.

funderscore1232 days ago

I think if +b fails (not sure why it would) and /kick works that's better than if it fails and /kick isn't ran because of that.

funderscore1232 days ago

but sure I'll look into that

src/irc/BridgedClient.ts
527 this.log.debug("Banning %s from channel %s", nick, channel);
528
529 // best effort ban
530
await c.send("MODE", channel, "+b", nick + "!*@*");
Half-Shot235 days ago

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?

funderscore1235 days ago

Why would this be implementation dependent?

funderscore1235 days ago

I'd love to know how we could offer ban config when banning from Matrix.

src/bridge/IrcBridge.ts
12661265 await this.matrixHandler.onLeave(request, memberEvent, target);
12671266 }
12681267 }
1268
else if (event.content.membership === "ban") {
Half-Shot235 days ago

How do unban?

funderscore1235 days ago

Not implemented because I didn't wrap my head on how unbanning works on Matrix and how to check that.

f0x52
f0x52235 days ago👍 1

I think this could nicely be supplemented with an !irc ban <mask> command which sets +b modes, to allow for more flexible matching on nick/user/host parts.

Afaik removing a ban on the matrix side consists of the user's m.room.member state event changing to anything that isn't ban, so you could check the prev_content key in state update events to unset the mode

funderscore1
funderscore1232 days ago

I think this could nicely be supplemented with an !irc ban <mask> command which sets +b modes, to allow for more flexible matching on nick/user/host parts.

Afaik removing a ban on the matrix side consists of the user's m.room.member state event changing to anything that isn't ban, so you could check the prev_content key in state update events to unset the mode

@f0x52 I'll look into that, thanks

Login to write a write a comment.

Login via GitHub

Assignees
No one assigned
Labels
Milestone