Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import com.bitwarden.vault.SecureNote
import com.bitwarden.vault.SecureNoteType
import com.bitwarden.vault.SshKey
import com.bitwarden.vault.UriMatchType
import timber.log.Timber

/**
* Converts a Bitwarden SDK [Cipher] object to a corresponding
Expand Down Expand Up @@ -66,6 +67,9 @@ fun Cipher.toEncryptedNetworkCipher(
card = card?.toEncryptedNetworkCard(),
key = key,
sshKey = sshKey?.toEncryptedNetworkSshKey(),
bankAccount = null,
driversLicense = null,
passport = null,
archivedDate = archivedDate,
encryptedFor = encryptedFor,
)
Expand Down Expand Up @@ -96,6 +100,9 @@ fun Cipher.toEncryptedNetworkCipherResponse(
card = card?.toEncryptedNetworkCard(),
attachments = attachments?.toNetworkAttachmentList(),
sshKey = sshKey?.toEncryptedNetworkSshKey(),
bankAccount = null,
driversLicense = null,
passport = null,
shouldOrganizationUseTotp = organizationUseTotp,
shouldEdit = edit,
revisionDate = revisionDate,
Expand Down Expand Up @@ -380,7 +387,11 @@ private fun CipherType.toNetworkCipherType(): CipherTypeJson =
* Bitwarden SDK [Cipher] objects.
*/
fun List<SyncResponseJson.Cipher>.toEncryptedSdkCipherList(): List<Cipher> =
map { it.toEncryptedSdkCipher() }
mapNotNull {
runCatching { it.toEncryptedSdkCipher() }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this need to be wrapped in runCatching

.onFailure { e -> Timber.w(e, "Failed to convert cipher: %s", it.id) }
.getOrNull()
}

/**
* Converts a [SyncResponseJson.Cipher] object to a corresponding
Expand Down Expand Up @@ -607,6 +618,10 @@ fun CipherTypeJson.toSdkCipherType(): CipherType =
CipherTypeJson.CARD -> CipherType.CARD
CipherTypeJson.IDENTITY -> CipherType.IDENTITY
CipherTypeJson.SSH_KEY -> CipherType.SSH_KEY
CipherTypeJson.BANK_ACCOUNT,
CipherTypeJson.DRIVERS_LICENSE,
CipherTypeJson.PASSPORT,
-> throw IllegalArgumentException("SDK mapping not yet available for $this")
Comment on lines +621 to +624
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ IMPORTANT: Single-cipher 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.

Comment on lines +621 to +624
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ IMPORTANT: Single-cipher 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.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,21 @@ enum class CreateVaultItemType(
*/
SSH_KEY(BitwardenString.type_ssh_key),

/**
* A bank account cipher.
*/
BANK_ACCOUNT(BitwardenString.type_bank_account),

/**
* A driver's license cipher.
*/
DRIVERS_LICENSE(BitwardenString.type_drivers_license),

/**
* A passport cipher.
*/
PASSPORT(BitwardenString.type_passport),

