Skip to content

[PM-40211] Authorize Access Rule endpoints with IOrganizationRequirement - #8162

Open
Hinton wants to merge 1 commit into
pam/enable-pam-flowsfrom
pam/authorize-access-rules
Open

[PM-40211] Authorize Access Rule endpoints with IOrganizationRequirement#8162
Hinton wants to merge 1 commit into
pam/enable-pam-flowsfrom
pam/authorize-access-rules

Conversation

@Hinton

@Hinton Hinton commented Aug 7, 2026

Copy link
Copy Markdown
Member

🎟️ 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:

  • Deviation: 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 code into src/Libraries/OrganizationAuthorization, which depends only on Core — so Pam references the library directly and attaches requirements per route. Policies.cs and Startup.cs are untouched.
  • Requirements combine with, not replace, the group policy. They're carried as endpoint metadata, which AuthorizationMiddleware combines with the group-level Policies.Application; a test asserts both are present on every route. OrganizationRequirementHandler resolves the org from the {orgId:guid} group prefix and is already DI-registered.
  • Scope: the Access Rule half only. The usage gate on access-request submission (which must check the member's AccessPam and the org's UsePam) and the CipherLeaseGate parity decision cannot land yet — SubmitAccessRequestCommand does not exist on main, since the access-request and cipher-lease handlers are still NotImplementedException scaffolds. There is no submission path to gate; that work belongs with the request/lease behaviour slices. For the same reason there were no imperative EnsureMemberAsync/EnsureAdminAsync checks to delete — AccessRuleEndpointsHandler never had them on main. PM-40211 should stay open for the remaining half.
  • The two observable changes flagged in breakdown review note 8 hold here: unauthorized calls return 403 from the middleware rather than 404 from a handler, and BasePermissionRequirement/MemberOrProviderRequirement authorize provider users.

Lockfile changes are the new project reference only — no platform churn.

Depends on #8160 and #8161 — review those first.

@Hinton
Hinton force-pushed the pam/authorize-access-rules branch from a3b75df to ae5a67e Compare August 7, 2026 11:11
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.07%. Comparing base (b937e2f) to head (0356bd4).

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.
📢 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 force-pushed the pam/authorize-access-rules branch from ae5a67e to f10979c Compare August 7, 2026 11:36
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.
@Hinton
Hinton force-pushed the pam/authorize-access-rules branch from f10979c to 0356bd4 Compare August 7, 2026 12:00
@Hinton Hinton added the t:feature Change Type - Feature Development label Aug 7, 2026
@Hinton
Hinton marked this pull request as ready for review August 7, 2026 12:02
@Hinton
Hinton requested review from a team as code owners August 7, 2026 12:02
@Hinton
Hinton requested a review from eliykat August 7, 2026 12:02
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the declarative authorization added to the PAM access-rule routes: a group-level MemberRequirement plus ManageAccessRulesRequirement on the three write endpoints, the new ManageAccessRulesRequirement in PermissionRequirements.cs, and the OrganizationAuthorization project reference from Pam.csproj. Verified that Permissions.ManageAccessRules and its claim already exist, that OrganizationRequirementHandler is DI-registered in Api via AddAuthorizationHandlers(), that the {orgId:guid} group prefix satisfies HttpContextExtensions.GetOrganizationId(), and that ASP.NET's AuthorizationPolicy.CombineAsync accumulates the group's Policies.Application with the per-endpoint requirement metadata rather than replacing it — matching what the new tests assert. Lock files are consistent with the single project-reference addition across all six affected manifests; no new NuGet packages, so no AppSec dependency approval is required.

Code Review Details

No blocking findings.

Notes considered and intentionally not raised as findings:

  • Reads being gated at plain organization membership is stated as the intended design in the PR description, and the handlers are still NotImplementedException scaffolds, so there is no live data exposure.
  • A provider user who is also an org member would pass both gates on writes; this is inherent to BasePermissionRequirement's provider fallback across all existing consumers, not introduced here.
  • The group documents 400/404 in OpenAPI but not 401/403; a documentation-only gap.

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