[PM-12473] refactor: add CollectionAuthorizationService and wire it into the Update endpoint behind a flag

[r-tome]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-12473

📔 Objective

Our first goal was different. We wanted to split BulkCollectionAuthorizationHandler into smaller pieces. One piece would check CollectionUser access. Another piece would check CollectionGroup access.

We built these as two new classes: CollectionUserAuthorizationHandler and CollectionGroupAuthorizationHandler. Both use ASP.NET's IAuthorizationHandler framework.

This design became too complex. The controller had to fetch all the data first. Then it had to pass pieces of that data to each handler. A reviewer flagged this problem on PR #8075.

So we built a new pattern instead. We call it [Resource]AuthorizationService. Each service fetches its own data. Then it calls a shared static Rules class. This class is the single source of truth for the decision.

This PR adds the first service in this new pattern: ICollectionAuthorizationService. It calls a shared class named CollectionRules. CollectionRules has three methods: CanUpdate, CanModifyUserAccess, and CanModifyGroupAccess.

This PR does not change the Authorize<T> role checks.

This PR also fixes a bug. The CanModifyGroupAccess method checked Permissions.ManageUsers. It should check Permissions.ManageGroups.

PUT organizations/{orgId}/collections/{id} now calls AuthorizeUpdateAsync. This sits behind a new feature flag, pm-35160-authorization-services. The old IAuthorizationHandler check stays as the fallback when the flag is off.

AuthorizeModifyUserAccessAsync and AuthorizeModifyGroupAccessAsync have no caller yet. The plan is to wire them into the endpoints that still call BulkCollectionAuthorizationHandler for ModifyUserAccess/ModifyGroupAccess today, in CollectionsController, GroupsController, and OrganizationUsersController. That's follow-up work in a later PR.

📸 Screenshots

N/A

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.25%. Comparing base (6fbf0f2) to head (6392403).

Files with missing lines	Patch %	Lines
...nsole/Authorization/Collections/CollectionRules.cs	88.88%	4 Missing and 2 partials ⚠️
...tion/Collections/CollectionAuthorizationService.cs	93.93%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8211      +/-   ##
==========================================
+ Coverage   63.22%   63.25%   +0.02%     
==========================================
  Files        2381     2383       +2     
  Lines      103757   103845      +88     
  Branches     9385     9409      +24     
==========================================
+ Hits        65604    65684      +80     
- Misses      35924    35928       +4     
- Partials     2229     2233       +4     

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

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed after commit 5d0b0fc2b, which adds the CanModifyUserAccess / CanModifyGroupAccess rules and wires AuthorizeUpdateAsync into PUT /organizations/{orgId}/collections/{id} behind pm-35160-authorization-services. All three CollectionRules methods were re-compared branch-by-branch against BulkCollectionAuthorizationHandler.CanUpdateCollectionAsync / CanUpdateUserAccessAsync / CanUpdateGroupAccessAsync and remain semantically equivalent — the reordered ManageUsers / ManageGroups bypasses are pure OR branches, and the organization is { Permissions.X: true } pattern is null-safe where the old org?.Permissions.X == true was not. The flag-on Put path preserves parity with the old BulkCollectionOperations.Update check (a null or cross-organization collection still yields NotFoundException, and the new collection.OrganizationId != organizationId guard is stricter than the flag-off path), and DefaultUserCollection rejection is still enforced downstream in UpdateCollectionCommand. No new findings this round.

Code Review Details

No new findings.

Previously raised and still open:

• ♻️ : The shared rule tail (callerManagesCollection, Owner/Admin gate, isOrphaned) is now duplicated across all three rule methods rather than composed
  • src/Api/AdminConsole/Authorization/Collections/CollectionRules.cs:70
  • The second half of that thread ("the service's three-way && is equivalent to CanUpdate alone") no longer applies — the service now exposes three separate methods.
• ♻️ : CollectionRules.CanUpdate has no direct rule-level tests
  • test/Api.Test/AdminConsole/Authorization/CollectionRulesTests.cs:10
  • Partially narrowed by the new AuthorizeUpdateAsync_* service tests, which now cover the callerManagesCollection and orphaned-collection branches. The remaining uncovered branch is allowAdminAccessToAllCollectionItems: true with Owner/Admin at CollectionRules.cs:26 — the only CanUpdate grant path now reachable in production behind the flag.

