Skip to content

Commit 2763746

Browse files
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
1 parent 76dc4c7 commit 2763746

2 files changed

Lines changed: 198 additions & 20 deletions

File tree

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
/**
2+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
3+
* SPDX-License-Identifier: AGPL-3.0-or-later
4+
*/
5+
6+
import axios from '@nextcloud/axios'
7+
import { mount } from '@vue/test-utils'
8+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
9+
import Vue from 'vue'
10+
import Vuex, { Store } from 'vuex'
11+
import NcSelect from '@nextcloud/vue/components/NcSelect'
12+
import AdminTwoFactor from './AdminTwoFactor.vue'
13+
14+
// Mirrors apps/settings/src/store/admin-security.js's shape - not imported
15+
// directly since that module references the webpack-injected PRODUCTION
16+
// global, which isn't defined under vitest.
17+
Vue.use(Vuex)
18+
const store = new Store({
19+
state: { enforced: false, enforcedGroups: [], excludedGroups: [] },
20+
mutations: {
21+
setEnforced(state, val) { state.enforced = val },
22+
setEnforcedGroups(state, val) { state.enforcedGroups = val },
23+
setExcludedGroups(state, val) { state.excludedGroups = val },
24+
},
25+
})
26+
27+
vi.mock('@nextcloud/axios', () => ({
28+
default: {
29+
get: vi.fn(),
30+
put: vi.fn(),
31+
},
32+
}))
33+
vi.mock('@nextcloud/router', () => ({
34+
generateOcsUrl(url, params) {
35+
return url.replace(/\{(\w+)\}/g, (_, key) => encodeURIComponent(params[key] ?? ''))
36+
},
37+
generateUrl(url) {
38+
return url
39+
},
40+
}))
41+
vi.mock('@nextcloud/initial-state', () => ({
42+
loadState: () => '',
43+
}))
44+
45+
function detailsResponse(groups) {
46+
return { data: { ocs: { data: { groups } } } }
47+
}
48+
49+
function mountAdminTwoFactor() {
50+
return mount(AdminTwoFactor, {
51+
store,
52+
mocks: {
53+
t: (_app, text) => text,
54+
},
55+
})
56+
}
57+
58+
describe('AdminTwoFactor', () => {
59+
beforeEach(() => {
60+
store.replaceState({ enforced: true, enforcedGroups: [], excludedGroups: [] })
61+
})
62+
63+
afterEach(() => {
64+
vi.clearAllMocks()
65+
})
66+
67+
it('resolves already-enforced/excluded groups to their real display name, even when absent from the first page', async () => {
68+
// Two missing groups, not one - a single missing group can't tell a
69+
// parallel Promise.all(...) apart from a debounced call that
70+
// collapses concurrent invocations down to the last one.
71+
store.replaceState({
72+
enforced: true,
73+
enforcedGroups: ['plugin_abc'],
74+
excludedGroups: ['plugin_xyz'],
75+
})
76+
axios.get.mockImplementation((url) => {
77+
if (url.includes('search=plugin_abc')) {
78+
return Promise.resolve(detailsResponse([{ id: 'plugin_abc', displayname: 'My Plugin Group' }]))
79+
}
80+
if (url.includes('search=plugin_xyz')) {
81+
return Promise.resolve(detailsResponse([{ id: 'plugin_xyz', displayname: 'My Other Plugin Group' }]))
82+
}
83+
// The general, unfiltered first page - deliberately doesn't include
84+
// either already-configured group, as if they sorted past the page cap.
85+
return Promise.resolve(detailsResponse([{ id: 'everyone', displayname: 'Everyone' }]))
86+
})
87+
88+
const wrapper = mountAdminTwoFactor()
89+
90+
await expect.poll(() => axios.get.mock.calls.length).toBe(3)
91+
await expect.poll(() => wrapper.vm.resolveGroup('plugin_abc').displayname).toBe('My Plugin Group')
92+
expect(wrapper.vm.resolveGroup('plugin_xyz').displayname).toBe('My Other Plugin Group')
93+
})
94+
95+
it('fetches from the details endpoint (id + displayname), not the plain group-ID list', async () => {
96+
axios.get.mockResolvedValue(detailsResponse([{ id: 'plugin_abc', displayname: 'My Plugin Group' }]))
97+
98+
const wrapper = mountAdminTwoFactor()
99+
100+
await expect.poll(() => axios.get.mock.calls.length).toBeGreaterThan(0)
101+
expect(axios.get).toHaveBeenCalledWith(expect.stringContaining('cloud/groups/details'))
102+
await expect.poll(() => wrapper.vm.groups).toEqual([{ id: 'plugin_abc', displayname: 'My Plugin Group' }])
103+
})
104+
105+
it('renders NcSelect options by display name, not the raw option object', async () => {
106+
axios.get.mockResolvedValue(detailsResponse([{ id: 'plugin_abc', displayname: 'My Plugin Group' }]))
107+
108+
const wrapper = mountAdminTwoFactor()
109+
await expect.poll(() => wrapper.vm.groups.length).toBe(1)
110+
111+
const selects = wrapper.findAll(NcSelect)
112+
expect(selects).toHaveLength(2)
113+
expect(selects.at(0).props('label')).toBe('displayname')
114+
expect(selects.at(1).props('label')).toBe('displayname')
115+
})
116+
117+
// The getter/setter translation layer is new in this change, so this
118+
// guards the intermediate state (object-returning getters paired with a
119+
// saveChanges() that still read them directly) rather than a pre-fix
120+
// regression - reverting AdminTwoFactor.vue to origin/master passes this
121+
// test too, since the old getters already returned plain ID arrays.
122+
it('sends plain group ID arrays when saving, not display objects', async () => {
123+
store.replaceState({
124+
enforced: true,
125+
enforcedGroups: ['a', 'b'],
126+
excludedGroups: [],
127+
})
128+
axios.get.mockResolvedValue(detailsResponse([
129+
{ id: 'a', displayname: 'Group A' },
130+
{ id: 'b', displayname: 'Group B' },
131+
]))
132+
axios.put.mockResolvedValue({ data: {} })
133+
134+
const wrapper = mountAdminTwoFactor()
135+
await expect.poll(() => wrapper.vm.groups.length).toBe(2)
136+
137+
await wrapper.vm.saveChanges()
138+
139+
expect(axios.put).toHaveBeenCalledWith(
140+
expect.any(String),
141+
expect.objectContaining({ enforcedGroups: ['a', 'b'], excludedGroups: [] }),
142+
expect.anything(),
143+
)
144+
})
145+
146+
it('commits plain group IDs to Vuex when NcSelect emits a selection, not the display objects', async () => {
147+
axios.get.mockResolvedValue(detailsResponse([]))
148+
const wrapper = mountAdminTwoFactor()
149+
await expect.poll(() => axios.get.mock.calls.length).toBeGreaterThan(0)
150+
151+
// Simulates NcSelect's v-model update - it emits the selected option
152+
// objects (matching :options="groups"), not plain IDs.
153+
wrapper.vm.enforcedGroups = [{ id: 'plugin_abc', displayname: 'My Plugin Group' }]
154+
155+
expect(store.state.enforcedGroups).toEqual(['plugin_abc'])
156+
})
157+
})

