From 82dbf96aa8f4e886fef21656c475655c8fb45460 Mon Sep 17 00:00:00 2001 From: Mick Letofsky Date: Thu, 13 Nov 2025 10:59:28 +0100 Subject: [PATCH 1/3] Implement the ability to delete an attachment --- crates/bitwarden-vault/src/cipher/cipher.rs | 8 +- .../cipher/cipher_client/delete_attachment.rs | 453 ++++++++++++++++++ .../src/cipher/cipher_client/mod.rs | 1 + 3 files changed, 461 insertions(+), 1 deletion(-) create mode 100644 crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs diff --git a/crates/bitwarden-vault/src/cipher/cipher.rs b/crates/bitwarden-vault/src/cipher/cipher.rs index 9b6854f76..32c3234fe 100644 --- a/crates/bitwarden-vault/src/cipher/cipher.rs +++ b/crates/bitwarden-vault/src/cipher/cipher.rs @@ -1,5 +1,7 @@ use bitwarden_api_api::{ - apis::ciphers_api::{PutShareError, PutShareManyError}, + apis::ciphers_api::{ + DeleteAttachmentAdminError, DeleteAttachmentError, PutShareError, PutShareManyError, + }, models::{ CipherDetailsResponseModel, CipherRequestModel, CipherResponseModel, CipherWithIdRequestModel, @@ -72,6 +74,10 @@ pub enum CipherError { Chrono(#[from] chrono::ParseError), #[error(transparent)] SerdeJson(#[from] serde_json::Error), + #[error(transparent)] + DeleteAttachment(#[from] bitwarden_api_api::apis::Error), + #[error(transparent)] + DeleteAttachmentAdmin(#[from] bitwarden_api_api::apis::Error), } /// Helper trait for operations on cipher types. diff --git a/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs b/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs new file mode 100644 index 000000000..76f93110b --- /dev/null +++ b/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs @@ -0,0 +1,453 @@ +use bitwarden_api_api::{apis::ciphers_api::CiphersApi, models}; +use bitwarden_core::require; +use bitwarden_state::repository::Repository; +#[cfg(feature = "wasm")] +use wasm_bindgen::prelude::wasm_bindgen; + +use crate::{Cipher, CipherError, CipherId, CiphersClient, VaultParseError}; + +/// Standalone function to delete an attachment from a cipher that is extracted for ease of unit +/// testing. +async fn delete_attachment( + api_client: &dyn CiphersApi, + repository: &dyn Repository, + cipher_id: CipherId, + attachment_id: &str, + admin: bool, +) -> Result { + let response = if admin { + api_client + .delete_attachment_admin(cipher_id.into(), attachment_id) + .await? + } else { + api_client + .delete_attachment(cipher_id.into(), attachment_id) + .await? + }; + + let boxed_cipher: Box = require!(response.cipher); + let cipher_response = *boxed_cipher; + let mut cipher = require!(repository.get(cipher_id.to_string()).await?); + + cipher.revision_date = require!(cipher_response.revision_date) + .parse() + .map_err(Into::::into)?; + + if let Some(ref mut attachments) = cipher.attachments { + attachments.retain(|a| a.id.as_deref() != Some(attachment_id)); + } + + repository + .set(cipher_id.to_string(), cipher.clone()) + .await?; + + Ok(cipher) +} + +#[cfg_attr(feature = "wasm", wasm_bindgen)] +impl CiphersClient { + /// Delete an attachment from a cipher + pub async fn delete_attachment( + &self, + cipher_id: CipherId, + attachment_id: String, + ) -> Result { + let config = self.client.internal.get_api_configurations().await; + + delete_attachment( + config.api_client.ciphers_api(), + &*self.get_repository()?, + cipher_id, + &attachment_id, + false, + ) + .await + } + + /// Delete an attachment from a cipher as an administrator + pub async fn delete_attachment_as_admin( + &self, + cipher_id: CipherId, + attachment_id: String, + ) -> Result { + let config = self.client.internal.get_api_configurations().await; + + delete_attachment( + config.api_client.ciphers_api(), + &*self.get_repository()?, + cipher_id, + &attachment_id, + true, + ) + .await + } +} + +#[cfg(test)] +mod tests { + use bitwarden_api_api::{apis::ApiClient, models}; + use bitwarden_test::MemoryRepository; + + use super::*; + use crate::{Attachment, CipherRepromptType, CipherType}; + + const TEST_CIPHER_ID: &str = "5faa9684-c793-4a2d-8a12-b33900187097"; + const TEST_ATTACHMENT_ID_TO_DELETE: &str = "attachment-to-delete"; + const TEST_ATTACHMENT_ID_TO_KEEP: &str = "attachment-to-keep"; + + fn test_attachment_to_delete() -> Attachment { + Attachment { + id: Some(TEST_ATTACHMENT_ID_TO_DELETE.to_string()), + url: Some("http://localhost:4000/attachments/path1".to_string()), + file_name: Some( + "2.mV50WiLq6duhwGbhM1TO0A==|dTufWNH8YTPP0EMlNLIpFA==|QHp+7OM8xHtEmCfc9QPXJ0Ro2BeakzvLgxJZ7NdLuDc=" + .parse() + .unwrap(), + ), + key: None, + size: Some("65".to_string()), + size_name: Some("65 Bytes".to_string()), + } + } + + fn test_attachment_to_keep() -> Attachment { + Attachment { + id: Some(TEST_ATTACHMENT_ID_TO_KEEP.to_string()), + url: Some("http://localhost:4000/attachments/path2".to_string()), + file_name: Some( + "2.GhazFdCYQcM5v+AtVwceQA==|98bMUToqC61VdVsSuXWRwA==|bsLByMht9Hy5QO9pPMRz0K4d0aqBiYnnROGM5YGbNu4=" + .parse() + .unwrap(), + ), + key: Some( + "2.6TPEiYULFg/4+3CpDRwCqw==|6swweBHCJcd5CHdwBBWuRN33XRV22VoroDFDUmiM4OzjPEAhgZK57IZS1KkBlCcFvT+t+YbsmDcdv+Lqr+iJ3MmzfJ40MCB5TfYy+22HVRA=|UvmtuC96O+96TvemAC7SFj1xJkXwK3Su5AnGmXcwXH0=" + .parse() + .unwrap(), + ), + size: Some("100".to_string()), + size_name: Some("100 Bytes".to_string()), + } + } + + fn create_cipher_fixture(cipher_id: CipherId) -> Cipher { + Cipher { + id: Some(cipher_id), + r#type: CipherType::Login, + login: Some(crate::cipher::Login { + username: Some( + "2.EI9Km5BfrIqBa1W+WCccfA==|laWxNnx+9H3MZww4zm7cBSLisjpi81zreaQntRhegVI=|x42+qKFf5ga6DIL0OW5pxCdLrC/gm8CXJvf3UASGteI=" + .parse() + .unwrap(), + ), + password: Some( + "2.EI9Km5BfrIqBa1W+WCccfA==|laWxNnx+9H3MZww4zm7cBSLisjpi81zreaQntRhegVI=|x42+qKFf5ga6DIL0OW5pxCdLrC/gm8CXJvf3UASGteI=" + .parse() + .unwrap(), + ), + password_revision_date: None, + uris: None, + totp: None, + autofill_on_page_load: None, + fido2_credentials: None, + }), + name: "2.EI9Km5BfrIqBa1W+WCccfA==|laWxNnx+9H3MZww4zm7cBSLisjpi81zreaQntRhegVI=|x42+qKFf5ga6DIL0OW5pxCdLrC/gm8CXJvf3UASGteI=" + .parse() + .unwrap(), + notes: Some("2.rSw0uVQEFgUCEmOQx0JnDg==|MKqHLD25aqaXYHeYJPH/mor7l3EeSQKsI7A/R+0bFTI=|ODcUScISzKaZWHlUe4MRGuTT2S7jpyDmbOHl7d+6HiM=".parse().unwrap()), + favorite: false, + reprompt: CipherRepromptType::None, + organization_use_totp: true, + edit: true, + view_password: true, + local_data: None, + attachments: Some(vec![test_attachment_to_delete(), test_attachment_to_keep()]), + fields: None, + password_history: None, + creation_date: "2024-01-20T17:00:00.000Z".parse().unwrap(), + deleted_date: None, + revision_date: "2024-01-20T17:55:36.150Z".parse().unwrap(), + archived_date: None, + folder_id: None, + organization_id: None, + collection_ids: vec![], + key: None, + identity: None, + card: None, + secure_note: None, + ssh_key: None, + permissions: None, + data: None, + } + } + + fn mock_delete_attachment_success() -> ApiClient { + ApiClient::new_mocked(move |mock| { + mock.ciphers_api + .expect_delete_attachment() + .returning(move |_id, _attachment_id| { + Ok(models::DeleteAttachmentResponseData { + cipher: Some(Box::new(models::Cipher { + revision_date: Some("2024-01-30T18:00:00.000Z".to_string()), + ..Default::default() + })), + }) + }); + }) + } + + fn mock_delete_attachment_admin_success() -> ApiClient { + ApiClient::new_mocked(move |mock| { + mock.ciphers_api.expect_delete_attachment_admin().returning( + move |_id, _attachment_id| { + Ok(models::DeleteAttachmentResponseData { + cipher: Some(Box::new(models::Cipher { + revision_date: Some("2024-01-30T18:00:00.000Z".to_string()), + ..Default::default() + })), + }) + }, + ); + }) + } + + #[tokio::test] + async fn test_delete_attachment_returns_updated_cipher() { + let cipher_id: CipherId = TEST_CIPHER_ID.parse().unwrap(); + let api_client = mock_delete_attachment_success(); + let repository = MemoryRepository::::default(); + let cipher = create_cipher_fixture(cipher_id); + repository + .set(TEST_CIPHER_ID.to_string(), cipher.clone()) + .await + .unwrap(); + + let result = delete_attachment( + api_client.ciphers_api(), + &repository, + cipher_id, + TEST_ATTACHMENT_ID_TO_DELETE, + false, + ) + .await; + + assert!(result.is_ok()); + let updated_cipher = result.unwrap(); + let attachments = updated_cipher + .attachments + .as_ref() + .expect("Attachments should exist"); + let stored_cipher = repository + .get(TEST_CIPHER_ID.to_string()) + .await + .unwrap() + .expect("Cipher exists"); + let stored_attachments = stored_cipher + .attachments + .as_ref() + .expect("Attachments exist"); + + assert_eq!(attachments.len(), 1, "Should have 1 attachment remaining"); + assert_eq!( + attachments[0].id.as_deref(), + Some(TEST_ATTACHMENT_ID_TO_KEEP), + "The remaining attachment should be the one we didn't delete" + ); + + assert_eq!( + updated_cipher.revision_date.to_rfc3339(), + "2024-01-30T18:00:00+00:00", + "Revision date should be updated from API response" + ); + + assert_eq!(stored_attachments.len(), 1); + + assert_eq!( + stored_attachments[0].id.as_deref(), + Some(TEST_ATTACHMENT_ID_TO_KEEP), + "Stored cipher should have the correct remaining attachment" + ); + + assert_eq!(stored_cipher.revision_date, updated_cipher.revision_date); + + assert_eq!(stored_cipher.name, cipher.name, "Name should not change"); + } + + #[tokio::test] + async fn test_delete_attachment_uses_non_admin_api() { + let cipher_id: CipherId = TEST_CIPHER_ID.parse().unwrap(); + let api_client = mock_delete_attachment_success(); + let repository = MemoryRepository::::default(); + let cipher = create_cipher_fixture(cipher_id); + repository + .set(TEST_CIPHER_ID.to_string(), cipher.clone()) + .await + .unwrap(); + + let result = delete_attachment( + api_client.ciphers_api(), + &repository, + cipher_id, + TEST_ATTACHMENT_ID_TO_DELETE, + false, // admin = false + ) + .await; + + assert!(result.is_ok()); + let updated_cipher = result.unwrap(); + let attachments = updated_cipher + .attachments + .as_ref() + .expect("Attachments should exist"); + assert_eq!(attachments.len(), 1, "Should have 1 attachment remaining"); + assert_eq!( + attachments[0].id.as_deref(), + Some(TEST_ATTACHMENT_ID_TO_KEEP) + ); + } + + #[tokio::test] + async fn test_delete_attachment_as_admin_client_method_fails_without_mock_server() { + let cipher_id: CipherId = TEST_CIPHER_ID.parse().unwrap(); + let api_client = mock_delete_attachment_admin_success(); + let repository = MemoryRepository::::default(); + let cipher = create_cipher_fixture(cipher_id); + repository + .set(TEST_CIPHER_ID.to_string(), cipher.clone()) + .await + .unwrap(); + + let result = delete_attachment( + api_client.ciphers_api(), + &repository, + cipher_id, + TEST_ATTACHMENT_ID_TO_DELETE, + true, // admin = true + ) + .await; + + assert!(result.is_ok()); + let updated_cipher = result.unwrap(); + let attachments = updated_cipher + .attachments + .as_ref() + .expect("Attachments should exist"); + assert_eq!(attachments.len(), 1, "Should have 1 attachment remaining"); + assert_eq!( + attachments[0].id.as_deref(), + Some(TEST_ATTACHMENT_ID_TO_KEEP) + ); + } + + #[tokio::test] + async fn test_delete_attachment_missing_cipher_in_repository() { + let cipher_id: CipherId = TEST_CIPHER_ID.parse().unwrap(); + let api_client = mock_delete_attachment_success(); + let repository = MemoryRepository::::default(); + + let result = delete_attachment( + api_client.ciphers_api(), + &repository, + cipher_id, + TEST_ATTACHMENT_ID_TO_DELETE, + false, + ) + .await; + + assert!(result.is_err()); + assert!(matches!(result.unwrap_err(), CipherError::MissingField(_))); + } + + #[tokio::test] + async fn test_delete_attachment_api_error() { + let cipher_id: CipherId = TEST_CIPHER_ID.parse().unwrap(); + let api_client = ApiClient::new_mocked(move |mock| { + mock.ciphers_api + .expect_delete_attachment() + .returning(move |_id, _attachment_id| { + Err(bitwarden_api_api::apis::Error::Io(std::io::Error::new( + std::io::ErrorKind::NotFound, + "Attachment not found", + ))) + }); + }); + let repository = MemoryRepository::::default(); + let cipher = create_cipher_fixture(cipher_id); + repository + .set(TEST_CIPHER_ID.to_string(), cipher.clone()) + .await + .unwrap(); + + let result = delete_attachment( + api_client.ciphers_api(), + &repository, + cipher_id, + TEST_ATTACHMENT_ID_TO_DELETE, + false, + ) + .await; + + assert!(result.is_err()); + } + + #[tokio::test] + async fn test_delete_attachment_admin_api_error() { + let cipher_id: CipherId = TEST_CIPHER_ID.parse().unwrap(); + let api_client = ApiClient::new_mocked(move |mock| { + mock.ciphers_api.expect_delete_attachment_admin().returning( + move |_id, _attachment_id| { + Err(bitwarden_api_api::apis::Error::Io(std::io::Error::new( + std::io::ErrorKind::NotFound, + "Attachment not found as admin", + ))) + }, + ); + }); + let repository = MemoryRepository::::default(); + let cipher = create_cipher_fixture(cipher_id); + repository + .set(TEST_CIPHER_ID.to_string(), cipher.clone()) + .await + .unwrap(); + + let result = delete_attachment( + api_client.ciphers_api(), + &repository, + cipher_id, + TEST_ATTACHMENT_ID_TO_DELETE, + true, + ) + .await; + + assert!(result.is_err()); + } + + #[tokio::test] + async fn test_delete_attachment_no_attachments_on_cipher() { + let cipher_id: CipherId = TEST_CIPHER_ID.parse().unwrap(); + let api_client = mock_delete_attachment_success(); + let repository = MemoryRepository::::default(); + let mut cipher = create_cipher_fixture(cipher_id); + cipher.attachments = None; + repository + .set(TEST_CIPHER_ID.to_string(), cipher.clone()) + .await + .unwrap(); + + let result = delete_attachment( + api_client.ciphers_api(), + &repository, + cipher_id, + TEST_ATTACHMENT_ID_TO_DELETE, + false, + ) + .await; + + assert!(result.is_ok()); + let updated_cipher = result.unwrap(); + assert!(updated_cipher.attachments.is_none()); + assert_eq!( + updated_cipher.revision_date.to_rfc3339(), + "2024-01-30T18:00:00+00:00" + ); + } +} diff --git a/crates/bitwarden-vault/src/cipher/cipher_client/mod.rs b/crates/bitwarden-vault/src/cipher/cipher_client/mod.rs index 5e9546a6a..d16ce04d2 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_client/mod.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_client/mod.rs @@ -19,6 +19,7 @@ use crate::{ }; mod create; +mod delete_attachment; mod edit; mod get; mod share_cipher; From 5c894114fed58354532a20ddf5d3875b507def0e Mon Sep 17 00:00:00 2001 From: Mick Letofsky Date: Thu, 13 Nov 2025 17:09:05 +0100 Subject: [PATCH 2/3] Implement suggestions for code improvement --- .../src/cipher/cipher_client/delete_attachment.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs b/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs index 76f93110b..662a3d689 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs @@ -25,8 +25,7 @@ async fn delete_attachment( .await? }; - let boxed_cipher: Box = require!(response.cipher); - let cipher_response = *boxed_cipher; + let cipher_response: Box = require!(response.cipher); let mut cipher = require!(repository.get(cipher_id.to_string()).await?); cipher.revision_date = require!(cipher_response.revision_date) @@ -50,7 +49,7 @@ impl CiphersClient { pub async fn delete_attachment( &self, cipher_id: CipherId, - attachment_id: String, + attachment_id: &str, ) -> Result { let config = self.client.internal.get_api_configurations().await; @@ -68,7 +67,7 @@ impl CiphersClient { pub async fn delete_attachment_as_admin( &self, cipher_id: CipherId, - attachment_id: String, + attachment_id: &str, ) -> Result { let config = self.client.internal.get_api_configurations().await; From 038a9ef9d75d82d11265fbae09f1a6b5b9f6a653 Mon Sep 17 00:00:00 2001 From: Mick Letofsky Date: Mon, 17 Nov 2025 15:45:46 +0100 Subject: [PATCH 3/3] Changes to error handling --- crates/bitwarden-vault/src/cipher/cipher.rs | 8 +--- .../cipher/cipher_client/delete_attachment.rs | 39 ++++++++++++++----- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/crates/bitwarden-vault/src/cipher/cipher.rs b/crates/bitwarden-vault/src/cipher/cipher.rs index 32c3234fe..9b6854f76 100644 --- a/crates/bitwarden-vault/src/cipher/cipher.rs +++ b/crates/bitwarden-vault/src/cipher/cipher.rs @@ -1,7 +1,5 @@ use bitwarden_api_api::{ - apis::ciphers_api::{ - DeleteAttachmentAdminError, DeleteAttachmentError, PutShareError, PutShareManyError, - }, + apis::ciphers_api::{PutShareError, PutShareManyError}, models::{ CipherDetailsResponseModel, CipherRequestModel, CipherResponseModel, CipherWithIdRequestModel, @@ -74,10 +72,6 @@ pub enum CipherError { Chrono(#[from] chrono::ParseError), #[error(transparent)] SerdeJson(#[from] serde_json::Error), - #[error(transparent)] - DeleteAttachment(#[from] bitwarden_api_api::apis::Error), - #[error(transparent)] - DeleteAttachmentAdmin(#[from] bitwarden_api_api::apis::Error), } /// Helper trait for operations on cipher types. diff --git a/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs b/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs index 662a3d689..39be4b53a 100644 --- a/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs +++ b/crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs @@ -1,10 +1,12 @@ use bitwarden_api_api::{apis::ciphers_api::CiphersApi, models}; -use bitwarden_core::require; -use bitwarden_state::repository::Repository; +use bitwarden_core::{ApiError, MissingFieldError, require}; +use bitwarden_error::bitwarden_error; +use bitwarden_state::repository::{Repository, RepositoryError}; +use thiserror::Error; #[cfg(feature = "wasm")] use wasm_bindgen::prelude::wasm_bindgen; -use crate::{Cipher, CipherError, CipherId, CiphersClient, VaultParseError}; +use crate::{Cipher, CipherId, CiphersClient, VaultParseError}; /// Standalone function to delete an attachment from a cipher that is extracted for ease of unit /// testing. @@ -14,15 +16,17 @@ async fn delete_attachment( cipher_id: CipherId, attachment_id: &str, admin: bool, -) -> Result { +) -> Result { let response = if admin { api_client .delete_attachment_admin(cipher_id.into(), attachment_id) - .await? + .await + .map_err(ApiError::from)? } else { api_client .delete_attachment(cipher_id.into(), attachment_id) - .await? + .await + .map_err(ApiError::from)? }; let cipher_response: Box = require!(response.cipher); @@ -50,7 +54,7 @@ impl CiphersClient { &self, cipher_id: CipherId, attachment_id: &str, - ) -> Result { + ) -> Result { let config = self.client.internal.get_api_configurations().await; delete_attachment( @@ -68,7 +72,7 @@ impl CiphersClient { &self, cipher_id: CipherId, attachment_id: &str, - ) -> Result { + ) -> Result { let config = self.client.internal.get_api_configurations().await; delete_attachment( @@ -82,6 +86,20 @@ impl CiphersClient { } } +#[allow(missing_docs)] +#[bitwarden_error(flat)] +#[derive(Debug, Error)] +pub enum CipherDeleteAttachmentError { + #[error(transparent)] + Repository(#[from] RepositoryError), + #[error(transparent)] + ApiError(#[from] ApiError), + #[error(transparent)] + VaultParse(#[from] VaultParseError), + #[error(transparent)] + MissingField(#[from] MissingFieldError), +} + #[cfg(test)] mod tests { use bitwarden_api_api::{apis::ApiClient, models}; @@ -353,7 +371,10 @@ mod tests { .await; assert!(result.is_err()); - assert!(matches!(result.unwrap_err(), CipherError::MissingField(_))); + assert!(matches!( + result.unwrap_err(), + CipherDeleteAttachmentError::MissingField(_) + )); } #[tokio::test]