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

Don't ban users in moderator room #544

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

Don't ban users in moderator room #544

wants to merge 11 commits into from

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Oct 9, 2024

Prevents the bot from banning itself/other users in the moderator room. Also adds a command to add others who are not in room to list of entities that shouldn't be banned/ACL'd.

Reviewable commit-by-commit.

@H-Shay H-Shay requested a review from turt2live October 9, 2024 21:40
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise generally lgtm. Tests would also be good, if possible

src/Mjolnir.ts Outdated
/**
* Members of the moderator room and others who should not be banned, ACL'd etc.
*/
public moderators: string[];
Copy link
Member

Choose a reason for hiding this comment

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

Users who are invited/joined after the bot starts up would get banned. Adding a cache would help ensure we don't miss anyone. We'd invalidate the cache on two conditions I think:

  1. Every 60 minutes.
  2. Whenever we see an m.room.member event in the management room. (we don't have to inspect the event: seeing it fly by should be enough to invalidate&repopulate)

For deployments like matrix.org this may mean we pick up on displayname changes quite often, but I'd consider that a feature for keeping the cache active.

src/Mjolnir.ts Outdated Show resolved Hide resolved
src/commands/CommandHandler.ts Outdated Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Thanks! Let's give it a go.

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.

2 participants