apps/settings/src/components/AdminTwoFactor.vue

Lines changed: 41 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
<NcSelect
3232
v-model="enforcedGroups"
3333
input-id="enforcedGroups"
34+
label="displayname"
3435
:options="groups"
3536
:disabled="loading"
3637
:multiple="true"
@@ -48,6 +49,7 @@
4849
<NcSelect
4950
v-model="excludedGroups"
5051
input-id="excludedGroups"
52+
label="displayname"
5153
:options="groups"
5254
:disabled="loading"
5355
:multiple="true"
@@ -80,7 +82,6 @@ import { loadState } from '@nextcloud/initial-state'
8082
import { PwdConfirmationMode } from '@nextcloud/password-confirmation'
8183
import { generateOcsUrl, generateUrl } from '@nextcloud/router'
8284
import debounce from 'lodash/debounce.js'
83-
import sortedUniq from 'lodash/sortedUniq.js'
8485
import uniq from 'lodash/uniq.js'
8586
import NcButton from '@nextcloud/vue/components/NcButton'
8687
import NcCheckboxRadioSwitch from '@nextcloud/vue/components/NcCheckboxRadioSwitch'
@@ -119,57 +120,77 @@ export default {
119120
},
120121
},
121122
123+
// enforcedGroups/excludedGroups store plain group IDs in Vuex, but NcSelect
124+
// needs {id, displayname} objects (matching :options="groups") to render
125+
// anything but the raw ID.
122126
enforcedGroups: {
123127
get() {
124-
return this.$store.state.enforcedGroups
128+
return this.$store.state.enforcedGroups.map((id) => this.resolveGroup(id))
125129
},
126130
127131
set(val) {
128132
this.dirty = true
129-
this.$store.commit('setEnforcedGroups', val)
133+
this.$store.commit('setEnforcedGroups', val.map((group) => group.id))
130134
},
131135
},
132136
133137
excludedGroups: {
134138
get() {
135-
return this.$store.state.excludedGroups
139+
return this.$store.state.excludedGroups.map((id) => this.resolveGroup(id))
136140
},
137141
138142
set(val) {
139143
this.dirty = true
140-
this.$store.commit('setExcludedGroups', val)
144+
this.$store.commit('setExcludedGroups', val.map((group) => group.id))
141145
},
142146
},
143147
},
144148
145-
mounted() {
146-
// Groups are loaded dynamically, but the assigned ones *should*
147-
// be valid groups, so let's add them as initial state
148-
this.groups = sortedUniq(uniq(this.enforcedGroups.concat(this.excludedGroups)))
149-
149+
async mounted() {
150150
// Populate the groups with a first set so the dropdown is not empty
151151
// when opening the page the first time
152-
this.searchGroup('')
152+
await this.fetchGroups('')
153+
154+
// The first set above is capped and may not include every already
155+
// enforced/excluded group, so those wouldn't otherwise resolve to a
156+
// display name - or be selectable at all, since NcSelect can only
157+
// show options present in :options="groups".
158+
const selectedIds = uniq(this.$store.state.enforcedGroups.concat(this.$store.state.excludedGroups))
159+
const missingIds = selectedIds.filter((id) => !this.groups.some((group) => group.id === id))
160+
await Promise.all(missingIds.map((id) => this.fetchGroups(id)))
153161
},
154162
155163
methods: {
156-
searchGroup: debounce(function(query) {
164+
resolveGroup(id) {
165+
return this.groups.find((group) => group.id === id) || { id, displayname: id }
166+
},
167+
168+
async fetchGroups(query) {
157169
this.loadingGroups = true
158-
axios.get(generateOcsUrl('cloud/groups?offset=0&search={query}&limit=20', { query }))
159-
.then((res) => res.data.ocs)
160-
.then((ocs) => ocs.data.groups)
161-
.then((groups) => { this.groups = sortedUniq(uniq(this.groups.concat(groups))) })
162-
.catch((error) => logger.error('could not search groups', { error }))
163-
.then(() => { this.loadingGroups = false })
170+
try {
171+
const res = await axios.get(generateOcsUrl('cloud/groups/details?offset=0&search={query}&limit=20', { query }))
172+
const fetched = res.data.ocs.data.groups.map(({ id, displayname }) => ({ id, displayname }))
173+
const merged = new Map(this.groups.map((group) => [group.id, group]))
174+
fetched.forEach((group) => merged.set(group.id, group))
175+
this.groups = [...merged.values()]
176+
} catch (error) {
177+
logger.error('could not search groups', { error })
178+
} finally {
179+
this.loadingGroups = false
180+
}
181+
},
182+
183+
searchGroup: debounce(function(query) {
184+
this.fetchGroups(query)
164185
}, 500),
165186
166187
saveChanges() {
167188
this.loading = true
168189
169190
const data = {
170191
enforced: this.enforced,
171-
enforcedGroups: this.enforcedGroups,
172-
excludedGroups: this.excludedGroups,
192+
enforcedGroups: this.$store.state.enforcedGroups,
193+
excludedGroups: this.$store.state.excludedGroups,
173194
}
174195
axios.put(generateUrl('/settings/api/admin/twofactorauth'), data, { confirmPassword: PwdConfirmationMode.Strict })
175196
.then((resp) => resp.data)

0 commit comments

Comments
 (0)