From b9e1789021c219234b4fe01d217c50b0d6605275 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacek=20=C5=81yp?= Date: Thu, 19 Oct 2023 00:05:55 +0200 Subject: [PATCH 1/4] Add triggers for critical error handling --- .../ContentBlocking/ContentBlockerRulesManager.swift | 2 ++ .../ContentBlockerRulesSourceManager.swift | 12 ++++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift index 13fda8930..52c02d635 100644 --- a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift +++ b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift @@ -116,6 +116,7 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { public var updatesPublisher: AnyPublisher { updatesSubject.eraseToAnyPublisher() } + public var onCriticalError: (() -> Void)? private let errorReporting: EventMapping? private let getLog: () -> OSLog @@ -288,6 +289,7 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { sourceManager = ContentBlockerRulesSourceManager(rulesList: rulesList, exceptionsSource: self.exceptionsSource, errorReporting: self.errorReporting, + onCriticalError: self.onCriticalError, log: log) self.sourceManagers[rulesList.name] = sourceManager } diff --git a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesSourceManager.swift b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesSourceManager.swift index 3a93bee2f..365f85097 100644 --- a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesSourceManager.swift +++ b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesSourceManager.swift @@ -96,6 +96,7 @@ public class ContentBlockerRulesSourceManager { public private(set) var fallbackTDSFailure = false private let errorReporting: EventMapping? + private let onCriticalError: (() -> Void)? private let getLog: () -> OSLog private var log: OSLog { getLog() @@ -105,10 +106,12 @@ public class ContentBlockerRulesSourceManager { init(rulesList: ContentBlockerRulesList, exceptionsSource: ContentBlockerRulesExceptionsSource, errorReporting: EventMapping? = nil, + onCriticalError: (() -> Void)? = nil, log: @escaping @autoclosure () -> OSLog = .disabled) { self.rulesList = rulesList self.exceptionsSource = exceptionsSource self.errorReporting = errorReporting + self.onCriticalError = onCriticalError self.getLog = log } @@ -186,7 +189,7 @@ public class ContentBlockerRulesSourceManager { compilationFailed(for: input, with: error, brokenSources: brokenSources) return } - + compilationFailed(for: input, with: error, brokenSources: brokenSources) } @@ -196,6 +199,7 @@ public class ContentBlockerRulesSourceManager { private func compilationFailed(for input: ContentBlockerRulesSourceIdentifiers, with error: Error, brokenSources: RulesSourceBreakageInfo) { + if input.tdsIdentifier != rulesList.fallbackTrackerData.etag { os_log(.debug, log: log, "Falling back to embedded TDS") // We failed compilation for non-embedded TDS, marking it as broken. @@ -243,7 +247,11 @@ public class ContentBlockerRulesSourceManager { parameters: params, onComplete: { _ in if input.name == DefaultContentBlockerRulesListsSource.Constants.trackerDataSetRulesListName { - fatalError("Could not compile embedded rules list") + if let onCriticalError = self.onCriticalError { + onCriticalError() + } else { + fatalError("Could not compile embedded rules list") + } } }) fallbackTDSFailure = true From 31f8d8b4875a7ff3035b0750ea428553f4eb4ac4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacek=20=C5=81yp?= Date: Thu, 19 Oct 2023 11:28:28 +0200 Subject: [PATCH 2/4] Implement email sign out scenario --- .../Email/EmailManager.swift | 34 ++++++++++++------- .../Email/EmailManagerTests.swift | 2 +- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/Sources/BrowserServicesKit/Email/EmailManager.swift b/Sources/BrowserServicesKit/Email/EmailManager.swift index 7cc298919..a5f88c9a9 100644 --- a/Sources/BrowserServicesKit/Email/EmailManager.swift +++ b/Sources/BrowserServicesKit/Email/EmailManager.swift @@ -104,8 +104,10 @@ public protocol EmailManagerRequestDelegate: AnyObject { httpBody: Data?, timeoutInterval: TimeInterval) async throws -> Data - func emailManagerKeychainAccessFailed(accessType: EmailKeychainAccessType, error: EmailKeychainAccessError) - + func emailManagerKeychainAccessFailed(_ emailManager: EmailManager, + accessType: EmailKeychainAccessType, + error: EmailKeychainAccessError) + } // swiftlint:enable function_parameter_count @@ -164,6 +166,7 @@ public class EmailManager { public enum NotificationParameter { public static let cohort = "cohort" + public static let isForcedSignOut = "isForcedSignOut" } private lazy var emailUrls = EmailUrls() @@ -184,7 +187,7 @@ public class EmailManager { return try storage.getUsername() } catch { if let error = error as? EmailKeychainAccessError { - requestDelegate?.emailManagerKeychainAccessFailed(accessType: .getUsername, error: error) + requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .getUsername, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } @@ -203,7 +206,7 @@ public class EmailManager { return try storage.getToken() } catch { if let error = error as? EmailKeychainAccessError { - requestDelegate?.emailManagerKeychainAccessFailed(accessType: .getToken, error: error) + requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .getToken, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } @@ -222,7 +225,7 @@ public class EmailManager { return try storage.getAlias() } catch { if let error = error as? EmailKeychainAccessError { - requestDelegate?.emailManagerKeychainAccessFailed(accessType: .getAlias, error: error) + requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .getAlias, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } @@ -241,7 +244,7 @@ public class EmailManager { return try storage.getCohort() } catch { if let error = error as? EmailKeychainAccessError { - requestDelegate?.emailManagerKeychainAccessFailed(accessType: .getCohort, error: error) + requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .getCohort, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } @@ -260,7 +263,7 @@ public class EmailManager { return try storage.getLastUseDate() ?? "" } catch { if let error = error as? EmailKeychainAccessError { - requestDelegate?.emailManagerKeychainAccessFailed(accessType: .getLastUseData, error: error) + requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .getLastUseData, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } @@ -281,7 +284,7 @@ public class EmailManager { try storage.store(lastUseDate: dateString) } catch { if let error = error as? EmailKeychainAccessError { - requestDelegate?.emailManagerKeychainAccessFailed(accessType: .storeLastUseDate, error: error) + requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .storeLastUseDate, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } @@ -314,7 +317,7 @@ public class EmailManager { dateFormatter.timeZone = TimeZone(identifier: "America/New_York") // Use ET time zone } - public func signOut() throws { + public func signOut(isForced: Bool = false) throws { Self.lock.lock() defer { Self.lock.unlock() @@ -331,12 +334,13 @@ public class EmailManager { if let currentCohortValue = currentCohortValue { notificationParameters[NotificationParameter.cohort] = currentCohortValue } + notificationParameters[NotificationParameter.isForcedSignOut] = isForced ? "true" : nil NotificationCenter.default.post(name: .emailDidSignOut, object: self, userInfo: notificationParameters) } catch { if let error = error as? EmailKeychainAccessError { - self.requestDelegate?.emailManagerKeychainAccessFailed(accessType: .deleteAuthenticationState, error: error) + self.requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .deleteAuthenticationState, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } @@ -344,6 +348,10 @@ public class EmailManager { } } + public func forceSignOut() { + try? signOut(isForced: true) + } + public func emailAddressFor(_ alias: String) -> String { return alias + "@" + Self.emailDomain } @@ -540,7 +548,7 @@ public extension EmailManager { } catch { if let error = error as? EmailKeychainAccessError { - requestDelegate?.emailManagerKeychainAccessFailed(accessType: .storeTokenUsernameCohort, error: error) + requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .storeTokenUsernameCohort, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } @@ -597,7 +605,7 @@ private extension EmailManager { try storage.deleteAlias() } catch { if let error = error as? EmailKeychainAccessError { - self.requestDelegate?.emailManagerKeychainAccessFailed(accessType: .deleteAlias, error: error) + self.requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .deleteAlias, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } @@ -642,7 +650,7 @@ private extension EmailManager { try self.storage.store(alias: alias) } catch { if let error = error as? EmailKeychainAccessError { - self.requestDelegate?.emailManagerKeychainAccessFailed(accessType: .storeAlias, error: error) + self.requestDelegate?.emailManagerKeychainAccessFailed(self, accessType: .storeAlias, error: error) } else { assertionFailure("Expected EmailKeychainAccessFailure") } diff --git a/Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift b/Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift index d4cccb1e2..d96baa190 100644 --- a/Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift +++ b/Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift @@ -391,7 +391,7 @@ class MockEmailManagerRequestDelegate: EmailManagerRequestDelegate { var keychainAccessErrorAccessType: EmailKeychainAccessType? var keychainAccessError: EmailKeychainAccessError? - func emailManagerKeychainAccessFailed(accessType: EmailKeychainAccessType, error: EmailKeychainAccessError) { + func emailManagerKeychainAccessFailed(self, accessType: EmailKeychainAccessType, error: EmailKeychainAccessError) { keychainAccessErrorAccessType = accessType keychainAccessError = error } From ce64fd6c61e0a323cd565d6a253ce4bf94ef609c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacek=20=C5=81yp?= Date: Thu, 19 Oct 2023 11:36:03 +0200 Subject: [PATCH 3/4] Fix runtime issues --- .../SettingsSyncHandlers/EmailProtectionSyncHandler.swift | 4 ++-- Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/Sources/SyncDataProviders/Settings/SettingsSyncHandlers/EmailProtectionSyncHandler.swift b/Sources/SyncDataProviders/Settings/SettingsSyncHandlers/EmailProtectionSyncHandler.swift index c983be43a..e0bd5838e 100644 --- a/Sources/SyncDataProviders/Settings/SettingsSyncHandlers/EmailProtectionSyncHandler.swift +++ b/Sources/SyncDataProviders/Settings/SettingsSyncHandlers/EmailProtectionSyncHandler.swift @@ -28,7 +28,7 @@ public protocol EmailManagerSyncSupporting: AnyObject { func getToken() throws -> String? func signIn(username: String, token: String) throws - func signOut() throws + func signOut(isForced: Bool) throws var userDidToggleEmailProtectionPublisher: AnyPublisher { get } } @@ -60,7 +60,7 @@ class EmailProtectionSyncHandler: SettingsSyncHandling { func setValue(_ value: String?) throws { guard let value, let valueData = value.data(using: .utf8) else { - try emailManager.signOut() + try emailManager.signOut(isForced: false) return } diff --git a/Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift b/Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift index d96baa190..44a23bf87 100644 --- a/Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift +++ b/Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift @@ -391,7 +391,9 @@ class MockEmailManagerRequestDelegate: EmailManagerRequestDelegate { var keychainAccessErrorAccessType: EmailKeychainAccessType? var keychainAccessError: EmailKeychainAccessError? - func emailManagerKeychainAccessFailed(self, accessType: EmailKeychainAccessType, error: EmailKeychainAccessError) { + func emailManagerKeychainAccessFailed(_ emailManager: EmailManager, + accessType: EmailKeychainAccessType, + error: EmailKeychainAccessError) { keychainAccessErrorAccessType = accessType keychainAccessError = error } From 4c550de80006625ef21592af30a05a3db5beacf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacek=20=C5=81yp?= Date: Thu, 19 Oct 2023 15:36:29 +0200 Subject: [PATCH 4/4] Fix swiftlint warning --- .../ContentBlockerRulesSourceManager.swift | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesSourceManager.swift b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesSourceManager.swift index 365f85097..b67712c6f 100644 --- a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesSourceManager.swift +++ b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesSourceManager.swift @@ -247,14 +247,19 @@ public class ContentBlockerRulesSourceManager { parameters: params, onComplete: { _ in if input.name == DefaultContentBlockerRulesListsSource.Constants.trackerDataSetRulesListName { - if let onCriticalError = self.onCriticalError { - onCriticalError() - } else { - fatalError("Could not compile embedded rules list") - } + self.handleCriticalError() } }) fallbackTDSFailure = true } } + + private func handleCriticalError() { + if let onCriticalError = self.onCriticalError { + onCriticalError() + } else { + fatalError("Could not compile embedded rules list") + } + } + }