/**
* A cipher item folder
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,8 @@ fun CreateVaultItemType.toVaultItemCipherTypeOrNull(): VaultItemCipherType? = wh
CreateVaultItemType.IDENTITY -> VaultItemCipherType.IDENTITY
CreateVaultItemType.SECURE_NOTE -> VaultItemCipherType.SECURE_NOTE
CreateVaultItemType.SSH_KEY -> VaultItemCipherType.SSH_KEY
CreateVaultItemType.BANK_ACCOUNT -> VaultItemCipherType.BANK_ACCOUNT
CreateVaultItemType.DRIVERS_LICENSE -> VaultItemCipherType.DRIVERS_LICENSE
CreateVaultItemType.PASSPORT -> VaultItemCipherType.PASSPORT
CreateVaultItemType.FOLDER -> null
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ fun CoachMarkScope<AddEditItemCoachMark>.VaultAddEditContent(

is VaultAddEditState.ViewState.Content.ItemType.Identity -> Unit
is VaultAddEditState.ViewState.Content.ItemType.SshKey -> Unit
is VaultAddEditState.ViewState.Content.ItemType.BankAccount -> Unit
is VaultAddEditState.ViewState.Content.ItemType.DriversLicense -> Unit
is VaultAddEditState.ViewState.Content.ItemType.Passport -> Unit
is VaultAddEditState.ViewState.Content.ItemType.Login -> {
loginItemTypeHandlers.onSetupTotpClick(isGranted)
}
Expand Down Expand Up @@ -275,6 +278,10 @@ fun CoachMarkScope<AddEditItemCoachMark>.VaultAddEditContent(
sshKeyTypeHandlers = sshKeyItemTypeHandlers,
)
}

is VaultAddEditState.ViewState.Content.ItemType.BankAccount -> Unit
is VaultAddEditState.ViewState.Content.ItemType.DriversLicense -> Unit
is VaultAddEditState.ViewState.Content.ItemType.Passport -> Unit
}

vaultAddEditAdditionalOptions(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ import com.x8bit.bitwarden.ui.vault.model.VaultCardExpirationMonth
import com.x8bit.bitwarden.ui.vault.model.VaultCollection
import com.x8bit.bitwarden.ui.vault.model.VaultIdentityTitle
import com.x8bit.bitwarden.ui.vault.model.VaultItemCipherType
import com.x8bit.bitwarden.ui.vault.model.VaultBankAccountType
import com.x8bit.bitwarden.ui.vault.model.VaultLinkedFieldType
import com.x8bit.bitwarden.ui.vault.util.detectCardBrand
import dagger.hilt.android.lifecycle.HiltViewModel
Expand Down Expand Up @@ -439,6 +440,14 @@ class VaultAddEditViewModel @Inject constructor(

@Suppress("LongMethod")
private fun handleSaveClick() = onContent { content ->
if (!content.type.isSdkSupported) {
sendEvent(
VaultAddEditEvent.ShowSnackbar(
message = BitwardenString.an_error_has_occurred.asText(),
),
)
return@onContent
}
if (hasValidationErrors(content)) return@onContent

mutableStateFlow.update {
Expand Down Expand Up @@ -2552,6 +2561,9 @@ data class VaultAddEditState(
VaultItemCipherType.IDENTITY -> BitwardenString.new_identity.asText()
VaultItemCipherType.SECURE_NOTE -> BitwardenString.new_note.asText()
VaultItemCipherType.SSH_KEY -> BitwardenString.new_ssh_key.asText()
VaultItemCipherType.BANK_ACCOUNT -> BitwardenString.new_bank_account.asText()
VaultItemCipherType.DRIVERS_LICENSE -> BitwardenString.new_drivers_license.asText()
VaultItemCipherType.PASSPORT -> BitwardenString.new_passport.asText()
}

is VaultAddEditType.EditItem -> when (cipherType) {
Expand All @@ -2560,6 +2572,9 @@ data class VaultAddEditState(
VaultItemCipherType.IDENTITY -> BitwardenString.edit_identity.asText()
VaultItemCipherType.SECURE_NOTE -> BitwardenString.edit_note.asText()
VaultItemCipherType.SSH_KEY -> BitwardenString.edit_ssh_key.asText()
VaultItemCipherType.BANK_ACCOUNT -> BitwardenString.edit_bank_account.asText()
VaultItemCipherType.DRIVERS_LICENSE -> BitwardenString.edit_drivers_license.asText()
VaultItemCipherType.PASSPORT -> BitwardenString.edit_passport.asText()
}
}

Expand Down Expand Up @@ -2653,6 +2668,9 @@ data class VaultAddEditState(
IDENTITY(BitwardenString.type_identity),
SECURE_NOTES(BitwardenString.type_secure_note),
SSH_KEYS(BitwardenString.type_ssh_key),
BANK_ACCOUNT(BitwardenString.type_bank_account),
DRIVERS_LICENSE(BitwardenString.type_drivers_license),
PASSPORT(BitwardenString.type_passport),
}

/**
Expand Down Expand Up @@ -2761,6 +2779,11 @@ data class VaultAddEditState(
*/
abstract val vaultLinkedFieldTypes: ImmutableList<VaultLinkedFieldType>

/**
* Whether this item type has SDK support for save operations.
*/
open val isSdkSupported: Boolean get() = true

/**
* Represents the login item information.
*
Expand Down Expand Up @@ -2936,6 +2959,83 @@ data class VaultAddEditState(
override val vaultLinkedFieldTypes: ImmutableList<VaultLinkedFieldType>
get() = persistentListOf()
}

/**
* Represents the bank account item information.
*/
@Parcelize
data class BankAccount(
val bankName: String = "",
val nameOnAccount: String = "",
val accountType: VaultBankAccountType = VaultBankAccountType.SELECT,
val accountNumber: String = "",
val routingNumber: String = "",
val branchNumber: String = "",
val pin: String = "",
val swiftCode: String = "",
val iban: String = "",
val bankContactPhone: String = "",
) : ItemType() {
override val itemTypeOption: ItemTypeOption
get() = ItemTypeOption.BANK_ACCOUNT

override val isSdkSupported: Boolean get() = false

override val vaultLinkedFieldTypes: ImmutableList<VaultLinkedFieldType>
get() = persistentListOf()
}

