Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BITAU-191] Handle KeychainServiceError When Sync Has Been Turned Off on All Accounts #1094

Merged
merged 4 commits into from
Oct 31, 2024
+23 −2
Merged

[BITAU-191] Handle KeychainServiceError When Sync Has Been Turned Off on All Accounts #1094

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e2dd98
[BITAU-191] Handle KeychainServiceError When Sync Has Been Turned Off…
brant-livefront Oct 30, 2024
72cd347
Refactored approach to make the SharedCryptogaphyService better. This…
brant-livefront Oct 31, 2024
73ddd6a
Merge branch 'main' into brant/BITAU-191-handle-keychainserviceerror-…
brant-livefront Oct 31, 2024
b1643ec
Merge branch 'main' into brant/BITAU-191-handle-keychainserviceerror-…
brant-livefront Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions AuthenticatorBridgeKit/AuthenticatorBridgeItemService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, thinking about this some more. Will this trap other exceptions that the Authenticator app expects to catch and display an error? Should you only catch the keychain error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we keep going with this approach, then I agree with Victor that this should only catch KeychainServiceError and, if possible, the specific error for this situation so it doesn't hide any other potential bugs; which in this case I imagine is KeychainServiceError.keyNotFound.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran into this bug while working on an Authenticator PR that clarified the reason we're still seeing this when turning off sync:

  • The publisher is asking for the list to be decrypted even when there are no items - i.e. it sends [] to be decrypted.
  • The crypto service is looking for the key first thing, which throws the error. But in the case of an empty list, it never needed the key to decrypt.

So when a user turns off sync (or launches the first time with the feature flag enabled) we should not have any items to be decrypted. But because it's looking for the key, it will still fail here.

I'm going to re-work this PR to address the issue in the cypto service, and leave this error handling in place. 👍

return []
}
return items
}
.eraseToAnyPublisher()
}
Expand Down
24 changes: 24 additions & 0 deletions AuthenticatorBridgeKit/Tests/AuthenticatorBridgeItemServiceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions AuthenticatorBridgeKit/Tests/TestHelpers/MockSharedCryptographyService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down
Loading