Merge branch 'main' into brant/authenticator-sync-integration-test

.github/workflows/build.yml

@@ -23,7 +23,6 @@ on:
       compiler-flags:
         description: "Compiler Flags - e.g. 'DEBUG_MENU FEATURE2'"
         type: string
-        default: "DEBUG_MENU"
       base_version_number:
         description: "Base Version Number - Will be added to the calculated version number"
         type: number

BitwardenShared/Core/Auth/Models/Response/ResponseValidationErrorModel.swift

@@ -12,7 +12,7 @@ struct ResponseValidationErrorModel: Codable, Equatable {
     let error: String
 
     /// A string that provides a description of the error.
-    let errorDescription: String
+    let errorDescription: String?
 
     /// An `ErrorModel` object that provides more details about the error.
     let errorModel: ErrorModel

BitwardenShared/Core/Auth/Models/Response/ResponseValidationErrorModelTests.swift

@@ -23,4 +23,32 @@ class ResponseValidationErrorModelTests: BitwardenTestCase {
         let subject = try ResponseValidationErrorModel.decoder.decode(ResponseValidationErrorModel.self, from: json)
         XCTAssertEqual(subject.errorModel.message, "Username or password is incorrect. Try again.")
     }
+
+    /// Tests the successful decoding of a JSON response with no error description.
+    func test_decode_success_missingErrorDescription() throws {
+        let json = """
+        {
+          "error": "invalid_grant",
+          "ErrorModel": {
+            "Message": "Two-step token is invalid. Try again.",
+            "Object": "error"
+          }
+        }
+        """
+        let subject = try ResponseValidationErrorModel.decoder.decode(
+            ResponseValidationErrorModel.self,
+            from: XCTUnwrap(json.data(using: .utf8))
+        )
+        XCTAssertEqual(
+            subject,
+            ResponseValidationErrorModel(
+                error: "invalid_grant",
+                errorDescription: nil,
+                errorModel: ErrorModel(
+                    message: "Two-step token is invalid. Try again.",
+                    object: "error"
+                )
+            )
+        )
+    }
 }

BitwardenShared/Core/Auth/Services/AuthService.swift

@@ -407,11 +407,11 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
             minNumber: 1,
             minSpecial: 0
         )
-        codeVerifier = try await clientService.generators().password(settings: passwordSettings)
+        codeVerifier = try await clientService.generators(isPreAuth: true).password(settings: passwordSettings)
         let codeChallenge = Data(codeVerifier.utf8)
             .generatedHashBase64Encoded(using: SHA256.self)
             .urlEncoded()
