[PM-41472] feat: add bulk folder delete endpoint by gbubemismith · Pull Request #8157 · bitwarden/server

gbubemismith · 2026-08-06T17:52:23Z

🎟️ Tracking

📔 Objective

VFO1 introduces a new My Folders page in the web client, where users can multiselect folders and delete them in bulk. There is no bulk folder delete anywhere in the stack today, so the client has to issue one DELETE /folders/{id} per selected folder — N round trips for a single user action, and non-atomic.

This adds DELETE /folders, which takes a list of folder ids and deletes them in one request.

Add DELETE /folders for deleting multiple personal folders in one request, with a Folder_DeleteByIds stored procedure and matching EF implementation. Also fixes EF single-folder delete, which left ciphers pointing at the deleted folder and never bumped the account revision date.

mkincaid-bw · 2026-08-06T22:20:22Z

+            [UserId] = @UserId
+            AND [Status] = 2 -- Confirmed
+    )
+    UPDATE


Since there's no explicit transaction here and none in the C# caller, are there any concerns if these 3 data modification statements do not all complete atomically?

Nice catch. It wasn't atomic as written, although it was fail safe
Fixed 55067ab

github-actions · 2026-08-07T15:32:43Z

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed the full branch after d1c985c7f, which adds [RequireFeature(FeatureFlagKeys.VFO1Foundation)] to DELETE /folders. The flag key exists in Constants.cs and app.UseFeatureFlagChecks() is registered in src/Api/Startup.cs, so the gate is effective. Verified authorization (both the command and each repository re-filter by UserId, so unowned ids cannot be deleted), MSSQL/EF parity, migration-to-SSDT sproc equivalence, transaction handling in the Dapper path, and DI registration; the earlier findings about EF query breadth and missing integration-test assertions are addressed at HEAD.

Code Review Details

No new findings this pass.

Notes verified rather than flagged:

FolderBulkDeleteRequestModel.Ids null/empty bodies return 400 — PublicApiControllersModelConvention attaches ModelStateValidationFilterAttribute to every Api controller, so no unguarded model.Ids.Count() dereference.
EF Folders.Contains(userId.ToString()) matches the existing pattern in Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs, and the EXISTS-over-TVP rewrite in the sproc preserves the semantics of Folder_DeleteById.
DeleteAsync override in the EF repository brings single-folder delete to parity with the MSSQL sproc (ciphers are now unfiled), which is a behavior fix rather than a regression.
Migration 2026-08-06_01_AddFolderDeleteByIds.sql is byte-identical to the SSDT sproc apart from CREATE OR ALTER / GO, and sorts after all existing scripts.

claude · 2026-08-07T15:37:58Z

