From 14f3dd7144df3e497371accead99ff0c8c3cf8f6 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 12:33:51 +0100 Subject: [PATCH] fix(purchase): refuse creator-less purchase creation while four-eyes is on The stateless admin API key has no user UUID, so resolveCreatorUserID stamped NULL onto every pending execution it created. Under four-eyes, requireDifferentApprover and checkDifferentApprover deny a NULL-creator row that is not created_by_system for every approver, so those rows could only expire (and a direct execute was refused after the row was written). Per the issue's option (b), executePurchase (both the approval-email and direct-execute paths) and createPlannedPurchases now refuse such a request with a 403 while four-eyes is on, before anything is written. The retry successor gets the same guard as defense in depth. Sessions with a user UUID, including per-user API keys, are unaffected and skip the config read. A config-load failure denies (fail closed). No approval check is relaxed. Closes #677 Co-Authored-By: Claude Opus 5.5 --- .../four_eyes_unattributed_creator_test.go | 212 ++++++++++++++++++ internal/api/handler_plans.go | 5 +- internal/api/handler_purchases.go | 58 ++++- internal/api/openapi.yaml | 23 +- 4 files changed, 294 insertions(+), 4 deletions(-) create mode 100644 internal/api/four_eyes_unattributed_creator_test.go diff --git a/internal/api/four_eyes_unattributed_creator_test.go b/internal/api/four_eyes_unattributed_creator_test.go new file mode 100644 index 000000000..f1ac4ab94 --- /dev/null +++ b/internal/api/four_eyes_unattributed_creator_test.go @@ -0,0 +1,212 @@ +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/config" +) + +// Issue #677: the stateless admin API key has no user UUID, so every pending +// purchase execution it minted was stored with created_by_user_id NULL and +// created_by_system false. Under 4-eyes, requireDifferentApprover and the +// manager's checkDifferentApprover deny that row for every approver, so it +// could only expire. The fix refuses the creating request while 4-eyes is on. +// These tests drive the three API writers end to end with the admin key. + +const fourEyesAdminKey = "test-key" + +func adminKeyRequest(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": fourEyesAdminKey}, + Body: body, + } +} + +// errPersistReached is returned by a registered WithTx expectation in the +// mode-off controls: getting it back proves the request passed the new guard +// and reached the write. +var errPersistReached = NewClientError(418, "test sentinel: persistence reached") + +func requireFourEyesUnattributedRefusal(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "no attributable user") +} + +func TestExecutePurchase_AdminAPIKey_FourEyesOn_RefusedBeforeAnyWrite(t *testing.T) { + for _, body := range map[string]string{ + "approval email": `{"recommendations":[{"id":"rec-1","provider":"aws","service":"ec2","count":1,"term":1,"payment":"all-upfront","upfront_cost":500.0,"savings":100.0}]}`, + "direct execute": directExecRecBody, + } { + ctx := context.Background() + store := new(MockConfigStore) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + expectStoredRecs(store, config.RecommendationRecord{ + ID: "rec-1", Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 100, + }) + handler := &Handler{config: store, apiKey: fourEyesAdminKey} + + _, err := handler.executePurchase(ctx, adminKeyRequest(body)) + + requireFourEyesUnattributedRefusal(t, err) + store.AssertNotCalled(t, "WithTx") + store.AssertNotCalled(t, "SavePurchaseExecutionTx") + store.AssertNotCalled(t, "SavePurchaseExecution") + } +} + +func TestExecutePurchase_AdminAPIKey_FourEyesOff_ReachesPersistence(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOff(), nil) + expectStoredRecs(store, config.RecommendationRecord{ + ID: "rec-1", Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 100, + }) + store.On("WithTx", ctx, mock.Anything).Return(errPersistReached) + handler := &Handler{config: store, apiKey: fourEyesAdminKey} + + _, err := handler.executePurchase(ctx, adminKeyRequest(`{"recommendations":[{"id":"rec-1","provider":"aws","service":"ec2","count":1,"term":1,"payment":"all-upfront","upfront_cost":500.0,"savings":100.0}]}`)) + + require.ErrorIs(t, err, errPersistReached) +} + +func TestRetryPurchase_UnattributedSession_FourEyesOn_RefusedBeforeAnyWrite(t *testing.T) { + creator := retryOtherID + failed := &config.PurchaseExecution{ + ExecutionID: retryExecID, + Status: "failed", + Error: "send failed: transient SES throttle", + CreatedByUserID: &creator, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, + } + ctx := context.Background() + store := new(MockConfigStore) + store.On("GetExecutionByID", ctx, retryExecID).Return(failed, nil) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + handler := &Handler{config: store, auth: unattributedRetryAuth(t)} + + _, err := handler.retryPurchase(ctx, sessionRetryReq(), retryExecID) + + requireFourEyesUnattributedRefusal(t, err) + store.AssertNotCalled(t, "WithTx") + store.AssertNotCalled(t, "SavePurchaseExecutionTx") + store.AssertNotCalled(t, "LinkRetryExecutionAtomic") +} + +func TestRetryPurchase_UnattributedSession_FourEyesOff_ReachesPersistence(t *testing.T) { + creator := retryOtherID + failed := &config.PurchaseExecution{ + ExecutionID: retryExecID, + Status: "failed", + Error: "send failed: transient SES throttle", + CreatedByUserID: &creator, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, + } + ctx := context.Background() + store := new(MockConfigStore) + store.On("GetExecutionByID", ctx, retryExecID).Return(failed, nil) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOff(), nil) + store.On("WithTx", ctx, mock.Anything).Return(errPersistReached) + handler := &Handler{config: store, auth: unattributedRetryAuth(t)} + + _, err := handler.retryPurchase(ctx, sessionRetryReq(), retryExecID) + + require.ErrorIs(t, err, errPersistReached) +} + +// unattributedRetryAuth resolves the retry request's bearer token to a session +// whose UserID is not a user UUID. The retry route needs a bearer session, so +// the admin API key cannot reach it today (it gets a 401); this covers any +// session type that would still stamp a NULL creator onto the successor. +func unattributedRetryAuth(t *testing.T) *MockAuthService { + t.Helper() + session := &Session{UserID: "service-principal"} + auth := new(MockAuthService) + auth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, nil) + auth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-any", "purchases").Return(true, nil).Maybe() + auth.On("GetAllowedAccountsAPI", mock.Anything, session.UserID).Return([]string{}, nil).Maybe() + auth.allowConstraintChecks() + return auth +} + +const fourEyesPlanID = "11111111-1111-1111-1111-111111111111" + +func TestCreatePlannedPurchases_AdminAPIKey_FourEyesOn_RefusedBeforeAnyWrite(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + handler := &Handler{config: store, apiKey: fourEyesAdminKey} + + _, err := handler.createPlannedPurchases(ctx, adminKeyRequest(`{"count": 1, "start_date": "2030-01-01"}`), fourEyesPlanID) + + requireFourEyesUnattributedRefusal(t, err) + store.AssertNotCalled(t, "WithTx") + store.AssertNotCalled(t, "SavePurchaseExecutionTx") +} + +func TestCreatePlannedPurchases_AdminAPIKey_FourEyesOff_ReachesPersistence(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOff(), nil) + store.On("GetPurchasePlan", ctx, fourEyesPlanID).Return(&config.PurchasePlan{ID: fourEyesPlanID, Enabled: true}, nil) + store.On("WithTx", ctx, mock.Anything).Return(errPersistReached) + handler := &Handler{config: store, apiKey: fourEyesAdminKey} + + _, err := handler.createPlannedPurchases(ctx, adminKeyRequest(`{"count": 1, "start_date": "2030-01-01"}`), fourEyesPlanID) + + require.ErrorIs(t, err, errPersistReached) +} + +// The guard itself, for the cases the endpoint tests above do not reach. +func TestRequireAttributablePurchaseCreator(t *testing.T) { + ctx := context.Background() + adminKey := &Session{UserID: apiKeyAdminUserID} + + t.Run("user session passes without reading config", func(t *testing.T) { + store := new(MockConfigStore) + h := &Handler{config: store} + require.NoError(t, h.requireAttributablePurchaseCreator(ctx, &Session{UserID: retryCallerID, Email: "u@example.com"})) + require.NoError(t, h.requireAttributablePurchaseCreator(ctx, &Session{UserID: retryCallerID, UserAPIKeyID: "key-1"})) + store.AssertNotCalled(t, "GetGlobalConfig") + }) + + t.Run("config load failure fails closed", func(t *testing.T) { + store := new(MockConfigStore) + store.On("GetGlobalConfig", ctx).Return(nil, errors.New("db down")) + h := &Handler{config: store} + err := h.requireAttributablePurchaseCreator(ctx, adminKey) + require.Error(t, err) + assert.Contains(t, err.Error(), "4-eyes policy check") + }) + + t.Run("nil config means 4-eyes off", func(t *testing.T) { + store := new(MockConfigStore) + store.On("GetGlobalConfig", ctx).Return(nil, nil) + h := &Handler{config: store} + require.NoError(t, h.requireAttributablePurchaseCreator(ctx, adminKey)) + }) + + t.Run("nil session under 4-eyes is refused", func(t *testing.T) { + store := new(MockConfigStore) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + h := &Handler{config: store} + requireFourEyesUnattributedRefusal(t, h.requireAttributablePurchaseCreator(ctx, nil)) + }) + + t.Run("non-UUID user id under 4-eyes is refused", func(t *testing.T) { + store := new(MockConfigStore) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + h := &Handler{config: store} + requireFourEyesUnattributedRefusal(t, h.requireAttributablePurchaseCreator(ctx, &Session{UserID: "not-a-uuid"})) + }) +} diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 5d877873b..5a6598d19 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -457,7 +457,10 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La // hold this verb; Plan Authors (create/update/delete:plans only) // do not, closing issue #1406 / #1418. The prior gate ("create","plans") // incorrectly admitted Plan Authors because they hold create:plans. - session, err := h.requirePermission(ctx, httpReq, "update", "purchases") + // Scheduled rows are stamped with resolveCreatorUserID(session); a + // creator-less row could never be approved under 4-eyes, so that case + // is refused here (issue #677). + session, err := h.requirePurchaseCreatingPermission(ctx, httpReq, "update", "purchases") if err != nil { return nil, err } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index af2c99a92..f5e96f8b7 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2372,6 +2372,14 @@ func (h *Handler) persistRetryExecution(ctx context.Context, failedExec *config. // "successor" is a footgun we want to remove at the source. copiedRecs := append([]config.RecommendationRecord(nil), failedExec.Recommendations...) + // The successor is stamped with the retrying session's creator below. A + // creator-less successor under 4-eyes could never be approved, and the + // linkage written in the same tx would also block retrying the original + // again, so refuse before anything is written (issue #677). + if err := h.requireAttributablePurchaseCreator(ctx, session); err != nil { + return nil, "", err + } + // Generate a crypto/rand-backed approval token for the retry execution // (issue #408). uuid.New().String() was used here previously, which only // provides 122 bits of entropy in a known format; common.GenerateApprovalToken @@ -2791,7 +2799,9 @@ type ExecutePurchaseRequest struct { // the per-rec validation, for the response to surface to the caller (#1503 // follow-up); it is empty/nil when every rec was already canonical. func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *events.LambdaFunctionURLRequest) (ExecutePurchaseRequest, *Session, []PaymentAdjustment, error) { - session, err := h.requirePermission(ctx, req, "execute", "purchases") + // Refuses a creator-less submit under 4-eyes before anything is + // persisted, on both the approval-email and direct-execute paths (#677). + session, err := h.requirePurchaseCreatingPermission(ctx, req, "execute", "purchases") if err != nil { return ExecutePurchaseRequest{}, nil, nil, err } @@ -3073,6 +3083,52 @@ func resolveCreatorUserID(session *Session) *string { return &uid } +// requireAttributablePurchaseCreator refuses, while 4-eyes mode is on, any +// request that would mint a pending purchase execution with no recorded +// creator (issue #677). resolveCreatorUserID stores NULL for a session whose +// UserID is not a user UUID, which in practice is the stateless admin API +// key. Under 4-eyes, requireDifferentApprover and the manager's +// checkDifferentApprover deny a NULL-creator row that is not +// created_by_system for every approver, so such a row could only sit pending +// until it expired (and, on the direct-execute path, be refused after it was +// already written). Refusing up front gives the caller a clear error instead. +// The key is never stamped with a human creator on its behalf: that would let +// that human's colleague "approve" a purchase no human requested, or let the +// key holder approve their own request. +// +// Sessions with a user UUID (signed-in users and per-user API keys) pass +// without a config read. A config-load failure denies (fail closed), matching +// requireDifferentApprover. A nil config means 4-eyes is off, also matching it. +func (h *Handler) requireAttributablePurchaseCreator(ctx context.Context, session *Session) error { + if resolveCreatorUserID(session) != nil { + return nil + } + cfg, err := h.config.GetGlobalConfig(ctx) + if err != nil { + return fmt.Errorf("4-eyes policy check: failed to load global config: %w", err) + } + if cfg == nil || !cfg.RequireDifferentApprover { + return nil + } + logging.Warnf("4-eyes: mode on; refusing to create a purchase execution with no attributable creator") + return NewClientError(403, "4-eyes approval mode is enabled and this request has no attributable user (for example the admin API key), so the purchase it creates could never be approved; sign in or use a per-user API key instead") +} + +// requirePurchaseCreatingPermission is requirePermission for endpoints that +// mint pending purchase executions: after the RBAC check it applies +// requireAttributablePurchaseCreator so no NULL-creator row is created while +// 4-eyes mode is on (issue #677). +func (h *Handler) requirePurchaseCreatingPermission(ctx context.Context, req *events.LambdaFunctionURLRequest, action, resource string) (*Session, error) { + session, err := h.requirePermission(ctx, req, action, resource) + if err != nil { + return nil, err + } + if err := h.requireAttributablePurchaseCreator(ctx, session); err != nil { + return nil, err + } + return session, nil +} + // fourEyesActorIdentity returns the identity string passed as the actor to // purchase.Manager.ApproveAndExecute for the 4-eyes dual-control comparison // (issue #1005 / PR #1500 adversarial review). Real human sessions always diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 5ad33ce0a..e32dede78 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -481,7 +481,16 @@ paths: '401': $ref: '#/components/responses/Unauthorized' '403': - $ref: '#/components/responses/Forbidden' + description: > + CSRF validation failed, the caller lacks permission or access to + the plan, or 4-eyes mode is on and the caller has no attributable + user (the admin API key). Rows scheduled that way could never be + approved under 4-eyes, so the request is refused; sign in or use a + per-user API key instead. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' '404': $ref: '#/components/responses/NotFound' '409': @@ -522,7 +531,17 @@ paths: '401': $ref: '#/components/responses/Unauthorized' '403': - $ref: '#/components/responses/Forbidden' + description: > + CSRF validation failed, the caller lacks permission or exceeds its + Constraints, 4-eyes mode rejects a direct execute, or 4-eyes mode is + on and the caller has no attributable user (the admin API key). An + execution created that way could never be approved under 4-eyes, + so it is refused before anything is written; sign in or use a + per-user API key instead. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' '409': description: > One of two conflict families. Idempotency: an identical submit