Backport 'release/2024.10-rc1' (#1108)

Co-authored-by: Katherine Bertelsen <[email protected]> Co-authored-by: Matt Czech <[email protected]> Co-authored-by: Pranish <[email protected]> Co-authored-by: aj-rosado <[email protected]> Co-authored-by: Phil Cappelli <[email protected]> Co-authored-by: Álison Fernandes <[email protected]>
bitwarden · Nov 5, 2024 · 6da628e · 6da628e
1 parent 91e8329
commit 6da628e
Show file tree

Hide file tree

Showing 10 changed files with 91 additions and 40 deletions.
diff --git a/BitwardenShared/Core/Auth/Services/AuthService.swift b/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),

diff --git a/BitwardenShared/Core/Platform/Services/ClientService.swift b/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 {

diff --git a/BitwardenShared/Core/Platform/Services/ClientServiceTests.swift b/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")

diff --git a/BitwardenShared/Core/Platform/Services/TestHelpers/MockClientService.swift b/BitwardenShared/Core/Platform/Services/TestHelpers/MockClientService.swift
@@ -9,6 +9,8 @@ class MockClientService: ClientService {
     var mockCrypto: MockClientCrypto
     var mockExporters: MockClientExporters
     var mockGenerators: MockClientGenerators
+    var mockGeneratorsIsPreAuth = false
+    var mockGeneratorsUserId: String?
     var mockPlatform: MockClientPlatformService
     var mockSends: MockClientSends
     var mockVault: MockClientVaultService
@@ -46,8 +48,10 @@ class MockClientService: ClientService {
         mockExporters
     }
 
-    func generators(for userId: String?) -> ClientGeneratorsProtocol {
-        mockGenerators
+    func generators(for userId: String?, isPreAuth: Bool) -> ClientGeneratorsProtocol {
+        mockGeneratorsIsPreAuth = isPreAuth
+        mockGeneratorsUserId = userId
+        return mockGenerators
     }
 
     func platform(for userId: String?) -> ClientPlatformService {

diff --git a/BitwardenShared/Core/Tools/Repositories/GeneratorRepository.swift b/BitwardenShared/Core/Tools/Repositories/GeneratorRepository.swift
@@ -171,7 +171,7 @@ extension DefaultGeneratorRepository: GeneratorRepository {
     // MARK: Generator
 
     func generateMasterPassword() async throws -> String {
-        try await clientService.generators().passphrase(
+        try await clientService.generators(isPreAuth: false).passphrase(
             settings: PassphraseGeneratorRequest(
                 numWords: 3,
                 wordSeparator: "-",
@@ -182,15 +182,15 @@ extension DefaultGeneratorRepository: GeneratorRepository {
     }
 
     func generatePassphrase(settings: PassphraseGeneratorRequest) async throws -> String {
-        try await clientService.generators().passphrase(settings: settings)
+        try await clientService.generators(isPreAuth: false).passphrase(settings: settings)
     }
 
     func generatePassword(settings: PasswordGeneratorRequest) async throws -> String {
-        try await clientService.generators().password(settings: settings)
+        try await clientService.generators(isPreAuth: false).password(settings: settings)
     }
 
     func generateUsername(settings: UsernameGeneratorRequest) async throws -> String {
-        try await clientService.generators().username(settings: settings)
+        try await clientService.generators(isPreAuth: false).username(settings: settings)
     }
 
     func getPasswordGenerationOptions() async throws -> PasswordGenerationOptions {

diff --git a/BitwardenShared/Core/Vault/Repositories/VaultRepository.swift b/BitwardenShared/Core/Vault/Repositories/VaultRepository.swift
@@ -1071,7 +1071,7 @@ extension DefaultVaultRepository: VaultRepository {
 
     func needsSync() async throws -> Bool {
         let userId = try await stateService.getActiveAccountId()
-        return try await syncService.needsSync(for: userId)
+        return try await syncService.needsSync(for: userId, onlyCheckLocalData: true)
     }
 
     func refreshTOTPCode(for key: TOTPKeyModel) async throws -> LoginTOTPState {

diff --git a/BitwardenShared/Core/Vault/Repositories/VaultRepositoryTests.swift b/BitwardenShared/Core/Vault/Repositories/VaultRepositoryTests.swift
@@ -760,6 +760,33 @@ class VaultRepositoryTests: BitwardenTestCase { // swiftlint:disable:this type_b
         XCTAssertTrue(isDisabled)
     }
 
+    /// `needsSync()` Calls the sync service to check it.
+    func test_needsSync() async throws {
+        stateService.activeAccount = .fixture()
+        syncService.needsSyncResult = .success(true)
+        let needsSync = try await subject.needsSync()
+        XCTAssertTrue(needsSync)
+        XCTAssertTrue(syncService.needsSyncOnlyCheckLocalData)
+    }
+
+    /// `needsSync()` throws when no active account.
+    func test_needsSync_throwsNoActiveAccount() async throws {
+        stateService.activeAccount = nil
+        await assertAsyncThrows(error: StateServiceError.noActiveAccount) {
+            _ = try await subject.needsSync()
+        }
+    }
+
+    /// `needsSync()` throws when sync service throws.
+    func test_needsSync_throwsSyncService() async throws {
+        stateService.activeAccount = .fixture()
+        syncService.needsSyncResult = .failure(BitwardenTestError.example)
+        await assertAsyncThrows(error: BitwardenTestError.example) {
+            _ = try await subject.needsSync()
+        }
+        XCTAssertTrue(syncService.needsSyncOnlyCheckLocalData)
+    }
+
     /// `isVaultEmpty()` throws an error if one occurs.
     func test_isVaultEmpty_error() async {
         cipherService.cipherCountResult = .failure(BitwardenTestError.example)

diff --git a/BitwardenShared/Core/Vault/Services/SyncService.swift b/BitwardenShared/Core/Vault/Services/SyncService.swift
@@ -57,11 +57,13 @@ protocol SyncService: AnyObject {
     func fetchUpsertSyncSend(data: SyncSendNotification) async throws
 
     /// Does a given account need a sync?
+    /// - Parameters:
+    ///   - userId: The user id of the account
+    ///   - onlyCheckLocalData: If `true` it will only check local data to establish whether sync is needed.
+    ///   Otherwise, it can also perform requests to server to have additional data to check.
     ///
-    /// - Parameter userId: The user id of the account.
     /// - Returns: A bool indicating if the user needs a sync or not.
-    ///
-    func needsSync(for userId: String) async throws -> Bool
+    func needsSync(for userId: String, onlyCheckLocalData: Bool) async throws -> Bool
 }
 
 // MARK: - SyncServiceDelegate
@@ -188,14 +190,8 @@ class DefaultSyncService: SyncService {
         self.timeProvider = timeProvider
     }
 
-    /// Determine if a full sync is necessary.
-    ///
-    /// - Parameters:
-    ///   - userId: The user ID of the account to sync.
-    /// - Returns: Whether a sync should be performed.
-    ///
-    func needsSync(for userId: String) async throws -> Bool {
-        try await needsSync(forceSync: false, userId: userId)
+    func needsSync(for userId: String, onlyCheckLocalData: Bool) async throws -> Bool {
+        try await needsSync(forceSync: false, onlyCheckLocalData: onlyCheckLocalData, userId: userId)
     }
 
     // MARK: Private
@@ -205,13 +201,22 @@ class DefaultSyncService: SyncService {
     /// - Parameters:
     ///   - forceSync: Whether syncing should be forced, bypassing the account revision and minimum
     ///     sync interval checks.
+    ///   - onlyCheckLocalData: If `true` it will only check local data to establish whether sync is needed.
+    ///     Otherwise, it can also perform requests to server to have additional data to check.
     ///   - userId: The user ID of the account to sync.
     /// - Returns: Whether a sync should be performed.
     ///
-    private func needsSync(forceSync: Bool, userId: String) async throws -> Bool {
-        guard let lastSyncTime = try await stateService.getLastSyncTime(userId: userId), !forceSync else { return true }
-        guard lastSyncTime.addingTimeInterval(Constants.minimumSyncInterval)
-            < timeProvider.presentTime else { return false }
+    private func needsSync(forceSync: Bool, onlyCheckLocalData: Bool = false, userId: String) async throws -> Bool {
+        guard !forceSync, let lastSyncTime = try await stateService.getLastSyncTime(userId: userId) else {
+            return true
+        }
+        guard lastSyncTime.addingTimeInterval(Constants.minimumSyncInterval) < timeProvider.presentTime else {
+            return false
+        }
+        guard !onlyCheckLocalData else {
+            return true
+        }
+
         do {
             guard let accountRevisionDate = try await accountAPIService.accountRevisionDate()
             else { return true }

diff --git a/BitwardenShared/Core/Vault/Services/SyncServiceTests.swift b/BitwardenShared/Core/Vault/Services/SyncServiceTests.swift
@@ -727,6 +727,19 @@ class SyncServiceTests: BitwardenTestCase {
         try await subject.fetchUpsertSyncSend(data: notification)
         XCTAssertEqual(sendService.syncSendWithServerId, "id")
     }
+
+    /// `needsSync(forceSync:onlyCheckLocalData:userId:)` returns `true` when
+    /// only checking local data and not enough time hasn't passed since the last sync.
+    func test_needsSync_onlyCheckLocalData() async throws {
+        stateService.activeAccount = .fixture()
+        let lastSync = timeProvider.presentTime.addingTimeInterval(-(Constants.minimumSyncInterval + 1))
+        stateService.lastSyncTimeByUserId["1"] = try XCTUnwrap(
+            lastSync
+        )
+        let needsSync = try await subject.needsSync(for: "1", onlyCheckLocalData: true)
+        XCTAssertTrue(needsSync)
+        XCTAssertTrue(client.requests.isEmpty)
+    }
 }
 
 class MockSyncServiceDelegate: SyncServiceDelegate {

diff --git a/BitwardenShared/Core/Vault/Services/TestHelpers/MockSyncService.swift b/BitwardenShared/Core/Vault/Services/TestHelpers/MockSyncService.swift
@@ -27,6 +27,7 @@ class MockSyncService: SyncService {
     var fetchUpsertSyncSendResult: Result<Void, Error> = .success(())
 
     var needsSyncResult: Result<Bool, Error> = .success(false)
+    var needsSyncOnlyCheckLocalData: Bool = false
 
     func fetchSync(forceSync: Bool) async throws {
         didFetchSync = true
@@ -64,7 +65,8 @@ class MockSyncService: SyncService {
         return try fetchUpsertSyncSendResult.get()
     }
 
-    func needsSync(for userId: String) async throws -> Bool {
-        try needsSyncResult.get()
+    func needsSync(for userId: String, onlyCheckLocalData: Bool) async throws -> Bool {
+        needsSyncOnlyCheckLocalData = onlyCheckLocalData
+        return try needsSyncResult.get()
     }
 }