fix(api): refuse a planned purchase cancel whose plan the caller cannot disable

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,35 +143,90 @@ 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")
 	}
 
 	_, err := f.delete(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) {

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

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

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