diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index 6308fb37..a34010cc 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -186,20 +186,46 @@ final class AccountKeychainCredentials { @discardableResult func adoptLegacyCredential(for account: Account) -> Bool { guard let scope = currentScope() else { return false } + guard !markers.hasAdopted(instanceID: account.instanceID, scope: scope) else { return false } guard let legacy = legacyCredential(for: account) else { return false } + // Eligibility is re-checked here, not just when the offer was rendered: + // iCloud Keychain may have delivered the real credential in between, and + // replacing it with the older legacy secret would be a silent downgrade. + guard credential(for: account) == nil else { + LoggingService.shared.info( + "Skipping legacy \(credentialNoun) adoption — a scoped credential already arrived", + category: .keychain, + details: "accountID=\(account.id)" + ) + return false + } + let key = KeychainCredentialStore.credentialKey(scope: scope, accountID: account.id) - let status = store.set(legacy, forKey: key, synchronizable: shouldSyncToiCloud) + let status = store.addIfAbsent(legacy, forKey: key, synchronizable: shouldSyncToiCloud) + + // A duplicate means a scoped credential landed between the check above + // and this write — leave it strictly alone, it is newer than the legacy + // secret. + guard status == errSecSuccess else { + LoggingService.shared.error( + "Legacy \(credentialNoun) adoption did not write", + category: .keychain, + details: "accountID=\(account.id), status=\(store.describe(status))" + ) + return false + } // 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 status == errSecSuccess, store.value(forKey: key) == legacy else { + guard store.value(forKey: key) == legacy else { LoggingService.shared.error( "Legacy \(credentialNoun) adoption failed verification", category: .keychain, - details: "accountID=\(account.id), status=\(store.describe(status))" + details: "accountID=\(account.id)" ) + // Safe to remove: this write is the item that just landed. store.delete(key: key) return false } @@ -229,13 +255,14 @@ final class AccountKeychainCredentials { guard markers.canDeleteLegacyItem( instanceID: account.instanceID, + accountID: account.id, scope: scope, currentLegacyValue: currentValue ) else { LoggingService.shared.info( - "Leaving the legacy \(credentialNoun) in place — not this identity's to delete", + "Leaving the legacy \(credentialNoun) in place — not this account's to delete", category: .keychain, - details: "instanceID=\(account.instanceID)" + details: "instanceID=\(account.instanceID), accountID=\(account.id)" ) return } diff --git a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift index b61e4886..975ebf51 100644 --- a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift +++ b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift @@ -87,23 +87,28 @@ final class CredentialAdoptionMarkerStore { // MARK: - Legacy deletion authority - /// Whether an account deletion may also delete the **unscoped** legacy item. + /// Whether deleting an account may also delete the **unscoped** legacy item. /// - /// Three conditions, all required: + /// Four conditions, all required: /// /// 1. This device has a marker for the instance — no marker means this /// device never adopted anything, so the legacy item is not ours to touch. /// 2. The marker belongs to the current identity — Apple account A's marker /// cannot authorize a deletion while B is signed in. - /// 3. The legacy item still holds the exact secret that was adopted — if it - /// has since been replaced (B signed in again on an older release, and - /// that release rewrote the unscoped key), it is someone else's live - /// login and must survive. + /// 3. The marker belongs to the account being deleted. An instance can hold + /// several accounts, and only the one that actually adopted the legacy + /// secret may retire it — deleting a *sibling* account must leave it + /// alone. + /// 4. The legacy item still holds the exact secret that was adopted — if it + /// has since been replaced (someone signed in again on an older release, + /// and that release rewrote the unscoped key), it is a live login of + /// theirs and must survive. /// /// Deleting a synchronizable item propagates everywhere, so a wrong answer /// here logs other devices out. func canDeleteLegacyItem( instanceID: UUID, + accountID: UUID, scope: String, currentLegacyValue: String? ) -> Bool { @@ -111,6 +116,7 @@ final class CredentialAdoptionMarkerStore { $0.instanceID == instanceID && $0.identityScope == scope }) else { return false } + guard marker.accountID == accountID else { return false } guard let currentLegacyValue else { return false } return fingerprint(of: currentLegacyValue) == marker.secretFingerprint diff --git a/Yattee/Services/Credentials/CredentialIdentityScope.swift b/Yattee/Services/Credentials/CredentialIdentityScope.swift index 0dcc4398..220dac45 100644 --- a/Yattee/Services/Credentials/CredentialIdentityScope.swift +++ b/Yattee/Services/Credentials/CredentialIdentityScope.swift @@ -58,9 +58,18 @@ final class CredentialIdentityScope { private let defaults: UserDefaults private let resolveUserRecordID: @Sendable () async -> String? + /// 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? private static let cacheKey = "credentialIdentityScopeCache" + /// Attempts made for the Apple account currently being resolved. Reset + /// whenever the identity changes. + private var resolutionAttempts = 0 + private var isResolving = false + private static let maxResolutionAttempts = 4 + /// The resolved scope, or `nil` while unresolved. var current: String? { if case let .resolved(scope) = state { return scope } @@ -76,6 +85,37 @@ final class CredentialIdentityScope { self.defaults = defaults self.resolveUserRecordID = resolveUserRecordID adoptCurrentIdentity() + observeIdentityChanges() + } + + deinit { + if let identityChangeObserver { + NotificationCenter.default.removeObserver(identityChangeObserver) + } + } + + /// Re-scopes as soon as the Apple account changes under a running app. + /// + /// Without this the scope would stay pinned to whichever account was + /// signed in at launch, and a sign-out or switch mid-session would let + /// reads and writes reach the previous account's retained credentials — + /// the exact leak scoping exists to prevent. + private func observeIdentityChanges() { + identityChangeObserver = NotificationCenter.default.addObserver( + forName: .NSUbiquityIdentityDidChange, + object: nil, + queue: .main + ) { [weak self] _ in + MainActor.assumeIsolated { + guard let self else { return } + LoggingService.shared.info( + "Apple account changed — re-resolving the credential identity scope", + category: .keychain + ) + self.adoptCurrentIdentity() + Task { await self.resolve() } + } + } } /// Test seam: a scope pinned up front, never consulting CloudKit. @@ -96,6 +136,9 @@ final class CredentialIdentityScope { /// closed shows up here, before any CloudKit request could report it. func adoptCurrentIdentity() { let current = CloudKitIdentity.ubiquityFingerprint + if current != fingerprint { + resolutionAttempts = 0 + } fingerprint = current guard CloudKitAvailability.isAvailable else { @@ -127,34 +170,67 @@ final class CredentialIdentityScope { /// rather than "not signed in" — callers must not treat that as a missing /// credential. func resolve() async { - guard !isResolved else { return } + guard !isResolved, !isResolving else { return } - // Captured before the await so a switch mid-flight can be detected: - // a record ID fetched under account A must never be cached for B. + isResolving = true + defer { isResolving = false } + + // 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. let requested = fingerprint - guard let recordID = await resolveUserRecordID() else { return } + while resolutionAttempts < Self.maxResolutionAttempts { + resolutionAttempts += 1 - guard CloudKitIdentity.ubiquityFingerprint == requested, fingerprint == requested else { - LoggingService.shared.info( - "Discarding credential scope resolved for a previous Apple account", - category: .keychain - ) - return + if let recordID = await resolveUserRecordID() { + guard CloudKitIdentity.ubiquityFingerprint == requested, fingerprint == requested else { + LoggingService.shared.info( + "Discarding credential scope resolved for a previous Apple account", + category: .keychain + ) + return + } + + if let requested { + cache(recordID, for: requested) + } + state = .resolved(recordID) + + LoggingService.shared.info( + "Resolved credential identity scope", + category: .keychain, + details: "fingerprint=\(requested ?? "none"), attempt=\(resolutionAttempts)" + ) + return + } + + guard resolutionAttempts < Self.maxResolutionAttempts else { break } + + try? await Task.sleep(for: .seconds(pow(2.0, Double(resolutionAttempts)))) + guard fingerprint == requested else { return } } - if let requested { - cache(recordID, for: requested) - } - state = .resolved(recordID) - - LoggingService.shared.info( - "Resolved credential identity scope", - category: .keychain, - details: "fingerprint=\(requested ?? "none")" + // Every credential read is disabled until this succeeds, so failing + // quietly would look exactly like "signed out". Foreground activation + // retries via resolveIfNeeded(). + LoggingService.shared.warning( + "Could not resolve the credential identity scope — credentials stay unavailable until it does", + category: .keychain ) } + /// Retries a resolution that previously ran out of attempts. + /// + /// Called when the app returns to the foreground: a device that launched + /// offline would otherwise keep credentials unavailable for the entire + /// process lifetime, long after connectivity returned. + func resolveIfNeeded() async { + guard !isResolved else { return } + resolutionAttempts = 0 + await resolve() + } + // MARK: - Cache private func cachedScope(for fingerprint: String) -> String? { diff --git a/Yattee/Services/Credentials/KeychainCredentialStore.swift b/Yattee/Services/Credentials/KeychainCredentialStore.swift index 02d83f52..0f80b55e 100644 --- a/Yattee/Services/Credentials/KeychainCredentialStore.swift +++ b/Yattee/Services/Credentials/KeychainCredentialStore.swift @@ -72,6 +72,25 @@ struct KeychainCredentialStore: Sendable { return value } + /// Writes a value **only if the key is currently empty**, returning + /// `errSecDuplicateItem` otherwise. + /// + /// Used by legacy adoption: between rendering the offer and the user + /// tapping it, iCloud Keychain may deliver the real scoped credential, and + /// a replacing write would silently overwrite it with the older secret. + @discardableResult + func addIfAbsent(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus { + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecAttrAccount as String: key, + kSecAttrSynchronizable as String: synchronizable, + kSecValueData as String: Data(value.utf8) + ] + + return SecItemAdd(query as CFDictionary, nil) + } + /// Writes a value, replacing any existing item in either synchronizable form. @discardableResult func set(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus { diff --git a/Yattee/YatteeApp.swift b/Yattee/YatteeApp.swift index bc7846c8..d68f10cd 100644 --- a/Yattee/YatteeApp.swift +++ b/Yattee/YatteeApp.swift @@ -288,6 +288,12 @@ struct YatteeApp: App { // Refresh media source password status (Keychain state can change in background) appEnvironment.mediaSourcesManager.refreshPasswordStoredStatus() + // Retry a credential identity scope that never resolved (e.g. + // launched offline) - account credentials stay unavailable until it does + Task { + await appEnvironment.credentialIdentityScope.resolveIfNeeded() + } + // Validate subscription account (auto-correct if invalid) appEnvironment.subscriptionAccountValidator.validateAndCorrectIfNeeded() SubscriptionFeedCache.shared.warmIfNeeded(using: appEnvironment) @@ -335,6 +341,12 @@ struct YatteeApp: App { // Refresh media source password status (Keychain state can change in background) appEnvironment.mediaSourcesManager.refreshPasswordStoredStatus() + // Retry a credential identity scope that never resolved (e.g. + // launched offline) - account credentials stay unavailable until it does + Task { + await appEnvironment.credentialIdentityScope.resolveIfNeeded() + } + // Validate subscription account (auto-correct if invalid) appEnvironment.subscriptionAccountValidator.validateAndCorrectIfNeeded() SubscriptionFeedCache.shared.warmIfNeeded(using: appEnvironment) diff --git a/YatteeTests/AccountCredentialsTests.swift b/YatteeTests/AccountCredentialsTests.swift index a4bbd453..c471dc46 100644 --- a/YatteeTests/AccountCredentialsTests.swift +++ b/YatteeTests/AccountCredentialsTests.swift @@ -253,6 +253,51 @@ struct LegacyCredentialAdoptionTests { identityA.deleteCredential(for: account) } + @Test("Deleting a sibling account leaves the adopter's legacy item alone") + func legacyDeletionRequiresAccountMatch() { + let service = makeService() + let markers = makeMarkers() + let instanceID = UUID() + let adopter = Account.migrated(from: instanceID) + // A second login on the same instance — a fresh UUID, not the migrated one. + let sibling = Account(instanceID: instanceID) + seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret") + + let credentials = makeCredentials(service: service, scope: "identity-A", markers: markers) + #expect(credentials.adoptLegacyCredential(for: adopter) == true) + + // Removing the sibling must not retire the secret the migrated account + // adopted — old-release devices are still using it. + credentials.deleteLegacyCredentialIfOwned(for: sibling) + #expect(legacyValue(service: service, instanceID: instanceID) == "legacy-secret") + + credentials.deleteLegacyCredentialIfOwned(for: adopter) + #expect(legacyValue(service: service, instanceID: instanceID) == nil) + + credentials.deleteCredential(for: adopter) + } + + @Test("Adoption does not overwrite a scoped credential that arrived first") + func adoptionDoesNotClobberArrivedCredential() { + let service = makeService() + let instanceID = UUID() + let account = Account.migrated(from: instanceID) + seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret") + + let credentials = makeCredentials(service: service, scope: "identity-A", markers: makeMarkers()) + // iCloud Keychain delivers the real credential between the offer being + // rendered and the user tapping it. + credentials.setCredential("synced-secret", for: account) + + #expect(credentials.hasAdoptableLegacyCredential(for: account) == false) + #expect(credentials.adoptLegacyCredential(for: account) == false) + #expect(credentials.credential(for: account) == "synced-secret") + + credentials.deleteCredential(for: account) + KeychainCredentialStore(service: service) + .delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) + } + @Test("A legacy item replaced by another login survives the adopter's deletion") func legacyDeletionRequiresFingerprintMatch() { let service = makeService()