[PM-40336] Access Rules: API - #7983
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the commercial CRUD API for org-scoped PAM Access Rules — the endpoint handler, request/response models, condition models, create/update/delete commands, and the shared write validator, plus their unit tests. Security-sensitive paths are sound: every endpoint is gated behind Code Review DetailsNo blocking findings. Notes considered and not flagged:
|
0351a28 to
37a5507
Compare
37a5507 to
a85b172
Compare
a85b172 to
9c09687
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## pam/access-rule #7983 +/- ##
===================================================
- Coverage 67.30% 62.80% -4.51%
===================================================
Files 2307 2315 +8
Lines 100433 100700 +267
Branches 9028 9052 +24
===================================================
- Hits 67599 63241 -4358
- Misses 30553 35265 +4712
+ Partials 2281 2194 -87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
56c6cac to
44b46da
Compare
c17c469 to
ccf6444
Compare
e5f25d5 to
801a80f
Compare
801a80f to
ca963d2
Compare
77d93e9 to
9a6165c
Compare
9a6165c to
d6deeaa
Compare
d6deeaa to
e13bb46
Compare
e13bb46 to
e2e3279
Compare
e2e3279 to
d66c669
Compare
d66c669 to
c52178c
Compare
| sutProvider.GetDependency<ICurrentContext>().ManageAccessRules(organization.Id).Returns(true); | ||
|
|
||
| await sutProvider.Sut.ValidateOrganizationUserUpdatePermissions( | ||
| organization.Id, organizationUserInvite.Type.Value, null, invitePermissions); |
|
|
||
| var exception = await Assert.ThrowsAsync<BadRequestException>( | ||
| () => sutProvider.Sut.ValidateOrganizationUserUpdatePermissions( | ||
| organization.Id, organizationUserInvite.Type.Value, null, invitePermissions)); |
c52178c to
394ed90
Compare
Remove the time_of_day condition kind along with the AccessWeekday enum and its JSON converter, which existed only to type the weekday tokens inside a time window. Access rules now expose human_approval and ip_allowlist; a document that still carries a time_of_day entry is rejected as an unknown kind.
The create and update commands each carried their own copy of the same checks: name required, a positive maximum when extensions are allowed, the conditions document, name uniqueness within the organization, and the collection lookup that confirms every requested collection exists, belongs to the organization, and is not already governed by another rule. Move all of it into AccessRuleWriteValidator, which takes the id of the rule being updated, or null when creating. Both places where the two paths differ reduce to a comparison against that id: an update excludes itself from the uniqueness check and may keep the collections it already governs, and with a null id the same expressions give a create its stricter behaviour, where any governed collection conflicts. The validator returns the deduplicated collection ids so callers do not normalize them a second time. The commands keep only what is theirs. Neither needs ICollectionRepository any more, and update keeps its existence guard and the plain AccessRule it maps for persistence. Update now resolves the rule before judging the payload, so editing a rule that does not exist, or belongs to another organization, is a 404 rather than a 400 from a field check. The command tests assert the validator is never reached in those cases to hold that order in place, and the shared rules are covered once in AccessRuleWriteValidatorTests, including the create and update asymmetries.
….csproj Pam.csproj now references Pam.Domain directly for the AccessRule command and validator implementations, so consumers of the Pam service need the reference recorded in their own lock files too. Force-evaluated a full-solution restore to bring every lock file back in sync.
The access-rule commands now take ICollectionRepository for the collection association write, matching the method's new home after review feedback on #7981.
PAM's access-rule endpoints need an organization-scoped permission to authorize against. Adding it to Permissions rather than inventing a PAM-local requirement means Custom members can be granted rule administration without also being made Admins, and it reuses BasePermissionRequirement so Owners, Admins and providers are covered by the same rule everything else follows. Permissions is a JSON blob on OrganizationUser, so there is no migration. The guard in OrganizationService.ValidateCustomPermissionsGrant is load-bearing rather than mechanical: that path is an explicit per-permission if-chain, and without an entry a Custom member holding ManageUsers could grant a permission they do not hold themselves. The newer OrganizationUserValidationService walks ClaimsMap generically and needs no change. PermissionsModel carries the new field in both directions because the public API round-trips a member through it — omitting it would silently strip an existing grant on update. Granting this from the web vault still needs a matching toggle in clients. Until that ships the permission is simply never set, so behaviour is unchanged.
The handler was hand-rolling ICurrentContext membership and admin checks, which
is what OrganizationAuthorization was extracted out of Api to avoid. Pam can
reference the library directly now, so the endpoints authorize the way
controllers do: MemberRequirement on the group and ManageAccessRulesRequirement
on the write endpoints. ASP.NET combines the group and endpoint policies, so a
write has to satisfy both.
The group uses MemberRequirement rather than MemberOrProviderRequirement, which
narrows what the handler used to allow: currentContext.OrganizationUser() falls
through to OrganizationOwner(), which authorizes a provider for the
organization. Providers manage an organization's billing and configuration, but
access rules gate who can lease credentials out of it, and that is not theirs to
change.
Worth knowing for later: the group gate is the only thing keeping providers out.
ManageAccessRulesRequirement derives from BasePermissionRequirement, whose final
case authorizes a provider for the organization, so widening the group would
silently readmit them to the writes as well. There is a test for that.
No DI change — Api already calls AddOrganizationAuthorization() from
AddAdminConsoleAuthorizationHandlers, and Pam is hosted in Api.
Behaviour change: an unauthorized caller now gets 403 rather than the 404 the
handler used to throw. That matches every other organization-scoped endpoint in
Api, and these endpoints are unreleased and gated behind FeatureFlagKeys.Pam.
The rule.OrganizationId != orgId check stays in the handler. That one is
resource scoping rather than authorization: it stops a rule ID from one
organization being read through another organization's route.
Leaves /leases, /access-requests and /leases/ciphers/{id} alone — they carry no
{orgId} in the route, so IOrganizationRequirement cannot bind, and their
handlers are still NotImplementedException scaffolds.
Adds tests over the materialized endpoint metadata, since nothing in the
handler fails closed any more if a RequireAuthorization call is dropped.
394ed90 to
919ae0d
Compare
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40336
📔 Objective
Add the commercial CRUD API for org-scoped PAM Access Rules, consuming the
domain and persistence layer from #7981. This PR is stacked on #7981 — review
and merge that first; GitHub will retarget this PR to
mainonce it lands.All endpoints are gated behind the
pm-37044-pam-v-0(FeatureFlagKeys.Pam)feature flag, so nothing is reachable in production until PAM v0 ships. Access-rule
data (name, description, conditions) is organization configuration metadata, not
Vault Data — no zero-knowledge surface is touched.
What's included
API —
bitwarden_license/src/Services/PamGET(list),GET {id},POST,PUT {id},DELETE {id}, all org-scoped and feature-flag gated.AccessRuleRequestModel,AccessRuleResponseModel)and the condition models:
human_approvalandip_allowlist.Create/Update/Deletecommands behind interfaces;AccessRuleValidatorvalidating the conditions JSON (max 10 conditions, per-type rules; empty list is
vacuously valid and still routes access through PAM for audit).
📸 Screenshots
N/A — backend only.
Update: authorization now runs in the middleware
AccessRuleEndpointsHandlerwas hand-rollingICurrentContextmembership and adminchecks. Now that #8095 has landed on
main,Pamcan referencesrc/Libraries/OrganizationAuthorizationdirectly andauthorize the way controllers do — which is the follow-up that PR deliberately left
out ("Pam is not wired up here — that belongs with the endpoint changes that consume
it").
MemberOrProviderRequirementon the group, matching theOrganizationUsercheck itreplaces.
ManageAccessRulescustom permission, withManageAccessRulesRequirementonPOST/PUT/DELETE. ASP.NET combines the group and endpoint policies, so a writehas to satisfy both.
rule.OrganizationId != orgIdcheck stays in the handler — that is resourcescoping, not authorization.
/leases,/access-requestsand/leases/ciphers/{id}are untouched: they carry no{orgId}in the route, soIOrganizationRequirementcannot bind.Behaviour change: a non-member now gets 403 rather than the 404 the handler
used to throw. That matches every other organization-scoped endpoint in
Api, andthese endpoints are unreleased and gated behind
FeatureFlagKeys.Pam.Important
@bitwarden/team-admin-console-dev — the first of the two new commits
(
Add the ManageAccessRules custom permission) is AC-owned and PAM-free: it touchesPermissions,Claims,CurrentContext,OrganizationService, theOrganizationAuthorizationlibrary, and the public APIPermissionsModel. Happy tosplit it into its own PR against
mainif you would rather review it separately.Two parts of it are load-bearing rather than mechanical:
OrganizationService.ValidateCustomPermissionsGrant— that path is anexplicit per-permission if-chain, and without an entry a Custom member holding
ManageUserscould grant a permission they do not hold themselves. The newerOrganizationUserValidationServicewalksClaimsMapgenerically and needs no change.PermissionsModelcarries the field in both directions, because the public APIround-trips a member through it — omitting it would silently strip an existing grant
on update.
No migration:
Permissionsis a JSON blob onOrganizationUser. Granting the permissionfrom the web vault still needs a matching toggle in
bitwarden/clients; until that shipsthe permission is never set, so Owners, Admins and providers retain exactly today's access.