From 06944f062d35df4f7f5c19236d663dcace5dbeb0 Mon Sep 17 00:00:00 2001 From: Stefan Benten Date: Fri, 4 Feb 2022 18:31:24 +0100 Subject: [PATCH] satellite/{admin,payments,satellitedb}: add checks for deletion of free tier accounts This change adds some more checks to the deletion process for projects and users, since we ran into a race condition during invoicing, where projects have been deleted before the invoicing was finished, leading to missing references. This PR changes the logic to block user deletion if we are in exactly that period, while also allowing the deletion of projects/users on free tier during the month. Change-Id: Ic0735205e6633762fb7e3c2fa13e744cdfa5ec32 --- satellite/admin/project.go | 103 +++++++++++------- satellite/admin/project_test.go | 98 ++++++++++++++++- satellite/admin/user_test.go | 2 +- satellite/console/users.go | 2 + .../payments/stripecoinpayments/service.go | 10 +- satellite/satellitedb/dbx/satellitedb.dbx | 4 + satellite/satellitedb/dbx/satellitedb.dbx.go | 62 +++++++++++ satellite/satellitedb/users.go | 10 ++ 8 files changed, 245 insertions(+), 46 deletions(-) diff --git a/satellite/admin/project.go b/satellite/admin/project.go index ad8720731..80defe197 100644 --- a/satellite/admin/project.go +++ b/satellite/admin/project.go @@ -502,59 +502,82 @@ func (server *Server) deleteProject(w http.ResponseWriter, r *http.Request) { } } -func (server *Server) checkUsage(ctx context.Context, w http.ResponseWriter, projectID uuid.UUID) (hasUsage bool) { - // do not delete projects that have usage for the current month. +func (server *Server) checkInvoicing(ctx context.Context, w http.ResponseWriter, projectID uuid.UUID) (openInvoices bool) { year, month, _ := server.nowFn().UTC().Date() firstOfMonth := time.Date(year, month, 1, 0, 0, 0, 0, time.UTC) - currentUsage, err := server.db.ProjectAccounting().GetProjectTotal(ctx, projectID, firstOfMonth, server.nowFn()) - if err != nil { - sendJSONError(w, "unable to list project usage", err.Error(), http.StatusInternalServerError) - return true - } - if currentUsage.Storage > 0 || currentUsage.Egress > 0 || currentUsage.SegmentCount > 0 { - sendJSONError(w, "usage for current month exists", "", http.StatusConflict) - return true - } - - // if usage of last month exist, make sure to look for billing records - lastMonthUsage, err := server.db.ProjectAccounting().GetProjectTotal(ctx, projectID, firstOfMonth.AddDate(0, -1, 0), firstOfMonth.AddDate(0, 0, -1)) - if err != nil { - sendJSONError(w, "error getting project totals", - "", http.StatusInternalServerError) - return true - } - - if lastMonthUsage.Storage > 0 || lastMonthUsage.Egress > 0 || lastMonthUsage.SegmentCount > 0 { - // time passed into the check function need to be the UTC midnight dates - // of the first day of the current month and the first day of the last - // month - err := server.db.StripeCoinPayments().ProjectRecords().Check(ctx, projectID, firstOfMonth.AddDate(0, -1, 0), firstOfMonth) - if errors.Is(err, stripecoinpayments.ErrProjectRecordExists) { - record, err := server.db.StripeCoinPayments().ProjectRecords().Get(ctx, projectID, firstOfMonth.AddDate(0, -1, 0), firstOfMonth) - if err != nil { - sendJSONError(w, "unable to get project records", err.Error(), http.StatusInternalServerError) - return true - } - // state = 0 means unapplied and not invoiced yet. - if record.State == 0 { - sendJSONError(w, "unapplied project invoice record exist", "", http.StatusConflict) - return true - } - // Record has been applied, so project can be deleted. - return false - } + // Check if an invoice project record exists already + err := server.db.StripeCoinPayments().ProjectRecords().Check(ctx, projectID, firstOfMonth.AddDate(0, -1, 0), firstOfMonth) + if errors.Is(err, stripecoinpayments.ErrProjectRecordExists) { + record, err := server.db.StripeCoinPayments().ProjectRecords().Get(ctx, projectID, firstOfMonth.AddDate(0, -1, 0), firstOfMonth) if err != nil { sendJSONError(w, "unable to get project records", err.Error(), http.StatusInternalServerError) return true } - sendJSONError(w, "usage for last month exist, but is not billed yet", "", http.StatusConflict) + // state = 0 means unapplied and not invoiced yet. + if record.State == 0 { + sendJSONError(w, "unapplied project invoice record exist", "", http.StatusConflict) + return true + } + // Record has been applied, so project can be deleted. + return false + } + if err != nil { + sendJSONError(w, "unable to get project records", err.Error(), http.StatusInternalServerError) return true } return false } +func (server *Server) checkUsage(ctx context.Context, w http.ResponseWriter, projectID uuid.UUID) (hasUsage bool) { + year, month, _ := server.nowFn().UTC().Date() + firstOfMonth := time.Date(year, month, 1, 0, 0, 0, 0, time.UTC) + + prj, err := server.db.Console().Projects().Get(ctx, projectID) + if err != nil { + sendJSONError(w, "unable to get project details", + err.Error(), http.StatusInternalServerError) + return + } + + // If user is paid tier, check the usage limit, otherwise it is ok to delete it. + paid, err := server.db.Console().Users().GetUserPaidTier(ctx, prj.OwnerID) + if err != nil { + sendJSONError(w, "unable to project owner tier", + err.Error(), http.StatusInternalServerError) + return + } + if paid { + // check current month usage and do not allow deletion if usage exists + currentUsage, err := server.db.ProjectAccounting().GetProjectTotal(ctx, projectID, firstOfMonth, server.nowFn()) + if err != nil { + sendJSONError(w, "unable to list project usage", err.Error(), http.StatusInternalServerError) + return true + } + if currentUsage.Storage > 0 || currentUsage.Egress > 0 || currentUsage.SegmentCount > 0 { + sendJSONError(w, "usage for current month exists", "", http.StatusConflict) + return true + } + // check usage for last month + lastMonthUsage, err := server.db.ProjectAccounting().GetProjectTotal(ctx, projectID, firstOfMonth.AddDate(0, -1, 0), firstOfMonth.AddDate(0, 0, -1)) + if err != nil { + sendJSONError(w, "error getting project totals", + "", http.StatusInternalServerError) + return true + } + + // project had usage that is not billed yet + if lastMonthUsage.Storage > 0 || lastMonthUsage.Egress > 0 || lastMonthUsage.SegmentCount > 0 { + sendJSONError(w, "usage for last month exist, but is not billed yet", "", http.StatusConflict) + return true + } + } + + // If we have open invoice items, do not delete the project yet and wait for invoice completion. + return server.checkInvoicing(ctx, w, projectID) +} + func bucketNames(buckets []storj.Bucket) []string { var xs []string for _, b := range buckets { diff --git a/satellite/admin/project_test.go b/satellite/admin/project_test.go index 02ba58b4a..35af18eca 100644 --- a/satellite/admin/project_test.go +++ b/satellite/admin/project_test.go @@ -17,6 +17,7 @@ import ( "go.uber.org/zap" "storj.io/common/macaroon" + "storj.io/common/memory" "storj.io/common/storj" "storj.io/common/testcontext" "storj.io/common/uuid" @@ -437,17 +438,45 @@ func TestProjectCheckUsage_withUsage(t *testing.T) { err = planet.Satellites[0].DB.ProjectAccounting().CreateStorageTally(ctx, tally) require.NoError(t, err) + // Ensure User is free tier. + paid, err := planet.Satellites[0].DB.Console().Users().GetUserPaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID) + require.NoError(t, err) + require.False(t, paid) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("http://"+address.String()+"/api/projects/%s/usage", projectID), nil) require.NoError(t, err) req.Header.Set("Authorization", planet.Satellites[0].Config.Console.AuthToken) response, err := http.DefaultClient.Do(req) require.NoError(t, err) - require.Equal(t, http.StatusConflict, response.StatusCode) + require.Equal(t, http.StatusOK, response.StatusCode) require.Equal(t, "application/json", response.Header.Get("Content-Type")) responseBody, err := ioutil.ReadAll(response.Body) require.NoError(t, err) + require.Equal(t, "{\"result\":\"no project usage exist\"}", string(responseBody)) + require.NoError(t, response.Body.Close()) + + // Make User paid tier. + err = planet.Satellites[0].DB.Console().Users().UpdatePaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID, true, memory.PB, memory.PB, 1000000) + require.NoError(t, err) + + // Ensure User is paid tier. + paid, err = planet.Satellites[0].DB.Console().Users().GetUserPaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID) + require.NoError(t, err) + require.True(t, paid) + + req, err = http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("http://"+address.String()+"/api/projects/%s/usage", projectID), nil) + require.NoError(t, err) + req.Header.Set("Authorization", planet.Satellites[0].Config.Console.AuthToken) + + response, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.Equal(t, http.StatusConflict, response.StatusCode) + require.Equal(t, "application/json", response.Header.Get("Content-Type")) + + responseBody, err = ioutil.ReadAll(response.Body) + require.NoError(t, err) require.Equal(t, "{\"error\":\"usage for current month exists\",\"detail\":\"\"}", string(responseBody)) require.NoError(t, response.Body.Close()) }) @@ -531,6 +560,8 @@ func TestProjectCheckUsage_lastMonthUnappliedInvoice(t *testing.T) { }) } +// TestProjectDelete_withUsageCurrentMonth first tries to delete a actively used project of a paid tier user, which +// should fail and afterwards converts the user to free tier and tries the deletion again. That deletion should succeed. func TestProjectDelete_withUsageCurrentMonth(t *testing.T) { testplanet.Run(t, testplanet.Config{ SatelliteCount: 1, @@ -581,6 +612,15 @@ func TestProjectDelete_withUsageCurrentMonth(t *testing.T) { err = planet.Satellites[0].DB.ProjectAccounting().CreateStorageTally(ctx, tally) require.NoError(t, err) + // Make User paid tier. + err = planet.Satellites[0].DB.Console().Users().UpdatePaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID, true, memory.PB, memory.PB, 1000000) + require.NoError(t, err) + + // Ensure User is paid tier. + paid, err := planet.Satellites[0].DB.Console().Users().GetUserPaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID) + require.NoError(t, err) + require.True(t, paid) + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, fmt.Sprintf("http://"+address.String()+"/api/projects/%s", projectID), nil) require.NoError(t, err) req.Header.Set("Authorization", planet.Satellites[0].Config.Console.AuthToken) @@ -594,9 +634,34 @@ func TestProjectDelete_withUsageCurrentMonth(t *testing.T) { require.NoError(t, err) require.Equal(t, "{\"error\":\"usage for current month exists\",\"detail\":\"\"}", string(responseBody)) require.NoError(t, response.Body.Close()) + + // Make User free tier again. + err = planet.Satellites[0].DB.Console().Users().UpdatePaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID, false, memory.TB, memory.TB, 100000) + require.NoError(t, err) + + // Ensure User is free tier. + paid, err = planet.Satellites[0].DB.Console().Users().GetUserPaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID) + require.NoError(t, err) + require.False(t, paid) + + req, err = http.NewRequestWithContext(ctx, http.MethodDelete, fmt.Sprintf("http://"+address.String()+"/api/projects/%s", projectID), nil) + require.NoError(t, err) + req.Header.Set("Authorization", planet.Satellites[0].Config.Console.AuthToken) + + response, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.Equal(t, http.StatusOK, response.StatusCode) + + responseBody, err = ioutil.ReadAll(response.Body) + require.NoError(t, err) + require.Equal(t, "", string(responseBody)) + require.NoError(t, response.Body.Close()) }) } +// TestProjectDelete_withUsageCurrentMonth first tries to delete a last month used project of a paid tier user, which +// should fail and afterwards converts the user to free tier and tries the deletion again. That deletion should succeed. +// This test ensures we bill paid tier users for past months usage and do not forget to do so. func TestProjectDelete_withUsagePreviousMonth(t *testing.T) { testplanet.Run(t, testplanet.Config{ SatelliteCount: 1, @@ -649,6 +714,15 @@ func TestProjectDelete_withUsagePreviousMonth(t *testing.T) { err = planet.Satellites[0].DB.ProjectAccounting().CreateStorageTally(ctx, tally) require.NoError(t, err) + // Make User paid tier. + err = planet.Satellites[0].DB.Console().Users().UpdatePaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID, true, memory.PB, memory.PB, 1000000) + require.NoError(t, err) + + // Ensure User is paid tier. + paid, err := planet.Satellites[0].DB.Console().Users().GetUserPaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID) + require.NoError(t, err) + require.True(t, paid) + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, fmt.Sprintf("http://"+address.String()+"/api/projects/%s", projectID), nil) require.NoError(t, err) req.Header.Set("Authorization", planet.Satellites[0].Config.Console.AuthToken) @@ -660,5 +734,27 @@ func TestProjectDelete_withUsagePreviousMonth(t *testing.T) { require.Equal(t, "{\"error\":\"usage for last month exist, but is not billed yet\",\"detail\":\"\"}", string(responseBody)) require.NoError(t, response.Body.Close()) require.Equal(t, http.StatusConflict, response.StatusCode) + + // Make User free tier again. + err = planet.Satellites[0].DB.Console().Users().UpdatePaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID, false, memory.TB, memory.TB, 100000) + require.NoError(t, err) + + // Ensure User is free tier. + paid, err = planet.Satellites[0].DB.Console().Users().GetUserPaidTier(ctx, planet.Uplinks[0].Projects[0].Owner.ID) + require.NoError(t, err) + require.False(t, paid) + + req, err = http.NewRequestWithContext(ctx, http.MethodDelete, fmt.Sprintf("http://"+address.String()+"/api/projects/%s", projectID), nil) + require.NoError(t, err) + req.Header.Set("Authorization", planet.Satellites[0].Config.Console.AuthToken) + + response, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.Equal(t, http.StatusOK, response.StatusCode) + + responseBody, err = ioutil.ReadAll(response.Body) + require.NoError(t, err) + require.Equal(t, "", string(responseBody)) + require.NoError(t, response.Body.Close()) }) } diff --git a/satellite/admin/user_test.go b/satellite/admin/user_test.go index 62783bc1a..0c158a21e 100644 --- a/satellite/admin/user_test.go +++ b/satellite/admin/user_test.go @@ -224,7 +224,7 @@ func TestUserDelete(t *testing.T) { body = assertReq(ctx, t, link, http.MethodDelete, "", http.StatusOK, "", planet.Satellites[0].Config.Console.AuthToken) require.Len(t, body, 0) - // Deleting unexisting user returns Not Found. + // Deleting non-existing user returns Not Found. body = assertReq(ctx, t, link, http.MethodDelete, "", http.StatusNotFound, "", planet.Satellites[0].Config.Console.AuthToken) require.Contains(t, string(body), "does not exist") }) diff --git a/satellite/console/users.go b/satellite/console/users.go index 69484cbb4..245b9a568 100644 --- a/satellite/console/users.go +++ b/satellite/console/users.go @@ -34,6 +34,8 @@ type Users interface { GetProjectLimit(ctx context.Context, id uuid.UUID) (limit int, err error) // GetUserProjectLimits is a method to get the users storage and bandwidth limits for new projects. GetUserProjectLimits(ctx context.Context, id uuid.UUID) (limit *ProjectLimits, err error) + // GetUserPaidTier is a method to gather whether the specified user is on the Paid Tier or not. + GetUserPaidTier(ctx context.Context, id uuid.UUID) (isPaid bool, err error) } // UserInfo holds User updatable data. diff --git a/satellite/payments/stripecoinpayments/service.go b/satellite/payments/stripecoinpayments/service.go index 1c1bb8267..88467e392 100644 --- a/satellite/payments/stripecoinpayments/service.go +++ b/satellite/payments/stripecoinpayments/service.go @@ -539,12 +539,14 @@ func (service *Service) applyProjectRecords(ctx context.Context, records []Proje for _, record := range records { if err = ctx.Err(); err != nil { - return err + return errs.Wrap(err) } proj, err := service.projectsDB.Get(ctx, record.ProjectID) if err != nil { - return err + // This should never happen, but be sure to log info to further troubleshoot before exiting. + service.log.Error("project ID for corresponding project record not found", zap.Stringer("Record ID", record.ID), zap.Stringer("Project ID", record.ProjectID)) + return errs.Wrap(err) } cusID, err := service.db.Customers().GetCustomerID(ctx, proj.OwnerID) @@ -554,11 +556,11 @@ func (service *Service) applyProjectRecords(ctx context.Context, records []Proje continue } - return err + return errs.Wrap(err) } if err = service.createInvoiceItems(ctx, cusID, proj.Name, record); err != nil { - return err + return errs.Wrap(err) } } diff --git a/satellite/satellitedb/dbx/satellitedb.dbx b/satellite/satellitedb/dbx/satellitedb.dbx index 8d7834a6b..2bd7933bb 100644 --- a/satellite/satellitedb/dbx/satellitedb.dbx +++ b/satellite/satellitedb/dbx/satellitedb.dbx @@ -332,6 +332,10 @@ read one ( select user.project_limit where user.id = ? ) +read one ( + select user.paid_tier + where user.id = ? +) read one ( select user.project_storage_limit user.project_bandwidth_limit user.project_segment_limit diff --git a/satellite/satellitedb/dbx/satellitedb.dbx.go b/satellite/satellitedb/dbx/satellitedb.dbx.go index aebb6bb90..d385a3a3f 100644 --- a/satellite/satellitedb/dbx/satellitedb.dbx.go +++ b/satellite/satellitedb/dbx/satellitedb.dbx.go @@ -10864,6 +10864,10 @@ type Paged_StoragenodeBandwidthRollup_By_StoragenodeId_And_IntervalStart_Greater _set bool } +type PaidTier_Row struct { + PaidTier bool +} + type Placement_Row struct { Placement *int } @@ -12373,6 +12377,28 @@ func (obj *pgxImpl) Get_User_ProjectLimit_By_Id(ctx context.Context, } +func (obj *pgxImpl) Get_User_PaidTier_By_Id(ctx context.Context, + user_id User_Id_Field) ( + row *PaidTier_Row, err error) { + defer mon.Task()(&ctx)(&err) + + var __embed_stmt = __sqlbundle_Literal("SELECT users.paid_tier FROM users WHERE users.id = ?") + + var __values []interface{} + __values = append(__values, user_id.value()) + + var __stmt = __sqlbundle_Render(obj.dialect, __embed_stmt) + obj.logStmt(__stmt, __values...) + + row = &PaidTier_Row{} + err = obj.queryRowContext(ctx, __stmt, __values...).Scan(&row.PaidTier) + if err != nil { + return (*PaidTier_Row)(nil), obj.makeErr(err) + } + return row, nil + +} + func (obj *pgxImpl) Get_User_ProjectStorageLimit_User_ProjectBandwidthLimit_User_ProjectSegmentLimit_By_Id(ctx context.Context, user_id User_Id_Field) ( row *ProjectStorageLimit_ProjectBandwidthLimit_ProjectSegmentLimit_Row, err error) { @@ -18720,6 +18746,28 @@ func (obj *pgxcockroachImpl) Get_User_ProjectLimit_By_Id(ctx context.Context, } +func (obj *pgxcockroachImpl) Get_User_PaidTier_By_Id(ctx context.Context, + user_id User_Id_Field) ( + row *PaidTier_Row, err error) { + defer mon.Task()(&ctx)(&err) + + var __embed_stmt = __sqlbundle_Literal("SELECT users.paid_tier FROM users WHERE users.id = ?") + + var __values []interface{} + __values = append(__values, user_id.value()) + + var __stmt = __sqlbundle_Render(obj.dialect, __embed_stmt) + obj.logStmt(__stmt, __values...) + + row = &PaidTier_Row{} + err = obj.queryRowContext(ctx, __stmt, __values...).Scan(&row.PaidTier) + if err != nil { + return (*PaidTier_Row)(nil), obj.makeErr(err) + } + return row, nil + +} + func (obj *pgxcockroachImpl) Get_User_ProjectStorageLimit_User_ProjectBandwidthLimit_User_ProjectSegmentLimit_By_Id(ctx context.Context, user_id User_Id_Field) ( row *ProjectStorageLimit_ProjectBandwidthLimit_ProjectSegmentLimit_Row, err error) { @@ -24720,6 +24768,16 @@ func (rx *Rx) Get_User_By_NormalizedEmail_And_Status_Not_Number(ctx context.Cont return tx.Get_User_By_NormalizedEmail_And_Status_Not_Number(ctx, user_normalized_email) } +func (rx *Rx) Get_User_PaidTier_By_Id(ctx context.Context, + user_id User_Id_Field) ( + row *PaidTier_Row, err error) { + var tx *Tx + if tx, err = rx.getTx(ctx); err != nil { + return + } + return tx.Get_User_PaidTier_By_Id(ctx, user_id) +} + func (rx *Rx) Get_User_ProjectLimit_By_Id(ctx context.Context, user_id User_Id_Field) ( row *ProjectLimit_Row, err error) { @@ -25724,6 +25782,10 @@ type Methods interface { user_normalized_email User_NormalizedEmail_Field) ( user *User, err error) + Get_User_PaidTier_By_Id(ctx context.Context, + user_id User_Id_Field) ( + row *PaidTier_Row, err error) + Get_User_ProjectLimit_By_Id(ctx context.Context, user_id User_Id_Field) ( row *ProjectLimit_Row, err error) diff --git a/satellite/satellitedb/users.go b/satellite/satellitedb/users.go index 83bee7ae6..d91e5027b 100644 --- a/satellite/satellitedb/users.go +++ b/satellite/satellitedb/users.go @@ -197,6 +197,16 @@ func (users *users) GetUserProjectLimits(ctx context.Context, id uuid.UUID) (lim return limitsFromDBX(ctx, row) } +func (users *users) GetUserPaidTier(ctx context.Context, id uuid.UUID) (isPaid bool, err error) { + defer mon.Task()(&ctx)(&err) + + row, err := users.db.Get_User_PaidTier_By_Id(ctx, dbx.User_Id(id[:])) + if err != nil { + return false, err + } + return row.PaidTier, nil +} + // toUpdateUser creates dbx.User_Update_Fields with only non-empty fields as updatable. func toUpdateUser(user *console.User) (*dbx.User_Update_Fields, error) { update := dbx.User_Update_Fields{