nhyatt/bitwarden
1
0
Fork 0
mirror of https://github.com/bitwarden/server.git synced 2025-04-04 12:40:22 -05:00
Code

[PM-18088] Implement LimitItemDeletion permission checks for all cipher operations (#5476)

Browse Source 
* Implement enhanced cipher deletion and restore permissions with feature flag support

- Add new method `CanDeleteOrRestoreCipherAsAdminAsync` in CiphersController
- Update NormalCipherPermissions to support more flexible cipher type checking
- Modify CipherService to use new permission checks with feature flag
- Refactor test methods to support new permission logic
- Improve authorization checks for organization cipher management

* Refactor cipher methods to use CipherDetails and simplify type handling

- Update CiphersController to use GetByIdAsync with userId
- Modify NormalCipherPermissions to remove unnecessary type casting
- Update ICipherService and CipherService method signatures to use CipherDetails
- Remove redundant type checking in CipherService methods
- Improve type consistency in cipher-related operations

* Enhance CiphersControllerTests with detailed permission and feature flag scenarios

- Add test methods for DeleteAdmin with edit and manage permission checks
- Implement tests for LimitItemDeletion feature flag scenarios
- Update test method names to reflect more precise permission conditions
- Improve test coverage for admin cipher deletion with granular permission handling

* Add comprehensive test coverage for admin cipher restore operations

- Implement test methods for PutRestoreAdmin and PutRestoreManyAdmin
- Add scenarios for owner and admin roles with LimitItemDeletion feature flag
- Cover permission checks for manage and edit permissions
- Enhance test coverage for single and bulk cipher restore admin operations
- Verify correct invocation of RestoreAsync and RestoreManyAsync methods

* Refactor CiphersControllerTests to remove redundant assertions and mocking

- Remove unnecessary assertions for null checks
- Simplify mocking setup for cipher repository and service methods
- Clean up redundant type and data setup in test methods
- Improve test method clarity by removing extraneous code

* Add comprehensive test coverage for cipher restore, delete, and soft delete operations

- Implement test methods for RestoreAsync with org admin override and LimitItemDeletion feature flag
- Add scenarios for checking manage and edit permissions during restore operations
- Extend test coverage for DeleteAsync with similar permission and feature flag checks
- Enhance SoftDeleteAsync tests with org admin override and permission validation
- Improve test method names to reflect precise permission conditions

* Add comprehensive test coverage for cipher restore, delete, and soft delete operations

- Extend test methods for RestoreManyAsync with various permission scenarios
- Add test coverage for personal and organization ciphers in restore operations
- Implement tests for RestoreManyAsync with LimitItemDeletion feature flag
- Add detailed test scenarios for delete and soft delete operations
- Improve test method names to reflect precise permission and feature flag conditions

* Refactor authorization checks in CiphersController to use All() method for improved readability

* Refactor filtering of ciphers in CipherService to streamline organization ability checks and improve readability
This commit is contained in:
Rui Tomé 2025-04-02 10:52:23 +01:00 committed by GitHub
parent f90bcd44de
commit abe593d221
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 2067 additions and 278 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

96
src/Api/Vault/Controllers/CiphersController.cs
View File

@ -16,6 +16,7 @@ using Bit.Core.Services;
using Bit.Core.Settings;
using Bit.Core.Tools.Services;
using Bit.Core.Utilities;
using Bit.Core.Vault.Authorization.Permissions;
using Bit.Core.Vault.Entities;
using Bit.Core.Vault.Models.Data;
using Bit.Core.Vault.Queries;
@ -345,6 +346,77 @@ public class CiphersController : Controller
return await CanEditCiphersAsync(organizationId, cipherIds);
}
private async Task<bool> CanDeleteOrRestoreCipherAsAdminAsync(Guid organizationId, IEnumerable<Guid> cipherIds)
{
if (!_featureService.IsEnabled(FeatureFlagKeys.LimitItemDeletion))
{
return await CanEditCipherAsAdminAsync(organizationId, cipherIds);
}
var org = _currentContext.GetOrganization(organizationId);
// If we're not an "admin", we don't need to check the ciphers
if (org is not ({ Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or { Permissions.EditAnyCollection: true }))
{
// Are we a provider user? If so, we need to be sure we're not restricted
// Once the feature flag is removed, this check can be combined with the above
if (await _currentContext.ProviderUserForOrgAsync(organizationId))
{
// Provider is restricted from editing ciphers, so we're not an "admin"
if (_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess))
{
return false;
}
// Provider is unrestricted, so we're an "admin", don't return early
}
else
{
// Not a provider or admin
return false;
}
}
// If the user can edit all ciphers for the organization, just check they all belong to the org
if (await CanEditAllCiphersAsync(organizationId))
{
// TODO: This can likely be optimized to only query the requested ciphers and then checking they belong to the org
var orgCiphers = (await _cipherRepository.GetManyByOrganizationIdAsync(organizationId)).ToDictionary(c => c.Id);
// Ensure all requested ciphers are in orgCiphers
return cipherIds.All(c => orgCiphers.ContainsKey(c));
}
// The user cannot access any ciphers for the organization, we're done
if (!await CanAccessOrganizationCiphersAsync(organizationId))
{
return false;
}
var user = await _userService.GetUserByPrincipalAsync(User);
// Select all deletable ciphers for this user belonging to the organization
var deletableOrgCipherList = (await _cipherRepository.GetManyByUserIdAsync(user.Id, true))
.Where(c => c.OrganizationId == organizationId && c.UserId == null).ToList();
// Special case for unassigned ciphers
if (await CanAccessUnassignedCiphersAsync(organizationId))
{
var unassignedCiphers =
(await _cipherRepository.GetManyUnassignedOrganizationDetailsByOrganizationIdAsync(
organizationId));
// Users that can access unassigned ciphers can also delete them
deletableOrgCipherList.AddRange(unassignedCiphers.Select(c => new CipherDetails(c) { Manage = true }));
}
var organizationAbility = await _applicationCacheService.GetOrganizationAbilityAsync(organizationId);
var deletableOrgCiphers = deletableOrgCipherList
.Where(c => NormalCipherPermissions.CanDelete(user, c, organizationAbility))
.ToDictionary(c => c.Id);
return cipherIds.All(c => deletableOrgCiphers.ContainsKey(c));
}
/// <summary>
/// TODO: Move this to its own authorization handler or equivalent service - AC-2062
/// </summary>
@ -763,12 +835,12 @@ public class CiphersController : Controller
[HttpDelete("{id}/admin")]
[HttpPost("{id}/delete-admin")]
public async Task DeleteAdmin(string id)
public async Task DeleteAdmin(Guid id)
{
var userId = _userService.GetProperUserId(User).Value;
var cipher = await _cipherRepository.GetByIdAsync(new Guid(id));
var cipher = await GetByIdAsync(id, userId);
if (cipher == null || !cipher.OrganizationId.HasValue ||
!await CanEditCipherAsAdminAsync(cipher.OrganizationId.Value, new[] { cipher.Id }))
!await CanDeleteOrRestoreCipherAsAdminAsync(cipher.OrganizationId.Value, new[] { cipher.Id }))
{
throw new NotFoundException();
}
@ -808,7 +880,7 @@ public class CiphersController : Controller
var cipherIds = model.Ids.Select(i => new Guid(i)).ToList();
if (string.IsNullOrWhiteSpace(model.OrganizationId) ||
!await CanEditCipherAsAdminAsync(new Guid(model.OrganizationId), cipherIds))
!await CanDeleteOrRestoreCipherAsAdminAsync(new Guid(model.OrganizationId), cipherIds))
{
throw new NotFoundException();
}
@ -830,12 +902,12 @@ public class CiphersController : Controller
}
[HttpPut("{id}/delete-admin")]
public async Task PutDeleteAdmin(string id)
public async Task PutDeleteAdmin(Guid id)
{
var userId = _userService.GetProperUserId(User).Value;
var cipher = await _cipherRepository.GetByIdAsync(new Guid(id));
var cipher = await GetByIdAsync(id, userId);
if (cipher == null || !cipher.OrganizationId.HasValue ||
!await CanEditCipherAsAdminAsync(cipher.OrganizationId.Value, new[] { cipher.Id }))
!await CanDeleteOrRestoreCipherAsAdminAsync(cipher.OrganizationId.Value, new[] { cipher.Id }))
{
throw new NotFoundException();
}
@ -871,7 +943,7 @@ public class CiphersController : Controller
var cipherIds = model.Ids.Select(i => new Guid(i)).ToList();
if (string.IsNullOrWhiteSpace(model.OrganizationId) ||
!await CanEditCipherAsAdminAsync(new Guid(model.OrganizationId), cipherIds))
!await CanDeleteOrRestoreCipherAsAdminAsync(new Guid(model.OrganizationId), cipherIds))
{
throw new NotFoundException();
}
@ -899,12 +971,12 @@ public class CiphersController : Controller
}
[HttpPut("{id}/restore-admin")]
public async Task<CipherMiniResponseModel> PutRestoreAdmin(string id)
public async Task<CipherMiniResponseModel> PutRestoreAdmin(Guid id)
{
var userId = _userService.GetProperUserId(User).Value;
var cipher = await _cipherRepository.GetOrganizationDetailsByIdAsync(new Guid(id));
var cipher = await GetByIdAsync(id, userId);
if (cipher == null || !cipher.OrganizationId.HasValue ||
!await CanEditCipherAsAdminAsync(cipher.OrganizationId.Value, new[] { cipher.Id }))
!await CanDeleteOrRestoreCipherAsAdminAsync(cipher.OrganizationId.Value, new[] { cipher.Id }))
{
throw new NotFoundException();
}
@ -944,7 +1016,7 @@ public class CiphersController : Controller
var cipherIdsToRestore = new HashSet<Guid>(model.Ids.Select(i => new Guid(i)));
if (model.OrganizationId == default || !await CanEditCipherAsAdminAsync(model.OrganizationId, cipherIdsToRestore))
if (model.OrganizationId == default || !await CanDeleteOrRestoreCipherAsAdminAsync(model.OrganizationId, cipherIdsToRestore))
{
throw new NotFoundException();
}

