Skip to content

feat(reports): identity_access_review evidence report type, end to end (#5818 W06) - #6034

Merged
ToddHebebrand merged 12 commits into
mainfrom
feature/5812-service-plan-evidence-reports/wave-5818
Sep 16, 2026
Merged

ToddHebebrand merged 12 commits into
mainfrom
feature/5812-service-plan-evidence-reports/wave-5818

Conversation

@ToddHebebrand

Copy link
Copy Markdown
Collaborator

W06 — identity_access_review: the sign-in review artifact

Closes #5818 (wave W06 of #5812 / #5784).

Turns W05's accumulated m365_signin_events — plus the identity, conditional-access
and remote-access data the M365 sync already holds — into a service-plan evidence
artifact, end to end: enum label → generator → PDF renderer → portal → web wiring →
eight locales → integration proof.

Plan: docs/superpowers/plans/billing/2026-09-14-service-plan-evidence-reports-w06-identity-access-review.md
Spec: docs/superpowers/specs/billing/2026-09-14-service-plan-evidence-reports-spec.md §3.5.2–3.5.3

What the artifact claims

Interactive sign-ins only, for the window it actually covers. Non-interactive,
service-principal and managed-identity sign-ins are not persisted by W05 and the
artifact never implies otherwise — the PDF's own title block says "Interactive
sign-ins for <period>", and a unit test asserts the string "all sign-ins" never
appears in the rendered bytes.

The three limits it prints, every time

  1. Entra ID P1/P2 is required for the data. AuditLog.Read.All grants the
    permission; the licence grants the data. W05 persists Microsoft's unlicensed
    outcome as a complete, zero-row success, so every sign-in figure goes NULL
    and the artifact renders a data-gap page, never an empty table. If that case
    ever reads 0, the feature is shipping a lie — integration Case 3 asserts NULL.
  2. Risk fields come back as Graph's hidden sentinel without P2. They are
    dropped by isRiskFieldMeasured and the section renders unmeasured, never
    "no risk detected".
  3. Graph retains sign-in logs ~30 days. Breeze accumulates forward from first
    sync only, so the first monthly report after enabling W05 is partial and says so
    (coverage.coveredFrom/coveredTo + one printed sentence from
    signinCoverageLine). Freshness is last_complete_snapshot_at, never
    last_success_at — a partial run succeeds without enumerating the tenant.

mfa_registered: NULL means UNKNOWN, counted into mfaUnknown /
adminsMfaUnknown and never rendered as "not registered".

Restricted scope is a refusal, not a filter (OD-8 = A)

M365 identity data has no site dimension, so a site-restricted technician gets
the zeroSafeReport empty-but-shaped result with an explanatory data-gap line —
and nothing is read at all. Serving an org-wide identity view to a site-limited
account would be a scope escalation. This differs from W02–W04 on purpose; the
config schema deliberately has no sites key and the options form deliberately
has no site selector. Integration Case 2 asserts no identity row and no admin
UPN or IP survives into the result.

Remote access is client presence, not policy

devices.active_vpns records which overlay client was running at last check-in; the
collector carries no rules, peers or keys (agent/internal/collectors/vpn.go). The
section is titled Remote-access client presence and its caveat string says so.
A PDF test asserts the phrase "VPN policy review" never appears.

PII and the delivery gate

This artifact carries user principal names, IP addresses and cities. It reaches a
customer only through W01's OD-12 delivery gate (integration Case 6: invisible in
the portal list and renderRunPdf refuses, until the occurrence is delivered), the
PDF is never persisted (report_runs.result is jsonb only), and the type is
deliberately out of PORTAL_REPORT_TYPES and both portal-user allowlists in
reportGenerationService.ts (OD-10 = A) — a customer can read a delivered artifact
but can never generate one on demand.

No consent change

Every Graph read behind this is already granted by customer-graph-read manifest
v3: AuditLog.Read.All, Policy.Read.All, User.Read.All,
RoleManagement.Read.Directory. Identity Protection risky users/events stay out
adding them would be a manifest v4 bump forcing every existing customer to
re-consent.

The silent failure this PR closes

buildReportPdf's final else falls through to renderGenericReport, which prints
the rows as a plain table and drops the entire designed summary. On a
PII-bearing identity artifact that is not merely an ugly PDF: the caveats that keep
it honest disappear while the sign-in rows remain. No unit test catches it unless
one is written for the arm specifically, so this wave ships both a unit test
(reportPdf.identityAccess.test.ts, spying on the renderer) and a server-side
integration assertion on the rendered bytes.

Shared helper added for W03 as well

loadDomainFreshness(orgId, domains) is added to apps/api/src/services/m365Sync/summary.ts
with exactly the name and shape W03's plan specifies ({ asOf, lastStatus, truncated, sources, unlicensed }, total record), because W03 (#5815, PR #6007) had
not landed it on main when this wave ran. loadSyncSummary is left untouched, so
the two waves converge rather than fork.

