feat(events): improve xo5 event handling (#10568)

XO-3152

Improves XO5 event handling

Event improvements - Front end test.md

Event improvements - Performance impact.md

@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",

@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, {}))
+      })
+    }
+  })
+})

@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<any>,
+ *   on: (event: string, listener: (...args: any[]) => void) => void,
+ *   removeListener: (event: string, listener: (...args: any[]) => void) => void,
+ *   close: () => Promise<void>,
+ * }} 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<XoClient>} 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<string, object>, exited: Set<string>, 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<boolean>} 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 })
+      }
+    }
+  }
+}

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

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
+  }
+}

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 }
+}

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 })
+  })
+})

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
+}

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)
+    })
+  })
+})

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'])
+    })
+  })
+})

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')

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 },

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 = {

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'

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,

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')
+    })
+  })
+})

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(),

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<typeof setTimeout>,
+ *   previousSources: Map<string, Promise<Sources>>,
+ *   promise: Promise<void>,
+ *   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<string, Promise<Sources>>}
+   */
+  #sourcesByUserId = new TTLCache({ ttl: 5 * 60 * 1e3 })
+
+  /**
+   * Resyncs waiting for the debounce window to close, by user id (or
+   * `ALL_USERS`).
+   *
+   * @type {Map<string | symbol, PendingResync>}
+   */
+  #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<import('../_objectVisibility.mjs').IsObjectVisible>}
+   */
+  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<void>} 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<Sources>}
+   */
+  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<Sources>}
+   */
+  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<string, Promise<Sources>>} previousSources
+   * @returns {Promise<void>}
+   */
+  #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<string, Promise<Sources>>} 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<string, Promise<Sources>>} 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 })
+    }
+  }
+}

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')

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',

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,
+      }
+    )
+  }
 }

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 })
+      )
     })
   )
 

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],
         })),
       })

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

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({