From 4e995b76f3b3dfa4806a898504eb870274f67a9c Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 7 Jun 2017 14:27:24 -0400 Subject: [PATCH 1/3] Don't try to share file to untrusted identity There's a problem with this approach - specifically we need to dismiss the external file share view controller eventually. But more generally, because SN can present a view controller, it's kind success/retry handling is contextual to it's presenting view controller. So, probably the thing to do is duplicate this logic at it's various presentation contexts. // FREEBIE --- .../ConversationView/MessagesViewController.m | 60 +++++-------------- .../SendExternalFileViewController.m | 8 ++- Signal/src/util/ThreadUtil.h | 6 +- Signal/src/util/ThreadUtil.m | 47 ++++++++++++++- 4 files changed, 68 insertions(+), 53 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/MessagesViewController.m b/Signal/src/ViewControllers/ConversationView/MessagesViewController.m index ce78559e0..0b82f9a48 100644 --- a/Signal/src/ViewControllers/ConversationView/MessagesViewController.m +++ b/Signal/src/ViewControllers/ConversationView/MessagesViewController.m @@ -1167,8 +1167,7 @@ typedef enum : NSUInteger { * NO if there were no unconfirmed identities */ - (BOOL)showSafetyNumberConfirmationIfNecessaryWithConfirmationText:(NSString *)confirmationText - completion: - (void (^)(BOOL didConfirmedIdentity))completionHandler + completion:(void (^)(BOOL didConfirmIdentity))completionHandler { return [SafetyNumberConfirmationAlert presentAlertIfNecessaryWithRecipientIds:self.thread.recipientIdentifiers confirmationText:confirmationText @@ -1282,31 +1281,14 @@ typedef enum : NSUInteger { return; } - BOOL didShowSNAlert = - [self showSafetyNumberConfirmationIfNecessaryWithConfirmationText: - NSLocalizedString(@"SAFETY_NUMBER_CHANGED_CONFIRM_SEND_ACTION", - @"button title to confirm sending to a recipient whose " - @"safety number recently changed") - completion:^(BOOL didConfirmIdentity) { - if (didConfirmIdentity) { - [weakSelf didPressSendButton:button - withMessageText:text - senderId:senderId - senderDisplayName:senderDisplayName - date:date - updateKeyboardState:NO]; - } - }]; - if (didShowSNAlert) { - return; - } - text = [text stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceCharacterSet]]; if (text.length > 0) { if ([Environment.preferences soundInForeground]) { [JSQSystemSoundPlayer jsq_playMessageSentSound]; } + + TSOutgoingMessage *_Nullable outgoingMessage; // Limit outgoing text messages to 16kb. // // We convert large text messages to attachments @@ -1317,21 +1299,24 @@ typedef enum : NSUInteger { [SignalAttachment attachmentWithData:[text dataUsingEncoding:NSUTF8StringEncoding] dataUTI:SignalAttachment.kOversizeTextAttachmentUTI filename:nil]; - [self updateLastVisibleTimestamp:[ThreadUtil sendMessageWithAttachment:attachment - inThread:self.thread - messageSender:self.messageSender] - .timestampForSorting]; + outgoingMessage = + [ThreadUtil sendMessageWithAttachment:attachment inThread:self.thread messageSender:self.messageSender]; + if (outgoingMessage != nil) { + [self updateLastVisibleTimestamp:outgoingMessage.timestampForSorting]; + } } else { - [self updateLastVisibleTimestamp:[ThreadUtil sendMessageWithText:text - inThread:self.thread - messageSender:self.messageSender] - .timestampForSorting]; + outgoingMessage = + [ThreadUtil sendMessageWithText:text inThread:self.thread messageSender:self.messageSender]; + if (outgoingMessage != nil) { + [self updateLastVisibleTimestamp:outgoingMessage.timestampForSorting]; + } } self.lastMessageSentDate = [NSDate new]; [self clearUnreadMessagesIndicator]; - if (updateKeyboardState) { + // Don't pop keyboard if we popped the SN alert, else it obscures the SN alert. + if (outgoingMessage != nil && updateKeyboardState) { [self toggleDefaultKeyboard]; } [self clearDraft]; @@ -3703,21 +3688,6 @@ typedef enum : NSUInteger { return; } - BOOL didShowSNAlert = [self - showSafetyNumberConfirmationIfNecessaryWithConfirmationText: - NSLocalizedString(@"SAFETY_NUMBER_CHANGED_CONFIRM_SEND_ACTION", - @"button title to confirm sending to a recipient whose " - @"safety number recently changed") - completion:^(BOOL didConfirmIdentity) { - if (didConfirmIdentity) { - [weakSelf - tryToSendAttachmentIfApproved:attachment]; - } - }]; - if (didShowSNAlert) { - return; - } - if (attachment == nil || [attachment hasError]) { DDLogWarn(@"%@ %s Invalid attachment: %@.", self.tag, diff --git a/Signal/src/ViewControllers/SendExternalFileViewController.m b/Signal/src/ViewControllers/SendExternalFileViewController.m index d075d89cf..691dc9b05 100644 --- a/Signal/src/ViewControllers/SendExternalFileViewController.m +++ b/Signal/src/ViewControllers/SendExternalFileViewController.m @@ -47,9 +47,13 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(self.attachment); OWSAssert(thread); - [ThreadUtil sendMessageWithAttachment:self.attachment inThread:thread messageSender:self.messageSender]; + TSOutgoingMessage *outgoingMessage = + [ThreadUtil sendMessageWithAttachment:self.attachment inThread:thread messageSender:self.messageSender]; - [Environment messageThreadId:thread.uniqueId]; + // Don't pop to thread if we were blocked from creating an outgoing message by untrusted SN. + if (outgoingMessage != nil) { + [Environment messageThreadId:thread.uniqueId]; + } } - (BOOL)canSelectBlockedContact diff --git a/Signal/src/util/ThreadUtil.h b/Signal/src/util/ThreadUtil.h index 8d2e6b16f..c1d8b5126 100644 --- a/Signal/src/util/ThreadUtil.h +++ b/Signal/src/util/ThreadUtil.h @@ -50,9 +50,9 @@ NS_ASSUME_NONNULL_BEGIN inThread:(TSThread *)thread messageSender:(OWSMessageSender *)messageSender; -+ (TSOutgoingMessage *)sendMessageWithAttachment:(SignalAttachment *)attachment - inThread:(TSThread *)thread - messageSender:(OWSMessageSender *)messageSender; ++ (nullable TSOutgoingMessage *)sendMessageWithAttachment:(SignalAttachment *)attachment + inThread:(TSThread *)thread + messageSender:(OWSMessageSender *)messageSender; // This method will create and/or remove any offers and indicators // necessary for this thread. This includes: diff --git a/Signal/src/util/ThreadUtil.m b/Signal/src/util/ThreadUtil.m index 31d1fe507..f81071e4d 100644 --- a/Signal/src/util/ThreadUtil.m +++ b/Signal/src/util/ThreadUtil.m @@ -41,6 +41,26 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(thread); OWSAssert(messageSender); + OWSContactsManager *contactsManager = [Environment getCurrent].contactsManager; + OWSAssert(contactsManager); + + BOOL didShowSNAlert = [SafetyNumberConfirmationAlert + presentAlertIfNecessaryWithRecipientIds:thread.recipientIdentifiers + confirmationText:NSLocalizedString(@"SAFETY_NUMBER_CHANGED_CONFIRM_SEND_ACTION", + @"button title to confirm sending to a recipient whose safety " + @"number recently changed") + contactsManager:contactsManager + completion:^(BOOL didConfirmIdentity) { + if (didConfirmIdentity) { + [self sendMessageWithText:text + inThread:thread + messageSender:messageSender]; + } + }]; + if (didShowSNAlert) { + return nil; + } + OWSDisappearingMessagesConfiguration *configuration = [OWSDisappearingMessagesConfiguration fetchObjectWithUniqueID:thread.uniqueId]; TSOutgoingMessage *message = @@ -61,9 +81,9 @@ NS_ASSUME_NONNULL_BEGIN } -+ (TSOutgoingMessage *)sendMessageWithAttachment:(SignalAttachment *)attachment - inThread:(TSThread *)thread - messageSender:(OWSMessageSender *)messageSender ++ (nullable TSOutgoingMessage *)sendMessageWithAttachment:(SignalAttachment *)attachment + inThread:(TSThread *)thread + messageSender:(OWSMessageSender *)messageSender { OWSAssert([NSThread isMainThread]); OWSAssert(attachment); @@ -72,6 +92,27 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(thread); OWSAssert(messageSender); + OWSContactsManager *contactsManager = [Environment getCurrent].contactsManager; + OWSAssert(contactsManager); + + BOOL didShowSNAlert = [SafetyNumberConfirmationAlert + presentAlertIfNecessaryWithRecipientIds:thread.recipientIdentifiers + confirmationText:NSLocalizedString(@"SAFETY_NUMBER_CHANGED_CONFIRM_SEND_ACTION", + @"button title to confirm sending to a recipient whose safety " + @"number recently changed") + contactsManager:contactsManager + completion:^(BOOL didConfirmIdentity) { + if (didConfirmIdentity) { + [self sendMessageWithAttachment:attachment + inThread:thread + messageSender:messageSender]; + } + }]; + + if (didShowSNAlert) { + return nil; + } + OWSDisappearingMessagesConfiguration *configuration = [OWSDisappearingMessagesConfiguration fetchObjectWithUniqueID:thread.uniqueId]; TSOutgoingMessage *message = From f7468acad2300465a6c813acaf5e4b68cd5d24b3 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 7 Jun 2017 15:31:54 -0400 Subject: [PATCH 2/3] Properly dismiss file view after proceeding with share to no-longer verified identity // FREEBIE --- Signal/src/UserInterface/Strings.swift | 5 ++ .../ConversationView/MessagesViewController.m | 51 ++++++++++++++----- .../SendExternalFileViewController.m | 23 ++++++--- Signal/src/util/ThreadUtil.h | 6 +-- Signal/src/util/ThreadUtil.m | 47 ++--------------- 5 files changed, 65 insertions(+), 67 deletions(-) diff --git a/Signal/src/UserInterface/Strings.swift b/Signal/src/UserInterface/Strings.swift index b457b18ba..358a9314c 100644 --- a/Signal/src/UserInterface/Strings.swift +++ b/Signal/src/UserInterface/Strings.swift @@ -29,3 +29,8 @@ import Foundation static let missedCallWithIdentityChangeNotificationBodyWithoutCallerName = NSLocalizedString("MISSED_CALL_WITH_CHANGED_IDENTITY_BODY_WITHOUT_CALLER_NAME", comment: "notification title") static let missedCallWithIdentityChangeNotificationBodyWithCallerName = NSLocalizedString("MISSED_CALL_WITH_CHANGED_IDENTITY_BODY_WITH_CALLER_NAME", comment: "notification title. Embeds {{caller's name or phone number}}") } + +@objc class SafetyNumberStrings: NSObject { + static let confirmSendButton = NSLocalizedString("SAFETY_NUMBER_CHANGED_CONFIRM_SEND_ACTION", + comment: "button title to confirm sending to a recipient whose safety number recently changed") +} diff --git a/Signal/src/ViewControllers/ConversationView/MessagesViewController.m b/Signal/src/ViewControllers/ConversationView/MessagesViewController.m index 0b82f9a48..b744b7528 100644 --- a/Signal/src/ViewControllers/ConversationView/MessagesViewController.m +++ b/Signal/src/ViewControllers/ConversationView/MessagesViewController.m @@ -1281,14 +1281,28 @@ typedef enum : NSUInteger { return; } + BOOL didShowSNAlert = + [self showSafetyNumberConfirmationIfNecessaryWithConfirmationText:[SafetyNumberStrings confirmSendButton] + completion:^(BOOL didConfirmIdentity) { + if (didConfirmIdentity) { + [weakSelf didPressSendButton:button + withMessageText:text + senderId:senderId + senderDisplayName:senderDisplayName + date:date + updateKeyboardState:NO]; + } + }]; + if (didShowSNAlert) { + return; + } + text = [text stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceCharacterSet]]; if (text.length > 0) { if ([Environment.preferences soundInForeground]) { [JSQSystemSoundPlayer jsq_playMessageSentSound]; } - - TSOutgoingMessage *_Nullable outgoingMessage; // Limit outgoing text messages to 16kb. // // We convert large text messages to attachments @@ -1299,24 +1313,21 @@ typedef enum : NSUInteger { [SignalAttachment attachmentWithData:[text dataUsingEncoding:NSUTF8StringEncoding] dataUTI:SignalAttachment.kOversizeTextAttachmentUTI filename:nil]; - outgoingMessage = - [ThreadUtil sendMessageWithAttachment:attachment inThread:self.thread messageSender:self.messageSender]; - if (outgoingMessage != nil) { - [self updateLastVisibleTimestamp:outgoingMessage.timestampForSorting]; - } + [self updateLastVisibleTimestamp:[ThreadUtil sendMessageWithAttachment:attachment + inThread:self.thread + messageSender:self.messageSender] + .timestampForSorting]; } else { - outgoingMessage = - [ThreadUtil sendMessageWithText:text inThread:self.thread messageSender:self.messageSender]; - if (outgoingMessage != nil) { - [self updateLastVisibleTimestamp:outgoingMessage.timestampForSorting]; - } + [self updateLastVisibleTimestamp:[ThreadUtil sendMessageWithText:text + inThread:self.thread + messageSender:self.messageSender] + .timestampForSorting]; } self.lastMessageSentDate = [NSDate new]; [self clearUnreadMessagesIndicator]; - // Don't pop keyboard if we popped the SN alert, else it obscures the SN alert. - if (outgoingMessage != nil && updateKeyboardState) { + if (updateKeyboardState) { [self toggleDefaultKeyboard]; } [self clearDraft]; @@ -3688,6 +3699,18 @@ typedef enum : NSUInteger { return; } + BOOL didShowSNAlert = [self + showSafetyNumberConfirmationIfNecessaryWithConfirmationText:[SafetyNumberStrings confirmSendButton] + completion:^(BOOL didConfirmIdentity) { + if (didConfirmIdentity) { + [weakSelf + tryToSendAttachmentIfApproved:attachment]; + } + }]; + if (didShowSNAlert) { + return; + } + if (attachment == nil || [attachment hasError]) { DDLogWarn(@"%@ %s Invalid attachment: %@.", self.tag, diff --git a/Signal/src/ViewControllers/SendExternalFileViewController.m b/Signal/src/ViewControllers/SendExternalFileViewController.m index 691dc9b05..65e76fcb2 100644 --- a/Signal/src/ViewControllers/SendExternalFileViewController.m +++ b/Signal/src/ViewControllers/SendExternalFileViewController.m @@ -15,6 +15,7 @@ NS_ASSUME_NONNULL_BEGIN @interface SendExternalFileViewController () +@property (nonatomic, readonly) OWSContactsManager *contactsManager; @property (nonatomic, readonly) OWSMessageSender *messageSender; @end @@ -35,6 +36,7 @@ NS_ASSUME_NONNULL_BEGIN { [super loadView]; + _contactsManager = [Environment getCurrent].contactsManager; _messageSender = [Environment getCurrent].messageSender; self.title = NSLocalizedString(@"SEND_EXTERNAL_FILE_VIEW_TITLE", @"Title for the 'send external file' view."); @@ -47,13 +49,22 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(self.attachment); OWSAssert(thread); - TSOutgoingMessage *outgoingMessage = - [ThreadUtil sendMessageWithAttachment:self.attachment inThread:thread messageSender:self.messageSender]; - - // Don't pop to thread if we were blocked from creating an outgoing message by untrusted SN. - if (outgoingMessage != nil) { - [Environment messageThreadId:thread.uniqueId]; + BOOL didShowSNAlert = + [SafetyNumberConfirmationAlert presentAlertIfNecessaryWithRecipientIds:thread.recipientIdentifiers + confirmationText:[SafetyNumberStrings confirmSendButton] + contactsManager:self.contactsManager + completion:^(BOOL didConfirm) { + if (didConfirm) { + [self threadWasSelected:thread]; + } + }]; + if (didShowSNAlert) { + return; } + + [ThreadUtil sendMessageWithAttachment:self.attachment inThread:thread messageSender:self.messageSender]; + + [Environment messageThreadId:thread.uniqueId]; } - (BOOL)canSelectBlockedContact diff --git a/Signal/src/util/ThreadUtil.h b/Signal/src/util/ThreadUtil.h index c1d8b5126..8d2e6b16f 100644 --- a/Signal/src/util/ThreadUtil.h +++ b/Signal/src/util/ThreadUtil.h @@ -50,9 +50,9 @@ NS_ASSUME_NONNULL_BEGIN inThread:(TSThread *)thread messageSender:(OWSMessageSender *)messageSender; -+ (nullable TSOutgoingMessage *)sendMessageWithAttachment:(SignalAttachment *)attachment - inThread:(TSThread *)thread - messageSender:(OWSMessageSender *)messageSender; ++ (TSOutgoingMessage *)sendMessageWithAttachment:(SignalAttachment *)attachment + inThread:(TSThread *)thread + messageSender:(OWSMessageSender *)messageSender; // This method will create and/or remove any offers and indicators // necessary for this thread. This includes: diff --git a/Signal/src/util/ThreadUtil.m b/Signal/src/util/ThreadUtil.m index f81071e4d..31d1fe507 100644 --- a/Signal/src/util/ThreadUtil.m +++ b/Signal/src/util/ThreadUtil.m @@ -41,26 +41,6 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(thread); OWSAssert(messageSender); - OWSContactsManager *contactsManager = [Environment getCurrent].contactsManager; - OWSAssert(contactsManager); - - BOOL didShowSNAlert = [SafetyNumberConfirmationAlert - presentAlertIfNecessaryWithRecipientIds:thread.recipientIdentifiers - confirmationText:NSLocalizedString(@"SAFETY_NUMBER_CHANGED_CONFIRM_SEND_ACTION", - @"button title to confirm sending to a recipient whose safety " - @"number recently changed") - contactsManager:contactsManager - completion:^(BOOL didConfirmIdentity) { - if (didConfirmIdentity) { - [self sendMessageWithText:text - inThread:thread - messageSender:messageSender]; - } - }]; - if (didShowSNAlert) { - return nil; - } - OWSDisappearingMessagesConfiguration *configuration = [OWSDisappearingMessagesConfiguration fetchObjectWithUniqueID:thread.uniqueId]; TSOutgoingMessage *message = @@ -81,9 +61,9 @@ NS_ASSUME_NONNULL_BEGIN } -+ (nullable TSOutgoingMessage *)sendMessageWithAttachment:(SignalAttachment *)attachment - inThread:(TSThread *)thread - messageSender:(OWSMessageSender *)messageSender ++ (TSOutgoingMessage *)sendMessageWithAttachment:(SignalAttachment *)attachment + inThread:(TSThread *)thread + messageSender:(OWSMessageSender *)messageSender { OWSAssert([NSThread isMainThread]); OWSAssert(attachment); @@ -92,27 +72,6 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(thread); OWSAssert(messageSender); - OWSContactsManager *contactsManager = [Environment getCurrent].contactsManager; - OWSAssert(contactsManager); - - BOOL didShowSNAlert = [SafetyNumberConfirmationAlert - presentAlertIfNecessaryWithRecipientIds:thread.recipientIdentifiers - confirmationText:NSLocalizedString(@"SAFETY_NUMBER_CHANGED_CONFIRM_SEND_ACTION", - @"button title to confirm sending to a recipient whose safety " - @"number recently changed") - contactsManager:contactsManager - completion:^(BOOL didConfirmIdentity) { - if (didConfirmIdentity) { - [self sendMessageWithAttachment:attachment - inThread:thread - messageSender:messageSender]; - } - }]; - - if (didShowSNAlert) { - return nil; - } - OWSDisappearingMessagesConfiguration *configuration = [OWSDisappearingMessagesConfiguration fetchObjectWithUniqueID:thread.uniqueId]; TSOutgoingMessage *message = From 842baa30722a6efc4671805aff67d0edcf808fa9 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 7 Jun 2017 22:33:29 -0400 Subject: [PATCH 3/3] CR: use weak self // FREEBIE --- Signal/src/ViewControllers/SendExternalFileViewController.m | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Signal/src/ViewControllers/SendExternalFileViewController.m b/Signal/src/ViewControllers/SendExternalFileViewController.m index 65e76fcb2..a1bf00c47 100644 --- a/Signal/src/ViewControllers/SendExternalFileViewController.m +++ b/Signal/src/ViewControllers/SendExternalFileViewController.m @@ -49,13 +49,15 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(self.attachment); OWSAssert(thread); + __weak typeof(self) weakSelf = self; + BOOL didShowSNAlert = [SafetyNumberConfirmationAlert presentAlertIfNecessaryWithRecipientIds:thread.recipientIdentifiers confirmationText:[SafetyNumberStrings confirmSendButton] contactsManager:self.contactsManager completion:^(BOOL didConfirm) { if (didConfirm) { - [self threadWasSelected:thread]; + [weakSelf threadWasSelected:thread]; } }]; if (didShowSNAlert) {