From b276e8eb027b671d57b8440c56bb97319dc9ffd6 Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Tue, 14 Jul 2020 19:12:21 -0700 Subject: [PATCH] IOS-445: Do CDS lookup during message send for recipients without UUID This adds a new async operation in the message send flow. MessageSender will now verify that the intended recipients of a message all have UUIDs before proceeding. If any recipients are missing a UUID, it'll kick off a CDS lookup for the invalid recipients. While this code appears to work, this is a first pass. Looking to get some inital feedback in a pull request. It's unclear to me whether or not this pattern fits in with the existing error propogation flow in MessagSender promises. It's also unclear whether or not the error I'm using is correct. Some other changes that come along for the ride: - Add a -perform() method to ContactDiscoveryService to automatically submit itself and its dependencies to its static operation queue --- .../OWSContactDiscoveryOperation.swift | 9 ++- .../src/Contacts/UUIDBackfillTask.swift | 3 +- .../src/Messages/OWSMessageSender.m | 56 +++++++++++-------- .../src/Messages/OWSMessageSender.swift | 56 +++++++++++++++++++ SignalServiceKit/src/SignalServiceKit.h | 1 + 5 files changed, 97 insertions(+), 28 deletions(-) diff --git a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift index fb8eb909e1..6525b6e4bb 100644 --- a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift +++ b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift @@ -95,8 +95,7 @@ public class LegacyContactDiscoveryOperation: OWSOperation { // Compare against new CDS service let modernContactDiscoveryOperation = ContactDiscoveryOperation(phoneNumbersToLookup: self.phoneNumbersToLookup) - let operations = modernContactDiscoveryOperation.dependencies + [modernContactDiscoveryOperation] - ContactDiscoveryOperation.operationQueue.addOperations(operations, waitUntilFinished: false) + modernContactDiscoveryOperation.perform() guard let legacyRegisteredPhoneNumbers = self.registeredPhoneNumbers else { owsFailDebug("legacyRegisteredPhoneNumbers was unexpectedly nil") @@ -187,6 +186,12 @@ public class ContactDiscoveryOperation: OWSOperation { } } + /// Asynchronously start the operation and its dependencies + @objc public func perform() { + let operationSet = self.dependencies + [self] + Self.operationQueue.addOperations(operationSet, waitUntilFinished: false) + } + // MARK: Mandatory overrides // Called every retry, this is where the bulk of the operation's work should go. diff --git a/SignalServiceKit/src/Contacts/UUIDBackfillTask.swift b/SignalServiceKit/src/Contacts/UUIDBackfillTask.swift index 80401311cc..6296483917 100644 --- a/SignalServiceKit/src/Contacts/UUIDBackfillTask.swift +++ b/SignalServiceKit/src/Contacts/UUIDBackfillTask.swift @@ -210,8 +210,7 @@ extension UUIDBackfillTask { operation.completionBlock = { completion(operation.registeredContacts, operation.failingError) } - ContactDiscoveryOperation.operationQueue.addOperations(operation.dependencies, waitUntilFinished: false) - ContactDiscoveryOperation.operationQueue.addOperation(operation) + operation.perform() } } diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 925bcceb95..4c3c97a248 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -683,22 +683,6 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; return recipientAddresses.allObjects; } -- (NSArray *)recipientsForAddresses:(NSArray *)addresses -{ - OWSAssertDebug(!NSThread.isMainThread); - OWSAssertDebug(addresses.count > 0); - - NSMutableArray *recipients = [NSMutableArray new]; - [self.databaseStorage readWithBlock:^(SDSAnyReadTransaction *transaction) { - for (SignalServiceAddress *address in addresses) { - SignalRecipient *recipient = [SignalRecipient getOrBuildUnsavedRecipientForAddress:address - transaction:transaction]; - [recipients addObject:recipient]; - } - }]; - return [recipients copy]; -} - - (AnyPromise *)unlockPreKeyUpdateFailuresPromise { OWSAssertDebug(!NSThread.isMainThread); @@ -742,11 +726,23 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; OWSAssertDebug(message); OWSAssertDebug(thread); + BOOL disallowUuidlessRecipients = SSKFeatureFlags.useOnlyModernContactDiscovery; + NSMutableArray *validRecipients = [[NSMutableArray alloc] init]; + NSMutableArray *invalidRecipients = [[NSMutableArray alloc] init]; + + for (SignalRecipient *recipient in recipients) { + if (recipient.address.uuid == nil && disallowUuidlessRecipients) { + [invalidRecipients addObject:recipient]; + } else { + [validRecipients addObject:recipient]; + } + } + // 1. gather "ud sending access" using a single write transaction. NSMutableDictionary *sendingAccessMap = [NSMutableDictionary new]; - if (senderCertificate != nil) { + if (senderCertificate != nil && validRecipients.count > 0) { DatabaseStorageWrite(self.databaseStorage, ^(SDSAnyWriteTransaction *transaction) { - for (SignalRecipient *recipient in recipients) { + for (SignalRecipient *recipient in validRecipients) { if (!recipient.address.isLocalAddress) { sendingAccessMap[recipient.address] = [self.udManager udSendingAccessForAddress:recipient.address requireSyncAccess:YES @@ -759,7 +755,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; // 2. Build a "OWSMessageSend" for each recipient. NSMutableArray *messageSends = [NSMutableArray new]; - for (SignalRecipient *recipient in recipients) { + for (SignalRecipient *recipient in validRecipients) { OWSUDSendingAccess *_Nullable udSendingAccess = sendingAccessMap[recipient.address]; OWSMessageSend *messageSend = [[OWSMessageSend alloc] initWithMessage:message thread:thread @@ -777,17 +773,28 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; return [MessageSending ensureSessionsforMessageSendsObjc:messageSends ignoreErrors:YES].thenInBackground(^(id value) { // 4. Perform the per-recipient message sends. - NSMutableArray *sendPromises = [NSMutableArray array]; + NSMutableArray *allPromises = [NSMutableArray array]; for (OWSMessageSend *messageSend in messageSends) { [self sendMessageToRecipient:messageSend]; - [sendPromises addObject:messageSend.asAnyPromise]; + [allPromises addObject:messageSend.asAnyPromise]; + } + + // 5. Fail any message sends to invalid recipients + for (SignalRecipient *invalidRecipient in invalidRecipients) { + NSError *invalidRecipientError = OWSErrorMakeNoSuchSignalRecipientError(); + // Retryable because we can refetch from CDS + [invalidRecipientError setIsRetryable:YES]; + sendErrorBlock(invalidRecipient, invalidRecipientError); + + AnyPromise *failedPromise = [AnyPromise promiseWithValue:invalidRecipientError]; + [allPromises addObject:failedPromise]; } // We use PMKJoin(), not PMKWhen(), because we don't want the // completion promise to execute until _all_ send promises // have either succeeded or failed. PMKWhen() executes as // soon as any of its input promises fail. - return PMKJoin(sendPromises); + return PMKJoin(allPromises); }); } @@ -897,14 +904,15 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; return; } - NSArray *recipients = [self recipientsForAddresses:recipientAddresses]; - BOOL isGroupSend = thread.isGroupThread; NSMutableArray *sendErrors = [NSMutableArray array]; NSMutableDictionary *sendErrorPerRecipient = [NSMutableDictionary dictionary]; [self unlockPreKeyUpdateFailuresPromise] .thenInBackground(^(id value) { + return [self recipientPromiseForAddressesObjcWithAddressList:recipientAddresses]; + }) + .thenInBackground(^(NSArray *recipients) { return [self sendPromiseForRecipients:recipients message:message diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.swift b/SignalServiceKit/src/Messages/OWSMessageSender.swift index 89510ea21b..4b8a01e106 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.swift +++ b/SignalServiceKit/src/Messages/OWSMessageSender.swift @@ -474,3 +474,59 @@ fileprivate extension MessageSending { } } } + +// MARK: - SignalServiceAddress fetching + +extension MessageSender { + + @objc func recipientPromiseForAddressesObjc(addressList: [SignalServiceAddress]) -> AnyPromise { + return AnyPromise(recipientPromiseForAddresses(addressList: addressList)) + } + + func recipientPromiseForAddresses(addressList: [SignalServiceAddress]) -> Promise<[SignalRecipient]> { + return firstly { () -> Promise<[SignalServiceAddress]> in + Promise { resolver in + let invalidAddresses = addressList.filter { $0.uuid == nil } + + if invalidAddresses.count > 0 && FeatureFlags.useOnlyModernContactDiscovery { + // If we have any invalid addresses, block on a one-shot CDS fetch to get their UUIDs + // We'll fail the send later for any UUID-less addresses + let phoneNumbersToFetch = invalidAddresses.compactMap { $0.phoneNumber } + let operation = ContactDiscoveryOperation(phoneNumbersToLookup: phoneNumbersToFetch) + + operation.completionBlock = { + let discoveredContactMap = operation.registeredContacts.reduce(into: [:], { (builder, contact) in + builder[contact.e164PhoneNumber] = contact.signalUuid + }) + let rawNumberToUUIDMap: [String: UUID] = operation.phoneNumbersToLookup.reduce(into: [:], { (builder, rawNumber) in + if let e164 = PhoneNumber.tryParsePhoneNumber(fromUserSpecifiedText: rawNumber)?.toE164() { + builder[rawNumber] = discoveredContactMap[e164] + } + }) + + let updatedAddressList = addressList.map { (address: SignalServiceAddress) -> SignalServiceAddress in + guard address.uuid == nil, + let phoneNumber = address.phoneNumber, + let uuid = rawNumberToUUIDMap[phoneNumber] + else { return address } + + return SignalServiceAddress(uuid: uuid, phoneNumber: phoneNumber) + } + resolver.fulfill(updatedAddressList) + } + operation.perform() + + } else { + resolver.fulfill(addressList) + } + } + + }.map { (addresses: [SignalServiceAddress]) -> [SignalRecipient] in + return SDSDatabaseStorage.shared.read { (readTx) in + addresses.map { + SignalRecipient.getOrBuildUnsavedRecipient(for: $0, transaction: readTx) + } + } + } + } +} diff --git a/SignalServiceKit/src/SignalServiceKit.h b/SignalServiceKit/src/SignalServiceKit.h index a0886a63cc..feb58fefe7 100644 --- a/SignalServiceKit/src/SignalServiceKit.h +++ b/SignalServiceKit/src/SignalServiceKit.h @@ -28,6 +28,7 @@ #import #import #import +#import #import #import #import