From 2dba7d141a4840eb96c1482e0dd2db11a854e6a3 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Fri, 23 Sep 2016 16:55:56 -0400 Subject: [PATCH] Fix contact/group sync messages. (#32) Initially we don't have device messages for our remote device. We need to send the empty device list to get the updated list of remote devices. Introduced lots of dependency injection to make MessagesManager more testable. // FREEBIE --- .../TSKitiOSTestApp.xcodeproj/project.pbxproj | 4 + src/Contacts/ContactsUpdater.h | 5 - src/Contacts/ContactsUpdater.m | 2 +- src/Messages/TSMessagesManager+attachments.m | 18 +- src/Messages/TSMessagesManager+sendMessages.m | 31 +--- src/Messages/TSMessagesManager.h | 8 + src/Messages/TSMessagesManager.m | 22 ++- tests/Messages/TSMessagesManagerTest.m | 171 ++++++++++++++++++ 8 files changed, 220 insertions(+), 41 deletions(-) create mode 100644 tests/Messages/TSMessagesManagerTest.m diff --git a/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/project.pbxproj b/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/project.pbxproj index 271be3c8d..0447848e9 100644 --- a/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/project.pbxproj +++ b/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/project.pbxproj @@ -8,6 +8,7 @@ /* Begin PBXBuildFile section */ 308D7DFA789594CEA62740D9 /* libPods-TSKitiOSTestAppTests.a in Frameworks */ = {isa = PBXBuildFile; fileRef = C0DC1A83C39CBC09FB2405A3 /* libPods-TSKitiOSTestAppTests.a */; }; + 45046FE01D95A6130015EFF2 /* TSMessagesManagerTest.m in Sources */ = {isa = PBXBuildFile; fileRef = 45046FDF1D95A6130015EFF2 /* TSMessagesManagerTest.m */; }; 452EE6CF1D4A754C00E934BA /* TSThreadTest.m in Sources */ = {isa = PBXBuildFile; fileRef = 452EE6CE1D4A754C00E934BA /* TSThreadTest.m */; }; 452EE6D51D4AC43300E934BA /* OWSOrphanedDataCleanerTest.m in Sources */ = {isa = PBXBuildFile; fileRef = 452EE6D41D4AC43300E934BA /* OWSOrphanedDataCleanerTest.m */; }; 45458B751CC342B600A02153 /* SignedPreKeyDeletionTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 45458B6A1CC342B600A02153 /* SignedPreKeyDeletionTests.m */; }; @@ -46,6 +47,7 @@ 1A50A62A8930EE2BC9B8AC11 /* Pods-TSKitiOSTestApp.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-TSKitiOSTestApp.release.xcconfig"; path = "Pods/Target Support Files/Pods-TSKitiOSTestApp/Pods-TSKitiOSTestApp.release.xcconfig"; sourceTree = ""; }; 31DFDA8F9523F5B15EA2376B /* Pods-TSKitiOSTestApp.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-TSKitiOSTestApp.debug.xcconfig"; path = "Pods/Target Support Files/Pods-TSKitiOSTestApp/Pods-TSKitiOSTestApp.debug.xcconfig"; sourceTree = ""; }; 36DA6C703F99948D553F4E3F /* Pods-TSKitiOSTestAppTests.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-TSKitiOSTestAppTests.debug.xcconfig"; path = "Pods/Target Support Files/Pods-TSKitiOSTestAppTests/Pods-TSKitiOSTestAppTests.debug.xcconfig"; sourceTree = ""; }; + 45046FDF1D95A6130015EFF2 /* TSMessagesManagerTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = TSMessagesManagerTest.m; path = ../../../tests/Messages/TSMessagesManagerTest.m; sourceTree = ""; }; 452EE6CE1D4A754C00E934BA /* TSThreadTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = TSThreadTest.m; sourceTree = ""; }; 452EE6D41D4AC43300E934BA /* OWSOrphanedDataCleanerTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OWSOrphanedDataCleanerTest.m; sourceTree = ""; }; 45458B6A1CC342B600A02153 /* SignedPreKeyDeletionTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SignedPreKeyDeletionTests.m; sourceTree = ""; }; @@ -154,6 +156,7 @@ isa = PBXGroup; children = ( 45C6A0981D2F0264007D8AC0 /* Interactions */, + 45046FDF1D95A6130015EFF2 /* TSMessagesManagerTest.m */, ); name = Messages; sourceTree = ""; @@ -476,6 +479,7 @@ 459850C11D22C6F2006FFEDB /* PhoneNumberTest.m in Sources */, 45458B7A1CC342B600A02153 /* TSStorageSignedPreKeyStore.m in Sources */, 45458B771CC342B600A02153 /* TSMessageStorageTests.m in Sources */, + 45046FE01D95A6130015EFF2 /* TSMessagesManagerTest.m in Sources */, 45458B7C1CC342B600A02153 /* MessagePaddingTests.m in Sources */, 45A856AC1D220BFF0056CD4D /* TSAttributesTest.m in Sources */, 6323E339D5B8F4CB77EB3ED4 /* SignalRecipientTest.m in Sources */, diff --git a/src/Contacts/ContactsUpdater.h b/src/Contacts/ContactsUpdater.h index 1f25bece5..a3a6bfd1f 100644 --- a/src/Contacts/ContactsUpdater.h +++ b/src/Contacts/ContactsUpdater.h @@ -1,10 +1,5 @@ -// -// ContactsManager+updater.h -// Signal -// // Created by Frederic Jacobs on 21/11/15. // Copyright © 2015 Open Whisper Systems. All rights reserved. -// #import "SignalRecipient.h" diff --git a/src/Contacts/ContactsUpdater.m b/src/Contacts/ContactsUpdater.m index 88e68e0d4..cecd775f1 100644 --- a/src/Contacts/ContactsUpdater.m +++ b/src/Contacts/ContactsUpdater.m @@ -17,7 +17,7 @@ static dispatch_once_t onceToken; static id sharedInstance = nil; dispatch_once(&onceToken, ^{ - sharedInstance = [self.class new]; + sharedInstance = [self new]; }); return sharedInstance; } diff --git a/src/Messages/TSMessagesManager+attachments.m b/src/Messages/TSMessagesManager+attachments.m index 6b4259c32..b756d46fb 100644 --- a/src/Messages/TSMessagesManager+attachments.m +++ b/src/Messages/TSMessagesManager+attachments.m @@ -51,13 +51,13 @@ dispatch_queue_t attachmentsQueue() { thread = [TSContactThread getOrCreateThreadWithContactId:envelope.source]; } - OWSAttachmentsProcessor *attachmentsProcessor = - [[OWSAttachmentsProcessor alloc] initWithAttachmentPointersProtos:attachmentPointerProtos - timestamp:envelope.timestamp - relay:envelope.relay - avatarGroupId:avatarGroupId - inThread:thread - messagesManager:[TSMessagesManager sharedManager]]; + OWSAttachmentsProcessor *attachmentsProcessor = [[OWSAttachmentsProcessor alloc] + initWithAttachmentPointersProtos:attachmentPointerProtos + timestamp:envelope.timestamp + relay:envelope.relay + avatarGroupId:avatarGroupId + inThread:thread + messagesManager:[TSMessagesManager sharedManager]]; // TODO self? if (attachmentsProcessor.hasSupportedAttachments) { TSIncomingMessage *possiblyCreatedMessage = @@ -106,7 +106,7 @@ dispatch_queue_t attachmentsQueue() { success:(successSendingCompletionBlock)successCompletionBlock failure:(failedSendingCompletionBlock)failedCompletionBlock { TSRequest *allocateAttachment = [[TSAllocAttachmentRequest alloc] init]; - [[TSNetworkManager sharedManager] makeRequest:allocateAttachment + [self.networkManager makeRequest:allocateAttachment success:^(NSURLSessionDataTask *task, id responseObject) { dispatch_async(attachmentsQueue(), ^{ if ([responseObject isKindOfClass:[NSDictionary class]]) { @@ -187,7 +187,7 @@ dispatch_queue_t attachmentsQueue() { TSAttachmentRequest *attachmentRequest = [[TSAttachmentRequest alloc] initWithId:[attachment identifier] relay:attachment.relay]; - [[TSNetworkManager sharedManager] makeRequest:attachmentRequest + [self.networkManager makeRequest:attachmentRequest success:^(NSURLSessionDataTask *task, id responseObject) { if ([responseObject isKindOfClass:[NSDictionary class]]) { dispatch_async(attachmentsQueue(), ^{ diff --git a/src/Messages/TSMessagesManager+sendMessages.m b/src/Messages/TSMessagesManager+sendMessages.m index 467fe8ce0..af72e710e 100644 --- a/src/Messages/TSMessagesManager+sendMessages.m +++ b/src/Messages/TSMessagesManager+sendMessages.m @@ -26,7 +26,10 @@ #define InvalidDeviceException @"InvalidDeviceException" @interface TSMessagesManager () + dispatch_queue_t sendingQueue(void); +@property TSNetworkManager *networkManager; + @end typedef void (^messagesQueue)(NSArray *messages); @@ -56,7 +59,7 @@ dispatch_queue_t sendingQueue() { if (!recipient) { - [[self contactUpdater] synchronousLookup:recipientId + [self.contactsUpdater synchronousLookup:recipientId success:^(SignalRecipient *newRecipient) { [recipients addObject:newRecipient]; } @@ -78,10 +81,6 @@ dispatch_queue_t sendingQueue() { return; } -- (ContactsUpdater *)contactUpdater { - return [ContactsUpdater sharedUpdater]; -} - - (void)resendMessage:(TSOutgoingMessage *)message toRecipient:(SignalRecipient *)recipient inThread:(TSThread *)thread @@ -142,7 +141,7 @@ dispatch_queue_t sendingQueue() { }]; if (!recipient) { - [[ContactsUpdater sharedUpdater] synchronousLookup:contactThread.contactIdentifier + [self.contactsUpdater synchronousLookup:contactThread.contactIdentifier success:^(SignalRecipient *recip) { recipient = recip; } @@ -172,7 +171,7 @@ dispatch_queue_t sendingQueue() { // Special situation: if we are sending to ourselves in a single thread, we treat this as an incoming // message [self handleMessageSent:message]; - [[TSMessagesManager sharedManager] handleSendToMyself:message]; + [[TSMessagesManager sharedManager] handleSendToMyself:message]; // TODO self? } } }); @@ -261,26 +260,12 @@ dispatch_queue_t sendingQueue() { } } - if (deviceMessages.count == 0) { - DDLogWarn(@"%@ Failed to build any device messages. Not sending.", self.tag); - // Retrying incase we fixed our stale devices last time 'round. - dispatch_async(sendingQueue(), ^{ - [self sendMessage:message - toRecipient:recipient - inThread:thread - withAttemps:remainingAttempts - success:successBlock - failure:failureBlock]; - }); - return; - } - TSSubmitMessageRequest *request = [[TSSubmitMessageRequest alloc] initWithRecipient:recipient.uniqueId messages:deviceMessages relay:recipient.relay timeStamp:message.timestamp]; - [[TSNetworkManager sharedManager] makeRequest:request + [self.networkManager makeRequest:request success:^(NSURLSessionDataTask *task, id responseObject) { dispatch_async(sendingQueue(), ^{ [recipient save]; @@ -456,7 +441,7 @@ dispatch_queue_t sendingQueue() { __block dispatch_semaphore_t sema = dispatch_semaphore_create(0); __block PreKeyBundle *bundle; - [[TSNetworkManager sharedManager] + [self.networkManager makeRequest:[[TSRecipientPrekeyRequest alloc] initWithRecipient:identifier deviceId:[deviceNumber stringValue]] success:^(NSURLSessionDataTask *task, id responseObject) { diff --git a/src/Messages/TSMessagesManager.h b/src/Messages/TSMessagesManager.h index 5063df57a..a9b6cc740 100644 --- a/src/Messages/TSMessagesManager.h +++ b/src/Messages/TSMessagesManager.h @@ -7,14 +7,22 @@ @class TSCall; @class YapDatabaseConnection; +@class TSNetworkManager; @class OWSSignalServiceProtosEnvelope; @class OWSSignalServiceProtosDataMessage; +@class ContactsUpdater; @interface TSMessagesManager : NSObject +- (instancetype)initWithNetworkManager:(TSNetworkManager *)networkManager + dbConnection:(YapDatabaseConnection *)dbConnection + contactsUpdater:(ContactsUpdater *)contactsUpdater NS_DESIGNATED_INITIALIZER; + + (instancetype)sharedManager; @property (readonly) YapDatabaseConnection *dbConnection; +@property (readonly) TSNetworkManager *networkManager; +@property (readonly) ContactsUpdater *contactsUpdater; - (void)handleReceivedEnvelope:(OWSSignalServiceProtosEnvelope *)envelope; diff --git a/src/Messages/TSMessagesManager.m b/src/Messages/TSMessagesManager.m index 638bf7bfc..6956b3ea6 100644 --- a/src/Messages/TSMessagesManager.m +++ b/src/Messages/TSMessagesManager.m @@ -3,6 +3,7 @@ #import "TSMessagesManager.h" #import "ContactsManagerProtocol.h" +#import "ContactsUpdater.h" #import "MimeTypeUtil.h" #import "NSData+messagePadding.h" #import "OWSIncomingSentMessageTranscript.h" @@ -19,6 +20,7 @@ #import "TSInfoMessage.h" #import "TSInvalidIdentityKeyReceivingErrorMessage.h" #import "TSMessagesManager+attachments.h" +#import "TSNetworkManager.h" #import "TSStorageHeaders.h" #import "TextSecureKitEnv.h" #import @@ -35,13 +37,27 @@ return sharedMyManager; } -- (instancetype)init { +- (instancetype)init +{ + return [self initWithNetworkManager:[TSNetworkManager sharedManager] + dbConnection:[TSStorageManager sharedManager].newDatabaseConnection + contactsUpdater:[ContactsUpdater sharedUpdater]]; +} + +- (instancetype)initWithNetworkManager:(TSNetworkManager *)networkManager + dbConnection:(YapDatabaseConnection *)dbConnection + contactsUpdater:(ContactsUpdater *)contactsUpdater +{ self = [super init]; - if (self) { - _dbConnection = [TSStorageManager sharedManager].newDatabaseConnection; + if (!self) { + return self; } + _networkManager = networkManager; + _dbConnection = dbConnection; + _contactsUpdater = contactsUpdater; + return self; } diff --git a/tests/Messages/TSMessagesManagerTest.m b/tests/Messages/TSMessagesManagerTest.m new file mode 100644 index 000000000..ffd0a73eb --- /dev/null +++ b/tests/Messages/TSMessagesManagerTest.m @@ -0,0 +1,171 @@ +// Created by Michael Kirk on 9/23/16. +// Copyright © 2016 Open Whisper Systems. All rights reserved. + +#import + +#import "ContactsUpdater.h" +#import "OWSSignalServiceProtos.pb.h" +#import "TSMessagesManager.h" +#import "TSNetworkManager.h" +#import "TSStorageManager.h" +#import "objc/runtime.h" + +@interface TSMessagesManagerTest : XCTestCase + +@end + +@interface TSMessagesManager (Testing) + +// private method we are testing +- (void)handleIncomingEnvelope:(OWSSignalServiceProtosEnvelope *)messageEnvelope + withSyncMessage:(OWSSignalServiceProtosSyncMessage *)syncMessage; + +// private method we are stubbing via swizzle. +- (BOOL)uploadDataWithProgress:(NSData *)cipherText location:(NSString *)location attachmentID:(NSString *)attachmentID; + +@end + +@implementation TSMessagesManager (Testing) + ++ (void)swapOriginalSelector:(SEL)originalSelector replacement:(SEL)replacementSelector +{ + Class class = [self class]; + Method originalMethod = class_getInstanceMethod(class, originalSelector); + Method replacementMethod = class_getInstanceMethod(class, replacementSelector); + + // When swizzling a class method, use the following: + // Class class = object_getClass((id)self); + // ... + // Method originalMethod = class_getClassMethod(class, originalSelector); + // Method swizzledMethod = class_getClassMethod(class, swizzledSelector); + + BOOL didAddMethod = class_addMethod(class, + originalSelector, + method_getImplementation(replacementMethod), + method_getTypeEncoding(replacementMethod)); + + if (didAddMethod) { + class_replaceMethod(class, + replacementSelector, + method_getImplementation(originalMethod), + method_getTypeEncoding(originalMethod)); + } else { + method_exchangeImplementations(originalMethod, replacementMethod); + } +} + ++ (void)load +{ + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + [self swapOriginalSelector:@selector(uploadDataWithProgress:location:attachmentID:) + replacement:@selector(stubbedUploadDataWithProgress:location:attachmentID:)]; + + [self swapOriginalSelector:@selector(deviceMessages:forRecipient:inThread:) + replacement:@selector(stubbedDeviceMessages:forRecipient:inThread:)]; + }); +} + +#pragma mark - Method Swizzling + +- (BOOL)stubbedUploadDataWithProgress:(NSData *)cipherText + location:(NSString *)location + attachmentID:(NSString *)attachmentID +{ + NSLog(@"Faking successful upload."); + return YES; +} + +- (NSArray *)stubbedDeviceMessages:(TSOutgoingMessage *)message + forRecipient:(SignalRecipient *)recipient + inThread:(TSThread *)thread +{ + // Upon originally provisioning, we won't have a device to send to. + NSLog(@"Stubbed device message to return empty list."); + return @[]; +} + +@end + + +@interface OWSTSMessagesManagerTestNetworkManager : TSNetworkManager + +- (instancetype)initWithExpectation:(XCTestExpectation *)messageWasSubmitted; + +@property XCTestExpectation *messageWasSubmitted; + +@end + +@implementation OWSTSMessagesManagerTestNetworkManager + +- (instancetype)initWithExpectation:(XCTestExpectation *)messageWasSubmitted +{ + _messageWasSubmitted = messageWasSubmitted; + + return self; +} + +- (void)makeRequest:(TSRequest *)request + success:(void (^)(NSURLSessionDataTask *task, id responseObject))success + failure:(void (^)(NSURLSessionDataTask *task, NSError *error))failure +{ + if ([request isKindOfClass:[TSAllocAttachmentRequest class]]) { + NSDictionary *fakeResponse = @{ @"id" : @(1234), @"location" : @"fake-location" }; + success(nil, fakeResponse); + } else if ([request isKindOfClass:[TSSubmitMessageRequest class]]) { + [self.messageWasSubmitted fulfill]; + } else { + NSLog(@"Ignoring unhandled request: %@", request); + } +} + +@end + +@interface OWSFakeContactsUpdater : ContactsUpdater + +@end + +@implementation OWSFakeContactsUpdater + +- (void)synchronousLookup:(NSString *)identifier + success:(void (^)(SignalRecipient *))success + failure:(void (^)(NSError *error))failure +{ + NSLog(@"Fake contact lookup."); + SignalRecipient *fakeRecipient = + [[SignalRecipient alloc] initWithTextSecureIdentifier:@"fake-recipient-id" relay:nil supportsVoice:YES]; + success(fakeRecipient); +} + +@end + +@implementation TSMessagesManagerTest + +- (void)testIncomingSyncContactMessage +{ + OWSFakeContactsUpdater *fakeContactsUpdater = [OWSFakeContactsUpdater new]; + XCTestExpectation *messageWasSubmitted = [self expectationWithDescription:@"message was submitted"]; + OWSTSMessagesManagerTestNetworkManager *fakeNetworkManager = + [[OWSTSMessagesManagerTestNetworkManager alloc] initWithExpectation:messageWasSubmitted]; + TSMessagesManager *messagesManager = + [[TSMessagesManager alloc] initWithNetworkManager:fakeNetworkManager + dbConnection:[TSStorageManager sharedManager].newDatabaseConnection + contactsUpdater:fakeContactsUpdater]; + + OWSSignalServiceProtosEnvelopeBuilder *envelopeBuilder = [OWSSignalServiceProtosEnvelopeBuilder new]; + OWSSignalServiceProtosSyncMessageBuilder *messageBuilder = [OWSSignalServiceProtosSyncMessageBuilder new]; + OWSSignalServiceProtosSyncMessageRequestBuilder *requestBuilder = + [OWSSignalServiceProtosSyncMessageRequestBuilder new]; + [requestBuilder setType:OWSSignalServiceProtosSyncMessageRequestTypeGroups]; + [messageBuilder setRequest:[requestBuilder build]]; + + [messagesManager handleIncomingEnvelope:[envelopeBuilder build] withSyncMessage:[messageBuilder build]]; + + [self waitForExpectationsWithTimeout:5 + handler:^(NSError *error) { + NSLog(@"No message submitted."); + }]; +} + + +@end