Skip to content

Commit 087492c

Browse files
akshay-vizCopilot
andcommitted
feat(model-apps): verify what persona security roles actually grant
Two metadata-only verification gaps, both found while auditing what `verify` can and cannot prove. Neither needs a live browser or new infrastructure. 1. verify now proves what a persona role GRANTS, not just that it exists. The `role` check asserted only that a role ROW exists carrying the SDK ownership marker. It never looked at privileges - so a role created with the wrong access, or one whose privilege write failed after the row landed, verified clean. The new `role-privileges` check resolves each declared (entity, access) to its Dataverse PrivilegeId from the SAME metadata source the SDK writes against - EntityDefinitions(...)?$select=Privileges - and asserts the role holds it at AT LEAST the declared depth. SUBSET, not equality, and lib/role-privileges.js records why: appAccess injects appmodule read, unioned jobs escalate a shared entity+access to the max declared scope, and distinct entities can share ONE Dataverse privilege (a role holds one depth per privilege). Equality would fail on all three while telling us nothing true. Fails CLOSED on an unreadable role or table. The read deliberately does NOT go through sdk.fetchEntityMetadata: that returns a projected shape which drops Privileges entirely, so routing through it would have silently reported every privilege as unreadable. 2. personas[].jobs[].surfaces[] is checked instead of documentary. app-spec.js validated each entry as a non-empty string and stopped; spec-lint warned only when the array was EMPTY. So a job could name "My Open Work Orders" when no such view existed anywhere in the spec and every gate passed. lib/surface-resolver.js resolves each entry against the spec's own views, forms, pages (key OR name), dashboards, tables and sitemap titles. spec-lint WARNS on no match - a warning, never an error, because app-spec.js is loose on purpose: a surface may legitimately name an out-of-the-box artifact this spec does not author. verify adds a `job-surface` rollup - a PURE rollup over checks already computed, so it costs no extra reads - reporting a deployed failure as the job it broke ("persona P can no longer do job J") rather than only "view X is missing". Both wire into verifySpec's existing READER-GATED seam (the pattern entityRelationships / commandBar already use), so an existence-only reader behaves exactly as it did before. Tests: 28 new (1446 -> 1474 pass, 0 fail). Both features red-green verified: disabling the lint wiring fails 2, disabling the privilege check fails 3. Evals 159 pass, 6/6 validators. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42626da2-b66f-4162-acaa-b1127ef23d89
1 parent abe68e0 commit 087492c

11 files changed

Lines changed: 663 additions & 6 deletions

File tree

plugins/model-apps/AGENTS.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,9 @@ scripts/
423423
sdk-teardown.js ← app-builder teardown engine (planTeardown is pure)
424424
sdk-http-client.js ← az-token HttpClient for the vendored SDK
425425
spec-lint.js / app-spec.js ← App Spec guardrail lint + validation
426+
spec-shape.js ← shared structural normalization for both authoring gates
427+
surface-resolver.js ← pure: resolve personas[].jobs[].surfaces[] to the spec artifacts that satisfy them
428+
role-privileges.js ← pure: declared persona privileges + subset comparison against a deployed role
426429
odata.js ← OData literal escaping helpers
427430
genpage-cli.js ← pac model genpage upload/list/download wrapper
428431
hydrate-spec.js ← reconstruct an App Spec from a deployed app (edit flow)

plugins/model-apps/CHANGELOG.md

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,29 @@ All notable changes to the **model-apps** plugin.
44

55
## [Unreleased] — 2.4.4
66

7-
Adds plugin update notices, fixes four crash paths, and corrects a smoke-eval
8-
assertion that could never pass live.
7+
Adds plugin update notices, proves what persona roles actually grant, makes
8+
jobs-to-be-done surfaces checkable, fixes four crash paths, and corrects a
9+
smoke-eval assertion that could never pass live.
910

