[PM-41861] Add Ability to Invited Staged Users - #8283
Conversation
sven-bitwarden
added
ai-review-vnext
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Re-reviewed after the switch to a bulk response model. The earlier Two findings remain, both in the seat-rollback paths rather than the invite logic itself. Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| foreach (var organizationUser in organizationUsers) | ||
| { | ||
| organizationUser.Status = OrganizationUserStatusType.Invited; | ||
| // The update stored procedure persists whatever RevisionDate the entity carries, so bump it here or | ||
| // the row's watermark stays at its staged-creation timestamp and watermark-driven consumers miss the | ||
| // change. | ||
| organizationUser.RevisionDate = revisionDate; | ||
| await organizationUserRepository.ReplaceAsync(organizationUser); | ||
| } |
There was a problem hiding this comment.
🎨 SUGGESTED: ReplaceManyAsync would collapse this loop (and the revert loop) into one round trip and make the batch update atomic.
Details and fix
RunAsync accepts an arbitrary list of ids, so this issues one OrganizationUser_Update round trip per member, and the revert loop at lines 97-102 issues another. IOrganizationUserRepository.ReplaceManyAsync sends the whole set to [dbo].[OrganizationUser_UpdateMany] in a single call; ConfirmOrganizationUserCommand.cs:177 already uses it for the same bulk status-change shape.
foreach (var organizationUser in organizationUsers)
{
organizationUser.Status = OrganizationUserStatusType.Invited;
organizationUser.RevisionDate = revisionDate;
}
await organizationUserRepository.ReplaceManyAsync(organizationUsers);Beyond the N→1 round trips, it also closes a partial-failure gap: this loop sits outside the try, so if ReplaceAsync throws on member 3 of 5, members 1-2 are left Invited with no invitation email sent and no revert path, while seats were already added. A single UPDATE ... FROM OPENJSON cannot land half-applied.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8283 +/- ##
==========================================
+ Coverage 63.66% 69.55% +5.89%
==========================================
Files 2430 2473 +43
Lines 104962 106145 +1183
Branches 9506 9620 +114
==========================================
+ Hits 66822 73829 +7007
+ Misses 35844 29877 -5967
- Partials 2296 2439 +143 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return new OrganizationUserNotStaged(); | ||
| } | ||
|
|
||
| var seatReservationError = await ReserveSeatsAsync(organization, organizationUsers.Count); |
There was a problem hiding this comment.
❓ QUESTION: Should this path also account for Secrets Manager seats, the way the invite-dialog path does?
Details
OrganizationUser_ReadOccupiedSmSeatCountByOrganizationId counts Status IN (0, 1, 2) and AccessSecretsManager = 1, so a staged member holding SM access occupies no SM seat until this command flips them to Invited — the same reason ReserveSeatsAsync exists for PM seats.
Staged members can be pre-configured before anyone invites them (this PR's own test Invite_WhenTheDialogSelectsNoAccess_LeavesTheStagedMembersExistingAccessAlone relies on that), and UpdateOrganizationUserCommand will happily set AccessSecretsManager on a staged row. Because TryEnablingSecretsManagerAsync sizes against an occupancy count that excludes staged rows, enabling SM on two staged members can each see the same free seat, and promoting both here pushes the org past SmSeats with no autoscale.
SaveUsersSendInvitesAsync was changed in this PR to count staged promotions toward inviteWithSmAccessCount for exactly this reason; is the row action intentionally left out, or is SM access on staged members considered out of scope for now?
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| // Directory Connector key off both, and only the fields the invite specifies are overwritten. | ||
| foreach (var (orgUser, invite) in stagedInvitations.Values) | ||
| { | ||
| orgUser.Type = invite.Type.Value; |
| // Staged users' changes are handled separately to avoid unnecessary conditions above | ||
| foreach (var (orgUser, invite) in stagedInvitations.Values) | ||
| { | ||
| if (invite.Collections != null && invite.Collections.Any()) | ||
| { | ||
| await _organizationUserRepository.ReplaceAsync(orgUser, invite.Collections); | ||
| } | ||
| else | ||
| { | ||
| await _organizationUserRepository.ReplaceAsync(orgUser); | ||
| } | ||
|
|
||
| if (invite.Groups != null && invite.Groups.Any()) | ||
| { | ||
| await _organizationUserRepository.UpdateGroupsAsync(orgUser.Id, invite.Groups, revisionDate); | ||
| } | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| // Revert any added users. | ||
| var invitedOrgUserIds = allOrgUsers.Select(ou => ou.Id); | ||
| await _organizationUserRepository.DeleteManyAsync(invitedOrgUserIds); | ||
| // Revert any created/non-staged users. | ||
| await _organizationUserRepository.DeleteManyAsync(createdOrgUsers.Select(ou => ou.Id)); |
There was a problem hiding this comment.
Details and fix
The revert no longer removes the promoted staged rows (correct — they pre-date the call), but it still shrinks Seats back to initialSeatCount. Those two are now inconsistent when the loop fails after at least one ReplaceAsync has committed.
Trace, with an org whose seats are full (availableSeats == 0, which is exactly why AutoAddSeatsAsync ran) and two staged members being invited:
AutoAddSeatsAsyncraisesSeatsfrominitialSeatCounttoinitialSeatCount + 2.SendInvitesAsyncsucceeds — both members are emailed.- Member 1 is replaced as
Invited; member 2'sReplaceAsynchits a transient DB failure (deadlock/timeout). - Catch:
DeleteManyAsyncremoves nothing (createdOrgUsersis empty here). AdjustSeatsAsync(organization, initialSeatCount - currentSeats)reaches its occupancy guard (OrganizationService.cs:249-268).GetOccupiedSeatCountByOrganizationIdAsyncnow counts member 1, becauseStatus IN (0,1,2)includesInvited, soseatCounts.Total == initialSeatCount + 1 > newSeatTotaland it throwsBadRequestException("Your organization currently has N seats filled. Remove some users.").
Because that throw happens before exceptions.Add(e) on line 735, the original exception is discarded and the caller sees the misleading "Remove some users" 400 instead. Seats also stays at the autoscaled value, since the revert never completed.
Before this change the revert deleted every row in allOrgUsers, so occupancy always returned to its starting value and this branch was unreachable.
Restoring the promoted rows in the catch keeps occupancy consistent with the seat revert:
catch (Exception e)
{
// Revert any created/non-staged users.
await _organizationUserRepository.DeleteManyAsync(createdOrgUsers.Select(ou => ou.Id));
// Put any staged rows already promoted above back the way their provisioning tool left them.
var promotedStagedUsers = stagedInvitations.Values
.Select(s => s.OrgUser)
.Where(ou => ou.Status == OrganizationUserStatusType.Invited)
.ToList();
if (promotedStagedUsers.Count > 0)
{
foreach (var orgUser in promotedStagedUsers)
{
orgUser.Status = OrganizationUserStatusType.Staged;
}
await _organizationUserRepository.ReplaceManyAsync(promotedStagedUsers);
}
var currentOrganization = await _organizationRepository.GetByIdAsync(organization.Id);
// ...
}Alternatively, wrapping the seat revert so a failure there cannot displace e would at least preserve the real error for the caller and the logs.
| var seatReservationError = await ReserveSeatsAsync(organization, eligible.Count); | ||
| if (seatReservationError is not null) | ||
| { | ||
| return seatReservationError; | ||
| } | ||
|
|
||
| // Password Manager seats first: the subscription rejects more Secrets Manager seats than it has PM seats. | ||
| var secretsManagerSeatReservationError = await ReserveSecretsManagerSeatsAsync(organization, eligible); | ||
| if (secretsManagerSeatReservationError is not null) | ||
| { | ||
| return secretsManagerSeatReservationError; | ||
| } |
There was a problem hiding this comment.
Details and fix
ReserveSeatsAsync is not a dry run — AutoAddSeatsAsync → AdjustSeatsAsync charges the gateway and persists the new Seats (OrganizationService.cs:291-296). When ReserveSecretsManagerSeatsAsync then returns an error, RunAsync returns straight to the caller: no member is promoted, the admin gets a 400, and the organization keeps paying for the seats.
Trace, for an org with UseSecretsManager, no PM headroom, and two staged members carrying AccessSecretsManager:
ReserveSeatsAsyncautoscales PM seats by 2 — Stripe is charged,Organization.Seatsis written.ReserveSecretsManagerSeatsAsynccallsUpdateSubscriptionAsync, which throwsBadRequestException(SM autoscale cap, orMaxAutoscaleSmSeatsreached).- Line 63 returns
SecretsManagerSeatExpansionFailed→ 400.InviteAsyncnever runs.
Net result: +2 PM seats billed, 0 members invited, and the error message only mentions Secrets Manager. Retrying after the admin raises the SM cap buys 2 more PM seats on top, since ReserveSeatsAsync recomputes from occupancy that never moved.
The invite-dialog path added in this same PR does not have this hole — SaveUsersSendInvitesAsync unwinds the autoscale in its catch (OrganizationService.cs:729-733). The distinction that justifies keeping seats on a send failure (members were promoted, the admin can retry without paying twice) does not hold here, because nothing was promoted.
Rolling the PM reservation back before returning keeps billing consistent with the all-or-nothing result this path already advertises:
var secretsManagerSeatReservationError = await ReserveSecretsManagerSeatsAsync(organization, eligible);
if (secretsManagerSeatReservationError is not null)
{
await ReleaseSeatsAsync(organization, reservedSeats);
return secretsManagerSeatReservationError;
}Alternatively, reserving SM seats only after InviteAsync succeeds would at least mean the seats that were bought correspond to members who were actually invited.
| @@ -0,0 +1,19 @@ | |||
| CREATE PROCEDURE [dbo].[OrganizationUser_ReadByOrganizationIdEmails] | |||
There was a problem hiding this comment.
⛏️ I think the stored proc name should probably be OrganizationUser_ReadManyByOrganizationIdEmails
2 participants
🎟️ Tracking
PM-41861
📔 Objective
We need the ability to invite staged users with the same configuration capabilities as regular/net-new users. This PR surgically modifies OrganizationService to do so, while providing a new API entrypoint to invite for the dedicated row action.
Because this PR modifies OrganizationService, I have added quite a few integration tests on this behavior.
Proof
Screen.Recording.2026-08-31.at.10.51.22.AM.mov