From 2f994f67629d2038540d948a60fc8be576d50cb6 Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Mon, 15 Dec 2025 19:02:23 -0300 Subject: [PATCH 1/4] PM-22202 Perform sync in Fido2 findCredentials only if there are Fido2 credentials with counter and sync is actually needed (30min has passed since last one). --- .../Platform/Services/ServiceContainer.swift | 2 ++ .../Fido2CredentialStoreService.swift | 28 +++++++++++++++---- .../Fido2CredentialStoreServiceTests.swift | 4 +++ 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/BitwardenShared/Core/Platform/Services/ServiceContainer.swift b/BitwardenShared/Core/Platform/Services/ServiceContainer.swift index 2adce4b9bf..110ea16578 100644 --- a/BitwardenShared/Core/Platform/Services/ServiceContainer.swift +++ b/BitwardenShared/Core/Platform/Services/ServiceContainer.swift @@ -801,6 +801,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le cipherService: cipherService, clientService: clientService, errorReporter: errorReporter, + stateService: stateService, syncService: syncService, ), ) @@ -809,6 +810,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le cipherService: cipherService, clientService: clientService, errorReporter: errorReporter, + stateService: stateService, syncService: syncService, ) #endif diff --git a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift index 7280efe1a6..4eb3c1b851 100644 --- a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift +++ b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift @@ -16,6 +16,9 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { /// The service used by the application to report non-fatal errors. private let errorReporter: ErrorReporter + /// The service used by the application to manage account state. + private let stateService: StateService + /// The service used to handle syncing vault data with the API private let syncService: SyncService @@ -26,16 +29,19 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { /// - cipherService: The service used to manage syncing and updates to the user's ciphers. /// - clientService: The service that handles common client functionality such as encryption and decryption. /// - errorReporter: The service used by the application to report non-fatal errors. + /// - stateService: The service used by the application to manage account state. /// - syncService: The service used to handle syncing vault data with the API. init( cipherService: CipherService, clientService: ClientService, errorReporter: ErrorReporter, + stateService: StateService, syncService: SyncService, ) { self.cipherService = cipherService self.clientService = clientService self.errorReporter = errorReporter + self.stateService = stateService self.syncService = syncService } @@ -55,12 +61,6 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { /// - ripId: The `ripId` to match the Fido2 credential `rpId`. /// - Returns: All the ciphers that matches the filter. func findCredentials(ids: [Data]?, ripId: String) async throws -> [BitwardenSdk.CipherView] { - do { - try await syncService.fetchSync(forceSync: false) - } catch { - errorReporter.log(error: error) - } - let activeCiphersWithFido2Credentials = try await cipherService.fetchAllCiphers() .filter(\.isActiveWithFido2Credentials) .asyncMap { cipher in @@ -83,6 +83,22 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { continue } + // Only perform sync if there are Fido2 credentials with counter. + if fido2CredentialAutofillViews.contains(where: { $0.hasCounter }) { + do { + let userId = try await stateService.getActiveAccountId() + let needsSync = try await syncService.needsSync(for: userId, onlyCheckLocalData: true) + if needsSync { + try await syncService.fetchSync(forceSync: false, isPeriodic: true) + + // After sync re-call this function to find the up-to-date credentials. + return try await findCredentials(ids: ids, ripId: ripId) + } + } catch { + errorReporter.log(error: error) + } + } + result.append(cipherView) } return result diff --git a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreServiceTests.swift b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreServiceTests.swift index bd53caa75f..0299e7652a 100644 --- a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreServiceTests.swift +++ b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreServiceTests.swift @@ -13,6 +13,7 @@ class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable var cipherService: MockCipherService! var clientService: MockClientService! var errorReporter: MockErrorReporter! + var stateService: MockStateService! var subject: Fido2CredentialStoreService! var syncService: MockSyncService! @@ -24,12 +25,14 @@ class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable cipherService = MockCipherService() clientService = MockClientService() errorReporter = MockErrorReporter() + stateService = MockStateService() syncService = MockSyncService() subject = Fido2CredentialStoreService( cipherService: cipherService, clientService: clientService, errorReporter: errorReporter, + stateService: stateService, syncService: syncService, ) } @@ -40,6 +43,7 @@ class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable cipherService = nil clientService = nil errorReporter = nil + stateService = nil subject = nil syncService = nil } From 7c01ff69d65ffdc7d03b030b9b92791be004f023 Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Mon, 15 Dec 2025 19:07:52 -0300 Subject: [PATCH 2/4] PM-22202 Moved logic to clean it up and improve performance. --- .../Fido2CredentialStoreService.swift | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift index 4eb3c1b851..d4ed3abc81 100644 --- a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift +++ b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift @@ -67,6 +67,9 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { try await self.clientService.vault().ciphers().decrypt(cipher: cipher) } + let userId = try await stateService.getActiveAccountId() + let needsSync = try await syncService.needsSync(for: userId, onlyCheckLocalData: true) + var result = [BitwardenSdk.CipherView]() for cipherView in activeCiphersWithFido2Credentials { let fido2CredentialAutofillViews = try await clientService.platform() @@ -83,17 +86,13 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { continue } - // Only perform sync if there are Fido2 credentials with counter. - if fido2CredentialAutofillViews.contains(where: { $0.hasCounter }) { + // Only perform sync if there are Fido2 credentials with counter and a sync is needed. + if fido2CredentialAutofillViews.contains(where: { $0.hasCounter }), needsSync { do { - let userId = try await stateService.getActiveAccountId() - let needsSync = try await syncService.needsSync(for: userId, onlyCheckLocalData: true) - if needsSync { - try await syncService.fetchSync(forceSync: false, isPeriodic: true) - - // After sync re-call this function to find the up-to-date credentials. - return try await findCredentials(ids: ids, ripId: ripId) - } + try await syncService.fetchSync(forceSync: false, isPeriodic: true) + + // After sync re-call this function to find the up-to-date credentials. + return try await findCredentials(ids: ids, ripId: ripId) } catch { errorReporter.log(error: error) } From 72582a79636640b89986518b026ee61db9cf6954 Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Tue, 16 Dec 2025 16:22:03 -0300 Subject: [PATCH 3/4] PM-22202 Avoid infinite loop while finding credentials for Fido2 flows and added unit tests. --- .../Fido2CredentialStoreService.swift | 59 +++++-- .../Fido2CredentialStoreServiceTests.swift | 158 +++++++++++++++++- 2 files changed, 198 insertions(+), 19 deletions(-) diff --git a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift index d4ed3abc81..f52bd8d765 100644 --- a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift +++ b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift @@ -61,14 +61,45 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { /// - ripId: The `ripId` to match the Fido2 credential `rpId`. /// - Returns: All the ciphers that matches the filter. func findCredentials(ids: [Data]?, ripId: String) async throws -> [BitwardenSdk.CipherView] { + try await findCredentials(ids: ids, ripId: ripId, shouldCheckSync: true) + } + + /// Saves a cipher credential that contains a Fido2 credential, either creating it or updating it to server. + /// - Parameter cred: Cipher/Credential to add/update. + func saveCredential(cred: BitwardenSdk.EncryptionContext) async throws { + if cred.cipher.id == nil { + try await cipherService.addCipherWithServer(cred.cipher, encryptedFor: cred.encryptedFor) + } else { + try await cipherService.updateCipherWithServer(cred.cipher, encryptedFor: cred.encryptedFor) + } + } + + // MARK: Private methods + + /// Finds active login ciphers that have Fido2 credentials, match the `ripId` and if `ids` is sent + /// then filters the one which the Fido2 `credentialId` matches some of the one in `ids`. + /// - Parameters: + /// - ids: An array of possible `credentialId` to filter credentials that matches one of them. + /// When `nil` the `credentialId` filter is not applied. + /// - ripId: The `ripId` to match the Fido2 credential `rpId`. + /// - shouldCheckSync: Whether it should check if sync is needed. This is particular useful to vaoid + /// inifinite loops by calling this method recursively. + /// - Returns: All the ciphers that matches the filter. + private func findCredentials( + ids: [Data]?, + ripId: String, + shouldCheckSync: Bool, + ) async throws -> [BitwardenSdk.CipherView] { let activeCiphersWithFido2Credentials = try await cipherService.fetchAllCiphers() .filter(\.isActiveWithFido2Credentials) .asyncMap { cipher in try await self.clientService.vault().ciphers().decrypt(cipher: cipher) } - let userId = try await stateService.getActiveAccountId() - let needsSync = try await syncService.needsSync(for: userId, onlyCheckLocalData: true) + var needsSync = false + if shouldCheckSync { + needsSync = await needsSyncCheckingLocally() + } var result = [BitwardenSdk.CipherView]() for cipherView in activeCiphersWithFido2Credentials { @@ -86,13 +117,13 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { continue } - // Only perform sync if there are Fido2 credentials with counter and a sync is needed. - if fido2CredentialAutofillViews.contains(where: { $0.hasCounter }), needsSync { + // Only perform sync if it's needed and there are Fido2 credentials with counter. + if needsSync, fido2CredentialAutofillViews.contains(where: \.hasCounter) { do { try await syncService.fetchSync(forceSync: false, isPeriodic: true) // After sync re-call this function to find the up-to-date credentials. - return try await findCredentials(ids: ids, ripId: ripId) + return try await findCredentials(ids: ids, ripId: ripId, shouldCheckSync: false) } catch { errorReporter.log(error: error) } @@ -103,13 +134,15 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { return result } - /// Saves a cipher credential that contains a Fido2 credential, either creating it or updating it to server. - /// - Parameter cred: Cipher/Credential to add/update. - func saveCredential(cred: BitwardenSdk.EncryptionContext) async throws { - if cred.cipher.id == nil { - try await cipherService.addCipherWithServer(cred.cipher, encryptedFor: cred.encryptedFor) - } else { - try await cipherService.updateCipherWithServer(cred.cipher, encryptedFor: cred.encryptedFor) + /// Whether the current user needs to perform a sync. It only performs local verifications. + /// - Returns: `true` if needed, `false` otherwise. + private func needsSyncCheckingLocally() async -> Bool { + do { + let userId = try await stateService.getActiveAccountId() + return try await syncService.needsSync(for: userId, onlyCheckLocalData: true) + } catch { + errorReporter.log(error: error) + return false } } } @@ -126,7 +159,7 @@ private extension Cipher { #if DEBUG /// A wrapper of a `Fido2CredentialStore` which adds debugging info for the `Fido2DebugginReportBuilder`. -class DebuggingFido2CredentialStoreService: Fido2CredentialStore { +final class DebuggingFido2CredentialStoreService: Fido2CredentialStore { let fido2CredentialStore: Fido2CredentialStore init(fido2CredentialStore: Fido2CredentialStore) { diff --git a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreServiceTests.swift b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreServiceTests.swift index 0299e7652a..576be48234 100644 --- a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreServiceTests.swift +++ b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreServiceTests.swift @@ -6,6 +6,7 @@ import XCTest // swiftlint:disable file_length @testable import BitwardenShared +@testable import BitwardenSharedMocks class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_length // MARK: Properties @@ -106,7 +107,7 @@ class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable } /// `.findCredentials(ids:ripId:)` returns the login ciphers that are active, have Fido2 credentials - /// and match the `ripId` and the credential `ids` if any. + /// and match the `ripId` and the credential `ids` if any. Syncs when needed and credentials have counter. func test_findCredentials() async throws { let expectedRpId = Fido2CredentialAutofillView.defaultRpId let expectedCredentialId = Data(repeating: 1, count: 16) @@ -118,24 +119,32 @@ class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable setupFindCredentials(cipherIdWithFullFido2Credential: expectedCipherId, expectedRpId: expectedRpId) + stateService.activeAccount = .fixture(profile: .fixture(userId: "user123")) + syncService.needsSyncResult = .success(true) + + var callCount = 0 clientService.mockPlatform.fido2Mock.decryptFido2AutofillCredentialsMocker - .withResult { cipherView in + .withResult { (cipherView: CipherView) -> [Fido2CredentialAutofillView] in guard let cipherId = cipherView.id else { return [] } + let hasExpectedCredentialId = cipherId == expectedCipherId + callCount += 1 return [ - .fixture( + Fido2CredentialAutofillView.fixture( credentialId: hasExpectedCredentialId ? expectedCredentialId : Data(repeating: 123, count: 16), cipherId: cipherId, rpId: expectedRpId, + hasCounter: true, ), - .fixture( + Fido2CredentialAutofillView.fixture( credentialId: Data(repeating: 123, count: 16), cipherId: cipherId, rpId: "test", + hasCounter: false, ), ] } @@ -143,18 +152,25 @@ class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable let result = try await subject.findCredentials(ids: credentialIds, ripId: expectedRpId) XCTAssertTrue(syncService.didFetchSync) + XCTAssertEqual(syncService.fetchSyncForceSync, false) + XCTAssertEqual(syncService.fetchSyncIsPeriodic, true) XCTAssertTrue(result.count == 1) XCTAssertTrue(result[0].id == expectedCipherId) + // Verify recursive call happened after sync (should be called twice: before and after sync) + XCTAssertTrue(callCount >= 2) } /// `.findCredentials(ids:ripId:)` returns the login ciphers that are active, have Fido2 credentials - /// and match the `ripId` and the credential `ids` if any. + /// and match the `ripId` and the credential `ids` if any. Doesn't sync when credentials have no counter. func test_findCredentials_noCredentialIds() async throws { let expectedRpId = Fido2CredentialAutofillView.defaultRpId let expectedCipherIds = ["3", "4"] setupFindCredentials(cipherIdWithFullFido2Credential: "4", expectedRpId: expectedRpId) + stateService.activeAccount = .fixture(profile: .fixture(userId: "user123")) + syncService.needsSyncResult = .success(true) + clientService.mockPlatform.fido2Mock.decryptFido2AutofillCredentialsMocker .withResult { cipherView in guard let cipherId = cipherView.id, @@ -166,11 +182,13 @@ class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable credentialId: Data(repeating: 1, count: 16), cipherId: cipherId, rpId: expectedRpId, + hasCounter: false, ), .fixture( credentialId: Data(repeating: 123, count: 16), cipherId: cipherId, rpId: "test", + hasCounter: false, ), ] } @@ -180,6 +198,8 @@ class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable XCTAssertTrue(result.count == 2) XCTAssertTrue(result[0].id == expectedCipherIds[0]) XCTAssertTrue(result[1].id == expectedCipherIds[1]) + // Verify sync was NOT performed because no credentials have counter + XCTAssertFalse(syncService.didFetchSync) } /// `.findCredentials(ids:ripId:)` returns empty if there are active Fido2 credentials. @@ -191,11 +211,137 @@ class Fido2CredentialStoreServiceTests: BitwardenTestCase { // swiftlint:disable XCTAssertTrue(result.isEmpty) } + /// `.findCredentials(ids:ripId:)` doesn't sync when credentials have no counter even if sync is needed. + func test_findCredentials_noCounterNoSync() async throws { + let expectedRpId = Fido2CredentialAutofillView.defaultRpId + let expectedCipherId = "4" + + setupFindCredentials(cipherIdWithFullFido2Credential: expectedCipherId, expectedRpId: expectedRpId) + + stateService.activeAccount = .fixture(profile: .fixture(userId: "user123")) + syncService.needsSyncResult = .success(true) + + clientService.mockPlatform.fido2Mock.decryptFido2AutofillCredentialsMocker + .withResult { cipherView in + guard let cipherId = cipherView.id else { + return [] + } + let hasExpectedCredentialId = cipherId == expectedCipherId + return [ + .fixture( + credentialId: hasExpectedCredentialId + ? Data(repeating: 1, count: 16) + : Data(repeating: 123, count: 16), + cipherId: cipherId, + rpId: expectedRpId, + hasCounter: false, + ), + ] + } + + let result = try await subject.findCredentials(ids: nil, ripId: expectedRpId) + + XCTAssertTrue(result.count == 4) + // Verify sync was NOT performed despite sync being needed, because no credentials have counter + XCTAssertFalse(syncService.didFetchSync) + } + + /// `.findCredentials(ids:ripId:)` doesn't sync when sync is not needed even if credentials have counter. + func test_findCredentials_syncNotNeeded() async throws { + let expectedRpId = Fido2CredentialAutofillView.defaultRpId + let expectedCipherId = "4" + + setupFindCredentials(cipherIdWithFullFido2Credential: expectedCipherId, expectedRpId: expectedRpId) + + stateService.activeAccount = .fixture(profile: .fixture(userId: "user123")) + syncService.needsSyncResult = .success(false) + + clientService.mockPlatform.fido2Mock.decryptFido2AutofillCredentialsMocker + .withResult { cipherView in + guard let cipherId = cipherView.id else { + return [] + } + let hasExpectedCredentialId = cipherId == expectedCipherId + return [ + .fixture( + credentialId: hasExpectedCredentialId + ? Data(repeating: 1, count: 16) + : Data(repeating: 123, count: 16), + cipherId: cipherId, + rpId: expectedRpId, + hasCounter: true, + ), + ] + } + + let result = try await subject.findCredentials(ids: nil, ripId: expectedRpId) + + XCTAssertTrue(result.count == 4) + // Verify sync was NOT performed despite credentials having counter, because sync not needed + XCTAssertFalse(syncService.didFetchSync) + } + + /// `.findCredentials(ids:ripId:)` logs error and continues when needsSync check fails. + func test_findCredentials_needsSyncError() async throws { + let expectedRpId = Fido2CredentialAutofillView.defaultRpId + let expectedCipherId = "4" + + setupFindCredentials(cipherIdWithFullFido2Credential: expectedCipherId, expectedRpId: expectedRpId) + + stateService.activeAccount = .fixture(profile: .fixture(userId: "user123")) + syncService.needsSyncResult = .failure(BitwardenTestError.example) + + clientService.mockPlatform.fido2Mock.decryptFido2AutofillCredentialsMocker + .withResult { cipherView in + guard let cipherId = cipherView.id else { + return [] + } + let hasExpectedCredentialId = cipherId == expectedCipherId + return [ + .fixture( + credentialId: hasExpectedCredentialId + ? Data(repeating: 1, count: 16) + : Data(repeating: 123, count: 16), + cipherId: cipherId, + rpId: expectedRpId, + hasCounter: true, + ), + ] + } + + let result = try await subject.findCredentials(ids: nil, ripId: expectedRpId) + + XCTAssertFalse(errorReporter.errors.isEmpty) + // Verify sync was NOT performed because needsSync check failed + XCTAssertFalse(syncService.didFetchSync) + XCTAssertTrue(result.count == 4) + } + /// `.findCredentials(ids:ripId)` throws when syncing. func test_findCredentials_throwsSync() async throws { syncService.fetchSyncResult = .failure(BitwardenTestError.example) + stateService.activeAccount = .fixture(profile: .fixture(userId: "user123")) + syncService.needsSyncResult = .success(true) + + let expectedRpId = Fido2CredentialAutofillView.defaultRpId + setupFindCredentials(cipherIdWithFullFido2Credential: "4", expectedRpId: expectedRpId) + + clientService.mockPlatform.fido2Mock.decryptFido2AutofillCredentialsMocker + .withResult { cipherView in + guard let cipherId = cipherView.id else { + return [] + } + return [ + .fixture( + credentialId: Data(repeating: 1, count: 16), + cipherId: cipherId, + rpId: expectedRpId, + hasCounter: true, + ), + ] + } - _ = try await subject.findCredentials(ids: nil, ripId: "something") + _ = try await subject.findCredentials(ids: nil, ripId: expectedRpId) XCTAssertFalse(errorReporter.errors.isEmpty) XCTAssertTrue(cipherService.fetchAllCiphersCalled) From 3c5fa1fa10e1dd90ab30f316d61990839fadc103 Mon Sep 17 00:00:00 2001 From: Federico Maccaroni Date: Fri, 19 Dec 2025 12:50:03 -0300 Subject: [PATCH 4/4] Update BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift Co-authored-by: Matt Czech --- .../Core/Vault/Services/Fido2CredentialStoreService.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift index f52bd8d765..bec8f5542f 100644 --- a/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift +++ b/BitwardenShared/Core/Vault/Services/Fido2CredentialStoreService.swift @@ -82,8 +82,8 @@ final class Fido2CredentialStoreService: Fido2CredentialStore { /// - ids: An array of possible `credentialId` to filter credentials that matches one of them. /// When `nil` the `credentialId` filter is not applied. /// - ripId: The `ripId` to match the Fido2 credential `rpId`. - /// - shouldCheckSync: Whether it should check if sync is needed. This is particular useful to vaoid - /// inifinite loops by calling this method recursively. + /// - shouldCheckSync: Whether it should check if sync is needed. This is particular useful to avoid + /// infinite loops by calling this method recursively. /// - Returns: All the ciphers that matches the filter. private func findCredentials( ids: [Data]?,