From c5a3ffddf9ee3a0f21dda4188c089de628005b82 Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Wed, 23 Mar 2022 15:34:51 -0700 Subject: [PATCH] Limit number of GV2 banned members --- protos/Groups.proto | 3 +- ts/components/conversation/GroupV2Change.tsx | 1 + ts/groups.ts | 143 ++++++++++++++---- ts/model-types.d.ts | 7 +- ts/models/conversations.ts | 4 +- ts/sql/migrations/53-gv2-banned-members.ts | 92 +++++++++++ ts/sql/migrations/index.ts | 2 + ts/test-both/groups/add_banned_member_test.ts | 118 +++++++++++++++ ts/test-node/sql_migrations_test.ts | 56 +++++++ 9 files changed, 389 insertions(+), 37 deletions(-) create mode 100644 ts/sql/migrations/53-gv2-banned-members.ts create mode 100644 ts/test-both/groups/add_banned_member_test.ts diff --git a/protos/Groups.proto b/protos/Groups.proto index 262cb06a784b..f9345ed8754a 100644 --- a/protos/Groups.proto +++ b/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 { diff --git a/ts/components/conversation/GroupV2Change.tsx b/ts/components/conversation/GroupV2Change.tsx index 6d8706342f8e..f50fe30b46d1 100644 --- a/ts/components/conversation/GroupV2Change.tsx +++ b/ts/components/conversation/GroupV2Change.tsx @@ -202,6 +202,7 @@ function GroupV2Detail({ { action: () => blockGroupLinkRequests(detail.uuid), text: i18n('PendingRequests--block--confirm'), + style: 'affirmative', }, ]} i18n={i18n} diff --git a/ts/groups.ts b/ts/groups.ts index b2985eaf2cbb..af0ba3f85dde 100644 --- a/ts/groups.ts +++ b/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; + 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 = fromPairs( - (result.bannedMembersV2 || []).map(uuid => [uuid, uuid]) + const bannedMembers = new Map( + (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; + addMembersBanned?: ReadonlyArray; deleteMembersBanned?: ReadonlyArray; } & 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; + membersBanned?: Array; }; 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 { diff --git a/ts/model-types.d.ts b/ts/model-types.d.ts index 966fb1f64190..0f4f6f41e9c6 100644 --- a/ts/model-types.d.ts +++ b/ts/model-types.d.ts @@ -351,7 +351,7 @@ export type ConversationAttributesType = { membersV2?: Array; pendingMembersV2?: Array; pendingAdminApprovalV2?: Array; - bannedMembersV2?: Array; + bannedMembersV2?: Array; groupInviteLinkPassword?: string; previousGroupV1Id?: string; previousGroupV1Members?: Array; @@ -390,6 +390,11 @@ export type GroupV2PendingMemberType = { role: MemberRoleEnum; }; +export type GroupV2BannedMemberType = { + uuid: UUIDStringType; + timestamp: number; +}; + export type GroupV2PendingAdminApprovalType = { uuid: UUIDStringType; timestamp: number; diff --git a/ts/models/conversations.ts b/ts/models/conversations.ts index 65b4aa3ac680..e18c5b721e51 100644 --- a/ts/models/conversations.ts +++ b/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( diff --git a/ts/sql/migrations/53-gv2-banned-members.ts b/ts/sql/migrations/53-gv2-banned-members.ts new file mode 100644 index 000000000000..34221a727393 --- /dev/null +++ b/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; + }; + + 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 = { + 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( + ` + SELECT json, profileLastFetchedAt + FROM conversations + WHERE type = 'group' + ORDER BY id ASC; + ` + ) + .all() + .map(({ json }) => jsonToObject(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!'); +} diff --git a/ts/sql/migrations/index.ts b/ts/sql/migrations/index.ts index d6d1c9b940eb..648e4e039563 100644 --- a/ts/sql/migrations/index.ts +++ b/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 { diff --git a/ts/test-both/groups/add_banned_member_test.ts b/ts/test-both/groups/add_banned_member_test.ts new file mode 100644 index 000000000000..245d8a6b2da1 --- /dev/null +++ b/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); + }); +}); diff --git a/ts/test-node/sql_migrations_test.ts b/ts/test-node/sql_migrations_test.ts index 0780f5454a39..f02a917bb53b 100644 --- a/ts/test-node/sql_migrations_test.ts +++ b/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 }, + ], + }, + ] + ); + }); + }); });