feat(model-apps): probe what each persona can actually DO, as that persona

[Akshay Maloo (akshaymaloo)]

Ready to review — #425 has merged, so this is now based on main and is a standalone 3-commit change. It depends on lib/role-privileges.js, which #425 delivered.

Rebuilt on main after #425 was squash-merged: the branch previously carried #425's commits, which conflicted with the squashed version now on main. Only the three probe commits remain — the model-apps content is byte-identical to what was reviewed.

The gap this closes

#425 added a role-privileges check that proves a deployed security role holds the privileges the spec declares. That is a metadata comparison, and it stops there.

Whether the persona can actually perform the operation additionally depends on record ownership, business-unit placement, team membership, sharing, and server-side plug-ins — none of which appear in roleprivileges. So a role can verify perfectly clean and still leave the persona unable to do their job.

probe-persona.js executes real reads as the persona and reports what actually happens.

Why this needs no human, and no new auth

Dataverse impersonation. Effective privileges are the intersection of caller and target, so a System Administrator driving the probe cannot mask a permission the persona lacks.

Two findings made this far cheaper than expected:

1. personas[].assignTo.users[] already holds systemuserid GUIDs — exactly what the legacy MSCRMCallerID header takes. So the common case needs no directory lookup and no application user. It upgrades to the documented-preferred CallerObjectId when azureactivedirectoryobjectid is readable.
2. The existing transport already carries a per-call header — verified empirically rather than assumed: Authorization, Accept, OData-MaxVersion, OData-Version, CallerObjectId, with auth intact. No change to the http client.

Caller needs prvActOnBehalfOfAnotherUser, assigned directly (a team-inherited grant does not satisfy it). Docs

Three decisions carry the design

1. It probes the NEGATIVE direction. For each persona it reads an entity that another persona declares and this one does not. An over-broad role is invisible from the inside — every operation the user tries succeeds — so it can only be found by trying something that should fail. appmodule is excluded, because the build injects it for every persona and it would fail on every run.

2. An empty 200 on a negative probe is inconclusive, never a pass. Dataverse answers "no privilege" with 403 but "narrower scope" with a filtered 200, which is indistinguishable from an authorized read of an empty table. Reporting that as a pass would manufacture confidence in the one direction that matters. Inconclusive results do not fail the run — they are genuine unknowns, and failing on them would train the operator to ignore the tool — but they are always counted and listed, so an all-inconclusive run cannot masquerade as clean.

3. A WhoAmI canary runs first. The dangerous failure is not a 403, which is loud. It is the header being accepted and silently ignored: every probe would then run as the signed-in admin, every allow-probe would pass, and the report would look authoritative while proving nothing. WhoAmI returns the effective user id, so comparing it against the impersonated id catches that. A 403 there reports the real cause rather than a wall of role findings.

Safety

• Read-only by default. --allow-mutations only plans write probes; it does not execute them. Writing to someone's environment in order to verify it deserves its own explicit design.
• Every un-runnable probe (no test user, unresolvable entity set, transport error) degrades to inconclusive, never to a role failure — otherwise the operator is sent to fix a security role when the real problem is a missing test user.

Scope limit — stated so results are not over-read

This exercises the Web API. It says nothing about UCI navigation, form or control visibility, client-side script, the command bar, layout, or accessibility. A green run means the data operations are authorized — not that the app works. That limit is recorded in the script header, AGENTS.md, the CHANGELOG and the skill entry, not just here.

Placement

role-privileges stays in verify; this sits beside it as an opt-in step in app-builder Phase 3. They are different questions at different costs: verify is free, binary, prerequisite-free and belongs in the build gate; the probe costs N × M round trips, needs a test principal, and can legitimately answer "inconclusive" — which cannot serve a gate that must exit zero or non-zero. Moving role-privileges out would restore exactly the hole #425 closes.

Invocation is a script call, not a skill-to-skill call — the plugin forbids the latter (AGENTS.md), while shared scripts are the intended reuse path.

Verification

• 1511 plugin tests (1480 → 1511; 31 new), 0 fail
• Orchestration lives behind injected IO (mirroring readerFor/verifySpec), so principal resolution, entity-set resolution, caching and error containment are all tested without a network
• Tests cover the load-bearing judgements directly: the empty-200 inconclusive rule, the silently-ignored-header canary, 403 → missing-privilege message, and that appmodule is never probed negatively
• Docs updated per the AGENTS.md map: file tree, script section, CHANGELOG, skill entry

[Copilot]

Pull request overview

