diff --git a/@xen-orchestra/qa-test/package.json b/@xen-orchestra/qa-test/package.json index 2812129c6d..d90d1bdc3f 100644 --- a/@xen-orchestra/qa-test/package.json +++ b/@xen-orchestra/qa-test/package.json @@ -32,6 +32,7 @@ "scripts": { "qa": "node --env-file-if-exists=.env --test --test-concurrency=1 tests/*.test.js", "qa:infrastructure": "node --env-file-if-exists=.env --test tests/infrastructure.test.js", + "qa:acl": "node --env-file-if-exists=.env --test tests/acl.test.js", "qa:vm": "node --env-file-if-exists=.env --test tests/vm.test.js", "qa:backup:base": "node --env-file-if-exists=.env --test tests/backup.test.js", "qa:backup:nbd": "node --env-file-if-exists=.env --test tests/backup.nbd.test.js", diff --git a/@xen-orchestra/qa-test/tests/acl.test.js b/@xen-orchestra/qa-test/tests/acl.test.js new file mode 100644 index 0000000000..52046ba4e3 --- /dev/null +++ b/@xen-orchestra/qa-test/tests/acl.test.js @@ -0,0 +1,604 @@ +import '../logSetup.js' +import assert from 'node:assert/strict' +import { after, before, describe, it } from 'node:test' +import { createLogger } from '@xen-orchestra/log' + +import { getRequiredEnv } from '../utils/index.js' +import { xoConnection } from '../client/xoLibClient.js' +import { + connectAs, + createTestGroup, + createTestUser, + deleteTestGroup, + deleteTestUser, + EVENT_WINDOW, + fetchAllObjects, + grant, + findVmStorage, + pickObjectsByType, + purgeLeftoverFixtures, + recordPushedObjects, + revoke, + summarize, + wait, + waitUntil, + withTag, +} from '../utils/aclUtils.js' + +const log = createLogger('xo:qa-test:acl') + +// Time left for the server to apply an ACL change before it is observed. +const SETTLE = 2_000 + +describe('ACL — server-side object filtering', () => { + /** admin connection: arranges the fixtures */ + let admin + /** low-privilege connection: observes what the server sends it */ + let user + let testUser + let recorder + let referenceVmId + let referencePoolId + + before(async () => { + admin = await xoConnection({ + xoUrl: getRequiredEnv('HOSTNAME'), + username: getRequiredEnv('USERNAME'), + password: getRequiredEnv('PASSWORD'), + }) + + // an `after` hook does not run when its `before` threw, so a suite that + // failed half way leaves fixtures — and their ACLs — on the appliance + await purgeLeftoverFixtures(admin) + + referenceVmId = getRequiredEnv('REFERENCE_VM_ID') + + // fail early and clearly rather than through a confusing assertion later + const allObjects = await fetchAllObjects(admin) + const referenceVm = allObjects[referenceVmId] + assert.notEqual(referenceVm, undefined, `REFERENCE_VM_ID ${referenceVmId} is not a known XO object`) + assert.equal(referenceVm.type, 'VM', `REFERENCE_VM_ID ${referenceVmId} is a ${referenceVm.type}, not a VM`) + + referencePoolId = referenceVm.$pool + assert.notEqual(referencePoolId, undefined, 'the reference VM has no $pool') + + log.debug('admin sees the whole infrastructure', { + summary: summarize(allObjects, 20), + referencePoolId, + }) + + testUser = await createTestUser(admin) + user = await connectAs(testUser) + recorder = recordPushedObjects(user) + }) + + after(async () => { + recorder?.stop() + await user?.close() + + if (testUser !== undefined) { + await deleteTestUser(admin, testUser.id) + } + await admin?.close() + }) + + describe('a user without any ACL', () => { + it('is granted no permission at all', async () => { + const permissions = await user.call('acl.getCurrentPermissions') + const granted = Object.keys(permissions) + + assert.equal(granted.length, 0, `expected no ACL, got: ${granted.join(', ')}`) + }) + + // Path 1 of 2: the bulk fetch xo-web performs on sign-in. + it('receives no object from xo.getAllObjects', async () => { + const objects = await fetchAllObjects(user) + const received = Object.keys(objects).length + + assert.equal(received, 0, `expected no object, received ${received}: ${summarize(objects)}`) + }) + + // Tags carry organisational meaning — `customer-acme`, `prod-finance` — + // so the configured list is scoped to the objects the caller may see + // rather than locked to admins, which would cost everyone tag colours. + it('receives no configured tag', async () => { + const tags = await user.call('tag.getAllConfigured') + + assert.equal(tags.length, 0, `expected no tag, received: ${tags.map(({ id }) => id).join(', ')}`) + }) + + // Path 2 of 2: the push stream. A fix could land one path and miss the + // other, so they are asserted separately. + it('receives no object on the event stream while the infrastructure changes', async () => { + recorder.reset() + + await withTag(admin, referenceVmId, `qa-acl-${Date.now()}`, () => wait(EVENT_WINDOW)) + + // `exited` is not asserted: removals carry ids only and are forwarded to + // every connection by design + const pushed = Object.fromEntries(recorder.entered) + const nPushed = recorder.entered.size + + assert.equal(nPushed, 0, `expected nothing to be pushed, received ${nPushed}: ${summarize(pushed)}`) + }) + }) + + describe('a user with `view` on a single VM', () => { + before(async () => { + await grant(admin, testUser.id, referenceVmId) + await wait(SETTLE) + }) + + after(async () => { + await revoke(admin, testUser.id, referenceVmId) + }) + + it('receives that VM', async () => { + const objects = await fetchAllObjects(user) + + assert.ok(referenceVmId in objects, `the granted VM was not returned; received ${summarize(objects)}`) + }) + + it('receives no other VM, and no host, pool, SR or network', async () => { + const objects = await fetchAllObjects(user) + + const otherVmIds = Object.keys(objects).filter(id => objects[id].type === 'VM' && id !== referenceVmId) + assert.equal( + otherVmIds.length, + 0, + `expected only the granted VM, also received ${otherVmIds.length} others, e.g. ${otherVmIds.slice(0, 3).join(', ')}` + ) + + // the VM's own disks, interfaces and snapshots are expected: access is + // inherited downwards. Its container is not. + const infrastructure = Object.fromEntries( + Object.entries(objects).filter(([, object]) => ['host', 'pool', 'SR', 'network'].includes(object.type)) + ) + const nLeaked = Object.keys(infrastructure).length + assert.equal(nLeaked, 0, `expected no infrastructure object, received ${nLeaked}: ${summarize(infrastructure)}`) + + log.debug('objects visible with a single VM grant', { summary: summarize(objects, 20) }) + }) + }) + + // Permissions are inherited downwards: granting a pool grants everything it + // contains. This is the resolver's `checkMember('$container')` / `$pool` + // rules, and it is what makes a single ACL usable in practice. + describe('a user with `view` on a pool', () => { + let visible + + before(async () => { + await grant(admin, testUser.id, referencePoolId) + await wait(SETTLE) + visible = await fetchAllObjects(user) + }) + + after(async () => { + await revoke(admin, testUser.id, referencePoolId) + }) + + it('receives the pool itself', () => { + assert.ok(referencePoolId in visible, `the granted pool was not returned; received ${summarize(visible)}`) + }) + + it('receives every host of that pool', async () => { + const everything = await fetchAllObjects(admin) + const expected = Object.keys(everything) + .filter(id => everything[id].type === 'host' && everything[id].$pool === referencePoolId) + .sort() + const received = Object.keys(visible) + .filter(id => visible[id].type === 'host') + .sort() + + // under-sharing is as much a bug as over-sharing + assert.deepEqual(received, expected, 'the hosts of the granted pool were not all returned') + }) + + it('receives the VMs, SRs and networks of that pool', () => { + for (const type of ['VM', 'SR', 'network']) { + const n = Object.values(visible).filter(object => object.type === type).length + assert.ok(n > 0, `expected at least one ${type}, received ${summarize(visible)}`) + } + }) + + // Every XO object carries the pool it belongs to, which makes the + // cross-pool check exact. On a single-pool lab this passes trivially. + it('receives nothing belonging to another pool', () => { + const foreign = Object.fromEntries( + Object.entries(visible).filter(([, object]) => object.$pool !== referencePoolId) + ) + const nForeign = Object.keys(foreign).length + + assert.equal( + nForeign, + 0, + `expected nothing outside the granted pool, received ${nForeign}: ${summarize(foreign)}` + ) + }) + }) + + // A grant made to a group must reach its members, and must stop reaching + // them as soon as they leave it — which is also the only coverage of the + // group-membership cache invalidation. + describe('a user granted access through a group', () => { + let group + + before(async () => { + group = await createTestGroup(admin) + await admin.call('group.addUser', { id: group.id, userId: testUser.id }) + await grant(admin, group.id, referenceVmId) + await wait(SETTLE) + }) + + after(async () => { + await revoke(admin, group.id, referenceVmId) + await deleteTestGroup(admin, group.id) + }) + + it('receives the VM granted to the group', async () => { + const objects = await fetchAllObjects(user) + + assert.ok( + referenceVmId in objects, + `the VM granted to the group was not returned; received ${summarize(objects)}` + ) + }) + + it('stops receiving it once removed from the group', async () => { + await admin.call('group.removeUser', { id: group.id, userId: testUser.id }) + await wait(SETTLE) + + const objects = await fetchAllObjects(user) + const received = Object.keys(objects).length + + assert.equal( + received, + 0, + `expected no object after leaving the group, received ${received}: ${summarize(objects)}` + ) + }) + }) + + // Access flows from a container to what it holds, never back up. Reaching a + // VM's storage must not reach the VM: on a shared SR that would expose every + // other tenant's disks and names. + describe('a user with `view` on an SR', () => { + let srId + let vdiId + let visible + + before(async () => { + const everything = await fetchAllObjects(admin) + const storage = findVmStorage(everything, referenceVmId) + assert.notEqual(storage, undefined, `the reference VM ${referenceVmId} has no disk to derive an SR from`) + ;({ srId, vdiId } = storage) + + await grant(admin, testUser.id, srId) + await wait(SETTLE) + visible = await fetchAllObjects(user) + }) + + after(async () => { + await revoke(admin, testUser.id, srId) + }) + + it('receives the SR itself', () => { + assert.ok(srId in visible, `the granted SR was not returned; received ${summarize(visible)}`) + }) + + // Managed and unmanaged disks alike: an unmanaged VDI is the same XAPI + // object, and the ISO/disk pickers walk the SR's whole VDI list. + it('receives every disk held by that SR, managed or not', async () => { + assert.ok(vdiId in visible, `the disk ${vdiId} on the granted SR was not returned`) + + const everything = await fetchAllObjects(admin) + const onSr = Object.keys(everything).filter( + id => ['VDI', 'VDI-unmanaged'].includes(everything[id].type) && everything[id].$SR === srId + ) + const missing = onSr.filter(id => !(id in visible)) + + assert.equal( + missing.length, + 0, + `${missing.length} of the ${onSr.length} disks on the SR were not returned, e.g. ${missing.slice(0, 3).join(', ')}` + ) + }) + + it('receives no disk belonging to another SR', () => { + const foreign = Object.values(visible).filter( + object => ['VDI', 'VDI-unmanaged'].includes(object.type) && object.$SR !== srId + ) + + assert.equal(foreign.length, 0, `received ${foreign.length} disks belonging to another SR`) + }) + + // The reference VM is on this SR by construction, so there is something + // real to leak here. + it('receives no VM, even the ones whose disks are on that SR', () => { + const vms = Object.fromEntries(Object.entries(visible).filter(([, object]) => object.type === 'VM')) + const nVms = Object.keys(vms).length + + assert.equal( + nVms, + 0, + `expected no VM through an SR grant, received ${nVms} (including the reference VM: ${referenceVmId in vms})` + ) + }) + + it('receives no host and no pool', () => { + const containers = Object.fromEntries( + Object.entries(visible).filter(([, object]) => object.type === 'host' || object.type === 'pool') + ) + const nContainers = Object.keys(containers).length + + assert.equal(nContainers, 0, `expected no container, received ${nContainers}: ${summarize(containers)}`) + }) + }) + + // Filtering is computed per user. A regression that reused one user's filter + // for every connection would still pass every test above, since they all + // observe a single account. + describe('two users connected at once', () => { + let otherUser + let otherConnection + let otherRecorder + + before(async () => { + otherUser = await createTestUser(admin) + otherConnection = await connectAs(otherUser) + otherRecorder = recordPushedObjects(otherConnection) + + await grant(admin, testUser.id, referenceVmId) + await wait(SETTLE) + }) + + after(async () => { + otherRecorder?.stop() + await otherConnection?.close() + await revoke(admin, testUser.id, referenceVmId) + if (otherUser !== undefined) { + await deleteTestUser(admin, otherUser.id) + } + }) + + it('the granted user receives the VM', async () => { + const objects = await fetchAllObjects(user) + + assert.ok(referenceVmId in objects, 'the granted user did not receive the VM') + }) + + it('the other user receives nothing', async () => { + const objects = await fetchAllObjects(otherConnection) + const received = Object.keys(objects).length + + assert.equal(received, 0, `the ungranted user received ${received} objects: ${summarize(objects)}`) + }) + + it('an update to the VM is pushed only to the granted user', async () => { + recorder.reset() + otherRecorder.reset() + + await withTag(admin, referenceVmId, `qa-acl-${Date.now()}`, () => wait(EVENT_WINDOW)) + + assert.ok(recorder.entered.has(referenceVmId), 'the granted user was not told about the change') + + const leaked = Object.fromEntries(otherRecorder.entered) + const nLeaked = otherRecorder.entered.size + assert.equal(nLeaked, 0, `the ungranted user was pushed ${nLeaked} objects: ${summarize(leaked)}`) + }) + + // An ACL change invalidates every cached permission set, but concerns only + // its own subject. Without the comparison that follows the invalidation, + // one grant re-sends the whole visible set of every connected client. + it('does not re-push to a user whose own permissions did not change', async () => { + // let the previous test's tag cleanup reach us before we start counting + await wait(SETTLE) + recorder.reset() + + await grant(admin, otherUser.id, referenceVmId) + try { + await wait(SETTLE) + + const pushed = Object.fromEntries(recorder.entered) + const nPushed = recorder.entered.size + assert.equal( + nPushed, + 0, + `an unrelated grant re-sent ${nPushed} objects to a user whose permissions were untouched: ${summarize(pushed)}` + ) + } finally { + await revoke(admin, otherUser.id, referenceVmId) + } + }) + }) + + // The event stream only carries *changes*, so a user granted access to an + // object that already exists would never hear about it. The server pushes + // their visible set on a permission change to close that gap — nothing else + // in the suite, nor in the unit tests, covers it. + describe('a permission granted while connected', () => { + before(async () => { + // The grant below must be a *change*, in two senses. An ACL that already + // exists is silently ignored and emits no event at all. And a grant made + // inside the debounce window of a preceding revocation is merged with + // it: the comparison is then made against the state from before that + // revocation, the permissions come out identical, and no push is due — + // correctly, since the client never lost the objects. + // + // So: revoke, then let the window close. + await revoke(admin, testUser.id, referenceVmId) + await wait(SETTLE) + }) + + after(async () => { + await revoke(admin, testUser.id, referenceVmId) + }) + + it('reaches the client without reconnecting', async () => { + const before = await user.call('acl.getCurrentPermissions') + assert.equal( + Object.keys(before).length, + 0, + `the user already holds permissions, so the grant would change nothing: ${Object.keys(before).join(', ')}` + ) + + recorder.reset() + + await grant(admin, testUser.id, referenceVmId) + const arrived = await waitUntil(() => recorder.entered.has(referenceVmId)) + + assert.ok(arrived, `the VM was never pushed after the grant (received ${recorder.entered.size} objects)`) + }) + + it('stops reaching the client once revoked', async () => { + await revoke(admin, testUser.id, referenceVmId) + await wait(SETTLE) + + // a fresh fetch must no longer return it + const objects = await fetchAllObjects(user) + const received = Object.keys(objects).length + assert.equal(received, 0, `expected no object after the revocation, received ${received}: ${summarize(objects)}`) + + // and no further update about it must be pushed + recorder.reset() + await withTag(admin, referenceVmId, `qa-acl-${Date.now()}`, () => wait(EVENT_WINDOW)) + + assert.ok( + !recorder.entered.has(referenceVmId), + 'updates about the VM were still pushed after the permission was revoked' + ) + }) + }) + + // Self-service users hold no ACL on the resources they build from: the + // server grants them an implicit `view` on their resource sets, which is + // what keeps the VM creation form usable once filtering is on. + describe('a user who is the subject of a resource set', () => { + let resourceSetId + let setObjects + let outsider + let outsiderConnection + let visible + + before(async () => { + const everything = await fetchAllObjects(admin) + setObjects = pickObjectsByType(everything, referencePoolId, ['VM-template', 'SR', 'network']) + for (const type of ['VM-template', 'SR', 'network']) { + assert.notEqual(setObjects[type], undefined, `no ${type} found in pool ${referencePoolId}`) + } + + const set = await admin.call('resourceSet.create', { + name: `qa-acl-${Date.now()}`, + subjects: [testUser.id], + objects: Object.values(setObjects), + }) + resourceSetId = set.id + + outsider = await createTestUser(admin) + outsiderConnection = await connectAs(outsider) + + await wait(SETTLE) + visible = await fetchAllObjects(user) + }) + + after(async () => { + await outsiderConnection?.close() + if (outsider !== undefined) { + await deleteTestUser(admin, outsider.id) + } + if (resourceSetId !== undefined) { + await admin + .call('resourceSet.delete', { id: resourceSetId }) + .catch(error => log.warn('failed to delete the test resource set', { error })) + } + }) + + it('receives the resources of the set', () => { + for (const [type, id] of Object.entries(setObjects)) { + assert.ok(id in visible, `the ${type} of the resource set was not returned; received ${summarize(visible)}`) + } + }) + + // what the ISO and disk pickers of the VM creation form rely on + it('receives the disks of the set SR', () => { + const vdis = Object.values(visible).filter(object => object.type === 'VDI' && object.$SR === setObjects.SR) + + assert.ok(vdis.length > 0, `expected disks on the resource set SR; received ${summarize(visible)}`) + }) + + it('receives no VM belonging to anyone else', () => { + // guard against passing for the wrong reason: a user who sees nothing at + // all trivially sees no VM + assert.ok(Object.keys(visible).length > 0, 'the user sees nothing, so this proves nothing') + + const vms = Object.values(visible).filter(object => object.type === 'VM') + + assert.equal(vms.length, 0, `a resource set must not expose VMs, received ${vms.length}`) + }) + + it('can read the set it is a subject of', async () => { + const set = await user.call('resourceSet.get', { id: resourceSetId }) + + assert.equal(set.id, resourceSetId) + }) + + it('is refused to a user who is not a subject', async () => { + await assert.rejects( + () => outsiderConnection.call('resourceSet.get', { id: resourceSetId }), + error => error.code === 2, + 'a non-subject was able to read the resource set' + ) + }) + }) + + // Filtering must not reach admins: over-filtering would break XO for the + // people who run it, and no other test would notice. + describe('an admin', () => { + it('still receives the whole infrastructure', async () => { + const objects = await fetchAllObjects(admin) + + assert.ok(referenceVmId in objects, 'the admin lost sight of the reference VM') + + for (const type of ['pool', 'host', 'SR', 'network', 'VM']) { + const n = Object.values(objects).filter(object => object.type === type).length + assert.ok(n > 0, `the admin received no ${type}; got ${summarize(objects)}`) + } + + const pools = new Set(Object.values(objects).map(object => object.$pool)) + assert.ok(pools.size > 0, 'the admin received objects from no pool at all') + + log.debug('admin view unchanged', { objects: Object.keys(objects).length, pools: pools.size }) + }) + }) + + // Methods that returned infrastructure-wide data to any authenticated user. + describe('methods restricted to admins', () => { + const RESTRICTED = ['pool.listPoolsMatchingCriteria', 'backupNg.getSuggestedExcludedTags'] + + // kept open on purpose, see the comment in src/api/cloud-config.mjs + const OPEN = ['cloudConfig.getAll', 'cloudConfig.getAllNetworkConfigs'] + + for (const method of RESTRICTED) { + it(`${method} is refused to a non-admin`, async () => { + await assert.rejects( + () => user.call(method, {}), + error => error.code === 2, + `${method} did not refuse a non-admin` + ) + }) + } + + for (const method of OPEN) { + it(`${method} still answers a non-admin`, async () => { + await assert.doesNotReject(() => user.call(method, {})) + }) + } + + // the lock must not cost admins the feature + for (const method of RESTRICTED) { + it(`${method} still answers an admin`, async () => { + await assert.doesNotReject(() => admin.call(method, {})) + }) + } + }) +}) diff --git a/@xen-orchestra/qa-test/utils/aclUtils.js b/@xen-orchestra/qa-test/utils/aclUtils.js new file mode 100644 index 0000000000..5e2e5b983c --- /dev/null +++ b/@xen-orchestra/qa-test/utils/aclUtils.js @@ -0,0 +1,393 @@ +import { createLogger } from '@xen-orchestra/log' + +import { getRequiredEnv } from './index.js' +import { xoConnection } from '../client/xoLibClient.js' + +const log = createLogger('xo:qa-test:acl') + +/** + * The subset of the xo-lib client this suite uses. + * + * @typedef {{ + * user: { id: string, email: string, permission: string }, + * call: (method: string, params?: object) => Promise, + * on: (event: string, listener: (...args: any[]) => void) => void, + * removeListener: (event: string, listener: (...args: any[]) => void) => void, + * close: () => Promise, + * }} XoClient + */ + +/** + * How long to watch the event stream before concluding that nothing arrived. + * + * The event assertions are the only source of flakiness in the ACL suite: + * they prove a *negative* ("nothing was pushed"), so the window has to be long + * enough for a change made by the admin to have travelled back. + */ +const configured = Number(process.env.ACL_EVENT_WINDOW) +export const EVENT_WINDOW = Number.isFinite(configured) && configured > 0 ? configured : 10_000 + +/** Prefix of every account created by this suite, so leftovers are recognizable. */ +export const TEST_USER_PREFIX = 'qa-acl-' + +export const wait = ms => new Promise(resolve => setTimeout(resolve, ms)) + +/** + * Opens a second, independent XO connection signed in as another account. + * + * The whole suite rests on this: an admin connection arranges the fixtures + * while a low-privilege connection observes what the server actually sends it. + * The bug is only visible on the wire — xo-web filters client-side, so the UI + * looks identical whether or not the server does its job. + * + * @param {object} credentials + * @param {string} credentials.email + * @param {string} credentials.password + * @returns {Promise} a signed-in xo-lib client + */ +export function connectAs({ email, password }) { + return xoConnection({ + xoUrl: getRequiredEnv('HOSTNAME'), + // XO looks accounts up by name, which is the email for local accounts + username: email, + password, + }) +} + +/** + * Creates a throw-away account. + * + * @param {XoClient} xo - an admin connection + * @param {object} [opts] + * @param {string} [opts.permission] - defaults to `none`, i.e. the "User" role + * @returns {Promise<{ id: string, email: string, password: string }>} + */ +export async function createTestUser(xo, { permission = 'none' } = {}) { + const email = `${TEST_USER_PREFIX}${Date.now()}-${Math.random().toString(36).slice(2, 8)}@qa.test` + const password = Math.random().toString(36).slice(2) + Math.random().toString(36).slice(2) + + const id = await xo.call('user.create', { email, password, permission }) + log.debug('created test user', { id, email, permission }) + + return { id, email, password } +} + +/** + * Deletes an account, ignoring the error if it is already gone, so that + * teardown never masks the failure that led to it. + * + * @param {XoClient} xo - an admin connection + * @param {string} id + */ +export async function deleteTestUser(xo, id) { + try { + await xo.call('user.delete', { id }) + log.debug('deleted test user', { id }) + } catch (error) { + log.warn('failed to delete test user', { error, id }) + } +} + +/** + * Creates a throw-away group. + * + * @param {XoClient} xo - an admin connection + * @returns {Promise<{ id: string, name: string }>} + */ +export async function createTestGroup(xo) { + const name = `${TEST_USER_PREFIX}${Date.now()}-${Math.random().toString(36).slice(2, 8)}` + const id = await xo.call('group.create', { name }) + log.debug('created test group', { id, name }) + + return { id, name } +} + +/** + * Deletes a group, ignoring the error if it is already gone, so that teardown + * never masks the failure that led to it. + * + * @param {XoClient} xo - an admin connection + * @param {string} id + */ +export async function deleteTestGroup(xo, id) { + try { + await xo.call('group.delete', { id }) + log.debug('deleted test group', { id }) + } catch (error) { + log.warn('failed to delete test group', { error, id }) + } +} + +/** + * Accumulates everything the server pushes on a connection. + * + * Note that `exited` may legitimately contain ids the connection never + * received: removals carry ids only and the object is already gone from the + * collection when the event is emitted, so the server cannot evaluate who was + * allowed to see it and forwards them to everyone. Assertions about leaked + * data must therefore be made on `entered`. + * + * @param {XoClient} xo + * @returns {{ entered: Map, exited: Set, reset(): void, stop(): void }} + */ +export function recordPushedObjects(xo) { + const entered = new Map() + const exited = new Set() + + const onNotification = notification => { + if (notification.method !== 'all') { + return + } + + const { type, items } = notification.params + for (const id of Object.keys(items)) { + if (type === 'enter') { + entered.set(id, items[id]) + } else { + exited.add(id) + } + } + } + + xo.on('notification', onNotification) + + return { + entered, + exited, + reset() { + entered.clear() + exited.clear() + }, + stop() { + xo.removeListener('notification', onNotification) + }, + } +} + +/** + * Everything the server is willing to hand over to this connection. + * + * @param {XoClient} xo + * @returns {Promise<{ [id: string]: object }>} + */ +export function fetchAllObjects(xo) { + return xo.call('xo.getAllObjects', {}) +} + +/** + * Counts the returned objects by type, to make failures readable. + * + * @param {{ [id: string]: object }} objects + * @returns {{ [type: string]: number }} + */ +export function countByType(objects) { + const counts = {} + for (const id of Object.keys(objects)) { + const { type } = objects[id] + counts[type] = (counts[type] ?? 0) + 1 + } + return counts +} + +/** + * Polls until a condition holds, or gives up. + * + * Preferable to a flat sleep for assertions that wait for something to + * *happen*: the test finishes as soon as it does, and only pays the full + * timeout when it genuinely failed. + * + * @param {() => boolean} predicate + * @param {object} [opts] + * @param {number} [opts.timeout] + * @param {number} [opts.interval] + * @returns {Promise} whether the condition held before the timeout + */ +export async function waitUntil(predicate, { timeout = EVENT_WINDOW, interval = 250 } = {}) { + const deadline = Date.now() + timeout + do { + if (predicate()) { + return true + } + await wait(interval) + } while (Date.now() < deadline) + + return predicate() +} + +/** + * Picks one object of each given type belonging to a pool. + * + * @param {{ [id: string]: object }} objects - the admin's view + * @param {string} poolId + * @param {string[]} types + * @returns {{ [type: string]: string | undefined }} one id per type + */ +export function pickObjectsByType(objects, poolId, types) { + const picked = {} + for (const id of Object.keys(objects)) { + const object = objects[id] + if (object.$pool === poolId && types.includes(object.type) && picked[object.type] === undefined) { + picked[object.type] = id + } + } + return picked +} + +/** + * Finds the SR holding one of a VM's disks, along with that disk. + * + * Granting an SR is only a meaningful non-leak test if a VM actually lives on + * it: the point is to prove that reaching a VM's storage does not reach the VM. + * + * @param {{ [id: string]: object }} objects - the admin's view + * @param {string} vmId + * @returns {{ srId: string, vdiId: string } | undefined} + */ +export function findVmStorage(objects, vmId) { + const vm = objects[vmId] + for (const vbdId of vm?.$VBDs ?? []) { + const vbd = objects[vbdId] + if (vbd === undefined || vbd.is_cd_drive || vbd.VDI == null) { + continue + } + + const vdi = objects[vbd.VDI] + if (vdi?.$SR != null) { + return { srId: vdi.$SR, vdiId: vdi.id } + } + } +} + +/** + * Renders a type breakdown on a single line, most numerous first. + * + * Assertions in this suite compare *counts*, never the object collections + * themselves: a failing `deepEqual` on 28k ids buries the result under a diff + * nobody can read. The numbers go in the assertion, the detail goes here. + * + * @param {{ [id: string]: object }} objects + * @param {number} [max] - number of types to show before summarising the rest + * @returns {string} + */ +export function summarize(objects, max = 6) { + const entries = Object.entries(countByType(objects)).sort((a, b) => b[1] - a[1]) + if (entries.length === 0) { + return 'nothing' + } + + const shown = entries.slice(0, max).map(([type, count]) => `${count} ${type}`) + const remaining = entries.length - max + if (remaining > 0) { + shown.push(`and ${remaining} other type${remaining > 1 ? 's' : ''}`) + } + return shown.join(', ') +} + +/** + * Makes an object change so that the server has something to push. + * + * Tagging goes all the way to XAPI, so it produces a genuine object update + * rather than something synthesised by xo-server. + * + * @param {XoClient} xo - an admin connection + * @param {string} objectId + * @param {string} tag + */ +export async function withTag(xo, objectId, tag, fn) { + await xo.call('tag.add', { id: objectId, tag }) + try { + return await fn() + } finally { + await xo.call('tag.remove', { id: objectId, tag }).catch(error => { + log.warn('failed to remove the test tag', { error, objectId, tag }) + }) + } +} + +/** + * Grants a permission, making sure it is a *change*. + * + * `addAcl` silently ignores an ACL that already exists, which emits no event + * and therefore triggers no invalidation — correct behaviour, but it turns a + * test that expects a push into a silent no-op. Revoking first guarantees the + * grant produces something to observe. + * + * @param {XoClient} xo - an admin connection + * @param {string} subject - a user or group id + * @param {string} object + * @param {string} [action] + */ +export async function grant(xo, subject, object, action = 'viewer') { + await revoke(xo, subject, object, action) + await xo.call('acl.add', { subject, object, action }) +} + +/** + * Revokes a permission, tolerating its absence. + * + * @param {XoClient} xo - an admin connection + * @param {string} subject + * @param {string} object + * @param {string} [action] + */ +export async function revoke(xo, subject, object, action = 'viewer') { + try { + await xo.call('acl.remove', { subject, object, action }) + } catch (error) { + log.warn('failed to revoke an ACL', { error, subject, object, action }) + } +} + +/** + * Deletes whatever a previous run left behind. + * + * An `after` hook does not run when its `before` threw, so a suite that fails + * half way leaves users, groups and resource sets on the appliance — and their + * ACLs with them. The next run then starts from a state it did not choose, + * which is how a grant becomes a no-op and a test fails for reasons that have + * nothing to do with the code under test. + * + * Deleting a user or a group removes its ACLs, so they need no separate pass. + * + * @param {XoClient} xo - an admin connection + */ +export async function purgeLeftoverFixtures(xo) { + const mine = name => name !== undefined && name.startsWith(TEST_USER_PREFIX) + + const [users, groups, resourceSets] = await Promise.all([ + xo.call('user.getAll'), + xo.call('group.getAll'), + xo.call('resourceSet.getAll'), + ]) + + const stale = { + users: users.filter(({ email }) => mine(email)), + groups: groups.filter(({ name }) => mine(name)), + resourceSets: resourceSets.filter(({ name }) => mine(name)), + } + + const nStale = stale.users.length + stale.groups.length + stale.resourceSets.length + if (nStale === 0) { + return + } + + log.warn('deleting fixtures left by a previous run', { + users: stale.users.length, + groups: stale.groups.length, + resourceSets: stale.resourceSets.length, + }) + + for (const [method, records] of [ + ['user.delete', stale.users], + ['group.delete', stale.groups], + ['resourceSet.delete', stale.resourceSets], + ]) { + for (const { id } of records) { + try { + await xo.call(method, { id }) + } catch (error) { + log.warn('failed to delete a leftover fixture', { error, method, id }) + } + } + } +} diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index e9e64cf13b..34b4188013 100644 --- a/CHANGELOG.unreleased.md +++ b/CHANGELOG.unreleased.md @@ -52,6 +52,7 @@ - [XO5/Host/Patches] Warn when installing patches on a host while its pool master is not up to date (PR [#10523](https://github.com/vatesfr/xen-orchestra/pull/10523)) - [XO6] Add copy button for multiple information on Host/VM Dashboard and Pool/Host/VM System tabs (PR [#9976](https://github.com/vatesfr/xen-orchestra/pull/9976)) - [XO6] Export VDI as vmdk (PR [#10591](https://github.com/vatesfr/xen-orchestra/pull/10591)) +- [XO5] Improve event handling (PR [#10568](https://github.com/vatesfr/xen-orchestra/pull/10568)) ### Bug fixes @@ -106,6 +107,7 @@ - @xen-orchestra/backup-archive patch - @xen-orchestra/backups minor - @xen-orchestra/fs minor +- @xen-orchestra/qa-test minor - @xen-orchestra/qcow2 patch - @xen-orchestra/rest-api minor - @xen-orchestra/vmdk major @@ -115,6 +117,7 @@ - @xen-orchestra/xapi minor - vhd-lib minor - xen-api minor +- xo-acl-resolver minor - xo-cli patch - xo-server minor - xo-server-load-balancer minor diff --git a/packages/xo-acl-resolver/index.js b/packages/xo-acl-resolver/index.js index 47037baeff..e268d74422 100644 --- a/packages/xo-acl-resolver/index.js +++ b/packages/xo-acl-resolver/index.js @@ -57,6 +57,28 @@ const checkSelf = ({ id }, permission) => { return permissionsForObject && permissionsForObject[permission] } +function checkVdi(vdi, permission) { + // Check authorization for the containing SR. + if (checkAuthorization(vdi.$SR, permission)) { + return true + } + + // Check authorization for each of the connected VMs. + for (const vbdId of vdi.$VBDs) { + const vbd = getObject(vbdId) + + if (vbd === undefined) { + continue + } + + if (checkAuthorization(vbd.VM, permission)) { + return true + } + } + + return false +} + // =================================================================== const checkAuthorizationByTypes = { @@ -84,24 +106,12 @@ const checkAuthorizationByTypes = { // Access to a VDI is granted if the user has access to the // containing SR or to a linked VM. - VDI(vdi, permission) { - // Check authorization for the containing SR. - if (checkAuthorization(vdi.$SR, permission)) { - return true - } - - // Check authorization for each of the connected VMs. - for (const vbdId of vdi.$VBDs) { - if (checkAuthorization(getObject(vbdId).VM, permission)) { - return true - } - } - - return false - }, + VDI: checkVdi, 'VDI-snapshot': checkMember('$snapshot_of'), + 'VDI-unmanaged': checkVdi, + VIF: or(checkMember('$network'), checkMember('$VM')), VM: or(checkSelf, checkMember('$container')), @@ -167,3 +177,18 @@ exports.check = function checkPermissions() { throw error } } + +// We use this function to test if a user can see an object, instead of check +// as its throw/catch costs ~2.5 µs per denied object +exports.test = function testPermission(permissionsByObject_, getObject_, objectId, permission) { + // Assign global variables. + permissionsByObject = permissionsByObject_ + getObject = getObject_ + + try { + return Boolean(checkAuthorization(objectId, permission)) + } finally { + // Free the global variables. + permissionsByObject = getObject = null + } +} diff --git a/packages/xo-server/src/_objectNotifications.mjs b/packages/xo-server/src/_objectNotifications.mjs new file mode 100644 index 0000000000..0e6807baeb --- /dev/null +++ b/packages/xo-server/src/_objectNotifications.mjs @@ -0,0 +1,43 @@ +import isEmpty from 'lodash/isEmpty.js' + +/** + * @typedef {import('./_objectVisibility.mjs').IsObjectVisible} IsObjectVisible + */ + +/** + * Turns a batch of object changes into the messages to push to a single + * connection. + * + * An object can leave a user's scope without being destroyed — a VM migrated + * to a host they cannot see reaches us as an update — so an entered object the + * user may not see is reported as exited, otherwise their client would keep a + * stale copy of it forever. + * + * Exits are forwarded as-is: the object is already gone from the collection + * when the event is emitted, so its visibility can no longer be evaluated. + * They carry ids only. + * + * @param {object} opts + * @param {{ [id: string]: object }} opts.entered + * @param {{ [id: string]: 0 }} opts.exited + * @param {IsObjectVisible} [opts.isObjectVisible] + * @returns {{ enter?: { [id: string]: object }, exit?: { [id: string]: 0 } }} + */ +export function computeObjectNotifications({ entered, exited, isObjectVisible }) { + let enter + let exit + + for (const objectId in entered) { + if (isObjectVisible(objectId)) { + if (enter === undefined) enter = { __proto__: null } + enter[objectId] = entered[objectId] + } else { + if (exit === undefined) exit = { __proto__: null } + exit[objectId] = 0 + } + } + + if (!isEmpty(exited)) exit = { ...exit, ...exited } + + return { enter, exit } +} diff --git a/packages/xo-server/src/_objectNotifications.test.mjs b/packages/xo-server/src/_objectNotifications.test.mjs new file mode 100644 index 0000000000..9518fa7c51 --- /dev/null +++ b/packages/xo-server/src/_objectNotifications.test.mjs @@ -0,0 +1,109 @@ +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' + +import { computeObjectNotifications } from './_objectNotifications.mjs' + +// =================================================================== + +const VM_1 = { id: 'vm-1', type: 'VM' } +const VM_2 = { id: 'vm-2', type: 'VM' } + +// mimics what `Xo#_watchObjects` accumulates between two `finish` events +const entered = (...objects) => Object.fromEntries(objects.map(object => [object.id, object])) +const exited = (...ids) => Object.fromEntries(ids.map(id => [id, 0])) + +const onlyVm1 = id => id === 'vm-1' + +// =================================================================== + +describe('computeObjectNotifications', function () { + it('forwards everything when the predicate accepts everything', function () { + const { enter, exit } = computeObjectNotifications({ + entered: entered(VM_1, VM_2), + exited: exited('vm-3'), + isObjectVisible: () => true, + }) + + assert.deepEqual({ ...enter }, { 'vm-1': VM_1, 'vm-2': VM_2 }) + assert.deepEqual({ ...exit }, { 'vm-3': 0 }) + }) + + it('drops entered objects the user cannot see', function () { + const { enter } = computeObjectNotifications({ + entered: entered(VM_1, VM_2), + exited: {}, + isObjectVisible: onlyVm1, + }) + + assert.deepEqual({ ...enter }, { 'vm-1': VM_1 }) + }) + + // an object can leave the user's scope without being destroyed, e.g. a VM + // migrated to a host they cannot see: it reaches us as an update, and the + // client must be told to forget it rather than keep a stale copy + it('reports an entered object the user cannot see as exited', function () { + const { enter, exit } = computeObjectNotifications({ + entered: entered(VM_1, VM_2), + exited: {}, + isObjectVisible: onlyVm1, + }) + + assert.deepEqual({ ...enter }, { 'vm-1': VM_1 }) + assert.deepEqual({ ...exit }, { 'vm-2': 0 }) + }) + + // the object is already gone from the collection when `exit` is emitted, so + // its visibility can no longer be evaluated: exits carry ids only and are + // forwarded to everyone + it('forwards exits regardless of visibility', function () { + const { exit } = computeObjectNotifications({ + entered: {}, + exited: exited('vm-2'), + isObjectVisible: onlyVm1, + }) + + assert.deepEqual({ ...exit }, { 'vm-2': 0 }) + }) + + it('merges invisible entries into the existing exits', function () { + const { exit } = computeObjectNotifications({ + entered: entered(VM_2), + exited: exited('vm-3'), + isObjectVisible: onlyVm1, + }) + + assert.deepEqual({ ...exit }, { 'vm-2': 0, 'vm-3': 0 }) + }) + + it('returns no message when there is nothing to send', function () { + const { enter, exit } = computeObjectNotifications({ + entered: {}, + exited: {}, + isObjectVisible: onlyVm1, + }) + + assert.equal(enter, undefined) + assert.equal(exit, undefined) + }) + + it('returns no enter message when every entered object is hidden', function () { + const { enter } = computeObjectNotifications({ + entered: entered(VM_2), + exited: {}, + isObjectVisible: onlyVm1, + }) + + assert.equal(enter, undefined) + }) + + // the same batch is used to build the messages of every connection + it('does not mutate the batch it was given', function () { + const enteredBatch = entered(VM_1, VM_2) + const exitedBatch = exited('vm-3') + + computeObjectNotifications({ entered: enteredBatch, exited: exitedBatch, isObjectVisible: onlyVm1 }) + + assert.deepEqual(enteredBatch, { 'vm-1': VM_1, 'vm-2': VM_2 }) + assert.deepEqual(exitedBatch, { 'vm-3': 0 }) + }) +}) diff --git a/packages/xo-server/src/_objectVisibility.mjs b/packages/xo-server/src/_objectVisibility.mjs new file mode 100644 index 0000000000..d216d810e6 --- /dev/null +++ b/packages/xo-server/src/_objectVisibility.mjs @@ -0,0 +1,71 @@ +import { test as testPermission } from 'xo-acl-resolver' + +/** + * @typedef {(objectId: string) => boolean} IsObjectVisible + */ + +/** + * Recognizable instance of IsObjectVisible so no one has to treat `undefined` as everything + * + * @type {IsObjectVisible} + */ +export const ALL_VISIBLE = () => true + +/** + * Builds the predicate deciding whether a user may see an XO object. + * + * It is the very same resolver xo-web uses to filter its own store, so the + * result is identical to what a non-admin UI already displays. + * + * The returned predicate memoizes its answers and must therefore not outlive + * the batch of objects it was created for, as the object graph keeps moving. + * Its permission map, on the other hand, only changes when permissions do, so + * callers that already hold one should pass it as `permissionsByObject` and + * omit `resourceSetObjectIds` rather than have it rebuilt on every batch. + * + * @param {object} opts + * @param {(id: string) => object | undefined} opts.getObject + * @param {{ [objectId: string]: { [permission: string]: 1 } }} opts.permissionsByObject + * @param {string[]} [opts.resourceSetObjectIds] + * @returns {IsObjectVisible} + */ +export function createIsObjectVisible({ getObject, permissionsByObject, resourceSetObjectIds = [] }) { + const permissions = createPermissions({ permissionsByObject, resourceSetObjectIds }) + + const cache = new Map() + return function isObjectVisible(objectId) { + if (cache.has(objectId)) { + return cache.get(objectId) + } else { + const visible = testPermission(permissions, getObject, objectId, 'view') + cache.set(objectId, visible) + + return visible + } + } +} + +/** + * Objects of a resource set are granted an implicit `view` permission: they + * carry no ACL of their own, but their subjects legitimately see them — and, + * through the resolver, their children, e.g. the VDIs of a resource set SR. + * + * @param {object} opts + * @param {{ [objectId: string]: { [permission: string]: 1 } }} opts.permissionsByObject + * @param {string[]} opts.resourceSetObjectIds + * @returns {{ [objectId: string]: { [permission: string]: 1 } }} + */ +export function createPermissions({ permissionsByObject, resourceSetObjectIds }) { + let permissions + if (resourceSetObjectIds.length > 0) { + permissions = { __proto__: null, ...permissionsByObject } + } else { + permissions = permissionsByObject + } + + for (const objectId of resourceSetObjectIds) { + permissions[objectId] = { ...permissions[objectId], view: 1 } + } + + return permissions +} diff --git a/packages/xo-server/src/_objectVisibility.test.mjs b/packages/xo-server/src/_objectVisibility.test.mjs new file mode 100644 index 0000000000..38482143ca --- /dev/null +++ b/packages/xo-server/src/_objectVisibility.test.mjs @@ -0,0 +1,184 @@ +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' + +import { createIsObjectVisible } from './_objectVisibility.mjs' + +// =================================================================== + +// A minimal but realistic XO object graph, shaped like the output of +// `xapi-object-to-xo.mjs`: +// +// pool-1 +// ├── host-1 +// │ ├── vm-1 ── vbd-1 ── vdi-1 (on sr-1) +// │ │ └─ vif-1 (on net-1) +// │ └── vm-2 +// │ └── pgpu-1 (type unknown to xo-acl-resolver) +// ├── sr-1 +// ├── net-1 +// └── tpl-1 +const OBJECTS = { + 'pool-1': { id: 'pool-1', type: 'pool' }, + 'host-1': { id: 'host-1', type: 'host', $pool: 'pool-1' }, + 'vm-1': { id: 'vm-1', type: 'VM', $container: 'host-1', $pool: 'pool-1', $VBDs: ['vbd-1'] }, + 'vm-2': { id: 'vm-2', type: 'VM', $container: 'host-1', $pool: 'pool-1', $VBDs: [] }, + 'sr-1': { id: 'sr-1', type: 'SR', $container: 'pool-1', $pool: 'pool-1' }, + 'vdi-1': { id: 'vdi-1', type: 'VDI', $SR: 'sr-1', $VBDs: ['vbd-1'] }, + // same XAPI object as a VDI, only not handled by the SM + 'vdi-u': { id: 'vdi-u', type: 'VDI-unmanaged', $SR: 'sr-1', $VBDs: [] }, + 'vdi-u-vm': { id: 'vdi-u-vm', type: 'VDI-unmanaged', $SR: 'sr-other', $VBDs: ['vbd-1'] }, + // a disk whose VBD link dangles, as happens mid-update + 'vdi-dangling': { id: 'vdi-dangling', type: 'VDI', $SR: 'sr-other', $VBDs: ['vbd-gone'] }, + 'vbd-1': { id: 'vbd-1', type: 'VBD', VDI: 'vdi-1', VM: 'vm-1' }, + 'net-1': { id: 'net-1', type: 'network', $pool: 'pool-1' }, + 'vif-1': { id: 'vif-1', type: 'VIF', $network: 'net-1', $VM: 'vm-1' }, + 'tpl-1': { id: 'tpl-1', type: 'VM-template', $pool: 'pool-1' }, + 'pgpu-1': { id: 'pgpu-1', type: 'PGPU', $host: 'host-1' }, +} + +const view = (...ids) => { + const permissions = { __proto__: null } + for (const id of ids) { + permissions[id] = { view: 1 } + } + return permissions +} + +const makeIsVisible = ({ permissionsByObject = {}, resourceSetObjectIds = [], getObject } = {}) => + createIsObjectVisible({ + getObject: getObject ?? (id => OBJECTS[id]), + permissionsByObject, + resourceSetObjectIds, + }) + +// =================================================================== + +describe('createIsObjectVisible', function () { + describe('ACL-derived visibility', function () { + it('hides everything from a user without any ACL', function () { + const isVisible = makeIsVisible() + + for (const id of Object.keys(OBJECTS)) { + assert.equal(isVisible(id), false, `${id} should not be visible`) + } + }) + + it('grants access to an explicitly granted object', function () { + const isVisible = makeIsVisible({ permissionsByObject: view('vm-1') }) + + assert.equal(isVisible('vm-1'), true) + assert.equal(isVisible('vm-2'), false) + }) + + it('inherits access from the container', function () { + const isVisible = makeIsVisible({ permissionsByObject: view('pool-1') }) + + for (const id of ['pool-1', 'host-1', 'vm-1', 'vm-2', 'sr-1', 'net-1', 'tpl-1']) { + assert.equal(isVisible(id), true, `${id} should be visible`) + } + }) + + const objectContainerArray = [ + ['vdi-1', 'sr-1'], + ['vdi-1', 'vm-1'], + ['vdi-u', 'sr-1'], + ['vdi-u-vm', 'vm-1'], + ['vif-1', 'net-1'], + ['vif-1', 'vm-1'], + ] + for (const [objectId, containerId] of objectContainerArray) { + it(`resolves ${objectId} through ${containerId}`, function () { + assert.equal(makeIsVisible({ permissionsByObject: view(containerId) })(objectId), true) + }) + } + + // `getObject` returns undefined for an unknown id on the server, where it + // used to return an empty object in the client: dereferencing it blindly + // would throw on the hot path + it('does not throw when a VBD link dangles', function () { + const isVisible = makeIsVisible({ permissionsByObject: view('vm-1') }) + + assert.equal(isVisible('vdi-dangling'), false) + }) + + it('does not expose a VM through the SR holding its disks', function () { + const isVisible = makeIsVisible({ permissionsByObject: view('sr-1') }) + + assert.equal(isVisible('vm-1'), false) + assert.equal(isVisible('vm-2'), false) + }) + + it('hides object types the resolver does not know about', function () { + const isVisible = makeIsVisible({ permissionsByObject: view('host-1') }) + + assert.equal(isVisible('host-1'), true) + assert.equal(isVisible('pgpu-1'), false) + }) + + it('returns false for an unknown id instead of throwing', function () { + const isVisible = makeIsVisible({ permissionsByObject: view('pool-1') }) + + assert.equal(isVisible('does-not-exist'), false) + }) + }) + + describe('resource set visibility', function () { + it('grants access to the objects of the resource sets the user is a subject of', function () { + const isVisible = makeIsVisible({ resourceSetObjectIds: ['tpl-1', 'sr-1', 'net-1'] }) + + assert.equal(isVisible('tpl-1'), true) + assert.equal(isVisible('sr-1'), true) + assert.equal(isVisible('net-1'), true) + }) + + it('resolves the VDIs of a resource set SR, as the ISO picker needs them', function () { + const isVisible = makeIsVisible({ resourceSetObjectIds: ['sr-1'] }) + + assert.equal(isVisible('vdi-1'), true) + }) + + it('does not expose other users VMs through a shared resource set SR', function () { + const isVisible = makeIsVisible({ resourceSetObjectIds: ['sr-1'] }) + + assert.equal(isVisible('vm-1'), false) + assert.equal(isVisible('vm-2'), false) + }) + + it('combines ACLs and resource sets', function () { + const isVisible = makeIsVisible({ + permissionsByObject: view('vm-1'), + resourceSetObjectIds: ['tpl-1'], + }) + + assert.equal(isVisible('vm-1'), true) + assert.equal(isVisible('tpl-1'), true) + assert.equal(isVisible('vm-2'), false) + }) + + it('does not mutate the permissions it was given', function () { + const permissionsByObject = view('vm-1') + makeIsVisible({ permissionsByObject, resourceSetObjectIds: ['tpl-1'] })('tpl-1') + + assert.deepEqual(Object.keys(permissionsByObject), ['vm-1']) + }) + }) + + describe('caching', function () { + it('resolves a given id only once', function () { + let nCalls = 0 + const isVisible = makeIsVisible({ + permissionsByObject: view('pool-1'), + getObject: id => { + nCalls++ + return OBJECTS[id] + }, + }) + + assert.equal(isVisible('vm-1'), true) + const nCallsAfterFirstLookup = nCalls + + assert.equal(isVisible('vm-1'), true) + assert.equal(nCalls, nCallsAfterFirstLookup) + }) + }) +}) diff --git a/packages/xo-server/src/api/_xo.test.mjs b/packages/xo-server/src/api/_xo.test.mjs new file mode 100644 index 0000000000..21163da998 --- /dev/null +++ b/packages/xo-server/src/api/_xo.test.mjs @@ -0,0 +1,120 @@ +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' +import { Writable } from 'node:stream' +import { ALL_VISIBLE } from '../_objectVisibility.mjs' + +import * as handlers from './xo.mjs' + +// =================================================================== + +const OBJECTS = { + 'vm-1': { id: 'vm-1', type: 'VM', $pool: 'pool-1' }, + 'vm-2': { id: 'vm-2', type: 'VM', $pool: 'pool-1' }, + 'sr-1': { id: 'sr-1', type: 'SR', $pool: 'pool-1' }, +} + +// Minimal XoApp mock: `getObjects` behaves like `Xo#getObjects`, and +// `getObjectFilterForUser` is the only source of truth for visibility +const createMockApp = ({ user, permission, isObjectVisible }) => ({ + apiContext: { user, permission }, + getObjectFilterForUser: async () => isObjectVisible, + getObjects: ({ filter, limit = Infinity } = {}) => { + const results = { __proto__: null } + for (const id of Object.keys(OBJECTS)) { + const object = OBJECTS[id] + if ((filter === undefined || filter(object, id, OBJECTS)) && limit-- > 0) { + results[id] = object + } + } + return results + }, + registerHttpRequest: async (fn, data) => { + // expose what the HTTP handler was registered with so the ndjson path can + // be exercised without an actual HTTP round-trip + registered = { fn, data } + return '/api/token' + }, +}) + +let registered + +const ADMIN = { id: 'admin-1' } +const USER = { id: 'user-1' } + +// =================================================================== + +describe('xo.getAllObjects', function () { + it('returns every object to an admin', async function () { + const app = createMockApp({ user: ADMIN, permission: 'admin', isObjectVisible: ALL_VISIBLE }) + + const result = await handlers.getAllObjects.call(app, {}) + + assert.deepEqual(Object.keys(result).sort(), ['sr-1', 'vm-1', 'vm-2']) + }) + + // the core of the issue: a user with no ACL used to receive the whole + // infrastructure and was expected to filter it client-side + it('returns nothing to a user who may not see any object', async function () { + const app = createMockApp({ user: USER, permission: 'none', isObjectVisible: () => false }) + + const result = await handlers.getAllObjects.call(app, {}) + + assert.deepEqual(Object.keys(result), []) + }) + + it('returns only the objects the user may see', async function () { + const app = createMockApp({ user: USER, permission: 'none', isObjectVisible: id => id === 'vm-1' }) + + const result = await handlers.getAllObjects.call(app, {}) + + assert.deepEqual(Object.keys(result), ['vm-1']) + }) + + it('applies the caller filter on top of the visibility filter', async function () { + const app = createMockApp({ user: USER, permission: 'none', isObjectVisible: id => id === 'vm-1' }) + + const result = await handlers.getAllObjects.call(app, { filter: 'type:VM' }) + + assert.deepEqual(Object.keys(result), ['vm-1']) + }) + + // `limit` must count what the user actually receives, otherwise the number of + // returned objects leaks how many they are not allowed to see + it('counts the limit against the visible objects only', async function () { + const app = createMockApp({ user: USER, permission: 'none', isObjectVisible: id => id === 'sr-1' }) + + const result = await handlers.getAllObjects.call(app, { limit: 1 }) + + assert.deepEqual(Object.keys(result), ['sr-1']) + }) + + describe('ndjson', function () { + it('filters the streamed objects too', async function () { + const app = createMockApp({ user: USER, permission: 'none', isObjectVisible: id => id === 'vm-2' }) + + registered = undefined + const { $getFrom } = await handlers.getAllObjects.call(app, { ndjson: true }) + assert.equal(typeof $getFrom, 'string') + + // a real writable, so the stream pipeline behaves as in production + const chunks = [] + const res = new Writable({ + write(chunk, _encoding, callback) { + chunks.push(String(chunk)) + callback() + }, + }) + res.set = () => {} + + await registered.fn.call(app, {}, res, registered.data) + + const ids = chunks + .join('') + .split('\n') + .filter(line => line !== '') + .map(line => JSON.parse(line).id) + + assert.deepEqual(ids, ['vm-2']) + }) + }) +}) diff --git a/packages/xo-server/src/api/backup-ng.mjs b/packages/xo-server/src/api/backup-ng.mjs index 9184fa7f34..75a4573f44 100644 --- a/packages/xo-server/src/api/backup-ng.mjs +++ b/packages/xo-server/src/api/backup-ng.mjs @@ -105,6 +105,7 @@ createJob.params = { export function getSuggestedExcludedTags() { return ['Continuous Replication', 'Disaster Recovery', this.config.get('xo-proxy.vmTag')] } +getSuggestedExcludedTags.permission = 'admin' export function deleteJob({ id }) { return this.deleteBackupNgJob(id, 'backup') diff --git a/packages/xo-server/src/api/pool.mjs b/packages/xo-server/src/api/pool.mjs index 73f55a00e9..6db7d86901 100644 --- a/packages/xo-server/src/api/pool.mjs +++ b/packages/xo-server/src/api/pool.mjs @@ -469,6 +469,8 @@ export async function listPoolsMatchingCriteria(params) { return this.listPoolsMatchingCriteria(params) } +listPoolsMatchingCriteria.permission = 'admin' + listPoolsMatchingCriteria.params = { minAvailableHostMemory: { type: 'number', optional: true }, minAvailableSrSize: { type: 'number', optional: true }, diff --git a/packages/xo-server/src/api/resource-set.mjs b/packages/xo-server/src/api/resource-set.mjs index 79f45632ea..f18c711878 100644 --- a/packages/xo-server/src/api/resource-set.mjs +++ b/packages/xo-server/src/api/resource-set.mjs @@ -1,3 +1,5 @@ +import { unauthorized } from 'xo-common/api-errors.js' + export function create({ name, shareByDefault, subjects, objects, tags, limits }) { return this.createResourceSet(name, subjects, objects, limits, shareByDefault, tags) } @@ -118,8 +120,20 @@ set.params = { // ------------------------------------------------------------------- -export function get({ id }) { - return this.getResourceSet(id) +export async function get({ id }) { + const resourceSet = await this.getResourceSet(id) + + const { permission, user } = this.apiContext + if (permission !== 'admin') { + const subjects = new Set(user.groups) + subjects.add(user.id) + + if (!resourceSet.subjects.some(subject => subjects.has(subject))) { + throw unauthorized() + } + } + + return resourceSet } get.params = { diff --git a/packages/xo-server/src/api/tag.mjs b/packages/xo-server/src/api/tag.mjs index 607207e48c..6989cd6e85 100644 --- a/packages/xo-server/src/api/tag.mjs +++ b/packages/xo-server/src/api/tag.mjs @@ -46,7 +46,27 @@ set.params = { set.permission = 'admin' export async function getAllConfigured() { - return this.getConfiguredTags() + const configuredTags = await this.getConfiguredTags() + + const { permission, user } = this.apiContext + if (permission === 'admin') { + return configuredTags + } + + const isObjectVisible = await this.getObjectFilterForUser(user.id) + + const visibleTags = new Set() + const objects = this.getObjects() + for (const id in objects) { + const { tags } = objects[id] + if (tags !== undefined && tags.length !== 0 && isObjectVisible(id)) { + for (const tag of tags) { + visibleTags.add(tag) + } + } + } + + return configuredTags.filter(({ id }) => visibleTags.has(id)) } getAllConfigured.description = 'Get all configured tags' diff --git a/packages/xo-server/src/api/xo.mjs b/packages/xo-server/src/api/xo.mjs index 54230f895d..f1e43eeeea 100644 --- a/packages/xo-server/src/api/xo.mjs +++ b/packages/xo-server/src/api/xo.mjs @@ -7,6 +7,8 @@ import { pipeline } from 'readable-stream' import { safeDateFormat } from '../utils.mjs' import createNdJsonStream from '../_createNdJsonStream.mjs' import { getCurrentVmUuid } from '../_XenStore.mjs' +import iteratee from 'lodash/iteratee.js' +import { ALL_VISIBLE } from '../_objectVisibility.mjs' // =================================================================== @@ -59,11 +61,26 @@ function handleGetAllObjects(req, res, { filter, limit }) { return fromCallback(pipeline, createNdJsonStream(objects), res) } -export function getAllObjects({ filter, limit, ndjson = false }) { +function composeFilter(filter, isObjectVisible) { + const predicate = filter === undefined ? undefined : iteratee(filter) + + if (isObjectVisible === ALL_VISIBLE) { + return predicate + } else if (predicate === undefined) { + return (object, id) => isObjectVisible(id) + } else { + return (object, id, objects) => isObjectVisible(id) && predicate(object, id, objects) + } +} + +export async function getAllObjects({ filter, limit, ndjson = false }) { if (typeof filter === 'string') { filter = CM.parse(filter).createPredicate() } + const isObjectVisible = await this.getObjectFilterForUser(this.apiContext.user.id) + filter = composeFilter(filter, isObjectVisible) + return ndjson ? this.registerHttpRequest(handleGetAllObjects, { filter, diff --git a/packages/xo-server/src/xo-mixins/_object-visibility.test.mjs b/packages/xo-server/src/xo-mixins/_object-visibility.test.mjs new file mode 100644 index 0000000000..22fccbb23c --- /dev/null +++ b/packages/xo-server/src/xo-mixins/_object-visibility.test.mjs @@ -0,0 +1,407 @@ +import assert from 'node:assert/strict' +import { afterEach, beforeEach, describe, it, mock } from 'node:test' + +import ObjectVisibility from './object-visibility.mjs' + +// =================================================================== + +const OBJECTS = { + 'pool-1': { id: 'pool-1', type: 'pool' }, + 'vm-1': { id: 'vm-1', type: 'VM', $container: 'pool-1', $pool: 'pool-1' }, + 'vm-2': { id: 'vm-2', type: 'VM', $container: 'pool-1', $pool: 'pool-1' }, + 'tpl-1': { id: 'tpl-1', type: 'VM-template', $pool: 'pool-1' }, +} + +// Minimal XoApp mock, counting the calls the cache is meant to avoid +// A connection as `Api#createApiConnection` hands it over, recording what it +// was notified of +const createMockConnection = userId => { + const notified = [] + return { + notified, + get: (key, defaultValue) => (key === 'user_id' ? userId : defaultValue), + notify: (method, params) => notified.push({ method, params }), + } +} + +const createMockApp = ({ users, permissionsByUser = {}, resourceSetsByUser = {}, connections = [] } = {}) => { + const calls = { getUser: 0, getPermissionsForUser: 0, getAllResourceSets: 0 } + + return { + calls, + apiConnections: connections, + objects: { all: OBJECTS }, + getObject: id => OBJECTS[id], + getUser: async id => { + calls.getUser++ + const user = users[id] + if (user === undefined) { + throw new Error(`no such user ${id}`) + } + return user + }, + getPermissionsForUser: async id => { + calls.getPermissionsForUser++ + return permissionsByUser[id] ?? {} + }, + getAllResourceSets: async id => { + calls.getAllResourceSets++ + return resourceSetsByUser[id] ?? [] + }, + } +} + +const clearAndFlush = (mixin, userId) => { + const promise = mixin.clearObjectFilterCache(userId) + mock.timers.tick(1000) + return promise +} + +const ADMIN = { id: 'admin-1', permission: 'admin' } +const USER = { id: 'user-1', permission: 'none' } +const OTHER_USER = { id: 'user-2', permission: 'none' } + +const USERS = { 'admin-1': ADMIN, 'user-1': USER, 'user-2': OTHER_USER } + +// =================================================================== + +describe('ObjectVisibility', function () { + beforeEach(function () { + mock.timers.enable({ apis: ['setTimeout'] }) + }) + + afterEach(function () { + mock.timers.reset() + }) + + describe('#getObjectFilterForUser()', function () { + it('gives an admin a predicate that accepts everything', async function () { + const app = createMockApp({ users: USERS }) + const mixin = new ObjectVisibility(app) + + const isVisible = await mixin.getObjectFilterForUser('admin-1') + + assert.equal(isVisible('vm-1'), true) + }) + + it('never reads the ACLs of an admin', async function () { + const app = createMockApp({ users: USERS }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('admin-1') + + assert.equal(app.calls.getPermissionsForUser, 0) + }) + + it('returns a predicate restricted to the ACLs of a non-admin', async function () { + const app = createMockApp({ + users: USERS, + permissionsByUser: { 'user-1': { 'vm-1': { view: 1 } } }, + }) + const mixin = new ObjectVisibility(app) + + const isVisible = await mixin.getObjectFilterForUser('user-1') + + assert.equal(isVisible('vm-1'), true) + assert.equal(isVisible('vm-2'), false) + }) + + it('includes the objects of the resource sets the user is a subject of', async function () { + const app = createMockApp({ + users: USERS, + resourceSetsByUser: { 'user-1': [{ id: 'rs-1', objects: ['tpl-1'] }] }, + }) + const mixin = new ObjectVisibility(app) + + const isVisible = await mixin.getObjectFilterForUser('user-1') + + assert.equal(isVisible('tpl-1'), true) + assert.equal(isVisible('vm-1'), false) + }) + }) + + // the filter is rebuilt on every object batch; reading Redis each time would + // put the ACL store on the hot path of the event loop + describe('caching', function () { + it('reads the ACLs only once across several calls', async function () { + const app = createMockApp({ users: USERS }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + await mixin.getObjectFilterForUser('user-1') + + assert.equal(app.calls.getPermissionsForUser, 1) + assert.equal(app.calls.getAllResourceSets, 1) + }) + + it('caches per user', async function () { + const app = createMockApp({ users: USERS }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + await mixin.getObjectFilterForUser('user-2') + + assert.equal(app.calls.getPermissionsForUser, 2) + }) + + it('recomputes after the cache has been cleared', async function () { + const app = createMockApp({ users: USERS }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + await clearAndFlush(mixin) + await mixin.getObjectFilterForUser('user-1') + + assert.equal(app.calls.getPermissionsForUser, 2) + }) + + it('picks up an ACL granted after the first call', async function () { + const permissionsByUser = { 'user-1': {} } + const app = createMockApp({ users: USERS, permissionsByUser }) + const mixin = new ObjectVisibility(app) + + assert.equal((await mixin.getObjectFilterForUser('user-1'))('vm-1'), false) + + permissionsByUser['user-1'] = { 'vm-1': { view: 1 } } + await clearAndFlush(mixin) + + assert.equal((await mixin.getObjectFilterForUser('user-1'))('vm-1'), true) + }) + + it('clears a single user without dropping the others', async function () { + const app = createMockApp({ users: USERS }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + await mixin.getObjectFilterForUser('user-2') + await clearAndFlush(mixin, 'user-1') + await mixin.getObjectFilterForUser('user-1') + await mixin.getObjectFilterForUser('user-2') + + assert.equal(app.calls.getPermissionsForUser, 3) + }) + }) + + // The event stream only carries changes, so a client whose permissions grew + // would never hear about objects that already existed. + describe('resync on a permission change', () => { + it('pushes the newly visible objects to a non-admin', async () => { + const permissionsByUser = { 'user-1': {} } + const connection = createMockConnection('user-1') + const app = createMockApp({ users: USERS, permissionsByUser, connections: [connection] }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + + permissionsByUser['user-1'] = { 'vm-1': { view: 1 } } + await clearAndFlush(mixin, 'user-1') + + assert.equal(connection.notified.length, 1) + assert.deepEqual(Object.keys(connection.notified[0].params.items), ['vm-1']) + }) + + // A client promoted to admin stops filtering locally, so it must be given + // everything it was previously denied — otherwise it shows a near-empty XO + // until the page is reloaded. + it('pushes the whole collection to a user who has just become an admin', async () => { + const users = { 'user-1': { id: 'user-1', permission: 'none' } } + const connection = createMockConnection('user-1') + const app = createMockApp({ users, connections: [connection] }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + + users['user-1'] = { id: 'user-1', permission: 'admin' } + await clearAndFlush(mixin, 'user-1') + + assert.equal(connection.notified.length, 1) + assert.deepEqual(Object.keys(connection.notified[0].params.items).sort(), Object.keys(OBJECTS).sort()) + }) + + // With an external auth provider the user record is written on every + // sign-in: an admin who was never filtered must not be sent the whole + // collection each time they log in. + it('pushes nothing to an admin who was never filtered', async () => { + const connection = createMockConnection('admin-1') + const app = createMockApp({ users: USERS, connections: [connection] }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('admin-1') + await clearAndFlush(mixin, 'admin-1') + + assert.deepEqual(connection.notified, []) + }) + + it('leaves the connections of other users alone', async () => { + const connection = createMockConnection('user-2') + const app = createMockApp({ + users: USERS, + permissionsByUser: { 'user-2': { 'vm-1': { view: 1 } } }, + connections: [connection], + }) + const mixin = new ObjectVisibility(app) + + await clearAndFlush(mixin, 'user-1') + + assert.deepEqual(connection.notified, []) + }) + }) + + // An ACL or resource set change invalidates everyone, but concerns almost + // nobody: without this, one grant rebuilds and re-sends the whole visible + // set of every connected client. + describe('resync of users whose permissions did not move', function () { + it('pushes nothing to a user whose permissions are unchanged', async function () { + const connection = createMockConnection('user-1') + const app = createMockApp({ + users: USERS, + permissionsByUser: { 'user-1': { 'vm-1': { view: 1 } } }, + connections: [connection], + }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + await clearAndFlush(mixin) + + assert.deepEqual(connection.notified, []) + }) + + it('still pushes to the user whose permissions did move', async function () { + const permissionsByUser = { 'user-1': {} } + const connection = createMockConnection('user-1') + const app = createMockApp({ users: USERS, permissionsByUser, connections: [connection] }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + + permissionsByUser['user-1'] = { 'vm-1': { view: 1 } } + await clearAndFlush(mixin) + + assert.equal(connection.notified.length, 1) + }) + + it('pushes when nothing is known about the previous state', async function () { + const connection = createMockConnection('user-1') + const app = createMockApp({ + users: USERS, + permissionsByUser: { 'user-1': { 'vm-1': { view: 1 } } }, + connections: [connection], + }) + const mixin = new ObjectVisibility(app) + + // never looked at before: withholding would be the riskier guess + await clearAndFlush(mixin) + + assert.equal(connection.notified.length, 1) + }) + + it('ignores the order resource set objects come back in', async function () { + const resourceSetsByUser = { 'user-1': [{ id: 'rs-1', objects: ['tpl-1', 'vm-1'] }] } + const connection = createMockConnection('user-1') + const app = createMockApp({ users: USERS, resourceSetsByUser, connections: [connection] }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + + resourceSetsByUser['user-1'] = [{ id: 'rs-1', objects: ['vm-1', 'tpl-1'] }] + await clearAndFlush(mixin) + + assert.deepEqual(connection.notified, []) + }) + }) + + // ACLs are written in loops: creating a VM in a resource set grants one per + // subject, and each write would otherwise resync on its own. + describe('debouncing', function () { + it('collapses a burst of invalidations into a single resync', async function () { + const permissionsByUser = { 'user-1': {} } + const connection = createMockConnection('user-1') + const app = createMockApp({ users: USERS, permissionsByUser, connections: [connection] }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + + permissionsByUser['user-1'] = { 'vm-1': { view: 1 } } + const resyncs = [mixin.clearObjectFilterCache(), mixin.clearObjectFilterCache(), mixin.clearObjectFilterCache()] + mock.timers.tick(1000) + await Promise.all(resyncs) + + assert.equal(connection.notified.length, 1) + }) + + // the comparison must be made against the state before the burst started, + // not against the state left by its first invalidation + it('compares against the state from before the burst', async function () { + const permissionsByUser = { 'user-1': { 'vm-1': { view: 1 } } } + const connection = createMockConnection('user-1') + const app = createMockApp({ users: USERS, permissionsByUser, connections: [connection] }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + + const first = mixin.clearObjectFilterCache() + permissionsByUser['user-1'] = { 'vm-2': { view: 1 } } + await mixin.getObjectFilterForUser('user-1') + const second = mixin.clearObjectFilterCache() + mock.timers.tick(1000) + await Promise.all([first, second]) + + assert.equal(connection.notified.length, 1) + }) + }) + + describe('robustness', function () { + // `getUser` is uncached and hits Redis: on the notification path that + // would put a round trip between every batch and its clients + it('reads the user record once across several calls', async function () { + const app = createMockApp({ users: USERS }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + await mixin.getObjectFilterForUser('user-1') + await mixin.getObjectFilterForUser('user-1') + + assert.equal(app.calls.getUser, 1) + }) + + it('reads the user record again once the cache is cleared', async function () { + const app = createMockApp({ users: USERS }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-1') + await clearAndFlush(mixin, 'user-1') + await mixin.getObjectFilterForUser('user-1') + + assert.equal(app.calls.getUser, 2) + }) + + it('does not read the ACLs of an admin, even once', async function () { + const app = createMockApp({ users: USERS }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('admin-1') + await mixin.getObjectFilterForUser('admin-1') + + assert.equal(app.calls.getUser, 1) + assert.equal(app.calls.getPermissionsForUser, 0) + }) + + // one unreadable user — a deleted account still holding a socket — must + // not stop every other connection from being resynced + it('resyncs the other connections when one user cannot be read', async function () { + const permissionsByUser = { 'user-2': {} } + const broken = createMockConnection('ghost') + const healthy = createMockConnection('user-2') + const app = createMockApp({ users: USERS, permissionsByUser, connections: [broken, healthy] }) + const mixin = new ObjectVisibility(app) + + await mixin.getObjectFilterForUser('user-2') + + permissionsByUser['user-2'] = { 'vm-1': { view: 1 } } + await clearAndFlush(mixin) + + assert.deepEqual(broken.notified, []) + assert.equal(healthy.notified.length, 1, 'the healthy connection was skipped') + }) + }) +}) diff --git a/packages/xo-server/src/xo-mixins/acls.mjs b/packages/xo-server/src/xo-mixins/acls.mjs index 4484885c5c..489bc7ccfc 100644 --- a/packages/xo-server/src/xo-mixins/acls.mjs +++ b/packages/xo-server/src/xo-mixins/acls.mjs @@ -5,9 +5,13 @@ import map from 'lodash/map.js' import { ModelAlreadyExists } from '../collection.mjs' import { Acls } from '../models/acl.mjs' +import { EventListenersManager } from '@vates/event-listeners-manager' +import { createLogger } from '@xen-orchestra/log' // =================================================================== +const log = createLogger('xo:acls') + export default class { constructor(app) { this._app = app @@ -20,6 +24,16 @@ export default class { crypto: app.cryptoCredentials, })) + const onAclsChange = () => + app.clearObjectFilterCache().catch(error => log.warn('failed to invalidate the object filter cache', { error })) + + const events = new EventListenersManager(aclsDb) + for (const event of ['add', 'update', 'remove']) { + events.add(event, onAclsChange) + } + + app.hooks.on('stop', () => events.removeAll()) + app.addConfigManager( 'acls', () => aclsDb.get(), diff --git a/packages/xo-server/src/xo-mixins/object-visibility.mjs b/packages/xo-server/src/xo-mixins/object-visibility.mjs new file mode 100644 index 0000000000..63e5047e88 --- /dev/null +++ b/packages/xo-server/src/xo-mixins/object-visibility.mjs @@ -0,0 +1,321 @@ +import { createPermissions, createIsObjectVisible, ALL_VISIBLE } from '../_objectVisibility.mjs' +import TTLCache from '@isaacs/ttlcache' +import isEqual from 'lodash/isEqual.js' +import { createLogger } from '@xen-orchestra/log' + +const log = createLogger('xo:object-visibility') + +/** + * @typedef {{ [objectId: string]: { [permission: string]: 1 } }} PermissionsByObject + * + * @typedef {{ permission: 'admin' }} AdminSources + * + * @typedef {{ + * permission: string, + * permissionsByObject: PermissionsByObject, + * resourceSetObjectIds: string[], + * permissions: PermissionsByObject, + * }} UserSources + * + * An admin is resolved from their permission alone: the ACLs and resource sets + * are never read, so the rest of the fields are simply absent. + * + * @typedef {AdminSources | UserSources} Sources + * + * @typedef {{ + * timer: ReturnType, + * previousSources: Map>, + * promise: Promise, + * resolve: () => void, + * }} PendingResync + */ + +/** + * A missing side means there is nothing to compare against, so the caller must + * treat it as changed and push rather than risk withholding. + * + * @param {Sources} [sourceA] + * @param {Sources} [sourceB] + * @returns {boolean} + */ +function sourcesAreEqual(sourceA, sourceB) { + if (sourceA === undefined || sourceB === undefined) { + return false + } + + return ( + isEqual(sourceA.permissionsByObject, sourceB.permissionsByObject) && + isEqual(sourceA.resourceSetObjectIds, sourceB.resourceSetObjectIds) + ) +} + +// ACLs are commonly written in loops — creating a VM in a resource set grants +// one per subject — and each write would otherwise resync every connected +// client. Pushes are collapsed over this window; the cache itself is always +// dropped immediately, so no stale permission is ever used. +const DELAY = 500 + +// key standing for "every user", since a blanket invalidation carries no id +const ALL_USERS = Symbol('all users') + +export default class ObjectVisibility { + constructor(app) { + this._app = app + app.hooks?.on('stop', () => { + for (const { timer, resolve } of this.#pendingResyncs.values()) { + clearTimeout(timer) + resolve() + } + + this.#pendingResyncs.clear() + }) + } + + /** + * The TTL is a safety net: should an invalidation path ever be missed, a + * stale filter heals itself instead of lasting until the next restart. + * + * @type {TTLCache>} + */ + #sourcesByUserId = new TTLCache({ ttl: 5 * 60 * 1e3 }) + + /** + * Resyncs waiting for the debounce window to close, by user id (or + * `ALL_USERS`). + * + * @type {Map} + */ + #pendingResyncs = new Map() + + /** + * Returns the predicate deciding which objects a user may see. An admin + * gets one that accepts everything. + * + * @param {string} userId + * @returns {Promise} + */ + async getObjectFilterForUser(userId) { + const { permission, permissions } = await this.#getSources(userId) + + if (permission === 'admin') { + return ALL_VISIBLE + } else { + return createIsObjectVisible({ + getObject: objectId => { + try { + return this._app.getObject(objectId) + } catch { + return undefined + } + }, + permissionsByObject: permissions, + }) + } + } + + /** + * Drops the cached permissions of a user, or of every user when called + * without argument, and pushes the objects that just became visible to the + * connections concerned. + * + * The cache is dropped straight away: a revoked permission must never be + * used again, whatever the push does afterwards. What the previous state was + * is kept so the resync can tell whose permissions actually moved. + * + * @param {string} [userId] + * @returns {Promise} resolves once the connections have been resynced or when the server is stopped; + * callers may ignore it, it never rejects + */ + async clearObjectFilterCache(userId = undefined) { + const previousSources = new Map() + + const storeSourceInPrevious = userId => { + const sources = this.#sourcesByUserId.get(userId) + + if (sources !== undefined) { + previousSources.set(userId, sources) + } + } + + if (userId !== undefined) { + storeSourceInPrevious(userId) + this.#sourcesByUserId.delete(userId) + } else { + for (const userId of this.#sourcesByUserId.keys()) { + storeSourceInPrevious(userId) + } + this.#sourcesByUserId.clear() + } + + return this.#scheduleResync(userId, previousSources) + } + + /** + * The permission is read first and cached with the rest: this runs on the + * notification path, and `getUser` is uncached, so an uncached lookup would + * put Redis between every batch and its clients. + * + * @param {string} userId + * @returns {Promise} + */ + async #computeSourcesForUser(userId) { + const { permission } = await this._app.getUser(userId) + + if (permission === 'admin') { + return { permission } + } else { + const [permissionsByObject, resourceSets] = await Promise.all([ + this._app.getPermissionsForUser(userId), + this._app.getAllResourceSets(userId), + ]) + const resourceSetObjectIds = resourceSets.flatMap(resourceSet => resourceSet.objects).sort() + const permissions = createPermissions({ permissionsByObject, resourceSetObjectIds }) + + return { permission, permissionsByObject, resourceSetObjectIds, permissions } + } + } + + /** + * The promise is cached rather than its result, so two callers arriving + * together share one computation instead of both hitting Redis. A rejection + * is never kept: a transient failure must not be served for the whole TTL. + * + * @param {string} userId + * @returns {Promise} + */ + async #getSources(userId) { + if (this.#sourcesByUserId.has(userId)) { + return this.#sourcesByUserId.get(userId) + } else { + try { + const sources = this.#computeSourcesForUser(userId) + this.#sourcesByUserId.set(userId, sources) + + return await sources + } catch (error) { + this.#sourcesByUserId.delete(userId) + + throw error + } + } + } + + /** + * Defers the push, collapsing a burst of invalidations into a single resync. + * + * When a burst is merged, the earliest snapshot wins: the comparison must be + * made against the state from before the burst began, not against the one + * its own first invalidation left behind. + * + * @param {string | undefined} userId + * @param {Map>} previousSources + * @returns {Promise} + */ + #scheduleResync(userId, previousSources) { + const key = userId ?? ALL_USERS + let entry = this.#pendingResyncs.get(key) + + if (entry === undefined) { + let resolveResync + const promise = new Promise(resolve => { + resolveResync = resolve + }) + + entry = { previousSources, promise, resolve: resolveResync } + this.#pendingResyncs.set(key, entry) + } else { + clearTimeout(entry.timer) + + for (const [id, sources] of previousSources) { + if (!entry.previousSources.has(id)) { + entry.previousSources.set(id, sources) + } + } + } + + const pending = entry + pending.timer = setTimeout(() => { + this.#pendingResyncs.delete(key) + this.#resyncObjects(userId, pending.previousSources) + .catch(error => log.warn('failed to resync objects', { error, userId })) + .then(pending.resolve) + }, DELAY) + + return pending.promise + } + + /** + * Each connection is isolated: one unreadable user — a deleted account still + * holding a socket, a Redis hiccup — must not stop the others from being + * resynced. + * + * @param {string | undefined} userId - when given, only that user's connections + * @param {Map>} previousSources + */ + async #resyncObjects(userId, previousSources) { + for (const connection of this._app.apiConnections ?? []) { + const connectionUserId = connection.get('user_id', undefined) + + if ( + connectionUserId === undefined || + connection.notify === undefined || + (userId !== undefined && connectionUserId !== userId) + ) { + continue + } + + try { + await this.#resyncConnection(connection, connectionUserId, previousSources) + } catch (error) { + log.warn('failed to resync a connection', { error, userId: connectionUserId }) + } + } + } + + /** + * The event stream only carries changes, so a user granted access to an + * object that already exists would not hear about it until they reload: + * their whole visible set is pushed instead, and their client merges it. + * + * The opposite case needs nothing: xo-web keeps filtering client-side and + * refreshes its permissions every 5s. + * + * @param {import('../connection.mjs').default} connection + * @param {string} userId + * @param {Map>} previousSources + */ + async #resyncConnection(connection, userId, previousSources) { + const current = await this.#getSources(userId) + const previous = await previousSources.get(userId)?.catch(() => undefined) + + // A client promoted to admin stops filtering locally, so it must be given + // everything it was previously denied — otherwise it shows a near-empty XO + // until the page is reloaded. An admin we have never filtered already has + // everything and needs nothing. + const promoted = current.permission === 'admin' && previous !== undefined && previous.permission !== 'admin' + if (current.permission === 'admin' && !promoted) { + return + } + + // An ACL or resource set change invalidates every user, but concerns + // almost none of them: without this, granting one ACL rebuilds and + // re-sends the whole visible set of every connected client. + if (!promoted && sourcesAreEqual(previous, current)) return + + const objects = this._app.objects.all + const items = { __proto__: null } + let isEmpty = true + + const isObjectVisible = await this.getObjectFilterForUser(userId) + for (const id in objects) { + if (isObjectVisible(id)) { + items[id] = objects[id] + isEmpty = false + } + } + + if (!isEmpty) { + connection.notify('all', { type: 'enter', items }) + } + } +} diff --git a/packages/xo-server/src/xo-mixins/resource-sets.mjs b/packages/xo-server/src/xo-mixins/resource-sets.mjs index 0b55f4dcb4..19b1403443 100644 --- a/packages/xo-server/src/xo-mixins/resource-sets.mjs +++ b/packages/xo-server/src/xo-mixins/resource-sets.mjs @@ -69,6 +69,10 @@ const normalize = set => ({ tags: set.tags || [], }) +const sameMembers = (a = [], b = []) => { + return a.length === b.length && difference(a, b).length === 0 +} + // =================================================================== export default class { @@ -96,8 +100,44 @@ export default class { return id } - _save(set) { - return this._store.put(set.id, set) + async _save(set) { + let previous + try { + previous = await this._store.get(set.id) + } catch (error) { + // expected when the set is being created + } + + await this._store.put(set.id, set) + + const previousSubjects = previous?.subjects ?? [] + const previousObjects = previous?.objects ?? [] + if (sameMembers(previousSubjects, set.subjects) && sameMembers(previousObjects, set.objects)) return + + await this.#clearCacheForSubjects([...previousSubjects, ...set.subjects]) + } + + async #clearCacheForSubjects(subjectIds) { + const usersByGroupId = new Map() + for (const group of await this._app.getAllGroups()) { + usersByGroupId.set(group.id, group.users) + } + + const userIds = new Set() + for (const subjectId of subjectIds) { + const groupUserIds = usersByGroupId.get(subjectId) + if (groupUserIds !== undefined) { + for (const userId of groupUserIds) { + userIds.add(userId) + } + } else { + userIds.add(subjectId) + } + } + + for (const userId of userIds) { + this._app.clearObjectFilterCache(userId) + } } async checkResourceSetConstraints(id, userId, objectIds) { @@ -154,7 +194,7 @@ export default class { shareByDefault, }) - await this._store.put(id, set) + await this._save(set) return set } @@ -168,7 +208,20 @@ export default class { this.setVmResourceSet(vm.id, null, true) ) ) - return store.del(id) + + // read the subjects before the set is gone: they are whose cache must be + // dropped + let previous + try { + previous = await this._store.get(id) + } catch (error) { + // `has` already said it was there, so this should not happen + } + + await store.del(id) + await this.#clearCacheForSubjects(previous?.subjects ?? []) + + return } throw noSuchObject(id, 'resourceSet') diff --git a/packages/xo-server/src/xo-mixins/subjects.mjs b/packages/xo-server/src/xo-mixins/subjects.mjs index 52ef24bd7b..9eb5a7fd66 100644 --- a/packages/xo-server/src/xo-mixins/subjects.mjs +++ b/packages/xo-server/src/xo-mixins/subjects.mjs @@ -22,6 +22,8 @@ const log = createLogger('xo:xo-mixins:subjects') const addToArraySet = (set, value) => (set !== undefined ? (set.includes(value) ? set : set.concat(value)) : [value]) const removeFromArraySet = (set, value) => set && filter(set, current => current !== value) +const onCacheInvalidationError = error => log.warn('failed to invalidate the object filter cache', { error }) + // =================================================================== export default class { @@ -38,6 +40,18 @@ export default class { indexes: ['name'], crypto: app.cryptoCredentials, })) + + for (const event of ['add', 'update']) { + groupsDb.on(event, groups => { + for (const group of groups) { + for (const userId of group.users ?? []) { + app.clearObjectFilterCache(userId).catch(onCacheInvalidationError) + } + } + }) + } + groupsDb.on('remove', () => app.clearObjectFilterCache().catch(onCacheInvalidationError)) + app.hooks.emit('registerCollection', { collection: groupsDb, type: 'group', @@ -49,6 +63,20 @@ export default class { indexes: UNIQUE_FIELDS, crypto: app.cryptoCredentials, })) + + for (const event of ['add', 'update']) { + usersDb.on(event, models => { + for (const model of models) { + app.clearObjectFilterCache(model.id).catch(onCacheInvalidationError) + } + }) + } + usersDb.on('remove', userIds => { + for (const userId of userIds) { + app.clearObjectFilterCache(userId).catch(onCacheInvalidationError) + } + }) + app.hooks.emit('registerCollection', { collection: usersDb, type: 'user', diff --git a/packages/xo-server/src/xo.mjs b/packages/xo-server/src/xo.mjs index 31f7fd8c61..49a6aa68fc 100644 --- a/packages/xo-server/src/xo.mjs +++ b/packages/xo-server/src/xo.mjs @@ -22,6 +22,8 @@ import { noSuchObject } from 'xo-common/api-errors.js' import { parseDuration } from '@vates/parse-duration' import { pipeline } from 'node:stream' import { UniqueIndex as XoUniqueIndex } from 'xo-collection/unique-index.js' +import { computeObjectNotifications } from './_objectNotifications.mjs' +import { asyncEach } from '@vates/async-each' import mixins from './xo-mixins/index.mjs' import { generateToken, noop } from './utils.mjs' @@ -416,32 +418,77 @@ export default class Xo extends EventEmitter { }) objects.on('finish', () => { - const enteredMessage = !isEmpty(entered) && { - type: 'enter', - items: entered, - } - const exitedMessage = !isEmpty(exited) && { - type: 'exit', - items: exited, - } + const enteredBatch = entered + const exitedBatch = exited - if (!enteredMessage && !exitedMessage) { + reset() + + if (isEmpty(enteredBatch) && isEmpty(exitedBatch)) { return } - for (const connection of this.apiConnections) { - // Notifies only authenticated clients. - if (connection.has('user_id') && connection.notify) { - if (enteredMessage) { - connection.notify('all', enteredMessage) - } - if (exitedMessage) { - connection.notify('all', exitedMessage) - } - } - } - - reset() + this.#notifyObjectChanges(enteredBatch, exitedBatch).catch(error => + log.warn('failed to notify object changes', { error }) + ) }) } + + async #notifyObjectChanges(entered, exited) { + const connections = [] + for (const apiConnection of this.apiConnections) { + if (apiConnection.has('user_id') && apiConnection.notify !== undefined) { + connections.push(apiConnection) + } + } + + if (connections.length === 0) { + return + } + + const connectionsByUserId = new Map() + for (const connection of connections) { + const connectionUserId = connection.get('user_id') + + const userConnections = connectionsByUserId.get(connectionUserId) + if (userConnections === undefined) { + connectionsByUserId.set(connectionUserId, [connection]) + } else { + userConnections.push(connection) + } + } + + await asyncEach( + connectionsByUserId, + async ([userId, userConnections]) => { + let objectFilter + try { + objectFilter = await this.getObjectFilterForUser(userId) + } catch (error) { + log.warn('failed to compute the object filter of a user', { error, userId }) + } + + if (objectFilter) { + const { enter, exit } = computeObjectNotifications({ + entered, + exited, + isObjectVisible: objectFilter, + }) + + for (const userConnection of userConnections) { + if (enter !== undefined) { + userConnection.notify('all', { type: 'enter', items: enter }) + } + + if (exit !== undefined) { + userConnection.notify('all', { type: 'exit', items: exit }) + } + } + } + }, + { + concurrency: 10, + stopOnError: false, + } + ) + } } diff --git a/packages/xo-web/src/common/utils.js b/packages/xo-web/src/common/utils.js index 8aecf50193..640a518255 100644 --- a/packages/xo-web/src/common/utils.js +++ b/packages/xo-web/src/common/utils.js @@ -233,20 +233,23 @@ export const formatLogs = logs => // value can be: float, Infinity, -Infinity and NaN const matches = /^value:\s*(Infinity|NaN|-Infinity|[0-9.]+)\s+config:\s*([^]*)$/.exec(body) if (matches === null) { - return + return { id } } const [, value, xml] = matches - return fromCallback(xml2js.parseString, xml).then((result = {}) => { - const object = mapValues(result.variable, value => get(value, '[0].$.value')) - if (object.name === undefined) { - return - } + return fromCallback(xml2js.parseString, xml).then( + (result = {}) => { + const object = mapValues(result.variable, value => get(value, '[0].$.value')) + if (object.name === undefined) { + return { id } + } - const { name, ...alarmAttributes } = object + const { name, ...alarmAttributes } = object - return { name, value, alarmAttributes, id } - }, noop) + return { name, value, alarmAttributes, id } + }, + () => ({ id }) + ) }) ) diff --git a/packages/xo-web/src/xo-app/host/tab-logs.js b/packages/xo-web/src/xo-app/host/tab-logs.js index 2a9c948013..39f880819d 100644 --- a/packages/xo-web/src/xo-app/host/tab-logs.js +++ b/packages/xo-web/src/xo-app/host/tab-logs.js @@ -104,7 +104,7 @@ export default class TabLogs extends Component { formatLogs(logs).then(formattedLogs => { this.setState({ logs: map(formattedLogs, ({ id, ...formattedLogs }) => ({ - formatted: formattedLogs, + formatted: isEmpty(formattedLogs) ? undefined : formattedLogs, ...logs[id], })), }) diff --git a/packages/xo-web/src/xo-app/new-vm/index.js b/packages/xo-web/src/xo-app/new-vm/index.js index b54dc5ec2a..5f64717fca 100644 --- a/packages/xo-web/src/xo-app/new-vm/index.js +++ b/packages/xo-web/src/xo-app/new-vm/index.js @@ -131,7 +131,6 @@ const Item = ({ label, children, className }) => ( ) @addSubscriptions({ - // eslint-disable-next-line standard/no-callback-literal ipPoolsConfigured: cb => subscribeIpPools(ipPools => cb(ipPools.length > 0)), }) @injectIntl @@ -302,6 +301,18 @@ export default class NewVm extends BaseComponent { const template = this.props.template if (get(() => prevProps.template.id) !== get(() => template.id)) { this._initTemplate(template) + } else if ( + prevProps.resourceSets === undefined && + this.props.resourceSets !== undefined && + template !== undefined + ) { + // the template may have been skipped by `_initTemplate` because the + // resource sets were not fetched yet + // + // deferred until the pending updates are applied: the `share` update + // below is batched with this one and, as `_setState` spreads the current + // state, it would otherwise discard the values set by `_initTemplate` + this.setState(null, () => this._initTemplate(this.props.template)) } // as an admin, the resource set is selected in the advanced settings and @@ -575,6 +586,12 @@ export default class NewVm extends BaseComponent { return this._reset() } + // self-service: the template can be known (e.g. deep link) before the + // resource sets are fetched, `componentDidUpdate` will retry once they are + if (this.props.pool === undefined && this._getResolvedResourceSet() === undefined) { + return + } + const storeState = store.getState() const isInResourceSet = this._getIsInResourceSet() const { state } = this.state @@ -878,19 +895,20 @@ export default class NewVm extends BaseComponent { return } - const defaultSr = getObject(store.getState(), template.$pool, true).default_SR - - return includes( - resolveIds( - filter( - this._getResolvedResourceSet().objectsByType.SR, - sr => sr.$pool === template.$pool && sr.content_type !== 'iso' && sr.size > 0 - ) - ), - defaultSr + const usableSrs = resolveIds( + filter( + this._getResolvedResourceSet().objectsByType.SR, + sr => sr.$pool === template.$pool && sr.content_type !== 'iso' && sr.size > 0 + ) ) - ? defaultSr - : undefined + + // the pool is not necessarily visible: a resource set grants its objects, + // never their container + const defaultSr = getObject(store.getState(), template.$pool, true)?.default_SR + + // fall back to any SR of the set, otherwise the disk rows come up with + // nothing selected + return includes(usableSrs, defaultSr) ? defaultSr : usableSrs[0] } _addVdi = () => { const { state } = this.state diff --git a/packages/xo-web/src/xo-app/vm/tab-general.js b/packages/xo-web/src/xo-app/vm/tab-general.js index e895ffc275..1377763f8f 100644 --- a/packages/xo-web/src/xo-app/vm/tab-general.js +++ b/packages/xo-web/src/xo-app/vm/tab-general.js @@ -32,6 +32,7 @@ import { connectStore, formatSizeShort, getVirtualizationModeLabel, + noop, osFamily, NumericDate, } from 'utils' @@ -158,7 +159,8 @@ const GeneralTab = decorate([ addSubscriptions(({ isAdmin, vm }) => ({ vmCreator: isAdmin ? cb => subscribeUsers(users => cb(find(users, user => user.id === vm.creation?.user))) - : () => {}, + : // a subscription must return its unsubscribe function + () => noop, vmSecurebootReadiness: subscribeSecurebootReadiness(vm), })), provideState({