[PM-29224] Remove unused billing endpoints and code paths

[amorask-bitwarden]

🎟️ Tracking

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

📔 Objective

This PR cleans up our API contract by removing endpoints that are not invoked by the platform and some supporting methods.

📸 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

[claude]

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

---

Code Review Complete

Tasks

• Launch bitwarden-code-reviewer agent to analyze PR changes
• Review findings and provide comprehensive feedback
• Post final review with recommendations

---

Overall Assessment: REQUEST CHANGES ⚠️

This PR removes 2,056 lines of billing code including entire controllers, endpoints, and supporting methods. While the cleanup aligns with the VNext billing architecture migration, there are critical concerns about breaking changes and incomplete migration documentation.

---

Critical Findings

🚨 Finding 1: Breaking Change - LicensesController Deletion

Issue: The deleted LicensesController contained endpoints used by self-hosted installations for license synchronization:

• GET /licenses/user/{id} - Retrieve user licenses
• GET /licenses/organization/{id} - Sync organization licenses

These endpoints used the Licensing authorization policy (server-to-server communication), different from the standard Application policy used by client applications.

Impact:

• Self-hosted installations currently syncing licenses will receive 404 errors after this deploys
• The replacement SelfHostedOrganizationLicensesController uses different routes (/organizations/licenses/self-hosted) and authorization (Application vs Licensing)
• This is a breaking API change requiring coordinated deployment
• The old endpoints were GET requests, new endpoints are POST - different semantics

Required Actions:

1. Document the migration path from old to new endpoints in the PR description
2. Verify no current self-hosted versions depend on these endpoints via usage metrics
3. Add API version deprecation notices if gradual migration is needed
4. Update any client code or documentation referencing these endpoints

---

📝 Finding 2: Incomplete Migration Documentation

Issue: Multiple TODO comments indicate intended migration to VNext controllers, but no comprehensive migration guide exists:

// TODO: Migrate to Query / AccountBillingVNextController
// TODO: Migrate to Command / OrganizationBillingVNextController  
// TODO: Remove when pm-24996-implement-upgrade-from-free-dialog is removed

Questions Needing Answers:

1. Are all removed endpoints truly unused, or are they being migrated to VNext?
2. Were API usage metrics analyzed to confirm zero usage?
3. What is the timeline for completing the TODO migrations?
4. Are there any client applications (web vault, mobile, desktop, CLI) still calling these endpoints?

The PR objective states endpoints are "not invoked by the platform," but this needs verification, especially for the license sync endpoints.

---

🔍 Finding 3: Tax and Payment Method Endpoint Removals

Removed Endpoints:

• Account tax info: GET/PUT /accounts/billing/{id}/tax
• Organization tax info: GET/PUT /organizations/{id}/tax, GET/PUT /organizations/{id}/billing/tax-information
• Provider tax info: GET/PUT /providers/{id}/billing/tax-information
• Payment methods: GET/PUT */payment-method
• Bank verification: POST */verify-bank-account
• Invoice preview: POST /accounts/billing/preview-invoice

Status: VNext replacements exist (GetPaymentMethodQuery, UpdatePaymentMethodCommand found in codebase at /account/billing/vnext and /organizations/{id}/billing/vnext routes), but:

• No explicit mapping documented from old routes to new routes
• Clients may need updates to use new endpoints
• Potential breaking change if any external integrations or older clients exist

---

Positive Observations ✅

1. Clean Service Layer Cleanup: All deleted request/response models have no remaining references - excellent cleanup work
2. Appropriate Renames: VNext controller renames (SelfHostedAccountBillingController → SelfHostedAccountBillingVNextController, SelfHostedBillingController → SelfHostedOrganizationBillingVNextController) improve naming consistency
3. Test Removal: Associated tests properly removed for deleted code
4. VNext Migration: Changes align with modern CQRS pattern (Commands/Queries architecture)
5. No Dead Code: Verified no lingering references to deleted models or methods

---

Questions Requiring Clarification ❓

Question 1: API Usage Verification

How was it determined that these endpoints are "not invoked by the platform"?

• Were production server logs analyzed for the deleted routes?
• Were all client codebases (web, mobile, desktop, CLI) searched for references?
• Is there usage telemetry data showing zero calls over a meaningful time period?

