fix(api): bound update-any by its Constraints on planned-purchase DELETE

internal/api/handler_purchases.go

@@ -436,51 +436,67 @@ func (h *Handler) requireDeleteOrCancelPurchasePermission(ctx context.Context, r
 //
 // Gate logic:
 //   - stateless admin API key: always permitted.
-//   - update-any:purchases: permitted without an owner check or Constraints.
-//   - cancel-any:purchases: permitted without an owner check when its
-//     Constraints cover every recommendation (issue #678); a Constraints
-//     denial falls back to the creator branch.
-//   - otherwise: fetch the execution and require creator match, then apply
-//     cancel-own Constraints if the creator also holds cancel-own.
-//     Legacy rows with a NULL creator are out of reach for non-privileged
-//     users (matching cancel-own / approve-own / retry-own behavior).
+//   - update-any:purchases, then cancel-any:purchases: each is permitted
+//     without an owner check when its Constraints cover every recommendation
+//     (issues #961 and #678). A Constraints denial falls through to the next
+//     full-scope verb and then to the creator branch.
+//   - otherwise: require creator match, then apply cancel-own Constraints if
+//     the creator also holds cancel-own. Legacy rows with a NULL creator are
+//     out of reach for non-privileged users (matching cancel-own /
+//     approve-own / retry-own behavior).
 func (h *Handler) authorizePlannedPurchaseCancel(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")
 	}
-	// update-any grants full-scope access without an owner check or
-	// Constraints (tracked by #961). Both full-scope verbs are evaluated
-	// against the session's effective permissions: the key's for a user API
-	// key, never the owner's alone (issue #979).
-	hasUpdate, err := h.sessionHoldsVerb(ctx, session, auth.ActionUpdateAny)
-	if err != nil {
-		return err
-	}
-	if hasUpdate {
-		return nil
-	}
-	hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionCancelAny)
+	execution, err := h.loadPlannedExecution(ctx, executionID)
 	if err != nil {
 		return err
 	}
