[PM-18091] Update cipher delete & restore permissions #1474
Conversation
…issions for delete.
… cipher permissions
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1474 +/- ##
==========================================
+ Coverage 89.56% 89.59% +0.02%
==========================================
Files 769 773 +4
Lines 48392 48539 +147
==========================================
+ Hits 43343 43489 +146
- Misses 5049 5050 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// | ||
| func loadRestrictItemDeletionFlag() async { | ||
| state.restrictItemDeletionFlagEnabled = await services.configService.getFeatureFlag( | ||
| FeatureFlag.restrictItemDeletion |
There was a problem hiding this comment.
⛏️ I think there's no need to indicate the enum, it should be implicit.
| FeatureFlag.restrictItemDeletion | |
| .restrictItemDeletion |
| /// A feature flag for the use of new cipher permission properties. | ||
| case restrictItemDeletion = "pm-15493-restrict-item-deletion-to-can-manage-permission" | ||
|
|
There was a problem hiding this comment.
⛏️ 🤔 Does this affect any item of the app, like a send or an attachment? Or does this only affect deletion of ciphers? If the latter I'd update the case name to something more explicit like restrictCipherItemDeletion. I would guess you wanted to match the case name with the flag string value but if we can be more explicit the better so we don't actually need to read the flag comment here to see what's being used for.
However, if you have reasons to keep using the current one please let me know.
There was a problem hiding this comment.
Yes this is a property only for ciphers, I'll update the naming to better reflect the scope of action.
| /// Whether or not this item can be deleted by the user. | ||
| /// New permission model from PM-18091 | ||
| var canBeDeletedPermission: Bool { | ||
| // backwards compatibility for old server versions | ||
| guard let cipherPermissions = cipher.permissions else { | ||
| return canBeDeleted | ||
| } | ||
| return cipherPermissions.delete | ||
| } |
There was a problem hiding this comment.
🎨 I believe you don't need the extra property and you can also save making the checks in the views if you combine this property with canBeDeleted, i.e.:
/// Whether or not this item can be deleted by the user.
var canBeDeleted: Bool {
// New permission model from PM-18091
if restrictItemDeletionFlagEnabled, let cipherPermissions = cipher.permissions {
return cipherPermissions.delete
}
guard !collectionIds.isEmpty else { return true }
return collections.contains { collection in
guard let id = collection.id else { return false }
return collection.manage && collectionIds.contains(id)
}
}| let restrictItemDeletionFlagEnabled: Bool = await services.configService.getFeatureFlag( | ||
| FeatureFlag.restrictItemDeletion | ||
| ) |
There was a problem hiding this comment.
⛏️ I believe you can skip the enum name reference:
| let restrictItemDeletionFlagEnabled: Bool = await services.configService.getFeatureFlag( | |
| FeatureFlag.restrictItemDeletion | |
| ) | |
| let restrictItemDeletionFlagEnabled: Bool = await services.configService.getFeatureFlag( | |
| .restrictItemDeletion | |
| ) |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-18091
📔 Objective
Use new cipher permissions properties to show or hide the delete and restore buttons on the UI.
📸 Screenshots
Screen.Recording.2025-03-31.at.23.02.18.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