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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal/api/coverage_gaps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,9 @@ func (s *stubEmailNotifier) SendRIExchangeCompleted(_ context.Context, _ email.R
func (s *stubEmailNotifier) SendPurchaseApprovalRequest(_ 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
}
Expand Down
324 changes: 324 additions & 0 deletions internal/api/handler_purchases.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package api
import (
"context"
"crypto/sha256"
"crypto/subtle"
"encoding/hex"
"encoding/json"
"errors"
Expand Down Expand Up @@ -390,6 +391,8 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio
if err := h.purchase.ApproveExecution(ctx, execID, token, actor); err != nil {
return nil, err
}
// Best-effort post-execution notification (issue #291).
h.sendPurchaseExecutedEmail(ctx, req, execution, actor)
return map[string]string{"status": "completed"}, nil
}

Expand Down Expand Up @@ -440,6 +443,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
}

Expand Down Expand Up @@ -611,6 +619,148 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb
return map[string]string{"status": "cancelled"}, nil
}

// revokePurchase 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, revokePurchase 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 behaviour: 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.
func (h *Handler) revokePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execID, token string) (any, error) {
if err := validateUUID(execID); err != nil {
return nil, err
}
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 cancelled instead.
if err := checkRevokableStatus(execution); err != nil {
return nil, err
}

// Three-mode dispatch — same shape as cancelPurchase.
if result, handled, err := h.tryRevokeViaSession(ctx, req, execution); handled {
return result, err
}

if token == "" {
return nil, NewClientError(401, "sign in or use the revocation link from the notification email")
}

// Token-authed path: validate the token (reusing the approval token for
// this iteration) and confirm the caller is the authorised 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)
}
Comment on lines +681 to +685
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce token expiry/window on revoke token path

Line 681 validates token equality, but never checks token/window expiry before accepting revocation. With current logic, an old completed execution can still be marked revocation_requested as long as the token matches.

Suggested fix
 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: revocation link has expired", execution.ExecutionID))
+	}
 	if subtle.ConstantTimeCompare([]byte(execution.ApprovalToken), []byte(token)) != 1 {
 		return NewClientError(403, "invalid revocation token")
 	}
 	return nil
 }

Also applies to: 733-741

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/handler_purchases.go` around lines 681 - 685, The current revoke
path calls validateRevokeToken(execution, token) but never verifies the
token/window expiry, allowing old executions to be marked revocation_requested;
update the revoke flow to enforce expiry by adding an expiry/window check before
calling revokeViaSession: either extend validateRevokeToken to also validate
token expiration and execution window timestamps (e.g., compare
execution.RevokeTokenExpiry or execution.WindowEnd against time.Now()) or call a
new helper (e.g., validateRevokeTokenExpiry or isRevokeWindowValid) after
validateRevokeToken and before revokeViaSession(ctx, execution, actor), and
return an error if the token/window is expired; apply the same change to the
other revoke path handling lines ~733-741 so both code paths enforce expiry.


// tryRevokeViaSession attempts the session-authenticated branch of the
// revokePurchase 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
// revokePurchase to keep that function under the cyclomatic limit.
func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution) (any, bool, error) {
session := h.tryGetSession(ctx, req)
if session == nil {
return nil, false, nil
}
switch sessErr := h.authorizeSessionCancel(ctx, session, execution); {
case sessErr == nil:
result, err := h.revokeViaSession(ctx, execution, session.Email)
return result, true, err
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 revokePurchase 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 and that it matches the supplied token using constant-time
// comparison (same guard as ApproveExecution in internal/purchase/approvals.go).
// Extracted from revokePurchase 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 subtle.ConstantTimeCompare([]byte(execution.ApprovalToken), []byte(token)) != 1 {
return NewClientError(403, "invalid revocation token")
}
return nil
}

// 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.
func (h *Handler) revokeViaSession(ctx context.Context, execution *config.PurchaseExecution, revokedBy string) (any, error) {
execution.Status = "revocation_requested"
if revokedBy != "" {
rb := revokedBy
execution.CancelledBy = &rb
}
if err := h.config.SavePurchaseExecution(ctx, execution); err != nil {
return nil, fmt.Errorf("failed to record revocation request for execution %s: %w", execution.ExecutionID, err)
}
Comment on lines +748 to +756
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use an atomic status transition for revocation_requested

revokeViaSession mutates the loaded struct then calls SavePurchaseExecution, which is vulnerable to race/lost-update behavior if the row changes between read and write. This should use a conditional transition (completed|partially_completed -> revocation_requested) like other state transitions.

Suggested fix
 func (h *Handler) revokeViaSession(ctx context.Context, execution *config.PurchaseExecution, revokedBy string) (any, error) {
-	execution.Status = "revocation_requested"
-	if revokedBy != "" {
-		rb := revokedBy
-		execution.CancelledBy = &rb
-	}
-	if err := h.config.SavePurchaseExecution(ctx, execution); err != nil {
-		return nil, fmt.Errorf("failed to record revocation request for execution %s: %w", execution.ExecutionID, err)
-	}
+	updated, err := h.config.TransitionExecutionStatus(ctx, execution.ExecutionID, []string{"completed", "partially_completed"}, "revocation_requested")
+	if err != nil {
+		return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be revoked: %v", execution.ExecutionID, err))
+	}
+	if revokedBy != "" {
+		rb := revokedBy
+		updated.CancelledBy = &rb
+		if err := h.config.SavePurchaseExecution(ctx, updated); err != nil {
+			return nil, fmt.Errorf("failed to record revocation requester for execution %s: %w", execution.ExecutionID, err)
+		}
+	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/handler_purchases.go` around lines 748 - 756, revokeViaSession
currently mutates execution.Status and calls h.config.SavePurchaseExecution,
which can cause lost-updates; change it to perform an atomic/conditional status
transition that only sets Status = "revocation_requested" when the current
status is one of the allowed predecessors ("completed" or
"partially_completed"). Locate revokeViaSession and replace the blind
mutation+SavePurchaseExecution with a single conditional update call (or add a
new config method) that issues an UPDATE ... WHERE execution_id = ? AND status
IN ('completed','partially_completed') (or equivalent ORM/Repo call) and returns
a clear error when no rows were updated; ensure CancelledBy is included in the
same atomic update and preserve existing SavePurchaseExecution behavior for
other flows.

logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, revokedBy)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid logging raw actor email in revoke path

Line 757 logs revokedBy directly. That leaks PII into logs and conflicts with the PR’s stated PII-safe logging behavior.

Suggested fix
-	logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, revokedBy)
+	logging.Infof("Revocation requested for execution %s (actor_present=%t)", execution.ExecutionID, revokedBy != "")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, revokedBy)
logging.Infof("Revocation requested for execution %s (actor_present=%t)", execution.ExecutionID, revokedBy != "")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/handler_purchases.go` at line 757, The log call currently prints
the raw actor email via revokedBy in logging.Infof("Revocation requested for
execution %s by %s", execution.ExecutionID, revokedBy); change this to emit a
PII-safe identifier instead (e.g., mask or hash the email or log the actor's
internal ID). Update the call to use a helper like redactEmail(revokedBy) or
hashActor(revokedBy) (create such a helper if missing) so the message becomes
logging.Infof("Revocation requested for execution %s by %s",
execution.ExecutionID, redactEmail(revokedBy)); ensure redactEmail/hashActor is
deterministic and irreversible to avoid leaking the actual email.

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.
Expand Down Expand Up @@ -1655,6 +1805,180 @@ 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, requesterName := 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 _, rec := range execution.Recommendations {
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,
RequestedByEmail: requesterEmail,
RequestedByName: requesterName,
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 (and name, currently always "") for
// the user who originally submitted the execution. The lookup is non-fatal:
// when auth is unavailable or the user cannot be found, both fields are
// returned empty and the notification is sent without them. Extracted from
// sendPurchaseExecutedEmail to keep that function under the cyclomatic limit.
func (h *Handler) lookupRequesterInfo(ctx context.Context, execution *config.PurchaseExecution) (email, name string) {
if h.auth == nil || execution.CreatedByUserID == nil || *execution.CreatedByUserID == "" {
return "", ""
}
u, err := h.auth.GetUser(ctx, *execution.CreatedByUserID)
if err == nil && u != nil {
return u.Email, ""
}
return "", ""
}

// 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
Expand Down
Loading
Loading