diff --git a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift index a3888ccc..d3cff726 100644 --- a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift +++ b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift @@ -86,6 +86,19 @@ final class AccountSyncCoordinator { private var lastResolvedNamespace: String? + /// Set once the DEBUG identity-scope simulator has invented a scope, and + /// never cleared: from that moment this process is describing an Apple + /// account it is not signed in to. + /// + /// Only the *namespaces* are invented — the database, the zone and the + /// records in it stay real. Without this, activating under `debug-identity-A` + /// would bootstrap that namespace out of the genuine private zone and then + /// upload the results back into it stamped with a name no identity will ever + /// answer to, which is real data lost inside a real zone. Compiled in every + /// configuration so the guards below read the same everywhere; only DEBUG + /// has any way to set it. + private var debugSimulationActive = false + private var isActivating = false private var activationRequested = false @@ -250,6 +263,31 @@ final class AccountSyncCoordinator { database = nil } + #if DEBUG + /// Suspends account sync for the rest of this process, for the identity + /// scope simulator in Developer settings. + /// + /// Called *before* the first invented scope is announced. The simulator's + /// whole point is to drive the local→identity import without an Apple + /// account to sign into, and the namespaces it invents are only safe as long + /// as nothing carries them to the server: the private database underneath is + /// the real one, so a bootstrap under a fake namespace reads genuine records + /// and the upload that follows writes them back stamped with a name the real + /// identity will never recognise. + /// + /// Never lifted. The simulated scope itself lasts until the app is + /// relaunched, and a coordinator that resumed while the scope was still + /// invented would be exactly the thing this prevents. + func enterDebugScopeSimulation() { + guard !debugSimulationActive else { return } + debugSimulationActive = true + + LoggingService.shared.logCloudKit( + "Account sync is suspended for this launch: the identity scope is being simulated" + ) + } + #endif + // MARK: - Fetching /// Notes which identity the fetch now starting belongs to. @@ -274,6 +312,23 @@ final class AccountSyncCoordinator { guard !accountRecords.isEmpty || !accountDeletions.isEmpty else { return } + // Refused rather than staged, and treated exactly like records that + // arrived before an identity could be attributed to them. Staging is + // namespaced, so these genuine records would be filed under an invented + // name — where applying them is the pollution this is preventing, and + // not applying them is a durable copy of real data nothing will read + // again. The recovery flag is what makes them recoverable: the engine's + // token has moved past them, and the real identity re-bootstraps its + // zone from a nil token on the next launch, which is when the scope is + // real again. + guard !debugSimulationActive else { + store.hasPendingIdentityRecovery = true + LoggingService.shared.logCloudKit( + "Dropping \(accountRecords.count + accountDeletions.count) account records fetched under a simulated identity" + ) + return + } + // The identity the *fetch* began under, not whatever is current now. let owner = fetchIdentity ?? (namespace, identityGeneration) @@ -343,6 +398,15 @@ final class AccountSyncCoordinator { } private func performActivation() async { + // Before anything is read or written, and on the same terms as the + // category being off: staged records are kept, nothing is applied, and + // nothing is enqueued. A simulated scope names an Apple account this + // device is not signed in to, while the database it would activate + // against is that device's real one. + guard !debugSimulationActive else { + LoggingService.shared.logCloudKit("Account sync is suspended: the identity scope is simulated") + return + } guard let namespace else { return } guard isCategoryEnabled() else { LoggingService.shared.logCloudKit("Account sync category is off — incoming records stay deferred") @@ -545,6 +609,10 @@ final class AccountSyncCoordinator { /// Runs outside the engine's own token: the engine's state and every other /// category's pending changes are untouched. private func runBootstrapIfNeeded(in namespace: String) async { + // Unreachable through `performActivation`, which already refuses. Kept + // because this is the one place that reads the real zone from a nil + // token, and it must never do that on behalf of an invented namespace. + guard !debugSimulationActive else { return } guard !store.isBootstrapCompleted(in: namespace) else { return } guard let database else { return } @@ -634,6 +702,8 @@ final class AccountSyncCoordinator { /// Safe to call repeatedly: the engine collapses duplicate pending changes, /// and each entry is cleared only by a server confirmation. func enqueueDirtyRecords() { + // Nothing an invented namespace owes is owed to anybody's real zone. + guard !debugSimulationActive else { return } guard let enqueueChanges, let namespace else { return } guard isCategoryEnabled(), store.isBootstrapCompleted(in: namespace) else { return } guard isManagerLoaded(for: namespace) else { return } @@ -668,6 +738,10 @@ final class AccountSyncCoordinator { /// Registers every account record for upload, for a zone with nothing in it /// — first enable, or a zone recreated after being deleted elsewhere. func uploadEverything() { + // A simulated identity has no zone to fill, and marking its accounts + // dirty would only leave an outbox nothing can ever settle. + guard !debugSimulationActive else { return } + // Marking everything dirty commits the *in-memory* list to this // identity's zone, so it must not run against a snapshot loaded under // another one. @@ -719,6 +793,10 @@ final class AccountSyncCoordinator { /// upload ordering. Holding is safe because the outbox is durable: the /// change is registered again as soon as what it waits on is confirmed. func recordToSave(named recordName: String) -> CKRecord? { + // The last gate before a record reaches the server, and the one that + // would stamp it with a simulated identity. Withdrawn instead: the + // outbox is durable, and this launch has nowhere legitimate to send it. + guard !debugSimulationActive else { return nil } guard let namespace, isCategoryEnabled(), store.isBootstrapCompleted(in: namespace) else { return nil } guard isManagerLoaded(for: namespace) else { return nil } diff --git a/Yattee/Services/Credentials/AccountCredentialsManager.swift b/Yattee/Services/Credentials/AccountCredentialsManager.swift index fd58e87e..1385088c 100644 --- a/Yattee/Services/Credentials/AccountCredentialsManager.swift +++ b/Yattee/Services/Credentials/AccountCredentialsManager.swift @@ -61,9 +61,10 @@ protocol AccountCredentialsManager: AnyObject, Observable { /// current one, for a record being imported out of the `local` namespace /// when an Apple account claims it. /// - /// - Returns: `true` when `sourceScope` no longer holds anything for this - /// account. `false` means the move failed and left the source intact, so - /// the record it belongs to must stay where it is and be retried. + /// - Returns: `true` only when `sourceScope` holds nothing for this account + /// any more — credential and username both. `false` means something is + /// still keyed to the source, so the record it belongs to must stay where + /// it is: that record is what brings the retry. @discardableResult func migrateCredential(forAccountID accountID: UUID, fromScope sourceScope: String) -> Bool @@ -73,7 +74,11 @@ protocol AccountCredentialsManager: AnyObject, Observable { /// refused because this identity already tombstoned the account has to take /// the secret with it, and that secret is keyed by the scope being left /// rather than the one in force. - func deleteCredential(forAccountID accountID: UUID, inScope scope: String) + /// + /// - Returns: whether that scope is now free of the account, on the same + /// terms as `migrateCredential(forAccountID:fromScope:)`. + @discardableResult + func deleteCredential(forAccountID accountID: UUID, inScope scope: String) -> Bool // MARK: - Legacy adoption diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index 59a9f125..38b7cf08 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -224,11 +224,12 @@ final class AccountKeychainCredentials { /// the source left completely untouched on any failure so the next sweep can /// try again. /// - /// - Returns: `true` when `sourceScope` no longer owes this account - /// anything — the credential moved, the target already held one and the - /// source copy was redundant, or there was never one to move. `false` - /// means the source is exactly as it was, and the caller must not prune - /// the record that points at it. + /// - Returns: `true` only when `sourceScope` really holds nothing for this + /// account any more: the target credential verified (or there was never + /// one to move), the source items are gone, and the username pair is + /// settled the same way. `false` means something is still keyed to the + /// source, so the caller must not prune the record that points at it — + /// that record is the only thing that will bring a retry back here. @discardableResult func migrateCredential(forAccountID accountID: UUID, fromScope sourceScope: String) -> Bool { guard let scope = currentScope() else { return false } @@ -241,21 +242,28 @@ final class AccountKeychainCredentials { guard let secret = store.value(forKey: sourceKey) else { // An account that was never signed in under the source scope. Its // username still moves — it is what the sign-in sheet prefills — - // and there is nothing left for a retry to recover, so the record - // may be pruned. - migrateUsername(forAccountID: accountID, from: sourceScope, to: scope) + // and the record may only be pruned once that move is settled too. loginsByScope[sourceScope]?.remove(accountID) - return true + return migrateUsername(forAccountID: accountID, from: sourceScope, to: scope) } // Decision 5: a credential already stored under the current identity was // written by a sign-in this Apple account actually made, so the copy // being migrated is redundant rather than a replacement. if store.value(forKey: targetKey) != nil { - migrateUsername(forAccountID: accountID, from: sourceScope, to: scope) - store.delete(key: sourceKey) - loginsByScope[sourceScope]?.remove(accountID) + let usernameSettled = migrateUsername(forAccountID: accountID, from: sourceScope, to: scope) + let sourceCleared = removed(key: sourceKey) loginsByScope[scope, default: []].insert(accountID) + if sourceCleared { loginsByScope[sourceScope]?.remove(accountID) } + + guard usernameSettled, sourceCleared else { + LoggingService.shared.error( + "A redundant \(credentialNoun) could not be cleared out of the namespace being left", + category: .keychain, + details: "accountID=\(accountID), from=\(sourceScope)" + ) + return false + } LoggingService.shared.info( "Dropped a \(credentialNoun) made redundant by one already held under this identity", @@ -304,9 +312,14 @@ final class AccountKeychainCredentials { return false } - migrateUsername(forAccountID: accountID, from: sourceScope, to: scope) - store.delete(key: sourceKey) + let usernameSettled = migrateUsername(forAccountID: accountID, from: sourceScope, to: scope) + let sourceCleared = removed(key: sourceKey) + // Recorded on the strength of the verified copy, not of the cleanup + // that follows it. A retry after a failed deletion takes the + // already-has-a-credential path above, which never looks at markers — + // so a marker left unwritten here would describe a copy that has moved + // and never be corrected. if wasAdopted { // The marker describes where the adopted copy now lives, or the // deletion that retires the unscoped item would no longer recognise @@ -326,8 +339,22 @@ final class AccountKeychainCredentials { ) } - loginsByScope[sourceScope]?.remove(accountID) loginsByScope[scope, default: []].insert(accountID) + if sourceCleared { loginsByScope[sourceScope]?.remove(accountID) } + + guard usernameSettled, sourceCleared else { + // The credential is safely under this identity, but something is + // still filed under the one being left. Reporting success would + // prune the record that names it and strand that item forever, so + // the sweep is told to come back: the retry finds the target + // credential in place and only has the leftovers to clear. + LoggingService.shared.error( + "A moved \(credentialNoun) left something behind in the namespace being left", + category: .keychain, + details: "accountID=\(accountID), from=\(sourceScope)" + ) + return false + } LoggingService.shared.info( "Moved a \(credentialNoun) into the current identity", @@ -339,20 +366,51 @@ final class AccountKeychainCredentials { /// Moves the username item, which is a convenience rather than a secret. /// - /// Never fails the migration around it: a credential is what a session - /// depends on, and a username that could not be copied costs a prefilled - /// field. The source is deleted only once the target really holds one. - private func migrateUsername(forAccountID accountID: UUID, from sourceScope: String, to scope: String) { + /// Counts towards the migration's success all the same. Not because a + /// prefilled field is worth a retry, but because the record being imported + /// is the only thing that ever brings the sweep back to this key: prune it + /// while `|#username` still exists and that item is orphaned for + /// good, under a namespace nothing will read again. + /// + /// - Returns: whether the source scope is done with this account's username + /// — nothing to move, or the target holds one and the source item is gone. + private func migrateUsername(forAccountID accountID: UUID, from sourceScope: String, to scope: String) -> Bool { let sourceKey = KeychainCredentialStore.usernameKey(scope: sourceScope, accountID: accountID) - guard let username = store.value(forKey: sourceKey) else { return } + guard let username = store.value(forKey: sourceKey) else { return true } let targetKey = KeychainCredentialStore.usernameKey(scope: scope, accountID: accountID) if store.value(forKey: targetKey) == nil { store.addIfAbsent(username, forKey: targetKey, synchronizable: shouldSyncToiCloud) } - guard store.value(forKey: targetKey) != nil else { return } - store.delete(key: sourceKey) + guard store.value(forKey: targetKey) != nil else { + LoggingService.shared.error( + "A login identifier could not be moved into the current identity", + category: .keychain, + details: "accountID=\(accountID), from=\(sourceScope)" + ) + return false + } + return removed(key: sourceKey) + } + + /// Deletes a Keychain item, reading "there was nothing there" as done. + /// + /// Every caller here is clearing a key out of a namespace being left, and + /// for that question an absent item and a deleted one are the same answer. + /// Anything else is a real failure the sweep has to retry — the item would + /// otherwise outlive the record that knows to come back for it. + private func removed(key: String) -> Bool { + let status = store.delete(key: key) + guard status == errSecSuccess || status == errSecItemNotFound else { + LoggingService.shared.error( + "Could not delete a Keychain item", + category: .keychain, + details: "status=\(store.describe(status))" + ) + return false + } + return true } /// Deletes an account's credential and username under a scope that is not @@ -362,26 +420,51 @@ final class AccountKeychainCredentials { /// deliberately deleted is never imported, and the secret it was keyed to /// goes with the record rather than sitting under a namespace nothing will /// read again. - func deleteCredential(forAccountID accountID: UUID, inScope scope: String) { - store.delete(key: KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID)) - store.delete(key: KeychainCredentialStore.usernameKey(scope: scope, accountID: accountID)) - loginsByScope[scope]?.remove(accountID) - - // Its marker described a copy that no longer exists. Keyed by instance, - // and only a migrated account can have adopted anything, so the account - // ID is the instance to name here. - markers.removeMarker( - service: store.service, - instanceID: accountID, - accountID: accountID, - scope: scope + /// + /// - Returns: whether that scope now holds nothing for the account. `false` + /// keeps the local record where it is: it is the only thing that brings a + /// sweep back to these keys, so pruning it over a failed deletion would + /// strand the secret it refused. + @discardableResult + func deleteCredential(forAccountID accountID: UUID, inScope scope: String) -> Bool { + let credentialCleared = removed( + key: KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID) ) + let usernameCleared = removed( + key: KeychainCredentialStore.usernameKey(scope: scope, accountID: accountID) + ) + + if credentialCleared { + loginsByScope[scope]?.remove(accountID) + + // Its marker described a copy that no longer exists. Keyed by + // instance, and only a migrated account can have adopted anything, + // so the account ID is the instance to name here. Kept while the + // copy is still there: it is the only record of where that secret + // came from, and the retry that deletes it still needs it. + markers.removeMarker( + service: store.service, + instanceID: accountID, + accountID: accountID, + scope: scope + ) + } + + guard credentialCleared, usernameCleared else { + LoggingService.shared.error( + "A refused import's \(credentialNoun) could not be deleted — leaving it for the next sweep", + category: .keychain, + details: "accountID=\(accountID), scope=\(scope)" + ) + return false + } LoggingService.shared.info( "Deleted a \(credentialNoun) left behind by a refused import", category: .keychain, details: "accountID=\(accountID), scope=\(scope)" ) + return true } // MARK: - Username diff --git a/Yattee/Services/Credentials/InvidiousCredentialsManager.swift b/Yattee/Services/Credentials/InvidiousCredentialsManager.swift index a730755f..b7476cb3 100644 --- a/Yattee/Services/Credentials/InvidiousCredentialsManager.swift +++ b/Yattee/Services/Credentials/InvidiousCredentialsManager.swift @@ -314,7 +314,8 @@ extension InvidiousCredentialsManager: AccountCredentialsManager { accountCredentials.migrateCredential(forAccountID: accountID, fromScope: sourceScope) } - func deleteCredential(forAccountID accountID: UUID, inScope scope: String) { + @discardableResult + func deleteCredential(forAccountID accountID: UUID, inScope scope: String) -> Bool { accountCredentials.deleteCredential(forAccountID: accountID, inScope: scope) } diff --git a/Yattee/Services/Credentials/LocalNamespaceImporter.swift b/Yattee/Services/Credentials/LocalNamespaceImporter.swift index a7e45b29..6b7dbb08 100644 --- a/Yattee/Services/Credentials/LocalNamespaceImporter.swift +++ b/Yattee/Services/Credentials/LocalNamespaceImporter.swift @@ -88,11 +88,29 @@ final class LocalNamespaceImporter { // Decision 5: the account was deliberately deleted under this // identity, so the local copy is not resurrected — and its // secret goes with it rather than outliving the record. + // + // Held to the same per-item atomicity as an import: the local + // record is the only thing that names the secret being deleted, + // so pruning it over a Keychain failure would orphan that + // secret permanently. Left in place, the next sweep refuses the + // account again and retries the deletion. refused.insert(account.id) + + var deleted = true for store in credentialStores(account) { - store.deleteCredential(forAccountID: account.id, inScope: local) + let storeDeleted = store.deleteCredential(forAccountID: account.id, inScope: local) + deleted = deleted && storeDeleted + } + + if deleted { + pruneable.insert(account.id) + } else { + LoggingService.shared.warning( + "A refused account's credential could not be deleted — leaving it for the next sweep", + category: .keychain, + details: "accountID=\(account.id)" + ) } - pruneable.insert(account.id) case .imported, .mergedIntoExisting: // Per-item atomicity: the local record is only dropped once its @@ -165,7 +183,7 @@ final class LocalNamespaceImporter { LoggingService.shared.info( "Imported the signed-out account namespace into the active identity", category: .general, - details: "imported=\(pruneable.count - refused.count), refused=\(refused.count)" + details: "imported=\(pruneable.subtracting(refused).count), refused=\(refused.count)" ) } return didChange diff --git a/Yattee/Services/Credentials/PipedCredentialsManager.swift b/Yattee/Services/Credentials/PipedCredentialsManager.swift index 6590e54c..8d3ec6f9 100644 --- a/Yattee/Services/Credentials/PipedCredentialsManager.swift +++ b/Yattee/Services/Credentials/PipedCredentialsManager.swift @@ -243,7 +243,8 @@ extension PipedCredentialsManager: AccountCredentialsManager { accountCredentials.migrateCredential(forAccountID: accountID, fromScope: sourceScope) } - func deleteCredential(forAccountID accountID: UUID, inScope scope: String) { + @discardableResult + func deleteCredential(forAccountID accountID: UUID, inScope scope: String) -> Bool { accountCredentials.deleteCredential(forAccountID: accountID, inScope: scope) } diff --git a/Yattee/Views/Settings/DeveloperSettingsView.swift b/Yattee/Views/Settings/DeveloperSettingsView.swift index de81cc15..39681fdc 100644 --- a/Yattee/Views/Settings/DeveloperSettingsView.swift +++ b/Yattee/Views/Settings/DeveloperSettingsView.swift @@ -238,9 +238,12 @@ struct DeveloperSettingsView: View { /// do the rest — reload the account namespace, sweep the signed-out one /// into it, refresh statuses. /// - /// Nothing can be uploaded from an invented scope: every send is gated on - /// that namespace's bootstrap having completed, which needs a real zone - /// fetch under a real identity and so never happens for one of these. + /// Account sync is suspended for the rest of the launch before the first + /// switch, and stays suspended. The namespaces are invented but the private + /// database is the device's real one, so an activation under a made-up scope + /// would bootstrap it out of the genuine zone and upload the results back + /// stamped with a name no Apple account answers to. The barrier is in the + /// coordinator, not in this caption. @ViewBuilder private var identityScopeSimulatorSection: some View { if let scope = appEnvironment?.credentialIdentityScope { @@ -266,7 +269,9 @@ struct DeveloperSettingsView: View { verbatim: """ Debug only. The scope stays where you put it until the app is relaunched, \ so it no longer describes the Apple account this device is signed in to. \ - Nothing is uploaded from an invented scope — its namespace never bootstraps. + Switching suspends account iCloud sync — no fetching, no uploading — \ + for the rest of the launch, so a made-up identity can never reach the \ + real zone. Relaunch to sync again. """ ) } @@ -280,6 +285,12 @@ struct DeveloperSettingsView: View { ) -> some View { HStack { Button { + // Suspended first, and for good: the scope change is announced + // synchronously, so a coordinator told afterwards could already + // have activated against the real database under a name that + // belongs to nobody. + appEnvironment?.accountSyncCoordinator.enterDebugScopeSimulation() + // Announcing, so the observers that reload and import run — // exactly what a resolution does. identityScope.setFixedScope(newScope) diff --git a/YatteeTests/AccountSyncTests.swift b/YatteeTests/AccountSyncTests.swift index 590352a5..15e9f7f6 100644 --- a/YatteeTests/AccountSyncTests.swift +++ b/YatteeTests/AccountSyncTests.swift @@ -1127,6 +1127,43 @@ struct AccountSyncTests { #expect(didDiscardCachedRecords) } + // MARK: - Simulated identities + + @Test("A simulated identity scope cannot reach the real zone") + func debugScopeSimulationSuspendsSync() async throws { + // Everything a send is normally gated on is satisfied: the category is + // on, the namespace has bootstrapped, the manager is loaded, and the + // outbox is owed a record. + let harness = Self.makeHarness() + let account = harness.accounts.addAccount(instanceID: UUID()) + let recordName = "account-\(account.id.uuidString)" + #expect(harness.accounts.pendingUploads.contains(account.id)) + + harness.coordinator.enterDebugScopeSimulation() + harness.recorder.clear() + + await harness.coordinator.activate() + harness.coordinator.enqueueDirtyRecords() + harness.coordinator.uploadEverything() + + #expect(harness.recorder.changes.isEmpty) + #expect(harness.coordinator.recordToSave(named: recordName) == nil) + + // Records the engine delivers are refused rather than filed under an + // invented name — and the real identity is told to bootstrap again, so + // what the change token skipped is not lost. + harness.coordinator.stageFetched( + records: [harness.mapper.toCKRecord(accountDeletionID: UUID())], + deletions: [] + ) + #expect(harness.store.stagedChanges(in: harness.scope).isEmpty) + #expect(harness.store.hasPendingIdentityRecovery) + + // The debt itself is untouched: it is owed to whatever identity is real + // after the relaunch that ends the simulation. + #expect(harness.accounts.pendingUploads.contains(account.id)) + } + // MARK: - Importing the signed-out namespace /// What the accounts created while signed out owe the server once an Apple diff --git a/YatteeTests/AccountsScreenTests.swift b/YatteeTests/AccountsScreenTests.swift index c9ab3f88..ce255e38 100644 --- a/YatteeTests/AccountsScreenTests.swift +++ b/YatteeTests/AccountsScreenTests.swift @@ -244,7 +244,7 @@ private final class DeletionOrderStore: AccountCredentialsManager { } func migrateCredential(forAccountID _: UUID, fromScope _: String) -> Bool { true } - func deleteCredential(forAccountID _: UUID, inScope _: String) {} + func deleteCredential(forAccountID _: UUID, inScope _: String) -> Bool { true } func resyncCredentialsWithCurrentSetting() {} func setUsername(_: String, for _: Account) {} func username(for _: Account) -> String? { nil } diff --git a/YatteeTests/LocalNamespaceImportTests.swift b/YatteeTests/LocalNamespaceImportTests.swift index 567dcbc4..ddedc788 100644 --- a/YatteeTests/LocalNamespaceImportTests.swift +++ b/YatteeTests/LocalNamespaceImportTests.swift @@ -160,6 +160,100 @@ struct LocalNamespaceImportTests { #expect(harness.storedAccounts(in: localScope).isEmpty) } + // MARK: - Keychain failures + + @Test("A credential the Keychain refuses to move keeps the account local until it can") + func aFailedCredentialMoveIsRetriedUntilItSettles() async throws { + let flaky = FlakyCredentialsManager() + let harness = Harness(startingScope: CredentialIdentityScope.localScope, credentialStore: flaky) + let account = harness.accounts.addAccount(instanceID: UUID(), displayName: "Main") + defer { harness.cleanUp(account.id) } + + try await harness.flip(to: "identity-A") + + // The record is in both namespaces: imported under the identity, and + // still local because the secret it names has not moved. That is what + // brings the sweep back — pruning here would orphan `local|` with + // nothing left to say it exists. + #expect(flaky.migrateCalls == 1) + #expect(harness.accounts.account(id: account.id)?.displayName == "Main") + #expect(harness.storedAccounts(in: localScope).map(\.id) == [account.id]) + + // The retry converges: the collision rule reads the record it imported + // last time as one this identity already holds, so nothing duplicates. + flaky.refusesEverything = false + #expect(harness.importer.sweepIfNeeded()) + + #expect(flaky.migrateCalls == 2) + #expect(harness.accounts.accounts.map(\.id) == [account.id]) + #expect(harness.accounts.pendingUploads == [account.id]) + #expect(harness.storedAccounts(in: localScope).isEmpty) + } + + @Test("A refused account whose secret cannot be deleted is refused again rather than forgotten") + func aFailedTombstoneDeletionIsRetried() async throws { + let flaky = FlakyCredentialsManager() + let harness = Harness(startingScope: CredentialIdentityScope.localScope, credentialStore: flaky) + let deleted = UUID() + defer { harness.cleanUp(deleted) } + + harness.seed("identity-A") { $0.applyRemoteTombstone(deleted) } + harness.accounts.applySyncedAccount(Account(id: deleted, instanceID: UUID())) + + try await harness.flip(to: "identity-A") + + #expect(flaky.deleteCalls == 1) + #expect(harness.accounts.account(id: deleted) == nil) + #expect(harness.storedAccounts(in: localScope).map(\.id) == [deleted]) + + flaky.refusesEverything = false + #expect(harness.importer.sweepIfNeeded()) + + #expect(flaky.deleteCalls == 2) + #expect(harness.accounts.account(id: deleted) == nil) + #expect(harness.accounts.pendingUploads.isEmpty) + #expect(harness.storedAccounts(in: localScope).isEmpty) + } + + @Test("A login identifier with no credential beside it still moves before the record is pruned") + func aUsernameWithoutACredentialMoves() async throws { + let harness = Harness(startingScope: CredentialIdentityScope.localScope) + let account = harness.accounts.addAccount(instanceID: UUID(), displayName: "Never signed in") + harness.credentials.setUsername("btest1", for: account) + defer { harness.cleanUp(account.id) } + + try await harness.flip(to: "identity-A") + + #expect(harness.credentials.username(for: account) == "btest1") + #expect(harness.rawUsername(scope: localScope, accountID: account.id) == nil) + #expect(harness.storedAccounts(in: localScope).isEmpty) + } + + @Test("A redundant copy takes its login identifier with it") + func aRedundantCopyClearsTheLocalUsername() async throws { + let harness = Harness(startingScope: CredentialIdentityScope.localScope) + let accountID = UUID() + defer { harness.cleanUp(accountID) } + + harness.seed("identity-A") { $0.applySyncedAccount(Account(id: accountID, instanceID: UUID())) } + harness.seedCredential("identity-secret", accountID: accountID, scope: "identity-A") + harness.seedUsername("identity-login", accountID: accountID, scope: "identity-A") + + harness.accounts.applySyncedAccount(Account(id: accountID, instanceID: UUID())) + harness.seedCredential("local-secret", accountID: accountID, scope: localScope) + harness.seedUsername("local-login", accountID: accountID, scope: localScope) + + try await harness.flip(to: "identity-A") + + // The identity's own sign-in wins, and nothing of the redundant copy is + // left under the namespace being abandoned. + #expect(harness.rawCredential(scope: "identity-A", accountID: accountID) == "identity-secret") + #expect(harness.rawUsername(scope: "identity-A", accountID: accountID) == "identity-login") + #expect(harness.rawCredential(scope: localScope, accountID: accountID) == nil) + #expect(harness.rawUsername(scope: localScope, accountID: accountID) == nil) + #expect(harness.storedAccounts(in: localScope).isEmpty) + } + // MARK: - Context @Test("A context chosen while signed out is dropped when the identity has one") @@ -386,7 +480,10 @@ private final class Harness { private let store: KeychainCredentialStore - init(startingScope: String?) { + /// - Parameter credentialStore: replaces the real, Keychain-backed store the + /// importer is given, for the failures a live Keychain cannot be made to + /// produce on demand. + init(startingScope: String?, credentialStore substitute: (any AccountCredentialsManager)? = nil) { let defaults = UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! // Unique per run so a crashed run's leftovers can never be read back. let service = "com.yattee.tests.\(UUID().uuidString)" @@ -415,7 +512,7 @@ private final class Harness { self.importer = LocalNamespaceImporter( accountsManager: accounts, identityScope: scope, - credentialStores: { _ in [credentials] } + credentialStores: { _ in [substitute ?? credentials] } ) accounts.onNamespaceActivated = { [weak self] in @@ -464,6 +561,14 @@ private final class Harness { ) } + func seedUsername(_ value: String, accountID: UUID, scope: String) { + store.set( + value, + forKey: KeychainCredentialStore.usernameKey(scope: scope, accountID: accountID), + synchronizable: false + ) + } + // MARK: Reading storage directly func storedAccounts(in namespace: String) -> [Account] { @@ -545,7 +650,8 @@ private final class TestCredentialsManager: AccountCredentialsManager { credentials.migrateCredential(forAccountID: accountID, fromScope: sourceScope) } - func deleteCredential(forAccountID accountID: UUID, inScope scope: String) { + @discardableResult + func deleteCredential(forAccountID accountID: UUID, inScope scope: String) -> Bool { credentials.deleteCredential(forAccountID: accountID, inScope: scope) } @@ -562,3 +668,52 @@ private final class TestCredentialsManager: AccountCredentialsManager { credentials.deleteLegacyCredentialIfOwned(for: account) } } + +/// A credential store that can be told to refuse the two operations the sweep's +/// per-item atomicity rests on. +/// +/// The real Keychain cannot be made to fail a delete on demand — `SecItemDelete` +/// against a key that exists and is accessible succeeds — so the failure branch +/// is exercised through the protocol the importer actually talks to. What the +/// engine does when it *can* complete the move is +/// `AccountKeychainCredentials`' own tests' subject, driven against a live +/// Keychain by the rest of this suite. +@MainActor +@Observable +private final class FlakyCredentialsManager: AccountCredentialsManager { + var refusesEverything = true + private(set) var migrateCalls = 0 + private(set) var deleteCalls = 0 + + var loggedInAccountIDs: Set { [] } + var isScopeResolved: Bool { true } + + func setCredential(_: String, for _: Account) {} + func credential(for _: Account) -> String? { nil } + func deleteCredential(for _: Account) {} + func deleteCredential(forAccountID _: UUID) {} + func resyncCredentialsWithCurrentSetting() {} + func setUsername(_: String, for _: Account) {} + func username(for _: Account) -> String? { nil } + func isLoggedIn(for _: Account) -> Bool { false } + func refreshLoginStatus(for _: Account) {} + + @discardableResult + func migrateCredential(forAccountID _: UUID, fromScope _: String) -> Bool { + migrateCalls += 1 + return !refusesEverything + } + + @discardableResult + func deleteCredential(forAccountID _: UUID, inScope _: String) -> Bool { + deleteCalls += 1 + return !refusesEverything + } + + func hasAdoptableLegacyCredential(for _: Account) -> Bool { false } + + @discardableResult + func adoptLegacyCredential(for _: Account) -> Bool { false } + + func deleteLegacyCredentialIfOwned(for _: Account) {} +}