diff --git a/Sources/App/AppMain.swift b/Sources/App/AppMain.swift index 38e29e1..d8a4ee1 100644 --- a/Sources/App/AppMain.swift +++ b/Sources/App/AppMain.swift @@ -19,15 +19,6 @@ struct AppMain: App { registry.register(DeepSeekProvider()) registry.register(OpenAICodexProvider()) registry.register(CursorProvider()) - do { - try LegacyBrandMigration.migratePreferences( - providerIds: registry.registeredProviders.map(\.id) - ) - } catch { - FileHandle.standardError.write( - Data("[Filbert] Preference migration failed: \(error.localizedDescription)\n".utf8) - ) - } _viewModel = State(initialValue: QuotaViewModel(registry: registry)) } diff --git a/Sources/Core/Keychain.swift b/Sources/Core/Keychain.swift index fab1aec..d321008 100644 --- a/Sources/Core/Keychain.swift +++ b/Sources/Core/Keychain.swift @@ -20,13 +20,9 @@ public final class Keychain: @unchecked Sendable { public static let shared = Keychain() private let service: String - private let previousService: String? private let storage: any KeychainStorage /// Account under which the consolidated JSON blob is stored. private let account = "providers" - /// Prefix of the pre-consolidation per-provider accounts, read only during - /// the one-time migration below. - private let legacyAccountPrefix = "provider-" /// Decoded provider-ID → provider-owned secret-field map. `nil` until the /// first keychain read; an empty dictionary is a valid loaded state. @@ -45,19 +41,16 @@ public final class Keychain: @unchecked Sendable { private convenience init() { self.init( storage: SecurityKeychainStorage(), - service: "filbert", - previousService: LegacyBrandIdentifiers.keychainService + service: "filbert" ) } init( storage: any KeychainStorage, - service: String, - previousService: String? + service: String ) { self.storage = storage self.service = service - self.previousService = previousService } public func save(_ key: String, for providerId: String) throws { @@ -108,8 +101,7 @@ public final class Keychain: @unchecked Sendable { private extension Keychain { // MARK: - Store access (all callers hold `lock`) - /// Returns the cached store, loading it from the keychain — migrating any - /// legacy per-provider items — on first use. + /// Returns the cached store, loading it from the keychain on first use. private func loadedStore( authenticationContext: KeychainAuthenticationContext ) throws -> [String: [String: String]] { @@ -141,28 +133,19 @@ private extension Keychain { private func readStore( authenticationContext: KeychainAuthenticationContext ) throws -> LoadedStore { - let current: ConsolidatedStore? do { - if let data = try storage.readData( + guard let data = try storage.readData( service: service, account: account, authenticationContext: authenticationContext - ) { - current = try decodeStore(data, error: .loadFailed(errSecDecode)) - } else { - current = nil + ) else { + return LoadedStore(store: [:], data: nil) } + let store = try decodeStore(data) + return LoadedStore(store: store, data: data) } catch let error as KeychainStorageError { throw KeychainError.loadFailed(error.status) } - - if let current, !current.isLegacy { - return LoadedStore(store: current.store, data: current.data) - } - return try migrateStore( - current: current, - authenticationContext: authenticationContext - ) } private func writeStore( @@ -180,99 +163,6 @@ private extension Keychain { return data } - private func migrateStore( - current: ConsolidatedStore?, - authenticationContext: KeychainAuthenticationContext - ) throws -> LoadedStore { - var migrated: [String: [String: String]] = [:] - var itemsToDelete: [(service: String, account: String)] = [] - var requiresWrite = current?.isLegacy ?? false - - if let previousService { - let previousHasItems = try mergeMigrationItems( - service: previousService, - into: &migrated, - itemsToDelete: &itemsToDelete, - authenticationContext: authenticationContext - ) - requiresWrite = requiresWrite || previousHasItems - - if let consolidated = try legacyConsolidatedStore( - service: previousService, - authenticationContext: authenticationContext - ) { - migrated.merge(consolidated.store) { _, latest in latest } - itemsToDelete.append((previousService, account)) - requiresWrite = true - } - } - - let currentHasItems = try mergeMigrationItems( - service: service, - into: &migrated, - itemsToDelete: &itemsToDelete, - authenticationContext: authenticationContext - ) - requiresWrite = requiresWrite || currentHasItems - - if let current { - migrated.merge(current.store) { _, latest in latest } - } - - guard requiresWrite else { - return LoadedStore(store: current?.store ?? [:], data: current?.data) - } - - let data = try JSONEncoder().encode(migrated) - try replaceAndVerify( - data, - previousData: current?.data, - authenticationContext: authenticationContext, - errorFactory: { .migrationFailed($0) } - ) - for item in itemsToDelete { - storage.delete( - service: item.service, - account: item.account, - authenticationContext: authenticationContext - ) - } - return LoadedStore(store: migrated, data: data) - } - - private func mergeMigrationItems( - service: String, - into migrated: inout [String: [String: String]], - itemsToDelete: inout [(service: String, account: String)], - authenticationContext: KeychainAuthenticationContext - ) throws -> Bool { - let items = try migrationItems( - service: service, - authenticationContext: authenticationContext - ) - migrated.merge(items.store) { _, latest in latest } - itemsToDelete.append(contentsOf: items.accounts.map { (service, $0) }) - return !items.accounts.isEmpty - } - - private func legacyConsolidatedStore( - service: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> ConsolidatedStore? { - do { - guard let data = try storage.readData( - service: service, - account: account, - authenticationContext: authenticationContext - ) else { - return nil - } - return try decodeStore(data, error: .migrationFailed(errSecDecode)) - } catch let error as KeychainStorageError { - throw KeychainError.migrationFailed(error.status) - } - } - private func replaceAndVerify( _ data: Data, previousData: Data?, @@ -330,53 +220,15 @@ private extension Keychain { } } - private func migrationItems( - service: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> (store: [String: [String: String]], accounts: [String]) { - let items: [StoredKeychainItem] + private func decodeStore(_ data: Data) throws -> [String: [String: String]] { do { - items = try storage.readLegacyItems( - service: service, - accountPrefix: legacyAccountPrefix, - authenticationContext: authenticationContext - ) - } catch let error as KeychainStorageError { - throw KeychainError.migrationFailed(error.status) - } - - var migrated: [String: [String: String]] = [:] - var accounts: [String] = [] - for item in items where item.account.hasPrefix(legacyAccountPrefix) { - guard let key = String(data: item.data, encoding: .utf8) else { continue } - let providerId = String(item.account.dropFirst(legacyAccountPrefix.count)) - migrated[providerId] = ["value": key] - accounts.append(item.account) - } - return (migrated, accounts) - } - - private func decodeStore( - _ data: Data, - error: KeychainError - ) throws -> ConsolidatedStore { - if let store = try? JSONDecoder().decode([String: [String: String]].self, from: data) { - return ConsolidatedStore(store: store, data: data, isLegacy: false) - } - if let legacyStore = try? JSONDecoder().decode([String: String].self, from: data) { - let store = legacyStore.mapValues { ["value": $0] } - return ConsolidatedStore(store: store, data: data, isLegacy: true) + return try JSONDecoder().decode([String: [String: String]].self, from: data) + } catch { + throw KeychainError.loadFailed(errSecDecode) } - throw error } } -private struct ConsolidatedStore { - let store: [String: [String: String]] - let data: Data - let isLegacy: Bool -} - private struct LoadedStore { let store: [String: [String: String]] let data: Data? @@ -386,7 +238,6 @@ public enum KeychainError: Error, Equatable { case saveFailed(OSStatus) case loadFailed(OSStatus) case deleteFailed(OSStatus) - case migrationFailed(OSStatus) } private extension KeychainStorageError { diff --git a/Sources/Core/KeychainStorage.swift b/Sources/Core/KeychainStorage.swift index 75859af..ad4abe1 100644 --- a/Sources/Core/KeychainStorage.swift +++ b/Sources/Core/KeychainStorage.swift @@ -6,22 +6,12 @@ final class KeychainAuthenticationContext: @unchecked Sendable { let localAuthenticationContext = LAContext() } -struct StoredKeychainItem: Sendable { - let account: String - let data: Data -} - protocol KeychainStorage: Sendable { func readData( service: String, account: String, authenticationContext: KeychainAuthenticationContext ) throws -> Data? - func readLegacyItems( - service: String, - accountPrefix: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> [StoredKeychainItem] func replaceData( _ data: Data, service: String, @@ -69,48 +59,6 @@ struct SecurityKeychainStorage: KeychainStorage { } } - func readLegacyItems( - service: String, - accountPrefix: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> [StoredKeychainItem] { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecReturnAttributes as String: true, - kSecMatchLimit as String: kSecMatchLimitAll, - kSecUseAuthenticationContext as String: authenticationContext.localAuthenticationContext, - ] - - var result: CFTypeRef? - let status = SecItemCopyMatching(query as CFDictionary, &result) - if status == errSecItemNotFound { - return [] - } - guard status == errSecSuccess else { - throw KeychainStorageError.status(status) - } - guard let items = result as? [[String: Any]] else { - throw KeychainStorageError.status(errSecDecode) - } - - var legacyItems: [StoredKeychainItem] = [] - for item in items { - guard let account = item[kSecAttrAccount as String] as? String, - account.hasPrefix(accountPrefix), - let data = try readData( - service: service, - account: account, - authenticationContext: authenticationContext - ) - else { - continue - } - legacyItems.append(StoredKeychainItem(account: account, data: data)) - } - return legacyItems - } - func replaceData( _ data: Data, service: String, diff --git a/Sources/Core/LegacyBrandMigration.swift b/Sources/Core/LegacyBrandMigration.swift deleted file mode 100644 index e0dcd8d..0000000 --- a/Sources/Core/LegacyBrandMigration.swift +++ /dev/null @@ -1,63 +0,0 @@ -import Foundation - -public enum LegacyBrandMigration { - public static func migratePreferences(providerIds: [String]) throws { - try migratePreferences( - sourceDomainName: LegacyBrandIdentifiers.bundleIdentifier, - destination: .standard, - providerIds: providerIds - ) - } - - static func migratePreferences( - sourceDomainName: String, - destination: UserDefaults, - providerIds: [String] - ) throws { - guard let source = destination.persistentDomain(forName: sourceDomainName) else { - return - } - - let keys = preferenceKeys(providerIds: providerIds) - var copiedKeys = Set() - for key in keys where destination.object(forKey: key) == nil { - guard let value = source[key] else { continue } - destination.set(value, forKey: key) - copiedKeys.insert(key) - } - - for key in copiedKeys { - guard let destinationValue = destination.object(forKey: key), - let sourceValue = source[key], - valuesMatch(sourceValue, destinationValue) - else { - throw LegacyBrandMigrationError.preferenceVerificationFailed(key) - } - } - - destination.removePersistentDomain(forName: sourceDomainName) - } - - private static func preferenceKeys(providerIds: [String]) -> [String] { - [ - "provider-order", - "balance-thresholds-low", - "balance-thresholds-ok", - "provider-collapse-state", - "vintage-mac-icon-enabled", - ] + providerIds.map { "provider-\($0)-base-url" } - } - - private static func valuesMatch(_ lhs: Any, _ rhs: Any) -> Bool { - NSDictionary(dictionary: ["value": lhs]).isEqual(to: ["value": rhs]) - } -} - -enum LegacyBrandIdentifiers { - static let bundleIdentifier = "com.victorhqc.ai-usage" - static let keychainService = "ai-usage" -} - -public enum LegacyBrandMigrationError: Error, Equatable { - case preferenceVerificationFailed(String) -} diff --git a/Tests/CoreTests/KeychainTests.swift b/Tests/CoreTests/KeychainTests.swift index 5c38371..a2e7d0b 100644 --- a/Tests/CoreTests/KeychainTests.swift +++ b/Tests/CoreTests/KeychainTests.swift @@ -1,7 +1,11 @@ -import Core +@testable import Core +import Foundation +import Security import XCTest final class KeychainTests: XCTestCase { + private let currentService = "filbert" + func testKeychainError_casesExist() { // Verify KeychainError cases exist and are distinct. // We can't test real Keychain I/O in CI, but the enum must compile. @@ -9,10 +13,9 @@ final class KeychainTests: XCTestCase { .saveFailed(-1), .loadFailed(-1), .deleteFailed(-1), - .migrationFailed(-1), ] - XCTAssertEqual(errors.count, 4) + XCTAssertEqual(errors.count, 3) } func testKeychainError_isEquatable() { @@ -23,4 +26,158 @@ final class KeychainTests: XCTestCase { XCTAssertEqual(lhs, rhs) XCTAssertNotEqual(lhs, other) } + + func testAbsentItemLoadsEmptyStoreThenCreatesOnFirstSave() throws { + let storage = InMemoryKeychainStorage() + let keychain = makeKeychain(storage: storage) + + XCTAssertNil(storage.items[currentService]?["providers"]) + + try keychain.save("zai-key", for: "zai") + let savedData = try XCTUnwrap(storage.items[currentService]?["providers"]) + XCTAssertEqual( + try JSONDecoder().decode([String: [String: String]].self, from: savedData), + ["zai": ["value": "zai-key"]] + ) + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + } + + func testLegacyFlatPayloadSurfacesAsLoadError() throws { + let storage = InMemoryKeychainStorage() + let payload = try JSONEncoder().encode(["zai": "legacy-key"]) + storage.items[currentService] = [ + "providers": payload, + ] + let keychain = makeKeychain(storage: storage) + + XCTAssertThrowsError(try keychain.load(for: "zai")) { error in + XCTAssertEqual(error as? KeychainError, .loadFailed(errSecDecode)) + } + // The store is untouched — recovery happens via the normal setup flow, + // not an in-place rewrite (core 06 AC2). + XCTAssertNotNil(storage.items[currentService]?["providers"]) + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + func testFieldMapsRoundTripAndPreserveOtherProviders() throws { + let storage = InMemoryKeychainStorage() + let zaiFields = ["value": "zai-key", "metadata": "keep"] + let payload = try JSONEncoder().encode(["zai": zaiFields]) + storage.items[currentService] = [ + "providers": payload, + ] + let keychain = makeKeychain(storage: storage) + + try keychain.save( + ["accessToken": "cursor-access", "refreshToken": "cursor-refresh"], + for: "cursor" + ) + + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + XCTAssertEqual( + try keychain.loadFields(for: "cursor"), + ["accessToken": "cursor-access", "refreshToken": "cursor-refresh"] + ) + let savedData = try XCTUnwrap(storage.items[currentService]?["providers"]) + let saved = try JSONDecoder().decode([String: [String: String]].self, from: savedData) + XCTAssertEqual(saved["zai"], zaiFields) + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + func testFailedUpdatePreservesCachedAndStoredFields() throws { + let storage = InMemoryKeychainStorage() + let original = ["zai": ["value": "zai-key"], "deepseek": ["value": "deepseek-key"]] + let originalData = try JSONEncoder().encode(original) + storage.items[currentService] = ["providers": originalData] + let keychain = makeKeychain(storage: storage) + + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + storage.replaceError = errSecNotAvailable + + XCTAssertThrowsError(try keychain.save("new-key", for: "zai")) { error in + XCTAssertEqual(error as? KeychainError, .saveFailed(errSecNotAvailable)) + } + XCTAssertEqual(storage.items[currentService]?["providers"], originalData) + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + XCTAssertEqual(try keychain.load(for: "deepseek"), "deepseek-key") + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + func testCreateFailureDoesNotCreateConsolidatedItem() { + let storage = InMemoryKeychainStorage() + storage.createError = errSecNotAvailable + let keychain = makeKeychain(storage: storage) + + XCTAssertThrowsError(try keychain.save("zai-key", for: "zai")) { error in + XCTAssertEqual(error as? KeychainError, .saveFailed(errSecNotAvailable)) + } + XCTAssertNil(storage.items[currentService]?["providers"]) + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + func testVerificationFailureRestoresExistingConsolidatedItem() throws { + let storage = InMemoryKeychainStorage() + let originalData = try JSONEncoder().encode(["zai": ["value": "zai-key"]]) + storage.items[currentService] = ["providers": originalData] + let keychain = makeKeychain(storage: storage) + + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + storage.corruptNextReplacement = true + + XCTAssertThrowsError(try keychain.save("new-key", for: "zai")) { error in + XCTAssertEqual(error as? KeychainError, .saveFailed(errSecVerifyFailed)) + } + XCTAssertEqual(storage.items[currentService]?["providers"], originalData) + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + private func makeKeychain(storage: InMemoryKeychainStorage) -> Keychain { + Keychain(storage: storage, service: currentService) + } +} + +private final class InMemoryKeychainStorage: KeychainStorage, @unchecked Sendable { + var items: [String: [String: Data]] = [:] + var deletedItems: [(service: String, account: String)] = [] + var replaceError: OSStatus? + var createError: OSStatus? + var corruptNextReplacement = false + + func readData( + service: String, + account: String, + authenticationContext: KeychainAuthenticationContext + ) throws -> Data? { + _ = authenticationContext + return items[service]?[account] + } + + func replaceData( + _ data: Data, + service: String, + account: String, + authenticationContext: KeychainAuthenticationContext + ) throws { + _ = authenticationContext + if let replaceError { + throw KeychainStorageError.status(replaceError) + } + if items[service]?[account] == nil, let createError { + throw KeychainStorageError.status(createError) + } + let replacement = corruptNextReplacement ? Data("not-json".utf8) : data + corruptNextReplacement = false + items[service, default: [:]][account] = replacement + } + + func delete( + service: String, + account: String, + authenticationContext: KeychainAuthenticationContext + ) { + _ = authenticationContext + items[service]?[account] = nil + deletedItems.append((service, account)) + } } diff --git a/Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift b/Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift deleted file mode 100644 index f4096a6..0000000 --- a/Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift +++ /dev/null @@ -1,317 +0,0 @@ -@testable import Core -import Foundation -import Security -import XCTest - -final class LegacyBrandKeychainMigrationTests: XCTestCase { - private let currentService = "filbert" - private let previousService = "ai-usage" - - func testLoadMigratesConsolidatedAndPerProviderSecretsThenDeletesOldItems() throws { - let storage = InMemoryKeychainStorage() - storage.items[previousService] = try [ - "providers": JSONEncoder().encode(["zai": "consolidated-key"]), - "provider-deepseek": Data("deepseek-key".utf8), - ] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "consolidated-key") - XCTAssertEqual(try keychain.load(for: "deepseek"), "deepseek-key") - - let migratedData = try XCTUnwrap(storage.items[currentService]?["providers"]) - let migrated = try JSONDecoder().decode([String: [String: String]].self, from: migratedData) - XCTAssertEqual( - migrated, - [ - "zai": ["value": "consolidated-key"], - "deepseek": ["value": "deepseek-key"], - ] - ) - XCTAssertNil(storage.items[previousService]?["providers"]) - XCTAssertNil(storage.items[previousService]?["provider-deepseek"]) - } - - func testLoadPrefersCurrentServicePerProviderItemDuringMigration() throws { - let storage = InMemoryKeychainStorage() - storage.items[previousService] = try [ - "providers": JSONEncoder().encode(["zai": "previous-key"]), - ] - storage.items[currentService] = [ - "provider-zai": Data("current-key".utf8), - ] - - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "current-key") - } - - func testMigrationReadsSecretDataOnlyForLegacyProviderAccounts() throws { - let storage = InMemoryKeychainStorage() - storage.items[previousService] = [ - "provider-zai": Data("zai-key".utf8), - "unrelated": Data("unrelated-secret".utf8), - ] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertEqual( - storage.legacyDataReadRequests, - [LegacyDataReadRequest(service: previousService, account: "provider-zai")] - ) - } - - func testMigrationReusesOneAuthenticationContext() throws { - let storage = InMemoryKeychainStorage() - storage.items[previousService] = ["provider-zai": Data("zai-key".utf8)] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertEqual(storage.authenticationContextIdentifiers.count, 1) - } - - func testStructuredStoreBypassesLegacyMigration() throws { - let storage = InMemoryKeychainStorage() - storage.items[currentService] = try [ - "providers": JSONEncoder().encode(["zai": ["value": "zai-key"]]), - ] - storage.items[previousService] = ["provider-deepseek": Data("legacy-key".utf8)] - storage.legacyItemReadError = errSecAuthFailed - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - try keychain.save("deepseek-key", for: "deepseek") - - XCTAssertTrue(storage.legacyItemLookupServices.isEmpty) - XCTAssertEqual( - try keychain.load(for: "deepseek"), - "deepseek-key" - ) - } - - func testMigrationFailureLeavesPreviousItemsIntact() throws { - let storage = InMemoryKeychainStorage() - let previousData = try JSONEncoder().encode(["zai": "secret-key"]) - storage.items[previousService] = ["providers": previousData] - storage.replaceError = errSecNotAvailable - let keychain = makeKeychain(storage: storage) - - XCTAssertThrowsError(try keychain.load(for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .migrationFailed(errSecNotAvailable)) - } - XCTAssertEqual(storage.items[previousService]?["providers"], previousData) - XCTAssertFalse( - storage.deletedItems.contains { - $0.service == previousService && $0.account == "providers" - } - ) - } - - func testDeniedLegacyReadLeavesEveryItemIntact() { - let storage = InMemoryKeychainStorage() - let legacyData = Data("legacy-key".utf8) - storage.items[previousService] = ["provider-zai": legacyData] - storage.legacyItemReadError = errSecAuthFailed - let keychain = makeKeychain(storage: storage) - - XCTAssertThrowsError(try keychain.load(for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .migrationFailed(errSecAuthFailed)) - } - XCTAssertEqual(storage.items[previousService]?["provider-zai"], legacyData) - XCTAssertNil(storage.items[currentService]?["providers"]) - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - func testVerificationFailureLeavesPreviousItemsIntact() throws { - let storage = InMemoryKeychainStorage() - let previousData = try JSONEncoder().encode(["zai": "secret-key"]) - storage.items[previousService] = ["providers": previousData] - storage.corruptNextReplacement = true - let keychain = makeKeychain(storage: storage) - - XCTAssertThrowsError(try keychain.load(for: "zai")) { error in - guard case .migrationFailed = error as? KeychainError else { - XCTFail("Expected migrationFailed, got \(error)") - return - } - } - XCTAssertEqual(storage.items[previousService]?["providers"], previousData) - XCTAssertFalse( - storage.deletedItems.contains { - $0.service == previousService && $0.account == "providers" - } - ) - } - - func testLoadConvertsCurrentServiceLegacyPayloadWithoutChangingAPIKeyLoads() throws { - let storage = InMemoryKeychainStorage() - storage.items[currentService] = try [ - "providers": JSONEncoder().encode(["zai": "legacy-key"]), - ] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "legacy-key") - - let migratedData = try XCTUnwrap(storage.items[currentService]?["providers"]) - XCTAssertEqual( - try JSONDecoder().decode([String: [String: String]].self, from: migratedData), - ["zai": ["value": "legacy-key"]] - ) - } - - func testFieldMapsRoundTripAndPreserveOtherProviders() throws { - let storage = InMemoryKeychainStorage() - let zaiFields = ["value": "zai-key", "metadata": "keep"] - storage.items[currentService] = try [ - "providers": JSONEncoder().encode(["zai": zaiFields]), - ] - let keychain = makeKeychain(storage: storage) - - try keychain.save( - ["accessToken": "cursor-access", "refreshToken": "cursor-refresh"], - for: "cursor" - ) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertEqual( - try keychain.loadFields(for: "cursor"), - ["accessToken": "cursor-access", "refreshToken": "cursor-refresh"] - ) - let savedData = try XCTUnwrap(storage.items[currentService]?["providers"]) - let saved = try JSONDecoder().decode([String: [String: String]].self, from: savedData) - XCTAssertEqual(saved["zai"], zaiFields) - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - func testFailedUpdatePreservesCachedAndStoredFields() throws { - let storage = InMemoryKeychainStorage() - let original = ["zai": ["value": "zai-key"], "deepseek": ["value": "deepseek-key"]] - let originalData = try JSONEncoder().encode(original) - storage.items[currentService] = ["providers": originalData] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - storage.replaceError = errSecNotAvailable - - XCTAssertThrowsError(try keychain.save("new-key", for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .saveFailed(errSecNotAvailable)) - } - XCTAssertEqual(storage.items[currentService]?["providers"], originalData) - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertEqual(try keychain.load(for: "deepseek"), "deepseek-key") - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - func testCreateFailureDoesNotCreateConsolidatedItem() { - let storage = InMemoryKeychainStorage() - storage.createError = errSecNotAvailable - let keychain = makeKeychain(storage: storage) - - XCTAssertThrowsError(try keychain.save("zai-key", for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .saveFailed(errSecNotAvailable)) - } - XCTAssertNil(storage.items[currentService]?["providers"]) - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - func testVerificationFailureRestoresExistingConsolidatedItem() throws { - let storage = InMemoryKeychainStorage() - let originalData = try JSONEncoder().encode(["zai": ["value": "zai-key"]]) - storage.items[currentService] = ["providers": originalData] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - storage.corruptNextReplacement = true - - XCTAssertThrowsError(try keychain.save("new-key", for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .saveFailed(errSecVerifyFailed)) - } - XCTAssertEqual(storage.items[currentService]?["providers"], originalData) - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - private func makeKeychain(storage: InMemoryKeychainStorage) -> Keychain { - Keychain( - storage: storage, - service: currentService, - previousService: previousService - ) - } -} - -private struct LegacyDataReadRequest: Equatable { - let service: String - let account: String -} - -private final class InMemoryKeychainStorage: KeychainStorage, @unchecked Sendable { - var items: [String: [String: Data]] = [:] - var deletedItems: [(service: String, account: String)] = [] - var replaceError: OSStatus? - var createError: OSStatus? - var corruptNextReplacement = false - var legacyDataReadRequests: [LegacyDataReadRequest] = [] - var legacyItemLookupServices: [String] = [] - var legacyItemReadError: OSStatus? - var authenticationContextIdentifiers = Set() - - func readData( - service: String, - account: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> Data? { - record(authenticationContext) - return items[service]?[account] - } - - func readLegacyItems( - service: String, - accountPrefix: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> [StoredKeychainItem] { - record(authenticationContext) - legacyItemLookupServices.append(service) - if let legacyItemReadError { - throw KeychainStorageError.status(legacyItemReadError) - } - return (items[service] ?? [:]).compactMap { account, data in - guard account.hasPrefix(accountPrefix) else { return nil } - legacyDataReadRequests.append( - LegacyDataReadRequest(service: service, account: account) - ) - return StoredKeychainItem(account: account, data: data) - } - } - - func replaceData( - _ data: Data, - service: String, - account: String, - authenticationContext: KeychainAuthenticationContext - ) throws { - record(authenticationContext) - if let replaceError { - throw KeychainStorageError.status(replaceError) - } - if items[service]?[account] == nil, let createError { - throw KeychainStorageError.status(createError) - } - let replacement = corruptNextReplacement ? Data("not-json".utf8) : data - corruptNextReplacement = false - items[service, default: [:]][account] = replacement - } - - func delete( - service: String, - account: String, - authenticationContext: KeychainAuthenticationContext - ) { - record(authenticationContext) - items[service]?[account] = nil - deletedItems.append((service, account)) - } - - private func record(_ authenticationContext: KeychainAuthenticationContext) { - authenticationContextIdentifiers.insert(ObjectIdentifier(authenticationContext)) - } -} diff --git a/Tests/CoreTests/LegacyBrandMigrationTests.swift b/Tests/CoreTests/LegacyBrandMigrationTests.swift deleted file mode 100644 index 6b8cc6d..0000000 --- a/Tests/CoreTests/LegacyBrandMigrationTests.swift +++ /dev/null @@ -1,97 +0,0 @@ -@testable import Core -import XCTest - -final class LegacyBrandMigrationTests: XCTestCase { - private var destinationDomain: String! - private var sourceDomain: String! - private var defaults: UserDefaults! - - override func setUpWithError() throws { - try super.setUpWithError() - destinationDomain = "filbert.tests.brand-migration.\(UUID().uuidString)" - sourceDomain = "legacy.tests.brand-migration.\(UUID().uuidString)" - defaults = try XCTUnwrap(UserDefaults(suiteName: destinationDomain)) - defaults.removePersistentDomain(forName: destinationDomain) - defaults.removePersistentDomain(forName: sourceDomain) - } - - override func tearDown() { - defaults.removePersistentDomain(forName: destinationDomain) - defaults.removePersistentDomain(forName: sourceDomain) - defaults = nil - sourceDomain = nil - destinationDomain = nil - super.tearDown() - } - - func testMigratePreferencesCopiesOnlyKnownValuesAndRemovesSourceDomain() throws { - defaults.setPersistentDomain( - [ - "provider-order": ["zai", "deepseek"], - "provider-zai-base-url": "https://proxy.example.com", - "balance-thresholds-low": 7.0, - "provider-collapse-state": ["zai": true], - "vintage-mac-icon-enabled": true, - "unrelated-value": "leave behind", - ], - forName: sourceDomain - ) - - try LegacyBrandMigration.migratePreferences( - sourceDomainName: sourceDomain, - destination: defaults, - providerIds: ["zai"] - ) - - XCTAssertEqual(defaults.array(forKey: "provider-order") as? [String], ["zai", "deepseek"]) - XCTAssertEqual( - defaults.string(forKey: "provider-zai-base-url"), - "https://proxy.example.com" - ) - XCTAssertEqual(defaults.double(forKey: "balance-thresholds-low"), 7) - XCTAssertEqual( - defaults.dictionary(forKey: "provider-collapse-state") as? [String: Bool], - ["zai": true] - ) - XCTAssertTrue(defaults.bool(forKey: "vintage-mac-icon-enabled")) - XCTAssertNil(defaults.object(forKey: "unrelated-value")) - XCTAssertNil(defaults.persistentDomain(forName: sourceDomain)) - } - - func testMigratePreferencesKeepsExistingFilbertValues() throws { - defaults.set(["deepseek", "zai"], forKey: "provider-order") - defaults.setPersistentDomain( - ["provider-order": ["zai", "deepseek"]], - forName: sourceDomain - ) - - try LegacyBrandMigration.migratePreferences( - sourceDomainName: sourceDomain, - destination: defaults, - providerIds: ["zai", "deepseek"] - ) - - XCTAssertEqual(defaults.array(forKey: "provider-order") as? [String], ["deepseek", "zai"]) - XCTAssertNil(defaults.persistentDomain(forName: sourceDomain)) - } - - func testMigratePreferencesIsIdempotent() throws { - defaults.setPersistentDomain( - ["balance-thresholds-ok": 25.0], - forName: sourceDomain - ) - - try LegacyBrandMigration.migratePreferences( - sourceDomainName: sourceDomain, - destination: defaults, - providerIds: [] - ) - try LegacyBrandMigration.migratePreferences( - sourceDomainName: sourceDomain, - destination: defaults, - providerIds: [] - ) - - XCTAssertEqual(defaults.double(forKey: "balance-thresholds-ok"), 25) - } -} diff --git a/specs/core/06-remove-keychain-and-brand-migrations.md b/specs/core/06-remove-keychain-and-brand-migrations.md new file mode 100644 index 0000000..1744263 --- /dev/null +++ b/specs/core/06-remove-keychain-and-brand-migrations.md @@ -0,0 +1,169 @@ +## Objective + +Retire the one-time Keychain consolidation migration and the Core preferences +branding migration now that they have served their purpose. + +## Context + +- `Sources/Core/Keychain.swift` — `migrateStore`, `mergeMigrationItems`, + `legacyConsolidatedStore`, `migrationItems`, the `previousService` / + `legacyAccountPrefix` properties, the `[String: String]` fallback + + `isLegacy` in `decodeStore`, and `KeychainError.migrationFailed` are all + migration-only; they exist to land (core 04 AC2, AC3) and are dead weight + afterwards. +- `Sources/Core/KeychainStorage.swift` — `StoredKeychainItem` and + `KeychainStorage.readLegacyItems` (protocol requirement + + `SecurityKeychainStorage` impl) exist only to feed the per-provider item + scan inside `migrateStore`. +- `Sources/Core/LegacyBrandMigration.swift` — the whole file + (`LegacyBrandMigration`, `LegacyBrandIdentifiers`, + `LegacyBrandMigrationError`) is the one-time `com.victorhqc.ai-usage` → + current bundle-id preferences rename. +- `Sources/App/AppMain.swift` (L22–30) — the `do/catch` block that calls + `LegacyBrandMigration.migratePreferences(providerIds:)` in `init()`; it is + the only call site and goes away with the migration. +- `Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift` and + `Tests/CoreTests/LegacyBrandMigrationTests.swift` — migration coverage to + delete; `Tests/CoreTests/KeychainTests.swift` references + `.migrationFailed` and needs trimming. +- Non-migration behavior of (core 04) is unchanged: the consolidated + field-map API (AC1), in-place update-or-create with read-back verify and + restore-on-failure (AC3), and Cursor's shared-vault read / bootstrap / + re-import (AC4, AC7) all stay. +- Out of scope: the ClaudeCode provider's own brand migration + (`Sources/Providers/ClaudeCode/LegacyBrandMigration.swift`, + `StatuslineHelperInstaller`'s `migrateLegacy*` / `hasLegacy*` / + `removeLegacyArtifacts`). It is provider-local, still reachable through + (providers 02) setup paths, and mixing it into a Core cleanup would + violate provider orthogonality (AGENTS.md §1). Track it under its own + provider-scoped spec if desired. + +## Acceptance Criteria + +### AC1: Keychain loads the consolidated item directly, with no migration path + +- **Given** the `filbert` / `providers` item holds a + `[String: [String: String]]` payload (or no item exists yet) +- **When** any provider calls `Keychain.load` / `loadFields` / `save` / + `delete` +- **Then** the store is decoded from the current service and account only, + with no read of any other service, no per-provider item scan, and no + payload rewrite on first load +- **And** an absent item yields an empty in-memory store (as today), and + the first `save` creates it + +### AC2: Legacy Keychain payloads surface as a typed error, not a migration or crash + +- **Given** the `filbert` / `providers` item still holds a pre-(core 04) + `[String: String]` payload, or a stale `ai-usage` service item, or a + `provider-` item exists +- **When** the Keychain is loaded +- **Then** the current item either decodes or throws + `KeychainError.loadFailed(...)`; the app never prompts for a legacy + service, never scans for per-provider items, and never deletes anything +- **And** the user recovers through the normal setup flow — re-entering an + API key or triggering Cursor's re-import (core 04 AC7) — without data + loss beyond the already-stale pre-migration secret + +### AC3: Migration code is gone from Core's Keychain layer + +- **Given** the change lands +- **When** inspecting `Sources/Core/Keychain.swift` and + `Sources/Core/KeychainStorage.swift` +- **Then** none of `migrateStore`, `mergeMigrationItems`, + `legacyConsolidatedStore`, `migrationItems`, `previousService`, + `legacyAccountPrefix`, `ConsolidatedStore.isLegacy`, + `KeychainError.migrationFailed`, `KeychainStorage.readLegacyItems`, + `SecurityKeychainStorage.readLegacyItems`, or `StoredKeychainItem` exist +- **And** `decodeStore` decodes only `[String: [String: String]]` +- **And** `Keychain` no longer references `LegacyBrandIdentifiers` + +### AC4: The preferences branding migration is fully removed + +- **Given** the change lands +- **When** inspecting `Sources/Core/LegacyBrandMigration.swift` and + `Sources/App/AppMain.swift` +- **Then** the file `Sources/Core/LegacyBrandMigration.swift` is deleted + (including `LegacyBrandMigration`, `LegacyBrandIdentifiers`, and + `LegacyBrandMigrationError`) +- **And** the `do/catch` block calling + `migratePreferences(providerIds:)` in `AppMain.init()` is gone, so + startup proceeds straight from provider registration to + `QuotaViewModel` construction +- **And** no remaining production code references any of the removed + symbols + +### AC5: Non-migration Keychain behavior is unchanged + +- **Given** the change lands +- **When** providers save / load / delete API keys, save / load Cursor + field maps, Cursor refresh updates only its access-token field, or a + save fails mid-write +- **Then** behavior matches (core 04 AC1, AC3, AC6, AC8): field-map round + trips preserve every other provider's fields, in-place update-or-create + with read-back verification still runs, a failed save restores the prior + item and leaves the in-memory cache intact, and Cursor's shared-vault + read / bootstrap / re-import path is untouched + +### AC6: Tests reflect the removal and keep non-migration coverage + +- **Given** the completed implementation +- **When** `swift test` runs +- **Then** `Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift` and + `Tests/CoreTests/LegacyBrandMigrationTests.swift` are deleted +- **And** `Tests/CoreTests/KeychainTests.swift` no longer references + `.migrationFailed` (the case no longer exists) +- **And** the non-migration assertions that lived in + `LegacyBrandKeychainMigrationTests` (field-map round trip preserving + other providers, failed-update preservation, create-failure and + verify-failure restoration) survive — relocated into + `KeychainTests.swift` if not already covered elsewhere — so AC5 keeps + coverage +- **And** `swift build` and the full `swift test` suite pass with no + warnings + +## Plan + +1. **Simplify `Keychain.readStore`.** Return the current payload directly + (or an empty store when the item is absent). Delete `migrateStore`, + `mergeMigrationItems`, `legacyConsolidatedStore`, `migrationItems`, + the `previousService` and `legacyAccountPrefix` properties, and the + `previousService` init parameter. In `decodeStore`, keep only the + `[String: [String: String]]` branch and drop `isLegacy` from + `ConsolidatedStore`. Remove `KeychainError.migrationFailed`. +2. **Drop the migration seam from `KeychainStorage`.** Remove + `readLegacyItems` from the protocol and the + `SecurityKeychainStorage` implementation, and delete + `StoredKeychainItem`. `InMemoryKeychainStorage` in the tests loses the + same method. +3. **Remove the branding migration.** Delete + `Sources/Core/LegacyBrandMigration.swift` and the `migratePreferences` + `do/catch` block in `AppMain.init()`. +4. **Update tests.** Delete the two migration test files; trim + `KeychainTests.testKeychainError_casesExist` to the remaining cases; + move any non-migration safety assertions (AC5 behavior) into + `KeychainTests.swift` where they are not already covered. + +No production code is written until this spec is reviewed. + +## Risks + +- **Stragglers lose silent migration.** A user who has not launched Filbert + since before (core 04) still carries a `[String: String]` payload or an + `ai-usage` item. After this change they see a load error / empty store + and must re-enter keys or re-import Cursor credentials (AC2). That is + the explicit cost of retiring the migration; confirm it is acceptable + before implementing, and call it out in release notes. Mitigation: the + failure is typed and non-destructive, and Cursor recovers via its + existing re-import action (core 04 AC7). +- **The consolidated item is the only supported payload.** Removing the + schema fallback means a corrupt or hand-edited item is no longer papered + over by a re-migration; it surfaces as `loadFailed`. Acceptable, but + worth a release-note line. +- **Provider orthogonality.** The ClaudeCode provider's brand migration + (helper/cache rename) is deliberately out of scope. Bundling it would + mix a provider change into a Core cleanup (AGENTS.md §1); track it + separately. +- **Test relocation.** Several assertions in + `LegacyBrandKeychainMigrationTests` cover non-migration safety netted by + AC5. They must survive the file deletion, or AC5 loses coverage.