Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions internal/api/handler_history.go
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma
PlanID: exec.PlanID,
Status: exec.Status,
CreatedByUserID: createdBy,
CreatedBySystem: exec.CreatedBySystem,
CreatedByUserEmail: createdByEmail,
RetryExecutionID: retryExecID,
RetryAttemptN: exec.RetryAttemptN,
Expand Down
24 changes: 24 additions & 0 deletions internal/api/handler_history_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2165,3 +2165,27 @@ func TestExecutionToHistoryRow_RedriveUnsafe(t *testing.T) {
})
}
}

// TestHandler_getHistory_ProjectsCreatedBySystem: the History UI needs the #233 marker.
func TestHandler_getHistory_ProjectsCreatedBySystem(t *testing.T) {
ctx := context.Background()
mockStore := new(MockConfigStore)
approverEmail := "ops@example.com"
recs := []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1"}}
execs := []config.PurchaseExecution{
{ExecutionID: "sys", Status: "pending", ScheduledDate: time.Now(), CreatedBySystem: true, Recommendations: recs},
{ExecutionID: "human", Status: "pending", ScheduledDate: time.Now(), Recommendations: recs},
}
mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil)
mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(execs, nil)
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil)
mockAuth, req := adminHistoryReq(ctx)

result, err := (&Handler{auth: mockAuth, config: mockStore}).getHistory(ctx, req, map[string]string{})
require.NoError(t, err)
got := map[string]bool{}
for _, p := range result.(HistoryResponse).Purchases {
got[p.PurchaseID] = p.CreatedBySystem
}
assert.Equal(t, map[string]bool{"sys": true, "human": false}, got)
}
1 change: 1 addition & 0 deletions internal/api/handler_plans_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,7 @@ func TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil(t *testing.T) {
Run(func(args mock.Arguments) {
exec := args.Get(1).(*config.PurchaseExecution)
savedCreators = append(savedCreators, exec.CreatedByUserID)
assert.False(t, exec.CreatedBySystem, "an API-key row has a human initiator and must not be system-created (#233)")
}).
Return(nil).Times(2)
mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil)
Expand Down
13 changes: 10 additions & 3 deletions internal/api/handler_purchases.go
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,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.ExecutionID, execution.CreatedByUserID); err != nil {
if err := h.requireDifferentApprover(ctx, tokenSession, execution.ExecutionID, execution.CreatedByUserID, execution.CreatedBySystem); err != nil {
return nil, err
}
// SEC-01 follow-up (issue #60 review): the deep-link flow always forces
Expand Down Expand Up @@ -1019,7 +1019,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.ExecutionID, execution.CreatedByUserID)
return h.requireDifferentApprover(ctx, session, execution.ExecutionID, execution.CreatedByUserID, execution.CreatedBySystem)
}

// authorizeScopedSessionApprove runs the account-scope gate (issue #92)
Expand Down Expand Up @@ -1355,7 +1355,7 @@ func (h *Handler) sendPurchaseScheduledEmail(ctx context.Context, execution *con
//
// 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, subjectID string, creatorUserID *string) error {
func (h *Handler) requireDifferentApprover(ctx context.Context, session *Session, subjectID string, creatorUserID *string, systemCreated bool) error {
cfg, err := h.config.GetGlobalConfig(ctx)
if err != nil {
return fmt.Errorf("4-eyes policy check: failed to load global config: %w", err)
Expand All @@ -1370,6 +1370,13 @@ func (h *Handler) requireDifferentApprover(ctx context.Context, session *Session
return NewClientError(500, "4-eyes approval mode is enabled but no session could be resolved; sign in before approving")
}

if creatorUserID == nil && systemCreated {
// Scheduler-minted row: no human requester exists, so any identified
// approver (session != nil, checked above) is the second pair of eyes.
logging.Infof("4-eyes[%s]: mode on; system-created row approved by user %s", subjectID, session.UserID)
return nil
}

if creatorUserID == nil {
logging.Warnf("4-eyes[%s]: mode on; NULL creator (legacy row) attempted by user %s, denied",
subjectID, session.UserID)
Expand Down
57 changes: 51 additions & 6 deletions internal/api/handler_purchases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6471,7 +6471,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.ExecutionID, exec.CreatedByUserID)
err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID, false)
require.NoError(t, err, "mode off: creator self-approve must be allowed")
}

Expand All @@ -6489,7 +6489,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.ExecutionID, exec.CreatedByUserID)
err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID, false)
require.Error(t, err)
ce, ok := IsClientError(err)
require.True(t, ok)
Expand All @@ -6511,7 +6511,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.ExecutionID, exec.CreatedByUserID)
err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID, false)
require.NoError(t, err, "mode on: different user must be allowed")
}

