From f5e88df403b757a64f3446f307ff51a0d245209e Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Sun, 23 Aug 2026 23:56:28 +0200 Subject: [PATCH] Stop adoption rollback from revoking a synced credential Two more issues from a second review pass. Adoption verified its write by reading the key back and deleted it on any mismatch. But a mismatch can also mean iCloud Keychain delivered a real credential for that key between the add and the read - and since the delete spans both synchronizable forms, it would propagate and revoke that credential on every device. A readable value that is not ours is now left strictly alone; only an unreadable key (the add reported success but produced nothing usable) is rolled back so the offer can be retried. An identity switch arriving while a resolution was in flight was dropped: the observer's resolve() returned early on the isResolving guard, and the in-flight one then bailed on its fingerprint check without starting a lookup for the new identity, leaving credentials disabled until a foreground transition or relaunch. Requests that arrive mid-resolution are now replayed once the running one finishes. Adds a fingerprint provider and an injectable backoff so identity resolution is testable without a real Apple account or real seconds, plus tests for retry, foreground recovery, mid-flight switching, signed-out scoping and the cached-scope fast path. --- .../AccountKeychainCredentials.swift | 22 ++- .../Credentials/CredentialIdentityScope.swift | 42 +++++- YatteeTests/AccountCredentialsTests.swift | 138 ++++++++++++++++++ 3 files changed, 195 insertions(+), 7 deletions(-) diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index a34010cc..eef397b1 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -219,13 +219,31 @@ final class AccountKeychainCredentials { // The marker is only written after the scoped copy reads back: a marker // for a credential that never landed would suppress the adoption offer // and strand the user with no way to sign in short of retyping. - guard store.value(forKey: key) == legacy else { + let readback = store.value(forKey: key) + + guard readback == legacy else { + // A *different* readable value means iCloud Keychain delivered a + // credential for this key between the add and the read. It is newer + // than the legacy secret and belongs to a live login, so it is left + // strictly alone — deleting here would propagate through iCloud + // Keychain and revoke it on every device. + if readback != nil { + LoggingService.shared.info( + "Legacy \(credentialNoun) adoption superseded by a credential that arrived mid-write", + category: .keychain, + details: "accountID=\(account.id)" + ) + return false + } + + // Nothing readable is there: the add reported success but produced + // no usable item, so rolling it back destroys nothing and lets the + // adoption offer be retried. LoggingService.shared.error( "Legacy \(credentialNoun) adoption failed verification", category: .keychain, details: "accountID=\(account.id)" ) - // Safe to remove: this write is the item that just landed. store.delete(key: key) return false } diff --git a/Yattee/Services/Credentials/CredentialIdentityScope.swift b/Yattee/Services/Credentials/CredentialIdentityScope.swift index 220dac45..56dc540f 100644 --- a/Yattee/Services/Credentials/CredentialIdentityScope.swift +++ b/Yattee/Services/Credentials/CredentialIdentityScope.swift @@ -58,6 +58,9 @@ final class CredentialIdentityScope { private let defaults: UserDefaults private let resolveUserRecordID: @Sendable () async -> String? + private let fingerprintProvider: @Sendable () -> String? + /// Backoff between attempts. Injectable so tests don't pay real seconds. + private let retryDelay: @Sendable (Int) async -> Void /// Written only on the main actor; read in `deinit`, which runs after the /// last reference is gone and so cannot race with it. private nonisolated(unsafe) var identityChangeObserver: NSObjectProtocol? @@ -68,6 +71,10 @@ final class CredentialIdentityScope { /// whenever the identity changes. private var resolutionAttempts = 0 private var isResolving = false + /// Set when a resolution is requested while one is already running — the + /// running one is for a now-stale identity and will bail on the fingerprint + /// check, so the request has to be replayed rather than dropped. + private var pendingResolve = false private static let maxResolutionAttempts = 4 /// The resolved scope, or `nil` while unresolved. @@ -80,10 +87,16 @@ final class CredentialIdentityScope { init( defaults: UserDefaults = .standard, - resolveUserRecordID: @escaping @Sendable () async -> String? = { await CloudKitIdentity.userRecordID() } + fingerprintProvider: @escaping @Sendable () -> String? = { CloudKitIdentity.ubiquityFingerprint }, + resolveUserRecordID: @escaping @Sendable () async -> String? = { await CloudKitIdentity.userRecordID() }, + retryDelay: @escaping @Sendable (Int) async -> Void = { attempt in + try? await Task.sleep(for: .seconds(pow(2.0, Double(attempt)))) + } ) { self.defaults = defaults + self.fingerprintProvider = fingerprintProvider self.resolveUserRecordID = resolveUserRecordID + self.retryDelay = retryDelay adoptCurrentIdentity() observeIdentityChanges() } @@ -122,7 +135,9 @@ final class CredentialIdentityScope { /// `nil` pins the unresolved state. init(fixedScope: String?, defaults: UserDefaults = .standard) { self.defaults = defaults + fingerprintProvider = { nil } resolveUserRecordID = { nil } + retryDelay = { _ in } fingerprint = nil state = fixedScope.map { State.resolved($0) } ?? .unresolved } @@ -135,7 +150,7 @@ final class CredentialIdentityScope { /// Call again on `.accountChange`: a switch that happened while the app was /// closed shows up here, before any CloudKit request could report it. func adoptCurrentIdentity() { - let current = CloudKitIdentity.ubiquityFingerprint + let current = fingerprintProvider() if current != fingerprint { resolutionAttempts = 0 } @@ -170,11 +185,28 @@ final class CredentialIdentityScope { /// rather than "not signed in" — callers must not treat that as a missing /// credential. func resolve() async { - guard !isResolved, !isResolving else { return } + guard !isResolved else { return } + + // A resolution already running is for whatever identity was current + // when it started. If that is no longer this one it will bail on its + // fingerprint check, so the request has to be replayed afterwards + // rather than dropped — otherwise a switch mid-flight leaves the new + // identity unresolved until a foreground transition or relaunch. + guard !isResolving else { + pendingResolve = true + return + } isResolving = true defer { isResolving = false } + repeat { + pendingResolve = false + await performResolution() + } while pendingResolve && !isResolved + } + + private func performResolution() async { // Captured before the first await so a switch mid-flight can be // detected: a record ID fetched under account A must never be cached // for B. @@ -184,7 +216,7 @@ final class CredentialIdentityScope { resolutionAttempts += 1 if let recordID = await resolveUserRecordID() { - guard CloudKitIdentity.ubiquityFingerprint == requested, fingerprint == requested else { + guard fingerprintProvider() == requested, fingerprint == requested else { LoggingService.shared.info( "Discarding credential scope resolved for a previous Apple account", category: .keychain @@ -207,7 +239,7 @@ final class CredentialIdentityScope { guard resolutionAttempts < Self.maxResolutionAttempts else { break } - try? await Task.sleep(for: .seconds(pow(2.0, Double(resolutionAttempts)))) + await retryDelay(resolutionAttempts) guard fingerprint == requested else { return } } diff --git a/YatteeTests/AccountCredentialsTests.swift b/YatteeTests/AccountCredentialsTests.swift index c471dc46..c2121236 100644 --- a/YatteeTests/AccountCredentialsTests.swift +++ b/YatteeTests/AccountCredentialsTests.swift @@ -343,6 +343,144 @@ struct LegacyCredentialAdoptionTests { } } +@Suite("Credential identity scope resolution") +@MainActor +struct CredentialIdentityScopeTests { + private func makeDefaults() -> UserDefaults { + UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! + } + + /// Mutable identity + resolver so a switch can be staged mid-resolution. + private final class Harness: @unchecked Sendable { + var fingerprint: String? = "identity-A" + var resultsByFingerprint: [String: String] = [:] + var failuresRemaining = 0 + var onResolveStart: (() -> Void)? + var switchTask: Task? + + func resolve() async -> String? { + onResolveStart?() + if failuresRemaining > 0 { + failuresRemaining -= 1 + return nil + } + guard let fingerprint else { return nil } + return resultsByFingerprint[fingerprint] + } + } + + @Test("A transient failure is retried rather than disabling credentials forever") + func transientFailureIsRetried() async { + let harness = Harness() + harness.resultsByFingerprint = ["identity-A": "_recordA"] + harness.failuresRemaining = 2 + + let scope = CredentialIdentityScope( + defaults: makeDefaults(), + fingerprintProvider: { harness.fingerprint }, + resolveUserRecordID: { await harness.resolve() }, + retryDelay: { _ in } + ) + + #expect(scope.isResolved == false) + await scope.resolve() + #expect(scope.current == "_recordA") + } + + @Test("Exhausted attempts are retried on foreground activation") + func exhaustedAttemptsRetryOnForeground() async { + let harness = Harness() + harness.resultsByFingerprint = ["identity-A": "_recordA"] + harness.failuresRemaining = 99 // every attempt fails + + let scope = CredentialIdentityScope( + defaults: makeDefaults(), + fingerprintProvider: { harness.fingerprint }, + resolveUserRecordID: { await harness.resolve() }, + retryDelay: { _ in } + ) + + await scope.resolve() + #expect(scope.isResolved == false) + + // Connectivity returns; the app comes back to the foreground. + harness.failuresRemaining = 0 + await scope.resolveIfNeeded() + #expect(scope.current == "_recordA") + } + + @Test("An identity switch mid-resolution resolves the new identity") + func switchDuringResolutionResolvesNewIdentity() async { + let harness = Harness() + harness.resultsByFingerprint = ["identity-A": "_recordA", "identity-B": "_recordB"] + + let scope = CredentialIdentityScope( + defaults: makeDefaults(), + fingerprintProvider: { harness.fingerprint }, + resolveUserRecordID: { await harness.resolve() }, + retryDelay: { _ in } + ) + + // The Apple account switches while A's lookup is in flight — exactly + // what the notification observer does, only deterministically. + harness.onResolveStart = { [weak scope] in + harness.onResolveStart = nil + harness.fingerprint = "identity-B" + MainActor.assumeIsolated { + scope?.adoptCurrentIdentity() + harness.switchTask = Task { await scope?.resolve() } + } + } + + await scope.resolve() + await harness.switchTask?.value + + // A's result must be discarded, and B must not be left unresolved. + #expect(scope.current != "_recordA") + #expect(scope.current == "_recordB") + } + + @Test("Signing out scopes to local without a lookup") + func signedOutUsesLocalScope() { + let harness = Harness() + harness.fingerprint = nil + + let scope = CredentialIdentityScope( + defaults: makeDefaults(), + fingerprintProvider: { harness.fingerprint }, + resolveUserRecordID: { await harness.resolve() }, + retryDelay: { _ in } + ) + + #expect(scope.current == CredentialIdentityScope.localScope) + } + + @Test("A known Apple account resolves synchronously on relaunch") + func cachedScopeResolvesSynchronously() async { + let defaults = makeDefaults() + let harness = Harness() + harness.resultsByFingerprint = ["identity-A": "_recordA"] + + let first = CredentialIdentityScope( + defaults: defaults, + fingerprintProvider: { harness.fingerprint }, + resolveUserRecordID: { await harness.resolve() }, + retryDelay: { _ in } + ) + await first.resolve() + #expect(first.current == "_recordA") + + // Relaunch: no network involved this time. + let second = CredentialIdentityScope( + defaults: defaults, + fingerprintProvider: { harness.fingerprint }, + resolveUserRecordID: { nil }, + retryDelay: { _ in } + ) + #expect(second.current == "_recordA") + } +} + @Suite("Migrated account identity") @MainActor struct MigratedAccountTests {