fix(config/db): seed-group rollbacks, partition-safe 000027 dedupe, fail-closed plan providers, revocation_in_flight clear

internal/api/handler_accounts.go

@@ -1262,9 +1262,12 @@ func (h *Handler) deleteAccountServiceOverride(ctx context.Context, req *events.
 //   - 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)
+//   - 500 ClientError when a stored service key names no provider (fail
+//     closed: skipping the key could leave the provider set empty, which
+//     would accept an account of any provider, issue #316)
+//   - nil when all accounts match (or when the plan has no services at
+//     all — 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
@@ -1275,7 +1278,13 @@ func (h *Handler) validatePlanAccountProviders(ctx context.Context, planID strin
 		return err
 	}
 
-	expected := config.DerivePlanProviders(plan)
+	expected, err := config.DerivePlanProviders(plan)
+	if err != nil {
+		// The key is stored data, not the caller's input; log it and keep
+		// the plan UUID out of the response, as the 500s above do.
+		logging.Errorf("validatePlanAccountProviders: %v", err)
+		return NewClientError(500, "failed to validate plan accounts")
+	}
 	if len(expected) == 0 {
 		return nil
 	}

internal/api/handler_accounts_test.go

@@ -1430,6 +1430,128 @@ func TestSetPlanAccounts_EmptyServicesSkipsValidation(t *testing.T) {
 	assert.Equal(t, []string{azureAcct1}, capturedIDs, "empty services map → validation skipped → write proceeds")
 }
 
+// TestSetPlanAccounts_ColonKeyedPlanStillChecksProvider: migration 000040
+// wrote "aws:savings-plans-<type>" keys. DerivePlanProviders used to skip
+// them, so a plan keyed only that way had an empty provider set and accepted
+// an Azure account (issue #316, A04-012).
+func TestSetPlanAccounts_ColonKeyedPlanStillChecksProvider(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 &config.PurchasePlan{ID: planID209, Name: "sp-plan", Services: map[string]config.ServiceConfig{
+			"aws:savings-plans-compute":     {Provider: "aws", Service: "savings-plans"},
+			"aws:savings-plans-ec2instance": {Provider: "aws", Service: "savings-plans"},
+		}}, 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, "an Azure account must not be assignable to an AWS-only plan")
+	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.False(t, setCalled, "SetPlanAccounts must NOT be called when validation fails")
+}
+
+// TestSetPlanAccounts_LegacyKeyedPlanUsesProviderField: a legacy alias kept
+// under its original key (#1123) has no provider prefix, but its Provider
+// field names one. A matching account is accepted and a mismatched one is
+// refused, instead of the plan accepting every provider (main) or none (the
+// first #316 commit).
+func TestSetPlanAccounts_LegacyKeyedPlanUsesProviderField(t *testing.T) {
+	ctx := context.Background()
+	for _, tc := range []struct {
+		name, accountProvider string
+		wantCode              int
+	}{
+		{"matching provider accepted", "azure", 0},
+		{"mismatched provider refused", "aws", 400},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			mockAuth := new(MockAuthService)
+			setupAdminAuth(ctx, mockAuth)
+			setCalled := false
+			store := setupAdminMock(ctx)
+			store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) {
+				return &config.PurchasePlan{ID: planID209, Name: "alias plan", Services: map[string]config.ServiceConfig{
+					"original-key": {Provider: "azure", Service: "vm", Enabled: true},
+				}}, nil
+			}
+			store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+				return &config.CloudAccount{ID: id, Name: "acct-" + tc.accountProvider, Provider: tc.accountProvider}, 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)
+			if tc.wantCode == 0 {
+				require.NoError(t, err)
+				assert.True(t, setCalled)
+				return
+			}
+			require.Error(t, err)
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "expected a clientError, got %T", err)
+			assert.Equal(t, tc.wantCode, ce.code)
+			assert.False(t, setCalled)
+		})
+	}
+}
+
+// TestSetPlanAccounts_UnparseableServiceKeyFailsClosed: stored entries that
+// identify no provider (no prefix in the key, empty Provider field) used to be
+// skipped silently; when none identified one, the provider set came out empty
+// and any account was accepted. It now fails the request instead (issue #316,
+// A04-012).
+func TestSetPlanAccounts_UnparseableServiceKeyFailsClosed(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 &config.PurchasePlan{ID: planID209, Name: "bad-key", Services: map[string]config.ServiceConfig{
+			"ec2":    {Service: "ec2"},
+			"future": {},
+		}}, 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, 500, ce.code)
+	assert.NotContains(t, ce.Error(), planID209, "the plan UUID must not leak into the response")
+	assert.False(t, setCalled, "SetPlanAccounts must NOT be called when the plan's providers cannot be derived")
+}
+
 // 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

internal/config/interfaces.go

