Skip to content

Commit

Permalink
Alert user about abnormal app conditions (#539)
Browse files Browse the repository at this point in the history 
Task/Issue URL: https://app.asana.com/0/72649045549333/1204420573063618/f
iOS PR: duckduckgo/iOS#2110
What kind of version bump will this require?: Patch

Description:

Introduce better handling of critical errors and adjust some EmailManager code to handle errors within.
  • Loading branch information
@jaceklyp
jaceklyp authored Nov 4, 2023
1 parent 5620205 commit 86e4aba
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 18 deletions.
2 changes: 2 additions & 0 deletions Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource {
public var updatesPublisher: AnyPublisher<UpdateEvent, Never> {
updatesSubject.eraseToAnyPublisher()
}
public var onCriticalError: (() -> Void)?

private let errorReporting: EventMapping<ContentBlockerDebugEvents>?
private let getLog: () -> OSLog
Expand Down Expand Up @@ -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
}
Expand Down
17 changes: 15 additions & 2 deletions Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesSourceManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ public class ContentBlockerRulesSourceManager {
public private(set) var fallbackTDSFailure = false

private let errorReporting: EventMapping<ContentBlockerDebugEvents>?
private let onCriticalError: (() -> Void)?
private let getLog: () -> OSLog
private var log: OSLog {
getLog()
Expand All @@ -105,10 +106,12 @@ public class ContentBlockerRulesSourceManager {
init(rulesList: ContentBlockerRulesList,
exceptionsSource: ContentBlockerRulesExceptionsSource,
errorReporting: EventMapping<ContentBlockerDebugEvents>? = nil,
onCriticalError: (() -> Void)? = nil,
log: @escaping @autoclosure () -> OSLog = .disabled) {
self.rulesList = rulesList
self.exceptionsSource = exceptionsSource
self.errorReporting = errorReporting
self.onCriticalError = onCriticalError
self.getLog = log
}

Expand Down Expand Up @@ -186,7 +189,7 @@ public class ContentBlockerRulesSourceManager {
compilationFailed(for: input, with: error, brokenSources: brokenSources)
return
}

compilationFailed(for: input, with: error, brokenSources: brokenSources)
}

Expand All @@ -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.
Expand Down Expand Up @@ -243,10 +247,19 @@ public class ContentBlockerRulesSourceManager {
parameters: params,
onComplete: { _ in
if input.name == DefaultContentBlockerRulesListsSource.Constants.trackerDataSetRulesListName {
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")
}
}

}
34 changes: 21 additions & 13 deletions Sources/BrowserServicesKit/Email/EmailManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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()
Expand All @@ -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")
}
Expand All @@ -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")
}
Expand All @@ -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")
}
Expand All @@ -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")
}
Expand All @@ -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")
}
Expand All @@ -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")
}
Expand Down Expand Up @@ -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()
Expand All @@ -331,19 +334,24 @@ 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")
}
throw error
}
}

public func forceSignOut() {
try? signOut(isForced: true)
}

public func emailAddressFor(_ alias: String) -> String {
return alias + "@" + Self.emailDomain
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand Down
4 changes: 2 additions & 2 deletions Sources/SyncDataProviders/Settings/SettingsSyncHandlers/EmailProtectionSyncHandler.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Void, Never> { get }
}
Expand Down Expand Up @@ -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
}

Expand Down
4 changes: 3 additions & 1 deletion Tests/BrowserServicesKitTests/Email/EmailManagerTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,9 @@ class MockEmailManagerRequestDelegate: EmailManagerRequestDelegate {
var keychainAccessErrorAccessType: EmailKeychainAccessType?
var keychainAccessError: EmailKeychainAccessError?

func emailManagerKeychainAccessFailed(accessType: EmailKeychainAccessType, error: EmailKeychainAccessError) {
func emailManagerKeychainAccessFailed(_ emailManager: EmailManager,
accessType: EmailKeychainAccessType,
error: EmailKeychainAccessError) {
keychainAccessErrorAccessType = accessType
keychainAccessError = error
}
Expand Down

0 comments on commit 86e4aba

Please sign in to comment.