Skip to content

[PM-39210] fix: Block converting a Secrets-Manager-enabled org to a Business Unit Portal#7892

Open
cyprain-okeke wants to merge 4 commits into
mainfrom
billing/pm-39210/block-converting-a-secrets-manager-enabled-org-to-a-business-unit-portal
Open

[PM-39210] fix: Block converting a Secrets-Manager-enabled org to a Business Unit Portal#7892
cyprain-okeke wants to merge 4 commits into
mainfrom
billing/pm-39210/block-converting-a-secrets-manager-enabled-org-to-a-business-unit-portal

Conversation

@cyprain-okeke

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

Converting an organization to a Business Unit Portal (BUP) is the only existing-org provider-attach path that is both unguarded against Secrets Manager and billing-destructive. BusinessUnitConverter gated only on Enterprise tier (the tier that carries SM) and never checked UseSecretsManager, so an SM-enabled org could be converted — after which the Password Manager line is swapped to the BUP price and the org's GatewaySubscriptionId is nulled, orphaning the Secrets Manager subscription line. The org could no longer self-serve its SM seat count.

This adds a UseSecretsManager guard to both validators in BusinessUnitConverter, mirroring the existing precedent in ProviderService (ProviderService.cs:447-451):

  • ValidateInitiationAsync — appends a user-facing problem string ("...Please contact Customer Support to convert this organization to a business unit.") to the returned problems list, so the Admin operator sees an actionable message and no provider is created.
  • ValidateFinalizationAsync — fails via the file's existing log-and-throw Fail() convention (BillingException), as defense-in-depth.

Both guards are placed before the Stripe GetSubscription call, so a disqualified org short-circuits without a wasted round-trip and well before any destructive mutation.

Tests (BusinessUnitConverterTests.cs):

  • InitiateConversion_OrganizationUsesSecretsManager_ReturnsError — SM-on Enterprise org is blocked; asserts no provider is created.
  • FinalizeConversion_OrganizationUsesSecretsManager_ThrowsBillingException — asserts the throw and that GetSubscription is never called (proving the short-circuit).
  • Extended InitiateConversion_ValidationFails_ReturnsErrors to set UseSecretsManager = true on a non-Enterprise org, proving the SM gate is independent of the tier check (the accumulating path).
  • Hardened the two _Succeeds_ tests with UseSecretsManager = false to avoid BitAutoData flakiness now that the gate exists.

Open question for Billing: the guard keys off the UseSecretsManager flag (matching the ProviderService precedent), so it blocks any SM-enabled org even if there is no actively-billed SM line on the Stripe subscription. This is the safe default; if conversion should be permitted for orgs that have the flag set without a billable SM line, that is a follow-up scoping decision.

…usiness Unit Portal

BusinessUnitConverter gated org-to-Business-Unit conversion only on Enterprise tier and never checked UseSecretsManager, so a Secrets-Manager-enabled org could be converted. The Password Manager line was then swapped to the Business Unit price and the org's GatewaySubscriptionId nulled, orphaning the Secrets Manager subscription line.

Add a UseSecretsManager guard to both validators, mirroring ProviderService, placed before the Stripe GetSubscription call so a disqualified org short-circuits. ValidateInitiationAsync appends a user-facing problem; ValidateFinalizationAsync fails via the existing log-and-throw convention.
@cyprain-okeke cyprain-okeke added the ai-review Request a Claude code review label Jun 30, 2026
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed a defensive billing change that blocks converting a Secrets-Manager-enabled organization to a Business Unit Portal. The change adds a UseSecretsManager guard to both validators in BusinessUnitConverter and hides the Convert-to-Business-Unit option in the Admin Edit view. The guards correctly short-circuit before the Stripe GetSubscription call and mirror the established precedent in ProviderService.cs. Tests cover both the finalization throw path and the accumulating-problems initiation path, and harden the existing success tests.

Code Review Details

No findings. The logic is correct: Fail() is [DoesNotReturn] and throws BillingException, so the finalization guard short-circuits before any destructive mutation; ValidateInitiationAsync accumulates a user-facing problem string independent of the tier check. Both paths match the confirmed ProviderService.cs:447 precedent, and the Razor view guard provides consistent UI-side defense.

@cyprain-okeke cyprain-okeke added the t:feature Change Type - Feature Development label Jun 30, 2026
…manager-enabled-org-to-a-business-unit-portal
@cyprain-okeke cyprain-okeke marked this pull request as ready for review June 30, 2026 11:36
@cyprain-okeke cyprain-okeke requested a review from a team as a code owner June 30, 2026 11:36
@cyprain-okeke cyprain-okeke requested a review from kdenney June 30, 2026 11:36
@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.25%. Comparing base (11130a3) to head (582c64f).

Files with missing lines Patch % Lines
...Admin/AdminConsole/Views/Organizations/Edit.cshtml 0.00% 2 Missing ⚠️
...illing/Providers/Services/BusinessUnitConverter.cs 87.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7892   +/-   ##
=======================================
  Coverage   61.25%   61.25%           
=======================================
  Files        2222     2222           
  Lines       98174    98183    +9     
  Branches     8867     8869    +2     
=======================================
+ Hits        60137    60145    +8     
- Misses      35918    35919    +1     
  Partials     2119     2119           

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

@amorask-bitwarden amorask-bitwarden left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These are both valuable validations, but we should prevent the option from showing in the Admin Portal at all if the organization uses Secrets Manager in order to avoid confusion for the end user.

@cyprain-okeke cyprain-okeke requested a review from a team as a code owner June 30, 2026 14:55

@amorask-bitwarden amorask-bitwarden left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good - thanks for making that change.

…manager-enabled-org-to-a-business-unit-portal
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants