Skip to content

[PM-33500] - delete attachments from deleted ciphers #7208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jaasen-livefront wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[PM-33500] - delete attachments from deleted ciphers #7208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe0f808
delete attachments from deleted ciphers
jaasen-livefront Mar 13, 2026
3048893
Merge branch 'main' into PM-33500
gbubemismith Mar 18, 2026
834a1b9
add DeleteAttachmentsForOrganizationAsync to cipher service. update s…
jaasen-livefront Mar 18, 2026
5dcb0f2
fix test
jaasen-livefront Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/Core/AdminConsole/OrganizationFeatures/Organizations/OrganizationDeleteCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Bit.Core.Exceptions;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Vault.Services;

namespace Bit.Core.AdminConsole.OrganizationFeatures.Organizations;

Expand All @@ -15,17 +16,20 @@ public class OrganizationDeleteCommand : IOrganizationDeleteCommand
private readonly IOrganizationRepository _organizationRepository;
private readonly IStripePaymentService _paymentService;
private readonly ISsoConfigRepository _ssoConfigRepository;
private readonly ICipherService _cipherService;

public OrganizationDeleteCommand(
IApplicationCacheService applicationCacheService,
IOrganizationRepository organizationRepository,
IStripePaymentService paymentService,
ISsoConfigRepository ssoConfigRepository)
ISsoConfigRepository ssoConfigRepository,
ICipherService cipherService)
{
_applicationCacheService = applicationCacheService;
_organizationRepository = organizationRepository;
_paymentService = paymentService;
_ssoConfigRepository = ssoConfigRepository;
_cipherService = cipherService;
}

public async Task DeleteAsync(Organization organization)
Expand All @@ -43,6 +47,7 @@ public async Task DeleteAsync(Organization organization)
catch (GatewayException) { }
}

await _cipherService.DeleteAttachmentsForOrganizationAsync(organization.Id);
await _organizationRepository.DeleteAsync(organization);
await _applicationCacheService.DeleteOrganizationAbilityAsync(organization.Id);
}
Expand Down
1 change: 1 addition & 0 deletions src/Core/Vault/Services/ICipherService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,5 @@ Task<IEnumerable<CipherDetails>> ShareManyAsync(IEnumerable<(CipherDetails ciphe
Task<bool> ValidateCipherAttachmentFile(Cipher cipher, CipherAttachment.MetaData attachmentData);
Task ValidateBulkCollectionAssignmentAsync(IEnumerable<Guid> collectionIds, IEnumerable<Guid> cipherIds, Guid userId);
Task ValidateCipherEditForAttachmentAsync(Cipher cipher, Guid savingUserId, bool orgAdmin, long requestLength);
Task DeleteAttachmentsForOrganizationAsync(Guid organizationId);
}
22 changes: 22 additions & 0 deletions src/Core/Vault/Services/Implementations/CipherService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,12 @@ public async Task DeleteManyAsync(IEnumerable<Guid> cipherIds, Guid deletingUser
await _cipherRepository.DeleteAsync(deletingCiphers.Select(c => c.Id), deletingUserId);
}

// Clean up attachment files from storage
foreach (var cipher in deletingCiphers)
{
await _attachmentStorageService.DeleteAttachmentsForCipherAsync(cipher.Id);
Copy link
Contributor

@JaredScar JaredScar Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By doing what I suggested above, we can probably also get rid of another for loop here as well

}

var events = deletingCiphers.Select(c =>
new Tuple<Cipher, EventType, DateTime?>(c, EventType.Cipher_Deleted, null));
foreach (var eventsBatch in events.Chunk(100))
Expand Down Expand Up @@ -494,10 +500,26 @@ public async Task PurgeAsync(Guid organizationId)
{
throw new NotFoundException();
}


await DeleteAttachmentsForOrganizationAsync(organizationId);

await _cipherRepository.DeleteByOrganizationIdAsync(organizationId);

await _eventService.LogOrganizationEventAsync(org, EventType.Organization_PurgedVault);
}

public async Task DeleteAttachmentsForOrganizationAsync(Guid organizationId)
{
var ciphersWithAttachments = (await _cipherRepository.GetManyByOrganizationIdAsync(organizationId))
.Where(c => c.GetAttachments()?.Count > 0);
Comment on lines +514 to +515
Copy link
Contributor

@gbubemismith gbubemismith Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ @jaasen-livefront, is there a reason we will need the whole cipher object? since we just need the cipher Ids, we can add a .Select(c => c.Id) to this


foreach (var cipher in ciphersWithAttachments)
{
await _attachmentStorageService.DeleteAttachmentsForCipherAsync(cipher.Id);
Copy link
Contributor

@JaredScar JaredScar Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By having 1 spot where we only supply the organization ID to pass, we can get rid of all these for loops! 😄

}
}

public async Task MoveManyAsync(IEnumerable<Guid> cipherIds, Guid? destinationFolderId, Guid movingUserId)
{
if (destinationFolderId.HasValue)
Expand Down
10 changes: 7 additions & 3 deletions ...re.Test/AdminConsole/OrganizationFeatures/Organizations/OrganizationDeleteCommandTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Test.AutoFixture.OrganizationFixtures;
using Bit.Core.Vault.Services;
using Bit.Test.Common.AutoFixture;
using Bit.Test.Common.AutoFixture.Attributes;
using NSubstitute;
Expand All @@ -19,15 +20,18 @@ namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.Organizations;
public class OrganizationDeleteCommandTests
{
[Theory, PaidOrganizationCustomize, BitAutoData]
public async Task Delete_Success(Organization organization, SutProvider<OrganizationDeleteCommand> sutProvider)
public async Task Delete_Success(Organization organization,
SutProvider<OrganizationDeleteCommand> sutProvider)
{
var organizationRepository = sutProvider.GetDependency<IOrganizationRepository>();
var applicationCacheService = sutProvider.GetDependency<IApplicationCacheService>();
var cipherService = sutProvider.GetDependency<ICipherService>();

await sutProvider.Sut.DeleteAsync(organization);

await organizationRepository.Received().DeleteAsync(organization);
await applicationCacheService.Received().DeleteOrganizationAbilityAsync(organization.Id);
await cipherService.Received(1).DeleteAttachmentsForOrganizationAsync(organization.Id);
await organizationRepository.Received(1).DeleteAsync(organization);
await applicationCacheService.Received(1).DeleteOrganizationAbilityAsync(organization.Id);
}

[Theory, PaidOrganizationCustomize, BitAutoData]
Expand Down
86 changes: 86 additions & 0 deletions test/Core.Test/Vault/Services/CipherServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1802,6 +1802,12 @@ await sutProvider.GetDependency<ICipherRepository>()
.Received(1)
.DeleteByIdsOrganizationIdAsync(Arg.Is<IEnumerable<Guid>>(ids => ids.Count() == cipherIds.Count() &&
ids.All(id => cipherIds.Contains(id))), organizationId);
foreach (var cipher in ciphers)
{
await sutProvider.GetDependency<IAttachmentStorageService>()
.Received(1)
.DeleteAttachmentsForCipherAsync(cipher.Id);
}
await sutProvider.GetDependency<IEventService>()
.Received(1)
.LogCipherEventsAsync(Arg.Any<IEnumerable<Tuple<Cipher, EventType, DateTime?>>>());
Expand Down Expand Up @@ -1840,6 +1846,12 @@ await sutProvider.GetDependency<ICipherRepository>()
.Received(1)
.DeleteAsync(Arg.Is<IEnumerable<Guid>>(ids => ids.Count() == cipherIds.Count() &&
ids.All(id => cipherIds.Contains(id))), deletingUserId);
foreach (var cipher in ciphers)
{
await sutProvider.GetDependency<IAttachmentStorageService>()
.Received(1)
.DeleteAttachmentsForCipherAsync(cipher.Id);
}
await sutProvider.GetDependency<IEventService>()
.Received(1)
.LogCipherEventsAsync(Arg.Any<IEnumerable<Tuple<Cipher, EventType, DateTime?>>>());
Expand Down Expand Up @@ -1980,6 +1992,41 @@ await sutProvider.GetDependency<IPushNotificationService>()
.PushSyncCiphersAsync(deletingUserId);
}

[Theory]
[BitAutoData]
public async Task PurgeAsync_WithOrganizationId_DeletesCiphersAndAttachments(
Organization org, List<Cipher> ciphers, SutProvider<CipherService> sutProvider)
{
foreach (var cipher in ciphers)
{
cipher.OrganizationId = org.Id;
cipher.Attachments = JsonSerializer.Serialize(
new Dictionary<string, CipherAttachment.MetaData> { { "attachment1", new CipherAttachment.MetaData() } });
}

sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(org.Id)
.Returns(org);
sutProvider.GetDependency<ICipherRepository>()
.GetManyByOrganizationIdAsync(org.Id)
.Returns(ciphers);

await sutProvider.Sut.PurgeAsync(org.Id);

await sutProvider.GetDependency<ICipherRepository>()
.Received(1)
.DeleteByOrganizationIdAsync(org.Id);
foreach (var cipher in ciphers)
{
await sutProvider.GetDependency<IAttachmentStorageService>()
.Received(1)
.DeleteAttachmentsForCipherAsync(cipher.Id);
}
await sutProvider.GetDependency<IEventService>()
.Received(1)
.LogOrganizationEventAsync(org, EventType.Organization_PurgedVault);
}

[Theory]
[BitAutoData]
public async Task SoftDeleteAsync_WithPersonalCipherOwner_SoftDeletesCipher(
Expand Down Expand Up @@ -2724,4 +2771,43 @@ public async Task GetAttachmentDownloadDataAsync_ReturnsUrlFromStorageService(
Assert.Equal(expectedUrl, result.Url);
Assert.Equal(attachmentId, result.Id);
}

[Theory, BitAutoData]
public async Task DeleteAttachmentsForOrganizationAsync_OnlyDeletesAttachmentsForCiphersWithAttachments(
SutProvider<CipherService> sutProvider,
Guid organizationId,
List<Cipher> ciphersWithAttachments,
List<Cipher> ciphersWithoutAttachments)
{
foreach (var cipher in ciphersWithAttachments)
{
cipher.Attachments = JsonSerializer.Serialize(
new Dictionary<string, CipherAttachment.MetaData> { { "attachment1", new CipherAttachment.MetaData() } });
}

foreach (var cipher in ciphersWithoutAttachments)
{
cipher.Attachments = null;
}

sutProvider.GetDependency<ICipherRepository>()
.GetManyByOrganizationIdAsync(organizationId)
.Returns(ciphersWithAttachments.Concat(ciphersWithoutAttachments).ToList());

await sutProvider.Sut.DeleteAttachmentsForOrganizationAsync(organizationId);

foreach (var cipher in ciphersWithAttachments)
{
await sutProvider.GetDependency<IAttachmentStorageService>()
.Received(1)
.DeleteAttachmentsForCipherAsync(cipher.Id);
}

foreach (var cipher in ciphersWithoutAttachments)
{
await sutProvider.GetDependency<IAttachmentStorageService>()
.DidNotReceive()
.DeleteAttachmentsForCipherAsync(cipher.Id);
}
}
}
Loading