bitwarden · matt-livefront · Sep 6, 2024 · Sep 5, 2024 · Sep 6, 2024 · Sep 6, 2024
@@ -106,9 +106,11 @@ protocol AuthRepository: AnyObject {
     /// - Parameters:
     ///   - email: The user's email.
     ///   - password: The user's password.
+    ///   - isPreAuth: Whether the client is being used for a user prior to authentication (when
+    ///     the user's ID doesn't yet exist).
     /// - Returns: The password strength of the password.
     ///
-    func passwordStrength(email: String, password: String) async throws -> UInt8
+    func passwordStrength(email: String, password: String, isPreAuth: Bool) async throws -> UInt8
 
    /// Gets the profiles state for a user.
     ///
@@ -589,8 +591,9 @@ extension DefaultAuthRepository: AuthRepository {
         await vaultTimeoutService.remove(userId: userId)
     }
 
-    func passwordStrength(email: String, password: String) async throws -> UInt8 {
-        try await clientService.auth().passwordStrength(password: password, email: email, additionalInputs: [])
+    func passwordStrength(email: String, password: String, isPreAuth: Bool) async throws -> UInt8 {
+        try await clientService.auth(isPreAuth: isPreAuth)
+            .passwordStrength(password: password, email: email, additionalInputs: [])
     }
 
     func sessionTimeoutAction(userId: String?) async throws -> SessionTimeoutAction {

@@ -1097,22 +1097,31 @@ class AuthRepositoryTests: BitwardenTestCase { // swiftlint:disable:this type_bo
     /// `passwordStrength(email:password)` returns the calculated password strength.
     func test_passwordStrength() async throws {
         clientService.mockAuth.passwordStrengthResult = 0
-        let weakPasswordStrength = try await subject.passwordStrength(email: "user@bitwarden.com", password: "password")
+        let weakPasswordStrength = try await subject.passwordStrength(
+            email: "user@bitwarden.com",
+            password: "password",
+            isPreAuth: false
+        )
         XCTAssertEqual(weakPasswordStrength, 0)
         XCTAssertEqual(clientService.mockAuth.passwordStrengthEmail, "user@bitwarden.com")
         XCTAssertEqual(clientService.mockAuth.passwordStrengthPassword, "password")
+        XCTAssertFalse(clientService.mockAuthIsPreAuth)
 
         clientService.mockAuth.passwordStrengthResult = 4
         let strongPasswordStrength = try await subject.passwordStrength(
             email: "user@bitwarden.com",
-            password: "ghu65zQ0*TjP@ij74g*&FykWss#Kgv8L8j8XmC03"
+            password: "ghu65zQ0*TjP@ij74g*&FykWss#Kgv8L8j8XmC03",
+            isPreAuth: true
         )
         XCTAssertEqual(strongPasswordStrength, 4)
         XCTAssertEqual(clientService.mockAuth.passwordStrengthEmail, "user@bitwarden.com")
         XCTAssertEqual(
             clientService.mockAuth.passwordStrengthPassword,
             "ghu65zQ0*TjP@ij74g*&FykWss#Kgv8L8j8XmC03"
         )
+
+        XCTAssertTrue(clientService.mockAuthIsPreAuth)
+        XCTAssertNil(clientService.mockAuthUserId)
     }
 
     /// `sessionTimeoutAction()` returns the session timeout action for a user.

@@ -34,6 +34,7 @@ class MockAuthRepository: AuthRepository { // swiftlint:disable:this type_body_l
     var migrateUserToKeyConnectorPassword: String?
     var migrateUserToKeyConnectorResult: Result<Void, Error> = .success(())
     var passwordStrengthEmail: String?
+    var passwordStrengthIsPreAuth = false
     var passwordStrengthPassword: String?
     var passwordStrengthResult: UInt8 = 0
     var pinProtectedUserKey = "123"
@@ -174,9 +175,10 @@ class MockAuthRepository: AuthRepository { // swiftlint:disable:this type_body_l
         try isPinUnlockAvailableResult.get()
     }
 
-    func passwordStrength(email: String, password: String) async -> UInt8 {
+    func passwordStrength(email: String, password: String, isPreAuth: Bool) async -> UInt8 {
         passwordStrengthEmail = email
         passwordStrengthPassword = password
+        passwordStrengthIsPreAuth = isPreAuth
         return passwordStrengthResult
     }
 

@@ -537,7 +537,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
         let response = try await accountAPIService.preLogin(email: username)
 
         // Get the identity token to log in to Bitwarden.
-        let hashedPassword = try await clientService.auth().hashPassword(
+        let hashedPassword = try await clientService.auth(isPreAuth: true).hashPassword(
             email: username,
             password: masterPassword,
             kdfParams: response.sdkKdf,

@@ -8,10 +8,13 @@ protocol ClientService {
 
     /// Returns a `ClientAuthProtocol` for auth data tasks.
     ///
-    /// - Parameter userId: The user ID mapped to the client instance.
+    /// - Parameters:
+    ///   - userId: The user ID mapped to the client instance.
+    ///   - isPreAuth: Whether the client is being used for a user prior to authentication (when
+    ///     the user's ID doesn't yet exist).
     /// - Returns: A `ClientAuthProtocol` for auth data tasks.
     ///
-    func auth(for userId: String?) async throws -> ClientAuthProtocol
+    func auth(for userId: String?, isPreAuth: Bool) async throws -> ClientAuthProtocol
 
     /// Returns a `ClientCryptoProtocol` for crypto data tasks.
     ///
@@ -68,7 +71,16 @@ extension ClientService {
     /// Returns a `ClientAuthProtocol` for auth data tasks.
     ///
     func auth() async throws -> ClientAuthProtocol {
-        try await auth(for: nil)
+        try await auth(for: nil, isPreAuth: false)
+    }
+
+    /// Returns a `ClientAuthProtocol` for auth data tasks.
+    ///
+    /// - Parameter isPreAuth: Whether the client is being used for a user prior to authentication
+    ///     (when the user's ID doesn't yet exist).
+    ///
+    func auth(isPreAuth: Bool) async throws -> ClientAuthProtocol {
+        try await auth(for: nil, isPreAuth: isPreAuth)
     }
 
     /// Returns a `ClientCryptoProtocol` for crypto data tasks.
@@ -119,7 +131,7 @@ extension ClientService {
 /// A default `ClientService` implementation. This is a thin wrapper around the SDK `Client` so that
 /// it can be swapped to a mock instance during tests.
 ///
-class DefaultClientService: ClientService {
+actor DefaultClientService: ClientService {
     // MARK: Private properties
 
     /// A helper object that builds a Bitwarden SDK `Client`.
@@ -161,8 +173,8 @@ class DefaultClientService: ClientService {
 
     // MARK: Methods
 
-    func auth(for userId: String?) async throws -> ClientAuthProtocol {
-        try await client(for: userId).auth()
+    func auth(for userId: String?, isPreAuth: Bool = false) async throws -> ClientAuthProtocol {
+        try await client(for: userId, isPreAuth: isPreAuth).auth()
     }
 
     func crypto(for userId: String?) async throws -> ClientCryptoProtocol {
@@ -199,13 +211,22 @@ class DefaultClientService: ClientService {
     /// Returns a user's client if it exists. If the user has no client, create one and map it to their user ID.
     ///
     ///
-    /// If there is no active user/there are no accounts, return the original client.
+    /// If there is no active user/there are no accounts, return a new client.
     /// This could occur if the app is launched from a fresh install.
     ///
-    /// - Parameter userId: A user ID for which a `Client` is mapped to or will be mapped to.
+    /// - Parameters:
+    ///   - userId: A user ID for which a `Client` is mapped to or will be mapped to.
+    ///   - isPreAuth: Whether the client is being used for a user prior to authentication (when
+    ///     the user's ID doesn't yet exist).
     /// - Returns: A user's client.
     ///
-    private func client(for userId: String?) async throws -> BitwardenSdkClient {
+    private func client(for userId: String?, isPreAuth: Bool = false) async throws -> BitwardenSdkClient {
+        guard !isPreAuth else {
+            // If this client is being used for a new user prior to authentication, a user ID doesn't
+            // exist for the user to map the client to, so return a new client.
+            return clientBuilder.buildClient()
+        }
+
         do {
             let userId = try await stateService.getAccountIdOrActiveId(userId: userId)
 
@@ -217,8 +238,16 @@ class DefaultClientService: ClientService {
             }
             return client
         } catch StateServiceError.noAccounts, StateServiceError.noActiveAccount {
-            // If there is no active account, or if no accounts exist,
-            // return the original client.
+            // If there's no accounts nor an active account, `isPreAuth` should be set. But to be
+            // safe, return a new client here and log an error for the missing `isPreAuth` parameter.
+            errorReporter.log(
+                error: BitwardenError.generalError(
+                    type: "Missing isPreAuth",
+                    message: "DefaultClientService.client(for:) was called without the isPreAuth " +
+                        "flag set and there's no active account. Consider if isPreAuth should be " +
+                        "set in this scenario."
+                )
+            )
             return clientBuilder.buildClient()
         }
     }

@@ -40,19 +40,45 @@ final class ClientServiceTests: BitwardenTestCase {
 
     /// `auth(for:)` returns a new `ClientAuthProtocol` for every user.
     func test_auth() async throws {
+        stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
+
         let auth = try await subject.auth()
         XCTAssertIdentical(auth, clientBuilder.clients.first?.clientAuth)
 
         let user2Auth = try await subject.auth(for: "2")
         XCTAssertNotIdentical(auth, user2Auth)
     }
 
+    /// `auth(for:)` logs an error if there's no accounts and the `isPreAuth` flag isn't set.
+    func test_auth_noAccountsNotPreAuth() async throws {
+        let auth1 = try await subject.auth()
+        let auth2 = try await subject.auth()
+        XCTAssertNotIdentical(auth1, auth2)
+
+        XCTAssertEqual(errorReporter.errors.count, 2)
+        XCTAssertEqual((errorReporter.errors[0] as NSError).domain, "General Error: Missing isPreAuth")
+        XCTAssertEqual((errorReporter.errors[1] as NSError).domain, "General Error: Missing isPreAuth")
+    }
+
+    /// `client(for:)` called concurrently doesn't crash.
+    func test_client_calledConcurrently() async throws {
+        // Calling `client(for:)` concurrently shouldn't throw an exception due to simultaneous
+        // access to shared state. Since it's a race condition, running it repeatedly should expose
+        // the failure if it's going to fail.
+        for _ in 0 ..< 5 {
+            async let concurrentTask1 = subject.auth(for: "1")
+            async let concurrentTask2 = subject.auth(for: "1")
+
+            _ = try await (concurrentTask1, concurrentTask2)
+        }
+    }
+
     /// Tests that `client(for:)` creates a new client if there is no active user/if there are no users.
     /// Also tests that `client(for:)` returns a user's existing client.
     /// Also tests that a `client(for:)` creates a new client if a user doesn't  have one.
     func test_client_multiple_users() async throws {
         // No active user.
-        let noActiveUserAuth = try await subject.auth()
+        let noActiveUserAuth = try await subject.auth(isPreAuth: true)
         let auth = clientBuilder.clients.first?.clientAuth
         XCTAssertIdentical(noActiveUserAuth, auth)
 
@@ -72,55 +98,83 @@ final class ClientServiceTests: BitwardenTestCase {
 
     /// `crypto(for:)` returns a new `ClientCryptoProtocol` for every user.
     func test_crypto() async throws {
+        stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
+
         let crypto = try await subject.crypto()
 
         XCTAssertIdentical(crypto, clientBuilder.clients.first?.clientCrypto)
-        let user2Crypto = try await subject.crypto(for: "1")
+        let user2Crypto = try await subject.crypto(for: "2")
         XCTAssertNotIdentical(crypto, user2Crypto)
     }
 
     /// `exporters(for:)` returns a new `ClientExportersProtocol` for every user.
     func test_exporters() async throws {
+        stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
+
         let exporters = try await subject.exporters()
         XCTAssertIdentical(exporters, clientBuilder.clients.first?.clientExporters)
 
-        let user2Exporters = try await subject.exporters(for: "1")
+        let user2Exporters = try await subject.exporters(for: "2")
         XCTAssertNotIdentical(exporters, user2Exporters)
     }
 
     /// `generators(for:)` returns a new `ClientGeneratorsProtocol` for every user.
     func test_generators() async throws {
+        stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
+
         let generators = try await subject.generators()
         XCTAssertIdentical(generators, clientBuilder.clients.first?.clientGenerators)
 
-        let user2Generators = try await subject.generators(for: "1")
+        let user2Generators = try await subject.generators(for: "2")
         XCTAssertNotIdentical(generators, user2Generators)
     }
 
     /// `platform(for:)` returns a new `ClientPlatformProtocol` for every user.
     func test_platform() async throws {
+        stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
+
         let platform = try await subject.platform()
         XCTAssertIdentical(platform, clientBuilder.clients.first?.clientPlatform)
 
-        let user2Platform = try await subject.platform(for: "1")
+        let user2Platform = try await subject.platform(for: "2")
         XCTAssertNotIdentical(platform, user2Platform)
     }
 
+    /// `removeClient(for:)` removes a cached client for a user.
+    func test_removeClient() async throws {
+        stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
+
+        let crypto = try await subject.crypto()
+        let crypto2 = try await subject.crypto()
+        // The same client should be returned for subsequent requests.
+        XCTAssertIdentical(crypto, crypto2)
+
+        try await subject.removeClient()
+        // After removing the client, a new client should be returned for the user.
+        let cryptoAfterRemoving = try await subject.crypto()
+
+        XCTAssertNotIdentical(crypto, cryptoAfterRemoving)
+    }
+
     /// `sends(for:)` returns a new `ClientVaultProtocol` for every user.
     func test_sends() async throws {
+        stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
+
         let sends = try await subject.sends()
         XCTAssertIdentical(sends, clientBuilder.clients.first?.clientSends)
 
-        let user2Sends = try await subject.sends(for: "1")
+        let user2Sends = try await subject.sends(for: "2")
         XCTAssertNotIdentical(sends, user2Sends)
     }
 
     /// `vault(for:)` returns a new `ClientVaultProtocol` for every user.
     func test_vault() async throws {
+        stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
+
         let vault = try await subject.vault()
         XCTAssertIdentical(vault, clientBuilder.clients.first?.clientVault)
 
-        let user2Vault = try await subject.vault(for: "1")
+        let user2Vault = try await subject.vault(for: "2")
         XCTAssertNotIdentical(vault, user2Vault)
     }
 }
@@ -19,6 +19,9 @@ enum BitwardenError {
 
         /// An error occurred with data from the API.
         case dataError = 3000
+
+        /// A general-purpose error.
+        case generalError = 4000
     }
 
     // MARK: Errors
@@ -37,6 +40,22 @@ enum BitwardenError {
         )
     }
 
+    /// A general-purpose error.
+    ///
+    /// - Parameters:
+    ///   - type: The type of error. This is used to group the errors in the Crashlytics dashboard.
+    ///   - message: A message describing the error that occurred.
+    ///
+    static func generalError(type: String, message: String) -> NSError {
+        NSError(
+            domain: "General Error: \(type)",
+            code: Code.generalError.rawValue,
+            userInfo: [
+                "ErrorMessage": message,
+            ]
+        )
+    }
+
     /// An error that occurred persisting the generator options.
     ///
     /// - Parameter error: The underlying error that caused the logout error.

@@ -4,6 +4,8 @@ import BitwardenSdk
 
 class MockClientService: ClientService {
     var mockAuth: MockClientAuth
+    var mockAuthIsPreAuth = false
+    var mockAuthUserId: String?
     var mockCrypto: MockClientCrypto
     var mockExporters: MockClientExporters
     var mockGenerators: MockClientGenerators
@@ -30,8 +32,10 @@ class MockClientService: ClientService {
         mockVault = vault
     }
 
-    func auth(for userId: String?) -> ClientAuthProtocol {
-        mockAuth
+    func auth(for userId: String?, isPreAuth: Bool) -> ClientAuthProtocol {
+        mockAuthIsPreAuth = isPreAuth
+        mockAuthUserId = userId
+        return mockAuth
     }
 
     func crypto(for userId: String?) -> ClientCryptoProtocol {