fix(ri-exchange): require an identified approver for creator-less exchanges under 4-eyes

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.

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

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

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"},

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)
+	})
+}

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

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,