PM-27241: feat: TDE encryption v2#6821
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6821 +/- ##
==========================================
- Coverage 85.80% 85.75% -0.06%
==========================================
Files 836 839 +3
Lines 59408 59494 +86
Branches 8672 8676 +4
==========================================
+ Hits 50977 51020 +43
- Misses 5442 5483 +41
- Partials 2989 2991 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
f2d2d21 to
cb67878
Compare
b5f6dd7 to
30fb187
Compare
30fb187 to
6016e1b
Compare
6016e1b to
bf0e8a6
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR introduces the v2 TDE encryption flow for new SSO users behind the Code Review Details
|
| .map { response -> | ||
| // Clear the 'should trust device' flag, since the SDK trusted the device above. | ||
| authDiskSource.storeShouldTrustDevice(userId = userId, shouldTrustDevice = null) | ||
| this | ||
| .unlockVault( | ||
| accountCryptographicState = response.accountCryptographicState, | ||
| accountProfile = profile, | ||
| initUserCryptoMethod = InitUserCryptoMethod.DecryptedKey( | ||
| decryptedUserKey = response.userKey, | ||
| ), | ||
| ) | ||
| .also { result -> | ||
| if (result is VaultUnlockResult.Success) { | ||
| authDiskSource.storeAccountKeys( | ||
| userId = userId, | ||
| accountKeys = response.accountCryptographicState.accountKeysJson, | ||
| ) | ||
|
|
||
| // Storing the private key here for legacy purposes, the | ||
| // `accountKeysJson` stored above will be used for most purposes. | ||
| authDiskSource.storePrivateKey( | ||
| userId = userId, | ||
| privateKey = response.accountCryptographicState.privateKey, | ||
| ) | ||
| if (shouldTrustDevice) { | ||
| authDiskSource.storeDeviceKey( | ||
| userId = userId, | ||
| deviceKey = response.deviceKey, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
unlockVault failures are silently swallowed and reported as NewSsoUserResult.Success.
Details and fix
registerUserForTdeV2 returns Result<VaultUnlockResult>. When postKeysForTdeRegistration succeeds but unlockVault returns a VaultUnlockError variant (e.g., GenericError, InvalidStateError), the .map { ... } block still completes and produces Result.success(VaultUnlockResult.GenericError(...)). The outer createNewSsoUser then runs .fold(onSuccess = { NewSsoUserResult.Success }, ...), which ignores the wrapped error variant and returns NewSsoUserResult.Success. The caller (TrustedDeviceViewModel.handleContinueClick) treats Success as a fully completed login and navigates the user forward, but the vault is locked, account keys were never stored, and shouldTrustDevice has already been cleared on disk.
The AuthRepository.createNewSsoUser KDoc explicitly states: "Upon success the new user will also have the vault automatically unlocked for them." That contract is broken here.
The V2 JIT password flow already handles this correctly — see registerUserForJitPasswordV2 around line 1314:
.flatMap { response ->
when (val result = vaultRepository.unlockVaultWithMasterPassword(password)) {
VaultUnlockResult.Success -> response.asSuccess()
is VaultUnlockError -> {
(result.error ?: IllegalStateException("Failed to unlock vault"))
.asFailure()
}
}
}Suggest converting the .map { ... } here to a .flatMap { ... } that turns a VaultUnlockError into Result.failure, matching that pattern. The on-success disk writes can then run inside the Success branch (or in a subsequent .onSuccess).
|
Thanks @quexten & @SaintPatrck |

🎟️ Tracking
PM-27241
📔 Objective
This PR adds the updated v2 encryption flow for a SSO TDE new user.