@@ -693,10 +722,10 @@ function renderPlannedPurchaseRow(purchase: PlannedPurchase): string {
| ${formatCurrency(purchase.estimated_savings)}/mo |
${escapeHtml(purchase.status)} |
- ${canManagePlan && canRun ? `` : ''}
- ${canManagePlan && isPending ? `` : ''}
- ${canManagePlan && isPaused ? `` : ''}
- ${canManagePlan ? `` : ''}
+ ${canRunPurchase ? `` : ''}
+ ${canPauseOrResumePurchase && isPending ? `` : ''}
+ ${canPauseOrResumePurchase && isPaused ? `` : ''}
+ ${canEditPlan ? `` : ''}
${canDisablePlan ? `` : ''}
|
diff --git a/frontend/src/types.ts b/frontend/src/types.ts
index 5b37e7fe3..0413014d1 100644
--- a/frontend/src/types.ts
+++ b/frontend/src/types.ts
@@ -47,6 +47,11 @@ export interface UpcomingPurchase {
step_number: number;
total_steps: number;
estimated_savings: number;
+ // created_by_user_id mirrors the field on api.UpcomingPurchase so the
+ // dashboard widget can apply the issue-#950 creator-scope ownership
+ // gate on the Cancel button. Optional: legacy / scheduler-tick rows
+ // ship NULL here.
+ created_by_user_id?: string;
}
// Recommendations types
diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go
index 2321355dd..56a897f8a 100644
--- a/internal/api/handler_dashboard.go
+++ b/internal/api/handler_dashboard.go
@@ -394,6 +394,7 @@ func upcomingFromExecution(plan *config.PurchasePlan, exec *config.PurchaseExecu
StepNumber: exec.StepNumber,
TotalSteps: plan.RampSchedule.TotalSteps,
EstimatedSavings: exec.EstimatedSavings,
+ CreatedByUserID: exec.CreatedByUserID,
}
}
diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go
index ec13d43e9..181b75f63 100644
--- a/internal/api/handler_dashboard_test.go
+++ b/internal/api/handler_dashboard_test.go
@@ -429,6 +429,66 @@ func TestHandler_getUpcomingPurchases(t *testing.T) {
assert.Equal(t, 2, second.StepNumber)
}
+// TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID is the
+// issue-#950 follow-up regression: the dashboard widget on the frontend
+// applies a creator-scope ownership gate on the Cancel button (mirrors
+// the Plans page); it can only do so if the backend ships
+// created_by_user_id on every row. Pre-fix the field was absent, so the
+// widget defaulted to "no owner known" and either showed Cancel for
+// everyone (when ungated) or for nobody (when gated) -- both wrong.
+func TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID(t *testing.T) {
+ ctx := context.Background()
+ mockStore := new(MockConfigStore)
+
+ scheduled := time.Now().AddDate(0, 0, 7)
+ plan := config.PurchasePlan{
+ ID: "11111111-1111-1111-1111-111111111111",
+ Name: "Owned Plan",
+ Enabled: true,
+ Services: map[string]config.ServiceConfig{
+ "aws/ec2": {Provider: "aws", Service: "ec2"},
+ },
+ RampSchedule: config.RampSchedule{TotalSteps: 4},
+ }
+ creator := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"
+ pending := []config.PurchaseExecution{
+ {
+ ExecutionID: "11112222-3333-4444-5555-666677778888",
+ PlanID: plan.ID,
+ Status: "pending",
+ ScheduledDate: scheduled,
+ StepNumber: 1,
+ CreatedByUserID: &creator,
+ },
+ {
+ // Legacy / scheduler-tick row: NULL creator. Must serialise as
+ // no created_by_user_id field (omitempty on the JSON tag) so
+ // the frontend treats it as out-of-reach for non-update-any
+ // users -- the documented #950 behaviour.
+ ExecutionID: "99998888-7777-6666-5555-444433332222",
+ PlanID: plan.ID,
+ Status: "pending",
+ ScheduledDate: scheduled.AddDate(0, 0, 7),
+ StepNumber: 2,
+ CreatedByUserID: nil,
+ },
+ }
+
+ mockStore.On("GetPendingExecutions", ctx).Return(pending, nil)
+ mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return([]config.PurchasePlan{plan}, nil)
+
+ mockAuth, req := adminDashboardReq(ctx)
+ handler := &Handler{auth: mockAuth, config: mockStore}
+
+ result, err := handler.getUpcomingPurchases(ctx, req)
+ require.NoError(t, err)
+ require.Len(t, result.Purchases, 2)
+
+ require.NotNil(t, result.Purchases[0].CreatedByUserID, "owned-row CreatedByUserID must propagate")
+ assert.Equal(t, creator, *result.Purchases[0].CreatedByUserID)
+ assert.Nil(t, result.Purchases[1].CreatedByUserID, "legacy NULL-creator row must stay nil")
+}
+
// TestHandler_getUpcomingPurchases_OrphanExecutionSkipped guards against the
// "execution row with deleted parent plan" cleanup-gap edge case: rather
// than crash, the widget hides the orphan. Cleanup is a separate concern.
diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go
index 15f7cdb58..eed686d06 100644
--- a/internal/api/handler_plans.go
+++ b/internal/api/handler_plans.go
@@ -289,9 +289,20 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La
// pointer). A retry would then duplicate rows 1-3. WithTx makes
// both classes of corruption impossible — the caller can safely
// retry on transient errors knowing nothing was committed.
+ //
+ // Issue #950: stamp the session user onto each new execution's
+ // created_by_user_id so the per-row creator-scope ownership gate
+ // (authorizeExecutionManagement) recognises the actor who scheduled
+ // the purchases as their owner. Without this the rows ship NULL and
+ // are unreachable for pause / resume / run / delete by anyone except
+ // admins / update-any holders, including the user who just clicked
+ // "Create planned purchases" for their own plan. Admin-API-key and
+ // non-UUID sessions resolve to nil, matching the executePurchase /
+ // retry paths and the migration-000041 fail-closed policy.
+ creator := resolveCreatorUserID(session)
created := 0
if err := h.config.WithTx(ctx, func(tx pgx.Tx) error {
- n, txErr := h.createPurchaseExecutionsTx(ctx, tx, plan, planID, req.Count, startDate)
+ n, txErr := h.createPurchaseExecutionsTx(ctx, tx, plan, planID, req.Count, startDate, creator)
if txErr != nil {
return txErr
}
@@ -344,7 +355,14 @@ func (h *Handler) getPlanForPurchaseCreation(ctx context.Context, planID string)
// Returns the number of rows that would have been committed had the
// loop completed — used for the user-visible response on success;
// undefined (and unused) on error since the rollback voids them all.
-func (h *Handler) createPurchaseExecutionsTx(ctx context.Context, tx pgx.Tx, plan *config.PurchasePlan, planID string, count int, startDate time.Time) (int, error) {
+//
+// creator carries the session user's UUID (or nil for the admin-API-key /
+// non-UUID-session paths) and is stamped onto every inserted row's
+// created_by_user_id so the issue-#950 ownership gate downstream can
+// recognise the actor as the rightful manager. A nil value mirrors the
+// migration-000041 fail-closed semantics: legacy / unattributed rows are
+// reachable only by admin / update-any holders.
+func (h *Handler) createPurchaseExecutionsTx(ctx context.Context, tx pgx.Tx, plan *config.PurchasePlan, planID string, count int, startDate time.Time, creator *string) (int, error) {
intervalDays := plan.RampSchedule.StepIntervalDays
if intervalDays == 0 {
intervalDays = 7 // Default to weekly if not set
@@ -359,12 +377,13 @@ func (h *Handler) createPurchaseExecutionsTx(ctx context.Context, tx pgx.Tx, pla
return created, fmt.Errorf("failed to generate approval token (row %d/%d): %w", created+1, count, err)
}
execution := &config.PurchaseExecution{
- PlanID: planID,
- ExecutionID: uuid.New().String(),
- Status: "pending",
- StepNumber: plan.RampSchedule.CurrentStep + i + 1,
- ScheduledDate: scheduledDate,
- ApprovalToken: approvalToken,
+ PlanID: planID,
+ ExecutionID: uuid.New().String(),
+ Status: "pending",
+ StepNumber: plan.RampSchedule.CurrentStep + i + 1,
+ ScheduledDate: scheduledDate,
+ ApprovalToken: approvalToken,
+ CreatedByUserID: creator,
}
if err := h.config.SavePurchaseExecutionTx(ctx, tx, execution); err != nil {
diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go
index 40bdf2387..3f7e3e29b 100644
--- a/internal/api/handler_plans_test.go
+++ b/internal/api/handler_plans_test.go
@@ -445,6 +445,117 @@ func TestHandler_createPlannedPurchases(t *testing.T) {
assert.Equal(t, 3, result.Created)
}
+// TestHandler_createPlannedPurchases_StampsCreator is the issue-#950 regression
+// guard: every execution row written through POST /api/plans/{id}/purchases
+// MUST carry the session user's UUID in CreatedByUserID, otherwise the
+// per-row ownership gate (authorizeExecutionManagement in
+// handler_purchases.go) downstream cannot recognise the actor as the
+// rightful manager and the user who just scheduled the purchases is
+// locked out of pause / resume / run / delete until an admin steps in.
+//
+// Pre-fix the field shipped zero-valued (nil pointer), making every
+// freshly scheduled row look like a legacy unattributed entry.
+func TestHandler_createPlannedPurchases_StampsCreator(t *testing.T) {
+ ctx := context.Background()
+ mockStore := new(MockConfigStore)
+ mockAuth := new(MockAuthService)
+
+ const userID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"
+ userSession := &Session{UserID: userID, Email: "u@example.com"}
+
+ plan := &config.PurchasePlan{
+ ID: "11111111-1111-1111-1111-111111111111",
+ Name: "Test Plan",
+ RampSchedule: config.RampSchedule{
+ StepIntervalDays: 7,
+ CurrentStep: 0,
+ },
+ }
+
+ mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil)
+ mockAuth.grantAdmin()
+ mockStore.On("GetPurchasePlan", ctx, "11111111-1111-1111-1111-111111111111").Return(plan, nil)
+
+ // Capture every saved execution's CreatedByUserID so we can assert
+ // the field is stamped on each row in the batch (not just the first).
+ var savedCreators []*string
+ mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).
+ Run(func(args mock.Arguments) {
+ exec := args.Get(1).(*config.PurchaseExecution)
+ savedCreators = append(savedCreators, exec.CreatedByUserID)
+ }).
+ Return(nil).Times(3)
+ mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil)
+
+ handler := &Handler{config: mockStore, auth: mockAuth}
+
+ body := `{"count": 3, "start_date": "2024-12-01"}`
+ req := &events.LambdaFunctionURLRequest{
+ Headers: map[string]string{"Authorization": "Bearer user-token"},
+ Body: body,
+ }
+ result, err := handler.createPlannedPurchases(ctx, req, "11111111-1111-1111-1111-111111111111")
+ require.NoError(t, err)
+ assert.Equal(t, 3, result.Created)
+
+ require.Len(t, savedCreators, 3, "expected 3 saved executions")
+ for i, c := range savedCreators {
+ require.NotNil(t, c, "execution %d shipped a nil CreatedByUserID (issue #950 regression)", i)
+ assert.Equal(t, userID, *c, "execution %d shipped the wrong CreatedByUserID", i)
+ }
+}
+
+// TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil locks in that
+// the stateless admin-API-key path (UserID == apiKeyAdminUserID, not a UUID)
+// stamps NULL rather than the literal sentinel. resolveCreatorUserID rejects
+// non-UUID UserIDs so the FK to users stays valid; the row falls through to
+// the admin / update-any management path exactly like a legacy scheduler-
+// created row would.
+func TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil(t *testing.T) {
+ ctx := context.Background()
+ mockStore := new(MockConfigStore)
+ mockAuth := new(MockAuthService)
+
+ apiKeySession := &Session{UserID: apiKeyAdminUserID, Email: "admin-api-key"}
+
+ plan := &config.PurchasePlan{
+ ID: "11111111-1111-1111-1111-111111111111",
+ Name: "Test Plan",
+ RampSchedule: config.RampSchedule{
+ StepIntervalDays: 7,
+ CurrentStep: 0,
+ },
+ }
+
+ mockAuth.On("ValidateSession", ctx, "api-key").Return(apiKeySession, nil)
+ mockAuth.grantAdmin()
+ mockStore.On("GetPurchasePlan", ctx, "11111111-1111-1111-1111-111111111111").Return(plan, nil)
+
+ var savedCreators []*string
+ mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).
+ Run(func(args mock.Arguments) {
+ exec := args.Get(1).(*config.PurchaseExecution)
+ savedCreators = append(savedCreators, exec.CreatedByUserID)
+ }).
+ Return(nil).Times(2)
+ mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil)
+
+ handler := &Handler{config: mockStore, auth: mockAuth}
+
+ body := `{"count": 2, "start_date": "2024-12-01"}`
+ req := &events.LambdaFunctionURLRequest{
+ Headers: map[string]string{"Authorization": "Bearer api-key"},
+ Body: body,
+ }
+ _, err := handler.createPlannedPurchases(ctx, req, "11111111-1111-1111-1111-111111111111")
+ require.NoError(t, err)
+
+ require.Len(t, savedCreators, 2)
+ for i, c := range savedCreators {
+ assert.Nil(t, c, "execution %d should ship a nil CreatedByUserID for the admin-API-key path", i)
+ }
+}
+
// TestHandler_createPlannedPurchases_MidLoopFailureRollsBack verifies
// the partial-failure regression CodeRabbit flagged: a save failure on
// row N must NOT leave rows 1..N-1 persisted (they would be retried as
diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go
index 6df17dea0..397d6db75 100644
--- a/internal/api/handler_purchases.go
+++ b/internal/api/handler_purchases.go
@@ -188,9 +188,65 @@ func buildPlannedPurchase(plan *config.PurchasePlan, exec *config.PurchaseExecut
Status: exec.Status,
StepNumber: exec.StepNumber,
TotalSteps: plan.RampSchedule.TotalSteps,
+ CreatedByUserID: exec.CreatedByUserID,
}
}
+// authorizeExecutionManagement enforces creator-scope ownership on the
+// scheduled-purchase management handlers (pause / resume / run / delete),
+// closing the authz hole in issue #950 where any holder of update:purchases
+// could act on another user's scheduled purchase. It runs AFTER the
+// per-handler verb check (update / execute / delete) and the account-scope
+// check (requireExecutionAccess); those still apply unchanged.
+//
+// Gate logic (mirrors authorizeSessionCancel / authorizeSessionApprove):
+// - stateless admin API key: always permitted (apiKeyAdminUserID sentinel).
+// - update-any:purchases: permitted regardless of creator. Administrators-
+// group users pass here because {admin, *} matches ActionUpdateAny
+// (update-any is not in adminCarvedOuts).
+// - otherwise: permitted only when the execution's CreatedByUserID is
+// non-nil and equals a non-empty session.UserID (the caller created it).
+// Legacy rows with a NULL creator are out of reach for non-update-any
+// users, matching the cancel-own / approve-own / retry-own model.
+// - any other case: 403 fail-closed. A nil auth component is a 500 per
+// feedback_fail_closed_middleware.md.
+//
+// Only fetches the execution on the creator-match path: admin and update-any
+// callers are authorised without a store round-trip (and admin sessions have
+// unrestricted access, so requireExecutionAccess skipped the fetch too).
+func (h *Handler) authorizeExecutionManagement(ctx context.Context, session *Session, executionID string) error {
+ if session.UserID == apiKeyAdminUserID {
+ return nil
+ }
+ if h.auth == nil {
+ return NewClientError(500, "authentication service not configured")
+ }
+
+ hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionUpdateAny, auth.ResourcePurchases)
+ if err != nil {
+ return fmt.Errorf("permission check failed: %w", err)
+ }
+ if hasAny {
+ return nil
+ }
+
+ execution, err := h.config.GetExecutionByID(ctx, executionID)
+ if err != nil {
+ return fmt.Errorf("failed to get execution: %w", err)
+ }
+ if execution == nil {
+ return errNotFound
+ }
+
+ // Creator match: both IDs must be non-empty and equal. An empty-string
+ // collision (legacy NULL creator + missing session UserID) must not
+ // grant access.
+ if session.UserID == "" || execution.CreatedByUserID == nil || *execution.CreatedByUserID != session.UserID {
+ return NewClientError(403, "permission denied: cannot manage another user's scheduled purchase")
+ }
+ return nil
+}
+
func (h *Handler) pausePlannedPurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, executionID string) (*StatusResponse, error) {
if err := validateUUID(executionID); err != nil {
return nil, err
@@ -203,6 +259,9 @@ func (h *Handler) pausePlannedPurchase(ctx context.Context, req *events.LambdaFu
if err := h.requireExecutionAccess(ctx, session, executionID); err != nil {
return nil, err
}
+ if err := h.authorizeExecutionManagement(ctx, session, executionID); err != nil {
+ return nil, err
+ }
// Atomically transition to paused
if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "running"}, "paused"); err != nil {
@@ -224,6 +283,9 @@ func (h *Handler) resumePlannedPurchase(ctx context.Context, req *events.LambdaF
if err := h.requireExecutionAccess(ctx, session, executionID); err != nil {
return nil, err
}
+ if err := h.authorizeExecutionManagement(ctx, session, executionID); err != nil {
+ return nil, err
+ }
// Atomically transition from paused back to pending
if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"paused"}, "pending"); err != nil {
@@ -245,6 +307,9 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc
if err := h.requireExecutionAccess(ctx, session, executionID); err != nil {
return nil, err
}
+ if err := h.authorizeExecutionManagement(ctx, session, executionID); err != nil {
+ return nil, err
+ }
// Atomically transition to running — only one concurrent caller can succeed.
// TransitionExecutionStatus handles not-found and wrong-status cases.
@@ -271,30 +336,13 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF
if err := h.requireExecutionAccess(ctx, session, executionID); err != nil {
return nil, err
}
+ if err := h.authorizeExecutionManagement(ctx, session, executionID); err != nil {
+ return nil, err
+ }
- // Cancel the scheduled execution. The RETURNING clause gives us the
- // parent plan_id so we can disable the plan in the same handler call.
- //
- // Idempotency: if TransitionExecutionStatus returns
- // ErrExecutionNotInExpectedStatus the row is already in a terminal state
- // (most likely "cancelled" from a previous attempt). In that case we
- // fetch the execution to recover the PlanID and still attempt to disable
- // the plan, so a retry never leaves plan.enabled=true.
- cancelled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "cancelled")
+ cancelled, err := h.cancelOrRecoverExecution(ctx, executionID)
if err != nil {
- if !errors.Is(err, config.ErrExecutionNotInExpectedStatus) {
- return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", executionID, err))
- }
- // The cancel already landed (e.g. a prior request succeeded and was
- // retried). Recover the execution so we can still disable the plan.
- existing, getErr := h.config.GetExecutionByID(ctx, executionID)
- if getErr != nil {
- return nil, fmt.Errorf("disable plan: failed to get execution %s after conflict: %w", executionID, getErr)
- }
- if existing == nil {
- return nil, NewClientError(404, fmt.Sprintf("execution %s not found", executionID))
- }
- cancelled = existing
+ return nil, err
}
// Set the parent plan's enabled flag to false so the Plans page toggle
@@ -310,6 +358,34 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF
return &StatusResponse{Status: "cancelled"}, nil
}
+// cancelOrRecoverExecution transitions the execution to "cancelled" if it is
+// still in {pending, paused}. If a prior attempt already cancelled it
+// (ErrExecutionNotInExpectedStatus), it fetches the row instead so the caller
+// can still drive the plan-disable side-effect, keeping the operation
+// idempotent across retries.
+func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID string) (*config.PurchaseExecution, error) {
+ cancelled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "cancelled")
+ if err == nil {
+ return cancelled, nil
+ }
+ if !errors.Is(err, config.ErrExecutionNotInExpectedStatus) {
+ return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", executionID, err))
+ }
+ existing, getErr := h.config.GetExecutionByID(ctx, executionID)
+ if getErr != nil {
+ return nil, fmt.Errorf("disable plan: failed to get execution %s after conflict: %w", executionID, getErr)
+ }
+ if existing == nil {
+ return nil, NewClientError(404, fmt.Sprintf("execution %s not found", executionID))
+ }
+ if existing.Status != "cancelled" {
+ return nil, NewClientError(409, fmt.Sprintf(
+ "execution %s cannot be cancelled (status=%s)",
+ executionID, existing.Status))
+ }
+ return existing, nil
+}
+
// disablePlan fetches the plan identified by planID and sets Enabled=false if
// it is currently true. It is idempotent: calling it against an already-
// disabled plan is a no-op. Returns a 404 ClientError when the plan does not
diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go
index bebd33576..44f9ec8ae 100644
--- a/internal/api/handler_purchases_test.go
+++ b/internal/api/handler_purchases_test.go
@@ -1319,6 +1319,56 @@ func TestHandler_deletePlannedPurchase_ConflictRetryAlreadyDisabled(t *testing.T
assert.Equal(t, "cancelled", result.Status)
}
+// TestHandler_deletePlannedPurchase_ConflictRetryRunningReturns409 is a
+// regression test for CR #995 Finding 1: when TransitionExecutionStatus
+// returns ErrExecutionNotInExpectedStatus but the fetched row is NOT
+// "cancelled" (e.g. the execution raced to "running"), cancelOrRecoverExecution
+// must return a 409 and must NOT call disablePlan (no GetPurchasePlan call).
+func TestHandler_deletePlannedPurchase_ConflictRetryRunningReturns409(t *testing.T) {
+ ctx := context.Background()
+ mockStore := new(MockConfigStore)
+ mockAuth := new(MockAuthService)
+ t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+ adminSession := &Session{
+ UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
+ Email: "admin@example.com",
+ }
+
+ execID := "abababab-abab-abab-abab-abababababab"
+ planID := "cdcdcdcd-cdcd-cdcd-cdcd-cdcdcdcdcdcd"
+
+ conflictErr := fmt.Errorf("%w: execution %s cannot transition", config.ErrExecutionNotInExpectedStatus, execID)
+
+ // The execution raced to "running" — not "cancelled".
+ runningExec := &config.PurchaseExecution{
+ ExecutionID: execID,
+ PlanID: planID,
+ Status: "running",
+ }
+
+ mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
+ mockAuth.grantAdmin()
+ mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled").Return(nil, conflictErr)
+ mockStore.On("GetExecutionByID", ctx, execID).Return(runningExec, nil)
+ // GetPurchasePlan must NOT be called — AssertExpectations verifies this.
+
+ handler := &Handler{config: mockStore, auth: mockAuth}
+
+ req := &events.LambdaFunctionURLRequest{
+ Headers: map[string]string{"Authorization": "Bearer admin-token"},
+ }
+ result, err := handler.deletePlannedPurchase(ctx, req, execID)
+ require.Error(t, err, "racing-to-running execution must fail")
+ assert.Nil(t, result)
+
+ ce, ok := IsClientError(err)
+ require.True(t, ok, "expected ClientError, got %T: %v", err, err)
+ assert.Equal(t, 409, ce.code, "status mismatch must return 409")
+ assert.Contains(t, ce.message, "cannot be cancelled", "error must name the action")
+ assert.Contains(t, ce.message, "running", "error must include actual status")
+}
+
func TestHandler_pausePlannedPurchase_NilExecution(t *testing.T) {
ctx := context.Background()
mockStore := new(MockConfigStore)
@@ -3317,3 +3367,143 @@ func TestHandler_authorizeSessionExecuteDirect_NilAuth(t *testing.T) {
require.True(t, ok, "expected a clientError")
assert.Equal(t, 500, ce.code)
}
+
+// --- Issue #950: creator-scope ownership gate on scheduled-purchase mgmt ---
+//
+// These tests replicate the QA scenario in issue #950: a Standard user with
+// update:purchases (and account access) must NOT be able to pause/resume/
+// cancel a scheduled purchase created by ANOTHER user. They drive the real
+// handlers end-to-end (ValidateSession -> requirePermission -> account scope
+// -> authorizeExecutionManagement -> transition) and FAIL against the pre-fix
+// handler, which honoured the request because only update:purchases was
+// checked.
+
+const ownExecID = "12121212-1212-1212-1212-121212121212"
+const ownUserA = "aaaa1111-1111-1111-1111-111111111111" // creator of P1
+const ownUserB = "bbbb2222-2222-2222-2222-222222222222" // creator of P2
+
+// buildManageHandler wires a non-admin "user-token" session for userID with
+// account access (empty allowed_accounts -> all accessible) and the given
+// update-any grant. The stored execution is created by creatorID.
+func buildManageHandler(userID, creatorID string, hasUpdateAny bool) (*Handler, *MockConfigStore, *MockAuthService) {
+ mockAuth := new(MockAuthService)
+ mockAuth.On("ValidateSession", mock.Anything, "user-token").Return(&Session{UserID: userID}, nil)
+ mockAuth.On("HasPermissionAPI", mock.Anything, userID, "update", "purchases").Return(true, nil).Maybe()
+ mockAuth.On("HasPermissionAPI", mock.Anything, userID, "execute", "purchases").Return(true, nil).Maybe()
+ mockAuth.On("HasPermissionAPI", mock.Anything, userID, "delete", "purchases").Return(true, nil).Maybe()
+ mockAuth.On("HasPermissionAPI", mock.Anything, userID, "update-any", "purchases").Return(hasUpdateAny, nil).Maybe()
+ mockAuth.On("GetAllowedAccountsAPI", mock.Anything, userID).Return([]string{}, nil).Maybe()
+
+ creator := creatorID
+ exec := &config.PurchaseExecution{ExecutionID: ownExecID, Status: "pending", CreatedByUserID: &creator}
+ mockConfig := new(MockConfigStore)
+ mockConfig.On("GetExecutionByID", mock.Anything, ownExecID).Return(exec, nil)
+
+ return &Handler{config: mockConfig, auth: mockAuth}, mockConfig, mockAuth
+}
+
+func manageReq() *events.LambdaFunctionURLRequest {
+ return &events.LambdaFunctionURLRequest{
+ Headers: map[string]string{"authorization": "Bearer user-token"},
+ }
+}
+
+// TestHandler_pausePlannedPurchase_NonOwner_Rejected is the core #950
+// regression: user A pauses a scheduled purchase created by user B -> 403,
+// and the status transition never runs.
+func TestHandler_pausePlannedPurchase_NonOwner_Rejected(t *testing.T) {
+ handler, mockConfig, mockAuth := buildManageHandler(ownUserA, ownUserB, false)
+
+ _, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID)
+ require.Error(t, err)
+ ce, ok := IsClientError(err)
+ require.True(t, ok)
+ assert.Equal(t, 403, ce.code)
+ assert.Contains(t, ce.message, "another user's scheduled purchase")
+ mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+ mockAuth.AssertExpectations(t)
+}
+
+func TestHandler_resumePlannedPurchase_NonOwner_Rejected(t *testing.T) {
+ handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, false)
+
+ _, err := handler.resumePlannedPurchase(context.Background(), manageReq(), ownExecID)
+ require.Error(t, err)
+ ce, ok := IsClientError(err)
+ require.True(t, ok)
+ assert.Equal(t, 403, ce.code)
+ mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
+func TestHandler_deletePlannedPurchase_NonOwner_Rejected(t *testing.T) {
+ handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, false)
+
+ _, err := handler.deletePlannedPurchase(context.Background(), manageReq(), ownExecID)
+ require.Error(t, err)
+ ce, ok := IsClientError(err)
+ require.True(t, ok)
+ assert.Equal(t, 403, ce.code)
+ mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
+func TestHandler_runPlannedPurchase_NonOwner_Rejected(t *testing.T) {
+ handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, false)
+
+ _, err := handler.runPlannedPurchase(context.Background(), manageReq(), ownExecID)
+ require.Error(t, err)
+ ce, ok := IsClientError(err)
+ require.True(t, ok)
+ assert.Equal(t, 403, ce.code)
+ mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
+// TestHandler_pausePlannedPurchase_Owner_Allowed: user A manages their OWN P1.
+func TestHandler_pausePlannedPurchase_Owner_Allowed(t *testing.T) {
+ handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserA, false)
+ mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused").
+ Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "paused"}, nil)
+
+ res, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID)
+ require.NoError(t, err)
+ assert.Equal(t, "paused", res.Status)
+ mockConfig.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused")
+}
+
+// TestHandler_pausePlannedPurchase_UpdateAny_AllowsAny: a privileged user with
+// update-any:purchases manages P2 created by user B.
+func TestHandler_pausePlannedPurchase_UpdateAny_AllowsAny(t *testing.T) {
+ handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, true)
+ mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused").
+ Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "paused"}, nil)
+
+ res, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID)
+ require.NoError(t, err)
+ assert.Equal(t, "paused", res.Status)
+}
+
+// TestHandler_authorizeExecutionManagement_NilAuth: fail-closed 500.
+func TestHandler_authorizeExecutionManagement_NilAuth(t *testing.T) {
+ handler := &Handler{auth: nil}
+ err := handler.authorizeExecutionManagement(context.Background(), &Session{UserID: ownUserA}, ownExecID)
+ require.Error(t, err)
+ ce, ok := IsClientError(err)
+ require.True(t, ok)
+ assert.Equal(t, 500, ce.code)
+}
+
+// TestHandler_authorizeExecutionManagement_LegacyNullCreator: a legacy row
+// with a NULL creator is unreachable for a non-update-any user.
+func TestHandler_authorizeExecutionManagement_LegacyNullCreator(t *testing.T) {
+ mockAuth := new(MockAuthService)
+ mockAuth.On("HasPermissionAPI", mock.Anything, ownUserA, "update-any", "purchases").Return(false, nil)
+ mockConfig := new(MockConfigStore)
+ mockConfig.On("GetExecutionByID", mock.Anything, ownExecID).
+ Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "pending", CreatedByUserID: nil}, nil)
+ handler := &Handler{config: mockConfig, auth: mockAuth}
+
+ err := handler.authorizeExecutionManagement(context.Background(), &Session{UserID: ownUserA}, ownExecID)
+ require.Error(t, err)
+ ce, ok := IsClientError(err)
+ require.True(t, ok)
+ assert.Equal(t, 403, ce.code)
+}
diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go
index b435fd985..50f30e1b9 100644
--- a/internal/api/router_660_permission_flips_test.go
+++ b/internal/api/router_660_permission_flips_test.go
@@ -194,13 +194,18 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) {
const userID = "33333333-3333-3333-3333-333333333333"
const execID = "cccccccc-cccc-cccc-cccc-cccccccccccc"
- t.Run("user with update:purchases can pause a planned purchase", func(t *testing.T) {
+ t.Run("creator with update:purchases can pause their own planned purchase", func(t *testing.T) {
+ // Issue #950: a standard user manages only the scheduled purchases
+ // they created. update-any is false; the creator match authorises.
mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", true)
mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil)
+ mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil)
+ creator := userID
mockStore := new(MockConfigStore)
- // requireExecutionAccess calls GetExecutionByID; stub a minimal row.
+ // requireExecutionAccess + authorizeExecutionManagement both call
+ // GetExecutionByID; stub a row created by this user.
mockStore.On("GetExecutionByID", ctx, execID).
- Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil)
+ Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil)
// TransitionExecutionStatus is called next; stub it.
mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused").
Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil)
@@ -210,6 +215,44 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) {
assertNotForbidden(t, err)
})
+ t.Run("non-creator with update:purchases is rejected with 403 (issue #950)", func(t *testing.T) {
+ // The user holds update:purchases (and account access) but did NOT
+ // create the execution and lacks update-any -> 403. This is the
+ // regression guard for the pre-fix authz hole.
+ mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", true)
+ mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil)
+ mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil)
+ otherCreator := "99999999-9999-9999-9999-999999999999"
+ mockStore := new(MockConfigStore)
+ mockStore.On("GetExecutionByID", ctx, execID).
+ Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &otherCreator}, nil)
+
+ h := &Handler{auth: mockAuth, config: mockStore}
+ _, err := h.pausePlannedPurchase(ctx, reqWithBearer("user-token"), execID)
+ assert403(t, err)
+ // The status transition must never run for a non-owner.
+ mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+ })
+
+ t.Run("update-any holder can pause another user's planned purchase (issue #950)", func(t *testing.T) {
+ // An operator role with update-any:purchases bypasses the creator
+ // check, mirroring cancel-any/approve-any on History.
+ mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", true)
+ mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil)
+ mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(true, nil)
+ mockStore := new(MockConfigStore)
+ // update-any short-circuits the ownership fetch in
+ // authorizeExecutionManagement; only requireExecutionAccess fetches.
+ mockStore.On("GetExecutionByID", ctx, execID).
+ Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil)
+ mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused").
+ Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil)
+
+ h := &Handler{auth: mockAuth, config: mockStore}
+ _, err := h.pausePlannedPurchase(ctx, reqWithBearer("user-token"), execID)
+ assertNotForbidden(t, err)
+ })
+
t.Run("user without update:purchases is rejected with 403", func(t *testing.T) {
mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", false)
h := &Handler{auth: mockAuth, config: new(MockConfigStore)}
@@ -239,12 +282,16 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) {
const userID = "44444444-4444-4444-4444-444444444444"
const execID = "dddddddd-dddd-dddd-dddd-dddddddddddd"
- t.Run("user with delete:purchases can delete a planned purchase", func(t *testing.T) {
+ t.Run("creator with delete:purchases can delete their own planned purchase", func(t *testing.T) {
+ // Issue #950: ownership gate also applies to delete; a creator with
+ // delete:purchases (no update-any) is authorised by the creator match.
mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", true)
mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil)
+ mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil)
+ creator := userID
mockStore := new(MockConfigStore)
mockStore.On("GetExecutionByID", ctx, execID).
- Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil)
+ Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil)
mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled").
Return(&config.PurchaseExecution{ExecutionID: execID, Status: "cancelled"}, nil)
@@ -253,6 +300,21 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) {
assertNotForbidden(t, err)
})
+ t.Run("non-creator with delete:purchases is rejected with 403 (issue #950)", func(t *testing.T) {
+ mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", true)
+ mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil)
+ mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil)
+ otherCreator := "99999999-9999-9999-9999-999999999999"
+ mockStore := new(MockConfigStore)
+ mockStore.On("GetExecutionByID", ctx, execID).
+ Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &otherCreator}, nil)
+
+ h := &Handler{auth: mockAuth, config: mockStore}
+ _, err := h.deletePlannedPurchase(ctx, reqWithBearer("user-token"), execID)
+ assert403(t, err)
+ mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+ })
+
t.Run("user without delete:purchases is rejected with 403", func(t *testing.T) {
mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", false)
h := &Handler{auth: mockAuth, config: new(MockConfigStore)}
diff --git a/internal/api/types.go b/internal/api/types.go
index 40b979d7e..64aff5d71 100644
--- a/internal/api/types.go
+++ b/internal/api/types.go
@@ -624,6 +624,15 @@ type UpcomingPurchase struct {
StepNumber int `json:"step_number"`
TotalSteps int `json:"total_steps"`
EstimatedSavings float64 `json:"estimated_savings"`
+ // CreatedByUserID propagates the underlying execution's
+ // created_by_user_id so the dashboard widget can apply the same
+ // creator-scope ownership gate the Plans page uses (issue #950).
+ // Without it the widget renders a "Cancel" button on every row
+ // while the backend now 403s for non-owners -- a UX hole that
+ // surfaces as a confusing toast on click. Mirrors the field on
+ // PlannedPurchase / PurchaseHistoryEntry. omitempty so legacy
+ // NULL-creator rows keep the JSON shape they had pre-fix.
+ CreatedByUserID *string `json:"created_by_user_id,omitempty"`
}
// PlannedPurchasesResponse holds the list of planned purchases
@@ -649,6 +658,11 @@ type PlannedPurchase struct {
Status string `json:"status"`
StepNumber int `json:"step_number"`
TotalSteps int `json:"total_steps"`
+ // CreatedByUserID is the UUID of the user who created the scheduled
+ // purchase, mirroring PurchaseHistoryRecord.CreatedByUserID. The
+ // frontend gates the row action buttons on creator-scope ownership
+ // (issue #950); omitted for legacy rows with a NULL creator.
+ CreatedByUserID *string `json:"created_by_user_id,omitempty"`
}
// PlanRequest represents the API request format for creating/updating plans
diff --git a/internal/auth/types.go b/internal/auth/types.go
index 4958182d7..398ddc93c 100644
--- a/internal/auth/types.go
+++ b/internal/auth/types.go
@@ -426,6 +426,28 @@ const (
// check. No default non-admin grant; add to a custom operator group.
ActionExecuteOwn = "execute-own"
ActionExecuteAny = "execute-any"
+ // ActionUpdateAny is the privileged escape that lets a holder manage
+ // (pause / resume / run / delete) a SCHEDULED purchase execution
+ // regardless of who created it (issue #950). It complements the base
+ // update:purchases verb every authenticated user already holds: that
+ // base verb authorises managing only your OWN scheduled purchases
+ // (created_by_user_id == session.UserID), while update-any drops the
+ // per-record ownership check.
+ //
+ // * RoleAdmin — implicit via {ActionAdmin, ResourceAll}; update-any is
+ // NOT in adminCarvedOuts, so admins manage every scheduled purchase.
+ // * RoleUser — NO default grant. A standard user manages only the
+ // scheduled purchases they created (base update:purchases + creator
+ // match). Legacy rows with NULL created_by_user_id are out of reach
+ // for non-admins (they hold neither update-any nor a creator match).
+ // * Custom operator groups — add update-any:purchases to let a role
+ // manage everyone's scheduled purchases without escalating to admin.
+ //
+ // There is no separate update-own verb: the existing update:purchases
+ // grant already plays that role, mirroring how cancel-own/approve-own
+ // gate History rows. The creator match is enforced in the handler
+ // (authorizeExecutionManagement), not in HasPermission.
+ ActionUpdateAny = "update-any"
)
// Predefined resources