Repository navigation
Conversation
…ed trials PM-40118 specified that trials add seats on the organization only; the branch was deferred until trial detection existed. The command now asks the sales-initiated trial query when seats are short and within the limit, and on a trial raises PamSeats without a change set or Stripe update. The empty GatewaySubscriptionId guard is removed because the trial query treats a missing subscription id as a trial. Its test, which expected "No subscription found.", is replaced by Run_NoSubscriptionId_TreatedAsTrial_SavesSeats. The library now exposes internals to DynamicProxyGenAssembly2 so tests can substitute the internal trial query interface.
…trols seats Inviting staged members already gives back Password Manager and Secrets Manager seats when the invite fails, and needs the same for Privileged Controls.
…ry host The organization user commands every host registers will depend on the autoscale step, and hosts validate DI at startup, so each host registers the library's implementation as the library README requires. A new AddPrivilegedControlsSeatAutoscaling method registers the autoscale and revert commands with their trial query and seat change set factory; AddOrganizationSubscriptions calls it, so Api is unaffected.
…s for admin actions Admin actions that grant Privileged Controls access (editing a member, bulk activate, restore, staged invite) all need the same seat decision. This command centralizes it: cloud organizations autoscale up to their limit, self-hosted organizations report a shortfall against the license, and organizations without a seat limit are unaffected. Callers map the outcome to their own errors.
…lper The static helper class shared its name with the PrivilegedControlsSeats namespace, so callers under the OrganizationUsers namespace resolved the simple name to the namespace and needed an alias. Renaming it to PrivilegedControlsSeatOccupancy removes the clash.
…d Controls seat Self-service joins (SSO, invite links) and SCIM restores cannot be blocked by a Privileged Controls seat limit. This command reserves a seat for the joining member, and when none is available it removes the member's Privileged Controls access in memory and emails the Owners so they can raise the limit. The caller saves the member in its existing save.
…ves a member access Editing a member to grant Privileged Controls access previously only blocked self-hosted organizations that were short on seats, and cloud organizations were never checked. Route the grant through the shared seat reservation command so cloud organizations autoscale up to their limit and report a clear error when the limit is reached. Revoked and staged members do not occupy a seat, so no seat is reserved for them.
…ting members Bulk activation previously only blocked self-hosted organizations that were short on seats, and cloud organizations were never checked. Route the grant through the shared seat reservation command so cloud organizations autoscale up to their limit and nothing is saved when the limit is reached. Only members who occupy a seat are counted. Revoked and staged members still receive access without a seat.
…embers Restoring a revoked member who has Privileged Controls access returns them to a seat, so the seat has to be reserved first. Admin single and bulk restores now reserve seats and fail with the seat limit message when none can be added. SCIM restores cannot fail the request, so the member is restored without Privileged Controls access and the Owners are told. Public API restores are unchanged.
…aged members Staged members who already have Privileged Controls access start using a seat once they are invited, so the invite has to reserve those seats. At the limit the whole invite fails before any Password Manager seats are bought. If a later step fails, the Privileged Controls seats bought in this request are given back, the same way the command already gives back Password Manager and Secrets Manager seats.
…leged Controls at the seat limit A staged member can already hold Privileged Controls access before joining. Joining by invite link now reserves a Privileged Controls seat for that member. When none can be reserved the member still joins, without Privileged Controls access, and the Owners are emailed, instead of the join being blocked at the seat limit.
…leged Controls at the seat limit A staged member can already hold Privileged Controls access before their first SSO sign-in. Promoting that member to Invited now reserves a Privileged Controls seat. When none can be reserved the member still signs in, without Privileged Controls access, and the Owners are emailed, instead of the sign-in being blocked at the seat limit. A failure while reserving the seat is reported as no seats available, like the existing seat check. If the invite email fails and the promotion is reverted, the member's Privileged Controls access is restored too.
…the SSO user For a staged member signing in through SSO for the first time, the Privileged Controls seat was reserved after the new user was created. A failure there left an orphaned user row and sent a welcome email for a sign-in that could not finish. The seat is now reserved up front, next to the existing seat check, so a failure stops the sign-in before any user is created. The same member row is still saved later, which persists the removed Privileged Controls access.
…d members fails unexpectedly Privileged Controls seats are reserved before the Password Manager and Secrets Manager reservations, but those only convert known failures into error results. Any other exception skipped the release and left the seats bought for an invite that never happened. Unexpected exceptions now release the Privileged Controls seats and rethrow.
…anization has no seats Billing's autoscale command rejects an organization with fewer than one Privileged Controls seat, so reserving for such an organization surfaced as a seat limit failure. An organization with zero seats is treated like one without a seat limit, the same as a null seat count.
…ror next to the other seat errors The error is raised by restore, staged invite, bulk activate and edit, so it belongs with the shared Privileged Controls seat errors rather than in the edit-member namespace. The message is unchanged.
…t a subscription An organization with no Stripe subscription would otherwise get Privileged Controls seats raised without anyone being billed. The tests fake the trial query the way the other Subscriptions.Organization tests stand in for internal types, so the library no longer exposes its internals to the mocking library.
… only after the member joins Sending the email while reserving the seat could tell Owners about a join that later failed, and send it again on a retry. Self-hosted organizations get no email, matching the existing seat limit emails, and a member whose seat reservation fails now joins without Privileged Controls instead of being turned away.
…rols seats when inviting staged members When one action adds both a Password Manager seat and a Privileged Controls seat, the Password Manager seat comes first. A Privileged Controls failure gives back the Password Manager and Secrets Manager seats just added, and a billing failure is logged and returned as an error like the other seat steps.
…for edit member and invite links An admin granting access at the limit gets the limit error, and a staged member joining by invite link at the limit joins without Privileged Controls.
…ocked on Privileged Controls seats An admin restore buys any Password Manager seats it needs before reserving Privileged Controls seats. When the Privileged Controls reservation hits the limit, finds a self-hosted shortfall, or throws, the restore is abandoned, so the Password Manager seats it just bought are handed back instead of being billed for members who were never restored. A failure to hand them back is logged rather than hiding the original error.
…otes a staged member with access Edit member and bulk activate can give a staged member access without taking a seat, on the understanding that the seat is taken once they become Invited. An email invite whose address matches that staged member promoted them to Invited without reserving anything, so an admin could put members past the autoscale limit unbilled. The Admin Console invite endpoint and Directory Connector import both take this path. The invite now reserves the seats after the Password Manager and Secrets Manager seats are bought and before any invite is sent, fails at the limit like the other admin actions, and gives the Privileged Controls seats back in its existing failure handling. OrganizationService now depends on the reserve and revert commands, and the autoscale and revert commands used to save through IOrganizationService, which would have been a dependency loop. Saving moves into IOrganizationReplaceCommand, which OrganizationService.ReplaceAndUpdateCacheAsync forwards to, and the two seat commands depend on that instead. The reserve commands move into Subscriptions.Organization next to the autoscale and revert commands, registered by AddPrivilegedControlsSeatAutoscaling, with their contracts left in Core for Admin Console to call. They check for self-hosting through IBitwardenEnvironment, like the autoscale command.
…seats must be added Every cloud grant of Privileged Controls access now goes through the autoscale step, including grants the organization's bought seats already cover. Rejecting an organization without a Stripe subscription before checking whether any seats were needed blocked those admin grants and stripped access from joining members for no reason. The check now runs only when seats must be added, so an organization without billing still never gets seats it isn't charged for.
…ls while inviting staged members Inviting staged members buys Password Manager and Secrets Manager seats before reserving Privileged Controls seats. Only a bad request or gateway failure from that reservation handed the earlier seats back, so a ConflictException, which the subscription update raises when the subscription is unpaid or has a conflicting schedule, left those seats billed for members who were never invited. Any failure from the reservation now hands back every seat the invite added before the error propagates.
…eature folders The commands folder had grown to cover three unrelated areas: invoice previews, Privileged Controls seats, and the new-subscription purchase. Each now lives in its own folder with a matching namespace. The invoice preview folder is named InvoicePreviews, matching Bit.Invoicing.InvoicePreviews, because a namespace named InvoicePreview hides the InvoicePreview result type the preview commands return.
…ures as errors The reserve command now returns a result carrying the Admin Console error instead of an enum, and catches its own failures so callers no longer need try/catch around it. Business failures map to a 400 and anything unexpected maps to a 500, with the exception attached for callers that throw. An organization with no Privileged Controls seat count still skips the check, while an organization with zero seats is treated as having none free. The joining-member wrapper is removed: self-service callers clear access themselves based on the result, and the Owner email moves to its own command. Seat contracts, errors and helpers move under Core's Billing namespace, and the organization replace command is renamed to describe that it also refreshes the cache.
…ts at the paths they cover The self-hosted bulk activate test flipped the settings flag, but Billing reads self-hosted from the environment, so the request took the cloud path. The staged invite autoscale tests sent a placeholder subscription id to the real Stripe adapter, which can't find it. Both now stub the dependency the code actually reads.
…nset until it runs
…ount Admin restore now records the Password Manager seat count before buying seats and gives back the difference when the Privileged Controls seats are blocked, instead of recomputing how many seats the purchase added. The method that reserves and fails is renamed to say it throws, and comments explain which restores reserve seats and why bulk restore counts only revoked members.
…heck the limit first The reserve and autoscale commands take the number of members about to start using a seat, which the old names read as the number of seats to buy. Both parameters are now seatsRequested, and the autoscale and revert commands use RunAsync to match the other Privileged Controls seat commands. The autoscale command now checks the limit before the subscription, so an organization without a subscription that is over its limit gets the limit-reached result. Admins see the ask-an-Owner error, and self-service joins still email the Owners.
… seat-needed email Design review signed off on revised subject and body copy for the email sent to owners when a member can't be given a Privileged Controls seat.
…jects Design review asked for both Privileged Controls seat emails to end with the same sentence, and for each subject to follow its email's heading so owners who receive both can tell them apart.
connerbw
left a comment
There was a problem hiding this comment.
Looks good overall! Just one comment/question
…ubscription id An organization can have Privileged Controls enabled with no Stripe subscription id, as when an admin enables a sales-initiated trial. The trial query already treats a missing subscription id as a trial and skips Stripe, so the autoscale command no longer rejects these organizations up front.
|
AC has some underlying passions to disassemble OrganizationService - would it be tenable to replace the callsites of Other than that - I messaged you about it but we now have 3 kinds of seats to check for, PM/SM/PAM. Not in this PR but for sure we will need some kinda abstraction around the concept of "I am an organization, I wish to reserve X/Y/Z seats across A/B/C products, and if it fails, need to rollback" |
There was a problem hiding this comment.
Requesting changes for two reasons.
1. Integration tests. The SSO AccountController changes need integration tests for the ExternalCallback flows they change. I put them up in #8822, which targets this branch. Please review it, confirm the expected results it lists, and merge it in. I've reached out to auth to get a prompt review.
2. No kill switch. Nothing on the SSO reservation path checks a feature flag. Here is what I checked in the server repo (code, SQL, and migrations) and the clients repo:
UsePamis not behind a flag. The Admin portal edit action sets it with only theOrg_Plan_Editpermission, and the checkbox shows whenPM39883_Pam_Billing_Supportis off. No client sets it.AccessPamis behindpm-37044-pam-v-0in the web client only. The edit-member checkbox and the bulk "Activate Privileged Controls" action needusePamand that flag. On the server,PUT organizations/{orgId}/users/{id}andPUT enable-pamcheck no flag. They only require the organization to haveUsePam.PamSeatsis the real gate. On cloud, every path that sets it is behindPM39883_Pam_Billing_Support. So today the new SSO code does nothing until that flag has been on.- Turning off either flag hides the UI, but it does not change organizations that already have
PamSeatsor members that already haveAccessPam. For those, SSO still autoscales seats (a Stripe subscription update, unless on a sales-initiated trial), removesAccessPam, and emails Owners.
Should the two reservations in AccountController check PM39883_Pam_Billing_Support, so the flag can act as a kill switch?
Question: if autoscale buys a seat and a later step fails (the invite send, or user registration), the controller keeps the seat. The restore command and the invite-Staged-members command on this branch give the seat back. Is the SSO behavior intended? This mirrors @sven-bitwarden 's comment here.
…eAsync with its command Moves every caller onto IReplaceOrganizationAndUpdateCacheCommand and deletes the service method, so saving an organization and refreshing its cached abilities has one implementation. This also retires the private copy in UpdateSecretsManagerSubscriptionCommand, which existed only to avoid a circular dependency on OrganizationService that the command does not have.
| if (privilegedControlsSeatReservation?.Error is PrivilegedControlsSeatLimitReachedError) | ||
| { | ||
| await _sendPrivilegedControlsSeatNeededEmailCommand.RunAsync( | ||
| organization, possibleOrgUser.Email ?? newUser.Email); | ||
| } |
There was a problem hiding this comment.
❓ : @ike-kottlowski asked a great question here. Why does the _reservePrivilegedControlsSeatsCommand.RunAsync not own the error handling of calling _sendPrivilegedControlsSeatNeededEmailCommand.RunAsync?
There was a problem hiding this comment.
If we do change behavior here, then #8822 will need to be updated as well. Feel free to modify those tests.
There was a problem hiding this comment.
If the reserve command sent the email, it would go out before the member is saved, which is handled by the calling code. For some SSO paths, steps after the reservation can still fail and roll the join back. The Owners would then be emailed about a member who never took the seat, and again on every retry. Sending the email from the caller after the save avoids that. It's the same reason the Password Manager autoscale limit email goes out only after the seats are saved.
…ling support flag Gives PM39883_Pam_Billing_Support a kill switch over every member action that reserves Privileged Controls seats. With the flag off, cloud reservations succeed without buying seats, so members get access the way they did before seat autoscaling: no subscription change, no rollback, no seat-needed email, and no access removed at the seat limit. Self-hosted keeps rejecting a seat shortfall regardless of the flag, since self-hosted flags read off and that check predates autoscaling.
…scale-pam-seats-on-access-grant
Thanks for this pr!
Yes, good call. I've added a check for this flag in the reserve command in 7162305 Note that your above PR with integration tests will need changes to mock this feature flag. I'll comment over there as well.
Yes, this is intended. The ticket's AC doesn't specify what should happen to an autoscaled seat when a later step fails, but as I pointed out in my other comment, I decided to match the behavior of the password manager seat autoscaling in the SSO paths. But if there is a reason to question that decision, please let me know! |
Done in bbe50b4 |
Pattern match good Co-authored-by: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com>
…ls seat integration tests Seat reservations now skip seat checks on cloud when PM39883_Pam_Billing_Support is off, so the bulk enable and invite link integration tests that assert autoscaling and seat-limit behavior need the flag on to exercise it.
🎟️ Tracking
PM-45107 (parent epic PM-39883)
📔 Objective
Whenever a member with Privileged Controls access starts using a seat, the server now reserves that seat first. A member uses a seat when
AccessPamis enabled and they are Invited, Accepted or Confirmed. That happens either when an admin enablesAccessPamfor a member already in one of those statuses, or when a member withAccessPamenabled is invited, restored or joins. If the organization needs more seats, the reservation calls Billing's autoscale step (IAutoscalePrivilegedControlsSeatsCommand, from #8633), which buys seats up to the organization's autoscale limit.Admin actions fail when the seat limit is reached
AccessPamenabled: editing a member, bulk activating members, restoring revoked members (single and bulk), inviting staged members, and inviting by email when the email matches a staged member.OrganizationUseris saved.Self-service joins and SCIM restores remove access when no seat can be reserved
AccessPamenabled: SSO sign-in by a user matching a staged member (existing or new user), accepting an invite link, confirming through an invite link, and restoring a member through SCIM.AccessPamset to false. No error is returned to the member or to SCIM.PrivilegedControlsSeatNeededMail) once the member is saved.Seats are given back when a later step fails
Changes to the autoscale step
PamSeatsis updated, with no change in Stripe.Supporting changes
Bit.Core.Billing.Organizations.Commands) and their implementations inSubscriptions.Organization:IReservePrivilegedControlsSeatsCommand,IRevertPrivilegedControlsSeatsCommandandISendPrivilegedControlsSeatNeededEmailCommand.AddPrivilegedControlsSeatAutoscaling()registers these commands. The Admin, Billing, Identity, SCIM and SSO hosts now call it, and the Api host gets it throughAddOrganizationSubscriptions().IReplaceOrganizationAndUpdateCacheCommandtakes over the body ofOrganizationService.ReplaceAndUpdateCacheAsync. The autoscale and revert commands save through it, which letsOrganizationServicedepend on them without a circular dependency.PrivilegedControlsErrors.csinBit.Core.Billing.Organizations, and the existing Secrets Manager errors file is renamed toSecretsManagerErrors.cs.Subscriptions.Organizationcommands are grouped intoInvoicePreviews,PrivilegedControlsandPurchasefolders.No client changes are needed, because the web vault already shows these 400 errors. Design has approved both the new email and the changes to the existing PAC autoscale email.