[PM-19383] add admin endpoint, fix typecasting error #5681
Conversation
|
New Issues (1)Checkmarx found the following issues in this Pull Request
Fixed Issues (5)Great job! The following issues were fixed in this Pull Request
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5681 +/- ##
==========================================
+ Coverage 46.85% 47.18% +0.32%
==========================================
Files 1615 1633 +18
Lines 73587 74207 +620
Branches 6622 6683 +61
==========================================
+ Hits 34482 35015 +533
- Misses 37660 37740 +80
- Partials 1445 1452 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -891,7 +891,7 @@ private async Task<DeleteAttachmentResponseData> DeleteAttachmentAsync(Cipher ci | |||
|
|
|||
| // Update the revision date when an attachment is deleted | |||
| cipher.RevisionDate = DateTime.UtcNow; | |||
| await _cipherRepository.ReplaceAsync((CipherDetails)cipher); | |||
| await _cipherRepository.ReplaceAsync(cipher); | |||
There was a problem hiding this comment.
Was a runtime error occurring because of the cast?
There was a problem hiding this comment.
Yes:
Bit.Api.Utilities.ExceptionHandlerFilterAttribute[0]
Unable to cast object of type 'Bit.Core.Vault.Entities.Cipher' to type 'Bit.Core.Vault.Models.Data.CipherDetails'.
System.InvalidCastException: Unable to cast object of type 'Bit.Core.Vault.Entities.Cipher' to type 'Bit.Core.Vault.Models.Data.CipherDetails'.
at Bit.Core.Vault.Services.CipherService.DeleteAttachmentAsync(Cipher cipher, MetaData attachmentData) in /Users/brandontreston/development/server/src/Core/Vault/Services/Implementations/CipherService.cs:line 894
I'll investigate this a bit more and see what we need here and why a Cipher object is just working here.
There was a problem hiding this comment.
At a first glance, it seems to be because Cipher is a superclass of CipherOrganizationDetails => CipherDetails. It works because it still has the data we need to be able to update the Cipher in the DB. Only concern here is that it leaves a {} in the DB after delete instead of null. Not sure how big an issue that really is though.
There was a problem hiding this comment.
Without the cast I'm now getting an error trying to delete an attachment from the web vault.
I'm jumping in an out of meetings for a majority of my day today so I haven't had a chance to dig into why yet.
cc: @shane-melton @gbubemismith
There was a problem hiding this comment.
This is fixed in this commit. It's odd that we retrieve this object that is a CipherDetails object but we're passing it to methods explicitly expecting Cipher only to seemingly "cast" it back to its original type. Might be a 🌱 for a refactor (out of scope for this).
There was a problem hiding this comment.
Thanks for looking into it! I agree there is some oddity here. I'm able to delete attachments from password manager now ✅
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-19383
📔 Objective
Fixes admins not being able to manage attachments
Clients Pr: bitwarden/clients#14363
📸 Screenshots
Screen.Recording.2025-04-21.at.2.47.39.PM.mov
⏰ Reminders before review
🦮 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 confirmed issue 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