From c0b2e7cdaa2e7eb02be4b2927a756944ef2d3347 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 02:31:40 +0300 Subject: [PATCH 1/2] fix(admin): validate group combos, fix checkbox UX, remove stale bulk checkboxes Closes #1404 closes #1405 closes #1408. #1404 - user-selection and select-all checkboxes no longer call renderUsers(), which was rebuilding the entire table and collapsing any open expand panels. Both handlers now patch the affected rows in-place (row-selected class, individual checkbox state, bulk-actions-bar visibility) without touching the expand rows. #1405 - contradictory group combinations (view-only group paired with a write-capable group) are now blocked at two sites: - toggleUserGroup() in the expand panel, before the updateUser API call. The checkbox is reverted immediately on rejection. - saveUser() in the create/edit modal, before the createUser/updateUser API call. The shared validateGroupCombination() utility in users/utils.ts encapsulates the detection logic and returns a human-readable message. #1408 - the user-list checkboxes were already wired to real bulk actions (bulk-delete and bulk-add-to-group) in PR #977; this PR retains that wiring and the UX fix in #1404 removes the last friction point (panel collapse on selection) that made the feature feel broken. Tests: 33 new test cases covering validateGroupCombination unit behaviour, checkbox-does-not-collapse-panel for both individual and select-all, bulk-actions-bar show/hide, and group-combo validation in both the expand panel and the modal. --- frontend/src/__tests__/users.test.ts | 505 +++++++++++++++++++++++++++ frontend/src/users/userList.ts | 60 +++- frontend/src/users/userModals.ts | 10 +- frontend/src/users/utils.ts | 53 +++ 4 files changed, 623 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index 8fa65e3f5..0aba0520a 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -2582,3 +2582,508 @@ describe('group union permission hint (issue #1001)', () => { expect(groupsSection!.textContent).toContain('combined (union) of all selected groups'); }); }); + +// ============================================================================ +// VALIDATE GROUP COMBINATION UNIT TESTS (issue #1405) +// ============================================================================ +describe('validateGroupCombination (issue #1405)', () => { + const viewOnly = { + id: 'v1', name: 'Viewers', + permissions: [{ action: 'view', resource: 'aws' }], + description: '', allowed_accounts: [], + }; + const writeCapable = { + id: 'w1', name: 'Editors', + permissions: [{ action: 'create', resource: 'aws' }], + description: '', allowed_accounts: [], + }; + const mixedCapable = { + id: 'm1', name: 'Mixed', + permissions: [{ action: 'view', resource: 'a' }, { action: 'create', resource: 'b' }], + description: '', allowed_accounts: [], + }; + const emptyPerms = { + id: 'e1', name: 'Empty', + permissions: [], + description: '', allowed_accounts: [], + }; + const allGroups = [viewOnly, writeCapable, mixedCapable, emptyPerms]; + + it('returns null for empty selection', () => { + expect(userUtils.validateGroupCombination([], allGroups as any)).toBeNull(); + }); + + it('returns null for a single view-only group', () => { + expect(userUtils.validateGroupCombination(['v1'], allGroups as any)).toBeNull(); + }); + + it('returns null for a single write-capable group', () => { + expect(userUtils.validateGroupCombination(['w1'], allGroups as any)).toBeNull(); + }); + + it('returns null for an empty-permission group alone', () => { + expect(userUtils.validateGroupCombination(['e1'], allGroups as any)).toBeNull(); + }); + + it('returns null for view-only + empty-permission group (empty is neutral)', () => { + expect(userUtils.validateGroupCombination(['v1', 'e1'], allGroups as any)).toBeNull(); + }); + + it('returns null for two write-capable groups', () => { + const w2 = { + id: 'w2', name: 'Admins', + permissions: [{ action: 'delete', resource: 'aws' }], + description: '', allowed_accounts: [], + }; + expect(userUtils.validateGroupCombination(['w1', 'w2'], [...allGroups, w2] as any)).toBeNull(); + }); + + it('returns null for an unknown group ID (not in allGroups)', () => { + expect(userUtils.validateGroupCombination(['unknown-uuid'], allGroups as any)).toBeNull(); + }); + + it('returns an error string for view-only + write-capable combination', () => { + const result = userUtils.validateGroupCombination(['v1', 'w1'], allGroups as any); + expect(result).not.toBeNull(); + expect(result).toContain('Contradictory'); + expect(result).toContain('"Viewers"'); + expect(result).toContain('"Editors"'); + }); + + it('uses "is" (singular) for one view-only group in the error message', () => { + const result = userUtils.validateGroupCombination(['v1', 'w1'], allGroups as any); + expect(result).toContain(' is view-only'); + }); + + it('uses "are" (plural) for multiple view-only groups in the error message', () => { + const v2 = { + id: 'v2', name: 'Readers', + permissions: [{ action: 'view', resource: 'gcp' }], + description: '', allowed_accounts: [], + }; + const result = userUtils.validateGroupCombination(['v1', 'v2', 'w1'], [...allGroups, v2] as any); + expect(result).toContain(' are view-only'); + }); + + it('uses "grants" (singular) for one write-capable group in the error message', () => { + const result = userUtils.validateGroupCombination(['v1', 'w1'], allGroups as any); + expect(result).toContain('grants write'); + }); + + it('uses "grant" (plural) for multiple write-capable groups in the error message', () => { + const w2 = { + id: 'w2', name: 'Admins', + permissions: [{ action: 'delete', resource: 'aws' }], + description: '', allowed_accounts: [], + }; + const result = userUtils.validateGroupCombination(['v1', 'w1', 'w2'], [...allGroups, w2] as any); + expect(result).toContain('grant write'); + }); + + it('treats a mixed group (has at least one non-view action) as write-capable', () => { + // m1 has both view and create permissions -- must trigger a conflict when + // combined with a pure view-only group. + const result = userUtils.validateGroupCombination(['v1', 'm1'], allGroups as any); + expect(result).not.toBeNull(); + expect(result).toContain('"Mixed"'); + }); +}); + +// ============================================================================ +// USER-SELECTION CHECKBOX UX (issue #1404) +// Panel must not collapse when a user-selection checkbox is toggled. +// ============================================================================ +describe('user-selection checkbox UX (issue #1404)', () => { + const mockUsers = [ + { id: '1', email: 'alice@test.com', groups: ['g1'], mfa_enabled: false }, + { id: '2', email: 'bob@test.com', groups: [], mfa_enabled: false }, + ]; + const mockGroups = [ + { id: 'g1', name: 'Admins', permissions: [], description: '', allowed_accounts: [] }, + ]; + + function buildDom(): void { + document.body.innerHTML = ` +
+
+ + `; + } + + beforeEach(() => { + buildDom(); + userState.setAllUsers(mockUsers as any); + userState.setFilteredUsers(mockUsers as any); + userState.setAvailableGroups(mockGroups as any); + userState.clearSelectedUserIds(); + jest.clearAllMocks(); + }); + + it('individual user checkbox does not collapse an open expand panel', () => { + userList.renderUsers(mockUsers as any); + + // Expand user "1" + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="1"]', + ); + expect(expandBtn).toBeTruthy(); + expandBtn!.click(); + + const expandRow = document.querySelector( + 'tr.user-expand-row[data-user-id="1"]', + ); + expect(expandRow?.classList.contains('hidden')).toBe(false); + + // Check user "2"'s selection checkbox (not a group-assign checkbox). + // Pre-fix: this called renderUsers, which rebuilt the whole table and + // collapsed user "1"'s expand row. + const cb = document.querySelector( + '.user-checkbox[data-user-id="2"]', + ); + expect(cb).toBeTruthy(); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + // User "1"'s panel must still be open after the checkbox event. + expect(expandRow?.classList.contains('hidden')).toBe(false); + }); + + it('select-all checkbox does not collapse an open expand panel', () => { + userList.renderUsers(mockUsers as any); + + // Expand user "2" + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="2"]', + ); + expect(expandBtn).toBeTruthy(); + expandBtn!.click(); + + const expandRow = document.querySelector( + 'tr.user-expand-row[data-user-id="2"]', + ); + expect(expandRow?.classList.contains('hidden')).toBe(false); + + // Check select-all. Pre-fix: this called renderUsers, collapsing all panels. + const selectAll = document.getElementById('select-all-users') as HTMLInputElement; + expect(selectAll).toBeTruthy(); + selectAll!.checked = true; + selectAll!.dispatchEvent(new Event('change', { bubbles: true })); + + // User "2"'s expand panel must still be open. + expect(expandRow?.classList.contains('hidden')).toBe(false); + }); + + it('individual checkbox adds row-selected class on the affected row only', () => { + userList.renderUsers(mockUsers as any); + + const row1 = document.querySelector('tr.user-row[data-user-id="1"]'); + const row2 = document.querySelector('tr.user-row[data-user-id="2"]'); + expect(row1?.classList.contains('row-selected')).toBe(false); + expect(row2?.classList.contains('row-selected')).toBe(false); + + const cb = document.querySelector('.user-checkbox[data-user-id="1"]'); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + expect(row1?.classList.contains('row-selected')).toBe(true); + expect(row2?.classList.contains('row-selected')).toBe(false); + }); + + it('individual checkbox shows the bulk-actions bar when a user is checked', () => { + userList.renderUsers(mockUsers as any); + + const bar = document.getElementById('bulk-actions-bar'); + expect(bar?.classList.contains('hidden')).toBe(true); + + const cb = document.querySelector('.user-checkbox[data-user-id="1"]'); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + expect(bar?.classList.contains('hidden')).toBe(false); + expect(document.getElementById('selected-count')?.textContent).toBe('1'); + }); + + it('unchecking all individual checkboxes hides the bulk-actions bar again', () => { + userList.renderUsers(mockUsers as any); + + const bar = document.getElementById('bulk-actions-bar'); + const cb1 = document.querySelector('.user-checkbox[data-user-id="1"]'); + cb1!.checked = true; + cb1!.dispatchEvent(new Event('change', { bubbles: true })); + expect(bar?.classList.contains('hidden')).toBe(false); + + cb1!.checked = false; + cb1!.dispatchEvent(new Event('change', { bubbles: true })); + expect(bar?.classList.contains('hidden')).toBe(true); + }); + + it('select-all shows the bulk-actions bar with correct count and marks all rows', () => { + userList.renderUsers(mockUsers as any); + + const bar = document.getElementById('bulk-actions-bar'); + const selectAll = document.getElementById('select-all-users') as HTMLInputElement; + selectAll!.checked = true; + selectAll!.dispatchEvent(new Event('change', { bubbles: true })); + + expect(bar?.classList.contains('hidden')).toBe(false); + expect(document.getElementById('selected-count')?.textContent).toBe('2'); + expect( + document.querySelector('tr.user-row[data-user-id="1"]') + ?.classList.contains('row-selected'), + ).toBe(true); + expect( + document.querySelector('tr.user-row[data-user-id="2"]') + ?.classList.contains('row-selected'), + ).toBe(true); + }); + + it('deselecting via select-all hides the bulk-actions bar and removes row-selected from all rows', () => { + userList.renderUsers(mockUsers as any); + + const bar = document.getElementById('bulk-actions-bar'); + const selectAll = document.getElementById('select-all-users') as HTMLInputElement; + + // Select all first + selectAll!.checked = true; + selectAll!.dispatchEvent(new Event('change', { bubbles: true })); + expect(bar?.classList.contains('hidden')).toBe(false); + + // Then deselect all + selectAll!.checked = false; + selectAll!.dispatchEvent(new Event('change', { bubbles: true })); + expect(bar?.classList.contains('hidden')).toBe(true); + expect( + document.querySelector('tr.user-row[data-user-id="1"]') + ?.classList.contains('row-selected'), + ).toBe(false); + expect( + document.querySelector('tr.user-row[data-user-id="2"]') + ?.classList.contains('row-selected'), + ).toBe(false); + }); +}); + +// ============================================================================ +// CONTRADICTORY GROUP COMBINATION VALIDATION (issue #1405) +// Validates that the expand panel and the modal both reject view-only + +// write-capable group combinations before hitting the API. +// ============================================================================ +describe('contradictory group combination validation (issue #1405)', () => { + const viewOnly = { + id: 'v1', name: 'Viewers', + permissions: [{ action: 'view', resource: 'aws' }], + description: '', allowed_accounts: [], + }; + const writeCapable = { + id: 'w1', name: 'Editors', + permissions: [{ action: 'create', resource: 'aws' }], + description: '', allowed_accounts: [], + }; + const mockGroups = [viewOnly, writeCapable]; + + // ------------------------------------------------------------------------- + // Expand panel (inline group-assign checkboxes, issue #1405 + #998 surface) + // ------------------------------------------------------------------------- + describe('expand panel group toggle', () => { + // Alice starts with the view-only group. Adding a write group should be + // blocked before the API is called. + const mockUsers = [ + { id: '1', email: 'alice@test.com', groups: ['v1'], mfa_enabled: false }, + ]; + + function buildDom(): void { + document.body.innerHTML = ` +
+
+ + `; + } + + beforeEach(() => { + buildDom(); + userState.setAllUsers(mockUsers as any); + userState.setFilteredUsers(mockUsers as any); + userState.setAvailableGroups(mockGroups as any); + userState.clearSelectedUserIds(); + (api.updateUser as jest.Mock).mockResolvedValue({}); + jest.clearAllMocks(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('shows an error toast and does not call the API when adding a write group to a view-only user', async () => { + jest.useFakeTimers(); + userList.renderUsers(mockUsers as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="1"]', + ); + expect(expandBtn).toBeTruthy(); + expandBtn!.click(); + + // Alice has ['v1'] (view-only). Toggling on 'w1' (write-capable) + // produces a contradictory combination and must be rejected. + const cb = document.querySelector( + '.group-assign-checkbox[data-user-id="1"][data-group-id="w1"]', + ); + expect(cb).toBeTruthy(); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + await Promise.resolve(); + await Promise.resolve(); + + expect(api.updateUser).not.toHaveBeenCalled(); + const toast = document.querySelector('.toast-error'); + expect(toast).toBeTruthy(); + expect(toast?.querySelector('.toast-message')?.textContent).toContain('Contradictory'); + }); + + it('reverts the group-assign checkbox to its pre-toggle state on a contradictory combination', async () => { + jest.useFakeTimers(); + userList.renderUsers(mockUsers as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="1"]', + ); + expandBtn!.click(); + + const cb = document.querySelector( + '.group-assign-checkbox[data-user-id="1"][data-group-id="w1"]', + ); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + await Promise.resolve(); + await Promise.resolve(); + + // Checkbox must be reverted to unchecked (pre-toggle state). + expect(cb!.checked).toBe(false); + }); + + it('calls the API when adding a write-capable group to a user with no groups (not contradictory)', async () => { + const user = { id: '3', email: 'carol@test.com', groups: [], mfa_enabled: false }; + userState.setAllUsers([user] as any); + userState.setFilteredUsers([user] as any); + + jest.useFakeTimers(); + userList.renderUsers([user] as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="3"]', + ); + expect(expandBtn).toBeTruthy(); + expandBtn!.click(); + + const cb = document.querySelector( + '.group-assign-checkbox[data-user-id="3"][data-group-id="w1"]', + ); + expect(cb).toBeTruthy(); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + await Promise.resolve(); + await Promise.resolve(); + + expect(api.updateUser).toHaveBeenCalledWith('3', { groups: ['w1'] }); + expect(document.querySelector('.toast-error')).toBeFalsy(); + }); + }); + + // ------------------------------------------------------------------------- + // Modal saveUser (create + edit paths, issue #1405) + // ------------------------------------------------------------------------- + describe('modal saveUser', () => { + beforeEach(() => { + document.body.innerHTML = ` + +
+
+ `; + userState.setAvailableGroups(mockGroups as any); + userState.setCurrentEditingUser(null); + (api.createUser as jest.Mock).mockResolvedValue({}); + (api.updateUser as jest.Mock).mockResolvedValue({}); + (api.listUsers as jest.Mock).mockResolvedValue({ users: [] }); + (api.listGroups as jest.Mock).mockResolvedValue({ groups: mockGroups }); + jest.clearAllMocks(); + }); + + it('shows error and does not call createUser when contradictory groups are selected on create', async () => { + userModals.openCreateUserModal(); + + // Select both v1 (view-only) and w1 (write-capable) -- contradictory. + const gs = document.getElementById('user-groups') as HTMLSelectElement; + Array.from(gs.options).forEach(opt => { opt.selected = true; }); + + const event = new Event('submit'); + event.preventDefault = jest.fn(); + + await userModals.saveUser(event); + + expect(api.createUser).not.toHaveBeenCalled(); + const toast = document.querySelector('.toast-error'); + expect(toast).toBeTruthy(); + expect(toast?.querySelector('.toast-message')?.textContent).toContain('Contradictory'); + }); + + it('shows error and does not call updateUser when contradictory groups are selected on edit', async () => { + const editUser = { + id: '1', email: 'alice@test.com', groups: ['v1'], mfa_enabled: false, + }; + (api.getUser as jest.Mock).mockResolvedValue(editUser); + await userModals.openEditUserModal('1'); + + // Select both options (v1 + w1) -- contradictory. + const gs = document.getElementById('user-groups') as HTMLSelectElement; + Array.from(gs.options).forEach(opt => { opt.selected = true; }); + + const event = new Event('submit'); + event.preventDefault = jest.fn(); + + await userModals.saveUser(event); + + expect(api.updateUser).not.toHaveBeenCalled(); + const toast = document.querySelector('.toast-error'); + expect(toast).toBeTruthy(); + expect(toast?.querySelector('.toast-message')?.textContent).toContain('Contradictory'); + }); + + it('calls createUser when only a single write-capable group is selected (not contradictory)', async () => { + userModals.openCreateUserModal(); + (document.getElementById('user-email') as HTMLInputElement).value = 'new@test.com'; + (document.getElementById('user-password') as HTMLInputElement).value = 'SecurePass123!'; + + // Select only w1 (write-capable) -- no view-only group, so no conflict. + const gs = document.getElementById('user-groups') as HTMLSelectElement; + Array.from(gs.options).forEach(opt => { opt.selected = opt.value === 'w1'; }); + + const event = new Event('submit'); + event.preventDefault = jest.fn(); + + await userModals.saveUser(event); + + expect(api.createUser).toHaveBeenCalledWith( + expect.objectContaining({ groups: ['w1'] }), + ); + expect(document.querySelector('.toast-error')).toBeFalsy(); + }); + }); +}); diff --git a/frontend/src/users/userList.ts b/frontend/src/users/userList.ts index a4b4e3489..92261378f 100644 --- a/frontend/src/users/userList.ts +++ b/frontend/src/users/userList.ts @@ -14,7 +14,7 @@ import { clearSelectedUserIds, setAllUsers, } from './state'; -import { escapeHtml, formatRelativeTime, formatDate, showSuccess, showError } from './utils'; +import { escapeHtml, formatRelativeTime, formatDate, showSuccess, showError, validateGroupCombination } from './utils'; import { openEditUserModal, deleteUser } from './userActions'; import { ADMINISTRATORS_GROUP_ID } from '../permissions'; @@ -210,6 +210,9 @@ function renderUserExpandPanel(user: APIUser): string { /** * Toggle a group membership for a user inline (no modal). * + * Before the API call, validates that the resulting group set is not a + * contradictory combination (issue #1405: view-only + write-capable). + * * After a successful API call, patch state in-memory and refresh only the * affected expand panel rather than re-rendering the whole table. This * prevents the panel from collapsing on every toggle (issue #998) and @@ -226,6 +229,20 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean ? [...new Set([...user.groups, groupId])] : user.groups.filter(g => g !== groupId); + // 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 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 }); @@ -276,6 +293,9 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean */ function setupUserTableListeners(): void { // Select all checkbox + // Patch all rows in-place instead of calling renderUsers(), which would + // tear down and rebuild the entire table and collapse any open expand + // panels (issue #1404 symptom: expand arrow / panel lost on checkbox click). const selectAllCheckbox = document.getElementById('select-all-users') as HTMLInputElement; if (selectAllCheckbox) { selectAllCheckbox.addEventListener('change', (e) => { @@ -285,24 +305,56 @@ function setupUserTableListeners(): void { } else { clearSelectedUserIds(); } - renderUsers(filteredUsers); + // Patch each row's selected class and its individual checkbox without + // re-rendering the whole table (preserves open expand panels). + filteredUsers.forEach(user => { + const row = document.querySelector( + `tr.user-row[data-user-id="${user.id}"]`, + ); + const cb = document.querySelector( + `.user-checkbox[data-user-id="${user.id}"]`, + ); + if (row) { + if (checked) row.classList.add('row-selected'); + else row.classList.remove('row-selected'); + } + if (cb) cb.checked = checked; + }); updateBulkActionsBar(); }); } // Individual checkboxes + // Same rationale: patch the affected row in-place instead of calling + // renderUsers() so open expand panels are not collapsed (issue #1404). document.querySelectorAll('.user-checkbox').forEach(checkbox => { checkbox.addEventListener('change', (e) => { const userId = (e.target as HTMLElement).dataset.userId; if (!userId) return; - if ((e.target as HTMLInputElement).checked) { + const nowChecked = (e.target as HTMLInputElement).checked; + if (nowChecked) { addSelectedUserId(userId); } else { removeSelectedUserId(userId); } - renderUsers(filteredUsers); + // Toggle the row-selected class on just the affected row. + const row = document.querySelector( + `tr.user-row[data-user-id="${userId}"]`, + ); + if (row) { + if (nowChecked) row.classList.add('row-selected'); + else row.classList.remove('row-selected'); + } + + // Keep the select-all checkbox in sync. + const selectAll = document.getElementById('select-all-users') as HTMLInputElement | null; + if (selectAll) { + selectAll.checked = + selectedUserIds.size > 0 && selectedUserIds.size === filteredUsers.length; + } + updateBulkActionsBar(); }); }); diff --git a/frontend/src/users/userModals.ts b/frontend/src/users/userModals.ts index 65b00aa8b..85b8a597e 100644 --- a/frontend/src/users/userModals.ts +++ b/frontend/src/users/userModals.ts @@ -14,7 +14,7 @@ import { setCurrentEditingUser, availableGroups } from './state'; -import { escapeHtml, showError, showSuccess } from './utils'; +import { escapeHtml, showError, showSuccess, validateGroupCombination } from './utils'; import { loadUsers } from './userActions'; import { openModal, closeModal } from '../modal'; @@ -119,6 +119,14 @@ export async function saveUser(e: Event): Promise { return; } + // Reject contradictory group combinations (view-only + write-capable) + // before hitting the API so the user gets an actionable message. + const groupConflict = validateGroupCombination(selectedGroups, availableGroups); + if (groupConflict) { + showError(groupConflict); + return; + } + try { if (currentEditingUser) { // Update existing user diff --git a/frontend/src/users/utils.ts b/frontend/src/users/utils.ts index 0e78640b8..8b5730628 100644 --- a/frontend/src/users/utils.ts +++ b/frontend/src/users/utils.ts @@ -19,8 +19,61 @@ export { formatRelativeTime, formatDate } from '../utils'; */ export { escapeHtml } from '../utils'; +import type { APIGroup } from '../api'; import { showToast } from '../toast'; +/** + * Validate that a proposed group-ID list does not contain a contradictory + * combination: a view-only group (all permissions carry action === 'view') + * paired with a write-capable group (at least one permission has a + * non-view action such as create/update/delete/execute/approve/admin). + * + * Because CUDly's permission model is additive (the user gets the union of + * all group permissions), pairing a view-only group with a write group is + * semantically contradictory: the intent of a "Viewer" group is that the + * user should have read-only access, but adding any write-capable group + * silently elevates them. The UI should surface this immediately rather + * than letting the combination go through. + * + * Groups with zero permissions are skipped (they grant nothing and do not + * participate in the conflict). + * + * Returns a human-readable error string when the combination is invalid, + * or null when it is acceptable. + * + * @param groupIds UUIDs of groups the user would be assigned + * @param allGroups Full list of loaded groups (from availableGroups) + */ +export function validateGroupCombination( + groupIds: string[], + allGroups: APIGroup[], +): string | null { + const groups = groupIds + .map(id => allGroups.find(g => g.id === id)) + .filter((g): g is APIGroup => g !== undefined); + + const viewOnlyGroups = groups.filter( + g => Array.isArray(g.permissions) && + g.permissions.length > 0 && + g.permissions.every(p => p.action === 'view'), + ); + const writeGroups = groups.filter( + g => Array.isArray(g.permissions) && + g.permissions.some(p => p.action !== 'view'), + ); + + if (viewOnlyGroups.length === 0 || writeGroups.length === 0) return null; + + const viewNames = viewOnlyGroups.map(g => `"${g.name}"`).join(', '); + const writeNames = writeGroups.map(g => `"${g.name}"`).join(', '); + return ( + `Contradictory group combination: ${viewNames} ` + + `${viewOnlyGroups.length === 1 ? 'is' : 'are'} view-only but ` + + `${writeNames} grant${writeGroups.length === 1 ? 's' : ''} write permissions. ` + + `Assign a view-only group or write-capable groups, not both.` + ); +} + /** * Show error message. Delegates to the shared toast system so callers * across the codebase get consistent bottom-right stacked toasts with From 3c0fe9f9e0f94ddf7872d47322edcb6dc09135ad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 02:53:43 +0300 Subject: [PATCH 2/2] fix(admin): enforce group-combination validation on the bulk-add path The #1405 validation added in this branch guarded the inline expand-panel toggle and the create/edit modal, but bulkAddToGroup (wired up by the #1408 bulk-actions bar) computed the resulting group set and called updateUser with no validation. Selecting a view-only-group user and bulk-adding a write group silently created the exact contradictory combination #1405 is meant to prevent. bulkAddToGroup now partitions the selected users: each user's resulting group set is run through validateGroupCombination before any API call. Contradictory users are skipped (never silently written) and named in an error toast so the admin knows which were left unchanged; they stay selected so the offending membership can be fixed and retried. Compatible users are applied and deselected as before. Regression tests (fail-before/pass-after, confirmed against pre-fix code): bulk-adding a write group to a view-only user does not call updateUser and surfaces the error naming the user; the skipped user stays selected; a mixed selection applies to the compatible user and skips only the contradictory one. --- frontend/src/__tests__/users.test.ts | 97 ++++++++++++++++++++++++++++ frontend/src/users/userActions.ts | 56 ++++++++++++---- 2 files changed, 140 insertions(+), 13 deletions(-) diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index 0aba0520a..503d2c2ba 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -1356,6 +1356,103 @@ describe('users/userActions', () => { expect(api.updateUser).toHaveBeenCalledTimes(1); expect(api.updateUser).toHaveBeenCalledWith('1', { groups: ['admins'] }); }); + + // ----------------------------------------------------------------------- + // Contradictory group combination guard on the bulk path (issue #1405). + // Regression: before the fix, bulkAddToGroup wrote the contradictory + // combination without validation. These assert the bulk path enforces + // the same rule as the inline expand panel and the modal. + // ----------------------------------------------------------------------- + describe('contradictory group combination guard (issue #1405)', () => { + const permGroups = [ + { + id: 'viewers', name: 'Viewers', + permissions: [{ action: 'view', resource: 'aws' }], + description: '', allowed_accounts: [], + }, + { + id: 'editors', name: 'Editors', + permissions: [{ action: 'create', resource: 'aws' }], + description: '', allowed_accounts: [], + }, + ]; + const permUsers = [ + // Alice already holds the view-only group. + { id: 'a', email: 'alice@test.com', groups: ['viewers'], mfa_enabled: false }, + // Bob has no groups yet. + { id: 'b', email: 'bob@test.com', groups: [], mfa_enabled: false }, + ]; + + beforeEach(() => { + userState.setAllUsers(permUsers as any); + userState.setAvailableGroups(permGroups as any); + (global.confirm as jest.Mock).mockReturnValue(true); + (api.listUsers as jest.Mock).mockResolvedValue({ users: permUsers }); + (api.listGroups as jest.Mock).mockResolvedValue({ groups: permGroups }); + }); + + it('does not call updateUser and shows an error when bulk-adding a write group to a view-only user', async () => { + userState.addSelectedUserId('a'); + + await userActions.bulkAddToGroup('editors'); + + // Pre-fix: this called updateUser('a', { groups: ['viewers', 'editors'] }). + // Post-fix: the contradictory combination is skipped, no API call. + expect(api.updateUser).not.toHaveBeenCalled(); + const toast = document.querySelector('.toast-error'); + expect(toast).toBeTruthy(); + expect(toast?.querySelector('.toast-message')?.textContent).toContain('contradictory'); + expect(toast?.querySelector('.toast-message')?.textContent).toContain('alice@test.com'); + }); + + it('keeps the skipped contradictory user selected so the admin can fix it', async () => { + userState.addSelectedUserId('a'); + + await userActions.bulkAddToGroup('editors'); + + expect(userState.selectedUserIds.has('a')).toBe(true); + }); + + it('applies to compatible users and skips only the contradictory ones in a mixed selection', async () => { + userState.addSelectedUserId('a'); // view-only -> would conflict + userState.addSelectedUserId('b'); // no groups -> safe to add + + await userActions.bulkAddToGroup('editors'); + + // Bob gets the write group; Alice is skipped. + expect(api.updateUser).toHaveBeenCalledTimes(1); + expect(api.updateUser).toHaveBeenCalledWith('b', { groups: ['editors'] }); + // Compatible user is deselected, contradictory user stays selected. + expect(userState.selectedUserIds.has('b')).toBe(false); + expect(userState.selectedUserIds.has('a')).toBe(true); + // The error toast names the skipped user. + expect(document.querySelector('.toast-error')?.textContent).toContain('alice@test.com'); + }); + + it('bulk-adding a compatible write group to a user with only write groups still works', async () => { + const writeOnly = [ + { id: 'c', email: 'carol@test.com', groups: ['editors'], mfa_enabled: false }, + ]; + const extraGroups = [ + ...permGroups, + { + id: 'admins2', name: 'Admins2', + permissions: [{ action: 'delete', resource: 'aws' }], + description: '', allowed_accounts: [], + }, + ]; + userState.setAllUsers(writeOnly as any); + userState.setAvailableGroups(extraGroups as any); + (api.listUsers as jest.Mock).mockResolvedValue({ users: writeOnly }); + (api.listGroups as jest.Mock).mockResolvedValue({ groups: extraGroups }); + userState.addSelectedUserId('c'); + + await userActions.bulkAddToGroup('admins2'); + + expect(api.updateUser).toHaveBeenCalledWith('c', { groups: ['editors', 'admins2'] }); + expect(document.querySelector('.toast-error')).toBeFalsy(); + }); + }); }); }); diff --git a/frontend/src/users/userActions.ts b/frontend/src/users/userActions.ts index 0e1eda65b..ab1a3bc45 100644 --- a/frontend/src/users/userActions.ts +++ b/frontend/src/users/userActions.ts @@ -11,9 +11,10 @@ import { selectedUserIds, setAllUsers, setAvailableGroups, - clearSelectedUserIds + clearSelectedUserIds, + removeSelectedUserId } from './state'; -import { showError, showSuccess } from './utils'; +import { showError, showSuccess, validateGroupCombination } from './utils'; import { confirmDialog } from '../confirmDialog'; import { applyFilters } from './filters'; import { renderUsers, renderUserStats, populateBulkGroupSelect } from './userList'; @@ -200,21 +201,50 @@ export async function bulkAddToGroup(groupId: string): Promise { return; } - try { - // Get current user data and add group - const updates = Array.from(selectedUserIds).map(async userId => { - const user = allUsers.find(u => u.id === userId); - if (!user) return; + // Partition the selected users by whether adding this group would produce a + // contradictory combination (view-only group + write-capable group, issue + // #1405). The bulk path must enforce the same rule as the inline expand + // panel and the create/edit modal -- otherwise selecting a Viewer-group + // user and bulk-adding a write group silently creates the exact + // combination #1405 is meant to prevent. Contradictory users are skipped + // (never silently written) and named in an error toast so the admin knows + // which ones were left unchanged. + const applicable: { userId: string; groups: string[] }[] = []; + const skipped: string[] = []; + for (const userId of selectedUserIds) { + const user = allUsers.find(u => u.id === userId); + if (!user) continue; + + const updatedGroups = [...new Set([...user.groups, groupId])]; + if (validateGroupCombination(updatedGroups, availableGroups)) { + skipped.push(user.email); + } else { + applicable.push({ userId, groups: updatedGroups }); + } + } + + if (skipped.length > 0) { + showError( + `Skipped ${skipped.length} user(s) whose group set would be contradictory: ` + + `${skipped.join(', ')}. A view-only group cannot be combined with ` + + `write-capable groups. Remove the conflicting membership first.` + ); + } - const updatedGroups = [...new Set([...user.groups, groupId])]; - await api.updateUser(userId, { groups: updatedGroups }); - }); + // Nothing safe to apply -- leave the selection intact so the admin can + // adjust the offending memberships and retry. + if (applicable.length === 0) return; - await Promise.all(updates); + try { + await Promise.all( + applicable.map(({ userId, groups }) => api.updateUser(userId, { groups })), + ); - clearSelectedUserIds(); + // Deselect only the successfully-applied users so any skipped + // (contradictory) users stay selected for the admin to fix. + applicable.forEach(({ userId }) => removeSelectedUserId(userId)); await loadUsers(); - showSuccess(`Successfully added ${count} user(s) to group`); + showSuccess(`Successfully added ${applicable.length} user(s) to group`); } catch (error) { console.error('Failed to add users to group:', error); showError('Failed to add some users to group');