-
Notifications
You must be signed in to change notification settings - Fork 24
[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
Open
nikwithak
wants to merge
44
commits into
main
Choose a base branch
from
vault/pm-25821/cipher-admin-ops
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
f426fba
Add create and edit admin operations
nikwithak 3756f38
Add delete operations
nikwithak 99d1685
Add soft delete (PUT delete) operations
nikwithak 0cd1aee
Add update_collection call for ciphers
nikwithak 9f82e22
Add get_ciphers_for_org to CiphersClient
nikwithak f89f43d
Add admin endpoints for update and restore
nikwithak fe95edd
Add CipherError::Api(ApiError) variant
nikwithak bd13498
Clean up error handling
nikwithak c173c98
index on vault/pm-25821/cipher-admin-ops: bd134987 Clean up error hanโฆ
nikwithak 079cb89
Consolidate delete & restore operations
nikwithak f9eaafa
Cleanup and logic consolidation
nikwithak 4e48743
Update docs for CiphersClient::list_org_ciphers
nikwithak f3a06f6
Add separate delete_as_admin functions
nikwithak eb3291a
Add tests for new create methods
nikwithak 74714e6
Add tests for edit admin cipher endpoints
nikwithak 3330a78
Add tests for delete cipher endpoints
nikwithak 8307f07
Update repository when a cipher is soft-deleted
nikwithak 6485b2e
Update tests for delete
nikwithak 28880f9
Move restore operations to separate file
nikwithak 0356b2d
Fix test_restore_many tests
nikwithak 2246af5
Fix soft_delete_as_admin test
nikwithak 0a558c1
Housekeeping - remove comments & warnings
nikwithak 7dc66a5
Fix edit_as_admin tests
nikwithak 28aac12
Merge branch 'main' of https://github.com/bitwarden/sdk-internal intoโฆ
nikwithak a0ba6e3
Fix clippy warnings
nikwithak f90129b
Add soft-delete funciton to Cipher
nikwithak 8c633b9
Move cipher admin functions to separate client
nikwithak 3e38626
Move delete logic to isolated functions, outside of CiphersClient
nikwithak 7467c01
Move restore functions to isolated functions, remove wiremock use
nikwithak 6d59590
Move admin delete ops to new CipherAdminClient
nikwithak ef3fef3
Move restore operations to CipherAdminClient
nikwithak 715f75c
Move create admin operations to CreateAdminClient
nikwithak feb335f
Improve docs on delete.rs
nikwithak 1f6c8ab
Move admin edit operations to CipherAdminController
nikwithak a1c61ec
Fix cipher admin create tests
nikwithak f029f56
Fix edit cipher admin tests
nikwithak 8cbf8c8
Remove helper function for get_api_configurations in CiphersClient
nikwithak 8aa3692
Move list_org_ciphers operation to admin client
nikwithak 16e28b0
Housekeeping: Remove commented code, change ::into -> ::from
nikwithak ca6ae3f
Merge branch 'main' of https://github.com/bitwarden/sdk-internal intoโฆ
nikwithak 1cf82de
Remove test code added to real function by mistake
nikwithak a3c29f0
Add admin() function to get CipherAdminClient
nikwithak 2077e04
Fix date string format for API requests
nikwithak 58e01df
Change TryFrom implementations to PartialCipher on partial server resโฆ
nikwithak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. | ||||||||
|
|
@@ -636,6 +637,11 @@ impl Cipher { | |||||||
| } | ||||||||
| Ok(()) | ||||||||
| } | ||||||||
|
|
||||||||
| pub(crate) fn soft_delete(&mut self) { | ||||||||
|
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. suggestion: Document?
Suggested change
|
||||||||
| self.deleted_date = Some(Utc::now()); | ||||||||
| self.archived_date = None; | ||||||||
| } | ||||||||
| } | ||||||||
| impl CipherView { | ||||||||
| #[allow(missing_docs)] | ||||||||
|
|
@@ -970,6 +976,15 @@ impl TryFrom<CipherDetailsResponseModel> for Cipher { | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| impl PartialCipher for CipherDetailsResponseModel { | ||||||||
| fn merge_with_cipher(self, cipher: Option<Cipher>) -> Result<Cipher, VaultParseError> { | ||||||||
| Ok(Cipher { | ||||||||
| local_data: cipher.and_then(|c| c.local_data), | ||||||||
| ..self.try_into()? | ||||||||
| }) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| impl From<bitwarden_api_api::models::CipherType> for CipherType { | ||||||||
| fn from(t: bitwarden_api_api::models::CipherType) -> Self { | ||||||||
| match t { | ||||||||
|
|
@@ -991,6 +1006,13 @@ impl From<bitwarden_api_api::models::CipherRepromptType> for CipherRepromptType | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| /// A trait for merging partial cipher data into a full cipher. | ||||||||
| /// Used to convert from API response models to full Cipher structs, | ||||||||
| /// without losing local data that may not be present in the API response. | ||||||||
| pub(crate) trait PartialCipher { | ||||||||
| fn merge_with_cipher(self, cipher: Option<Cipher>) -> Result<Cipher, VaultParseError>; | ||||||||
|
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. question: Why accept an optional cipher? The function might be nicer if cipher is required and you just do |
||||||||
| } | ||||||||
|
|
||||||||
| impl From<CipherType> for bitwarden_api_api::models::CipherType { | ||||||||
| fn from(t: CipherType) -> Self { | ||||||||
| match t { | ||||||||
|
|
@@ -1061,6 +1083,131 @@ impl TryFrom<CipherResponseModel> for Cipher { | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| impl PartialCipher for CipherMiniResponseModel { | ||||||||
| fn merge_with_cipher(self, cipher: Option<Cipher>) -> Result<Cipher, VaultParseError> { | ||||||||
| let cipher = cipher.as_ref(); | ||||||||
| Ok(Cipher { | ||||||||
| id: self.id.map(CipherId::new), | ||||||||
| organization_id: self.organization_id.map(OrganizationId::new), | ||||||||
| key: EncString::try_from_optional(self.key)?, | ||||||||
| name: require!(EncString::try_from_optional(self.name)?), | ||||||||
| notes: EncString::try_from_optional(self.notes)?, | ||||||||
| r#type: require!(self.r#type).into(), | ||||||||
| login: self.login.map(|l| (*l).try_into()).transpose()?, | ||||||||
| identity: self.identity.map(|i| (*i).try_into()).transpose()?, | ||||||||
| card: self.card.map(|c| (*c).try_into()).transpose()?, | ||||||||
| secure_note: self.secure_note.map(|s| (*s).try_into()).transpose()?, | ||||||||
| ssh_key: self.ssh_key.map(|s| (*s).try_into()).transpose()?, | ||||||||
| reprompt: self | ||||||||
| .reprompt | ||||||||
| .map(|r| r.into()) | ||||||||
| .unwrap_or(CipherRepromptType::None), | ||||||||
| organization_use_totp: self.organization_use_totp.unwrap_or(true), | ||||||||
| attachments: self | ||||||||
| .attachments | ||||||||
| .map(|a| a.into_iter().map(|a| a.try_into()).collect()) | ||||||||
| .transpose()?, | ||||||||
| fields: self | ||||||||
| .fields | ||||||||
| .map(|f| f.into_iter().map(|f| f.try_into()).collect()) | ||||||||
| .transpose()?, | ||||||||
| password_history: self | ||||||||
| .password_history | ||||||||
| .map(|p| p.into_iter().map(|p| p.try_into()).collect()) | ||||||||
| .transpose()?, | ||||||||
| creation_date: require!(self.creation_date) | ||||||||
| .parse() | ||||||||
| .map_err(Into::<VaultParseError>::into)?, | ||||||||
| deleted_date: self | ||||||||
| .deleted_date | ||||||||
| .map(|d| d.parse()) | ||||||||
| .transpose() | ||||||||
| .map_err(Into::<VaultParseError>::into)?, | ||||||||
| revision_date: require!(self.revision_date) | ||||||||
| .parse() | ||||||||
| .map_err(Into::<VaultParseError>::into)?, | ||||||||
| archived_date: self | ||||||||
| .archived_date | ||||||||
| .map(|d| d.parse()) | ||||||||
| .transpose() | ||||||||
| .map_err(Into::<VaultParseError>::into)?, | ||||||||
| folder_id: cipher.map_or(Default::default(), |c| c.folder_id), | ||||||||
| favorite: cipher.map_or(Default::default(), |c| c.favorite), | ||||||||
| edit: cipher.map_or(Default::default(), |c| c.edit), | ||||||||
| permissions: cipher.map_or(Default::default(), |c| c.permissions), | ||||||||
| view_password: cipher.map_or(Default::default(), |c| c.view_password), | ||||||||
| local_data: cipher.map_or(Default::default(), |c| c.local_data.clone()), | ||||||||
| data: cipher.map_or(Default::default(), |c| c.data.clone()), | ||||||||
| collection_ids: cipher.map_or(Default::default(), |c| c.collection_ids.clone()), | ||||||||
| }) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| impl PartialCipher for CipherMiniDetailsResponseModel { | ||||||||
| fn merge_with_cipher(self, cipher: Option<Cipher>) -> Result<Cipher, VaultParseError> { | ||||||||
| let cipher = cipher.as_ref(); | ||||||||
| Ok(Cipher { | ||||||||
| id: self.id.map(CipherId::new), | ||||||||
| organization_id: self.organization_id.map(OrganizationId::new), | ||||||||
| key: EncString::try_from_optional(self.key)?, | ||||||||
| name: require!(EncString::try_from_optional(self.name)?), | ||||||||
| notes: EncString::try_from_optional(self.notes)?, | ||||||||
| r#type: require!(self.r#type).into(), | ||||||||
| login: self.login.map(|l| (*l).try_into()).transpose()?, | ||||||||
| identity: self.identity.map(|i| (*i).try_into()).transpose()?, | ||||||||
| card: self.card.map(|c| (*c).try_into()).transpose()?, | ||||||||
| secure_note: self.secure_note.map(|s| (*s).try_into()).transpose()?, | ||||||||
| ssh_key: self.ssh_key.map(|s| (*s).try_into()).transpose()?, | ||||||||
| reprompt: self | ||||||||
| .reprompt | ||||||||
| .map(|r| r.into()) | ||||||||
| .unwrap_or(CipherRepromptType::None), | ||||||||
| organization_use_totp: self.organization_use_totp.unwrap_or(true), | ||||||||
| attachments: self | ||||||||
| .attachments | ||||||||
| .map(|a| a.into_iter().map(|a| a.try_into()).collect()) | ||||||||
| .transpose()?, | ||||||||
| fields: self | ||||||||
| .fields | ||||||||
| .map(|f| f.into_iter().map(|f| f.try_into()).collect()) | ||||||||
| .transpose()?, | ||||||||
| password_history: self | ||||||||
| .password_history | ||||||||
| .map(|p| p.into_iter().map(|p| p.try_into()).collect()) | ||||||||
| .transpose()?, | ||||||||
| creation_date: require!(self.creation_date) | ||||||||
| .parse() | ||||||||
| .map_err(Into::<VaultParseError>::into)?, | ||||||||
| deleted_date: self | ||||||||
| .deleted_date | ||||||||
| .map(|d| d.parse()) | ||||||||
| .transpose() | ||||||||
| .map_err(Into::<VaultParseError>::into)?, | ||||||||
| revision_date: require!(self.revision_date) | ||||||||
| .parse() | ||||||||
| .map_err(Into::<VaultParseError>::into)?, | ||||||||
| archived_date: self | ||||||||
| .archived_date | ||||||||
| .map(|d| d.parse()) | ||||||||
| .transpose() | ||||||||
| .map_err(Into::<VaultParseError>::into)?, | ||||||||
| collection_ids: self | ||||||||
| .collection_ids | ||||||||
| .into_iter() | ||||||||
| .flatten() | ||||||||
| .map(CollectionId::new) | ||||||||
| .collect(), | ||||||||
| folder_id: cipher.map_or(Default::default(), |c| c.folder_id), | ||||||||
| favorite: cipher.map_or(Default::default(), |c| c.favorite), | ||||||||
| edit: cipher.map_or(Default::default(), |c| c.edit), | ||||||||
| permissions: cipher.map_or(Default::default(), |c| c.permissions), | ||||||||
| view_password: cipher.map_or(Default::default(), |c| c.view_password), | ||||||||
| data: cipher.map_or(Default::default(), |c| c.data.clone()), | ||||||||
| local_data: cipher.map_or(Default::default(), |c| c.local_data.clone()), | ||||||||
| }) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| #[cfg(test)] | ||||||||
| mod tests { | ||||||||
|
|
||||||||
|
|
||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.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.
Yea sounds good.