From 3255fc3dfea2ddc9bde74790bd53dad32b486c65 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 8 Jul 2020 08:53:44 -0300 Subject: [PATCH] Add pending members. --- ...rsationViewController+MessageRequest.swift | 2 +- .../AddGroupMembersViewController.swift | 24 +++++++- .../groups/GroupsV2ChangeSetImpl.swift | 61 ++++++++++--------- SignalMessaging/groups/GroupsV2Changes.swift | 2 +- SignalMessaging/groups/GroupsV2Protos.swift | 4 +- .../Contacts/Threads/TSGroupThread+OWS.swift | 2 +- .../TSInfoMessage+GroupUpdateCopy.swift | 20 +++++- .../src/groups/GroupManager.swift | 25 +++++--- .../src/groups/GroupMembership.swift | 30 +++++---- SignalServiceKit/src/groups/GroupsV2.swift | 2 +- 10 files changed, 114 insertions(+), 58 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController+MessageRequest.swift b/Signal/src/ViewControllers/ConversationView/ConversationViewController+MessageRequest.swift index ba48e69d92..5c2454304e 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController+MessageRequest.swift +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController+MessageRequest.swift @@ -103,7 +103,7 @@ extension ConversationViewController: MessageRequestDelegate { return } let groupMembership = groupThread.groupModel.groupMembership - guard groupMembership.isPending(localAddress) else { + guard groupMembership.isPendingMember(localAddress) else { owsFailDebug("Can't reject invite if not pending.") return } diff --git a/Signal/src/ViewControllers/ThreadSettings/AddGroupMembersViewController.swift b/Signal/src/ViewControllers/ThreadSettings/AddGroupMembersViewController.swift index c6dc4d4f19..51efe35674 100644 --- a/Signal/src/ViewControllers/ThreadSettings/AddGroupMembersViewController.swift +++ b/Signal/src/ViewControllers/ThreadSettings/AddGroupMembersViewController.swift @@ -28,6 +28,10 @@ public class AddGroupMembersViewController: BaseGroupMemberViewController { return Environment.shared.contactsManager } + private var groupsV2: GroupsV2Swift { + return SSKEnvironment.shared.groupsV2 as! GroupsV2Swift + } + // MARK: - weak var addGroupMembersViewControllerDelegate: AddGroupMembersViewControllerDelegate? @@ -133,11 +137,12 @@ private extension AddGroupMembersViewController { return nil } for address in addresses { - guard !oldGroupMembership.isPendingOrNonPendingMember(address) else { + guard !oldGroupMembership.isNonPendingMember(address) else { owsFailDebug("Recipient is already in group.") continue } // GroupManager will separate out members as pending if necessary. + groupMembershipBuilder.remove(address) groupMembershipBuilder.addNonPendingMember(address, role: .normal) } builder.groupMembership = groupMembershipBuilder.build() @@ -265,7 +270,22 @@ extension AddGroupMembersViewController: GroupMemberViewDelegate { owsFailDebug("Invalid recipient.") return false } - return oldGroupModel.groupMembership.isPendingOrNonPendingMember(address) + if oldGroupModel.groupMembership.isNonPendingMember(address) { + return true + } + if oldGroupModel.groupMembership.isPendingMember(address) { + // We can "add" pending members if they support gv2 + // and we know their profile key credential. + let canAddMember = databaseStorage.read { transaction -> Bool in + guard GroupManager.doesUserSupportGroupsV2(address: address, transaction: transaction) else { + return false + } + return self.groupsV2.hasProfileKeyCredential(for: address, transaction: transaction) + } + + return !canAddMember + } + return false } func groupMemberViewIsGroupsV2Required() -> Bool { diff --git a/SignalMessaging/groups/GroupsV2ChangeSetImpl.swift b/SignalMessaging/groups/GroupsV2ChangeSetImpl.swift index e7d429a5c9..06597ca3bb 100644 --- a/SignalMessaging/groups/GroupsV2ChangeSetImpl.swift +++ b/SignalMessaging/groups/GroupsV2ChangeSetImpl.swift @@ -70,12 +70,12 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { private var membersToChangeRole = [UUID: TSGroupMemberRole]() private var pendingMembersToAdd = [UUID: TSGroupMemberRole]() private var invalidInvitesToRemove = [Data: InvalidInvite]() + private var pendingMembersToPromote = [UUID]() // These access properties should only be set if the value is changing. private var accessForMembers: GroupV2Access? private var accessForAttributes: GroupV2Access? - private var shouldAcceptInvite = false private var shouldLeaveGroupDeclineInvite = false private var shouldRevokeInvalidInvites = false @@ -139,8 +139,8 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { let newUserUuids = Set(newGroupMembership.allUsers.compactMap { $0.uuid }) for uuid in newUserUuids.subtracting(oldUserUuids) { - let isAdministrator = newGroupMembership.isAdministrator(SignalServiceAddress(uuid: uuid)) - let isPending = newGroupMembership.isPending(SignalServiceAddress(uuid: uuid)) + let isAdministrator = newGroupMembership.isAdministrator(uuid) + let isPending = newGroupMembership.isPendingMember(uuid) let role: TSGroupMemberRole = isAdministrator ? .administrator : .normal if isPending { addPendingMember(uuid, role: role) @@ -159,12 +159,19 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { } } + for uuid in oldUserUuids.intersection(newUserUuids) { + if oldGroupMembership.isPendingMember(uuid), + newGroupMembership.isNonPendingMember(uuid) { + Logger.verbose("promotePendingMember") + addMember(uuid, role: .normal) + } + } + let oldMemberUuids = Set(oldGroupMembership.nonPendingMembers.compactMap { $0.uuid }) let newMemberUuids = Set(newGroupMembership.nonPendingMembers.compactMap { $0.uuid }) for uuid in oldMemberUuids.intersection(newMemberUuids) { - let address = SignalServiceAddress(uuid: uuid) - let oldIsAdministrator = oldGroupMembership.isAdministrator(address) - let newIsAdministrator = newGroupMembership.isAdministrator(address) + let oldIsAdministrator = oldGroupMembership.isAdministrator(uuid) + let newIsAdministrator = newGroupMembership.isAdministrator(uuid) guard oldIsAdministrator != newIsAdministrator else { continue } @@ -227,6 +234,12 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { membersToRemove.append(uuid) } + @objc + public func promotePendingMember(_ uuid: UUID) { + assert(!pendingMembersToPromote.contains(uuid)) + pendingMembersToPromote.append(uuid) + } + public func changeRoleForMember(_ uuid: UUID, role: TSGroupMemberRole) { assert(membersToChangeRole[uuid] == nil) membersToChangeRole[uuid] = role @@ -237,11 +250,6 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { pendingMembersToAdd[uuid] = role } - public func setShouldAcceptInvite() { - assert(!shouldAcceptInvite) - shouldAcceptInvite = true - } - public func setShouldLeaveGroupDeclineInvite() { assert(!shouldLeaveGroupDeclineInvite) shouldLeaveGroupDeclineInvite = true @@ -306,13 +314,11 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { // NOTE: We don't (and can't) gather profile key credentials for pending members. var uuidsForProfileKeyCredentials = Set() uuidsForProfileKeyCredentials.formUnion(membersToAdd.keys) + uuidsForProfileKeyCredentials.formUnion(pendingMembersToPromote) // This should be redundant, but we'll also double-check that we have // the local profile key credential. uuidsForProfileKeyCredentials.insert(localUuid) let addressesForProfileKeyCredentials: [SignalServiceAddress] = uuidsForProfileKeyCredentials.map { SignalServiceAddress(uuid: $0) } - if shouldAcceptInvite || shouldUpdateLocalProfileKey { - uuidsForProfileKeyCredentials.insert(localUuid) - } return firstly { groupsV2Impl.tryToEnsureProfileKeyCredentials(for: addressesForProfileKeyCredentials) @@ -372,7 +378,6 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { guard let localUuid = tsAccountManager.localUuid else { throw OWSAssertionError("Missing localUuid.") } - let localAddress = SignalServiceAddress(uuid: localUuid) let oldRevision = currentGroupModel.revision let newRevision = oldRevision + 1 @@ -414,8 +419,8 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { let currentGroupMembership = currentGroupModel.groupMembership for (uuid, role) in self.membersToAdd { - guard !currentGroupMembership.isPendingOrNonPendingMember(uuid) else { - // Another user has already added or invited this member. + guard !currentGroupMembership.isNonPendingMember(uuid) else { + // Another user has already added this member. // They may have been added with a different role. // We don't treat that as a conflict. continue @@ -443,7 +448,7 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { actionsBuilder.addDeleteMembers(try actionBuilder.build()) didChange = true - if currentGroupMembership.isAdministrator(SignalServiceAddress(uuid: uuid)) { + if currentGroupMembership.isAdministrator(uuid) { nonPendingAdministratorCount -= 1 } allMemberCount -= 1 @@ -516,7 +521,7 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { // User is no longer a member. throw GroupsV2Error.conflictingChange } - let currentRole = currentGroupMembership.role(for: SignalServiceAddress(uuid: uuid)) + let currentRole = currentGroupMembership.role(for: uuid) guard currentRole != newRole else { // Another user has already modifed the role of this member. // We don't treat that as a conflict. @@ -558,13 +563,13 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { } } - if self.shouldAcceptInvite { - // Check that we are still invited. - guard currentGroupMembership.pendingMembers.contains(localAddress) else { + for uuid in pendingMembersToPromote { + // Check that pending member is still invited. + guard currentGroupMembership.isPendingMember(uuid) else { throw GroupsV2Error.redundantChange } - guard let profileKeyCredential = profileKeyCredentialMap[localUuid] else { - throw OWSAssertionError("Missing profile key credential: \(localUuid)") + guard let profileKeyCredential = profileKeyCredentialMap[uuid] else { + throw OWSAssertionError("Missing profile key credential: \(uuid)") } let actionBuilder = GroupsProtoGroupChangeActionsPromotePendingMemberAction.builder() actionBuilder.setPresentation(try GroupsV2Protos.presentationData(profileKeyCredential: profileKeyCredential, @@ -574,8 +579,8 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { } if self.shouldLeaveGroupDeclineInvite { - let isLastAdminInV2Group = (currentGroupMembership.isNonPendingMember(localAddress) && - currentGroupMembership.isAdministrator(localAddress) && + let isLastAdminInV2Group = (currentGroupMembership.isNonPendingMember(localUuid) && + currentGroupMembership.isAdministrator(localUuid) && nonPendingAdministratorCount == 1) guard !isLastAdminInV2Group else { // This could happen if the last two admins leave at the same time @@ -584,14 +589,14 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { } // Check that we are still invited or in group. - if currentGroupMembership.pendingMembers.contains(localAddress) { + if currentGroupMembership.isPendingMember(localUuid) { // Decline invite let actionBuilder = GroupsProtoGroupChangeActionsDeletePendingMemberAction.builder() let localUserId = try groupV2Params.userId(forUuid: localUuid) actionBuilder.setDeletedUserID(localUserId) actionsBuilder.addDeletePendingMembers(try actionBuilder.build()) didChange = true - } else if currentGroupMembership.nonPendingMembers.contains(localAddress) { + } else if currentGroupMembership.isNonPendingMember(localUuid) { // Leave group let actionBuilder = GroupsProtoGroupChangeActionsDeleteMemberAction.builder() let localUserId = try groupV2Params.userId(forUuid: localUuid) diff --git a/SignalMessaging/groups/GroupsV2Changes.swift b/SignalMessaging/groups/GroupsV2Changes.swift index f70bf494be..c5460ee125 100644 --- a/SignalMessaging/groups/GroupsV2Changes.swift +++ b/SignalMessaging/groups/GroupsV2Changes.swift @@ -146,7 +146,7 @@ public class GroupsV2Changes { // the service. This is one. let uuid = try groupV2Params.uuid(forUserId: userId) - guard !oldGroupMembership.isPendingOrNonPendingMember(uuid) else { + guard !oldGroupMembership.isNonPendingMember(uuid) else { throw OWSAssertionError("Invalid membership.") } groupMembershipBuilder.removeInvalidInvite(userId: userId) diff --git a/SignalMessaging/groups/GroupsV2Protos.swift b/SignalMessaging/groups/GroupsV2Protos.swift index 16f6036f40..0e1ddebffe 100644 --- a/SignalMessaging/groups/GroupsV2Protos.swift +++ b/SignalMessaging/groups/GroupsV2Protos.swift @@ -110,7 +110,7 @@ public class GroupsV2Protos { let groupMembership = groupModel.groupMembership let localAddress = SignalServiceAddress(uuid: localUuid) assert(groupMembership.isAdministrator(localAddress)) - assert(!groupMembership.isPending(localAddress)) + assert(!groupMembership.isPendingMember(localAddress)) groupBuilder.addMembers(try buildMemberProto(profileKeyCredential: localProfileKeyCredential, role: .administrator, groupV2Params: groupV2Params)) @@ -120,7 +120,7 @@ public class GroupsV2Protos { } let address = SignalServiceAddress(uuid: uuid) let isAdministrator = groupMembership.isAdministrator(address) - let isPending = groupMembership.isPending(address) + let isPending = groupMembership.isPendingMember(address) let role: GroupsProtoMemberRole = isAdministrator ? .administrator : .`default` guard !isPending else { continue diff --git a/SignalServiceKit/src/Contacts/Threads/TSGroupThread+OWS.swift b/SignalServiceKit/src/Contacts/Threads/TSGroupThread+OWS.swift index 4e4d58b42c..76f305ebaa 100644 --- a/SignalServiceKit/src/Contacts/Threads/TSGroupThread+OWS.swift +++ b/SignalServiceKit/src/Contacts/Threads/TSGroupThread+OWS.swift @@ -24,6 +24,6 @@ public extension TSGroupThread { guard let localAddress = TSAccountManager.localAddress else { return false } - return groupModel.groupMembership.isPending(localAddress) + return groupModel.groupMembership.isPendingMember(localAddress) } } diff --git a/SignalServiceKit/src/Messages/Interactions/TSInfoMessage+GroupUpdateCopy.swift b/SignalServiceKit/src/Messages/Interactions/TSInfoMessage+GroupUpdateCopy.swift index 998f72630b..b185ee0c9d 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInfoMessage+GroupUpdateCopy.swift +++ b/SignalServiceKit/src/Messages/Interactions/TSInfoMessage+GroupUpdateCopy.swift @@ -401,8 +401,22 @@ extension GroupUpdateCopy { case .invited: switch newMembershipStatus { case .normalMember: - addUserInviteWasAccepted(for: address, - oldGroupMembership: oldGroupMembership) + var wasInviteAccepted = false + switch updater { + case .localUser: + wasInviteAccepted = localAddress == address + case .otherUser(_, let updaterAddress): + wasInviteAccepted = updaterAddress == address + case .unknown: + wasInviteAccepted = false + } + + if wasInviteAccepted { + addUserInviteWasAccepted(for: address, + oldGroupMembership: oldGroupMembership) + } else { + addUserWasAddedToTheGroup(for: address) + } case .invited: // Membership status didn't change. break @@ -1288,7 +1302,7 @@ extension GroupUpdateCopy { in groupMembership: GroupMembership) -> MembershipStatus { if groupMembership.isNonPendingMember(address) { return .normalMember - } else if groupMembership.isPending(address) { + } else if groupMembership.isPendingMember(address) { return .invited } else { return .none diff --git a/SignalServiceKit/src/groups/GroupManager.swift b/SignalServiceKit/src/groups/GroupManager.swift index 0fdaec7302..62404bc83c 100644 --- a/SignalServiceKit/src/groups/GroupManager.swift +++ b/SignalServiceKit/src/groups/GroupManager.swift @@ -391,7 +391,7 @@ public class GroupManager: NSObject { return newGroupMembership } let localAddress = SignalServiceAddress(uuid: localUuid) - let newMembers: Set + var newMembers: Set var builder = GroupMembership.Builder() if let oldGroupModel = oldGroupModel { // Updating existing group @@ -405,7 +405,13 @@ public class GroupManager: NSObject { // Carry over existing members as they stand. let existingMembers = oldGroupMembership.allUsers.intersection(newGroupMembership.allUsers) for address in existingMembers { - builder.copyMember(address, from: oldGroupMembership) + if oldGroupMembership.isPendingMember(address), + newGroupMembership.isNonPendingMember(address) { + // If we're adding a pending member, treat them as a new member. + newMembers.insert(address) + } else { + builder.copyMember(address, from: oldGroupMembership) + } } } else { // Creating new group @@ -456,7 +462,9 @@ public class GroupManager: NSObject { builder.addNonPendingMember(address, role: role) } } - return builder.build() + let groupMembership = builder.build() + assert(groupMembership.isValid) + return groupMembership } // success and failure are invoked on the main thread. @@ -1100,10 +1108,13 @@ public class GroupManager: NSObject { wasLocallyInitiated: true, transaction: transaction) } - }.then(on: .global()) { _ in - updateGroupV2(groupModel: groupModel, - description: "Accept invite") { groupChangeSet in - groupChangeSet.setShouldAcceptInvite() + }.then(on: .global()) { _ -> Promise in + guard let localUuid = tsAccountManager.localUuid else { + throw OWSAssertionError("Missing localUuid.") + } + return updateGroupV2(groupModel: groupModel, + description: "Accept invite") { groupChangeSet in + groupChangeSet.promotePendingMember(localUuid) } } } diff --git a/SignalServiceKit/src/groups/GroupMembership.swift b/SignalServiceKit/src/groups/GroupMembership.swift index 5fd24703d1..7c5b77f054 100644 --- a/SignalServiceKit/src/groups/GroupMembership.swift +++ b/SignalServiceKit/src/groups/GroupMembership.swift @@ -220,7 +220,9 @@ public class GroupMembership: MTLModel { addedByUuid: UUID) { for address in addresses { if memberStateMap[address] != nil { - owsFailDebug("Duplicate address.") + Logger.error("Duplicate address.") + continue +// owsFailDebug("Duplicate address.") } memberStateMap[address] = MemberState(role: role, isPending: true, addedByUuid: addedByUuid) } @@ -326,13 +328,6 @@ public class GroupMembership: MTLModel { return memberState.role } - public func isPending(_ address: SignalServiceAddress) -> Bool { - guard let memberState = memberStateMap[address] else { - return false - } - return memberState.isPending - } - public func isAdministrator(_ address: SignalServiceAddress) -> Bool { guard let memberState = memberStateMap[address] else { return false @@ -346,7 +341,10 @@ public class GroupMembership: MTLModel { @objc public func isNonPendingMember(_ address: SignalServiceAddress) -> Bool { - return nonPendingMembers.contains(address) + guard let memberState = memberStateMap[address] else { + return false + } + return !memberState.isPending } public func isNonPendingMember(_ uuid: UUID) -> Bool { @@ -355,11 +353,14 @@ public class GroupMembership: MTLModel { @objc public func isPendingMember(_ address: SignalServiceAddress) -> Bool { - return pendingMembers.contains(address) + guard let memberState = memberStateMap[address] else { + return false + } + return memberState.isPending } public func isPendingMember(_ uuid: UUID) -> Bool { - return isPendingMember(SignalServiceAddress(uuid: uuid)) + isPendingMember(SignalServiceAddress(uuid: uuid)) } // When we check "is X a member?" we might mean... @@ -382,6 +383,8 @@ public class GroupMembership: MTLModel { } public func addedByUuid(forPendingMember address: SignalServiceAddress) -> UUID? { + assert(isPendingMember(address)) + return memberStateMap[address]?.addedByUuid } @@ -389,7 +392,6 @@ public class GroupMembership: MTLModel { public static func normalize(_ addresses: [SignalServiceAddress]) -> [SignalServiceAddress] { return Array(Set(addresses)) .sorted(by: { (l, r) in l.compare(r) == .orderedAscending }) - } public func hasInvalidInvite(forUserId userId: Data) -> Bool { @@ -428,4 +430,8 @@ public class GroupMembership: MTLModel { result += "]" return result } + + public var isValid: Bool { + return true + } } diff --git a/SignalServiceKit/src/groups/GroupsV2.swift b/SignalServiceKit/src/groups/GroupsV2.swift index b176724875..80a5663dc1 100644 --- a/SignalServiceKit/src/groups/GroupsV2.swift +++ b/SignalServiceKit/src/groups/GroupsV2.swift @@ -116,7 +116,7 @@ public protocol GroupsV2ChangeSet: AnyObject { func setAccessForAttributes(_ value: GroupV2Access) - func setShouldAcceptInvite() + func promotePendingMember(_ uuid: UUID) func setShouldLeaveGroupDeclineInvite()