diff --git a/frontend/src/__tests__/api.test.ts b/frontend/src/__tests__/api.test.ts index 521c81b01..458435380 100644 --- a/frontend/src/__tests__/api.test.ts +++ b/frontend/src/__tests__/api.test.ts @@ -767,6 +767,7 @@ describe('Plans API', () => { notification_days_before: 3, services: { 'aws:ec2': { provider: 'aws', service: 'ec2', enabled: true, term: 3, payment: 'all-upfront', coverage: 80 } }, ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0, current_step: 0, total_steps: 1 }, + target_accounts: ['11111111-1111-1111-1111-111111111111'], }; await createPlan(plan); @@ -789,6 +790,7 @@ describe('Plans API', () => { notification_days_before: 5, services: { 'aws:rds': { provider: 'aws', service: 'rds', enabled: true, term: 1, payment: 'no-upfront', coverage: 70 } }, ramp_schedule: { type: 'weekly', percent_per_step: 25, step_interval_days: 7, current_step: 0, total_steps: 4 }, + target_accounts: ['22222222-2222-2222-2222-222222222222'], }; await updatePlan('plan-123', plan); diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index 5c728a240..333805fbe 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -148,6 +148,13 @@ describe('Plans Module', () => { + +
+ + @@ -885,6 +892,11 @@ describe('Plans Module', () => { (document.getElementById('plan-auto-purchase') as HTMLInputElement).checked = true; (document.getElementById('plan-notify-days') as HTMLInputElement).value = '3'; (document.getElementById('plan-enabled') as HTMLInputElement).checked = true; + // Universal-plans fix: savePlan rejects an empty Target Accounts list, + // so default the hidden field to a single account UUID for every test + // in this block. Tests that exercise the empty-accounts rejection set + // it back to '' explicitly inside the test. + (document.getElementById('plan-account-ids') as HTMLInputElement).value = '11111111-1111-1111-1111-111111111111'; }); test('prevents default form submission', async () => { @@ -943,6 +955,15 @@ describe('Plans Module', () => { })); }); + // Helper: openCreatePlanModal/openNewPlanModal call form.reset(), which + // clears the hidden plan-account-ids field stamped by the beforeEach. + // Universal-plans fix requires that field to be non-empty at savePlan + // time, so any test that opens the modal must re-stamp it before submit. + const stampAccountIds = () => { + (document.getElementById('plan-account-ids') as HTMLInputElement).value + = '11111111-1111-1111-1111-111111111111'; + }; + test('includes the snapshot stamped by openCreatePlanModal (#273 CR)', async () => { // #273 CR follow-up: savePlan now reads the snapshot stamped at // Plan-button click time via openCreatePlanModal(snapshot), instead @@ -956,6 +977,7 @@ describe('Plans Module', () => { { id: 'rec-2', service: 'rds' }, ] as unknown as readonly api.Recommendation[]; openCreatePlanModal(snapshot); + stampAccountIds(); (api.createPlan as jest.Mock).mockResolvedValue({}); (api.getPlans as jest.Mock).mockResolvedValue({ plans: [] }); @@ -980,6 +1002,7 @@ describe('Plans Module', () => { { id: 'rec-2', service: 'rds' }, ] as unknown as readonly api.Recommendation[]; openCreatePlanModal(snapshotAtClickTime); + stampAccountIds(); // Now simulate post-modal-open state mutations: deselection, // refresh-replaced visible set, etc. None of these should affect @@ -1015,6 +1038,7 @@ describe('Plans Module', () => { // (the New-Plan-from-scratch path explicitly clears the cache so a // subsequent New-Plan submit doesn't inherit a previous flow's recs). openNewPlanModal(); + stampAccountIds(); (api.createPlan as jest.Mock).mockResolvedValue({}); (api.getPlans as jest.Mock).mockResolvedValue({ plans: [] }); (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); @@ -1032,6 +1056,7 @@ describe('Plans Module', () => { // time was empty for some reason), savePlan must still submit without // a recommendations field, not blow up. openCreatePlanModal([] as unknown as readonly api.Recommendation[]); + stampAccountIds(); (api.createPlan as jest.Mock).mockResolvedValue({}); (api.getPlans as jest.Mock).mockResolvedValue({ plans: [] }); (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); @@ -1130,6 +1155,49 @@ describe('Plans Module', () => { expect(api.updatePlan).toHaveBeenCalledWith('plan-123', expect.any(Object)); expect(mockOpenArcheraOfferModal).not.toHaveBeenCalled(); }); + + test('rejects submit and never calls createPlan when Target Accounts is empty (universal-plans fix)', async () => { + // Universal plans (purchase_plans rows with no plan_accounts row) are + // no longer allowed. The Save Plan button is also disabled in this + // state via refreshPlanSaveButtonState; this assertion is the defence- + // in-depth at the savePlan layer for scripted submissions or any + // future regression that bypasses the disabled UI. + (document.getElementById('plan-account-ids') as HTMLInputElement).value = ''; + (api.createPlan as jest.Mock).mockResolvedValue({ id: 'p1' }); + + const event = { preventDefault: jest.fn() } as unknown as Event; + await savePlan(event); + + expect(api.createPlan).not.toHaveBeenCalled(); + expect(api.setPlanAccounts).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ + kind: 'error', + message: expect.stringContaining('Target Accounts'), + })); + }); + + test('forwards selected target_accounts on createPlan (universal-plans fix)', async () => { + // Verifies the new wire contract: savePlan stamps the selected account + // chip IDs onto the request body so the backend can validate and + // persist plan_accounts in the same call. The 2-step PUT remains a + // belt-and-suspenders write for update flows, but the create path + // must include target_accounts inline. + (document.getElementById('plan-account-ids') as HTMLInputElement).value + = '11111111-1111-1111-1111-111111111111,22222222-2222-2222-2222-222222222222'; + (api.createPlan as jest.Mock).mockResolvedValue({ id: 'p1' }); + (api.getPlans as jest.Mock).mockResolvedValue({ plans: [] }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + const event = { preventDefault: jest.fn() } as unknown as Event; + await savePlan(event); + + expect(api.createPlan).toHaveBeenCalledWith(expect.objectContaining({ + target_accounts: [ + '11111111-1111-1111-1111-111111111111', + '22222222-2222-2222-2222-222222222222', + ], + })); + }); }); describe('closePlanModal', () => { diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index f9194ab0d..39a694d17 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -168,6 +168,11 @@ export interface CreatePlanRequest { notification_days_before: number; services: Record; ramp_schedule: PlanRampSchedule; + // Required server-side (universal-plans fix): a plan must be tied to at + // least one cloud account. The frontend bundles the selected account IDs + // here so the backend receives plan creation + account assignment in a + // single request and can validate atomically. + target_accounts: string[]; } // History types diff --git a/frontend/src/index.html b/frontend/src/index.html index 181d5b036..59813a16c 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -869,8 +869,8 @@

Automation Settings

-

Target Accounts

-

Leave empty to target all enabled accounts for the selected provider.

+

Target Accounts

+

Required. Select at least one account this plan will purchase for. The Save button stays disabled until you add one.

diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 6ce65b95b..39a9cb395 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -625,6 +625,22 @@ export async function savePlan(e: Event): Promise { plan.recommendations = [...pendingPlanRecommendations]; } + // Universal-plans fix: read the selected account chips and reject submit + // when the list is empty. The Save button is also disabled in the same + // condition via refreshPlanSaveButtonState() so this branch is mostly a + // belt-and-suspenders against scripted form submission; the toast keeps + // the failure mode loud either way. + const accountIdsField = document.getElementById('plan-account-ids') as HTMLInputElement | null; + const accountIds = accountIdsField?.value ? accountIdsField.value.split(',').filter(Boolean) : []; + if (accountIds.length === 0) { + showToast({ + message: 'Target Accounts is required: pick at least one account before saving the plan.', + kind: 'error', + }); + return; + } + plan.target_accounts = accountIds; + try { let savedPlanId = planId; if (planId) { @@ -634,8 +650,12 @@ export async function savePlan(e: Event): Promise { savedPlanId = created.id; } - const accountIdsField = document.getElementById('plan-account-ids') as HTMLInputElement | null; - const accountIds = accountIdsField?.value ? accountIdsField.value.split(',').filter(Boolean) : []; + // On update, the create path's atomic plan_accounts insert doesn't fire + // (we only POST /plans on create). For updates we still need to push the + // selected account list via the dedicated endpoint. On create, we also + // re-push here so that subsequent reselection is reflected even if the + // backend later opens an "atomic-create-only" path that diverges from + // PUT semantics — same call already handles dedupe via DELETE+INSERT. if (savedPlanId) { await api.setPlanAccounts(savedPlanId, accountIds); } @@ -650,6 +670,21 @@ export async function savePlan(e: Event): Promise { } } +// refreshPlanSaveButtonState toggles the Save button's disabled state based +// on whether at least one Target Account is selected. Universal plans (rows +// in purchase_plans with no plan_accounts row) are no longer allowed by the +// API; surfacing the failure in the disabled state is friendlier than +// letting the user fill in every other field and get rejected at submit. +function refreshPlanSaveButtonState(): void { + const form = document.getElementById('plan-form') as HTMLFormElement | null; + if (!form) return; + const submitBtn = form.querySelector('button[type="submit"]'); + if (!submitBtn) return; + const hasAccounts = planSelectedAccounts.length > 0; + submitBtn.disabled = !hasAccounts; + submitBtn.title = hasAccounts ? '' : 'Select at least one Target Account to save the plan'; +} + /** * Close plan modal */ @@ -697,6 +732,9 @@ function renderPlanAccountChips(): void { function updatePlanAccountIdsField(): void { const field = document.getElementById('plan-account-ids') as HTMLInputElement | null; if (field) field.value = planSelectedAccounts.map(a => a.id).join(','); + // Recalc Save-button disabled state every time the account list changes + // so the user gets immediate feedback when they remove the last chip. + refreshPlanSaveButtonState(); } let planAccountSearchTimer: ReturnType | null = null; diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 0c9c66e61..ca2f495d8 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -152,6 +152,11 @@ export interface SavePlanData { custom_step_percent?: number; custom_interval_days?: number; recommendations?: api.Recommendation[]; + // UUIDs of cloud accounts the plan will purchase for. The server now + // requires at least one (universal-plans fix). The modal renders selected + // accounts as chips; savePlan reads them out of the hidden #plan-account- + // ids field and stamps the array here before POST /plans. + target_accounts?: string[]; } // History types diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 637ec2a2d..780e83814 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -1224,6 +1224,14 @@ func (h *Handler) setPlanAccounts(ctx context.Context, httpReq *events.LambdaFun return nil, NewClientError(400, "invalid request body") } + // Reject empty account_ids: a plan must remain tied to at least one + // cloud_account row. Allowing the PUT to clear all rows would recreate + // the universal-plan bug class (purchase_plans row with no matching + // plan_accounts row) that createPlan now refuses at insert time. + if len(body.AccountIDs) == 0 { + return nil, NewClientError(400, "account_ids is required: a plan must be tied to at least one account") + } + for _, aid := range body.AccountIDs { if err := validateUUID(aid); err != nil { return nil, NewClientError(400, fmt.Sprintf("invalid account_id %q: must be a valid UUID", aid)) diff --git a/internal/api/handler_accounts_router_test.go b/internal/api/handler_accounts_router_test.go index 880322980..7dcf66d94 100644 --- a/internal/api/handler_accounts_router_test.go +++ b/internal/api/handler_accounts_router_test.go @@ -112,7 +112,12 @@ func TestRouterDispatch_PlanAccountsPUT_CorrectDispatch(t *testing.T) { r := setupRouterForDispatch(ctx) planID := "22222222-2222-2222-2222-222222222222" - req, method, path := routerReq("PUT", "/api/plans/"+planID+"/accounts", `{"account_ids":[]}`) + // Body uses a single valid UUID — universal-plans fix rejects empty + // account_ids as a 400, which would mask a routing-misdispatch bug we + // actually want this test to catch. Dispatch verification still works + // with any well-formed body. + body := `{"account_ids":["11111111-1111-1111-1111-111111111111"]}` + req, method, path := routerReq("PUT", "/api/plans/"+planID+"/accounts", body) _, err := r.Route(ctx, method, path, req) require.NoError(t, err, "plan/accounts PUT should not error — invalid ID means wrong handler") } diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index 1aebeddf7..ffd94c9d5 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -1028,6 +1028,33 @@ func TestSetPlanAccounts_EmptyServicesSkipsValidation(t *testing.T) { assert.Equal(t, []string{azureAcct1}, capturedIDs, "empty services map → validation skipped → write proceeds") } +// TestSetPlanAccounts_RejectsEmpty verifies the universal-plans fix: +// PUT /api/plans/:id/accounts with an empty account_ids returns HTTP 400 +// and never reaches the store. Eliminates the back-door for re-creating a +// universal plan by updating an existing one to have zero target accounts. +func TestSetPlanAccounts_RejectsEmpty(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + setCalled := false + store := setupAdminMock(ctx) + store.SetPlanAccountsFn = func(_ context.Context, _ string, _ []string) error { + setCalled = true + return nil + } + handler := &Handler{auth: mockAuth, config: store} + + body := `{"account_ids":[]}` + _, err := handler.setPlanAccounts(ctx, adminRequest(body), planID209) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.Error(), "account_ids is required") + assert.False(t, setCalled, "SetPlanAccounts must NOT be called when account_ids is empty") +} + func TestListPlanAccounts_Success(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 6e07ba464..5f6a1b7ef 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -9,6 +9,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-lambda-go/events" "github.com/google/uuid" "github.com/jackc/pgx/v5" @@ -72,6 +73,16 @@ func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunction return nil, NewClientError(400, "invalid request body") } + // target_accounts is required: a plan must be tied to at least one + // cloud_account row. The historical "leave blank to mean all accounts of + // this provider" behaviour created "universal plans" (rows in + // purchase_plans with no matching plan_accounts row) that were hard to + // scope, hard to filter, and hard to govern. Reject early with a clear + // 400 so the frontend can surface the error before any DB write. + if err := validateTargetAccounts(req.TargetAccounts); err != nil { + return nil, err + } + plan := req.toPurchasePlan() // Validate the plan @@ -83,9 +94,53 @@ func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunction return nil, err } + // Provider-match validation + plan_accounts insert. SetPlanAccounts is + // transactional internally, but the plan-row insert above is not part of + // that tx. If either step here fails we roll the plan row back so the + // invariant "every purchase_plans row has at least one plan_accounts + // row" holds end-to-end — otherwise a validation failure would leave a + // fresh universal plan behind, which is exactly the bug class we're + // eliminating. + // + // rollbackPlan undoes the partial CreatePurchasePlan insert. If the + // rollback delete itself errors (DB blip, row already gone, etc.), log + // at WARN with the plan ID so an operator can clean up manually — we + // still surface the original cause to the caller so the user-facing + // error is unchanged. + rollbackPlan := func() { + if delErr := h.config.DeletePurchasePlan(ctx, plan.ID); delErr != nil { + logging.Warnf("createPlan rollback: failed to delete partial plan %s: %v (manual cleanup may be required)", plan.ID, delErr) + } + } + if err := h.validatePlanAccountProviders(ctx, plan.ID, req.TargetAccounts); err != nil { + rollbackPlan() + return nil, err + } + if err := h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts); err != nil { + rollbackPlan() + return nil, fmt.Errorf("accounts: %w", err) + } + return plan, nil } +// validateTargetAccounts rejects a missing/empty target_accounts payload and +// rejects entries that are not valid UUIDs. Mirrors the validation the +// dedicated PUT /plans/:id/accounts endpoint already performs (see +// setPlanAccounts in handler_accounts.go) so a request that gets past +// createPlan would also get past that endpoint. +func validateTargetAccounts(ids []string) error { + if len(ids) == 0 { + return NewClientError(400, "target_accounts is required: a plan must be tied to at least one account") + } + for _, aid := range ids { + if err := validateUUID(aid); err != nil { + return NewClientError(400, fmt.Sprintf("invalid target_account %q: must be a valid UUID", aid)) + } + } + return nil +} + func (h *Handler) getPlan(ctx context.Context, req *events.LambdaFunctionURLRequest, planID string) (any, error) { // Validate UUID format to prevent injection attacks if err := validateUUID(planID); err != nil { diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 7cd1f5877..62524b04c 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -93,12 +93,22 @@ func TestHandler_createPlan(t *testing.T) { Role: "admin", } + targetAccountID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockStore.On("CreatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + mockStore.On("SetPlanAccounts", ctx, mock.AnythingOfType("string"), []string{targetAccountID}).Return(nil) + + // validatePlanAccountProviders looks up each target account and checks + // its provider matches the plan's. Stub GetCloudAccount to return an + // aws account so the provider-match passes (plan service is aws:rds). + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, Name: "test-aws", Provider: "aws"}, nil + } handler := &Handler{config: mockStore, auth: mockAuth} - body := `{"name": "New Plan", "enabled": true, "auto_purchase": false, "provider": "aws", "service": "rds"}` + body := `{"name": "New Plan", "enabled": true, "auto_purchase": false, "provider": "aws", "service": "rds", "target_accounts": ["` + targetAccountID + `"]}` req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{ "Authorization": "Bearer admin-token", @@ -111,6 +121,7 @@ func TestHandler_createPlan(t *testing.T) { plan := result.(*config.PurchasePlan) assert.Equal(t, "New Plan", plan.Name) assert.True(t, plan.Enabled) + mockStore.AssertCalled(t, "SetPlanAccounts", ctx, mock.AnythingOfType("string"), []string{targetAccountID}) } func TestHandler_createPlan_InvalidBody(t *testing.T) { @@ -139,6 +150,133 @@ func TestHandler_createPlan_InvalidBody(t *testing.T) { assert.Nil(t, result) } +// TestHandler_createPlan_RejectsEmptyTargetAccounts verifies the universal-plan +// fix: a POST /plans without target_accounts (or with an empty list) returns +// HTTP 400 and never reaches CreatePurchasePlan. This is the design invariant +// every purchase_plans row must have at least one matching plan_accounts row. +func TestHandler_createPlan_RejectsEmptyTargetAccounts(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + Role: "admin", + } + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + + cases := []struct { + name string + body string + }{ + {"missing field", `{"name": "P", "provider": "aws", "service": "rds"}`}, + {"empty array", `{"name": "P", "provider": "aws", "service": "rds", "target_accounts": []}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Fresh store per case: AssertNotCalled below would otherwise + // see the previous case's setup; tests share nothing. + mockStore := new(MockConfigStore) + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: tc.body, + } + result, err := handler.createPlan(ctx, req) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.Error(), "target_accounts") + mockStore.AssertNotCalled(t, "CreatePurchasePlan", mock.Anything, mock.Anything) + }) + } +} + +// TestHandler_createPlan_RollbackDeleteFailureSurfacesOriginalError verifies +// that when SetPlanAccounts fails after CreatePurchasePlan succeeds, AND the +// rollback DeletePurchasePlan also fails, the caller still receives the +// original SetPlanAccounts error (wrapped as "accounts: …") rather than the +// rollback error. The rollback failure is logged at WARN so an operator can +// clean up manually — see handler_plans.go createPlan rollbackPlan closure. +// Regression guard for CR #743 finding F1: rollback errors must not be +// silently discarded. +func TestHandler_createPlan_RollbackDeleteFailureSurfacesOriginalError(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + Role: "admin", + } + targetAccountID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockStore.On("CreatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + setAccountsErr := errors.New("setplanaccounts boom") + mockStore.On("SetPlanAccounts", ctx, mock.AnythingOfType("string"), []string{targetAccountID}).Return(setAccountsErr) + // Rollback DeletePurchasePlan also fails — caller must still get the + // original SetPlanAccounts error, not the rollback error. + deleteErr := errors.New("rollback delete boom") + mockStore.On("DeletePurchasePlan", ctx, mock.AnythingOfType("string")).Return(deleteErr) + + // Stub GetCloudAccount so provider-match validation passes — we want to + // reach the SetPlanAccounts step. + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, Name: "test-aws", Provider: "aws"}, nil + } + + handler := &Handler{config: mockStore, auth: mockAuth} + body := `{"name": "New Plan", "enabled": true, "provider": "aws", "service": "rds", "target_accounts": ["` + targetAccountID + `"]}` + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } + result, err := handler.createPlan(ctx, req) + assert.Nil(t, result) + require.Error(t, err) + // Original error from SetPlanAccounts is what reaches the caller — + // wrapped as "accounts: …" per the handler. The rollback error is logged + // (not returned), so it must NOT appear in the user-facing error chain. + assert.Contains(t, err.Error(), "accounts:") + assert.True(t, errors.Is(err, setAccountsErr), "expected wrapped SetPlanAccounts error, got %v", err) + assert.NotContains(t, err.Error(), "rollback delete boom", "rollback error must not leak into user-facing error") + mockStore.AssertCalled(t, "DeletePurchasePlan", ctx, mock.AnythingOfType("string")) +} + +// TestHandler_createPlan_RejectsInvalidTargetAccountUUID verifies that a +// malformed UUID in target_accounts is rejected before any DB write — same +// validation contract as PUT /plans/:id/accounts (handler_accounts.go). +func TestHandler_createPlan_RejectsInvalidTargetAccountUUID(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + Role: "admin", + } + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + body := `{"name": "P", "provider": "aws", "service": "rds", "target_accounts": ["not-a-uuid"]}` + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } + result, err := handler.createPlan(ctx, req) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockStore.AssertNotCalled(t, "CreatePurchasePlan", mock.Anything, mock.Anything) +} + func TestHandler_getPlan(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 5f8e3b3f0..3eedae9ba 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -576,6 +576,13 @@ func TestHandler_HandleRequest_CreatePlan(t *testing.T) { mockAuth.On("ValidateCSRFToken", ctx, mock.Anything, mock.Anything).Return(nil) mockStore.On("CreatePurchasePlan", mock.Anything, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + mockStore.On("SetPlanAccounts", mock.Anything, mock.AnythingOfType("string"), mock.AnythingOfType("[]string")).Return(nil) + // Universal-plans fix: createPlan now validates target_accounts' + // providers match the plan's. Stub the cloud-account lookup so the + // provider-match passes (plan has aws:rds, account returns aws). + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, Name: "test-aws", Provider: "aws"}, nil + } handler := &Handler{config: mockStore, auth: mockAuth, apiKey: "test-key"} @@ -586,7 +593,10 @@ func TestHandler_HandleRequest_CreatePlan(t *testing.T) { "X-CSRF-Token": "test-csrf", "Content-Type": "application/json", }, - Body: `{"name": "New Plan"}`, + // target_accounts is required (universal-plans fix). Provider must + // also be set so DerivePlanProviders returns non-empty; otherwise + // the validation skip-branch would mask the contract. + Body: `{"name": "New Plan", "provider": "aws", "service": "rds", "target_accounts": ["11111111-1111-1111-1111-111111111111"]}`, RequestContext: events.LambdaFunctionURLRequestContext{ HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ Method: "POST", diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 01e8fdc33..ec83a4488 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -271,7 +271,17 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/PlanRequest' + # target_accounts is required ONLY on plan creation. The shared + # PlanRequest schema is reused by PUT/PATCH where omitting the + # field means "leave the existing account list unchanged"; only + # POST enforces non-empty target_accounts to prevent universal + # plans. Scoping the requirement here (instead of in the shared + # schema) keeps the update payload contract loose while still + # advertising the create contract correctly to client codegen. + allOf: + - $ref: '#/components/schemas/PlanRequest' + - type: object + required: [target_accounts] responses: '200': description: Created plan @@ -2006,6 +2016,16 @@ components: type: integer custom_interval_days: type: integer + target_accounts: + type: array + minItems: 1 + description: > + UUIDs of cloud accounts the plan will purchase for. Required on + POST /plans — a plan must be tied to at least one account. Sending + an empty array returns HTTP 400. + items: + type: string + format: uuid PurchasePlan: type: object diff --git a/internal/api/router_handlers_test.go b/internal/api/router_handlers_test.go index 9698c72ca..0383e30ab 100644 --- a/internal/api/router_handlers_test.go +++ b/internal/api/router_handlers_test.go @@ -364,9 +364,12 @@ func TestRouter_setPlanAccountsHandler(t *testing.T) { h := &Handler{auth: mockAuth, config: store} r := newTestRouter(h) + // Universal-plans fix: setPlanAccounts now rejects empty account_ids. + // This router-dispatch test uses a single valid UUID to exercise the + // success path — empty body is covered by handler-level rejection tests. req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{"Authorization": "Bearer admin-token"}, - Body: `{"account_ids": []}`, + Body: `{"account_ids": ["11111111-1111-1111-1111-111111111111"]}`, } _, err := r.setPlanAccountsHandler(ctx, req, map[string]string{"id": "22222222-2222-2222-2222-222222222222"}) require.NoError(t, err) diff --git a/internal/api/types.go b/internal/api/types.go index 6b4ec4f64..067a4e2d2 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -596,6 +596,14 @@ type PlanRequest struct { RampSchedule string `json:"ramp_schedule,omitempty"` CustomStepPercent int `json:"custom_step_percent,omitempty"` CustomIntervalDays int `json:"custom_interval_days,omitempty"` + + // TargetAccounts is the list of cloud_account UUIDs the plan will purchase + // for. Required (non-empty) on POST /plans — a plan with no rows in + // plan_accounts is a "universal plan", which the design no longer allows: + // every plan must be tied to at least one explicit account. The handler + // inserts the plan_accounts rows immediately after CreatePurchasePlan so + // the two writes are observed together by downstream consumers. + TargetAccounts []string `json:"target_accounts,omitempty"` } // toPurchasePlan converts a PlanRequest to a config.PurchasePlan diff --git a/scripts/list_universal_plans.sql b/scripts/list_universal_plans.sql new file mode 100644 index 000000000..56fd47b4f --- /dev/null +++ b/scripts/list_universal_plans.sql @@ -0,0 +1,52 @@ +-- list_universal_plans.sql +-- +-- Read-only diagnostic for the "universal plans" cleanup (issue #742): +-- enumerates every row in purchase_plans that has no matching row in +-- plan_accounts. After PR "fix(plans): eliminate universal plans" lands the +-- API can no longer create such rows, but pre-existing rows from the legacy +-- "leave Target Account blank" behaviour remain until an operator decides +-- what to do with each one. +-- +-- Run this against the live DB (read-only — no DELETE/UPDATE) to see the +-- scope: +-- +-- psql "$DB_URL" -f scripts/list_universal_plans.sql +-- +-- Three operator-driven cleanup options for each row returned, decided per +-- plan (see the follow-up ops issue linked from the PR body): +-- +-- (a) Delete the plan if it is genuinely orphaned (no consumer, no +-- executions, never used). Safest when the row predates any +-- intentional use: +-- +-- DELETE FROM purchase_plans WHERE id = ''; +-- +-- (b) Fan-out: attach every enabled cloud_account whose provider matches +-- the plan's services map (mimics the historical "blank == all +-- provider accounts" intent). Concrete; explicit; recoverable: +-- +-- INSERT INTO plan_accounts (plan_id, account_id) +-- SELECT pp.id, ca.id +-- FROM purchase_plans pp +-- CROSS JOIN LATERAL jsonb_each(pp.services) AS svc(k, v) +-- JOIN cloud_accounts ca +-- ON ca.provider = v->>'provider' +-- AND ca.enabled = true +-- WHERE pp.id = '' +-- ON CONFLICT DO NOTHING; +-- +-- (c) Manual review: pull the plan up in the UI, talk to the owner if +-- known, and assign the right account(s) via the New/Edit Plan modal +-- (which now requires target_accounts). Recommended for plans that +-- had clear intent the SQL can't infer. + +SELECT pp.id, + pp.name, + pp.created_at, + pp.updated_at, + pp.enabled, + pp.services +FROM purchase_plans pp +LEFT JOIN plan_accounts pa ON pa.plan_id = pp.id +WHERE pa.plan_id IS NULL +ORDER BY pp.created_at ASC;