feat(api): validate plan account provider matches plan provider on assignment (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

Author: cristim

internal/api/handler_accounts.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"slices"
+	"sort"
 	"strings"
 	"time"
 
@@ -1037,7 +1039,100 @@ func (h *Handler) deleteAccountServiceOverride(ctx context.Context, req *events.
 	return nil, nil
 }
 
+// derivePlanProviders extracts the distinct set of providers a plan
+// targets by parsing the keys of plan.Services (format "provider:service",
+// e.g. "aws:ec2"). Returns a sorted slice for stable error messages.
+// An empty result means the plan has no parseable services — production
+// plans always carry at least one (frontend enforces this), so an empty
+// return is a defensive case that signals to skip provider validation.
+func derivePlanProviders(plan *config.PurchasePlan) []string {
+	if plan == nil {
+		return nil
+	}
+	seen := make(map[string]struct{}, len(plan.Services))
+	for k := range plan.Services {
+		// Keys are "provider:service"; skip malformed keys rather than guess.
+		parts := strings.SplitN(k, ":", 2)
+		if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
+			continue
+		}
+		seen[parts[0]] = struct{}{}
+	}
+	out := make([]string, 0, len(seen))
+	for p := range seen {
+		out = append(out, p)
+	}
+	sort.Strings(out)
+	return out
+}
+
+// validatePlanAccountProviders enforces the issue-#209 / spec E-4 rule
+// that every account assigned to a plan must have its provider match
+// one of the plan's derived providers. Returns:
+//   - 404 ClientError when the plan does not exist
+//   - 404 ClientError when an account_id does not exist (referencing
+//     the offending ID)
+//   - 400 ClientError listing every provider mismatch in one message
+//     (so clients fix everything in one round-trip rather than
+//     resubmitting to discover the next)
+//   - nil when all accounts match (or when the plan has no parseable
+//     services — defensive skip; production plans always carry at least
+//     one service, frontend enforces this)
+//
+// Pulled out of setPlanAccounts to keep that function under the gocyclo
+// budget (limit 10). No business logic lives here that isn't otherwise
+// described in setPlanAccounts' doc comment.
+func (h *Handler) validatePlanAccountProviders(ctx context.Context, planID string, accountIDs []string) error {
+	plan, err := h.config.GetPurchasePlan(ctx, planID)
+	if err != nil {
+		return fmt.Errorf("accounts: failed to get plan: %w", err)
+	}
+	if plan == nil {
+		return NewClientError(404, fmt.Sprintf("plan not found: %s", planID))
+	}
+
+	expected := derivePlanProviders(plan)
+	if len(expected) == 0 {
+		return nil
+	}
+
+	type mismatch struct {
+		ID       string
+		Name     string
+		Provider string
+	}
+	var mismatches []mismatch
+	for _, aid := range accountIDs {
+		acct, getErr := h.config.GetCloudAccount(ctx, aid)
+		if getErr != nil {
+			return fmt.Errorf("accounts: failed to get account %s: %w", aid, getErr)
+		}
+		if acct == nil {
+			return NewClientError(404, fmt.Sprintf("account not found: %s", aid))
+		}
+		if !slices.Contains(expected, acct.Provider) {
+			mismatches = append(mismatches, mismatch{ID: aid, Name: acct.Name, Provider: acct.Provider})
+		}
+	}
+	if len(mismatches) == 0 {
+		return nil
+	}
+
+	parts := make([]string, len(mismatches))
+	for i, m := range mismatches {
+		parts[i] = fmt.Sprintf("account %q has provider=%q, expected one of %v",
+			m.Name, m.Provider, expected)
+	}
+	return NewClientError(400, "plan provider mismatch: "+strings.Join(parts, "; "))
+}
+
 // setPlanAccounts handles PUT /api/plans/:id/accounts.