Expand All @@ -6529,7 +6529,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.ExecutionID, exec.CreatedByUserID)
err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID, false)
require.Error(t, err, "mode on: admin self-approve must be denied")
ce, ok := IsClientError(err)
require.True(t, ok)
Expand All @@ -6550,7 +6550,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.ExecutionID, exec.CreatedByUserID)
err := handler.requireDifferentApprover(ctx, session, exec.ExecutionID, exec.CreatedByUserID, false)
require.Error(t, err)
ce, ok := IsClientError(err)
require.True(t, ok)
Expand All @@ -6570,7 +6570,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.ExecutionID, exec.CreatedByUserID)
err := handler.requireDifferentApprover(ctx, nil, exec.ExecutionID, exec.CreatedByUserID, false)
require.Error(t, err)
ce, ok := IsClientError(err)
require.True(t, ok)
Expand Down Expand Up @@ -6768,3 +6768,48 @@ func TestFourEyesActorIdentity(t *testing.T) {
assert.Equal(t, apiKeyAdminUserID, fourEyesActorIdentity(session))
})
}

// TestRequireDifferentApprover_ModeOn_SystemCreated pins the #233 rule matrix
// for scheduler-minted rows versus unflagged nil-creator rows.
func TestRequireDifferentApprover_ModeOn_SystemCreated(t *testing.T) {
t.Parallel()
ctx := context.Background()
creator := "user-creator"
cases := []struct {
name string
session *Session
creator *string
system bool
code int
msg string
}{
{"system row, other user", &Session{UserID: "approver"}, nil, true, 0, ""},
{"unflagged nil row", &Session{UserID: "approver"}, nil, false, 403, "predates the dual-control feature"},
{"flag does not override creator", &Session{UserID: creator}, &creator, true, 403, "different approver"},
{"system row, no session", nil, nil, true, 500, "no session"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
mockConfig := new(MockConfigStore)
mockConfig.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil)
err := (&Handler{config: mockConfig}).requireDifferentApprover(ctx, tc.session, "exec-1", tc.creator, tc.system)
if tc.code == 0 {
require.NoError(t, err)
return
}
ce, ok := IsClientError(err)
require.True(t, ok)
assert.Equal(t, tc.code, ce.code)
assert.Contains(t, ce.Error(), tc.msg)
})
}
}

// TestNewPendingExecution_NotSystemCreated: only the sweep may set the flag (#233).
func TestNewPendingExecution_NotSystemCreated(t *testing.T) {
t.Parallel()
exec, _, err := newPendingExecution(&ExecutePurchaseRequest{CapacityPercent: 100}, 0, 0)
require.NoError(t, err)
assert.False(t, exec.CreatedBySystem)
}
2 changes: 1 addition & 1 deletion internal/api/handler_ri_exchange.go
Original file line number Diff line number Diff line change
Expand Up @@ -2582,7 +2582,7 @@ func (h *Handler) requireNoDirectExchangeUnderFourEyes(ctx context.Context) erro
// any identified approver, but never by an anonymous token holder (issue #518).
func (h *Handler) requireDifferentRIExchangeApprover(ctx context.Context, session *Session, record *config.RIExchangeRecord) error {
if record.CreatedByUserID != nil {
return h.requireDifferentApprover(ctx, session, record.ID, record.CreatedByUserID)
return h.requireDifferentApprover(ctx, session, record.ID, record.CreatedByUserID, false)
}
if session != nil {
return nil
Expand Down
30 changes: 16 additions & 14 deletions internal/config/store_postgres.go
Original file line number Diff line number Diff line change
Expand Up @@ -1092,8 +1092,8 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx,
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23, $24, $25, $26, $27, $28)
idempotency_key, scheduled_execution_at, no_submission, created_by_system
) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23, $24, $25, $26, $27, $28, $29)
ON CONFLICT (execution_id) DO UPDATE SET
status = $3,
notification_sent = $6,
Expand Down Expand Up @@ -1130,7 +1130,7 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx,
planIDArg = execution.PlanID
}

