[MPDX-9771] - MPD leader impersonation (frontend)

[wjames111]

Summary

Frontend half of MPDX-9771: lets MPD leaders — any user who coaches an account list — impersonate the staff they coach under a restricted, server-enforced scope, so they can help with MPD forms without seeing contacts, tasks, or contact PII.

Companion API PR (deploy prerequisite): https://github.com/CruGlobal/mpdx_api/pull/3455 — the API mints the scoped JWT (impersonation_scope: 'mpd_leader') and enforces every restriction server-side. This PR wires the scope through the frontend session and adapts the UI.

Permission model (4 tiers)

1. Hidden — Contacts, Tasks, and (non-HR) Tools: nav tabs hidden, routes redirect to the dashboard via blockRestrictedImpersonation, and the API independently blocks the data.
2. Editable — NS Goal Calculator, NSO MPD Questionnaire, Salary Calculator, and MHA requests remain fully editable (matches the API mutation allowlist).
3. Read-only — all other HR tools: enforced by the API's mutation allowlist (non-allowlisted mutations are denied server-side). Per review, this PR adds no frontend read-only affordances — attempted edits surface the API's authorization error.
4. Visible — all reports and the dashboard; contact PII (addresses, emails, phones, donor account numbers) is scrubbed by the API.

Changes

• Session plumbing: GetUserAccess now selects coachingAccountLists(first: 1) { totalCount } and the NextAuth JWT/session derive a coach boolean from it (plus impersonationScope); eligibility is coaching-based, not an admin-granted flag; impersonateHelper stores the API's impersonation_scope in a signed mpdx-handoff.impersonationScope cookie which setUserInfo verifies and folds into the JWT.
• Route guards (pagePropsHelpers): new enforceAdminOrCoach (admin console access for coaches) and blockRestrictedImpersonation (contacts/tasks/tools pages redirect during scoped impersonation); applied to 15 pages + admin.page.tsx.
• Nav: contacts/tasks/tools tabs hidden during scoped impersonation (useNavPages); Admin settings nav item visible to coaches; Reset Account accordion stays admin-only.
• Impersonate entry points: Admin Console (existing form, now accessible to coaches) and a new ImpersonateStaffModal on the MPD Goal Admin table (lazy-loaded via the Dynamic modal pattern, Formik/Yup reason field).
• Restricted-impersonation signal: useRestrictedImpersonation hook + isRestrictedImpersonation server-side predicate (used for nav/route hiding). Per review, the earlier per-mutation frontend guards and disabled states were removed — the API is the enforcement layer for read-only tools.

Testing

• yarn lint, yarn lint:ts clean; full yarn test green (16 suites that failed under parallel load all pass in isolation — known flakiness, verified with --onlyFailures).
• New/updated tests cover session callbacks, route guards, nav hiding, and the impersonate modal.

Deploy notes / follow-up

• Requires CruGlobal/mpdx_api#3455 to be deployed first; without it the API neither issues nor honors impersonation_scope.
• The API denies non-allowlisted mutations by default during scoped impersonation. Background/incidental mutations (e.g. preference writes fired by shared components on reports pages) should be spot-checked on staging while impersonating — no local API run was feasible in this environment.

🤖 Generated with Claude Code

[github-actions]

Bundle sizes [mpdx-react]

Compared against d14c76a

Dynamic import	Size (gzipped)	Diff
src/components/HrTools/MpdGoalAdmin/ImpersonateStaffModal/DynamicImpersonateStaffModal.tsx -> ./ImpersonateStaffModal	1.1 KB	added

[wjames111]

Multi-Agent Code Review — BLOCKERS FOUND

Mode: standard · Agents: 7 specialized reviewers + dependency-impact + 2 gap reviewers, with cross-examination debate · Risk: CRITICAL (10/10 — NextAuth session + impersonation authorization, 61 files)

Verdict: 1 high-priority blocker

The core of the PR is solid — the HMAC-signed scope trust chain is sound, all 15 contacts/tasks/tools sub-pages are correctly blocked, read-only guards are correctly placed with correct dependency arrays, i18n is complete, the dynamic-modal pattern is followed exactly, and the Standards checklist passes across the board. One gap must be fixed before merge (below), plus one strongly-recommended test gap and a handful of medium/suggestion items posted as inline comments.

