fix(plans): refuse Add Purchases and account changes on managed plans

internal/api/handler_accounts.go

@@ -1329,6 +1329,9 @@ func (h *Handler) setPlanAccounts(ctx context.Context, httpReq *events.LambdaFun
 		if errors.Is(err, config.ErrNotFound) {
 			return nil, NewClientError(404, err.Error())
 		}
+		if errors.Is(err, config.ErrEditorReconciliationRequired) {
+			return nil, editorReconciliationError()
+		}
 		return nil, fmt.Errorf("accounts: %w", err)
 	}
 

internal/api/handler_plans.go

@@ -511,6 +511,9 @@ func (h *Handler) createPlannedPurchasesTx(ctx context.Context, tx pgx.Tx, planI
 	if err := h.requirePlanMutationAccess(ctx, session, planID); err != nil {
 		return 0, err
 	}
+	if plan.RampSchedule.DatesManaged {
+		return 0, editorReconciliationError()
+	}
 	if err := h.validatePurchaseScheduling(ctx, plan, count, startDate); err != nil {
 		return 0, err
 	}

internal/api/handler_plans_managed_add_accounts_integration_test.go

@@ -0,0 +1,155 @@
+//go:build integration
+
+package api
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"net/http"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/stretchr/testify/require"
+)
+
+func (f *createConcurrencyFixture) addPurchases(ctx context.Context, h *Handler, startDate time.Time) error {
+	body := fmt.Sprintf(`{"count":1,"start_date":%q}`, startDate.Format("2006-01-02"))
+	_, err := h.createPlannedPurchases(ctx, managedGuardRequest(body), f.planID)
+	return err
+}
+
+func (f *createConcurrencyFixture) assign(ctx context.Context, h *Handler, ids ...string) error {
+	body := `{"account_ids":["` + ids[0] + `"`
+	for _, id := range ids[1:] {
+		body += `,"` + id + `"`
+	}
+	_, err := h.setPlanAccounts(ctx, managedGuardRequest(body+`]}`), f.planID)
+	return err
+}
+
+func (f *createConcurrencyFixture) count(ctx context.Context, t *testing.T, query string) (n int) {
+	t.Helper()
+	require.NoError(t, f.pool.QueryRow(ctx, query, f.planID).Scan(&n))
+	return n
+}
+
+func (f *createConcurrencyFixture) extraAccount(ctx context.Context, t *testing.T, name string) string {
+	t.Helper()
+	acct := &config.CloudAccount{Name: name, Provider: "azure", ExternalID: "ext-" + name, Enabled: true}
+	require.NoError(t, f.store.CreateCloudAccount(ctx, acct))
+	return acct.ID
+}
+
+const (
+	execCount = `SELECT count(*) FROM purchase_executions WHERE plan_id = $1`
+	acctCount = `SELECT count(*) FROM plan_accounts WHERE plan_id = $1`
+)
+
+func TestAddPurchasesRefusesManagedPlanWhole(t *testing.T) {
+	ctx := context.Background()
+	f := newCreateConcurrencyFixture(ctx, t)
+	start := time.Now().AddDate(0, 0, 7).UTC().Truncate(24 * time.Hour)
+
+	setPlanManaged(ctx, t, f, true)
+	// Equal dates make the later plan write a no-op, so only the early guard refuses.
+	_, err := f.pool.Exec(ctx, `UPDATE purchase_plans SET next_execution_date = $2,
+		ramp_schedule = jsonb_set(ramp_schedule, '{start_date}', to_jsonb(to_char($2::timestamptz AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS"Z"'))) WHERE id = $1`, f.planID, start)
+	require.NoError(t, err)
+	requireEditorRefusal(t, f.addPurchases(ctx, f.handler, start))
+	require.Zero(t, f.count(ctx, t, execCount))
+
+	setPlanManaged(ctx, t, f, false)
+	require.NoError(t, f.addPurchases(ctx, f.handler, start))
+	require.Equal(t, 1, f.count(ctx, t, execCount))
+}
+
+func TestAssignAccountsOnManagedPlan(t *testing.T) {
+	ctx := context.Background()
+	f := newCreateConcurrencyFixture(ctx, t)
+	var current string
+	require.NoError(t, f.pool.QueryRow(ctx, `SELECT account_id::text FROM plan_accounts WHERE plan_id = $1`, f.planID).Scan(&current))
+	other := f.extraAccount(ctx, t, "acct-b")
+
+	require.NoError(t, f.assign(ctx, f.handler, current, other), "unmanaged plan assigns normally")
+	require.NoError(t, f.assign(ctx, f.handler, current))
+	setPlanManaged(ctx, t, f, true)
+
+	require.NoError(t, f.assign(ctx, f.handler, current, current), "identical set with a duplicate passes")
+	require.NoError(t, f.assign(ctx, f.handler, strings.ToUpper(current)), "identical set in upper-case hex passes")
+	for name, ids := range map[string][]string{"added": {current, other}, "replaced": {other}} {
+		requireEditorRefusal(t, f.assign(ctx, f.handler, ids...))
+		require.Equal(t, 1, f.count(ctx, t, acctCount), name)
+	}
+}
+
+func TestAddAndAssignAuthorizationPrecedeManagedGuard(t *testing.T) {
+	ctx := context.Background()
+	f := newCreateConcurrencyFixture(ctx, t)
+	setPlanManaged(ctx, t, f, true)
+	other := f.extraAccount(ctx, t, "acct-b")
+	view := []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourcePlans}}
+	admin := []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}
+	start := time.Now().AddDate(0, 0, 7)
+
+	for name, tc := range map[string]struct {
+		perms  []auth.Permission
+		scoped []string
+		status int
+	}{
+		"no permission": {view, nil, http.StatusForbidden},
+		"out of scope":  {admin, []string{"unrelated-account"}, http.StatusNotFound},
+	} {
+		h := limitedHandler(f, tc.perms, tc.scoped)
+		for op, err := range map[string]error{"add": f.addPurchases(ctx, h, start), "assign": f.assign(ctx, h, other)} {
+			var ce *clientError
+			if errors.As(err, &ce) {
+				require.Equal(t, tc.status, ce.code, "%s %s: %s", name, op, ce.message)
+			} else {
+				require.True(t, IsNotFoundError(err) && tc.status == http.StatusNotFound, "%s %s: %v", name, op, err)
+			}
+		}
+	}
+}
+
+// A writer that starts while the editor command holds the plan lock waits, then
+// judges the committed marker.
+func TestManagedMarkerCommittedUnderLockRefusesWaitingWriters(t *testing.T) {
+	ctx := context.Background()
+	other := func(f *createConcurrencyFixture) string { return f.extraAccount(ctx, t, "acct-b") }
+	writers := map[string]func(f *createConcurrencyFixture, otherID string) error{
+		"add": func(f *createConcurrencyFixture, _ string) error {
+			return f.addPurchases(ctx, f.handler, time.Now().AddDate(0, 0, 7))
+		},
+		"assign": func(f *createConcurrencyFixture, o string) error { return f.assign(ctx, f.handler, o) },
+	}
+	for name, write := range writers {
+		t.Run(name, func(t *testing.T) {
+			f := newCreateConcurrencyFixture(ctx, t)
+			o := other(f)
+			command, err := f.pool.Begin(ctx)
+			require.NoError(t, err)
+			defer func() { _ = command.Rollback(ctx) }()
+			_, err = command.Exec(ctx, `SELECT 1 FROM purchase_plans WHERE id = $1 FOR UPDATE`, f.planID)
+			require.NoError(t, err)
+
+			done := make(chan error, 1)
+			go func() { done <- write(f, o) }()
+			require.Eventually(t, func() bool {
+				var n int
+				require.NoError(t, f.pool.QueryRow(ctx, `SELECT count(*) FROM pg_stat_activity WHERE wait_event_type = 'Lock'`).Scan(&n))
+				return n > 0
+			}, 15*time.Second, 50*time.Millisecond, "writer must wait on the plan lock")
+
+			_, err = command.Exec(ctx, `UPDATE purchase_plans SET ramp_schedule = ramp_schedule || '{"dates_managed":true}' WHERE id = $1`, f.planID)
+			require.NoError(t, err)
+			require.NoError(t, command.Commit(ctx))
+			requireEditorRefusal(t, <-done)
+			require.Zero(t, f.count(ctx, t, execCount))
+			require.Equal(t, 1, f.count(ctx, t, acctCount))
+		})
+	}
+}