@@ -372,7 +372,9 @@ type StoreInterface interface {
 	// support_case_id on a purchase_history row identified by purchase_id.
 	// calcRefundAmount and calcRefundCurrency capture the Azure CalculateRefund
 	// quote for audit (migration 000071, Finding #4); both nil/empty for
-	// non-Azure paths or legacy rows written before the migration.
+	// non-Azure paths or legacy rows written before the migration. Also
+	// clears revocation_in_flight (migration 000072): a revoked row is no
+	// longer in flight.
 	// Returns a not-found error when no row matches. Idempotent: a second
 	// call for the same row is a no-op (revoked_at is not overwritten when
 	// it is already non-null). Used by the revoke endpoint (issue #290).

internal/config/purchase_plan_providers.go

@@ -1,29 +1,52 @@
 package config
 
 import (
+	"fmt"
 	"sort"
 	"strings"
 )
 
-// DerivePlanProviders extracts the distinct set of providers a plan targets
-// by parsing the keys of plan.Services. Keys are expected to use the
-// "provider/service" format produced by buildServiceConfig.
-func DerivePlanProviders(plan *PurchasePlan) []string {
+// DerivePlanProviders extracts the distinct set of providers a plan targets.
+// Each service entry names its provider through its key, in the
+// "provider/service" form buildServiceConfig writes or the legacy
+// "provider:service" form migration 000040 wrote (internal/api findServiceKey
+// accepts both). An entry whose key has neither form, such as a legacy alias
+// kept under its original key, is identified by its own Provider field, the
+// same field the legacy edit path (mergeRequestedServices) and plan step
+// resolution go by.
+//
+// An entry identified neither way is skipped: that can only narrow the set,
+// so the check stays at least as strict. What it must never do is leave the
+// set empty, because callers read an empty set as "no restriction" and would
+// accept an account of any provider (issue #316). So when the plan has
+// services but none of them identifies a provider, this returns an error. A
+// nil plan, or a plan with no services, yields an empty set.
+func DerivePlanProviders(plan *PurchasePlan) ([]string, error) {
 	if plan == nil {
-		return nil
+		return nil, nil
 	}
 	seen := make(map[string]struct{}, len(plan.Services))
 	for k := range plan.Services {
-		provider, _, ok := strings.Cut(k, "/")
-		if !ok || provider == "" {
-			continue
+		if provider := serviceEntryProvider(k, plan.Services[k].Provider); provider != "" {
+			seen[provider] = struct{}{}
 		}
-		seen[provider] = struct{}{}
+	}
+	if len(seen) == 0 && len(plan.Services) > 0 {
+		return nil, fmt.Errorf("none of the plan's %d services identifies a provider by key or provider field", len(plan.Services))
 	}
 	providers := make([]string, 0, len(seen))
 	for provider := range seen {
 		providers = append(providers, provider)
 	}
 	sort.Strings(providers)
-	return providers
+	return providers, nil
+}
+
+// serviceEntryProvider returns the provider a services entry names: the key's
+// prefix when the key has one, else the entry's Provider field, else "".
+func serviceEntryProvider(key, fieldProvider string) string {
+	if i := strings.IndexAny(key, "/:"); i > 0 {
+		return key[:i]
+	}
+	return strings.TrimSpace(fieldProvider)
 }

internal/config/purchase_plan_providers_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestDerivePlanProviders(t *testing.T) {
@@ -12,11 +13,90 @@ func TestDerivePlanProviders(t *testing.T) {
 			"aws/ec2":       {},
 			"aws/rds":       {},
 			"azure/compute": {},
-			"gcp:compute":   {},
-			"malformed":     {},
 		},
 	}
 
-	assert.Equal(t, []string{"aws", "azure"}, DerivePlanProviders(plan))
-	assert.Nil(t, DerivePlanProviders(nil))
+	got, err := DerivePlanProviders(plan)
+	require.NoError(t, err)
+	assert.Equal(t, []string{"aws", "azure"}, got)
+
+	got, err = DerivePlanProviders(nil)
+	require.NoError(t, err)
+	assert.Nil(t, got)
+
+	got, err = DerivePlanProviders(&PurchasePlan{})
+	require.NoError(t, err)
+	assert.Empty(t, got, "a plan with no services restricts nothing")
+}
+
+// TestDerivePlanProviders_ColonKeys covers the "provider:service" keys that
+// migration 000040 wrote (issue #316, A04-012). They name a provider just as
+// clearly as the "provider/service" keys buildServiceConfig writes, and
+// findServiceKey in internal/api already treats them as valid. Skipping them
+// left a colon-keyed plan with an empty provider set, which both
+// account-assignment guards read as "no restriction".
+func TestDerivePlanProviders_ColonKeys(t *testing.T) {
+	plan := &PurchasePlan{
+		Services: map[string]ServiceConfig{
+			"aws:savings-plans-compute":     {},
+			"aws:savings-plans-ec2instance": {},
+			"gcp/compute":                   {},
+		},
+	}
+
+	got, err := DerivePlanProviders(plan)
+	require.NoError(t, err)
+	assert.Equal(t, []string{"aws", "gcp"}, got)
+}
+
+// TestDerivePlanProviders_UnidentifiableEntries: an entry whose key names no
+// provider and whose Provider field is empty is skipped when other entries do
+// identify one (skipping can only narrow the set, and plans may carry entries
+// this version does not know). When no entry identifies a provider the result
+// must be an error, not an empty set: both callers read an empty set as
+// "accept an account of any provider" (issue #316, A04-012).
+func TestDerivePlanProviders_UnidentifiableEntries(t *testing.T) {
+	for _, key := range []string{"malformed", "/ec2", ":ec2", ""} {
+		t.Run("only "+key, func(t *testing.T) {
+			got, err := DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{key: {Service: "ec2", Provider: "  "}}})
+			require.Error(t, err)
+			assert.Nil(t, got)
+			assert.Contains(t, err.Error(), "identifies a provider")
+		})
+		t.Run("alongside aws/ec2 "+key, func(t *testing.T) {
+			got, err := DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{
+				"aws/ec2": {},
+				key:       {},
+			}})
+			require.NoError(t, err)
+			assert.Equal(t, []string{"aws"}, got)
+		})
+	}
+}
+
+// TestDerivePlanProviders_LegacyKeyUsesProviderField: a legacy alias kept
+// under its original key (the singleton-alias edit path, #1123) has no
+// provider in its key, but its ServiceConfig names one. That is the field the
+// legacy edit path and plan step resolution go by, so it counts. Main skipped
+// such keys (provider set empty, no restriction); the first #316 commit
+// refused them outright.
+func TestDerivePlanProviders_LegacyKeyUsesProviderField(t *testing.T) {
+	got, err := DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{
+		"original-key": {Provider: "azure", Service: "vm"},
+	}})
+	require.NoError(t, err)
+	assert.Equal(t, []string{"azure"}, got)
+
+	got, err = DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{
+		"aws/ec2":      {Provider: "aws", Service: "ec2"},
+		"original-key": {Provider: "azure", Service: "vm"},
+	}})
+	require.NoError(t, err)
+	assert.Equal(t, []string{"aws", "azure"}, got, "every entry's provider counts, since plan step resolution buys for each")
+
+	got, err = DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{
+		"gcp/compute": {Provider: "azure", Service: "compute"},
+	}})
+	require.NoError(t, err)
+	assert.Equal(t, []string{"gcp"}, got, "a key that names a provider still wins, as before")
 }

