Limit number of GV2 banned members

protos/Groups.proto

@@ -46,7 +46,8 @@ message MemberPendingAdminApproval {
 }
 
 message MemberBanned {
-  bytes userId = 1;
+  bytes userId        = 1;
+  uint64 timestamp    = 2;  // ms since epoch
 }
 
 message AccessControl {

ts/components/conversation/GroupV2Change.tsx

@@ -202,6 +202,7 @@ function GroupV2Detail({
             {
               action: () => blockGroupLinkRequests(detail.uuid),
               text: i18n('PendingRequests--block--confirm'),
+              style: 'affirmative',
             },
           ]}
           i18n={i18n}

ts/groups.ts

@@ -33,6 +33,7 @@ import type {
   GroupV2MemberType,
   GroupV2PendingAdminApprovalType,
   GroupV2PendingMemberType,
+  GroupV2BannedMemberType,
   MessageAttributesType,
 } from './model-types.d';
 import {
@@ -727,7 +728,9 @@ export async function buildAddMembersChange(
         addPendingMembers.push(addPendingMemberAction);
       }
 
-      const doesMemberNeedUnban = conversation.bannedMembersV2?.includes(uuid);
+      const doesMemberNeedUnban = conversation.bannedMembersV2?.find(
+        bannedMember => bannedMember.uuid === uuid
+      );
       if (doesMemberNeedUnban) {
         const uuidCipherTextBuffer = encryptUuid(clientZkGroupCipher, uuid);
 
@@ -976,6 +979,67 @@ export function buildAccessControlMembersChange(
   return actions;
 }
 
+export function _maybeBuildAddBannedMemberActions({
+  clientZkGroupCipher,
+  group,
+  ourUuid,
+  uuid,
+}: {
+  clientZkGroupCipher: ClientZkGroupCipher;
+  group: Pick<ConversationAttributesType, 'bannedMembersV2'>;
+  ourUuid: UUIDStringType;
+  uuid: UUIDStringType;
+}): Pick<
+  Proto.GroupChange.IActions,
+  'addMembersBanned' | 'deleteMembersBanned'
+> {
+  const doesMemberNeedBan =
+    !group.bannedMembersV2?.find(member => member.uuid === uuid) &&
+    uuid !== ourUuid;
+  if (!doesMemberNeedBan) {
+    return {};
+  }
+  // Sort current banned members by decreasing timestamp
+  const sortedBannedMembers = [...(group.bannedMembersV2 ?? [])].sort(
+    (a, b) => {
+      return b.timestamp - a.timestamp;
+    }
+  );
+
+  // All members after the limit have to be deleted and are older than the
+  // rest of the list.
+  const deletedBannedMembers = sortedBannedMembers.slice(
+    Math.max(0, getGroupSizeHardLimit() - 1)
+  );
+
+  let deleteMembersBanned = null;
+  if (deletedBannedMembers.length > 0) {
+    deleteMembersBanned = deletedBannedMembers.map(bannedMember => {
+      const deleteMemberBannedAction =
+        new Proto.GroupChange.Actions.DeleteMemberBannedAction();
+
+      deleteMemberBannedAction.deletedUserId = encryptUuid(
+        clientZkGroupCipher,
+        bannedMember.uuid
+      );
+
+      return deleteMemberBannedAction;
+    });
+  }
+
+  const addMemberBannedAction =
+    new Proto.GroupChange.Actions.AddMemberBannedAction();
+
+  const uuidCipherTextBuffer = encryptUuid(clientZkGroupCipher, uuid);
+  addMemberBannedAction.added = new Proto.MemberBanned();
+  addMemberBannedAction.added.userId = uuidCipherTextBuffer;
+
+  return {
+    addMembersBanned: [addMemberBannedAction],
+    deleteMembersBanned,
+  };
+}
+
 // TODO AND-1101
 export function buildDeletePendingAdminApprovalMemberChange({
   group,
@@ -1005,16 +1069,19 @@ export function buildDeletePendingAdminApprovalMemberChange({
     deleteMemberPendingAdminApproval,
   ];
 
-  const doesMemberNeedBan =
-    !group.bannedMembersV2?.includes(uuid) && uuid !== ourUuid;
-  if (doesMemberNeedBan) {
-    const addMemberBannedAction =
-      new Proto.GroupChange.Actions.AddMemberBannedAction();
+  const { addMembersBanned, deleteMembersBanned } =
+    _maybeBuildAddBannedMemberActions({
+      clientZkGroupCipher,
+      group,
+      ourUuid,
+      uuid,
+    });
 
-    addMemberBannedAction.added = new Proto.MemberBanned();
-    addMemberBannedAction.added.userId = uuidCipherTextBuffer;
-
-    actions.addMembersBanned = [addMemberBannedAction];
+  if (addMembersBanned) {
+    actions.addMembersBanned = addMembersBanned;
+  }
+  if (deleteMembersBanned) {
+    actions.deleteMembersBanned = deleteMembersBanned;
   }
 
   return actions;
@@ -1098,7 +1165,9 @@ export function buildAddMember({
   actions.version = (group.revision || 0) + 1;
   actions.addMembers = [addMember];
 
-  const doesMemberNeedUnban = group.bannedMembersV2?.includes(uuid);
+  const doesMemberNeedUnban = group.bannedMembersV2?.find(
+    member => member.uuid === uuid
+  );
   if (doesMemberNeedUnban) {
     const clientZkGroupCipher = getClientZkGroupCipher(group.secretParams);
     const uuidCipherTextBuffer = encryptUuid(clientZkGroupCipher, uuid);
@@ -1166,17 +1235,19 @@ export function buildDeleteMemberChange({
   actions.version = (group.revision || 0) + 1;
   actions.deleteMembers = [deleteMember];
 
-  const doesMemberNeedBan =
-    !group.bannedMembersV2?.includes(uuid) && uuid !== ourUuid;
+  const { addMembersBanned, deleteMembersBanned } =
+    _maybeBuildAddBannedMemberActions({
+      clientZkGroupCipher,
+      group,
+      ourUuid,
+      uuid,
+    });
 
-  if (doesMemberNeedBan) {
-    const addMemberBannedAction =
-      new Proto.GroupChange.Actions.AddMemberBannedAction();
-
-    addMemberBannedAction.added = new Proto.MemberBanned();
-    addMemberBannedAction.added.userId = uuidCipherTextBuffer;
-
-    actions.addMembersBanned = [addMemberBannedAction];
+  if (addMembersBanned) {
+    actions.addMembersBanned = addMembersBanned;
+  }
+  if (deleteMembersBanned) {
+    actions.deleteMembersBanned = deleteMembersBanned;
   }
 
   return actions;
@@ -4362,8 +4433,8 @@ async function applyGroupChange({
   > = fromPairs(
     (result.pendingAdminApprovalV2 || []).map(member => [member.uuid, member])
   );
-  const bannedMembers: Record<UUIDStringType, UUIDStringType> = fromPairs(
-    (result.bannedMembersV2 || []).map(uuid => [uuid, uuid])
+  const bannedMembers = new Map<UUIDStringType, GroupV2BannedMemberType>(
+    (result.bannedMembersV2 || []).map(member => [member.uuid, member])
   );
 
   // version?: number;
@@ -4787,28 +4858,28 @@ async function applyGroupChange({
   }
 
   if (actions.addMembersBanned && actions.addMembersBanned.length > 0) {
-    actions.addMembersBanned.forEach(uuid => {
-      if (bannedMembers[uuid]) {
+    actions.addMembersBanned.forEach(member => {
+      if (bannedMembers.has(member.uuid)) {
         log.warn(
           `applyGroupChange/${logId}: Attempt to add banned member failed; was already in banned list.`
         );
         return;
       }
 
-      bannedMembers[uuid] = uuid;
+      bannedMembers.set(member.uuid, member);
     });
   }
 
   if (actions.deleteMembersBanned && actions.deleteMembersBanned.length > 0) {
     actions.deleteMembersBanned.forEach(uuid => {
-      if (!bannedMembers[uuid]) {
+      if (!bannedMembers.has(uuid)) {
         log.warn(
           `applyGroupChange/${logId}: Attempt to remove banned member failed; was not in banned list.`
         );
         return;
       }
 
-      delete bannedMembers[uuid];
+      bannedMembers.delete(uuid);
     });
   }
 
@@ -4820,7 +4891,7 @@ async function applyGroupChange({
   result.membersV2 = values(members);
   result.pendingMembersV2 = values(pendingMembers);
   result.pendingAdminApprovalV2 = values(pendingAdminApprovalMembers);
-  result.bannedMembersV2 = values(bannedMembers);
+  result.bannedMembersV2 = Array.from(bannedMembers.values());
 
   return {
     newAttributes: result,
@@ -5181,7 +5252,7 @@ type DecryptedGroupChangeActions = {
   modifyAnnouncementsOnly?: {
     announcementsOnly: boolean;
   };
-  addMembersBanned?: ReadonlyArray<UUIDStringType>;
+  addMembersBanned?: ReadonlyArray<GroupV2BannedMemberType>;
   deleteMembersBanned?: ReadonlyArray<UUIDStringType>;
 } & Pick<
   Proto.GroupChange.IActions,
@@ -5720,10 +5791,13 @@ function decryptGroupChange(
           );
           return null;
         }
-        return normalizeUuid(
+        const uuid = normalizeUuid(
           decryptUuid(clientZkGroupCipher, item.added.userId),
           'addMembersBanned.added.userId'
         );
+        const timestamp = normalizeTimestamp(item.added.timestamp);
+
+        return { uuid, timestamp };
       })
       .filter(isNotNil);
   }
@@ -5804,7 +5878,7 @@ type DecryptedGroupState = {
   descriptionBytes?: Proto.GroupAttributeBlob;
   avatar?: string;
   announcementsOnly?: boolean;
-  membersBanned?: Array<UUIDStringType>;
+  membersBanned?: Array<GroupV2BannedMemberType>;
 };
 
 function decryptGroupState(
@@ -5951,10 +6025,13 @@ function decryptGroupState(
           );
           return null;
         }
-        return normalizeUuid(
+        const uuid = normalizeUuid(
           decryptUuid(clientZkGroupCipher, item.userId),
           'membersBanned.added.userId'
         );
+        const timestamp = item.timestamp?.toNumber() ?? 0;
+
+        return { uuid, timestamp };
       })
       .filter(isNotNil);
   } else {

ts/model-types.d.ts

@@ -351,7 +351,7 @@ export type ConversationAttributesType = {
   membersV2?: Array<GroupV2MemberType>;
   pendingMembersV2?: Array<GroupV2PendingMemberType>;
   pendingAdminApprovalV2?: Array<GroupV2PendingAdminApprovalType>;
-  bannedMembersV2?: Array<UUIDStringType>;
+  bannedMembersV2?: Array<GroupV2BannedMemberType>;
   groupInviteLinkPassword?: string;
   previousGroupV1Id?: string;
   previousGroupV1Members?: Array<string>;
@@ -390,6 +390,11 @@ export type GroupV2PendingMemberType = {
   role: MemberRoleEnum;
 };
 
+export type GroupV2BannedMemberType = {
+  uuid: UUIDStringType;
+  timestamp: number;
+};
+
 export type GroupV2PendingAdminApprovalType = {
   uuid: UUIDStringType;
   timestamp: number;

ts/models/conversations.ts

@@ -435,7 +435,7 @@ export class ConversationModel extends window.Backbone
     }
 
     const uuid = UUID.checkedLookup(id).toString();
-    return bannedMembersV2.some(item => item === uuid);
+    return bannedMembersV2.some(member => member.uuid === uuid);
   }
 
   isMemberAwaitingApproval(id: string): boolean {
@@ -3555,7 +3555,7 @@ export class ConversationModel extends window.Backbone
       return [];
     }
 
-    return this.get('bannedMembersV2') || [];
+    return (this.get('bannedMembersV2') || []).map(member => member.uuid);
   }
 
   getMembers(

ts/sql/migrations/53-gv2-banned-members.ts

@@ -0,0 +1,92 @@
+// Copyright 2022 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import type { Database } from 'better-sqlite3';
+
+import type { LoggerType } from '../../types/Logging';
+import type { UUIDStringType } from '../../types/UUID';
+import { jsonToObject } from '../util';
+import type { EmptyQuery } from '../util';
+import type { ConversationType } from '../Interface';
+
+export default function updateToSchemaVersion53(
+  currentVersion: number,
+  db: Database,
+  logger: LoggerType
+): void {
+  if (currentVersion >= 53) {
+    return;
+  }
+
+  type LegacyConversationType = {
+    id: string;
+    groupId: string;
+    bannedMembersV2?: Array<UUIDStringType>;
+  };
+
+  const updateConversationStmt = db.prepare(
+    `
+      UPDATE conversations SET
+        json = json_patch(json, $jsonPatch)
+      WHERE id = $id;
+    `
+  );
+
+  const upgradeConversation = (convo: ConversationType): boolean => {
+    const legacy = convo as unknown as LegacyConversationType;
+
+    const logId = `(${legacy.id}) groupv2(${legacy.groupId})`;
+
+    if (!legacy.bannedMembersV2?.length) {
+      return false;
+    }
+
+    const jsonPatch: Pick<ConversationType, 'bannedMembersV2'> = {
+      bannedMembersV2: legacy.bannedMembersV2.map(uuid => ({
+        uuid,
+        timestamp: 0,
+      })),
+    };
+
+    logger.info(
+      `updateToSchemaVersion53: Updating ${logId} with ` +
+        `${legacy.bannedMembersV2.length} banned members`
+    );
+
+    updateConversationStmt.run({
+      id: legacy.id,
+      jsonPatch: JSON.stringify(jsonPatch),
+    });
+
+    return true;
+  };
+
+  db.transaction(() => {
+    const allConversations = db
+      .prepare<EmptyQuery>(
+        `
+          SELECT json, profileLastFetchedAt
+          FROM conversations
+          WHERE type = 'group'
+          ORDER BY id ASC;
+        `
+      )
+      .all()
+      .map(({ json }) => jsonToObject<ConversationType>(json));
+
+    logger.info(
+      'updateToSchemaVersion53: About to iterate through ' +
+        `${allConversations.length} conversations`
+    );
+
+    let updated = 0;
+    for (const convo of allConversations) {
+      updated += upgradeConversation(convo) ? 1 : 0;
+    }
+
+    logger.info(`updateToSchemaVersion53: Updated ${updated} conversations`);
+
+    db.pragma('user_version = 53');
+  })();
+  logger.info('updateToSchemaVersion53: success!');
+}

ts/sql/migrations/index.ts

@@ -28,6 +28,7 @@ import updateToSchemaVersion49 from './49-fix-preview-index';
 import updateToSchemaVersion50 from './50-fix-messages-unread-index';
 import updateToSchemaVersion51 from './51-centralize-conversation-jobs';
 import updateToSchemaVersion52 from './52-optimize-stories';
+import updateToSchemaVersion53 from './53-gv2-banned-members';
 
 function updateToSchemaVersion1(
   currentVersion: number,
@@ -1919,6 +1920,7 @@ export const SCHEMA_VERSIONS = [
   updateToSchemaVersion50,
   updateToSchemaVersion51,
   updateToSchemaVersion52,
+  updateToSchemaVersion53,
 ];
 
 export function updateSchema(db: Database, logger: LoggerType): void {

ts/test-both/groups/add_banned_member_test.ts

@@ -0,0 +1,118 @@
+// Copyright 2022 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import { assert } from 'chai';
+
+import { UUID } from '../../types/UUID';
+import { _maybeBuildAddBannedMemberActions } from '../../groups';
+import { getClientZkGroupCipher, decryptUuid } from '../../util/zkgroup';
+import { updateRemoteConfig } from '../helpers/RemoteConfigStub';
+
+const HARD_LIMIT_KEY = 'global.groupsv2.groupSizeHardLimit';
+
+describe('group add banned member', () => {
+  const uuid = UUID.generate().toString();
+  const ourUuid = UUID.generate().toString();
+  const existing = Array.from({ length: 10 }, (_, index) => ({
+    uuid: UUID.generate().toString(),
+    timestamp: index,
+  }));
+  const secretParams =
+    'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAd/rq8//fR' +
+    '4RzhvN3G9KcKlQoj7cguQFjTOqLV6JUSbrURzeILsUmsymGJmHt3kpBJ2zosqp4ex' +
+    'sg+qwF1z6YdB/rxKnxKRLZZP/V0F7bERslYILy2lUh3Sh3iA98yO4CGfzjjFVo1SI' +
+    '7U8XApLeVNQHJo7nkflf/JyBrqPft5gEucbKW/h+S3OYjfQ5zl2Cpw3XrV7N6OKEu' +
+    'tLUWPHQuJx11A4xDPrmtAOnGy2NBxoOybDNlWipeNbn1WQJqOjMF7YA80oEm+5qnM' +
+    'kEYcFVqbYaSzPcMhg3mQ0SYfQpxYgSOJpwp9f/8EDnwJV4ISPBOo2CiaSqVfnd8Dw' +
+    'ZOc58gQA==';
+  const clientZkGroupCipher = getClientZkGroupCipher(secretParams);
+
+  before(async () => {
+    await updateRemoteConfig([
+      { name: HARD_LIMIT_KEY, value: '5', enabled: true },
+    ]);
+  });
+
+  it('should add banned member without deleting', () => {
+    const actions = _maybeBuildAddBannedMemberActions({
+      clientZkGroupCipher,
+      uuid,
+      ourUuid,
+      group: {
+        bannedMembersV2: [],
+      },
+    });
+
+    assert.strictEqual(actions.addMembersBanned?.length, 1);
+    assert.strictEqual(
+      decryptUuid(
+        clientZkGroupCipher,
+        actions.addMembersBanned?.[0]?.added?.userId ?? new Uint8Array(0)
+      ),
+      uuid
+    );
+    assert.strictEqual(actions.deleteMembersBanned, null);
+  });
+
+  it('should add banned member while deleting the oldest', () => {
+    const actions = _maybeBuildAddBannedMemberActions({
+      clientZkGroupCipher,
+      uuid,
+      ourUuid,
+      group: {
+        bannedMembersV2: [...existing],
+      },
+    });
+
+    const deleted = actions.deleteMembersBanned?.map(({ deletedUserId }) => {
+      return decryptUuid(
+        clientZkGroupCipher,
+        deletedUserId ?? new Uint8Array(0)
+      );
+    });
+
+    assert.strictEqual(actions.addMembersBanned?.length, 1);
+    assert.strictEqual(
+      decryptUuid(
+        clientZkGroupCipher,
+        actions.addMembersBanned?.[0]?.added?.userId ?? new Uint8Array(0)
+      ),
+      uuid
+    );
+    assert.deepStrictEqual(
+      deleted,
+      existing
+        .slice(0, 6)
+        .reverse()
+        .map(member => member.uuid)
+    );
+  });
+
+  it('should not ban ourselves', () => {
+    const actions = _maybeBuildAddBannedMemberActions({
+      clientZkGroupCipher,
+      uuid: ourUuid,
+      ourUuid,
+      group: {
+        bannedMembersV2: [],
+      },
+    });
+
+    assert.isUndefined(actions.addMembersBanned);
+    assert.isUndefined(actions.deleteMembersBanned);
+  });
+
+  it('should not ban already banned person', () => {
+    const actions = _maybeBuildAddBannedMemberActions({
+      clientZkGroupCipher,
+      uuid,
+      ourUuid,
+      group: {
+        bannedMembersV2: [{ uuid, timestamp: 1 }],
+      },
+    });
+
+    assert.isUndefined(actions.addMembersBanned);
+    assert.isUndefined(actions.deleteMembersBanned);
+  });
+});

ts/test-node/sql_migrations_test.ts

@@ -1670,4 +1670,60 @@ describe('SQL migrations test', () => {
       }
     });
   });
+
+  describe('updateToSchemaVersion53', () => {
+    it('remaps bannedMembersV2 to array of objects', () => {
+      updateToVersion(52);
+
+      const UUID_A = generateGuid();
+      const UUID_B = generateGuid();
+      const UUID_C = generateGuid();
+
+      const noMembers = { id: 'a', groupId: 'gv2a' };
+      const emptyMembers = {
+        id: 'b',
+        groupId: 'gv2b',
+        bannedMembersV2: [],
+      };
+
+      const nonEmptyMembers = {
+        id: 'c',
+        groupId: 'gv2c',
+        bannedMembersV2: [UUID_A, UUID_B],
+      };
+
+      db.exec(
+        `
+        INSERT INTO conversations
+          (id, type, uuid, json)
+          VALUES
+          ('a', 'group', '${UUID_A}', '${JSON.stringify(noMembers)}'),
+          ('b', 'group', '${UUID_B}', '${JSON.stringify(emptyMembers)}'),
+          ('c', 'group', '${UUID_C}', '${JSON.stringify(nonEmptyMembers)}');
+        `
+      );
+
+      updateToVersion(53);
+
+      const entries: Array<{ id: string; json: string }> = db
+        .prepare('SELECT id, json FROM conversations ORDER BY id')
+        .all();
+
+      assert.deepStrictEqual(
+        entries.map(({ id, json }) => ({ id, ...JSON.parse(json) })),
+        [
+          { id: 'a', groupId: 'gv2a' },
+          { id: 'b', groupId: 'gv2b', bannedMembersV2: [] },
+          {
+            id: 'c',
+            groupId: 'gv2c',
+            bannedMembersV2: [
+              { uuid: UUID_A, timestamp: 0 },
+              { uuid: UUID_B, timestamp: 0 },
+            ],
+          },
+        ]
+      );
+    });
+  });
 });