From c7313eb266b91f34774e92be2cf67bd579cf2442 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 22:57:56 +0100 Subject: [PATCH 1/3] fix(api): cap resume of a planned purchase by the caller's amount limit Resuming a paused planned purchase puts it back on schedule, so it now counts against MaxPurchaseAmount like a spend. update-any is charged with the full purchase constraints; update (including the creator fallback) is checked on amount and currency only; the admin API key is unrestricted. The amount is the total commitment, not the upfront cost. A nil auth component now fails closed with a 500. Closes #1130 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 109 +++++++++++++----- .../handler_purchases_apikey_scope_test.go | 2 +- .../api/handler_purchases_notified_test.go | 4 +- ...purchases_pause_resume_constraints_test.go | 6 + 4 files changed, 92 insertions(+), 29 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index be9073a32..4e0dbaa33 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -235,7 +235,8 @@ func (h *Handler) authorizeBoundedExecutionManagement(ctx context.Context, sessi if err != nil { return err } - return h.authorizeUpdateAnyOnRow(ctx, session, execution) + _, err = h.authorizeUpdateAnyOnRow(ctx, session, execution) + return err } // authorizeUpdateAnyOnRow checks a session holding update-any against the @@ -245,15 +246,15 @@ func (h *Handler) authorizeBoundedExecutionManagement(ctx context.Context, sessi // denial falls back to the creator match and otherwise stays the refusal; any // other error (a probe or constraint-store failure) is returned and never read // as a denial, so it cannot fall through to a weaker branch. -func (h *Handler) authorizeUpdateAnyOnRow(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { +func (h *Handler) authorizeUpdateAnyOnRow(ctx context.Context, session *Session, execution *config.PurchaseExecution) (creatorFallback bool, err error) { anyErr := h.requirePermissionConstraints(ctx, session, auth.ActionUpdateAny, auth.ResourcePurchases, revokeExecutionConstraintSets(execution)) if anyErr == nil || !isForbidden(anyErr) { - return anyErr + return false, anyErr } if isExecutionCreator(session, execution) { - return nil + return true, nil } - return anyErr + return false, anyErr } // authorizeClaimedRowManagement re-runs authorizeBoundedExecutionManagement's @@ -261,20 +262,41 @@ func (h *Handler) authorizeUpdateAnyOnRow(ctx context.Context, session *Session, // (issue #415), so an edit landing between the pre-claim read and the claim // cannot carry the row out of a bounded update-any's scope. func (h *Handler) authorizeClaimedRowManagement(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { + _, err := h.authorizeRowManagement(ctx, session, execution) + return err +} + +// authorizeRowManagement is the decision behind authorizeClaimedRowManagement +// that also reports the verb that authorized the caller: "" for the admin API +// key (unrestricted), auth.ActionUpdateAny when the update-any scope check +// passed, auth.ActionUpdate when the session has no update-any or the creator +// match won after an update-any scope denial. Resume bounds the row's amount +// by that verb's cap. +func (h *Handler) authorizeRowManagement(ctx context.Context, session *Session, execution *config.PurchaseExecution) (verb string, err error) { if session.UserID == apiKeyAdminUserID { - return nil + return "", nil + } + if h.auth == nil { + return "", NewClientError(500, "authentication service not configured") } hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionUpdateAny) if err != nil { - return err + return "", err } if hasAny { - return h.authorizeUpdateAnyOnRow(ctx, session, execution) + creatorFallback, anyErr := h.authorizeUpdateAnyOnRow(ctx, session, execution) + if anyErr != nil { + return "", anyErr + } + if creatorFallback { + return auth.ActionUpdate, nil + } + return auth.ActionUpdateAny, nil } if !isExecutionCreator(session, execution) { - return NewClientError(403, "permission denied: cannot manage another user's scheduled purchase") + return "", NewClientError(403, "permission denied: cannot manage another user's scheduled purchase") } - return nil + return auth.ActionUpdate, nil } // isExecutionCreator requires both IDs to be non-empty and equal, so an @@ -317,22 +339,36 @@ func (h *Handler) resumePlannedPurchase(ctx context.Context, req *events.LambdaF if err != nil { return nil, err } - if err := h.requireExecutionAccess(ctx, session, executionID); err != nil { + if accessErr := h.requireExecutionAccess(ctx, session, executionID); accessErr != nil { + return nil, accessErr + } + row, err := h.loadPlannedExecution(ctx, executionID) + if err != nil { return nil, err } - if err := h.authorizeBoundedExecutionManagement(ctx, session, executionID); err != nil { + verb, err := h.authorizeRowManagement(ctx, session, row) + if err != nil { return nil, err } // A recommendation-bearing row that is past its date is moved to // now + NotificationDaysBefore on resume; a plan without a veto window // would move it to "now", so refuse before changing anything. - if checkErr := h.requireResumeVetoWindow(ctx, executionID, time.Now()); checkErr != nil { + if checkErr := h.requireResumeVetoWindow(ctx, row, time.Now()); checkErr != nil { return nil, checkErr } - // Atomically transition from paused back to pending - resumed, transitionErr := h.config.TransitionExecutionStatus(ctx, executionID, []string{"paused"}, "pending", resolveCreatorUserID(session)) + // Resuming puts the row back on schedule, so it counts against + // MaxPurchaseAmount like a spend (issue #1130). + if capErr := h.enforceResumeAmountCap(ctx, session, verb, row.Recommendations); capErr != nil { + return nil, capErr + } + + // Atomically transition from paused back to pending. Pinned so a + // recommendation-set change (empty to non-empty) between the cap check + // and this write loses the CAS; it does not catch in-set content edits, + // which no writer performs. + resumed, transitionErr := h.config.TransitionExecutionStatusPinned(ctx, executionID, []string{"paused"}, "pending", resolveCreatorUserID(session), len(row.Recommendations) == 0) if transitionErr != nil { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be resumed: %v", executionID, transitionErr)) } @@ -343,25 +379,39 @@ func (h *Handler) resumePlannedPurchase(ctx context.Context, req *events.LambdaF return &StatusResponse{Status: "resumed"}, nil } +// enforceResumeAmountCap bounds the row's total commitment by the cap of the +// verb that authorized the resume (issue #1130). An update-any holder is +// checked on update-any's full scope and amount, as approve-any is. A creator +// (or any update holder) is checked on amount and currency only, with no new +// scope matching on update:purchases. The admin API key (empty verb) and a +// row without recommendations skip the check (recommendations attach later +// only to pending/notified rows and the scheduler re-checks the plan cap). +func (h *Handler) enforceResumeAmountCap(ctx context.Context, session *Session, verb string, recs []config.RecommendationRecord) error { + if verb == "" || len(recs) == 0 { + return nil + } + if verb == auth.ActionUpdateAny { + return h.enforcePurchaseConstraints(ctx, session, verb, recs) + } + total := purchaseConstraintSets(recs)[0].MaxPurchaseAmount + return h.enforcePurchaseConstraintSets(ctx, session, verb, recs, []auth.PermissionConstraints{{MaxPurchaseAmount: total}}) +} + // requireResumeVetoWindow refuses to resume a recommendation-bearing row whose // scheduled_date has passed when its plan has no veto window (S13). -func (h *Handler) requireResumeVetoWindow(ctx context.Context, executionID string, now time.Time) error { - row, err := h.config.GetExecutionByID(ctx, executionID) - if err != nil { - return fmt.Errorf("failed to read execution %s before resume: %w", executionID, err) - } +func (h *Handler) requireResumeVetoWindow(ctx context.Context, row *config.PurchaseExecution, now time.Time) error { if len(row.Recommendations) == 0 || !row.ScheduledDate.Before(now) { return nil } plan, err := h.config.GetPurchasePlan(ctx, row.PlanID) if err != nil { - return fmt.Errorf("failed to read plan %s before resuming execution %s: %w", row.PlanID, executionID, err) + return fmt.Errorf("failed to read plan %s before resuming execution %s: %w", row.PlanID, row.ExecutionID, err) } if plan.RampSchedule.DatesManaged { return editorReconciliationError() } if vetoErr := plan.RequireVetoWindow(); vetoErr != nil { - return NewClientError(409, fmt.Sprintf("execution %s cannot be resumed: %v", executionID, vetoErr)) + return NewClientError(409, fmt.Sprintf("execution %s cannot be resumed: %v", row.ExecutionID, vetoErr)) } return nil } @@ -3262,14 +3312,21 @@ func parseExecutePurchaseBody(body string) (ExecutePurchaseRequest, error) { // USD, so a batch with any rec not provably priced in USD is checked by // requireUncappedPurchaseGrant instead of by its raw amount (issue #361). func (h *Handler) enforcePurchaseConstraints(ctx context.Context, session *Session, action string, recs []config.RecommendationRecord) error { - constraintSets := purchaseConstraintSets(recs) - if err := requireNonZeroCommitment(constraintSets); err != nil { + return h.enforcePurchaseConstraintSets(ctx, session, action, recs, purchaseConstraintSets(recs)) +} + +// enforcePurchaseConstraintSets is enforcePurchaseConstraints for a caller- +// chosen constraint set list: the zero-commitment guard, the USD rule for +// non-USD recs, then the permission check. sets must come from recs +// (purchaseConstraintSets) or carry the same total in MaxPurchaseAmount. +func (h *Handler) enforcePurchaseConstraintSets(ctx context.Context, session *Session, action string, recs []config.RecommendationRecord, sets []auth.PermissionConstraints) error { + if err := requireNonZeroCommitment(sets); err != nil { return err } if currency := nonUSDPurchaseCurrency(recs); currency != "" { - return h.requireUncappedPurchaseGrant(ctx, session, action, constraintSets, currency) + return h.requireUncappedPurchaseGrant(ctx, session, action, sets, currency) } - return h.requirePermissionConstraints(ctx, session, action, "purchases", constraintSets) + return h.requirePermissionConstraints(ctx, session, action, "purchases", sets) } // purchaseConstraintSets builds one auth.PermissionConstraints per diff --git a/internal/api/handler_purchases_apikey_scope_test.go b/internal/api/handler_purchases_apikey_scope_test.go index 8738213e6..420795c0c 100644 --- a/internal/api/handler_purchases_apikey_scope_test.go +++ b/internal/api/handler_purchases_apikey_scope_test.go @@ -117,7 +117,7 @@ func keyScopeExec(status, creator string) *config.PurchaseExecution { ExecutionID: keyScopeExecID, Status: status, CreatedByUserID: &creator, ScheduledDate: time.Now().Add(72 * time.Hour), Recommendations: []config.RecommendationRecord{{ - ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, UpfrontCost: 100, + ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, UpfrontCost: 100, CommitmentCurrency: "USD", }}, } } diff --git a/internal/api/handler_purchases_notified_test.go b/internal/api/handler_purchases_notified_test.go index 561e1f357..f53ddb542 100644 --- a/internal/api/handler_purchases_notified_test.go +++ b/internal/api/handler_purchases_notified_test.go @@ -43,7 +43,7 @@ func TestPausePlannedPurchase_AcceptsNotifiedRow(t *testing.T) { func TestResumePlannedPurchase_PastScheduledDate(t *testing.T) { ctx := context.Background() - recs := []config.RecommendationRecord{{ID: "rec-1", Count: 1}} + recs := []config.RecommendationRecord{{ID: "rec-1", Count: 1, UpfrontCost: 100, CommitmentCurrency: "USD"}} cases := []struct { name string recs []config.RecommendationRecord @@ -101,7 +101,7 @@ func TestCancelOrRecoverExecution_ReleasesSuppressionsOfNotifiedRow(t *testing.T // recommendation-bearing row moved to "now"; nothing is changed. func TestResumePlannedPurchase_RefusesWithoutVetoWindow(t *testing.T) { ctx := context.Background() - recs := []config.RecommendationRecord{{ID: "rec-1", Count: 1}} + recs := []config.RecommendationRecord{{ID: "rec-1", Count: 1, UpfrontCost: 100, CommitmentCurrency: "USD"}} for _, tc := range []struct { name string recs []config.RecommendationRecord diff --git a/internal/api/handler_purchases_pause_resume_constraints_test.go b/internal/api/handler_purchases_pause_resume_constraints_test.go index dfe54417f..f33426bbf 100644 --- a/internal/api/handler_purchases_pause_resume_constraints_test.go +++ b/internal/api/handler_purchases_pause_resume_constraints_test.go @@ -43,6 +43,12 @@ func runPauseResume(t *testing.T, op pauseResumeOp, creator *string, recs []conf if recs != nil { exec.Recommendations = recs } + // Resume refuses a zero-commitment batch (issue #1130), so price the rows. + for i := range exec.Recommendations { + if exec.Recommendations[i].UpfrontCost == 0 { + exec.Recommendations[i].UpfrontCost = 100 + } + } h, store := cancelRouteHandler(t, exec, append(perms, revokePerm(auth.ActionUpdate, nil))...) resp, err := op.run(h) return resp, store, err From 5e78876b0e5b8686ddf60ef4f9413ace0c2e8185 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 22:58:16 +0100 Subject: [PATCH 2/3] test(api): pin the resume amount cap Covers update-any, update and the creator fallback, admin API keys, non-USD and zero-commitment rows, empty recs, plan-step rows and the nil auth component (500). Each refusal test fails against the old handler. Refs #1130 Co-Authored-By: Claude Opus 5.5 --- ...andler_purchases_resume_amount_cap_test.go | 371 ++++++++++++++++++ 1 file changed, 371 insertions(+) create mode 100644 internal/api/handler_purchases_resume_amount_cap_test.go diff --git a/internal/api/handler_purchases_resume_amount_cap_test.go b/internal/api/handler_purchases_resume_amount_cap_test.go new file mode 100644 index 000000000..135487179 --- /dev/null +++ b/internal/api/handler_purchases_resume_amount_cap_test.go @@ -0,0 +1,371 @@ +package api + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Issue #1130: resuming a paused planned purchase puts it back on schedule, so +// it counts against MaxPurchaseAmount like a spend. The cap is on the TOTAL +// commitment (upfront plus recurring over the term), by the verb that +// authorized the resume. Rows are azure/compute/westeurope on "acct-1". + +func capOf(amount float64) *auth.PermissionConstraints { + return &auth.PermissionConstraints{MaxPurchaseAmount: amount} +} + +// resumeCapRec is a one-year rec in the cancelRouteExec scope with the given costs. +func resumeCapRec(upfront, monthly float64, currency string) config.RecommendationRecord { + acct := "acct-1" + rec := config.RecommendationRecord{ + ID: "r1", Provider: "azure", Service: "compute", Region: "westeurope", CloudAccountID: &acct, + Count: 1, UpfrontCost: upfront, Term: 1, CommitmentCurrency: currency, + } + if monthly != 0 { + rec.MonthlyCost = &monthly + } + return rec +} + +// noUpfrontRecs totals 1200: no upfront, 100/month over one year. +func noUpfrontRecs() []config.RecommendationRecord { + return []config.RecommendationRecord{resumeCapRec(0, 100, "USD")} +} + +type resumeCapCase struct { + creator *string + recs []config.RecommendationRecord + perms []auth.Permission + // totalUpfront is the stored TotalUpfrontCost, deliberately unrelated to recs. + totalUpfront float64 +} + +// runResumeCap resumes a paused row as session u-1 with exactly perms; the +// pinned transition expects sawEmpty to match whether recs is empty. +func runResumeCap(t *testing.T, c resumeCapCase) (*StatusResponse, *MockConfigStore, error) { + t.Helper() + exec := cancelRouteExec("paused") + exec.CreatedByUserID = c.creator + exec.ScheduledDate = time.Now().Add(24 * time.Hour) + exec.Recommendations = c.recs + exec.TotalUpfrontCost = c.totalUpfront + h, store := cancelRouteHandler(t, exec, c.perms...) + resumed := *exec + resumed.Status = "pending" + store.On("TransitionExecutionStatusPinned", mock.Anything, cancelExecID, []string{"paused"}, "pending", mock.Anything, len(c.recs) == 0). + Return(&resumed, nil).Maybe() + resp, err := h.resumePlannedPurchase(context.Background(), deletePlannedReq(), cancelExecID) + return resp, store, err +} + +func requireResumeNotTransitioned(t *testing.T, store *MockConfigStore) { + t.Helper() + requireNotTransitioned(t, store) + store.AssertNotCalled(t, "TransitionExecutionStatusPinned", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestResumeAmountCap_UpdateAnyChargesTotalCommitment(t *testing.T) { + t.Parallel() + other := pauseResumeOtherUser + for _, tc := range []struct { + name string + cap float64 + totalUpfront float64 + recs []config.RecommendationRecord + denied bool + }{ + {"no-upfront recurring cost above the cap", 500, 0, noUpfrontRecs(), true}, + {"stored upfront below the cap does not hide the recs", 500, 1, noUpfrontRecs(), true}, + {"stored upfront above the cap does not price the recs", 500, 9999, []config.RecommendationRecord{resumeCapRec(100, 0, "USD")}, false}, + {"total equal to the cap", 1200, 0, noUpfrontRecs(), false}, + {"uncapped", 0, 0, noUpfrontRecs(), false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var c *auth.PermissionConstraints + if tc.cap != 0 { + c = capOf(tc.cap) + } + resp, store, err := runResumeCap(t, resumeCapCase{ + creator: &other, recs: tc.recs, totalUpfront: tc.totalUpfront, + perms: []auth.Permission{revokePerm(auth.ActionUpdate, nil), revokePerm(auth.ActionUpdateAny, c)}, + }) + if tc.denied { + requireRevokeForbidden(t, err, constraintDenied) + assert.NotContains(t, err.Error(), "1200", "the refusal must not echo the commitment") + requireResumeNotTransitioned(t, store) + return + } + require.NoError(t, err) + assert.Equal(t, "resumed", resp.Status) + }) + } +} + +func TestResumeAmountCap_CreatorOnUpdateIsCapped(t *testing.T) { + t.Parallel() + me := revokeConstraintsUser + run := func(cap float64) (*StatusResponse, *MockConfigStore, error) { + var c *auth.PermissionConstraints + if cap != 0 { + c = capOf(cap) + } + return runResumeCap(t, resumeCapCase{creator: &me, recs: noUpfrontRecs(), perms: []auth.Permission{revokePerm(auth.ActionUpdate, c)}}) + } + t.Run("above the cap is refused", func(t *testing.T) { + _, store, err := run(500) + requireRevokeForbidden(t, err, constraintDenied) + requireResumeNotTransitioned(t, store) + }) + t.Run("at the cap resumes", func(t *testing.T) { + _, _, err := run(1200) + require.NoError(t, err) + }) + t.Run("uncapped resumes", func(t *testing.T) { + _, _, err := run(0) + require.NoError(t, err) + }) +} + +// The creator fallback after an update-any scope denial must not bypass the cap +// on update. +func TestResumeAmountCap_CreatorFallbackStillCapped(t *testing.T) { + t.Parallel() + me := revokeConstraintsUser + wrongAccount := revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{AccountIDs: []string{"other-acct"}}) + _, store, err := runResumeCap(t, resumeCapCase{creator: &me, recs: noUpfrontRecs(), + perms: []auth.Permission{wrongAccount, revokePerm(auth.ActionUpdate, capOf(500))}}) + requireRevokeForbidden(t, err, constraintDenied) + requireResumeNotTransitioned(t, store) +} + +// An in-scope update-any holder is charged on update-any's cap even for their +// own row, as approve-any is. +func TestResumeAmountCap_InScopeUpdateAnyCreatorChargedOnUpdateAny(t *testing.T) { + t.Parallel() + me := revokeConstraintsUser + _, store, err := runResumeCap(t, resumeCapCase{creator: &me, recs: noUpfrontRecs(), + perms: []auth.Permission{revokePerm(auth.ActionUpdateAny, capOf(500)), revokePerm(auth.ActionUpdate, nil)}}) + requireRevokeForbidden(t, err, constraintDenied) + requireResumeNotTransitioned(t, store) +} + +// A creator whose update-any is scoped elsewhere resumes their own uncapped row. +func TestResumeAmountCap_ScopedUpdateAnyCreatorUncappedUpdateResumes(t *testing.T) { + t.Parallel() + me := revokeConstraintsUser + wrongAccount := revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{AccountIDs: []string{"acct-A"}}) + resp, _, err := runResumeCap(t, resumeCapCase{creator: &me, recs: noUpfrontRecs(), + perms: []auth.Permission{wrongAccount, revokePerm(auth.ActionUpdate, nil)}}) + require.NoError(t, err) + assert.Equal(t, "resumed", resp.Status) +} + +// The update branch bounds the amount only: a region-scoped update grant must +// not newly refuse a creator's own row. +func TestResumeAmountCap_UpdateBranchAddsNoScope(t *testing.T) { + t.Parallel() + me := revokeConstraintsUser + scoped := &auth.PermissionConstraints{Regions: []string{"eastus"}, MaxPurchaseAmount: 5000} + resp, _, err := runResumeCap(t, resumeCapCase{creator: &me, recs: noUpfrontRecs(), + perms: []auth.Permission{revokePerm(auth.ActionUpdate, scoped)}}) + require.NoError(t, err) + assert.Equal(t, "resumed", resp.Status) +} + +func TestResumeAmountCap_NonUSDUnderCap(t *testing.T) { + t.Parallel() + other := pauseResumeOtherUser + me := revokeConstraintsUser + eur := []config.RecommendationRecord{resumeCapRec(100, 0, "EUR")} + mixed := []config.RecommendationRecord{resumeCapRec(100, 0, "USD"), resumeCapRec(100, 0, "EUR")} + for _, branch := range []struct { + name string + creator *string + verb string + }{{"update-any", &other, auth.ActionUpdateAny}, {"update", &me, auth.ActionUpdate}} { + perms := func(c *auth.PermissionConstraints) []auth.Permission { + return []auth.Permission{revokePerm(branch.verb, c), revokePerm(auth.ActionUpdate, c)} + } + if branch.verb == auth.ActionUpdate { + perms = func(c *auth.PermissionConstraints) []auth.Permission { + return []auth.Permission{revokePerm(auth.ActionUpdate, c)} + } + } + t.Run(branch.name+"/non-USD under a cap is refused", func(t *testing.T) { + t.Parallel() + _, store, err := runResumeCap(t, resumeCapCase{creator: branch.creator, recs: eur, perms: perms(capOf(5000))}) + requireRevokeForbidden(t, err, "USD spend cap") + requireResumeNotTransitioned(t, store) + }) + t.Run(branch.name+"/mixed USD and non-USD under a cap is refused", func(t *testing.T) { + t.Parallel() + _, store, err := runResumeCap(t, resumeCapCase{creator: branch.creator, recs: mixed, perms: perms(capOf(5000))}) + requireRevokeForbidden(t, err, "USD spend cap") + requireResumeNotTransitioned(t, store) + }) + t.Run(branch.name+"/non-USD uncapped resumes", func(t *testing.T) { + t.Parallel() + _, _, err := runResumeCap(t, resumeCapCase{creator: branch.creator, recs: eur, perms: perms(nil)}) + require.NoError(t, err) + }) + } +} + +// A row whose recs total zero is refused (400) as approve refuses it, on both branches. +func TestResumeAmountCap_ZeroCommitmentRefused(t *testing.T) { + t.Parallel() + other := pauseResumeOtherUser + me := revokeConstraintsUser + zero := []config.RecommendationRecord{resumeCapRec(0, 0, "USD")} + for name, c := range map[string]resumeCapCase{ + "update-any": {creator: &other, recs: zero, perms: []auth.Permission{revokePerm(auth.ActionUpdate, nil), revokePerm(auth.ActionUpdateAny, capOf(500))}}, + "update": {creator: &me, recs: zero, perms: []auth.Permission{revokePerm(auth.ActionUpdate, capOf(500))}}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + _, store, err := runResumeCap(t, c) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.Error(), "zero total commitment") + requireResumeNotTransitioned(t, store) + }) + } +} + +// Deliberate behavior change (issue #1130): the zero-commitment guard applies +// to every caller, uncapped ones included, exactly as approve applies it. +func TestResumeAmountCap_ZeroCommitmentRefusedEvenWhenUncapped(t *testing.T) { + t.Parallel() + other := pauseResumeOtherUser + me := revokeConstraintsUser + zero := []config.RecommendationRecord{resumeCapRec(0, 0, "USD")} + for name, c := range map[string]resumeCapCase{ + "update-any": {creator: &other, recs: zero, perms: []auth.Permission{revokePerm(auth.ActionUpdate, nil), revokePerm(auth.ActionUpdateAny, nil)}}, + "update": {creator: &me, recs: zero, perms: []auth.Permission{revokePerm(auth.ActionUpdate, nil)}}, + "admin": {creator: &other, recs: zero, perms: []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + _, store, err := runResumeCap(t, c) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 400, ce.code) + requireResumeNotTransitioned(t, store) + }) + } +} + +// With no recommendations there is nothing to price: the amount check is +// skipped and the transition is pinned to the empty-recs state. +func TestResumeAmountCap_EmptyRecsSkipCheckAndPinTheTransition(t *testing.T) { + t.Parallel() + other := pauseResumeOtherUser + resp, store, err := runResumeCap(t, resumeCapCase{creator: &other, recs: []config.RecommendationRecord{}, + perms: []auth.Permission{revokePerm(auth.ActionUpdate, nil), revokePerm(auth.ActionUpdateAny, capOf(500))}}) + require.NoError(t, err) + assert.Equal(t, "resumed", resp.Status) + store.AssertCalled(t, "TransitionExecutionStatusPinned", mock.Anything, cancelExecID, []string{"paused"}, "pending", mock.Anything, true) +} + +// Plan-step rows have no creator: update-any or admin only. +func TestResumeAmountCap_PlanStepRow(t *testing.T) { + t.Parallel() + recs := noUpfrontRecs() + run := func(perms ...auth.Permission) (*StatusResponse, *MockConfigStore, error) { + exec := cancelRouteExec("paused") + exec.PlanID = "plan-1" + exec.CreatedByUserID = nil + exec.ScheduledDate = time.Now().Add(24 * time.Hour) + exec.Recommendations = recs + h, store := cancelRouteHandler(t, exec, perms...) + resumed := *exec + resumed.Status = "pending" + store.On("TransitionExecutionStatusPinned", mock.Anything, cancelExecID, []string{"paused"}, "pending", mock.Anything, false).Return(&resumed, nil).Maybe() + resp, err := h.resumePlannedPurchase(context.Background(), deletePlannedReq(), cancelExecID) + return resp, store, err + } + t.Run("update alone is refused", func(t *testing.T) { + t.Parallel() + _, store, err := run(revokePerm(auth.ActionUpdate, nil)) + requireRevokeForbidden(t, err, "another user's scheduled purchase") + requireResumeNotTransitioned(t, store) + }) + t.Run("update-any within its cap resumes", func(t *testing.T) { + t.Parallel() + _, _, err := run(revokePerm(auth.ActionUpdate, nil), revokePerm(auth.ActionUpdateAny, capOf(1200))) + require.NoError(t, err) + }) + t.Run("update-any above its cap is refused", func(t *testing.T) { + t.Parallel() + _, store, err := run(revokePerm(auth.ActionUpdate, nil), revokePerm(auth.ActionUpdateAny, capOf(500))) + requireRevokeForbidden(t, err, constraintDenied) + requireResumeNotTransitioned(t, store) + }) +} + +// A user API key is capped by its own constraints, not its uncapped owner's. +func TestResumeAmountCap_APIKeyFollowsKeyCap(t *testing.T) { + rt := keyScopeRoute{name: "resume", method: http.MethodPost, suffix: "/resume", status: "paused"} + keyPerms := func(cap float64) []auth.Permission { + return []auth.Permission{revokePerm(auth.ActionUpdate, capOf(cap))} + } + t.Run("key cap below the commitment is refused", func(t *testing.T) { + code, reached := serveKeyScope(t, rt, newKeyScopeAuth(keyPerms(50)), keyScopeOwnerID, keyHeaders()) + assert.Equal(t, http.StatusForbidden, code) + assert.False(t, reached) + }) + t.Run("key cap covering the commitment resumes", func(t *testing.T) { + _, reached := serveKeyScope(t, rt, newKeyScopeAuth(keyPerms(100)), keyScopeOwnerID, keyHeaders()) + assert.True(t, reached) + }) +} + +// The admin API key is unrestricted: no amount check. +func TestResumeAmountCap_AdminAPIKeyUnrestricted(t *testing.T) { + t.Parallel() + ctx := context.Background() + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + m := new(MockAuthService) + t.Cleanup(func() { m.AssertExpectations(t) }) + m.grantAdmin() + row := cancelRouteExec("paused") + row.ScheduledDate = time.Now().Add(24 * time.Hour) + row.Recommendations = []config.RecommendationRecord{resumeCapRec(1e9, 0, "USD")} + resumed := *row + resumed.Status = "pending" + store.On("GetExecutionByID", mock.Anything, cancelExecID).Return(row, nil) + store.On("TransitionExecutionStatusPinned", mock.Anything, cancelExecID, []string{"paused"}, "pending", mock.Anything, false).Return(&resumed, nil) + h := &Handler{config: store, auth: m, corsAllowedOrigin: "*", apiKey: "test-key"} + resp, err := h.HandleRequest(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"X-API-Key": "test-key", "Authorization": "Bearer t", "X-CSRF-Token": "c", "Content-Type": "application/json"}, + RequestContext: events.LambdaFunctionURLRequestContext{HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "POST", Path: "/api/purchases/planned/" + cancelExecID + "/resume"}}, + }) + require.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode) +} + +// A nil auth component fails closed with a 500 on the claimed-row path too. +func TestAuthorizeRowManagement_NilAuthIsServerError(t *testing.T) { + t.Parallel() + creator := revokeConstraintsUser + row := &config.PurchaseExecution{CreatedByUserID: &creator} + verb, err := (&Handler{}).authorizeRowManagement(context.Background(), &Session{UserID: creator}, row) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 500, ce.code) + assert.Empty(t, verb) +} From 3406a0b0191117cf24719f64333a0b9d008c4324 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 13:52:29 +0100 Subject: [PATCH 3/3] fix(api): check the resume cap on the row's full scope The update branch checked an amount-only constraint set, so an uncapped update grant scoped elsewhere satisfied the cap for any row. Both branches now use enforcePurchaseConstraints, the scoped set approve-own uses, and the enforcePurchaseConstraintSets extraction is reverted. Tests pin the mixed-grant case on update and update-any and the admin key on a zero-commitment row. Refs #1130 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_purchases.go | 32 +++------- ...andler_purchases_resume_amount_cap_test.go | 61 ++++++++++++++++--- 2 files changed, 62 insertions(+), 31 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 4e0dbaa33..6b6bcf753 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -380,21 +380,16 @@ func (h *Handler) resumePlannedPurchase(ctx context.Context, req *events.LambdaF } // enforceResumeAmountCap bounds the row's total commitment by the cap of the -// verb that authorized the resume (issue #1130). An update-any holder is -// checked on update-any's full scope and amount, as approve-any is. A creator -// (or any update holder) is checked on amount and currency only, with no new -// scope matching on update:purchases. The admin API key (empty verb) and a -// row without recommendations skip the check (recommendations attach later -// only to pending/notified rows and the scheduler re-checks the plan cap). +// verb that authorized the resume (issue #1130), checked on the row's full +// scope and amount as approve is: a grant scoped elsewhere must not lift the +// cap of the one that covers the row. The admin API key (empty verb) and a row +// without recommendations skip the check (recommendations attach later only +// to pending/notified rows and the scheduler re-checks the plan cap). func (h *Handler) enforceResumeAmountCap(ctx context.Context, session *Session, verb string, recs []config.RecommendationRecord) error { if verb == "" || len(recs) == 0 { return nil } - if verb == auth.ActionUpdateAny { - return h.enforcePurchaseConstraints(ctx, session, verb, recs) - } - total := purchaseConstraintSets(recs)[0].MaxPurchaseAmount - return h.enforcePurchaseConstraintSets(ctx, session, verb, recs, []auth.PermissionConstraints{{MaxPurchaseAmount: total}}) + return h.enforcePurchaseConstraints(ctx, session, verb, recs) } // requireResumeVetoWindow refuses to resume a recommendation-bearing row whose @@ -3312,21 +3307,14 @@ func parseExecutePurchaseBody(body string) (ExecutePurchaseRequest, error) { // USD, so a batch with any rec not provably priced in USD is checked by // requireUncappedPurchaseGrant instead of by its raw amount (issue #361). func (h *Handler) enforcePurchaseConstraints(ctx context.Context, session *Session, action string, recs []config.RecommendationRecord) error { - return h.enforcePurchaseConstraintSets(ctx, session, action, recs, purchaseConstraintSets(recs)) -} - -// enforcePurchaseConstraintSets is enforcePurchaseConstraints for a caller- -// chosen constraint set list: the zero-commitment guard, the USD rule for -// non-USD recs, then the permission check. sets must come from recs -// (purchaseConstraintSets) or carry the same total in MaxPurchaseAmount. -func (h *Handler) enforcePurchaseConstraintSets(ctx context.Context, session *Session, action string, recs []config.RecommendationRecord, sets []auth.PermissionConstraints) error { - if err := requireNonZeroCommitment(sets); err != nil { + constraintSets := purchaseConstraintSets(recs) + if err := requireNonZeroCommitment(constraintSets); err != nil { return err } if currency := nonUSDPurchaseCurrency(recs); currency != "" { - return h.requireUncappedPurchaseGrant(ctx, session, action, sets, currency) + return h.requireUncappedPurchaseGrant(ctx, session, action, constraintSets, currency) } - return h.requirePermissionConstraints(ctx, session, action, "purchases", sets) + return h.requirePermissionConstraints(ctx, session, action, "purchases", constraintSets) } // purchaseConstraintSets builds one auth.PermissionConstraints per diff --git a/internal/api/handler_purchases_resume_amount_cap_test.go b/internal/api/handler_purchases_resume_amount_cap_test.go index 135487179..8832e2640 100644 --- a/internal/api/handler_purchases_resume_amount_cap_test.go +++ b/internal/api/handler_purchases_resume_amount_cap_test.go @@ -171,16 +171,45 @@ func TestResumeAmountCap_ScopedUpdateAnyCreatorUncappedUpdateResumes(t *testing. assert.Equal(t, "resumed", resp.Status) } -// The update branch bounds the amount only: a region-scoped update grant must -// not newly refuse a creator's own row. -func TestResumeAmountCap_UpdateBranchAddsNoScope(t *testing.T) { +// The update branch is checked on the row's scope, as approve-own is: an +// update grant scoped to another region does not cover the row. +func TestResumeAmountCap_UpdateBranchUsesRowScope(t *testing.T) { t.Parallel() me := revokeConstraintsUser scoped := &auth.PermissionConstraints{Regions: []string{"eastus"}, MaxPurchaseAmount: 5000} - resp, _, err := runResumeCap(t, resumeCapCase{creator: &me, recs: noUpfrontRecs(), + _, store, err := runResumeCap(t, resumeCapCase{creator: &me, recs: noUpfrontRecs(), perms: []auth.Permission{revokePerm(auth.ActionUpdate, scoped)}}) - require.NoError(t, err) - assert.Equal(t, "resumed", resp.Status) + requireRevokeForbidden(t, err, constraintDenied) + requireResumeNotTransitioned(t, store) +} + +// An uncapped grant scoped elsewhere must not lift the cap of the grant that +// covers the row (the azure/westeurope row totals 1200), on either verb. +func TestResumeAmountCap_UncappedGrantElsewhereDoesNotLiftCap(t *testing.T) { + t.Parallel() + me := revokeConstraintsUser + other := pauseResumeOtherUser + for _, tc := range []struct { + name string + verb string + creator *string + }{ + {"update, creator's own row", auth.ActionUpdate, &me}, + {"update-any, another user's row", auth.ActionUpdateAny, &other}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + capped := revokePerm(tc.verb, &auth.PermissionConstraints{Providers: []string{"azure"}, MaxPurchaseAmount: 500}) + elsewhere := revokePerm(tc.verb, &auth.PermissionConstraints{Regions: []string{"eastus"}}) + perms := []auth.Permission{capped, elsewhere} + if tc.verb == auth.ActionUpdateAny { + perms = append(perms, revokePerm(auth.ActionUpdate, nil)) // so the request passes the update gate + } + _, store, err := runResumeCap(t, resumeCapCase{creator: tc.creator, recs: noUpfrontRecs(), perms: perms}) + requireRevokeForbidden(t, err, constraintDenied) + requireResumeNotTransitioned(t, store) + }) + } } func TestResumeAmountCap_NonUSDUnderCap(t *testing.T) { @@ -332,9 +361,23 @@ func TestResumeAmountCap_APIKeyFollowsKeyCap(t *testing.T) { }) } -// The admin API key is unrestricted: no amount check. +// The admin API key is unrestricted: no amount check and, as on approve, no +// zero-commitment refusal. func TestResumeAmountCap_AdminAPIKeyUnrestricted(t *testing.T) { t.Parallel() + for name, rec := range map[string]config.RecommendationRecord{ + "above any cap": resumeCapRec(1e9, 0, "USD"), + "zero commitment": resumeCapRec(0, 0, "USD"), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, 200, resumeAsAdminKey(t, rec)) + }) + } +} + +func resumeAsAdminKey(t *testing.T, rec config.RecommendationRecord) int { + t.Helper() ctx := context.Background() store := new(MockConfigStore) t.Cleanup(func() { store.AssertExpectations(t) }) @@ -343,7 +386,7 @@ func TestResumeAmountCap_AdminAPIKeyUnrestricted(t *testing.T) { m.grantAdmin() row := cancelRouteExec("paused") row.ScheduledDate = time.Now().Add(24 * time.Hour) - row.Recommendations = []config.RecommendationRecord{resumeCapRec(1e9, 0, "USD")} + row.Recommendations = []config.RecommendationRecord{rec} resumed := *row resumed.Status = "pending" store.On("GetExecutionByID", mock.Anything, cancelExecID).Return(row, nil) @@ -355,7 +398,7 @@ func TestResumeAmountCap_AdminAPIKeyUnrestricted(t *testing.T) { Method: "POST", Path: "/api/purchases/planned/" + cancelExecID + "/resume"}}, }) require.NoError(t, err) - assert.Equal(t, 200, resp.StatusCode) + return resp.StatusCode } // A nil auth component fails closed with a 500 on the claimed-row path too.