6
src/Core/Vault/Services/ICipherService.cs
View File

@ -15,7 +15,7 @@ public interface ICipherService
long requestLength, Guid savingUserId, bool orgAdmin = false);
Task CreateAttachmentShareAsync(Cipher cipher, Stream stream, string fileName, string key, long requestLength,
string attachmentId, Guid organizationShareId);
Task DeleteAsync(Cipher cipher, Guid deletingUserId, bool orgAdmin = false);
Task DeleteAsync(CipherDetails cipherDetails, Guid deletingUserId, bool orgAdmin = false);
Task DeleteManyAsync(IEnumerable<Guid> cipherIds, Guid deletingUserId, Guid? organizationId = null, bool orgAdmin = false);
Task<DeleteAttachmentResponseData> DeleteAttachmentAsync(Cipher cipher, string attachmentId, Guid deletingUserId, bool orgAdmin = false);
Task PurgeAsync(Guid organizationId);
@ -27,9 +27,9 @@ public interface ICipherService
Task ShareManyAsync(IEnumerable<(Cipher cipher, DateTime? lastKnownRevisionDate)> ciphers, Guid organizationId,
IEnumerable<Guid> collectionIds, Guid sharingUserId);
Task SaveCollectionsAsync(Cipher cipher, IEnumerable<Guid> collectionIds, Guid savingUserId, bool orgAdmin);
Task SoftDeleteAsync(Cipher cipher, Guid deletingUserId, bool orgAdmin = false);
Task SoftDeleteAsync(CipherDetails cipherDetails, Guid deletingUserId, bool orgAdmin = false);
Task SoftDeleteManyAsync(IEnumerable<Guid> cipherIds, Guid deletingUserId, Guid? organizationId = null, bool orgAdmin = false);
Task RestoreAsync(Cipher cipher, Guid restoringUserId, bool orgAdmin = false);
Task RestoreAsync(CipherDetails cipherDetails, Guid restoringUserId, bool orgAdmin = false);
Task<ICollection<CipherOrganizationDetails>> RestoreManyAsync(IEnumerable<Guid> cipherIds, Guid restoringUserId, Guid? organizationId = null, bool orgAdmin = false);
Task UploadFileForExistingAttachmentAsync(Stream stream, Cipher cipher, CipherAttachment.MetaData attachmentId);
Task<AttachmentResponseData> GetAttachmentDownloadDataAsync(Cipher cipher, string attachmentId);

