diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index 1fcc4588..b29f343c 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -1154,10 +1154,19 @@ final class AppEnvironment { // A rejection belongs to the credential that earned it. A // replacement has not been rejected by anyone. - if accountsManager.status(for: account.id) == .credentialRejected, !didReplaceCredential { + let wasRejected = accountsManager.status(for: account.id) == .credentialRejected + if wasRejected, !didReplaceCredential { continue } + if wasRejected { + LoggingService.shared.info( + "The rejected login was replaced — letting the account try again", + category: .keychain, + details: "accountID=\(account.id)" + ) + } + accountsManager.setStatus(.signedIn, for: account.id) } @@ -1165,19 +1174,29 @@ final class AppEnvironment { } /// Re-reads the Keychain for a couple of minutes while any account is - /// missing its credential. + /// waiting on one. /// /// iCloud Keychain delivers on its own schedule and announces nothing when /// it does, so an account that syncs ahead of its credential — the ordinary /// case on a new device — would sit at "sign in required", with reads /// falling back to local, until something unrelated happened to recompute - /// statuses. Stops as soon as nothing is waiting, and gives up rather than - /// polling forever: on tvOS, where app Keychain items never sync at all, - /// the wait is permanent by design. + /// statuses. + /// + /// A rejected account is waiting on the very same delivery: the user fixes + /// the login on one device, and this one goes on holding a secret the + /// server has already refused until the replacement lands. Nobody sitting + /// in front of that screen can hurry it along, and without this the account + /// stays red until something unrelated recomputes — which is how a + /// second device was measured still saying "sign in again" 41 minutes after + /// the login had been fixed. + /// + /// Only the Keychain is re-read; the server is asked nothing. A secret that + /// has not changed therefore costs one local lookup a tick and no request. + /// Stops as soon as nothing is waiting, and gives up rather than polling + /// forever: on tvOS, where app Keychain items never sync at all, the wait + /// is permanent by design. private func waitForArrivingCredentialsIfNeeded() { - let isWaiting = accountsManager.accounts.contains { accountsManager.status(for: $0.id) == .credentialMissing } - - guard isWaiting else { + guard isWaitingForCredential else { credentialArrivalTask?.cancel() credentialArrivalTask = nil return @@ -1188,29 +1207,28 @@ final class AppEnvironment { credentialArrivalTask = Task { [weak self] in for _ in 0 ..< 8 { try? await Task.sleep(for: .seconds(15)) - guard !Task.isCancelled, let self else { return } + guard !Task.isCancelled, let self, self.isWaitingForCredential else { break } - let stillWaiting = self.accountsManager.accounts.contains { - self.accountsManager.status(for: $0.id) == .credentialMissing - } - guard stillWaiting else { break } - - for account in self.accountsManager.accounts - where self.accountsManager.status(for: account.id) == .credentialMissing { - guard let credentials = self.accountCredentialsManager(for: account), - credentials.isScopeResolved else { continue } - - credentials.refreshLoginStatus(for: account) - if credentials.isLoggedIn(for: account) { - self.accountsManager.setStatus(.signedIn, for: account.id) - } - } + // The whole verdict rather than a shortened version of it: a + // rejection lifts only when the stored secret is a different + // one, and that comparison lives in there. + self.refreshAccountStatuses() } self?.credentialArrivalTask = nil } } + /// Whether any account is holding out for a credential iCloud Keychain has + /// yet to deliver — either none at all, or one to replace what the server + /// refused. + private var isWaitingForCredential: Bool { + accountsManager.accounts.contains { + let status = accountsManager.status(for: $0.id) + return status == .credentialMissing || status == .credentialRejected + } + } + // MARK: - Credentials Management /// Returns the appropriate credentials manager for an instance type. diff --git a/Yattee/Services/Credentials/KeychainCredentialStore.swift b/Yattee/Services/Credentials/KeychainCredentialStore.swift index 98dc8b96..27ca864a 100644 --- a/Yattee/Services/Credentials/KeychainCredentialStore.swift +++ b/Yattee/Services/Credentials/KeychainCredentialStore.swift @@ -48,29 +48,50 @@ struct KeychainCredentialStore: Sendable { // MARK: - Access - /// Reads a value, matching both synchronizable forms. + /// Reads a value, matching both synchronizable forms — the one that syncs + /// first. /// - /// `SynchronizableAny` is safe for scoped keys — the scope, not the sync - /// flag, is what provides identity isolation. + /// Matching both is safe for scoped keys: the scope, not the sync flag, is + /// what provides identity isolation. The *order* matters though, because + /// `kSecAttrSynchronizable` is part of a generic password's primary key, so + /// one key can hold two items at once. Legacy adoption writes a + /// device-local copy (`adoptLegacyCredential`), and a credential the user + /// signed in for on another device arrives beside it as a separate, + /// synchronizable item. A single `SynchronizableAny` match may answer with + /// either, and answering with the local copy is how a login already fixed + /// elsewhere stays invisible here — for as long as the copy exists. + /// + /// The synced item is the one somebody proved they hold, so it is asked for + /// first, and the local copy only answers when nothing arrived. func value(forKey key: String) -> String? { - let query: [String: Any] = [ + for synchronizable in [true, false] { + var query = itemQuery(key: key, synchronizable: synchronizable) + query[kSecReturnData as String] = true + query[kSecMatchLimit as String] = kSecMatchLimitOne + + var result: AnyObject? + let status = SecItemCopyMatching(query as CFDictionary, &result) + + guard status == errSecSuccess, + let data = result as? Data, + let value = String(data: data, encoding: .utf8) + else { continue } + + return value + } + + return nil + } + + /// The attributes identifying one item: class, service, key and the + /// synchronizable form. + private func itemQuery(key: String, synchronizable: Any) -> [String: Any] { + [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: service, kSecAttrAccount as String: key, - kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, - kSecReturnData as String: true, - kSecMatchLimit as String: kSecMatchLimitOne + kSecAttrSynchronizable as String: synchronizable ] - - var result: AnyObject? - let status = SecItemCopyMatching(query as CFDictionary, &result) - - guard status == errSecSuccess, - let data = result as? Data, - let value = String(data: data, encoding: .utf8) - else { return nil } - - return value } /// Writes a value **only if the key is currently empty**, returning @@ -81,13 +102,8 @@ struct KeychainCredentialStore: Sendable { /// replacing write would silently overwrite the newer 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) - ] + var query = itemQuery(key: key, synchronizable: synchronizable) + query[kSecValueData as String] = Data(value.utf8) return SecItemAdd(query as CFDictionary, nil) } @@ -100,13 +116,8 @@ struct KeychainCredentialStore: Sendable { /// account", which it cannot answer (measured). func isSynchronizable(key: String) -> Bool? { for synchronizable in [true, false] { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: key, - kSecAttrSynchronizable as String: synchronizable, - kSecMatchLimit as String: kSecMatchLimitOne - ] + var query = itemQuery(key: key, synchronizable: synchronizable) + query[kSecMatchLimit as String] = kSecMatchLimitOne if SecItemCopyMatching(query as CFDictionary, nil) == errSecSuccess { return synchronizable @@ -117,17 +128,20 @@ struct KeychainCredentialStore: Sendable { } /// Writes a value, replacing any existing item in either synchronizable form. + /// + /// Deliberately delete-then-add, and **not** `SecItemUpdate` on the item + /// already there. Measured on two devices sharing one Apple account: a + /// value written this way reached the other device in 15 s, while a value + /// changed in place with `SecItemUpdate` had still not arrived 40 minutes + /// later. Replacing the item is what iCloud Keychain propagates, and a + /// sign-in the user performs on one device is worth nothing to their other + /// ones unless it gets there. @discardableResult func set(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus { delete(key: key) - 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) - ] + var query = itemQuery(key: key, synchronizable: synchronizable) + query[kSecValueData as String] = Data(value.utf8) return SecItemAdd(query as CFDictionary, nil) } @@ -139,14 +153,9 @@ struct KeychainCredentialStore: Sendable { /// legacy item is never deleted as a side effect of adoption. @discardableResult func delete(key: String) -> OSStatus { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: key, - kSecAttrSynchronizable as String: kSecAttrSynchronizableAny - ] - - return SecItemDelete(query as CFDictionary) + SecItemDelete( + itemQuery(key: key, synchronizable: kSecAttrSynchronizableAny) as CFDictionary + ) } func describe(_ status: OSStatus) -> String { diff --git a/YatteeTests/AccountCredentialsTests.swift b/YatteeTests/AccountCredentialsTests.swift index 5a1ab533..d23597e9 100644 --- a/YatteeTests/AccountCredentialsTests.swift +++ b/YatteeTests/AccountCredentialsTests.swift @@ -7,6 +7,7 @@ import Testing import Foundation +import Security @testable import Yattee @Suite("Account credential scoping") @@ -610,6 +611,146 @@ struct LegacyCredentialAdoptionTests { } } +@Suite("Credential replacement across devices") +@MainActor +struct CredentialReplacementTests { + private func makeService() -> String { + "com.yattee.tests.\(UUID().uuidString)" + } + + private func makeMarkers() -> CredentialAdoptionMarkerStore { + CredentialAdoptionMarkerStore( + defaults: UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! + ) + } + + /// One item's attributes, asked for in exactly one synchronizable form. + /// + /// The store deliberately matches both, so a test that wants to know which + /// of the two answered has to ask for itself. + private func attributes(service: String, key: String, synchronizable: Bool) -> [String: Any]? { + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecAttrAccount as String: key, + kSecAttrSynchronizable as String: synchronizable, + kSecReturnAttributes as String: true, + kSecMatchLimit as String: kSecMatchLimitOne + ] + + var result: AnyObject? + guard SecItemCopyMatching(query as CFDictionary, &result) == errSecSuccess else { return nil } + return result as? [String: Any] + } + + @Test("A credential that arrived from another device wins over a device-local copy") + func synchronizableItemAnswersFirst() { + let service = makeService() + let store = KeychainCredentialStore(service: service) + let key = KeychainCredentialStore.credentialKey(scope: "identity-A", accountID: UUID()) + + // The sync flag is part of a generic password's primary key, so both of + // these land — neither write displaces the other. + #expect(store.addIfAbsent("kept-on-this-device", forKey: key, synchronizable: false) == errSecSuccess) + #expect(store.addIfAbsent("arrived-from-elsewhere", forKey: key, synchronizable: true) == errSecSuccess) + + #expect(store.value(forKey: key) == "arrived-from-elsewhere") + + store.delete(key: key) + } + + @Test("A device-local credential is still read when nothing has arrived") + func localItemAnswersWhenAlone() { + let service = makeService() + let store = KeychainCredentialStore(service: service) + let key = KeychainCredentialStore.credentialKey(scope: "identity-A", accountID: UUID()) + + #expect(store.addIfAbsent("kept-on-this-device", forKey: key, synchronizable: false) == errSecSuccess) + #expect(store.value(forKey: key) == "kept-on-this-device") + + store.delete(key: key) + } + + @Test("Replacing a credential writes a new item rather than editing the old one") + func replacementReplacesTheItem() { + let service = makeService() + let store = KeychainCredentialStore(service: service) + let key = KeychainCredentialStore.credentialKey(scope: "identity-A", accountID: UUID()) + + #expect(store.set("first-secret", forKey: key, synchronizable: true) == errSecSuccess) + let created = attributes(service: service, key: key, synchronizable: true)?[ + kSecAttrCreationDate as String + ] as? Date + + #expect(store.set("second-secret", forKey: key, synchronizable: true) == errSecSuccess) + #expect(store.value(forKey: key) == "second-secret") + + // A fresh item, not the old one edited. This is the difference between + // a sign-in the user's other devices receive and one they never hear + // about: an in-place `SecItemUpdate` was measured not to propagate + // through iCloud Keychain at all, while a replacement arrived in 15 s. + let replaced = attributes(service: service, key: key, synchronizable: true)?[ + kSecAttrCreationDate as String + ] as? Date + #expect(created != nil) + #expect(replaced != nil) + #expect(replaced != created) + + store.delete(key: key) + } + + @Test("Replacing a credential retires the twin in the other form") + func replacementRetiresTheOtherForm() { + let service = makeService() + let store = KeychainCredentialStore(service: service) + let key = KeychainCredentialStore.credentialKey(scope: "identity-A", accountID: UUID()) + + #expect(store.addIfAbsent("kept-on-this-device", forKey: key, synchronizable: false) == errSecSuccess) + #expect(store.set("signed-in-again", forKey: key, synchronizable: true) == errSecSuccess) + + #expect(attributes(service: service, key: key, synchronizable: false) == nil) + #expect(store.value(forKey: key) == "signed-in-again") + + store.delete(key: key) + } + + @Test("A sign-in on another device reaches an account that adopted its legacy login") + func adoptedCredentialYieldsToAnArrivingOne() { + let service = makeService() + let store = KeychainCredentialStore(service: service) + let instanceID = UUID() + let account = Account.migrated(from: instanceID) + + store.set( + "legacy-secret", + forKey: KeychainCredentialStore.legacyKey(instanceID: instanceID), + synchronizable: false + ) + + let credentials = AccountKeychainCredentials( + service: service, + credentialNoun: "credential", + identityScope: CredentialIdentityScope(fixedScope: "identity-A"), + markers: makeMarkers() + ) + #expect(credentials.adoptLegacyCredential(for: account) == true) + #expect(credentials.credential(for: account) == "legacy-secret") + + // The user signs in again on their other device. That credential + // arrives as a synchronizable item under the very key the adopted copy + // occupies, and the copy is device-local, so both sit there at once. + // Reading the stale one is how a rejection the user already fixed + // elsewhere would never lift here. + let key = KeychainCredentialStore.credentialKey(scope: "identity-A", accountID: account.id) + #expect(store.addIfAbsent("signed-in-elsewhere", forKey: key, synchronizable: true) == errSecSuccess) + + #expect(credentials.credential(for: account) == "signed-in-elsewhere") + + credentials.deleteCredential(for: account) + store.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) + } +} + @Suite("Credential identity scope resolution") @MainActor struct CredentialIdentityScopeTests {