sec(api): bound update-any by its Constraints on planned-purchase run

internal/api/claimed_row_check.go

@@ -21,10 +21,14 @@ import (
 // without touching the row. A post-claim refusal ends the row "failed" with
 // no_submission set (see purchase.ClaimedRowCheck).
 
-// runNowClaimCheck is runPlannedPurchase's execute:purchases Constraints
-// check, against the claimed row.
+// runNowClaimCheck is runPlannedPurchase's update-any / creator check
+// (issue #1129) and execute:purchases Constraints check, against the claimed
+// row.
 func (h *Handler) runNowClaimCheck(session *Session) purchase.ClaimedRowCheck {
 	return func(ctx context.Context, claimed *config.PurchaseExecution) error {
+		if err := h.authorizeClaimedRowManagement(ctx, session, claimed); err != nil {
+			return err
+		}
 		return h.enforcePurchaseConstraints(ctx, session, "execute", claimed.Recommendations)
 	}
 }

internal/api/handler_purchases.go

@@ -152,7 +152,9 @@ func buildPlannedPurchase(plan *config.PurchasePlan, exec *config.PurchaseExecut
 // 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.
+// check (requireExecutionAccess); those still apply unchanged. Pause, resume
+// and run reach it through authorizeBoundedExecutionManagement, which bounds
+// update-any by its Constraints first (issues #1098, #1129).
 //
 // Gate logic (mirrors authorizeSessionCancel / authorizeSessionApprove):
 //   - stateless admin API key: always permitted (apiKeyAdminUserID sentinel).
@@ -205,17 +207,12 @@ func (h *Handler) authorizeExecutionManagement(ctx context.Context, session *Ses
 	return nil
 }
 
-// authorizeExecutionPauseResume is authorizeExecutionManagement for pause and
-// resume, with update-any bounded by its Constraints (issue #1098). A session
-// holding update-any is checked against the strict per-recommendation scope of
-// the STORED row (revokeExecutionConstraintSets), mirroring the planned-purchase
-// cancel gates. An in-scope grant is permitted without an owner check. A
-// Constraints 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.
-// Sessions without update-any take the unchanged authorizeExecutionManagement
-// path.
-func (h *Handler) authorizeExecutionPauseResume(ctx context.Context, session *Session, executionID string) error {
+// authorizeBoundedExecutionManagement is authorizeExecutionManagement with
+// update-any bounded by its Constraints, used by pause, resume (issue #1098)
+// and run (issue #1129). Sessions without update-any take the unchanged
+// authorizeExecutionManagement path; a session holding update-any is checked
+// against the stored row by authorizeUpdateAnyOnRow.
+func (h *Handler) authorizeBoundedExecutionManagement(ctx context.Context, session *Session, executionID string) error {
 	if session.UserID == apiKeyAdminUserID {
 		return nil
 	}
@@ -233,6 +230,17 @@ func (h *Handler) authorizeExecutionPauseResume(ctx context.Context, session *Se
 	if err != nil {
 		return err
 	}
+	return h.authorizeUpdateAnyOnRow(ctx, session, execution)
+}
+
+// authorizeUpdateAnyOnRow checks a session holding update-any against the
+// strict per-recommendation scope of the STORED row
+// (revokeExecutionConstraintSets), mirroring the planned-purchase cancel
+// gates. An in-scope grant is permitted without an owner check. A Constraints
+// 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 {
 	anyErr := h.requirePermissionConstraints(ctx, session, auth.ActionUpdateAny, auth.ResourcePurchases, revokeExecutionConstraintSets(execution))
 	if anyErr == nil || !isForbidden(anyErr) {
 		return anyErr
@@ -243,6 +251,27 @@ func (h *Handler) authorizeExecutionPauseResume(ctx context.Context, session *Se
 	return anyErr
 }
 
+// authorizeClaimedRowManagement re-runs authorizeBoundedExecutionManagement's
+// decision on a row already in hand: the row run's claim CAS returned
+// (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 {
+	if session.UserID == apiKeyAdminUserID {
+		return nil
+	}
+	hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionUpdateAny)
+	if err != nil {
+		return err
+	}
+	if hasAny {
+		return h.authorizeUpdateAnyOnRow(ctx, session, execution)
+	}
+	if !isExecutionCreator(session, execution) {
+		return NewClientError(403, "permission denied: cannot manage another user's scheduled purchase")
+	}
+	return nil
+}
+
 // isExecutionCreator requires both IDs to be non-empty and equal, so an
 // empty-string collision (legacy NULL creator, missing session UserID) never
 // matches.
@@ -262,7 +291,7 @@ 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.authorizeExecutionPauseResume(ctx, session, executionID); err != nil {
+	if err := h.authorizeBoundedExecutionManagement(ctx, session, executionID); err != nil {
 		return nil, err
 	}
 
@@ -286,7 +315,7 @@ 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.authorizeExecutionPauseResume(ctx, session, executionID); err != nil {
+	if err := h.authorizeBoundedExecutionManagement(ctx, session, executionID); err != nil {
 		return nil, err
 	}
 
@@ -383,16 +412,18 @@ 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 {
+	// update-any lets a non-creator run the row only within its Constraints
+	// (issue #1129).
+	if err := h.authorizeBoundedExecutionManagement(ctx, session, executionID); err != nil {
 		return nil, err
 	}
 
 	// Re-hydrate the persisted recommendations and enforce the session's
 	// execute:purchases Constraints against them before RunPlannedPurchaseNow
 	// fires the purchase (issue #60). requirePermission above only checked
 	// the bare execute:purchases verb: a session running a plan via
-	// update-any:purchases (which authorizeExecutionManagement lets bypass
-	// the creator check) would otherwise bypass any MaxPurchaseAmount/
+	// update-any:purchases (which lets a non-creator past the creator check
+	// within its own Constraints) would otherwise bypass any MaxPurchaseAmount/
 	// Providers/Services/Regions/AccountIDs cap configured on their own
 	// execute:purchases permission, exactly the fail-open class the web
 	// execute path (validateExecutePurchaseRequest) already closed for

internal/api/handler_purchases_run_update_any_integration_test.go

@@ -0,0 +1,115 @@
+//go:build integration
+
+package api
+
+import (
+	"encoding/json"
+	"sync"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// Issue #1129 against real PostgreSQL: a non-creator runs the row through an
+// update-any bounded to provider aws. A second writer moves the row to gcp
+// after the handler's last pre-claim check and before the claim CAS; the
+// funnel's post-claim re-check must refuse the row the CAS returned. Real
+// store, handler and purchase manager, counting fake provider (runNowService);
+// the Constraints are decided by the real auth matcher. Nothing here reaches a
+// cloud API.
+
+// insertNonCreator adds a real users row for the operator: the claim stamps
+// the session user onto transitioned_by, a foreign key.
+func insertNonCreator(t *testing.T, f *pauseClaimFixture) string {
+	t.Helper()
+	id := uuid.NewString()
+	_, err := f.pool.Exec(f.ctx, `INSERT INTO users (id,email,password_hash,salt,active,group_ids,created_at,updated_at) SELECT $1,'operator@example.invalid','x','y',true,ARRAY[g.id],now(),now() FROM groups g ORDER BY g.name LIMIT 1`, id)
+	require.NoError(t, err)
+	require.NotEqual(t, id, *f.execution.CreatedByUserID)
+	return id
+}
+
+// moveProviderDuringExecuteCheck answers permission checks for a non-creator
+// session with the real matcher and, on the first execute Constraints check
+// (the handler's last pre-claim check), rewrites the row's provider with a
+// separate UPDATE. move=false leaves the row alone (the unmodified control).
+func moveProviderDuringExecuteCheck(t *testing.T, f *pauseClaimFixture, perms []auth.Permission, move bool) *MockAuthService {
+	t.Helper()
+	operator := insertNonCreator(t, f)
+	var once sync.Once
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{UserID: operator, Email: "operator@example.invalid"}, nil).Maybe()
+	mockAuth.On("ValidateCSRFToken", mock.Anything, "admin-token", "").Return(nil).Maybe()
+	authCtx := &auth.AuthContext{Permissions: perms}
+	mockAuth.On("HasPermissionAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+		Return(func(action, resource string) bool { return authCtx.HasPermission(action, resource) }, nil).Maybe()
+	mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe()
+	mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+		Return(constraintDecisionFunc(func(action, resource string, sets []auth.PermissionConstraints) (bool, error) {
+			allowed, err := auth.PermissionsAllowForConstraintSets(perms, action, resource, sets)
+			if move && action == auth.ActionExecute {
+				once.Do(func() { moveRowProvider(t, f, "gcp") })
+			}
+			return allowed, err
+		}), nil).Maybe()
+	return mockAuth
+}
+
+func moveRowProvider(t *testing.T, f *pauseClaimFixture, provider string) {
+	t.Helper()
+	recs := fanoutRecs()
+	for i := range recs {
+		recs[i].CloudAccountID = f.execution.CloudAccountID
+		recs[i].Provider = provider
+	}
+	payload, err := json.Marshal(recs)
+	require.NoError(t, err)
+	tag, err := f.pool.Exec(f.ctx, `UPDATE purchase_executions SET recommendations = $2 WHERE execution_id = $1`, f.execution.ExecutionID, payload)
+	require.NoError(t, err)
+	require.EqualValues(t, 1, tag.RowsAffected())
+}
+
+func runBoundPerms() []auth.Permission {
+	return []auth.Permission{
+		{Action: auth.ActionExecute, Resource: auth.ResourcePurchases},
+		{Action: auth.ActionUpdateAny, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{Providers: []string{"aws"}}},
+	}
+}
+
+func TestRunNow_Postgres_RowMovedOutOfUpdateAnyScopeBeforeClaimIsRefused(t *testing.T) {
+	f, service := newRunNowFixture(t, 0, fanoutRecs())
+	f.handler.auth = moveProviderDuringExecuteCheck(t, f, runBoundPerms(), true)
+
+	_, err := f.runNow()
+
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a client-facing error, got %v", err)
+	assert.Equal(t, 403, ce.code)
+	assert.Contains(t, ce.message, "nothing was bought")
+	assert.Zero(t, service.calls.Load(), "the moved row must never reach the provider")
+	row, getErr := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID)
+	require.NoError(t, getErr)
+	assert.Equal(t, "gcp", row.Recommendations[0].Provider, "the race must have landed")
+	assert.Equal(t, "failed", row.Status)
+	assert.True(t, row.NoSubmission, "refused before any provider call")
+	var histories int
+	require.NoError(t, f.pool.QueryRow(f.ctx, "SELECT count(*) FROM purchase_history WHERE plan_id=$1", f.planID).Scan(&histories))
+	assert.Zero(t, histories)
+}
+
+func TestRunNow_Postgres_InScopeUpdateAnyNonCreatorPurchases(t *testing.T) {
+	f, service := newRunNowFixture(t, 0, fanoutRecs())
+	f.handler.auth = moveProviderDuringExecuteCheck(t, f, runBoundPerms(), false)
+
+	_, err := f.runNow()
+	require.NoError(t, err)
+
+	assert.EqualValues(t, 1, service.calls.Load())
+	row, getErr := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID)
+	require.NoError(t, getErr)
+	assert.Equal(t, "completed", row.Status)
+}