[PM-23301] feat: Share user session key across process boundaries#2661
[PM-23301] feat: Share user session key across process boundaries#2661fedemkr wants to merge 1 commit into
Conversation
Stores the decrypted user key in the keychain after unlock so app extensions can auto-unlock without user interaction when the vault timeout allows it. Clears the key on manual lock, timeout expiry, and when switching to an incompatible timeout value.
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed PM-23301, which introduces a Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| func unlockVaultWithSessionKey() async throws -> Bool { | ||
| let id = try await stateService.getActiveAccountId() | ||
| do { | ||
| let sessionKey = try await keychainService.getUserAuthKeyValue(for: .userSessionKey(userId: id)) | ||
| do { | ||
| try await unlockVault(method: .decryptedKey(decryptedUserKey: sessionKey), hadUserInteraction: false) | ||
| } catch { | ||
| try? await keychainService.deleteUserAuthKey(for: .userSessionKey(userId: id)) | ||
| throw error | ||
| } | ||
| return true | ||
| } catch KeychainServiceError.osStatusError(errSecItemNotFound), KeychainServiceError.keyNotFound { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
unlockVaultWithSessionKey() has no unit tests covering its three branches.
Details and fix
AuthRepositoryTests.swift contains zero references to unlockVaultWithSessionKey. Per Docs/Testing.md, "every type containing logic must be tested." Existing parallel functionality (unlockVaultWithNeverlockKey) is fully covered. Please add tests for:
- Returns
trueand callsunlockVault(method: .decryptedKey(...))when a key exists in the keychain. - Returns
falsewhen keychain throwsKeychainServiceError.osStatusError(errSecItemNotFound)orKeychainServiceError.keyNotFound— no exception escapes. - Deletes the keychain item and rethrows when
unlockVault(...)itself throws (e.g., simulate aninitializeUserCryptofailure). - The call passes
hadUserInteraction: false(important because this controls trust-device behavior).
This branch is exercised by extensions, so silent regressions here can leave users unable to autofill until they manually unlock the main app.
| case (_, true, false): | ||
| guard try await services.stateService.isAuthenticated(userId: userId) else { | ||
| return .landingSoftLoggedOut(email: activeAccount.profile.email) | ||
| } | ||
| // If a session key is present, auto-unlock without user interaction. | ||
| // checkSessionTimeouts() has already verified the timeout hasn't elapsed. | ||
| if try await services.authRepository.unlockVaultWithSessionKey() { | ||
| return .completeWithUserSessionKey | ||
| } | ||
| return .vaultUnlock( | ||
| activeAccount, | ||
| animated: animated, | ||
| attemptAutomaticBiometricUnlock: attemptAutomaticBiometricUnlock, | ||
| didSwitchAccountAutomatically: didSwitchAccountAutomatically, | ||
| ) |
There was a problem hiding this comment.
(_, true, false) redirect branch (auto-unlock via session key) has no test coverage in AuthRouterTests.swift.
Details and fix
This branch decides whether to bypass the unlock screen entirely — a security-relevant behavior. Suggested tests:
- When
unlockVaultWithSessionKey()returnstrue, the redirect resolves to.completeWithUserSessionKey. - When
unlockVaultWithSessionKey()returnsfalse, the redirect resolves to.vaultUnlock(...)with the correctanimated,attemptAutomaticBiometricUnlock, anddidSwitchAccountAutomaticallypropagated. - When
unlockVaultWithSessionKey()throws, the outercatchis invoked, the error is logged viaerrorReporter, and the route is.vaultUnlock(...). - When
isAuthenticatedreturnsfalse, the route is.landingSoftLoggedOut(email:)andunlockVaultWithSessionKeyis not called (avoids decrypting a soft-logged-out account's data).
Also worth adding a complementary test in AuthCoordinatorTests.swift for .completeWithUserSessionKey mirroring the existing test_navigate_completeWithNeverUnlockKey.
| private func listenForWillResignActive() { | ||
| Task { | ||
| for await _ in services.notificationCenterService.willResignActivePublisher() { | ||
| do { | ||
| let userId = try await self.services.stateService.getActiveAccountId() | ||
| try await services.vaultTimeoutService.setLastActiveTime(userId: userId) | ||
| } catch StateServiceError.noActiveAccount { | ||
| // No-op: nothing to do if there's no active account. | ||
| } catch { | ||
| services.errorReporter.log(error: error) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
listenForWillResignActive() has no test coverage in AppProcessorTests.swift.
Details and fix
This subscription gates whether the "foreground → app-switcher-kill" auto-unlock path correctly records the last-active timestamp. Without a test, a regression here silently breaks the timeout window check on cold restart from the switcher. Suggested tests (mirroring the existing didEnterBackground tests):
- Sending a value through
mockNotificationCenterService.willResignActiveSubjectcallsvaultTimeoutService.setLastActiveTime(userId:)with the active account id. - When
getActiveAccountId()throwsStateServiceError.noActiveAccount, no error is logged andsetLastActiveTimeis not called. - When
setLastActiveTime(userId:)throws an unrelated error, it is logged viaerrorReporter.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2661 +/- ##
===========================================
- Coverage 87.34% 40.96% -46.39%
===========================================
Files 1919 593 -1326
Lines 171022 32702 -138320
===========================================
- Hits 149386 13396 -135990
+ Misses 21636 19306 -2330 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-23301
📔 Objective
Stores the decrypted user encryption key in the keychain after a successful vault unlock, so that app extensions (AutoFill, Action, Share) can auto-unlock the vault without requiring user interaction, as long as the active session timeout policy permits it.
Key changes:
BitwardenKeychainItem.userSessionKey— new keychain item stored withkSecAttrAccessibleWhenUnlockedThisDeviceOnly, cleared on logout alongside other per-user items.SessionTimeoutValue.allowsUserSessionKeySharing— returnstruefor all timed timeout values;falsefor.immediately,.onAppRestart, and.never(which uses its own dedicated keychain path).AuthRepository.unlockVaultWithSessionKey()— reads the session key from the keychain and unlocks the vault. Returnsfalse(not an error) if no key is stored; deletes the stale key and rethrows on a decryption failure.AuthRoute.completeWithUserSessionKey/AuthRouter+Redirects— new redirect branch that auto-unlocks via the session key when the vault is locked but a valid key exists, bypassing the unlock screen entirely.NotificationCenterService.willResignActivePublisher()— new publisher forUIApplication.willResignActiveNotification;AppProcessorsubscribes to it to record the last-active timestamp, covering the foreground → app-switcher-kill path wheredidEnterBackgroundnever fires.