Repository navigation
fix(api): scope Azure exchangeable-RI listing to session's allowed accounts - #1711
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Azure exchangeable-reservation listing now enforces subscription scope and filters tenant-wide results by registered billing scopes. Reservations without resolvable scopes are excluded. Regression tests cover scoped, empty, and out-of-scope responses. ChangesAzure RI exchange authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant listExchangeableAzureRIs
participant SessionScope
participant AzureClient
participant AzureAccountRegistry
Caller->>listExchangeableAzureRIs: Request Azure exchangeable reservations
alt subscription_id supplied
listExchangeableAzureRIs->>SessionScope: Validate subscription scope
alt subscription is out of scope
listExchangeableAzureRIs-->>Caller: errNotFound
else subscription is allowed
listExchangeableAzureRIs->>AzureClient: List reservations
AzureClient-->>listExchangeableAzureRIs: Reservations
listExchangeableAzureRIs-->>Caller: Matching BillingScopeID rows
end
else subscription_id omitted
listExchangeableAzureRIs->>AzureClient: List tenant-wide reservations
AzureClient-->>listExchangeableAzureRIs: Reservations
listExchangeableAzureRIs->>AzureAccountRegistry: Resolve BillingScopeID values
AzureAccountRegistry-->>listExchangeableAzureRIs: Registered account scopes
listExchangeableAzureRIs-->>Caller: Reservations matching allowed accounts
end
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/handler_ri_exchange_test.go (1)
951-967: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that
ListExchangeableReservationsis not called.
ownsAzureSourceregisters this method with.Maybe(), soAssertExpectationspasses whether it is called or not. AddopsClient.AssertNotCalled(t, "ListExchangeableReservations")after the assertions.🤖 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_ri_exchange_test.go` around lines 951 - 967, The test should explicitly verify that the out-of-scope request never invokes ListExchangeableReservations. In the test around listExchangeableAzureRIs, add opsClient.AssertNotCalled(t, "ListExchangeableReservations") after the existing error assertions, while retaining the current ownsAzureSource and cleanup behavior.
🤖 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.
Nitpick comments:
In `@internal/api/handler_ri_exchange_test.go`:
- Around line 951-967: The test should explicitly verify that the out-of-scope
request never invokes ListExchangeableReservations. In the test around
listExchangeableAzureRIs, add opsClient.AssertNotCalled(t,
"ListExchangeableReservations") after the existing error assertions, while
retaining the current ownsAzureSource and cleanup behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc762f90-97e7-4a04-89d1-c079fcde3ccc
📒 Files selected for processing (2)
internal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_test.go
Adversarial review — PR #1711 @
|
| Gate | Result |
|---|---|
go build ./... |
exit 0 |
go vet ./... |
exit 0 |
go test ./... |
exit 0, 41 packages |
go test ./internal/api/ |
2028 passed |
golangci-lint run --timeout=10m ./... @ v2.10.1 (CI-pinned; binary version asserted before use) |
exit 0, 0 issues. |
gocyclo -over 10 -ignore "_test\.go" ./internal/api |
clean (no output) |
Findings
F1 — Medium. The UI sends a CloudAccount UUID as subscription_id; the new gate turns that into a 404 for every scoped user
The topbar account chip's option value is the CloudAccount UUID, not the Azure subscription ID:
frontend/src/topbar-filters.ts:107—value: a.idinternal/api/handler_accounts.go:180—AccountSummary{ID: acct.ID, ...}(UUID), withExternalIDcarried as a separate fieldfrontend/src/riexchange.ts:65→:303—accountIDs[0]is passed straight toapi.listExchangeableAzureRIs(accountID)frontend/src/api/riexchange.ts:41— placed into?subscription_id=
Before this PR, buildAzureExchangeClient looked that UUID up as an external_id, found no row, and returned the graceful empty 200. After this PR, handler_ri_exchange.go:282-286 runs requireAzureSubscriptionScope first, which resolves the same UUID to no account and returns errNotFound → the Azure RI-Exchange tab renders Failed to load Azure reservations: not found for every restricted session that selects an account chip. Admin sessions are unaffected (the scope check short-circuits on IsUnrestrictedAccess).
Verified by execution, not by reading: a throwaway test driving listExchangeableAzureRIs through the real buildAzureExchangeClient (no factory injection) with subscription_id = a UUID returned errNotFound for a scoped session and an empty 200 for an unrestricted one.
The 404 is the correct security posture given the handler's contract — the real defect is the frontend sending the wrong identifier, and the listing was already returning empty for chip-selected accounts before this PR. So this changes silently-broken into loudly-broken. That is arguably an improvement, but it is a user-visible behaviour change this PR does not mention. Suggested: file a follow-up issue for the FE/BE identifier contract (a.id vs a.external_id on this one call) and note the transition in the PR body.
F2 — Medium. The bulk of the new code is production-unreachable; the reachable filter is untested
azureExchangeFactory is declared at internal/api/handler.go:87 and assigned only in tests (grep over internal/ cmd/ excluding _test.go finds no writer). So in production, with no subscription_id, buildAzureExchangeClient calls GetCloudAccountByExternalID(ctx, "azure", ""), whose query is WHERE ca.provider = $1 AND ca.external_id = $2 (internal/config/store_postgres.go:3056), matches no row, and the handler returns the empty short-circuit at handler_ri_exchange.go:292-297 — before the listing call and therefore before the filter.
That makes filterAzureReservationsByScope (:356), azureScopeIndex (:390) and filterReservationsByScopeIndex (:406) unreachable in production. Meanwhile filterAzureReservationsBySubscription (:328) — the only filter on the path production actually takes — has zero coverage: no test names it, and TestListExchangeableAzureRIs_SubscriptionIDOutOfScope stops at the scope gate without ever reaching it.
Verified by execution: the same throwaway test, with a ListCloudAccountsFn tripwire, showed the no-subscription_id path never called ListCloudAccounts — and the mock's GetAllowedAccountsAPI expectation went unmet, i.e. filterAzureReservationsByScope was never entered.
The doc comment at :259-262 describes the no-parameter branch as the live behaviour. This is exactly the class of claim ("comment asserting cross-file behaviour") that reviews here have been weakest on.
Suggested fix:
- Add a handler-level test for the in-scope
?subscription_id=path asserting only that subscription's rows come back — that is the filter protecting production. - Either state in the doc comment that the no-parameter branch is defence-in-depth for the
external_id = ''account edge case, or confirm the intended reachable surface with the issue author.
F3 — Low. buildAzureExchangeClient's doc states a conjunction the code does not implement
handler_ri_exchange.go:196-198: "subscriptionID is empty AND no Azure accounts are registered at all". There is no such conjunction — an empty subscriptionID yields a nil client unconditionally, whatever is registered. Pre-existing, but this PR restates it verbatim at :271-273, where it now carries weight (it is what makes the no-parameter branch look reachable). Suggested: "subscriptionID is empty: no account can be resolved, so the caller returns an empty list."
F4 — Low. Two different comparison methods for the same discriminator, and a case-collision in the index
filterAzureReservationsBySubscription:332 uses strings.EqualFold; azureScopeIndex:396 / filterReservationsByScopeIndex:409 use a strings.ToLower-keyed map. They agree for ASCII. They diverge on the Turkish dotted I — strings.ToLower("İ") == "i" but strings.EqualFold("İ", "i") == false — and the ToLower path is the more permissive of the two, so an account whose ExternalID carried such a character would index a scope EqualFold would reject. Azure subscription IDs are GUIDs, so this is not reachable from Azure-sourced data; ExternalID is operator-supplied. Not a live hole.
Separately: cloud_accounts declares UNIQUE(provider, external_id) (migration 000011_cloud_accounts.up.sql:38 — verified, not taken from the comment), and Postgres compares that case-sensitively, so two rows differing only in case can coexist. azureScopeIndex collapses them and last-write-wins; if the surviving entry is not the one in allowed_accounts, the user's own rows are dropped. Fails closed, so it is an availability nit rather than a leak.
Suggested fix, which removes both: mirror filterDashboardRecommendations more closely — iterate the accounts, keep the ones auth.MatchesAccount passes, and build a map[string]struct{} of allowed lower-cased scopes. Same cyclomatic cost, no collision semantics, one comparison rule.
F5 — Nit. Duplicated comment
The "Counts, not the dropped reservation/billing-scope identifiers, are safe to log…" paragraph appears twice: in the doc comment (:351-355) and inline (:372-374).
F6 — Nit (optional). provider := "azure" at :365
common.ProviderAzure is used in this same file at :1050. That said, exchange_lookup.go:187 and handler_accounts.go:1508 both use the bare literal for exactly this filter, so the new code matches local convention. Flagging only for completeness.
F7 — Nit. Admin passthrough can emit null
:362 returns the client's slice unchanged for unrestricted sessions, which can be nil → {"reservations": null}, while every other path emits []. Admin-only, and the frontend does resp.reservations ?? [].
F8 — Not this PR, but it is what is red
Security Scanning fails at npm audit --audit-level=high: brace-expansion GHSA-rgw5-rvv9-x895 and fast-uri GHSA-7p8r-x3mc-p8w7. That step's exit 1 aborted the job before gosec and Trivy wrote their SARIF, which is why the visible errors are the two Path does not exist: *.sarif uploads. The branch is 0 behind origin/main, so these advisories are on main too (main's last green ci.yml run predates them). Needs its own npm audit fix PR; nothing here to change.
Hypotheses tested and cleared
- Is the filter an oracle? No.
requireAzureSubscriptionScope:792treatsaccount == niland!MatchesAccount(...)identically, returning the sameerrNotFoundsentinel (handler_router.go:19-26,Error() == "not found", 404) with no distinguishing text, and both cost exactly oneGetCloudAccountByExternalID. The gate runs at:282, beforebuildAzureExchangeClientand the tenant-wide ARM listing, so there is no timing signal from the listing either. - Do the two empty cases look identical? Yes. Every non-admin return path emits a non-nil empty slice —
make([]T, 0, len(...))at:330and:407, and the literal at:296— so all three empty cases serialize as{"reservations":[]}. A scoped caller cannot separate "my subscriptions have zero exchangeable reservations" from "no row could be attributed". - Fail-closed? Yes.
getAllowedAccountsandListCloudAccountserrors are both wrapped and returned; no fall-through to an unfiltered listing. No("", nil)resolver anywhere on this path.h.auth == nilcannot reachgetAllowedAccountshere becauserequirePermissionfails first — asserted by the pre-existingTestListExchangeableAzureRIs_RequiresPermission. - Does empty
allowed_accountsmean "all"? Yes, viaauth.IsUnrestrictedAccess(internal/auth/types.go:157-178) — the repo-wide, explicitly documented 03-L5 fail-open backward-compat default, identical to whatrequireAzureSubscriptionScope,filterDashboardRecommendationsandlistAccountsMinimalalready do. Not this PR's to change. One wording note: the doc comment says "Unrestricted/admin sessions see everything", which hides that "noallowed_accountsconfigured" also lands in that bucket. - Is
IsUnrestrictedAccessthe right admin predicate? Yes — the same predicate the POST siblings on this resource use, so no class of session is treated more broadly here than there. - Reach parity. The
?subscription_id=path enforces strictly more: rows ⊆ {that subscription}, and that subscription ∈allowed_accountsby the gate. The samesubscriptionID != ""condition drives both the gate (:282) and the row filter (:304), so the two axes cannot desynchronize. - Logging.
:376logs onlydroppedandlen(reservations). No reservation IDs, no billing scopes, no account identifiers. - Do the tests fail pre-fix? Yes — verified by execution, not from the report. With
git checkout origin/main -- internal/api/handler_ri_exchange.goand the new tests in place, all three fail: length 2 vs the required 1 (res-bpresent);Should be empty, but was [{res-b …}];An error is expected but got nil. Restored afterwards; worktree clean. - Reuse / proportionality. Mirrors
filterDashboardRecommendations(handler_dashboard.go:169) rather than inventing a third pattern. No new scoping abstraction, no mode enum, no config knob.
Not verified, and why
- Live Azure behaviour — no tenant available. In particular, whether ARM ever returns a
BillingScopeIDcased differently from the registeredExternalID. Both filters are case-insensitive, so the conclusion holds either way. - Whether any non-UI client (CLI, MCP) calls this endpoint with a real subscription GUID. Grep over the repo found only
frontend/src/api/riexchange.tsandinternal/api/router.go:306, so F1's blast radius is the web UI.
No merge action taken.
|
Addressed the adversarial review (F1-F8) and the CodeRabbit nitpick. Pushed as CodeRabbit nitpick ( F2 (Medium, required): fixed. Added F1 (Medium, sizing requested): sized, not attempted. It ripples. The account chip's value ( F3 (Low, optional -- taken): fixed. F4 (Low): not changed, per direction -- confirmed settled: F5/F6 (Nit, duplicated comment): fixed. Removed the inline repeat of the "counts, not identifiers, are safe to log" paragraph; kept the doc-comment copy. F6/other (Nit, bare F7 (Nit): fixed. The admin passthrough in F8: not this PR ( Gates re-run after all of the above: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Requesting a full review, paced to one request per hour across this repo. The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land. Full form rather than incremental: this head was pushed while the quota was exhausted, so its automatic review never ran and is not retried retroactively. An incremental request would skip exactly those commits and report clean on a diff it never read. @coderabbitai full review |
|
✏️ Learnings added
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/handler_ri_exchange_test.go (1)
996-999: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case-variant row to cover the case-insensitive match.
filterAzureReservationsBySubscriptioncompares withstrings.EqualFold, andazureScopeIndexnormalizes withstrings.ToLower. No new test exercises aBillingScopeIDwhose case differs from the registeredExternalID. A regression to an exact==comparison would still pass this suite and would silently drop a caller's own reservations.Add one row whose billing scope uses different casing.
🧪 Proposed test data change
stub := &stubAzureExchangeClient{reservations: []azurecompute.ExchangeableReservation{ {ReservationID: "res-own", BillingScopeID: "/subscriptions/sub-1", SKU: "Standard_D2s_v3", Quantity: 1, Region: "eastus"}, + {ReservationID: "res-own-mixed-case", BillingScopeID: "/Subscriptions/SUB-1", SKU: "Standard_D2s_v3", Quantity: 1, Region: "eastus"}, {ReservationID: "res-other", BillingScopeID: "/subscriptions/sub-2", SKU: "Standard_D4s_v3", Quantity: 5, Region: "westeurope"}, }}Update the length assertion at Line 1014 to
2when you add the row.🤖 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_ri_exchange_test.go` around lines 996 - 999, Add a third reservation to the stubAzureExchangeClient data with the same subscription scope as the registered account but different casing in BillingScopeID, then update the result-length assertion from 1 to 2. Keep the existing distinct-scope reservation unchanged so the test verifies case-insensitive matching while still excluding unrelated reservations.
🤖 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.
Nitpick comments:
In `@internal/api/handler_ri_exchange_test.go`:
- Around line 996-999: Add a third reservation to the stubAzureExchangeClient
data with the same subscription scope as the registered account but different
casing in BillingScopeID, then update the result-length assertion from 1 to 2.
Keep the existing distinct-scope reservation unchanged so the test verifies
case-insensitive matching while still excluding unrelated reservations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98d1b4e5-dfd4-4ff1-aa70-293dc17c9862
📒 Files selected for processing (2)
internal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_test.go
…counts
listExchangeableAzureRIs enumerated every reservation the deployment's
Azure credential could read across the whole tenant, regardless of the
caller's allowed_accounts scope. A caller with view:purchases saw every
subscription's reservations, including ones outside their scope.
Narrow the GET listing the same way the POST siblings (compatible-
offerings, execute) already narrow their own scope:
- subscription_id supplied: requireAzureSubscriptionScope gates access
to that subscription (errNotFound, matching the POST siblings so a
scoped caller cannot probe which subscriptions exist), then the
listing is narrowed to that subscription's own rows via BillingScopeID.
- No subscription_id: the listing is narrowed to whatever the session's
allowed_accounts scope covers, mirroring filterDashboardRecommendations.
Unrestricted/admin sessions pass through unchanged.
Rows with no BillingScopeID, or one matching no registered CloudAccount,
are dropped rather than kept, so an unresolvable scope never degrades to
"show everything" and the two empty-result cases stay indistinguishable.
Split the scope filter into azureScopeIndex and
filterReservationsByScopeIndex to keep filterAzureReservationsByScope
under the project's gocyclo threshold.
Adds regression tests covering: cross-subscription filtering with no
subscription_id, the two indistinguishable-empty cases, and the
out-of-scope subscription_id 404 shape.
Closes #1656
…oping
F2: filterAzureReservationsByScope has no production caller today --
buildAzureExchangeClient resolves subscriptionID via an exact external_id
match, so an empty subscriptionID never resolves a registered account and
the graceful empty-state branch returns before the tenant-wide listing is
ever fetched. Correct the doc comments on listExchangeableAzureRIs and
buildAzureExchangeClient to say so plainly instead of describing the
no-subscription_id branch as live. Add a regression test that exercises
the filter production actually reaches on this path,
filterAzureReservationsBySubscription, against a mix of in-scope and
out-of-scope rows.
F7: normalize a nil reservations slice to [] on the admin passthrough so
an unrestricted session can't emit {"reservations": null} while every
other path on this handler emits [].
Also drops a duplicated log-safety comment (doc comment already states
it) and a stale "AND no accounts registered" conjunction in
buildAzureExchangeClient's doc that the code never implements.
…rvations ownsAzureSource registers ListExchangeableReservations with .Maybe(), so AssertExpectations alone passes whether it was called or not. Add an explicit AssertNotCalled to prove the scope gate refuses the request before the tenant-wide listing is ever fetched.
99efcd0 to
cbd12d9
Compare
|
Rebased onto current
Verification after rebase:
|
1 participant
Summary
listExchangeableAzureRIs(GET /api/ri-exchange/azure-instances) enumerated every reservation the deployment's Azure credential could read across the whole tenant, regardless of the caller'sallowed_accountsscope. Any session holdingview:purchasescould see every subscription's exchangeable reservations, including subscriptions outside their own scope.This narrows the GET listing to match the scoping the two POST siblings on the same resource (
getAzureCompatibleOfferings,executeAzureExchange) already enforce:?subscription_id=supplied:requireAzureSubscriptionScopegates whether the session may use that subscription at all, returning the same genericerrNotFound(404, not 403) the POST siblings use, so a scoped caller cannot probe which subscriptions exist. The listing is then narrowed to that subscription's own rows viaBillingScopeID(filterAzureReservationsBySubscription) -- the same discriminatorrequireAzureSourceOwnershipalready keys on for issue Azure RI exchange: source reservations are never scoped to the authorized subscription #1527.subscription_id: the listing is narrowed to whatever the session'sallowed_accountsscope covers (filterAzureReservationsByScope), mirroringfilterDashboardRecommendationsinhandler_dashboard.go. Unrestricted/admin sessions pass through unchanged.Rows that can't be attributed to a registered
CloudAccount(noBillingScopeID, or one that matches no registered account) are dropped rather than kept, so an unresolvable scope never degrades to "show everything" -- and the two possible empty-result cases (a scoped caller with zero reservations of their own vs. one whose rows are unattributable) stay indistinguishable.filterAzureReservationsByScopewas split intoazureScopeIndex+filterReservationsByScopeIndexto stay under the project's gocyclo threshold (-over 10); the combined function measured 11.Sibling convention matched
Follows the exact pattern already established for the POST siblings on this resource (
requireAzureSubscriptionScope,requireAzureSourceOwnership,filterDashboardRecommendations): fail-closed scope resolution, indistinguishable 404 denials, and counts-only logging (never reservation or billing-scope identifiers).Test plan
Added to
internal/api/handler_ri_exchange_test.go:TestListExchangeableAzureRIs_ScopedFiltersToOwnSubscription-- a session scoped to subscription A, nosubscription_idfilter, against a fixture holding reservations in A and B: only A's reservation comes back, B's identifiers never appear in the response.TestListExchangeableAzureRIs_ScopedEmptyCasesIndistinguishable-- compares a scoped caller whose own subscription has zero reservations against one whose only visible row is unattributable; both render the identical empty list.TestListExchangeableAzureRIs_SubscriptionIDOutOfScope--?subscription_id=naming a subscription the session isn't scoped to returns the sameerrNotFoundshape as the POST siblings.Confirmed the three new tests fail against the pre-fix handler (stashed just the handler change, kept the tests) and pass after:
Pre-fix (handler reverted, tests unchanged):
Post-fix:
Gates run locally:
go build ./...go vet ./...go test ./...(full suite, all packages pass)gocyclo -over 10 -ignore "_test\.go" .-- clean (0 issues after the split)golangci-lint runat the CI-pinnedv2.10.1(not the locally-installed 2.11.4) --0 issues.Closes #1656
Summary by CodeRabbit
Bug Fixes
Tests