feat(api): validate plan account provider matches plan provider on assignment (closes #209)#228
Conversation
…signment (closes #209) Backend gate for PUT /api/plans/:id/accounts. Every account assigned to a plan must have its provider match one of the providers derived from the plan's services map (key format "provider:service", e.g. "aws:ec2"). Mismatches return a single HTTP 400 listing every offender; the underlying SetPlanAccounts store write is never invoked on failure. Per spec acceptance criterion E-4 in specs/multi-account-execution/acceptance.md, this is the backend hardening of the existing frontend single-provider-per-plan rule. Implementation -------------- - New `derivePlanProviders(plan)` helper extracts the distinct provider set from `plan.Services` keys (sorted slice for stable error messages). - New `Handler.validatePlanAccountProviders(ctx, planID, accountIDs)` helper holds the validation block — extracted from setPlanAccounts to stay under the gocyclo budget (limit 10). - `setPlanAccounts` loads the plan, derives providers, then for each account_id in the request loads the account and checks its provider against the derived set. All offenders are collected and reported in a single error rather than failing fast — clients fix everything in one round-trip. - Plan not found → 404 with the plan ID. - Account not found → 404 with the account ID. - Mismatch(es) → 400 "plan provider mismatch: account "<name>" has provider="<got>", expected one of [<sorted list>]; ..." (single line, parseable). - Empty services map → defensive skip of validation; production plans always have ≥1 service (frontend enforces this), and the test pins the behaviour so a future change is conscious. Mocks ----- - `MockConfigStore.GetPurchasePlan` now resolves to (in order): `GetPurchasePlanFn` override, registered testify expectation, or a default minimal `{ID: planID}` plan with empty Services. The default fallback lets pre-existing tests like `TestSetPlanAccounts_Success` keep working without setting up the new mock call — empty Services trips the defensive skip-validation branch. - `MockConfigStore.SetPlanAccounts` now uses `SetPlanAccountsFn` when set, so tests can capture and assert on the call (mismatch tests verify the underlying store write is NOT invoked on failure). Tests ----- Seven new tests in `handler_accounts_test.go`: - TestSetPlanAccounts_SingleMismatch — one Azure account vs aws plan → 400 - TestSetPlanAccounts_MultipleMismatches — Azure + GCP vs aws plan → 400 listing both - TestSetPlanAccounts_ValidHappyPath — AWS account vs aws plan → 200, store write captured - TestSetPlanAccounts_PlanNotFound — GetPurchasePlan returns (nil, nil) → 404 - TestSetPlanAccounts_AccountNotFound — GetCloudAccount returns (nil, nil) → 404 referencing the account ID - TestSetPlanAccounts_MixedValidAndMismatch — only the offender named in error; store write NOT called - TestSetPlanAccounts_EmptyServicesSkipsValidation — defensive behaviour pinned
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPUT /api/plans/:id/accounts now validates that each referenced cloud account’s provider is among the plan’s derived providers (from plan.Services). Validation runs in the handler and in-store (transactional); mismatches are aggregated into a single 400, missing plan/account map to 404, and no writes occur when validation fails. ChangesPlan Account Provider Validation
Sequence DiagramssequenceDiagram
participant Client as Client
participant Handler as Handler
participant ConfigStore as ConfigStore
participant CloudStore as CloudAccounts
Client->>Handler: PUT /api/plans/:id/accounts [accountIDs]
Handler->>ConfigStore: GetPurchasePlan(planID)
ConfigStore-->>Handler: PurchasePlan {services: {...}}
Handler->>Handler: DerivePlanProviders(services) -> [allowedProviders]
loop for each accountID
Handler->>CloudStore: GetCloudAccount(accountID)
CloudStore-->>Handler: CloudAccount {id,name,provider}
Handler->>Handler: check provider ∈ allowedProviders
alt mismatch
Handler-->>Handler: collect offender(info)
end
end
alt any offenders
Handler-->>Client: 400 ClientError (aggregated offenders)
else all valid
Handler->>ConfigStore: SetPlanAccounts(planID, accountIDs)
ConfigStore-->>Handler: nil
Handler-->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/5 reviews remaining, refill in 27 minutes and 17 seconds. Comment |
|
@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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/api/mocks_test.go (1)
325-330: ⚡ Quick winPreserve testify expectation path in
SetPlanAccountsfor mock consistency.When
SetPlanAccountsFnis nil, this now always returnsniland ignores any registeredm.On("SetPlanAccounts", ...)expectation. Add theisExpected/m.Calledfallback like other mock methods.💡 Proposed fix
func (m *MockConfigStore) SetPlanAccounts(ctx context.Context, planID string, accountIDs []string) error { if m.SetPlanAccountsFn != nil { return m.SetPlanAccountsFn(ctx, planID, accountIDs) } + if m.isExpected("SetPlanAccounts") { + return m.Called(ctx, planID, accountIDs).Error(0) + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/mocks_test.go` around lines 325 - 330, The MockConfigStore.SetPlanAccounts implementation currently returns nil when SetPlanAccountsFn is nil, ignoring any testify expectations; update SetPlanAccounts to call m.Called(ctx, planID, accountIDs) as a fallback and use the returned isExpected/args to produce the return value (e.g., if args.Error(0) is set return that error), matching the pattern used by other mock methods and preserving m.On("SetPlanAccounts", ...) expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/handler_accounts.go`:
- Around line 1043-1057: The derivePlanProviders function is parsing
plan.Services keys with the wrong delimiter (":"), causing provider extraction
and subsequent validation to be skipped; update derivePlanProviders to split
keys using "/" instead of ":" (change strings.SplitN(k, ":", 2) to use "/" and
adjust the empty-part checks accordingly) so provider names are correctly
collected, keep the existing behavior of skipping malformed keys, and ensure the
same delimiter fix is applied where validation uses the parsed providers (the
validation block around expected/mismatch handling referenced near lines
1094-1097) so mismatched providers are rejected as intended; run/update any
tests that assume provider/service keys produced by buildServiceConfig.
---
Nitpick comments:
In `@internal/api/mocks_test.go`:
- Around line 325-330: The MockConfigStore.SetPlanAccounts implementation
currently returns nil when SetPlanAccountsFn is nil, ignoring any testify
expectations; update SetPlanAccounts to call m.Called(ctx, planID, accountIDs)
as a fallback and use the returned isExpected/args to produce the return value
(e.g., if args.Error(0) is set return that error), matching the pattern used by
other mock methods and preserving m.On("SetPlanAccounts", ...) expectations.
🪄 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: c6ce80d2-0c17-44f5-946e-321f0493e7d0
📒 Files selected for processing (3)
internal/api/handler_accounts.gointernal/api/handler_accounts_test.gointernal/api/mocks_test.go
cristim
added
priority/p1
) A fork that pushes a PR, pings CodeRabbit, then exits leaves CR threads unresolved — exactly what happened to PRs #228, #229, #230, etc., where CR posted Actionable findings + Nitpicks that were never triaged. This adds a paragraph to the CR-loop section explicitly addressing the delegation case: subagent prompts MUST include the full loop with the exit criteria spelled out. Stops the failure mode where the rule is correctly stated for humans but doesn't get mirrored into fork prompts.
- derivePlanProviders: fix key delimiter ":" → "/" to match buildServiceConfig
(CR actionable: provider extraction was silently skipping all services,
disabling validation; keys are "provider/service" not "provider:service")
- awsPlan209() test fixture: update key to "aws/ec2" to match the corrected
delimiter; previously the test passed only because both code and fixture used
the wrong format
- MockConfigStore.SetPlanAccounts: add isExpected/m.Called fallback
(CR nitpick: testify .On("SetPlanAccounts",...) expectations were ignored)
CR feedback addressedActionable fix (critical)
Nitpick fix
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review now. [review] |
|
Addressed CR feedback in commit 70b8c54: fixed wrong delimiter to slash in derivePlanProviders, added mock fallback via isExpected/m.Called pattern. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh incremental review now to verify the commit. [review] |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/handler_accounts.go (1)
1086-1167:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake the validation/write path atomic.
validatePlanAccountProvidersreads accounts in a separate step fromSetPlanAccounts, so an account'sProvidercan change after it is checked but before the association write commits. That leaves a TOCTOU window where an invalid plan/account pairing can still be persisted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da4308b9-26e8-4eb8-b492-1362953df007
📒 Files selected for processing (3)
internal/api/handler_accounts.gointernal/api/handler_accounts_test.gointernal/api/mocks_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/mocks_test.go
- internal/api/handler_accounts_test.go
Move plan provider derivation into config so the API and store share the same provider/service parsing rule. Recheck account providers inside the Postgres SetPlanAccounts transaction before deleting or inserting plan_accounts, locking the plan and account rows that determine validity. Add pgxmock coverage that a provider mismatch rolls back before any plan_accounts mutation.
|
@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 the current code and only fix it if needed.
Inline comments:
In `@internal/api/handler_accounts.go`:
- Around line 1058-1064: The store currently returns generic errors for missing
rows which causes handlers to return 500; add a sentinel/typed error (e.g. var
ErrNotFound = errors.New("not found")) in the store package and update
PostgresStore.GetPurchasePlan and the store method behind SetPlanAccounts to
return that sentinel when a row is not present, then in the handler code that
calls GetPurchasePlan and SetPlanAccounts use errors.Is(err, store.ErrNotFound)
to translate those cases to NewClientError(404, fmt.Sprintf("plan not found:
%s", planID)) or an appropriate 404 message; ensure any existing nil checks
remain as defensive code but rely on errors.Is for mapping store-level not-found
to 404.
In `@internal/config/store_postgres.go`:
- Around line 1955-1957: SetPlanAccounts currently returns early when accountIDs
is empty which skips the plan existence check and allows a nonexistent planID to
silently succeed; ensure the plan is validated before returning on empty
assignments by invoking the existing getPlanServicesForShareTx (or an equivalent
plan-existence check) for the supplied planID and tx context, and only then
proceed to return if there are no accounts to assign. Update SetPlanAccounts to
call getPlanServicesForShareTx (or perform the same validation logic) prior to
the len(accountIDs) == 0 early return so invalid planIDs are detected even when
clearing assignments.
🪄 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: 7ced29d2-7bb1-44fc-ac4c-3433f06fb790
📒 Files selected for processing (5)
internal/api/handler_accounts.gointernal/config/purchase_plan_providers.gointernal/config/purchase_plan_providers_test.gointernal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.go
Add a config-store ErrNotFound sentinel and use it for Postgres plan and account lookups in the atomic SetPlanAccounts validation path. The accounts handler now maps those store-level not-found races to 404s. Also validate the plan row before accepting empty account assignments so clearing a nonexistent plan cannot silently succeed.
|
Addressed latest CR feedback in c566d95: added typed config.ErrNotFound mapping to 404 for atomic SetPlanAccounts races, and validated plan existence before empty account assignments. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh incremental review now. [review] |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/store_postgres_pgxmock_test.go`:
- Around line 1179-1198: The test TestPGXMock_SetPlanAccounts_Empty is missing a
final assertion that the pgxmock expectations were met; add a call to
mock.ExpectationsWereMet() at the end of the test and assert no error (e.g.,
require.NoError(t, mock.ExpectationsWereMet())) so any unmet SQL expectations in
SetPlanAccounts are surfaced; locate the mock variable created by newMock(t) and
append the expectation check after the existing require.NoError(t, err) return.
🪄 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: 2a463594-f249-4dea-aa5c-7ac89300c068
📒 Files selected for processing (5)
internal/api/handler_accounts.gointernal/api/handler_accounts_test.gointernal/config/errors.gointernal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/config/store_postgres.go
- internal/api/handler_accounts.go
- internal/api/handler_accounts_test.go
Add the missing pgxmock expectation assertion to the empty SetPlanAccounts test so unmet SQL expectations are surfaced.
|
Addressed latest CR test comment in 50f1af9: added the pgxmock expectations assertion for empty SetPlanAccounts. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Closes #209.
Adds a backend gate to
PUT /api/plans/:id/accountsso account assignments must respect the plan's provider. Today the frontend enforces single-provider-per-plan but the backend accepts any account, leaving room for an out-of-band call (curl, API key, badly-behaved client) to attach a cross-cloud account and break downstream purchase execution. This PR closes that gap per acceptance criterion E-4 inspecs/multi-account-execution/acceptance.md.What changed
derivePlanProviders(plan)(private) — extracts the distinct set of providers a plan targets by parsingplan.Serviceskeys ("provider:service"). Returns a sorted slice for stable error messages.Handler.validatePlanAccountProviders(ctx, planID, accountIDs)— the validation block, extracted fromsetPlanAccountsto keep it under the gocyclo budget (10).setPlanAccountsnow invokesvalidatePlanAccountProvidersimmediately after the existing UUID-format check and before the store write.Behaviour matrix
404 plan not found: <plan-id>account_idnot found in store404 account not found: <account-id>400 plan provider mismatch: account "<name>" has provider="<got>", expected one of [<sorted list>]; ...(every offender named in one message)SetPlanAccountsruns — same as beforeServicesmapWhy a single error listing every mismatch
Failing fast on the first bad account would force the client to resubmit N times to discover N mismatches. Collecting them all and naming each one in a single 400 lets the UI render every offender in one toast.
Mock changes (test-only)
MockConfigStore.GetPurchasePlannow resolves in this order:GetPurchasePlanFnoverride.On("GetPurchasePlan", ...)expectation&PurchasePlan{ID: planID}with empty ServicesThe default-fallback path is what lets pre-existing tests like
TestSetPlanAccounts_Successkeep working without registering the new mock — the empty Services map trips the defensive skip-validation branch in the handler.MockConfigStore.SetPlanAccountsnow honours a newSetPlanAccountsFnso the mismatch tests can capture (or assert against) the call. The mismatch tests verify the store write is not invoked when validation fails.Test coverage
Seven new tests in
handler_accounts_test.go:TestSetPlanAccounts_SingleMismatch— one Azure account vs an AWS plan → 400, store write not invokedTestSetPlanAccounts_MultipleMismatches— Azure + GCP vs an AWS plan → 400 listing bothTestSetPlanAccounts_ValidHappyPath— AWS account vs an AWS plan → 200, store write capturedTestSetPlanAccounts_PlanNotFound—GetPurchasePlan → (nil, nil)→ 404TestSetPlanAccounts_AccountNotFound—GetCloudAccount → (nil, nil)for one of the IDs → 404 referencing that IDTestSetPlanAccounts_MixedValidAndMismatch— only the offender appears in the error; store write not invokedTestSetPlanAccounts_EmptyServicesSkipsValidation— pins the defensive skipExisting
TestSetPlanAccounts_SuccessandTestListPlanAccounts_Successcontinue to pass (the mock fallback covers them).Verification
Three consecutive clean local passes before commit:
go test ./internal/api/... -count=1→ 1016 passed (after rebase onto base)go test ./... -count=1→ 4192 passed across 37 packagesgo vet ./...→ cleangofmt -l .→ cleangocyclo -over 10 internal/api/handler_accounts.go→ no findingsTest plan
Summary by CodeRabbit
New Features
Tests