From 3873741dad44db9f152f961402cf5cd344693265 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 4 Feb 2022 16:08:58 -0800 Subject: [PATCH] Handle a group "change" that has only a snapshot (no actions) The server will include this if the backlog of group updates *should* have brought the group up to date, but the actual change action for the latest revision is missing. This should never happen under normal operation, but it's best to be defensive against data loss. (Or in this case, new server features tested in upcoming commits.) --- .../groups/GroupV2UpdatesImpl.swift | 57 +++++++++++-------- SignalMessaging/groups/GroupsV2Protos.swift | 30 ++++++---- SignalServiceKit/src/groups/GroupsV2.swift | 29 +++------- 3 files changed, 59 insertions(+), 57 deletions(-) diff --git a/SignalMessaging/groups/GroupV2UpdatesImpl.swift b/SignalMessaging/groups/GroupV2UpdatesImpl.swift index 89c105e8c7..fbff46687f 100644 --- a/SignalMessaging/groups/GroupV2UpdatesImpl.swift +++ b/SignalMessaging/groups/GroupV2UpdatesImpl.swift @@ -693,21 +693,16 @@ public class GroupV2UpdatesImpl: NSObject, GroupV2UpdatesSwift { } } - let diff = groupChange.diff - let changeActionsProto = diff.changeActionsProto - // Many change actions have author info, e.g. addedByUserID. But we can - // safely assume that all actions in the "change actions" have the same author. - guard let changeAuthorUuidData = changeActionsProto.sourceUuid else { - throw OWSAssertionError("Missing changeAuthorUuid.") - } - guard let oldGroupModel = groupThread.groupModel as? TSGroupModelV2 else { throw OWSAssertionError("Invalid group model.") } let isSingleRevisionUpdate = oldGroupModel.revision + 1 == changeRevision var groupUpdateSourceAddress: SignalServiceAddress? - if isSingleRevisionUpdate { + if isSingleRevisionUpdate, let changeAuthorUuidData = groupChange.changeActionsProto?.sourceUuid { + // Many change actions have author info, e.g. addedByUserID. But we can + // safely assume that all actions in the "change actions" have the same author. + // // The "group update" info message should only reflect // the "change author" if the change/diff reflects a // single revision. Eventually there will be gaps in @@ -717,6 +712,10 @@ public class GroupV2UpdatesImpl: NSObject, GroupV2UpdatesSwift { // the service. This is one. let changeAuthorUuid = try groupV2Params.uuid(forUserId: changeAuthorUuidData) groupUpdateSourceAddress = SignalServiceAddress(uuid: changeAuthorUuid) + } else { + owsAssertDebug(groupChange.changeActionsProto == nil + || groupChange.changeActionsProto?.sourceUuid != nil, + "explicit changes should always have authors") } // We should only replace placeholder models using @@ -750,15 +749,18 @@ public class GroupV2UpdatesImpl: NSObject, GroupV2UpdatesSwift { newGroupModel = try builder.build(transaction: transaction) newDisappearingMessageToken = snapshot.disappearingMessageToken newProfileKeys = snapshot.profileKeys - } else { + } else if let changeActionsProto = groupChange.changeActionsProto { let changedGroupModel = try GroupsV2IncomingChanges.applyChangesToGroupModel(groupThread: groupThread, changeActionsProto: changeActionsProto, - downloadedAvatars: diff.downloadedAvatars, + downloadedAvatars: groupChange.downloadedAvatars, groupModelOptions: groupModelOptions, transaction: transaction) newGroupModel = changedGroupModel.newGroupModel newDisappearingMessageToken = changedGroupModel.newDisappearingMessageToken newProfileKeys = changedGroupModel.profileKeys + } else { + owsFailDebug("neither a snapshot nor a change action (should have been validated earlier)") + continue } // We should only replace placeholder models using @@ -840,26 +842,30 @@ public class GroupV2UpdatesImpl: NSObject, GroupV2UpdatesSwift { guard let snapshot = firstGroupChange.snapshot else { throw OWSAssertionError("Missing snapshot.") } - // Many change actions have author info, e.g. addedByUserID. But we can - // safely assume that all actions in the "change actions" have the same author. - guard let changeAuthorUuidData = firstGroupChange.diff.changeActionsProto.sourceUuid else { - throw OWSAssertionError("Missing changeAuthorUuid.") + + var groupUpdateSourceAddress: SignalServiceAddress? + if let changeAuthorUuidData = firstGroupChange.changeActionsProto?.sourceUuid { + // Many change actions have author info, e.g. addedByUserID. But we can + // safely assume that all actions in the "change actions" have the same author. + // + // Some userIds/uuidCiphertexts can be validated by + // the service. This is one. + let changeAuthorUuid = try groupV2Params.uuid(forUserId: changeAuthorUuidData) + groupUpdateSourceAddress = SignalServiceAddress(uuid: changeAuthorUuid) + } else { + owsAssertDebug(firstGroupChange.changeActionsProto == nil + || firstGroupChange.changeActionsProto?.sourceUuid != nil, + "explicit changes should always have authors") } - // Some userIds/uuidCiphertexts can be validated by - // the service. This is one. - let changeAuthorUuid = try groupV2Params.uuid(forUserId: changeAuthorUuidData) var builder = try TSGroupModelBuilder.builderForSnapshot(groupV2Snapshot: snapshot, transaction: transaction) - if snapshot.revision == 0, - let localUuid = tsAccountManager.localUuid, - localUuid == changeAuthorUuid { + if snapshot.revision == 0, groupUpdateSourceAddress?.isLocalAddress == true { builder.wasJustCreatedByLocalUser = true } let newGroupModel = try builder.build(transaction: transaction) - let groupUpdateSourceAddress = SignalServiceAddress(uuid: changeAuthorUuid) let newDisappearingMessageToken = snapshot.disappearingMessageToken let didAddLocalUserToV2Group = self.didAddLocalUserToV2Group(groupChange: firstGroupChange, groupV2Params: groupV2Params) @@ -887,12 +893,15 @@ public class GroupV2UpdatesImpl: NSObject, GroupV2UpdatesSwift { guard let localUuid = tsAccountManager.localUuid else { return false } - if groupChange.diff.revision == 0 { + if groupChange.revision == 0 { // Revision 0 is a special case and won't have actions to // reflect the initial membership. return true } - let changeActionsProto = groupChange.diff.changeActionsProto + guard let changeActionsProto = groupChange.changeActionsProto else { + // We're missing a change here, so we can't assume this is how we got into the group. + return false + } for action in changeActionsProto.addMembers { do { diff --git a/SignalMessaging/groups/GroupsV2Protos.swift b/SignalMessaging/groups/GroupsV2Protos.swift index a8b32b9612..75e7ef839a 100644 --- a/SignalMessaging/groups/GroupsV2Protos.swift +++ b/SignalMessaging/groups/GroupsV2Protos.swift @@ -490,19 +490,27 @@ public class GroupsV2Protos { var result = [GroupV2Change]() for changeStateData in groupChangesProto.groupChanges { let changeStateProto = try GroupsProtoGroupChangesGroupChangeState(serializedData: changeStateData) + var snapshot: GroupV2Snapshot? if let snapshotProto = changeStateProto.groupState { snapshot = try parse(groupProto: snapshotProto, downloadedAvatars: downloadedAvatars, groupV2Params: groupV2Params) } - guard let changeProto = changeStateProto.groupChange else { - throw OWSAssertionError("Missing groupChange proto.") + + var changeActionsProto: GroupsProtoGroupChangeActions? + if let changeProto = changeStateProto.groupChange { + // We can ignoreSignature because these protos came from the service. + changeActionsProto = try parseAndVerifyChangeActionsProto(changeProto, ignoreSignature: true) } - // We can ignoreSignature because these protos came from the service. - let changeActionsProto: GroupsProtoGroupChangeActions = try parseAndVerifyChangeActionsProto(changeProto, ignoreSignature: true) - let diff = GroupV2Diff(changeActionsProto: changeActionsProto, downloadedAvatars: downloadedAvatars) - result.append(GroupV2Change(snapshot: snapshot, diff: diff)) + + guard snapshot != nil || changeActionsProto != nil else { + throw OWSAssertionError("both groupState and groupChange are absent") + } + + result.append(GroupV2Change(snapshot: snapshot, + changeActionsProto: changeActionsProto, + downloadedAvatars: downloadedAvatars)) } return result } @@ -542,13 +550,11 @@ public class GroupsV2Protos { } avatarUrlPaths += collectAvatarUrlPaths(groupProto: groupState) - guard let changeProto = changeStateProto.groupChange else { - owsFailDebug("Missing groupChange proto.") - throw GroupsV2Error.missingGroupChangeProtos + if let changeProto = changeStateProto.groupChange { + // We can ignoreSignature because these protos came from the service. + let changeActionsProto = try parseAndVerifyChangeActionsProto(changeProto, ignoreSignature: ignoreSignature) + avatarUrlPaths += self.collectAvatarUrlPaths(changeActionsProto: changeActionsProto) } - // We can ignoreSignature because these protos came from the service. - let changeActionsProto = try parseAndVerifyChangeActionsProto(changeProto, ignoreSignature: ignoreSignature) - avatarUrlPaths += self.collectAvatarUrlPaths(changeActionsProto: changeActionsProto) } return avatarUrlPaths } diff --git a/SignalServiceKit/src/groups/GroupsV2.swift b/SignalServiceKit/src/groups/GroupsV2.swift index b18d735ac3..486263753b 100644 --- a/SignalServiceKit/src/groups/GroupsV2.swift +++ b/SignalServiceKit/src/groups/GroupsV2.swift @@ -317,35 +317,22 @@ public protocol GroupV2Snapshot { // MARK: - -public struct GroupV2Diff { - public let changeActionsProto: GroupsProtoGroupChangeActions +public struct GroupV2Change { + public var snapshot: GroupV2Snapshot? + public var changeActionsProto: GroupsProtoGroupChangeActions? public let downloadedAvatars: GroupV2DownloadedAvatars - public init(changeActionsProto: GroupsProtoGroupChangeActions, + public init(snapshot: GroupV2Snapshot?, + changeActionsProto: GroupsProtoGroupChangeActions?, downloadedAvatars: GroupV2DownloadedAvatars) { + owsAssert(snapshot != nil || changeActionsProto != nil) + self.snapshot = snapshot self.changeActionsProto = changeActionsProto self.downloadedAvatars = downloadedAvatars } public var revision: UInt32 { - return changeActionsProto.revision - } -} - -// MARK: - - -public struct GroupV2Change { - public var snapshot: GroupV2Snapshot? - public var diff: GroupV2Diff - - public init(snapshot: GroupV2Snapshot?, - diff: GroupV2Diff) { - self.snapshot = snapshot - self.diff = diff - } - - public var revision: UInt32 { - return diff.revision + return changeActionsProto?.revision ?? snapshot!.revision } }