From 2541be1619ef7045a5fcf7ba1cb04179d299defc Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 30 Oct 2018 12:06:20 -0400 Subject: [PATCH 1/8] Apply refinements to UD logic. --- .../AppSettings/OWSLinkDeviceViewController.m | 3 - SignalMessaging/profiles/OWSProfileManager.m | 9 +++ .../profiles/ProfileFetcherJob.swift | 51 ++------------- .../src/Contacts/SignalRecipient.m | 6 +- SignalServiceKit/src/Devices/OWSDevice.m | 7 +- .../src/Messages/OWSMessageManager.m | 17 +++-- .../src/Messages/OWSMessageSend.swift | 10 +-- .../src/Messages/OWSMessageSender.m | 32 +++++----- .../src/Messages/UD/OWSRequestMaker.swift | 64 ++++++++++++++----- .../src/Messages/UD/OWSUDManager.swift | 50 +++++++++++++-- .../tests/Messages/OWSUDManagerTest.swift | 30 ++++----- 11 files changed, 155 insertions(+), 124 deletions(-) diff --git a/Signal/src/ViewControllers/AppSettings/OWSLinkDeviceViewController.m b/Signal/src/ViewControllers/AppSettings/OWSLinkDeviceViewController.m index 79c01657b..273bbff63 100644 --- a/Signal/src/ViewControllers/AppSettings/OWSLinkDeviceViewController.m +++ b/Signal/src/ViewControllers/AppSettings/OWSLinkDeviceViewController.m @@ -193,9 +193,6 @@ NS_ASSUME_NONNULL_BEGIN // so all sync message sends will fail on the socket until it is cycled. [TSSocketManager.shared cycleSocket]; - [self.udManager setUnidentifiedAccessMode:UnidentifiedAccessModeUnknown - recipientId:self.tsAccountManager.localNumber]; - // Fetch the local profile to determine if all // linked devices support UD. [self.profileManager fetchLocalUsersProfile]; diff --git a/SignalMessaging/profiles/OWSProfileManager.m b/SignalMessaging/profiles/OWSProfileManager.m index 988b9db07..c31f64414 100644 --- a/SignalMessaging/profiles/OWSProfileManager.m +++ b/SignalMessaging/profiles/OWSProfileManager.m @@ -155,6 +155,13 @@ typedef void (^ProfileManagerFailureBlock)(NSError *error); return SSKEnvironment.shared.syncManager; } +- (id)udManager +{ + OWSAssertDebug(SSKEnvironment.shared.udManager); + + return SSKEnvironment.shared.udManager; +} + #pragma mark - User Profile Accessor - (void)ensureLocalProfileCached @@ -1007,6 +1014,8 @@ typedef void (^ProfileManagerFailureBlock)(NSError *error); dbConnection:self.dbConnection completion:^{ dispatch_async(dispatch_get_main_queue(), ^(void) { + [self.udManager setUnidentifiedAccessMode:UnidentifiedAccessModeUnknown + recipientId:recipientId]; [self fetchProfileForRecipientId:recipientId]; }); }]; diff --git a/SignalMessaging/profiles/ProfileFetcherJob.swift b/SignalMessaging/profiles/ProfileFetcherJob.swift index d740e59cf..ecad059ac 100644 --- a/SignalMessaging/profiles/ProfileFetcherJob.swift +++ b/SignalMessaging/profiles/ProfileFetcherJob.swift @@ -135,53 +135,14 @@ public class ProfileFetcherJob: NSObject { Logger.error("getProfile: \(recipientId)") - switch self.udManager.unidentifiedAccessMode(forRecipientId: recipientId) { - case .unknown: - if let udAccessKey = udManager.udAccessKey(forRecipientId: recipientId) { - // If we are in unknown mode and have a profile key, - // try using the profile key. - return self.requestProfile(recipientId: recipientId, - udAccessKey: udAccessKey, - canFailoverUDAuth: true) - } else { - // If we are in unknown mode and don't have a profile key, - // try using a random UD access key in case they support - // unrestricted access. - let randomUDAccessKey = self.udManager.randomUDAccessKey() - return requestProfile(recipientId: recipientId, - udAccessKey: randomUDAccessKey, - canFailoverUDAuth: true) - } - case .unrestricted: - let randomUDAccessKey = self.udManager.randomUDAccessKey() - return requestProfile(recipientId: recipientId, - udAccessKey: randomUDAccessKey, - canFailoverUDAuth: false) - .recover { (_: Error) -> Promise in - Logger.verbose("Failing over to non-random access.") - let udAccessKey = self.udManager.udAccessKey(forRecipientId: recipientId) - // This may fail over again to non-UD-auth. - return self.requestProfile(recipientId: recipientId, - udAccessKey: udAccessKey, - canFailoverUDAuth: true) - } - case .disabled: - // This may fail over to non-UD-auth. - return requestProfile(recipientId: recipientId, - udAccessKey: nil, - canFailoverUDAuth: true) - case .enabled: - // This may be nil if we don't have a profile key for them. - let udAccessKey = udManager.udAccessKey(forRecipientId: recipientId) - // This may fail over to non-UD-auth. - return requestProfile(recipientId: recipientId, - udAccessKey: udAccessKey, - canFailoverUDAuth: true) - } + let udAccess = udManager.udAccess(forRecipientId: recipientId) + return requestProfile(recipientId: recipientId, + udAccess: udAccess, + canFailoverUDAuth: true) } private func requestProfile(recipientId: String, - udAccessKey: SMKUDAccessKey?, + udAccess: OWSUDAccess?, canFailoverUDAuth: Bool) -> Promise { AssertIsOnMainThread() @@ -193,7 +154,7 @@ public class ProfileFetcherJob: NSObject { }, websocketFailureBlock: { // Do nothing }, recipientId: recipientId, - udAccessKey: udAccessKey, + udAccess: udAccess, canFailoverUDAuth: canFailoverUDAuth) return requestMaker.makeRequest() .map { (result: RequestMakerResult) -> SignalServiceProfile in diff --git a/SignalServiceKit/src/Contacts/SignalRecipient.m b/SignalServiceKit/src/Contacts/SignalRecipient.m index 170a1c7ef..3ce3bebb3 100644 --- a/SignalServiceKit/src/Contacts/SignalRecipient.m +++ b/SignalServiceKit/src/Contacts/SignalRecipient.m @@ -148,11 +148,7 @@ NS_ASSUME_NONNULL_BEGIN // Device changes dispatch_async(dispatch_get_main_queue(), ^{ // Device changes can affect the UD access mode for a recipient, - // so we need to: - // - // * Mark the UD access mode as "unknown". - // * Fetch the profile for this user to update UD access mode. - [self.udManager setUnidentifiedAccessMode:UnidentifiedAccessModeUnknown recipientId:self.recipientId]; + // so we need to fetch the profile for this user to update UD access mode. [self.profileManager fetchProfileForRecipientId:self.recipientId]; }); } diff --git a/SignalServiceKit/src/Devices/OWSDevice.m b/SignalServiceKit/src/Devices/OWSDevice.m index 8fec95a7f..4800f4da5 100644 --- a/SignalServiceKit/src/Devices/OWSDevice.m +++ b/SignalServiceKit/src/Devices/OWSDevice.m @@ -217,12 +217,7 @@ NSString *const kOWSPrimaryStorage_MayHaveLinkedDevices = @"kTSStorageManager_Ma if (didAddOrRemove) { dispatch_async(dispatch_get_main_queue(), ^{ // Device changes can affect the UD access mode for a recipient, - // so we need to: - // - // * Mark the UD access mode as "unknown". - // * Fetch the profile for this user to update UD access mode. - [self.udManager setUnidentifiedAccessMode:UnidentifiedAccessModeUnknown - recipientId:self.tsAccountManager.localNumber]; + // so we need to fetch the profile for this user to update UD access mode. [self.profileManager fetchLocalUsersProfile]; }); return YES; diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index 06a2a5213..8f03c364d 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -156,6 +156,11 @@ NS_ASSUME_NONNULL_BEGIN return SSKEnvironment.shared.tsAccountManager; } +- (id)profileManager +{ + return SSKEnvironment.shared.profileManager; +} + #pragma mark - - (void)startObserving @@ -564,11 +569,6 @@ NS_ASSUME_NONNULL_BEGIN }]; } -- (id)profileManager -{ - return SSKEnvironment.shared.profileManager; -} - - (void)handleIncomingEnvelope:(SSKProtoEnvelope *)envelope withReceiptMessage:(SSKProtoReceiptMessage *)receiptMessage transaction:(YapDatabaseReadWriteTransaction *)transaction @@ -1453,12 +1453,13 @@ NS_ASSUME_NONNULL_BEGIN return; } + BOOL isRecipientDevice = YES; SignalRecipient *_Nullable recipient = [SignalRecipient registeredRecipientForRecipientId:localNumber transaction:transaction]; if (!recipient) { OWSFailDebug(@"No local SignalRecipient."); } else { - BOOL isRecipientDevice = [recipient.devices containsObject:@(envelope.sourceDevice)]; + isRecipientDevice = [recipient.devices containsObject:@(envelope.sourceDevice)]; if (!isRecipientDevice) { OWSLogInfo(@"Message received from unknown linked device; adding to local SignalRecipient: %lu.", (unsigned long) envelope.sourceDevice); @@ -1480,6 +1481,10 @@ NS_ASSUME_NONNULL_BEGIN [OWSDevicesService refreshDevices]; } + + if (!isRecipientDevice || !isInDeviceList) { + [self.profileManager fetchLocalUsersProfile]; + } } @end diff --git a/SignalServiceKit/src/Messages/OWSMessageSend.swift b/SignalServiceKit/src/Messages/OWSMessageSend.swift index 29b7f8b2c..366e5b99c 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSend.swift +++ b/SignalServiceKit/src/Messages/OWSMessageSend.swift @@ -32,7 +32,7 @@ public class OWSMessageSend: NSObject { public var hasWebsocketSendFailed = false @objc - public var udAccessKey: SMKUDAccessKey? + public var udAccess: OWSUDAccess? @objc public var senderCertificate: SMKSenderCertificate? @@ -54,7 +54,7 @@ public class OWSMessageSend: NSObject { thread: TSThread?, recipient: SignalRecipient, senderCertificate: SMKSenderCertificate?, - udAccessKey: SMKUDAccessKey?, + udAccess: OWSUDAccess?, localNumber: String, success: @escaping () -> Void, failure: @escaping (Error) -> Void) { @@ -63,7 +63,7 @@ public class OWSMessageSend: NSObject { self.recipient = recipient self.localNumber = localNumber self.senderCertificate = senderCertificate - self.udAccessKey = udAccessKey + self.udAccess = udAccess if let recipientId = recipient.uniqueId { self.isLocalNumber = localNumber == recipientId @@ -78,13 +78,13 @@ public class OWSMessageSend: NSObject { @objc public var isUDSend: Bool { - return udAccessKey != nil && senderCertificate != nil + return udAccess != nil && senderCertificate != nil } @objc public func disableUD() { Logger.verbose("\(recipient.recipientId)") - udAccessKey = nil + udAccess = nil } @objc diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 81b6953f3..782b17b43 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -579,7 +579,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; message:(TSOutgoingMessage *)message thread:(nullable TSThread *)thread senderCertificate:(nullable SMKSenderCertificate *)senderCertificate - selfUDAccessKey:(nullable SMKUDAccessKey *)selfUDAccessKey + selfUDAccess:(nullable OWSUDAccess *)selfUDAccess sendErrors:(NSMutableArray *)sendErrors { OWSAssertDebug(recipients.count > 0); @@ -591,16 +591,16 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; for (SignalRecipient *recipient in recipients) { // Use chained promises to make the code more readable. AnyPromise *sendPromise = [AnyPromise promiseWithResolverBlock:^(PMKResolver resolve) { - SMKUDAccessKey *_Nullable theirUDAccessKey; - if (senderCertificate != nil && selfUDAccessKey != nil) { - theirUDAccessKey = [self.udManager udSendAccessKeyForRecipientId:recipient.recipientId]; + OWSUDAccess *_Nullable theirUDAccess; + if (senderCertificate != nil && selfUDAccess != nil) { + theirUDAccess = [self.udManager udAccessForRecipientId:recipient.recipientId]; } OWSMessageSend *messageSend = [[OWSMessageSend alloc] initWithMessage:message thread:thread recipient:recipient senderCertificate:senderCertificate - udAccessKey:theirUDAccessKey + udAccess:theirUDAccess localNumber:self.tsAccountManager.localNumber success:^{ // The value doesn't matter, we just need any non-NSError value. @@ -631,16 +631,16 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; { AssertIsOnSendingQueue(); - SMKUDAccessKey *_Nullable selfUDAccessKey; + OWSUDAccess *_Nullable selfUDAccess; if (senderCertificate) { - selfUDAccessKey = [self.udManager udSendAccessKeyForRecipientId:self.tsAccountManager.localNumber]; + selfUDAccess = [self.udManager udAccessForRecipientId:self.tsAccountManager.localNumber]; } void (^successHandler)(void) = ^() { dispatch_async([OWSDispatch sendingQueue], ^{ [self handleMessageSentLocally:message senderCertificate:senderCertificate - selfUDAccessKey:selfUDAccessKey + selfUDAccess:selfUDAccess success:^{ successHandlerParam(); } @@ -658,7 +658,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; dispatch_async([OWSDispatch sendingQueue], ^{ [self handleMessageSentLocally:message senderCertificate:senderCertificate - selfUDAccessKey:selfUDAccessKey + selfUDAccess:selfUDAccess success:^{ failureHandlerParam(error); } @@ -740,7 +740,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; message:message thread:thread senderCertificate:senderCertificate - selfUDAccessKey:selfUDAccessKey + selfUDAccess:selfUDAccess sendErrors:sendErrors] .then(^(id value) { successHandler(); @@ -1092,7 +1092,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; messageSend.hasWebsocketSendFailed = YES; } recipientId:recipient.recipientId - udAccessKey:messageSend.udAccessKey + udAccess:messageSend.udAccess canFailoverUDAuth:NO]; [[requestMaker makeRequestObjc] .then(^(OWSRequestMakerResult *result) { @@ -1341,7 +1341,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)handleMessageSentLocally:(TSOutgoingMessage *)message senderCertificate:(nullable SMKSenderCertificate *)senderCertificate - selfUDAccessKey:(nullable SMKUDAccessKey *)selfUDAccessKey + selfUDAccess:(nullable OWSUDAccess *)selfUDAccess success:(void (^)(void))success failure:(RetryableFailureHandler)failure { @@ -1358,7 +1358,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; [self sendSyncTranscriptForMessage:message senderCertificate:senderCertificate - selfUDAccessKey:selfUDAccessKey + selfUDAccess:selfUDAccess success:^{ // TODO: We might send to a recipient, then to another recipient on retry. // To ensure desktop receives all "delivery status" info, we might @@ -1375,7 +1375,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)sendSyncTranscriptForMessage:(TSOutgoingMessage *)message senderCertificate:(nullable SMKSenderCertificate *)senderCertificate - selfUDAccessKey:(nullable SMKUDAccessKey *)selfUDAccessKey + selfUDAccess:(nullable OWSUDAccess *)selfUDAccess success:(void (^)(void))success failure:(RetryableFailureHandler)failure { @@ -1392,7 +1392,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; thread:message.thread recipient:recipient senderCertificate:senderCertificate - udAccessKey:selfUDAccessKey + udAccess:selfUDAccess localNumber:self.tsAccountManager.localNumber success:^{ OWSLogInfo(@"Successfully sent sync transcript."); @@ -1584,7 +1584,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; messageSend.hasWebsocketSendFailed = YES; } recipientId:recipientId - udAccessKey:messageSend.udAccessKey + udAccess:messageSend.udAccess canFailoverUDAuth:YES]; [[requestMaker makeRequestObjc] .then(^(OWSRequestMakerResult *result) { diff --git a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift index ad0478e2e..d3193bcec 100644 --- a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift +++ b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift @@ -47,7 +47,7 @@ public class RequestMaker: NSObject { private let udAuthFailureBlock: UDAuthFailureBlock private let websocketFailureBlock: WebsocketFailureBlock private let recipientId: String - private let udAccessKey: SMKUDAccessKey? + private let udAccess: OWSUDAccess? private let canFailoverUDAuth: Bool @objc @@ -56,14 +56,14 @@ public class RequestMaker: NSObject { udAuthFailureBlock : @escaping UDAuthFailureBlock, websocketFailureBlock : @escaping WebsocketFailureBlock, recipientId: String, - udAccessKey: SMKUDAccessKey?, + udAccess: OWSUDAccess?, canFailoverUDAuth: Bool) { self.label = label self.requestFactoryBlock = requestFactoryBlock self.udAuthFailureBlock = udAuthFailureBlock self.websocketFailureBlock = websocketFailureBlock self.recipientId = recipientId - self.udAccessKey = udAccessKey + self.udAccess = udAccess self.canFailoverUDAuth = canFailoverUDAuth } @@ -81,6 +81,10 @@ public class RequestMaker: NSObject { return SSKEnvironment.shared.udManager } + private var profileManager: ProfileManagerProtocol { + return SSKEnvironment.shared.profileManager + } + // MARK: - @objc @@ -104,37 +108,41 @@ public class RequestMaker: NSObject { } private func makeRequestInternal(skipUD: Bool, skipWebsocket: Bool) -> Promise { - var udAccessKeyForRequest: SMKUDAccessKey? + var udAccessForRequest: OWSUDAccess? if !skipUD { - udAccessKeyForRequest = udAccessKey + udAccessForRequest = udAccess } - let isUDSend = udAccessKeyForRequest != nil - let request = requestFactoryBlock(udAccessKeyForRequest) - let webSocketType: OWSWebSocketType = (isUDSend ? .UD : .default) + let isUDRequest = udAccessForRequest != nil + let request = requestFactoryBlock(udAccessForRequest?.udAccessKey) + let webSocketType: OWSWebSocketType = (isUDRequest ? .UD : .default) let canMakeWebsocketRequests = (socketManager.canMakeRequests(of: webSocketType) && !skipWebsocket) if canMakeWebsocketRequests { return Promise { resolver in socketManager.make(request, webSocketType: webSocketType, success: { (responseObject: Any?) in if self.udManager.isUDVerboseLoggingEnabled() { - if isUDSend { + if isUDRequest { Logger.debug("UD websocket request '\(self.label)' succeeded.") } else { Logger.debug("Non-UD websocket request '\(self.label)' succeeded.") } } - resolver.fulfill(RequestMakerResult(responseObject: responseObject, wasSentByUD: isUDSend)) + self.requestSucceeded(udAccess: udAccessForRequest) + + resolver.fulfill(RequestMakerResult(responseObject: responseObject, wasSentByUD: isUDRequest)) }) { (statusCode: Int, responseData: Data?, error: Error) in resolver.reject(RequestMakerError.websocketRequestError(statusCode: statusCode, responseData: responseData, underlyingError: error)) } }.recover { (error: Error) -> Promise in switch error { case RequestMakerError.websocketRequestError(let statusCode, _, _): - if isUDSend && (statusCode == 401 || statusCode == 403) { - // If a UD send fails due to service response (as opposed to network + if isUDRequest && (statusCode == 401 || statusCode == 403) { + // If a UD request fails due to service response (as opposed to network // failure), mark recipient as _not_ in UD mode, then retry. self.udManager.setUnidentifiedAccessMode(.disabled, recipientId: self.recipientId) + self.profileManager.fetchProfile(forRecipientId: self.recipientId) + self.udAuthFailureBlock() if self.canFailoverUDAuth { Logger.info("UD websocket request '\(self.label)' auth failed; failing over to non-UD websocket request.") @@ -157,23 +165,27 @@ public class RequestMaker: NSObject { return self.networkManager.makePromise(request: request) .map { (networkManagerResult: TSNetworkManager.NetworkManagerResult) -> RequestMakerResult in if self.udManager.isUDVerboseLoggingEnabled() { - if isUDSend { + if isUDRequest { Logger.debug("UD REST request '\(self.label)' succeeded.") } else { Logger.debug("Non-UD REST request '\(self.label)' succeeded.") } } + self.requestSucceeded(udAccess: udAccessForRequest) + // Unwrap the network manager promise into a request maker promise. - return RequestMakerResult(responseObject: networkManagerResult.responseObject, wasSentByUD: isUDSend) + return RequestMakerResult(responseObject: networkManagerResult.responseObject, wasSentByUD: isUDRequest) }.recover { (error: Error) -> Promise in switch error { case NetworkManagerError.taskError(let task, _): let statusCode = task.statusCode() - if isUDSend && (statusCode == 401 || statusCode == 403) { - // If a UD send fails due to service response (as opposed to network + if isUDRequest && (statusCode == 401 || statusCode == 403) { + // If a UD request fails due to service response (as opposed to network // failure), mark recipient as _not_ in UD mode, then retry. self.udManager.setUnidentifiedAccessMode(.disabled, recipientId: self.recipientId) + self.profileManager.fetchProfile(forRecipientId: self.recipientId) + self.udAuthFailureBlock() if self.canFailoverUDAuth { Logger.info("UD REST request '\(self.label)' auth failed; failing over to non-UD REST request.") @@ -193,4 +205,24 @@ public class RequestMaker: NSObject { } } } + + private func requestSucceeded(udAccess: OWSUDAccess?) { + guard let udAccess = udAccess else { + return + } + guard udAccess.udAccessMode == .unknown else { + return + } + + if udAccess.isRandomKey { + // If a UD request succeeds for an unknown user with a random key, + // mark recipient as .unrestricted. + self.udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: self.recipientId) + } else { + // If a UD request succeeds for an unknown user with a non-random key, + // mark recipient as .enabled. + self.udManager.setUnidentifiedAccessMode(.enabled, recipientId: self.recipientId) + } + self.profileManager.fetchProfile(forRecipientId: self.recipientId) + } } diff --git a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift index ce6686121..95219d9f9 100644 --- a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift +++ b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift @@ -33,6 +33,27 @@ private func string(forUnidentifiedAccessMode mode: UnidentifiedAccessMode) -> S } } +@objc +public class OWSUDAccess: NSObject { + @objc + public let udAccessKey: SMKUDAccessKey + + @objc + public let udAccessMode: UnidentifiedAccessMode + + @objc + public let isRandomKey: Bool + + @objc + public required init(udAccessKey: SMKUDAccessKey, + udAccessMode: UnidentifiedAccessMode, + isRandomKey: Bool) { + self.udAccessKey = udAccessKey + self.udAccessMode = udAccessMode + self.isRandomKey = isRandomKey + } +} + @objc public protocol OWSUDManager: class { @objc func setup() @@ -58,7 +79,7 @@ private func string(forUnidentifiedAccessMode mode: UnidentifiedAccessMode) -> S func udAccessKey(forRecipientId recipientId: RecipientIdentifier) -> SMKUDAccessKey? @objc - func udSendAccessKey(forRecipientId recipientId: RecipientIdentifier) -> SMKUDAccessKey? + func udAccess(forRecipientId recipientId: RecipientIdentifier) -> OWSUDAccess? // MARK: Sender Certificate @@ -220,7 +241,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { // Returns the UD access key for sending to a given recipient. @objc - public func udSendAccessKey(forRecipientId recipientId: RecipientIdentifier) -> SMKUDAccessKey? { + public func udAccess(forRecipientId recipientId: RecipientIdentifier) -> OWSUDAccess? { // This check is currently redundant with the "send access key for local number" // check below, but behavior of isUDEnabled() may change. guard isUDEnabled() else { @@ -236,7 +257,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { return nil } if localNumber != recipientId { - guard udSendAccessKey(forRecipientId: localNumber) != nil else { + guard udAccess(forRecipientId: localNumber) != nil else { if isUDVerboseLoggingEnabled() { Logger.info("UD Send disabled for \(recipientId), UD disabled for sync messages.") } @@ -245,11 +266,20 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { } let accessMode = unidentifiedAccessMode(forRecipientId: recipientId) if accessMode == .unrestricted { - if isUDVerboseLoggingEnabled() { - Logger.info("UD Send enabled for \(recipientId) with random key.") + if let udAccessKey = udAccessKey(forRecipientId: recipientId) { + if isUDVerboseLoggingEnabled() { + Logger.info("UD Send enabled for \(recipientId) with unverified key.") + } + return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: false) + } else { + if isUDVerboseLoggingEnabled() { + Logger.info("UD Send enabled for \(recipientId) with random key.") + } + let udAccessKey = randomUDAccessKey() + return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: true) } - return randomUDAccessKey() } + guard accessMode == .enabled else { if isUDVerboseLoggingEnabled() { Logger.info("UD Send disabled for \(recipientId), UD not enabled for this recipient.") @@ -259,7 +289,13 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { if isUDVerboseLoggingEnabled() { Logger.info("UD Send enabled for \(recipientId).") } - return udAccessKey(forRecipientId: recipientId) + guard let udAccessKey = udAccessKey(forRecipientId: recipientId) else { + if isUDVerboseLoggingEnabled() { + Logger.info("UD Send disabled for \(recipientId), no profile key for this recipient.") + } + return nil + } + return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: false) } // MARK: - Sender Certificate diff --git a/SignalServiceKit/tests/Messages/OWSUDManagerTest.swift b/SignalServiceKit/tests/Messages/OWSUDManagerTest.swift index c6cb8fc11..1322fe113 100644 --- a/SignalServiceKit/tests/Messages/OWSUDManagerTest.swift +++ b/SignalServiceKit/tests/Messages/OWSUDManagerTest.swift @@ -78,23 +78,23 @@ class OWSUDManagerTest: SSKBaseTestSwift { let aliceRecipientId = "+13213214321" XCTAssert(UnidentifiedAccessMode.unknown == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: aliceRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) udManager.setUnidentifiedAccessMode(.unknown, recipientId: aliceRecipientId) XCTAssert(UnidentifiedAccessMode.unknown == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: aliceRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) udManager.setUnidentifiedAccessMode(.disabled, recipientId: aliceRecipientId) XCTAssert(UnidentifiedAccessMode.disabled == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: aliceRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) udManager.setUnidentifiedAccessMode(.enabled, recipientId: aliceRecipientId) XCTAssert(UnidentifiedAccessMode.enabled == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNotNil(udManager.udSendAccessKey(forRecipientId: aliceRecipientId)) + XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: aliceRecipientId) XCTAssert(UnidentifiedAccessMode.unrestricted == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNotNil(udManager.udSendAccessKey(forRecipientId: aliceRecipientId)) + XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) } func testMode_noProfileKey() { @@ -111,24 +111,24 @@ class OWSUDManagerTest: SSKBaseTestSwift { XCTAssertNotEqual(bobRecipientId, tsAccountManager.localNumber()!) XCTAssertEqual(UnidentifiedAccessMode.unknown, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) udManager.setUnidentifiedAccessMode(.unknown, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unknown, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) udManager.setUnidentifiedAccessMode(.disabled, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.disabled, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) udManager.setUnidentifiedAccessMode(.enabled, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.enabled, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) // Bob should work in unrestricted mode, even if he doesn't have a profile key. udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unrestricted, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNotNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) } func testMode_withProfileKey() { @@ -145,22 +145,22 @@ class OWSUDManagerTest: SSKBaseTestSwift { profileManager.setProfileKeyData(OWSAES256Key.generateRandom().keyData, forRecipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unknown, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) udManager.setUnidentifiedAccessMode(.unknown, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unknown, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) udManager.setUnidentifiedAccessMode(.disabled, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.disabled, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) udManager.setUnidentifiedAccessMode(.enabled, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.enabled, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNotNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unrestricted, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNotNil(udManager.udSendAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) } } From c5f4711595076b6b869ed93317f9052ea94c60ce Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 30 Oct 2018 13:25:01 -0400 Subject: [PATCH 2/8] Apply refinements to UD logic. --- .../profiles/ProfileFetcherJob.swift | 3 +- .../src/Messages/OWSMessageSender.m | 4 +- .../src/Messages/UD/OWSUDManager.swift | 56 +++++++++++-------- 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/SignalMessaging/profiles/ProfileFetcherJob.swift b/SignalMessaging/profiles/ProfileFetcherJob.swift index ecad059ac..c28236745 100644 --- a/SignalMessaging/profiles/ProfileFetcherJob.swift +++ b/SignalMessaging/profiles/ProfileFetcherJob.swift @@ -135,7 +135,8 @@ public class ProfileFetcherJob: NSObject { Logger.error("getProfile: \(recipientId)") - let udAccess = udManager.udAccess(forRecipientId: recipientId) + let udAccess = udManager.udAccess(forRecipientId: recipientId, + requireSyncAccess: false) return requestProfile(recipientId: recipientId, udAccess: udAccess, canFailoverUDAuth: true) diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 782b17b43..6ea0ce7ce 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -593,7 +593,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; AnyPromise *sendPromise = [AnyPromise promiseWithResolverBlock:^(PMKResolver resolve) { OWSUDAccess *_Nullable theirUDAccess; if (senderCertificate != nil && selfUDAccess != nil) { - theirUDAccess = [self.udManager udAccessForRecipientId:recipient.recipientId]; + theirUDAccess = [self.udManager udAccessForRecipientId:recipient.recipientId requireSyncAccess:YES]; } OWSMessageSend *messageSend = [[OWSMessageSend alloc] initWithMessage:message @@ -633,7 +633,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; OWSUDAccess *_Nullable selfUDAccess; if (senderCertificate) { - selfUDAccess = [self.udManager udAccessForRecipientId:self.tsAccountManager.localNumber]; + selfUDAccess = [self.udManager udAccessForRecipientId:self.tsAccountManager.localNumber requireSyncAccess:YES]; } void (^successHandler)(void) = ^() { diff --git a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift index 95219d9f9..65a4eb7a7 100644 --- a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift +++ b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift @@ -79,7 +79,8 @@ public class OWSUDAccess: NSObject { func udAccessKey(forRecipientId recipientId: RecipientIdentifier) -> SMKUDAccessKey? @objc - func udAccess(forRecipientId recipientId: RecipientIdentifier) -> OWSUDAccess? + func udAccess(forRecipientId recipientId: RecipientIdentifier, + requireSyncAccess: Bool) -> OWSUDAccess? // MARK: Sender Certificate @@ -152,13 +153,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { @objc public func isUDEnabled() -> Bool { - // Only enable UD if UD is supported by all linked devices, - // so that sync messages can also be sent via UD. - guard let localNumber = tsAccountManager.localNumber() else { - return false - } - let ourAccessMode = unidentifiedAccessMode(forRecipientId: localNumber) - return ourAccessMode == .enabled || ourAccessMode == .unrestricted + return true } @objc @@ -184,35 +179,44 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { } private func unidentifiedAccessMode(forRecipientId recipientId: RecipientIdentifier, + isLocalNumber: Bool, transaction: YapDatabaseReadTransaction) -> UnidentifiedAccessMode { + let defaultValue: UnidentifiedAccessMode = isLocalNumber ? .enabled : .unknown guard let existingRawValue = transaction.object(forKey: recipientId, inCollection: kUnidentifiedAccessCollection) as? Int else { - return .unknown + return defaultValue } guard let existingValue = UnidentifiedAccessMode(rawValue: existingRawValue) else { - return .unknown + return defaultValue } return existingValue } @objc public func unidentifiedAccessMode(forRecipientId recipientId: RecipientIdentifier) -> UnidentifiedAccessMode { + var isLocalNumber = false + if let localNumber = tsAccountManager.localNumber() { + isLocalNumber = recipientId == localNumber + } + var mode: UnidentifiedAccessMode = .unknown dbConnection.read { (transaction) in - mode = self.unidentifiedAccessMode(forRecipientId: recipientId, transaction: transaction) + mode = self.unidentifiedAccessMode(forRecipientId: recipientId, isLocalNumber: isLocalNumber, transaction: transaction) } return mode } @objc public func setUnidentifiedAccessMode(_ mode: UnidentifiedAccessMode, recipientId: String) { + var isLocalNumber = false if let localNumber = tsAccountManager.localNumber() { if recipientId == localNumber { Logger.info("Setting local UD access mode: \(string(forUnidentifiedAccessMode: mode))") + isLocalNumber = true } } dbConnection.readWrite { (transaction) in - let oldMode = self.unidentifiedAccessMode(forRecipientId: recipientId, transaction: transaction) + let oldMode = self.unidentifiedAccessMode(forRecipientId: recipientId, isLocalNumber: isLocalNumber, transaction: transaction) transaction.setObject(mode.rawValue as Int, forKey: recipientId, inCollection: self.kUnidentifiedAccessCollection) @@ -241,29 +245,33 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { // Returns the UD access key for sending to a given recipient. @objc - public func udAccess(forRecipientId recipientId: RecipientIdentifier) -> OWSUDAccess? { - // This check is currently redundant with the "send access key for local number" - // check below, but behavior of isUDEnabled() may change. + public func udAccess(forRecipientId recipientId: RecipientIdentifier, + requireSyncAccess: Bool) -> OWSUDAccess? { guard isUDEnabled() else { if isUDVerboseLoggingEnabled() { Logger.info("UD Send disabled for \(recipientId), UD disabled.") } return nil } - guard let localNumber = tsAccountManager.localNumber() else { - if isUDVerboseLoggingEnabled() { - Logger.info("UD Send disabled for \(recipientId), no local number.") - } - return nil - } - if localNumber != recipientId { - guard udAccess(forRecipientId: localNumber) != nil else { + + if requireSyncAccess { + guard let localNumber = tsAccountManager.localNumber() else { if isUDVerboseLoggingEnabled() { - Logger.info("UD Send disabled for \(recipientId), UD disabled for sync messages.") + Logger.info("UD Send disabled for \(recipientId), no local number.") } return nil } + if localNumber != recipientId { + let selfAccessMode = unidentifiedAccessMode(forRecipientId: localNumber) + guard selfAccessMode != .disabled else { + if isUDVerboseLoggingEnabled() { + Logger.info("UD Send disabled for \(recipientId), UD disabled for sync messages.") + } + return nil + } + } } + let accessMode = unidentifiedAccessMode(forRecipientId: recipientId) if accessMode == .unrestricted { if let udAccessKey = udAccessKey(forRecipientId: recipientId) { From ee87f1b489ccbe1c0af0ac76402186e729c3b228 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 30 Oct 2018 13:28:08 -0400 Subject: [PATCH 3/8] Fix test breakage. --- .../tests/Messages/OWSUDManagerTest.swift | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/SignalServiceKit/tests/Messages/OWSUDManagerTest.swift b/SignalServiceKit/tests/Messages/OWSUDManagerTest.swift index 1322fe113..bc302d941 100644 --- a/SignalServiceKit/tests/Messages/OWSUDManagerTest.swift +++ b/SignalServiceKit/tests/Messages/OWSUDManagerTest.swift @@ -77,24 +77,24 @@ class OWSUDManagerTest: SSKBaseTestSwift { let aliceRecipientId = "+13213214321" - XCTAssert(UnidentifiedAccessMode.unknown == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) + XCTAssert(UnidentifiedAccessMode.enabled == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) + XCTAssertNotNil(udManager.udAccess(forRecipientId: aliceRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.unknown, recipientId: aliceRecipientId) XCTAssert(UnidentifiedAccessMode.unknown == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) + XCTAssertNil(udManager.udAccess(forRecipientId: aliceRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.disabled, recipientId: aliceRecipientId) XCTAssert(UnidentifiedAccessMode.disabled == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) + XCTAssertNil(udManager.udAccess(forRecipientId: aliceRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.enabled, recipientId: aliceRecipientId) XCTAssert(UnidentifiedAccessMode.enabled == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) + XCTAssertNotNil(udManager.udAccess(forRecipientId: aliceRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: aliceRecipientId) XCTAssert(UnidentifiedAccessMode.unrestricted == udManager.unidentifiedAccessMode(forRecipientId: aliceRecipientId)) - XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: aliceRecipientId)) + XCTAssertNotNil(udManager.udAccess(forRecipientId: aliceRecipientId, requireSyncAccess: false)) } func testMode_noProfileKey() { @@ -111,24 +111,24 @@ class OWSUDManagerTest: SSKBaseTestSwift { XCTAssertNotEqual(bobRecipientId, tsAccountManager.localNumber()!) XCTAssertEqual(UnidentifiedAccessMode.unknown, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.unknown, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unknown, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.disabled, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.disabled, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.enabled, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.enabled, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) // Bob should work in unrestricted mode, even if he doesn't have a profile key. udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unrestricted, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNotNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) } func testMode_withProfileKey() { @@ -145,22 +145,22 @@ class OWSUDManagerTest: SSKBaseTestSwift { profileManager.setProfileKeyData(OWSAES256Key.generateRandom().keyData, forRecipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unknown, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.unknown, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unknown, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.disabled, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.disabled, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.enabled, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.enabled, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNotNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: bobRecipientId) XCTAssertEqual(UnidentifiedAccessMode.unrestricted, udManager.unidentifiedAccessMode(forRecipientId: bobRecipientId)) - XCTAssertNotNil(udManager.udRequestAccessKey(forRecipientId: bobRecipientId)) + XCTAssertNotNil(udManager.udAccess(forRecipientId: bobRecipientId, requireSyncAccess: false)) } } From e11d43d1f8e3c4ce9702c0ba9917ce9828d37b6e Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 30 Oct 2018 13:29:42 -0400 Subject: [PATCH 4/8] Respond to CR. --- SignalServiceKit/src/Messages/UD/OWSUDManager.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift index 65a4eb7a7..23b3dcf2b 100644 --- a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift +++ b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift @@ -274,6 +274,8 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { let accessMode = unidentifiedAccessMode(forRecipientId: recipientId) if accessMode == .unrestricted { + // Unrestricted users should use a derived key if possible, + // and fall back to a random key otherwise. if let udAccessKey = udAccessKey(forRecipientId: recipientId) { if isUDVerboseLoggingEnabled() { Logger.info("UD Send enabled for \(recipientId) with unverified key.") From c28d131f970441ab1a6bdc8e6ce97b0eedb935c6 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 30 Oct 2018 14:02:23 -0400 Subject: [PATCH 5/8] Respond to CR. --- .../src/Messages/UD/OWSUDManager.swift | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift index 23b3dcf2b..8fa6666cf 100644 --- a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift +++ b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift @@ -276,18 +276,11 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { if accessMode == .unrestricted { // Unrestricted users should use a derived key if possible, // and fall back to a random key otherwise. - if let udAccessKey = udAccessKey(forRecipientId: recipientId) { - if isUDVerboseLoggingEnabled() { - Logger.info("UD Send enabled for \(recipientId) with unverified key.") - } - return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: false) - } else { - if isUDVerboseLoggingEnabled() { - Logger.info("UD Send enabled for \(recipientId) with random key.") - } - let udAccessKey = randomUDAccessKey() - return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: true) + if isUDVerboseLoggingEnabled() { + Logger.info("UD Send enabled for \(recipientId) with random key.") } + let udAccessKey = randomUDAccessKey() + return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: true) } guard accessMode == .enabled else { From 44f6774396d73199957bbec0ff4707d4f01928e8 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 30 Oct 2018 14:30:10 -0400 Subject: [PATCH 6/8] Apply refinements to UD logic. --- SignalServiceKit/src/Messages/OWSMessageManager.m | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index 8f03c364d..ce403e995 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -1453,13 +1453,12 @@ NS_ASSUME_NONNULL_BEGIN return; } - BOOL isRecipientDevice = YES; SignalRecipient *_Nullable recipient = [SignalRecipient registeredRecipientForRecipientId:localNumber transaction:transaction]; if (!recipient) { OWSFailDebug(@"No local SignalRecipient."); } else { - isRecipientDevice = [recipient.devices containsObject:@(envelope.sourceDevice)]; + BOOL isRecipientDevice = [recipient.devices containsObject:@(envelope.sourceDevice)]; if (!isRecipientDevice) { OWSLogInfo(@"Message received from unknown linked device; adding to local SignalRecipient: %lu.", (unsigned long) envelope.sourceDevice); @@ -1480,9 +1479,6 @@ NS_ASSUME_NONNULL_BEGIN (unsigned long) envelope.sourceDevice); [OWSDevicesService refreshDevices]; - } - - if (!isRecipientDevice || !isInDeviceList) { [self.profileManager fetchLocalUsersProfile]; } } From 7d8b20d0914721e88233caf8093d3cb9bfa7214a Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 30 Oct 2018 14:56:51 -0400 Subject: [PATCH 7/8] Apply refinements to UD logic. --- .../src/Messages/UD/OWSUDManager.swift | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift index 8fa6666cf..95e5a8142 100644 --- a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift +++ b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift @@ -249,7 +249,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { requireSyncAccess: Bool) -> OWSUDAccess? { guard isUDEnabled() else { if isUDVerboseLoggingEnabled() { - Logger.info("UD Send disabled for \(recipientId), UD disabled.") + Logger.info("UD disabled for \(recipientId), UD disabled.") } return nil } @@ -257,7 +257,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { if requireSyncAccess { guard let localNumber = tsAccountManager.localNumber() else { if isUDVerboseLoggingEnabled() { - Logger.info("UD Send disabled for \(recipientId), no local number.") + Logger.info("UD disabled for \(recipientId), no local number.") } return nil } @@ -265,7 +265,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { let selfAccessMode = unidentifiedAccessMode(forRecipientId: localNumber) guard selfAccessMode != .disabled else { if isUDVerboseLoggingEnabled() { - Logger.info("UD Send disabled for \(recipientId), UD disabled for sync messages.") + Logger.info("UD disabled for \(recipientId), UD disabled for sync messages.") } return nil } @@ -273,32 +273,46 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { } let accessMode = unidentifiedAccessMode(forRecipientId: recipientId) - if accessMode == .unrestricted { - // Unrestricted users should use a derived key if possible, - // and fall back to a random key otherwise. + switch accessMode { + case .unrestricted: + // Unrestricted users should use a random key. if isUDVerboseLoggingEnabled() { - Logger.info("UD Send enabled for \(recipientId) with random key.") + Logger.info("UD enabled for \(recipientId) with random key.") } let udAccessKey = randomUDAccessKey() return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: true) - } - - guard accessMode == .enabled else { + case .unknown: + // Unknown users should use a derived key if possible, + // and otherwise use a random key. + if let udAccessKey = udAccessKey(forRecipientId: recipientId) { + if isUDVerboseLoggingEnabled() { + Logger.info("UD unknown for \(recipientId); trying random key.") + } + return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: false) + } else { + if isUDVerboseLoggingEnabled() { + Logger.info("UD unknown for \(recipientId); trying random key.") + } + let udAccessKey = randomUDAccessKey() + return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: true) + } + case .enabled: + guard let udAccessKey = udAccessKey(forRecipientId: recipientId) else { + if isUDVerboseLoggingEnabled() { + Logger.info("UD disabled for \(recipientId), no profile key for this recipient.") + } + return nil + } if isUDVerboseLoggingEnabled() { - Logger.info("UD Send disabled for \(recipientId), UD not enabled for this recipient.") + Logger.info("UD enabled for \(recipientId).") } - return nil - } - if isUDVerboseLoggingEnabled() { - Logger.info("UD Send enabled for \(recipientId).") - } - guard let udAccessKey = udAccessKey(forRecipientId: recipientId) else { + return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: false) + case .disabled: if isUDVerboseLoggingEnabled() { - Logger.info("UD Send disabled for \(recipientId), no profile key for this recipient.") + Logger.info("UD disabled for \(recipientId), UD not enabled for this recipient.") } return nil } - return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: false) } // MARK: - Sender Certificate From 698e48f2d85ef2498b99b28593dfb1caf9c8ecf0 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 30 Oct 2018 16:15:27 -0400 Subject: [PATCH 8/8] Respond to security review. --- SignalMessaging/profiles/OWSProfileManager.m | 2 +- .../profiles/ProfileFetcherJob.swift | 4 ++-- .../src/Messages/OWSMessageManager.m | 4 ++++ .../src/Messages/OWSMessageSender.m | 21 +++++++++++++++--- .../src/Messages/UD/OWSRequestMaker.swift | 16 ++++++++------ .../src/Messages/UD/OWSUDManager.swift | 22 ++++--------------- .../src/Network/WebSockets/OWSWebSocket.m | 5 ----- 7 files changed, 38 insertions(+), 36 deletions(-) diff --git a/SignalMessaging/profiles/OWSProfileManager.m b/SignalMessaging/profiles/OWSProfileManager.m index c31f64414..e846fff17 100644 --- a/SignalMessaging/profiles/OWSProfileManager.m +++ b/SignalMessaging/profiles/OWSProfileManager.m @@ -1013,7 +1013,7 @@ typedef void (^ProfileManagerFailureBlock)(NSError *error); [userProfile clearWithProfileKey:profileKey dbConnection:self.dbConnection completion:^{ - dispatch_async(dispatch_get_main_queue(), ^(void) { + dispatch_async(dispatch_get_main_queue(), ^{ [self.udManager setUnidentifiedAccessMode:UnidentifiedAccessModeUnknown recipientId:recipientId]; [self fetchProfileForRecipientId:recipientId]; diff --git a/SignalMessaging/profiles/ProfileFetcherJob.swift b/SignalMessaging/profiles/ProfileFetcherJob.swift index c28236745..1261b3b0e 100644 --- a/SignalMessaging/profiles/ProfileFetcherJob.swift +++ b/SignalMessaging/profiles/ProfileFetcherJob.swift @@ -195,13 +195,13 @@ public class ProfileFetcherJob: NSObject { } let dataToVerify = Data(count: 32) - guard let expectedVerfier = Cryptography.computeSHA256HMAC(dataToVerify, withHMACKey: udAccessKey.keyData) else { + guard let expectedVerifier = Cryptography.computeSHA256HMAC(dataToVerify, withHMACKey: udAccessKey.keyData) else { owsFailDebug("could not compute verification") udManager.setUnidentifiedAccessMode(.disabled, recipientId: recipientId) return } - guard expectedVerfier == verifier else { + guard expectedVerifier.ows_constantTimeIsEqual(to: verifier) else { Logger.verbose("verifier mismatch, new profile key?") udManager.setUnidentifiedAccessMode(.disabled, recipientId: recipientId) return diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index ce403e995..1247a4a20 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -1453,6 +1453,8 @@ NS_ASSUME_NONNULL_BEGIN return; } + // Consult the device list cache we use for message sending + // whether or not we know about this linked device. SignalRecipient *_Nullable recipient = [SignalRecipient registeredRecipientForRecipientId:localNumber transaction:transaction]; if (!recipient) { @@ -1469,6 +1471,8 @@ NS_ASSUME_NONNULL_BEGIN } } + // Consult the device list cache we use for the "linked device" UI + // whether or not we know about this linked device. NSMutableSet *deviceIdSet = [NSMutableSet new]; for (OWSDevice *device in [OWSDevice currentDevicesWithTransaction:transaction]) { [deviceIdSet addObject:@(device.deviceId)]; diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 6ea0ce7ce..c65d29bc0 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -1076,8 +1076,15 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; OWSLogWarn(@"Sending a message with no device messages."); } - // NOTE: canFailoverUDAuth is NO because UD-auth and Non-UD-auth requests - // use different device lists. + // NOTE: canFailoverUDAuth depends on whether or not we're sending a + // sync message because sync messages use different device lists + // for UD-auth and Non-UD-auth requests. + // + // Therefore, for sync messages, we can't use OWSRequestMaker's + // retry/failover logic; we need to use the message sender's retry + // logic that will build a new set of device messages. + BOOL isSyncMessageSend = messageSend.isLocalNumber; + BOOL canFailoverUDAuth = !isSyncMessageSend; OWSRequestMaker *requestMaker = [[OWSRequestMaker alloc] initWithLabel:@"Message Send" requestFactoryBlock:^(SMKUDAccessKey *_Nullable udAccessKey) { return [OWSRequestFactory submitMessageRequestWithRecipient:recipient.recipientId @@ -1086,14 +1093,18 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; udAccessKey:udAccessKey]; } udAuthFailureBlock:^{ + // Note the UD auth failure so subsequent retries + // to this recipient also use basic auth. [messageSend setHasUDAuthFailed]; } websocketFailureBlock:^{ + // Note the websocket failure so subsequent retries + // to this recipient also use REST. messageSend.hasWebsocketSendFailed = YES; } recipientId:recipient.recipientId udAccess:messageSend.udAccess - canFailoverUDAuth:NO]; + canFailoverUDAuth:canFailoverUDAuth]; [[requestMaker makeRequestObjc] .then(^(OWSRequestMakerResult *result) { dispatch_async([OWSDispatch sendingQueue], ^{ @@ -1578,9 +1589,13 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; udAccessKey:udAccessKey]; } udAuthFailureBlock:^{ + // Note the UD auth failure so subsequent retries + // to this recipient also use basic auth. [messageSend setHasUDAuthFailed]; } websocketFailureBlock:^{ + // Note the websocket failure so subsequent retries + // to this recipient also use REST. messageSend.hasWebsocketSendFailed = YES; } recipientId:recipientId diff --git a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift index d3193bcec..6a9b7b88b 100644 --- a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift +++ b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift @@ -112,8 +112,8 @@ public class RequestMaker: NSObject { if !skipUD { udAccessForRequest = udAccess } - let isUDRequest = udAccessForRequest != nil - let request = requestFactoryBlock(udAccessForRequest?.udAccessKey) + let isUDRequest: Bool = udAccessForRequest != nil + let request: TSRequest = requestFactoryBlock(udAccessForRequest?.udAccessKey) let webSocketType: OWSWebSocketType = (isUDRequest ? .UD : .default) let canMakeWebsocketRequests = (socketManager.canMakeRequests(of: webSocketType) && !skipWebsocket) @@ -142,8 +142,8 @@ public class RequestMaker: NSObject { // failure), mark recipient as _not_ in UD mode, then retry. self.udManager.setUnidentifiedAccessMode(.disabled, recipientId: self.recipientId) self.profileManager.fetchProfile(forRecipientId: self.recipientId) - self.udAuthFailureBlock() + if self.canFailoverUDAuth { Logger.info("UD websocket request '\(self.label)' auth failed; failing over to non-UD websocket request.") return self.makeRequestInternal(skipUD: true, skipWebsocket: skipWebsocket) @@ -185,8 +185,8 @@ public class RequestMaker: NSObject { // failure), mark recipient as _not_ in UD mode, then retry. self.udManager.setUnidentifiedAccessMode(.disabled, recipientId: self.recipientId) self.profileManager.fetchProfile(forRecipientId: self.recipientId) - self.udAuthFailureBlock() + if self.canFailoverUDAuth { Logger.info("UD REST request '\(self.label)' auth failed; failing over to non-UD REST request.") return self.makeRequestInternal(skipUD: true, skipWebsocket: skipWebsocket) @@ -207,9 +207,11 @@ public class RequestMaker: NSObject { } private func requestSucceeded(udAccess: OWSUDAccess?) { + // If this was a UD request... guard let udAccess = udAccess else { return } + // ...made for a user in "unknown" UD access mode... guard udAccess.udAccessMode == .unknown else { return } @@ -217,12 +219,12 @@ public class RequestMaker: NSObject { if udAccess.isRandomKey { // If a UD request succeeds for an unknown user with a random key, // mark recipient as .unrestricted. - self.udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: self.recipientId) + udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: recipientId) } else { // If a UD request succeeds for an unknown user with a non-random key, // mark recipient as .enabled. - self.udManager.setUnidentifiedAccessMode(.enabled, recipientId: self.recipientId) + udManager.setUnidentifiedAccessMode(.enabled, recipientId: recipientId) } - self.profileManager.fetchProfile(forRecipientId: self.recipientId) + profileManager.fetchProfile(forRecipientId: recipientId) } } diff --git a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift index 95e5a8142..012c03eed 100644 --- a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift +++ b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift @@ -60,8 +60,6 @@ public class OWSUDAccess: NSObject { @objc func trustRoot() -> ECPublicKey - @objc func isUDEnabled() -> Bool - @objc func isUDVerboseLoggingEnabled() -> Bool // MARK: - Recipient State @@ -69,9 +67,6 @@ public class OWSUDAccess: NSObject { @objc func setUnidentifiedAccessMode(_ mode: UnidentifiedAccessMode, recipientId: String) - @objc - func randomUDAccessKey() -> SMKUDAccessKey - @objc func unidentifiedAccessMode(forRecipientId recipientId: RecipientIdentifier) -> UnidentifiedAccessMode @@ -151,11 +146,6 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { // MARK: - - @objc - public func isUDEnabled() -> Bool { - return true - } - @objc public func isUDVerboseLoggingEnabled() -> Bool { return true @@ -186,6 +176,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { return defaultValue } guard let existingValue = UnidentifiedAccessMode(rawValue: existingRawValue) else { + owsFailDebug("Couldn't parse mode value.") return defaultValue } return existingValue @@ -247,18 +238,12 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { @objc public func udAccess(forRecipientId recipientId: RecipientIdentifier, requireSyncAccess: Bool) -> OWSUDAccess? { - guard isUDEnabled() else { - if isUDVerboseLoggingEnabled() { - Logger.info("UD disabled for \(recipientId), UD disabled.") - } - return nil - } - if requireSyncAccess { guard let localNumber = tsAccountManager.localNumber() else { if isUDVerboseLoggingEnabled() { Logger.info("UD disabled for \(recipientId), no local number.") } + owsFailDebug("Missing local number.") return nil } if localNumber != recipientId { @@ -286,7 +271,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { // and otherwise use a random key. if let udAccessKey = udAccessKey(forRecipientId: recipientId) { if isUDVerboseLoggingEnabled() { - Logger.info("UD unknown for \(recipientId); trying random key.") + Logger.info("UD unknown for \(recipientId); trying derived key.") } return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: false) } else { @@ -301,6 +286,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { if isUDVerboseLoggingEnabled() { Logger.info("UD disabled for \(recipientId), no profile key for this recipient.") } + owsFailDebug("Couldn't find profile key for UD-enabled user.") return nil } if isUDVerboseLoggingEnabled() { diff --git a/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.m b/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.m index ad7757151..b2f514781 100644 --- a/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.m +++ b/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.m @@ -1045,11 +1045,6 @@ NSString *NSStringFromOWSWebSocketType(OWSWebSocketType type) } #endif - if (!self.udManager.isUDEnabled && self.webSocketType == OWSWebSocketTypeUD) { - OWSLogWarn(@"Suppressing UD socket in prod."); - return; - } - if (!AppReadiness.isAppReady) { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{