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

[brant-livefront]

🎟️ Tracking

BITAU-191

📔 Objective

Luke found an issue in testing that was causing the Authenticator app to fail:

• Turn on sync for an account in the PM app
• Open Authenticator and see the results
• Switch back to the PM app and turn off syncing
  • Make sure this is the only account that had sync turned on
  • This is important because the authenticator key is only deleted when the last account sync is turned off. If you still have at least one account, you'll still have the key.
• Switch back to the Authenticator app and pull to refresh

The issue is that when the Authenticator app refreshes the data, the decryption key has been deleted. This causes the keychain to throw KeychainServiceError because the key doesn't exist. However, this is an expected condition because we delete the keys when turning off syncing.

The fix in this PR is to catch any error occurring in the decryption process and simply return []. This allows the Authenticator app to continue without any synced values, rather than falling into an error state.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[victor-livefront]

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?

[fedemkr]

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.

[brant-livefront]

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. 👍

Open question

[fedemkr]

🤔 @brant-livefront If I'm understanding correctly by doing this: turning off Sync in the PM app will "hide" all shared items from the Authenticator app, right?
So, is that the goal of the "Sync" toggle? Because some users may be confused on the wording "Allow authenticator syncing", i.e. if the vault has already synced and then I toggle it off then someone could expect the already synced ciphers to be available on the Authenticator app. Is Product aware of how this will work?

[brant-livefront]

🤔 @brant-livefront If I'm understanding correctly by doing this: turning off Sync in the PM app will "hide" all shared items from the Authenticator app, right? So, is that the goal of the "Sync" toggle? Because some users may be confused on the wording "Allow authenticator syncing", i.e. if the vault has already synced and then I toggle it off then someone could expect the already synced ciphers to be available on the Authenticator app. Is Product aware of how this will work?

Yes, this is the intended behavior. In fact, we not only hide but delete all the items from the shared store. Toggling sync to off means saying "Don't allow syncing" - i.e. get rid of synced items. If we left the Authenticator items in place, there'd be no way for a user to change their mind and remove the items from sync.

[victor-livefront]

Looks good!

[fedemkr]

LGTM 👍 Although if we find a KeychainServiceError again in Crashlytics, will it be easy to spot this is the place it was originated from?
I'm approving and I'll leave it up to you to whether a more particular error should be thrown here in case the authenticator key is nil or that's highly unlikely to happen thus it can stay as is 😄

[brant-livefront]

LGTM 👍 Although if we find a KeychainServiceError again in Crashlytics, will it be easy to spot this is the place it was originated from? I'm approving and I'll leave it up to you to whether a more particular error should be thrown here in case the authenticator key is nil or that's highly unlikely to happen thus it can stay as is 😄

I think it's highly unlikely because we delete the key when we delete all the shared items. So either you have the key because you have items, or you don't have the key and this fix will return immediately without throwing.

However, I did think about actually removing the throws entirely on this method and returning [] if the key is missing (which is the only try). While this means we would not be bubbling up that error, it would be safer overall. What do you think about that?

[fedemkr]

IMO we either bubble up the error or log the error there and return [] but logging should always be applied so we know something went wrong. As to which to choose I think it's a matter of UI/UX, because if you bubble it up then the user sees an error alert so they know something went wrong and in the other hand if we return [] they won't see the alert but these ciphers won't be shown so the user may question themselves as to why they don't load and no error is shown.
For now, I think it's ok to bubble it up since it'd be highly unlikely to happen and we can revisit this if that happens again. Do you agree?

[brant-livefront]

@fedemkr I agree with that. Now that we've dealt with the likely problem, the other cases are truly exceptional. So it makes sense to bubble them up. 👍