From 42b852e6aad0a35da3ca4f0d415d6111815686a1 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 16:00:13 +0100 Subject: [PATCH] fix(ri-exchange): require an identified approver for creator-less exchanges under 4-eyes Every ri_exchange_history row is written by the scheduler or the ladder engine through exchangeToConfigRecord, and pkg/exchange.ExchangeRecord has no creator field, so created_by_user_id is always NULL. The NULL-creator early return in requireDifferentRIExchangeApprover skipped the 4-eyes gate entirely, including its fail-closed "no session" check: with RequireDifferentApprover on, an anonymous email-token POST could approve and execute any production exchange with no recorded actor, contradicting the documented contract in openapi.yaml. A creator-less exchange now needs a resolved session when 4-eyes mode is on (same 500 and message as creator-bearing rows), and a config-load failure fails closed. With a session it stays approvable by any single authorized approver: there is no human requester to differ from, and denying it would make every exchange unapprovable (the purchase-side #233 failure). Mode off is unchanged. Refs #518 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange.go | 20 ++- internal/api/handler_ri_exchange_test.go | 36 ++++++ internal/api/openapi.yaml | 5 +- .../ri_exchange_approve_reject_status_test.go | 5 +- ...hange_four_eyes_system_integration_test.go | 115 ++++++++++++++++++ internal/config/types.go | 5 +- internal/server/handler_ri_exchange.go | 3 + 7 files changed, 182 insertions(+), 7 deletions(-) create mode 100644 internal/api/ri_exchange_four_eyes_system_integration_test.go diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 35e14e422..181190fd7 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2539,12 +2539,26 @@ func (h *Handler) requireNoDirectExchangeUnderFourEyes(ctx context.Context) erro } // requireDifferentRIExchangeApprover applies the purchase 4-eyes check when a creator -// is recorded; automated exchanges have none and stay approvable (see issue #233). +// is recorded. Every exchange row today is written by the scheduler or the ladder +// engine with no creator (pkg/exchange.ExchangeRecord carries none), so there is +// no human requester to differ from: under 4-eyes such a row stays approvable by +// any identified approver, but never by an anonymous token holder (issue #518). func (h *Handler) requireDifferentRIExchangeApprover(ctx context.Context, session *Session, record *config.RIExchangeRecord) error { - if record.CreatedByUserID == nil { + if record.CreatedByUserID != nil { + return h.requireDifferentApprover(ctx, session, record.ID, record.CreatedByUserID) + } + if 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 } - return h.requireDifferentApprover(ctx, session, record.ID, record.CreatedByUserID) + logging.Warnf("4-eyes[%s]: mode on; system-created exchange approved with no session; denying (fail-closed)", record.ID) + return NewClientError(500, "4-eyes approval mode is enabled but no session could be resolved; sign in before approving") } // validateExchangeApproval validates ID, token, and record state for an exchange approval. diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 4c835b300..ed155a530 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -716,6 +716,42 @@ func TestApproveRIExchange_FourEyes(t *testing.T) { _, err := h.approveRIExchange(ctx, csrfSessionReq(), id, "") approved(t, store, err) }) + + // Issue #518: every production exchange has no creator, so the creator-less + // branch is the one that runs. Under 4-eyes it must still demand an + // identified approver instead of letting an anonymous token holder through. + t.Run("automated exchange: token without session fails closed", func(t *testing.T) { + h, store, id, token := fourEyesRIExchangeFixture(t, nil, "approver-uuid", grantRIAdmin, true) + _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, token) + denied(t, store, err, 500) + assert.Contains(t, err.Error(), "no session could be resolved") + store.AssertNotCalled(t, "StampRIExchangeApprovedBy", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("automated exchange: token with a resolved session allowed", func(t *testing.T) { + h, store, id, token := fourEyesRIExchangeFixture(t, nil, "approver-uuid", grantRINone, true) + _, err := h.approveRIExchange(ctx, sessionReq("sess"), id, token) + approved(t, store, err) + }) + + t.Run("automated exchange: mode off, token without session allowed", func(t *testing.T) { + h, store, id, token := fourEyesRIExchangeFixture(t, nil, "approver-uuid", grantRIAdmin, false) + _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, token) + approved(t, store, err) + }) +} + +// TestRequireDifferentRIExchangeApprover_ConfigLoadFailsClosed (issue #518): a +// creator-less exchange approved with no session must not be let through when +// the 4-eyes setting cannot be read. +func TestRequireDifferentRIExchangeApprover_ConfigLoadFailsClosed(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + store.On("GetGlobalConfig", ctx).Return(nil, errors.New("db down")) + h := &Handler{config: store} + err := h.requireDifferentRIExchangeApprover(ctx, nil, &config.RIExchangeRecord{ID: "x"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to load global config") } // TestApproveRIExchange_TokenPathAttributesResolvedApprover (issue #518): a token diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 15a10aeb4..517b21e47 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1364,7 +1364,10 @@ paths: created, within the deployment account scope. Token mode (email link): the approval token in the query string, no session needed unless 4-eyes mode is on, in which case a token request with no - session fails with 500. A session that holds approve permission wins + session fails with 500, including for an exchange with no recorded + creator (every scheduler- or ladder-created exchange); such an + exchange has no requester to differ from, so under 4-eyes any + signed-in approver may approve it. A session that holds approve permission wins over a token in the same request (the token is ignored); a session that lacks the permission, or holds only approve-own for an exchange it did not create, falls through to token mode when a token is diff --git a/internal/api/ri_exchange_approve_reject_status_test.go b/internal/api/ri_exchange_approve_reject_status_test.go index cadd2b1fb..84519eea6 100644 --- a/internal/api/ri_exchange_approve_reject_status_test.go +++ b/internal/api/ri_exchange_approve_reject_status_test.go @@ -33,6 +33,7 @@ const ( // riStatusOpts tweaks the fixture for the less common cases. type riStatusOpts struct { fourEyes bool // global 4-eyes mode on; the admin session is the creator + noCreator bool // with fourEyes: the record has no creator, as every scheduler/ladder row does noApprove bool // the session holds no approve permission approveOwn bool // the session holds approve-own only (not the creator) dbError bool // the record lookup fails @@ -49,7 +50,7 @@ func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { if rec == riRecNoToken { record.ApprovalToken = "" } - if o.fourEyes || o.approveOwn { + if (o.fourEyes && !o.noCreator) || o.approveOwn { creator := "admin-uuid" if o.approveOwn { creator = "someone-else" @@ -182,6 +183,8 @@ func TestHandleRequest_ApproveRIExchange_StatusCodes(t *testing.T) { {name: "403 session without approve right and no token", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{noApprove: true}, want: 403, wantError: "permission denied: requires approve-any or approve-own on purchases"}, {name: "403 4-eyes creator cannot approve own exchange", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{fourEyes: true}, want: 403, wantError: "approval declined: 4-eyes mode requires a different approver than the requester"}, {name: "500 4-eyes on and token without session", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{fourEyes: true}, want: 500, wantError: "4-eyes approval mode is enabled but no session could be resolved; sign in before approving"}, + {name: "500 4-eyes on and token without session on a creator-less exchange", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{fourEyes: true, noCreator: true}, want: 500, wantError: "4-eyes approval mode is enabled but no session could be resolved; sign in before approving"}, + {name: "200 4-eyes on and session approves a creator-less exchange", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{fourEyes: true, noCreator: true}, want: 200, wantBody: `"status"`}, {name: "200 approve-own session that is not the creator falls through to the token", rec: riRecPending, sess: riSessCSRF, token: riStatusRawToken, opts: riStatusOpts{approveOwn: true}, want: 200, wantBody: `"status"`}, {name: "403 body token is not read", rec: riRecPending, sess: riSessNone, body: `{"token":"` + riStatusRawToken + `"}`, want: 403, wantError: "CSRF validation failed"}, {name: "429 rate limited", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{rateLimited: true}, want: 429, wantError: "too many requests, please try again later"}, diff --git a/internal/api/ri_exchange_four_eyes_system_integration_test.go b/internal/api/ri_exchange_four_eyes_system_integration_test.go new file mode 100644 index 000000000..39f21037b --- /dev/null +++ b/internal/api/ri_exchange_four_eyes_system_integration_test.go @@ -0,0 +1,115 @@ +//go:build integration +// +build integration + +package api + +// Real-Postgres check for issue #518: every production RI exchange row is +// written by the scheduler or the ladder engine through the server package's +// exchangeToConfigRecord, which stores no creator. The 4-eyes gate must read +// that NULL back from the real store and still refuse an approval that carries +// no identified approver, leaving the row pending and unattributed. + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +const systemExchangeRawToken = "system-exchange-raw-token" + +// seedSystemCreatedExchange stores a pending exchange in the shape the scheduler +// writes it: no creator, a raw approval token (hashed by the store). +func seedSystemCreatedExchange(ctx context.Context, t *testing.T, store *config.PostgresStore) string { + t.Helper() + expires := time.Now().Add(24 * time.Hour) + rec := &config.RIExchangeRecord{ + AccountID: "123456789012", + Region: "us-east-1", + SourceRIIDs: []string{"ri-518"}, + SourceInstanceType: "m5.large", + SourceCount: 1, + TargetOfferingID: "offering-518", + TargetInstanceType: "c5.large", + TargetCount: 1, + PaymentDue: "10.00", + Status: "pending", + ApprovalToken: systemExchangeRawToken, + Mode: "manual", + ExpiresAt: &expires, + } + require.NoError(t, store.SaveRIExchangeRecord(ctx, rec)) + got, err := store.GetRIExchangeRecord(ctx, rec.ID) + require.NoError(t, err) + require.Nil(t, got.CreatedByUserID, "fixture must read back with no creator, as scheduler rows do") + return rec.ID +} + +func setupSystemExchangeHandler(ctx context.Context, t *testing.T, fourEyes bool) (*Handler, *config.PostgresStore, *int) { + t.Helper() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { _ = container.Cleanup(context.Background()) }) + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "")) + store := config.NewPostgresStore(container.DB) + _, err = store.UpdateGlobalConfigAtomic(ctx, func(c *config.GlobalConfig) error { + c.RequireDifferentApprover = fourEyes + return nil + }) + require.NoError(t, err) + + executions := 0 + h := &Handler{ + config: store, + // Never reach AWS: count the call and fail it. + executeExchangeFn: func(context.Context, exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { + executions++ + return "", nil, errors.New("fixture executor: no real exchange") + }, + } + return h, store, &executions +} + +func TestApproveRIExchange_Integration_SystemCreatedFourEyes(t *testing.T) { + ctx := context.Background() + + t.Run("4-eyes on: token without session is refused and the row stays pending", func(t *testing.T) { + h, store, executions := setupSystemExchangeHandler(ctx, t, true) + id := seedSystemCreatedExchange(ctx, t, store) + + _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, systemExchangeRawToken) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want ClientError, got %v", err) + assert.Equal(t, 500, ce.code) + assert.Contains(t, err.Error(), "no session could be resolved") + + got, err := store.GetRIExchangeRecord(ctx, id) + require.NoError(t, err) + assert.Equal(t, "pending", got.Status) + assert.Nil(t, got.ApprovedBy) + assert.Zero(t, *executions) + }) + + // Control: the same seed is approvable with 4-eyes off, so the refusal + // above comes from the 4-eyes gate and not from the fixture. + t.Run("4-eyes off: the same token approval leaves pending", func(t *testing.T) { + h, store, _ := setupSystemExchangeHandler(ctx, t, false) + id := seedSystemCreatedExchange(ctx, t, store) + + _, _ = h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, systemExchangeRawToken) + + got, err := store.GetRIExchangeRecord(ctx, id) + require.NoError(t, err) + assert.NotEqual(t, "pending", got.Status) + }) +} diff --git a/internal/config/types.go b/internal/config/types.go index dfc3436b9..66fd45916 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -1055,8 +1055,9 @@ type RIExchangeRecord struct { Mode string `json:"mode"` // CreatedByUserID is the UUID of the session user who submitted the exchange. // Always nil today: every exchange is created by the scheduler or the ladder - // engine, and pkg/exchange.ExchangeRecord carries no creator (see issue #233 - // for the policy on creator-less rows). Exposed to the frontend so the Approve button + // engine, and pkg/exchange.ExchangeRecord carries no creator (the 4-eyes + // policy for creator-less rows is requireDifferentRIExchangeApprover in + // internal/api, issue #518). Exposed to the frontend so the Approve button // can apply the approve-own ownership check client-side. CreatedByUserID *string `json:"created_by_user_id,omitempty"` // ApprovedBy carries the email of the session user who approved the exchange diff --git a/internal/server/handler_ri_exchange.go b/internal/server/handler_ri_exchange.go index d2f7df967..156488d67 100644 --- a/internal/server/handler_ri_exchange.go +++ b/internal/server/handler_ri_exchange.go @@ -345,6 +345,9 @@ func (a *configExchangeStoreAdapter) FailRIExchange(ctx context.Context, id, err return a.store.FailRIExchange(ctx, id, errorMsg) } +// exchangeToConfigRecord leaves CreatedByUserID nil on purpose: these rows are +// written by the scheduler or the ladder engine, which have no human creator, +// and pkg/exchange.ExchangeRecord has no field to carry one (issue #518). func exchangeToConfigRecord(r *exchange.ExchangeRecord) *config.RIExchangeRecord { return &config.RIExchangeRecord{ ID: r.ID,