diff --git a/nodejs/models/resource.js b/nodejs/models/resource.js index ab1752a..fcca1cb 100644 --- a/nodejs/models/resource.js +++ b/nodejs/models/resource.js @@ -1,3 +1,4 @@ +const crypto = require('crypto'); const { Model } = require('@simpleworkjs/orm'); const { Group } = require('./group_ldap'); @@ -228,6 +229,18 @@ class ResourceGroup extends Model { groupCn: { type: 'string', isRequired: true }, accessLevel: { type: 'string', isRequired: true } }; + + // No DB-level unique constraint on (resourceId, groupCn) exists, so callers + // MUST check-then-create rather than relying on a constraint violation to + // catch a dupe. A caller that skips this (raw ResourceGroup.create()) and + // runs more than once for the same resource -- e.g. discovery reconciling + // the same LXC from multiple Proxmox cluster nodes -- silently accumulates + // duplicate access/admin rows every pass, with no error to notice it by. + static async ensure(resourceId, groupCn, accessLevel) { + const existing = await this.list({ where: { resourceId, groupCn } }); + if (existing.length) return existing[0]; + return this.create({ id: crypto.randomUUID(), resourceId, groupCn, accessLevel }); + } } module.exports = { diff --git a/nodejs/package.json b/nodejs/package.json index 3b6c636..4556858 100755 --- a/nodejs/package.json +++ b/nodejs/package.json @@ -11,7 +11,7 @@ "scripts": { "start": "node ./bin/www", "dev": "npx nodemon --ignore public/ ./bin/www", - "test": "NODE_ENV=test jest tests/groups.test.js tests/subtypes.test.js tests/site_join.test.js tests/site_config.test.js tests/site_replicate.test.js tests/proxy_client.test.js tests/nmap_plugin.test.js --forceExit" + "test": "NODE_ENV=test jest tests/groups.test.js tests/subtypes.test.js tests/site_join.test.js tests/site_config.test.js tests/site_replicate.test.js tests/proxy_client.test.js tests/reconciler.test.js tests/nmap_plugin.test.js --forceExit" }, "jest": { "testEnvironment": "node", diff --git a/nodejs/routes/api_directory_admin.js b/nodejs/routes/api_directory_admin.js index 7da419b..5ebaf57 100644 --- a/nodejs/routes/api_directory_admin.js +++ b/nodejs/routes/api_directory_admin.js @@ -68,10 +68,10 @@ async function ensureGroup(name, ownerDn, description) { // naive create on every Directory self-heal (which runs ensureSiteGroups / // provisionResourceGroups on each load) was accumulating duplicate links -- the // "groups appear 3x under a resource" bug. Always check first. +// (services/discovery_reconciler.js's autoPromote path had the same bug via +// its own raw ResourceGroup.create() -- both now share ResourceGroup.ensure().) async function ensureResourceGroup(resourceId, groupCn, accessLevel) { - const existing = await ResourceGroup.list({ where: { resourceId, groupCn } }); - if (existing.length) return existing[0]; - return ResourceGroup.create({ resourceId, groupCn, accessLevel }); + return ResourceGroup.ensure(resourceId, groupCn, accessLevel); } // Provision the site-level groups + the aggregates the per-resource groups nest @@ -215,35 +215,17 @@ router.get('/resources', async (req, res, next) => { }); // Even admins never receive secret metadata (e.g. client_secret_hash) over // the wire; projectResources strips it unconditionally. - - // Self-heal the group model (docs/GROUPS.md): ensure every site has its - // site-level groups (S_super_admin, S_hosts_*, S_apps_*, S_everyone) + the - // aggregates, and every host/app resource has its per-resource groups nested - // into them. Idempotent, so this is a cheap no-op once present -- it's what - // backfills a directory seeded by an older release without a rebuild. - // Never fails the list. - const sites = resources.filter(r => r.kind === 'site'); - await Promise.all(sites.map(site => - ensureSiteGroups(site.slug, req.user.dn, site.name, site.id) - .catch(err => console.error(`ensureSiteGroups(${site.slug}) failed:`, err.message)) - )); - const siteByResource = new Map(); - for (const site of sites) siteByResource.set(site.id, site.slug); - const siteOf = async (r) => { - const direct = siteByResource.get(r.id); - if (direct) return direct; - // findAncestorSiteSlug returns the site's full slug (`site_local`) -- the - // group-model builders take it verbatim, so do NOT strip the `site_` prefix. - return await Resource.findAncestorSiteSlug(r.id).catch(() => null); - }; - await Promise.all(resources.map(async (r) => { - const gKind = groupKind(r); - if (!gKind) return; - const siteSlug = await siteOf(r); - if (!siteSlug) return; - await provisionResourceGroups(r, gKind, siteSlug, req.user.dn) - .catch(err => console.error(`provisionResourceGroups(${r.slug}) failed:`, err.message)); - })); + // + // Group-model self-heal (docs/GROUPS.md) used to run here, on every GET -- + // idempotent per-call, but the fan-out (ensureSiteGroups per site + + // provisionResourceGroups per resource, each several sequential LDAP + // round-trips) ran unconditionally on every single list, which is what + // made this route slow/unresponsive once a directory had more than a + // handful of resources. Healing now happens where resources actually + // change instead: POST /resources, PUT /resources/:id (see below), and + // POST /discovery/promote/:slug. See POST /resources/heal-groups for an + // on-demand equivalent of what this GET used to do implicitly, for + // backfilling a directory seeded before this change. const projected = projectResources(resources, { fullMetadata: true }).map(r => { r.hasSecret = !!(r.metadata?.hasSecret || (r.metadata?.secretKeys && r.metadata.secretKeys.length > 0)); @@ -290,6 +272,40 @@ router.use((req, res, next) => { next(); }); +// On-demand equivalent of the group-model self-heal that GET /resources used +// to run implicitly on every list (see the comment there). Same fan-out, +// same idempotent ensure()-based helpers -- just explicit and admin- +// triggered instead of hidden in every page load, for backfilling a +// directory whose resources predate write-time healing. +router.post('/resources/heal-groups', async (req, res, next) => { + try { + const resources = await Resource.list(); + const sites = resources.filter(r => r.kind === 'site'); + await Promise.all(sites.map(site => + ensureSiteGroups(site.slug, req.user.dn, site.name, site.id) + .catch(err => console.error(`ensureSiteGroups(${site.slug}) failed:`, err.message)) + )); + const siteByResource = new Map(); + for (const site of sites) siteByResource.set(site.id, site.slug); + const siteOf = async (r) => { + const direct = siteByResource.get(r.id); + if (direct) return direct; + return await Resource.findAncestorSiteSlug(r.id).catch(() => null); + }; + let healed = 0; + await Promise.all(resources.map(async (r) => { + const gKind = groupKind(r); + if (!gKind) return; + const siteSlug = await siteOf(r); + if (!siteSlug) return; + await provisionResourceGroups(r, gKind, siteSlug, req.user.dn) + .then(() => { healed += 1; }) + .catch(err => console.error(`provisionResourceGroups(${r.slug}) failed:`, err.message)); + })); + res.json({ status: 'ok', sitesHealed: sites.length, resourcesHealed: healed }); + } catch (err) { next(err); } +}); + router.post('/resources', async (req, res, next) => { try { if (!req.body.hostId && req.body.parentSlug) { @@ -407,7 +423,24 @@ router.put('/resources/:id', async (req, res, next) => { await ResourceEdge.create({ parentId: req.body.hostId, childId: r.id, relation: updated.kind === 'oauth' ? 'oauth' : 'hosts' }); } } - + + // Group provisioning (docs/GROUPS.md), same as POST /resources -- an + // update can be what first makes a resource group-eligible (e.g. a + // manual `metadata.managed` edit, or a reparent moving it under a + // different site), and this route never provisioned groups at all + // before. Never fails the update: groups are repairable via + // POST /resources/heal-groups if this best-effort attempt fails. + const gKind = groupKind(updated); + if (gKind) { + const ancestorSite = await Resource.findAncestorSiteSlug(updated.id).catch(() => null); + if (ancestorSite) { + await ensureSiteGroups(ancestorSite, req.user.dn, updated.name) + .catch(err => console.error(`ensureSiteGroups(${ancestorSite}) failed:`, err.message)); + await provisionResourceGroups(updated, gKind, ancestorSite, req.user.dn) + .catch(err => console.error(`provisionResourceGroups(${updated.slug}) failed:`, err.message)); + } + } + res.json({ results: updated }); } catch (err) { next(err); diff --git a/nodejs/routes/discovery.js b/nodejs/routes/discovery.js index b63d518..5963ea5 100644 --- a/nodejs/routes/discovery.js +++ b/nodejs/routes/discovery.js @@ -169,20 +169,14 @@ router.post('/promote/:slug', async (req, res, next) => { else throw e; } - // Link them - const crypto = require('crypto'); - await ResourceGroup.create({ - id: crypto.randomUUID(), - resourceId: resource.id, - groupCn: accessGroup, - accessLevel: 'user' - }); - await ResourceGroup.create({ - id: crypto.randomUUID(), - resourceId: resource.id, - groupCn: adminGroup, - accessLevel: 'admin' - }); + // Link them. ensure(), not create(): a re-submitted/retried Promote + // click (or the modal being saved twice) had no existence check here, + // so repeated promotion attempts on the same resource accumulated + // duplicate access/admin group rows -- see ResourceGroup.ensure()'s + // comment on models/resource.js for why this can't rely on a DB + // constraint instead. + await ResourceGroup.ensure(resource.id, accessGroup, 'user'); + await ResourceGroup.ensure(resource.id, adminGroup, 'admin'); const meta = resource.metadata || {}; meta.managed = true; diff --git a/nodejs/services/discovery_reconciler.js b/nodejs/services/discovery_reconciler.js index f7bc73d..f636667 100644 --- a/nodejs/services/discovery_reconciler.js +++ b/nodejs/services/discovery_reconciler.js @@ -320,8 +320,14 @@ class DiscoveryReconciler { await Group.get(adminGroup).catch(async (e) => { if (e.status === 404) await Group.add({ name: adminGroup, description: `Admin access to ${res.name}`, owner: 'cn=admin' }); }); - await ResourceGroup.create({ id: crypto.randomUUID(), resourceId: res._actualId, groupCn: accessGroup, accessLevel: 'user' }).catch(() => {}); - await ResourceGroup.create({ id: crypto.randomUUID(), resourceId: res._actualId, groupCn: adminGroup, accessLevel: 'admin' }).catch(() => {}); + // ensure(), not create(): reconcile() runs on every discovery pass + // (e.g. once per Proxmox cluster node reporting the same LXC), and + // a raw create() here had no existence check, so a resource ended + // up with the same access/admin group rows duplicated once per + // pass -- see ResourceGroup.ensure()'s comment for why this can't + // rely on a DB constraint instead. + await ResourceGroup.ensure(res._actualId, accessGroup, 'user').catch(() => {}); + await ResourceGroup.ensure(res._actualId, adminGroup, 'admin').catch(() => {}); } catch (err) { console.error(`[DiscoveryReconciler] autoPromote failed for ${res.slug}:`, err.message); } diff --git a/nodejs/tests/reconciler.test.js b/nodejs/tests/reconciler.test.js index 8c7ad05..1891c51 100644 --- a/nodejs/tests/reconciler.test.js +++ b/nodejs/tests/reconciler.test.js @@ -1,5 +1,5 @@ require('./setup'); -const { Resource } = require('../models/resource'); +const { Resource, ResourceGroup } = require('../models/resource'); const { DiscoveryReconciler } = require('../services/discovery_reconciler'); describe('DiscoveryReconciler', () => { @@ -72,4 +72,27 @@ describe('DiscoveryReconciler', () => { expect(merged.metadata.interfaces).toHaveLength(1); expect(merged.metadata.interfaces[0].ip).toBe('10.0.0.6'); // Updated IP }); + + it('does not duplicate access/admin groups across repeated autoPromote passes', async () => { + // Regression: autoPromote used to call ResourceGroup.create() directly + // with no existence check, so reconciling the same managed resource + // more than once (e.g. a Proxmox cluster reporting one LXC from + // multiple nodes) accumulated duplicate access/admin rows every pass. + const payload = { + resources: [{ + kind: 'host', + name: 'LXC 127', + slug: 'lxc-127', + metadata: { interfaces: [{ mac: '00:11:22:33:44:99', ip: '10.0.0.99' }] } + }] + }; + + await DiscoveryReconciler.reconcile('plugin-A', payload, { autoPromote: true }); + await DiscoveryReconciler.reconcile('plugin-A', payload, { autoPromote: true }); + await DiscoveryReconciler.reconcile('plugin-A', payload, { autoPromote: true }); + + const resource = (await Resource.list()).find((r) => r.slug === 'lxc-127'); + const groups = await ResourceGroup.list({ where: { resourceId: resource.id } }); + expect(groups.map((g) => g.groupCn).sort()).toEqual(['lxc-127_access', 'lxc-127_admin']); + }); });