[BITAU-141] [BITAU-142] [BITAU-143] [BITAU-150] Add Vault Unlock #1032
Conversation
…and save to keychain
… to be more clear
…ant/add-vault-background-unlock
…ant/add-vault-background-unlock
…ant/add-vault-background-unlock
…ant/add-vault-background-unlock
…ant/add-vault-background-unlock
|
No New Or Fixed Issues Found |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
==========================================
+ Coverage 89.29% 89.30% +0.01%
==========================================
Files 670 670
Lines 42056 42098 +42
==========================================
+ Hits 37553 37595 +42
Misses 4503 4503 ☔ View full report in Codecov by Sentry. |
| let useKey = vaultTimeoutService.isLocked(userId: userId) | ||
| if useKey { | ||
| try await authRepository.unlockVaultWithAuthenticatorVaultKey(userId: userId) | ||
| } | ||
| let items = try await decryptTOTPs(ciphers, userId: userId) | ||
| if useKey { | ||
| await vaultTimeoutService.lockVault(userId: userId) | ||
| } |
There was a problem hiding this comment.
🤔 Hmmm TBH I wouldn't include an automatic unlock without the user explicitly knowing, I feel like this would at least need to be an opt-in feature so the user consents on having this behavior and we only automatically do this if the vault is currently unlocked. Could you check with Product about this?
There was a problem hiding this comment.
When we originally put together a POC around this behavior, we presented options for warning the user about this (similar to the never lock) to Kyle Spearrin and Brian Gentry. Where we landed was that the toggle to turn on the feature was the opt in enough, and doesn't leave the entire PM permanently unlocked like never lock does.
There was a problem hiding this comment.
Oh alright, if they agree then I'll approve; thanks for clarifying! 😄
There was a problem hiding this comment.
@victor-livefront something else now that I re-read it is if let items = try await decryptTOTPs(ciphers, userId: userId) throws then the vault would remain unlocked. I think a do .... catch should be added here to lock the vault if the func throws and then re-throw the error for the caller.
There was a problem hiding this comment.
That's a good callout. Brant and I discussed threading here but overlooked the throw.
There was a problem hiding this comment.
@fedemkr @victor-livefront OK, I've pushed a commit that I think addresses all of these issues:
- I've moved the fetching of the account to this method. If that fails, it will throw immediately and get caught/logged by the CipherPublisherTask's catch.
- I've changed the
keychainService.getAuthenticatorVaultKeyto return a non-optionalString. It was always throwingKeychainServiceError.keyNotFoundin the case of the key missing, so there's no reason for it to be optional- if this is thrown in the course of unlocking their vault, it will now be caught by the
catchinwriteCiphersand immediately logged.
- if this is thrown in the course of unlocking their vault, it will now be caught by the
- Likewise, if decrypting ciphers or writing them to the shared store fails, we're catching that explicitly and logging it right here in the method, so we should have insight into what's failing
- Lastly, outside of all of that, we check the
useKeyat the end of the method and make sure to lock the Vault back up even if any errors have occurred.
| let authenticatorKey = try await keychainService.getAuthenticatorVaultKey(userId: userId) ?? "" | ||
| try await unlockVault(method: .decryptedKey(decryptedUserKey: authenticatorKey), hadUserInteraction: false) |
There was a problem hiding this comment.
🤔 If keychainService.getAuthenticatorVaultKey is nil then unlocking will definitely fail so it feels like a guard should be added here and throw an error if that happens so we have a more specific error as to why this failed.
| XCTAssertTrue(vaultTimeoutService.isLocked(userId: "1")) | ||
| } | ||
|
|
||
| /// Verify that `writeCiphers()` correctly catches and logs errors that occur in `replaceAllItems`. The user's vault is |
There was a problem hiding this comment.
🤦♂️ Fixed in latest.
🎟️ Tracking
[BITAU-141] Add ability to read vault key from user's unlocked vault and save to keychain
[BITAU-142] Unlock Vault using stored Authenticator Vault Key
[BITAU-143] Delete Authenticator Vault Key on Turning Sync Off
[BITAU-150] Create Keychain Item for Authenticator Vault Key
📔 Objective
This PR adds the ability to handle background and locked-vault syncs for the user. There's a few different parts:
authenticatorVaultKey.authenticatorVaultKeyNote: There is one known issue with this PR (I'll handle it as part of BITAU-153): Since the keys in the PM app are stored with an accessibility level of
kSecAttrAccessibleWhenUnlockedThisDeviceOnlythey are not available if the app is truly in the background (i.e. an update comes in via Push notification while the app is backgrounded and the device is locked). Thus we can't unlock the vault if the app is not in the foreground. I will need to add a refactor to move just this one key tokSecAttrAccessibleAfterFirstUnlockThisDeviceOnlywhich does allow background access.⏰ Reminders before review
🦮 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