From e6ac40a07caf7aec22e2abaffc71df3c7f52a988 Mon Sep 17 00:00:00 2001 From: Cody Henthorne Date: Fri, 18 Mar 2022 11:00:23 -0400 Subject: [PATCH] Fix various corner case block/reject join request bugs. --- .../conversation/ConversationFragment.java | 2 + .../conversation/ConversationUpdateItem.java | 68 ++++++++++++++----- .../securesms/groups/GroupManager.java | 16 ++++- .../securesms/groups/GroupManagerV2.java | 4 +- .../securesms/groups/LiveGroup.java | 6 ++ app/src/main/res/values/strings.xml | 2 + .../api/groupsv2/GroupsV2Operations.java | 9 +-- ...roupsV2Operations_decrypt_change_Test.java | 2 +- 8 files changed, 83 insertions(+), 26 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationFragment.java b/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationFragment.java index b21fb3ed07..86866fe890 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationFragment.java +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationFragment.java @@ -1934,6 +1934,8 @@ public class ConversationFragment extends LoggingFragment implements Multiselect if (result.isFailure()) { int failureReason = GroupErrors.getUserDisplayMessage(((GroupBlockJoinRequestResult.Failure) result).getReason()); Toast.makeText(requireContext(), failureReason, Toast.LENGTH_SHORT).show(); + } else { + Toast.makeText(requireContext(), R.string.ConversationFragment__blocked, Toast.LENGTH_SHORT).show(); } }) ); diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationUpdateItem.java b/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationUpdateItem.java index afb16bedad..38ff6118ec 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationUpdateItem.java +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationUpdateItem.java @@ -17,6 +17,7 @@ import androidx.core.content.ContextCompat; import androidx.lifecycle.LifecycleOwner; import androidx.lifecycle.LiveData; import androidx.lifecycle.Observer; +import androidx.lifecycle.Transformations; import com.google.android.material.button.MaterialButton; import com.google.common.collect.Sets; @@ -57,7 +58,9 @@ import java.util.Locale; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.UUID; import java.util.concurrent.ExecutionException; +import java.util.stream.Collectors; public final class ConversationUpdateItem extends FrameLayout implements BindableConversationItem @@ -85,7 +88,7 @@ public final class ConversationUpdateItem extends FrameLayout private final PresentOnChange presentOnChange = new PresentOnChange(); private final RecipientObserverManager senderObserver = new RecipientObserverManager(presentOnChange); private final RecipientObserverManager groupObserver = new RecipientObserverManager(presentOnChange); - private final GroupDataManager groupData = new GroupDataManager(presentOnChange); + private final GroupDataManager groupData = new GroupDataManager(); public ConversationUpdateItem(Context context) { super(context); @@ -272,42 +275,68 @@ public final class ConversationUpdateItem extends FrameLayout private final class GroupDataManager { - private final Observer recipientObserver; - private final Observer isSelfAdminSetter; + private final Observer updater; - private LiveGroup liveGroup; - private LiveData liveIsSelfAdmin; - private boolean isSelfAdmin; - private Recipient conversationRecipient; + private LiveGroup liveGroup; + private LiveData liveIsSelfAdmin; + private LiveData> liveBannedMembers; + private LiveData> liveFullMembers; + private Recipient conversationRecipient; - GroupDataManager(@NonNull Observer observer) { - this.recipientObserver = observer; - this.isSelfAdminSetter = isSelfAdmin -> { - this.isSelfAdmin = isSelfAdmin; - present(conversationMessage, nextMessageRecord, conversationRecipient, isMessageRequestAccepted); - }; + GroupDataManager() { + this.updater = unused -> update(); } public void observe(@NonNull LifecycleOwner lifecycleOwner, @Nullable Recipient recipient) { if (liveGroup != null) { - liveIsSelfAdmin.removeObserver(isSelfAdminSetter); - liveIsSelfAdmin = null; + liveIsSelfAdmin.removeObserver(updater); + liveBannedMembers.removeObserver(updater); + liveFullMembers.removeObserver(updater); } if (recipient != null) { conversationRecipient = recipient; liveGroup = new LiveGroup(recipient.requireGroupId()); liveIsSelfAdmin = liveGroup.isSelfAdmin(); + liveBannedMembers = liveGroup.getBannedMembers(); + liveFullMembers = Transformations.map(liveGroup.getFullMembers(), + members -> members.stream() + .map(m -> m.getMember().requireServiceId().uuid()) + .collect(Collectors.toSet())); - liveIsSelfAdmin.observe(lifecycleOwner, isSelfAdminSetter); + liveIsSelfAdmin.observe(lifecycleOwner, updater); + liveBannedMembers.observe(lifecycleOwner, updater); + liveFullMembers.observe(lifecycleOwner, updater); } else { conversationRecipient = null; liveGroup = null; + liveBannedMembers = null; + liveIsSelfAdmin = null; } } public boolean isSelfAdmin() { - return isSelfAdmin; + return liveIsSelfAdmin.getValue() != null ? liveIsSelfAdmin.getValue() : false; + } + + public boolean isBanned(Recipient recipient) { + Set bannedMembers = liveBannedMembers.getValue(); + if (bannedMembers != null) { + return recipient.getServiceId().isPresent() && bannedMembers.contains(recipient.requireServiceId().uuid()); + } + return false; + } + + public boolean isFullMember(Recipient recipient) { + Set members = liveFullMembers.getValue(); + if (members != null) { + return recipient.getServiceId().isPresent() && members.contains(recipient.requireServiceId().uuid()); + } + return false; + } + + private void update() { + present(conversationMessage, nextMessageRecord, conversationRecipient, isMessageRequestAccepted); } } @@ -469,7 +498,10 @@ public final class ConversationUpdateItem extends FrameLayout eventListener.onChangeNumberUpdateContact(conversationMessage.getMessageRecord().getIndividualRecipient()); } }); - } else if (conversationMessage.getMessageRecord().isCollapsedGroupV2JoinUpdate() && groupData.isSelfAdmin()) { + } else if (conversationMessage.getMessageRecord().isCollapsedGroupV2JoinUpdate() && + groupData.isSelfAdmin() && + !groupData.isBanned(conversationMessage.getMessageRecord().getIndividualRecipient()) && + !groupData.isFullMember(conversationMessage.getMessageRecord().getIndividualRecipient())) { actionButton.setText(R.string.ConversationUpdateItem_block_request); actionButton.setVisibility(VISIBLE); actionButton.setOnClickListener(v -> { diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManager.java b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManager.java index 1892e11667..89a784683e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManager.java +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManager.java @@ -6,6 +6,8 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.WorkerThread; +import com.google.protobuf.ByteString; + import org.signal.core.util.logging.Log; import org.signal.storageservice.protos.groups.GroupExternalCredential; import org.signal.storageservice.protos.groups.local.DecryptedGroup; @@ -22,6 +24,7 @@ import org.thoughtcrime.securesms.profiles.AvatarHelper; import org.thoughtcrime.securesms.recipients.Recipient; import org.thoughtcrime.securesms.recipients.RecipientId; import org.whispersystems.signalservice.api.groupsv2.GroupLinkNotActiveException; +import org.whispersystems.signalservice.api.util.UuidUtil; import java.io.IOException; import java.util.Collection; @@ -280,8 +283,19 @@ public final class GroupManager { @NonNull RecipientId recipientId) throws GroupChangeBusyException, IOException, GroupChangeFailedException, GroupNotAMemberException, GroupInsufficientRightsException { + GroupDatabase.GroupRecord groupRecord = SignalDatabase.groups().requireGroup(groupId); + Recipient recipient = Recipient.resolved(recipientId); + + if (groupRecord.requireV2GroupProperties().getBannedMembers().contains(recipient.requireServiceId().uuid())) { + Log.i(TAG, "Attempt to ban already banned recipient: " + recipientId); + return; + } + + ByteString uuid = UuidUtil.toByteString(recipient.requireServiceId().uuid()); + boolean rejectJoinRequest = groupRecord.requireV2GroupProperties().getDecryptedGroup().getRequestingMembersList().stream().anyMatch(m -> m.getUuid().equals(uuid)); + try (GroupManagerV2.GroupEditor editor = new GroupManagerV2(context).edit(groupId.requireV2())) { - editor.ban(Collections.singleton(Recipient.resolved(recipientId).requireServiceId().uuid())); + editor.ban(Collections.singleton(recipient.requireServiceId().uuid()), rejectJoinRequest); } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java index c572b84092..90cb35b554 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java @@ -530,8 +530,8 @@ final class GroupManagerV2 { return commitChangeWithConflictResolution(groupOperations.createAcceptInviteChange(groupCandidate.getProfileKeyCredential().get())); } - public GroupManager.GroupActionResult ban(Set uuids) throws GroupChangeFailedException, GroupNotAMemberException, GroupInsufficientRightsException, IOException { - return commitChangeWithConflictResolution(groupOperations.createBanUuidsChange(uuids)); + public GroupManager.GroupActionResult ban(Set uuids, boolean rejectJoinRequest) throws GroupChangeFailedException, GroupNotAMemberException, GroupInsufficientRightsException, IOException { + return commitChangeWithConflictResolution(groupOperations.createBanUuidsChange(uuids, rejectJoinRequest)); } public GroupManager.GroupActionResult unban(Set uuids) throws GroupChangeFailedException, GroupNotAMemberException, GroupInsufficientRightsException, IOException { diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/LiveGroup.java b/app/src/main/java/org/thoughtcrime/securesms/groups/LiveGroup.java index 862eefc8a6..b8dd5fe60e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/LiveGroup.java +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/LiveGroup.java @@ -32,6 +32,8 @@ import org.whispersystems.signalservice.api.push.ServiceId; import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.Set; +import java.util.UUID; public final class LiveGroup { @@ -141,6 +143,10 @@ public final class LiveGroup { return Transformations.map(groupRecord, g -> g.isAdmin(Recipient.self())); } + public LiveData> getBannedMembers() { + return Transformations.map(groupRecord, g -> g.isV2Group() ? g.requireV2GroupProperties().getBannedMembers() : Collections.emptySet()); + } + public LiveData isActive() { return Transformations.map(groupRecord, GroupDatabase.GroupRecord::isActive); } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 4fa85aa261..6b80957845 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -375,6 +375,8 @@ Block request Cancel + + Blocked Delete selected conversation? diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations.java index 83c5aca2c9..7799b65a55 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations.java @@ -210,7 +210,7 @@ public final class GroupsV2Operations { } public GroupChange.Actions.Builder createRefuseGroupJoinRequest(Set requestsToRemove, boolean alsoBan) { - GroupChange.Actions.Builder actions = alsoBan ? createBanUuidsChange(requestsToRemove) + GroupChange.Actions.Builder actions = alsoBan ? createBanUuidsChange(requestsToRemove, false) : GroupChange.Actions.newBuilder(); for (UUID uuid : requestsToRemove) { @@ -236,7 +236,7 @@ public final class GroupsV2Operations { } public GroupChange.Actions.Builder createRemoveMembersChange(final Set membersToRemove, boolean alsoBan) { - GroupChange.Actions.Builder actions = alsoBan ? createBanUuidsChange(membersToRemove) + GroupChange.Actions.Builder actions = alsoBan ? createBanUuidsChange(membersToRemove, false) : GroupChange.Actions.newBuilder(); for (UUID remove: membersToRemove) { @@ -350,8 +350,9 @@ public final class GroupsV2Operations { .setAnnouncementsOnly(isAnnouncementGroup)); } - public GroupChange.Actions.Builder createBanUuidsChange(Set banUuids) { - GroupChange.Actions.Builder builder = GroupChange.Actions.newBuilder(); + public GroupChange.Actions.Builder createBanUuidsChange(Set banUuids, boolean rejectJoinRequest) { + GroupChange.Actions.Builder builder = rejectJoinRequest ? createRefuseGroupJoinRequest(banUuids, false) + : GroupChange.Actions.newBuilder(); for (UUID uuid : banUuids) { builder.addAddBannedMembers(GroupChange.Actions.AddBannedMemberAction.newBuilder().setAdded(BannedMember.newBuilder().setUserId(encryptUuid(uuid)).build())); diff --git a/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations_decrypt_change_Test.java b/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations_decrypt_change_Test.java index 03e480b592..09a5a709e3 100644 --- a/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations_decrypt_change_Test.java +++ b/libsignal/service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations_decrypt_change_Test.java @@ -393,7 +393,7 @@ public final class GroupsV2Operations_decrypt_change_Test { public void can_decrypt_member_bans_field22() { UUID ban = UUID.randomUUID(); - assertDecryption(groupOperations.createBanUuidsChange(Collections.singleton(ban)) + assertDecryption(groupOperations.createBanUuidsChange(Collections.singleton(ban), false) .setRevision(13), DecryptedGroupChange.newBuilder() .setRevision(13)