Skip to content

[PM-40210] Add single + bulk PAM enable flows for AccessPam - #8161

Open
Hinton wants to merge 1 commit into
pam/claims-permission-plumbingfrom
pam/enable-pam-flows
Open

[PM-40210] Add single + bulk PAM enable flows for AccessPam#8161
Hinton wants to merge 1 commit into
pam/claims-permission-plumbingfrom
pam/enable-pam-flows

Conversation

@Hinton

@Hinton Hinton commented Aug 7, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

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

📔 Objective

The member-management flows that grant and revoke AccessPam, the admin-controlled per-member PAM access grant. Both paths are plain field writes behind an org-level UsePam check.

Worth flagging for review:

  • No seat logic anywhere. PAM has no seats, so there is deliberately no analogue of the SM pattern — no CountNewSmSeatsRequiredQuery, no UpdateSecretsManagerSubscriptionCommand, no autoscale-last ordering rule. The story's earlier AllocatePamSeats version is superseded.
  • Why an explicit UsePam guard. SM has no explicit check in these flows; it leans on CountNewSmSeatsRequiredQuery throwing "Organization does not use Secrets Manager" as a side effect of counting seats. With no seat count to piggyback on, PAM checks UsePam directly — otherwise AccessPam = true on a non-PAM org is inert but unvalidated, since claim emission ANDs the two (PM-40209), and the admin gets a silently ineffective toggle instead of an error.
  • Only the grant is gated. Revoking access is not checked against UsePam, so removing a member's access stays possible on an org whose entitlement has lapsed. Editing a member who already holds access is likewise not a grant.
  • Both sides of the flag. The single-user path is implemented in both the v1 command and the v2 command behind ChangeMemberEmailNoMp, so the toggle behaves identically whichever serves the request. v2 gets an IsEnablingPam() transition helper and a typed PamNotEnabled validator error; v1 guards inline with the other validation.
  • Bulk endpoint is PUT organizations/{orgId}/users/enable-pam, mirroring BulkEnableSecretsManagerAsync minus the seat block. SM's obsolete PATCH alias is not replicated, and like SM's bulk path it logs no events (breakdown review note 5).

UpdateOrganizationUser gains an accessPam parameter; its only production caller is the v2 command.

Depends on #8160 (claims plumbing) — review that first.

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 67.53%. Comparing base (76f8d09) to head (b937e2f).

Files with missing lines Patch % Lines
...Console/Controllers/OrganizationUsersController.cs 94.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                        Coverage Diff                         @@
##           pam/claims-permission-plumbing    #8161      +/-   ##
==================================================================
+ Coverage                           63.06%   67.53%   +4.47%     
==================================================================
  Files                                2315     2315              
  Lines                              100530   100561      +31     
  Branches                             9045     9052       +7     
==================================================================
+ Hits                                63395    67910    +4515     
+ Misses                              34945    30364    -4581     
- Partials                             2190     2287      +97     

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

Comment on lines +136 to +142
// Granting PAM access to a member of an organization without PAM would be inert: claim emission ANDs
// AccessPam with the organization's UsePam. Reject so the admin gets an actionable error instead.
// Only the grant is gated — revoking access stays possible on an organization whose entitlement has lapsed.
if (!originalOrganizationUser.AccessPam && organizationUser.AccessPam && !organization.UsePam)
{
throw new BadRequestException("To grant PAM access the organization must have PAM enabled.");
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could technically skip this check as enabling pam for a user without it being enabled in the org does nothing. I'll leave it up to AC to decide if it's worth keeping. It would eventually be replaced by the billing seat logic.

@Hinton
Hinton force-pushed the pam/enable-pam-flows branch from 708e586 to 43281c3 Compare August 7, 2026 11:11
The member-management flows that grant and revoke AccessPam, the admin-controlled
per-member PAM access grant.

Both paths are plain field writes behind an org-level UsePam check. PAM has no
seats, so there is deliberately no analogue of the Secrets Manager pattern here —
no CountNewSmSeatsRequiredQuery, no UpdateSecretsManagerSubscriptionCommand, and
no autoscale-last ordering rule to respect.

The UsePam guard replaces SM's, which is not explicit in the member flows at all:
SM leans on CountNewSmSeatsRequiredQuery throwing "Organization does not use
Secrets Manager" as a side effect of counting seats. With no seat count to
piggyback on, PAM checks UsePam directly. AccessPam = true on a non-PAM org would
otherwise be inert but unvalidated, since claim emission ANDs AccessPam with
UsePam (PM-40209) — the guard exists so admins get an actionable error instead of
a silently ineffective toggle.

Only the grant is gated. Revoking access is not checked against UsePam, so
removing a member's access stays possible on an organization whose PAM
entitlement has lapsed. Editing a member who already holds access is likewise not
a grant and is not blocked.

The single-user path is implemented on both sides of the ChangeMemberEmailNoMp
flag, so the toggle behaves the same whichever command serves the request:

- v1 threads AccessPam through OrganizationUserUpdateRequestModel.ToOrganizationUser
  and guards the transition alongside the other validation.
- v2 adds NewAccessPam to UpdateOrganizationUserRequest with an IsEnablingPam()
  transition helper, applies it via UpdateOrganizationUser, and returns a typed
  PamNotEnabled error from the validator.

The bulk path is a new PUT organizations/{orgId}/users/enable-pam mirroring
BulkEnableSecretsManagerAsync minus the seat block. SM's obsolete PATCH alias is
not replicated, and like SM's bulk path it logs no events (breakdown review
note 5). The generic OrganizationUser_Updated event still covers the single-user
path.

UpdateOrganizationUser gains an accessPam parameter; its only production caller is
the v2 command.
@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 11:36
@Hinton
Hinton requested a review from a team as a code owner August 7, 2026 11:36
@Hinton
Hinton requested a review from BTreston August 7, 2026 11:36
@Hinton
Hinton force-pushed the pam/enable-pam-flows branch from 43281c3 to b937e2f Compare August 7, 2026 11:36
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