From a16dafe5583cf4c04c14690e2abc788c50eeb813 Mon Sep 17 00:00:00 2001 From: Caroline Desprat Date: Tue, 6 Oct 2026 14:01:45 +0000 Subject: [PATCH 01/15] feat(users): search, confirm-before-wide-grant and block redundant scopes in the roles modal Fixes the instance-wide grant sending domain=* for instance_admin instead of omitting it entirely. Adds a search filter over the granted-roles table, a confirmation step (listing what will be revoked) before an instance/domain-wide grant replaces a user's other roles, and hides/disables project or domain scope options once a wider role already covers them, with a tooltip explaining why. --- src/lang/en.json | 11 +- .../SettingsViewUsersRolesCascadeModal.vue | 54 ++++++++ .../SettingsViewUsersRolesModal.vue | 105 ++++++++++++--- ...SettingsViewUsersRolesCascadeModal.spec.js | 55 ++++++++ .../SettingsViewUsersRolesModal.spec.js | 121 +++++++++++++++++- 5 files changed, 324 insertions(+), 22 deletions(-) create mode 100644 src/views/Settings/SettingsView/SettingsViewUsersRolesCascadeModal.vue create mode 100644 tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersRolesCascadeModal.spec.js diff --git a/src/lang/en.json b/src/lang/en.json index 9be6484371..bbd42f693e 100644 --- a/src/lang/en.json +++ b/src/lang/en.json @@ -1772,6 +1772,8 @@ "searchPlaceholder": "Search scopes", "noResults": "No project or role matches your search.", "selectScope": "Select scope", + "scopePickerDisabledWideRole": "This user already has a role covering every project. Revoke it first to grant a project-specific role.", + "scopePickerDisabledNoOptions": "This user already has a role on every available project.", "scopeColumn": "Scope", "roleColumn": "Role", "scope": { @@ -1786,7 +1788,14 @@ "revokeSuccessOnProject": "Revoked {role} on {project} from {uid}", "revokeErrorOnProject": "Failed to revoke {role} on {project} from {uid}.", "grantSuccessOnProject": "Granted {role} on {project} to {uid}", - "grantErrorOnProject": "Failed to grant {role} on {project} to {uid}." + "grantErrorOnProject": "Failed to grant {role} on {project} to {uid}.", + "cascadeModal": { + "title": "Replace existing grants?", + "body": "This role already covers every project, so these existing grants will be revoked:", + "confirm": "Grant and revoke the rest", + "cancel": "Cancel", + "cleanupError": "The role was granted, but some of the previous grants could not be revoked." + } } }, "snapshots": { diff --git a/src/views/Settings/SettingsView/SettingsViewUsersRolesCascadeModal.vue b/src/views/Settings/SettingsView/SettingsViewUsersRolesCascadeModal.vue new file mode 100644 index 0000000000..6921af0a32 --- /dev/null +++ b/src/views/Settings/SettingsView/SettingsViewUsersRolesCascadeModal.vue @@ -0,0 +1,54 @@ + + + diff --git a/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue b/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue index 2e8c580709..04690e9c39 100644 --- a/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue +++ b/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue @@ -16,6 +16,7 @@ import ProjectButton from '@/components/Project/ProjectButton.vue' import ProjectDropdownSelector from '@/components/Project/ProjectDropdownSelector/ProjectDropdownSelector.vue' import ProjectUsersRoleDropdown from '@/components/ProjectUsers/ProjectUsersRoleDropdown.vue' import SettingsViewUsersNotFound from '@/views/Settings/SettingsView/SettingsViewUsersNotFound.vue' +import SettingsViewUsersRolesCascadeModal from '@/views/Settings/SettingsView/SettingsViewUsersRolesCascadeModal.vue' import { useAuth } from '@/composables/useAuth.js' import { usePolicies } from '@/composables/usePolicies.js' @@ -98,14 +99,21 @@ const emptyLabel = computed(() => const assignedProjects = computed(() => new Set(roles.value.map(({ project }) => project))) -const availableProjects = computed(() => - core.projects +// An instance or domain admin grant already covers every project: offering a project-specific +// grant on top would be dead data (and reappear as a surprise if the wide role is later +// revoked), so no project entry is offered while the user holds either. +const targetHasWideRole = computed(() => roles.value.some(({ role }) => isInstanceOrDomainRole(role))) + +const availableProjects = computed(() => { + if (targetHasWideRole.value) return [] + return core.projects .filter(({ name }) => !assignedProjects.value.has(name)) .sort((a, b) => displayLabelOf(a).localeCompare(displayLabelOf(b))) -) +}) const canGrantInstanceRole = computed(() => isInstanceAdmin.value && !roles.value.some(({ role }) => role === ROLE.INSTANCE_ADMIN)) -const canGrantDomainRole = computed(() => isInstanceAdmin.value && !roles.value.some(({ role }) => role === ROLE.DOMAIN_ADMIN)) +// Domain admin is strictly weaker than instance admin, so it's not offered on top of it either. +const canGrantDomainRole = computed(() => isInstanceAdmin.value && !roles.value.some(({ role }) => role === ROLE.DOMAIN_ADMIN || role === ROLE.INSTANCE_ADMIN)) const instanceScopeEntry = computed(() => ({ name: INSTANCE_SCOPE, label: t('settings.users.rolesModal.scope.instance') })) const domainScopeEntry = computed(() => ({ name: DOMAIN_SCOPE, label: t('settings.users.rolesModal.scope.domain') })) @@ -116,6 +124,16 @@ const projectPickerOptions = computed(() => [ ...(isAuthWithUsersProvider.value ? availableProjects.value : []) ]) +// Explains the disabled scope picker: either this user already holds a role covering every +// project, or every project already has a grant and this viewer cannot offer instance/domain +// scope. Not shown while merely mid-save, since that disablement is unrelated and temporary. +const scopePickerDisabledTitle = computed(() => { + if (saving.value || projectPickerOptions.value.length) return null + return targetHasWideRole.value + ? t('settings.users.rolesModal.scopePickerDisabledWideRole') + : t('settings.users.rolesModal.scopePickerDisabledNoOptions') +}) + function scopeEntry({ role }) { return role === ROLE.DOMAIN_ADMIN ? domainScopeEntry.value : instanceScopeEntry.value } @@ -170,6 +188,14 @@ function toastMessage(key, role, project) { return t(`settings.users.rolesModal.${key}${isProjectScope ? 'OnProject' : ''}`, params) } +function revokeGrant(item) { + if (isInstanceOrDomainRole(item.role)) { + const domain = item.role === ROLE.DOMAIN_ADMIN ? item.domain : null + return core.api.revokeInstanceRole(props.user.uid, ROLE_LOWERCASE[item.role], domain) + } + return core.api.revokeUserRole(props.user.uid, item.project, { ifExists: true }) +} + // Only project rows have a role picker (instance/domain rows show a fixed badge: they're // separate grants, revoke and grant again to switch). grantUserRole overwrites the existing // role for that user/project. @@ -193,13 +219,7 @@ async function revokeRole(item) { if (!canRevoke(item)) return saving.value = true try { - if (isInstanceOrDomainRole(item.role)) { - const domain = item.role === ROLE.DOMAIN_ADMIN ? item.domain : null - await core.api.revokeInstanceRole(props.user.uid, ROLE_LOWERCASE[item.role], domain) - } - else { - await core.api.revokeUserRole(props.user.uid, item.project, { ifExists: true }) - } + await revokeGrant(item) toast.success(toastMessage('revokeSuccess', item.role, item.project)) emit('user:updated', { uid: props.user.uid }) } @@ -211,14 +231,39 @@ async function revokeRole(item) { } } +// An instance or domain admin role gives access to every project of its scope: any grant the +// user already holds becomes redundant, so granting one of these roles revokes every other grant +// after a confirmation step (see SettingsViewUsersRolesCascadeModal). +const showCascadeModal = ref(false) +const cascadeGrants = ref([]) + async function grantRole() { if (!canGrant.value) return + if ((isInstanceScope.value || isDomainScope.value) && roles.value.length) { + cascadeGrants.value = roles.value + showCascadeModal.value = true + return + } + await performGrant() +} + +function onCascadeConfirm() { + return performGrant() +} + +async function performGrant() { saving.value = true try { if (isInstanceScope.value || isDomainScope.value) { // The domain only matters for DOMAIN_ADMIN; the backend ignores it for INSTANCE_ADMIN. const domain = selectedRole.value === ROLE.DOMAIN_ADMIN ? DEFAULT_DOMAIN : null await core.api.grantInstanceRole(props.user.uid, ROLE_LOWERCASE[selectedRole.value], domain) + if (cascadeGrants.value.length) { + const results = await Promise.allSettled(cascadeGrants.value.map(revokeGrant)) + if (results.some(result => result.status === 'rejected')) { + toast.error(t('settings.users.rolesModal.cascadeModal.cleanupError')) + } + } } else { await core.api.grantUserRole(props.user.uid, selectedProjectName.value, ROLE_LOWERCASE[selectedRole.value]) @@ -232,6 +277,7 @@ async function grantRole() { } finally { saving.value = false + cascadeGrants.value = [] } } @@ -243,6 +289,7 @@ defineExpose({ projectPickerOptions, canGrantInstanceRole, canGrantDomainRole, + scopePickerDisabledTitle, canRevoke, isInstanceScope, isDomainScope, @@ -251,6 +298,9 @@ defineExpose({ selectedRole, selectedProjectName, canGrant, + showCascadeModal, + cascadeGrants, + onCascadeConfirm, revokeRole, grantRole, changeRole @@ -309,14 +359,25 @@ defineExpose({ diff --git a/tests/unit/specs/components/RowPagination/RowPagination.spec.js b/tests/unit/specs/components/RowPagination/RowPagination.spec.js new file mode 100644 index 0000000000..d4e76fec49 --- /dev/null +++ b/tests/unit/specs/components/RowPagination/RowPagination.spec.js @@ -0,0 +1,22 @@ +import { shallowMount } from '@vue/test-utils' + +import RowPagination from '@/components/RowPagination/RowPagination.vue' + +describe('RowPagination.vue', () => { + function mountComponent(props = {}) { + return shallowMount(RowPagination, { props: { totalRows: 10, ...props } }) + } + + // With zero rows, TinyPagination's row-number input is merely disabled, not hidden, and still + // shows a literal "0" next to the "of 0 ..." label - reading as "0 of 0 users". The + // `row-pagination--empty` class hides that now-meaningless input via CSS. + it('flags itself as empty when there are no rows, to hide the row-number input', () => { + const wrapper = mountComponent({ totalRows: 0 }) + expect(wrapper.classes()).toContain('row-pagination--empty') + }) + + it('does not flag itself as empty when there are rows', () => { + const wrapper = mountComponent({ totalRows: 10 }) + expect(wrapper.classes()).not.toContain('row-pagination--empty') + }) +}) From f39a12c90532831d5f75d121094258c054be0e87 Mon Sep 17 00:00:00 2001 From: Caroline Desprat Date: Tue, 6 Oct 2026 16:27:36 +0000 Subject: [PATCH 08/15] fix(users): flag the username field inline on a 409 conflict A duplicate username only showed a toast, with no indication on the field itself; the username input now gets :state="false" and an inline message, cleared as soon as the username is edited again. --- .../SettingsViewUsersCreateModal.vue | 16 +++++++++++++ .../SettingsViewUsersCreateModal.spec.js | 24 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/src/views/Settings/SettingsView/SettingsViewUsersCreateModal.vue b/src/views/Settings/SettingsView/SettingsViewUsersCreateModal.vue index b470b3a617..39e45cdd29 100644 --- a/src/views/Settings/SettingsView/SettingsViewUsersCreateModal.vue +++ b/src/views/Settings/SettingsView/SettingsViewUsersCreateModal.vue @@ -30,6 +30,12 @@ const email = ref('') const name = ref('') const { password, confirmPassword, passwordMismatch, isPasswordValid, clearPasswords } = usePasswordConfirm() const saving = ref(false) +// Set on a 409 from the server; cleared as soon as the user edits the username again, since the +// stale conflict no longer necessarily applies to whatever they're about to submit. +const usernameConflict = ref(false) +watch(username, () => { + usernameConflict.value = false +}) const isValid = computed(() => { if (!username.value.trim().length) return false @@ -42,6 +48,7 @@ function resetForm() { username.value = '' email.value = '' name.value = '' + usernameConflict.value = false clearPasswords() } @@ -76,6 +83,7 @@ async function saveUser(bvModalEvent) { catch (err) { const status = err?.response?.status ?? err?.request?.response?.status if (status === 409) { + usernameConflict.value = true toast.error(t('settings.users.create.saveErrorConflict')) } else { @@ -95,6 +103,7 @@ defineExpose({ confirmPassword, isValid, passwordMismatch, + usernameConflict, saving, saveUser, form @@ -126,9 +135,16 @@ defineExpose({ v-model="username" :placeholder="t('settings.users.create.fields.username.placeholder')" :disabled="saving" + :state="usernameConflict ? false : null" autofocus name="uid" /> + + {{ t('settings.users.create.saveErrorConflict') }} + { expect(mockToast.error).toHaveBeenCalledWith('This username is already taken.') }) + it('flags the username field inline on a 409 conflict, not just a toast', async () => { + mockApi.createUser.mockRejectedValue({ response: { status: 409 } }) + const wrapper = mountComponent() + stubFormValidity(wrapper, true) + fillRequiredFields(wrapper) + await wrapper.vm.saveUser() + expect(wrapper.vm.usernameConflict).toBe(true) + expect(wrapper.findComponent(BFormInput).props('state')).toBe(false) + expect(wrapper.text()).toContain('This username is already taken.') + }) + + it('clears the inline username conflict once the username is edited again', async () => { + mockApi.createUser.mockRejectedValue({ response: { status: 409 } }) + const wrapper = mountComponent() + stubFormValidity(wrapper, true) + fillRequiredFields(wrapper) + await wrapper.vm.saveUser() + expect(wrapper.vm.usernameConflict).toBe(true) + wrapper.vm.username = 'someone-else' + await wrapper.vm.$nextTick() + expect(wrapper.vm.usernameConflict).toBe(false) + }) + it('shows the generic error toast when createUser fails with a non-conflict error', async () => { mockApi.createUser.mockRejectedValue({ response: { status: 500 } }) const wrapper = mountComponent() From f28cf99b9df1814e5e6ac456c00050733b06c402 Mon Sep 17 00:00:00 2001 From: Caroline Desprat Date: Tue, 6 Oct 2026 16:27:46 +0000 Subject: [PATCH 09/15] fix(users): stop using v-html for the delete-modal consequence list Each item embedded a verb as a raw HTML string rendered via v-html, which vue-i18n flags as an XSS-prone pattern. Reworks each string around a {verb} slot and renders through i18n-t, same pattern the modal title already uses for its own embedded component. --- src/lang/en.json | 9 ++++--- .../SettingsViewUsersDeleteModal.vue | 27 ++++++++++++++++--- .../SettingsViewUsersDeleteModal.spec.js | 2 +- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/src/lang/en.json b/src/lang/en.json index 8896628835..68767aa060 100644 --- a/src/lang/en.json +++ b/src/lang/en.json @@ -1755,9 +1755,12 @@ "title": "Delete user {name}?", "body": { "intro": "This action cannot be undone. Deleting this account will:", - "rolesRevoked": "remove all of their role grants (project, domain and instance);", - "dataDeleted": "delete all their personal data (stars, history, tags, saved searches etc.) across all projects;", - "tasksKept": "keep their tasks and task results." + "rolesRevoked": "{verb} all of their role grants (project, domain and instance);", + "rolesRevokedVerb": "remove", + "dataDeleted": "{verb} all their personal data (stars, history, tags, saved searches etc.) across all projects;", + "dataDeletedVerb": "delete", + "tasksKept": "{verb} their tasks and task results.", + "tasksKeptVerb": "keep" }, "confirm": "Delete user", "success": "User deleted successfully", diff --git a/src/views/Settings/SettingsView/SettingsViewUsersDeleteModal.vue b/src/views/Settings/SettingsView/SettingsViewUsersDeleteModal.vue index c08c0fd185..acd5b52ad3 100644 --- a/src/views/Settings/SettingsView/SettingsViewUsersDeleteModal.vue +++ b/src/views/Settings/SettingsView/SettingsViewUsersDeleteModal.vue @@ -81,9 +81,30 @@ defineExpose({ confirmDeletion }) {{ t('settings.users.deleteModal.body.intro') }}

    -
  • -
  • -
  • + + + + + + + + +
