[PM-42319] - Add Inject Collection

[jrmccannon]

🎟️ Tracking

PM-42319

📔 Objective

Adding an InjectCollectionAttribute that will retrieve the collection and validate that the route in the url matches the organization. This is then used with the authorization logic to make sure they have access to it. Added a few other checks to validate orgid in route is used to validate against collections being retrieved/acted upon.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR closes a cross-tenant IDOR class in CollectionsController by adding an [InjectCollection] model binder that loads the collection and rejects it unless its OrganizationId matches the orgId/organizationId route value, plus hand-written equivalents on GetDetails, Put, and DeleteMany. I traced every action in the controller and all collection-scoped paths are now org-checked: Get, GetUsers, Delete, and PostDelete via the binder; GetDetails, Put, and DeleteMany via explicit checks; GetAll, GetManyWithDetails, and Post were already org-scoped, and PostBulkCollectionAccess had its check already. The new attribute mirrors the established InjectOrganizationUserAttribute/BindOrganizationAttribute pattern, and NotFoundException/BadRequestException thrown from the binder are mapped to 404/400 by the controller-level ExceptionHandlerFilterAttribute. I also confirmed the DeleteMany count check cannot be bypassed with duplicate or foreign IDs, since a foreign-org entry always drives the matched count below model.Ids.Count().

Code Review Details

• 🎨 : Put re-implements by hand exactly what [InjectCollection] does; the attribute would work here too
  • src/Api/AdminConsole/Controllers/CollectionsController.cs:198

[claude]

🎨 SUGGESTED: Put re-implements by hand exactly what [InjectCollection] does — consider using the attribute here too.

Details and rationale

Put does GetByIdAsync(id) followed by a null + OrganizationId != orgId check, which is byte-for-byte the contract of CollectionModelBinder. Using the attribute would look like:

[HttpPut("{id}")]
public async Task<CollectionResponseModel> Put(Guid orgId, [InjectCollection] Collection collection,
    [FromBody] UpdateCollectionRequestModel model)

and PostPut would forward collection the same way PostDelete already forwards to Delete. Mixing a custom-binder parameter with [FromBody] in one action is already proven in this codebase — see OrganizationUsersController.Put([BindOrganization] Organization organization, Guid id, [FromBody] ...) at src/Api/AdminConsole/Controllers/OrganizationUsersController.cs:437.

The value is bug prevention rather than fixing a defect: every endpoint that keeps a hand-written org-scoping check is another place the check can be dropped or copied incorrectly, which is the exact class of bug this PR is closing. GetDetails genuinely cannot use the binder (it needs CollectionAdminDetails from GetByIdWithPermissionsAsync), but Put can.

Non-blocking — the current code is correct, and moving the check into the binder would mean reworking Put_CollectionBelongsToDifferentOrg_ThrowsNotFound into a binder test, so it is a fair trade to decline.

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.34%. Comparing base (5855190) to head (a45f0d2).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
.../AdminConsole/Controllers/CollectionsController.cs	66.66%	2 Missing and 1 partial ⚠️
...minConsole/Attributes/InjectCollectionAttribute.cs	96.29%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8267      +/-   ##
==========================================
- Coverage   63.36%   63.34%   -0.02%     
==========================================
  Files        2430     2431       +1     
  Lines      104847   104950     +103     
  Branches     9505     9509       +4     
==========================================
+ Hits        66433    66484      +51     
- Misses      36137    36186      +49     
- Partials     2277     2280       +3     

☔ 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.

[r-tome]

Looks good, just a couple of minor tweaks needed

[r-tome]

⛏️ use the domain example.com for tests

[r-tome]

I think we usually we only write integration tests for happy path tests. These are already covered by the unit tests