From 4a04265e1544fcf48c19d238a32ae023b52fc637 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 28 Apr 2020 10:48:57 -0300 Subject: [PATCH 1/2] Revise group v2 proto parsing robustness. --- SignalMessaging/groups/GroupsV2Changes.swift | 56 +++++++++----------- SignalMessaging/groups/GroupsV2Protos.swift | 31 +++++------ 2 files changed, 41 insertions(+), 46 deletions(-) diff --git a/SignalMessaging/groups/GroupsV2Changes.swift b/SignalMessaging/groups/GroupsV2Changes.swift index d9e553f44e..1da113ce46 100644 --- a/SignalMessaging/groups/GroupsV2Changes.swift +++ b/SignalMessaging/groups/GroupsV2Changes.swift @@ -107,18 +107,14 @@ public class GroupsV2Changes { groupMembershipBuilder.remove(address) groupMembershipBuilder.addNonPendingMember(address, role: role) - do { - guard let profileKeyCiphertextData = member.profileKey else { - throw OWSAssertionError("Missing profileKeyCiphertext.") - } - let profileKeyCiphertext = try ProfileKeyCiphertext(contents: [UInt8](profileKeyCiphertextData)) - let profileKey = try groupV2Params.profileKey(forProfileKeyCiphertext: profileKeyCiphertext, - uuid: uuid) - - profileKeys[uuid] = profileKey - } catch { - owsFailDebug("Error parsing profile key: \(error)") + guard let profileKeyCiphertextData = member.profileKey else { + throw OWSAssertionError("Missing profileKeyCiphertext.") } + let profileKeyCiphertext = try ProfileKeyCiphertext(contents: [UInt8](profileKeyCiphertextData)) + let profileKey = try groupV2Params.profileKey(forProfileKeyCiphertext: profileKeyCiphertext, + uuid: uuid) + + profileKeys[uuid] = profileKey } for action in changeActionsProto.deleteMembers { @@ -168,14 +164,10 @@ public class GroupsV2Changes { throw OWSAssertionError("Invalid membership.") } - do { - let profileKeyCiphertext = try presentation.getProfileKeyCiphertext() - let profileKey = try groupV2Params.profileKey(forProfileKeyCiphertext: profileKeyCiphertext, - uuid: uuid) - profileKeys[uuid] = profileKey - } catch { - owsFailDebug("Error parsing profile key: \(error)") - } + let profileKeyCiphertext = try presentation.getProfileKeyCiphertext() + let profileKey = try groupV2Params.profileKey(forProfileKeyCiphertext: profileKeyCiphertext, + uuid: uuid) + profileKeys[uuid] = profileKey } for action in changeActionsProto.addPendingMembers { @@ -194,13 +186,19 @@ public class GroupsV2Changes { guard let role = TSGroupMemberRole.role(for: protoRole) else { throw OWSAssertionError("Invalid role: \(protoRole.rawValue)") } - let uuid = try groupV2Params.uuid(forUserId: userId) - let address = SignalServiceAddress(uuid: uuid) guard let addedByUserID = pendingMember.addedByUserID else { throw OWSAssertionError("Group pending member missing addedByUserID.") } - let addedByUuid = try groupV2Params.uuid(forUserId: addedByUserID) - + let uuid: UUID + let addedByUuid: UUID + do { + uuid = try groupV2Params.uuid(forUserId: userId) + addedByUuid = try groupV2Params.uuid(forUserId: addedByUserID) + } catch { + owsFailDebug("Error parsing uuid: \(error)") + continue + } + let address = SignalServiceAddress(uuid: uuid) guard !oldGroupMembership.allUsers.contains(address) else { throw OWSAssertionError("Invalid membership.") } @@ -242,14 +240,10 @@ public class GroupsV2Changes { groupMembershipBuilder.remove(address) groupMembershipBuilder.addNonPendingMember(address, role: role) - do { - let profileKeyCiphertext = try presentation.getProfileKeyCiphertext() - let profileKey = try groupV2Params.profileKey(forProfileKeyCiphertext: profileKeyCiphertext, - uuid: uuid) - profileKeys[uuid] = profileKey - } catch { - owsFailDebug("Error parsing profile key: \(error)") - } + let profileKeyCiphertext = try presentation.getProfileKeyCiphertext() + let profileKey = try groupV2Params.profileKey(forProfileKeyCiphertext: profileKeyCiphertext, + uuid: uuid) + profileKeys[uuid] = profileKey } if let action = changeActionsProto.modifyTitle { diff --git a/SignalMessaging/groups/GroupsV2Protos.swift b/SignalMessaging/groups/GroupsV2Protos.swift index 762688930b..46d1fcaae9 100644 --- a/SignalMessaging/groups/GroupsV2Protos.swift +++ b/SignalMessaging/groups/GroupsV2Protos.swift @@ -242,18 +242,13 @@ public class GroupsV2Protos { role: role) members.append(member) - do { - let uuid = try groupV2Params.uuid(forUserId: userID) - guard let profileKeyCiphertextData = memberProto.profileKey else { - throw OWSAssertionError("Group member missing profileKeyCiphertextData.") - } - let profileKeyCiphertext = try ProfileKeyCiphertext(contents: [UInt8](profileKeyCiphertextData)) - let profileKey = try groupV2Params.profileKey(forProfileKeyCiphertext: profileKeyCiphertext, - uuid: uuid) - profileKeys[uuid] = profileKey - } catch { - owsFailDebug("Error parsing profile key: \(error)") + guard let profileKeyCiphertextData = memberProto.profileKey else { + throw OWSAssertionError("Group member missing profileKeyCiphertextData.") } + let profileKeyCiphertext = try ProfileKeyCiphertext(contents: [UInt8](profileKeyCiphertextData)) + let profileKey = try groupV2Params.profileKey(forProfileKeyCiphertext: profileKeyCiphertext, + uuid: uuid) + profileKeys[uuid] = profileKey } var pendingMembers = [GroupV2SnapshotImpl.PendingMember]() @@ -271,13 +266,19 @@ public class GroupsV2Protos { throw OWSAssertionError("Group pending member missing addedByUserID.") } let timestamp = pendingMemberProto.timestamp - - let uuid = try groupV2Params.uuid(forUserId: userId) - let addedByUuid = try groupV2Params.uuid(forUserId: addedByUserId) - guard memberProto.hasRole, let role = memberProto.role else { throw OWSAssertionError("Group member missing role.") } + + let uuid: UUID + let addedByUuid: UUID + do { + uuid = try groupV2Params.uuid(forUserId: userId) + addedByUuid = try groupV2Params.uuid(forUserId: addedByUserId) + } catch { + owsFailDebug("Error parsing uuid: \(error)") + continue + } let pendingMember = GroupV2SnapshotImpl.PendingMember(userID: userId, uuid: uuid, timestamp: timestamp, From d60af64d290026003b1b6949aefbfb01e9c07654 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 28 Apr 2020 10:49:58 -0300 Subject: [PATCH 2/2] Suppress "group updated" info messages without user-facing changes. --- .../UpdateGroupViewController.m | 2 +- .../groups/GroupV2UpdatesImpl.swift | 8 ++--- .../src/groups/GroupManager.swift | 31 ++++++++++++------- SignalServiceKit/src/groups/TSGroupModel.h | 2 +- SignalServiceKit/src/groups/TSGroupModel.m | 6 ++-- .../src/groups/TSGroupModel.swift | 7 +++-- 6 files changed, 32 insertions(+), 24 deletions(-) diff --git a/Signal/src/ViewControllers/ThreadSettings/UpdateGroupViewController.m b/Signal/src/ViewControllers/ThreadSettings/UpdateGroupViewController.m index 8336b91b16..c88cbe3e1b 100644 --- a/Signal/src/ViewControllers/ThreadSettings/UpdateGroupViewController.m +++ b/Signal/src/ViewControllers/ThreadSettings/UpdateGroupViewController.m @@ -357,7 +357,7 @@ NS_ASSUME_NONNULL_BEGIN newTitle:newTitle newAvatarData:newAvatarData v1Members:memberSet]; - if ([self.oldGroupModel isEqualToGroupModel:newGroupModel]) { + if ([self.oldGroupModel isEqualToGroupModel:newGroupModel ignoreRevision:YES]) { return nil; } return newGroupModel; diff --git a/SignalMessaging/groups/GroupV2UpdatesImpl.swift b/SignalMessaging/groups/GroupV2UpdatesImpl.swift index e8d748fbfa..3065ef09ee 100644 --- a/SignalMessaging/groups/GroupV2UpdatesImpl.swift +++ b/SignalMessaging/groups/GroupV2UpdatesImpl.swift @@ -362,10 +362,10 @@ public class GroupV2UpdatesImpl: NSObject, GroupV2UpdatesSwift { throw OWSAssertionError("Invalid group model.") } guard updatedGroupModel.revision > changedGroupModel.oldGroupModel.revision else { - throw OWSAssertionError("Invalid groupV2Revision: \(changedGroupModel.newGroupModel.revision).") + throw OWSAssertionError("Invalid groupV2Revision: \(updatedGroupModel.revision) <= \(changedGroupModel.oldGroupModel.revision).") } guard updatedGroupModel.revision >= changedGroupModel.newGroupModel.revision else { - throw OWSAssertionError("Invalid groupV2Revision: \(changedGroupModel.newGroupModel.revision).") + throw OWSAssertionError("Invalid groupV2Revision: \(updatedGroupModel.revision) < \(changedGroupModel.newGroupModel.revision).") } return updatedGroupThread } @@ -471,11 +471,11 @@ public class GroupV2UpdatesImpl: NSObject, GroupV2UpdatesSwift { let newGroupModel = try builder.build(transaction: transaction) if changeRevision == oldGroupModel.revision { - if !oldGroupModel.isEqual(to: newGroupModel) { + if !oldGroupModel.isEqual(to: newGroupModel, ignoreRevision: false) { // Sometimes we re-apply the snapshot corresponding to the // current revision when refreshing the group from the service. // This should match the state in the database. If it doesn't, - // this reflects a bug, perhaps\ a deviation in how the service + // 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)") diff --git a/SignalServiceKit/src/groups/GroupManager.swift b/SignalServiceKit/src/groups/GroupManager.swift index 8afbb327a0..32900c94ce 100644 --- a/SignalServiceKit/src/groups/GroupManager.swift +++ b/SignalServiceKit/src/groups/GroupManager.swift @@ -10,7 +10,8 @@ public class UpsertGroupResult: NSObject { @objc public enum Action: UInt { case inserted - case updated + case updatedWithUserFacingChanges + case updatedWithoutUserFacingChanges case unchanged } @@ -821,7 +822,7 @@ public class GroupManager: NSObject { groupModelBuilder.groupMembership = groupMembership let newGroupModel = try groupModelBuilder.build(transaction: transaction) - if oldGroupModel.isEqual(to: newGroupModel) { + if oldGroupModel.isEqual(to: newGroupModel, ignoreRevision: false) { // Skip redundant update. throw GroupsV2Error.redundantChange } @@ -891,7 +892,7 @@ public class GroupManager: NSObject { builder.groupV2Revision = newRevision let newGroupModel = try builder.build(transaction: transaction) - if oldGroupModel.isEqual(to: newGroupModel) { + if oldGroupModel.isEqual(to: newGroupModel, ignoreRevision: false) { // Skip redundant update. throw GroupsV2Error.redundantChange } @@ -1544,11 +1545,6 @@ public class GroupManager: NSObject { // Step 3: Update group in database, if necessary. let oldGroupModel = groupThread.groupModel let updateThreadResult: UpsertGroupResult = { - guard !oldGroupModel.isEqual(to: newGroupModel) else { - // Skip redundant update. - return UpsertGroupResult(action: .unchanged, groupThread: groupThread) - } - if let newGroupModelV2 = newGroupModel as? TSGroupModelV2, let oldGroupModelV2 = oldGroupModel as? TSGroupModelV2 { guard newGroupModelV2.revision >= oldGroupModelV2.revision else { @@ -1566,14 +1562,25 @@ public class GroupManager: NSObject { } } + guard !oldGroupModel.isEqual(to: newGroupModel, ignoreRevision: false) else { + // Skip redundant update. + return UpsertGroupResult(action: .unchanged, groupThread: groupThread) + } + + let hasUserFacingChange = !oldGroupModel.isEqual(to: newGroupModel, ignoreRevision: true) + groupThread.update(with: newGroupModel, transaction: transaction) - return UpsertGroupResult(action: .updated, groupThread: groupThread) + let action: UpsertGroupResult.Action = (hasUserFacingChange + ? .updatedWithUserFacingChanges + : .updatedWithoutUserFacingChanges) + return UpsertGroupResult(action: action, groupThread: groupThread) }() if updateDMResult.action == .unchanged && - updateThreadResult.action == .unchanged { - // Neither DM config nor thread model changed. + (updateThreadResult.action == .unchanged || + updateThreadResult.action == .updatedWithoutUserFacingChanges) { + // Neither DM config nor thread model had user-facing changes. return updateThreadResult } @@ -1590,7 +1597,7 @@ public class GroupManager: NSObject { break } - return UpsertGroupResult(action: .updated, groupThread: groupThread) + return UpsertGroupResult(action: .updatedWithUserFacingChanges, groupThread: groupThread) } // MARK: - Storage Service diff --git a/SignalServiceKit/src/groups/TSGroupModel.h b/SignalServiceKit/src/groups/TSGroupModel.h index 675efa7c8f..076d1499e3 100644 --- a/SignalServiceKit/src/groups/TSGroupModel.h +++ b/SignalServiceKit/src/groups/TSGroupModel.h @@ -53,7 +53,7 @@ typedef NS_CLOSED_ENUM( members:(NSArray *)members NS_DESIGNATED_INITIALIZER; - (BOOL)isEqual:(id)other; -- (BOOL)isEqualToGroupModel:(TSGroupModel *)model; +- (BOOL)isEqualToGroupModel:(TSGroupModel *)model ignoreRevision:(BOOL)ignoreRevision; #endif @property (nonatomic, readonly) NSString *groupNameOrDefault; diff --git a/SignalServiceKit/src/groups/TSGroupModel.m b/SignalServiceKit/src/groups/TSGroupModel.m index 00bfaeadc3..949edf543a 100644 --- a/SignalServiceKit/src/groups/TSGroupModel.m +++ b/SignalServiceKit/src/groups/TSGroupModel.m @@ -153,10 +153,11 @@ NSUInteger const TSGroupModelSchemaVersion = 1; if (!other || ![other isKindOfClass:[self class]]) { return NO; } - return [self isEqualToGroupModel:other]; + return [self isEqualToGroupModel:other ignoreRevision:NO]; } -- (BOOL)isEqualToGroupModel:(TSGroupModel *)other { +- (BOOL)isEqualToGroupModel:(TSGroupModel *)other ignoreRevision:(BOOL)ignoreRevision +{ if (self == other) { return YES; } @@ -174,7 +175,6 @@ NSUInteger const TSGroupModelSchemaVersion = 1; if (![myGroupMembersSet isEqualToSet:otherGroupMembersSet]) { return NO; } - if (self.groupsVersion != other.groupsVersion) { return NO; } diff --git a/SignalServiceKit/src/groups/TSGroupModel.swift b/SignalServiceKit/src/groups/TSGroupModel.swift index 2544cfd039..3b896c112b 100644 --- a/SignalServiceKit/src/groups/TSGroupModel.swift +++ b/SignalServiceKit/src/groups/TSGroupModel.swift @@ -77,8 +77,9 @@ public class TSGroupModelV2: TSGroupModel { return Array(groupMembership.nonPendingMembers) } - public override func isEqual(to model: TSGroupModel) -> Bool { - guard super.isEqual(to: model) else { + public override func isEqual(to model: TSGroupModel, + ignoreRevision: Bool) -> Bool { + guard super.isEqual(to: model, ignoreRevision: ignoreRevision) else { return false } guard let other = model as? TSGroupModelV2 else { @@ -93,7 +94,7 @@ public class TSGroupModelV2: TSGroupModel { guard other.secretParamsData == secretParamsData else { return false } - guard other.revision == revision else { + guard ignoreRevision || other.revision == revision else { return false } guard other.avatarUrlPath == avatarUrlPath else {