[PM-32009] feat: Add infrastructure for new vault item types#6828
[PM-32009] feat: Add infrastructure for new vault item types#6828SaintPatrck wants to merge 2 commits intomainfrom
Conversation
Register pm-32009-new-item-types feature flag and add foundational support for Bank Account, Driver's License, and Passport cipher types across the network model, SDK mapping, and UI layers. Network: CipherTypeJson enum values (6-8), SyncResponseJson nested data classes, CipherJsonRequest fields, LinkedIdTypeJson entries (600-812). SDK: Defensive mapping that gracefully skips unsupported cipher types during sync decryption (SDK types not yet available). UI: VaultBankAccountType enum, VaultItemCipherType/CreateVaultItemType extensions, ItemType state classes in VaultAddEditViewModel, string resources, and exhaustive when-branch updates across ViewModels.
C1: Gate new item types behind pm-32009-new-item-types feature flag in VaultViewModel and VaultItemListingViewModel excluded options. I1: Add Timber.w logging when cipher conversion fails during sync. I2: Add isSdkSupported property to ItemType and guard save path in VaultAddEditViewModel to prevent crash on unsupported types.
SaintPatrck
added
the
ai-review-vnext
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the network/model/enum/feature-flag foundation for Bank Account, Driver's License, and Passport cipher types in Code Review Details
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6828 +/- ##
==========================================
- Coverage 85.58% 85.32% -0.27%
==========================================
Files 830 834 +4
Lines 61509 59179 -2330
Branches 8592 8611 +19
==========================================
- Hits 52642 50492 -2150
+ Misses 5900 5708 -192
- Partials 2967 2979 +12
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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| CipherTypeJson.BANK_ACCOUNT, | ||
| CipherTypeJson.DRIVERS_LICENSE, | ||
| CipherTypeJson.PASSPORT, | ||
| -> throw IllegalArgumentException("SDK mapping not yet available for $this") |
There was a problem hiding this comment.
toSdkCipherType() throws; callers outside toEncryptedSdkCipherList are not defensive and can crash on new-type ciphers.
Details and fix
The PR description states "the mapping layer defensively drops unknown types instead of throwing, keeping sync resilient." That promise only holds for toEncryptedSdkCipherList (line 389–394). The single-item path still throws via toSdkCipherType(), and several call sites invoke toEncryptedSdkCipher() directly without a runCatching wrapper:
SdkCipherRepository.list()→map { it.toEncryptedSdkCipher() }(line 27). The SDK invokes this during its own crypto operations.SdkCipherRepository.get(id)→?.toEncryptedSdkCipher()(line 20).VaultRepositoryImpl.exportVaultDataToString→.map { it.toEncryptedSdkCipher() }(line 457).VaultRepositoryImpl.exportVaultDataToCxf→.map { it.toEncryptedSdkCipher() }(line 518).CipherManagerImpl.getCipher→syncResponseCipher.toEncryptedSdkCipher()(line 335).
Trigger path: A web vault with the server-side flag enabled stores a bank account cipher. Mobile syncs (storing the CipherTypeJson.BANK_ACCOUNT row in vaultDiskSource regardless of the mobile feature flag). User then exports their vault or the SDK enumerates ciphers for a crypto op → IllegalArgumentException propagates.
Suggested fix: either (a) filter new types out of the disk source before they reach these call sites, or (b) make SdkCipherRepository.list()/get() and the export paths use the same mapNotNull { runCatching { ... }.getOrNull() } pattern, or (c) relocate the exhaustive branch to a non-throwing toSdkCipherTypeOrNull() and adjust callers to skip nulls. This aligns the runtime behavior with the resilience claim in the PR description.
| CipherTypeJson.BANK_ACCOUNT, | ||
| CipherTypeJson.DRIVERS_LICENSE, | ||
| CipherTypeJson.PASSPORT, | ||
| -> throw IllegalArgumentException("SDK mapping not yet available for $this") |
There was a problem hiding this comment.
toSdkCipherType() throws; callers outside toEncryptedSdkCipherList are not defensive and can crash on new-type ciphers.
Details and fix
The PR description states "the mapping layer defensively drops unknown types instead of throwing, keeping sync resilient." That promise only holds for toEncryptedSdkCipherList (line 389–394). The single-item path still throws via toSdkCipherType(), and several call sites invoke toEncryptedSdkCipher() directly without a runCatching wrapper:
SdkCipherRepository.list()→map { it.toEncryptedSdkCipher() }(line 27). The SDK invokes this during its own crypto operations.SdkCipherRepository.get(id)→?.toEncryptedSdkCipher()(line 20).VaultRepositoryImpl.exportVaultDataToString→.map { it.toEncryptedSdkCipher() }(line 457).VaultRepositoryImpl.exportVaultDataToCxf→.map { it.toEncryptedSdkCipher() }(line 518).CipherManagerImpl.getCipher→syncResponseCipher.toEncryptedSdkCipher()(line 335).
Trigger path: A web vault with the server-side flag enabled stores a bank account cipher. Mobile syncs (storing the CipherTypeJson.BANK_ACCOUNT row in vaultDiskSource regardless of the mobile feature flag). User then exports their vault or the SDK enumerates ciphers for a crypto op → IllegalArgumentException propagates.
Suggested fix: either (a) filter new types out of the disk source before they reach these call sites, or (b) make SdkCipherRepository.list()/get() and the export paths use the same mapNotNull { runCatching { ... }.getOrNull() } pattern, or (c) relocate the exhaustive branch to a non-throwing toSdkCipherTypeOrNull() and adjust callers to skip nulls. This aligns the runtime behavior with the resilience claim in the PR description.
| fun List<SyncResponseJson.Cipher>.toEncryptedSdkCipherList(): List<Cipher> = | ||
| map { it.toEncryptedSdkCipher() } | ||
| mapNotNull { | ||
| runCatching { it.toEncryptedSdkCipher() } |
There was a problem hiding this comment.
Why would this need to be wrapped in runCatching
| VaultItemCipherType.DRIVERS_LICENSE -> | ||
| VaultAddEditState.ViewState.Content.ItemType.DriversLicense() | ||
| VaultItemCipherType.PASSPORT -> | ||
| VaultAddEditState.ViewState.Content.ItemType.Passport() |
There was a problem hiding this comment.
Ding on formatting
| .entries | ||
| .find { vaultBankAccountType -> | ||
| vaultBankAccountType.name.lowercaseWithoutSpacesOrUnderscores == | ||
| this.lowercaseWithoutSpacesOrUnderscores |
There was a problem hiding this comment.
Can we just give these values?
enum class VaultBankAccountType(val value: String) {
SELECT(value = "select"),
...
companion object {
fun parse(value: String?): VaultBankAccountType =
VaultBankAccountType
.entries
.firstOrNull { it.value.equals(other = value, ignoreCase = true) }
?: VaultBankAccountType.OTHER
}
}| val nameOnAccount: String?, | ||
|
|
||
| @SerialName("accountType") | ||
| val accountType: String?, |
There was a problem hiding this comment.
Should this be enumerated?
| sshKey: SyncResponseJson.Cipher.SshKey? = createMockSshKey(number = number), | ||
| bankAccount: SyncResponseJson.Cipher.BankAccount? = null, | ||
| driversLicense: SyncResponseJson.Cipher.DriversLicense? = null, | ||
| passport: SyncResponseJson.Cipher.Passport? = null, |
There was a problem hiding this comment.
Can we add the helper functions?
fun createMockBankAccount(
number: Int,
bankName: String? = "mockBankName-$number",
nameOnAccount: String? = "mockNameOnAccount-$number",
accountType: String? = "mockAccountType-$number",
accountNumber: String? = "mockAccountNumber-$number",
routingNumber: String? = "mockRoutingNumber-$number",
branchNumber: String? = "mockBranchNumber-$number",
pin: String? = "mockPin-$number",
swiftCode: String? = "mokSwiftCode-$number",
iban: String? = "mockIban-$number",
bankContactPhone: String? = "mockBankContractPhone-$number",
): SyncResponseJson.Cipher.BankAccount =
SyncResponseJson.Cipher.BankAccount(
bankName = bankName,
nameOnAccount = nameOnAccount,
accountType = accountType,
accountNumber = accountNumber,
routingNumber = routingNumber,
branchNumber = branchNumber,
pin = pin,
swiftCode = swiftCode,
iban = iban,
bankContactPhone = bankContactPhone,
)
There was a problem hiding this comment.
Ohh, there is one literally a few lines down.
| val issuingCountry: String = "", | ||
| val issuingState: String = "", | ||
| val expirationMonth: String = "", | ||
| val expirationYear: String = "", |
There was a problem hiding this comment.
All this still should also be made for VaultItemState.ViewState.Content.ItemType., right?
2 participants
🎟️ Tracking
PM-32009
📔 Objective
First of three stacked PRs introducing Bank Account, Driver's License, and Passport cipher types. This PR lands the foundation only — network/model plumbing, enums, feature-flag registration, and exhaustive-when branches — so the UI and vault-integration PRs can be reviewed in isolation on top of it.
The entire effort ships behind the
pm-32009-new-item-typesfeature flag so infrastructure can merge well ahead of user-visible surface area.Why it looks the way it does
CipherTypevalues. The mapping layer defensively drops unknown types instead of throwing, keeping sync resilient while SDK variants are being added.whenbranches are updated across every affected ViewModel. Adding the enum values forces compile-time coverage everywhere — reviewers can trust that no call site silently falls into anelse.LinkedIdTypeJsonentries (600–812) mirror the server's contract so linked-field resolution works the moment data arrives, without a second round-trip PR.Stacked on:
mainFollowed by:
new-item-types/phase-05-07_cipher-type-ui(UI screens)