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

Rework kick command for ACL clean+glob kick. #315

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

Conversation

Gnuxie
Copy link
Contributor

@Gnuxie Gnuxie commented Jul 1, 2022

This PR adds a command !mjolnir acl-clean which kicks all of the users from the room or all protected rooms that whose servers are banned by the room's server ACL. It has also reworked the kick / glob kick command a little to allow this to happen.

#279

@Gnuxie Gnuxie requested review from Yoric and jesopo July 1, 2022 10:34
Copy link
Contributor

@Yoric Yoric left a comment

Choose a reason for hiding this comment

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

Sorry, I don't understand well enough the objective of this command to be able to review it yet.

config/harness.yaml Show resolved Hide resolved
@@ -132,6 +134,7 @@ export async function handleCommand(roomId: string, event: { content: { body: st
"!mjolnir redact <user ID> [room alias/ID] [limit] - Redacts messages by the sender in the target room (or all rooms), up to a maximum number of events in the backlog (default 1000)\n" +
"!mjolnir redact <event permalink> - Redacts a message by permalink\n" +
"!mjolnir kick <glob> [room alias/ID] [reason] - Kicks a user or all of those matching a glob in a particular room or all protected rooms\n" +
"!mjolnir acl-clean [room alias/ID] - Kicks all of the users from the room or all protected rooms that whose servers are banned by the room's server ACL.\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't understand that docline.

Copy link
Contributor Author

@Gnuxie Gnuxie Jul 5, 2022

Choose a reason for hiding this comment

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

I have reworded it a little, though I need help to make it better

The command takes a room's m.room.server_acl event and applies it to each user in the room following the algorithm described here

}
return [...Object.keys(mjolnir.protectedRooms)];
})();
const reason = (rooms.length === 1 ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: One of these days, we should really implement proper argument parsing.

rooms = [await mjolnir.client.resolveRoom(parts[3])];
reasonIndex = 4;
/// Instead of having --force on commands like these were confirmation is required after some context,
/// wouldn't it be better if we showed what would happen and then ask yes/no to confirm?
Copy link
Contributor

Choose a reason for hiding this comment

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

definitely. i assume we need to cache state for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, we just need to figure out how to split large messages up. This is already implemented in a way where it knows who all the members are before it kicks them

Copy link
Contributor

Choose a reason for hiding this comment

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

That would definitely be better UX. How much work is that?

@Gnuxie Gnuxie requested review from Yoric and jesopo July 26, 2022 15:40
@@ -132,6 +134,7 @@ export async function handleCommand(roomId: string, event: { content: { body: st
"!mjolnir redact <user ID> [room alias/ID] [limit] - Redacts messages by the sender in the target room (or all rooms), up to a maximum number of events in the backlog (default 1000)\n" +
"!mjolnir redact <event permalink> - Redacts a message by permalink\n" +
"!mjolnir kick <glob> [room alias/ID] [reason] - Kicks a user or all of those matching a glob in a particular room or all protected rooms\n" +
"!mjolnir acl-clean [room alias/ID] - Kicks all of the users from a room or all protected rooms (when no arguments are given) whose servers are banned by the room's server ACL event.\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we as for a special string * for "all protected rooms"? This feels safer.

return [...Object.keys(mjolnir.protectedRooms)];
})();
const reason = (rooms.length === 1 ?
// we don't forget to remove the `--force` argument from the reason.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think that "we" makes this comment harder to parse.

membership => {
const memberId = new UserID(membership.membershipFor);
// check the user's server isn't on the deny list.
for (const deny of currentAcl.safeAclContent().deny) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of performance, shouldn't we rather have an outerloop on currentAcl.safeAclContent() and an inner loop on individual users? Feels like we would spend considerably less time building and rebuilding regexps.

rooms = [await mjolnir.client.resolveRoom(parts[3])];
reasonIndex = 4;
/// Instead of having --force on commands like these were confirmation is required after some context,
/// wouldn't it be better if we showed what would happen and then ask yes/no to confirm?
Copy link
Contributor

Choose a reason for hiding this comment

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

That would definitely be better UX. How much work is that?

await mjolnir.logMessage(LogLevel.WARN, "KickCommand", `Tried to kick ${victim} in ${protectedRoomId} but the bot is running in no-op mode.`, protectedRoomId);
}
async function kickMembers(mjolnir: Mjolnir, roomId: string, membersToKick: string[], force: boolean, reason: string) {
// I do not think it makes much sense to notify who was kicked like this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, for auditing purposes, I think that it does. Could be a single message, though, instead of one message per user.

const ips = acl['allow_ip_literals'];

if (Array.isArray(allow)) {
allow.forEach(this.allowServer, this);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't trust allow, we should validate that each value in this array is a string. Same for deny.

if (Array.isArray(deny)) {
deny.forEach(this.denyServer, this);
}
if (Boolean(ips)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I believe that !!ips would be more idiomatic.

afterEach(function() { this.moderator?.stop(); });

it("Kicks users matching ACL", async function () {
// How tf
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand. Did you forget to implement this test?


it("Kicks users matching ACL", async function () {
// How tf
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: ; here and in a few other places.

// create a bunch of users with a pattern in the name.
const usersToRemove = await Promise.all([...Array(20)].map(_ => newTestUser({ name: { contains: "remove-me"}})));
const usersToKeep = await Promise.all([...Array(20)].map(_ => newTestUser({ name: { contains: "keep-me"}})));
// FIXME: Does our kick command kick from all protected rooms or just one room???
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand. Did you forget to remove this comment?

@Gnuxie
Copy link
Contributor Author

Gnuxie commented Oct 17, 2022

Waiting for #378

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