diff --git a/Yattee/Core/InstancesManager.swift b/Yattee/Core/InstancesManager.swift index b0d4d418..ee35ed0e 100644 --- a/Yattee/Core/InstancesManager.swift +++ b/Yattee/Core/InstancesManager.swift @@ -98,8 +98,8 @@ final class InstancesManager { // Instances arriving from iCloud are as much a change as a local edit. // Without this, a legacy authenticated instance delivered here never - // reaches account migration — the account, and the action to adopt its - // saved login, would appear only after an unrelated edit or a relaunch. + // reaches account migration — the account, and the adoption of its + // saved login, would happen only after an unrelated edit or a relaunch. NotificationCenter.default.post(name: .instancesDidChange, object: nil) } diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index de4a5512..ad822424 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -92,6 +92,14 @@ final class AccountKeychainCredentials { loginsByScope[scope, default: []].insert(account.id) + // Whatever was adopted here is gone, replaced by a login the user just + // proved they hold. Leaving the marker would keep this account out of + // the resync that follows the iCloud toggle, so the new credential + // would stay on this device and every other one would show an account + // it never receives a login for. It would also let a deletion retire an + // unscoped item this account no longer has anything to do with. + markers.removeMarker(service: store.service, instanceID: account.instanceID, scope: scope) + LoggingService.shared.info( "Stored \(credentialNoun)", category: .keychain, @@ -131,10 +139,22 @@ final class AccountKeychainCredentials { let adopted = markers.adoptedAccountIDs(service: store.service, scope: scope) for accountID in loginsByScope[scope] ?? [] where !adopted.contains(accountID) { - rewrite( - key: KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID), - synchronizable: syncEnabled - ) + let credentialKey = KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID) + + // The marker is written *after* the copy it describes, so a launch + // that ends in between leaves an adopted credential no marker will + // ever claim — and adoption will not retry, because the key is no + // longer empty. Recognising the value itself closes that gap: a + // scoped credential identical to the unscoped item it could only + // have come from is an adopted copy, marker or not. Migrated + // accounts reuse their instance's UUID, which is what the legacy + // key is made of, and they are the only accounts allowed to adopt. + if let stored = store.value(forKey: credentialKey), + stored == store.value(forKey: KeychainCredentialStore.legacyKey(instanceID: accountID)) { + continue + } + + rewrite(key: credentialKey, synchronizable: syncEnabled) rewrite( key: KeychainCredentialStore.usernameKey(scope: scope, accountID: accountID), synchronizable: syncEnabled diff --git a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift index 9c0afc2e..0ce00fa8 100644 --- a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift +++ b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift @@ -60,8 +60,10 @@ final class CredentialAdoptionMarkerStore { /// Whether this identity has already adopted the legacy item for an instance. /// - /// Once true, the app stops offering adoption and stops reading the legacy - /// key entirely for that account. + /// Once true, the app stops adopting and stops reading the legacy key + /// entirely for that account. Cleared when the user signs in again: the + /// credential is then one they proved they hold, and nothing about the + /// adopted copy applies to it any more. func hasAdopted(service: String, instanceID: UUID, scope: String) -> Bool { marker(service: service, instanceID: instanceID, scope: scope) != nil } @@ -121,6 +123,11 @@ final class CredentialAdoptionMarkerStore { /// /// 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. + /// Since adoption became automatic this proves only that the device took + /// the secret, not that the identity signed in now originally owned it. + /// Deleting anyway is deliberate: the user is removing this login from + /// this device, which is exactly what the pre-revamp release did to the + /// same item on logout. Condition 4 is what carries the real weight now. /// 2. The marker belongs to the current identity — Apple account A's marker /// cannot authorize a deletion while B is signed in. /// 3. The marker belongs to the account being deleted. An instance can hold diff --git a/Yattee/Services/Credentials/CredentialIdentityScope.swift b/Yattee/Services/Credentials/CredentialIdentityScope.swift index 7fd350d3..325dd755 100644 --- a/Yattee/Services/Credentials/CredentialIdentityScope.swift +++ b/Yattee/Services/Credentials/CredentialIdentityScope.swift @@ -24,8 +24,10 @@ import Foundation /// keeps `kSecAttrSynchronizable = true` and still matches a strict /// `== true` query with **no** Apple account signed in — so the attribute /// cannot be used to tell "belongs to the current account" from "left over -/// from the previous one". Scoping the key is the only reliable defence, and -/// legacy items are never adopted automatically (see +/// from the previous one". Scoping the key is the only reliable defence. +/// A pre-revamp item is adopted into a scoped key automatically, but only as a +/// device-local copy, so an unproven secret never reaches the current +/// identity's iCloud Keychain (see /// ``AccountKeychainCredentials/adoptLegacyCredential(for:)``). /// /// ### Why the CloudKit user record, not the ubiquity token diff --git a/Yattee/Services/Credentials/KeychainCredentialStore.swift b/Yattee/Services/Credentials/KeychainCredentialStore.swift index 3e8749d5..98dc8b96 100644 --- a/Yattee/Services/Credentials/KeychainCredentialStore.swift +++ b/Yattee/Services/Credentials/KeychainCredentialStore.swift @@ -39,8 +39,9 @@ struct KeychainCredentialStore: Sendable { /// The pre-revamp key: a bare instance UUID, with no identity scope. /// - /// Only the adoption path may read this, and only when the user explicitly - /// asks for it. + /// Only the adoption path may read this. Ordinary credential reads never + /// fall back to it — an implicit read-through would leave no record of what + /// was taken, and nothing to bound a later deletion with. static func legacyKey(instanceID: UUID) -> String { instanceID.uuidString } @@ -75,9 +76,9 @@ struct KeychainCredentialStore: Sendable { /// 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. + /// Used by legacy adoption: iCloud Keychain may deliver the real scoped + /// credential while the migration is deciding to copy the older one, and a + /// replacing write would silently overwrite the newer secret. @discardableResult func addIfAbsent(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus { let query: [String: Any] = [ diff --git a/YatteeTests/AccountCredentialsTests.swift b/YatteeTests/AccountCredentialsTests.swift index 416d029d..ad11b1b2 100644 --- a/YatteeTests/AccountCredentialsTests.swift +++ b/YatteeTests/AccountCredentialsTests.swift @@ -281,6 +281,97 @@ struct LegacyCredentialAdoptionTests { store.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) } + @Test("An adopted credential stays local even if its marker was never written") + func resyncRecognizesAnAdoptedCopyWithoutItsMarker() { + let service = makeService() + let instanceID = UUID() + let account = Account.migrated(from: instanceID) + seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret") + + let settings = SettingsManager() + let previousSync = settings.iCloudSyncEnabled + let previousInstances = settings.syncInstances + settings.iCloudSyncEnabled = false + settings.syncInstances = false + defer { + settings.iCloudSyncEnabled = previousSync + settings.syncInstances = previousInstances + } + + let store = KeychainCredentialStore(service: service) + let key = KeychainCredentialStore.credentialKey(scope: "identity-A", accountID: account.id) + // Exactly the state left behind when the app dies between the adoption + // copy and the marker that describes it: the scoped item exists, no + // marker claims it, and adoption will not retry because the key is + // occupied. + store.set("legacy-secret", forKey: key, synchronizable: false) + + let credentials = makeCredentials( + service: service, + scope: "identity-A", + markers: makeMarkers(), + settingsManager: settings + ) + #expect(credentials.isLoggedIn(for: account) == true) + + settings.iCloudSyncEnabled = true + settings.syncInstances = true + credentials.resyncCredentialsWithCurrentSetting() + + #expect(store.isSynchronizable(key: key) == false) + + credentials.deleteCredential(for: account) + store.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) + } + + @Test("Signing in again lets the new credential sync") + func signingInAgainClearsTheAdoptionMarker() { + let service = makeService() + let instanceID = UUID() + let account = Account.migrated(from: instanceID) + seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret") + + let settings = SettingsManager() + let previousSync = settings.iCloudSyncEnabled + let previousInstances = settings.syncInstances + settings.iCloudSyncEnabled = false + settings.syncInstances = false + defer { + settings.iCloudSyncEnabled = previousSync + settings.syncInstances = previousInstances + } + + let markers = makeMarkers() + let credentials = makeCredentials( + service: service, + scope: "identity-A", + markers: markers, + settingsManager: settings + ) + #expect(credentials.adoptLegacyCredential(for: account) == true) + + // The user signs in again while sync is off, so the replacement is + // stored locally like any other credential. + credentials.setCredential("freshly-typed-secret", for: account) + #expect(markers.hasAdopted(service: service, instanceID: instanceID, scope: "identity-A") == false) + + // Turning sync on must now carry it to the other devices — they are + // showing an account whose credential never arrives. + settings.iCloudSyncEnabled = true + settings.syncInstances = true + credentials.resyncCredentialsWithCurrentSetting() + + let store = KeychainCredentialStore(service: service) + #expect( + store.isSynchronizable( + key: KeychainCredentialStore.credentialKey(scope: "identity-A", accountID: account.id) + ) == true + ) + + credentials.deleteCredential(for: account) + store.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) + } + @Test("A login the user added themselves never inherits the legacy item") func onlyTheMigratedAccountAdopts() { let service = makeService()