diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index 6568bbd0..13d81c97 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -1054,6 +1054,44 @@ final class AccountsManager { case deferred } + /// What this identity would do with a record from the local namespace, asked + /// without doing any of it. + enum ImportEligibility { + case adoptable + case rejectedByTombstone + /// The namespace moved under the question. Nothing may be decided. + case deferred + } + + /// Answers a record's fate without writing anything. + /// + /// Separate from `adoptImportedAccount` because the sweep has to know what + /// it is dealing with *before* it touches the Keychain: adopting announces + /// the record, and an announcement is enough to bring observers — status + /// refresh, and the legacy adoption it can trigger — down on an account + /// whose credential has not settled yet. + func importEligibility(for account: Account) -> ImportEligibility { + guard canImportIntoCurrentNamespace else { return .deferred } + + // Decision 5, first rule: this identity deleted the account, and the + // IDs are deterministic enough for that to be the same account rather + // than a coincidence — a migrated one reuses its instance's UUID. A + // stale local copy must never resurrect it. The instance's own deletion + // says the same thing about everything that lived on it, and arrives as + // its own permanent fact. + guard !deletedAccountIDs.contains(account.id), + !deletedInstanceIDs.contains(account.instanceID) else { + LoggingService.shared.info( + "Refusing to import an account this identity has deleted", + category: .general, + details: "accountID=\(account.id)" + ) + return .rejectedByTombstone + } + + return .adoptable + } + /// Whether the local namespace may be imported into what is loaded now. /// /// Three fences in one place. The first is the one `save()` uses: the scope @@ -1095,24 +1133,16 @@ final class AccountsManager { } /// Offers a record from the local namespace to the active identity. + /// + /// Only ever called once the record's credential is settled — the fate it + /// re-checks here is `importEligibility(for:)`'s, asked again because this + /// is the call that writes. @discardableResult func adoptImportedAccount(_ account: Account) -> ImportOutcome { - guard canImportIntoCurrentNamespace else { return .deferred } - - // Decision 5, first rule: this identity deleted the account, and the - // IDs are deterministic enough for that to be the same account rather - // than a coincidence — a migrated one reuses its instance's UUID. A - // stale local copy must never resurrect it. The instance's own deletion - // says the same thing about everything that lived on it, and arrives as - // its own permanent fact. - guard !deletedAccountIDs.contains(account.id), - !deletedInstanceIDs.contains(account.instanceID) else { - LoggingService.shared.info( - "Refusing to import an account this identity has deleted", - category: .general, - details: "accountID=\(account.id)" - ) - return .rejectedByTombstone + switch importEligibility(for: account) { + case .deferred: return .deferred + case .rejectedByTombstone: return .rejectedByTombstone + case .adoptable: break } // Decision 5, second rule: the identity's own copy stands. A strictly @@ -1152,30 +1182,50 @@ final class AccountsManager { func adoptImportedMigratedInstances(_ ids: Set) { guard canImportIntoCurrentNamespace else { return } - let added = ids.subtracting(migratedInstanceIDs) + // Only markers for instances this identity actually holds an account on + // — imported by the sweep that is running, or already here. The local + // set outlives the accounts it describes: deleting a migrated account + // while signed out leaves its marker behind (decisions 3 and 4 both keep + // the local namespace as it is), and copying that one over would say + // "this instance's legacy login is already an account" under an identity + // where it is not. Synthesis would be suppressed there forever, which is + // a deletion made while signed out leaking into an Apple account — the + // exact thing decision 3 refuses. + let added = ids + .subtracting(migratedInstanceIDs) + .filter { !accounts(forInstance: $0).isEmpty } guard !added.isEmpty else { return } migratedInstanceIDs.formUnion(added) - // A marker suppresses synthesis on every other device, so it may only be - // published for an instance whose account this identity actually holds. - // Publishing it for one whose account was refused would leave that - // instance's pre-revamp login unrepresented everywhere. - for instanceID in added where !accounts(forInstance: instanceID).isEmpty { - pendingMarkerUploads.insert(instanceID) - } + // A marker suppresses synthesis on every other device, so it is only + // published for an instance whose account this identity holds — which, + // after the filter above, is all of them. + pendingMarkerUploads.formUnion(added) save() } - /// Carries the local namespace's context over, if this identity has none. + /// Carries the local namespace's context over, if nothing better stands here. /// /// Epoch-stamped through the same rule the legacy-context migration uses: /// a selection made while signed out is a guess at what the user wants under /// this Apple account, so any explicit or synced choice outranks it whichever /// arrives first (decision 6). Held back from the server by the existing /// `canUploadContext` gate until the account it names is confirmed. + /// + /// "Nothing better" is the part that has to be judged rather than assumed. + /// `reloadForCurrentIdentity()` runs `migrateLegacyContextIfNeeded()` before + /// the sweep, so this identity can already hold an auto-derived context that + /// was never chosen by anyone — and refusing on the mere presence of one + /// would drop the user's own signed-out selection in favour of a guess, with + /// the local keys cleared behind it. Auto-derived contexts are recognisable: + /// they are stamped at the epoch, which is exactly the rank that says "any + /// real choice beats this". A repair that inherited an epoch stamp is + /// likewise a fallback rather than a choice, and is replaced on the same + /// terms. An explicit or synced selection is not epoch-stamped, and still + /// wins. func adoptImportedContext(_ stored: StoredDataContext) { guard canImportIntoCurrentNamespace else { return } - guard !hasStoredContext else { return } + guard !hasStoredContext || storedContext.isMigrated else { return } storedContext = .migrated(stored.context) hasStoredContext = true diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index 38b7cf08..19a261f4 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -244,7 +244,11 @@ final class AccountKeychainCredentials { // username still moves — it is what the sign-in sheet prefills — // and the record may only be pruned once that move is settled too. loginsByScope[sourceScope]?.remove(accountID) - return migrateUsername(forAccountID: accountID, from: sourceScope, to: scope) + let usernameSettled = migrateUsername(forAccountID: accountID, from: sourceScope, to: scope) + // Or a previous attempt moved the secret and died before the marker + // that describes where it went. + carryAdoptionProvenance(forAccountID: accountID, from: sourceScope, to: scope) + return usernameSettled } // Decision 5: a credential already stored under the current identity was @@ -256,6 +260,15 @@ final class AccountKeychainCredentials { loginsByScope[scope, default: []].insert(accountID) if sourceCleared { loginsByScope[sourceScope]?.remove(accountID) } + // "Already occupied" is not always somebody else's sign-in: it is + // also what a previous attempt at *this* move looks like when it + // died between the verified copy and the marker that records where + // the copy went. Left unrepaired, the next resync would read an + // unmarked credential as one the user proved they hold and upload + // an adopted secret into this identity's iCloud Keychain. The + // fingerprint tells the two apart. + carryAdoptionProvenance(forAccountID: accountID, from: sourceScope, to: scope) + guard usernameSettled, sourceCleared else { LoggingService.shared.error( "A redundant \(credentialNoun) could not be cleared out of the namespace being left", @@ -316,28 +329,10 @@ final class AccountKeychainCredentials { 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 - // it as this account's to retire. - markers.record( - service: store.service, - instanceID: accountID, - accountID: accountID, - scope: scope, - secret: secret - ) - markers.removeMarker( - service: store.service, - instanceID: accountID, - accountID: accountID, - scope: sourceScope - ) - } + // that follows it: the marker has to describe where the adopted copy + // now lives, or the deletion that retires the unscoped item would no + // longer recognise it as this account's to retire. + carryAdoptionProvenance(forAccountID: accountID, from: sourceScope, to: scope) loginsByScope[scope, default: []].insert(accountID) if sourceCleared { loginsByScope[sourceScope]?.remove(accountID) } @@ -364,6 +359,73 @@ final class AccountKeychainCredentials { return true } + /// Re-files an adoption marker under the scope its credential now lives in. + /// + /// A marker is the only thing that says a credential was *adopted* — copied + /// from the pre-revamp unscoped item rather than signed in for. That claim + /// has to follow the secret across a namespace move, because everything + /// downstream reads it: `resyncCredentialsWithCurrentSetting` leaves adopted + /// copies device-local instead of uploading a secret of unproven provenance + /// into an iCloud Keychain, and `canDeleteLegacyItem` needs it to recognise + /// the unscoped item as this account's to retire. + /// + /// Called from every path a migration can settle through, not just the one + /// that does the copying, because a move is several Keychain calls and any + /// of them can be the last one before the process dies. A retry then finds + /// the target already holding the secret and would otherwise report success + /// while the provenance stayed behind under a namespace nothing reads again. + /// + /// What makes that safe to repair blind is the fingerprint: the marker is + /// bound to one value, so a target credential that still fingerprints the + /// same is the adopted copy, and a different one is a real sign-in this + /// identity made — which the adopted copy was merely redundant beside, and + /// which nothing about the adoption applies to. + private func carryAdoptionProvenance(forAccountID accountID: UUID, from sourceScope: String, to scope: String) { + // Markers are keyed by instance, and only a migrated account — whose ID + // *is* its instance's — is ever allowed to adopt, so the account ID is + // the right instance to ask about. + guard markers.hasAdopted(service: store.service, instanceID: accountID, scope: sourceScope) else { return } + + let targetKey = KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID) + + if let stored = store.value(forKey: targetKey), + markers.hasAdopted( + service: store.service, + instanceID: accountID, + scope: sourceScope, + matching: stored + ) { + markers.record( + service: store.service, + instanceID: accountID, + accountID: accountID, + scope: scope, + secret: stored + ) + + // And the copy itself stays where adoption put it. A repair reached + // through a path that never chose the sync flag — or after a resync + // that ran while the marker was missing — could be looking at an + // item marked synchronizable, which is the one thing an adopted + // secret must not be. + rewrite(key: targetKey, synchronizable: false) + } + + // Retired only once the copy it described is really gone. While the + // source item survives — a deletion the Keychain refused — the marker + // is still the truth about it, and the sweep is coming back anyway. + guard store.value( + forKey: KeychainCredentialStore.credentialKey(scope: sourceScope, accountID: accountID) + ) == nil else { return } + + markers.removeMarker( + service: store.service, + instanceID: accountID, + accountID: accountID, + scope: sourceScope + ) + } + /// Moves the username item, which is a convenience rather than a secret. /// /// Counts towards the migration's success all the same. Not because a diff --git a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift index 7982e750..ca12c831 100644 --- a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift +++ b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift @@ -80,6 +80,20 @@ final class CredentialAdoptionMarkerStore { ) } + /// Whether the marker under `scope` describes exactly this value. + /// + /// For deciding what a credential found under another scope *is*. A marker + /// says a secret was adopted rather than signed in for, and the fingerprint + /// is what ties that claim to one value: a copy that still fingerprints the + /// same is the adopted one, and a different value is a real login the + /// marker has nothing to say about. + func hasAdopted(service: String, instanceID: UUID, scope: String, matching secret: String) -> Bool { + guard let marker = marker(service: service, instanceID: instanceID, scope: scope) else { + return false + } + return marker.secretFingerprint == fingerprint(of: secret) + } + private func marker(service: String, instanceID: UUID, scope: String) -> CredentialAdoptionMarker? { markers().first { $0.service == service && $0.instanceID == instanceID && $0.identityScope == scope diff --git a/Yattee/Services/Credentials/LocalNamespaceImporter.swift b/Yattee/Services/Credentials/LocalNamespaceImporter.swift index 6b7dbb08..9fa0cfa5 100644 --- a/Yattee/Services/Credentials/LocalNamespaceImporter.swift +++ b/Yattee/Services/Credentials/LocalNamespaceImporter.swift @@ -82,8 +82,34 @@ final class LocalNamespaceImporter { var pruneable: Set = [] var refused: Set = [] + // The credential is settled before the record is adopted, never the + // other way round, and both are needed twice over. + // + // Adopting announces: `.accountsDidChange` brings the status refresh — + // and the silent legacy adoption it can reach — down on the account + // synchronously, before this loop gets to its secret. That observer + // would find the identity's key empty and fill it with the pre-revamp + // unscoped item, an older secret of unproven provenance; the move that + // follows would then read its own target as occupied and delete the + // valid login as redundant. Nothing is announced here until there is + // nothing left to move. + // + // And a record adopted beside a credential that would not follow leaves + // the account owned by two namespaces at once. Signing into a second + // identity before the retry claims it there as well, and no later sweep + // can take it back out of the first — an account permanently in two + // Apple accounts. Settling first means a failure leaves *nothing* under + // the identity, so whoever signs in next simply imports it cleanly. + // + // Residual, and the reason this order rather than a transaction: a + // process death between the credential settling and the record being + // adopted strands the secret under the identity that was active. The + // record is still local, so it lands wherever the user signs in next and + // reads as sign-in-required there. Inconvenient, and recoverable by + // signing in again — never a silent corruption, which is what the other + // order risks. for account in snapshot.accounts { - switch accountsManager.adoptImportedAccount(account) { + switch accountsManager.importEligibility(for: account) { case .rejectedByTombstone: // Decision 5: the account was deliberately deleted under this // identity, so the local copy is not resurrected — and its @@ -112,13 +138,11 @@ final class LocalNamespaceImporter { ) } - case .imported, .mergedIntoExisting: + case .adoptable: // Per-item atomicity: the local record is only dropped once its - // credential is verifiably no longer owed by the local scope. A - // failure leaves the record in *both* namespaces until the next - // sweep — safe, because the collision rule then reads it as an - // account the identity already holds and only fills in what is - // missing. + // credential is verifiably no longer owed by the local scope, + // and it is only handed to the identity once the same is true. + // A failure leaves both where they are for the next sweep. // Every store is asked even after one fails: they hold separate // Keychain services, and whichever can complete its move should, // rather than waiting on a retry it has nothing to do with. @@ -128,14 +152,30 @@ final class LocalNamespaceImporter { moved = moved && storeMoved } - if moved { - pruneable.insert(account.id) - } else { + guard moved else { LoggingService.shared.warning( - "An imported account's credential could not be moved — leaving it for the next sweep", + "An account's credential could not be moved — leaving the record where it is", category: .keychain, details: "accountID=\(account.id)" ) + continue + } + + switch accountsManager.adoptImportedAccount(account) { + case .imported, .mergedIntoExisting: + pruneable.insert(account.id) + + case .rejectedByTombstone, .deferred: + // Neither is reachable: both answers come from the state + // this loop read a moment ago, and nothing between the two + // calls awaits or gives anything else a turn to change it. + // Left local all the same — a next sweep re-reads the fate + // and finishes the job whichever way it now reads. + LoggingService.shared.warning( + "An account's fate changed mid-import — leaving the record where it is", + category: .general, + details: "accountID=\(account.id)" + ) } case .deferred: diff --git a/YatteeTests/LocalNamespaceImportTests.swift b/YatteeTests/LocalNamespaceImportTests.swift index ddedc788..f16b6f13 100644 --- a/YatteeTests/LocalNamespaceImportTests.swift +++ b/YatteeTests/LocalNamespaceImportTests.swift @@ -171,16 +171,18 @@ struct LocalNamespaceImportTests { 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. + // The record stays where its secret is: entirely local, with the + // identity owning no part of it. Handing it over first would announce an + // account whose credential is still in another namespace — and leave it + // owned by two identities if the user signed into a second one before + // the retry. #expect(flaky.migrateCalls == 1) - #expect(harness.accounts.account(id: account.id)?.displayName == "Main") + #expect(harness.accounts.account(id: account.id) == nil) + #expect(harness.accounts.pendingUploads.isEmpty) + #expect(harness.storedAccounts(in: "identity-A").isEmpty) #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. + // The retry converges: with the secret settled, the record follows it. flaky.refusesEverything = false #expect(harness.importer.sweepIfNeeded()) @@ -190,6 +192,148 @@ struct LocalNamespaceImportTests { #expect(harness.storedAccounts(in: localScope).isEmpty) } + @Test("A settlement that fails leaves nothing for a second identity to collide with") + func aFailedSettlementNeverLeavesTheAccountInTwoIdentities() 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") + + // A holds nothing of it, so nothing of it is A's to keep. + #expect(harness.accounts.account(id: account.id) == nil) + #expect(harness.storedAccounts(in: "identity-A").isEmpty) + #expect(harness.storedAccounts(in: localScope).map(\.id) == [account.id]) + + // B signs in before A ever gets its retry. There is exactly one owner + // afterwards — the alternative, a record adopted beside a credential + // that would not follow, is an account permanently in two Apple + // accounts with nothing able to take it back out of the first. + flaky.refusesEverything = false + try await harness.flip(to: "identity-B") + + #expect(harness.accounts.account(id: account.id)?.displayName == "Main") + #expect(harness.storedAccounts(in: "identity-B").map(\.id) == [account.id]) + #expect(harness.storedAccounts(in: "identity-A").isEmpty) + #expect(harness.storedAccounts(in: localScope).isEmpty) + } + + // MARK: - Ordering + + @Test("An imported record is never announced before the credential that goes with it") + func nothingIsAnnouncedBeforeItsCredentialSettles() async throws { + let harness = Harness(startingScope: CredentialIdentityScope.localScope) + let account = harness.accounts.addAccount(instanceID: UUID(), displayName: "Main") + harness.credentials.setCredential("secret", for: account) + defer { harness.cleanUp(account.id) } + + // What this guards against is an observer of `.accountsDidChange` acting + // on a record the moment it appears — the status refresh, and the silent + // legacy adoption it can reach, which would fill the empty identity key + // with the pre-revamp unscoped secret and leave the real login to be + // deleted as redundant. Announcements are answered here on exactly those + // terms: synchronously, from inside the sweep, asking the only question + // such an observer would act on. + let witness = AnnouncementWitness() + let observer = NotificationCenter.default.addObserver( + forName: .accountsDidChange, + object: nil, + queue: nil + ) { _ in + MainActor.assumeIsolated { + guard harness.accounts.account(id: account.id) != nil else { return } + witness.samples.append(harness.rawCredential(scope: "identity-A", accountID: account.id)) + } + } + defer { NotificationCenter.default.removeObserver(observer) } + + try await harness.flip(to: "identity-A") + + // The record was announced, and the secret was already there every time. + #expect(!witness.samples.isEmpty) + #expect(witness.samples.allSatisfy { $0 == "secret" }) + #expect(harness.rawCredential(scope: localScope, accountID: account.id) == nil) + #expect(harness.storedAccounts(in: localScope).isEmpty) + } + + // MARK: - Provenance + + @Test("A move interrupted before its provenance was filed repairs it on the retry") + func adoptionProvenanceIsRepairedOnTheRetry() async throws { + let harness = Harness(startingScope: CredentialIdentityScope.localScope) + let instance = harness.addInstance(usesAccountLogin: true) + harness.accounts.reconcileLegacyAccounts() + let account = try #require(harness.accounts.accounts(forInstance: instance.id).first) + defer { harness.cleanUp(account.id, instance.id) } + + // What a process death between the verified copy and the marker leaves: + // the adopted secret is already under the identity, the namespace being + // left still holds its copy, and the only record of where that secret + // came from is filed under that namespace. + harness.seedCredential("legacy-secret", accountID: account.id, scope: localScope) + harness.seedCredential("legacy-secret", accountID: account.id, scope: "identity-A") + harness.markers.record( + service: harness.service, + instanceID: account.id, + accountID: account.id, + scope: localScope, + secret: "legacy-secret" + ) + + try await harness.flip(to: "identity-A") + + // The retry recognises its own interrupted work by the fingerprint and + // re-files the provenance where the secret now is. Without it the next + // resync would read an unmarked credential as one the user proved they + // hold, and upload a secret of unproven origin into this identity's + // iCloud Keychain. + #expect( + harness.markers.hasAdopted( + service: harness.service, + instanceID: account.id, + scope: "identity-A", + matching: "legacy-secret" + ) + ) + #expect(!harness.markers.hasAdopted(service: harness.service, instanceID: account.id, scope: localScope)) + #expect(harness.rawCredential(scope: "identity-A", accountID: account.id) == "legacy-secret") + #expect(harness.rawCredential(scope: localScope, accountID: account.id) == nil) + #expect(harness.storedAccounts(in: localScope).isEmpty) + } + + @Test("A real sign-in the adopted copy was redundant beside is not marked as adopted") + func aGenuineCredentialIsNotClaimedByTheMarker() async throws { + let harness = Harness(startingScope: CredentialIdentityScope.localScope) + let instance = harness.addInstance(usesAccountLogin: true) + harness.accounts.reconcileLegacyAccounts() + let account = try #require(harness.accounts.accounts(forInstance: instance.id).first) + defer { harness.cleanUp(account.id, instance.id) } + + // Same shape, different secret under the identity: this one was signed + // in for, and the adopted copy is genuinely redundant beside it. + harness.seedCredential("legacy-secret", accountID: account.id, scope: localScope) + harness.seedCredential("signed-in-secret", accountID: account.id, scope: "identity-A") + harness.markers.record( + service: harness.service, + instanceID: account.id, + accountID: account.id, + scope: localScope, + secret: "legacy-secret" + ) + + try await harness.flip(to: "identity-A") + + // Nothing about the adoption applies to a credential the user proved + // they hold — claiming it would keep a real login off iCloud Keychain + // forever. + #expect(!harness.markers.hasAdopted(service: harness.service, instanceID: account.id, scope: "identity-A")) + // ...and the marker for the copy that was deleted goes with it. + #expect(!harness.markers.hasAdopted(service: harness.service, instanceID: account.id, scope: localScope)) + #expect(harness.rawCredential(scope: "identity-A", accountID: account.id) == "signed-in-secret") + #expect(harness.rawCredential(scope: localScope, accountID: account.id) == nil) + } + @Test("A refused account whose secret cannot be deleted is refused again rather than forgotten") func aFailedTombstoneDeletionIsRetried() async throws { let flaky = FlakyCredentialsManager() @@ -256,6 +400,37 @@ struct LocalNamespaceImportTests { // MARK: - Context + @Test("A choice made while signed out beats a context the identity only derived") + func aCarriedChoiceBeatsAnAutoDerivedContext() async throws { + let harness = Harness(startingScope: CredentialIdentityScope.localScope) + let account = harness.accounts.addAccount(instanceID: UUID(), displayName: "Main") + defer { harness.cleanUp(account.id) } + + harness.accounts.setContext(.account(account.id)) + + // The pre-revamp selection is not namespaced by identity, so it is still + // readable when the identity's namespace loads — and + // `reloadForCurrentIdentity` turns it into a context *before* the sweep + // runs. Refusing to carry anything over on the strength of that would + // lose the user's own choice to a value nobody made, with the local keys + // cleared behind it. + harness.defaults.set( + try JSONEncoder().encode(SubscriptionAccount(type: .local, instanceID: nil)), + forKey: "subscriptionAccount" + ) + + try await harness.flip(to: "identity-A") + + #expect(harness.accounts.currentContext == .account(account.id)) + // Still epoch-stamped: a choice made while signed out is a guess at what + // the user wants under *this* Apple account, so a real one from any of + // their devices outranks it whichever arrives first. + #expect(harness.accounts.storedContext.isMigrated) + #expect(harness.accounts.pendingContextUpload) + #expect(harness.defaults.data(forKey: "accounts.local.context") == nil) + #expect(!harness.defaults.bool(forKey: "accounts.local.hasContext")) + } + @Test("A context chosen while signed out is dropped when the identity has one") func identityContextWins() async throws { let harness = Harness(startingScope: CredentialIdentityScope.localScope) @@ -268,6 +443,9 @@ struct LocalNamespaceImportTests { try await harness.flip(to: "identity-A") #expect(harness.accounts.currentContext == .local) + // An explicit choice under this identity is not epoch-stamped, and is + // exactly what the carried one must not displace. + #expect(!harness.accounts.storedContext.isMigrated) // The local keys are cleared either way, or every activation would offer // the same stale selection again. #expect(harness.defaults.data(forKey: "accounts.local.context") == nil) @@ -371,6 +549,35 @@ struct LocalNamespaceImportTests { #expect(harness.accounts.migratedInstanceIDs.contains(instance.id)) } + @Test("A marker for an account deleted while signed out is not copied over") + func markersWithoutAnAccountAreNotAdopted() async throws { + let harness = Harness(startingScope: CredentialIdentityScope.localScope) + let keptInstanceID = UUID() + let abandonedInstanceID = UUID() + + let kept = harness.accounts.addAccount(instanceID: keptInstanceID) + // Created and then deleted while signed out. Creating it marked its + // instance migrated, and the deletion deliberately leaves that marker + // standing — locally, where it belongs. + let doomed = harness.accounts.addAccount(instanceID: abandonedInstanceID) + harness.accounts.removeAccount(doomed.id) + defer { harness.cleanUp(kept.id, doomed.id) } + + #expect(harness.localMigratedInstanceIDs() == [keptInstanceID, abandonedInstanceID]) + + try await harness.flip(to: "identity-A") + + // A marker suppresses synthesis wherever it lands, so one whose account + // no longer exists would say "this instance's legacy login is already an + // account" under an identity where it is not — a deletion made while + // signed out leaking into an Apple account, which decision 3 refuses. + #expect(harness.accounts.migratedInstanceIDs == [keptInstanceID]) + #expect(harness.accounts.pendingMarkerUploads == [keptInstanceID]) + + // The local set itself is untouched either way (decision 4). + #expect(harness.localMigratedInstanceIDs() == [keptInstanceID, abandonedInstanceID]) + } + @Test("A deletion made while signed out is never published into the Apple account") func localDeletionFactsStayLocal() async throws { let harness = Harness(startingScope: CredentialIdentityScope.localScope) @@ -474,6 +681,12 @@ private final class Harness { let accounts: AccountsManager let credentials: TestCredentialsManager let importer: LocalNamespaceImporter + /// The provenance records that say a credential was adopted rather than + /// signed in for. Reachable so a test can leave one where an interrupted + /// move would have. + let markers: CredentialAdoptionMarkerStore + /// The Keychain service the markers are filed against. + let service: String /// Sweeps that actually took something over. private(set) var sweeps = 0 @@ -489,6 +702,7 @@ private final class Harness { let service = "com.yattee.tests.\(UUID().uuidString)" self.defaults = defaults + self.service = service self.store = KeychainCredentialStore(service: service) self.instances = makeTestInstancesManager() @@ -502,10 +716,13 @@ private final class Harness { ) self.accounts = accounts + let markers = CredentialAdoptionMarkerStore(defaults: defaults) + self.markers = markers + let credentials = TestCredentialsManager( service: service, identityScope: scope, - markers: CredentialAdoptionMarkerStore(defaults: defaults) + markers: markers ) self.credentials = credentials @@ -571,6 +788,11 @@ private final class Harness { // MARK: Reading storage directly + func localMigratedInstanceIDs() -> Set { + let stored = defaults.array(forKey: "accounts.local.migrated") as? [String] ?? [] + return Set(stored.compactMap(UUID.init(uuidString:))) + } + func storedAccounts(in namespace: String) -> [Account] { guard let data = defaults.data(forKey: "accounts.\(namespace).accounts"), let decoded = try? JSONDecoder().decode([Account].self, from: data) @@ -599,6 +821,15 @@ private final class Harness { } } +/// What the identity could see of a credential each time it announced a record. +/// +/// A reference type so the notification block can write to it without capturing +/// anything of the test's own state. +@MainActor +private final class AnnouncementWitness { + var samples: [String?] = [] +} + /// An account-keyed credential store over a Keychain service of its own. /// /// The shipping managers hard-code their service names, which under a test host