Adds an opt-in “probe persona” capability to the model-apps app-builder plugin that executes real Dataverse read operations under impersonation to validate what each persona can actually do (including negative probes to detect over-broad roles), complementing the existing metadata-only role-privileges verification.

Changes:

• Introduces scripts/probe-persona.js plus a pure planning/execution library scripts/lib/persona-probe.js to run allow/deny authorization probes via Dataverse impersonation.
• Adds a dedicated test suite covering probe planning, outcome interpretation, execution caching, and the impersonation (WhoAmI) canary.
• Documents the new optional step in the app-builder skill docs, plugin AGENTS map, and CHANGELOG.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
plugins/model-apps/skills/app-builder/SKILL.md	Documents the optional probe-persona.js invocation and its limitations.
plugins/model-apps/scripts/tests/verify-model-app.test.js	Fixes a couple comment formatting issues in an existing test.
plugins/model-apps/scripts/tests/persona-probe.test.js	Adds tests for probe planning/execution logic and CLI canary seams.
plugins/model-apps/scripts/probe-persona.js	New CLI that provisions SDK/http client, validates spec, runs WhoAmI canary, and executes probes.
plugins/model-apps/scripts/lib/persona-probe.js	New pure module: plan allow/deny probes, interpret outcomes, summarize results, execute probes with injected IO.
plugins/model-apps/CHANGELOG.md	Announces probe-persona.js and explains the key design decisions/limitations.
plugins/model-apps/AGENTS.md	Adds the new script/library to the documented script map and usage examples.

Suppressed comments (2)

plugins/model-apps/scripts/probe-persona.js:88

• makePrincipalResolver() drops the persona’s systemuserid when it upgrades to CallerObjectId. That makes it impossible for the WhoAmI canary to validate the effective user id (and avoid false negatives when the test user matches the signed-in user). Include the systemuserid alongside the header/value so the preflight can pass it into checkImpersonation().

    const systemUserId = users.find((u) => typeof u === 'string' && u.trim());
    if (!systemUserId) return null;
    const oid = cache.get(String(systemUserId).toLowerCase());
    return oid
      ? { header: 'CallerObjectId', value: oid }
      : { header: 'MSCRMCallerID', value: String(systemUserId).trim() };

plugins/model-apps/scripts/probe-persona.js:156

• After enhancing checkImpersonation() to accept an expected target systemuserid and potentially return a warning, the preflight call should pass firstPrincipal.systemUserId (when available) and surface any canary warning in the emitted warnings list.

  const pre = await checkImpersonation(httpClient, apiRoot, firstPrincipal.header, firstPrincipal.value);
  if (!pre.ok) { emitResult(false, { ok: false, preflight: pre.detail, warnings }); return; }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (3)

plugins/model-apps/scripts/probe-persona.js:69

• checkImpersonation treats effectiveId === callerId as proof the impersonation header was ignored. That’s a false failure when the selected target user is actually the signed-in user (self-impersonation), which makes the canary unable to prove anything either way but shouldn’t block the run.

  const effectiveId = String((asUser.body && asUser.body.UserId) || '').toLowerCase();
  if (effectiveId && callerId && effectiveId === callerId) {
    return { ok: false, detail: `the ${header} header was accepted but IGNORED — every probe would run as the signed-in admin and report false passes` };
  }

plugins/model-apps/scripts/probe-persona.js:156

• The preflight currently uses the first resolved persona principal from principalFor(...), which can be the signed-in user (common when using yourself as the test user). In that case, checkImpersonation can misdiagnose “header ignored” and block the probe run even though impersonation may be working. Prefer using a known systemuserid (MSCRMCallerID) for the canary, independent of whether probes later use CallerObjectId.

  // Preflight once, on the first persona that actually has a principal. Impersonation is an
  // environment-wide capability, so a second check would cost a round trip and tell us nothing new.
  const firstPrincipal = [...new Set(probes.map((p) => p.persona))].map(principalFor).find(Boolean);
  if (!firstPrincipal) {
    emitResult(false, {

plugins/model-apps/scripts/lib/persona-probe.js:171

• For negative (expect: 'deny') probes, a 404 currently falls into the generic “unexpected status 404” bucket. 404 is common/meaningful (wrong entity-set name, entity removed, metadata mismatch) and it doesn’t indicate an over-broad role; it should be called out explicitly as inconclusive with a clearer message.

  // expect === 'deny'
  if (status === 403) return finding('pass', 'correctly denied (403)');
  if (typeof status === 'number' && status >= 200 && status < 300) {
    if (rowCount > 0) {
      return finding('fail', `readable (${rowCount} row(s) visible) but no job declares this entity — the role is broader than the spec`);

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.