diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index adcd046eb..243d8a389 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -279,6 +279,10 @@ func (m *mockConfigStore) TransitionExecutionStatus(ctx context.Context, executi return nil, nil } +func (m *mockConfigStore) SetCancelledBy(_ context.Context, _ string, _ string) error { + return nil +} + func (m *mockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) { return false, "", nil } diff --git a/internal/api/coverage_gaps_test.go b/internal/api/coverage_gaps_test.go index b2ebf6206..984c6e886 100644 --- a/internal/api/coverage_gaps_test.go +++ b/internal/api/coverage_gaps_test.go @@ -929,6 +929,9 @@ func (s *stubEmailNotifier) SendPurchaseApprovalRequest(_ context.Context, _ ema func (s *stubEmailNotifier) SendPurchaseScheduledNotification(_ context.Context, _ email.NotificationData) error { return nil } +func (s *stubEmailNotifier) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { + return nil +} func (s *stubEmailNotifier) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error { return nil } diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go new file mode 100644 index 000000000..136ffaac9 --- /dev/null +++ b/internal/api/executed_notification_flow_test.go @@ -0,0 +1,343 @@ +package api + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/internal/email" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// recordingExecutedNotifier captures the NotificationData passed to +// SendPurchaseExecutedNotification so the execution-flow tests can assert the +// post-execution notification (issue #291) fires with the expected recipients +// and body fingerprints. All other SenderInterface methods are no-ops via the +// embedded stubEmailNotifier. +type recordingExecutedNotifier struct { + stubEmailNotifier + calls int + captured email.NotificationData +} + +func (r *recordingExecutedNotifier) SendPurchaseExecutedNotification(_ context.Context, data email.NotificationData) error { + r.calls++ + r.captured = data + return nil +} + +// assertExecutedNotificationFingerprints asserts the shared invariants of the +// post-execution notification across all three execution paths: it fired +// exactly once, resolved the per-account contact email as the primary To, and +// carried the expected revocation token + the executor in the body. +// +// For the token-authed approve path, expectedToken is the fresh revocation +// token minted by mintRevocationToken (obtained from the re-fetched execution). +// For the session-approve and direct-execute paths, it is the original +// approval token (those paths do not rotate the token). +func assertExecutedNotificationFingerprints(t *testing.T, n *recordingExecutedNotifier, contact, executedBy, expectedToken string) { + t.Helper() + require.Equal(t, 1, n.calls, "SendPurchaseExecutedNotification must fire exactly once") + assert.Equal(t, contact, n.captured.RecipientEmail, + "primary To must be the per-account contact email") + assert.Equal(t, executedBy, n.captured.ExecutedBy, + "body must record the executing actor") + assert.Equal(t, expectedToken, n.captured.RevocationToken, + "email must carry the expected revocation token") + assert.NotEmpty(t, n.captured.ExecutedAt, "executed-at timestamp must be set") +} + +// TestExecutedNotification_TokenApprovePath covers the email one-click +// (token-authed) approve branch of approvePurchase: after ApproveExecution +// succeeds, sendPurchaseExecutedEmail must fire with the fresh revocation +// token from the re-fetched execution, not the stale pre-approve token. +// +// This is the regression test for the defect where approveViaToken passed +// the stale pre-approve execution struct to sendPurchaseExecutedEmail. +// mintRevocationToken (called inside ApproveExecution) had already overwritten +// ApprovalToken in the DB, so the email embedded the old consumed token which +// validateRevokeToken rejected with 403 on every revoke attempt. +// +// Fail-before: without the re-fetch, GetExecutionByID is called only once +// (the second .Once() expectation goes unconsumed), the email carries the +// stale "valid-token", and the RevocationToken assertion fails. +// Pass-after: approveViaToken re-fetches, both .Once() expectations are +// consumed, and the email carries the fresh "fresh-revoke-token". +func TestExecutedNotification_TokenApprovePath(t *testing.T) { + ctx := context.Background() + execID := "12345678-1234-1234-1234-123456789abc" + contact := "contact@example.com" + freshToken := "fresh-revoke-token" + accountID := "acct-1" + recentCompleted := time.Now().Add(-1 * time.Minute) + + // pre-approve: pending execution with the original approval token. + mockConfig := new(MockConfigStore) + exec := approvalTestExec(execID, contact, mockConfig) + + // post-approve: completed execution with the fresh revocation token written + // by mintRevocationToken. Recommendations must be present so + // gatherAccountContactEmails can resolve the contact email via GetCloudAccountFn. + freshExec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: freshToken, + Status: "completed", + CompletedAt: &recentCompleted, + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + + // First call: loadApproveExecution fetches the pending execution. + // Second call: approveViaToken re-fetches after ApproveExecution returns to + // pick up the fresh revocation token written by mintRevocationToken. + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil).Once() + mockConfig.On("GetExecutionByID", ctx, execID).Return(freshExec, nil).Once() + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + NotificationEmail: &contact, + }, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) + mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() + + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(nil) + + notifier := &recordingExecutedNotifier{} + handler := &Handler{ + purchase: mockPurchase, + config: mockConfig, + auth: mockAuth, + emailNotifier: notifier, + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + result, err := handler.approvePurchase(ctx, req, execID, "valid-token") + require.NoError(t, err) + assert.Equal(t, "completed", result.(map[string]string)["status"]) + + // The email must carry the fresh revocation token from the re-fetched row, + // not the stale "valid-token" from the pre-approve struct. + assertExecutedNotificationFingerprints(t, notifier, contact, contact, freshToken) + + // Confirm the fresh token is actually valid for revocation: validateRevokeToken + // against the post-approve execution must succeed. This guards the end-to-end + // scenario: recipient clicks "Revoke" in the email -> token validates -> 200. + require.NoError(t, validateRevokeToken(freshExec, freshToken), + "the token embedded in the email must pass validateRevokeToken on the post-approve execution") + + mockPurchase.AssertExpectations(t) + mockConfig.AssertExpectations(t) +} + +// TestExecutedNotification_TokenApprovePath_RefetchFailureSuppressesPanel is the +// regression test for the degraded-path nit: when the post-approve re-fetch +// fails, approveViaToken must NOT email the stale pre-approve execution struct +// (which still carries the OLD approval token that mintRevocationToken has +// already replaced in the DB -- that token would 403 on every Revoke click, +// resurrecting the original defect). Instead the email must carry an EMPTY +// RevocationToken so the email template's {{if .RevocationToken}} suppresses the +// Revoke panel entirely (no broken button). +func TestExecutedNotification_TokenApprovePath_RefetchFailureSuppressesPanel(t *testing.T) { + ctx := context.Background() + execID := "12345678-1234-1234-1234-123456789abc" + contact := "contact@example.com" + + mockConfig := new(MockConfigStore) + exec := approvalTestExec(execID, contact, mockConfig) + + // First call: loadApproveExecution fetches the pending execution. + // Second call: approveViaToken re-fetches after ApproveExecution returns, + // but this time the store errors -- the fresh token cannot be obtained. + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil).Once() + mockConfig.On("GetExecutionByID", ctx, execID). + Return(nil, errors.New("transient store failure")).Once() + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + NotificationEmail: &contact, + }, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) + mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() + + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(nil) + + notifier := &recordingExecutedNotifier{} + handler := &Handler{ + purchase: mockPurchase, + config: mockConfig, + auth: mockAuth, + emailNotifier: notifier, + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + result, err := handler.approvePurchase(ctx, req, execID, "valid-token") + require.NoError(t, err, "approve must still succeed even when the email re-fetch fails") + assert.Equal(t, "completed", result.(map[string]string)["status"]) + + // The notification still fires (best-effort), but with an EMPTY revocation + // token so the Revoke panel is suppressed rather than showing a broken button. + require.Equal(t, 1, notifier.calls, "notification must still fire on the degraded path") + assert.Empty(t, notifier.captured.RevocationToken, + "re-fetch failure must blank the revocation token (suppress panel), never email the stale token") + + // The stale pre-approve struct must be untouched: blanking happens on a COPY. + assert.Equal(t, "valid-token", exec.ApprovalToken, + "the fallback must blank a COPY, not mutate the caller's execution struct") + + mockPurchase.AssertExpectations(t) + mockConfig.AssertExpectations(t) +} + +// TestExecutedNotification_SessionApprovePath covers the dashboard +// (session-authed) approve branch via approvePurchaseViaSession: after +// ApproveAndExecute succeeds, the notification must fire. +func TestExecutedNotification_SessionApprovePath(t *testing.T) { + ctx := context.Background() + execID := "23456789-2345-2345-2345-23456789abcd" + adminEmail := "admin@example.com" + contact := "contact@example.com" + + mockConfig := new(MockConfigStore) + exec := approvalTestExec(execID, contact, mockConfig) + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + NotificationEmail: &contact, + }, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) + mockAuth.grantAdmin() + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil) + + notifier := &recordingExecutedNotifier{} + handler := &Handler{ + purchase: mockPurchase, + config: mockConfig, + auth: mockAuth, + emailNotifier: notifier, + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + // Empty token forces the dashboard (session) branch. + result, err := handler.approvePurchase(ctx, req, execID, "") + require.NoError(t, err) + assert.Equal(t, "completed", result.(map[string]string)["status"]) + + // Admin approved, so the executor recorded in the body is the admin. + assertExecutedNotificationFingerprints(t, notifier, contact, adminEmail, "valid-token") + mockPurchase.AssertExpectations(t) + mockPurchase.AssertNotCalled(t, "ApproveExecution") +} + +// TestExecutedNotification_DirectExecutePath is the regression test for the +// adversarial-verification blocker: the direct-execute path (issue #289) sent +// NO notification at all before the #291 wiring. After ApproveAndExecute +// succeeds, directExecutePurchase must fire the post-execution notification -- +// this is the only email the direct-execute flow emits. +func TestExecutedNotification_DirectExecutePath(t *testing.T) { + ctx := context.Background() + execID := "34567890-3456-3456-3456-34567890abcd" + adminEmail := "admin@example.com" + contact := "contact@example.com" + accountID := "acct-1" + + mockConfig := new(MockConfigStore) + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "valid-token", + Status: "pending", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + mockConfig.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contact}, nil + } + // directExecutePurchase stamps audit fields, then sendPurchaseExecutedEmail + // reads the global config. + mockConfig.On("SavePurchaseExecution", ctx, exec).Return(nil) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + NotificationEmail: &contact, + }, nil) + + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil) + + notifier := &recordingExecutedNotifier{} + handler := &Handler{ + purchase: mockPurchase, + config: mockConfig, + emailNotifier: notifier, + // auth left nil: lookupRequesterInfo tolerates a nil auth and the + // execution has no CreatedByUserID, so the requester lookup is skipped. + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + session := &Session{Email: adminEmail, UserID: "admin-uid"} + + result, err := handler.directExecutePurchase(ctx, req, exec, session) + require.NoError(t, err) + resultMap := result.(map[string]any) + assert.Equal(t, "completed", resultMap["status"]) + assert.Equal(t, true, resultMap["direct_execute"]) + + assertExecutedNotificationFingerprints(t, notifier, contact, adminEmail, "valid-token") + mockPurchase.AssertExpectations(t) +} + +// TestExecutedNotification_DirectExecute_NilNotifierNoPanic guards the +// best-effort contract: a direct-execute with no email notifier configured +// must still complete the purchase without panicking. +func TestExecutedNotification_DirectExecute_NilNotifierNoPanic(t *testing.T) { + ctx := context.Background() + execID := "45678901-4567-4567-4567-45678901abcd" + adminEmail := "admin@example.com" + + mockConfig := new(MockConfigStore) + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "valid-token", + Status: "pending", + Recommendations: []config.RecommendationRecord{ + {ID: "r1"}, + }, + } + mockConfig.On("SavePurchaseExecution", ctx, exec).Return(nil) + + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil) + + handler := &Handler{ + purchase: mockPurchase, + config: mockConfig, + emailNotifier: nil, // best-effort send is skipped + } + + req := &events.LambdaFunctionURLRequest{} + session := &Session{Email: adminEmail, UserID: "admin-uid"} + + result, err := handler.directExecutePurchase(ctx, req, exec, session) + require.NoError(t, err) + assert.Equal(t, "completed", result.(map[string]any)["status"]) + mockPurchase.AssertExpectations(t) +} diff --git a/internal/api/handler.go b/internal/api/handler.go index ac0979d12..88f733a6c 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -341,7 +341,7 @@ func (h *Handler) HandleRequest(ctx context.Context, req *events.LambdaFunctionU } path := req.RequestContext.HTTP.Path - logging.Debugf("API Request: %s %s", method, path) + logging.Debugf("API Request: %s %s", method, redactURL(path)) // Validate request if response := h.validateRequest(ctx, req, method, path, corsHeaders); response != nil { diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index d09f16e09..4bede9f29 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -4,6 +4,7 @@ package api import ( "context" "crypto/sha256" + "crypto/subtle" "encoding/hex" "encoding/json" "errors" @@ -519,6 +520,32 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio if err := h.purchase.ApproveExecution(ctx, execution.ExecutionID, token, actor); err != nil { return nil, err } + // Re-fetch the execution to pick up the fresh revocation token written by + // mintRevocationToken inside ApproveExecution. The stale pre-approve struct + // still carries the old approval token; passing it to sendPurchaseExecutedEmail + // would embed that now-overwritten token as RevocationToken, causing any + // subsequent revoke attempt to return 403 (validateRevokeToken checks the + // stored token, which mintRevocationToken has already replaced). + emailExec, fetchErr := h.config.GetExecutionByID(ctx, execution.ExecutionID) + if fetchErr != nil || emailExec == nil { + // Best-effort fallback: the re-fetch failed, so we cannot obtain the + // fresh revocation token minted inside ApproveExecution. The stale + // pre-approve struct still carries the OLD approval token, which + // mintRevocationToken has already replaced in the DB -- emailing it + // would embed an invalid token and every Revoke click would 403. + // Instead, send a COPY with ApprovalToken blanked so the email + // template's {{if .RevocationToken}} suppresses the Revoke panel + // entirely (no broken button). The purchase is complete regardless. + logging.Warnf("approveViaToken[%s]: re-fetch for email failed (%v); suppressing Revoke panel (no valid token available)", + execution.ExecutionID, fetchErr) + emailCopy := *execution + emailCopy.ApprovalToken = "" + emailExec = &emailCopy + } + // Best-effort post-execution notification email (issue #291). Mirrors + // the session-authed path; errors are logged inside sendPurchaseExecutedEmail + // and never propagate — the purchase is already done at this point. + h.sendPurchaseExecutedEmail(ctx, req, emailExec, actor) return map[string]string{"status": "completed"}, nil } @@ -587,6 +614,11 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam logging.Infof("purchase[%s]: approvePurchaseViaSession completed in %s (auth=session)", execution.ExecutionID, time.Since(t0)) + // Best-effort post-execution notification email (issue #291). Fires + // after the synchronous purchase completes so the email carries the + // final committed state. Errors are logged inside sendPurchaseExecutedEmail + // and never propagate — the purchase is already done at this point. + h.sendPurchaseExecutedEmail(ctx, req, execution, session.Email) return map[string]string{"status": "completed"}, nil } @@ -971,6 +1003,301 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb return map[string]string{"status": "canceled"}, nil } +// revokeViaEmailToken is the one-click revocation handler embedded in the +// post-execution notification email (issue #291). It accepts the same +// three-mode dispatch shape as approvePurchase / cancelPurchase: +// +// 1. Session present AND RBAC-authorized (admin / cancel-any / +// cancel-own) → session-authed path. +// 2. token != "" → token-authed path via the email one-click link. +// 3. No token, no qualifying session → 401. +// +// The revocation window check is intentionally limited in this +// iteration: because the sibling "AWS RI/SP revocation via support case" +// issue has not yet landed its dedicated revocation_window_closes_at +// column, revokeViaEmailToken reuses the ApprovalToken. Once the sibling lands +// and adds a RevocationToken + RevocationWindowClosesAt column, this +// handler should validate RevocationWindowClosesAt here before +// attempting the cancellation. +// +// At this scope the revoke action is equivalent to a cancel on a +// completed/completed execution — the underlying cloud revocation +// (AWS support-case path) is out of scope for this issue and is handled +// by the sibling "AWS RI/SP revocation" issue. +// +// Present-day behavior: calling this route within the AWS revocation +// window requests the cancellation and returns {"status":"revocation_requested"}. +// Past the window it returns a friendly 409 with a plain-language message +// rather than a stack trace. +// revokeConfirmPageTmpl is the minimal HTML form rendered on GET /revoke. +// The form POSTs the token from a hidden input so the actual mutation never +// fires via a GET request (prevents email prefetchers from accidentally +// triggering the revocation). +const revokeConfirmPageTmpl = ` + +Confirm Revocation + + + +

Confirm Revocation

+

You are about to request revocation for purchase execution {{.ExecutionID}}.

+

This will record a revocation request. Contact AWS Support to complete the +cancellation within the allowed window.

+
+ + +
+

If you did not request this, ignore this page. No action has been taken.

+ +` + +func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFunctionURLRequest, execID, token string) (any, error) { + if err := validateUUID(execID); err != nil { + return nil, err + } + + // GET: render a confirmation page so email prefetchers cannot auto-trigger + // the revocation. The token travels in the URL query string (unavoidable for + // the email one-click link), but no mutation occurs on GET. + if req.RequestContext.HTTP.Method == "GET" { + return renderRevokeConfirmPage(execID, token), nil + } + + execution, err := h.config.GetExecutionByID(ctx, execID) + if err != nil { + return nil, fmt.Errorf("failed to get execution: %w", err) + } + if execution == nil { + return nil, NewClientError(404, "execution not found") + } + + // Only completed/partially_completed purchases have anything to revoke. + // A pending/notified purchase should be canceled instead. + if statusErr := checkRevokableStatus(execution); statusErr != nil { + return nil, statusErr + } + + // Three-mode dispatch — same shape as cancelPurchase. + result, handled, revokeErr := h.tryRevokeViaSession(ctx, req, execution) + if handled { + return result, revokeErr + } + + if token == "" { + return nil, NewClientError(401, "sign in or use the revocation link from the notification email") + } + + // One-click limitation (issue #291): the email link carries a token, but + // authorizeApprovalAction derives the actor from the *session* + // (tryResolveActorEmail), not from the token's bound contact email. A + // recipient who clicks the link while logged out therefore gets a 401 and + // must sign in with the matching contact email first. This intentionally + // matches the existing approve/cancel email-link behavior -- we do not + // widen the authz model here. True tokenless one-click (deriving the actor + // from a single-use, contact-bound token) is deferred to the sibling + // "AWS RI/SP revocation" work, which adds a dedicated revocation token. + // + // Token-authed path: validate the token (reusing the approval token for + // this iteration) and confirm the caller is the authorized contact email. + actor, err := h.authorizeApprovalAction(ctx, req, execution) + if err != nil { + return nil, err + } + if err := validateRevokeToken(execution, token); err != nil { + return nil, err + } + return h.revokeViaSession(ctx, execution, actor) +} + +// renderRevokeConfirmPage returns a rawResponse containing the HTML +// confirmation form for GET /revoke. The form POSTs the token so the actual +// mutation only happens when the user explicitly clicks "Confirm Revoke". +func renderRevokeConfirmPage(execID, token string) *rawResponse { + // Manual substitution to avoid importing html/template just for this one + // small page. The values substituted here (execID and token) are both + // treated as opaque strings — execID is a UUID (hex+hyphens only), + // token is a generated random string. HTML-escape them defensively anyway. + escaped := func(s string) string { + s = strings.ReplaceAll(s, "&", "&") + s = strings.ReplaceAll(s, "<", "<") + s = strings.ReplaceAll(s, ">", ">") + s = strings.ReplaceAll(s, "\"", """) + return s + } + page := strings.ReplaceAll(revokeConfirmPageTmpl, "{{.ExecutionID}}", escaped(execID)) + page = strings.ReplaceAll(page, "{{.Token}}", escaped(token)) + return &rawResponse{ + contentType: "text/html; charset=utf-8", + body: page, + } +} + +// tryRevokeViaSession attempts the session-authenticated branch of the +// revokeViaEmailToken three-mode dispatch (same shape as the session branch of +// cancelPurchase). Returns (result, true, err) when the session was present +// and either completed the revocation or encountered a hard error; returns +// (nil, false, nil) when the session was absent or returned a permission-denied +// error so the caller can fall through to the token branch. Extracted from +// revokeViaEmailToken to keep that function under the cyclomatic limit. +func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution) (result any, handled bool, err error) { + session := h.tryGetSession(ctx, req) + if session == nil { + return nil, false, nil + } + switch sessErr := h.authorizeSessionCancel(ctx, session, execution); { + case sessErr == nil: + // These endpoints are AuthPublic so the outer middleware skips CSRF. + // Enforce it here for the session-authed revoke sub-path, consistent + // with cancelPurchaseViaSession and approvePurchaseViaSession which both + // validate CSRF before mutating state (nit #1). + if csrfErr := h.validateCSRF(ctx, req); csrfErr != nil { + return nil, true, NewClientError(403, "CSRF validation failed") + } + res, revokeErr := h.revokeViaSession(ctx, execution, session.Email) + return res, true, revokeErr + case isPermissionDenied(sessErr): + // Fall through to the token branch. + return nil, false, nil + default: + return nil, true, sessErr + } +} + +// checkRevokableStatus returns nil when the execution is in a state that allows +// revocation (completed or partially_completed), or a 409 ClientError when the +// status makes revocation impossible. Extracted from revokeViaEmailToken to keep +// that function under the cyclomatic limit. +func checkRevokableStatus(execution *config.PurchaseExecution) error { + switch execution.Status { + case "completed", "partially_completed": + return nil + case "pending", "notified": + return NewClientError(409, fmt.Sprintf( + "execution %s is still pending — use the Cancel link instead of Revoke", execution.ExecutionID)) + default: + return NewClientError(409, fmt.Sprintf( + "execution %s cannot be revoked (status=%s); the revocation window may have closed or the purchase was not completed", + execution.ExecutionID, execution.Status)) + } +} + +// validateRevokeToken checks that the execution carries a non-empty +// ApprovalToken, that the token has not expired, that the revocation window +// has not closed (config.RevocationWindow after CompletedAt), and that the +// token matches using constant-time comparison. Pre-migration rows without an +// ApprovalTokenExpiresAt pass the expiry check (legacy backward compat). +// Extracted from revokeViaEmailToken to keep that function under the cyclomatic limit. +func validateRevokeToken(execution *config.PurchaseExecution, token string) error { + if execution.ApprovalToken == "" { + return NewClientError(403, "invalid revocation token") + } + if execution.ApprovalTokenExpiresAt != nil && time.Now().After(*execution.ApprovalTokenExpiresAt) { + return NewClientError(409, fmt.Sprintf( + "execution %s cannot be revoked: the revocation link has expired", execution.ExecutionID)) + } + // Enforce the 24-hour revocation window (issue #291 Finding #6). + // Use CompletedAt as the reference timestamp; fall back to ExecutedAt + // for direct-execute rows which may not have a CompletedAt yet. + if windowErr := checkRevocationWindow(execution); windowErr != nil { + return windowErr + } + storedHash := sha256.Sum256([]byte(execution.ApprovalToken)) + userHash := sha256.Sum256([]byte(token)) + if subtle.ConstantTimeCompare(storedHash[:], userHash[:]) != 1 { + return NewClientError(403, "invalid revocation token") + } + return nil +} + +// checkRevocationWindow returns a 403 ClientError when the 24-hour revocation +// window has passed. The reference timestamp is CompletedAt if set, otherwise +// ExecutedAt. When neither is set (legacy rows without these columns) the +// check is skipped to preserve backward compatibility. +func checkRevocationWindow(execution *config.PurchaseExecution) error { + var ref *time.Time + if execution.CompletedAt != nil { + ref = execution.CompletedAt + } else if execution.ExecutedAt != nil { + ref = execution.ExecutedAt + } + if ref == nil { + // Legacy row: no timestamp to anchor the window; allow the revoke. + return nil + } + windowCloses := ref.Add(config.RevocationWindow) + if time.Now().After(windowCloses) { + return NewClientError(403, fmt.Sprintf( + "revocation window has closed (window closed at %s)", windowCloses.UTC().Format(time.RFC3339))) + } + return nil +} + +// revocationWindowClosesAt returns the human-readable UTC string at which the +// 24-hour revocation window closes. Used by sendPurchaseExecutedEmail to +// populate the RevocationWindowClosesAt field in the email template. +// Returns "" when no reference timestamp is available (legacy rows). +func revocationWindowClosesAt(execution *config.PurchaseExecution) string { + var ref *time.Time + if execution.CompletedAt != nil { + ref = execution.CompletedAt + } else if execution.ExecutedAt != nil { + ref = execution.ExecutedAt + } + if ref == nil { + return "" + } + return ref.Add(config.RevocationWindow).UTC().Format("2006-01-02 15:04 UTC") +} + +// revokeViaSession performs the post-execution revocation action by recording +// a "revocation_requested" status on the execution. Actual cloud-side +// revocation (AWS support-case) is out of scope for issue #291 — it is +// handled by the sibling "AWS RI/SP revocation via support case" issue. +// This call records the intent so the History UI can surface it. +// +// The status flip uses TransitionExecutionStatus, which issues a conditional +// UPDATE WHERE status IN ('completed','partially_completed'). This guards +// against lost-update races: if the row changed between the GetExecutionByID +// read and this write (e.g. a concurrent transition), zero rows are affected +// and we return a 409 rather than blindly overwriting. CancelledBy is stamped +// in a follow-up SavePurchaseExecution on the freshly-returned row. +func (h *Handler) revokeViaSession(ctx context.Context, execution *config.PurchaseExecution, revokedBy string) (any, error) { + actor := &revokedBy + updated, err := h.config.TransitionExecutionStatus( + ctx, execution.ExecutionID, + []string{"completed", "partially_completed"}, "revocation_requested", actor) + if err != nil { + if errors.Is(err, config.ErrExecutionNotInExpectedStatus) { + return nil, NewClientError(409, fmt.Sprintf( + "execution %s cannot be revoked: a concurrent operation changed its status", execution.ExecutionID)) + } + if errors.Is(err, config.ErrNotFound) { + return nil, NewClientError(404, "execution not found") + } + return nil, fmt.Errorf("failed to record revocation request for execution %s: %w", execution.ExecutionID, err) + } + if revokedBy != "" { + // Use a narrow SetCancelledBy UPDATE rather than a full-row + // SavePurchaseExecution to avoid clobbering concurrent writes + // between the TransitionExecutionStatus and this attribution step + // (Finding #5). The targeted UPDATE only touches the cancellation + // attribution column so any other concurrent column change (e.g. a + // background webhook stamping a cloud reference) is preserved. + if err := h.config.SetCancelledBy(ctx, execution.ExecutionID, revokedBy); err != nil { + return nil, fmt.Errorf("failed to record revocation requester for execution %s: %w", execution.ExecutionID, err) + } + _ = updated // kept for future use; CancelledBy is now persisted atomically + } + logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, redactEmail(revokedBy)) + return map[string]string{ + "status": "revocation_requested", + "message": "Revocation request recorded. Contact AWS Support to complete the cancellation within the allowed window.", + }, nil +} + // authorizeSessionCancel returns nil when the session is permitted to cancel // the given execution under the cancel-any / cancel-own RBAC rules added in // issue #46. Returns a 403 ClientError otherwise. @@ -1946,7 +2273,7 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio if err := h.authorizeSessionExecuteDirect(ctx, session, creatorID); err != nil { return nil, err } - return h.directExecutePurchase(ctx, execution, session) + return h.directExecutePurchase(ctx, req, execution, session) } // Send approval email synchronously so the response can surface the @@ -2008,7 +2335,11 @@ func buildApprovalPendingResponse( // synchronously. ApproveAndExecute already stamps ApprovedBy; we pass // the session email as the actor so the approved_by column also records // who direct-executed. -// 3. Return a "completed" status to the caller. +// 3. Send the best-effort post-execution notification email (issue #291). +// This is the only email the direct-execute flow emits -- no approval +// email precedes it -- so it is the path where the executed-notification +// matters most. +// 4. Return a "completed" status to the caller. // // The audit fields are best-effort if ApproveAndExecute's SavePurchaseExecution // races with our pre-call stamp -- but in practice ApproveAndExecute calls @@ -2017,7 +2348,7 @@ func buildApprovalPendingResponse( // TransitionExecutionStatus. The critical audit invariant is that a non-nil // executed_by_user_id always co-occurs with a non-nil pre_approval_skip_reason, // and both are set atomically in the same SavePurchaseExecution call here. -func (h *Handler) directExecutePurchase(ctx context.Context, execution *config.PurchaseExecution, session *Session) (any, error) { +func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, session *Session) (any, error) { t0 := time.Now() executionID := execution.ExecutionID logging.Infof("purchase[%s]: directExecutePurchase entry (auth=session)", executionID) @@ -2048,6 +2379,15 @@ func (h *Handler) directExecutePurchase(ctx context.Context, execution *config.P } logging.Infof("purchase[%s]: directExecutePurchase completed in %s", executionID, time.Since(t0)) + // Best-effort post-execution notification email (issue #291). The + // direct-execute path sends no approval email (the whole point is to skip + // the approval round-trip), so this is the only notification the recipients + // receive -- making the executed-notification the most valuable here. + // Mirrors the approve-path call sites (see approvePurchase and + // approvePurchaseViaSession); recipient resolution and the nil-notifier + // guard live inside sendPurchaseExecutedEmail. session.Email is the actor + // who direct-executed, matching the actor passed to ApproveAndExecute above. + h.sendPurchaseExecutedEmail(ctx, req, execution, session.Email) return map[string]any{ "execution_id": executionID, "status": "completed", @@ -2165,6 +2505,193 @@ func (h *Handler) sendPurchaseApprovalEmail(ctx context.Context, req *events.Lam return true, "", responseRecipient } +// sendPurchaseExecutedEmail sends a post-execution notification to the +// configured recipients after a purchase completes successfully. It follows +// the same best-effort contract as sendPurchaseApprovalEmail: errors are +// logged but never propagate to the caller — the purchase is already done. +// +// Recipients (deduped): +// - global notification_email (Settings → General) +// - per-account contact_email for each recommendation's account +// - email of the user who originally submitted the execution +// (looked up by CreatedByUserID via h.auth.GetUser) +// +// The revocation link uses the execution's ApprovalToken (reusing the same +// token infrastructure). When the sibling "AWS RI/SP revocation via support +// case" issue lands and adds a dedicated revocation token + window field, +// this method should be updated to use those fields instead. +func (h *Handler) sendPurchaseExecutedEmail(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, executedByEmail string) { + if h.emailNotifier == nil { + logging.Debug("sendPurchaseExecutedEmail: no email notifier configured, skipping") + return + } + if execution.ExecutionID == "" { + logging.Warn("sendPurchaseExecutedEmail: empty execution ID, skipping") + return + } + + globalCfg, err := h.config.GetGlobalConfig(ctx) + if err != nil { + logging.Errorf("sendPurchaseExecutedEmail: failed to load global config: %v", err) + return + } + globalNotify := globalNotifyEmail(globalCfg) + + // Gather per-account contact emails for the recommendations. + contactEmails, err := h.gatherAccountContactEmails(ctx, execution.Recommendations) + if err != nil { + logging.Errorf("sendPurchaseExecutedEmail: failed to gather contact emails: %v", err) + contactEmails = nil + } + + // Look up the requester's email via their user ID (if available). + requesterEmail := h.lookupRequesterInfo(ctx, execution) + + // Build the deduplicated To / Cc list. + // Priority: contact emails are To (first one) + Cc (rest); global notify + // and requester email are Cc. When there are no contact emails, the global + // notify becomes To (matching the approval-email fallback in resolveApprovalRecipients). + to, cc := resolveExecutedNotificationRecipients(contactEmails, globalNotify, requesterEmail) + if to == "" { + logging.Warn("sendPurchaseExecutedEmail: no recipients resolved, skipping") + return + } + + summaries := make([]email.RecommendationSummary, 0, len(execution.Recommendations)) + for i := range execution.Recommendations { + rec := &execution.Recommendations[i] + summaries = append(summaries, email.RecommendationSummary{ + Service: rec.Service, + ResourceType: rec.ResourceType, + Engine: rec.Engine, + Region: rec.Region, + Count: rec.Count, + MonthlySavings: rec.Savings, + Term: rec.Term, + Payment: rec.Payment, + UpfrontCost: rec.UpfrontCost, + }) + } + + dashboardBase := h.resolveDashboardURL(req) + data := email.NotificationData{ + DashboardURL: dashboardBase, + ExecutionID: execution.ExecutionID, + TotalSavings: execution.EstimatedSavings, + TotalUpfrontCost: execution.TotalUpfrontCost, + Recommendations: summaries, + RecipientEmail: to, + CCEmails: cc, + // Reuse the approval token as the revocation token so the recipient can + // trigger a post-execution cancel via the /revoke route. A dedicated + // revocation token will be added when the sibling "AWS RI/SP revocation" + // issue lands its own DB column. + RevocationToken: execution.ApprovalToken, + // Populate the revocation window deadline so the email copy matches + // the enforced 24-hour window in validateRevokeToken (Finding #6). + RevocationWindowClosesAt: revocationWindowClosesAt(execution), + RequestedByEmail: requesterEmail, + // api.User has no display-name field yet; leave RequestedByName empty + // until a name source lands (the email template tolerates ""). + RequestedByName: "", + RequestedAt: executionTimestamp(execution), + ExecutedBy: executedByEmail, + ExecutedAt: time.Now().UTC().Format(time.RFC3339), + } + if dashboardBase != "" { + data.ArcheraEducationURL = dashboardBase + "/archera-insurance" + } + + if sendErr := h.emailNotifier.SendPurchaseExecutedNotification(ctx, data); sendErr != nil { + logging.Errorf("sendPurchaseExecutedEmail: send failed for execution %s: %v", execution.ExecutionID, sendErr) + } +} + +// globalNotifyEmail returns the trimmed notification email from a GlobalConfig, +// or "" when the config is nil or the field is unset. Extracted from +// sendPurchaseExecutedEmail to keep that function under the cyclomatic limit. +func globalNotifyEmail(globalCfg *config.GlobalConfig) string { + if globalCfg != nil && globalCfg.NotificationEmail != nil { + return strings.TrimSpace(*globalCfg.NotificationEmail) + } + return "" +} + +// lookupRequesterInfo resolves the email for the user who originally submitted +// the execution. The lookup is non-fatal: when auth is unavailable or the user +// cannot be found, "" is returned and the notification is sent without it. +// Extracted from sendPurchaseExecutedEmail to keep that function under the +// cyclomatic limit. Only the email is returned -- api.User carries no display +// name field, so the notification's RequestedByName is populated separately if +// and when a name source lands. +func (h *Handler) lookupRequesterInfo(ctx context.Context, execution *config.PurchaseExecution) (requesterEmail string) { + if h.auth == nil || execution.CreatedByUserID == nil || *execution.CreatedByUserID == "" { + return "" + } + u, err := h.auth.GetUser(ctx, *execution.CreatedByUserID) + if err != nil { + logging.Warnf("lookupRequesterInfo: GetUser(%s) failed: %v", *execution.CreatedByUserID, err) + return "" + } + if u == nil { + logging.Warnf("lookupRequesterInfo: GetUser(%s) returned nil user", *execution.CreatedByUserID) + return "" + } + return u.Email +} + +// resolveExecutedNotificationRecipients builds the To / Cc pair for the +// post-execution notification from the three input email sets. The logic +// mirrors resolveApprovalRecipients: first contact email is To; remaining +// contact emails, globalNotify, and requesterEmail are Cc (deduped). When +// no contact emails are present, globalNotify becomes To and requesterEmail +// becomes Cc. +func resolveExecutedNotificationRecipients(contactEmails []string, globalNotify, requesterEmail string) (to string, cc []string) { + seen := map[string]bool{} + addCc := func(addr string) { + norm := strings.ToLower(strings.TrimSpace(addr)) + if norm == "" || seen[norm] { + return + } + seen[norm] = true + cc = append(cc, addr) + } + + if len(contactEmails) > 0 { + to = contactEmails[0] + seen[strings.ToLower(strings.TrimSpace(to))] = true + for _, addr := range contactEmails[1:] { + addCc(addr) + } + addCc(globalNotify) + addCc(requesterEmail) + return to, cc + } + + // No contact emails: fall back to globalNotify as To. + if globalNotify != "" { + to = globalNotify + seen[strings.ToLower(strings.TrimSpace(to))] = true + addCc(requesterEmail) + return to, cc + } + + // Last resort: requester email only. + to = strings.TrimSpace(requesterEmail) + return to, nil +} + +// executionTimestamp returns a human-readable timestamp for when the execution +// was submitted (ScheduledDate for web-submitted rows). Returns empty string +// when the execution has no timestamp (shouldn't happen in practice but +// guards against zero-value time panics in templates). +func executionTimestamp(exec *config.PurchaseExecution) string { + if exec.ScheduledDate.IsZero() { + return "" + } + return exec.ScheduledDate.UTC().Format(time.RFC3339) +} + // resolveDashboardURL returns the absolute base URL to embed in email // approval/cancel links. Preference order matches the OIDC issuer helper's // strategy for the same underlying problem (Lambda's Function URL can't be diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 8efd3c374..753d9ed5c 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -4004,3 +4004,693 @@ func TestApproveWithDelay_CASLostMaps409(t *testing.T) { assert.Equal(t, 409, ce.code, "concurrent cancel CAS race must map to 409, not 500") mockConfig.AssertExpectations(t) } + +// --------------------------------------------------------------------------- +// revokePurchase handler tests (issue #291) +// --------------------------------------------------------------------------- + +// buildCompletedExec returns a completed execution suitable for revoke tests. +func buildCompletedExec(execID, approvalToken string) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: approvalToken, + Status: "completed", + } +} + +// TestHandler_revokePurchase_ValidToken verifies that a valid token on a +// completed execution records a revocation_requested status. +func TestHandler_revokePurchase_ValidToken(t *testing.T) { + ctx := context.Background() + execID := "11111111-1111-1111-1111-111111111111" + token := "abc123validtoken" + revokerEmail := "contact@acct.example.com" + accountID := "acct-1" + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: token, + Status: "completed", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + // authorizeApprovalAction: GetGlobalConfig for global notify + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + // GetCloudAccount returns the contact email so authorizeApprovalAction can + // resolve the approver and verify the session's email matches. + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: revokerEmail}, nil + } + // Atomic conditional transition completed/partially_completed -> + // revocation_requested, returning the updated row. + mockStore.On("TransitionExecutionStatus", ctx, execID, + []string{"completed", "partially_completed"}, "revocation_requested", + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == revokerEmail })). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil) + // SetCancelledBy stamps the actor without a full-row overwrite (Finding #5). + mockStore.On("SetCancelledBy", ctx, execID, revokerEmail).Return(nil) + + mockAuth := new(MockAuthService) + // Provide a session for the revoker so authorizeApprovalAction resolves actor. + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: revokerEmail}, nil) + // RBAC: revoker has no cancel-any or cancel-own, so falls through to token path. + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + QueryStringParameters: map[string]string{"token": token}, + } + result, err := handler.revokeViaEmailToken(ctx, req, execID, token) + require.NoError(t, err, "valid token on completed execution must not error") + resultMap := result.(map[string]string) + assert.Equal(t, "revocation_requested", resultMap["status"]) + mockStore.AssertExpectations(t) +} + +// TestHandler_revokePurchase_InvalidToken verifies that a wrong token returns 403. +// The session provides an email that matches the contact email (so +// authorizeApprovalAction passes), but the token itself is wrong. +func TestHandler_revokePurchase_InvalidToken(t *testing.T) { + ctx := context.Background() + execID := "22222222-2222-2222-2222-222222222222" + contactEmail := "contact@acct.example.com" + accountID := "acct-1" + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "the-real-token", + Status: "completed", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + QueryStringParameters: map[string]string{"token": "wrong-token"}, + } + _, err := handler.revokeViaEmailToken(ctx, req, execID, "wrong-token") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 403, ce.code) +} + +// TestHandler_revokePurchase_PendingExecution verifies that a pending execution +// returns 409 with a friendly message directing the user to Cancel instead. +func TestHandler_revokePurchase_PendingExecution(t *testing.T) { + ctx := context.Background() + execID := "33333333-3333-3333-3333-333333333333" + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "tok", + Status: "pending", + } + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + + handler := &Handler{config: mockStore} + req := &events.LambdaFunctionURLRequest{ + QueryStringParameters: map[string]string{"token": "tok"}, + } + _, err := handler.revokeViaEmailToken(ctx, req, execID, "tok") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "Cancel") +} + +// TestHandler_revokePurchase_NotFound verifies 404 when the execution does not exist. +func TestHandler_revokePurchase_NotFound(t *testing.T) { + ctx := context.Background() + execID := "44444444-4444-4444-4444-444444444444" + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(nil, nil) + + handler := &Handler{config: mockStore} + req := &events.LambdaFunctionURLRequest{} + _, err := handler.revokeViaEmailToken(ctx, req, execID, "some-token") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 404, ce.code) +} + +// TestHandler_revokePurchase_ExpiredToken verifies that a matching token whose +// ApprovalTokenExpiresAt deadline has passed is rejected with 409, so a stale +// completed execution cannot be marked revocation_requested via an old link. +// Regression test for the revoke-token expiry CR finding (PR #889). +func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { + ctx := context.Background() + execID := "55555555-5555-5555-5555-555555555555" + token := "expiredbutmatching" + contactEmail := "contact@acct.example.com" + accountID := "acct-1" + past := time.Now().Add(-1 * time.Hour) + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: token, + ApprovalTokenExpiresAt: &past, + Status: "completed", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + QueryStringParameters: map[string]string{"token": token}, + } + _, err := handler.revokeViaEmailToken(ctx, req, execID, token) + require.Error(t, err, "expired revocation token must be rejected") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "expired") +} + +// TestHandler_revokePurchase_SessionAdminCancelAny exercises the tokenless +// session-authorized revoke branch (tryRevokeViaSession -> revokeViaSession) +// for an admin holding cancel-any on purchases. It asserts the atomic +// TransitionExecutionStatus call and the revocation_requested result. +// Covers the session-auth revoke branch CR finding (PR #889). +func TestHandler_revokePurchase_SessionAdminCancelAny(t *testing.T) { + ctx := context.Background() + execID := "66666666-6666-6666-6666-666666666666" + adminEmail := "admin@example.com" + adminUserID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + + exec := buildCompletedExec(execID, "unused-token") + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, + []string{"completed", "partially_completed"}, "revocation_requested", + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == adminEmail })). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil) + // SetCancelledBy replaces the full-row SavePurchaseExecution (Finding #5). + mockStore.On("SetCancelledBy", ctx, execID, adminEmail).Return(nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: adminUserID, Email: adminEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, adminUserID, "cancel-any", "purchases").Return(true, nil) + // CSRF is enforced for the session-authed revoke path (tryRevokeViaSession). + // No CSRF token header in this request, so csrfToken is "". + mockAuth.On("ValidateCSRFToken", ctx, "admin-token", "").Return(nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + // No token query param: forces the tokenless session-auth path. + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer admin-token"}, + } + result, err := handler.revokeViaEmailToken(ctx, req, execID, "") + require.NoError(t, err, "session admin with cancel-any must succeed") + resultMap := result.(map[string]string) + assert.Equal(t, "revocation_requested", resultMap["status"]) + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) +} + +// TestHandler_revokePurchase_SessionOwnerCancelOwn exercises the tokenless +// session-auth branch for a non-admin owner holding cancel-own on a purchase +// they created. Covers the session-auth revoke branch CR finding (PR #889). +func TestHandler_revokePurchase_SessionOwnerCancelOwn(t *testing.T) { + ctx := context.Background() + execID := "77777777-7777-7777-7777-777777777777" + ownerEmail := "owner@example.com" + ownerUserID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + + exec := buildCompletedExec(execID, "unused-token") + exec.CreatedByUserID = &ownerUserID + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, + []string{"completed", "partially_completed"}, "revocation_requested", + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == ownerEmail })). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil) + // SetCancelledBy replaces the full-row SavePurchaseExecution (Finding #5). + mockStore.On("SetCancelledBy", ctx, execID, ownerEmail).Return(nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "owner-token"). + Return(&Session{UserID: ownerUserID, Email: ownerEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-own", "purchases").Return(true, nil) + // CSRF is enforced for the session-authed revoke path (tryRevokeViaSession). + mockAuth.On("ValidateCSRFToken", ctx, "owner-token", "").Return(nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer owner-token"}, + } + result, err := handler.revokeViaEmailToken(ctx, req, execID, "") + require.NoError(t, err, "session owner with cancel-own must succeed") + resultMap := result.(map[string]string) + assert.Equal(t, "revocation_requested", resultMap["status"]) + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) +} + +// TestHandler_revokePurchase_SessionNoPermissionNoToken verifies that a session +// lacking both cancel-any and cancel-own, with no token supplied, is denied: +// the session branch returns permission-denied and falls through to the +// tokenless 401. Covers the session-auth revoke branch CR finding (PR #889). +func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { + ctx := context.Background() + execID := "88888888-8888-8888-8888-888888888888" + userEmail := "nobody@example.com" + userID := "cccccccc-cccc-cccc-cccc-cccccccccccc" + + exec := buildCompletedExec(execID, "unused-token") + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "user-token"). + Return(&Session{UserID: userID, Email: userEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-own", "purchases").Return(false, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer user-token"}, + } + _, err := handler.revokeViaEmailToken(ctx, req, execID, "") + require.Error(t, err, "no permission and no token must be denied") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 401, ce.code) + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) +} + +// TestHandler_revokeViaSession_ConcurrentTransition verifies that a lost-update +// race (the row's status changed between read and the conditional UPDATE) is +// surfaced as a 409 rather than silently overwriting. Regression test for the +// atomic-status-transition CR finding (PR #889). +func TestHandler_revokeViaSession_ConcurrentTransition(t *testing.T) { + ctx := context.Background() + execID := "99999999-9999-9999-9999-999999999999" + + exec := buildCompletedExec(execID, "tok") + + mockStore := new(MockConfigStore) + mockStore.On("TransitionExecutionStatus", ctx, execID, + []string{"completed", "partially_completed"}, "revocation_requested", + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == "someone@example.com" })). + Return(nil, fmt.Errorf("%w: execution %s", config.ErrExecutionNotInExpectedStatus, execID)) + + handler := &Handler{config: mockStore} + _, err := handler.revokeViaSession(ctx, exec, "someone@example.com") + require.Error(t, err, "a concurrent transition must surface an error") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 409, ce.code) + mockStore.AssertExpectations(t) +} + +// --------------------------------------------------------------------------- +// resolveExecutedNotificationRecipients unit tests (issue #291) +// --------------------------------------------------------------------------- + +// TestResolveExecutedNotificationRecipients_ContactEmailAsTo verifies the +// first contact email becomes To and the rest + global notify + requester +// are added as Cc (deduplicated). +func TestResolveExecutedNotificationRecipients_ContactEmailAsTo(t *testing.T) { + to, cc := resolveExecutedNotificationRecipients( + []string{"contact@a.example.com", "contact@b.example.com"}, + "notify@example.com", + "requester@example.com", + ) + assert.Equal(t, "contact@a.example.com", to) + assert.Contains(t, cc, "contact@b.example.com") + assert.Contains(t, cc, "notify@example.com") + assert.Contains(t, cc, "requester@example.com") + // No duplicates. + assert.Equal(t, 3, len(cc)) +} + +// TestResolveExecutedNotificationRecipients_GlobalNotifyFallback verifies that +// when no contact emails are available, the global notification email becomes To. +func TestResolveExecutedNotificationRecipients_GlobalNotifyFallback(t *testing.T) { + to, cc := resolveExecutedNotificationRecipients( + nil, + "notify@example.com", + "requester@example.com", + ) + assert.Equal(t, "notify@example.com", to) + assert.Contains(t, cc, "requester@example.com") + assert.Equal(t, 1, len(cc)) +} + +// TestResolveExecutedNotificationRecipients_RequesterOnlyFallback verifies that +// when neither contact emails nor global notify are set, the requester email +// becomes To (last resort). +func TestResolveExecutedNotificationRecipients_RequesterOnlyFallback(t *testing.T) { + to, cc := resolveExecutedNotificationRecipients(nil, "", "requester@example.com") + assert.Equal(t, "requester@example.com", to) + assert.Empty(t, cc) +} + +// TestResolveExecutedNotificationRecipients_Deduplication verifies that the +// same email in multiple lists is not repeated. +func TestResolveExecutedNotificationRecipients_Deduplication(t *testing.T) { + // Same email in all three slots. + to, cc := resolveExecutedNotificationRecipients( + []string{"same@example.com"}, + "SAME@example.com", // case-insensitive dedup + "same@example.com", + ) + assert.Equal(t, "same@example.com", to) + assert.Empty(t, cc, "duplicate emails must be deduplicated") +} + +// --------------------------------------------------------------------------- +// Finding #5: revokeViaSession uses SetCancelledBy (no lost-update clobber) +// --------------------------------------------------------------------------- + +// TestRevokeViaSession_CancelledByFoldsIntoTransition_NoLostUpdate verifies +// that revokeViaSession uses SetCancelledBy (a narrow single-column UPDATE) +// rather than a full-row SavePurchaseExecution, preventing a lost-update +// race between the TransitionExecutionStatus and the attribution write. +func TestRevokeViaSession_CancelledByFoldsIntoTransition_NoLostUpdate(t *testing.T) { + ctx := context.Background() + execID := "55555555-5555-5555-5555-555555555555" + revokedBy := "revoker@example.com" + + exec := buildCompletedExec(execID, "tok") + + mockStore := new(MockConfigStore) + mockStore.On("TransitionExecutionStatus", ctx, execID, + []string{"completed", "partially_completed"}, "revocation_requested", + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == revokedBy })). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil) + // SetCancelledBy must be called; SavePurchaseExecution must NOT. + mockStore.On("SetCancelledBy", ctx, execID, revokedBy).Return(nil) + + handler := &Handler{config: mockStore} + result, err := handler.revokeViaSession(ctx, exec, revokedBy) + require.NoError(t, err) + resultMap := result.(map[string]string) + assert.Equal(t, "revocation_requested", resultMap["status"]) + + mockStore.AssertExpectations(t) + mockStore.AssertNotCalled(t, "SavePurchaseExecution") +} + +// --------------------------------------------------------------------------- +// Finding #3: redactURL strips token from logged URLs +// --------------------------------------------------------------------------- + +// TestRedactURL_StripsTokenQueryParam verifies the redactURL helper removes +// the token value from URLs before they are written to logs. +func TestRedactURL_StripsTokenQueryParam(t *testing.T) { + cases := []struct { + input string + want string + }{ + { + input: "/api/purchases/revoke/exec-id?token=supersecret", + want: "/api/purchases/revoke/exec-id?token=REDACTED", + }, + { + input: "/api/purchases/revoke/exec-id?other=val&token=supersecret", + want: "/api/purchases/revoke/exec-id?other=val&token=REDACTED", + }, + { + input: "/api/purchases/revoke/exec-id?token=supersecret&other=val", + want: "/api/purchases/revoke/exec-id?token=REDACTED&other=val", + }, + { + input: "/api/purchases/approve/exec-id?token=abc123", + want: "/api/purchases/approve/exec-id?token=REDACTED", + }, + { + // No token param: pass through unchanged. + input: "/api/purchases/approve/exec-id?other=x", + want: "/api/purchases/approve/exec-id?other=x", + }, + { + // No query string: pass through unchanged. + input: "/api/purchases/revoke/exec-id", + want: "/api/purchases/revoke/exec-id", + }, + } + for _, tc := range cases { + t.Run(tc.input, func(t *testing.T) { + assert.Equal(t, tc.want, redactURL(tc.input)) + }) + } +} + +// TestRevokePOSTReadsTokenFromBody verifies that resolveApprovalToken reads +// the token from the HTML form POST body (application/x-www-form-urlencoded) +// rather than the query string, so the token does not appear in access logs. +// This test lives in the api package and calls the router-level function. +func TestRevokePOSTReadsTokenFromBody(t *testing.T) { + req := &events.LambdaFunctionURLRequest{ + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "POST"}, + }, + // Token present in BOTH body and query string; body must win. + Body: "token=body-token", + QueryStringParameters: map[string]string{"token": "query-token"}, + } + got := resolveApprovalToken(req) + assert.Equal(t, "body-token", got, "POST body token must take priority over query string") +} + +// --------------------------------------------------------------------------- +// Finding #1: Revoke must reject a cleared or mismatched ApprovalToken +// --------------------------------------------------------------------------- + +// TestRevokePurchase_RejectsClearedToken verifies that after cancel clears +// the ApprovalToken to "", any revoke attempt is rejected with 403. The cancel +// path uses clearApprovalToken (not mintRevocationToken), so the stored token +// is empty and no revoke is possible. +func TestRevokePurchase_RejectsClearedToken(t *testing.T) { + exec := &config.PurchaseExecution{ + ExecutionID: "exec-post-cancel", + Status: "completed", + ApprovalToken: "", // cleared by clearApprovalToken after cancel + } + err := validateRevokeToken(exec, "pre-cancel-token") + require.Error(t, err, "cleared (empty) stored token must be rejected") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError") + assert.Equal(t, 403, ce.code, "must return 403 for a cleared token") +} + +// TestRevokePurchase_RejectsRotatedToken verifies that after approve mints a +// fresh revocation token (mintRevocationToken), the OLD approval token from the +// approval email link can no longer be used for revocation. Only the fresh +// token embedded in the executed-notification email is valid. +// +// This guards against a recipient of the approval-request email using that +// link's token to also revoke the purchase after it executed. +func TestRevokePurchase_RejectsRotatedToken(t *testing.T) { + freshToken := "fresh-revoke-token" + exec := &config.PurchaseExecution{ + ExecutionID: "exec-post-approve", + Status: "completed", + ApprovalToken: freshToken, // minted by mintRevocationToken after approve + } + // The OLD approval token (from the approve-request email) must be rejected + // because the stored token has been rotated to the fresh revocation token. + err := validateRevokeToken(exec, "old-approval-token") + require.Error(t, err, "old approval token must be rejected after token rotation") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError") + assert.Equal(t, 403, ce.code, "must return 403 for the rotated-away old token") + + // Confirm the fresh token IS accepted, proving the guard is hash-based + // (not a blanket rejection). + require.NoError(t, validateRevokeToken(exec, freshToken), + "fresh revocation token must be accepted for revoke") +} + +// --------------------------------------------------------------------------- +// Finding #2: GET /revoke must render confirmation form, not mutate state +// --------------------------------------------------------------------------- + +// --------------------------------------------------------------------------- +// Finding #6: 24-hour revocation window enforcement +// --------------------------------------------------------------------------- + +// TestRevoke_WithinWindow_Allowed verifies that a revoke token is accepted +// when the purchase completed less than 24 hours ago. +func TestRevoke_WithinWindow_Allowed(t *testing.T) { + recentCompleted := time.Now().Add(-2 * time.Hour) + exec := &config.PurchaseExecution{ + ExecutionID: "exec-in-window", + Status: "completed", + ApprovalToken: "valid-token", + CompletedAt: &recentCompleted, + } + err := validateRevokeToken(exec, "valid-token") + require.NoError(t, err, "revoke within 24h window must succeed") +} + +// TestRevoke_AfterWindow_Denied verifies that validateRevokeToken returns 403 +// when the 24-hour revocation window has passed. +func TestRevoke_AfterWindow_Denied(t *testing.T) { + staleCompleted := time.Now().Add(-25 * time.Hour) + exec := &config.PurchaseExecution{ + ExecutionID: "exec-past-window", + Status: "completed", + ApprovalToken: "valid-token", + CompletedAt: &staleCompleted, + } + err := validateRevokeToken(exec, "valid-token") + require.Error(t, err, "revoke after 24h window must be denied") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError") + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "revocation window has closed") +} + +// TestRevoke_NoTimestamp_Allowed verifies that legacy rows without CompletedAt +// or ExecutedAt skip the window check and are allowed through. +func TestRevoke_NoTimestamp_Allowed(t *testing.T) { + exec := &config.PurchaseExecution{ + ExecutionID: "exec-legacy", + Status: "completed", + ApprovalToken: "valid-token", + // No CompletedAt or ExecutedAt set (legacy row). + } + err := validateRevokeToken(exec, "valid-token") + require.NoError(t, err, "legacy rows without timestamp must not be blocked by window check") +} + +// TestRevoke_4EyesMode_TODO is a placeholder to surface the 4-eyes-mode +// interaction once issue #1005 lands. +func TestRevoke_4EyesMode_TODO(t *testing.T) { + t.Skip("placeholder until #1005 lands: verify that 4-eyes-mode approval flows interact correctly with the revocation window") +} + +// TestRevokePurchase_GETRendersConfirmationPage_NoMutation verifies that a GET +// to the revoke handler returns 200 HTML containing a confirmation form and +// does NOT call TransitionExecutionStatus (no mutation). +func TestRevokePurchase_GETRendersConfirmationPage_NoMutation(t *testing.T) { + ctx := context.Background() + execID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + token := "revoke-token-abc" + + // Store must NOT be called at all: the GET path short-circuits before any DB ops. + mockStore := new(MockConfigStore) + + handler := &Handler{config: mockStore} + req := &events.LambdaFunctionURLRequest{ + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "GET"}, + }, + QueryStringParameters: map[string]string{"token": token}, + } + result, err := handler.revokeViaEmailToken(ctx, req, execID, token) + require.NoError(t, err) + + raw, ok := result.(*rawResponse) + require.True(t, ok, "GET /revoke must return a *rawResponse (HTML confirmation page)") + assert.Contains(t, raw.body, `