[PM-27553] Resolve premium purchase for user with account credit that used payment method

[amorask-bitwarden]

🎟️ Tracking

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

📔 Objective

If a user previously purchased account credit, they'll have a Stripe customer. If they then try to purchase a premium subscription using an actual payment method, our code will see they have a customer and assume they already have a payment method as well.

This resolves that by checking for this scenario and updating the customer's payment method in that case.

📸 Screenshots

Screen.Recording.2025-10-29.at.7.35.40.AM.mov

⏰ 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

Summary of Changes

This PR fixes a billing issue where users who previously purchased account credit (creating a Stripe customer without a payment method) encounter an error when trying to purchase premium using an actual payment method. The fix adds logic to detect this scenario and update the customer's payment method before creating the subscription.

Key Changes:

1. PaymentMethod.cs: Added convenience properties AsTokenized and AsNonTokenized for easier access to underlying types
2. CreatePremiumCloudHostedSubscriptionCommand.cs: Added new logic to handle existing customers without payment methods
3. Tests: Added comprehensive test coverage for the new scenario

---

Critical Issues

1. Race Condition in Payment Method Check

Location: CreatePremiumCloudHostedSubscriptionCommand.cs:98-101

else if (paymentMethod.IsTokenized && !await hasPaymentMethodQuery.Run(user))
{
    await updatePaymentMethodCommand.Run(user, paymentMethod.AsTokenized, billingAddress);
    customer = await subscriberService.GetCustomerOrThrow(user, new CustomerGetOptions { Expand = _expand });
}

There's a TOCTOU (Time-of-Check-Time-of-Use) race condition here. Between checking hasPaymentMethodQuery.Run(user) and calling updatePaymentMethodCommand.Run(), another process could update the payment method. While unlikely in practice, this could cause issues in concurrent scenarios.

Suggestion: Consider checking the result of updatePaymentMethodCommand.Run() to ensure the payment method was actually set, or make the check-and-update atomic.

---

2. Missing Null Reference Check

Location: CreatePremiumCloudHostedSubscriptionCommand.cs:98

The code calls !await hasPaymentMethodQuery.Run(user) but hasPaymentMethodQuery could theoretically return unexpected results if the user's GatewayCustomerId exists but the customer was deleted in Stripe. The HasPaymentMethodQuery.Run() method calls subscriberService.GetCustomer() which may return null, but then continues to check properties.

Suggestion: Review error handling in HasPaymentMethodQuery to ensure it gracefully handles deleted Stripe customers.

---

3. Inconsistent Error Handling for NonTokenized Payment Methods

Location: CreatePremiumCloudHostedSubscriptionCommand.cs:88-106

The code path for NonTokenized payment methods (like AccountCredit) with an existing customer (line 104-106) doesn't validate that the customer has sufficient credit or that the billing address is valid before attempting subscription creation. This could lead to cryptic Stripe errors downstream.

Verification needed: Does the existing customer validation in ReconcileBillingLocationAsync handle this adequately?

---

Suggested Improvements

4. Code Clarity - Switch Statement Refactoring

Location: CreatePremiumCloudHostedSubscriptionCommand.cs:112-138

The nested Switch with early returns and the underscore discard pattern (_) makes the logic hard to follow. The non-tokenized branch uses an inverted condition (if (subscription.Status != SubscriptionStatus.Active)) followed by a return, which is less clear than the tokenized branch.

Suggested refactoring

paymentMethod.Switch(
    tokenized => HandleTokenizedPaymentMethod(tokenized, subscription, user),
    nonTokenized => HandleNonTokenizedPaymentMethod(nonTokenized, subscription, user)
);

// Extract to private methods for clarity
private void HandleTokenizedPaymentMethod(TokenizedPaymentMethod tokenized, Subscription subscription, User user)
{
    var shouldSetPremium = tokenized.Type switch
    {
        TokenizablePaymentMethodType.PayPal => subscription.Status == SubscriptionStatus.Incomplete,
        _ => subscription.Status == SubscriptionStatus.Active
    };
    
    if (shouldSetPremium)
    {
        user.Premium = true;
        user.PremiumExpirationDate = subscription.GetCurrentPeriodEnd();
    }
}

private void HandleNonTokenizedPaymentMethod(NonTokenizedPaymentMethod nonTokenized, Subscription subscription, User user)
{
    if (subscription.Status == SubscriptionStatus.Active)
    {
        user.Premium = true;
        user.PremiumExpirationDate = subscription.GetCurrentPeriodEnd();
    }
}

---

5. Test Coverage Gap - NonTokenized Payment Method Edge Case

Location: Test file line 699-722

The test Run_NonTokenizedPaymentWithoutExistingCustomer_ThrowsBillingException correctly tests the error case, but there's no test for the scenario where:

• User has existing GatewayCustomerId
• Payment method is NonTokenized (AccountCredit)
• User previously had a tokenized payment method that was removed
• Billing address needs reconciliation

This is a legitimate scenario that should be covered.

---

6. Potential Billing Address Validation Issue

Location: CreatePremiumCloudHostedSubscriptionCommand.cs:271-302

The ReconcileBillingLocationAsync method only updates the address if both Country and PostalCode are missing or empty. However, it doesn't validate that the provided billingAddress parameter has these fields. If the incoming billing address is also incomplete, the tax calculation could fail with an unhelpful error.

Suggestion: Add validation of the incoming billingAddress before attempting to update the customer:

if (string.IsNullOrEmpty(billingAddress.Country) || string.IsNullOrEmpty(billingAddress.PostalCode))
{
    _logger.LogError("Cannot reconcile billing location for customer without valid billing address");
    throw new BillingException("Valid billing address required for subscription");
}

---