internal/config/store_postgres.go

@@ -2920,9 +2920,11 @@ func (s *PostgresStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purc
 
 // MarkPurchaseRevoked stamps revoked_at / revoked_via / support_case_id and
 // the refund-quote audit columns (calc_refund_amount, calc_refund_currency) on
-// the purchase_history row identified by purchaseID. The UPDATE is a no-op
-// when revoked_at is already non-null (idempotency guard). Returns a not-found
-// error when zero rows are affected and revoked_at was previously NULL.
+// the purchase_history row identified by purchaseID, and clears
+// revocation_in_flight as migration 000072 documents: a completed revocation
+// is no longer in flight (issue #316). The UPDATE is a no-op when revoked_at
+// is already non-null (idempotency guard). Returns a not-found error when zero
+// rows are affected and revoked_at was previously NULL.
 func (s *PostgresStore) MarkPurchaseRevoked(ctx context.Context, purchaseID string, revokedAt time.Time, revokedVia, supportCaseID string, calcRefundAmount *float64, calcRefundCurrency string) error {
 	var supportCaseIDPtr *string
 	if supportCaseID != "" {
@@ -2938,7 +2940,8 @@ func (s *PostgresStore) MarkPurchaseRevoked(ctx context.Context, purchaseID stri
 		       revoked_via           = $3,
 		       support_case_id       = $4,
 		       calc_refund_amount    = $5,
-		       calc_refund_currency  = $6
+		       calc_refund_currency  = $6,
+		       revocation_in_flight  = false
 		 WHERE purchase_id = $1
 		   AND revoked_at IS NULL
 	`, purchaseID, revokedAt, revokedVia, supportCaseIDPtr, calcRefundAmount, calcCurrencyPtr)
@@ -4402,7 +4405,10 @@ func (s *PostgresStore) validatePlanAccountProvidersTx(ctx context.Context, tx p
 	if len(accountIDs) == 0 {
 		return nil
 	}
-	expected := DerivePlanProviders(&PurchasePlan{Services: services})
+	expected, err := DerivePlanProviders(&PurchasePlan{Services: services})
+	if err != nil {
+		return fmt.Errorf("validate plan account providers: %w", err)
+	}
 	if len(expected) == 0 {
 		return nil
 	}