internal/api/handler_plans_managed_guard_integration_test.go

@@ -41,7 +41,7 @@ func setPlanManaged(ctx context.Context, t *testing.T, f *createConcurrencyFixtu
 		markers = `{"dates_managed":true,"allocation_mode":"equal_count"}`
 	}
 	_, err := f.pool.Exec(ctx, `UPDATE purchase_plans SET next_execution_date = now() + interval '3 days',
-		ramp_schedule = jsonb_set(ramp_schedule, '{current_step}', '0') || $2::jsonb WHERE id = $1`, f.planID, markers)
+		ramp_schedule = (jsonb_set(ramp_schedule, '{current_step}', '0') - 'dates_managed') || $2::jsonb WHERE id = $1`, f.planID, markers)
 	require.NoError(t, err)
 }
 
@@ -98,6 +98,15 @@ func TestManagedPlanRefusalIsHTTP409WithCode(t *testing.T) {
 	require.JSONEq(t, `{"error":"this plan is managed by the schedule editor; make this change in the editor","code":"editor_reconciliation_required"}`, resp.Body)
 }
 
+// limitedHandler serves the same store to a caller holding only perms, with
+// account scope restricted to scoped (nil means unrestricted).
+func limitedHandler(f *createConcurrencyFixture, perms []auth.Permission, scoped []string) *Handler {
+	limited := new(MockAuthService)
+	limited.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{UserID: "00000000-0000-0000-0000-000000000001", Email: "limited@example.com"}, nil).Maybe()
+	limited.grantPermissionsScoped(perms, scoped)
+	return &Handler{config: f.store, auth: limited}
+}
+
 func TestManagedPlanAuthorizationPrecedesGuard(t *testing.T) {
 	ctx := context.Background()
 	update := auth.Permission{Action: auth.ActionUpdate, Resource: auth.ResourcePlans}
@@ -120,10 +129,7 @@ func TestManagedPlanAuthorizationPrecedesGuard(t *testing.T) {
 			setPlanManaged(ctx, t, f, true)
 			_, err := f.pool.Exec(ctx, `UPDATE purchase_plans SET max_step_commitment = 1000 WHERE id = $1`, f.planID)
 			require.NoError(t, err)
-			limited := new(MockAuthService)
-			limited.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{UserID: "00000000-0000-0000-0000-000000000001", Email: "limited@example.com"}, nil).Maybe()
-			limited.grantPermissionsScoped(tc.perms, tc.scoped)
-			h := &Handler{config: f.store, auth: limited}
+			h := limitedHandler(f, tc.perms, tc.scoped)
 			if tc.patch {
 				_, err = h.patchPlan(ctx, managedGuardRequest(tc.body), f.planID)
 			} else {

internal/config/store_postgres.go

@@ -4147,6 +4147,9 @@ func (s *PostgresStore) SetPlanAccounts(ctx context.Context, planID string, acco
 	}
 	defer rollbackTx(ctx, tx, "SetPlanAccounts")
 
+	if guardErr := requireManagedAccountsTx(ctx, tx, planID, accountIDs); guardErr != nil {
+		return guardErr
+	}
 	if setErr := s.setPlanAccountsTx(ctx, tx, planID, accountIDs); setErr != nil {
 		return setErr
 	}

internal/config/store_postgres_managed_guard.go

@@ -2,9 +2,14 @@ package config
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"strings"
 	"time"
+
+	"github.com/jackc/pgx/v5"
 )
 
 // requireManagedPlanUpdateTx refuses a legacy plan write that would change what
@@ -58,3 +63,58 @@ func sameDate(a, b *time.Time) bool {
 	}
 	return a.Equal(*b)
 }
+
+// requireManagedAccountsTx refuses a changed account set on an editor-managed
+// plan. The plan row is read FOR SHARE, the lock SetPlanAccounts already takes
+// for its provider check, so an editor command holding FOR UPDATE makes the
+// writer wait and then see the committed marker. An identical set passes: the
+// legacy edit modal re-sends the unchanged selection after every rename.
+func requireManagedAccountsTx(ctx context.Context, tx pgx.Tx, planID string, accountIDs []string) error {
+	var managed bool
+	err := tx.QueryRow(ctx,
+		`SELECT COALESCE(ramp_schedule->>'dates_managed' = 'true', false) FROM purchase_plans WHERE id = $1 FOR SHARE`,
+		planID).Scan(&managed)
+	if errors.Is(err, pgx.ErrNoRows) || (err == nil && !managed) {
+		return nil
+	}
+	if err != nil {
+		return fmt.Errorf("failed to check plan management: %w", err)
+	}
+	rows, err := tx.Query(ctx, `SELECT account_id FROM plan_accounts WHERE plan_id = $1`, planID)
+	if err != nil {
+		return fmt.Errorf("failed to read plan accounts: %w", err)
+	}
+	current, err := pgx.CollectRows(rows, pgx.RowTo[string])
+	if err != nil {
+		return fmt.Errorf("failed to read plan accounts: %w", err)
+	}
+	same := sameAccountSet(current, accountIDs)
+	if !same {
+		return fmt.Errorf("%w: account set changed on plan %s", ErrEditorReconciliationRequired, planID)
+	}
+	return nil
+}
+
+// sameAccountSet compares account id lists as sets, so request duplicates
+// collapse the way ON CONFLICT DO NOTHING collapses them on write. Ids are
+// lower-cased because the uuid column returns lower-case text while request
+// validation accepts upper-case hex.
+func sameAccountSet(a, b []string) bool {
+	set := func(ids []string) map[string]struct{} {
+		m := make(map[string]struct{}, len(ids))
+		for _, id := range ids {
+			m[strings.ToLower(id)] = struct{}{}
+		}
+		return m
+	}
+	sa, sb := set(a), set(b)
+	if len(sa) != len(sb) {
+		return false
+	}
+	for id := range sa {
+		if _, ok := sb[id]; !ok {
+			return false
+		}
+	}
+	return true
+}

internal/config/store_postgres_managed_guard_test.go

@@ -0,0 +1,25 @@
+package config
+
+import "testing"
+
+func TestSameAccountSet(t *testing.T) {
+	cases := []struct {
+		name string
+		a, b []string
+		want bool
+	}{
+		{"reordered", []string{"a", "b"}, []string{"b", "a"}, true},
+		{"duplicate", []string{"a"}, []string{"a", "a"}, true},
+		{"added", []string{"a"}, []string{"a", "b"}, false},
+		{"removed", []string{"a", "b"}, []string{"a"}, false},
+		{"replaced", []string{"a"}, []string{"b"}, false},
+		{"both empty", nil, []string{}, true},
+		{"mixed case", []string{"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"}, []string{"AAAAAAAA-BBBB-CCCC-DDDD-EEEEEEEEEEEE"}, true},
+		{"mixed case duplicate", []string{"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"}, []string{"AAAAAAAA-BBBB-CCCC-DDDD-EEEEEEEEEEEE", "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"}, true},
+	}
+	for _, tc := range cases {
+		if got := sameAccountSet(tc.a, tc.b); got != tc.want {
+			t.Errorf("%s: got %v, want %v", tc.name, got, tc.want)
+		}
+	}
+}

internal/config/store_postgres_pgxmock_test.go

@@ -2036,6 +2036,11 @@ func TestPGXMock_ListAccountServiceOverrides_Error(t *testing.T) {
 
 // ─── SetPlanAccounts ──────────────────────────────────────────────────────────
 
+func expectUnmanagedPlan(mock pgxmock.PgxPoolIface) {
+	mock.ExpectQuery("SELECT COALESCE.*FOR SHARE").WithArgs("plan-1").
+		WillReturnRows(pgxmock.NewRows([]string{"managed"}).AddRow(false))
+}
+
 func TestPGXMock_SetPlanAccounts_Success(t *testing.T) {
 	mock := newMock(t)
 	store := storeWith(mock)
@@ -2046,6 +2051,7 @@ func TestPGXMock_SetPlanAccounts_Success(t *testing.T) {
 	require.NoError(t, err)
 
 	mock.ExpectBegin()
+	expectUnmanagedPlan(mock)
 	mock.ExpectQuery("SELECT services").WithArgs("plan-1").WillReturnRows(
 		pgxmock.NewRows([]string{"services"}).AddRow(servicesJSON),
 	)
@@ -2080,6 +2086,7 @@ func TestPGXMock_SetPlanAccounts_ProviderMismatchRollsBackBeforeDelete(t *testin
 	require.NoError(t, err)
 
 	mock.ExpectBegin()
+	expectUnmanagedPlan(mock)
 	mock.ExpectQuery("SELECT services").WithArgs("plan-1").WillReturnRows(
 		pgxmock.NewRows([]string{"services"}).AddRow(servicesJSON),
 	)
@@ -2105,6 +2112,7 @@ func TestPGXMock_SetPlanAccounts_Empty(t *testing.T) {
 	require.NoError(t, err)
 
 	mock.ExpectBegin()
+	expectUnmanagedPlan(mock)
 	mock.ExpectQuery("SELECT services").WithArgs("plan-1").WillReturnRows(
 		pgxmock.NewRows([]string{"services"}).AddRow(servicesJSON),
 	)
@@ -2123,6 +2131,7 @@ func TestPGXMock_SetPlanAccounts_EmptyMissingPlanReturnsNotFound(t *testing.T) {
 	ctx := context.Background()
 
 	mock.ExpectBegin()
+	mock.ExpectQuery("SELECT COALESCE.*FOR SHARE").WithArgs("missing-plan").WillReturnError(pgx.ErrNoRows)
 	mock.ExpectQuery("SELECT services").WithArgs("missing-plan").WillReturnError(pgx.ErrNoRows)
 	mock.ExpectRollback()