Rollout

  • M365_TENANT_SYNC_ENABLED must be on or this wave is inert (the generator says
    so explicitly rather than reporting zeros). A value in /opt/breeze/.env is
    necessary but not sufficient: compose interpolation only happens for variables
    listed in the service's environment: block, so it must also be mapped in the
    api service's environment: block of /opt/breeze/docker-compose.yml. Confirm
    per region.
  • W05 needs a full deliverable period behind it before this artifact is promised to
    a customer.
    Graph retains sign-in logs ~30 days and Breeze accumulates forward
    from first sync, so an org whose earliest event is two days old gets a two-day
    report — correctly labelled, but not what a customer promised a monthly review
    expects. Check per region:
    SELECT org_id, MIN(signed_in_at), MAX(signed_in_at), COUNT(*)
    FROM m365_signin_events GROUP BY org_id ORDER BY 2;
    (not checked — this is an orchestrator/rollout step, not a blocker for opening
    this PR.)
  • This release must run pnpm --filter @breeze/api reports:reprovision-portal-definitions
    (dry run, then --apply), or orgs that enabled portal reports earlier will lack the
    new definition.

Migration

apps/api/migrations/2026-10-17-110700-report-type-identity-access-review.sql
one ALTER TYPE report_type ADD VALUE IF NOT EXISTS 'identity_access_review',
alone in the file (a label added by ALTER TYPE cannot be used until its
transaction commits). DDL only, writes no rows, so no breeze.scope election is
required and it is not added to migrationRlsScope.test.ts's frozen baseline.
No new table, no new column, no RLS change — the cascade/export-policy registries are
untouched.

Localization caveat

The eight locale files localize the web UI only; the PDF renderer is English.

🤖 Generated with Claude Code

