From fbfa3abffd1128aa09755a6da4021ad02bc539f0 Mon Sep 17 00:00:00 2001 From: Alan Evans Date: Wed, 5 Aug 2020 14:01:00 -0300 Subject: [PATCH] Skip delete actions where the removed member/pending member is not in the group. --- .../api/groupsv2/DecryptedGroupUtil.java | 137 +++++++++++++++--- .../DecryptedGroupUtil_apply_Test.java | 93 +++++++++--- 2 files changed, 187 insertions(+), 43 deletions(-) diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtil.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtil.java index a0b3d8130c..6b574324df 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtil.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtil.java @@ -11,6 +11,7 @@ import org.signal.storageservice.protos.groups.local.DecryptedModifyMemberRole; import org.signal.storageservice.protos.groups.local.DecryptedPendingMember; import org.signal.storageservice.protos.groups.local.DecryptedPendingMemberRemoval; import org.signal.zkgroup.util.UUIDUtil; +import org.whispersystems.libsignal.logging.Log; import org.whispersystems.libsignal.util.guava.Optional; import org.whispersystems.signalservice.api.util.UuidUtil; @@ -24,6 +25,8 @@ import java.util.UUID; public final class DecryptedGroupUtil { + private static final String TAG = DecryptedGroupUtil.class.getSimpleName(); + static final int MAX_CHANGE_FIELD = 14; public static ArrayList toUuidList(Collection membersList) { @@ -194,21 +197,57 @@ public final class DecryptedGroupUtil { public static DecryptedGroup applyWithoutRevisionCheck(DecryptedGroup group, DecryptedGroupChange change) throws NotAbleToApplyGroupV2ChangeException { - DecryptedGroup.Builder builder = DecryptedGroup.newBuilder(group); + DecryptedGroup.Builder builder = DecryptedGroup.newBuilder(group) + .setRevision(change.getRevision()); - builder.addAllMembers(change.getNewMembersList()); + applyAddMemberAction(builder, change.getNewMembersList()); - for (ByteString removedMember : change.getDeleteMembersList()) { + applyDeleteMemberActions(builder, change.getDeleteMembersList()); + + applyModifyMemberRoleActions(builder, change.getModifyMemberRolesList()); + + applyModifyMemberProfileKeyActions(builder, change.getModifiedProfileKeysList()); + + applyAddPendingMemberActions(builder, change.getNewPendingMembersList()); + + applyDeletePendingMemberActions(builder, change.getDeletePendingMembersList()); + + applyPromotePendingMemberActions(builder, change.getPromotePendingMembersList()); + + applyModifyTitleAction(builder, change); + + applyModifyAvatarAction(builder, change); + + applyModifyDisappearingMessagesTimerAction(builder, change); + + applyModifyAttributesAccessControlAction(builder, change); + + applyModifyMembersAccessControlAction(builder, change); + + return builder.build(); + } + + private static void applyAddMemberAction(DecryptedGroup.Builder builder, List newMembersList) { + builder.addAllMembers(newMembersList); + + removePendingMembersNowInGroup(builder); + } + + protected static void applyDeleteMemberActions(DecryptedGroup.Builder builder, List deleteMembersList) { + for (ByteString removedMember : deleteMembersList) { int index = indexOfUuid(builder.getMembersList(), removedMember); if (index == -1) { - throw new NotAbleToApplyGroupV2ChangeException(); + Log.w(TAG, "Deleted member on change not found in group"); + continue; } builder.removeMembers(index); } + } - for (DecryptedModifyMemberRole modifyMemberRole : change.getModifyMemberRolesList()) { + private static void applyModifyMemberRoleActions(DecryptedGroup.Builder builder, List modifyMemberRolesList) throws NotAbleToApplyGroupV2ChangeException { + for (DecryptedModifyMemberRole modifyMemberRole : modifyMemberRolesList) { int index = indexOfUuid(builder.getMembersList(), modifyMemberRole.getUuid()); if (index == -1) { @@ -221,28 +260,50 @@ public final class DecryptedGroupUtil { builder.setMembers(index, DecryptedMember.newBuilder(builder.getMembers(index)).setRole(modifyMemberRole.getRole()).build()); } + } - for (DecryptedMember modifyProfileKey : change.getModifiedProfileKeysList()) { + private static void applyModifyMemberProfileKeyActions(DecryptedGroup.Builder builder, List modifiedProfileKeysList) throws NotAbleToApplyGroupV2ChangeException { + for (DecryptedMember modifyProfileKey : modifiedProfileKeysList) { int index = indexOfUuid(builder.getMembersList(), modifyProfileKey.getUuid()); if (index == -1) { throw new NotAbleToApplyGroupV2ChangeException(); } - builder.setMembers(index, DecryptedMember.newBuilder(builder.getMembers(index)).setProfileKey(modifyProfileKey.getProfileKey()).build()); + builder.setMembers(index, withNewProfileKey(builder.getMembers(index), modifyProfileKey.getProfileKey())); } + } - for (DecryptedPendingMemberRemoval removedMember : change.getDeletePendingMembersList()) { + private static void applyAddPendingMemberActions(DecryptedGroup.Builder builder, List newPendingMembersList) throws NotAbleToApplyGroupV2ChangeException { + Set fullMemberSet = getMemberUuidSet(builder.getMembersList()); + Set pendingMemberCipherTexts = getPendingMemberCipherTextSet(builder.getPendingMembersList()); + + for (DecryptedPendingMember pendingMember : newPendingMembersList) { + if (fullMemberSet.contains(pendingMember.getUuid())) { + throw new NotAbleToApplyGroupV2ChangeException(); + } + + if (!pendingMemberCipherTexts.contains(pendingMember.getUuidCipherText())) { + builder.addPendingMembers(pendingMember); + } + } + } + + protected static void applyDeletePendingMemberActions(DecryptedGroup.Builder builder, List deletePendingMembersList) { + for (DecryptedPendingMemberRemoval removedMember : deletePendingMembersList) { int index = findPendingIndexByUuidCipherText(builder.getPendingMembersList(), removedMember.getUuidCipherText()); if (index == -1) { - throw new NotAbleToApplyGroupV2ChangeException(); + Log.w(TAG, "Deleted pending member on change not found in group"); + continue; } builder.removePendingMembers(index); } + } - for (DecryptedMember newMember : change.getPromotePendingMembersList()) { + protected static void applyPromotePendingMemberActions(DecryptedGroup.Builder builder, List promotePendingMembersList) throws NotAbleToApplyGroupV2ChangeException { + for (DecryptedMember newMember : promotePendingMembersList) { int index = findPendingIndexByUuid(builder.getPendingMembersList(), newMember.getUuid()); if (index == -1) { @@ -252,39 +313,69 @@ public final class DecryptedGroupUtil { builder.removePendingMembers(index); builder.addMembers(newMember); } + } - builder.addAllPendingMembers(change.getNewPendingMembersList()); - + protected static void applyModifyTitleAction(DecryptedGroup.Builder builder, DecryptedGroupChange change) { if (change.hasNewTitle()) { builder.setTitle(change.getNewTitle().getValue()); } + } + protected static void applyModifyAvatarAction(DecryptedGroup.Builder builder, DecryptedGroupChange change) { if (change.hasNewAvatar()) { builder.setAvatar(change.getNewAvatar().getValue()); } + } + protected static void applyModifyDisappearingMessagesTimerAction(DecryptedGroup.Builder builder, DecryptedGroupChange change) { if (change.hasNewTimer()) { builder.setDisappearingMessagesTimer(change.getNewTimer()); } + } + protected static void applyModifyAttributesAccessControlAction(DecryptedGroup.Builder builder, DecryptedGroupChange change) { if (change.getNewAttributeAccess() != AccessControl.AccessRequired.UNKNOWN) { builder.setAccessControl(AccessControl.newBuilder(builder.getAccessControl()) .setAttributesValue(change.getNewAttributeAccessValue()) .build()); } - - if (change.getNewMemberAccess() != AccessControl.AccessRequired.UNKNOWN) { - builder.setAccessControl(AccessControl.newBuilder(builder.getAccessControl()) - .setMembersValue(change.getNewMemberAccessValue()) - .build()); - } - - removePendingMembersNowInGroup(builder); - - return builder.setRevision(change.getRevision()).build(); } - static void removePendingMembersNowInGroup(DecryptedGroup.Builder builder) { + protected static void applyModifyMembersAccessControlAction(DecryptedGroup.Builder builder, DecryptedGroupChange change) { + if (change.getNewMemberAccess() != AccessControl.AccessRequired.UNKNOWN) { + builder.setAccessControl(AccessControl.newBuilder(builder.getAccessControl()) + .setMembersValue(change.getNewMemberAccessValue()) + .build()); + } + } + + private static DecryptedMember withNewProfileKey(DecryptedMember member, ByteString profileKey) { + return DecryptedMember.newBuilder(member) + .setProfileKey(profileKey) + .build(); + } + + private static Set getMemberUuidSet(List membersList) { + Set memberUuids = new HashSet<>(membersList.size()); + + for (DecryptedMember members : membersList) { + memberUuids.add(members.getUuid()); + } + + return memberUuids; + } + + private static Set getPendingMemberCipherTextSet(List pendingMemberList) { + Set pendingMemberCipherTexts = new HashSet<>(pendingMemberList.size()); + + for (DecryptedPendingMember pendingMember : pendingMemberList) { + pendingMemberCipherTexts.add(pendingMember.getUuidCipherText()); + } + + return pendingMemberCipherTexts; + } + + private static void removePendingMembersNowInGroup(DecryptedGroup.Builder builder) { Set allMembers = membersToUuidByteStringSet(builder.getMembersList()); for (int i = builder.getPendingMembersCount() - 1; i >= 0; i--) { diff --git a/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtil_apply_Test.java b/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtil_apply_Test.java index 4b01f969a5..df1cfe2c85 100644 --- a/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtil_apply_Test.java +++ b/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtil_apply_Test.java @@ -107,19 +107,25 @@ public final class DecryptedGroupUtil_apply_Test { newGroup); } - @Test(expected = NotAbleToApplyGroupV2ChangeException.class) + @Test public void apply_remove_members_not_found() throws NotAbleToApplyGroupV2ChangeException { DecryptedMember member1 = member(UUID.randomUUID()); DecryptedMember member2 = member(UUID.randomUUID()); - DecryptedGroupUtil.apply(DecryptedGroup.newBuilder() - .setRevision(13) - .addMembers(member1) - .build(), - DecryptedGroupChange.newBuilder() - .setRevision(14) - .addDeleteMembers(member2.getUuid()) - .build()); + DecryptedGroup newGroup = DecryptedGroupUtil.apply(DecryptedGroup.newBuilder() + .setRevision(13) + .addMembers(member1) + .build(), + DecryptedGroupChange.newBuilder() + .setRevision(14) + .addDeleteMembers(member2.getUuid()) + .build()); + + assertEquals(DecryptedGroup.newBuilder() + .addMembers(member1) + .setRevision(14) + .build(), + newGroup); } @Test @@ -279,6 +285,47 @@ public final class DecryptedGroupUtil_apply_Test { newGroup); } + @Test + public void apply_new_pending_member_already_pending() throws NotAbleToApplyGroupV2ChangeException { + DecryptedMember member1 = member(UUID.randomUUID()); + DecryptedPendingMember pending = pendingMember(UUID.randomUUID()); + + DecryptedGroup newGroup = DecryptedGroupUtil.apply(DecryptedGroup.newBuilder() + .setRevision(10) + .addMembers(member1) + .addPendingMembers(pending) + .build(), + DecryptedGroupChange.newBuilder() + .setRevision(11) + .addNewPendingMembers(pending) + .build()); + + assertEquals(DecryptedGroup.newBuilder() + .setRevision(11) + .addMembers(member1) + .addPendingMembers(pending) + .build(), + newGroup); + } + + @Test(expected = NotAbleToApplyGroupV2ChangeException.class) + public void apply_new_pending_member_already_in_group() throws NotAbleToApplyGroupV2ChangeException { + DecryptedMember member1 = member(UUID.randomUUID()); + UUID uuid2 = UUID.randomUUID(); + DecryptedMember member2 = member(uuid2); + DecryptedPendingMember pending2 = pendingMember(uuid2); + + DecryptedGroupUtil.apply(DecryptedGroup.newBuilder() + .setRevision(10) + .addMembers(member1) + .addMembers(member2) + .build(), + DecryptedGroupChange.newBuilder() + .setRevision(11) + .addNewPendingMembers(pending2) + .build()); + } + @Test public void remove_pending_member() throws NotAbleToApplyGroupV2ChangeException { DecryptedMember member1 = member(UUID.randomUUID()); @@ -304,21 +351,27 @@ public final class DecryptedGroupUtil_apply_Test { newGroup); } - @Test(expected = NotAbleToApplyGroupV2ChangeException.class) + @Test public void cannot_remove_pending_member_if_not_in_group() throws NotAbleToApplyGroupV2ChangeException { DecryptedMember member1 = member(UUID.randomUUID()); UUID pendingUuid = UUID.randomUUID(); - DecryptedGroupUtil.apply(DecryptedGroup.newBuilder() - .setRevision(10) - .addMembers(member1) - .build(), - DecryptedGroupChange.newBuilder() - .setRevision(11) - .addDeletePendingMembers(DecryptedPendingMemberRemoval.newBuilder() - .setUuidCipherText(ProtoTestUtils.encrypt(pendingUuid)) - .build()) - .build()); + DecryptedGroup newGroup = DecryptedGroupUtil.apply(DecryptedGroup.newBuilder() + .setRevision(10) + .addMembers(member1) + .build(), + DecryptedGroupChange.newBuilder() + .setRevision(11) + .addDeletePendingMembers(DecryptedPendingMemberRemoval.newBuilder() + .setUuidCipherText(ProtoTestUtils.encrypt(pendingUuid)) + .build()) + .build()); + + assertEquals(DecryptedGroup.newBuilder() + .setRevision(11) + .addMembers(member1) + .build(), + newGroup); } @Test