diff --git a/frontend/src/__tests__/allowed-accounts.test.ts b/frontend/src/__tests__/allowed-accounts.test.ts index 7ac125ba4..43ca2fe5a 100644 --- a/frontend/src/__tests__/allowed-accounts.test.ts +++ b/frontend/src/__tests__/allowed-accounts.test.ts @@ -29,7 +29,11 @@ // --------------------------------------------------------------------------- jest.mock('../api', () => ({ - listAccounts: jest.fn(), + // #949/#951: the topbar dropdown now reads the minimal-disclosure endpoint + // (view:recommendations) instead of the view:accounts list. The backend + // applies the same allowed_accounts filter, so the mock still stands in for + // a backend-filtered subset. + listAccountsMinimal: jest.fn(), getHistory: jest.fn(), cancelPurchase: jest.fn(), })); @@ -150,7 +154,7 @@ describe('Account chip - allowed_accounts enforcement (issue #313)', () => { test('chip lists only the accounts returned by listAccounts (backend-filtered subset)', async () => { // Simulate backend returning only the two accounts the user is allowed // to access (the rest are filtered server-side by allowed_accounts). - (api.listAccounts as jest.Mock).mockResolvedValue([ + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([ { id: 'allowed-1', name: 'Prod AWS', external_id: '111111111111' }, { id: 'allowed-2', name: 'Staging AWS', external_id: '222222222222' }, ]); @@ -173,7 +177,7 @@ describe('Account chip - allowed_accounts enforcement (issue #313)', () => { }); test('chip does not contain accounts absent from the listAccounts response', async () => { - (api.listAccounts as jest.Mock).mockResolvedValue([ + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([ { id: 'allowed-only', name: 'Allowed Prod', external_id: '999999999999' }, ]); @@ -193,7 +197,7 @@ describe('Account chip - allowed_accounts enforcement (issue #313)', () => { }); test('chip shows only All Accounts when listAccounts returns empty list (zero allowed accounts)', async () => { - (api.listAccounts as jest.Mock).mockResolvedValue([]); + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([]); initTopbarFilters(); await new Promise((r) => setTimeout(r, 0)); @@ -221,7 +225,7 @@ describe('History list - allowed_accounts enforcement (issue #313)', () => { jest.clearAllMocks(); (getCurrentUser as jest.Mock).mockReturnValue(ADMIN); (confirmDialog as jest.Mock).mockResolvedValue(true); - (api.listAccounts as jest.Mock).mockResolvedValue([]); + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([]); }); test('renders exactly the rows returned by getHistory (no extra client-side rows)', async () => { diff --git a/frontend/src/__tests__/api-accounts.test.ts b/frontend/src/__tests__/api-accounts.test.ts index 3de6028e7..7a9e634a2 100644 --- a/frontend/src/__tests__/api-accounts.test.ts +++ b/frontend/src/__tests__/api-accounts.test.ts @@ -3,6 +3,7 @@ */ import { listAccounts, + listAccountsMinimal, createAccount, getAccount, updateAccount, @@ -83,6 +84,38 @@ describe('Accounts API Module', () => { }); }); + // #949/#951: minimal-disclosure list available to Standard / Read-Only users. + describe('listAccountsMinimal', () => { + const mockSummary = { id: 'acc-1', name: 'Test', external_id: '123', provider: 'aws' as const }; + + test('calls apiRequest with /accounts/list and no query string when no filters', async () => { + (apiRequest as jest.Mock).mockResolvedValue([mockSummary]); + + const result = await listAccountsMinimal(); + + expect(apiRequest).toHaveBeenCalledWith('/accounts/list'); + expect(result).toEqual([mockSummary]); + }); + + test('passes provider and search filters in the query string', async () => { + (apiRequest as jest.Mock).mockResolvedValue([]); + + await listAccountsMinimal({ provider: 'aws', search: 'prod' }); + + expect(apiRequest).toHaveBeenCalledWith('/accounts/list?provider=aws&search=prod'); + }); + + // The minimal endpoint intentionally does NOT support the enabled filter + // (it's a read-only projection for filter/prefill), so enabled is ignored. + test('ignores the enabled filter (not part of the minimal projection)', async () => { + (apiRequest as jest.Mock).mockResolvedValue([]); + + await listAccountsMinimal({ enabled: false }); + + expect(apiRequest).toHaveBeenCalledWith('/accounts/list'); + }); + }); + describe('createAccount', () => { test('calls apiRequest with POST and serialized body', async () => { (apiRequest as jest.Mock).mockResolvedValue(mockAccount); diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index be3f4cd6d..e297cc445 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -20,6 +20,10 @@ jest.mock('../api', () => ({ listPlanAccounts: jest.fn().mockResolvedValue([]), setPlanAccounts: jest.fn().mockResolvedValue(undefined), listAccounts: jest.fn().mockResolvedValue([]), + // #949/#951: plan-account search + commitment-target prefill now use the + // minimal-disclosure list (view:recommendations) so Standard / Read-Only + // users get a populated list instead of a 403 from the view:accounts paths. + listAccountsMinimal: jest.fn().mockResolvedValue([]), getAccount: jest.fn().mockResolvedValue(null) })); @@ -1157,14 +1161,25 @@ describe('Plans Module', () => { expect(modal?.classList.contains('hidden')).toBe(true); }); - test('shows error on save failure', async () => { + test('shows error toast and keeps the modal open on save failure (#949)', async () => { + // Issue #949 symptom: a failed create-plan left the modal open with NO + // feedback. Guard both halves: the error toast surfaces AND the modal + // stays open (so the user can retry) — never a silent no-op. (api.createPlan as jest.Mock).mockRejectedValue(new Error('Save failed')); console.error = jest.fn(); + const modal = document.getElementById('plan-modal'); + modal?.classList.remove('hidden'); + const event = { preventDefault: jest.fn() } as unknown as Event; await savePlan(event); - expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ message: 'Failed to save plan: Save failed' })); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ + kind: 'error', + message: 'Failed to save plan: Save failed', + })); + // Modal must NOT be closed when the save errors. + expect(modal?.classList.contains('hidden')).toBe(false); }); test('uses weekly-25pct ramp schedule', async () => { @@ -1387,12 +1402,15 @@ describe('Plans Module', () => { ); }); - test('fetches account by cloud_account_id to prefill chip', async () => { - (api.getAccount as jest.Mock).mockResolvedValueOnce({ - id: 'acct-uuid-123', - name: 'Prod AWS', - external_id: '123456789012', - }); + test('fetches account via minimal list by cloud_account_id to prefill chip (#949/#951)', async () => { + // The prefill now resolves the target from the minimal-disclosure list + // (view:recommendations) so it works for Standard / Read-Only users — + // the old per-id GET /api/accounts/:id (view:accounts) 403'd for them, + // leaving the target empty and making Save Plan silently no-op. + (api.listAccountsMinimal as jest.Mock).mockResolvedValueOnce([ + { id: 'other-acct', name: 'Other', external_id: '999', provider: 'aws' }, + { id: 'acct-uuid-123', name: 'Prod AWS', external_id: '123456789012', provider: 'aws' }, + ]); openCreatePlanModal([fixture]); @@ -1400,15 +1418,15 @@ describe('Plans Module', () => { await Promise.resolve(); await Promise.resolve(); - expect(api.getAccount).toHaveBeenCalledWith('acct-uuid-123'); + expect(api.listAccountsMinimal).toHaveBeenCalled(); const chips = document.getElementById('plan-accounts-selected'); expect(chips?.textContent).toContain('Prod AWS'); const hiddenIds = (document.getElementById('plan-account-ids') as HTMLInputElement).value; expect(hiddenIds).toContain('acct-uuid-123'); }); - test('does not throw and leaves account section empty when getAccount fails', async () => { - (api.getAccount as jest.Mock).mockRejectedValueOnce(new Error('network error')); + test('does not throw and leaves account section empty when minimal list fails', async () => { + (api.listAccountsMinimal as jest.Mock).mockRejectedValueOnce(new Error('network error')); openCreatePlanModal([fixture]); @@ -1420,27 +1438,42 @@ describe('Plans Module', () => { expect(hiddenIds).toBe(''); }); + test('leaves account section empty when cloud_account_id is not in the minimal list', async () => { + // Defensive: the target account isn't visible to this user (scoped out + // by allowed_accounts). No chip should be added; the user can search. + (api.listAccountsMinimal as jest.Mock).mockResolvedValueOnce([ + { id: 'some-other-acct', name: 'Other', external_id: '999', provider: 'aws' }, + ]); + + openCreatePlanModal([fixture]); + await Promise.resolve(); + await Promise.resolve(); + + const hiddenIds = (document.getElementById('plan-account-ids') as HTMLInputElement).value; + expect(hiddenIds).toBe(''); + }); + test('skips prefill when no cloud_account_id present', async () => { const noAccount: api.Recommendation = { ...fixture, cloud_account_id: undefined }; openCreatePlanModal([noAccount]); await Promise.resolve(); - expect(api.getAccount).not.toHaveBeenCalled(); + expect(api.listAccountsMinimal).not.toHaveBeenCalled(); }); - test('discards stale getAccount result when modal is reopened before promise resolves', async () => { - // Simulate a slow first getAccount call that resolves after the modal + test('discards stale minimal-list result when modal is reopened before promise resolves', async () => { + // Simulate a slow first list call that resolves after the modal // is closed and a new modal session starts (the race condition fixed // by the planModalSession guard — #770 CR Major). - let resolveFirstCall!: (value: api.CloudAccount) => void; - const firstCallPromise = new Promise(resolve => { + let resolveFirstCall!: (value: Array<{ id: string; name: string; external_id: string; provider: string }>) => void; + const firstCallPromise = new Promise>(resolve => { resolveFirstCall = resolve; }); - (api.getAccount as jest.Mock) + (api.listAccountsMinimal as jest.Mock) .mockReturnValueOnce(firstCallPromise) // first open: hangs - .mockResolvedValueOnce(null); // second open: no account + .mockResolvedValueOnce([]); // second open: no accounts // First modal open with cloud_account_id openCreatePlanModal([fixture]); @@ -1451,7 +1484,7 @@ describe('Plans Module', () => { openCreatePlanModal([noAccount]); // Now resolve the stale first promise — should be discarded - resolveFirstCall({ id: 'acct-uuid-123', name: 'Prod AWS', external_id: '123456789012' } as api.CloudAccount); + resolveFirstCall([{ id: 'acct-uuid-123', name: 'Prod AWS', external_id: '123456789012', provider: 'aws' }]); await Promise.resolve(); await Promise.resolve(); @@ -1478,18 +1511,16 @@ describe('Plans Module', () => { }); test('prefills the account chip when all commitments share one account', async () => { - (api.getAccount as jest.Mock).mockResolvedValueOnce({ - id: 'acct-uuid-123', - name: 'Prod AWS', - external_id: '123456789012', - }); + (api.listAccountsMinimal as jest.Mock).mockResolvedValueOnce([ + { id: 'acct-uuid-123', name: 'Prod AWS', external_id: '123456789012', provider: 'aws' }, + ]); const second: api.Recommendation = { ...fixture, id: 'rec-771', region: 'us-west-2' }; openCreatePlanModal([fixture, second]); await Promise.resolve(); await Promise.resolve(); - expect(api.getAccount).toHaveBeenCalledWith('acct-uuid-123'); + expect(api.listAccountsMinimal).toHaveBeenCalled(); const hiddenIds = (document.getElementById('plan-account-ids') as HTMLInputElement).value; expect(hiddenIds).toContain('acct-uuid-123'); }); @@ -1501,9 +1532,9 @@ describe('Plans Module', () => { await Promise.resolve(); // Config still prefills from the homogeneous fields, but the account - // is ambiguous so no chip is fetched. + // is ambiguous so no list lookup happens. expect((document.getElementById('plan-service') as HTMLSelectElement).value).toBe('ec2'); - expect(api.getAccount).not.toHaveBeenCalled(); + expect(api.listAccountsMinimal).not.toHaveBeenCalled(); const hiddenIds = (document.getElementById('plan-account-ids') as HTMLInputElement).value; expect(hiddenIds).toBe(''); }); @@ -2445,8 +2476,8 @@ describe('Plans Module', () => { jest.useRealTimers(); }); - test('account search passes the current plan provider to listAccounts', async () => { - (api.listAccounts as jest.Mock).mockResolvedValue([]); + test('account search passes the current plan provider to listAccountsMinimal (#949/#951)', async () => { + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([]); // openNewPlanModal calls form.reset() synchronously, resetting the provider // select back to its first option ('aws'). Wire up the modal first, then @@ -2470,13 +2501,15 @@ describe('Plans Module', () => { await Promise.resolve(); await Promise.resolve(); - expect(api.listAccounts).toHaveBeenCalledWith( + // Search uses the minimal-disclosure endpoint so it works for Standard / + // Read-Only users (view:recommendations), not the view:accounts list. + expect(api.listAccountsMinimal).toHaveBeenCalledWith( expect.objectContaining({ search: 'my-azure', provider: 'azure' }) ); }); test('account search passes aws provider when plan provider is aws', async () => { - (api.listAccounts as jest.Mock).mockResolvedValue([]); + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([]); openNewPlanModal(); await Promise.resolve(); @@ -2490,7 +2523,7 @@ describe('Plans Module', () => { await Promise.resolve(); await Promise.resolve(); - expect(api.listAccounts).toHaveBeenCalledWith( + expect(api.listAccountsMinimal).toHaveBeenCalledWith( expect.objectContaining({ provider: 'aws' }) ); }); diff --git a/frontend/src/__tests__/recommendations-permissions.test.ts b/frontend/src/__tests__/recommendations-permissions.test.ts index fab581dcb..3f1a95636 100644 --- a/frontend/src/__tests__/recommendations-permissions.test.ts +++ b/frontend/src/__tests__/recommendations-permissions.test.ts @@ -204,7 +204,13 @@ describe('Recommendations action-box permission gating (issue #365)', () => { const actionBox = document.getElementById('recommendations-action-box')!; const banners = actionBox.querySelectorAll('.info-banner'); expect(banners.length).toBe(1); - expect(banners[0]?.textContent).toContain('You can view but not execute purchases'); + expect(banners[0]?.textContent).toContain('not execute purchases directly'); + // Row 550: must not imply the user can't plan, and must use the real nav + // name ("Admin", not "Settings"), and must not claim non-admins can + // self-serve in Admin → Users. + expect(banners[0]?.textContent).toContain('Admin → Users'); + expect(banners[0]?.textContent).not.toContain('Settings → Users'); + expect(banners[0]?.textContent).toContain('view and plan'); }); test('custom (non-seeded) group with explicit execute:purchases shows Purchase + no banner (CR #924 F3/F4)', async () => { diff --git a/frontend/src/__tests__/topbar-filters.test.ts b/frontend/src/__tests__/topbar-filters.test.ts index 64bdf319d..724452e1c 100644 --- a/frontend/src/__tests__/topbar-filters.test.ts +++ b/frontend/src/__tests__/topbar-filters.test.ts @@ -14,7 +14,9 @@ */ jest.mock('../api', () => ({ - listAccounts: jest.fn(), + // #949/#951: the dropdown now uses the minimal-disclosure endpoint + // (view:recommendations) so it populates for Standard / Read-Only users. + listAccountsMinimal: jest.fn(), })); import * as api from '../api'; @@ -34,7 +36,7 @@ describe('Topbar filters', () => { setupSlot(); state.setCurrentProvider(''); state.setCurrentAccountIDs([]); - (api.listAccounts as jest.Mock).mockResolvedValue([]); + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([]); }); afterEach(() => { @@ -51,6 +53,31 @@ describe('Topbar filters', () => { expect(chips?.length).toBe(2); }); + // #951: a Standard / Read-Only user (who can call listAccountsMinimal but + // would 403 on the full listAccounts) gets a populated Account dropdown. + // The mock stands in for the minimal endpoint succeeding for such a user. + test('populates the account dropdown from the minimal endpoint (Standard user, #951)', async () => { + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([ + { id: 'acct-1', name: 'Prod', external_id: '111', provider: 'aws' }, + { id: 'acct-2', name: 'Dev', external_id: '222', provider: 'aws' }, + ]); + + initTopbarFilters(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.listAccountsMinimal).toHaveBeenCalled(); + + // Open the account chip (second trigger) and assert the real accounts show + // up beyond the seed "All Accounts" option. + const triggers = document.querySelectorAll('.chip-select'); + (triggers[1] as HTMLButtonElement).click(); + const optionValues = Array.from( + document.querySelectorAll('.chip-select-option'), + ).map((el) => el.dataset['value']); + expect(optionValues).toContain('acct-1'); + expect(optionValues).toContain('acct-2'); + }); + // Issue #477: URL hydration + writeback tests. // // jsdom honours history.replaceState() — setting it before initTopbarFilters @@ -84,7 +111,7 @@ describe('Topbar filters', () => { test('account-chip change writes back to URL query params', async () => { window.history.replaceState({}, '', '/opportunities'); - (api.listAccounts as jest.Mock).mockResolvedValue([ + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([ { id: 'acct-99', name: 'prod', external_id: '999' }, ]); @@ -111,7 +138,7 @@ describe('Topbar filters', () => { test('selecting "All Accounts" removes the account query param', async () => { // Start with an account already selected via URL. window.history.replaceState({}, '', '/opportunities?account=acct-7'); - (api.listAccounts as jest.Mock).mockResolvedValue([ + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([ { id: 'acct-7', name: 'old', external_id: '7' }, ]); @@ -167,7 +194,7 @@ describe('Topbar filters', () => { const firstBlocker = new Promise((r) => { resolveFirst = r; }); let resolveSecond: ((value: unknown[]) => void) | undefined; const secondBlocker = new Promise((r) => { resolveSecond = r; }); - (api.listAccounts as jest.Mock) + (api.listAccountsMinimal as jest.Mock) .mockReturnValueOnce(firstBlocker) .mockReturnValueOnce(secondBlocker); diff --git a/frontend/src/api/accounts.ts b/frontend/src/api/accounts.ts index bee2bfdcc..c07318e99 100644 --- a/frontend/src/api/accounts.ts +++ b/frontend/src/api/accounts.ts @@ -106,6 +106,35 @@ export interface AccountServiceOverrideRequest { exclude_types?: string[]; } +/** + * Minimal-disclosure account projection returned by GET /api/accounts/list. + * Carries only the fields the global topbar filter and the create-plan target + * prefill need; it deliberately omits every credential/config field so the + * endpoint can be served to Standard / Read-Only users (gated on + * view:recommendations rather than the admin-grade view:accounts). See issues + * #949 / #951. + */ +export interface AccountSummary { + id: string; + name: string; + external_id: string; + provider: Provider; +} + +/** + * Fetch the minimal account list available to any user who can view + * recommendations. Used by the global filter dropdown and the plan modal so + * non-admin users get a populated account list without the 403 they'd hit on + * the full GET /api/accounts endpoint. + */ +export async function listAccountsMinimal(filters?: AccountListFilters): Promise { + const params = new URLSearchParams(); + if (filters?.provider) params.set('provider', filters.provider); + if (filters?.search) params.set('search', filters.search); + const qs = params.toString(); + return apiRequest(`/accounts/list${qs ? `?${qs}` : ''}`); +} + export async function listAccounts(filters?: AccountListFilters): Promise { const params = new URLSearchParams(); if (filters?.provider) params.set('provider', filters.provider); diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index 4aa14e779..7a99645c9 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -210,6 +210,7 @@ export { // Re-export accounts functions and types export type { CloudAccount, + AccountSummary, CloudAccountRequest, AccountListFilters, AccountCredentialsRequest, @@ -222,6 +223,7 @@ export type { export { listAccounts, + listAccountsMinimal, createAccount, createSelfAccount, getAccount, diff --git a/frontend/src/history.ts b/frontend/src/history.ts index c0a204d80..d18024368 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -609,8 +609,8 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { banner.className = 'info-banner'; banner.setAttribute('role', 'note'); banner.textContent = - 'You can view but not execute purchases. ' + - 'Ask an admin to add you to the Purchaser group, or add yourself in Settings → Users.'; + 'You can view and plan, but not execute purchases directly. ' + + 'Ask an admin to add you to the Purchaser group (Admin → Users) to execute purchases.'; container.parentElement?.insertBefore(banner, container); } else if (existingBanner && hasAnyCarvedOut) { existingBanner.remove(); diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index f3a3676c0..8834ad6cb 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -1269,7 +1269,10 @@ async function handlePlanAccountSearch(value: string): Promise { try { const providerSelect = document.getElementById('plan-provider') as HTMLSelectElement | null; const provider = providerSelect?.value as api.Provider | undefined; - const accounts = await api.listAccounts({ search: value, ...(provider ? { provider } : {}) }); + // Minimal-disclosure list (view:recommendations) so plan-account search + // works for Standard / Read-Only users; the full view:accounts list 403s + // for them. See issues #949/#951. + const accounts = await api.listAccountsMinimal({ search: value, ...(provider ? { provider } : {}) }); suggestions.textContent = ''; if (accounts.length === 0) { suggestions.classList.add('hidden'); @@ -1389,7 +1392,12 @@ function prefillPurchaseConfigFromCommitment(rec: api.Recommendation): void { */ async function prefillAccountChipFromId(accountId: string, session: number): Promise { try { - const account = await api.getAccount(accountId); + // Resolve via the minimal-disclosure list (view:recommendations) instead of + // GET /api/accounts/:id (view:accounts) so the target prefills for + // Standard / Read-Only users too — the per-id endpoint 403s for them, which + // previously left the target empty and made Save Plan silently no-op once + // the empty-target guard rejected submit. See issues #949/#951. + const account = (await api.listAccountsMinimal()).find((a) => a.id === accountId); // Session guard: discard the result if the modal was closed and reopened // while this promise was in-flight. planModalSession is incremented on // each new modal open, so a mismatch means this callback is stale. diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 51701491f..6955a7ac1 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3467,9 +3467,14 @@ function mountBottomActionBox(): HTMLElement | null { const noPurchaseBanner = document.createElement('div'); noPurchaseBanner.className = 'info-banner'; noPurchaseBanner.setAttribute('role', 'note'); + // Scope the message to the execute-purchases capability only (row 550): + // Standard users CAN create plans, so the banner must not imply otherwise. + // The nav tab is "Admin" (renamed from Settings in #340), and Admin → Users + // is admin-only, so a non-admin can't self-serve there — only an admin can + // add them to the Purchaser group. noPurchaseBanner.textContent = - 'You can view but not execute purchases. ' + - 'Ask an admin to add you to the Purchaser group, or add yourself in Settings → Users.'; + 'You can view and plan, but not execute purchases directly. ' + + 'Ask an admin to add you to the Purchaser group (Admin → Users) to execute purchases.'; box.appendChild(noPurchaseBanner); } diff --git a/frontend/src/topbar-filters.ts b/frontend/src/topbar-filters.ts index 1457ba2da..1baa04d8a 100644 --- a/frontend/src/topbar-filters.ts +++ b/frontend/src/topbar-filters.ts @@ -95,7 +95,10 @@ async function populateAccountOptions(provider: string): Promise { provider && provider !== '' && provider !== 'all' ? { provider: provider as 'aws' | 'azure' | 'gcp' } : undefined; - const accounts = await api.listAccounts(filter); + // Use the minimal-disclosure endpoint (view:recommendations) so the + // dropdown populates for Standard / Read-Only users too — the full + // GET /api/accounts (view:accounts) 403s for them. See issues #949/#951. + const accounts = await api.listAccountsMinimal(filter); // Discard if a newer request has already started. if (gen !== _accountRequestGen) return; const options: ChipSelectOption[] = [ diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 78f3a6313..b23802413 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -127,6 +127,67 @@ func (h *Handler) listAccounts(ctx context.Context, req *events.LambdaFunctionUR return accounts, nil } +// AccountSummary is the minimal-disclosure projection of a cloud account used +// by the global topbar filter and the create-plan-from-commitment target +// prefill (issues #949, #951). It deliberately omits every credential/config +// field (role ARNs, subscription IDs, client emails, auth modes, bastion IDs, +// the self-account marker) so the endpoint can be gated on a low read verb that +// Standard / Read-Only users already hold, without leaking sensitive account +// configuration. The full CloudAccount object stays behind GET /api/accounts +// (view:accounts, admin-grade). +type AccountSummary struct { + ID string `json:"id"` + Name string `json:"name"` + ExternalID string `json:"external_id"` + Provider string `json:"provider"` +} + +// listAccountsMinimal handles GET /api/accounts/list. +// +// Returns the minimal AccountSummary projection scoped by the session's +// allowed_accounts list. Gated on view:recommendations (held by both the +// Standard Users and Read-Only Users groups) rather than view:accounts, so the +// account dropdown in the global filter and the plan-target prefill work for +// non-admin users without exposing the credential/config metadata carried by +// the full GET /api/accounts response. See issues #949 / #951. +func (h *Handler) listAccountsMinimal(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + session, err := h.requirePermission(ctx, req, "view", "recommendations") + if err != nil { + return nil, err + } + + filter := buildAccountFilter(req.QueryStringParameters) + + accounts, err := h.config.ListCloudAccounts(ctx, filter) + if err != nil { + return nil, fmt.Errorf("accounts: %w", err) + } + + allowedAccounts, err := h.getAllowedAccounts(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + unrestricted := auth.IsUnrestrictedAccess(allowedAccounts) + + // Build the minimal projection in place, applying allowed_accounts scoping + // during the copy so a restricted user only ever sees their entitled rows. + summaries := make([]AccountSummary, 0, len(accounts)) + for i := range accounts { + acct := &accounts[i] + if !unrestricted && !auth.MatchesAccount(allowedAccounts, acct.ID, acct.Name) { + continue + } + summaries = append(summaries, AccountSummary{ + ID: acct.ID, + Name: acct.Name, + ExternalID: acct.ExternalID, + Provider: acct.Provider, + }) + } + + return summaries, nil +} + // markSelfAccount sets IsSelf=true on the account matching the source identity. func (h *Handler) markSelfAccount(ctx context.Context, accounts []config.CloudAccount) { si := h.resolveSourceIdentity(ctx) diff --git a/internal/api/handler_accounts_router_test.go b/internal/api/handler_accounts_router_test.go index 749a05c3c..380723f31 100644 --- a/internal/api/handler_accounts_router_test.go +++ b/internal/api/handler_accounts_router_test.go @@ -89,6 +89,21 @@ func TestRouterDispatch_DeleteAccount_RoutesCorrectly(t *testing.T) { require.NoError(t, err) } +// TestRouterDispatch_AccountsList_RoutesToMinimalHandler verifies that +// GET /api/accounts/list reaches listAccountsMinimal and is NOT swallowed by +// the generic "/api/accounts/" GET prefix route (getAccount), which would treat +// "list" as a :id and reject it with a 400 invalid-UUID error. A successful +// (non-error) result proves the more-specific exact-path route won. (#949/#951) +func TestRouterDispatch_AccountsList_RoutesToMinimalHandler(t *testing.T) { + ctx := context.Background() + r := setupRouterForDispatch(ctx) + + req, method, path := routerReq("GET", "/api/accounts/list", "") + result, err := r.Route(ctx, method, path, req) + require.NoError(t, err, "expected listAccountsMinimal to handle /api/accounts/list, not getAccount's UUID validation") + assert.NotNil(t, result) +} + // ── Plan ↔ Account association routes ─────────────────────────────────────── // TestRouterDispatch_PlanAccountsRoute_RequiresCorrectOrdering verifies that diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index 5bab7ad12..3561f65b0 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -2,6 +2,7 @@ package api import ( "context" + "encoding/json" "errors" "fmt" "testing" @@ -97,6 +98,155 @@ func TestListAccounts_ReturnsEmptySlice(t *testing.T) { assert.Len(t, got, 0) } +// --- listAccountsMinimal (GET /api/accounts/list) --- +// +// Authz contract for issues #949/#951: the minimal endpoint is gated on +// view:recommendations (held by Standard / Read-Only users) NOT view:accounts, +// so a Standard user can populate the global filter dropdown + plan-target +// prefill. The response must carry only id/name/external_id/provider — never the +// credential/config metadata the full GET /api/accounts response carries. + +// standardUserSession models a Standard-group user (view:recommendations, no +// view:accounts). +func standardUserSession() *Session { + return &Session{ + UserID: "cccccccc-cccc-cccc-cccc-cccccccccccc", + Email: "standard@example.com", + } +} + +// setupStandardUserAuth stubs ValidateSession + a single HasPermissionAPI verb +// for the Standard user. It deliberately does NOT grant view:accounts so a test +// can assert the full listAccounts handler 403s while the minimal one succeeds. +func setupStandardUserAuth(ctx context.Context, mockAuth *MockAuthService, verb, resource string, allow bool, allowed []string) { + session := standardUserSession() + mockAuth.On("ValidateSession", ctx, "standard-token").Return(session, nil) + mockAuth.On("HasPermissionAPI", ctx, session.UserID, verb, resource).Return(allow, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, session.UserID).Return(allowed, nil).Maybe() +} + +func standardRequest(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer standard-token"}, + Body: body, + } +} + +// A Standard user holding view:recommendations gets the minimal list, and the +// projection contains none of the sensitive credential/config fields. +func TestListAccountsMinimal_StandardUserAllowed_NoSensitiveFields(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupStandardUserAuth(ctx, mockAuth, "view", "recommendations", true, []string{"*"}) + + full := sampleAccount() + full.AWSAuthMode = "role_arn" + full.AWSRoleARN = "arn:aws:iam::123456789012:role/CUDly" + full.AWSExternalID = "super-secret-external-id-1234" + full.AzureSubscriptionID = "11111111-2222-3333-4444-555555555555" + full.GCPClientEmail = "svc@project.iam.gserviceaccount.com" + customStore := &mockConfigStoreAccounts{ + MockConfigStore: setupAdminMock(ctx), + listResult: []config.CloudAccount{full}, + } + + handler := &Handler{auth: mockAuth, config: customStore} + result, err := handler.listAccountsMinimal(ctx, standardRequest("")) + require.NoError(t, err) + + got := result.([]AccountSummary) + require.Len(t, got, 1) + assert.Equal(t, full.ID, got[0].ID) + assert.Equal(t, full.Name, got[0].Name) + assert.Equal(t, full.ExternalID, got[0].ExternalID) + assert.Equal(t, full.Provider, got[0].Provider) + + // Defence-in-depth: the JSON-serialized summary must not leak any sensitive + // field, even by accident (e.g. a future struct-embedding refactor). + blob, marshalErr := json.Marshal(got) + require.NoError(t, marshalErr) + for _, secret := range []string{ + "role_arn", "aws_external_id", "azure_subscription_id", + "gcp_client_email", "aws_auth_mode", "credentials_configured", + "CUDly", "super-secret-external-id-1234", + } { + assert.NotContains(t, string(blob), secret, "minimal projection must not leak %q", secret) + } +} + +// The full GET /api/accounts (view:accounts) 403s for the same Standard user — +// proving the minimal endpoint is the correct least-privilege path, not a +// redundant copy of an already-reachable one. +func TestListAccounts_StandardUserDenied_403(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupStandardUserAuth(ctx, mockAuth, "view", "accounts", false, nil) + + handler := &Handler{auth: mockAuth, config: setupAdminMock(ctx)} + result, err := handler.listAccounts(ctx, standardRequest("")) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) +} + +// A Standard user lacking view:recommendations is denied the minimal endpoint +// too (it is gated, not public). +func TestListAccountsMinimal_NoViewRecommendations_403(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupStandardUserAuth(ctx, mockAuth, "view", "recommendations", false, nil) + + handler := &Handler{auth: mockAuth, config: setupAdminMock(ctx)} + result, err := handler.listAccountsMinimal(ctx, standardRequest("")) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) +} + +// allowed_accounts scoping applies to the minimal endpoint: a restricted user +// only sees their entitled rows. +func TestListAccountsMinimal_ScopesByAllowedAccounts(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupStandardUserAuth(ctx, mockAuth, "view", "recommendations", true, []string{"Production"}) + + prod := sampleAccount() + prod.Name = "Production" + staging := sampleAccount() + staging.ID = outOfScopeID + staging.Name = "Staging" + customStore := &mockConfigStoreAccounts{ + MockConfigStore: setupAdminMock(ctx), + listResult: []config.CloudAccount{prod, staging}, + } + + handler := &Handler{auth: mockAuth, config: customStore} + result, err := handler.listAccountsMinimal(ctx, standardRequest("")) + require.NoError(t, err) + got := result.([]AccountSummary) + require.Len(t, got, 1) + assert.Equal(t, "Production", got[0].Name) +} + +// Empty store returns a non-nil empty slice (JSON `[]`, not `null`) so the +// frontend's `.map` is always safe. +func TestListAccountsMinimal_ReturnsEmptySlice(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupStandardUserAuth(ctx, mockAuth, "view", "recommendations", true, []string{"*"}) + + handler := &Handler{auth: mockAuth, config: setupAdminMock(ctx)} + result, err := handler.listAccountsMinimal(ctx, standardRequest("")) + require.NoError(t, err) + got := result.([]AccountSummary) + assert.NotNil(t, got) + assert.Len(t, got, 0) +} + // --- createAccount --- func TestCreateAccount_Success(t *testing.T) { diff --git a/internal/api/router.go b/internal/api/router.go index e5a3e700e..d0f27e82c 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -241,6 +241,12 @@ func (r *Router) registerRoutes() { // response shape, not gated at the route layer. {ExactPath: "/api/accounts/discover-org", Method: "POST", Handler: r.discoverOrgAccountsHandler, Auth: AuthAdmin}, {ExactPath: "/api/accounts/self", Method: "POST", Handler: r.createSelfAccountHandler, Auth: AuthAdmin}, + // Minimal-disclosure accounts list for the global filter + plan-target + // prefill (issues #949/#951). Gated in-handler on view:recommendations so + // Standard / Read-Only users can populate the dropdown; the full account + // 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}, {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}, @@ -774,6 +780,10 @@ func (r *Router) listAccountsHandler(ctx context.Context, req *events.LambdaFunc return r.h.listAccounts(ctx, req) } +func (r *Router) listAccountsMinimalHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return r.h.listAccountsMinimal(ctx, req) +} + func (r *Router) createAccountHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { return r.h.createAccount(ctx, req) }