+//
+// Per issue #209 / spec acceptance criterion E-4, every account assigned
+// to a plan must have its provider match one of the plan's derived
+// providers (extracted from plan.Services keys). Mismatches return 400
+// listing every offender; the assignment is rejected atomically (no
+// partial writes).
 func (h *Handler) setPlanAccounts(ctx context.Context, httpReq *events.LambdaFunctionURLRequest, id string) (any, error) {
 	if err := validateUUID(id); err != nil {
 		return nil, err
@@ -1060,6 +1155,12 @@ func (h *Handler) setPlanAccounts(ctx context.Context, httpReq *events.LambdaFun
 		}
 	}
 
+	// Provider-match validation (issue #209). Extracted to keep
+	// setPlanAccounts under the gocyclo budget (10).
+	if err := h.validatePlanAccountProviders(ctx, id, body.AccountIDs); err != nil {
+		return nil, err
+	}
+
 	if err := h.config.SetPlanAccounts(ctx, id, body.AccountIDs); err != nil {
 		return nil, fmt.Errorf("accounts: %w", err)
 	}

internal/api/handler_accounts_test.go

@@ -485,6 +485,253 @@ func TestSetPlanAccounts_Success(t *testing.T) {
 	assert.Nil(t, result)
 }
 
+// ── Provider-validation tests for setPlanAccounts (issue #209)
+// 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"). Mismatches return a single 400 listing every
+// offender; the underlying store write is never invoked on failure.
+
+const (
+	planID209   = "22222222-2222-2222-2222-222222222222"
+	awsAcct209  = "11111111-1111-1111-1111-111111111111"
+	azureAcct1  = "33333333-3333-3333-3333-333333333333"
+	azureAcct2  = "44444444-4444-4444-4444-444444444444"
+	missingAcct = "55555555-5555-5555-5555-555555555555"
+)
+
+// awsPlan209 returns a plan whose services map yields a single derived
+// provider ("aws"). Used as the default fixture across the mismatch
+// tests below.
+func awsPlan209() *config.PurchasePlan {
+	return &config.PurchasePlan{
+		ID:       planID209,
+		Name:     "AWS-only plan",
+		Services: map[string]config.ServiceConfig{"aws:ec2": {}},
+	}
+}
+
+func TestSetPlanAccounts_SingleMismatch(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	setCalled := false
+	store := setupAdminMock(ctx)
+	store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) {
+		return awsPlan209(), nil
+	}
+	store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		return &config.CloudAccount{ID: id, Name: "prod-azure", Provider: "azure"}, nil
+	}
+	store.SetPlanAccountsFn = func(_ context.Context, _ string, _ []string) error {
+		setCalled = true
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	body := `{"account_ids":["` + azureAcct1 + `"]}`
+	_, err := handler.setPlanAccounts(ctx, adminRequest(body), planID209)
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a clientError, got %T", err)
+	assert.Equal(t, 400, ce.code)
+	assert.Contains(t, ce.Error(), "prod-azure")
+	assert.Contains(t, ce.Error(), "azure")
+	assert.Contains(t, ce.Error(), "aws")
+	assert.False(t, setCalled, "SetPlanAccounts must NOT be called when validation fails")
+}
+
+func TestSetPlanAccounts_MultipleMismatches(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	setCalled := false
+	store := setupAdminMock(ctx)
+	store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) {
+		return awsPlan209(), nil
+	}
+	store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		switch id {
+		case azureAcct1:
+			return &config.CloudAccount{ID: id, Name: "prod-azure", Provider: "azure"}, nil
+		case azureAcct2:
+			return &config.CloudAccount{ID: id, Name: "stage-gcp", Provider: "gcp"}, nil
+		}
+		return nil, nil
+	}
+	store.SetPlanAccountsFn = func(_ context.Context, _ string, _ []string) error {
+		setCalled = true
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	body := `{"account_ids":["` + azureAcct1 + `","` + azureAcct2 + `"]}`
+	_, 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)
+	// Both offenders named in a single error so the client gets the full
+	// picture in one round-trip.
+	assert.Contains(t, ce.Error(), "prod-azure")
+	assert.Contains(t, ce.Error(), "stage-gcp")
+	assert.Contains(t, ce.Error(), "azure")
+	assert.Contains(t, ce.Error(), "gcp")
+	assert.False(t, setCalled, "SetPlanAccounts must NOT be called when validation fails")
+}
+
+func TestSetPlanAccounts_ValidHappyPath(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	var capturedIDs []string
+	store := setupAdminMock(ctx)
+	store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) {
+		return awsPlan209(), nil
+	}
+	store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		return &config.CloudAccount{ID: id, Name: "prod-aws", Provider: "aws"}, nil
+	}
+	store.SetPlanAccountsFn = func(_ context.Context, _ string, ids []string) error {
+		capturedIDs = ids
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	body := `{"account_ids":["` + awsAcct209 + `"]}`
+	result, err := handler.setPlanAccounts(ctx, adminRequest(body), planID209)
+	require.NoError(t, err)
+	assert.Nil(t, result)
+	assert.Equal(t, []string{awsAcct209}, capturedIDs, "SetPlanAccounts should be called with the validated IDs")
+}
+
+func TestSetPlanAccounts_PlanNotFound(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	setCalled := false
+	store := setupAdminMock(ctx)
+	store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) {
+		return nil, nil // store-style "not found": (nil, nil)
+	}
+	store.SetPlanAccountsFn = func(_ context.Context, _ string, _ []string) error {
+		setCalled = true
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	body := `{"account_ids":["` + awsAcct209 + `"]}`
+	_, err := handler.setPlanAccounts(ctx, adminRequest(body), planID209)
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok)
+	assert.Equal(t, 404, ce.code)
+	assert.Contains(t, ce.Error(), planID209)
+	assert.False(t, setCalled, "SetPlanAccounts must NOT be called when the plan is not found")
+}
+
+func TestSetPlanAccounts_AccountNotFound(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	setCalled := false
+	store := setupAdminMock(ctx)
+	store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) {
+		return awsPlan209(), nil
+	}
+	store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		if id == missingAcct {
+			return nil, nil // store-style not-found
+		}
+		return &config.CloudAccount{ID: id, Name: "prod-aws", Provider: "aws"}, nil
+	}
+	store.SetPlanAccountsFn = func(_ context.Context, _ string, _ []string) error {
+		setCalled = true
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	body := `{"account_ids":["` + missingAcct + `"]}`
+	_, err := handler.setPlanAccounts(ctx, adminRequest(body), planID209)
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok)
+	assert.Equal(t, 404, ce.code)
+	assert.Contains(t, ce.Error(), missingAcct, "404 should reference the missing account ID")
+	assert.False(t, setCalled, "SetPlanAccounts must NOT be called when an account is not found")
+}
+
+func TestSetPlanAccounts_MixedValidAndMismatch(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	setCalled := false
+	store := setupAdminMock(ctx)
+	store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) {
+		return awsPlan209(), nil
+	}
+	store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		switch id {
+		case awsAcct209:
+			return &config.CloudAccount{ID: id, Name: "prod-aws", Provider: "aws"}, nil
+		case azureAcct1:
+			return &config.CloudAccount{ID: id, Name: "prod-azure", Provider: "azure"}, nil
+		}
+		return nil, nil
+	}
+	store.SetPlanAccountsFn = func(_ context.Context, _ string, _ []string) error {
+		setCalled = true
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	body := `{"account_ids":["` + awsAcct209 + `","` + azureAcct1 + `"]}`
+	_, 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)
+	// Only the Azure account is the offender; the AWS account does not
+	// appear in the error.
+	assert.Contains(t, ce.Error(), "prod-azure")
+	assert.NotContains(t, ce.Error(), "prod-aws")
+	assert.False(t, setCalled, "SetPlanAccounts must NOT be called when even one account fails validation")
+}
+
+func TestSetPlanAccounts_EmptyServicesSkipsValidation(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	var capturedIDs []string
+	store := setupAdminMock(ctx)
+	// Plan with an empty services map — derived provider set is empty;
+	// the validation block skips and the assignment passes through.
+	// Pins the defensive behaviour so a future change is conscious.
+	store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) {
+		return &config.PurchasePlan{ID: planID209, Name: "no-services"}, nil
+	}
+	store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		return &config.CloudAccount{ID: id, Name: "prod-azure", Provider: "azure"}, nil
+	}
+	store.SetPlanAccountsFn = func(_ context.Context, _ string, ids []string) error {
+		capturedIDs = ids
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	body := `{"account_ids":["` + azureAcct1 + `"]}`
+	result, err := handler.setPlanAccounts(ctx, adminRequest(body), planID209)
+	require.NoError(t, err)
+	assert.Nil(t, result)
+	assert.Equal(t, []string{azureAcct1}, capturedIDs, "empty services map → validation skipped → write proceeds")
+}
+
 func TestListPlanAccounts_Success(t *testing.T) {
 	ctx := context.Background()
 	mockAuth := new(MockAuthService)