Skip to content

[PM-42318] Add the collection reads behind PAM's access rules - #8243

Open
Hinton wants to merge 3 commits into
mainfrom
pam/collection-managing-users
Open

[PM-42318] Add the collection reads behind PAM's access rules#8243
Hinton wants to merge 3 commits into
mainfrom
pam/collection-managing-users

Conversation

@Hinton

@Hinton Hinton commented Aug 21, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

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

📔 Objective

Two independent reads, one per commit.

1. Who manages a collection

Bitwarden can already answer which collections may this user manage: callers pull GetManyByUserIdAsync and filter on Manage, then fold in the members who manage everything by role rather than by assignment. BulkCollectionAuthorizationHandler.CanManageCollectionsAsync and CollectionAuthorizationHandler both work that way.

Nothing answers the inverse — given a collection, who manages it. Anything that has to reach the people responsible for a collection, rather than check one person's access to one collection, has no read to call. Composing the existing one means loading every member of the organization and asking the per-user question once each: the wrong shape for the question, and it scales with the organization instead of with the collection's access list.

ICollectionRepository.GetManagingUserIdsAsync returns the distinct confirmed members who can Manage, by any of the four routes:

Route to Manage Resolved from
Direct assignment CollectionUser.Manage
Through a group CollectionGroup.Manage + GroupUser
By role Owners and Admins, when the organization sets AllowAdminAccessToAllCollectionItems
By permission Custom members holding EditAnyCollection

MSSQL resolves it in Collection_ReadManagingUserIds as a union of those sources; EF reads the Permissions column in memory, because the JSON accessors the procedure uses are not portable across providers. The procedure is deliberately shaped like its neighbour CollectionCipher_ReadUserIdsByCollectionIds, which unions the same three assignment sources to resolve access rather than management.

2. Which collections an access rule governs

The collection browser renders a Controlled access column, but a collection row has nothing to put in it. No collection response model carried the association, so a client could only learn that a collection is governed by reading the organization's access rules itself.

A member can do that. A provider cannot: GET organizations/{orgId}/access-rules requires MemberRequirement, deliberately — providers manage an organization's billing and configuration, but access rules gate who can lease credentials out of it, which is not theirs to read. Providers do browse the client organization's collection list, so their column renders and every cell stays empty however the client is written.

HasEnabledAccessRule surfaces the fact on the collection instead, on the responses that feed sync and the organization listing.

It is derived, not the stored association. Collection.AccessRuleId records which rule governs a collection, but a rule that is switched off gates nothing — returning the association alone would start badging collections whose rule is disabled. Each read path joins AccessRule and reports whether it is enabled: MSSQL in Collection_ReadByUserId, Collection_ReadByIdWithPermissions and Collection_ReadSharedCollectionsByOrganizationIdWithPermissions; EF through an EXISTS subquery in UserCollectionDetailsQuery and CollectionAdminDetailsQuery, since Collection has no AccessRule navigation to traverse.

A boolean rather than the rule id, deliberately: the id is useless to a client, and it avoids handing providers the rule identity the access-rules endpoint withholds from them. Nothing writes the column and no request model gains a fieldSetAccessRuleAssociationsAsync remains the single writer of the association.

Testing

CollectionRepositoryGetManagingUserIdsTests and CollectionRepositoryHasEnabledAccessRuleTests, plus every other ~CollectionRepository integration test: 153 passing on SQL Server, SQLite and Postgres (204 with the MySQL/MariaDB rows skipped — no local instance). Api.Test 187 and Core.Test 219 on ~Collection.

All three providers earn their place here:

  • SQL Server is the only one running the stored procedures; the others take the EF paths.
  • Postgres is the only one that groups by the EXISTS subquery in translated SQL — SQLite takes the client-side .GroupBy branch instead.
  • Postgres also caught a defect the other two hid. CreateTestUserAsync names a user {identifier}-{guid}, so an identifier over 13 characters overflows User.Name. SQL Server passes the name to a procedure parameter declared NVARCHAR(50) and truncates silently on assignment rather than raising, and SQLite does not enforce declared lengths at all, so the test passed on both while inserting a name it had quietly cut short.

