From db2678e86b71be78fa96ade7735715bcf2f44efc Mon Sep 17 00:00:00 2001 From: Max Radermacher Date: Mon, 13 Feb 2023 10:48:54 -0800 Subject: [PATCH] Filter storage service contacts more consistently --- Signal.xcodeproj/project.pbxproj | 4 ++ .../StorageServiceManager.swift | 48 ++++++------- .../StorageServiceProto+Sync.swift | 70 +++++++++++++++++-- .../test/StorageServiceContactTest.swift | 50 +++++++++++++ .../src/Contacts/SignalRecipient.h | 2 + .../src/Contacts/SignalRecipient.m | 6 +- .../src/Contacts/SignalRecipient.swift | 13 ---- 7 files changed, 149 insertions(+), 44 deletions(-) create mode 100644 SignalMessaging/test/StorageServiceContactTest.swift diff --git a/Signal.xcodeproj/project.pbxproj b/Signal.xcodeproj/project.pbxproj index 3f999928ce..6e3524e749 100644 --- a/Signal.xcodeproj/project.pbxproj +++ b/Signal.xcodeproj/project.pbxproj @@ -764,6 +764,7 @@ 5049FA3028BEAABE00D6E099 /* ContactDiscoveryV2Connection.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5049FA2E28BEAABE00D6E099 /* ContactDiscoveryV2Connection.swift */; }; 5049FA3228BEAAD800D6E099 /* cdsi.pb.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5049FA3128BEAAD800D6E099 /* cdsi.pb.swift */; }; 505C2ED029937B5B00C23FB2 /* Data+KeyVersionByte.swift in Sources */ = {isa = PBXBuildFile; fileRef = 505C2ECF29937B5B00C23FB2 /* Data+KeyVersionByte.swift */; }; + 505C2EDB29974D2000C23FB2 /* StorageServiceContactTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 505C2EDA29974D2000C23FB2 /* StorageServiceContactTest.swift */; }; 506A924F28FA232100A2EFC2 /* ContactDiscoveryManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = 506A924E28FA232100A2EFC2 /* ContactDiscoveryManager.swift */; }; 506A92512900888900A2EFC2 /* RecipientPickerContainerViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 506A92502900888900A2EFC2 /* RecipientPickerContainerViewController.swift */; }; 506A9252290326A600A2EFC2 /* TextHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4521C3BF1F59F3BA00B4C582 /* TextHelper.swift */; }; @@ -3164,6 +3165,7 @@ 5049FA2E28BEAABE00D6E099 /* ContactDiscoveryV2Connection.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ContactDiscoveryV2Connection.swift; sourceTree = ""; }; 5049FA3128BEAAD800D6E099 /* cdsi.pb.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = cdsi.pb.swift; sourceTree = ""; }; 505C2ECF29937B5B00C23FB2 /* Data+KeyVersionByte.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Data+KeyVersionByte.swift"; sourceTree = ""; }; + 505C2EDA29974D2000C23FB2 /* StorageServiceContactTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StorageServiceContactTest.swift; sourceTree = ""; }; 506A924E28FA232100A2EFC2 /* ContactDiscoveryManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ContactDiscoveryManager.swift; sourceTree = ""; }; 506A92502900888900A2EFC2 /* RecipientPickerContainerViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RecipientPickerContainerViewController.swift; sourceTree = ""; }; 508F0345296F72F4001D88D0 /* CustomCellBackgroundColor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CustomCellBackgroundColor.swift; sourceTree = ""; }; @@ -8931,6 +8933,7 @@ isa = PBXGroup; children = ( F9C612B3284E466B00B2199A /* CGPointExtensionsTest.swift */, + 505C2EDA29974D2000C23FB2 /* StorageServiceContactTest.swift */, F992ACC328F8C9D900906038 /* StripeTest.swift */, 503BDDB3296F3E2C00FED3B2 /* SystemContactsDataProviderTest.swift */, ); @@ -11224,6 +11227,7 @@ 34843B2421432293004DED45 /* SignalBaseTest.m in Sources */, F9844C492867936400B16DD4 /* SignalMeTest.swift in Sources */, 4C3EF802210918740007EBF7 /* SSKProtoEnvelopeTest.swift in Sources */, + 505C2EDB29974D2000C23FB2 /* StorageServiceContactTest.swift in Sources */, 8803FF6628EF89B50023574A /* StorySharingTests.swift in Sources */, 452D1AF12081059C00A67F7F /* StringAdditionsTest.swift in Sources */, F992ACC428F8C9D900906038 /* StripeTest.swift in Sources */, diff --git a/SignalMessaging/Storage Service/StorageServiceManager.swift b/SignalMessaging/Storage Service/StorageServiceManager.swift index 70c2f47538..bb0bdd924a 100644 --- a/SignalMessaging/Storage Service/StorageServiceManager.swift +++ b/SignalMessaging/Storage Service/StorageServiceManager.swift @@ -1030,15 +1030,17 @@ class StorageServiceOperation: OWSOperation { } var orphanedAccountCount = 0 + let currentDate = Date() for (accountId, identifier) in mutableState.accountIdToIdentifierMap where !allManifestItems.contains(identifier) { - // Only consider registered recipients as orphaned, if another client removes - // an unregistered recipient allow it. + // Only consider registered recipients as orphaned. If another client + // removes an unregistered recipient, allow it. guard - let address = OWSAccountIdFinder.address(forAccountId: accountId, transaction: transaction), - let recipient = AnySignalRecipientFinder().signalRecipient(for: address, transaction: transaction), - recipient.isRegistered - else { continue } - + let storageServiceContact = StorageServiceContact.fetch(for: accountId, transaction: transaction), + storageServiceContact.shouldBeInStorageService(currentDate: currentDate), + storageServiceContact.registrationStatus(currentDate: currentDate) == .registered + else { + continue + } mutableState.accountIdChangeMap[accountId] = .updated orphanedAccountCount += 1 } @@ -1254,26 +1256,24 @@ class StorageServiceOperation: OWSOperation { } private func cleanUpOrphanedAccounts(transaction: SDSAnyWriteTransaction) { - // We don't keep unregistered accounts in storage service after a certain amount of time. - // We may also have records for accounts that no longer exists, e.g. that SignalRecipient - // was merged with another recipient. We try to proactively delete these records from storage - // service, but there was a period of time we didn't and we need to cleanup after ourselves. - let orphanedAccountIds = State.current(transaction: transaction) - .accountIdToIdentifierMap - .keys - .filter { accountId in - guard let address = OWSAccountIdFinder.address( - forAccountId: accountId, - transaction: transaction - ) else { return true } + // We don't keep unregistered accounts in storage service after a certain + // amount of time. We may also have records for accounts that no longer + // exist, e.g. that SignalRecipient was merged with another recipient. We + // try to proactively delete these records from storage service, but there + // was a period of time we didn't, and we need to cleanup after ourselves. - guard let recipient = AnySignalRecipientFinder().signalRecipient( - for: address, - transaction: transaction - ) else { return true } + let currentDate = Date() - return !recipient.shouldBeRepresentedInStorageService + func shouldRecipientBeInStorageService(accountId: AccountId) -> Bool { + guard let storageServiceContact = StorageServiceContact.fetch(for: accountId, transaction: transaction) else { + return false } + return storageServiceContact.shouldBeInStorageService(currentDate: currentDate) + } + + let orphanedAccountIds = State.current(transaction: transaction).accountIdToIdentifierMap.keys.filter { + !shouldRecipientBeInStorageService(accountId: $0) + } guard !orphanedAccountIds.isEmpty else { return } diff --git a/SignalMessaging/Storage Service/StorageServiceProto+Sync.swift b/SignalMessaging/Storage Service/StorageServiceProto+Sync.swift index 5ee5f0fc00..8e12bb465f 100644 --- a/SignalMessaging/Storage Service/StorageServiceProto+Sync.swift +++ b/SignalMessaging/Storage Service/StorageServiceProto+Sync.swift @@ -69,6 +69,63 @@ enum StorageServiceMergeResult { // MARK: - Contact Record +struct StorageServiceContact { + private enum Constant { + static let storageServiceUnregisteredThreshold = kMonthInterval + } + + /// Contact records may be unregistered. + var unregisteredAtTimestamp: UInt64? + + init(unregisteredAtTimestamp: UInt64?) { + self.unregisteredAtTimestamp = unregisteredAtTimestamp + } + + enum RegistrationStatus { + case registered + case unregisteredRecently + case unregisteredMoreThanOneMonthAgo + } + + func registrationStatus(currentDate: Date) -> RegistrationStatus { + switch unregisteredAtTimestamp { + case .none: + return .registered + + case .some(let timestamp) where currentDate.timeIntervalSince(Date(millisecondsSince1970: timestamp)) <= Constant.storageServiceUnregisteredThreshold: + return .unregisteredRecently + + case .some: + return .unregisteredMoreThanOneMonthAgo + } + } + + static func fetch(for accountId: AccountId, transaction: SDSAnyReadTransaction) -> Self? { + SignalRecipient.anyFetch(uniqueId: accountId, transaction: transaction).flatMap { Self($0) } + } + + fileprivate init?(_ signalRecipient: SignalRecipient) { + let unregisteredAtTimestamp: UInt64? + if signalRecipient.isRegistered { + unregisteredAtTimestamp = nil + } else { + unregisteredAtTimestamp = ( + signalRecipient.unregisteredAtTimestamp?.uint64Value ?? SignalRecipientDistantPastUnregisteredTimestamp + ) + } + self.init(unregisteredAtTimestamp: unregisteredAtTimestamp) + } + + func shouldBeInStorageService(currentDate: Date) -> Bool { + switch registrationStatus(currentDate: currentDate) { + case .registered, .unregisteredRecently: + return true + case .unregisteredMoreThanOneMonthAgo: + return false + } + } +} + class StorageServiceContactRecordUpdater: StorageServiceRecordUpdater { typealias IdType = AccountId typealias RecordType = StorageServiceProtoContactRecord @@ -103,19 +160,22 @@ class StorageServiceContactRecordUpdater: StorageServiceRecordUpdater { unknownFields: UnknownStorage?, transaction: SDSAnyReadTransaction ) -> StorageServiceProtoContactRecord? { - guard - let address = OWSAccountIdFinder.address(forAccountId: accountId, transaction: transaction), - let recipient = AnySignalRecipientFinder().signalRecipient(for: address, transaction: transaction) - else { + guard let recipient = SignalRecipient.anyFetch(uniqueId: accountId, transaction: transaction) else { + return nil + } + + guard let contact = StorageServiceContact(recipient), contact.shouldBeInStorageService(currentDate: Date()) else { return nil } var builder = StorageServiceProtoContactRecord.builder() - if !recipient.isRegistered, let unregisteredAtTimestamp = recipient.unregisteredAtTimestamp?.uint64Value { + if let unregisteredAtTimestamp = contact.unregisteredAtTimestamp { builder.setUnregisteredAtTimestamp(unregisteredAtTimestamp) } + let address = recipient.address + if let phoneNumber = address.phoneNumber { if PhoneNumber.resemblesE164(phoneNumber) { builder.setServiceE164(phoneNumber) diff --git a/SignalMessaging/test/StorageServiceContactTest.swift b/SignalMessaging/test/StorageServiceContactTest.swift new file mode 100644 index 0000000000..8657f29bd3 --- /dev/null +++ b/SignalMessaging/test/StorageServiceContactTest.swift @@ -0,0 +1,50 @@ +// +// Copyright 2023 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only +// + +import XCTest + +@testable import SignalMessaging + +class StorageServiceContactTest: XCTestCase { + func testRegistrationStatus() { + let now = Date() + let nowMs = now.ows_millisecondsSince1970 + + let testCases: [(UInt64?, StorageServiceContact.RegistrationStatus)] = [ + (nil, .registered), + (nowMs, .unregisteredRecently), + (nowMs, .unregisteredRecently), + (nowMs - 29 * kDayInMs, .unregisteredRecently), + (nowMs - 32 * kDayInMs, .unregisteredMoreThanOneMonthAgo), + (nowMs + 100 * kDayInMs, .unregisteredRecently) + ] + + for (unregisteredAtTimestamp, expectedValue) in testCases { + let storageServiceContact = StorageServiceContact(unregisteredAtTimestamp: unregisteredAtTimestamp) + let actualValue = storageServiceContact.registrationStatus(currentDate: now) + XCTAssertEqual(actualValue, expectedValue, String(describing: unregisteredAtTimestamp)) + } + } + + func testShouldBeInStorageService() { + let now = Date() + let nowMs = now.ows_millisecondsSince1970 + + let testCases: [(UInt64?, Bool)] = [ + (nil, true), + (nowMs, true), + (nowMs, true), + (nowMs - 29 * kDayInMs, true), + (nowMs + 100 * kDayInMs, true), + (nowMs - 32 * kDayInMs, false) + ] + + for (unregisteredAtTimestamp, expectedValue) in testCases { + let storageServiceContact = StorageServiceContact(unregisteredAtTimestamp: unregisteredAtTimestamp) + let actualValue = storageServiceContact.shouldBeInStorageService(currentDate: now) + XCTAssertEqual(actualValue, expectedValue, String(describing: unregisteredAtTimestamp)) + } + } +} diff --git a/SignalServiceKit/src/Contacts/SignalRecipient.h b/SignalServiceKit/src/Contacts/SignalRecipient.h index 855ef3931b..a843b06241 100644 --- a/SignalServiceKit/src/Contacts/SignalRecipient.h +++ b/SignalServiceKit/src/Contacts/SignalRecipient.h @@ -21,6 +21,8 @@ typedef NS_CLOSED_ENUM(NSUInteger, SignalRecipientSource) { SignalRecipientSourceStorageService, }; +extern const uint64_t SignalRecipientDistantPastUnregisteredTimestamp; + /// SignalRecipient serves two purposes: /// /// a) It serves as a cache of "known" Signal accounts. When the service indicates diff --git a/SignalServiceKit/src/Contacts/SignalRecipient.m b/SignalServiceKit/src/Contacts/SignalRecipient.m index 684b947394..f814ed4585 100644 --- a/SignalServiceKit/src/Contacts/SignalRecipient.m +++ b/SignalServiceKit/src/Contacts/SignalRecipient.m @@ -14,6 +14,8 @@ NS_ASSUME_NONNULL_BEGIN const NSUInteger SignalRecipientSchemaVersion = 1; +const uint64_t SignalRecipientDistantPastUnregisteredTimestamp = 1; + @interface SignalRecipient () @property (nonatomic) NSOrderedSet *devices; @@ -37,7 +39,7 @@ const NSUInteger SignalRecipientSchemaVersion = 1; _recipientPhoneNumber = nil; _recipientSchemaVersion = SignalRecipientSchemaVersion; // New recipients start out as "unregistered in the distant past" - _unregisteredAtTimestamp = @1; + _unregisteredAtTimestamp = @(SignalRecipientDistantPastUnregisteredTimestamp); _devices = [NSOrderedSet orderedSet]; return self; @@ -55,7 +57,7 @@ const NSUInteger SignalRecipientSchemaVersion = 1; _recipientPhoneNumber = address.phoneNumber; _recipientSchemaVersion = SignalRecipientSchemaVersion; // New recipients start out as "unregistered in the distant past" - _unregisteredAtTimestamp = @1; + _unregisteredAtTimestamp = @(SignalRecipientDistantPastUnregisteredTimestamp); _devices = [NSOrderedSet orderedSet]; return self; diff --git a/SignalServiceKit/src/Contacts/SignalRecipient.swift b/SignalServiceKit/src/Contacts/SignalRecipient.swift index cc9851a4a9..b688c47cc7 100644 --- a/SignalServiceKit/src/Contacts/SignalRecipient.swift +++ b/SignalServiceKit/src/Contacts/SignalRecipient.swift @@ -16,19 +16,6 @@ extension SignalRecipient { (devices.array as? [NSNumber])?.map { $0.uint32Value } } - private static let storageServiceUnregisteredThreshold = kMonthInterval - - @objc - public var shouldBeRepresentedInStorageService: Bool { - guard !isRegistered else { return true } - - guard let unregisteredAtTimestamp = unregisteredAtTimestamp?.uint64Value else { - return false - } - - return Date().timeIntervalSince(Date(millisecondsSince1970: unregisteredAtTimestamp)) <= Self.storageServiceUnregisteredThreshold - } - // MARK: - public func markAsUnregistered(at timestamp: UInt64? = nil, source: SignalRecipientSource = .local, transaction: SDSAnyWriteTransaction) {