diff --git a/src/api/ctClient.ts b/src/api/ctClient.ts index 1dbc755..cb01262 100644 --- a/src/api/ctClient.ts +++ b/src/api/ctClient.ts @@ -97,8 +97,10 @@ export class CtClient { throw new CtApiError("Login succeeded but no session cookie was returned", res.status, null); } await this.refreshCsrfToken(); - const body = (await res.json()) as { data: WhoAmI }; - return body.data; + // Same tolerant unwrap as request(): prefer `.data`, but fall back to the raw body if the + // envelope is absent, so authenticate and request() agree on the shape. + const body = (await res.json()) as { data?: WhoAmI }; + return (body.data ?? body) as WhoAmI; } async get(path: string): Promise { @@ -155,20 +157,12 @@ export class CtClient { } private async refreshCsrfToken(): Promise { - if (!this.cookie) { - return; - } - const res = await fetchWithRetry( - `${this.config.host}/api/csrftoken`, - { headers: { Accept: "application/json", Cookie: this.cookie } }, - { isIdempotent: true }, - ); - this.captureCookie(res); - if (!res.ok) { - throw new CtApiError("Failed to fetch CSRF token", res.status, await safeBody(res)); - } - const body = (await res.json()) as { data: string }; - this.csrfToken = body.data; + // A plain authenticated GET: it rides the session cookie and GET skips the CSRF branch in + // request(), so this cannot recurse — and it reuses request()'s envelope unwrap + guarded 2xx + // parsing instead of duplicating the bootstrap fetch here. Callers only reach this once a cookie + // exists (authenticate sets it; request()'s write path guards on it), so the old empty-cookie + // early-return is unreachable and dropped. + this.csrfToken = await this.get("/csrftoken"); } /** Merge any Set-Cookie values into the stored cookie header. */ diff --git a/src/auth/tokenStore.ts b/src/auth/tokenStore.ts index 8127fd7..5f229be 100644 --- a/src/auth/tokenStore.ts +++ b/src/auth/tokenStore.ts @@ -64,7 +64,25 @@ async function keychainSet(value: string): Promise { ]); } +/** + * Memoized keychain blob for this process. A single run resolves the host + * (via `resolveConfig`) AND the token (via `authedSession`) — each of which + * reaches for the stored credentials — so without this cache the same entry is + * fetched up to 3× per command, spawning `security find-generic-password` + * (and prompting to unlock a locked Keychain) every time. `undefined` = not yet + * read; `null` = read and absent. Invalidated on any write (`resetKeychainCache`). + */ +let cachedKeychainBlob: string | null | undefined; + +/** Drop the memoized keychain read. Called after every store/clear; exported for tests. */ +export function resetKeychainCache(): void { + cachedKeychainBlob = undefined; +} + async function keychainGet(): Promise { + if (cachedKeychainBlob !== undefined) { + return cachedKeychainBlob; + } try { const { stdout } = await run("security", [ "find-generic-password", @@ -74,10 +92,11 @@ async function keychainGet(): Promise { KEYCHAIN_ACCOUNT, "-w", ]); - return stdout.trim() || null; + cachedKeychainBlob = stdout.trim() || null; } catch { - return null; + cachedKeychainBlob = null; } + return cachedKeychainBlob; } async function keychainDelete(account: string): Promise { @@ -96,6 +115,7 @@ export async function storeCredentials(creds: Credentials): Promise { ); } await keychainSet(JSON.stringify(creds)); + resetKeychainCache(); // a fresh login must invalidate any read the process already cached return `macOS Keychain (service "${KEYCHAIN_SERVICE}", account "${KEYCHAIN_ACCOUNT}")`; } @@ -128,4 +148,5 @@ export async function clearCredentials(): Promise { // Also drop the pre-host bare-token entry so an upgrade doesn't leave a secret behind. await keychainDelete(LEGACY_KEYCHAIN_ACCOUNT); } + resetKeychainCache(); // a later read in the same process must not return the cleared secret } diff --git a/src/commands/apply.ts b/src/commands/apply.ts index ffd7738..7443f30 100644 --- a/src/commands/apply.ts +++ b/src/commands/apply.ts @@ -1,16 +1,15 @@ import { dirname, join } from "node:path"; import { Command } from "commander"; -import type { CtClient } from "../api/ctClient.js"; import { authedSession } from "../api/session.js"; import { resolveConfig } from "../config.js"; -import { loadState, resolveStatePath, saveState, type State } from "../state/state.js"; +import { loadState, resolveStatePath, saveState } from "../state/state.js"; import { loadConfig, resolveConfigPath } from "../config/load.js"; import { buildPlan } from "../engine/build.js"; import { executePlan } from "../engine/execute.js"; +import { runPostApplyHooks } from "../engine/synthetic.js"; import { writeBackup } from "../engine/backup.js"; import { renderPlan } from "../engine/render.js"; -import { summarize, type Plan } from "../engine/types.js"; -import { assertNotPeople } from "../engine/guard.js"; +import { summarize } from "../engine/types.js"; import { buildPermissionPlan } from "../permissions/plan.js"; import { renderPermissionPlan } from "../permissions/render.js"; import { applyPermissionPlan } from "../permissions/apply.js"; @@ -26,49 +25,6 @@ interface ApplyOptions { refresh?: boolean; } -interface RefreshResult { - created: number; - updated: number; - deleted: number; -} - -/** - * Post-apply dynamic-group refresh (opt-in via `--refresh`). For each applied - * item whose changes touched the `dynamic` synthetic field, POST the - * per-group `/dynamicgroups/{id}/refresh` endpoint to materialize computed - * membership. Deliberately per-group only — the all-groups - * `/dynamicgroups/refresh` endpoint has a huge blast radius and must never be - * called from here. - * - * The id is read from state (post-apply, so creates have their real id) using - * an explicit `undefined` check — CT ids can legitimately be `0`. - */ -export async function refreshChangedDynamicGroups( - plan: Plan, - state: State, - client: Pick, -): Promise { - for (const item of plan.items) { - if (item.action === "no-op" || item.action === "delete") continue; - const dynamicChange = item.changes.find((c) => c.field === "dynamic"); - if (!dynamicChange) continue; - const to = dynamicChange.to as { status?: string } | undefined; - if (to?.status === "none") continue; // demoted to a non-dynamic group — nothing to refresh - const id = state.resources[item.key]?.id; - if (id === undefined) continue; - const path = `/dynamicgroups/${id}/refresh`; - assertNotPeople(path); - try { - const res = await client.request("POST", path); - const r = res?.[0]; - if (r) info(`refreshed ${item.key}: +${r.created} ~${r.updated} -${r.deleted}`); - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - warn(`Failed to refresh ${item.key} (#${id}): ${message}`); - } - } -} - /** backups/ dir: explicit flag → CT_BACKUP_DIR → `backups/` beside the state file. */ export function resolveBackupDir( explicit: string | undefined, @@ -97,8 +53,13 @@ export function applyCommand(): Command { const state = await loadState(statePath, config.host); const { client } = await authedSession(); - const { plan, actual, fetchErrors } = await buildPlan(client, state, desired, { configDir }); - const { items: permItems, fetchErrors: permFetchErrors } = await buildPermissionPlan(client, state, permissions, desired); + // Independent fetches: the resource plan and the permission plan (whose instance-wide + // /permissions/ reads are slow) run concurrently rather than back-to-back. + const [{ plan, actual, fetchErrors }, { items: permItems, fetchErrors: permFetchErrors }] = + await Promise.all([ + buildPlan(client, state, desired, { configDir }), + buildPermissionPlan(client, state, permissions, desired), + ]); const allFetchErrors = [...fetchErrors, ...permFetchErrors]; if (allFetchErrors.length > 0) { @@ -163,9 +124,21 @@ export function applyCommand(): Command { if (permResult.granted > 0 || permResult.deleted > 0) { success(`Permissions applied: ${permResult.granted} granted, ${permResult.deleted} deleted.`); } + if (permResult.failed.length > 0) { + // Mirror executePlan's resumable stance: report which tuples failed (not a raw stack) and + // exit non-zero. Grants are reconciled statelessly, so a plain re-run resumes idempotently. + error( + `${permResult.failed.length} permission write(s) failed — re-run to resume (grant reconciliation is idempotent):`, + ); + for (const f of permResult.failed) { + info(` ${f.method} ${f.path} (authId ${f.authId}${f.dataId.length ? ` dataId ${f.dataId.join(",")}` : ""}): ${f.message}`); + } + process.exitCode = 1; + return; + } if (opts.refresh) { - await refreshChangedDynamicGroups(plan, state, client); + await runPostApplyHooks(plan, state, client); } }); } diff --git a/src/commands/get.ts b/src/commands/get.ts index cb3f683..627ee4e 100644 --- a/src/commands/get.ts +++ b/src/commands/get.ts @@ -1,6 +1,6 @@ import { Command } from "commander"; import { authedSession } from "../api/session.js"; -import { loadCatalog } from "../permissions/catalog.js"; +import { CATALOG } from "../permissions/catalog.js"; import { out } from "../ui.js"; /** @@ -40,9 +40,8 @@ export function getCommand(): Command { .command("permissions-catalog") .description("List the static permission-name → authId catalog (for use in config `grants`)") .action(() => { - const catalog = loadCatalog(); - for (const name of Object.keys(catalog).sort()) { - const entry = catalog[name]; + for (const name of Object.keys(CATALOG).sort()) { + const entry = CATALOG[name]; if (!entry) continue; const scoped = entry.scopeField ? "scoped" : "unscoped"; process.stdout.write(`${name} -> ${entry.authId} (${scoped})\n`); diff --git a/src/commands/plan.ts b/src/commands/plan.ts index a452110..cf51780 100644 --- a/src/commands/plan.ts +++ b/src/commands/plan.ts @@ -25,14 +25,16 @@ export function planCommand(): Command { const config = await resolveConfig(); const configPath = resolveConfigPath(opts.config); const { resources: desired, permissions, configDir } = await loadConfig(configPath); + // loadState already refuses a host mismatch (state.ts) — no second guard needed here. const state = await loadState(resolveStatePath(opts.state), config.host); - if (state.host !== config.host) { - throw new Error(`State host (${state.host}) does not match CT_HOST (${config.host}).`); - } const { client } = await authedSession(); - const { plan, fetchErrors } = await buildPlan(client, state, desired, { configDir }); - const { items: permItems, fetchErrors: permFetchErrors } = await buildPermissionPlan(client, state, permissions, desired); + // Independent fetches run concurrently (see commands/apply.ts). + const [{ plan, fetchErrors }, { items: permItems, fetchErrors: permFetchErrors }] = + await Promise.all([ + buildPlan(client, state, desired, { configDir }), + buildPermissionPlan(client, state, permissions, desired), + ]); if (opts.json) { out({ plan, permissions: permItems }); } else { diff --git a/src/engine/graph.ts b/src/engine/graph.ts index a791d37..9e665c3 100644 --- a/src/engine/graph.ts +++ b/src/engine/graph.ts @@ -8,21 +8,18 @@ * runs in the exact reverse. */ import type { DesiredResource } from "./types.js"; +import { RESOURCES } from "../resources/registry.js"; -/** Lower tier is applied first. Delete runs highest tier first. */ -export const TYPE_TIER: Record = { - campus: 0, - "group-type": 0, - "group-status": 0, - "age-group": 0, - "target-group": 0, - "relationship-type": 0, - group: 1, - "group-hierarchy": 2, - "group-role": 3, - permission: 4, - "dynamic-group": 5, -}; +/** + * Lower tier is applied first; delete runs highest tier first. Derived from the resource registry + * (each entry owns its `tier`) rather than hand-maintained here — a new type gets ordered by adding + * one registry entry, and phantom types (`group-hierarchy`, `permission`, `dynamic-group`, …) that + * are synthetic sub-resources or separate plan domains, never `DesiredResource` types, cannot creep + * back in. The exported shape (`Record`) is unchanged, so `computePlan` still reads it. + */ +export const TYPE_TIER: Record = Object.fromEntries( + Object.entries(RESOURCES).map(([type, spec]) => [type, spec.tier]), +); export function tierOf(type: string): number { return TYPE_TIER[type] ?? 0; diff --git a/src/engine/hierarchy.ts b/src/engine/hierarchy.ts index 20e928e..08cf182 100644 --- a/src/engine/hierarchy.ts +++ b/src/engine/hierarchy.ts @@ -73,14 +73,17 @@ export function applyHierarchy( } } - // For each opted-in desired group that already exists in state, set its actual `parents` to the - // managed parent keys — mirroring the desired-side guard below (one pass, no separate opted-in set). - // A group not yet in state (fresh, first apply) has no actual to annotate; it is created instead. + // Single pass over the desired opt-ins (one copy of the predicate, mirroring the desired-side + // guard below). A group's actual gets a `parents` set only when it opted in AND is a managed + // GROUP in state — the managed-type guard from the old state-side iteration is preserved via the + // `state.resources[d.key]` lookup. A group not yet in state (fresh, first apply) has no actual to + // annotate; it is created instead. for (const d of desired) { if (d.type !== "group" || d.parents === undefined) continue; const managed = state.resources[d.key]; - const a = managed && actual.get(d.key); - if (managed && a) { + if (!managed || managed.type !== "group") continue; + const a = actual.get(d.key); + if (a) { a.parents = managedParentKeys(parentIdsByGroup.get(managed.id) ?? [], groupIdToKey); } } diff --git a/src/engine/plan.ts b/src/engine/plan.ts index 12e70c8..894a09c 100644 --- a/src/engine/plan.ts +++ b/src/engine/plan.ts @@ -20,7 +20,7 @@ import { orderKeys, isKnownType } from "./graph.js"; * change (a `JSON.stringify` comparison would flag it, proposing an update that * can never converge). */ -function deepEqual(a: unknown, b: unknown): boolean { +export function deepEqual(a: unknown, b: unknown): boolean { if (a === b) { return true; } @@ -92,11 +92,23 @@ export function computePlan( for (const d of desired) { if (!isKnownType(d.type)) { throw new Error( - `Unknown resource type "${d.type}" for "${d.key}" — no apply tier defined. Add it to TYPE_TIER.`, + `Unknown resource type "${d.type}" for "${d.key}" — no apply tier defined. Add a registry entry in src/resources/registry.ts.`, ); } } + // Reject duplicate desired keys up front. The DSL path (evaluateConfig) already dedups, but a + // programmatic caller (import command, test harness) that hands `computePlan` a raw array must not + // silently last-wins: `desiredByKey` would collapse the duplicates while the loop below emits both, + // corrupting the plan. Fail loudly instead. + const seen = new Set(); + for (const d of desired) { + if (seen.has(d.key)) { + throw new Error(`Duplicate desired key "${d.key}" — each resource must have a unique logical key.`); + } + seen.add(d.key); + } + const desiredByKey = new Map(desired.map((d) => [d.key, d])); const creates: PlanItem[] = []; const updates: PlanItem[] = []; diff --git a/src/engine/synthetic.ts b/src/engine/synthetic.ts index abe6d25..83b59c5 100644 --- a/src/engine/synthetic.ts +++ b/src/engine/synthetic.ts @@ -8,13 +8,25 @@ import type { CtClient } from "../api/ctClient.js"; import { CtApiError } from "../api/ctClient.js"; import type { State } from "../state/state.js"; -import type { DesiredResource, FieldChange } from "./types.js"; +import type { DesiredResource, FieldChange, Plan, PlanItem } from "./types.js"; import { applyHierarchy, parentIdsByGroupId, type HierarchyEntry } from "./hierarchy.js"; import { assertNotPeople } from "./guard.js"; -import { warn } from "../ui.js"; +import { deepEqual } from "./plan.js"; +import { mapConcurrent } from "../util/concurrency.js"; +import { info, warn } from "../ui.js"; import { normalizeDynamic, normalizeRuleset, resolveRulesetRef } from "./dynamic.js"; import type { DynamicStatus } from "./types.js"; +/** How many dynamic groups to fetch (ruleset + status) from ChurchTools at once. Mirrors build.ts. */ +const DYNAMIC_FETCH_CONCURRENCY = 8; + +/** The per-group counts CT returns from POST /dynamicgroups/{id}/refresh. */ +interface RefreshResult { + created: number; + updated: number; + deleted: number; +} + export interface SyntheticFoldCtx { client: Pick; state: State; @@ -29,10 +41,25 @@ export interface SyntheticApplyCtx { id: number; change: FieldChange; } +export interface SyntheticPostApplyCtx { + client: Pick; + state: State; + /** Post-execute id from state (creates already carry their real id). */ + id: number; + item: PlanItem; + change: FieldChange; +} export interface SyntheticField { field: string; fold(ctx: SyntheticFoldCtx): Promise<{ desired: DesiredResource[]; errors: string[] }>; apply(ctx: SyntheticApplyCtx): Promise; + /** + * Optional opt-in side effect run AFTER the whole plan has applied (e.g. `ct apply --refresh` + * materializing dynamic-group membership). Keeps field-specific post-apply knowledge in the + * field, not the command layer. Must swallow its own errors — one field's failure must not + * abort the others. + */ + postApply?(ctx: SyntheticPostApplyCtx): Promise; } function resolveId(state: State, key: string): number { @@ -83,13 +110,24 @@ const parentsField: SyntheticField = { const dynamicField: SyntheticField = { field: "dynamic", async fold({ client, state, desired, actual, configDir }) { - const optedIn = new Set(desired.filter((d) => d.type === "group" && d.dynamic !== undefined).map((d) => d.key)); - if (optedIn.size === 0) return { desired, errors: [] }; - const errors: string[] = []; - for (const managed of Object.values(state.resources)) { - if (managed.type !== "group" || !optedIn.has(managed.key)) continue; - const a = actual.get(managed.key); - if (!a) continue; // vanished from CT → handled as a recreate by the plain plan + // Single pass over the DESIRED opt-ins (mirrors hierarchy's desired-side gate): a group is + // folded only if it declared `dynamic` AND is under management AND was fetched. Replaces the + // old build-a-Set-then-invert-over-state pattern (one predicate, not three). + const targets = desired.flatMap((d) => { + if (d.type !== "group" || d.dynamic === undefined) return []; + const managed = state.resources[d.key]; + if (!managed || managed.type !== "group") return []; // not adopted yet → created by the plain plan + const a = actual.get(d.key); + if (!a) return []; // vanished from CT → handled as a recreate by the plain plan + return [{ managed, a }]; + }); + if (targets.length === 0) return { desired, errors: [] }; + // Fetch each group's (ruleset, status) concurrently — 2N serial round-trips otherwise dominate + // plan/apply latency on a config with many dynamic groups. Within a group the two GETs stay + // sequential: the status GET must run only after the ruleset GET succeeds (a 404 there means + // "not a dynamic group" and short-circuits). Per-group error strings are collected in input + // order so the plan-degradation output is deterministic regardless of completion order. + const perGroupErrors = await mapConcurrent(targets, DYNAMIC_FETCH_CONCURRENCY, async ({ managed, a }) => { // The ruleset GET and the status GET have distinct failure meanings, so they get distinct // try/catch blocks: only a ruleset 404 means "not a dynamic group". A status GET that fails // AFTER a successful ruleset GET must NOT fabricate the "none" sentinel (that would discard a @@ -102,11 +140,10 @@ const dynamicField: SyntheticField = { // Group exists but is not (yet) a dynamic group — its ruleset 404s. Sentinel so a promote // (desired active vs actual none) diffs as a real change and demote-to-none is a clean no-op. a.dynamic = { status: "none", ruleset: {} }; - continue; + return []; } const message = err instanceof Error ? err.message : String(err); - errors.push(`dynamic ${managed.key} (#${managed.id}): ${message}`); - continue; + return [`dynamic ${managed.key} (#${managed.id}): ${message}`]; } try { const statusRes = await client.get<{ dynamicGroupStatus?: string }>(`/dynamicgroups/${managed.id}/status`); @@ -114,11 +151,13 @@ const dynamicField: SyntheticField = { status: (statusRes?.dynamicGroupStatus ?? "none") as DynamicStatus, ruleset: normalizeRuleset(ruleset), }; + return []; } catch (err) { const message = err instanceof Error ? err.message : String(err); - errors.push(`dynamic ${managed.key} status (#${managed.id}): ${message}`); + return [`dynamic ${managed.key} status (#${managed.id}): ${message}`]; } - } + }); + const errors = perGroupErrors.flat(); const augmented = desired.map((d) => { if (d.type !== "group" || d.dynamic === undefined) return d; // Demote-to-none: fold to the SAME sentinel the actual side uses for a non-dynamic group @@ -135,6 +174,7 @@ const dynamicField: SyntheticField = { }, async apply({ client, id, change }) { const to = change.to as { status: DynamicStatus; ruleset: Record } | undefined; + const from = change.from as { status?: DynamicStatus; ruleset?: Record } | undefined; if (!to || to.status === "none") { assertNotPeople(`/dynamicgroups/${id}/ruleset`); // A group that was never dynamic (or is already demoted) has no ruleset to delete — CT 404s. @@ -148,11 +188,34 @@ const dynamicField: SyntheticField = { await client.request("PUT", `/dynamicgroups/${id}/status`, { dynamicGroupStatus: "none" }); return; } - assertNotPeople(`/dynamicgroups/${id}/ruleset`); - await client.request("PUT", `/dynamicgroups/${id}/ruleset`, { dynamicGroupRuleSet: to.ruleset }); + // A pure status flip (active↔inactive) leaves the ruleset byte-identical — skip the re-PUT so we + // don't rewrite an unchanged ruleset (wasteful, and may trigger a server-side recalculation). A + // fresh promote (`from` undefined / previously non-dynamic) has no comparable ruleset, so PUT it. + const rulesetChanged = from?.ruleset === undefined || !deepEqual(from.ruleset, to.ruleset); + if (rulesetChanged) { + assertNotPeople(`/dynamicgroups/${id}/ruleset`); + await client.request("PUT", `/dynamicgroups/${id}/ruleset`, { dynamicGroupRuleSet: to.ruleset }); + } assertNotPeople(`/dynamicgroups/${id}/status`); await client.request("PUT", `/dynamicgroups/${id}/status`, { dynamicGroupStatus: to.status }); }, + async postApply({ client, id, item, change }) { + // `ct apply --refresh`: materialize computed membership for a changed dynamic group. Per-group + // only — the all-groups /dynamicgroups/refresh endpoint has a huge blast radius and is never + // called from here. Owns the demote-sentinel knowledge so the command layer stays field-agnostic. + const to = change.to as { status?: string } | undefined; + if (to?.status === "none") return; // demoted to a non-dynamic group — nothing to refresh + const path = `/dynamicgroups/${id}/refresh`; + assertNotPeople(path); + try { + const res = await client.request("POST", path); + const r = res?.[0]; + if (r) info(`refreshed ${item.key}: +${r.created} ~${r.updated} -${r.deleted}`); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + warn(`Failed to refresh ${item.key} (#${id}): ${message}`); + } + }, }; export const SYNTHETIC_FIELDS: SyntheticField[] = [parentsField, dynamicField]; @@ -165,6 +228,30 @@ export function syntheticField(field: string): SyntheticField | undefined { return BY_FIELD.get(field); } +/** + * Drive every synthetic field's optional `postApply` hook over an applied plan (e.g. the opt-in + * `ct apply --refresh` dynamic-group refresh). Runs after `executePlan`, so ids are read from the + * POST-execute state — a create already carries its real id. Skips no-op/delete items and any item + * whose key is not (yet) resolvable in state (explicit `undefined` check — CT ids can be `0`). Each + * hook swallows its own errors, so one field/group failing never blocks the rest. + */ +export async function runPostApplyHooks( + plan: Plan, + state: State, + client: Pick, +): Promise { + for (const item of plan.items) { + if (item.action === "no-op" || item.action === "delete") continue; + for (const change of item.changes) { + const f = syntheticField(change.field); + if (!f?.postApply) continue; + const id = state.resources[item.key]?.id; + if (id === undefined) continue; + await f.postApply({ client, state, id, item, change }); + } + } +} + /** Run every registered fold in order, threading the (immutably) augmented desired through each. */ export async function foldSynthetic( ctx: SyntheticFoldCtx, diff --git a/src/permissions/apply.ts b/src/permissions/apply.ts index cabbd36..d770be4 100644 --- a/src/permissions/apply.ts +++ b/src/permissions/apply.ts @@ -7,10 +7,14 @@ import type { CtClient } from "../api/ctClient.js"; import type { State } from "../state/state.js"; import { assertNotPeople } from "../engine/guard.js"; +import { mapConcurrent } from "../util/concurrency.js"; import type { PermissionPlanItem } from "./plan.js"; import type { GrantTuple } from "./grants.js"; import { reresolveTuple } from "./scope.js"; +/** How many permission tuples to write at once. Tuples are independent rows, so a modest fan-out is safe. */ +const WRITE_CONCURRENCY = 6; + function body(t: GrantTuple): Record { if (t.pending) { // A pending tuple's dataId is unknown until it is re-resolved against post-execute state. @@ -22,30 +26,82 @@ function body(t: GrantTuple): Record { return b; } +/** One tuple write, flattened out of the per-domain diff so all writes share one concurrency pool. */ +interface WriteOp { + method: "PUT" | "DELETE"; + path: string; + tuple: GrantTuple; +} + +export interface FailedWrite { + method: "PUT" | "DELETE"; + path: string; + authId: number; + dataId: number[]; + message: string; +} + +export interface PermissionApplyResult { + granted: number; + deleted: number; + /** Tuples whose write threw, so the caller can report a clean resumable summary (#35 item 14). */ + failed: FailedWrite[]; +} + /** * Apply a permission plan. When `state` is provided, each scoped grant's dataId is RE-RESOLVED * against it just before the PUT — `state` here is the POST-execute state (executePlan has upserted * every created/recreated group), so grants are always written with fresh ids and a group created in * the same apply gets its real id (#29, #33.3). Without `state`, tuples are written as-is. + * + * Writes fan out at {@link WRITE_CONCURRENCY} (independent rows). Result counts are collected in the + * flattened op order — deterministic regardless of completion order — and a write that throws is + * captured in `failed` instead of aborting the batch, so the command can print a resumable summary. */ export async function applyPermissionPlan( items: PermissionPlanItem[], client: Pick, state?: State, -): Promise<{ granted: number; deleted: number }> { - let granted = 0; - let deleted = 0; +): Promise { + const ops: WriteOp[] = []; + // Concurrent writes are race-free only because evaluateConfig rejects two declarations targeting + // the same (domainType, domainId) — so all ops for a path come from ONE item's disjoint diff. + // Programmatic callers bypassing evaluateConfig must uphold that invariant themselves. for (const item of items) { const path = `/permissions/${item.domainType}/${item.domainId}`; assertNotPeople(path); - for (const t of item.diff.toPut) { - await client.request("PUT", path, body(state ? reresolveTuple(t, state) : t)); - granted++; + for (const t of item.diff.toPut) ops.push({ method: "PUT", path, tuple: t }); + for (const t of item.diff.toDelete) ops.push({ method: "DELETE", path, tuple: t }); + } + + const outcomes = await mapConcurrent(ops, WRITE_CONCURRENCY, async (op) => { + try { + // toPut tuples may be scoped/pending → re-resolve against post-execute state; toDelete tuples + // come from actuals (never pending), so pass them through. `body()` guards a stray pending row. + const b = op.method === "PUT" && state ? body(reresolveTuple(op.tuple, state)) : body(op.tuple); + await client.request(op.method, op.path, b); + return { ok: true as const, op }; + } catch (err) { + return { ok: false as const, op, message: err instanceof Error ? err.message : String(err) }; } - for (const t of item.diff.toDelete) { - await client.request("DELETE", path, body(t)); - deleted++; + }); + + let granted = 0; + let deleted = 0; + const failed: FailedWrite[] = []; + for (const o of outcomes) { + if (o.ok) { + if (o.op.method === "PUT") granted++; + else deleted++; + } else { + failed.push({ + method: o.op.method, + path: o.op.path, + authId: o.op.tuple.authId, + dataId: o.op.tuple.dataId, + message: o.message, + }); } } - return { granted, deleted }; + return { granted, deleted, failed }; } diff --git a/src/permissions/catalog.ts b/src/permissions/catalog.ts index e49b352..ccfd14a 100644 --- a/src/permissions/catalog.ts +++ b/src/permissions/catalog.ts @@ -11,17 +11,18 @@ import catalogData from "./catalog.json" with { type: "json" }; export interface CatalogEntry { authId: number; scopeField: string | null; revocable: boolean; desc: string } -const catalog = catalogData as Record; - -export function loadCatalog(): Record { - return catalog; -} +/** + * The permission catalog: name → authId bridge, inlined at build time (see the module header). + * It is a constant, not something "loaded" — callers that need a snapshot already spread it, so it + * is exported directly rather than behind a `loadCatalog()` wrapper. + */ +export const CATALOG = catalogData as Record; export function resolveAuthId(name: string): CatalogEntry { - const entry = loadCatalog()[name]; + const entry = CATALOG[name]; if (!entry) { const [mod] = name.split(":"); - const near = Object.keys(loadCatalog()).filter((k) => k.startsWith(`${mod}:`)).slice(0, 6); + const near = Object.keys(CATALOG).filter((k) => k.startsWith(`${mod}:`)).slice(0, 6); const hint = near.length ? ` Did you mean one of: ${near.join(", ")}?` : ""; throw new Error(`Unknown permission "${name}".${hint}`); } diff --git a/src/permissions/grants.ts b/src/permissions/grants.ts index 3f00520..2424749 100644 --- a/src/permissions/grants.ts +++ b/src/permissions/grants.ts @@ -45,8 +45,9 @@ export function normalizeActual(rows: RawPermission[]): GrantTuple[] { for (const r of rows) { if (r.meta?.modifiedPid === -1) continue; // system baseline — invisible to reconciliation if (r.isInherited) continue; // inherited — not directly owned here - const dataId = r.dataId == null ? [] : [r.dataId]; - out.push({ authId: r.authId, dataId: dataId.sort((a, b) => a - b), type: r.type }); + // dataId is [] or a single element (CT reads scoped grants back one row per dataId), so there is + // nothing to sort here — and tupleKey sorts defensively anyway when it builds the identity key. + out.push({ authId: r.authId, dataId: r.dataId == null ? [] : [r.dataId], type: r.type }); } return out; } diff --git a/src/resources/registry.ts b/src/resources/registry.ts index d16bbbf..9d74374 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -14,6 +14,12 @@ export interface AdoptableResource { itemPath: (id: number) => string; /** Update verb: `group` is PATCH; every other type is PUT. */ updateMethod: "PUT" | "PATCH"; + /** + * Apply tier: lower applies first, delete runs highest first (see engine/graph.ts). Owned here so + * a new resource type declares its ordering in the same place as its paths — `engine/graph.ts` + * derives `TYPE_TIER` from these entries instead of maintaining a parallel (drift-prone) table. + */ + tier: number; /** Stable logical key derived from the fetched resource. */ deriveKey: (resource: Record) => string; /** The subset of fields we manage — the desired-state baseline. */ @@ -60,6 +66,7 @@ export const RESOURCES: Record = { campus: define({ collectionPath: "/campuses", updateMethod: "PUT", + tier: 0, // CT's campus short name is `shorty` (1–10 chars, required on create) — verified // live. `shortName` is a vestigial, usually-null sibling; do not use it for writes. deriveKey: (r) => slug(str(r, "shorty") || str(r, "name")), @@ -68,6 +75,7 @@ export const RESOURCES: Record = { group: define({ collectionPath: "/groups", updateMethod: "PATCH", + tier: 1, deriveKey: (r) => slug(str(r, "name")), managedFields: (r) => ({ name: r.name, @@ -78,24 +86,28 @@ export const RESOURCES: Record = { "group-type": define({ collectionPath: "/group/grouptypes", updateMethod: "PUT", + tier: 0, deriveKey: (r) => slug(str(r, "name")), managedFields: (r) => ({ name: r.name, nameTranslated: r.nameTranslated }), }), "age-group": define({ collectionPath: "/group/agegroups", updateMethod: "PUT", + tier: 0, deriveKey: (r) => slug(str(r, "name")), managedFields: (r) => ({ name: r.name, nameTranslated: r.nameTranslated, sortKey: r.sortKey }), }), "target-group": define({ collectionPath: "/group/targetgroups", updateMethod: "PUT", + tier: 0, deriveKey: (r) => slug(str(r, "name")), managedFields: (r) => ({ name: r.name, nameTranslated: r.nameTranslated, sortKey: r.sortKey }), }), "relationship-type": define({ collectionPath: "/person/relationshiptypes", updateMethod: "PUT", + tier: 0, deriveKey: (r) => slug(str(r, "name")), // CT names the two ends degreeNameA/degreeNameB (verified live) — not degreeForward/Reverse. managedFields: (r) => ({ @@ -108,6 +120,7 @@ export const RESOURCES: Record = { "group-role": define({ collectionPath: "/group/roles", updateMethod: "PUT", + tier: 3, deriveKey: (r) => slug(str(r, "name")), managedFields: (r) => ({ name: r.name, nameTranslated: r.nameTranslated, groupTypeId: r.groupTypeId }), // `groupRole` is taken by the permissions DSL (`ct.groupRole` = definePermission("group_role")), diff --git a/tests/apply-refresh.test.ts b/tests/apply-refresh.test.ts index dd387eb..6f60d31 100644 --- a/tests/apply-refresh.test.ts +++ b/tests/apply-refresh.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect } from "vitest"; -import { refreshChangedDynamicGroups } from "../src/commands/apply.js"; +// The dynamic-group refresh is a `postApply` hook on the `dynamic` synthetic field now, driven +// generically by `runPostApplyHooks` — the command layer no longer hardcodes the field/sentinel. +import { runPostApplyHooks } from "../src/engine/synthetic.js"; import { emptyState, type State } from "../src/state/state.js"; import type { Plan } from "../src/engine/types.js"; @@ -36,7 +38,7 @@ function stateWith(entries: Record): State { return state; } -describe("refreshChangedDynamicGroups", () => { +describe("runPostApplyHooks (dynamic refresh)", () => { it("POSTs /dynamicgroups/{id}/refresh once for each item whose changes include a dynamic field", async () => { const state = stateWith({ dyn_a: 42, dyn_b: 43, plain: 44 }); const plan: Plan = { @@ -74,7 +76,7 @@ describe("refreshChangedDynamicGroups", () => { "POST /dynamicgroups/43/refresh": [{ created: 0, updated: 1, deleted: 1 }], }); - await refreshChangedDynamicGroups(plan, state, client); + await runPostApplyHooks(plan, state, client); const refreshCalls = calls.filter((c) => c.path.startsWith("/dynamicgroups/")); expect(refreshCalls).toHaveLength(2); @@ -96,7 +98,7 @@ describe("refreshChangedDynamicGroups", () => { ], }; const { client, calls } = recorder(); - await refreshChangedDynamicGroups(plan, state, client); + await runPostApplyHooks(plan, state, client); expect(calls.some((c) => c.path === "/dynamicgroups/refresh")).toBe(false); }); @@ -114,7 +116,7 @@ describe("refreshChangedDynamicGroups", () => { ], }; const { client, calls } = recorder(); - await refreshChangedDynamicGroups(plan, state, client); + await runPostApplyHooks(plan, state, client); expect(calls).toHaveLength(0); }); @@ -141,7 +143,7 @@ describe("refreshChangedDynamicGroups", () => { const { client, calls } = recorder({ "POST /dynamicgroups/43/refresh": [{ created: 1, updated: 0, deleted: 0 }], }); - await refreshChangedDynamicGroups(plan, state, client); + await runPostApplyHooks(plan, state, client); const refreshCalls = calls.filter((c) => c.path.startsWith("/dynamicgroups/")); expect(refreshCalls).toEqual([{ method: "POST", path: "/dynamicgroups/43/refresh", body: undefined }]); }); @@ -174,7 +176,7 @@ describe("refreshChangedDynamicGroups", () => { return [{ created: 1, updated: 0, deleted: 0 }] as T; }, }; - await expect(refreshChangedDynamicGroups(plan, state, client)).resolves.toBeUndefined(); + await expect(runPostApplyHooks(plan, state, client)).resolves.toBeUndefined(); const refreshCalls = calls.filter((c) => c.path.startsWith("/dynamicgroups/")); expect(refreshCalls).toHaveLength(2); expect(refreshCalls).toContainEqual({ method: "POST", path: "/dynamicgroups/42/refresh", body: undefined }); @@ -203,7 +205,7 @@ describe("refreshChangedDynamicGroups", () => { ], }; const { client, calls } = recorder({ "POST /dynamicgroups/0/refresh": [{ created: 0, updated: 0, deleted: 0 }] }); - await refreshChangedDynamicGroups(plan, state, client); + await runPostApplyHooks(plan, state, client); expect(calls).toEqual([{ method: "POST", path: "/dynamicgroups/0/refresh", body: undefined }]); }); }); diff --git a/tests/context.test.ts b/tests/context.test.ts index cc2aa71..6edb105 100644 --- a/tests/context.test.ts +++ b/tests/context.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from "vitest"; import { createContext, evaluateConfig, type ConfigContext } from "../src/config/context.js"; import { isKnownType } from "../src/engine/graph.js"; +import { RESOURCES } from "../src/resources/registry.js"; describe("config context", () => { it("builds desired resources from DSL calls, separating key/parent from fields", () => { @@ -113,6 +114,20 @@ describe("config context", () => { } }); + it("every registry type has a matching ConfigContext resource method (registry ↔ context sync, #35 item 6)", () => { + // ConfigContext isn't derived from the registry, so lock the two in sync: each registry entry's + // DSL name (its `dslName`, else the camelCase of the type) must be a callable ct method — a + // renamed/added type without its DSL surface fails here instead of at config-load time. + const { ct } = createContext(); + const camel = (t: string): string => t.replace(/-([a-z])/g, (_, c: string) => c.toUpperCase()); + for (const [type, spec] of Object.entries(RESOURCES)) { + const fn = spec.dslName ?? camel(type); + expect(typeof (ct as unknown as Record)[fn], `registry type "${type}" expects ct.${fn}()`).toBe( + "function", + ); + } + }); + it("declares the master-data group role as a plannable group-role resource (not the permission surface)", () => { const { ct, resources, permissions } = createContext(); ct.roleDefinition({ key: "leiter", name: "Leiter", groupTypeId: 2 }); diff --git a/tests/graph.test.ts b/tests/graph.test.ts index b323390..488d0c8 100644 --- a/tests/graph.test.ts +++ b/tests/graph.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "vitest"; -import { orderKeys, tierOf } from "../src/engine/graph.js"; +import { orderKeys, tierOf, isKnownType, TYPE_TIER } from "../src/engine/graph.js"; +import { RESOURCES } from "../src/resources/registry.js"; import type { DesiredResource } from "../src/engine/types.js"; function res(type: string, key: string, deps: string[] = [], parent?: string): DesiredResource { @@ -8,11 +9,23 @@ function res(type: string, key: string, deps: string[] = [], parent?: string): D describe("tierOf", () => { it("ranks metadata below groups below the things that reference groups", () => { + // Tiers are derived from the resource registry now; only real DesiredResource types have one. expect(tierOf("campus")).toBeLessThan(tierOf("group")); - expect(tierOf("group")).toBeLessThan(tierOf("group-hierarchy")); - expect(tierOf("permission")).toBeLessThan(tierOf("dynamic-group")); + expect(tierOf("group-type")).toBeLessThan(tierOf("group")); + expect(tierOf("group")).toBeLessThan(tierOf("group-role")); expect(tierOf("unknown")).toBe(0); }); + + it("derives TYPE_TIER from the resource registry — exactly the registry types, no phantoms (#35 item 6)", () => { + // The old hand-maintained table carried phantom types (group-status, group-hierarchy, permission, + // dynamic-group) that are not DesiredResource types. Derivation keeps the two in lockstep. + expect(Object.keys(TYPE_TIER).sort()).toEqual(Object.keys(RESOURCES).sort()); + for (const [type, spec] of Object.entries(RESOURCES)) { + expect(TYPE_TIER[type]).toBe(spec.tier); + expect(isKnownType(type)).toBe(true); + } + expect(isKnownType("group-status")).toBe(false); // phantom removed + }); }); describe("orderKeys", () => { diff --git a/tests/permission-apply.test.ts b/tests/permission-apply.test.ts index 32400f0..9dde5c7 100644 --- a/tests/permission-apply.test.ts +++ b/tests/permission-apply.test.ts @@ -12,9 +12,36 @@ describe("applyPermissionPlan", () => { preserved: [], }, }], { request } as never); - expect(res).toEqual({ granted: 2, deleted: 1 }); + expect(res).toEqual({ granted: 2, deleted: 1, failed: [] }); expect(request).toHaveBeenCalledWith("PUT", "/permissions/group_type_role/8", { authId: 1104, dataId: [42], type: "grant" }); expect(request).toHaveBeenCalledWith("PUT", "/permissions/group_type_role/8", { authId: 1101, type: "grant" }); // no dataId when unscoped expect(request).toHaveBeenCalledWith("DELETE", "/permissions/group_type_role/8", { authId: 2000, type: "grant" }); }); + + it("collects a failed write instead of aborting the batch, and keeps writing the rest (#35 items 3+14)", async () => { + // With concurrency, a mid-batch throw must not stop the others — it is captured in `failed` so the + // command can print a clean resumable summary rather than a raw stack. + const request = vi.fn(async (_m: string, _p: string, b: Record) => { + if (b.authId === 1104) throw new Error("boom"); + return {}; + }); + const res = await applyPermissionPlan([{ + key: "t", domainType: "group_type_role", domainId: 8, + diff: { + toPut: [ + { authId: 1104, dataId: [42], type: "grant" }, + { authId: 1101, dataId: [], type: "grant" }, + ], + toDelete: [{ authId: 2000, dataId: [], type: "grant" }], + preserved: [], + }, + }], { request } as never); + expect(res.granted).toBe(1); // only the 1101 PUT succeeded + expect(res.deleted).toBe(1); + expect(res.failed).toEqual([ + { method: "PUT", path: "/permissions/group_type_role/8", authId: 1104, dataId: [42], message: "boom" }, + ]); + // Every op was still attempted despite the failure. + expect(request).toHaveBeenCalledTimes(3); + }); }); diff --git a/tests/permission-catalog.test.ts b/tests/permission-catalog.test.ts index 8a28a19..9daba04 100644 --- a/tests/permission-catalog.test.ts +++ b/tests/permission-catalog.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { resolveAuthId, loadCatalog } from "../src/permissions/catalog.js"; +import { resolveAuthId, CATALOG } from "../src/permissions/catalog.js"; describe("permission catalog", () => { it("resolves a known global right to its authId", () => { @@ -15,7 +15,7 @@ describe("permission catalog", () => { it("throws a helpful error for an unknown right", () => { expect(() => resolveAuthId("churchgroup:no such right")).toThrow(/unknown permission "churchgroup:no such right"/i); }); - it("loads the whole catalog (187 rights)", () => { - expect(Object.keys(loadCatalog()).length).toBeGreaterThanOrEqual(180); + it("exposes the whole catalog (187 rights)", () => { + expect(Object.keys(CATALOG).length).toBeGreaterThanOrEqual(180); }); }); diff --git a/tests/permission.integration.test.ts b/tests/permission.integration.test.ts index 6361086..3615d6c 100644 --- a/tests/permission.integration.test.ts +++ b/tests/permission.integration.test.ts @@ -17,7 +17,7 @@ import { authedSession } from "../src/api/session.js"; import { buildPermissionPlan } from "../src/permissions/plan.js"; import { applyPermissionPlan } from "../src/permissions/apply.js"; import { normalizeActual, type RawPermission } from "../src/permissions/grants.js"; -import { loadCatalog } from "../src/permissions/catalog.js"; +import { CATALOG } from "../src/permissions/catalog.js"; import type { State, ManagedResource } from "../src/state/state.js"; import type { DesiredPermission, Grant } from "../src/permissions/types.js"; @@ -28,7 +28,7 @@ const DOMAIN_ID = Number(process.env.CT_PERM_FIXTURE_ID ?? "0"); // a disposable /** authId → catalog name, for reversing actual grants back into declarable rights. */ function reverseCatalog(): Map { const byAuthId = new Map(); - for (const [name, entry] of Object.entries(loadCatalog())) { + for (const [name, entry] of Object.entries(CATALOG)) { if (!byAuthId.has(entry.authId)) byAuthId.set(entry.authId, name); } return byAuthId; diff --git a/tests/plan.test.ts b/tests/plan.test.ts index 97da59d..8209340 100644 --- a/tests/plan.test.ts +++ b/tests/plan.test.ts @@ -75,6 +75,18 @@ describe("computePlan", () => { ).toThrow(/Unknown resource type/); }); + it("throws on duplicate desired keys instead of silently last-wins (#35 item 7)", () => { + // A raw array from a programmatic caller (import command, test harness) could carry duplicates — + // desiredByKey would collapse them while the plan loop emits both. Reject up front. + expect(() => + computePlan( + [desired("dup", { name: "A" }), desired("dup", { name: "B" })], + stateOf(), + new Map(), + ), + ).toThrow(/Duplicate desired key "dup"/); + }); + it("plans an update with just the changed fields", () => { const plan = computePlan( [desired("mainz", { name: "Mainz HQ", shortName: "MZ" })], diff --git a/tests/synthetic-dynamic.test.ts b/tests/synthetic-dynamic.test.ts index e9021b0..8f1af8b 100644 --- a/tests/synthetic-dynamic.test.ts +++ b/tests/synthetic-dynamic.test.ts @@ -94,6 +94,35 @@ describe("dynamic synthetic field — fold", () => { expect(diffFields(out.desired[0]!.fields, actual.get("g")!).find((c) => c.field === "dynamic")).toBeUndefined(); }); + it("folds many opted-in groups concurrently, each with its own (ruleset, status) pair (#35 item 1)", async () => { + // Behavior must be identical to the old serial loop: every opted-in managed group gets its actual + // dynamic filled from its own ruleset+status GETs, keyed by group id. + const state: State = { version: 1, host: "h", resources: { + g1: { type: "group", id: 1, key: "g1", fields: { name: "G1" }, adoptedAt: "t", updatedAt: "t" }, + g2: { type: "group", id: 2, key: "g2", fields: { name: "G2" }, adoptedAt: "t", updatedAt: "t" }, + g3: { type: "group", id: 3, key: "g3", fields: { name: "G3" }, adoptedAt: "t", updatedAt: "t" }, + } }; + const actual = new Map>([ + ["g1", { name: "G1" }], ["g2", { name: "G2" }], ["g3", { name: "G3" }], + ]); + const desired: DesiredResource[] = ["g1", "g2", "g3"].map((key) => ({ + type: "group", key, fields: { name: key.toUpperCase() }, dependsOn: [], + dynamic: { status: "manual", ruleset: { description: key, query: {}, process: {} } }, + })); + const client = { get: vi.fn(async (p: string) => { + const id = p.match(/dynamicgroups\/(\d+)\//)![1]; + return p.endsWith("/ruleset") + ? { description: `rs${id}`, query: {}, process: {} } + : { dynamicGroupStatus: "manual" }; + }) }; + const out = await dynamicField().fold({ client: getClient(client), state, desired, actual }); + expect(out.errors).toEqual([]); + expect(actual.get("g1")?.dynamic).toEqual({ status: "manual", ruleset: { description: "rs1", query: {}, process: {} } }); + expect(actual.get("g2")?.dynamic).toEqual({ status: "manual", ruleset: { description: "rs2", query: {}, process: {} } }); + expect(actual.get("g3")?.dynamic).toEqual({ status: "manual", ruleset: { description: "rs3", query: {}, process: {} } }); + expect(client.get).toHaveBeenCalledTimes(6); // ruleset + status per group + }); + it("ignores groups that did not opt into dynamic", async () => { const state: State = { version: 1, host: "h", resources: { g: { type: "group", id: 5, key: "g", fields: { name: "G" }, adoptedAt: "t", updatedAt: "t" } } }; @@ -118,6 +147,31 @@ describe("dynamic synthetic field — apply", () => { expect(request).toHaveBeenNthCalledWith(2, "PUT", "/dynamicgroups/5/status", { dynamicGroupStatus: "active" }); }); + it("status-only change (ruleset byte-identical) PUTs only the status, skipping the ruleset re-PUT (#35 item 15)", async () => { + const request = vi.fn(async () => ({})); + const state: State = { version: 1, host: "h", resources: {} }; + const ruleset = { description: "x", query: {}, process: {} }; + await dynamicField().apply({ client: { request } as unknown as Pick, state, id: 5, + change: { field: "dynamic", + from: { status: "active", ruleset }, // same ruleset, only the status flips + to: { status: "inactive", ruleset } } }); + expect(request).toHaveBeenCalledTimes(1); + expect(request).toHaveBeenCalledWith("PUT", "/dynamicgroups/5/status", { dynamicGroupStatus: "inactive" }); + expect(request).not.toHaveBeenCalledWith("PUT", "/dynamicgroups/5/ruleset", expect.anything()); + }); + + it("still PUTs both when the ruleset changed alongside the status", async () => { + const request = vi.fn(async () => ({})); + const state: State = { version: 1, host: "h", resources: {} }; + await dynamicField().apply({ client: { request } as unknown as Pick, state, id: 5, + change: { field: "dynamic", + from: { status: "active", ruleset: { description: "old", query: {}, process: {} } }, + to: { status: "active", ruleset: { description: "new", query: {}, process: {} } } } }); + expect(request).toHaveBeenNthCalledWith(1, "PUT", "/dynamicgroups/5/ruleset", + { dynamicGroupRuleSet: { description: "new", query: {}, process: {} } }); + expect(request).toHaveBeenNthCalledWith(2, "PUT", "/dynamicgroups/5/status", { dynamicGroupStatus: "active" }); + }); + it("demotes to a normal group when status is none: DELETE ruleset then status none", async () => { const request = vi.fn(async () => ({})); const state: State = { version: 1, host: "h", resources: {} }; diff --git a/tests/tokenStore-cache.test.ts b/tests/tokenStore-cache.test.ts new file mode 100644 index 0000000..e163d05 --- /dev/null +++ b/tests/tokenStore-cache.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// A single command run resolves the host (resolveConfig → readStoredHost) AND the token +// (authedSession → readCredentials), each reaching for the same Keychain entry. Without memoization +// that spawns `security find-generic-password` up to 3× (and prompts to unlock a locked Keychain +// every time). Mock the platform to macOS and the `security` spawn so we can count the reads. +const execFileMock = vi.hoisted(() => vi.fn()); +vi.mock("node:child_process", () => ({ execFile: execFileMock })); +vi.mock("node:os", () => ({ platform: () => "darwin" })); + +import { readCredentials, readStoredHost, resetKeychainCache } from "../src/auth/tokenStore.js"; + +beforeEach(() => { + execFileMock.mockReset(); + resetKeychainCache(); + // promisify(execFile) over this mock resolves with whatever we pass as the callback's value arg, + // so return the { stdout } shape the real execFile promise yields. + execFileMock.mockImplementation((_cmd: string, _args: string[], cb: (e: unknown, v: unknown) => void) => + cb(null, { stdout: '{"host":"https://x.church.tools","token":"tok"}', stderr: "" }), + ); +}); + +describe("keychain read is memoized per process (#35 item 4)", () => { + it("spawns `security` only once across multiple credential reads", async () => { + const a = await readCredentials(); + const host = await readStoredHost(); + const b = await readCredentials(); + expect(a).toEqual({ host: "https://x.church.tools", token: "tok" }); + expect(b).toEqual(a); + expect(host).toBe("https://x.church.tools"); + expect(execFileMock).toHaveBeenCalledTimes(1); // memoized: 3 logical reads → 1 spawn + }); + + it("resetKeychainCache forces the next read to re-spawn (write invalidation seam)", async () => { + await readCredentials(); + expect(execFileMock).toHaveBeenCalledTimes(1); + resetKeychainCache(); + await readCredentials(); + expect(execFileMock).toHaveBeenCalledTimes(2); + }); +});