Reviewer notes

  • Neither read has a consumer on main yet. The first is called by a notifier on pam/uat that pushes to a collection's approvers; the second is paired with a clients change. Both are the PAM-free halves, split out so the collection layer reviews separately from PAM. The same situation as the AccessRule tables and procedures already on main.
  • The manage predicate is now stated a fourth time, alongside BulkCollectionAuthorizationHandler.CanManageCollectionsAsync, CollectionAuthorizationHandler, and (on pam/uat) ApproverCollectionAccessQuery. Centralizing it is worth doing, but not here: all three existing statements are user-first and cannot answer the collection-first question without the per-member loop above.
  • The two bare-Collection CollectionAccessDetailsResponseModel constructors — create/update responses and the provider fallbacks — cannot compute HasEnabledAccessRule and leave it false. Documented on the property; no vault list reads those responses.
  • Collection_ReadByUserId needed its columns qualified: joining AccessRule makes Id, OrganizationId and Name ambiguous against the UserCollectionDetails table function.
  • Collection_ReadManagingUserIds's migration adds a CREATE OR ALTER PROCEDURE and no table or column DDL, so it carries no existence guard. Neither migration changes the EF model, so there is no snapshot diff.

Hinton and others added 2 commits August 21, 2026 12:46
Bitwarden can already answer "which collections may this user manage": callers pull
GetManyByUserIdAsync and filter on Manage, then fold in the members who manage everything
by role rather than by assignment. BulkCollectionAuthorizationHandler and
CollectionAuthorizationHandler both work that way.

Nothing answers the inverse -- given a collection, who manages it. Anything that has to
reach the people responsible for a collection, rather than check one person's access to
one collection, has no read to call. Composing the existing one means fetching every
member of the organization and asking the per-user question once each, which is the wrong
shape for the question and scales with the size of the organization instead of the size of
the collection's access list.

Add GetManagingUserIdsAsync, which returns the distinct confirmed members who can Manage a
collection: direct Manage assignments, Manage through a group, organization Owners and
Admins when the organization allows admin access to all collection items, and Custom
members holding EditAnyCollection. MSSQL resolves it in Collection_ReadManagingUserIds as
a union of those sources; Entity Framework reads the Permissions column in memory, because
the JSON accessors the stored procedure uses are not portable across the providers.

The stored procedure is deliberately shaped like its neighbour
CollectionCipher_ReadUserIdsByCollectionIds, which unions the same three sources to resolve
access rather than management.

This does leave the manage predicate stated in a fourth place. Centralizing it is worth
doing, but not here: the three existing statements are all user-first and cannot answer
this question without the per-member loop above.

Covered by CollectionRepositoryGetManagingUserIdsTests, which runs each route to Manage --
and, for the roles, the case where the organization withholds it -- against both
implementations.
The collection browser renders a "Controlled access" column, but a collection row
had nothing to put in it: no collection response model carried the PAM association,
so a client could only learn that a collection is governed by reading the
organization's access rules itself. Providers cannot do that -- the access-rules
endpoint requires organization membership, deliberately, because rules gate who may
lease credentials out of an organization and that is not a provider's to read. Their
column stays empty however the client is written.

Surface the fact on the collection instead, as HasEnabledAccessRule on the responses
that feed sync and the organization listing.

The flag is derived rather than the stored association. Collection.AccessRuleId
records which rule governs a collection, but a rule that is switched off gates
nothing, so reporting the association alone would start badging collections whose
rule is disabled. Each read path therefore joins AccessRule and reports whether it is
enabled: MSSQL in Collection_ReadByUserId, Collection_ReadByIdWithPermissions and
Collection_ReadSharedCollectionsByOrganizationIdWithPermissions, Entity Framework via
an EXISTS subquery in UserCollectionDetailsQuery and CollectionAdminDetailsQuery
(Collection has no AccessRule navigation property to traverse) carried through the
GROUP BY key of all three repository reads.

Collection_ReadByUserId needed its columns qualified: joining AccessRule makes Id,
OrganizationId and Name ambiguous against the UserCollectionDetails table function.

Nothing writes the column and no request model gains a field --
SetAccessRuleAssociationsAsync remains the single writer of the association.

Covered by CollectionRepositoryHasEnabledAccessRuleTests, which runs the same truth
table -- ungoverned, governed by an enabled rule, governed by a disabled rule --
against every read path on both implementations.

(cherry picked from commit e5cd839)
@Hinton
Hinton force-pushed the pam/collection-managing-users branch from 63f8802 to 567860e Compare August 21, 2026 10:48
@Hinton Hinton changed the title Read the users who can manage a collection Add the collection reads behind PAM's access rules Aug 21, 2026
@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.91837% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.60%. Comparing base (ac309aa) to head (c8d0cf7).

Files with missing lines Patch % Lines
.../AdminConsole/Repositories/CollectionRepository.cs 94.93% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8243      +/-   ##
==========================================
+ Coverage   63.29%   68.60%   +5.30%     
==========================================
  Files        2401     2401              
  Lines      104043   104130      +87     
  Branches     9426     9434       +8     
==========================================
+ Hits        65857    71439    +5582     
+ Misses      35930    30340    -5590     
- Partials     2256     2351      +95     

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

@Hinton Hinton added the t:feature Change Type - Feature Development label Aug 21, 2026
Comment thread src/Sql/dbo/AdminConsole/Stored Procedures/Collection_ReadByUserId.sql Outdated
Collection_ReadManagingUserIds and the three HasEnabledAccessRule read paths
go out together, so they are one deployment step rather than two dated
scripts. Re-dated to the day the pair lands.

Also drops the note above Collection_ReadByUserId's SELECT explaining why its
columns carry the UCD prefix.
@Hinton Hinton changed the title Add the collection reads behind PAM's access rules [PM-42318] Add the collection reads behind PAM's access rules Aug 21, 2026
@Hinton
Hinton marked this pull request as ready for review August 21, 2026 14:29
@Hinton
Hinton requested review from a team as code owners August 21, 2026 14:29
@Hinton
Hinton requested a review from JaredScar August 21, 2026 14:29
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed two additive collection reads: a new ICollectionRepository.GetManagingUserIdsAsync (MSSQL Collection_ReadManagingUserIds plus an EF equivalent) and a derived HasEnabledAccessRule flag threaded through three collection read paths and their API response models. Verified dual-ORM parity between the stored procedures and the EF/LINQ reimplementations, that the manage predicate matches BulkCollectionAuthorizationHandler.CanUpdateCollectionAsync (EditAnyCollection unconditional, Owner/Admin gated on AllowAdminAccessToAllCollectionItems), that JSON_VALUE(..., '$.editAnyCollection') matches the camelCase serialization used by CoreHelpers.ClassToJsonData, and that the new LEFT JOIN [dbo].[AccessRule] is on a primary key so it cannot fan out the existing GROUP BY results. The migration is procedure-only (CREATE OR ALTER), so it is idempotent without existence guards, needs no EF snapshot change, and the added result column is backwards compatible in both rolling-deployment directions since Dapper ignores unmapped columns.

Code Review Details

No findings. Notes from validation that did not rise to findings:

  • The unguarded CoreHelpers.LoadClassFromJsonData<Permissions>(ou.Permissions) in the EF path diverges from the procedure's ISJSON(...) = 1 guard, but it matches the established pattern in CurrentContextOrganization and OrganizationUserUserDetails.
  • The bare-Collection CollectionAccessDetailsResponseModel constructors leave HasEnabledAccessRule false on create/update responses; the provider collection listing goes through GetManySharedByOrganizationIdWithPermissionsAsyncCollectionAdminDetails, so the targeted provider path is populated. Already documented on the property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant