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

Introduced a protection which lets the room bans propagate to a defined banlist #223

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gergof
Copy link

@gergof gergof commented Feb 11, 2022

I made a protection which can be used to propagate room bans to a mjolnir banlist.

I needed this since there are some rooms which aren't only moderated by my homeserver's moderators and I don't want to invite a lot of strangers to the control room.

By enabling this protection it will be possible for a room admin to ban a user in a protected room and that ban will be published to a banlist, resulting the user being banned in the other rooms as well. This also adds a convenience factor as well. IMO it's easier to click on the user and hit ban than to search for the control room and ban the user from there.

Signed-off-by: Fándly Gergő-Zoltán [email protected]

@Yoric
Copy link
Contributor

Yoric commented Feb 14, 2022

I believe that it's a good idea. @jesopo , @Gnuxie what do you think?

@Gnuxie Gnuxie self-requested a review February 23, 2022 14:16
Copy link
Contributor

@Gnuxie Gnuxie left a comment

Choose a reason for hiding this comment

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

@gergof Thanks for taking the time to make this, it's a great idea 🏅

The suggested changes are required I think (This clashed with a refactor of logMessage from LogProxy, but it's no big deal).

I also have a question about how we handle the banListShortcode setting, I'm mostly worried about it becoming inconsistent/confused with the existing default ban list setting (for Mjolnir in general)


export class PropagateRoomBan extends Protection {
settings = {
banlistShortcode: new StringProtectionSetting(),
Copy link
Contributor

Choose a reason for hiding this comment

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

internally we use BanList and banList so this should probably be banListShortcode

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think that this should be tied to to the default list used by Mjolnir's own ban command, to avoid confusion?

(Like this https://github.com/matrix-org/mjolnir/blob/v1.3.2/src/commands/UnbanBanCommand.ts#L33-L42 )

Copy link
Author

Choose a reason for hiding this comment

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

Probably we should default to the default list, but I think we should allow the admins to overwrite it.

Copy link
Author

Choose a reason for hiding this comment

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

So I made a change where it defaults to the default banlist, but still allows it to be overwritten. Tell me what you think. The main reason I want it to be overwritable because I would like to separate the room admin bans from the server admin bans (since the latter have a higher trust level).

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should default to the default list, but I think we should allow the admins to overwrite it

That seems fair and the way you have done this works. 👍

src/protections/PropagateRoomBan.ts Outdated Show resolved Hide resolved
src/protections/PropagateRoomBan.ts Outdated Show resolved Hide resolved
src/protections/PropagateRoomBan.ts Outdated Show resolved Hide resolved
src/protections/PropagateRoomBan.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Gnuxie Gnuxie left a comment

Choose a reason for hiding this comment

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

Looks good

I regret not asking yesterday but do you think you could do that? (It is optional though, I'm not going to make you do it if you don't want to 😆 )

I think there will be examples in test/integration/ProtectionSettings.ts but of course it should be in its own file. Don't be afraid to ask here or in #mjolnir:matrix.org if you have questions`

);
banListShortcode = data['shortcode'];
} catch (e) {
await mjolnir.logMessage(
Copy link
Contributor

Choose a reason for hiding this comment

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

question: How come these are separate messages? Does the error make it look ugly or something? The only reason I ask is because I'm worried someone doesn't understand where the error in the second message comes from (but granted we might even do this elsewhere, so it is probably ok)

Copy link
Author

Choose a reason for hiding this comment

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

It's done in UnbanBanCommand.ts the same way. I've got the inspiration from there.

@gergof
Copy link
Author

gergof commented Mar 4, 2022

Looks good

I regret not asking yesterday but do you think you could do that? (It is optional though, I'm not going to make you do it if you don't want to laughing )

I think there will be examples in test/integration/ProtectionSettings.ts but of course it should be in its own file. Don't be afraid to ask here or in #mjolnir:matrix.org if you have questions`

You mean to make some tests? I need to do a bit of research, but I could look into that.

@Yoric
Copy link
Contributor

Yoric commented Apr 7, 2022

@gergof Any progress?

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.

3 participants