Introduced a protection which lets the room bans propagate to a defined banlist #223
Conversation
There was a problem hiding this comment.
@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)
src/protections/PropagateRoomBan.ts
Outdated
|
|
||
| export class PropagateRoomBan extends Protection { | ||
| settings = { | ||
| banlistShortcode: new StringProtectionSetting(), |
There was a problem hiding this comment.
internally we use BanList and banList so this should probably be banListShortcode
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
Probably we should default to the default list, but I think we should allow the admins to overwrite it.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
It's done in UnbanBanCommand.ts the same way. I've got the inspiration from there.
You mean to make some tests? I need to do a bit of research, but I could look into that. |
|
@gergof Any progress? |
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]