Test more edge cases in unban command (#743)

* Cover users that are banned indirectly by removed policies

https://github.com/the-draupnir-project/Draupnir/issues/740.

* Move findUnbanInformationForMember to UnbanUsers.tsx

* Remove rules on unban even if there are no matching members.

Fixes https://github.com/the-draupnir-project/Draupnir/issues/742.

* Add even more test cases to unban test.

Fixes #740 
Fixes #742

src/commands/unban/Unban.tsx

@@ -22,7 +22,6 @@ import {
   WatchedPolicyRooms,
 } from "matrix-protection-suite";
 import {
-  MatrixGlob,
   MatrixRoomID,
   MatrixUserID,
   StringUserID,
@@ -44,11 +43,7 @@ import {
 } from "../DraupnirCommandPrerequisites";
 import ManagementRoomOutput from "../../managementroom/ManagementRoomOutput";
 import { UnlistedUserRedactionQueue } from "../../queues/UnlistedUserRedactionQueue";
-import {
-  findMembersMatchingGlob,
-  findBanPoliciesMatchingUsers,
-  unbanMembers,
-} from "./UnbanUsers";
+import { findUnbanInformationForMember, unbanMembers } from "./UnbanUsers";
 import { findPoliciesToRemove, unbanEntity } from "./UnbanEntity";
 import { ListMatches, renderListRules } from "../Rules";
 import { renderRoomSetResult } from "../../capabilities/CommonRenderers";
@@ -219,29 +214,6 @@ DraupnirContextToCommandContextTranslator.registerTranslation(
   }
 );
 
-export function findUnbanInformationForMember(
-  setRoomMembership: SetRoomMembership,
-  entity: MatrixUserID,
-  watchedPolicyRooms: WatchedPolicyRooms,
-  { inviteMembers }: { inviteMembers: boolean }
-): UnbanMembersPreview {
-  const membersToUnban = findMembersMatchingGlob(
-    setRoomMembership,
-    new MatrixGlob(entity.toString()),
-    { inviteMembers }
-  );
-  const policyMatchesToRemove = findBanPoliciesMatchingUsers(
-    watchedPolicyRooms,
-    membersToUnban.map((memberRooms) => memberRooms.member)
-  );
-  const unbanInformation = {
-    policyMatchesToRemove,
-    membersToUnban,
-    entity,
-  };
-  return unbanInformation;
-}
-
 function renderPoliciesToRemove(policyMatches: ListMatches[]): DocumentNode {
   return (
     <fragment>

src/commands/unban/UnbanUsers.tsx

@@ -9,6 +9,8 @@ import {
   MatrixGlob,
   StringRoomID,
   userServerName,
+  isStringUserID,
+  MatrixUserID,
 } from "@the-draupnir-project/matrix-basic-types";
 import {
   SetRoomMembership,
@@ -30,9 +32,12 @@ import { UnlistedUserRedactionQueue } from "../../queues/UnlistedUserRedactionQu
 import { ListMatches } from "../Rules";
 import { MemberRooms, UnbanMembersPreview, UnbanMembersResult } from "./Unban";
 
-export function findMembersMatchingGlob(
+export function matchMembers(
   setRoomMembership: SetRoomMembership,
-  glob: MatrixGlob,
+  matches: {
+    globs: MatrixGlob[];
+    literals: StringUserID[];
+  },
   options: { inviteMembers: boolean }
 ): MemberRooms[] {
   const map = new Map<StringUserID, MemberRooms>();
@@ -74,13 +79,17 @@ export function findMembersMatchingGlob(
   };
   for (const revision of setRoomMembership.allRooms) {
     for (const membership of revision.members()) {
-      if (glob.test(membership.userID)) {
+      if (
+        matches.literals.includes(membership.userID) ||
+        matches.globs.some((glob) => glob.test(membership.userID))
+      ) {
         addRoomMembership(membership, revision.room);
       }
     }
   }
   return [...map.values()];
 }
+
 export function findBanPoliciesMatchingUsers(
   watchedPolicyRooms: WatchedPolicyRooms,
   users: StringUserID[]
@@ -196,3 +205,83 @@ export async function unbanMembers(
     usersInvited: invitationsSent.getResult(),
   });
 }
+
+export function findUnbanInformationForMember(
+  setRoomMembership: SetRoomMembership,
+  entity: MatrixUserID,
+  watchedPolicyRooms: WatchedPolicyRooms,
+  { inviteMembers }: { inviteMembers: boolean }
+): UnbanMembersPreview {
+  const membersMatchingEntity = matchMembers(
+    setRoomMembership,
+    {
+      ...(entity.isContainingGlobCharacters()
+        ? { globs: [new MatrixGlob(entity.toString())], literals: [] }
+        : { literals: [entity.toString()], globs: [] }),
+    },
+    { inviteMembers }
+  );
+  // we need to also search for policies that match the literal rule given to us
+  const policyEntitiesToSearchFor = new Set([
+    ...membersMatchingEntity.map((memberRooms) => memberRooms.member),
+    entity.toString(),
+  ]);
+  const policyMatchesToRemove = findBanPoliciesMatchingUsers(
+    watchedPolicyRooms,
+    [...policyEntitiesToSearchFor]
+  );
+  const globsToScan: MatrixGlob[] = [
+    ...(entity.isContainingGlobCharacters()
+      ? [new MatrixGlob(entity.toString())]
+      : []),
+  ];
+  const literalsToScan: StringUserID[] = [
+    ...(entity.isContainingGlobCharacters() ? [] : [entity.toString()]),
+  ];
+  for (const { matches } of policyMatchesToRemove) {
+    for (const match of matches) {
+      if (match.isGlob()) {
+        globsToScan.push(new MatrixGlob(match.entity));
+      } else if (isStringUserID(match.entity)) {
+        literalsToScan.push(match.entity);
+      }
+    }
+  }
+  const membersMatchingPoliciesAndEntity = matchMembers(
+    setRoomMembership,
+    { globs: globsToScan, literals: literalsToScan },
+    { inviteMembers }
+  );
+  const isMemberStillBannedAfterPolicyRemoval = (member: MemberRooms) => {
+    const policiesMatchingMember = [
+      ...watchedPolicyRooms.currentRevision.allRulesMatchingEntity(
+        member.member,
+        PolicyRuleType.User,
+        Recommendation.Ban
+      ),
+      ...watchedPolicyRooms.currentRevision.allRulesMatchingEntity(
+        userServerName(member.member),
+        PolicyRuleType.Server,
+        Recommendation.Ban
+      ),
+    ];
+    return (
+      policiesMatchingMember.filter(
+        (policy) =>
+          !policyMatchesToRemove
+            .flatMap((list) => list.matches)
+            .includes(policy)
+      ).length > 0
+    );
+  };
+  // Now we need to filter out only members that are completly free of policies.
+  const membersToUnban = membersMatchingPoliciesAndEntity.filter(
+    (member) => !isMemberStillBannedAfterPolicyRemoval(member)
+  );
+  const unbanInformation = {
+    policyMatchesToRemove,
+    membersToUnban,
+    entity,
+  };
+  return unbanInformation;
+}

src/protections/BanPropagation.tsx

@@ -47,10 +47,10 @@ import {
 import { sendMatrixEventsFromDeadDocument } from "../commands/interface-manager/MPSMatrixInterfaceAdaptor";
 import { sendConfirmationPrompt } from "../commands/interface-manager/MatrixPromptForConfirmation";
 import {
-  findUnbanInformationForMember,
   UnbanMembersPreview,
   renderUnbanMembersPreview,
 } from "../commands/unban/Unban";
+import { findUnbanInformationForMember } from "../commands/unban/UnbanUsers";
 
 const log = new Logger("BanPropagationProtection");
 

test/integration/commands/unbanTest.ts

@@ -343,4 +343,285 @@ describe("unbanCommandTest", function () {
       }
     } as unknown as Mocha.AsyncFunc
   );
+  it(
+    "unbans users when there is no membership and only policies",
+    async function (this: DraupnirTestContext) {
+      const draupnir = this.draupnir;
+      if (draupnir === undefined) {
+        throw new TypeError(`setup didn't run properly`);
+      }
+      const falsePositiveUser = await newTestUser(this.config.homeserverUrl, {
+        name: { contains: "accidentally-banned" },
+      });
+      const falsePositiveUserID =
+        (await falsePositiveUser.getUserId()) as StringUserID;
+      const policyRoomID = await createWatchedPolicyRoom(draupnir);
+      const policyRoomEditor = (
+        await draupnir.policyRoomManager.getPolicyRoomEditor(
+          MatrixRoomReference.fromRoomID(policyRoomID)
+        )
+      ).expect(
+        "Should be able to get a policy room editor for the newly created policy room"
+      );
+      await createPoliciesBanningUser(policyRoomEditor, falsePositiveUserID);
+      // wait for policies to be detected.
+      await new Promise((resolve) => setTimeout(resolve, 1000));
+
+      // Now we can use the unban command to test the preview has no effects
+      // So the way this can work is we can send the command, get back the event and just know that we can send 'OK' and 'Cancel' to it later and it'll work.
+      const previewResult = (
+        await draupnir.sendTextCommand(
+          draupnir.clientUserID,
+          `!draupnir unban ${falsePositiveUserID}`
+        )
+      ).expect(
+        "We should have been able to get a preview"
+      ) as UnbanMembersPreview;
+      expect(previewResult.membersToUnban.length).toBe(0);
+      expect(previewResult.policyMatchesToRemove.length).toBe(1);
+      const listMatches = previewResult.policyMatchesToRemove.at(0);
+      if (listMatches === undefined) {
+        throw new TypeError("We should have some matches");
+      }
+      expect(listMatches.matches.length).toBe(3);
+      (
+        await draupnir.sendTextCommand(
+          draupnir.clientUserID,
+          `!draupnir unban ${falsePositiveUserID} --no-confirm`
+        )
+      ).expect(
+        "We should have been able to run the command"
+      ) as UnbanMembersResult;
+      // wait for events to come down sync
+      await new Promise((resolve) => setTimeout(resolve, 1000));
+      // verify the policies are removed.
+      const policyRevision =
+        draupnir.protectedRoomsSet.watchedPolicyRooms.currentRevision;
+      expect(policyRevision.allRules().length).toBe(0);
+    } as unknown as Mocha.AsyncFunc
+  );
+  it(
+    "Unbans all members targetted by a glob that is being removed indirectly by unbanning a single user in the command",
+    async function (this: DraupnirTestContext) {
+      const draupnir = this.draupnir;
+      if (draupnir === undefined) {
+        throw new TypeError(`setup didn't run properly`);
+      }
+      const falsePositiveUser = await newTestUser(this.config.homeserverUrl, {
+        name: { contains: "banned-and-cool" },
+      });
+      const indirectlyBannedUser = await newTestUser(
+        this.config.homeserverUrl,
+        {
+          name: { contains: "indirectly-banned-via-glob-and-cool" },
+        }
+      );
+      const indirectButShouldBeBanned = await newTestUser(
+        this.config.homeserverUrl,
+        {
+          name: { contains: "indirectly-banned-but-should-be-banned-and-cool" },
+        }
+      );
+      const falsePositiveUserID =
+        (await falsePositiveUser.getUserId()) as StringUserID;
+      const indirectlyBannedUserID =
+        (await indirectlyBannedUser.getUserId()) as StringUserID;
+      const indirectButShouldBeBannedUserID =
+        (await indirectButShouldBeBanned.getUserId()) as StringUserID;
+      const protectedRooms = await createProtectedRoomsSetWithBan(
+        draupnir,
+        falsePositiveUserID,
+        { numberOfRooms: 5 }
+      );
+      for (const room of protectedRooms) {
+        (
+          await draupnir.clientPlatform
+            .toRoomBanner()
+            .banUser(room, indirectlyBannedUserID, "spam")
+        ).expect("Should be able to ban people");
+        (
+          await draupnir.clientPlatform
+            .toRoomBanner()
+            .banUser(room, indirectButShouldBeBannedUserID, "spam")
+        ).expect("should be able to ban people");
+      }
+      const policyRoomID = await createWatchedPolicyRoom(draupnir);
+      const policyRoomEditor = (
+        await draupnir.policyRoomManager.getPolicyRoomEditor(
+          MatrixRoomReference.fromRoomID(policyRoomID)
+        )
+      ).expect(
+        "Should be able to get a policy room editor for the newly created policy room"
+      );
+      (
+        await policyRoomEditor.createPolicy(
+          PolicyRuleType.User,
+          Recommendation.Ban,
+          indirectButShouldBeBannedUserID,
+          "spam",
+          {}
+        )
+      ).expect(
+        "Should be able to create a policy banning just the user we want to stay banned"
+      );
+      (
+        await policyRoomEditor.createPolicy(
+          PolicyRuleType.User,
+          Recommendation.Ban,
+          `@*and-cool*:*`,
+          "spam",
+          {}
+        )
+      ).expect(
+        "Should be able to create a glob policy that bans all our targets"
+      );
+      // wait for policies to be detected.
+      await new Promise((resolve) => setTimeout(resolve, 1000));
+      // verify policies exist
+      expect(
+        draupnir.protectedRoomsSet.watchedPolicyRooms.currentRevision.allRules()
+          .length
+      ).toBe(2);
+      // Now we can use the unban command to test the preview has no effects
+      // So the way this can work is we can send the command, get back the event and just know that we can send 'OK' and 'Cancel' to it later and it'll work.
+      const previewResult = (
+        await draupnir.sendTextCommand(
+          draupnir.clientUserID,
+          `!draupnir unban ${falsePositiveUserID}`
+        )
+      ).expect(
+        "We should have been able to get a preview"
+      ) as UnbanMembersPreview;
+      expect(previewResult.membersToUnban.length).toBe(2);
+      expect(previewResult.policyMatchesToRemove.length).toBe(1);
+      const listMatches = previewResult.policyMatchesToRemove.at(0);
+      if (listMatches === undefined) {
+        throw new TypeError("We should have some matches");
+      }
+      expect(listMatches.matches.length).toBe(1);
+      const falsePositiveMember = previewResult.membersToUnban.at(0);
+      if (falsePositiveMember === undefined) {
+        throw new TypeError("We should have some details here");
+      }
+      const indirectlyBannedMember = previewResult.membersToUnban.at(1);
+      if (indirectlyBannedMember === undefined) {
+        throw new TypeError("We should have some details here");
+      }
+      expect(indirectlyBannedMember.roomsBannedFrom.length).toBe(5);
+      expect(falsePositiveMember.member).toBe(falsePositiveUserID);
+      expect(indirectlyBannedMember.member).toBe(indirectlyBannedUserID);
+      // now checked that the user is still banned in all 5 rooms
+      for (const roomID of protectedRooms) {
+        const membershipRevision =
+          draupnir.protectedRoomsSet.setRoomMembership.getRevision(roomID);
+        if (membershipRevision === undefined) {
+          throw new TypeError(
+            "Unable to find membership revision for a protected room, shouldn't happen"
+          );
+        }
+        expect(
+          membershipRevision.membershipForUser(indirectButShouldBeBannedUserID)
+            ?.membership
+        ).toBe(Membership.Ban);
+      }
+      (
+        await draupnir.sendTextCommand(
+          draupnir.clientUserID,
+          `!draupnir unban ${falsePositiveUserID} --no-confirm`
+        )
+      ).expect(
+        "We should have been able to run the command"
+      ) as UnbanMembersResult;
+      // wait for events to come down sync
+      await new Promise((resolve) => setTimeout(resolve, 1000));
+      // now check that they are unbanned
+      for (const roomID of protectedRooms) {
+        const membershipRevision =
+          draupnir.protectedRoomsSet.setRoomMembership.getRevision(roomID);
+        if (membershipRevision === undefined) {
+          throw new TypeError(
+            "Unable to find membership revision for a protected room, shouldn't happen"
+          );
+        }
+        expect(
+          membershipRevision.membershipForUser(indirectlyBannedUserID)
+            ?.membership
+        ).toBe(Membership.Leave);
+        expect(
+          membershipRevision.membershipForUser(indirectButShouldBeBannedUserID)
+            ?.membership
+        ).toBe(Membership.Ban);
+      }
+      // verify the policies are removed.
+      const policyRevision =
+        draupnir.protectedRoomsSet.watchedPolicyRooms.currentRevision;
+      expect(policyRevision.allRules().length).toBe(1);
+    } as unknown as Mocha.AsyncFunc
+  );
+  it(
+    "removes glob rules when there are only glob rules and no banned members",
+    async function (this: DraupnirTestContext) {
+      const draupnir = this.draupnir;
+      if (draupnir === undefined) {
+        throw new TypeError(`setup didn't run properly`);
+      }
+      const policyRoomID = await createWatchedPolicyRoom(draupnir);
+      const policyRoomEditor = (
+        await draupnir.policyRoomManager.getPolicyRoomEditor(
+          MatrixRoomReference.fromRoomID(policyRoomID)
+        )
+      ).expect(
+        "Should be able to get a policy room editor for the newly created policy room"
+      );
+      const globRuleEntity = `@*glob*:*`;
+      (
+        await policyRoomEditor.createPolicy(
+          PolicyRuleType.User,
+          Recommendation.Ban,
+          globRuleEntity,
+          "spam",
+          {}
+        )
+      ).expect(
+        "Should be able to create a glob policy that bans all our targets"
+      );
+      // wait for policies to be detected.
+      await new Promise((resolve) => setTimeout(resolve, 1000));
+      expect(
+        draupnir.protectedRoomsSet.watchedPolicyRooms.currentRevision.allRules()
+          .length
+      ).toBe(1);
+      // Now we can use the unban command to test the preview has no effects
+      // So the way this can work is we can send the command, get back the event and just know that we can send 'OK' and 'Cancel' to it later and it'll work.
+      const previewResult = (
+        await draupnir.sendTextCommand(
+          draupnir.clientUserID,
+          `!draupnir unban ${globRuleEntity}`
+        )
+      ).expect(
+        "We should have been able to get a preview"
+      ) as UnbanMembersPreview;
+      expect(previewResult.membersToUnban.length).toBe(0);
+      expect(previewResult.policyMatchesToRemove.length).toBe(1);
+      const listMatches = previewResult.policyMatchesToRemove.at(0);
+      if (listMatches === undefined) {
+        throw new TypeError("We should have some matches");
+      }
+      expect(listMatches.matches.length).toBe(1);
+      (
+        await draupnir.sendTextCommand(
+          draupnir.clientUserID,
+          `!draupnir unban ${globRuleEntity} --no-confirm`
+        )
+      ).expect(
+        "We should have been able to run the command"
+      ) as UnbanMembersResult;
+      // wait for events to come down sync
+      await new Promise((resolve) => setTimeout(resolve, 1000));
+      // verify the policies are removed.
+      const policyRevision =
+        draupnir.protectedRoomsSet.watchedPolicyRooms.currentRevision;
+      expect(policyRevision.allRules().length).toBe(0);
+    } as unknown as Mocha.AsyncFunc
+  );
 });