diff --git a/bitchat/App/AppRuntime.swift b/bitchat/App/AppRuntime.swift index 81f2995b..abd1e49f 100644 --- a/bitchat/App/AppRuntime.swift +++ b/bitchat/App/AppRuntime.swift @@ -40,7 +40,7 @@ final class AppRuntime: ObservableObject { #endif init( - keychain: KeychainManagerProtocol = KeychainManager(), + keychain: KeychainManagerProtocol = KeychainManager.makeDefault(), idBridge: NostrIdentityBridge = NostrIdentityBridge() ) { self.idBridge = idBridge diff --git a/bitchat/Nostr/NostrIdentityBridge.swift b/bitchat/Nostr/NostrIdentityBridge.swift index ab0c89db..c91ffc42 100644 --- a/bitchat/Nostr/NostrIdentityBridge.swift +++ b/bitchat/Nostr/NostrIdentityBridge.swift @@ -15,7 +15,7 @@ final class NostrIdentityBridge { private let keychain: KeychainManagerProtocol - init(keychain: KeychainManagerProtocol = KeychainManager()) { + init(keychain: KeychainManagerProtocol = KeychainManager.makeDefault()) { self.keychain = keychain } @@ -49,29 +49,10 @@ final class NostrIdentityBridge { /// Clear all Nostr identity associations and current identity func clearAllAssociations() { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: keychainService, - kSecMatchLimit as String: kSecMatchLimitAll, - kSecReturnAttributes as String: true - ] - - var result: AnyObject? - let status = SecItemCopyMatching(query as CFDictionary, &result) - if status == errSecSuccess, let items = result as? [[String: Any]] { - for item in items { - var deleteQuery: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: keychainService - ] - if let account = item[kSecAttrAccount as String] as? String { - deleteQuery[kSecAttrAccount as String] = account - } - SecItemDelete(deleteQuery as CFDictionary) - } - } else if status == errSecItemNotFound { - // nothing persisted; no action needed - } + // Must go through the injected keychain, not raw SecItem calls: + // under test that keychain is in-memory, and a direct delete here + // would wipe the developer's real Nostr identity on every test run. + keychain.deleteAll(service: keychainService) deviceSeedCache = nil // Also drop the in-memory derived per-geohash identities. These hold the diff --git a/bitchat/Services/FavoritesPersistenceService.swift b/bitchat/Services/FavoritesPersistenceService.swift index aa045841..84abe5d3 100644 --- a/bitchat/Services/FavoritesPersistenceService.swift +++ b/bitchat/Services/FavoritesPersistenceService.swift @@ -34,23 +34,7 @@ final class FavoritesPersistenceService: ObservableObject { static let shared = FavoritesPersistenceService() - /// Default keychain for the `shared` singleton. Under test this is an - /// in-memory keychain so touching `shared` never blocks on securityd - /// (`SecItemCopyMatching` can hang in test environments) and never reads - /// or writes the developer's real keychain. Production behavior is - /// unchanged. Tests that need their own instance keep injecting a mock - /// via `init(keychain:)`. - private nonisolated static func makeDefaultKeychain() -> KeychainManagerProtocol { - // PreviewKeychainManager lives in _PreviewHelpers, a development - // asset excluded from archive builds — release code must not - // reference it. Tests always run Debug, so the guard is lossless. - #if DEBUG - if TestEnvironment.isRunningTests { return PreviewKeychainManager() } - #endif - return KeychainManager() - } - - init(keychain: KeychainManagerProtocol = FavoritesPersistenceService.makeDefaultKeychain()) { + init(keychain: KeychainManagerProtocol = KeychainManager.makeDefault()) { self.keychain = keychain loadFavorites() diff --git a/bitchat/Services/KeychainManager.swift b/bitchat/Services/KeychainManager.swift index 39035547..cbd61f18 100644 --- a/bitchat/Services/KeychainManager.swift +++ b/bitchat/Services/KeychainManager.swift @@ -12,6 +12,32 @@ import Foundation import Security final class KeychainManager: KeychainManagerProtocol { + /// Default keychain for components that construct their own rather than + /// having one injected. Under test this is an in-memory keychain: the + /// xctest runner's code signature changes every build, so any read of a + /// real login-keychain item triggers a macOS password prompt that + /// "Always Allow" can never satisfy — and tests must never read or + /// mutate the developer's real keychain (`SecItemCopyMatching` can also + /// hang in test environments). Production behavior is unchanged. + static func makeDefault() -> KeychainManagerProtocol { + // PreviewKeychainManager lives in _PreviewHelpers, a development + // asset excluded from archive builds — release code must not + // reference it. Tests always run Debug, so the guard is lossless. + #if DEBUG + if TestEnvironment.isRunningTests { return sharedTestKeychain } + #endif + return KeychainManager() + } + + #if DEBUG + /// One store per process, mirroring the real keychain: separate + /// default-constructed components (e.g. two NostrIdentityBridge + /// instances in BoardManager's publish and delete paths) must see each + /// other's writes, or they would derive different Nostr identities + /// under test. + private static let sharedTestKeychain = PreviewKeychainManager() + #endif + // Use consistent service name for all keychain items private let service = BitchatApp.bundleID private let appGroup = "group.\(BitchatApp.bundleID)" @@ -540,4 +566,30 @@ final class KeychainManager: KeychainManagerProtocol { SecItemDelete(query as CFDictionary) } + + /// Delete every item stored under a custom service + func deleteAll(service customService: String) { + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: customService, + kSecMatchLimit as String: kSecMatchLimitAll, + kSecReturnAttributes as String: true + ] + + var result: AnyObject? + let status = SecItemCopyMatching(query as CFDictionary, &result) + guard status == errSecSuccess, let items = result as? [[String: Any]] else { + return + } + for item in items { + var deleteQuery: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: customService + ] + if let account = item[kSecAttrAccount as String] as? String { + deleteQuery[kSecAttrAccount as String] = account + } + SecItemDelete(deleteQuery as CFDictionary) + } + } } diff --git a/bitchat/_PreviewHelpers/PreviewKeychainManager.swift b/bitchat/_PreviewHelpers/PreviewKeychainManager.swift index 49e63d3f..32827f48 100644 --- a/bitchat/_PreviewHelpers/PreviewKeychainManager.swift +++ b/bitchat/_PreviewHelpers/PreviewKeychainManager.swift @@ -10,25 +10,37 @@ import BitFoundation import Foundation final class PreviewKeychainManager: KeychainManagerProtocol { + // Locked: KeychainManager.makeDefault() hands one shared instance to + // every default-constructed component under test, which access it from + // arbitrary threads. + private let lock = NSLock() private var storage: [String: Data] = [:] private var serviceStorage: [String: [String: Data]] = [:] init() {} func saveIdentityKey(_ keyData: Data, forKey key: String) -> Bool { + lock.lock() + defer { lock.unlock() } storage[key] = keyData return true } func getIdentityKey(forKey key: String) -> Data? { - storage[key] + lock.lock() + defer { lock.unlock() } + return storage[key] } func deleteIdentityKey(forKey key: String) -> Bool { + lock.lock() + defer { lock.unlock() } storage.removeValue(forKey: key) return true } func deleteAllKeychainData() -> Bool { + lock.lock() + defer { lock.unlock() } storage.removeAll() serviceStorage.removeAll() return true @@ -39,11 +51,15 @@ final class PreviewKeychainManager: KeychainManagerProtocol { func secureClear(_ string: inout String) {} func verifyIdentityKeyExists() -> Bool { - storage["identity_noiseStaticKey"] != nil + lock.lock() + defer { lock.unlock() } + return storage["identity_noiseStaticKey"] != nil } // BCH-01-009: New methods with proper error classification func getIdentityKeyWithResult(forKey key: String) -> KeychainReadResult { + lock.lock() + defer { lock.unlock() } if let data = storage[key] { return .success(data) } @@ -51,6 +67,8 @@ final class PreviewKeychainManager: KeychainManagerProtocol { } func saveIdentityKeyWithResult(_ keyData: Data, forKey key: String) -> KeychainSaveResult { + lock.lock() + defer { lock.unlock() } storage[key] = keyData return .success } @@ -58,17 +76,26 @@ final class PreviewKeychainManager: KeychainManagerProtocol { // MARK: - Generic Data Storage (consolidated from KeychainHelper) func save(key: String, data: Data, service: String, accessible: CFString?) { - if serviceStorage[service] == nil { - serviceStorage[service] = [:] - } - serviceStorage[service]?[key] = data + lock.lock() + defer { lock.unlock() } + serviceStorage[service, default: [:]][key] = data } func load(key: String, service: String) -> Data? { - serviceStorage[service]?[key] + lock.lock() + defer { lock.unlock() } + return serviceStorage[service]?[key] } func delete(key: String, service: String) { + lock.lock() + defer { lock.unlock() } serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + lock.lock() + defer { lock.unlock() } + serviceStorage.removeValue(forKey: service) + } } diff --git a/bitchatTests/Mocks/MockKeychain.swift b/bitchatTests/Mocks/MockKeychain.swift index 2c8d7c32..cb974bae 100644 --- a/bitchatTests/Mocks/MockKeychain.swift +++ b/bitchatTests/Mocks/MockKeychain.swift @@ -85,6 +85,10 @@ final class MockKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } } /// Typealias for backwards compatibility with tests using MockKeychainHelper @@ -198,4 +202,8 @@ final class TrackingMockKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } } diff --git a/bitchatTests/Services/SecureIdentityStateManagerTests.swift b/bitchatTests/Services/SecureIdentityStateManagerTests.swift index 27e57fdf..a280d02e 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerTests.swift @@ -496,4 +496,8 @@ private final class FailingCacheSaveKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } } diff --git a/localPackages/BitFoundation/Sources/BitFoundation/KeychainManagerProtocol.swift b/localPackages/BitFoundation/Sources/BitFoundation/KeychainManagerProtocol.swift index ea78288e..5a7ec448 100644 --- a/localPackages/BitFoundation/Sources/BitFoundation/KeychainManagerProtocol.swift +++ b/localPackages/BitFoundation/Sources/BitFoundation/KeychainManagerProtocol.swift @@ -34,6 +34,8 @@ public protocol KeychainManagerProtocol { func load(key: String, service: String) -> Data? /// Delete data from a custom service func delete(key: String, service: String) + /// Delete every item stored under a custom service + func deleteAll(service: String) } // MARK: - Keychain Error Types diff --git a/localPackages/BitFoundation/Tests/BitFoundationTests/MockKeychain.swift b/localPackages/BitFoundation/Tests/BitFoundationTests/MockKeychain.swift index 5d4071ec..870911ce 100644 --- a/localPackages/BitFoundation/Tests/BitFoundationTests/MockKeychain.swift +++ b/localPackages/BitFoundation/Tests/BitFoundationTests/MockKeychain.swift @@ -85,6 +85,10 @@ final class MockKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } } /// Typealias for backwards compatibility with tests using MockKeychainHelper @@ -198,4 +202,8 @@ final class TrackingMockKeychain: KeychainManagerProtocol { func delete(key: String, service: String) { serviceStorage[service]?.removeValue(forKey: key) } + + func deleteAll(service: String) { + serviceStorage.removeValue(forKey: service) + } }