This is especially critical for the LicensesController which was designed for self-hosted server-to-cloud license validation.

Question 2: Self-Hosted License Migration Strategy

For the deleted license sync endpoints (/licenses/user/{id}, /licenses/organization/{id}):

• What is the migration path for existing self-hosted installations?
• Do self-hosted servers need updates before cloud deploys this change?
• Is there backward compatibility concern with older self-hosted versions (e.g., v2023.x trying to sync with v2025.x cloud)?
• Were these endpoints actually unused, or is this an intentional breaking change with a migration plan?

Question 3: Codecov 0% Patch Coverage

The codecov report shows 0% coverage on 2 lines in the renamed VNext controllers. These are just class declaration lines from the renames - is this acceptable given it's not new logic?

---

Technical Debt Considerations 🌱

1. nullable directive removal: The renamed VNext controllers had #nullable enable removed. While not breaking, this reduces type safety. Per ADR-0024, Bitwarden uses nullable reference types. Consider keeping it enabled.

2. TODO proliferation: Multiple TODO comments added without Jira tracking. These should be captured in tickets to ensure follow-through on the VNext migrations.

3. Feature flag dependencies: Several endpoints marked for removal "when feature flag X is removed" (e.g., pm-24996-implement-upgrade-from-free-dialog). Ensure feature flag cleanup is tracked.

---

Security Considerations 🔒

No security regressions identified:

• Payment and tax endpoints removed have VNext replacements with equivalent authorization
• Authorization checks maintained in VNext controllers
• No sensitive data exposure in the changes
• No PII logging concerns

Checkmarx reported one fixed CSRF issue (unrelated to this PR's changes).

---

Recommendations

Before Merge (Required):

1. Document breaking changes - Add to PR description:

   • License endpoint migration guide (or confirmation they're truly unused)
   • Tax/payment endpoint route mapping (old → new VNext routes)
   • Authorization policy changes (Licensing → Application)

2. Verify usage via metrics - Provide evidence that deleted endpoints receive zero production traffic over the last 30-90 days

3. Self-hosted communication plan - If license endpoints were used, notify self-hosted administrators of the change with migration instructions

After Merge (Recommended):

1. Complete TODO migrations - Create Jira tickets for all TODO comments to track VNext migration completion
2. Monitor for 404s - Watch production logs for calls to deleted endpoints in the first week post-deploy
3. Update API documentation - Ensure OpenAPI specs and public API docs reflect the removals and new VNext routes

---

Codecov Analysis

The 0% patch coverage on 2 lines is acceptable:

• Both lines are class declarations in renamed files
• No actual logic changes in those files
• This appears to be a codecov artifact from the git rename operation

---

Review Verdict

REQUEST CHANGES ⚠️

The code quality is excellent and the cleanup is well-executed. However, the breaking changes for self-hosted license synchronization require:

1. Verification that the deleted license endpoints are truly unused (via metrics), OR
2. Migration documentation explaining how self-hosted installations should adapt

The PR objective states endpoints are "not invoked by the platform," but the LicensesController was specifically designed for self-hosted-to-cloud server communication using the Licensing authorization policy. This needs explicit verification or a documented migration strategy before merge.

Once clarified, this PR will be ready to approve. The technical implementation is sound.

---

Files Reviewed: All 24 changed files analyzed

---

[github-actions]

Checkmarx One – Scan Summary & Details – d6c17dfb-21c3-445b-ade1-e80281666a12

Fixed Issues (1)

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

Severity	Issue	Source File / Package
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 207

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.88%. Comparing base (acc2529) to head (dc1959a).

Files with missing lines	Patch %	Lines
...s/VNext/SelfHostedAccountBillingVNextController.cs	0.00%	1 Missing ⚠️
...xt/SelfHostedOrganizationBillingVNextController.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6692      +/-   ##
==========================================
+ Coverage   53.63%   53.88%   +0.24%     
==========================================
  Files        1926     1916      -10     
  Lines       85717    84978     -739     
  Branches     7686     7600      -86     
==========================================
- Hits        45978    45790     -188     
+ Misses      37966    37426     -540     
+ Partials     1773     1762      -11     

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

[cyprain-okeke]

LGTM have confirmed that clients are migrated and self-hosted license sync has a replacement.