1011
### Added
12+
- **`verify` now proves what a persona security role GRANTS, not just that it
13+
exists.** The `role` check only asserted a role row carrying the SDK ownership
14+
marker, so a role built with the wrong access — or one whose privilege write
15+
failed after the row landed — verified clean. The new `role-privileges` check
16+
resolves every declared `(entity, access)` to its Dataverse `PrivilegeId` from
17+
the same metadata source the SDK writes against, and asserts the role holds it
18+
at **at least** the declared depth. A **subset** check by design: extra
19+
privileges are never a finding, because `appAccess` injects `appmodule` read,
20+
unioned jobs escalate a shared entity+access to the max declared scope, and
21+
distinct entities can share one Dataverse privilege. Fails **closed** on an
22+
unreadable role or table. Reader-gated, so existence-only callers are unchanged.
23+
- **`personas[].jobs[].surfaces[]` is checked instead of documentary.** Each entry
24+
is now resolved against the spec's own views, forms, pages, dashboards, tables
25+
and sitemap titles. `spec-lint` **warns** when a surface matches nothing — a
26+
warning, not an error, because a surface may legitimately name an out-of-the-box
27+
artifact this spec never authors. `verify` adds a `job-surface` rollup that
28+
reports a deployed failure as the job it broke ("persona P can no longer do job
29+
J"), rather than only "view X is missing".
1130
- **Automatic plugin update notice.** Every user-invocable skill now runs the
1231
non-blocking `scripts/check-version.js` preflight, which compares the installed
1332
Model Apps version with `origin/main` and shows update commands for the active

plugins/model-apps/docs/app-builder-roadmap.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,21 @@ Source: `IMPROVEMENTS-07-15-app-builder.md` (Project Management V1/V2 diff + a s
234234
- 🔲 **Spec templates** — domain starters (support desk, CRM, asset tracking) as one-shot scaffolds.
235235

236236
### Phase: Quality & docs
237+
-**Security-role and jobs-to-be-done verification (2026-08-14)** — both **metadata-only**:
238+
- **`verify` now proves what a persona role GRANTS, not just that it exists.** The `role` check
239+
only asserted a row carrying the SDK marker, so a role built with the wrong access — or one whose
240+
privilege write failed after the row landed — verified clean. The new `role-privileges` check
241+
resolves each declared `(entity, access)` to its Dataverse `PrivilegeId` from the **same
242+
metadata source the SDK writes against** and asserts the role holds it at **at least** the
243+
declared depth. Deliberately a **subset** check (`lib/role-privileges.js` explains why equality
244+
would false-fail on `appAccess` injection, max-scope union, and shared privileges), and
245+
**fail-closed** on an unreadable role or table.
246+
- **`surfaces[]` is no longer documentary.** `lib/surface-resolver.js` resolves every
247+
`personas[].jobs[].surfaces[]` entry against the spec's own views/forms/pages/dashboards/tables
248+
/sitemap titles; `spec-lint` **warns** when one matches nothing (a warning, not an error — a
249+
surface may legitimately name an OOB artifact), and `verify` adds a `job-surface` rollup that
250+
reports a *deployed* failure as the job it broke ("persona P can no longer do job J") rather
251+
than only "view X is missing".
237252
-**Sample-run UX fixes (2026-07-27, from a live Property-Listings build).**
238253
-**#1 live build status.** A long build now writes `<workspace>/.maker-workspace/build-status.json` (a single-object snapshot — `state`/`steps`/`lastPhase`/`lastLabel` — overwritten every step) alongside the `build-log.jsonl` trace, and prints a `▸ live progress:` path at start. So a multi-minute build is observable even when the launching shell buffers stdout. SKILL.md now tells the agent to stream (Tee, not Select-Object) and read the status file.
239254
-**#2 wireframes shown.** SKILL.md Phase-1 preview step now REQUIRES pasting the `preview-app.js`/`preview-form.js` wireframe output to the user (not summarizing "looks right") before the approval gate — the user must see the forms/sitemap/pages they approve.

plugins/model-apps/references/app-spec-schema.md

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -605,7 +605,7 @@ privilege removes it — the role converges to the spec).
605605
**Field reference**
606606
- `persona` (**required**) — the security role's display name; also its idempotency key. Must be unique across `personas[]`.
607607
- `jobs[]` (**required**, ≥1) — `{ name, description?, surfaces?, privileges[] }`. `privileges[]` is required and non-empty per job.
608-
- `jobs[].surfaces[]` (optional) — the view/form/page names (or page `key`s) that let this persona **do** the job. Documentary only: it is never applied to Dataverse. It renders the jobs→surfaces traceability table in `model-app-plan.md`, and a job with no `surfaces[]` is flagged by `spec-lint.js` as a design gapnothing in the app demonstrably lets that persona do that job.
608+
- `jobs[].surfaces[]` (optional) — the view/form/page names (or page `key`s) that let this persona **do** the job. Never applied to Dataverse. It renders the jobs→surfaces traceability table in `model-app-plan.md`; a job with no `surfaces[]` is flagged by `spec-lint.js` as a design gap, and a surface that **matches nothing this spec builds** is flagged too (`lib/surface-resolver.js` resolves each entry against `views[]` / `forms[]` / `pages[]` (key **or** name) / `dashboards[]` / `entities[]` / sitemap subarea titles, case-insensitively). Both are **warnings**, never errors — a surface may legitimately name an out-of-the-box artifact this spec does not author. `verify-model-app` additionally rolls a *deployed* failure up to the job that depended on it (`job-surface`), so "view X is missing" also reads as "persona P can no longer do job J".
609609
- `privileges[].entity` (**required**) — a table **logical name** (e.g. `account`, `msdyn_workorder`). May be a table this spec doesn't author (standard/system tables are common); existence is resolved against live metadata by the build, not at lint time.
610610
- `privileges[].access` (**required**) — one or more of `read · create · write · delete · append · appendTo · assign · share`.
611611
- `privileges[].scope` (optional, default `user`) — `user` (Basic) · `businessUnit` (Local) · `parentChild` (Deep) · `organization` (Global), least→most permissive.
@@ -627,6 +627,15 @@ of the **same name in the same business unit** share one role by design (the sec
627627
first's) — give personas distinct names, or a distinct `businessUnitId`, if you need separate roles. In
628628
`--changed-only` mode a persona change forces a **full build** (there is no partial security apply yet).
629629
630+
**Verification.** `verify-model-app` proves the role **row** exists carrying the SDK ownership marker
631+
(`role`) *and* — when the reader supplies role/entity privilege access — that the role actually
632+
**grants** every declared privilege at **at least** the declared depth (`role-privileges`). The depth
633+
comparison is a **subset** check by design: extra privileges are never a finding, because `appAccess`
634+
injects `appmodule` read, unioned jobs escalate a shared entity+access to the max declared scope, and
635+
distinct entities can share one Dataverse privilege (a role holds one depth per privilege). It fails
636+
**closed** — an unreadable role, or a table whose privilege metadata cannot be read, is reported
637+
rather than skipped.
638+
630639
**Validation rules** (`validateAppSpec`): `persona` required + unique; each job needs a `name` and a
631640
non-empty `privileges[]`; `access` values and `scope` must be valid tokens; `appAccess` must be a
632641
boolean; `businessUnitId` and `assignTo` ids must be GUIDs. Two apply-time checks need live metadata and
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
// plugins/model-apps/scripts/lib/role-privileges.js
2+
// PURE: what privileges a persona's security role MUST hold, and whether a deployed role holds them.
3+
//
4+
// WHY this exists. `verifySpec`'s role check proved only that a role ROW exists carrying the SDK
5+
// ownership marker. It never looked at the role's privileges — so a role created with the wrong
6+
// access, or one whose privilege write silently failed after the row was created, verified clean.
7+
// That is a metadata read, which is why it is cheap enough to run on every verify.
8+
//// SUBSET, not equality. We assert the role holds AT LEAST every declared privilege at AT LEAST the
9+
// declared depth. Extra privileges are never a finding, and that is deliberate — three legitimate
10+
// sources add privileges the spec does not literally list:
11+
// 1. `appAccess` injects `appmodule` read (see personaRoleSpecFor).
12+
// 2. Several jobs unioned together escalate a shared entity+access to the MAX declared scope.
13+
// 3. Distinct entities can share ONE Dataverse privilege, and a role holds one depth per
14+
// privilege — so the SDK raises that privilege to the highest scope any of them asked for.
15+
// An equality check would fail on all three while telling us nothing true.
16+
//
17+
// Dataverse reference — privilege depth (`Depth` on ReplacePrivilegesRole, `RolePrivilegeDepth`):
18+
// Basic (user) < Local (business unit) < Deep (parent/child) < Global (organization).
19+
// https://learn.microsoft.com/en-us/power-apps/developer/data-platform/security-model
20+
'use strict';
21+
22+
// App Spec scope -> Dataverse depth name, and its ORDER. The order is what makes this a subset
23+
// check: a role holding Global satisfies a declared Basic. Mirrors the vendored SDK's own mapping
24+
// (verified against scripts/vendor/cds-maker-sdk.cjs) so a comparison cannot disagree with the write.
25+
const SCOPE_DEPTH = { user: 'Basic', businessUnit: 'Local', parentChild: 'Deep', organization: 'Global' };
26+
const DEPTH_RANK = { basic: 1, local: 2, deep: 3, global: 4 };
27+
// App Spec access token -> Dataverse PrivilegeType, again mirroring the SDK.
28+
const ACCESS_TYPE = { read: 'Read', create: 'Create', write: 'Write', delete: 'Delete', append: 'Append', appendTo: 'AppendTo', assign: 'Assign', share: 'Share' };
29+
30+
const rankOf = (depth) => DEPTH_RANK[String(depth == null ? '' : depth).trim().toLowerCase()] || 0;
31+
32+
// Flatten a persona to the (entity, access, scope) triples its role must satisfy, taking the MAX
33+
// scope per (entity, access) exactly as the builder's union does. `appAccess` is folded in here so
34+
// the expectation matches what the build actually writes rather than what the author typed.
35+
function declaredPrivileges(persona) {
36+
const byKey = new Map(); // "<entity>|<access>" -> { entity, access, scope }
37+
const addAll = (list) => {
38+
for (const pr of list || []) {
39+
if (!pr || !pr.entity) continue;
40+
const entity = String(pr.entity).trim().toLowerCase();
41+
const scope = pr.scope || 'user';
42+
for (const a of pr.access || []) {
43+
const access = String(a).trim();
44+
if (!access) continue;
45+
const key = `${entity}|${access.toLowerCase()}`;
46+
const prev = byKey.get(key);
47+
// Max scope wins — the same rule the builder applies when unioning jobs into one role.
48+
if (!prev || rankOf(SCOPE_DEPTH[scope]) > rankOf(SCOPE_DEPTH[prev.scope])) byKey.set(key, { entity, access, scope });
49+
}
50+
}
51+
};
52+
for (const j of (persona && persona.jobs) || []) addAll(j && j.privileges);
53+
addAll(persona && persona.additionalPrivileges);
54+
// Mirrors personaRoleSpecFor: unless the persona opts out, the build grants appmodule read so the
55+
// app actually opens for them. Verifying it matters — without it the role exists but the app does not.
56+
if (!persona || persona.appAccess !== false) addAll([{ entity: 'appmodule', access: ['read'], scope: 'organization' }]);
57+
return [...byKey.values()];
58+
}
59+
60+
// Compare declared privileges against what the role actually holds.
61+
// `entityPrivileges`: Map<entityLogical, [{ Name, PrivilegeId, PrivilegeType }]> — from
62+
// EntityDefinitions(LogicalName='x')?$select=Privileges, the SAME source the
63+
// SDK resolves against when it writes.
64+
// `actualByPrivilegeId`: Map<privilegeId(lowercased), depthName>
65+
// Returns { ok, missing:[{ entity, access, scope, reason, privilegeName? }] }.
66+
// An entity whose metadata could not be read is reported as a finding, never skipped — a read
67+
// failure must not read as "nothing missing" (fail closed).
68+
function compareRolePrivileges(declared, entityPrivileges, actualByPrivilegeId) {
69+
const missing = [];
70+
for (const d of declared) {
71+
const privs = entityPrivileges.get(d.entity);
72+
if (!privs) {
73+
missing.push({ ...d, reason: `could not read privilege metadata for '${d.entity}'` });
74+
continue;
75+
}
76+
const type = ACCESS_TYPE[d.access.toLowerCase()];
77+
const p = type && privs.find((x) => x && x.PrivilegeType === type);
78+
if (!p) {
79+
missing.push({ ...d, reason: `'${d.entity}' exposes no '${d.access}' privilege` });
80+
continue;
81+
}
82+
const held = actualByPrivilegeId.get(String(p.PrivilegeId || '').trim().toLowerCase());
83+
if (!held) {
84+
missing.push({ ...d, privilegeName: p.Name, reason: `role does not hold ${p.Name}` });
85+
continue;
86+
}
87+
const want = SCOPE_DEPTH[d.scope] || 'Basic';
88+
if (rankOf(held) < rankOf(want)) {
89+
missing.push({ ...d, privilegeName: p.Name, reason: `role holds ${p.Name} at ${held}, below the declared ${want}` });
90+
}
91+
}
92+
return { ok: missing.length === 0, missing };
93+
}
94+
95+
module.exports = { declaredPrivileges, compareRolePrivileges, SCOPE_DEPTH, ACCESS_TYPE, DEPTH_RANK };

plugins/model-apps/scripts/lib/spec-lint.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
// relationship schema-name vs lookup-name collision Dataverse rejects.
55
const { relationshipSchemaName, relationshipFor, invalidChoiceSampleTokens, isPlatformIconRef } = require('./app-spec.js');
66
const { normalizeSpecShape } = require('./spec-shape.js');
7+
const { resolveSurfaces, unresolvedSurfaceMessage } = require('./surface-resolver.js');
78

89
const CHOICE_OPTION_WARN = 12;
910
const SEQNUM_RE = /\{SEQNUM(:\d+)?\}/i;
@@ -390,6 +391,11 @@ function lintAppSpec(spec) {
390391
W(`persona "${persona}" job "${job.name}" is not mapped to a surface (jobs[].surfaces[]) — nothing in this app demonstrably lets that persona do the job.`);
391392
}
392393
}
394+
// A surface that names nothing this spec builds is the NEXT failure after "no surfaces at all":
395+
// the job claims coverage that does not exist. Only a warning, because a surface may legitimately
396+
// name an out-of-the-box artifact this spec never authors (the same reason app-spec.js validates
397+
// surfaces as shape-only) — see lib/surface-resolver.js.
398+
for (const u of resolveSurfaces(spec).unresolved) W(unresolvedSurfaceMessage(u));
393399
if (!(Array.isArray(spec.pages) && spec.pages.length)) {
394400
W('no pages[] — per the genpage-first policy, non-record surfaces (overview/landing, dashboard, analytics, guided or wizard flows) should be generative pages. If this app is genuinely record-CRUD only, ignore this.');
395401
}

0 commit comments

Comments
 (0)