/**
* Represents the driver's license item information.
*/
@Parcelize
data class DriversLicense(
val firstName: String = "",
val middleName: String = "",
val lastName: String = "",
val licenseNumber: String = "",
val issuingCountry: String = "",
val issuingState: String = "",
val expirationMonth: String = "",
val expirationYear: String = "",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this still should also be made for VaultItemState.ViewState.Content.ItemType., right?

val licenseClass: String = "",
) : ItemType() {
override val itemTypeOption: ItemTypeOption
get() = ItemTypeOption.DRIVERS_LICENSE

override val isSdkSupported: Boolean get() = false

override val vaultLinkedFieldTypes: ImmutableList<VaultLinkedFieldType>
get() = persistentListOf()
}

/**
* Represents the passport item information.
*/
@Parcelize
data class Passport(
val surname: String = "",
val givenName: String = "",
val dobMonth: String = "",
val dobYear: String = "",
val nationality: String = "",
val passportNumber: String = "",
val passportType: String = "",
val issuingCountry: String = "",
val issuingAuthority: String = "",
val issueMonth: String = "",
val issueYear: String = "",
val expirationMonth: String = "",
val expirationYear: String = "",
) : ItemType() {
override val itemTypeOption: ItemTypeOption
get() = ItemTypeOption.PASSPORT

override val isSdkSupported: Boolean get() = false

override val vaultLinkedFieldTypes: ImmutableList<VaultLinkedFieldType>
get() = persistentListOf()
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,7 @@ private val VaultAddEditState.ViewState.Content.ItemType.defaultLinkedFieldTypeO
is VaultAddEditState.ViewState.Content.ItemType.Login -> VaultLinkedFieldType.USERNAME
is VaultAddEditState.ViewState.Content.ItemType.SecureNotes -> null
is VaultAddEditState.ViewState.Content.ItemType.SshKey -> null
is VaultAddEditState.ViewState.Content.ItemType.BankAccount -> null
is VaultAddEditState.ViewState.Content.ItemType.DriversLicense -> null
is VaultAddEditState.ViewState.Content.ItemType.Passport -> null
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,10 @@ fun VaultItemCipherType.toItemType(): VaultAddEditState.ViewState.Content.ItemTy
VaultItemCipherType.IDENTITY -> VaultAddEditState.ViewState.Content.ItemType.Identity()
VaultItemCipherType.SECURE_NOTE -> VaultAddEditState.ViewState.Content.ItemType.SecureNotes
VaultItemCipherType.SSH_KEY -> VaultAddEditState.ViewState.Content.ItemType.SshKey()
VaultItemCipherType.BANK_ACCOUNT ->
VaultAddEditState.ViewState.Content.ItemType.BankAccount()
VaultItemCipherType.DRIVERS_LICENSE ->
VaultAddEditState.ViewState.Content.ItemType.DriversLicense()
VaultItemCipherType.PASSPORT ->
VaultAddEditState.ViewState.Content.ItemType.Passport()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ding on formatting

}
Original file line number Diff line number Diff line change
Expand Up @@ -1460,6 +1460,9 @@ data class VaultItemState(
VaultItemCipherType.IDENTITY -> BitwardenString.view_identity.asText()
VaultItemCipherType.SECURE_NOTE -> BitwardenString.view_note.asText()
VaultItemCipherType.SSH_KEY -> BitwardenString.view_ssh_key.asText()
VaultItemCipherType.BANK_ACCOUNT -> BitwardenString.view_bank_account.asText()
VaultItemCipherType.DRIVERS_LICENSE -> BitwardenString.view_drivers_license.asText()
VaultItemCipherType.PASSPORT -> BitwardenString.view_passport.asText()
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.collections.immutable.ImmutableList
import kotlinx.collections.immutable.persistentListOf
import kotlinx.collections.immutable.toImmutableList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.map
Expand Down Expand Up @@ -152,7 +153,7 @@ class VaultItemListingViewModel @Inject constructor(
private val relyingPartyParser: RelyingPartyParser,
private val toastManager: ToastManager,
snackbarRelayManager: SnackbarRelayManager<SnackbarRelay>,
featureFlagManager: FeatureFlagManager,
private val featureFlagManager: FeatureFlagManager,
) : BaseViewModel<VaultItemListingState, VaultItemListingEvent, VaultItemListingsAction>(
initialState = run {
val userState = requireNotNull(authRepository.userStateFlow.value)
Expand Down Expand Up @@ -706,6 +707,9 @@ class VaultItemListingViewModel @Inject constructor(
CreateVaultItemType.IDENTITY,
CreateVaultItemType.SECURE_NOTE,
CreateVaultItemType.SSH_KEY,
CreateVaultItemType.BANK_ACCOUNT,
CreateVaultItemType.DRIVERS_LICENSE,
CreateVaultItemType.PASSPORT,
-> {
vaultItemType
.toVaultItemCipherTypeOrNull()
Expand Down Expand Up @@ -842,8 +846,15 @@ class VaultItemListingViewModel @Inject constructor(
}

private fun createVaultItemTypeSelectionExcludedOptions(): ImmutableList<CreateVaultItemType> {
val isNewItemTypesEnabled = featureFlagManager
.getFeatureFlag(FlagKey.NewItemTypes)
val newItemTypeExclusions = listOfNotNull(
CreateVaultItemType.BANK_ACCOUNT.takeUnless { isNewItemTypesEnabled },
CreateVaultItemType.DRIVERS_LICENSE.takeUnless { isNewItemTypesEnabled },
CreateVaultItemType.PASSPORT.takeUnless { isNewItemTypesEnabled },
)
// If policy is enable for any organization, exclude the card option
return if (state.restrictItemTypesPolicyOrgIds.isNotEmpty()) {
val baseExclusions = if (state.restrictItemTypesPolicyOrgIds.isNotEmpty()) {
persistentListOf(
CreateVaultItemType.CARD,
CreateVaultItemType.FOLDER,
Expand All @@ -855,6 +866,7 @@ class VaultItemListingViewModel @Inject constructor(
CreateVaultItemType.FOLDER,
)
}
return (baseExclusions + newItemTypeExclusions).toPersistentList()
}

private fun handleAddVaultItemClick() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class VaultViewModel @Inject constructor(
private val browserAutofillDialogManager: BrowserAutofillDialogManager,
private val credentialExchangeRegistryManager: CredentialExchangeRegistryManager,
private val buildInfoManager: BuildInfoManager,
featureFlagManager: FeatureFlagManager,
private val featureFlagManager: FeatureFlagManager,
snackbarRelayManager: SnackbarRelayManager<SnackbarRelay>,
) : BaseViewModel<VaultState, VaultEvent, VaultAction>(
initialState = run {
Expand Down Expand Up @@ -433,12 +433,17 @@ class VaultViewModel @Inject constructor(
}

private fun handleSelectAddItemType() {
val isNewItemTypesEnabled = featureFlagManager
.getFeatureFlag(FlagKey.NewItemTypes)
// If policy is enable for any organization, exclude the card option
val excludedOptions = persistentListOfNotNull(
CreateVaultItemType.SSH_KEY,
CreateVaultItemType.CARD.takeUnless {
state.restrictItemTypesPolicyOrgIds.isEmpty()
},
CreateVaultItemType.BANK_ACCOUNT.takeUnless { isNewItemTypesEnabled },
CreateVaultItemType.DRIVERS_LICENSE.takeUnless { isNewItemTypesEnabled },
CreateVaultItemType.PASSPORT.takeUnless { isNewItemTypesEnabled },
)

mutableStateFlow.update {
Expand Down Expand Up @@ -499,6 +504,9 @@ class VaultViewModel @Inject constructor(
CreateVaultItemType.IDENTITY,
CreateVaultItemType.SECURE_NOTE,
CreateVaultItemType.SSH_KEY,
CreateVaultItemType.BANK_ACCOUNT,
CreateVaultItemType.DRIVERS_LICENSE,
CreateVaultItemType.PASSPORT,
-> {
vaultItemType
.toVaultItemCipherTypeOrNull()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ private fun VaultAddEditState.ViewState.Content.ItemType.toCipherType(): CipherT
is VaultAddEditState.ViewState.Content.ItemType.Login -> CipherType.LOGIN
is VaultAddEditState.ViewState.Content.ItemType.SecureNotes -> CipherType.SECURE_NOTE
is VaultAddEditState.ViewState.Content.ItemType.SshKey -> CipherType.SSH_KEY
is VaultAddEditState.ViewState.Content.ItemType.BankAccount,
is VaultAddEditState.ViewState.Content.ItemType.DriversLicense,
is VaultAddEditState.ViewState.Content.ItemType.Passport,
-> throw IllegalArgumentException("SDK mapping not yet available for $this")
}

private fun VaultAddEditState.ViewState.Content.ItemType.toSshKeyView(): SshKeyView? =
Expand Down
Loading
Loading