From 8178ca217baaa0bb86afa95af6a403f4d57ed553 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 11:03:53 +0000 Subject: [PATCH 01/10] feat(api): add the override transfer row and its CSV codec Issue #9 needs a file format for account_service_overrides that can move between tenants, so each row carries the account's provider and external ID next to its UUID. The CSV form uses the export's column names, semicolon-joined lists and empty cells for unset fields, and prefixes formula-like account names so a spreadsheet does not run them. Refs #9 Co-Authored-By: Claude Opus 5.5 --- internal/api/override_transfer.go | 135 ++++++++++++++++ internal/api/override_transfer_csv.go | 221 ++++++++++++++++++++++++++ 2 files changed, 356 insertions(+) create mode 100644 internal/api/override_transfer.go create mode 100644 internal/api/override_transfer_csv.go diff --git a/internal/api/override_transfer.go b/internal/api/override_transfer.go new file mode 100644 index 000000000..a08881c6b --- /dev/null +++ b/internal/api/override_transfer.go @@ -0,0 +1,135 @@ +package api + +import ( + "reflect" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// OverrideTransferRow is one account_service_overrides row in an export or +// import file (issue #9). The account is named twice: AccountID is the +// tenant-local UUID, and Provider plus AccountExternalID (AWS account number, +// Azure subscription, GCP project) identify it portably, so a file exported +// from one tenant can be imported into another. AccountName, CreatedAt and +// UpdatedAt are informational: import ignores them. +// +// An import row is the desired state of the override: an unset field clears +// that field, unlike the sparse PUT /api/accounts/:id/service-overrides/... +// merge. Overrides missing from the file are left alone. +type OverrideTransferRow struct { + AccountID string `json:"account_id,omitempty"` + AccountName string `json:"account_name,omitempty"` + AccountExternalID string `json:"account_external_id,omitempty"` + Provider string `json:"provider"` + Service string `json:"service"` + Enabled *bool `json:"enabled,omitempty"` + Term *int `json:"term,omitempty"` + Payment *string `json:"payment,omitempty"` + Coverage *float64 `json:"coverage,omitempty"` + RampSchedule *string `json:"ramp_schedule,omitempty"` + IncludeEngines []string `json:"include_engines,omitempty"` + ExcludeEngines []string `json:"exclude_engines,omitempty"` + IncludeRegions []string `json:"include_regions,omitempty"` + ExcludeRegions []string `json:"exclude_regions,omitempty"` + IncludeTypes []string `json:"include_types,omitempty"` + ExcludeTypes []string `json:"exclude_types,omitempty"` + CreatedAt *time.Time `json:"created_at,omitempty"` + UpdatedAt *time.Time `json:"updated_at,omitempty"` +} + +// OverrideExport is the JSON export document. +type OverrideExport struct { + Version int `json:"version"` + ExportedAt time.Time `json:"exported_at"` + Overrides []OverrideTransferRow `json:"overrides"` +} + +// overrideExportVersion is bumped when the row shape changes incompatibly. +const overrideExportVersion = 1 + +// overrideTransferRowFrom renders a stored override with its account. +func overrideTransferRowFrom(acct *config.CloudAccount, o *config.AccountServiceOverride) OverrideTransferRow { + row := OverrideTransferRow{ + AccountID: acct.ID, + AccountName: acct.Name, + AccountExternalID: acct.ExternalID, + Provider: o.Provider, + Service: o.Service, + CreatedAt: timePtrOrNil(o.CreatedAt), + UpdatedAt: timePtrOrNil(o.UpdatedAt), + } + setOverrideFields(&row, overrideFieldsOf(o)) + return row +} + +func timePtrOrNil(t time.Time) *time.Time { + if t.IsZero() { + return nil + } + u := t.UTC() + return &u +} + +// overrideFields are the settable fields of an override, shared by the stored +// row and the transfer row so import can compare and copy them as one value. +type overrideFields struct { + Enabled *bool + Term *int + Payment *string + Coverage *float64 + RampSchedule *string + IncludeEngines []string + ExcludeEngines []string + IncludeRegions []string + ExcludeRegions []string + IncludeTypes []string + ExcludeTypes []string +} + +func overrideFieldsOf(o *config.AccountServiceOverride) overrideFields { + return overrideFields{ + Enabled: o.Enabled, Term: o.Term, Payment: o.Payment, Coverage: o.Coverage, RampSchedule: o.RampSchedule, + IncludeEngines: o.IncludeEngines, ExcludeEngines: o.ExcludeEngines, + IncludeRegions: o.IncludeRegions, ExcludeRegions: o.ExcludeRegions, + IncludeTypes: o.IncludeTypes, ExcludeTypes: o.ExcludeTypes, + } +} + +func (r *OverrideTransferRow) fields() overrideFields { + return overrideFields{ + Enabled: r.Enabled, Term: r.Term, Payment: r.Payment, Coverage: r.Coverage, RampSchedule: r.RampSchedule, + IncludeEngines: r.IncludeEngines, ExcludeEngines: r.ExcludeEngines, + IncludeRegions: r.IncludeRegions, ExcludeRegions: r.ExcludeRegions, + IncludeTypes: r.IncludeTypes, ExcludeTypes: r.ExcludeTypes, + } +} + +func setOverrideFields(r *OverrideTransferRow, f overrideFields) { + r.Enabled, r.Term, r.Payment, r.Coverage, r.RampSchedule = f.Enabled, f.Term, f.Payment, f.Coverage, f.RampSchedule + r.IncludeEngines, r.ExcludeEngines = f.IncludeEngines, f.ExcludeEngines + r.IncludeRegions, r.ExcludeRegions = f.IncludeRegions, f.ExcludeRegions + r.IncludeTypes, r.ExcludeTypes = f.IncludeTypes, f.ExcludeTypes +} + +func applyOverrideFieldsTo(o *config.AccountServiceOverride, f overrideFields) { + o.Enabled, o.Term, o.Payment, o.Coverage, o.RampSchedule = f.Enabled, f.Term, f.Payment, f.Coverage, f.RampSchedule + o.IncludeEngines, o.ExcludeEngines = f.IncludeEngines, f.ExcludeEngines + o.IncludeRegions, o.ExcludeRegions = f.IncludeRegions, f.ExcludeRegions + o.IncludeTypes, o.ExcludeTypes = f.IncludeTypes, f.ExcludeTypes +} + +// normalized maps empty lists to nil, so a stored empty array and an absent +// list compare equal. +func (f overrideFields) normalized() overrideFields { + for _, l := range []*[]string{&f.IncludeEngines, &f.ExcludeEngines, &f.IncludeRegions, &f.ExcludeRegions, &f.IncludeTypes, &f.ExcludeTypes} { + if len(*l) == 0 { + *l = nil + } + } + return f +} + +func (f overrideFields) equal(g overrideFields) bool { + return reflect.DeepEqual(f.normalized(), g.normalized()) +} diff --git a/internal/api/override_transfer_csv.go b/internal/api/override_transfer_csv.go new file mode 100644 index 000000000..220384717 --- /dev/null +++ b/internal/api/override_transfer_csv.go @@ -0,0 +1,221 @@ +package api + +import ( + "bytes" + "encoding/csv" + "errors" + "fmt" + "io" + "strconv" + "strings" + "time" +) + +// overrideCSVColumns is the CSV header, in order. List cells hold +// semicolon-separated values; an empty cell is an unset field. +var overrideCSVColumns = []string{ + "account_id", "account_name", "account_external_id", "provider", "service", + "enabled", "term", "payment", "coverage", "ramp_schedule", + "include_engines", "exclude_engines", "include_regions", "exclude_regions", + "include_types", "exclude_types", "created_at", "updated_at", +} + +const overrideCSVListSep = ";" + +// utf8BOM is stripped from the first header cell (spreadsheet exports add it). +const utf8BOM = "\uFEFF" + +// encodeOverridesCSV writes rows as CSV with a header line. +func encodeOverridesCSV(rows []OverrideTransferRow) (string, error) { + var buf bytes.Buffer + w := csv.NewWriter(&buf) + if err := w.Write(overrideCSVColumns); err != nil { + return "", err + } + for i := range rows { + if err := w.Write(overrideCSVRecord(&rows[i])); err != nil { + return "", err + } + } + w.Flush() + return buf.String(), w.Error() +} + +func overrideCSVRecord(r *OverrideTransferRow) []string { + return []string{ + r.AccountID, csvSafeText(r.AccountName), r.AccountExternalID, r.Provider, r.Service, + formatBoolPtr(r.Enabled), formatIntPtr(r.Term), derefString(r.Payment), formatFloatPtr(r.Coverage), derefString(r.RampSchedule), + strings.Join(r.IncludeEngines, overrideCSVListSep), strings.Join(r.ExcludeEngines, overrideCSVListSep), + strings.Join(r.IncludeRegions, overrideCSVListSep), strings.Join(r.ExcludeRegions, overrideCSVListSep), + strings.Join(r.IncludeTypes, overrideCSVListSep), strings.Join(r.ExcludeTypes, overrideCSVListSep), + formatTimePtr(r.CreatedAt), formatTimePtr(r.UpdatedAt), + } +} + +// csvSafeText stops a spreadsheet from running a free-text cell as a formula +// (CSV injection). Only the account name is free text; import ignores it, so +// the prefix never round-trips into stored data. +func csvSafeText(s string) string { + if s != "" && strings.ContainsRune("=+-@\t\r", rune(s[0])) { + return "'" + s + } + return s +} + +func formatBoolPtr(p *bool) string { + if p == nil { + return "" + } + return strconv.FormatBool(*p) +} + +func formatIntPtr(p *int) string { + if p == nil { + return "" + } + return strconv.Itoa(*p) +} + +func formatFloatPtr(p *float64) string { + if p == nil { + return "" + } + return strconv.FormatFloat(*p, 'f', -1, 64) +} + +func formatTimePtr(p *time.Time) string { + if p == nil { + return "" + } + return p.UTC().Format(time.RFC3339) +} + +// decodeOverridesCSV parses a CSV export back into rows. The header must name +// every column it uses; unknown columns are an error so a typo cannot silently +// drop a field. Column order is free. +func decodeOverridesCSV(text string) ([]OverrideTransferRow, error) { + r := csv.NewReader(strings.NewReader(text)) + header, err := r.Read() + if err != nil { + return nil, fmt.Errorf("csv header: %w", err) + } + index, err := overrideCSVHeaderIndex(header) + if err != nil { + return nil, err + } + var rows []OverrideTransferRow + for line := 2; ; line++ { + rec, err := r.Read() + if errors.Is(err, io.EOF) { + return rows, nil + } + if err != nil { + return nil, fmt.Errorf("csv line %d: %w", line, err) + } + row, err := overrideRowFromCSV(rec, index) + if err != nil { + return nil, fmt.Errorf("csv line %d: %w", line, err) + } + rows = append(rows, row) + } +} + +func overrideCSVHeaderIndex(header []string) (map[string]int, error) { + known := make(map[string]bool, len(overrideCSVColumns)) + for _, c := range overrideCSVColumns { + known[c] = true + } + index := make(map[string]int, len(header)) + for i, h := range header { + name := strings.TrimSpace(strings.TrimPrefix(h, utf8BOM)) + if !known[name] { + return nil, fmt.Errorf("csv header: unknown column %q", name) + } + if _, dup := index[name]; dup { + return nil, fmt.Errorf("csv header: duplicate column %q", name) + } + index[name] = i + } + return index, nil +} + +func overrideRowFromCSV(rec []string, index map[string]int) (OverrideTransferRow, error) { + cell := func(name string) string { + if i, ok := index[name]; ok && i < len(rec) { + return strings.TrimSpace(rec[i]) + } + return "" + } + row := OverrideTransferRow{ + AccountID: cell("account_id"), AccountExternalID: cell("account_external_id"), + Provider: cell("provider"), Service: cell("service"), + Payment: stringPtrOrNil(cell("payment")), RampSchedule: stringPtrOrNil(cell("ramp_schedule")), + IncludeEngines: splitCSVList(cell("include_engines")), ExcludeEngines: splitCSVList(cell("exclude_engines")), + IncludeRegions: splitCSVList(cell("include_regions")), ExcludeRegions: splitCSVList(cell("exclude_regions")), + IncludeTypes: splitCSVList(cell("include_types")), ExcludeTypes: splitCSVList(cell("exclude_types")), + } + var err error + if row.Enabled, err = parseBoolCell(cell("enabled")); err != nil { + return row, fmt.Errorf("enabled: %w", err) + } + if row.Term, err = parseIntCell(cell("term")); err != nil { + return row, fmt.Errorf("term: %w", err) + } + if row.Coverage, err = parseFloatCell(cell("coverage")); err != nil { + return row, fmt.Errorf("coverage: %w", err) + } + return row, nil +} + +func stringPtrOrNil(s string) *string { + if s == "" { + return nil + } + return &s +} + +func splitCSVList(s string) []string { + if s == "" { + return nil + } + var out []string + for _, v := range strings.Split(s, overrideCSVListSep) { + if v = strings.TrimSpace(v); v != "" { + out = append(out, v) + } + } + return out +} + +func parseBoolCell(s string) (*bool, error) { + if s == "" { + return nil, nil + } + b, err := strconv.ParseBool(s) + if err != nil { + return nil, fmt.Errorf("%q is not true or false", s) + } + return &b, nil +} + +func parseIntCell(s string) (*int, error) { + if s == "" { + return nil, nil + } + n, err := strconv.Atoi(s) + if err != nil { + return nil, fmt.Errorf("%q is not a whole number", s) + } + return &n, nil +} + +func parseFloatCell(s string) (*float64, error) { + if s == "" { + return nil, nil + } + f, err := strconv.ParseFloat(s, 64) + if err != nil { + return nil, fmt.Errorf("%q is not a number", s) + } + return &f, nil +} From 12c9eaacc34c3cc7e899568c0481f6d82fa94d78 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 11:03:56 +0000 Subject: [PATCH 02/10] feat(api): export account service overrides as JSON or CSV Overrides could only be read one account at a time, so there was no way to back them up, diff them or copy them to another environment. The export reads every override on the accounts the caller's scope allows (view:accounts), sorted so two exports diff cleanly. The account index resolves import rows by account ID, else by provider and external ID, and hides out-of-scope accounts. Refs #9 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_override_transfer.go | 93 +++++++++++++++++++++++ internal/api/override_account_index.go | 81 ++++++++++++++++++++ 2 files changed, 174 insertions(+) create mode 100644 internal/api/handler_override_transfer.go create mode 100644 internal/api/override_account_index.go diff --git a/internal/api/handler_override_transfer.go b/internal/api/handler_override_transfer.go new file mode 100644 index 000000000..9615c7223 --- /dev/null +++ b/internal/api/handler_override_transfer.go @@ -0,0 +1,93 @@ +package api + +import ( + "context" + "fmt" + "sort" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" +) + +// exportAccountServiceOverrides handles GET +// /api/accounts/service-overrides/export (issue #9). It returns every +// override on the accounts the caller's scope allows, as JSON +// (OverrideExport) or, with ?format=csv, as CSV. +func (h *Handler) exportAccountServiceOverrides(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + session, err := h.requirePermission(ctx, req, "view", "accounts") + if err != nil { + return nil, err + } + format := req.QueryStringParameters["format"] + if format != "" && format != "json" && format != "csv" { + return nil, NewClientError(400, "format must be json or csv") + } + accounts, err := h.scopedAccounts(ctx, session) + if err != nil { + return nil, err + } + rows, err := h.overrideTransferRows(ctx, accounts) + if err != nil { + return nil, err + } + if format != "csv" { + return OverrideExport{Version: overrideExportVersion, ExportedAt: time.Now().UTC(), Overrides: rows}, nil + } + body, err := encodeOverridesCSV(rows) + if err != nil { + return nil, fmt.Errorf("accounts: encode overrides csv: %w", err) + } + return &rawResponse{contentType: "text/csv; charset=utf-8", body: body}, nil +} + +// scopedAccounts lists the cloud accounts the session's account scope allows, +// ordered by provider, external ID and ID. +func (h *Handler) scopedAccounts(ctx context.Context, session *Session) ([]config.CloudAccount, error) { + accts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{}) + if err != nil { + return nil, fmt.Errorf("accounts: %w", err) + } + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + out := make([]config.CloudAccount, 0, len(accts)) + for i := range accts { + if scope.Allows(accts[i].ID, accts[i].Name) { + out = append(out, accts[i]) + } + } + sort.Slice(out, func(i, j int) bool { + a, b := out[i], out[j] + if a.Provider != b.Provider { + return a.Provider < b.Provider + } + if a.ExternalID != b.ExternalID { + return a.ExternalID < b.ExternalID + } + return a.ID < b.ID + }) + return out, nil +} + +// overrideTransferRows reads each account's overrides, sorted by service. +func (h *Handler) overrideTransferRows(ctx context.Context, accounts []config.CloudAccount) ([]OverrideTransferRow, error) { + rows := []OverrideTransferRow{} + for i := range accounts { + overrides, err := h.config.ListAccountServiceOverrides(ctx, accounts[i].ID) + if err != nil { + return nil, fmt.Errorf("accounts: %w", err) + } + sort.Slice(overrides, func(a, b int) bool { + if overrides[a].Provider != overrides[b].Provider { + return overrides[a].Provider < overrides[b].Provider + } + return overrides[a].Service < overrides[b].Service + }) + for j := range overrides { + rows = append(rows, overrideTransferRowFrom(&accounts[i], &overrides[j])) + } + } + return rows, nil +} diff --git a/internal/api/override_account_index.go b/internal/api/override_account_index.go new file mode 100644 index 000000000..b36d78480 --- /dev/null +++ b/internal/api/override_account_index.go @@ -0,0 +1,81 @@ +package api + +import ( + "errors" + "fmt" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// overrideRowRejection is a row-level refusal, reported rather than returned. +type overrideRowRejection struct{ msg string } + +func (e *overrideRowRejection) Error() string { return e.msg } + +func rejectRow(format string, args ...any) error { + return &overrideRowRejection{msg: fmt.Sprintf(format, args...)} +} + +func isRowRejection(err error) bool { + var rej *overrideRowRejection + return errors.As(err, &rej) +} + +// overrideAccountIndex looks up the in-scope accounts an import row can name. +// Out-of-scope accounts are absent, so a row naming one reads as unknown and +// does not reveal that the account exists. +type overrideAccountIndex struct { + byID map[string]*config.CloudAccount + byExternal map[string][]*config.CloudAccount +} + +func newOverrideAccountIndex(accounts []config.CloudAccount) *overrideAccountIndex { + idx := &overrideAccountIndex{ + byID: make(map[string]*config.CloudAccount, len(accounts)), + byExternal: make(map[string][]*config.CloudAccount, len(accounts)), + } + for i := range accounts { + a := &accounts[i] + idx.byID[a.ID] = a + key := a.Provider + ":" + a.ExternalID + idx.byExternal[key] = append(idx.byExternal[key], a) + } + return idx +} + +// resolve finds the row's account: account_id first, then provider plus +// account_external_id (an account_id from another tenant matches nothing +// here, so the portable key takes over). The account's provider must be the +// row's provider. +func (idx *overrideAccountIndex) resolve(row *OverrideTransferRow) (*config.CloudAccount, error) { + if !validAccountProviders[row.Provider] { + return nil, rejectRow("invalid provider %q: must be one of aws, azure, gcp", row.Provider) + } + if a, ok := idx.byID[row.AccountID]; ok { + return checkResolvedAccount(a, row) + } + if row.AccountExternalID == "" { + return nil, rejectRow("unknown account %q and no account_external_id to match on", row.AccountID) + } + matches := idx.byExternal[row.Provider+":"+row.AccountExternalID] + switch len(matches) { + case 0: + return nil, rejectRow("no %s account with external ID %q", row.Provider, row.AccountExternalID) + case 1: + return matches[0], nil + default: + return nil, rejectRow("%d %s accounts share external ID %q; name the account by account_id", len(matches), row.Provider, row.AccountExternalID) + } +} + +// checkResolvedAccount refuses an account_id whose account belongs to another +// provider, or whose external ID disagrees with the row's. +func checkResolvedAccount(a *config.CloudAccount, row *OverrideTransferRow) (*config.CloudAccount, error) { + if a.Provider != row.Provider { + return nil, rejectRow("account %s is a %s account, not %s", a.ID, a.Provider, row.Provider) + } + if row.AccountExternalID != "" && row.AccountExternalID != a.ExternalID { + return nil, rejectRow("account %s has external ID %q, not %q", a.ID, a.ExternalID, row.AccountExternalID) + } + return a, nil +} From ce5e5dc7a14968e26f742d28e9ed60b38a18e806 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 11:03:56 +0000 Subject: [PATCH 03/10] feat(api): import account service overrides with dry run and per-row report The import applies an export file as desired state: each row replaces its override (unset fields clear), and overrides not in the file stay. Rows are validated with the service-config rules and the PUT endpoint's commitment-option check. Strict mode (the default) writes nothing if any row is rejected; best_effort writes the valid rows. dry_run reports insert/update/unchanged/rejected without writing. Writes upsert, so a re-run after a failure is safe. Requires update:accounts. Refs #9 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_override_import.go | 266 ++++++++++++++++++++++++ internal/api/router.go | 16 ++ 2 files changed, 282 insertions(+) create mode 100644 internal/api/handler_override_import.go diff --git a/internal/api/handler_override_import.go b/internal/api/handler_override_import.go new file mode 100644 index 000000000..a80d97c6f --- /dev/null +++ b/internal/api/handler_override_import.go @@ -0,0 +1,266 @@ +package api + +import ( + "context" + "encoding/json" + "fmt" + "math" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/google/uuid" +) + +// maxOverrideImportRows bounds one import request. +const maxOverrideImportRows = 5000 + +// Import modes: strict applies nothing when any row is rejected; best_effort +// applies the valid rows and reports the rest. +const ( + overrideImportStrict = "strict" + overrideImportBestEffort = "best_effort" +) + +// Per-row import actions. +const ( + overrideActionInsert = "insert" + overrideActionUpdate = "update" + overrideActionUnchanged = "unchanged" + overrideActionRejected = "rejected" +) + +// OverrideImportRequest is the POST /api/accounts/service-overrides/import +// body. Exactly one of Overrides (the JSON export's rows) or CSV (the CSV +// export's text) is set. +type OverrideImportRequest struct { + Mode string `json:"mode,omitempty"` + DryRun bool `json:"dry_run,omitempty"` + Overrides []OverrideTransferRow `json:"overrides,omitempty"` + CSV string `json:"csv,omitempty"` +} + +// OverrideImportRowResult reports what import did, or would do, with one row. +// Row is 1-based in file order. +type OverrideImportRowResult struct { + Row int `json:"row"` + AccountID string `json:"account_id,omitempty"` + Provider string `json:"provider"` + Service string `json:"service"` + Action string `json:"action"` + Error string `json:"error,omitempty"` +} + +// OverrideImportReport is the import response. Applied is true when the +// insert and update rows were written (never on a dry run, or in strict mode +// with a rejected row). +type OverrideImportReport struct { + Mode string `json:"mode"` + DryRun bool `json:"dry_run"` + Applied bool `json:"applied"` + Summary map[string]int `json:"summary"` + Rows []OverrideImportRowResult `json:"rows"` +} + +// plannedOverride is a validated row with the stored override it replaces. +type plannedOverride struct { + account *config.CloudAccount + row *OverrideTransferRow + existing *config.AccountServiceOverride +} + +// importAccountServiceOverrides handles POST +// /api/accounts/service-overrides/import (issue #9). Each row is resolved to +// an in-scope account (by account_id, else by provider and +// account_external_id, so a file from another tenant imports), validated with +// the same rules as a service config, and compared with the stored override. +// A row sets every field of its override; overrides not in the file are left +// alone. A strict import with a rejected row answers 400 with the report and +// writes nothing. Re-running an import is safe: writes are upserts. +func (h *Handler) importAccountServiceOverrides(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + session, err := h.requirePermission(ctx, req, "update", "accounts") + if err != nil { + return nil, err + } + in, rows, err := parseOverrideImportRequest(req.Body) + if err != nil { + return nil, err + } + accounts, err := h.scopedAccounts(ctx, session) + if err != nil { + return nil, err + } + report, plan, err := h.planOverrideImport(ctx, newOverrideAccountIndex(accounts), rows) + if err != nil { + return nil, err + } + report.Mode, report.DryRun = in.Mode, in.DryRun + if report.Summary[overrideActionRejected] > 0 && in.Mode == overrideImportStrict { + return nil, NewClientErrorWithDetails(400, "import rejected: fix the rejected rows or use best_effort mode", map[string]any{"report": report}) + } + if in.DryRun { + return report, nil + } + if err := h.applyOverrideImport(ctx, plan); err != nil { + return nil, err + } + report.Applied = true + return report, nil +} + +func parseOverrideImportRequest(body string) (OverrideImportRequest, []OverrideTransferRow, error) { + var in OverrideImportRequest + if err := json.Unmarshal([]byte(body), &in); err != nil { + return in, nil, NewClientError(400, "invalid request body") + } + if in.Mode == "" { + in.Mode = overrideImportStrict + } + if in.Mode != overrideImportStrict && in.Mode != overrideImportBestEffort { + return in, nil, NewClientError(400, "mode must be strict or best_effort") + } + if (in.CSV == "") == (len(in.Overrides) == 0) { + return in, nil, NewClientError(400, "send exactly one of overrides or csv, with at least one row") + } + rows, err := overrideImportRows(&in) + return in, rows, err +} + +// overrideImportRows returns the request's rows (decoding the CSV form) and +// enforces the 1..maxOverrideImportRows bound. +func overrideImportRows(in *OverrideImportRequest) ([]OverrideTransferRow, error) { + rows := in.Overrides + if in.CSV != "" { + parsed, err := decodeOverridesCSV(in.CSV) + if err != nil { + return nil, NewClientError(400, err.Error()) + } + rows = parsed + } + if len(rows) == 0 || len(rows) > maxOverrideImportRows { + return nil, NewClientError(400, fmt.Sprintf("an import needs 1 to %d rows, got %d", maxOverrideImportRows, len(rows))) + } + return rows, nil +} + +// planOverrideImport resolves and validates every row and decides its action. +// Only store read failures are returned as errors; a bad row is a rejection. +func (h *Handler) planOverrideImport(ctx context.Context, idx *overrideAccountIndex, rows []OverrideTransferRow) (*OverrideImportReport, []plannedOverride, error) { + report := &OverrideImportReport{Summary: map[string]int{ + overrideActionInsert: 0, overrideActionUpdate: 0, overrideActionUnchanged: 0, overrideActionRejected: 0, + }} + existingByAccount := map[string][]config.AccountServiceOverride{} + seen := map[string]int{} + var plan []plannedOverride + for i := range rows { + res := OverrideImportRowResult{Row: i + 1, Provider: rows[i].Provider, Service: rows[i].Service} + p, rowErr := h.planOverrideRow(ctx, idx, &rows[i], seen, res.Row) + if rowErr == nil { + p.existing, rowErr = h.findStoredOverride(ctx, existingByAccount, p.account.ID, &rows[i]) + } + if rowErr != nil && !isRowRejection(rowErr) { + return nil, nil, rowErr + } + res.Action, res.Error = overrideRowAction(p, rowErr) + if p.account != nil { + res.AccountID = p.account.ID + } + report.Summary[res.Action]++ + report.Rows = append(report.Rows, res) + if res.Action == overrideActionInsert || res.Action == overrideActionUpdate { + plan = append(plan, p) + } + } + return report, plan, nil +} + +func overrideRowAction(p plannedOverride, err error) (action, msg string) { + switch { + case err != nil: + return overrideActionRejected, err.Error() + case p.existing == nil: + return overrideActionInsert, "" + case overrideFieldsOf(p.existing).equal(p.row.fields()): + return overrideActionUnchanged, "" + default: + return overrideActionUpdate, "" + } +} + +// planOverrideRow resolves the row's account and validates it. seen maps each +// account/provider/service already in the file to its row number. +func (h *Handler) planOverrideRow(ctx context.Context, idx *overrideAccountIndex, row *OverrideTransferRow, seen map[string]int, rowNum int) (plannedOverride, error) { + acct, err := idx.resolve(row) + if err != nil { + return plannedOverride{}, err + } + p := plannedOverride{account: acct, row: row} + key := acct.ID + "/" + row.Provider + "/" + row.Service + if first, dup := seen[key]; dup { + return p, rejectRow("duplicate of row %d", first) + } + seen[key] = rowNum + return p, h.validateOverrideRow(ctx, row) +} + +// validateOverrideRow applies the service-config rules (provider, service +// name, term, provider-specific payment token, coverage) and the +// commitment-option combination check the PUT endpoint runs. +func (h *Handler) validateOverrideRow(ctx context.Context, row *OverrideTransferRow) error { + if !serviceNameRegex.MatchString(row.Service) { + return rejectRow("invalid service name %q", row.Service) + } + cfg := config.ServiceConfig{Provider: row.Provider, Service: row.Service, Term: derefInt(row.Term), Payment: derefString(row.Payment)} + if row.Coverage != nil { + if math.IsNaN(*row.Coverage) || math.IsInf(*row.Coverage, 0) { + return rejectRow("coverage must be a number between %d and %d", config.MinCoverage, config.MaxCoverage) + } + cfg.Coverage = *row.Coverage + } + if err := cfg.Validate(); err != nil { + return rejectRow("%s", err.Error()) + } + if err := h.checkCommitmentOptionCombo(ctx, cfg); err != nil { + if ce, ok := IsClientError(err); ok && ce.code == 400 { + return rejectRow("%s", ce.Error()) + } + return err + } + return nil +} + +// findStoredOverride returns the stored override the row replaces, or nil. +// Each account's overrides are read once per import. +func (h *Handler) findStoredOverride(ctx context.Context, cache map[string][]config.AccountServiceOverride, accountID string, row *OverrideTransferRow) (*config.AccountServiceOverride, error) { + stored, ok := cache[accountID] + if !ok { + var err error + if stored, err = h.config.ListAccountServiceOverrides(ctx, accountID); err != nil { + return nil, fmt.Errorf("accounts: %w", err) + } + cache[accountID] = stored + } + for i := range stored { + if stored[i].Provider == row.Provider && stored[i].Service == row.Service { + return &stored[i], nil + } + } + return nil, nil +} + +// applyOverrideImport writes the insert and update rows. A store failure +// stops the import part way; re-running it is safe because writes upsert. +func (h *Handler) applyOverrideImport(ctx context.Context, plan []plannedOverride) error { + now := time.Now() + for i, p := range plan { + o := &config.AccountServiceOverride{ID: uuid.New().String(), AccountID: p.account.ID, Provider: p.row.Provider, Service: p.row.Service, CreatedAt: now, UpdatedAt: now} + if p.existing != nil { + o.ID, o.CreatedAt = p.existing.ID, p.existing.CreatedAt + } + applyOverrideFieldsTo(o, p.row.fields()) + if err := h.config.SaveAccountServiceOverride(ctx, o); err != nil { + return fmt.Errorf("accounts: import stopped after %d of %d writes: %w", i, len(plan), err) + } + } + return nil +} diff --git a/internal/api/router.go b/internal/api/router.go index 48fe7a20c..f5c2782b0 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -273,6 +273,12 @@ func (r *Router) registerRoutes() { // object stays behind view:accounts on GET /api/accounts below. Must // precede the generic "/api/accounts/" GET prefix route. {ExactPath: "/api/accounts/list", Method: "GET", Handler: r.listAccountsMinimalHandler, Auth: AuthUser}, + // Override export/import (issue #9). Exact paths, so they must precede + // the generic "/api/accounts/" prefix routes. Gated in-handler on + // view:accounts (export) and update:accounts (import), scoped to the + // caller's allowed accounts. + {ExactPath: "/api/accounts/service-overrides/export", Method: "GET", Handler: r.exportAccountServiceOverridesHandler, Auth: AuthUser}, + {ExactPath: "/api/accounts/service-overrides/import", Method: "POST", Handler: r.importAccountServiceOverridesHandler, Auth: AuthAdmin}, {ExactPath: "/api/accounts", Method: "GET", Handler: r.listAccountsHandler, Auth: AuthUser}, {ExactPath: "/api/accounts", Method: "POST", Handler: r.createAccountHandler, Auth: AuthAdmin}, {PathPrefix: "/api/accounts/", PathSuffix: "/credentials", Method: "POST", Handler: r.saveAccountCredentialsHandler, Auth: AuthAdmin}, @@ -941,6 +947,16 @@ func (r *Router) getAccountHandler(ctx context.Context, req *events.LambdaFuncti return r.h.getAccount(ctx, req, params["id"]) } +// exportAccountServiceOverridesHandler handles GET /api/accounts/service-overrides/export. +func (r *Router) exportAccountServiceOverridesHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return r.h.exportAccountServiceOverrides(ctx, req) +} + +// importAccountServiceOverridesHandler handles POST /api/accounts/service-overrides/import. +func (r *Router) importAccountServiceOverridesHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return r.h.importAccountServiceOverrides(ctx, req) +} + // updateAccountOrServiceOverrideHandler handles PUT /api/accounts/:id and // PUT /api/accounts/:id/service-overrides/:provider/:service. // The router puts everything after /api/accounts/ into params["id"]. From e18246be319295a2df92242ef66647ad580d0143 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 11:03:57 +0000 Subject: [PATCH 04/10] docs(api): document override export/import and test the export Adds the two endpoints and their schemas to openapi.yaml (the route coverage test requires every route to be documented), plus tests for scoped, ordered JSON export, the CSV round trip with formula neutralization, the format check, and route precedence over the generic /api/accounts/ prefix routes. Refs #9 Co-Authored-By: Claude Opus 5.5 --- .../api/handler_override_transfer_test.go | 162 ++++++++++++++ internal/api/openapi.yaml | 200 ++++++++++++++++++ 2 files changed, 362 insertions(+) create mode 100644 internal/api/handler_override_transfer_test.go diff --git a/internal/api/handler_override_transfer_test.go b/internal/api/handler_override_transfer_test.go new file mode 100644 index 000000000..6b8d83545 --- /dev/null +++ b/internal/api/handler_override_transfer_test.go @@ -0,0 +1,162 @@ +package api + +import ( + "context" + "encoding/json" + "strings" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// overrideTransferStore is an in-memory account and override store for the +// export/import tests (issue #9). +type overrideTransferStore struct { + *MockConfigStore + accounts []config.CloudAccount + overrides map[string][]config.AccountServiceOverride + saved []config.AccountServiceOverride +} + +func (s *overrideTransferStore) ListCloudAccounts(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { + return append([]config.CloudAccount(nil), s.accounts...), nil +} + +func (s *overrideTransferStore) ListAccountServiceOverrides(_ context.Context, accountID string) ([]config.AccountServiceOverride, error) { + return append([]config.AccountServiceOverride(nil), s.overrides[accountID]...), nil +} + +func (s *overrideTransferStore) SaveAccountServiceOverride(_ context.Context, o *config.AccountServiceOverride) error { + s.saved = append(s.saved, *o) + list := s.overrides[o.AccountID] + for i := range list { + if list[i].Provider == o.Provider && list[i].Service == o.Service { + list[i] = *o + return nil + } + } + s.overrides[o.AccountID] = append(list, *o) + return nil +} + +const ( + xferAWS = "11111111-1111-1111-1111-111111111111" + xferAzure = "22222222-2222-2222-2222-222222222222" + xferOther = "33333333-3333-3333-3333-333333333333" +) + +func ptr[T any](v T) *T { return &v } + +func newOverrideTransferStore() *overrideTransferStore { + created := time.Date(2026, 9, 1, 0, 0, 0, 0, time.UTC) + return &overrideTransferStore{ + MockConfigStore: new(MockConfigStore), + accounts: []config.CloudAccount{ + {ID: xferAzure, Name: "Azure prod", Provider: "azure", ExternalID: "sub-1"}, + {ID: xferAWS, Name: "=HYPERLINK(\"x\")", Provider: "aws", ExternalID: "123456789012"}, + {ID: xferOther, Name: "Other team", Provider: "aws", ExternalID: "210987654321"}, + }, + overrides: map[string][]config.AccountServiceOverride{ + xferAWS: { + {ID: "o-2", AccountID: xferAWS, Provider: "aws", Service: "rds", Coverage: ptr(50.0), IncludeRegions: []string{"us-east-1", "eu-west-1"}, CreatedAt: created, UpdatedAt: created}, + {ID: "o-1", AccountID: xferAWS, Provider: "aws", Service: "ec2", Enabled: ptr(true), Term: ptr(3), Payment: ptr("all-upfront"), CreatedAt: created, UpdatedAt: created}, + }, + xferAzure: {{ID: "o-3", AccountID: xferAzure, Provider: "azure", Service: "vm", Enabled: ptr(false), CreatedAt: created, UpdatedAt: created}}, + xferOther: {{ID: "o-4", AccountID: xferOther, Provider: "aws", Service: "ec2", Term: ptr(1), CreatedAt: created, UpdatedAt: created}}, + }, + } +} + +// transferHandler returns a handler whose session may see only scope (all +// accounts when scope is nil). +func transferHandler(t *testing.T, store *overrideTransferStore, scope []string) *Handler { + t.Helper() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", context.Background(), "admin-token").Return(adminAccountSession(), nil) + mockAuth.grantPermissionsScoped([]auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, scope) + return &Handler{auth: mockAuth, config: store} +} + +func exportRequest(format string) *events.LambdaFunctionURLRequest { + req := adminRequest("") + if format != "" { + req.QueryStringParameters = map[string]string{"format": format} + } + return req +} + +func TestExportOverrides_JSONListsScopedAccountsInOrder(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, []string{xferAWS, xferAzure}) + + got, err := h.exportAccountServiceOverrides(context.Background(), exportRequest("")) + require.NoError(t, err) + doc := got.(OverrideExport) + assert.Equal(t, overrideExportVersion, doc.Version) + + keys := make([]string, 0, len(doc.Overrides)) + for _, r := range doc.Overrides { + keys = append(keys, r.Provider+":"+r.AccountExternalID+"/"+r.Service) + } + // Sorted by provider, external ID, then service; the out-of-scope + // account's override is absent. + assert.Equal(t, []string{"aws:123456789012/ec2", "aws:123456789012/rds", "azure:sub-1/vm"}, keys) + assert.Equal(t, xferAWS, doc.Overrides[0].AccountID) + assert.Equal(t, 3, *doc.Overrides[0].Term) + assert.Equal(t, []string{"us-east-1", "eu-west-1"}, doc.Overrides[1].IncludeRegions) +} + +func TestExportOverrides_CSVRoundTripsAndNeutralizesFormulas(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, nil) + + got, err := h.exportAccountServiceOverrides(context.Background(), exportRequest("csv")) + require.NoError(t, err) + raw := got.(*rawResponse) + assert.Equal(t, "text/csv; charset=utf-8", raw.contentType) + assert.True(t, strings.HasPrefix(raw.body, strings.Join(overrideCSVColumns, ",")+"\n")) + assert.Contains(t, raw.body, `"'=HYPERLINK(""x"")"`, "a formula-like account name must be prefixed") + + rows, err := decodeOverridesCSV(raw.body) + require.NoError(t, err) + require.Len(t, rows, 4) + assert.Equal(t, "us-east-1;eu-west-1", strings.Join(rows[1].IncludeRegions, ";")) + assert.Equal(t, 50.0, *rows[1].Coverage) + assert.Nil(t, rows[1].Term, "an empty cell is an unset field") +} + +func TestExportOverrides_RejectsUnknownFormat(t *testing.T) { + h := transferHandler(t, newOverrideTransferStore(), nil) + _, err := h.exportAccountServiceOverrides(context.Background(), exportRequest("xml")) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) +} + +func TestOverrideTransferRoutes_PrecedeAccountPrefixRoutes(t *testing.T) { + store := newOverrideTransferStore() + r := NewRouter(transferHandler(t, store, nil)) + + got, err := r.Route(context.Background(), "GET", "/api/accounts/service-overrides/export", exportRequest("")) + require.NoError(t, err) + assert.IsType(t, OverrideExport{}, got) + + body := `{"dry_run":true,"overrides":[{"account_id":"` + xferAWS + `","provider":"aws","service":"ec2"}]}` + got, err = r.Route(context.Background(), "POST", "/api/accounts/service-overrides/import", adminRequest(body)) + require.NoError(t, err) + assert.IsType(t, &OverrideImportReport{}, got) +} + +// importBody marshals an import request. +func importBody(t *testing.T, in OverrideImportRequest) string { + t.Helper() + b, err := json.Marshal(in) + require.NoError(t, err) + return string(b) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 51a497744..1bfd2f1c8 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3966,6 +3966,78 @@ paths: '403': $ref: '#/components/responses/Forbidden' + /api/accounts/service-overrides/export: + get: + operationId: exportAccountServiceOverrides + tags: [Accounts] + summary: Export every per-service override in the caller's account scope + description: > + Returns every account_service_overrides row on the accounts the + caller's scope allows, with each account's provider and external ID + so the file can be imported into another tenant (issue #9). Requires + view:accounts. JSON by default; format=csv returns the same rows as + CSV (list cells are semicolon-separated, an empty cell is an unset + field). + parameters: + - name: format + in: query + description: json (default) or csv. Any other value is a 400. + schema: + type: string + enum: [json, csv] + responses: + '200': + description: The overrides + content: + application/json: + schema: + $ref: '#/components/schemas/OverrideExport' + text/csv: + schema: + type: string + '400': + $ref: '#/components/responses/BadRequest' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + $ref: '#/components/responses/Forbidden' + + /api/accounts/service-overrides/import: + post: + operationId: importAccountServiceOverrides + tags: [Accounts] + summary: Import per-service overrides from an export file + description: > + Applies an export file (JSON rows or CSV text) as desired state + (issue #9). Each row names its account by account_id, else by + provider and account_external_id, and must be in the caller's account + scope. A row sets every field of its override (an unset field clears + it); overrides not in the file are left alone. Rows are validated with + the service-config rules and the commitment-option check of the PUT + endpoint. strict (default) writes nothing when any row is rejected + and answers 400 with the report under details.report; best_effort + writes the valid rows. dry_run reports without writing. Requires + update:accounts. Writes are upserts, so re-running an import is safe. + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/OverrideImportRequest' + responses: + '200': + description: Per-row report + content: + application/json: + schema: + $ref: '#/components/schemas/OverrideImportReport' + '400': + $ref: '#/components/responses/BadRequest' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + $ref: '#/components/responses/Forbidden' + /api/accounts: get: operationId: listAccounts @@ -7590,6 +7662,134 @@ components: message: type: string + OverrideTransferRow: + type: object + description: > + One override in an export or import file. account_name, created_at + and updated_at are informational and ignored on import. + required: [provider, service] + properties: + account_id: + type: string + format: uuid + account_name: + type: string + account_external_id: + type: string + provider: + type: string + enum: [aws, azure, gcp] + service: + type: string + enabled: + type: boolean + term: + type: integer + payment: + type: string + coverage: + type: number + format: double + ramp_schedule: + type: string + include_engines: + type: array + items: + type: string + exclude_engines: + type: array + items: + type: string + include_regions: + type: array + items: + type: string + exclude_regions: + type: array + items: + type: string + include_types: + type: array + items: + type: string + exclude_types: + type: array + items: + type: string + created_at: + type: string + format: date-time + updated_at: + type: string + format: date-time + + OverrideExport: + type: object + required: [version, exported_at, overrides] + properties: + version: + type: integer + exported_at: + type: string + format: date-time + overrides: + type: array + items: + $ref: '#/components/schemas/OverrideTransferRow' + + OverrideImportRequest: + type: object + description: Send exactly one of overrides or csv (1 to 5000 rows). + properties: + mode: + type: string + enum: [strict, best_effort] + dry_run: + type: boolean + overrides: + type: array + items: + $ref: '#/components/schemas/OverrideTransferRow' + csv: + type: string + + OverrideImportReport: + type: object + required: [mode, dry_run, applied, summary, rows] + properties: + mode: + type: string + enum: [strict, best_effort] + dry_run: + type: boolean + applied: + type: boolean + summary: + type: object + description: Row count per action (insert, update, unchanged, rejected). + additionalProperties: + type: integer + rows: + type: array + items: + type: object + required: [row, provider, service, action] + properties: + row: + type: integer + account_id: + type: string + format: uuid + provider: + type: string + service: + type: string + action: + type: string + enum: [insert, update, unchanged, rejected] + error: + type: string + AccountServiceOverride: type: object description: > From 577d59d9339bb1b93ee66c4b72268226e900aa1e Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 11:03:57 +0000 Subject: [PATCH 05/10] test(api): cover override import modes, matching and validation Covers export then re-import as unchanged (JSON and CSV), matching another tenant's rows by external ID, desired-state updates, strict refusal with the report and no writes, best-effort partial apply with a reason per rejected row, dry run, malformed requests, NaN coverage from CSV, and the update:accounts gate. Refs #9 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_override_import_test.go | 199 +++++++++++++++++++ 1 file changed, 199 insertions(+) create mode 100644 internal/api/handler_override_import_test.go diff --git a/internal/api/handler_override_import_test.go b/internal/api/handler_override_import_test.go new file mode 100644 index 000000000..335dfca23 --- /dev/null +++ b/internal/api/handler_override_import_test.go @@ -0,0 +1,199 @@ +package api + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func runImport(t *testing.T, h *Handler, in OverrideImportRequest) (*OverrideImportReport, error) { + t.Helper() + got, err := h.importAccountServiceOverrides(context.Background(), adminRequest(importBody(t, in))) + if err != nil { + return nil, err + } + return got.(*OverrideImportReport), nil +} + +func actions(r *OverrideImportReport) []string { + out := make([]string, 0, len(r.Rows)) + for _, row := range r.Rows { + out = append(out, row.Action) + } + return out +} + +func TestImportOverrides_ExportReimportIsUnchanged(t *testing.T) { + for _, format := range []string{"json", "csv"} { + t.Run(format, func(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, nil) + exported, err := h.exportAccountServiceOverrides(context.Background(), exportRequest(format)) + require.NoError(t, err) + + in := OverrideImportRequest{} + if format == "csv" { + in.CSV = exported.(*rawResponse).body + } else { + in.Overrides = exported.(OverrideExport).Overrides + } + report, err := runImport(t, h, in) + require.NoError(t, err) + assert.Equal(t, 4, report.Summary[overrideActionUnchanged], "%+v", report.Rows) + assert.True(t, report.Applied) + assert.Empty(t, store.saved, "an unchanged import writes nothing") + }) + } +} + +func TestImportOverrides_MatchesAnotherTenantsRowsByExternalID(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, nil) + rows := []OverrideTransferRow{{ + AccountID: "99999999-9999-9999-9999-999999999999", AccountExternalID: "123456789012", + Provider: "aws", Service: "elasticache", Term: ptr(1), Payment: ptr("no-upfront"), + }} + + report, err := runImport(t, h, OverrideImportRequest{Overrides: rows}) + require.NoError(t, err) + assert.Equal(t, []string{overrideActionInsert}, actions(report)) + assert.Equal(t, xferAWS, report.Rows[0].AccountID) + require.Len(t, store.saved, 1) + assert.Equal(t, xferAWS, store.saved[0].AccountID) + assert.Equal(t, 1, *store.saved[0].Term) +} + +func TestImportOverrides_RowIsDesiredState(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, nil) + // The stored ec2 override has enabled, term and payment; the row keeps + // only enabled, so term and payment are cleared. + rows := []OverrideTransferRow{{AccountID: xferAWS, Provider: "aws", Service: "ec2", Enabled: ptr(true)}} + + report, err := runImport(t, h, OverrideImportRequest{Overrides: rows}) + require.NoError(t, err) + assert.Equal(t, []string{overrideActionUpdate}, actions(report)) + require.Len(t, store.saved, 1) + assert.Equal(t, "o-1", store.saved[0].ID, "an update keeps the stored row's ID") + assert.Nil(t, store.saved[0].Term) + assert.Nil(t, store.saved[0].Payment) +} + +func badImportRows() []OverrideTransferRow { + return []OverrideTransferRow{ + {AccountID: xferAWS, Provider: "aws", Service: "ec2", Term: ptr(1)}, // 1 update + {AccountID: xferAWS, Provider: "aws", Service: "ec2", Term: ptr(3)}, // 2 duplicate + {AccountID: xferAzure, Provider: "azure", Service: "vm", Payment: ptr("all-upfront")}, // 3 AWS token on Azure + {AccountID: xferAWS, Provider: "azure", Service: "vm"}, // 4 provider mismatch + {AccountID: xferOther, Provider: "aws", Service: "ec2"}, // 5 out of scope + {AccountID: xferAWS, Provider: "aws", Service: "redshift", Coverage: ptr(150.0)}, // 6 coverage range + {AccountID: xferAWS, AccountExternalID: "000000000000", Provider: "aws", Service: "opensearch"}, // 7 external ID mismatch + {AccountID: xferAWS, Provider: "aws", Service: "Bad Name"}, // 8 service name + {AccountID: xferAzure, Provider: "azure", Service: "sql", Term: ptr(2)}, // 9 term + {AccountID: "", AccountExternalID: "sub-1", Provider: "azure", Service: "cosmos", Term: ptr(1)}, // 10 insert by external ID + } +} + +func TestImportOverrides_StrictRejectsAllWithReport(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, []string{xferAWS, xferAzure}) + + _, err := runImport(t, h, OverrideImportRequest{Overrides: badImportRows()}) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 400, ce.code) + report := ce.details["report"].(*OverrideImportReport) + assert.Equal(t, 8, report.Summary[overrideActionRejected]) + assert.False(t, report.Applied) + assert.Empty(t, store.saved, "strict mode writes nothing when a row is rejected") +} + +func TestImportOverrides_BestEffortAppliesValidRows(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, []string{xferAWS, xferAzure}) + + report, err := runImport(t, h, OverrideImportRequest{Mode: overrideImportBestEffort, Overrides: badImportRows()}) + require.NoError(t, err) + assert.Equal(t, []string{ + overrideActionUpdate, overrideActionRejected, overrideActionRejected, overrideActionRejected, overrideActionRejected, + overrideActionRejected, overrideActionRejected, overrideActionRejected, overrideActionRejected, overrideActionInsert, + }, actions(report)) + errs := map[int]string{} + for _, r := range report.Rows { + errs[r.Row] = r.Error + } + assert.Contains(t, errs[2], "duplicate of row 1") + assert.Contains(t, errs[3], "invalid payment option") + assert.Contains(t, errs[4], "is a aws account, not azure") + assert.Contains(t, errs[5], "unknown account", "an out-of-scope account reads as unknown") + assert.Contains(t, errs[6], "coverage must be between") + assert.Contains(t, errs[7], "has external ID") + assert.Contains(t, errs[8], "invalid service name") + assert.Contains(t, errs[9], "term must be 1 or 3") + assert.True(t, report.Applied) + require.Len(t, store.saved, 2) + assert.Equal(t, xferAzure, store.saved[1].AccountID) +} + +func TestImportOverrides_DryRunWritesNothing(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, nil) + rows := []OverrideTransferRow{ + {AccountID: xferAWS, Provider: "aws", Service: "ec2", Term: ptr(1)}, + {AccountID: xferAWS, Provider: "aws", Service: "memorydb"}, + } + + report, err := runImport(t, h, OverrideImportRequest{DryRun: true, Overrides: rows}) + require.NoError(t, err) + assert.Equal(t, []string{overrideActionUpdate, overrideActionInsert}, actions(report)) + assert.True(t, report.DryRun) + assert.False(t, report.Applied) + assert.Empty(t, store.saved) +} + +func TestImportOverrides_RejectsMalformedRequests(t *testing.T) { + cases := map[string]string{ + "bad json": `{`, + "bad mode": `{"mode":"yolo","csv":"provider,service\naws,ec2"}`, + "no rows": `{}`, + "both forms": `{"csv":"provider,service\naws,ec2","overrides":[{"provider":"aws","service":"ec2"}]}`, + "unknown column": `{"csv":"provider,service,shade\naws,ec2,red"}`, + "bad bool": `{"csv":"provider,service,enabled\naws,ec2,maybe"}`, + "header only": `{"csv":"provider,service\n"}`, + } + for name, body := range cases { + t.Run(name, func(t *testing.T) { + h := transferHandler(t, newOverrideTransferStore(), nil) + _, err := h.importAccountServiceOverrides(context.Background(), adminRequest(body)) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 400, ce.code) + }) + } +} + +func TestImportOverrides_CSVRejectsNaNCoverage(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, nil) + csv := "account_id,provider,service,coverage\n" + xferAWS + ",aws,ec2,NaN\n" + + report, err := runImport(t, h, OverrideImportRequest{Mode: overrideImportBestEffort, CSV: csv}) + require.NoError(t, err) + assert.Equal(t, []string{overrideActionRejected}, actions(report)) + assert.Contains(t, report.Rows[0].Error, "coverage must be a number") + assert.Empty(t, store.saved) +} + +func TestImportOverrides_RequiresUpdateAccounts(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", context.Background(), "admin-token").Return(adminAccountSession(), nil) + mockAuth.grantPermissions(nil) + h := &Handler{auth: mockAuth, config: newOverrideTransferStore()} + + _, err := h.importAccountServiceOverrides(context.Background(), adminRequest(`{"csv":"provider,service\naws,ec2"}`)) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 403, ce.code) +} From 42f30940cd1d56cae94beecbc2035fb6458e35ec Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 11:03:57 +0000 Subject: [PATCH 06/10] feat(frontend): add a service override backup panel to Admin Accounts Gives admins buttons for the new export (JSON or CSV) and an import flow: pick a JSON or CSV export, choose all-or-nothing or skip rejected rows, preview with a dry run, then import after a confirmation. Each row's outcome shows as a badge with the reason for any rejection. Refs #9 Co-Authored-By: Claude Opus 5.5 --- frontend/src/api/accounts.ts | 71 ++++++- frontend/src/api/index.ts | 9 + frontend/src/index.html | 24 +++ frontend/src/modules/override-transfer.ts | 220 ++++++++++++++++++++++ frontend/src/settings.ts | 1 + frontend/src/styles/settings.css | 11 ++ 6 files changed, 335 insertions(+), 1 deletion(-) create mode 100644 frontend/src/modules/override-transfer.ts diff --git a/frontend/src/api/accounts.ts b/frontend/src/api/accounts.ts index 57de0dc84..56933349b 100644 --- a/frontend/src/api/accounts.ts +++ b/frontend/src/api/accounts.ts @@ -2,7 +2,8 @@ * Cloud Accounts API functions */ -import { apiRequest } from './client'; +import { apiRequest, getApiBase, getAuthHeaders } from './client'; +import { httpStatusMessage, makeApiError } from './errors'; import type { Provider } from './types'; export interface CloudAccount { @@ -212,6 +213,74 @@ export async function deleteAccountServiceOverride( ); } +/** One override in an export/import file (GET/POST /api/accounts/service-overrides/*, issue #9). */ +export interface OverrideTransferRow extends AccountServiceOverrideRequest { + account_id?: string; + account_name?: string; + account_external_id?: string; + provider: string; + service: string; + created_at?: string; + updated_at?: string; +} + +export interface OverrideExport { + version: number; + exported_at: string; + overrides: OverrideTransferRow[]; +} + +export type OverrideImportMode = 'strict' | 'best_effort'; + +export interface OverrideImportRequest { + mode: OverrideImportMode; + dry_run: boolean; + overrides?: OverrideTransferRow[]; + csv?: string; +} + +export interface OverrideImportRowResult { + row: number; + account_id?: string; + provider: string; + service: string; + action: 'insert' | 'update' | 'unchanged' | 'rejected'; + error?: string; +} + +export interface OverrideImportReport { + mode: OverrideImportMode; + dry_run: boolean; + applied: boolean; + summary: Record; + rows: OverrideImportRowResult[]; +} + +export async function exportAccountServiceOverrides(): Promise { + return apiRequest('/accounts/service-overrides/export'); +} + +/** The same export as CSV text. apiRequest parses JSON only, so this fetches directly. */ +export async function exportAccountServiceOverridesCSV(): Promise { + const endpoint = '/accounts/service-overrides/export?format=csv'; + const response = await fetch(`${getApiBase()}${endpoint}`, { headers: getAuthHeaders() }); + if (!response.ok) { + throw makeApiError(httpStatusMessage(response.status), response.status, `HTTP ${response.status} from ${endpoint}`); + } + return response.text(); +} + +/** + * Import overrides. A strict import with a rejected row answers 400 with the + * report under error.details.report. + */ +export async function importAccountServiceOverrides(req: OverrideImportRequest): Promise { + return apiRequest('/accounts/service-overrides/import', { + method: 'POST', + body: JSON.stringify(req), + }); +} + export interface OrgDiscoveryResult { job_id?: string; status?: string; diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index 04a5b124d..dfbf7cd15 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -253,6 +253,12 @@ export type { AccountTestResult, AccountServiceOverride, AccountServiceOverrideRequest, + OverrideTransferRow, + OverrideExport, + OverrideImportMode, + OverrideImportRequest, + OverrideImportRowResult, + OverrideImportReport, OrgDiscoveryResult, FederationIaCResponse } from './accounts'; @@ -270,6 +276,9 @@ export { listAccountServiceOverrides, saveAccountServiceOverride, deleteAccountServiceOverride, + exportAccountServiceOverrides, + exportAccountServiceOverridesCSV, + importAccountServiceOverrides, discoverOrgAccounts, listPlanAccounts, setPlanAccounts, diff --git a/frontend/src/index.html b/frontend/src/index.html index 7e4baac72..ada145cde 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -821,6 +821,30 @@