// created_by_user_id and retry_attempt_n are INSERT-only: the original
// created_by_user_id, created_by_system and retry_attempt_n are INSERT-only: the original
// creator and the retry-chain position must not be rewritten by an
// ON CONFLICT update (e.g. the scheduler upserting status transitions).
// They are omitted from the DO UPDATE SET clause above.
Expand Down Expand Up @@ -1172,6 +1172,7 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx,
execution.IdempotencyKey,
execution.ScheduledExecutionAt,
execution.NoSubmission,
execution.CreatedBySystem,
)

if err != nil {
Expand Down Expand Up @@ -1323,7 +1324,7 @@ func (s *PostgresStore) transitionExecutionStatus(ctx context.Context, execution
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
`

records, err := s.queryExecutions(ctx, query, executionID, toStatus, fromStatuses, actor, cancelActor, sawEmpty, tokenHash)
Expand Down Expand Up @@ -1539,7 +1540,7 @@ func (s *PostgresStore) GetExecutionsByStatuses(ctx context.Context, statuses []
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE status = ANY($1)
AND NOT (status = 'completed' AND COALESCE(error, '') = '')
Expand Down Expand Up @@ -1653,7 +1654,7 @@ func (s *PostgresStore) GetPlannedExecutions(ctx context.Context, statuses []str
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE status = ANY($1)
ORDER BY scheduled_date ASC NULLS LAST, id ASC
Expand All @@ -1678,7 +1679,7 @@ func (s *PostgresStore) GetStaleApprovedExecutions(ctx context.Context, olderTha
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE status = 'approved' AND updated_at < NOW() - $1::interval
`
Expand Down Expand Up @@ -1720,7 +1721,7 @@ func (s *PostgresStore) ListStuckExecutions(ctx context.Context, statuses []stri
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE status = ANY($1)
AND updated_at < NOW() - $2::interval
Expand All @@ -1741,7 +1742,7 @@ func (s *PostgresStore) GetPendingExecutions(ctx context.Context) ([]PurchaseExe
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE status IN ('pending', 'notified')
AND (expires_at IS NULL OR expires_at > NOW())
Expand All @@ -1765,7 +1766,7 @@ func (s *PostgresStore) GetPendingExecutionsTx(ctx context.Context, tx pgx.Tx) (
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE status IN ('pending', 'notified')
AND (expires_at IS NULL OR expires_at > NOW())
Expand Down Expand Up @@ -1852,7 +1853,7 @@ func (s *PostgresStore) ListRecentSubmitsTx(ctx context.Context, tx pgx.Tx, crea
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE source = $3
AND status IN (` + submitDedupeStatuses + `)
Expand Down Expand Up @@ -1882,7 +1883,7 @@ func (s *PostgresStore) GetExecutionByID(ctx context.Context, executionID string
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE execution_id = $1
`
Expand All @@ -1909,7 +1910,7 @@ func (s *PostgresStore) GetExecutionByPlanAndDate(ctx context.Context, planID st
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE plan_id = $1 AND scheduled_date = $2
`
Expand Down Expand Up @@ -2098,6 +2099,7 @@ func scanExecutionRows(rows pgx.Rows) ([]PurchaseExecution, error) {
&idempotencyKey,
&scheduledExecutionAt,
&exec.NoSubmission,
&exec.CreatedBySystem,
)
if err != nil {
return nil, fmt.Errorf("failed to scan execution: %w", err)
Expand Down Expand Up @@ -2138,7 +2140,7 @@ func (s *PostgresStore) GetScheduledExecutionsDue(ctx context.Context) ([]Purcha
created_by_user_id, retry_execution_id, retry_attempt_n,
approval_token_expires_at,
executed_by_user_id, executed_at, pre_approval_skip_reason,
idempotency_key, scheduled_execution_at, no_submission
idempotency_key, scheduled_execution_at, no_submission, created_by_system
FROM purchase_executions
WHERE status = 'scheduled'
AND scheduled_execution_at IS NOT NULL
Expand Down
Loading
Loading