sec(api): apply allowed_accounts scope to RI exchange listing endpoints (closes #1030)

[cristim]

Summary

• listConvertibleRIs, getRIUtilization, and getReshapeRecommendations gated on view:purchases but never applied the session's allowed_accounts filter, so a restricted user could see the full convertible-RI inventory and utilization for accounts outside their scope.
• getRIExchangeHistory (the sibling handler) already applied this filter; the three listing handlers now match that behaviour exactly.
• The fix resolves the deployment CloudAccount UUID via resolveAWSCloudAccountID (test-injectable via the existing reshapeAccountResolver field) and returns an empty typed response when the deployment account is outside the session's allowed_accounts.
• Six new regression tests in handler_per_account_perms_test.go cover the out-of-scope (returns empty) and admin/in-scope (bypasses gate) paths for all three handlers.

Closes #1030

Test plan

• go test ./internal/api/... -count=1 passes (1501 tests, no regressions)
• New tests TestPerAccountPerms_ListConvertibleRIs_ScopedOutsideAllowedReturnsEmpty confirms empty list returned for scoped user outside allowed accounts
• New tests TestPerAccountPerms_GetRIUtilization_ScopedOutsideAllowedReturnsEmpty confirms same for utilization endpoint
• New tests TestPerAccountPerms_GetReshapeRecommendations_ScopedOutsideAllowedReturnsEmpty confirms same for reshape recommendations
• New admin positive tests confirm IsUnrestrictedAccess bypasses the scope check for admins
• go vet ./internal/api/... clean

Scope expanded

Additional findings from FOLD-1042 (docs/code-review/16-remaining-findings-plan.md):

01-L4 -- region defaults to hardcoded us-east-1 in quote/execute (finding 01-L4)

getExchangeQuote and executeExchange silently defaulted an absent region to
the hardcoded string us-east-1, while getReshapeRecommendations carefully
resolved the region from the AWS SDK configuration chain. On executeExchange
(a financially irreversible mutation), a wrong-region default could exchange RIs
in a region where none of the deployment's RIs live.

Fixes:

• getExchangeQuote: resolves region from the AWS SDK chain via
  loadAWSConfigWithRegion when the caller omits it (matches getReshapeRecommendations).
• executeExchange: returns 400 when region is absent from the body.
• executeApprovedExchange: fails the exchange via failExchange when
  record.Region is empty; the region must be captured at record-creation time.

Three regression tests pin each path. 04-D4 (nit: unify config.RIExchangeRecord
and exchange.ExchangeRecord) is skipped -- the two types must remain separate
because pkg/exchange is a distinct Go module and cannot import internal/config
(see pkg/exchange/auto.go:13-15). Documented in
docs/code-review/open-questions/fold-1042.md.

Also closes #1060

Summary by CodeRabbit

• New Features

  • RI-exchange endpoints now enforce account-level access control to ensure users only see data for authorized accounts.

• Bug Fixes

  • Improved region handling in exchange operations with stricter validation for missing or invalid region values.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 679dbc8f-ebb7-4b8b-b104-5d44f1d44dc2

📥 Commits

Reviewing files that changed from the base of the PR and between cbdc4be and aef7307.

📒 Files selected for processing (4)

• internal/api/handler_per_account_perms_test.go
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_test.go
• pkg/exchange/reshape_crossfamily_test.go

---

📝 Walkthrough

Walkthrough

Handlers listConvertibleRIs, getRIUtilization, and getReshapeRecommendations now fetch the session’s allowed_accounts and, for restricted sessions, resolve the deployment/reshape cloud account and return empty typed responses when out-of-scope. Exchange flows now require/resolve region explicitly and remove us-east-1 fallbacks; tests were added/updated for both scoping and region behaviors.

Changes

RI-exchange Scope Enforcement & Region Validation

Layer / File(s)	Summary
Handler helpers and scope enforcement internal/api/handler_ri_exchange.go	Adds helpers to resolve reshape cloud account IDs and check allowed_accounts; applies scope checks in listConvertibleRIs, getRIUtilization, and getReshapeRecommendations; refactors reshape request parsing and normalizes omitted region to the AWS config region.
Exchange region validation & execution guards internal/api/handler_ri_exchange.go	getExchangeQuote resolves region from the AWS config; validateExecuteExchangeBody requires body.Region (400 if missing); executeExchange no longer falls back to us-east-1; executeApprovedExchange fails when stored record.Region is empty.
Per-account permission tests (RI-exchange) internal/api/handler_per_account_perms_test.go	Expands documented endpoints from 9→12 and adds tests asserting scoped-out sessions receive empty typed responses for /ri-exchange/instances, /ri-exchange/utilization, and /ri-exchange/reshape-recommendations, and that admin sessions bypass the scope gate.
Exchange region regression tests internal/api/handler_ri_exchange_test.go	Adds tests: TestExecuteExchange_EmptyRegionReturns400, TestGetExchangeQuote_EmptyRegionResolvesFromSDK, TestExecuteApprovedExchange_EmptyRecordRegionFails; updates an existing validation test to include region.
Reshape tests formatting-only changes pkg/exchange/reshape_crossfamily_test.go	Whitespace and alignment-only formatting updates in composite-score test fixtures; no logic changes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• LeanerCloud/CUDly#309: Expands the per-account permission regression suite with endpoint-specific scoped vs admin coverage similar to these test additions.
• LeanerCloud/CUDly#230: Overlaps on reshape region handling and tests that rely on SDK/config-region resolution.

Poem

🐰 I hopped through handlers, sniffed the air,

Scoped accounts now checked with care,
Regions asked, no defaults blind,
Tests ensure the rules they find,
A little hop, a safer line.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'sec(api): apply allowed_accounts scope to RI exchange listing endpoints (closes #1030)' clearly summarizes the main change: enforcing account scoping on RI exchange listing handlers, with a reference to the primary issue.
Linked Issues check	✅ Passed	All code requirements from #1030 (scoped-out empty responses, allowed_accounts filtering) and #1060 (region handling, 400 validation, SDK resolution, fail-fast approval) are implemented with corresponding regression tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to #1030 (allowed_accounts filtering on RI-exchange handlers) and #1060 (region handling). The whitespace-only reformatting in reshape_crossfamily_test.go is incidental code-health cleanup.
Docstring Coverage	✅ Passed	Docstring coverage is 89.47% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sec/ri-exchange-account-scoping

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

FOLD-1042 additions

Added commits for the two findings in the FOLD-1042 bucket (docs/code-review/16-remaining-findings-plan.md):

01-L4 -- region hardcoded to us-east-1 in quote/execute (Low)

Fix: commit 2d6868d

Three changes in handler_ri_exchange.go:

• getExchangeQuote: resolves region from the AWS SDK chain via loadAWSConfigWithRegion(ctx, body.Region) when the caller omits it, matching getReshapeRecommendations instead of hardcoding us-east-1.
• executeExchange / validateExecuteExchangeBody: returns 400 when region is absent from the body. RI exchanges are financially irreversible; a wrong-region execute could exchange RIs in a region the deployment does not use.
• executeApprovedExchange: calls failExchange when record.Region == "" instead of defaulting to us-east-1. The region must have been captured at record-creation time; proceeding without it is unsafe.

Three regression tests added:

• TestExecuteExchange_EmptyRegionReturns400 -- execute rejects missing region
• TestGetExchangeQuote_EmptyRegionResolvesFromSDK -- quote resolves from SDK chain
• TestExecuteApprovedExchange_EmptyRecordRegionFails -- empty stored region fails the approved exchange

1501 tests pass, go vet clean.

04-D4 -- unify config.RIExchangeRecord / exchange.ExchangeRecord (Nit/opt)

Skipped. The two types must remain separate: pkg/exchange is a distinct Go module and cannot import internal/config (module boundary; see pkg/exchange/auto.go:13-15). Documented in docs/code-review/open-questions/fold-1042.md.

---

Also closes #1060.

@coderabbitai review

[coderabbitai]

@cristim, noted — the FOLD-1042 additions look well-reasoned:

• 01-L4 fix (commit 2d6868d): The fail-fast approach for executeExchange/executeApprovedExchange on a missing region is the correct choice given the financial irreversibility of RI exchanges. Resolving from the SDK chain for getExchangeQuote (matching getReshapeRecommendations) is a nice consistency win.
• 04-D4 skip: The module-boundary justification is valid — pkg/exchange as a separate Go module cannot import internal/config without creating a circular dependency or violating module isolation. Documenting it in the open-questions folder is the right call.

Triggering a review to cover the new commits.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T07:04:28.213Z
Learning: In LeanerCloud/CUDly, PR `#907` (`feat(auth): group-membership-only authorization, remove roles, require >=1 group`) landed on `feat/multicloud-web-frontend` and removed `Session.Role` and `User.Role` entirely. Any `Principal` struct must use `GroupIDs []string` (or similar group-based field) instead of `Role string`. The `*auth.User` type no longer exposes `GetRole()`, `GetID()`, or `GetEmail()` accessors via interface assertion — using those in a type assertion causes a silent always-false fail-closed regression at runtime. The group-membership model also requires >=1 group for a valid user session; zero-groups users must be rejected at the authentication layer. Follow-up design issue is `#1010`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.