Accounts

+ + +
+ Service Override Backup +

Export every account's service overrides to back them up or copy them to another CUDly install, or import an export file. Rows are matched by account ID, then by provider and account number. Each row replaces that account's override for its service; overrides not in the file are not changed.

+
+ + +
+
+ + + + + + +
+

+
+
diff --git a/frontend/src/modules/override-transfer.ts b/frontend/src/modules/override-transfer.ts new file mode 100644 index 000000000..31cb4bbb5 --- /dev/null +++ b/frontend/src/modules/override-transfer.ts @@ -0,0 +1,220 @@ +/** + * Service override export/import panel in Admin -> Accounts (issue #9). + * + * Export downloads every per-account service override as JSON or CSV. Import + * reads a JSON or CSV export, previews it with a dry run, then applies it. + * Rows are matched to accounts on the server (by account ID, else by provider + * and account number), so a file from another CUDly install imports too. + */ + +import * as api from '../api'; +import type { + OverrideImportMode, + OverrideImportReport, + OverrideImportRequest, + OverrideTransferRow, +} from '../api/accounts'; +import type { ApiError } from '../api/types'; +import { friendlyError } from '../api/errors'; +import { badgeClass, type BadgeTone } from '../lib/status-badge'; +import { showToast } from '../toast'; +import { confirmDialog } from '../confirmDialog'; + +const ACTION_TONES: Record = { + insert: 'success', + update: 'warning', + unchanged: 'muted', + rejected: 'danger', +}; + +const ACTION_LABELS: Record = { + insert: 'New', + update: 'Changed', + unchanged: 'Unchanged', + rejected: 'Rejected', +}; + +/** The selected file's rows, ready to send. Reset when the file changes. */ +let pendingImport: Pick | null = null; + +function byId(id: string): T | null { + return document.getElementById(id) as T | null; +} + +function downloadText(filename: string, content: string, contentType: string): void { + const blob = new Blob([content], { type: contentType }); + const a = document.createElement('a'); + a.href = URL.createObjectURL(blob); + a.download = filename; + document.body.appendChild(a); + a.click(); + document.body.removeChild(a); + URL.revokeObjectURL(a.href); +} + +function exportFilename(ext: string): string { + return `cudly-service-overrides-${new Date().toISOString().slice(0, 10)}.${ext}`; +} + +export async function exportOverrides(format: 'json' | 'csv'): Promise { + try { + if (format === 'csv') { + downloadText(exportFilename('csv'), await api.exportAccountServiceOverridesCSV(), 'text/csv'); + } else { + const doc = await api.exportAccountServiceOverrides(); + downloadText(exportFilename('json'), JSON.stringify(doc, null, 2), 'application/json'); + } + } catch (err) { + showToast({ message: `Failed to export overrides: ${friendlyError(err)}`, kind: 'error' }); + } +} + +/** + * Turns a file's text into an import payload: CSV text is sent as is (the + * server parses it), JSON may be an export document or a bare row array. + */ +export function parseImportFile(name: string, text: string): Pick { + if (name.toLowerCase().endsWith('.csv')) { + return { csv: text }; + } + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + throw new Error('The file is not valid JSON or CSV.'); + } + const rows = Array.isArray(parsed) ? parsed : (parsed as { overrides?: unknown })?.overrides; + if (!Array.isArray(rows)) { + throw new Error('The JSON file has no "overrides" list.'); + } + return { overrides: rows as OverrideTransferRow[] }; +} + +function setStatus(text: string): void { + const el = byId('override-import-status'); + if (el) el.textContent = text; +} + +function setImportButtonsEnabled(enabled: boolean): void { + for (const id of ['override-import-preview-btn', 'override-import-apply-btn']) { + const btn = byId(id); + if (btn) btn.disabled = !enabled; + } +} + +async function onFileChosen(input: HTMLInputElement): Promise { + pendingImport = null; + setImportButtonsEnabled(false); + renderReport(null); + const file = input.files?.[0]; + if (!file) { + setStatus(''); + return; + } + try { + pendingImport = parseImportFile(file.name, await file.text()); + setImportButtonsEnabled(true); + setStatus(`${file.name} is ready. Preview it to see what would change.`); + } catch (err) { + setStatus((err as Error).message); + } +} + +function selectedMode(): OverrideImportMode { + return byId('override-import-mode')?.value === 'best_effort' ? 'best_effort' : 'strict'; +} + +/** Runs the import (dry run or apply) and renders its per-row report. */ +export async function runImport(dryRun: boolean): Promise { + if (!pendingImport) return; + setImportButtonsEnabled(false); + try { + const report = await api.importAccountServiceOverrides({ ...pendingImport, mode: selectedMode(), dry_run: dryRun }); + renderReport(report); + setStatus(reportSummary(report)); + if (report.applied) { + showToast({ message: 'Service overrides imported', kind: 'success' }); + } + } catch (err) { + const report = (err as ApiError).details?.['report'] as OverrideImportReport | undefined; + renderReport(report ?? null); + setStatus(report ? `${reportSummary(report)} ${friendlyError(err)}` : friendlyError(err)); + } finally { + setImportButtonsEnabled(pendingImport !== null); + } +} + +async function confirmAndApply(): Promise { + const ok = await confirmDialog({ + title: 'Import service overrides?', + body: 'Each row in the file replaces that account\'s override for its service, including clearing fields the row leaves empty. Overrides not in the file are not changed.', + confirmLabel: 'Import', + }); + if (ok) await runImport(false); +} + +export function reportSummary(report: OverrideImportReport): string { + const s = report.summary; + const counts = `${s['insert'] ?? 0} new, ${s['update'] ?? 0} changed, ${s['unchanged'] ?? 0} unchanged, ${s['rejected'] ?? 0} rejected.`; + if (report.applied) return `Imported: ${counts}`; + if (report.dry_run) return `Preview (nothing saved): ${counts}`; + return `Not imported: ${counts}`; +} + +function cell(text: string): HTMLTableCellElement { + const td = document.createElement('td'); + td.textContent = text; + return td; +} + +function renderReport(report: OverrideImportReport | null): void { + const container = byId('override-import-report'); + if (!container) return; + container.textContent = ''; + if (!report || report.rows.length === 0) return; + + const table = document.createElement('table'); + table.className = 'data-table'; + const head = table.createTHead().insertRow(); + for (const h of ['Row', 'Provider', 'Service', 'Account', 'Result', 'Reason']) { + const th = document.createElement('th'); + th.textContent = h; + head.appendChild(th); + } + const body = table.createTBody(); + for (const r of report.rows) { + const tr = body.insertRow(); + tr.appendChild(cell(String(r.row))); + tr.appendChild(cell(r.provider)); + tr.appendChild(cell(r.service)); + const acct = cell(r.account_id ?? ''); + acct.className = 'override-transfer-account'; + tr.appendChild(acct); + const badgeTd = document.createElement('td'); + const badge = document.createElement('span'); + badge.className = badgeClass(ACTION_TONES[r.action] ?? 'muted'); + badge.textContent = ACTION_LABELS[r.action] ?? r.action; + badgeTd.appendChild(badge); + tr.appendChild(badgeTd); + tr.appendChild(cell(r.error ?? '')); + } + container.appendChild(table); +} + +/** Wires the panel once; safe to call on every Accounts tab visit. */ +export function initOverrideTransfer(): void { + const panel = byId('accounts-override-transfer'); + if (!panel || panel.dataset['wired'] === '1') return; + panel.dataset['wired'] = '1'; + byId('override-export-json-btn')?.addEventListener('click', () => void exportOverrides('json')); + byId('override-export-csv-btn')?.addEventListener('click', () => void exportOverrides('csv')); + const file = byId('override-import-file'); + file?.addEventListener('change', () => void onFileChosen(file)); + byId('override-import-preview-btn')?.addEventListener('click', () => void runImport(true)); + byId('override-import-apply-btn')?.addEventListener('click', () => void confirmAndApply()); +} + +/** Test hook: reset module state between tests. */ +export function resetOverrideTransferForTest(): void { + pendingImport = null; +} diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 5ced7a3ac..7a651f2c8 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3281,6 +3281,7 @@ export async function loadAccountsTab(): Promise { void loadAccountsForProvider('gcp'); void initFederationPanel(source); void import('./modules/registrations').then(m => m.initRegistrations()); + void import('./modules/override-transfer').then(m => m.initOverrideTransfer()); void renderAccountsOverview(); } diff --git a/frontend/src/styles/settings.css b/frontend/src/styles/settings.css index c564fd613..5e677adc9 100644 --- a/frontend/src/styles/settings.css +++ b/frontend/src/styles/settings.css @@ -710,3 +710,14 @@ tr.override-disabled { overflow-x: auto; } } + +/* Service override backup panel (issue #9): keep the export and import + controls grouped at the start of the row instead of the controls-bar's + space-between spread, and keep account IDs on one line. */ +.override-transfer-controls { + justify-content: flex-start; +} + +.override-transfer-account { + white-space: nowrap; +} From c561b38ff7bdc67f7310a876a823fdef96adbe19 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 11:03:57 +0000 Subject: [PATCH 07/10] test(frontend): cover the service override backup panel Covers file parsing (CSV passthrough, export document or bare array, bad JSON), the summary wording, enabling the buttons on a valid file, dry-run requests in the chosen mode, rendering a strict refusal's report from the error details, the confirmation before import, and both downloads. Refs #9 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/override-transfer.test.ts | 179 ++++++++++++++++++ 1 file changed, 179 insertions(+) create mode 100644 frontend/src/__tests__/override-transfer.test.ts diff --git a/frontend/src/__tests__/override-transfer.test.ts b/frontend/src/__tests__/override-transfer.test.ts new file mode 100644 index 000000000..8be33cdc0 --- /dev/null +++ b/frontend/src/__tests__/override-transfer.test.ts @@ -0,0 +1,179 @@ +/** + * Service override export/import panel (issue #9). + */ +import fs from 'fs'; +import path from 'path'; +import { + exportOverrides, + initOverrideTransfer, + parseImportFile, + reportSummary, + resetOverrideTransferForTest, + runImport, +} from '../modules/override-transfer'; +import type { OverrideImportReport } from '../api/accounts'; + +jest.mock('../api', () => ({ + exportAccountServiceOverrides: jest.fn(), + exportAccountServiceOverridesCSV: jest.fn(), + importAccountServiceOverrides: jest.fn(), +})); +jest.mock('../toast', () => ({ showToast: jest.fn() })); +jest.mock('../confirmDialog', () => ({ confirmDialog: jest.fn() })); + +import * as api from '../api'; +import { showToast } from '../toast'; +import { confirmDialog } from '../confirmDialog'; + +const html = fs.readFileSync(path.join(__dirname, '..', 'index.html'), 'utf8'); + +function mountPanel(): void { + const doc = new DOMParser().parseFromString(html, 'text/html'); + const panel = doc.getElementById('accounts-override-transfer'); + if (!panel) throw new Error('panel missing from index.html'); + document.body.replaceChildren(document.importNode(panel, true)); +} + +function report(overrides: Partial = {}): OverrideImportReport { + return { + mode: 'strict', + dry_run: true, + applied: false, + summary: { insert: 1, update: 1, unchanged: 0, rejected: 0 }, + rows: [ + { row: 1, account_id: 'a-1', provider: 'aws', service: 'ec2', action: 'update' }, + { row: 2, account_id: 'a-1', provider: 'aws', service: 'rds', action: 'insert' }, + ], + ...overrides, + }; +} + +async function chooseFile(name: string, text: string): Promise { + const input = document.getElementById('override-import-file') as HTMLInputElement; + const file = new File([text], name); + Object.defineProperty(file, 'text', { value: () => Promise.resolve(text) }); + Object.defineProperty(input, 'files', { value: [file], configurable: true }); + input.dispatchEvent(new Event('change')); + await new Promise((r) => setTimeout(r, 0)); +} + +const button = (id: string) => document.getElementById(id) as HTMLButtonElement; + +beforeEach(() => { + jest.clearAllMocks(); + resetOverrideTransferForTest(); + mountPanel(); + initOverrideTransfer(); +}); + +describe('parseImportFile', () => { + test('sends CSV text as is', () => { + expect(parseImportFile('x.CSV', 'provider,service\naws,ec2')).toEqual({ csv: 'provider,service\naws,ec2' }); + }); + + test('accepts an export document or a bare row array', () => { + const rows = [{ provider: 'aws', service: 'ec2' }]; + expect(parseImportFile('x.json', JSON.stringify({ version: 1, overrides: rows }))).toEqual({ overrides: rows }); + expect(parseImportFile('x.json', JSON.stringify(rows))).toEqual({ overrides: rows }); + }); + + test('rejects invalid JSON and JSON without rows', () => { + expect(() => parseImportFile('x.json', '{')).toThrow('not valid JSON or CSV'); + expect(() => parseImportFile('x.json', '{"rows":[]}')).toThrow('no "overrides" list'); + }); +}); + +describe('reportSummary', () => { + test('says whether anything was saved', () => { + expect(reportSummary(report())).toBe('Preview (nothing saved): 1 new, 1 changed, 0 unchanged, 0 rejected.'); + expect(reportSummary(report({ dry_run: false, applied: true }))).toMatch(/^Imported: /); + expect(reportSummary(report({ dry_run: false }))).toMatch(/^Not imported: /); + }); +}); + +describe('import flow', () => { + test('buttons stay disabled until a file is chosen', async () => { + expect(button('override-import-preview-btn').disabled).toBe(true); + await chooseFile('o.json', '[{"provider":"aws","service":"ec2"}]'); + expect(button('override-import-preview-btn').disabled).toBe(false); + expect(button('override-import-apply-btn').disabled).toBe(false); + }); + + test('a bad file explains itself and keeps the buttons disabled', async () => { + await chooseFile('o.json', 'nope'); + expect(document.getElementById('override-import-status')?.textContent).toContain('not valid JSON or CSV'); + expect(button('override-import-preview-btn').disabled).toBe(true); + }); + + test('preview sends a dry run in the chosen mode and renders the rows', async () => { + (api.importAccountServiceOverrides as jest.Mock).mockResolvedValue(report()); + await chooseFile('o.csv', 'provider,service\naws,ec2'); + (document.getElementById('override-import-mode') as HTMLSelectElement).value = 'best_effort'; + + await runImport(true); + + expect(api.importAccountServiceOverrides).toHaveBeenCalledWith({ csv: 'provider,service\naws,ec2', mode: 'best_effort', dry_run: true }); + const cells = [...document.querySelectorAll('#override-import-report tbody tr')].map((tr) => tr.textContent); + expect(cells).toEqual(['1awsec2a-1Changed', '2awsrdsa-1New']); + expect(showToast).not.toHaveBeenCalled(); + }); + + test('a strict refusal renders the report from the error details', async () => { + const refused = report({ summary: { insert: 0, update: 0, unchanged: 0, rejected: 1 }, rows: [ + { row: 1, provider: 'aws', service: 'Bad', action: 'rejected', error: 'invalid service name "Bad"' }, + ] }); + (api.importAccountServiceOverrides as jest.Mock).mockRejectedValue( + Object.assign(new Error('import rejected: fix the rejected rows or use best_effort mode'), { status: 400, details: { report: refused } }), + ); + await chooseFile('o.json', '[{"provider":"aws","service":"Bad"}]'); + + await runImport(true); + + expect(document.querySelector('#override-import-report tbody')?.textContent).toContain('invalid service name'); + expect(document.getElementById('override-import-status')?.textContent).toContain('1 rejected'); + }); + + test('import asks for confirmation before applying', async () => { + (api.importAccountServiceOverrides as jest.Mock).mockResolvedValue(report({ dry_run: false, applied: true })); + (confirmDialog as jest.Mock).mockResolvedValueOnce(false).mockResolvedValueOnce(true); + await chooseFile('o.json', '[{"provider":"aws","service":"ec2"}]'); + + button('override-import-apply-btn').click(); + await new Promise((r) => setTimeout(r, 0)); + expect(api.importAccountServiceOverrides).not.toHaveBeenCalled(); + + button('override-import-apply-btn').click(); + await new Promise((r) => setTimeout(r, 0)); + expect(api.importAccountServiceOverrides).toHaveBeenCalledWith(expect.objectContaining({ dry_run: false, mode: 'strict' })); + expect(showToast).toHaveBeenCalledWith({ message: 'Service overrides imported', kind: 'success' }); + }); +}); + +describe('export', () => { + beforeEach(() => { + URL.createObjectURL = jest.fn(() => 'blob:x'); + URL.revokeObjectURL = jest.fn(); + }); + + test('downloads JSON and CSV exports', async () => { + (api.exportAccountServiceOverrides as jest.Mock).mockResolvedValue({ version: 1, exported_at: 't', overrides: [] }); + (api.exportAccountServiceOverridesCSV as jest.Mock).mockResolvedValue('account_id\n'); + const clicks: string[] = []; + jest.spyOn(HTMLAnchorElement.prototype, 'click').mockImplementation(function (this: HTMLAnchorElement) { + clicks.push(this.download); + }); + + await exportOverrides('json'); + await exportOverrides('csv'); + + expect(clicks).toHaveLength(2); + expect(clicks[0]).toMatch(/^cudly-service-overrides-\d{4}-\d{2}-\d{2}\.json$/); + expect(clicks[1]).toMatch(/\.csv$/); + }); + + test('an export failure is a toast', async () => { + (api.exportAccountServiceOverrides as jest.Mock).mockRejectedValue(new Error('boom')); + await exportOverrides('json'); + expect(showToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' })); + }); +}); From 48bdb4ee3392b07770d31cbec68cc8e4dd5a7909 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 11:14:13 +0000 Subject: [PATCH 08/10] test(api): keep a fixture account ID off the secret scanner The cross-tenant fixture put an all-digit UUID and a 12-digit account number on one line next to AccountID, which the git-secrets account-ID detector flags in the full-tree scan. Use a UUID without a 12-digit run and put the fields on separate lines. Refs #9 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_override_import_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_override_import_test.go b/internal/api/handler_override_import_test.go index 335dfca23..fb187bd17 100644 --- a/internal/api/handler_override_import_test.go +++ b/internal/api/handler_override_import_test.go @@ -52,8 +52,9 @@ func TestImportOverrides_MatchesAnotherTenantsRowsByExternalID(t *testing.T) { store := newOverrideTransferStore() h := transferHandler(t, store, nil) rows := []OverrideTransferRow{{ - AccountID: "99999999-9999-9999-9999-999999999999", AccountExternalID: "123456789012", - Provider: "aws", Service: "elasticache", Term: ptr(1), Payment: ptr("no-upfront"), + AccountID: "f0000000-0000-4000-8000-00000000f00d", + AccountExternalID: "123456789012", + Provider: "aws", Service: "elasticache", Term: ptr(1), Payment: ptr("no-upfront"), }} report, err := runImport(t, h, OverrideImportRequest{Overrides: rows}) From d9f183f10b00ab36d74886d64291b113d714d4b7 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 14:17:21 +0000 Subject: [PATCH 09/10] fix(api): neutralise every text cell in the override CSV export The export only prefixed formula-like account names, but ramp_schedule, payment, the external ID and the six include/exclude lists are stored user input too, so a stored "=HYPERLINK(...)" ran as a formula when an admin opened the export in a spreadsheet (rE7 review on #1486). Every text cell now gets the OWASP quote prefix when it starts with = + - @ tab or CR, and also when it starts with a quote, so import can strip exactly one prefix and restore the stored value. A quote before any other character is not an escape and is kept. Numbers, booleans and timestamps are formatted here and stay as they are. Co-Authored-By: Claude Opus 5.5 --- .../api/handler_override_transfer_test.go | 108 +++++++++++++++++- internal/api/override_transfer_csv.go | 41 +++++-- 2 files changed, 138 insertions(+), 11 deletions(-) diff --git a/internal/api/handler_override_transfer_test.go b/internal/api/handler_override_transfer_test.go index 6b8d83545..ddfb5d6c1 100644 --- a/internal/api/handler_override_transfer_test.go +++ b/internal/api/handler_override_transfer_test.go @@ -2,12 +2,16 @@ package api import ( "context" + "encoding/csv" "encoding/json" + "fmt" "strings" "testing" "time" + "unicode/utf8" "github.com/aws/aws-lambda-go/events" + "github.com/jackc/pgx/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -16,12 +20,32 @@ import ( ) // overrideTransferStore is an in-memory account and override store for the -// export/import tests (issue #9). +// export/import tests (issue #9). Like Postgres, it refuses a ramp_schedule +// longer than the VARCHAR(32) column, and WithTx undoes every write of a +// failed transaction. failSave, when set, injects a store failure. type overrideTransferStore struct { *MockConfigStore accounts []config.CloudAccount overrides map[string][]config.AccountServiceOverride saved []config.AccountServiceOverride + failSave func(*config.AccountServiceOverride) error +} + +func (s *overrideTransferStore) WithTx(_ context.Context, fn func(pgx.Tx) error) error { + overrides := make(map[string][]config.AccountServiceOverride, len(s.overrides)) + for k, v := range s.overrides { + overrides[k] = append([]config.AccountServiceOverride(nil), v...) + } + saved := append([]config.AccountServiceOverride(nil), s.saved...) + if err := fn(nil); err != nil { + s.overrides, s.saved = overrides, saved + return err + } + return nil +} + +func (s *overrideTransferStore) SaveAccountServiceOverrideTx(ctx context.Context, _ pgx.Tx, o *config.AccountServiceOverride) error { + return s.SaveAccountServiceOverride(ctx, o) } func (s *overrideTransferStore) ListCloudAccounts(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { @@ -33,6 +57,14 @@ func (s *overrideTransferStore) ListAccountServiceOverrides(_ context.Context, a } func (s *overrideTransferStore) SaveAccountServiceOverride(_ context.Context, o *config.AccountServiceOverride) error { + if o.RampSchedule != nil && utf8.RuneCountInString(*o.RampSchedule) > 32 { + return fmt.Errorf("value too long for type character varying(32)") + } + if s.failSave != nil { + if err := s.failSave(o); err != nil { + return err + } + } s.saved = append(s.saved, *o) list := s.overrides[o.AccountID] for i := range list { @@ -160,3 +192,77 @@ func importBody(t *testing.T, in OverrideImportRequest) string { require.NoError(t, err) return string(b) } + +// formulaOverride holds a spreadsheet formula in every user-supplied text +// field the service-config rules do not restrict, and a value that itself +// starts with the escape quote. +func formulaOverride() config.AccountServiceOverride { + return config.AccountServiceOverride{ + ID: "o-5", AccountID: xferAWS, Provider: "aws", Service: "opensearch", + RampSchedule: ptr(`=HYPERLINK("http://x","y")`), + IncludeEngines: []string{"-2+3", "postgres"}, ExcludeEngines: []string{"@SUM(A1)"}, + IncludeRegions: []string{"+1"}, ExcludeRegions: []string{"'quoted", "=x"}, + IncludeTypes: []string{"=1+2"}, ExcludeTypes: []string{"'=1+2"}, + } +} + +// csvTextColumns are the CSV cells that carry stored or user-supplied text. +var csvTextColumns = map[string]bool{ + "account_id": true, "account_name": true, "account_external_id": true, "provider": true, "service": true, + "payment": true, "ramp_schedule": true, + "include_engines": true, "exclude_engines": true, "include_regions": true, + "exclude_regions": true, "include_types": true, "exclude_types": true, +} + +func TestOverridesCSV_NeutralizesEveryTextCellAndRoundTrips(t *testing.T) { + o := formulaOverride() + o.Payment = ptr("+cmd") + acct := &config.CloudAccount{ID: xferAWS, Name: "@name", Provider: "aws", ExternalID: "-123"} + row := overrideTransferRowFrom(acct, &o) + row.Provider, row.Service = "=aws", "+svc" + + text, err := encodeOverridesCSV([]OverrideTransferRow{row}) + require.NoError(t, err) + records, err := csv.NewReader(strings.NewReader(text)).ReadAll() + require.NoError(t, err) + require.Len(t, records, 2) + for i, cell := range records[1] { + col := records[0][i] + if csvTextColumns[col] && cell != "" { + assert.NotContains(t, "=+-@\t\r", cell[:1], "column %s cell %q would run as a formula", col, cell) + } + } + + back, err := decodeOverridesCSV(text) + require.NoError(t, err) + require.Len(t, back, 1) + assert.Equal(t, row.fields(), back[0].fields(), "import must strip the escape prefix again") + assert.Equal(t, []string{"-123", "=aws", "+svc"}, []string{back[0].AccountExternalID, back[0].Provider, back[0].Service}) +} + +func TestOverridesCSV_KeepsAQuoteThatIsNotAnEscape(t *testing.T) { + csvText := "account_id,provider,service,include_types,ramp_schedule\n" + xferAWS + ",aws,ec2,'m5;'=x,'immediate\n" + rows, err := decodeOverridesCSV(csvText) + require.NoError(t, err) + require.Len(t, rows, 1) + assert.Equal(t, []string{"'m5", "'=x"}, rows[0].IncludeTypes, "only a leading cell prefix is an escape") + assert.Equal(t, "'immediate", *rows[0].RampSchedule, "a quote before a non-formula character is data") +} + +func TestImportOverrides_CSVWithFormulaValuesReimportsUnchanged(t *testing.T) { + store := newOverrideTransferStore() + store.overrides[xferAWS] = append(store.overrides[xferAWS], formulaOverride()) + h := transferHandler(t, store, nil) + + exported, err := h.exportAccountServiceOverrides(context.Background(), exportRequest("csv")) + require.NoError(t, err) + body := exported.(*rawResponse).body + assert.Contains(t, body, `"'=HYPERLINK(""http://x"",""y"")"`) + assert.Contains(t, body, ",'=1+2,''=1+2,") + + got, err := h.importAccountServiceOverrides(context.Background(), adminRequest(importBody(t, OverrideImportRequest{CSV: body}))) + require.NoError(t, err) + report := got.(*OverrideImportReport) + assert.Equal(t, 5, report.Summary[overrideActionUnchanged], "%+v", report.Rows) + assert.Empty(t, store.saved) +} diff --git a/internal/api/override_transfer_csv.go b/internal/api/override_transfer_csv.go index 220384717..5a3ae9a49 100644 --- a/internal/api/override_transfer_csv.go +++ b/internal/api/override_transfer_csv.go @@ -41,27 +41,48 @@ func encodeOverridesCSV(rows []OverrideTransferRow) (string, error) { return buf.String(), w.Error() } +// overrideCSVRecord renders one row. Every text cell goes through csvSafeText; +// only the cells this package formats itself (booleans, numbers, timestamps) +// are written as is. func overrideCSVRecord(r *OverrideTransferRow) []string { + list := func(l []string) string { return csvSafeText(strings.Join(l, overrideCSVListSep)) } return []string{ - r.AccountID, csvSafeText(r.AccountName), r.AccountExternalID, r.Provider, r.Service, - formatBoolPtr(r.Enabled), formatIntPtr(r.Term), derefString(r.Payment), formatFloatPtr(r.Coverage), derefString(r.RampSchedule), - strings.Join(r.IncludeEngines, overrideCSVListSep), strings.Join(r.ExcludeEngines, overrideCSVListSep), - strings.Join(r.IncludeRegions, overrideCSVListSep), strings.Join(r.ExcludeRegions, overrideCSVListSep), - strings.Join(r.IncludeTypes, overrideCSVListSep), strings.Join(r.ExcludeTypes, overrideCSVListSep), + csvSafeText(r.AccountID), csvSafeText(r.AccountName), csvSafeText(r.AccountExternalID), csvSafeText(r.Provider), csvSafeText(r.Service), + formatBoolPtr(r.Enabled), formatIntPtr(r.Term), csvSafeText(derefString(r.Payment)), formatFloatPtr(r.Coverage), csvSafeText(derefString(r.RampSchedule)), + list(r.IncludeEngines), list(r.ExcludeEngines), + list(r.IncludeRegions), list(r.ExcludeRegions), + list(r.IncludeTypes), list(r.ExcludeTypes), formatTimePtr(r.CreatedAt), formatTimePtr(r.UpdatedAt), } } -// csvSafeText stops a spreadsheet from running a free-text cell as a formula -// (CSV injection). Only the account name is free text; import ignores it, so -// the prefix never round-trips into stored data. +// csvFormulaLeads are the leading characters that make a spreadsheet read a +// cell as a formula (OWASP CSV injection). A leading quote is escaped too, so +// that csvUnescapeText can undo the prefix without guessing. +const csvFormulaLeads = "=+-@\t\r'" + +// csvSafeText stops a spreadsheet from running a text cell as a formula (CSV +// injection) by prefixing a quote. Stored override values (ramp_schedule, the +// include/exclude lists, payment) are user-supplied, so every text cell is +// escaped, not only the account name. func csvSafeText(s string) string { - if s != "" && strings.ContainsRune("=+-@\t\r", rune(s[0])) { + if s != "" && strings.ContainsRune(csvFormulaLeads, rune(s[0])) { return "'" + s } return s } +// csvUnescapeText undoes csvSafeText on import: a quote followed by a +// formula-lead character (or another quote) is the escape prefix and is +// dropped, so an escaped value round-trips to the stored original. A quote +// before anything else is not something csvSafeText writes, so it is kept. +func csvUnescapeText(s string) string { + if len(s) >= 2 && s[0] == '\'' && strings.ContainsRune(csvFormulaLeads, rune(s[1])) { + return s[1:] + } + return s +} + func formatBoolPtr(p *bool) string { if p == nil { return "" @@ -142,7 +163,7 @@ func overrideCSVHeaderIndex(header []string) (map[string]int, error) { func overrideRowFromCSV(rec []string, index map[string]int) (OverrideTransferRow, error) { cell := func(name string) string { if i, ok := index[name]; ok && i < len(rec) { - return strings.TrimSpace(rec[i]) + return csvUnescapeText(strings.TrimSpace(rec[i])) } return "" } From 978259a70d4055680db06b359094a5023c182868 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 14:17:21 +0000 Subject: [PATCH 10/10] fix(api): validate override column bounds and import in one transaction A strict import could still write part of a file: nothing checked ramp_schedule against its VARCHAR(32) column, so a long value late in the file passed planning, the rows before it were written, and the upsert failure answered 500 with no report (rE7 review on #1486). Planning now rejects, with a per-row reason, a ramp_schedule over 32 characters, coverage with more than 2 decimals (DECIMAL(5,2) would round it and the row would never read back as unchanged), and control characters in ramp_schedule or any list value (Postgres refuses NUL in TEXT). The writes then run in one WithTx transaction through the new SaveAccountServiceOverrideTx, so a store failure rolls the whole import back instead of leaving the rows before it. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_override_import.go | 102 +++++++++++++++--- ...andler_override_import_integration_test.go | 73 +++++++++++++ internal/api/handler_override_import_test.go | 91 ++++++++++++++++ internal/api/openapi.yaml | 19 ++-- internal/config/interfaces.go | 3 + internal/config/store_postgres.go | 18 +++- internal/mocks/stores.go | 11 ++ 7 files changed, 296 insertions(+), 21 deletions(-) create mode 100644 internal/api/handler_override_import_integration_test.go diff --git a/internal/api/handler_override_import.go b/internal/api/handler_override_import.go index a80d97c6f..007e76196 100644 --- a/internal/api/handler_override_import.go +++ b/internal/api/handler_override_import.go @@ -5,16 +5,30 @@ import ( "encoding/json" "fmt" "math" + "strconv" + "strings" "time" + "unicode" + "unicode/utf8" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/aws/aws-lambda-go/events" "github.com/google/uuid" + "github.com/jackc/pgx/v5" ) // maxOverrideImportRows bounds one import request. const maxOverrideImportRows = 5000 +// Column bounds of account_service_overrides (migration 000011): ramp_schedule +// is VARCHAR(32) and coverage is DECIMAL(5,2). provider, service, payment and +// term are bounded by the service-config rules. Rows outside a bound are +// rejected at planning time, so the database never refuses an import write. +const ( + maxOverrideRampScheduleLen = 32 + maxOverrideCoverageDecimals = 2 +) + // Import modes: strict applies nothing when any row is rejected; best_effort // applies the valid rows and reports the rest. const ( @@ -64,6 +78,7 @@ type OverrideImportReport struct { // plannedOverride is a validated row with the stored override it replaces. type plannedOverride struct { + rowNum int account *config.CloudAccount row *OverrideTransferRow existing *config.AccountServiceOverride @@ -76,7 +91,8 @@ type plannedOverride struct { // the same rules as a service config, and compared with the stored override. // A row sets every field of its override; overrides not in the file are left // alone. A strict import with a rejected row answers 400 with the report and -// writes nothing. Re-running an import is safe: writes are upserts. +// writes nothing. The writes run in one transaction, so a store failure rolls +// the whole import back. Re-running an import is safe: writes are upserts. func (h *Handler) importAccountServiceOverrides(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { session, err := h.requirePermission(ctx, req, "update", "accounts") if err != nil { @@ -194,7 +210,7 @@ func (h *Handler) planOverrideRow(ctx context.Context, idx *overrideAccountIndex if err != nil { return plannedOverride{}, err } - p := plannedOverride{account: acct, row: row} + p := plannedOverride{rowNum: rowNum, account: acct, row: row} key := acct.ID + "/" + row.Provider + "/" + row.Service if first, dup := seen[key]; dup { return p, rejectRow("duplicate of row %d", first) @@ -204,12 +220,15 @@ func (h *Handler) planOverrideRow(ctx context.Context, idx *overrideAccountIndex } // validateOverrideRow applies the service-config rules (provider, service -// name, term, provider-specific payment token, coverage) and the -// commitment-option combination check the PUT endpoint runs. +// name, term, provider-specific payment token, coverage), the column bounds +// and the commitment-option combination check the PUT endpoint runs. func (h *Handler) validateOverrideRow(ctx context.Context, row *OverrideTransferRow) error { if !serviceNameRegex.MatchString(row.Service) { return rejectRow("invalid service name %q", row.Service) } + if err := validateOverrideColumns(row); err != nil { + return err + } cfg := config.ServiceConfig{Provider: row.Provider, Service: row.Service, Term: derefInt(row.Term), Payment: derefString(row.Payment)} if row.Coverage != nil { if math.IsNaN(*row.Coverage) || math.IsInf(*row.Coverage, 0) { @@ -229,6 +248,55 @@ func (h *Handler) validateOverrideRow(ctx context.Context, row *OverrideTransfer return nil } +// validateOverrideColumns checks the fields the service-config rules do not +// cover against the column bounds: ramp_schedule length, coverage precision, +// and no control characters (Postgres refuses a NUL in TEXT) in ramp_schedule +// or any list value. +func validateOverrideColumns(row *OverrideTransferRow) error { + if row.RampSchedule != nil { + if n := utf8.RuneCountInString(*row.RampSchedule); n > maxOverrideRampScheduleLen { + return rejectRow("ramp_schedule is %d characters; at most %d are allowed", n, maxOverrideRampScheduleLen) + } + if hasControlChar(*row.RampSchedule) { + return rejectRow("ramp_schedule must not contain control characters") + } + } + if row.Coverage != nil && decimalPlaces(*row.Coverage) > maxOverrideCoverageDecimals { + return rejectRow("coverage must have at most %d decimal places, got %v", maxOverrideCoverageDecimals, *row.Coverage) + } + return validateOverrideLists(row) +} + +func validateOverrideLists(row *OverrideTransferRow) error { + lists := []struct { + name string + values []string + }{ + {"include_engines", row.IncludeEngines}, {"exclude_engines", row.ExcludeEngines}, + {"include_regions", row.IncludeRegions}, {"exclude_regions", row.ExcludeRegions}, + {"include_types", row.IncludeTypes}, {"exclude_types", row.ExcludeTypes}, + } + for _, l := range lists { + for _, v := range l.values { + if hasControlChar(v) { + return rejectRow("%s must not contain control characters", l.name) + } + } + } + return nil +} + +func hasControlChar(s string) bool { + return strings.IndexFunc(s, unicode.IsControl) >= 0 +} + +// decimalPlaces counts the digits after the point in v's shortest decimal +// form, the text pgx sends to the DECIMAL column. +func decimalPlaces(v float64) int { + _, frac, _ := strings.Cut(strconv.FormatFloat(v, 'f', -1, 64), ".") + return len(frac) +} + // findStoredOverride returns the stored override the row replaces, or nil. // Each account's overrides are read once per import. func (h *Handler) findStoredOverride(ctx context.Context, cache map[string][]config.AccountServiceOverride, accountID string, row *OverrideTransferRow) (*config.AccountServiceOverride, error) { @@ -248,19 +316,25 @@ func (h *Handler) findStoredOverride(ctx context.Context, cache map[string][]con return nil, nil } -// applyOverrideImport writes the insert and update rows. A store failure -// stops the import part way; re-running it is safe because writes upsert. +// applyOverrideImport writes the insert and update rows in one transaction: +// either every planned row is written or, on any store failure, none is. func (h *Handler) applyOverrideImport(ctx context.Context, plan []plannedOverride) error { now := time.Now() - for i, p := range plan { - o := &config.AccountServiceOverride{ID: uuid.New().String(), AccountID: p.account.ID, Provider: p.row.Provider, Service: p.row.Service, CreatedAt: now, UpdatedAt: now} - if p.existing != nil { - o.ID, o.CreatedAt = p.existing.ID, p.existing.CreatedAt - } - applyOverrideFieldsTo(o, p.row.fields()) - if err := h.config.SaveAccountServiceOverride(ctx, o); err != nil { - return fmt.Errorf("accounts: import stopped after %d of %d writes: %w", i, len(plan), err) + err := h.config.WithTx(ctx, func(tx pgx.Tx) error { + for _, p := range plan { + o := &config.AccountServiceOverride{ID: uuid.New().String(), AccountID: p.account.ID, Provider: p.row.Provider, Service: p.row.Service, CreatedAt: now, UpdatedAt: now} + if p.existing != nil { + o.ID, o.CreatedAt = p.existing.ID, p.existing.CreatedAt + } + applyOverrideFieldsTo(o, p.row.fields()) + if err := h.config.SaveAccountServiceOverrideTx(ctx, tx, o); err != nil { + return fmt.Errorf("row %d: %w", p.rowNum, err) + } } + return nil + }) + if err != nil { + return fmt.Errorf("accounts: import rolled back, nothing was written: %w", err) } return nil } diff --git a/internal/api/handler_override_import_integration_test.go b/internal/api/handler_override_import_integration_test.go new file mode 100644 index 000000000..265238f3d --- /dev/null +++ b/internal/api/handler_override_import_integration_test.go @@ -0,0 +1,73 @@ +//go:build integration + +package api + +import ( + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +// TestImportOverrides_StrictIsAllOrNothingPostgres runs the override import +// (issue #9) against Postgres: a strict import whose last row breaks a column +// bound is a 400 with a report and writes nothing, and a write the database +// refuses part way rolls the whole import back. +func TestImportOverrides_StrictIsAllOrNothingPostgres(t *testing.T) { + ctx := context.Background() + pg := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(pg.DB) + acct := &config.CloudAccount{Name: "import fixture", Provider: "aws", ExternalID: "123456789012", Enabled: true} + require.NoError(t, store.CreateCloudAccount(ctx, acct)) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminAccountSession(), nil) + mockAuth.grantPermissionsScoped([]auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, nil) + h := &Handler{auth: mockAuth, config: store} + + rows := fortyImportRows() + for i := range rows { + rows[i].AccountID = acct.ID + } + count := func() int { + var n int + require.NoError(t, pg.DB.Pool().QueryRow(ctx, `SELECT count(*) FROM account_service_overrides WHERE account_id = $1`, acct.ID).Scan(&n)) + return n + } + + // Row 40's ramp_schedule does not fit VARCHAR(32). + bad := append([]OverrideTransferRow(nil), rows...) + bad[39].RampSchedule = ptr(strings.Repeat("r", 33)) + _, err := runImport(t, h, OverrideImportRequest{Overrides: bad}) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 400, ce.code) + report := ce.details["report"].(*OverrideImportReport) + assert.Equal(t, overrideActionRejected, report.Rows[39].Action) + assert.Equal(t, 0, count(), "a rejected strict import writes nothing") + + // The database refuses row 40: rows 1-39 roll back with it. + _, err = pg.DB.Pool().Exec(ctx, ` + CREATE FUNCTION reject_svc_40() RETURNS trigger AS $$ + BEGIN RAISE EXCEPTION 'svc-40 rejected'; END $$ LANGUAGE plpgsql; + CREATE TRIGGER reject_svc_40 BEFORE INSERT ON account_service_overrides + FOR EACH ROW WHEN (NEW.service = 'svc-40') EXECUTE FUNCTION reject_svc_40();`) + require.NoError(t, err) + _, err = runImport(t, h, OverrideImportRequest{Overrides: rows}) + require.Error(t, err) + assert.Contains(t, err.Error(), "row 40") + assert.Equal(t, 0, count(), "a failed write rolls the whole import back") + + _, err = pg.DB.Pool().Exec(ctx, `DROP TRIGGER reject_svc_40 ON account_service_overrides`) + require.NoError(t, err) + got, err := runImport(t, h, OverrideImportRequest{Overrides: rows}) + require.NoError(t, err) + assert.True(t, got.Applied) + assert.Equal(t, 40, count()) +} diff --git a/internal/api/handler_override_import_test.go b/internal/api/handler_override_import_test.go index fb187bd17..d0374f310 100644 --- a/internal/api/handler_override_import_test.go +++ b/internal/api/handler_override_import_test.go @@ -2,10 +2,15 @@ package api import ( "context" + "errors" + "fmt" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) func runImport(t *testing.T, h *Handler, in OverrideImportRequest) (*OverrideImportReport, error) { @@ -198,3 +203,89 @@ func TestImportOverrides_RequiresUpdateAccounts(t *testing.T) { require.True(t, ok, "got %v", err) assert.Equal(t, 403, ce.code) } + +// fortyImportRows is a valid 40-row file of new overrides on one account. +func fortyImportRows() []OverrideTransferRow { + rows := make([]OverrideTransferRow, 40) + for i := range rows { + rows[i] = OverrideTransferRow{AccountID: xferAWS, Provider: "aws", Service: fmt.Sprintf("svc-%02d", i+1), Term: ptr(1), RampSchedule: ptr("immediate")} + } + return rows +} + +func TestImportOverrides_StrictLateOverlongRampWritesNothing(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, nil) + rows := fortyImportRows() + rows[39].RampSchedule = ptr(strings.Repeat("r", 33)) // ramp_schedule is VARCHAR(32) + + _, err := runImport(t, h, OverrideImportRequest{Overrides: rows}) + ce, ok := IsClientError(err) + require.True(t, ok, "a bad row must be a 4xx with a report, got %v", err) + assert.Equal(t, 400, ce.code) + report := ce.details["report"].(*OverrideImportReport) + require.Len(t, report.Rows, 40) + assert.Equal(t, 39, report.Summary[overrideActionInsert]) + assert.Equal(t, overrideActionRejected, report.Rows[39].Action) + assert.Contains(t, report.Rows[39].Error, "ramp_schedule is 33 characters") + assert.Empty(t, store.saved, "strict mode must not write rows 1-39") + assert.Len(t, store.overrides[xferAWS], 2) +} + +func TestImportOverrides_StoreFailureRollsBackEveryWrite(t *testing.T) { + for _, mode := range []string{overrideImportStrict, overrideImportBestEffort} { + t.Run(mode, func(t *testing.T) { + store := newOverrideTransferStore() + store.failSave = func(o *config.AccountServiceOverride) error { + if o.Service == "svc-40" { + return errors.New("connection reset") + } + return nil + } + h := transferHandler(t, store, nil) + + _, err := runImport(t, h, OverrideImportRequest{Mode: mode, Overrides: fortyImportRows()}) + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a store failure is a server error") + assert.Contains(t, err.Error(), "row 40") + assert.Empty(t, store.saved, "a failed write must roll back rows 1-39") + assert.Len(t, store.overrides[xferAWS], 2) + }) + } +} + +func TestImportOverrides_RejectsValuesOutsideColumnBounds(t *testing.T) { + cases := map[string]struct { + row OverrideTransferRow + want string + }{ + "ramp too long": {OverrideTransferRow{RampSchedule: ptr(strings.Repeat("é", 33))}, "ramp_schedule is 33 characters"}, + "ramp control char": {OverrideTransferRow{RampSchedule: ptr("weekly\n")}, "ramp_schedule must not contain control characters"}, + "coverage precision": {OverrideTransferRow{Coverage: ptr(33.333)}, "coverage must have at most 2 decimal places"}, + "list NUL": {OverrideTransferRow{IncludeTypes: []string{"m5\x00"}}, "include_types must not contain control characters"}, + "list tab": {OverrideTransferRow{ExcludeRegions: []string{"\tus-east-1"}}, "exclude_regions must not contain control characters"}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + store := newOverrideTransferStore() + h := transferHandler(t, store, nil) + row := tc.row + row.AccountID, row.Provider, row.Service = xferAWS, "aws", "memorydb" + + report, err := runImport(t, h, OverrideImportRequest{Mode: overrideImportBestEffort, Overrides: []OverrideTransferRow{row}}) + require.NoError(t, err) + require.Equal(t, []string{overrideActionRejected}, actions(report)) + assert.Contains(t, report.Rows[0].Error, tc.want) + assert.Empty(t, store.saved) + }) + } + + // The bounds themselves are accepted. + store := newOverrideTransferStore() + h := transferHandler(t, store, nil) + row := OverrideTransferRow{AccountID: xferAWS, Provider: "aws", Service: "memorydb", RampSchedule: ptr(strings.Repeat("é", 32)), Coverage: ptr(33.33)} + report, err := runImport(t, h, OverrideImportRequest{Overrides: []OverrideTransferRow{row}}) + require.NoError(t, err) + assert.Equal(t, []string{overrideActionInsert}, actions(report)) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 5204b9f6b..32279b5de 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3995,7 +3995,9 @@ paths: so the file can be imported into another tenant (issue #9). Requires view:accounts. JSON by default; format=csv returns the same rows as CSV (list cells are semicolon-separated, an empty cell is an unset - field). + field). A CSV text cell starting with = + - @ tab, CR or ' gets a ' + prefix so a spreadsheet does not run it as a formula; import strips + that prefix again. parameters: - name: format in: query @@ -4031,11 +4033,15 @@ paths: provider and account_external_id, and must be in the caller's account scope. A row sets every field of its override (an unset field clears it); overrides not in the file are left alone. Rows are validated with - the service-config rules and the commitment-option check of the PUT - endpoint. strict (default) writes nothing when any row is rejected - and answers 400 with the report under details.report; best_effort - writes the valid rows. dry_run reports without writing. Requires - update:accounts. Writes are upserts, so re-running an import is safe. + the service-config rules, the commitment-option check of the PUT + endpoint and the column bounds (ramp_schedule at most 32 characters, + coverage at most 2 decimal places, no control characters in + ramp_schedule or list values). strict (default) writes nothing when + any row is rejected and answers 400 with the report under + details.report; best_effort writes the valid rows. dry_run reports + without writing. The writes run in one transaction, so a store + failure writes nothing. Requires update:accounts. Writes are upserts, + so re-running an import is safe. requestBody: required: true content: @@ -7710,6 +7716,7 @@ components: format: double ramp_schedule: type: string + maxLength: 32 include_engines: type: array items: diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index e3397c086..43bd93014 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -585,6 +585,9 @@ type StoreInterface interface { // Account service overrides GetAccountServiceOverride(ctx context.Context, accountID, provider, service string) (*AccountServiceOverride, error) SaveAccountServiceOverride(ctx context.Context, override *AccountServiceOverride) error + // SaveAccountServiceOverrideTx is SaveAccountServiceOverride inside the + // caller's WithTx transaction. + SaveAccountServiceOverrideTx(ctx context.Context, tx pgx.Tx, override *AccountServiceOverride) error DeleteAccountServiceOverride(ctx context.Context, accountID, provider, service string) error ListAccountServiceOverrides(ctx context.Context, accountID string) ([]AccountServiceOverride, error) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 9b3e82fc4..750d3723b 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -4277,6 +4277,22 @@ func (s *PostgresStore) GetAccountServiceOverride(ctx context.Context, accountID // SaveAccountServiceOverride upserts an account service override. func (s *PostgresStore) SaveAccountServiceOverride(ctx context.Context, o *AccountServiceOverride) error { + return saveAccountServiceOverride(ctx, s.db, o) +} + +// SaveAccountServiceOverrideTx upserts an account service override inside the +// caller's transaction, so a batch of writes (the override import) commits or +// rolls back as one. +func (s *PostgresStore) SaveAccountServiceOverrideTx(ctx context.Context, tx pgx.Tx, o *AccountServiceOverride) error { + return saveAccountServiceOverride(ctx, tx, o) +} + +// overrideExecutor is satisfied by both the pool and a pgx.Tx. +type overrideExecutor interface { + Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) +} + +func saveAccountServiceOverride(ctx context.Context, db overrideExecutor, o *AccountServiceOverride) error { if o.ID == "" { o.ID = uuid.New().String() } @@ -4302,7 +4318,7 @@ func (s *PostgresStore) SaveAccountServiceOverride(ctx context.Context, o *Accou include_types = $14, exclude_types = $15, updated_at = NOW() ` - _, err := s.db.Exec(ctx, query, + _, err := db.Exec(ctx, query, o.ID, o.AccountID, o.Provider, o.Service, o.Enabled, o.Term, o.Payment, o.Coverage, o.RampSchedule, o.IncludeEngines, o.ExcludeEngines, o.IncludeRegions, o.ExcludeRegions, diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index c4c46b586..6897e073f 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1569,6 +1569,17 @@ func (m *MockConfigStore) SaveAccountServiceOverride(ctx context.Context, overri return args.Error(0) } +// SaveAccountServiceOverrideTx falls back to SaveAccountServiceOverride when +// no expectation is registered; see UpdatePurchasePlanTx. +func (m *MockConfigStore) SaveAccountServiceOverrideTx(ctx context.Context, tx pgx.Tx, override *config.AccountServiceOverride) error { + m.record("SaveAccountServiceOverrideTx", ctx, tx, override) + if !isExpected(&m.Mock, "SaveAccountServiceOverrideTx") { + return m.SaveAccountServiceOverride(ctx, override) + } + args := m.Called(ctx, tx, override) + return args.Error(0) +} + func (m *MockConfigStore) DeleteAccountServiceOverride(ctx context.Context, accountID, provider, service string) error { m.record("DeleteAccountServiceOverride", ctx, accountID, provider, service) if !isExpected(&m.Mock, "DeleteAccountServiceOverride") {