From 23545b1a0be9e72af038668d3db8a6a655eed0b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 19:43:57 +0000 Subject: [PATCH 1/2] feat(demo): staff the demo org so position-based sharing and approvals resolve to real people (#640) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a fresh install exactly one user exists, `demo_bootstrap` claims every seeded record for them, and `sys_user_position` is empty. Measured on 17.0.0-rc.1: `sys_record_share` 0 rows, so all nine declared sharing rules granted nobody anything, and `opportunity_approval`'s `manager_review` opened with an empty approver slate while `lockRecord` held the record. `pnpm demo:staff` creates three non-admin demo users on a LOCAL dev server (NA rep, EU rep, sales manager), assigns their positions and re-evaluates every sharing rule so already-seeded records materialise grants. Who exists is a table — `src/sharing/demo-staffing.ts`; adding a person is adding a row. Deliberately a script, not metadata: nothing in the published artifact may be able to create a user, so synthetic accounts cannot reach a customer org. `test/demo-staffing.test.ts` fails if a seed dataset or a flow node ever writes `sys_user` / `sys_member` / `sys_user_position`, and the built artifact carries none of these addresses. Users are created through better-auth's admin endpoint, so they are real, loginable accounts — identity tables are `managedBy: 'better-auth'` and a row inserted around that surface would have no credential. Re-evaluating the rules is load-bearing: plugin-sharing materialises grants from a record-write hook that returns early on `isSystem` writes, and every seeded row is written with `isSystem: true`. Fixes #640. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019SS7C5SXpniKeCApxgARyf --- .changeset/staff-the-demo-org.md | 44 ++++ docs/MAINTENANCE.md | 48 +++++ package.json | 3 +- scripts/demo-staff.ts | 325 +++++++++++++++++++++++++++++ src/flows/demo-bootstrap.flow.ts | 28 +++ src/sharing/demo-staffing.ts | 140 +++++++++++++ test/demo-staffing.test.ts | 338 +++++++++++++++++++++++++++++++ 7 files changed, 925 insertions(+), 1 deletion(-) create mode 100644 .changeset/staff-the-demo-org.md create mode 100644 scripts/demo-staff.ts create mode 100644 src/sharing/demo-staffing.ts create mode 100644 test/demo-staffing.test.ts diff --git a/.changeset/staff-the-demo-org.md b/.changeset/staff-the-demo-org.md new file mode 100644 index 00000000..f74548d2 --- /dev/null +++ b/.changeset/staff-the-demo-org.md @@ -0,0 +1,44 @@ +--- +'hotcrm': patch +--- + +Give the demo org people, so the position-based mechanisms this app ships stop +resolving to an empty recipient set. A new `pnpm demo:staff` command creates +three non-admin demo users on a local dev server — an NA rep, an EU rep and a +sales manager — assigns the positions they hold, and re-evaluates the sharing +rules so the already-seeded records materialise grants. + +This was the last dark layer of the same gap #621 and #638 closed from the other +two sides. The rules installed and the records matched them, but nobody held any +position, so a matching account still granted nothing: on a fresh install +`sys_user_position` had 0 rows and `sys_record_share` 0 rows, every +position-based sharing rule granted nobody anything, and submitting a deal for +approval opened `opportunity_approval`'s `manager_review` with an empty approver +slate while `lockRecord` held the record with no in-product recovery. After +staffing, the same fresh install shows `north_america_territory` granting its 6 +accounts and `europe_territory` its 2, the NA rep reading exactly the six US/CA +accounts she does not own (and neither the two EU ones nor the one account in no +territory), and `manager_review` routing to a real approver. + +Who exists and which positions they hold is a table +(`src/sharing/demo-staffing.ts`) — adding a person is adding a row. The two reps +must be users who do NOT own the accounts, because `crm_account` is `private` +and the OWD baseline already admits a record's owner, so a share to the owner +would prove nothing; ownership stays with `demo_bootstrap`'s first user and the +script exits non-zero if that ever stops being true. The other seven positions +stay unstaffed on purpose: a real deployment staffs its own people. + +**These accounts can never reach a customer org.** Staffing is a repo script +that drives a local dev server through the platform's own admin endpoints, not +metadata: nothing in the published artifact can create a user, and +`test/demo-staffing.test.ts` fails if a seed dataset or a flow node ever writes +`sys_user`, `sys_member` or `sys_user_position`. (It could not have worked as +metadata either — identity tables are `managedBy: 'better-auth'`, so a row +inserted around that surface has no credential and nobody can sign in as it.) + +One platform behaviour worth carrying forward, measured here: `plugin-sharing` +materialises rule grants from a record-write hook that returns early on +`isSystem` writes, and every seeded row is written with `isSystem: true`. So +staffing alone leaves `sys_record_share` empty until a rule is re-evaluated — +which the script does, and which a server restart also does via the boot +backfill. Fixes #640. Refs #621, #638, #622, #488. diff --git a/docs/MAINTENANCE.md b/docs/MAINTENANCE.md index 39d10a59..b29808e9 100644 --- a/docs/MAINTENANCE.md +++ b/docs/MAINTENANCE.md @@ -190,6 +190,54 @@ After any platform upgrade, or whenever Studio shows validation banners: 4. If a banner persists, fix the offending fixture in `src/data/`, not the designer or the platform. +### 4.1 Staffing the demo org (`pnpm demo:staff`) + +A reseeded org has records but no PEOPLE. On a fresh install exactly one user +exists (the dev admin), `demo_bootstrap` claims every seeded record for them, +and `sys_user_position` is empty — so every position-based sharing rule this app +ships grants nobody anything, and `opportunity_approval`'s `manager_review` node +opens with an empty approver slate while `lockRecord` holds the record ([#640]). + +```bash +pnpm dev # terminal 1 — leave running +pnpm demo:staff # terminal 2 — once, after the server is up +``` + +That creates three non-admin demo users (`na.rep@` / `eu.rep@` / +`sales.manager@objectos.ai`, all `demo1234`), assigns their positions, and +re-evaluates every sharing rule so the already-seeded accounts materialise +grants. It is idempotent, self-verifying (non-zero exit if the layers do not +connect) and prints what each user can see: + +``` +north_america_territory matched= 6 holders=1 granted=6 +europe_territory matched= 2 holders=1 granted=2 +na.rep@objectos.ai sees 6 account(s) · countries: [CA, US] +eu.rep@objectos.ai sees 2 account(s) · countries: [DE, UK] +``` + +Who exists and which positions they hold is a table — +[`src/sharing/demo-staffing.ts`](../src/sharing/demo-staffing.ts). Adding a +person is adding a row. + +Three things worth knowing before changing any of it: + +- **It is a script, not metadata, on purpose.** A real deployment must install + none of these accounts, so nothing in the published artifact may be able to + create a user. `test/demo-staffing.test.ts` fails if a seed dataset or a flow + node ever writes `sys_user` / `sys_member` / `sys_user_position`. +- **Re-evaluating the rules is not optional.** `plugin-sharing` materialises + grants from a record-write hook that returns early on `isSystem` writes, and + every seeded row is written with `isSystem: true`. Staffing alone therefore + leaves `sys_record_share` empty until a rule is re-evaluated (a server restart + does it too, via the boot backfill). +- **The reps must not own the accounts.** `crm_account` is `private`, so the OWD + baseline already admits a record's owner — a share to the owner demonstrates + nothing. Ownership stays with `demo_bootstrap`'s first user; the script exits + non-zero if a demo user turns out to own a seeded account. + +[#640]: https://github.com/objectstack-ai/hotcrm/issues/640 + ## 5. Releasing HotCRM ships as **one** app package (`hotcrm` / `app.objectstack.hotcrm`). Before diff --git a/package.json b/package.json index cebf935b..1949c467 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,8 @@ "changeset:status": "changeset status --since=origin/main", "publish:marketplace": "node scripts/publish-marketplace.mjs", "publish:marketplace:dry-run": "DRY_RUN=1 node scripts/publish-marketplace.mjs", - "demo:reset": "rm -rf .objectstack/data && pnpm build && echo '✅ DB reset — start the server with: pnpm dev (or pnpm start). Seed data loads on first boot.'" + "demo:reset": "rm -rf .objectstack/data && pnpm build && echo '✅ DB reset — start the server with: pnpm dev (or pnpm start). Seed data loads on first boot, then run: pnpm demo:staff'", + "demo:staff": "tsx scripts/demo-staff.ts" }, "packageManager": "pnpm@10.33.0", "dependencies": { diff --git a/scripts/demo-staff.ts b/scripts/demo-staff.ts new file mode 100644 index 00000000..69a8e8ab --- /dev/null +++ b/scripts/demo-staff.ts @@ -0,0 +1,325 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +// +// Demo-org staffing (#640) — give the demo org people, so the position-based +// mechanisms this app ships stop resolving to an empty recipient set. +// +// pnpm dev # terminal 1 — leave it running +// pnpm demo:staff # terminal 2 — once, against that server +// +// It drives a LOCAL, RUNNING dev server through the platform's own admin +// surfaces. Nothing here ships in the artifact: `objectstack.config.ts` never +// imports this file or the table it reads, which is what makes "a real +// deployment installs none of these people" structural rather than hopeful +// (see the header of `src/sharing/demo-staffing.ts`). +// +// Four steps, all idempotent — rerun it any time, including against a +// half-staffed org: +// +// 1. create each person in `DemoOrgStaffing` via +// POST /api/v1/auth/admin/create-user (better-auth: a REAL, loginable +// account with a credential and a `sys_member` row — not a raw `sys_user` +// insert, which ADR-0092's write guard refuses and which would produce an +// un-loginable row anyway); +// 2. assign the positions they hold (`sys_user_position`); +// 3. RE-EVALUATE every active sharing rule. This step is not optional and is +// the reason staffing alone was never enough: `plugin-sharing` materialises +// grants from a record-write hook that returns early on `isSystem` writes, +// and every seeded row is written with `isSystem: true`. So the seeded +// accounts carry no grants no matter who holds a position, until a rule is +// re-evaluated (boot backfill does it too — this just avoids the restart); +// 4. VERIFY, as each demo user, that the three layers actually connect, and +// exit non-zero if they do not. +// +// Flags: --url (default http://localhost:4001, the port `pnpm dev` binds), +// --admin-email / --admin-password (default the platform's dev-admin +// seed, which only exists when NODE_ENV=development). + +import { DemoOrgStaffing, type DemoStaffMember } from '../src/sharing/demo-staffing.js'; + +type Json = Record; + +const DEFAULT_URL = 'http://localhost:4001'; + +/** `--flag value` / `--flag=value`, else the env var, else the fallback. */ +function arg(name: string, envName: string, fallback: string): string { + const argv = process.argv.slice(2); + const eq = argv.find((a) => a.startsWith(`--${name}=`)); + if (eq) return eq.slice(name.length + 3); + const i = argv.indexOf(`--${name}`); + if (i !== -1 && argv[i + 1]) return argv[i + 1]; + return process.env[envName]?.trim() || fallback; +} + +/** + * Refuse anything that is not this machine. + * + * The staffing itself is already gated by needing dev-admin credentials, but a + * mistyped `--url` must fail with a sentence rather than start provisioning + * accounts somewhere real. Synthetic users in a customer org is the one + * outcome #640 rules out unconditionally. + */ +function assertLocal(url: string): URL { + const parsed = new URL(url); + const loopback = ['127.0.0.1', '[::1]', '::1', '0.0.0.0']; + if (parsed.hostname !== 'localhost' && !loopback.includes(parsed.hostname)) { + throw new Error( + `refusing to staff a non-local server (${parsed.hostname}). These are demo accounts with ` + + `well-known passwords; they belong on a developer's own dev server and nowhere else. ` + + `A real deployment staffs its own people through Setup → Users.`, + ); + } + // Loopback LITERALS are rewritten to `localhost`, which is not cosmetic: + // better-auth's default trusted-origin list is `http://localhost:*`, so + // `--url http://127.0.0.1:4001` answers every auth call with + // `403 INVALID_ORIGIN` (measured). Same machine, one spelling. + if (loopback.includes(parsed.hostname)) parsed.hostname = 'localhost'; + return parsed; +} + +class Api { + private cookie = ''; + constructor(private readonly base: URL) {} + + private async call(method: string, path: string, body?: Json): Promise<{ status: number; json: Json }> { + const res = await fetch(new URL(path, this.base), { + method, + headers: { + 'Content-Type': 'application/json', + // better-auth rejects a cross-origin-looking request with + // INVALID_ORIGIN; the server's own origin is always trusted. + Origin: this.base.origin, + ...(this.cookie ? { Cookie: this.cookie } : {}), + }, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + }); + const setCookie = res.headers.getSetCookie?.() ?? []; + if (setCookie.length) { + this.cookie = setCookie.map((c) => c.split(';')[0]).join('; '); + } + const text = await res.text(); + let json: Json = {}; + try { json = text ? JSON.parse(text) : {}; } catch { json = { raw: text }; } + return { status: res.status, json }; + } + + get(path: string) { return this.call('GET', path); } + post(path: string, body: Json = {}) { return this.call('POST', path, body); } + + /** POST, throwing with the server's own message when it is not a 2xx. */ + async postOk(path: string, body: Json = {}): Promise { + const { status, json } = await this.post(path, body); + if (status < 200 || status >= 300) { + const msg = json?.error?.message ?? json?.error ?? json?.message ?? JSON.stringify(json); + throw new Error(`POST ${path} → ${status}: ${msg}`); + } + return json; + } + + /** Rows of `object` matching `filters` (the data API's own query verb). */ + async query(object: string, filters: unknown[][], fields?: string[]): Promise { + const json = await this.postOk(`/api/v1/data/${object}/query`, { + filters, + ...(fields ? { fields } : {}), + top: 500, + }); + return (json.records ?? []) as Json[]; + } + + /** Sign in and keep the session cookie for every later call. */ + async signIn(email: string, password: string): Promise { + this.cookie = ''; + const { status, json } = await this.post('/api/v1/auth/sign-in/email', { email, password }); + if (status !== 200 || !json?.user?.id) { + throw new Error( + `sign-in failed for ${email} (${status}: ${json?.message ?? json?.code ?? 'unknown'}). ` + + `The dev admin exists only on a development server started with \`pnpm dev\` ` + + `(the platform hard-gates that seed on NODE_ENV=development).`, + ); + } + return json.user as Json; + } +} + +type StaffOutcome = { + member: DemoStaffMember; + userId: string; + created: boolean; + positionsAdded: string[]; + positionsAlready: string[]; +}; + +/** + * Create the person if they are not there yet; either way return their id. + * + * Org membership is not passed: `create-user` binds the new account to the sole + * organization itself (`sys_member`, role `member`), which is the whole reason + * this goes through the auth surface rather than the data API. + */ +async function ensureUser(api: Api, member: DemoStaffMember) { + const existing = await api.query('sys_user', [['email', '=', member.email]], ['id', 'email']); + if (existing.length > 0) return { userId: String(existing[0].id), created: false }; + + const res = await api.postOk('/api/v1/auth/admin/create-user', { + email: member.email, + password: member.password, + name: member.name, + // Without this the account is stamped must-change-password and every API + // call answers 403 PASSWORD_EXPIRED — a demo login that cannot demo. + mustChangePassword: false, + }); + const userId = res?.data?.user?.id; + if (typeof userId !== 'string' || !userId) { + throw new Error(`create-user returned no id for ${member.email}: ${JSON.stringify(res)}`); + } + return { userId, created: true }; +} + +/** Grant every position the row declares, skipping ones already held. */ +async function ensurePositions(api: Api, userId: string, member: DemoStaffMember, organizationId: string | null) { + const held = await api.query('sys_user_position', [['user_id', '=', userId]], ['id', 'position']); + const heldNames = new Set(held.map((r) => String(r.position))); + const added: string[] = []; + for (const position of member.positions) { + if (heldNames.has(position)) continue; + await api.postOk('/api/v1/data/sys_user_position', { + user_id: userId, + position, + ...(organizationId ? { organization_id: organizationId } : {}), + }); + added.push(position); + } + return { added, already: member.positions.filter((p) => heldNames.has(p)) }; +} + +/** + * Re-materialise the grants of every active rule. + * + * `POST /sharing/rules/:id/evaluate` is diff-based: it grants what now matches, + * revokes what no longer does, and is safe to run repeatedly. + */ +async function evaluateRules(api: Api) { + const rules = await api.query('sys_sharing_rule', [['active', '=', true]], ['id', 'name']); + const results: Array<{ name: string; matched: number; users: number; created: number; revoked: number }> = []; + for (const rule of rules) { + const out = await api.postOk(`/api/v1/sharing/rules/${rule.id}/evaluate`); + results.push({ + name: String(rule.name), + matched: Number(out.matchedRecords ?? 0), + users: Number(out.expandedUsers ?? 0), + created: Number(out.grantsCreated ?? 0), + revoked: Number(out.grantsRevoked ?? 0), + }); + } + return results; +} + +/** + * The point of the whole exercise, asserted rather than assumed: each demo user + * signs in and reads the accounts, and none of them may OWN what they were + * granted (a share to the owner proves nothing — the OWD baseline already + * admits them). + */ +async function verify(base: URL, outcomes: StaffOutcome[], adminAccounts: Json[]): Promise { + const failures: string[] = []; + const countryOf = (name: string) => + String(adminAccounts.find((a) => a.name === name)?.billing_country ?? '??'); + + for (const { member, userId } of outcomes) { + const asUser = new Api(base); + await asUser.signIn(member.email, member.password); + const rows = await asUser.query('crm_account', [], ['id', 'name', 'billing_country', 'owner_id']); + const names = rows.map((r) => String(r.name)).sort(); + const countries = [...new Set(names.map(countryOf))].sort(); + console.log(` ${member.email} sees ${rows.length} account(s): ${names.join(', ') || '—'}`); + console.log(` countries: [${countries.join(', ')}]`); + + const owned = rows.filter((r) => String(r.owner_id ?? '') === userId).map((r) => String(r.name)); + if (owned.length > 0) { + failures.push( + `${member.email} OWNS ${owned.join(', ')} — a share to a record's owner demonstrates ` + + `nothing, because the private OWD baseline already admits the owner. Ownership belongs ` + + `to demo_bootstrap's first user; staffing must not move it.`, + ); + } + if (member.positions.includes('na_sales_team') || member.positions.includes('eu_sales_team')) { + if (rows.length === 0) { + failures.push( + `${member.email} holds a territory position but reads no account at all — the rule ` + + `matched nothing, or no grant was materialised for it.`, + ); + } + const wanted = member.positions.includes('na_sales_team') + ? ['US', 'CA', 'MX'] + : ['UK', 'DE', 'FR', 'IT', 'ES']; + const strays = countries.filter((c) => !wanted.includes(c)); + if (strays.length > 0) { + failures.push( + `${member.email} reads accounts outside their territory (${strays.join(', ')}) — a ` + + `match-all regression looks exactly like this.`, + ); + } + } + } + return failures; +} + +async function main(): Promise { + const base = assertLocal(arg('url', 'OS_DEMO_URL', DEFAULT_URL)); + const adminEmail = arg('admin-email', 'OS_SEED_ADMIN_EMAIL', 'admin@objectos.ai'); + const adminPassword = arg('admin-password', 'OS_SEED_ADMIN_PASSWORD', 'admin123'); + + console.log(`\n── Demo-org staffing · ${base.origin} ──\n`); + const api = new Api(base); + const admin = await api.signIn(adminEmail, adminPassword); + console.log(`👤 signed in as ${admin.email}`); + + // The org every member belongs to — read off the admin's own membership so a + // single-org demo box needs no configuration. + const membership = await api.query('sys_member', [['user_id', '=', String(admin.id)]], ['organization_id']); + const organizationId = membership[0]?.organization_id ? String(membership[0].organization_id) : null; + console.log(`🏢 organization: ${organizationId ?? '(none — single-tenant boot)'}\n`); + + const outcomes: StaffOutcome[] = []; + for (const member of DemoOrgStaffing) { + const { userId, created } = await ensureUser(api, member); + const { added, already } = await ensurePositions(api, userId, member, organizationId); + outcomes.push({ member, userId, created, positionsAdded: added, positionsAlready: already }); + console.log( + `${created ? '➕' : '✓ '} ${member.email.padEnd(26)} ` + + `${added.length ? `+[${added.join(', ')}]` : ''}${already.length ? ` (already: ${already.join(', ')})` : ''}`, + ); + } + + console.log('\n── Re-evaluating sharing rules ──'); + for (const r of await evaluateRules(api)) { + console.log( + ` ${r.name.padEnd(30)} matched=${String(r.matched).padStart(3)} ` + + `holders=${r.users} granted=${r.created} revoked=${r.revoked}`, + ); + } + + const shares = await api.query('sys_record_share', [['source', '=', 'rule']], ['id']); + const accounts = await api.query('crm_account', [], ['name', 'billing_country']); + console.log(`\n── Verifying (sys_record_share: ${shares.length} rule-materialised grants) ──`); + const failures = await verify(base, outcomes, accounts); + + if (failures.length > 0) { + console.log('\n🔴 staffing did not connect:'); + for (const f of failures) console.log(` · ${f}`); + return 1; + } + console.log( + `\n🎉 demo org staffed. Sign in as any of: ` + + `${DemoOrgStaffing.map((m) => `${m.email} / ${m.password}`).join(' · ')}\n` + + ` Submit an opportunity over $100K to see manager_review route to ` + + `${DemoOrgStaffing.find((m) => m.positions.includes('sales_manager'))?.email}.\n`, + ); + return 0; +} + +main() + .then((code) => process.exit(code)) + .catch((err: unknown) => { + console.error(`\n🔴 ${err instanceof Error ? err.message : String(err)}\n`); + process.exit(1); + }); diff --git a/src/flows/demo-bootstrap.flow.ts b/src/flows/demo-bootstrap.flow.ts index a79858d4..fb82c0dc 100644 --- a/src/flows/demo-bootstrap.flow.ts +++ b/src/flows/demo-bootstrap.flow.ts @@ -34,6 +34,34 @@ type Flow = Automation.Flow; * user is. A real deployment assigns ownership through import or territory * rules instead, and by then nothing is ownerless for this to pick up. * + * ─── This flow must NOT staff anybody (#640) ───────────────────────────── + * + * The demo org now also has PEOPLE — an NA rep, an EU rep and a sales manager + * holding the positions the sharing rules and `opportunity_approval` route to + * (`src/sharing/demo-staffing.ts`). That staffing deliberately does not happen + * here, and the obvious "just add a create_record on sys_user" is refused twice + * over: + * + * - This flow ships in the ARTIFACT, so it runs in a customer's org too. The + * one outcome #640 rules out unconditionally is synthetic users appearing + * there, and the only way to make that impossible rather than unlikely is + * for the artifact to contain no mechanism that can create one. + * `test/demo-staffing.test.ts` fails on any flow node that writes an + * identity table. + * - It would not produce usable people anyway: identity tables are + * `managedBy: 'better-auth'` (ADR-0092), and a row inserted around that + * surface has no credential — an account nobody can sign in as. + * + * Staffing therefore lives in `pnpm demo:staff`, which drives a LOCAL dev + * server through the platform's own admin endpoints. It also depends on this + * flow's behaviour staying exactly as it is: the demo's whole point is that a + * rep reads accounts they do NOT own (a `private` OWD already admits the owner, + * so a share to the owner proves nothing). The reps are created after the dev + * admin and appended to `sys_user`, so `get_user`'s unordered "first user" is + * unchanged by staffing — and the staffing script re-checks that from the other + * side, failing if any demo user turns out to own a seeded account. Ownership + * itself is #548's subject; do not redefine it here. + * * ─── TWO ownership columns, not one (#622) ─────────────────────────────── * * Every claimed object carries two: diff --git a/src/sharing/demo-staffing.ts b/src/sharing/demo-staffing.ts new file mode 100644 index 00000000..18c1709d --- /dev/null +++ b/src/sharing/demo-staffing.ts @@ -0,0 +1,140 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Demo-org staffing — WHO HOLDS THE POSITIONS `positions.ts` DECLARES (#640). + * + * ### The gap this table closes + * + * The app ships three layers of position-based access and, until now, only two + * of them were real on a fresh install: + * + * 1. the rules install — #621 (`sys_sharing_rule`: 9 seeded, 0 skipped) + * 2. records match them — #638 (`crm_account.billing_country`: NA 6 / EU 2 / neither 1) + * 3. someone holds the position — **nobody did** (`sys_user_position`: 0 rows) + * + * With layer 3 empty, a matching account materialises no grant: measured on a + * fresh 17.0.0-rc.1 install, `sys_record_share` had 0 rows and every + * position-based rule this app ships granted nobody anything. The same hole ran + * through `opportunity_approval` — its `manager_review` node routes to the + * `sales_manager` position, so submitting a deal opened a request with an empty + * approver slate while `lockRecord: true` held the record hostage. + * + * ### Why this is a TABLE and not code + * + * Adding a person to the demo org is adding a ROW here. Nothing else changes: + * `scripts/demo-staff.ts` walks this array, and `test/demo-staffing.test.ts` + * checks each row against the declared positions, the sharing rules and the + * approval nodes. A row that names a position no rule and no permission set + * mentions is a row that grants nothing (#488) — the test says so by name. + * + * ### Why this file is NOT metadata, and MUST NOT become metadata + * + * It is deliberately not exported from `./index.ts` and not registered in + * `objectstack.config.ts`, so it is not in the published artifact. That is the + * hard constraint of #640, not a stylistic choice: **a real deployment must + * install none of these people.** Shipping synthetic users into a customer org + * would be worse than shipping no staffing at all, and the only way to make + * that impossible rather than unlikely is for the artifact to contain no + * mechanism that can create a user. `test/demo-staffing.test.ts` pins exactly + * that: no seed dataset and no flow node in this app targets `sys_user`, + * `sys_member`, `sys_user_position` or `sys_record_share`. + * + * Staffing therefore happens on the demo path only — `pnpm demo:staff`, run by + * a developer against their own local dev server, through the platform's own + * admin surfaces (`/api/v1/auth/admin/create-user`, the `sys_user_position` + * data API, `POST /api/v1/sharing/rules/:id/evaluate`). Those users are REAL, + * loginable accounts created by better-auth, not raw `sys_user` rows: identity + * tables are `managedBy: 'better-auth'` and direct data-API inserts are refused + * by the ADR-0092 write guard, which is also why a seed cannot do this and why + * the note at the foot of `src/data/index.ts` says a seed can never name a user. + * + * ### Why exactly these three people + * + * Not an org chart — the smallest set that makes each dark mechanism visible: + * + * - the two reps make TERRITORY SHARING observable for the first time. They + * must be users who do NOT own the accounts: `crm_account` is `private`, so + * the OWD baseline already admits a record's owner and a share to the owner + * proves nothing. `demo_bootstrap` claims every seeded record for the first + * user (the dev admin, #622) and staffing deliberately does not touch that — + * the reps stay non-owners, which is the whole point. + * - the sales manager makes `opportunity_approval`'s `manager_review` resolve + * to a non-empty slate for the first time. + * + * The other seven positions (`sales_director`, `executive`, `service_manager`, + * `service_director`, `marketing_manager`, `marketing_director`, + * `marketing_user`) stay UNSTAFFED on purpose: a real deployment staffs its own + * people, and an empty bench is the honest depiction of that. The approval + * nodes that route to them declare `onEmptyApprovers: 'admin_rescue'`, so an + * empty bench holds for admin takeover instead of stranding the record. + * + * ### Why a rep holds TWO positions + * + * `na_sales_team` / `eu_sales_team` are territory groupings — no permission set + * is bound to them, so on their own they widen WHICH RECORDS a user sees + * without saying what a user may DO. Measured on a fresh install: a user + * holding only `eu_sales_team` still reads the 2 EU accounts, because the + * platform's additive baseline (ADR-0090 D5) gives every org member + * `member_default` — i.e. the demo would show a generic member who happens to + * see two accounts, not a sales rep. Pairing the territory with the functional + * `sales_rep` position (which name-binds this app's `SalesRepProfile`) is what + * makes the persona real: their own book, plus the territory. + */ + +/** One person in the demo org. Add a person = add a row. */ +export type DemoStaffMember = { + /** Stable key used in logs and test failures. Never stored. */ + readonly key: string; + /** Display name on the account. */ + readonly name: string; + /** Login. `@objectos.ai` matches the platform's own dev-admin convention. */ + readonly email: string; + /** + * Dev-only password. Same class of secret as the platform's `admin123` + * dev-admin seed: it only ever reaches a local demo box, because nothing in + * the published artifact reads this file. Minimum 8 chars (better-auth's + * `minPasswordLength`). + */ + readonly password: string; + /** + * Positions this person holds, by machine name. Every entry must exist in + * `CrmPositions` — a position the app does not declare grants nothing and + * cannot be granted anything. + */ + readonly positions: readonly string[]; + /** What staffing this person makes observable. Asserted to be non-empty. */ + readonly demonstrates: string; +}; + +export const DemoOrgStaffing: readonly DemoStaffMember[] = [ + { + key: 'na_rep', + name: 'Nina Reyes', + email: 'na.rep@objectos.ai', + password: 'demo1234', + positions: ['sales_rep', 'na_sales_team'], + demonstrates: + 'north_america_territory: reads the 6 US/CA/MX accounts she does not own, and neither ' + + 'the 2 EU accounts nor the 1 account in no territory.', + }, + { + key: 'eu_rep', + name: 'Emil Roth', + email: 'eu.rep@objectos.ai', + password: 'demo1234', + positions: ['sales_rep', 'eu_sales_team'], + demonstrates: + 'europe_territory: reads the 2 UK/DE accounts he does not own, and neither the 6 NA ' + + 'accounts nor the 1 account in no territory.', + }, + { + key: 'sales_manager', + name: 'Marta Quinn', + email: 'sales.manager@objectos.ai', + password: 'demo1234', + positions: ['sales_manager'], + demonstrates: + "opportunity_approval's manager_review node resolves to a non-empty approver slate, and " + + 'account_team_sharing hands her the active customer accounts.', + }, +]; diff --git a/test/demo-staffing.test.ts b/test/demo-staffing.test.ts new file mode 100644 index 00000000..5292e9e4 --- /dev/null +++ b/test/demo-staffing.test.ts @@ -0,0 +1,338 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { compileCelToFilter } from '@objectstack/formula'; +import stack from '../objectstack.config'; +import accountHook from '../src/objects/account.hook'; +import { CrmSeedData } from '../src/data/index'; +import { DemoOrgStaffing } from '../src/sharing/demo-staffing'; +import * as sharingBarrel from '../src/sharing/index'; + +/** + * Demo-org staffing (#640) — the third layer of position-based access. + * + * ### What was dark + * + * #621 made the sharing rules install; #638 gave them records to match. Nobody + * held a position, so a matching account still materialised no grant. Measured + * on a fresh 17.0.0-rc.1 install: `sys_user_position` 0 rows, + * `sys_record_share` 0 rows, and `opportunity_approval`'s `manager_review` + * opened with an empty approver slate while `lockRecord: true` held the record. + * + * `src/sharing/demo-staffing.ts` closes it with a TABLE of demo people, applied + * by `pnpm demo:staff` against a local dev server. Measured after that script, + * on the same fresh install: + * + * north_america_territory matched=6 holders=1 granted=6 + * europe_territory matched=2 holders=1 granted=2 + * account_team_sharing matched=5 holders=1 granted=5 + * sys_record_share 13 rows, all source='rule' + * na.rep@objectos.ai sees 6 accounts [CA, US] + * eu.rep@objectos.ai sees 2 accounts [DE, UK] + * sys_approval_request current_step=manager_review + * pending_approvers=[sales.manager@objectos.ai] + * + * ### What this file can and cannot check + * + * It is a static suite: it cannot run the script. What it CAN pin is every + * authoring-time premise the script depends on — that each staffed position is + * declared, that the positions chosen are the ones the sharing rules and the + * approval nodes actually name, that the territory arithmetic still comes out + * 6/2/1 against the real seeds, and — the load-bearing one — that **the + * published artifact contains no mechanism that could create these people**. + * That last group is what keeps "a real deployment installs none of them" a + * structural fact rather than a promise. + */ + +type AnyRec = Record; + +const positions: AnyRec[] = (stack as any).positions ?? []; +const sharingRules: AnyRec[] = (stack as any).sharingRules ?? []; +const permissionSets: AnyRec[] = (stack as any).permissions ?? []; +const flows: AnyRec[] = (stack as any).flows ?? []; + +const positionNames = new Set(positions.map((p) => String(p.name))); +const staffedPositions = new Set(DemoOrgStaffing.flatMap((m) => m.positions)); +const holdersOf = (position: string) => DemoOrgStaffing.filter((m) => m.positions.includes(position)); + +/** + * Identity/access tables only the platform may write. A seed dataset or flow + * node naming any of these is the #640 hard constraint being violated: it would + * mean the shipped app can conjure users, memberships or grants inside a + * customer's org. + */ +const IDENTITY_OBJECTS = [ + 'sys_user', + 'sys_member', + 'sys_user_position', + 'sys_position_permission_set', + 'sys_user_permission_set', + 'sys_record_share', +]; + +describe('the staffing table is well-formed', () => { + it('has people at all', () => { + // Guard the guard: an empty table would make every assertion below vacuous. + expect(DemoOrgStaffing.length).toBeGreaterThanOrEqual(3); + }); + + it('gives every row a unique key, a unique email, a login and a reason', () => { + const problems: string[] = []; + const keys = new Set(); + const emails = new Set(); + for (const m of DemoOrgStaffing) { + if (keys.has(m.key)) problems.push(`duplicate key "${m.key}"`); + keys.add(m.key); + if (emails.has(m.email)) problems.push(`duplicate email "${m.email}"`); + emails.add(m.email); + if (!/^[^@\s]+@[^@\s]+\.[a-z]+$/.test(m.email)) problems.push(`${m.key}: "${m.email}" is not an email`); + // better-auth rejects a shorter password at create-user time, which would + // fail the staffing run halfway through with a 400 from the auth API. + if (m.password.length < 8) problems.push(`${m.key}: password is under better-auth's 8-char minimum`); + if (m.positions.length === 0) problems.push(`${m.key}: holds no position, so staffing them changes nothing`); + if (m.demonstrates.trim().length === 0) problems.push(`${m.key}: no 'demonstrates' — say what this person makes visible`); + } + expect(problems, `staffing rows that cannot be applied:\n ${problems.join('\n ')}`).toEqual([]); + }); + + it('only names positions the app declares', () => { + // A position the app does not declare cannot be granted anything and cannot + // grant anything (#488) — the assignment row would sit there inert. + const unknown = [...staffedPositions].filter((p) => !positionNames.has(p)); + expect( + unknown, + `staffing names positions that are not in CrmPositions: ${unknown.join(', ')}`, + ).toEqual([]); + }); +}); + +describe('the staffing table encodes the #640 decision, not an org chart', () => { + it('staffs both territories and the sales manager — and nothing else', () => { + // The three people are exactly the ones that turn a dark mechanism on: + // two territory holders (who must not own the accounts) and one approver. + expect(holdersOf('na_sales_team').length, 'nobody holds na_sales_team').toBe(1); + expect(holdersOf('eu_sales_team').length, 'nobody holds eu_sales_team').toBe(1); + expect(holdersOf('sales_manager').length, 'nobody holds sales_manager').toBe(1); + expect( + holdersOf('na_sales_team')[0].email, + 'the two territories must be held by DIFFERENT people, or neither rule is distinguishable', + ).not.toBe(holdersOf('eu_sales_team')[0].email); + }); + + it('leaves the leadership bench empty on purpose', () => { + // Decided in #640: staffing exists to make the mechanism observable, not to + // populate an org chart. A real deployment staffs its own people, and an + // empty bench is the honest depiction of that. Filling one of these in + // needs the same conversation the first three had — hence a failing test, + // not a silent addition. + const DELIBERATELY_UNSTAFFED = [ + 'executive', 'sales_director', 'service_director', 'service_manager', + 'service_agent', 'marketing_director', 'marketing_manager', 'marketing_user', + ]; + const surprises = DELIBERATELY_UNSTAFFED.filter((p) => staffedPositions.has(p)); + expect( + surprises, + `these positions were deliberately left unstaffed in #640 — staffing them is a decision, ` + + `not a tidy-up: ${surprises.join(', ')}`, + ).toEqual([]); + }); + + it('pairs each territory position with a functional one that carries a permission set', () => { + // A territory position is a RECORD grouping: no permission set binds to + // `na_sales_team` / `eu_sales_team`, so on its own it widens which rows a + // user sees without saying what they may do. Measured: a user holding only + // `eu_sales_team` still reads the 2 EU accounts — via the platform's + // additive `member_default` baseline (ADR-0090 D5) — i.e. the demo would + // show a generic org member, not a sales rep. + const setNames = new Set(permissionSets.map((p) => String(p.name))); + const bad: string[] = []; + for (const m of DemoOrgStaffing) { + const territorial = m.positions.some((p) => p === 'na_sales_team' || p === 'eu_sales_team'); + if (!territorial) continue; + // A set whose NAME matches a position is bound to it at install time. + if (!m.positions.some((p) => setNames.has(p))) { + bad.push(`${m.email}: holds only territory position(s) [${m.positions.join(', ')}], so no permission set applies`); + } + } + expect(bad, `demo reps that would log in as generic members:\n ${bad.join('\n ')}`).toEqual([]); + }); +}); + +describe('staffing lines up with the rules that grant, and the nodes that route', () => { + it('every staffed position is named by a sharing rule or a permission set', () => { + const referenced = new Set(); + for (const rule of sharingRules) { + if (rule.sharedWith?.type === 'position') referenced.add(String(rule.sharedWith.value)); + } + for (const flow of flows) { + for (const node of (flow.nodes ?? []) as AnyRec[]) { + for (const a of (node.config?.approvers ?? []) as AnyRec[]) { + if (a?.type === 'position') referenced.add(String(a.value)); + } + } + } + for (const ps of permissionSets) if (positionNames.has(String(ps.name))) referenced.add(String(ps.name)); + + const inert = [...staffedPositions].filter((p) => !referenced.has(p)); + expect( + inert, + `staffed positions nothing grants through — holding them changes nothing (#488):\n ${inert.join('\n ')}`, + ).toEqual([]); + }); + + it("makes opportunity_approval's manager_review resolve to a real person", () => { + const flow = flows.find((f) => f.name === 'opportunity_approval'); + expect(flow, 'opportunity_approval is not registered').toBeTruthy(); + const node = (flow!.nodes as AnyRec[]).find((n) => n.id === 'manager_review'); + expect(node, 'manager_review node is gone').toBeTruthy(); + + const approvers = (node!.config?.approvers ?? []) as AnyRec[]; + expect(approvers.length, 'manager_review declares no approvers at all').toBeGreaterThan(0); + const unstaffed = approvers + .filter((a) => a?.type === 'position' && !staffedPositions.has(String(a.value))) + .map((a) => String(a.value)); + expect( + unstaffed, + `manager_review routes to position(s) nobody in the demo org holds (${unstaffed.join(', ')}), so a ` + + `submitted deal opens an EMPTY approver slate and — with lockRecord: true — locks the record ` + + `with no in-product recovery. Staff the position, or reroute the node.`, + ).toEqual([]); + }); + + it('keeps an empty-bench policy on the approval nodes it does NOT staff', () => { + // `director_signoff` routes to `sales_director`, deliberately unstaffed. That + // is only safe because the node declares what happens when the bench is + // empty; without it the record locks undecidably. + const bad: string[] = []; + for (const flow of flows) { + for (const node of (flow.nodes ?? []) as AnyRec[]) { + if (node.type !== 'approval') continue; + const approvers = (node.config?.approvers ?? []) as AnyRec[]; + const groupRouted = approvers.filter((a) => a?.type === 'position'); + if (groupRouted.length === 0) continue; + const allStaffed = groupRouted.every((a) => staffedPositions.has(String(a.value))); + if (allStaffed) continue; + if (node.config?.onEmptyApprovers == null) { + bad.push(`${flow.name} · ${node.id}: routes to an unstaffed position and declares no onEmptyApprovers`); + } + } + } + expect(bad, `approval nodes that can strand a locked record:\n ${bad.join('\n ')}`).toEqual([]); + }); +}); + +/** + * The arithmetic the demo rests on, recomputed from the REAL seeds through the + * REAL hook and the seeder's own CEL compiler — the same chain + * `test/territory-seed-coverage.test.ts` walks, asked here from the staffing + * side: what does each staffed person actually get? + */ +describe('what each staffed person actually receives', () => { + type Dataset = { object: string; records: AnyRec[] }; + const accountRecords = (CrmSeedData as unknown as Dataset[]) + .filter((d) => d.object === 'crm_account') + .flatMap((d) => d.records); + + const project = async (record: AnyRec): Promise => { + const input: AnyRec = { ...record }; + await (accountHook as AnyRec).handler({ event: 'beforeInsert', input }); + return input; + }; + + const matches = (filter: AnyRec, row: AnyRec): boolean => { + const entries = Object.entries(filter); + if (entries.length !== 1) throw new Error(`expected a single-field filter, got ${JSON.stringify(filter)}`); + const [field, condition] = entries[0]; + if (condition && typeof condition === 'object' && !Array.isArray(condition)) { + const ops = Object.entries(condition as AnyRec); + if (ops.length === 1 && ops[0][0] === '$in' && Array.isArray(ops[0][1])) { + return (ops[0][1] as unknown[]).includes(row[field]); + } + throw new Error(`unsupported compiled operator: ${JSON.stringify(condition)}`); + } + return row[field] === condition; + }; + + /** Account names one staffed member receives through a named territory rule. */ + const receives = async (ruleName: string): Promise => { + const rule = sharingRules.find((r) => r.name === ruleName); + if (!rule) throw new Error(`no sharing rule named ${ruleName}`); + const compiled = compileCelToFilter(rule.condition ?? '', { variables: {} }); + if (!compiled.ok) throw new Error(`${ruleName}: condition does not compile (${compiled.reason})`); + const rows = await Promise.all(accountRecords.map(project)); + return rows.filter((row) => matches(compiled.filter as AnyRec, row)).map((r) => String(r.name)); + }; + + it('hands the NA rep six accounts and the EU rep two', async () => { + const na = await receives('north_america_territory'); + const eu = await receives('europe_territory'); + expect(na.length, `north_america_territory covers [${na.join(', ')}]`).toBe(6); + expect(eu.length, `europe_territory covers [${eu.join(', ')}]`).toBe(2); + expect(na.filter((n) => eu.includes(n)), 'an account in BOTH territories hides which rule granted it').toEqual([]); + }); + + it('leaves the out-of-territory account invisible to both reps', async () => { + // #638 seeded one account (SG) that matches NEITHER rule, deliberately: a + // set with nothing outside the territories cannot tell a working filter + // apart from a match-all one. It is a probe — do not remove or retune it. + const na = await receives('north_america_territory'); + const eu = await receives('europe_territory'); + const rows = await Promise.all(accountRecords.map(project)); + const outside = rows.map((r) => String(r.name)).filter((n) => !na.includes(n) && !eu.includes(n)); + expect( + outside.length, + `every seeded account falls in a territory, so a match-all regression would be invisible`, + ).toBeGreaterThan(0); + }); +}); + +describe('the published artifact cannot create these people (#640 hard constraint)', () => { + it('ships no seed dataset that writes an identity table', () => { + const bad = (CrmSeedData as unknown as Array<{ object: string }>) + .filter((d) => IDENTITY_OBJECTS.includes(d.object)) + .map((d) => d.object); + expect( + bad, + `seed datasets targeting identity tables: ${bad.join(', ')}. A seed runs in EVERY install, ` + + `including a customer's — synthetic users must never reach one. (It would not work either: ` + + `identity tables are managedBy better-auth and a seed cannot name a user.)`, + ).toEqual([]); + }); + + it('ships no flow node that writes an identity table', () => { + const bad: string[] = []; + const walk = (nodes: AnyRec[], flowName: string) => { + for (const node of nodes ?? []) { + const objectName = node?.config?.objectName; + const writes = ['create_record', 'update_record', 'delete_record'].includes(String(node?.type)); + if (writes && IDENTITY_OBJECTS.includes(String(objectName))) { + bad.push(`${flowName} · ${node.id}: ${node.type} on ${objectName}`); + } + if (node?.config?.body?.nodes) walk(node.config.body.nodes as AnyRec[], flowName); + } + }; + for (const flow of flows) walk((flow.nodes ?? []) as AnyRec[], String(flow.name)); + expect( + bad, + `flow nodes that would provision identity inside any install, customer orgs included:\n ${bad.join('\n ')}`, + ).toEqual([]); + }); + + it('keeps the staffing table out of the stack entirely', () => { + // The strongest form of the constraint: whatever `objectstack build` writes + // into the artifact, none of these accounts are in it. The table is reached + // only by `scripts/demo-staff.ts` and by this suite. + expect( + Object.keys(sharingBarrel), + 'src/sharing/index.ts must not re-export DemoOrgStaffing — the barrel is what objectstack.config.ts reads', + ).not.toContain('DemoOrgStaffing'); + + const serialized = JSON.stringify(stack, (_k, v) => (typeof v === 'function' ? undefined : v)); + for (const member of DemoOrgStaffing) { + expect( + serialized.includes(member.email), + `${member.email} appears in the app manifest — it would install into a customer org`, + ).toBe(false); + } + }); +}); From c447a77efca4f4dff3d61b33cbb2f35268c561aa Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 19:53:55 +0000 Subject: [PATCH 2/2] fix(demo): stop echoing demo passwords in the staffing banner, and pin what actually bounds a territory rep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL (js/clear-text-logging of sensitive information, high) flagged the success banner, which printed `email / password` for each demo user. The alert is correct and worth fixing on its own terms: nothing is concealed by dropping it — the passwords are declared one file away in `src/sharing/demo-staffing.ts`, and the script already refuses non-loopback targets — but a run's stdout reaches terminals, CI logs and pasted snippets, and "echo the credential you just used" is the last shape a reference app should teach. The banner now names the accounts and points at the file that declares their passwords. Measured: 0 occurrences of the password in a full run's stdout. The `password` field stays in the table — `verify()` signs in as each demo user with it, which is the layer that proves the three tiers actually connect. Also corrects a claim in the staffing note that was true in outcome but wrong in mechanism. Adding `sales_rep` does NOT stop the platform's additive baseline from admitting the 2 EU accounts; the baseline never admitted them. `POST /api/v1/security/explain` as the NA rep, on crm_account/read: positions [org_member, na_sales_team, sales_rep, everyone] permissionSets [sales_rep, member_default] object_crud grants — read granted by [sales_rep, member_default] owd_baseline narrows — private: rows are owner-visible only; sharing can only WIDEN from here depth Effective read depth: 'own' (ADR-0057 D1 — widest across granting sets) sharing widens — shares/rules OR-in additional rows vama_bypass not_applicable — No View/Modify All Data bypass So `member_default` opens the OBJECT door, not rows; the row set is OWD-private (own — and the reps own nothing) OR-in their shares. `sales_rep` computes the same 'own' depth and adds no bypass, so it widens nothing — it only decides what the persona may DO. The falsifiable corollary is now a test: no set bound to a position a territory rep holds may grant `viewAllRecords` on `crm_account`, or the rep reads all nine and the territory grant proves nothing while the org still looks staffed (exactly what `sales_manager` does, correctly, at 9/9). Refs #640. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019SS7C5SXpniKeCApxgARyf --- .changeset/staff-the-demo-org.md | 25 +++++++++---- scripts/demo-staff.ts | 9 ++++- src/sharing/demo-staffing.ts | 54 ++++++++++++++++++++++------ test/demo-staffing.test.ts | 61 ++++++++++++++++++++++++++++---- 4 files changed, 124 insertions(+), 25 deletions(-) diff --git a/.changeset/staff-the-demo-org.md b/.changeset/staff-the-demo-org.md index f74548d2..57693365 100644 --- a/.changeset/staff-the-demo-org.md +++ b/.changeset/staff-the-demo-org.md @@ -36,9 +36,22 @@ metadata: nothing in the published artifact can create a user, and metadata either — identity tables are `managedBy: 'better-auth'`, so a row inserted around that surface has no credential and nobody can sign in as it.) -One platform behaviour worth carrying forward, measured here: `plugin-sharing` -materialises rule grants from a record-write hook that returns early on -`isSystem` writes, and every seeded row is written with `isSystem: true`. So -staffing alone leaves `sys_record_share` empty until a rule is re-evaluated — -which the script does, and which a server restart also does via the boot -backfill. Fixes #640. Refs #621, #638, #622, #488. +Two platform behaviours worth carrying forward, both measured here. + +`plugin-sharing` materialises rule grants from a record-write hook that returns +early on `isSystem` writes, and every seeded row is written with +`isSystem: true`. So staffing alone leaves `sys_record_share` empty until a rule +is re-evaluated — which the script does, and which a server restart also does +via the boot backfill. + +And what bounds a rep to their territory is not their profile. Object-level read +on `crm_account` comes from `member_default`, the additive baseline every org +member holds (ADR-0090 D5); the row set comes from `crm_account` being `private` +(rows are owner-visible only, and the reps own nothing) plus the shares their +territory rule materialised. Each rep also holds `sales_rep`, which widens no +rows — `viewAllRecords: false, readScope: 'own'` is the same depth the baseline +computes — but makes the persona a sales rep rather than a generic member. The +corollary is now a test: a set bound to a position a territory rep holds must +never grant `viewAllRecords` on `crm_account`, or the rep reads all nine +accounts and the territory grant proves nothing while the org still looks +staffed. Fixes #640. Refs #621, #638, #622, #488. diff --git a/scripts/demo-staff.ts b/scripts/demo-staff.ts index 69a8e8ab..f3cf9405 100644 --- a/scripts/demo-staff.ts +++ b/scripts/demo-staff.ts @@ -308,9 +308,16 @@ async function main(): Promise { for (const f of failures) console.log(` · ${f}`); return 1; } + // The banner names the accounts but never their passwords. Nothing is hidden + // by that — the passwords are declared in `src/sharing/demo-staffing.ts`, one + // file away — but a run's stdout ends up in terminals, CI logs and pasted + // snippets, and "echo the credential you just used" is the one shape this + // reference app should not be teaching. (CodeQL says the same thing: + // js/clear-text-logging of sensitive information.) console.log( `\n🎉 demo org staffed. Sign in as any of: ` + - `${DemoOrgStaffing.map((m) => `${m.email} / ${m.password}`).join(' · ')}\n` + + `${DemoOrgStaffing.map((m) => m.email).join(' · ')}\n` + + ` Passwords are declared in src/sharing/demo-staffing.ts.\n` + ` Submit an opportunity over $100K to see manager_review route to ` + `${DemoOrgStaffing.find((m) => m.positions.includes('sales_manager'))?.email}.\n`, ); diff --git a/src/sharing/demo-staffing.ts b/src/sharing/demo-staffing.ts index 18c1709d..dd940342 100644 --- a/src/sharing/demo-staffing.ts +++ b/src/sharing/demo-staffing.ts @@ -68,17 +68,49 @@ * nodes that route to them declare `onEmptyApprovers: 'admin_rescue'`, so an * empty bench holds for admin takeover instead of stranding the record. * - * ### Why a rep holds TWO positions - * - * `na_sales_team` / `eu_sales_team` are territory groupings — no permission set - * is bound to them, so on their own they widen WHICH RECORDS a user sees - * without saying what a user may DO. Measured on a fresh install: a user - * holding only `eu_sales_team` still reads the 2 EU accounts, because the - * platform's additive baseline (ADR-0090 D5) gives every org member - * `member_default` — i.e. the demo would show a generic member who happens to - * see two accounts, not a sales rep. Pairing the territory with the functional - * `sales_rep` position (which name-binds this app's `SalesRepProfile`) is what - * makes the persona real: their own book, plus the territory. + * ### Why a rep holds TWO positions — and what actually bounds the territory + * + * `na_sales_team` / `eu_sales_team` are territory groupings: no permission set + * is bound to either, so holding one says nothing about what a user may DO. + * Measured on a fresh install, a user holding ONLY `eu_sales_team` already + * reads the 2 EU accounts — `POST /api/v1/security/explain` for that user: + * + * principal positions [org_member, eu_sales_team, everyone] + * → permission set(s) [member_default] + * object_crud grants — read on 'crm_account' is granted by [member_default] + * owd_baseline narrows — Record baseline (OWD) is private: rows are + * owner-visible only; sharing can only WIDEN from here. + * depth Effective read depth: 'own' (ADR-0057 D1 — widest + * across granting sets) + * + * Read that layer stack carefully, because it says which layer is doing which + * job — and the answer is NOT "the profile narrows them to their territory": + * + * - the OBJECT-LEVEL door on `crm_account` is opened by `member_default`, the + * platform's additive baseline that every org member gets (ADR-0090 D5). + * It is open with or without `sales_rep`. + * - the ROW SET is decided one layer down and is the whole ballgame: + * `crm_account` is `sharingModel: 'private'`, so the OWD baseline admits + * only rows the caller OWNS, and `sys_record_share` can only widen it. The + * reps own nothing (`demo_bootstrap` claimed every seeded record for the + * dev admin), so their row set is exactly the grants their territory rule + * materialised — 6 for NA, 2 for EU, and the SG account for nobody. + * + * So adding `sales_rep` widens no rows at all: its `crm_account` grant is + * `viewAllRecords: false, readScope: 'own'`, the same depth `member_default` + * already computes ("widest across granting sets" — an equal depth changes + * nothing). What it adds is what the persona may DO — create/edit, the + * `health_score` / `annual_revenue` field grants, export — i.e. it turns a + * generic org member who happens to see two records into a sales rep. + * + * The falsifiable part, and the reason `test/demo-staffing.test.ts` pins it: a + * set bound to a position a territory rep holds MUST NOT grant `viewAllRecords` + * on `crm_account`. That would widen the depth to all-rows and the rep would + * read all nine accounts, so the territory grant would stop proving anything + * while still looking staffed. This is not hypothetical — it is exactly what + * the sales manager does: `SalesManagerProfile` declares `viewAllRecords: true` + * and she reads all 9 (measured), which is correct for a manager and would be + * silent death for a territory demo. */ /** One person in the demo org. Add a person = add a row. */ diff --git a/test/demo-staffing.test.ts b/test/demo-staffing.test.ts index 5292e9e4..0030b4bd 100644 --- a/test/demo-staffing.test.ts +++ b/test/demo-staffing.test.ts @@ -55,6 +55,10 @@ const positionNames = new Set(positions.map((p) => String(p.name))); const staffedPositions = new Set(DemoOrgStaffing.flatMap((m) => m.positions)); const holdersOf = (position: string) => DemoOrgStaffing.filter((m) => m.positions.includes(position)); +/** Someone whose visible rows are supposed to be bounded by a territory rule. */ +const territorial = (m: { positions: readonly string[] }) => + m.positions.some((p) => p === 'na_sales_team' || p === 'eu_sales_team'); + /** * Identity/access tables only the platform may write. A seed dataset or flow * node naming any of these is the #640 hard constraint being violated: it would @@ -139,16 +143,16 @@ describe('the staffing table encodes the #640 decision, not an org chart', () => it('pairs each territory position with a functional one that carries a permission set', () => { // A territory position is a RECORD grouping: no permission set binds to - // `na_sales_team` / `eu_sales_team`, so on its own it widens which rows a - // user sees without saying what they may do. Measured: a user holding only - // `eu_sales_team` still reads the 2 EU accounts — via the platform's - // additive `member_default` baseline (ADR-0090 D5) — i.e. the demo would - // show a generic org member, not a sales rep. + // `na_sales_team` / `eu_sales_team`, so on its own it says nothing about + // what a user may DO. Measured: a user holding only `eu_sales_team` still + // reads the 2 EU accounts — the object door is opened by the platform's + // additive `member_default` baseline (ADR-0090 D5) and the rows come from + // the share — i.e. the demo would show a generic org member who happens to + // see two records, not a sales rep. const setNames = new Set(permissionSets.map((p) => String(p.name))); const bad: string[] = []; for (const m of DemoOrgStaffing) { - const territorial = m.positions.some((p) => p === 'na_sales_team' || p === 'eu_sales_team'); - if (!territorial) continue; + if (!territorial(m)) continue; // A set whose NAME matches a position is bound to it at install time. if (!m.positions.some((p) => setNames.has(p))) { bad.push(`${m.email}: holds only territory position(s) [${m.positions.join(', ')}], so no permission set applies`); @@ -156,6 +160,49 @@ describe('the staffing table encodes the #640 decision, not an org chart', () => } expect(bad, `demo reps that would log in as generic members:\n ${bad.join('\n ')}`).toEqual([]); }); + + it('never gives a territory rep an org-wide view of crm_account', () => { + // THE mechanism question, pinned. What bounds a rep to their territory is + // NOT their profile — it is `crm_account`'s `private` OWD (rows are + // owner-visible only, and the reps own nothing) plus the `sys_record_share` + // rows their territory rule materialised. Read DEPTH is the "widest across + // granting sets" (ADR-0057 D1), and both sets in play compute `own`: + // `member_default` (the additive baseline every org member holds) and + // `SalesRepProfile.crm_account` (`viewAllRecords: false, readScope: 'own'`). + // + // So a single flipped bit on any set bound to a position a rep holds — + // `viewAllRecords: true` — widens the depth to all-rows, the rep reads all + // nine accounts, and the territory grant proves nothing while the org still + // looks correctly staffed. That is not hypothetical: it is exactly what + // `sales_manager` does (measured: she reads all 9, which is right for a + // manager). Without this assertion the whole demo would be true by + // coincidence, and a profile edit could switch it off in silence. + const setByName = new Map(permissionSets.map((p) => [String(p.name), p])); + const bad: string[] = []; + for (const m of DemoOrgStaffing) { + if (!territorial(m)) continue; + for (const position of m.positions) { + const grant = (setByName.get(position)?.objects ?? {})['crm_account']; + if (!grant) continue; + if (grant.viewAllRecords === true) { + bad.push( + `${m.email}: position "${position}" grants viewAllRecords on crm_account — they would ` + + `read every account, territory or not`, + ); + } + if (grant.readScope != null && grant.readScope !== 'own') { + bad.push( + `${m.email}: position "${position}" reads crm_account at scope "${grant.readScope}", ` + + `which is wider than the 'own' depth the territory demo rests on`, + ); + } + } + } + expect( + bad, + `territory reps whose row set is no longer bounded by sharing:\n ${bad.join('\n ')}`, + ).toEqual([]); + }); }); describe('staffing lines up with the rules that grant, and the nodes that route', () => {