[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 the full change end to end. The seat arithmetic in One new finding: the invite-dialog revert path is no longer self-consistent now that promoted staged rows are excluded from the delete but seats are still rolled back. Code Review Details
|
![claude[bot]](https://cdn.statically.io/img/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.45% +5.79%
==========================================
Files 2430 2470 +40
Lines 104962 105892 +930
Branches 9506 9571 +65
==========================================
+ Hits 66822 73552 +6730
+ Misses 35844 29905 -5939
- Partials 2296 2435 +139 ☔ 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://cdn.statically.io/img/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.
1 participant
🎟️ 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