fix(api): scope revoke-any to the caller's accounts on executed purchases - #491
Conversation
…ases authorizeSessionRevoke returned early for revoke-any, so a user holding revoke-any but restricted to some cloud accounts could quote and return an executed Azure reservation in an account outside their scope. The scheduled-execution revoke path already gates on requireExecutionAccess (issue #92); the purchase-history path now applies the same account scope to revoke-any as to revoke-own. Only an unrestricted revoke-any caller may revoke a row with no cloud account association; scoped callers fail closed on it. Refs #386
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reached
This review includes 1 billable file and costs up to $0.25.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 36 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughRevocation authorization now applies account-scope checks to ChangesPurchase revocation authorization
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change closes a gap where callers restricted to specific cloud accounts could revoke purchases outside their scope. Tests cover the scoped and unrestricted cases. No merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
The comment named the removed checkRevokeOwnAccountAccess and said admins revoke unattributed rows via revoke-any. Only an unrestricted revoke-any caller can, per checkRevokeAccountAccess.
What
Fixes item 4 of #386:
authorizeSessionRevoke(purchase-history revoke and the revoke quote endpoint) returned early forrevoke-any, so a user holdingrevoke-anybut restricted to some cloud accounts could quote and return an executed Azure reservation in an account outside their scope. Same class as #92.revoke-anynow lifts only the ownership requirement, not the account scope. Therevoke-anyandrevoke-ownbranches share one check (checkRevokeAccountAccess):revoke-any: allowed, including rows with nocloud_account_id(admins keep revoking legacy rows);revoke-any: the row's account must be in scope; a nil or emptycloud_account_idfails closed with 403;revoke-own: unchanged (nil account is a 403 even for unrestricted scope; out of scope is a 403).The stateless admin API key still short-circuits before any lookup.
How verified
GOTOOLCHAIN=go1.26.6 GOWORK=off GOFLAGS='-p=2 -count=1' AWS_EC2_METADATA_DISABLED=trueNew tests:
TestAuthorizeSessionRevoke_RevokeAny_AccountScope: scopedrevoke-anyuser against in-scope, other-account, nil and empty account rows.TestRevokePurchase_RevokeAnyOutOfScopeNeverCallsAzure: drivesrevokePurchase(mocked store/auth, stubbed Azure client factory) for a scopedrevoke-anyuser against an Azure purchase in another account; asserts 403 and that no Azure client is built.On main (fix reverted, tests kept):
(The
in_scopesubtest and the two adjusted existing tests also fail on main, but only because main never callsGetAllowedAccountsAPIon therevoke-anypath, so the mock expectation goes unmet.)With the fix:
go test ./internal/api/ ./internal/auth/passes (3268 tests).go build ./...,go vet ./internal/api/,go vet -tags integration ./internal/api/andgolangci-lint run ./internal/api/...are clean.Mutation check: changing the guard to
revokeAny || scope.AllowsAll()(revoke-anyskips scope again, unrestricted own skips attribution) failsother_account,nil_account,empty_account, the endpoint test andTestAuthorizeSessionRevoke_RevokeOwn_NilAccountID.These are mock-based unit tests. No real Azure return or purchase was performed.
Deliberately left for follow-up (from #386)
UpdatePurchasePlanTxnow updatesWHERE id = $1 AND updated_at = $8, andupdatePlanpasses the snapshot'sUpdatedAt, so a step completing between read and write produces a 409 instead of a staleCurrentStep.ExecutedAt/ExecutedByUserIDstamping, hardcoded"completed"status): still open on main (handler_purchases.gomaps everyRunPlannedPurchaseNowerror to 409 and returns"status": "completed"). It is a separate concern and gets its own PR.Reviewer notes
scope.Allows(id, "")matches by account ID only, as the existingrevoke-owncheck did. A scopedrevoke-anyuser whose allow-list names accounts by display name will now get a 403 on those rows. That fails closed, but say if you want the name resolved (lookupCloudAccount) here.Refs #386
Summary by CodeRabbit