From 810ccf8e94a7ede6f2ca49b4288c171643ad6c23 Mon Sep 17 00:00:00 2001 From: Alan Evans Date: Thu, 6 Aug 2020 15:06:15 -0300 Subject: [PATCH] Improve GV2 Invitation revoke experience. --- .../model/GroupsV2UpdateMessageProducer.java | 12 +- .../MessageRequestRepository.java | 46 ++++++-- .../MessageRequestViewModel.java | 4 +- .../securesms/mms/MessageGroupContext.java | 1 + app/src/main/res/values/strings.xml | 3 +- .../GroupsV2UpdateMessageProducerTest.java | 36 +++++- .../securesms/groups/v2/ChangeBuilder.java | 7 +- .../api/groupsv2/DecryptedGroupUtil.java | 46 ++++++-- .../api/groupsv2/DecryptedGroupUtilTest.java | 111 ++++++++++++++++++ 9 files changed, 241 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducer.java b/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducer.java index 7b4668a57f..e4e34a875c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducer.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducer.java @@ -297,7 +297,13 @@ final class GroupsV2UpdateMessageProducer { boolean newMemberIsYou = invitee.getUuid().equals(selfUuidBytes); if (newMemberIsYou) { - updates.add(updateDescription(context.getString(R.string.MessageRecord_you_were_invited_to_the_group))); + UUID uuid = UuidUtil.fromByteStringOrUnknown(invitee.getAddedByUuid()); + + if (UuidUtil.UNKNOWN_UUID.equals(uuid)) { + updates.add(updateDescription(context.getString(R.string.MessageRecord_you_were_invited_to_the_group))); + } else { + updates.add(updateDescription(invitee.getAddedByUuid(), editor -> context.getString(R.string.MessageRecord_s_invited_you_to_the_group, editor))); + } } else { notYouInviteCount++; } @@ -320,6 +326,8 @@ final class GroupsV2UpdateMessageProducer { } else { updates.add(updateDescription(context.getString(R.string.MessageRecord_someone_declined_an_invitation_to_the_group))); } + } else if (invitee.getUuid().equals(selfUuidBytes)) { + updates.add(updateDescription(change.getEditor(), editor -> context.getString(R.string.MessageRecord_s_revoked_your_invitation_to_the_group, editor))); } else { notDeclineCount++; } @@ -342,7 +350,7 @@ final class GroupsV2UpdateMessageProducer { boolean inviteeWasYou = invitee.getUuid().equals(selfUuidBytes); if (inviteeWasYou) { - updates.add(updateDescription(context.getString(R.string.MessageRecord_your_invitation_to_the_group_was_revoked))); + updates.add(updateDescription(context.getString(R.string.MessageRecord_an_admin_revoked_your_invitation_to_the_group))); } else { notDeclineCount++; } diff --git a/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestRepository.java b/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestRepository.java index 645977f8ba..43c17d4f7a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestRepository.java +++ b/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestRepository.java @@ -3,6 +3,7 @@ package org.thoughtcrime.securesms.messagerequests; import android.content.Context; import androidx.annotation.NonNull; +import androidx.annotation.WorkerThread; import androidx.core.util.Consumer; import org.signal.storageservice.protos.groups.local.DecryptedGroup; @@ -67,15 +68,29 @@ final class MessageRequestRepository { } void getMessageRequestState(@NonNull Recipient recipient, long threadId, @NonNull Consumer state) { - executor.execute(() -> { - if (!RecipientUtil.isMessageRequestAccepted(context, threadId)) { - state.accept(MessageRequestState.UNACCEPTED); - } else if (RecipientUtil.isPreMessageRequestThread(context, threadId) && !RecipientUtil.isLegacyProfileSharingAccepted(recipient)) { - state.accept(MessageRequestState.LEGACY); - } else { - state.accept(MessageRequestState.ACCEPTED); + executor.execute(() -> state.accept(findMessageRequestState(recipient, threadId))); + } + + @WorkerThread + private MessageRequestState findMessageRequestState(@NonNull Recipient recipient, long threadId) { + if (!RecipientUtil.isMessageRequestAccepted(context, threadId)) { + if (recipient.isGroup()) { + GroupDatabase.MemberLevel memberLevel = DatabaseFactory.getGroupDatabase(context) + .getGroup(recipient.getId()) + .transform(g -> g.memberLevel(Recipient.self())) + .or(GroupDatabase.MemberLevel.NOT_A_MEMBER); + + if (memberLevel == GroupDatabase.MemberLevel.NOT_A_MEMBER) { + return MessageRequestState.NOT_REQUIRED; + } } - }); + + return MessageRequestState.REQUIRED; + } else if (RecipientUtil.isPreMessageRequestThread(context, threadId) && !RecipientUtil.isLegacyProfileSharingAccepted(recipient)) { + return MessageRequestState.LEGACY; + } else { + return MessageRequestState.NOT_REQUIRED; + } } void acceptMessageRequest(@NonNull LiveRecipient liveRecipient, @@ -218,6 +233,19 @@ final class MessageRequestRepository { } enum MessageRequestState { - ACCEPTED, UNACCEPTED, LEGACY + /** + * Message request permission does not need to be gained at this time. + *

+ * Either: + * - Explicit message request has been accepted, or; + * - Did not need to be shown because they are a contact etc, or; + * - It's a group that they are no longer in or invited to. + */ + NOT_REQUIRED, + + /** Explicit message request permission is required. */ + REQUIRED, + + LEGACY } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestViewModel.java b/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestViewModel.java index 88e472017b..749f1bef9a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestViewModel.java +++ b/app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestViewModel.java @@ -161,10 +161,10 @@ public class MessageRequestViewModel extends ViewModel { repository.getMessageRequestState(recipient, threadId, accepted -> { switch (accepted) { - case ACCEPTED: + case NOT_REQUIRED: displayState.postValue(DisplayState.DISPLAY_NONE); break; - case UNACCEPTED: + case REQUIRED: displayState.postValue(DisplayState.DISPLAY_MESSAGE_REQUEST); break; case LEGACY: diff --git a/app/src/main/java/org/thoughtcrime/securesms/mms/MessageGroupContext.java b/app/src/main/java/org/thoughtcrime/securesms/mms/MessageGroupContext.java index c2b4c8b19e..8ae675d6b3 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/mms/MessageGroupContext.java +++ b/app/src/main/java/org/thoughtcrime/securesms/mms/MessageGroupContext.java @@ -168,6 +168,7 @@ public final class MessageGroupContext { memberUuids.addAll(DecryptedGroupUtil.membersToUuidList(decryptedGroupV2Context.getGroupState().getMembersList())); memberUuids.addAll(DecryptedGroupUtil.pendingToUuidList(decryptedGroupV2Context.getGroupState().getPendingMembersList())); memberUuids.addAll(DecryptedGroupUtil.removedMembersUuidList(decryptedGroupV2Context.getChange())); + memberUuids.addAll(DecryptedGroupUtil.removedPendingMembersUuidList(decryptedGroupV2Context.getChange())); return UuidUtil.filterKnown(memberUuids); } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index bff77016f4..7060828d3d 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -865,7 +865,8 @@ Someone declined an invitation to the group. You declined the invitation to the group. - Your invitation to the group was revoked. + %1$s revoked your invitation to the group. + An admin revoked your invitation to the group. An invitation to the group was revoked. %1$d invitations to the group were revoked. diff --git a/app/src/test/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducerTest.java b/app/src/test/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducerTest.java index f6fd9d18bc..111d095330 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducerTest.java +++ b/app/src/test/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducerTest.java @@ -391,7 +391,16 @@ public final class GroupsV2UpdateMessageProducerTest { } @Test - public void unknown_invited_you() { + public void unknown_editor_but_known_invitee_invited_you() { + DecryptedGroupChange change = changeByUnknown() + .inviteBy(you, alice) + .build(); + + assertThat(describeChange(change), is(singletonList("Alice invited you to the group."))); + } + + @Test + public void unknown_editor_and_unknown_inviter_invited_you() { DecryptedGroupChange change = changeByUnknown() .invite(you) .build(); @@ -430,6 +439,18 @@ public final class GroupsV2UpdateMessageProducerTest { assertThat(describeChange(change), is(Arrays.asList("You were invited to the group.", "3 people were invited to the group."))); } + @Test + public void unknown_editor_invited_3_persons_and_you_inviter_known() { + DecryptedGroupChange change = changeByUnknown() + .invite(alice) + .inviteBy(you, bob) + .invite(UUID.randomUUID()) + .invite(UUID.randomUUID()) + .build(); + + assertThat(describeChange(change), is(Arrays.asList("Bob invited you to the group.", "3 people were invited to the group."))); + } + // Member invitation revocation @Test @@ -494,7 +515,7 @@ public final class GroupsV2UpdateMessageProducerTest { .uninvite(you) .build(); - assertThat(describeChange(change), is(singletonList("Your invitation to the group was revoked."))); + assertThat(describeChange(change), is(singletonList("An admin revoked your invitation to the group."))); } @Test @@ -525,7 +546,16 @@ public final class GroupsV2UpdateMessageProducerTest { .uninvite(UUID.randomUUID()) .build(); - assertThat(describeChange(change), is(Arrays.asList("Your invitation to the group was revoked.", "3 invitations to the group were revoked."))); + assertThat(describeChange(change), is(Arrays.asList("An admin revoked your invitation to the group.", "3 invitations to the group were revoked."))); + } + + @Test + public void your_invite_was_revoked_by_known_member() { + DecryptedGroupChange change = changeBy(bob) + .uninvite(you) + .build(); + + assertThat(describeChange(change), is(singletonList("Bob revoked your invitation to the group."))); } // Promote pending members diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/v2/ChangeBuilder.java b/app/src/test/java/org/thoughtcrime/securesms/groups/v2/ChangeBuilder.java index 06c9cc776b..986510eb30 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/groups/v2/ChangeBuilder.java +++ b/app/src/test/java/org/thoughtcrime/securesms/groups/v2/ChangeBuilder.java @@ -72,8 +72,13 @@ public final class ChangeBuilder { } public ChangeBuilder invite(@NonNull UUID potentialMember) { + return inviteBy(potentialMember, UuidUtil.UNKNOWN_UUID); + } + + public ChangeBuilder inviteBy(@NonNull UUID potentialMember, @NonNull UUID inviter) { builder.addNewPendingMembers(DecryptedPendingMember.newBuilder() - .setUuid(UuidUtil.toByteString(potentialMember))); + .setUuid(UuidUtil.toByteString(potentialMember)) + .setAddedByUuid(UuidUtil.toByteString(inviter))); return this; } 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 6b574324df..0ebc1cf01a 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 @@ -10,7 +10,6 @@ import org.signal.storageservice.protos.groups.local.DecryptedMember; 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; @@ -43,7 +42,11 @@ public final class DecryptedGroupUtil { ArrayList uuidList = new ArrayList<>(membersList.size()); for (DecryptedMember member : membersList) { - uuidList.add(toUuid(member)); + UUID uuid = toUuid(member); + + if (!UuidUtil.UNKNOWN_UUID.equals(uuid)) { + uuidList.add(uuid); + } } return uuidList; @@ -59,6 +62,9 @@ public final class DecryptedGroupUtil { return uuidList; } + /** + * Can return non-decryptable member UUIDs as {@link UuidUtil#UNKNOWN_UUID}. + */ public static ArrayList pendingToUuidList(Collection membersList) { ArrayList uuidList = new ArrayList<>(membersList.size()); @@ -69,11 +75,37 @@ public final class DecryptedGroupUtil { return uuidList; } + /** + * Will not return any non-decryptable member UUIDs. + */ public static ArrayList removedMembersUuidList(DecryptedGroupChange groupChange) { - ArrayList uuidList = new ArrayList<>(groupChange.getDeleteMembersCount()); + List deletedMembers = groupChange.getDeleteMembersList(); + ArrayList uuidList = new ArrayList<>(deletedMembers.size()); - for (ByteString member : groupChange.getDeleteMembersList()) { - uuidList.add(toUuid(member)); + for (ByteString member : deletedMembers) { + UUID uuid = toUuid(member); + + if (!UuidUtil.UNKNOWN_UUID.equals(uuid)) { + uuidList.add(uuid); + } + } + + return uuidList; + } + + /** + * Will not return any non-decryptable member UUIDs. + */ + public static ArrayList removedPendingMembersUuidList(DecryptedGroupChange groupChange) { + List deletedPendingMembers = groupChange.getDeletePendingMembersList(); + ArrayList uuidList = new ArrayList<>(deletedPendingMembers.size()); + + for (DecryptedPendingMemberRemoval member : deletedPendingMembers) { + UUID uuid = toUuid(member.getUuid()); + + if(!UuidUtil.UNKNOWN_UUID.equals(uuid)) { + uuidList.add(uuid); + } } return uuidList; @@ -87,8 +119,8 @@ public final class DecryptedGroupUtil { return toUuid(member.getUuid()); } - private static UUID toUuid(ByteString member) { - return UUIDUtil.deserialize(member.toByteArray()); + private static UUID toUuid(ByteString memberUuid) { + return UuidUtil.fromByteStringOrUnknown(memberUuid); } /** diff --git a/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtilTest.java b/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtilTest.java index 672ec4498c..9f112aa7e2 100644 --- a/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtilTest.java +++ b/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/DecryptedGroupUtilTest.java @@ -5,12 +5,25 @@ import com.google.protobuf.ByteString; import org.junit.Test; import org.signal.storageservice.protos.groups.local.DecryptedGroupChange; import org.signal.storageservice.protos.groups.local.DecryptedMember; +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.signalservice.api.util.UuidUtil; +import org.whispersystems.signalservice.internal.util.Util; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; import java.util.UUID; +import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; +import static java.util.Collections.singleton; +import static java.util.Collections.singletonList; +import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; public final class DecryptedGroupUtilTest { @@ -26,6 +39,17 @@ public final class DecryptedGroupUtilTest { assertEquals(uuid, parsed); } + @Test + public void can_extract_uuid_from_bad_decrypted_member() { + DecryptedMember decryptedMember = DecryptedMember.newBuilder() + .setUuid(ByteString.copyFrom(new byte[0])) + .build(); + + UUID parsed = DecryptedGroupUtil.toUuid(decryptedMember); + + assertEquals(UuidUtil.UNKNOWN_UUID, parsed); + } + @Test public void can_extract_editor_uuid_from_decrypted_group_change() { UUID uuid = UUID.randomUUID(); @@ -39,4 +63,91 @@ public final class DecryptedGroupUtilTest { assertEquals(uuid, parsed); } + @Test + public void can_extract_uuid_from_decrypted_pending_member() { + UUID uuid = UUID.randomUUID(); + DecryptedPendingMember decryptedMember = DecryptedPendingMember.newBuilder() + .setUuid(UuidUtil.toByteString(uuid)) + .build(); + + UUID parsed = DecryptedGroupUtil.toUuid(decryptedMember); + + assertEquals(uuid, parsed); + } + + @Test + public void can_extract_uuid_from_bad_decrypted_pending_member() { + DecryptedPendingMember decryptedMember = DecryptedPendingMember.newBuilder() + .setUuid(ByteString.copyFrom(Util.getSecretBytes(17))) + .build(); + + UUID parsed = DecryptedGroupUtil.toUuid(decryptedMember); + + assertEquals(UuidUtil.UNKNOWN_UUID, parsed); + } + + @Test + public void can_extract_uuids_for_all_pending_including_bad_entries() { + UUID uuid1 = UUID.randomUUID(); + UUID uuid2 = UUID.randomUUID(); + DecryptedPendingMember decryptedMember1 = DecryptedPendingMember.newBuilder() + .setUuid(UuidUtil.toByteString(uuid1)) + .build(); + DecryptedPendingMember decryptedMember2 = DecryptedPendingMember.newBuilder() + .setUuid(UuidUtil.toByteString(uuid2)) + .build(); + DecryptedPendingMember decryptedMember3 = DecryptedPendingMember.newBuilder() + .setUuid(ByteString.copyFrom(Util.getSecretBytes(17))) + .build(); + + DecryptedGroupChange groupChange = DecryptedGroupChange.newBuilder() + .addNewPendingMembers(decryptedMember1) + .addNewPendingMembers(decryptedMember2) + .addNewPendingMembers(decryptedMember3) + .build(); + + List pendingUuids = DecryptedGroupUtil.pendingToUuidList(groupChange.getNewPendingMembersList()); + + assertThat(pendingUuids, is(asList(uuid1, uuid2, UuidUtil.UNKNOWN_UUID))); + } + + @Test + public void can_extract_uuids_for_all_deleted_pending_excluding_bad_entries() { + UUID uuid1 = UUID.randomUUID(); + UUID uuid2 = UUID.randomUUID(); + DecryptedPendingMemberRemoval decryptedMember1 = DecryptedPendingMemberRemoval.newBuilder() + .setUuid(UuidUtil.toByteString(uuid1)) + .build(); + DecryptedPendingMemberRemoval decryptedMember2 = DecryptedPendingMemberRemoval.newBuilder() + .setUuid(UuidUtil.toByteString(uuid2)) + .build(); + DecryptedPendingMemberRemoval decryptedMember3 = DecryptedPendingMemberRemoval.newBuilder() + .setUuid(ByteString.copyFrom(Util.getSecretBytes(17))) + .build(); + + DecryptedGroupChange groupChange = DecryptedGroupChange.newBuilder() + .addDeletePendingMembers(decryptedMember1) + .addDeletePendingMembers(decryptedMember2) + .addDeletePendingMembers(decryptedMember3) + .build(); + + List removedUuids = DecryptedGroupUtil.removedPendingMembersUuidList(groupChange); + + assertThat(removedUuids, is(asList(uuid1, uuid2))); + } + + @Test + public void can_extract_uuids_for_all_deleted_members_excluding_bad_entries() { + UUID uuid1 = UUID.randomUUID(); + UUID uuid2 = UUID.randomUUID(); + DecryptedGroupChange groupChange = DecryptedGroupChange.newBuilder() + .addDeleteMembers(UuidUtil.toByteString(uuid1)) + .addDeleteMembers(UuidUtil.toByteString(uuid2)) + .addDeleteMembers(ByteString.copyFrom(Util.getSecretBytes(17))) + .build(); + + List removedUuids = DecryptedGroupUtil.removedMembersUuidList(groupChange); + + assertThat(removedUuids, is(asList(uuid1, uuid2))); + } }