[PM-12473] feat: Add CollectionUser authorization handler and delta access endpoint - #8075
[PM-12473] feat: Add CollectionUser authorization handler and delta access endpoint#8075r-tome wants to merge 21 commits into
Conversation
…cross multiple collections Generalizes ModifyUserAccessAsync to accept a list of collection ids instead of one, and adds a CollectionUser_DeleteMany procedure so the same delta can be applied to multiple collections in a single call, in both the Dapper and EF Core implementations.
… access Adds CollectionUserAuthorizationHandler and CollectionUserAuthorizationRules to decide whether the caller can add, change, or remove another user's access to one or more collections, reusing BulkAuthorizationHandler so the same handler covers single- and multi-collection requests.
Adds the domain command and validator that turn an add/update/remove delta into a validated, per-target set of user-access changes across one or more collections, plus the feature flag gating the new endpoint.
Adds CollectionUserController with single- and bulk-collection PATCH routes that apply an add/update/remove delta to a collection's user access, behind the PM12473CollectionUserAccessEndpoint feature flag.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed after the commits that removed the Code Review Details
PR Metadata Assessment
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8075 +/- ##
===========================================
+ Coverage 14.96% 67.44% +52.48%
===========================================
Files 1389 2317 +928
Lines 60360 100516 +40156
Branches 4793 9050 +4257
===========================================
+ Hits 9030 67790 +58760
+ Misses 51175 30453 -20722
- Partials 155 2273 +2118 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var addIds = add.Select(a => a.Id).ToHashSet(); | ||
| var updateIds = update.Select(u => u.Id).ToHashSet(); | ||
| var removeIds = remove.ToHashSet(); |
There was a problem hiding this comment.
null for add/update/remove/collectionIds in the JSON body throws NullReferenceException (HTTP 500).
Details and fix
The = [] initializers on CollectionUserAccessDeltaRequestModel / BulkCollectionUserAccessDeltaRequestModel only apply when the property is absent from the payload. System.Text.Json overwrites the property with null when the JSON contains an explicit null, so:
PATCH /organizations/{orgId}/collections/{id}/users
{ "add": null }
reaches add.Select(a => a.Id).ToHashSet() on line 48 with add == null and produces a 500 rather than a 400. Same for model.CollectionIds.ToList() on line 41.
Either null-coalesce at the boundary:
=> ModifyUserAccessAsync(orgId, [id], model.Add ?? [], model.Update ?? [], model.Remove ?? []);or make the properties null-tolerant/validated on the request models (e.g. [Required] on CollectionIds, ?? [] in a getter). Note PostBulkCollectionAccess in CollectionsController guards with model.Users?.Select(...) for the same reason.
| var updateIds = update.Select(u => u.Id).ToHashSet(); | ||
| var removeIds = remove.ToHashSet(); | ||
|
|
||
| var organizationCollections = await collectionRepository.GetManyByOrganizationIdWithAccessAsync(orgId); |
There was a problem hiding this comment.
🎨 SUGGESTED: The single-collection route reads the entire organization's collection access graph to resolve one collection.
Details and fix
GetManyByOrganizationIdWithAccessAsync runs Collection_ReadWithGroupsAndUsersByOrganizationId, which returns every Collection, every CollectionGroup, and every CollectionUser row in the org (three result sets). For an org with 5k collections and 100k access rows, changing one member on one collection transfers the whole graph — and it happens before any authorization check, so an unauthorized caller triggers it too.
GetByIdWithAccessAsync(id) already exists and returns exactly what CollectionUserAccessTarget needs, so the PatchCollectionUserAccessAsync path can be O(1) rows instead of O(org):
var (collection, accessDetails) = await collectionRepository.GetByIdWithAccessAsync(id);
if (collection is null || collection.OrganizationId != orgId) { throw new NotFoundException(); }The bulk route has no equivalent targeted repository method today, so keeping the org-wide read there (or adding a GetManyByManyIdsWithAccessAsync) is a reasonable follow-up. The only current caller of the org-wide query on a hot path is BulkCollectionAuthorizationHandler, and it uses it only as an orphaned-collection fallback.
| /// <param name="upserts">The user access selections to create or update.</param> | ||
| /// <param name="removeOrganizationUserIds">The Organization User IDs to remove access for.</param> | ||
| /// <param name="revisionDate">The revision date to use for the collections.</param> | ||
| Task ModifyUserAccessAsync(Guid organizationId, IEnumerable<Guid> collectionIds, |
There was a problem hiding this comment.
♻️ DEBT: New dual-ORM method has no Infrastructure.IntegrationTest coverage, and the two implementations apply operations in opposite order.
Details and parity risk
The Dapper path runs CollectionUser_DeleteMany (removals) then Collection_CreateOrUpdateAccessForMany (upserts). The EF path runs upserts then removals. For an organization user id present in both upserts and removeOrganizationUserIds, MSSQL ends with the user having access while PostgreSQL/MySQL/SQLite end with the user removed.
ModifyCollectionUserAccessValidator rejects overlapping ids today (OverlappingOrganizationUserId), so the divergence isn't reachable through the new endpoints — but this is public interface surface and the next caller won't know that invariant. Either document the disjointness requirement on this XML doc comment, or align the order across both implementations.
Either way, the sibling CreateOrUpdateAccessForManyAsync is covered by test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs:613, which is what validates behavior across all four supported databases. ModifyUserAccessAsync — a new stored procedure plus ~90 lines of hand-written EF logic — has none.
|
|
||
| public bool IsAdminOrOwner => Type is OrganizationUserType.Owner or OrganizationUserType.Admin; | ||
|
|
||
| public bool HasPermission(Func<Permissions, bool> permissionPicker) => permissionPicker(Permissions); |
There was a problem hiding this comment.
♻️ DEBT: HasPermission(Func<Permissions, bool>) adds delegate-based indirection to a core context class for a single call site.
Details
Permissions is already public, and the codebase consistently uses property pattern matching for this (org is { Permissions.EditAnyCollection: true } in BulkCollectionAuthorizationHandler, and on the line directly above the only consumer in CollectionUserAuthorizationRules.cs:23). That file ends up mixing both idioms in the same method:
if (organization is { Permissions.EditAnyCollection: true }) { ... } // line 23
if (... && organization?.HasPermission(p => p.ManageUsers) == true) { ... } // line 28organization is { Permissions.ManageUsers: true } is equivalent, needs no new public surface on CurrentContextOrganization, and keeps the rules file internally consistent. IsAdminOrOwner is a worthwhile addition (it replaces a genuinely repeated two-way type check); HasPermission is not carrying its weight yet.
r-tome
changed the title
…esultExtensions Pulls the CommandResult -> HTTP response mapping out of BaseAdminConsoleController into extension methods on CommandResult, so it can be shared with Minimal API handlers instead of only MVC controllers.
…ller to Minimal API Replaces CollectionUserController with Minimal API endpoint groups (CollectionUserEndpoints, mapped from the new AdminConsoleEndpoints umbrella) and a plain DI-injected handler class, matching the same pattern PAM already uses in this codebase. Feature-gating uses the SDK-native RequireFeature() endpoint-convention-builder extension, which already returns RFC 7807 ProblemDetails on a disabled flag via the existing UseFeatureFlagChecks() middleware. A new AdminConsoleExceptionHandlerEndpointFilter, scoped to admin console endpoints, translates NotFoundException and unexpected exceptions into ProblemDetails, since Minimal API doesn't run MVC exception filters.
…extension Pulls the parts of the endpoint chain that are the same for every admin console Minimal API group (authorization policy, exception filter) into a shared extension, matching PAM's WithPamDefaults. Feature-flag gating stays a separate per-group call since each admin console feature has its own flag.
Adds a sweep test over AdminConsoleEndpoints.MapAdminConsoleEndpoints that asserts every mapped endpoint has authorization or allow-anonymous metadata. The existing controller-based authorization sweep only reflects over ControllerBase types, so it can't see Minimal API endpoints. Handler types are discovered by reflection, so future admin console endpoint groups are covered automatically.
CannotAddSelfToCollection only blocks joining a collection you're not already in. Documents that raising your own access on a collection you already belong to is intentionally left to the authorization layer, which already requires you to manage that collection before this validator runs.
| group.MapPatch("{id:guid}/users", | ||
| (Guid orgId, Guid id, CollectionUserAccessDeltaRequestModel model, ClaimsPrincipal user, | ||
| CollectionUserEndpointsHandler handler) => | ||
| handler.PatchUserAccessAsync(orgId, [id], model.Add, model.Update, model.Remove, user)) | ||
| .WithName("PatchCollectionUserAccess"); | ||
|
|
||
| group.MapPatch("users", | ||
| (Guid orgId, BulkCollectionUserAccessDeltaRequestModel model, ClaimsPrincipal user, | ||
| CollectionUserEndpointsHandler handler) => | ||
| handler.PatchUserAccessAsync(orgId, model.CollectionIds.ToList(), model.Add, model.Update, model.Remove, user)) | ||
| .WithName("PatchBulkCollectionUserAccess"); |
There was a problem hiding this comment.
❌ CRITICAL: CollectionUserEndpointsHandler is never registered in DI, so Minimal API binds it from the request body and endpoint construction throws.
Details and fix
RequestDelegateFactory resolves a complex parameter from DI only when IServiceProviderIsService.IsService(type) returns true; otherwise it infers a body parameter. CollectionUserEndpointsHandler has no registration anywhere in src/:
$ grep -rn "CollectionUserEndpointsHandler" --include=*.cs src/
src/Api/AdminConsole/Endpoints/CollectionUserEndpoints.cs:21
src/Api/AdminConsole/Endpoints/CollectionUserEndpoints.cs:27
src/Api/AdminConsole/Endpoints/Handlers/CollectionUserEndpointsHandler.cs:15
So both model and handler are inferred as body parameters and the delegate factory throws InvalidOperationException: Failure to infer how parameters ... should be bound. Endpoints are built lazily by EndpointRoutingMiddleware over the composite data source, so this takes down routing for the whole API on the first request, not just these two routes — and the feature flag does not protect against it, since RequireFeature only adds metadata checked at request time after the endpoint has been built.
The unit tests pass because they register it themselves (CollectionUserEndpointsTests.cs:20, and AdminConsoleEndpointsAuthorizationTests.cs:16-21 reflects over the Handlers namespace and registers everything), so no test exercises the real container.
PAM handles this with bitwarden_license/src/Services/Pam/Utilities/ServiceCollectionExtensions.cs:
services.AddScoped<LeaseEndpointsHandler>();
services.AddScoped<AccessRequestEndpointsHandler>();
services.AddScoped<AccessRuleEndpointsHandler>();wired up via services.AddPamServices() in Startup.cs:211. An equivalent AddAdminConsoleEndpointHandlers() (or a single services.AddScoped<CollectionUserEndpointsHandler>()) needs to be called from Startup.ConfigureServices.
| public static RouteGroupBuilder WithAdminConsoleDefaults(this RouteGroupBuilder group) | ||
| { | ||
| group.RequireAuthorization(Policies.Application); | ||
| group.AddEndpointFilter<AdminConsoleExceptionHandlerEndpointFilter>(); | ||
| return group; | ||
| } |
There was a problem hiding this comment.
WithGroupName("internal"), these endpoints are emitted into the published public OpenAPI spec.
Details and fix
Swashbuckle's default DocInclusionPredicate is apiDescription.GroupName == null || apiDescription.GroupName == documentName, so an endpoint with no group name lands in every SwaggerDoc — here both internal and public (src/Api/Utilities/ServiceCollectionExtensions.cs:23,48). The public document is the customer-facing Bitwarden Public API spec shipped to the help center and used for SDK generation.
MVC controllers avoid this via ApiExplorerGroupConvention (Startup.cs:218), but that is an IControllerModelConvention and does not apply to Minimal APIs. AddEndpointsApiExplorer() is called (Startup.cs:223), so these endpoints do reach ApiExplorer. PAM sets the group name explicitly for exactly this reason (PamEndpointsExtensions.cs:33).
Since WithAdminConsoleDefaults is the shared chain every future admin console group will use, fixing it here covers all of them:
group.RequireAuthorization(Policies.Application);
group.AddEndpointFilter<AdminConsoleExceptionHandlerEndpointFilter>();
group.WithGroupName("internal");
return group;
| catch (NotFoundException) | ||
| { | ||
| return TypedResults.Problem(title: "Resource not found.", statusCode: StatusCodes.Status404NotFound); | ||
| } | ||
| catch (Exception exception) | ||
| { | ||
| var endpointName = context.HttpContext.GetEndpoint()?.DisplayName; | ||
| context.HttpContext.RequestServices.GetRequiredService<ILogger<AdminConsoleExceptionHandlerEndpointFilter>>() | ||
| .LogError(exception, "Unhandled exception in {EndpointName}", endpointName); | ||
| return TypedResults.Problem( | ||
| title: "An error has occurred.", statusCode: StatusCodes.Status500InternalServerError); | ||
| } |
There was a problem hiding this comment.
♻️ DEBT: These endpoints return two different error body shapes — ProblemDetails for thrown exceptions, ErrorResponseModel for command errors.
Details
CommandResultExtensions.MapError returns new ErrorResponseModel(...) ({"object":"error","message":"..."}), which is the contract Bitwarden clients parse. This filter returns RFC 7807 ProblemDetails ({"type":...,"title":...,"status":...}) with no message field, so the same endpoint answers a 404 two different ways depending on whether it came from NotFoundError or from AuthorizeOrThrowAsync.
The 404-from-authorization path is the common one here (AuthorizeOrThrowAsync throws NotFoundException on every failed check), so in practice clients get the shape they can't read for the most frequent failure.
PamExceptionHandlerEndpointFilter was written for this exact problem and documents it: "this filter translates thrown exceptions into Bitwarden's ErrorResponseModel with the same status codes the controllers produced." It also maps BadRequestException → 400, UnauthorizedAccessException → 401, and ConflictException → 409; this filter sends all of those to 500. CollectionUserAuthorizationHandler throws BadRequestException for a cross-organization resource set — not reachable through today's two routes, but it will be as soon as a group is added whose handler doesn't pre-filter by orgId.
Consider reusing the PAM mapping (lifting it into a shared filter) rather than a second, narrower translation layer.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Shortens XML docs and inline comments throughout, removing restatements of what the code already says.
Minimal API's RequestDelegateFactory only resolves a complex parameter from DI when it's registered as a service; otherwise it infers a body parameter. CollectionUserEndpointsHandler was never registered, so both the model and the handler parameter were inferred as body, and endpoint construction threw InvalidOperationException for both PATCH routes. Unit tests missed this because they register the handler themselves.
… requests The = [] initializers on the request models only apply when a property is absent from the JSON body; an explicit null overwrites them, and the handler called .Select(...) on it with no null-check, producing a 500 instead of a 400. Matches the existing precedent in CollectionsController (model.Users?.Select(...) ?? new List<...>()).
…ublic OpenAPI spec Without a group name, Swashbuckle's default inclusion predicate emits an endpoint into every SwaggerDoc, including the customer-facing public API document shipped to the help center and used for SDK generation. MVC controllers avoid this via ApiExplorerGroupConvention, which doesn't apply to Minimal API. PAM sets the same group name for the same reason. Fixing it in the shared WithAdminConsoleDefaults chain covers every current and future admin console group.
…ide fetch Both routes called GetManyByOrganizationIdWithAccessAsync, pulling every collection, group, and user-access row in the organization before any authorization check, just to resolve 1 (or a few) requested collections. GetByIdWithAccessAsync already returns exactly the shape needed, so the single-collection route (the common case) now resolves in O(1) rows instead of O(org). This is a single branch inside the one shared handler method, so single and bulk still share the same validation, authorization, and command path.
…rder Dapper ran removes then upserts; EF Core ran upserts then removes. Not reachable today because the validator rejects an id present in both lists, but it's public interface surface with nothing to catch a future regression: for an id present in both lists, MSSQL and PostgreSQL/MySQL/SQLite would have ended in opposite states. Reorders EF Core to match Dapper, and adds an integration test exercising add, update, and remove in one call across all four supported databases — the sibling CreateOrUpdateAccessForManyAsync already had this kind of coverage, this method had none.
Had exactly one call site. The codebase's established idiom for this is
property-pattern matching, which the same file already uses one check
above (organization is { Permissions.EditAnyCollection: true }); the
delegate-based HasPermission added indirection without a second consumer
to justify it.
…andlerEndpointFilter Thrown exceptions returned RFC 7807 ProblemDetails while CommandResult validation failures returned ErrorResponseModel, so the same two endpoints answered a 404 two different ways depending on which layer caught it. The exception path is the common one here, since AuthorizeOrThrowAsync throws NotFoundException on every failed authorization check, so most callers hit the shape without a message field. ErrorResponseModel is what every other Bitwarden endpoint already returns; changing CommandResultExtensions instead would ripple into every MVC controller that shares it. Also adds a BadRequestException case (400), since CollectionUserAuthorizationHandler's cross-organization guard throws it and a future admin console group could reach it.
…n user access None of the existing tests exercise the real DI container end-to-end through actual HTTP, which is exactly why the missing handler registration shipped past unit tests that register the handler themselves. Adds CollectionUserEndpointsTests using the same ApiApplicationFactory pattern as CollectionsControllerTests: signs up an org, adds/updates a real collection's user access via a real PATCH request, and verifies the resulting access through the repository.
The feature flag key belongs to the parent epic PM-35160, not PM-12473.
…roundwork for unified endpoint The team decided the final API shape is a single unified endpoint handling collection metadata + users delta + groups delta in one call. Shipping separate CollectionUser PATCH endpoints now would create temporary API surface to retire later. The groundwork (repository, auth handler, command, validator) is all reusable for the unified endpoint.
…mandResultExtensions CommandResultExtensions was extracted to share HTTP mapping between MVC controllers and Minimal API handlers. With the endpoints removed there is only one consumer, so the extraction adds indirection with no benefit. Revert BaseAdminConsoleController to its original self-contained form.
| var upsertIds = addIds.Concat(updateIds).ToList(); | ||
| if (upsertIds.Count > 0) | ||
| { | ||
| var organizationId = request.Targets.First().Collection.OrganizationId; |
There was a problem hiding this comment.
Targets is non-empty or that all targets share one organization — the two guards its sibling BulkAddCollectionAccessCommand does have.
Details and fix
Two consequences:
- Empty
Targetswith a non-empty delta reaches line 58 andrequest.Targets.First()throwsInvalidOperationException("Sequence contains no elements") rather than returning aBadRequestError.ModifyCollectionUserAccessCommand.ModifyAsynchas the sameTargets.First()on the organization id. - Targets spanning organizations:
organizationIdis taken fromTargets.First(), soOrganizationUsersNotInOrganizationonly validates users against the first target's org.Collection_CreateOrUpdateAccessForManythen CROSS JOINs@CollectionIdswith@Usersand only org-scopes the users side — org A's members would be granted access to org B's collection.
CollectionUserAuthorizationHandler does throw on mixed-organization resources today, so this is only reachable if a future caller passes a different set to the command than it authorized. Given this is the domain-layer guard for a multi-tenant boundary, the check belongs here too:
if (request.Targets.Count == 0)
{
return Invalid(request, new NoCollectionsProvided());
}
var organizationId = request.Targets.First().Collection.OrganizationId;
if (request.Targets.Any(t => t.Collection.OrganizationId != organizationId))
{
return Invalid(request, new TargetsMustBelongToSameOrganization());
}Compare src/Core/AdminConsole/OrganizationFeatures/Collections/BulkAddCollectionAccessCommand.cs lines 56-71.
| var authorized = true; | ||
| foreach (var resource in resources) | ||
| { | ||
| var callerManagesCollection = await CallerManagesCollectionAsync(resource.Collection.Id); | ||
| if (!CollectionUserAuthorizationRules.CanModifyUserAccess( | ||
| resource.AccessDetails, organization, allowAdminAccessToAllCollectionItems, callerManagesCollection)) | ||
| { | ||
| authorized = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎨 SUGGESTED: CallerManagesCollectionAsync is awaited before the rules run, so every authorization call issues a GetManyByUserIdAsync query — including the admin paths that never need it.
Details and fix
CanModifyUserAccess short-circuits on EditAnyCollection, AllowAdminAccess + ManageUsers, and AllowAdminAccess + IsAdminOrOwner before it ever looks at callerManagesCollection. But because the value is computed eagerly at line 59, the repository call always happens. The handler this replaces (BulkCollectionAuthorizationHandler.CanUpdateUserAccessAsync → CanUpdateCollectionAsync) returned before touching CanManageCollectionsAsync for those cases, so this adds one DB round-trip per authorization for the most common Admin Console caller.
Checking the cheap org-level rules first keeps the query on the path that actually needs it:
var authorized = true;
foreach (var resource in resources)
{
if (CollectionUserAuthorizationRules.CanModifyUserAccess(
resource.AccessDetails, organization, allowAdminAccessToAllCollectionItems,
callerManagesCollection: false))
{
continue;
}
if (!CollectionUserAuthorizationRules.CanModifyUserAccess(
resource.AccessDetails, organization, allowAdminAccessToAllCollectionItems,
await CallerManagesCollectionAsync(resource.Collection.Id)))
{
authorized = false;
break;
}
}Passing a Func<Task<bool>> into the rules instead would read better if you'd rather not evaluate twice.
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12473
📔 Objective
This is part of the authz refactoring on server to decouple
CollectionUserauthorization handling fromBulkCollectionAuthorizationHandler, which currently handles everything.This PR ships the groundwork:
CollectionUserAuthorizationHandler,ModifyCollectionUserAccessCommand,ModifyCollectionUserAccessValidator,ModifyUserAccessAsyncon the repository (Dapper + EF Core), and a newCollectionUser_DeleteManysproc so changes are applied atomically. The endpoint is deferred to PM-41448.📸 Screenshots
N/A — server-only change.