diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 3ddb854dc..e8380f42b 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -84,6 +84,8 @@ func TestHandler_approvePurchase_PurchaseError(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) // After issue #286, approvePurchase is session-first: with a Bearer // header present the dispatch consults the approve-{any,own} RBAC @@ -155,6 +157,8 @@ func TestHandler_cancelPurchase_PurchaseError(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) // Session has no admin role / cancel permissions → cancelPurchase's // session-authed pre-check falls through to authorizeApprovalAction → diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go index 5b73ff6ea..2e3f223ab 100644 --- a/internal/api/executed_notification_flow_test.go +++ b/internal/api/executed_notification_flow_test.go @@ -103,6 +103,8 @@ func TestExecutedNotification_TokenApprovePath(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() @@ -166,6 +168,8 @@ func TestExecutedNotification_TokenApprovePath_RefetchFailureSuppressesPanel(t * }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go new file mode 100644 index 000000000..4e32047e2 --- /dev/null +++ b/internal/api/execution_scope_test.go @@ -0,0 +1,397 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-lambda-go/events" + "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 #92 regression guards for the per-execution account-scope gate on the +// execution endpoints. The caller is an admin restricted by allowed_accounts +// who holds every money verb, so every RBAC check passes and only the scope +// gate can stop the action. +const ( + scopeTestExecID = "92929292-0000-0000-0000-000000000001" + scopeTestPlanID = "92929292-0000-0000-0000-000000000002" +) + +var errScopeTestReached = errors.New("mutation reached") + +var ( + scopeTestProd = config.CloudAccount{ID: "acc-prod", Name: "Production", ContactEmail: "prod-owner@example.com"} + scopeTestStage = config.CloudAccount{ID: "acc-stage", Name: "Staging", ContactEmail: "operator@example.com"} +) + +// scopeTestKind is one shape of execution, differing in how its buy-in +// accounts are derived (executionAccounts). +type scopeTestKind struct { + name string + planID string + cloudAccountID *string + recs []config.RecommendationRecord + // partialScope holds some but not all of the accounts the execution can + // buy in (or, for the single-account child, the plan's other account). + partialScope []string +} + +// scopeTestRec is a purchasable recommendation, so an allowed call clears +// the post-scope constraint checks and reaches the mutation. +func scopeTestRec(id string, account *string) config.RecommendationRecord { + return config.RecommendationRecord{ + ID: id, Provider: "aws", Service: "ec2", Region: "us-east-1", + Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, CloudAccountID: account, + } +} + +var scopeTestBothRecs = []config.RecommendationRecord{ + scopeTestRec("rec-prod", &scopeTestProd.ID), + scopeTestRec("rec-stage", &scopeTestStage.ID), +} + +var scopeTestKinds = []scopeTestKind{ + // Plan-level parent: CloudAccountID nil, fans out to every plan account. + {name: "plan parent", planID: scopeTestPlanID, recs: scopeTestBothRecs, partialScope: []string{scopeTestProd.Name}}, + // Fan-out child of the same plan, buying only in Staging. + {name: "fan-out child", planID: scopeTestPlanID, cloudAccountID: &scopeTestStage.ID, + recs: []config.RecommendationRecord{scopeTestRec("rec-stage", &scopeTestStage.ID)}, partialScope: []string{scopeTestProd.Name}}, + // Ad-hoc web purchase: plan_id NULL, one recommendation per account. + {name: "ad-hoc", recs: scopeTestBothRecs, partialScope: []string{scopeTestProd.Name}}, +} + +type scopeTestScope struct { + name string + scope func(k scopeTestKind) []string + allow bool +} + +var scopeTestScopes = []scopeTestScope{ + {name: "all in scope", allow: true, scope: func(scopeTestKind) []string { return []string{scopeTestProd.Name, scopeTestStage.Name} }}, + {name: "partial scope", scope: func(k scopeTestKind) []string { return k.partialScope }}, + {name: "no scope", scope: func(scopeTestKind) []string { return []string{"Other"} }}, +} + +type scopeTestEndpoint struct { + name string + status string + call func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) + // readOnly endpoints reach no mutation, so an allowed call succeeds + // instead of answering errScopeTestReached. + readOnly bool +} + +var scopeTestEndpoints = []scopeTestEndpoint{ + { + name: "approve", status: "pending", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.approvePurchase(context.Background(), req, scopeTestExecID, "") + }, + }, + { + name: "cancel", status: "pending", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.cancelPurchase(context.Background(), req, scopeTestExecID, "") + }, + }, + { + name: "retry", status: "failed", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.retryPurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "revoke completed via session", status: "completed", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }, + }, + { + name: "revoke scheduled", status: "scheduled", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokePurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "run now", status: "pending", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.runPlannedPurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "pause", status: "pending", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.pausePlannedPurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "resume", status: "paused", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.resumePlannedPurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "delete", status: "pending", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.deletePlannedPurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "details", status: "completed", readOnly: true, + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.getPurchaseDetails(context.Background(), req, scopeTestExecID) + }, + }, +} + +// scopeTestExecution builds the execution row for a kind. +func scopeTestExecution(k scopeTestKind, status string) *config.PurchaseExecution { + creator := "operator-1" + return &config.PurchaseExecution{ + ExecutionID: scopeTestExecID, + PlanID: k.planID, + Status: status, + CloudAccountID: k.cloudAccountID, + Recommendations: k.recs, + CreatedByUserID: &creator, + } +} + +// newScopeTestHandler wires a handler whose session is scoped to scope and +// whose store serves exec. GetPlanAccounts("") errors the way the UUID +// plan_id column does in Postgres, so a gate that queries it for an ad-hoc +// execution surfaces as a 500 rather than a quiet deny. +// +// The caller constructs the mocks so their types resolve at its assertion +// sites for TestNoUnfailableMockAssertions. +func newScopeTestHandler(t *testing.T, exec *config.PurchaseExecution, scope []string, store *MockConfigStore, mockPurchase *MockPurchaseManager) *Handler { + t.Helper() + mockAuth := new(MockAuthService) + + mockAuth.On("ValidateSession", mock.Anything, "operator-token").Return(&Session{ + UserID: "operator-1", Email: "operator@example.com", + }, nil) + mockAuth.On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + // admin:* plus the money verbs it deliberately does not cover + // (adminCarvedOuts), so every RBAC gate passes. + mockAuth.grantPermissionsScoped([]auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases}, + {Action: auth.ActionApproveAny, Resource: auth.ResourcePurchases}, + {Action: auth.ActionRetryAny, Resource: auth.ResourcePurchases}, + }, scope) + + store.On("GetExecutionByID", mock.Anything, scopeTestExecID).Return(exec, nil) + store.GetPlanAccountsFn = func(_ context.Context, planID string) ([]config.CloudAccount, error) { + switch planID { + case "": + return nil, errors.New(`invalid input syntax for type uuid: ""`) + case scopeTestPlanID: + return []config.CloudAccount{scopeTestProd, scopeTestStage}, nil + } + return nil, nil + } + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + for _, a := range []config.CloudAccount{scopeTestProd, scopeTestStage} { + if a.ID == id { + return &a, nil + } + } + return nil, nil + } + // Every mutation answers errScopeTestReached, so an allowed call proves + // it got past all gates without wiring each endpoint's success path. + store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(errScopeTestReached).Maybe() + store.On("TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errScopeTestReached).Maybe() + store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errScopeTestReached).Maybe() + store.On("CancelScheduledExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errScopeTestReached).Maybe() + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errScopeTestReached).Maybe() + mockPurchase.On("RunPlannedPurchaseNow", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errScopeTestReached).Maybe() + mockPurchase.On("CancelExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + + return &Handler{auth: mockAuth, config: store, purchase: mockPurchase} +} + +func scopeTestRequest() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer operator-token", "X-CSRF-Token": "csrf"}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "POST"}, + }, + } +} + +// TestExecutionEndpoints_ScopeMatrix requires EVERY account an execution can +// buy in to be in scope: a session holding only some of them (or none) gets +// the enumeration-safe 404 and never reaches the mutation, while a session +// holding all of them passes the gate. +func TestExecutionEndpoints_ScopeMatrix(t *testing.T) { + for _, k := range scopeTestKinds { + for _, sc := range scopeTestScopes { + for _, ep := range scopeTestEndpoints { + t.Run(k.name+"/"+sc.name+"/"+ep.name, func(t *testing.T) { + store := new(MockConfigStore) + mockPurchase := new(MockPurchaseManager) + h := newScopeTestHandler(t, scopeTestExecution(k, ep.status), sc.scope(k), store, mockPurchase) + + _, err := ep.call(h, scopeTestRequest()) + + if !sc.allow { + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + // A denied call reaches no mutation of any endpoint. + store.AssertNotCalled(t, "SavePurchaseExecution") + store.AssertNotCalled(t, "TransitionExecutionStatus") + store.AssertNotCalled(t, "CancelExecutionAtomic") + store.AssertNotCalled(t, "CancelScheduledExecutionAtomic") + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "RunPlannedPurchaseNow", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "CancelExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + return + } + if ep.readOnly { + require.NoError(t, err) + return + } + assert.ErrorContains(t, err, errScopeTestReached.Error()) + }) + } + } + } +} + +// TestExecutionEndpoints_OutOfScopeNoTokenIs404 guards the token fall-through: +// without a token an out-of-scope session must get the same 404 as a missing +// row, not the token/session branch's status guards (409/410/401), which +// would confirm the execution exists and leak its status. +func TestExecutionEndpoints_OutOfScopeNoTokenIs404(t *testing.T) { + adhoc := scopeTestKinds[2] + for _, tc := range []struct { + name string + status string + call func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) + }{ + {"approve wrong status", "completed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.approvePurchase(context.Background(), req, scopeTestExecID, "") + }}, + {"cancel wrong status", "completed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.cancelPurchase(context.Background(), req, scopeTestExecID, "") + }}, + {"revoke completed", "completed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }}, + {"revoke pending", "pending", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }}, + {"revoke notified", "notified", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }}, + {"revoke failed", "failed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }}, + } { + t.Run(tc.name, func(t *testing.T) { + h := newScopeTestHandler(t, scopeTestExecution(adhoc, tc.status), []string{"Other"}, new(MockConfigStore), new(MockPurchaseManager)) + _, err := tc.call(h, scopeTestRequest()) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + }) + } +} + +// TestRevoke_AuthorizedNonRevocableStatusIs409: moving the status check +// behind authorization must not drop it. An authorized caller, via the +// session or the email token, still gets the 409 for a status with nothing +// to revoke, and no mutation runs. +func TestRevoke_AuthorizedNonRevocableStatusIs409(t *testing.T) { + stageOnly := scopeTestKind{recs: []config.RecommendationRecord{scopeTestRec("rec-stage", &scopeTestStage.ID)}} + for _, tc := range []struct { + name string + scope []string + token string + }{ + {"session", []string{scopeTestStage.Name}, ""}, + {"token", []string{"Other"}, "email-token"}, + } { + for _, status := range []string{"pending", "notified", "failed"} { + t.Run(tc.name+"/"+status, func(t *testing.T) { + exec := scopeTestExecution(stageOnly, status) + exec.ApprovalToken = "email-token" + store := new(MockConfigStore) + h := newScopeTestHandler(t, exec, tc.scope, store, new(MockPurchaseManager)) + + _, err := h.revokeViaEmailToken(context.Background(), scopeTestRequest(), scopeTestExecID, tc.token) + + var ce *clientError + require.ErrorAs(t, err, &ce) + assert.Equal(t, 409, ce.code, "got %v", err) + if status == "pending" { + assert.Contains(t, ce.message, "Cancel") + } + store.AssertNotCalled(t, "TransitionExecutionStatus") + }) + } + } +} + +// TestCancel_OutOfScopeSessionWithTokenUsesContactGate is the positive +// control for the fall-through: a session outside the account scope that +// carries the email token reaches the token branch, and succeeds because the +// session email is the account's contact_email. +func TestCancel_OutOfScopeSessionWithTokenUsesContactGate(t *testing.T) { + exec := scopeTestExecution(scopeTestKind{recs: []config.RecommendationRecord{ + scopeTestRec("rec-stage", &scopeTestStage.ID), + }}, "pending") + mockPurchase := new(MockPurchaseManager) + h := newScopeTestHandler(t, exec, []string{scopeTestProd.Name}, new(MockConfigStore), mockPurchase) + + res, err := h.cancelPurchase(context.Background(), scopeTestRequest(), scopeTestExecID, "email-token") + + require.NoError(t, err) + assert.Equal(t, map[string]string{"status": "canceled"}, res) + mockPurchase.AssertCalled(t, "CancelExecution", mock.Anything, scopeTestExecID, "email-token", "operator@example.com") +} + +// TestExecutionAccounts_AdHocUnattributedDenied: an ad-hoc execution with a +// recommendation lacking a cloud account cannot be attributed, so a scoped +// session is denied even when every attributed account is in scope. +func TestExecutionAccounts_AdHocUnattributedDenied(t *testing.T) { + empty := "" + for _, missing := range []*string{nil, &empty} { + exec := scopeTestExecution(scopeTestKind{recs: []config.RecommendationRecord{ + scopeTestRec("rec-prod", &scopeTestProd.ID), + scopeTestRec("rec-none", missing), + }}, "pending") + h := newScopeTestHandler(t, exec, []string{scopeTestProd.Name, scopeTestStage.Name}, new(MockConfigStore), new(MockPurchaseManager)) + err := h.requireExecutionAccess(context.Background(), &Session{UserID: "operator-1"}, scopeTestExecID) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + } +} + +// TestPlannedPurchases_HidesPartiallyScopedParent: the list filters with the +// same rule as the per-row gate, so a plan parent that also buys in an +// out-of-scope account is hidden from a partially-scoped session. +func TestPlannedPurchases_HidesPartiallyScopedParent(t *testing.T) { + for _, tc := range []struct { + scope []string + want int + }{ + {[]string{scopeTestProd.Name}, 0}, + {[]string{scopeTestProd.Name, scopeTestStage.Name}, 1}, + } { + exec := scopeTestExecution(scopeTestKinds[0], "pending") + store := new(MockConfigStore) + h := newScopeTestHandler(t, exec, tc.scope, store, new(MockPurchaseManager)) + store.On("GetPlannedExecutions", mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{*exec}, nil) + store.On("ListPurchasePlans", mock.Anything, mock.Anything).Return([]config.PurchasePlan{{ID: scopeTestPlanID, Name: "p"}}, nil) + + resp, err := h.getPlannedPurchases(context.Background(), scopeTestRequest()) + require.NoError(t, err) + assert.Len(t, resp.Purchases, tc.want, "scope %v", tc.scope) + } +} diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 20294e64d..b468029b0 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -429,10 +429,12 @@ func (h *Handler) getUpcomingPurchases(ctx context.Context, req *events.LambdaFu planMap[plans[i].ID] = &plans[i] } - // Per-plan access cache mirrors getPlannedPurchases — all executions - // for the same plan share the same account scope, so we resolve once - // per plan. - allowedPlan := make(map[string]bool) + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + // Memoises GetPlanAccounts, mirroring getPlannedPurchases. + planAccounts := make(map[string][]config.CloudAccount) var upcoming []UpcomingPurchase for i := range executions { @@ -443,7 +445,7 @@ func (h *Handler) getUpcomingPurchases(ctx context.Context, req *events.LambdaFu // Hide rather than crash; cleanup is a separate concern. continue } - ok, err := h.isPlanAllowedCached(ctx, session, exec.PlanID, allowedPlan) + ok, err := h.isExecutionInScope(ctx, scope, &exec, planAccounts) if err != nil { return nil, err } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 713ce6d37..d9e386c6a 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -128,9 +128,12 @@ func (h *Handler) getPlannedPurchases(ctx context.Context, req *events.LambdaFun planMap[plans[i].ID] = &plans[i] } - // Cache per-plan access decisions — all executions for the same plan share - // the same account scope. Avoids GetPlanAccounts round-trips per execution. - allowedPlan := make(map[string]bool) + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + // Memoises GetPlanAccounts: plan-level parents of the same plan share it. + planAccounts := make(map[string][]config.CloudAccount) var purchases []PlannedPurchase for _rvc := range executions { @@ -139,7 +142,7 @@ func (h *Handler) getPlannedPurchases(ctx context.Context, req *events.LambdaFun if plan == nil { continue } - ok, err := h.isPlanAllowedCached(ctx, session, exec.PlanID, allowedPlan) + ok, err := h.isExecutionInScope(ctx, scope, &exec, planAccounts) if err != nil { return nil, err } @@ -154,20 +157,15 @@ func (h *Handler) getPlannedPurchases(ctx context.Context, req *events.LambdaFun }, nil } -// isPlanAllowedCached resolves and memoises whether the session may see the given plan. -// NotFound errors are treated as "not allowed" (not an error) so missing plans don't -// surface as 500s, mirroring the previous inline behavior. -func (h *Handler) isPlanAllowedCached(ctx context.Context, session *Session, planID string, cache map[string]bool) (bool, error) { - if ok, cached := cache[planID]; cached { - return ok, nil +// isExecutionInScope is requireExecutionInScope for list filtering: an +// out-of-scope execution is hidden (false) rather than an error, while store +// failures still propagate. +func (h *Handler) isExecutionInScope(ctx context.Context, scope auth.AccountScope, exec *config.PurchaseExecution, planAccounts map[string][]config.CloudAccount) (bool, error) { + err := h.requireExecutionInScope(ctx, scope, exec, planAccounts) + if IsNotFoundError(err) { + return false, nil } - planErr := h.requirePlanAccess(ctx, session, planID) - if planErr != nil && !IsNotFoundError(planErr) { - return false, planErr - } - ok := planErr == nil - cache[planID] = ok - return ok, nil + return err == nil, err } // buildPlannedPurchase converts a (plan, execution) pair into the API-facing PlannedPurchase. @@ -594,13 +592,14 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio // approvePurchaseViaSession runs the approve-any / // approve-own RBAC matrix and rejects sessions without it. if session := h.tryGetSession(ctx, req); session != nil { - switch err := h.authorizeSessionApprove(ctx, session, execution); { + switch err := h.authorizeScopedSessionApprove(ctx, session, execution); { case err == nil: return h.approvePurchaseViaSession(ctx, req, execution) - case isPermissionDenied(err): - // Explicit 403 → fall through to the token branch so the - // contact_email gate gets a chance (a logged-in user without - // approve-* may still be the per-account contact recipient). + case fallsThroughToToken(err, token): + // Explicit 403, or an out-of-scope 404 with a token → fall + // through to the token branch so the contact_email gate gets a + // chance (a logged-in user without approve-* or outside the + // account scope may still be the per-account contact recipient). default: // Transient failure — propagate instead of silently widening. return nil, err @@ -728,15 +727,7 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be approved (status=%s)", execution.ExecutionID, execution.Status)) } - if err := h.authorizeSessionApprove(ctx, session, execution); err != nil { - return nil, err - } - - // 4-eyes mode (issue #1005): enforce after RBAC so only users who already - // pass the approve-any / approve-own gate reach this check. The admin - // wildcard inside authorizeSessionApprove short-circuits RBAC but NOT this - // check; admins who created the row must disable the mode first. - if err := h.requireDifferentApprover(ctx, session, execution); err != nil { + if err := h.authorizeApproveSession(ctx, session, execution); err != nil { return nil, err } @@ -780,6 +771,31 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam return map[string]string{"status": "completed"}, nil } +// authorizeApproveSession runs every gate approvePurchaseViaSession must pass +// before mutating state: the account-scope check (issue #92), the +// approve-any/approve-own RBAC matrix with its Constraints, and the 4-eyes +// distinct-approver rule. +func (h *Handler) authorizeApproveSession(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { + if err := h.authorizeScopedSessionApprove(ctx, session, execution); err != nil { + return err + } + // 4-eyes mode (issue #1005): enforce after RBAC so only users who already + // pass the approve-any / approve-own gate reach this check. The admin + // wildcard inside authorizeSessionApprove short-circuits RBAC but NOT this + // check; admins who created the row must disable the mode first. + return h.requireDifferentApprover(ctx, session, execution) +} + +// authorizeScopedSessionApprove runs the account-scope gate (issue #92) +// before the approve RBAC and Constraints checks, so an out-of-scope caller +// gets the enumeration-safe 404 before a 403 can reveal the execution exists. +func (h *Handler) authorizeScopedSessionApprove(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { + if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { + return err + } + return h.authorizeSessionApprove(ctx, session, execution) +} + // authorizeSessionApprove returns nil when the session is permitted to // approve the given execution under the approve-any / approve-own RBAC // rules added in issue #286. Returns a 403 ClientError otherwise. @@ -1178,15 +1194,16 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction // token. cancelPurchaseViaSession runs the cancel-any / // cancel-own RBAC matrix and rejects sessions without it. if session := h.tryGetSession(ctx, req); session != nil { - switch err := h.authorizeSessionCancel(ctx, session, execution); { + switch err := h.authorizeCancelSession(ctx, session, execution); { case err == nil: // Session is RBAC-authorized → run the session-authed cancel. return h.cancelPurchaseViaSession(ctx, req, execution) - case isPermissionDenied(err): - // Explicit "permission denied" (403) → fall through to the - // token branch so the contact_email gate still gets a chance - // (a logged-in user without admin / cancel-* may still be - // the per-account contact email recipient). + case fallsThroughToToken(err, token): + // Explicit "permission denied" (403), or an out-of-scope 404 + // with a token → fall through to the token branch so the + // contact_email gate still gets a chance (a logged-in user + // without admin / cancel-* may still be the per-account contact + // email recipient). default: // Transient failure (auth-service down, HasPermissionAPI // returning a wrapped error, h.auth==nil 500). Propagate @@ -1262,7 +1279,7 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb return nil, err } - if err := h.authorizeSessionCancel(ctx, session, execution); err != nil { + if err := h.authorizeCancelSession(ctx, session, execution); err != nil { return nil, err } @@ -1370,14 +1387,8 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun return nil, NewClientError(404, "execution not found") } - // Only completed/partially_completed purchases have anything to revoke. - // A pending/notified purchase should be canceled instead. - if statusErr := checkRevokableStatus(execution); statusErr != nil { - return nil, statusErr - } - // Three-mode dispatch — same shape as cancelPurchase. - result, handled, revokeErr := h.tryRevokeViaSession(ctx, req, execution) + result, handled, revokeErr := h.tryRevokeViaSession(ctx, req, execution, token) if handled { return result, revokeErr } @@ -1405,6 +1416,9 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun if err := validateRevokeToken(execution, token); err != nil { return nil, err } + if err := checkRevokableStatus(execution); err != nil { + return nil, err + } return h.revokeViaSession(ctx, execution, actor) } @@ -1438,12 +1452,12 @@ func renderRevokeConfirmPage(execID, token string) *rawResponse { // (nil, false, nil) when the session was absent or returned a permission-denied // error so the caller can fall through to the token branch. Extracted from // revokeViaEmailToken to keep that function under the cyclomatic limit. -func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution) (result any, handled bool, err error) { +func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, token string) (result any, handled bool, err error) { session := h.tryGetSession(ctx, req) if session == nil { return nil, false, nil } - switch sessErr := h.authorizeSessionCancel(ctx, session, execution); { + switch sessErr := h.authorizeCancelSession(ctx, session, execution); { case sessErr == nil: // These endpoints are AuthPublic so the outer middleware skips CSRF. // Enforce it here for the session-authed revoke sub-path, consistent @@ -1452,9 +1466,14 @@ func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFun if csrfErr := h.validateCSRF(ctx, req); csrfErr != nil { return nil, true, NewClientError(403, "CSRF validation failed") } + // Status is checked only once the caller is authorized, so an + // out-of-scope caller cannot learn it from a 409. + if statusErr := checkRevokableStatus(execution); statusErr != nil { + return nil, true, statusErr + } res, revokeErr := h.revokeViaSession(ctx, execution, session.Email) return res, true, revokeErr - case isPermissionDenied(sessErr): + case fallsThroughToToken(sessErr, token): // Fall through to the token branch. return nil, false, nil default: @@ -1594,6 +1613,17 @@ func (h *Handler) revokeViaSession(ctx context.Context, execution *config.Purcha }, nil } +// authorizeCancelSession runs the account-scope gate (issue #92) before the +// cancel-any/cancel-own RBAC matrix, so an out-of-scope caller gets the +// enumeration-safe 404 before a 403 can reveal the execution exists. Shared +// by the cancel and revoke session paths. +func (h *Handler) authorizeCancelSession(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { + if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { + return err + } + return h.authorizeSessionCancel(ctx, session, execution) +} + // authorizeSessionCancel returns nil when the session is permitted to cancel // the given execution under the cancel-any / cancel-own RBAC rules added in // issue #46. Returns a 403 ClientError otherwise. @@ -1807,16 +1837,7 @@ func (h *Handler) loadAndValidateRetryRequest(ctx context.Context, req *events.L return nil, nil, err } - if failedExec.Status != "failed" { - return nil, nil, NewClientError(409, fmt.Sprintf("execution %s cannot be retried (status=%s)", failedExec.ExecutionID, failedExec.Status)) - } - - // Authorize BEFORE the already-retried guard so an unauthorized - // caller can't enumerate descendant execution IDs by probing - // failed-row UUIDs (CR #168 review). The status check above is - // allowed pre-RBAC because non-admins can already see the row's - // status via the History endpoint they're entitled to read. - if err := h.authorizeSessionRetry(ctx, session, failedExec); err != nil { + if err := h.authorizeRetrySession(ctx, session, failedExec); err != nil { return nil, nil, err } @@ -1838,6 +1859,31 @@ func (h *Handler) loadAndValidateRetryRequest(ctx context.Context, req *events.L return failedExec, session, nil } +// authorizeRetrySession runs the account-scope gate (issue #92), the status +// guard, and the retry-any/retry-own RBAC matrix, in that order. Extracted +// from loadAndValidateRetryRequest to keep that function under the +// cyclomatic-complexity ceiling; the order is preserved from the original +// inline sequence, so the security boundary is unchanged. +func (h *Handler) authorizeRetrySession(ctx context.Context, session *Session, failedExec *config.PurchaseExecution) error { + // Account-scope gate (issue #92): retry relied only on the retry-any/ + // retry-own RBAC matrix below and never consulted allowed_accounts. Runs + // before the status check so an out-of-scope caller learns nothing about + // the execution, matching the 404 a scoped-out session already gets from + // the History endpoint for the same row. + if err := h.requireExecutionAccess(ctx, session, failedExec.ExecutionID); err != nil { + return err + } + if failedExec.Status != "failed" { + return NewClientError(409, fmt.Sprintf("execution %s cannot be retried (status=%s)", failedExec.ExecutionID, failedExec.Status)) + } + // Authorize BEFORE the already-retried guard so an unauthorized + // caller can't enumerate descendant execution IDs by probing + // failed-row UUIDs (CR #168 review). The status check above is + // allowed pre-RBAC because non-admins can already see the row's + // status via the History endpoint they're entitled to read. + return h.authorizeSessionRetry(ctx, session, failedExec) +} + // checkRetryEligibilityGates runs the provider re-drive-safety (issue // #1668), persistent-failure (Q3) and retry-attempt-threshold (Q2) // gates and returns the appropriate 409 ClientError when any fires. @@ -2185,6 +2231,18 @@ func wrapConstraintDenied(err error) error { return &constraintDeniedError{clientError: ce} } +// fallsThroughToToken reports whether a session-authorization failure should +// hand the request to the email-token branch: an RBAC denial (403), or an +// account-scope miss (404) when a token is present. The token branch's +// contact_email gate is deliberately not account-scoped (issue #92). Without +// a token the 404 is returned as-is, so the token branch's status guards +// cannot reveal an out-of-scope execution's existence or status. Both +// matches are strict, like isPermissionDenied, so a wrapped error from +// deeper in the chain propagates. +func fallsThroughToToken(err error, token string) bool { + return isPermissionDenied(err) || (token != "" && err == errNotFound) //nolint:errorlint // strict sentinel identity is deliberate +} + // isPermissionDenied reports whether err is *directly* a 403 ClientError // (not merely something that wraps one). Used by the cancel-from-email // session pre-check to distinguish a legitimate "your session lacks @@ -2273,8 +2331,11 @@ func (h *Handler) getPurchaseDetails(ctx context.Context, req *events.LambdaFunc return nil, fmt.Errorf("failed to get execution: %w", err) } - // Scope: reject if the execution's plan isn't accessible to the session. - if err := h.requirePlanAccess(ctx, session, execution.PlanID); err != nil { + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + if err := h.requireExecutionInScope(ctx, scope, execution, nil); err != nil { return nil, err } diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index 884863d7a..b4ced4e15 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -246,6 +246,11 @@ func (h *Handler) revokeScheduledExecution(ctx context.Context, session *Session // cancel it before any cloud call. Let CancelScheduledExecutionAtomic be the // sole arbiter: it returns canceled=false (-> 410) only when the row has // actually moved out of "scheduled". + // Account-scope gate (issue #92), ahead of RBAC so an out-of-scope caller + // gets the enumeration-safe 404 rather than a 403 that confirms the row. + if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { + return nil, err + } if err := h.authorizeSessionRevokeExecution(ctx, session, execution); err != nil { return nil, err } diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index 7e889f8af..5c3dee2e3 100644 --- a/internal/api/handler_purchases_revoke_test.go +++ b/internal/api/handler_purchases_revoke_test.go @@ -727,6 +727,9 @@ func TestRevokePurchase_ScheduledExecution_RevokeOwnCreator(t *testing.T) { mockAuth.On("ValidateSession", ctx, "tok").Return(sess, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "revoke-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "revoke-own", "purchases").Return(true, nil) + // requireExecutionAccess (issue #92) runs before RBAC; unrestricted + // here since scope is not under test. + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) exec := scheduledExecution(execID, userID) mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) @@ -746,6 +749,7 @@ func TestRevokePurchase_ScheduledExecution_RevokeOwnWrongCreator(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() t.Cleanup(func() { mockStore.AssertExpectations(t) mockAuth.AssertExpectations(t) @@ -806,6 +810,122 @@ func TestAuthorizeSessionRevokeExecution_NilCreatorDenied(t *testing.T) { assert.Contains(t, ce.message, "cannot revoke another user") } +// TestAuthorizeSessionRevokeExecution_Matrix is the audit finding A16-009 +// regression guard. Before this test, the only three tests reaching +// authorizeSessionRevokeExecution (TestRevokePurchase_ScheduledExecution_ +// RevokeOwnCreator/RevokeOwnWrongCreator and +// TestAuthorizeSessionRevokeExecution_NilCreatorDenied) all stubbed +// revoke-any=false and revoke-own=true, so nothing proved a session holding +// NEITHER permission was refused, nor that the revoke-any allow path or +// either permission-lookup error path worked. Table-drives the four +// remaining branches of the function (handler_purchases_revoke.go): +// +// - hasAny allow (:312): revoke-any:purchases grants access regardless of +// creator, mirroring the sibling authorizeSessionRevoke's revoke-any path. +// - !hasOwn 403 (:320): neither revoke-any nor revoke-own must fail closed. +// - revoke-any lookup error (:309): a HasPermissionAPI failure on the FIRST +// check must propagate rather than silently falling through to the +// revoke-own check. +// - revoke-own lookup error (:317): a HasPermissionAPI failure on the +// SECOND check must propagate rather than being read as "no own +// permission" and denied with the wrong (403) error. +func TestAuthorizeSessionRevokeExecution_Matrix(t *testing.T) { + t.Parallel() + + creator := "u-owner" + permErr := errors.New("permission service unavailable") + + tests := []struct { + name string + hasAnyReturn bool + hasAnyErr error + hasOwnReturn bool + hasOwnErr error + callsHasOwn bool + creatorUserID *string + wantErr bool + wantClientErr bool + wantCode int + }{ + { + // :312 hasAny allow -- grants access without ever consulting + // revoke-own or the execution's creator. + name: "revoke-any allows regardless of creator", + hasAnyReturn: true, + callsHasOwn: false, + wantErr: false, + }, + { + // :320 !hasOwn 403 -- neither permission held. + name: "neither permission denies with 403", + hasAnyReturn: false, + hasOwnReturn: false, + callsHasOwn: true, + creatorUserID: &creator, + wantErr: true, + wantClientErr: true, + wantCode: 403, + }, + { + // :309 revoke-any lookup error -- must propagate as a bare error, + // not get swallowed into a 403 permission-denied. + name: "revoke-any lookup error propagates", + hasAnyErr: permErr, + callsHasOwn: false, + wantErr: true, + }, + { + // :317 revoke-own lookup error -- must propagate as a bare error + // once revoke-any has already returned false. + name: "revoke-own lookup error propagates", + hasAnyReturn: false, + hasOwnErr: permErr, + callsHasOwn: true, + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockAuth.On("HasPermissionAPI", ctx, "u-1", "revoke-any", "purchases"). + Return(tc.hasAnyReturn, tc.hasAnyErr) + if tc.callsHasOwn { + mockAuth.On("HasPermissionAPI", ctx, "u-1", "revoke-own", "purchases"). + Return(tc.hasOwnReturn, tc.hasOwnErr) + } + + h := &Handler{auth: mockAuth} + sess := &Session{UserID: "u-1"} + exec := &config.PurchaseExecution{ExecutionID: "e-1", CreatedByUserID: tc.creatorUserID} + err := h.authorizeSessionRevokeExecution(ctx, sess, exec) + + if !tc.wantErr { + require.NoError(t, err) + return + } + require.Error(t, err) + if !tc.wantClientErr { + // A propagated lookup error must NOT be misread as a + // ClientError (which would surface as a clean 403/404 to the + // caller instead of the 500 a transient auth-backend failure + // deserves). + _, ok := IsClientError(err) + assert.False(t, ok, "expected a bare propagated error, got ClientError: %v", err) + assert.ErrorIs(t, err, permErr) + return + } + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, tc.wantCode, ce.code) + }) + } +} + // --- Two-step quote-then-confirm: Finding #4 TOCTOU tests --- // stubCalcRefundClientWithAmount is a CalculateRefund stub that returns a @@ -1544,6 +1664,7 @@ func TestRevokePurchase_ConcurrentScheduledRevoke_OneWinsOneGets410(t *testing.T t.Parallel() mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() t.Cleanup(func() { mockStore.AssertExpectations(t) mockAuth.AssertExpectations(t) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 798a71e4f..b75ac8d5d 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -58,6 +58,8 @@ func TestHandler_approvePurchase(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) // After issue #286 the session-authed approve dispatch consults the // approve-{any,own} verb matrix BEFORE falling through to the token @@ -99,6 +101,8 @@ func TestHandler_cancelPurchase(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) // Session has no admin role and no cancel permissions, so cancelPurchase's // session-authed pre-check (added to fix the deep-link contact_email gate) @@ -135,6 +139,8 @@ func TestHandler_approvePurchase_RejectsMismatchedSession(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Session belongs to someone who is NOT the authorized approver. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: "wrong@example.com"}, nil) // After issue #286 the dispatch consults approve-{any,own} BEFORE @@ -193,6 +199,8 @@ func TestHandler_approvePurchase_RejectsMissingContactEmail(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: globalNotify}, nil) // Issue #286 dispatch consults approve-{any,own} BEFORE the contact_email // gate. globalNotify session has neither verb → 403 → fall through to @@ -259,6 +267,8 @@ func TestHandler_approvePurchase_AcceptsContactEmailSession(t *testing.T) { } mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Session email matches the account contact email — global notify is // NOT enough here because a contact email exists for the account. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) @@ -427,6 +437,8 @@ func TestHandler_approvePurchase_SessionApproveAny_PermissionConstraintsDenied(t mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() t.Cleanup(func() { mockAuth.AssertExpectations(t) }) userSession := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", Email: "capped@example.com"} mockAuth.On("ValidateSession", ctx, "sess-tok").Return(userSession, nil) @@ -503,6 +515,8 @@ func TestHandler_approvePurchase_SessionApproveAny_TokenPresent_ConstraintsDenie // (wired to succeed) to isolate approveConstraintsForSession's own check. mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockAuth.On("ValidateSession", ctx, "capped-token").Return(capped, nil) mockAuth.On("HasPermissionAPI", ctx, capped.UserID, "approve-any", "purchases").Return(true, nil) @@ -584,6 +598,8 @@ func TestHandler_approveViaToken_GlobalConfigError_FailsClosed(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(nil, errors.New("db transient error")) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) // No approve-any / approve-own permissions — dispatch falls to token path. mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() @@ -910,6 +926,8 @@ func TestHandler_approvePurchase_RejectsGlobalNotifyWhenContactSet(t *testing.T) } mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: globalNotify}, nil) // Issue #286: dispatch consults approve-{any,own} BEFORE the // contact_email gate. Returning false for both verbs lets the @@ -959,6 +977,8 @@ func TestHandler_approvePurchase_RejectsCreatorWithoutApprovePermission(t *testi mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Session belongs to the creator themselves but holds no approve verb. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{ UserID: creatorUserID, @@ -2896,6 +2916,101 @@ func TestHandler_pausePlannedPurchase_OutOfScope(t *testing.T) { mockStore.AssertNotCalled(t, "TransitionExecutionStatus") } +// TestHandler_approvePurchase_OutOfScope is the issue #92 regression guard: +// approvePurchase (unlike pause/resume/run/delete-planned) relied only on the +// approve-any/approve-own RBAC matrix and never consulted the session's +// allowed_accounts, so a scoped operator holding approve-any could approve +// -- and fire the real SDK purchase for -- an execution belonging to an +// account outside their scope. A session scoped to allowed_accounts= +// ["Production"] holding approve-any:purchases must get the same 404 that +// requireExecutionAccess already returns for pause/resume/delete-planned +// when acting on an execution whose plan is scoped to a different account +// ("Staging"). Locks down that approvePurchaseViaSession never reaches +// GetGlobalConfig (the first store call past the scope gate) for an +// out-of-scope execution. +func TestHandler_approvePurchase_OutOfScope(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + executionID := "aaaaaaaa-1111-1111-1111-111111111111" + planID := "bbbbbbbb-1111-1111-1111-111111111111" + + mockAuth.On("ValidateSession", ctx, "operator-token").Return(&Session{ + UserID: "operator-1", Email: "operator@example.com", + }, nil) + // approve-any:purchases -- the RBAC matrix that pre-fix was the ONLY gate + // on this endpoint. + mockAuth.On("HasPermissionAPI", ctx, "operator-1", "approve-any", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "operator-1").Return([]string{"Production"}, nil) + mockAuth.On("ValidateCSRFToken", ctx, "operator-token", "").Return(nil) + + mockStore.On("GetExecutionByID", ctx, executionID).Return(&config.PurchaseExecution{ + ExecutionID: executionID, PlanID: planID, Status: "pending", + }, nil) + + store := &mockStoreWithPlanAccounts{ + MockConfigStore: mockStore, + planAccounts: map[string][]config.CloudAccount{ + planID: {{ID: "acc-stage", Name: "Staging"}}, + }, + } + + handler := &Handler{auth: mockAuth, config: store} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer operator-token"}, + } + _, err := handler.approvePurchase(ctx, req, executionID, "") + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + mockStore.AssertNotCalled(t, "GetGlobalConfig") +} + +// TestHandler_retryPurchase_OutOfScope is the issue #92 regression guard for +// the retry endpoint: retryPurchase relied only on the retry-any/retry-own +// RBAC matrix and never consulted allowed_accounts, so a scoped operator +// holding retry-any could relaunch a failed purchase belonging to an account +// outside their scope. A session scoped to allowed_accounts=["Production"] +// holding retry-any:purchases must get the same 404 requireExecutionAccess +// already returns for pause/resume/delete-planned when acting on an +// execution whose plan is scoped to a different account ("Staging"). +func TestHandler_retryPurchase_OutOfScope(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + executionID := "cccccccc-1111-1111-1111-111111111111" + planID := "dddddddd-1111-1111-1111-111111111111" + + mockAuth.On("ValidateSession", ctx, "operator-token").Return(&Session{ + UserID: "operator-1", Email: "operator@example.com", + }, nil) + // retry-any:purchases -- the RBAC matrix that pre-fix was the ONLY gate + // on this endpoint. + mockAuth.On("HasPermissionAPI", ctx, "operator-1", "retry-any", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "operator-1").Return([]string{"Production"}, nil) + + mockStore.On("GetExecutionByID", ctx, executionID).Return(&config.PurchaseExecution{ + ExecutionID: executionID, PlanID: planID, Status: "failed", + }, nil) + + store := &mockStoreWithPlanAccounts{ + MockConfigStore: mockStore, + planAccounts: map[string][]config.CloudAccount{ + planID: {{ID: "acc-stage", Name: "Staging"}}, + }, + } + + handler := &Handler{auth: mockAuth, config: store} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer operator-token"}, + } + _, err := handler.retryPurchase(ctx, req, executionID) + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + mockStore.AssertNotCalled(t, "SavePurchaseExecution") +} + // ─── Session-authed Cancel (issue #46) ───────────────────────────────────── // // Covers the full cancel-any / cancel-own RBAC matrix on the @@ -2936,6 +3051,10 @@ func buildSessionCancelHandler(exec *config.PurchaseExecution, session *Session, if session != nil { mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-any", "purchases").Return(hasAny, nil).Maybe() mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-own", "purchases").Return(hasOwn, nil).Maybe() + // cancelPurchaseViaSession now consults requireExecutionAccess (issue + // #92); an empty allowed_accounts list resolves to unrestricted, which + // is what these RBAC-matrix tests intend to exercise. + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, session.UserID).Return([]string{}, nil).Maybe() } return &Handler{config: mockConfig, auth: mockAuth}, mockConfig, mockAuth @@ -3400,6 +3519,8 @@ func TestHandler_cancelPurchase_DeepLink_TransientAuthErrorPropagates(t *testing mockConfig.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, nil) // Simulate a transient auth-backend failure on the cancel-any check. // authorizeSessionCancel wraps this as "permission check failed: …" @@ -3490,6 +3611,10 @@ func buildSessionRetryHandler(failed *config.PurchaseExecution, session *Session if session != nil { mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-any", "purchases").Return(hasAny, nil).Maybe() mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-own", "purchases").Return(hasOwn, nil).Maybe() + // retryPurchase now consults requireExecutionAccess (issue #92); an + // empty allowed_accounts list resolves to unrestricted, which is what + // these RBAC-matrix tests intend to exercise. + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, session.UserID).Return([]string{}, nil).Maybe() } // SEC-01 constraint check (adversarial review follow-up to #1210): the // retry path now re-evaluates the retrying session's execute:purchases @@ -3976,6 +4101,9 @@ func TestHandler_retryPurchase_PermissionConstraintsDenied(t *testing.T) { // Caller owns the row; retry-own authorizes it (issue #907). mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-own", "purchases").Return(true, nil) + // requireExecutionAccess (issue #92) resolves the session's scope before + // the constraints check; unrestricted here since scope is not under test. + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, session.UserID).Return([]string{}, nil) // The retrying session's execute:purchases permission is capped below // the failed batch's $5,000 total commitment. mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, session.UserID, "execute", "purchases", @@ -5408,6 +5536,8 @@ func TestHandler_revokePurchase_ValidToken(t *testing.T) { mockStore.On("SetCancelledBy", ctx, execID, revokerEmail).Return(nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Provide a session for the revoker so authorizeApprovalAction resolves actor. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: revokerEmail}, nil) // RBAC: revoker has no cancel-any or cancel-own, so falls through to token path. @@ -5453,6 +5583,8 @@ func TestHandler_revokePurchase_InvalidToken(t *testing.T) { } mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -5470,8 +5602,10 @@ func TestHandler_revokePurchase_InvalidToken(t *testing.T) { assert.Equal(t, 403, ce.code) } -// TestHandler_revokePurchase_PendingExecution verifies that a pending execution -// returns 409 with a friendly message directing the user to Cancel instead. +// TestHandler_revokePurchase_PendingExecution verifies that a caller with no +// session gets the 401 for a pending execution, not the status 409: status is +// only disclosed once the caller is authorized. The authorized 409 is covered by +// TestRevoke_AuthorizedNonRevocableStatusIs409. func TestHandler_revokePurchase_PendingExecution(t *testing.T) { ctx := context.Background() execID := "33333333-3333-3333-3333-333333333333" @@ -5493,8 +5627,7 @@ func TestHandler_revokePurchase_PendingExecution(t *testing.T) { require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok, "expected a client error") - assert.Equal(t, 409, ce.code) - assert.Contains(t, ce.message, "Cancel") + assert.Equal(t, 401, ce.code) } // TestHandler_revokePurchase_NotFound verifies 404 when the execution does not exist. @@ -5544,6 +5677,8 @@ func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { } mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -5587,6 +5722,9 @@ func TestHandler_revokePurchase_SessionAdminCancelAny(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token"). Return(&Session{UserID: adminUserID, Email: adminEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, adminUserID, "cancel-any", "purchases").Return(true, nil) + // requireExecutionAccess (issue #92) runs before RBAC; unrestricted + // here since scope is not under test. + mockAuth.On("GetAllowedAccountsAPI", ctx, adminUserID).Return([]string{}, nil) // CSRF is enforced for the session-authed revoke path (tryRevokeViaSession). // No CSRF token header in this request, so csrfToken is "". mockAuth.On("ValidateCSRFToken", ctx, "admin-token", "").Return(nil) @@ -5630,6 +5768,9 @@ func TestHandler_revokePurchase_SessionOwnerCancelOwn(t *testing.T) { Return(&Session{UserID: ownerUserID, Email: ownerEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-own", "purchases").Return(true, nil) + // requireExecutionAccess (issue #92) runs before RBAC; unrestricted + // here since scope is not under test. + mockAuth.On("GetAllowedAccountsAPI", ctx, ownerUserID).Return([]string{}, nil) // CSRF is enforced for the session-authed revoke path (tryRevokeViaSession). mockAuth.On("ValidateCSRFToken", ctx, "owner-token", "").Return(nil) @@ -5661,6 +5802,8 @@ func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "user-token"). Return(&Session{UserID: userID, Email: userEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) @@ -6028,6 +6171,8 @@ func TestRevokePurchase_POSTPerformsRevoke(t *testing.T) { mockStore.On("SetCancelledBy", ctx, execID, revokerEmail).Return(nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: revokerEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -6218,6 +6363,7 @@ func TestRequireDifferentApprover_EmailTokenPath_ModeOn(t *testing.T) { // Session identifies the user as the CREATOR via UserID. creatorSession := &Session{UserID: creatorID, Email: contactEmail} mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Session exists but lacks approve-* permission, so the dispatch falls // through to the token branch. The 4-eyes check then uses the session // identity against CreatedByUserID and must deny. diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 340304de5..c01779b87 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -864,6 +864,8 @@ func TestHandler_HandleRequest_ApprovePurchase(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil) // Issue #286: dispatch consults approve-{any,own} BEFORE the // contact_email gate. Returning false for both verbs lets the @@ -924,6 +926,8 @@ func TestHandler_HandleRequest_CancelPurchase(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil) // Session has no admin role / cancel permissions → cancelPurchase's // session-authed pre-check falls through to authorizeApprovalAction → diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 8a319ccf4..9988e8a84 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -662,6 +662,7 @@ func TestTokenOnlyApprove_BypassesCSRF(t *testing.T) { // falls through to the token branch. CSRF is NOT called on the token // branch (approvePurchaseViaSession is never reached). mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 6ab6c671b..8d16cf25a 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) @@ -143,11 +144,11 @@ func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, sessi return nil } -// requireExecutionAccess rejects with errNotFound when the execution's plan's -// associated accounts don't intersect with the session's allowed_accounts. -// Convenience wrapper around requirePlanAccess for the pause/resume/run/ -// delete/details handlers that key on executionID. Returns errNotFound when -// the execution itself doesn't exist, so unauthenticated probing can't +// requireExecutionAccess rejects with errNotFound unless every cloud account +// the execution can buy in (executionAccounts) is inside the session's +// allowed_accounts (issue #92). Used by the approve/cancel/retry/revoke and +// pause/resume/run/delete handlers that key on executionID. Returns +// errNotFound when the execution itself doesn't exist, so probing can't // distinguish "no such execution" from "you can't see it". // // Short-circuits for admin / unrestricted sessions BEFORE the @@ -155,21 +156,116 @@ func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, sessi // round-trip (and to keep existing unit-test fixtures for admin operations // working without adding execution mocks). func (h *Handler) requireExecutionAccess(ctx context.Context, session *Session, executionID string) error { - allowed, err := h.getAccountScope(ctx, session) + scope, err := h.getAccountScope(ctx, session) if err != nil { return fmt.Errorf("failed to get allowed accounts: %w", err) } - if allowed.AllowsAll() { + if scope.AllowsAll() { return nil } execution, err := h.config.GetExecutionByID(ctx, executionID) - if errors.Is(err, config.ErrNotFound) { + if errors.Is(err, config.ErrNotFound) || (err == nil && execution == nil) { return errNotFound } if err != nil { return fmt.Errorf("failed to get execution: %w", err) } - return h.requirePlanAccess(ctx, session, execution.PlanID) + return h.requireExecutionInScope(ctx, scope, execution, nil) +} + +// requireExecutionInScope returns errNotFound unless the scope allows every +// account in executionAccounts(execution). An empty set is a deny: an +// execution that cannot be attributed must not be actionable by a scoped +// user. planAccounts memoises GetPlanAccounts for list callers; nil disables +// it. +func (h *Handler) requireExecutionInScope(ctx context.Context, scope auth.AccountScope, execution *config.PurchaseExecution, planAccounts map[string][]config.CloudAccount) error { + if scope.AllowsAll() { + return nil + } + accounts, err := h.executionAccounts(ctx, execution, planAccounts) + if err != nil { + return err + } + if len(accounts) == 0 { + return errNotFound + } + for i := range accounts { + if !scope.Allows(accounts[i].ID, accounts[i].Name) { + return errNotFound + } + } + return nil +} + +// executionAccounts returns every cloud account the execution can buy in: +// - CloudAccountID set: that account only (a fan-out child or a +// single-account execution). +// - otherwise PlanID set: all of the plan's accounts, since a plan-level +// parent fans out to every one of them (purchase.Manager.executeScopeAware). +// - otherwise (an ad-hoc web purchase, plan_id NULL): the account of every +// recommendation. One without an account is unattributable, so the whole +// execution is refused with errNotFound. +// +// An account ID with no cloud_accounts row comes back without a name, so it +// can only match an ID entry in the scope. +func (h *Handler) executionAccounts(ctx context.Context, execution *config.PurchaseExecution, planAccounts map[string][]config.CloudAccount) ([]config.CloudAccount, error) { + if execution.CloudAccountID != nil { + a, err := h.lookupCloudAccount(ctx, *execution.CloudAccountID) + if err != nil { + return nil, err + } + return []config.CloudAccount{a}, nil + } + if execution.PlanID != "" { + if cached, ok := planAccounts[execution.PlanID]; ok { + return cached, nil + } + accounts, err := h.config.GetPlanAccounts(ctx, execution.PlanID) + if err != nil { + return nil, fmt.Errorf("failed to get plan accounts: %w", err) + } + if planAccounts != nil { + planAccounts[execution.PlanID] = accounts + } + return accounts, nil + } + return h.recommendationAccounts(ctx, execution.Recommendations) +} + +// recommendationAccounts returns the distinct accounts of recs, or +// errNotFound when any recommendation has no account. +func (h *Handler) recommendationAccounts(ctx context.Context, recs []config.RecommendationRecord) ([]config.CloudAccount, error) { + seen := make(map[string]bool, len(recs)) + accounts := make([]config.CloudAccount, 0, len(recs)) + for i := range recs { + id := recs[i].CloudAccountID + if id == nil || *id == "" { + return nil, errNotFound + } + if seen[*id] { + continue + } + seen[*id] = true + a, err := h.lookupCloudAccount(ctx, *id) + if err != nil { + return nil, err + } + accounts = append(accounts, a) + } + return accounts, nil +} + +// lookupCloudAccount resolves an account ID for a scope check; a missing row +// yields an ID-only account rather than an error (see executionAccounts). +func (h *Handler) lookupCloudAccount(ctx context.Context, id string) (config.CloudAccount, error) { + a, err := h.config.GetCloudAccount(ctx, id) + if err != nil { + return config.CloudAccount{}, fmt.Errorf("accounts: %w", err) + } + if a == nil { + return config.CloudAccount{ID: id}, nil + } + return *a, nil } // resolveAccountFilterIDs maps requested cloud_accounts UUIDs to the