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

Gnuxie · 2022-07-01T10:34:24Z

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

#279 This isn't going to be the end, i don't like it.

Yoric

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

config/harness.yaml

Yoric · 2022-07-05T06:49:33Z

src/commands/CommandHandler.ts

@@ -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" +


Sorry, I don't understand that docline.

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

Yoric · 2022-07-05T06:51:23Z

src/commands/KickCommand.ts

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


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

jesopo · 2022-07-13T16:03:44Z

src/commands/KickCommand.ts

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


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

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

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

Yoric · 2022-09-05T09:13:21Z

src/commands/CommandHandler.ts

@@ -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" +


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

Yoric · 2022-09-05T09:19:57Z

src/commands/KickCommand.ts

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


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

Yoric · 2022-09-05T10:26:04Z

src/commands/KickCommand.ts

+            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) {


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.

Yoric · 2022-09-05T10:26:46Z

src/commands/KickCommand.ts

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


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

Yoric · 2022-09-05T10:28:40Z

src/commands/KickCommand.ts

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


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

Yoric · 2022-09-06T07:12:44Z

src/models/ServerAcl.ts

+        const ips = acl['allow_ip_literals'];
+
+        if (Array.isArray(allow)) {
+            allow.forEach(this.allowServer, this);


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

Yoric · 2022-09-06T07:13:31Z

src/models/ServerAcl.ts

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


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

Yoric · 2022-09-06T07:13:53Z

test/integration/commands/kickCommandTest.ts

+    afterEach(function() { this.moderator?.stop(); });
+
+    it("Kicks users matching ACL", async function () {
+        // How tf


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

Yoric · 2022-09-06T07:14:08Z

test/integration/commands/kickCommandTest.ts

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


Nit: ; here and in a few other places.

Yoric · 2022-09-06T07:15:01Z

test/integration/commands/kickCommandTest.ts

+        // 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???


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

Gnuxie · 2022-10-17T18:09:42Z

Waiting for #378

Gnuxie added 2 commits July 1, 2022 11:11

Rework kick command for ACL clean+glob kick.

7cb7573

#279 This isn't going to be the end, i don't like it.

oops forgot the command handler

bf73249

Gnuxie requested review from Yoric and jesopo July 1, 2022 10:34

Yoric reviewed Jul 5, 2022

View reviewed changes

improve doc

03602b7

jesopo reviewed Jul 13, 2022

View reviewed changes

Gnuxie requested review from Yoric and jesopo July 26, 2022 15:40

Yoric reviewed Sep 6, 2022

View reviewed changes

Gnuxie mentioned this pull request Aug 29, 2023

Incorperate: Rework kick command for ACL clean+glob kick. the-draupnir-project/Draupnir#82

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Gnuxie commented Jul 1, 2022

Yoric left a comment

Yoric Jul 5, 2022

Gnuxie Jul 5, 2022 •

edited

Yoric Jul 5, 2022

jesopo Jul 13, 2022

Gnuxie Jul 20, 2022

Yoric Sep 5, 2022

Yoric Sep 5, 2022

Yoric Sep 5, 2022

Yoric Sep 5, 2022

Yoric Sep 5, 2022

Yoric Sep 5, 2022

Yoric Sep 6, 2022

Yoric Sep 6, 2022

Yoric Sep 6, 2022

Yoric Sep 6, 2022

Yoric Sep 6, 2022

Gnuxie commented Oct 17, 2022

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

Are you sure you want to change the base?

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

Conversation

Gnuxie commented Jul 1, 2022

Yoric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gnuxie Jul 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gnuxie commented Oct 17, 2022

Gnuxie Jul 5, 2022 •

edited