[PM-25820] - Implement the ability to delete an attachment #557
Conversation
theMickster
requested review from
Copilot and
jaasen-livefront
and removed request for
jaasen-livefront
|
Claude finished @theMickster's task —— View job Code Review SummaryI've reviewed the latest commits implementing attachment deletion functionality. The code follows good patterns with comprehensive test coverage, but there are critical issues that must be addressed before merging. Critical IssuesFinding 1: ❌ Race condition causing potential data loss remains unaddressed nikwithak's concern about the race condition in delete_attachment.rs:35-41 is valid and has not been resolved. The current implementation creates a scenario where concurrent updates can be lost: Race Condition ExampleThe API returns a full Resolution Required: Either:
Finding 2: ❌ Feature-specific errors polluting general CipherError enum Hinton correctly identified that adding
Status: ✅ Resolved in commit 038a9ef Issues Requiring AttentionFinding 3: dani-garcia's suggestion about removing the manual deref was implemented in commit 5c89411: // Before: let cipher_response = *boxed_cipher;
// After: let cipher_response: Box<models::Cipher> = require!(response.cipher);Status: ✅ Resolved in commit 5c89411 Finding 4: dani-garcia's suggestion to change Status: ✅ Resolved in commit 5c89411 Code Quality ObservationsFinding 5: 💭 Test coverage at 92.85% with 28 uncovered lines Codecov reports 92.85% patch coverage with 28 lines missing coverage in delete_attachment.rs. The test suite is comprehensive with tests for:
The uncovered lines are likely error path branches. While 92.85% is good, consider adding tests for:
PR Metadata ReviewFinding 6: 💭 PR title and description could be more specific The PR title "PM-25820 - Implement the ability to delete an attachment" is adequate but could be more specific about the scope:
The Objective section provides good context about the implementation approach, though the justification for ignoring the full API response needs reconsideration given the race condition concern. RecommendationsMust address before merge:
Should consider: Good Practices Observed
Commits Reviewed: 82dbf96 (initial), 5c89411 (suggestions), 038a9ef (error handling) |
There was a problem hiding this comment.
Pull Request Overview
This PR implements the ability to delete attachments from ciphers by adding two new public methods to CiphersClient: delete_attachment for regular users and delete_attachment_as_admin for administrators. The implementation follows a minimal approach, only updating the cipher's revision date and removing the deleted attachment from the attachments collection, rather than re-syncing all cipher data from the server response.
Key Changes
- Added standalone
delete_attachmentfunction that handles API calls, updates the cipher's revision date from the server response, removes the deleted attachment from the local cipher, and persists the updated cipher to the repository - Added two new error variants to
CipherErrorfor handling delete attachment API errors - Comprehensive test coverage including success cases, error cases, and edge cases like missing ciphers and ciphers with no attachments
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs | New module containing the delete attachment implementation and comprehensive unit tests covering various scenarios |
| crates/bitwarden-vault/src/cipher/cipher.rs | Added two new error variants (DeleteAttachment and DeleteAttachmentAdmin) to CipherError enum for API error handling |
| crates/bitwarden-vault/src/cipher/cipher_client/mod.rs | Added module declaration for the new delete_attachment module |
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #557 +/- ##
==========================================
- Coverage 79.94% 79.86% -0.08%
==========================================
Files 298 301 +3
Lines 32129 32636 +507
==========================================
+ Hits 25686 26066 +380
- Misses 6443 6570 +127 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
theMickster
changed the title
dani-garcia
left a comment
dani-garcia
left a comment
There was a problem hiding this comment.
Looks pretty good to me, just some small suggestions and a possible future improvement.
crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs
Outdated
Show resolved
Hide resolved
crates/bitwarden-vault/src/cipher/cipher_client/delete_attachment.rs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| repository | ||
| .set(cipher_id.to_string(), cipher.clone()) |
There was a problem hiding this comment.
Non-blocking improvement: The API seems to return the full cipher, so rather than manually modify the cipher we have locally, it might be easier and less error prone to just take the response cipher and stick it into the repository. This might need some conversion function like impl TryFrom<models::Cipher> for Cipher to be implemented though, so we can leave that for later.
There was a problem hiding this comment.
So it appears to bring back the full Cipher, but it does not It's just the Cipher entity which lacks a few properties.
| cipher.revision_date = require!(cipher_response.revision_date) | ||
| .parse() | ||
| .map_err(Into::<VaultParseError>::into)?; | ||
|
|
||
| if let Some(ref mut attachments) = cipher.attachments { | ||
| attachments.retain(|a| a.id.as_deref() != Some(attachment_id)); | ||
| } |
There was a problem hiding this comment.
e.g.
Client A -> Changes password on Cipher.
Client B -> Deletes an Attachment
Client B -> Changes name of Cipher
Client A -- Original password change is overwritten (Client B didn't sync the latest password, but did have the correct revision_date)
In line with @dani-garcia's comment below, I think it's best to store the cipher in the response directly, which will also save you these steps of manually updating the cipher.
There was a problem hiding this comment.
Ok; I have added a note in Slack for us to chat @nikwithak, @gbubemismith & @shane-melton.
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
theMickster
added
help wanted
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-25820
📔 Objective
Analyzed the functionality found in both the client and server code regarding deleting of attachments and chose to implement as simply as possible. I found these three steps: removal of the file from storage, removal from the collection of attachments, and an update of the RevisionDate { see Vault/Services/Implementations/CipherService.cs for details}.
Unless I am missing something & we are in some unforeseen data state with a Chiper, I do not think we need to concern ourselves with the rest of the payload returned and going through the re-mapping of data that should not have changed. Also, given that the payload from the delete attachments API endpoints is an entity instead of a response model, I feel we only seek to complicate things further by over engineering.
More than welcome to other perspectives.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes