From fd5c5f4e88c2bfae6236495847cd5edb92516a11 Mon Sep 17 00:00:00 2001 From: William Mantly Date: Tue, 4 Aug 2026 19:01:51 -0400 Subject: [PATCH] fix: use verbatim resource slugs in group names (matches access-request tests + live convention) The group naming inserts a kind segment (resourceGroupCns(site, kind, slug, level)), but the access-request tests + the live directory convention are verbatim ({site}_{slug}_{level} -- the kind is carried in the resource slug, e.g. host_theta-env). For bare test slugs this produced site_x_host_artest-host_x_access instead of the expected site_x_artest-host_x_access, so the requester was never removed from the auto-provisioned access group and every request 409'd. resourceGroupCns is now (site, slug, level) with the verbatim slug; the kind is used only to pick the aggregate the group nests into. --- CHANGELOG.md | 2 +- nodejs/routes/api_directory_admin.js | 35 +++++++++------------- nodejs/tests/groups.test.js | 45 +++++++++++++--------------- nodejs/utils/groups.js | 30 +++++++++---------- 4 files changed, 50 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cb9736..e5491ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,5 @@ # v1.26.0 -- feat: complete the group model (docs/GROUPS.md) — `god_admin` is now seeded into LDAP and nested into `app_super_admin`; every site auto-provisions `{site}_super_admin`, `{site}_hosts_*`/`{site}_apps_*` aggregates and `{site}_everyone`; per-resource `_admin`/`_access` groups are nested into the site aggregates so the inheritance lattice exists in LDAP, not just in the resolver; service resources map to the group model's `app` kind (`site_local_app__*` instead of the kind-less `site_local__*`). Site/aggregate groups are self-healed idempotently on every Directory load, so a directory seeded by an older release picks them up without a rebuild. +- feat: complete the group model (docs/GROUPS.md) — `god_admin` is now seeded into LDAP and nested into `app_super_admin`; every site auto-provisions `{site}_super_admin`, `{site}_hosts_*`/`{site}_apps_*` aggregates and `{site}_everyone`; per-resource `_admin`/`_access` groups (named `{site}_{slug}_{level}`, the kind carried in the resource slug) are nested into the site aggregates so the inheritance lattice exists in LDAP, not just in the resolver. Site/aggregate groups are self-healed idempotently on every Directory load, so a directory seeded by an older release picks them up without a rebuild. - feat: the naming convention is now enforced server-side — `POST /api/directory-admin/groups` rejects a group CN that isn't a valid group for the target resource (its own `_admin`/`_access`/capability, a site aggregate, a site-level group, or `god_admin`), so the free-text field can no longer mint `*_accessmember`-style names - feat: `god_admin` is managed from the Directory — the site resource modal surfaces `god_admin` + the site-level groups as associated groups, so its members (and the site's) are editable right there - fix: Directory agent status dots no longer paint every host red when the `/api/agent/nodes` endpoint is unreachable (older app or transient outage) — they now show a neutral grey "agent service unreachable" instead of a false alarm diff --git a/nodejs/routes/api_directory_admin.js b/nodejs/routes/api_directory_admin.js index 7a63730..5695cdd 100644 --- a/nodejs/routes/api_directory_admin.js +++ b/nodejs/routes/api_directory_admin.js @@ -47,15 +47,6 @@ function groupKind(resource) { return null; } -// The group-model slug for a resource: strip the kind prefix that directory -// resource slugs carry (`site_local` -> `local`, `host_theta-env` -> `theta-env`) -// so the group-model builders can re-apply their own `{site}_{kind}_{slug}` -// structure without double-prefixing. Services are stored without a prefix, so -// this is a no-op for them. -function resourceSlug(slug) { - return String(slug || '').replace(/^(site|host|app)_/, ''); -} - // Create a groupOfNames if it doesn't already exist. Idempotent; `ownerDn` // seeds the mandatory first member. Returns true when created. async function ensureGroup(name, ownerDn, description) { @@ -122,16 +113,18 @@ async function ensureSiteGroups(siteSlug, ownerDn, siteName, siteResourceId) { } // Provision the per-resource groups for a host/app and nest them into the site -// aggregates (so a site/aggregate admin reaches this resource by membership): +// aggregates (so a site/aggregate admin reaches this resource by membership). +// The specific group name uses the resource's slug verbatim +// (`{site}_{slug}_{level}` -- the kind is carried in the slug, e.g. `host_theta-env`); +// `kind` (host/app) selects which aggregate the group nests into: // -// {site}_{kind}_{slug}_admin -> {site}_{kind}_{slug}_access -// {site}_{kind}_{slug}_admin -> {site}_{kind}s_admin (aggregate) -// {site}_{kind}_{slug}_access -> {site}_{kind}s_access (aggregate) -// app_super_admin -> {site}_{kind}_{slug}_admin (legacy cross-app) +// {site}_{slug}_admin -> {site}_{slug}_access +// {site}_{slug}_admin -> {site}_{kind}s_admin (aggregate) +// {site}_{slug}_access -> {site}_{kind}s_access (aggregate) +// app_super_admin -> {site}_{slug}_admin (legacy cross-app) async function provisionResourceGroups(resource, kind, siteSlug, ownerDn) { - const slug = resourceSlug(resource.slug); - const accessCn = groups.resourceGroupCns(siteSlug, kind, slug, 'access'); - const adminCn = groups.resourceGroupCns(siteSlug, kind, slug, 'admin'); + const accessCn = groups.resourceGroupCns(siteSlug, resource.slug, 'access'); + const adminCn = groups.resourceGroupCns(siteSlug, resource.slug, 'admin'); await ensureGroup(accessCn, ownerDn, `Access group for ${resource.name}`); await ensureGroup(adminCn, ownerDn, `Admin group for ${resource.name}`); @@ -165,14 +158,14 @@ function validGroupCnsForResource(resource, siteSlug) { } const kind = groupKind(resource); // 'host'|'app'|null if (kind) { - const slug = resourceSlug(resource.slug); - valid.add(groups.resourceGroupCns(siteSlug, kind, slug, 'admin')); - valid.add(groups.resourceGroupCns(siteSlug, kind, slug, 'access')); + const slug = resource.slug; // verbatim (kind is carried in the slug) + valid.add(groups.resourceGroupCns(siteSlug, slug, 'admin')); + valid.add(groups.resourceGroupCns(siteSlug, slug, 'access')); valid.add(groups.aggregateGroupCns(siteSlug, kind, 'admin')); valid.add(groups.aggregateGroupCns(siteSlug, kind, 'access')); valid.add(groups.siteSuperAdminCns(siteSlug)); valid.add(groups.siteEveryoneCns(siteSlug)); - return { valid, capRe: new RegExp(`^${siteSlug}_(${kind}_${slug}_|${kind}s_)[a-z0-9-]+$`) }; + return { valid, capRe: new RegExp(`^${siteSlug}_(${slug}_|${kind}s_)[a-z0-9-]+$`) }; } // oauth/container etc. — only the global god_admin makes sense to pin here. valid.add(groups.siteSuperAdminCns(siteSlug)); diff --git a/nodejs/tests/groups.test.js b/nodejs/tests/groups.test.js index 110ada5..9ead71a 100644 --- a/nodejs/tests/groups.test.js +++ b/nodejs/tests/groups.test.js @@ -12,13 +12,12 @@ const { GOD_ADMIN, } = require('../utils/groups'); -// Resource fixtures use already-normalized slugs (the group-model builders treat -// the site + resource slugs as opaque, not re-slugified -- see groups.js). Raw -// display names like "Main Office" are normalized by the resource model before -// they reach the resolver. -const HOST = { site: 'main-office', kind: 'host', slug: 'web-01' }; +// Resource fixtures mirror the directory's real slugs: hosts carry a `host_` +// prefix, services/apps are stored bare. The group-model builders use these +// verbatim (no re-slugifying, no kind insertion) -- see groups.js. +const HOST = { site: 'main-office', kind: 'host', slug: 'host_web-01' }; const APP = { site: 'main-office', kind: 'app', slug: 'emby' }; -const OTHER_SITE_HOST = { site: 'branch-office', kind: 'host', slug: 'db' }; +const OTHER_SITE_HOST = { site: 'branch-office', kind: 'host', slug: 'host_db' }; describe('slugify', () => { test('lowercases, spaces and underscores become hyphens, no leading/trailing dash', () => { @@ -28,18 +27,14 @@ describe('slugify', () => { expect(slugify(' Mixed CASE--name ')).toBe('mixed-case-name'); expect(slugify('')).toBe(''); }); - test('never contains an underscore (the structural delimiter)', () => { - expect(slugify('a_b_c')).not.toContain('_'); - expect(resourceGroupCns('Main Office', 'host', 'Web 01', 'access')).not.toContain('__'); - }); }); describe('group cn builders', () => { - test('per-resource uses singular kind', () => { - expect(resourceGroupCns('main-office', 'host', 'web-01', 'admin')).toBe('main-office_host_web-01_admin'); - expect(resourceGroupCns('main-office', 'app', 'emby', 'access')).toBe('main-office_app_emby_access'); + test('per-resource uses the resource slug verbatim (kind is carried in the slug)', () => { + expect(resourceGroupCns('main-office', 'host_web-01', 'admin')).toBe('main-office_host_web-01_admin'); + expect(resourceGroupCns('main-office', 'emby', 'access')).toBe('main-office_emby_access'); }); - test('aggregate uses plural kind', () => { + test('aggregate uses the plural kind', () => { expect(aggregateGroupCns('main-office', 'host', 'admin')).toBe('main-office_hosts_admin'); expect(aggregateGroupCns('main-office', 'app', 'access')).toBe('main-office_apps_access'); }); @@ -53,10 +48,10 @@ describe('group cn builders', () => { expect(siteSuperAdminCns('site_local')).toBe('site_local_super_admin'); expect(siteEveryoneCns('site_local')).toBe('site_local_everyone'); expect(aggregateGroupCns('site_local', 'host', 'admin')).toBe('site_local_hosts_admin'); - expect(resourceGroupCns('site_local', 'host', 'theta-env', 'access')).toBe('site_local_host_theta-env_access'); + expect(resourceGroupCns('site_local', 'host_theta-env', 'access')).toBe('site_local_host_theta-env_access'); }); - test('invalid kind throws', () => { - expect(() => resourceGroupCns('s', 'service', 'x', 'admin')).toThrow(); + test('invalid kind throws (aggregates only — per-resource has no kind arg)', () => { + expect(() => aggregateGroupCns('s', 'service', 'admin')).toThrow(); }); }); @@ -94,32 +89,32 @@ describe('hasPermission — inheritance', () => { }); test('specific host group grants only that host', () => { - const cn = resourceGroupCns('main-office', 'host', 'web-01', 'admin'); + const cn = resourceGroupCns('main-office', 'host_web-01', 'admin'); expect(hasPermission([cn], HOST, 'admin')).toBe(true); expect(hasPermission([cn], OTHER_SITE_HOST, 'admin')).toBe(false); }); test('admin implies access; access does not imply admin', () => { - expect(hasPermission([resourceGroupCns('main-office', 'host', 'web-01', 'admin')], HOST, 'access')).toBe(true); - expect(hasPermission([resourceGroupCns('main-office', 'host', 'web-01', 'access')], HOST, 'admin')).toBe(false); + expect(hasPermission([resourceGroupCns('main-office', 'host_web-01', 'admin')], HOST, 'access')).toBe(true); + expect(hasPermission([resourceGroupCns('main-office', 'host_web-01', 'access')], HOST, 'admin')).toBe(false); }); test('capabilities are exact — admin does not grant a capability', () => { - expect(hasPermission([resourceGroupCns('main-office', 'host', 'web-01', 'reboot')], HOST, 'reboot')).toBe(true); - expect(hasPermission([resourceGroupCns('main-office', 'host', 'web-01', 'admin')], HOST, 'reboot')).toBe(false); + expect(hasPermission([resourceGroupCns('main-office', 'host_web-01', 'reboot')], HOST, 'reboot')).toBe(true); + expect(hasPermission([resourceGroupCns('main-office', 'host_web-01', 'admin')], HOST, 'reboot')).toBe(false); // aggregate capability expect(hasPermission(['main-office_hosts_reboot'], HOST, 'reboot')).toBe(true); }); test('hosts and apps are orthogonal namespaces', () => { - const hostAdmin = resourceGroupCns('main-office', 'host', 'web-01', 'admin'); + const hostAdmin = resourceGroupCns('main-office', 'host_web-01', 'admin'); expect(hasPermission([hostAdmin], APP, 'access')).toBe(false); - const appAdmin = resourceGroupCns('main-office', 'app', 'emby', 'admin'); + const appAdmin = resourceGroupCns('main-office', 'emby', 'admin'); expect(hasPermission([appAdmin], APP, 'access')).toBe(true); }); test('cross-site isolation', () => { - const mainHostAdmin = resourceGroupCns('main-office', 'host', 'web-01', 'admin'); + const mainHostAdmin = resourceGroupCns('main-office', 'host_web-01', 'admin'); expect(hasPermission([mainHostAdmin], OTHER_SITE_HOST, 'access')).toBe(false); expect(hasPermission(['branch-office_hosts_admin'], OTHER_SITE_HOST, 'admin')).toBe(true); }); diff --git a/nodejs/utils/groups.js b/nodejs/utils/groups.js index 22b6e59..ae0d42a 100644 --- a/nodejs/utils/groups.js +++ b/nodejs/utils/groups.js @@ -41,16 +41,17 @@ function assertKind(kind) { if (!KINDS.includes(kind)) throw new Error(`invalid resource kind: ${kind} (must be host or app)`); } -// {site}_host__ / {site}_app__ +// {site}_{slug}_{level} — the per-resource group for one resource. // -// `site` is the site resource's slug verbatim (e.g. `site_local`, or a clean -// `main-office`), NOT slugified: directory resource slugs may legitimately carry -// a kind prefix (`site_local`, `host_theta-env`), and re-slugifying those would -// corrupt the delimiter (site_local -> site-local). Callers pass already- -// normalized site + kind-stripped resource slugs. -function resourceGroupCns(site, kind, slug, level) { - assertKind(kind); - return `${site}_${kind}_${slugify(slug)}_${level}`; +// Both `site` and `slug` are the resource slugs verbatim (e.g. `site_local`, +// `host_theta-env`), NOT slugified or kind-inserted: directory resource slugs +// carry their kind as a prefix (`host_theta-env`), so `site_local` + `host_theta-env` +// yields `site_local_host_theta-env_access`. Services are stored without a +// prefix (`sso-manager`), yielding `site_local_sso-manager_access`. This is the +// convention the auto-provisioner, the resolver, and the access-request tests +// all share -- re-slugifying or inserting a kind would double the delimiter. +function resourceGroupCns(site, slug, level) { + return `${site}_${slug}_${level}`; } // {site}_hosts_ / {site}_apps_ (plural kind — the aggregate). @@ -93,12 +94,11 @@ function levelGrants(level, wanted) { // granted groups (see permission.onResource). This keeps the function pure over // the user's membership only. function hasPermission(memberOf, resource, level) { - // `site` is used verbatim (resource slugs may carry a kind prefix, e.g. - // `site_local`) -- see resourceGroupCns; the resource `slug` is expected - // kind-stripped and already-normalized, so slugify() is a harmless guard. + // `site` and `slug` are used verbatim (resource slugs may carry a kind prefix, + // e.g. `site_local` / `host_theta-env`) -- see resourceGroupCns. const site = resource && resource.site; const kind = resource && resource.kind; - const slug = slugify(resource && resource.slug); + const slug = resource && resource.slug; const set = new Set(memberOf || []); if (set.has(GOD_ADMIN)) return true; @@ -107,13 +107,13 @@ function hasPermission(memberOf, resource, level) { if (isKnownLevel(level)) { // admin / access if (set.has(aggregateGroupCns(site, kind, level))) return true; - if (set.has(resourceGroupCns(site, kind, slug, level))) return true; + if (set.has(resourceGroupCns(site, slug, level))) return true; if (level === 'access' && hasPermission(memberOf, resource, 'admin')) return true; return false; } // Opaque capability — exact aggregate or specific grant only. if (set.has(aggregateGroupCns(site, kind, level))) return true; - if (set.has(resourceGroupCns(site, kind, slug, level))) return true; + if (set.has(resourceGroupCns(site, slug, level))) return true; return false; }