From 7cc873a62adb08be25be547a219b0170c9611cca Mon Sep 17 00:00:00 2001
From: Wilfred Asomani <devwilfredagyei@gmail.com>
Date: Tue, 18 Jul 2023 13:13:04 +0000
Subject: [PATCH] satellite/payments: prevent removing other users' cards

This change patches a loophole that allowed accounts to remove cards
that belong to other users.

Closes #storj/storj-private#326

Change-Id: I33e9efe5c9cdb03aa48ad4c6b1d3283c396a7890
---
 satellite/payments/stripe/creditcards.go      | 31 ++++++++++++++++++-
 satellite/payments/stripe/creditcards_test.go | 25 ++++++++++-----
 satellite/payments/stripe/stripemock.go       |  9 ++++++
 3 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/satellite/payments/stripe/creditcards.go b/satellite/payments/stripe/creditcards.go
index 2064b4768..f4f4dbabe 100644
--- a/satellite/payments/stripe/creditcards.go
+++ b/satellite/payments/stripe/creditcards.go
@@ -13,6 +13,13 @@ import (
 	"storj.io/storj/satellite/payments"
 )
 
+var (
+	// ErrCardNotFound is returned when card is not found for a user.
+	ErrCardNotFound = errs.Class("card not found")
+	// ErrDefaultCard is returned when a user tries to delete their default card.
+	ErrDefaultCard = errs.Class("default card")
+)
+
 // creditCards is an implementation of payments.CreditCards.
 //
 // architecture: Service
@@ -156,7 +163,29 @@ func (creditCards *creditCards) Remove(ctx context.Context, userID uuid.UUID, ca
 	if customer.InvoiceSettings != nil &&
 		customer.InvoiceSettings.DefaultPaymentMethod != nil &&
 		customer.InvoiceSettings.DefaultPaymentMethod.ID == cardID {
-		return Error.Wrap(errs.New("can not detach default payment method."))
+		return ErrDefaultCard.New("can not detach default payment method.")
+	}
+
+	cardIter := creditCards.service.stripeClient.PaymentMethods().List(&stripe.PaymentMethodListParams{
+		ListParams: stripe.ListParams{Context: ctx},
+		Customer:   &customerID,
+		Type:       stripe.String(string(stripe.PaymentMethodTypeCard)),
+	})
+
+	isUserCard := false
+	for cardIter.Next() {
+		if cardIter.PaymentMethod().ID == cardID {
+			isUserCard = true
+			break
+		}
+	}
+
+	if err = cardIter.Err(); err != nil {
+		return Error.Wrap(err)
+	}
+
+	if !isUserCard {
+		return ErrCardNotFound.New("this card is not attached to this account.")
 	}
 
 	cardParams := &stripe.PaymentMethodDetachParams{Params: stripe.Params{Context: ctx}}
diff --git a/satellite/payments/stripe/creditcards_test.go b/satellite/payments/stripe/creditcards_test.go
index abf27b73c..1016d70c2 100644
--- a/satellite/payments/stripe/creditcards_test.go
+++ b/satellite/payments/stripe/creditcards_test.go
@@ -87,31 +87,42 @@ func TestCreditCards_Add(t *testing.T) {
 
 func TestCreditCards_Remove(t *testing.T) {
 	testplanet.Run(t, testplanet.Config{
-		SatelliteCount: 1, StorageNodeCount: 0, UplinkCount: 1,
+		SatelliteCount: 1, StorageNodeCount: 0, UplinkCount: 2,
 	}, func(t *testing.T, ctx *testcontext.Context, planet *testplanet.Planet) {
 		satellite := planet.Satellites[0]
 		userID := planet.Uplinks[0].Projects[0].Owner.ID
+		user2ID := planet.Uplinks[1].Projects[0].Owner.ID
 
-		_, err := satellite.API.Payments.Accounts.CreditCards().Add(ctx, userID, "test")
+		card1, err := satellite.API.Payments.Accounts.CreditCards().Add(ctx, userID, "test")
 		require.NoError(t, err)
 
-		_, err = satellite.API.Payments.Accounts.CreditCards().Add(ctx, userID, "test2")
+		// card2 becomes the default card
+		card2, err := satellite.API.Payments.Accounts.CreditCards().Add(ctx, userID, "test2")
 		require.NoError(t, err)
 
 		cards, err := satellite.API.Payments.Accounts.CreditCards().List(ctx, userID)
 		require.NoError(t, err)
 		require.Len(t, cards, 2)
 
-		// Save card that should remain after deletion.
-		savedCard := cards[0]
+		// user2ID should not be able to delete userID's cards
+		for _, card := range cards {
+			err = satellite.API.Payments.Accounts.CreditCards().Remove(ctx, user2ID, card.ID)
+			require.Error(t, err)
+			require.True(t, stripe.ErrCardNotFound.Has(err))
+		}
 
-		err = satellite.API.Payments.Accounts.CreditCards().Remove(ctx, userID, cards[1].ID)
+		// Can not remove default card
+		err = satellite.API.Payments.Accounts.CreditCards().Remove(ctx, userID, card2.ID)
+		require.Error(t, err)
+		require.True(t, stripe.ErrDefaultCard.Has(err))
+
+		err = satellite.API.Payments.Accounts.CreditCards().Remove(ctx, userID, card1.ID)
 		require.NoError(t, err)
 
 		cards, err = satellite.API.Payments.Accounts.CreditCards().List(ctx, userID)
 		require.NoError(t, err)
 		require.Len(t, cards, 1)
-		require.Equal(t, savedCard, cards[0])
+		require.Equal(t, card2, cards[0])
 	})
 }
 
diff --git a/satellite/payments/stripe/stripemock.go b/satellite/payments/stripe/stripemock.go
index 186b3c682..9940b06f9 100644
--- a/satellite/payments/stripe/stripemock.go
+++ b/satellite/payments/stripe/stripemock.go
@@ -356,6 +356,15 @@ func (m *mockCustomers) Update(id string, params *stripe.CustomerParams) (*strip
 	if params.Balance != nil {
 		customer.Balance = *params.Balance
 	}
+	if params.InvoiceSettings != nil {
+		if params.InvoiceSettings.DefaultPaymentMethod != nil {
+			customer.InvoiceSettings = &stripe.CustomerInvoiceSettings{
+				DefaultPaymentMethod: &stripe.PaymentMethod{
+					ID: *params.InvoiceSettings.DefaultPaymentMethod,
+				},
+			}
+		}
+	}
 	// TODO update customer with more params as necessary
 
 	return customer, nil