---

Findings on Related Files (Not in This PR)

[High] pages/accountLists/[accountListId]/tools.page.tsx:46 — Tools landing page is not blocked during restricted impersonation

• Severity: 8.0/10 — BLOCKER (flagged by Dependency Impact, escalated by Security, supported by all 6 debating agents; verified twice at PR head)
• All 15 contacts/tasks/tools sub-pages got blockRestrictedImpersonation, but the /tools landing page still exports getServerSideProps = ensureSessionAndAccountList. The nav tab is hidden, but a bookmarked/typed URL renders the full Tools home during a restricted impersonation session. ToolsHome unconditionally fires GetToolNotifications, which queries the impersonated staffer's contacts/people aggregate counts (partner-status/address/email/phone filters) — exactly the data class the block exists to withhold. This contradicts the commit "Block contacts, tasks, and tools routes during restricted impersonation."
• Fix (one line): swap ensureSessionAndAccountList → blockRestrictedImpersonation in tools.page.tsx, matching its 15 siblings. Consider also asserting the guard in tools.page.test.tsx the way hrTools/mpdGoalAdmin/index.page.test.tsx asserts its getServerSideProps.

(This file is not in the PR diff, so it cannot carry a line comment — but it is counted as a blocker because it is a missing change required by the PR's stated goal: 16 routes in the access matrix, 15 guarded, 1 missed.)

---

Debate resolutions worth knowing

• Canonical scope value: the wire value is impersonation_scope: 'mpd_leader'. mpdSupervisorAdmin is the admin capability flag — a different concept. Test fixtures currently use 'mpd_supervisor' and 'mpd_leader' interchangeably for the scope (harmless today since only truthiness is checked, but worth a shared fixture before any allow-list lands).
• TransfersTable unguarded handlers: debate verified MUI's disabled IconButton genuinely blocks the child-SVG onClick via mouse AND keyboard — so this is a defense-in-depth/consistency gap, not a live bypass (down-revised 6.5 → 6.0 merged finding).
• Server-side enforcement: confirmed posture — the API rejects restricted mutations; all client guards are defense-in-depth.

Pre-existing issues (informational, separate PR)

• 6.0 hrTools/mpdGoalAdmin/scenario/[scenarioGoalId]/index.page.tsx uses bare ensureSessionAndAccountList — no admin gate or restriction block, now more meaningful since mpdGoalAdmin hosts the impersonate action.
• 3.5 impersonateHelper.ts:118 sets handoff cookies even when the token fetch fails (the PR's new scope cookie is correctly guarded).
• Shared Modal lacks explicit aria-labelledby.

Verified clean

Scope trust chain (HMAC-signed cookie, tamper-rejection tested, JWT_SECRET server-only, no token logging) · mpdSupervisorAdmin exists in the schema, yarn gql will pass, id in selection set · guard placement precedes all optimistic state, dependency arrays correct · no financial calculation code touched · i18n byte-identical across all 7 read-only tooltip sites · no debug output / any / new Date() / empty catches.

[wjames111]

🤖 Fixed blocker in b499a65: tools.page.tsx getServerSideProps now uses blockRestrictedImpersonation (matching its 15 sub-page siblings), with a test asserting the guard. Posted by Claude Code via /agent-fix.

[wjames111]

Multi-Agent Code Review — Round 2 (re-review after fixes) — APPROVED WITH SUGGESTIONS

Mode: standard · Agents: 7 specialized reviewers + dependency impact · Risk: CRITICAL (10/10 — auth/impersonation surface; human review always required at this risk level)

Verdict: 0 blockers — all 12 round-1 findings VERIFIED-FIXED

Every round-1 finding, including the 8.0 blocker, was independently re-verified by multiple agents:

• Blocker (tools.page.tsx): now blockRestrictedImpersonation, identical to its 15 siblings, with an identity-assertion test. The 59-page route audit confirms contacts/tasks/tools coverage is complete.
• Shared predicate (src/lib/restrictedImpersonation.ts): fully adopted — whole-tree grep finds zero remaining inline !!impersonationScope checks; both SSR helpers and the client hook route through it; behavior byte-identical; server/client import boundary safe (type-only next-auth import). Net tech debt reduced.
• All 8 test-gap fixes: verified non-vacuous (each asserts the negative against a positive baseline); suites pass.
• Merge of origin/main: no auth-relevant, financial, or structural changes entered the PR diff; codegen and lint:ts now pass cleanly.

Remaining suggestions (non-blocking, inline comments)

1. (5.0) Harden the mockSession leakage mitigation in useAdditionalSalaryRequestForm.test.tsx — reset the session in beforeEach instead of relying on test order.
2. (3.5) ImpersonationScope union is exported but unreferenced — forward-looking by design; optionally give it a live consumer.
3. (3.0) GoalsTable test could also assert View/Edit still renders while impersonating.

Informational / pre-existing (no action in this PR)

• (4.0) [Pre-existing] Two profile menus (ProfileMenu.tsx:364, ProfileMenuPanel.tsx:166) still show the Admin Console link to developer-only users, who get bounced by enforceAdminOrMpdLeader — the same dead-link UX fixed in useSettingsNavItems. Suggest a follow-up ticket to align all three surfaces.
• (3.0) [Pre-existing, from main merge] coaching/[coachingId]/nsGoalCalculator/index.page.tsx uses bare ensureSessionAndAccountList with no impersonation gate — authored on main (commit 9cda1b7), not by this PR. Confirm coaching-scoped calculators are intentionally outside the restricted-impersonation surface.
• (3.0) The ?? false default in the session callback is unreachable per the JWT type but correct as runtime defense for pre-deploy tokens — intentional, keep.
• Known items carried forward: scenario page guard, impersonateHelper failure-path cookies, fixture value 'mpd_supervisor' vs wire value 'mpd_leader' (truthiness-only today).

Standards

Full checklist PASS. Lint, type-check, codegen, and all touched test suites pass (verified by orchestrator).

[github-actions]

Preview branch generated at https://MPDX-9771.d3dytjb8adxkk5.amplifyapp.com

[canac]

It's a lot of code, so I focused on the hotspots and had Claude review the rest. I'm concerned about needing to check the impersonation scope in so many parts of the application and in lots of new code. That seems like a maintenance nightmare, so I'd like to think through alternatives before committing to that route.

The server blocks mutations, right? So what do you think allowing mutations, know that the server will reject them? We can just make sure that the server returns an understandable error message like, "Changes are not allowed during impersonation".

[canac]

It's going to be annoying and error-prone to have to check this before every mutation in our application.

[wjames111]

I think your suggestion of just relying on the back-end to block it works better.

[wjames111]

🤖 Fixed in 5a88882: removed the per-mutation frontend guards and disabled states from the HR tools — the API's mutation allowlist is now the sole enforcement layer. Nav/route hiding for contacts/tasks/tools remains.

Posted by Claude Code.

[canac]

If the frontend needs to check for one more role (4+), I'd consider making this an enum.

[wjames111]

Agreed — the plan is to finish the role column work in CruGlobal/mpdx_api#3285, migrate admins/developers/MPD supervisors to it, and have the frontend read a single role value instead of these booleans. Leaving the booleans as-is here so the swap happens once, in the role-migration PR.

Posted by Claude Code.

[canac]

Is this right? Why are we removing developer?

[wjames111]

Thanks for catching this!

[wjames111]

🤖 Fixed in 9ac10be: restored 'developer' in the Admin Console grantedAccess and admitted developers in enforceAdminOrMpdLeader so the nav item and page guard agree.

Posted by Claude Code.

[wjames111]

@canac allowing mutations works for simplifying things a little but. but we still have the PII leaking everywhere across the app, so either way it's kind of a nightmare. Could we just give them access to SAA? Architecturally this is a really bad request

[canac]

but we still have the PII leaking everywhere across the app, so either way it's kind of a nightmare.

Doesn't your backend PR redact PII? Or are you saying there are more fields that could include PII?