[PM-40211] Authorize Access Rule endpoints with IOrganizationRequirement - #8162
[PM-40211] Authorize Access Rule endpoints with IOrganizationRequirement#8162Hinton wants to merge 1 commit into
Conversation
a3b75df to
ae5a67e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pam/enable-pam-flows #8162 +/- ##
========================================================
- Coverage 67.53% 63.07% -4.46%
========================================================
Files 2315 2315
Lines 100561 100566 +5
Branches 9052 9052
========================================================
- Hits 67910 63431 -4479
- Misses 30364 34944 +4580
+ Partials 2287 2191 -96 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ae5a67e to
f10979c
Compare
Declarative authorization on the access-rule routes (ADR-0022): the group requires
organization membership, and writes additionally require authority over rule
authorship via the new ManageAccessRulesRequirement. ASP.NET combines the group and
endpoint policies, so a write has to satisfy both.
Deviation from the story: no named-policy bridge. The story specifies policy-name
constants in Policies.cs registered in Api/Startup.cs, because Pam.csproj cannot
reference Api and so could not reach AuthorizeAttribute<T>. PM-41272 has since
extracted that authorization code into src/Libraries/OrganizationAuthorization,
which depends only on Core — so Pam references the library directly and attaches
AuthorizeAttribute<T> to the group and to the write routes. Policies.cs and
Startup.cs are untouched.
The requirements are carried as endpoint metadata, which AuthorizationMiddleware
combines with the group-level Policies.Application rather than replacing it; a test
asserts both are present on every route. OrganizationRequirementHandler resolves the
organization from the {orgId:guid} group prefix and is already DI-registered by
AddOrganizationAuthorization.
Providers are excluded from the resource entirely. The group gate is deliberately
MemberRequirement and not MemberOrProviderRequirement: 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 or change. That gate is
load-bearing rather than decorative — ManageAccessRulesRequirement derives from
BasePermissionRequirement, whose final arm authorizes any provider for the
organization, so the write routes would admit providers on the permission alone.
Two tests pin this: every write carries MemberRequirement alongside the permission,
and no access-rule route carries MemberOrProviderRequirement.
Scope: this covers the Access Rule half of the story only. The remaining half — the
usage gate on access-request submission, which must check both the member's
AccessPam and the organization's UsePam, plus the CipherLeaseGate parity decision —
cannot land yet. SubmitAccessRequestCommand does not exist on main: the access-request
and cipher-lease handlers are still NotImplementedException scaffolds, so there is no
submission path to gate. That gate belongs with the request/lease behaviour slices.
For the same reason there are no imperative EnsureMemberAsync/EnsureAdminAsync checks
to delete — AccessRuleEndpointsHandler never had them on main.
Note the observable change the story flags (breakdown review note 8) holds here:
unauthorized calls now return 403 from the authorization middleware.
f10979c to
0356bd4
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the declarative authorization added to the PAM access-rule routes: a group-level Code Review DetailsNo blocking findings. Notes considered and intentionally not raised as findings:
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40211
📔 Objective
Declarative authorization on the access-rule routes (ADR-0022): reads require organization membership, writes require authority over rule authorship via the new
ManageAccessRulesRequirement.Worth flagging for review:
Policies.csregistered inApi/Startup.cs, becausePam.csprojcannot reference Api and so could not reachAuthorizeAttribute<T>. PM-41272 has since extracted that code intosrc/Libraries/OrganizationAuthorization, which depends only on Core — so Pam references the library directly and attaches requirements per route.Policies.csandStartup.csare untouched.AuthorizationMiddlewarecombines with the group-levelPolicies.Application; a test asserts both are present on every route.OrganizationRequirementHandlerresolves the org from the{orgId:guid}group prefix and is already DI-registered.AccessPamand the org'sUsePam) and theCipherLeaseGateparity decision cannot land yet —SubmitAccessRequestCommanddoes not exist on main, since the access-request and cipher-lease handlers are stillNotImplementedExceptionscaffolds. There is no submission path to gate; that work belongs with the request/lease behaviour slices. For the same reason there were no imperativeEnsureMemberAsync/EnsureAdminAsyncchecks to delete —AccessRuleEndpointsHandlernever had them on main. PM-40211 should stay open for the remaining half.BasePermissionRequirement/MemberOrProviderRequirementauthorize provider users.Lockfile changes are the new project reference only — no platform churn.
Depends on #8160 and #8161 — review those first.