From 6edab46d979f3bc28c733a8f9cb2bfbee0251cee Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Wed, 24 Sep 2025 12:52:04 -0500 Subject: [PATCH] [PM-24357] Do not purge ciphers in the default collection (#6320) * do not purge ciphers in the default collection * Update `DeleteByOrganizationId` procedure to be more performant based on PR review feedback * update EF integration for purge to match new SQL implementation * update Cipher_DeleteByOrganizationId based on PR feedback from dbops team --- .../Vault/Repositories/CipherRepository.cs | 23 +-- .../Cipher/Cipher_DeleteByOrganizationId.sql | 120 ++++++++----- .../Repositories/CipherRepositoryTests.cs | 160 ++++++++++++++++++ ...9-22_00_ExcludeDefaultCiphersFromPurge.sql | 91 ++++++++++ 4 files changed, 345 insertions(+), 49 deletions(-) create mode 100644 util/Migrator/DbScripts/2025-09-22_00_ExcludeDefaultCiphersFromPurge.sql diff --git a/src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs b/src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs index 4b2d09f87b..d88f0e98bb 100644 --- a/src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs +++ b/src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs @@ -281,17 +281,20 @@ public class CipherRepository : Repository + cc.Collection.Type == CollectionType.DefaultUserCollection) + select c; + dbContext.RemoveRange(ciphersToDelete); - var ciphers = from c in dbContext.Ciphers - where c.OrganizationId == organizationId - select c; - dbContext.RemoveRange(ciphers); + var collectionCiphersToRemove = from cc in dbContext.CollectionCiphers + join col in dbContext.Collections on cc.CollectionId equals col.Id + join c in dbContext.Ciphers on cc.CipherId equals c.Id + where col.Type != CollectionType.DefaultUserCollection + && c.OrganizationId == organizationId + select cc; + dbContext.RemoveRange(collectionCiphersToRemove); await OrganizationUpdateStorage(organizationId); await dbContext.UserBumpAccountRevisionDateByOrganizationIdAsync(organizationId); diff --git a/src/Sql/dbo/Vault/Stored Procedures/Cipher/Cipher_DeleteByOrganizationId.sql b/src/Sql/dbo/Vault/Stored Procedures/Cipher/Cipher_DeleteByOrganizationId.sql index d2324a1d00..c14a612b0f 100644 --- a/src/Sql/dbo/Vault/Stored Procedures/Cipher/Cipher_DeleteByOrganizationId.sql +++ b/src/Sql/dbo/Vault/Stored Procedures/Cipher/Cipher_DeleteByOrganizationId.sql @@ -1,49 +1,91 @@ CREATE PROCEDURE [dbo].[Cipher_DeleteByOrganizationId] - @OrganizationId AS UNIQUEIDENTIFIER -AS -BEGIN - SET NOCOUNT ON + @OrganizationId UNIQUEIDENTIFIER + AS + BEGIN + SET NOCOUNT ON; - DECLARE @BatchSize INT = 100 + DECLARE @BatchSize INT = 1000; - -- Delete collection ciphers - WHILE @BatchSize > 0 - BEGIN - BEGIN TRANSACTION Cipher_DeleteByOrganizationId_CC + BEGIN TRY + BEGIN TRANSACTION; - DELETE TOP(@BatchSize) CC - FROM - [dbo].[CollectionCipher] CC - INNER JOIN - [dbo].[Collection] C ON C.[Id] = CC.[CollectionId] - WHERE - C.[OrganizationId] = @OrganizationId + --------------------------------------------------------------------- + -- 1. Delete organization ciphers that are NOT in any default + -- user collection (Collection.Type = 1). + --------------------------------------------------------------------- + WHILE 1 = 1 + BEGIN + ;WITH Target AS + ( + SELECT TOP (@BatchSize) C.Id + FROM dbo.Cipher C + WHERE C.OrganizationId = @OrganizationId + AND NOT EXISTS ( + SELECT 1 + FROM dbo.CollectionCipher CC2 + INNER JOIN dbo.Collection Col2 + ON Col2.Id = CC2.CollectionId + AND Col2.Type = 1 -- Default user collection + WHERE CC2.CipherId = C.Id + ) + ORDER BY C.Id -- Deterministic ordering (matches clustered index) + ) + DELETE C + FROM dbo.Cipher C + INNER JOIN Target T ON T.Id = C.Id; - SET @BatchSize = @@ROWCOUNT + IF @@ROWCOUNT = 0 BREAK; + END - COMMIT TRANSACTION Cipher_DeleteByOrganizationId_CC - END + --------------------------------------------------------------------- + -- 2. Remove remaining CollectionCipher rows that reference + -- non-default (Type = 0 / shared) collections, for ciphers + -- that were preserved because they belong to at least one + -- default (Type = 1) collection. + --------------------------------------------------------------------- + SET @BatchSize = 1000; + WHILE 1 = 1 + BEGIN + ;WITH ToDelete AS + ( + SELECT TOP (@BatchSize) + CC.CipherId, + CC.CollectionId + FROM dbo.CollectionCipher CC + INNER JOIN dbo.Collection Col + ON Col.Id = CC.CollectionId + AND Col.Type = 0 -- Non-default collections + INNER JOIN dbo.Cipher C + ON C.Id = CC.CipherId + WHERE C.OrganizationId = @OrganizationId + ORDER BY CC.CollectionId, CC.CipherId -- Matches clustered index + ) + DELETE CC + FROM dbo.CollectionCipher CC + INNER JOIN ToDelete TD + ON CC.CipherId = TD.CipherId + AND CC.CollectionId = TD.CollectionId; - -- Reset batch size - SET @BatchSize = 100 + IF @@ROWCOUNT = 0 BREAK; + END - -- Delete ciphers - WHILE @BatchSize > 0 - BEGIN - BEGIN TRANSACTION Cipher_DeleteByOrganizationId + --------------------------------------------------------------------- + -- 3. Bump revision date (inside transaction for consistency) + --------------------------------------------------------------------- + EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationId] @OrganizationId; - DELETE TOP(@BatchSize) - FROM - [dbo].[Cipher] - WHERE - [OrganizationId] = @OrganizationId + COMMIT TRANSACTION ; - SET @BatchSize = @@ROWCOUNT - - COMMIT TRANSACTION Cipher_DeleteByOrganizationId - END - - -- Cleanup organization - EXEC [dbo].[Organization_UpdateStorage] @OrganizationId - EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationId] @OrganizationId -END \ No newline at end of file + --------------------------------------------------------------------- + -- 4. Update storage usage (outside the transaction to avoid + -- holding locks during long-running calculation) + --------------------------------------------------------------------- + EXEC [dbo].[Organization_UpdateStorage] @OrganizationId; + END TRY + BEGIN CATCH + IF @@TRANCOUNT > 0 + ROLLBACK TRANSACTION; + THROW; + END CATCH + END + GO diff --git a/test/Infrastructure.IntegrationTest/Vault/Repositories/CipherRepositoryTests.cs b/test/Infrastructure.IntegrationTest/Vault/Repositories/CipherRepositoryTests.cs index ef28d776d7..ee8cd0247d 100644 --- a/test/Infrastructure.IntegrationTest/Vault/Repositories/CipherRepositoryTests.cs +++ b/test/Infrastructure.IntegrationTest/Vault/Repositories/CipherRepositoryTests.cs @@ -1241,4 +1241,164 @@ public class CipherRepositoryTests Assert.NotNull(archivedCipher); Assert.NotNull(archivedCipher.ArchivedDate); } + + [DatabaseTheory, DatabaseData] + public async Task DeleteByOrganizationIdAsync_ExcludesDefaultCollectionCiphers( + IOrganizationRepository organizationRepository, + IUserRepository userRepository, + ICipherRepository cipherRepository, + ICollectionRepository collectionRepository, + ICollectionCipherRepository collectionCipherRepository) + { + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{Guid.NewGuid()}@email.com", + ApiKey = "TEST", + SecurityStamp = "stamp", + }); + + var organization = await organizationRepository.CreateAsync(new Organization + { + Name = "Test Organization", + BillingEmail = user.Email, + Plan = "Test" + }); + + var defaultCollection = await collectionRepository.CreateAsync(new Collection + { + Name = "Default Collection", + OrganizationId = organization.Id, + Type = CollectionType.DefaultUserCollection + }); + + var sharedCollection = await collectionRepository.CreateAsync(new Collection + { + Name = "Shared Collection", + OrganizationId = organization.Id, + }); + + async Task CreateOrgCipherAsync() => await cipherRepository.CreateAsync(new Cipher + { + Type = CipherType.Login, + OrganizationId = organization.Id, + Data = "" + }); + + var cipherInDefaultCollection = await CreateOrgCipherAsync(); + var cipherInSharedCollection = await CreateOrgCipherAsync(); + var cipherInBothCollections = await CreateOrgCipherAsync(); + var unassignedCipher = await CreateOrgCipherAsync(); + + await collectionCipherRepository.UpdateCollectionsForAdminAsync(cipherInDefaultCollection.Id, organization.Id, + new List { defaultCollection.Id }); + await collectionCipherRepository.UpdateCollectionsForAdminAsync(cipherInSharedCollection.Id, organization.Id, + new List { sharedCollection.Id }); + await collectionCipherRepository.UpdateCollectionsForAdminAsync(cipherInBothCollections.Id, organization.Id, + new List { defaultCollection.Id, sharedCollection.Id }); + + await cipherRepository.DeleteByOrganizationIdAsync(organization.Id); + + var remainingCipherInDefault = await cipherRepository.GetByIdAsync(cipherInDefaultCollection.Id); + var deletedCipherInShared = await cipherRepository.GetByIdAsync(cipherInSharedCollection.Id); + var remainingCipherInBoth = await cipherRepository.GetByIdAsync(cipherInBothCollections.Id); + var deletedUnassignedCipher = await cipherRepository.GetByIdAsync(unassignedCipher.Id); + + Assert.Null(deletedCipherInShared); + Assert.Null(deletedUnassignedCipher); + + Assert.NotNull(remainingCipherInDefault); + Assert.NotNull(remainingCipherInBoth); + + var remainingCollectionCiphers = await collectionCipherRepository.GetManyByOrganizationIdAsync(organization.Id); + + // Should still have the default collection cipher relationships + Assert.Contains(remainingCollectionCiphers, cc => + cc.CipherId == cipherInDefaultCollection.Id && cc.CollectionId == defaultCollection.Id); + Assert.Contains(remainingCollectionCiphers, cc => + cc.CipherId == cipherInBothCollections.Id && cc.CollectionId == defaultCollection.Id); + + // Should not have the shared collection cipher relationships + Assert.DoesNotContain(remainingCollectionCiphers, cc => cc.CollectionId == sharedCollection.Id); + } + + [DatabaseTheory, DatabaseData] + public async Task DeleteByOrganizationIdAsync_DeletesAllWhenNoDefaultCollections( + IOrganizationRepository organizationRepository, + IUserRepository userRepository, + ICipherRepository cipherRepository, + ICollectionRepository collectionRepository, + ICollectionCipherRepository collectionCipherRepository) + { + // Arrange + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{Guid.NewGuid()}@email.com", + ApiKey = "TEST", + SecurityStamp = "stamp", + }); + + var organization = await organizationRepository.CreateAsync(new Organization + { + Name = "Test Organization", + BillingEmail = user.Email, + Plan = "Test" + }); + + var sharedCollection1 = await collectionRepository.CreateAsync(new Collection + { + Name = "Shared Collection 1", + OrganizationId = organization.Id, + Type = CollectionType.SharedCollection + }); + + var sharedCollection2 = await collectionRepository.CreateAsync(new Collection + { + Name = "Shared Collection 2", + OrganizationId = organization.Id, + Type = CollectionType.SharedCollection + }); + + // Create ciphers + var cipherInSharedCollection1 = await cipherRepository.CreateAsync(new Cipher + { + Type = CipherType.Login, + OrganizationId = organization.Id, + Data = "" + }); + + var cipherInSharedCollection2 = await cipherRepository.CreateAsync(new Cipher + { + Type = CipherType.Login, + OrganizationId = organization.Id, + Data = "" + }); + + var unassignedCipher = await cipherRepository.CreateAsync(new Cipher + { + Type = CipherType.Login, + OrganizationId = organization.Id, + Data = "" + }); + + await collectionCipherRepository.UpdateCollectionsForAdminAsync(cipherInSharedCollection1.Id, organization.Id, + new List { sharedCollection1.Id }); + await collectionCipherRepository.UpdateCollectionsForAdminAsync(cipherInSharedCollection2.Id, organization.Id, + new List { sharedCollection2.Id }); + + await cipherRepository.DeleteByOrganizationIdAsync(organization.Id); + + var deletedCipher1 = await cipherRepository.GetByIdAsync(cipherInSharedCollection1.Id); + var deletedCipher2 = await cipherRepository.GetByIdAsync(cipherInSharedCollection2.Id); + var deletedUnassignedCipher = await cipherRepository.GetByIdAsync(unassignedCipher.Id); + + Assert.Null(deletedCipher1); + Assert.Null(deletedCipher2); + Assert.Null(deletedUnassignedCipher); + + // All collection cipher relationships should be removed + var remainingCollectionCiphers = await collectionCipherRepository.GetManyByOrganizationIdAsync(organization.Id); + Assert.Empty(remainingCollectionCiphers); + } } diff --git a/util/Migrator/DbScripts/2025-09-22_00_ExcludeDefaultCiphersFromPurge.sql b/util/Migrator/DbScripts/2025-09-22_00_ExcludeDefaultCiphersFromPurge.sql new file mode 100644 index 0000000000..8eb37a570f --- /dev/null +++ b/util/Migrator/DbScripts/2025-09-22_00_ExcludeDefaultCiphersFromPurge.sql @@ -0,0 +1,91 @@ +CREATE OR ALTER PROCEDURE [dbo].[Cipher_DeleteByOrganizationId] + @OrganizationId UNIQUEIDENTIFIER + AS + BEGIN + SET NOCOUNT ON; + + DECLARE @BatchSize INT = 1000; + + BEGIN TRY + BEGIN TRANSACTION; + + --------------------------------------------------------------------- + -- 1. Delete organization ciphers that are NOT in any default + -- user collection (Collection.Type = 1). + --------------------------------------------------------------------- + WHILE 1 = 1 + BEGIN + ;WITH Target AS + ( + SELECT TOP (@BatchSize) C.Id + FROM dbo.Cipher C + WHERE C.OrganizationId = @OrganizationId + AND NOT EXISTS ( + SELECT 1 + FROM dbo.CollectionCipher CC2 + INNER JOIN dbo.Collection Col2 + ON Col2.Id = CC2.CollectionId + AND Col2.Type = 1 -- Default user collection + WHERE CC2.CipherId = C.Id + ) + ORDER BY C.Id -- Deterministic ordering (matches clustered index) + ) + DELETE C + FROM dbo.Cipher C + INNER JOIN Target T ON T.Id = C.Id; + + IF @@ROWCOUNT = 0 BREAK; + END + + --------------------------------------------------------------------- + -- 2. Remove remaining CollectionCipher rows that reference + -- non-default (Type = 0 / shared) collections, for ciphers + -- that were preserved because they belong to at least one + -- default (Type = 1) collection. + --------------------------------------------------------------------- + SET @BatchSize = 1000; + WHILE 1 = 1 + BEGIN + ;WITH ToDelete AS + ( + SELECT TOP (@BatchSize) + CC.CipherId, + CC.CollectionId + FROM dbo.CollectionCipher CC + INNER JOIN dbo.Collection Col + ON Col.Id = CC.CollectionId + AND Col.Type = 0 -- Non-default collections + INNER JOIN dbo.Cipher C + ON C.Id = CC.CipherId + WHERE C.OrganizationId = @OrganizationId + ORDER BY CC.CollectionId, CC.CipherId -- Matches clustered index + ) + DELETE CC + FROM dbo.CollectionCipher CC + INNER JOIN ToDelete TD + ON CC.CipherId = TD.CipherId + AND CC.CollectionId = TD.CollectionId; + + IF @@ROWCOUNT = 0 BREAK; + END + + --------------------------------------------------------------------- + -- 3. Bump revision date (inside transaction for consistency) + --------------------------------------------------------------------- + EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationId] @OrganizationId; + + COMMIT TRANSACTION ; + + --------------------------------------------------------------------- + -- 4. Update storage usage (outside the transaction to avoid + -- holding locks during long-running calculation) + --------------------------------------------------------------------- + EXEC [dbo].[Organization_UpdateStorage] @OrganizationId; + END TRY + BEGIN CATCH + IF @@TRANCOUNT > 0 + ROLLBACK TRANSACTION; + THROW; + END CATCH + END + GO