theta42/sso-manager-node
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #201 from theta42/fix-group-dup-and-resources-perf

Browse Source 
fix(directory): dedupe access/admin groups; stop self-healing groups on every GET
This commit is contained in:
William Mantly
2026-08-10 19:23:17 -07:00
committed by GitHub
parent b6a82d58d5 2c3ec4e967
commit 0a3bdbef06
6 changed files with 120 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nodejs/models/resource.js
+13
View File
@@ -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 = {
nodejs/package.json
+1 -1
View File
@@ -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 --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 --forceExit"
},
"jest": {
"testEnvironment": "node",
nodejs/routes/api_directory_admin.js
+66 -33
View File
@@ -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);
nodejs/routes/discovery.js
+8 -14
View File
@@ -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;
nodejs/services/discovery_reconciler.js
+8 -2
View File
@@ -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);
}
nodejs/tests/reconciler.test.js
+24 -1
View File
@@ -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']);
});
});