[PM-41270] feat: add CollectionGroup authorization and access command - #8132
[PM-41270] feat: add CollectionGroup authorization and access command#8132r-tome wants to merge 4 commits into
Conversation
…across multiple collections Adds ModifyGroupAccessAsync to ICollectionRepository with Dapper (SQL Server) and EF Core (Postgres/MySQL/SQLite) implementations. A new CollectionGroup_DeleteMany sproc ensures removes and upserts are applied atomically in one transaction.
…p access Adds CollectionGroupAuthorizationHandler, CollectionGroupAuthorizationRules, CollectionGroupOperations, and CollectionGroupAccessResource — a direct mirror of the CollectionUser authorization layer. No self-add check since groups don't have that concept.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ac/pm-12473/collection-user-access-endpoint #8132 +/- ##
===============================================================================
- Coverage 67.44% 66.92% -0.52%
===============================================================================
Files 2317 2309 -8
Lines 100516 100539 +23
Branches 9050 9087 +37
===============================================================================
- Hits 67790 67284 -506
- Misses 30453 30976 +523
- Partials 2273 2279 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the CollectionGroup groundwork: the new Code Review Details
Also noted, no action strictly required: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return true; | ||
| } | ||
|
|
||
| if (allowAdminAccessToAllCollectionItems && organization is { Permissions.ManageUsers: true }) |
There was a problem hiding this comment.
❌ CRITICAL: Group-access rule checks Permissions.ManageUsers instead of Permissions.ManageGroups
Details and fix
This looks like a copy-paste carry-over from CollectionUserAuthorizationRules.CanModifyUserAccess (the two classes are otherwise byte-identical). The established rule for modifying a group's collection access is ManageGroups — see BulkCollectionAuthorizationHandler:
private async Task<bool> CanUpdateGroupAccessAsync(...)
{
if (await AllowAdminAccessToAllCollectionItems(org) && org?.Permissions.ManageGroups == true)As written, once PM-41448 wires up the endpoint:
- a Custom user with
ManageUsersbut notManageGroupsgains the ability to change group access (privilege escalation relative to the existingModifyGroupAccessrule); - a Custom user with
ManageGroupsloses it (regression).
Suggested change:
if (allowAdminAccessToAllCollectionItems && organization is { Permissions.ManageGroups: true })CollectionGroupAuthorizationRulesTests.CanModifyGroupAccess_WithManageUsersPermission_AllowAdminAccessTrue_Success and ..._CustomUserWithManageUsersPermission_AllowAdminAccessFalse_Failure were also copied over and will need renaming/retargeting to ManageGroups.
Related: CollectionGroupAuthorizationHandler, CollectionGroupAccessResource, and CollectionGroupAuthorizationRules are exact duplicates of their CollectionUser* counterparts apart from type names. Worth considering a shared generic base (e.g. parameterised on the permission to check) so a single authorization rule change can't silently diverge between the two paths again.
| var collections = await dbContext.Collections | ||
| .Where(c => collectionIds.Contains(c.Id)) | ||
| .ToListAsync(); | ||
| foreach (var collection in collections) | ||
| { | ||
| collection.RevisionDate = revisionDate; | ||
| } |
There was a problem hiding this comment.
RevisionDate for upserted groups, diverging from MSSQL
Details and fix
Collection_CreateOrUpdateAccessForMany (used by the Dapper path) bumps the revision date of every group in @Groups:
-- Bump the revision date on all affected groups
UPDATE G SET G.[RevisionDate] = @RevisionDate
FROM [dbo].[Group] G
INNER JOIN @Groups GR ON G.[Id] = GR.[Id]
WHERE G.[OrganizationId] = @OrganizationIdThe EF CreateOrUpdateAccessForManyAsync mirrors this (lines 945-957). ModifyGroupAccessAsync bumps only the removed groups, so on PostgreSQL/MySQL/SQLite an add/update of group access leaves Group.RevisionDate stale while SQL Server updates it.
Suggested addition alongside the collection bump:
if (upserts.Any())
{
var upsertIds = upserts.Select(u => u.Id).ToList();
var affectedGroups = await dbContext.Groups
.Where(g => g.OrganizationId == organizationId && upsertIds.Contains(g.Id))
.ToListAsync();
foreach (var g in affectedGroups)
{
g.RevisionDate = revisionDate;
}
}
| UPDATE | ||
| [dbo].[Group] | ||
| SET | ||
| [RevisionDate] = GETUTCDATE() | ||
| WHERE | ||
| [Id] IN (SELECT [Id] FROM @GroupIds) | ||
| END |
There was a problem hiding this comment.
AccountRevisionDate bumped, so their clients won't re-sync
Details and fix
The user-side counterpart, CollectionUser_DeleteMany, ends with:
EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationUserIds] @OrganizationUserIdsThis proc only bumps [Group].[RevisionDate]. The User_BumpAccountRevisionDateByCollectionIds call at the end of Collection_CreateOrUpdateAccessForMany (invoked afterwards in ModifyGroupAccessAsync) joins through CollectionUser/CollectionGroup, so users whose only access path was the just-deleted CollectionGroup row are no longer matched and are not bumped.
Net effect: a member who loses collection access through a group removal gets no sync signal, and their client keeps the collection and its ciphers cached locally until an unrelated change triggers a sync. Group_DeleteById avoids this by bumping the whole org before deleting.
Suggested addition before the DELETE (there is no User_BumpAccountRevisionDateByGroupIds proc, so resolve the org users first):
DECLARE @AffectedOrganizationUserIds [dbo].[GuidIdArray]
INSERT INTO @AffectedOrganizationUserIds ([Id])
SELECT DISTINCT GU.[OrganizationUserId]
FROM [dbo].[GroupUser] GU
WHERE GU.[GroupId] IN (SELECT [Id] FROM @GroupIds)
EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationUserIds] @AffectedOrganizationUserIdsThe same gap exists in the EF ModifyGroupAccessAsync removal branch, and in util/Migrator/DbScripts/2026-08-04_00_AddCollectionGroupDeleteMany.sql.
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41270
📔 Objective
Adds the CollectionGroup groundwork that mirrors PM-12473 for groups. No endpoint is added here — the unified collection PATCH endpoint lands in PM-41448.
Includes
CollectionGroup_DeleteManystored procedure,ModifyGroupAccessAsynconICollectionRepository(Dapper + EF Core),CollectionGroupAuthorizationHandler/CollectionGroupAuthorizationRuleswith supporting resource and operations types,ModifyCollectionGroupAccessCommandandModifyCollectionGroupAccessValidator, and DI registrations.📸 Screenshots
N/A — server-only change.