Resolved by earlier commits: the unconditional GetManyByUserIdAsync query is now deferred behind the organization-level bypasses (6392403).

PR Metadata Assessment

• ❓ QUESTION: The description states "This PR adds the service only. No controller uses it yet. PR [PM-12473] Add delta-shaped PATCH endpoint for collection access #8212 adds the endpoint that uses it," but the diff wires CollectionsController.Put to AuthorizeUpdateAsync behind FeatureFlagKeys.AuthorizationServices. Worth updating so reviewers assess the correct production blast radius.

[claude]

♻️ DEBT: The shared tail is copied into all three rules, which also makes the service's three-way && equivalent to CanUpdate alone.

Details and fix

The callerManagesCollection check, the Owner/Admin gate, and the isOrphaned computation appear verbatim in CanUpdate, CanModifyUserAccess, and CanModifyGroupAccess. Any future change to "orphaned" semantics or to the Owner/Admin fallback has to land in three places to stay consistent.

BulkCollectionAuthorizationHandler composed these instead — CanUpdateUserAccessAsync was bypass || CanUpdateCollectionAsync. The same shape works here:

public static bool CanModifyUserAccess(
    CollectionAccessDetails accessDetails,
    CurrentContextOrganization? organization,
    bool allowAdminAccessToAllCollectionItems,
    bool callerManagesCollection)
{
    if (allowAdminAccessToAllCollectionItems && organization is { Permissions.ManageUsers: true })
    {
        return true;
    }

    return CanUpdate(accessDetails, organization, allowAdminAccessToAllCollectionItems, callerManagesCollection);
}

Related consequence at CollectionAuthorizationService.cs:36: every branch that makes CanUpdate return true is also a branch of the other two rules, so canUpdate && canModifyUserAccess && canModifyGroupAccess can never differ from canUpdate. That's fine if the AND is deliberate future-proofing, but as written it reads as though the ManageUsers/ManageGroups bypasses are reachable through this service when they never are.

[claude]

🎨 SUGGESTED: This issues an extra query on every authorization call, even when the org-level branches already decide the outcome.

Details and fix

CallerManagesCollectionAsync runs Collection_ReadByUserId, which returns every collection the caller can access across all organizations, before any rule is evaluated. For the common admin paths — EditAnyCollection, or Owner/Admin with AllowAdminAccessToAllCollectionItems — the value is never consulted, so this is a second round trip per request with no effect on the result.

BulkCollectionAuthorizationHandler.CanManageCollectionsAsync deferred that query until the earlier short-circuits had failed. Deferring it here too (for example, evaluating the organization-level branches first and only resolving callerManagesCollection when they don't already grant access) keeps the hot path at one query.

Note that GetByIdWithPermissionsAsync looks like a one-query alternative but is not a drop-in: Collection_ReadByIdWithPermissions does not filter on OrganizationUser.Status = Confirmed or Organization.Enabled, whereas UserCollectionDetails does.

[claude]

♻️ DEBT: CollectionRules.CanUpdate has no direct tests, leaving its two admin grant paths uncovered everywhere.

Details and fix

CanUpdate is the rule that effectively decides AuthorizeUpdateAsync (the other two rules are supersets of it), but this file only covers CanModifyUserAccess and CanModifyGroupAccess — there is no CanUpdate_* test.

CollectionAuthorizationServiceTests doesn't fill the gap: GetOrganizationAbilityAsync is never stubbed, so organizationAbility is null and allowAdminAccessToAllCollectionItems is false in every service test. That leaves two grant branches with no coverage in either suite:

• allowAdminAccessToAllCollectionItems: true with Owner/Admin (CollectionRules.cs:26)
• orphaned collection with Owner/Admin while allowAdminAccessToAllCollectionItems: false (CollectionRules.cs:42)

Mirroring the existing CanModifyUserAccess theories for CanUpdate covers both, and would also close the codecov gap reported on this PR. Adding one service-level test with an OrganizationAbility { AllowAdminAccessToAllCollectionItems = true } would additionally verify the ability lookup is wired to the right flag.