From 07f7e9ea263e281ec46e5bfd41d4722816f7a598 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 16:23:31 +0200 Subject: [PATCH 1/2] fix(plans): refuse Add Purchases and account changes on managed plans Add Purchases and account assignment could still change an editor-managed plan's schedule roots and target accounts behind the editor's back. Add Purchases now refuses a managed plan whole before it writes anything, and assigning a different account set to a managed plan is refused under a share lock on the plan row, so a writer that waited behind the editor's Save sees the committed marker. Both answer 409 editor_reconciliation_required, after authorization. Re-sending the current account set still succeeds. Refs #631 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_accounts.go | 3 + internal/api/handler_plans.go | 3 + ...s_managed_add_accounts_integration_test.go | 153 ++++++++++++++++++ ...er_plans_managed_guard_integration_test.go | 16 +- internal/config/store_postgres.go | 3 + .../config/store_postgres_managed_guard.go | 57 +++++++ .../store_postgres_managed_guard_test.go | 23 +++ .../config/store_postgres_pgxmock_test.go | 9 ++ 8 files changed, 262 insertions(+), 5 deletions(-) create mode 100644 internal/api/handler_plans_managed_add_accounts_integration_test.go create mode 100644 internal/config/store_postgres_managed_guard_test.go diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 0c6a19dd..f07aabf3 100644 --- a/internal/api/handler_accounts.go +++ b/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) } diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index a378d45c..46784cd7 100644 --- a/internal/api/handler_plans.go +++ b/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 } diff --git a/internal/api/handler_plans_managed_add_accounts_integration_test.go b/internal/api/handler_plans_managed_add_accounts_integration_test.go new file mode 100644 index 00000000..1e1f1f7a --- /dev/null +++ b/internal/api/handler_plans_managed_add_accounts_integration_test.go @@ -0,0 +1,153 @@ +//go:build integration + +package api + +import ( + "context" + "errors" + "fmt" + "net/http" + "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(¤t)) + 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") + 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)) + }) + } +} diff --git a/internal/api/handler_plans_managed_guard_integration_test.go b/internal/api/handler_plans_managed_guard_integration_test.go index 0b60bde4..9f8d79cc 100644 --- a/internal/api/handler_plans_managed_guard_integration_test.go +++ b/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 { diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 37647e4f..3448d9d8 100644 --- a/internal/config/store_postgres.go +++ b/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 } diff --git a/internal/config/store_postgres_managed_guard.go b/internal/config/store_postgres_managed_guard.go index 1c3b22f5..bc40e1dc 100644 --- a/internal/config/store_postgres_managed_guard.go +++ b/internal/config/store_postgres_managed_guard.go @@ -2,9 +2,13 @@ package config import ( "bytes" + "context" "encoding/json" + "errors" "fmt" "time" + + "github.com/jackc/pgx/v5" ) // requireManagedPlanUpdateTx refuses a legacy plan write that would change what @@ -58,3 +62,56 @@ 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. +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[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 +} diff --git a/internal/config/store_postgres_managed_guard_test.go b/internal/config/store_postgres_managed_guard_test.go new file mode 100644 index 00000000..c7a7c8db --- /dev/null +++ b/internal/config/store_postgres_managed_guard_test.go @@ -0,0 +1,23 @@ +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}, + } + 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) + } + } +} diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 60366373..37b18d1a 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/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() From a4696f0f337af47fe8a2aa209c6d9e789410760d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 21:54:22 +0200 Subject: [PATCH 2/2] fix(config): compare plan account ids case-insensitively in the managed guard validatePlanAccountIDs accepts upper-case UUID hex, but Postgres returns plan_accounts.account_id from the uuid column in lower-case. sameAccountSet compared the raw strings, so resubmitting an unchanged account set written in upper-case hex on a managed plan got a false 409 editor_reconciliation_required. Lower-case both sides when building the sets. Adds a unit case and an integration case for the upper-case resubmit. Refs #631 Co-Authored-By: Claude Opus 5.5 --- .../handler_plans_managed_add_accounts_integration_test.go | 2 ++ internal/config/store_postgres_managed_guard.go | 7 +++++-- internal/config/store_postgres_managed_guard_test.go | 2 ++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_plans_managed_add_accounts_integration_test.go b/internal/api/handler_plans_managed_add_accounts_integration_test.go index 1e1f1f7a..21cdaa98 100644 --- a/internal/api/handler_plans_managed_add_accounts_integration_test.go +++ b/internal/api/handler_plans_managed_add_accounts_integration_test.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "net/http" + "strings" "testing" "time" @@ -78,6 +79,7 @@ func TestAssignAccountsOnManagedPlan(t *testing.T) { 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) diff --git a/internal/config/store_postgres_managed_guard.go b/internal/config/store_postgres_managed_guard.go index bc40e1dc..9ca35d9c 100644 --- a/internal/config/store_postgres_managed_guard.go +++ b/internal/config/store_postgres_managed_guard.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "strings" "time" "github.com/jackc/pgx/v5" @@ -95,12 +96,14 @@ func requireManagedAccountsTx(ctx context.Context, tx pgx.Tx, planID string, acc } // sameAccountSet compares account id lists as sets, so request duplicates -// collapse the way ON CONFLICT DO NOTHING collapses them on write. +// 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[id] = struct{}{} + m[strings.ToLower(id)] = struct{}{} } return m } diff --git a/internal/config/store_postgres_managed_guard_test.go b/internal/config/store_postgres_managed_guard_test.go index c7a7c8db..c41b6d78 100644 --- a/internal/config/store_postgres_managed_guard_test.go +++ b/internal/config/store_postgres_managed_guard_test.go @@ -14,6 +14,8 @@ func TestSameAccountSet(t *testing.T) { {"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 {