diff --git a/Fluid.xcodeproj/project.pbxproj b/Fluid.xcodeproj/project.pbxproj index d72a6a01..62902296 100644 --- a/Fluid.xcodeproj/project.pbxproj +++ b/Fluid.xcodeproj/project.pbxproj @@ -12,6 +12,7 @@ 7C3697892ED70F9C005874CE /* DynamicNotchKit in Frameworks */ = {isa = PBXBuildFile; productRef = 7C3697882ED70F9C005874CE /* DynamicNotchKit */; }; 7C5AF14B2F15041600DE21B0 /* MediaRemoteAdapter in Frameworks */ = {isa = PBXBuildFile; productRef = 7C5AF14A2F15041600DE21B0 /* MediaRemoteAdapter */; }; 7C91B0012F42AA0100C0DEF0 /* HotkeyShortcutTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7C91B0022F42AA0100C0DEF0 /* HotkeyShortcutTests.swift */; }; + 7C91B0032F42AA0100C0DEF0 /* ProviderAPIKeyMigrationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7C91B0042F42AA0100C0DEF0 /* ProviderAPIKeyMigrationTests.swift */; }; 7CDB0A2D2F3C4D5600FB7CAD /* DictationE2ETests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7CDB0A292F3C4D5600FB7CAD /* DictationE2ETests.swift */; }; 7CDB0A2E2F3C4D5600FB7CAD /* AudioFixtureLoader.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7CDB0A2A2F3C4D5600FB7CAD /* AudioFixtureLoader.swift */; }; 7CDB0A2F2F3C4D5600FB7CAD /* dictation_fixture.wav in Resources */ = {isa = PBXBuildFile; fileRef = 7CDB0A2B2F3C4D5600FB7CAD /* dictation_fixture.wav */; }; @@ -33,6 +34,7 @@ 7C078D8F2E3B339200FB7CAC /* FluidVoice Debug.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = "FluidVoice Debug.app"; sourceTree = BUILT_PRODUCTS_DIR; }; 7CDB0A202F3C4D5600FB7CAD /* FluidDictationIntegrationTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = FluidDictationIntegrationTests.xctest; sourceTree = BUILT_PRODUCTS_DIR; }; 7C91B0022F42AA0100C0DEF0 /* HotkeyShortcutTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HotkeyShortcutTests.swift; sourceTree = ""; }; + 7C91B0042F42AA0100C0DEF0 /* ProviderAPIKeyMigrationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ProviderAPIKeyMigrationTests.swift; sourceTree = ""; }; 7CDB0A292F3C4D5600FB7CAD /* DictationE2ETests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DictationE2ETests.swift; sourceTree = ""; }; 7CDB0A2A2F3C4D5600FB7CAD /* AudioFixtureLoader.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AudioFixtureLoader.swift; sourceTree = ""; }; 7CDB0A2B2F3C4D5600FB7CAD /* dictation_fixture.wav */ = {isa = PBXFileReference; lastKnownFileType = audio.wav; path = dictation_fixture.wav; sourceTree = ""; }; @@ -104,6 +106,7 @@ 7CDB0A272F3C4D5600FB7CAD /* Resources */, 7CDB0A292F3C4D5600FB7CAD /* DictationE2ETests.swift */, 7C91B0022F42AA0100C0DEF0 /* HotkeyShortcutTests.swift */, + 7C91B0042F42AA0100C0DEF0 /* ProviderAPIKeyMigrationTests.swift */, ); path = FluidDictationIntegrationTests; sourceTree = ""; @@ -258,6 +261,7 @@ 7CDB0A2E2F3C4D5600FB7CAD /* AudioFixtureLoader.swift in Sources */, 7CDB0A2D2F3C4D5600FB7CAD /* DictationE2ETests.swift in Sources */, 7C91B0012F42AA0100C0DEF0 /* HotkeyShortcutTests.swift in Sources */, + 7C91B0032F42AA0100C0DEF0 /* ProviderAPIKeyMigrationTests.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/Sources/Fluid/Persistence/KeychainService.swift b/Sources/Fluid/Persistence/KeychainService.swift index 58547d1c..c2b795ec 100644 --- a/Sources/Fluid/Persistence/KeychainService.swift +++ b/Sources/Fluid/Persistence/KeychainService.swift @@ -18,9 +18,19 @@ enum KeychainServiceError: Error, LocalizedError { } } +/// Provider-API-key storage surface used by `SettingsStore`, extracted so a mock +/// can be injected in tests. `KeychainService` is the production implementation. +protocol ProviderKeychain { + func storeKey(_ key: String, for providerID: String) throws + func fetchAllKeys() throws -> [String: String] + func storeAllKeys(_ values: [String: String]) throws + func legacyProviderEntries() throws -> [String: String] + func removeLegacyEntries(providerIDs: [String]) throws +} + /// Lightweight helper for storing provider API keys in the system Keychain. /// Keys are stored as generic passwords scoped to the FluidVoice service. -final class KeychainService { +final class KeychainService: ProviderKeychain { static let shared = KeychainService() private let service = "com.fluidvoice.provider-api-keys" diff --git a/Sources/Fluid/Persistence/SettingsStore.swift b/Sources/Fluid/Persistence/SettingsStore.swift index 936f530e..1e4f2b73 100644 --- a/Sources/Fluid/Persistence/SettingsStore.swift +++ b/Sources/Fluid/Persistence/SettingsStore.swift @@ -16,7 +16,9 @@ final class SettingsStore: ObservableObject { static let transcriptionPreviewCharLimitStep = 50 static let defaultTranscriptionPreviewCharLimit = 150 private let defaults = UserDefaults.standard - private let keychain = KeychainService.shared + // `var` only so a mock can be substituted in tests via `setKeychainForTesting(_:)`; + // production code never reassigns it. + private var keychain: ProviderKeychain = KeychainService.shared private(set) var launchAtStartupEnabled = false private(set) var launchAtStartupErrorMessage: String? private(set) var launchAtStartupStatusMessage = @@ -2900,28 +2902,37 @@ final class SettingsStore: ObservableObject { var merged = (try? self.keychain.fetchAllKeys()) ?? [:] var didMutate = false - if let legacyDefaults = defaults.dictionary(forKey: Keys.providerAPIKeys) as? [String: String], - legacyDefaults.isEmpty == false - { + let legacyDefaults = (self.defaults.dictionary(forKey: Keys.providerAPIKeys) as? [String: String]) ?? [:] + if legacyDefaults.isEmpty == false { merged.merge(self.sanitizeAPIKeys(legacyDefaults)) { _, new in new } didMutate = true } - self.defaults.removeObject(forKey: Keys.providerAPIKeys) - if let legacyKeychain = try? keychain.legacyProviderEntries(), - legacyKeychain.isEmpty == false - { + let legacyKeychain = (try? self.keychain.legacyProviderEntries()) ?? [:] + if legacyKeychain.isEmpty == false { merged.merge(self.sanitizeAPIKeys(legacyKeychain)) { _, new in new } didMutate = true - try? self.keychain.removeLegacyEntries(providerIDs: Array(legacyKeychain.keys)) } - if didMutate { - do { - _ = try self.saveProviderAPIKeys(merged) - } catch { - self.logProviderAPIKeyPersistenceFailure(error) - } + guard didMutate else { + // Nothing to migrate; clear any stale empty legacy defaults entry. + self.defaults.removeObject(forKey: Keys.providerAPIKeys) + return + } + + do { + _ = try self.saveProviderAPIKeys(merged) + } catch { + // The consolidated write failed. Keep every legacy source intact so the keys + // survive and can be migrated again on the next launch. + self.logProviderAPIKeyPersistenceFailure(error) + return + } + + // The consolidated entry is persisted; only now is it safe to drop the legacy sources. + self.defaults.removeObject(forKey: Keys.providerAPIKeys) + if legacyKeychain.isEmpty == false { + try? self.keychain.removeLegacyEntries(providerIDs: Array(legacyKeychain.keys)) } } @@ -3176,6 +3187,16 @@ final class SettingsStore: ObservableObject { let keyID = self.canonicalProviderKey(for: provider.id) do { try self.keychain.storeKey(trimmed, for: keyID) + // Only blank the in-memory plaintext copy once the Keychain write succeeded. + // Blanking regardless of the result would discard the user's only copy of a key + // whose Keychain write failed. + decoded[index] = SavedProvider( + id: provider.id, + name: provider.name, + baseURL: provider.baseURL, + apiKey: "", + models: provider.models + ) didModify = true } catch { DebugLogger.shared @@ -3184,14 +3205,6 @@ final class SettingsStore: ObservableObject { source: "SettingsStore" ) } - - decoded[index] = SavedProvider( - id: provider.id, - name: provider.name, - baseURL: provider.baseURL, - apiKey: "", - models: provider.models - ) } if didModify, @@ -4709,3 +4722,21 @@ extension SettingsStore { return newModel } } + +#if DEBUG +extension SettingsStore { + /// Substitutes the Keychain implementation so a throwing mock can be exercised in tests. + /// Reset to `KeychainService.shared` once the test finishes. + func setKeychainForTesting(_ keychain: ProviderKeychain) { + self.keychain = keychain + } + + func migrateProviderAPIKeysForTesting() { + self.migrateProviderAPIKeysIfNeeded() + } + + func scrubSavedProviderAPIKeysForTesting() { + self.scrubSavedProviderAPIKeys() + } +} +#endif diff --git a/Tests/FluidDictationIntegrationTests/ProviderAPIKeyMigrationTests.swift b/Tests/FluidDictationIntegrationTests/ProviderAPIKeyMigrationTests.swift new file mode 100644 index 00000000..337b4f81 --- /dev/null +++ b/Tests/FluidDictationIntegrationTests/ProviderAPIKeyMigrationTests.swift @@ -0,0 +1,210 @@ +@testable import FluidVoice_Debug +import Foundation +import XCTest + +/// Regression coverage for the provider-API-key migrations in `SettingsStore`. +/// Both migrations must persist the replacement before destroying the legacy source, +/// so a failed Keychain write can never silently lose a user's API key. +final class ProviderAPIKeyMigrationTests: XCTestCase { + private let savedProvidersKey = "SavedProviders" + private let providerAPIKeysKey = "ProviderAPIKeys" + private let providerAPIKeyIdentifiersKey = "ProviderAPIKeyIdentifiers" + + // MARK: - scrubSavedProviderAPIKeys + + func testScrubPreservesAPIKeyWhenKeychainStoreFailsForThatProvider() throws { + try self.withRestoredProviderState { defaults in + let keychain = MockProviderKeychain() + keychain.failingProviderIDs = ["custom:failing-provider"] + SettingsStore.shared.setKeychainForTesting(keychain) + + let succeeding = SettingsStore.SavedProvider( + id: "custom:ok-provider", + name: "OK Provider", + baseURL: "https://ok.example", + apiKey: "sk-ok", + models: [] + ) + let failing = SettingsStore.SavedProvider( + id: "custom:failing-provider", + name: "Failing Provider", + baseURL: "https://fail.example", + apiKey: "sk-keep-me", + models: [] + ) + defaults.set(try JSONEncoder().encode([succeeding, failing]), forKey: self.savedProvidersKey) + + SettingsStore.shared.scrubSavedProviderAPIKeysForTesting() + + let data = try XCTUnwrap(defaults.data(forKey: self.savedProvidersKey)) + let result = try JSONDecoder().decode([SettingsStore.SavedProvider].self, from: data) + let failingResult = try XCTUnwrap(result.first { $0.id == "custom:failing-provider" }) + let okResult = try XCTUnwrap(result.first { $0.id == "custom:ok-provider" }) + + XCTAssertEqual( + failingResult.apiKey, + "sk-keep-me", + "A provider whose Keychain write failed must keep its plaintext key, not lose it." + ) + XCTAssertEqual(okResult.apiKey, "", "A successfully migrated provider should be blanked in defaults.") + XCTAssertEqual(keychain.consolidatedKeys["custom:ok-provider"], "sk-ok") + } + } + + func testScrubMigratesAndBlanksKeyWhenKeychainStoreSucceeds() throws { + try self.withRestoredProviderState { defaults in + let keychain = MockProviderKeychain() + SettingsStore.shared.setKeychainForTesting(keychain) + + let provider = SettingsStore.SavedProvider( + id: "custom:provider", + name: "Provider", + baseURL: "https://provider.example", + apiKey: "sk-secret", + models: [] + ) + defaults.set(try JSONEncoder().encode([provider]), forKey: self.savedProvidersKey) + + SettingsStore.shared.scrubSavedProviderAPIKeysForTesting() + + let data = try XCTUnwrap(defaults.data(forKey: self.savedProvidersKey)) + let result = try JSONDecoder().decode([SettingsStore.SavedProvider].self, from: data) + XCTAssertEqual(result.first?.apiKey, "", "Key should be blanked in defaults once stored in the Keychain.") + XCTAssertEqual(keychain.consolidatedKeys["custom:provider"], "sk-secret") + } + } + + // MARK: - migrateProviderAPIKeysIfNeeded + + func testMigrateKeepsLegacySourcesWhenConsolidatedSaveFails() throws { + try self.withRestoredProviderState { defaults in + let keychain = MockProviderKeychain() + keychain.legacyEntries = ["anthropic": "legacy-keychain-secret"] + keychain.shouldThrowOnWrite = true + SettingsStore.shared.setKeychainForTesting(keychain) + + defaults.set(["openai": "legacy-defaults-secret"], forKey: self.providerAPIKeysKey) + + SettingsStore.shared.migrateProviderAPIKeysForTesting() + + let legacyDefaults = try XCTUnwrap(defaults.dictionary(forKey: self.providerAPIKeysKey) as? [String: String]) + XCTAssertEqual( + legacyDefaults["openai"], + "legacy-defaults-secret", + "Legacy UserDefaults keys must survive when the consolidated save fails." + ) + XCTAssertTrue( + keychain.removedLegacyProviderIDs.isEmpty, + "Legacy Keychain entries must not be removed when the consolidated save fails." + ) + XCTAssertEqual( + keychain.legacyEntries["anthropic"], + "legacy-keychain-secret", + "Legacy Keychain values must survive when the consolidated save fails." + ) + } + } + + func testMigrateRemovesLegacySourcesWhenConsolidatedSaveSucceeds() throws { + try self.withRestoredProviderState { defaults in + let keychain = MockProviderKeychain() + keychain.legacyEntries = ["anthropic": "legacy-keychain-secret"] + SettingsStore.shared.setKeychainForTesting(keychain) + + defaults.set(["openai": "legacy-defaults-secret"], forKey: self.providerAPIKeysKey) + + SettingsStore.shared.migrateProviderAPIKeysForTesting() + + XCTAssertNil( + defaults.dictionary(forKey: self.providerAPIKeysKey), + "Legacy UserDefaults keys should be removed once the consolidated save succeeds." + ) + XCTAssertEqual(keychain.consolidatedKeys["openai"], "legacy-defaults-secret") + XCTAssertEqual(keychain.consolidatedKeys["anthropic"], "legacy-keychain-secret") + XCTAssertEqual( + keychain.removedLegacyProviderIDs, + [["anthropic"]], + "Legacy Keychain entries should be removed once the consolidated save succeeds." + ) + } + } + + // MARK: - Helpers + + /// Snapshots the provider-related UserDefaults keys and the injected Keychain, runs `body` + /// against the shared store, then restores both. Mirrors `HotkeyShortcutTests`, which also + /// exercises `SettingsStore.shared` against `UserDefaults.standard`. + private func withRestoredProviderState(_ body: (UserDefaults) throws -> Void) throws { + let defaults = UserDefaults.standard + let keys = [self.savedProvidersKey, self.providerAPIKeysKey, self.providerAPIKeyIdentifiersKey] + var snapshot: [String: Any] = [:] + for key in keys { + if let value = defaults.object(forKey: key) { + snapshot[key] = value + } + } + + defer { + for key in keys { + if let previous = snapshot[key] { + defaults.set(previous, forKey: key) + } else { + defaults.removeObject(forKey: key) + } + } + SettingsStore.shared.setKeychainForTesting(KeychainService.shared) + } + + for key in keys { + defaults.removeObject(forKey: key) + } + + try body(defaults) + } +} + +private final class MockProviderKeychain: ProviderKeychain { + enum Failure: Error { + case write + } + + var consolidatedKeys: [String: String] + var legacyEntries: [String: String] + var failingProviderIDs: Set = [] + var shouldThrowOnWrite = false + private(set) var removedLegacyProviderIDs: [[String]] = [] + + init(consolidatedKeys: [String: String] = [:], legacyEntries: [String: String] = [:]) { + self.consolidatedKeys = consolidatedKeys + self.legacyEntries = legacyEntries + } + + func storeKey(_ key: String, for providerID: String) throws { + if self.shouldThrowOnWrite || self.failingProviderIDs.contains(providerID) { + throw Failure.write + } + self.consolidatedKeys[providerID] = key + } + + func storeAllKeys(_ values: [String: String]) throws { + if self.shouldThrowOnWrite { + throw Failure.write + } + self.consolidatedKeys = values + } + + func fetchAllKeys() throws -> [String: String] { + self.consolidatedKeys + } + + func legacyProviderEntries() throws -> [String: String] { + self.legacyEntries + } + + func removeLegacyEntries(providerIDs: [String]) throws { + self.removedLegacyProviderIDs.append(providerIDs) + for providerID in providerIDs { + self.legacyEntries.removeValue(forKey: providerID) + } + } +}