From 5463a37ffea41c15a091812293c9acb8de4e78a8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:11:22 +0200 Subject: [PATCH 01/16] fix(users): disable actions on the synthetic current-user row Edit, Delete, selection and group toggles on the placeholder row hit /api/users/current. Disable them, skip the row in select-all, and exclude it from the MFA stat. Closes #274 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/users.test.ts | 36 ++++++++++++++++++++++++++++ frontend/src/users/userList.ts | 25 +++++++++++-------- 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index cf77085cb..a33f91387 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -3184,3 +3184,39 @@ describe('contradictory group combination validation (issue #1405)', () => { }); }); }); + +describe('synthetic current-user row (issue #274)', () => { + const real = { id: '1', email: 'a@test.com', groups: [], mfa_enabled: true }; + const synthetic = { id: 'current', email: 'me@test.com', groups: [], mfa_enabled: false }; + + beforeEach(() => { + document.body.innerHTML = '
'; + userState.setAllUsers([synthetic, real] as any); + userState.setFilteredUsers([synthetic, real] as any); + userState.clearSelectedUserIds(); + }); + + it('disables select, Edit and Delete on the synthetic row only', () => { + userList.renderUsers([synthetic, real] as any); + const row = (id: string) => document.querySelector(`tr.user-row[data-user-id="${id}"]`)!; + expect(row('current').querySelector('.user-checkbox')!.disabled).toBe(true); + expect(row('current').querySelector('.edit-user-btn')!.disabled).toBe(true); + expect(row('current').querySelector('.delete-user-btn')!.disabled).toBe(true); + expect(row('1').querySelector('.edit-user-btn')!.disabled).toBe(false); + }); + + it('select-all skips the synthetic row', () => { + userList.renderUsers([synthetic, real] as any); + const all = document.getElementById('select-all-users') as HTMLInputElement; + all.checked = true; + all.dispatchEvent(new Event('change')); + expect([...userState.selectedUserIds]).toEqual(['1']); + }); + + it('excludes the synthetic row from the MFA stat', () => { + userState.setAllUsers([{ ...synthetic, mfa_enabled: true }, { ...real, mfa_enabled: false }] as any); + userList.renderUserStats(); + const card = [...document.querySelectorAll('.stat-card')].find(c => c.textContent!.includes('MFA Enabled'))!; + expect(card.querySelector('.stat-value')!.textContent).toBe('0'); + }); +}); diff --git a/frontend/src/users/userList.ts b/frontend/src/users/userList.ts index 92261378f..d8976581f 100644 --- a/frontend/src/users/userList.ts +++ b/frontend/src/users/userList.ts @@ -18,6 +18,9 @@ import { escapeHtml, formatRelativeTime, formatDate, showSuccess, showError, val import { openEditUserModal, deleteUser } from './userActions'; import { ADMINISTRATORS_GROUP_ID } from '../permissions'; +// Placeholder row id withCurrentUser prepends when the API omits the caller. +const SYNTHETIC_USER_ID = 'current'; + /** * Single AbortController for the document-level group-assign-checkbox * change listener. Replaced on every renderUsers() call so only one @@ -37,7 +40,7 @@ export function renderUserStats(): void { const adminUsers = allUsers.filter(u => Array.isArray(u.groups) && u.groups.includes(ADMINISTRATORS_GROUP_ID) ).length; - const mfaEnabled = allUsers.filter(u => u.mfa_enabled).length; + const mfaEnabled = allUsers.filter(u => u.id !== SYNTHETIC_USER_ID && u.mfa_enabled).length; const showing = filteredUsers.length; statsContainer.innerHTML = ` @@ -74,12 +77,13 @@ export function renderUsers(users: APIUser[]): void { return; } + const selectableCount = users.filter(u => u.id !== SYNTHETIC_USER_ID).length; const table = ` @@ -94,7 +98,7 @@ export function renderUsers(users: APIUser[]): void { ${users.map(user => ` @@ -191,7 +195,7 @@ function renderUserExpandPanel(user: APIUser): string { + ${userGroups.has(g.id) ? 'checked' : ''} ${user.id === SYNTHETIC_USER_ID ? 'disabled' : ''}> ${escapeHtml(g.name)} `).join('')} @@ -301,13 +305,13 @@ function setupUserTableListeners(): void { selectAllCheckbox.addEventListener('change', (e) => { const checked = (e.target as HTMLInputElement).checked; if (checked) { - filteredUsers.forEach(user => addSelectedUserId(user.id)); + filteredUsers.filter(u => u.id !== SYNTHETIC_USER_ID).forEach(user => addSelectedUserId(user.id)); } else { clearSelectedUserIds(); } // Patch each row's selected class and its individual checkbox without // re-rendering the whole table (preserves open expand panels). - filteredUsers.forEach(user => { + filteredUsers.filter(u => u.id !== SYNTHETIC_USER_ID).forEach(user => { const row = document.querySelector( `tr.user-row[data-user-id="${user.id}"]`, ); @@ -352,7 +356,8 @@ function setupUserTableListeners(): void { const selectAll = document.getElementById('select-all-users') as HTMLInputElement | null; if (selectAll) { selectAll.checked = - selectedUserIds.size > 0 && selectedUserIds.size === filteredUsers.length; + selectedUserIds.size > 0 && + selectedUserIds.size === filteredUsers.filter(u => u.id !== SYNTHETIC_USER_ID).length; } updateBulkActionsBar(); From cea1de9e1d5fbf675517a5ef314d68f351123a6d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:12:56 +0200 Subject: [PATCH 02/16] fix(a11y): name users and recommendations table checkboxes Add aria-labels: select-all and per-user labels in the users table, and service, type, region, account, term and payment in the recommendations rows so each checkbox is distinguishable. Closes #746 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/recommendations.test.ts | 24 +++++++++++++++++++ frontend/src/__tests__/users.test.ts | 14 +++++++++++ frontend/src/recommendations.ts | 5 +++- frontend/src/users/userList.ts | 4 ++-- frontend/tests-e2e/recommendations.spec.ts | 2 +- 5 files changed, 45 insertions(+), 4 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index ebf066a7f..551765614 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -533,6 +533,30 @@ describe('Recommendations Module', () => { expect(cardValue(/Recommendations/)).toBe('1'); }); + test('row checkboxes carry distinct accessible names (issue #746)', async () => { + const recs = [ + { id: 'rec-a', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment: 'no-upfront', savings: 150, upfront_cost: 1000 }, + { id: 'rec-b', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'eu-west-1', count: 1, term: 3, payment: 'all-upfront', savings: 200, upfront_cost: 0 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: { total_count: 2, total_monthly_savings: 350, total_upfront_cost: 1000, avg_payback_months: 1 }, + recommendations: recs, + regions: [], + }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + await loadRecommendations(); + + const label = (id: string) => + document.querySelector(`input[data-rec-id="${id}"]`)?.getAttribute('aria-label') ?? ''; + expect(label('rec-a')).toContain('ec2'); + expect(label('rec-a')).toContain('t3.medium'); + expect(label('rec-a')).toContain('us-east-1'); + expect(label('rec-b')).toContain('rds'); + expect(label('rec-a')).not.toBe(label('rec-b')); + }); + test('select-all change event updates summary cards in place (PR #283 CR pass-2)', async () => { // Verifies the select-all checkbox event path: after checking select-all, // summary cards reflect the picked-best-per-cell selection WITHOUT a diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index a33f91387..6c3674c76 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -3220,3 +3220,17 @@ describe('synthetic current-user row (issue #274)', () => { expect(card.querySelector('.stat-value')!.textContent).toBe('0'); }); }); + +describe('users table checkbox names (issue #746)', () => { + it('labels select-all and each row checkbox with the user email', () => { + document.body.innerHTML = '
'; + const users = [ + { id: 'u1', email: 'a@test.com', groups: [], mfa_enabled: false }, + { id: 'u2', email: 'b@test.com', groups: [], mfa_enabled: false }, + ]; + userList.renderUsers(users as any); + expect(document.getElementById('select-all-users')!.getAttribute('aria-label')).toBe('Select all users'); + const labels = [...document.querySelectorAll('.user-checkbox')].map(c => c.getAttribute('aria-label')); + expect(labels).toEqual(['Select a@test.com', 'Select b@test.com']); + }); +}); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 6f44b1be9..5a554762c 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3194,8 +3194,11 @@ function buildVariantRowMarkup( const planBtnHtml = canAccess('create', 'plans') ? `` : ''; + const selectLabel = escapeHtml( + `Select ${rec.service} ${rec.resource_type} ${rec.region} ${accountName} ${formatTerm(rec.term)}`, + ); const checkboxCell = showCheckboxes - ? `` + ? `` : ``; return ` diff --git a/frontend/src/users/userList.ts b/frontend/src/users/userList.ts index d8976581f..6a8c4f218 100644 --- a/frontend/src/users/userList.ts +++ b/frontend/src/users/userList.ts @@ -83,7 +83,7 @@ export function renderUsers(users: APIUser[]): void { @@ -98,7 +98,7 @@ export function renderUsers(users: APIUser[]): void { ${users.map(user => ` ` From 44b8de6e59dacf0f5e8b82ee8213d48d95963cfd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:59:14 +0200 Subject: [PATCH 11/16] fix(users): key self-demotion on user id and save the post-confirm group set Compare the session user id instead of the email, and recompute the group set after the confirm dialog resolves so a concurrent toggle is not reverted. Refs #736 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/users.test.ts | 19 +++++++++++++++++++ frontend/src/users/userList.ts | 17 +++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index 2a652169a..6e553ec6b 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -3287,6 +3287,25 @@ describe('group toggle confirmation (issue #736)', () => { destructive: true, }); }); + + it('detects self-demotion by user id, not email', async () => { + require('../state').setCurrentUser({ id: '1', email: 'ME@test.com', groups: [ADMIN] }); + await toggle('1', false); + expect(mockConfirmDialog.mock.calls[0]![0]).toMatchObject({ body: expect.stringContaining('removing yourself') }); + }); + + it('saves against the groups current after the confirm resolves', async () => { + let release!: (v: boolean) => void; + mockConfirmDialog.mockReturnValueOnce(new Promise((r) => { release = r; })); + document.querySelector('.user-expand-btn[data-user-id="2"]')!.click(); + const cb = document.querySelector('.group-assign-checkbox[data-user-id="2"]')!; + cb.checked = false; + cb.dispatchEvent(new Event('change', { bubbles: true })); + userState.setAllUsers(users.map(u => (u.id === '2' ? { ...u, groups: [ADMIN, 'g-new'] } : u)) as any); + release(true); + for (let i = 0; i < 5; i++) await Promise.resolve(); + expect(api.updateUser).toHaveBeenCalledWith('2', { groups: ['g-new'] }); + }); }); describe('saveUser submit button (issue #735)', () => { diff --git a/frontend/src/users/userList.ts b/frontend/src/users/userList.ts index e27a6c765..6d05685d2 100644 --- a/frontend/src/users/userList.ts +++ b/frontend/src/users/userList.ts @@ -231,9 +231,10 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean const group = availableGroups.find(g => g.id === groupId); const groupLabel = group ? group.name : groupId; - const next = checked - ? [...new Set([...user.groups, groupId])] - : user.groups.filter(g => g !== groupId); + const nextFor = (u: APIUser): string[] => (checked + ? [...new Set([...u.groups, groupId])] + : u.groups.filter(g => g !== groupId)); + const next = nextFor(user); // Validate the resulting group combination before hitting the API. // A view-only group (all permissions are view:*) combined with a @@ -251,7 +252,7 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean const selfDemotion = !checked && groupId === ADMINISTRATORS_GROUP_ID - && user.email === getCurrentUser()?.email; + && user.id === getCurrentUser()?.id; const ok = await confirmDialog({ title: checked ? `Add ${user.email} to "${groupLabel}"?` @@ -272,12 +273,16 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean return; } + // A concurrent toggle may have changed the user while the dialog was open. + const latest = allUsers.find(u => u.id === userId) ?? user; + const groupsToSave = nextFor(latest); + try { - await api.updateUser(userId, { groups: next }); + await api.updateUser(userId, { groups: groupsToSave }); // Patch the user in-memory so subsequent renders and effectivePermissions // calculations reflect the change without a full round-trip re-render. - const updatedUser: APIUser = { ...user, groups: next }; + const updatedUser: APIUser = { ...latest, groups: groupsToSave }; setAllUsers(allUsers.map(u => (u.id === userId ? updatedUser : u))); // Re-render the expand panel in place so the checkboxes and permissions From 3eeb90674f5d48736c9ec3965f0acf2de48fc096 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 23:52:10 +0200 Subject: [PATCH 12/16] test(e2e): select recommendation rows by their descriptive checkbox names The row checkbox aria-label became "Select ..." in #746, so the exact name "Select recommendation" no longer matches. Locate it with /^Select / as recommendations.spec.ts already does; the locators are scoped to a single row. Refs #746 Co-Authored-By: Claude Opus 5.5 --- frontend/tests-e2e/execute-now-warning.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/tests-e2e/execute-now-warning.spec.ts b/frontend/tests-e2e/execute-now-warning.spec.ts index 2a0f81c96..537a977b2 100644 --- a/frontend/tests-e2e/execute-now-warning.spec.ts +++ b/frontend/tests-e2e/execute-now-warning.spec.ts @@ -21,7 +21,7 @@ for (const sample of [ await page.goto('/opportunities'); const selected = page.locator(`tr.recommendation-row[data-rec-id="${row.id}"]`); await expect(selected).toBeVisible(); - await selected.getByRole('checkbox', { name: 'Select recommendation' }).check(); + await selected.getByRole('checkbox', { name: /^Select / }).check(); await page.locator('#bulk-purchase-btn').click(); await expect(page.locator('#purchase-details')).toContainText(row.resource_type); const warning = page.locator('#purchase-details .direct-execute-warning'); @@ -65,7 +65,7 @@ test('mixed providers use fan-out without cancellation reassurance', async ({ pa await page.goto('/opportunities'); for (const id of ['r01', 'r12']) { await page.locator(`tr.recommendation-row[data-rec-id="${id}"]`) - .getByRole('checkbox', { name: 'Select recommendation' }).check(); + .getByRole('checkbox', { name: /^Select / }).check(); } await page.locator('#bulk-purchase-btn').click(); await expect(page.locator('#fanout-summary')).toBeVisible(); From 4dd0aeb202db6cc5fa62202eab6cd9111240b685 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:01:57 +0200 Subject: [PATCH 13/16] fix(recommendations): include engine and clean payment in row checkbox names Build the label from non-empty parts, show resource_type (engine) and the same payment text as the table cell, so engine-only variants stay distinct. Refs #746 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/recommendations.test.ts | 29 +++++++++++++++++++ frontend/src/recommendations.ts | 10 ++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 3a41043fe..6904a91c4 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -581,6 +581,35 @@ describe('Recommendations Module', () => { expect(label('rec-np')).not.toBe(label('rec-au')); }); + test('engine-only and unmapped-payment rows get distinct clean checkbox names (issue #746)', async () => { + const base = { provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.r5.large', region: 'us-east-1', count: 1, term: 1, savings: 150, upfront_cost: 0 }; + const recs = [ + { ...base, id: 'rec-my', engine: 'mysql', payment: 'no-upfront' }, + { ...base, id: 'rec-pg', engine: 'postgresql', payment: 'no-upfront' }, + { ...base, id: 'rec-x', payment: 'weird-option' }, + { ...base, id: 'rec-none', resource_type: 'db.r5.xlarge' }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: { total_count: 4, total_monthly_savings: 600, total_upfront_cost: 0, avg_payback_months: 1 }, + recommendations: recs, + regions: [], + }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + await loadRecommendations(); + document.querySelectorAll('.rec-cell-chevron').forEach(b => b.click()); + + const label = (id: string) => + document.querySelector(`input[data-rec-id="${id}"]`)?.getAttribute('aria-label') ?? ''; + expect(label('rec-my')).toContain('db.r5.large (mysql)'); + expect(label('rec-my')).not.toBe(label('rec-pg')); + expect(label('rec-x')).toContain('weird-option'); + for (const id of ['rec-my', 'rec-pg', 'rec-x', 'rec-none']) { + expect(label(id)).not.toMatch(/\s{2,}|\s$/); + } + }); + test('select-all change event updates summary cards in place (PR #283 CR pass-2)', async () => { // Verifies the select-all checkbox event path: after checking select-all, // summary cards reflect the picked-best-per-cell selection WITHOUT a diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 03a7c93be..f868eee7a 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3195,7 +3195,15 @@ function buildVariantRowMarkup( ? `` : ''; const selectLabel = escapeHtml( - `Select ${rec.service} ${rec.resource_type} ${rec.region} ${accountName} ${formatTerm(rec.term)} ${PAYMENT_DISPLAY_LABELS[rec.payment ?? ''] ?? ''}`, + [ + 'Select', + rec.service, + rec.engine ? `${rec.resource_type} (${rec.engine})` : rec.resource_type, + rec.region, + accountName, + formatTerm(rec.term), + rec.payment ? formatPayment(rec.payment) : '', + ].filter(part => part).join(' '), ); const checkboxCell = showCheckboxes ? `` From 60bb028f61209d75d6ee53af4e0a2821b4d709ad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:02:09 +0200 Subject: [PATCH 14/16] fix(users): re-validate group combination after the confirm dialog Refs #736 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/users.test.ts | 35 ++++++++++++++++++++++++++++ frontend/src/users/userList.ts | 23 +++++++++++------- 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index 6e553ec6b..084956523 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -3308,6 +3308,41 @@ describe('group toggle confirmation (issue #736)', () => { }); }); +describe('group toggle re-validation after confirm (issue #736)', () => { + const viewOnly = { id: 'v1', name: 'Viewers', permissions: [{ action: 'view', resource: 'aws' }], description: '', allowed_accounts: [] }; + const writer = { id: 'w1', name: 'Editors', permissions: [{ action: 'create', resource: 'aws' }], description: '', allowed_accounts: [] }; + const user = { id: '2', email: 'bob@test.com', groups: [], mfa_enabled: false }; + + it('refuses to save when a concurrent change makes the combination contradictory', async () => { + document.body.innerHTML = '
'; + userState.setAllUsers([user] as any); + userState.setFilteredUsers([user] as any); + userState.setAvailableGroups([viewOnly, writer] as any); + (api.updateUser as jest.Mock).mockReset().mockResolvedValue({}); + let release!: (v: boolean) => void; + mockConfirmDialog.mockReset().mockReturnValueOnce(new Promise((r) => { release = r; })); + userList.renderUsers([user] as any); + document.querySelector('.user-expand-btn[data-user-id="2"]')!.click(); + const cb = document.querySelector('.group-assign-checkbox[data-user-id="2"][data-group-id="w1"]')!; + cb.checked = true; + cb.dispatchEvent(new Event('change', { bubbles: true })); + userState.setAllUsers([{ ...user, groups: ['v1'] }] as any); + release(true); + for (let i = 0; i < 5; i++) await Promise.resolve(); + expect(api.updateUser).not.toHaveBeenCalled(); + expect(cb.checked).toBe(false); + expect(document.querySelector('.toast-error .toast-message')?.textContent).toContain('Contradictory'); + }); +}); + +describe('select-all with only the synthetic current-user row (issue #274)', () => { + it('disables the select-all checkbox when no row is selectable', () => { + document.body.innerHTML = '
'; + userList.renderUsers([{ id: 'current', email: 'me@test.com', groups: [], mfa_enabled: false }] as any); + expect((document.getElementById('select-all-users') as HTMLInputElement).disabled).toBe(true); + }); +}); + describe('saveUser submit button (issue #735)', () => { it('disables Save User while the request is in flight and re-enables it after', async () => { document.body.innerHTML = ` diff --git a/frontend/src/users/userList.ts b/frontend/src/users/userList.ts index 6d05685d2..06b39a320 100644 --- a/frontend/src/users/userList.ts +++ b/frontend/src/users/userList.ts @@ -85,7 +85,7 @@ export function renderUsers(users: APIUser[]): void { @@ -239,14 +239,16 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean // Validate the resulting group combination before hitting the API. // A view-only group (all permissions are view:*) combined with a // write-capable group is semantically contradictory (issue #1405). - const combinationError = validateGroupCombination(next, availableGroups); - if (combinationError) { - showError(combinationError); - // Revert the checkbox immediately so the DOM reflects reality. + const revertCheckbox = (): void => { const cb = document.querySelector( `.group-assign-checkbox[data-user-id="${userId}"][data-group-id="${groupId}"]`, ); if (cb) cb.checked = !checked; + }; + const combinationError = validateGroupCombination(next, availableGroups); + if (combinationError) { + showError(combinationError); + revertCheckbox(); return; } @@ -266,16 +268,19 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean destructive: !checked, }); if (!ok) { - const cb = document.querySelector( - `.group-assign-checkbox[data-user-id="${userId}"][data-group-id="${groupId}"]`, - ); - if (cb) cb.checked = !checked; + revertCheckbox(); return; } // A concurrent toggle may have changed the user while the dialog was open. const latest = allUsers.find(u => u.id === userId) ?? user; const groupsToSave = nextFor(latest); + const latestError = validateGroupCombination(groupsToSave, availableGroups); + if (latestError) { + showError(latestError); + revertCheckbox(); + return; + } try { await api.updateUser(userId, { groups: groupsToSave }); From 57f2752b678444be63f1e511b95acf755f9712c4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:02:33 +0200 Subject: [PATCH 15/16] test(auth): assert the login handler is detached in the reset form Refs #733 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/auth.test.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/frontend/src/__tests__/auth.test.ts b/frontend/src/__tests__/auth.test.ts index da3b54aa8..4b2e4af67 100644 --- a/frontend/src/__tests__/auth.test.ts +++ b/frontend/src/__tests__/auth.test.ts @@ -303,6 +303,27 @@ describe('Auth Module', () => { expect(document.body.textContent).not.toContain('Enter email and password'); }); + test('Enter in the reset form never runs the login handler (issue #733)', async () => { + await showLoginModal(); + document.getElementById('forgot-password-link') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true })); + await new Promise(resolve => setTimeout(resolve, 10)); + + // Both handlers write synchronously and the reset handler writes last, + // so a stale login handler is only visible in the mutation records. + const writes: string[] = []; + const observer = new MutationObserver(records => { + for (const r of records) r.addedNodes.forEach(n => writes.push(n.textContent ?? '')); + }); + observer.observe(document.getElementById('login-modal')!, { childList: true, characterData: true, subtree: true }); + document.getElementById('login-form')!.dispatchEvent(new Event('submit', { bubbles: true, cancelable: true })); + await new Promise(resolve => setTimeout(resolve, 20)); + observer.disconnect(); + + expect(writes.some(text => /Enter email|Enter password/.test(text))).toBe(false); + expect(api.login).not.toHaveBeenCalled(); + }); + test('password reset swaps modal body to confirmation panel (issue #457)', async () => { (api.requestPasswordReset as jest.Mock).mockResolvedValue({}); await showLoginModal(); From e8ba0709dfeb4d92f707e0730598fd0fe419f323 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:13:57 +0200 Subject: [PATCH 16/16] fix(recommendations): keep unknown payment values out of checkbox names Only known payment display labels reach the aria-label; unknown raw API strings are omitted. Refs #746 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/recommendations.test.ts | 4 ++-- frontend/src/recommendations.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 6904a91c4..7fe35322c 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -581,7 +581,7 @@ describe('Recommendations Module', () => { expect(label('rec-np')).not.toBe(label('rec-au')); }); - test('engine-only and unmapped-payment rows get distinct clean checkbox names (issue #746)', async () => { + test('engine-only rows get distinct and unmapped-payment rows get clean checkbox names (issue #746)', async () => { const base = { provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.r5.large', region: 'us-east-1', count: 1, term: 1, savings: 150, upfront_cost: 0 }; const recs = [ { ...base, id: 'rec-my', engine: 'mysql', payment: 'no-upfront' }, @@ -604,7 +604,7 @@ describe('Recommendations Module', () => { document.querySelector(`input[data-rec-id="${id}"]`)?.getAttribute('aria-label') ?? ''; expect(label('rec-my')).toContain('db.r5.large (mysql)'); expect(label('rec-my')).not.toBe(label('rec-pg')); - expect(label('rec-x')).toContain('weird-option'); + expect(label('rec-x')).not.toContain('weird-option'); for (const id of ['rec-my', 'rec-pg', 'rec-x', 'rec-none']) { expect(label(id)).not.toMatch(/\s{2,}|\s$/); } diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index f868eee7a..f6999696e 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3202,7 +3202,7 @@ function buildVariantRowMarkup( rec.region, accountName, formatTerm(rec.term), - rec.payment ? formatPayment(rec.payment) : '', + PAYMENT_DISPLAY_LABELS[rec.payment ?? ''] ?? '', ].filter(part => part).join(' '), ); const checkboxCell = showCheckboxes
- 0 && selectedUserIds.size === users.length ? 'checked' : ''}> + 0 && selectedUserIds.size === selectableCount ? 'checked' : ''}> Email
- + @@ -102,7 +106,7 @@ export function renderUsers(users: APIUser[]): void {
${escapeHtml(user.email)} - ${user.id === 'current' ? 'You' : ''} + ${user.id === SYNTHETIC_USER_ID ? 'You' : ''}
@@ -119,8 +123,8 @@ export function renderUsers(users: APIUser[]): void { ${user.last_login ? formatRelativeTime(user.last_login) : 'Never'}
- - + +
${planBtnHtml}${planBtnHtml}
- 0 && selectedUserIds.size === selectableCount ? 'checked' : ''}> + 0 && selectedUserIds.size === selectableCount ? 'checked' : ''}> Email
- + diff --git a/frontend/tests-e2e/recommendations.spec.ts b/frontend/tests-e2e/recommendations.spec.ts index 4e3e632fb..b77073ded 100644 --- a/frontend/tests-e2e/recommendations.spec.ts +++ b/frontend/tests-e2e/recommendations.spec.ts @@ -13,7 +13,7 @@ async function openOpportunities(page: Page): Promise { async function selectRecommendation(page: Page, id: string): Promise { const row = page.locator(`tr.recommendation-row[data-rec-id="${id}"]`); - await row.getByRole('checkbox', { name: 'Select recommendation' }).check(); + await row.getByRole('checkbox', { name: /^Select / }).check(); } function callsTo(handle: MockHandle, suffix: string): Array> { From 8d08d0d84d932091bcd0f798cedb033274bb2992 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:14:13 +0200 Subject: [PATCH 03/16] fix(users): confirm group membership toggles in the users table Ask before adding or removing a group, and warn when an administrator removes their own Administrators membership. Declining reverts the checkbox without calling the API. Closes #736 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/users.test.ts | 50 ++++++++++++++++++++++++++++ frontend/src/users/userList.ts | 25 ++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index 6c3674c76..ef59abfae 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -3234,3 +3234,53 @@ describe('users table checkbox names (issue #746)', () => { expect(labels).toEqual(['Select a@test.com', 'Select b@test.com']); }); }); + +describe('group toggle confirmation (issue #736)', () => { + const ADMIN = '00000000-0000-5000-8000-000000000001'; + const users = [ + { id: '1', email: 'me@test.com', groups: [ADMIN], mfa_enabled: false }, + { id: '2', email: 'bob@test.com', groups: [ADMIN], mfa_enabled: false }, + ]; + const groups = [{ id: ADMIN, name: 'Administrators', permissions: [], description: '', allowed_accounts: [] }]; + + const toggle = async (userId: string, checked: boolean) => { + document.querySelector(`.user-expand-btn[data-user-id="${userId}"]`)!.click(); + const cb = document.querySelector(`.group-assign-checkbox[data-user-id="${userId}"]`)!; + cb.checked = checked; + cb.dispatchEvent(new Event('change', { bubbles: true })); + for (let i = 0; i < 5; i++) await Promise.resolve(); + return cb; + }; + + beforeEach(() => { + document.body.innerHTML = '
'; + userState.setAllUsers(users as any); + userState.setFilteredUsers(users as any); + userState.setAvailableGroups(groups as any); + (api.updateUser as jest.Mock).mockReset().mockResolvedValue({}); + mockConfirmDialog.mockReset().mockResolvedValue(true); + require('../state').setCurrentUser({ id: '1', email: 'me@test.com', groups: [ADMIN] }); + userList.renderUsers(users as any); + }); + + it('asks before changing membership and skips the API when declined', async () => { + mockConfirmDialog.mockResolvedValueOnce(false); + const cb = await toggle('2', false); + expect(mockConfirmDialog).toHaveBeenCalledTimes(1); + expect(api.updateUser).not.toHaveBeenCalled(); + expect(cb.checked).toBe(true); + }); + + it('applies the change once confirmed', async () => { + await toggle('2', false); + expect(api.updateUser).toHaveBeenCalledWith('2', { groups: [] }); + }); + + it('warns when the admin removes their own Administrators membership', async () => { + await toggle('1', false); + expect(mockConfirmDialog.mock.calls[0]![0]).toMatchObject({ + body: expect.stringContaining('removing yourself'), + destructive: true, + }); + }); +}); diff --git a/frontend/src/users/userList.ts b/frontend/src/users/userList.ts index 6a8c4f218..04e054b82 100644 --- a/frontend/src/users/userList.ts +++ b/frontend/src/users/userList.ts @@ -17,6 +17,8 @@ import { import { escapeHtml, formatRelativeTime, formatDate, showSuccess, showError, validateGroupCombination } from './utils'; import { openEditUserModal, deleteUser } from './userActions'; import { ADMINISTRATORS_GROUP_ID } from '../permissions'; +import { confirmDialog } from '../confirmDialog'; +import { getCurrentUser } from '../state'; // Placeholder row id withCurrentUser prepends when the API omits the caller. const SYNTHETIC_USER_ID = 'current'; @@ -247,6 +249,29 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean return; } + const selfDemotion = !checked + && groupId === ADMINISTRATORS_GROUP_ID + && user.email === getCurrentUser()?.email; + const ok = await confirmDialog({ + title: checked + ? `Add ${user.email} to "${groupLabel}"?` + : `Remove ${user.email} from "${groupLabel}"?`, + body: selfDemotion + ? 'You are removing yourself from Administrators. You will lose access to administration, and another administrator must restore it.' + : checked + ? `${user.email} will gain the permissions of the "${groupLabel}" group.` + : `${user.email} will lose the permissions of the "${groupLabel}" group.`, + confirmLabel: checked ? 'Add to group' : 'Remove from group', + destructive: !checked, + }); + if (!ok) { + const cb = document.querySelector( + `.group-assign-checkbox[data-user-id="${userId}"][data-group-id="${groupId}"]`, + ); + if (cb) cb.checked = !checked; + return; + } + try { await api.updateUser(userId, { groups: next }); From e85620eca3025d6af935af740262869cf368374c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:15:33 +0200 Subject: [PATCH 04/16] fix(apikeys): show revoke and delete failures when the create modal is closed showError wrote to the always-present #create-apikey-error inside the hidden create modal, so the toast fallback never ran. Use the inline error only while that modal is open. Closes #717 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/apikeys.test.ts | 16 ++++++++++++++++ frontend/src/apikeys.ts | 3 ++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/apikeys.test.ts b/frontend/src/__tests__/apikeys.test.ts index 771768ec2..e9852b003 100644 --- a/frontend/src/__tests__/apikeys.test.ts +++ b/frontend/src/__tests__/apikeys.test.ts @@ -602,6 +602,10 @@ describe('API Keys Module', () => { }); describe('handleCreateApiKey', () => { + beforeEach(() => { + document.getElementById('create-apikey-modal')?.classList.remove('hidden'); + }); + test('prevents default form submission', async () => { (api.createApiKey as jest.Mock).mockResolvedValue({ api_key: 'test-key', @@ -940,6 +944,17 @@ describe('API Keys Module', () => { expect(api.getApiKeys).toHaveBeenCalledTimes(2); // Initial load + after revoke }); + test('toasts a failure while the hidden create modal still has its error element (issue #717)', async () => { + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); + (api.revokeApiKey as jest.Mock).mockRejectedValue(new Error('Revoke failed')); + + await revokeApiKey('key-1'); + + expect(document.querySelector('.toast-error')?.querySelector('.toast-message')?.textContent).toBe('Failed to revoke API key'); + expect(document.getElementById('create-apikey-error')?.classList.contains('hidden')).toBe(true); + consoleError.mockRestore(); + }); + test('shows error on API failure', async () => { // Remove the create-apikey-error element so showError falls back to alert document.getElementById('create-apikey-error')?.remove(); @@ -1081,6 +1096,7 @@ describe('API Keys Module', () => { describe('Error display', () => { test('shows error in error element when available', async () => { + document.getElementById('create-apikey-modal')?.classList.remove('hidden'); const event = { preventDefault: jest.fn() } as unknown as Event; await handleCreateApiKey(event); diff --git a/frontend/src/apikeys.ts b/frontend/src/apikeys.ts index d8891c7ff..8b77c8230 100644 --- a/frontend/src/apikeys.ts +++ b/frontend/src/apikeys.ts @@ -555,7 +555,8 @@ export function initApiKeys(): void { */ function showError(message: string): void { const errorEl = document.getElementById('create-apikey-error'); - if (errorEl) { + const createModalOpen = !document.getElementById('create-apikey-modal')?.classList.contains('hidden'); + if (errorEl && createModalOpen) { errorEl.textContent = message; errorEl.classList.remove('hidden'); } else { From c6a8119cb2254990e9794adbee80ece56aa6417a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:16:56 +0200 Subject: [PATCH 05/16] fix(frontend): protect one-time API key and MFA recovery code secrets Disable API key creation while the request is in flight so a double click cannot mint a second key. Secret views (the API key created modal and the MFA recovery codes in the profile modal) ignore Escape until the user acknowledges via data-require-ack, and the profile Cancel button is disabled meanwhile. Closes #732 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/apikeys.test.ts | 32 +++++++++++++++++++ .../src/__tests__/auth-mfa-enroll.test.ts | 18 +++++++++++ frontend/src/apikeys.ts | 11 +++++++ frontend/src/auth.ts | 13 ++++++++ frontend/src/modal.ts | 5 +++ 5 files changed, 79 insertions(+) diff --git a/frontend/src/__tests__/apikeys.test.ts b/frontend/src/__tests__/apikeys.test.ts index e9852b003..ecf634f16 100644 --- a/frontend/src/__tests__/apikeys.test.ts +++ b/frontend/src/__tests__/apikeys.test.ts @@ -1094,6 +1094,38 @@ describe('API Keys Module', () => { }); }); + describe('secret handling (issue #732)', () => { + test('a second submit while the first is in flight mints only one key and disables submit', async () => { + document.getElementById('create-apikey-modal')?.classList.remove('hidden'); + (document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key'; + (document.getElementById('apikey-password') as HTMLInputElement).value = 'pw'; + (document.getElementById('apikey-expires-at') as HTMLInputElement).value = '2999-01-01'; + let resolve!: (v: unknown) => void; + (api.createApiKey as jest.Mock).mockReturnValue(new Promise((r) => { resolve = r; })); + (api.getApiKeys as jest.Mock).mockResolvedValue({ api_keys: [] }); + const event = { preventDefault: jest.fn() } as unknown as Event; + + const first = handleCreateApiKey(event); + await handleCreateApiKey(event); + expect((document.getElementById('create-apikey-submit-btn') as HTMLButtonElement).disabled).toBe(true); + resolve({ api_key: 'k', key_id: 'key-1', key: { id: 'key-1', name: 'Test Key', key_prefix: 'abc' } }); + await first; + + expect(api.createApiKey).toHaveBeenCalledTimes(1); + }); + + test('Escape does not dismiss the created-key modal; the button does', () => { + showKeyCreatedModal('secret-key'); + const modal = document.getElementById('apikey-created-modal')!; + modal.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + expect(document.getElementById('apikey-created-modal')).not.toBeNull(); + expect(modal.classList.contains('hidden')).toBe(false); + + document.getElementById('close-apikey-created-btn')!.click(); + expect(document.getElementById('apikey-created-modal')).toBeNull(); + }); + }); + describe('Error display', () => { test('shows error in error element when available', async () => { document.getElementById('create-apikey-modal')?.classList.remove('hidden'); diff --git a/frontend/src/__tests__/auth-mfa-enroll.test.ts b/frontend/src/__tests__/auth-mfa-enroll.test.ts index 7213d8630..e05e91b67 100644 --- a/frontend/src/__tests__/auth-mfa-enroll.test.ts +++ b/frontend/src/__tests__/auth-mfa-enroll.test.ts @@ -200,4 +200,22 @@ describe('MFA regenerate-recovery-codes flow', () => { expect(api.regenerateMFARecoveryCodes).toHaveBeenCalledWith('123456'); expect(document.body.textContent).toContain('NEW1-XXXX'); }); + + test('Escape and Cancel cannot discard recovery codes until acknowledged (issue #732)', async () => { + (api.regenerateMFARecoveryCodes as jest.Mock).mockResolvedValue({ recovery_codes: ['NEW1-XXXX'] }); + await openProfile(); + document.getElementById('mfa-regenerate-btn')!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + (document.getElementById('mfa-regen-code') as HTMLInputElement).value = '123456'; + document.getElementById('mfa-regen-submit')!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + await new Promise((r) => setTimeout(r, 10)); + + const modal = document.getElementById('profile-modal')!; + modal.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + expect(modal.classList.contains('hidden')).toBe(false); + expect((document.getElementById('profile-cancel') as HTMLButtonElement).disabled).toBe(true); + + document.getElementById('mfa-codes-done')!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + modal.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + expect(modal.classList.contains('hidden')).toBe(true); + }); }); diff --git a/frontend/src/apikeys.ts b/frontend/src/apikeys.ts index 8b77c8230..31cd71265 100644 --- a/frontend/src/apikeys.ts +++ b/frontend/src/apikeys.ts @@ -348,8 +348,11 @@ export async function createApiKey(name: string, password: string, expiresAt: Da /** * Handle create API key form submission */ +let createInFlight = false; + export async function handleCreateApiKey(e: Event): Promise { e.preventDefault(); + if (createInFlight) return; const errorEl = document.getElementById('create-apikey-error'); if (errorEl) errorEl.classList.add('hidden'); @@ -387,6 +390,9 @@ export async function handleCreateApiKey(e: Event): Promise { return; } + createInFlight = true; + const submitBtn = document.getElementById('create-apikey-submit-btn') as HTMLButtonElement | null; + if (submitBtn) submitBtn.disabled = true; try { const response = await createApiKey(name, password, expiresAt, permissions); closeCreateKeyModal(); @@ -395,6 +401,9 @@ export async function handleCreateApiKey(e: Event): Promise { } catch (error) { const err = error as Error; showError(`Failed to create API key: ${err.message}`); + } finally { + createInFlight = false; + updateCreateKeySubmitState(); } } @@ -408,6 +417,7 @@ export function showKeyCreatedModal(apiKey: string): void { const modal = document.createElement('div'); modal.id = 'apikey-created-modal'; modal.className = 'modal'; + modal.dataset.requireAck = 'true'; modal.innerHTML = `

API Key Created Successfully

@@ -459,6 +469,7 @@ export function showKeyCreatedModal(apiKey: string): void { const closeBtn = document.getElementById('close-apikey-created-btn'); if (closeBtn) { closeBtn.addEventListener('click', () => { + delete modal.dataset.requireAck; closeModal(modal); modal.remove(); }); diff --git a/frontend/src/auth.ts b/frontend/src/auth.ts index 64ad596e9..b3c9e3a16 100644 --- a/frontend/src/auth.ts +++ b/frontend/src/auth.ts @@ -1083,6 +1083,7 @@ async function openProfileModal(): Promise { // (Re-)render the MFA section so its state reflects whatever the // user's mfa_enabled flag is right now (could have flipped since // the modal was last opened). See renderMFASection below. + setProfileSecretPending(false); renderMFASection(); // Reset live indicators and any stale error so the previous open @@ -1298,6 +1299,16 @@ async function handleMFAEnrollVerify(): Promise { } } +function setProfileSecretPending(pending: boolean): void { + const modal = document.getElementById('profile-modal'); + if (modal) { + if (pending) modal.dataset.requireAck = 'true'; + else delete modal.dataset.requireAck; + } + const cancel = document.getElementById('profile-cancel') as HTMLButtonElement | null; + if (cancel) cancel.disabled = pending; +} + /** * Step 3 of enrollment: display the plaintext recovery codes. Shown * exactly once; backend stores only bcrypt hashes. Offers a download @@ -1321,10 +1332,12 @@ function renderMFAEnrollCodesStep(codes: string[]): void {
`; + setProfileSecretPending(true); document.getElementById('mfa-codes-download')?.addEventListener('click', () => downloadRecoveryCodes(codes)); document.getElementById('mfa-codes-done')?.addEventListener('click', () => { // Acknowledged — re-render the section so it now shows the // Enabled view (Disable / Regenerate buttons). + setProfileSecretPending(false); renderMFASection(); }); } diff --git a/frontend/src/modal.ts b/frontend/src/modal.ts index 7f9cfcab4..51e360ab2 100644 --- a/frontend/src/modal.ts +++ b/frontend/src/modal.ts @@ -11,6 +11,10 @@ * openModal(modalEl, { onClose: () => discardCallerState() }); // runs on EVERY close path (issue #331) * closeModal(modalEl); // hides + releases trap + restores focus + runs onClose * + * A modal holding a one-time secret sets `data-require-ack="true"` so + * Escape does not dismiss it; the caller removes the attribute once the user + * acknowledges. + * * The helper toggles the existing `.hidden` CSS class so it slots into the * established show/hide convention without touching the stylesheet. */ @@ -108,6 +112,7 @@ export function openModal( const keydownHandler = (e: KeyboardEvent): void => { if (e.key === 'Escape') { e.preventDefault(); + if (el.dataset.requireAck === 'true') return; closeModal(el); return; } From 33321dc09033b3d3cae6878afc46b29babf782a7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:17:50 +0200 Subject: [PATCH 06/16] fix(auth): make Enter in the forgot-password form send the reset The login submit listener stayed bound on the swapped-in reset form, so Enter showed a login error. Swap to a reset submit listener, make the button type=submit and disable it while the request is in flight. Closes #733 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/auth.test.ts | 16 ++++++++++++++++ frontend/src/auth.ts | 14 ++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/frontend/src/__tests__/auth.test.ts b/frontend/src/__tests__/auth.test.ts index cede48c71..da3b54aa8 100644 --- a/frontend/src/__tests__/auth.test.ts +++ b/frontend/src/__tests__/auth.test.ts @@ -287,6 +287,22 @@ describe('Auth Module', () => { expect(sendBtn).toBeTruthy(); }); + test('submitting the reset form (Enter) sends the reset, not a login error (issue #733)', async () => { + (api.requestPasswordReset as jest.Mock).mockResolvedValue({}); + await showLoginModal(); + document.getElementById('forgot-password-link') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true })); + await new Promise(resolve => setTimeout(resolve, 10)); + + (document.getElementById('reset-email') as HTMLInputElement).value = 'test@example.com'; + document.getElementById('login-form')!.dispatchEvent(new Event('submit', { bubbles: true, cancelable: true })); + await new Promise(resolve => setTimeout(resolve, 50)); + + expect(api.requestPasswordReset).toHaveBeenCalledWith('test@example.com'); + expect(api.login).not.toHaveBeenCalled(); + expect(document.body.textContent).not.toContain('Enter email and password'); + }); + test('password reset swaps modal body to confirmation panel (issue #457)', async () => { (api.requestPasswordReset as jest.Mock).mockResolvedValue({}); await showLoginModal(); diff --git a/frontend/src/auth.ts b/frontend/src/auth.ts index b3c9e3a16..c784aa3f8 100644 --- a/frontend/src/auth.ts +++ b/frontend/src/auth.ts @@ -780,6 +780,10 @@ function showMFACodeStep(): void { // same Function — arrow IIFEs would create a new function each time. const handleLoginSubmitListener = (e: Event): void => { void handleLogin(e); }; const handleMFACodeSubmitListener = (e: Event): void => { void handleMFACodeSubmit(e); }; +const handlePasswordResetSubmitListener = (e: Event): void => { + e.preventDefault(); + void handlePasswordReset(); +}; async function handleMFACodeSubmit(e: Event): Promise { e.preventDefault(); @@ -823,11 +827,14 @@ function showForgotPasswordForm(modal: HTMLElement): void {

We'll send you a link to reset your password.

- +

Back to login

`; - document.getElementById('send-reset-btn')?.addEventListener('click', () => void handlePasswordReset()); + // Keep the inline "enter your email" message instead of the native bubble. + (form as HTMLFormElement).noValidate = true; + form.removeEventListener('submit', handleLoginSubmitListener); + form.addEventListener('submit', handlePasswordResetSubmitListener); document.getElementById('back-to-login-link')?.addEventListener('click', (e) => { e.preventDefault(); reloadPage(); @@ -850,6 +857,8 @@ async function handlePasswordReset(): Promise { return; } + const sendBtn = document.getElementById('send-reset-btn') as HTMLButtonElement | null; + if (sendBtn) sendBtn.disabled = true; try { await api.requestPasswordReset(email); // Issue #457: previously this surfaced as an alert() that, when @@ -864,6 +873,7 @@ async function handlePasswordReset(): Promise { errorDiv.textContent = 'Failed to send reset email. Please try again.'; errorDiv.classList.remove('hidden'); } + if (sendBtn) sendBtn.disabled = false; } } From 35b94e936d442c7ba8a5871760ca98a9de50fa78 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:20:30 +0200 Subject: [PATCH 07/16] fix(frontend): disable Save while account and user requests are in flight Wrap handleAccountFormSubmit (which also serves the registration approval onSave path) and saveUser in a try/finally that disables the submit button, as submitOverrideForm already does. Closes #735 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/settings-accounts.test.ts | 19 ++++++++++ frontend/src/__tests__/users.test.ts | 37 +++++++++++++++++++ frontend/src/settings.ts | 10 +++++ frontend/src/users/userModals.ts | 9 +++++ 4 files changed, 75 insertions(+) diff --git a/frontend/src/__tests__/settings-accounts.test.ts b/frontend/src/__tests__/settings-accounts.test.ts index 0061d8f03..bd8db15fd 100644 --- a/frontend/src/__tests__/settings-accounts.test.ts +++ b/frontend/src/__tests__/settings-accounts.test.ts @@ -438,6 +438,25 @@ describe('Account form submit', () => { expect(api.listAccounts).toHaveBeenCalled(); // list refreshed }); + test('disables the Save button while the account request is in flight (issue #735)', async () => { + const btn = document.createElement('button'); + btn.type = 'submit'; + document.getElementById('account-form')!.appendChild(btn); + let resolve!: (v: unknown) => void; + (api.createAccount as jest.Mock).mockReturnValue(new Promise((r) => { resolve = r; })); + + document.getElementById('add-aws-account-btn')!.click(); + (document.getElementById('account-name') as HTMLInputElement).value = 'Acct'; + (document.getElementById('account-external-id') as HTMLInputElement).value = '123456789012'; + document.getElementById('account-form')!.dispatchEvent(new Event('submit')); + await new Promise(r => setTimeout(r, 0)); + expect(btn.disabled).toBe(true); + + resolve({ id: 'new-id', name: 'Acct', provider: 'aws', external_id: '123456789012', enabled: true }); + await new Promise(r => setTimeout(r, 0)); + expect(btn.disabled).toBe(false); + }); + test('updates existing account when account-id is set', async () => { (api.updateAccount as jest.Mock).mockResolvedValue({ id: 'existing-id', name: 'Updated', provider: 'aws', external_id: '123456789012', enabled: true diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index ef59abfae..f0411ed07 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -3284,3 +3284,40 @@ describe('group toggle confirmation (issue #736)', () => { }); }); }); + +describe('saveUser submit button (issue #735)', () => { + it('disables Save User while the request is in flight and re-enables it after', async () => { + document.body.innerHTML = ` + +
`; + userState.setAvailableGroups([{ id: 'users', name: 'Users', permissions: [], description: '' }] as any); + userState.setCurrentEditingUser(null); + userModals.openCreateUserModal(); + const btn = document.querySelector('#user-form button[type="submit"]')!; + (document.getElementById('user-email') as HTMLInputElement).value = 'new@test.com'; + (document.getElementById('user-password') as HTMLInputElement).value = 'SecurePass123!'; + const gs = document.getElementById('user-groups') as HTMLSelectElement; + if (gs.options.length > 0) gs.options[0]!.selected = true; + let resolve!: (v: unknown) => void; + (api.createUser as jest.Mock).mockReturnValueOnce(new Promise((r) => { resolve = r; })); + (api.listUsers as jest.Mock).mockResolvedValue({ users: [] }); + (api.listGroups as jest.Mock).mockResolvedValue({ groups: [] }); + + const event = new Event('submit'); + event.preventDefault = jest.fn(); + const pending = userModals.saveUser(event); + expect(btn.disabled).toBe(true); + resolve({}); + await pending; + expect(btn.disabled).toBe(false); + }); +}); diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index e8b0bb332..efbed45af 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -2572,6 +2572,16 @@ async function saveAccountCredentialsIfFilled(accountId: string, provider: Accou */ async function handleAccountFormSubmit(e: Event): Promise { e.preventDefault(); + const submitBtn = document.querySelector('#account-form button[type="submit"]'); + if (submitBtn) submitBtn.disabled = true; + try { + await submitAccountForm(); + } finally { + if (submitBtn) submitBtn.disabled = false; + } +} + +async function submitAccountForm(): Promise { const provider = accountModalProvider; const req = buildAccountRequest(provider); diff --git a/frontend/src/users/userModals.ts b/frontend/src/users/userModals.ts index 85b8a597e..107121362 100644 --- a/frontend/src/users/userModals.ts +++ b/frontend/src/users/userModals.ts @@ -105,7 +105,16 @@ export function closeUserModal(): void { */ export async function saveUser(e: Event): Promise { e.preventDefault(); + const submitBtn = document.querySelector('#user-form button[type="submit"]'); + if (submitBtn) submitBtn.disabled = true; + try { + await submitUserForm(); + } finally { + if (submitBtn) submitBtn.disabled = false; + } +} +async function submitUserForm(): Promise { const email = (document.getElementById('user-email') as HTMLInputElement).value; const password = (document.getElementById('user-password') as HTMLInputElement).value; // role selector removed: PR #912 drops the role column. From 759135d7eb688d8a33fdd89ab2d5cef6cfbc0196 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:56:58 +0200 Subject: [PATCH 08/16] fix(frontend): enforce the secret acknowledgement in closeModal Save Changes and Enter in the profile form closed the modal through closeModal, discarding pending MFA recovery codes. closeModal now does nothing while data-require-ack is set, saveProfile returns early and the profile Save button is disabled until the codes are acknowledged. Refs #732 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/auth-mfa-enroll.test.ts | 24 +++++++++++++++++++ frontend/src/auth.ts | 3 +++ frontend/src/modal.ts | 4 ++-- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/frontend/src/__tests__/auth-mfa-enroll.test.ts b/frontend/src/__tests__/auth-mfa-enroll.test.ts index e05e91b67..d12b50d6f 100644 --- a/frontend/src/__tests__/auth-mfa-enroll.test.ts +++ b/frontend/src/__tests__/auth-mfa-enroll.test.ts @@ -49,6 +49,7 @@ import * as state from '../state'; // trigger it indirectly by clicking the user-email element after // updateUserUI mounts it. import { updateUserUI } from '../auth'; +import { closeModal } from '../modal'; beforeEach(() => { document.body.innerHTML = ` @@ -218,4 +219,27 @@ describe('MFA regenerate-recovery-codes flow', () => { modal.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); expect(modal.classList.contains('hidden')).toBe(true); }); + + test('Save Changes and Enter cannot close the profile modal while codes are pending (issue #732)', async () => { + (api.regenerateMFARecoveryCodes as jest.Mock).mockResolvedValue({ recovery_codes: ['NEW1-XXXX'] }); + await openProfile(); + document.getElementById('mfa-regenerate-btn')!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + (document.getElementById('mfa-regen-code') as HTMLInputElement).value = '123456'; + document.getElementById('mfa-regen-submit')!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + await new Promise((r) => setTimeout(r, 10)); + + const modal = document.getElementById('profile-modal')!; + const save = modal.querySelector('#profile-form button[type="submit"]')!; + expect(save.disabled).toBe(true); + document.getElementById('profile-form')!.dispatchEvent(new Event('submit', { bubbles: true, cancelable: true })); + await new Promise((r) => setTimeout(r, 10)); + expect(modal.classList.contains('hidden')).toBe(false); + closeModal(modal); + expect(modal.classList.contains('hidden')).toBe(false); + + document.getElementById('mfa-codes-done')!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(save.disabled).toBe(false); + closeModal(modal); + expect(modal.classList.contains('hidden')).toBe(true); + }); }); diff --git a/frontend/src/auth.ts b/frontend/src/auth.ts index c784aa3f8..fa0dc89e0 100644 --- a/frontend/src/auth.ts +++ b/frontend/src/auth.ts @@ -1317,6 +1317,8 @@ function setProfileSecretPending(pending: boolean): void { } const cancel = document.getElementById('profile-cancel') as HTMLButtonElement | null; if (cancel) cancel.disabled = pending; + const save = document.querySelector('#profile-form button[type="submit"]'); + if (save) save.disabled = pending; } /** @@ -1479,6 +1481,7 @@ async function handleMFARegenerateSubmit(): Promise { */ async function saveProfile(e: Event): Promise { e.preventDefault(); + if (document.getElementById('profile-modal')?.dataset.requireAck === 'true') return; const email = (document.getElementById('profile-email') as HTMLInputElement).value; const currentPassword = (document.getElementById('profile-current-password') as HTMLInputElement).value; diff --git a/frontend/src/modal.ts b/frontend/src/modal.ts index 51e360ab2..d01a8e372 100644 --- a/frontend/src/modal.ts +++ b/frontend/src/modal.ts @@ -13,7 +13,7 @@ * * A modal holding a one-time secret sets `data-require-ack="true"` so * Escape does not dismiss it; the caller removes the attribute once the user - * acknowledges. + * acknowledges; until then closeModal is a no-op. * * The helper toggles the existing `.hidden` CSS class so it slots into the * established show/hide convention without touching the stylesheet. @@ -112,7 +112,6 @@ export function openModal( const keydownHandler = (e: KeyboardEvent): void => { if (e.key === 'Escape') { e.preventDefault(); - if (el.dataset.requireAck === 'true') return; closeModal(el); return; } @@ -162,6 +161,7 @@ export function openModal( * legacy code paths that call closeModal directly from a callback safe. */ export function closeModal(el: HTMLElement): void { + if (el.dataset.requireAck === 'true') return; const state = openModals.get(el); teardown(el); el.classList.add('hidden'); From 8668ae1a25376bd8e87a76b69697cc34367b1215 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:57:45 +0200 Subject: [PATCH 09/16] fix(users): carry the session MFA state into the synthetic current-user row The synthetic row hardcoded mfa_enabled false, so the MFA stat and the row badge were wrong for an admin with MFA on. Use the session user value and count the row in the stat again. Refs #274 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/users.test.ts | 12 ++++++++---- frontend/src/users/userActions.ts | 2 +- frontend/src/users/userList.ts | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index f0411ed07..2a652169a 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -3213,11 +3213,15 @@ describe('synthetic current-user row (issue #274)', () => { expect([...userState.selectedUserIds]).toEqual(['1']); }); - it('excludes the synthetic row from the MFA stat', () => { - userState.setAllUsers([{ ...synthetic, mfa_enabled: true }, { ...real, mfa_enabled: false }] as any); - userList.renderUserStats(); + it('carries the session user mfa_enabled into the synthetic row and the MFA stat', async () => { + require('../state').setCurrentUser({ id: 'u9', email: 'me@test.com', groups: [], mfa_enabled: true }); + (api.listUsers as jest.Mock).mockResolvedValue({ users: [{ ...real, mfa_enabled: false }] }); + (api.listGroups as jest.Mock).mockResolvedValue({ groups: [] }); + await userActions.loadUsers(); + const row = document.querySelector('tr.user-row[data-user-id="current"]')!; + expect(row.textContent).toContain('Enabled'); const card = [...document.querySelectorAll('.stat-card')].find(c => c.textContent!.includes('MFA Enabled'))!; - expect(card.querySelector('.stat-value')!.textContent).toBe('0'); + expect(card.querySelector('.stat-value')!.textContent).toBe('1'); }); }); diff --git a/frontend/src/users/userActions.ts b/frontend/src/users/userActions.ts index 8827509d0..a5d9dd991 100644 --- a/frontend/src/users/userActions.ts +++ b/frontend/src/users/userActions.ts @@ -60,7 +60,7 @@ function withCurrentUser(users: api.APIUser[]): api.APIUser[] { id: 'current', email: current.email, groups: Array.isArray(current.groups) ? current.groups : [], - mfa_enabled: false, + mfa_enabled: current.mfa_enabled === true, }; return [synthetic, ...users]; } diff --git a/frontend/src/users/userList.ts b/frontend/src/users/userList.ts index 04e054b82..e27a6c765 100644 --- a/frontend/src/users/userList.ts +++ b/frontend/src/users/userList.ts @@ -42,7 +42,7 @@ export function renderUserStats(): void { const adminUsers = allUsers.filter(u => Array.isArray(u.groups) && u.groups.includes(ADMINISTRATORS_GROUP_ID) ).length; - const mfaEnabled = allUsers.filter(u => u.id !== SYNTHETIC_USER_ID && u.mfa_enabled).length; + const mfaEnabled = allUsers.filter(u => u.mfa_enabled).length; const showing = filteredUsers.length; statsContainer.innerHTML = ` From 01b1683d24a3ed12b00ca29c11e42e35d7526894 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 19:58:21 +0200 Subject: [PATCH 10/16] fix(a11y): include payment in recommendation checkbox names Term and payment variants of one cell shared a name. Append the known payment display label (unknown values are omitted so raw API strings never reach the attribute). Refs #746 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/recommendations.test.ts | 24 +++++++++++++++++++ frontend/src/recommendations.ts | 2 +- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 551765614..3a41043fe 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -557,6 +557,30 @@ describe('Recommendations Module', () => { expect(label('rec-a')).not.toBe(label('rec-b')); }); + test('variants differing only by payment get distinct checkbox names (issue #746)', async () => { + const base = { provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, savings: 150, upfront_cost: 1000 }; + const recs = [ + { ...base, id: 'rec-np', payment: 'no-upfront' }, + { ...base, id: 'rec-au', payment: 'all-upfront' }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: { total_count: 2, total_monthly_savings: 300, total_upfront_cost: 2000, avg_payback_months: 1 }, + recommendations: recs, + regions: [], + }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + await loadRecommendations(); + document.querySelector('.rec-cell-chevron')!.click(); + + const label = (id: string) => + document.querySelector(`input[data-rec-id="${id}"]`)?.getAttribute('aria-label') ?? ''; + expect(label('rec-np')).toContain('No Upfront'); + expect(label('rec-au')).toContain('All Upfront'); + expect(label('rec-np')).not.toBe(label('rec-au')); + }); + test('select-all change event updates summary cards in place (PR #283 CR pass-2)', async () => { // Verifies the select-all checkbox event path: after checking select-all, // summary cards reflect the picked-best-per-cell selection WITHOUT a diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 5a554762c..03a7c93be 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3195,7 +3195,7 @@ function buildVariantRowMarkup( ? `` : ''; const selectLabel = escapeHtml( - `Select ${rec.service} ${rec.resource_type} ${rec.region} ${accountName} ${formatTerm(rec.term)}`, + `Select ${rec.service} ${rec.resource_type} ${rec.region} ${accountName} ${formatTerm(rec.term)} ${PAYMENT_DISPLAY_LABELS[rec.payment ?? ''] ?? ''}`, ); const checkboxCell = showCheckboxes ? `		${planBtnHtml}${planBtnHtml}
- 0 && selectedUserIds.size === selectableCount ? 'checked' : ''}> + 0 && selectedUserIds.size === selectableCount ? 'checked' : ''}> Email