diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index abe6373b6..5274c1548 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -273,7 +273,7 @@ func (m *mockConfigStore) ListPendingExecutionIDsForAccount(ctx context.Context, return nil, nil } -func (m *mockConfigStore) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string) (*config.PurchaseExecution, error) { +func (m *mockConfigStore) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string) (*config.PurchaseExecution, error) { return nil, nil } @@ -301,7 +301,7 @@ func (m *mockConfigStore) GetRIExchangeRecordByToken(ctx context.Context, token func (m *mockConfigStore) GetRIExchangeHistory(ctx context.Context, since time.Time, limit int) ([]config.RIExchangeRecord, error) { return nil, nil } -func (m *mockConfigStore) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string) (*config.RIExchangeRecord, error) { +func (m *mockConfigStore) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string, actor *string) (*config.RIExchangeRecord, error) { return nil, nil } func (m *mockConfigStore) CompleteRIExchange(ctx context.Context, id string, exchangeID string) error { @@ -386,7 +386,7 @@ func (m *mockConfigStore) ListAccountRegistrations(_ context.Context, _ config.A func (m *mockConfigStore) UpdateAccountRegistration(_ context.Context, _ *config.AccountRegistration) error { return nil } -func (m *mockConfigStore) TransitionRegistrationStatus(_ context.Context, _ *config.AccountRegistration, _ string) error { +func (m *mockConfigStore) TransitionRegistrationStatus(_ context.Context, _ *config.AccountRegistration, _ string, _ *string) error { return nil } func (m *mockConfigStore) DeleteAccountRegistration(_ context.Context, _ string) error { diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 63c311aba..ad0e72a73 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -330,7 +330,7 @@ func TestHandler_rejectRIExchange_AlreadyProcessed(t *testing.T) { mockStore.On("GetRIExchangeRecord", ctx, "11111111-1111-1111-1111-111111111111").Return( &config.RIExchangeRecord{ID: "11111111-1111-1111-1111-111111111111", ApprovalToken: "tok"}, nil) // Transition returns nil indicating already processed - mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "cancelled"). + mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "cancelled", mock.Anything). Return(nil, nil) h := &Handler{config: mockStore} @@ -364,7 +364,7 @@ func TestHandler_approveRIExchange_AlreadyProcessed(t *testing.T) { mockStore := new(MockConfigStore) mockStore.On("GetRIExchangeRecord", ctx, "11111111-1111-1111-1111-111111111111").Return( &config.RIExchangeRecord{ID: "11111111-1111-1111-1111-111111111111", ApprovalToken: "tok"}, nil) - mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "processing"). + mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "processing", mock.Anything). Return(nil, nil) h := &Handler{config: mockStore} @@ -372,3 +372,88 @@ func TestHandler_approveRIExchange_AlreadyProcessed(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "already processed") } + +// --------------------------------------------------------------------------- +// validUUIDPtrOrNil — actor-stamp helper (issue #1009) +// --------------------------------------------------------------------------- + +// TestValidUUIDPtrOrNil_ReturnsPointerForValidUUID asserts the happy-path: +// a string that parses as a UUID is passed through unchanged. +func TestValidUUIDPtrOrNil_ReturnsPointerForValidUUID(t *testing.T) { + uid := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + result := validUUIDPtrOrNil(&uid) + require.NotNil(t, result, "valid UUID must return non-nil pointer") + assert.Equal(t, uid, *result) +} + +// TestValidUUIDPtrOrNil_ReturnsNilForNonUUID asserts that a non-UUID string +// (e.g. "admin-api-key") returns nil so it is never used as a FK actor. +func TestValidUUIDPtrOrNil_ReturnsNilForNonUUID(t *testing.T) { + s := "admin-api-key" + assert.Nil(t, validUUIDPtrOrNil(&s), "non-UUID reviewer_by must map to nil actor") +} + +// TestValidUUIDPtrOrNil_ReturnsNilForNilInput asserts that a nil *string +// returns nil (no panic on nil dereference). +func TestValidUUIDPtrOrNil_ReturnsNilForNilInput(t *testing.T) { + assert.Nil(t, validUUIDPtrOrNil(nil)) +} + +// TestHandler_TransitionRegistrationStatus_ActorStamped drives the real +// rejectRegistration handler and asserts that TransitionRegistrationStatus is +// called with a non-nil actor equal to the reviewing admin session's UUID (the +// common human-reviewed path). Exercising the handler (not the store directly) +// keeps the test honest: it fails if the handler ever stops deriving the actor +// from the session and threading it through (CR feedback on PR #1011). +func TestHandler_TransitionRegistrationStatus_ActorStamped(t *testing.T) { + ctx := context.Background() + const regID = "11111111-1111-1111-1111-111111111111" + const actorID = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + reg := &config.AccountRegistration{ID: regID, Status: "pending"} + mockStore.On("GetAccountRegistration", ctx, regID).Return(reg, nil) + // setReviewMetadata stamps reg.ReviewedBy = session.UserID, so the actor + // threaded into the store must equal the session UUID. + mockStore.On("TransitionRegistrationStatus", ctx, reg, "pending", + mock.MatchedBy(func(a *string) bool { return a != nil && *a == actorID }), + ).Return(nil) + + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{UserID: actorID, Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + _, err := handler.rejectRegistration(ctx, req, regID) + require.NoError(t, err) +} + +// TestHandler_TransitionRegistrationStatus_NonUUIDActorIsNil drives the real +// rejectRegistration handler via the admin-API-key path, where requireAdmin +// returns a session whose UserID is the literal "admin-api-key" (not a UUID +// FK). The handler must pass nil as the actor so transitioned_by = NULL. +func TestHandler_TransitionRegistrationStatus_NonUUIDActorIsNil(t *testing.T) { + ctx := context.Background() + const regID = "22222222-2222-2222-2222-222222222222" + + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + reg := &config.AccountRegistration{ID: regID, Status: "pending"} + mockStore.On("GetAccountRegistration", ctx, regID).Return(reg, nil) + // API-key reviewer ("admin-api-key") is not a UUID: actor must be nil. + mockStore.On("TransitionRegistrationStatus", ctx, reg, "pending", (*string)(nil)).Return(nil) + + handler := &Handler{config: mockStore, apiKey: "admin-secret"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "admin-secret"}, + } + _, err := handler.rejectRegistration(ctx, req, regID) + require.NoError(t, err) +} diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index ee1a949fe..fa1344d16 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -210,7 +210,7 @@ func (h *Handler) expireStaleExecutionsAsync(staleExecs []config.PurchaseExecuti go func() { ctx := context.Background() for _, exec := range staleExecs { - _, err := h.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"pending", "notified"}, "expired") + _, err := h.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"pending", "notified"}, "expired", nil) if err != nil { logging.Warnf("history: async expire of execution %s failed: %v", exec.ExecutionID, err) } @@ -243,6 +243,25 @@ func (h *Handler) resolveUserEmails(ctx context.Context, executions []config.Pur return out } +// expireIfStale transitions a pending/notified execution to "expired" when +// its ScheduledDate is older than approvalExpiryWindow. Returns the possibly- +// updated execution. Transition failures are non-fatal — the row still +// renders, just with its original status. +func (h *Handler) expireIfStale(ctx context.Context, exec config.PurchaseExecution) config.PurchaseExecution { + if exec.Status != "pending" && exec.Status != "notified" { + return exec + } + if time.Since(exec.ScheduledDate) < approvalExpiryWindow { + return exec + } + updated, err := h.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"pending", "notified"}, "expired", nil) + if err != nil { + logging.Warnf("history: failed to expire execution %s: %v", exec.ExecutionID, err) + return exec + } + return *updated +} + // resolvePendingApproverEmail returns the notification email the approval // link was sent to (or would have been, if SES failed). Single-tenant // deployments share one value across every pending row, so this is looked up diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index bc5ff4022..253fbca4b 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -242,8 +242,9 @@ func TestHandler_getHistory_ExpireIfStale(t *testing.T) { Return([]config.PurchaseExecution{freshExec(), staleExec("pending")}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) // The goroutine uses context.Background(); context.Background() == ctx in - // this test, so the matcher fires correctly. - mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired"). + // this test, so the matcher fires correctly. The trailing mock.Anything + // matches the actor *string (nil for the system-initiated async expire). + mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired", mock.Anything). Run(waitForCall(done)). Return(&expired, nil).Once() @@ -263,7 +264,7 @@ func TestHandler_getHistory_ExpireIfStale(t *testing.T) { // Exactly one Transition call, only for the stale row. mockStore.AssertNumberOfCalls(t, "TransitionExecutionStatus", 1) - mockStore.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired") + mockStore.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired", mock.Anything) historyResp := result.(HistoryResponse) require.Len(t, historyResp.Purchases, 2, "both executions must render as history rows") @@ -300,7 +301,7 @@ func TestHandler_getHistory_ExpireIfStale(t *testing.T) { mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). Return([]config.PurchaseExecution{staleExec("notified")}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) - mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired"). + mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired", mock.Anything). Run(waitForCall(done)). Return(&expired, nil).Once() @@ -337,7 +338,7 @@ func TestHandler_getHistory_ExpireIfStale(t *testing.T) { mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). Return([]config.PurchaseExecution{staleExec("pending")}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) - mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired"). + mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired", mock.Anything). Run(waitForCall(done)). Return(nil, errors.New("simulated store failure")).Once() @@ -431,7 +432,7 @@ func TestHandler_getHistory_GetIsReadOnly(t *testing.T) { mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{stale}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) - mockStore.On("TransitionExecutionStatus", mock.Anything, "stale-ro-exec", []string{"pending", "notified"}, "expired"). + mockStore.On("TransitionExecutionStatus", mock.Anything, "stale-ro-exec", []string{"pending", "notified"}, "expired", mock.Anything). Run(func(_ mock.Arguments) { close(transitionCalled) // signal that the goroutine reached the transition <-gate // block until the test releases it @@ -540,6 +541,50 @@ func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) { assert.Equal(t, 1, resp.Summary.TotalPending) } +// TestHandler_expireStaleExecutionsAsync_SystemActorIsNil asserts that the +// async stale-expire sweep passes nil as the actor param to +// TransitionExecutionStatus. Expiry is a system-initiated path (no human +// session), so transitioned_by must be NULL on the affected row (issue #1009). +// The transition fires in a background goroutine (issue #1032: GET is a pure +// read), so the test blocks on a done channel rather than asserting +// synchronously. +func TestHandler_expireStaleExecutionsAsync_SystemActorIsNil(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + approverEmail := "ops@example.com" + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + staleID := "actor-nil-stale-exec" + expired := config.PurchaseExecution{ExecutionID: staleID, Status: "expired"} + done := make(chan struct{}) + + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). + Return([]config.PurchaseExecution{{ + ExecutionID: staleID, + Status: "pending", + ScheduledDate: time.Now().Add(-8 * 24 * time.Hour), + }}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + // System path: the async expire must pass nil actor so transitioned_by = + // NULL. The (*string)(nil) literal is the contract under test. The + // goroutine uses context.Background(); use mock.Anything for ctx. + mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired", + (*string)(nil), + ).Run(func(_ mock.Arguments) { close(done) }).Return(&expired, nil).Once() + + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + _, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err) + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("expire goroutine did not call TransitionExecutionStatus within 5s") + } +} + // TestHandler_getHistory_PermissionDenied asserts that a non-admin user without // view:purchases gets 403 and never reaches the store. func TestHandler_getHistory_PermissionDenied(t *testing.T) { diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index 36021848b..5fe51a00e 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -965,7 +965,7 @@ func TestPerAccountPerms_PlannedPurchase_AllowedAccountPlanSucceeds(t *testing.T mockStore.On("GetExecutionByID", ctx, executionID).Return(&config.PurchaseExecution{ ExecutionID: executionID, PlanID: planID, Status: "pending", }, nil) - mockStore.On("TransitionExecutionStatus", ctx, executionID, mock.Anything, "paused"). + mockStore.On("TransitionExecutionStatus", ctx, executionID, mock.Anything, "paused", mock.Anything). Return(transitoned, nil) // Plan is associated with account A — within the scoped user's allowed set. @@ -985,7 +985,7 @@ func TestPerAccountPerms_PlannedPurchase_AllowedAccountPlanSucceeds(t *testing.T require.NoError(t, err, "scoped user must be able to pause an account-A execution") require.NotNil(t, result) assert.Equal(t, "paused", result.Status, "result must reflect the paused status") - mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, executionID, mock.Anything, "paused") + mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, executionID, mock.Anything, "paused", mock.Anything) } // ─── 10. GET /ri-exchange/instances ────────────────────────────────────────── diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 3b3be8471..fd6e99fe7 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -272,7 +272,7 @@ func (h *Handler) pausePlannedPurchase(ctx context.Context, req *events.LambdaFu } // Atomically transition to paused - if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "running"}, "paused"); err != nil { + if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "running"}, "paused", resolveCreatorUserID(session)); err != nil { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be paused: %v", executionID, err)) } @@ -296,7 +296,7 @@ func (h *Handler) resumePlannedPurchase(ctx context.Context, req *events.LambdaF } // Atomically transition from paused back to pending - if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"paused"}, "pending"); err != nil { + if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"paused"}, "pending", resolveCreatorUserID(session)); err != nil { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be resumed: %v", executionID, err)) } @@ -321,7 +321,7 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc // Atomically transition to running — only one concurrent caller can succeed. // TransitionExecutionStatus handles not-found and wrong-status cases. - if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "running"); err != nil { + if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "running", resolveCreatorUserID(session)); err != nil { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be started: %v", executionID, err)) } @@ -348,7 +348,7 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF return nil, err } - cancelled, err := h.cancelOrRecoverExecution(ctx, executionID) + cancelled, err := h.cancelOrRecoverExecution(ctx, executionID, resolveCreatorUserID(session)) if err != nil { return nil, err } @@ -371,8 +371,9 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF // (ErrExecutionNotInExpectedStatus), it fetches the row instead so the caller // can still drive the plan-disable side-effect, keeping the operation // idempotent across retries. -func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID string) (*config.PurchaseExecution, error) { - cancelled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "cancelled") +// actor is the UUID of the user initiating the cancel (nil for system-initiated paths). +func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID string, actor *string) (*config.PurchaseExecution, error) { + cancelled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "cancelled", actor) if err == nil { return cancelled, nil } @@ -500,9 +501,11 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio 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). globalCfg, cfgErr := h.config.GetGlobalConfig(ctx) if cfgErr == nil && globalCfg.GetPurchaseDelay() > 0 { - return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), actor) + return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), actor, nil) } // ApproveExecution now runs the purchase synchronously inside the // same call (issue #372). When it returns nil the AWS API call @@ -552,15 +555,21 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam 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 + // pre-fire delay path). + actor := validUUIDPtrOrNil(&session.UserID) + // Check for Gmail-style pre-fire delay (issue #291 wave-2). When // PurchaseDelayHours > 0 the SDK call is deferred; the user gets a // "scheduled, revoke before X" email and a window to cancel at $0. globalCfg, cfgErr := h.config.GetGlobalConfig(ctx) if cfgErr == nil && globalCfg.GetPurchaseDelay() > 0 { - return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), session.Email) + return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), session.Email, actor) } - if err := h.purchase.ApproveAndExecute(ctx, execution.ExecutionID, session.Email); err != nil { + if err := h.purchase.ApproveAndExecute(ctx, execution.ExecutionID, session.Email, 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, @@ -626,8 +635,8 @@ func (h *Handler) authorizeSessionApprove(ctx context.Context, session *Session, // scheduled_execution_at <= NOW() and fires the actual SDK call. // Revoking a status=scheduled execution (via the revoke handler or the // History "Revoke" button) transitions it to "cancelled" at zero cloud cost. -func (h *Handler) approveWithDelay(ctx context.Context, execution *config.PurchaseExecution, delay time.Duration, actor string) (any, error) { - updated, err := h.scheduleApprovedExecution(ctx, execution, delay, actor) +func (h *Handler) approveWithDelay(ctx context.Context, execution *config.PurchaseExecution, delay time.Duration, actor string, transitionedBy *string) (any, error) { + updated, err := h.scheduleApprovedExecution(ctx, execution, delay, actor, transitionedBy) if err != nil { if errors.Is(err, config.ErrExecutionNotInExpectedStatus) { // A concurrent Cancel beat the approve: the CAS rejected because the row @@ -656,13 +665,17 @@ func (h *Handler) approveWithDelay(ctx context.Context, execution *config.Purcha // ScheduledExecutionAt = now+delay and ApprovedBy. No SDK call is made. // Returns the updated execution on success. // +// actor is the human-readable approver identity (email) recorded in +// ApprovedBy. transitionedBy is the approver's user UUID stamped onto the +// audit trail by the CAS (nil for token/scheduler paths with no session UUID). +// // The atomic CAS (TransitionExecutionStatus WHERE status IN (pending,notified)) // prevents a silent revoke loss: if a concurrent Cancel flipped the row to // "cancelled" between the caller's SELECT and this write, TransitionExecutionStatus // returns ErrExecutionNotInExpectedStatus and we surface a 409 instead of // blindly overwriting the cancelled state. -func (h *Handler) scheduleApprovedExecution(ctx context.Context, execution *config.PurchaseExecution, delay time.Duration, actor string) (*config.PurchaseExecution, error) { - updated, err := h.config.TransitionExecutionStatus(ctx, execution.ExecutionID, []string{"pending", "notified"}, "scheduled") +func (h *Handler) scheduleApprovedExecution(ctx context.Context, execution *config.PurchaseExecution, delay time.Duration, actor string, transitionedBy *string) (*config.PurchaseExecution, error) { + updated, err := h.config.TransitionExecutionStatus(ctx, execution.ExecutionID, []string{"pending", "notified"}, "scheduled", transitionedBy) if err != nil { return nil, fmt.Errorf("failed to transition execution %s to scheduled: %w", execution.ExecutionID, err) } @@ -1994,7 +2007,10 @@ func (h *Handler) directExecutePurchase(ctx context.Context, execution *config.P logging.Errorf("AUDIT GAP: failed to stamp direct-execute audit fields on %s: %v", executionID, err) } - if err := h.purchase.ApproveAndExecute(ctx, executionID, session.Email); err != nil { + // 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 { 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 27b7f94ae..115b48cf0 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -312,7 +312,7 @@ func TestHandler_approvePurchase_SessionApproveAnyChainsToExecute(t *testing.T) // ApproveAndExecute, not ApproveExecution. The token-only path runs // ApproveExecution; the dashboard click runs ApproveAndExecute. Both // converge inside the Manager. - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} @@ -356,7 +356,7 @@ func TestHandler_approvePurchase_SessionExecuteFailureSurfacesAs409(t *testing.T mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(errors.New("AWS RI purchase failed")) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(errors.New("AWS RI purchase failed")) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} @@ -411,7 +411,7 @@ func TestHandler_approvePurchase_AzureOrphanRejects409(t *testing.T) { assert.Contains(t, ce.Error(), "no longer exists") assert.Contains(t, ce.Error(), "azure") // Guard fires before the purchase manager is touched. - mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } @@ -447,7 +447,7 @@ func TestHandler_approvePurchase_GCPOrphanRejects409(t *testing.T) { assert.Equal(t, 409, ce.code) assert.Contains(t, ce.Error(), "no longer exists") assert.Contains(t, ce.Error(), "gcp") - mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } @@ -477,7 +477,7 @@ func TestHandler_approvePurchase_AWSOrphanFallsThrough(t *testing.T) { mockPurchase := new(MockPurchaseManager) // Guard does not fire; ApproveAndExecute is called normally. - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} @@ -516,7 +516,7 @@ func TestHandler_approvePurchase_NonOrphanUnchanged(t *testing.T) { mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} @@ -1007,7 +1007,7 @@ func TestHandler_pausePlannedPurchase(t *testing.T) { paused := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "paused"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "running"}, "paused").Return(paused, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "running"}, "paused", mock.Anything).Return(paused, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1034,7 +1034,7 @@ func TestHandler_pausePlannedPurchase_NotFound(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "running"}, "paused").Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "running"}, "paused", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1061,7 +1061,7 @@ func TestHandler_resumePlannedPurchase(t *testing.T) { resumed := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "pending"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"paused"}, "pending").Return(resumed, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"paused"}, "pending", mock.Anything).Return(resumed, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1093,7 +1093,7 @@ func TestHandler_runPlannedPurchase(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "running").Return(transitioned, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "running", mock.Anything).Return(transitioned, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1123,7 +1123,7 @@ func TestHandler_deletePlannedPurchase(t *testing.T) { cancelled := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "cancelled"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "cancelled").Return(cancelled, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "cancelled", mock.Anything).Return(cancelled, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1168,7 +1168,7 @@ func TestHandler_deletePlannedPurchase_DisablesPlan(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled").Return(cancelled, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled", mock.Anything).Return(cancelled, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) // Assert that UpdatePurchasePlan is called with enabled=false. mockStore.On("UpdatePurchasePlan", ctx, mock.MatchedBy(func(p *config.PurchasePlan) bool { @@ -1220,7 +1220,7 @@ func TestHandler_deletePlannedPurchase_AlreadyDisabledPlan(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled").Return(cancelled, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled", mock.Anything).Return(cancelled, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1271,7 +1271,7 @@ func TestHandler_deletePlannedPurchase_ConflictRetryDisablesPlan(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled").Return(nil, conflictErr) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled", mock.Anything).Return(nil, conflictErr) mockStore.On("GetExecutionByID", ctx, execID).Return(existingExec, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) mockStore.On("UpdatePurchasePlan", ctx, mock.MatchedBy(func(p *config.PurchasePlan) bool { @@ -1322,7 +1322,7 @@ func TestHandler_deletePlannedPurchase_ConflictRetryAlreadyDisabled(t *testing.T mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled").Return(nil, conflictErr) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled", mock.Anything).Return(nil, conflictErr) mockStore.On("GetExecutionByID", ctx, execID).Return(existingExec, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) // UpdatePurchasePlan is intentionally NOT registered; AssertExpectations @@ -1368,7 +1368,7 @@ func TestHandler_deletePlannedPurchase_ConflictRetryRunningReturns409(t *testing mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled").Return(nil, conflictErr) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled", mock.Anything).Return(nil, conflictErr) mockStore.On("GetExecutionByID", ctx, execID).Return(runningExec, nil) // GetPurchasePlan must NOT be called — AssertExpectations verifies this. @@ -1400,7 +1400,7 @@ func TestHandler_pausePlannedPurchase_NilExecution(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "running"}, "paused").Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "running"}, "paused", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1437,7 +1437,7 @@ func TestHandler_pausePlannedPurchase_IneligibleStatus(t *testing.T) { mockAuth.grantAdmin() // Store returns ErrExecutionNotInExpectedStatus when the row is 'completed' // and cannot be transitioned to 'paused'. - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "running"}, "paused"). + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "running"}, "paused", mock.Anything). Return(nil, fmt.Errorf("%w: execution 11111111-1111-1111-1111-111111111111 cannot transition from %q to %q", config.ErrExecutionNotInExpectedStatus, "completed", "paused")) @@ -1469,7 +1469,7 @@ func TestHandler_resumePlannedPurchase_NilExecution(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"paused"}, "pending").Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"paused"}, "pending", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1495,7 +1495,7 @@ func TestHandler_runPlannedPurchase_NilExecution(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "paused"}, "running").Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "paused"}, "running", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1521,7 +1521,7 @@ func TestHandler_deletePlannedPurchase_NilExecution(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "paused"}, "cancelled").Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "paused"}, "cancelled", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1535,6 +1535,38 @@ func TestHandler_deletePlannedPurchase_NilExecution(t *testing.T) { assert.Nil(t, result) } +// --- Audit actor stamping tests (issue #1009) --- + +// TestHandler_pausePlannedPurchase_ActorStamped asserts that the handler passes +// the session user UUID as the actor param to TransitionExecutionStatus so that +// transitioned_by is set on the execution row. +func TestHandler_pausePlannedPurchase_ActorStamped(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + const actorID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + session := &Session{UserID: actorID, Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + paused := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "paused"} + // Actor must equal the session UserID (pointer value comparison via reflect.DeepEqual). + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", + []string{"pending", "running"}, "paused", + mock.MatchedBy(func(a *string) bool { return a != nil && *a == actorID }), + ).Return(paused, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + } + res, err := handler.pausePlannedPurchase(ctx, req, "11111111-1111-1111-1111-111111111111") + require.NoError(t, err) + assert.Equal(t, "paused", res.Status) +} + func TestHandler_getPlannedPurchases_ErrorGettingPlans(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -3337,7 +3369,10 @@ func TestHandler_executePurchase_DirectExec_ExecuteAny(t *testing.T) { mockAuth.On("HasPermissionAPI", ctx, adminSession.UserID, "execute-any", "purchases").Return(true, nil) // Scope check: no allowed_accounts restriction for this test. mockAuth.On("GetAllowedAccountsAPI", ctx, adminSession.UserID).Return([]string{}, nil) - mockPurchase.On("ApproveAndExecute", ctx, mock.AnythingOfType("string"), adminSession.Email).Return(nil) + // Direct-execute is a human session action: the transitioned_by actor + // must be the session user's UUID, not nil (issue #1009 audit objective). + mockPurchase.On("ApproveAndExecute", ctx, mock.AnythingOfType("string"), adminSession.Email, + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == adminSession.UserID })).Return(nil) setupDirectExecMocks(ctx, mockStore) handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} @@ -3375,7 +3410,10 @@ func TestHandler_executePurchase_DirectExec_ExecuteOwn_Owner(t *testing.T) { 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) - mockPurchase.On("ApproveAndExecute", ctx, mock.AnythingOfType("string"), ownerSession.Email).Return(nil) + // Direct-execute is a human session action: the transitioned_by actor + // must be the session user's UUID, not nil (issue #1009 audit objective). + mockPurchase.On("ApproveAndExecute", ctx, mock.AnythingOfType("string"), ownerSession.Email, + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == ownerID })).Return(nil) setupDirectExecMocks(ctx, mockStore) handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} @@ -3532,7 +3570,7 @@ func TestHandler_pausePlannedPurchase_NonOwner_Rejected(t *testing.T) { require.True(t, ok) assert.Equal(t, 403, ce.code) assert.Contains(t, ce.message, "another user's scheduled purchase") - mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockAuth.AssertExpectations(t) } @@ -3544,7 +3582,7 @@ func TestHandler_resumePlannedPurchase_NonOwner_Rejected(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 403, ce.code) - mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } func TestHandler_deletePlannedPurchase_NonOwner_Rejected(t *testing.T) { @@ -3555,7 +3593,7 @@ func TestHandler_deletePlannedPurchase_NonOwner_Rejected(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 403, ce.code) - mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } func TestHandler_runPlannedPurchase_NonOwner_Rejected(t *testing.T) { @@ -3566,26 +3604,26 @@ func TestHandler_runPlannedPurchase_NonOwner_Rejected(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 403, ce.code) - mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestHandler_pausePlannedPurchase_Owner_Allowed: user A manages their OWN P1. func TestHandler_pausePlannedPurchase_Owner_Allowed(t *testing.T) { handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserA, false) - mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused"). + mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "paused"}, nil) res, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID) require.NoError(t, err) assert.Equal(t, "paused", res.Status) - mockConfig.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused") + mockConfig.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused", mock.Anything) } // TestHandler_pausePlannedPurchase_UpdateAny_AllowsAny: a privileged user with // update-any:purchases manages P2 created by user B. func TestHandler_pausePlannedPurchase_UpdateAny_AllowsAny(t *testing.T) { handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, true) - mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused"). + mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "paused"}, nil) res, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID) @@ -3794,12 +3832,12 @@ func TestHandler_scheduleApprovedExecution_CASGuardsConcurrentCancel(t *testing. mockConfig := new(MockConfigStore) // TransitionExecutionStatus fails because a concurrent Cancel already landed. - mockConfig.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "scheduled"). + mockConfig.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "scheduled", mock.Anything). Return(nil, concurrentCancelErr) handler := &Handler{config: mockConfig} - _, err := handler.scheduleApprovedExecution(ctx, exec, 48*time.Hour, "actor@example.com") + _, err := handler.scheduleApprovedExecution(ctx, exec, 48*time.Hour, "actor@example.com", nil) require.Error(t, err, "concurrent cancel must surface as an error, not a silent overwrite") // SavePurchaseExecution must NEVER be called: the cancelled row is untouched. mockConfig.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) @@ -3825,7 +3863,7 @@ func TestHandler_scheduleApprovedExecution_HappyPath(t *testing.T) { } mockConfig := new(MockConfigStore) - mockConfig.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "scheduled"). + mockConfig.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "scheduled", mock.Anything). Return(transitioned, nil) mockConfig.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { return e.ExecutionID == execID && @@ -3835,7 +3873,7 @@ func TestHandler_scheduleApprovedExecution_HappyPath(t *testing.T) { handler := &Handler{config: mockConfig} - result, err := handler.scheduleApprovedExecution(ctx, exec, 48*time.Hour, "actor@example.com") + result, err := handler.scheduleApprovedExecution(ctx, exec, 48*time.Hour, "actor@example.com", nil) require.NoError(t, err) assert.Equal(t, "scheduled", result.Status) assert.NotNil(t, result.ScheduledExecutionAt, "ScheduledExecutionAt must be stamped") @@ -3854,12 +3892,12 @@ func TestApproveWithDelay_CASLostMaps409(t *testing.T) { config.ErrExecutionNotInExpectedStatus, execID) mockConfig := new(MockConfigStore) - mockConfig.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "scheduled"). + mockConfig.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "scheduled", mock.Anything). Return(nil, concurrentCancelErr) handler := &Handler{config: mockConfig} - _, err := handler.approveWithDelay(ctx, exec, 48*time.Hour, "actor@example.com") + _, err := handler.approveWithDelay(ctx, exec, 48*time.Hour, "actor@example.com", nil) require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok, "CAS-lost error must be a ClientError") diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index a9e041216..7969cf1fb 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -263,7 +263,8 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd // Atomically transition to "approved" first — prevents double-approval. reg.Status = "approved" h.setReviewMetadata(ctx, reg, httpReq) - if err := h.config.TransitionRegistrationStatus(ctx, reg, "pending"); err != nil { + // reviewed_by may be "admin-api-key" (not a UUID FK); validate before passing as actor. + if err := h.config.TransitionRegistrationStatus(ctx, reg, "pending", validUUIDPtrOrNil(reg.ReviewedBy)); err != nil { if errors.Is(err, config.ErrRegistrationConflict) { return nil, NewClientError(409, "registration was already processed by another request") } @@ -352,7 +353,8 @@ func (h *Handler) rejectRegistration(ctx context.Context, httpReq *events.Lambda reg.Status = "rejected" reg.RejectionReason = body.Reason h.setReviewMetadata(ctx, reg, httpReq) - if err := h.config.TransitionRegistrationStatus(ctx, reg, "pending"); err != nil { + // reviewed_by may be "admin-api-key" (not a UUID FK); validate before passing as actor. + if err := h.config.TransitionRegistrationStatus(ctx, reg, "pending", validUUIDPtrOrNil(reg.ReviewedBy)); err != nil { if errors.Is(err, config.ErrRegistrationConflict) { return nil, NewClientError(409, "registration was already processed by another request") } diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 99dc800c9..2f4d72250 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1038,7 +1038,8 @@ func (h *Handler) approveRIExchangeViaToken(ctx context.Context, id, token strin return nil, err } - transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing") + // Token-based approval: no session user, so transitioned_by = NULL. + transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing", nil) if err != nil { return nil, fmt.Errorf("failed to transition exchange status: %w", err) } @@ -1070,7 +1071,8 @@ func (h *Handler) approveRIExchangeViaSession(ctx context.Context, req *events.L return nil, err } - transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing") + // Session-authed approval: stamp the session user as the actor. + transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing", resolveCreatorUserID(session)) if err != nil { return nil, fmt.Errorf("failed to transition exchange status: %w", err) } @@ -1326,7 +1328,8 @@ func (h *Handler) rejectRIExchange(ctx context.Context, id, token string) (any, return nil, NewClientError(403, "invalid rejection token") } - transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "cancelled") + // Token-based rejection: no session user, so transitioned_by = NULL. + transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "cancelled", nil) if err != nil { return nil, fmt.Errorf("failed to transition exchange status: %w", err) } diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index b93074866..250180c49 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -263,7 +263,7 @@ func TestRejectRIExchange_AlreadyCompleted(t *testing.T) { }, nil) // Transition from pending→cancelled fails (record is not pending) - mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "cancelled"). + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "cancelled", mock.Anything). Return((*config.RIExchangeRecord)(nil), nil) _, err := h.rejectRIExchange(ctx, id, token) @@ -291,7 +291,7 @@ func TestApproveRIExchange_AlreadyCancelled(t *testing.T) { }, nil) // Transition from pending→processing fails (record is cancelled) - mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything). Return((*config.RIExchangeRecord)(nil), nil) _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, token) @@ -321,7 +321,7 @@ func TestApproveRIExchange_DoubleApprove(t *testing.T) { }, nil) // Transition from pending→processing fails (already processing) - mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything). Return((*config.RIExchangeRecord)(nil), nil) _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, token) @@ -380,7 +380,7 @@ func TestApproveRIExchange_SessionAdmin(t *testing.T) { CreatedByUserID: &creatorID, }, nil).Once() - mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything). Return(&config.RIExchangeRecord{ID: id, Status: "processing", SourceRIIDs: []string{"ri-123"}, PaymentDue: "100.00"}, nil) // executeApprovedExchange calls GetRIExchangeDailySpend + GetGlobalConfig mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil) @@ -402,7 +402,7 @@ func TestApproveRIExchange_SessionAdmin(t *testing.T) { // The exchange execution will fail (no real AWS SDK) but we verify the // dispatch reached the session-authed path. mockStore.AssertCalled(t, "GetRIExchangeRecord", ctx, id) - mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing") + mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything) mockStore.AssertCalled(t, "FailRIExchange", ctx, id, mock.AnythingOfType("string")) mockStore.AssertCalled(t, "StampRIExchangeApprovedBy", ctx, id, adminSession.Email) } @@ -435,7 +435,7 @@ func TestApproveRIExchange_SessionApproveOwn(t *testing.T) { CreatedByUserID: &ownerID, }, nil) - mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). + 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("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ @@ -450,7 +450,7 @@ func TestApproveRIExchange_SessionApproveOwn(t *testing.T) { } _, err := h.approveRIExchange(ctx, req, id, "") require.NoError(t, err) - mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing") + mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything) }) t.Run("other user exchange rejected", func(t *testing.T) { @@ -499,7 +499,7 @@ func TestApproveRIExchange_LegacyTokenStillWorks(t *testing.T) { SourceRIIDs: []string{"ri-1"}, PaymentDue: "10.00", }, nil) - mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything). Return(&config.RIExchangeRecord{ID: id, Status: "processing"}, nil) mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ @@ -511,7 +511,7 @@ func TestApproveRIExchange_LegacyTokenStillWorks(t *testing.T) { req := &events.LambdaFunctionURLRequest{} _, err := h.approveRIExchange(ctx, req, id, token) require.NoError(t, err) - mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing") + mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything) } func TestRejectRIExchange_MissingToken(t *testing.T) { @@ -1395,3 +1395,64 @@ func TestClassifyRecsAge(t *testing.T) { }) } } + +// --- Audit actor stamping tests (issue #1009) --- + +// TestApproveRIExchange_SessionActorStamped asserts that the session-authed +// approval path passes the session UserID as the actor to TransitionRIExchangeStatus +// so that transitioned_by is set on the ri_exchange_history row. +func TestApproveRIExchange_SessionActorStamped(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + const actorID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + const id = "550e8400-e29b-41d4-a716-446655441001" + + adminSession := &Session{UserID: actorID, Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-bearer").Return(adminSession, nil) + mockAuth.grantAdmin() + + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, Status: "pending", ApprovalToken: "tok", SourceRIIDs: []string{"ri-1"}, PaymentDue: "10.00", + }, nil) + // Actor must be the session UserID. + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", + mock.MatchedBy(func(a *string) bool { return a != nil && *a == actorID }), + ).Return(&config.RIExchangeRecord{ID: id, Status: "processing", SourceRIIDs: []string{"ri-1"}, PaymentDue: "10.00"}, nil) + mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RIExchangeMaxDailyUSD: 1000, RIExchangeMaxPerExchangeUSD: 500, + }, nil) + mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil) + mockStore.On("StampRIExchangeApprovedBy", ctx, id, adminSession.Email).Return(nil) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer admin-bearer"}, + } + _, err := (&Handler{config: mockStore, auth: mockAuth}).approveRIExchange(ctx, req, id, "") + require.NoError(t, err) +} + +// TestRejectRIExchange_TokenPathActorIsNil asserts that the token-based rejection +// path passes nil as the actor param so that transitioned_by = NULL on the row. +// Token-based paths have no session, so no human actor can be attributed. +func TestRejectRIExchange_TokenPathActorIsNil(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + const id = "550e8400-e29b-41d4-a716-446655441002" + + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, Status: "pending", ApprovalToken: "tok", + }, nil) + // Token path: actor must be nil. + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "cancelled", + (*string)(nil), + ).Return(&config.RIExchangeRecord{ID: id, Status: "cancelled"}, nil) + + _, err := (&Handler{config: mockStore}).rejectRIExchange(ctx, id, "tok") + require.NoError(t, err) +} diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 3a474b1c7..e81896455 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -1067,7 +1067,7 @@ func TestHandler_HandleRequest_PausePlannedPurchase(t *testing.T) { mockAuth.On("ValidateCSRFToken", ctx, mock.Anything, mock.Anything).Return(nil) paused := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "paused"} - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "running"}, "paused").Return(paused, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "running"}, "paused", mock.Anything).Return(paused, nil) handler := &Handler{config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"} @@ -1102,7 +1102,7 @@ func TestHandler_HandleRequest_ResumePlannedPurchase(t *testing.T) { mockAuth.On("ValidateCSRFToken", ctx, mock.Anything, mock.Anything).Return(nil) resumed := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "pending"} - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"paused"}, "pending").Return(resumed, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"paused"}, "pending", mock.Anything).Return(resumed, nil) handler := &Handler{config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"} @@ -1137,7 +1137,7 @@ func TestHandler_HandleRequest_RunPlannedPurchase(t *testing.T) { mockAuth.On("ValidateCSRFToken", ctx, mock.Anything, mock.Anything).Return(nil) transitioned := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "running"} - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "running").Return(transitioned, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "running", mock.Anything).Return(transitioned, nil) handler := &Handler{config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"} @@ -1172,7 +1172,7 @@ func TestHandler_HandleRequest_DeletePlannedPurchase(t *testing.T) { mockAuth.On("ValidateCSRFToken", ctx, mock.Anything, mock.Anything).Return(nil) cancelled := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "cancelled"} - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "cancelled").Return(cancelled, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "cancelled", mock.Anything).Return(cancelled, nil) handler := &Handler{config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"} diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 38a4e5a1f..9b4e7312f 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -317,7 +317,7 @@ func TestApproveViaSession_PassesCSRF(t *testing.T) { t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil) handler := &Handler{config: mockConfig, auth: mockAuth, purchase: mockPurchase} diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index b6828877d..372bb9f09 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -51,8 +51,8 @@ func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, toke return args.Error(0) } -func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string) error { - args := m.Called(ctx, execID, actor) +func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) error { + args := m.Called(ctx, execID, actor, transitionedBy) return args.Error(0) } diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go index 50f30e1b9..c56a0a51a 100644 --- a/internal/api/router_660_permission_flips_test.go +++ b/internal/api/router_660_permission_flips_test.go @@ -207,7 +207,7 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) { mockStore.On("GetExecutionByID", ctx, execID). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil) // TransitionExecutionStatus is called next; stub it. - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused"). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) h := &Handler{auth: mockAuth, config: mockStore} @@ -231,7 +231,7 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) { _, err := h.pausePlannedPurchase(ctx, reqWithBearer("user-token"), execID) assert403(t, err) // The status transition must never run for a non-owner. - mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) }) t.Run("update-any holder can pause another user's planned purchase (issue #950)", func(t *testing.T) { @@ -245,7 +245,7 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) { // authorizeExecutionManagement; only requireExecutionAccess fetches. mockStore.On("GetExecutionByID", ctx, execID). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil) - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused"). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) h := &Handler{auth: mockAuth, config: mockStore} @@ -266,7 +266,7 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) { // immediately without calling GetAllowedAccountsAPI or GetExecutionByID). mockAuth := authForAdmin(ctx, t) mockStore := new(MockConfigStore) - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused"). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) h := &Handler{auth: mockAuth, config: mockStore} @@ -292,7 +292,7 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { mockStore := new(MockConfigStore) mockStore.On("GetExecutionByID", ctx, execID). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil) - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled"). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "cancelled"}, nil) h := &Handler{auth: mockAuth, config: mockStore} @@ -312,7 +312,7 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { h := &Handler{auth: mockAuth, config: mockStore} _, err := h.deletePlannedPurchase(ctx, reqWithBearer("user-token"), execID) assert403(t, err) - mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) }) t.Run("user without delete:purchases is rejected with 403", func(t *testing.T) { diff --git a/internal/api/router_handlers_test.go b/internal/api/router_handlers_test.go index 068f21e62..a0d326b69 100644 --- a/internal/api/router_handlers_test.go +++ b/internal/api/router_handlers_test.go @@ -785,7 +785,7 @@ func TestHandler_rejectRIExchange_ValidTokenAndRecord(t *testing.T) { Status: "pending", }, nil) // rejectRIExchange transitions to "cancelled", not "rejected" - mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "cancelled"). + mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "cancelled", mock.Anything). Return(&config.RIExchangeRecord{ ID: "11111111-1111-1111-1111-111111111111", Status: "cancelled", diff --git a/internal/api/types.go b/internal/api/types.go index 755ed17e2..c874204d0 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -147,9 +147,13 @@ type BreakdownValue struct { // attribution via the auth-gated deep-link flow; pass "" for token-only // paths (message workers, legacy callers) where attribution falls back to // the notification email at render time. +// +// `transitionedBy` (ApproveAndExecute) is the session user's UUID stamped +// onto purchase_executions.transitioned_by for human-initiated approvals; +// pass nil for token/SQS/system flows so transitioned_by = NULL (issue #1009). type PurchaseManagerInterface interface { ApproveExecution(ctx context.Context, execID, token, actor string) error - ApproveAndExecute(ctx context.Context, execID, actor string) error + ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) error CancelExecution(ctx context.Context, execID, token, actor string) error } diff --git a/internal/api/validation.go b/internal/api/validation.go index cde323a94..a538db92e 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -387,6 +387,17 @@ func validateUUID(id string) error { return nil } +// validUUIDPtrOrNil returns p when *p is a valid UUID, or nil otherwise. +// Used to convert reviewer/creator strings (which may be "admin-api-key" +// for API-key sessions — not a real UUID) to a nullable FK-safe actor pointer +// before stamping transitioned_by on state-transition rows. +func validUUIDPtrOrNil(p *string) *string { + if p == nil || validateUUID(*p) != nil { + return nil + } + return p +} + // validateContentType checks if the Content-Type header is acceptable for the request func validateContentType(req *events.LambdaFunctionURLRequest) error { method := req.RequestContext.HTTP.Method diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 0934e6a0f..a6ba6e43d 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -77,7 +77,11 @@ type StoreInterface interface { // is a one-off operator task rather than a button click anyway. ListPendingExecutionIDsForAccount(ctx context.Context, accountID string) ([]string, error) CleanupOldExecutions(ctx context.Context, retentionDays int) (int64, error) - TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string) (*PurchaseExecution, error) + // TransitionExecutionStatus atomically transitions an execution status. + // actor is the UUID of the user performing the transition (nil for system-initiated paths). + // When non-nil the actor is stamped onto transitioned_by + transitioned_at; when nil, + // transitioned_by is set to NULL and transitioned_at is still set to NOW() for ordering. + TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string) (*PurchaseExecution, error) // CancelExecutionAtomic atomically flips status from pending / notified / // scheduled to cancelled, setting cancelled_by. The 'scheduled' status // supports the Gmail-style pre-fire delay revoke path (issue #290). @@ -184,7 +188,9 @@ type StoreInterface interface { GetRIExchangeRecord(ctx context.Context, id string) (*RIExchangeRecord, error) GetRIExchangeRecordByToken(ctx context.Context, token string) (*RIExchangeRecord, error) GetRIExchangeHistory(ctx context.Context, since time.Time, limit int) ([]RIExchangeRecord, error) - TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string) (*RIExchangeRecord, error) + // TransitionRIExchangeStatus atomically transitions an RI exchange record status. + // actor is the UUID of the user performing the transition (nil for system-initiated paths). + TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string, actor *string) (*RIExchangeRecord, error) CompleteRIExchange(ctx context.Context, id string, exchangeID string) error // StampRIExchangeApprovedBy sets the approved_by column on a completed // exchange row (issue #300). Called after CompleteRIExchange when the @@ -269,7 +275,9 @@ type StoreInterface interface { GetAccountRegistrationByToken(ctx context.Context, token string) (*AccountRegistration, error) ListAccountRegistrations(ctx context.Context, filter AccountRegistrationFilter) ([]AccountRegistration, error) UpdateAccountRegistration(ctx context.Context, reg *AccountRegistration) error - TransitionRegistrationStatus(ctx context.Context, reg *AccountRegistration, fromStatus string) error + // TransitionRegistrationStatus atomically updates a registration's workflow fields. + // actor is the UUID of the reviewer (nil for system-initiated transitions). + TransitionRegistrationStatus(ctx context.Context, reg *AccountRegistration, fromStatus string, actor *string) error DeleteAccountRegistration(ctx context.Context, id string) error // Purchase suppressions. Written inside a WithTx block during bulk diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 1299ef493..e9c2d7bf8 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -891,10 +891,13 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, // TransitionExecutionStatus atomically transitions an execution from one of the // allowed statuses to a new status. Returns the updated record, or an error if // the execution was not found or not in an allowed status. -func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string) (*PurchaseExecution, error) { +// actor is the UUID of the user performing the transition (nil for system-initiated paths); +// it is stamped onto transitioned_by and transitioned_at is always set to NOW(). +func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string) (*PurchaseExecution, error) { query := ` UPDATE purchase_executions - SET status = $2, updated_at = NOW() + SET status = $2, updated_at = NOW(), + transitioned_by = $4, transitioned_at = NOW() WHERE execution_id = $1 AND status = ANY($3) RETURNING plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, @@ -906,7 +909,7 @@ func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, execution idempotency_key, scheduled_execution_at ` - records, err := s.queryExecutions(ctx, query, executionID, toStatus, fromStatuses) + records, err := s.queryExecutions(ctx, query, executionID, toStatus, fromStatuses, actor) if err != nil { return nil, err } @@ -2225,10 +2228,12 @@ func (s *PostgresStore) GetRIExchangeHistory(ctx context.Context, since time.Tim // TransitionRIExchangeStatus atomically transitions an RI exchange record status. // Uses a single UPDATE...WHERE...RETURNING for atomicity, then diagnoses failure // only if zero rows are returned. -func (s *PostgresStore) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string) (*RIExchangeRecord, error) { +// actor is the UUID of the user performing the transition (nil for system-initiated paths). +func (s *PostgresStore) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string, actor *string) (*RIExchangeRecord, error) { query := ` UPDATE ri_exchange_history - SET status = $3, updated_at = NOW() + SET status = $3, updated_at = NOW(), + transitioned_by = $4, transitioned_at = NOW() WHERE id = $1 AND status = $2 AND (expires_at IS NULL OR expires_at > NOW()) RETURNING id, account_id, exchange_id, region, source_ri_ids, source_instance_type, source_count, target_offering_id, @@ -2238,7 +2243,7 @@ func (s *PostgresStore) TransitionRIExchangeStatus(ctx context.Context, id strin created_by_user_id, approved_by ` - records, err := s.queryRIExchangeRecords(ctx, query, id, fromStatus, toStatus) + records, err := s.queryRIExchangeRecords(ctx, query, id, fromStatus, toStatus, actor) if err != nil { return nil, err } diff --git a/internal/config/store_postgres_nil_db_test.go b/internal/config/store_postgres_nil_db_test.go index 9773bdadf..152c05919 100644 --- a/internal/config/store_postgres_nil_db_test.go +++ b/internal/config/store_postgres_nil_db_test.go @@ -99,7 +99,7 @@ func TestPostgresStore_TransitionRIExchangeStatus_NilDB(t *testing.T) { ctx := context.Background() panicked := callWithRecover(func() { - _, _ = store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing") + _, _ = store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) }) assert.True(t, panicked, "expected panic with nil db connection") diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 179354662..1af3964d7 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -942,13 +942,13 @@ func TestPGXMock_TransitionRIExchangeStatus_WrongStatus(t *testing.T) { // UPDATE returns empty (status mismatch) → triggers diagnostic SELECT emptyRows := pgxmock.NewRows(riExchangeCols) - mock.ExpectQuery("UPDATE").WithArgs(anyArgsCfg(3)...).WillReturnRows(emptyRows) + mock.ExpectQuery("UPDATE").WithArgs(anyArgsCfg(4)...).WillReturnRows(emptyRows) // Diagnostic query returns current status diagRows := pgxmock.NewRows([]string{"status", "expired"}).AddRow("completed", false) mock.ExpectQuery("SELECT status").WithArgs(pgxmock.AnyArg()).WillReturnRows(diagRows) - _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing") + _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.Error(t, err) assert.Contains(t, err.Error(), "expected status") } @@ -960,12 +960,12 @@ func TestPGXMock_TransitionRIExchangeStatus_RecordNotFound(t *testing.T) { // UPDATE returns empty (not found) → triggers diagnostic SELECT emptyRows := pgxmock.NewRows(riExchangeCols) - mock.ExpectQuery("UPDATE").WithArgs(anyArgsCfg(3)...).WillReturnRows(emptyRows) + mock.ExpectQuery("UPDATE").WithArgs(anyArgsCfg(4)...).WillReturnRows(emptyRows) // Diagnostic query returns ErrNoRows (record not found) mock.ExpectQuery("SELECT status").WithArgs(pgxmock.AnyArg()).WillReturnError(errNoRows()) - _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing") + _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.Error(t, err) assert.Contains(t, err.Error(), "not found") } @@ -977,13 +977,13 @@ func TestPGXMock_TransitionRIExchangeStatus_Expired(t *testing.T) { // UPDATE returns empty (expired) → triggers diagnostic SELECT emptyRows := pgxmock.NewRows(riExchangeCols) - mock.ExpectQuery("UPDATE").WithArgs(anyArgsCfg(3)...).WillReturnRows(emptyRows) + mock.ExpectQuery("UPDATE").WithArgs(anyArgsCfg(4)...).WillReturnRows(emptyRows) // Diagnostic query: record exists but expired diagRows := pgxmock.NewRows([]string{"status", "expired"}).AddRow("pending", true) mock.ExpectQuery("SELECT status").WithArgs(pgxmock.AnyArg()).WillReturnRows(diagRows) - _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing") + _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.Error(t, err) assert.Contains(t, err.Error(), "expired") } @@ -997,9 +997,9 @@ func TestPGXMock_TransitionRIExchangeStatus_Success(t *testing.T) { // UPDATE succeeds → returns the updated record updateRows := pgxmock.NewRows(riExchangeCols).AddRow(riExchangeRow(now)...) - mock.ExpectQuery("UPDATE").WithArgs(anyArgsCfg(3)...).WillReturnRows(updateRows) + mock.ExpectQuery("UPDATE").WithArgs(anyArgsCfg(4)...).WillReturnRows(updateRows) - rec, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing") + rec, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.NoError(t, err) assert.Equal(t, "ri-id", rec.ID) assert.NoError(t, mock.ExpectationsWereMet()) diff --git a/internal/config/store_postgres_registrations.go b/internal/config/store_postgres_registrations.go index b1ca508ed..9b15db9f3 100644 --- a/internal/config/store_postgres_registrations.go +++ b/internal/config/store_postgres_registrations.go @@ -170,7 +170,8 @@ func (s *PostgresStore) UpdateAccountRegistration(ctx context.Context, reg *Acco // TransitionRegistrationStatus atomically updates a registration's workflow fields // only if the current status matches fromStatus. Returns ErrRegistrationConflict // when 0 rows are affected (another request already changed the status). -func (s *PostgresStore) TransitionRegistrationStatus(ctx context.Context, reg *AccountRegistration, fromStatus string) error { +// actor is the UUID of the reviewer (nil for system-initiated transitions). +func (s *PostgresStore) TransitionRegistrationStatus(ctx context.Context, reg *AccountRegistration, fromStatus string, actor *string) error { reg.UpdatedAt = time.Now() query := ` @@ -180,7 +181,9 @@ func (s *PostgresStore) TransitionRegistrationStatus(ctx context.Context, reg *A cloud_account_id = $4, reviewed_by = $5, reviewed_at = $6, - updated_at = $7 + updated_at = $7, + transitioned_by = $9, + transitioned_at = NOW() WHERE id = $1 AND status = $8 ` @@ -198,6 +201,7 @@ func (s *PostgresStore) TransitionRegistrationStatus(ctx context.Context, reg *A reviewedAt, reg.UpdatedAt, fromStatus, + actor, ) if err != nil { return fmt.Errorf("failed to transition account registration status: %w", err) diff --git a/internal/database/postgres/migrations/000074_audit_actor_stamps.down.sql b/internal/database/postgres/migrations/000074_audit_actor_stamps.down.sql new file mode 100644 index 000000000..07e811f8d --- /dev/null +++ b/internal/database/postgres/migrations/000074_audit_actor_stamps.down.sql @@ -0,0 +1,14 @@ +-- Rollback migration 000074: remove audit actor stamp columns. +-- The FK constraint on transitioned_by is dropped automatically with the column. + +ALTER TABLE purchase_executions + DROP COLUMN IF EXISTS transitioned_by, + DROP COLUMN IF EXISTS transitioned_at; + +ALTER TABLE ri_exchange_history + DROP COLUMN IF EXISTS transitioned_by, + DROP COLUMN IF EXISTS transitioned_at; + +ALTER TABLE account_registrations + DROP COLUMN IF EXISTS transitioned_by, + DROP COLUMN IF EXISTS transitioned_at; diff --git a/internal/database/postgres/migrations/000074_audit_actor_stamps.up.sql b/internal/database/postgres/migrations/000074_audit_actor_stamps.up.sql new file mode 100644 index 000000000..ce61e7d16 --- /dev/null +++ b/internal/database/postgres/migrations/000074_audit_actor_stamps.up.sql @@ -0,0 +1,17 @@ +-- Migration 000074: audit actor stamps on state transitions +-- Adds transitioned_by + transitioned_at to the three financial state machines: +-- purchase_executions, ri_exchanges (ri_exchange_history), account_registrations. +-- Idempotent: uses ADD COLUMN IF NOT EXISTS throughout. +-- Existing rows receive NULL for both columns (retroactive attribution is impossible). + +ALTER TABLE purchase_executions + ADD COLUMN IF NOT EXISTS transitioned_by UUID NULL REFERENCES users(id) ON DELETE SET NULL, + ADD COLUMN IF NOT EXISTS transitioned_at TIMESTAMPTZ NULL; + +ALTER TABLE ri_exchange_history + ADD COLUMN IF NOT EXISTS transitioned_by UUID NULL REFERENCES users(id) ON DELETE SET NULL, + ADD COLUMN IF NOT EXISTS transitioned_at TIMESTAMPTZ NULL; + +ALTER TABLE account_registrations + ADD COLUMN IF NOT EXISTS transitioned_by UUID NULL REFERENCES users(id) ON DELETE SET NULL, + ADD COLUMN IF NOT EXISTS transitioned_at TIMESTAMPTZ NULL; diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index c246ade3b..e6b32b4e6 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -170,8 +170,8 @@ func (m *MockConfigStore) SavePurchaseExecution(ctx context.Context, exec *confi } // TransitionExecutionStatus mocks the TransitionExecutionStatus operation -func (m *MockConfigStore) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string) (*config.PurchaseExecution, error) { - args := m.Called(ctx, executionID, fromStatuses, toStatus) +func (m *MockConfigStore) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string) (*config.PurchaseExecution, error) { + args := m.Called(ctx, executionID, fromStatuses, toStatus, actor) if args.Get(0) == nil { return nil, args.Error(1) } @@ -379,8 +379,8 @@ func (m *MockConfigStore) GetRIExchangeHistory(ctx context.Context, since time.T return args.Get(0).([]config.RIExchangeRecord), args.Error(1) } -func (m *MockConfigStore) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string) (*config.RIExchangeRecord, error) { - args := m.Called(ctx, id, fromStatus, toStatus) +func (m *MockConfigStore) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string, actor *string) (*config.RIExchangeRecord, error) { + args := m.Called(ctx, id, fromStatus, toStatus, actor) if args.Get(0) == nil { return nil, args.Error(1) } @@ -912,8 +912,8 @@ func (m *MockConfigStore) UpdateAccountRegistration(ctx context.Context, reg *co return args.Error(0) } -func (m *MockConfigStore) TransitionRegistrationStatus(ctx context.Context, reg *config.AccountRegistration, fromStatus string) error { - args := m.Called(ctx, reg, fromStatus) +func (m *MockConfigStore) TransitionRegistrationStatus(ctx context.Context, reg *config.AccountRegistration, fromStatus string, actor *string) error { + args := m.Called(ctx, reg, fromStatus, actor) return args.Error(0) } diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index b9253f78e..826327eae 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -56,7 +56,9 @@ func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, acto return err } - err = m.ApproveAndExecute(ctx, executionID, actor) + // Token/SQS path: no authenticated session UUID is available, so the + // transition is recorded as system-initiated (transitioned_by = NULL). + err = m.ApproveAndExecute(ctx, executionID, actor, nil) if err != nil { logging.Errorf("purchase[%s]: ApproveExecution (token path) failed after %s: %v", executionID, time.Since(t0), err) @@ -140,11 +142,15 @@ func OrphanExecutionError(execution *config.PurchaseExecution) error { // transition" error. Cross-execution concurrency is unaffected: each // approval drives its own executeAndFinalize, which already fans out // per-account in parallel via executeMultiAccount. -func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor string) error { +func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor string, transitionedBy *string) error { t0 := time.Now() logging.Infof("purchase[%s]: ApproveAndExecute starting (actor=%q)", executionID, maskActor(actor)) - updated, err := m.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "notified"}, "approved") + // 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 + // email is recorded separately onto approved_by (below). + updated, err := m.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "notified"}, "approved", transitionedBy) if err != nil { logging.Errorf("purchase[%s]: ApproveAndExecute status transition failed after %s: %v", executionID, time.Since(t0), err) diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index 4189845d8..51f6c22a4 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -67,7 +67,7 @@ func TestManager_ApproveExecution_Success(t *testing.T) { } store.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "approved").Return(updated, nil) + store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "approved", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-456") err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "") @@ -94,7 +94,7 @@ func TestManager_ApproveExecution_StampsApprovedBy(t *testing.T) { } store.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "approved").Return(updated, nil) + store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "approved", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-456") err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "operator@example.com") @@ -127,7 +127,7 @@ func TestManager_ApproveExecution_NotifiedStatus(t *testing.T) { } store.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "approved").Return(updated, nil) + store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "approved", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-456") err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "") @@ -155,7 +155,7 @@ func TestManager_ApproveExecution_InvalidToken(t *testing.T) { // Critically: TransitionExecutionStatus and SavePurchaseExecution must // NOT have been called. A token-validation bypass is exactly what the // constant-time comparison guards against. - store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) store.AssertExpectations(t) } @@ -174,7 +174,7 @@ func TestManager_ApproveExecution_EmptyToken(t *testing.T) { err := manager.ApproveExecution(ctx, "exec-123", "", "") assert.Error(t, err) assert.Contains(t, err.Error(), "invalid approval token") - store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } func TestManager_ApproveExecution_NotFound(t *testing.T) { @@ -216,7 +216,7 @@ func TestManager_ApproveExecution_TransitionFails(t *testing.T) { ApprovalToken: "valid-token", } store.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "approved"). + store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "approved", (*string)(nil)). Return(nil, errors.New(`execution exec-123 cannot transition from "cancelled" to "approved"`)) err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "") @@ -245,12 +245,12 @@ func TestManager_ApproveAndExecute_EmptyPlanID(t *testing.T) { Status: "approved", ApprovalToken: "tok", } - store.On("TransitionExecutionStatus", ctx, "exec-direct-1", approveFromStatuses, "approved").Return(updated, nil) + store.On("TransitionExecutionStatus", ctx, "exec-direct-1", approveFromStatuses, "approved", (*string)(nil)).Return(updated, nil) // No GetPurchasePlan / GetPlanAccounts / UpdatePurchasePlan calls — see AssertNotCalled below. sender.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil) store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) - err := manager.ApproveAndExecute(ctx, "exec-direct-1", "operator@example.com") + err := manager.ApproveAndExecute(ctx, "exec-direct-1", "operator@example.com", nil) require.NoError(t, err) // Crucially, the empty PlanID must never reach the UUID-typed store columns. @@ -273,19 +273,26 @@ func TestManager_ApproveAndExecute_SkipsTokenCheck(t *testing.T) { // Session-authed path: ApproveAndExecute is called directly without a // token, after the caller has run RBAC. Verifies the entry point works // independently of the token branch. + // + // Also the manager-level regression guard for issue #1009: a non-nil + // transitionedBy (the session user's UUID) must be threaded into + // TransitionExecutionStatus so transitioned_by is stamped for human + // session approvals. Pre-fix ApproveAndExecute always passed nil here. ctx := context.Background() manager, store, sender := newApproveManager(t) + actorUUID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" updated := &config.PurchaseExecution{ ExecutionID: "exec-456", PlanID: "plan-789", Status: "approved", ApprovalToken: "tok", } - store.On("TransitionExecutionStatus", ctx, "exec-456", approveFromStatuses, "approved").Return(updated, nil) + store.On("TransitionExecutionStatus", ctx, "exec-456", approveFromStatuses, "approved", + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == actorUUID })).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-789") - err := manager.ApproveAndExecute(ctx, "exec-456", "session-user@example.com") + err := manager.ApproveAndExecute(ctx, "exec-456", "session-user@example.com", &actorUUID) require.NoError(t, err) require.NotNil(t, updated.ApprovedBy) assert.Equal(t, "session-user@example.com", *updated.ApprovedBy) @@ -526,7 +533,7 @@ func TestManager_ApproveExecution_ExpiredToken(t *testing.T) { assert.Contains(t, err.Error(), "expired") // Token validation passes but the expiry check fires — TransitionExecutionStatus // must never be called on an expired token. - store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) store.AssertExpectations(t) } @@ -551,7 +558,7 @@ func TestManager_ApproveExecution_ValidTokenWithinTTL(t *testing.T) { ApprovalToken: "valid-token", } store.On("GetExecutionByID", ctx, "exec-live").Return(execution, nil) - store.On("TransitionExecutionStatus", ctx, "exec-live", approveFromStatuses, "approved").Return(updated, nil) + store.On("TransitionExecutionStatus", ctx, "exec-live", approveFromStatuses, "approved", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-live") err := manager.ApproveExecution(ctx, "exec-live", "valid-token", "") @@ -580,7 +587,7 @@ func TestManager_ApproveExecution_NilExpiresAt_LegacyRow(t *testing.T) { Status: "approved", } store.On("GetExecutionByID", ctx, "exec-legacy").Return(execution, nil) - store.On("TransitionExecutionStatus", ctx, "exec-legacy", approveFromStatuses, "approved").Return(updated, nil) + store.On("TransitionExecutionStatus", ctx, "exec-legacy", approveFromStatuses, "approved", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-legacy") err := manager.ApproveExecution(ctx, "exec-legacy", "valid-token", "") @@ -614,7 +621,7 @@ func TestManager_ApproveExecution_NonAWSOrphanReturnsError(t *testing.T) { assert.Contains(t, err.Error(), "no longer exists") assert.Contains(t, err.Error(), "azure") // Execute chain must not run after the orphan guard fires. - store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) store.AssertExpectations(t) } @@ -640,7 +647,7 @@ func TestManager_ApproveExecution_AWSOrphanFallsThrough(t *testing.T) { ApprovalToken: "valid-token", } store.On("GetExecutionByID", ctx, "exec-aws-ambient").Return(execution, nil) - store.On("TransitionExecutionStatus", ctx, "exec-aws-ambient", approveFromStatuses, "approved").Return(updated, nil) + store.On("TransitionExecutionStatus", ctx, "exec-aws-ambient", approveFromStatuses, "approved", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-aws") err := manager.ApproveExecution(ctx, "exec-aws-ambient", "valid-token", "") diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index b1f9ccdcd..078145fc4 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -208,7 +208,7 @@ func TestHandleExecutePurchase_ApprovedStatus(t *testing.T) { runningExec := *exec runningExec.Status = "running" mockStore.On("TransitionExecutionStatus", ctx, "exec-approved", - []string{"approved", "pending", "notified"}, "running").Return(&runningExec, nil) + []string{"approved", "pending", "notified"}, "running", (*string)(nil)).Return(&runningExec, nil) mockStore.On("GetPurchasePlan", ctx, "plan-approved").Return(plan, nil) mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) @@ -290,7 +290,7 @@ func TestHandleExecutePurchase_SaveError(t *testing.T) { runningExec := *exec runningExec.Status = "running" mockStore.On("TransitionExecutionStatus", ctx, "exec-save-err", - []string{"approved", "pending", "notified"}, "running").Return(&runningExec, nil) + []string{"approved", "pending", "notified"}, "running", (*string)(nil)).Return(&runningExec, nil) mockStore.On("GetPurchasePlan", ctx, "plan-save-err").Return(plan, nil) mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(errors.New("save failed")) mockSTS.On("GetCallerIdentity", ctx, mock.Anything).Return(nil, errors.New("sts error")) @@ -359,7 +359,7 @@ func TestProcessMessage_ApproveHappyPath(t *testing.T) { mockStore.On("GetExecutionByID", ctx, "exec-appv").Return(exec, nil).Twice() mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) // Atomic approve transition (issue #372 fix). - mockStore.On("TransitionExecutionStatus", ctx, "exec-appv", []string{"pending", "notified"}, "approved").Return(approved, nil) + mockStore.On("TransitionExecutionStatus", ctx, "exec-appv", []string{"pending", "notified"}, "approved", (*string)(nil)).Return(approved, nil) // Synchronous execute chain: GetPurchasePlan is called with the // approved execution's PlanID — pinning the non-empty id here means a // regression that drops it (e.g. passes "" or exec.ExecutionID by diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index d52d37ea7..c6879109c 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -184,7 +184,8 @@ func (m *Manager) finalizeExecution(exec *config.PurchaseExecution, execErr erro // // On a won claim it runs executeAndFinalize and returns (true, execErr). func (m *Manager) claimAndExecute(ctx context.Context, exec *config.PurchaseExecution) (claimed bool, err error) { - updated, claimErr := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"approved", "pending", "notified"}, "running") + // System-initiated: SQS/cron executor passes nil so transitioned_by = NULL. + updated, claimErr := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"approved", "pending", "notified"}, "running", nil) if claimErr != nil { if errors.Is(claimErr, config.ErrNotFound) || errors.Is(claimErr, config.ErrExecutionNotInExpectedStatus) { // Benign CAS race-loss: another worker/redelivery already owns this @@ -330,7 +331,8 @@ func (m *Manager) claimAndRedrive(ctx context.Context, exec *config.PurchaseExec // sweeps (or a late original completion) from both calling executeAndFinalize // on the same approved row. The CAS transitions "approved" -> "running"; // only the winner proceeds. - claimed, claimErr := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"approved"}, "running") + // System-initiated: recovery sweep passes nil so transitioned_by = NULL. + claimed, claimErr := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"approved"}, "running", nil) if claimErr != nil { // ErrNotFound: row vanished between SELECT and CAS - benign race. // ErrExecutionNotInExpectedStatus: another sweep or the original run @@ -375,7 +377,8 @@ func (m *Manager) claimAndRedrive(ctx context.Context, exec *config.PurchaseExec func (m *Manager) safeFail(ctx context.Context, exec *config.PurchaseExecution) (bool, error) { logging.Errorf("Recovering stranded approved execution %s (approved but never finalized; failing it for visibility)", exec.ExecutionID) - updated, txErr := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"approved"}, "failed") + // System-initiated: stranded-execution recovery passes nil so transitioned_by = NULL. + updated, txErr := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"approved"}, "failed", nil) if txErr != nil { // ErrNotFound means the row vanished between the stale SELECT and // this CAS attempt (e.g. deleted by an operator or a concurrent diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index c1386d8c6..6d6419855 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -211,7 +211,7 @@ func TestManager_ProcessScheduledPurchases_DuePurchase(t *testing.T) { mockStore.On("GetStaleApprovedExecutions", ctx, mock.Anything).Return([]config.PurchaseExecution{}, nil) mockStore.On("GetPendingExecutions", ctx).Return(executions, nil) mockStore.On("TransitionExecutionStatus", ctx, "exec-123", - []string{"approved", "pending", "notified"}, "running").Return(&claimedExec, nil) + []string{"approved", "pending", "notified"}, "running", (*string)(nil)).Return(&claimedExec, nil) mockStore.On("GetPurchasePlan", ctx, "plan-456").Return(plan, nil).Once() mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) @@ -307,7 +307,7 @@ func TestManager_ProcessScheduledPurchases_ExecutionFails(t *testing.T) { mockStore.On("GetStaleApprovedExecutions", ctx, mock.Anything).Return([]config.PurchaseExecution{}, nil) mockStore.On("GetPendingExecutions", ctx).Return(executions, nil) mockStore.On("TransitionExecutionStatus", ctx, "exec-123", - []string{"approved", "pending", "notified"}, "running").Return(&claimedExec, nil) + []string{"approved", "pending", "notified"}, "running", (*string)(nil)).Return(&claimedExec, nil) mockStore.On("GetPurchasePlan", ctx, "plan-456").Return(nil, errors.New("plan not found")).Once() mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) // updatePlanProgress is NOT called when execution fails @@ -356,7 +356,7 @@ func TestManager_RecoverStrandedApprovals_FailsStrandedRow(t *testing.T) { mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) // The atomic transition only flips rows still in "approved". - mockStore.On("TransitionExecutionStatus", ctx, "exec-stranded", []string{"approved"}, "failed"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-stranded", []string{"approved"}, "failed", (*string)(nil)). Return(&failedRow, nil) // The explanatory error is stamped on the now-failed row. var saved *config.PurchaseExecution @@ -406,7 +406,7 @@ func TestManager_RecoverStrandedApprovals_FreshRowUntouched(t *testing.T) { assert.Equal(t, 0, recovered) mockStore.AssertExpectations(t) - mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) } @@ -459,7 +459,7 @@ func TestManager_RecoverStrandedApprovals_AWSOnlyRedrives(t *testing.T) { mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) // CAS claim: approved -> running. The re-drive proceeds only after winning this. - mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-stranded", []string{"approved"}, "running"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-stranded", []string{"approved"}, "running", (*string)(nil)). Return(&runningRow, nil) mockStore.On("GetPurchasePlan", ctx, "plan-aws-456").Return(plan, nil).Once() mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) @@ -500,8 +500,8 @@ func TestManager_RecoverStrandedApprovals_AWSOnlyRedrives(t *testing.T) { // The provider was reached: the re-drive called PurchaseCommitment exactly once. mockServiceClient.AssertCalled(t, "PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")) // The CAS claim (approved -> running) was called; "failed" transition was not. - mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, "exec-aws-stranded", []string{"approved"}, "running") - mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "failed") + mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, "exec-aws-stranded", []string{"approved"}, "running", (*string)(nil)) + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "failed", mock.Anything) } // TestManager_RecoverStrandedApprovals_AzureReservationRedrives verifies that a @@ -547,7 +547,7 @@ func TestManager_RecoverStrandedApprovals_AzureReservationRedrives(t *testing.T) mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) // CAS claim: approved -> running before re-drive. - mockStore.On("TransitionExecutionStatus", ctx, "exec-azure-res-stranded", []string{"approved"}, "running"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-azure-res-stranded", []string{"approved"}, "running", (*string)(nil)). Return(&runningRow, nil) mockStore.On("GetPurchasePlan", ctx, "plan-azure-res").Return(plan, nil).Once() mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) @@ -582,8 +582,8 @@ func TestManager_RecoverStrandedApprovals_AzureReservationRedrives(t *testing.T) // Provider was reached: re-drive called PurchaseCommitment exactly once. mockServiceClient.AssertCalled(t, "PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")) // CAS claim (approved -> running) was called; "failed" transition was not. - mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, "exec-azure-res-stranded", []string{"approved"}, "running") - mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "failed") + mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, "exec-azure-res-stranded", []string{"approved"}, "running", (*string)(nil)) + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "failed", mock.Anything) } // TestManager_RecoverStrandedApprovals_GCPRedrives verifies that a stranded GCP @@ -628,7 +628,7 @@ func TestManager_RecoverStrandedApprovals_GCPRedrives(t *testing.T) { mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) // CAS claim: approved -> running before re-drive. - mockStore.On("TransitionExecutionStatus", ctx, "exec-gcp-stranded", []string{"approved"}, "running"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-gcp-stranded", []string{"approved"}, "running", (*string)(nil)). Return(&runningRow, nil) mockStore.On("GetPurchasePlan", ctx, "plan-gcp").Return(plan, nil).Once() mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) @@ -663,8 +663,8 @@ func TestManager_RecoverStrandedApprovals_GCPRedrives(t *testing.T) { // Provider was reached: re-drive called PurchaseCommitment exactly once. mockServiceClient.AssertCalled(t, "PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")) // CAS claim (approved -> running) was called; "failed" transition was not. - mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, "exec-gcp-stranded", []string{"approved"}, "running") - mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "failed") + mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, "exec-gcp-stranded", []string{"approved"}, "running", (*string)(nil)) + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "failed", mock.Anything) } // TestManager_RecoverStrandedApprovals_MixedAWSAzureSPSafeFails verifies that a @@ -692,7 +692,7 @@ func TestManager_RecoverStrandedApprovals_MixedAWSAzureSPSafeFails(t *testing.T) mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) - mockStore.On("TransitionExecutionStatus", ctx, "exec-mixed-sp", []string{"approved"}, "failed"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-mixed-sp", []string{"approved"}, "failed", (*string)(nil)). Return(&failedRow, nil) mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) @@ -734,7 +734,7 @@ func TestManager_RecoverStrandedApprovals_AzureSavingsPlansSafeFails(t *testing. mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) - mockStore.On("TransitionExecutionStatus", ctx, "exec-azure-sp", []string{"approved"}, "failed"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-azure-sp", []string{"approved"}, "failed", (*string)(nil)). Return(&failedRow, nil) mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) @@ -775,7 +775,7 @@ func TestManager_RecoverStrandedApprovals_LegacyNoExecutionIDSafeFails(t *testin mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) - mockStore.On("TransitionExecutionStatus", ctx, "", []string{"approved"}, "failed"). + mockStore.On("TransitionExecutionStatus", ctx, "", []string{"approved"}, "failed", (*string)(nil)). Return(&failedRow, nil) mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) @@ -808,7 +808,7 @@ func TestManager_RecoverStrandedApprovals_LateCompletionNotClobbered(t *testing. mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) - mockStore.On("TransitionExecutionStatus", ctx, "exec-raced", []string{"approved"}, "failed"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-raced", []string{"approved"}, "failed", (*string)(nil)). Return(nil, errors.New("execution exec-raced cannot transition from \"completed\" to \"failed\"")) // When TransitionExecutionStatus fails the manager calls GetExecutionByID to // distinguish a race (row already left "approved") from a real store error. @@ -849,7 +849,7 @@ func TestManager_RecoverStrandedApprovals_SafeFail_ErrNotFoundIsBenign(t *testin mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) // TransitionExecutionStatus wraps "row vanished" as config.ErrNotFound. - mockStore.On("TransitionExecutionStatus", ctx, "exec-vanished", []string{"approved"}, "failed"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-vanished", []string{"approved"}, "failed", (*string)(nil)). Return(nil, fmt.Errorf("%w: execution exec-vanished", config.ErrNotFound)) manager := &Manager{config: mockStore, dashboardURL: "https://dashboard.example.com"} @@ -892,7 +892,7 @@ func TestManager_RecoverStrandedApprovals_SafeFail_ErrExecutionNotInExpectedStat mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) // TransitionExecutionStatus signals the row is in a non-approved state. - mockStore.On("TransitionExecutionStatus", ctx, "exec-already-transitioned", []string{"approved"}, "failed"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-already-transitioned", []string{"approved"}, "failed", (*string)(nil)). Return(nil, fmt.Errorf("%w: execution exec-already-transitioned status is completed", config.ErrExecutionNotInExpectedStatus)) manager := &Manager{config: mockStore, dashboardURL: "https://dashboard.example.com"} @@ -956,7 +956,7 @@ func TestManager_RecoverStrandedApprovals_AWSRedrive_PersistenceFailurePropagate mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) // CAS claim: approved -> running. This succeeds -- the row is now "running". - mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-persist-fail", []string{"approved"}, "running"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-persist-fail", []string{"approved"}, "running", (*string)(nil)). Return(&runningRow, nil) mockStore.On("GetPurchasePlan", ctx, "plan-aws-persist-456").Return(plan, nil).Once() mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) @@ -1031,7 +1031,7 @@ func TestManager_RecoverStrandedApprovals_AWSRedrive_ExecAndPersistBothFail(t *t mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) // CAS claim succeeds: the row is now "running" with no terminal state yet. - mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-both-fail", []string{"approved"}, "running"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-both-fail", []string{"approved"}, "running", (*string)(nil)). Return(&runningRow, nil) // executePurchase calls GetPurchasePlan; make it fail so execErr != nil. mockStore.On("GetPurchasePlan", ctx, "plan-aws-both-fail").Return(nil, planErr).Once() @@ -1087,7 +1087,7 @@ func TestManager_RecoverStrandedApprovals_AWSClaimLost_NoRedrive(t *testing.T) { Return([]config.PurchaseExecution{stranded}, nil) // A concurrent sweep has already claimed the row (status changed to "running"), // so our CAS (approved -> running) is rejected as ErrExecutionNotInExpectedStatus. - mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-claimed", []string{"approved"}, "running"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-claimed", []string{"approved"}, "running", (*string)(nil)). Return(nil, fmt.Errorf("%w: execution exec-aws-claimed cannot transition from \"running\" to \"running\"", config.ErrExecutionNotInExpectedStatus)) manager := &Manager{ diff --git a/internal/purchase/messages_test.go b/internal/purchase/messages_test.go index 6cae5670a..444404b9e 100644 --- a/internal/purchase/messages_test.go +++ b/internal/purchase/messages_test.go @@ -124,7 +124,7 @@ func TestManager_ProcessMessage(t *testing.T) { // and returns ErrExecutionNotInExpectedStatus — a benign skip that the // handler acks without error. mockStore.On("TransitionExecutionStatus", ctx, "exec-123", - []string{"approved", "pending", "notified"}, "running"). + []string{"approved", "pending", "notified"}, "running", (*string)(nil)). Return(nil, fmt.Errorf("%w: cancelled", config.ErrExecutionNotInExpectedStatus)) err := manager.ProcessMessage(ctx, `{"type": "execute_purchase", "execution_id": "exec-123"}`) diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index 7b38b95f4..d8f661e96 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -239,9 +239,9 @@ func TestSQSRedeliveryDoesNotDoubleExecute(t *testing.T) { running := newPending() running.Status = "running" mockStore.On("TransitionExecutionStatus", ctx, "exec-dup", - []string{"approved", "pending", "notified"}, "running").Return(running, nil).Once() + []string{"approved", "pending", "notified"}, "running", (*string)(nil)).Return(running, nil).Once() mockStore.On("TransitionExecutionStatus", ctx, "exec-dup", - []string{"approved", "pending", "notified"}, "running"). + []string{"approved", "pending", "notified"}, "running", (*string)(nil)). Return(nil, fmt.Errorf("%w: row already running", config.ErrExecutionNotInExpectedStatus)).Once() mockStore.SavePurchaseExecutionFn = func(_ context.Context, _ *config.PurchaseExecution) error { return nil } @@ -309,7 +309,7 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { running := *exec running.Status = "running" mockStore.On("TransitionExecutionStatus", ctx, "root-partial", - []string{"approved", "pending", "notified"}, "running").Return(&running, nil) + []string{"approved", "pending", "notified"}, "running", (*string)(nil)).Return(&running, nil) mockStore.On("GetPurchasePlan", ctx, "plan-x").Return(plan, nil) // GetPlanAccounts is served by the Fn hook, not a testify expectation. mockStore.GetPlanAccountsFn = func(_ context.Context, _ string) ([]config.CloudAccount, error) { diff --git a/internal/purchase/reaper.go b/internal/purchase/reaper.go index 9312a34de..9aef30b91 100644 --- a/internal/purchase/reaper.go +++ b/internal/purchase/reaper.go @@ -170,7 +170,8 @@ func (m *Manager) reapOne(ctx context.Context, exec *config.PurchaseExecution, r logging.Warnf("purchase reaper: reaping execution %s (status=%s, age>=%dm sweep_at=%s)", exec.ExecutionID, prevStatus, ageMinutes, now.UTC().Format(time.RFC3339)) - transitioned, err := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, stuckStatuses, failedStatus) + // System-initiated: reaper passes nil so transitioned_by = NULL. + transitioned, err := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, stuckStatuses, failedStatus, nil) if err != nil { // Distinguish CAS race-loss (the real executor finished between // our SELECT and CAS, so the row is no longer in diff --git a/internal/purchase/reaper_test.go b/internal/purchase/reaper_test.go index 05a5305aa..8ba07c699 100644 --- a/internal/purchase/reaper_test.go +++ b/internal/purchase/reaper_test.go @@ -48,7 +48,7 @@ func TestReapStuckExecutions_StaleApprovedFlippedToFailed(t *testing.T) { store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-A", stuckStatuses, failedStatus). + store.On("TransitionExecutionStatus", ctx, "exec-A", stuckStatuses, failedStatus, (*string)(nil)). Return(&transitioned, nil) store.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { // Canonical error message + previous-status attribution. @@ -80,7 +80,7 @@ func TestReapStuckExecutions_StaleRunningFlippedToFailed(t *testing.T) { store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-B", stuckStatuses, failedStatus). + store.On("TransitionExecutionStatus", ctx, "exec-B", stuckStatuses, failedStatus, (*string)(nil)). Return(&transitioned, nil) store.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { // "running" attribution path @@ -117,7 +117,7 @@ func TestReapStuckExecutions_YoungerThanThresholdNotTouched(t *testing.T) { assert.Equal(t, 0, result.Found) assert.Equal(t, 0, result.Reaped) store.AssertExpectations(t) - store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) } @@ -163,7 +163,7 @@ func TestReapStuckExecutions_CASRaceLostNoError(t *testing.T) { row := stuckExec("exec-race", "approved") store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-race", stuckStatuses, failedStatus). + store.On("TransitionExecutionStatus", ctx, "exec-race", stuckStatuses, failedStatus, (*string)(nil)). Return(nil, fmt.Errorf("%w: execution exec-race cannot transition from %q to %q", config.ErrExecutionNotInExpectedStatus, "completed", "failed")) // No SavePurchaseExecution expectation — we lost the race, the real @@ -194,7 +194,7 @@ func TestReapStuckExecutions_RowVanishedTreatedAsRaceLost(t *testing.T) { row := stuckExec("exec-gone", "approved") store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-gone", stuckStatuses, failedStatus). + store.On("TransitionExecutionStatus", ctx, "exec-gone", stuckStatuses, failedStatus, (*string)(nil)). Return(nil, fmt.Errorf("%w: execution exec-gone", config.ErrNotFound)) mgr := newReaperManager(store) @@ -221,7 +221,7 @@ func TestReapStuckExecutions_HardDBErrorClassifiedAsErrored(t *testing.T) { row := stuckExec("exec-dbflake", "running") store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-dbflake", stuckStatuses, failedStatus). + store.On("TransitionExecutionStatus", ctx, "exec-dbflake", stuckStatuses, failedStatus, (*string)(nil)). Return(nil, errors.New("connection refused")) mgr := newReaperManager(store) @@ -246,7 +246,7 @@ func TestReapStuckExecutions_CASReturnsNilNilTreatedAsRaceLost(t *testing.T) { row := stuckExec("exec-nilnil", "running") store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-nilnil", stuckStatuses, failedStatus). + store.On("TransitionExecutionStatus", ctx, "exec-nilnil", stuckStatuses, failedStatus, (*string)(nil)). Return(nil, nil) mgr := newReaperManager(store) @@ -274,7 +274,7 @@ func TestReapStuckExecutions_ThreeStuckRowsAllReaped(t *testing.T) { for _, r := range rows { flipped := r flipped.Status = failedStatus - store.On("TransitionExecutionStatus", ctx, r.ExecutionID, stuckStatuses, failedStatus). + store.On("TransitionExecutionStatus", ctx, r.ExecutionID, stuckStatuses, failedStatus, (*string)(nil)). Return(&flipped, nil).Once() store.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { return e.ExecutionID == r.ExecutionID && e.Status == failedStatus @@ -322,7 +322,7 @@ func TestReapStuckExecutions_SaveErrorAfterCASStillCountsAsReaped(t *testing.T) flipped.Status = failedStatus store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-saveflake", stuckStatuses, failedStatus). + store.On("TransitionExecutionStatus", ctx, "exec-saveflake", stuckStatuses, failedStatus, (*string)(nil)). Return(&flipped, nil) store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). Return(errors.New("write conflict")) diff --git a/internal/purchase/scheduled_fire.go b/internal/purchase/scheduled_fire.go index f3b088632..80dcb4af1 100644 --- a/internal/purchase/scheduled_fire.go +++ b/internal/purchase/scheduled_fire.go @@ -77,7 +77,8 @@ func (m *Manager) fireOneDue(ctx context.Context, exec *config.PurchaseExecution // CAS: scheduled -> approved. If this fails with ErrExecutionNotInExpectedStatus // the revoke handler already transitioned the row to "cancelled" — that is // not an error, just a CAS race loss. - updated, err := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"scheduled"}, "approved") + // Scheduler-initiated fire: no human session UUID, so transitioned_by = NULL. + updated, err := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"scheduled"}, "approved", nil) if err != nil { if errors.Is(err, config.ErrExecutionNotInExpectedStatus) || errors.Is(err, config.ErrNotFound) { logging.Infof("fireOneDue[%s]: CAS lost (execution already transitioned by another actor)", exec.ExecutionID) diff --git a/internal/purchase/scheduled_fire_test.go b/internal/purchase/scheduled_fire_test.go index a4564d81b..184707f35 100644 --- a/internal/purchase/scheduled_fire_test.go +++ b/internal/purchase/scheduled_fire_test.go @@ -79,7 +79,7 @@ func TestFireScheduledDelayedPurchases_CASLostToRevokeClassifiedAsRaceLost(t *te row := dueExec("exec-revoked") store.On("GetScheduledExecutionsDue", ctx). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-revoked", []string{"scheduled"}, "approved"). + store.On("TransitionExecutionStatus", ctx, "exec-revoked", []string{"scheduled"}, "approved", (*string)(nil)). Return(nil, fmt.Errorf("%w: execution exec-revoked cannot transition from %q to %q", config.ErrExecutionNotInExpectedStatus, "cancelled", "approved")) @@ -106,7 +106,7 @@ func TestFireScheduledDelayedPurchases_RowVanishedTreatedAsRaceLost(t *testing.T row := dueExec("exec-gone") store.On("GetScheduledExecutionsDue", ctx). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-gone", []string{"scheduled"}, "approved"). + store.On("TransitionExecutionStatus", ctx, "exec-gone", []string{"scheduled"}, "approved", (*string)(nil)). Return(nil, fmt.Errorf("%w: execution exec-gone", config.ErrNotFound)) mgr := newFireManager(store) @@ -130,7 +130,7 @@ func TestFireScheduledDelayedPurchases_HardDBErrorClassifiedAsErrored(t *testing row := dueExec("exec-dberr") store.On("GetScheduledExecutionsDue", ctx). Return([]config.PurchaseExecution{row}, nil) - store.On("TransitionExecutionStatus", ctx, "exec-dberr", []string{"scheduled"}, "approved"). + store.On("TransitionExecutionStatus", ctx, "exec-dberr", []string{"scheduled"}, "approved", (*string)(nil)). Return(nil, fmt.Errorf("connection reset by peer")) mgr := newFireManager(store) diff --git a/internal/server/interfaces.go b/internal/server/interfaces.go index 72ac7cd98..da764d463 100644 --- a/internal/server/interfaces.go +++ b/internal/server/interfaces.go @@ -26,7 +26,7 @@ type PurchaseManagerInterface interface { SendUpcomingPurchaseNotifications(ctx context.Context) (*purchase.NotificationResult, error) ProcessMessage(ctx context.Context, body string) error ApproveExecution(ctx context.Context, execID, token, actor string) error - ApproveAndExecute(ctx context.Context, execID, actor string) error + ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) error CancelExecution(ctx context.Context, execID, token, actor string) error // ReapStuckExecutions sweeps purchase_executions stuck in // approved/running longer than reapAfter and flips them to "failed" diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index bfbd73916..5400f2d47 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -123,7 +123,7 @@ func (m *mockConfigStoreForHealth) ListPendingExecutionIDsForAccount(ctx context return nil, nil } -func (m *mockConfigStoreForHealth) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string) (*config.PurchaseExecution, error) { +func (m *mockConfigStoreForHealth) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string) (*config.PurchaseExecution, error) { return nil, nil } @@ -151,7 +151,7 @@ func (m *mockConfigStoreForHealth) GetRIExchangeRecordByToken(ctx context.Contex func (m *mockConfigStoreForHealth) GetRIExchangeHistory(ctx context.Context, since time.Time, limit int) ([]config.RIExchangeRecord, error) { return nil, nil } -func (m *mockConfigStoreForHealth) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string) (*config.RIExchangeRecord, error) { +func (m *mockConfigStoreForHealth) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string, actor *string) (*config.RIExchangeRecord, error) { return nil, nil } func (m *mockConfigStoreForHealth) CompleteRIExchange(ctx context.Context, id string, exchangeID string) error { @@ -236,7 +236,7 @@ func (m *mockConfigStoreForHealth) ListAccountRegistrations(_ context.Context, _ func (m *mockConfigStoreForHealth) UpdateAccountRegistration(_ context.Context, _ *config.AccountRegistration) error { return nil } -func (m *mockConfigStoreForHealth) TransitionRegistrationStatus(_ context.Context, _ *config.AccountRegistration, _ string) error { +func (m *mockConfigStoreForHealth) TransitionRegistrationStatus(_ context.Context, _ *config.AccountRegistration, _ string, _ *string) error { return nil } func (m *mockConfigStoreForHealth) DeleteAccountRegistration(_ context.Context, _ string) error { diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index f329824f5..0ab05e2c1 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -43,7 +43,7 @@ type MockPurchaseManager struct { SendUpcomingPurchaseNotificationsFunc func(ctx context.Context) (*purchase.NotificationResult, error) ProcessMessageFunc func(ctx context.Context, body string) error ApproveExecutionFunc func(ctx context.Context, execID, token, actor string) error - ApproveAndExecuteFunc func(ctx context.Context, execID, actor string) error + ApproveAndExecuteFunc func(ctx context.Context, execID, actor string, transitionedBy *string) error CancelExecutionFunc func(ctx context.Context, execID, token, actor string) error ReapStuckExecutionsFunc func(ctx context.Context, reapAfter time.Duration) (*purchase.ReapResult, error) FireScheduledDelayedPurchasesFunc func(ctx context.Context) (*purchase.FireResult, error) @@ -78,9 +78,9 @@ func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, toke return nil } -func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string) error { +func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) error { if m.ApproveAndExecuteFunc != nil { - return m.ApproveAndExecuteFunc(ctx, execID, actor) + return m.ApproveAndExecuteFunc(ctx, execID, actor, transitionedBy) } return nil }