Repository navigation
Add SSO ExternalCallback integration tests for Privileged Controls seats - #8822
Conversation
Cover the Privileged Controls seat reservation that #8812 adds to the SSO AccountController, for both the just-in-time and existing-user Staged promotion paths. Add SsoTestDataBuilder opt-ins for a self-hosted environment, a substituted Stripe adapter, and a substituted pricing client backed by MockPlans, so no test calls an external service. Move AssertNoSeatsAvailableRedirect to a shared ExternalCallbackAssertions helper.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR only adds tests. It adds 14 SSO |
JaredSnider-Bitwarden
added
ai-review
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## billing/PM-45107/autoscale-pam-seats-on-access-grant #8822 +/- ##
========================================================================================
- Coverage 68.63% 0 -68.64%
========================================================================================
Files 2765 0 -2765
Lines 118581 0 -118581
Branches 10826 0 -10826
========================================================================================
- Hits 81385 0 -81385
+ Misses 34643 0 -34643
+ Partials 2553 0 -2553 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kdenney
left a comment
kdenney
left a comment
There was a problem hiding this comment.
Looks good, thanks again for these. Only required change is the feature flag mock.
Everything from the pr description's expected results table looks correct to me except for the one technicality nitpick comment below.
| * the organization has a free Privileged Controls seat. The member keeps access, no seat is bought, and the | ||
| * Owners get no email. | ||
| */ | ||
| [Fact] |
There was a problem hiding this comment.
FYI: #8812 now has a PM39883_Pam_Billing_Support check in ReservePrivilegedControlsSeatsCommand. On cloud with the flag off, the reservation succeeds right away: the member keeps access, and nothing is bought or emailed. Self-hosted is unaffected, because that branch runs before the flag check.
| var testData = await new SsoTestDataBuilder() | ||
| .WithSsoConfig() | ||
| .WithMockedPricingClient() | ||
| .WithOrganization(org => WithPrivilegedControlsSeats(org, pamSeats: 1, maxAutoscalePamSeats: 1)) |
There was a problem hiding this comment.
This is a technicality nitpick, but FYI: with one seat bought, none in use and a limit of 1, this member brings the total to exactly the autoscale limit. In that case AutoscalePrivilegedControlsSeatsCommand.RunAsync sends the Owners PrivilegedControlsSeatLimitReachedMail. That's intended: Owners are told when seats in use land exactly on the limit, whether or not a seat was bought but that email was added in a previous branch. So the "Nothing bought or emailed" wording in the description table for this row isn't exactly accurate.
If you want, you could increase coverage by asserting that the limit-reached email is sent here. If you'd rather keep this test about the free-seat case alone, raising maxAutoscalePamSeats to 2 (or null) would also make "nothing emailed" true.
The same applies to the existing-user version at line 292.
| * seat and the member keeps access. | ||
| * | ||
| * The organization has no subscription id, so the autoscale treats it as a sales-initiated trial and raises | ||
| * the seat count without a Stripe call. |
There was a problem hiding this comment.
Non-blocking: this is the only test that adds a seat, and it goes down the no-subscription path, so Stripe is never called. Is it worth it to add another test that uses the WithMockedStripeAdapter helper so that's covered?
| * state. The Owners get no seat-needed email, because the member did not join. | ||
| */ | ||
| [Fact] | ||
| public async Task ExternalCallback_ExistingUserAgainstStagedOrgUserWithPrivilegedControls_AtSeatLimit_WhenInviteSendFails_RestoresPrivilegedControlsAndDoesNotEmail() |
There was a problem hiding this comment.
Non-blocking: might be worth adding another test here to increase coverage where the invite send fails after autoscale already bought a seat. The expected result would be that the member goes back to Staged and keeps AccessPam, and the bought Privileged Controls seat stays bought, which matches how this path already keeps the Password Manager seat.
🎟️ Tracking
No ticket. Stacked on #8812. Merge this PR into that branch.
📔 Objective
Add integration tests for the Privileged Controls seat reservation that #8812 adds to the SSO
AccountController. The reservation runs when an SSO sign-in moves a Staged member withAccessPamto Invited.Please confirm the expected results
I wrote these tests from the code, not from a spec, so each Expected result below is what the code does today. Please flag any row that is not what you intend. These rows are the least obvious:
GatewaySubscriptionIdgets a seat added without a Stripe call.What the tests cover
New file
AccountControllerExternalCallbackPrivilegedControlsSeatTests(14 tests). "JIT" is a new user that SSO provisions just in time. "Existing" is a user with a Bitwarden account, who gets an invite email.AccessPamAccessPamrestored. No email.Test infrastructure
SsoTestDataBuilderopt-ins:WithSelfHostedEnvironment(),WithMockedStripeAdapter(...), andWithMockedPricingClient(). No test calls Stripe or the pricing service.Sso.IntegrationTestnow referencesCore.TestforMockPlans, asIdentity.IntegrationTestdoes.AssertNoSeatsAvailableRedirectmoved to a sharedExternalCallbackAssertionshelper.Verification
AccountControllerlines, 14 killed. The survivor is equivalent (possibleOrgUser.Email ?? newUser.Email; the values are always equal in the test data).📸 Screenshots
N/A