From 380ed0f82b421676ab083432ec8e02181429cc48 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 3 Oct 2017 13:41:48 -0400 Subject: [PATCH 1/3] Create & access groups more carefully. // FREEBIE --- .../ConversationViewController.m | 37 ++- .../src/Contacts/Threads/TSGroupThread.h | 8 +- .../src/Contacts/Threads/TSGroupThread.m | 29 +- .../OWSIncomingSentMessageTranscript.m | 2 +- .../src/Messages/OWSMessageManager.m | 263 +++++++++++------- 5 files changed, 214 insertions(+), 125 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index 7323ba967..d72139dc0 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -1495,15 +1495,26 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { }]]; } - [subtitleText - appendAttributedString:[[NSAttributedString alloc] - initWithString:NSLocalizedString(@"MESSAGES_VIEW_TITLE_SUBTITLE", - @"The subtitle for the messages view title indicates that the " - @"title can be tapped to access settings for this conversation.") - attributes:@{ - NSFontAttributeName : [UIFont ows_regularFontWithSize:9.f], - NSForegroundColorAttributeName : [UIColor colorWithWhite:0.9f alpha:1.f], - }]]; + if (self.userLeftGroup) { + [subtitleText + appendAttributedString:[[NSAttributedString alloc] + initWithString:NSLocalizedString(@"GROUP_YOU_LEFT", @"") + attributes:@{ + NSFontAttributeName : [UIFont ows_regularFontWithSize:9.f], + NSForegroundColorAttributeName : [UIColor colorWithWhite:0.9f alpha:1.f], + }]]; + } else { + [subtitleText appendAttributedString: + [[NSAttributedString alloc] + initWithString:NSLocalizedString(@"MESSAGES_VIEW_TITLE_SUBTITLE", + @"The subtitle for the messages view title indicates that the " + @"title can be tapped to access settings for this conversation.") + attributes:@{ + NSFontAttributeName : [UIFont ows_regularFontWithSize:9.f], + NSForegroundColorAttributeName : [UIColor colorWithWhite:0.9f alpha:1.f], + }]]; + } + self.navigationBarSubtitleLabel.attributedText = subtitleText; [self.navigationBarSubtitleLabel sizeToFit]; } @@ -3649,7 +3660,13 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { TSGroupThread *gThread = (TSGroupThread *)self.thread; if (gThread.groupModel) { - self.thread = [TSGroupThread threadWithGroupModel:gThread.groupModel transaction:transaction]; + TSGroupThread *_Nullable updatedThread = + [TSGroupThread threadWithGroupId:gThread.groupModel.groupId transaction:transaction]; + if (updatedThread) { + self.thread = updatedThread; + } else { + OWSFail(@"%@ Could not reload thread.", self.tag); + } } }]; [self setNavigationTitle]; diff --git a/SignalServiceKit/src/Contacts/Threads/TSGroupThread.h b/SignalServiceKit/src/Contacts/Threads/TSGroupThread.h index fff927604..c5136e126 100644 --- a/SignalServiceKit/src/Contacts/Threads/TSGroupThread.h +++ b/SignalServiceKit/src/Contacts/Threads/TSGroupThread.h @@ -18,11 +18,11 @@ NS_ASSUME_NONNULL_BEGIN + (instancetype)getOrCreateThreadWithGroupModel:(TSGroupModel *)groupModel transaction:(YapDatabaseReadWriteTransaction *)transaction; -+ (instancetype)getOrCreateThreadWithGroupIdData:(NSData *)groupId; -+ (instancetype)getOrCreateThreadWithGroupIdData:(NSData *)groupId - transaction:(YapDatabaseReadWriteTransaction *)transaction; ++ (instancetype)getOrCreateThreadWithGroupId:(NSData *)groupId; ++ (instancetype)getOrCreateThreadWithGroupId:(NSData *)groupId + transaction:(YapDatabaseReadWriteTransaction *)transaction; -+ (instancetype)threadWithGroupModel:(TSGroupModel *)groupModel transaction:(YapDatabaseReadTransaction *)transaction; ++ (nullable instancetype)threadWithGroupId:(NSData *)groupId transaction:(YapDatabaseReadTransaction *)transaction; + (NSString *)threadIdFromGroupId:(NSData *)groupId; diff --git a/SignalServiceKit/src/Contacts/Threads/TSGroupThread.m b/SignalServiceKit/src/Contacts/Threads/TSGroupThread.m index 1549ac1a9..74c48e2a3 100644 --- a/SignalServiceKit/src/Contacts/Threads/TSGroupThread.m +++ b/SignalServiceKit/src/Contacts/Threads/TSGroupThread.m @@ -36,11 +36,19 @@ NS_ASSUME_NONNULL_BEGIN return self; } -- (instancetype)initWithGroupIdData:(NSData *)groupId +- (instancetype)initWithGroupId:(NSData *)groupId { OWSAssert(groupId.length > 0); - TSGroupModel *groupModel = [[TSGroupModel alloc] initWithTitle:nil memberIds:nil image:nil groupId:groupId]; + NSString *localNumber = [TSAccountManager localNumber]; + OWSAssert(localNumber.length > 0); + + TSGroupModel *groupModel = [[TSGroupModel alloc] initWithTitle:nil + memberIds:[@[ + localNumber, + ] mutableCopy] + image:nil + groupId:groupId]; self = [self initWithGroupModel:groupModel]; if (!self) { @@ -50,35 +58,34 @@ NS_ASSUME_NONNULL_BEGIN return self; } -+ (instancetype)threadWithGroupModel:(TSGroupModel *)groupModel transaction:(YapDatabaseReadTransaction *)transaction ++ (nullable instancetype)threadWithGroupId:(NSData *)groupId transaction:(YapDatabaseReadTransaction *)transaction { - OWSAssert(groupModel); - OWSAssert(groupModel.groupId.length > 0); + OWSAssert(groupId.length > 0); - return [self fetchObjectWithUniqueID:[self threadIdFromGroupId:groupModel.groupId] transaction:transaction]; + return [self fetchObjectWithUniqueID:[self threadIdFromGroupId:groupId] transaction:transaction]; } -+ (instancetype)getOrCreateThreadWithGroupIdData:(NSData *)groupId - transaction:(YapDatabaseReadWriteTransaction *)transaction ++ (instancetype)getOrCreateThreadWithGroupId:(NSData *)groupId + transaction:(YapDatabaseReadWriteTransaction *)transaction { OWSAssert(groupId.length > 0); OWSAssert(transaction); TSGroupThread *thread = [self fetchObjectWithUniqueID:[self threadIdFromGroupId:groupId] transaction:transaction]; if (!thread) { - thread = [[self alloc] initWithGroupIdData:groupId]; + thread = [[self alloc] initWithGroupId:groupId]; [thread saveWithTransaction:transaction]; } return thread; } -+ (instancetype)getOrCreateThreadWithGroupIdData:(NSData *)groupId ++ (instancetype)getOrCreateThreadWithGroupId:(NSData *)groupId { OWSAssert(groupId.length > 0); __block TSGroupThread *thread; [[self dbReadWriteConnection] readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - thread = [self getOrCreateThreadWithGroupIdData:groupId transaction:transaction]; + thread = [self getOrCreateThreadWithGroupId:groupId transaction:transaction]; }]; return thread; } diff --git a/SignalServiceKit/src/Messages/DeviceSyncing/OWSIncomingSentMessageTranscript.m b/SignalServiceKit/src/Messages/DeviceSyncing/OWSIncomingSentMessageTranscript.m index 5db266a9c..eb4267827 100644 --- a/SignalServiceKit/src/Messages/DeviceSyncing/OWSIncomingSentMessageTranscript.m +++ b/SignalServiceKit/src/Messages/DeviceSyncing/OWSIncomingSentMessageTranscript.m @@ -50,7 +50,7 @@ NS_ASSUME_NONNULL_BEGIN - (TSThread *)threadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction { if (self.dataMessage.hasGroup) { - return [TSGroupThread getOrCreateThreadWithGroupIdData:self.dataMessage.group.id transaction:transaction]; + return [TSGroupThread getOrCreateThreadWithGroupId:self.dataMessage.group.id transaction:transaction]; } else { return [TSContactThread getOrCreateThreadWithContactId:self.recipientId transaction:transaction]; } diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index b4a9aa2b5..d247e7a75 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -307,40 +307,22 @@ NS_ASSUME_NONNULL_BEGIN } if (dataMessage.hasGroup) { - TSGroupModel *emptyModelToFillOutId = - [[TSGroupModel alloc] initWithTitle:nil memberIds:nil image:nil groupId:dataMessage.group.id]; - TSGroupThread *gThread = [TSGroupThread threadWithGroupModel:emptyModelToFillOutId transaction:transaction]; + TSGroupThread *_Nullable gThread = + [TSGroupThread threadWithGroupId:dataMessage.group.id transaction:transaction]; BOOL unknownGroup = NO; if (gThread == nil && dataMessage.group.type != OWSSignalServiceProtosGroupContextTypeUpdate) { unknownGroup = YES; } if (unknownGroup) { if (dataMessage.group.type == OWSSignalServiceProtosGroupContextTypeRequestInfo) { - DDLogInfo( - @"%@ Ignoring group info request for group I don't know about from: %@", self.tag, envelope.source); + DDLogInfo(@"%@ Ignoring group info request for unknown group from: %@", self.tag, envelope.source); + return; + } else if (dataMessage.group.type == OWSSignalServiceProtosGroupContextTypeQuit) { + DDLogInfo(@"%@ Ignoring group quit for unknown group from: %@", self.tag, envelope.source); return; } - // FIXME: https://github.com/WhisperSystems/Signal-iOS/issues/1340 - DDLogInfo(@"%@ Received message from group that I left or don't know about from: %@", - self.tag, - envelopeAddress(envelope)); - - NSString *recipientId = envelope.source; - - TSThread *thread = [TSContactThread getOrCreateThreadWithContactId:recipientId transaction:transaction]; - - NSData *groupId = dataMessage.group.id; - OWSAssert(groupId); - OWSSyncGroupsRequestMessage *syncGroupsRequestMessage = - [[OWSSyncGroupsRequestMessage alloc] initWithThread:thread groupId:groupId]; - [self.messageSender sendMessage:syncGroupsRequestMessage - success:^{ - DDLogWarn(@"%@ Successfully sent Request Group Info message.", self.tag); - } - failure:^(NSError *error) { - DDLogError(@"%@ Failed to send Request Group Info message with error: %@", self.tag, error); - }]; + [self sendGroupInfoRequest:dataMessage.group.id envelope:envelope transaction:transaction]; return; } } @@ -355,6 +337,7 @@ NS_ASSUME_NONNULL_BEGIN [self handleReceivedMediaWithEnvelope:envelope dataMessage:dataMessage transaction:transaction]; } else { [self handleReceivedTextMessageWithEnvelope:envelope dataMessage:dataMessage transaction:transaction]; + if ([self isDataMessageGroupAvatarUpdate:dataMessage]) { DDLogVerbose(@"%@ Data message had group avatar attachment", self.tag); [self handleReceivedGroupAvatarUpdateWithEnvelope:envelope dataMessage:dataMessage transaction:transaction]; @@ -362,6 +345,36 @@ NS_ASSUME_NONNULL_BEGIN } } +- (void)sendGroupInfoRequest:(NSData *)groupId + envelope:(OWSSignalServiceProtosEnvelope *)envelope + transaction:(YapDatabaseReadWriteTransaction *)transaction +{ + OWSAssert(groupId.length > 0); + OWSAssert(envelope); + OWSAssert(transaction); + + if (groupId.length < 1) { + return; + } + + // FIXME: https://github.com/WhisperSystems/Signal-iOS/issues/1340 + DDLogInfo(@"%@ Sending group info request: %@", self.tag, envelopeAddress(envelope)); + + NSString *recipientId = envelope.source; + + TSThread *thread = [TSContactThread getOrCreateThreadWithContactId:recipientId transaction:transaction]; + + OWSSyncGroupsRequestMessage *syncGroupsRequestMessage = + [[OWSSyncGroupsRequestMessage alloc] initWithThread:thread groupId:groupId]; + [self.messageSender sendMessage:syncGroupsRequestMessage + success:^{ + DDLogWarn(@"%@ Successfully sent Request Group Info message.", self.tag); + } + failure:^(NSError *error) { + DDLogError(@"%@ Failed to send Request Group Info message with error: %@", self.tag, error); + }]; +} + - (id)profileManager { return [TextSecureKitEnv sharedEnv].profileManager; @@ -445,8 +458,13 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(dataMessage); OWSAssert(transaction); - TSGroupThread *groupThread = - [TSGroupThread getOrCreateThreadWithGroupIdData:dataMessage.group.id transaction:transaction]; + TSGroupThread *_Nullable groupThread = + [TSGroupThread threadWithGroupId:dataMessage.group.id transaction:transaction]; + if (!groupThread) { + [self sendGroupInfoRequest:dataMessage.group.id envelope:envelope transaction:transaction]; + return; + } + OWSAssert(groupThread); OWSAttachmentsProcessor *attachmentsProcessor = [[OWSAttachmentsProcessor alloc] initWithAttachmentProtos:@[ dataMessage.group.avatar ] @@ -482,8 +500,12 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(dataMessage); OWSAssert(transaction); - TSThread *thread = [self threadForEnvelope:envelope dataMessage:dataMessage transaction:transaction]; - OWSAssert(thread); + TSThread *_Nullable thread = [self threadForEnvelope:envelope dataMessage:dataMessage transaction:transaction]; + if (!thread) { + OWSFail(@"%@ ignoring media message for unknown group.", self.tag); + return; + } + OWSAttachmentsProcessor *attachmentsProcessor = [[OWSAttachmentsProcessor alloc] initWithAttachmentProtos:dataMessage.attachments timestamp:envelope.timestamp @@ -559,10 +581,16 @@ NS_ASSUME_NONNULL_BEGIN if ([self isDataMessageGroupAvatarUpdate:syncMessage.sent.message]) { [recordJob runWithAttachmentHandler:^(TSAttachmentStream *attachmentStream) { - TSGroupThread *groupThread = - [TSGroupThread getOrCreateThreadWithGroupIdData:syncMessage.sent.message.group.id - transaction:transaction]; - [groupThread updateAvatarWithAttachmentStream:attachmentStream]; + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + TSGroupThread *_Nullable groupThread = + [TSGroupThread threadWithGroupId:dataMessage.group.id transaction:transaction]; + if (!groupThread) { + [self sendGroupInfoRequest:dataMessage.group.id envelope:envelope transaction:transaction]; + return; + } + + [groupThread updateAvatarWithAttachmentStream:attachmentStream transaction:transaction]; + }]; } transaction:transaction]; } else { @@ -664,7 +692,11 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(dataMessage); OWSAssert(transaction); - TSThread *thread = [self threadForEnvelope:envelope dataMessage:dataMessage transaction:transaction]; + TSThread *_Nullable thread = [self threadForEnvelope:envelope dataMessage:dataMessage transaction:transaction]; + if (!thread) { + OWSFail(@"%@ ignoring expiring messages update for unknown group.", self.tag); + return; + } OWSDisappearingMessagesConfiguration *disappearingMessagesConfiguration; if (dataMessage.hasExpireTimer && dataMessage.expireTimer > 0) { @@ -776,9 +808,7 @@ NS_ASSUME_NONNULL_BEGIN DDLogWarn(@"%@ Received 'Request Group Info' message for group: %@ from: %@", self.tag, groupId, envelope.source); - TSGroupModel *emptyModelToFillOutId = - [[TSGroupModel alloc] initWithTitle:nil memberIds:nil image:nil groupId:dataMessage.group.id]; - TSGroupThread *gThread = [TSGroupThread threadWithGroupModel:emptyModelToFillOutId transaction:transaction]; + TSGroupThread *_Nullable gThread = [TSGroupThread threadWithGroupId:dataMessage.group.id transaction:transaction]; if (!gThread) { DDLogWarn(@"%@ Unknown group: %@", self.tag, groupId); return; @@ -828,101 +858,131 @@ NS_ASSUME_NONNULL_BEGIN return nil; } - if (groupId) { - NSMutableArray *uniqueMemberIds = [[[NSSet setWithArray:dataMessage.group.members] allObjects] mutableCopy]; - TSGroupModel *model = [[TSGroupModel alloc] initWithTitle:dataMessage.group.name - memberIds:uniqueMemberIds - image:nil - groupId:dataMessage.group.id]; - TSGroupThread *gThread = [TSGroupThread getOrCreateThreadWithGroupModel:model transaction:transaction]; + if (groupId.length > 0) { + NSMutableSet *newMemberIds = [NSMutableSet setWithArray:dataMessage.group.members]; + + // Group messages create the group if it doesn't already exist. + // + // We distinguish between the old group state (if any) and the new group state. + TSGroupThread *_Nullable oldGroupThread = [TSGroupThread threadWithGroupId:groupId transaction:transaction]; + if (oldGroupThread) { + // Don't trust other clients; ensure all known group members leave the group + // _unless_ it is a "quit" message in which case we should explicitly remove + // just the quiting member below. + [newMemberIds addObjectsFromArray:oldGroupThread.groupModel.groupMemberIds]; + } switch (dataMessage.group.type) { case OWSSignalServiceProtosGroupContextTypeUpdate: { - NSString *updateGroupInfo = - [gThread.groupModel getInfoStringAboutUpdateTo:model contactsManager:self.contactsManager]; - gThread.groupModel = model; - [gThread saveWithTransaction:transaction]; + // Ensures that the thread exists but doesn't update it. + TSGroupThread *newGroupThread = + [TSGroupThread getOrCreateThreadWithGroupId:groupId transaction:transaction]; + + TSGroupModel *newGroupModel = [[TSGroupModel alloc] initWithTitle:dataMessage.group.name + memberIds:[newMemberIds.allObjects mutableCopy] + image:nil + groupId:dataMessage.group.id]; + NSString *updateGroupInfo = [newGroupThread.groupModel getInfoStringAboutUpdateTo:newGroupModel + contactsManager:self.contactsManager]; + newGroupThread.groupModel = newGroupModel; + [newGroupThread saveWithTransaction:transaction]; + [[[TSInfoMessage alloc] initWithTimestamp:timestamp - inThread:gThread + inThread:newGroupThread messageType:TSInfoMessageTypeGroupUpdate customMessage:updateGroupInfo] saveWithTransaction:transaction]; + thread = newGroupThread; break; } case OWSSignalServiceProtosGroupContextTypeQuit: { - NSString *nameString = [self.contactsManager displayNameForPhoneIdentifier:envelope.source]; + if (!oldGroupThread) { + DDLogInfo(@"%@ ignoring quit group message from unknown group.", self.tag); + return nil; + } + [newMemberIds removeObject:envelope.source]; + oldGroupThread.groupModel.groupMemberIds = [newMemberIds.allObjects mutableCopy]; + [oldGroupThread saveWithTransaction:transaction]; + NSString *nameString = [self.contactsManager displayNameForPhoneIdentifier:envelope.source]; NSString *updateGroupInfo = [NSString stringWithFormat:NSLocalizedString(@"GROUP_MEMBER_LEFT", @""), nameString]; - NSMutableArray *newGroupMembers = [NSMutableArray arrayWithArray:gThread.groupModel.groupMemberIds]; - [newGroupMembers removeObject:envelope.source]; - gThread.groupModel.groupMemberIds = newGroupMembers; - - [gThread saveWithTransaction:transaction]; [[[TSInfoMessage alloc] initWithTimestamp:timestamp - inThread:gThread + inThread:oldGroupThread messageType:TSInfoMessageTypeGroupUpdate customMessage:updateGroupInfo] saveWithTransaction:transaction]; + thread = oldGroupThread; break; } case OWSSignalServiceProtosGroupContextTypeDeliver: { + if (!oldGroupThread) { + DDLogInfo(@"%@ ignoring quit group message from unknown group.", self.tag); + [self sendGroupInfoRequest:groupId envelope:envelope transaction:transaction]; + return nil; + } + if (body.length == 0 && attachmentIds.count < 1) { DDLogWarn(@"%@ ignoring empty incoming message from: %@ for group: %@ with timestamp: %lu", self.tag, envelopeAddress(envelope), groupId, (unsigned long)timestamp); - } else { - DDLogDebug(@"%@ incoming message from: %@ for group: %@ with timestamp: %lu", - self.tag, - envelopeAddress(envelope), - groupId, - (unsigned long)timestamp); - incomingMessage = [[TSIncomingMessage alloc] initWithTimestamp:timestamp - inThread:gThread - authorId:envelope.source - sourceDeviceId:envelope.sourceDevice - messageBody:body - attachmentIds:attachmentIds - expiresInSeconds:dataMessage.expireTimer]; - - [incomingMessage saveWithTransaction:transaction]; + return nil; } + + DDLogDebug(@"%@ incoming message from: %@ for group: %@ with timestamp: %lu", + self.tag, + envelopeAddress(envelope), + groupId, + (unsigned long)timestamp); + incomingMessage = [[TSIncomingMessage alloc] initWithTimestamp:timestamp + inThread:oldGroupThread + authorId:envelope.source + sourceDeviceId:envelope.sourceDevice + messageBody:body + attachmentIds:attachmentIds + expiresInSeconds:dataMessage.expireTimer]; + + [incomingMessage saveWithTransaction:transaction]; + thread = oldGroupThread; break; } default: { DDLogWarn(@"%@ Ignoring unknown group message type: %d", self.tag, (int)dataMessage.group.type); + return nil; } } - - thread = gThread; } else { if (body.length == 0 && attachmentIds.count < 1) { DDLogWarn(@"%@ ignoring empty incoming message from: %@ with timestamp: %lu", self.tag, envelopeAddress(envelope), (unsigned long)timestamp); - } else { - DDLogDebug(@"%@ incoming message from: %@ with timestamp: %lu", - self.tag, - envelopeAddress(envelope), - (unsigned long)timestamp); - TSContactThread *cThread = [TSContactThread getOrCreateThreadWithContactId:envelope.source - transaction:transaction - relay:envelope.relay]; - - incomingMessage = [[TSIncomingMessage alloc] initWithTimestamp:timestamp - inThread:cThread - authorId:[cThread contactIdentifier] - sourceDeviceId:envelope.sourceDevice - messageBody:body - attachmentIds:attachmentIds - expiresInSeconds:dataMessage.expireTimer]; - - [incomingMessage saveWithTransaction:transaction]; - thread = cThread; + return nil; } + + DDLogDebug(@"%@ incoming message from: %@ with timestamp: %lu", + self.tag, + envelopeAddress(envelope), + (unsigned long)timestamp); + TSContactThread *cThread = [TSContactThread getOrCreateThreadWithContactId:envelope.source + transaction:transaction + relay:envelope.relay]; + + incomingMessage = [[TSIncomingMessage alloc] initWithTimestamp:timestamp + inThread:cThread + authorId:[cThread contactIdentifier] + sourceDeviceId:envelope.sourceDevice + messageBody:body + attachmentIds:attachmentIds + expiresInSeconds:dataMessage.expireTimer]; + + [incomingMessage saveWithTransaction:transaction]; + thread = cThread; } + OWSAssert(thread); + OWSAssert(incomingMessage); + if (thread && incomingMessage) { // Any messages sent from the current user - from this device or another - should be // automatically marked as read. @@ -949,9 +1009,7 @@ NS_ASSUME_NONNULL_BEGIN DDLogDebug(@"%@ incoming extra text message: %@", self.tag, incomingMessage.debugDescription); [textMessage saveWithTransaction:transaction]; } - } - if (thread && incomingMessage) { // In case we already have a read receipt for this new message (this happens sometimes). [OWSReadReceiptManager.sharedManager applyEarlyReadReceiptsForIncomingMessage:incomingMessage transaction:transaction]; @@ -981,18 +1039,25 @@ NS_ASSUME_NONNULL_BEGIN /** * @returns - * Group or Contact thread for message, creating a new one if necessary. + * Group or Contact thread for message, creating a new contact thread if necessary, + * but never creating a new group thread. */ -- (TSThread *)threadForEnvelope:(OWSSignalServiceProtosEnvelope *)envelope - dataMessage:(OWSSignalServiceProtosDataMessage *)dataMessage - transaction:(YapDatabaseReadWriteTransaction *)transaction +- (nullable TSThread *)threadForEnvelope:(OWSSignalServiceProtosEnvelope *)envelope + dataMessage:(OWSSignalServiceProtosDataMessage *)dataMessage + transaction:(YapDatabaseReadWriteTransaction *)transaction { OWSAssert(envelope); OWSAssert(dataMessage); OWSAssert(transaction); if (dataMessage.hasGroup) { - return [TSGroupThread getOrCreateThreadWithGroupIdData:dataMessage.group.id transaction:transaction]; + NSData *groupId = dataMessage.group.id; + OWSAssert(groupId.length > 0); + TSGroupThread *_Nullable groupThread = [TSGroupThread threadWithGroupId:groupId transaction:transaction]; + // This method should only be called from a code path that has already verified + // that this is a "known" group. + OWSAssert(groupThread); + return groupThread; } else { return [TSContactThread getOrCreateThreadWithContactId:envelope.source transaction:transaction]; } From 2a5a0929e6a93407e3b0486000276a7a6d23fc1f Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 3 Oct 2017 13:59:46 -0400 Subject: [PATCH 2/3] Create & access groups more carefully. // FREEBIE --- SignalServiceKit/src/Messages/OWSMessageManager.m | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index d247e7a75..085138c30 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -461,7 +461,7 @@ NS_ASSUME_NONNULL_BEGIN TSGroupThread *_Nullable groupThread = [TSGroupThread threadWithGroupId:dataMessage.group.id transaction:transaction]; if (!groupThread) { - [self sendGroupInfoRequest:dataMessage.group.id envelope:envelope transaction:transaction]; + OWSFail(@"%@ Missing group for group avatar update", self.tag); return; } @@ -585,7 +585,7 @@ NS_ASSUME_NONNULL_BEGIN TSGroupThread *_Nullable groupThread = [TSGroupThread threadWithGroupId:dataMessage.group.id transaction:transaction]; if (!groupThread) { - [self sendGroupInfoRequest:dataMessage.group.id envelope:envelope transaction:transaction]; + OWSFail(@"%@ ignoring sync group avatar update for unknown group.", self.tag); return; } @@ -915,8 +915,7 @@ NS_ASSUME_NONNULL_BEGIN } case OWSSignalServiceProtosGroupContextTypeDeliver: { if (!oldGroupThread) { - DDLogInfo(@"%@ ignoring quit group message from unknown group.", self.tag); - [self sendGroupInfoRequest:groupId envelope:envelope transaction:transaction]; + OWSFail(@"%@ ignoring quit group message from unknown group.", self.tag); return nil; } From 13a665799120a86c90fe1ca9b12e178fc2b41bcb Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 4 Oct 2017 10:06:38 -0400 Subject: [PATCH 3/3] Respond to CR. // FREEBIE --- .../src/Account/TSAccountManager.m | 4 + .../src/Messages/OWSMessageManager.m | 202 ++++++++++-------- 2 files changed, 115 insertions(+), 91 deletions(-) diff --git a/SignalServiceKit/src/Account/TSAccountManager.m b/SignalServiceKit/src/Account/TSAccountManager.m index 5d4bdaf7e..a0b97d3d4 100644 --- a/SignalServiceKit/src/Account/TSAccountManager.m +++ b/SignalServiceKit/src/Account/TSAccountManager.m @@ -108,6 +108,10 @@ NSString *const TSAccountManager_LocalRegistrationIdKey = @"TSStorageLocalRegist [[NSNotificationCenter defaultCenter] postNotificationNameAsync:kNSNotificationName_RegistrationStateDidChange object:nil userInfo:nil]; + + // Warm these cached values. + [self isRegistered]; + [self localNumber]; } + (nullable NSString *)localNumber diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index 085138c30..e30872c97 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -307,23 +307,20 @@ NS_ASSUME_NONNULL_BEGIN } if (dataMessage.hasGroup) { - TSGroupThread *_Nullable gThread = + TSGroupThread *_Nullable groupThread = [TSGroupThread threadWithGroupId:dataMessage.group.id transaction:transaction]; - BOOL unknownGroup = NO; - if (gThread == nil && dataMessage.group.type != OWSSignalServiceProtosGroupContextTypeUpdate) { - unknownGroup = YES; - } - if (unknownGroup) { - if (dataMessage.group.type == OWSSignalServiceProtosGroupContextTypeRequestInfo) { - DDLogInfo(@"%@ Ignoring group info request for unknown group from: %@", self.tag, envelope.source); + + if (!groupThread) { + // Unknown group. + if (dataMessage.group.type == OWSSignalServiceProtosGroupContextTypeUpdate) { + // Accept group updates for unknown groups. + } else if (dataMessage.group.type == OWSSignalServiceProtosGroupContextTypeDeliver) { + [self sendGroupInfoRequest:dataMessage.group.id envelope:envelope transaction:transaction]; return; - } else if (dataMessage.group.type == OWSSignalServiceProtosGroupContextTypeQuit) { - DDLogInfo(@"%@ Ignoring group quit for unknown group from: %@", self.tag, envelope.source); + } else { + DDLogInfo(@"%@ Ignoring group message for unknown group from: %@", self.tag, envelope.source); return; } - - [self sendGroupInfoRequest:dataMessage.group.id envelope:envelope transaction:transaction]; - return; } } @@ -846,13 +843,6 @@ NS_ASSUME_NONNULL_BEGIN NSString *body = dataMessage.body; NSData *groupId = dataMessage.hasGroup ? dataMessage.group.id : nil; - __block TSIncomingMessage *_Nullable incomingMessage; - __block TSThread *thread; - - // Do this outside of a transaction to avoid deadlock - OWSAssert([TSAccountManager isRegistered]); - NSString *localNumber = [TSAccountManager localNumber]; - if (dataMessage.group.type == OWSSignalServiceProtosGroupContextTypeRequestInfo) { [self handleGroupInfoRequest:envelope dataMessage:dataMessage transaction:transaction]; return nil; @@ -866,9 +856,9 @@ NS_ASSUME_NONNULL_BEGIN // We distinguish between the old group state (if any) and the new group state. TSGroupThread *_Nullable oldGroupThread = [TSGroupThread threadWithGroupId:groupId transaction:transaction]; if (oldGroupThread) { - // Don't trust other clients; ensure all known group members leave the group - // _unless_ it is a "quit" message in which case we should explicitly remove - // just the quiting member below. + // Don't trust other clients; ensure all known group members remain in the + // group unless it is a "quit" message in which case we should only remove + // the quiting member below. [newMemberIds addObjectsFromArray:oldGroupThread.groupModel.groupMemberIds]; } @@ -880,7 +870,7 @@ NS_ASSUME_NONNULL_BEGIN TSGroupModel *newGroupModel = [[TSGroupModel alloc] initWithTitle:dataMessage.group.name memberIds:[newMemberIds.allObjects mutableCopy] - image:nil + image:oldGroupThread.groupModel.groupImage groupId:dataMessage.group.id]; NSString *updateGroupInfo = [newGroupThread.groupModel getInfoStringAboutUpdateTo:newGroupModel contactsManager:self.contactsManager]; @@ -891,8 +881,7 @@ NS_ASSUME_NONNULL_BEGIN inThread:newGroupThread messageType:TSInfoMessageTypeGroupUpdate customMessage:updateGroupInfo] saveWithTransaction:transaction]; - thread = newGroupThread; - break; + return nil; } case OWSSignalServiceProtosGroupContextTypeQuit: { if (!oldGroupThread) { @@ -910,12 +899,11 @@ NS_ASSUME_NONNULL_BEGIN inThread:oldGroupThread messageType:TSInfoMessageTypeGroupUpdate customMessage:updateGroupInfo] saveWithTransaction:transaction]; - thread = oldGroupThread; - break; + return nil; } case OWSSignalServiceProtosGroupContextTypeDeliver: { if (!oldGroupThread) { - OWSFail(@"%@ ignoring quit group message from unknown group.", self.tag); + OWSFail(@"%@ ignoring deliver group message from unknown group.", self.tag); return nil; } @@ -933,17 +921,21 @@ NS_ASSUME_NONNULL_BEGIN envelopeAddress(envelope), groupId, (unsigned long)timestamp); - incomingMessage = [[TSIncomingMessage alloc] initWithTimestamp:timestamp - inThread:oldGroupThread - authorId:envelope.source - sourceDeviceId:envelope.sourceDevice - messageBody:body - attachmentIds:attachmentIds - expiresInSeconds:dataMessage.expireTimer]; - - [incomingMessage saveWithTransaction:transaction]; - thread = oldGroupThread; - break; + TSIncomingMessage *incomingMessage = + [[TSIncomingMessage alloc] initWithTimestamp:timestamp + inThread:oldGroupThread + authorId:envelope.source + sourceDeviceId:envelope.sourceDevice + messageBody:body + attachmentIds:attachmentIds + expiresInSeconds:dataMessage.expireTimer]; + [self finalizeIncomingMessage:incomingMessage + thread:oldGroupThread + envelope:envelope + dataMessage:dataMessage + attachmentIds:attachmentIds + transaction:transaction]; + return incomingMessage; } default: { DDLogWarn(@"%@ Ignoring unknown group message type: %d", self.tag, (int)dataMessage.group.type); @@ -963,69 +955,97 @@ NS_ASSUME_NONNULL_BEGIN self.tag, envelopeAddress(envelope), (unsigned long)timestamp); - TSContactThread *cThread = [TSContactThread getOrCreateThreadWithContactId:envelope.source - transaction:transaction - relay:envelope.relay]; - - incomingMessage = [[TSIncomingMessage alloc] initWithTimestamp:timestamp - inThread:cThread - authorId:[cThread contactIdentifier] - sourceDeviceId:envelope.sourceDevice - messageBody:body - attachmentIds:attachmentIds - expiresInSeconds:dataMessage.expireTimer]; - - [incomingMessage saveWithTransaction:transaction]; - thread = cThread; + TSContactThread *thread = [TSContactThread getOrCreateThreadWithContactId:envelope.source + transaction:transaction + relay:envelope.relay]; + + TSIncomingMessage *incomingMessage = [[TSIncomingMessage alloc] initWithTimestamp:timestamp + inThread:thread + authorId:[thread contactIdentifier] + sourceDeviceId:envelope.sourceDevice + messageBody:body + attachmentIds:attachmentIds + expiresInSeconds:dataMessage.expireTimer]; + [self finalizeIncomingMessage:incomingMessage + thread:thread + envelope:envelope + dataMessage:dataMessage + attachmentIds:attachmentIds + transaction:transaction]; + return incomingMessage; } +} +- (void)finalizeIncomingMessage:(TSIncomingMessage *)incomingMessage + thread:(TSThread *)thread + envelope:(OWSSignalServiceProtosEnvelope *)envelope + dataMessage:(OWSSignalServiceProtosDataMessage *)dataMessage + attachmentIds:(NSArray *)attachmentIds + transaction:(YapDatabaseReadWriteTransaction *)transaction +{ OWSAssert(thread); OWSAssert(incomingMessage); + OWSAssert(envelope); + OWSAssert(dataMessage); + OWSAssert(attachmentIds); + OWSAssert(transaction); - if (thread && incomingMessage) { - // Any messages sent from the current user - from this device or another - should be - // automatically marked as read. - BOOL shouldMarkMessageAsRead = [envelope.source isEqualToString:localNumber]; - if (shouldMarkMessageAsRead) { - // Don't send a read receipt for messages sent by ourselves. - [incomingMessage markAsReadWithTransaction:transaction sendReadReceipt:NO updateExpiration:YES]; - } + OWSAssert([TSAccountManager isRegistered]); + NSString *localNumber = [TSAccountManager localNumber]; + NSString *body = dataMessage.body; + uint64_t timestamp = envelope.timestamp; + + if (!thread) { + OWSFail(@"%@ Can't finalize without thread", self.tag); + return; + } + if (!incomingMessage) { + OWSFail(@"%@ Can't finalize missing message", self.tag); + return; + } - DDLogDebug(@"%@ shouldMarkMessageAsRead: %d (%@)", self.tag, shouldMarkMessageAsRead, envelope.source); + [incomingMessage saveWithTransaction:transaction]; - // Other clients allow attachments to be sent along with body, we want the text displayed as a separate - // message - if ([attachmentIds count] > 0 && body != nil && body.length > 0) { - // We want the text to be displayed under the attachment. - uint64_t textMessageTimestamp = timestamp + 1; - TSIncomingMessage *textMessage = [[TSIncomingMessage alloc] initWithTimestamp:textMessageTimestamp - inThread:thread - authorId:envelope.source - sourceDeviceId:envelope.sourceDevice - messageBody:body - attachmentIds:@[] - expiresInSeconds:dataMessage.expireTimer]; - DDLogDebug(@"%@ incoming extra text message: %@", self.tag, incomingMessage.debugDescription); - [textMessage saveWithTransaction:transaction]; - } + // Any messages sent from the current user - from this device or another - should be + // automatically marked as read. + BOOL shouldMarkMessageAsRead = [envelope.source isEqualToString:localNumber]; + if (shouldMarkMessageAsRead) { + // Don't send a read receipt for messages sent by ourselves. + [incomingMessage markAsReadWithTransaction:transaction sendReadReceipt:NO updateExpiration:YES]; + } - // In case we already have a read receipt for this new message (this happens sometimes). - [OWSReadReceiptManager.sharedManager applyEarlyReadReceiptsForIncomingMessage:incomingMessage - transaction:transaction]; + DDLogDebug(@"%@ shouldMarkMessageAsRead: %d (%@)", self.tag, shouldMarkMessageAsRead, envelope.source); + + // Other clients allow attachments to be sent along with body, we want the text displayed as a separate + // message + if ([attachmentIds count] > 0 && body != nil && body.length > 0) { + // We want the text to be displayed under the attachment. + uint64_t textMessageTimestamp = timestamp + 1; + TSIncomingMessage *textMessage = [[TSIncomingMessage alloc] initWithTimestamp:textMessageTimestamp + inThread:thread + authorId:envelope.source + sourceDeviceId:envelope.sourceDevice + messageBody:body + attachmentIds:@[] + expiresInSeconds:dataMessage.expireTimer]; + DDLogDebug(@"%@ incoming extra text message: %@", self.tag, incomingMessage.debugDescription); + [textMessage saveWithTransaction:transaction]; + } - [OWSDisappearingMessagesJob becomeConsistentWithConfigurationForMessage:incomingMessage - contactsManager:self.contactsManager]; + // In case we already have a read receipt for this new message (this happens sometimes). + [OWSReadReceiptManager.sharedManager applyEarlyReadReceiptsForIncomingMessage:incomingMessage + transaction:transaction]; - // Update thread preview in inbox - [thread touchWithTransaction:transaction]; + [OWSDisappearingMessagesJob becomeConsistentWithConfigurationForMessage:incomingMessage + contactsManager:self.contactsManager]; - [[TextSecureKitEnv sharedEnv].notificationsManager notifyUserForIncomingMessage:incomingMessage - inThread:thread - contactsManager:self.contactsManager - transaction:transaction]; - } + // Update thread preview in inbox + [thread touchWithTransaction:transaction]; - return incomingMessage; + [[TextSecureKitEnv sharedEnv].notificationsManager notifyUserForIncomingMessage:incomingMessage + inThread:thread + contactsManager:self.contactsManager + transaction:transaction]; } #pragma mark - helpers