test(api): pin per-account permission scoping across recs/purchases/history/dashboard (closes #307)#309
Conversation
…istory/dashboard (closes #307) Add TestPerAccountPerms regression suite covering 8 endpoints that return or accept cloud_account_id-tagged data. Each test pins the negative case — a scoped user (allowed_accounts: ["a"]) is rejected or filtered when accessing account-b data — so removing any enforcement function causes the suite to fail. Endpoints covered: - GET /recommendations (filterRecommendationsByAllowedAccounts) - GET /recommendations/:id (cross-account 404 via detail filter) - GET /history (filterPurchaseHistoryByAllowedAccounts) - GET /history/analytics (validateAnalyticsAccountScope — cross-account 404) - GET /history/breakdown (validateAnalyticsAccountScope — cross-account 404) - GET /dashboard/summary (filterDashboardRecommendations — subset aggregate) - POST /purchases/execute (validatePurchaseRecommendationScope — 403 + 400) - POST /purchases/planned/:id/pause (requireExecutionAccess — cross-account 404)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new Go regression test suite that verifies per-account authorization scoping across recommendations, history/analytics, dashboard summary, execute purchase, and planned-purchase endpoints, asserting cross-account rejections and allowed-account successes with fixtures and mocks. Changes
Sequence Diagram(s)(Skipped — changes add tests and mocks but do not introduce a new multi-component control flow requiring sequence visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 docstrings
🧪 Generate unit tests (beta)
Comment |
cristim
added
priority/p1
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/api/handler_per_account_perms_test.go (2)
476-577: ⚡ Quick winAdd one scoped allowed-account execute-purchase success case.
Lines 483-577 assert only deny paths (403/400). A regression that rejects every scoped execute request would still satisfy this section. Add one account-A positive test to verify scoped users can execute in-scope recommendations.
🤖 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_per_account_perms_test.go` around lines 476 - 577, Add a positive test case that verifies a scoped user can successfully execute a recommendation for an allowed account: copy the pattern from TestPerAccountPerms_ExecutePurchase_CrossAccountRejected403 but set "cloud_account_id" to permsAccA, call handler.executePurchase(ctx, req) and assert no error, assert that mockStore.SavePurchaseExecution was called with the expected purchase execution payload, and verify returned result status (or lack of ClientError). Use the same setup helpers (scopedAuthMock, scopedReq, permsAccountList) and ensure the test function name indicates success for allowed account (e.g., TestPerAccountPerms_ExecutePurchase_AllowedAccountAccepted200).
386-412: ⚡ Quick winAdd a scoped allowed-account success test for
/history/breakdown.Line 389 currently validates only the cross-account rejection path. If the handler regresses to returning 404 for all scoped users, this test still passes. Add a paired account-A success case to pin non-overblocking behavior.
Suggested test shape
+func TestPerAccountPerms_HistoryBreakdown_AllowedAccountSucceeds(t *testing.T) { + ctx := context.Background() + + mockClient := new(MockAnalyticsClient) + mockClient.On("QueryBreakdown", ctx, permsAccA, mock.Anything, mock.Anything). + Return([]HistoryBreakdownPoint{}, nil) + + mockStore := new(MockConfigStore) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return permsAccountList(), nil + } + + handler := &Handler{ + auth: scopedAuthMock(ctx), + analyticsClient: mockClient, + config: mockStore, + } + + _, err := handler.getHistoryBreakdown(ctx, scopedReq(), map[string]string{"account_id": permsAccA}) + require.NoError(t, err) +}🤖 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_per_account_perms_test.go` around lines 386 - 412, The test TestPerAccountPerms_HistoryBreakdown_CrossAccountRejected only asserts the cross-account rejection path and can miss a regression that blocks all scoped users; add a new paired success case that calls handler.getHistoryBreakdown with the scoped request but "account_id" set to the allowed account (e.g. permsAccA), mock the analytics client (mockClient) to expect and return a valid QueryBreakdown response, assert no error and that the response equals the mocked result, and finally assert mockClient.AssertCalled for "QueryBreakdown" to ensure the handler permits scoped users for the correct account; mirror setup used in the existing test (scopedAuthMock, scopedReq, MockAnalyticsClient, MockConfigStore).
🤖 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_per_account_perms_test.go`:
- Around line 476-577: Add a positive test case that verifies a scoped user can
successfully execute a recommendation for an allowed account: copy the pattern
from TestPerAccountPerms_ExecutePurchase_CrossAccountRejected403 but set
"cloud_account_id" to permsAccA, call handler.executePurchase(ctx, req) and
assert no error, assert that mockStore.SavePurchaseExecution was called with the
expected purchase execution payload, and verify returned result status (or lack
of ClientError). Use the same setup helpers (scopedAuthMock, scopedReq,
permsAccountList) and ensure the test function name indicates success for
allowed account (e.g.,
TestPerAccountPerms_ExecutePurchase_AllowedAccountAccepted200).
- Around line 386-412: The test
TestPerAccountPerms_HistoryBreakdown_CrossAccountRejected only asserts the
cross-account rejection path and can miss a regression that blocks all scoped
users; add a new paired success case that calls handler.getHistoryBreakdown with
the scoped request but "account_id" set to the allowed account (e.g. permsAccA),
mock the analytics client (mockClient) to expect and return a valid
QueryBreakdown response, assert no error and that the response equals the mocked
result, and finally assert mockClient.AssertCalled for "QueryBreakdown" to
ensure the handler permits scoped users for the correct account; mirror setup
used in the existing test (scopedAuthMock, scopedReq, MockAnalyticsClient,
MockConfigStore).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4914776d-2866-4081-97ec-82bdefcd04ae
📒 Files selected for processing (1)
internal/api/handler_per_account_perms_test.go
… pass 1) Add TestPerAccountPerms_HistoryBreakdown_AllowedAccountSucceeds and TestPerAccountPerms_ExecutePurchase_AllowedAccountAccepted to complement the existing rejection tests and prevent false-positive passes when a regression blocks all scoped users (CodeRabbit nitpicks on PR #309).
|
CR pass 1 — addressed both nitpicks: Nitpick 1 ( Nitpick 2 ( 15 tests now pass (was 13). @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review of the updated branch now. [review] |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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_per_account_perms_test.go`:
- Around line 361-382: The test sets up MockAnalyticsClient.On("QueryHistory",
ctx, permsAccA, mock.Anything, mock.Anything, mock.Anything) but never asserts
it was invoked; update the test (around the handler.getHistoryAnalytics call) to
explicitly verify the mock was called by adding a call to
mockClient.AssertCalled(t, "QueryHistory", ctx, permsAccA, mock.Anything,
mock.Anything, mock.Anything) or mockClient.AssertExpectations(t) after
require.NotNil(t, result) so the test fails if getHistoryAnalytics
short-circuits and doesn't invoke QueryHistory.
- Around line 141-143: The assertion compares permsAccB (string) to
r.CloudAccountID (*string), so it always passes; change the check in the loop
over result.Recommendations to compare the actual string value safely by
dereferencing the pointer (handle nil): e.g., inside the loop, if
r.CloudAccountID != nil then assert.NotEqual(t, permsAccB, *r.CloudAccountID,
"account-B recommendation must not appear in scoped user's list") so the test
fails if a leaked account-B value appears.
🪄 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: cd90e411-6839-467f-9711-4cc427331da5
📒 Files selected for processing (1)
internal/api/handler_per_account_perms_test.go
…st (CR pass 2) - Fix loop in Recommendations_ListFilter: compare *r.CloudAccountID (string) not r.CloudAccountID (*string) so the regression anchor actually fails when account-B leaks through (CR inline comment, line 141-143). - Add mockClient.AssertCalled after HistoryAnalytics_AllowedAccountSucceeds so the test fails if the handler short-circuits without reaching QueryHistory (CR inline comment, line 361-382).
|
CR pass 2 — addressed both actionable items: Item 1 (line 141-143, pointer dereference): Fixed the loop in Item 2 (line 361-382, missing AssertCalled): Added 15 tests, 4286 total — all pass. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review now. [review] |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
CR loop complete — all passes resolvedPass 1 (2026-05-05T20:10:49Z)
Pass 2 (2026-05-05T20:20:04Z) — both items fixed and pushed
Pass 3CR's incremental review found no new delta after the pass 2 fixes were pushed. CR acknowledged both fixes as correct and confirmed no further actionable items. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/api/handler_per_account_perms_test.go (1)
483-490: ⚡ Quick winMock the account-B service config too so this stays an auth regression test.
Right now this test also relies on
GetServiceConfig("aws", "rds")never being reached. If the implementation ever resolves service configs before filtering, the test will fail on an unexpected mock call even if per-account scoping is still correct. Stubbing the account-B pair keeps failures tied to the aggregation assertions instead of call order.Proposed change
mockStore.On("GetServiceConfig", ctx, "aws", "ec2").Return((*config.ServiceConfig)(nil), nil) + mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return((*config.ServiceConfig)(nil), nil) // calculateCommitmentMetrics calls GetPurchaseHistory for YTD/committed totals. mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)🤖 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_per_account_perms_test.go` around lines 483 - 490, The test currently only stubs mockStore.GetServiceConfig for ("aws","ec2") so an unexpected call to GetServiceConfig(ctx,"aws","rds") (account-B service pair) will fail if service configs are resolved before filtering; update the test to also stub mockStore.On("GetServiceConfig", ctx, "aws", "rds") to return a nil *config.ServiceConfig and nil error so the test remains focused on aggregation assertions (refer to mockStore, GetServiceConfig and resolveCoverageByAccountKey in handler_per_account_perms_test.go).
🤖 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_per_account_perms_test.go`:
- Around line 738-757: Add explicit assertions that the
TransitionExecutionStatus mock was invoked with the "paused" target and that the
handler returned an execution whose Status is "paused": after calling
handler.pausePlannedPurchase(ctx, scopedReq(), executionID) assert the mockStore
received the call (e.g. mockStore.AssertCalled/AssertExpectations for
"TransitionExecutionStatus" with ctx, executionID, mock.Anything, "paused") and
assert result.Status == "paused" (or equivalent field on the returned execution)
so the test fails if the transition was not actually performed.
---
Nitpick comments:
In `@internal/api/handler_per_account_perms_test.go`:
- Around line 483-490: The test currently only stubs mockStore.GetServiceConfig
for ("aws","ec2") so an unexpected call to GetServiceConfig(ctx,"aws","rds")
(account-B service pair) will fail if service configs are resolved before
filtering; update the test to also stub mockStore.On("GetServiceConfig", ctx,
"aws", "rds") to return a nil *config.ServiceConfig and nil error so the test
remains focused on aggregation assertions (refer to mockStore, GetServiceConfig
and resolveCoverageByAccountKey in handler_per_account_perms_test.go).
🪄 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: 746e44aa-caa3-488f-a88a-46c61726c8a1
📒 Files selected for processing (1)
internal/api/handler_per_account_perms_test.go
Two CodeRabbit findings on PR #309: 1. Actionable: TestPerAccountPerms_PlannedPurchase_AllowedAccountPlanSucceeds only asserted NoError + NotNil, so a handler that skipped the TransitionExecutionStatus call would still pass. Added: - mockStore.AssertCalled(t, "TransitionExecutionStatus", ..., "paused") to verify the transition actually happened - assert.Equal for result.Status == "paused" to confirm the response 2. Nitpick: TestPerAccountPerms_DashboardSummary_AggregatesAllowedSubsetOnly only stubbed GetServiceConfig for (aws, ec2). Added a matching stub for (aws, rds) so the test doesn't break if the implementation ever resolves service configs before filtering.
|
Addressing two CodeRabbit findings from the 2026-05-05T20:55:33Z review (commit 016da84): Actionable fixed (
Nitpick applied (
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review now. [review] |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
CR loop complete — both findings from the 20:55 review addressed in 016da84
CI: all 3 checks green (sanity AWS, sanity Azure, pre-commit). 3-pass CR loop closed: pass-1 dismissed/refactored, pass-2 fixed (pointer deref + analytics AssertCalled), pass-3 fixed (this one). |
1 participant
Summary
TestPerAccountPermsregression suite (13 test functions, 1 new file) covering 8 endpoints that return or acceptcloud_account_id-tagged data.allowed_accounts: ["a"]) is rejected/filtered when accessing account-b data — removing any enforcement function causes the suite to fail.Endpoints covered
GET /recommendationsfilterRecommendationsByAllowedAccountsGET /recommendations/:idGET /historyfilterPurchaseHistoryByAllowedAccountsGET /history/analyticsvalidateAnalyticsAccountScopeGET /history/breakdownvalidateAnalyticsAccountScopeGET /dashboard/summaryfilterDashboardRecommendationsPOST /purchases/executevalidatePurchaseRecommendationScopePOST /purchases/planned/:id/pauserequireExecutionAccessAuth gaps discovered
None. All 8 endpoints have enforcement in place. The tests confirm it.
Test plan
go test ./internal/api/... -run "TestPerAccountPerms" -v— 13 tests passgo test ./internal/api/...— full 1068 tests pass (no regressions)go test ./...— full 4284 tests passSummary by CodeRabbit