From fae4d3ca1b06336b74b0c1ca71bf41e283b56803 Mon Sep 17 00:00:00 2001 From: Conner Turnbull <133619638+cturnbull-bitwarden@users.noreply.github.com> Date: Mon, 28 Aug 2023 09:22:07 -0400 Subject: [PATCH] [AC-1571] Handle null reference exception in stripe controller (#3147) * Added null checks when getting customer metadata * Added additional logging around paypal payments * Refactor region validation in StripeController * Update region retrieval method in StripeController Refactored the method GetCustomerRegionFromMetadata in StripeController. Previously, it returned null in case of nonexisting region key. Now, it checks all keys with case-insensitive comparison, and if no "region" key is found, it defaults to "US". This was done to handle cases where the region key might not be properly formatted or missing. * Updated switch expression to be switch statement * Updated new log to not log user input --- src/Billing/Controllers/StripeController.cs | 163 +++++++++++++------- 1 file changed, 106 insertions(+), 57 deletions(-) diff --git a/src/Billing/Controllers/StripeController.cs b/src/Billing/Controllers/StripeController.cs index 88f3f9e8a3..4a15541a17 100644 --- a/src/Billing/Controllers/StripeController.cs +++ b/src/Billing/Controllers/StripeController.cs @@ -10,12 +10,18 @@ using Bit.Core.Tools.Enums; using Bit.Core.Tools.Models.Business; using Bit.Core.Tools.Services; using Bit.Core.Utilities; +using Braintree; +using Braintree.Exceptions; using Microsoft.AspNetCore.Mvc; using Microsoft.Data.SqlClient; using Microsoft.Extensions.Options; using Stripe; +using Customer = Stripe.Customer; using Event = Stripe.Event; +using Subscription = Stripe.Subscription; using TaxRate = Bit.Core.Entities.TaxRate; +using Transaction = Bit.Core.Entities.Transaction; +using TransactionType = Bit.Core.Enums.TransactionType; namespace Bit.Billing.Controllers; @@ -490,60 +496,87 @@ public class StripeController : Controller /// private async Task ValidateCloudRegionAsync(Event parsedEvent) { - string customerRegion; - var serverRegion = _globalSettings.BaseServiceUri.CloudRegion; var eventType = parsedEvent.Type; + var expandOptions = new List { "customer" }; - switch (eventType) + try { - case HandledStripeWebhook.SubscriptionDeleted: - case HandledStripeWebhook.SubscriptionUpdated: - { - var subscription = await GetSubscriptionAsync(parsedEvent, true, new List { "customer" }); - customerRegion = GetCustomerRegionFromMetadata(subscription.Customer.Metadata); + Dictionary customerMetadata; + switch (eventType) + { + case HandledStripeWebhook.SubscriptionDeleted: + case HandledStripeWebhook.SubscriptionUpdated: + customerMetadata = (await GetSubscriptionAsync(parsedEvent, true, expandOptions))?.Customer + ?.Metadata; break; - } - case HandledStripeWebhook.ChargeSucceeded: - case HandledStripeWebhook.ChargeRefunded: - { - var charge = await GetChargeAsync(parsedEvent, true, new List { "customer" }); - customerRegion = GetCustomerRegionFromMetadata(charge.Customer.Metadata); + case HandledStripeWebhook.ChargeSucceeded: + case HandledStripeWebhook.ChargeRefunded: + customerMetadata = (await GetChargeAsync(parsedEvent, true, expandOptions))?.Customer?.Metadata; break; - } - case HandledStripeWebhook.UpcomingInvoice: - var eventInvoice = await GetInvoiceAsync(parsedEvent); - var customer = await GetCustomerAsync(eventInvoice.CustomerId); - customerRegion = GetCustomerRegionFromMetadata(customer.Metadata); - break; - case HandledStripeWebhook.PaymentSucceeded: - case HandledStripeWebhook.PaymentFailed: - case HandledStripeWebhook.InvoiceCreated: - { - var invoice = await GetInvoiceAsync(parsedEvent, true, new List { "customer" }); - customerRegion = GetCustomerRegionFromMetadata(invoice.Customer.Metadata); + case HandledStripeWebhook.UpcomingInvoice: + customerMetadata = (await GetInvoiceAsync(parsedEvent))?.Customer?.Metadata; break; - } - default: - { - // For all Stripe events that we're not listening to, just return 200 - return false; - } - } + case HandledStripeWebhook.PaymentSucceeded: + case HandledStripeWebhook.PaymentFailed: + case HandledStripeWebhook.InvoiceCreated: + customerMetadata = (await GetInvoiceAsync(parsedEvent, true, expandOptions))?.Customer?.Metadata; + break; + default: + customerMetadata = null; + break; + } - return customerRegion == serverRegion; + if (customerMetadata is null) + { + return false; + } + + var customerRegion = GetCustomerRegionFromMetadata(customerMetadata); + + return customerRegion == serverRegion; + } + catch (Exception e) + { + _logger.LogError(e, "Encountered unexpected error while validating cloud region"); + throw; + } } /// - /// Gets the region from the customer metadata. If no region is present, defaults to "US" + /// Gets the customer's region from the metadata. /// - /// - /// - private static string GetCustomerRegionFromMetadata(Dictionary customerMetadata) + /// The metadata of the customer. + /// The region of the customer. If the region is not specified, it returns "US", if metadata is null, + /// it returns null. It is case insensitive. + private static string GetCustomerRegionFromMetadata(IDictionary customerMetadata) { - return customerMetadata.TryGetValue("region", out var value) - ? value - : "US"; + const string defaultRegion = "US"; + + if (customerMetadata is null) + { + return null; + } + + if (customerMetadata.TryGetValue("region", out var value)) + { + return value; + } + + var miscasedRegionKey = customerMetadata.Keys + .FirstOrDefault(key => + key.Equals("region", StringComparison.OrdinalIgnoreCase)); + + if (miscasedRegionKey is null) + { + return defaultRegion; + } + + _ = customerMetadata.TryGetValue(miscasedRegionKey, out var regionValue); + + return !string.IsNullOrWhiteSpace(regionValue) + ? regionValue + : defaultRegion; } private Tuple GetIdsFromMetaData(IDictionary metaData) @@ -708,8 +741,11 @@ public class StripeController : Controller private async Task AttemptToPayInvoiceWithBraintreeAsync(Invoice invoice, Customer customer) { + _logger.LogDebug("Attempting to pay invoice with Braintree"); if (!customer?.Metadata?.ContainsKey("btCustomerId") ?? true) { + _logger.LogWarning( + "Attempted to pay invoice with Braintree but btCustomerId wasn't on Stripe customer metadata"); return false; } @@ -718,6 +754,8 @@ public class StripeController : Controller var ids = GetIdsFromMetaData(subscription?.Metadata); if (!ids.Item1.HasValue && !ids.Item2.HasValue) { + _logger.LogWarning( + "Attempted to pay invoice with Braintree but Stripe subscription metadata didn't contain either a organizationId or userId"); return false; } @@ -740,25 +778,36 @@ public class StripeController : Controller return false; } - var transactionResult = await _btGateway.Transaction.SaleAsync( - new Braintree.TransactionRequest - { - Amount = btInvoiceAmount, - CustomerId = customer.Metadata["btCustomerId"], - Options = new Braintree.TransactionOptionsRequest + Result transactionResult; + try + { + transactionResult = await _btGateway.Transaction.SaleAsync( + new Braintree.TransactionRequest { - SubmitForSettlement = true, - PayPal = new Braintree.TransactionOptionsPayPalRequest + Amount = btInvoiceAmount, + CustomerId = customer.Metadata["btCustomerId"], + Options = new Braintree.TransactionOptionsRequest { - CustomField = $"{btObjIdField}:{btObjId},region:{_globalSettings.BaseServiceUri.CloudRegion}" + SubmitForSettlement = true, + PayPal = new Braintree.TransactionOptionsPayPalRequest + { + CustomField = + $"{btObjIdField}:{btObjId},region:{_globalSettings.BaseServiceUri.CloudRegion}" + } + }, + CustomFields = new Dictionary + { + [btObjIdField] = btObjId.ToString(), + ["region"] = _globalSettings.BaseServiceUri.CloudRegion } - }, - CustomFields = new Dictionary - { - [btObjIdField] = btObjId.ToString(), - ["region"] = _globalSettings.BaseServiceUri.CloudRegion - } - }); + }); + } + catch (NotFoundException e) + { + _logger.LogError(e, + "Attempted to make a payment with Braintree, but customer did not exist for the given btCustomerId present on the Stripe metadata"); + throw; + } if (!transactionResult.IsSuccess()) {