-	execution, err := h.loadPlannedExecution(ctx, executionID)
-	if err != nil {
+	granted, denied, err := h.authorizePlannedFullScope(ctx, session, execution)
+	if err != nil || granted {
 		return err
 	}
-	// cancel-any is bounded by its Constraints (issue #678); a 403 falls back
-	// to the creator branch exactly as authorizeSessionCancel does.
-	var anyErr error
-	if hasAny {
+	return h.authorizePlannedCreatorCancel(ctx, session, execution, denied)
+}
+
+// plannedFullScopeVerbs are the verbs that let DELETE planned act on another
+// user's row, in the order they are tried.
+var plannedFullScopeVerbs = []string{auth.ActionUpdateAny, auth.ActionCancelAny}
+
+// authorizePlannedFullScope tries each full-scope verb the session holds
+// against the strict per-recommendation Constraints of the STORED row
+// (revokeExecutionConstraintSets), never the request. granted reports that one
+// of them covers the row. denied is the last Constraints 403, returned to a
+// non-creator in place of the ownership 403 so the refusal stays terminal; it
+// is nil when the session holds neither verb. Any other error (a probe or
+// constraint-store failure) is returned as err and never read as a denial, so
+// it cannot fall through to a weaker branch. Both verbs are evaluated against
+// the session's effective permissions: the key's for a user API key, never the
+// owner's alone (issue #979).
+func (h *Handler) authorizePlannedFullScope(ctx context.Context, session *Session, execution *config.PurchaseExecution) (granted bool, denied, err error) {
+	sets := revokeExecutionConstraintSets(execution)
+	for _, verb := range plannedFullScopeVerbs {
+		held, probeErr := h.sessionHoldsVerb(ctx, session, verb)
+		if probeErr != nil {
+			return false, nil, probeErr
+		}
+		if !held {
+			continue
+		}
 		// Wrapped for consistency; DELETE planned has no token fall-through.
-		anyErr = wrapConstraintDenied(h.requirePermissionConstraints(ctx, session, auth.ActionCancelAny, auth.ResourcePurchases, revokeExecutionConstraintSets(execution)))
-		if anyErr == nil || !isForbidden(anyErr) {
-			return anyErr
+		verbErr := wrapConstraintDenied(h.requirePermissionConstraints(ctx, session, verb, auth.ResourcePurchases, sets))
+		if verbErr == nil {
+			return true, nil, nil
+		}
+		if !isForbidden(verbErr) {
+			return false, nil, verbErr
 		}
+		denied = verbErr
 	}
-	return h.authorizePlannedCreatorCancel(ctx, session, execution, anyErr)
+	return false, denied, nil
 }
 
 // loadPlannedExecution fetches the row, mapping a missing one to errNotFound.
@@ -496,9 +512,9 @@ func (h *Handler) loadPlannedExecution(ctx context.Context, executionID string)
 }
 
 // authorizePlannedCreatorCancel is the creator branch of
-// authorizePlannedPurchaseCancel. anyErr is the cancel-any Constraints denial
-// (nil when the session has no cancel-any); it is returned instead of the
-// ownership 403 so a constraint refusal stays terminal.
+// authorizePlannedPurchaseCancel. anyErr is the update-any or cancel-any
+// Constraints denial (nil when the session holds neither); it is returned
+// instead of the ownership 403 so a constraint refusal stays terminal.
 func (h *Handler) authorizePlannedCreatorCancel(ctx context.Context, session *Session, execution *config.PurchaseExecution, anyErr error) error {
 	// requireDeleteOrCancelPurchasePermission already validated the session;
 	// UserID is either the API-key sentinel (handled by the caller) or a real

internal/api/handler_purchases_cancel_constraints_test.go

@@ -345,11 +345,11 @@ func TestDeletePlannedPurchase_CancelConstraints(t *testing.T) {
 		requireRevokeForbidden(t, err, "another user's scheduled purchase")
 		notTransitioned(t, store)
 	})
-	t.Run("update-any is unchanged and unconstrained", func(t *testing.T) {
+	t.Run("update-any out of scope is denied (#961)", func(t *testing.T) {
 		t.Parallel()
-		resp, _, err := run(nil, revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{Providers: []string{"aws"}}))
-		require.NoError(t, err)
-		assert.Equal(t, "canceled", resp.Status)
+		_, store, err := run(nil, awsOnly(auth.ActionUpdateAny))
+		requireRevokeForbidden(t, err, constraintDenied)
+		notTransitioned(t, store)
 	})
 }
 

internal/api/handler_purchases_planned_update_any_test.go

@@ -0,0 +1,221 @@
+package api
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+)
+
+// Issue #961: on DELETE /api/purchases/planned/{id}, update-any:purchases
+// granted full scope on the verb alone and never evaluated the granting
+// permission's Constraints, so a session whose update-any is bounded to one
+// provider could cancel any provider's scheduled purchase. Answers come from
+// the real matcher via grantPermissionsScoped (bearer sessions) or a real
+// auth.Service (API keys); the cancelRouteExec row is azure/compute/
+// westeurope on account "acct-1".
+
+const plannedOtherUser = "someone-else"
+
+// runUpdateAnyDelete drives deletePlannedPurchase for a row created by creator
+// (nil = legacy NULL creator), optionally replacing its recommendations.
+func runUpdateAnyDelete(creator *string, recs []config.RecommendationRecord, perms ...auth.Permission) (*DeletePlannedPurchaseResponse, *MockConfigStore, error) {
+	exec := cancelRouteExec("pending")
+	exec.CreatedByUserID = creator
+	if recs != nil {
+		exec.Recommendations = recs
+	}
+	h, store := cancelRouteHandler(exec, perms...)
+	resp, err := h.deletePlannedPurchase(context.Background(), deletePlannedReq(), cancelExecID)
+	return resp, store, err
+}
+
+func requireNotCanceled(t *testing.T, store *MockConfigStore) {
+	t.Helper()
+	store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
+func requireCanceled(t *testing.T, resp *DeletePlannedPurchaseResponse, err error) {
+	t.Helper()
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	assert.Equal(t, "canceled", resp.Status)
+}
+
+func TestDeletePlannedPurchase_UpdateAnyConstraintDimensions(t *testing.T) {
+	t.Parallel()
+	other := plannedOtherUser
+	denying := map[string]*auth.PermissionConstraints{
+		"providers":   {Providers: []string{"aws"}},
+		"services":    {Services: []string{"rds"}},
+		"regions":     {Regions: []string{"eastus"}},
+		"account ids": {AccountIDs: []string{"other-acct"}},
+	}
+	allowing := map[string]*auth.PermissionConstraints{
+		"providers":   {Providers: []string{"azure"}},
+		"services":    {Services: []string{"compute"}},
+		"regions":     {Regions: []string{"westeurope"}},
+		"account ids": {AccountIDs: []string{"acct-1"}},
+		"none":        nil,
+	}
+	for dim, c := range denying {
+		t.Run("deny/"+dim, func(t *testing.T) {
+			t.Parallel()
+			_, store, err := runUpdateAnyDelete(&other, nil, revokePerm(auth.ActionUpdateAny, c))
+			requireRevokeForbidden(t, err, constraintDenied)
+			requireNotCanceled(t, store)
+		})
+	}
+	for dim, c := range allowing {
+		t.Run("allow/"+dim, func(t *testing.T) {
+			t.Parallel()
+			resp, _, err := runUpdateAnyDelete(&other, nil, revokePerm(auth.ActionUpdateAny, c))
+			requireCanceled(t, resp, err)
+		})
+	}
+}
+
+func TestDeletePlannedPurchase_UpdateAnyStrictScopeAndRecommendations(t *testing.T) {
+	t.Parallel()
+	other := plannedOtherUser
+	azureOnly := revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{Providers: []string{"azure"}})
+	t.Run("unknown region is refused by a region-bounded grant", func(t *testing.T) {
+		t.Parallel()
+		recs := []config.RecommendationRecord{recFor("acct-1", "azure", "compute", "")}
+		_, store, err := runUpdateAnyDelete(&other, recs, revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{Regions: []string{"westeurope"}}))
+		requireRevokeForbidden(t, err, constraintDenied)
+		requireNotCanceled(t, store)
+	})
+	t.Run("every recommendation must be in scope", func(t *testing.T) {
+		t.Parallel()
+		recs := []config.RecommendationRecord{azureRec(), recFor("acct-uuid", "aws", "ec2", "us-east-1")}
+		_, store, err := runUpdateAnyDelete(&other, recs, azureOnly)
+		requireRevokeForbidden(t, err, constraintDenied)
+		requireNotCanceled(t, store)
+	})
+	t.Run("a row with no recommendations is refused by a bounded grant", func(t *testing.T) {
+		t.Parallel()
+		_, store, err := runUpdateAnyDelete(&other, []config.RecommendationRecord{}, azureOnly)
+		requireRevokeForbidden(t, err, constraintDenied)
+		requireNotCanceled(t, store)
+	})
+	t.Run("a row with no recommendations is allowed by an unbounded grant", func(t *testing.T) {
+		t.Parallel()
+		resp, _, err := runUpdateAnyDelete(&other, []config.RecommendationRecord{}, revokePerm(auth.ActionUpdateAny, nil))
+		requireCanceled(t, resp, err)
+	})
+}
+
+// A bounded update-any denial falls back to cancel-any and then to the
+// creator branch, as #678 does for cancel-any; a non-creator keeps the
+// constraint message.
+func TestDeletePlannedPurchase_UpdateAnyFallback(t *testing.T) {
+	t.Parallel()
+	creator, other := revokeConstraintsUser, plannedOtherUser
+	del := auth.Permission{Action: auth.ActionDelete, Resource: auth.ResourcePurchases}
+	updateOut := awsOnly(auth.ActionUpdateAny)
+	allowed := map[string]struct {
+		creator *string
+		perms   []auth.Permission
+	}{
+		"to unbounded cancel-any":          {&other, []auth.Permission{updateOut, revokePerm(auth.ActionCancelAny, nil)}},
+		"to in-scope cancel-any":           {&other, []auth.Permission{updateOut, revokePerm(auth.ActionCancelAny, &auth.PermissionConstraints{Providers: []string{"azure"}})}},
+		"to the creator with delete":       {&creator, []auth.Permission{updateOut, del}},
+		"to the creator with in-scope own": {&creator, []auth.Permission{updateOut, revokePerm(auth.ActionCancelOwn, nil)}},
+	}
+	for name, tc := range allowed {
+		t.Run("falls back "+name, func(t *testing.T) {
+			t.Parallel()
+			resp, _, err := runUpdateAnyDelete(tc.creator, nil, tc.perms...)
+			requireCanceled(t, resp, err)
+		})
+	}
+	denied := map[string]struct {
+		creator *string
+		perms   []auth.Permission
+	}{
+		"non-creator with delete":              {&other, []auth.Permission{updateOut, del}},
+		"non-creator with bounded cancel-any":  {&other, []auth.Permission{updateOut, awsOnly(auth.ActionCancelAny), del}},
+		"legacy row with no creator":           {nil, []auth.Permission{updateOut, del}},
+		"creator with out-of-scope cancel-own": {&creator, []auth.Permission{updateOut, awsOnly(auth.ActionCancelOwn)}},
+	}
+	for name, tc := range denied {
+		t.Run("denies "+name, func(t *testing.T) {
+			t.Parallel()
+			_, store, err := runUpdateAnyDelete(tc.creator, nil, tc.perms...)
+			requireRevokeForbidden(t, err, constraintDenied)
+			requireNotCanceled(t, store)
+		})
+	}
+}
+
+func TestDeletePlannedPurchase_UpdateAnyAdministratorsGroup(t *testing.T) {
+	t.Parallel()
+	other := plannedOtherUser
+	t.Run("unbounded admin still cancels", func(t *testing.T) {
+		t.Parallel()
+		resp, _, err := runUpdateAnyDelete(&other, nil, auth.Permission{Action: auth.ActionAdmin, Resource: auth.ResourceAll})
+		requireCanceled(t, resp, err)
+	})
+	t.Run("admin bounded to another provider is refused", func(t *testing.T) {
+		t.Parallel()
+		admin := auth.Permission{Action: auth.ActionAdmin, Resource: auth.ResourceAll, Constraints: &auth.PermissionConstraints{Providers: []string{"aws"}}}
+		_, store, err := runUpdateAnyDelete(&other, nil, admin)
+		requireRevokeForbidden(t, err, constraintDenied)
+		requireNotCanceled(t, store)
+	})
+}
+
+// An error evaluating update-any's Constraints is not a denial: it must not
+// fall back to cancel-any or to the creator branch, both of which would allow.
+func TestDeletePlannedPurchase_UpdateAnyConstraintErrorDoesNotFallBack(t *testing.T) {
+	t.Parallel()
+	boom := errors.New("constraint store down")
+	owner := revokeConstraintsUser
+	exec := cancelRouteExec("pending")
+	exec.CreatedByUserID = &owner
+	store := new(MockConfigStore)
+	store.On("GetExecutionByID", mock.Anything, cancelExecID).Return(exec, nil)
+	// Reached only if the error were swallowed; requireNotCanceled reports it.
+	store.On("TransitionExecutionStatus", mock.Anything, cancelExecID, mock.Anything, mock.Anything, mock.Anything).
+		Return(nil, errKeyScopeReached).Maybe()
+	m := new(MockAuthService)
+	m.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{UserID: owner}, nil)
+	m.On("GetAllowedAccountsAPI", mock.Anything, owner).Return([]string{}, nil)
+	m.On("HasPermissionAPI", mock.Anything, owner, mock.Anything, auth.ResourcePurchases).Return(true, nil)
+	m.On("HasPermissionForConstraintsAPI", mock.Anything, owner, auth.ActionUpdateAny, auth.ResourcePurchases, mock.Anything).Return(false, boom)
+	m.On("HasPermissionForConstraintsAPI", mock.Anything, owner, mock.Anything, auth.ResourcePurchases, mock.Anything).Return(true, nil).Maybe()
+
+	h := &Handler{config: store, auth: m}
+	_, err := h.deletePlannedPurchase(context.Background(), deletePlannedReq(), cancelExecID)
+	require.ErrorIs(t, err, boom)
+	requireNotCanceled(t, store)
+}
+
+// A user API key is bounded by the KEY's update-any Constraints (key
+// intersected with its owner), evaluated by the real auth.Service. The
+// keyScopeExec row is aws/ec2/us-east-1, created by another user.
+func TestDeletePlannedPurchase_APIKeyUpdateAnyFollowsKeyConstraints(t *testing.T) {
+	rt := keyScopeRoutes[0]
+	keyPerms := func(provider string) []auth.Permission {
+		return []auth.Permission{
+			{Action: auth.ActionDelete, Resource: auth.ResourcePurchases},
+			revokePerm(auth.ActionUpdateAny, &auth.PermissionConstraints{Providers: []string{provider}}),
+		}
+	}
+	t.Run("key bounded to another provider is refused", func(t *testing.T) {
+		code, reached := serveKeyScope(t, rt, newKeyScopeAuth(keyPerms("azure")), keyScopeVictim, keyHeaders())
+		assert.Equal(t, 403, code)
+		assert.False(t, reached, "a refused request must not reach the mutation")
+	})
+	t.Run("key bounded to the row's provider is allowed", func(t *testing.T) {
+		_, reached := serveKeyScope(t, rt, newKeyScopeAuth(keyPerms("aws")), keyScopeVictim, keyHeaders())
+		assert.True(t, reached)
+	})
+}

