[Auth] Conform 'AuthKeychainServices' to 'Sendable' (#14862)

Author: ncooke3

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)

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

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)
       }

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,19 +102,15 @@ final class AuthKeychainServices {
   /// been deleted.
   ///
   /// This dictionary is to avoid unnecessary keychain operations against legacy items.
-  private var legacyEntryDeletedForKey: Set<String> = []
-
-  static func storage(identifier: String) -> Self {
-    return Self(service: identifier)
-  }
+  private let legacyEntryDeletedForKey = FIRAllocatedUnfairLock<Set<String>>(initialState: [])
 
   func data(forKey key: String) throws -> Data? {
     if let data = try getItemLegacy(query: genericPasswordQuery(key: key)) {
       return data
     }
 
     // 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.

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

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)
   }

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()

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)
   }
 }

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.

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
   }