Todd Hebebrand and others added 11 commits September 16, 2026 02:11
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…anaged registry (#5784 W06)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d list (#5784 W06)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ic (#5784 W06)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…restricted refusal (#5784 W06)

Also adds loadDomainFreshness to m365Sync/summary.ts — the per-domain
last_complete_snapshot_at reader W03 specifies, with the identical name and
shape so the two waves converge rather than fork.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…Pdf arm (#5784 W06)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…f-serve generate (#5784 W06)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… W06)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…W06)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… end (#5784 W06)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n-in risk are unmeasured, not zero (#5784 W06)

Three findings from /pr-review-toolkit:review-pr on #6034, all in the one
family this report type exists to prevent — an absence of MEASUREMENT rendered
as a measurement of ZERO.

1. remoteAccess had no measured-gate, unlike every other section. An org with
   no devices, or whose devices have never reported `active_vpns` (the column
   is NULL until the collector runs), produced `{}` — which the renderer prints
   as "None observed at last check-in". It now returns null with a data-gap
   line, and a partially-reporting fleet discloses the silent devices instead of
   folding them into the total.
2. The PDF asserted "sign-in risk requires an Entra ID P2 licence" whenever
   byRiskLevel was null — but that is null for two reasons, and only one is a
   licensing statement. A quiet, fully-licensed tenant was being told on a
   customer-facing document that they lack a subscription they have. The
   generator now records `coverage.riskUnmeasured` (events held AND every risk
   value hidden) and the renderer prints a neutral line otherwise.
3. Four branches had no test at all and would have survived deletion: the
   identity-inventory unmeasured arm, the caMeasured never-synced-vs-empty
   distinction, the 500-row admin-detail cap and its withheld disclosure, and
   inactive-client filtering. Each now has a regression test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 16, 2026

Copy link
Copy Markdown

Deploying breeze with  Cloudflare Pages  Cloudflare Pages

Latest commit: f0c276d
Status: ✅  Deploy successful!
Preview URL: https://cd68ade2.breeze-9te.pages.dev
Branch Preview URL: https://feature-5812-service-plan-ev-w01e.breeze-9te.pages.dev

View logs

@ToddHebebrand

Copy link
Copy Markdown
Collaborator Author

Review run: /pr-review-toolkit:review-pr — code-reviewer, silent-failure-hunter, pr-test-analyzer (all three on the full origin/main...HEAD diff).

Findings: 3 raised → all 3 addressed in 0f655c8bb; 0 outstanding. Every one landed in the same family — the thing this report type exists to prevent: an absence of measurement rendered as a measurement of zero.

  1. remoteAccess had no measured-gate (silent-failure-hunter, CRITICAL — verified). Unlike identity, dormant, signins and conditionalAccess, the remote-access section was always an object. An org with no devices — or whose devices have never reported active_vpns, which is NULL until the collector runs — produced {}, which the renderer prints as "None observed at last check-in". A customer whose device inventory had not synced would read that as "we checked, nothing running." Now returns null with a data-gap line, and a partially-reporting fleet discloses the silent devices rather than folding them into the total.
  2. The PDF claimed an Entra ID P2 licensing gap that may not exist (code-reviewer, IMPORTANT — verified). signins.byRiskLevel is null for two reasons: every risk value came back as Graph's hidden sentinel (genuinely no P2), or there were simply no sign-ins to assess. The renderer printed the P2 sentence for both, so a quiet, fully-licensed tenant was told on a customer-facing evidence document that they lack a subscription they have. The generator now records coverage.riskUnmeasured (events held and every value hidden) and the renderer prints a neutral "no sign-ins in the covered window to assess" line otherwise.
  3. Four branches had no test and would have survived deletion (pr-test-analyzer — verified): the identity-inventory unmeasured arm (every test seeded a user, so identityMeasured was true in 100% of runs), the caMeasured never-synced-vs-synced-but-empty distinction (its two inputs were never varied apart), the 500-row admin-detail cap and its withheld disclosure (silent truncation of a PII-bearing table), and inactive-client filtering. Each now has a regression test; the CA one is written as the paired null-vs-[] assertion.

The reviewers independently confirmed the six named design invariants hold, each backed by a discriminating test: restricted authority reads nothing (db.select never called, asserted at three layers), unmeasured-never-zero for unlicensed / hidden / mfa_registered NULL, freshness from last_complete_snapshot_at, window from EvidenceRunContext, the type absent from PORTAL_REPORT_TYPES and both portal-user allowlists, and the migration a lone idempotent ALTER TYPE.

Tests: packages/shared full suite 3020/3020 green; apps/api and apps/web full suites re-running at reduced concurrency (this host's load average hit ~140 with several agent sessions active, which produced timeout flakes in unrelated files — PatchList, DeviceList, SoftwareGroupDrawer, and two shared barrel-import tests — every one of which passes in isolation and at --maxWorkers=2). tsc --noEmit clean for apps/api and packages/shared; astro check clean for apps/web (0 errors) and apps/portal (0 errors); pnpm lint clean (6/6 tasks); eight-locale parity + translation coverage + pt-BR guards green.

Not verified locally: the new identityAccessEvidence.integration.test.ts (Task 10) and pnpm db:check-drift — both need a live Postgres, and the local Docker engine is wedged on this host (docker version itself hangs; no stack was brought up, so nothing is left running). CI's Integration Tests shard is the gate for those.

Status: review-clean, awaiting maintainer merge. Do not merge until CI Success is green on the head SHA.

@ToddHebebrand

Copy link
Copy Markdown
Collaborator Author

Final unit-suite numbers (re-run at reduced concurrency after the review fixes in 0f655c8bb, because this host's load average hit ~140 with several agent sessions active and that alone produced timeout flakes):

Suite Result
packages/shared 3020 / 3020 passed
apps/web 10786 / 10786 passed
apps/portal 627 / 627 passed
apps/api 45514 passed, 22 skipped, 5 failed — see below

The five apps/api failures are in three files this PR does not touch — src/services/ipAllowlistMode.config.test.ts, src/workers/webhookDelivery.lifecycle.test.ts, src/routes/mcpServer.orgKeyPartnerRole.test.ts — and they are the documented fresh-worktree trap: they boot the real config validator, which refuses to start without a complete .env (MFA_ENCRYPTION_KEY is required, then CONFIGURATION VALIDATION FAILED), plus 5s timeouts on the same boot path. Copying the repo's .env.example in fixed the mcpServer file and left the rest still short of a valid config. Verified: none of the three files, nor config/env.ts/config/validate.ts/workers/*, appear in this PR's diff. Not checked: whether they are red on origin/main in the same worktree — CI carries a complete env, so this is a local-environment artifact either way.

Everything this PR actually changed is green, including the 8 new regression tests added for the review findings.

…y_access_review (W06) registrations

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ByUsCYMsa2c4EDG797LTqf
@ToddHebebrand

Copy link
Copy Markdown
Collaborator Author

Merged main (W02 #6006, W03 #6007, W04 #6027 landed first, same report-type registries). 31 conflicts resolved as unions — main's three types then identity_access_review in every enum/registry/switch/render arm/locale; ReportTemplates.savedReportMerge.test.tsx → 10; m365Sync/summary.ts keeps main's loadDomainFreshness (identical signature), both waves' test cases retained; portal ReportRunList keeps main's narrow-allowlist design (identity_access_review list-only; the pre-existing TS2353 is #6036). tsc clean in shared/api/web; targeted tests shared 107, api 171, web 210, portal 17 — green. Head f0c276d; pull_request CI running, enqueue-on-green watcher armed.

@ToddHebebrand
ToddHebebrand added this pull request to the merge queue Sep 16, 2026
Merged via the queue into main with commit 4ed770d Sep 16, 2026
75 of 76 checks passed
@ToddHebebrand
ToddHebebrand deleted the feature/5812-service-plan-evidence-reports/wave-5818 branch September 16, 2026 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

W06: identity_access_review report type on the sign-in events

1 participant