-
Notifications
You must be signed in to change notification settings - Fork 25
[PM-25821] Migrate Cipher Admin operation API calls to SDK #560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 25 commits
f426fba
3756f38
99d1685
0cd1aee
9f82e22
f89f43d
fe95edd
bd13498
c173c98
079cb89
f9eaafa
4e48743
f3a06f6
eb3291a
74714e6
3330a78
8307f07
6485b2e
28880f9
0356b2d
2246af5
0a558c1
7dc66a5
28aac12
a0ba6e3
f90129b
8c633b9
3e38626
7467c01
6d59590
ef3fef3
715f75c
feb335f
1f6c8ab
a1c61ec
f029f56
8cbf8c8
8aa3692
16e28b0
ca6ae3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,10 @@ | ||
| use bitwarden_api_api::{ | ||
| apis::ciphers_api::{PutShareError, PutShareManyError}, | ||
| models::{ | ||
| CipherDetailsResponseModel, CipherRequestModel, CipherResponseModel, | ||
| CipherWithIdRequestModel, | ||
| }, | ||
| use bitwarden_api_api::models::{ | ||
| CipherDetailsResponseModel, CipherMiniDetailsResponseModel, CipherMiniResponseModel, | ||
| CipherRequestModel, CipherResponseModel, CipherWithIdRequestModel, | ||
| }; | ||
| use bitwarden_collections::collection::CollectionId; | ||
| use bitwarden_core::{ | ||
| MissingFieldError, OrganizationId, UserId, | ||
| ApiError, MissingFieldError, OrganizationId, UserId, | ||
| key_management::{KeyIds, MINIMUM_ENFORCE_ICON_URI_HASH_VERSION, SymmetricKeyId}, | ||
| require, | ||
| }; | ||
|
|
@@ -63,15 +60,19 @@ pub enum CipherError { | |
| #[error("This cipher cannot be moved to the specified organization")] | ||
| OrganizationAlreadySet, | ||
| #[error(transparent)] | ||
| PutShare(#[from] bitwarden_api_api::apis::Error<PutShareError>), | ||
| #[error(transparent)] | ||
| PutShareMany(#[from] bitwarden_api_api::apis::Error<PutShareManyError>), | ||
| #[error(transparent)] | ||
| Repository(#[from] RepositoryError), | ||
| #[error(transparent)] | ||
| Chrono(#[from] chrono::ParseError), | ||
| #[error(transparent)] | ||
| SerdeJson(#[from] serde_json::Error), | ||
| #[error(transparent)] | ||
| Api(#[from] ApiError), | ||
| } | ||
|
|
||
| impl<T> From<bitwarden_api_api::apis::Error<T>> for CipherError { | ||
| fn from(value: bitwarden_api_api::apis::Error<T>) -> Self { | ||
| Self::Api(value.into()) | ||
| } | ||
| } | ||
|
|
||
| /// Helper trait for operations on cipher types. | ||
|
|
@@ -1061,6 +1062,138 @@ impl TryFrom<CipherResponseModel> for Cipher { | |
| } | ||
| } | ||
|
|
||
| impl TryFrom<CipherMiniResponseModel> for Cipher { | ||
| type Error = VaultParseError; | ||
| fn try_from(cipher_mini: CipherMiniResponseModel) -> Result<Self, Self::Error> { | ||
| Ok(Cipher { | ||
| id: cipher_mini.id.map(CipherId::new), | ||
| organization_id: cipher_mini.organization_id.map(OrganizationId::new), | ||
| key: EncString::try_from_optional(cipher_mini.key)?, | ||
| name: require!(EncString::try_from_optional(cipher_mini.name)?), | ||
| notes: EncString::try_from_optional(cipher_mini.notes)?, | ||
| r#type: require!(cipher_mini.r#type).into(), | ||
| login: cipher_mini.login.map(|l| (*l).try_into()).transpose()?, | ||
| identity: cipher_mini.identity.map(|i| (*i).try_into()).transpose()?, | ||
| card: cipher_mini.card.map(|c| (*c).try_into()).transpose()?, | ||
| secure_note: cipher_mini | ||
| .secure_note | ||
| .map(|s| (*s).try_into()) | ||
| .transpose()?, | ||
| ssh_key: cipher_mini.ssh_key.map(|s| (*s).try_into()).transpose()?, | ||
| reprompt: cipher_mini | ||
| .reprompt | ||
| .map(|r| r.into()) | ||
| .unwrap_or(CipherRepromptType::None), | ||
| organization_use_totp: cipher_mini.organization_use_totp.unwrap_or(true), | ||
| attachments: cipher_mini | ||
| .attachments | ||
| .map(|a| a.into_iter().map(|a| a.try_into()).collect()) | ||
| .transpose()?, | ||
| fields: cipher_mini | ||
| .fields | ||
| .map(|f| f.into_iter().map(|f| f.try_into()).collect()) | ||
| .transpose()?, | ||
| password_history: cipher_mini | ||
| .password_history | ||
| .map(|p| p.into_iter().map(|p| p.try_into()).collect()) | ||
| .transpose()?, | ||
| creation_date: require!(cipher_mini.creation_date) | ||
| .parse() | ||
| .map_err(Into::<VaultParseError>::into)?, | ||
| deleted_date: cipher_mini | ||
| .deleted_date | ||
| .map(|d| d.parse()) | ||
| .transpose() | ||
| .map_err(Into::<VaultParseError>::into)?, | ||
| revision_date: require!(cipher_mini.revision_date) | ||
| .parse() | ||
| .map_err(Into::<VaultParseError>::into)?, | ||
| archived_date: cipher_mini | ||
| .archived_date | ||
| .map(|d| d.parse()) | ||
| .transpose() | ||
| .map_err(Into::<VaultParseError>::into)?, | ||
| edit: Default::default(), | ||
| favorite: Default::default(), | ||
| folder_id: Default::default(), | ||
| permissions: Default::default(), | ||
| view_password: Default::default(), | ||
| local_data: Default::default(), | ||
| collection_ids: Default::default(), | ||
|
Comment on lines
+1121
to
+1127
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reflection: Putting default values here can be dangerous, because there is nothing to indicate the Cipher model is incomplete.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately several API operations don't return the full cipher, only the
Do you have a preference? Feel free to ping me on Slack when you're free for a deeper discussion - there are a handful of API operations that currently only return a subset of data like this. |
||
| data: None, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<CipherMiniDetailsResponseModel> for Cipher { | ||
| type Error = VaultParseError; | ||
|
|
||
| fn try_from(cipher_mini: CipherMiniDetailsResponseModel) -> Result<Self, Self::Error> { | ||
| Ok(Cipher { | ||
| id: cipher_mini.id.map(CipherId::new), | ||
| organization_id: cipher_mini.organization_id.map(OrganizationId::new), | ||
| key: EncString::try_from_optional(cipher_mini.key)?, | ||
| name: require!(EncString::try_from_optional(cipher_mini.name)?), | ||
| notes: EncString::try_from_optional(cipher_mini.notes)?, | ||
| r#type: require!(cipher_mini.r#type).into(), | ||
| login: cipher_mini.login.map(|l| (*l).try_into()).transpose()?, | ||
| identity: cipher_mini.identity.map(|i| (*i).try_into()).transpose()?, | ||
| card: cipher_mini.card.map(|c| (*c).try_into()).transpose()?, | ||
| secure_note: cipher_mini | ||
| .secure_note | ||
| .map(|s| (*s).try_into()) | ||
| .transpose()?, | ||
| ssh_key: cipher_mini.ssh_key.map(|s| (*s).try_into()).transpose()?, | ||
| reprompt: cipher_mini | ||
| .reprompt | ||
| .map(|r| r.into()) | ||
| .unwrap_or(CipherRepromptType::None), | ||
| organization_use_totp: cipher_mini.organization_use_totp.unwrap_or(true), | ||
| attachments: cipher_mini | ||
| .attachments | ||
| .map(|a| a.into_iter().map(|a| a.try_into()).collect()) | ||
| .transpose()?, | ||
| fields: cipher_mini | ||
| .fields | ||
| .map(|f| f.into_iter().map(|f| f.try_into()).collect()) | ||
| .transpose()?, | ||
| password_history: cipher_mini | ||
| .password_history | ||
| .map(|p| p.into_iter().map(|p| p.try_into()).collect()) | ||
| .transpose()?, | ||
| creation_date: require!(cipher_mini.creation_date) | ||
| .parse() | ||
| .map_err(Into::<VaultParseError>::into)?, | ||
| deleted_date: cipher_mini | ||
| .deleted_date | ||
| .map(|d| d.parse()) | ||
| .transpose() | ||
| .map_err(Into::<VaultParseError>::into)?, | ||
| revision_date: require!(cipher_mini.revision_date) | ||
| .parse() | ||
| .map_err(Into::<VaultParseError>::into)?, | ||
| archived_date: cipher_mini | ||
| .archived_date | ||
| .map(|d| d.parse()) | ||
| .transpose() | ||
| .map_err(Into::<VaultParseError>::into)?, | ||
| collection_ids: cipher_mini | ||
| .collection_ids | ||
| .into_iter() | ||
| .flatten() | ||
| .map(CollectionId::new) | ||
| .collect(), | ||
| data: None, | ||
| folder_id: Default::default(), | ||
| favorite: Default::default(), | ||
| edit: Default::default(), | ||
| permissions: Default::default(), | ||
| view_password: Default::default(), | ||
| local_data: Default::default(), | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods putting
Apierrors inCipherErrorshould really be updated to not use cipher error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only one still using CipherError is the
shareoperations, which calls existing functions that currently returnsCipherErroralready (e.g. https://github.com/bitwarden/sdk-internal/blob/vault/pm-25821/cipher-admin-ops/crates/bitwarden-vault/src/cipher/cipher_client/share_cipher.rs#L180-L184) - I think we can migrate this one to its own error type in the future.