Skip to content
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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Support for ban syncing #1821

wants to merge 3 commits into from

Conversation

tadzik
Copy link
Contributor

@tadzik tadzik commented Sep 12, 2024

Split off from #1807, credit to @funderscore1

Ferass El Hafidi added 2 commits September 12, 2024 11:56
The bridge used to *not* bridge Matrix bans to IRC. As such, when
someone banned an IRC user from Matrix, it would only prevent Matrix
users from seeing messages coming from said user, but would not ban that
user from IRC at all. This resulted in Matrix channel moderators being
confused when other IRC users are reporting spam that simply isn't
bridged at all as a result.

This commit adds support for bridging Matrix bans to IRC. Currently, it
just bans on IRC based on the IRC user's nickname, but this could change
in the future, and most importantly is better than not bridging the
ban at all.

Signed-off-by: Ferass El Hafidi <[email protected]>
Signed-off-by: Ferass El Hafidi <[email protected]>
@tadzik tadzik requested a review from a team as a code owner September 12, 2024 09:59
Signed-off-by: Ferass El Hafidi <[email protected]>
@tadzik
Copy link
Contributor Author

tadzik commented Nov 28, 2024

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

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -176,6 +176,118 @@ describe("Kicking", () => {
});


describe("Banning", () => {
Copy link
Contributor

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?

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 to kicking-banning.spec.js? I think they go well in one file.

if (ircRooms[i].server.domain !== server.domain) {
return;
}
senderClient.ban(bannedNick, ircRooms[i].channel);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to await either of these?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but sure I'll look into that

this.log.debug("Banning %s from channel %s", nick, channel);

// best effort ban
await c.send("MODE", channel, "+b", nick + "!*@*");
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this be implementation dependent?

Choose a reason for hiding this comment

The 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.

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") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do unban?

Choose a reason for hiding this comment

The 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.

@f0x52
Copy link
Contributor

f0x52 commented Nov 28, 2024

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants