Skip to content

fix(api): scope revoke-any to the caller's accounts on executed purchases - #491

Merged
cristim merged 2 commits into
mainfrom
fix/plan-run-now-revoke-followups
Oct 5, 2026
Merged

cristim merged 2 commits into
mainfrom
fix/plan-run-now-revoke-followups

Conversation

@cristim

@cristim cristim commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

What

Fixes item 4 of #386: authorizeSessionRevoke (purchase-history revoke and the revoke quote endpoint) 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. Same class as #92.

revoke-any now lifts only the ownership requirement, not the account scope. The revoke-any and revoke-own branches share one check (checkRevokeAccountAccess):

  • unrestricted scope + revoke-any: allowed, including rows with no cloud_account_id (admins keep revoking legacy rows);
  • scoped revoke-any: the row's account must be in scope; a nil or empty cloud_account_id fails 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=true

New tests:

  • TestAuthorizeSessionRevoke_RevokeAny_AccountScope: scoped revoke-any user against in-scope, other-account, nil and empty account rows.
  • TestRevokePurchase_RevokeAnyOutOfScopeNeverCallsAzure: drives revokePurchase (mocked store/auth, stubbed Azure client factory) for a scoped revoke-any user against an Azure purchase in another account; asserts 403 and that no Azure client is built.

On main (fix reverted, tests kept):

--- FAIL: TestAuthorizeSessionRevoke_RevokeAny_AccountScope/other_account
        Messages: expected ClientError, got <nil>: <nil>
--- FAIL: TestAuthorizeSessionRevoke_RevokeAny_AccountScope/nil_account
--- FAIL: TestAuthorizeSessionRevoke_RevokeAny_AccountScope/empty_account
--- FAIL: TestRevokePurchase_RevokeAnyOutOfScopeNeverCallsAzure
        Messages: expected ClientError, got *fmt.wrapError: revoke azure: obtain credential: azure must not be reached

(The in_scope subtest and the two adjusted existing tests also fail on main, but only because main never calls GetAllowedAccountsAPI on the revoke-any path, 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/ and golangci-lint run ./internal/api/... are clean.

Mutation check: changing the guard to revokeAny || scope.AllowsAll() (revoke-any skips scope again, unrestricted own skips attribution) fails other_account, nil_account, empty_account, the endpoint test and TestAuthorizeSessionRevoke_RevokeOwn_NilAccountID.

These are mock-based unit tests. No real Azure return or purchase was performed.

Deliberately left for follow-up (from #386)

  • Item 1 (in-progress ramp shape change restarts at step 0): needs an owner decision between carrying completed percentage into the new schedule and rejecting shape changes with 409. Not touched.
  • Item 2 (updatePlan read-modify-write outside the ramp lock): appears covered by fix(purchase): preserve ramp progress across stale plan updates #464. UpdatePurchasePlanTx now updates WHERE id = $1 AND updated_at = $8, and updatePlan passes the snapshot's UpdatedAt, so a step completing between read and write produces a 409 instead of a stale CurrentStep.
  • Item 3 (run-now error mapping, ExecutedAt/ExecutedByUserID stamping, hardcoded "completed" status): still open on main (handler_purchases.go maps every RunPlannedPurchaseNow error 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 existing revoke-own check did. A scoped revoke-any user 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

  • Bug Fixes
    • Purchase revocation now respects account access restrictions, including for users with broad revocation permissions.
    • Revocations are denied when a purchase has no associated cloud account or falls outside the caller’s allowed accounts.

…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
@cristim cristim added urgency/this-sprint Within the current sprint triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm impact/many Affects most users effort/m Days type/bug Defect labels Oct 5, 2026
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

  • Run on-demand review

This review includes 1 billable file and costs up to $0.25.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

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.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 51ba9325-74fb-4349-ac33-8923d0f42fc0
📥 Commits

Reviewing files that changed from the base of the PR and between 8761425 and 4e08927.

📒 Files selected for processing (1)
  • internal/auth/types.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: ac108239-6e5c-4913-8ef5-e7fad53da878
📥 Commits

Reviewing files that changed from the base of the PR and between 115ff70 and 8761425.

📒 Files selected for processing (2)
  • internal/api/handler_purchases_revoke.go
  • internal/api/handler_purchases_revoke_test.go

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.


📝 Walkthrough

Walkthrough

Revocation authorization now applies account-scope checks to revoke-any callers unless their scope allows all accounts. Tests cover account associations and confirm that an out-of-scope Azure purchase is rejected before Azure credentials are created.

Changes

Purchase revocation authorization

Layer / File(s) Summary
Account-scope authorization and validation
internal/api/handler_purchases_revoke.go, internal/api/handler_purchases_revoke_test.go
revoke-any callers are subject to account-scope checks unless their scope allows all accounts. Tests cover matching and out-of-scope accounts, nil and empty associations, and rejection of an out-of-scope Azure purchase before credential creation.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 87614

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: applying account scope to revoke-any for executed purchases.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Full 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.
@cristim
cristim merged commit d384d47 into main Oct 5, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant