[PM-24549] Remove feature flag: use-pricing-service

[amorask-bitwarden]

🎟️ Tracking

📔 Objective

This PR removes the use-pricing-service feature flag and completes the migration to using the pricing service as the single source of truth for plan information. It refactors how plans are stored and accessed throughout the codebase, separating production code from test mocks.

Changes

🏗️ Architecture & Refactoring

StaticStore Cleanup

• Removed plan storage logic from StaticStore (~55 lines removed)
• Moved test plan implementations from StaticStore to new MockPlans class
• Created dedicated SponsoredPlans class for production use

Plan Model Reorganization

• Moved all plan classes to test/Core.Test/Billing/Mocks/Plans/ directory:
• Created new test/Core.Test/Billing/Mocks/MockPlans.cs to centralize test plan access

Sponsored Plans

• Created new src/Core/Billing/Models/SponsoredPlans.cs for production code
• Updated references from StaticStore.SponsoredPlans to SponsoredPlans.All or SponsoredPlans.Get()

⚙️ Configuration Updates

Added pricingUri to Development appsettings for local development and integration tests:

• src/Admin/appsettings.Development.json
• src/Api/appsettings.Development.json
• src/Billing/appsettings.Development.json
• bitwarden_license/src/Scim/appsettings.Development.json
• bitwarden_license/src/Sso/appsettings.Development.json
• bitwarden_license/test/Scim.IntegrationTest/appsettings.Development.json (new file)

All configured to use: https://billingpricing.qa.bitwarden.pw

🧪 Test Updates (77 files)

Updated all test files to use the new mock structure:

• Changed StaticStore.GetPlan() → MockPlans.Get()
• Changed StaticStore.GetSponsoredPlan() → SponsoredPlans.Get()
• Updated imports to reference new Bit.Core.Test.Billing.Mocks namespace

Breaking Changes

None - this is an internal refactoring with no API or behavior changes.

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – a8b4917a-0307-424a-9528-4ccd3bb3f50b

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1539	details Method at line 1539 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: xCngxm4GZdUjjDPdghS9Gi%2BxmNY%3D Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1410	details Method at line 1410 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: %2FbyZ%2FGAj%2BJiinX%2B7FN2WsUxS2bM%3D Attack Vector

Fixed Issues (7)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 351
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 342
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 342
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 300
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 284
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 284
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 207

[claude]

Claude finished @amorask-bitwarden's task —— View job

---

Code Review Complete

This PR successfully removes the use-pricing-service feature flag and completes the migration to using the pricing service. The refactoring cleanly separates test mocks from production code.

---

Summary of Changes

Architecture improvements:

• Removed 55 lines of plan storage logic from StaticStore
• Created SponsoredPlans class for production use (25 lines)
• Created MockPlans class for test use (37 lines)
• Removed feature flag checks from PricingClient (17 lines removed)
• Updated 77 test files to use new mock structure
• Net result: 405 deletions, 366 additions

Configuration:

• Added pricingUri: "https://billingpricing.qa.bitwarden.pw" to Development appsettings across 6 projects

---

Findings

Finding 1: Null-forgiving operator could cause runtime exception in SponsoredPlans.Get()

⚠️ src/Core/Billing/Models/SponsoredPlans.cs:24

The null-forgiving operator (!) masks potential NullReferenceException if an invalid PlanSponsorshipType is passed:

public static SponsoredPlan Get(PlanSponsorshipType planSponsorshipType) =>
    All.FirstOrDefault(p => p.PlanSponsorshipType == planSponsorshipType)!;

While there's currently only one sponsorship type, the API accepts any PlanSponsorshipType enum value. If an invalid value is passed, this returns null but the ! operator tells the compiler it's non-null, leading to potential NullReferenceException downstream.

Recommended fix

public static SponsoredPlan Get(PlanSponsorshipType planSponsorshipType) =>
    All.FirstOrDefault(p => p.PlanSponsorshipType == planSponsorshipType)
    ?? throw new ArgumentException($"Unknown plan sponsorship type: {planSponsorshipType}", nameof(planSponsorshipType));

