diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index db5c834c4..12bbfbe1e 100644 --- a/internal/api/handler_purchases.go +++ b/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 } diff --git a/internal/api/handler_purchases_plan_step_approve_test.go b/internal/api/handler_purchases_plan_step_approve_test.go new file mode 100644 index 000000000..ae3178e40 --- /dev/null +++ b/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) +}