diff --git a/FirebaseAuth/CHANGELOG.md b/FirebaseAuth/CHANGELOG.md index 04ff2a7322f..0d7a15811b6 100644 --- a/FirebaseAuth/CHANGELOG.md +++ b/FirebaseAuth/CHANGELOG.md @@ -1,3 +1,7 @@ +# Unreleased +- [fixed] Synchronize internal `AuthKeychainServices` class to prevent + crashes from concurrent access. (#14835) + # 11.12.0 - [fixed] Fix a `fatalError` unenrolling from MFA. An invalid user token now throws an `invalidUserToken` error instead of crashing. (#14663) diff --git a/FirebaseAuth/Sources/Swift/Auth/Auth.swift b/FirebaseAuth/Sources/Swift/Auth/Auth.swift index 5d8050cc891..1f89d5ddfe4 100644 --- a/FirebaseAuth/Sources/Swift/Auth/Auth.swift +++ b/FirebaseAuth/Sources/Swift/Auth/Auth.swift @@ -1625,7 +1625,7 @@ extension Auth: AuthInterop { // MARK: Internal methods init(app: FirebaseApp, - keychainStorageProvider: AuthKeychainStorage = AuthKeychainStorageReal(), + keychainStorageProvider: AuthKeychainStorage = AuthKeychainStorageReal.shared, backend: AuthBackend = .init(rpcIssuer: AuthBackendRPCIssuer()), authDispatcher: AuthDispatcher = .init()) { self.app = app diff --git a/FirebaseAuth/Sources/Swift/Auth/AuthComponent.swift b/FirebaseAuth/Sources/Swift/Auth/AuthComponent.swift index 649d274c294..af50c79fd3d 100644 --- a/FirebaseAuth/Sources/Swift/Auth/AuthComponent.swift +++ b/FirebaseAuth/Sources/Swift/Auth/AuthComponent.swift @@ -78,7 +78,10 @@ class AuthComponent: NSObject, Library, ComponentLifecycleMaintainer { // This doesn't stop any request already issued, see b/27704535 if let keychainServiceName = Auth.deleteKeychainServiceNameForAppName(app.name) { - let keychain = AuthKeychainServices(service: keychainServiceName) + let keychain = AuthKeychainServices( + service: keychainServiceName, + storage: AuthKeychainStorageReal.shared + ) let userKey = "\(app.name)_firebase_user" try? keychain.removeData(forKey: userKey) } diff --git a/FirebaseAuth/Sources/Swift/Storage/AuthKeychainServices.swift b/FirebaseAuth/Sources/Swift/Storage/AuthKeychainServices.swift index fcf24de126a..349da7b73f8 100644 --- a/FirebaseAuth/Sources/Swift/Storage/AuthKeychainServices.swift +++ b/FirebaseAuth/Sources/Swift/Storage/AuthKeychainServices.swift @@ -13,6 +13,7 @@ // limitations under the License. import FirebaseCoreExtension +import FirebaseCoreInternal import Foundation /// The prefix string for keychain item account attribute before the key. @@ -22,16 +23,15 @@ private let kAccountPrefix = "firebase_auth_1_" /// The utility class to manipulate data in iOS Keychain. @available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *) -final class AuthKeychainServices { +final class AuthKeychainServices: Sendable { /// The name of the keychain service. - let service: String + private let service: String - let keychainStorage: AuthKeychainStorage + private let keychainStorage: AuthKeychainStorage // MARK: - Internal methods for shared keychain operations - required init(service: String = "Unset service", - storage: AuthKeychainStorage = AuthKeychainStorageReal()) { + required init(service: String = "Unset service", storage: AuthKeychainStorage) { self.service = service keychainStorage = storage } @@ -102,11 +102,7 @@ final class AuthKeychainServices { /// been deleted. /// /// This dictionary is to avoid unnecessary keychain operations against legacy items. - private var legacyEntryDeletedForKey: Set = [] - - static func storage(identifier: String) -> Self { - return Self(service: identifier) - } + private let legacyEntryDeletedForKey = FIRAllocatedUnfairLock>(initialState: []) func data(forKey key: String) throws -> Data? { if let data = try getItemLegacy(query: genericPasswordQuery(key: key)) { @@ -114,7 +110,7 @@ final class AuthKeychainServices { } // Check for legacy form. - if legacyEntryDeletedForKey.contains(key) { + if legacyEntryDeletedForKey.value().contains(key) { return nil } if let data = try getItemLegacy(query: legacyGenericPasswordQuery(key: key)) { @@ -124,7 +120,7 @@ final class AuthKeychainServices { return data } else { // Mark legacy data as non-existing so we don't have to query it again. - legacyEntryDeletedForKey.insert(key) + legacyEntryDeletedForKey.withLock { $0.insert(key) } return nil } } @@ -214,12 +210,12 @@ final class AuthKeychainServices { /// Deletes legacy item from the keychain if it is not already known to be deleted. /// - Parameter key: The key for the item. private func deleteLegacyItem(key: String) { - if legacyEntryDeletedForKey.contains(key) { + if legacyEntryDeletedForKey.value().contains(key) { return } let query = legacyGenericPasswordQuery(key: key) keychainStorage.delete(query: query) - legacyEntryDeletedForKey.insert(key) + legacyEntryDeletedForKey.withLock { $0.insert(key) } } /// Returns a keychain query of generic password to be used to manipulate key'ed value. diff --git a/FirebaseAuth/Sources/Swift/Storage/AuthKeychainStorage.swift b/FirebaseAuth/Sources/Swift/Storage/AuthKeychainStorage.swift index d53bb612402..a5e0689d6ed 100644 --- a/FirebaseAuth/Sources/Swift/Storage/AuthKeychainStorage.swift +++ b/FirebaseAuth/Sources/Swift/Storage/AuthKeychainStorage.swift @@ -17,7 +17,7 @@ import Foundation /// Protocol to manage keychain updates. Tests can do a fake implementation. @available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *) -protocol AuthKeychainStorage { +protocol AuthKeychainStorage: Sendable { func get(query: [String: Any], result: inout AnyObject?) -> OSStatus func add(query: [String: Any]) -> OSStatus func update(query: [String: Any], attributes: [String: Any]) -> OSStatus diff --git a/FirebaseAuth/Sources/Swift/Storage/AuthKeychainStorageReal.swift b/FirebaseAuth/Sources/Swift/Storage/AuthKeychainStorageReal.swift index 777ff056c3b..c2f4d762fcb 100644 --- a/FirebaseAuth/Sources/Swift/Storage/AuthKeychainStorageReal.swift +++ b/FirebaseAuth/Sources/Swift/Storage/AuthKeychainStorageReal.swift @@ -15,9 +15,12 @@ import Foundation /// The utility class to update the real keychain - @available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *) -class AuthKeychainStorageReal: AuthKeychainStorage { +final class AuthKeychainStorageReal: AuthKeychainStorage { + static let shared: AuthKeychainStorageReal = .init() + + private init() {} + func get(query: [String: Any], result: inout AnyObject?) -> OSStatus { return SecItemCopyMatching(query as CFDictionary, &result) } diff --git a/FirebaseAuth/Sources/Swift/Storage/AuthUserDefaults.swift b/FirebaseAuth/Sources/Swift/Storage/AuthUserDefaults.swift index a3e6719eb95..4a16e8150cc 100644 --- a/FirebaseAuth/Sources/Swift/Storage/AuthUserDefaults.swift +++ b/FirebaseAuth/Sources/Swift/Storage/AuthUserDefaults.swift @@ -19,17 +19,11 @@ private let kPersistentDomainNamePrefix = "com.google.Firebase.Auth." /// The utility class to manage data storage in NSUserDefaults. class AuthUserDefaults { /// The name of the persistent domain in user defaults. - private let persistentDomainName: String /// The backing NSUserDefaults storage for this instance. - private let storage: UserDefaults - static func storage(identifier: String) -> Self { - return Self(service: identifier) - } - required init(service: String) { persistentDomainName = kPersistentDomainNamePrefix + service storage = UserDefaults() diff --git a/FirebaseAuth/Tests/Unit/AuthKeychainServicesTests.swift b/FirebaseAuth/Tests/Unit/AuthKeychainServicesTests.swift index 82b643bf50d..4c1e1f2d43c 100644 --- a/FirebaseAuth/Tests/Unit/AuthKeychainServicesTests.swift +++ b/FirebaseAuth/Tests/Unit/AuthKeychainServicesTests.swift @@ -33,14 +33,15 @@ class AuthKeychainServicesTests: XCTestCase { } var keychain: AuthKeychainServices! + #if (os(macOS) && !FIREBASE_AUTH_TESTING_USE_MACOS_KEYCHAIN) || SWIFT_PACKAGE + let storage: AuthKeychainStorage = FakeAuthKeychainStorage() + #else + let storage: AuthKeychainStorage = AuthKeychainStorageReal.shared + #endif // (os(macOS) && !FIREBASE_AUTH_TESTING_USE_MACOS_KEYCHAIN) || SWIFT_PACKAGE override func setUp() { super.setUp() - #if (os(macOS) && !FIREBASE_AUTH_TESTING_USE_MACOS_KEYCHAIN) || SWIFT_PACKAGE - keychain = AuthKeychainServices(service: Self.service, storage: FakeAuthKeychainStorage()) - #else - keychain = AuthKeychainServices(service: Self.service) - #endif // (os(macOS) && !FIREBASE_AUTH_TESTING_USE_MACOS_KEYCHAIN) || SWIFT_PACKAGE + keychain = AuthKeychainServices(service: Self.service, storage: storage) } func testReadNonexisting() throws { @@ -142,7 +143,7 @@ class AuthKeychainServicesTests: XCTestCase { } var result: CFTypeRef? - let status = keychain.keychainStorage.get(query: query as [String: Any], result: &result) + let status = storage.get(query: query as [String: Any], result: &result) guard let result = result as? Data, status != errSecItemNotFound else { if let resultArray = result as? [[String: Any]], @@ -168,7 +169,7 @@ class AuthKeychainServicesTests: XCTestCase { if let service { query[kSecAttrService] = service } - XCTAssertEqual(keychain.keychainStorage.add(query: query as [String: Any]), errSecSuccess) + XCTAssertEqual(storage.add(query: query as [String: Any]), errSecSuccess) } private func setPassword(_ password: String?, @@ -192,6 +193,6 @@ class AuthKeychainServicesTests: XCTestCase { if let service { query[kSecAttrService] = service } - XCTAssertEqual(keychain.keychainStorage.delete(query: query as [String: Any]), errSecSuccess) + XCTAssertEqual(storage.delete(query: query as [String: Any]), errSecSuccess) } } diff --git a/FirebaseAuth/Tests/Unit/AuthTests.swift b/FirebaseAuth/Tests/Unit/AuthTests.swift index e1057f247b2..5ae1d522108 100644 --- a/FirebaseAuth/Tests/Unit/AuthTests.swift +++ b/FirebaseAuth/Tests/Unit/AuthTests.swift @@ -43,7 +43,7 @@ class AuthTests: RPCBaseTests { #if (os(macOS) && !FIREBASE_AUTH_TESTING_USE_MACOS_KEYCHAIN) || SWIFT_PACKAGE let keychainStorageProvider = FakeAuthKeychainStorage() #else - let keychainStorageProvider = AuthKeychainStorageReal() + let keychainStorageProvider = AuthKeychainStorageReal.shared #endif // (os(macOS) && !FIREBASE_AUTH_TESTING_USE_MACOS_KEYCHAIN) || SWIFT_PACKAGE // Stub the implementation to save the token refresh task for later execution. diff --git a/FirebaseAuth/Tests/Unit/Fakes/FakeAuthKeychainStorage.swift b/FirebaseAuth/Tests/Unit/Fakes/FakeAuthKeychainStorage.swift index 41a0d0e2b48..251c1b719db 100644 --- a/FirebaseAuth/Tests/Unit/Fakes/FakeAuthKeychainStorage.swift +++ b/FirebaseAuth/Tests/Unit/Fakes/FakeAuthKeychainStorage.swift @@ -13,29 +13,28 @@ // limitations under the License. @testable import FirebaseAuth +import FirebaseCoreInternal import Foundation import XCTest -/** @class AuthKeychainStorage - @brief The utility class to update the real keychain - */ +/// The utility class to update the real keychain @available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *) -class FakeAuthKeychainStorage: AuthKeychainStorage { +final class FakeAuthKeychainStorage: AuthKeychainStorage { // Fake Keychain. It's a dictionary, keyed by service name, for each key-value store dictionary - private var fakeKeychain: [String: [String: Any]] = [:] + private let fakeKeychain = FIRAllocatedUnfairLock<[String: [String: Any]]>(initialState: [:]) - private var fakeLegacyKeychain: [String: Any] = [:] + private let fakeLegacyKeychain = FIRAllocatedUnfairLock<[String: Any]>(initialState: [:]) func get(query: [String: Any], result: inout AnyObject?) -> OSStatus { if let service = queryService(query) { - guard let value = fakeKeychain[service]?[queryKey(query)] else { + guard let value = fakeKeychain.value()[service]?[queryKey(query)] else { return errSecItemNotFound } let returnArrayofDictionary = [[kSecValueData as String: value]] result = returnArrayofDictionary as AnyObject return noErr } else { - guard let value = fakeLegacyKeychain[queryKey(query)] else { + guard let value = fakeLegacyKeychain.value()[queryKey(query)] else { return errSecItemNotFound } let returnArrayofDictionary = [[kSecValueData as String: value]] @@ -46,9 +45,9 @@ class FakeAuthKeychainStorage: AuthKeychainStorage { func add(query: [String: Any]) -> OSStatus { if let service = queryService(query) { - fakeKeychain[service]?[queryKey(query)] = query[kSecValueData as String] + fakeKeychain.withLock { $0[service]?[queryKey(query)] = query[kSecValueData as String] } } else { - fakeLegacyKeychain[queryKey(query)] = query[kSecValueData as String] + fakeLegacyKeychain.withLock { $0[queryKey(query)] = query[kSecValueData as String] } } return noErr } @@ -59,9 +58,9 @@ class FakeAuthKeychainStorage: AuthKeychainStorage { @discardableResult func delete(query: [String: Any]) -> OSStatus { if let service = queryService(query) { - fakeKeychain[service]?[queryKey(query)] = nil + fakeKeychain.withLock { $0[service]?[queryKey(query)] = nil } } else { - fakeLegacyKeychain[queryKey(query)] = nil + fakeLegacyKeychain.withLock { $0[queryKey(query)] = nil } } return noErr } @@ -79,8 +78,10 @@ class FakeAuthKeychainStorage: AuthKeychainStorage { guard let service = query[kSecAttrService as String] as? String else { return nil } - if fakeKeychain[service] == nil { - fakeKeychain[service] = [:] + fakeKeychain.withLock { fakeKeychain in + if fakeKeychain[service] == nil { + fakeKeychain[service] = [:] + } } return service } diff --git a/FirebaseAuth/Tests/Unit/UserTests.swift b/FirebaseAuth/Tests/Unit/UserTests.swift index 0462c2306e3..c610e04a0bc 100644 --- a/FirebaseAuth/Tests/Unit/UserTests.swift +++ b/FirebaseAuth/Tests/Unit/UserTests.swift @@ -45,7 +45,7 @@ class UserTests: RPCBaseTests { #if (os(macOS) && !FIREBASE_AUTH_TESTING_USE_MACOS_KEYCHAIN) || SWIFT_PACKAGE let keychainStorageProvider = FakeAuthKeychainStorage() #else - let keychainStorageProvider = AuthKeychainStorageReal() + let keychainStorageProvider = AuthKeychainStorageReal.shared #endif // (os(macOS) && !FIREBASE_AUTH_TESTING_USE_MACOS_KEYCHAIN) || SWIFT_PACKAGE auth = Auth( app: FirebaseApp.app(name: "test-UserTests")!,