diff --git a/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersDeleteModal.spec.js b/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersDeleteModal.spec.js index 869ce54181..aee7ca130f 100644 --- a/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersDeleteModal.spec.js +++ b/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersDeleteModal.spec.js @@ -80,7 +80,7 @@ describe('SettingsViewUsersDeleteModal.vue', () => { it('bolds the key verb of each consequence, and says tasks are kept', () => { const wrapper = shallowMount(SettingsViewUsersDeleteModal, { - global: { ...global, renderStubDefaultSlot: true }, + global: { ...global, renderStubDefaultSlot: true, stubs: { 'i18n-t': false } }, props: { user, modelValue: true } }) const verbs = wrapper.findAll('li strong').map(el => el.text()) From a12715f328ba40b569f5941bf562a41eceb6542c Mon Sep 17 00:00:00 2001 From: Caroline Desprat Date: Tue, 6 Oct 2026 16:28:02 +0000 Subject: [PATCH 10/15] fix(users): show an inline error for a malformed email on edit type="email" already blocked the save via native checkValidity(), but nothing in the page said why - only the browser's own tooltip. Adds a visible inline message, same pattern as the password-mismatch field. --- src/lang/en.json | 3 ++- .../SettingsView/SettingsViewUsersEditModal.vue | 13 +++++++++++++ .../SettingsViewUsersEditModal.spec.js | 17 +++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/lang/en.json b/src/lang/en.json index 68767aa060..f875fe66a3 100644 --- a/src/lang/en.json +++ b/src/lang/en.json @@ -1738,7 +1738,8 @@ }, "email": { "label": "Email", - "placeholder": "Email" + "placeholder": "Email", + "invalid": "Enter a valid email address." }, "password": { "label": "Password", diff --git a/src/views/Settings/SettingsView/SettingsViewUsersEditModal.vue b/src/views/Settings/SettingsView/SettingsViewUsersEditModal.vue index 993e0757e1..36ad0b02be 100644 --- a/src/views/Settings/SettingsView/SettingsViewUsersEditModal.vue +++ b/src/views/Settings/SettingsView/SettingsViewUsersEditModal.vue @@ -40,6 +40,11 @@ const { t } = useI18n() const name = ref('') const email = ref('') const resetPassword = ref(false) +// Native checkValidity() already blocks the save on a malformed email (type="email"), but that +// only surfaces as a browser tooltip - nothing in the page itself says why. This mirrors it +// visibly, same pattern as the password-mismatch message below. +const EMAIL_PATTERN = /^[^\s@]+@[^\s@]+\.[^\s@]+$/ +const emailInvalid = computed(() => email.value.trim().length > 0 && !EMAIL_PATTERN.test(email.value.trim())) const { password, confirmPassword, passwordMismatch, isPasswordValid, clearPasswords } = usePasswordConfirm() const saving = ref(false) @@ -105,6 +110,7 @@ defineExpose({ confirmPassword, isValid, hasChanges, + emailInvalid, saveUser, form }) @@ -190,10 +196,17 @@ defineExpose({ v-model="email" :placeholder="t('settings.users.edit.fields.email.placeholder')" :disabled="saving" + :state="emailInvalid ? false : null" aria-required="true" type="email" name="email" /> + + {{ t('settings.users.edit.fields.email.invalid') }} + { expect(wrapper.vm.email).toBe('bob@example.org') }) + it('shows an inline error when the email is malformed, not just the native tooltip', async () => { + const wrapper = mountComponent() + wrapper.vm.email = 'not-an-email' + await wrapper.vm.$nextTick() + expect(wrapper.vm.emailInvalid).toBe(true) + const emailInput = wrapper.findAllComponents(BFormInput).find(c => c.attributes('name') === 'email') + expect(emailInput.props('state')).toBe(false) + expect(wrapper.text()).toContain('Enter a valid email address.') + }) + + it('does not flag the email as invalid while it is empty or well-formed', () => { + const wrapper = mountComponent() + expect(wrapper.vm.emailInvalid).toBe(false) + wrapper.vm.email = '' + expect(wrapper.vm.emailInvalid).toBe(false) + }) + it('isValid is false when name is empty', async () => { const wrapper = mountComponent() wrapper.vm.name = '' From 9a204c73055b9deb5b048d7998f3ed3429cb5ef2 Mon Sep 17 00:00:00 2001 From: Caroline Desprat Date: Tue, 6 Oct 2026 16:28:35 +0000 Subject: [PATCH 11/15] fix(users): refresh on open, confirm wide-role revokes, fix empty-state text - Refresh the user data every time the roles modal opens, instead of trusting whatever stale `user` prop the parent happened to have (a role changed elsewhere, e.g. via the CLI, while the modal sat open went unnoticed). - Confirm before revoking an instance/domain admin role, since it removes access to every project it covered, not just one row; plain project revokes stay single-click. - "This user has no project role grants yet." reworded to "no role grants", since the same modal also manages domain and instance grants. --- src/lang/en.json | 3 +- .../SettingsViewUsersRolesModal.vue | 20 ++++++ .../SettingsViewUsersRolesModal.spec.js | 61 +++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/lang/en.json b/src/lang/en.json index f875fe66a3..be50550ebc 100644 --- a/src/lang/en.json +++ b/src/lang/en.json @@ -1771,7 +1771,7 @@ "rolesModal": { "title": "Manage roles for {uid}", "close": "Close", - "empty": "This user has no project role grants yet.", + "empty": "This user has no role grants yet.", "searchPlaceholder": "Search scopes", "noResults": "No project or role matches your search.", "selectScope": "Select scope", @@ -1785,6 +1785,7 @@ "domain": "Domain" }, "grant": "Grant", + "revokeWideRoleConfirm": "Revoke {role} from {uid}? This immediately removes their access to every project it covered.", "revokeSuccess": "Revoked {role} from {uid}", "revokeError": "Failed to revoke {role} from {uid}.", "grantSuccess": "Granted {role} to {uid}", diff --git a/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue b/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue index 828ccafe5b..74b3dc180d 100644 --- a/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue +++ b/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue @@ -19,6 +19,7 @@ import SettingsViewUsersNotFound from '@/views/Settings/SettingsView/SettingsVie import SettingsViewUsersRolesCascadeModal from '@/views/Settings/SettingsView/SettingsViewUsersRolesCascadeModal.vue' import { useAuth } from '@/composables/useAuth.js' +import { useConfirmModal } from '@/composables/useConfirmModal.js' import { usePolicies } from '@/composables/usePolicies.js' import { useCore } from '@/composables/useCore.js' import { useToast } from '@/composables/useToast.js' @@ -55,6 +56,7 @@ const core = useCore() const { toast } = useToast() const { t } = useI18n() const { isInstanceAdmin, formatRole } = usePolicies() +const { confirm } = useConfirmModal() // Under OAuth, project membership comes from the identity provider's groups and is reconciled at // each login: a project role revoked here comes back, and one granted on a project the provider // does not list goes away. The role level on a project it does list is kept, so it can still be @@ -72,6 +74,14 @@ watch(() => props.user?.uid, () => { resetAddForm() }) +// The modal only ever shows whatever `roles` its `user` prop was given, which the parent only +// refreshes when this modal itself grants/revokes something (`user:updated`). A role changed +// elsewhere (another admin tab, the CLI) while this modal happened to be open would go unnoticed +// until something inside it triggers that refresh - so ask for one on every open too. +watch(modelValue, (visible) => { + if (visible) emit('user:updated', { uid: props.user.uid }) +}) + // Each permission is { v1: role, v2: 'domain::project' }, parsed into { role, domain, project }: the // domain is sent when revoking a domain admin grant and orders the rows. Sorted by role rank, then // A-Z (see compareGrants). @@ -224,8 +234,18 @@ async function changeRole(item, newRole) { } } +// An instance/domain admin grant covers every project: confirm before revoking it, since it's +// not just removing one row but immediately removing access to everything that role covered. +// Plain project-level revokes stay a single click, like everywhere else in this table. async function revokeRole(item) { if (!canRevoke(item)) return + if (isInstanceOrDomainRole(item.role)) { + const description = t('settings.users.rolesModal.revokeWideRoleConfirm', { + role: formatRole(t, item.role), + uid: props.user.uid + }) + if (!(await confirm({ description }))) return + } saving.value = true try { await revokeGrant(item) diff --git a/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersRolesModal.spec.js b/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersRolesModal.spec.js index 7dc4bd7c8e..848196c982 100644 --- a/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersRolesModal.spec.js +++ b/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersRolesModal.spec.js @@ -15,6 +15,11 @@ vi.mock('@/composables/useToast', () => ({ useToast: () => ({ toast: mockToast }) })) +const mockConfirm = vi.fn().mockResolvedValue(true) +vi.mock('@/composables/useConfirmModal', () => ({ + useConfirmModal: () => ({ confirm: mockConfirm }) +})) + const mockApi = { grantUserRole: vi.fn(), revokeUserRole: vi.fn(), @@ -194,6 +199,43 @@ describe('SettingsViewUsersRolesModal.vue', () => { ) }) + it('revokes a plain project role without asking for confirmation', async () => { + const wrapper = await mountResolved() + await wrapper.vm.revokeRole({ project: 'project-a', role: 'PROJECT_MEMBER', domain: 'default' }) + expect(mockConfirm).not.toHaveBeenCalled() + }) + + describe('revoking an instance or domain admin role', () => { + const wideRoleUser = { + uid: 'alice@example.org', + permissions: [{ v1: 'INSTANCE_ADMIN', v2: '*::*' }] + } + + beforeEach(() => { + core.config.set('policies', [{ projectId: '*', domainId: '*', role: 'INSTANCE_ADMIN' }]) + }) + + it('asks for confirmation before revoking, naming the role and the user', async () => { + const wrapper = await mountResolved({ user: wideRoleUser }) + await wrapper.vm.revokeRole({ project: '*', role: 'INSTANCE_ADMIN', domain: '*' }) + expect(mockConfirm).toHaveBeenCalledWith({ + description: core.i18n.global.t('settings.users.rolesModal.revokeWideRoleConfirm', { + role: 'Instance admin', + uid: 'alice@example.org' + }) + }) + expect(mockApi.revokeInstanceRole).toHaveBeenCalled() + }) + + it('does not revoke when the confirmation is declined', async () => { + mockConfirm.mockResolvedValueOnce(false) + const wrapper = await mountResolved({ user: wideRoleUser }) + await wrapper.vm.revokeRole({ project: '*', role: 'INSTANCE_ADMIN', domain: '*' }) + expect(mockApi.revokeInstanceRole).not.toHaveBeenCalled() + expect(wrapper.emitted('user:updated')).toBeFalsy() + }) + }) + // The granted-roles table reads props.user, which the parent refreshes on user:updated. It used // to refetch the user itself through GET /api/users/admin/:uid, which carries no permissions at // all, so the whole list emptied out after every grant. @@ -786,6 +828,25 @@ describe('SettingsViewUsersRolesModal.vue', () => { }) }) + describe('opening the modal', () => { + // A role changed elsewhere (another admin tab, the CLI) while the modal happened to be open + // would otherwise sit stale until this modal itself grants/revokes something. + it('asks the parent to refresh the user data every time the modal opens', async () => { + const wrapper = shallowMount(SettingsViewUsersRolesModal, { + global, + props: { modelValue: false, user } + }) + await wrapper.setProps({ modelValue: true }) + expect(wrapper.emitted('user:updated')).toEqual([[{ uid: 'alice@example.org' }]]) + }) + + it('does not emit user:updated while already open or while closing', async () => { + const wrapper = await mountResolved() + await wrapper.setProps({ modelValue: false }) + expect(wrapper.emitted('user:updated')).toBeFalsy() + }) + }) + describe('while a change is being saved', () => { it('keeps the rows and the add row disabled until the request settles', async () => { let resolveRevoke From 4b123e164a99f5e143546505b818330777c1aed0 Mon Sep 17 00:00:00 2001 From: Caroline Desprat Date: Tue, 6 Oct 2026 16:35:05 +0000 Subject: [PATCH 12/15] fix(users): leave grant cleanup to the backend on a wide grant The backend now deletes the grants a wide role replaces when it is granted: project grants, and domain admin under instance admin. The roles modal no longer revokes them itself after granting, which also drops the partial-failure path and its cleanup error toast. The confirmation still lists what will be replaced, and now says those grants will not come back if the role is revoked later. --- src/lang/en.json | 5 ++-- .../SettingsViewUsersRolesModal.vue | 15 +++-------- .../SettingsViewUsersRolesModal.spec.js | 25 ++++++++----------- 3 files changed, 16 insertions(+), 29 deletions(-) diff --git a/src/lang/en.json b/src/lang/en.json index be50550ebc..f218e62446 100644 --- a/src/lang/en.json +++ b/src/lang/en.json @@ -1796,10 +1796,9 @@ "grantErrorOnProject": "Failed to grant {role} on {project} to {uid}.", "cascadeModal": { "title": "Replace existing grants?", - "body": "This role already covers every project, so these existing grants will be revoked:", + "body": "This role already covers every project, so these existing grants will be revoked. They will not come back if this role is revoked later:", "confirm": "Grant and revoke the rest", - "cancel": "Cancel", - "cleanupError": "The role was granted, but some of the previous grants could not be revoked." + "cancel": "Cancel" } } }, diff --git a/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue b/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue index 74b3dc180d..29efe190f8 100644 --- a/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue +++ b/src/views/Settings/SettingsView/SettingsViewUsersRolesModal.vue @@ -261,8 +261,9 @@ async function revokeRole(item) { } // An instance or domain admin role gives access to every project of its scope: any grant the -// user already holds becomes redundant, so granting one of these roles revokes every other grant -// after a confirmation step (see SettingsViewUsersRolesCascadeModal). +// user already holds becomes redundant, so granting one of these roles replaces every other grant +// after a confirmation step (see SettingsViewUsersRolesCascadeModal). The backend deletes the +// replaced grants itself on a wide grant: project grants, and domain admin under instance admin. // TODO #DOMAIN: once multiple domains exist, a domain-admin grant should only cascade-revoke // grants within that domain, not every grant regardless of domain; harmless today since only one // domain exists. @@ -290,16 +291,6 @@ async function performGrant() { // The domain only matters for DOMAIN_ADMIN; the backend ignores it for INSTANCE_ADMIN. const domain = selectedRole.value === ROLE.DOMAIN_ADMIN ? DEFAULT_DOMAIN : null await core.api.grantInstanceRole(props.user.uid, ROLE_LOWERCASE[selectedRole.value], domain) - // The cascade-revoke below is this component's own invariant, not enforced by the - // grantInstanceRole endpoint itself: any other caller (a script, another admin screen) - // granting a wide role would leave stale project grants behind. Moving this server-side - // would need backend work beyond this component. - if (cascadeGrants.value.length) { - const results = await Promise.allSettled(cascadeGrants.value.map(revokeGrant)) - if (results.some(result => result.status === 'rejected')) { - toast.error(t('settings.users.rolesModal.cascadeModal.cleanupError')) - } - } } else { await core.api.grantUserRole(props.user.uid, selectedProjectName.value, ROLE_LOWERCASE[selectedRole.value]) diff --git a/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersRolesModal.spec.js b/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersRolesModal.spec.js index 848196c982..89356446be 100644 --- a/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersRolesModal.spec.js +++ b/tests/unit/specs/views/Settings/SettingsView/SettingsViewUsersRolesModal.spec.js @@ -702,35 +702,32 @@ describe('SettingsViewUsersRolesModal.vue', () => { ]) }) - it('grants the role and revokes every existing grant on confirm', async () => { + it('grants the role on confirm and leaves the project grants to the backend', async () => { const wrapper = await mountResolved() await selectInstanceScope(wrapper) await wrapper.vm.grantRole() await wrapper.vm.onCascadeConfirm() expect(mockApi.grantInstanceRole).toHaveBeenCalledWith('alice@example.org', 'instance_admin', null) - expect(mockApi.revokeUserRole).toHaveBeenCalledWith('alice@example.org', 'project-a', { ifExists: true }) - expect(mockApi.revokeUserRole).toHaveBeenCalledWith('alice@example.org', 'project-b', { ifExists: true }) + expect(mockApi.revokeUserRole).not.toHaveBeenCalled() expect(wrapper.emitted('user:updated')).toBeTruthy() }) - it('grants nothing while waiting for confirmation', async () => { - const wrapper = await mountResolved() + it('leaves a domain admin grant superseded by instance admin to the backend', async () => { + const permissions = [{ v1: 'DOMAIN_ADMIN', v2: 'default::*' }] + const wrapper = await mountResolved({ user: { uid: 'alice@example.org', permissions } }) await selectInstanceScope(wrapper) await wrapper.vm.grantRole() - expect(mockApi.grantInstanceRole).not.toHaveBeenCalled() - expect(mockApi.revokeUserRole).not.toHaveBeenCalled() + await wrapper.vm.onCascadeConfirm() + expect(mockApi.grantInstanceRole).toHaveBeenCalledWith('alice@example.org', 'instance_admin', null) + expect(mockApi.revokeInstanceRole).not.toHaveBeenCalled() }) - it('shows a cleanup error toast when a revoke fails, without losing the grant success toast', async () => { - mockApi.revokeUserRole.mockRejectedValueOnce(new Error('nope')) + it('grants nothing while waiting for confirmation', async () => { const wrapper = await mountResolved() await selectInstanceScope(wrapper) await wrapper.vm.grantRole() - await wrapper.vm.onCascadeConfirm() - expect(mockToast.success).toHaveBeenCalledWith( - core.i18n.global.t('settings.users.rolesModal.grantSuccess', { role: 'Instance admin', uid: 'alice@example.org' }) - ) - expect(mockToast.error).toHaveBeenCalledWith(core.i18n.global.t('settings.users.rolesModal.cascadeModal.cleanupError')) + expect(mockApi.grantInstanceRole).not.toHaveBeenCalled() + expect(mockApi.revokeUserRole).not.toHaveBeenCalled() }) it('skips the confirmation modal when the user has no existing grants', async () => { From 00f5ba4790d18b2e906b0ee814f4d02fc57b9c1a Mon Sep 17 00:00:00 2001 From: Caroline Desprat Date: Wed, 7 Oct 2026 11:49:30 +0000 Subject: [PATCH 13/15] fix(users): check the edit email field against its own native validity (review: SettingsViewUsersEditModal.vue:47) A hand-rolled regex required a dot in the domain and allowed commas in the local part, the opposite of what the native type="email" check (which actually gates the save) allows. Reading validity.typeMismatch off the input itself keeps the inline message and the save gate in permanent agreement. Co-authored-by: Pierre Romera Zhang <471176+pirhoo@users.noreply.github.com> --- .../SettingsViewUsersEditModal.vue | 18 ++++++++--- .../SettingsViewUsersEditModal.spec.js | 31 ++++++++++++++++--- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/views/Settings/SettingsView/SettingsViewUsersEditModal.vue b/src/views/Settings/SettingsView/SettingsViewUsersEditModal.vue index 36ad0b02be..83c5ab93af 100644 --- a/src/views/Settings/SettingsView/SettingsViewUsersEditModal.vue +++ b/src/views/Settings/SettingsView/SettingsViewUsersEditModal.vue @@ -1,5 +1,5 @@