From 90b39e1ba24084ba681d778c8e9f57d9bd636e9f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 14:16:36 +0200 Subject: [PATCH 1/2] refactor(api): take the creator ID in requireDifferentApprover The 4-eyes check only needs the subject ID and the creator's user ID, not a whole PurchaseExecution. Taking those two values lets the RI exchange approval path reuse the same policy instead of duplicating it. No behaviour change for purchase approvals; log lines now use a neutral "4-eyes[]" prefix. Refs #221 --- internal/api/handler_purchases.go | 26 +++++++++++++------------- internal/api/handler_purchases_test.go | 12 ++++++------ 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 31e2b5357..0a87277b0 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -651,7 +651,7 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio // 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 { + if err := h.requireDifferentApprover(ctx, tokenSession, execution.ExecutionID, execution.CreatedByUserID); err != nil { return nil, err } // SEC-01 follow-up (issue #60 review): the deep-link flow always forces @@ -809,7 +809,7 @@ func (h *Handler) authorizeApproveSession(ctx context.Context, session *Session, // pass the approve-any / approve-own gate reach this check. The admin // wildcard inside authorizeSessionApprove short-circuits RBAC but NOT this // check; admins who created the row must disable the mode first. - return h.requireDifferentApprover(ctx, session, execution) + return h.requireDifferentApprover(ctx, session, execution.ExecutionID, execution.CreatedByUserID) } // authorizeScopedSessionApprove runs the account-scope gate (issue #92) @@ -1082,15 +1082,15 @@ func (h *Handler) sendPurchaseScheduledEmail(ctx context.Context, execution *con // - 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 +// - session present AND creatorUserID == 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. +// - session present AND session.UserID == *creatorUserID: returns 403. +// - session present AND session.UserID != *creatorUserID: 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 { +func (h *Handler) requireDifferentApprover(ctx context.Context, session *Session, subjectID string, creatorUserID *string) error { cfg, err := h.config.GetGlobalConfig(ctx) if err != nil { return fmt.Errorf("4-eyes policy check: failed to load global config: %w", err) @@ -1101,19 +1101,19 @@ func (h *Handler) requireDifferentApprover(ctx context.Context, session *Session // Mode is on. if session == nil { - logging.Warnf("purchase[%s]: 4-eyes mode on but no session available; denying (fail-closed)", execution.ExecutionID) + logging.Warnf("4-eyes[%s]: mode on but no session available; denying (fail-closed)", subjectID) 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) + if creatorUserID == nil { + logging.Warnf("4-eyes[%s]: mode on; NULL creator (legacy row) attempted by user %s, denied", + subjectID, 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) + if session.UserID == *creatorUserID { + logging.Warnf("4-eyes[%s]: mode on; creator %s attempted self-approval, denied", + subjectID, session.UserID) return NewClientError(403, "approval declined: 4-eyes mode requires a different approver than the requester") } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 5c2d3a424..fa4f6f009 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -6308,7 +6308,7 @@ func TestRequireDifferentApprover_ModeOff_AllowsSameUser(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOff(), nil) handler := &Handler{config: mockConfig} - err := handler.requireDifferentApprover(ctx, session, exec) + err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID) require.NoError(t, err, "mode off: creator self-approve must be allowed") } @@ -6326,7 +6326,7 @@ func TestRequireDifferentApprover_ModeOn_DeniesSameUser(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) handler := &Handler{config: mockConfig} - err := handler.requireDifferentApprover(ctx, session, exec) + err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID) require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok) @@ -6348,7 +6348,7 @@ func TestRequireDifferentApprover_ModeOn_AllowsDifferentUser(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) handler := &Handler{config: mockConfig} - err := handler.requireDifferentApprover(ctx, session, exec) + err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID) require.NoError(t, err, "mode on: different user must be allowed") } @@ -6366,7 +6366,7 @@ func TestRequireDifferentApprover_ModeOn_DeniesAdminSelfApprove(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) handler := &Handler{config: mockConfig} - err := handler.requireDifferentApprover(ctx, session, exec) + err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID) require.Error(t, err, "mode on: admin self-approve must be denied") ce, ok := IsClientError(err) require.True(t, ok) @@ -6387,7 +6387,7 @@ func TestRequireDifferentApprover_ModeOn_NullCreatorDenied(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) handler := &Handler{config: mockConfig} - err := handler.requireDifferentApprover(ctx, session, exec) + err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID) require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok) @@ -6407,7 +6407,7 @@ func TestRequireDifferentApprover_NilAuth_500(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) handler := &Handler{config: mockConfig} - err := handler.requireDifferentApprover(ctx, nil, exec) + err := handler.requireDifferentApprover(ctx, nil, exec.ExecutionID, exec.CreatedByUserID) require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok) From a01bb1ab8abe9f58eaec6d673184645fee02d2ad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 14:16:55 +0200 Subject: [PATCH 2/2] fix(api): enforce four-eyes on RI exchange approval With RequireDifferentApprover on, the user who created an RI exchange could approve and execute it themselves: neither the session branch nor the email-token branch of approveRIExchange called requireDifferentApprover. Both branches now run the same check as purchase approvals before the pending -> processing transition. The token branch resolves the caller's session for the comparison and fails closed (500) when the mode is on and no session is present, as the purchase token path does. The token branch also matters because a session-branch 403 falls through to it when a token is supplied, so a session-only fix would be bypassed. Exchanges with no recorded creator (every exchange the automated pipeline writes today) keep their current behaviour. The purchase policy denies a nil creator, which applied here would make every RI exchange unapprovable while the mode is on; that decision is open in issue #233. Closes #221 --- internal/api/handler_ri_exchange.go | 22 +++- internal/api/handler_ri_exchange_test.go | 132 +++++++++++++++++++++++ 2 files changed, 152 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 0126dd9fb..2185af800 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2201,7 +2201,7 @@ func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunct } if token != "" { - return h.approveRIExchangeViaToken(ctx, id, token) + return h.approveRIExchangeViaToken(ctx, req, id, token) } return h.approveRIExchangeViaSession(ctx, req, id, nil) @@ -2210,11 +2210,16 @@ func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunct // approveRIExchangeViaToken is the legacy email-link branch of approveRIExchange. // It validates the approval token, transitions the exchange to processing, and // executes it. Extracted to keep approveRIExchange within cyclomatic-complexity limits. -func (h *Handler) approveRIExchangeViaToken(ctx context.Context, id, token string) (any, error) { +func (h *Handler) approveRIExchangeViaToken(ctx context.Context, req *events.LambdaFunctionURLRequest, id, token string) (any, error) { record, err := h.validateExchangeApproval(ctx, id, token) if err != nil { return nil, err } + // The deep link forces a login, so the approver's session is resolvable here. + err = h.requireDifferentRIExchangeApprover(ctx, h.tryGetSession(ctx, req), record) + if err != nil { + return nil, err + } // Token-based approval: no session user, so transitioned_by = NULL. transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing", nil) @@ -2258,6 +2263,10 @@ func (h *Handler) approveRIExchangeViaSession(ctx context.Context, req *events.L if err != nil { return nil, err } + err = h.requireDifferentRIExchangeApprover(ctx, session, record) + if err != nil { + return nil, err + } // Session-authed approval: stamp the session user as the actor. transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing", resolveCreatorUserID(session)) @@ -2380,6 +2389,15 @@ func (h *Handler) authorizeSessionApproveRIExchange(ctx context.Context, session return nil } +// requireDifferentRIExchangeApprover applies the purchase 4-eyes check when a creator +// is recorded; automated exchanges have none and stay approvable (see issue #233). +func (h *Handler) requireDifferentRIExchangeApprover(ctx context.Context, session *Session, record *config.RIExchangeRecord) error { + if record.CreatedByUserID == nil { + return nil + } + return h.requireDifferentApprover(ctx, session, record.ID, record.CreatedByUserID) +} + // validateExchangeApproval validates ID, token, and record state for an exchange approval. func (h *Handler) validateExchangeApproval(ctx context.Context, id, token string) (*config.RIExchangeRecord, error) { if err := validateUUID(id); err != nil { diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index ed3ec0239..94e4b2356 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -505,6 +505,138 @@ func TestApproveRIExchange_SessionApproveOwn(t *testing.T) { }) } +type riApproverGrant func(m *MockAuthService, userID string) + +func grantRIAdmin(m *MockAuthService, _ string) { m.grantAdminPurchaser() } + +func grantRIApproveOwn(m *MockAuthService, userID string) { + m.On("HasPermissionAPI", context.Background(), userID, auth.ActionApproveAny, auth.ResourcePurchases).Return(false, nil) + m.On("HasPermissionAPI", context.Background(), userID, auth.ActionApproveOwn, auth.ResourcePurchases).Return(true, nil) +} + +func grantRINone(m *MockAuthService, userID string) { + m.On("HasPermissionAPI", context.Background(), userID, mock.Anything, auth.ResourcePurchases).Return(false, nil) +} + +// fourEyesRIExchangeFixture wires a pending exchange created by creatorID and a +// session for approverID holding the given approve grant. +func fourEyesRIExchangeFixture(t *testing.T, creatorID *string, approverID string, grant riApproverGrant, fourEyes bool) (*Handler, *MockConfigStore, string, string) { + t.Helper() + ctx := context.Background() + id := "550e8400-e29b-41d4-a716-446655440021" + token := "four-eyes-token" + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + mockAuth.On("ValidateSession", ctx, "sess").Return(&Session{UserID: approverID, Email: approverID + "@example.com"}, nil) + mockAuth.On("ValidateCSRFToken", ctx, "sess", "csrf").Return(nil) + grant(mockAuth, approverID) + + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, + Status: "pending", + ApprovalToken: config.HashApprovalToken(token), + SourceRIIDs: []string{"ri-1"}, + PaymentDue: "50.00", + CreatedByUserID: creatorID, + }, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RequireDifferentApprover: fourEyes, + RIExchangeMaxDailyUSD: 1000, + RIExchangeMaxPerExchangeUSD: 500, + }, nil) + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything). + Return(&config.RIExchangeRecord{ID: id, Status: "processing", SourceRIIDs: []string{"ri-1"}, PaymentDue: "50.00"}, nil) + mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil) + mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil) + mockStore.On("StampRIExchangeApprovedBy", ctx, id, mock.Anything).Return(nil) + + return &Handler{config: mockStore, auth: mockAuth}, mockStore, id, token +} + +func csrfSessionReq() *events.LambdaFunctionURLRequest { + req := sessionReq("sess") + req.Headers["x-csrf-token"] = "csrf" + return req +} + +// TestApproveRIExchange_FourEyes (issue #221): with 4-eyes on, the creator cannot +// approve their own exchange through the session or the email-token branch. +func TestApproveRIExchange_FourEyes(t *testing.T) { + ctx := context.Background() + creator := "creator-uuid" + + denied := func(t *testing.T, mockStore *MockConfigStore, err error, code int) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want ClientError, got %v", err) + assert.Equal(t, code, ce.code) + mockStore.AssertNotCalled(t, "TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + } + approved := func(t *testing.T, mockStore *MockConfigStore, err error) { + t.Helper() + require.NoError(t, err) + mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, mock.Anything, "pending", "processing", mock.Anything) + } + + t.Run("session: creator with approve-own denied", func(t *testing.T) { + h, store, id, _ := fourEyesRIExchangeFixture(t, &creator, creator, grantRIApproveOwn, true) + _, err := h.approveRIExchange(ctx, csrfSessionReq(), id, "") + denied(t, store, err, 403) + assert.Contains(t, err.Error(), "4-eyes mode") + }) + + t.Run("session: admin creator denied", func(t *testing.T) { + h, store, id, _ := fourEyesRIExchangeFixture(t, &creator, creator, grantRIAdmin, true) + _, err := h.approveRIExchange(ctx, csrfSessionReq(), id, "") + denied(t, store, err, 403) + }) + + t.Run("session: creator holding the email token still denied", func(t *testing.T) { + h, store, id, token := fourEyesRIExchangeFixture(t, &creator, creator, grantRIApproveOwn, true) + _, err := h.approveRIExchange(ctx, csrfSessionReq(), id, token) + denied(t, store, err, 403) + }) + + t.Run("token: creator session without approve rights denied", func(t *testing.T) { + h, store, id, token := fourEyesRIExchangeFixture(t, &creator, creator, grantRINone, true) + _, err := h.approveRIExchange(ctx, sessionReq("sess"), id, token) + denied(t, store, err, 403) + assert.Contains(t, err.Error(), "4-eyes mode") + }) + + t.Run("token: no session fails closed", func(t *testing.T) { + h, store, id, token := fourEyesRIExchangeFixture(t, &creator, creator, grantRIAdmin, true) + _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, token) + denied(t, store, err, 500) + }) + + t.Run("session: different approver allowed", func(t *testing.T) { + h, store, id, _ := fourEyesRIExchangeFixture(t, &creator, "approver-uuid", grantRIAdmin, true) + _, err := h.approveRIExchange(ctx, csrfSessionReq(), id, "") + approved(t, store, err) + }) + + t.Run("token: different approver allowed", func(t *testing.T) { + h, store, id, token := fourEyesRIExchangeFixture(t, &creator, "approver-uuid", grantRINone, true) + _, err := h.approveRIExchange(ctx, sessionReq("sess"), id, token) + approved(t, store, err) + }) + + t.Run("mode off: creator allowed", func(t *testing.T) { + h, store, id, _ := fourEyesRIExchangeFixture(t, &creator, creator, grantRIApproveOwn, false) + _, err := h.approveRIExchange(ctx, csrfSessionReq(), id, "") + approved(t, store, err) + }) + + t.Run("automated exchange without creator stays approvable", func(t *testing.T) { + h, store, id, _ := fourEyesRIExchangeFixture(t, nil, "approver-uuid", grantRIAdmin, true) + _, err := h.approveRIExchange(ctx, csrfSessionReq(), id, "") + approved(t, store, err) + }) +} + // TestApproveRIExchange_LegacyTokenStillWorks verifies that the token-only path // continues to work for non-session callers after the dual-auth refactor (backwards-compat). func TestApproveRIExchange_LegacyTokenStillWorks(t *testing.T) {