-        let state = try await clientService.generators().password(settings: passwordSettings)
+        let state = try await clientService.generators(isPreAuth: true).password(settings: passwordSettings)
 
         let queryItems = [
             URLQueryItem(name: "client_id", value: Constants.clientType),

BitwardenShared/Core/Platform/Services/AuthenticatorSyncService.swift

@@ -47,6 +47,13 @@ actor DefaultAuthenticatorSyncService: NSObject, AuthenticatorSyncService {
     /// The service to get server-specified configuration.
     private let configService: ConfigService
 
+    /// A task that sets up syncing for a user.
+    ///
+    /// Using an actor-protected `Task` ensures that multiple threads don't attempt
+    /// to setup sync at the same time. `enableSyncForUserId(_)` `await`s
+    /// the result of this task before adding the next sync setup to the end of it.
+    private var enableSyncTask: Task<Void, Never>?
+
     /// The service used by the application to report non-fatal errors.
     private let errorReporter: ErrorReporter
 
@@ -246,19 +253,34 @@ actor DefaultAuthenticatorSyncService: NSObject, AuthenticatorSyncService {
     /// - Parameter userId: The userId of the user whose sync status is being determined.
     ///
     private func determineSyncForUserId(_ userId: String) async throws {
-        guard try await stateService.getSyncToAuthenticator(userId: userId) else {
+        if try await stateService.getSyncToAuthenticator(userId: userId) {
+            enableSyncForUserId(userId)
+        } else {
             cipherPublisherTasks[userId]?.cancel()
             cipherPublisherTasks.removeValue(forKey: userId)
             try await keychainRepository.deleteAuthenticatorVaultKey(userId: userId)
             try await authBridgeItemService.deleteAllForUserId(userId)
             try await deleteKeyIfSyncingIsOff()
-            return
         }
+    }
+
+    /// Enable sync for the provided userId.
+    ///
+    /// - Parameter userId: The userId of the user whose sync is being enabled.
+    ///
+    private func enableSyncForUserId(_ userId: String) {
+        guard !vaultTimeoutService.isLocked(userId: userId) else { return }
+
+        enableSyncTask = Task { [enableSyncTask] in
+            _ = await enableSyncTask?.result
 
-        if !vaultTimeoutService.isLocked(userId: userId) {
-            try await createAuthenticatorKeyIfNeeded()
-            try await createAuthenticatorVaultKeyIfNeeded(userId: userId)
-            subscribeToCipherUpdates(userId: userId)
+            do {
+                try await createAuthenticatorKeyIfNeeded()
+                try await createAuthenticatorVaultKeyIfNeeded(userId: userId)
+                subscribeToCipherUpdates(userId: userId)
+            } catch {
+                errorReporter.log(error: error)
+            }
         }
     }
 

BitwardenShared/Core/Platform/Services/AuthenticatorSyncServiceTests.swift

@@ -51,6 +51,7 @@ final class AuthenticatorSyncServiceTests: BitwardenTestCase { // swiftlint:disa
     override func tearDown() {
         super.tearDown()
 
+        subject = nil
         authBridgeItemService = nil
         authRepository = nil
         cipherDataStore = nil
@@ -60,7 +61,6 @@ final class AuthenticatorSyncServiceTests: BitwardenTestCase { // swiftlint:disa
         keychainRepository = nil
         sharedKeychainRepository = nil
         stateService = nil
-        subject = nil
         vaultTimeoutService = nil
     }
 
@@ -464,7 +464,9 @@ final class AuthenticatorSyncServiceTests: BitwardenTestCase { // swiftlint:disa
         // Unsubscribe from sync, wait for items to be deleted
         stateService.syncToAuthenticatorByUserId["1"] = false
         stateService.syncToAuthenticatorSubject.send(("1", false))
-        waitFor((authBridgeItemService.storedItems["1"]?.isEmpty) ?? false)
+        try await waitForAsync {
+            (self.authBridgeItemService.storedItems["1"]?.isEmpty) ?? false
+        }
 
         // Sending additional updates should not appear in Store
         cipherDataStore.cipherSubjectByUserId["1"]?.send([
@@ -481,6 +483,37 @@ final class AuthenticatorSyncServiceTests: BitwardenTestCase { // swiftlint:disa
         XCTAssertTrue(authBridgeItemService.storedItems["1"]?.isEmpty ?? false)
     }
 
+    /// The sync service should be properly handling multiple publishes which could happen on multiple threads.
+    /// By generating a `send` on both the sync status and the vault unlock, the service will receive two
+    /// simultaneous attempts to determine syncing.
+    ///
+    @MainActor
+    func test_determineSyncForUserId_threadSafetyCheck() async throws {
+        setupInitialState()
+        await subject.start()
+
+        for _ in 0 ..< 4 {
+            async let result1: Void = stateService.syncToAuthenticatorSubject.send(("1", true))
+            async let result2: Void = vaultTimeoutService.vaultLockStatusSubject.send(
+                VaultLockStatus(isVaultLocked: false, userId: "1")
+            )
+            await _ = (result1, result2)
+        }
+
+        cipherDataStore.cipherSubjectByUserId["1"]?.send([
+            .fixture(
+                id: "1234",
+                login: .fixture(
+                    username: "[email protected]",
+                    totp: "totp"
+                )
+            ),
+        ])
+        try await waitForAsync {
+            self.authBridgeItemService.storedItems["1"]?.first != nil
+        }
+    }
+
     /// When user "1" has sync turned on and user "2" unlocks their vault, the service should not take
     /// any action because "1" has a locked vault and "2" doesn't have sync turned on.
     ///

BitwardenShared/Core/Platform/Services/ClientService.swift

@@ -32,10 +32,13 @@ protocol ClientService {
 
     /// Returns a `ClientGeneratorsProtocol` for generator 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 `ClientGeneratorsProtocol` for generator data tasks.
     ///
-    func generators(for userId: String?) async throws -> ClientGeneratorsProtocol
+    func generators(for userId: String?, isPreAuth: Bool) async throws -> ClientGeneratorsProtocol
 
     /// Returns a `ClientPlatformService` for client platform tasks.
     ///
@@ -68,18 +71,12 @@ protocol ClientService {
 // MARK: Extension
 
 extension ClientService {
-    /// Returns a `ClientAuthProtocol` for auth data tasks.
-    ///
-    func auth() async throws -> ClientAuthProtocol {
-        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 {
+    func auth(isPreAuth: Bool = false) async throws -> ClientAuthProtocol {
         try await auth(for: nil, isPreAuth: isPreAuth)
     }
 
@@ -97,8 +94,11 @@ extension ClientService {
 
     /// Returns a `ClientGeneratorsProtocol` for generator data tasks.
     ///
-    func generators() async throws -> ClientGeneratorsProtocol {
-        try await generators(for: nil)
+    /// - Parameter isPreAuth: Whether the client is being used for a user prior to authentication
+    ///     (when the user's ID doesn't yet exist). This primarily will happen in SSO flows.
+    ///
+    func generators(isPreAuth: Bool = false) async throws -> ClientGeneratorsProtocol {
+        try await generators(for: nil, isPreAuth: isPreAuth)
     }
 
     /// Returns a `ClientPlatformService` for client platform tasks.
@@ -203,8 +203,8 @@ actor DefaultClientService: ClientService {
         try await client(for: userId).exporters()
     }
 
-    func generators(for userId: String?) async throws -> ClientGeneratorsProtocol {
-        try await client(for: userId).generators()
+    func generators(for userId: String?, isPreAuth: Bool = false) async throws -> ClientGeneratorsProtocol {
+        try await client(for: userId, isPreAuth: isPreAuth).generators()
     }
 
     func platform(for userId: String?) async throws -> ClientPlatformService {

BitwardenShared/Core/Platform/Services/ClientServiceTests.swift

@@ -352,7 +352,7 @@ final class ClientServiceTests: BitwardenTestCase { // swiftlint:disable:this ty
     func test_generators() async throws {
         stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
 
-        let generators = try await subject.generators()
+        let generators = try await subject.generators(isPreAuth: false)
         XCTAssertIdentical(generators, clientBuilder.clients.first?.clientGenerators)
 
         let user2Generators = try await subject.generators(for: "2")

BitwardenShared/Core/Platform/Services/ServiceContainer.swift

@@ -109,6 +109,9 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
     /// The service for managing the polices for the user.
     let policyService: PolicyService
 
+    /// The helper used for app rehydration.
+    let rehydrationHelper: RehydrationHelper
+
     /// The repository used by the application to manage send data for the UI layer.
     public let sendRepository: SendRepository
 
@@ -185,6 +188,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
     ///   - notificationCenterService: The service used by the application to access the system's notification center.
     ///   - notificationService: The service used by the application to handle notifications.
     ///   - pasteboardService: The service used by the application for sharing data with other apps.
+    ///   - rehydrationHelper: The helper used for app rehydration.
     ///   - policyService: The service for managing the polices for the user.
     ///   - sendRepository: The repository used by the application to manage send data for the UI layer.
     ///   - settingsRepository: The repository used by the application to manage data for the UI layer.
@@ -231,6 +235,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
         notificationService: NotificationService,
         pasteboardService: PasteboardService,
         policyService: PolicyService,
+        rehydrationHelper: RehydrationHelper,
         sendRepository: SendRepository,
         settingsRepository: SettingsRepository,
         stateService: StateService,
@@ -275,6 +280,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
         self.notificationService = notificationService
         self.pasteboardService = pasteboardService
         self.policyService = policyService
+        self.rehydrationHelper = rehydrationHelper
         self.sendRepository = sendRepository
         self.settingsRepository = settingsRepository
         self.stateService = stateService
@@ -324,6 +330,12 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
             keychainRepository: keychainRepository
         )
 
+        let rehydrationHelper = DefaultRehydrationHelper(
+            errorReporter: errorReporter,
+            stateService: stateService,
+            timeProvider: timeProvider
+        )
+
         let environmentService = DefaultEnvironmentService(errorReporter: errorReporter, stateService: stateService)
         let collectionService = DefaultCollectionService(collectionDataStore: dataStore, stateService: stateService)
         let settingsService = DefaultSettingsService(settingsDataStore: dataStore, stateService: stateService)
@@ -675,6 +687,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
             notificationService: notificationService,
             pasteboardService: pasteboardService,
             policyService: policyService,
+            rehydrationHelper: rehydrationHelper,
             sendRepository: sendRepository,
             settingsRepository: settingsRepository,
             stateService: stateService,

BitwardenShared/Core/Platform/Services/Services.swift

@@ -31,6 +31,7 @@ typealias Services = HasAPIService
     & HasOrganizationAPIService
     & HasPasteboardService
     & HasPolicyService
+    & HasRehydrationHelper
     & HasSendRepository
     & HasSettingsRepository
     & HasStateService
@@ -254,6 +255,12 @@ protocol HasPolicyService {
     var policyService: PolicyService { get }
 }
 
+/// Protocol for an object that provides a `RehydrationHelper`.
+protocol HasRehydrationHelper {
+    /// The helper for app rehydration.
+    var rehydrationHelper: RehydrationHelper { get }
+}
+
 /// Protocol for an object that provides a `SendRepository`.
 ///
 public protocol HasSendRepository {