From dbf29234feac882414a4fac1b41ee4de8f1f55ff Mon Sep 17 00:00:00 2001 From: Nikolaus Demmel Date: Mon, 31 Aug 2026 22:51:26 +0200 Subject: [PATCH] fix(settings): show group display names in the 2FA group picker AdminTwoFactor.vue's "Enforced groups"/"Excluded groups" pickers fetched cloud/groups, which only returns a group's raw ID - so a group with a friendly display name but an opaque ID (e.g. one created by an app via IGroupManager::createGroup()) rendered as that opaque ID, with no way to visually confirm it's the right group. Switch to cloud/groups/details (id + displayname, already used elsewhere for the same purpose, e.g. RequestUserGroup.vue) and render via NcSelect's label prop. Also fetch by ID for any already-enforced/ excluded group missing from the initial page, so a previously configured group whose ID doesn't sort into the first page still resolves to a real display name - and remains selectable at all in the "excluded" input, since NcSelect can only show options present in :options. The initial page's ORDER BY gid ASC + fixed limit=20 (so an unfiltered load can still omit some groups entirely) is unchanged - that's shared with every other group picker using this pattern and out of scope here; search by display name already works today regardless. Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: Nikolaus Demmel --- .../src/components/AdminTwoFactor.spec.js | 157 ++++++++++++++++++ .../src/components/AdminTwoFactor.vue | 61 ++++--- 2 files changed, 198 insertions(+), 20 deletions(-) create mode 100644 apps/settings/src/components/AdminTwoFactor.spec.js diff --git a/apps/settings/src/components/AdminTwoFactor.spec.js b/apps/settings/src/components/AdminTwoFactor.spec.js new file mode 100644 index 0000000000000..eabbdc6eac453 --- /dev/null +++ b/apps/settings/src/components/AdminTwoFactor.spec.js @@ -0,0 +1,157 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import axios from '@nextcloud/axios' +import { mount } from '@vue/test-utils' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import Vue from 'vue' +import Vuex, { Store } from 'vuex' +import NcSelect from '@nextcloud/vue/components/NcSelect' +import AdminTwoFactor from './AdminTwoFactor.vue' + +// Mirrors apps/settings/src/store/admin-security.js's shape - not imported +// directly since that module references the webpack-injected PRODUCTION +// global, which isn't defined under vitest. +Vue.use(Vuex) +const store = new Store({ + state: { enforced: false, enforcedGroups: [], excludedGroups: [] }, + mutations: { + setEnforced(state, val) { state.enforced = val }, + setEnforcedGroups(state, val) { state.enforcedGroups = val }, + setExcludedGroups(state, val) { state.excludedGroups = val }, + }, +}) + +vi.mock('@nextcloud/axios', () => ({ + default: { + get: vi.fn(), + put: vi.fn(), + }, +})) +vi.mock('@nextcloud/router', () => ({ + generateOcsUrl(url, params) { + return url.replace(/\{(\w+)\}/g, (_, key) => encodeURIComponent(params[key] ?? '')) + }, + generateUrl(url) { + return url + }, +})) +vi.mock('@nextcloud/initial-state', () => ({ + loadState: () => '', +})) + +function detailsResponse(groups) { + return { data: { ocs: { data: { groups } } } } +} + +function mountAdminTwoFactor() { + return mount(AdminTwoFactor, { + store, + mocks: { + t: (_app, text) => text, + }, + }) +} + +describe('AdminTwoFactor', () => { + beforeEach(() => { + store.replaceState({ enforced: true, enforcedGroups: [], excludedGroups: [] }) + }) + + afterEach(() => { + vi.clearAllMocks() + }) + + it('resolves already-enforced/excluded groups to their real display name, even when absent from the first page', async () => { + // Two missing groups, not one - a single missing group can't tell a + // parallel Promise.all(...) apart from a debounced call that + // collapses concurrent invocations down to the last one. + store.replaceState({ + enforced: true, + enforcedGroups: ['plugin_abc'], + excludedGroups: ['plugin_xyz'], + }) + axios.get.mockImplementation((url) => { + if (url.includes('search=plugin_abc')) { + return Promise.resolve(detailsResponse([{ id: 'plugin_abc', displayname: 'My Plugin Group' }])) + } + if (url.includes('search=plugin_xyz')) { + return Promise.resolve(detailsResponse([{ id: 'plugin_xyz', displayname: 'My Other Plugin Group' }])) + } + // The general, unfiltered first page - deliberately doesn't include + // either already-configured group, as if they sorted past the page cap. + return Promise.resolve(detailsResponse([{ id: 'everyone', displayname: 'Everyone' }])) + }) + + const wrapper = mountAdminTwoFactor() + + await expect.poll(() => axios.get.mock.calls.length).toBe(3) + await expect.poll(() => wrapper.vm.resolveGroup('plugin_abc').displayname).toBe('My Plugin Group') + expect(wrapper.vm.resolveGroup('plugin_xyz').displayname).toBe('My Other Plugin Group') + }) + + it('fetches from the details endpoint (id + displayname), not the plain group-ID list', async () => { + axios.get.mockResolvedValue(detailsResponse([{ id: 'plugin_abc', displayname: 'My Plugin Group' }])) + + const wrapper = mountAdminTwoFactor() + + await expect.poll(() => axios.get.mock.calls.length).toBeGreaterThan(0) + expect(axios.get).toHaveBeenCalledWith(expect.stringContaining('cloud/groups/details')) + await expect.poll(() => wrapper.vm.groups).toEqual([{ id: 'plugin_abc', displayname: 'My Plugin Group' }]) + }) + + it('renders NcSelect options by display name, not the raw option object', async () => { + axios.get.mockResolvedValue(detailsResponse([{ id: 'plugin_abc', displayname: 'My Plugin Group' }])) + + const wrapper = mountAdminTwoFactor() + await expect.poll(() => wrapper.vm.groups.length).toBe(1) + + const selects = wrapper.findAll(NcSelect) + expect(selects).toHaveLength(2) + expect(selects.at(0).props('label')).toBe('displayname') + expect(selects.at(1).props('label')).toBe('displayname') + }) + + // The getter/setter translation layer is new in this change, so this + // guards the intermediate state (object-returning getters paired with a + // saveChanges() that still read them directly) rather than a pre-fix + // regression - reverting AdminTwoFactor.vue to origin/master passes this + // test too, since the old getters already returned plain ID arrays. + it('sends plain group ID arrays when saving, not display objects', async () => { + store.replaceState({ + enforced: true, + enforcedGroups: ['a', 'b'], + excludedGroups: [], + }) + axios.get.mockResolvedValue(detailsResponse([ + { id: 'a', displayname: 'Group A' }, + { id: 'b', displayname: 'Group B' }, + ])) + axios.put.mockResolvedValue({ data: {} }) + + const wrapper = mountAdminTwoFactor() + await expect.poll(() => wrapper.vm.groups.length).toBe(2) + + await wrapper.vm.saveChanges() + + expect(axios.put).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ enforcedGroups: ['a', 'b'], excludedGroups: [] }), + expect.anything(), + ) + }) + + it('commits plain group IDs to Vuex when NcSelect emits a selection, not the display objects', async () => { + axios.get.mockResolvedValue(detailsResponse([])) + const wrapper = mountAdminTwoFactor() + await expect.poll(() => axios.get.mock.calls.length).toBeGreaterThan(0) + + // Simulates NcSelect's v-model update - it emits the selected option + // objects (matching :options="groups"), not plain IDs. + wrapper.vm.enforcedGroups = [{ id: 'plugin_abc', displayname: 'My Plugin Group' }] + + expect(store.state.enforcedGroups).toEqual(['plugin_abc']) + }) +}) diff --git a/apps/settings/src/components/AdminTwoFactor.vue b/apps/settings/src/components/AdminTwoFactor.vue index c6166987e2195..d375f02d22f9e 100644 --- a/apps/settings/src/components/AdminTwoFactor.vue +++ b/apps/settings/src/components/AdminTwoFactor.vue @@ -31,6 +31,7 @@ this.resolveGroup(id)) }, set(val) { this.dirty = true - this.$store.commit('setEnforcedGroups', val) + this.$store.commit('setEnforcedGroups', val.map((group) => group.id)) }, }, excludedGroups: { get() { - return this.$store.state.excludedGroups + return this.$store.state.excludedGroups.map((id) => this.resolveGroup(id)) }, set(val) { this.dirty = true - this.$store.commit('setExcludedGroups', val) + this.$store.commit('setExcludedGroups', val.map((group) => group.id)) }, }, }, - mounted() { - // Groups are loaded dynamically, but the assigned ones *should* - // be valid groups, so let's add them as initial state - this.groups = sortedUniq(uniq(this.enforcedGroups.concat(this.excludedGroups))) - + async mounted() { // Populate the groups with a first set so the dropdown is not empty // when opening the page the first time - this.searchGroup('') + await this.fetchGroups('') + + // The first set above is capped and may not include every already + // enforced/excluded group, so those wouldn't otherwise resolve to a + // display name - or be selectable at all, since NcSelect can only + // show options present in :options="groups". + const selectedIds = uniq(this.$store.state.enforcedGroups.concat(this.$store.state.excludedGroups)) + const missingIds = selectedIds.filter((id) => !this.groups.some((group) => group.id === id)) + await Promise.all(missingIds.map((id) => this.fetchGroups(id))) }, methods: { - searchGroup: debounce(function(query) { + resolveGroup(id) { + return this.groups.find((group) => group.id === id) || { id, displayname: id } + }, + + async fetchGroups(query) { this.loadingGroups = true - axios.get(generateOcsUrl('cloud/groups?offset=0&search={query}&limit=20', { query })) - .then((res) => res.data.ocs) - .then((ocs) => ocs.data.groups) - .then((groups) => { this.groups = sortedUniq(uniq(this.groups.concat(groups))) }) - .catch((error) => logger.error('could not search groups', { error })) - .then(() => { this.loadingGroups = false }) + try { + const res = await axios.get(generateOcsUrl('cloud/groups/details?offset=0&search={query}&limit=20', { query })) + const fetched = res.data.ocs.data.groups.map(({ id, displayname }) => ({ id, displayname })) + const merged = new Map(this.groups.map((group) => [group.id, group])) + fetched.forEach((group) => merged.set(group.id, group)) + this.groups = [...merged.values()] + } catch (error) { + logger.error('could not search groups', { error }) + } finally { + this.loadingGroups = false + } + }, + + searchGroup: debounce(function(query) { + this.fetchGroups(query) }, 500), saveChanges() { @@ -168,8 +189,8 @@ export default { const data = { enforced: this.enforced, - enforcedGroups: this.enforcedGroups, - excludedGroups: this.excludedGroups, + enforcedGroups: this.$store.state.enforcedGroups, + excludedGroups: this.$store.state.excludedGroups, } axios.put(generateUrl('/settings/api/admin/twofactorauth'), data, { confirmPassword: PwdConfirmationMode.Strict }) .then((resp) => resp.data)