From 8e2dd986a7791947a8739c70d7d6cc6260c82cc7 Mon Sep 17 00:00:00 2001 From: Brant DeBow Date: Wed, 30 Oct 2024 17:00:01 -0400 Subject: [PATCH 1/2] [BITAU-191] Handle KeychainServiceError When Sync Has Been Turned Off on All Accounts --- .../AuthenticatorBridgeItemService.swift | 7 ++++-- .../AuthenticatorBridgeItemServiceTests.swift | 24 +++++++++++++++++++ .../MockSharedCryptographyService.swift | 7 ++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/AuthenticatorBridgeKit/AuthenticatorBridgeItemService.swift b/AuthenticatorBridgeKit/AuthenticatorBridgeItemService.swift index 983c7ff17..8a5237251 100644 --- a/AuthenticatorBridgeKit/AuthenticatorBridgeItemService.swift +++ b/AuthenticatorBridgeKit/AuthenticatorBridgeItemService.swift @@ -201,11 +201,14 @@ public class DefaultAuthenticatorBridgeItemService: AuthenticatorBridgeItemServi context: dataStore.persistentContainer.viewContext, request: fetchRequest ) - .tryMap { dataItems in + .map { dataItems in dataItems.compactMap(\.model) } .asyncTryMap { itemModel in - try await self.cryptoService.decryptAuthenticatorItems(itemModel) + guard let items = try? await self.cryptoService.decryptAuthenticatorItems(itemModel) else { + return [] + } + return items } .eraseToAnyPublisher() } diff --git a/AuthenticatorBridgeKit/Tests/AuthenticatorBridgeItemServiceTests.swift b/AuthenticatorBridgeKit/Tests/AuthenticatorBridgeItemServiceTests.swift index 28bfce955..b3051b8dd 100644 --- a/AuthenticatorBridgeKit/Tests/AuthenticatorBridgeItemServiceTests.swift +++ b/AuthenticatorBridgeKit/Tests/AuthenticatorBridgeItemServiceTests.swift @@ -247,6 +247,30 @@ final class AuthenticatorBridgeItemServiceTests: AuthenticatorBridgeKitTestCase XCTAssertEqual(results[0], combined) } + /// When the shared items publisher encounters a missing key, return an empty array rather than throwing the error. + /// This is likely due to the keys being removed as part of the last user turning off sync, so returning an + /// empty list is safer. + /// + func test_sharedItemsPublisher_keyFailureReturnsEmpty() async throws { + let initialItems = AuthenticatorBridgeItemDataView.fixtures().sorted { $0.id < $1.id } + try await subject.insertItems(initialItems, forUserId: "userId") + + cryptoService.errorToThrow = AuthenticatorKeychainServiceError.keyNotFound(SharedKeychainItem.authenticatorKey) + + var results: [[AuthenticatorBridgeItemDataView]] = [] + let publisher = try await subject.sharedItemsPublisher() + .sink( + receiveCompletion: { _ in }, + receiveValue: { value in + results.append(value) + } + ) + defer { publisher.cancel() } + + waitFor(results.count == 1) + XCTAssertEqual(results[0], []) + } + /// Verify that the shared items publisher does not publish any temporary items /// func test_sharedItemsPublisher_noTemporaryItems() async throws { diff --git a/AuthenticatorBridgeKit/Tests/TestHelpers/MockSharedCryptographyService.swift b/AuthenticatorBridgeKit/Tests/TestHelpers/MockSharedCryptographyService.swift index 4e5c5ff33..db693a0c4 100644 --- a/AuthenticatorBridgeKit/Tests/TestHelpers/MockSharedCryptographyService.swift +++ b/AuthenticatorBridgeKit/Tests/TestHelpers/MockSharedCryptographyService.swift @@ -6,10 +6,14 @@ import Foundation class MockSharedCryptographyService: SharedCryptographyService { var decryptCalled = false var encryptCalled = false + var errorToThrow: Error? func decryptAuthenticatorItems( _ items: [AuthenticatorBridgeItemDataModel] ) async throws -> [AuthenticatorBridgeItemDataView] { + if let errorToThrow { + throw errorToThrow + } decryptCalled = true return items.map { model in AuthenticatorBridgeItemDataView( @@ -27,6 +31,9 @@ class MockSharedCryptographyService: SharedCryptographyService { func encryptAuthenticatorItems( _ items: [AuthenticatorBridgeItemDataView] ) async throws -> [AuthenticatorBridgeItemDataModel] { + if let errorToThrow { + throw errorToThrow + } encryptCalled = true return items.map { view in AuthenticatorBridgeItemDataModel( From 72cd347b0121bffcd05650a3c4bbfde778e74935 Mon Sep 17 00:00:00 2001 From: Brant DeBow Date: Thu, 31 Oct 2024 13:06:38 -0400 Subject: [PATCH 2/2] Refactored approach to make the SharedCryptogaphyService better. This should fix the main error while leaving all other error handling intact --- .../AuthenticatorBridgeItemService.swift | 5 +--- .../SharedCryptographyService.swift | 2 ++ .../AuthenticatorBridgeItemServiceTests.swift | 24 ------------------- .../SharedCryptographyServiceTests.swift | 14 ++++++++++- 4 files changed, 16 insertions(+), 29 deletions(-) diff --git a/AuthenticatorBridgeKit/AuthenticatorBridgeItemService.swift b/AuthenticatorBridgeKit/AuthenticatorBridgeItemService.swift index 8a5237251..e605fc341 100644 --- a/AuthenticatorBridgeKit/AuthenticatorBridgeItemService.swift +++ b/AuthenticatorBridgeKit/AuthenticatorBridgeItemService.swift @@ -205,10 +205,7 @@ public class DefaultAuthenticatorBridgeItemService: AuthenticatorBridgeItemServi dataItems.compactMap(\.model) } .asyncTryMap { itemModel in - guard let items = try? await self.cryptoService.decryptAuthenticatorItems(itemModel) else { - return [] - } - return items + try await self.cryptoService.decryptAuthenticatorItems(itemModel) } .eraseToAnyPublisher() } diff --git a/AuthenticatorBridgeKit/SharedCryptographyService.swift b/AuthenticatorBridgeKit/SharedCryptographyService.swift index c91a51116..dad146478 100644 --- a/AuthenticatorBridgeKit/SharedCryptographyService.swift +++ b/AuthenticatorBridgeKit/SharedCryptographyService.swift @@ -57,6 +57,8 @@ public class DefaultAuthenticatorCryptographyService: SharedCryptographyService public func decryptAuthenticatorItems( _ items: [AuthenticatorBridgeItemDataModel] ) async throws -> [AuthenticatorBridgeItemDataView] { + guard !items.isEmpty else { return [] } + let key = try await sharedKeychainRepository.getAuthenticatorKey() let symmetricKey = SymmetricKey(data: key) diff --git a/AuthenticatorBridgeKit/Tests/AuthenticatorBridgeItemServiceTests.swift b/AuthenticatorBridgeKit/Tests/AuthenticatorBridgeItemServiceTests.swift index b3051b8dd..28bfce955 100644 --- a/AuthenticatorBridgeKit/Tests/AuthenticatorBridgeItemServiceTests.swift +++ b/AuthenticatorBridgeKit/Tests/AuthenticatorBridgeItemServiceTests.swift @@ -247,30 +247,6 @@ final class AuthenticatorBridgeItemServiceTests: AuthenticatorBridgeKitTestCase XCTAssertEqual(results[0], combined) } - /// When the shared items publisher encounters a missing key, return an empty array rather than throwing the error. - /// This is likely due to the keys being removed as part of the last user turning off sync, so returning an - /// empty list is safer. - /// - func test_sharedItemsPublisher_keyFailureReturnsEmpty() async throws { - let initialItems = AuthenticatorBridgeItemDataView.fixtures().sorted { $0.id < $1.id } - try await subject.insertItems(initialItems, forUserId: "userId") - - cryptoService.errorToThrow = AuthenticatorKeychainServiceError.keyNotFound(SharedKeychainItem.authenticatorKey) - - var results: [[AuthenticatorBridgeItemDataView]] = [] - let publisher = try await subject.sharedItemsPublisher() - .sink( - receiveCompletion: { _ in }, - receiveValue: { value in - results.append(value) - } - ) - defer { publisher.cancel() } - - waitFor(results.count == 1) - XCTAssertEqual(results[0], []) - } - /// Verify that the shared items publisher does not publish any temporary items /// func test_sharedItemsPublisher_noTemporaryItems() async throws { diff --git a/AuthenticatorBridgeKit/Tests/SharedCryptographyServiceTests.swift b/AuthenticatorBridgeKit/Tests/SharedCryptographyServiceTests.swift index 7596561aa..e374d6da4 100644 --- a/AuthenticatorBridgeKit/Tests/SharedCryptographyServiceTests.swift +++ b/AuthenticatorBridgeKit/Tests/SharedCryptographyServiceTests.swift @@ -30,6 +30,17 @@ final class SharedCryptographyServiceTests: AuthenticatorBridgeKitTestCase { // MARK: Tests + /// Verify that `SharedCryptographyService.decryptAuthenticatorItems()' returns and empty + /// array when the input is an empty array even when the authenticator key is missing. + /// + func test_decryptAuthenticatorItems_returnsEmpty() async throws { + try sharedKeychainRepository.deleteAuthenticatorKey() + await assertAsyncDoesNotThrow { + let result = try await subject.decryptAuthenticatorItems([]) + XCTAssertEqual(result, []) + } + } + /// Verify that `SharedCryptographyService.decryptAuthenticatorItems(:)` correctly /// decrypts an encrypted array of `AuthenticatorBridgeItemDataModel`. /// @@ -44,11 +55,12 @@ final class SharedCryptographyServiceTests: AuthenticatorBridgeKitTestCase { /// when the `SharedKeyRepository` authenticator key is missing. /// func test_decryptAuthenticatorItems_throwsKeyMissingError() async throws { + let encryptedItems = try await subject.encryptAuthenticatorItems(items) let error = AuthenticatorKeychainServiceError.keyNotFound(SharedKeychainItem.authenticatorKey) try sharedKeychainRepository.deleteAuthenticatorKey() await assertAsyncThrows(error: error) { - _ = try await subject.decryptAuthenticatorItems([]) + _ = try await subject.decryptAuthenticatorItems(encryptedItems) } }