Skip to content

fix(accounts): least-privilege accounts list for standard-user filter + plan prefill (closes #949, closes #951) #987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
Jun 5, 2026
Merged

fix(accounts): least-privilege accounts list for standard-user filter + plan prefill (closes #949, closes #951) #987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions frontend/src/__tests__/allowed-accounts.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}));
Expand Down Expand Up @@ -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' },
]);
Expand All @@ -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' },
]);

Expand All @@ -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));
Expand Down Expand Up @@ -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 () => {
Expand Down
33 changes: 33 additions & 0 deletions frontend/src/__tests__/api-accounts.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import {
listAccounts,
listAccountsMinimal,
createAccount,
getAccount,
updateAccount,
Expand Down Expand Up @@ -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);
Expand Down
97 changes: 65 additions & 32 deletions frontend/src/__tests__/plans.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}));

Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -1387,28 +1402,31 @@ 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]);

// Wait for the async prefillAccountChipFromId to settle
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]);

Expand All @@ -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<api.CloudAccount>(resolve => {
let resolveFirstCall!: (value: Array<{ id: string; name: string; external_id: string; provider: string }>) => void;
const firstCallPromise = new Promise<Array<{ id: string; name: string; external_id: string; provider: string }>>(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]);
Expand All @@ -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();

Expand All @@ -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');
});
Expand All @@ -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('');
});
Expand Down Expand Up @@ -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
Expand All @@ -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();
Expand All @@ -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' })
);
});
Expand Down
8 changes: 7 additions & 1 deletion frontend/src/__tests__/recommendations-permissions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
Loading
Loading