From 174ab60e66283f41f581633306da902aefdfa55c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Feb 2020 12:13:39 -0300 Subject: [PATCH] Fix handling of access control in protos; improve debug descriptions; clean up. --- .../UpdateGroupViewController.swift | 2 +- .../groups/GroupV2SnapshotImpl.swift | 2 +- .../groups/GroupV2UpdatesImpl.swift | 2 ++ .../groups/GroupsV2ChangeSetImpl.swift | 6 ++--- SignalMessaging/groups/GroupsV2Changes.swift | 6 ++--- SignalMessaging/groups/GroupsV2Protos.swift | 13 ++++++---- SignalServiceKit/src/groups/GroupAccess.swift | 12 ++++----- .../src/groups/GroupManager.swift | 25 +++++++++++++------ 8 files changed, 42 insertions(+), 26 deletions(-) diff --git a/Signal/src/ViewControllers/ThreadSettings/UpdateGroupViewController.swift b/Signal/src/ViewControllers/ThreadSettings/UpdateGroupViewController.swift index 5baf8b81b7..2f3ff2034e 100644 --- a/Signal/src/ViewControllers/ThreadSettings/UpdateGroupViewController.swift +++ b/Signal/src/ViewControllers/ThreadSettings/UpdateGroupViewController.swift @@ -41,7 +41,7 @@ public extension UpdateGroupViewController { databaseStorage.read { transaction in for address in v1Members { guard GroupManager.doesUserSupportGroupsV2(address: address, transaction: transaction) else { - owsFailDebug("Invalid address: \(address)") + Logger.warn("Invalid address: \(address)") continue } if !oldGroupMembership.allUsers.contains(address) { diff --git a/SignalMessaging/groups/GroupV2SnapshotImpl.swift b/SignalMessaging/groups/GroupV2SnapshotImpl.swift index 1cf5597104..632176a940 100644 --- a/SignalMessaging/groups/GroupV2SnapshotImpl.swift +++ b/SignalMessaging/groups/GroupV2SnapshotImpl.swift @@ -99,7 +99,7 @@ public struct GroupV2SnapshotImpl: GroupV2Snapshot { } public var groupAccess: GroupAccess { - return GroupAccess(member: GroupAccess.groupV2Access(forProtoAccess: accessControlForMembers), + return GroupAccess(members: GroupAccess.groupV2Access(forProtoAccess: accessControlForMembers), attributes: GroupAccess.groupV2Access(forProtoAccess: accessControlForAttributes)) } } diff --git a/SignalMessaging/groups/GroupV2UpdatesImpl.swift b/SignalMessaging/groups/GroupV2UpdatesImpl.swift index 76b415fa3b..1e9d1c164a 100644 --- a/SignalMessaging/groups/GroupV2UpdatesImpl.swift +++ b/SignalMessaging/groups/GroupV2UpdatesImpl.swift @@ -441,6 +441,8 @@ public class GroupV2UpdatesImpl: NSObject, GroupV2UpdatesSwift { // This should match the state in the database. If it doesn't, // this reflects a bug, perhaps\ a deviation in how the service // and client apply the "group changes" to the local model. + Logger.verbose("oldGroupModel: \(oldGroupModel.debugDescription)") + Logger.verbose("newGroupModel: \(newGroupModel.debugDescription)") owsFailDebug("Group models don't match.") } } diff --git a/SignalMessaging/groups/GroupsV2ChangeSetImpl.swift b/SignalMessaging/groups/GroupsV2ChangeSetImpl.swift index e88d394f7c..35bd2cdb0c 100644 --- a/SignalMessaging/groups/GroupsV2ChangeSetImpl.swift +++ b/SignalMessaging/groups/GroupsV2ChangeSetImpl.swift @@ -165,8 +165,8 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { let oldAccess = oldGroupModel.groupAccess let newAccess = newGroupModel.groupAccess - if oldAccess.member != newAccess.member { - self.accessForMembers = newAccess.member + if oldAccess.members != newAccess.members { + self.accessForMembers = newAccess.members } if oldAccess.attributes != newAccess.attributes { self.accessForAttributes = newAccess.attributes @@ -408,7 +408,7 @@ public class GroupsV2ChangeSetImpl: NSObject, GroupsV2ChangeSet { let currentAccess = currentGroupModel.groupAccess if let access = self.accessForMembers { - if currentAccess.member != access { + if currentAccess.members != access { let actionBuilder = GroupsProtoGroupChangeActionsModifyMembersAccessControlAction.builder() actionBuilder.setMembersAccess(GroupAccess.protoAccess(forGroupV2Access: access)) actionsBuilder.setModifyMemberAccess(try actionBuilder.build()) diff --git a/SignalMessaging/groups/GroupsV2Changes.swift b/SignalMessaging/groups/GroupsV2Changes.swift index 09433a5a9e..a1d4fe5b53 100644 --- a/SignalMessaging/groups/GroupsV2Changes.swift +++ b/SignalMessaging/groups/GroupsV2Changes.swift @@ -80,7 +80,7 @@ public class GroupsV2Changes { var groupMembershipBuilder = oldGroupMembership.asBuilder let oldGroupAccess = oldGroupModel.groupAccess - var newMemberAccess = oldGroupAccess.member + var newMembersAccess = oldGroupAccess.members var newAttributesAccess = oldGroupAccess.attributes // This client can learn of profile keys from parsing group state protos. @@ -279,11 +279,11 @@ public class GroupsV2Changes { guard let protoAccess = action.membersAccess else { throw OWSAssertionError("Missing access.") } - newMemberAccess = GroupAccess.groupV2Access(forProtoAccess: protoAccess) + newMembersAccess = GroupAccess.groupV2Access(forProtoAccess: protoAccess) } let newGroupMembership = groupMembershipBuilder.build() - let newGroupAccess = GroupAccess(member: newMemberAccess, attributes: newAttributesAccess) + let newGroupAccess = GroupAccess(members: newMembersAccess, attributes: newAttributesAccess) // GroupsV2 TODO: Avatar. let avatarData: Data? = oldGroupModel.groupAvatarData diff --git a/SignalMessaging/groups/GroupsV2Protos.swift b/SignalMessaging/groups/GroupsV2Protos.swift index 79c982509f..49ff322948 100644 --- a/SignalMessaging/groups/GroupsV2Protos.swift +++ b/SignalMessaging/groups/GroupsV2Protos.swift @@ -86,11 +86,7 @@ public class GroupsV2Protos { groupBuilder.setTitle(try groupV2Params.encryptString(groupModel.groupName ?? "")) // GroupsV2 TODO: Avatar - let accessControl = GroupsProtoAccessControl.builder() - // GroupsV2 TODO: Pull these values from the group model. - accessControl.setAttributes(.member) - accessControl.setMembers(.member) - groupBuilder.setAccessControl(try accessControl.build()) + groupBuilder.setAccessControl(try buildAccessProto(groupAccess: groupModel.groupAccess)) // * You will be member 0 and the only admin. // * Other members will be non-admin members. @@ -136,6 +132,13 @@ public class GroupsV2Protos { return try groupBuilder.build() } + public class func buildAccessProto(groupAccess: GroupAccess) throws -> GroupsProtoAccessControl { + let builder = GroupsProtoAccessControl.builder() + builder.setAttributes(GroupAccess.protoAccess(forGroupV2Access: groupAccess.attributes)) + builder.setMembers(GroupAccess.protoAccess(forGroupV2Access: groupAccess.members)) + return try builder.build() + } + public class func buildGroupContextV2Proto(groupModel: TSGroupModel, changeActionsProtoData: Data?) throws -> SSKProtoGroupContextV2 { diff --git a/SignalServiceKit/src/groups/GroupAccess.swift b/SignalServiceKit/src/groups/GroupAccess.swift index ee18e579eb..d7121e2edd 100644 --- a/SignalServiceKit/src/groups/GroupAccess.swift +++ b/SignalServiceKit/src/groups/GroupAccess.swift @@ -32,13 +32,13 @@ public enum GroupV2Access: UInt, Codable { @objc public class GroupAccess: MTLModel { @objc - public var member: GroupV2Access = .unknown + public var members: GroupV2Access = .unknown @objc public var attributes: GroupV2Access = .unknown - public init(member: GroupV2Access, + public init(members: GroupV2Access, attributes: GroupV2Access) { - self.member = member + self.members = members self.attributes = attributes super.init() @@ -61,7 +61,7 @@ public class GroupAccess: MTLModel { @objc public static var allAccess: GroupAccess { - return GroupAccess(member: .any, attributes: .any) + return GroupAccess(members: .any, attributes: .any) } @objc @@ -71,7 +71,7 @@ public class GroupAccess: MTLModel { @objc public static var defaultForV2: GroupAccess { - return GroupAccess(member: .member, attributes: .member) + return GroupAccess(members: .member, attributes: .member) } public class func groupV2Access(forProtoAccess value: GroupsProtoAccessControlAccessRequired) -> GroupV2Access { @@ -101,6 +101,6 @@ public class GroupAccess: MTLModel { } public override var debugDescription: String { - return "[members: \(member.description), attributes: \(attributes.description), ]" + return "[members: \(members.description), attributes: \(attributes.description), ]" } } diff --git a/SignalServiceKit/src/groups/GroupManager.swift b/SignalServiceKit/src/groups/GroupManager.swift index 591cf46598..88b5dca5a8 100644 --- a/SignalServiceKit/src/groups/GroupManager.swift +++ b/SignalServiceKit/src/groups/GroupManager.swift @@ -342,7 +342,7 @@ public class GroupManager: NSObject { }.map(on: .global()) { (_) -> GroupMembership in return groupMembership } - }.then(on: .global()) { (proposedGroupMembership: GroupMembership) throws -> Promise in + }.map(on: .global()) { (proposedGroupMembership: GroupMembership) throws -> TSGroupModel in // GroupsV2 TODO: Let users specify access levels in the "new group" view. let groupAccess = GroupAccess.allAccess let groupModel = try self.databaseStorage.read { (transaction) throws -> TSGroupModel in @@ -366,15 +366,26 @@ public class GroupManager: NSObject { newGroupSeed: newGroupSeed, transaction: transaction) } - - guard groupModel.groupsVersion == .V2 else { + return groupModel + }.then(on: .global()) { (proposedGroupModel: TSGroupModel) -> Promise in + guard proposedGroupModel.groupsVersion == .V2 else { // v1 groups don't need to be created on the service. - return Promise.value(groupModel) + return Promise.value(proposedGroupModel) } return firstly { - self.groupsV2Swift.createNewGroupOnService(groupModel: groupModel) - }.map(on: .global()) { _ in - return groupModel + self.groupsV2Swift.createNewGroupOnService(groupModel: proposedGroupModel) + }.then(on: .global()) { _ in + self.groupsV2Swift.fetchCurrentGroupV2Snapshot(groupModel: proposedGroupModel) + }.map(on: .global()) { (groupV2Snapshot: GroupV2Snapshot) -> TSGroupModel in + let createdGroupModel = try self.databaseStorage.read { transaction in + return try GroupManager.buildGroupModel(groupV2Snapshot: groupV2Snapshot, transaction: transaction) + } + if proposedGroupModel != createdGroupModel { + Logger.verbose("proposedGroupModel: \(proposedGroupModel.debugDescription)") + Logger.verbose("createdGroupModel: \(createdGroupModel.debugDescription)") + owsFailDebug("Proposed group model does not match created group model.") + } + return createdGroupModel } }.then(on: .global()) { (groupModel: TSGroupModel) -> Promise in let thread = databaseStorage.write { (transaction: SDSAnyWriteTransaction) -> TSGroupThread in