This makes failures explicit and debuggable rather than mysterious null reference exceptions later.

Note: Author dismissed similar feedback on test code as over-optimization. However, this is production code that could receive invalid enum values from database records, API calls, or future changes.

---

Finding 2: Same issue in test mock MockPlans.Get()

💭 test/Core.Test/Billing/Mocks/MockPlans.cs:36

Test code has the same pattern:

public static Plan Get(PlanType planType) => Plans.SingleOrDefault(p => p.Type == planType)!;

While less critical in tests, explicit failures make test debugging easier. Consider the same fix if tests pass invalid plan types.

---

Finding 3: Code coverage gaps (78.57% patch coverage)

💭 Coverage Report Analysis

6 lines missing coverage across key files:

• StripeEventUtilityService.cs:114 - New SponsoredPlans.All reference (0% coverage)
• SponsorOrganizationSubscriptionUpdate.cs:20-21 - Fallback logic (0% coverage)
• PricingClient.cs:105 - Feature flag check (0% coverage)
• SetUpSponsorshipCommand.cs:53 - Sponsored plan lookup (50% coverage)
• ValidateSponsorshipCommand.cs:99 - Sponsored plan lookup (0% coverage)
• StripePaymentService.cs:70-75 - Sponsored plan logic (50% coverage)

These are all related to the sponsored plan refactoring. Consider adding tests for:

1. IsSponsoredSubscription() method with the new SponsoredPlans.All
2. SponsorOrganizationSubscriptionUpdate fallback behavior when sponsoredPlan is null
3. Sponsorship command paths that retrieve sponsored plans

---

Finding 4: Checkmarx CSRF findings are unrelated to this PR

ℹ️ Security Scan Analysis

The 2 new CSRF findings in CiphersController.cs (lines 1410, 1539) are false positives - CiphersController.cs was not modified in this PR. These likely appeared due to rescanning after base branch changes.

The 7 fixed CSRF issues are also unrelated to this PR's changes.

---

Good Practices Observed

• Clean separation of concerns: test mocks in test/ directory, production code in src/
• Consistent refactoring: all 77 test files updated systematically
• No breaking API changes
• Configuration properly scoped to Development environment
• Net negative lines of code while improving structure

---

Action Items

1. ⚠️ Consider adding explicit null handling in SponsoredPlans.Get() instead of null-forgiving operator
2. 💭 Add tests for uncovered sponsored plan logic paths (optional but recommended for production code)
3. ✅ No changes needed for CSRF findings (unrelated to this PR)

---

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.71%. Comparing base (30ff175) to head (32fc578).

Files with missing lines	Patch %	Lines
...vices/Implementations/StripeEventUtilityService.cs	0.00%	1 Missing ⚠️
...ns/Models/SponsorOrganizationSubscriptionUpdate.cs	0.00%	1 Missing ⚠️
src/Core/Billing/Pricing/PricingClient.cs	0.00%	1 Missing ⚠️
...liesForEnterprise/Cloud/SetUpSponsorshipCommand.cs	50.00%	0 Missing and 1 partial ⚠️
...sForEnterprise/Cloud/ValidateSponsorshipCommand.cs	0.00%	1 Missing ⚠️
...e/Services/Implementations/StripePaymentService.cs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6567      +/-   ##
==========================================
- Coverage   53.04%   52.71%   -0.33%     
==========================================
  Files        1911     1897      -14     
  Lines       85056    84176     -880     
  Branches     7611     7574      -37     
==========================================
- Hits        45117    44375     -742     
+ Misses      38181    38051     -130     
+ Partials     1758     1750       -8     

☔ View full report in Codecov by Sentry.
📢 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.

[eliykat]

A couple of general questions but LGTM from the AC Team side.

[JaredSnider-Bitwarden]

Approving auth file without reviewing others as the only change is a new is dev app setting.