From 4b18973d6bc8b90451ac8f8e99bfa1ecce2ba877 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 19:23:04 +0200 Subject: [PATCH 1/7] feat(db/config): add require_different_approver to global_config (issue #1005) Migration 000065 adds a BOOLEAN NOT NULL DEFAULT false column to global_config that enables 4-eyes approval mode. GlobalConfig struct + PostgresStore GetGlobalConfig/SaveGlobalConfig wired up; pgxmock tests updated to include the new column in SELECT fixtures. --- internal/config/store_postgres.go | 11 ++++++++--- internal/config/store_postgres_pgxmock_test.go | 5 +++++ internal/config/types.go | 8 ++++++++ .../000092_require_different_approver.down.sql | 1 + .../000092_require_different_approver.up.sql | 9 +++++++++ 5 files changed, 31 insertions(+), 3 deletions(-) create mode 100644 internal/database/postgres/migrations/000092_require_different_approver.down.sql create mode 100644 internal/database/postgres/migrations/000092_require_different_approver.up.sql diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index ea04ad6a9..c72cd7801 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -82,7 +82,8 @@ func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalCo COALESCE(purchase_delay_hours, 0), COALESCE(laddering_enabled, false), COALESCE(ladder_execution_enabled, false), - offering_class + offering_class, + require_different_approver FROM global_config WHERE id = 1 ` @@ -115,6 +116,7 @@ func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalCo &config.LadderingEnabled, &config.LadderExecutionEnabled, &config.OfferingClass, + &config.RequireDifferentApprover, ) if err != nil { @@ -214,8 +216,9 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G auto_collect, collection_schedule, notification_days_before, grace_period_days, recommendations_cache_stale_hours, recommendations_lookback_days, - purchase_delay_hours, laddering_enabled, ladder_execution_enabled, offering_class - ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23) + purchase_delay_hours, laddering_enabled, ladder_execution_enabled, offering_class, + require_different_approver + ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23, $24) ON CONFLICT (id) DO UPDATE SET enabled_providers = $1, notification_email = $2, @@ -240,6 +243,7 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G laddering_enabled = $21, ladder_execution_enabled = $22, offering_class = $23, + require_different_approver = $24, updated_at = NOW() ` @@ -307,6 +311,7 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G config.LadderingEnabled, config.LadderExecutionEnabled, offeringClass, + config.RequireDifferentApprover, ) if err != nil { diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 5953900da..b0fcc3762 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -62,6 +62,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { "laddering_enabled", "ladder_execution_enabled", "offering_class", + "require_different_approver", } rows := pgxmock.NewRows(cols).AddRow( []string{"aws"}, strPtr("ops@example.com"), true, @@ -75,6 +76,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { false, false, "convertible", + false, ) mock.ExpectQuery("SELECT").WillReturnRows(rows) @@ -86,6 +88,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { assert.Equal(t, 24, cfg.RecommendationsCacheStaleHours) assert.Equal(t, 7, cfg.RecommendationsLookbackDays) assert.Equal(t, "convertible", cfg.OfferingClass) + assert.False(t, cfg.RequireDifferentApprover) assert.NoError(t, mock.ExpectationsWereMet()) } @@ -119,6 +122,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { "laddering_enabled", "ladder_execution_enabled", "offering_class", + "require_different_approver", } baseRow := func(graceJSON string) []any { return []any{ @@ -133,6 +137,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { false, false, "convertible", + false, } } diff --git a/internal/config/types.go b/internal/config/types.go index a3f3d1535..4416cd365 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -86,6 +86,14 @@ type GlobalConfig struct { // instance type for the full term but are ~5% cheaper. // Unknown values are rejected at purchase time with an explicit error. OfferingClass string `json:"offering_class,omitempty" dynamodbav:"offering_class,omitempty"` + + // RequireDifferentApprover enables 4-eyes approval mode (issue #1005). + // When true, the user who created a purchase execution cannot approve it + // themselves; a different person with approval rights must do so. This is + // a standard SOX / SOC2 segregation-of-duties control. Default: false. + // Admins who created an execution and need to approve it must disable this + // mode first (the admin wildcard is NOT exempt from the restriction). + RequireDifferentApprover bool `json:"require_different_approver" dynamodbav:"require_different_approver"` } // DefaultGracePeriodDays is the fallback window used when a provider diff --git a/internal/database/postgres/migrations/000092_require_different_approver.down.sql b/internal/database/postgres/migrations/000092_require_different_approver.down.sql new file mode 100644 index 000000000..4e5a6bcd7 --- /dev/null +++ b/internal/database/postgres/migrations/000092_require_different_approver.down.sql @@ -0,0 +1 @@ +ALTER TABLE global_config DROP COLUMN IF EXISTS require_different_approver; diff --git a/internal/database/postgres/migrations/000092_require_different_approver.up.sql b/internal/database/postgres/migrations/000092_require_different_approver.up.sql new file mode 100644 index 000000000..53fc3f1b2 --- /dev/null +++ b/internal/database/postgres/migrations/000092_require_different_approver.up.sql @@ -0,0 +1,9 @@ +-- 000092: add require_different_approver to global_config (issue #1005, 4-eyes approval mode) +-- +-- When enabled, the user who created a purchase execution cannot approve it +-- themselves. A different person with approval rights must do so. This is a +-- standard SOX / SOC2 segregation-of-duties control. +-- +-- Default is false so existing deployments are unaffected on upgrade. +ALTER TABLE global_config + ADD COLUMN IF NOT EXISTS require_different_approver BOOLEAN NOT NULL DEFAULT false; From 692a9fdfc16c671989e69ba0000a0e32e3bcb3b1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 19:32:19 +0200 Subject: [PATCH 2/7] feat(api): add requireDifferentApprover helper and wire into approve handlers (issue #1005) - requireDifferentApprover enforces 4-eyes policy: when RequireDifferentApprover is set, the creator of an execution cannot approve it themselves (admin wildcard NOT exempt). NULL-creator legacy rows fail 403 with an admin-directed message. Nil session with mode on fails 500 (fail-closed). - Wired into approvePurchaseViaSession (session path) and the email-token path in approvePurchase (both paths enforced per the issue design). - 7 new backend tests covering: mode-off allow, mode-on deny same user, mode-on allow different user, admin self-deny, NULL creator deny, nil-auth 500, and email-token path enforcement. - Updated 3 existing tests to mock GetGlobalConfig (new call in the session path). --- internal/api/handler_purchases.go | 65 ++++++++ internal/api/handler_purchases_test.go | 202 ++++++++++++++++++++++++- internal/api/middleware_test.go | 1 + 3 files changed, 264 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index bdc8958bf..1351bd6ce 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -590,6 +590,16 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio if err != nil { return nil, err } + // 4-eyes mode (issue #1005): resolve the session again for identity + // comparison. On the email-token deep-link flow the user is logged in + // (the frontend forces a login before reaching this endpoint), so + // tryGetSession returns their session. Pure-email-client flows have no + // session; requireDifferentApprover returns 500 when mode is on and no + // session is available (fail-closed). + tokenSession := h.tryGetSession(ctx, req) + if err := h.requireDifferentApprover(ctx, tokenSession, execution); err != nil { + return nil, err + } // Check for Gmail-style pre-fire delay (issue #291 wave-2). // Token/email-link path: no authenticated session UUID is available, so the // scheduled transition is recorded as system-initiated (transitioned_by = NULL). @@ -676,6 +686,14 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam 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 { + return nil, err + } + // Human session approval: stamp the session user's UUID onto // transitioned_by (FK-safe via validUUIDPtrOrNil) so the audit trail // records who flipped the row to "approved" (or to "scheduled" on the @@ -905,6 +923,53 @@ func (h *Handler) sendPurchaseScheduledEmail(ctx context.Context, execution *con } } +// requireDifferentApprover enforces the 4-eyes approval policy (issue #1005). +// It is called on every approve path (session-authed and email-token) after +// the standard RBAC gate. Returns nil when the policy allows the approval. +// +// Decision logic: +// - mode off (RequireDifferentApprover == false): always returns nil (default behaviour preserved). +// - session == nil AND mode on: returns 500 (fail-closed; we cannot determine +// identity without a session; the email-token path should always carry a +// session when mode is on because the deep-link flow forces a login). +// - session present AND execution.CreatedByUserID == nil: returns 403 with a +// targeted message explaining that the legacy row predates dual-control and +// an admin must disable 4-eyes mode to proceed. +// - session present AND session.UserID == *execution.CreatedByUserID: returns 403. +// - session present AND session.UserID != *execution.CreatedByUserID: returns nil. +// +// Admin wildcard is intentionally NOT exempt: an admin who created an execution +// must disable the mode in Settings before approving their own row. +func (h *Handler) requireDifferentApprover(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { + 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 + } + + // Mode is on. + if session == nil { + logging.Warnf("purchase[%s]: 4-eyes mode on but no session available; denying (fail-closed)", execution.ExecutionID) + return NewClientError(500, "4-eyes approval mode is enabled but no session could be resolved; sign in before approving") + } + + if execution.CreatedByUserID == nil { + logging.Warnf("purchase[%s]: 4-eyes mode on; NULL creator (legacy row) attempted by user %s, denied", + execution.ExecutionID, session.UserID) + return NewClientError(403, "approval declined: this execution predates the dual-control feature and has no recorded creator; an admin must disable 4-eyes mode to approve") + } + + if session.UserID == *execution.CreatedByUserID { + logging.Warnf("purchase[%s]: 4-eyes mode on; creator %s attempted self-approval, denied", + execution.ExecutionID, session.UserID) + return NewClientError(403, "approval declined: 4-eyes mode requires a different approver than the requester") + } + + return nil +} + // authorizeSessionExecuteDirect returns nil when the session is permitted to // bypass the approval email and execute a purchase immediately under the // execute-any / execute-own RBAC rules added in issue #289. diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 92a1ae274..dde85872f 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -297,9 +297,11 @@ func TestHandler_approvePurchase_SessionApproveAnyChainsToExecute(t *testing.T) }, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) - // approvePurchaseViaSession checks PurchaseDelayHours to decide whether - // to defer the SDK call (Gmail-style pre-fire delay, issue #291 wave-2). - // Delay=0 means immediate execute (the legacy path being tested here). + // approvePurchaseViaSession calls GetGlobalConfig twice: once inside + // requireDifferentApprover (mode off by default, issue #1005) and once to + // check PurchaseDelayHours for the Gmail-style pre-fire delay (issue #291 + // wave-2). Delay=0 means immediate execute (the legacy path being tested + // here); RequireDifferentApprover's zero value is false. mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{PurchaseDelayHours: 0}, nil) mockAuth := new(MockAuthService) @@ -347,7 +349,8 @@ func TestHandler_approvePurchase_SessionExecuteFailureSurfacesAs409(t *testing.T Recommendations: []config.RecommendationRecord{{ID: "r1"}}, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) - // approvePurchaseViaSession checks PurchaseDelayHours (issue #291 wave-2). + // approvePurchaseViaSession checks PurchaseDelayHours (issue #291 wave-2) + // and requireDifferentApprover's mode-off default (issue #1005). mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{PurchaseDelayHours: 0}, nil) mockAuth := new(MockAuthService) @@ -558,6 +561,7 @@ func TestHandler_approvePurchase_AWSOrphanFallsThrough(t *testing.T) { Recommendations: []config.RecommendationRecord{{ID: "r1", Provider: "aws"}}, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) @@ -598,6 +602,7 @@ func TestHandler_approvePurchase_NonOrphanUnchanged(t *testing.T) { CloudAccountID: &accountID, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) @@ -5266,3 +5271,192 @@ func TestRevokePurchase_POSTPerformsRevoke(t *testing.T) { assert.Equal(t, "revocation_requested", resultMap["status"]) mockStore.AssertExpectations(t) } + +// ─── requireDifferentApprover (issue #1005: 4-eyes approval mode) ───────────── + +// fourEyesExec is a minimal execution fixture for the requireDifferentApprover +// tests. creatorID is nil for the legacy-NULL-creator variant. +func fourEyesExec(creatorID *string) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Status: "pending", + CreatedByUserID: creatorID, + } +} + +func fourEyesCfgOff() *config.GlobalConfig { + return &config.GlobalConfig{RequireDifferentApprover: false} +} + +func fourEyesCfgOn() *config.GlobalConfig { + return &config.GlobalConfig{RequireDifferentApprover: true} +} + +// TestRequireDifferentApprover_ModeOff_AllowsSameUser: with mode off (default), +// the creator can self-approve regardless of any other setting. +func TestRequireDifferentApprover_ModeOff_AllowsSameUser(t *testing.T) { + t.Parallel() + ctx := context.Background() + creatorID := "user-1" + session := &Session{UserID: creatorID, Email: "u1@example.com"} + exec := fourEyesExec(&creatorID) + + mockConfig := new(MockConfigStore) + mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOff(), nil) + + handler := &Handler{config: mockConfig} + err := handler.requireDifferentApprover(ctx, session, exec) + require.NoError(t, err, "mode off: creator self-approve must be allowed") +} + +// TestRequireDifferentApprover_ModeOn_DeniesSameUser: with mode on, the creator +// receives a 403 when attempting to approve their own execution even if they +// hold approve-any permission. +func TestRequireDifferentApprover_ModeOn_DeniesSameUser(t *testing.T) { + t.Parallel() + ctx := context.Background() + creatorID := "user-1" + session := &Session{UserID: creatorID, Email: "u1@example.com"} + exec := fourEyesExec(&creatorID) + + mockConfig := new(MockConfigStore) + mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + + handler := &Handler{config: mockConfig} + err := handler.requireDifferentApprover(ctx, session, exec) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "4-eyes mode") +} + +// TestRequireDifferentApprover_ModeOn_AllowsDifferentUser: with mode on, a +// different user with approve-own or approve-any must be allowed through. +func TestRequireDifferentApprover_ModeOn_AllowsDifferentUser(t *testing.T) { + t.Parallel() + ctx := context.Background() + creatorID := "user-creator" + approverID := "user-approver" + session := &Session{UserID: approverID, Email: "approver@example.com"} + exec := fourEyesExec(&creatorID) + + mockConfig := new(MockConfigStore) + mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + + handler := &Handler{config: mockConfig} + err := handler.requireDifferentApprover(ctx, session, exec) + require.NoError(t, err, "mode on: different user must be allowed") +} + +// TestRequireDifferentApprover_ModeOn_DeniesAdminSelfApprove: even an admin who +// created the row cannot self-approve under 4-eyes mode. The admin wildcard in +// authorizeSessionApprove short-circuits RBAC but NOT this check. +func TestRequireDifferentApprover_ModeOn_DeniesAdminSelfApprove(t *testing.T) { + t.Parallel() + ctx := context.Background() + adminID := "admin-uuid" + session := &Session{UserID: adminID, Email: "admin@example.com"} + exec := fourEyesExec(&adminID) + + mockConfig := new(MockConfigStore) + mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + + handler := &Handler{config: mockConfig} + err := handler.requireDifferentApprover(ctx, session, exec) + require.Error(t, err, "mode on: admin self-approve must be denied") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "4-eyes mode") +} + +// TestRequireDifferentApprover_ModeOn_NullCreatorDenied: a legacy row with a +// NULL creator cannot satisfy the "different from creator" predicate. Must +// return 403 with a message directing the admin to disable the mode. +func TestRequireDifferentApprover_ModeOn_NullCreatorDenied(t *testing.T) { + t.Parallel() + ctx := context.Background() + session := &Session{UserID: "any-user", Email: "approver@example.com"} + exec := fourEyesExec(nil) // NULL creator + + mockConfig := new(MockConfigStore) + mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + + handler := &Handler{config: mockConfig} + err := handler.requireDifferentApprover(ctx, session, exec) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "predates the dual-control feature") +} + +// TestRequireDifferentApprover_NilAuth_500: a nil session with mode on returns +// a 500 (fail-closed). The session-authed path always has a session; nil +// indicates an unexpected internal state. +func TestRequireDifferentApprover_NilAuth_500(t *testing.T) { + t.Parallel() + ctx := context.Background() + exec := fourEyesExec(strptr("some-creator")) + + mockConfig := new(MockConfigStore) + mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + + handler := &Handler{config: mockConfig} + err := handler.requireDifferentApprover(ctx, nil, exec) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 500, ce.code) +} + +// TestRequireDifferentApprover_EmailTokenPath_ModeOn: the email-token approve +// route also enforces 4-eyes mode when the approver is identified via session. +// Simulated by calling approvePurchase with a token AND a session that +// identifies as the creator; mode on must produce a 403. +func TestRequireDifferentApprover_EmailTokenPath_ModeOn(t *testing.T) { + ctx := context.Background() + creatorID := "creator-uuid" + execID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + contactEmail := "approver@example.com" + accountID := "acct-1" + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "email-tok", + Status: "pending", + CreatedByUserID: &creatorID, + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + mockConfig.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil + } + + // Session identifies the user as the CREATOR via UserID. + creatorSession := &Session{UserID: creatorID, Email: contactEmail} + mockAuth := new(MockAuthService) + // 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. + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(creatorSession, nil) + mockAuth.On("HasPermissionAPI", ctx, creatorID, "approve-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, creatorID, "approve-own", "purchases").Return(false, nil).Maybe() + + handler := &Handler{config: mockConfig, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + _, err := handler.approvePurchase(ctx, req, execID, "email-tok") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "4-eyes mode") +} diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index f83a598d7..a54a84019 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -354,6 +354,7 @@ func TestApproveViaSession_PassesCSRF(t *testing.T) { Recommendations: []config.RecommendationRecord{{ID: "r1"}}, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockAuth := new(MockAuthService) adminSession := &Session{Email: adminEmail} From 852a18ce14c33b8704ca16af17288c91fed80985 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 23 Jul 2026 01:19:56 +0200 Subject: [PATCH 3/7] test(config/api): update pgxmock fixtures + assertions for require_different_approver Rebasing the 4-eyes commits (issue #1005) onto current main required updating tests that hardcoded the SaveGlobalConfig arg count (23 -> 24) and the GetGlobalConfig column list, plus two fail-closed assertions that now catch the config-read error inside requireDifferentApprover before the purchase-delay check reaches it. --- internal/api/handler_purchases_test.go | 10 ++++++++-- internal/config/store_postgres_coverage_test.go | 8 +++++--- internal/config/store_postgres_pgxmock_test.go | 5 ++++- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index dde85872f..6dcd523f7 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -416,7 +416,10 @@ func TestHandler_approveViaToken_GlobalConfigError_FailsClosed(t *testing.T) { _, err := handler.approvePurchase(ctx, req, execID, "valid-token") require.Error(t, err, "config error must propagate; must not execute immediately (F3 token path)") - assert.Contains(t, err.Error(), "failed to read global config") + // requireDifferentApprover (issue #1005) now runs before the purchase-delay + // config read, so the failure surfaces there first; assert on the + // underlying cause rather than the specific wrapping call site. + assert.Contains(t, err.Error(), "db transient error") mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockPurchase.AssertNotCalled(t, "ApproveAndExecute", @@ -459,7 +462,10 @@ func TestHandler_approvePurchaseViaSession_GlobalConfigError_FailsClosed(t *test _, err := handler.approvePurchase(ctx, req, execID, "") require.Error(t, err, "config error must propagate; must not execute immediately (F3 session path)") - assert.Contains(t, err.Error(), "failed to read global config") + // requireDifferentApprover (issue #1005) now runs before the purchase-delay + // config read, so the failure surfaces there first; assert on the + // underlying cause rather than the specific wrapping call site. + assert.Contains(t, err.Error(), "db transient error") mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } diff --git a/internal/config/store_postgres_coverage_test.go b/internal/config/store_postgres_coverage_test.go index cf152869f..2be18fdd0 100644 --- a/internal/config/store_postgres_coverage_test.go +++ b/internal/config/store_postgres_coverage_test.go @@ -570,9 +570,10 @@ func TestSaveGlobalConfig_OfferingClassBindsAt23(t *testing.T) { OfferingClass: "standard", } - // Expect exactly 23 args; pgxmock validates arg count and types. + // Expect exactly 24 args; pgxmock validates arg count and types. // The 21st arg is laddering_enabled; the 22nd is ladder_execution_enabled; - // the 23rd arg must be "standard" (offering_class). + // the 23rd arg must be "standard" (offering_class); the 24th is + // require_different_approver (issue #1005). // If the real query regresses to a different arg count, pgxmock // will return an unexpected-call error and the test will fail. mock.ExpectExec(`INSERT INTO global_config`). @@ -600,11 +601,12 @@ func TestSaveGlobalConfig_OfferingClassBindsAt23(t *testing.T) { pgxmock.AnyArg(), // $21 laddering_enabled pgxmock.AnyArg(), // $22 ladder_execution_enabled "standard", // $23 offering_class -- the field this test guards + pgxmock.AnyArg(), // $24 require_different_approver ). WillReturnResult(pgxmock.NewResult("INSERT", 1)) err = store.SaveGlobalConfig(ctx, cfg) - require.NoError(t, err, "SaveGlobalConfig must succeed when the DB accepts all 23 args") + require.NoError(t, err, "SaveGlobalConfig must succeed when the DB accepts all 24 args") require.NoError(t, mock.ExpectationsWereMet(), "offering_class must be bound as the 23rd argument to SaveGlobalConfig") diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index b0fcc3762..39e7f9e50 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -197,6 +197,7 @@ var globalConfigCols = []string{ "laddering_enabled", "ladder_execution_enabled", "offering_class", + "require_different_approver", } // TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite proves the F2 @@ -227,6 +228,7 @@ func TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite(t *testing.T) { false, // laddering_enabled = false false, // ladder_execution_enabled = false "convertible", // offering_class + false, // require_different_approver ) // Strict order: the SELECT and the UPSERT must sit between the same @@ -235,7 +237,7 @@ func TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite(t *testing.T) { mock.ExpectExec("pg_advisory_xact_lock").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("SELECT", 1)) mock.ExpectQuery("FROM global_config").WillReturnRows(seeded) - mock.ExpectExec("INSERT INTO global_config").WithArgs(anyArgsCfg(23)...). + mock.ExpectExec("INSERT INTO global_config").WithArgs(anyArgsCfg(24)...). WillReturnResult(pgxmock.NewResult("INSERT", 1)) mock.ExpectCommit() @@ -283,6 +285,7 @@ func TestPGXMock_UpdateGlobalConfigAtomic_ApplyErrorRollsBack(t *testing.T) { false, false, "convertible", + false, ) mock.ExpectBegin() From 9f6037cbcceeee933a2aa861f7f47d2b7fdd9228 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 23 Jul 2026 01:27:14 +0200 Subject: [PATCH 4/7] fix(api): correct misspelling in requireDifferentApprover comment golangci-lint's misspell check flagged "behaviour" in the doc comment on internal/api/handler_purchases.go:931. --- internal/api/handler_purchases.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 1351bd6ce..a55cef5fd 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -928,7 +928,7 @@ func (h *Handler) sendPurchaseScheduledEmail(ctx context.Context, execution *con // the standard RBAC gate. Returns nil when the policy allows the approval. // // Decision logic: -// - mode off (RequireDifferentApprover == false): always returns nil (default behaviour preserved). +// - mode off (RequireDifferentApprover == false): always returns nil (default behavior preserved). // - session == nil AND mode on: returns 500 (fail-closed; we cannot determine // identity without a session; the email-token path should always carry a // session when mode is on because the deep-link flow forces a login). From 07585fac5610ee628f91ee044b40804075e5cf76 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 23 Jul 2026 01:36:17 +0200 Subject: [PATCH 5/7] feat(frontend/sec): wire 4-eyes approval mode toggle + UI gate + badge + banner Adds the Settings > Purchasing "Require different approver" checkbox (require_different_approver on GlobalConfig), an inline banner on the Purchases tab when the mode is on, and a canApproveUnder4Eyes gate in history.ts so a session cannot approve a purchase it created itself. Mirrors the backend's requireDifferentApprover (issue #1005): mode off always allows; mode on requires a different, non-null creator. When RBAC would otherwise show Approve but 4-eyes blocks it, an inline "Awaiting different approver" badge replaces the button instead of silently hiding the action. Several existing history/settings tests needed updated api mocks and assertions since loadHistory() and saveGlobalSettings() now also read/ write require_different_approver. --- .../src/__tests__/allowed-accounts.test.ts | 1 + .../src/__tests__/four-eyes-approval.test.ts | 235 ++++++++++++++++++ .../__tests__/history-approval-queue.test.ts | 1 + .../__tests__/history-approve-button.test.ts | 1 + .../__tests__/history-cancel-button.test.ts | 1 + .../history-cancel-permissions.test.ts | 1 + .../history-marketplace-sell-button.test.ts | 1 + .../__tests__/history-retry-button.test.ts | 1 + .../__tests__/history-revoke-button.test.ts | 1 + frontend/src/__tests__/history.test.ts | 3 +- frontend/src/__tests__/settings.test.ts | 3 + .../src/__tests__/xss-provider-class.test.ts | 1 + frontend/src/api/types.ts | 4 + frontend/src/history.ts | 77 ++++-- frontend/src/index.html | 21 ++ frontend/src/settings.ts | 13 + frontend/src/types.ts | 4 + 17 files changed, 352 insertions(+), 17 deletions(-) create mode 100644 frontend/src/__tests__/four-eyes-approval.test.ts diff --git a/frontend/src/__tests__/allowed-accounts.test.ts b/frontend/src/__tests__/allowed-accounts.test.ts index 06f9e27c7..f8b34ec8e 100644 --- a/frontend/src/__tests__/allowed-accounts.test.ts +++ b/frontend/src/__tests__/allowed-accounts.test.ts @@ -35,6 +35,7 @@ jest.mock('../api', () => ({ // a backend-filtered subset. listAccountsMinimal: jest.fn(), getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), cancelPurchase: jest.fn(), })); diff --git a/frontend/src/__tests__/four-eyes-approval.test.ts b/frontend/src/__tests__/four-eyes-approval.test.ts new file mode 100644 index 000000000..efb105f8b --- /dev/null +++ b/frontend/src/__tests__/four-eyes-approval.test.ts @@ -0,0 +1,235 @@ +/** + * 4-eyes approval mode tests (issue #1005). + * + * GlobalConfig.require_different_approver gates the inline Approve button so + * a session cannot approve a purchase execution it created itself, mirroring + * the backend's requireDifferentApprover in internal/api/handler_purchases.go. + * This is a UX gate only -- the backend remains the security boundary; these + * tests verify the button/badge rendering, not the API enforcement. + */ + +import { loadHistory } from '../history'; + +jest.mock('../api', () => ({ + getHistory: jest.fn(), + getConfig: jest.fn(), + approvePurchase: jest.fn(), + cancelPurchase: jest.fn(), +})); + +jest.mock('../navigation', () => ({ + switchTab: jest.fn(), +})); + +jest.mock('../utils', () => ({ + formatCurrency: jest.fn((val) => `$${val || 0}`), + formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), + formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), + escapeHtml: jest.fn((str) => str || ''), + escapeHtmlAttr: jest.fn((str: string | null | undefined) => { + if (!str) return ''; + return str.replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"').replace(/'/g, '''); + }), + populateAccountFilter: jest.fn(() => Promise.resolve()), +})); + +jest.mock('../confirmDialog', () => ({ + confirmDialog: jest.fn(), +})); + +jest.mock('../toast', () => ({ + showToast: jest.fn(), +})); + +jest.mock('../state', () => ({ + getCurrentUser: jest.fn(), + getCurrentProvider: jest.fn().mockReturnValue(''), + setCurrentProvider: jest.fn(), + getCurrentAccountIDs: jest.fn().mockReturnValue([]), + setCurrentAccountIDs: jest.fn(), + subscribeProvider: jest.fn().mockReturnValue(() => {}), + subscribeAccount: jest.fn().mockReturnValue(() => {}), + getAmortizeUpfront: jest.fn().mockReturnValue(false), + setAmortizeUpfront: jest.fn(), + subscribeAmortizeUpfront: jest.fn().mockReturnValue(() => {}), + getPurchaseHistoryColumnFilters: jest.fn().mockReturnValue({}), + setPurchaseHistoryColumnFilter: jest.fn(), + clearAllPurchaseHistoryColumnFilters: jest.fn(), + getApprovalQueueColumnFilters: jest.fn().mockReturnValue({}), + setApprovalQueueColumnFilter: jest.fn(), + clearAllApprovalQueueColumnFilters: jest.fn(), +})); + +import * as api from '../api'; +import { getCurrentUser } from '../state'; +import { ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions'; + +// Admin with approve-any:purchases (carved out of admin:* per issue #923; +// requires Purchaser group membership). Four-eyes must still block +// self-approval for this user -- the admin wildcard is NOT exempt. +const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', groups: [ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID] }; + +// Regular user with approve-own (but not approve-any) on purchases. +const REG_USER = { + id: 'user-uuid', + email: 'user@example.com', + groups: [], + effectivePermissions: [ + { action: 'approve-own', resource: 'purchases' }, + { action: 'cancel-own', resource: 'purchases' }, + { action: 'retry-own', resource: 'purchases' }, + { action: 'view', resource: 'history' }, + ], +}; +const OTHER_UUID = 'other-uuid'; + +function setupDOM(): void { + while (document.body.firstChild) document.body.removeChild(document.body.firstChild); + + const mkInput = (id: string): HTMLInputElement => { + const el = document.createElement('input'); + el.type = 'date'; + el.id = id; + return el; + }; + const mkSelect = (id: string): HTMLSelectElement => { + const el = document.createElement('select'); + el.id = id; + const opt = document.createElement('option'); + opt.value = ''; + opt.textContent = 'All'; + el.appendChild(opt); + return el; + }; + const mkDiv = (id: string): HTMLDivElement => { + const el = document.createElement('div'); + el.id = id; + return el; + }; + + document.body.appendChild(mkInput('history-start')); + document.body.appendChild(mkInput('history-end')); + document.body.appendChild(mkSelect('history-provider-filter')); + document.body.appendChild(mkSelect('history-account-filter')); + document.body.appendChild(mkDiv('history-summary')); + document.body.appendChild(mkDiv('history-list')); + document.body.appendChild(mkDiv('purchases-approval-queue')); + + // Banner toggled by loadHistory() based on GlobalConfig.require_different_approver. + const banner = document.createElement('div'); + banner.id = 'four-eyes-banner'; + banner.className = 'info-banner hidden'; + document.body.appendChild(banner); +} + +function makeRow(overrides: Record) { + return { + purchase_id: 'exec-1', + timestamp: '2024-01-15T00:00:00Z', + provider: 'aws', + service: 'ec2', + resource_type: 't3.medium', + region: 'us-east-1', + count: 1, + term: 1, + upfront_cost: 100, + estimated_savings: 50, + plan_name: '', + status: 'pending', + ...overrides, + }; +} + +describe('4-eyes approval mode (issue #1005)', () => { + beforeEach(() => { + setupDOM(); + jest.clearAllMocks(); + }); + + test('Approve hidden when mode on + own row', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(REG_USER); + (api.getConfig as jest.Mock).mockResolvedValue({ global: { require_different_approver: true } }); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-own', created_by_user_id: REG_USER.id })], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + expect(list.querySelectorAll('.history-approve-btn')).toHaveLength(0); + }); + + test('Approve shown when mode on + other row', async () => { + // ADMIN_USER holds approve-any, so RBAC alone would allow approving any + // row; this isolates the 4-eyes overlay: creator (OTHER_UUID) differs + // from the session (ADMIN_USER.id), so canApproveUnder4Eyes allows it. + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (api.getConfig as jest.Mock).mockResolvedValue({ global: { require_different_approver: true } }); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-other', created_by_user_id: OTHER_UUID })], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + const buttons = list.querySelectorAll('.history-approve-btn'); + expect(buttons).toHaveLength(1); + expect(buttons[0]?.dataset['approveId']).toBe('exec-other'); + }); + + test('Approve shown when mode off', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(REG_USER); + (api.getConfig as jest.Mock).mockResolvedValue({ global: { require_different_approver: false } }); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-own', created_by_user_id: REG_USER.id })], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + const buttons = list.querySelectorAll('.history-approve-btn'); + expect(buttons).toHaveLength(1); + expect(buttons[0]?.dataset['approveId']).toBe('exec-own'); + // Banner must stay hidden when mode is off. + expect(document.getElementById('four-eyes-banner')?.classList.contains('hidden')).toBe(true); + }); + + test('badge shown when button hidden by mode (admin approve-any self-approval)', async () => { + // Admin holds approve-any, which would normally show Approve on every + // pending row regardless of creator. Four-eyes still blocks self-approval + // -- the admin wildcard is NOT exempt -- so the badge must render instead. + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (api.getConfig as jest.Mock).mockResolvedValue({ global: { require_different_approver: true } }); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-own', created_by_user_id: ADMIN_USER.id })], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + expect(list.querySelectorAll('.history-approve-btn')).toHaveLength(0); + const badge = list.querySelector('.badge-muted'); + expect(badge).not.toBeNull(); + expect(badge?.textContent).toBe('Awaiting different approver'); + // Banner must show when mode is on. + expect(document.getElementById('four-eyes-banner')?.classList.contains('hidden')).toBe(false); + }); + + test('null creator (legacy row): approve hidden', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(REG_USER); + (api.getConfig as jest.Mock).mockResolvedValue({ global: { require_different_approver: true } }); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-legacy', created_by_user_id: undefined })], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + expect(list.querySelectorAll('.history-approve-btn')).toHaveLength(0); + }); +}); diff --git a/frontend/src/__tests__/history-approval-queue.test.ts b/frontend/src/__tests__/history-approval-queue.test.ts index 762a0fb09..5e787f7ff 100644 --- a/frontend/src/__tests__/history-approval-queue.test.ts +++ b/frontend/src/__tests__/history-approval-queue.test.ts @@ -24,6 +24,7 @@ import { loadHistory } from '../history'; jest.mock('../api', () => ({ getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), approvePurchase: jest.fn(), cancelPurchase: jest.fn(), })); diff --git a/frontend/src/__tests__/history-approve-button.test.ts b/frontend/src/__tests__/history-approve-button.test.ts index e60bf12cd..2123b8f0f 100644 --- a/frontend/src/__tests__/history-approve-button.test.ts +++ b/frontend/src/__tests__/history-approve-button.test.ts @@ -22,6 +22,7 @@ import { loadHistory } from '../history'; jest.mock('../api', () => ({ getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), approvePurchase: jest.fn(), cancelPurchase: jest.fn(), })); diff --git a/frontend/src/__tests__/history-cancel-button.test.ts b/frontend/src/__tests__/history-cancel-button.test.ts index 1ed6d3259..9f3ce5dca 100644 --- a/frontend/src/__tests__/history-cancel-button.test.ts +++ b/frontend/src/__tests__/history-cancel-button.test.ts @@ -20,6 +20,7 @@ import { loadHistory } from '../history'; jest.mock('../api', () => ({ getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), cancelPurchase: jest.fn(), })); diff --git a/frontend/src/__tests__/history-cancel-permissions.test.ts b/frontend/src/__tests__/history-cancel-permissions.test.ts index 99021e82c..fc183f54d 100644 --- a/frontend/src/__tests__/history-cancel-permissions.test.ts +++ b/frontend/src/__tests__/history-cancel-permissions.test.ts @@ -22,6 +22,7 @@ import { loadHistory } from '../history'; jest.mock('../api', () => ({ getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), cancelPurchase: jest.fn(), })); diff --git a/frontend/src/__tests__/history-marketplace-sell-button.test.ts b/frontend/src/__tests__/history-marketplace-sell-button.test.ts index fc468bc99..b19785d13 100644 --- a/frontend/src/__tests__/history-marketplace-sell-button.test.ts +++ b/frontend/src/__tests__/history-marketplace-sell-button.test.ts @@ -28,6 +28,7 @@ import { loadHistory } from '../history'; jest.mock('../api', () => ({ getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), createMarketplaceListing: jest.fn(), cancelMarketplaceListing: jest.fn(), })); diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index a96b1f366..d24d13392 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -28,6 +28,7 @@ import { loadHistory } from '../history'; jest.mock('../api', () => ({ getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), retryPurchase: jest.fn(), cancelPurchase: jest.fn(), })); diff --git a/frontend/src/__tests__/history-revoke-button.test.ts b/frontend/src/__tests__/history-revoke-button.test.ts index 325e7b788..264ae1f10 100644 --- a/frontend/src/__tests__/history-revoke-button.test.ts +++ b/frontend/src/__tests__/history-revoke-button.test.ts @@ -24,6 +24,7 @@ import { loadHistory } from '../history'; jest.mock('../api', () => ({ getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), revokePurchase: jest.fn(), })); diff --git a/frontend/src/__tests__/history.test.ts b/frontend/src/__tests__/history.test.ts index 0b47ff45f..7e0830f81 100644 --- a/frontend/src/__tests__/history.test.ts +++ b/frontend/src/__tests__/history.test.ts @@ -5,7 +5,8 @@ import { initHistoryDateRange, viewPlanHistory, loadHistory, setupHistoryHandler // Mock the dependent modules jest.mock('../api', () => ({ - getHistory: jest.fn() + getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), })); jest.mock('../navigation', () => ({ diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index 754bbb9b7..a31769b49 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -546,6 +546,9 @@ describe('Settings Module', () => { // offering_class select is absent in this test harness (no DOM element); // saveGlobalSettings falls back to 'convertible'. offering_class: 'convertible', + // setting-require-different-approver checkbox is absent in this test + // harness (no DOM element); saveGlobalSettings falls back to false. + require_different_approver: false, }); }); diff --git a/frontend/src/__tests__/xss-provider-class.test.ts b/frontend/src/__tests__/xss-provider-class.test.ts index d1accefea..8973fa3d3 100644 --- a/frontend/src/__tests__/xss-provider-class.test.ts +++ b/frontend/src/__tests__/xss-provider-class.test.ts @@ -26,6 +26,7 @@ jest.mock('../utils', () => { jest.mock('../api', () => ({ getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), })); jest.mock('../navigation', () => ({ diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 7a6823c3a..ec7dd3786 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -312,6 +312,10 @@ export interface Config { // Default false. When true, per-account LadderConfig.enabled settings // determine whether the engine runs for that account. laddering_enabled?: boolean; + // When true, a purchase execution cannot be approved by the same user who + // created it -- a different person with approval rights must do so. + // SOX / SOC2 segregation-of-duties control. Default: false. + require_different_approver?: boolean; } export interface ServiceConfig { diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 1aa48e7b6..2615de798 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -42,6 +42,11 @@ type StatusFilter = 'all' | 'pending' | 'completed' | 'failed' | 'expired' | 'ca let lastPurchases: HistoryPurchase[] = []; let activeStatusFilter: StatusFilter = 'all'; +// _fourEyesMode mirrors GlobalConfig.require_different_approver (issue #1005), +// refreshed on every loadHistory() call. Gates the inline Approve button so a +// creator can't approve their own pending purchase when dual-control is on. +let _fourEyesMode = false; + function normalizeStatus(p: HistoryPurchase): string { // Absent status → legacy DB row → counts as completed for filtering. return p.status || 'completed'; @@ -307,7 +312,18 @@ export async function loadHistory(): Promise { provider, account_ids: accountIDs }; - const data = await api.getHistory(filters) as unknown as HistoryResponse; + const [data, cfgResponse] = await Promise.all([ + api.getHistory(filters) as unknown as Promise, + // 4-eyes mode (issue #1005) lives on GlobalConfig; a failed fetch must + // not block the history render, so this leg fails closed to "no config" + // rather than throwing, and _fourEyesMode falls back to its last value. + api.getConfig().catch(() => null), + ]); + if (cfgResponse?.global) { + _fourEyesMode = cfgResponse.global.require_different_approver === true; + } + const banner = document.getElementById('four-eyes-banner'); + if (banner) banner.classList.toggle('hidden', !_fourEyesMode); renderHistorySummary(data.summary ?? null); const purchases = data.purchases || []; renderApprovalQueue(purchases); @@ -506,6 +522,38 @@ function canCancelPendingRow(p: HistoryPurchase): boolean { return canAccess('cancel-own', 'purchases') && p.created_by_user_id === user.id; } +// canApproveUnder4Eyes returns true when the 4-eyes dual-control policy +// (issue #1005, GlobalConfig.require_different_approver) allows sessionUserId +// to approve a row created by row.created_by_user_id. Mirrors the backend's +// requireDifferentApprover in internal/api/handler_purchases.go: mode off → +// always allowed; mode on → allowed only when the row has a recorded, +// different creator (a NULL/legacy creator is denied, matching the backend's +// fail-closed 403 for rows that predate dual-control). +function canApproveUnder4Eyes(row: HistoryPurchase, sessionUserId: string): boolean { + return _fourEyesMode === false || (row.created_by_user_id != null && row.created_by_user_id !== sessionUserId); +} + +// rbacAllowsApprove is the approve-permission decision (issue #286 / +// #1407) WITHOUT the 4-eyes overlay, so renderPendingActionButtons can +// distinguish "no permission at all" (no button, no badge) from +// "permission would allow it but 4-eyes blocks it" (badge instead of button). +function rbacAllowsApprove(p: HistoryPurchase, user: { id: string }): boolean { + const status = (p.status || '').toLowerCase(); + if (status !== 'pending' && status !== 'notified') return false; + // approve-any:purchases is carved out of admin:* (issue #923) and is + // granted by the seeded Purchaser group OR any custom group that + // explicitly lists the verb in effectivePermissions. Gate on the + // verb directly so a non-seeded role with the same grant still + // approves rows the backend would also let through. + if (canAccess('approve-any', 'purchases')) return true; + // Four-eyes RBAC (issue #1407): the session must hold an explicit + // approve-own grant before ownership is consulted. Ownership alone never + // grants approve. + if (!canAccess('approve-own', 'purchases')) return false; + if (!p.created_by_user_id) return false; + return p.created_by_user_id === user.id; +} + // canApprovePendingRow returns true when the current session is permitted // to approve the given pending history row via the inline Approve button // (issue #286). UX gate only — the backend authorizeSessionApprove in @@ -513,7 +561,7 @@ function canCancelPendingRow(p: HistoryPurchase): boolean { // false-positive here surfaces as a 403 toast on click rather than a // successful approve. // -// Heuristic (four-eyes — issue #1407): +// Heuristic (four-eyes RBAC — issue #1407; 4-eyes dual-control — issue #1005): // * status must be "pending" or "notified"; // * any session with approve-any:purchases (carved-out admin verb, // seeded on Purchaser group; can also come from a custom group via @@ -522,23 +570,14 @@ function canCancelPendingRow(p: HistoryPurchase): boolean { // even evaluated (four-eyes: ownership alone does NOT grant approve); // * only then: the row's created_by_user_id must match the current user; // * legacy rows with NULL created_by_user_id → no (the email-token -// path remains the escape hatch). +// path remains the escape hatch); +// * finally, canApproveUnder4Eyes must allow it: when dual-control mode is +// on, the session cannot approve a row it created itself. function canApprovePendingRow(p: HistoryPurchase): boolean { - const status = (p.status || '').toLowerCase(); - if (status !== 'pending' && status !== 'notified') return false; const user = getCurrentUser(); if (!user) return false; - // approve-any:purchases is carved out of admin:* (issue #923) and is - // granted by the seeded Purchaser group OR any custom group that - // explicitly lists the verb in effectivePermissions. Gate on the - // verb directly so a non-seeded role with the same grant still - // approves rows the backend would also let through. - if (canAccess('approve-any', 'purchases')) return true; - // Four-eyes (issue #1407): the session must hold an explicit approve-own - // grant before ownership is consulted. Ownership alone never grants approve. - if (!canAccess('approve-own', 'purchases')) return false; - if (!p.created_by_user_id) return false; - return p.created_by_user_id === user.id; + if (!rbacAllowsApprove(p, user)) return false; + return canApproveUnder4Eyes(p, user.id); } // canRetryFailedRow returns true when the current session is permitted @@ -748,8 +787,14 @@ function sameRowActions(btn: HTMLButtonElement): HTMLButtonElement[] { function renderPendingActionButtons(p: HistoryPurchase): string { if (!p.purchase_id) return ''; const buttons: string[] = []; + const user = getCurrentUser(); if (canApprovePendingRow(p)) { buttons.push(``); + } else if (user && rbacAllowsApprove(p, user) && !canApproveUnder4Eyes(p, user.id)) { + // RBAC would allow Approve, but 4-eyes dual-control (issue #1005) blocks + // this session from approving its own row. Surface the reason inline + // instead of silently hiding the action. + buttons.push('Awaiting different approver'); } if (canCancelPendingRow(p)) { buttons.push(``); diff --git a/frontend/src/index.html b/frontend/src/index.html index d4c026f49..7fec27def 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -154,6 +154,14 @@

Planned Purchases

+ + +
AWS Service Defaults diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index ab6498048..7b3fb9d18 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -154,6 +154,8 @@ const TRACKED_FIELDS = [ 'setting-grace-aws', 'setting-grace-azure', 'setting-grace-gcp', // Recommendations cycle params 'setting-recs-stale-hours', 'setting-recs-lookback-days', + // 4-eyes approval mode (issue #1005) + 'setting-require-different-approver', // Per-service fields ...SERVICE_FIELDS.map(f => f.termId), ...SERVICE_FIELDS.filter(f => f.paymentId !== null).map(f => f.paymentId as string), @@ -3222,6 +3224,12 @@ export async function loadGlobalSettings(): Promise { lookbackSelect.value = String(data.global.recommendations_lookback_days ?? 7); } + // 4-eyes approval mode (issue #1005) + const requireDiffApproverEl = byId('setting-require-different-approver'); + if (requireDiffApproverEl) { + requireDiffApproverEl.checked = data.global.require_different_approver === true; + } + // Update visibility based on loaded settings updateProviderSettingsVisibility(); updateCollectionScheduleVisibility(); @@ -3502,6 +3510,7 @@ export async function saveGlobalSettings(e: Event): Promise { recommendations_cache_stale_hours: rawStaleHours, recommendations_lookback_days: parseInt(byId('setting-recs-lookback-days')?.value || '7', 10), offering_class: offeringClass, + require_different_approver: byId('setting-require-different-approver')?.checked ?? false, }; // Include laddering_enabled in the payload when the Purchasing panel's @@ -3647,6 +3656,10 @@ export async function resetSettings(): Promise { const lookbackSelect = byId('setting-recs-lookback-days'); if (lookbackSelect) lookbackSelect.value = '7'; + // Reset 4-eyes mode to off (default). + const requireDiffApproverResetEl = byId('setting-require-different-approver'); + if (requireDiffApproverResetEl) requireDiffApproverResetEl.checked = false; + // Issue #466: setting el.value / .checked via JS does not fire // 'change'/'input', so none of the change-event-driven visibility // togglers nor the dirty-tracking listeners run on their own here. diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 2b9e51dfb..e9ff34734 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -386,6 +386,10 @@ export interface GlobalConfig { // per-account LadderConfig settings. Set to true to allow per-account // configs to activate individually. laddering_enabled?: boolean; + // When true, a purchase execution cannot be approved by the user who created + // it -- a different user with approve-any:purchases must approve (4-eyes / + // dual-control rule). Defaults to false (feature is opt-in). + require_different_approver?: boolean; } // API Keys types From 635dea4ec853d3cde21f7e01a61fd1ab9bc16f64 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 23 Jul 2026 01:41:25 +0200 Subject: [PATCH 6/7] test(frontend/settings): round-trip coverage for 4-eyes settings toggle (refs #1005) Adds two tests to settings.test.ts covering the load and save sides of the require_different_approver checkbox that 07585fac5 introduced: - populates the checkbox from config.global.require_different_approver on loadGlobalSettings (verifies the load-side deserialization). - sends require_different_approver: true through api.updateConfig when the box is checked and saveGlobalSettings runs (verifies the save-side serialization + payload shape). Both tests exercise the actual Settings module handlers rather than mocking them, matching the style of the surrounding tests in the file. Coverage gap noticed during the CR-fix review of #1500. --- frontend/src/__tests__/settings.test.ts | 41 +++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index a31769b49..0d74dfaa9 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -340,6 +340,29 @@ describe('Settings Module', () => { expect((document.getElementById('setting-notification-days') as HTMLInputElement).value).toBe('5'); }); + test('populates 4-eyes checkbox from config.global.require_different_approver (issue #1005)', async () => { + const checkbox = document.createElement('input'); + checkbox.type = 'checkbox'; + checkbox.id = 'setting-require-different-approver'; + document.getElementById('global-settings-form')?.appendChild(checkbox); + + (api.getConfig as jest.Mock).mockResolvedValue({ + global: { + enabled_providers: [], + default_term: 3, + default_payment: 'all-upfront', + default_coverage: 80, + notification_days_before: 3, + require_different_approver: true + }, + credentials: {} + }); + + await loadGlobalSettings(); + + expect(checkbox.checked).toBe(true); + }); + test('populates collection schedule from config', async () => { (api.getConfig as jest.Mock).mockResolvedValue({ global: { @@ -565,6 +588,24 @@ describe('Settings Module', () => { })); }); + test('sends require_different_approver: true when the 4-eyes checkbox is checked (issue #1005)', async () => { + const checkbox = document.createElement('input'); + checkbox.type = 'checkbox'; + checkbox.id = 'setting-require-different-approver'; + checkbox.checked = true; + document.getElementById('global-settings-form')?.appendChild(checkbox); + + (api.updateConfig as jest.Mock).mockResolvedValue({}); + window.alert = jest.fn(); + + const event = { preventDefault: jest.fn() } as unknown as Event; + await saveGlobalSettings(event); + + expect(api.updateConfig).toHaveBeenCalledWith(expect.objectContaining({ + require_different_approver: true, + })); + }); + test('shows error alert on failure', async () => { (api.updateConfig as jest.Mock).mockRejectedValue(new Error('Save failed')); window.alert = jest.fn(); From 97662ec528840b06b73159b3ebf1cc68954eaedc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 23 Jul 2026 16:57:48 +0200 Subject: [PATCH 7/7] fix(purchase): close 4-eyes bypasses via manager-layer enforcement (#1005) An adversarial review of PR #1500 found requireDifferentApprover wired into only 2 of 4 real approve/execute entry points: execute_mode= "direct" (unconditional self-authorization since the creator always equals the direct-executor on this path) and the SQS async approve worker (no creator comparison at all) could both bypass dual control. Move enforcement into purchase.Manager.ApproveAndExecute, the one choke point all four entry points funnel through, instead of relying on per-handler wiring. The new gate compares the acting actor's own UUID against the execution's creator UUID when available (the two session-based callers), falling back to a resolved-email comparison only for the token/SQS callers that have no actor UUID. The UUID tier also closes a second gap a follow-up adversarial pass caught in this same fix: a per-user API key session carries a real UUID but an empty Email, which an email-only comparison could never match against the creator's real email. Handler-level checks stay in place as defense-in-depth for the two already-gated paths. --- internal/analytics/collector_test.go | 4 + internal/api/handler_purchases.go | 26 ++- internal/api/handler_purchases_test.go | 237 +++++++++++++++++++++ internal/config/interfaces.go | 10 + internal/config/store_postgres.go | 20 ++ internal/mocks/stores.go | 16 ++ internal/purchase/approvals.go | 144 +++++++++++++ internal/purchase/approvals_test.go | 251 +++++++++++++++++++++++ internal/purchase/coverage_extra_test.go | 107 ++++++++++ internal/server/test_helpers_test.go | 4 + 10 files changed, 817 insertions(+), 2 deletions(-) diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 7830150d8..d65dabb3c 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -204,6 +204,10 @@ func (m *mockConfigStore) GetExecutionByPlanAndDate(ctx context.Context, planID return nil, nil } +func (m *mockConfigStore) GetUserEmailByID(ctx context.Context, userID string) (string, error) { + return "", nil +} + func (m *mockConfigStore) SavePurchaseHistory(ctx context.Context, record *config.PurchaseHistoryRecord) error { return nil } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index a55cef5fd..4b9e4efca 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -713,7 +713,7 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), session.Email, actor) } - if err := h.purchase.ApproveAndExecute(ctx, execution.ExecutionID, session.Email, actor); err != nil { + if err := h.purchase.ApproveAndExecute(ctx, execution.ExecutionID, fourEyesActorIdentity(session), actor); err != nil { // ApproveAndExecute returns either a transition error (the row // drifted out of pending/notified between our check and the UPDATE // -- race with cancel/expire) or an execution error (AWS API failed, @@ -2282,6 +2282,28 @@ func resolveCreatorUserID(session *Session) *string { return &uid } +// 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 +// carry a non-empty Email. The stateless admin API key session +// (apiKeyAdminUserID sentinel) does not — falling back to session.Email alone +// there would make the manager's enforceFourEyesPolicy treat every API-key- +// driven approve/direct-execute as "actor identity unknown" and fail closed, +// even when the row was created by a different real user (a legitimate, +// non-self-approval case the RBAC layer already permits via the +// apiKeyAdminUserID short-circuit in authorizeSessionApprove / +// authorizeSessionExecuteDirect). Falling back to the sentinel itself is +// safe: it is never a valid email, so it can never collide with a real +// user's resolved email in the 4-eyes comparison, and a row CREATED via the +// API key (CreatedByUserID stays nil per resolveCreatorUserID above) is +// still denied through the existing NULL-creator fail-closed branch. +func fourEyesActorIdentity(session *Session) string { + if session.Email != "" { + return session.Email + } + return session.UserID +} + // executePurchase handles direct purchase execution from recommendations // matchDuplicateInList scans a slice of pending executions for one that // matches creatorID + idempotencyKey within the idempotency window. @@ -2623,7 +2645,7 @@ func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaF // Human session direct-execute: stamp the session user's UUID onto // transitioned_by (FK-safe via validUUIDPtrOrNil) so the audit trail // records who flipped the row to "approved". - if err := h.purchase.ApproveAndExecute(ctx, executionID, session.Email, validUUIDPtrOrNil(&session.UserID)); err != nil { + if err := h.purchase.ApproveAndExecute(ctx, executionID, fourEyesActorIdentity(session), validUUIDPtrOrNil(&session.UserID)); err != nil { logging.Errorf("purchase[%s]: directExecutePurchase failed after %s: %v", executionID, time.Since(t0), err) return nil, NewClientError(409, fmt.Sprintf("execution %s could not be direct-executed: %v", executionID, err)) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 6dcd523f7..56da6b542 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -10,6 +10,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/internal/purchase" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -4009,6 +4010,117 @@ func TestHandler_executePurchase_DirectExec_ExecuteOwn_Owner(t *testing.T) { assert.Equal(t, true, resultMap["direct_execute"]) } +// TestHandler_executePurchase_DirectExec_FourEyesOn_DeniesSelfExecute is the +// true end-to-end regression test for the HIGH finding on PR #1500's +// adversarial review: execute_mode="direct" bypassed 4-eyes mode entirely +// because directExecutePurchase -> purchase.Manager.ApproveAndExecute never +// consulted RequireDifferentApprover, and by construction the creator IS the +// direct-executor on this path (see the doc comment on +// TestHandler_executePurchase_DirectExec_ExecuteOwn_NonOwner below), so this +// was unconditional self-authorization for anyone holding execute-own or +// execute-any. +// +// Unlike the other DirectExec tests above (which stub out purchase.Manager +// entirely via MockPurchaseManager and so never exercise the actual gate), +// this test wires a REAL *purchase.Manager backed by the same MockConfigStore +// the Handler uses, so the manager-layer 4-eyes check added in +// ApproveAndExecute is genuinely exercised through the full HTTP dispatch +// path. Confirmed to fail (proceed to a real TransitionExecutionStatus call +// with the pre-fix code) and pass after the fix. +func TestHandler_executePurchase_DirectExec_FourEyesOn_DeniesSelfExecute(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + ownerID := "cccccccc-cccc-cccc-cccc-cccccccccccc" + ownerSession := &Session{UserID: ownerID, Email: "owner@example.com"} + mockAuth.On("ValidateSession", ctx, "owner-token").Return(ownerSession, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerID, "execute", "purchases").Return(true, nil) + mockAuth.allowConstraintChecks() + mockAuth.On("HasPermissionAPI", ctx, ownerID, "execute-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerID, "execute-own", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, ownerID).Return([]string{}, nil) + + // Same store stubs as setupDirectExecMocks, except GetGlobalConfig + // returns 4-eyes mode ON instead of the {} default. + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + // enforceFourEyesPolicy re-loads the (freshly-created, randomly-ID'd) + // execution to read CreatedByUserID; the real one always carries the + // direct-executor's own UUID (see comment above), which is what makes + // this an unconditional self-approval case pre-fix. transitionedBy + // carries that same UUID, so the manager's tier-1 UUID comparison denies + // this without ever calling GetUserEmailByID. + mockStore.On("GetExecutionByID", ctx, mock.AnythingOfType("string")).Return( + &config.PurchaseExecution{CreatedByUserID: &ownerID}, nil) + + realManager := purchase.NewManager(purchase.ManagerConfig{ConfigStore: mockStore}) + handler := &Handler{config: mockStore, auth: mockAuth, purchase: realManager} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer owner-token"}, + Body: directExecRecBody, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError") + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.Error(), "4-eyes mode requires a different approver") + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) +} + +// TestHandler_executePurchase_DirectExec_FourEyesOn_PerUserAPIKey_DeniesSelfExecute +// is the true end-to-end regression test for the HIGH gap an independent +// adversarial review found in an earlier version of this fix: a per-user API +// key session (Session.UserAPIKeyID != "") carries a real user UUID as +// Session.UserID but an EMPTY Session.Email -- unlike the normal bearer- +// token session TestHandler_executePurchase_DirectExec_FourEyesOn_DeniesSelfExecute +// above uses. fourEyesActorIdentity's fallback to session.UserID for that +// empty Email produces a UUID-shaped string that a naive email-only 4-eyes +// comparison could never match against the creator's real resolved email, +// silently allowing self-direct-execute under 4-eyes mode. This drives the +// same direct-execute call directly (bypassing the outer RBAC/request +// plumbing already covered by the sibling DirectExec tests) with exactly +// that session shape, proving the manager's UUID-first comparison tier +// still denies it. +func TestHandler_executePurchase_DirectExec_FourEyesOn_PerUserAPIKey_DeniesSelfExecute(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + ownerID := "33333333-3333-3333-3333-333333333333" + // Per-user API key session: a real user UUID, but Email is empty (unlike + // a normal bearer-token session) and UserAPIKeyID is set. + ownerSession := &Session{UserID: ownerID, Email: "", UserAPIKeyID: "key-1"} + + mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + + execution := &config.PurchaseExecution{ + ExecutionID: "exec-apikey-direct", + Status: "pending", + CreatedByUserID: &ownerID, // stamped from the same session that direct-executes + } + mockStore.On("GetExecutionByID", ctx, "exec-apikey-direct").Return(execution, nil) + + realManager := purchase.NewManager(purchase.ManagerConfig{ConfigStore: mockStore}) + handler := &Handler{config: mockStore, purchase: realManager} + _, err := handler.directExecutePurchase(ctx, &events.LambdaFunctionURLRequest{}, execution, ownerSession) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError") + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.Error(), "4-eyes mode requires a different approver") + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) +} + // TestHandler_executePurchase_DirectExec_ExecuteOwn_NonOwner verifies the // execute-own ownership gate: a session with execute-own:purchases but a // different UserID than the execution creator receives a 403. @@ -5466,3 +5578,128 @@ func TestRequireDifferentApprover_EmailTokenPath_ModeOn(t *testing.T) { assert.Equal(t, 403, ce.code) assert.Contains(t, ce.Error(), "4-eyes mode") } + +// TestHandler_approvePurchaseViaSession_FourEyesOn_DeniesSelfApprove is the +// end-to-end regression test for approve path (a): a session holding +// approve-any (e.g. an admin) that also created the execution must be denied +// under 4-eyes mode. This is the pre-existing protection named in PR #1500 -- +// requireDifferentApprover was already wired into approvePurchaseViaSession -- +// but it had never been exercised end-to-end via the real dispatch (only the +// helper itself was unit-tested, per TestRequireDifferentApprover_ModeOn_* +// above). This test locks the handler-level denial in place as a continuity +// guard alongside the new manager-layer gate: mockPurchase asserts +// ApproveAndExecute is never reached, so the manager gate added for the (b)/ +// (c) bypasses is exercised as pure defense-in-depth here, not the primary +// control. +func TestHandler_approvePurchaseViaSession_FourEyesOn_DeniesSelfApprove(t *testing.T) { + ctx := context.Background() + execID := "cccccccc-cccc-cccc-cccc-ccccccccccc1" + adminID := "admin-uuid" + adminEmail := "admin@example.com" + + mockConfig := new(MockConfigStore) + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "valid-token", + Status: "pending", + CreatedByUserID: &adminID, + Recommendations: []config.RecommendationRecord{{ID: "r1"}}, + } + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{UserID: adminID, Email: adminEmail}, nil) + mockAuth.grantAdmin() + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + + mockPurchase := new(MockPurchaseManager) + // The 4-eyes denial must happen before the manager is ever consulted. + + handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + _, err := handler.approvePurchase(ctx, req, execID, "") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "4-eyes mode") + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", + mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +// TestHandler_approvePurchaseViaSession_FourEyesOn_DifferentApproverSucceeds +// is the positive control for path (a): a different (non-creator) approver +// must still succeed end to end, through BOTH the handler-level +// requireDifferentApprover gate AND the new manager-layer enforceFourEyesPolicy +// gate -- proving the fix does not over-broaden into blocking legitimate +// dual-control approvals. Uses a REAL *purchase.Manager (not +// MockPurchaseManager) so the manager gate is genuinely exercised. +func TestHandler_approvePurchaseViaSession_FourEyesOn_DifferentApproverSucceeds(t *testing.T) { + ctx := context.Background() + execID := "cccccccc-cccc-cccc-cccc-ccccccccccc2" + creatorID := "11111111-1111-1111-1111-111111111111" + approverID := "22222222-2222-2222-2222-222222222222" + approverEmail := "approver@example.com" + planID := "plan-fourEyes-session" + + mockConfig := new(MockConfigStore) + exec := &config.PurchaseExecution{ + ExecutionID: execID, + PlanID: planID, + ApprovalToken: "valid-token", + Status: "pending", + CreatedByUserID: &creatorID, + } + approved := &config.PurchaseExecution{ExecutionID: execID, PlanID: planID, Status: "approved"} + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) + mockConfig.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "approved", &approverID).Return(approved, nil) + plan := &config.PurchasePlan{ID: planID, Name: "test-plan"} + mockConfig.On("GetPurchasePlan", ctx, planID).Return(plan, nil) + mockConfig.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + mockConfig.On("IncrementPlanCurrentStep", ctx, planID).Return(nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{UserID: approverID, Email: approverEmail}, nil) + mockAuth.grantAdmin() + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + + realManager := purchase.NewManager(purchase.ManagerConfig{ConfigStore: mockConfig, EmailSender: &stubEmailNotifier{}}) + handler := &Handler{purchase: realManager, config: mockConfig, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + result, err := handler.approvePurchase(ctx, req, execID, "") + require.NoError(t, err) + resultMap := result.(map[string]string) + assert.Equal(t, "completed", resultMap["status"]) + // Session-based callers pass their own UUID as transitionedBy, so the + // manager's 4-eyes check compares UUIDs directly (tier 1) and must never + // need to resolve either identity's email. + mockConfig.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) +} + +// TestFourEyesActorIdentity is a regression guard discovered during review of +// the manager-layer 4-eyes fix: the stateless admin API key session +// (apiKeyAdminUserID sentinel) always carries an empty Session.Email. Passing +// session.Email directly as the 4-eyes actor identity would make +// enforceFourEyesPolicy treat every API-key-driven approve/direct-execute as +// "actor identity unknown" and deny it, even for a legitimate different- +// approver case the RBAC layer already permits (authorizeSessionApprove / +// authorizeSessionExecuteDirect short-circuit on apiKeyAdminUserID). +// fourEyesActorIdentity must fall back to session.UserID (the sentinel +// string) so the 4-eyes comparison has a non-empty, never-a-real-email +// identity to compare instead. +func TestFourEyesActorIdentity(t *testing.T) { + t.Run("real user session returns Email unchanged", func(t *testing.T) { + session := &Session{UserID: "real-uuid", Email: "user@example.com"} + assert.Equal(t, "user@example.com", fourEyesActorIdentity(session)) + }) + t.Run("API key sentinel session falls back to UserID", func(t *testing.T) { + session := &Session{UserID: apiKeyAdminUserID, Email: ""} + assert.Equal(t, apiKeyAdminUserID, fourEyesActorIdentity(session)) + }) +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index bee48e050..f78bd8796 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -73,6 +73,16 @@ type StoreInterface interface { // never returns (nil, nil). A nil error guarantees a non-nil execution. GetExecutionByID(ctx context.Context, executionID string) (*PurchaseExecution, error) GetExecutionByPlanAndDate(ctx context.Context, planID string, scheduledDate time.Time) (*PurchaseExecution, error) + // GetUserEmailByID resolves the email address of the auth user identified + // by userID (the `users` table, owned by internal/auth). Read-only helper + // used by the 4-eyes approval policy (issue #1005) to compare the acting + // approver's identity against a PurchaseExecution.CreatedByUserID without + // internal/purchase importing internal/auth (which would create an import + // cycle: internal/auth already imports internal/config). Returns ("", nil) + // when no user matches userID — the caller treats an empty result as + // "identity unresolved" and fails closed, never as "email intentionally + // blank". + GetUserEmailByID(ctx context.Context, userID string) (string, error) // CountPendingExecutionsForAccount returns the number of purchase_executions // in status 'pending' or 'notified' that reference the given cloud account. // Used by the deleteAccount handler to preflight DB-level FK violations diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index c72cd7801..14fee9b4e 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1422,6 +1422,26 @@ func (s *PostgresStore) GetExecutionByPlanAndDate(ctx context.Context, planID st return &executions[0], nil } +// GetUserEmailByID resolves the email address of the auth user identified by +// userID. The `users` table belongs to internal/auth's schema, but internal/ +// auth already imports internal/config (service_mfa.go), so internal/config +// cannot import internal/auth back without a cycle. This method queries the +// shared database directly by table/column name instead, returning a plain +// string so no auth.User type crosses the package boundary. Returns ("", nil) +// when userID does not resolve to a row -- callers must treat that as +// "identity unresolved," not as a legitimately blank email. +func (s *PostgresStore) GetUserEmailByID(ctx context.Context, userID string) (string, error) { + var email string + err := s.db.QueryRow(ctx, `SELECT email FROM users WHERE id = $1`, userID).Scan(&email) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return "", nil + } + return "", fmt.Errorf("failed to get user email: %w", err) + } + return email, nil +} + // CountPendingExecutionsForAccount returns the number of pending/notified // purchase executions still referencing this cloud account. The deleteAccount // handler calls this before issuing DELETE FROM cloud_accounts so it can diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 546c04e4a..9e283cb64 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -39,6 +39,7 @@ type MockConfigStore struct { CountPendingExecutionsForAccountFn func(ctx context.Context, accountID string) (int, error) ListPendingExecutionIDsForAccountFn func(ctx context.Context, accountID string) ([]string, error) SavePurchaseExecutionFn func(ctx context.Context, exec *config.PurchaseExecution) error + GetUserEmailByIDFn func(ctx context.Context, userID string) (string, error) mock.Mock } @@ -298,6 +299,21 @@ func (m *MockConfigStore) GetExecutionByPlanAndDate(ctx context.Context, planID return v, args.Error(1) } +// GetUserEmailByID mocks the GetUserEmailByID operation. Defaults to ("", nil) +// when no Fn is set and no expectation is registered, so tests that never +// exercise the 4-eyes identity-resolution path (mode off, the default) don't +// need to stub this method. +func (m *MockConfigStore) GetUserEmailByID(ctx context.Context, userID string) (string, error) { + if m.GetUserEmailByIDFn != nil { + return m.GetUserEmailByIDFn(ctx, userID) + } + if !isExpected(&m.Mock, "GetUserEmailByID") { + return "", nil + } + args := m.Called(ctx, userID) + return args.String(0), args.Error(1) +} + // CountPendingExecutionsForAccount mocks the CountPendingExecutionsForAccount operation. // Defaults to (0, nil) when no Fn is set and no expectation is registered. func (m *MockConfigStore) CountPendingExecutionsForAccount(ctx context.Context, accountID string) (int, error) { diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 43bb1fd5e..1fcd84d3d 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -6,6 +6,7 @@ import ( "crypto/subtle" "errors" "fmt" + "strings" "time" "github.com/LeanerCloud/CUDly/internal/config" @@ -162,6 +163,138 @@ func OrphanExecutionError(execution *config.PurchaseExecution) error { execution.ExecutionID, provider) } +// enforceFourEyesPolicy is the UNIVERSAL 4-eyes approval gate (issue #1005). +// It is the single choke point ApproveAndExecute runs before mutating any +// execution state, so every approve/execute entry point inherits the policy +// regardless of which caller reaches ApproveAndExecute: +// +// - approvePurchaseViaSession (session-authed dashboard approve) +// - directExecutePurchase (execute_mode="direct" -- issue #289) +// - ApproveExecution (email-token deep-link approve AND the SQS async +// approve worker, both of which funnel into ApproveAndExecute) +// +// Adversarial review of PR #1500 found that requireDifferentApprover in +// internal/api was wired into only 2 of these 4 entry points, leaving +// execute_mode="direct" (HIGH: the requester always equals the creator on +// this path, so this is unconditional self-authorization) and the SQS +// approve worker (MEDIUM: token + actor_email verification never compared +// the actor against the creator) able to bypass dual control entirely. This +// method closes both gaps by running at the one place all four paths share. +// +// Identity comparison has two tiers, checked in order of precision: +// +// 1. actorUserID (== transitionedBy, the actor's own UUID): populated by the +// two session-based callers (approvePurchaseViaSession, directExecutePurchase) +// via validUUIDPtrOrNil(&session.UserID). When present it is compared +// DIRECTLY against execution.CreatedByUserID -- both are UUIDs from the +// same `users` table, so this is authoritative identity, not a proxy. +// This tier is what closes a gap an independent adversarial review found +// in an earlier version of this fix: a per-user API key session +// (Session.UserAPIKeyID != "") carries a real user UUID but an EMPTY +// Session.Email, unlike a normal bearer-token session. An email-only +// comparison would fall back to a non-email placeholder for that empty +// Email and could never match the creator's real email, silently +// allowing self-direct-execute. Comparing UUIDs first sidesteps the +// email domain entirely for these two callers and cannot be fooled by an +// empty or substituted email string. +// 2. actorEmail: used only when actorUserID is nil -- the token +// (approveViaToken) and SQS (handleApproveMessage) callers into +// ApproveExecution, which always pass transitionedBy=nil (see +// ApproveExecution's own doc comment), so the RBAC/contact-email- +// verified actor identity from authorizeApprovalAction / +// verifyAsyncApprovalActor is the only signal available there. +// execution.CreatedByUserID is a UUID; internal/purchase cannot resolve +// it through internal/auth directly (internal/auth already imports +// internal/config, so the reverse import would cycle), so +// GetUserEmailByID resolves the creator's email via a minimal, +// auth-type-free query on the shared config store instead. +// +// Fails CLOSED whenever mode is on and any of the following holds: the +// execution has no recorded creator (legacy row); neither actorUserID nor a +// non-empty actorEmail identify the acting party; or the resolved identity +// (UUID or email) matches the creator's. +func (m *Manager) enforceFourEyesPolicy(ctx context.Context, executionID, actorEmail string, actorUserID *string) error { + cfg, err := m.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 + } + + execution, err := m.loadExecutionForFourEyes(ctx, executionID) + if err != nil || execution == nil { + return err + } + return m.checkDifferentApprover(ctx, executionID, execution, actorEmail, actorUserID) +} + +// loadExecutionForFourEyes fetches the execution enforceFourEyesPolicy needs +// to compare identities against. Returns (nil, nil) when the executionID +// does not resolve to a row -- there is nothing to gate, and the caller's +// subsequent TransitionExecutionStatus surfaces the standard not-found/ +// cannot-transition error for a bogus or already-terminal executionID. +// Extracted from enforceFourEyesPolicy to keep it under the gocyclo +// threshold. +func (m *Manager) loadExecutionForFourEyes(ctx context.Context, executionID string) (*config.PurchaseExecution, error) { + execution, err := m.config.GetExecutionByID(ctx, executionID) + if errors.Is(err, config.ErrNotFound) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("4-eyes policy check: failed to load execution: %w", err) + } + return execution, nil +} + +// checkDifferentApprover runs the actual identity comparison once mode is +// confirmed on and the execution is loaded. Extracted from +// enforceFourEyesPolicy to keep it under the gocyclo threshold. +func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string, execution *config.PurchaseExecution, actorEmail string, actorUserID *string) error { + if execution.CreatedByUserID == nil { + logging.Warnf("purchase[%s]: 4-eyes mode on; NULL creator (legacy row), denying", executionID) + return fmt.Errorf("approval declined: this execution predates the dual-control feature and has no recorded creator; an admin must disable 4-eyes mode to approve") + } + + // Tier 1: authoritative UUID comparison when the caller identified the + // actor's own user row (session-based callers). See enforceFourEyesPolicy's + // doc comment for why this must run before any email-based fallback. + if actorUserID != nil { + if *actorUserID == *execution.CreatedByUserID { + logging.Warnf("purchase[%s]: 4-eyes mode on; creator %s attempted self-approval (actor UUID match), denied", + executionID, *execution.CreatedByUserID) + return fmt.Errorf("approval declined: 4-eyes mode requires a different approver than the requester") + } + return nil + } + + // Tier 2: no actor UUID available (token/SQS callers) -- fall back to + // resolving and comparing emails. + actorEmail = strings.TrimSpace(actorEmail) + if actorEmail == "" { + logging.Warnf("purchase[%s]: 4-eyes mode on; no actor identity available, denying (fail-closed)", executionID) + return fmt.Errorf("4-eyes approval mode is enabled but no approver identity could be determined; sign in or supply a verified actor before approving") + } + + creatorEmail, err := m.config.GetUserEmailByID(ctx, *execution.CreatedByUserID) + if err != nil { + return fmt.Errorf("4-eyes policy check: failed to resolve creator identity: %w", err) + } + creatorEmail = strings.TrimSpace(creatorEmail) + if creatorEmail == "" { + logging.Warnf("purchase[%s]: 4-eyes mode on; creator account %s not found, denying (fail-closed)", + executionID, *execution.CreatedByUserID) + return fmt.Errorf("4-eyes approval mode is enabled but the creator's account could not be resolved; an admin must investigate before approving") + } + + if strings.EqualFold(creatorEmail, actorEmail) { + logging.Warnf("purchase[%s]: 4-eyes mode on; creator %s attempted self-approval via actor %q, denied", + executionID, *execution.CreatedByUserID, maskActor(actorEmail)) + return fmt.Errorf("approval declined: 4-eyes mode requires a different approver than the requester") + } + return nil +} + // ApproveAndExecute atomically flips a pending/notified execution to // "approved" (stamping ApprovedBy) and then runs the purchase // synchronously, returning the final outcome. Callers MUST have already @@ -181,6 +314,17 @@ func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor stri t0 := time.Now() logging.Infof("purchase[%s]: ApproveAndExecute starting (actor=%q)", executionID, maskActor(actor)) + // Universal 4-eyes gate (issue #1005 / PR #1500 adversarial review): runs + // before any state mutation so every caller -- session approve, direct + // execute, token approve, SQS approve -- is covered by one policy check. + // transitionedBy doubles as the actor's own UUID for this check when the + // caller has one (session-based callers); see enforceFourEyesPolicy's doc + // comment for the full rationale. + if err := m.enforceFourEyesPolicy(ctx, executionID, actor, transitionedBy); err != nil { + logging.Warnf("purchase[%s]: ApproveAndExecute denied by 4-eyes policy: %v", executionID, err) + return err + } + // transitionedBy carries the session user's UUID for human-initiated // approvals (stamped onto transitioned_by); it is nil for token/SQS/system // flows so transitioned_by = NULL on those hops. The human-readable actor diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index e82d8fe21..54723a8fc 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -301,6 +301,257 @@ func TestManager_ApproveAndExecute_SkipsTokenCheck(t *testing.T) { sender.AssertExpectations(t) } +// ─── enforceFourEyesPolicy at the ApproveAndExecute choke point ─────────────── +// (issue #1005 / PR #1500 adversarial review) +// +// ApproveAndExecute is the ONE place every approve/execute entry point +// funnels through: approvePurchaseViaSession, directExecutePurchase +// (execute_mode="direct", issue #289), and ApproveExecution (both the +// email-token deep-link caller and the SQS async approve worker). The +// tests below exercise the manager-layer gate directly with the exact +// (actor, transitionedBy) shapes each real caller passes, proving the two +// bypasses adversarial review found on PR #1500 -- execute_mode="direct" +// (HIGH) and the SQS approve worker (MEDIUM) -- are now closed at the +// shared choke point rather than only at the 2 HTTP handler call sites the +// original PR wired requireDifferentApprover into. + +func fourEyesCfgOnForManager() *config.GlobalConfig { + return &config.GlobalConfig{RequireDifferentApprover: true} +} + +func fourEyesManagerExec(executionID string, creatorID *string) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: executionID, + PlanID: "plan-fourEyes", + Status: "pending", + ApprovalToken: "tok", + CreatedByUserID: creatorID, + } +} + +// TestManager_ApproveAndExecute_FourEyesOn_DeniesSelfApprove models the +// execute_mode="direct" call shape (issue #289): directExecutePurchase always +// passes the session's own email as actor and its own UUID as transitionedBy, +// and by construction CreatedByUserID is stamped from that same session -- +// so self-approval is unconditional on this path. Pre-fix, ApproveAndExecute +// had no 4-eyes awareness at all and this call would have proceeded straight +// to TransitionExecutionStatus and executed the purchase (the HIGH finding). +// The actor's UUID (transitionedBy) is compared directly against +// CreatedByUserID -- no GetUserEmailByID call is needed or expected on this +// tier-1 (UUID) path; see checkDifferentApprover's doc comment. +func TestManager_ApproveAndExecute_FourEyesOn_DeniesSelfApprove(t *testing.T) { + ctx := context.Background() + manager, store, _ := newApproveManager(t) + + creatorID := "user-creator" + creatorEmail := "creator@example.com" + execution := fourEyesManagerExec("exec-direct-self", &creatorID) + + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + store.On("GetExecutionByID", ctx, "exec-direct-self").Return(execution, nil) + + err := manager.ApproveAndExecute(ctx, "exec-direct-self", creatorEmail, &creatorID) + require.Error(t, err) + assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") + store.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) + store.AssertExpectations(t) +} + +// TestManager_ApproveAndExecute_FourEyesOn_AllowsDifferentApprover is the +// positive control for the same call shape: a different actor than the +// creator must still succeed end to end (the fix must not over-broaden into +// blocking legitimate dual-control approvals). Compared by UUID (tier 1); no +// GetUserEmailByID call expected. +func TestManager_ApproveAndExecute_FourEyesOn_AllowsDifferentApprover(t *testing.T) { + ctx := context.Background() + manager, store, sender := newApproveManager(t) + + creatorID := "user-creator" + approverUUID := "user-approver" + approverEmail := "approver@example.com" + execution := fourEyesManagerExec("exec-direct-diff", &creatorID) + updated := &config.PurchaseExecution{ + ExecutionID: "exec-direct-diff", + PlanID: "plan-fourEyes", + Status: "approved", + } + + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + store.On("GetExecutionByID", ctx, "exec-direct-diff").Return(execution, nil) + store.On("TransitionExecutionStatus", ctx, "exec-direct-diff", approveFromStatuses, "approved", &approverUUID).Return(updated, nil) + stubExecuteChain(t, store, sender, "plan-fourEyes") + + err := manager.ApproveAndExecute(ctx, "exec-direct-diff", approverEmail, &approverUUID) + require.NoError(t, err) + require.NotNil(t, updated.ApprovedBy) + assert.Equal(t, approverEmail, *updated.ApprovedBy) + store.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) + store.AssertExpectations(t) + sender.AssertExpectations(t) +} + +// TestManager_ApproveAndExecute_FourEyesOn_PerUserAPIKey_DeniesSelfExecute +// closes the exact HIGH gap an independent adversarial review found in an +// earlier version of this fix: a per-user API key session +// (Session.UserAPIKeyID != "") carries a real user UUID as Session.UserID +// but an EMPTY Session.Email -- unlike a normal bearer-token session. +// internal/api's fourEyesActorIdentity falls back to session.UserID (a UUID +// string, not an email) whenever Email is empty; if checkDifferentApprover +// only ever compared emails, that UUID string could never equal the +// creator's real resolved email and self-direct-execute would silently +// succeed. Comparing UUIDs first (tier 1, via transitionedBy) closes this +// regardless of what the email-shaped actor string looks like. +func TestManager_ApproveAndExecute_FourEyesOn_PerUserAPIKey_DeniesSelfExecute(t *testing.T) { + ctx := context.Background() + manager, store, _ := newApproveManager(t) + + creatorID := "11111111-1111-1111-1111-111111111111" + execution := fourEyesManagerExec("exec-apikey-self", &creatorID) + + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + store.On("GetExecutionByID", ctx, "exec-apikey-self").Return(execution, nil) + + // actor is the UUID string itself (empty Session.Email fallback), exactly + // what internal/api's fourEyesActorIdentity would pass for a per-user API + // key session; transitionedBy is the same UUID, populated regardless of + // Email by validUUIDPtrOrNil(&session.UserID). + err := manager.ApproveAndExecute(ctx, "exec-apikey-self", creatorID, &creatorID) + require.Error(t, err) + assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") + store.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) + store.AssertExpectations(t) +} + +// TestManager_ApproveAndExecute_FourEyesOn_NullCreatorDenied: a legacy row +// with no recorded creator cannot satisfy "different from creator" and must +// fail closed, mirroring the handler-level requireDifferentApprover behavior +// for the same case. +func TestManager_ApproveAndExecute_FourEyesOn_NullCreatorDenied(t *testing.T) { + ctx := context.Background() + manager, store, _ := newApproveManager(t) + + execution := fourEyesManagerExec("exec-legacy", nil) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + store.On("GetExecutionByID", ctx, "exec-legacy").Return(execution, nil) + + err := manager.ApproveAndExecute(ctx, "exec-legacy", "someone@example.com", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "predates the dual-control feature") + store.AssertExpectations(t) +} + +// TestManager_ApproveAndExecute_FourEyesOn_EmptyActorDenied covers the +// "actor identity is unknown" fail-closed branch explicitly: mode is on, the +// execution has a recorded creator, but no actor identity was supplied. +func TestManager_ApproveAndExecute_FourEyesOn_EmptyActorDenied(t *testing.T) { + ctx := context.Background() + manager, store, _ := newApproveManager(t) + + creatorID := "user-creator" + execution := fourEyesManagerExec("exec-no-actor", &creatorID) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + store.On("GetExecutionByID", ctx, "exec-no-actor").Return(execution, nil) + + err := manager.ApproveAndExecute(ctx, "exec-no-actor", "", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "no approver identity could be determined") + store.AssertExpectations(t) +} + +// TestManager_ApproveAndExecute_FourEyesOff_AllowsSelfApprove is the +// regression-continuity guard: mode off (the default) must behave exactly +// as before this fix -- the creator may approve/direct-execute their own +// row, and the gate performs no extra DB calls. +func TestManager_ApproveAndExecute_FourEyesOff_AllowsSelfApprove(t *testing.T) { + ctx := context.Background() + manager, store, sender := newApproveManager(t) + + creatorID := "user-creator" + updated := &config.PurchaseExecution{ExecutionID: "exec-mode-off", PlanID: "plan-fourEyes", Status: "approved"} + store.On("TransitionExecutionStatus", ctx, "exec-mode-off", approveFromStatuses, "approved", &creatorID).Return(updated, nil) + stubExecuteChain(t, store, sender, "plan-fourEyes") + + err := manager.ApproveAndExecute(ctx, "exec-mode-off", "creator@example.com", &creatorID) + require.NoError(t, err) + store.AssertNotCalled(t, "GetExecutionByID", mock.Anything, mock.Anything) + store.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) + store.AssertExpectations(t) + sender.AssertExpectations(t) +} + +// TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied models +// the SQS async approve worker call shape (MEDIUM finding): ApproveExecution +// always passes transitionedBy=nil, and the only identity signal is the +// verified actor email. Pre-fix, ApproveExecution/ApproveAndExecute had no +// awareness of 4-eyes mode at all, so a replayed or forwarded token+actor +// matching the creator's own email would approve the row despite mode being +// on. +func TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied(t *testing.T) { + ctx := context.Background() + manager, store, _ := newApproveManager(t) + + creatorID := "user-creator" + creatorEmail := "creator@example.com" + execution := &config.PurchaseExecution{ + ExecutionID: "exec-sqs-self", + PlanID: "plan-fourEyes", + Status: "pending", + ApprovalToken: "valid-token", + CreatedByUserID: &creatorID, + } + + store.On("GetExecutionByID", ctx, "exec-sqs-self").Return(execution, nil) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + store.On("GetUserEmailByID", ctx, creatorID).Return(creatorEmail, nil) + + err := manager.ApproveExecution(ctx, "exec-sqs-self", "valid-token", creatorEmail) + require.Error(t, err) + assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") + store.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertExpectations(t) +} + +// TestManager_ApproveExecution_FourEyesOn_DifferentActor_Allowed is the +// positive control for the SQS/token call shape: an actor that resolves to +// a different email than the creator must still succeed. +func TestManager_ApproveExecution_FourEyesOn_DifferentActor_Allowed(t *testing.T) { + ctx := context.Background() + manager, store, sender := newApproveManager(t) + + creatorID := "user-creator" + execution := &config.PurchaseExecution{ + ExecutionID: "exec-sqs-diff", + PlanID: "plan-fourEyes", + Status: "pending", + ApprovalToken: "valid-token", + CreatedByUserID: &creatorID, + } + updated := &config.PurchaseExecution{ + ExecutionID: "exec-sqs-diff", + PlanID: "plan-fourEyes", + Status: "approved", + ApprovalToken: "valid-token", + } + + store.On("GetExecutionByID", ctx, "exec-sqs-diff").Return(execution, nil) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + store.On("GetUserEmailByID", ctx, creatorID).Return("creator@example.com", nil) + store.On("TransitionExecutionStatus", ctx, "exec-sqs-diff", approveFromStatuses, "approved", (*string)(nil)).Return(updated, nil) + stubExecuteChain(t, store, sender, "plan-fourEyes") + + err := manager.ApproveExecution(ctx, "exec-sqs-diff", "valid-token", "approver@example.com") + require.NoError(t, err) + require.NotNil(t, updated.ApprovedBy) + assert.Equal(t, "approver@example.com", *updated.ApprovedBy) + store.AssertExpectations(t) + sender.AssertExpectations(t) +} + func TestManager_CancelExecution(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index 92c83e0cf..3c2aaf7a6 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -383,6 +383,113 @@ func TestProcessMessage_ApproveHappyPath(t *testing.T) { mockEmail.AssertExpectations(t) } +// TestProcessMessage_ApproveFourEyesOn_SelfApproveDenied is the true +// end-to-end regression test for the MEDIUM finding on PR #1500's +// adversarial review: the SQS approve worker (handleApproveMessage -> +// verifyAsyncApprovalActor -> ApproveExecution) enforced token + actor_email +// + per-account contact_email matching, but never consulted 4-eyes mode. +// A replayed or forwarded {token, actor_email} pair belonging to the same +// person who created the execution would approve it even with dual control +// enabled. Pre-fix this test proceeds straight through to +// TransitionExecutionStatus (proven by stashing the fix and re-running); +// post-fix ProcessMessage returns the 4-eyes denial before any state change. +func TestProcessMessage_ApproveFourEyesOn_SelfApproveDenied(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + + accountID := "acct-1" + creatorID := "user-creator" + creatorEmail := "owner@example.com" + exec := &config.PurchaseExecution{ + ExecutionID: "exec-appv-self", + Status: "pending", + ApprovalToken: "correct-token", + CreatedByUserID: &creatorID, + Recommendations: []config.RecommendationRecord{ + {CloudAccountID: &accountID}, + }, + } + account := &config.CloudAccount{ID: accountID, ContactEmail: creatorEmail} + + // verifyAsyncApprovalActor + the manager's own fetch inside ApproveExecution + // both load the execution; enforceFourEyesPolicy loads it again. + mockStore.On("GetExecutionByID", ctx, "exec-appv-self").Return(exec, nil) + mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) + mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + mockStore.On("GetUserEmailByID", ctx, creatorID).Return(creatorEmail, nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + dashboardURL: "https://dashboard.example.com", + } + + err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-appv-self","token":"correct-token","actor_email":"owner@example.com"}`) + require.Error(t, err) + assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertExpectations(t) + mockEmail.AssertExpectations(t) +} + +// TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds is the +// positive control for the same SQS call shape: an actor_email verified +// against the per-account contact_email list, but belonging to a different +// person than the creator, must still succeed end to end. +func TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + + accountID := "acct-1" + planID := "plan-appv-diff" + creatorID := "user-creator" + exec := &config.PurchaseExecution{ + ExecutionID: "exec-appv-diff", + PlanID: planID, + Status: "pending", + ApprovalToken: "correct-token", + CreatedByUserID: &creatorID, + Recommendations: []config.RecommendationRecord{ + {CloudAccountID: &accountID}, + }, + } + approved := &config.PurchaseExecution{ + ExecutionID: "exec-appv-diff", + PlanID: planID, + Status: "approved", + Recommendations: exec.Recommendations, + } + account := &config.CloudAccount{ID: accountID, ContactEmail: "approver@example.com"} + + // verifyAsyncApprovalActor + ApproveExecution's own load + the new + // enforceFourEyesPolicy load (issue #1005 gate) + mintRevocationToken's + // post-success re-fetch: 4 loads total. + mockStore.On("GetExecutionByID", ctx, "exec-appv-diff").Return(exec, nil).Times(4) + mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) + mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + mockStore.On("GetUserEmailByID", ctx, creatorID).Return("owner@example.com", nil) + mockStore.On("TransitionExecutionStatus", ctx, "exec-appv-diff", []string{"pending", "notified"}, "approved", (*string)(nil)).Return(approved, nil) + plan := &config.PurchasePlan{ID: planID, Name: "test-plan"} + mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) + mockEmail.On("SendPurchaseConfirmation", ctx, mock.Anything).Return(nil) + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + mockStore.On("IncrementPlanCurrentStep", ctx, planID).Return(nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + dashboardURL: "https://dashboard.example.com", + } + + err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-appv-diff","token":"correct-token","actor_email":"approver@example.com"}`) + require.NoError(t, err) + mockStore.AssertExpectations(t) + mockEmail.AssertExpectations(t) +} + func TestProcessMessage_CancelHappyPath(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 67e8592e6..64bbe7033 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -93,6 +93,10 @@ func (m *mockConfigStoreForHealth) GetExecutionByPlanAndDate(ctx context.Context return nil, nil } +func (m *mockConfigStoreForHealth) GetUserEmailByID(ctx context.Context, userID string) (string, error) { + return "", nil +} + func (m *mockConfigStoreForHealth) SavePurchaseHistory(ctx context.Context, record *config.PurchaseHistoryRecord) error { return nil }