From 594e63f13ae9a176d8677d5191ed457f887c39c6 Mon Sep 17 00:00:00 2001 From: Wilfred Asomani Date: Wed, 4 Oct 2023 09:48:21 +0000 Subject: [PATCH] satellite/accountfreeze: mark billing frozen users for deletion This change modifies the billing freeze chore to set pending deletion status to users who are still frozen after a grace period. It also modifies the chore to skip already deleted users. Issue: https://github.com/storj/storj/issues/6303 https://github.com/storj/storj-private/issues/453 Change-Id: I4d0e7dd904463e99424372dc9ac81b71c0bc6e28 --- satellite/console/accountfreezes.go | 10 ++ satellite/console/accountfreezes_test.go | 13 +++ satellite/payments/accountfreeze/chore.go | 110 +++++++++++++----- .../payments/accountfreeze/chore_test.go | 40 ++++++- scripts/testdata/satellite-config.yaml.lock | 9 +- 5 files changed, 146 insertions(+), 36 deletions(-) diff --git a/satellite/console/accountfreezes.go b/satellite/console/accountfreezes.go index 5f71ec7f5..efcb3e43f 100644 --- a/satellite/console/accountfreezes.go +++ b/satellite/console/accountfreezes.go @@ -276,6 +276,16 @@ func (s *AccountFreezeService) BillingUnfreezeUser(ctx context.Context, userID u return err } + if user.Status == PendingDeletion { + status := Active + err = s.usersDB.Update(ctx, userID, UpdateUserRequest{ + Status: &status, + }) + if err != nil { + return ErrAccountFreeze.Wrap(errs.Combine(ErrFreezeUserStatusUpdate, err)) + } + } + s.tracker.TrackAccountUnfrozen(userID, user.Email) return nil } diff --git a/satellite/console/accountfreezes_test.go b/satellite/console/accountfreezes_test.go index 4aeeb37f3..490aaddba 100644 --- a/satellite/console/accountfreezes_test.go +++ b/satellite/console/accountfreezes_test.go @@ -108,7 +108,20 @@ func TestAccountBillingUnFreeze(t *testing.T) { require.NoError(t, projectsDB.UpdateUsageLimits(ctx, proj.ID, projLimits)) require.NoError(t, service.BillingFreezeUser(ctx, user.ID)) + + status := console.PendingDeletion + err = usersDB.Update(ctx, user.ID, console.UpdateUserRequest{ + Status: &status, + }) + require.NoError(t, err) + user, err = usersDB.Get(ctx, user.ID) + require.NoError(t, err) + require.Equal(t, status, user.Status) + require.NoError(t, service.BillingUnfreezeUser(ctx, user.ID)) + user, err = usersDB.Get(ctx, user.ID) + require.NoError(t, err) + require.Equal(t, console.Active, user.Status) user, err = usersDB.Get(ctx, user.ID) require.NoError(t, err) diff --git a/satellite/payments/accountfreeze/chore.go b/satellite/payments/accountfreeze/chore.go index 0a97f51ec..573f4573a 100644 --- a/satellite/payments/accountfreeze/chore.go +++ b/satellite/payments/accountfreeze/chore.go @@ -29,11 +29,12 @@ var ( // Config contains configurable values for account freeze chore. type Config struct { - Enabled bool `help:"whether to run this chore." default:"false"` - Interval time.Duration `help:"How often to run this chore, which is how often unpaid invoices are checked." default:"24h"` - GracePeriod time.Duration `help:"How long to wait between a warning event and freezing an account." default:"360h"` - PriceThreshold int64 `help:"The failed invoice amount (in cents) beyond which an account will not be frozen" default:"100000"` - ExcludeStorjscan bool `help:"whether to exclude storjscan-paying users from automatic warn/freeze" default:"false"` + Enabled bool `help:"whether to run this chore." default:"false"` + Interval time.Duration `help:"How often to run this chore, which is how often unpaid invoices are checked." default:"24h"` + BillingWarnGracePeriod time.Duration `help:"How long to wait between a billing warning event and billing freezing an account." default:"360h"` + BillingFreezeGracePeriod time.Duration `help:"How long to wait between a billing freeze event and setting pending deletion account status." default:"1440h"` + PriceThreshold int64 `help:"The failed invoice amount (in cents) beyond which an account will not be frozen" default:"100000"` + ExcludeStorjscan bool `help:"whether to exclude storjscan-paying users from automatic warn/freeze" default:"false"` } // Chore is a chore that checks for unpaid invoices and potentially freezes corresponding accounts. @@ -73,15 +74,15 @@ func (chore *Chore) Run(ctx context.Context) (err error) { defer mon.Task()(&ctx)(&err) return chore.Loop.Run(ctx, func(ctx context.Context) (err error) { - chore.attemptFreezeWarn(ctx) + chore.attemptBillingFreezeWarn(ctx) - chore.attemptUnfreezeUnwarn(ctx) + chore.attemptBillingUnfreezeUnwarn(ctx) return nil }) } -func (chore *Chore) attemptFreezeWarn(ctx context.Context) { +func (chore *Chore) attemptBillingFreezeWarn(ctx context.Context) { invoices, err := chore.payments.Invoices().ListFailed(ctx, nil) if err != nil { chore.log.Error("Could not list invoices", zap.Error(Error.Wrap(err))) @@ -90,8 +91,8 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { chore.log.Info("failed invoices found", zap.Int("count", len(invoices))) userMap := make(map[uuid.UUID]struct{}) - frozenMap := make(map[uuid.UUID]struct{}) - warnedMap := make(map[uuid.UUID]struct{}) + billingFrozenMap := make(map[uuid.UUID]struct{}) + billingWarnedMap := make(map[uuid.UUID]struct{}) bypassedLargeMap := make(map[uuid.UUID]struct{}) bypassedTokenMap := make(map[uuid.UUID]struct{}) @@ -114,7 +115,7 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { continue } - debugLog := func(message string) { + infoLog := func(message string) { chore.log.Info(message, zap.String("process", "billing freeze/warn"), zap.String("invoiceID", invoice.ID), @@ -141,12 +142,17 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { continue } + if user.Status == console.Deleted { + errorLog("Ignoring invoice; account already deleted", errs.New("user deleted, but has unpaid invoices")) + continue + } + if invoice.Amount > chore.config.PriceThreshold { if _, ok := bypassedLargeMap[userID]; ok { continue } bypassedLargeMap[userID] = struct{}{} - debugLog("Ignoring invoice; amount exceeds threshold") + infoLog("Ignoring invoice; amount exceeds threshold") chore.analytics.TrackLargeUnpaidInvoice(invoice.ID, userID, user.Email) continue } @@ -169,7 +175,7 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { } if len(cachedPayments) > 0 { bypassedTokenMap[userID] = struct{}{} - debugLog("Ignoring invoice; TX exists in storjscan") + infoLog("Ignoring invoice; TX exists in storjscan") chore.analytics.TrackStorjscanUnpaidInvoice(invoice.ID, userID, user.Email) continue } @@ -183,7 +189,7 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { } if freezes.ViolationFreeze != nil { - debugLog("Ignoring invoice; account already frozen due to violation") + infoLog("Ignoring invoice; account already frozen due to violation") chore.analytics.TrackViolationFrozenUnpaidInvoice(invoice.ID, userID, user.Email) continue } @@ -191,7 +197,7 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { // try to pay the invoice before freezing/warning. err = chore.payments.Invoices().AttemptPayOverdueInvoices(ctx, userID) if err == nil { - debugLog("Ignoring invoice; Payment attempt successful") + infoLog("Ignoring invoice; Payment attempt successful") if freezes.BillingWarning != nil { err = chore.freezeService.BillingUnWarnUser(ctx, userID) @@ -212,7 +218,37 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { } if freezes.BillingFreeze != nil { - debugLog("Ignoring invoice; account already billing frozen") + if chore.nowFn().Sub(freezes.BillingFreeze.CreatedAt) > chore.config.BillingFreezeGracePeriod { + if user.Status == console.PendingDeletion { + infoLog("Ignoring invoice; account already marked for deletion") + continue + } + + // check if the invoice has been paid by the time the chore gets here. + isPaid, err := checkInvPaid(invoice.ID) + if err != nil { + errorLog("Could not verify invoice status", err) + continue + } + if isPaid { + infoLog("Ignoring invoice; payment already made") + continue + } + + status := console.PendingDeletion + err = chore.usersDB.Update(ctx, userID, console.UpdateUserRequest{ + Status: &status, + }) + if err != nil { + errorLog("Could not mark account for deletion", err) + continue + } + + infoLog("account marked for deletion") + continue + } + + infoLog("Ignoring invoice; account already billing frozen") continue } @@ -224,7 +260,7 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { continue } if isPaid { - debugLog("Ignoring invoice; payment already made") + infoLog("Ignoring invoice; payment already made") continue } err = chore.freezeService.BillingWarnUser(ctx, userID) @@ -232,12 +268,12 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { errorLog("Could not add billing warning event", err) continue } - debugLog("user billing warned") - warnedMap[userID] = struct{}{} + infoLog("user billing warned") + billingWarnedMap[userID] = struct{}{} continue } - if chore.nowFn().Sub(freezes.BillingWarning.CreatedAt) > chore.config.GracePeriod { + if chore.nowFn().Sub(freezes.BillingWarning.CreatedAt) > chore.config.BillingWarnGracePeriod { // check if the invoice has been paid by the time the chore gets here. isPaid, err := checkInvPaid(invoice.ID) if err != nil { @@ -245,7 +281,7 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { continue } if isPaid { - debugLog("Ignoring invoice; payment already made") + infoLog("Ignoring invoice; payment already made") continue } err = chore.freezeService.BillingFreezeUser(ctx, userID) @@ -253,22 +289,22 @@ func (chore *Chore) attemptFreezeWarn(ctx context.Context) { errorLog("Could not billing freeze account", err) continue } - debugLog("user billing frozen") - frozenMap[userID] = struct{}{} + infoLog("user billing frozen") + billingFrozenMap[userID] = struct{}{} } } chore.log.Info("billing freezing/warning executed", zap.Int("total invoices", len(invoices)), zap.Int("user total", len(userMap)), - zap.Int("total warned", len(warnedMap)), - zap.Int("total frozen", len(frozenMap)), + zap.Int("total billing warned", len(billingWarnedMap)), + zap.Int("total billing frozen", len(billingFrozenMap)), zap.Int("total bypassed due to size of invoice", len(bypassedLargeMap)), zap.Int("total bypassed due to storjscan payments", len(bypassedTokenMap)), ) } -func (chore *Chore) attemptUnfreezeUnwarn(ctx context.Context) { +func (chore *Chore) attemptBillingUnfreezeUnwarn(ctx context.Context) { cursor := console.FreezeEventsCursor{ Limit: 100, } @@ -300,14 +336,28 @@ func (chore *Chore) attemptUnfreezeUnwarn(ctx context.Context) { zap.Error(Error.Wrap(err)), ) } - - if event.Type == console.ViolationFreeze { - chore.log.Info("Skipping violation freeze event", + infoLog := func(message string) { + chore.log.Info(message, zap.String("process", "billing unfreeze/unwarn"), zap.Any("userID", event.UserID), zap.String("eventType", event.Type.String()), zap.Error(Error.Wrap(err)), ) + } + + if event.Type == console.ViolationFreeze { + infoLog("Skipping violation freeze event") + continue + } + + user, err := chore.usersDB.Get(ctx, event.UserID) + if err != nil { + errorLog("Could not get user", err) + continue + } + + if user.Status == console.Deleted || user.Status == console.PendingDeletion { + infoLog("Skipping event; account already deleted or pending deletion") continue } @@ -332,7 +382,7 @@ func (chore *Chore) attemptUnfreezeUnwarn(ctx context.Context) { errorLog("Could not billing unfreeze user", err) } unfrozenCount++ - } else { + } else if event.Type == console.BillingWarning { err = chore.freezeService.BillingUnWarnUser(ctx, event.UserID) if err != nil { errorLog("Could not billing unwarn user", err) diff --git a/satellite/payments/accountfreeze/chore_test.go b/satellite/payments/accountfreeze/chore_test.go index 58998f12e..f83208a13 100644 --- a/satellite/payments/accountfreeze/chore_test.go +++ b/satellite/payments/accountfreeze/chore_test.go @@ -141,7 +141,7 @@ func TestAutoFreezeChore(t *testing.T) { require.NotNil(t, freezes.ViolationFreeze) }) - t.Run("No freeze event for paid invoice", func(t *testing.T) { + t.Run("No billing freeze event for paid invoice", func(t *testing.T) { // AnalyticsMock tests that events are sent once. service.TestChangeFreezeTracker(newFreezeTrackerMock(t)) _, err := stripeClient.InvoiceItems().New(&stripe.InvoiceItemParams{ @@ -238,7 +238,7 @@ func TestAutoFreezeChore(t *testing.T) { require.Nil(t, freezes.ViolationFreeze) chore.TestSetNow(func() time.Time { - // current date is now after grace period + // current date is now after billing warn grace period return time.Now().AddDate(0, 0, 50) }) chore.Loop.TriggerWait() @@ -248,7 +248,19 @@ func TestAutoFreezeChore(t *testing.T) { require.NoError(t, err) require.NotNil(t, freezes.BillingFreeze) - // Pay invoice so it doesn't show up in the next test. + chore.TestSetNow(func() time.Time { + // current date is now after billing freeze grace period + return time.Now().AddDate(0, 0, 70) + }) + chore.Loop.TriggerWait() + + // user should be marked for deletion after the grace period + // after being frozen + userPD, err := usersDB.Get(ctx, user.ID) + require.NoError(t, err) + require.Equal(t, console.PendingDeletion, userPD.Status) + + // Pay invoice so user qualifies to be removed from billing freeze. inv, err = stripeClient.Invoices().Pay(inv.ID, &stripe.InvoicePayParams{ Params: stripe.Params{Context: ctx}, PaymentMethod: stripe.String(stripe1.MockInvoicesPaySuccess), @@ -256,9 +268,31 @@ func TestAutoFreezeChore(t *testing.T) { require.NoError(t, err) require.Equal(t, stripe.InvoiceStatusPaid, inv.Status) + // set user status to deleted + status := console.Deleted + err = usersDB.Update(ctx, user.ID, console.UpdateUserRequest{ + Status: &status, + }) + require.NoError(t, err) + + chore.Loop.TriggerWait() + + // deleted user should be skipped, hence would not exist the + // billing freeze status. + isFrozen, err := service.IsUserBillingFrozen(ctx, user.ID) + require.NoError(t, err) + require.True(t, isFrozen) + // unfreeze user so they're not frozen in the next test. err = service.BillingUnfreezeUser(ctx, user.ID) require.NoError(t, err) + + // set user status back to active + status = console.Active + err = usersDB.Update(ctx, user.ID, console.UpdateUserRequest{ + Status: &status, + }) + require.NoError(t, err) }) t.Run("No freeze event for failed invoice (successful later payment attempt)", func(t *testing.T) { diff --git a/scripts/testdata/satellite-config.yaml.lock b/scripts/testdata/satellite-config.yaml.lock index 20011a6ac..a10dae9a2 100755 --- a/scripts/testdata/satellite-config.yaml.lock +++ b/scripts/testdata/satellite-config.yaml.lock @@ -1,12 +1,15 @@ +# How long to wait between a billing freeze event and setting pending deletion account status. +# account-freeze.billing-freeze-grace-period: 1440h0m0s + +# How long to wait between a billing warning event and billing freezing an account. +# account-freeze.billing-warn-grace-period: 360h0m0s + # whether to run this chore. # account-freeze.enabled: false # whether to exclude storjscan-paying users from automatic warn/freeze # account-freeze.exclude-storjscan: false -# How long to wait between a warning event and freezing an account. -# account-freeze.grace-period: 360h0m0s - # How often to run this chore, which is how often unpaid invoices are checked. # account-freeze.interval: 24h0m0s