[PM-41270] refactor: add GroupsAuthorizationService and wire it into GroupsController - #8270
Conversation
…roller GroupsController authorized collection access with the old handler in three places: a bulk check in Post, and in Put a per-collection loop over the posted collections plus a second loop over the group's current collections. Put also held the inline check that a caller cannot add themselves to a group. Behind pm-35160-authorization-services all four move into GroupsAuthorizationService. The service calls ICollectionAuthorizationService.AuthorizeModifyGroupAccessManyAsync once for the posted collections and once for the current ones, so both loops become two batched calls. It does not call CollectionRules itself: callerManagesCollection and isOrphaned belong to the collection, not to the group. One deliberate behavior change on the flag-on path, per the ticket. A posted collection id that does not exist, or that belongs to another organization, was dropped by GetManyByManyIdsAsync, never authorized, and then saved anyway. It now returns a 404. Post gains the same check. Two smaller differences, both safer than the old path. A null OrganizationAbility threw a NullReferenceException and now evaluates the rule instead. A null posted user list threw and is now treated as empty. Creating a group keeps its current behavior of allowing the caller to add themselves. A new group can only reach collections the caller was already authorized for, so joining it grants no access they did not have.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Code Review DetailsNo findings. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ac/pm-42605/collection-bulk-access-authorization #8270 +/- ##
=====================================================================================
+ Coverage 0 63.39% +63.39%
=====================================================================================
Files 0 2434 +2434
Lines 0 105110 +105110
Branches 0 9541 +9541
=====================================================================================
+ Hits 0 66630 +66630
- Misses 0 36199 +36199
- Partials 0 2281 +2281 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// Determines if the caller can save the group's collection access and its members. | ||
| /// </summary> | ||
| /// <param name="organizationId">The ID of the organization that owns the group.</param> | ||
| /// <param name="groupId">The ID of the group to update, or null when the group is being created.</param> |
There was a problem hiding this comment.
The nullable parameter is doing too much here: a caller could easily leave it out or pass a nullable Guid when they actually intend to update an existing collection, and then it bypasses permission checks.
I recommend separate interfaces: AuthorizeUpdateAsync and AuthorizeCreateAsync.
| // Creating a group has never had this rule. It is not watertight on update either: a caller can create an | ||
| // empty group with themselves in it, then add collections through update as an existing member. | ||
| if (groupId is null) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Example of where separate create vs. update paths would work better. The create path just never needs this logic.
| return true; | ||
| } | ||
|
|
||
| // A provider is not an organization member, so it has no organization user to add. |
There was a problem hiding this comment.
Not true. A provider can still be an organization member, although to be honest I'm not sure if/how this rule applies to providers. Please double check.
| Task<GroupsAuthorizationResult> AuthorizeSaveAsync( | ||
| Guid organizationId, | ||
| Guid? groupId, | ||
| IReadOnlyCollection<Guid> postedCollectionIds, | ||
| IReadOnlyCollection<Guid> currentCollectionIds, | ||
| IReadOnlyCollection<Guid> postedUserIds); |
There was a problem hiding this comment.
OK, so this is the hard part. We want to make this a good interface while still fitting the needs of the slightly weird controller logic/tech debt. But I also agree that fixing all that controller stuff is out of scope here - let's just do what we can.
I suggest:
- inputs: remove
currentCollectionIds- this is data the authz service should fetch for itself. - outputs: make the return object more straightforwardly useful. It could return:
- if authz fails:
Error authorizationError- a strongly typed error, either "cannot add yourself to a group" or a general NotFoundError. Covers all failure states, controller can just throw this if present. - if authz succeeds: the fully reconciled list of
collectionIdsto save. (That is: posted collectionIds + unauthorized collectionIds.) Figuring this out is basically an authz decision, so let's handle it within this service. Then the controller can pass this into the command.
- if authz fails:
You could use a OneOf for this, or just a simple record with the 2 fields that you check in order.
| Guid organizationId, | ||
| Guid? groupId, |
There was a problem hiding this comment.
We need to make sure that the group belongs to the organization. To help with this, I also think it would be OK to pass in the whole Group object.
There was a problem hiding this comment.
This should also do the role check, i.e. you should have a role that lets you edit groups.
This is duplicative with the Authorize attribute and the collectionAuthzService logic, which also check the role. But each layer should stand on its own and fully encapsulate its authz logic, and checking claims is essentially free, so we may as well.
| // The client only sends collections that the saving user has permissions to edit. | ||
| // We need to combine these with collections that the user doesn't have permissions for, so that we don't | ||
| // accidentally overwrite those | ||
| var editedCollectionAccess = model.Collections | ||
| .Select(c => c.ToSelectionReadOnly()); | ||
| var readonlyCollectionAccess = currentAccess | ||
| .Where(ca => readonlyCollectionIds.Contains(ca.Id)); | ||
| var collectionsToSave = editedCollectionAccess | ||
| .Concat(readonlyCollectionAccess) | ||
| .ToList(); |
There was a problem hiding this comment.
Further to my previous comment: this would all shift inside the authz service - at least for the new code path.
There was a problem hiding this comment.
I think integration tests are more useful for authorization, as it depends heavily on the state of the system. It's easy to mock the wrong thing and have your tests give you false negatives. It'll also be the most resilient to our refactoring.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41270
Third in the stack, after #8211 (PM-12473) and #8268 (PM-42605).
📔 Objective
GroupsControllerauthorized collection access withBulkCollectionAuthorizationHandlerin three places: a bulk check in
Post, and inPuta per-collection loop overthe posted collections plus a second loop over the group's current collections.
Putalso held the inline check that a caller cannot add themselves to a group.Behind
pm-35160-authorization-servicesall four move intoGroupsAuthorizationService. It callsICollectionAuthorizationService.AuthorizeModifyGroupAccessManyAsynconce for theposted collections and once for the current ones, so both loops become two batched
calls. It does not call
CollectionRulesitself —callerManagesCollectionandisOrphanedbelong to the collection, not to the group.Putis restructured so both paths share one tail: each returns the set of currentcollections the caller cannot change, and
collectionsToSaveis built once. Theflag-off branch is the old body moved unchanged.
Behavior changes on the flag-on path
to be dropped by
GetManyByManyIdsAsync, never authorized, and then written to thegroup anyway. This is the tightening PM-41270 asks for.
Postgains the same check.The web client filters read-only selections out before submitting, so it cannot trip
this.
orgIdis now authoritative. The old handler took its targetorganization from
resources.First().OrganizationId, so a caller who managed acollection in another organization could have it written onto a group in this one.
OrganizationAbilitythrew aNullReferenceExceptionand now takes therestrictive branch. A null posted user list threw and is now treated as empty.
Creating a group keeps its current behavior of letting the caller add themselves.
Posthas never had that rule, and it is not watertight onPuteither — a callercan create an empty group with themselves in it, then add collections as an existing
member. Unchanged here, and worth its own ticket.
Tests
Puthad no tests at all. Four flag-off characterization tests pin the existingbehavior first, including the self-add
BadRequestExceptionand thepreserve-unauthorized-access path. Nine service tests and five flag-on controller
tests follow.
📸 Screenshots
N/A