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) 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) {