131
src/Core/Vault/Services/Implementations/CipherService.cs
View File

@ -14,6 +14,7 @@ using Bit.Core.Tools.Enums;
using Bit.Core.Tools.Models.Business;
using Bit.Core.Tools.Services;
using Bit.Core.Utilities;
using Bit.Core.Vault.Authorization.Permissions;
using Bit.Core.Vault.Entities;
using Bit.Core.Vault.Enums;
using Bit.Core.Vault.Models.Data;
@ -44,6 +45,7 @@ public class CipherService : ICipherService
private readonly ICurrentContext _currentContext;
private readonly IGetCipherPermissionsForUserQuery _getCipherPermissionsForUserQuery;
private readonly IPolicyRequirementQuery _policyRequirementQuery;
private readonly IApplicationCacheService _applicationCacheService;
private readonly IFeatureService _featureService;
public CipherService(
@ -64,6 +66,7 @@ public class CipherService : ICipherService
ICurrentContext currentContext,
IGetCipherPermissionsForUserQuery getCipherPermissionsForUserQuery,
IPolicyRequirementQuery policyRequirementQuery,
IApplicationCacheService applicationCacheService,
IFeatureService featureService)
{
_cipherRepository = cipherRepository;
@ -83,6 +86,7 @@ public class CipherService : ICipherService
_currentContext = currentContext;
_getCipherPermissionsForUserQuery = getCipherPermissionsForUserQuery;
_policyRequirementQuery = policyRequirementQuery;
_applicationCacheService = applicationCacheService;
_featureService = featureService;
}
@ -421,19 +425,19 @@ public class CipherService : ICipherService
return response;
}
public async Task DeleteAsync(Cipher cipher, Guid deletingUserId, bool orgAdmin = false)
public async Task DeleteAsync(CipherDetails cipherDetails, Guid deletingUserId, bool orgAdmin = false)
{
if (!orgAdmin && !(await UserCanEditAsync(cipher, deletingUserId)))
if (!orgAdmin && !await UserCanDeleteAsync(cipherDetails, deletingUserId))
{
throw new BadRequestException("You do not have permissions to delete this.");
}
await _cipherRepository.DeleteAsync(cipher);
await _attachmentStorageService.DeleteAttachmentsForCipherAsync(cipher.Id);
await _eventService.LogCipherEventAsync(cipher, EventType.Cipher_Deleted);
await _cipherRepository.DeleteAsync(cipherDetails);
await _attachmentStorageService.DeleteAttachmentsForCipherAsync(cipherDetails.Id);
await _eventService.LogCipherEventAsync(cipherDetails, EventType.Cipher_Deleted);
// push
await _pushService.PushSyncCipherDeleteAsync(cipher);
await _pushService.PushSyncCipherDeleteAsync(cipherDetails);
}
public async Task DeleteManyAsync(IEnumerable<Guid> cipherIds, Guid deletingUserId, Guid? organizationId = null, bool orgAdmin = false)
@ -450,8 +454,8 @@ public class CipherService : ICipherService
else
{
var ciphers = await _cipherRepository.GetManyByUserIdAsync(deletingUserId);
deletingCiphers = ciphers.Where(c => cipherIdsSet.Contains(c.Id) && c.Edit).Select(x => (Cipher)x).ToList();
var filteredCiphers = await FilterCiphersByDeletePermission(ciphers, cipherIdsSet, deletingUserId);
deletingCiphers = filteredCiphers.Select(c => (Cipher)c).ToList();
await _cipherRepository.DeleteAsync(deletingCiphers.Select(c => c.Id), deletingUserId);
}
@ -703,33 +707,26 @@ public class CipherService : ICipherService
await _pushService.PushSyncCipherUpdateAsync(cipher, collectionIds);
}
public async Task SoftDeleteAsync(Cipher cipher, Guid deletingUserId, bool orgAdmin = false)
public async Task SoftDeleteAsync(CipherDetails cipherDetails, Guid deletingUserId, bool orgAdmin = false)
{
if (!orgAdmin && !(await UserCanEditAsync(cipher, deletingUserId)))
if (!orgAdmin && !await UserCanDeleteAsync(cipherDetails, deletingUserId))
{
throw new BadRequestException("You do not have permissions to soft delete this.");
}
if (cipher.DeletedDate.HasValue)
if (cipherDetails.DeletedDate.HasValue)
{
// Already soft-deleted, we can safely ignore this
return;
}
cipher.DeletedDate = cipher.RevisionDate = DateTime.UtcNow;
cipherDetails.DeletedDate = cipherDetails.RevisionDate = DateTime.UtcNow;
if (cipher is CipherDetails details)
{
await _cipherRepository.UpsertAsync(details);
}
else
{
await _cipherRepository.UpsertAsync(cipher);
}
await _eventService.LogCipherEventAsync(cipher, EventType.Cipher_SoftDeleted);
await _cipherRepository.UpsertAsync(cipherDetails);
await _eventService.LogCipherEventAsync(cipherDetails, EventType.Cipher_SoftDeleted);
// push
await _pushService.PushSyncCipherUpdateAsync(cipher, null);
await _pushService.PushSyncCipherUpdateAsync(cipherDetails, null);
}
public async Task SoftDeleteManyAsync(IEnumerable<Guid> cipherIds, Guid deletingUserId, Guid? organizationId, bool orgAdmin)
@ -746,8 +743,8 @@ public class CipherService : ICipherService
else
{
var ciphers = await _cipherRepository.GetManyByUserIdAsync(deletingUserId);
deletingCiphers = ciphers.Where(c => cipherIdsSet.Contains(c.Id) && c.Edit).Select(x => (Cipher)x).ToList();
var filteredCiphers = await FilterCiphersByDeletePermission(ciphers, cipherIdsSet, deletingUserId);
deletingCiphers = filteredCiphers.Select(c => (Cipher)c).ToList();
await _cipherRepository.SoftDeleteAsync(deletingCiphers.Select(c => c.Id), deletingUserId);
}
@ -762,34 +759,27 @@ public class CipherService : ICipherService
await _pushService.PushSyncCiphersAsync(deletingUserId);
}
public async Task RestoreAsync(Cipher cipher, Guid restoringUserId, bool orgAdmin = false)
public async Task RestoreAsync(CipherDetails cipherDetails, Guid restoringUserId, bool orgAdmin = false)
{
if (!orgAdmin && !(await UserCanEditAsync(cipher, restoringUserId)))
if (!orgAdmin && !await UserCanRestoreAsync(cipherDetails, restoringUserId))
{
throw new BadRequestException("You do not have permissions to delete this.");
}
if (!cipher.DeletedDate.HasValue)
if (!cipherDetails.DeletedDate.HasValue)
{
// Already restored, we can safely ignore this
return;
}
cipher.DeletedDate = null;
cipher.RevisionDate = DateTime.UtcNow;
cipherDetails.DeletedDate = null;
cipherDetails.RevisionDate = DateTime.UtcNow;
if (cipher is CipherDetails details)
{
await _cipherRepository.UpsertAsync(details);
}
else
{
await _cipherRepository.UpsertAsync(cipher);
}
await _eventService.LogCipherEventAsync(cipher, EventType.Cipher_Restored);
await _cipherRepository.UpsertAsync(cipherDetails);
await _eventService.LogCipherEventAsync(cipherDetails, EventType.Cipher_Restored);
// push
await _pushService.PushSyncCipherUpdateAsync(cipher, null);
await _pushService.PushSyncCipherUpdateAsync(cipherDetails, null);
}
public async Task<ICollection<CipherOrganizationDetails>> RestoreManyAsync(IEnumerable<Guid> cipherIds, Guid restoringUserId, Guid? organizationId = null, bool orgAdmin = false)
@ -812,8 +802,8 @@ public class CipherService : ICipherService
else
{
var ciphers = await _cipherRepository.GetManyByUserIdAsync(restoringUserId);
restoringCiphers = ciphers.Where(c => cipherIdsSet.Contains(c.Id) && c.Edit).Select(c => (CipherOrganizationDetails)c).ToList();
var filteredCiphers = await FilterCiphersByDeletePermission(ciphers, cipherIdsSet, restoringUserId);
restoringCiphers = filteredCiphers.Select(c => (CipherOrganizationDetails)c).ToList();
revisionDate = await _cipherRepository.RestoreAsync(restoringCiphers.Select(c => c.Id), restoringUserId);
}
@ -844,6 +834,34 @@ public class CipherService : ICipherService
return await _cipherRepository.GetCanEditByIdAsync(userId, cipher.Id);
}
private async Task<bool> UserCanDeleteAsync(CipherDetails cipher, Guid userId)
{
if (!_featureService.IsEnabled(FeatureFlagKeys.LimitItemDeletion))
{
return await UserCanEditAsync(cipher, userId);
}
var user = await _userService.GetUserByIdAsync(userId);
var organizationAbility = cipher.OrganizationId.HasValue ?
await _applicationCacheService.GetOrganizationAbilityAsync(cipher.OrganizationId.Value) : null;
return NormalCipherPermissions.CanDelete(user, cipher, organizationAbility);
}
private async Task<bool> UserCanRestoreAsync(CipherDetails cipher, Guid userId)
{
if (!_featureService.IsEnabled(FeatureFlagKeys.LimitItemDeletion))
{
return await UserCanEditAsync(cipher, userId);
}
var user = await _userService.GetUserByIdAsync(userId);
var organizationAbility = cipher.OrganizationId.HasValue ?
await _applicationCacheService.GetOrganizationAbilityAsync(cipher.OrganizationId.Value) : null;
return NormalCipherPermissions.CanRestore(user, cipher, organizationAbility);
}
private void ValidateCipherLastKnownRevisionDateAsync(Cipher cipher, DateTime? lastKnownRevisionDate)
{
if (cipher.Id == default || !lastKnownRevisionDate.HasValue)
@ -1010,4 +1028,35 @@ public class CipherService : ICipherService
cipher.Data = JsonSerializer.Serialize(newCipherData);
}
}
// This method is used to filter ciphers based on the user's permissions to delete them.
// It supports both the old and new logic depending on the feature flag.
private async Task<List<T>> FilterCiphersByDeletePermission<T>(
IEnumerable<T> ciphers,
HashSet<Guid> cipherIdsSet,
Guid userId) where T : CipherDetails
{
if (!_featureService.IsEnabled(FeatureFlagKeys.LimitItemDeletion))
{
return ciphers.Where(c => cipherIdsSet.Contains(c.Id) && c.Edit).ToList();
}
var user = await _userService.GetUserByIdAsync(userId);
var organizationAbilities = await _applicationCacheService.GetOrganizationAbilitiesAsync();
var filteredCiphers = ciphers
.Where(c => cipherIdsSet.Contains(c.Id))
.GroupBy(c => c.OrganizationId)
.SelectMany(group =>
{
var organizationAbility = group.Key.HasValue &&
organizationAbilities.TryGetValue(group.Key.Value, out var ability) ?
ability : null;
return group.Where(c => NormalCipherPermissions.CanDelete(user, c, organizationAbility));
})
.ToList();
return filteredCiphers;
}
}

957
test/Api.Test/Vault/Controllers/CiphersControllerTests.cs
View File

File diff suppressed because it is too large Load Diff

1155
test/Core.Test/Vault/Services/CipherServiceTests.cs
View File

File diff suppressed because it is too large Load Diff