From 9079297937a617901a5c1f3e30dbcf14c9f09465 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 00:28:15 +0100 Subject: [PATCH] fix(api): refuse a planned purchase cancel whose plan the caller cannot disable DELETE /api/purchases/planned/{id} cancels the execution and disables its plan. Since #621, a scoped caller whose scope covers only some of the plan's accounts got 200 "canceled" while the plan stayed enabled, so the scheduler kept buying for every plan account and the Plans page told the user nothing. Check the plan's every-account mutation scope before anything is written and refuse with a 403 that says why. The disable re-checks after the cancel; if the plan's accounts changed in between, the request returns 409 saying the purchase was canceled but the plan was not disabled. plan_disabled is now true whenever present. Unrestricted callers are unchanged. Closes #623 Co-Authored-By: Claude Opus 5.5 --- .../api/delete_planned_purchase_scope_test.go | 109 +++++++++++++++--- internal/api/handler_purchases.go | 81 +++++++++---- internal/api/openapi.yaml | 16 ++- internal/api/types.go | 7 +- 4 files changed, 167 insertions(+), 46 deletions(-) diff --git a/internal/api/delete_planned_purchase_scope_test.go b/internal/api/delete_planned_purchase_scope_test.go index fd2c5f396..d7dfa1bdf 100644 --- a/internal/api/delete_planned_purchase_scope_test.go +++ b/internal/api/delete_planned_purchase_scope_test.go @@ -18,6 +18,10 @@ import ( // then disables its plan. A single-account child clears requireExecutionAccess // on its own account while the plan spans accounts the caller cannot see, so // the plan disable needs the every-account check of the plan mutation routes. +// +// Issue #623: when that check fails, the whole request is refused with a 403 +// before anything is written, instead of canceling the execution and silently +// leaving the plan enabled. const plannedDeleteExecID = "55555555-5555-4555-8555-555555555555" @@ -82,18 +86,30 @@ func (f plannedDeleteFixture) planDisables() int { return n } -func TestRouterDispatch_DeletePlannedPurchase_PartiallyScopedKeepsPlanEnabled(t *testing.T) { +// requireRefusedBeforeCancel asserts the request was refused with a 403 that +// says why, and that neither the execution nor the plan was touched. +func (f plannedDeleteFixture) requireRefusedBeforeCancel(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error, got %T: %v", err, err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "every account on the plan") + store := f.store + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + assert.Zero(t, f.planDisables(), "a refused request must not disable the plan") +} + +// Issue #623: a caller who cannot disable the plan is refused before the +// cancel, so the plan is never left enabled behind a 200. +func TestRouterDispatch_DeletePlannedPurchase_PartiallyScopedRefusedBeforeCancel(t *testing.T) { f := newPlannedDeleteFixture(t, []config.CloudAccount{inScopeAccount(), outOfScopeAccount()}, scopedInAccount) store := f.store resp, err := f.delete(t) - require.NoError(t, err) - assert.Equal(t, "canceled", resp.Status) - require.NotNil(t, resp.PlanDisabled) - assert.False(t, *resp.PlanDisabled) - store.AssertNumberOfCalls(t, "TransitionExecutionStatus", 1) - assert.Zero(t, f.planDisables(), "a plan spanning an out-of-scope account must stay enabled") + assert.Nil(t, resp) + f.requireRefusedBeforeCancel(t, err) store.AssertNotCalled(t, "GetPurchasePlan", mock.Anything, mock.Anything) } @@ -127,25 +143,21 @@ func TestRouterDispatch_DeletePlannedPurchase_UnrestrictedDisablesPlanWithoutSco } // A plan with no accounts is hidden from scoped callers, so the disable is -// skipped like the partially scoped case. -func TestRouterDispatch_DeletePlannedPurchase_ZeroAccountPlanKeepsPlanEnabled(t *testing.T) { +// refused like the partially scoped case. +func TestRouterDispatch_DeletePlannedPurchase_ZeroAccountPlanRefusedBeforeCancel(t *testing.T) { f := newPlannedDeleteFixture(t, nil, scopedInAccount) - store := f.store - resp, err := f.delete(t) + _, err := f.delete(t) - require.NoError(t, err) - require.NotNil(t, resp.PlanDisabled) - assert.False(t, *resp.PlanDisabled) - store.AssertNumberOfCalls(t, "TransitionExecutionStatus", 1) - assert.Zero(t, f.planDisables()) + f.requireRefusedBeforeCancel(t, err) } // A store failure while resolving the plan's accounts is not a scope refusal: -// it surfaces and the plan is not disabled. -func TestRouterDispatch_DeletePlannedPurchase_PlanAccountLookupFailureSurfaces(t *testing.T) { +// it surfaces as a server error, and nothing is canceled or disabled. +func TestRouterDispatch_DeletePlannedPurchase_PlanAccountLookupFailureSurfacesBeforeCancel(t *testing.T) { f := newPlannedDeleteFixture(t, nil, scopedInAccount) - f.store.GetPlanAccountsFn = func(context.Context, string) ([]config.CloudAccount, error) { + store := f.store + store.GetPlanAccountsFn = func(context.Context, string) ([]config.CloudAccount, error) { return nil, errors.New("db down") } @@ -153,9 +165,68 @@ func TestRouterDispatch_DeletePlannedPurchase_PlanAccountLookupFailureSurfaces(t require.Error(t, err) assert.False(t, IsNotFoundError(err)) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a store failure must not read as a client refusal: %v", err) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) assert.Zero(t, f.planDisables()) } +// The plan's accounts can change between the pre-cancel check and the +// disable (an admin attaches an out-of-scope account mid-request). The +// disable re-checks and, when it is refused, reports a 409 naming the +// half-done state instead of a 200 that hides it. +func TestRouterDispatch_DeletePlannedPurchase_PlanAccountsChangeDuringCancel(t *testing.T) { + f := newPlannedDeleteFixture(t, nil, scopedInAccount) + lookups := 0 + f.store.GetPlanAccountsFn = func(context.Context, string) ([]config.CloudAccount, error) { + lookups++ + if lookups == 1 { + return []config.CloudAccount{inScopeAccount()}, nil + } + return []config.CloudAccount{inScopeAccount(), outOfScopeAccount()}, nil + } + + resp, err := f.delete(t) + + assert.Nil(t, resp) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error, got %T: %v", err, err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, err.Error(), "was canceled") + assert.Contains(t, err.Error(), "not disabled") + assert.Equal(t, 2, lookups, "the disable must re-check the plan's accounts after the cancel") + f.store.AssertNumberOfCalls(t, "TransitionExecutionStatus", 1) + assert.Zero(t, f.planDisables(), "the plan must not be disabled once it spans an out-of-scope account") +} + +// A scoped caller canceling an execution with no plan has nothing to disable, +// so no plan scope check runs and the cancel succeeds. +func TestRouterDispatch_DeletePlannedPurchase_ScopedNoPlanCancelsWithoutPlanCheck(t *testing.T) { + f := newPlannedDeleteFixture(t, nil, scopedInAccount) + store := f.store + account := scopedInAccount + noPlan := &config.PurchaseExecution{ExecutionID: plannedDeleteExecID, Status: "pending", CloudAccountID: &account} + canceled := *noPlan + canceled.Status = config.StatusCanceled + store.ExpectedCalls = nil + store.On("GetExecutionByID", mock.Anything, plannedDeleteExecID).Return(noPlan, nil) + store.On("TransitionExecutionStatus", mock.Anything, plannedDeleteExecID, mock.Anything, config.StatusCanceled, mock.Anything).Return(&canceled, nil) + lookups := 0 + store.GetPlanAccountsFn = func(context.Context, string) ([]config.CloudAccount, error) { + lookups++ + return nil, nil + } + + resp, err := f.delete(t) + + require.NoError(t, err) + assert.Equal(t, "canceled", resp.Status) + assert.Nil(t, resp.PlanDisabled) + assert.Zero(t, lookups) + store.AssertNumberOfCalls(t, "TransitionExecutionStatus", 1) +} + // The execution gate still comes first: a caller who holds none of the // execution's accounts is refused before anything is canceled or disabled. func TestRouterDispatch_DeletePlannedPurchase_OutOfScopeExecutionRefusedBeforeAnyWrite(t *testing.T) { diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index d90c7282b..af7c61b96 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -605,6 +605,10 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF if err != nil { return nil, err } + err = h.requirePlannedCancelPlanScope(ctx, session, executionID) + if err != nil { + return nil, err + } canceled, err := h.cancelOrRecoverExecution(ctx, executionID, resolveCreatorUserID(session)) if err != nil { @@ -615,38 +619,75 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF // reflects the disable action immediately. Issue #774: previously the // execution was canceled but plan.enabled was left true, causing // inconsistent state between the Scheduled Purchases and Plans views. - // - // A single-account child passes requireExecutionAccess on its own account - // while its plan may span accounts outside the caller's scope (issue #520). - // Disabling the plan changes those accounts too, so it needs the same - // every-account check as the plan mutation routes. Without it the - // execution is still canceled and the plan is left enabled. resp := &DeletePlannedPurchaseResponse{Status: "canceled"} if canceled.PlanID != "" { - disabled, err := h.disablePlanIfInScope(ctx, session, canceled.PlanID) - if err != nil { + if err := h.disablePlanAfterCancel(ctx, session, canceled.PlanID); err != nil { return nil, err } + disabled := true resp.PlanDisabled = &disabled } return resp, nil } -// disablePlanIfInScope disables the plan only when the session may mutate -// every plan account, and reports whether it did. A scope refusal is not an -// error: the caller's cancel stands and the plan is left enabled. -func (h *Handler) disablePlanIfInScope(ctx context.Context, session *Session, planID string) (bool, error) { - if err := h.requirePlanMutationAccess(ctx, session, planID); err != nil { - if IsNotFoundError(err) { - return false, nil - } - return false, err +// errPlannedCancelPlanOutOfScope refuses a planned purchase cancel whose plan +// disable the caller may not perform. The caller can already see the +// execution and its plan, so the 403 confirms nothing new except that the +// plan reaches accounts it cannot act on, which it needs to know. +var errPlannedCancelPlanOutOfScope = NewClientError(403, + "permission denied: canceling this scheduled purchase also disables its plan, "+ + "which requires access to every account on the plan; nothing was changed. "+ + "Ask an administrator with access to all of the plan's accounts to disable it") + +// requirePlannedCancelPlanScope runs before anything is written. Canceling a +// planned purchase also disables its plan, and a single-account child passes +// requireExecutionAccess on its own account while its plan may span accounts +// outside the caller's scope (issue #520). Disabling the plan changes those +// accounts too, so it needs the every-account check of the plan mutation +// routes. When that check fails the whole request is refused (issue #623): +// canceling and silently leaving the plan enabled would keep the scheduler +// buying on every plan account while the caller believes it stopped. +// Unrestricted sessions return before any store read. +func (h *Handler) requirePlannedCancelPlanScope(ctx context.Context, session *Session, executionID string) error { + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return fmt.Errorf("failed to get allowed accounts: %w", err) } - if err := h.disablePlan(ctx, planID); err != nil { - return false, err + if scope.AllowsAll() { + return nil + } + execution, err := h.loadPlannedExecution(ctx, executionID) + if err != nil { + return err + } + if execution.PlanID == "" { + return nil + } + err = h.requirePlanMutationAccess(ctx, session, execution.PlanID) + if IsNotFoundError(err) { + return errPlannedCancelPlanOutOfScope + } + return err +} + +// disablePlanAfterCancel re-runs the every-account check and disables the +// plan. requirePlannedCancelPlanScope already passed, but the plan's accounts +// can change before this point (an out-of-scope account attached while the +// request is in flight). That refusal comes after the cancel, so it is a 409 +// that says so rather than a 200 that hides the plan staying enabled. +func (h *Handler) disablePlanAfterCancel(ctx context.Context, session *Session, planID string) error { + err := h.requirePlanMutationAccess(ctx, session, planID) + if IsNotFoundError(err) { + return NewClientError(409, + "this scheduled purchase was canceled, but its plan was not disabled: "+ + "the plan changed during the request and now requires access to accounts outside yours. "+ + "Ask an administrator with access to all of the plan's accounts to disable it") + } + if err != nil { + return err } - return true, nil + return h.disablePlan(ctx, planID) } // cancelOrRecoverExecution transitions the execution to "canceled" if it is diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 0865e46c3..cf2edd4dd 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1071,6 +1071,12 @@ paths: operationId: deletePlannedPurchase tags: [PlannedPurchases] summary: Cancel and delete a planned purchase + description: >- + Cancels the execution and disables its plan. A caller scoped to some + accounts must have access to every account on the plan; otherwise the + request is refused with 403 before anything changes. If the plan's + accounts change during the request so that the disable is refused + after the cancel, the response is 409 and the plan stays enabled. parameters: - $ref: '#/components/parameters/CSRFToken' responses: @@ -1086,6 +1092,8 @@ paths: $ref: '#/components/responses/Forbidden' '404': $ref: '#/components/responses/NotFound' + '409': + $ref: '#/components/responses/Conflict' # ---- Generic Purchase Details ------------------------------------------- /api/purchases/{id}: @@ -2769,10 +2777,10 @@ components: plan_disabled: type: boolean description: >- - Present when the execution belongs to a plan. True when the plan - was disabled; false when it was left enabled because it spans - accounts outside the caller's scope. Omitted for executions - without a plan. + Present, and true, when the execution belongs to a plan, which + the request disabled. Omitted for executions without a plan. A + plan the caller may not disable fails the request instead of + returning false. StatusMessageResponse: type: object diff --git a/internal/api/types.go b/internal/api/types.go index 5e48ce2f1..9ff2a91f1 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -441,9 +441,10 @@ type StatusResponse struct { } // DeletePlannedPurchaseResponse is the body of DELETE /api/purchases/planned/{id}. -// PlanDisabled is omitted when the execution has no plan, true when the plan -// was disabled, and false when the plan was left enabled because it spans -// accounts outside the caller's scope (issue #520). +// PlanDisabled is omitted when the execution has no plan and true when the +// plan was disabled. A plan the caller may not disable fails the request +// (403 before the cancel, 409 if the plan changed after it) instead of +// reporting false (issues #520, #623). type DeletePlannedPurchaseResponse struct { Status string `json:"status"` PlanDisabled *bool `json:"plan_disabled,omitempty"`