fix(api): enforce approve-own constraints and the plan step cap on token approval of plan-step rows

internal/api/handler_purchases.go

@@ -958,35 +958,69 @@ func (h *Handler) authorizeSessionApprove(ctx context.Context, session *Session,
 
 // approveConstraintsForSession is approveViaToken's independent Constraints
 // check (issue #60 review follow-up): it mirrors authorizeSessionApprove's
-// approve-any/approve-own grant-then-constrain matrix, but returns nil
-// (rather than a 403) when the session holds neither verb, or holds only
-// approve-own for an execution it didn't create. Those are not denials here
-// -- a session with no applicable approve-* permission simply has no
-// Constraints to be bound by, and is being authorized entirely through the
-// token flow's separate contact_email mechanism. This function only ever
-// narrows what that mechanism would otherwise allow; it never widens it.
-//
-// session may be nil (a pure email-client click with no browser session at
-// all); that also returns nil, since there is no session-held permission to
-// evaluate. session.UserID == apiKeyAdminUserID or a nil h.auth are treated
-// the same way authorizeSessionApprove treats them.
+// approve-any/approve-own grant-then-constrain matrix. A session with no
+// applicable approve-* verb has no Constraints to be bound by on a
+// user-created row and returns nil (it is authorized through the token
+// flow's contact_email mechanism); this function only narrows, never widens.
+// Plan-step rows (PlanID set, no creator) are tighter (#631): approve-own
+// Constraints apply regardless of creator, and an approver with no verb or no
+// session is held to the plan's step cap (requirePlanStepCap). The admin API
+// key stays exempt.
 func (h *Handler) approveConstraintsForSession(ctx context.Context, session *Session, execution *config.PurchaseExecution) error {
-	if session == nil || session.UserID == apiKeyAdminUserID || h.auth == nil {
+	if session != nil && session.UserID == apiKeyAdminUserID {
 		return nil
 	}
+	if session != nil && h.auth != nil {
+		bound, err := h.enforceHeldApproveConstraints(ctx, session, execution)
+		if bound || err != nil {
+			return err
+		}
+	}
+	return h.requirePlanStepCap(ctx, execution)
+}
+
+// enforceHeldApproveConstraints applies the Constraints of the approve verb the
+// session holds (approve-any first, then approve-own). bound is false when no
+// verb applies to this row, so the caller falls back to the plan cap.
+func (h *Handler) enforceHeldApproveConstraints(ctx context.Context, session *Session, execution *config.PurchaseExecution) (bound bool, err error) {
 	hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveAny, auth.ResourcePurchases)
 	if err != nil {
-		return fmt.Errorf("permission check failed: %w", err)
+		return false, fmt.Errorf("permission check failed: %w", err)
 	}
 	if hasAny {
-		return h.enforcePurchaseConstraints(ctx, session, auth.ActionApproveAny, execution.Recommendations)
+		return true, h.enforcePurchaseConstraints(ctx, session, auth.ActionApproveAny, execution.Recommendations)
 	}
 	hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveOwn, auth.ResourcePurchases)
 	if err != nil {
-		return fmt.Errorf("permission check failed: %w", err)
+		return false, fmt.Errorf("permission check failed: %w", err)
+	}
+	// A plan-step row has no creator (plan_step_attach.go), so its approve-own
+	// holder is bound by their Constraints regardless of it.
+	if hasOwn && (execution.PlanID != "" || (execution.CreatedByUserID != nil && *execution.CreatedByUserID == session.UserID)) {
+		return true, h.enforcePurchaseConstraints(ctx, session, auth.ActionApproveOwn, execution.Recommendations)
+	}
+	return false, nil
+}
+
+// requirePlanStepCap applies the plan's max_step_commitment to a plan-step row
+// approved through the email-link flow by someone who holds no approve verb
+// (or no session at all): without it that approver is bound by no cap. A
+// manual-approval plan with no cap keeps its human approval as the only
+// control and approves as before (CheckStepCommitmentCap would refuse it);
+// an auto-purchase plan, or a manual one that sets a cap, is checked.
+func (h *Handler) requirePlanStepCap(ctx context.Context, execution *config.PurchaseExecution) error {
+	if execution.PlanID == "" {
+		return nil
+	}
+	plan, err := h.config.GetPurchasePlan(ctx, execution.PlanID)
+	if err != nil {
+		return fmt.Errorf("failed to read plan %s for the approval cap check: %w", execution.PlanID, err)
+	}
+	if !plan.AutoPurchase && plan.MaxStepCommitment == nil {
+		return nil
 	}
-	if hasOwn && execution.CreatedByUserID != nil && *execution.CreatedByUserID == session.UserID {
-		return h.enforcePurchaseConstraints(ctx, session, auth.ActionApproveOwn, execution.Recommendations)
+	if capErr := purchase.CheckStepCommitmentCap(plan, execution.Recommendations); capErr != nil {
+		return NewClientError(403, capErr.Error())
 	}
 	return nil
 }

internal/api/handler_purchases_plan_step_approve_test.go

@@ -0,0 +1,236 @@
+package api
+
+import (
+	"context"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/planstep"
+	"github.com/aws/aws-lambda-go/events"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// These tests use in-memory fakes for the stores and the purchase manager
+// (fixture/mock evidence). The recommendation under approval is the real
+// output of planstep.Resolve, and the permission Constraints are evaluated by
+// the real auth matcher.
+
+type stepSource struct{ shown []config.RecommendationRecord }
+
+func (s stepSource) ListRecommendationsStrict(context.Context, config.RecommendationFilter) ([]config.RecommendationRecord, error) {
+	return s.shown, nil
+}
+
+type stepStore struct{ rows []config.RecommendationRecord }
+
+func (s stepStore) ListStoredRecommendations(context.Context, config.RecommendationFilter) ([]config.RecommendationRecord, error) {
+	return s.rows, nil
+}
+func (s stepStore) GetGlobalConfig(context.Context) (*config.GlobalConfig, error) {
+	return &config.GlobalConfig{}, nil
+}
+
+const (
+	stepPlanID  = "21212121-2121-2121-2121-212121212121"
+	stepExecID  = "31313131-3131-3131-3131-313131313131"
+	stepAcct    = "acct-step"
+	stepContact = "contact@example.com"
+	stepUser    = "41414141-4141-4141-4141-414141414141"
+)
+
+// planStepExecution builds the pending row a plan-step attach produces: the
+// recs come from planstep.Resolve, and there is no CreatedByUserID.
+func planStepExecution(t *testing.T) (*config.PurchaseExecution, float64) {
+	t.Helper()
+	acct := stepAcct
+	monthly := 100.0
+	stored := config.RecommendationRecord{
+		ID: "azure|acct-step|compute|westeurope|Standard_D2s_v3||1|monthly", Provider: "azure", Service: "compute",
+		Region: "westeurope", ResourceType: "Standard_D2s_v3", Term: 1, Payment: "monthly",
+		Count: 20, UpfrontCost: 0, MonthlyCost: &monthly, CloudAccountID: &acct,
+	}
+	plan := &config.PurchasePlan{
+		ID:           stepPlanID,
+		Services:     map[string]config.ServiceConfig{"azure:compute": {Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80}},
+		RampSchedule: config.RampSchedule{PercentPerStep: 25},
+	}
+	recs, err := planstep.Resolve(context.Background(), stepSource{[]config.RecommendationRecord{stored}}, stepStore{[]config.RecommendationRecord{stored}}, plan, []string{stepAcct})
+	require.NoError(t, err)
+	require.Len(t, recs, 1)
+	total := recs[0].TotalCommitment()
+	require.Greater(t, total, 0.0)
+	return &config.PurchaseExecution{
+		ExecutionID: stepExecID, PlanID: stepPlanID, ApprovalToken: "valid-token", Status: "pending",
+		StepNumber: 1, Recommendations: recs,
+	}, total
+}
+
+func cappedPerm(action string, maxAmount float64) auth.Permission {
+	return auth.Permission{Action: action, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: maxAmount}}
+}
+
+type tokenApproveFixture struct {
+	handler  *Handler
+	purchase *MockPurchaseManager
+	req      *events.LambdaFunctionURLRequest
+}
+
+// newTokenApprove wires an approver who is the account's contact_email and
+// holds perms (a nil list holds no approve verb).
+func newTokenApprove(t *testing.T, perms []auth.Permission, plan *config.PurchasePlan) *tokenApproveFixture {
+	t.Helper()
+	ctx := context.Background()
+	mockConfig := new(MockConfigStore)
+	mockConfig.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		return &config.CloudAccount{ID: id, ContactEmail: stepContact}, nil
+	}
+	mockConfig.GetPurchasePlanFn = func(context.Context, string) (*config.PurchasePlan, error) { return plan, nil }
+	mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil)
+	mockAuth := new(MockAuthService)
+	mockAuth.grantPermissions(perms)
+	mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: stepUser, Email: stepContact}, nil)
+	mp := new(MockPurchaseManager)
+	// A reached ApproveExecution proves the approval was not refused up front;
+	// it answers with a conflict so the test needs no email or re-fetch wiring.
+	mp.On("ApproveExecution", mock.Anything, stepExecID, "valid-token", stepContact).Return("", config.ErrExecutionNotInExpectedStatus).Maybe()
+	return &tokenApproveFixture{
+		handler:  &Handler{purchase: mp, config: mockConfig, auth: mockAuth},
+		purchase: mp,
+		req:      &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer tok"}},
+	}
+}
+
+func (f *tokenApproveFixture) approve(exec *config.PurchaseExecution) error {
+	_, err := f.handler.approveViaToken(context.Background(), f.req, exec, "valid-token")
+	return err
+}
+
+func requireStatus(t *testing.T, err error, status int) {
+	t.Helper()
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a clientError, got: %v", err)
+	assert.Equal(t, status, ce.code)
+}
+
+func approveCalls(mp *MockPurchaseManager) int {
+	n := 0
+	for _, c := range mp.Calls {
+		if c.Method == "ApproveExecution" {
+			n++
+		}
+	}
+	return n
+}
+
+func requireApproveReached(t *testing.T, mp *MockPurchaseManager, err error) {
+	t.Helper()
+	requireStatus(t, err, 409)
+	assert.Equal(t, 1, approveCalls(mp), "approval should have been attempted")
+}
+
+func requireApproveRefused(t *testing.T, mp *MockPurchaseManager, err error, status int) {
+	t.Helper()
+	requireStatus(t, err, status)
+	assert.Zero(t, approveCalls(mp), "approval must be refused before the purchase manager")
+}
+
+func autoPlan(cap *float64) *config.PurchasePlan {
+	return &config.PurchasePlan{ID: stepPlanID, AutoPurchase: true, MaxStepCommitment: cap}
+}
+
+func TestApproveViaToken_PlanStep_ApproveOwnCappedContactEmail(t *testing.T) {
+	exec, total := planStepExecution(t)
+
+	over := newTokenApprove(t, []auth.Permission{cappedPerm(auth.ActionApproveOwn, total-1)}, autoPlan(nil))
+	requireApproveRefused(t, over.purchase, over.approve(exec), 403)
+
+	atCap := newTokenApprove(t, []auth.Permission{cappedPerm(auth.ActionApproveOwn, total)}, autoPlan(nil))
+	requireApproveReached(t, atCap.purchase, atCap.approve(exec))
+}
+
+func TestApproveViaToken_PlanStep_ApproveAnyCap(t *testing.T) {
+	exec, total := planStepExecution(t)
+
+	over := newTokenApprove(t, []auth.Permission{cappedPerm(auth.ActionApproveAny, total-1)}, autoPlan(nil))
+	requireApproveRefused(t, over.purchase, over.approve(exec), 403)
+
+	atCap := newTokenApprove(t, []auth.Permission{cappedPerm(auth.ActionApproveAny, total)}, autoPlan(nil))
+	requireApproveReached(t, atCap.purchase, atCap.approve(exec))
+}
+
+func TestApproveViaToken_PlanStep_ZeroTotalIs400(t *testing.T) {
+	exec, _ := planStepExecution(t)
+	exec.Recommendations[0].UpfrontCost = 0
+	exec.Recommendations[0].MonthlyCost = nil
+	f := newTokenApprove(t, []auth.Permission{cappedPerm(auth.ActionApproveOwn, 1000)}, autoPlan(nil))
+	requireApproveRefused(t, f.purchase, f.approve(exec), 400)
+}
+
+func TestApproveViaToken_PlanStep_NoVerbHonorsPlanCap(t *testing.T) {
+	exec, total := planStepExecution(t)
+	under, over := total+1, total-1
+
+	f := newTokenApprove(t, nil, autoPlan(&over))
+	requireApproveRefused(t, f.purchase, f.approve(exec), 403)
+
+	f = newTokenApprove(t, nil, autoPlan(&under))
+	requireApproveReached(t, f.purchase, f.approve(exec))
+
+	f = newTokenApprove(t, nil, autoPlan(&total))
+	requireApproveReached(t, f.purchase, f.approve(exec))
+
+	f = newTokenApprove(t, nil, autoPlan(nil))
+	requireApproveRefused(t, f.purchase, f.approve(exec), 403)
+}
+
+func TestApproveViaToken_PlanStep_ManualPlan(t *testing.T) {
+	exec, total := planStepExecution(t)
+	over := total - 1
+
+	noCap := newTokenApprove(t, nil, &config.PurchasePlan{ID: stepPlanID})
+	requireApproveReached(t, noCap.purchase, noCap.approve(exec))
+
+	withCap := newTokenApprove(t, nil, &config.PurchasePlan{ID: stepPlanID, MaxStepCommitment: &over})
+	requireApproveRefused(t, withCap.purchase, withCap.approve(exec), 403)
+}
+
+func TestApproveConstraintsForSession_PlanStep_SessionlessAndAdminKey(t *testing.T) {
+	ctx := context.Background()
+	exec, total := planStepExecution(t)
+	over := total - 1
+
+	f := newTokenApprove(t, nil, autoPlan(&over))
+	requireStatus(t, f.handler.approveConstraintsForSession(ctx, nil, exec), 403)
+
+	under := total + 1
+	f = newTokenApprove(t, nil, autoPlan(&under))
+	require.NoError(t, f.handler.approveConstraintsForSession(ctx, nil, exec))
+
+	f = newTokenApprove(t, nil, autoPlan(&over))
+	require.NoError(t, f.handler.approveConstraintsForSession(ctx, &Session{UserID: apiKeyAdminUserID}, exec))
+}
+
+func TestApproveConstraintsForSession_UserCreatedRowUnchanged(t *testing.T) {
+	ctx := context.Background()
+	exec, total := planStepExecution(t)
+	exec.PlanID = ""
+	f := newTokenApprove(t, []auth.Permission{cappedPerm(auth.ActionApproveOwn, total-1)}, autoPlan(nil))
+	// Another user's row with no plan: approve-own simply does not apply.
+	other := "51515151-5151-5151-5151-515151515151"
+	exec.CreatedByUserID = &other
+	require.NoError(t, f.handler.approveConstraintsForSession(ctx, &Session{UserID: stepUser, Email: stepContact}, exec))
+}
+
+func TestApproveConstraintsForSession_PlanStep_PlanReadErrorFailsClosed(t *testing.T) {
+	exec, _ := planStepExecution(t)
+	f := newTokenApprove(t, nil, nil)
+	f.handler.config.(*MockConfigStore).GetPurchasePlanFn = func(context.Context, string) (*config.PurchasePlan, error) {
+		return nil, assert.AnError
+	}
+	err := f.handler.approveConstraintsForSession(context.Background(), nil, exec)
+	require.ErrorIs(t, err, assert.AnError)
+}