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"`