From 762d016bff191e0ff9868e6d5350886cab40a061 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 11:54:28 +0200 Subject: [PATCH 1/2] fix(api): match history rows to scoped accounts by provider and external id The History scope filter keyed account names by external id alone, so two accounts on different providers sharing an external id made the visible rows depend on list order. Resolve the name from CloudAccountID when set, else from provider plus external id, and skip names that are ambiguous. Closes #550 --- internal/api/handler_history.go | 33 +++++- .../handler_history_scope_provider_test.go | 111 ++++++++++++++++++ 2 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 internal/api/handler_history_scope_provider_test.go diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 85e673c22..7d4aaefa4 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -997,7 +997,10 @@ func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, se if allowed.AllowsAll() { return purchases, nil } - nameByID := h.resolveAccountNamesByID(ctx) + accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{}) + if err != nil { + return nil, fmt.Errorf("failed to list cloud accounts: %w", err) + } filtered := make([]config.PurchaseHistoryRecord, 0, len(purchases)) for _rvc := range purchases { p := purchases[_rvc] @@ -1012,13 +1015,39 @@ func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, se } continue } - if allowed.Allows(p.AccountID, nameByID[p.AccountID]) { + if allowed.Allows(p.AccountID, historyRowAccountName(accounts, p)) { filtered = append(filtered, p) } } return filtered, nil } +// historyRowAccountName returns the registered account name for a history row, +// or "" when it cannot be determined unambiguously. External ids are unique +// only per provider, so a row without CloudAccountID is matched on provider plus +// external id; a legacy row without a provider matches only when exactly one +// account across all providers has that external id. +func historyRowAccountName(accounts []config.CloudAccount, p config.PurchaseHistoryRecord) string { + if p.CloudAccountID != nil { + return accountNameByID(accounts, *p.CloudAccountID) + } + if p.Provider != "" { + return accountNameByExternalID(accounts, p.Provider, p.AccountID) + } + var name string + matches := 0 + for i := range accounts { + if accounts[i].ExternalID == p.AccountID { + name = accounts[i].Name + matches++ + } + } + if matches != 1 { + return "" + } + return name +} + func summarizePurchaseHistory(purchases []config.PurchaseHistoryRecord) HistorySummary { summary := HistorySummary{TotalPurchases: len(purchases), TotalUpfront: new(float64)} for _rvc := range purchases { diff --git a/internal/api/handler_history_scope_provider_test.go b/internal/api/handler_history_scope_provider_test.go new file mode 100644 index 000000000..e879d1321 --- /dev/null +++ b/internal/api/handler_history_scope_provider_test.go @@ -0,0 +1,111 @@ +package api + +import ( + "context" + "encoding/json" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// historyScopeIDs drives GET /api/history through HandleRequest as a session +// user whose allow-list is `scope`, with the given accounts registered, and +// returns the purchase ids that come back. +func historyScopeIDs(t *testing.T, scope []string, accounts []config.CloudAccount, rows []config.PurchaseHistoryRecord) []string { + t.Helper() + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: "u-1", Email: "u@example.com"}, nil) + mockAuth.On("HasPermissionAPI", mock.Anything, "u-1", mock.Anything, mock.Anything).Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, "u-1").Return(scope, nil) + + store := new(MockConfigStore) + store.On("GetAllPurchaseHistory", mock.Anything, mock.Anything).Return(append([]config.PurchaseHistoryRecord(nil), rows...), nil) + store.On("GetExecutionsByStatuses", mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil) + store.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return accounts, nil + } + + h := &Handler{auth: mockAuth, config: store} + resp, err := h.HandleRequest(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer tok"}, + QueryStringParameters: map[string]string{}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "GET", Path: "/api/history"}, + }, + }) + require.NoError(t, err) + require.Equal(t, 200, resp.StatusCode, "body: %s", resp.Body) + + var body HistoryResponse + require.NoError(t, json.Unmarshal([]byte(resp.Body), &body)) + ids := make([]string, 0, len(body.Purchases)) + for _, p := range body.Purchases { + ids = append(ids, p.PurchaseID) + } + return ids +} + +// Issue #550: external ids are unique per (provider, external_id) only. A +// name-scoped History user must see rows of the account in their scope and +// none of the same-external-id account under another provider, whatever the +// order the accounts are listed in. +func TestHandleRequest_History_SameExternalIDAcrossProviders(t *testing.T) { + t.Parallel() + awsDev := config.CloudAccount{ID: "u-aws", Name: "dev", Provider: "aws", ExternalID: "123"} + azProd := config.CloudAccount{ID: "u-az", Name: "prod", Provider: "azure", ExternalID: "123"} + awsRow := config.PurchaseHistoryRecord{PurchaseID: "aws-row", Provider: "aws", AccountID: "123", Status: "completed"} + azRow := config.PurchaseHistoryRecord{PurchaseID: "az-row", Provider: "azure", AccountID: "123", Status: "completed"} + rows := []config.PurchaseHistoryRecord{awsRow, azRow} + bothOrders := map[string][]config.CloudAccount{ + "aws listed first": {awsDev, azProd}, + "azure listed first": {azProd, awsDev}, + } + + for order, accounts := range bothOrders { + t.Run("scope names azure account, "+order, func(t *testing.T) { + t.Parallel() + assert.Equal(t, []string{"az-row"}, historyScopeIDs(t, []string{"prod"}, accounts, rows)) + }) + t.Run("scope names aws account, "+order, func(t *testing.T) { + t.Parallel() + assert.Equal(t, []string{"aws-row"}, historyScopeIDs(t, []string{"dev"}, accounts, rows)) + }) + } + + t.Run("legacy row without provider on an ambiguous external id is not name-matched", func(t *testing.T) { + t.Parallel() + legacy := []config.PurchaseHistoryRecord{{PurchaseID: "legacy", AccountID: "123", Status: "completed"}} + assert.Empty(t, historyScopeIDs(t, []string{"prod"}, []config.CloudAccount{awsDev, azProd}, legacy)) + }) + + t.Run("a set cloud account id stays authoritative", func(t *testing.T) { + t.Parallel() + uuid := "u-az" + row := []config.PurchaseHistoryRecord{{PurchaseID: "by-uuid", Provider: "aws", AccountID: "123", CloudAccountID: &uuid, Status: "completed"}} + assert.Equal(t, []string{"by-uuid"}, historyScopeIDs(t, []string{"prod"}, []config.CloudAccount{awsDev, azProd}, row)) + assert.Empty(t, historyScopeIDs(t, []string{"dev"}, []config.CloudAccount{awsDev, azProd}, row)) + }) +} + +// Unambiguous accounts keep matching by name or id, with and without a +// provider on the row. +func TestHandleRequest_History_UniqueExternalIDStillMatches(t *testing.T) { + t.Parallel() + in := config.CloudAccount{ID: "u-in", Name: "in", Provider: "aws", ExternalID: "111"} + out := config.CloudAccount{ID: "u-out", Name: "out", Provider: "aws", ExternalID: "222"} + rows := []config.PurchaseHistoryRecord{ + {PurchaseID: "in-prov", Provider: "aws", AccountID: "111", Status: "completed"}, + {PurchaseID: "in-legacy", AccountID: "111", Status: "completed"}, + {PurchaseID: "out", Provider: "aws", AccountID: "222", Status: "completed"}, + } + got := historyScopeIDs(t, []string{"in"}, []config.CloudAccount{in, out}, rows) + assert.ElementsMatch(t, []string{"in-prov", "in-legacy"}, got) + got = historyScopeIDs(t, []string{"111"}, []config.CloudAccount{in, out}, rows) + assert.ElementsMatch(t, []string{"in-prov", "in-legacy"}, got) +} From eaefd3f0782c76e525ab68993ca732c4fefa2096 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 12:16:07 +0200 Subject: [PATCH 2/2] fix(api): resolve history row account by UUID before external id In-flight history rows store the account UUID in AccountID with no CloudAccountID, so provider plus external id lookup hid them from name-scoped users. Try the UUID first. Add tests for UUID rows and for same-provider external id ambiguity. --- internal/api/handler_history.go | 10 ++++-- .../handler_history_scope_provider_test.go | 35 +++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 7d4aaefa4..4f2e0e5f6 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -1023,14 +1023,18 @@ func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, se } // historyRowAccountName returns the registered account name for a history row, -// or "" when it cannot be determined unambiguously. External ids are unique -// only per provider, so a row without CloudAccountID is matched on provider plus -// external id; a legacy row without a provider matches only when exactly one +// or "" when it cannot be determined unambiguously. Without CloudAccountID it +// tries the account UUID first (in-flight rows store the UUID in AccountID), +// then provider plus external id, since external ids are unique only per +// provider. A legacy row without a provider matches only when exactly one // account across all providers has that external id. func historyRowAccountName(accounts []config.CloudAccount, p config.PurchaseHistoryRecord) string { if p.CloudAccountID != nil { return accountNameByID(accounts, *p.CloudAccountID) } + if name := accountNameByID(accounts, p.AccountID); name != "" { + return name + } if p.Provider != "" { return accountNameByExternalID(accounts, p.Provider, p.AccountID) } diff --git a/internal/api/handler_history_scope_provider_test.go b/internal/api/handler_history_scope_provider_test.go index e879d1321..8ed8d6681 100644 --- a/internal/api/handler_history_scope_provider_test.go +++ b/internal/api/handler_history_scope_provider_test.go @@ -109,3 +109,38 @@ func TestHandleRequest_History_UniqueExternalIDStillMatches(t *testing.T) { got = historyScopeIDs(t, []string{"111"}, []config.CloudAccount{in, out}, rows) assert.ElementsMatch(t, []string{"in-prov", "in-legacy"}, got) } + +// In-flight history rows carry the account UUID in AccountID with no +// CloudAccountID, so a name-scoped user must still see their own rows. +func TestHandleRequest_History_UUIDInAccountID(t *testing.T) { + t.Parallel() + dev := config.CloudAccount{ID: "u-aws", Name: "dev", Provider: "aws", ExternalID: "123"} + prod := config.CloudAccount{ID: "u-az", Name: "prod", Provider: "azure", ExternalID: "123"} + accounts := []config.CloudAccount{dev, prod} + + t.Run("pending row with provider", func(t *testing.T) { + t.Parallel() + rows := []config.PurchaseHistoryRecord{{PurchaseID: "p1", Provider: "aws", AccountID: "u-aws", Status: "pending"}} + assert.Equal(t, []string{"p1"}, historyScopeIDs(t, []string{"dev"}, accounts, rows)) + assert.Empty(t, historyScopeIDs(t, []string{"prod"}, accounts, rows)) + }) + t.Run("pending row without provider", func(t *testing.T) { + t.Parallel() + rows := []config.PurchaseHistoryRecord{{PurchaseID: "p2", AccountID: "u-aws", Status: "pending"}} + assert.Equal(t, []string{"p2"}, historyScopeIDs(t, []string{"dev"}, accounts, rows)) + assert.Empty(t, historyScopeIDs(t, []string{"prod"}, accounts, rows)) + }) +} + +// Two accounts of one provider sharing an external id are ambiguous: the row +// is hidden from both name scopes. +func TestHandleRequest_History_SameProviderAmbiguousExternalID(t *testing.T) { + t.Parallel() + a := config.CloudAccount{ID: "u-a", Name: "dev", Provider: "aws", ExternalID: "123"} + b := config.CloudAccount{ID: "u-b", Name: "prod", Provider: "aws", ExternalID: "123"} + rows := []config.PurchaseHistoryRecord{{PurchaseID: "amb", Provider: "aws", AccountID: "123", Status: "completed"}} + for _, scope := range []string{"dev", "prod"} { + assert.Empty(t, historyScopeIDs(t, []string{scope}, []config.CloudAccount{a, b}, rows), scope) + assert.Empty(t, historyScopeIDs(t, []string{scope}, []config.CloudAccount{b, a}, rows), scope) + } +}