From 12b23aa1566848ab42119a20335d7e354152ebd6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 17:07:14 +0100 Subject: [PATCH 1/3] fix(api): canonicalise account_ids and share the account-filter merge parseAccountIDs now trims and lowercases each id, so history and analytics callers compare canonical UUIDs: an uppercase id no longer yields an empty admin result or a refused scoped user. mergeAccountFilter reuses appendMissing and addExternalIDForProvider instead of its own copy, and the stale single-account comment in the analytics handler is corrected. Co-Authored-By: Claude Opus 5.5 --- internal/api/coverage_gaps_test.go | 7 +++++ internal/api/handler_analytics.go | 28 ++++++------------ .../api/handler_analytics_account_ids_test.go | 29 +++++++++++++++++++ internal/api/handler_history_test.go | 17 +++++++++++ internal/api/validation.go | 3 +- 5 files changed, 64 insertions(+), 20 deletions(-) diff --git a/internal/api/coverage_gaps_test.go b/internal/api/coverage_gaps_test.go index b90945607..62f415554 100644 --- a/internal/api/coverage_gaps_test.go +++ b/internal/api/coverage_gaps_test.go @@ -57,6 +57,13 @@ func TestParseAccountIDs(t *testing.T) { } } +func TestParseAccountIDs_Canonical(t *testing.T) { + id := "abcdefab-abcd-abcd-abcd-abcdefabcdef" + ids, err := parseAccountIDs(" " + strings.ToUpper(id) + " ,\t" + id) + require.NoError(t, err) + assert.Equal(t, []string{id, id}, ids) +} + // TestParseAccountIDs_Cap verifies the MaxAccountIDsPerRequest guard // blocks input amplification — an unbounded account_ids list could fan // out into thousands of per-account DB queries / cloud API calls. diff --git a/internal/api/handler_analytics.go b/internal/api/handler_analytics.go index db848de44..a6d132ab6 100644 --- a/internal/api/handler_analytics.go +++ b/internal/api/handler_analytics.go @@ -4,7 +4,6 @@ package api import ( "context" "fmt" - "slices" "time" "github.com/LeanerCloud/cloud-commitments-platform/internal/analytics" @@ -150,13 +149,13 @@ func (h *Handler) getHistoryAnalytics(ctx context.Context, req *events.LambdaFun return nil, err } - // For scoped users we require account_id and validate it's in their scope. - // We don't (yet) support analytics across a subset — the underlying - // aggregate takes a single account_id. An unrestricted/admin session - // can pass "" to mean "all accessible accounts". The requested account (a - // top-bar chip UUID, a raw external id, or "" for all) resolves to the - // dual-column filter inputs so rows that carry only the external account_id - // (cloud_account_id NULL) are aggregated (issue #701/#498/#866). + // Scoped users must name at least one account (account_ids or the legacy + // account_id) and every named account must be in their scope. An + // unrestricted/admin session can omit both to mean "all accessible + // accounts". The requested accounts (top-bar chip UUIDs, or a raw external + // id via the legacy param) resolve to the dual-column filter inputs so rows + // that carry only the external account_id (cloud_account_id NULL) are + // aggregated (issue #701/#498/#866). requested, err := requestedAnalyticsAccounts(params) if err != nil { return nil, err @@ -301,19 +300,10 @@ func (h *Handler) resolveAnalyticsAccountFilter(ctx context.Context, session *Se // mergeAccountFilter unions one account's dual-column filter into the // accumulated one, dropping duplicates. func mergeAccountFilter(uuids []string, ext map[string][]string, addUUIDs []string, addExt map[string][]string) (mergedUUIDs []string, mergedExt map[string][]string) { - for _, u := range addUUIDs { - if !slices.Contains(uuids, u) { - uuids = append(uuids, u) - } - } + uuids = appendMissing(uuids, addUUIDs...) for provider, ids := range addExt { - if ext == nil { - ext = map[string][]string{} - } for _, id := range ids { - if !slices.Contains(ext[provider], id) { - ext[provider] = append(ext[provider], id) - } + ext = addExternalIDForProvider(ext, provider, id) } } return uuids, ext diff --git a/internal/api/handler_analytics_account_ids_test.go b/internal/api/handler_analytics_account_ids_test.go index c255f3372..326a84e05 100644 --- a/internal/api/handler_analytics_account_ids_test.go +++ b/internal/api/handler_analytics_account_ids_test.go @@ -126,3 +126,32 @@ func TestAnalytics_AccountIDs_DuplicatesCollapse(t *testing.T) { assert.Equal(t, []string{idsAcctA}, uuids) assert.Equal(t, map[string][]string{"aws": {"111111111111"}}, externals) } + +// Issue #1452: parseAccountIDs canonicalises (trim + lowercase), so an +// uppercase or padded id behaves exactly like its lowercase form, including +// for a scoped user's own account. +func TestAnalytics_AccountIDs_UppercasePaddedCanonical(t *testing.T) { + t.Parallel() + // Hex letters so ToUpper really changes the ids. + const idA, idB = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb" + accounts := []config.CloudAccount{ + {ID: idA, Name: "a", Provider: "aws", ExternalID: "111111111111"}, + {ID: idB, Name: "b", Provider: "aws", ExternalID: "222222222222"}, + } + upper := " " + strings.ToUpper(idA) + " , " + strings.ToUpper(idB) + lower := idA + "," + idB + for _, path := range analyticsPaths { + for name, scope := range map[string][]string{"unrestricted": nil, "scoped": {"a", "b"}} { + t.Run(path+" "+name, func(t *testing.T) { + t.Parallel() + wantStatus, wantUUIDs, wantExt := analyticsParamsCall(t, path, scope, map[string]string{"account_ids": lower}, accounts) + status, uuids, externals := analyticsParamsCall(t, path, scope, map[string]string{"account_ids": upper}, accounts) + assert.Equal(t, 200, wantStatus) + assert.Equal(t, wantStatus, status) + assert.Equal(t, []string{idA, idB}, uuids) + assert.Equal(t, wantUUIDs, uuids) + assert.Equal(t, wantExt, externals) + }) + } + } +} diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 32760ef85..f9e6c9d3f 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -3,6 +3,7 @@ package api import ( "context" "errors" + "strings" "testing" "time" @@ -1082,6 +1083,22 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { assert.False(t, ids["exec-nil-account"], "execution with NULL exec.CloudAccountID AND no rec-level CloudAccountID must still be excluded (effective account is empty)") }) + t.Run("uppercase and padded account_ids are canonicalised (issue #1452)", func(t *testing.T) { + mockStore, handler, req, ctx := newHandler(t) + + uuidA := "aaaaaaaa-1111-1111-1111-111111111111" + uuidB := "bbbbbbbb-2222-2222-2222-222222222222" + + mockStore.On("ListPurchaseHistoryPage", ctx, config.PurchaseHistoryFilter{AccountIDs: []string{uuidA, uuidB}, Limit: historyStreamBatch(config.DefaultListLimit)}, noCursor). + Return([]config.PurchaseHistoryRecord{{AccountID: "acc-A", PurchaseID: "p-A"}}, nil).Once() + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil) + + result, err := handler.getHistory(ctx, req, map[string]string{"account_ids": " " + strings.ToUpper(uuidA) + " , " + strings.ToUpper(uuidB)}) + require.NoError(t, err) + require.Len(t, result.(HistoryResponse).Purchases, 1) + mockStore.AssertExpectations(t) + }) + t.Run("legacy singular account_id is folded into the dual-column filter", func(t *testing.T) { // account_id (singular) is an AWS-style external account number (not a // known cloud_accounts UUID in this fixture). resolveHistoryAccountFilter diff --git a/internal/api/validation.go b/internal/api/validation.go index b665337b4..c1e48ed0e 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -518,6 +518,7 @@ func validateRequestBodySize(body string) error { const MaxAccountIDsPerRequest = 200 // parseAccountIDs splits a comma-separated account_ids query parameter into a slice. +// Ids are trimmed and lowercased so every caller compares canonical UUIDs. // Empty entries are removed. Returns nil when the input is empty. // Returns an error if any value is not a valid UUID, or if the list // exceeds MaxAccountIDsPerRequest entries. @@ -547,7 +548,7 @@ func parseAccountIDs(raw string) ([]string, error) { if err := validateUUID(trimmed); err != nil { return nil, fmt.Errorf("invalid account_ids: %q is not a valid UUID", trimmed) } - ids = append(ids, trimmed) + ids = append(ids, strings.ToLower(trimmed)) } return ids, nil } From 3256bbe1b5291f70217636b41ecb2e4c71a62c8e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 17:07:22 +0100 Subject: [PATCH 2/3] fix(frontend): forward the full account selection to analytics routes The savings history chart and the trends view sent account_ids only when exactly one account was selected, so a multi-selection widened to all accounts for admins and returned 400 for scoped users. Both now forward every selected id as account_ids; getAnalyticsTrends takes accountIDs instead of the singular accountID. Closes #1452 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/api-analytics.test.ts | 9 +++++---- frontend/src/__tests__/savings-history.test.ts | 15 +++++++++++++++ frontend/src/__tests__/trends.test.ts | 6 +++--- frontend/src/api/analytics.ts | 6 +++--- frontend/src/modules/savings-history.ts | 10 ++++------ frontend/src/trends.ts | 6 ++---- 6 files changed, 32 insertions(+), 20 deletions(-) diff --git a/frontend/src/__tests__/api-analytics.test.ts b/frontend/src/__tests__/api-analytics.test.ts index 9fb588264..b3d9bab58 100644 --- a/frontend/src/__tests__/api-analytics.test.ts +++ b/frontend/src/__tests__/api-analytics.test.ts @@ -20,9 +20,9 @@ describe('getAnalyticsTrends', () => { expect(apiRequest).toHaveBeenCalledWith('/analytics/trends'); }); - test('forwards account_id, provider and the window, URL-encoded', async () => { + test('forwards account_ids, provider and the window, URL-encoded', async () => { await getAnalyticsTrends({ - accountID: 'acc/1 x', + accountIDs: ['acc/1 x', 'acc2'], provider: 'aws', start: '2026-05-01T00:00:00.000Z', end: '2026-10-23T12:00:00.000Z', @@ -30,14 +30,15 @@ describe('getAnalyticsTrends', () => { const url = (apiRequest as jest.Mock).mock.calls[0][0] as string; const qs = new URLSearchParams(url.split('?')[1]); expect(url.startsWith('/analytics/trends?')).toBe(true); - expect(qs.get('account_id')).toBe('acc/1 x'); + expect(qs.get('account_ids')).toBe('acc/1 x,acc2'); + expect(qs.has('account_id')).toBe(false); expect(qs.get('provider')).toBe('aws'); expect(qs.get('start')).toBe('2026-05-01T00:00:00.000Z'); expect(qs.get('end')).toBe('2026-10-23T12:00:00.000Z'); }); test('omits empty filter values', async () => { - await getAnalyticsTrends({ accountID: '', provider: '' }); + await getAnalyticsTrends({ accountIDs: [], provider: '' }); expect(apiRequest).toHaveBeenCalledWith('/analytics/trends'); }); }); diff --git a/frontend/src/__tests__/savings-history.test.ts b/frontend/src/__tests__/savings-history.test.ts index 14d842174..3d00a3690 100644 --- a/frontend/src/__tests__/savings-history.test.ts +++ b/frontend/src/__tests__/savings-history.test.ts @@ -662,6 +662,21 @@ describe('Savings History Module', () => { ); }); + test('forwards the full account selection when two or more are selected', async () => { + addPurchasesTab(true); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-A', 'acct-B']); + + initSavingsHistory(); + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + (getSavingsAnalytics as jest.Mock).mockClear(); + accountCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(getSavingsAnalytics).toHaveBeenCalledWith( + expect.objectContaining({ account_ids: ['acct-A', 'acct-B'] }) + ); + }); + test('provider chip change re-queries the chart when purchases-tab is active', async () => { addPurchasesTab(true); (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); diff --git a/frontend/src/__tests__/trends.test.ts b/frontend/src/__tests__/trends.test.ts index b3aaba1db..a35f072c4 100644 --- a/frontend/src/__tests__/trends.test.ts +++ b/frontend/src/__tests__/trends.test.ts @@ -170,11 +170,11 @@ describe('loadTrends', () => { expect(config.data.datasets[0].data).toEqual([null, null, null, null, 400, 400]); }); - test('forwards the single account chip and the window, never the provider when unset', async () => { - (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-uuid-1']); + test('forwards every selected account and the window, never the provider when unset', async () => { + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-uuid-1', 'acct-uuid-2']); await loadTrends(); const arg = (api.getAnalyticsTrends as jest.Mock).mock.calls[0][0]; - expect(arg.accountID).toBe('acct-uuid-1'); + expect(arg.accountIDs).toEqual(['acct-uuid-1', 'acct-uuid-2']); expect(arg.provider).toBeUndefined(); expect(arg.start).toBe(trendsWindow(6).start.toISOString()); }); diff --git a/frontend/src/api/analytics.ts b/frontend/src/api/analytics.ts index ad386f5ce..dc57b348b 100644 --- a/frontend/src/api/analytics.ts +++ b/frontend/src/api/analytics.ts @@ -53,8 +53,8 @@ export interface AnalyticsTrendsResponse { } export interface AnalyticsTrendsFilter { - /** Top-bar account chip (cloud_accounts UUID); omitted means all accessible. */ - accountID?: string; + /** Selected cloud_accounts UUIDs, sent as account_ids; empty or omitted means all accessible. */ + accountIDs?: string[]; /** Narrows by_service server-side; omitted means every provider. */ provider?: string; /** RFC 3339 window bounds. The backend caps the window at 366 days. */ @@ -68,7 +68,7 @@ export interface AnalyticsTrendsFilter { */ export async function getAnalyticsTrends(filter: AnalyticsTrendsFilter = {}): Promise { const params = new URLSearchParams(); - if (filter.accountID) params.set('account_id', filter.accountID); + if (filter.accountIDs && filter.accountIDs.length > 0) params.set('account_ids', filter.accountIDs.join(',')); if (filter.provider) params.set('provider', filter.provider); if (filter.start) params.set('start', filter.start); if (filter.end) params.set('end', filter.end); diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index ebb12fd61..66d348738 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -103,11 +103,9 @@ export async function loadSavingsHistory(): Promise { const period = periodSelect.value; const { start, end, interval } = getPeriodDates(period); - // Honour the global topbar filter chips (issue #503). The account chip - // is single-select, and the backend's /history/analytics takes a single - // account_id (see handler_analytics.go), so we forward the only selected - // ID, mirroring dashboard.ts loadSavingsTrendChart. The provider chip is - // forwarded too and the backend now applies it as a provider WHERE filter + // Honour the global topbar filter chips (issue #503). Every selected + // account is forwarded as account_ids; the backend scope-checks each id + // and sums them. The provider chip is forwarded too and the backend now applies it as a provider WHERE filter // on the savings-history query (issue #498, QA 2.3). const currentProvider = state.getCurrentProvider(); const currentAccountIDs = state.getCurrentAccountIDs(); @@ -123,7 +121,7 @@ export async function loadSavingsHistory(): Promise { end: end.toISOString(), interval, ...(currentProvider ? { provider: currentProvider } : {}), - ...(currentAccountIDs.length === 1 ? { account_ids: currentAccountIDs } : {}), + ...(currentAccountIDs.length > 0 ? { account_ids: currentAccountIDs } : {}), }); if (!data.data_points || data.data_points.length === 0) { diff --git a/frontend/src/trends.ts b/frontend/src/trends.ts index 111b102b2..05c389e5d 100644 --- a/frontend/src/trends.ts +++ b/frontend/src/trends.ts @@ -81,8 +81,6 @@ export async function loadTrends(): Promise { const provider = state.getCurrentProvider(); const accountIDs = state.getCurrentAccountIDs(); - // The account chip is single-select; forward only when exactly one is set. - const accountID = accountIDs.length === 1 ? accountIDs[0] : undefined; const { start, end } = trendsWindow(selectedPeriodMonths()); const seq = ++loadSeq; @@ -91,14 +89,14 @@ export async function loadTrends(): Promise { try { const data = await api.getAnalyticsTrends({ - accountID, + accountIDs, provider: provider || undefined, start: start.toISOString(), end: end.toISOString(), }); if (seq !== loadSeq) return; teardownSkeleton(container); - renderTrends(container, data, provider, monthKeysBetween(start, end), Boolean(provider || accountID)); + renderTrends(container, data, provider, monthKeysBetween(start, end), Boolean(provider || accountIDs.length > 0)); } catch (error) { if (seq !== loadSeq) return; teardownSkeleton(container); From b9f8cf4ffc045d7fcebd219158366831885c5120 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 17:22:58 +0100 Subject: [PATCH 3/3] test(api): scope the canonical-id test by account id Group scope entries match account ids only since #1116, so the scoped case of the uppercase account_ids test now lists ids, not display names. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_analytics_account_ids_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/api/handler_analytics_account_ids_test.go b/internal/api/handler_analytics_account_ids_test.go index caf1b4076..47c799d4f 100644 --- a/internal/api/handler_analytics_account_ids_test.go +++ b/internal/api/handler_analytics_account_ids_test.go @@ -142,7 +142,7 @@ func TestAnalytics_AccountIDs_UppercasePaddedCanonical(t *testing.T) { upper := " " + strings.ToUpper(idA) + " , " + strings.ToUpper(idB) lower := idA + "," + idB for _, path := range analyticsPaths { - for name, scope := range map[string][]string{"unrestricted": nil, "scoped": {"a", "b"}} { + for name, scope := range map[string][]string{"unrestricted": nil, "scoped": {idA, idB}} { t.Run(path+" "+name, func(t *testing.T) { t.Parallel() wantStatus, wantUUIDs, wantExt := analyticsParamsCall(t, path, scope, map[string]string{"account_ids": lower}, accounts)