[BITAU-153] [BITAU-144] [BITAU-208] Enable Background Syncing When the Phone Is Locked #1125
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1125 +/- ##
==========================================
+ Coverage 89.45% 89.49% +0.04%
==========================================
Files 682 682
Lines 43189 43212 +23
==========================================
+ Hits 38634 38672 +38
+ Misses 4555 4540 -15 ☔ View full report in Codecov by Sentry. |
|
New Issues
|
There was a problem hiding this comment.
Code looks good to me. Here are the scenarios I tested with this change on a physical device:
Fresh install new version starting as logged out
- foregrounded/unlocked vault: ✅
- foregrounded/locked vault: ✅
- backgrounded/unlocked vault: ✅
- backgrounded/locked value: ✅
- device locked/unlocked vault: ✅
- device locked/locked vault: ✅
Installed old version and logged out:
- foregrounded/unlocked vault: ✅
- foregrounded/locked vault: ✅
- backgrounded/unlocked vault: ✅
- backgrounded/locked value: ✅
- device locked/unlocked vault: ✅
- device locked/locked vault: ❌ Error: BitwardenShared.KeychainServiceError.osStatusError(-25308)
Updated to new version:
- foregrounded/unlocked vault: ✅
- foregrounded/locked vault: ✅
- backgrounded/unlocked vault: ✅
- backgrounded/locked value: ✅
- device locked/unlocked vault: ✅
- device locked/locked vault: ❌ Error: BitwardenShared.KeychainServiceError.osStatusError(-25308)
Auth Token refresh (updated my devices clock to 4 hours in the future, and foregrounded the app. Did not enter master password):
- foregrounded/unlocked vault: ✅
- foregrounded/locked vault: ✅
- backgrounded/unlocked vault: ✅
- backgrounded/locked value: ✅
- device locked/unlocked vault: ✅
- device locked/locked vault: ✅
I needed to comment the assertion failure in the OSLogErrorReporter to allow it to move forward for DEBUG, but this failure is based on other factors and is expected.
|
One thing I noticed when testing, which is only tangentially related to this since this PR is about keychain access, is that the authenticator sync service does not run if you start the app from cold and don't unlock it (but you have a valid auth token from a previous session). We may need to account for this scenario. |
… is locked at startup
…en-the-phone-is-locked
|
Per @victor-livefront's comment above, I added a ticket to track this. The last commit has a new test that accounts for this scenario and a fix for the issue. Here's the scenario:
Expected: Sync should sync the new cipher update because it already has the vault key when sync was setup. |
There was a problem hiding this comment.
Looks good. I'm not aware of any attacks on this protection level change but I would like @matt-livefront and @KatherineInCode to check it as well in case I'm missing something before approving.
Additionally, given that accessToken can now be used in background shouldn't we also allow refreshToken as well so when the accessToken is expired it can be refreshed? Otherwise sync will fail on background when trying to get the refreshToken on expired access token scenario given its protection level.
|
@matt-livefront @fedemkr I've moved the |
@fedemkr No concerns from me. |
🎟️ Tracking
BITAU-144
BITAU-153
📔 Objective
There is currently an error in the way we receive Push Notifications when the device is completely locked. All of the keys in the app are currently stored with the protection level
kSecAttrAccessibleWhenUnlockedThisDeviceOnly. This level of protection means that if we get a Push when the device is locked, none of the keys will be accessible. The first thing the NotificationService does is checkstateService.isAuthenticated()which immediately tries to access the accessToken. This throws a security error, which is caught, logged, and fails silently. Therefore all background Push notifications will fail if the device is locked.In addition, when we setup the Authenticator vault key to facilitate syncing to the Authenticator app, it was stored like all other keys with
kSecAttrAccessibleWhenUnlockedThisDeviceOnly. This was a known issue that would break syncing in the background if the device is locked.This PR attempts to fix both of these situations by allowing some keys to be accessed in the background even if the device is locked:
KeychainItem.kSecAttrAccessibleAfterFirstUnlockThisDeviceOnlyfor theaccessToken(which fixes Push) and theauthenticatorVaultKey(which enables full background syncing to Authenticator).With these changes in place, users can receive background pushes when the device is locked and the app will be able to process them. And those pushes can trigger the AuthenticatorSyncService to use it's key to unlock/lock the vault and perform the sync.
Existing users will already have accessToken with the old permissions:
accessTokens with the old protection level.setAccessTokenlike all keychain operations will first delete the existing item before adding the new value.accessTokenwith the new protection level. But this should be thoroughly tested.Testing Steps
Here's a quick rundown of what I did to test this case:
accessToken's protection levelaccessTokenhasn't been re-written and therefore has the old protection level)401. This causes the app to fetch and store a newaccessToken⏰ 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