+        var userCipherDetails = new UserCipherDetailsQuery(userId).Run(dbContext);
+        var filedCiphers = from ucd in userCipherDetails
+                           join c in dbContext.Ciphers.Where(c => c.Folders != null)
+                               on ucd.Id equals c.Id
+                           select c;
+
+        await filedCiphers.ForEachAsync(cipher =>


⚠️ IMPORTANT: This materializes and tracks every cipher the user can access, not just the ones filed under the deleted folders.

Details and fix

UserCipherDetailsQuery returns the user's personal ciphers plus every org cipher they can reach through a collection. The only server-side narrowing here is c.Folders != null, which is true for any cipher that any member has filed. So for a user in an org with 50k shared items, deleting a single folder streams and change-tracks ~50k Cipher rows (including the Data blob) into the DbContext. ForEachAsync streams, but tracked entities accumulate for the lifetime of the context, so peak memory scales with the accessible vault, not with the folders being deleted.

This also now applies to the pre-existing single-folder path, since DeleteAsync was overridden to delegate here.

A server-side filter on the user's key in the Folders map narrows this to only ciphers this user has filed, and translates to a LIKE on all three EF providers:

var userKey = userId.ToString(); var filedCiphers = from ucd in userCipherDetails join c in dbContext.Ciphers.Where(c => c.Folders != null && c.Folders.Contains(userKey)) on ucd.Id equals c.Id select c;

The same Folders.Contains(userId.ToString()) guard is already used in CipherRepository (src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs:617), so the JSON key format is consistent.

Addressed in 58fbb34

claude · 2026-08-07T15:37:59Z

+
+        await folderRepository.DeleteManyAsync([ownFolder.Id, otherUsersFolder.Id], user.Id);
+
+        Assert.Null(await folderRepository.GetByIdAsync(ownFolder.Id, user.Id));


⚠️ IMPORTANT: This test never asserts the property it is named for — the other user's folder is never checked.

Details and fix

otherUsersFolder and the other user's cipher are created and passed into DeleteManyAsync, but the only assertion is that ownFolder was deleted. The test passes today and would keep passing if the AND [UserId] = @UserId filter in Folder_DeleteByIds (or the f.UserId == userId predicate in the EF path) were dropped — which is exactly the cross-user data-deletion regression this test exists to catch.

Assert.Null(await folderRepository.GetByIdAsync(ownFolder.Id, user.Id)); Assert.NotNull(await folderRepository.GetByIdAsync(otherUsersFolder.Id, otherUser.Id));

Asserting the other user's cipher is still filed under otherUsersFolder.Id would also cover the JSON_MODIFY scoping.

claude · 2026-08-07T15:38:00Z

+
+        await folderRepository.DeleteManyAsync([deletedFolder.Id], user.Id);
+
+        Assert.Null(await folderRepository.GetByIdAsync(deletedFolder.Id, user.Id));


♻️ DEBT: Tests named for cipher unfiling create ciphers but never assert on them.

Details and fix

DeleteManyAsync_DeletesRequestedFolders_AndUnfilesTheirCiphers creates cipherInDeletedFolder, cipherInKeptFolder, unfiledCipher, and keptFolder, then asserts only that deletedFolder is gone. DeleteAsync_UnfilesTheCiphersInTheDeletedFolder (line 107) has the same shape.

The unfiling behavior is the newly added part of the EF path, and it is only covered by DeleteManyAsync_DeletesEveryRequestedFolder. Adding the assertions these tests already have the fixtures for closes the gap:

Assert.NotNull(await folderRepository.GetByIdAsync(keptFolder.Id, user.Id)); Assert.Null((await cipherRepository.GetByIdAsync(cipherInDeletedFolder.Id, user.Id)).FolderId); Assert.Equal(keptFolder.Id, (await cipherRepository.GetByIdAsync(cipherInKeptFolder.Id, user.Id)).FolderId);

codecov · 2026-08-07T15:41:11Z

Codecov Report

❌ Patch coverage is 86.02151% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.48%. Comparing base (16a41c2) to head (d1c985c).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
...tyFramework/Vault/Repositories/FolderRepository.cs	84.78%	5 Missing and 2 partials ⚠️
...ture.Dapper/Vault/Repositories/FolderRepository.cs	76.47%	4 Missing ⚠️
...rc/Core/Vault/Commands/DeleteManyFoldersCommand.cs	87.50%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8157      +/-   ##
==========================================
+ Coverage   67.79%   68.48%   +0.68%     
==========================================
  Files        2337     2360      +23     
  Lines      101456   102411     +955     
  Branches     9167     9248      +81     
==========================================
+ Hits        68787    70134    +1347     
+ Misses      30362    29957     -405     
- Partials     2307     2320      +13

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mkincaid-bw

LGTM

…ing in tests

nick-livefront

Only a single question but I'm assuming that no changes will come of it and I will learn something 😄

nick-livefront · 2026-08-10T15:11:22Z

+        // Deleting folders also re-assigns the ciphers filed under them, so clients need a full vault sync
+        // rather than a per-folder delete notification.


👏 Great comment

nick-livefront · 2026-08-10T15:13:43Z

+    [HttpDelete("")]
+    public async Task DeleteMany([FromBody] FolderBulkDeleteRequestModel model)
+    {
+        if (!_globalSettings.SelfHosted && model.Ids.Count() > 500)


❓ Why is selfhosted accounted for here?

We generally do this cause self-hosted is a single tenant the user owns and large operations will only affect them. For cloud instances we will want more control and a user submitting 10k+ will degrade for everyone

💯 Makes sense!

nick-livefront · 2026-08-10T15:16:43Z

+        var userCipherDetails = new UserCipherDetailsQuery(userId).Run(dbContext);
+        var filedCiphers = from ucd in userCipherDetails
+                           join c in dbContext.Ciphers.Where(c => c.Folders != null)
+                               on ucd.Id equals c.Id
+                           select c;
+
+        await filedCiphers.ForEachAsync(cipher =>


Addressed in 58fbb34

gbubemismith added ai-review-vnext Request a Claude code review using the vNext workflow t:feature Change Type - Feature Development labels Aug 6, 2026

Merge branch 'main' into vault/pm-41472/add-bulk-folder-delete-endpoint

35212a9

gbubemismith marked this pull request as ready for review August 6, 2026 18:43

gbubemismith requested review from a team as code owners August 6, 2026 18:43

gbubemismith requested a review from jaasen-livefront August 6, 2026 18:43

gbubemismith mentioned this pull request Aug 6, 2026

[PM-41472] feat(web): add the My folders page bitwarden/clients#22267

Draft

Merge branch 'main' into vault/pm-41472/add-bulk-folder-delete-endpoint

3ee0524

mkincaid-bw reviewed Aug 6, 2026

View reviewed changes

Merge branch 'main' into vault/pm-41472/add-bulk-folder-delete-endpoint

01c049f

claude Bot reviewed Aug 7, 2026

View reviewed changes

Wrap Folder_DeleteByIds in a transaction at the repository level

55067ab

gbubemismith requested a review from mkincaid-bw August 7, 2026 17:52

mkincaid-bw previously approved these changes Aug 7, 2026

View reviewed changes

Rename folder delete migration to resolve sequence collision

45a373d

gbubemismith dismissed mkincaid-bw’s stale review via 45a373d August 7, 2026 18:12

gbubemismith and others added 2 commits August 7, 2026 15:47

Narrow EF cipher scan, correct GetByIdAsync nullability, assert unfil…

26fcd5f

…ing in tests

Merge branch 'main' into vault/pm-41472/add-bulk-folder-delete-endpoint

58fbb34

nick-livefront previously approved these changes Aug 10, 2026

View reviewed changes

Feature gatted endpoint

d1c985c

gbubemismith dismissed nick-livefront’s stale review via d1c985c August 11, 2026 14:29


		await folderRepository.DeleteManyAsync([ownFolder.Id, otherUsersFolder.Id], user.Id);

		Assert.Null(await folderRepository.GetByIdAsync(ownFolder.Id, user.Id));

		// Deleting folders also re-assigns the ciphers filed under them, so clients need a full vault sync
		// rather than a per-folder delete notification.

Uh oh!

Conversation

gbubemismith commented Aug 6, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Bitwarden Claude Code Review

Uh oh!

claude Bot Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mkincaid-bw left a comment

Choose a reason for hiding this comment

Uh oh!

nick-livefront left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gbubemismith commented Aug 6, 2026 •

edited by atlassian Bot

Loading

github-actions Bot commented Aug 7, 2026 •

edited

Loading

codecov Bot commented Aug 7, 2026 •

edited

Loading