Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,10 @@ export class Api {
return this.sendAction(`/api/project/${project}`)
}

getUsers({ domain = 'default', index = null, q = null, sort = null, desc = null, from = 0, size = 10, noRole = true } = {}) {
const params = omitBy({ domain, index, q, sort, desc, from, size, noRole }, isNull)
// uid is an exact match and takes precedence over q server-side: use it to resolve one known
// user, q to search. omitBy drops the nulls, so passing { uid } alone sends no q.
getUsers({ domain = 'default', index = null, uid = null, q = null, sort = null, desc = null, from = 0, size = 10, noRole = true } = {}) {
const params = omitBy({ domain, index, uid, q, sort, desc, from, size, noRole }, isNull)
return this.sendAction('/api/users/admin', { method: Method.GET, params })
}

Expand Down
6 changes: 4 additions & 2 deletions src/views/Settings/SettingsView/SettingsViewUsers.vue
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,11 @@ async function fetchRoutedUser() {
const uid = routedUid.value
if (!uid) return
try {
const { items } = await api.getUsers({ domain: DEFAULT_DOMAIN, noRole: true, q: uid, size: 100 })
// An exact uid lookup: a q search would return every user whose uid merely contains this one,
// and on a large instance the user we want could fall outside the page.
const { items } = await api.getUsers({ domain: DEFAULT_DOMAIN, noRole: true, uid })
if (uid !== routedUid.value) return
routedUser.value = items?.find(user => user.uid === uid) ?? null
routedUser.value = items?.[0] ?? null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crucial: with items?.[0], any backend that doesn't support uid yet ignores the param and returns the first page of all users, so /settings/users/delete/zoe@example.org would open the delete modal on whoever comes first and delete them. Keeping the exact-match find costs nothing once the backend honours uid and keeps the wrong account from being picked:

Suggested change
routedUser.value = items?.[0] ?? null
routedUser.value = items?.find(user => user.uid === uid) ?? null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I checked the backend!

routedUserUid.value = uid
}
catch (error) {
Expand Down
12 changes: 12 additions & 0 deletions tests/unit/specs/api/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,18 @@ describe('Datashare backend client', () => {
expect(axios.request.mock.calls[0][0].params).not.toHaveProperty('user')
expect(axios.request.mock.calls[0][0].params).not.toHaveProperty('project')
})

it('sends an exact "uid" and no "q" when resolving a single user', async () => {
await api.getUsers({ uid: 'alice' })
expect(axios.request).toBeCalledWith(
expect.objectContaining({
url: Api.getFullUrl('/api/users/admin'),
method: 'GET',
params: expect.objectContaining({ uid: 'alice' })
})
)
expect(axios.request.mock.calls[0][0].params).not.toHaveProperty('q')
})
})

it('should call deleteUser with userId (uid) and no body', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ describe('SettingsViewUsers.vue', () => {
const zoe = { uid: 'zoe@example.org', name: 'Zoe', email: 'zoe@example.org', permissions: [] }

beforeEach(() => {
api.getUsers.mockImplementation(async ({ q }) => (q === zoe.uid ? { items: [zoe], pagination: { total: 1 } } : usersResponse))
api.getUsers.mockImplementation(async ({ uid }) => (uid === zoe.uid ? { items: [zoe], pagination: { total: 1 } } : usersResponse))
})

it.each([
Expand All @@ -256,11 +256,23 @@ describe('SettingsViewUsers.vue', () => {
expect(wrapper.findComponent(modal).props()).toMatchObject({ modelValue: true, user: zoe, notFound: false })
})

// A uid search by q only matches a substring, so a short uid could fall outside the page and
// the modal would claim the user does not exist (ICIJ/datashare#2434).
it('looks the routed user up by exact uid, not with a q search over a page of hits', async () => {
await core.router.push(`/settings/users/edit/${zoe.uid}`)
shallowMountComponent()
await flushPromises()
// noRole stays on: a user holding no role in the scope must still resolve
expect(api.getUsers).toHaveBeenCalledWith(expect.objectContaining({ uid: zoe.uid, noRole: true }))
expect(api.getUsers).not.toHaveBeenCalledWith(expect.objectContaining({ q: zoe.uid }))
})

it.each([
['manage', SettingsViewUsersRolesModal],
['edit', SettingsViewUsersEditModal],
['delete', SettingsViewUsersDeleteModal]
])('flags the %s modal as not found when the URL names an unknown user', async (action, modal) => {
api.getUsers.mockImplementation(async ({ uid }) => (uid ? { items: [], pagination: { total: 0 } } : usersResponse))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

important: this mock had to change to return no items because with items?.[0] the default response would resolve ghost to the first listed user. Nothing tests that case any more. Could we keep the original mock and add a case where the uid lookup returns { items: [otherUser] }, asserting the modal is flagged not found?

await core.router.push(`/settings/users/${action}/ghost`)
const wrapper = shallowMountComponent()
await flushPromises()
Expand Down Expand Up @@ -310,8 +322,8 @@ describe('SettingsViewUsers.vue', () => {
})

it('reports a failed lookup and goes back to the list, instead of saying the user does not exist', async () => {
api.getUsers.mockImplementation(async ({ q }) => {
if (q === zoe.uid) throw new Error('timeout')
api.getUsers.mockImplementation(async ({ uid }) => {
if (uid === zoe.uid) throw new Error('timeout')
return usersResponse
})
await core.router.push(`/settings/users/manage/${zoe.uid}`)
Expand Down Expand Up @@ -414,13 +426,13 @@ describe('SettingsViewUsers.vue', () => {

// Save then close: the refetch started by user:updated is dropped once the modal closes
const renamed = { ...zoe, name: 'Zoe Renamed' }
api.getUsers.mockImplementation(async ({ q }) => (q === zoe.uid ? { items: [renamed] } : usersResponse))
api.getUsers.mockImplementation(async ({ uid }) => (uid === zoe.uid ? { items: [renamed] } : usersResponse))
wrapper.findComponent(SettingsViewUsersEditModal).vm.$emit('user:updated', { uid: zoe.uid })
wrapper.findComponent(SettingsViewUsersEditModal).vm.$emit('update:modelValue', false)
await flushPromises()

let resolveLookup
api.getUsers.mockImplementation(({ q }) => (q === zoe.uid
api.getUsers.mockImplementation(({ uid }) => (uid === zoe.uid
? new Promise((resolve) => {
resolveLookup = () => resolve({ items: [renamed] })
})
Expand Down Expand Up @@ -451,12 +463,12 @@ describe('SettingsViewUsers.vue', () => {
await flushPromises()

const granted = { ...zoe, permissions: [{ v1: 'PROJECT_MEMBER', v2: 'default::project-a' }] }
api.getUsers.mockImplementation(async ({ q }) => (q === zoe.uid ? { items: [granted] } : usersResponse))
api.getUsers.mockImplementation(async ({ uid }) => (uid === zoe.uid ? { items: [granted] } : usersResponse))
api.getUsers.mockClear()
wrapper.findComponent(SettingsViewUsersRolesModal).vm.$emit('user:updated', { uid: zoe.uid })
await flushPromises()

expect(api.getUsers).toHaveBeenCalledWith(expect.objectContaining({ q: zoe.uid }))
expect(api.getUsers).toHaveBeenCalledWith(expect.objectContaining({ uid: zoe.uid }))
expect(api.getUsers).toHaveBeenCalledWith(expect.objectContaining({ q: null }))
expect(wrapper.findComponent(SettingsViewUsersRolesModal).props('user')).toEqual(granted)
})
Expand Down
Loading