7. Checkmarx CSRF Finding - False Positive

Location: AccountBillingVNextController.cs:72

The Checkmarx report flagged a CSRF vulnerability at line 72. This appears to be a false positive because:

1. The controller has [Authorize("Application")] at line 16, requiring authentication
2. ASP.NET Core's default antiforgery middleware should be active
3. The endpoint uses [HttpPost] and accepts [FromBody], which are automatically protected

However, verify that:

• Antiforgery tokens are properly configured in the application startup
• The [Authorize("Application")] policy is correctly implemented
• API endpoints follow the security patterns documented in the security whitepaper

---

8. Documentation - Missing XML Comments

Location: PaymentMethod.cs:14-16

The newly added properties lack XML documentation comments:

public TokenizedPaymentMethod AsTokenized => AsT0;
public NonTokenizedPaymentMethod AsNonTokenized => AsT1;

Suggestion: Add XML comments explaining when to use these vs the type checks:

/// <summary>
/// Gets the tokenized payment method. Only call after verifying IsTokenized is true.
/// </summary>
public TokenizedPaymentMethod AsTokenized => AsT0;

---

9. Logging Enhancement

Location: CreatePremiumCloudHostedSubscriptionCommand.cs:98-101

The new code path (updating payment method for existing customer) doesn't log this scenario. For debugging and monitoring purposes, it would be helpful to log when this specific edge case occurs.

Suggestion:

else if (paymentMethod.IsTokenized && !await hasPaymentMethodQuery.Run(user))
{
    _logger.LogInformation("User ({UserId}) has existing customer but no payment method. Updating payment method before subscription creation", user.Id);
    await updatePaymentMethodCommand.Run(user, paymentMethod.AsTokenized, billingAddress);
    customer = await subscriberService.GetCustomerOrThrow(user, new CustomerGetOptions { Expand = _expand });
}

Note: Ensure the log message doesn't expose PII - the user ID should be sufficient.

---

Good Practices Observed

• Comprehensive test coverage for the new scenario (line 372-438 in test file)
• Clear comments explaining the business logic (lines 85-97)
• Proper use of dependency injection for new dependencies
• OneOf pattern usage for type-safe payment method handling
• Proper null checking and validation
• Test naming follows clear conventions
• Billing exceptions are properly propagated

---

Action Items

For the Author:

1. High Priority: Review and address the race condition concern (Identity Server  #1)
2. High Priority: Add billing address validation before reconciliation (LDAP authentication? #6)
3. Medium Priority: Add logging for the new code path (Subfolders #9)
4. Medium Priority: Consider refactoring the Switch statement for clarity (Licence #4)
5. Low Priority: Add XML documentation for new properties (Add a Gitter chat badge to README.md #8)
6. Low Priority: Add test coverage for the edge case mentioned in Email 2-step Authentification #5

Security Review:

• Verify Checkmarx CSRF finding is indeed a false positive (Email for Logging In is Case Sensitive #7)
• Confirm error messages don't leak sensitive information

Before Merging:

• Ensure CI passes (currently passing)
• Code coverage: 73.47% on patch (13 lines missing) - consider adding tests for uncovered lines
• Run integration tests if available

---

[github-actions]

Checkmarx One – Scan Summary & Details – b815212d-a5a2-4755-9c4c-491f2e738f2d

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.87%. Comparing base (5f0e038) to head (38263e7).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...nds/CreatePremiumCloudHostedSubscriptionCommand.cs	75.92%	11 Missing and 2 partials ⚠️
src/Core/Billing/Payment/Models/PaymentMethod.cs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6514      +/-   ##
==========================================
+ Coverage   51.86%   51.87%   +0.01%     
==========================================
  Files        1901     1901              
  Lines       84051    84073      +22     
  Branches     7501     7500       -1     
==========================================
+ Hits        43594    43615      +21     
  Misses      38763    38763              
- Partials     1694     1695       +1     

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

[sbrown-livefront]

✅ Solid update on the PaymentMethod model. Makes it so much cleaner.

[amorask-bitwarden]

@sbrown-livefront pointed out here that we should make the same change in the PremiumUserBillingService, which is the old path for purchasing premium. I'm choosing to forego this because the work that will enable this new purchase path via the command would be released at the same time as this fix, rendering it moot.

[cyprain-okeke]

Looks Nice

[cyprain-okeke]

I noticed we're using Customer? customer but there's no null validation after assignment. While GetCustomerOrThrow guarantees non-null returns, CreateCustomerAsync calls the Stripe SDK which could theoretically return null in edge
cases. can we null check for customer

// Add this defensive check
  if (customer == null)
  {
      _logger.LogError(
          "Customer is unexpectedly null after creation/retrieval. UserId: {UserId}, GatewayCustomerId: {GatewayCustomerId}",
          user.Id,
          user.GatewayCustomerId ?? "none"
      );
      throw new BillingException("Customer is unexpectedly null after creation/retrieval");
  }

[amorask-bitwarden]

I noticed we're using Customer? customer but there's no null validation after assignment. While GetCustomerOrThrow guarantees non-null returns, CreateCustomerAsync calls the Stripe SDK which could theoretically return null in edge cases. can we null check for customer

// Add this defensive check
  if (customer == null)
  {
      _logger.LogError(
          "Customer is unexpectedly null after creation/retrieval. UserId: {UserId}, GatewayCustomerId: {GatewayCustomerId}",
          user.Id,
          user.GatewayCustomerId ?? "none"
      );
      throw new BillingException("Customer is unexpectedly null after creation/retrieval");
  }

@cyprain-okeke There's no case where Stripe's Create Customer operation would result in a null. The Stripe SDK would throw a StripeException in the case the request was invalid or some other networking error took place.