MemberJunction / MemberJunction/MJ
Review the magic-link impersonation/elevation model across all surfaces
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
# Review the magic-link impersonation/elevation model across all surfaces
**Target release: 6.0** (per @AN-BC — this is a design review to be worked in depth, not a patch on #3379).
Follow-up requested by @AN-BC in review of #3379:
> I am ok with including this for now ... **but** I think we need a much more thorough PR and review of the ramifications of this kind of allowed impersonation across all aspects of magic link use as it is both a security concern and an alignment/consistency issue.
#3379 fixes a specific, narrow failure (a scoped anonymous magic-link visitor could not run an agent at all, because their invite role deliberately holds no grants on the AI run entities). It does that by running certain realtime work as the system user *after* session ownership is proven. That is a targeted patch on one surface, not a considered position on impersonation generally — which is what this issue is for.
## Why this needs a deliberate decision
There are now **two independent elevation mechanisms** for magic-link principals, added at different times for different reasons, with different gates and different blast radii. Nothing states which one is canonical, when a new surface should use either, or what the invariants are.
| Mechanism | Location | Gate | Runs as |
|---|---|---|---|
| Widget-guest elevation | `resolveWidgetGuestRunContext` / `elevateUserPayload`, `packages/MJServer/src/realtimeWidget/widgetGuestElevation.ts` | `WidgetGuestContext.WidgetID` (from the `mj_widget_id` claim) | guest principal, agent pinned by the widget, RLS-scoped |
| Scoped-anonymous elevation | `ResolveScopedAnonymousRunUser`, same file (added in #3379) | `IsMagicLinkAnonymous` + `MagicLinkScope.ResourceID`, widget guests excluded | the **system user** |
The second one is the one worth scrutinising: it is a full identity swap to the system user, which by definition has every grant in the deployment.
## What a review should settle
1. **Is system-user substitution the right primitive at all?** The alternative is granting the anonymous invite role exactly the rights it needs on the AI run entities, so nothing is ever impersonated. That is more metadata but strictly less powerful, and it would make RLS the enforcement boundary rather than call-site discipline. #3379 took the substitution route because it needed no migration; that tradeoff should be re-litigated rather than inherited.
2. **What is the invariant, stated once?** #3379 works to "ownership and authorization are always evaluated on the caller; only execution is elevated." That invariant is currently enforced by hand at each call site and documented only in comments. If it is the rule, it should be expressed somewhere it cannot silently rot — a wrapper that takes both identities, or a lint/test that fails when an elevated user reaches a load/permission call.
3. **Which surfaces are in and out, and why?** Today's split was decided case by case. Deliberately elevated: relayed-tool dispatch, co-agent observability writes (create/transcript/tool-turn/usage/finalize), recording uploads. Deliberately **not** elevated: `SaveSessionChannelArtifact`, the server-bridged topology (`WireBridgeRealtimeSession`), the `RunAIAgentResolver` text path (client-supplied agent id — elevating there would let an anonymous caller run arbitrary agents as system). `writeReturningVisitorRecap` (`packages/MJServer/src/agentSessions/SessionManager.ts`) is un-elevated and currently unreachable for this principal, but becomes live the moment a scoped-anon conversation acquires a linked record. Someone should confirm that split is a policy rather than an accident.
4. **Agent authority under elevation.** #3379 (later commit) had to CanRun-gate the session's `allowedAgents` colleague union against the original caller, because `assertCanRunTarget` only ever gated the lead target and base-agent's own per-agent check sees the elevated user. Worth checking whether other permission checks downstream of an elevated run have the same shape.
5. **Attribution and memory scope.** Runs must stay attributable to the visitor even when executed as system (#3379 threads `AttributionUserID` → `userId` for this reason; `MJ: AI Agent Runs.UserID` and context-memory scoping both read it). Any new elevated path needs the same treatment or it silently mis-attributes.
6. **Claim provenance.** `buildMagicLinkSessionUser` (`packages/MJServer/src/context.ts`) derives `IsMagicLinkAnonymous`, `MagicLinkScope`, `WidgetGuestContext`, and `ReturningVisitorContext` from a signature-verified JWT, and the gating claims (`mj_scopes`, `mj_anon`, `mj_widget_id`, `mj_visitor_key`) are server-minted. That is the property the whole design rests on, so it should be an explicit, tested invariant rather than an incidental one.
7. **Audit.** An elevated action currently leaves no distinct audit record beyond a log line. Whether impersonation should be recorded as such is a product decision.
## Scope
Server-side magic-link surfaces: `packages/MJServer/src/realtimeWidget/`, `packages/MJServer/src/resolvers/RealtimeClientSessionResolver.ts`, `packages/MJServer/src/agentSessions/`, and the delegation path in `packages/AI/Agents/src/realtime/realtime-client-session-service.ts`.
Related: #3371 (the original defect), #3379 (the narrow fix).
Contributor guide
Research direction
Start by reading the elevation logic in packages/MJServer/src/realtimeWidget/widgetGuestElevation.ts and the claim construction in packages/MJServer/src/context.ts, then trace the listed realtime, resolver, agent-session, and delegation paths. Compare the current elevated and non-elevated surfaces, including attribution and permission checks. Done means the issue's primitive, invariant, scope, claim provenance, and audit treatment have a documented decision.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- authentication, authorization, backend-api-design, security
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100