fix(api/purchases): reject approval of non-AWS execution with deleted account (closes #609)#613
Conversation
… account (closes #609) Add a preflight guard in both approve entry points — the HTTP handler (approvePurchaseViaSession and the token path via approvePurchase) and the token-authenticated ApproveExecution in the purchase package — that rejects an approval request with a clear 409 when the execution's CloudAccountID is nil and the provider is non-AWS. AWS executions with a nil CloudAccountID are left through because the ambient-host-account fallback from PR #607/#604 handles them correctly. Without this guard the failure surfaces deep in the cloud SDK as an opaque chained-credential error (IMDS endpoint, missing env vars, CLIs absent from the Lambda runtime) that gives the operator no indication that the underlying account was deleted. Empty provider is treated as AWS (legacy rows pre-dating multi-cloud support) to preserve backward compatibility. Regression tests: Azure orphan 409, GCP orphan 409, AWS orphan falls through, non-orphan with valid account proceeds normally; plus ApproveExecution (token path) Azure orphan and AWS orphan cases.
cristim
added
triaged
|
@coderabbitai review |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR implements an orphan account guard that prevents approval of non-AWS executions lacking a CloudAccountID. The service layer validates the condition and returns a descriptive 409 error; the handler layer integrates this check via a new helper. Regression tests cover Azure/GCP rejection, AWS fallthrough, and non-orphan passthrough at both layers. ChangesOrphan Account Guard for Purchase Approvals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@internal/api/handler_purchases.go`:
- Around line 266-290: The orphan-check logic in loadApproveExecution duplicates
purchase.orphanExecutionError; remove the duplicated predicate/error
construction and call the centralized function instead (or extract the predicate
into a shared helper used by both). Specifically, replace the manual block in
loadApproveExecution that inspects execution.CloudAccountID and
execution.Recommendations with a call to purchase.orphanExecutionError (or the
new shared helper) to get the ClientError (or nil), return it if non-nil, and
otherwise return execution; ensure the function you call is exported or
accessible from internal/api, and preserve the same error semantics (HTTP 409
ClientError) and use execution.ExecutionID and provider info from the execution
when building the error via the shared routine.
In `@internal/purchase/approvals.go`:
- Around line 67-79: The orphanExecutionError currently treats executions with
nil execution.CloudAccountID as orphaned for non-AWS providers even when
recommendations carry account IDs; update orphanExecutionError to, after
obtaining provider := execution.Recommendations[0].Provider and confirming
provider is non-empty and not "aws", iterate execution.Recommendations and if
any recommendation has a non-nil CloudAccountID (e.g., rec.CloudAccountID)
return nil (not orphan); only return the fmt.Errorf referencing
execution.ExecutionID and provider if no recommendation-level CloudAccountID is
found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f23bdb42-589b-4336-af6e-41b9d74a6d12
📒 Files selected for processing (4)
internal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/purchase/approvals.gointernal/purchase/approvals_test.go
…level account id (CR pass-1) Export OrphanExecutionError from internal/purchase and call it from internal/api/handler_purchases.go, removing the duplicated predicate in loadApproveExecution (CR actionable 1). Update OrphanExecutionError to treat an execution as NOT orphaned when any individual recommendation carries a non-nil CloudAccountID, which covers the multi-rec fan-out path where the execution-level field is intentionally nil while recs still name their target accounts (CR actionable 2). Add TestOrphanExecutionError_RecLevelAccountIDPreventsOrphan as a regression guard for the new rec-level check.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
approvePurchasein the HTTP handler andApproveExecutionin the purchase package) that returns a clear409with a descriptive message when an execution'sCloudAccountIDisniland the provider is non-AWSnilCloudAccountIDare left through (ambient-host-account fallback from PR fix(scheduler/aws): tag ambient-path rec rows with registered host-account UUID (closes #604) #607/fix(scheduler/aws): tag rec rows with registered host-account UUID instead of nil when only ambient AWS account is configured #604 handles them); empty provider is treated as AWS (legacy rows)loadApproveExecution(handler) andorphanExecutionError(purchase package) helpers to stay below the gocyclo-10 thresholdFix approach
Without this guard, approving an orphan Azure/GCP execution surfaces an opaque chained-credential error from the cloud SDK (IMDS endpoint, missing env vars, CLIs absent from Lambda runtime). The
409lets the frontend show a useful toast directing the operator to cancel the purchase instead.Test plan
TestHandler_approvePurchase_AzureOrphanRejects409- Azure orphan returns 409 with descriptive messageTestHandler_approvePurchase_GCPOrphanRejects409- GCP orphan returns same 409TestHandler_approvePurchase_AWSOrphanFallsThrough- AWS orphan does NOT trigger the guardTestHandler_approvePurchase_NonOrphanUnchanged- execution with valid account proceeds normallyTestManager_ApproveExecution_NonAWSOrphanReturnsError- token path (SQS worker / email link) Azure orphan returns errorTestManager_ApproveExecution_AWSOrphanFallsThrough- token path AWS orphan falls throughSummary by CodeRabbit
Bug Fixes
Tests