internal/api/handler_purchases_test.go

@@ -1746,6 +1746,8 @@ func TestHandler_deletePlannedPurchase(t *testing.T) {
 	canceled := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "canceled"}
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
+	// authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961).
+	mockStore.On("GetExecutionByID", ctx, "11111111-1111-1111-1111-111111111111").Return(&config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "pending"}, nil).Once()
 	mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil)
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
@@ -1791,6 +1793,8 @@ func TestHandler_deletePlannedPurchase_DisablesPlan(t *testing.T) {
 
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
+	// authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961).
+	mockStore.On("GetExecutionByID", ctx, execID).Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil).Once()
 	mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil)
 	mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil)
 	// Assert that UpdatePurchasePlan is called with enabled=false.
@@ -1843,6 +1847,8 @@ func TestHandler_deletePlannedPurchase_AlreadyDisabledPlan(t *testing.T) {
 
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
+	// authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961).
+	mockStore.On("GetExecutionByID", ctx, execID).Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil).Once()
 	mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil)
 	mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil)
 
@@ -2098,9 +2104,12 @@ func TestHandler_deletePlannedPurchase_BackendErrorReturns5xx(t *testing.T) {
 
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
+	// authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961).
+	mockStore.On("GetExecutionByID", ctx, execID).Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil).Once()
 	mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, dbErr)
 	// On a real backend error we must NOT fall into the recovery path:
-	// GetExecutionByID must not be called. AssertExpectations verifies this.
+	// GetExecutionByID must not be called a second time. The .Once() above
+	// makes a recovery-path read fail the test.
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
 
@@ -2289,6 +2298,8 @@ func TestHandler_deletePlannedPurchase_NilExecution(t *testing.T) {
 
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
+	// authorizePlannedPurchaseCancel reads the row to bound update-any by its Constraints (#961).
+	mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999").Return(&config.PurchaseExecution{ExecutionID: "99999999-9999-9999-9999-999999999999", Status: "pending"}, nil).Once()
 	mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999"))
 
 	handler := &Handler{config: mockStore, auth: mockAuth}