Skip to content

Commit

Permalink
Sync: Send pixels for account removal + decoding issues (#1065)
Browse files Browse the repository at this point in the history 
Please review the release process for BrowserServicesKit
[here](https://app.asana.com/0/1200194497630846/1200837094583426).

**Required**:

Task/Issue URL:
https://app.asana.com/0/1201493110486074/1208723035104886/f
iOS PR: duckduckgo/iOS#3557
macOS PR: duckduckgo/macos-browser#3530
What kind of version bump will this require?: Minor

**Description**:

Adds sync errors for catching when the keychain read throws a decoding
error and when the account is deleted for various reasons

**Steps to test this PR**:
1. Make sure you've already activated sync.
2. Deactivate sync (either delete the server data or just turn it off)
3. You should see the `sync_account_removed_reason_user-turned-off`
pixel in the console.

**OS Testing**:

* [ ] iOS 14
* [ ] iOS 15
* [ ] iOS 16
* [ ] macOS 10.15
* [ ] macOS 11
* [ ] macOS 12

---
###### Internal references:
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
  • Loading branch information
@graeme @mgurgel
graeme authored and mgurgel committed Nov 18, 2024
1 parent 66186d5 commit 5375ef7
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 25 deletions.
56 changes: 37 additions & 19 deletions Sources/DDGSync/DDGSync.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public class DDGSync: DDGSyncing {
}
do {
try await disconnect(deviceId: deviceId)
try updateAccount(nil)
try removeAccount(reason: .userTurnedOffSync)
} catch {
try handleUnauthenticated(error)
}
Expand Down Expand Up @@ -180,7 +180,7 @@ public class DDGSync: DDGSyncing {

do {
try await dependencies.account.deleteAccount(account)
try updateAccount(nil)
try removeAccount(reason: .userDeletedAccount)
} catch {
try handleUnauthenticated(error)
}
Expand All @@ -194,7 +194,7 @@ public class DDGSync: DDGSyncing {
}

public func updateServerEnvironment(_ serverEnvironment: ServerEnvironment) {
try? updateAccount(nil)
try? removeAccount(reason: .serverEnvironmentUpdated)
dependencies.updateServerEnvironment(serverEnvironment)
authState = .initializing
initializeIfNeeded()
Expand Down Expand Up @@ -227,11 +227,12 @@ public class DDGSync: DDGSyncing {
let syncEnabled = dependencies.keyValueStore.object(forKey: Constants.syncEnabledKey) != nil
guard syncEnabled else {
try? dependencies.secureStore.removeAccount()
dependencies.errorEvents.fire(.accountRemoved(.syncEnabledNotSetOnKeyValueStore))
authState = .inactive
return
}

var account: SyncAccount?
let account: SyncAccount?
do {
account = try dependencies.secureStore.account()
} catch {
Expand All @@ -241,29 +242,30 @@ public class DDGSync: DDGSyncing {

authState = account?.state ?? .inactive

guard let account else {
do {
try removeAccount(reason: .notFoundInSecureStorage)
} catch {
dependencies.errorEvents.fire(.failedToRemoveAccount, error: error)
}
return
}

do {
try updateAccount(account)
} catch {
dependencies.errorEvents.fire(.failedToSetupEngine, error: error)
}
}

private func updateAccount(_ account: SyncAccount? = nil) throws {
guard account?.state != .initializing else {
private func updateAccount(_ account: SyncAccount) throws {
guard account.state != .initializing else {
assertionFailure("Sync has not been initialized properly")
return
}

guard var account, account.state != .inactive else {
dependencies.scheduler.isEnabled = false
startSyncCancellable?.cancel()
syncQueueCancellable?.cancel()
isDataSyncingFeatureFlagEnabledCancellable?.cancel()
try syncQueue?.dataProviders.forEach { try $0.deregisterFeature() }
syncQueue = nil
authState = .inactive
try dependencies.secureStore.removeAccount()
dependencies.keyValueStore.set(nil, forKey: Constants.syncEnabledKey)
guard account.state != .inactive else {
try removeAccount(reason: .authStateInactive)
return
}

Expand All @@ -274,9 +276,12 @@ public class DDGSync: DDGSyncing {
try syncQueue.prepareDataModelsForSync(needsRemoteDataFetch: account.state == .addingNewDevice)

if account.state != .active {
account = account.updatingState(.active)
let activatedAccount = account.updatingState(.active)
try dependencies.secureStore.persistAccount(activatedAccount)
} else {
try dependencies.secureStore.persistAccount(account)
}
try dependencies.secureStore.persistAccount(account)

authState = account.state
dependencies.keyValueStore.set(true, forKey: Constants.syncEnabledKey)

Expand Down Expand Up @@ -328,6 +333,19 @@ public class DDGSync: DDGSyncing {
self.syncQueue = syncQueue
}

private func removeAccount(reason: SyncError.AccountRemovedReason) throws {
dependencies.scheduler.isEnabled = false
startSyncCancellable?.cancel()
syncQueueCancellable?.cancel()
isDataSyncingFeatureFlagEnabledCancellable?.cancel()
try syncQueue?.dataProviders.forEach { try $0.deregisterFeature() }
syncQueue = nil
authState = .inactive
try dependencies.secureStore.removeAccount()
dependencies.keyValueStore.set(nil, forKey: Constants.syncEnabledKey)
dependencies.errorEvents.fire(.accountRemoved(reason))
}

private func handleUnauthenticated(_ error: Error) throws {
guard let syncError = error as? SyncError,
case .unexpectedStatusCode(let statusCode) = syncError,
Expand All @@ -336,7 +354,7 @@ public class DDGSync: DDGSyncing {
}

do {
try updateAccount(nil)
try removeAccount(reason: .unauthenticatedRequest)
throw SyncError.unauthenticatedWhileLoggedIn
} catch {
Logger.sync.error("Failed to delete account upon unauthenticated server response: \(error.localizedDescription, privacy: .public)")
Expand Down
26 changes: 24 additions & 2 deletions Sources/DDGSync/SyncError.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,22 @@ import Foundation

public enum SyncError: Error, Equatable {

public enum AccountRemovedReason: String, Equatable {
case authStateInactive = "auth-state-inactive"
case syncEnabledNotSetOnKeyValueStore = "not-set-on-key-value-store"
case notFoundInSecureStorage = "not-found-in-secure-storage"
case userTurnedOffSync = "user-turned-off"
case userDeletedAccount = "user-deleted-account"
case unauthenticatedRequest = "unauthenticated-request"
case serverEnvironmentUpdated = "server-environment-updated"
}

case noToken

case failedToMigrate
case failedToLoadAccount
case failedToSetupEngine
case failedToRemoveAccount

case failedToCreateAccountKeys(_ message: String)
case accountNotFound
Expand All @@ -38,7 +49,7 @@ public enum SyncError: Error, Equatable {
case unableToEncodeRequestBody(_ message: String)
case unableToDecodeResponse(_ message: String)
case invalidDataInResponse(_ message: String)
case accountRemoved
case accountRemoved(_ reason: AccountRemovedReason)

case failedToEncryptValue(_ message: String)
case failedToDecryptValue(_ message: String)
Expand All @@ -49,6 +60,7 @@ public enum SyncError: Error, Equatable {
case failedToWriteSecureStore(status: OSStatus)
case failedToReadSecureStore(status: OSStatus)
case failedToRemoveSecureStore(status: OSStatus)
case failedToDecodeSecureStoreData(error: NSError)

case credentialsMetadataMissingBeforeFirstSync
case receivedCredentialsWithoutUUID
Expand Down Expand Up @@ -140,6 +152,10 @@ public enum SyncError: Error, Equatable {
return [syncErrorString: "unauthenticatedWhileLoggedIn"]
case .patchPayloadCompressionFailed:
return [syncErrorString: "patchPayloadCompressionFailed"]
case .failedToRemoveAccount:
return [syncErrorString: "failedToRemoveAccount"]
case .failedToDecodeSecureStoreData:
return [syncErrorString: "failedToDecodeSecureStoreData"]
}
}
}
Expand Down Expand Up @@ -187,14 +203,20 @@ extension SyncError: CustomNSError {
case .settingsMetadataNotPresent: return 27
case .unauthenticatedWhileLoggedIn: return 28
case .patchPayloadCompressionFailed: return 29
case .failedToRemoveAccount: return 30
case .failedToDecodeSecureStoreData: return 31
}
}

public var errorUserInfo: [String: Any] {
switch self {
case .failedToReadSecureStore(let status), .failedToWriteSecureStore(let status), .failedToRemoveSecureStore(let status):
case .failedToReadSecureStore(let status),
.failedToWriteSecureStore(let status),
.failedToRemoveSecureStore(let status):
let underlyingError = NSError(domain: "secError", code: Int(status))
return [NSUnderlyingErrorKey: underlyingError]
case .failedToDecodeSecureStoreData(let error):
return [NSUnderlyingErrorKey: error]
default:
return [:]
}
Expand Down
6 changes: 5 additions & 1 deletion Sources/DDGSync/internal/SecureStorage.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ struct SecureStorage: SecureStoring {
}

if let data = item as? Data {
return try JSONDecoder.snakeCaseKeys.decode(SyncAccount.self, from: data)
do {
return try JSONDecoder.snakeCaseKeys.decode(SyncAccount.self, from: data)
} catch {
throw SyncError.failedToDecodeSecureStoreData(error: error as NSError)
}
}

return nil
Expand Down
6 changes: 3 additions & 3 deletions Tests/DDGSyncTests/DDGSyncLifecycleTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ final class DDGSyncLifecycleTests: XCTestCase {
XCTAssertEqual(syncService.authState, .initializing)
syncService.initializeIfNeeded()
XCTAssertEqual(syncService.authState, .inactive)
XCTAssertEqual(mockErrorHandler.handledErrors, [])
XCTAssertEqual(mockErrorHandler.handledErrors, [.accountRemoved(.notFoundInSecureStorage)])
}

func testWhenInitializingAndOnThenStateIsActive() {
Expand All @@ -72,7 +72,7 @@ final class DDGSyncLifecycleTests: XCTestCase {
syncService.initializeIfNeeded()
XCTAssertEqual(syncService.authState, .inactive)
XCTAssertNil(secureStorageStub.theAccount)
XCTAssertEqual(mockErrorHandler.handledErrors, [])
XCTAssertEqual(mockErrorHandler.handledErrors, [.accountRemoved(.syncEnabledNotSetOnKeyValueStore)])
}

func testWhenInitializingAndKeysBeenRemovedThenStateIsInactive() {
Expand All @@ -88,7 +88,7 @@ final class DDGSyncLifecycleTests: XCTestCase {
// XCTAssertNil(mockKeyValueStore.isSyncEnabled)

XCTAssertNil(secureStorageStub.theAccount)
XCTAssertEqual(mockErrorHandler.handledErrors, [])
XCTAssertEqual(mockErrorHandler.handledErrors, [.accountRemoved(.notFoundInSecureStorage)])
}

func testWhenInitializingAndCannotReadAccountThenErrorIsReportedAndInitializationIsPostponed() {
Expand Down

0 comments on commit 5375ef7

Please sign in to comment.